<!--
SPDX-FileCopyrightText: The Khronos Group, Inc.
SPDX-License-Identifier: CC-BY-4.0
-->

# 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:

```bash
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.
