Imported from Rune-kit/rune (skills/review/SKILL.md). Install upstream with npx skills add Rune-kit/rune --skill review. Copyright stays with the author.

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)

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.

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:

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:

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:

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

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?"

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

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.

Anchoring resolves a finding; it does not filter one. Only the Falsification Pass drops findings — a survivor of that pass always reaches the report, anchored or not.

Then, before reporting:

• Consolidate similar issues: "8 functions missing error handling in src/services/" — not 8 separate findings

• Skip stylistic preferences unless they violate conventions found in .eslintrc, CLAUDE.md, or CONTRIBUTING.md

• Adapt to project type: a console.log in a CLI tool is fine; in a production API handler it is not

• Group findings by severity: CRITICAL → HIGH → MEDIUM → LOW

• Give every finding its path:line from the Anchor Pass — or path (unanchored) — plus the evidence block underneath

• Include a Positive Notes section (good patterns observed)

• Include a Verdict: APPROVE | REQUEST CHANGES | NEEDS DISCUSSION

Step 6.5: Fix-First Triage

A review that produces 20 findings and delegates every one of them back to the user has moved work, not done it.

Classify each finding as AUTO-FIX or ASK before reporting:

After classification:

• Apply AUTO-FIX findings directly via rune:fix — include all in a single batch
• Collect ASK findings into ONE AskUserQuestion — not 5 separate questions
• Report both: "Auto-fixed 4 issues. 2 findings need your input: [...]"

Rationalization prevention: "This looks fine" is NOT acceptable without evidence. If you cannot cite a specific file:line or convention that justifies the code, do not wave it through — report it as an ASSUMED finding naming the premise you could not check.

This is the same asymmetry as the Falsification Pass, applied in the other direction: uncertainty never justifies dropping a finding, and it never justifies clearing code either. Both resolve by reading, or by saying plainly what was not read.

Step 6.6: Scope Drift Detection

Comparing stated intent against the actual diff catches scope creep that plan-based guards miss — the plan was right, the diff simply grew past it.

After reviewing code, compare stated intent vs actual diff:

1. Read the originating source: TODO list, PR description, commit messages, or plan file
2. Extract stated intent: "what was this change supposed to do?"
3. Run git diff --stat to see actual file changes
4. Compare:

This is informational, not blocking. Scope drift is common and sometimes intentional — but making it visible prevents silent creep.

After reporting:

• If any CRITICAL findings: call rune:fix immediately with the finding details
• If any HIGH findings: call rune:fix with the finding details
• If untested code: call rune:test with specific coverage gaps identified
• Call neural-memory (Capture Mode) to save any novel code quality patterns or recurring issues found.

UI/UX Anti-Pattern Checks

Apply only when .tsx, .jsx, .svelte, .vue, or .html files are in the diff. Skip for backend-only changes.

These are the "AI UI signature" — patterns that make AI-generated frontends visually identifiable as non-human-designed. Flag each as MEDIUM severity.

Preamble — load design contract first: If .rune/design-system.md exists, read it first. Pull the project's Scale Minimums block (if authored by rune:design v0.5.0+) and apply those thresholds instead of the defaults below. Missing design-system.md → use defaults and add a LOW finding: "Project has no design-system.md — run rune design to lock visual decisions." Never enforce stale defaults against a project that has already declared stricter/looser minimums.

AI_ANTIPATTERN — Purple/indigo default accent with no domain justification:

// BAD: LLM default color bias — signals "AI-generated" to experienced designers
className="bg-indigo-600 text-white"  // every button/CTA is indigo
// GOOD: domain-appropriate — trading → neutral dark, healthcare → trust blue,
//        e-commerce → conversion-optimized warm. Purple is only appropriate for
//        AI-native tools and creative platforms.

AI_ANTIPATTERN — Card-grid monotony (every section is 3-col cards, zero layout variation):

// BAD: every section uses the same grid pattern
<div className="grid grid-cols-3 gap-6">  // features
<div className="grid grid-cols-3 gap-6">  // testimonials
<div className="grid grid-cols-3 gap-6">  // pricing
// GOOD: mix layouts — split sections, bento grids, full-bleed hero, list+detail

AI_ANTIPATTERN — Centeritis (everything centered, no directional flow):

// BAD: no visual tension, no reading direction
<div className="text-center flex flex-col items-center">  // hero
<div className="text-center">  // every feature section
// GOOD: left-align body copy, use centering intentionally for hero/CTAs only

AI_ANTIPATTERN — Numeric/financial values in non-monospace font:

// BAD: prices, stats, metrics in Inter/Roboto
<span className="text-2xl font-bold">${price}</span>
// GOOD: monospace for all numbers that need alignment
<span className="font-mono text-2xl font-bold">${price}</span>

AI_ANTIPATTERN — Scale Minimum violations (AI boilerplate tell):

// BAD: body text at 14px (AI default) — primary content must be ≥16px
<p className="text-sm">Welcome to the dashboard.</p>

// BAD: hero/display text below 40px — reads as "section heading", not "hero"
<h1 className="text-3xl font-bold">Ship Faster</h1>  // 30px

// BAD: touch target below 44×44px on mobile
<button className="w-8 h-8"><XIcon /></button>  // 32px — WCAG 2.5.8 failure

// GOOD: hero ≥48px, body ≥16px, touch ≥44×44px
<h1 className="text-5xl md:text-6xl font-bold">Ship Faster</h1>   // 48-60px
<p className="text-base">Welcome to the dashboard.</p>             // 16px
<button className="w-11 h-11"><XIcon /></button>                   // 44px

Pull project-specific overrides from .rune/design-system.md § Scale Minimums.

AI_ANTIPATTERN — Hand-rolled SVG for standard iconography:

// BAD: custom <svg> for dashboard/menu/close/chevron — AI geometry almost always malformed
<svg viewBox="0 0 24 24"><path d="M3 3h18v18H3z M3 9h18 M9 3v18"/></svg>

// GOOD: Phosphor Icons (preferred) or Huge Icons
import { House, List, X } from '@phosphor-icons/react';
<House weight="bold" size={24} />

// GOOD: labeled placeholder when no icon library available yet
<span className="icon-placeholder" aria-label="Dashboard icon — design pass needed">
  [ ICON: dashboard ]
</span>

Exceptions: inline SVG for project-unique logos, data visualizations (charts/graphs), or decorative illustrations generated by a human designer — these are not "standard iconography."

AI_ANTIPATTERN — Manual hex shading for accent states (oklch() violation):

/* BAD: hand-darkened hex — breaks perceived lightness consistency */
--accent: #3b82f6;
--accent-hover: #2563eb;    /* guessed darker */
--accent-pressed: #1d4ed8;  /* guessed even darker */

/* GOOD: relative oklch() derivation */
--accent: oklch(62% 0.19 258);
--accent-hover:   oklch(from var(--accent) calc(l - 0.08) c h);
--accent-pressed: oklch(from var(--accent) calc(l - 0.15) c h);
--accent-subtle:  oklch(from var(--accent) calc(l + 0.3) calc(c * 0.4) h);

Flag any CSS file defining 2+ hover/pressed/active variants with sibling hex literals. Not a finding if accent uses a design-token library (Radix Colors, Tailwind palette) that already ships perceptually-tuned scales.

AI_ANTIPATTERN — Missing UI states (only happy path rendered):

// BAD: data rendering without empty/error/loading states
{data.map(item => <Card key={item.id} {...item} />)}
// GOOD: all 4 states covered
{isLoading && <CardSkeleton />}
{error && <ErrorState message={error.message} />}
{!data.length && <EmptyState />}
{data.map(item => <Card key={item.id} {...item} />)}

Accessibility — flag as HIGH (these are WCAG 2.2 failures):

// BAD: icon button with no accessible name
<button onClick={close}><XIcon /></button>
// GOOD
<button onClick={close} aria-label="Close dialog"><XIcon aria-hidden="true" /></button>

// BAD: placeholder as label
<input placeholder="Email address" type="email" />
// GOOD
<label htmlFor="email">Email address</label>
<input id="email" type="email" />

// BAD: removes focus ring without replacement
className="focus:outline-none"
// GOOD: must have focus-visible replacement
className="focus:outline-none focus-visible:ring-2 focus-visible:ring-blue-500"

// BAD: color as sole information conveyor
<span className="text-red-500">{errorMessage}</span>
// GOOD: icon + color + text
<span className="text-red-500 flex gap-1"><ErrorIcon aria-hidden />Error: {errorMessage}</span>

WCAG 2.2 New Rules — flag as MEDIUM:

• position: sticky or position: fixed header/footer without scroll-padding-top → Focus Not Obscured (2.4.11)
• Interactive elements with width < 24px or height < 24px without 8px spacing → Target Size (2.5.8)
• Multi-step form re-asking for previously entered data → Redundant Entry (3.3.7)

Platform-Specific — flag as MEDIUM when platform is detectable:

• iOS target: solid-background cards (iOS 26 Liquid Glass deprecates this visual language) — should use translucent/blur surfaces
• Android target: hardcoded hex colors instead of MaterialTheme.colorScheme tokens → not adaptive to dynamic color

Motion Craft Checks

Apply only when the diff touches motion. Strong signals: @keyframes, motion., animate={, useSpring, cubic-bezier, @starting-style, transition: / transition-. Weak signals: bare transform or ease- — these also match static layout transforms (translate(-50%,-50%)) and non-motion identifiers, so treat them as a trigger only when they co-occur with a strong signal. Skip entirely for diffs with no motion code.

These are advisory — default severity MEDIUM. Escalate to HIGH only for feel-breaking regressions (the first five triggers below). Motion is a taste call: when feel can't be judged from source, say so and recommend a slow-motion / frame-by-frame check rather than asserting a defect. For every finding, cite the exact remediation value from skills/design/MOTION-CRAFT.md — do not restate its tables here.

Escalation triggers — flag on sight (first five = HIGH, feel-breaking):

• ease-in on an entrance or arrival (delays the moment the user watches most) — not a finding on something departing the frame or falling, where ease-in is correct (MOTION-CRAFT §3). Also catches ease-in hiding inside a composed curve (smootherstep starts at zero velocity)
• scale(0) or pure-fade entrances with no initial transform (nothing appears from nothing → initial scale + opacity per MOTION-CRAFT §5)
• Animation on a keyboard shortcut / command-palette toggle / 100+/day action (should have none)
• Animating layout properties (width/height/margin/padding/top/left) instead of transform/opacity (off-GPU, drops frames)
• UI animation duration over the budget with no stated reason (duration budget + modals/drawers exemption per MOTION-CRAFT §4)
• transition: all — unbounded property animation (MEDIUM)
• transform-origin: center on a trigger-anchored popover/dropdown/tooltip (should scale from trigger; modals exempt) (MEDIUM)
• Keyframes on toasts/toggles/anything triggered rapidly (can't retarget → use CSS transitions or springs) (MEDIUM)
• Framer Motion x/y/scale shorthands on motion that runs while the page is busy (not hardware-accelerated → full transform string) (MEDIUM)
• Driving a child transform via a CSS variable on the parent (style-recalc storm) (MEDIUM)
• Missing prefers-reduced-motion handling on movement, or ungated :hover motion (missing @media (hover: hover) and (pointer: fine)) (MEDIUM)
• A reduced-motion branch that is present but cannot work — the guard existing is not the finding; these are (MOTION-CRAFT §10). Do not close this check on a grep hit: touches ≠ reachable ≠ correct (HIGH):
  • Dead branch — an outer if (reduced) return / if (!shouldAnimate) return that runs before the helper applying the end state. Reduced users get the raw default DOM. Greps clean
  • One-shot matchMedia(...).matches read at mount with no change listener — preference toggled mid-session leaves the page on the old path
  • duration: 0 on sequence-as-content — every key pose applied in one frame, stacking mid-poses instead of landing on the destination
  • 0s while something awaits transitionend / animationend / onComplete — engines may drop completion events for true zero-length transitions and the state machine hangs; ~0.01ms is the fix
  • JS motion ungated — CSS has the media query, GSAP/Motion/Lenis/canvas keep running. A media query cannot stop a JS timeline
  • Blanket * { animation-duration: 0 !important } as the only handling — over-removes focus-ring and loading cues that should survive reduce
• Curve chosen by name for a large rotation or long translate, with no peak-speed check — peak = maxSlope × travel / duration against a ~30°/frame ceiling (a third of the pitch for a rotationally symmetric part). --ease-out-strong on a spinner is the common instance (MOTION-CRAFT §3) (MEDIUM)
• Both segments of a two-segment keyframe move eased — the element dead-stops at the join (MOTION-CRAFT §3, velocity continuity) (MEDIUM)
• Animated SVG passing through SVGO without cleanupIds / mergePaths / removeHiddenElems / removeViewBox / inlineStyles disabled — source correct, build output broken (MOTION-CRAFT §14) (HIGH)
• Transform on a root <svg> or HTML wrapper with a literal px value not derived from the rendered size — breaks at every size but the authored one (MOTION-CRAFT §14) (MEDIUM)
• Animated SVG with an element resting state declared only in the 0% keyframe — snaps to a broken pose the moment animation: none applies under reduce (MOTION-CRAFT §14) (MEDIUM)
• Reusable animated SVG with hardcoded url(#id) mask/clipPath references and no per-instance id rewrite — every copy on the page renders the first one's mask (MOTION-CRAFT §14) (MEDIUM)
• Symmetric enter/exit timing on a press-and-release or hold interaction (deliberate phase should be slower, response snappier) (LOW–MEDIUM)
• Everything-at-once entrance where a stagger belongs (stagger interval per MOTION-CRAFT §11) (LOW)

Remedial preference (propose the earliest that applies): delete the animation (high-frequency / no purpose) → reduce it → fix the easing → fix origin/physicality → make it interruptible → move it to the GPU → asymmetric timing → polish (blur/stagger/@starting-style/spring). Pull the exact curve, duration, or spring config from MOTION-CRAFT.md. Findings feed the existing Report + Fix-First triage — do not create a parallel output path.

Weighted Composite Scoring

When a review is part of a recurring quality-gate cycle (e.g., sprint review, pre-release gate), produce a composite quality score alongside the findings list. This makes review output numeric and comparable across runs.

Formula

Quality Score = (Correctness × 0.35) + (Security × 0.30) + (Test Coverage × 0.20) + (Conventions × 0.15)

Each dimension is scored 0–100 based on findings count and severity:

• 0 CRITICAL/HIGH findings → 100 for that dimension
• 1 CRITICAL → dimension capped at 40
• 1 HIGH → dimension capped at 70
• Each additional MEDIUM → subtract 5 (floor: 50)

Grade Thresholds

When to include: Only when mode: "scored" is passed by the caller, or when invoked by audit. Default review output uses the standard severity-ranked report without the score.

Severity Levels

CRITICAL  — security vulnerability, data loss risk, crash bug
HIGH      — logic error, missing validation, broken edge case
MEDIUM    — code smell, performance issue, missing error handling
LOW       — style inconsistency, naming suggestion, minor refactor opportunity

Output Format

## Code Review Report
- **Files Reviewed**: [count]
- **Findings**: [count by severity]
- **Review Commit**: [git hash at time of review]
- **Council**: [not invoked | MULTI_FAMILY (N families) | NO_DECORRELATION — same-family subagents only]
- **Unanchored**: [count — findings whose evidence did not resolve to a line]
- **Overall**: APPROVE | REQUEST CHANGES | NEEDS DISCUSSION

### Spec Compliance
- [PASS/FAIL]: [acceptance criteria coverage]

### CRITICAL
- `src/auth/session.ts:42` — [OBSERVED] loose equality on the session token: a request with no token and a session with no token are both `undefined`, and `undefined == undefined` is true, so an anonymous caller is granted that session's user
  ```ts
  if (req.token == session.token) return grant(session.user);
  ```

### HIGH
- `src/db/users.ts:85` — [DERIVED] create is not awaited, so the handler returns success before the write lands; a failed insert is reported to the client as 201
  ```ts
  db.users.create(data);
  return { success: true };
  ```

### MEDIUM
- `src/cache/store.ts (unanchored)` — [ASSUMED: caller in worker.ts not read] cache write has no TTL; if the worker path also writes this key the entry never expires
  ```ts
  cache.set(key, value)
  ```

### Blast Radius
- [High-impact symbols with caller counts]

### Positive Notes
- [good patterns observed]

### Verdict
[Summary and recommendation]

### Follow-ups (outside this scope)
- `other/file.ts` — [one line, no severity, does not affect the verdict; omit section if none]

Read the MEDIUM entry above as the shape of an honest weak finding: unanchored (so path (unanchored), already downgraded one level), ASSUMED with its unchecked premise named, and still reported — because none of that is counter-evidence.

Every finding carries its claim type from the Falsification Pass and its evidence block from the Evidence Contract. ASSUMED findings name the premise that was not checked and never appear under CRITICAL. A finding over the 5-line evidence cap is a design comment — it ships without a block, at MEDIUM or below. The Follow-ups section is the only place an out-of-scope observation may appear.

Review Staleness Detection

Track the git commit hash at review time. If code changes after review → review is STALE.

Review commit: abc123 → Code changed to def456 → Review is STALE, re-review required

When cook or ship checks review status: compare review commit hash with current HEAD. If different → WARN: "Review is stale — code changed since last review."

Constraints

1. MUST read the full diff — not just the files the user pointed at
2. MUST give every finding a verbatim evidence snippet, and resolve its line via the Anchor Pass rather than recall — an unresolved finding is reported (unanchored) and downgraded, never renumbered by guess
3. MUST NOT rubber-stamp with generic praise ("well-structured", "clean code") without evidence
4. MUST check: correctness, security, performance, conventions, test coverage
5. MUST categorize findings: CRITICAL (blocks commit) / HIGH / MEDIUM / LOW
6. MUST escalate to sentinel if auth/crypto/secrets code is touched
7. MUST flag untested code paths and recommend tests via rune:test

Returns

Chain Metadata

Append to Code Review Report when invoked standalone. Suppress when called as sub-skill inside an L1 orchestrator (cook, team, etc.) — the orchestrator emits a consolidated block. See docs/references/chain-metadata.md.

chain_metadata:
  skill: "rune:review"
  version: "1.5.0"
  status: "[DONE | DONE_WITH_CONCERNS]"
  domain: "[area reviewed]"
  files_changed: []  # review doesn't change files
  exports:
    findings_count: { critical: [N], high: [N], medium: [N], low: [N] }
    findings:
      - { severity: "[level]", file: "[path]", line: [N or null when unanchored], anchored: [true | false], evidence: "[verbatim snippet, ≤5 lines]", message: "[issue]", claim_type: "[OBSERVED | DERIVED | ASSUMED]" }
    verdict: "[APPROVE | REQUEST_CHANGES | NEEDS_DISCUSSION]"
    quality_score: [0-100]  # when mode: "scored"
  suggested_next:
    - skill: "rune:fix"
      reason: "[grounded in findings — e.g., '2 HIGH findings in api/users.ts need remediation']"
      consumes: ["findings"]

Sharp Edges

Done When

• All changed files in the diff read and analyzed
• Rule files matching the diff's extensions loaded (plus default.md), and no others
• Every finding carries a verbatim evidence snippet and a severity label, with its line resolved by the Anchor Pass or marked (unanchored) and downgraded
• Security-critical code escalated to sentinel (or confirmed not present)
• Test coverage gaps identified and documented
• UI anti-pattern checks ran for any frontend files in diff (or confirmed not applicable)
• Structured report emitted with APPROVE / REQUEST CHANGES / NEEDS DISCUSSION verdict
• (council) If blast radius 50+ with HIGH severity: council invoked on the diff, decorrelation stamp reported plainly, consensus/dissent folded into findings

Cost Profile

~3000-6000 tokens input, ~1000-2000 tokens output. Sonnet default, opus for security-critical reviews. Runs once per implementation cycle. When Step 1.6 fires (high-blast-radius only): add council's cost profile (~1500-4000 tokens per voice × 2-5 voices) — reserved for the same tier that already requires adversarial escalation.