Aif Review

# Project Rules for /aif-review

> Curated from review pass-through findings.
> Sections under "Auto-generated rules" are managed by `/aif-evolve`; do not hand-edit them.
> Last updated: 2026-09-04
> Based on: 2 analyzed patches + 1 adversarial-review pass-through (M7 / 2026-05-28)
> + /aif-evolve patch analysis (M9-M10 docs-drift + auth-first, 2026-05-30;
> M11b/M11c adversarial-review batch, 2026-06-01)
> + /aif-evolve 107-patch batch (2026-06-17, cursor 2026-05-30 → 2026-06-16)
> + /aif-evolve 114-patch batch (2026-07-11, cursor 2026-06-16 → 2026-07-07)
> + /aif-evolve 74-patch batch (2026-09-04, cursor 2026-07-07 → 2026-09-04)

## Rules

### Body-controlled cross-resource identifier — flag as CRITICAL when server-state holds the same value
**Source**: M7 adversarial review pass-through (2026-05-28)
**Rule**: For every HTTP route handler in the diff (Next.js Route Handler, Fastify route, Server Action), enumerate every field consumed from the request body. For each body field that NAMES a cross-resource locator — project slug, run id, step id, workspace id, tenant id, any filesystem path component — locate whether the handler has access to the same value from a `server-state` source on the same request:

- Session registry / connection record (e.g. `registry.get(sessionId).record`).
- DB row fetched via a URL parameter (e.g. `db.select … where id = urlParams.runId`).
- Auth context / JWT claim / API key binding.

If the answer is yes (server-state has it), the body field is **redundant AND a trust-boundary smell**. Flag as CRITICAL `#trust-boundary`. The expected fix is to drop the body field and derive from server-state, NOT add a regex to the body field — regex only protects against syntactic abuse, not against using `sessionA`'s valid id to operate on `sessionB`'s resources.

When the body field genuinely IS needed (e.g. multi-tenant route not yet authenticated), confirm there is an explicit comparison against the corresponding `server-state` value AND a stated mismatch response (typically 409). Absence of either → flag.

Severity stays CRITICAL even when the affected service is documented as "internal-network only" — that assumption rots.

### Idempotency marker committed before downstream side-effect — flag as CRITICAL
**Source**: M7 adversarial review pass-through (2026-05-28)
**Rule**: For every state-changing route handler in the diff, locate any column that functions as an idempotency / completion marker (`respondedAt`, `completedAt`, `processedAt`, `deliveredAt`, `acknowledgedAt`, `finishedAt`). For each marker, walk the handler in execution order and identify:

1. The DB statement that writes the marker (or relies on a default `now()`).
2. Every downstream side-effect that happens AFTER the marker write (HTTP call to a sibling service, file write to disk, queue publish, RPC call, subprocess spawn).

If any downstream side-effect runs AFTER the marker is committed, the route is **non-retryable on side-effect failure** — flag as CRITICAL `#retry-semantics`. The expected fix is the two-phase pattern documented in `.ai-factory/skill-context/aif-plan/SKILL.md` ("Plan MUST specify two-phase commit for routes with downstream side-effects"):

- Phase 1 commits user intent (request body, response choice) WITHOUT the marker.
- Side-effect runs outside the transaction.
- Phase 2 commits the marker only after the side-effect acks.

Also flag if the failure classification is missing — a route that returns identical 4xx/5xx on every failure flavor cannot distinguish retryable from terminal. The diff should include separate handler branches with separate status codes for: downstream 404 (terminal), downstream 5xx (retryable), network/timeout (retryable), validation errors (4xx terminal).

Severity stays CRITICAL even when the downstream service is co-located in the same compose stack — "supervisor restarts mid-deploy" is a real failure mode the route must survive.

Extensions: a resume/idempotent handler re-drives on STATUS, not the idempotency marker — the resource stays in the awaiting state (`NeedsInput|NeedsInputIdle`) until the runner claims it via an idempotent CAS claim (`claimAndResumeAgentRun`, 0 rows = already advanced = no double-spawn) callable from both the response path and the already-delivered retry; never CAS `NeedsInput→Running` inside the response tx — the runner owns that transition. Flag a handler with multiple outcomes (insert / reinforce-update / no-op) whose idempotency is keyed on ONE branch's side-effect — require a consumption ledger (`brain_harvested_events`) written on ALL branches in the effect's transaction. A composite with an unrollbackable external side effect needs a durable idempotency key persisted BEFORE it (retry adopts the already-launched run, never relaunches). Operator/human input files deleted before downstream durable side-effects succeed = a durability-boundary violation — flag it.
**Source (additions)**: 2026-06-24-02.29, 2026-07-02-15.32, 2026-07-03-00.00-budget-breach-review-followup, 2026-06-24-18.46

### catch block in deferred-creating code path — flag as CRITICAL when explicit release is missing
**Source**: M7 adversarial review pass-through (2026-05-28)
**Rule**: For every catch block in the diff, ask: does the try block (or the SSE event handler / queue consumer / etc enclosing the catch) observe a remote-deferred-creation signal — a "request created", "permission requested", "tool call awaiting approval", or any signal where another process has registered a pending promise / timer / handle that THIS code path is expected to release?

If yes, the catch block MUST contain an explicit call to release the deferred (resolve / cancel / reject — pick the right semantic for the failure cause). "Log the error and continue" is wrong here — flag as CRITICAL `#hidden-deferred`. The deferred-creating side is now leaked until its own timeout fires (typically 30+ min), invisible to operators.

Concrete patterns to spot:

- A handler for an SSE event named `*.request`, `*.permission_request`, `*.awaiting_approval`, `*.requested` whose catch block does not call a sibling `*.cancel` / `*.reject` / `*.deny` API.
- A consumer of a queue message of type "request" whose catch block does not publish a corresponding "cancel" / "reject" / "ack-failed" message.
- A callback registered for a long-poll handle whose catch block does not call the corresponding "abort" / "close" API.

Also flag tests in the same diff: a regression test MUST exist that simulates a failure in the deferred-releasing code path AND asserts the release API was invoked exactly once with the right arguments (spy / mock assertion). Without that test, the rule cannot regress safely.

### Path-operation audit: trace every path operation back to its validator
**Source**: 2026-05-26-12.45.md
**Rule**: For every `path.resolve` / `path.join` / `fs.*` / `child_process.spawn` argv (including `cwd`) usage in the diff, locate the upstream validator that produced the input. If the input arrived as `z.string()`, `z.string().min(N)`, or any other "open" string type without a regex constraint (`^[A-Za-z0-9._-]+$` for filesystem path segments) or a path-shape `.refine()` (`startsWith("/")` + no `..` for absolute paths) — flag as CRITICAL `#path-traversal`. Apply the same scrutiny to SQL parameter construction and any raw HTML/`innerHTML` sink. Severity stays CRITICAL even when the affected service is documented as "internal-network only" — that assumption rots.

### Boolean simplification: flag `A && X !== K` where A constrains X
**Source**: 2026-05-26-12.45.md
**Rule**: Flag conditions of the shape `A && X !== K` where `A` already implies the right side. Example: `res.ok && res.status !== 204` — `res.ok` is true iff status is in [200, 299], so the `!== 204` branch is dead code. ESLint will not catch this class of redundancy; the reviewer must. Tag findings as `#dead-code`. Add to the "Best Practices" section of the review summary, not "Critical".

### When the diff adds a Zod regex/refine to one field, check sibling fields
**Source**: 2026-05-26-12.53.md
**Rule**: When reviewing a Zod schema where the diff adds a security regex / `.refine()` to one field, scan every sibling field of similar type in the same schema. Flag any sibling that flows into a filesystem / subprocess argv / SQL / HTML sink without an equivalent constraint. Same finding type as the first field — do not let partial-coverage waves through.

### Review exit MUST re-derive every analytics/spec surface from the diff and treat drift as a must-fix
**Source**: M7 adversarial review passes 3-4 (2026-05-28-20.01, 2026-05-28-20.32); M9 branch review (2026-05-29-15.38); M10 verify pass-through (2026-05-30)
**Rule**: Before a review can exit clean, enumerate every analytics/spec surface the diff touches and reconcile EACH against the code at HEAD — never trust the doc's own status tag. The reconciliation set, by changed area:

- **state machine / lifecycle change** → the matching `docs/system-analytics/*.md` `stateDiagram` AND its Expectations + Edge-cases bullets AND its refusal/precondition table must match the code's actual transitions and guards (M10: the analytics refusal table was a deny-list while the code was a stricter allow-list);
- **DB schema change** → `docs/database-schema.md` AND the relevant `docs/db/*.md` `erDiagram` both updated (M10: the ERDs were left stale);
- **HTTP/SSE wire change** → `docs/api/*.openapi.yaml` / `docs/api/async/*.asyncapi.yaml` paths, bodies, status codes, and example payloads;
- **error-code / semantic change** → `docs/error-taxonomy.md`;
- **a route target newly referenced by the UI** → a matching `page.tsx` (a link to a route is a contract).

Per `docs/CLAUDE.md`'s "code wins" policy, analytics drift is a MUST-FIX in the SAME review, not a follow-up — "code lands, docs follow" is the exact pattern that produced consecutive M7 review rounds describing pre-Mx behavior. Also flag any spec section that describes behavior absent at HEAD unless it is explicitly tagged Designed Mx / Phase 2 (R6). The review summary MUST list each analytics surface checked (even "no change required"), mirroring the `/aif-verify` contract-surface enumeration on the review side.

**Highest-risk drift sub-surfaces (re-found across M18-M34):** a **documented edge-case bullet** is the most dangerous drift — it reads as a reviewed invariant and steers future implementers back to the old shape, so a converged/removed special case MUST delete or rewrite its edge-case bullet (not just the diagrams/enums). On a **dormant-capability flip** (a previously-off actor/kind going live), grep older-stage analytics for `never` / `not yet` / `MUST NOT` claims + `(Designed)` status tags the flip invalidates. Derive DB cascade/default claims from the migration SQL `ON DELETE`, not prose. A retirement (a dropped route/status/field) is a contract-WIDE sweep: grep the whole docs tree for the old token (`413`, `files/content`) across Expectations bullets, mermaid labels, edge-case lists, error-taxonomy, env tables, and ADR wording — they drift independently of the primary prose. A `[ADR-NNN]` cite with no `### ADR-NNN` header at HEAD, or a migration number that disagrees with `_journal.json`, is a build break — grep `git show main:docs/decisions.md` for the number before approving a merge.

Contract-fidelity additions: code must enforce the CONTRACT, not a stricter paraphrase — when a guard's comment and OpenAPI say "resolvable from A OR B" and the code checks only A, the CODE is the defect (the spec was right); cross-check guards against the published spec and require the B-path regression. A documented MUST/edge-case bullet the code doesn't satisfy is a must-fix, not a doc-only nit — and prefer implementing the SAFER documented behavior over downgrading the doc to match weaker code. When a diff touches ANY part of an operation (one response field, one example), reconcile the operation's WHOLE request+response surface against code at HEAD — "I only touched `count`" does not scope the review. A mermaid step below an early `return` is drift — verify neither diagrammed branch returns before the step (`squashRunBranch` was diagrammed feeding both merge and PR while the code returned first for PR mode, making `commits=squash_rework` a silent no-op for PR promotions).
**Source (additions)**: 2026-06-25-19.58, 2026-06-25-17.42, 2026-07-02-16.42, 2026-06-20-18.20

2026-07..09 extensions: when a code fix changes the event/trigger an interceptor keys on, `git grep` the OLD trigger phrase across `docs/` acceptance bullets, mechanics prose and the ADR (a stale ACCEPTANCE bullet is worse than stale narrative — a test written from it re-asserts the fixed bug). Every Expectation bullet must NAME what enforces it (a constraint, a CAS, a singleton, a test) — a MUST that cannot is a wish: weaken it to the mechanism's scope or add the mechanism; MOTIVATION prose copied into an Expectation or an OpenAPI operation description becomes a false promise ("whether its summon was accepted"), and a schema qualifier ("a report, not a guarantee") must also sit on the operation description callers read. When a doc's rationale names a MECHANISM, check the mechanism is READ (`pr_state_checked_at` written in 6 places, read in 0, elevated to a MUST) and is the REAL reason (an INVERTED rationale — "target-scoped fetch" as why a lease is safe — steers a reader to remove the real guard). A "mechanism" sentence or refusal-table "Where checked" cell is re-derived line by line (plan REQ "discriminated union" vs the shipped strict object + refinement). Adding auth to a route changes its status-code surface — the spec entry gains `401`/`403` in the same diff; a `.strict()` body and its `additionalProperties:false` schema are a PAIR (missing property = rejected request); OpenAPI `format: uuid` where zod is `z.string().min(1)`, index names, and mermaid transition labels stating the wrong trigger are drift; sequence diagrams show the REAL branch ORDER; response-shape-vs-code drift is invisible to `validate:contracts` — diff re-authored schemas field-by-field against the handler AND the pre-port version. When a write path changes its STORED form, every read DTO returning that text is in scope. When the UI phase of a feature lands after its backend merged, the as-built reconcile is re-run; a bulk status flip reports the FORMS covered (`| Designed |` table cells vs `(Designed — ADR-x)` prose). Route-level error docs describe the DURABLE state after dispatch, not an absorbed upstream status.
**Source (additions)**: 2026-07-12-10.43, 2026-07-27-09.30, 2026-07-16-16.05, 2026-07-16-14.05, 2026-09-02-03.51, 2026-07-11-21.30, 2026-07-16-15.20, 2026-07-16-16.40, 2026-07-21-23.50, 2026-07-25-11.14, 2026-07-12-22.40, 2026-08-06-19.10, 2026-07-16-13.06

### Auth-first ordering — authenticate before any resource lookup, read routes included
**Source**: M9 adversarial review rounds 2-3 (2026-05-29-13.52, 2026-05-29-14.38); M9 branch review (2026-05-29-15.38)
**Rule**: For every route handler / server action in the diff, confirm the FIRST awaited operation is the session/active-session gate (`requireActiveSession` or equivalent, INCLUDING the forced-password-change gate) BEFORE any DB lookup of the named resource. A handler that reads `hitl_requests` / `runs` / `projects` before authenticating lets an unauthenticated or password-gated caller probe resource existence (a timing / error-shape oracle). This applies to READ routes (GET / stream) too, not only state-changing ones. Project-role authz then runs against a server-DERIVED `projectId` (from the resource row), never a body field. Flag ordering violations as a security finding, not a nit. (Three consecutive M9 review rounds re-found this on different routes — it is a systemic ordering bug, not a one-off.)

**Extends to body-parse and write-scope (re-found across the M16/M17/M27/M34 routes):**
- The auth gate MUST be the FIRST `await` BEFORE `req.json()` / Zod `.parse()` — parsing the body first returns `422 CONFIG` (leaking the body schema) to an unauthenticated caller instead of `401`. Flag any `req.json()` / `.parse(` appearing above `requireActiveSession`/`requireGlobalRole`/`ctx.authorize`. After reordering N routes, grep EVERY changed route — an "auth-first on all routes" claim was refuted by a `tasks` POST still parsing first.
- `ctx.authorize` runs immediately after the minimal lookups that resolve `projectId`; every other precondition/classification/enriched error (launchability, archived, `KEY-N` blocker ids) comes AFTER it (else it leaks state to an unauthorized caller).
- **Authz scope must match WRITE scope:** a project-URL route whose handler mutates platform-scoped rows (fans `setup.sh` trust across every project on a shared `package_installs` row) needs `requireGlobalRole("admin")`, not project `managePackages`. A project route updating rows with no `projectId` in the WHERE is a red flag.
- **Fanout tables are not authorization:** a read model over per-recipient fanout rows (`inbox_items` filtered only by `(recipient_type, recipient_id)`) must re-derive visibility at read time (membership inner-join on `project_members` for non-admin + `archived_at IS NULL`); apply to the unused-but-exported sibling too.
- Server-authoritative fields (`readinessStatus`, ids, timestamps) must NOT appear in request Zod schemas — `.strict()` then rejects them (422); a present-but-overridden field is still a trust smell.
- **`requireSession(` is the password-change escape hatch ONLY** (it enforces neither `accountStatus === active` nor `mustChangePassword === false`): every state-changing or role-scoped route uses `requireActiveSession()` / `requireGlobalRole` / `requireProjectAction` — grep `requireSession(` in route handlers during review; a single route gated weaker than its siblings is the tell. Confirm the error taxonomy maps ALL auth codes (`ACCOUNT_INACTIVE → 403` was missing, surfacing a disabled session as 500).
- **Auth-first applies to CHANGED routes, not only new ones** — an "auth-first on all routes" claim was refuted twice by a missed twin (a `tasks` POST; the promotion-hold PUT next to the fixed settings PATCH); grep every changed route AND its route-class siblings, and require the regression test asserting unauthenticated-malformed-body → 401/403, never 422.
**Source (additions)**: 2026-06-20-21.18-m36-flow-package-viewer, 2026-06-23-14.56, 2026-06-30-12.23, 2026-07-01-11.45, 2026-07-03-14.58

- Only an in-function, CALL-SITE-AWARE scan is evidence of an ordering violation: a whole-file grep flagged 44 routes, per-function 18, and most were `parseJson` HELPERS whose call sites auth first; parse-before-auth remains a REPO-WIDE pattern (~17 handlers), so record fixed / already-safe / out-of-scope and never claim the class fixed from one route; widening a pre-auth schema (`toTaskKey` + refine) widens what leaks.
- The READ gate is chosen from the DATA'S CLASS, not the route's neighbourhood: a file an agent writes with repo access needs `readRepoFiles` (ADR-053), not `readBoard` → `#read-gate-below-data-class`.
- An allow-list entry whose written JUSTIFICATION is narrower than the grant ("the edge it created" vs any `relations:delete`) → the enforcement belongs in the operation's predicate (`onlyAuthoredBy` as AND-terms on the DELETE's own WHERE, never a preceding SELECT) → `#scope-vs-justification`.
- A client-minted session/bearer token accepted as identity at a privileged write without a server-authoritative principal (`locked_by_user_id`), or generated from `Date.now()`/`Math.random()` → `#unbound-bearer`.
- A scope set that is safe only by spreading another (`[...AGENT_TOKEN_SCOPES]`) needs its own explicit NEGATIVE test, or the next append hands a machine actor a human-reserved action.
- Every new grant/scope needs a POSITIVE test exercising the granted path; every denial keeps the existence-hidden 404.
  **Source (additions)**: 2026-08-06-17.03, 2026-07-27-21.50, 2026-07-22-01.14, 2026-07-16-16.40, 2026-08-06-19.10

## Auto-generated rules (managed by `/aif-evolve` — do not hand-edit below this line)

### Multi-write transition without one transaction / CAS-first ordering — flag as CRITICAL
**Source**: 2026-05-31-22.46, 2026-05-31-23.49, 2026-06-01-12.55
**Rule**: For every state-changing handler OR background sweep/watchdog in the diff that writes more than one row (status column + ledger/`node_attempts` row + cursor + artifact), walk it in execution order and check four things:

1. **Atomicity + crash windows.** Are all the persistent writes inside ONE `db.transaction`/CAS? If not, is there a reachable PROCESS-DEATH partial state (crash between two independent commits) with no tested recovery path? A recovery sweep that filters on a single status only rescues states that reach it. Missing recovery for a reachable partial state → flag CRITICAL `#atomicity`.
2. **Guard-before-side-effect (CAS first).** When a contention guard (status CAS) coexists with another write that has its OWN unique constraint, the CAS MUST run FIRST so only the winner performs the constrained insert. If the constrained insert runs before the CAS, a concurrent loser raises a raw Postgres `23505` that surfaces as 500 instead of the documented 409 → flag CRITICAL `#error-contract`.
3. **Ledger clobber.** A background sweep that updates a ledger row with NO status predicate (`markNodeFailed(id, …)` not guarded by `WHERE status='Running' AND current_step_id=<node>`) can overwrite a concurrently-`Succeeded` attempt to `Failed` → flag `#ledger-clobber`.
4. **Lie about terminal state / store split.** Anything marked terminal `Failed` BEFORE the underlying execution is confirmed stopped or absent (a retryable teardown error treated as "give up") → flag `#split-brain`. A release/abandon that flips `runs.status` but leaves the takeover `node_attempts` row open → flag `#store-split`.

Require a REAL concurrency test (two racers) for the 409-vs-500 contract, not a single-threaded one. Severity stays CRITICAL even after a green per-phase gate — these are the exact edges per-phase reviews miss.

5. **Hard deletes, supersessions, and emits.** A `.delete(<table>)` in production code is checked against the table's inbound `ON DELETE CASCADE` FKs (`tasks` cascades from `runs` AND `domain_events`) → `#cascade-delete`; the terminal exit is `Abandoned` with the guard inside the UPDATE. A successor-created supersession outside the successor's insert tx, dependent rows created outside the tx of the keyed record that can reject them, or non-final response effects (child stamp, assignment completion, audit, outbox row) split from the response-marker tx → `#split-tx`; the outbox emit must be CONDITIONAL on a newly-stamped row. A TERMINAL idempotency marker written on a lost-CAS path (`notified.tree="terminate"` — the marker belongs to the winner) → `#marker-without-transition`. A "MUST emit" invariant implemented at ONE writer of a status instead of a helper every writer calls (`emitDelegatedReviewIfChild`) → `#emit-one-edge`; a shared emit whose payload lacks a CAUSE discriminant once new writers join (`RUN_REVIEW_CAUSES` required on `ReviewFlipRow`) → `#cause-missing` — ask what every consumer will DO with the widened population (an operator stop auto-merged a half-done diff).
   **Source (additions)**: 2026-09-02-03.51, 2026-09-02-12.10, 2026-07-14-human-ask-review-remediation, 2026-07-25-12.09, 2026-07-14-22.50-plan-review-review-fixes, 2026-09-03-17.21

### New run status / enum value / state-changing route not fanned out to ALL consumers — flag drift; deny-list guards are a finding
**Source**: 2026-05-31-22.46 (#3/#4), 2026-05-31-23.49 (#1), 2026-06-01-12.55 (#3)
**Rule**: When the diff adds a `runs.status` value, an enum case, or a state-changing route, grep the new value into EVERY consumer and flag the misses:

- present in one read model but absent from another (board has it, portfolio/home does not — a claimed run vanishes from the home grid while holding a slot) → `#read-model-drift`;
- absent from the scheduler/cap predicate or a recovery/idle sweep's candidate filter → `#consumer-miss`;
- a state guard written as a deny-list of a coarse property (`!terminal`) rather than an allow-list of exact valid states (`status ∈ {NeedsInput, NeedsInputIdle}`) → flag as a security/correctness finding `#guard-denylist`: it silently admits the new status (M11b: `HumanWorking` slipped a `!terminal` HITL guard and let a stale pre-takeover decision be stored);
- a new terminal transition that frees a concurrency slot but never calls `promoteNextPending`/`releaseSlotOnIdle` → `#stranded-queue`;
- a new state-changing route shipped with no OpenAPI/AsyncAPI path → `#missing-contract`.

These are not nits — a claimed run holding a slot invisibly and a guard admitting an un-vetted status are real availability/correctness defects.

Extensions: reusing an existing status SET for a new concern without a semantics check is the same defect class as a deny-list guard — `SETTLED` (= done WRITING) was silently reused as "safe to AUTO-merge" while failure-terminal siblings are settled-for-writing but not shippable; require a concern-named predicate (`countFailureTerminalSharedSiblings`). A schema accepting an enum value with NO implementation → `#silent-noop` (require implement-or-reject-at-boundary via `assertExecutionPolicySupported`-style typed `CONFIG`; grep every consumer of the enum's resolver). A column going NULLABLE fans out like a new enum value (slot counters, per-project sweep candidate queries, authz gates that throw on null). A new enum/kind value fans into AUTH surfaces too: actor-authorization guards (default new HITL/escalation kinds to the HUMAN-ONLY side — a machine token could dismiss its own `hook_trip` guardrail), TS Extract/narrowing types, dispatch switches, i18n, AND the agent-token grant list (`AGENT_TOKEN_SCOPES` + scope→action map move with the route scope). Two independent enumerations of one closed set (schema union + UI mirror constant) → require derive-from-schema or a compile-time exhaustiveness assertion (`satisfies Record<SchemaType, …>`); a map keyed by a closed union whose miss silently falls back (`Record<string, …>` + runtime default) hides drift — require `Record<Union, …>` so a missing key is a compile error.
**Source (additions)**: 2026-06-22-11.08, 2026-06-22-13.40, 2026-06-20-18.20, 2026-06-27-23.45, 2026-06-23-13.22, 2026-06-24-19.45

2026-07..09 extensions: widening a type + producer without the DECODER/parser allow-list (`parseScratchMessageContent` after `capability_guard`) → `#half-fan-out`; a jsonb discriminated union widened without grepping readers for FIELD SNIFFING (`"agentDefinitionId" in`, `-> 'agentId' IS NULL`) and DTO/UI projections → same finding (run-tree, board cards, relaunch lineage). A new scheduler kind fans out to registration, lifecycle, the `claimDueJobs` CTE, dispatch, admin, OpenAPI AND the localized admin label map. A real third state entering an `else` branch written for another → require a discriminated-union MEMBER. Status guards derived from an exhaustive `satisfies Record<RunStatus, boolean>` map shared by every consumer are the expected shape; `NOT IN (...)` over statuses → `#guard-denylist` (it admitted `Pending`). A cancel/stop tool on a child whose resulting status still counts as LIVE for the admission cap → `#counter-not-freed`. A typed union with zero importers (`RunSyncPhase`) or a `Record<string, unknown>` payload field a consumer branches on → `#type-hole`.
**Source (additions)**: 2026-07-12-10.43, 2026-09-02-03.51, 2026-07-12-20.46-observatory-agentization-review-remediation, 2026-08-06-19.10, 2026-09-04-00.21, 2026-07-16-11.20, 2026-09-03-17.20

### Public response returns a DB row or a server-only handle — flag as #response-leak
**Source**: 2026-05-30-13.38, 2026-05-31-23.58, 2026-06-01-01.29
**Rule**: For every public route handler / server action response in the diff, flag `#response-leak` if it returns a DB row or a service object verbatim, OR includes any server-only operational field: `acp_session_id`, supervisor session id, adapter launch argv/env, materialized paths, worktree paths, internal cost handles. The expected fix is an explicit response-DTO projection at the boundary mirrored in `docs/api/web.openapi.yaml` + a test that asserts the EXACT shape (no extra keys) — never redaction-by-hope or "the service type is internal so it's fine". Also flag a public response whose documented shape does not match the route's actual return (duplicate OpenAPI 409 entries, a `capabilityProfile` schema that diverges from the handler) — a contract that lies is a review finding. (M11/scratch-runs leaked supervisor/materialization handles and shipped register routes returning richer-than-contracted bodies.)

**Project DTOs PER discriminant, and treat any guarded field as sensitive at ALL boundaries.** A shared read-model DTO that crosses to the browser must be projected per `kind`/variant — a `permission` HITL row carries `supervisorSessionId` / `requestId` / `toolCall` in its `schema`, so the projection must be `row.kind === "permission" ? null : row.rawSchema`. This exact leak shipped across FOUR readers (board, inbox, run-detail, ext `GET …/hitl`) and into the RSC flight payload — when one boundary already guards a field, that is a signal it is sensitive at EVERY boundary; grep all sibling readers. A `{ ok, id }` response (not the stored row) is the secure default for create/mutate routes. Flag a "present for `<variant>`" field note in a shared schema the moment a second variant gains the field (same instinct as sibling-Zod-sweep, for docs).

2026-07..09 extensions: an RSC read that feeds client props ungated by the SAME authorization as the surface that displays the value (the lock holder's email in the Flight payload for viewers) → `#rsc-read-leak` — grep the READ, not the render. A read that spans two authorization domains (a relation's far end in another project) authorized only on the URL project → `#two-domain-read`; require redaction behind an explicit `redacted` boolean while keeping the ADDRESS the contract needs. A sensitive aggregation as a generic DTO field (cross-project attachment counts) → require explicit opt-in at the query boundary. A module-local error serializer diverging from the canonical one (`catalogErrorResponse` dropping `details`) → `#serializer-drift`; host-local paths in admin summaries and ledger handles on wire bodies stay `#response-leak`.
**Source (additions)**: 2026-07-22-01.14, 2026-08-06-19.10, 2026-07-13-11.21, 2026-07-21-23.50, 2026-07-11-13.34, 2026-09-03-17.20

### Concurrency review red-flags — a transaction is atomicity, NOT serialization
**Source**: 2026-06-02-18.44, 2026-06-03-00.52, 2026-06-03-01.27, 2026-06-07-18.35, 2026-06-08-13.13, 2026-06-10-11.42, 2026-06-10-20.48, 2026-06-11-15.00
**Rule**: For every concurrent read-then-write / check-then-act in the diff, flag:
- `SELECT … FOR UPDATE` + an unconditional `UPDATE … WHERE id` → last-writer-wins disguised as a lock. The write predicate must carry the client-observed value (`WHERE version = expected` / `WHERE role = expectedRole` / `WHERE status = observed RETURNING`); 0 rows → typed `CONFLICT`. → `#optimistic-concurrency`.
- A `SELECT id … FOR UPDATE` that selects only `id` and never re-reads the guarded column (`status`) under the lock → the guard is on the optimistic pre-read, not the locked path. → `#toctou`.
- A lock taken on table A (`runs`) while the mutation writes table B (`gate_results`) that other writers touch without A's lock → wrong-lock-scope; lock the row whose invariant you protect. → `#wrong-lock-scope`.
- A set-membership CAS (`status IN (...)`) where the requirement is "reject if it changed under me" — use the EXACT observed value (`status = live.status`); a set CAS silently permits within-set transitions (a `stale` gate restored by a racing report). → `#cas`.
- An insert gated by a read-then-write `SELECT` instead of `onConflictDoNothing().returning()` (TOCTOU → raw `23505` → 500 instead of 409). → `#race-condition`.
- A CAS whose guard state is re-enterable by a timeout/reclaim with no fencing token (per-attempt stamp, `eq(lastFiredAt, stagedAt)`) → a hung attempt overwrites a newer reclaim's marker. → `#fencing`.
- A batch loop staging side-effects that reads a pre-batch count (`countLiveRuns`) for each item instead of threading `reservedSlots` → cap-3 launched 10. → `#batch-state`.
- `NOT (nullable_col = X AND …)` — three-valued logic drops NULL rows; require `IS DISTINCT FROM` / explicit `IS NULL` arms. → `#sql-null`.
- A write-once/idempotency marker set AFTER a destructive or divergent side-effect (irreversible `discard` before the CAS) → claim-before-side-effect; marker-after is acceptable ONLY when every racer's effect is idempotent AND identical. → `#claim-order`.
Require a REAL two-racer test (uncommitted write on a 2nd pg connection + `pg_stat_activity` wait, or one-shot `vi.spyOn(db,"transaction")`) for any 409-vs-500 contract — a single-threaded test is not evidence.
- An UPDATE acting on a row selected EARLIER without re-asserting the selecting predicate in its WHERE (+ `RETURNING`) → `#read-committed-toctou` (a racing decay sweep expired the row mid-reinforce and the domain event was silently consumed); a claim tx that re-reads only `status` while trusting caller-read preconditions written by out-of-band transactions (`promotion_hold`, master toggles) has one TOCTOU per un-rechecked predicate.
- A lock-free pre-read feeding a decision made after the lock (`loadRun` before the claim) → the lock is defeated; require re-read UNDER the lock (and fix inverted lock orders by re-reading, not second-locking).
- A guard on ONE of N transitions into the guarded state (`tryStartRun` but not `promoteNextPending`) → `#guard-every-edge`.
- Concurrent ops on a shared NAMED resource (a reused git remote like `maister-publish`, a temp branch, a singleton row) without per-op isolation or a mutex → cross-request corruption (committed content pushed to the wrong repo).
- A capped background sweep: ask "if the first N rows are permanently ineligible and never change, does row N+1 ever get processed?" — no durable attempt-marker/rotating cursor → `#starvation`.
- One `try` spanning an irreversible side-effect AND a best-effort follow-up, whose `catch` means "the side-effect failed" → `#try-scope`; follow-ups get their own WARN-only catch.
**Source (additions)**: 2026-07-02-17.52, 2026-07-03-14.58, 2026-06-22-14.06, 2026-06-21-03.05, 2026-06-25-20.14, 2026-07-03-16.16

- A transaction trusted to serialize a GLOBAL invariant (count, cap, quota) while updating per-row → `#lock-not-gate`; require the codebase's advisory lock first (`takeSchedulerLock(tx)` before `countLiveRuns`).
- A lock whose KEY is narrower than the invariant's scope (parent-keyed lock guarding an ancestor-scoped `max_child_runs`) → `#lock-scope-vs-invariant`; reviewing the lock's existence is not reviewing its key — require the tree-root lock, parent-then-root on every path.
- A status-guarded CAS treated as the PRECONDITION (irreversible cascade above it; a status read five awaits earlier) → `#cas-not-precondition`; require validate-before-mutate and a re-read at the moment it matters.
- A call to a status-guarded transition whose result is DISCARDED before dependent effects (`crashRunningRun`, `finalizeAgentRun`, `markAbandoned`, `crashWaitingOnChildren`) → `#discarded-cas-result`; state read before a CAS and acted on after it must be covered BY the CAS (`current_step_id`, not only `status`, across a cross-process `await`) or re-read in the tx.
- A read → compare → write over a FILESYSTEM described as a CAS with nothing holding the interval → `#filesystem-cas` (`atomicWrite*` prevents torn files, not lost updates); require the `pg_advisory_xact_lock` and the post-write state captured inside it.
- A destructive side effect (`reset --hard`, `git clean -fd`) sequenced BEFORE the claim tx, justified as "idempotent on retry" → `#effect-before-claim` (that argument covers crash recovery, not admission); require resolution before / application after the marker, winner only.
- A precondition protecting irreversible work checked at the route level BEFORE the exclusive lease → `#precondition-outside-lease`.
- A `// NO same-session escape` mutex on one writer with sibling writers of the same working dir under a weaker lock → `#mutex-siblings`; guarded and unguarded variants of one write coexisting (`failAttemptIfActive` at 1 of 9 sites) → fix the class at the choke point with a CAS on the EXACT observed phase.
- A mutation on the SHARED parent repo without `withRepoPromotionLock`, or divergence claimed without `isAncestor` proof → flag (a dirty parent also fails `merge --ff-only`).
- A proposal to INVERT an ordering to close a race without enumerating the recovery for the NEW partial state → `#unrecovered-window`; a tested backstop beats a narrower untested window.
  **Source (additions)**: 2026-09-01-04.15, 2026-09-03-14.30, 2026-09-03-15.37, 2026-09-03-17.21, 2026-09-03-23.46, 2026-07-27-20.55, 2026-07-16-20.43-canonical-create-flow-review-findings, 2026-07-12-11.12, 2026-07-17-00.00, 2026-07-16-16.05

### Establish branch scope with merge-base before trusting a diff — guard against the false-revert
**Source**: 2026-06-09-14.23, 2026-06-09-18.47, 2026-06-05-15.06
**Rule**: Before reviewing/diff-reasoning, run `git merge-base main HEAD`; if main is AHEAD of the merge-base, review `main...HEAD` (THREE-dot), never `main..HEAD` (two-dot) — a diverged base renders main-only files as branch "deletions" (false-revert) and contaminates `--stat`. A reviewer claim of "feature removed" MUST be backed by a removing hunk in a branch commit (`git log -p --follow <file>`), not mere absence in `base..HEAD`. Separately, for `git diff` RANGES in the code under review: a fixed stored SHA base → `base..branch` (literal tree delta); two moving branch tips → `base...branch`. A `...` against a fixed point silently omits committed changes; flag every diff helper and confirm which side is fixed. The project has multiple base→branch diff sites (`diffRange`, `diffRunWorkspace`, `diffNameStatus`) — they must share one semantic; a "match the other helper" comment is a consistency argument, not a correctness one.

### DRY/SSOT — a `// mirrors X` comment is a must-fix, and a verdict must come from the one function that computes it
**Source**: 2026-06-02-02.50, 2026-06-03-02.56, 2026-06-04-12.33, 2026-06-04-16.25, 2026-06-13-20.02
**Rule**: A `// mirrors X:line`, `// Task N reconciles both`, or `byte-equivalent to X` comment is the author confessing duplication that WILL drift — treat it as a must-fix and require extracting the shared core (not "shared leaf helpers" — the COMPOSITION must be shared; a read model re-assembling shared primitives is a divergence waiting to happen). A user-facing capability flag (`isRunRecoverable`, badge readiness, "needs you" count) MUST derive from the ONE function that performs the capability (`classifyRecover`, `assertEvidenceReady`, `portfolio.totalNeeds`), never a re-implemented predicate that "should match"; when re-eval needs DB state, factor the pure DECISION into the shared module and keep the FETCH in the caller (don't drag IO into the SSOT). Flag a value that is a server-degradation signal (truncated/capped diff) smuggled as an in-band marker string instead of a structured `{ text, truncated }` flag — and flag any "can't fully see X" state at a promote/merge/approve gate that only annotates instead of blocking (or blocking + explicit override).

Parity/SSOT additions: a SECOND classifier/verdict parallel to an existing one (force vs manual launchability) is diffed against the original's COMPOSITION — every layer it applies (flow-setup issues via `flowIssue ?? "launchable"`, supervisor readiness, archived) is applied or consciously documented as excluded; "verdict B mirrors A" is checked end-to-end, not at the base classifier. A load-time scanner that mirrors a runtime executor derives the scanned FIELDS from the executor's actual per-branch selection through ONE shared helper (`renderedGateField`) used by both — with `.passthrough()` schemas "unused field" is representable, so scan `action.prompt` only for prompt-action node types and `action.command` only for command-action types, tested per kind × field. A hardcoded constant where every sibling reads a config value (`workspacePolicy: "keep"` next to `workspacePolicies[0]`) is a parity must-check. Protocol encoders live in the schema-owning module so serialization cannot drift from validation. UI permission props mirror backend action gates at the SAME granularity — no collapsing kind-specific route authz into one screen-level boolean. Contract mirrors drift BOTH directions: flag phantom spec params the code never reads AND code fields the spec lost; a route must accept the documented optional/empty body (map to the documented default, e.g. `{ stopLiveRuns: true }`); the UI must expose the documented contract range (the server accepted 2..12 variants while the screen offered fixed A/B).
**Source (additions)**: 2026-06-30-13.02, 2026-07-01-01.27, 2026-07-01-12.38, 2026-06-22-20.53, 2026-06-25-17.16, 2026-07-04-01.04, 2026-07-02-14.14, 2026-07-04-14.25, 2026-07-04-00.47

2026-07..09 extensions: the extraction is the COMPOSITION with its messages (the whole launchability check sequence), not the constant it reads — three copies had already diverged (`#partial-ssot`); "kept local — surgical, not a refactor" and "drift is safe" are the same confession as `// mirrors X`. Near-duplicate helpers diverging on a SAFETY property (`releaseSyncClaim` fenced vs `releaseClaim` unfenced; `failResolver` vs `failAgentAttemptToReview`) → `#safety-twins`. A fan-out (pairs, checks) and its aggregation deriving "the same" participant set in two modules → require one shared set function. A new predicate beside an existing one (`connectable` vs `candidatesFor`) that omits its guards (`enabled`, source-scoped match) → `#sibling-parity`; two UI surfaces invoking one irreversible operation with separate eligibility/payload builders → same. A hook that exports a guard (`confirmed`) consumed by one of two consumers → `#guard-not-consumed`. A capability flag re-implemented as an inline predicate (`strict && enforced`) instead of the verdict function (`evaluateNodeEnforcement`) → flag even when provably equal today. An enum/nullable set hand-mirrored where an alias/derivation from the taxonomy or DB union exists → `#hand-mirror`.
**Source (additions)**: 2026-09-02-03.51, 2026-07-16-14.05, 2026-07-12-22.40, 2026-07-16-16.05, 2026-07-26-12.32, 2026-07-13-11.21, 2026-07-21-23.50, 2026-07-12-10.43, 2026-07-27-11.56

### Run an adversarial/invariant pass on security- and state-machine-heavy diffs; stated invariants are checklist items
**Source**: 2026-06-21-11.58, 2026-06-18-15.42, 2026-07-01-12.55, 2026-07-02-17.03
**Rule**: For diffs touching trust boundaries, filesystem mutation, error classification, or run-state machines, run an explicit adversarial pass that challenges invariants under edge/terminal conditions — it finds a different bug class than defect-focused review (it caught two holes a normal /aif-review plus six review subagents all missed; four completeness/consistency self-review passes still shipped a supply-chain hole). Every stated invariant (INV-N in a `system-analytics` Expectations section) becomes a review checklist item: enumerate EVERY code path that could violate it — "unconditional / both pools" means ALL paths including sibling subsystems (hook_trip approve, budget-raise, orchestrator transitions), not the one the feature author had in mind.

2026-07..09 extensions: the adversarial pass is also mandatory for interceptor / launch-wiring / event-ORDERING diffs (a solo pass rated three seam-vs-adapter blockers SAFE), for promotion / trust-boundary / concurrency diffs (a green multi-agent review did not clear the apply-site/actor-authority class), and for EVERY FIX CYCLE — the fix is a change too (two of five findings in one round were regressions from the previous fix; an adversarial pass on a just-shipped fix found the silent-substitution bug it introduced). Attack the SEAMS between subsystems: an FSM edge declared with no driver, a guard validated at WRITE but unenforced at READ (write-only snapshots), a lineage predicate migrated at four consumers but not the fifth/sixth (sync, GC), "immutable" enforced by convention where an upsert path exists. Findings against a NEWLY-ACTIVATED feature get provenance triage — "this commit caused it" vs "this commit made it reachable" — and each is checked against the spec (one asked to reverse a decision the spec explicitly locked as "not v1").
**Source (additions)**: 2026-07-11-14.47, 2026-07-12-11.12, 2026-07-18-14.55, 2026-09-03-17.21, 2026-07-25-12.09, 2026-09-03-15.06

### Any-ever-true reads over append-only ledgers — flag as #temporal
**Source**: 2026-07-03-16.40
**Rule**: `rows.some(passed)` (or any historical-max read) over a temporal, append-only table (`gate_results`) is almost always wrong — the latest LIVE record governs, not the historical max: a stale `passed` from a superseded attempt must not satisfy a gate over a newer `pending`/`failed`, and advisory external checks bypass `readinessGreen()`. Require the SSOT reducer helpers (latest-attempt filter + newest-report collapse + allow-list readiness predicate) and a temporal test case (older passed attempt + newer live pending ⇒ `declared_not_passed`).

2026-07..09 extensions: a read-model time window on insertion `created_at` instead of the event's `occurred_at` → `#event-time`; a `domain_events` replay/bootstrap read without the commit-horizon fence `tx_id < pg_snapshot_xmin(pg_current_snapshot())` (an older in-flight tx is skipped forever) or a pagination cursor without a stable-item tiebreak → `#cursor-contract`; eligibility applied in a post-query validator after a bounded `LIMIT` (historical rows consume the limit) → `#limit-before-filter`; a first-match `LIMIT 1` with no deterministic `ORDER BY` → `#row-order`; an "unknown ⇒ leave as-is" rule on a nullable flag with no companion for states where the field is MEANINGLESS (`pr_has_conflicts` on a terminal PR) → `#null-semantics`.
**Source (additions)**: 2026-07-12-22.35-observatory-agentization-final-hardening, 2026-07-27-11.56, 2026-07-14-22.10, 2026-07-14-13.45, 2026-07-15-18.10

### force/skip/already-done flags computed from side-effect returns — flag as #retry-safety
**Source**: 2026-06-20-19.48
**Rule**: Any new `force`/`overwrite`/`skip`/`already-done` flag derived from a non-idempotent operation's RETURN value, on a path guarded by a durable claim/reclaim, is a retry-safety smell — the second attempt sees the already-consumed state (no-op squash → `forcePush=false` → non-fast-forward `CONFLICT`). Require derivation from the durable snapshot (a pure predicate like `squashesRunHistory(claim)`) and a reclaim-path regression test (stale claim + no-op side-effect + assert the flag).

2026-07..09 extensions: a "return existing on key conflict" branch without a content/digest compare (`existing.definitionDigest === contentDigest(definition)`) → `#silent-substitution`; an idempotency branch reached by a DIFFERENT payload that mutates anything → `#same-request-not-same-decision`; a non-deterministic value (`Date.now()`, random) injected ABOVE a dedup digest → `#nondeterministic-digest`; a provenance record copying the DECLARED value when the seam could not honor it, or an earlier stage's "immutable" record not overridden by the stage that actually decided (`flowRevisionId` from the launcher, like the branch pair per D7) → `#provenance-intent`.
**Source (additions)**: 2026-07-25-12.09, 2026-09-01-04.15, 2026-07-25-11.14, 2026-09-02-12.10

### Authority gates keyed on data attributes instead of the actor — flag as #actor-authority
**Source**: 2026-06-22-12.47
**Rule**: A gate deciding "automated vs human" from a data attribute of the object acted upon (`launchMode === "auto"` describes the child's ORIGIN) is wrong the moment a human acts on such an object — key on the actor/context (`isHumanPromotion(ctx)`: actor absent or `kind === "user"` ⇒ human; `system`/`agent` ⇒ unattended). Ask: "would a human ever act on an entity with that attribute?" — if yes, flag. Also verify a test's ACTOR matches its stated intent: a "manual promote" test driving the system/token entry point lies about its actor — check that before weakening a gate to keep it green.

### Kill-switches, disable paths, and gated resources — review as recovery paths
**Source**: 2026-07-04-14.30, 2026-07-02-15.32, 2026-07-02-17.52, 2026-06-30-13.02
**Rule**: A soft-disable/kill-switch operation must not require the broken dependency it is disabling — `updateAgentLink(..., { enabled: false })`-style pure metadata updates skip content validation (readability stays required for enabled-state edits); test disable-of-broken-resource separately from happy-path disable. When a feature has a disable switch or authorization gate, enumerate EVERY consumer of the gated resource and confirm each asserts the gate (ambient recall checked launch-time `runs.brain_context` but not live `projects.brain_enabled` — "disabled → no-op" must hold for every door) — prefer ONE predicate enforced INSIDE the performing service (`guard.ts` inside `recall()`/`retain()`) over copy-pasted caller checks (callers keep checks only for ordering/UX). "Already configured" (an id/fk exists) ≠ "currently valid/launchable" (the referenced resource still resolves and is enabled) — a gate relying on the former silently admits the latter's failure modes.

2026-07..09 extensions: for any freeze/snapshot/pin feature, grep the LIVE source's accessor inside the execution pipeline (`orderedStudyParticipantIds`, `isNull(removedAt)`, `flow.enabledRevisionId` after a pin exists) — "recorded for provenance" with an unswitched read path is the defect, and a WARN or provenance record over a known gap is a SURFACING measure, not a fix. Validation that lives only on an optional preview route while the action path never invokes it → `#advisory-only-gate`; a "co-evolve deferral" comment claiming a safety net ("preflight already refuses X") is verified against the actual gate. When a dead guard is re-enabled, the review covers the SECOND task — semantics never exercised (a `Running` root's tree breach) and compensating workarounds that depended on the bug ("X never fires, so we also do Y"). A guard's PRESENCE is not its REACHABILITY — trace each field it reads to the producer (`ref.parentRunId` never populated); ask whether its FAILING state is reachable in production (an always-non-empty `merge-base..branch`). Skipping a meter as "redundant" is checked against the LIMITS (the stricter ceiling binds), and a coupled default (two ceilings from one value) requires a coupled raise on exact equality. Every documented MUST names its enforcement point (a MUST with only a runtime symptom is a convention). Preflight probes assert only revision-invariant state; branch-owned config is validated from the execution revision. A proposed fence is verified to BLOCK the racing path before it is built (the runner takes no lifecycle claim).
**Source (additions)**: 2026-07-26-12.32, 2026-07-24-15.19, 2026-07-25-11.14, 2026-09-03-15.06, 2026-09-03-17.20, 2026-09-01-05.40, 2026-09-03-17.21, 2026-08-01-09.09

### Compensation past the point of no return — flag as #irreversible-rollback

**Source**: 2026-07-15-18.10, 2026-07-16-11.20, 2026-07-17-00.00, 2026-07-16-16.05, 2026-09-03-15.37, 2026-07-11-12.25
**Rule**: For every `catch`/rollback/restore in the diff, list the side effects that may ALREADY have landed above it (force-push, PR create, publish, a committed cascade). A handler that "cleans up" after an irreversible external effect manufactures divergence — flag `#irreversible-rollback` unless a point-of-no-return flag (`pushCommitted`) gates the compensation and the post-effect branch settles FORWARD (release the run, log the decisive SHAs) instead of restoring; a recovery arm that widens a restore list to a phase past that point (`pushing`) is the same finding — it must ask ORIGIN and settle forward, and an unproven push is never restored. When a fix "adds cleanup" (routing a catch through `failResolver`), check the sibling branch it now ALSO covers — round N's uniform cleanup became round N+1's corruption on the post-push path. Ask BOTH directions of every side-effect-before-transaction: "the side effect succeeds and the tx fails" is the one the author skipped (a GC'd worktree re-attached before the tx, `removed_at` rolled back, no sweep converges). A refused request must leave NO TRACE — a 4xx path that has already mutated (an already-terminal orchestrator's live descendants cascaded, then `409`) is `#validate-before-mutate`, however narrow the trigger. Filesystem mutation INSIDE a terminal DB transaction is `#post-commit-cleanup` — an expected release refusal must not roll back the terminal status. An OVERLOADED error code (`CONFLICT` = conflicted AND push-rejected) caught by a resolver-spawning handler is a `#try-scope` finding whose fix is structural (move the push outside the catch), not a smarter predicate.

### Test-quality review — seeds, mocks, deny-only tests, discriminants, and wire assertions

**Source**: 2026-07-16-16.05, 2026-07-15-18.10, 2026-07-27-20.55, 2026-09-03-14.30, 2026-07-21-23.50, 2026-08-06-19.10, 2026-07-16-14.05, 2026-07-27-21.50, 2026-07-14-13.45, 2026-07-27-09.30, 2026-07-12-11.12, 2026-09-04-01.33, 2026-09-03-16.10, 2026-07-16-13.03, 2026-09-01-05.40, 2026-09-03-15.06, 2026-09-03-21.47, 2026-09-03-23.46
**Rule**: Review tests as code that can lie. Flag `#test-seed-realism` when a fixture seeds a state no production writer produces (a claim with no `lifecycle_operation_attempt_id`, `prState:"closed"+prHasConflicts:true`, a self-stamped `root_run_id`, a hand-built identity column, a paused run with no in-process driver, a branch initialized exactly at `main` so the guard fires) — correct the seed, never weaken the gate; read the DB CHECK constraints before judging a seed (`hitl_requests` task binding). Flag `#wrong-layer-race` when a two-racer runs at a layer whose identical I/O self-serializes (route, not store) or contends on the LOCK the author wrote instead of the INVARIANT (two admissions under one parent cannot expose an ancestor-scoped count; the winner must INSERT uncommitted rows). Flag `#timing-assertion` when a test infers "X already happened" from which promise settled first instead of awaiting a signal, asserts one ordering of a race, or leaves a barrier unreleased on failure (a parked winner contaminates the next test). Flag `#wire-not-asserted` when a `details`/`code`/header exists for a client but no test reads it off a real `Response` — a value placed in a MOCK is not tested ("if I deleted this field from the mock, would any assertion fail?"). Flag `#deny-only` when every test naming a new scope/grant asserts a refusal — a deny-only suite cannot distinguish "correctly refused" from "does not work" (an oracle fix made the granted path unreachable and stayed green); require the positive end-to-end case. Flag `#non-discriminating` when the asserted outcome is also produced by the bug (`status === "Failed"`; `expect(code).toBe("CONFLICT")` satisfied by a second mechanism) — require the field only the new path produces, and `errors: []` on composition summaries. Flag `#pins-weak-behavior` when an assertion's comment explains why a questionable behavior is fine (viewer→200 on repo-class data) — the behavior, not the test, is the finding; a role mock that refuses only a non-viewer permission is not a viewer. Read a fixture's DATA, not its comment (a "colliding" fixture with distinct refs proves the weaker property); a "self-X" test for a NEIGHBOURING concept is not coverage of the new one — grep the specific actor. A security-control test must assert the BYPASS the control exists to stop (the scp-form `ext::…@h:` shape), not a payload already rejected for an unrelated reason. Flag a hook mock returning a fresh function per render (`next-intl`) as an OOM loop, a page-wide `getByText` status assertion (shared chrome renders every sibling run's status), and an e2e fixture sharing one mutable git ref across scenarios. Run integration tests with the REAL child kind the fixture produces.

### Process-boundary review — single-flight lookups, fake parity, wire order, connection threading, required params

**Source**: 2026-09-04-00.21, 2026-07-11-14.47, 2026-07-11-21.30, 2026-07-11-23.47-mcp-v2-review-fixes, 2026-07-15-18.10, 2026-07-27-20.55, 2026-09-01-04.15, 2026-09-03-17.21, 2026-07-27-11.56, 2026-09-03-15.06
**Rule**: Ask of every resolver/registrar/single-flight promise: "does anything inside it call back into the same resolver?" — a lookup that awaits a DOMAIN sweep (`runReconcileSweep` → `runFlow` → the same resolver) is a process-wide deadlock, flag `#single-flight-reentry`. When a contract spans two processes, require the PARITY test (one scenario table against the fake AND a real peer) — a fake not diffed rule-by-rule against its peer proves the fake's contract, flag `#fake-drift`. For interceptors keyed on ACP event order, confirm the fixture emits the REAL adapter sequence (`tool_call` pending streams BEFORE `requestPermission`; execution = `tool_call_update` completed/failed) — a mock modelled on the author's mental model proves nothing, flag `#mock-order`. A guard whose policy/param gates a security refusal and is OPTIONAL is a caller-gap generator — require it REQUIRED so a dropped caller fails typecheck (`assertEnforcementEvidence` never received `permissionPolicy`; `verifySyncGate` gained a `branch` a `sed` sweep missed at the third caller) — prefer the type system over coverage when sweeping a new argument to N sites, flag `#optional-param-sweep`. A value derived in one module and accepted in another needs the SEND-site test (the spy on the outgoing `createSession` body). Derive-then-inject (append the facade to `mcpServers` after `allowServers` was derived) is `#ordering`. Inside any db-injected function (`materialize*`, `launch*`, anything taking `db`/`_db`/`tx`), grep every `loadX(id)` for a `getDb()`-fallback helper that did not receive the caller's connection — "does this DB call use the SAME connection as its siblings?" (`#getDb-bypass`: a second connection is a tx-isolation bug). An entry point serving two lifecycles (`startAgentSession` with `resumeSessionId`) needs a gate per lifecycle. When a helper is called, check its parameter semantics against its SIBLING call sites (`markDownstreamStale` passed `[target]` where both neighbours pass `[node, ...downstreamOf(graph, node)]`), and whether the neighbouring code already uses a helper for the same input (`budgetFromSnapshot`). When one replay reader uses a consistency fence, confirm every sibling replay surface has it. Two functions answering the same classification question (`deleteSession` vs `deleteSessionIfPresent`) with the correct one unused on the path that matters — that is the bug.

### Comments, prescriptions, and prose are claims — verify the inference, not just the file:line

**Source**: 2026-07-14-13.45, 2026-07-17-00.00, 2026-09-03-14.30, 2026-09-03-17.21, 2026-07-16-14.05, 2026-07-16-16.40, 2026-07-16-16.05, 2026-07-27-21.50, 2026-09-03-15.37, 2026-09-03-17.20, 2026-07-12-20.46-observatory-agentization-review-remediation, 2026-09-03-23.46
**Rule**: A comment asserting an invariant ("flows.id is a UUID PK", "a sync can only ever be claimed by a Review run", "under the same lock that already serializes this orchestrator", "the scratch finalizer is never a tree root") is a LEAD, not evidence — grep whether anything ENFORCES it (open the schema: `text()` vs `uuid()`; find the lock; find the filter) and flag `#unenforced-invariant` when nothing does. A plan's or reviewer's PRESCRIBED fix is a hypothesis: re-derive the defect from source before applying it (three of one plan's fixes were wrong AS WRITTEN — routing the victim through the function that IS the thief; a data source that only exists for Review runs while reopen targets Done runs); verify the PREMISE of a prescription ("every sibling ext route uses a path param" — 7 of 9 put `runId` in the body); verify the CONSEQUENCES, not just the mechanism (two of a finding's stated consequences were refuted by recovery code the reviewer never opened); an accurate file:line does not validate the inference attached to it (which file, which mechanism, which DIRECTION — a spec `nullable` became MORE wrong after the cited change); "verified in source" on the observation does not transfer to the prescription; when a plan says "X becomes right after Y", re-derive after Y lands. A "deliberate non-goal" from a prior round is not evidence — re-derive it when challenged. When spec PROSE and a spec TABLE disagree, the prose usually carries the reasoning and the table the shortcut — reconcile before trusting either. Group defects by CONTROL FLOW, not by grep: a shared callee is a hypothesis; confirm each caller reaches the hazardous sequence before filing them as one class (two of four "same-shape" cascade sites had no exposure; one was misfiled). Check whether a "race" is actually DETERMINISTIC before prescribing a concurrency fix. Misleading log wording (`"skipped crash"` on a path that destroyed a sub-tree) and an audit line that names only the triggering scope are findings. Confirm page reads that must be cache-only (Observatory) make no Git or scheduler call.

### Recovery-window and orphan completeness — every parking spot has an owner

**Source**: 2026-07-16-16.05, 2026-07-15-11.19-gate-chat-recovery, 2026-07-16-11.20, 2026-09-03-21.47, 2026-09-03-23.46, 2026-09-02-12.10, 2026-07-15-18.10, 2026-07-16-13.06, 2026-07-17-00.10-workspace-lifecycle-review-remediation
**Rule**: For any feature that parks a run/turn/claim, require a NORMATIVE recovery-window table (every `status × mode` cell) in the ADR/analytics and check each cell has an arm — the resolver's own parking spot (`NeedsInput`, `agent_running`) had none because reconcile was `Running`-only; a mechanical sync parks in `Review` and had none either (`#uncovered-cell`). A recovery arm that enumerates phases POSITIVELY is diffed against every `setPhase*` call in the driver — grep the PRODUCER, not the arm; when the set grows on the producer's side, the arm derives the complement (`#enumeration-drift`). Under a dead coordinator, ask what each child can be HOLDING (crash paths never cascade) and require per-status handling (`Pending → Abandoned`, awaiting states → `Crashed` with the HITL row closed in-tx, `HumanWorking` left + WARN, sub-orchestrators via the stuck path); coordinator death is ANY terminal parent, derived from the TS constant via `sql.join`, never a literal `Crashed|Abandoned` pair. A new route into an existing arm inherits that arm's preconditions — check what the classifier guaranteed for the old route (`!liveSession`) before reusing its action. A choke point's CAS SOURCE SET is checked against every caller's need (`finalizeAgentRun` still refused two of three orphan statuses after the classifier was fixed). Any durable interaction settled by an in-memory callback (lease timers) needs a reconciliation owner after restart, and each recovery lease is acquired at PROCESSING time. A function whose result says "what you must do next" (`cascadedRunIds`) with callers that discard it is `#half-composition`; the crash window between the halves needs a sweep. A "best-effort, keep going" comment is checked against the degenerate case (ALL sources refused → the opposite policy). Compare a batch's worst case against its lease (`batch × per-call budget vs lease`) and derive the bound from the container's real value; a lease around a LONG external operation needs renewal AND a fence at the persistence boundary, with stale-completion tests; a targeted orphan loader (`NOT EXISTS parent alive`) beats widening a hot sweep. Service-level bundle failure and per-candidate retry are distinct outcomes.