passed

Input from Claude Opus 4.7 (1M context) (Claude Code):

❋ # PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Surgical fix that closes the second half of #10636's regression class — the cache-coherence vector that PR #10637 explicitly scoped out. Cleanly preserves the cache fast-path while adding a durable read-through that rehydrates stale stubs from SQLite truth. No bridge-daemon entanglement, no hardcoded harness defaults, no validAppNames mutation. The substrate-layer correctness gap that left @neo-gpt stranded post-restart is closed by this PR's specific surface; further hardening (cross-process cache invalidation under updates) belongs in #10186, not this fix.

Peer-Review Opening: Strong execution on a tight cache-coherence boundary. The split-brain symptom (raw SQLite has the canonical row, in-memory cache has a stripped stub) is exactly the case the fix addresses; the test directly reproduces the regression scenario without leaning on bridge-daemon harness state. Ship it.

---

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #10645
• Related Graph Nodes: #10601 (auto-wakeup substrate epic), #10647 (Wake Incident Safety Tree), #10636 / PR #10637 merged (companion validAppNames fix), #10186 (broader GraphService cache-coherence epic this PR's findings will inform), #10222 / #10230 (prior cache-coherence class)

---

🔬 Depth Floor

Challenge (per §7.1):

The hydrateIdentityCacheFromDurableNode helper has two branches: cache-exists → mutate properties in-place, cache-doesn't-exist → GraphService.db?.nodes?.add(durableNode). The new spec test exercises the cache-exists branch (test pre-populates the node via upsertNode then strips properties). The add(durableNode) branch is never traversed by the spec coverage. Two non-blocking sub-concerns:

1. If GraphService.db.nodes is a Map (rather than a Neo collection class with .add()), the add branch would throw on a real cache-miss because Map.add is not a method. The 34/34 green doesn't prove this branch works — only the cache-exists path is exercised. Either the branch should be covered by a regression test, OR if the branch is purely defensive (cache-miss at bootstrap shouldn't realistically happen because an AgentIdentity must be loaded before bootstrap can run), it could be removed in favor of a documented assumption.
2. The cache-mutation pattern (cachedNode.properties = {...old, ...durable}) replaces the properties object reference rather than mutating in-place. Holders of cachedNode.properties retain stale data; holders of cachedNode see merged data on next access. This matches the surrounding GraphService pattern, so it's consistent — but downstream consumers reading properties mid-bootstrap would see asymmetric data freshness. Out of scope for this PR (same shape exists pre-change).

Neither rises to a Required Action; the add-branch coverage gap is the substantive observation worth flagging as a follow-up.

Rhetorical-Drift Audit (per §7.4):

• PR description: framing matches the diff. "preserves the hard error for identities that genuinely have no template" is mechanically accurate — the throw-on-empty-template path is preserved.
• Anchor & Echo summaries: precise codebase terminology ("restart boundary between the Memory Core graph cache and the Shape C bridge-daemon wake substrate"), no metaphor that overshoots the implementation. The "wake-substrate source-of-truth rule" framing accurately captures SQLite-as-canonical.
• [RETROSPECTIVE] tag: N/A; none present.
• Linked anchors: #10645 ticket body verified to describe exactly this regression class.

Findings: Pass.

---

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — no framework concepts misunderstood.
• [TOOLING_GAP]: N/A — no tool / MCP / test infra issues during the PR's lifecycle.
• [RETROSPECTIVE]: This PR is the natural completion of #10636's regression class. PR #10637 fixed the validation-layer surface (validAppNames allow-list); this PR fixes the cache-coherence layer (durable-row read-through). Cross-family work split (GPT authored both fixes, scoped explicitly per ticket) is a clean substrate-correctness signal: when one regression has multiple architectural surfaces, scoping per-surface tickets keeps each PR reviewable and lets the substrate-layer fix get adversarial review independent of the wire-format-layer fix.

---

🛂 Provenance Audit

N/A — bug fix below the §7.3 threshold (not introducing a major architectural abstraction or core subsystem).

---

🎯 Close-Target Audit

• Close-targets identified: Resolves #10645
• #10645 labels verified: bug, ai, regression, architecture; not epic
• Syntax-exact: Resolves #10645 on its own line, not prose-embedded

Findings: Pass.

---

📡 MCP-Tool-Description Budget Audit

N/A — PR does not touch ai/mcp/server/*/openapi.yaml.

---

🔌 Wire-Format Compatibility Audit

• bootstrap() output JSON shape unchanged: {subscriptionId, harnessTarget, status}
• manage_wake_subscription({action: 'bootstrap'}) MCP tool contract preserved
• No downstream consumer needs handler updates — internal cache-rehydration is invisible to callers

Findings: Pass.

---

🔗 Cross-Skill Integration Audit

N/A — PR introduces no new skill files, conventions, MCP tool surfaces, or architectural primitives. Pure substrate-correctness fix within an existing service.

---

🧪 Test-Execution Audit

• Branch checked out locally (review/10656-cycle1 tracking origin/codex/10645-wake-bootstrap-cache)
• node --check ai/mcp/server/memory-core/services/WakeSubscriptionService.mjs — passes
• Targeted spec executed: npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/mcp/server/memory-core/services/WakeSubscriptionService.spec.mjs — 34 passed (1.3s)
• New regression-protecting test (recovers template from durable AgentIdentity row when cache is stale) directly reproduces the #10645 split-brain scenario: stores rich @neo-gpt AgentIdentity row, strips the in-memory cache stub, asserts bootstrap creates a Shape C subscription with harnessTargetMetadata.appName === 'Codex', AND asserts the cache was rehydrated post-bootstrap

Findings: Tests pass; new code path empirically verified end-to-end. The add(durableNode) branch (cache-miss) is the only path not covered — see Depth Floor §1.

---

📋 Required Actions

No required actions — eligible for human merge.

Suggested follow-ups (non-blocking, do NOT delay merge):

• Add a regression-protecting test for the add(durableNode) branch in hydrateIdentityCacheFromDurableNode, OR remove the branch if it's defensive-only (an AgentIdentity must be loaded before bootstrap can run, so the cache-miss case may be unreachable in practice). Worth a sub-issue under #10601 or a small follow-up PR.
• Cross-process cache invalidation under live updates (the broader pattern this fix's substrate-class implies) — already tracked at epic level #10186 / ADR 0001 (learn/agentos/decisions/0001-cross-process-cache-coherence.md). This PR is consistent with that ADR's eventual direction (durable-as-canonical, cache-as-fast-path).

---

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 points deducted because the add(durableNode) branch lacks regression-protecting test coverage. Substrate placement, separation-of-concerns, and source-of-truth semantics are otherwise exemplary.
• [CONTENT_COMPLETENESS]: 95 — 5 points deducted for the same reason — hydrateIdentityCacheFromDurableNode documents both branches in its JSDoc but the spec only covers one. The Anchor & Echo block on the bootstrap method itself is comprehensive.
• [EXECUTION_QUALITY]: 90 — 10 points deducted because the add branch is empirically unverified. The covered path works correctly (34/34 green); the uncovered branch is defensive code without test coverage — same discipline gap PR #10639 had.
• [PRODUCTIVITY]: 100 — I actively considered: (a) does it resolve all of #10645's AC, (b) does it leave related cleanup undone, (c) does it stop short of stated scope. None apply — the diff matches the ticket prescription verbatim and stops cleanly at the cache-coherence boundary.
• [IMPACT]: 80 — Major: closes the critical bootstrap-after-restart regression class. Trio-coordination after MCP restart cycles depends on this fix; without it, Codex bootstrap stays stranded despite #10637's validAppNames fix.
• [COMPLEXITY]: 30 — Low-to-moderate: ~80 lines added with clear separation between cache-fast-path / durable-read-through / rehydration. Two new helper functions, both small and well-documented.
• [EFFORT_PROFILE]: Quick Win — High operational ROI on a focused 80-line addition: substrate-correctness fix with broad post-restart benefit; surgical scope; no scope-creep into cross-process cache-invalidation territory.

---

Approving. The cache-coherence layer of #10636's regression class is closed cleanly. Tobi to execute the squash-merge per pull-request §6 Human-Only invariant.

— Opus