Menü
This skill should be used when performing exhaustive code reviews using multi-agent analysis, ultra-thinking, and worktrees.
Mit Codex oder Claude installieren Kopieren Sie diesen Prompt, fügen Sie ihn in Codex, Claude oder einen anderen Assistant ein und lassen Sie die Skill-Seite prüfen und installieren.
Basierend auf der SOC-Berufsklassifikation
This skill should be used when auditing the recurring per-Anthropic-model-release checklist (model IDs, claude-code-action pin freshness, pricing drift, tier-map re-evaluation): it auto-fixes stale model-ID swaps into a CI-gated PR and flags the rest.
This skill should be used when designing agent-native applications where agents are first-class citizens: architecting autonomous agents, creating MCP tools, building apps where features are agent-driven outcomes.
This skill should be used when working with DSPy.rb, a Ruby framework for type-safe, composable LLM applications.
This skill provides a promptfoo eval harness that measures whether a Soleur skill or agent edit actually improves behavior, comparing a skill arm against a baseline control arm.
This skill should be used when resolving all TODO comments in the codebase using parallel processing. It analyzes dependencies, creates a resolution plan with a mermaid flow diagram, and spawns parallel resolver agents.
This skill should be used to widen an existing Cloudflare API token's scope via Playwright dashboard automation, then run the ADR-130 retained-scope probe set verifying the target scope was added and none was dropped.
| name | review |
| description | This skill should be used when performing exhaustive code reviews using multi-agent analysis, ultra-thinking, and worktrees. |
Lifecycle handoff (standalone /review): When no parent orchestrator (one-shot, work) owns the pipeline, invoke /compound then /ship after review — do not end at the review summary. In pipeline mode, emit the compact ## Review Phase Complete marker only (see Step 3 pipeline detection).
Dynamic-workflow alternative (opt-in). A
Workflow-tool port of this skill's engine lives atworkflows/review.workflow.js— deterministic change-class fan-out, per-finding adversarial verification, and CONCUR-gated filing. Run it withWorkflow({ scriptPath: "plugins/soleur/skills/review/workflows/review.workflow.js", args: "<PR#>" }). Seeworkflows/README.md. The prose skill below stays the default; the two coexist during calibration.
<command_purpose> Perform exhaustive code reviews using multi-agent analysis, ultra-thinking, and Git worktrees for deep local inspection. </command_purpose>
Senior Code Review Architect with expertise in security, performance, architecture, and quality assurance
Load project conventions:
# Load project conventions
if [[ -f "CLAUDE.md" ]]; then
cat CLAUDE.md
fi
Read CLAUDE.md if it exists - apply project conventions during review.
<review_target> #$ARGUMENTS </review_target>
First, I need to determine the review target type and set up the code for analysis.<task_list>
skill: git-worktree with branch namegh pr view --json for title, body, files, linked issuesEnsure that the code is ready for analysis (either in worktree or on current branch). ONLY then proceed to the next step.
</task_list>
Before spawning review agents, classify the PR to avoid spawning agents whose expertise is irrelevant to the change.
Run git diff --name-only origin/main...HEAD | head -n 200 to get the list of changed files. Also capture status letters and line counts. Use git rev-parse --git-dir to resolve a writable tmp path that works in both regular checkouts and worktrees: in a worktree .git is a file (gitdir pointer), not a directory, so > .git/review-*.txt fails with Not a directory (os error 20). The resolver returns the worktree's actual gitdir (e.g., <bare>/worktrees/<name>/):
REVIEW_TMP="$(git rev-parse --git-dir)"
git diff --name-only origin/main...HEAD > "$REVIEW_TMP/review-changed.txt"
git diff --name-status origin/main...HEAD > "$REVIEW_TMP/review-status.txt"
git diff --numstat origin/main...HEAD > "$REVIEW_TMP/review-numstat.txt"
All downstream cat .git/review-*.txt references in the predicates below must use "$REVIEW_TMP/review-*.txt" instead. The pre-existing .git/... literals work in non-worktree checkouts but silently break in worktrees (where every PR review increasingly happens by default).
Check for override: scan $ARGUMENTS for "deep review" or "full review". Also run gh pr view --json body,title --jq '.body + " " + .title' and check for the same phrases. If override detected, skip classification and spawn all 8 agents.
Apply the four-class decision tree below in order; first match wins (override always trumps):
If $ARGUMENTS or PR body/title contains "deep review" / "full review":
class = code (full override) → 8 agents
Else if every changed file matches the lockfile glob OR
(lockfile glob + optional knowledge-base/** or *.md edit)
AND zero source-code extensions are present:
class = lockfile-only → 2 agents (git-history-analyzer + security-sentinel)
Else if total_files > 0 AND total_lines > 0 AND
(deleted_files * 100 / total_files) >= 80 AND
(deleted_lines * 100 / total_lines) >= 80 AND
zero source-code extensions are present in the diff:
class = deletion-dominated → 2 agents (git-history-analyzer + security-sentinel)
Else if any changed file has a source-code extension:
class = code → 8 agents
Else:
class = non-code → 4 agents
The "zero source-code extensions" guard on deletion-dominated closes a piggyback class: a 1000-line cleanup PR that adds a 50-line .ts file would otherwise route to 2 agents and bypass pattern-recognition / code-quality / architecture / data-integrity / performance / agent-native review on the new source file. Mirroring lockfile-only's $has_source empty requirement keeps the savings on legitimate orphan-cleanup PRs while routing any deletion-dominated PR with new source code through the full 8-agent path.
Source-code extensions: .ts, .tsx, .js, .jsx, .rb, .py, .go, .rs, .swift, .kt, .java, .c, .cpp, .cs, .php, .sh, .bash, .zsh, .mjs, .cjs — any file containing executable logic. Non-code: .md, .txt, .yml, .yaml, .toml, .json, .css, .html, .njk, .svg, .png, .jpg, .gif, .pen, LICENSE, CHANGELOG*, .github/** workflow files, and plugin/agent/skill definition files (plugins/**/*.md, agents/**/*.md).
Compute the predicates inline (set -uo pipefail — drop the e so legitimately-empty greps don't abort):
total_files=$(wc -l < "$REVIEW_TMP/review-changed.txt")
deleted_files=$(grep -cE '^D' "$REVIEW_TMP/review-status.txt" || true)
added_lines=$(awk 'BEGIN{s=0} {if ($1 != "-") s += $1} END{print s}' "$REVIEW_TMP/review-numstat.txt")
deleted_lines=$(awk 'BEGIN{s=0} {if ($2 != "-") s += $2} END{print s}' "$REVIEW_TMP/review-numstat.txt")
total_lines=$((added_lines + deleted_lines))
LOCKFILE_RE='(^|/)(package-lock\.json|bun\.lock|yarn\.lock|Cargo\.lock|go\.sum|Gemfile\.lock|poetry\.lock|uv\.lock)$'
ALLOWED_NONLOCK_RE='^(knowledge-base/|.*\.md$)'
SOURCE_RE='\.(ts|tsx|js|jsx|rb|py|go|rs|swift|kt|java|c|cpp|cs|php|sh|bash|zsh|mjs|cjs)$'
non_lock_files=$(grep -vE "$LOCKFILE_RE" "$REVIEW_TMP/review-changed.txt" || true)
non_lock_non_doc=$(printf '%s\n' "$non_lock_files" | grep -vE "$ALLOWED_NONLOCK_RE" | grep -v '^$' || true)
has_source=$(grep -E "$SOURCE_RE" "$REVIEW_TMP/review-changed.txt" | head -1 || true)
any_lockfile=$(grep -E "$LOCKFILE_RE" "$REVIEW_TMP/review-changed.txt" | head -1 || true)
lockfile-only matches when $non_lock_non_doc is empty AND $any_lockfile is non-empty AND $has_source is empty.deletion-dominated matches when total_files > 0 AND total_lines > 0 AND (deleted_files * 100 / total_files) >= 80 AND (deleted_lines * 100 / total_lines) >= 80 AND $has_source is empty. Bash arithmetic evaluates left-to-right; multiply-first avoids the integer-truncation-to-zero trap. Note: git diff --name-only does not distinguish added/deleted paths, so $has_source may match a path that is itself a deletion — this is intentionally conservative (we want zero source-file activity in either direction) and prevents a piggyback attack where a backdoor .ts file rides along on a bulk-deletion cleanup PR.Announce the classification result before spawning agents.
<parallel_tasks>
If override is detected (deep review / full review), spawn all 8 agents regardless of class:
Else if class is code (any source-code extension and not deletion-dominated/lockfile-only), spawn all 8 agents (existing behavior).
Else if class is non-code (no source files, not lockfile-only or deletion-dominated), spawn 4 agents:
Skipped for non-code PRs: architecture-strategist, performance-oracle, data-integrity-guardian, agent-native-reviewer. These agents analyze source code structure, runtime performance, database integrity, and agent accessibility — none are relevant to documentation, configuration, or CI changes.
Else if class is lockfile-only or deletion-dominated (and override not detected), spawn 2 agents:
Skipped for lockfile-only / deletion-dominated PRs: pattern-recognition-specialist, code-quality-analyst, architecture-strategist, performance-oracle, data-integrity-guardian, agent-native-reviewer. Lockfile diffs and bulk deletions do not contain semantic patterns or quality regressions for the pattern/quality agents to find; architecture/perf/integrity/agent-native agents have no source code to analyze. Use deep review to force full pipeline.
Announce: "Change classified as [code/non-code/deletion-dominated/lockfile-only]. Spawning [N]/8 review agents. [If skipped agents: Skipped: — not relevant to changes. Use 'deep review' to force full pipeline.]"
</parallel_tasks>
Note: The conditional agents block below (agents 9-14: Rails reviewers, migration experts, test-design-reviewer, semgrep) is unaffected by the classification gate. Both gates run independently — the classification controls only the always-on agents above.
<conditional_agents>
These agents are run ONLY when the PR matches specific criteria. Check the PR files list and project structure to determine if they apply:
If project is a Rails app (Gemfile AND config/routes.rb exist at repo root):
When to run Rails review agents:
Gemfile and config/routes.rbWhat these agents check:
kieran-rails-reviewer: Strict Rails conventions, naming clarity, controller complexity, Turbo patternsdhh-rails-reviewer: Rails philosophy adherence, JavaScript framework contamination, unnecessary abstractionIf PR contains database migrations (db/migrate/*.rb files) or data backfills:
When to run migration agents:
db/migrate/*.rbWhat these agents check:
data-migration-expert: Verifies hard-coded mappings match production reality (prevents swapped IDs), checks for orphaned associations, validates dual-write patternsdeployment-verification-agent: Produces executable pre/post-deploy checklists with SQL queries, rollback procedures, and monitoring plansON DELETE RESTRICT FK AND a same-PR RPC documented in the migration COMMENT as the cascade pre-step (pattern: MUST call <rpc_name> or runbook MUST call), the reviewer MUST run git grep -n '<rpc_name>' and require at least one match outside supabase/migrations/, knowledge-base/, and the plan file. The migration's own prose is the STATEMENT of the obligation, not evidence the obligation is satisfied. See [[2026-05-16-migration-mandates-must-have-wired-call-sites-in-same-pr]] (PR #3853 surfaced this via five concurring agent findings).If PR contains test files:
When to run test review agent:
*_test.rb, *_spec.rbtest_*.py, *_test.py*.test.ts, *.test.js, *.spec.ts, *.spec.js*_test.go*_test.swift, *Tests.swift__tests__/ or spec/ or test/ directoriesWhat this agent checks:
test-design-reviewer: Scores tests against Farley's 8 properties, produces a weighted Test Quality Score with letter grade and top 3 improvement recommendationsIf PR modifies source code files, semgrep-sast is a mandatory gate:
When to run SAST agent:
.sh/.bash/.zsh, no other source extensions): OSS semgrep's tree-sitter bash parser cannot analyze bash files end-to-end (parses ~100% of lines but matches 0 rules — vacuous "0 findings"). Skip semgrep-sast and substitute shellcheck as the deterministic gate. See knowledge-base/project/learnings/2026-05-19-cache-llm-outputs-flag-for-rerunnable-benches.md for the bench-pattern session that surfaced this. Why: PR #4045 — semgrep-sast on a 1336-line bash diagnostic returned vacuous output that could mislead future readers; shellcheck is the bash-native equivalent.Bootstrap (mandatory before spawning the agent): Run ensure-semgrep.sh from the repo root. The script checks PATH first, then auto-installs via brew → pipx → pip --user in that order. Exits 0 when semgrep is reachable. Exit 1 means an install was attempted and failed; exit 2 means no install path was available (no brew, pipx, or python3 with pip). On non-zero exit, print the script's stderr to the user and abort the review. Do NOT silently skip — the deterministic SAST pass is what catches CodeQL-equivalent patterns like js/file-system-race before push.
Custom rules file: semgrep-custom-rules.yaml ships alongside the public rule packs and covers CodeQL queries the public packs miss (e.g. the TOCTOU patterns that blocked PR #2463 in CI). The semgrep-sast agent loads it via --config=plugins/soleur/skills/review/references/semgrep-custom-rules.yaml. Extend it whenever a CodeQL finding in CI was not caught locally — the goal is no-surprises on CI. Run semgrep from the worktree/repo root — that --config path is repo-root-relative, so a persisted cd apps/web-platform (left over from a prior tsc/vitest call; the Bash tool keeps CWD across calls) makes semgrep exit 7 config path does not exist. Use cd <root> && semgrep … in one call, or pass an absolute --config. Why: #4742.
What this agent checks:
semgrep-sast: Known vulnerability signatures (CWE patterns), hardcoded secrets, insecure function calls, taint analysis. Complements security-sentinel's LLM-based architectural review with deterministic rule-based scanning.If the plan declares Brand-survival threshold as single-user incident:
## User-Brand Impact section mitigates or scope-outs eachWhen to run user-impact-reviewer:
Brand-survival threshold: single-user incident## User-Brand Impact section with that threshold labelWhat this agent checks:
user-impact-reviewer: Enumerates concrete user-facing artifacts exposed by the change (user.email, workspace.name, api_key.token, conversation.id, message.body, billing.amount, oauth.installation_id, etc.) AND a concrete exposure vector per artifact (cross-tenant read, RLS bypass, credential leak in logs, data loss on rollback, double-charge on retry, silent drop on degraded fallback). Rejects generic boilerplate (e.g., "users experience a bug", "error state", TBD/TODO placeholders). Coexists with security-sentinel — security-sentinel handles OWASP/CWE scanning across all PRs; user-impact-reviewer handles user-facing-outcome enumeration when the plan declares the brand-survival threshold as single-user incident.If the diff matches hr-gdpr-gate-on-regulated-data-surfaces:
When to run gdpr-gate at review time:
git diff main...HEAD --name-only | grep -E "$CANONICAL_REGEX" returns at least one match (mirrored regex source: plugins/soleur/skills/gdpr-gate/SKILL.md §"Path globs (canonical)").What this agent checks:
gdpr-gate: Deterministic Art. 9 / RoPA / lawful-basis pattern checks. Output is advisory-only; Critical findings (Art. 9) escalate to operator-acknowledged write to compliance-posture.md Active Items + GitHub issue with label compliance/critical.If the diff touches a domain-model business-rule surface (#5871):
git diff main...HEAD --name-only matches (^|/)apps/web-platform/supabase/migrations/.*\.sql$, (^|/)apps/web-platform/server/workspace-resolver\.ts$, or (^|/)knowledge-base/engineering/architecture/domain-model\.md$ (same surface as preflight Check 11). Run bash scripts/domain-model-drift.sh drift --repo . --register knowledge-base/engineering/architecture/domain-model.md and surface one informational line in the review summary: domain-model register: N stale citation(s), M undocumented table(s) — see /soleur:sync domain-model. Purely informational — the blocking enforcement is preflight Check 11 (stale-only); the non-redundant value here is the undocumented-facts pointer, which the ship gate deliberately does not surface (the register is a curated subset). Never blocks; no coordination logic.Use gdpr-gate for deterministic Art. 9 / RoPA / lawful-basis pattern checks; use data-integrity-guardian for migration safety and judgment-based PII review; use security-sentinel for OWASP/CWE security-of-processing flaws AND multi-org / workspace boundary integrity (the R1–R6 checklist — RLS routing through is_workspace_member(), JWT current_organization_id consumption, attestation owner-checks, SECURITY DEFINER search_path pinning, write-boundary sentinel on workspace_id-bearing tables). The three reviewers complement each other and may all fire on the same migration PR — each owns a distinct lens. This is the canonical disambiguation prose; sibling agent files reference back here as the single source of truth.
If the diff touches apps/web-platform/(app|components)/.*\.(tsx|jsx|css)$ OR apps/web-platform/server/.*\.(ts|tsx)$ OR plugins/soleur/docs/.*\.(njk|css)$:
Run the soleur:frontend-anti-slop Tier 1 scanner inline (no separate agent spawn — v1 simplification per plan PR #4265). Scope covers the Next.js platform, the server-side email/HTML templates, and the Eleventy marketing site so AI-assisted edits to landing pages, transactional emails, or blog posts get the same audit as React component changes.
# Keep NUL framing end-to-end. The host `grep` is ugrep, where the NUL-data
# flag means `--decompress` (NOT GNU `--null-data`) and silently matches
# zero files (the #4635 false-clean). Do NOT use grep at all in this
# collector: read the NUL-delimited diff with `read -r -d ''` and match each
# path against EXT_RE in bash, so filenames containing literal newlines
# survive intact. The path regex mirrors `DEFAULT_PATH_RE_SOURCE` in
# tier1-scan.ts (parity-tested).
EXT_RE='(apps/web-platform/(app|components)/.*\.(tsx|jsx|css)|apps/web-platform/server/.*\.(ts|tsx)|plugins/soleur/docs/.*\.(njk|css))$'
CHANGED_FILES=()
HAS_EXT_FILE=0
while IFS= read -r -d '' f; do
[[ "$f" =~ $EXT_RE ]] && CHANGED_FILES+=("$f")
[[ "$f" =~ \.(tsx|jsx|ts|css|njk)$ ]] && HAS_EXT_FILE=1
done < <(git diff --name-only -z origin/main...HEAD)
if (( ${#CHANGED_FILES[@]} > 0 )); then
bun run plugins/soleur/skills/frontend-anti-slop/scripts/tier1-scan.ts \
--paths "${CHANGED_FILES[@]}" --json
elif (( HAS_EXT_FILE == 1 )); then
# Guard against silent false-clean: the diff DOES contain scanner-extension
# files but none matched the scope regex (or the collector mis-fired).
# Warn loudly instead of reporting clean — this is the #4635 failure class.
echo "WARNING: diff contains scanner-extension files but none matched the anti-slop scope regex; the scanner did NOT run — verify the path regex / collector did not silently drop files." >&2
fi
What this hook checks:
transition-all, uniform hover:scale-105, placeholder names, zero-chroma neutrals, off-scale spacing, prose-width out of range, two-icon-library imports, plus 3 brand-category gates (raw hex, white-on-gold contrast, non-zero corners), etc. See slop-rules.md.soleur:frontend-anti-slop SKILL.md §"Calibration mode").brand findings are a required-fix gate, NOT operator triage. When the scanner reports a finding whose originating rule is category: brand and severity: high (BRAND-RAW-HEX, BRAND-WHITE-ON-GOLD), the scanner exits non-zero (1) — the diff must be fixed before merge, the reviewing agent does not get to narrate it away as a likely false positive. Brand medium findings (BRAND-NONZERO-CORNER) stay advisory like the rest.finding.schema.json with category: "anti-slop", selector: "<file-path>#<RULE-ID>". Pretty-print the JSON array directly into the review output as a fenced code block; the reviewing agent narrates which findings look like true positives.</conditional_agents>
<decision_gate>
After all parallel and conditional agents complete, check their outputs:
This is a binary gate: all-failed triggers the fallback; any-succeeded means continue.
</decision_gate>
<ultrathink_instruction> For each phase below, spend maximum cognitive effort. Think step by step. Consider all angles. Question assumptions. And bring all reviews in a synthesis to the user.</ultrathink_instruction>
Complete system context map with component interactions<thinking_prompt> ULTRA-THINK: Put yourself in each stakeholder's shoes. What matters to them? What are their pain points? </thinking_prompt>
<stakeholder_perspectives>
Developer Perspective
Operations Perspective
End User Perspective
Security Team Perspective
Business Perspective
<thinking_prompt> ULTRA-THINK: Explore edge cases and failure scenarios. What could go wrong? How does the system behave under stress? </thinking_prompt>
<scenario_checklist>
Run the Task code-simplicity-reviewer() to see if we can simplify the code.
When reviewing a PR that changes *.njk, *.md, README, or content under
apps/**, scan every fenced code block tagged bash, sh, shell, or
untagged-but-CLI-shaped. For each <command> <subcommand> pair:
WebFetch or run <tool> --help. If unsure, flag as
cli-verification-unverified and require an explicit annotation or
citation before approving../scripts/*,
plugins/soleur/skills/*/scripts/*), verify the script exists at the
path.<model>:<tag>,
@<version>), fetch the registry or cite the registry URL.Flag any unverified CLI invocation as P1 (docs-trust) — NOT P3 polish. A fabricated CLI command on a high-intent landing page breaks first-touch trust (#1810/#2550).
Workspaces-flag precondition: When the diff documents an npm run -w <workspace> <script> invocation, grep the repo-root package.json for "workspaces" and refuse the documented form if the field is absent. Without a root workspaces: declaration, npm aborts with "No workspaces found". The grep is one line; the false-negative cost is an operator runbook that returns the error on first use. Why: PR #3751 — see knowledge-base/project/learnings/2026-05-13-npm-workspaces-flag-fails-without-root-workspaces-declaration.md.
When a review agent claims that a build-step CI gate (e.g., post-Eleventy
grep -rEn ... _site/, post-Webpack chunk regex, post-tsc output scan)
will fail on rendered output, rebuild the artifact directory BEFORE
running the gate locally. Never run the gate against an existing
_site/, dist/, build/, or .next/ from a prior session — those
predate the source change under review and return false-pass (zero
matches) even when the rendered output post-rebuild contains the flagged
strings.
The verification command order is non-negotiable:
<rebuild command> && <literal CI gate command>
Examples:
npx @11ty/eleventy --quiet && grep -rEn '<regex>' _site/bun run build && grep -rEn '<regex>' .next/If the rebuild step is unfamiliar, read the corresponding .github/workflows/
job to find the exact build command the gate runs against — match it, do
not invent one. A stale-artifact false-pass is the most common dismissal
class for build-output gates (PR #3296 → #3347 hotfix). Treat any agent
finding of the form "rendered/built artifact X contains Y" as a
fresh-build-required claim by default.
<critical_requirement>
Each finding's default action is to FIX IT INLINE on the PR branch: make the edit,
commit with a message review: <summary> (P<N>), and push. Apply to P1, P2, P3
equally.
Cost-of-filing gate (FIRST FILTER — apply BEFORE invoking the CONCUR
second-reviewer gate AND BEFORE evaluating the four scope-out criteria below):
If the fix is ≤100 lines of code AND touches ≤4 files AND no reviewer agent
independently dissents on technical grounds (e.g., contested-design with named
alternatives), fix inline. The bookkeeping cost of gh issue create + scope-out justification + future triage + closure + follow-up PR averages ~30 minutes of
cumulative human attention, and that cost is fixed — it does not shrink
with the size of the deferred fix. The edit cost is what scales: a ≤100-line
edit runs roughly 5–20 minutes. So the two curves cross well above the old
30-line boundary, and everything below the crossover is NET-NEGATIVE work to
file.
Why 100/4 and not 30/2 (raised 2026-07-20). The old boundary was set when filing looked cheap. Measured over the 7 days to 2026-07-20: 269 issues filed against 132 merged PRs (2.04 filed per PR) and 125 closed, growing the queue +144/week — up from +7.2/day over the prior 23 days. A 30-line boundary sends most real findings to the queue, and the queue does not drain. Raising to ≤100 lines AND ≤4 files moves the crossover to where the arithmetic actually sits. This threshold is instrumented, not guessed: every disposition emits a telemetry row (see the auto-flip below), so the next tuning pass reads data instead of re-arguing from intuition.
This gate is load-bearing: a PR that opens more issues than it closes is a
workflow failure, not a normal review outcome. That is now enforced rather
than asserted — see the blocking net-issue-flow gate in
ship/SKILL.md and
net-issue-flow.sh.
Mechanical pre-CONCUR auto-flip:
Before invoking code-simplicity-reviewer, self-assess fix size. If ≤100 lines AND ≤4 files, BYPASS the CONCUR gate — the disposition is auto-flipped to fix-inline. Apply the fix; do not file.
Instrumentation (REQUIRED, not optional). Emit one telemetry row per finding disposition, so the next threshold tuning reads measured flip-vs-file ratios instead of re-arguing from intuition. This is the half of the change that makes the next change cheap:
source "$(git rev-parse --show-toplevel)/.claude/hooks/lib/incidents.sh"
# DISPOSITION is exactly one of: flip-inline | file
emit_incident "cost-of-filing-${DISPOSITION}" applied \
"review disposition: ${DISPOSITION} (${LINES} lines, ${FILES} files)"
The disposition rides in the rule_id (cost-of-filing-flip-inline vs
cost-of-filing-file) and the event stays applied. That is not a stylistic
choice: the rule-metrics-aggregate.sh report keys every counter on rule_id and
gates on event_type ∈ {deny,bypass,applied,warn} — it never reads .kind,
so a kind-based scheme would write rows that no report ever surfaces. Read the
resulting ratio with bash scripts/rule-metrics-aggregate.sh and compare the
two applied_count values.
If the fix size cannot be confidently bounded without writing it, write a 5-minute spike. If the spike exceeds 100 lines, run CONCUR; if it doesn't, commit the spike. Do NOT run CONCUR on a fix you've already written and verified to be small.
The gate fails (fix-inline is required) when:
pr-introduced (per Step 1 provenance triage) — these always
fix inline.X into the sibling set for the first time — the
asymmetry is pr-introduced regardless of when X's underlying capability
shipped. Mechanical test: git diff origin/main --name-only | xargs grep -l "<X>" against the sibling set on main. If main had zero X mentions
across {A, B, C} and this PR adds X to A only, the asymmetry between
A-present and (B, C)-silent is created by this PR. The pre-existing-unrelated
scope-out criterion fails; fix inline. Why: PR #3755 (#3708) tried to file
gdpr-policy/privacy-policy Sentry-gap as pre-existing-unrelated;
code-simplicity-reviewer DISSENTed precisely on this rule. See
knowledge-base/project/learnings/2026-05-14-discrete-enumeration-relockstep-and-pr-introduced-asymmetry.md.The gate may pass (proceed to evaluate the four scope-out criteria) when:
Filing a GitHub issue instead of fixing is allowed ONLY when both the cost-of- filing gate above AND one of these four scope-out criteria are satisfied:
apps/web-platform/, plugins/soleur/) as the primary
changed file. Bare multi-file fixes do NOT qualify; the unrelatedness
must be concrete and defensible — count specific files or drop the
scope-out.main before this PR and
is not exacerbated by the PR's changes. (Does NOT block merge.) Only
reachable through the pre-existing branch of the provenance triage in
Step 1 below — never applies to pr-introduced findings. Mirroring an
existing brittle pattern "for symmetry" is exacerbation, not preservation:
if git diff origin/main...HEAD -- <file> | grep '^+' | grep <pattern>
returns ≥1 line, the criterion fails — fix inline. See
knowledge-base/project/learnings/2026-05-04-in-isolation-probe-missed-user-shape-and-scope-out-exacerbation.md.When filing:
## Scope-Out Justification section naming the
specific criterion and a 1-3 sentence rationale.--label deferred-scope-out and --milestone
(per guardrails:require-milestone).review:, Code review #,
Refactor:, arch:, compound:, follow-through:).gh issue create --body-file <path> — never --body "$VAR" — so
untrusted finding text (diffs, agent output) cannot shell-interpolate.Auto-wire deferred-scope-outs into the follow-through sweeper. When a
scope-out passes the CONCUR gate AND its Re-eval by: trigger is a concrete
date / dependency / event-grep / counter form, the filing ALSO wires the issue
into the follow-through auto-close substrate so it cannot rot open past its
trigger:
--label follow-through to the gh issue create call (alongside
--label deferred-scope-out);<slug>-<issue-or-pr>.sh under the
followthroughs directory by cp-ing the
stub template and
replacing the TODO body with the exit-code probe for the trigger shape
(mapping in review-todo-structure.md
§Re-evaluation Trigger), then chmod +x;<!-- soleur:followthrough script=… earliest=… [secrets=…] -->
directive in the issue body (earliest= = the trigger date for a date form;
the filing date for dependency/event-grep/counter forms, which self-gate via
the probe's transient exit). For any gh-using probe shape (dependency /
event-grep / counter) the directive MUST declare secrets=GH_TOKEN — the
sweeper's env -i sandbox strips all but PATH/HOME + declared secrets, so a
gh-probe without it is unauthenticated in CI and never closes (silent
never-close). Only the date shape needs no secrets=.Validation is NOT re-implemented here — the gh issue create --label follow-through call is intercepted by
.claude/hooks/follow-through-directive-gate.sh, which fails-closed if the
directive is missing/malformed, the script path escapes the followthroughs
root, the script is absent/non-executable, or earliest doesn't parse. Ordering:
scaffold + chmod +x the script BEFORE the gh issue create call (the gate and
the sweeper both require the file on disk; for review-time filings it lands in
the review PR's branch). Full contract:
followthrough-convention.md
§Trigger → verification mapping. This subsection is additive — the cost-of-filing
gate, the four scope-out criteria, and the CONCUR gate above are unchanged.
Everything else (magic numbers, duplicated helpers, small refactors, missing tests for PR-introduced code, polish, naming, a11y on PR-introduced surfaces, performance issues introduced by the PR) MUST be fixed inline.
Bundle scope-outs by trigger. Before filing, group candidates by trigger-equality (same date OR same counter threshold OR same #N dependency OR same human-review gate). File ONE issue per group with a sub-task checklist of the bundled items. CONCUR runs once per group, not per item. See plugins/soleur/skills/review/references/review-todo-structure.md §Bundling example.
The bundling check is operator-side because code-simplicity-reviewer only
sees one finding at a time and cannot recognize trigger-sharing across the
batch. Run the check on the synthesized candidate list before any CONCUR
invocation. If the operator misses a bundling opportunity and CONCUR is
invoked on items that obviously share a trigger, code-simplicity-reviewer
SHOULD DISSENT with DISSENT: bundle with #<sibling-finding> so the
operator collapses the filings.
Second-reviewer confirmation gate: Before creating a scope-out issue under
any criterion (including a bundled issue), invoke code-simplicity-reviewer
via Task. The prompt MUST include:
CONCUR (to co-sign the
filing) or DISSENT: <one-sentence reason> (to flip to fix-inline).
Everything after the first line is advisory context."Concrete re-evaluation triggers. Every scope-out filing's Re-eval by: field MUST take exactly one of four shapes: date / counter / event-grep / dependency (the last subsumes human-review gates via a reminder issue). The canonical definitions, examples, and rejected phrasings live in plugins/soleur/skills/review/references/review-todo-structure.md §Re-evaluation Trigger — code-simplicity-reviewer MUST DISSENT on any filing whose trigger does not match one of those four shapes.
If the first line of the agent's reply begins with DISSENT, the disposition
flips to fix-inline — do not file the issue. If the first line is CONCUR,
proceed with filing. Any other first-line content is treated as DISSENT
(fail-safe toward fix-inline).
Write-time self-check: Before invoking gh issue create --label deferred-scope-out, scroll up in the conversation and confirm the most
recent code-simplicity-reviewer Task reply begins with CONCUR for THIS
finding. If no such Task exists in this conversation, or the reply begins
with anything other than CONCUR, STOP — invoke the agent first. Filing
first and co-signing second is a protocol violation even when the agent
eventually returns CONCUR; the gate exists for the DISSENT case, and
filing-first leaves a publicly-visible issue that has to be closed if the
agent dissents. See learning
knowledge-base/project/learnings/best-practices/2026-05-05-extracted-bash-functions-need-self-contained-state.md
Pattern 3.
Rationale: One agent's "scope-out is fine here" can be wrong in the same
way a single test can miss a bug. Requiring a second, simplicity-biased agent
to co-sign blocks the most common regression pattern: an agent-author pair
rationalizing a filing that a fresh pair of eyes would reject. See
knowledge-base/project/learnings/2026-04-15-multi-agent-review-catches-bugs-tests-miss.md.
Filing without scope-out justification will be caught by /ship Phase 5.5 Review- Findings Exit Gate and BLOCK merge. See rule rf-review-finding-default-fix-inline. </critical_requirement>
<synthesis_tasks>
pr-introduced or pre-existing.
A finding is pr-introduced if the code the finding critiques was added
or modified by this PR's diff (verify with git log -L :<function>:<file> origin/main..HEAD or git diff origin/main...HEAD -- <file>). A finding
is pre-existing if the code existed on main before this PR and the
PR neither changed nor moved it. Provenance-ambiguous findings (e.g., a
helper the PR refactored but didn't introduce) default to
pr-introduced — the PR touched it, the PR owns the fix.Disposition by provenance:
pre-existing-unrelated criterion AND a concrete re-evaluation
trigger in one of the four forms (date / counter / event-grep /
dependency — see "Concrete re-evaluation triggers" below and
review-todo-structure.md). Vague phrasings ("post-MVP",
"later", "when ready", bare phase labels with no linked
phase-completion issue) are NOT permitted — they become the backlog
this rule exists to drain.The pr-introduced → fix inline rule is the mechanical version of rule
rf-review-finding-default-fix-inline: it removes the judgment loophole ("is
this really cross-cutting?") for findings the PR itself introduced.
</synthesis_tasks>
Coupling note: Ship Phase 1.5, Phase 5.5, and pre-merge hook pre-merge:review-evidence-gate detect review evidence by searching for GitHub issues with the code-review label whose body contains PR #<number>. If the issue body template or label changes, update detection logic in ship/SKILL.md and .claude/hooks/pre-merge-rebase.sh. Phase 5.5 Review-Findings Exit Gate (new in #2374) additionally detects open review-origin issues cross-referencing the PR by body regex (Ref|Closes|Fixes) #<N>\b without deferred-scope-out label; filing without scope-out justification will block merge.
<critical_instruction> Fix inline or, where a scope-out criterion applies, create a deferred-scope-out issue. Do NOT present findings for per-item user approval. </critical_instruction>
Read plugins/soleur/skills/review/references/review-todo-structure.md now for the complete GitHub issue creation flow: label prerequisite, issue body template, --body-file pattern, label/milestone selection, duplicate detection, error handling, and batch strategy.
Pipeline detection (run BEFORE writing the summary): Scan the conversation for skill: soleur:work or skill: soleur:one-shot output. If either is present, you are in pipeline mode — the calling orchestrator owns the lifecycle and is waiting on you to return so it can run step 5 / Phase 4. Emit the compact progress marker below instead of the verbose summary, then return immediately. Do NOT use the heading ## Code Review Complete, do NOT include a ### Next Steps section, and do NOT write a wrap-up sentence — those framings cause one-shot to mistake the summary for a turn boundary and stop mid-pipeline.
Pre-emission cost-of-filing pass (run BEFORE the marker): Build the candidate "Filed as scope-out" list from your synthesis. For each candidate, re-apply the cost-of-filing gate from §5:
code-simplicity-reviewer actually CONCUR on this specific item (not
just on the batch)? Required even in pipeline mode. → If no CONCUR, fix
inline.Only items that survive ALL three checks appear in "Filed as scope-out". This loop prevents the failure mode where pipeline mode rationalizes filing ≤100-line cleanup items because the marker template makes filing look like a first-class option. Target: the marker frequently shows "Filed as scope-out: 0". A PR that nets +N issues from review is a workflow failure.
Compact progress marker (pipeline mode):
## Review Phase Complete
- **Findings:** N total — N1 P1 / N2 P2 / N3 P3
- **Fixed inline:** N (commits: <sha>, <sha>, …)
- **Filed as scope-out:** N (#NNN, #NNN — criteria listed below)
- **Agents run:** <comma-separated list>
[Optional 1-line table of scope-out issues with criteria, if any.]
Self-audit: if the "Filed as scope-out" count exceeds 1 on a PR <500 lines, re-run the cost-of-filing pass above with a stricter posture before emitting. The target is fewer-issues-opened than issues-closed, measured across the team's PR throughput.
After emitting the marker, the calling skill's continuation gate takes over — control returns to one-shot step 5 / work Phase 4 in the SAME response.
Direct invocation summary (interactive mode only — no soleur:work or soleur:one-shot in conversation): Use the verbose summary template below.
## Code Review Complete
**Review Target:** PR #XXXX - [PR Title] **Branch:** [branch-name]
### Findings Summary
- **Total Findings:** [X]
- **P1 CRITICAL:** [count] - BLOCKS MERGE
- **P2 IMPORTANT:** [count] - Should Fix
- **P3 NICE-TO-HAVE:** [count] - Enhancements
- **By provenance:** [pr-introduced count] pr-introduced, [pre-existing count] pre-existing
- **Pre-existing disposition:** [fix-inline count] fixed, [scope-out count] scoped-out, [wontfix count] wontfix
### Fixed Inline
**P1 - Critical (BLOCKS MERGE):**
- {description} — commit {sha}
- {description} — commit {sha}
**P2 - Important:**
- {description} — commit {sha}
**P3 - Nice-to-Have:**
- {description} — commit {sha}
### Filed as Deferred Scope-Out
**Scope-out criterion required per finding (cross-cutting-refactor | contested-design | architectural-pivot | pre-existing-unrelated):**
- #NNN - review: {description} — criterion: {name} — rationale: {1-3 sentences}
- #NNN - review: {description} — criterion: {name} — rationale: {1-3 sentences}
**Failed (if any):**
- {description} - Error: {error message}
### Review Agents Used
- security-sentinel
- performance-oracle
- architecture-strategist
- agent-native-reviewer
- [other agents]
### Next Steps
1. **Verify inline fixes landed**: Each finding above should have a commit on the PR branch.
```bash
git log --oneline origin/main..HEAD | grep '^[a-f0-9]* review:'
```
2. **Inspect any scope-out issues**: Review findings filed as `deferred-scope-out` with justification.
```bash
# --state open is deliberate (#6786): this previews ship's Phase 5.5 gate, which
# blocks on OPEN review-origin issues only, so the states must match.
gh issue list --label deferred-scope-out --state open -L 200 --search "Ref #<PR_NUMBER>"
```
3. **Phase 5.5 gate self-check**: `/ship` will run the Review-Findings Exit Gate and block merge on any open review-origin issue cross-referencing the PR without the `deferred-scope-out` label. If the gate blocks, either fix inline and close the issue, or add the `deferred-scope-out` label + `## Scope-Out Justification`.
P1 (Critical - Blocks Merge):
P2 (Important - Should Fix):
P3 (Nice-to-Have):
Pipeline detection: If the conversation contains skill: soleur:work output earlier (indicating review was invoked by work's Phase 4 chain) or soleur:one-shot output (indicating review was invoked by one-shot step 4), skip the exit gate. The calling pipeline handles compound, commit, and lifecycle progression. When review is invoked by work or one-shot, do not duplicate these steps and do not output the verbose ## Code Review Complete block from Step 3 — the compact ## Review Phase Complete marker (Step 3, pipeline mode) is the only output and the orchestrator's continuation gate handles progression. The verbose summary's ### Next Steps block is the failure mode that causes orchestrators to mistake the report for a turn-ending deliverable.
If invoked directly by the user (no work or one-shot orchestrator in the conversation):
Run skill: soleur:compound to capture learnings from the review session.
If compound finds nothing to capture, it will skip gracefully — do not block on this.
Commit any local artifacts. GitHub issues are already created remotely,
but local files may have been modified (plan updates, todo resolutions).
Run git status --short. If there are changes:
git add <changed files>
git commit -m "docs: review artifacts for feat-<name>"
git push
If there are no local changes, skip the commit (this is the expected case — review's primary output is GitHub issues, which are remote-only). If push fails (no network), warn and continue.
Emit the review-evidence trailer (ALWAYS — not conditional on step 2), via emit-review-trailer.sh.
bash "${CLAUDE_PLUGIN_ROOT:-./plugins/soleur}/skills/review/scripts/emit-review-trailer.sh" --findings <n>
This is a script invocation rather than a described git commit line because
the described form has measured zero compliance on exactly the branches that
matter. Step 2 above tells you to skip the commit when there are no local
changes — which is the expected case — so a review that finds nothing
leaves no local evidence at all, and every downstream review-evidence gate
then reads "review never ran" and denies the merge with no escape hatch
(issue 6724).
The script therefore commits --allow-empty. It is idempotent (a second
review pass will not stack a duplicate), it refuses to run on main/master
or in detached HEAD, and it verifies the trailer actually parses before
reporting success — an unparseable trailer looks like evidence to a human
reading the log while being invisible to the gate that consumes it.
Run it even when step 2 committed something: the trailer is the durable machine-readable signal, and the commit subject is only a legacy fallback.
Display: "Review complete. All findings are tracked as GitHub issues.
Run /clear then /soleur:work or /soleur:ship for maximum context headroom."
Read plugins/soleur/skills/review/references/review-e2e-testing.md now for project type detection, testing offers (Web/iOS/Hybrid), and subagent procedures for browser and Xcode testing.
Multi-agent parallel review has been shown to catch bugs in shipped, green-CI code across these classes (each a real P1 caught on PR #2347):
Shared mutable state across co-mounted instances — module-level let bindings captured by a once-built object that multiple components import. Pattern-recognition and code-quality agents spot the closure capture in seconds; unit tests rarely co-mount instances.
Validator scope on sibling message fields — new top-level fields added to a schema whose existing validator covers only one field. Security-sentinel asks "what if the client sends X?" for every permutation without waiting for the test author to imagine it.
DB partial-index predicate drift — the application's query filter (.is("archived_at", null)) no longer matches the index's WHERE clause. Data-integrity-guardian reads both files and compares WHERE clauses symbolically; the bug stays silent until a user archives a row.
Feature-wiring composition bugs — module A is correct in isolation, module B is correct in isolation, but A+B together violate a constraint that lives in module C (downstream consumer, scheduler, taxonomy). Examples: leaderId: "system" reusing an internal taxonomy value whose UI semantics collide with router output; a registry.reap() method with no scheduler outside tests (tsc is silent on "never called in prod"); a nullable callback parameter the caller contract forbids but the implementer maps to a value that breaks invariants. Review prompts must enumerate the downstream consumer / scheduler / invariant explicitly for agents to reach it. See knowledge-base/project/learnings/best-practices/2026-04-24-multi-agent-review-catches-feature-wiring-bugs.md.
Runtime-content tamper between authoring and execution — when a workflow fetches content (issue comment, file at remote URL, external service response) at fire/run time and acts on it, the gap between fetch-time integrity and execution-time mutability is a single-user incident-class vector. "No inline prompts" prevents leak-via-committed-YAML; it does NOT prevent attacker-edits-the-source-between-create-and-fire. user-impact-reviewer's "name artifact + name vector" mandate reliably surfaces this where simplicity-biased peer review at plan time does not. PR #3067 added D5 (commenter-author-pin + immutability-pin) after the 11-agent review caught the gap that 3-reviewer plan-time review missed. See knowledge-base/project/learnings/2026-05-03-user-impact-reviewer-catches-runtime-content-tamper-vectors.md.
Cross-stream format-contract drift in telemetry joins — when a feature joins two telemetry streams (a producer and a consumer that look up by name), test fixtures that use a simplified shared format on both sides hide bugs where the producers actually emit different shapes (namespaced "plugin:name" vs bare "name", dotted IDs vs slashed IDs, hashed keys vs raw keys). Review agents and unit tests both miss this because each side's tests look internally consistent. The defect surfaces only via a derived-metric counter (orphan rate, miss rate, fall-through rate) whose surprising value points back at the contract. PR #3124 surfaced a soleur:plan (hook) vs plan (inventory) mismatch only after the orphan-skill counter — added as polish — reported a non-zero count in production data. See knowledge-base/project/learnings/2026-05-04-telemetry-join-format-mismatch-caught-by-orphan-counter.md. Reviewer takeaway: when a PR adds a join across two streams, ask whether at least one fixture per side uses each producer's actual emission format, not a normalized placeholder.
Handshake schema drift between producer (skill) and consumer (file) — when a skill instructs an operator to write a row/entry to a knowledge-base file, the producer's instruction template and the consumer's documented schema can drift in the same PR. Same column count + different semantics = silent table-corruption when followed verbatim. data-integrity-guardian catches this by reading both sides and comparing column-by-column. Reviewer takeaway: when a PR adds an instruction "write a row to file Y" alongside a schema documented in Y, grep Y's schema and assert the producer's row template matches column-by-column. Prefer reference-and-defer (instruction says "use the schema in Y") over embed-and-pray. PR #3501 shipped gdpr-gate with this exact drift; data-integrity-guardian flagged it as P1 pre-merge. See knowledge-base/project/learnings/2026-05-10-handshake-schema-drift-and-stale-precondition-budgets.md.
Replicated literals across ≥2 source files without parity test — canonical regexes, schema strings, taxonomy IDs replicated across SKILL.md prose, hook scripts, test files, and config globs drift independently. Three reviewers in PR #3501 independently flagged a path-regex stored in 4 places. Reviewer takeaway: when a PR adds the same literal across ≥2 source files, expect a parity test (expect(scriptContent.match(/^FOO='([^']+)'/)![1]).toBe(SOURCE_LITERAL)). If absent, file as P2 inline-fix. Same learning file as above.
Self-claimed cross-artifact contract drift — when a code/config file carries a comment, README line, or docstring claiming fidelity to another artifact (e.g., globals.css: "Token names mirror brand-guide.md exactly", schema.sql: "matches the TypeScript types in lib/types.ts"), edits to either side can silently break the contract. Pattern-recognition, architecture, and code-quality reviewers approve the diff in isolation because each reads only the LOCAL file. Only an agent that reads BOTH files surfaces the contradiction. Reviewer takeaway: when the PR touches a file containing a "mirrors X" / "matches X" / "kept in sync with X" / "tracks X" self-claim comment, include in the review prompt: "Read the named artifact X and verify the claim still holds post-diff." Cheapest gate: git diff origin/main...HEAD --name-only | xargs rg -l "(mirror|matches|kept in sync|tracks|reflects) (the )?(knowledge-base/|docs/|spec/)" — every hit demands cross-artifact verification. PR #3556 (font normalization) shipped with the dashboard typography diverging from brand-guide.md; only git-history-analyzer caught it pre-merge. PR #3596 (Anthropic DPA row) confirmed an implicit sub-pattern: the grep above returns zero hits (no self-claim comment exists), but compliance-posture.md's vendor-row framing still contradicted docs/legal/gdpr-policy.md's public disclosure for the same vendor — only security-sentinel caught it. Domain-specific gate: for any diff under knowledge-base/legal/, the review prompt MUST instruct an agent to read docs/legal/{gdpr,privacy}-policy.md for the vendor name(s) in the diff and verify the diff's framing of vendor role / data flow / transfer mechanism agrees with the public disclosure. See knowledge-base/project/learnings/2026-05-11-multi-agent-review-catches-cross-artifact-contract-drift.md.
Vendor-pipeline trust-contract gaps (auto-PR-of-untrusted-bytes / tautological integrity / exit-code-as-result) — when a PR establishes a new vendored-content pipeline (pinned upstream blob SHAs + scheduled drift workflow + integrity gate + severity classifier), four trust-model classes compose badly across the pipeline's boundary contracts: (1) auto-PR routing across security-relevant drift classes converts a detection signal into a write primitive (compromised upstream → bot-authored PR → review fatigue); (2) self-consistency integrity check (working-tree hash + frontmatter SHA both PR-author-mutable) is tautological — ask "what other thing must move to bypass this?"; (3) classifier that emits ONE exit-code result silently under-labels co-occurring categories (e.g., security + license drift in one upstream commit); (4) inline-Python/awk parsers with non-greedy or fragile tokenization can no-op silently when YAML formatting drifts. user-impact-reviewer names the adversary model; data-integrity-guardian runs the regex against the real input and produces the falsifying case. PR #3521 shipped all four; multi-agent review caught them pre-merge. Reviewer takeaway: for PRs establishing trust contracts, require (a) integrity check has at least one cross-domain anchor (CI-side gh api upstream verification, signed-commit, CODEOWNERS), (b) classifier emits multi-category stdout AND exit code, (c) auto-PR routing restricted to lowest-risk class only, (d) post-condition assertions on regex/awk substitutions (subn count == expected). See knowledge-base/project/learnings/2026-05-11-multi-agent-review-vendor-pipeline-trust-model.md.
Single-literal gate over a multi-member union/enum — when a TypeScript predicate gates behavior on X === <literal> (or !isFoo, status === "completed") and X is a union/enum with ≥ 3 members, the gate is correct only by coincidence unless every union member has been classified include/exclude in the originating FR. user-impact-reviewer and pattern-recognition-specialist reliably catch this only when the review-spawn prompt explicitly enumerates the union members — without the prompt, agents echo the plan's single-value framing as a false-pass. Reviewer takeaway: the review-spawn prompt MUST enumerate the union members literally — without that, agents echo the plan's single-value framing as a false-pass. Concretely: when reviewing a gate conditioned on X === <literal> where X is a TypeScript union/enum, grep the type's declaration (rg "type X =" <module> or grep -nE "X = .*\|") and pass the resulting member list verbatim into the spawn prompt, then ask "is the gate correct for each value?" Single-literal gates against multi-member unions are a known defect class. Why: PR #3653 — plan §FR2 conditioned on !isStreamingAssistant; /work bound the gate to streamState === "streaming" while StreamState = "idle" | "streaming" | "stopping" (ws-client.ts:47). "stopping" is a distinct in-flight substate that mid-aborts traverse; a Stop click could have flashed the marker during that window. Caught only because the spawn prompt explicitly named the 3-value enum. See knowledge-base/project/learnings/2026-05-12-plan-precondition-and-3-value-enum-gate-drift.md.
Plan-time empirical-probe assumptions vs. actual caller surfaces — when an ADR captures a plan-time probe that validated a discriminator field (e.g., "the hook event carries authentication_method='otp' ONLY on the runtime path"), the probe ran against ONE caller. The assumption may not hold for other callers in the codebase that hit the same upstream API differently (e.g., user-facing dashboard auth that ALSO triggers authentication_method='otp' via a different SDK shape). security-sentinel should grep the codebase for non-canonical caller patterns whenever the diff includes a SECURITY DEFINER function or auth-issuance hook that gates on a probe-validated field. Reviewer takeaway: when reviewing a PR that adds an auth-event-gated function, the spawn prompt MUST instruct security-sentinel to enumerate every caller-side surface that hits the same upstream API and confirm the gate's assumption holds for each. Why: PR #3983 — ADR-033 §0.4 pre-committed authentication_method='otp' as the runtime/dashboard discriminator (probe-validated for runtime); user-facing dashboard uses signInWithOtp which produces the same authentication_method='otp' → every dashboard JWT was being hook-rewritten with aud=soleur-runtime, exp=600s (10-min auto-logout). Marker-table pivot landed as migrations 049/050. See knowledge-base/project/learnings/2026-05-18-supabase-custom-access-token-hook-discriminator.md.
Parser-consumer invariant seam bypass — multi-layer pipelines (awk emits per-token → bash read loop assigns last-wins; JSON-parser emits per-array-element → consumer overwrites by key; regex-extract → Map.set last-write-wins) where the parser-side enforced invariant (first-wins, deduplicated, unique-by-key) is silently violated at the consumer boundary. Plan-time review of the parser fix in isolation misses this because the bypass lives in the seam BETWEEN layers. Multi-agent review reliably catches it when the spawn prompt explicitly instructs "trace the data flow from raw input through every transformation layer and assert the claimed invariant holds at every consumer boundary". Reviewer takeaway: when a PR's plan claims a parser-side invariant (e.g., "first directive wins," "deduplicated by key"), enumerate every consumer layer the parsed output crosses and require at least one test that injects N>1 matching tokens of the SAME key per record. PR #4200 — security-sentinel surfaced multi-script= last-wins WITHIN a single directive after the plan's Gap-2 fix closed multi-DIRECTIVE first-wins; the awk for-NF-loop emits one line per matching token and the bash case "$key" in script) script=$val was still last-wins. See knowledge-base/project/learnings/2026-05-20-parser-emits-per-token-bash-read-loop-last-wins-within-directive.md.
Legal-disclosure prose hallucinated against the actual migration body — when a docs-only PR discloses a database substrate landed by a prior PR (legal docs, privacy policy, vendor DPAs, transparency reports), the disclosure prose is typically authored from the plan's conceptual narrative rather than from the migration body; the writer hallucinates plausible-sounding column names, RPC signatures, trigger bypass mechanisms, and DSAR allowlist semantics. The plan-time loop and per-AC grep gates do NOT catch this because none cross-grep the prose against the implementing files. Reviewer takeaway: when the diff touches docs/legal/, plugins/soleur/docs/pages/legal/, or knowledge-base/legal/ AND cites an implementing PR/migration, the spawn prompt for security-sentinel AND code-quality-analyst MUST instruct: "Cross-check every implementation-detail claim in the new prose (column names, RPC signatures, trigger bypass mechanism, cascade step numbers, DSAR allowlist entry, ON DELETE behavior) against the migration body, the RPC body, and the consuming TypeScript file; produce a column-by-column drift table." Why: PR #4353 — two independent agents (security-sentinel + code-quality-analyst) caught 4+ fabricated identifiers (organization_id, user_id vs actual removed_user_id, removed_user_email_hash, removal_reason, SET LOCAL session_replication_role) that the plan's deepen-pass + 11 AC grep gates all missed. See knowledge-base/project/learnings/2026-05-23-legal-disclosure-prose-must-be-grep-validated-against-actual-migration.md.
Stale plan-time RLS-policy enumeration drift — when a PR sweeps RLS policies across "all" tenant tables based on a plan-time grep, the table list decays as sibling PRs land between plan-write and PR-merge. Multi-agent review reliably catches this when the spawn prompt for data-integrity-guardian AND security-sentinel instructs: "Re-derive the canonical authenticated-policied table list at review time via grep -rnE 'POLICY.*ON public\.[a-z_]+ .*TO authenticated' apps/web-platform/supabase/migrations/*.sql and assert every match has the new RESTRICTIVE policy." PR #4418 — both agents independently caught 2 missed tables (organizations, workspace_member_removals) the plan's enumerated "19 tables" list missed; verify sentinel widened to per-table intersection. See knowledge-base/project/learnings/2026-05-25-multi-agent-review-catches-stale-precedent-grep-and-unreachable-ux-toast.md.
RLS-policy-expression edit breaks exact-string verify/ sentinels AND aborts on dev/prod policy divergence — when a migration edits an RLS policy's DEPARSED expression (an auth_rls_initplan wrap auth.uid() → (select auth.uid()), a predicate rewrite, a role/qual edit), it has two blast radii beyond the policy itself that pass tsc + the vitest suite + migration-shape lints and only fail POST-MERGE (verify-migrations against prod, tenant-integration against dev). The spawn prompt for data-integrity-guardian AND security-sentinel MUST instruct: (a) git grep -l "<policyname>" apps/web-platform/supabase/verify/ for every touched policy name and update each stale exact-string sentinel (ILIKE '%...auth.uid()%') in the SAME PR — or make it wrap-tolerant (~* 'user_id = \(? *(select +)?auth\.uid\(\)', preserving the anti-false-green prefix), verified against live prod bad=0; and (b) require each ALTER POLICY to be guarded by a pg_policies existence check (DO $do$ BEGIN IF EXISTS (SELECT 1 FROM pg_policies WHERE schemaname='public' AND tablename=... AND policyname=...) THEN ALTER POLICY ...; END IF; END $do$;) because ALTER POLICY has no IF EXISTS and a policy name sourced from ONE project's live catalog is not guaranteed present on the other (hr-dev-prd-distinct-supabase-projects) — a single absent policy aborts the whole migration. Why: PR #6663 — an initplan wrap (migration 134) broke verify/129's exact string-match (hotfix #6671) and aborted the dev apply on conversations_owner_delete (absent on dev). See knowledge-base/project/learnings/2026-07-18-rls-initplan-wrap-breaks-verify-sentinels-and-dev-prod-policy-divergence.md.
Closed privacy-field-list classified at column-NAME instead of column-VALUE-SHAPE — when a PR introduces a closed denylist of "fields to null on rows belonging to a third party" (Art. 15(4) author redaction, DSAR allowlist, log scrub field set, response-redaction filter), plan-time review reliably approves the list at column-name level ("tier is an enum, looks structural") without cracking open the migration COMMENT body to read the actual value shape. The asymmetry is dangerous: one too many preserved column = single-user privacy leak (brand-survival); one too many redacted column = a structural-shell row the subject can still see. Two ORTHOGONAL post-implementation agents (one reading migration COMMENTs for namespace patterns, one reading COLUMN TYPES + ROPA prose) reliably catch this where plan-time review misses it. Reviewer takeaway: when reviewing a PR that defines a closed field-list over a database table, the spawn prompt for security-sentinel AND data-integrity-guardian MUST instruct: "For each column NOT in the redact list, read the migration ADD COLUMN line AND its COMMENT body. Classify as (a) free-text → REDACT, (b) namespace-identifier shape <prefix>-<org>:<value> or email-shaped → REDACT, (c) signal-about-third-party (even closed enum like tier='external_brand_critical') → REDACT, (d) UUID/integer/timestamp/known-bounded numeric → preserve. Produce a column-by-column classification table." Also require a CI sentinel test that parses migrations for ALTER TABLE <table> ADD COLUMN and asserts every observed column is classified in REDACT or ALLOWLIST. Why: PR #4351 — security-sentinel + data-integrity-guardian independently flagged that source_ref / owning_domain / urgency / leader_id / template_id / tier / source / trust_tier (8 columns the plan classified as "structural preserve") carry free-text business semantics that leak third-party content; user-impact-reviewer silently approved the original 5-field list at column-name level. Sentinel test added at apps/web-platform/test/dsar-message-redact-fields-sweep.test.ts. See knowledge-base/project/learnings/2026-05-25-closed-field-list-must-classify-at-value-shape-not-column-name.md.
Temporal-qualifier gap on sequenced legal-then-code rollouts — when PR-1 of a multi-PR sequence lands disclosure prose for behavior that PR-N+1 will implement (the "land legal before code" Art. 13(1)(e) prior-disclosure pattern), present-tense disclosure claims ("transient: true MANDATORY", "data egresses to vendor X") misrepresent the current code state. Plan-time review optimizes for "is the disclosure accurate post-PR-N+1?" (yes) and misses "is the disclosure accurate at PR-1 merge?" (no). Multi-agent review at PR-1 time reliably catches this only when the spawn prompt for legal-compliance-auditor AND security-sentinel instructs: "For each forward-looking claim in the new disclosure (verbs like MANDATORY, MUST pass, always, every call, egresses), identify the PR that lands the code-side enforcement, then grep the current codebase to confirm the claim is or is not live. Each claim must be either backed by current code OR qualified with a temporal marker ('effective on PR-N merge', 'will pass', 'once PR-N merges')." Article 13(3) prior-disclosure is the legal precedent. Why: PR #4455 (umbrella #4456 PR-1) — Flagsmith sub-processor disclosure landed asserting transient: true MANDATORY + orgId egress as present-state facts; actual code at apps/web-platform/lib/feature-flags/server.ts:86 called getIdentityFlags(\role:${role}`, { role })(PR-2 lands those). 3 agents independently surfaced; fixed with Art. 13(3) qualifiers + explicit "Current code-side state at PR-1 merge" subsection. Seeknowledge-base/project/learnings/2026-05-25-pr1-of-sequenced-legal-disclosures-needs-temporal-qualifiers.md`.