review

Purpose

Code quality analysis. Review finds bugs, bad patterns, security issues, and untested code. It does NOT fix anything — it reports findings and delegates: bugs go to rune:fix, untested code goes to rune:test, security-critical code goes to rune:sentinel.

Triggers

• Called by cook Phase 5 REVIEW — after implementation complete
• Called by fix for self-review on complex fixes
• /rune review — manual code review
• Auto-trigger: when PR is created or significant code changes committed

Calls (outbound)

• scout (L2): find related code for fuller context during review
• test (L2): when untested edge cases found — write tests for them
• fix (L2): when bugs found during review — trigger fix
• sentinel (L2): when security-critical code detected (auth, input, crypto)
• docs-seeker (L3): verify API usage is current and correct
• hallucination-guard (L3): verify imports and API calls in reviewed code
• design (L2): when UI anti-patterns suggest missing design system — recommend design skill invocation
• perf (L2): when performance patterns detected in frontend diff
• review-intake (L2): structured intake for complex multi-file reviews
• sast (L3): static analysis security scan on reviewed code
• L4 extension packs: domain-specific review patterns when context matches (e.g., @rune/ui for frontend, @rune/security for auth code)
• neural-memory | After review complete | Capture code quality insight
• council (L3): Step 1.6 — decorrelated bug-finding on the diff when blast radius is 50+ callers with a HIGH-severity change, mode=review

Called By (inbound)

• cook (L1): Phase 5 REVIEW — post-implementation quality check
• fix (L2): complex fix requests self-review
• User: /rune review direct invocation
• surgeon (L2): review refactored code quality
• rescue (L1): review refactored code quality
• design (L2): review UI/design implementation quality
• graft (L2): review grafted code integration

Cross-Hub Connections

• review → test — untested edge case found → test writes it
• review → fix — bug found during review → fix applies correction
• review → scout — needs more context → scout finds related code
• review → improve-architecture — when reviewer flag mentions "shallow", "wrapper", "indirection", or pass-through pattern
• review ← fix — complex fix requests self-review
• review → sentinel — security-critical code → sentinel deep scan
• review → council — blast radius 50+ callers with HIGH-severity change → decorrelated bug-finding on the diff

Execution

Review Policy

Three rules govern every step below. When a later step seems to conflict with one of these, the policy wins.

1. Precision over recall. A false alarm costs more reviewer trust than a missed LOW finding. One wrong CRITICAL teaches the developer to skim the next report; a missed style nit costs nothing. Optimise for a report where every line is worth reading — not for coverage.

2. Blocking split. Correctness and security findings are blocking. Style and idiom findings are non-blocking and never gate a merge on their own. Report both; only the first kind may produce REQUEST CHANGES.

3. Never guess at missing context. When the surrounding code, caller, or convention needed to judge a line is not in front of you, either go read it (Read, Grep, rune:scout) or stay silent. A finding invented to fill a gap in your own reading is the single most expensive kind of false alarm.

Step 1: Scope

Determine what to review.

• If triggered by a commit or PR: use Bash with git diff main...HEAD or git diff HEAD~1 to see exactly what changed
• If triggered by a specific file or feature: use Read on each named file
• If context is unclear: use rune:scout to identify all files touched by the change
• List every file in scope before proceeding — do not review files outside the stated scope

Strict Focus Rule — the scope list you just wrote is the only set of files this review may produce findings about.

• Reads outside that list are for understanding only. Open any file you need to judge the diff correctly (callers, types, config, conventions) — that is encouraged, not scope creep.
• A finding whose subject is a file outside scope is dropped, not reported — not downgraded to LOW, not filed under "while I was in there". Dropped.
• If you spot a genuine issue elsewhere while gathering context, record it as a one-line follow-up in the report footer, never as a review finding. It carries no severity and does not affect the verdict.

The reason is trust, not bureaucracy: a review that wanders produces findings the author cannot act on in this change, and trains them to skim the ones they can.

Rule Loading — pick the checklists this diff actually needs, then stop.

1. Collect the distinct file extensions across the scope list you just wrote.
2. Read references/rules/index.md and follow its mapping to the matching rule files — at most one per language present in the diff.
3. Always read references/rules/default.md. It carries the five dimensions every file is judged on.
4. Read nothing further. Never read all rule files — a Go rule cannot produce a true finding about a diff with no Go in it, only a plausible-looking one, and that is the noise this split exists to remove.

Every rule in those files ends with a Do not report when… clause. That clause is as binding as the rule above it: a rule that only says when to fire will fire on everything. Rules do not restate the Review Policy, the Evidence Contract, or the claim types — they inherit all three from this file, and a rule never invents a severity or a gate of its own.

Step 1.5: Blast Radius Assessment

For each modified function/class, estimate its blast radius before reviewing.

Use Grep to count direct callers/importers of each modified symbol:
  blast_radius = count(files importing or calling this symbol)

Blast Radius	Risk	Review Depth
1-5 callers	Low	Standard review
6-20 callers	Medium	Check all callers for compatibility
21-50 callers	High	Thorough review + regression test check
50+ callers	Critical	MUST escalate to adversarial analysis (rune:adversary) even in quick triage

Step 1.6: Decorrelated Bug-Finding (council, high-blast-radius only)

review's own pass is one model reading the diff once. For the same 50+ caller / HIGH-severity symbols that trigger the Step 1.5 HARD-GATE, call rune:council (mode=review) on the diff itself BEFORE writing up Step 2's findings — a second architecture reading the same code independently catches bugs a single pass rationalizes past.

This is complementary to (not a replacement for) the existing adversary escalation: council here examines the ALREADY-WRITTEN diff for bugs; adversary examines plan-level risk before code exists. Both can fire on the same high-blast-radius change.

Request: { question: <the diff, symbol name, and blast radius context — self-contained>, mode: "review", n: 3, diversity: { prefer_model_families: true }, evidence_required: [repro, reasoning] }.

Consume: fold agreement.consensus_claims into Step 2's CRITICAL/HIGH findings, tagged [council-verified]. Fold agreement.dissent into MEDIUM findings or the report's NEEDS DISCUSSION section, tagged [council-dissent]. If decorrelation: NO_DECORRELATION, do not claim independent confirmation in the report — say plainly that no second model family was reachable.

Skip if: blast radius is under 50 callers, or the change is not HIGH-severity — council is opt-in overhead reserved for the same tier that already requires adversarial escalation, not a default tax on every review.

Step 2: Logic Check (Production-Critical Focus)

Read each changed file. Prioritize bugs that pass CI but break production — these are the highest-value findings because linters and type checkers already catch the rest.

• Use Read on every file in scope
• Race conditions: async operations without proper sequencing, shared mutable state, missing locks
• State corruption: mutations that affect other consumers, cache invalidation gaps, stale closures
• Silent failures: caught errors that swallow context, empty catch blocks, promises without rejection handling
• Data loss paths: write operations without confirmation, delete without soft-delete, truncation without backup
• Edge cases: empty input, null/undefined, zero, negative numbers, empty arrays, Unicode, timezone boundaries
• Check for: logic errors, off-by-one errors, incorrect conditionals, broken async/await patterns
• Flag each finding using the Evidence Contract below — a snippet you copied, not a line number you remembered

Evidence Contract

Every finding carries these five fields. The line number is the one field you do not produce yourself.

Field	Required	Source
path	yes	the Step 1 scope list
evidence	yes	verbatim snippet copied out of the file
line	resolved	Grep on evidence at report time — never model recall
severity	yes	your judgement
claim	yes	OBSERVED / DERIVED / ASSUMED (Step 6)

A line number recalled from a long context drifts, and a correct finding pointing at the wrong line is unactionable — the reader looks, sees nothing, and stops trusting the report. A snippet can be checked against the file; a remembered number cannot. So produce what you can copy, and let a tool resolve the rest (Step 6, Anchor Pass).

Evidence rules:

• Copy the lines verbatim — no rewriting, reformatting, re-indenting, or tidying
• Strip diff markers (+, -, and the leading space on context lines) before recording
• Include only the lines directly involved — no surrounding context padding
• Cap at 5 lines. A finding that needs more than 5 lines to show is a design comment, not a defect — report it without evidence at MEDIUM or below
• Multiple disjoint locations → pick the single most relevant one and file the rest as separate findings

Evidence blocks are substance, not shape. Under context-engine's caveman output mode the prose around a finding compresses; the evidence block does not (../context-engine/references/output-modes.md — "shape is negotiable, substance is not").

Strict Focus applies here (restated from Step 1, because this is the step that breaks it): reading a caller or a helper outside the diff to decide whether a changed line is correct is expected. Reporting a bug you noticed in that caller is not — that finding is dropped, or goes to the report footer as a one-line follow-up. The question this step answers is only ever "is the changed code correct?"

Language-specific patterns for the files in scope come from the rule files loaded in Step 1 — each carries the concrete triggers and the conditions under which they must not be reported.

Step 3: Pattern Check

Check consistency with project conventions.

• Compare naming against existing codebase patterns (use Grep to sample similar code)
• Check file structure: is it in the right layer/directory per project conventions?
• Check for mutations — all state changes should use immutable patterns
• Check for hardcoded values that should be constants or config
• Check TypeScript: no any, full type coverage, no non-null assertions without justification
• Flag inconsistencies as MEDIUM or LOW depending on impact

Mutation, type-escape, and idiom triggers are language-specific — take them from the Step 1 rule files rather than applying one language's conventions to another's.

Step 4: Security Check

Check for security-relevant issues.

• Scan for: hardcoded secrets, API keys, passwords in code or comments
• Scan for: unvalidated user input passed to queries, file paths, or shell commands
• Scan for: missing authentication checks on new routes or functions
• Scan for: XSS vectors (unsanitized HTML output), CSRF exposure, open redirects
• If any security-sensitive code found (auth logic, input handling, crypto, payment): call rune:sentinel for deep scan
• Sentinel escalation is mandatory — do not skip it for auth or crypto code

Optional cross-model second opinion (security-critical / opus-escalated reviews only): a same-family reviewer shares blind spots with the author. For genuinely irreversible or attacker-facing changes (auth, crypto, payment, data migration), you MAY offer the user a different-architecture second pass via an external CLI (Gemini/Codex). This is opt-in and interactive-only — offer, never auto-invoke; skip in non-interactive runs (CI, /loop, scheduled) and announce the skip. If the user accepts, follow the safe transport in ../adversary/references/cross-model-escalation.md (per-call authorization, read-only sandbox, stdin not inline args), pass the diff + the security contract (not your verdict), and reconcile the reply as data — not a ruling.

Step 4.5: API Pit-of-Success Check

For code that exposes APIs, shared utilities, or reusable interfaces, evaluate through 3 adversary personas:

Adversary	Mindset	What They Reveal
The Scoundrel	Malicious — controls config, crafts inputs, exploits edge cases	Security holes, privilege escalation, injection surfaces
The Lazy Developer	Copy-pastes from docs, skips error handling, uses defaults	Unsafe defaults, missing validation, footgun APIs
The Confused Developer	Misunderstands API semantics, passes wrong types, ignores return values	Ambiguous interfaces, poor naming, missing type safety

Pit-of-Success principle: Secure, correct usage should be the path of least resistance. If the API makes it EASIER to use it wrong than right → WARN.

Check: Does the API have sensible defaults? Does misuse fail loudly (not silently)? Is the happy path obvious from the signature?

Skip if: Code is internal-only (no external consumers), single-use utility, or test-only.

Step 4.7: API Contract / Breaking Change Check

For any change that modifies exported functions, REST endpoints, event schemas, or shared types, check for backward-compatibility violations before proceeding.

Breaking change signals — flag any of these as HIGH:

Signal	Example	Why it Breaks
Removed export	export function getUser deleted	Callers crash at import
Renamed parameter	id: string → userId: string	Named-argument callers break
Narrowed return type	User | null → User (null removed)	Callers that handle null crash
Required arg added	fn(a) → fn(a, b: string)	All existing callers missing b
Status code changed	200 → 204 on success	Clients checking for body break
Event schema changed	{ userId } → { user_id }	Consumers miss the field
Endpoint path renamed	/users/:id → /users/:userId	All client URLs broken

Versioning check:

1. Run git diff main...HEAD — list every changed exported symbol
2. For each changed export: check if old signature still exists as an alias or overload
3. If breaking and no version bump → WARN: "Breaking change detected in [symbol] — needs CHANGELOG entry and version bump"
4. If CHANGELOG.md found: check that breaking changes are documented in the current version entry

Skip if: Change is internal-only (no exports changed, no public API surface affected), or in test files only.

Step 5: Test Coverage

Identify gaps in test coverage.

• Use Bash to check if a test file exists for each changed file
• Use Glob to find test files: **/*.test.ts, **/*.spec.ts, **/__tests__/**
• Read the test file and verify: are the new functions covered? are edge cases tested?
• If untested code found: call rune:test with specific instructions on what to test
• Flag as HIGH if business logic is untested, MEDIUM if utility code is untested

Per-Function Test Gap Analysis

Go beyond "test file exists" — check coverage at function granularity:

1. Extract changed functions — from the diff, list every function/method that was added or modified (name + file:line)
2. Map to test assertions — for each changed function, Grep the test file for its name. Count distinct test cases (look for it(, test(, describe( blocks that reference the function)
3. Classify gap severity:

Function Type	0 tests	1 test	2+ tests
Business logic (money, auth, state)	BLOCK	WARN: "only happy path"	PASS
Data transform (parse, format, map)	HIGH	PASS	PASS
Event handler (onClick, onSubmit)	MEDIUM	PASS	PASS
Pure utility (string, math, date)	MEDIUM	PASS	PASS

4. Output per-function table in review report:

### Test Gap Analysis
| Function | File | Tests Found | Verdict |
|----------|------|-------------|---------|
| calculateTotal | src/billing.ts:42 | 3 (happy, zero, overflow) | PASS |
| processRefund | src/billing.ts:89 | 0 | BLOCK — business logic untested |
| formatCurrency | src/utils.ts:12 | 1 | PASS |

5. Flag untested edge cases — for functions with only 1 test, check if the test covers: empty/null input, boundary values, error path. If only happy path → WARN: "only happy path tested for {function}"

Skip if: Diff only touches config, docs, styles, or test files themselves.

Step 5.5: Two-Stage Review Gate

Separate spec compliance from code quality. Most reviews conflate both — this gate forces the distinction.

Stage 1 — Spec Compliance (check FIRST)

Before evaluating code quality, verify the implementation matches what was asked:

• Load the originating plan, task, ticket, or requirements.md if available
• Does the implementation cover every acceptance criterion? Check each one explicitly
• Is there under-engineering — requirements stated but not implemented?
• Is there over-engineering — abstractions, generalization, or features beyond scope?
• Does the file/function structure match what the plan specified?

Flag spec deviations as HIGH — clean code that misses requirements ships broken products.

# Spec Compliance Checklist
[ ] All acceptance criteria from plan/ticket covered
[ ] No stated requirements missing from implementation
[ ] No unrequested features added (scope creep)
[ ] API surface matches what was specified (signatures, endpoints, return types)
[ ] File structure matches plan (no renamed or relocated files without justification)

If spec violations found: document them separately from code quality findings in the report. Label as SPEC-MISS or SPEC-CREEP.

Stage 2 — Code Quality

Proceed to Step 6 only after Stage 1 passes. Code quality findings (bugs, patterns, security, coverage) are the existing Steps 2–5 above.

The review report MUST show both stages: spec compliance verdict first, then code quality findings.

Step 5.7: Subtractive Pass (over-engineering lens)

Stage 1 catches SPEC-CREEP (unrequested features). This pass goes further: it hunts complexity worth deleting even inside requested scope, and reports it as a one-line-per-finding cut list with a net-lines total. Run it on any diff that adds a class, wrapper, config option, or dependency. Skip for pure config/docs/style diffs.

Tag each finding, one line: <file>:L<line>: <tag> <what>. <replacement>.

Tag	Cuts	Replacement
delete:	Dead code, unused flexibility, speculative feature	Nothing
stdlib:	Hand-rolled thing the standard library ships	Name the function
native:	Dependency or code doing what the platform already does	Name the feature
yagni:	Abstraction with one implementation, config nobody sets, layer with one caller	Inline it until a 2nd caller exists
shrink:	Same logic, fewer lines	Show the shorter form

End with the only metric that matters: net: -<N> lines, -<M> deps possible. Nothing to cut → Lean already. and move on.

Ranking discipline: these are LOW/MEDIUM findings (complexity, not correctness) — a subtractive suggestion NEVER outranks a real bug. A single smoke test or assert-based self-check is the ponytail minimum, not bloat — never flag it for deletion. This pass lists cuts; it does not apply them (route to rune:fix).

Step 6: Report

Produce a structured severity-ranked report.

Falsification Pass (run before writing the report)

Falsify, not verify. Do not ask "am I confident enough to report this?" — a model cannot calibrate its own confidence to a number, so that question filters nothing and quietly drops true findings. Ask the answerable question instead: "did I read something that disproves this?"

	Condition	Action
DROP	The code you read contains direct counter-evidence against the finding's key claim — the null check exists three lines up, the await is there, the input is validated by the caller you opened	Discard it
KEEP	The finding depends on context outside the diff that you did read via tools — that context is evidence, not a disqualification	Report it
KEEP	You can neither verify nor disprove it	Report it, typed honestly (below)

"Unsure" is not grounds to drop. Only counter-evidence is. A finding you could not confirm is still a finding — it is reported at the severity its claim type allows, not deleted to keep the report tidy.

Dropped findings are discarded silently — never listed as "considered and dismissed". A disproven finding is noise whether or not you label it as such.

Type every surviving finding with a claim type from ../completion-gate/references/claim-discipline.md:

Type	Means	Ceiling
OBSERVED	You read the code path this session and saw the defect	Any severity
DERIVED	Follows from what you read through a mechanism you can state in the finding	Any severity
ASSUMED	Requires an unverified premise (a caller you did not open, a runtime condition you cannot see)	Never CRITICAL — state the premise in the finding

An ASSUMED finding capped below CRITICAL is the honest form of "this looks wrong but I could not confirm the call path". Promotion happens by reading the code, never by rephrasing the finding more confidently.

Anchor Pass (run per surviving finding)

Resolve every line number now, with a tool. Climb the ladder and stop at the first rung that hits:

1. Grep the exact evidence string in path. A hit → the finding is anchored; use the line number Grep returned.
2. No hit → retry once with whitespace normalised (collapse runs of spaces) and the first and last lines of the snippet dropped. A hit → anchored on the remaining core.
3. Still no hit → the finding is UNANCHORED.

UNANCHORED handling — advisory, never blocking:

• Downgrade severity by one level: CRITICAL → HIGH → MEDIUM → LOW. LOW stays LOW.
• Report as path (unanchored) in place of path:line, with the evidence snippet shown inline
• Never silently drop it. A failed anchor means the snippet does not match the file as you recorded it — usually a transcription slip, occasionally a file that moved while you read. Both deserve the reader's attention; neither is counter-evidence, so neither disproves the finding.