fix(mcp): resolve implicit session_id to active hook-created session#400
Open
SacrilegeTx wants to merge 5 commits into
Open
fix(mcp): resolve implicit session_id to active hook-created session#400SacrilegeTx wants to merge 5 commits into
SacrilegeTx wants to merge 5 commits into
Conversation
mem_save and four sibling handlers fell back to manual-save-{project}
whenever session_id was not provided in the tool args, leaving the UUID
sessions created by the Claude Code SessionStart hook with zero
observations and accumulating all writes into one bucket per project.
New Store.LookupActiveSession(project, directory) resolves the most
recent open session matching the MCP process cwd; the manual-save
fallback is preserved when no active session is found.
Fixes Gentleman-Programming#386
The Gentleman-Programming#386 data-attachment fix routes observations to the active hook-created UUID session, but four handlers still recorded/read activity under defaultSessionID(project). After this change all four handlers key activity against the same session ID they use for writes: handleSessionSummary reads under the resolved ID, handleCapturePassive records under the resolved ID (resolution moved before the activity call), handleSessionStart/End use the explicit id from the request — which is the correct bucket regardless of Gentleman-Programming#386.
…the active-session index handleCapturePassive reorder in the previous commit moved ensureImplicitSessionWithCWD and RecordToolCall above the content guard, so empty-content calls were creating a session row in the DB and incrementing the activity counter before returning the error. Restore the content guard above the side-effecting block; only the read-only resolveImplicitSessionID remains relevant ahead of the activity call, which now lives after the guard alongside session ensurement. LookupActiveSession's index lacked the sort column, so ORDER BY started_at DESC triggered a TEMP B-TREE sort despite the covering WHERE. Recreate the index with started_at DESC appended so the planner can serve the ordered LIMIT 1 directly from the index; DROP IF EXISTS first to supersede the prior shape. Adds a regression test that empty content does not mutate the session table or the activity tracker.
- resolveImplicitSessionID now returns (id, found) so handlers skip
ensureImplicitSessionWithCWD when the lookup resolved to an existing
UUID session, preventing spurious SyncOpUpsert mutations on every
mem_save when the hook-registered session is active. Caller-provided
session IDs preserve the pre-existing auto-create behavior.
- Surface DB errors from LookupActiveSession via stderr instead of
swallowing them; a transient lookup failure was silently routing
writes back to manual-save-{project} with no signal.
- Rename TestCapturePassiveRecordsToolCall to clarify it exercises
the fallback path; UUID resolution is covered separately.
- Drop the 1100ms sleep in TestLookupActiveSessionReturnsMostRecent...
by writing explicit started_at values, eliminating clock-step flake
risk and trimming suite time.
…overage gap Item 1: drop rotting line-number reference in resolveImplicitSessionID's error-log comment; replace with a stable prose reference to handleSave. Item 2: add TestMemSavePromptSkipsSessionUpsertWhenResolvedToExistingUUID and TestMemSessionSummarySkipsSessionUpsertWhenResolvedToExistingUUID, mirroring the existing handleSave test from 939cc2c. Both handlers already carry the !sessionFound guard, but had no test pinning it — a regression could reintroduce the spurious SyncOpUpsert flood silently. Each new test was verified: FAIL with the unconditional ensure, PASS with the guard.
SacrilegeTx
force-pushed
the
fix/session-id-propagation
branch
from
1a693e9 to
084ce71
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes MCP write tools (e.g., mem_save) incorrectly defaulting to manual-save-{project} when a real UUID session was already opened by the Claude Code SessionStart hook, ensuring observations/prompts/summaries attach to the active open session instead.
Changes:
- Added
Store.LookupActiveSession(project, directory)and a supporting SQLite index to find the most recent open session for a(project, directory)pair. - Routed implicit session resolution in MCP through
resolveImplicitSessionID, updating multiple handlers and aligningSessionActivityto be keyed by the resolved/explicit session ID. - Added store- and MCP-level tests covering resolution behavior, fallbacks, and activity keying.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/store/store.go | Adds a new session lookup API and a migration-time index intended to speed up open-session resolution. |
| internal/store/sessions_lookup_test.go | New unit tests for LookupActiveSession behavior (ended sessions, ordering, blank inputs, etc.). |
| internal/mcp/mcp.go | Uses store-side lookup to resolve implicit session IDs and fixes activity tracking to use the resolved/explicit session. |
| internal/mcp/mcp_test.go | Adds/updates tests validating implicit session resolution across affected MCP handlers and activity tracking. |
Comments suppressed due to low confidence (1)
internal/store/store.go:1925
- LookupActiveSession matches sessions by exact project string, but sessions are stored with NormalizeProject() in CreateSession (and other query APIs normalize too, e.g. RecentSessions). Without normalizing here, callers that pass an unnormalized project will silently miss the active session and fall back to manual-save. Normalize/trim the project (and document/consider normalizing directory if you want stable matching).
title,
content,
tool_name,
type,
project,
topic_key,
content='observations',
content_rowid='id'
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1060
to
+1071
| // Phase: session-id-propagation (#386) — index for LookupActiveSession. | ||
| // Includes started_at DESC so the planner can satisfy ORDER BY started_at DESC | ||
| // directly from the index without a TEMP B-TREE sort step. DROP first to | ||
| // supersede any pre-existing index that lacked the sort column. | ||
| if _, err := s.execHook(s.db, `DROP INDEX IF EXISTS idx_sessions_active_lookup;`); err != nil { | ||
| return err | ||
| } | ||
| if _, err := s.execHook(s.db, ` | ||
| CREATE INDEX IF NOT EXISTS idx_sessions_active_lookup | ||
| ON sessions(project, directory, ended_at, started_at DESC); | ||
| `); err != nil { | ||
| return err |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #386
Summary
When the
SessionStarthook registers a session viaPOST /sessions, subsequentmem_savecalls without an explicitsession_idwere falling back tomanual-save-{project}instead of attaching observations to the active UUID session. The result: every Claude Code session left an empty UUID row, while one ever-growingmanual-save-*bucket accumulated all observations.This PR implements direction (3) from the issue (store-side resolution), with
directorymatching to avoid cross-cwd collisions inside the same project.Approach
store.LookupActiveSession(project, directory)returns the most recent open session (ended_at IS NULL) for the resolved project, preferring the row whosedirectorymatches the caller's cwd.resolveImplicitSessionID(s, project)ininternal/mcpis the single bridge used by every handler that previously calleddefaultSessionID(project)directly. It falls back tomanual-save-{project}only when no active session is found.SessionActivityis now keyed by the resolved session ID rather than the raw argument, so activity tracking stays consistent whether the caller passessession_idexplicitly or relies on implicit resolution.Handlers updated
handleSave(mem_save)handleSavePrompt(mem_save_prompt)handleSessionSummaryhandleSessionStart/handleSessionEndhandleCapturePassiveBehavior preserved
session_idargument still wins; resolution only kicks in when the caller omits it.manual-save-{project}remains the final fallback when there is no active session row, so existing flows that never opened a session are unaffected.Tests
internal/store/sessions_lookup_test.go(new) coversLookupActiveSession: directory match, project scoping,ended_atfiltering, ordering by most recent.internal/mcp/mcp_test.goadds coverage across every affected handler: implicit resolution to an active session, fallback tomanual-save-*when none is open, activity tracker keyed by the resolved ID, and thecapture_passiveandsave_prompt/session_summarypaths.Test plan
go test ./internal/store/... -run LookupActiveSessiongo test ./internal/mcp/...go test ./...mem_savewithoutsession_id, verify the UUID session'sobservation_countincrements instead of themanual-save-*row.