# Pre Merge Review

> |

- **Type:** Skill
- **Install:** `agentstack add skill-napnap11-claude-skills-pre-merge-review`
- **Verified:** Pending review
- **Seller:** [napnap11](https://agentstack.voostack.com/s/napnap11)
- **Installs:** 0
- **Category:** [Agent Skills](https://agentstack.voostack.com/c/agent-skills)
- **Latest version:** 0.1.0
- **License:** MIT
- **Upstream author:** [napnap11](https://github.com/napnap11)
- **Source:** https://github.com/napnap11/claude-skills/tree/main/pre-merge-review

## Install

```sh
agentstack add skill-napnap11-claude-skills-pre-merge-review
```

Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent.

## About

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

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: 
   Delivered: 
   [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_.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/`.

```bash
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

```bash
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."`
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

…

## Source & license

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

- **Author:** [napnap11](https://github.com/napnap11)
- **Source:** [napnap11/claude-skills](https://github.com/napnap11/claude-skills)
- **License:** MIT

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

## Pricing

- **Free** — Free

## Security capabilities

Automated source analysis of v0.1.0 — what this tool can access:

- **Network access:** no
- **Filesystem access:** no
- **Shell / process execution:** yes
- **Environment & secrets:** no
- **Dynamic code execution:** no

*"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.*


## Versions

- **0.1.0** — security scan: flagged — Imported from the upstream source.

## Links

- Listing page: https://agentstack.voostack.com/l/skill-napnap11-claude-skills-pre-merge-review
- Seller: https://agentstack.voostack.com/s/napnap11
- Browse the marketplace: https://agentstack.voostack.com/browse

---
Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.