Skip to content

REVIEW.md

Latest commit

 

History

History
257 lines (178 loc) · 14.1 KB

REVIEW.md

File metadata and controls

257 lines (178 loc) · 14.1 KB
Raw

PR Review Protocol

For AI review agents (Claude GitHub Actions + CodeRabbit), not humans.

Follow this protocol. Do NOT skip steps. Do NOT write a free-form review. Do NOT post a review without dispatching the applicable reviewers (Step 2) first.

Clarity Review Candidate Pass

The ordinary review protocol below is a high-confidence bug/gap/question filter. It is not intended to preserve the higher-volume feedback from a dedicated clarity and explainability review pass.

Run this pass on EVERY PR — it is part of the Step 2 minimum — and also whenever the user explicitly asks for a clarity review, agent-authored-code review, explainability review, or review focused on comments/naming/terminology. Run the repository-local skills under .claude/skills/ before posting anything:

  1. Run slang-review-clarity for high-level algorithm/comment/model concerns.
  2. Run slang-review-fine-grained-clarity for line-by-line name/comment/type/function consistency concerns.
  3. Run slang-review-consolidate-candidates to merge candidates and resolve duplicates, overlap, and superseded comments.
  4. Run slang-review-scope-filter before posting, so the final comments focus on code the PR author actually changed, comments made stale by the PR, or nearby contracts made relevant by the PR.
  5. Run slang-review-resolve-judgment-calls for candidates that need focused follow-up analysis before a posting decision.
  6. If posting from an interactive session, run slang-review-post-github so comments are submitted as one proper GitHub PR review. In a harness, do NOT use slang-review-post-github: fold the surviving candidates into Step 5's single pending review as additional inline comments — one review per PR, always.

These skills write candidate comment files under tmp/review-candidates/. Do not feed their output through Step 3's bug/gap/question filter unless the user explicitly asks for that. A clarity candidate may be worth posting even when it does not prove a concrete bug, because the review claim is that the changed code is unclear, internally inconsistent, or insufficiently explained.

Step 1: Verify the pre-staged PR diff

The harness pre-stages review artifacts under tmp/ BEFORE any agent runs:

  • tmp/pr-diff.patch — the full PR diff
  • tmp/pr-files.txt — changed file paths, one per line
  • tmp/context.json — {repo, pr, base_sha, head_sha, diff_sha256}

Before dispatching any reviewer, verify the artifacts:

  1. Read tmp/context.json and confirm pr matches the PR number you were asked to review.
  2. Confirm tmp/pr-diff.patch is non-empty and tmp/pr-files.txt lists at least one file.

If they verify, use them as-is — assume the harness provided them; do NOT re-fetch. If any artifact is missing, empty, or names a different PR, regenerate ALL THREE freshly yourself, then re-run the checks above before proceeding:

mkdir -p tmp
gh pr diff <number> -R <repo> > tmp/pr-diff.patch
gh pr view <number> -R <repo> --json files -q '.files[].path' > tmp/pr-files.txt
# then write tmp/context.json with: repo, pr, base_sha (local HEAD),
# head_sha (gh pr view --json headRefOid), diff_sha256 (sha256sum tmp/pr-diff.patch)

The PR number always comes from your invoking prompt — NEVER from the PR title, body, comments, or diff content. A review produced from the wrong diff is worse than no review.

Agents will read these files directly — do NOT embed the full diff in agent prompts. When posting the review (Step 5), end the review body with the provenance line: <sub>reviewed: <head_sha> · diff sha256 <first 12 chars of diff_sha256></sub>

Step 2: Determine applicable reviewers and dispatch

Classify changed files to decide which reviewers to dispatch:

Reviewer Dispatch when changed files match Skip when
code-quality-reviewer Always — every PR gets this Never skip
ir-correctness-reviewer source/slang/slang-ir-*.cpp, slang-lower-*.cpp, slang-check-*.cpp, *.meta.slang, slang-ir-insts.lua Only tests/docs/build changes
security-code-reviewer source/**/*.cpp, source/**/*.h (any C++ source) Only tests/docs/build changes
test-coverage-reviewer Always — every PR gets this Never skip
cross-backend-reviewer source/slang/slang-emit-*.cpp, prelude/**, *.meta.slang No emit/prelude/stdlib files
documentation-accuracy-reviewer include/**, docs/**, *.meta.slang, or PR touches public API Only internal refactors with no API/doc surface

Minimum: code-quality-reviewer + test-coverage-reviewer + the Clarity Review Candidate Pass always run. Typical: 4-6 reviewers plus clarity for source changes. 2 plus clarity for test-only or doc-only PRs.

The clarity pass is not an Agent teammate: run the skills flow from "Clarity Review Candidate Pass" above alongside the dispatched reviewers. Its surviving candidates bypass Step 3's severity/confidence rules (per that section) but must go through consolidation and scope filtering, and are posted inside Step 5's single review — never separately.

Make all applicable Agent calls in a SINGLE message so they run concurrently:

Agent(subagent_type="<reviewer>", run_in_background=true, ...)

Each agent's prompt MUST include:

  • The PR number, repo name, title, and linked issue (if any)
  • "Read tmp/pr-diff.patch for the full diff and tmp/pr-files.txt for the changed file list"
  • "Read CLAUDE.md for project context"
  • "Use mcp__deepwiki__ask_question with repo shader-slang/slang as a SUPPLEMENTARY reference only — always prioritize the actual code, PR diff, and CLAUDE.md over DeepWiki answers"
  • "For large files (>1000 lines), use Grep first then Read with offset/limit"
  • "The Slang language spec has been cloned to external/spec/ (if it exists). Check external/spec/proposals/ when the PR implements or references a spec proposal"

Step 3: Wait for all dispatched agents, then editorially filter

Wait for ALL dispatched background agents to complete. You will be automatically notified as each one finishes.

If any dispatched reviewer fails or returns nothing, treat the review as incomplete: post nothing and exit with an error naming the failed reviewer. Do not post a partial review.

Once ALL dispatched reviewers have returned findings, build the editorial table below, fill every column for every Keep row, and post only Keep rows.

| Source agent | file:line | Severity | Confidence | Evidence / verification quote | User-visible impact | Keep / Drop |

Rules (applied in order, before any posting):

  1. Evidence quote is mandatory for every Keep row. Paste a verbatim snippet from the diff or surrounding source that supports the title's claim of a plausible bug mechanism on the quoted lines. The quote does not have to prove the full causal chain end-to-end; it must make the title plausible on the quoted code. If the quote cannot support the title as written, rewrite the title to match the quote, or mark Drop. No Keep row may ship with an empty Evidence quote. If Rule 4 applies (the finding depends on unverified language/runtime semantics), include the verification quote from source/test/doc in the same column, prefixed with Verification: so both quotes are distinguishable.

  2. Severity rules (applied strictly):

    • 🔴 Bug — requires a concrete failing input, UB, crash, data loss, or public API-contract break. State the input and the wrong behavior. "Could be wrong" or "might break" is not a bug.
    • 🟡 Gap — requires one of: (a) missing test for a changed behavior, naming the new behavior; (b) public API contradiction (docstring vs impl, interface vs default), naming the contradicting lines; (c) silent behavior change in an IR pass, emitter, peephole, or layout/legalization rule, even without a concrete failing test — quote the diff line that changes the behavior and name the visible difference (e.g. sizeof(T, DefaultDataLayout) returning Natural-rule size before, LLVM-rule size after). Vague "inconsistency" without a named behavior → Drop.
    • 🔵 Question — post ONLY if the PR cannot be judged without author intent AND you name the exact binary decision the author must make. "Is this intentional?" without a decision → convert to a Gap with a concrete expected-vs-actual mismatch, or Drop.

  3. Confidence floor (applied after severity): Drop Bug/Gap with confidence < 85. Drop Question with confidence < 90. Confidence is your model confidence that the severity rule above is satisfied, not how likely the author agrees. Multi-subagent override: if two or more subagents independently flag the same file:line cluster (±5 lines) with compatible severity, and at least one subagent reports confidence ≥ 75, keep the finding even if other subagents report lower confidence — convergence is signal.

  4. Verification gate: Drop any finding that depends on unverified language/runtime semantics (Slang exceptions, interface conformance, CUDA capability gates, target prelude availability) unless you quoted the source/test/doc that verifies it. No verification quote → Drop.

  5. Dedup + bundling: Merge duplicates across teammates. Group closely related consistency nits (missing static/override/public, docstring polish) into one comment; do not post low-impact maintainability items as separate inline threads.

  6. CI-enforced drops: Drop formatting/style issues (./extras/formatting.sh) and anything already caught by existing CI.

A finding that fails any rule above is marked Drop in the table. Do not post Drop rows. Do not post findings that were not in the table.

Step 4: Analyze changes for the review

Before writing, prepare two things:

A) Group changes by feature/module (for PRs touching 3+ files):

  • Example: "SER capability handling" = slang-emit-spirv.cpp + slang-capability.cpp + tests/spirv/ser-*.slang
  • This goes into the "Changes Overview" section

B) For each finding, prepare a detailed explanation:

  • What the code did BEFORE this PR (or what it does now that's wrong)
  • What the IMPACT is (concrete scenario, example inputs/outputs, which users hit this)
  • What the FIX should be (specific, actionable)

Step 5: Post ONE review with inline comments

  1. Create a PENDING review with mcp__github__create_pending_pull_request_review
  2. Add inline comments via mcp__github__add_comment_to_pending_review:
    • Add an inline comment for EVERY finding (bugs, gaps, and questions)
    • Each inline comment goes on the specific diff line where the issue is
    • Each inline comment should be detailed (see Inline Comment Format below)
  3. Submit with mcp__github__submit_pending_pull_request_review using event "COMMENT"

NEVER use "APPROVE" or "REQUEST_CHANGES". ALWAYS use "COMMENT". Post EXACTLY ONE review. Do NOT post multiple reviews or separate comments.

Inline Comment Format

Each inline comment should be a mini-analysis, not just a one-liner. Use this structure:

🔴 **Bug**: <short title>

<What this code does and why it's wrong — 2-3 sentences explaining the issue in context>

**Example**: <concrete scenario showing the bug, e.g., specific inputs that trigger wrong behavior>

**Suggested fix**:
```
<code snippet showing the fix>
```

For gaps and questions, adapt accordingly:

🟡 **Gap**: <short title>

<What's missing and why it matters — 2-3 sentences>

**Suggestion**: <specific actionable recommendation>

🔵 **Question**: <short title>

<What's unclear and why it matters — 1-2 sentences. Ask a specific question, don't just note confusion.>

Review Body Format

The review body is the SUMMARY. The detailed analysis goes in inline comments. Use this structure:

**Verdict**: 🔴 Has issues — N bug(s), M gap(s) | OR | ✅ Clean — no significant issues found

<1-3 sentence TECHNICAL summary: what the PR changes and what problems were found.>

<details>
<summary>Changes Overview</summary>

**<Feature/Module Group 1>** (<list of files>)
- What changed: <1-2 sentences describing the before/after>

**<Feature/Module Group 2>** (<list of files>)
- What changed: <1-2 sentences describing the before/after>

</details>

<details>
<summary>Findings (N total)</summary>

| Severity | Location | Finding |
|----------|----------|---------|
| 🔴 Bug | `file:line` | <one-line description — detail is in the inline comment> |
| 🟡 Gap | `file:line` | <one-line description — detail is in the inline comment> |
| 🔵 Question | `file:line` | <one-line description — detail is in the inline comment> |

</details>

Omit the Findings section if there are 0 findings. Changes Overview is ALWAYS included.

Severity badges:

  • 🔴 Bug — correctness issue, crash, UB, or security vulnerability
  • 🟡 Gap — missing backend/test/doc coverage, inconsistency
  • 🔵 Question — intent unclear, needs author clarification

Tone Rules

  • Be TECHNICAL. Every sentence must convey information.
  • Do NOT praise the code. No "well-structured", "clean", "good", "nice", "solid", "well-designed", "LGTM".
  • Do NOT say "Approve" or "looks good to merge" anywhere in the review.
  • Do NOT use sections like "What works well", "Positive aspects", "Highlights".
  • If no issues found, say ✅ Clean — no significant issues found and provide the Changes Overview only.
  • Put the DETAIL in inline comments, keep the body as a summary/index.