---
name: rune-review
description: Rune review workflow (compiled from Rune kit)
tags: [rune, review]
---

# rune-review

> Rune L2 Skill | development


# review

> **RUNE COMPLIANCE**: Before ANY code response, you MUST:
> 1. Classify this request (CODE_CHANGE | QUESTION | DEBUG | REVIEW | EXPLORE)
> 2. Route through the correct Rune skill (see skill-router routing table)
> 3. Follow the skill's workflow — do NOT freelance or skip steps
> Violation: writing code without skill routing = incorrect behavior.

## Platform Constraints

- MUST NOT: Never run commands containing hardcoded secrets, API keys, or tokens. Scan all shell commands for secret patterns before execution.
- SHOULD: Monitor your context usage. If working on a long task, summarize progress before context fills up.
- MUST: Before summarizing/compacting context, save important decisions and progress to project files.
- SHOULD: Before ending, save architectural decisions and progress to .rune/ directory for future sessions.

## 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 the rune-fix rule file, untested code goes to the rune-test rule file, security-critical code goes to the rune-sentinel rule file.

<HARD-GATE>
A review that says "LGTM" or "code looks good" without specific file:line references is NOT a review.
Every review MUST cite at least one specific concern, suggestion, or explicit approval per file changed.
</HARD-GATE>

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

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

## 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` ← `fix` — complex fix requests self-review
- `review` → `sentinel` — security-critical code → sentinel deep scan

## Execution

### Step 1: Scope

Determine what to review.

- If triggered by a commit or PR: use run a shell command 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 the file on each named file
- If context is unclear: use `the rune-scout rule file` to identify all files touched by the change
- List every file in scope before proceeding — do not review files outside the stated scope

### 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 (the rune-adversary rule file) even in quick triage |

<HARD-GATE>
Modifying a symbol with 50+ callers + HIGH severity change (logic, types, behavior) → adversarial analysis REQUIRED. Quick review is NOT sufficient for high-blast-radius changes.
</HARD-GATE>

### 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 the file 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 with file path, line number, and severity

**Common patterns to flag:**

```typescript
// BAD — missing await causes race condition
async function saveUser(data) {
  db.users.create(data); // caller proceeds before save completes
  return { success: true };
}
// GOOD
async function saveUser(data) {
  await db.users.create(data);
  return { success: true };
}
```

```typescript
// BAD — null deref crash
function getUsername(user) {
  return user.profile.name.toUpperCase(); // crashes if profile or name is null
}
// GOOD — safe access
function getUsername(user) {
  return user?.profile?.name?.toUpperCase() ?? 'Anonymous';
}
```

### Step 3: Pattern Check

Check consistency with project conventions.

- Compare naming against existing codebase patterns (Search file contents 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

**Common patterns to flag:**

```typescript
// BAD — mutation
function addItem(cart, item) {
  cart.items.push(item); // mutates in place
  return cart;
}
// GOOD — immutable
function addItem(cart, item) {
  return { ...cart, items: [...cart.items, item] };
}
```

```typescript
// BAD — any defeats TypeScript's purpose
function process(data: any): any {
  return data.items.map((i: any) => i.value);
}
// GOOD — typed
function process(data: { items: Array<{ value: string }> }): string[] {
  return data.items.map(i => i.value);
}
```

### 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 `the rune-sentinel rule file` for deep scan
- Sentinel escalation is mandatory — do not skip it for auth or crypto code

### 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 5: Test Coverage

Identify gaps in test coverage.

- Run a shell command to check if a test file exists for each changed file
- Find files by pattern 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 `the rune-test rule file` with specific instructions on what to test
- Flag as HIGH if business logic is untested, MEDIUM if utility code is untested

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

Produce a structured severity-ranked report.

**Before reporting, apply confidence filter:**
- Only report findings with >80% confidence it is a real issue
- 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
- Include file path and line number for every finding
- Include a Positive Notes section (good patterns observed)
- Include a Verdict: APPROVE | REQUEST CHANGES | NEEDS DISCUSSION

### Step 6.5: Fix-First Triage

> From gstack (garrytan/gstack, 50.9k★): "Reviews that produce 20 findings and delegate all to the user waste everyone's time."

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

| Category | Auto-Fix? | Examples |
|----------|-----------|---------|
| Dead imports, unused variables | AUTO-FIX | `import { foo } from './bar'` where foo is never used |
| Missing error handling on obvious paths | AUTO-FIX | `await fetch()` without try/catch in production code |
| Console.log in production code | AUTO-FIX | Remove `console.log` from non-CLI production files |
| Architectural concern, trade-off | ASK | "This bypasses the auth middleware — intentional?" |
| Ambiguous intent | ASK | "Is this fallback behavior correct for null users?" |
| Style/convention disagreement | ASK | "Project uses camelCase but this file uses snake_case" |

**After classification:**
- Apply AUTO-FIX findings directly via `the rune-fix rule file` — 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 can't cite a specific file:line or convention that justifies the code, flag it as UNVERIFIED — don't rationalize away uncertainty.

### Step 6.6: Scope Drift Detection

> From gstack (garrytan/gstack, 50.9k★): "Intent vs diff catches scope creep that plan-based guards miss."

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:

| Result | Meaning | Action |
|--------|---------|--------|
| **CLEAN** | All changed files serve the stated intent | Note in report |
| **DRIFT** | 1-2 files changed that don't relate to stated intent | WARN — "These files were modified but aren't mentioned in the task: [list]" |
| **REQUIREMENTS_MISSING** | Stated intent mentions files/features not in the diff | WARN — "Task mentions X but it's not in the diff" |

**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 `the rune-fix rule file` immediately with the finding details
- If any HIGH findings: call `the rune-fix rule file` with the finding details
- If untested code: call `the rune-test rule file` with specific coverage gaps identified
- Call `neural-memory` (Capture Mode) to save any novel code quality patterns or recurring issues found.

## Framework-Specific Checks

Apply **only** if the framework is detected in the changed files. Skip if not relevant.

**React / Next.js** (detect: `import React` or `.tsx` files)
- `useEffect` with missing dependencies (stale closure) → flag HIGH
- List items using index as key on reorderable lists: `key={i}` → flag MEDIUM
- Props drilled through 3+ levels without Context or composition → flag MEDIUM
- Client-side hooks (`useState`, `useEffect`) in Server Components (Next.js App Router) → flag HIGH

**Node.js / Express** (detect: `import express` or `require('express')`)
- Missing rate limiting on public endpoints → flag MEDIUM
- `req.body` passed directly to DB without validation schema → flag HIGH
- Synchronous operations blocking the event loop inside async handlers → flag HIGH

**Python** (detect: `.py` files with `django`, `flask`, or `fastapi` imports)
- `except:` bare catch without specific exception type → flag MEDIUM
- Mutable default arguments: `def func(items=[])` → flag HIGH
- Missing type hints on public functions (if project uses mypy/pyright) → flag LOW

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

**AI_ANTIPATTERN — Purple/indigo default accent with no domain justification:**
```tsx
// 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):**
```tsx
// 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):**
```tsx
// 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:**
```tsx
// 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 — Missing UI states (only happy path rendered):**
```tsx
// 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):**
```tsx
// 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

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

| Score | Grade | Verdict |
|-------|-------|---------|
| 90–100 | Excellent | APPROVE |
| 75–89 | Good | APPROVE with notes |
| 60–74 | Fair | REQUEST CHANGES (MEDIUM issues) |
| 40–59 | Poor | REQUEST CHANGES (HIGH issues present) |
| 0–39 | Critical | REQUEST CHANGES (CRITICAL present) |

**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]
- **Overall**: APPROVE | REQUEST CHANGES | NEEDS DISCUSSION

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

### CRITICAL
- `path/to/file.ts:42` — [description of critical issue]

### HIGH
- `path/to/file.ts:85` — [description of high-severity issue]

### MEDIUM
- `path/to/file.ts:120` — [description of medium issue]

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

### Positive Notes
- [good patterns observed]

### Verdict
[Summary and recommendation]
```

### 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 reference specific file:line for every finding
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 the rune-test rule file

## Returns

| Artifact | Format | Location |
|----------|--------|----------|
| Code review report | Markdown | inline (chat output) |
| Severity-ranked findings | Markdown table | inline |
| Spec compliance verdict | Markdown | inline |
| Composite quality score | Markdown table | inline (when `mode: "scored"`) |
| Blast radius assessment | Markdown table | inline |

## Sharp Edges

| Failure Mode | Severity | Mitigation |
|---|---|---|
| Finding flood — 20+ findings overwhelm developer | MEDIUM | Confidence filter: only >80% confidence, consolidate similar issues per file |
| "LGTM" without file:line evidence | HIGH | HARD-GATE blocks this — cite at least one specific item per changed file |
| Expanding review scope beyond the diff | MEDIUM | Limit to `git diff` scope — do not creep into adjacent unchanged files |
| Security finding without sentinel escalation | HIGH | Any auth/crypto/payment code touched → MUST call the rune-sentinel rule file |
| Skipping UI anti-pattern checks for frontend changes | MEDIUM | Any .tsx/.jsx/.svelte/.vue in diff → MUST run UI/UX Anti-Pattern Checks section |
| Skipping spec compliance check (Step 5.5 Stage 1) | HIGH | Code quality without spec check ships clean code that does the wrong thing — always load the plan/ticket before reviewing quality |
| Treating purple/indigo accent as "just a color choice" | MEDIUM | It is a documented AI-generated UI signature — always flag for domain justification |
| Suggesting "add X" without checking if X is used | MEDIUM | YAGNI pushback: grep codebase for the suggested feature → if uncalled anywhere → respond "Not called anywhere. Remove? (YAGNI)". Valid pushback, not laziness |
| Adding abstractions "for future flexibility" | MEDIUM | Three similar lines > premature abstraction. Only abstract when there are 3+ concrete callers today |
| Missing cross-phase integration check at phase boundary | MEDIUM | When reviewing a phase completion: check orphaned exports, uncalled routes, auth gaps, E2E flow continuity. Delegate to completion-gate Step 4.5 |
| Review loop exceeds 3 iterations without resolution | MEDIUM | Cap at 3 review loops. After 3rd iteration with unresolved findings → surface to user with "these findings persist after 3 fix attempts — needs human decision" |
| Auto-fixing something that should have been ASK | HIGH | When in doubt, ASK. AUTO-FIX only for mechanical issues (dead imports, console.log). Anything involving intent or trade-offs = ASK |
| Scope drift flagged on intentional refactoring | LOW | Scope drift is informational, not blocking. User can override with "intentional" — don't re-flag after override |

## Done When

- All changed files in the diff read and analyzed
- Every finding references specific file:line with severity label
- 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

## Cost Profile

~3000-6000 tokens input, ~1000-2000 tokens output. Sonnet default, opus for security-critical reviews. Runs once per implementation cycle.

---
> **Rune Skill Mesh** — 59 skills, 200+ connections, 14 extension packs
> [Landing Page](https://rune-kit.github.io/rune) · [Source](https://github.com/rune-kit/rune) (MIT)
> **Rune Pro** ($49 lifetime) — product, sales, data-science, support packs → [rune-kit/rune-pro](https://github.com/rune-kit/rune-pro)
> **Rune Business** ($149 lifetime) — finance, legal, HR, enterprise-search packs → [rune-kit/rune-business](https://github.com/rune-kit/rune-business)