PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe head delivering every close-target AC at the required evidence class — the ticket demanded a live or CI container-recreate witness and this PR shipped the CI witness rather than deferring it. No correctness deferral and no scope transfer exists, so neither Request Changes nor Approve+Follow-Up has substrate. The one named watch-item (off-host export of the new volume) is V-B-A-calibrated below as operational preference, not deferred correctness.

Peer-Review Opening: Thanks Emmy — this closes exactly the gap #15759 named, with the strongest evidence shape of the persistence arc so far: the two-container SHA-256 witness lands the rung we had to leave L3-deferred on PR #15753. The fail-closed healthcheck is a genuine improvement over the ticket's minimum.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #15759 (fat body + Contract Ledger + intake comment), current dev compose (orchestrator mounts sqlite / tenant-repos / handoff volumes but nothing at /app/.neo-ai-data/orchestrator-daemon; the healthcheck at ai/deploy/docker-compose.yml:253 carried a hardcoded path fallback), ai/configBase.mjs:593 (dataDir: leaf('.neo-ai-data/orchestrator-daemon', 'NEO_AI_ORCHESTRATOR_DIR', 'string')), ai/daemons/orchestrator/daemon.mjs:132-166 (singleton reconciliation body), sibling composes (docker-compose.dev.yml / docker-compose.test.yml define no orchestrator service), ADR 0019 (re-read this session per the read-gate) + ADR 0014 boundary, and precedent PR #15753 (named-volume pattern, my prior review).
• Expected Solution Shape: A dedicated named volume mounted only into the orchestrator at the leaf's cloud path; deployment env pins the EXISTING AiConfig.orchestrator.dataDir leaf (no second path authority, no ai/ config touch); healthcheck on the same contract; a compose-contract test plus a recreate witness; guides de-overclaimed. Must NOT hardcode a parallel path constant in ai/ runtime and must not leak the mount into the kb/mc services. Witness isolation belongs on a disposable volume, never the shared integration stack.
• Patch Verdict: Matches and improves. Concrete improvements over the expected minimum: the healthcheck goes fail-closed (if(!d)process.exit(1)) instead of keeping the env-or-default path duplication; the sole-owner sweep asserts no alias by volume SOURCE and no alias by mount TARGET across every other service; the integration witness derives volume name and target from the production compose instead of restating literals.
• Premise Coherence: Coheres — verify-before-assert applied to the repo's own docs (the unsupported "already backed up" claim is removed and replaced with the exact durability-vs-backup boundary), and least-privilege ownership (sole-owner mount) over convenience.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15759
• Related Graph Nodes: #11724 (predecessor persistence overclaim), #11937 (healthcheck reads the state file), #15604 / PR #15753 (named-volume precedent, handoff), #15748 / PR #15752 (fail-closed ingest checkpoints), #15740 / PR #15757 (the restore ledgers this volume now persists), ADR 0014, ADR 0019.

🔬 Depth Floor

Challenge (per guide §7.1):

• Follow-up concern (non-blocking, calibrated): orchestrator-state is now load-bearing continuity state that the logical backup bundle deliberately excludes. I verified the actual bundler — ai/scripts/maintenance/backup.mjs:87 documents its by-design exclusion list and enumerates only memory/summary/graph sources; the orchestrator state dir appears nowhere — so the guides' "not off-host backed up" statement is true as shipped. I then V-B-A'd whether that gap is must-fix: on host loss, tenant-repo-sync-revisions.json costs a bounded re-ingestion pass (mirrors are reproducible, Chroma re-ingests), orchestrator-state.json regenerates on the next cycles, and ledgers are forensic — bounded rework, not primary-store loss (unlike the MC sqlite volume). Off-host export therefore stays an operational preference, correctly documented rather than ticketed. Watch-item: if a future consumer makes revision continuity compliance-relevant (audit-trail semantics), that calculus flips — hypothesis, needs V-B-A before implementation.

Documented search (supplementary): I actively looked for (1) healthcheck divergence in sibling composes — none define an orchestrator service; (2) import-side-effects of newly exporting enforceSingleton — the spec already imports from daemon.mjs, daemon start is guarded, and unit CI green at head re-proves it; (3) nested-mount conflicts with the existing sqlite / tenant-repos / handoff / backups mounts — all sibling paths, no shadowing; (4) behavior drift in the parameterization — {pidFile = pidFilePath()} = {} preserves the zero-arg call site at daemon.mjs:223 exactly, and the dead/unrelated/live-owner triage matches the new JSDoc claim line-for-line (daemon.mjs:135-165); (5) platform sensitivity of the fixture PID 2147483647 — it exceeds macOS and default Linux pid_max, so process.kill(pid, 0) takes the ESRCH dead-path on both, and CI plus local receipts confirm. No concerns found.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology; the compose comment states the durability-not-backup boundary at the definition site
• [RETROSPECTIVE] tag: N/A — none present in the PR body
• Linked anchors: ADR 0014 / ADR 0019 citations verified — the leaf pre-exists at ai/configBase.mjs:593 and the topology boundary is unchanged, so "no amendment required" holds

Findings: Pass — "no sibling service can mount the source or target" is mechanically enforced by the new contract test; "fails closed" matches the shipped healthcheck exactly; "the daemon's existing singleton reconciliation is now directly testable" is precisely what the export plus test deliver, with zero behavior change.

🧠 Graph Ingestion Notes

• [KB_GAP]: None observed — the cookbook/pipeline/tutorial edits themselves close the documentation gap the ticket named.
• [TOOLING_GAP]: None observed in this PR's lifecycle.
• [RETROSPECTIVE]: The named-volume persistence pattern from PR #15753 (dedicated volume + ownership discipline + compose-contract test) generalizes here from writer/reader-pair to sole-owner, and gains the missing evidence rung: a two-container SHA-256 recreate witness that derives its contract values from the production compose instead of restating them. Also worth remembering: the witness fixture includes a restore-empty-target-ledger/ entry — the restore arc (#15740 / PR #15757) and the persistence arc now compose on the same volume boundary.

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml surface is touched and no new workflow convention, skill file, or MCP tool is introduced — deployment wiring, tests, and guides only.

🎯 Close-Target Audit

• Close-targets identified: #15759 (PR body, newline-isolated Resolves #15759; the single commit c8819e7a18 headline carries the ticket ref only)
• For each #N: confirmed not epic-labeled — #15759 carries bug, ai, testing, architecture, build

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (leaf resolution via the existing env binding · dedicated sole-owner volume · durable-vs-process-epoch state split with boot reconciliation coverage · durability-vs-backup doc distinction)

Findings: Pass — no drift; every ledger row has a shipped, evidence-backed counterpart.

🪜 Evidence Audit

• PR body contains the greppable Evidence: declaration line
• Achieved ≥ required: ticket AC7 requires a live or CI container-recreate witness; the integration-unified job executed Milestone 7a (real Docker, two distinct containers, SHA-256 value equality) at the exact head — the requirement is MET in CI, not deferred
• Residuals: the live cloud rollout is correctly framed as Post-Merge Validation with a concrete protocol (record values/hashes → recreate without down -v → verify persistence, healthcheck, and unblocked boot)
• Two-ceiling distinction: explicit ("The external cloud deployment remains a post-merge rollout validation, not evidence fabricated by local logic tests")
• Evidence-class collapse check: none — local logic receipts and the CI Docker witness are kept distinct throughout the body
• Deployment causality: the witness ran from this exact unmerged head inside required CI

Findings: Pass — the strongest evidence calibration of the persistence arc to date.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at c8819e7a18 (14/14 incl. unit, integration-unified, lint-pr-body, CodeQL) + author non-CI receipts (targeted unit runs 40 passed; docker compose --profile cloud config --quiet; ai:lint-config-template-ssot; agent-preflight) — present and head-appropriate
• Reviewer falsifier: N/A — no named behavioral concern survived the source reads; my verification was source-level (daemon.mjs:132-166, configBase.mjs:593, bundler exclusion list at backup.mjs:87)
• Test location: pass — unit additions extend the canonical test/playwright/unit/ai/daemons/orchestrator/daemon.spec.mjs; the witness lands in the owning AdoptionLadderJourney.integration.spec.mjs Milestone 7 slot, with 7b's skip note keeping whole-stack round-trip ownership explicit

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 - Persistence lands at the exact ADR 0014 Compose boundary; the env pins the pre-existing ADR 0019 leaf instead of minting a second path authority; sole-owner mount + fail-closed healthcheck collapse the path truth into one contract. −3: the path literal is now pinned in compose, the unit contract test, and the docs table — intentional as contract witnesses, but a future path move must touch all three (the integration witness, which derives its values, shows the drift-proof alternative).
• [CONTENT_COMPLETENESS]: 95 - New and changed functions carry Anchor & Echo JSDoc (enforceSingleton, runDocker, parseLastJsonLine); the compose comment documents the boundary at the definition site; the PR body is a complete fat ticket with honest deltas. −5: the Day-0 checklist tells operators to "record hashes" without naming a how — the witness script embodies one; a one-line operator recipe would close it.
• [EXECUTION_QUALITY]: 95 - Real two-container recreate witness with content hashing; the sole-owner negative sweep covers alias-by-source AND alias-by-target; dead-PID reclaim proven at the actual reconciliation function with zero behavior change. −5: the fail-closed healthcheck's env-missing exit path is asserted structurally (env-usage plus no-fallback text) but not behaviorally, and 7a's payload is representative fixture data rather than daemon-written state — both honestly bridged by the contract test plus the post-merge protocol.
• [PRODUCTIVITY]: 98 - All seven ACs delivered with evidence at the required class; the doc overclaim corrected across all four named guides. −2: the off-host export policy the guides now demand has no owning surface yet (deliberately out-of-scope; see Depth Floor calibration).
• [IMPACT]: 72 - Protects scheduler cadence, ingestion cursors, recovery ledgers, and diagnostics across every cloud container recreate, and stops the healthcheck from depending on state the container layer could erase — significant operational-continuity repair, bounded by being deployment-layer rather than core architecture.
• [COMPLEXITY]: 58 - Eight files across four substrate classes (compose topology, daemon export, two test layers, four guides); the Docker witness introduces real container orchestration into the test surface but stays bounded and self-cleaning.
• [EFFORT_PROFILE]: Quick Win - High continuity-protection ROI on a bounded change with an established precedent pattern (#15753); the marginal novelty — the recreate witness — is exactly where the effort went.

The fail-closed ingest arc (#15748), the restore arc (#15740), and now the persistence arc converge on one discipline: state the boundary, pin it mechanically, calibrate the evidence honestly. This PR is a model instance of it. 🌿