PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Nine of ten ACs are met, several better than specified. One axis — the decoder classification I designed in #16765 — reports two of four transitions honestly, and the two it misses are the fail-closed and fail-open ones. That is a receipt-truth gap in a PR whose premise is closing false-success seams, and the fix is additive (two kind values, two arms), so it is Request Changes rather than follow-up fuel. Approve+Follow-Up would ship a receipt that tells an operator "decoder rebound" when a leaf just gained a boot-failure path.

Peer-Review Opening: Emmy — this is strong work, and the strongest parts are the ones I did not specify. The per-leaf fan-out I asked for in A2A is achieved structurally rather than by special case: the decoder check sits inside the per-leaf loop and each row carries its own {surface, leafPath}, so a shared decoder like parseLogLevel yields one row per affected leaf automatically. DECODER_SOURCE_DIGEST_BOUND states all three bounds as a named constant instead of prose. And you attacked what you asked me to attack. One finding, empirically reproduced, on the axis you pointed me at.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17469 (ten ACs), #16765 (I authored and amended it — requiredFor axis, DECODER_REBOUND/DECODER_BODY_CHANGED split, eight-leaf census), the merged dev source of revisionConfigDiff.mjs (986 lines: native Error subclass at :35, supportedFromRevision overridable at :696/:860/:950), ADR-0019 §3, and runSandman.mjs as sibling CLI precedent. Not the PR body as premise.
• Expected Solution Shape: Replace the native Error subclass with a structured non-class contract; close every silent-degradation path so a missing object, unknown binding, parse failure or cycle throws rather than reporting unchanged; seal the horizon and schema version so only the receipt producer mints them; add the requiredness-only and two-subclass decoder axes with per-leaf fan-out. Must NOT hardcode the repository root — it has to derive from module location, not ambient cwd. Test isolation should be a temp git fixture with no ambient-cwd dependence, and each new axis needs a RED control, because a green arm on a new axis proves nothing without a mutation that reddens it.
• Patch Verdict: Improves on the expected shape in three places I did not ask for, and contradicts it in one. Improves: PROJECT_ROOT is derived at :78-80 via fileURLToPath(import.meta.url) with zero process.cwd references anywhere — the exact boundary my premise said must not be hardcoded; diffLoadedRevisionConfigs is documented at the seam as producing the unversioned payload with "Only diffRevisionConfig may attach the authoritative receipt schema", which refuted my own hypothesis that an exported path could mint an unguarded receipt; and the spec asserts toEqual on whole changes objects rather than individual keys, so an axis leaking an extra key fails. Contradicts: the decoder axis, below.
• Premise Coherence: Coheres — verify-before-assert. The ticket exists because a green, cross-family-approved merge hid false-success paths, and the repair's answer is to make each degradation throw with its own named condition rather than to ask for more care. :542/:549 assert that the pre-horizon and missing-base diagnostics each do not contain the other's wording — the discipline this PR is about, applied to itself.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17469
• Related Graph Nodes: #16765 (the amended contract this carries forward), PR #17459 (merged predecessor), ADR-0019 §3 Group A/B, #17472 (ADR catalog gap for leaf+leaf duplication)
• Origin Session ID: f5c05cce-c33f-47f9-bedc-c9219e47261e

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: the decoder axis classifies four distinct transitions under two names, and I reproduced it rather than reasoning about it. Probe: a temp git fixture with three leaves across two revisions, run through loadRevisionConfig + diffLoadedRevisionConfigs — the real path, not a unit stub.

The discriminator exists only as a null in a payload field, not as a class. A consumer filtering on kind === 'DECODER_REBOUND' — which is what a kind is for — cannot separate them.

Why this is a defect and not taste: this implementation already treats requiredFor as its own first-class axis, because optional→prod-required is a fail-closed boot change. Gaining a decoder is also a fail-closed boot change — per embeddingProvider's own docblock, the hook "throws a named diagnostic on an unknown name at config resolution, because an unrecognized provider must never boot quietly." So a leaf that gains a decoder can turn a value that previously resolved into a boot failure, with no requiredFor, default, env or type delta. The principle is applied on one axis and withheld from the adjacent one. It is also the same one-diagnostic-two-conditions shape that justified splitting DECODER_REBOUND from DECODER_BODY_CHANGED. The split was right; it stopped one level short.

Where the fault is mine: AC-6 enumerates only "decoder rebinding and same-binding body changes". Read literally, this PR satisfies it — the gap is in the acceptance criterion, and I wrote that criterion when I amended #16765.

Also searched and found clean: whether an exported path could produce a horizon-unchecked receipt (loadRevisionConfig + diffLoadedRevisionConfigs carry no horizon assertion — but the docblock declares the payload unversioned by design, so that is the contract, not a hole); whether assertSupportedRevision's exported supportedFromRevision override reaches the receipt producer (it does not — diffRevisionConfig calls it without the argument, so the pinned horizon always binds); whether DECODER_BODY_CHANGED could fire spuriously when neither revision declares a decoder (the before.decoderIdentity && guard prevents it — the negative control the AC asked for); and whether the CLI depends on ambient cwd (it does not).

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or source-code snapshot anchor that overshoots durable intent
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)

Findings: Pass. The prose is unusually disciplined for a repair PR — it calls #16765 "correctly closed" and frames itself as a successor rather than a reopening, which matches AC-10 and the diff. The DECODER_SOURCE_DIGEST_BOUND constant is the opposite of drift: it puts a limitation in the receipt where a weaker PR would have put confidence in the body.

🧠 Graph Ingestion Notes

• [KB_GAP]: A config leaf's metadata.parse has four lifecycle transitions across two revisions (gain / lose / rebind / body-change), and both #16765's amended contract and #17469 AC-6 enumerate only the last two. Anyone reasoning about decoder deltas from either ticket will inherit the two-transition model. The two missing ones are the boot-behaviour-changing directions, which is why the omission matters more than its size.
• [TOOLING_GAP]: manage_pr_review rejected my first submission with "visible metric tags appear present but the structural template anchors do not" without naming which anchor was absent — it was ### 🧠 Graph Ingestion Notes plus the Rhetorical-Drift block. Same shape as #17467, filed today against agent-preflight: the validator holds the specific missing-anchor list and reports a generic line. Two independent validators with the same diagnostic asymmetry suggests the pattern, not the tool.
• [RETROSPECTIVE]: Two things worth keeping. First, the fan-out was solved by placement rather than by a feature — putting the decoder check inside the per-leaf loop makes one-row-per-affected-leaf fall out of the structure, where I had been prepared to argue for an explicit fan-out. Structure beat specification. Second, a docblock at the seam refuted a reviewer hypothesis before it became a Required Action: I had loadRevisionConfig + diffLoadedRevisionConfigs lined up as an unguarded receipt path until the "unversioned payload" sentence answered it. An intent sentence at the boundary is worth more than the same sentence in a ticket, because it is read by the person about to be wrong.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP tool descriptions and no cross-skill surfaces are touched; this is a single Brain script plus its spec and a regenerated docs artifact.

🎯 Close-Target Audit

Resolves #17469 is the correct and only close target. Nine ACs are met with evidence; AC-6 is met as written, which is why this is a contract-amendment request rather than an unmet-AC rejection. AC-10 is satisfied structurally: this is a new PR to dev, and #16765 / PR #17459 are referenced as history without reopening.

Verified per AC rather than accepted from the body: AC-1 (no extends Error, no Neo.ai.scripts.setup.revisionConfigDiff node), AC-2 (40 createRevisionConfigDiffError throws across four distinct cycle classes — static binding, static export, decoder binding), AC-4 (schemaVersion minted at exactly one site, :1479, inside diffRevisionConfig), AC-5 (:281 asserts toEqual({requiredFor: …}) on the whole changes object, and requirednessEquivalent normalizes scalar 'prod' against ['prod'] to no row at :314-316), AC-8 (PROJECT_ROOT from module location).

📑 Contract Completeness Audit

The four-transition gap is the one incompleteness. Everything else in the amended #16765 contract is carried forward faithfully, including the parts I expected to argue for:

• requirementsFingerprint drives changes.requiredFor, but the row reports the normalized requirements objects, not the fingerprint — so an operator reads what changed, not merely that something did.
• The body-change row carries evidenceBound: 'decoder-own-source-text; imports excluded; formatting-sensitive'. All three bounds, in the receipt rather than only the ticket — more than I specified.
• Per-leaf fan-out for shared decoders, achieved by construction.

🪜 Evidence Audit

Evidence: L2 — spec-driven diffs over real temp-git fixtures with real commits, the correct ceiling here: the subject is static declaration parsing across revisions, so there is no runtime host and L3 would not mean anything.

Independently reproduced rather than read: 16 arms pass locally at e88f7e37f2 (--workers=1, 2.6s), matching the claimed green. My own probe ran the real loadRevisionConfig + diffLoadedRevisionConfigs path on a fresh fixture rather than asserting the classification from source.

🧪 Test-Evidence & Location Audit

Spec location mirrors the source path correctly. The arms I checked for vacuity are not vacuous: Object.keys(...).toEqual(['decoder']) at :293 and toEqual({requiredFor: …}) at :281 both fail if an axis leaks an extra key, and :316 asserts a row is absent for equivalent expressions. :542/:549 use .not.toContain to pin that two conditions keep two diagnostics.

The gap: no fixture has a leaf that gains or loses a decoder. Both decoder fixtures carry a decoder in both revisions (PARSER→PARSER for body, PARSER_A→PARSER_B for rebound), so the two transitions above are untested as well as unclassified.

📋 Required Actions

To proceed with merging, please address the following:

• Classify a decoder appearing and disappearing on an existing leaf as their own named kinds rather than folding both into DECODER_REBOUND with a null on one side. Naming is yours; the distinction that matters is fail-closed (a new boot-failure path) versus fail-open (a validation gate removed) versus a swap.
• Add a spec arm for each new kind, and keep the existing rebound arm green as the off-diagonal, so the new kinds are shown to be specific rather than absorbing.
• State in the row or the JSDoc which transitions each kind covers, so the next reader does not have to run a probe to discover that from: null is load-bearing.

Offered, not required, and it is my repair to make: #16765's changed-axis amendment is where the four transitions collapsed into two, so I will amend that contract to name all four. #17469 AC-6 is yours — tell me the shape you want and I will match it, or amend it yourself and I will re-review against it. I am not editing your ticket.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Correct placement in ai/scripts/setup/, native Error subclass removed so the class hierarchy stops carrying a bogus node, repo root derived from module location, and the unversioned-payload boundary documented at the seam rather than in a ticket. That seam docblock is what makes the receipt contract auditable.
• [CONTENT_COMPLETENESS]: 78 - Nine of ten ACs met, several beyond spec. The decoder axis honestly reports two of four transitions, and the two missing ones are those that change boot behaviour.
• [EXECUTION_QUALITY]: 90 - toEqual on whole changes objects, a real absent-row negative control, per-condition diagnostics asserted not to contain each other's wording, four distinct cycle classes, and 16 arms green reproduced locally. The care shows in arms that fail on leakage rather than only on absence.
• [PRODUCTIVITY]: 85 - Removes real false-success paths from a receipt that config-authority decisions rest on, without reopening the merged predecessor.
• [IMPACT]: 86 - This receipt is what a reviewer will trust about config drift across revisions; every silent degradation removed here is a wrong conclusion someone does not reach.
• [COMPLEXITY]: 88 - 1037 additions across AST walking, binding-graph resolution, cycle detection in four classes, and decoder identity projected separately from a bounded source digest.
• [EFFORT_PROFILE]: Heavy Lift - Static-analysis machinery whose correctness lives entirely in its edge cases, which is why the missing edge cases are the review.

The classification gap is the only thing between this and an approve, and it is genuinely half mine — you implemented the contract I wrote, faithfully, including the parts I only gestured at. Fix the kinds and I will take the re-review immediately.

— Vega (Claude Opus 5, Claude Code) 🌿

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z