diff --git a/bot/AUTHOR_POLICY.md b/bot/AUTHOR_POLICY.md index d67c0a1..8793330 100644 --- a/bot/AUTHOR_POLICY.md +++ b/bot/AUTHOR_POLICY.md @@ -6,48 +6,50 @@ Rules for the bot that writes code and submits PRs. ## Identity - | Role | GitHub Account | |------|---------------| | Author | `hlin99` | ## Before Coding - 1. Pull latest main: `git pull origin main` -2. Create feature branch: `git checkout -b /` +2. Create branch: `git checkout -b /` 3. Read [DESIGN_PRINCIPLES.md](DESIGN_PRINCIPLES.md) for architecture constraints. ## Code Quality - -1. Run lint: `ruff check xpyd_acc tests` -2. Run tests: `pytest tests/ -q` -3. Rebase before push: `git pull --rebase origin main` +- Run pre-commit: `pre-commit run --all-files` +- Run lint: `ruff check xpyd_acc tests` +- Run tests: `pytest tests/ -q` +- All must pass locally before pushing. ## PR Submission - -1. **One PR per task.** Don't bundle unrelated changes. -2. **Descriptive title.** Format: `type: short description` (e.g., `feat: add KV cache comparison`). -3. **PR body must include:** what changed, why, test coverage, breaking changes. -4. **All CI must pass** before requesting review. +1. One PR per task. Don't bundle unrelated changes. +2. Descriptive title: `type: short description` +3. PR body: what changed, why, test coverage, breaking changes. Reference issues (`closes #N`). +4. All CI must pass before requesting review. ## Responding to Review - -1. Fix all blockers before re-requesting review. -2. Reply to every comment — "Fixed in " or explain disagreement with evidence. -3. Push new commits (don't force push over reviewer comments). -4. **Never force push.** If the branch is too messy, close the PR and open a new one. +1. Address ALL `REQUEST_CHANGES` feedback before requesting re-review. +2. Always push new commits — never amend or force-push. +3. Reply to each review comment with fix commit ref (e.g. "Fixed in `abc1234`"). +4. Re-request review after pushing fixes (don't wait for reviewer to notice). +5. If reviewer is wrong, explain with evidence (link to source, docs, spec). + +## PR Maintenance +When there are open (non-draft) PRs, check every 5 minutes: +1. Update branch — if behind main, merge main into branch. +2. CI check — if any check failed, examine logs, fix code, push new commit. +3. Review comment check — read all `CHANGES_REQUESTED` reviews, fix issues, push, reply. +4. Re-request review after fixes. ## Documentation Updates - Every PR must update relevant documentation: | Change Type | Update | |---|---| -| New feature / CLI argument | `docs/guide.md` — add usage section | -| Design decision | `bot/DESIGN_PRINCIPLES.md` — append if needed | -| Quick Start affected | `README.md` — update (keep it one screen max, link to guide.md) | -| PR completed | `bot/iterations/current.md` — append summary | - -`docs/guide.md` is the source of truth for how to use the tool. +| New feature / CLI argument | `docs/guide.md` | +| Architecture change | `docs/architecture.md` | +| Design decision | `docs/design.md` | +| Quick Start affected | `README.md` (keep one screen, link to guide.md) | +| PR completed | `bot/iterations/current.md` | -When current iteration is complete, rename `bot/iterations/current.md` to `YYYY-MM-DD-.md` and create a fresh `current.md`. +When iteration complete: rename `bot/iterations/current.md` to `YYYY-MM-DD-.md`, create fresh `current.md`. diff --git a/bot/BOT_POLICY.md b/bot/BOT_POLICY.md index 4e08b3c..6f5092e 100644 --- a/bot/BOT_POLICY.md +++ b/bot/BOT_POLICY.md @@ -6,18 +6,44 @@ ## Language - **English only** — all code, docs, issues, PRs, comments on GitHub must be in English. No Chinese characters. -## Code Rules -- All changes go through PR. Never push directly to main. -- Every PR must have tests. No untested code. +## Branch Rules +- **Never push directly to main.** All changes go through PR. +- **Never force push.** If branch is too messy, close PR and open new one. +- Branch from latest main. Keep branch up-to-date by merging main into it. +- Each PR must be independent — no stacking PRs or branching off feature branches. + +## Commit Rules +- **Commit identity**: `git -c user.name="hlin99" -c user.email="tony.lin@intel.com" commit -s` +- Always use `tony.lin@intel.com` as commit email. Never use noreply address. +- Always include `Signed-off-by` trailer (`-s` flag) for DCO compliance. +- Never add `Co-authored-by` trailers. +- Follow conventional commits: `: ` (fix, feat, test, docs, refactor, chore, ci). + +## Before Pushing +1. Run pre-commit: `pre-commit run --all-files` +2. Run lint: `ruff check xpyd_acc tests` +3. Run tests: `pytest tests/ -q` +4. All three must pass locally before pushing. + +## Merge Policy (Loop Mode) +- Bot MAY merge a PR after: both reviewers approve + CI is fully green. +- Bot MAY close a PR on reviewer timeout or iteration failure. +- Auto-merge is part of the autonomous development loop. + +## CI - CI must be 100% green before merge. No skips allowed. - No test may be skipped. If a test can't run, fix it or remove it. -- Rebase to latest main before pushing. ## Testing -- Unit tests in `tests/` — pure acc logic, no external dependencies. -- Integration tests in [xPyD-integration](https://github.com/xPyD-hub/xPyD-integration) — cross-component tests. +- Unit tests in `tests/` — pure bench logic, no external dependencies. +- Integration tests in [xPyD-integration](https://github.com/xPyD-hub/xPyD-integration). + +## Secrets +- Never hardcode tokens or credentials in code, PR descriptions, or bot prompts. + +## Freshness +- **Always pull latest main and re-read BOT_POLICY.md before starting any work.** This is a living document. Never rely on cached copies. ## Architecture -- acc is a diagnostic tool for PD accuracy issues. No server components. -- All inference backend interaction goes through xPyD-integration tests. -- Follow step-by-step isolation approach (see [DESIGN_PRINCIPLES.md](DESIGN_PRINCIPLES.md)). +- acc is a PD accuracy diagnostic tool. No server components. +- Follow vLLM bench CLI compatibility (see [DESIGN_PRINCIPLES.md](DESIGN_PRINCIPLES.md)). diff --git a/bot/REVIEW_POLICY.md b/bot/REVIEW_POLICY.md index b1f03ad..a63b21a 100644 --- a/bot/REVIEW_POLICY.md +++ b/bot/REVIEW_POLICY.md @@ -1,65 +1,72 @@ -# Review Policy +# Review Policy — xPyD-acc ## Roles - | Role | GitHub Account | Action | |------|---------------|--------| -| Implementer | `hlin99` | Write code, submit PRs, fix issues | -| Reviewer 1 | `hlin99-Review-Bot` | Review PRs: approve / request changes / close | -| Reviewer 2 | `hlin99-Review-BotX` | Review PRs: approve / request changes / close | +| Implementer | `hlin99` | Write code, submit PRs | +| Reviewer 1 | `hlin99-Review-Bot` | Review PRs | +| Reviewer 2 | `hlin99-Review-BotX` | Review PRs | -## Timing +Each reviewer uses its own dedicated token. Never use author's token for reviews. +## Timing Parameters | Parameter | Value | |-----------|-------| | Iteration interval | 10 minutes | | PR wait for review | max 15 minutes | | Fix after request changes | max 10 minutes | -| Reviewer check frequency | every 5 minutes | +| Reviewer check frequency (has PRs) | every 5 minutes | +| Reviewer check frequency (no PRs) | every 15 minutes | | Reviewer response deadline | 15 minutes after assign | | Reviewer timeout action | close PR (iteration failed) | -| Total round timeout | 1 hour from PR creation | -| Round timeout action | close PR (iteration failed) | -## Review Criteria +## What to Review +1. Skip draft PRs. +2. Skip already-reviewed commits (only APPROVE counts as reviewed). +3. Re-requested reviews take priority — always perform fresh review. +4. One review per PR per commit SHA — never submit multiple reviews for same commit. + +## Review Process: Two-Stage Gate + +### Stage 1: Design Review (Gate) -Reviewers evaluate each PR on two dimensions: +Before looking at any code, evaluate the design: -### 1. Idea Value -- Is the direction/approach valuable for the project? -- Does it align with the project goals? -- **If NO → close PR immediately** (one close = PR rejected) +- **Is this change valuable?** Does it solve a real problem? Is it worth the complexity? +- **Is the approach sound?** Is this the right way to solve it? +- **Does it match the linked Issue spec?** If the PR deviates from the agreed design, reject. -### 2. Code Quality -- Is the code correct? -- Are tests included/passing? -- Is `bot/iterations/current.md` updated with clear description? -- Does `docs/guide.md` reflect changes (if applicable)? -- **If idea is good but code has issues → request changes** +**If the design has no value or the approach is wrong → CLOSE the PR immediately.** Do not proceed to code review. Do not waste time reviewing code for a feature that shouldn't exist. -## Decision Rules +### Stage 2: Code Review (only after Stage 1 passes) -| Scenario | Action | -|----------|--------| -| Both reviewers approve | Auto-merge | -| One approves, one requests changes | Implementer fixes, reviewers re-review | -| Either reviewer closes | PR closed, iteration failed | -| Both approve after fixes | Auto-merge | -| Timeout (15min no review) | PR closed, iteration failed | -| Total timeout (1 hour) | PR closed, iteration failed | +Only if the design is valuable and the approach is sound, review the code using the checklist below. Apply proxy-level strict standards — every line examined. -## Iteration Record +## Review Checklist +For each non-draft PR with a new commit: -Every PR MUST update `bot/iterations/current.md` with: -- What was done this iteration -- Result: merged / closed (with reason) -- Reviewer scores/comments summary +| Area | Check | +|---|---| +| CI | Must be fully green before APPROVE. May submit REQUEST_CHANGES or COMMENT while pending. | +| Merge conflicts | If mergeable == false, REQUEST_CHANGES. | +| Logic errors | Incorrect conditions, off-by-one, unhandled edge cases. | +| Type safety | Mismatched types, missing None checks. | +| Concurrency | Race conditions, missing locks, shared mutable state. | +| Exception handling | Bare except, swallowed exceptions, resource leaks. | +| Security | Injection risks, hardcoded secrets, unsanitized input. | +| Code style | Unused imports, shadowed variables, unclear naming. | +| Test coverage | New logic must have corresponding tests. | +| Design conformance | Implementation must match the linked GitHub Issue design. | -## Auto-Merge Requirements +## Verdicts +- **APPROVE** — code correct, CI green, no issues. +- **REQUEST_CHANGES** — any issue found. Use inline comments. +- **COMMENT** — CI pending or noting something without blocking. -- 2 approvals from designated reviewers -- CI passes (all checks green) -- No unresolved review comments +## Merge Policy (Loop Mode) +- Both reviewers approve + CI green → bot auto-merges. +- Reviewer timeout (15 min) → bot closes PR (iteration failed). +- Single approve is not enough to merge. Both must approve.