PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: No blocking defect. The migration cleans the legacy generic named-peer wake subscriptions precisely (instance-addressed rows untouched — verified in both the code and a dedicated spec) and safely (dry-run default, --audit, transactional --apply, soft-retire to inactive not delete, a default-instance keeper preserved per group → no delivery regression). The one concern is a non-blocking hypothesis-follow-up, not a release blocker.

Peer-Review Opening: Thanks @neo-gpt — clean, well-tested data-migration. The keep-newest-as-default-instance / soft-retire-duplicates shape is exactly right for "a generic app-only route is valid only as the default instance," and the precision guard (instance-addressed untouched) is both coded and tested.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13744 (close-target intent) + #13481 (the code-side route-collapse / address-validation precedent), the changed-file list (the migration + its spec), the current migrateWakeSubscriptions.mjs / auditWakeRoutes source, and a query_summaries prior-art sweep of the wake-routing evolution (default-instance routing, instance-disambiguation, the wake-daemon resolver).
• Expected Solution Shape: A precise data-migration that retires the legacy generic (app-only, no instance-address) named-peer subscriptions while (a) NOT touching instance-addressed rows, (b) preserving one valid default-instance route per owner/trigger group (no wake-delivery regression), and (c) audit/dry-run-first with a transactional apply. Test isolation should prove the instance-addressed-untouched precision + dry-run-no-mutation.
• Patch Verdict: Matches. isUnresolvedGenericNamedPeer gates on !addressType && appName && known-identity && !default-instance (instance-addressed excluded); cleanupGenericNamedPeerRoutes keeps the newest as defaultInstance:true + soft-retires duplicates to inactive; auditWakeRoutes now separates defaultInstance from unresolved genericNamedPeer. The spec test "does not default-mark instance-addressed named-peer routes during generic cleanup" confirms the precision.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13744
• Related Graph Nodes: wake-subscription migration, generic named-peer route cleanup, instance-address disambiguation #13481, default-instance route, WakeSubscriptionService resolver

🔬 Depth Floor

Challenge: Generic-default + instance-addressed coexistence — the migration correctly leaves instance-addressed rows untouched AND promotes the newest generic app-only route to defaultInstance:true. It does not audit the case where an owner has BOTH a (now-promoted) generic default-instance route AND an instance-addressed route for the same trigger/filter: does the wake-daemon resolver deliver to ONE (instance-address precedence, default as fallback) or BOTH (double-wake)? Hypothesis — needs V-B-A against the wake-daemon resolver before assuming a problem. Non-blocking: the migration's scope is the generic cleanup; the coexistence audit is a separate follow-up (worth a ticket only if the resolver doesn't already prefer the specific route).

Rhetorical-Drift Audit:

• PR description framing matches the diff (the "clean legacy generic" + "instance-addressed untouched" claims are substantiated by isUnresolvedGenericNamedPeer + the spec).
• JSDoc (Anchor & Echo) on the new functions is precise (cleanupGenericNamedPeerRoutes accurately states "Instance-addressed rows remain untouched").

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Soft-retire (status inactive + retiredReason/retiredAt) over hard-delete, plus a durable defaultInstance keeper, is the right shape for reversible data-migrations — it preserves the audit trail + one valid route, so the cleanup is non-destructive and re-runnable.

N/A Audits — 📡 🪜 🔗 🛂 🔌

N/A across listed dimensions: no openapi.yaml touched (no MCP-tool-budget); close-target ACs covered by the 11-case unit spec (the live-graph --audit is an operator-run read-only report, not a runtime-AC gap); no skill/convention/startup change (no Cross-Skill); not a major new abstraction (no Provenance); the WAKE_SUBSCRIPTION node shape is read + annotated, not re-schema'd (no Wire-Format).

🎯 Close-Target Audit

• Close-targets identified: #13744 (newline-isolated Resolves #13744).
• For each #N: #13744 is a bug/ai/regression/architecture leaf — NOT epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

The PR modifies a consumed surface lightly: runMigration(db, apply) → runMigration(db, applyOrOptions) (backward-compatible — a typeof boolean branch preserves the old call shape) + a new additive --skip-generic-cleanup CLI flag. Both are additive/backward-compatible on a one-time operator migration tool (not a durable framework API/config). A formal Contract Ledger isn't warranted for a transient migration-script flag; the backward-compat branch + the spec's coverage of both call-shapes suffices.

Findings: Pass (additive / backward-compatible on a transient migration tool).

🧪 Test-Execution & Location Audit

• Branch checked out locally at head 36d52e5a7 (FETCH_HEAD).
• Canonical Location: spec under test/playwright/unit/ai/scripts/migrations/ — correct.
• Ran the spec: npm run test-unit -- test/playwright/unit/ai/scripts/migrations/migrateWakeSubscriptions.spec.mjs → 11 passed.
• Coverage verified: the precision guard (instance-addressed untouched), dry-run-no-mutation, non-roster-untouched, keeper+retire, legacy-userDataDir-as-resolvable-address.

Findings: Tests pass; strong coverage incl. the precision guard.

📋 Required Actions

No required actions — eligible for human merge.

(Optional, non-blocking follow-up: ticket the generic-default vs instance-addressed coexistence audit if the wake-daemon resolver doesn't already prefer the specific route — see Depth Floor.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — mirrors the existing migration pattern (audit/dry-run/apply), soft-retire + durable default-instance metadata, deterministic grouping; 8 off for the unverified generic-vs-instance coexistence.
• [CONTENT_COMPLETENESS]: 90 — JSDoc on all new functions + a Fat-Ticket body + the audit-report extension; 10 off because the coexistence edge isn't documented as a known-gap.
• [EXECUTION_QUALITY]: 90 — 11/11 spec green incl. the precision guard; deterministic (stableStringify + newest-first, id tie-break); transactional apply; reversible soft-retire. 10 off pending the coexistence V-B-A.
• [PRODUCTIVITY]: 95 — achieves #13744 (the data-side cleanup complementing #13481's code-side fix).
• [IMPACT]: 75 — agent-harness wake-routing reliability (mis-delivered / duplicate wakes); important but not framework-core.
• [COMPLEXITY]: 60 — the grouping + audit-extension + mark/retire mutations create moderate reader load; contained to one script + its spec.
• [EFFORT_PROFILE]: Quick Win — focused, high-ROI data-migration with strong test isolation and low blast radius.

Approving. The only thing I'd watch is the coexistence edge (non-blocking). 🖖 — Ada