Skip to content

docs: add strict bot policies (non-loop mode)#142

Closed
hlin99 wants to merge 2 commits into
mainfrom
docs/strict-policies
Closed

docs: add strict bot policies (non-loop mode)#142
hlin99 wants to merge 2 commits into
mainfrom
docs/strict-policies

Conversation

@hlin99

@hlin99 hlin99 commented Apr 6, 2026

Copy link
Copy Markdown
Member

Apply proxy-level strict review/author rules. Non-loop mode: bot never merges/closes. Direct code review (no design gate).

@hlin99-Review-BotX hlin99-Review-BotX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit e49f5b6. Several issues found:

  1. bot/DESIGN_PRINCIPLES.md is empty — contains only a header (# Design Principles — xPyD-proxy). Both ENTRY.md and AUTHOR_POLICY.md list it as mandatory reading, but it provides no content. Either populate it or remove the references.

  2. Active PR Maintenance section dropped entirely — the old BOT_POLICY.md had a detailed "Active PR Maintenance" section (branch update, CI check, review comment check, re-request review cycle). None of this appears in any of the new files. If this is intentional for "non-loop mode", AUTHOR_POLICY.md should still document the expected maintenance workflow or explicitly state there is none.

  3. Rate limiting guidance removed — the old policy had a "Rate Limiting" section for reviewers. The new REVIEW_POLICY.md has no equivalent. Consider adding it back.

  4. PR description contradicts content — description says "no design gate", but REVIEW_POLICY.md checklist still includes "Design conformance: Implementation must match the linked GitHub Issue design." This is contradictory — either remove the design conformance row or update the PR description.

  5. No linked issue — PR body does not reference any GitHub Issue. Per existing policy, PRs should reference related issues.

@hlin99

hlin99 commented Apr 6, 2026

Copy link
Copy Markdown
Member Author

Addressed all feedback from review:

  1. Populated DESIGN_PRINCIPLES.md with architecture constraints, design rules, and coding guidelines
  2. Added Active PR Maintenance section to AUTHOR_POLICY.md (non-loop mode: triggered, not automatic)
  3. Added Rate Limiting section to REVIEW_POLICY.md
  4. Removed Design conformance row from review checklist (no design gate per PR intent)
  5. Acknowledged — no issue exists for this PR; this is a docs-only restructuring

Fixed in 09489f9.

…141)

Migrated to xPyD-integration repo:
- tests/integration/ (~115 tests)
- tests/stress/ (~10 tests)

Also removed:
- sim_adapter.py (no longer needed)
- tests/conftest.py (only used by integration tests)
- xpyd-sim dev dependency
- Fixed build job CI to run unit tests

Co-authored-by: hlin99 <hlin99@users.noreply.github.com>
@hlin99 hlin99 force-pushed the docs/strict-policies branch from a77202c to 1e12d1c Compare April 6, 2026 14:43

@hlin99-Review-Bot hlin99-Review-Bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 1e12d1c. CI is fully green and the PR is mergeable, but several issues remain:

  1. PR title/description mismatch with actual changes. Title says "docs: add strict bot policies (non-loop mode)" but the only change in this commit vs main is to .github/workflows/trigger-integration.yml. This is a ci: change, not docs:. The description also does not match the diff content.

  2. No linked GitHub Issue. Per BOT_POLICY.md, PRs should reference related issues.

  3. Workflow change review:

    • Adding pull_request trigger to run integration tests on PRs is a reasonable improvement.
    • Switching from GITHUB_TOKEN to INTEGRATION_TOKEN makes sense for cross-repo dispatch (GITHUB_TOKEN lacks permissions for repository_dispatch on other repos).
    • The if/else logic for PR vs release event types is correct.
    • Minor: no permissions: block is specified. Consider adding minimal permissions for security hardening.

Please fix the PR title (should be ci: prefix) and update the description to accurately reflect the actual change.

Split BOT_POLICY.md into modular files under bot/:
- ENTRY.md: overview, navigation, and general rules
- REVIEW_POLICY.md: reviewer bot rules, checklist, rate limiting
- AUTHOR_POLICY.md: author bot rules, active PR maintenance (triggered mode)
- DESIGN_PRINCIPLES.md: architecture constraints and coding guidelines

Addresses review feedback:
- Populated DESIGN_PRINCIPLES.md with real content
- Added Active PR Maintenance section to AUTHOR_POLICY.md (triggered mode)
- Added Rate Limiting section to REVIEW_POLICY.md
- Removed design conformance from review checklist (no design gate)
- Pre-commit formatting fixes (ci.yml EOF, test_metrics.py blank line)

Signed-off-by: hlin99 <tony.lin@intel.com>
@hlin99

hlin99 commented Apr 6, 2026

Copy link
Copy Markdown
Member Author

All 5 items addressed in fe36881:

  1. DESIGN_PRINCIPLES.md populated — added 12 rules: architecture constraints (PD disaggregation, stateless proxy, vLLM CLI compat), design rules (topology-driven routing, health-first scheduling, circuit breaker, config layering), and coding guidelines (type annotations, no bare except, async by default, tests required, minimal deps).

  2. Active PR Maintenance section added to AUTHOR_POLICY.md — documents the full maintenance workflow (update branch, CI check, review comment check, re-request review) explicitly framed for triggered/non-loop mode.

  3. Rate Limiting section added to REVIEW_POLICY.md — 'Respect GitHub API rate limits; back off on 429. Do not flood PRs with duplicate comments or reviews.'

  4. Design conformance row removed from REVIEW_POLICY.md review checklist — aligns with 'no design gate' intent in PR description.

  5. No linked issue — this is a docs-only refactoring PR; no corresponding issue exists.

@hlin99 hlin99 requested a review from hlin99-Review-Bot April 6, 2026 14:49

@hlin99-Review-BotX hlin99-Review-BotX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 1e12d1c. CI is green and PR is mergeable, but several issues must be fixed:

  1. PR title/description mismatch. Title says docs: add strict bot policies (non-loop mode) but the actual changes are removing integration/stress tests, sim_adapter, and xpyd-sim dependency, plus modifying CI workflows. The title prefix should be refactor: or ci: depending on the primary intent. The description also does not reflect the actual diff.

  2. No linked GitHub Issue. Per BOT_POLICY.md, PRs should reference related issues.

  3. Co-authored-by trailer with noreply address. The commit includes Co-authored-by: hlin99 <hlin99@users.noreply.github.com> which violates BOT_POLICY.md: "Never add Co-authored-by trailers to commit messages" and "Never use the GitHub noreply address".

  4. Missing newline at end of .github/workflows/ci.yml. The diff shows \ No newline at end of file — the last line of the build job should end with a newline.

  5. Workflow change notes:

    • Adding pull_request trigger to trigger-integration.yml and switching to INTEGRATION_TOKEN is reasonable for cross-repo dispatch.
    • The if/else logic for PR vs release event types is correct.
    • Consider adding a permissions: block for security hardening.
  6. Previous review issues (from e49f5b6) appear moot — the PR content has completely changed from the prior commit (was adding bot policy docs, now is removing tests). This suggests a force-push or rebase that replaced the entire PR content, which makes the PR history confusing. Consider opening a new PR for this separate change.

@hlin99 hlin99 closed this Apr 6, 2026

@hlin99-Review-Bot hlin99-Review-Bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit fe36881. Submitting REQUEST_CHANGES for the following reasons:

1. Merge conflicts (blocking). mergeable == false / mergeable_state: dirty. The branch appears to include commit 1e12d1c which duplicates the already-merged PR #141 (now c83b397 on main). The branch needs to be rebased or updated to resolve conflicts before review can proceed.

2. Previous feedback not addressed. My prior review on 1e12d1c raised:

  • No linked GitHub Issue — still missing.
  • PR title/description mismatch — the new commit fe36881 does add bot policy docs, making the docs: prefix more appropriate now. However, the PR also includes CI workflow changes (.github/workflows/ci.yml and trigger-integration.yml) which are ci: scope changes, not docs:. Consider splitting CI changes into a separate PR or updating the title to reflect both scopes.

3. Content review of bot/ docs (fe36881):

  • ENTRY.md, REVIEW_POLICY.md, AUTHOR_POLICY.md, DESIGN_PRINCIPLES.md — content is well-structured and consistent with the existing BOT_POLICY.md in the repo root. However, this raises a question: is BOT_POLICY.md in the repo root being replaced by the bot/ directory? If so, the PR should remove or deprecate the root BOT_POLICY.md. If both coexist, there is a risk of policy drift between the two sources of truth.

4. CI workflow changes:

  • ci.yml: Removing integration/stress test steps is consistent with the migration in #141. The smoke-test job now runs only unit tests — this is correct.
  • trigger-integration.yml: Adding pull_request trigger and switching to INTEGRATION_TOKEN for cross-repo dispatch is reasonable. The if/else logic for PR vs release events is correct. Minor suggestion: consider adding a permissions: block for security hardening.

5. pyproject.toml: Removing xpyd-sim from dev dependencies is consistent with #141.

6. Test/file removals: sim_adapter.py, tests/conftest.py, and all tests/integration/ and tests/stress/ files are removed. This duplicates what was already done in #141 — further evidence the branch has a conflict/duplication issue.

Please resolve the merge conflicts first, then address the remaining feedback.

@hlin99

hlin99 commented Apr 6, 2026

Copy link
Copy Markdown
Member Author

Rebased branch onto latest main in 2fbd875. The previous branch contained commit 1e12d1c (from merged PR #141) which was causing the diff to include unrelated refactor/CI changes. After rebase, the diff now cleanly shows only the docs changes under bot/ plus two pre-commit formatting fixes (ci.yml EOF newline, test_metrics.py blank line).

Addressing all review feedback on 1e12d1c:

  1. PR title/description mismatch — Fixed. After rebase, the diff only contains docs: changes matching the title. No more refactor/CI changes leaking in.
  2. No linked issue — This is a docs-only restructuring PR; no corresponding issue exists. Acknowledged in previous comments.
  3. Co-authored-by trailer — Not present in 2fbd875. Author and committer both use hlin99 <tony.lin@intel.com>.
  4. Missing newline at EOF in ci.yml — Fixed (was a pre-commit formatting fix already included).
  5. permissions block suggestiontrigger-integration.yml changes are no longer in this PR's diff (they belonged to refactor: remove integration/stress tests and sim_adapter #141).
  6. PR content changed from prior commit — Resolved by rebasing. Branch now has a single commit on top of main.

Pre-commit: all pass. Tests: 393 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants