Code Review

Code Review

Review changes across Quality, Security, Dependencies lenses with optional advisor adjudication. Valid scopes: commit | staged | working | --folder <path> | --file <paths> | hash | A..B | PR branch. Empty scope defaults to feature-branch-vs-default-branch first-parent review (default branch auto-detected; see Step 1).

Input

$ARGUMENTS — scope: commit | staged | working | --folder <path> | --file <paths> | a commit hash | A..B range | PR branch name. Empty defaults to feature-branch-vs-default-branch (first-parent).

Metadata

node "${SKILL_DIR}/../_shared/now.mjs"
echo
node "${SKILL_DIR}/../_shared/git-context.mjs"

Scope resolution (default branch, range, ChangedFiles) is LLM-invoked at Step 1.1 via the bundled _helpers/review-range.mjs — it depends on $ARGUMENTS and on conversational clarification, which render-time substitution cannot capture.

Flow

1. Input → 2. Wave-1 dispatch → 3. Wave-2 dispatch → 4. Wave-3 dispatch → 5. Reconcile → 6. Verify → 7. Write artifact → 8. Present → 9. Follow-ups

File-orientation contract: agents reason about files as coherent units. Hunks are evidence within a file's analysis, never the unit of analysis. The -U30 patch (Step 1) inlines function-level context so agents rarely need extra Read calls.

Every Wave-2 agent prompt contains EXACTLY: (a) Known Context: followed by the Discovery Map verbatim, and (b) the resolved <patch_path> value (the helper's patch_path: field) as the patch path. Exception: strategy: tree direct-read mode passes ChangedFiles for direct file Reads instead of <patch_path>. Nothing else from Wave-1 outputs — NOT the raw integration-scanner dump, NOT precedent-locator output, NOT Dependencies/CVE output. See "Wave-2 context isolation" in Step 3 for the failure mode when this is violated. Wave-1 agents that do not consume the Discovery Map (precedents, dependencies, CVE) get ChangedFiles / manifest-diff only.

Steps

Step 1: Resolve Scope and Assemble the Diff

1. Resolve scope via the bundled helper. Determine the scope spec from the value the user supplied (visible in ## Input above as the substituted argument). If empty, use the literal string auto; if ambiguous (prose, mixed list, unrecognised branch name), clarify via ask_user_question — options: (A) "review current branch vs default branch (first-parent)" → auto, (B) "review every tracked change vs HEAD (staged + unstaged)" → modified, (C) "review unstaged changes only" → working, (D) "restate scope" → free-text — then re-invoke. Then run:

   node "${SKILL_DIR}/_helpers/review-range.mjs" "<scope-spec>"
   

   The helper emits labeled key/value lines (default_branch:, strategy:, oldest:, newest:, base:, tip:, range:, fp_flag:, patch_path:; plus null_tree: for strategy: tree) followed by a ---changed-files--- block. Read those as authoritative for the rest of Step 1. If strategy: unrecognised appears, the note: field explains why — clarify via ask_user_question and re-invoke with a valid spec.

   <scope-spec> translation table — map the user's substituted argument to one of these forms:

   Argument shape	Pass to helper
   empty (no argument provided)	auto
   literal commit / staged / working / modified	same word verbatim
   hex commit hash (4-40 chars, e.g. abc1234)	the hash verbatim
   <A>..<B> (e.g. HEAD~5..HEAD, main..feature)	the range verbatim
   comma- or whitespace-separated hashes (h1,h2,h3 or h1 h2 h3)	the list verbatim
   branch name (must be checked out at HEAD locally)	the branch name verbatim
   --folder <path> (directory relative to repo root)	--folder <path> verbatim
   --file <path1>,<path2> (one or more tracked files; comma-separate multiple paths; literal commas in paths are not supported)	--file <paths> verbatim
   folder:<path> / file:<paths> legacy aliases	pass through verbatim
   anything else (prose, mixed list, unresolvable ref)	clarify via ask_user_question, then re-invoke

2. Confirm strategy from the helper output. The mapping into the rest of the skill:

   • strategy: first-parent (auto / PR branch / commit list) — use <range> AND <fp_flag> (which is --first-parent) in the subsequent git commands. <base> is the parent-of-first-feature-commit (helper computes via merge-base), so the range already includes OLDEST's own changes — do NOT add ^ anywhere.
   • strategy: explicit-range (single hash / A..B) — use <range> without <fp_flag> (it's empty for this strategy). <base> is OLDEST^ (so the range includes OLDEST itself, matching the original "user-inclusive endpoint" intent).
   • strategy: working-tree (commit / staged / working) — no <range>; use the working-tree commands listed in the next bullet.
   • strategy: tree (--folder / --file, plus legacy folder: / file: aliases) — no <range>; review tracked files in the specified path(s) as complete entities. The helper emits null_tree: plus tree_path: (folder) or files_list: (file) instead. Use the tree commands listed below. Precedent-locator is skipped for this strategy (no creation history to compare); all other Wave-1 agents still follow their normal gates.

   --first-parent is orthogonal to --no-merges: the former prunes second-parent subtrees from reachability, the latter drops merge commits themselves from the log. Both flags are independently controllable below.

3. Assemble the UNION of changes (not the net endpoint-diff — so reverted intermediate work stays visible). Save the patch to a tempfile once with generous context; do NOT re-run git log --patch to slice windows later. Substitute literal <range>, <fp_flag>, and <patch_path> values from the helper output (<fp_flag> is --first-parent or empty — omit the flag entirely when empty; <patch_path> is the worktree-safe diff tempfile — a literal .git/… path fails inside a worktree):

   • ChangedFiles — read from the helper's ---changed-files--- block. If a (... N more files truncated ...) footer appears, the change set exceeded the helper's 2000-line/40 KB cap; scope the review tighter or run the patch-tempfile command below to recover the full surface from disk.
   • git log "<range>" <fp_flag> --stat --reverse → per-commit size summary
   • git log "<range>" <fp_flag> --patch --reverse --no-merges -U30 > <patch_path> → union patches with 30 lines of surrounding context per hunk (function-level context inline)
   • git log "<range>" --reverse --format="%H %s%n%n%b%n---" → commit-message context
   • Working-tree branch (strategy: working-tree, no <range>): for staged use git diff --cached --stat + git diff --cached -U30 > <patch_path>; for working use git diff --stat + git diff -U30 > <patch_path> (unstaged only); for modified use git diff HEAD --stat + git diff HEAD -U30 > <patch_path> (every tracked change vs HEAD — staged + unstaged, no untracked); for commit use git show HEAD --stat + git show HEAD -U30 > <patch_path>. Commit-message context is N/A for staged / working / modified; for commit use git show HEAD --format="%H %s%n%n%b%n---" --no-patch. ChangedFiles still comes from the helper.
   • Tree branch (strategy: tree, no <range>): <paths> is tree_path: (from helper) or the comma-separated files_list: entries split on commas only (quote each resulting pathspec; never split files_list: on whitespace; literal commas in file paths are not supported); <null_tree> is the helper's null_tree: value. Emit the size summary separately with git diff --cached --stat <null_tree> -- <paths>. Then choose the review input mode before generating any patch:
     • Direct-read mode (len(ChangedFiles) > 10): do not generate a tree patch. Record TreeInputMode = direct-read; leave <patch_path> unused for Wave-2 and instruct Wave-2 agents to Read files directly from ChangedFiles. This prevents large folder reviews from filling the main context with full-file additions.
     • Patch mode (len(ChangedFiles) ≤ 10): generate the patch with fallback chain (each step only if previous produced empty output):
       1. git diff --cached -U30 <null_tree> -- <paths> > <patch_path> — full-file additions: index vs the repository's empty tree, matching the helper's ls-files --cached enumeration exactly (a <null_tree> HEAD diff would silently drop staged-but-uncommitted files from the patch)
       2. Synthetic patch — for each non-binary file in ChangedFiles, emit a valid unified patch: diff --git a/<f> b/<f> / new file mode 100644 / --- /dev/null / +++ b/<f> / @@ -0,0 +1,<line_count> @@ / then prefix every source line with + and append to <patch_path>. For binary files, emit Binary files /dev/null and b/<f> differ. If both steps produce empty output, print No patchable content in scope. Exiting. and STOP. Commit-message context is N/A. ChangedFiles comes from the helper.
   • Patch-size fallback: -U30 produces ~2–3× the size of -U0. If the resulting patch exceeds ~1MB, drop to -U10 for this run; never use -U0 — it defeats the skill's design.

4. Bail-out: if ChangedFiles is empty, print No changes in scope {scope}. Exiting. and STOP. Do not write an artifact.

5. Derive scope + flags (orchestrator-side, used in later steps):

   • InScopeFiles — used by the Step 6 pre-filter. ChangedFiles reflects tree-reachability (inflated on branches that back-merged the default branch — each post-merge first-parent commit inherits the merge's tree, so --name-only includes every file the merge resolved); InScopeFiles reflects commits' own diffs and is what the developer actually authored. Derivation:

     • strategy=first-parent (auto / PR branch / commit-list inputs) → InScopeFiles = ⋃ git diff-tree --no-commit-id --name-only -r <h> over git log "<range>" --first-parent --no-merges --pretty=%H (each feature commit's own file delta; back-merge sidecars drop out even when the merge is on the first-parent line). For commit-list input, iterate over the user-named hashes instead of the first-parent walk to preserve non-contiguous-list intent.
     • strategy=explicit-range → InScopeFiles = ChangedFiles (user explicitly asked for range semantics; merges in the range are part of the intent).
     • strategy=working-tree → InScopeFiles = ChangedFiles (no merge surface).
     • strategy=tree → InScopeFiles = ChangedFiles (no merge surface; all files are treated as added).

   • Invariant: InScopeFiles ⊆ ChangedFiles. On back-merged feature branches, InScopeFiles ⊊ ChangedFiles is the primary mechanism by which sidecar findings get dropped at Step 6.

   • ManifestChanged = ChangedFiles intersects any dependency manifest or lockfile (e.g. package.json/lockfile, Cargo.toml/Cargo.lock, go.mod/go.sum, pyproject.toml/requirements*.txt/poetry.lock, Gemfile*, *.csproj, pom.xml/build.gradle*, composer.json, …) OR a peer/optional/dev-dependency field was touched.

   • LockstepSelfReview = repository root contains scripts/sync-versions.js AND every packages/*/package.json shares the same version: AND the diff touches packages/*/package.json.

   • HasGatingPredicate = diff adds or modifies a status/enum-comparison predicate (Status == X, Status is X or Y, X.Contains(Status), pattern-match on a discriminator) OR introduces a new value into an enum referenced by existing gating predicates. NOT merely the presence of if (!x) return.

   • ReviewType = one of commit | pr | staged | working | tree.

   • PeerPairs = (new_file, peer_file) tuples. new_file is in git log "<range>" --diff-filter=A --name-only (working-tree: git diff --diff-filter=A --name-only [--cached]; tree: all ChangedFiles are treated as additions). peer_file exists at HEAD (git ls-tree HEAD) and matches one heuristic:

     • Stem similarity ≥ 60% of the longer stem (e.g. PhysicalProductSubscription ↔ Subscription).
     • Interface/impl pair: I<Name> ↔ <Name>, <Name> ↔ <Name>.impl, <Name>{Abstract,Base,Protocol} ↔ <Name>.
     • Shared suffix from {Handler, Service, Repository, Aggregate, Reducer, Controller, Resolver, Command, Query, Job, Processor, Strategy, Policy, Event, Listener, Subscriber, Publisher, Exception, Eligibility, Ability, QueryParam, Specification, Factory, Builder}.

     Drop a pair only when the peer doesn't exist at HEAD, no heuristic matches, or both files were added in this diff. Empty list ⇒ skip the peer-mirror agent. Co-modified peers are KEPT — the agent Reads them at HEAD (post-diff tree state), so any invariant present at HEAD counts as peer evidence regardless of whether the peer was edited in this diff.

   • Intra-folder peers (strategy: tree only): when len(ChangedFiles) ≥ 3 and files share a common parent directory, compute (file_a, file_b) pairs among ChangedFiles themselves using the same heuristics (stem similarity, shared suffix, interface/impl). This catches pattern divergence within the reviewed folder (e.g., two skills sharing execute(self, ctx) but one missing validation_failed skip logic). Merge these pairs into PeerPairs alongside any cross-repo peers.

Step 2: Dispatch Wave-1 — Integration + Precedents + Deps/CVE + Peer-Mirror

Spawn ALL of the following in parallel at T=0 in a single message with multiple Agent tool calls. Do NOT wait for integration-scanner before dispatching precedents / dependencies / CVE — they do not consume Discovery-Map output, only ChangedFiles and the manifest diff (both orchestrator-produced in Step 1).

Agent — Integration map:

• subagent_type: integration-scanner
• Prompt: "Map inbound references, outbound dependencies, and infrastructure wiring for the following changed files: {ChangedFiles, one per line}. Flag any auth-boundary crossings (middleware, guards, interceptors, authorize-style decorators) and config/DI/event registration touching these paths. Do NOT analyse code quality — connections only, in your standard output format."

Agent — Precedents (always, except strategy: tree): use the precedent-locator prompt defined in Step 3 below — dispatch it here, not in Wave-2. Input it needs: ChangedFiles only. For tree strategy, skip entirely — there is no creation history to compare, and the files are the baseline.

Tree strategy Wave-1 note (load-bearing): for strategy: tree, skip only precedent-locator. Still dispatch integration-scanner; still dispatch Dependencies and CVE / advisory when ManifestChanged; still dispatch peer-comparator when len(PeerPairs) > 0.

Agent — Dependencies (only when ManifestChanged): use the codebase-analyzer Dependencies prompt defined in Step 3 below — dispatch here. Input it needs: touched manifest paths + LockstepSelfReview flag.

Agent — CVE / advisory (only when ManifestChanged): use the web-search-researcher prompt defined in Step 3 below — dispatch here. Input it needs: parsed name@version list from the manifest diff (orchestrator extracts and hands over directly).

Agent — Peer-Mirror (only when len(PeerPairs) > 0): subagent_type: peer-comparator. Input: the PeerPairs list verbatim, nothing else — no Discovery Map (it isn't built yet and the agent doesn't need it), no patch path (the work is peer-vs-new entity comparison, not diff analysis). Prompt:

Peer-mirror check.

PeerPairs (orchestrator-computed):
{list of (new_file, peer_file) tuples}

For each pair, Read BOTH files in full. Enumerate the peer's PUBLIC surface as rows:
- every public method / exported function
- every domain event / notification / message fired (language-agnostic: method calls named `fire*`, `emit*`, `publish*`, `dispatch*`, `raise*`, `notify*`, `AddDomainEvent`, or idiomatic equivalents)
- every state transition (name + precondition guard + side-effects)
- every constructor-injected / DI-supplied collaborator
- every persisted field / column / serialised property
- every registration this file contributes to a switch/map/table/route/handler registry elsewhere (match by type name appearing in a `switch`/`match`/`when`/dispatch table)

For each row, check the new file. Emit ONE row per peer invariant:

  peer_site (file:line — `<verbatim line>`) | new_site (file:line — `<verbatim line>` OR `<absent>`) | status | one-sentence delta

status ∈ {Mirrored, Missing, Diverged, Intentionally-absent}.

"Intentionally-absent" requires an explicit cite — a comment in the new file, a commit-message line mentioning the omission, or a type-system constraint that makes the invariant inapplicable (e.g. the peer's `Trial*` methods are absent because the new entity's type says it doesn't support trials). Suspicion is not sufficient; when in doubt, emit Missing.

Output format: markdown table per pair, heading `### Peer pair: <new_file> ↔ <peer_file>`. No prose outside the tables. No severity. No recommendations. Citation contract applies to every cell.

While these agents run, the orchestrator produces the rest of the Discovery Map inline from Step 1's data:

• ChangedFiles, ManifestChanged, LockstepSelfReview, ReviewType
• Semantic file map (per file: path (+A -B) + role tag + top-level symbol names touched; see format and rules below)
• Commit-message context (if applicable)

Wait for integration-scanner AND the peer-mirror agent (when dispatched) before dispatching Wave-2. Wave-2 agents consume both via the Discovery Map (auth-boundary crossings, inbound refs, peer-mirror Missing/Diverged rows). Precedents / Dependencies / CVE continue running in the background; Precedents MUST be awaited before Step 5 begins (Reconciliation reads its follow-up-within-30-days counts to weight severity; see Step 5). Dependencies / CVE also merge in at Step 5 but may arrive later in the wait barrier.

Synthesize the Discovery Map — a compact block that Wave-2 agents receive verbatim as Known Context. Each file line carries a role tag and a symbols-touched hint; files are clustered by shared directory prefix so agents orient without re-reading the patch.

#### Discovery Map

Review type: {ReviewType}
Scope: {scope argument}
Commit/range: {git ref}
Manifest changed: {yes|no}
Lockstep self-review: {yes|no}

Changed files ({N}):

  ## {cluster — shared directory prefix}
    path/file.ext (+A -B) {role-tag} — top 1–3 symbols touched
    ...

Auth-boundary crossings: {integration-scanner, file:line}
Inbound refs (files with ≥3 consumers): {integration-scanner}
Outbound deps: {integration-scanner}
Wiring/config: {integration-scanner}
Peer mirrors: {peer-mirror agent output verbatim — Missing/Diverged rows only; Mirrored and Intentionally-absent rows are summarised as counts}

Clustering: group files by longest shared directory prefix yielding clusters of 2+ files; singletons form their own cluster labelled with the filename. Emerges from the repo — no framework assumptions.

Role-tag (one tag per file, first match wins):

1. [boundary] — in integration-scanner's auth-boundary output
2. [persistence] — path contains migration/schema/repository/dao/model, or matches an ORM/migration convention visible in the repo
3. [test] — path contains /test//spec/__tests__, or filename ends in a test suffix (.test.*, .spec.*, _test.*, Test.*)
4. [config] — in integration-scanner's wiring/config output, or is a manifest/lockfile/settings file
5. [hub] — in integration-scanner's inbound-refs with ≥3 consumers
6. [code] — default

Symbols-touched hint: extract top 1–3 top-level definitions from the diff's + lines using a heuristic appropriate to the file's language (class/function/def/fn/struct/trait/interface/type/export). Cap at ~80 chars. Leave blank if ambiguous — orientation, not completeness.

Step 3: Dispatch Wave-2 — Quality + Security Lenses

Spawn Quality + Security in parallel using the Agent tool. Each receives the Discovery Map block inline as Known Context above its task. For all non-tree-direct-read strategies, also pass the resolved <patch_path> value for the diff itself. In tree direct-read mode, do not pass or paste <patch_path>; pass ChangedFiles and instruct agents to Read each file directly. Precedents / Dependencies / CVE are already running from Wave-1 — do NOT re-dispatch them here; the prompts below document what those Wave-1 agents received, they are not re-issued.

Wave-2 context isolation (LOAD-BEARING — violations cause silent quality collapse): Each Wave-2 agent receives EXACTLY two things, nothing else: (1) the Discovery Map (digested form) and (2) for non-tree-direct-read strategies, the resolved <patch_path> value; for tree direct-read mode, the ChangedFiles direct-read list.

DO NOT paste into Wave-2 prompts, under any circumstance, even if the orchestrator has already received them:

• raw integration-scanner output (the Discovery Map already summarises its auth/ref/wiring findings)
• precedent-locator output
• Dependencies lens output
• CVE lens output
• any prior Wave-2 or Wave-3 output from earlier runs in the same Pi session

Why this is load-bearing: summary context induces narrativisation — the agent treats the preamble as "the orchestrator already framed the findings, I just classify them" instead of independently reading the patch file. Observed failure signatures when this is violated: Quality drops from ~40 tool calls / 3M tokens / 500s to ~5-15 tool calls / 300k tokens / 100-200s, and returns hallucinated findings (invented statuses, mis-cited line numbers, claims that files are "missing from patch" when they are in fact present).

Self-check before dispatching Wave-2: read your outgoing Agent prompt. If it contains any content from Wave-1 agent RESULTS beyond the Discovery Map you synthesised, strip it. The Discovery Map is the contract; raw outputs are reconciliation-only.

Tree strategy direct-read mode (strategy: tree): when len(ChangedFiles) > 10, the orchestrator should already have skipped patch generation in Step 1. For safety, if a tree patch exists and exceeds ~2MB, ignore it. In either case, skip <patch_path> entirely and instruct each Wave-2 agent to Read each file directly from ChangedFiles. The agents receive the same Discovery Map + surfaces list, but substitute Read calls for patch grep. This avoids context explosion from massive synthetic diffs. For len(ChangedFiles) ≤ 10, use the patch as normal.

When in tree direct-read mode, adapt the Quality and Security prompts below by replacing every instruction to inspect <patch_path> / diff regions with: "Read each file in ChangedFiles directly and analyse the complete file. Findings must cite actual file lines with verbatim quotes." Do not create, paste, summarize, or attach a synthetic patch for those agents.

Citation contract (applies to every Wave-2+ agent, every step): every file:line citation MUST be accompanied by the literal line text in backticks — format file:line — \` — `. Omit findings whose lines you cannot quote verbatim.

Quality lens (diff-auditor) — file-oriented:

Analyse changes file by file. For each file in ChangedFiles, read its diff region in `<patch_path>` (patch has `-U30` — full function context is already inline; rarely need an extra Read call), form a mental model of what the file does and what the diff changes about it, then apply the 13 surfaces below to the file as a whole. Cite `file:line` with verbatim line text (citation contract) for every finding. Omit findings not traceable to a diff-touched change. No severity.

**File order strategy**: prioritise by role tag — `[boundary]` files first (security-sensitive), then `[persistence]` (durable-state surfaces), then `[hub]` (blast-radius amplifiers), then `[code]`, then `[config]`, then `[test]` last. Within the same tag, prioritise files with the largest diffs.

**Per file**, write a short section (`### file/path.ext`) containing only the surfaces that APPLY to that file's changes. Use sub-headings for grouped evidence. A surface may be flagged across multiple files — report the evidence where it lives, and rely on cross-layer surfaces (8, 9) to tie them together.

**Surfaces** — each surface's mechanical trigger decides whether it APPLIES to a given file's changes. Walk every applicable trigger:

1. **Logic & flow** (always, per file with new/modified code) — validation, error paths, off-by-one, null misses, branch ordering, return/await, unguarded mutation.
2. **Pattern coherence** (≥2 similar constructs within a file, or ≥2 files in the same cluster) — cite nearby line broken from.
3. **Blast radius** (Discovery Map lists inbound refs to this file, OR this file is flagged as a hub) — `consumer:line` + what changes for each inbound ref.
4. **Test coverage gaps** (always, checked once across the whole changeset) — for each risk-bearing function/method added/changed, check whether any `[test]`-tagged file in ChangedFiles contains a corresponding test. Flag risk-bearing behavior with no adjacent test.
5. **Predicate-set coherence** (`HasGatingPredicate`) — ≥2 conditionals on the same enum/type across the changeset. Tabulate `predicate file:line | accepted | rejected`. Flag mismatches. Surface 5 output MUST use heading `### Predicate-set coherence` at review-scope level (not nested inside a per-file section) — downstream step consumes it verbatim.
6. **Registration coverage** (changeset adds discriminator value / enum variant / handler key / route / event type / strategy entry) — every dispatch/registry/switch across ChangedFiles that must enumerate it. Cite each registration site + each enumeration site. Flag gaps.
7. **Query/write symmetry** (changeset adds setter/linker, shape change, or new persisted field) — trace BOTH creation and renewal/update paths; cite the setter file:line AND the reader file:line.
8. **Cross-layer drift** (same entity/enum/key appears in ≥2 files in different clusters or role tags — e.g. model ↔ DTO ↔ schema ↔ registry ↔ presentation) — open each file, tabulate presence, flag asymmetry. When the entity is a **key fanned out across parallel tables** (locale maps, theme maps, strategy registries, handler/command tables, feature-flag tables), every table must carry the added key and none must retain a removed/renamed one — flag orphaned references and missing entries.
9. **Peer-member consistency** (a file gains a new method/hook/case/handler in a set of peers — class methods, hook siblings, reducer cases, handler registrations, CLI subcommands) — tabulate the invariants the peers share (state mutation, emitted event/signal, precondition guard, bookkeeping counter, teardown symmetry); flag omissions in the new member.
10. **Durable-state hygiene** (file is a schema migration, repository/DAO, file-backed config, KV/cache, serialized artifact, or adds a new persisted field) — trace the forward-write AND the reverse/rollback path; flag irreversible or data-losing rollback, missing lookup affordance for a new query field (index, key, sorted structure), iteration over an unbounded/mutable source without a stable cursor, and new invariants at the storage layer not mirrored by the in-memory validator.
11. **Shared-state acquisition** (file introduces async handler, event listener, singleton init, queue consumer, file-lock region, or global cache mutation) — trace acquire/release around every mutation; flag unguarded check-then-act across an `await`/callback/IPC boundary, stale reads while another writer is in flight, non-commutative acquire order between distinct state slots, and replay/retry paths that lack an idempotency key.
12. **Multi-step commitment** (file issues ≥2 writes that must all succeed or all be undone — DB transaction, cross-table mutation, multi-file write, filesystem+network pair, multi-API orchestration, compensating-action chain) — trace the commit boundary; flag missing undo/compensation on partial failure, retry paths that re-apply non-idempotent steps, and divergence between two stores that must agree without a coordinating primitive.
13. **Error handling & idempotency** (file adds a failure-response construct — retry loop, catch/except block, error boundary, fallback branch, circuit breaker, timeout, resumable step) — trace the error-propagation path; flag swallowed errors, retry without an idempotency key, fallback that silently degrades observable behavior, unjustified timeout values.

**Economising Reads**: issue a `Read` only when (a) you need a file NOT in ChangedFiles (hub, peer, test), or (b) the changed function is longer than the `-U30` window can show. Never re-Read a file just to re-orient — that's what the symbols-touched hint is for.

Security lens (diff-auditor) — file-oriented:

Analyse each changed file as a whole, looking for sinks in the classes below. For each file, grep the file's diff region in `<patch_path>` (patch has `-U30` — sink context is inline) for the sink patterns, and for each hit provide the verbatim line (citation contract) plus 2 surrounding lines and `confidence: N/10` that user-controlled input can reach the sink under current deployment. Drop hits with confidence < 8. Cross-reference Discovery Map auth-boundary crossings and inbound refs — a sink in a file reached from an auth-boundary file is in scope even if the sink file itself doesn't cross the boundary.

**File order strategy**: `[boundary]` files first (direct source→sink exposure); then `[persistence]` (query injection, unsafe deserialization); then `[code]` (command exec, SSRF, explicit-trust rendering); then `[hub]` / `[config]`; skip `[test]` unless a test helper touches a sink.

IN-SCOPE RULE: only report findings whose sink line is inside a changed file's diff region (add/modify/adjacent-context rewrite). Pre-existing sinks in files the diff did not change are out of scope, UNLESS the diff changes how data flows TO the sink (e.g. new user-controlled source routed to an untouched sink) — then cite both locations. When in doubt, trace data flow from Discovery Map boundary crossings through the changed files.

Sink classes — match the concept in whatever language the diff uses:
- **Command execution** — shell/process spawn w/ user input (e.g. `exec`, `subprocess.run(shell=True)`, `Runtime.exec`, …).
- **Dynamic code / unsafe deserialization** — `eval` / dynamic-function constructors; deserializers that can execute code (e.g. `pickle.loads`, `yaml.load` w/o safe loader, `ObjectInputStream`, `BinaryFormatter`, …).
- **Query injection** — user input concatenated or interpolated into a query string interpreted by an external engine (SQL, NoSQL operators, LDAP filters, XPath, GraphQL constructed as string). Parameterized/prepared queries and typed query builders are safe.
- **Explicit-trust rendering** — user input emitted into a channel that will interpret it as code/markup rather than data. In-scope only when the framework's explicit-trust API is invoked (`dangerouslySetInnerHTML`, `bypassSecurityTrustHtml`, `v-html`, `template.HTML`, `mark_safe`, `html_safe`, triple-stache, raw-HTML markdown, …), plain `innerHTML =` / `document.write(` in vanilla/server-rendered code, or equivalent passthroughs in non-HTML renderers (unescaped ANSI-escape emission to a TTY, unquoted shell-prompt interpolation, template-engine raw blocks).
- **Path traversal** — user-controlled components into file-system APIs without normalization/allowlist.
- **SSRF** — outbound HTTP/TCP with user-controlled host OR protocol (not just path).
- **Secrets in diff** — literal credentials, API keys, PEM blocks, connection strings w/ embedded passwords, `.env` content.
- **Missing trust-boundary check** — a traced sink reached from a Discovery-Map boundary crossing (HTTP handler, RPC endpoint, IPC message, CLI flag that flows to a privileged operation, webhook receiver) without an upstream authorization/validation step (middleware, guard, attribute/decorator, allowlist check, signature verification).

Do NOT report: DOS/resource exhaustion/rate-limiting, missing hardening without a traced sink, theoretical races/timing without reproducer, log spoofing/prototype pollution/tabnabbing/open redirects/XS-Leaks/regex DOS, client-side-only authn/authz (server is the authority), findings sourced only from env var / CLI flag / UUID, test-only or notebook files without a concrete untrusted-input path, outdated-dependency CVEs (CVE lens handles).

Name the sink class and the matched idiom. Evidence only. No CVE lookups.

Dependencies lens (codebase-analyzer, only when ManifestChanged; otherwise SKIP and omit ### Dependencies in artifact):

Lockstep self-review: {yes|no}

Identify the ecosystem from touched manifests (npm, Cargo, Go modules, PyPI/Poetry, Bundler, NuGet, Maven/Gradle, …). Parse the changed manifest(s) and list:
1. Added deps: `name@version` with `file:line`.
2. Bumped deps: `name: old -> new` with `file:line`.
3. Removed deps.
4. Peer / optional / dev-scope changes (whatever the ecosystem calls them).
5. License field changes in manifest or lockfile.
6. Lockstep=yes: flag only intra-monorepo drift where a sibling pin diverges from the lockstep version. Treat wildcard peer pins as intentional.
7. Lockstep=no: flag version conflicts between direct dep and lockfile resolution.

Evidence only. No CVE lookups.

Precedents lens (precedent-locator):

Code review of {scope}. Changed files: {ChangedFiles}.
Find similar past changes touching these files or nearby. Per precedent: commit hash, blast radius, follow-up fixes within 30 days, one-sentence takeaway. Distil composite lessons.

CVE/advisory lens (web-search-researcher, only when ManifestChanged):

Look up CVEs / GitHub Advisories / OSS Index entries for the target versions. Return LINKS. Per vulnerability: severity (Critical/High/Moderate/Low), affected range, whether bumped-to version is fixed.

Dependencies:
{name@version per line — orchestrator-extracted}

Wait for Quality + Security to complete before proceeding. Precedents / Dependencies / CVE from Wave-1 may still be running; gather them before Step 5, not before Step 4.

Step 4: Dispatch Wave-3 — Predicate-Trace + Interaction Sweep + Gap-Finder

Once Wave-2 (Quality + Security) completes, dispatch 4a and 4b as parallel agents in a single message; compute 4c inline (orchestrator-side set arithmetic — no agent). They do NOT consume each other's output:

• Interaction Sweep (4b) receives Quality's Predicate-set coherence table directly as its predicate-row source. Quality's table already flags mismatches — Predicate-Trace (4a) only elaborates them through consumers. Interaction Sweep's categories 1–6 don't need 4a at all; categories 7–9 (stranded-state, false-promise, co-tenant filter gap) operate on the same rows 4a would trace.
• Gap-Finder (4c) is coverage arithmetic: {in-scope files} − {files with ≥1 Quality/Security finding} = {uncovered files}. Orchestrator already holds both sets post-Wave-2 — an agent would discard context only to re-receive it via prompt. Inline is strictly cheaper and deterministic.
• If Predicate-Trace (4a) surfaces a row that was not visible in Quality's table, append it via a Step 9 follow-up — cheaper than a serial gate.

Step 4a: Predicate-Trace

Gate: SKIP this sub-step (do not dispatch 4a) unless HasGatingPredicate is true AND the Quality lens returned ≥2 rows in its Predicate-set coherence table referencing the same enum/type. If skipped, 4b and 4c still dispatch.

Otherwise spawn ONE codebase-analyzer in parallel with 4b:

Coherence rows (Quality — Predicate-set coherence): {paste verbatim}
Gating predicates in diff: {`file:line` list}

Per predicate, return: `predicate file:line | inputs | promise (what TRUE/matching branch implies) | consumer file:line | consumer filter | fulfils? | gap`.

Flag:
- *False promise* — matching branch depends on a consumer/filter elsewhere that excludes this entity's source/type/state.
- *Stranded state* — entity reaches state X via one conditional, but every conditional that operates on this entity elsewhere excludes X (no exit path).

Evidence only. Citation contract applies.

Do NOT wait — 4b (Interaction Sweep) dispatches in the same message as 4a; 4c runs inline in the orchestrator.

Step 4b: Interaction Sweep

Gate: SKIP this sub-step (do not dispatch 4b) when EITHER len(ChangedFiles) < 2 OR the Quality lens returned fewer than 4 total observations across all files. Emergent interactions need surface area; tiny diffs cannot structurally produce them.

Otherwise spawn ONE codebase-analyzer in parallel with 4a:

Quality Evidence: {verbatim}
Security Evidence: {verbatim}
Predicate-set coherence rows (verbatim from Quality's table — full Step 4a output is NOT required and is NOT awaited): {verbatim | "not applicable"}
Precedents: {verbatim if Wave-1 finished; else "deferred to Step 5"}

Group evidence by shared entity, state machine, workflow, data flow path, API boundary, background process, or producer-consumer contract.

Per group, check for emergent defects:
1. contradictory assumptions between components/layers,
2. unreachable, stuck, or non-terminal states,
3. retry/reprocess mechanisms made inert by another behavior,
4. duplicate-processing / idempotency gaps from ordering or missing guards,
5. guards in one layer invalidating transitions in another,
6. one finding masking, amplifying, or permanently triggering another,
7. stranded state — state X reachable via one conditional but every conditional operating on this entity elsewhere excludes X,
8. false-promise predicate — matching branch's consumer excludes this entity's source/type/state,
9. co-tenant filter gap — shared discriminator filter where a new or terminal value the diff touches falls through every consumer's filter.

Return only findings with ≥2 concrete `file:line` facts from different files/components, each quoted verbatim per the citation contract. No recommendations. No single-location repeats.

For findings involving ordering/races/concurrency across processes or handlers, name the ordering primitive that would prevent the race (distributed lock, exclusive-key wrapper, ordered partition, transaction, idempotency key, etc.) and explain why it does NOT apply here. Drop the finding if the primitive exists in the diff or nearby and your argument against it is speculative.

Step 4c: Gap-Finder (orchestrator-side coverage arithmetic)

Gate: SKIP when len(ChangedFiles) < 2. Tiny diffs cannot structurally have coverage gaps.

No agent dispatch. Compute inline while 4a / 4b run:

For strategy: tree (completeness audit — "is everything right?"):

1. Symbol inventory — for each [code]/[boundary]/[persistence]/[hub] file in ChangedFiles, extract top-level exported symbols (functions, classes, handlers, entry points). Read the file directly if patch is unavailable.
2. Test coverage map — for each symbol, check whether any [test]-tagged file in ChangedFiles (or a sibling *.test.* / *_test.* / test_*.* at HEAD) contains a test referencing that symbol name. Symbols with no test are uncovered.
3. Error-path scan — for each uncovered symbol, check whether it contains try/catch, if (!x) return, or equivalent error handling. Flag symbols with no error path.
4. Emit gap findings —

…(truncated)