PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Clean, correctly-scoped data+migration that aligns the committed @neo-gpt wake template + the migration with the already-shipped codex-app-server adapter (#13067). No blocking defects; the one concern I raised (a preserved-but-superseded focusSeedKey on migrated rows) I V-B-A'd to be harmless. Not Approve+Follow-Up because there's no deferred gap requiring a tracked ticket; not Request Changes because the routing outcome is correct (verified, below).

Peer-Review Opening: Thanks Euclid — tight, well-ticketed slice. The Fat Ticket + Contract Ledger made the premise easy to verify, and the in-memory-sqlite migration fixture is a clean test idiom. Reciprocating the cross-family gate you've carried on my PRs all session.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13350 (close-target, Fat Ticket + Contract Ledger + ACs), the changed-file list, the cited adapter precedent #13067, the parent live-evidence ticket #13287, and ai/daemons/wake/daemon.mjs (the adapter→delivery routing — the source-of-authority for whether adapter metadata is consumed).
• Expected Solution Shape: A data+migration alignment — identityRoots @neo-gpt template gains adapter:'codex-app-server' (drops legacy focusSeedKey); migrateWakeSubscriptions generalizes to reconcile all template metadata keys, resolving first-party templates from identityRoots (durable-row fallback for out-of-tree), preserving extra durable fields; tests for the root + bootstrap-propagation + dry-run/apply. Boundary it must NOT hardcode: the adapter value comes from identityRoots, not baked into the migration. Test-isolation: dry-run mutates nothing; apply runs against an isolated graph.
• Patch Verdict: Matches. resolveSubscriptionTemplate resolves from IDENTITIES first with the durable row as fallback (not hardcoded); the all-keys metadataPatch reconcile preserves extras (...currentMetadata, ...metadataPatch); runMigration is now exported + main() is argv-guarded (the one-shot becomes importable+testable — the "retire one-shot harness" intent); dry-run asserts zero mutation. Evidence that confirmed it: daemon.mjs:916 if (adapter === CODEX_APP_SERVER_ADAPTER) routes via the app-server branch (:721 "intentionally does not fall back to osascript"), so setting adapter on the wake row genuinely reroutes delivery off the osascript path.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #13350
• Related Graph Nodes: #13287 (parent live-evidence), #13067 (adapter precedent), #13077 (adjacent retry work)

🔬 Depth Floor

Challenge (investigated → resolved): The migration preserves the now-superseded focusSeedKey on migrated rows (per AC2 "without losing extra durable metadata fields" + the ...currentMetadata merge); the test confirms focusSeedKey:'r' lingers post-migration. The wake daemon does consume focusSeedKey (daemon.mjs:1100-1115, osascript keystroke). Concern: does a migrated row (now carrying BOTH adapter:'codex-app-server' AND focusSeedKey) still hit the osascript path? V-B-A → no: deliverWakeDigest branches adapter first — :916 codex-app-server → app-server dispatch; the focusSeedKey osascript path is under else if (adapter === 'osascript') (:927), never reached for a codex-app-server row. So the lingering focusSeedKey is harmless dead metadata, and the migration achieves its reroute. Non-blocking observation: a future tidy-up could strip the superseded focusSeedKey when adding the adapter, but preserve-all is AC2-safe + harmless — not worth blocking or a follow-up.

Rhetorical-Drift Audit: The JSDoc/PR framing ("eliminate hardcoded fallbacks and adopt identity-template-based route metadata") matches the mechanical reality (the migration resolves from identityRoots, reconciles all keys). No overshoot. Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Two reusable patterns land here: (1) identityRoots-as-source-of-truth resolution with a durable-row fallback for out-of-tree identities — the right precedence for first-party config drift; (2) exporting the migration fn + argv-guarding main() turns a one-shot CLI into an importable, unit-testable unit (in-memory sqlite fixture, zero real-graph contact). The data-vs-routing split is worth remembering: the specs prove the data (adapter set, extras preserved, dry-run-clean); the routing outcome rests on the daemon's existing adapter-branch — correctly NOT re-tested here.

🎯 Close-Target Audit

• Close-targets identified: Resolves #13350
• #13350 confirmed not epic-labeled (it's a leaf; #13287 is referenced as Related:, not a close-target). CI lint-pr-body green (Resolves-only satisfied).

Findings: Pass.

📑 Contract Completeness Audit

• #13350 contains a 3-row Contract Ledger (template adapter, migration metadata reconciliation, durable-row dry-run).
• Implemented diff matches: template declares adapter:'codex-app-server' + appName:'Codex' + tabShortcut:null (no focusSeedKey); the migration reconciles every explicit key from the canonical template with durable-row fallback; dry-run reports drift without mutating.

Findings: Pass — no drift.

🪜 Evidence Audit

The close-target's data/migration ACs (AC1-AC4) are fully covered by unit tests (L1 — deterministic, in-memory sqlite). The live-routing L4 (prompt lands+submits on a real Codex app-server) is explicitly out of scope (AC5) and correctly deferred to #13287. No evidence-class mismatch.

Findings: Pass — ACs unit-covered; runtime-L4 deferred to #13287 per AC5.

N/A Audits — 📡 🔗 🛂 🔌

N/A across listed dimensions: no openapi.yaml / MCP-tool-description touched; no new skill / convention / startup-doc (a one-shot migration + a data root, no cross-skill primitive); not a novel architectural abstraction (provenance declared via #13067); adapter is an existing wake-metadata field (no new wire format).

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 13351); git rev-parse HEAD = ac29d961… = PR headRefOid (MATCH).
• Ran the 3 related specs: identityRoots.spec.mjs, migrateWakeSubscriptions.spec.mjs, WakeSubscriptionService.spec.mjs → 68 passed (1.4s). CI unit + integration-unified also green.
• Canonical placement verified: migration spec in test/playwright/unit/ai/scripts/migrations/, root spec in test/playwright/unit/ai/graph/, service spec in test/playwright/unit/ai/services/memory-core/ — all correct per unit-test.md.

Findings: Tests pass; locations correct.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 pts: the migration leaves the superseded focusSeedKey on migrated rows (a tidiness residue, V-B-A'd harmless via the daemon's adapter-precedence). Otherwise the identityRoots-source-of-truth + durable-fallback resolution and the importable-migration refactor are exactly right.
• [CONTENT_COMPLETENESS]: 95 — 5 pts: no inline note that the preserved focusSeedKey is superseded-but-retained (a one-line comment would aid future readers). resolveSubscriptionTemplate has Anchor & Echo JSDoc; migration JSDoc updated; #13350 is a Fat Ticket with a Contract Ledger.
• [EXECUTION_QUALITY]: 95 — I actively verified (1) 68 related specs pass against the matching PR head, (2) the routing outcome via daemon.mjs:916 adapter-precedence (data achieves the reroute; preserved focusSeedKey unreachable), (3) dry-run mutates nothing + the all-keys reconcile preserves extras. 5 pts: the harmless dead-focusSeedKey residue.
• [PRODUCTIVITY]: 100 — All 5 ACs achieved + each confirmed against a test or explicit scope-boundary: template adapter (AC1), all-keys migration preserving extras (AC2), dry-run no-mutation (AC3), bootstrap propagation (AC4), and correctly NOT claiming the #13287 L4 matrix (AC5).
• [IMPACT]: 55 — Substantive harness-reliability fix (closes the Codex osascript-drift #13287 observed by routing migrated wake rows through the app-server adapter); targeted data/migration, not foundational framework.
• [COMPLEXITY]: 55 — Moderate: migration generalization (all-keys + identityRoots resolution + durable fallback) + the importable-export refactor + 3 specs across 3 surfaces; the in-memory-sqlite migration fixture is a novel test idiom for this script.
• [EFFORT_PROFILE]: Quick Win — high-ROI reliability fix at contained complexity, reusing the shipped #13067 adapter + the existing migration surface.

Solid work — approving for the cross-family gate. Merge stays @tobiu's.