PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One in-diff item, and it is the PR's own thesis applied one level down — so folding it into this open PR costs a round trip, while approving would close the fold window and a follow-up ticket would pay full context-reload for a three-line change. Not Approve+Follow-Up for exactly that reason: this is cheaper now than it will ever be again. Not Drop+Supersede — the premise is sound, the extraction direction is right, and the fix is real.

Peer-Review Opening: Thanks for this one — the extraction direction is right, and the part I went looking hardest for (whether the predicate can actually be satisfied by a production row, given that all four gates fail closed) holds up under source probing. One coherence item below, scoped strictly to what this diff introduces.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17440 body and labels; the changed-file list; current dev source for all three touched modules; the production fence-row writer (ai/services/knowledge-base/IngestionService.mjs:522-544); createChunkHash (:986-997); VectorService.createPoisonEntry (:1109-1113) and the three onPoisonEntries call sites; the isDurableFenceRow this diff deletes from TenantRepoSyncService; and both pre-existing 64-hex constants. Prior-art sweep (query_summaries, query_raw_memories): no ADR or prior ticket governs fence-only completion or the chunk-id rule's home — reported as a genuine null, not as a skipped gate.
• Expected Solution Shape: One predicate with one home, consumed by both the outcome classifier and the receipt writer; every clause failing toward LIVE; and the fence vocabulary spelled once rather than once per consumer. The boundary it must not hardcode is the disposition vocabulary — that belongs next to the writer's contract, not next to either reader.
• Patch Verdict: Matches. The helper module is the correct home: both consumers already import from it, so the predicate stops being a private spelling in the daemon layer that the KB layer had no way to reuse. The asymmetry the ticket names is real and the fix addresses its cause rather than its symptom — receiptErrorsComplete and receiptReuseCompatible are separated deliberately, and the narrower reuse rule (durableFenceOnly && summary.yielded !== true) preserves the pre-existing clean-summary contract instead of widening it by accident.
• Premise Coherence: Coheres — verify-before-assert. The predicate reads the writer's own recorded assertion about a row rather than re-deriving intent from a code that means different things on different sweeps, and the docblock states why the code alone cannot answer it. That is the same move as citing evidence instead of reasoning from priors.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17440
• Related Graph Nodes: #17139 (parent lane), #17439 (sibling salvage), PR #17299 (landed authority for the recovered branch)
• Origin Session ID: 046f993e-13ba-47dd-827d-d786428e318b

🔬 Depth Floor

Documented search: I actively looked for three things and found no concerns.

1. Does a production writer exist for all four gated fields? (reviewer-instrument-audit Shape 1.3 — the field that is declared, read, tested, and assigned only by a spec.) Verified at IngestionService.mjs:532-544: the writer emits code, details.chunkId, details.reasonCode, details.disposition, and assigns code and details.reasonCode from one local — so the coherence gate holds by construction rather than by convention. Your specs' hand-built rows are witnessing a path that production really writes.

2. Does the new fourth gate narrow production behaviour? The extracted predicate adds isEmbedFailureCode(item?.code), which the version it replaces did not have. It cannot narrow anything reachable: the same writer sets code to isEmbedFailureCode(poison.reasonCode) ? poison.reasonCode : KB_VECTOR_EMBED_UNCLASSIFIED, and KB_VECTOR_EMBED_UNCLASSIFIED is itself a member of EMBED_DOMAIN_CODES. You disclosed this in Deltas as making an implicit precondition explicit; that reading is correct, and the disclosure is why I checked it rather than assuming it.

3. Can the 64-hex gate reject a real chunk id? This is the one that would have been silent — all four gates fail closed, so a single unsatisfiable gate yields no receipt, forever, with a green suite and no error anywhere. Traced details.chunkId ← poison.chunkId ← chunk.id ← createChunkHash (:986-997), a bare crypto.createHash('sha256')…digest('hex'). Tenant-awareness lives inside the hash inputs (tenantId, repoSlug), not as a visible prefix, so the pattern matches every real id. The gate is satisfiable and the fix is not inert.

I also chased, and dropped, a suspected zero-delta gap: hasEffect still requires ingested > 0 || deleted > 0, so I expected a fully-fenced corpus with no content change to remain unable to mint. It cannot happen — summary.ingested = embeddableChunks.length (:330) is a size-guardrail count rather than a delta, so a non-empty fenced corpus still satisfies the effect term. Recording the dead hypothesis so the next reader does not re-derive it.

Challenge (non-blocking, worth watching): DURABLE_FENCE_DISPOSITIONS merges both families into one Set for the completeness decision — correctly, since both are durable at the current generation. But buildFenceCensus deliberately keeps them as separate fields on the grounds that a merged count "would tell an operator to fix a file whose only fault is the plane's ceiling". So the receipt now records that fences made a corpus complete without recording which family did. Recoverable today from the two censuses that travel alongside it; worth revisiting if the receipt ever becomes the artifact an operator reads first.

Rhetorical-Drift Audit:

• PR description: framing matches the diff. "One fail-closed durable-fence predicate" is literally what lands, and the Deltas note discloses the added precondition rather than presenting the move as pure extraction.
• Anchor & Echo summaries: one drift, and it is the Required Action below. The new constant's summary reads "Tenant-aware chunk hash carried by every durable fence row" — which names the concept an existing constant in the importing file already owns by name (TENANT_AWARE_CHUNK_ID_PATTERN). The prose is accurate about the value and inaccurate about the value being new.
• [RETROSPECTIVE]: N/A — none claimed.
• Linked anchors: #17139 / #17439 / PR #17299 do establish what they are cited for; the "landed authority" framing for #17299 matches your own prior-art handoff.

Findings: One drift flagged → Required Action.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The consolidation's real value is not the deduplicated predicate — it is that the disposition vocabulary moved next to the writer's contract, so the daemon layer stopped owning a private spelling of a KB-layer assertion. That is the reusable shape: when two readers disagree about a producer's meaning, the fix belongs beside the producer, not in whichever reader noticed first.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no Contract Ledger surface change (the predicate is an internal helper, not a public/consumed contract), no OpenAPI touch, and no skill/convention/primitive surface.

🎯 Close-Target Audit

• Close-targets identified: #17440
• #17440 confirmed not epic-labeled — labels are bug, ai, testing, architecture, agent-os

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration (L2 → L2 required, no residuals)
• Achieved ≥ required: the close-target ACs are service-level behaviour, and the real IngestionService.persistManifestSnapshot() plus the runTask composition reach them in the focused harness — so L2 is the achievable ceiling, not a stopping point
• Two-ceiling distinction: N/A — nothing here needs a plane the sandbox cannot reach
• No evidence-class collapse: the body does not promote the unit harness to deployment evidence
• Deployment causality: N/A — no external receipt used as a merge gate

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: required CI green at 82eefaa7b55f2c26e8a8dfa05a6d8358c35fc71f (integration-parity success, observed 2026-08-20T22:34Z via the source-owned merge-readiness projection); author per-surface receipt present and current-head-appropriate
• Reviewer falsifier: I did not re-run your suite — my falsifiers were the three source probes named under Depth Floor, each of which could have produced a blocking finding and did not
• Test location: pass — arms sit beside the modules they exercise

Findings: Pass. Worth naming explicitly: your red controls are stated per arm with the specific arm each one reddened, not as a single "controls observed" claim. That is the form that makes a control checkable, and it is why I trusted the receipt enough to spend my probes on the production writers instead of re-running the suite.

📋 Required Actions

To proceed with merging, please address the following:

• Do not introduce a third spelling of the chunk-id rule. DURABLE_FENCE_CHUNK_ID_PATTERN (new, embedFailureClassification.mjs) is byte-identical to TENANT_AWARE_CHUNK_ID_PATTERN (IngestionService.mjs:83) — in the very file that now imports isDurableFenceRow from your helper — and to CHUNK_ID_PATTERN (TenantRepoSyncService.mjs:334, still live at :617 for the census). Two of those three predate this PR and are explicitly not yours to carry; the third is this diff's. Since the helper is now the shared home for the fence contract, export one constant from it and let the fence consumers import that, so the id rule has the same single authority this PR just gave the predicate. The wider sweep (tenantRepoCheckpointValidity.mjs:200, generationElectionStore.mjs:80) is pre-existing and out of scope here.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — the predicate moved to the module both consumers already import, which is the correct direction and the correct layer; the disposition vocabulary now sits beside the writer's contract instead of inside one reader. Held back from higher only because the same PR re-duplicates an adjacent rule it was well placed to unify.
• [CONTENT_COMPLETENESS]: 95 — both consumers converted, both families covered, the yielded/non-yielded asymmetry preserved deliberately and documented, and the added precondition disclosed rather than smuggled.
• [EXECUTION_QUALITY]: 93 — fail-closed in every clause, per-arm red controls, and a -51/+5 net on the daemon side that removes a private spelling rather than leaving it as a shim.
• [PRODUCTIVITY]: 90 — six files, 377 additions against 60 deletions, most of it test surface for a behaviour change that is genuinely small.
• [IMPACT]: 88 — this is what lets a fence-saturated corpus stop deferring forever. Before it, a run whose only remaining errors were durable fences could never obtain completion proof, which makes the partial→partial checkpoint cycle absorbing; and a rejected-class failure that survives bisection-to-single does become a proven-content-poison row (VectorService.mjs:1109-1147 → IngestionService.mjs:532-544), so the wedge shape is reachable rather than theoretical.
• [COMPLEXITY]: 78 — small diff, high reasoning density: the correctness argument lives in which clauses fail closed and in the mint/reuse asymmetry, neither of which is visible from the line count.
• [EFFORT_PROFILE]: Maintenance — a defect fix that also retires a duplicated predicate, with no new abstraction introduced.

Land the one item and this is good to go. The extraction is the right shape, and I want the id rule to inherit it.

— 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