|
| 1 | +name: Claude Code Review |
| 2 | + |
| 3 | +on: |
| 4 | + issue_comment: |
| 5 | + types: [created] |
| 6 | + |
| 7 | +jobs: |
| 8 | + # ────────────────────────────────────────────────────────────────── |
| 9 | + # Deep ModelOpt-focused review covering numerical correctness, |
| 10 | + # mode/state composition, export compatibility, and backward |
| 11 | + # compatibility for saved checkpoints / recipes. |
| 12 | + # |
| 13 | + # CodeRabbit (.coderabbit.yaml) auto-reviews every PR for routine |
| 14 | + # bugs, typos, style, and security anti-patterns — this Claude job |
| 15 | + # is on-demand and complements that with deeper architectural |
| 16 | + # analysis. Trigger: /claude review |
| 17 | + # ────────────────────────────────────────────────────────────────── |
| 18 | + review: |
| 19 | + name: Claude Review |
| 20 | + if: | |
| 21 | + github.event_name == 'issue_comment' && |
| 22 | + github.event.issue.pull_request && |
| 23 | + contains(github.event.comment.body, '/claude review') && |
| 24 | + contains(fromJson('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.comment.author_association) |
| 25 | + runs-on: ubuntu-latest |
| 26 | + permissions: |
| 27 | + contents: read |
| 28 | + pull-requests: write |
| 29 | + issues: write |
| 30 | + id-token: write |
| 31 | + env: |
| 32 | + GH_TOKEN: ${{ github.token }} |
| 33 | + REPO: ${{ github.repository }} |
| 34 | + PR_NUMBER: ${{ github.event.issue.number }} |
| 35 | + steps: |
| 36 | + - name: Get PR info |
| 37 | + id: pr-info |
| 38 | + run: | |
| 39 | + PR_DATA=$(gh pr view $PR_NUMBER --repo $REPO --json headRefOid,baseRefName) |
| 40 | + echo "sha=$(echo $PR_DATA | jq -r .headRefOid)" >> $GITHUB_OUTPUT |
| 41 | + echo "base_ref=$(echo $PR_DATA | jq -r .baseRefName)" >> $GITHUB_OUTPUT |
| 42 | +
|
| 43 | + - name: Checkout repository |
| 44 | + uses: actions/checkout@v6 |
| 45 | + with: |
| 46 | + fetch-depth: 1 |
| 47 | + ref: ${{ steps.pr-info.outputs.sha }} |
| 48 | + |
| 49 | + - name: Fetch base branch for diff analysis |
| 50 | + run: git fetch origin ${{ steps.pr-info.outputs.base_ref }} |
| 51 | + |
| 52 | + - name: React to trigger comment |
| 53 | + run: | |
| 54 | + gh api repos/$REPO/issues/comments/${{ github.event.comment.id }}/reactions \ |
| 55 | + --method POST \ |
| 56 | + -f content='eyes' |
| 57 | +
|
| 58 | + - name: Run Claude Review |
| 59 | + uses: anthropics/claude-code-action@v1 |
| 60 | + env: |
| 61 | + ANTHROPIC_BASE_URL: ${{ secrets.ANTHROPIC_BASE_URL }} |
| 62 | + with: |
| 63 | + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} |
| 64 | + trigger_phrase: "/claude review" |
| 65 | + show_full_output: true |
| 66 | + claude_args: | |
| 67 | + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Bash(git diff:*),Bash(git show:*),Bash(git log:*),Read,Grep,Glob" |
| 68 | + --model "${{ vars.CLAUDE_MODEL }}" |
| 69 | + prompt: | |
| 70 | + REPO: ${{ env.REPO }} |
| 71 | + PR NUMBER: ${{ env.PR_NUMBER }} |
| 72 | + BASE REF: origin/${{ steps.pr-info.outputs.base_ref }} |
| 73 | +
|
| 74 | + Mandatory workflow — never skip or reorder: |
| 75 | + 1. Read the PR diff first (gh pr diff). |
| 76 | + 2. Read CLAUDE.md and CONTRIBUTING.md for project conventions and architecture. |
| 77 | + 3. For changed files under `modelopt/torch/<sub-package>/`, read the sub-package's |
| 78 | + `__init__.py` plus any `mode.py` / `config.py` to understand mode registration |
| 79 | + and config schema. |
| 80 | + 4. Only then perform the review using that context. |
| 81 | +
|
| 82 | + You are performing a deep code review on a **NVIDIA Model Optimizer (ModelOpt)** PR. |
| 83 | + ModelOpt is NVIDIA's open-source library for model optimization (quantization, pruning, |
| 84 | + distillation, sparsity, speculative decoding, NAS, PEFT) targeting PyTorch / ONNX / |
| 85 | + HuggingFace / Megatron, with deployment into TRT-LLM / vLLM / SGLang. |
| 86 | +
|
| 87 | + Apply general correctness reasoning to whichever ModelOpt sub-package the diff touches — |
| 88 | + you already know the algorithms (quantization formulas, distillation losses, N:M sparsity |
| 89 | + mask selection, speculative draft-token alignment, etc.). The prompt below covers the |
| 90 | + **ModelOpt-specific structural concerns** that you can't infer from the diff alone. |
| 91 | +
|
| 92 | + ## Division of labor with CodeRabbit |
| 93 | +
|
| 94 | + CodeRabbit (`.coderabbit.yaml`) already auto-reviews every PR with the `chill` profile |
| 95 | + and runs a hard pre-merge gate on security anti-patterns. **Do NOT duplicate its work.** |
| 96 | + Specifically, do NOT comment on: |
| 97 | + - Style, formatting, or naming nits (handled by ruff + CodeRabbit) |
| 98 | + - Simple typos in code/comments/strings (CodeRabbit catches these) |
| 99 | + - The security anti-patterns enumerated in `.coderabbit.yaml`: |
| 100 | + `torch.load(weights_only=False)` without justification, `numpy.load(allow_pickle=True)` |
| 101 | + without justification, hardcoded `trust_remote_code=True`, `eval`/`exec` on external |
| 102 | + input, `# nosec` bypasses, non-permissive new PIP dependencies — these are already |
| 103 | + gated. Skip them entirely. |
| 104 | + - Generic "consider adding a test" suggestions for trivial changes. |
| 105 | +
|
| 106 | + Your value is in things CodeRabbit's pattern-matching cannot do well: tracing dataflow |
| 107 | + across multiple files, reasoning about mode/state composition, judging export |
| 108 | + compatibility, and catching algorithm-level correctness bugs. |
| 109 | +
|
| 110 | + ## Review Procedure |
| 111 | +
|
| 112 | + 1. Get PR metadata: `gh pr view $PR_NUMBER --repo $REPO --json title,body,baseRefName,headRefName,files,additions,deletions,changedFiles,author` |
| 113 | + 2. Get the full diff: `gh pr diff $PR_NUMBER --repo $REPO` |
| 114 | + - For large PRs (>50 files), prioritize source code over config/lock/auto-generated files. |
| 115 | + 3. For each significant changed file, read the full file for surrounding context. |
| 116 | + 4. Trace the algorithm end-to-end through the diff. Verify the math/logic matches the |
| 117 | + intended technique (whatever sub-package it belongs to). |
| 118 | + 5. For each newly introduced variable/argument/field, verify it has a meaningful runtime |
| 119 | + use path — not just declaration/docstring or discard assignment (`_ = new_arg`). |
| 120 | + Use Grep to search for usage beyond declaration sites. |
| 121 | + 6. Post findings as inline comments with severity and category tags. |
| 122 | +
|
| 123 | + ## Critical Issues (Must Fix) |
| 124 | +
|
| 125 | + ### Algorithm Correctness |
| 126 | + - Verify the implementation matches the intended technique. Apply your knowledge of the |
| 127 | + relevant algorithm family (quantization scales/rounding/saturation, distillation loss |
| 128 | + composition, sparsity mask selection, pruning importance scoring, speculative draft |
| 129 | + acceptance, NAS supernet weight sharing, etc.). |
| 130 | + - Watch for silent numerical bugs: missing fp32 upcast in reductions, wrong reduction |
| 131 | + dimension, division-by-zero guards, casts that wrap instead of saturate, gradient |
| 132 | + flow through stop_gradient boundaries. |
| 133 | + - Watch for state corruption across calibration / search / training passes — leftover |
| 134 | + statistics from a previous run are a common foot-gun. |
| 135 | +
|
| 136 | + ### Mode & State Composability (ModelOpt-specific) |
| 137 | + - **Mode registration**: New modes must register correctly with `apply_mode()` / |
| 138 | + `restore()`, declare their dependencies, and produce a `modelopt_state` entry that |
| 139 | + round-trips through save/restore. |
| 140 | + - **State dict schema**: Modified `modelopt_state` schema must include a migration path |
| 141 | + or version bump — silently changing keys breaks restore for existing checkpoints. |
| 142 | + - **Restore fidelity**: After `restore(model, state)`, the model must be functionally |
| 143 | + identical to the saved one. Verify module replacements, hooks, and parameters are |
| 144 | + re-applied. |
| 145 | + - **Plugin laziness**: Optional integrations (HF, Megatron, TRT-LLM, ONNX) must not |
| 146 | + hard-import at module load — gate behind `import_plugin()` so users without those |
| 147 | + extras don't break. |
| 148 | +
|
| 149 | + ### Export Compatibility (ModelOpt-specific) |
| 150 | + - HF export (`unified_export_hf.py`) must produce a checkpoint that loads cleanly in |
| 151 | + transformers and matches the on-device dtype. |
| 152 | + - TRT-LLM export (`model_config_export.py`) must emit a valid `config.json` with |
| 153 | + correct `quant_algo`, `kv_cache_quant_algo`, scale tensor names, and weight layout. |
| 154 | + - ONNX export must use opsets and operator versions supported by the target consumer |
| 155 | + (TRT, ORT). |
| 156 | +
|
| 157 | + ## Important Issues (Should Fix) |
| 158 | +
|
| 159 | + ### Backward Compatibility |
| 160 | + - Renamed or removed arguments / config fields without deprecation path — breaks |
| 161 | + existing user scripts. |
| 162 | + - `modelopt_recipes/*.yaml` schema changes without a version bump — old recipes |
| 163 | + silently misparse. |
| 164 | + - Changed defaults silently alter behavior for users relying on them. |
| 165 | + - Changed function signatures, return types, or side effects in |
| 166 | + `modelopt/torch/*/__init__.py` (public API) without a backward-compat shim. |
| 167 | + - Modified `modelopt_state` keys/structure without migration — makes existing |
| 168 | + optimized checkpoints unloadable. |
| 169 | +
|
| 170 | + ### Performance |
| 171 | + - Unnecessary CPU-GPU synchronization in hot paths: `.item()`, `.cpu()`, |
| 172 | + `torch.cuda.synchronize()`, Python-side tensor value checks. |
| 173 | + - Memory regressions: double-allocating weights, holding tensors past their lifetime. |
| 174 | +
|
| 175 | + ## Suggestions (Nice to Have) |
| 176 | + - Stale, imprecise, or misleading comments/docstrings — a wrong docstring is worse |
| 177 | + than none. |
| 178 | + - Missing shape/dtype assertions at module/parallelism boundaries where they would |
| 179 | + catch real bugs. |
| 180 | + - Functions mixing many unrelated responsibilities that would benefit from splitting. |
| 181 | +
|
| 182 | + ## Comment Format |
| 183 | +
|
| 184 | + Prefix each comment with severity and category tag: |
| 185 | + - `**[CRITICAL Algorithm]**`, `**[CRITICAL ModeState]**`, `**[CRITICAL Export]**` |
| 186 | + - `**[IMPORTANT Compatibility]**`, `**[IMPORTANT Performance]**` |
| 187 | + - `**[SUGGESTION]**` |
| 188 | +
|
| 189 | + For each finding, explain: (1) what the issue is, (2) why it matters (impact/risk), (3) specific suggestion for fix. |
| 190 | +
|
| 191 | + Only use inline ```suggestion blocks for simple, self-contained line replacements (typos, |
| 192 | + renames, single-line fixes). For structural changes that add, remove, or reorganize blocks |
| 193 | + of code, use a top-level PR comment with a code block showing the proposed change instead. |
| 194 | +
|
| 195 | + ## Completion |
| 196 | +
|
| 197 | + After posting all inline comments, post a summary PR comment: |
| 198 | + - List total findings by severity (CRITICAL: N, IMPORTANT: N, SUGGESTION: N) |
| 199 | + - Highlight the most impactful findings |
| 200 | + - Overall assessment of the PR's risk level |
| 201 | +
|
| 202 | + If no significant issues are found, approve the PR: |
| 203 | + gh pr review $PR_NUMBER --repo $REPO --approve --body "Claude review passed — no significant issues found. LGTM" |
0 commit comments