fix: resolve session identity from disk, not the registry (GIT-89) - #25
Merged
Conversation
Option A: the session id is the identity. PID is no longer an identity key,
only a disambiguator, and the active-sessions registry is no longer consulted
to answer "is there a session".
findResumableSessionOnDisk() replaces GIT-51's adoptSessionForCurrentProcess().
The old path iterated registry.sessions, so an empty or diverged registry meant
"no session" regardless of what was on disk — and the registry is precisely the
store that gets lost. Observed in the wild: active-sessions.json holding
{"sessions": []} with intact sessions/<id>/session.json files beside it. The
session directories are the durable evidence (session_close deletes them), so
the scan reads those and repairs the registry from what it finds.
PID still separates candidates: own pid takes a fast path, a dead pid is an
orphan left by a restart, and a live foreign pid is another server's session
that is never touched — which is what keeps GIT-19..23 multi-session
resolution intact.
Also closes two false-alarm paths that survive the identity fix:
- addSurfacedScars returned void and no-op'd without a session, so recall
printed scars, nothing recorded them, and confirm_scars later rejected with
nothing to confirm. It now resolves identity, writes through to session.json,
and returns whether tracking happened; recall says so loudly when it did not.
- recall_called lived only in memory, so after a restart every create_learning
/ create_decision / session_close warned that recall had never run in
sessions where it had. Persisted to session.json and restored on recovery.
+18 tests (1154 -> 1172): 13 unit for the disk resolver and surfacing/recall
symmetry, 5 for recall's untracked-surfacing notice. Verified against the built
dist in a fresh process reproducing the production shape (empty registry +
session on disk): zero warnings across recall, create_learning, create_thread,
create_decision and session_close over two simulated restarts, registry
repaired, surfacing persisted, live foreign session never claimed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
Gate 2 — E2E attribution: pre-existing on
|
checkEnforcement() runs on every session-required tool call and its header still advertised "pure in-memory checks, no I/O". GIT-89 routed getCurrentSession() through resolveCurrentSession(), which scans the per-session directories to rebind identity after an MCP restart — that is the fix working as designed, but it makes the stated invariant false for anyone reasoning about enforcement's cost. Documents what the cost actually is: bounded, not per-call. The scan runs until identity binds, and a failed scan is memoised against the registry fingerprint so a session-less process does not rescan on every tool call. No behaviour change. 1172 tests, typecheck clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…GIT-89 AC#4)
AC#4 asks that session_close resolve the correct session after an MCP restart
without the agent passing session_id. It could not, and the reason was not in
the resolution logic — that was already correct and already reachable-looking.
sessionClose() was written for an absent id throughout: it guards
`params.session_id &&` before validating the format (session-close.ts:834) and
recovers identity from resolveCurrentSession() when the field is empty (:857).
But SessionCloseParamsSchema declared session_id required, so validateToolArgs
rejected the call with "session_id: Required" before sessionClose() ran. The
recovery branch was dead code in the only scenario it existed for.
This is the failure GIT-89 describes landing at its worst point: a restart, or a
context compaction, is exactly what takes the id away from the agent, and close
is where the reflection has already been written and has nowhere else to go.
Found by driving the built server over the MCP stdio protocol across a real
SIGKILL and respawn — a consumer-level check rather than an in-process one.
Calling session_close with only close_type returned
{"error":"Invalid parameters for session_close: session_id: Required"}; the same
call with the id succeeded, isolating the fault to the resolution path rather
than to close itself. After this change the same harness closes the recovered
session across the restart with no id passed.
Also fails loudly when resolution genuinely comes up empty, instead of letting
an undefined id reach Supabase and surface as a "session not found" that reads
like data loss.
Two existing tests asserted the old contract (registry.test.ts,
session-close.test.ts) — they encoded the requirement that made the branch
unreachable, so they are updated to the intended contract rather than worked
around. +8 tests (1172 -> 1180), including schema-contract and disk-resolution
coverage in tests/unit/tools/session-close-identity.test.ts.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…T-89) The verification for this fix lived in a scratchpad script, which meant the acceptance criteria were checked once by hand and could regress silently. This lands it as a suite that runs in CI, using the existing e2e MCP client and its restartServer() helper rather than a bespoke harness. The restart is real: the server process is killed and a new one takes over the same GITMEM_DIR, driven over MCP stdio. An in-process test of this bug proves nothing, because the thing that fails is the process boundary. Covers AC#2 (no false "No active session" from recall / create_learning / create_thread / create_decision after a restart; registry reclaimed by a live PID; recall_called restored), AC#4 (close resolves identity with no session_id passed), and AC#5 (a genuinely absent session still warns — the fix must not invent sessions). Confirmed to actually catch the defect: with session_id restored to required in SessionCloseParamsSchema, the AC#4 case fails; with the fix in place it passes. A regression test that passes against the broken build would be worse than none. AC#3 (surfacing continuity) is deliberately not asserted here and is documented as such in the file header. recall() surfaces nothing against an empty free-tier store, so the assertion would read before=0/after=0 and pass while testing nothing. It stays covered at unit level where surfacing can be seeded. Two notes for reviewers: - Assertions read the PID from the registry rather than McpTestClient.process. That field is typed ChildProcess but createMcpClient never assigns it (`process: serverProcess!` with serverProcess left null), so any test touching it throws at runtime. The registry PID is the better witness regardless: it is what the product wrote, not what the harness happens to know. - The suite pins GITMEM_DIR and HOME to a temp dir, so it cannot read or write the developer's real .gitmem (GIT-92). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI ran unit + smoke only, so the restart behaviour this branch fixes had no automated gate — and the publish job needs: build, which meant a regression could reach npm with a green pipeline. Runs the single e2e file rather than `npm run test:e2e`: the Pro suites in that config gate on Docker and Supabase and hang instead of skipping when neither is present (GIT-90), which would make CI unreliable. This file spawns the built server directly on the free tier with GITMEM_DIR pinned to a temp dir — no Docker, no credentials, no developer state touched — so it is safe to run everywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every clean-room image had been failing to build. `RUN npm install -g npm@latest`
on a node:20-slim base stopped working once npm@latest began requiring
node ^22.22.2 || ^24.15.0 || >=26:
npm error notsup Required: {"node":"^22.22.2 || ^24.15.0 || >=26.0.0"}
npm error notsup Actual: {"npm":"10.8.2","node":"v20.20.2"}
The build died at that layer, so `build-and-run.sh <client> local` — the step
that tests the packaged tarball the way a user installs it — could not run at
all. The pre-publish gate was gone, and silently: nobody hits it until they try
to use it, which is exactly when it is needed.
The line was cosmetic (its comment said "suppresses upgrade nag"). Removed
rather than pinned: the npm bundled with the base image is what a real user on
node 20 actually has, which is what a clean room is supposed to reproduce.
node 20 stays a supported engine (package.json engines >=18, CI matrix 18/20/22).
All four images were affected; all four fixed.
Verified: gitmem-claude-local now builds, and the packaged tarball installed
globally inside it survives a real MCP restart — recall, create_learning,
create_thread, create_decision all clean, registry reclaimed, and session_close
resolving with no session_id passed (GIT-89 AC#4) on node 20.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
What
Fixes GIT-89 — Option A: the session id is the identity. PID is no longer an identity key, only a disambiguator, and the
active-sessions.jsonregistry is no longer consulted to answer "is there a session".findResumableSessionOnDisk()replaces GIT-51'sadoptSessionForCurrentProcess(). Identity resolves by scanning.gitmem/sessions/*/session.json; the registry is repaired from that scan rather than gating it.session_closedeletes the session directory, so a directory that still exists is a session that was never closed — which is what makes the scan safe without a separate "closed" marker.PID survives only to separate candidates:
That last row is what preserves GIT-19..23 multi-session resolution now that PID is not identity. Brain's review is on record that this disambiguation rule satisfies the multi-session precondition.
Supporting changes: the resolved PID is written back to
session.jsonvia read-modify-write of thepidfield only, so accumulatedsurfaced_scars/threadsare not clobbered;getRegistryFingerprint()now covers the sessions directory as well as the registry, or the "already failed recovery" latch could stay closed over a session that had since appeared on disk.Why
After any MCP server restart mid-session — app update, context compaction,
npm run build, CLI relaunch — every session-required tool reported "No active session" for the rest of the session, while the session was demonstrably live and writes were landing.GIT-51 already fixed more than the ticket assumed (recovery runs on any tool call, adoption exists,
ADOPT_THRESHOLD_MSis gone,pruneStaleno longer prunes on a dead PID). The defect that remained: both recovery paths iteratedregistry.sessions, and the registry is precisely the store that gets lost. Observed during triage in this repo —active-sessions.jsonholding{"sessions": []}with three intactsessions/<id>/session.jsonfiles sitting beside it. The durable evidence was present and unreachable.The bug reproduced live in the session opened to fix it:
recallreturned 6 scars while printing "No active session",confirm_scarshard-rejected all 6, and every one was silently dropped from tracking.Beyond the ticket — two discoveries
1.
recallCalledwas in-memory only. Even with identity fixed, everycreate_learning/create_decision/session_closeafter a restart hit enforcement Check 3 and warned "No recall() was run this session" — in sessions where it plainly had. Same class of untrue warning as the "No active session" banner, and directly against the acceptance criterion that enforcement warnings, when they do fire, are true. Found by the verification script, not by the ticket. Now persisted tosession.json, restored on recovery, and preserved acrosssession_startrefresh (which rewrites the file wholesale).2. Free-tier surfacing was never persisted at all. The inline write-through lived in
recall's pro-tier branch only. This is pre-existing structural loss, not restart-dependent — free-tier surfacing has never survived a process boundary. Centralising the write inaddSurfacedScarsfixes it as a side effect.Also in this change:
addSurfacedScarsresolves identity instead of reading in-memory state and returns whether tracking happened, sorecallcan emit aSURFACING NOT TRACKEDblock at the same volume asconfirm_scars' rejection — closing the asymmetry. The notice returns""on the healthy path, so a tracked recall costs no extra tokens (scar55dd6d73).isRecallCalled()andgetProject()now resolve rather than reading in-memory state; the latter was silently falling back to project"default"after a restart.How to Test
Against the built
dist/in a fresh process, seeded with the exact production shape (active-sessions.json={"sessions": []}, intactsession.jsonwritten by a dead PID) — 9/9:Unit suite: 1172 passing, 69 files, zero failures (was 1154 — +18 tests: 13 for the disk resolver and surfacing/recall symmetry, 5 for the untracked-surfacing notice).
Note: verifying any fix here needs a fresh MCP server process — there is no hot-reload after
npm run build(scarcf6736fb).🚦 Blocking gates
Gate 1 — AC-2 consumer check ⬜ NOT STARTED
One real CLI session spanning an actual MCP server relaunch on this build:
session_starton the new buildrecallsession_closePasses when: zero false "No active session" warnings, and closing
surfaced_scarscount matches what was actually recalled (no silent discard). The next natural CLI restart is the test.Gate 2 — E2E attribution ✅ CLEARED — pre-existing, does not block
main@c091a75stalls identically. Same ~15-minute cap, isolated worktree with its own build: 2 of 7 files completed (31 tests, both passing), then nothing for the remaining ~14 minutes. Killed at 900s, exit 143.Per the decision rule: hangs on
maintoo → pre-existing, does not block merge.Correction to my first report. I claimed the branch run "produced no output for ~15 minutes." The cause was my own command —
npx vitest ... | grep | tail -15, andtailholds all input until upstream exits, so an empty file meant "not finished," not "hung." The unbufferedmainrun shows the suite running normally. The observation was an artifact; the conclusion below rests on the re-run, not on it.The stall is environmental. The 5 non-completing files need Docker testcontainers and/or a reachable Supabase; the log is full of
fetch failed.pro-fresh.test.tsgates onDOCKER_AVAILABLE, which is true here, so it attempts container startup rather than skipping.Branch checked on what is actually runnable. The 2 network-free files on
bc69b95: 31/31 passing, exit 0 — identical tomain. These includegit-67-fail-open.test.ts, which exercises the sessionless-write guard and is squarely in this change's blast radius.Full E2E remains unverified end-to-end on both
mainand this branch, for the same environmental reason. That is a pre-existing CI/environment gap, not a property of this PR.Checklist
npm run test:unitpasses — 1172/1172npm run test:e2epasses — cannot complete in this environment, onmainor here (Gate 2). The 2 network-free files pass 31/31 on both.Breaking Changes
None.
adoptSessionForCurrentProcess()is removed, but it was internal and had exactly one caller;findSessionByHostPid()is retained as a registry query. Existingsession.jsonfiles withoutrecall_calledread asfalse, which is the prior behaviour.Follow-up candidates — explicitly NOT in scope
Both are recorded on GIT-89 as deliberate exclusions, not oversights:
resolveCurrentSession()is synchronous and called from the sync enforcement path; adding a network call means making it async and threading that through every caller. The local scan covers every failure mode reproduced so far..gitmemroot. Resolution is still order-dependent (env → cache → cwd walk-up →~/.gitmem). Pinning it to the git root would change where.gitmemlives for existing installs — a migration, not a bugfix.Separately noticed and already spun off:
src/tools/recall.test.ts(11 tests) is never executed — no vitest config includessrc/**/*.test.ts.🤖 Generated with Claude Code