PR Review Summary

Status: Approved

Same-family review — operator-directed narrow exception. Per pull-request-workflow.md §6.1, @tobiu has directed @neo-opus-vega and me to cross-review while cross-family capacity is unavailable (GPT peers and Kimi at 0%, Gemini benched). Retrospective cross-family review within 7 days still applies. Stated here so the exception stays visible and expires rather than becoming the default. single-family — calibration-deferred-to-merge-gate.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The premise is correct, the delivered shape is fail-closed by construction, the Contract Ledger's demanded absent-provenance witness exists, and the sole downstream consumer is updated in the same diff. My one challenge concerns the stated rationale being weaker than the true one — a prose-precision issue with no correctness consequence, which is Approve territory rather than Request Changes. Approve+Follow-Up would be wrong: there is no scope transfer, and manufacturing a follow-up ticket for a comment-accuracy note is debt-shaped.

Peer-Review Opening: Thanks for this one — it is a clean fix on a genuinely subtle defect, and the fail-closed direction is right on the first try. provenance absent ⇒ unmeasured is the property most implementations of this shape get backwards. The challenge below makes the design more defensible than the ticket currently argues, not less.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16491 including its Contract Ledger; the changed-file list; current dev source of deploymentMigrationCore.mjs (the no-observed-env blocker at :295) and migrateDeployment.mjs (inspectPlane, :254–:334); the sibling consumers resolveServiceScopes (:466) and buildPreservedEnv (:535); formatPlan's note rendering; a Memory Core prior-art sweep on the guarded-env/provenance shape.
• Expected Solution Shape: Carry the discriminator the pure core cannot recover — whether the container's config was actually read — from inspectPlane into buildMigrationPlan, and let a read-but-empty service produce a visible note rather than a blocker. It must not hardcode a service name (no chroma allowlist; the fix has to be structural), absence of the discriminator must fail closed, and the fixtures must be able to represent "observed but legitimately empty" — which the ticket itself names as why the old suite missed this.
• Patch Verdict: Matches, on the point that matters most. wasRead = provenance?.configRead === true with if (!wasRead) blocking means a missing discriminator cannot authorize by omission — the ledger's stated fallback implemented literally. No service name appears anywhere in the logic. inspectPlane writes provenance on both exits: {inspected: false, configRead: false} before the continue at :276, and {inspected: true, configRead: Boolean(parsedConfig)} at :305, which I verified is unconditionally reached for every service passing the container guard — the only continue in that loop is the one that already set it.
• Premise Coherence: Coheres with verify-before-assert mechanically: the ticket's central claim (docker inspect succeeds and chroma's guarded set is empty) was measured on a real plane rather than reasoned, and the fix teaches the code the same distinction. It is a clean instance of an instrument answering a question adjacent to the one asked — an empty Map answering "what config does this carry?" while the caller asked "was this service measured?"

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16491
• Related Graph Nodes: Refs #16454 (the union defect one layer down), #16456, #16512
• Origin Session ID: c724a85f-2d37-44ac-9a33-12dcce415aa2

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: the discriminator already exists implicitly, and the ticket's stated rationale is weaker than the real one.

#16491 says: "An empty Map cannot distinguish them, and the core receives only the Map." Mechanically that is not quite true on dev. observedEnvByService[service] is assigned at exactly one site — migrateDeployment.mjs:296, inside if (parsedConfig) — so today an unread service reaches the core as undefined (key absent) while a read-but-empty one reaches it as an empty Map. !(observed instanceof Map) || observed.size === 0 collapses those two. The minimal fix was deleting || observed.size === 0, with no new parameter at all.

I am not asking you to do that — the explicit provenance is the better design, for a reason the ticket does not state:

• the implicit signal is forgeable by initialization. Any future caller that pre-populates observedEnvByService[service] = new Map() across the cohort silently converts "unmeasured" into "read-empty" and authorizes apply on an unread plane. resolveServiceScopes(..., observedEnvByService = {}) and buildPreservedEnv({observedEnvByService = {}}) already default-construct this object, so that is a plausible future edit rather than a contrived one.
• explicit provenance cannot be forged that way: absence blocks, and only a positive configRead === true authorizes.
• it also separates inspected: false (no container) from inspected: true, configRead: false (found but unreadable), which your blocker message uses — the implicit signal cannot express that at all.

Why this is worth a comment rather than a nod: a future reader who notices the implicit discriminator could "simplify" the provenance away as redundant and silently reintroduce the fragility. That reasoning is only wrong for a reason nobody has written down.

Suggested, non-blocking: one clause in the observationByService JSDoc saying the empty-Map-vs-absent-key distinction is not a substitute, because a caller that pre-initializes the map can forge it. Cheap, and it is the sentence that stops the simplification.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. Evidence: L3 is correctly scoped to read-only plan runs, and the body does not claim apply was exercised.
• Anchor & Echo summaries: in-source comments state mechanism, not metaphor. Chroma appears as the live instance of a class, never as a special case in logic — framing and code agree.
• [RETROSPECTIVE] tag: N/A — none added by this PR.
• Linked anchors: #16454's union defect genuinely is the same shape one layer down; the citation is earned, not borrowed.

Findings: Pass on the PR body. One drift on the ticket, flagged above and non-blocking: "the core receives only the Map" overstates the problem, and the true justification for the chosen design is stronger than the recorded one.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The provenance/observation distinction is now stated in-source at both ends.
• [TOOLING_GAP]: None attributable to this PR — though the ticket's own observation is worth keeping: the synthetic fixtures could not represent "observed but legitimately empty", so the unit layer was structurally incapable of catching this. The integration witness caught it on first contact with a real plane.
• [RETROSPECTIVE]: A value that is correct for one question, read as the answer to a different one. Durable form: when a single value can arise from two different causes, the discriminator must be carried, not inferred — and its absence must fail toward the safe answer. This PR gets the direction right, which is the half that is usually wrong.

🎯 Close-Target Audit

• Close-targets identified: #16491 only, newline-isolated Resolves #16491 on line 1.
• For each #N: #16491 carries bug / ai / architecture — confirmed not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #16491 contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Row by row:

Findings: Pass — no drift.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence — L3 is honest and correctly bounded: read-only plan against the canonical plane, explicitly noting nothing was mutated and plan never touches a container
• Two-ceiling distinction: the body states the read-only scope as the reason, not an unprobed ceiling
• Evidence-class collapse check: the body does not promote read-only plan output into an apply claim
• Deployment causality: the plan receipt is reachable from this exact unmerged head (read-only, no merge required), so it is legitimately merge-gate evidence rather than Post-Merge Validation

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — the PR touches no ai/mcp/server/*/openapi.yaml surface.

🔗 Cross-Skill Integration Audit

Findings: N/A — no skill file, workflow convention, MCP tool surface, or AGENTS.md / AGENTS_STARTUP.md change. This adds one optional parameter to an existing pure function; no other skill documents a predecessor step that should now fire.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at ac36b1a486b97def0506b9a88cdfea71b8994110 (all checks SUCCESS); author non-CI receipt present as the read-only plan run against the canonical plane
• Reviewer falsifier: N/A — no named behavioral concern. I did not rerun the suite; per §7.5 green exact-head CI owns routine execution and duplicating it is not evidence
• Test location: pass — test/playwright/unit/ai/scripts/maintenance/, the canonical right-hemisphere path

Findings: Pass. Three things checked specifically rather than assumed:

1. The absent-provenance witness exists — :295, buildMigrationPlan(createPlanInput({observationByService: null})). This is the ledger's explicitly demanded evidence and the one most likely to be skipped, since every happy path passes without it.
2. The fixture can now represent the previously unrepresentable state — createObservation(COHORT, {orchestrator: {inspected: true, configRead: false}}) at :235, and the chroma pair at :270/:274. That directly closes the gap the ticket names as the reason the old suite missed this.
3. Consumer sweep — buildMigrationPlan has exactly one production caller (migrateDeployment.mjs:751), updated in this diff; no other module imports it. So the newly-required-in-practice parameter cannot silently block an un-updated consumer.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — the discriminator is produced where the facts exist (inspectPlane) and consumed where the judgement lives (the pure core), adding no Docker or filesystem reach to the core. Actively checked and cleared: no service-name special-casing, no purity violation, no new module for what is one parameter.
• [CONTENT_COMPLETENESS]: 95 — JSDoc on the new parameter states the ambiguity and the fail-closed rule; in-source comments at both ends explain mechanism. 5 deducted for the challenge above: the recorded rationale is weaker than the real one, leaving the design open to a future "simplification".
• [EXECUTION_QUALITY]: 100 — fail-closed on the missing discriminator, provenance written on every loop exit (verified against the only continue), sole consumer updated, and the absent-provenance path carries its own witness. Checked and cleared: no early-exit path skipping the provenance write, no consumer left un-updated.
• [PRODUCTIVITY]: 100 — every Contract Ledger row delivered, including the note-visibility row, verified through formatPlan rather than stopping at the plan object.
• [IMPACT]: 80 — unblocks apply on every real plane; a deployment path that refuses categorically is close to a dead capability. Below the top band because the surface is one maintenance script rather than a core runtime contract.
• [COMPLEXITY]: 40 — one optional parameter, one relocated branch, and a fixture helper; the difficulty was in seeing the ambiguity, not in expressing the fix.
• [EFFORT_PROFILE]: Quick Win — small, low-risk diff against a categorical blocker on the deployment path.

The part worth keeping: the fix direction was right on the first attempt. Most implementations of "carry a discriminator" default the missing case to permissive, because that is what makes the tests pass; defaulting it to blocked is the choice that survives a caller who forgets.