PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Approve because the patch implements the narrower converged fix: let userDataDir use its live getInstancePid proof, while preserving HARNESS_PRESENCE freshness for address types that do not have an equivalent dispatch-time liveness oracle.

Peer-Review Opening: Peer review of PR #12576 at exact head 0990bac978fa1b586bf24cc22f2abfe1462612ab. This body update corrects the prior evidence wording after verifying that GitHub attached the approval to the boundary-test head.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #12571 body and Contract Ledger, PR changed-file list, current bridge daemon dispatch source, instanceResolver precedent from the prior #12563 review context, branch commit body, delta b68c39634..0990bac97, local exact-head checkout, related daemon unit tests, and live CI state.
• Expected Solution Shape: The correct patch should relax the stale-presence veto only where a stronger live process proof exists, i.e. userDataDir -> getInstancePid. It must not generalize the relaxation to pid, tmuxSession, or webhookUrl, and tests should isolate live-target, dead-target, and unchanged non-userDataDir behavior.
• Patch Verdict: Matches and improves the reviewed shape. deliverDigest exempts only addressType === 'userDataDir' from assertFreshTargetPresence; the later getInstancePid branch still fails closed when no process maps to the address; exact head 0990bac97 adds the explicit stale-pid boundary test proving the relaxation did not leak beyond userDataDir.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12571
• Related Graph Nodes: HARNESS_PRESENCE, WakeSubscriptionService, bridge-daemon, getInstancePid({userDataDir}), Epic #12416, PR #12570, PR #12563

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Documented search: I actively checked the production guard delta, the added stale-pid boundary test, the branch commit close-target, the Contract Ledger mapping, and live CI, and found no code-level concerns.
• Non-blocking evidence hygiene: The PR body still under-describes the final test surface as two new cases / 22 passed, while exact head now contains three #12571 cases and the related daemon file has 23 tests. This is not a blocking safety issue because it underclaims coverage rather than overstating behavior, and this updated review records the exact evidence.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: core framing matches the implementation; it claims a userDataDir-only freshness exemption, not a broad target-address relaxation.
• Anchor & Echo summaries: daemon docstring accurately scopes the exemption to userDataDir and names the retained target types.
• [RETROSPECTIVE] tag: N/A; none added.
• Linked anchors: #12571 establishes the stale-presence problem and the getInstancePid({userDataDir}) candidate liveness signal.

Findings: Pass, with the non-blocking evidence-hygiene note above.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A
• [TOOLING_GAP]: Local shared-checkout daemon tests can be poisoned by ignored ai/config.mjs drift; exact-head review used /private/tmp/neo-pr-12576-review with template-derived config to isolate the PR.
• [RETROSPECTIVE]: For address-specific wake routing, HARNESS_PRESENCE is a freshness overlay, not always the strongest liveness proof. userDataDir has a dispatch-time OS-process oracle; target types without such an oracle still need the freshness guard.

🎯 Close-Target Audit

For every issue named as close-target, verify it does NOT carry the epic label:

• Close-targets identified: #12571
• #12571 labels verified as bug, ai, architecture; no epic label.
• Branch commit body checked at 0990bac97: single Resolves #12571; no stale extra close-target keyword.

Findings: Pass

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the ledger: HARNESS_PRESENCE.lastSeenAt remains the freshness source for non-userDataDir; getInstancePid({userDataDir}) becomes the authoritative liveness signal for userDataDir dispatch.

Findings: Pass

🪜 Evidence Audit

• PR body contains an Evidence: declaration line via the Test Evidence section.
• Achieved evidence is appropriate for the close-target: local exact-head daemon tests cover live userDataDir, dead userDataDir, and stale-pid boundary behavior; live idle >5m same-app delivery remains correctly listed as post-merge validation.
• Evidence-class collapse check: review language keeps local unit evidence separate from post-merge live harness validation.

Findings: Pass

📡 MCP-Tool-Description Budget Audit

• No OpenAPI/MCP tool descriptions touched.

Findings: N/A

🔗 Cross-Skill Integration Audit

• No skill files or workflow substrate changed.
• No AGENTS_STARTUP.md update needed.
• No new MCP tool added.
• The behavior change is documented at the code boundary: daemon guard comment and assertFreshTargetPresence docstring.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 0990bac978fa1b586bf24cc22f2abfe1462612ab in /private/tmp/neo-pr-12576-review.
• Canonical Location: modified test remains in test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs, the existing right-hemisphere daemon test location.
• Specific test file run in isolated worktree: npm run test-unit -- test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs produced 22 passed and 1 sandbox-only localhost bind failure (listen EPERM for the pre-existing webhookUrl test).
• Sandbox-failed local-bind test rerun escalated: npm run test-unit -- test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs -g "addressType webhookUrl" -> 1 passed.
• git diff --check origin/dev...origin/pr/12576 clean.
• Branch freshness verified: merge-base origin/dev origin/pr/12576 == origin/dev.
• Live PR checks at update time: unit, integration-unified, lint-pr-body, lint-pr-review-body, check, and CodeQL green; mergeStateStatus: CLEAN.

Findings: Related local tests pass with sandbox caveat isolated; CI is green.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 - I actively considered broad target-address relaxation, presence-heartbeat overreach, and default-instance regression, and confirmed none apply.
• [CONTENT_COMPLETENESS]: 90 - 10 points deducted because the PR body undercounts the final test surface after the boundary-test fixup, but the code comments/docstring and review evidence are exact and the PR body does not overstate delivered behavior.
• [EXECUTION_QUALITY]: 100 - I actively considered stale-presence leakage to pid, live/dead userDataDir behavior, local sandbox listener failure, branch freshness, and CI status; exact-head local tests plus live CI confirm none block.
• [PRODUCTIVITY]: 100 - I actively considered each #12571 AC: idle userDataDir wake path, default/no-address path, dead-instance fail-closed behavior, and unit coverage; the patch satisfies them.
• [IMPACT]: 80 - This restores targeted wake reliability for same-app named instances after the five-minute presence window, a high-impact wake-substrate fix.
• [COMPLEXITY]: 35 - Low-to-moderate: one dispatch guard plus focused integration tests, but the correctness depends on distinguishing freshness overlay from live OS-process liveness.
• [EFFORT_PROFILE]: Quick Win - Small code delta with high operational ROI for Vega/Claude targeted wakes.

Approved on exact-head code, local evidence, and green CI. Human-only merge gate still applies; I did not merge.