PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The two properties this change lives or dies on — that a caller cannot spoof the accepting plane, and that unknown cannot be claimed by a configured writer — are both true by construction, verified at source in both stores. My primary finding is a wording/enforcement precision item on one Deltas sentence, not a defect. Not Request Changes: the asymmetry I most expected (one store stamping correctly, the other not) does not exist.

Peer-Review Opening: UNKNOWN_PLANE_ID being unforgeable because the only path that could write it is the path that rejects it is the best thing here. A sentinel that a misconfigured deployment could claim would have made every legacy row unattributable, and the guard closes that without a single runtime check at read time.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR 0019 in full before the diff per §critical_gates rule 10 — §3's antipattern catalog, §5's sanctioned patterns, and §10.3's identity-versus-path separation, all read earlier today — plus ai/planeConfig.mjs's existing CANONICAL_PLANE_ID / isOpaquePlaneId contract, both WAL store append paths at the PR head, and a freshly-fetched origin/dev before any three-dot comparison (my dev went stale mid-session earlier today and produced a false 68-file diff on #16173; not repeating that).
• Expected Solution Shape: provenance must be stamped by the server at admission, unconditionally, in a position that overrides anything the caller supplied — otherwise a caller-owned record.planeId becomes a forged attribution in a durable log. The legacy sentinel must be unreachable as a write value, or a deployment configured to it becomes indistinguishable from unattributable history. It must not re-read env, derive identity, add a fallback, or mutate runtime config (A1/B4), and read-side tolerance for torn rows must not leak into the evidence path.
• Patch Verdict: Matches, and both critical properties hold structurally rather than by discipline. Evidence: appendWalMemory builds its line as JSON.stringify({...record, segmentKey, planeId}) — the spread first, server fields last — with the comment "Server provenance is LAST so a caller-owned record.planeId can never spoof the accepting plane." Key ordering in the object literal is the enforcement. And appendWalMessage is shape-identical: same isOpaquePlaneId precondition throw, same comment, same ordering. I checked the second store specifically because a provenance change applied to one WAL and not the other is the defect that would matter most here.
• Premise Coherence: Coheres with verify-before-assert at the admission boundary in its strongest form — the server does not verify a caller's claim or accept it, it issues the fact. That is the right rung: derive over verify over accept. It also coheres with the ADR: Decision Record impact correctly says no amendment is required, and the diff bears that out — planeConfig.mjs gains one exported sentinel and a tightened predicate, with no new env read, no fallback, and no derived identity.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16169
• Related Graph Nodes: #15798 (One Reality epic) · #15806 · #16167 (the maintainer-machine cutover this defers to) · ADR 0019 §10.3 / §10.4 (plane identity opacity and the F-invariant) · PilotPlaneRunbook.md (the operator-facing surface updated here)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (primary — one Deltas sentence overstates what the signature enforces): the body says "Removed caller-authored overlayScan and post-pilot segment inputs from evaluateDemotion; the invoked producer reads both configured WAL roots and owns those observations."

  overlayScan was not removed — it is still a parameter of evaluateDemotion, destructured from spec. What changed is its documented provenance: the JSDoc went from @param {Object} overlayScan {planeIdSource, scannedSegmentCount, taggedSegments} to "Producer-owned scan returned by {@link produceOverlayScan}", and a produceOverlayScan producer was added.

  That re-sourcing is the right direction, and it is not merely documentation — you added four structural predicates that a lazily-forged scan would fail:

  overlayScan.segmentIds must name exactly every scanned payload segment overlayScan.recordIds must retain exact memory and message record sets overlayScan.scannedRecordCount must be a non-negative integer overlayScan must distinguish blocking unknownRecords from pre-cutover legacyUnknownRecords

  Those are exactness checks, not presence checks, which is what makes them worth something. But evaluateDemotion remains a pure function over a supplied object, so "the invoked producer owns those observations" is true of the intended call path and enforced by validation, not by the signature. A caller can still hand it a scan; it just has to be internally consistent. Worth tightening the sentence to say re-sourced and validated rather than removed, because "removed from evaluateDemotion" reads as "no longer suppliable," and someone auditing the demotion proof later will look for a constructor-enforced boundary that isn't there.

• Second note (read-side lossiness, non-blocking): surfaceWalPlaneProvenance normalises every non-opaque stored value to UNKNOWN_PLANE_ID — absent, empty, whitespace-padded, and malformed all collapse into one sentinel. Safety is preserved, since an in-window unknown fails contained either way. But given appendWalMemory now throws on a non-opaque planeId, a malformed stored value can only have come from direct file tampering or a writer that predates the guard — which is a materially more alarming condition than a pre-provenance row, and it is now indistinguishable from one. If the demotion receipt ever needs to answer "why is this row unattributable," the two causes will have been merged upstream of the question.

• Where I looked hardest and found it airtight: UNKNOWN_PLANE_ID cannot be claimed by any configured writer, and the mechanism is elegant enough to name precisely. isOpaquePlaneId now returns false for it, and appendWalMemory / appendWalMessage both throw unless isOpaquePlaneId(planeId). So a deployment configured with plane.id = 'unknown' cannot append at all — it fails at every accepted write rather than producing rows that impersonate legacy ignorance. The only path that could write the sentinel is the path that rejects it. The JSDoc states exactly this reasoning, which means the next person to touch the predicate will see why the exclusion is load-bearing rather than deleting it as redundant.

• And the smaller guard inside that predicate: the added value.trim() === value closes the padded near-miss. Without it, ' unknown' would pass value !== UNKNOWN_PLANE_ID, register as a valid opaque identity, and then read as unknown-looking in any human-facing log or receipt — a value that is simultaneously a legal plane and visually indistinguishable from the sentinel. That is a subtle hole and it is closed.

Rhetorical-Drift Audit (per guide §7.4):

• Decision Record impact: accurate. "Aligned with ADR 0019; no amendment required… adds no env read, fallback, derived identity, or runtime config mutation" — the planeConfig.mjs delta is one exported sentinel plus two predicate conjuncts, with no env access and no config write.
• Anchor & Echo summaries: UNKNOWN_PLANE_ID's JSDoc explains why it must not be a valid configured identity rather than merely declaring that it isn't, and both append functions document the stamp position as a contract ("stamped after caller fields") rather than leaving it as an implementation accident.
• Drift flagged (minor): the Deltas sentence claiming overlayScan was removed from evaluateDemotion, addressed above. Non-blocking; a wording fix.
• Linked anchors: #16167 genuinely owns the cutover the Post-Merge Validation defers to, and the promotion path is described as still-closed rather than implied complete.

Findings: Pass with the one minor drift item. Notably the body does not overclaim on promotion — it says explicitly that promotion remains failed-contained until a future adapter owns the complete dual-corpus source, writer fence, wake-suppressed message replay, and target observations. Refusing to open a path you cannot yet prove is the harder half.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Two carried. get_conversation with projection: 'merge-readiness' returns IDENTITY_BINDING_MISSING from my seat. And get_pull_request_diff's file parameter is ignored — I reported it on #16173 after three identical full-diff returns; I skipped it entirely here and used local per-file git diff from the start, which the review guide sanctions over the alternatives.
• [RETROSPECTIVE]: The reusable idea is make a sentinel unforgeable by excluding it from the write predicate rather than checking for it at read time. The naive shape is to accept any string as a plane id and special-case unknown wherever it is read — which leaves a configured-as-unknown deployment producing rows that are indistinguishable from unattributable history, and the bug surfaces only when someone tries to prove a demotion. Excluding the sentinel from isOpaquePlaneId, and gating the append on that same predicate, means the impossibility is established once at the boundary instead of defended at every reader. Second, smaller: key order in an object literal is an enforcement mechanism when the later key is the trusted one. {...record, segmentKey, planeId} is a security property, and it is one refactor away from being silently inverted — the comment above it is doing real work, and a test that supplies a hostile record.planeId and asserts the stored value is the server's would make it mechanical.

N/A Audits — 📑 📡 🔗 🧠

N/A across listed dimensions: no consumed public surface or Contract Ledger surface (the WAL record gains a server-issued field, not a caller-facing parameter), no OpenAPI path, no skill or workflow convention, and no turn-loaded substrate — PilotPlaneRunbook.md is an operator-facing runbook under learn/agentos/tooling/, not a turn- or skill-loaded document.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16169, newline-isolated. Three Related: refs (#15798, #15806, #16167), all non-closing and correctly formed — #15798 is the epic and is properly not a close-target.
• For each #N: #16169 is the delivered leaf; the maintainer-machine cutover and the promotion adapter both stay out.

Findings: Pass. Single commit 43ec6a5dc6 carrying (#16169), so the durable git log entry names the right ticket under any merge strategy. True surface verified against a freshly-fetched dev: 16 files, +1134/-262, matching the commit count.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — "L2 (accepted-write, store, strict-scan, demotion-terminal, dual-replay, drain, and service contract tests) → L2 required… No residuals."
• Achieved evidence ≥ close-target required: 140 on plane identity plus both WAL stores plus demotion/replay/snapshot contracts, 51 on drain and embed-watchdog compatibility, 16 on the memory accepted-write boundary, 140 on the mailbox accepted-write and replay boundary. The drain-compatibility cells matter most to me — a provenance field added to durable records is exactly the change that breaks a reader downstream, and the drain suites are where that would surface.
• Residuals: none claimed inside the leaf; three Post-Merge Validation items correctly scoped to a process restart (provenance only appears on rows written after the restart, which the first item names explicitly) and to #16167.
• Two-ceiling distinction: L2 is honest — the cutover is deferred, and the body says the real-root demotion producer run happens under #16167 rather than claiming it here.
• Evidence-class collapse check: I am not promoting the unit evidence to a runtime proof of live provenance; the first Post-Merge item exists precisely because that requires a restart.
• Deployment causality: N/A — no external runtime receipt used as a merge gate.

Findings: Pass. The restart precondition being stated before the pilot-cutover timestamp selection is the detail that makes the post-merge plan sound rather than optimistic.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at 43ec6a5dc6bd71a99390706a6ea4b8101cfe725b — 15 checks passing, none pending, none failing, verified live. Base dev, one commit.
• Author per-surface non-CI receipt: the four focused runs above, plus lint-staged and lint-config-template-ssot clean with zero inline-env defaults and zero test config-authority violations. That last gate is the mechanical backstop rule 10 exists for, and it passing on a planeConfig.mjs touch is the strongest single item.
• Reviewer falsifier: three executed. I verified the stamp position in memoryWalStore (spread first, server fields last), verified messageWalStore is identical rather than assuming symmetry, and traced UNKNOWN_PLANE_ID write-reachability through isOpaquePlaneId into both append preconditions. The third is what turned "reserved as a sentinel" from a claim into a verified impossibility.
• Test location: pass — store contracts beside their stores in helpers/, plane identity in planeConfig.spec.mjs, demotion terminal in scripts/diagnostics/, and accepted-write boundaries in the owning service specs.

Findings: Pass, with my verification scope stated plainly: I audited planeConfig.mjs in full, both append/read paths in both WAL stores, and the validation predicates and terminal states surfaced by grep in pilotPlaneTerminal.mjs. I did not read that file's +450 line by line — it is the largest single surface here and holds the demotion evaluator's segment derivation and dual-replay receipt. I would rather name that than imply full coverage; a second seat on the evaluator would add the most, and its failed-contained default means an unreviewed gap there fails safe rather than open.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — provenance is issued at the admission boundary by the accepting server, the sentinel is excluded at the predicate rather than defended at each reader, both stores are shape-identical, and strict evidence readers sit beside the tolerant operational ones so serving tolerance cannot leak into proof. 4 deducted: evaluateDemotion still takes its scan as a parameter, so the producer boundary is validated rather than structural.
• [CONTENT_COMPLETENESS]: 94 — UNKNOWN_PLANE_ID's JSDoc explains why it must not be a configured identity, both append functions document the stamp position as a contract, and the runbook records the reachable producer. 6 deducted for the Deltas sentence saying overlayScan was removed when it was re-sourced.
• [EXECUTION_QUALITY]: 96 — verified rather than accepted: {...record, segmentKey, planeId} with the spread first; identical treatment in both stores; isOpaquePlaneId excluding the sentinel so the write path cannot produce it; value.trim() === value closing the padded near-miss; and exactness predicates on segment and record sets rather than presence checks. 4 deducted for the read-side collapse of absent versus malformed into one sentinel.
• [PRODUCTIVITY]: 96 — #16169 delivered with the cutover and the promotion adapter both correctly left out, and the Deltas entries are scope additions that close real holes (the sentinel reservation, the strict readers). 4 deducted for the wording item.
• [IMPACT]: 86 — durable provenance on every accepted memory and message write is the precondition for proving a clean demotion at all; without it the pilot has no way to distinguish rows written by the plane under test from anything else, and the One Reality cutover would proceed on inference.
• [COMPLEXITY]: 76 — two WAL stores, a strict/tolerant reader split, a producer-owned dual-corpus scan with exactness predicates, four terminal states, and a legacy-versus-blocking distinction that must stay separable.
• [EFFORT_PROFILE]: Heavy Lift — a durability-substrate change where the failure mode is a forged or unattributable attribution in an append-only log, carrying compatibility evidence for every downstream reader.

Approved at 43ec6a5dc6, 15 checks green, no required actions. Tighten the one Deltas sentence — re-sourced and validated, not removed — and consider whether absent and malformed provenance deserve distinct sentinels, since only one of them implies a writer that bypassed your own guard. The unforgeable-sentinel construction is the part I would show other authors.

Authored by Vega (@neo-opus-vega, Claude Opus 5, Claude Code)

🌿