Claude review: traps file, blind read, structured verdict, callers rule, Opus full tier (#3710)

[erikdarlingdata]

The review was LGTM-shaped by construction, and this gives it the three things it was missing

Across several hundred PRs the Claude review posted LGTM on very nearly all of them, and every defect it missed that later surfaced — the sweep-id precision loss (#3487), the twelve file-growth cards from a per-collection fire on an hourly collector (#3636, the shape #3628 had just fixed one alert over), MAX(max_dop) grouped across plans (#3648), ORDER BY 3 + 4 DESC folding to a constant (#3613), the Slack fifty-block breach (#3612), integer division in the Full-edition rate columns (#3653 Q9) — was found by behaviour: a post-install read, a production page, an EXPLAIN on a rig. Never by the review. #3710 names the three handicaps that made LGTM the honest output rather than the lazy one: no memory of this repository's recurring shapes, the diff read beside the author's essay (consistency, not correctness), and a verdict that could be one word. This PR is slice one, plus the model and tier ruling from the issue's comment.

Mechanism. Four changes, all in the prompt and the verdict check, none in the tool allowlist.

1. .github/REVIEW-TRAPS.md — fifty-odd lines across seven layers (SQL, collectors and deltas, analysis, alerting and notifications, storage and MCP payloads, tests and censuses, workflows and CI, repository mechanics), each one line, each naming the PR or issue that bit. Every citation was read (gh issue view / gh pr view) before it was written: >= limit truncation (MCP pages now say what bounded them: caps bind to the caller's limit, truncation is detected not inferred, and no page count is called a total (#3541 A3) #3594 → get_pg_server_config counted the page and called it the server: non_default_count is now the snapshot's, computed above the cap, truncated is observed off a limit + 1 fetch, and its three sibling pages stop inferring truncation from a full page (#3653 A3 residue) #3679 → Three PostgreSQL pages observe truncation off limit + 1 through one shared helper, the census sweeps the inference class instead of a roster, and four payload rules become facts and inventories, so three tools stop guessing truncation from a full page and four of the eleven payload rules stop being sentences (partial #3653) #3699, three tools plus the Take(limit) cousin, bound through McpHelpers.BoundPage); the constant sort key (MCP percents name their denominator: shares are of the window or say they are of the page, so a three-row page stops summing to 100% of everything (#3541 A7) #3613, DarlingPgKernelStatsReader); the per-plan cumulative max_dop (Top Cpu Queries' Max Dop is a cross-plan cumulative maximum with no provenance: it said 16 on a MAXDOP-1 instance whose stored plan is serial, and drove a wrong tuning recommendation #3648 → Top Cpu Queries' Max Dop is the newest plan's reading with the cross-plan maximum kept as a dated history, so a MAXDOP-1 instance stops reporting 16 from a plan compiled before the pin (#3648) #3651); cntr_value_per_second integer division (The frozen Dashboard mirrors five of today's honesty fixes in its own idiom: page counts stop posing as totals, one slow wait stops paging, a mute that matched nothing says so, an unbanded collector dot is Unknown, and the per-second counter stops truncating (#3653 Dashboard mirrors) #3658 for the Dashboard, install/02/06/47 approved under Brains-review campaign: deferred structural residue (from #3538 / #3539 / #3540 / #3541) #3653 Q9 and not yet landed); per-collection re-fire (Forced Plan Failing fires once per observation, not once per cooldown: the adapter returns the collection that produced the rise and the engine remembers which one it already reported (#3579) #3628, Database File Growth's rise arm re-fires every cooldown against the same hourly observation: up to twelve cards per growth event — #3579's mechanism, one condition over #3636) and one-sample fire/resolve (tempdb Space fired on one collected sample and resolved on the next: the arm now holds for 3 consecutive collections behind the shared persistence gate, counted per collection rather than per sweep, with no-data freezing it instead of resolving to N/A (#3653 A5, tempdb slice) #3692); restart zeros in deltas (The four naked delta families store the interval their deltas accrued over, so a restart's fabricated zero reads as unknowable instead of 0.00 ms/sec (#3540, V127 / Lite v60) #3595), baselines (The wait and perfmon baselines stop counting every restart's fabricated zero as a quiet sample: interval-honest successor aggregates replace the legacy pair in place, the provider reads whichever supply covers its window and gates the LAG heuristic on a NULL interval, the legacy pair retires by a self-executing coverage condition, the startup backfill reads coverage off the materialization, and the Daily-routed calendar answers its unmaterialized days from raw (partial #3653: A6 + A10 mechanical half) #3698), identity epochs (Five families subtracted from the old instance once before anyone noticed it had changed; Aurora's wait counters had no one watching for a restart at all: wait_stats carries the SQL Server identity first in the order, pg_wait_stats carries pg_postmaster_start_time(), and both census rosters are empty (partial #3653) #3705) and the gauge-as-counter reset (A falling gauge read as a counter reset because the store did not know it was a gauge: perfmon_stats stores each counter's cntr_type (Darling V132 / Lite v62), the collector writes a gauge as its level with no delta, and every reader classifies by the stored type with the /sec name proxy as the pre-rung fallback (partial #3653 A7) #3708); FactScorer.ScoreAll skipping amplifiers at BaseSeverity <= 0 (FactScorer.cs:100, still that line), the non-virtual GetActiveEdges seam and the PgTargetFactKeys censuses (The PostgreSQL analysis vocabulary absorbs what six content lanes reported back: a stable PG_BAD_ACTOR alias the graph resolves to the window's heaviest statement, the two vacuum-side config facts that were inert because no arm ever gave them a base, PostgreSQL 16's reserved_connections in the connection ceiling, and the bad actor's first next-read (#3542, between waves) #3688); pg_total_relation_size vs the heap, timescaledb_information.hypertables hiding materializations, chunk_compression_stats returning nothing with compression off (get_store_metrics' inventory was blind to 57% of the store it reported on: it now enumerates the continuous aggregates and the named plain tables, reconciles every sweep against pg_database_size and states its coverage, and the owner's job_history count rides the sweep so the managed-mode block is a measurement too (#3582, #3574) #3610), job_history's ownership filter (get_store_metrics: the job_history block said 'recording' and never said whose eyes the rows are visible to — it now evaluates the view's ownership filter for its own reader and proves rows exist where it can (#3574) #3585); the 2.26.4 crash-backoff reading of -infinity and the re-arm that resets it (The compression dead-job alert says 'the scheduler will never run it again' — true below TimescaleDB 2.26.4, false above it, where the row is a crash backoff the scheduler clears itself and the re-arm measurably delays it #3629); the Slack block budget (Analysis pages with long compound stories die on Slack with invalid_attachments: #3493's size discipline covers the prose, not the details that carry the finding #3612 → Slack analysis pages stay inside the 50-block message limit: details are budgeted like the prose, dropped from the end with the omission stated (#3612) #3618), two-across fields (Top CPU Queries renders as a two-column field jumble on Slack — one section block per query, not seven interleaved fields #3644) and the surrogate-pair cut where System.Text.Json substitutes U+FFFD rather than rejecting (Slack text cuts land on whole characters, not UTF-16 indexes: an emoji astride a limit no longer reaches the reader as a replacement glyph, and a cut field's omitted count is in characters the reader can count (#3622) #3625); LCK_M_SCH_M folding into LCK so no SCH_M fact exists and every SCH_M arm is dormant (Maintenance-family anomalies fold onto the fired job's incident and name the job, so a server's own index-maintenance job no longer pages four times without saying its name (Fixes #3704) #3709), one ANOMALY_WAIT_PROFILE per run (Maintenance-family anomalies fold onto the fired job's incident and name the job, so a server's own index-maintenance job no longer pages four times without saying its name (Fixes #3704) #3709), PopulateStoryText running before the fold (Maintenance-family anomalies fold onto the fired job's incident and name the job, so a server's own index-maintenance job no longer pages four times without saying its name (Fixes #3704) #3709); the dorny v4 independent-predicate rule ([THROWAWAY - do not merge] Validate the #1712 docs fast path #1714, Darling PostgreSQL tests skips shared-library changes its two sibling jobs catch, reports green, and blames documentation #3116); set -e killing a step on a transient 503 ([CI] Review guard hard-fails on an unguarded lookup 404 — a lookup failure becoming the verdict its own comments forbid #2309); the CRLF round-trip. The two-week git log added the rest (The daily digest and sweep rollup asked 'did I send' when the channel asks 'was it delivered': the once-a-day gate now stamps delivered-today in the store, so a restart re-announces nothing and a failed delivery still retries (#3580) #3633, IAlertDeliverer.DeliverAndReportAsync is required, not defaulted: CONTRIBUTING names this seam, so Lite's deliverer and every fake state their answer by hand (#3580 follow-up) #3640, Zero is a measurement: health parsers say whether their source was ever seen, a regression with no baseline stays null, and the first point of a differenced trend is no longer a fabricated 0 (#3541 A12) #3642, The delta-seeder tests measured the CI scheduler, not the seeder: two clock reads bracketing the DuckDB phase gave the interval a ±2 window that failed at 123 and 243 on unrelated PRs. One whole-second anchor owns both ends now, on both hosts, and the assertions are exact — not wider. #3676, Darling's get_pvs_stats trend drew an unmeasured pass as 0 MB: the point is now null with pvs_measured beside it, the same two keys Lite's twin has carried since #3666, and a measured zero is still a zero (#3653) #3682, The MCP trend reader's tier ladder and hourly SQL were pinned EQUAL to the Storage definitions #3666 moved them to, which fails only after two copies drift — they are now aliases; and the Query Store duration chart, the one Performance Trends chart left untitled, says what served it and where the rollup's floor cut the window's head (#3653) #3684, The analysis pass said "collection appears to have stopped or broken … NOT an all-clear" for a window the coverage witness proves the collector observed: the empty-window rule tested facts.Count == 0 before it tested the witness, so an observed window that simply produced no fact wore the dead-collector envelope — unavailable with a pointer at collection health, the Viewer's dead-collector marker. The rule now gates on the witness alone, and an observed window with no facts runs the pass and reads empty at its stated coverage, both SKUs (#3653) #3687, A quiet hour halved the next hour's Query Store rate, and three trend readers recomputed an interval the store had stored all along: rollup points are rated over their own bucket width, the query-stats trends read sample_interval_seconds three-state on both SKUs, and Compose delta aggregates FILTER out the restart marker (partial #3653: A8, A11's LAG clause, the Cumulative archetype) #3695, Fifteen rate arms spelled "unknowable" as 0 behind a guard that happened to hide it: every guarded delta / interval now ends at END, the PostgreSQL trend readers carry the NULL rather than reading it back as 0, and both census rosters are empty (partial #3653) #3707, Continuous-aggregate materializations are chunked at one raw chunk, like every raw table, so retention drops what it was designed to and compression reaches the newest day instead of stopping ten days short (#3620) #3621, The File Growth threshold means one thing — megabytes per hour — at every surface that shows it, so a 10 GB bar stops meaning 10 GB per five minutes on one store and per day on another (#3539 A8c) #3631, Story confidence measures corroboration instead of path length, and the nightly rebuild's three cards become one incident that names the job (#3538 A6/A9) #3632, A server nothing has banded yet is Unknown, not Healthy, and the alert-history grids show the severity the alert actually fired at instead of the colour its name implies (#3539 A6/A8e) #3635/The four PostgreSQL host alerts fire with the tier they earned instead of Severity: null, so their history rows stop wearing the colour their name implies (#3653 A8e, PostgreSQL half) #3680, Every latest-snapshot MCP read says when it was captured, and no tool accepts a window it does not read (#3541 A10) #3637, The duration-trend trio routes by retention tier and discloses its source like get_query_trend already does, so a 7-day request no longer returns 4 days labelled quiet (#3541 A2) #3590, The daily summary stops painting purged months green, and every MCP filter is part of the query: parallel_only/min_dop/blocking_only cut before the page, a negative window is refused, an unknown source names the accepted set (#3541 A9/A13) #3641, The T-SQL convention guard reads ForcePlanBotPolicy.Names and .Evidence short and dev is red on it: both are block-bodied now, whole range, no KnownTruncatedRanges entry (#3654 follow-up to #3663) #3667). The bar stated at the top of the file: bitten at least once in a merged PR or filed issue, and a diff reader who knew the shape would have caught it.

2. A blind read. The prompt runs in two phases inside one job. Phase 1 forbids gh pr view, git log and git show <commit> — the body and the commit messages are the author's account — and allows gh pr diff, Read, Grep, Glob, git diff. The reviewer reads every changed region in its surrounding context, applies the callers rule (for every changed public signature, constant, SQL statement or behaviour, Grep for its callers and readers in both SKUs and both test projects, judge each still-holds-or-not; a caller not re-read is UNPINNED), reads the traps file and decides per nearby trap whether it applies here with the file:line that proves it, and writes its own account. Phase 2 runs gh pr view, extracts every factual claim the body makes, grades each VERIFIED (how) / UNVERIFIED (why not) / REFUTED (evidence), and reports divergence between the two accounts in either direction. The ordering is enforced by instruction and by the prompt withholding the body: it interpolates the repo, the PR number, the tier and the model — never the title or body, which it never did carry. The blind read costs nothing the action was not already permitted to do; it is an ordering of the gh verbs that were already in the allowlist.

3. A structured verdict. The review body is a fixed block — ## Verdict: LGTM | CHANGES REQUESTED, ### Claims (VERIFIED / UNVERIFIED / REFUTED bullets), ### Riskiest lines (three, each pinned by <test> or UNPINNED), ### Traps near this diff (applies / does not apply, with why), ### Divergence (blind read vs PR body), ### Reviewer checklist — ending in one machine-readable line, <!-- review-ledger: verdict=… verified=N unverified=N refuted=N traps=N tier=full|light model=<id> -->, so a later pass can compute the miss rate against post-merge defects instead of an impression. It is posted through gh pr review --body-file - with a quoted heredoc: the same gh pr review prefix the allowlist already matches, and no Write tool is needed to stage a body file. The Claude review runs finish green having posted nothing: the allowlist forbids every read tool the prompt invites, denial churn ends sessions without a verdict, and the job never checks that it posted #3650 verdict check now enforces the shape. After counting the window's verdicts it takes the newest (last line of the stream — the API is oldest-first and --paginate runs --jq per page, the same fact the guard's scan relies on), fetches that review by id, strips CRs, and fails the job when the body lacks the header, lacks the ledger, or carries a ledger verdict that disagrees with the review's API state. That third arm is the one that protects the gate: the guard reads the state, so a body saying "CHANGES REQUESTED" inside a review submitted with --comment is a substantive finding the guard would wave through. A ledger tier that disagrees with the tier step's decision is a warning, not a failure — it corrupts the ledger, not the verdict. The section headings are logged and not graded: a mark that fails on a heading's wording is one people learn to ignore.

4. Model and tier. A dorny/paths-filter@v4 step (the repo's pin style — build.yml uses @v4, nothing in this repo is SHA-pinned, so neither is this; the brief said @v3, the repo says @v4) classifies the PR: the full set is PerformanceMonitor.Analysis|Collectors|Common/**, Darling/PerformanceMonitor.Darling.Storage|Analysis|Service/** (the issue's Compose and Mcp are under Service), Lite/Services|Mcp|Database/**, install/**, **/*.sql, every directory pattern in the dir/**/!(*.md) extglob shape build.yml and darling-paths-filter.yml already explain (dorny evaluates each pattern as an independent predicate under some; a bare !**/*.md is its own rule and made every filter true on PR [THROWAWAY - do not merge] Validate the #1712 docs fast path #1714). A README under Analysis/ is a light-tier change on purpose. One decision step reads the filter's outcome and output and emits tier, model, max_turns; one review step consumes them — two review steps would double the "review posted" accounting the guard does on the step named Claude review. Full: --model "claude-opus-5[1m]" --max-turns 60 (the [1m] selector is the 1M context window; claude-code-action's docs/configuration.md names Opus 5 and Sonnet 5 as carrying it, verified for this PR). Light: --model "claude-sonnet-5" --max-turns 30 — the Sonnet family's current id in Claude Code's own catalog (latest_per_family.sonnet, read from the installed 2.1.271), spelled out so the ledger names it. A filter that dies (continue-on-error, outcome read by the decision step) degrades to full with a warning, and the review step's || 'claude-opus-5[1m]' / || '60' fallbacks take the same direction if an output is ever empty: under-reviewing a Storage change is the expensive way to be wrong, and --model "" is not a review.

Why this shape and not another. The traps live in a file, not the prompt, so a lane that learns a new shape edits one line without touching the workflow (which cannot be Claude-reviewed and disables review repo-wide until the next release — see the drift arm). The verdict shape check lives in the review job, not the guard, because the review job has the run's window and the guard's verdict arm is authoritative about state; the two stay compatible by construction — the step name Claude review and the ALWAYS POST sentinel the guard keys on are byte-identical, and the guard file is untouched. The ledger's grammar is fixed (token order included) because a machine reads it; the prompt gives the exact line and the check rejects a reordered one — deliberately strict, and named as a risk below. The tier is a boolean from the filter, never a file name, into the prompt: a PR author controls file names, and an interpolated name in a prompt is the pull_request script-injection footgun build.yml describes for scripts.

What this does NOT do. It does not change the tool allowlist: no Edit, Write, commit or push verb; the read-only three git verbs and the four gh verbs as before. It does not touch claude-review-guard.yml. It does not give the review a database (slice two: the PG tests' TimescaleDB container, after the ledger shows the SQL class is what remains). It does not route PerformanceMonitor.Alerting/** or PerformanceMonitor.Notifications/** to the full tier — they are not in the brief's list, and five of the traps (#3612, #3618, #3625, #3628, #3636, #3644) live there; flagged for the coordinator, a one-line change to the filter if wanted. It does not add the PR body to the prompt (do not).

Verification. actionlint 1.7.12 clean on the workflow (with shellcheck on both run: blocks); yaml.safe_load parses it and the prompt still carries ALWAYS POST. The verdict shape check's grep/sed/case block was run verbatim in a harness against nine bodies: structured LGTM on COMMENTED → pass; structured CHANGES REQUESTED on CHANGES_REQUESTED → pass; a CHANGES REQUESTED body submitted as --comment → fail, state disagreement named; bare LGTM — checked the diff → fail, header and ledger both named; header without ledger → fail; a CRLF body → pass; ledger tier=light when the step decided full → pass with the tier warning; the Sonnet ledger at light → pass; ledger tokens reordered → fail. Every trap citation was verified against the issue or PR body before it was written; FactScorer.cs:100, RelationshipGraph.GetActiveEdges virtual, IsGeneralLockWait, the three census test names and the InternalsVisibleTo list were re-read in the tree at this head. Both scrub regexes from FleetIdentifierScrubTests have no hits in either new file; LockedModeRestoreCoverageTests and ProductVersionDeclarationTests sweep .github/workflows/*.yml and find no restore line and no version read here. The action cannot run locally, and this PR's own review run is dark: claude-code-action refuses when this file differs from main's copy, so the "Claude review guard" red on this PR is the known drift signature. The first live run of the new prompt is the first PR after the release sync; the scratchpad item to verify it (and to re-add the guard to dev's required checks) exists.

Riskiest lines. (1) --model "${{ steps.tier.outputs.model || 'claude-opus-5[1m]' }}" — the || fallback on an empty output; checked by expression semantics (an empty string is falsy) and by the tier step being the only writer. (2) The ledger regex in the verify step — fixed token order, model=[^[:space:]]+ accepting the [1m] selector; nine-case harness above. (3) The tier decision's elif [ "${FULL:-}" = "true" ] — dorny emits the string true/false, matched exactly the way build.yml's gate steps match it.

Reviewer checklist. Read the prompt block once as the reviewer would: is anything in Phase 1 only reachable through a forbidden verb? Confirm the guard's two couplings survive (grep -n "name: Claude review\|ALWAYS POST" .github/workflows/claude-review.yml). Decide the Alerting/Notifications tier question. After the release sync, watch the first live run: the ledger line on its verdict, the tier notice in the job log, and whether the shape check passes on the model's first attempt at the template.

Closes #3710