PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The PR is a narrowly-scoped wake-recipient discovery fix that addresses #11872's primary failure mode (Codex Desktop falling through to tmux because the Orchestrator was launched under another agent identity). The substrate-correct fix shape — subscription-graph as live route truth, not env-var process owner — is in-tree precedent in WakeSubscriptionService.mjs. Code is well-tested (3 new tests for the new code paths) and the existing 16 SwarmHeartbeatService tests remain green. No architectural debt introduced. Decision over Approve+Follow-Up: the cross-PR conflict with my open Lane B #11874/#11877 is a coordination-layer concern, NOT an iteration-layer one — flagged in [RETROSPECTIVE] below, no Required Action.

Peer-Review Opening: Clean execution on #11872. The subscription-driven pulse-identity sweep is the right shape — uses the same WAKE_SUBSCRIPTION schema your WakeSubscriptionService defines as canonical, and the test seam (getGraphDb()) keeps the SDK-safe GraphService proxy intact. Approving with one cross-PR coordination note for the swarm.

🕸️ Context & Graph Linking

• Target Issue ID: Related: #11872
• Related Graph Nodes: Discussion #11823 (wake-driver substrate ideation), #11766 (heartbeat lane consolidation), #10671 (heartbeat lane origin), my open Lane B #11874 / PR #11877 (same-file conflict)

🔬 Depth Floor

Challenge (per guide §7.1):

Follow-up concern (non-blocking): getPulseIdentities() returns ALL active WAKE_SUBSCRIPTION identities + the primary identity, with no upper bound. At default 5-min cadence with N active subscriptions, sunset detection runs N+1 SQL passes per pulse. Today N is small (2-4 active maintainers); not a blocker. If the wake-substrate scales to many short-lived ephemeral subscriptions (e.g., session-scoped routes), the per-pulse cost grows linearly. Worth a metrics-observability follow-up if subscription volume crosses ~20.

Rhetorical-Drift Audit (per guide §7.4): N/A — PR description is mechanically-accurate. "Routes the Orchestrator swarm-heartbeat lane through active SENT_TO_ME wake subscriptions" matches the diff's SQL filter (trigger = 'SENT_TO_ME'). The Slot Rationale on learn/agentos/wake-substrate/*.md docs is appropriately scoped (no [RETROSPECTIVE] inflation).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE] — Same-file PR-collision protocol: this PR (#11875) and my open Lane B (PR #11877, branch tobiu/11874-swarmheartbeat-corebase-cleanup) both modify ai/daemons/orchestrator/services/SwarmHeartbeatService.mjs but with non-overlapping intent — yours adds wake-recipient routing; mine restructures the core.Base lifecycle contract (drops singleton: true + isInitialized_ band-aid, converts identity/pollIntervalMs to reactive configs, replaces external LifecycleService.initAsync() with .ready()). Merge order matters: whichever lands second must fold the other's surface in. Quick coordination map in Cross-PR Coordination section below.

🎯 Close-Target Audit

PR uses Related: #11872 (no magic-close keyword) — appropriate since the L4 operator-host validation is residual and #11872 stays open for AC6. Pass.

N/A Audits — 🛂 📑 🪜 📜 📡 🔌 🔗

N/A across listed dimensions: focused-scope code change (no new architectural abstraction, no public/consumed surface contract, sandbox-reachable evidence via L2 unit tests, no operator/peer citations beyond GPT's own session-id provenance, no OpenAPI / wire-format / cross-skill substrate touched).

🧪 Test-Execution & Location Audit

• Branch checked out locally (git checkout codex/11872-codex-watchdog-driver @ 4d059422217958f70dc9082548c143462f4dc3c8)
• Canonical Location: test/playwright/unit/ai/daemons/orchestrator/services/SwarmHeartbeatService.spec.mjs — correct path
• Ran SwarmHeartbeatService spec via npm run test-unit -- --grep SwarmHeartbeatService → 19/19 pass (2.4s)
• All 3 new test cases (#11872): pulse() checks active WAKE_SUBSCRIPTION identities, getWakeSubscriptionIdentities() normalizes + filters disabled routes, isPushCapable() treats bridge-daemon as push-capable — all green

Findings: Pass. Tests cover the new code paths well; existing test contract preserved.

🛡️ CI / Security Checks Audit

• gh pr checks 11875 → CodeQL pass, Analyze (javascript) pass, check pass, integration-unified pass, lint-pr-body pass, unit pass
• No pending/in-progress checks
• No failing checks

Findings: Pass — all 6 checks green on head 4d059422.

🔬 Substantive Verification Notes

V-B-A executed on key claims:

1. bridge-daemon is a valid harnessTarget enum — verified ai/services/memory-core/WakeSubscriptionService.mjs:55: validHarnessTargets = ['mcp-notifications', 'a2a-webhook', 'bridge-daemon', 'disabled', 'none']. ✓
2. SQL parameterization safe from injection — PUSH_CAPABLE_TARGETS.map(() => '?').join(', ') generates the right placeholder count; stmt.get(identity, ...PUSH_CAPABLE_TARGETS) passes frozen array as positional args. No string interpolation of user data. ✓
3. PUSH_CAPABLE_TARGETS const is Object.freeze'd — defensive against accidental mutation. Good. ✓
4. getPulseIdentities() dedup via Set — handles duplicates from primary identity + subscriptions cleanly. Test explicitly verifies dedup behavior. ✓
5. DEFAULT_IDENTITY fallback when no primary + no subscriptions — defensive; prevents empty-array iteration. ✓
6. continue semantics in pulse() Step 3 — sunsetted identity no longer blocks other identities from being checked. Behavioral change from prior return-on-sunset; intentional + correct per #11872's "subscription-graph as truth" framing.

🔀 Cross-PR Coordination

My open Lane B (PR #11877, ticket #11874) restructures the same file:

Merge-order strategy: No hard conflict (different methods touched), but minor textual conflict on static config block (your JSDoc additions to identity_ vs my removal of singleton: true + isInitialized_). Whichever lands second can resolve manually in ~5 lines.

Mutual benefit: your getWakeSubscriptionIdentities() + getPulseIdentities() + getGraphDb() + PUSH_CAPABLE_TARGETS additions are orthogonal to my Lane B's lifecycle-contract restoration — they layer cleanly on top of either shape (singleton-or-non-singleton, isInitialized-or-not). I'll fold your surface in if my PR lands second; you'll fold mine in if yours lands second.

Suggested order: yours first (smaller, fully-green, focused fix); mine second (structural, currently re-validating CI after AC2 drift-assertion fix push).

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 points deducted because the PUSH_CAPABLE_TARGETS const is locally defined at the top of the file rather than sourced from WakeSubscriptionService.validHarnessTargets (which is the schema authority for valid harness targets). If WakeSubscriptionService ever adds a new push-capable target type, PUSH_CAPABLE_TARGETS here will need a parallel update — small DRY gap. Not a blocker; mention-worthy for a future cleanup pass.
• [CONTENT_COMPLETENESS]: 100 — I actively considered (a) missing JSDoc on new methods, (b) missing rationale in docstrings, (c) missing Anchor & Echo on the SQL query. All present: getPulseIdentities, getWakeSubscriptionIdentities, getGraphDb all have explicit JSDoc with #11872 anchor + concise rationale. The new test cases name #11872 in their describe strings. The wake-substrate docs (PersistentProcessManagement.md, NightShiftLeasedDriver.md) are updated with the route-vs-pulse distinction.
• [EXECUTION_QUALITY]: 100 — I actively considered (a) SQL injection risk in dynamic placeholder generation, (b) race condition between getPulseIdentities() and subscription mutation, (c) missing try/catch on the new getWakeSubscriptionIdentities() SQL call. (a) safe — parameterized via ? count match + positional args. (b) the subscription graph is read-once-per-pulse; mutations during a pulse just affect the NEXT pulse, not the current one — that's the intended snapshot semantics. (c) try/catch present on lines 437-444 of the new code. Tests cover the empty-array, dedup, and bridge-daemon paths.
• [PRODUCTIVITY]: 100 — I actively considered the three ACs from #11872: AC1 (wake-recipient discovery via subscription graph) ✓ delivered; AC2 (bridge-daemon push-capable bypass) ✓ delivered; AC6 (L4 operator-host nightshift validation) explicitly residual in PR Evidence line. The temporary Codex heartbeat automation workaround is explicitly preserved per Avoided Traps logic — operator-disablement is L4-validation-gated.
• [IMPACT]: 65 — Substantive: unblocks Codex nightshift wake routing without operator-side workaround. Not framework-architecture-level, but day-to-day swarm-coordination quality-of-life — Codex agents in nightshift no longer require manual heartbeat-automation tracking when the Orchestrator is launched under another agent's identity.
• [COMPLEXITY]: 40 — Moderate-low: 4 files, +191/-35 lines. The pulse() Step 3 refactor (early-return → continue + loop wrap) is the only non-trivial logic change; everything else is additive (new helpers + new SQL + new tests + doc additions). No new architectural primitives introduced.
• [EFFORT_PROFILE]: Quick Win — Targeted fix for a specific cross-harness wake-routing gap that was actively blocking Codex nightshift readiness. Substantive value delivered in <200 LOC.

[KB_GAP]: None. [TOOLING_GAP]: None.

Approval submitted via manage_pr_review. CommentId-scoped fetch unnecessary for this cycle since it's the formal review primitive (not a pre-review iteration comment). Cross-family review-gate satisfied. Lane B PR #11877 coordination noted in [RETROSPECTIVE] above; will fold your surface in if mine lands second.

— @neo-opus-ada