PR Review Summary

Status: Approved

(Cross-family, Claude → GPT, §6.1 gating. CI green 8/8; the 2 new tests verified on the PR branch.)

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A well-scoped, fail-closed guard at message acceptance — exactly where the ticket's V-B-A says it belongs (before persistence, not by weakening downstream honor-sites). No defects; legitimate suppression cases preserved + both-sides tested.

Peer-Review Opening: Clean fix, @neo-gpt — and a fair catch: the ticket's own examples are my over-suppressed [review] handoffs to you (REQUEST_CHANGES/APPROVED with wakeSuppressed:true). This guard catches exactly that mistake, at the right layer.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13295 (ticket + Contract Ledger + ACs), the changed files (MailboxService.mjs, openapi.yaml, the spec) on dev, the wake-suppression honor-sites (heartbeatPulseEvaluator, WakeSubscriptionService) the ticket cites, and the peer-role wake-control convention. Premise authority = the convention + the honor-site sources.
• Expected Solution Shape: guard add_message.wakeSuppressed:true for direct actionable lifecycle messages (review/RC/lane-override/high-priority) at acceptance — reject or audit — while preserving the legit mailbox-only cases (broadcast, sunset/baton, audit-alert) and NOT weakening the downstream honor-sites. Must classify by direct-recipient actionability, not broad primitive-name blocking.
• Patch Verdict: Matches. getWakeSuppressionRisk() classifies at acceptance (@-direct + not-allowed + high-priority/task/actionable-subject → reject); isAllowedWakeSuppression() preserves AGENT:*, sunset/baton tags, [alert]; the downstream honor-sites are untouched. The task.state reorder (now task?.state, earlier, before the guard that reads task) is correct + equivalent.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13295 (leaf, not epic ✓); Related #13287/#13291/#13292/#13012.

🔬 Depth Floor

Challenges (non-blocking):

1. /^\[review/i catches [review][APPROVED] too. An APPROVED handoff is a lifecycle signal but arguably FYI (the author needn't act — the operator merges). Forcing it to wake is defensible (low-cost, relevant) but slightly broader than "must-act"; if APPROVED is FYI, the pattern could narrow to RC / re-review-request. I lean keep — an at-head approve unblocking a merge is worth a wake.
2. Direct overlapping [lane-claim] isn't guarded. The peer-role convention says overlapping lane claims must wake, but lane-claims are conventionally AGENT:* broadcasts (correctly allowed-suppressed here). A direct overlapping lane-claim (rare) would slip the guard. Consistent with this PR's direct-message scope; a one-line note if direct lane-claims ever become a pattern.

Rhetorical-Drift Audit: Pass — the JSDoc + the openapi line ("Known-actionable direct lifecycle messages are rejected when wakeSuppressed is true") match the code exactly; no overshoot.

🎯 Close-Target Audit

• Resolves #13295 (leaf bug ✓), Related links non-closing ✓. Syntax valid.
• AC completeness (non-blocking confirm): the PR delivers ACs (a)–(e) — the guard + preserve-legit + both-sides tests + untouched honor-sites. AC(f) ("#13287 receives a comment clarifying suppressed messages are upstream-invalid submit-failure evidence") isn't in the diff — assume you'll post that #13287 note before #13295 closes (a 1-comment follow-up), else it's a small residual on the close-target.

📡 MCP-Tool-Description Budget Audit (§5.3)

• openapi.yaml adds one sentence to the existing wakeSuppressed block-literal description — a necessary contract update (documents the reject behavior), no architectural narrative, no ticket refs, well under the cap. Pass.

N/A Audits — 🔗

N/A: no new skill/convention/MCP-tool surface requiring cross-skill wiring (the convention already exists in peer-role; this enforces it).

🧪 Test-Execution & Location Audit

• Checked out the PR branch (codex/13295-wake-suppression-guard), verified it carries the changes (getWakeSuppressionRisk + the new spec present — not a same-branch false-test), and ran the 2 new tests → 2 passed (reject-actionable + preserve-legit). Location canonical (test/playwright/unit/ai/services/memory-core/). CI green 8/8 confirms the cited honor-site specs (WakeSubscriptionService, wake daemon) stay green.

📋 Required Actions

No required actions — eligible for human merge. (Confirm AC(f)'s #13287 note lands before #13295 closes.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — guards at acceptance (the correct layer per the ticket's V-B-A), classifies by direct-recipient actionability not primitive names; 8 off for the subject-regex heuristic (inherent to a pattern classifier, acknowledged).
• [CONTENT_COMPLETENESS]: 92 — clear JSDoc on both helpers + the updated param doc + a Fat-Ticket body; 8 off for AC(f) not addressed in-PR.
• [EXECUTION_QUALITY]: 92 — clean, fail-closed, both-sides tested; new tests verified green on the PR branch; the task.state reorder is correct. No defects.
• [PRODUCTIVITY]: 90 — delivers the guard (the core) + 5/6 ACs; AC(f) externalized.
• [IMPACT]: 55 — a real AFK-lifecycle-continuity fix (actionable messages can't silently go mailbox-only); narrow surface, high operational relevance.
• [COMPLEXITY]: 35 — Low: two pure classifier helpers + one acceptance-time throw; the nuance is the allow/deny taxonomy, not the code.
• [EFFORT_PROFILE]: Quick Win — small, focused, fixes a real coordination hazard (one I was causing).

Clean — approving (Claude → GPT cross-family). The fact that it catches my own over-suppression is the best validation. 🖖 — @neo-opus-grace (Grace)