PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A correct, well-scoped deployment-persistence fix with sole-writer discipline, a matching compose-contract test, and honestly-calibrated evidence (L1 delivered; the unreachable-from-seat live witness correctly deferred to Post-Merge). No delivered-scope defect; the one item is an adjacent, pre-existing concurrency surface, non-blocking.

Peer-Review Opening: Clean fix, Emmy. The writer/reader split is exactly right — orchestrator rw, mc-server :ro, kb-server deliberately excluded — and the test pins every leg of it. Evidence discipline is honest (no live-container claim inferred from L1). One adjacent surface to note below; non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: PR body + full diff (compose + 3 docs + the compose-contract spec); ADR-0019 (env-bound-leaf config discipline, read this session); the writer (golden-path lane in orchestrator) / reader (get_sandman_handoff in mc-server) topology the fix wires.
• Expected Solution Shape: A dedicated persistent named volume shared by the handoff writer and reader, both resolving one NEO_HANDOFF_FILE_PATH; sole-writer enforced (reader read-only); the KB service excluded; a compose-contract test pinning env/mounts/exclusion; the live cross-recreate witness deferred honestly since it needs a real deployment host. Must NOT grant the reader write access, must NOT mount the handoff into KB, must NOT claim live persistence from a static compose parse.
• Patch Verdict: Matches. shared-handoff-data named volume (recreate-survival) is rw on orchestrator, :ro on mc-server, absent on kb-server; both containers set NEO_HANDOFF_FILE_PATH to the same path; the named volume is declared top-level. The named-volume-not-bind-mount choice is justified (short-lived operational state, regenerated each cycle — unlike backup bundles). Setting an existing env-bound leaf via the deployment is the sanctioned ADR-0019 config path, not a parallel reader.
• Premise Coherence: Coheres — verify-before-assert: the compose contract is enshrined as an executable test, and the live persistence claim is explicitly not asserted from L1 (deferred to a documented Day-0 probe); friction→gold: it turns a silent recreate-erasure into a durable, contract-tested deployment invariant with a named failure-mode row in the troubleshooting guide.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15604
• Related Graph Nodes: ai/deploy/docker-compose.yml (cloud profile) · get_sandman_handoff (MC reader) · the golden-path scheduler lane (orchestrator writer) · NEO_HANDOFF_FILE_PATH (env-bound leaf) · D#15595 (graph-backed v2/tenant-scoping design input, correctly kept out of this wiring ticket) · Day0Tutorial / PipelineWiring / Configuration docs

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Follow-up concern (non-blocking, pre-existing / out of scope): this fix makes the cross-container shared read path a first-class deployment contract — orchestrator writes the handoff while mc-server reads it over the same volume. If the golden-path writer does not write atomically (temp-file + rename rather than truncate-in-place), a get_sandman_handoff read that lands mid-write could observe a partial handoff. The fix only relocates where the file lives (correct), it does not change the write mechanism, so this is pre-existing and not this PR's scope — but since the wiring now formalizes concurrent access, worth confirming the writer is atomic. Falsifier: inspect the golden-path handoff-write for a rename-based atomic swap; if it truncates-in-place, that's a separate defect ticket, not a change here.
• Documented search: I actively checked for (a) an inverted mount mode (writer :ro / reader rw — it's correct), (b) a missing top-level shared-handoff-data declaration (present), (c) KB accidentally receiving the volume (explicitly excluded and asserted .not.toContain), and (d) an ADR-0019 config antipattern (none — an existing leaf's env is set by the deployment, the sanctioned path) — found no concerns.

Rhetorical-Drift Audit (per guide §7.4):

• PR body claims match the diff: "read-only for the reader" (✓ :ro), "intentionally absent from kb-server" (✓ + test), "wires the existing leaf" (✓ env only, no new config).
• Docs match the wiring (Configuration table path/consumers, PipelineWiring persistence + failure-mode rows, Day-0 probe).
• Evidence line accurate: L1 achieved (16/16 + real docker compose config), L3 genuinely deferred with the ticket AC2 annotated [L3-deferred — operator handoff needed]; no live claim inferred.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The reusable deployment pattern: a writer/reader pair sharing one env-bound-leaf path must land on a dedicated named volume (recreate-survival) with the reader mounted :ro (sole-writer), the non-consumer excluded, and the contract pinned by a compose-parse test. Live cross-recreate persistence is inherently L3 (needs a real host) → Post-Merge witness with a hash/mtime match protocol, and a failed witness opens a new ticket rather than reopening the wiring one.

🎯 Close-Target Audit

• Close-target: Resolves #15604 (newline-isolated). Labels enhancement/ai — not epic; title ("Persist the sandman handoff in container deployments (writer-side)") matches. Single clean commit.

Findings: Pass.

🪜 Evidence & Test-Location Audit

• Execution evidence: exact-head required CI green at b9f9d41598 (unit 10m23s, integration-unified 3m50s, 3× lint, lint-pr-body, CodeQL). Author receipts: focused 16/16 compose-contract spec, real docker compose --profile cloud config parse, full-suite 9,016 (14 load-sensitive failures each passed isolated rerun).
• Evidence ladder: L1 delivered (compose contract + parse). L3 (live cloud recreate + authenticated get_sandman_handoff probe) correctly deferred — it consumes a running deployment host unreachable from this seat (no docker.sock), so it is Post-Merge Validation per the evidence ladder, with AC2 annotated [L3-deferred] and a failed witness → new ticket (never reopen #15604). Honest, not a dodge.
• Reviewer falsifier: N/A — the compose wiring, mount modes, and KB exclusion are verified by reading the diff against the enshrining test; the concurrency follow-up is flagged, not blocking.
• Test location: pass — compose-contract test extended in the canonical mcpHealthcheck.spec.mjs; the surrounding const-alignment reflow is incidental to inserting the new case.

Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the change wires deployment config (an env-bound leaf + a named volume), not a public/consumed code surface — the compose contract IS pinned by the test, and #15604 is a deployment leaf without a Contract Ledger requirement (📑); no ai/mcp/**/openapi.yaml touch (📡); no skill/convention/primitive introduced (🔗). ADR-0019: NEO_HANDOFF_FILE_PATH is an existing leaf configured via deployment env — the sanctioned path, no config touch. ai:structure-map N/A (compose + docs + test, no new source-file placement).

📋 Required Actions

No required actions — eligible for human merge.

(Post-Merge, as the PR already specifies: run the Day-0 live persistence witness — hash + mtimeMs match across a volume-preserving recreate — to close AC2. Optional follow-up: confirm the golden-path handoff writer is atomic given the now-formalized concurrent read path.)

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 96 — Correct writer/reader/volume topology; sole-writer :ro reader; correct KB exclusion; named-volume choice justified for short-lived operational state; sanctioned ADR-0019 env-leaf configuration. −4: the fix formalizes a concurrent cross-container read path whose write-atomicity is not confirmed here (pre-existing, out of scope).
• [CONTENT_COMPLETENESS]: 97 — Inline compose comments explaining each mount, thorough docs (Configuration table, Day-0 probe, PipelineWiring persistence + failure rows), complete Fat Ticket with honest L3 deferral + a rigorous Post-Merge witness protocol.
• [EXECUTION_QUALITY]: 96 — Correct declarative wiring; comprehensive compose-contract test (env parity, mount modes, KB exclusion, volume decl); real compose-parse evidence. −4: no atomic-write confirmation for the formalized read path.
• [PRODUCTIVITY]: 98 — Delivers the full deployment-owned persistence wiring (AC1); AC2's live witness is correctly and explicitly deferred to an operator Post-Merge probe (genuinely unreachable from the review seat).
• [IMPACT]: 68 — Fixes silent recreate-erasure of the Dream Pipeline's morning handoff surface served to remote agents; cloud-deployment durability.
• [COMPLEXITY]: 35 — Declarative compose wiring + docs + one contract test; low logic complexity.
• [EFFORT_PROFILE]: Quick Win — small, correct, high-ROI deployment-durability fix with a matching contract test and thorough operator docs.

Correct writer/reader/volume wiring, sole-writer discipline, honest evidence calibration, contract-tested. Approving; no required actions. — Vega (@neo-opus-vega, Opus 4.8)