/mr-review - MR/PR Code Review (Read-Only: Working Tree Untouched)

Provider resolution (GitHub or GitLab)

This command works on either GitHub or GitLab (self-hosted or SaaS). Resolve the provider once at the start, then use that provider's CLI for every operation below.

1. Detect the provider from the push remote's host — git remote get-url origin:
   • host is github.com (or *.github.com) → GitHub · CLI gh · term PR
   • any other host (self-hosted GitLab, gitlab.com, …) → GitLab · CLI glab · term MR
   • Override wins: if .claude/repo-config.json has "provider": "github" or "gitlab", use that (for ambiguous/self-hosted hosts).
2. Target resolution — let the provider CLI auto-detect host/namespace/IDs from git remotes; never hardcode them. Fork workflow (both origin and upstream remotes present): origin = your push target, upstream = the MR/PR target.

Review a merge/pull request without touching the working tree, the current branch, or any local file. The only local write is a fetched ref under refs/mr/ / refs/pr/ (Step 3).

Usage

/mr-review [<mr-or-pr-number-or-url>] [--comment] [--deep] [free-text follow-up]

Accepts a bare number (123, !123, #123) or a full MR/PR URL on either provider. Strip any !/# prefix and, for a URL, extract the trailing number.

No number (the usual case — ~2 in 3 runs): resolve the open MR/PR for the current branch — glab mr list --source-branch "$(git branch --show-current)" -F json | jq '.[0].iid' / gh pr view --json number -q .number. Inside a worktree, that is the worktree's branch. Zero or several matches → ask; never guess.

Free text after the reference ("then /fix-review", "merge when done") is a follow-up instruction for after the report — not part of the reference, and not permission to post or merge on its own.

--comment posts directly (Step 7). Without it, posting normally happens through /fix-review, which reads findings.json from Step 7's path.

CRITICAL RULES

1. NEVER run git stash, git checkout, git switch, git pull, or any command that modifies local git state
2. NEVER run linters/formatters/static analysis on local files (e.g. Pint, PHPStan, Rector, ESLint, Prettier, ruff, gofmt, clippy) — this is a remote review
3. File content comes from the MR/PR head ref (Step 3) or, as fallback, the provider's API — never from the working tree, which may be on a different branch
4. The local working directory must be untouched when this command finishes
5. Scratch files MUST live under .claude/tmp/mr-review/<NUMBER>/ — never /tmp, never the project root. See "Scratch Files" below.

Scratch Files

If you need to write any temporary/scratch artefact during this review (raw API JSON, per-agent review dumps, aggregation buffers, anything you'll re-read later), put it under:

.claude/tmp/mr-review/<NUMBER>/

Rules:

• Resolve it to an absolute path once, from the main checkout, and use that string everywhere: SCRATCH="$(git worktree list | head -1 | awk '{print $1}')/.claude/tmp/mr-review/<NUMBER>"; mkdir -p "$SCRATCH". A relative path breaks inside worktrees — one run wrote findings.json to a doubled path and mr-note.py then failed with FileNotFoundError.
• Use descriptive filenames (e.g. mr-meta.json, diffs.json, review-security.md).
• .claude/ is already gitignored, so nothing leaks into the repo.
• Existing Read/Write permissions cover this path — no extra prompts.
• Do NOT write to /tmp, /var/tmp, $TMPDIR, or anywhere outside the project root.
• Cleanup is optional — leave the dir for the user to inspect; it's gitignored.

Step-by-Step Implementation

Step 0: Run the repo's deterministic checks FIRST

If the repo has .claude/checks/run.py (or scripts/checks/), run it before spawning anything — it needs BASE and the refs/mr/<N> ref, so run it right after Step 3:

python3 .claude/checks/run.py "$BASE" refs/mr/<N>

Seconds, no agents, and it cannot miss its class — the one thing agents cannot promise. Measured on a real production MR: this layer found a cross-tenant IDOR in 3 seconds that a 303k-token security agent took 11 minutes to reach and a sibling agent read the same file and missed entirely — plus a second unscoped admin endpoint that seven agent runs never surfaced at all.

Triage the output before reporting any of it (checks trade precision for recall, so expect false positives), fold the survivors into the final report, and tell the Step 4 agents which classes are already covered so they don't spend budget re-deriving them.

Not in cwd? Look in the main checkout before concluding there are none — .claude/ is often gitignored, so a worktree (e.g. .claude/tmp/<slug>/) lacks it while the main checkout has it: python3 "$(git worktree list | head -1 | awk '{print $1}')/.claude/checks/run.py" <base_sha> <head_sha>. No checks directory in either? Say so once and continue. Afterwards, any confirmed finding whose class is mechanically detectable should become a new check — that is what makes reviews compound instead of re-rolling the dice at ~250k a throw.

Step 1: Parse the MR/PR reference

Extract the number from the argument. Strip a leading ! (GitLab) or # (GitHub); if a full URL was passed, take the trailing number. Below, <N> is that number.

Step 2: Fetch MR/PR metadata + changed files from the API

If the description contains a <!-- mr-guide --> block, read it first. It is the author's reviewer guide (from /mr-guide) — what changed, before/after, data model, UI states. Use it as context for the review. Do not regenerate it, do not review it, and do not repeat it in the report; the reviewer already has it above the diff.

GitLab:

# MR metadata (title, description, author, labels, state, diff_refs)
glab api projects/:id/merge_requests/<N>

# Changed-file list ONLY — project the diff bodies away, they never enter this context
glab api "projects/:id/merge_requests/<N>/diffs?per_page=100" --paginate \
  | jq -r '.[] | [.new_path, (if .new_file then "A" elif .deleted_file then "D" elif .renamed_file then "R" else "M" end)] | @tsv'

Check the metadata's web_url names the repo you're in before going further. glab can resolve :id to a different default project (seen on a multi-repo machine: MR 1 came back from another project). If it does, prefix every glab api and mr-note.py call with GITLAB_REPO=<namespace/repo>.

GitHub:

# PR metadata (title, body, author, labels, state, head/base SHAs)
gh pr view <N> --json number,title,body,author,labels,state,headRefName,baseRefName,headRefOid,baseRefOid,files

--json files gives the changed-file list with additions/deletions — that is all this context needs. Do not run gh pr diff here; agents read the diff from the ref. headRefOid is the head commit SHA you'll need when posting inline comments.

Display:

🔍 Reviewing MR/PR <N>: <title>
👤 Author: <author>
🎯 Target: <target_branch>

📊 Changed Files (<count>):
   • path/to/file1.ext (+40, -12)
   • path/to/file2.ext (+8, -3)
   ...

Step 3: Fetch the MR/PR head into a local ref — do NOT pull content into this context

Agents read the code themselves, from a git ref. Nothing is checked out, no branch is switched, and the working tree is never touched — the ref just lands in .git.

GitLab:

git fetch <remote> "refs/merge-requests/<N>/head:refs/mr/<N>"

GitHub:

git fetch <remote> "refs/pull/<N>/head:refs/pr/<N>"

<remote> is the remote pointing at the target project — if git remote lists upstream, use it; otherwise origin. Fetching from the fork fails with couldn't find remote ref refs/merge-requests/<N>/head — that error means wrong remote, not that refs are disabled. Both providers expose the MR/PR head on the target project, so this covers fork MRs without adding the fork as a remote.

Then compute the base commit once. These two short strings are all this context needs to keep:

BASE=$(git merge-base refs/mr/<N> <remote>/<target_branch>)

Agents receive refs/mr/<N>, BASE, and the changed-file list — never file content. They read what they need with git show refs/mr/<N>:<path> and git diff $BASE..refs/mr/<N> -- <path>.

Why: on an 8-file MR, full content plus diff is ~48k tokens — which under the old flow landed in this context and got copied into all five agent prompts. Reading on demand costs each agent about the same as being handed the payload (measured on MR !3192: 55.7k on demand vs ~47.6k handed in) while this context holds a file list instead of the files. The cost moves into subagent context that is discarded, out of main context that is not.

Fallback — if the ref fetch fails (some self-hosted GitLab instances disable merge-requests/* refs; the checkout may also lack the right remote), fall back to fetching raw content from the API and passing it inline as before:

• GitLab: glab api "projects/<source_project_id>/repository/files/<url_encoded_path>/raw?ref=<source_branch>" — URL-encode / as %2F; source_project_id is the fork's id for fork MRs, else the upstream id.
• GitHub: gh api "repos/{owner}/{repo}/contents/<path>?ref=<headRefName>" -q '.content' | base64 -d — for fork PRs read the head repo from gh pr view <N> --json headRepositoryOwner,headRepository.
• If raw content also fails, review the diff content only.

State which path you took, so the user knows whether agents read the tree or were handed a payload.

Step 4: Spawn Parallel Review Agents

Spawn the review agents the scope gate selects (up to five) in a SINGLE message so they run in parallel. Each agent receives the git ref, the base sha, and the changed-file list — not file content.

Scope gate — match the reviewers to what the diff touches. Decide from the changed-file list alone:

• Only test files → spawn general + testing only. Measured: a 1-file test-only MR ran 4-5 reviewers for 341k tokens and zero findings; security/performance/architecture average ~60-70k each and have nothing to review there. General stays as the broad net (a test that disables a policy check is still a regression).
• Only docs / comments / translations (*.md, lang/locale files) → general only.

• Anything else → general, architecture, testing always, plus:

  • security only if the diff touches request input, routes/controllers/middleware, policies/gates/permissions, tenant or ownership scoping, queries built from input, file/upload handling, secrets/config, or outbound calls. Skip it on pure UI/styling, copy, migrations that only add columns, and internal refactors with no new entry point.
  • performance only if the diff touches queries/models/repositories, loops over collections, jobs/commands, caching, eager loading, or list/table pages. Skip it on pure UI/styling, copy, config, and validation-only changes.

  Measured over 395 agents (2026-10-05): security and performance each returned nothing ~35% of the time and cost ~150k tokens per surviving finding, against ~46-82k for the other three. When unsure, run it — the gate only drops a reviewer whose domain is provably absent.

State in the report's Coverage which reviewers ran and why any were skipped.

You never review the code yourself in place of the agents on a risky diff. A diff is risky if any changed path matches a regex in high_risk_paths of .claude/mr-review-risk.json (when the file exists), or it touches money, auth or permissions, state transitions, uniqueness constraints, or concurrency. On a risky diff, spawn the reviewers the scope gate selects, and treat Step 4b as required. Cost is not a reason to skip them. Measured on a booking/payment test set (2026-10-09): when this command followed its own gate and ran agents, it caught both planted bugs in 3 of 3 runs ($1.1-1.5 each); when it reviewed the diff itself instead, it missed one of the two bugs in 3 of 3 runs ($0.25 each). Self-review is only for the diff types the gate already trims to a single reviewer (docs, tests-only).

IMPORTANT: Do NOT paste file content or diffs into these prompts. Hand agents the ref and let them read what they need. If Step 3 fell back to the API path, paste content inline as the old flow did — that fallback is the only case where inline content is correct.

All five agents are pinned to sonnet (except under --deep, below) — keep the model= argument on every call. Pinning matters independently of the value: without it, editing an agent's frontmatter silently retunes /mr-review, because these agents are shared with /review.

Why sonnet across the board, measured rather than assumed — an A/B on MR !3192 (a controller dedupe touching a policy path and a form-request authorize()) ran the security reviewer on both tiers with identical input:

Same verdict, 1.9× the tokens, 4.2× the time. The wall clock is the decisive part: these five run in parallel, so the slowest agent gates the entire review. One opus agent turns every review into a nine-minute wait — paid on all reviews, including the clean majority.

--deep — escalation for security-sensitive MRs. Pin security and architecture to model="opus"; keep general, performance and testing on sonnet. Reach for it when the MR touches authorization, policies, gates, middleware, tenant scoping, or moves money — not on routine reviews, because the slowest agent gates the whole review (see the A/B above). Everything else — scope gate, testing reviewer, round 2, verification, posting — is unchanged; --deep changes two model pins and nothing more. (It replaces /nitpick, which ran an older copy of this flow with no testing reviewer and posted threads /fix-review could not read.)

Caveat on the A/B above: both agents were told to return only a JSON array. Sonnet complied; opus narrated first. That makes opus's checking visible and sonnet's invisible — it does not establish that sonnet checked less. The result supports the cost claim, not a claim about relative depth.

The read block — substitute this verbatim wherever a prompt below says <READ BLOCK>, filling in <N>, <BASE>, and the file list from Step 3. The checkout warning is not optional: all five agents share your working tree, and one git checkout would yank it out from under the other four and the user.

The MR/PR head is available as the local git ref `refs/mr/<N>`. It is NOT checked out.
Do NOT run `git checkout`, do NOT switch branches, do NOT stash, do NOT modify the
working tree in any way — other review agents and the user are sharing it right now.

Read full file content:   git show refs/mr/<N>:<path>
See what this MR changed: git diff <BASE>..refs/mr/<N> -- <path>

Read every changed file you need in full — do not review from the diff alone.

Do NOT run tests, artisan/npm/composer commands, or anything that writes a file. Tests here run
against a database other sessions share, and a scratch test file lands in the user's tree. If a
finding would be proven by mutating code ("delete line X and the suite stays green"), state the
mutation and the test that should catch it as the finding — do not perform it.

Changed files:
<one path per line>

The return block — substitute verbatim for <RETURN BLOCK>:

Notes first if you like, then the findings as ONE fenced json code block containing a JSON array
(`[]` if none). Report only what this MR introduces or newly makes reachable; a problem already
present at <BASE> goes in with `\"pre_existing\": true`. CRITICAL means data loss, a security hole,
money moved wrongly, or a crash on a common path — a missing test is IMPORTANT at most.
Each finding MUST have:
- severity: CRITICAL, IMPORTANT, or MINOR
- file: the new path of the file (for a missing test, the production file)
- line: the exact line in the NEW version of the file (for a missing test, the untested method's line)
- title: short one-line summary
- description: detailed explanation with suggested fix
- evidence: the exact line(s) from the NEW version of the file that prove the claim, copied verbatim, at most 200 characters. No quotable line, no finding.
- confidence: 1-10, how sure you are this is a real defect in how the code is actually used. 9-10 you traced it end to end; 7-8 clear in the code; 5-6 plausible but you did not rule out a mitigation; 4 or less you could not quote a proving line.
Example: [{\"severity\":\"IMPORTANT\",\"file\":\"src/foo.ext\",\"line\":42,\"title\":\"Missing null check\",\"description\":\"...\",\"evidence\":\"$x = $y->z;\",\"confidence\":8}]

If the named reviewer agents don't exist in this environment (security-reviewer, testing-reviewer, …), don't fail and don't silently skip a role: run each role's brief on general-purpose pinned to sonnet, and say in the report which substitution you made. Observed 2026-09-16 on a host that had none of them.

IMPORTANT (every agent): end each prompt with a required coverage declaration — "After the JSON, add two lines — COVERAGE-GAPS: what you did NOT examine closely, and why; and RULES: the project rules file you reviewed against, or none — generic practice." The RULES: lines feed the report's Rules used.

This is not bookkeeping. Silence from a reviewer currently reads as "checked, it's fine" when it usually means "never looked", and the gaps are where the next round should start. Measured on !3379: round 1's declared gaps became round 2's targets, and round 2 found the worst bug in the MR. Read the field for what it is, though — it reports what was not read, not what was read and not noticed, which is where most misses actually live.

IMPORTANT (every agent): Append this line to each agent prompt below — "Report only objective defects (crashes, security holes, logic errors, broken/contradictory behavior, real data bugs). Do NOT report subjective preferences, aesthetic opinions, or behavior that is plausibly intentional as findings. If you think a behavior might be a bug but it could just as easily be a deliberate design choice, do not assert it — leave it out (the synthesizer handles intent questions). Never invent a 'fix' for an intended behavior."

Agent 1 — General Review:

Agent(subagent_type="general-reviewer", model="sonnet", description="General MR/PR review", prompt="
Review this MR/PR for cross-cutting concerns, logic errors, code quality, and test coverage.

MR/PR: <N> - <title>
Description: <description>

<READ BLOCK>

Focus on: logic errors, missing edge cases, error handling, broken behaviour across files (backend/frontend mismatch, callers not updated). Security, performance and test-coverage reviewers run alongside you — leave their core topics to them unless the issue spans domains.
<RETURN BLOCK>
")

Agent 2 — Security Review:

Agent(subagent_type="security-reviewer", model="sonnet", description="Security MR/PR review", prompt="
Deep security review of this MR/PR.

MR/PR: <N> - <title>

<READ BLOCK>

Check for: injection (SQL/command/template), XSS, CSRF, missing authorization/permission checks, hardcoded secrets, mass assignment / over-posting, tenant or ownership scoping in multi-tenant systems, unvalidated user input, unsafe deserialization.
<RETURN BLOCK>
")

Agent 3 — Performance Review:

Agent(subagent_type="performance-reviewer", model="sonnet", description="Performance MR/PR review", prompt="
Performance review of this MR/PR.

MR/PR: <N> - <title>

<READ BLOCK>

Check for: N+1 / repeated queries (missing eager loading/batching), fetching more columns/rows than needed, missing pagination, missing caching, expensive work inside loops, missing indexes for new query patterns, unnecessary data loading.
<RETURN BLOCK>
")

Agent 4 — Architecture Review:

Agent(subagent_type="architecture-reviewer", model="sonnet", description="Architecture MR/PR review", prompt="
Architecture review of this MR/PR.

MR/PR: <N> - <title>

<READ BLOCK>

Check for: business logic leaking into controllers/handlers (should live in a service/domain layer), missing input-validation layer, missing DTOs/value objects for complex data, correct transaction boundaries for multi-entity writes, dependency injection vs hardcoded construction, proper separation of concerns.
<RETURN BLOCK>
")

Agent 5 — Testing Review:

Agent(subagent_type="testing-reviewer", model="sonnet", description="Testing MR/PR review", prompt="
Testing review of this MR/PR.

MR/PR: <N> - <title>
Description: <description>

<READ BLOCK>

Check for: missing test coverage for new/changed public methods, weak assertions (status-only without checking the response body/state), missing edge-case tests, isolation between test cases, correct fixtures/factories, and (in multi-tenant systems) tenant/ownership scoping in tests.
<RETURN BLOCK>
")

Re-runs. If the MR already has review threads (~/.claude/scripts/mr-note.py open <N> on GitLab), add them to every prompt as "already reported — do NOT re-report these", and say which commits are new since the last review. 22% of MRs are reviewed more than once, and about a fifth of a re-run's findings repeated an earlier location.

Step 4b: One handoff round (high-stakes MRs)

When the change moves money, touches auth/tenancy, or is otherwise expensive to get wrong, run ONE more agent after the parallel set — same generalist brief, but handed:

• every finding so far, with "do NOT re-report these", and
• the union of the agents' COVERAGE-GAPS, as its starting priorities.

Ask it for new findings, plus a CORRECTIONS: line naming anything already reported that is wrong or mis-severitied.

Give it this checklist. The first pass tends to skip the same five things, so tell the round-2 agent to go through every changed code path and say which apply, then either confirm a defect (quote the line, 200 characters at most) or state why it is safe: (a) concurrent or repeated execution of the same request: check-then-act with no lock or unique constraint; (b) a second occurrence of an action assumed to happen once: second subscription, retry, replay, re-upload after expiry; (c) state reached after a deadline, expiry or status change; (d) values that must be unique but are derived from fixed or reused inputs: references, keys; (e) limits and quotas enforced outside the transaction that writes. It returns only new defects, each with evidence and confidence 1-10, and marks them round 2 in the report.

This is the highest-yield agent in the whole command, because it is the only one not re-sampling ground already covered. On !3379 it cost ~219k and returned two findings nobody else had — including the most severe in the MR (a Cart whose auto-capture fails takes the customer's money and never creates an order, with no recovery path). It also re-verified five earlier findings and corrected none, which is itself worth knowing.

Round 2 is required on every risky diff (see Step 4). Skip it on low-risk diffs; it is a real cost (~140-220k). A trigger beats diff size — a 4-file change to login still gets round 2. Name the trigger in the report's Coverage ("round 2: moves money — Cart capture path") — no nameable trigger, no round 2. Measured: a 4-file MR ran two full rounds for 1.17M tokens with no trigger recorded, more than several larger MRs.

Step 5: Collect and Merge Results

1. Wait for every agent you spawned (up to five — whatever the scope gate selected)
2. Parse the JSON arrays from each agent's response
3. Tag each finding with its source category: security, performance, architecture, testing, or general
4. Critically evaluate each finding — do NOT blindly accept agent findings. For each finding, ask: "Is this a real problem in the actual usage context, or just a theoretical edge case?" Downgrade or discard findings that are technically correct but practically irrelevant. Review agents tend to flag theoretical issues that may never occur in practice — your job is to filter signal from noise. 4b. Separate DEFECTS from DESIGN/INTENT questions. A finding is only a defect (CRITICAL/IMPORTANT/MINOR) when it is objectively wrong: a crash, a security hole, a logic error, a broken/contradictory behavior, a real data bug. If instead the finding is a subjective preference, an aesthetic opinion, or a behavior that is plausibly intentional (e.g. "roundness 0 makes corners fully square", "this default could be different", "this copy/UX could be nicer", "consider a different threshold"), it is NOT a defect — do not assign it a severity and do not prescribe a "fix". Reclassify it as an Open Question with category: "question" and phrase it as a question to the author ("Is X intended?"), never as "Fix: change X". When unsure whether something is a defect or an intentional choice, default to treating it as a question. The bar: would a reasonable author unambiguously agree it's broken? If not, it's a question, not a finding. 4c. Verify every surviving finding against the code before it reaches the report. Open the file, confirm the claim, and check the mitigation the agent may not have looked for — a global scope, a middleware, a framework default. Agents are accurate about what they looked at and confidently wrong about what they assumed. Measured on !3379: 7 of 7 findings were substantively correct, but one claimed a lock was held indefinitely "because no timeout is set" when Laravel's HTTP client defaults to timeout => 30 — real worst case ~90s, not unbounded. Posting the overstated version would have got it dismissed, and taken the credibility of the other six with it. Cost: a handful of greps. The agent's evidence quote makes this cheap: git show refs/mr/<N>:<path> | sed -n '<line>p' and check the quote is there. A quote that is not at that line downgrades the finding to confidence 3.

4d. Pre-existing findings (pre_existing: true, or the code is unchanged since <BASE> — check with git show <BASE>:<path>) go in a short "Pre-existing (not introduced here)" note in the report, not in Findings, and are never posted as threads. Measured: "pre-existing, outside the diff" was the commonest reason a posted thread was dismissed. 4e. Route by confidence, to protect the reader's time. After verification: every CRITICAL stays in the main list whatever its confidence. Otherwise confidence 7+ → Findings; 5-6 → Worth a look; 4 or less, or an evidence quote that did not check out → not shown, only counted in Coverage, with the full text saved to .claude/tmp/mr-review/<N>/lower-confidence.md. Show at most 10 items in Findings; the rest go to Worth a look. Never raise confidence because two agents agreed. On a risky diff (Step 4), never hide an IMPORTANT or CRITICAL finding because of low confidence: it goes in Worth a look, labelled with its confidence, so only MINOR items and unverifiable quotes can end up in lower-confidence.md. Hidden items are always counted in Coverage's Not shown line.

5. Deduplicate — if 2+ agents found the same issue (same file + same/adjacent line), keep the most detailed one
6. Cross-reference — if 2+ agents flagged the same thing, note it as corroborated but do NOT automatically upgrade severity. Multiple agents agreeing on a theoretical issue doesn't make it more real.
7. Sort by severity: CRITICAL > IMPORTANT > MINOR

Step 6: Generate Report

Write it for the user in plain language — no skill jargon ("structural", "round 2", "a×b/m") without saying what it means; users repeatedly asked "explain this" when it leaked through.

## Code Review — MR/PR <N>: <title>

**<C> critical · <I> important · <M> minor** across <files> files — reviewers: <which ran>

### Findings
<Terse: each is `**[SEVERITY c<confidence>] title** — path:line — what is wrong in one or two sentences. Fix: one sentence.` Longer reasoning lives in `findings.json`, not here. One list, ordered CRITICAL → IMPORTANT → MINOR, then by confidence. Add the category tag(s) (several if corroborated). One list because a deduplicated finding has no single reviewer to file it under, and the reader acts by severity, not by who found it.>

### Worth a look (medium confidence)
<Same one-line form, confidence 5-6. Omit if none. Not counted in the verdict, never posted as threads.>

### Pre-existing (not introduced here)
<One line each, `path:line` — title. Omit if none. Not counted, not posted.>

### Open Questions (design / intent — NOT defects)
<Anything reclassified per Step 5 item 4b, phrased as a question for the author, with NO prescribed fix and NO severity. Omit if none. Never counts toward the verdict.>

### Coverage
- **Checks run**: <which deterministic checks ran, and what they covered>
- **Rules used**: <from the agents' `RULES:` lines — name any role that fell back to generic practice>
- **Round 2**: <"ran — <trigger>: <what>" or "skipped — no money/auth/tenancy trigger">
- **Not examined**: <union of the agents' COVERAGE-GAPS>
- **Estimated remaining**: <estimate, or `n/a — single pass`>
- **Not shown**: <N findings below confidence 5 or with an unverifiable quote — see `lower-confidence.md`>

### Verdict
**APPROVED** / **APPROVED WITH SUGGESTIONS** / **CHANGES REQUESTED**
**Re-review after fixes?** <"no — fixes are additive (tests, guards, validation); merge on green tests" or "yes — fixing <finding> means <structural change>">

---
🤖 Generated by Claude Code `/mr-review`

Verdict rule:

• Any CRITICAL, or any IMPORTANT that is a correctness/security/data defect → CHANGES REQUESTED
• Only MINOR, or IMPORTANT limited to maintainability/structure → APPROVED WITH SUGGESTIONS
• No findings → APPROVED — worded as "no findings from the passes that ran", never "clean"

Re-review rule — answer it up front, because "do we really need another /mr-review?" was the most common user question after a review. Use /fix-review's definition: a fix is structural if it deletes a branch, moves a decision across a lock/transaction/guard, changes lock ownership or constructor visibility, or merges two paths into one — those need a re-review. Additive fixes (a test, a tightened validation, a null guard) do not.

Never present a clean review as proof the code is clean. Say "no findings from the passes that ran", and name what was not examined. This is not hedging; it is measured. On !3379, two runs of the same reviewer role agreed on only 20–33% of findings; one agent read the exact file containing a cross-tenant IDOR and reported something else in it; and after seven runs and ~1.6M tokens a fresh reviewer still found three issues with zero overlap with the previous thirteen. A single pass finds roughly a quarter to a half of what is there, and you cannot tell which.

Estimate what's left only when two or more passes covered the same ground (round 2 ran, or two reviewers share a domain); otherwise write n/a — single pass and move on: if pass A found a, pass B found b, and they share m, then total ≈ a × b / m, so remaining ≈ that minus what you have. It is a floor — easy bugs are found by both passes and inflate m, which biases the estimate down. If m is 0, you have no estimate at all and no evidence of saturation; say exactly that and recommend another round rather than implying completeness.

Step 7: Post to the provider (if --comment)

If --comment was passed, ask the user first, then post each defect from the Findings list (not Worth a look, not the lower-confidence file) as a resolvable thread on its line, and one summary comment. Open Questions go in the summary, phrased as questions — not as threads.

GitLab — run the script; never hand-write the posting loop. Write the findings with a real serializer (a Python heredoc calling json.dump) to .claude/tmp/mr-review/<N>/findings.json:

[{"title": "<title>", "path": "<new path>", "line": 42,
  "body": "### <EMOJI> <SEVERITY>: <title>\n\n<description>\n\n---\n_Category: <category> | Review by Claude Code `/mr-review`_"}]

line is the finding's line in the NEW file, "old:<n>" for removed code, or null. The heading must carry the same title string — that is how a later run recognises the thread.

~/.claude/scripts/mr-note.py post <N> .claude/tmp/mr-review/<N>/findings.json

It anchors each finding against the MR's current head (context lines snap to the nearest added line; lines outside the diff become general threads carrying path:line), checks every note came back anchored, skips findings already on the MR, and prints one line per finding — anchored|general|exists <discussion_id> or FAILED <reason>. Report any FAILED line. Re-running is safe: nothing is posted twice.

Why a script: posting was the most-failing step of this command. Measured 2026-08-31..09-23 across 81 runs: GitLab rejected context-line anchors (line_code can't be blank) in ~25 sessions, and hand-rolled response parsing threw JSONDecodeError in 8+ — each run re-deriving the same loop and tripping a different trap. The traps are listed in the script's header.

GitHub — anchor first (git diff <BASE>..refs/pr/<N> | ~/.claude/scripts/diff-anchor.py findings.txt, lines <n>|<path>|<line>), then per finding:

gh api --method POST "repos/{owner}/{repo}/pulls/<N>/comments" \
  -f body="<body>" -f commit_id="<headRefOid>" -f path="<file>" -F line=<line> -f side=RIGHT

side=LEFT for an old_line anchor; unanchorable → gh pr comment <N> --body "…" with path:line in the body.

Summary comment — glab mr note <N> -m "…" / gh pr comment <N> --body "…":

## Code Review Summary — MR/PR <N>

| Severity | Count |
|----------|-------|
| 🔴 CRITICAL | N |
| 🟡 IMPORTANT | N |
| 🔵 MINOR | N |

**Verdict**: <APPROVED / APPROVED WITH SUGGESTIONS / CHANGES REQUESTED>

Each finding is posted as a separate resolvable thread on the relevant line.
<Open Questions, if any, as questions>

---
_Review by Claude Code `/mr-review`_

Large MRs/PRs (20+ files)

With Step 3's ref, this context never holds file content — so there is nothing to pre-fetch and no top-N cap to enforce. What still needs managing is each agent's own budget on a very large change.

If the MR/PR has 20+ changed files:

1. Tell the user: "This MR/PR has X files. Agents will read the highest-risk ones in full (entry points, business logic, data models) and take the rest as needed."
2. Order the paths in the read block's file list by risk — request handlers / controllers, services, models/schemas, and validation/input layers first; tests, fixtures, and generated files last.
3. Add to each agent prompt: "Read the highest-risk files in full first. If you are running low on context, review the remainder from the diff alone and say so explicitly in your response."
4. Note in the report if any agent said it fell back to diff-only, and for which files.

This is a soft budget, not the old hard cap: an agent that needs file #40 can still read it. Nothing is withheld from agents — they simply choose what to spend context on.

What This Command Does NOT Do

• Does NOT run git stash, git checkout, git switch, or any git state changes
• Does NOT run linters/formatters/static analysis locally (Pint, PHPStan, Rector, ESLint, Prettier, ruff, gofmt, clippy, …)
• Does NOT modify any local files
• Does NOT require the MR/PR branch to exist locally
• Does NOT require a clean working tree