Pre-Merge Review

You are doing a final structural review before this branch lands. Read the current branch's diff against its base and find the problems that pass CI but bite in production.

Posture: a staff engineer with firm opinions about data safety, concurrency, and trust boundaries. Finding issues isn't enough — you fix the mechanical ones yourself and surface the ambiguous ones as decisions. Every finding ends in an action. (Default is fix-first; if the user asked for report-only up front, surface every finding as a recommendation and edit nothing.)

Gather context first

This skill ships standalone, so it can't assume your project's conventions, house style, or preferences the way it could for its author. Before doing the main work:

1. Auto-detect what you safely can from the repo — language/stack, base branch, build/test commands, existing config and docs. Never ask for something you can read for yourself.
2. Ask, don't assume, for the rest. Where an input, convention, or preference would change the result and you can't reliably detect it, ask ONE concise AskUserQuestion (put a sensible default first, labelled Recommended) instead of guessing. The user has less context than this skill's author assumed — a wrong silent default is worse than a quick question. Don't ask about things you can detect, and don't ask more than you need.

For this skill, confirm up front (only the items you can't already detect):

• Base / diff target — Step 0 auto-detects the base (PR base → default branch → origin/HEAD → main). If that detection is ambiguous — main vs develop, multiple long-lived branches, or you actually meant a specific PR / commit / ref — confirm which base to diff against rather than guessing.
• Fix-first vs report-only — the default posture edits source (auto-fixing mechanical issues, asking before risky ones). If you'd rather it stay read-only and only report findings, say so up front.
• Where findings land — the default home is a ## Staff Review Findings section in REVIEWED.md. Confirm if you'd prefer a different file, or just an inline summary in this conversation.
• Severity bar / strictness — how aggressive to be: surface only high-confidence structural blockers, or also include lower-confidence and stylistic items (the confidence gates default to suppressing anything below 3).

How this runs

This skill runs in your current conversation. Its interactive spine — the Step 4 fix-first decisions, the batched AskUserQuestion for genuinely ambiguous items, the HIGH-impact discrepancy escalation, and every file edit — needs you and the filesystem directly, so it always stays in this context.

The analytical passes (the specialist lenses in Step 3.5, the Red Team pass, and the always-on adversarial pass in Step 7) are independent reads over the same diff. By default they run inline, right here — walk each one as a focused, fresh pass without leaving the conversation. On a large job you can instead fan them out to parallel subagents, but only after asking. Before spawning any subagent or Workflow, stop and ask the user, for example:

This is large ([N files / big diff], [K specialist lenses selected]). I can review it inline here (cheaper, slower) or fan out [M] parallel subagents (faster, more tokens). Which do you want?

Spawn subagents/workflows only after an explicit yes. If the user declines or doesn't answer, do the whole job inline.

If you do fan out: background agents and Workflow agent() calls cannot call AskUserQuestion, cannot talk to the user, and cannot write files. So they do headless compute only — analyze the diff, return structured findings — while every fix-first decision, batched ASK question, HIGH-impact escalation, and file write (auto-fixes, approved fixes, test stubs, and the REVIEWED.md "## Staff Review Findings" section) stays in this main context after they return. Set model: 'opus' on every agent() call so a lighter tier never leaks in, and pass data through args (Workflow scripts have no Date.now/Math.random/filesystem access — timestamps and writes happen here). Whether inline or fanned out, the merge/classify/fix-first flow is identical; only the waiting differs.

Arguments

Inspect $ARGUMENTS:

• A base branch / ref / diff target (main, develop, a PR number, a commit ref) → use it as <base>, overriding the Step 0 auto-detection.
• A force flag — --security, --performance, --testing, --maintainability, --data-migration, --api-contract, --design, or --all-specialists → force-include that specialist in Step 3.5.
• Empty → auto-detect the base per Step 0 and diff the current branch against it.

Step 0: Detect base branch

1. Is there a PR? gh pr view --json baseRefName -q .baseRefName
2. No PR — get the default branch: gh repo view --json defaultBranchRef -q .defaultBranchRef.name
3. Fallback: git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's|refs/remotes/origin/||'
4. Last resort: main

Print the detected base. If detection was ambiguous — it fell through to the main last resort, or several long-lived branches could plausibly be the base — confirm with the user before diffing instead of silently assuming. Use the agreed base as <base> everywhere below.

Step 1: Check branch

1. git branch --show-current to learn where you are.
2. If you're sitting on the base branch, print "Nothing to review — you're on the base branch." and stop.
3. git fetch origin <base> --quiet && git diff origin/<base> --stat. If the diff is empty, print the same message and stop.

Step 1.5: Scope Drift Detection

Before judging code quality, answer one question: did they build what was asked — no more, no less?

1. Read TODOS.md if present. Read the PR description (gh pr view --json body --jq .body 2>/dev/null || true). Read commit messages (git log origin/<base>..HEAD --oneline).
2. Pin down the stated intent — what was this branch supposed to do?
3. git diff origin/<base>...HEAD --stat and weigh the files touched against that intent.

4. Read it skeptically:

   SCOPE CREEP:

   • Files changed that have nothing to do with the stated intent
   • New features or refactors the plan never mentioned
   • "While I was in there..." edits that widen the blast radius

   MISSING REQUIREMENTS:

   • TODOS.md / PR-description requirements the diff never touches
   • Test coverage absent for stated requirements
   • Half-finished work — started but not landed

5. Emit this before the main review:

   Scope Check: [CLEAN / DRIFT DETECTED / REQUIREMENTS MISSING]
   Intent: <one line — what was requested>
   Delivered: <one line — what the diff actually does>
   [If drift: each out-of-scope change]
   [If missing: each unaddressed requirement]

6. This is INFORMATIONAL and never blocks. Continue.

Plan File Discovery

Look for plan files left by an earlier /scope-review or /plan-review session. Those skills write SCOPE_REVIEW_<ref>.md and PLAN_REVIEW_<ref>.md to the project root; /design-review writes DESIGN_REVIEW_<ref>.md. A repo may also keep ad-hoc plans under .claude/plans/.

BRANCH=$(git branch --show-current 2>/dev/null | tr '/' '-')
PLAN=""
for PLAN_GLOB in SCOPE_REVIEW_*.md PLAN_REVIEW_*.md DESIGN_REVIEW_*.md .claude/plans/*.md; do
  P=$(ls -t $PLAN_GLOB 2>/dev/null | xargs grep -l "$BRANCH" 2>/dev/null | head -1)
  [ -z "$P" ] && P=$(ls -t $PLAN_GLOB 2>/dev/null | head -1)
  [ -n "$P" ] && PLAN="$P" && break
done
[ -n "$PLAN" ] && echo "PLAN_FILE: $PLAN" || echo "NO_PLAN_FILE"

If a plan file turns up, read its first 20 lines and confirm it actually maps to this branch. If it belongs to a different effort, treat it as no plan file.

Actionable Item Extraction

With a plan file in hand, pull out every actionable item:

• Checkboxes: - [ ] ... / - [x] ...
• Numbered steps under implementation headings: "1. Create ...", "2. Add ..."
• Imperatives: "Add X to Y", "Create a Z service", "Modify the W controller"
• File specs: "New file: path/to/file.ts", "Modify path/to/existing.rb"
• Test requirements: "Test that X", "Add test for Y"
• Data-model changes: "Add column X to table Y", "Create migration for Z"

Skip:

• Context / Background / Problem sections
• Open questions (?, "TBD", "TODO: decide")
• Explicitly deferred work ("Future:", "Out of scope:", "NOT in scope:", "P2:", "P3:")

Cap: 50 items max.

Tag each item with a category: CODE | TEST | MIGRATION | CONFIG | DOCS.

Cross-Reference Against Diff

For each extracted item, classify it against the diff:

• DONE — the diff clearly implements it. Cite the specific file(s).
• PARTIAL — some work exists but it's incomplete (model created, controller missing).
• NOT DONE — nothing in the diff addresses it.
• CHANGED — done a different way than the plan said, but the goal is met. Note the difference.

Be strict with DONE — a touched file isn't proof; the functionality must actually be present. Be generous with CHANGED — goal met by other means still counts.

Output:

PLAN COMPLETION AUDIT
═══════════════════════════════
Plan: {plan file path}

## Implementation Items
  [DONE]      Create UserService — src/services/user_service.rb (+142 lines)
  [PARTIAL]   Add validation — model validates but missing controller checks
  [NOT DONE]  Add caching layer — no cache-related changes in diff
  [CHANGED]   "Redis queue" → implemented with Sidekiq instead

## Test Items
  [DONE]      Unit tests for UserService — test/services/user_service_test.rb
  [NOT DONE]  E2E test for signup flow

─────────────────────────────────
COMPLETION: 4/7 DONE, 1 PARTIAL, 1 NOT DONE, 1 CHANGED
─────────────────────────────────

Investigation Depth

For each PARTIAL or NOT DONE item, dig into WHY:

1. Scan git log origin/<base>..HEAD --oneline for commits that hint the work was begun, attempted, or reverted.
2. Read the relevant code to see what got built instead.
3. Name the likely cause:
   • Scope cut — intentional removal (a revert commit, a deleted TODO)
   • Context exhaustion — started, then stalled mid-way
   • Misunderstood requirement — something exists but doesn't match the plan
   • Blocked by dependency — the item waits on something unavailable
   • Genuinely forgotten — no trace of any attempt

For each discrepancy:

DISCREPANCY: {PARTIAL|NOT_DONE} | {plan item} | {what was actually delivered}
INVESTIGATION: {likely reason with evidence from git log / code}
IMPACT: {HIGH|MEDIUM|LOW} — {what breaks or degrades if this stays undelivered}

Integration with Scope Drift

If you find HIGH-impact discrepancies, raise an AskUserQuestion (this context):

• Show the investigation findings.
• Options: A) Stop and implement the missing items, B) Ship anyway + file P1 TODOs, C) Intentionally dropped.

No plan file: fall back to commit messages and TODOS.md for intent. With no intent sources at all, skip: "No intent sources detected — skipping completion audit."

Step 2: Get the diff

git fetch origin <base> --quiet
git diff origin/<base>

This captures both committed and uncommitted changes against the latest base.

Step 3: Critical pass (core review)

Run the checklist over the diff. Every finding carries a confidence score (see Confidence Calibration).

Pass 1 — CRITICAL (highest severity)

Data Safety

• String interpolation in SQL/queries — parameterize instead
• TOCTOU races: check-then-set sequences that should be atomic
• Update calls that skip validations on constrained fields
• N+1 queries: missing eager loading for associations used in loops/views

Race Conditions & Concurrency

• Read-check-write with no uniqueness constraint or conflict handling
• Find-or-create on columns lacking a unique DB index — concurrent calls duplicate rows
• Status transitions that don't use an atomic WHERE-old-UPDATE-new — concurrent updates skip or double-apply
• dangerouslySetInnerHTML (or equivalent) on user-controlled data (XSS)

Order-of-Operations Verification (mandatory for timing claims)

Any finding whose truth depends on when code runs — races, "X fires before Y finishes", split-brain state between two callers, missing-update-propagation, before/after ordering — must cite the real trigger path. Reading the function body is not enough: two functions with identical bodies can have opposite timing if one is the API initiator and the other is wired into that API's .then(...) callback.

Do this before reporting any CRITICAL timing/race/sequencing finding:

1. Locate the call site(s). Grep every place the function is invoked. If it's exposed through a callback prop (onUpdate, onSuccess, onComplete, …), follow the prop binding to the actual caller — not just where the prop is declared.
2. Read the wrapping context at the call site. Is the call inside await, .then(...), Promise.all([...]), an event handler that already awaited an upstream promise, a useEffect cleanup, a setTimeout/setInterval, an RxJS pipe? The wrapper decides the timing.
3. State the trigger path in the finding. For example: "Function X runs at time T because it's invoked from Y:line Z, which sits inside <Promise/await/event-handler> W. So Y's async work has already resolved by the time X runs."
4. If you can't trace the path (the function is called from outside the diff, or the binding goes through bind/apply/dynamic dispatch you can't follow), downgrade from CRITICAL to INFORMATIONAL and note the path is unverified. Never assert timing without evidence.

The pattern that motivates this rule: a function named completeQueueHandler doing optimistic UI + refetch looks like the API initiator. It can just as easily be a post-API callback wired through someApi().then(() => onCompleted()). The two are indistinguishable in isolation — only the call site tells you which.

API/External Output Trust Boundary

• External API responses (LLM output included) written to the DB without format validation
• Structured responses accepted with no type/shape check before a write
• User input forwarded to external services without sanitization

Shell Injection

• exec, system, popen, subprocess.run(shell=True) fed unsanitized input
• Template strings assembling shell commands from user input

Enum & Value Completeness

When the diff adds a new enum value, status string, or type constant:

• Trace it through every consumer. Grep for everything that switches on, filters by, or renders that value. READ each hit. Any consumer that doesn't handle the new value is a flag.
• Check allowlists / filter arrays. Find arrays holding sibling values; confirm the new one is included where it needs to be.
• Check switch/case/if-else chains. Does the new value fall through to the wrong default?
• This requires reading code OUTSIDE the diff.

Production-State Simulation Test

If the diff adds code that reads or writes production state — durable storage, runtime configuration, container/topology dependencies, multi-instance class state — confirm the PR ships at least one integration test that simulates a fresh production environment: empty storage, no seeded keys, env set the way production sets it, the production wiring composition.

The underlying rule: a code path that touches durable production state should ship with an integration test that boots from a fresh production environment — empty storage, no pre-seeded keys, the same construction order and dependencies as the production entry point — not just a hand-seeded fixture. This is the "wired in tests, unwired in production" failure: the suite passes because every object was constructed with its state already in place, while production breaks because that state isn't written on the feature's first real use.

What to grep for:

• New constructor calls to stateful classes — if 2+ instances of one class exist, verify the test exercises state propagation across BOTH.
• Any default= for a value that should fall back to an env var or config file — verify the test covers the "default fired" branch (state missing on a fresh system).
• New paths writing to the filesystem or shared infrastructure — verify the test environment matches production's persistence topology, or documents why a tmp-path test suffices.

CRITICAL when:

• The feature touches boot-time recovery / rehydration AND no test covers the empty-state branch.
• A new write path targets a location production doesn't make durable AND no test surfaces the persistence question.
• A stateful class is constructed in 2+ source files AND the test only updates one instance.

SHOULD FIX when:

• All tests pass but the plan-review's Production State Coverage (Q1–Q4) enumeration is missing or incomplete in the lockdown doc.

Pass 2 — INFORMATIONAL (lower severity)

Conditional Side Effects

• Branches that perform a side effect on one path but forget it on another (records left inconsistent)
• Log lines claiming an action happened when that action was conditionally skipped

Magic Numbers & String Coupling

• Bare numeric literals reused across files — promote to named constants
• Error-message strings doubling as query filters elsewhere

Dead Code & Consistency

• Variables assigned but never read
• Comments/docstrings describing behavior the code no longer has
• Unused imports or props (cross-reference the component)

Commented-block awareness (every lens, not just this section): before flagging a commented line (e.g. // s.redis.DeleteKeysByPattern(...)), walk upward to the nearest function/block boundary. If the enclosing function header is itself commented (e.g. // func submitVisits(...) {), the line lives inside a fully-dead function:

• Don't recommend "investigate whether the enclosing function is live" — read it yourself first.
• Don't recommend restoring, tracking, or filing a follow-up on a line inside a dead function — there's no live code to fix.
• Don't propose a TODO/issue pointing at dead code — a tracked item with no live target just confuses the next reader.
• The right call is either (a) delete the dead function as a separate cleanup, or (b) ignore it.
• When you do flag something around commented code, name the enclosing function and confirm its commented status so the reviewer can verify in a single read.

Grep-driven analysis surfaces single-line matches with no block context; this rule is the correction, and it binds every specialist lens plus the adversarial pass.

Test Gaps

• Negative-path tests that assert type/status but never the side effect
• Security-enforcement features with no integration test exercising the enforcement path
• Missing edge-case tests that would be trivial to add

Completeness Gaps

• Shortcut implementations where the complete version costs under 30 minutes
• Partial enum handling, half-built error paths, missing edge cases
• Features sitting at 80–90% when 100% is a modest amount of extra code

Type Coercion at Boundaries

• Values crossing language/serialization boundaries where the type can shift
• Hash/digest inputs not normalized before serialization

View/Frontend

• Inline <style> blocks in components (re-parsed every render)
• O(n*m) lookups in views (Array.find in a loop instead of a Map/index)
• Client-side filtering of API results that belongs on the server

Search before recommending: when you propose a fix pattern (especially for concurrency, caching, auth, or framework-specific behavior):

• Confirm it's current best practice for the framework version in use.
• Check whether a built-in solution exists in a newer version before recommending a workaround.
• Verify API signatures against current docs.

If WebSearch isn't available, say so and proceed on in-distribution knowledge.

Confidence Calibration

Every finding MUST carry a confidence score (1–10):

Finding format:

[SEVERITY] (confidence: N/10) file:line — description

Examples: [CRITICAL] (confidence: 9/10) app/models/user.rb:42 — SQL injection via string interpolation in where clause [INFORMATIONAL] (confidence: 5/10) app/controllers/api/v1/users_controller.rb:18 — Possible N+1 query, verify with production logs

Cap on timing/sequencing findings. If a finding asserts when code runs relative to other code — race, split-brain, "X before Y", missing post-API refetch — its confidence caps at 6/10 unless you ran the Order-of-Operations Verification steps and cited the trigger path in the finding text. A function body alone never earns a 7+ timing claim.

Step 3.5: Review Army — Specialist Lenses

Detect stack and scope

# Stack, for specialist context
STACK=""
[ -f Gemfile ] && STACK="${STACK}ruby "
[ -f package.json ] && STACK="${STACK}node "
[ -f requirements.txt ] || [ -f pyproject.toml ] && STACK="${STACK}python "
[ -f go.mod ] && STACK="${STACK}go "
[ -f Cargo.toml ] && STACK="${STACK}rust "
echo "STACK: ${STACK:-unknown}"

DIFF_INS=$(git diff origin/<base> --stat | tail -1 | grep -oE '[0-9]+ insertion' | grep -oE '[0-9]+' || echo "0")
DIFF_DEL=$(git diff origin/<base> --stat | tail -1 | grep -oE '[0-9]+ deletion' | grep -oE '[0-9]+' || echo "0")
DIFF_LINES=$((DIFF_INS + DIFF_DEL))
echo "DIFF_LINES: $DIFF_LINES"

# Test framework, for test-stub generation
TEST_FW=""
{ [ -f jest.config.ts ] || [ -f jest.config.js ]; } && TEST_FW="jest"
[ -f vitest.config.ts ] && TEST_FW="vitest"
{ [ -f spec/spec_helper.rb ] || [ -f .rspec ]; } && TEST_FW="rspec"
{ [ -f pytest.ini ] || [ -f conftest.py ]; } && TEST_FW="pytest"
[ -f go.mod ] && TEST_FW="go-test"
echo "TEST_FW: ${TEST_FW:-unknown}"

# Scope signals from the changed paths
CHANGED=$(git diff origin/<base> --name-only)
echo "$CHANGED" | grep -qE '(auth|session|login|oauth|jwt|password|token)' && SCOPE_AUTH=true || SCOPE_AUTH=false
echo "$CHANGED" | grep -qE '\.(rb|py|go|rs|ts|js)$|Gemfile|package\.json|go\.mod' && SCOPE_BACKEND=true || SCOPE_BACKEND=false
echo "$CHANGED" | grep -qE '\.(tsx|jsx|vue|svelte|css|scss)$' && SCOPE_FRONTEND=true || SCOPE_FRONTEND=false
echo "$CHANGED" | grep -qE 'migrations?/|schema\.(rb|prisma)$|db/migrate/' && SCOPE_MIGRATIONS=true || SCOPE_MIGRATIONS=false
echo "$CHANGED" | grep -qE 'api/|routes?/|controllers?/|graphql|openapi|swagger' && SCOPE_API=true || SCOPE_API=false
echo "SCOPE: auth=$SCOPE_AUTH backend=$SCOPE_BACKEND frontend=$SCOPE_FRONTEND migrations=$SCOPE_MIGRATIONS api=$SCOPE_API"

Select specialists

If DIFF_LINES < 50: skip all specialists. Print "Small diff ($DIFF_LINES lines) — specialists skipped." and go to Step 4.

Always on (any review of 50+ changed lines):

1. Testing — coverage gaps, test quality, flakiness risk
2. Maintainability — naming, abstractions, organization

Conditional (include when the matching scope signal is true): 3. Security — if SCOPE_AUTH OR (SCOPE_BACKEND AND DIFF_LINES > 100) 4. Performance — if SCOPE_BACKEND OR SCOPE_FRONTEND 5. Data Migration — if SCOPE_MIGRATIONS 6. API Contract — if SCOPE_API 7. Design — if SCOPE_FRONTEND

Force flags: if $ARGUMENTS carries --security, --performance, --testing, --maintainability, --data-migration, --api-contract, --design, or --all-specialists, force-include that lens.

Print the selection: "Running N specialist lenses: [names]. Skipped: [names] (scope not detected)."

Run the specialist lenses

Default (inline). Work through each selected lens one at a time in this context. For each lens, look at the diff (git diff origin/<base> if you need to re-read it), apply that lens's checklist below as if it were the only thing you were hunting for, and record findings in the structured shape: severity (CRITICAL | INFORMATIONAL), confidence (1–10), path, line (optional), category, summary, fix, fingerprint (path:line:category), and optionally evidence and a test_stub. When a finding is catchable by a test, write a minimal test_stub for the detected framework ($TEST_FW) — describe/it/test skeleton with clear intent; skip stubs for architectural or design-only findings. Then go to the merge step.

Opt-in (parallel). If the user opted into subagents (see How this runs), run the selected lenses as parallel subagents instead — one per lens, each with fresh, unbiased context, all launched concurrently. Use the Workflow below (or, if Workflow is unavailable, parallel Agent calls in a single message). Either way the subagents only return findings; the merge, fix-first classification, and any file writes happen back here.

export const meta = {
  name: 'pre-merge-review-specialists',
  description: 'Parallel specialist review army over the branch diff — one lens per agent',
  phases: [{ title: 'Specialist' }],
}
const FINDING_SCHEMA = {
  type: 'object',
  properties: {
    specialist: { type: 'string' },
    findings: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          severity: { type: 'string' },          // CRITICAL | INFORMATIONAL
          confidence: { type: 'number' },         // 1-10
          path: { type: 'string' },
          line: { type: 'number' },
          category: { type: 'string' },
          summary: { type: 'string' },
          fix: { type: 'string' },
          fingerprint: { type: 'string' },        // path:line:category
          evidence: { type: 'string' },
          test_stub: { type: 'string' },
        },
        required: ['severity', 'confidence', 'path', 'category', 'summary'],
      },
    },
  },
  required: ['specialist', 'findings'],
}
const reviews = await parallel(args.specialists.map(s => () =>
  agent(
    `You are a specialist code reviewer for ${s.domain} on a ${args.stack} project.\n\n` +
    `Run \`git diff origin/${args.base}\` to get the full diff. Review against the checklist below.\n\n` +
    `For each finding, return an object with: severity (CRITICAL|INFORMATIONAL), confidence (1-10), ` +
    `path, line (optional), category, summary, fix (recommended fix), fingerprint (path:line:category), ` +
    `specialist ("${s.domain}"). Optionally include evidence and test_stub.\n\n` +
    `If you can write a test that would catch an issue, include it in test_stub. Use the detected ` +
    `test framework (${args.testFw}). Write a minimal skeleton — describe/it/test blocks with clear ` +
    `intent. Skip test_stub for architectural or design-only findings.\n\n` +
    `Before flagging a commented line, walk upward to the nearest function/block boundary; if the ` +
    `enclosing function header is itself commented, the line is dead — do not file a follow-up on it, ` +
    `recommend deleting the dead function or ignoring, and cite the enclosing function name.\n\n` +
    `If no findings, return an empty findings array. Return findings only — no preamble, no commentary.\n\n` +
    `CHECKLIST:\n${s.checklist}`,
    { label: `specialist:${s.key}`, phase: 'Specialist', model: 'opus', schema: FINDING_SCHEMA }
  )
))
return reviews.filter(Boolean)

Pass args: { base, stack, testFw, specialists: [{ key, domain, checklist }, ...] }, where each checklist is the matching checklist below (verbatim), domain is the human-readable lens name, base is the detected base, and stack/testFw come from the detection block. Include only the selected lenses. If the workflow fails or times out, fall back to parallel Agent calls or skip with a note.

Specialist checklists (use the relevant one whether inline or fanned out):

Testing specialist

• Missing tests for new codepaths (cross-reference the diff)
• Happy-path-only tests with no negative/edge coverage
• Tests asserting existence/type but not behavior
• Flaky-test risks (time dependencies, ordering, external services, random data)
• Test-pyramid inversions (heavy E2E, thin unit)
• Regression tests missing for bug fixes

Maintainability specialist

• Naming that describes the implementation instead of the intent
• New abstractions solving problems that don't exist yet
• Duplication across files (DRY violations)
• Cyclomatic complexity >5 in a single method
• Comments explaining WHAT instead of WHY
• Stale docstrings/comments contradicting the code

Security specialist

• Authentication bypasses (missing auth checks on new endpoints)
• Authorization scoping errors (user A reaches user B's data)
• Input-validation gaps (no nil/empty/wrong-type/injection checks)
• Secrets in source (hardcoded tokens, API keys, passwords)
• Insecure direct object references (IDs in URLs with no ownership check)
• CSRF, XSS, SQL-injection vectors
• Dependency risk (newly added packages with known CVEs)

Performance specialist

• N+1 queries (loops triggering a DB call per iteration)
• Missing indexes for new query patterns
• Synchronous calls in hot paths that should be async
• Unbounded memory (no cap on array/list size with production data)
• Missing caching on expensive computation or external calls
• Render perf: O(n*m) lookups in lists, inline styles, needless re-renders

Data Migration specialist

• Backward-incompatible schema changes without a multi-step migration
• Migrations with no rollback plan
• Non-transactional migrations that can leave the DB inconsistent
• Lock-heavy operations on large tables (add column with default, add NOT NULL)
• Data backfills without batching (OOM risk on large tables)
• Zero-downtime gaps: old code + new schema compatibility

API Contract specialist

• Breaking changes to public API signatures without versioning
• Removed/renamed fields clients depend on
• New required request fields with no default handling
• Response-shape changes (type, nullability, enum values)
• Missing error-response documentation
• Status-code changes

Design specialist (frontend)

• Missing loading/empty/error/success/partial states for new UI
• Keyboard-accessibility gaps (tab order, focus rings, escape handling)
• Color-contrast failures (WCAG AA)
• Touch-target size (<44x44 on mobile)
• Missing responsive breakpoints
• Information-hierarchy issues (what does the user see first?)

Step 3.6: Collect and merge findings

Once the lenses are done — whether you walked them inline or they came back from subagents — pull everything together here.

Parse:

1. An empty findings array from a lens → skip it.
2. Otherwise, fold every finding into one list, each tagged with its lens name.

Fingerprint and deduplicate:

• Use the fingerprint field when present.
• Otherwise {path}:{line}:{category} (line present) or {path}:{category}.

For findings sharing a fingerprint:

• Keep the one with the highest confidence.
• Tag it "MULTI-SPECIALIST CONFIRMED ({lens1} + {lens2})".
• Boost confidence by +1 (cap 10).
• Note the confirming lenses.

Confidence gates:

• 7+: show normally
• 5–6: show with caveat
• 3–4: appendix
• 1–2: suppress

PR Quality Score: quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5)), capped at 10.

Output:

SPECIALIST REVIEW: N findings (X critical, Y informational) from Z lenses

[Each finding, CRITICAL first, then confidence descending]
[SEVERITY] (confidence: N/10, specialist: name) path:line — summary
  Fix: recommended fix
  [If MULTI-SPECIALIST CONFIRMED: confirmation note]

PR Quality Score: X/10

These feed into Step 4 Fix-First alongside the critical-pass findings.

Red Team pass (conditional)

Activate only if DIFF_LINES > 200 OR any lens produced a CRITICAL finding.

When activated, do one more fresh-eyes pass that knows the merged findings summary and hunts for what the lenses missed.

Default (inline): run it yourself, here — clear your head of the per-lens checklists and look at the diff for cross-cutting gaps, integration-boundary issues, and failure modes the structured checklists don't cover.

Opt-in (parallel): if the user opted into subagents, run it as one more subagent (the Workflow below, or a single Agent call) with fresh context.

export const meta = {
  name: 'pre-merge-review-red-team',
  description: 'Red-team reviewer — find what the specialist panel missed',
  phases: [{ title: 'RedTeam' }],
}
const FINDING_SCHEMA = {
  type: 'object',
  properties: {
    specialist: { type: 'string' },                 // always "red-team"
    findings: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          severity: { type: 'string' },
          confidence: { type: 'number' },
          path: { type: 'string' },
          line: { type: 'number' },
          category: { type: 'string' },
          summary: { type: 'string' },
          fix: { type: 'string' },
          fingerprint: { type: 'string' },
        },
        required: ['severity', 'confidence', 'path', 'category', 'summary'],
      },
    },
  },
  required: ['specialist', 'findings'],
}
const result = await agent(
  `You are a red team reviewer. The code has already been reviewed by specialists who found the ` +
  `following issues:\n${args.mergedSummary}\n\nYour job is to find what they MISSED. Run ` +
  `\`git diff origin/${args.base}\` and look for gaps. Set specialist="red-team" on every finding. ` +
  `Focus on cross-cutting concerns, integration boundary issues, and failure modes that specialist ` +
  `checklists don't cover.\n\nCHECKLIST:\n` +
  `- Cross-cutting concerns: changes that affect multiple subsystems simultaneously\n` +
  `- Integration boundaries: where services/modules meet, what's the contract?\n` +
  `- Failure modes: cascading failures, retry storms, thundering herd\n` +
  `- Silent failures: errors that get swallowed, data that gets lost quietly\n` +
  `- Consistency: transactional boundaries, eventual consistency pitfalls\n` +
  `- Observability: can you debug this from logs alone 3 weeks from now?\n` +
  `- Rollback: can this be reverted safely in production?\n` +
  `- Blast radius: if this breaks in prod, who/what else breaks?`,
  { label: 'red-team', phase: 'RedTeam', model: 'opus', schema: FINDING_SCHEMA }
)
return result

Pass args: { base, mergedSummary: "<merged findings summary>" }. Merge any Red Team findings into the list before Step 4, tagged "specialist":"red-team".

No additional issues → "Red Team review: no additional issues found." Workflow fails or times out → skip silently.

Step 4: Fix-First Review

Every finding ends in an action — not just the critical ones. This whole step lives here in the main context: the classification, the AskUserQuestion for ambiguous items, and every file edit. (Subagents can't ask you or write files, so even on the opt-in path the fix-first decisions and edits land here once their findings come back.)

Header: Pre-Landing Review: N issues (X critical, Y informational) | PR Quality Score: X/10

Step 4a: Classify each finding

For every finding (critical pass + specialist lenses + red-team/adversarial), sort into AUTO-FIX or ASK:

AUTO-FIX (fix without asking):            ASK (needs human judgment):
- Dead code / unused variables             - Security (auth, XSS, injection)
- N+1 queries (missing eager loading)      - Race conditions
- Stale comments contradicting code        - Design decisions
- Magic numbers -> named constants         - Large fixes (>20 lines)
- Missing API output validation            - Enum completeness
- Unused imports/props                     - Removing functionality
- Inline styles, O(n*m) view lookups       - Anything changing user-visible behavior

Rule of thumb: mechanical, and a senior engineer would just apply it → AUTO-FIX. Reasonable engineers could disagree → ASK.

Critical findings lean ASK (riskier by nature). Informational findings lean AUTO-FIX (more mechanical).

Test-stub override: any finding with a test_stub becomes ASK regardless of its original class. When presenting it, show the proposed test file path and code. On approval, write the fix + test file. Derive the path from project convention (spec/ for RSpec, __tests__/ for Jest/Vitest, test_ prefix for pytest, _test.go for Go); append if the file already exists. Output: [FIXED + TEST] [file:line] Problem -> fix + test at [test_path].

Step 4b: Auto-fix all AUTO-FIX items

Apply each one directly. For each: [AUTO-FIXED] [file:line] Problem → what you did

Step 4c: Batch-ask about ASK items

If there are ASK items, present them in ONE AskUserQuestion:

I auto-fixed N issues. M need your input:

1. [CRITICAL] (confidence: 9/10) path/to/file.ts:42 — Race condition in status transition
   Fix: Add atomic WHERE-status-old UPDATE status=new
   → A) Fix  B) Skip

2. [INFORMATIONAL] (confidence: 7/10) path/to/service.ts:88 — API output not validated before DB write
   Fix: Add JSON schema validation before insert
   → A) Fix  B) Skip

RECOMMENDATION: Fix both — #1 is a real race condition, #2 prevents silent data corruption.

With 3 or fewer ASK items you may use individual AskUserQuestion calls instead.

Step 4d: Apply user-approved fixes

Apply the fixes the user chose "Fix" on. Report what changed.

No ASK items → skip the question entirely.

Verification of claims

Before the final output:

• Claim "this pattern is safe" → cite the line that proves it.
• Claim "handled elsewhere" → read and cite the handling code.
• Claim "tests cover this" → name the test file and method.
• Never "likely handled" or "probably tested" — verify or flag as unknown.

No rationalizing: "this looks fine" is not a finding. Either cite evidence it's fine, or flag it unverified.

Step 5: TODOS cross-reference

Read TODOS.md if present and reconcile against the PR:

• Does this PR close open TODOs? Note which: "This PR addresses TODO: <title>".
• Does this PR create work that should become a TODO? Flag it informational.
• Are there related TODOs for context? Reference them next to the relevant findings.

No TODO file → skip silently.

Step 6: Documentation staleness check

For each .md in the repo root (README.md, ARCHITECTURE.md, CONTRIBUTING.md, CLAUDE.md):

1. Do the code changes touch features that doc describes?
2. If the code changed but the doc didn't, flag INFORMATIONAL: "Documentation may be stale: [file] describes [feature] but code changed in this branch. Consider running /doc-sync."

Informational only — never critical.

Step 7: Adversarial review (always on)

Every diff gets an adversarial, fresh-eyes pass. LOC is no proxy for risk — a 5-line auth change can be critical. This pass runs on every review, regardless of diff size.

Default (inline): do it yourself, here. Drop the checklists, think like an attacker and a chaos engineer, and look for the ways this code fails in production: edge cases, races, security holes, resource leaks, failure modes, silent data corruption, logic that's quietly wrong, error handling that swallows failures, trust-boundary violations. No compliments — just the problems. Classify each as FIXABLE (you know the fix) or INVESTIGATE (needs human judgment).

Opt-in (parallel): if the user opted into subagents, run it as a fresh-context subagent (the Workflow below, or a single Agent call) so its independence is structural, not just intentional.

export const meta = {
  name: 'pre-merge-review-adversarial',
  description: 'Adversarial fresh-context review of the branch diff',
  phases: [{ title: 'Adversarial' }],
}
const ADVERSARIAL_SCHEMA = {
  type: 'object',
  properties: {
    findings: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          classification: { type: 'string' },     // FIXABLE | INVESTIGATE
          path: { type: 'string' },
          line: { type: 'number' },
          summary: { type: 'string' },
          fix: { type: 'string' },
        },
        required: ['classification', 'summary'],
      },
    },
  },
  required: ['findings'],
}
const result = await agent(
  `Read the diff for this branch with \`git diff origin/${args.base}\`. Think like an attacker and ` +
  `a chaos engineer. Your job is to find ways this code will fail in production. Look for: edge cases, ` +
  `race conditions, security holes, resource leaks, failure modes, silent data corruption, logic ` +
  `errors that produce wrong results silently, error handling that swallows failures, and trust ` +
  `boundary violations. Be adversarial. Be thorough. No compliments — just the problems. For each ` +
  `finding, classify as FIXABLE (you know how to fix it) or INVESTIGATE (needs human judgment).`,
  { label: 'adversarial', phase: 'Adversarial', model: 'opus', schema: ADVERSARIAL_SCHEMA }
)
return result

Pass args: { base }. Present the results under an ADVERSARIAL REVIEW: header. FIXABLE findings flow into the same Step 4 Fix-First pipeline (auto-fix mechanical, ask about ambiguous). INVESTIGATE findings are presented as informational. If the opt-in workflow fails or times out: "Adversarial subagent unavailable — falling back to an inline adversarial pass."

Suppressions — DO NOT flag these

• "X is redundant with Y" when the redundancy aids readability
• "Add a comment explaining why this threshold was chosen" — thresholds change, comments rot
• "This assertion could be tighter" when it already covers the behavior
• Consistency-only changes with no functional impact
• Harmless no-ops
• ANYTHING already handled in the diff you're reviewing — read the FULL diff before commenting

Output Format

Pre-Landing Review: N issues (X critical, Y informational) | PR Quality Score: X/10

Scope Check: [CLEAN / DRIFT DETECTED / REQUIREMENTS MISSING]
[If plan completion audit ran: summary line]

**AUTO-FIXED:**
- [file:line] (confidence: N/10) Problem → fix applied

**NEEDS INPUT:**
- [file:line] (confidence: N/10) Problem description
  Recommended fix: suggested fix

**ADVERSARIAL REVIEW:**
- [file:line] (FIXABLE/INVESTIGATE) Problem description

**APPENDIX (low-confidence findings):**
- [file:line] (confidence: 3/10) Suspicious pattern, may be fine — verify manually

**DOCUMENTATION STALENESS:** [if any]
**TODOS:** [if any]

Nothing found: Pre-Landing Review: No issues found. PR Quality Score: 10/10.

Be terse. One line problem, one line fix. No preamble, no summary, no "looks good overall."

Output Convention — REVIEWED.md

By default — matching the review-output convention when this skill ships inside a bundle — pre-merge-review findings go inline in REVIEWED.md under a ## Staff Review Findings section; pre-merge-review doesn't write its own separate review file. If the user confirmed a different destination up front (another file, or just an inline summary in this conversation), honor that instead. Otherwise write that section here in the main context. Don't create a STAFF_REVIEW_<ref>.md and don't mix staff findings into another review type's file; absent a stated preference, the ## Staff Review Findings section in REVIEWED.md is the canonical home.

Important Rules

1. Read the FULL diff before commenting. Don't flag what the diff already fixes.
2. Fix-first, not read-only. AUTO-FIX items go in directly; ASK items only after approval.
3. Never commit, push, or create PRs. Edit files, nothing more.
4. Be terse. One line problem, one line fix. No preamble.
5. Only flag real problems. Skip whatever's fine.
6. Enum completeness means reading outside the diff. Grep every consumer. Read each match.
7. Every finding has a confidence score. Suppress below 3 unless severity would be P0.
8. Inline by default; fan out only on opt-in. The specialist lenses, Red Team, and adversarial pass run inline in this context unless the user explicitly chose parallel subagents (see How this runs). When fanned out, set model: 'opus' on every agent().
9. Red Team only if diff > 200 lines OR any critical finding. Otherwise skip.
10. Adversarial pass is always on. Cheap, fast, catches what structured review misses.
11. Verify claims before writing them. No "likely" or "probably" — cite the code or flag it unknown.
12. Interactive work stays here; only headless compute may fan out. Never put a fix-first decision, an ASK question, the HIGH-impact-discrepancy escalation, or any file write (auto-fixes, approved fixes, test stubs, the REVIEWED.md section) inside a subagent or Workflow agent() — they can't talk to the user or write files.