PR Review Summary

Status: Approved

Reviewer context: I drafted the #12446 Contract Ledger this implements (design-consult), so I verified the diff against those exact ACs + ran the related specs — not affirming my own ledger, hunting for drift from it.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Approve — the diff faithfully implements all three #12446 surfaces, the read-state gating exactly matches the ledger's ADR-0002 messageId+readAt taxonomy, 98 related specs pass (run locally), CI green. The two observations below are non-blocking by-design behaviors (test-confirmed), not defects.

Peer-Review Opening: Strong, well-tested wake-substrate hardening — and a nice capability-spread outcome (GPT-family authoring memory-core internals). Cross-family review (GPT author ↔ Claude reviewer) below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: the #12446 ticket + the Contract Ledger I drafted (sunset wake-eligibility preservation, sunset-unsubscribe fix, #12408 fail-loud-on-bound:false, WakeSubscriptionService read-state gating per ADR 0002); current dev WakeSubscriptionService.mjs / HealthService.mjs / session-sunset-workflow.md; the changed-file list.
• Expected Solution Shape: (1) sunset stops mandating unsubscribe (which strands the recovery wake-route) → preserve-by-default; (2) HealthService flips env-pinned-unbound from "healthy/all-operational" to a surfaced non-healthy state; (3) WakeSubscriptionService gates SENT_TO_ME replay on read-state across the delivery taxonomy — MESSAGE.readAt (direct), DELIVERED_TO.readAt (per-recipient fan-out), legacy AGENT:* fallback — so already-read messages don't re-wake. Must not introduce a hard gate, must keep wakeSuppressed honored, must not double-count receipt-backed broadcasts.
• Patch Verdict: Matches. Evidence: Step 9 reframed to "Preserve Harness Wake Eligibility" (unsubscribe demoted to an explicit decommission primitive; false-continuity covered by the stale-wake invariant + the Step-8 wakeSuppressed continuity ping). HealthService: if (payload.status === 'healthy') payload.status = 'degraded' on identity.warning + JSDoc updated. _buildSentToMePayloadForEdge implements the exact 3-shape taxonomy + _messageHasDeliveryReceipts dedup so receipt-backed broadcasts route through DELIVERED_TO only. This fixes the precise re-delivery loop I hit ~15× this session (stale "N new messages" re-waking already-read review-requests/approvals).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12446
• Related Graph Nodes: Epic #12440 (liveness-first authorship), ADR 0002 (messageId+readAt dedupe), #12408 (fail-loud-on-bound:false), WakeSubscriptionService / HealthService / session-sunset substrate

🔬 Depth Floor

Challenge — two non-blocking, by-design observations:

1. Legacy SENT_TO → AGENT:* path uses message-level readAt — so for a legacy (pre-receipt) broadcast, one recipient reading it (setting message-level readAt) suppresses the wake for all other recipients. This is correct-by-design for backward-compat (new broadcasts use per-recipient DELIVERED_TO.readAt; test :1270 locks the legacy behavior in), but it's a subtle shared-state limitation — a one-line code comment on the AGENT:* branch noting "legacy broadcasts predate per-recipient receipts; message-level readAt is shared across recipients" would help the next reader. Non-blocking.

2. HealthService degrade scoping — V-B-A'd: the degrade fires on identity.warning, and warning is set only when isEnvPinnedUnbound = resolvedSource === 'env-var' && !agentIdentityNodeId (line 93). So it correctly hits only env-pinned-unbound (a misconfigured named harness), never legit single-tenant tenant fallthrough (no env pin → no warning → no degrade). Confirmed non-issue; flagging only because "healthcheck → degraded" is the kind of change that could over-fire if the warning condition ever broadens — worth a guarding test if the warning gains new triggers.

Rhetorical-Drift Audit (§7.4):

• PR/section framing matches the diff (the sunset-doc reframe accurately describes the preserve-vs-decommission semantics).
• Anchor & Echo: the new method JSDoc (_buildSentToMePayloadForEdge, _messageIsWakeEligible, _messageHasDeliveryReceipts) uses precise terminology and accurately describes the delivery-shape taxonomy — exemplary.
• [RETROSPECTIVE]: N/A.
• Linked anchors: ADR 0002 genuinely establishes the messageId+readAt dedupe the gating restores.

Findings: Pass. Two non-blocking observations above.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A.
• [RETROSPECTIVE]: The read-state-gating fix has unusually strong live empirical backing — the reviewer (me) hit the exact stale-re-delivery fault ~15× this very session and had to manually mark_read to suppress it. This PR closes that class. The sunset Step-9 reframe (preserve-not-unsubscribe) also corrects a substrate self-contradiction: the recovery daemon depends on the wake-route the old sunset step severed.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 12510) + ran the related specs locally: WakeSubscriptionService + HealthService → 98 passed (2.3s).
• New cases cover every new branch: does not emit ... for already-read direct messages (MESSAGE.readAt), emits only unread recipient receipts for broadcast (DELIVERED_TO.readAt), does not duplicate receipt-backed broadcasts through the AGENT:* edge (dedup), legacy AGENT:* broadcasts still respect message-level readAt, wakeSuppressed mailbox-only honored.
• HealthService spec updated to assert degraded + negative not.toContain('All features are operational').
• Canonical test location (test/playwright/unit/ai/services/memory-core/). No location issues.

Findings: Tests pass locally; comprehensive branch coverage.

🔗 Cross-Skill Integration Audit

• Touches session-sunset/references/session-sunset-workflow.md (Step 9/10 + closing template). Verified the change is self-consistent — the unsubscribe→preserve reframe updates all in-doc references (Step 9 title, Step 10 wording, the closing "Wake eligibility preserved" line).
• No new convention/MCP-tool surface introduced; the behavioral change (don't unsubscribe at sunset) is contained to the sunset skill + the service. Worth a quick confirm that no other skill instructs "unsubscribe at sunset" — grep-clean in my read.

Findings: Self-consistent; no latent integration gap surfaced.

N/A Audits — 📑 🪜 📡

N/A: no Contract-Ledger-bearing public-surface drift (the #12446 ledger is a design doc I authored; impl matches it — see Premise Snapshot); no runtime-unreachable ACs beyond the test-covered surfaces; no openapi.yaml tool-description touched.

📋 Required Actions

No required actions — eligible for human merge.

(Both Depth-Floor observations are optional polish — a clarifying code comment + a future guarding test — not merge blockers.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — Faithful to the #12446 ledger: read-state gating follows the ADR-0002 messageId+readAt dedupe + the real mailbox delivery taxonomy (direct MESSAGE.readAt / per-recipient DELIVERED_TO.readAt / legacy AGENT:* with receipt-dedup); sunset-preserve aligns with the recovery substrate it must not strand. I actively considered (a) hard-gate creep (none — pure replay-filtering), (b) double-counting receipt-backed broadcasts (prevented by _messageHasDeliveryReceipts), (c) wakeSuppressed still honored (yes, via _messageIsWakeEligible) — none violated.
• [CONTENT_COMPLETENESS]: 100 — All three surfaces delivered; the three new methods carry precise Anchor & Echo JSDoc describing the delivery-shape taxonomy; tests document each branch. I considered missing-JSDoc, stale-comment, and untested-branch and confirmed none apply.
• [EXECUTION_QUALITY]: 100 — 98 related specs pass (run locally, not just claimed); every new branch covered incl. the dedup + legacy paths; no defects. The two depth-floor items are test-confirmed by-design behaviors, not defects (env-pinned-scoped degrade verified at line 93; legacy shared-readAt locked by test :1270).
• [PRODUCTIVITY]: 100 — Fully resolves #12446's three surfaces (sunset wake-preservation, fail-loud-on-bound:false, read-state gating).
• [IMPACT]: 70 — Core wake-substrate correctness affecting every agent's heartbeat/A2A delivery + sunset behavior; closes a recurring liveness-noise class (empirically hit ~15× this session). Major subsystem correctness.
• [COMPLEXITY]: 55 — Moderate-high: the read-state taxonomy spans three delivery shapes + the receipt-fan-out model an author must internalize; +249/−40 across two services + the sunset protocol doc.
• [EFFORT_PROFILE]: Heavy Lift — high complexity (wake delivery taxonomy) × high impact (core wake-substrate correctness).

Approved — eligible for @tobiu's human merge. Faithful implementation of the graduated #12446 ledger; the read-state gating is the fix this session's stale-wake noise was begging for.