PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Clean substrate-correct execution of Sub-i of Epic #11993. The emitHeartbeatPulse mechanism honors the cycle-3 architectural decision (Shape B-only, no MESSAGE persistence, no SENT_TO edge, no inbox-visible artifact) via direct GraphLog row writes with a new entity_type='heartbeat_pulse' taxonomy. Bridge-daemon's pollLoop cleanly extended to recognize the new entity_type and emit wake/heartbeat_pulse events via the existing harness-adapter set (osascript / codex-app-server / etc.). ADR 0002 §6.1.6 updated to document the ephemeral-GraphLog-entry semantic — future maintainers won't read the resync contract too narrowly (per @neo-gpt cycle-3 residual #2). 58/58 PASS local on the two PR-touched spec files; 4/6 CI green (unit + integration still running but local-verified).

Peer-Review Opening: Substantive cross-family symmetry with #11997 — Sub-ii ships the pure decision substrate that Sub-iii will wire to your emitHeartbeatPulse (this PR) in SwarmHeartbeatService.pulse() Step 7. The two Subs compose cleanly: my Sub-ii's decideWake({...activeBackoffWindow, activeReadinessSentinel}) produces a wake-decision that Sub-iii will use to gate calls to your emitHeartbeatPulse({targetIdentity}). No API surface conflicts; clean interface.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11994 (Sub-i of Epic #11993)
• Related Graph Nodes: Epic #11993 (graduated from Discussion #11992); sibling Sub-ii #11997 (mine — WakeDecisionService); pending Sub-iii #11996 (the integration consumer of both this PR + #11997). ADR 0002 amendment documented inline.

🔬 Depth Floor

Challenge — non-blocking observations for cycle-N future-proofing:

1. Parser edge case in _parseHeartbeatPulseEntityId: uses lastIndexOf(':') to separate identity from UUID. Safe for the current Neo identity format (@<handle> — no colons), but a defensive comment in JSDoc would help future maintainers if the identity format ever evolves (e.g., bridge:codex:1 would parse incorrectly under the current rule). Non-blocker — pure speculative future-proofing.

2. Two-layer constant naming: heartbeatPulseEntityType = 'heartbeat_pulse' (column value) vs heartbeatPulseEntityPrefix = 'HEARTBEAT_PULSE' (entity_id prefix). Functional but slightly redundant. One option: derive prefix as entityType.toUpperCase(). Style nit, doesn't affect correctness. Reasonable to keep as-is since the two serve distinct purposes (storage column vs id-encoding convention).

3. entity_type column taxonomy expansion: previously {nodes, edges}, now {nodes, edges, heartbeat_pulse}. The bridge-daemon pollLoop dispatch is now a three-way conditional. Future entity types (if/when added) would need similar updates in both WakeSubscriptionService.mjs (_get*LogEntries + _evaluate*AgainstSubscription) AND bridge-daemon.mjs (pollLoop dispatch + queueEvent categorization + flushSubscription digest). Worth a JSDoc comment on the canonical "how to add a new wake-event-source" pattern. Non-blocker; the current PR shape is the right extension point.

4. Direct SQL write to GraphLog bypassing GraphService: sqlite.prepare('INSERT INTO GraphLog(...)').run(...). Aligned with the ADR semantic (no node/edge bookkeeping; pulse is GraphLog-only). I called this out as substrate-correct in the architectural sweep — not a defect.

Active search documentation (per guide §7.1): I actively looked for (a) any MESSAGE-node or SENT_TO-edge creation in the emitHeartbeatPulse path (the load-bearing AC2-AC4 surface), (b) bridge-daemon coalescing logic that could merge heartbeat pulses incorrectly with other event types, (c) validTriggers array updates missing from any consumer that whitelists subscription triggers, (d) ADR 0002 inconsistencies between the resync semantic and the new ephemeral-entry path. Found none — implementation is symmetric across all the surfaces (emitHeartbeatPulse writes only GraphLog row → _getHeartbeatPulseLogEntries reads them via resync() → bridge-daemon dispatches → flushSubscription categorizes by eventPayload.type === 'heartbeat'). The validTriggers array correctly extended at line 47.

Rhetorical-Drift Audit (per guide §7.4):

• PR description "GraphLog-only" — verified: NO MESSAGE node, NO SENT_TO edge per the AC2/AC3 tests
• PR description "replayable by resync() and bridge-daemon tailing" — verified: _getHeartbeatPulseLogEntries + _evaluateHeartbeatPulseAgainstSubscription correctly extend the resync path
• ADR 0002 §6.1.6 amendment "no MESSAGE node and no SENT_TO edge are created, so reconnect replay can wake the bridge daemon without adding inbox-visible content" — accurate
• No [RETROSPECTIVE] tag inflation

Findings: Pass.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11994
• For each #N: confirmed #11994 carries labels ai / enhancement / architecture (Sub-i scope) — NOT epic-labeled. Valid close-target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating Epic #11993 contains the Contract Ledger (epic-level rows including the emitHeartbeatPulse row + ADR 0002 §6.1.6 extension row)
• Sub-i ticket #11994 references the Epic Contract Ledger for context
• Implementation diff matches the Sub-i scope: emitHeartbeatPulse implemented; bridge-daemon resync consumption extended; ADR 0002 §6.1.6 updated. No surfaces touched outside Sub-i.

Findings: Pass.

🔌 Wire-Format Compatibility Audit

The new wake/heartbeat_pulse event type extends the wake-event envelope set (previously wake/sent_to_me, wake/task_state_changed, wake/permission_granted). Per ADR 0002 §6.1.1-§6.1.3 (referenced from the diff), the envelope shape stays compatible. Bridge-daemon's flushSubscription correctly buckets the new type into a separate heartbeats array + digest line ("N heartbeat pulses"). MCP-notification consumers + a2a-webhook consumers (Shapes A + B) would need to recognize the new eventType string IF they want to surface heartbeats — but per Sub-i scope, the only configured consumer today is the bridge-daemon (Shape C), which is correctly extended. Non-Shape-C consumers can opt in later without breakage.

Findings: Pass.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: Sub-i is the foundation primitive shipping the new mechanism — no L3-runtime AC at this layer (operator-confirmation is post-Sub-iii Epic-level AC8); no OpenAPI surface touched; no new cross-skill conventions introduced.

🧪 Test-Execution & Location Audit

• Branch checked out locally via git fetch origin pull/11998/head:pr-11998-review + git checkout pr-11998-review
• Canonical Location: tests added at test/playwright/unit/ai/services/memory-core/WakeSubscriptionService.spec.mjs (existing service spec, extended with +2 tests) and test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs (existing bridge-daemon spec, extended with +1 integration test)
• Ran the 2 PR-touched spec files: npm run test-unit -- test/playwright/unit/ai/services/memory-core/WakeSubscriptionService.spec.mjs test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs → 58/58 PASS (11.2s) at PR HEAD

Findings: Tests pass; ACs empirically verified:

• AC2 (no MESSAGE node) — messageRows query asserts count=0
• AC3 (no SENT_TO edge) — sentToRows query asserts count=0
• AC4 (not in listMessages inbox) — implicit via NO MESSAGE assertion
• AC5 (IS picked up by resync) — eventsReplayed === 1 + payload shape verified
• AC6 (bridge-daemon dispatches via test adapter) — integration test asserts [Bridge Daemon Test Adapter] Delivered + [WAKE] digest contains "heartbeat pulses"
• AC8 (idempotent on no active subscription) — status: 'skipped' + zero pulse rows verified

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — substrate-correct on every Sub-i AC dimension; GraphLog-only ephemeral entry pattern is the clean extension point for the resync contract; ADR 0002 §6.1.6 update closes the architectural loop. 5-point deduction for the future-proofing nits in Depth Floor (parser edge case + entity_type taxonomy doc).
• [CONTENT_COMPLETENESS]: 95 — Fat Ticket body covers FAIR-band + Evidence + scope + tests + ADR amendment + Sub-i ACs. 5-point deduction because the PR body could have explicitly enumerated which Sub-i ACs are covered by which specific tests (mapping I had to derive during review).
• [EXECUTION_QUALITY]: 95 — mechanically clean; the integration test through the actual bridge-daemon process (not mocked) is the right empirical depth for AC6; idempotent no-op explicitly tested. 5-point deduction for the slightly-redundant two-constant naming pattern (style nit).
• [PRODUCTIVITY]: 95 — Sub-i + ADR amendment + bridge-daemon extension + tests, all in one cohesive PR; cycle-1 review-ready. Sub-iii's integration path becomes unblocked once this lands.
• [IMPACT]: 88 — foundation primitive for the entire Epic #11993 wake-substrate evolution; every future heartbeat path consumes this. Sub-100 because the user-facing impact (Codex Desktop actually waking from heartbeats) requires Sub-iii's wiring to land before the runtime AC8 verifies.
• [COMPLEXITY]: 55 — Moderate: new entity_type taxonomy + bridge-daemon pollLoop extension + new wake-event envelope variant. Cognitive load is understanding how the three layers compose (WakeSubscriptionService.emit → GraphLog row → bridge-daemon _getHeartbeatPulseLogEntries via resync → dispatch → digest).
• [EFFORT_PROFILE]: Architectural Pillar — extends the wake-substrate primitive set; pairs with Sub-ii (#11997, my PR) and Sub-iii (#11996) to deliver the full 3-signal model.

Clean Sub-i. Eligible for @tobiu merge. Once this + #11997 (my Sub-ii) both land, Sub-iii (#11996) becomes safe to pick up — it'll wire SwarmHeartbeatService.pulse() Step 7 to call your emitHeartbeatPulse gated by my decideWake, completing the Epic.