|
| 1 | +--- |
| 2 | +name: pr-review |
| 3 | +description: Review a GitHub PR with design-first analysis, posted as a GitHub review. |
| 4 | +--- |
| 5 | + |
| 6 | +# PR Review |
| 7 | + |
| 8 | +Review a GitHub pull request with design-first analysis, posted as a GitHub review. |
| 9 | + |
| 10 | +## Input |
| 11 | + |
| 12 | +$ARGUMENTS — PR number or URL. If empty, detect from the current branch. |
| 13 | + |
| 14 | +## Instructions |
| 15 | + |
| 16 | +You are reviewing a PR on the `speculators` repo — a library for training and serving speculative decoding models (Eagle3, DFlash, MTP) with vLLM integration. |
| 17 | + |
| 18 | +### Phase 0: Eligibility check |
| 19 | + |
| 20 | +1. Resolve the PR number from `$ARGUMENTS` (or current branch via `gh pr view -R vllm-project/speculators --json number`). |
| 21 | +2. Check `gh pr view -R vllm-project/speculators <number> --json state,isDraft`. **Do not proceed** if the PR is closed, merged, or a draft. |
| 22 | +3. Check if you (the current `gh` user) already posted a review on this PR. If so, compare the latest commit SHA on the PR against the commit SHA at the time of your last review. **Do not post again** if there are no new commits since your last review — report to the user that the PR hasn't changed since the last review. Even with new commits, if your review would say substantially the same thing as your previous one (same verdict, same outstanding items), do not post — report to the user that nothing has materially changed. |
| 23 | +4. After gathering context (Phase 1–2), reassess: if the discussion thread shows the PR is effectively still in progress — active experiments, trial-and-error debugging, the author saying "still working on this", or unresolved back-and-forth about the approach — treat it as a draft and do not review. Report to the user that the PR appears to still be in flux. |
| 24 | + |
| 25 | +### Phase 1: Gather context |
| 26 | + |
| 27 | +Run these in parallel: |
| 28 | + |
| 29 | +- `gh pr view -R vllm-project/speculators <number> --json title,body,baseRefName,headRefName,author,labels,files` |
| 30 | +- `gh pr diff -R vllm-project/speculators <number>` |
| 31 | +- `gh api repos/vllm-project/speculators/pulls/<number>/reviews` — existing reviews |
| 32 | +- `gh api repos/vllm-project/speculators/pulls/<number>/comments` — inline comments |
| 33 | +- `gh api repos/vllm-project/speculators/issues/<number>/comments` — conversation thread |
| 34 | + |
| 35 | +Then read the full diff. For changed files larger than 300 lines, also read the full file for surrounding context. |
| 36 | + |
| 37 | +If the PR description contains links (issues, discussions, external docs, benchmarks, etc.), fetch them and incorporate the context. Verify that the PR actually addresses what the linked resources describe — flag mismatches between linked context and the implementation. |
| 38 | + |
| 39 | +### Phase 2: Understand existing discussion |
| 40 | + |
| 41 | +Before forming any opinions, catalog: |
| 42 | + |
| 43 | +- What has each reviewer already said? |
| 44 | +- What has the author responded to? |
| 45 | +- What is unresolved? |
| 46 | + |
| 47 | +**Do NOT echo points already raised by other reviewers.** If you agree with an existing comment, do not post your own version of it — even with additional detail or a different explanation. The only exception is if the author asked a clarifying question on the existing comment and the original reviewer hasn't answered; in that case, reply to that comment thread rather than posting a separate inline comment. Drop any finding that overlaps with an existing review comment. |
| 48 | + |
| 49 | +If reviewers or the author linked to sources (docs, issues, code snippets, benchmarks, papers) in their comments, fetch and read them — they often contain context that informs whether a concern is valid or already resolved. |
| 50 | + |
| 51 | +### Phase 3: Design & big-picture review |
| 52 | + |
| 53 | +Evaluate these first — they matter more than line-level nits: |
| 54 | + |
| 55 | +- **Motivation**: Is the PR description clear about *why* this change is needed? If the motivation is missing or vague, ask the author to clarify. A PR that doesn't explain the problem it solves is hard to review properly. |
| 56 | +- **Purpose & scope**: Does the PR do what the description claims? Is the scope appropriate or should it be split? |
| 57 | +- **Architectural fit**: Does this fit the existing patterns in `speculators`? Does it introduce unnecessary abstractions or bypass existing ones? Read surrounding code in the same module to understand local conventions (naming, error handling, structure) and flag deviations. |
| 58 | +- **Correctness of approach**: For the problem being solved, is this the right approach? Have alternatives been considered? If you see a clearly better alternative, name it concretely and explain the tradeoff. |
| 59 | +- **Design principles**: |
| 60 | + - *DRY*: Does this duplicate logic that already exists elsewhere in the codebase? If so, point to the existing code. |
| 61 | + - *YAGNI*: Does this add abstractions, parameters, or code paths not justified by the PR's stated goal? Premature generalization is a flag. |
| 62 | + - *KISS*: Is there a simpler way to achieve the same result? Complexity should be proportional to the problem. |
| 63 | +- **Backward compatibility**: Could this break existing checkpoints, configs, or CLI invocations? |
| 64 | +- **Test coverage**: Does the PR include tests proportional to the change? New logic should have unit tests. Changes to training, data generation, or model forward passes that affect end-to-end behavior should have integration tests (or the author should explain why they're impractical). For bug fixes specifically, there should be a regression test that fails without the fix and passes with it — if missing, ask for one. If tests are missing, ask for them — specify what scenarios should be covered. |
| 65 | +- **Distributed training correctness** (if applicable): barrier placement, device consistency, FSDP wrapping, gradient handling. |
| 66 | +- **Tensor/numerical correctness** (if applicable): shape mismatches, dtype promotions, masking, off-by-one in shift-based alignment. |
| 67 | + |
| 68 | +### Phase 3.5: RFC compliance |
| 69 | + |
| 70 | +If the PR description or linked issues reference an RFC (issues labeled `rfc` in this repo), fetch the RFC issue via `gh issue view` and verify: |
| 71 | + |
| 72 | +1. Does the implementation match what was agreed in the RFC discussion? Check the final consensus, not just the original proposal — decisions often evolve in comments. |
| 73 | +2. Are there open objections or unresolved questions in the RFC that this PR should not proceed without addressing? |
| 74 | +3. If the implementation deviates from the RFC, is the deviation documented and justified in the PR description? |
| 75 | + |
| 76 | +Flag any discrepancy between the RFC and the implementation, quoting the relevant part of the RFC discussion. |
| 77 | + |
| 78 | +### Phase 3.6: Paper validation |
| 79 | + |
| 80 | +If the PR description, commit messages, or code comments reference a paper (arXiv, conference proceedings, etc.): |
| 81 | + |
| 82 | +1. Fetch the paper (use `WebFetch` on the arXiv abstract page or PDF URL). |
| 83 | +2. Identify the specific equations, algorithms, or architectural details the PR claims to implement. |
| 84 | +3. Compare the implementation against the paper: |
| 85 | + - Do the equations match? Check operator ordering, normalization, index conventions. |
| 86 | + - Are any simplifications or deviations from the paper intentional and documented, or silent divergences? |
| 87 | + - Are hyperparameter defaults consistent with what the paper recommends? |
| 88 | +4. In your review, **quote the relevant section/equation from the paper** and compare it to the code. For example: "Paper Eq. 5 defines the loss as `L = ...`, but the implementation at `file:line` computes `...` instead — is this intentional?" |
| 89 | +5. If no paper is referenced but the PR implements a known algorithm (Eagle, EAGLE-2, Eagle3, DFlash, MTP, Medusa, etc.), check whether the implementation aligns with the canonical paper. If you cannot fetch or verify the paper, state that explicitly rather than guessing. |
| 90 | + |
| 91 | +Only flag mismatches you can concretely demonstrate by quoting both the paper and the code. Do not flag stylistic differences in how math is expressed if the computation is equivalent. |
| 92 | + |
| 93 | +### Phase 4: Line-level review |
| 94 | + |
| 95 | +Apply path-specific focus based on which files changed: |
| 96 | + |
| 97 | +- **`src/speculators/train/**`**: Distributed training correctness (barriers, device placement, FSDP), LR scheduler logic, checkpoint save/resume, loss computation with padding masks, multi-step loss aggregation and decay, bfloat16 safety. |
| 98 | +- **`src/speculators/models/**`**: Architecture correctness, attention mask construction for speculative positions, vocabulary mapping (draft→target) in forward/loss, KL divergence alignment, Pydantic config serialization. |
| 99 | +- **`src/speculators/data_generation/**`**: Hidden state extraction correctness, shift-based alignment (off-by-one), loss mask application before storage, vLLM client error handling. |
| 100 | +- **`src/speculators/convert/**`**: Weight mapping completeness, shape compatibility, key renaming correctness, legacy format assumptions. |
| 101 | +- **`src/speculators/config.py`**: Pydantic validators, serialization roundtrip, backward compat with saved checkpoints, registry auto-discovery. |
| 102 | +- **`tests/**`**: Coverage of new code paths, edge cases for speculative decoding (vocab boundaries, multi-step loss), proper mocking of vLLM/GPU. |
| 103 | + |
| 104 | +Ignore: `**/*.pyc`, `**/build/**`, `**/.pytest_cache/**`, `**/.ruff_cache/**`, `**/__pycache__/**`, `**/*.egg-info/**`. |
| 105 | + |
| 106 | +### Phase 5: Confidence scoring & filtering |
| 107 | + |
| 108 | +Rate each finding 0–100: |
| 109 | + |
| 110 | +- **0–25**: False positive, pre-existing issue, or something a linter/typechecker would catch. |
| 111 | +- **26–50**: Minor nitpick not tied to a project convention or concrete bug. |
| 112 | +- **51–75**: Real but low-impact or unlikely in practice. |
| 113 | +- **76–90**: Verified issue that will impact functionality, or violates a documented project convention. |
| 114 | +- **91–100**: Confirmed critical bug with concrete failure scenario. |
| 115 | + |
| 116 | +**Only keep findings with confidence ≥ 80.** |
| 117 | + |
| 118 | +False positives to actively filter out: |
| 119 | + |
| 120 | +- Pre-existing issues not introduced by this PR. |
| 121 | +- Pedantic style nitpicks a senior engineer would skip. |
| 122 | +- Issues a linter, typechecker, or CI would catch — do not run these yourself. |
| 123 | +- General quality concerns (documentation, naming style) that are purely cosmetic and not tied to correctness. |
| 124 | +- Issues silenced in code (e.g., lint ignore comments). |
| 125 | +- Intentional functionality changes that align with the PR's stated purpose. |
| 126 | + |
| 127 | +### Phase 6: Draft the review |
| 128 | + |
| 129 | +For each surviving finding: |
| 130 | + |
| 131 | +- State the problem in one sentence. |
| 132 | +- If the issue involves an external API, library behavior, or spec, **link to the source or quote the relevant documentation**. Do not make unverified claims about external behavior. |
| 133 | +- If you propose an alternative, show a concrete code suggestion and confirm the alternative works in context. |
| 134 | +- If the author's intent is unclear, **ask a question** instead of assuming. |
| 135 | + |
| 136 | +**Do NOT speculate.** Only flag issues you can validate from the diff, surrounding code, or linked documentation. If you're unsure whether something is a bug, phrase it as a question to the author. |
| 137 | + |
| 138 | +**Source attribution**: If the PR implements logic derived from a paper, spec, or external reference and the code doesn't link to it (in comments, docstrings, or the PR description), ask the author to add a reference. Future readers shouldn't have to reverse-engineer which paper or doc a piece of logic came from. |
| 139 | + |
| 140 | +Structure: |
| 141 | + |
| 142 | +**Review body** (high-level summary): |
| 143 | + |
| 144 | +- 1–3 sentences on the overall design assessment. |
| 145 | +- Note any high-level concerns or open questions. |
| 146 | +- If everything looks good at the design level, say so briefly. |
| 147 | + |
| 148 | +**Inline comments** (line-level): |
| 149 | + |
| 150 | +- Each non-high-level comment must reference a specific file and line range from the diff. |
| 151 | +- Order: correctness issues → design concerns → suggestions → questions. |
| 152 | +- When linking to code, use full-SHA permalink format for proper rendering: `https://github.com/vllm-project/speculators/blob/{full_sha}/{path}#L{start}-L{end}` Get the SHA via `git rev-parse HEAD` on the PR branch, and include ≥1 line of context above and below. |
| 153 | + |
| 154 | +### Phase 7: Verify before posting |
| 155 | + |
| 156 | +Re-check **every** comment against this checklist: |
| 157 | + |
| 158 | +1. **Is the claim actually true?** Re-read the relevant code. If you reference external behavior (PyTorch, HuggingFace, vLLM), verify it via docs or source — or remove the claim. |
| 159 | +2. **Is this already addressed in the PR?** Check if a later hunk in the diff fixes the issue. |
| 160 | +3. **Was this already raised by another reviewer?** Drop if so. |
| 161 | +4. **Is this preference disguised as correctness?** If yes, either drop it or explicitly mark as suggestion. |
| 162 | +5. **For alternatives you propose**: have you verified the alternative actually works in this context? |
| 163 | +6. **Does this pass the confidence ≥ 80 bar after re-examination?** |
| 164 | + |
| 165 | +Drop any comment that fails any check. |
| 166 | + |
| 167 | +### Phase 8: Post |
| 168 | + |
| 169 | +Use `gh api` to post the review with all inline comments in a single atomic review submission. **Do not use `-f 'comments[0][...]'` flags** — `gh` serializes those as a JSON object with string keys, not an array, causing a 422 error. Instead, pipe raw JSON via `--input -`: |
| 170 | + |
| 171 | +```bash |
| 172 | +cat <<'REVIEW_EOF' | gh api repos/vllm-project/speculators/pulls/{number}/reviews -X POST --input - --jq '.html_url' |
| 173 | +{ |
| 174 | + "event": "COMMENT", |
| 175 | + "body": "<review summary>", |
| 176 | + "comments": [ |
| 177 | + {"path": "<file>", "line": <line>, "body": "<comment>"}, |
| 178 | + {"path": "<file>", "start_line": <start>, "line": <end>, "side": "RIGHT", "body": "<comment>"} |
| 179 | + ] |
| 180 | +} |
| 181 | +REVIEW_EOF |
| 182 | +``` |
| 183 | + |
| 184 | +For single-line comments, use `line` only. For multi-line, use `start_line` + `line`. Use `side: "RIGHT"` for additions. |
| 185 | + |
| 186 | +If there are zero inline comments beyond the summary, post just the review body. |
| 187 | + |
| 188 | +If no issues survive filtering (all < 80 confidence), post a short review comment. Keep it to 1–3 sentences max. Do NOT list everything you checked — that's noise. Examples: |
| 189 | + |
| 190 | +- "LGTM. Recommend approving." |
| 191 | +- "LGTM. Agree with @coderabbitai's suggestion to add tests for auto-detection and `_save` happy path. Recommend approving once addressed." |
| 192 | +- "LGTM, one minor non-blocking note below. Recommend approving." |
| 193 | + |
| 194 | +**Never approve a PR automatically.** Always use `event="COMMENT"`, never `event="APPROVE"`. Only the human reviewer should submit an approval. |
| 195 | + |
| 196 | +End every review body with: |
| 197 | + |
| 198 | +``` |
| 199 | +🤖 Generated with [Claude Code](https://claude.ai/code) using the `/pr-review` skill |
| 200 | +``` |
| 201 | + |
| 202 | +Report the review URL when done, along with a brief summary: number of findings posted, number filtered out, and wall-clock time elapsed since Phase 0 started. |
0 commit comments