Pre Merge Review

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 ``, 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 `` 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 --quiet && git diff origin/ --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/..HEAD --oneline).
2. Pin down the stated intent — what was this branch supposed to do?
3. git diff origin/...HEAD --stat and weigh the files touched against that intent.

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

1. Emit this before the main review:

`` Scope Check: [CLEAN / DRIFT DETECTED / REQUIREMENTS MISSING] Intent: Delivered: [If drift: each out-of-scope change] [If missing: each unaddressed requirement] ``

1. 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_.md and PLAN_REVIEW_.md to the project root; /design-review writes DESIGN_REVIEW_.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/..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  --quiet
git diff origin/

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 W. So Y's async work has already resolved by the time X runs."

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

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: napnap11
• Source: napnap11/claude-skills
• License: MIT

Install and usage instructions live in the source repository linked above.