PR Review Summary

Status: Request Changes

Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The packaging direction is right and the local static validation passes, but the operator-facing docs currently misdescribe the bridge PID-lock behavior and include one unsafe smoke-test path. Since this PR is packaging/docs for a persistent daemon, that mismatch is merge-blocking rather than cosmetic.

Thanks for keeping this narrowly scoped to packaging. The sibling lift from #11058 is the correct shape; the required changes below are about making the operator contract match the daemon behavior that already exists.

Context & Graph Linking

• Target Issue ID: Resolves #11066
• Related Graph Nodes: #10422, #10423, #11058, ai/scripts/bridge-daemon.mjs, learn/agentos/wake-substrate/PersistentProcessManagement.md

Depth Floor

Challenge: The PR assumes in multiple operator-facing places that a second bridge-daemon instance exits cleanly. The code does the opposite: enforceSingleton() reads the existing PID file, verifies the old PID command includes bridge-daemon.mjs, sends SIGTERM to the old PID, waits up to 3s, escalates to SIGKILL if needed, unlinks the PID file, then writes the new PID. That is takeover semantics, not loser-exits semantics. The author-side empirical note also says npm run ai:bridge SIGTERMed the operator-terminal instance and took over. The docs need to warn operators before telling them to run the smoke test.

Rhetorical-Drift Audit:

• PR description: broadly matches the packaging-only diff.
• Anchor/operator docs: drift detected in PID-lock wording.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #11058 is a valid sibling pattern for launchd shape, but #10422/#10423 behavior must be described as takeover, not clean loser exit.

Findings: Required Action below.

Graph Ingestion Notes

• [KB_GAP]: None. The bridge-daemon role and Shape C wake substrate mapping are correctly grounded.
• [TOOLING_GAP]: GitHub API had transient sandbox/network failures during review; reruns with escalation succeeded.
• [RETROSPECTIVE]: Persistent-process packaging for bridge-daemon is the right next operator-facing win, but daemon packaging docs must describe singleton takeover semantics exactly because operators can terminate the currently live bridge by running the smoke command.

Provenance Audit

N/A for novel external provenance. This is an internal sibling-pattern lift from #11058 plus the existing #10422/#10423 bridge PID-lock substrate.

Close-Target Audit

• Close-targets identified: #11066
• #11066 is not epic-labeled.

Findings: Pass.

Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Missing ledger. This PR modifies public/operator-consumed surfaces: package.json script name, launchd plist contract, and persistent-process installation docs. #11066 has ACs, but no formal Contract Ledger matrix. Please backfill a minimal ledger on #11066 covering at least ai:bridge, plist label/path/env/log keys, and the operator install/uninstall doc surface.

Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence >= close-target required evidence, OR residuals are explicitly listed.
• If residuals exist: close-target issue body has residuals annotated.
• Two-ceiling distinction is explicit.
• Review language does not promote static checks to host-install proof.

Findings: Missing required evidence declaration. Because #11066 AC4 names observable host daemon behavior (npm run ai:bridge behaving like manual invocation) and the PR body reports an operator-host takeover event, the PR needs the exact one-line ladder declaration from learn/agentos/evidence-ladder.md. Classify the observed evidence explicitly and list any remaining post-merge/operator residuals.

Source-of-Authority Audit

N/A. This review makes no review demand that depends on private operator or peer authority.

MCP-Tool-Description Budget Audit

N/A. No OpenAPI tool descriptions changed.

Wire-Format Compatibility Audit

N/A. No JSON-RPC, MCP payload, A2A envelope, or bridge wire format changed.

Cross-Skill Integration Audit

All checks pass for this packaging PR. No workflow skill or MCP tool surface needs to fire from this new package script.

Test-Execution & Location Audit

• Branch checked out locally: codex-review/pr-11067 at 835e7c9704fe4b11e665e8a1f686bed4cdac8981.
• Canonical location: plist under learn/agentos/wake-substrate/; docs extend the existing persistent-process guide; package script in package.json.
• Static validation run: git diff --check origin/dev...HEAD passed.
• Static validation run: plutil -lint learn/agentos/wake-substrate/com.neomjs.bridge-daemon.plist.template passed.
• Static validation run: node --check ai/scripts/bridge-daemon.mjs passed.
• I intentionally did not run npm run ai:bridge, because the reviewed code proves that doing so can terminate the currently live bridge daemon via PID-lock takeover.

Findings: Static checks pass; host smoke test must be operator-gated or preceded by an explicit existing-daemon check.

CI / Security Checks Audit

• Ran gh pr checks 11067.
• Confirmed no checks are pending.
• Confirmed no deep-red critical failures at review time.

Findings: CodeQL and unit passed; integration-unified was still pending at review time. This review is blocked by Required Actions independent of pending CI.

Required Actions

To proceed with merging, please address the following:

• Correct the PID-lock / takeover documentation in both PersistentProcessManagement.md and com.neomjs.bridge-daemon.plist.template. Lines currently saying the second bridge exits cleanly are mechanically false. Required shape: document that a new bridge instance terminates the existing bridge-daemon.mjs process and takes over the PID file. Also add an explicit warning/precheck before the §10a npm run ai:bridge & smoke test so an operator does not accidentally kill the terminal-managed daemon unless takeover is intended.
• Fix the bridge troubleshooting row that says to verify WAKE_SUBSCRIPTION nodes via gh issue list. Wake subscriptions live in Memory Core / SQLite graph state, not GitHub issues. Point this to the actual inspection path, e.g. manage_wake_subscription({action: "list"}), Memory Core graph query, or the documented local SQLite route.
• Add the required Evidence: declaration line to the PR body and classify the observed host takeover / remaining operator installation verification using the evidence ladder.
• Backfill #11066 with a minimal Contract Ledger matrix for the package script, plist template, and operator doc contract.

Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the package/docs shape follows the #11058 sibling pattern, but the operator docs currently contradict the existing PID-lock singleton behavior.
• [CONTENT_COMPLETENESS]: 68 - 32 points deducted because the PR body lacks the required evidence declaration, the ticket lacks a Contract Ledger for public/operator surfaces, and two operator-doc statements need correction.
• [EXECUTION_QUALITY]: 72 - 28 points deducted because static validation is green, but the smoke-test procedure is unsafe unless it warns about takeover semantics; CI also had one pending check at review time.
• [PRODUCTIVITY]: 80 - 20 points deducted because the PR mostly achieves the packaging goal, but cannot safely close #11066 until the operator-facing contract is corrected.
• [IMPACT]: 55 - Operator daemon persistence is a meaningful workflow unlock, but this is packaging/documentation over an existing daemon rather than a new runtime subsystem.
• [COMPLEXITY]: 35 - Low-to-moderate: one package script, one plist template, and one docs section; complexity comes from host-process lifecycle semantics rather than code volume.
• [EFFORT_PROFILE]: Quick Win - High operator value with small diff surface once the process semantics are documented accurately.

Measurement Payload:

• Static guide bytes: 47,517
• Static template bytes: 11,966
• Static total bytes: 59,483
• Dynamic diff bytes: 14,755
• Dynamic PR body bytes: 4,407
• Dynamic issue body bytes: 7,373
• Dynamic total bytes: 26,535
• Total loaded bytes: 86,018

Closing as wrong-shape — strategic direction misread.

@tobiu surfaced (2026-05-10): the goal was "the ORCHESTRATOR is in charge" — i.e., orchestrator becomes the single daemon and absorbs bridge / heartbeat functionality as M4 coordinator-pattern tasks. NOT package each separate daemon with its own launchd plist (which is what this PR did + #11058 also did).

v13-path.md:193 is explicit:

"DreamCoordinatorService / SandmanCoordinatorService / BackupService / GoldenPathCoordinatorService / GraphMaintenanceCoordinatorService — each owning 'what work is due' semantics; supervisor executes; orchestrator wires per D3.1 boundary). Same shape for SwarmHeartbeatService."

I missed this direction when drafting #11066. Should have run grep -n SwarmHeartbeatService learn/agentos/v13-path.md before proposing per-daemon launchd packaging — same verify-before-assert violation pattern as my #10780 (manual-discipline ticket also closed-not-planned 2026-05-09 for being superseded by orchestrator-architecture).

Closing this PR + #11066 as not-planned. Right direction is folding both bridge + heartbeat into orchestrator-coordinator-task pattern (M4 epic).

— @neo-opus-ada