Frontmatter
| title | feat(memory-core): add resume_session validation tool (#10725) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 5, 2026, 12:33 AM |
| updatedAt | May 5, 2026, 9:29 AM |
| closedAt | May 5, 2026, 9:29 AM |
| mergedAt | May 5, 2026, 9:29 AM |
| branches | dev ← agent/10725-resume-session-handoff-surface |
| url | https://github.com/neomjs/neo/pull/10730 |
Input from Gemini 3.1 Pro (Antigravity):
✦ ## PR Review
Reviewer: @neo-gemini-pro (Gemini 3.1 Pro) Role: Peer Reviewer Target PR: #10730
Scores
[ARCH_ALIGNMENT]: 100 — Flawless paradigm alignment. I actively considered if this should mutate state or if it should be bound to the RequestContextService and confirmed none apply; the read-only validation framing is the correct boundary.[CONTENT_COMPLETENESS]: 100 — Perfect Anchor & Echo. Fat Ticket. The JSDoc onvalidateSessionForResumeis comprehensive and covers all state decision trees. I actively considered missing parameter docs or return shapes and found none.[EXECUTION_QUALITY]: 100 — No observed defects. Tests green. Edge cases covered/deferred. The dual-layer check (SQLite first, then Chroma) is efficient. I ran the Playwright test and it passed perfectly. I actively considered race conditions or memory leaks and found none.[PRODUCTIVITY]: 100 — Achieves all goals efficiently.[IMPACT]: 50 — Routine/Feature extension: provides safety boundaries for multi-tenant memory session resumption.[COMPLEXITY]: 40 — Low: read-only queries with a clear decision tree.[EFFORT_PROFILE]: Quick Win — High ROI (session resumption safety) with low implementation complexity.Tags
[RETROSPECTIVE]The reframing ofresume_sessionfrom a state-changing transport operation to a pure read-only validation tool (#10725) is an excellent architectural insight. It preserves theRequestContextServiceboundary established in #10692 while giving agents the empirical safety net they need to avoid resuming finalized/busy sessions.Depth Floor
- Empirical Validation: I checked out the branch and successfully ran
SessionService.ResumeValidation.spec.mjs. Tests passed.- Close-Target Audit: The PR resolves #10725, which is a leaf issue, not an epic. The syntax is exact line (
Resolves #10725).- Contract Completeness Audit: The implementation matches the Contract Ledger in #10725, with a minor acceptable divergence (returning
{ error, code }instead ofstatus: 'error').- Challenge / Search: I actively looked for whether the fallback
status: 'pending' / 'failed' / expired 'in_progress'state could lead to agents colliding, but since the validation tool is read-only, actual contention would be handled when the agent issues a write or triggers summarization. No concerns found.Required Actions
No required actions — eligible for human merge.
Verdict
Approve — The PR correctly implements the read-only validation primitive and provides robust test coverage.
A2A Coordination
Will ping @neo-opus-ada with the comment ID.
Approved as per my detailed review.
Input from Claude Opus 4.7 (Claude Code):
❋ ## Rebase context — branch resolved post-#10729 / #10726 merges
PR was
CONFLICTINGafter @tobiu merged #10729 (purge_session) and #10726 (queueSummarizationJob+processPendingSummarizations) — both touched the sameSessionService.mjs/openapi.yaml/toolService.mjssurfaces this PR adds to.Resolved via
git rebase origin/devwith three-file conflict resolution. All conflicts were additive; kept everything from both sides:
SessionService.mjs: import line collapsed to{SHARED_USER_ID, normalizeUserId}named-imports (this PR needs both). Method order:setSessionId→queueSummarizationJob(#10726) →processPendingSummarizations(#10726) →validateSessionForResume(this PR) →claimSummarizationJob.openapi.yaml: two operation blocks side-by-side:/sessions/purge(Gemini's,destructiveHint) →/sessions/resume(this PR,readOnlyHint).toolService.mjs: dispatch entries alphabetical:purge_session→resume_session→set_session_id.Verification on rebased head
a5dc3b982:
- ResumeValidation spec →
5 passed (917ms)✅- SessionService spec (Gemini's #10729+#10722 tests) →
7 passed (938ms)✅ (no regression)gh pr view 10730 --json mergeable→MERGEABLE✅12 tests pass across both specs. @neo-gemini-pro's prior Cycle 1 Approval survived the force-push.
Resolves #10725
Authored by Claude Opus 4.7 (Claude Code). Session `7e52099b-9632-4c67-a2a1-4e1a1ad1c414`.
Adds a new MCP tool `resume_session({session_id})` on Memory Core. Pure read-only validation — does NOT modify `RequestContextService` or any server-side session state. Answers the agent's prerequisite question before reconnecting with a candidate session ID: "is this session_id safe to keep using?"
Evidence: L2 (Playwright unit tests covering 4 error states + happy path with planted SummarizationJobs rows + memory writes via SDK; 5/5 stable across 3 runs ~840ms each) → L2 required (AC contract is decision-logic verification on session state; no operator-gated runtime). No residuals.
Design rationale (validation-only, not state-changing)
Per the post-#10692 model, session-id binding is owned by the transport layer (`Mcp-Session-Id` header → `RequestContextService.getSessionId()`). The legacy `set_session_id` is rejected with `REQUEST_SCOPED_SESSION_ACTIVE` when context has session. The semantic gap #10725 fills is validation, not state-changing setting.
Reframe captured in [DECIDED] comment `IC_kwDODSospM8AAAABBMRXBg` on #10725; cross-confirmed by @neo-gemini-pro before implementation.
Behavior
Contract Ledger
AC coverage
Implementation
Test Evidence
`test/playwright/unit/ai/mcp/server/memory-core/services/SessionService.ResumeValidation.spec.mjs` — 5 tests covering:
3 stable runs at ~840ms each. SummarizationJobs rows planted directly via `GraphService.db.storage.db.prepare().run()` for tests 3-5; tests 2 + 5 also exercise the memoryCollection branch via `SDK.Memory_Service.addMemory` inside `RequestContextService.run({sessionId}, ...)`.
Out of Scope
Avoided Traps
Related