mr-review

Imported from mohamed-abdelsamei/bq (skills/mr-review/SKILL.md). Install upstream with npx skills add mohamed-abdelsamei/bq --skill mr-review. Copyright stays with the author.

MR review

Review a merge request, pull request, branch, or diff on two axes at once:

• Code — is it correct, secure, and well-made?
• Business — does it deliver the intended requirement and real user value?

Catch both failures: clean code that solves the wrong problem, and the right problem solved unsafely. This is the reviewer's (Cass's) code-facing craft; stress-testing a plan or decision is the critique skill instead.

Read-only by default. Fetch metadata, diffs, logs, and context freely. Don't comment, approve, merge, push, or rewrite code unless the user explicitly asks for that action.

Severity — one vocabulary, so a verdict means the same thing every time

• Critical (blocking) — breaks correctness, security, data integrity, or an acceptance criterion. Merging causes an incident, data loss, a breach, or a shipped bug. Fix before merge.
• Important (usually blocking) — a real defect or meaningful risk: changed behavior with no test, an unhandled edge case, a regression risk, a requirement gap. Fix before merge unless the user knowingly accepts it.
• Minor (non-blocking) — a genuine correctness or maintainability improvement worth doing, but not worth blocking on.
• Nit (optional) — style or preference with no functional impact. Label [nit]; never let it hold up a merge.

Signal over noise

A review is only useful if its findings are trusted — protect that trust.

• Every finding names a concrete risk: what breaks, and when. If you can't state that, it's a [nit] at most.
• Don't relitigate scope, restyle working code, or demand refactors the diff didn't touch. Flag a pre-existing issue only when this change makes it materially worse.
• Three real findings beat twenty cosmetic ones — reviewer fatigue buries the critical one.
• Respect the repo's configured linters/formatters; don't hand-flag what a tool already owns.

The two axes

Code

• Correctness against the stated intent — edge cases, boundaries, failure modes, off-by-ones.
• Security — injection, auth/authz, secrets, unsafe deserialization, trust boundaries, sensitive-data exposure, dependency risk, unsafe defaults.
• Failure scope & blast radius — when this fails, what else fails with it? A feature that fails closed must fail closed narrowly: a broken dependency (upstream down, one corrupt file, a transient disk error) for feature X must not take down unrelated plane Y. Watch for an error path that is ?-propagated where only the success value was meant to gate — the guard is discarded but the error still escapes, so an outage in an optional check becomes a hard failure for every request. Trace each new error return to the widest caller it can reach.
• Enablement & rollout transitions — what happens the day a new flag flips on, or the migration runs, against existing data and identities? Existing sessions, tokens (PATs/API keys/service creds), cached state, and records predating the change. A gate that is correct for new users can lock out or silently break every existing automation the moment it activates.
• Errors & observability — failures surfaced not swallowed, errors actionable, no secrets in logs, enough logging/metrics to debug this in production.
• Performance & concurrency — hot-path cost, N+1 queries, unbounded growth, blocking calls on async paths, data races, lock/transaction scope. For any check-then-act sequence (validate a condition, then perform the effect that depends on it), name the exact shared state, which writers can change it unsynchronized, and the window between check and act: a guard is real only if the mutation side takes the same lock — moving the check next to the act shrinks a TOCTOU window but does not close it on a concurrent runtime, and a comment claiming an atomicity the code doesn't hold is itself a finding.
• Compatibility & migrations — API/schema/config/contract changes, data migrations and their rollback, impact on existing callers and persisted data.
• Necessity & fit — does this code need to exist, can it be smaller, does it follow local patterns?
• Removal completeness — when the change deletes, renames, or refactors out an asset (a file, a symbol, a config knob, a capability), everything that existed only to serve it is now stale or dead. Enumerate what was removed, then hunt each dependent: dangling doc/instruction pointers, comments and defaults that still describe the gone behavior, and now-caller-less exports or trait hooks. Verify dead-ness by usage search before asserting it — if only definitions and internal self-use remain, it is orphaned; name the exact symbols. Distrust relabel-and-retain: repurposing a helper that lost its last caller as a “generic” one is usually dead code with a new name.
• Tests — meaningful coverage for the changed behavior, failure paths included, not just the happy path.
• Honesty & drift — the diff does what the title and description claim; flag undisclosed changes riding along. Treat the description, any "acceptance criteria met" claim, spec/contract docs, in-code doc comments, and changelog entries as assertions to diff against the code, and when one is wrong fix every parallel copy — the same statement is often repeated across a doc file, an architecture note, and an inline comment. (See "Drift and the re-review loop".)

Business

• Walk the user-facing flow against the acceptance criteria.
• Check the forgotten states: empty, loading, error, permission-denied, migration, rollback, partial failure.
• Watch for scope creep, gaps against the requirement, and second-order costs (operational, support, scale).
• Flag a change that is technically clean but doesn't solve the requested problem.

Process & convention (repo rules are often the real blockers)

• Honor the repo's own contract — read AGENTS.md / CONTRIBUTING / CLAUDE.md and any skill they reference. Many repos make changelog fragments, doc/instruction sync ("instruction drift is a blocker"), and commit-message format mandatory; a diff can be flawless code and still be un-mergeable because it skipped one. These are as blocking as the repo declares them — surface them explicitly, don't bury them under code nits.
• Check that user-visible / config / permission / UI changes carry their required paperwork (changelog entry in the right place, updated docs, new feature flags documented).
• A changelog / release note must describe the behavior that will ship, not the branch's history of reversed decisions: flag entries that still describe a superseded state (a default later flipped, an approach later abandoned) and ask to collapse them into the final behavior.

Drift and the re-review loop — two high-yield passes

On mature code the sharpest findings are rarely bugs the author never saw. They are drift between what the code promises and what it does, and the residual gap a first fix leaves behind. Work both deliberately:

• Diff every written claim against the code. A spec value, a versioned contract, a description line, an acceptance-criteria "all met" claim, an in-code doc comment, and a changelog entry are all assertions — read each, then find the line that must honor it. A value the spec pins exactly but the code accepts loosely, a doc comment describing behavior the code no longer has, a description that overstates what shipped — each is a real finding even when the code in isolation looks fine. Flag the mismatch in either direction: code moved and the doc didn't, or the claim overstates what the code actually does.
• Guard the load-bearing invariant by name. Where the change touches a correctness or security boundary (identity, authorization, ordering, uniqueness, an equality/versioning check), know which fields and conditions the invariant depends on, and object the moment a convenience change relaxes one — a generalization that is harmless on an incidental field can be a correctness breach on a load-bearing one. Don't accept a broadened rule without confirming the boundary still holds.
• Offer a fork, not an order. For a mismatch either side can be the source of truth: ask to fix the code to match the spec or update and version the spec to match intended behavior — phrased as a question. It unblocks faster and respects that you may not know which was intended.
• Re-review as a loop: credit the fix, then name the residual. When a prior finding was addressed, state what the fix achieved, then pinpoint the exact gap it leaves rather than re-raising the whole issue. Narrow the severity to the residual instead of re-blocking at full weight, and verify the fix's own new comment, doc, or changelog line is itself accurate — fixes introduce fresh drift.
• Sweep the orphans of a removal, across every parallel copy. A stale pointer or dead knob left by a deletion almost never appears once — the same “see the old file” reference lives in a README and an app-local instruction file; the same removed-capability default sits in a build file and its CI mirror. Find one, then search for its siblings and fix them together; a half-swept removal is a real finding. Anchor it in the change's own stated goal to make it land: “this now makes the README the source of truth, yet line 214 still points at the deleted file,” “the suite no longer asserts Trust Check, yet this default still enables its debug filter.” The contradiction with the change's own intent is the argument.

Procedure

1. Resolve the real diff — never review from the description alone. Accept any target: an MR/PR number or URL (!123, #456), a branch, current branch, current changes, or a pasted diff/patch.
   • Hosted MR/PR — use the platform tool/CLI to fetch both metadata and the diff.
   • Branch — fetch the remote if needed, find the merge base against the target branch, review git diff <base>...<head>.
   • Local — review git diff (include staged changes if relevant).
   • Capture the target branch, changed files, commits, title/description, and the linked issue or requirement. If the target can't be resolved to concrete changes, stop and say exactly what's missing.
2. Anchor to intent. Read the linked requirement, acceptance criteria, design notes, and — when present — ~/.ai/<project>/charter.md and relevant decisions/. If intent is missing, review code risk and mark business validation as limited.
3. Triage the existing conversation — don't start from zero, and don't parrot it. Fetch the MR's existing threads: prior human reviews, and automated scanners (security bots, linters, CI annotations). Then add signal the bots can't:
   • Independently verify each open finding against the code. Automated security findings carry false positives — confirm or refute each with evidence, and say which. Refuting a wrong finding with a concrete reason is as valuable as raising a real one; it unblocks the author.
   • Don't re-report a finding an existing thread already covers unless you're adding evidence, confirming, or disputing it. Your job is the delta, not an echo.
   • Note which prior findings are already fixed in the current diff so stale threads don't block.
4. Review both axes. Read the whole diff first, then size effort to risk — a security boundary or a migration earns deeper scrutiny than a rename. For a large or cross-cutting diff, summarize the changed areas first and review by risk area, not file order.
5. Validate when it's cheap and telling. Prefer a narrow test/typecheck/lint/build that exercises the changed slice. If CI is already green on the reviewed SHA, say so and lean on it rather than re-running everything. Never claim a command passed unless you actually ran it.
6. Report findings first, then the verdict (format below).

Edge cases

• No resolvable diff → stop and ask for a ref, branch, or pasted diff. For a logically-scoped series, review commit-by-commit to keep each change in context.
• Security boundary touched → elevate scrutiny; unresolved auth/secret/injection/exposure risk usually means Request changes.
• Tests absent for changed behavior → Important when behavior, data, security, or a user-facing flow changed.
• Removal / rename / refactor-out diff → run the orphan sweep: list what's gone, then grep the tree for surviving references, stale config/comments, and now-dead exports/hooks — including their parallel copies in sibling docs and the CI mirror of a build file. Verify “no callers remain” by usage search before calling anything dead.
• Generated or vendored code → skip deep style review; check provenance, necessity, security, and integration points.
• Already reviewed (human or bot threads present) → don't restate the thread. Verify its open findings against the code, refute the false positives with evidence, note what's since fixed, and spend your effort on what it missed.
• Only nits found → don't inflate them; Approve and list them as optional.

Verdict

The scale is deliberately its own — a diff is judged for whether to merge, not stress-tested for whether to proceed (that's the critique skill's Proceed / Proceed-with-mitigations / Reconsider; the two aren't meant to converge). Lead with the call:

• Approve — no blocking findings; residual risks acceptable or clearly noted.
• Approve with changes — only minor or straightforward non-blocking fixes remain.
• Request changes — any critical or important issue: correctness, security, acceptance criteria, data integrity, or user value.

End with the single most important thing to fix first. Record a substantive review to reviews/ in project memory (see the memory skill).

Output format

Use this structure unless the user asked for another:

## Code findings
- [critical|important|minor|nit] Title — evidence and risk. Fix: concrete action. (`file:line`)

## Business findings
- [critical|important|minor|nit] Title — evidence and requirement/user impact. Fix: concrete action.

## Process & convention findings
- [critical|important|minor|nit] Title — which repo rule (changelog, doc/instruction sync, commit
  format) and how to satisfy it. Omit this section if the repo has no such rules or all are met.

## Prior findings triaged
- Confirmed / Refuted (with reason) / Already fixed — one line each. Omit if there were none.

## Validation
- Commands run, with pass/fail. CI status on the reviewed SHA. Checks skipped, with why.

## Verdict
Approve | Approve with changes | Request changes
Most important fix first: …

Order findings by severity. Keep [nit]s visually distinct from blockers so the verdict is actionable at a glance. If there are no findings, say so plainly and still note what you validated and the residual risk. For a contract/spec mismatch, phrase Fix: as a fork — correct the code or update-and-version the spec — since either side may be the intended source of truth.