From bc69b9513bad64556cfb4945ce0eeb5be5b68d67 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 21:34:48 -0400 Subject: [PATCH 1/6] fix: resolve session identity from disk, not the registry (GIT-89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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//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 --- src/services/active-sessions.ts | 225 ++++++++++++---- src/services/session-state.ts | 122 +++++++-- src/tools/recall.ts | 57 ++-- src/tools/session-start.ts | 21 +- tests/unit/services/active-sessions.test.ts | 250 ++++++++++++------ .../services/session-state-surfacing.test.ts | 248 +++++++++++++++++ tests/unit/tools/recall-surfacing.test.ts | 173 ++++++++++++ 7 files changed, 921 insertions(+), 175 deletions(-) create mode 100644 tests/unit/services/session-state-surfacing.test.ts create mode 100644 tests/unit/tools/recall-surfacing.test.ts diff --git a/src/services/active-sessions.ts b/src/services/active-sessions.ts index 4ff0348..9be4ac0 100644 --- a/src/services/active-sessions.ts +++ b/src/services/active-sessions.ts @@ -16,7 +16,7 @@ import * as path from "path"; import * as os from "os"; import { getGitmemDir, getSessionPath, sanitizePathComponent } from "./gitmem-dir.js"; import { ActiveSessionsRegistrySchema } from "../schemas/active-sessions.js"; -import type { ActiveSessionEntry, ActiveSessionsRegistry } from "../types/index.js"; +import type { ActiveSessionEntry, ActiveSessionsRegistry, AgentIdentity } from "../types/index.js"; import { withLockSync } from "./file-lock.js"; const REGISTRY_FILENAME = "active-sessions.json"; @@ -61,15 +61,27 @@ function getRegistryPath(): string { * own CLI process (scar 55d1bccd) — so "I already failed to recover" is only * valid until someone else touches the file. * - * Returns null when the registry does not exist, which is itself a stable - * state worth caching against. + * Returns null when neither the registry nor the sessions directory exists, + * which is itself a stable state worth caching against. + * + * GIT-89: covers the sessions directory as well as the registry. Identity now + * resolves from sessions//session.json, so a fingerprint that watched only + * active-sessions.json would keep the "already failed" latch closed over a + * session that had since appeared on disk — reintroducing the permanent + * fail-closed behaviour the fingerprint was introduced to prevent. */ export function getRegistryFingerprint(): number | null { - try { - return fs.statSync(getRegistryPath()).mtimeMs; - } catch { - return null; + const mtimes: number[] = []; + for (const target of [getRegistryPath(), path.join(getGitmemDir(), "sessions")]) { + try { + mtimes.push(fs.statSync(target).mtimeMs); + } catch { + // Missing target contributes nothing — absence is part of the fingerprint. + } } + if (mtimes.length === 0) return null; + // Sum, not concat: this only needs to change when either input changes. + return mtimes.reduce((a, b) => a + b, 0); } function getLockPath(): string { @@ -213,62 +225,177 @@ function isPidAlive(pid: number): boolean { } /** - * GIT-51: Adopt a session left behind by a previous incarnation of this MCP - * server process, rebinding it to the current PID. + * GIT-89: Resolve this process's session from the per-session directories. * - * The MCP server restarts routinely mid-session (context compaction, rebuild, - * client restart). The hostname stays the same but the PID changes, so - * findSessionByHostPid() no longer matches and the session looks gone. A - * registry entry on this host whose PID is dead, with its session file still on - * disk, is that session. + * This replaces registry-gated recovery (GIT-51's adoptSessionForCurrentProcess). + * That approach iterated `registry.sessions`, so an empty or diverged registry + * meant "no session" no matter 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//session.json files beside it. * - * Deliberately narrow: - * - Adopts at most ONE entry. Rebinding every dead-PID entry would leave - * multiple rows sharing hostname+pid, which findSessionByHostPid() then - * resolves arbitrarily by array order. - * - Never adopts a session whose PID is still alive (GIT-20: "never resume - * another process's session"). - * - Not gated on session age beyond the 24h stale horizon. The old 2h adoption - * window was shorter than a normal working session, so the sessions most in - * need of recovery were the ones excluded from it. + * `.gitmem/sessions//session.json` is the durable evidence. session_close + * deletes the directory (session-close.ts cleanupSessionFiles), so a directory + * that still exists is a session that was never closed. The registry is derived + * from this scan and repaired by it — it is an index, never an answer. * - * Returns the adopted entry (with the updated PID), or null if there is nothing - * to adopt. + * PID is no longer an identity key, only a disambiguator among candidates: + * - own PID -> this process's session, no adoption needed + * - dead PID -> orphaned by an MCP restart, adoptable + * - live foreign PID -> another concurrent server owns it, never touched + * (GIT-20: "never resume another process's session"; keeps GIT-19..23 + * multi-session resolution intact) + * + * Returns the resolved entry (PID rebound to this process), or null when there + * is genuinely nothing to resume. Never invents a session. */ -export function adoptSessionForCurrentProcess(): ActiveSessionEntry | null { +const AGENT_IDENTITIES = ["cli", "desktop", "autonomous", "local", "cloud"] as const; + +/** + * Coerce a session.json `agent` field to AgentIdentity. + * + * Case-insensitive because session files in the field carry values like "CLI" + * that never matched the lowercase union. Anything unrecognised becomes + * "Unknown" rather than being asserted through — a wrong agent label is a + * display problem, an invalid one is a type lie. + */ +function toAgentIdentity(value: unknown): AgentIdentity { + if (typeof value !== "string") return "Unknown"; + const normalized = value.toLowerCase(); + return AGENT_IDENTITIES.find((id) => id === normalized) ?? "Unknown"; +} + +export function findResumableSessionOnDisk(): ActiveSessionEntry | null { const currentHostname = os.hostname(); const currentPid = process.pid; const gitmemDir = getGitmemDir(); + const sessionsDir = path.join(gitmemDir, "sessions"); - return withLockSync(getLockPath(), () => { - const registry = readRegistry(); - const now = Date.now(); + let dirNames: string[]; + try { + if (!fs.existsSync(sessionsDir)) return null; + dirNames = fs.readdirSync(sessionsDir); + } catch (error) { + console.warn("[active-sessions] Failed to scan sessions directory:", error); + return null; + } - const candidates = registry.sessions - .filter((entry) => { - if (entry.hostname !== currentHostname) return false; - if (isPidAlive(entry.pid)) return false; + const now = Date.now(); + const candidates: { entry: ActiveSessionEntry; pidAlive: boolean }[] = []; - const age = now - new Date(entry.started_at).getTime(); - if (!Number.isFinite(age) || age > STALE_THRESHOLD_MS) return false; + for (const dirName of dirNames) { + const sessionFile = path.join(sessionsDir, dirName, "session.json"); + let data: Record; + try { + if (!fs.existsSync(sessionFile)) continue; + data = JSON.parse(fs.readFileSync(sessionFile, "utf-8")); + } catch { + continue; // unreadable or corrupt session file — not resumable + } - // The session file is the evidence that the session is real and resumable. - const sessionFile = path.join(gitmemDir, "sessions", entry.session_id, "session.json"); - return fs.existsSync(sessionFile); - }) - .sort((a, b) => new Date(b.started_at).getTime() - new Date(a.started_at).getTime()); + // The directory name IS the session id (getSessionDir). A mismatch means the + // file was hand-edited or copied; refuse rather than resolve to the wrong id. + if (typeof data.session_id !== "string" || data.session_id !== dirName) continue; + + // Sessions are host-local. A session file synced from another machine + // (shared checkout, backup restore) is not ours to resume. + if (typeof data.hostname === "string" && data.hostname !== currentHostname) continue; + + const startedAt = typeof data.started_at === "string" ? data.started_at : ""; + const age = now - new Date(startedAt).getTime(); + if (!Number.isFinite(age) || age > STALE_THRESHOLD_MS) continue; + + const pid = typeof data.pid === "number" ? data.pid : -1; + candidates.push({ + entry: { + session_id: data.session_id, + agent: toAgentIdentity(data.agent), + started_at: startedAt, + hostname: currentHostname, + pid, + project: typeof data.project === "string" ? data.project : "default", + }, + pidAlive: pid > 0 && pid !== currentPid && isPidAlive(pid), + }); + } - const adopted = candidates[0]; - if (!adopted) return null; + const newestFirst = ( + a: { entry: ActiveSessionEntry }, + b: { entry: ActiveSessionEntry } + ) => new Date(b.entry.started_at).getTime() - new Date(a.entry.started_at).getTime(); - console.error( - `[active-sessions] Adopting orphaned session ${adopted.session_id.slice(0, 8)} (dead pid ${adopted.pid} → ${currentPid})` - ); - adopted.pid = currentPid; - writeRegistry(registry); + // 1. Our own PID — nothing to adopt, just rebuild in-memory state. + const mine = candidates.filter((c) => c.entry.pid === currentPid).sort(newestFirst)[0]; + if (mine) { + reconcileRegistryEntry(mine.entry); + return { ...mine.entry }; + } - return { ...adopted }; - }); + // 2. Orphaned by a restart. Adopt at most one — rebinding every dead-PID + // session would leave several rows sharing hostname+pid. + const orphaned = candidates.filter((c) => !c.pidAlive).sort(newestFirst)[0]; + if (!orphaned) return null; + + const rebound: ActiveSessionEntry = { ...orphaned.entry, pid: currentPid }; + console.error( + `[active-sessions] Resuming session ${rebound.session_id.slice(0, 8)} from disk ` + + `(dead pid ${orphaned.entry.pid} → ${currentPid})` + ); + + persistSessionPid(rebound.session_id, currentPid); + reconcileRegistryEntry(rebound); + return rebound; +} + +/** + * GIT-89: Write the resolved PID back into session.json so the next scan takes + * the own-PID fast path instead of re-adopting. + * + * Read-modify-write of only the pid field: session.json accumulates state from + * several writers (surfaced_scars from recall, threads from session_start), and + * rewriting the whole object from an ActiveSessionEntry would drop all of it. + */ +function persistSessionPid(sessionId: string, pid: number): void { + try { + const sessionFile = path.join(getGitmemDir(), "sessions", sessionId, "session.json"); + const data = JSON.parse(fs.readFileSync(sessionFile, "utf-8")); + data.pid = pid; + atomicWriteFileSync(sessionFile, JSON.stringify(data, null, 2)); + } catch (error) { + // Non-fatal: identity is resolved either way, the next scan just re-adopts. + console.warn(`[active-sessions] Failed to persist pid for ${sessionId.slice(0, 8)}:`, error); + } +} + +/** + * GIT-89: Repair the registry from a disk-resolved session. + * + * The registry is now derived state. When the scan finds a session the registry + * has lost or mis-keyed, this puts it back so registry consumers + * (findSessionByHostPid, list-sessions diagnostics) agree with disk again. + */ +function reconcileRegistryEntry(entry: ActiveSessionEntry): void { + try { + withLockSync(getLockPath(), () => { + const registry = readRegistry(); + const existing = registry.sessions.find((s) => s.session_id === entry.session_id); + if (existing && existing.pid === entry.pid && existing.hostname === entry.hostname) { + return; // already agrees — no write + } + registry.sessions = registry.sessions.filter( + (s) => + s.session_id !== entry.session_id && + !(s.hostname === entry.hostname && s.pid === entry.pid) + ); + registry.sessions.push(entry); + writeRegistry(registry); + console.error( + `[active-sessions] Registry reconciled from disk for ${entry.session_id.slice(0, 8)}` + ); + }); + } catch (error) { + // Non-fatal: the registry is an index, not the answer. + console.warn("[active-sessions] Failed to reconcile registry from disk:", error); + } } /** diff --git a/src/services/session-state.ts b/src/services/session-state.ts index e9957b0..9fca222 100644 --- a/src/services/session-state.ts +++ b/src/services/session-state.ts @@ -13,10 +13,9 @@ */ import fs from "fs"; -import * as os from "os"; import type { SurfacedScar, ScarConfirmation, ScarReflection, Observation, SessionChild, ThreadObject } from "../types/index.js"; import { getSessionPath } from "./gitmem-dir.js"; -import { findSessionByHostPid, adoptSessionForCurrentProcess, getRegistryFingerprint } from "./active-sessions.js"; +import { findResumableSessionOnDisk, listActiveSessions, getRegistryFingerprint } from "./active-sessions.js"; interface SessionContext { sessionId: string; @@ -92,18 +91,27 @@ export function setCurrentSession(context: Omit s.session_id === entry.session_id) ?? null; const sessionFilePath = getSessionPath(entry.session_id, "session.json"); if (!fs.existsSync(sessionFilePath)) return null; @@ -123,15 +131,18 @@ function recoverSessionFromDisk(): SessionContext | null { // because it is written by the session itself, while the registry entry is // an index that can lag. The difference is that a conflict is now recorded // and logged rather than resolved in silence. + // + // GIT-89: compared against the pre-resolution registry snapshot. `entry` is + // derived from session.json now, so comparing the two would always agree. const recoveryConflict = Boolean(data.project) && - Boolean(entry.project) && - data.project !== entry.project; + Boolean(registryEntry?.project) && + data.project !== registryEntry?.project; if (recoveryConflict) { console.error( `[session-state] RECOVERY CONFLICT for ${String(data.session_id).slice(0, 8)}: ` + - `session.json project "${data.project}" != registry project "${entry.project}". ` + + `session.json project "${data.project}" != registry project "${registryEntry?.project}". ` + `Using session.json (authoritative); registry entry is a lagging index.` ); } @@ -147,6 +158,13 @@ function recoverSessionFromDisk(): SessionContext | null { recoveryConflict, }); + // GIT-89: restore the recall flag. setCurrentSession resets it to false, + // which would make enforcement Check 3 warn that recall never ran in a + // session where it had — a false alarm that survives the identity fix. + if (currentSession && data.recall_called === true) { + currentSession.recallCalled = true; + } + console.error( `[session-state] Recovered session ${data.session_id.slice(0, 8)} from disk after MCP restart ` + `(${currentSession?.surfacedScars.length ?? 0} surfaced scars)` @@ -203,7 +221,10 @@ export function clearCurrentSession(): void { * Used by list_threads to inherit the correct project default. */ export function getProject(): string | null { - return currentSession?.project || null; + // GIT-89: resolves rather than reading in-memory state. After a restart this + // returned null and callers silently fell back to project "default", scoping + // the rest of the session to the wrong namespace. + return resolveCurrentSession()?.project || null; } /** @@ -216,11 +237,31 @@ export function hasActiveIssue(): boolean { /** * Mark that recall() was called this session (independent of whether it returned scars). * Called by recall tool before any early return. + * + * GIT-89: persisted to session.json. This flag drove enforcement Check 3 ("No + * recall() was run this session"), and it lived only in memory — so after an + * MCP restart every create_learning / create_decision / session_close warned + * that recall had never run, in sessions where it demonstrably had. That is the + * same class of false alarm as the "No active session" banner: a warning the + * agent learns to read past, which is what erodes the enforcement layer. */ export function setRecallCalled(): void { - if (currentSession) { - currentSession.recallCalled = true; - console.error("[session-state] recall() marked as called"); + const session = resolveCurrentSession(); + if (!session) return; + + session.recallCalled = true; + console.error("[session-state] recall() marked as called"); + + try { + const sessionFilePath = getSessionPath(session.sessionId, "session.json"); + if (!fs.existsSync(sessionFilePath)) return; + const data = JSON.parse(fs.readFileSync(sessionFilePath, "utf-8")); + if (data.recall_called === true) return; // already recorded — no write + data.recall_called = true; + fs.writeFileSync(sessionFilePath, JSON.stringify(data, null, 2)); + } catch (error) { + // Non-fatal: the flag still holds for this process. + console.warn("[session-state] Failed to persist recall_called:", error); } } @@ -229,27 +270,66 @@ export function setRecallCalled(): void { * Used by enforcement to avoid false positives when recall returns 0 scars. */ export function isRecallCalled(): boolean { - return currentSession?.recallCalled ?? false; + // GIT-89: resolves rather than reading in-memory state, so the flag restored + // from session.json is visible to callers that reach this directly. + return resolveCurrentSession()?.recallCalled ?? false; } /** * Add surfaced scars to tracking (deduplicates by scar_id) * Called by session_start and recall when scars are surfaced. + * + * GIT-89: returns whether the scars were actually tracked. + * + * This used to read `currentSession` directly and, when it was null, log a + * console warning and return. That was the silent discard behind the + * recall/confirm_scars asymmetry (scar 810a1624): recall printed scars to the + * agent, nothing recorded that it had, and confirm_scars later rejected with + * nothing to confirm. The agent saw a green "no scars to confirm" for scars it + * had just been shown. + * + * Two changes close that gap. Identity is resolved (so a session recovered + * after an MCP restart still tracks), and the outcome is returned so callers + * can fail as loudly as confirm_scars does instead of proceeding as if tracked. */ -export function addSurfacedScars(scars: SurfacedScar[]): void { - if (!currentSession) { +export function addSurfacedScars(scars: SurfacedScar[]): boolean { + const session = resolveCurrentSession(); + if (!session) { console.warn("[session-state] Cannot add surfaced scars: no active session"); - return; + return false; } for (const scar of scars) { - const exists = currentSession.surfacedScars.some(s => s.scar_id === scar.scar_id); + const exists = session.surfacedScars.some(s => s.scar_id === scar.scar_id); if (!exists) { - currentSession.surfacedScars.push(scar); + session.surfacedScars.push(scar); } } - console.error(`[session-state] Surfaced scars tracked: ${currentSession.surfacedScars.length} total`); + console.error(`[session-state] Surfaced scars tracked: ${session.surfacedScars.length} total`); + persistSurfacedScars(session); + return true; +} + +/** + * GIT-89: Write surfaced scars through to session.json. + * + * Surfacing has to outlive the process that did it. If it lives only in memory, + * an MCP restart between recall and confirm_scars loses it, and the identity + * break turns into a tracking break. Centralised here so there is exactly one + * writer — callers previously did this inline and only on their own success path. + */ +function persistSurfacedScars(session: SessionContext): void { + try { + const sessionFilePath = getSessionPath(session.sessionId, "session.json"); + if (!fs.existsSync(sessionFilePath)) return; + const data = JSON.parse(fs.readFileSync(sessionFilePath, "utf-8")); + data.surfaced_scars = session.surfacedScars; + fs.writeFileSync(sessionFilePath, JSON.stringify(data, null, 2)); + } catch (error) { + // Non-fatal: scars remain tracked in memory for this process. + console.warn("[session-state] Failed to persist surfaced scars:", error); + } } /** diff --git a/src/tools/recall.ts b/src/tools/recall.ts index bbdae21..53d7a98 100644 --- a/src/tools/recall.ts +++ b/src/tools/recall.ts @@ -35,9 +35,6 @@ import { import { addSurfacedScars, getCurrentSession, setRecallCalled } from "../services/session-state.js"; import { getAgentIdentity } from "../services/agent-detection.js"; import { v4 as uuidv4 } from "uuid"; -import * as fs from "fs"; -import * as path from "path"; -import { getSessionPath } from "../services/gitmem-dir.js"; import { wrapDisplay, productLine, SEV, boldText, dimText, ANSI, CITATION_LINE } from "../services/display-protocol.js"; import { formatNudgeHeader } from "../services/nudge-variants.js"; import { fetchDismissalCounts, type DismissalCounts } from "../services/behavioral-decay.js"; @@ -164,6 +161,29 @@ export interface RecallResult { performance: PerformanceData; } +/** + * GIT-89: Tell the agent when surfacing was not recorded. + * + * recall and confirm_scars used to fail asymmetrically: recall returned scars + * with a soft "No active session" banner and dropped them from tracking, while + * confirm_scars hard-rejected. The agent had no way to tell a tracked recall + * from an untracked one, and a later confirm reported "no scars to confirm" — + * a green result that actually meant the scars were discarded (scar 810a1624). + * + * Returns "" on the tracked path, so a healthy recall costs no extra tokens. + */ +function untrackedSurfacingNotice(tracked: boolean, scarCount: number): string { + if (tracked || scarCount === 0) return ""; + return [ + "", + "--- gitmem enforcement ---", + `SURFACING NOT TRACKED — the ${scarCount} scar(s) above were not recorded against a session.`, + "confirm_scars will reject them and they will not count toward scar application.", + "Call session_start() to open a session, then re-run recall().", + "---", + ].join("\n"); +} + /** * Format scars into a readable response for Claude */ @@ -454,9 +474,10 @@ export async function recall(params: RecallParams): Promise { surfaced_at: recallSurfacedAt, source: "recall" as const, })); - addSurfacedScars(recallSurfacedScars); + const tracked = addSurfacedScars(recallSurfacedScars); - const freeFormatted = formatResponse(scars, plan); + const freeFormatted = + formatResponse(scars, plan) + untrackedSurfacingNotice(tracked, scars.length); return { activated: scars.length > 0, plan, @@ -651,23 +672,11 @@ export async function recall(params: RecallParams): Promise { source: "recall" as const, variant_id: variantResults.get(scar.id)?.assignment?.variant_id, })); - addSurfacedScars(recallSurfacedScars); - - // Update per-session dir with accumulated surfaced scars - try { - const session = getCurrentSession(); - if (session) { - const sessionFilePath = getSessionPath(session.sessionId, "session.json"); - if (fs.existsSync(sessionFilePath)) { - const sessionData = JSON.parse(fs.readFileSync(sessionFilePath, "utf-8")); - sessionData.surfaced_scars = session.surfacedScars; - fs.writeFileSync(sessionFilePath, JSON.stringify(sessionData, null, 2)); - } - } - } catch (error) { - // Non-fatal: surfaced scars still tracked in memory - console.warn("[recall] Failed to update per-session file with surfaced scars:", error); - } + // GIT-89: addSurfacedScars now writes through to the per-session file itself, + // so surfacing survives an MCP restart between recall and confirm_scars. + // The inline write that used to live here duplicated that and ran only on + // this path, leaving the free-tier path's surfacing memory-only. + const surfacingTracked = addSurfacedScars(recallSurfacedScars); const latencyMs = timer.stop(); const memoriesSurfaced = scars.map((s) => s.id); @@ -694,7 +703,9 @@ export async function recall(params: RecallParams): Promise { }); // Record metrics asynchronously - const mainFormatted = formatResponse(scars, plan, dismissalCounts); + const mainFormatted = + formatResponse(scars, plan, dismissalCounts) + + untrackedSurfacingNotice(surfacingTracked, scars.length); const result = { activated: scars.length > 0, plan, diff --git a/src/tools/session-start.ts b/src/tools/session-start.ts index 91fd1b3..5b53148 100644 --- a/src/tools/session-start.ts +++ b/src/tools/session-start.ts @@ -37,7 +37,7 @@ import { resolveThreadScope, computePanelOmission, formatOmissionLine } from ".. import type { ThreadScopeCounts } from "../services/thread-scope.js"; import type { ThreadDisplayInfo } from "../services/thread-supabase.js"; import { setGitmemDir, getGitmemDir, getSessionPath, getConfigProject } from "../services/gitmem-dir.js"; -import { registerSession, findSessionByHostPid, adoptSessionForCurrentProcess, pruneStale, migrateFromLegacy } from "../services/active-sessions.js"; +import { registerSession, findSessionByHostPid, findResumableSessionOnDisk, pruneStale, migrateFromLegacy } from "../services/active-sessions.js"; import * as os from "os"; import { formatDate } from "../services/timezone.js"; import { productLine, dimText, boldText } from "../services/display-protocol.js"; @@ -711,11 +711,12 @@ function checkExistingSession( // GIT-20: Prune stale sessions from crashed/dead containers pruneStale(); - // GIT-20: Check registry for THIS process's session (hostname + PID match). - // GIT-51: if the server restarted, the PID no longer matches — adopt the - // session this process left behind rather than starting a second one. + // GIT-89: resolve this process's session from the per-session directories. + // The registry check stays as a fast path, but a miss there is no longer an + // answer — a lost or diverged registry used to make an intact session on + // disk invisible, so session_start would open a second session alongside it. const mySession = - findSessionByHostPid(os.hostname(), process.pid) ?? adoptSessionForCurrentProcess(); + findSessionByHostPid(os.hostname(), process.pid) ?? findResumableSessionOnDisk(); if (mySession) { console.error(`[session_start] Found own session in registry: ${mySession.session_id} (host: ${mySession.hostname}, pid: ${mySession.pid})`); const data = readSessionFile(mySession.session_id); @@ -759,8 +760,12 @@ function writeSessionFiles( // Preserve original started_at on resume/refresh to keep duration accurate let effectiveStartedAt = startedAt?.toISOString() || new Date().toISOString(); + // GIT-89: recall_called is preserved for the same reason. This write replaces + // session.json wholesale, so a refresh would otherwise clear the flag and + // enforcement would warn that recall never ran in a session where it had. + let effectiveRecallCalled = false; if (isRefresh || startedAt) { - // On refresh or resume, try to read the existing started_at from the session file + // On refresh or resume, try to read the existing state from the session file try { const existingPath = getSessionPath(sessionId, "session.json"); if (fs.existsSync(existingPath)) { @@ -768,8 +773,9 @@ function writeSessionFiles( if (existing.started_at) { effectiveStartedAt = existing.started_at; } + effectiveRecallCalled = existing.recall_called === true; } - } catch { /* use calculated value */ } + } catch { /* use calculated values */ } } const data = { @@ -781,6 +787,7 @@ function writeSessionFiles( pid: process.pid, gitmem_dir: gitmemDir, surfaced_scars: surfacedScars, + recall_called: effectiveRecallCalled, threads, ...(recordingPath && { recording_path: recordingPath }), ...(isRefresh && { last_refreshed: new Date().toISOString() }), diff --git a/tests/unit/services/active-sessions.test.ts b/tests/unit/services/active-sessions.test.ts index 64924c2..8e163a2 100644 --- a/tests/unit/services/active-sessions.test.ts +++ b/tests/unit/services/active-sessions.test.ts @@ -16,7 +16,7 @@ import { listActiveSessions, findSessionByHostPid, findSessionById, - adoptSessionForCurrentProcess, + findResumableSessionOnDisk, pruneStale, migrateFromLegacy, resetMigrationFlag, @@ -36,11 +36,33 @@ function makeEntry(overrides: Partial = {}): ActiveSessionEn }; } -/** Create per-session directory with session.json so pruneStale orphan check doesn't remove it */ -function createSessionFile(sessionId: string): void { +/** + * Create per-session directory with session.json. + * + * GIT-89: session.json is now the identity source, not just a liveness sentinel + * for pruneStale, so it carries the full record a real session_start writes. + * Overrides let a test seed a session the registry has never heard of. + */ +function createSessionFile(sessionId: string, overrides: Record = {}): void { const sessionDir = path.join(tmpDir, "sessions", sessionId); fs.mkdirSync(sessionDir, { recursive: true }); - fs.writeFileSync(path.join(sessionDir, "session.json"), JSON.stringify({ session_id: sessionId })); + fs.writeFileSync( + path.join(sessionDir, "session.json"), + JSON.stringify({ + session_id: sessionId, + agent: "cli", + started_at: new Date().toISOString(), + hostname: os.hostname(), + pid: process.pid, + project: "default", + ...overrides, + }) + ); +} + +/** Seed a session that exists only on disk — no registry entry at all. */ +function seedDiskOnlySession(sessionId: string, overrides: Record = {}): void { + createSessionFile(sessionId, overrides); } beforeEach(() => { @@ -417,123 +439,201 @@ describe("pruneStale", () => { }); }); -describe("adoptSessionForCurrentProcess (GIT-51)", () => { - it("adopts a session left by a previous incarnation of this process", () => { - const dead = makeEntry({ - session_id: "11111111-1111-1111-1111-111111111111", - hostname: os.hostname(), - pid: 99999999, - started_at: new Date().toISOString(), +describe("findResumableSessionOnDisk (GIT-89)", () => { + const DEAD_PID = 99999999; + + it("resolves a session the registry has lost entirely", () => { + // The GIT-89 regression. Observed in the field: active-sessions.json holding + // {"sessions": []} with intact sessions//session.json beside it. Every + // registry-gated path answered "no session" while the evidence sat on disk. + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { pid: DEAD_PID }); + expect(listActiveSessions()).toHaveLength(0); + + const resolved = findResumableSessionOnDisk(); + + expect(resolved).not.toBeNull(); + expect(resolved!.session_id).toBe("11111111-1111-1111-1111-111111111111"); + expect(resolved!.pid).toBe(process.pid); + }); + + it("repairs the registry from what it found on disk", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { pid: DEAD_PID }); + + findResumableSessionOnDisk(); + + const sessions = listActiveSessions(); + expect(sessions).toHaveLength(1); + expect(sessions[0].session_id).toBe("11111111-1111-1111-1111-111111111111"); + expect(sessions[0].pid).toBe(process.pid); + }); + + it("writes the rebound pid back to session.json so the next scan is a fast path", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { pid: DEAD_PID }); + + findResumableSessionOnDisk(); + + const raw = fs.readFileSync( + path.join(tmpDir, "sessions", "11111111-1111-1111-1111-111111111111", "session.json"), + "utf-8" + ); + expect(JSON.parse(raw).pid).toBe(process.pid); + }); + + it("preserves accumulated session state when rebinding the pid", () => { + // session.json accumulates surfaced_scars (recall) and threads + // (session_start). Rewriting the file from a registry entry would drop them. + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { + pid: DEAD_PID, + surfaced_scars: [{ scar_id: "abc123", scar_title: "keep me" }], + threads: [{ id: "t1", status: "open" }], }); - registerSession(dead); - createSessionFile(dead.session_id); - const adopted = adoptSessionForCurrentProcess(); + findResumableSessionOnDisk(); - expect(adopted).not.toBeNull(); - expect(adopted!.session_id).toBe(dead.session_id); - expect(adopted!.pid).toBe(process.pid); - expect(listActiveSessions()[0].pid).toBe(process.pid); + const data = JSON.parse( + fs.readFileSync( + path.join(tmpDir, "sessions", "11111111-1111-1111-1111-111111111111", "session.json"), + "utf-8" + ) + ); + expect(data.surfaced_scars).toHaveLength(1); + expect(data.surfaced_scars[0].scar_id).toBe("abc123"); + expect(data.threads).toHaveLength(1); + }); + + it("takes the own-pid fast path without adopting", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { pid: process.pid }); + + const resolved = findResumableSessionOnDisk(); + + expect(resolved!.session_id).toBe("11111111-1111-1111-1111-111111111111"); + expect(resolved!.pid).toBe(process.pid); }); - it("adopts a session older than 2 hours", () => { + it("resolves a session older than 2 hours", () => { // The old ADOPT_THRESHOLD_MS window was shorter than a normal working // session, so long sessions — the ones that most need recovery — were // excluded from it. - const dead = makeEntry({ - session_id: "11111111-1111-1111-1111-111111111111", - hostname: os.hostname(), - pid: 99999999, + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { + pid: DEAD_PID, started_at: new Date(Date.now() - 6 * 60 * 60 * 1000).toISOString(), }); - registerSession(dead); - createSessionFile(dead.session_id); - expect(adoptSessionForCurrentProcess()?.session_id).toBe(dead.session_id); + expect(findResumableSessionOnDisk()?.session_id).toBe("11111111-1111-1111-1111-111111111111"); }); - it("adopts at most one entry — never leaves two rows sharing hostname+pid", () => { - const first = makeEntry({ - session_id: "11111111-1111-1111-1111-111111111111", - hostname: os.hostname(), + it("adopts at most one session — never leaves two rows sharing hostname+pid", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { pid: 99999998, started_at: new Date(Date.now() - 60 * 60 * 1000).toISOString(), }); - const second = makeEntry({ - session_id: "22222222-2222-2222-2222-222222222222", - hostname: os.hostname(), - pid: 99999999, + seedDiskOnlySession("22222222-2222-2222-2222-222222222222", { + pid: DEAD_PID, started_at: new Date().toISOString(), }); - registerSession(first); - registerSession(second); - createSessionFile(first.session_id); - createSessionFile(second.session_id); - const adopted = adoptSessionForCurrentProcess(); + const resolved = findResumableSessionOnDisk(); - // Most recently started wins — deterministic, not array order. - expect(adopted!.session_id).toBe(second.session_id); + // Most recently started wins — deterministic, not directory-listing order. + expect(resolved!.session_id).toBe("22222222-2222-2222-2222-222222222222"); - const sessions = listActiveSessions(); - const mine = sessions.filter((s) => s.hostname === os.hostname() && s.pid === process.pid); + const mine = listActiveSessions().filter( + (s) => s.hostname === os.hostname() && s.pid === process.pid + ); expect(mine).toHaveLength(1); - expect(mine[0].session_id).toBe(second.session_id); + expect(mine[0].session_id).toBe("22222222-2222-2222-2222-222222222222"); }); - it("never claims a session whose PID is still alive", () => { - const live = makeEntry({ - session_id: "11111111-1111-1111-1111-111111111111", - hostname: os.hostname(), - pid: process.pid, // this process is alive + it("never claims a session owned by another live process", () => { + // GIT-20: never resume another process's session. This is what keeps + // GIT-19..23 multi-session resolution intact now that PID is not identity. + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { + pid: 1, // init — alive, and definitely not us }); - registerSession(live); - createSessionFile(live.session_id); - // findSessionByHostPid already resolves our own session; adoption must not - // also claim it (nor any other live process's session). - expect(adoptSessionForCurrentProcess()).toBeNull(); + expect(findResumableSessionOnDisk()).toBeNull(); }); - it("ignores dead-PID sessions on other hosts", () => { - const remote = makeEntry({ - session_id: "11111111-1111-1111-1111-111111111111", + it("resolves its own orphan while leaving a live neighbour alone", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { + pid: 1, // another live server's session + started_at: new Date().toISOString(), + }); + seedDiskOnlySession("22222222-2222-2222-2222-222222222222", { + pid: DEAD_PID, + started_at: new Date(Date.now() - 60 * 60 * 1000).toISOString(), + }); + + const resolved = findResumableSessionOnDisk(); + + expect(resolved!.session_id).toBe("22222222-2222-2222-2222-222222222222"); + + // The live neighbour's file is untouched. + const neighbour = JSON.parse( + fs.readFileSync( + path.join(tmpDir, "sessions", "11111111-1111-1111-1111-111111111111", "session.json"), + "utf-8" + ) + ); + expect(neighbour.pid).toBe(1); + }); + + it("ignores sessions from other hosts", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { hostname: "some-other-container", - pid: 99999999, + pid: DEAD_PID, }); - registerSession(remote); - createSessionFile(remote.session_id); - expect(adoptSessionForCurrentProcess()).toBeNull(); + expect(findResumableSessionOnDisk()).toBeNull(); }); - it("ignores entries with no session file on disk", () => { + it("ignores sessions past the 24h stale horizon", () => { + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { + pid: DEAD_PID, + started_at: new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(), + }); + + expect(findResumableSessionOnDisk()).toBeNull(); + }); + + it("ignores a registry entry with no session file on disk", () => { + // Inverted from the GIT-51 version: the registry is no longer evidence. registerSession( makeEntry({ session_id: "11111111-1111-1111-1111-111111111111", hostname: os.hostname(), - pid: 99999999, + pid: DEAD_PID, }) ); - expect(adoptSessionForCurrentProcess()).toBeNull(); + expect(findResumableSessionOnDisk()).toBeNull(); }); - it("ignores entries past the 24h stale horizon", () => { - const ancient = makeEntry({ - session_id: "11111111-1111-1111-1111-111111111111", - hostname: os.hostname(), - pid: 99999999, - started_at: new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(), + it("refuses a session.json whose id disagrees with its directory name", () => { + // A hand-copied or hand-edited directory. Resolving it would bind this + // process to the wrong session id. + seedDiskOnlySession("11111111-1111-1111-1111-111111111111", { + session_id: "99999999-9999-9999-9999-999999999999", + pid: DEAD_PID, }); - registerSession(ancient); - createSessionFile(ancient.session_id); - expect(adoptSessionForCurrentProcess()).toBeNull(); + expect(findResumableSessionOnDisk()).toBeNull(); + }); + + it("skips a corrupt session.json without failing the scan", () => { + const badDir = path.join(tmpDir, "sessions", "11111111-1111-1111-1111-111111111111"); + fs.mkdirSync(badDir, { recursive: true }); + fs.writeFileSync(path.join(badDir, "session.json"), "{ not json"); + + seedDiskOnlySession("22222222-2222-2222-2222-222222222222", { pid: DEAD_PID }); + + expect(findResumableSessionOnDisk()?.session_id).toBe( + "22222222-2222-2222-2222-222222222222" + ); }); - it("returns null on an empty registry", () => { - expect(adoptSessionForCurrentProcess()).toBeNull(); + it("returns null when there is nothing on disk", () => { + expect(findResumableSessionOnDisk()).toBeNull(); }); }); diff --git a/tests/unit/services/session-state-surfacing.test.ts b/tests/unit/services/session-state-surfacing.test.ts new file mode 100644 index 0000000..edb3040 --- /dev/null +++ b/tests/unit/services/session-state-surfacing.test.ts @@ -0,0 +1,248 @@ +/** + * GIT-89: recall/confirm_scars must fail (or succeed) together. + * + * The two halves of the protocol used to fail asymmetrically. recall() returned + * scars with a soft "No active session" banner and dropped them from tracking; + * confirm_scars() hard-rejected the same condition. The agent saw scars, acted + * on them, and a later confirm reported "no scars to confirm" — a green result + * that actually meant the surfacing had been discarded (scar 810a1624). + * + * addSurfacedScars was the discard point: it read the in-memory session + * directly, and on null logged a console warning and returned void. Nothing + * downstream could tell a tracked recall from an untracked one. + * + * These tests use the real registry and real session files in a temp dir — the + * defect was in how in-memory state, disk state, and the return contract relate. + */ + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import * as fs from "fs"; +import * as path from "path"; +import * as os from "os"; +import { setGitmemDir, clearGitmemDirCache } from "../../../src/services/gitmem-dir.js"; +import { + setCurrentSession, + clearCurrentSession, + addSurfacedScars, + getSurfacedScars, + setRecallCalled, + isRecallCalled, +} from "../../../src/services/session-state.js"; +import type { SurfacedScar } from "../../../src/types/index.js"; + +const HOSTNAME = os.hostname(); +const DEAD_PID = 99999999; + +let tmpDir: string; + +const SCARS: SurfacedScar[] = [ + { + scar_id: "aaaa1111-1111-1111-1111-111111111111", + scar_title: "Trace execution path first", + severity: "high", + surfaced_at: "2026-08-08T10:00:00.000Z", + source: "recall", + }, + { + scar_id: "bbbb2222-2222-2222-2222-222222222222", + scar_title: "Done != Deployed", + severity: "high", + surfaced_at: "2026-08-08T10:00:00.000Z", + source: "recall", + }, +] as SurfacedScar[]; + +function sessionFilePath(sessionId: string): string { + return path.join(tmpDir, "sessions", sessionId, "session.json"); +} + +/** Write a session.json as session_start would, with no registry entry. */ +function seedSessionFile(sessionId: string, overrides: Record = {}): void { + const dir = path.join(tmpDir, "sessions", sessionId); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + sessionFilePath(sessionId), + JSON.stringify({ + session_id: sessionId, + agent: "cli", + started_at: new Date().toISOString(), + hostname: HOSTNAME, + pid: process.pid, + project: "orchestra_dev", + surfaced_scars: [], + threads: [], + ...overrides, + }) + ); +} + +beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "gitmem-git89-")); + setGitmemDir(tmpDir); + clearCurrentSession(); +}); + +afterEach(() => { + clearCurrentSession(); + clearGitmemDirCache(); + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +describe("addSurfacedScars reports whether tracking happened (GIT-89)", () => { + it("returns false when there is genuinely no session", () => { + // The signal recall needs in order to warn as loudly as confirm_scars does, + // instead of printing scars as though they were recorded. + expect(addSurfacedScars(SCARS)).toBe(false); + }); + + it("returns true when the scars were tracked", () => { + const sessionId = "11111111-1111-1111-1111-111111111111"; + seedSessionFile(sessionId); + setCurrentSession({ + sessionId, + project: "orchestra_dev", + startedAt: new Date(), + }); + + expect(addSurfacedScars(SCARS)).toBe(true); + expect(getSurfacedScars()).toHaveLength(2); + }); + + it("tracks against a session recovered from disk rather than discarding", () => { + // The exact production shape: the MCP server restarted, so in-memory state + // is gone, but the session is alive and its file is on disk. Before GIT-89 + // this returned void after a console warning and the scars vanished. + const sessionId = "22222222-2222-2222-2222-222222222222"; + seedSessionFile(sessionId, { pid: DEAD_PID }); + + expect(addSurfacedScars(SCARS)).toBe(true); + expect(getSurfacedScars().map((s) => s.scar_id)).toEqual([ + "aaaa1111-1111-1111-1111-111111111111", + "bbbb2222-2222-2222-2222-222222222222", + ]); + }); + + it("deduplicates by scar_id across repeated recalls", () => { + const sessionId = "33333333-3333-3333-3333-333333333333"; + seedSessionFile(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + addSurfacedScars(SCARS); + addSurfacedScars(SCARS); + + expect(getSurfacedScars()).toHaveLength(2); + }); +}); + +describe("surfacing survives the process that recorded it (GIT-89)", () => { + it("writes surfaced scars through to session.json", () => { + // Surfacing held only in memory is lost to the next restart, which turns an + // identity break into a tracking break. + const sessionId = "44444444-4444-4444-4444-444444444444"; + seedSessionFile(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + addSurfacedScars(SCARS); + + const data = JSON.parse(fs.readFileSync(sessionFilePath(sessionId), "utf-8")); + expect(data.surfaced_scars).toHaveLength(2); + expect(data.surfaced_scars[0].scar_id).toBe("aaaa1111-1111-1111-1111-111111111111"); + }); + + it("write-through covers the free tier path too", () => { + // The old inline write lived in recall's pro-tier branch only, so free-tier + // surfacing was memory-only and did not survive a restart at all. + const sessionId = "55555555-5555-5555-5555-555555555555"; + seedSessionFile(sessionId, { pid: DEAD_PID }); + + expect(addSurfacedScars(SCARS)).toBe(true); + + const data = JSON.parse(fs.readFileSync(sessionFilePath(sessionId), "utf-8")); + expect(data.surfaced_scars).toHaveLength(2); + }); + + it("a restart between recall and confirm still sees the surfaced scars", () => { + const sessionId = "66666666-6666-6666-6666-666666666666"; + seedSessionFile(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + addSurfacedScars(SCARS); + + // MCP server restart: in-memory state dies, disk survives. + clearCurrentSession(); + + expect(getSurfacedScars()).toHaveLength(2); + }); + + it("preserves unrelated session state when persisting scars", () => { + const sessionId = "77777777-7777-7777-7777-777777777777"; + seedSessionFile(sessionId, { + threads: [{ id: "t1", status: "open" }], + recording_path: "/tmp/recording.jsonl", + }); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + addSurfacedScars(SCARS); + + const data = JSON.parse(fs.readFileSync(sessionFilePath(sessionId), "utf-8")); + expect(data.threads).toHaveLength(1); + expect(data.recording_path).toBe("/tmp/recording.jsonl"); + expect(data.surfaced_scars).toHaveLength(2); + }); + + it("does not fail the call when the session file is unwritable", () => { + // Persistence is best-effort — losing the write must not lose the tracking. + const sessionId = "88888888-8888-8888-8888-888888888888"; + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + // No session.json on disk at all. + expect(addSurfacedScars(SCARS)).toBe(true); + expect(getSurfacedScars()).toHaveLength(2); + }); +}); + +describe("recall_called survives a restart (GIT-89)", () => { + it("persists the flag to session.json", () => { + const sessionId = "aaaa0000-0000-0000-0000-000000000001"; + seedSessionFile(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + setRecallCalled(); + + const data = JSON.parse(fs.readFileSync(sessionFilePath(sessionId), "utf-8")); + expect(data.recall_called).toBe(true); + }); + + it("still reports recall as called after in-memory state is lost", () => { + // Enforcement Check 3 reads this flag. Held only in memory, every + // create_learning / create_decision / session_close after a restart warned + // "No recall() was run this session" in sessions where it plainly had — + // the same false-alarm class as the "No active session" banner. + const sessionId = "aaaa0000-0000-0000-0000-000000000002"; + seedSessionFile(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + setRecallCalled(); + + clearCurrentSession(); // MCP server restart + + expect(isRecallCalled()).toBe(true); + }); + + it("stays false when recall genuinely never ran", () => { + // The warning has to remain true when it fires. + const sessionId = "aaaa0000-0000-0000-0000-000000000003"; + seedSessionFile(sessionId, { pid: DEAD_PID }); + + expect(isRecallCalled()).toBe(false); + }); + + it("does not rewrite the file once the flag is already set", () => { + const sessionId = "aaaa0000-0000-0000-0000-000000000004"; + seedSessionFile(sessionId, { recall_called: true }); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + const before = fs.statSync(sessionFilePath(sessionId)).mtimeMs; + setRecallCalled(); + + expect(fs.statSync(sessionFilePath(sessionId)).mtimeMs).toBe(before); + }); +}); diff --git a/tests/unit/tools/recall-surfacing.test.ts b/tests/unit/tools/recall-surfacing.test.ts new file mode 100644 index 0000000..7ed384a --- /dev/null +++ b/tests/unit/tools/recall-surfacing.test.ts @@ -0,0 +1,173 @@ +/** + * GIT-89: recall and confirm_scars must fail together. + * + * recall used to return scars under a soft "No active session" banner while + * silently dropping them from tracking; confirm_scars hard-rejected the same + * condition, and a later confirm reported "no scars to confirm" — green output + * for scars that had just been discarded (scar 810a1624). Nothing in recall's + * response let an agent tell a tracked recall from an untracked one. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import * as fs from "fs"; +import * as path from "path"; +import * as os from "os"; +import { recall } from "../../../src/tools/recall.js"; +import * as supabase from "../../../src/services/supabase-client.js"; +import { setGitmemDir, clearGitmemDirCache } from "../../../src/services/gitmem-dir.js"; +import { setCurrentSession, clearCurrentSession } from "../../../src/services/session-state.js"; + +// Plain functions, not vi.fn().mockReturnValue(...). vi.mock factories are +// hoisted above the imports, and a mockReturnValue chained inside one resolves +// to undefined at call time — which silently drops recall onto its free-tier +// branch instead of the Supabase branch these tests stage. +vi.mock("../../../src/services/tier.js", () => ({ + getTier: () => "pro", + hasSupabase: () => true, + hasVariants: () => false, + hasMetrics: () => false, + hasCacheManagement: () => true, + hasCompliance: () => false, + hasTranscripts: () => false, + hasBatchOperations: () => false, + hasEmbeddings: () => true, + hasAdvancedAgentDetection: () => false, + hasMultiProject: () => false, + hasEnforcementFields: () => false, + hasProInsights: () => false, + getTablePrefix: () => "gitmem_", + getTableName: (base: string) => `gitmem_${base}`, +})); + +vi.mock("../../../src/services/supabase-client.js", () => ({ + isConfigured: vi.fn(), + cachedScarSearch: vi.fn(), + upsertRecord: async () => undefined, + directUpsert: async () => undefined, + fetchRelatedTriples: async () => new Map(), +})); + +// Force the Supabase branch. Otherwise recall prefers the local vector cache, +// whose results are not what these tests are staging. +vi.mock("../../../src/services/local-vector-search.js", () => ({ + isLocalSearchReady: () => false, + localScarSearch: async () => [], +})); + +const ONE_SCAR = { + results: [ + { + id: "test-scar-1", + title: "Test Scar", + description: "This is a test scar about deployment", + severity: "high", + counter_arguments: ["You might think it's easy"], + applies_when: ["deploying"], + similarity: 0.85, + }, + ], + cache_hit: false, +}; + +let tmpDir: string; + +beforeEach(() => { + vi.clearAllMocks(); + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "gitmem-git89-recall-")); + setGitmemDir(tmpDir); + clearCurrentSession(); +}); + +afterEach(() => { + clearCurrentSession(); + clearGitmemDirCache(); + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +/** Write a session.json as session_start would. */ +function seedSession(sessionId: string, overrides: Record = {}): void { + fs.mkdirSync(path.join(tmpDir, "sessions", sessionId), { recursive: true }); + fs.writeFileSync( + path.join(tmpDir, "sessions", sessionId, "session.json"), + JSON.stringify({ + session_id: sessionId, + agent: "cli", + started_at: new Date().toISOString(), + hostname: os.hostname(), + pid: process.pid, + project: "orchestra_dev", + surfaced_scars: [], + ...overrides, + }) + ); +} + +describe("recall surfacing-tracked signal (GIT-89)", () => { + it("warns that surfacing was not tracked when there is no session", async () => { + vi.mocked(supabase.isConfigured).mockReturnValue(true); + vi.mocked(supabase.cachedScarSearch).mockResolvedValue(ONE_SCAR); + + const result = await recall({ plan: "deploy to production" }); + + expect(result.scars).toHaveLength(1); + expect(result.formatted_response).toContain("SURFACING NOT TRACKED"); + expect(result.formatted_response).toContain("confirm_scars will reject them"); + }); + + it("stays silent when the scars were tracked", async () => { + // Zero token cost on the healthy path — the notice marks the broken case, + // it does not decorate every recall (scar 55dd6d73: audit payload weight). + vi.mocked(supabase.isConfigured).mockReturnValue(true); + vi.mocked(supabase.cachedScarSearch).mockResolvedValue(ONE_SCAR); + + const sessionId = "11111111-1111-1111-1111-111111111111"; + seedSession(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + const result = await recall({ plan: "deploy to production" }); + + expect(result.scars).toHaveLength(1); + expect(result.formatted_response).not.toContain("SURFACING NOT TRACKED"); + }); + + it("stays silent when the session was recovered after an MCP restart", async () => { + // The production shape: in-memory state died with the old process, the + // session file is intact, and identity resolves from disk. Surfacing is + // tracked, so there is nothing to warn about. + vi.mocked(supabase.isConfigured).mockReturnValue(true); + vi.mocked(supabase.cachedScarSearch).mockResolvedValue(ONE_SCAR); + + seedSession("22222222-2222-2222-2222-222222222222", { pid: 99999999 }); + + const result = await recall({ plan: "deploy to production" }); + + expect(result.formatted_response).not.toContain("SURFACING NOT TRACKED"); + }); + + it("persists the surfaced scars to the session file", async () => { + vi.mocked(supabase.isConfigured).mockReturnValue(true); + vi.mocked(supabase.cachedScarSearch).mockResolvedValue(ONE_SCAR); + + const sessionId = "33333333-3333-3333-3333-333333333333"; + seedSession(sessionId); + setCurrentSession({ sessionId, project: "orchestra_dev", startedAt: new Date() }); + + await recall({ plan: "deploy to production" }); + + const data = JSON.parse( + fs.readFileSync(path.join(tmpDir, "sessions", sessionId, "session.json"), "utf-8") + ); + expect(data.surfaced_scars).toHaveLength(1); + expect(data.surfaced_scars[0].scar_id).toBe("test-scar-1"); + }); + + it("does not warn when there were no scars to surface", async () => { + // Nothing was discarded, so there is nothing to warn about. + vi.mocked(supabase.isConfigured).mockReturnValue(true); + vi.mocked(supabase.cachedScarSearch).mockResolvedValue({ results: [], cache_hit: false }); + + const result = await recall({ plan: "unique task with no history" }); + + expect(result.formatted_response).not.toContain("SURFACING NOT TRACKED"); + }); +}); From 0223fc2026e13efbdea4be95fc94905fcdce5a72 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:16:21 -0400 Subject: [PATCH 2/6] docs: correct enforcement's "no I/O" claim invalidated by GIT-89 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/services/enforcement.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/services/enforcement.ts b/src/services/enforcement.ts index d65ea5d..d6a53ae 100644 --- a/src/services/enforcement.ts +++ b/src/services/enforcement.ts @@ -8,7 +8,15 @@ * - Advisory, not blocking: warnings append to responses, never prevent execution * - Zero overhead on compliant calls: only fires when state is missing * - Universal: works in ALL MCP clients, no IDE hooks needed - * - Lightweight: pure in-memory checks, no I/O + * - Lightweight: in-memory once identity is bound; at most one disk scan per + * process to recover it (GIT-89) + * + * The "no I/O" claim here predated GIT-89. getCurrentSession() now falls through + * to resolveCurrentSession(), which scans the per-session directories when + * in-memory identity is empty — the whole point being that a session survives an + * MCP restart. That scan is bounded, not per-call: it runs until identity binds, + * and a failed scan is memoised against the registry fingerprint so a genuinely + * session-less process does not re-scan on every tool call. */ import { getCurrentSession, hasUnconfirmedScars, getSurfacedScars, isRecallCalled } from "./session-state.js"; From 3823972f9808d744d17f9e31ebbe3880065e4c22 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:31:59 -0400 Subject: [PATCH 3/6] fix: make session_id optional so session_close can resolve identity (GIT-89 AC#4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/schemas/session-close.ts | 9 +- src/tools/session-close.ts | 26 ++++ tests/unit/schemas/registry.test.ts | 11 +- tests/unit/schemas/session-close.test.ts | 15 ++- .../unit/tools/session-close-identity.test.ts | 114 ++++++++++++++++++ 5 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 tests/unit/tools/session-close-identity.test.ts diff --git a/src/schemas/session-close.ts b/src/schemas/session-close.ts index 3f8bc43..0a2fc5e 100644 --- a/src/schemas/session-close.ts +++ b/src/schemas/session-close.ts @@ -86,7 +86,14 @@ const SafeTranscriptPathSchema = z.string().refine( ); export const SessionCloseParamsSchema = z.object({ - session_id: SessionIdSchema, + // GIT-89 AC#4: optional, because session_close is expected to resolve the + // active session itself after an MCP restart. sessionClose() was always + // written for that — it guards `params.session_id &&` before validating and + // recovers identity when absent — but this field was required, so the MCP + // layer rejected the call with "session_id: Required" and the recovery branch + // could never execute. The agent had to already know the id, which is exactly + // what a restart (and context compaction) takes away. + session_id: SessionIdSchema.optional(), close_type: CloseTypeSchema, task_completion: TaskCompletionSchema.optional(), closing_reflection: ClosingReflectionSchema.optional(), diff --git a/src/tools/session-close.ts b/src/tools/session-close.ts index 4f988d2..42fc99e 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -874,6 +874,32 @@ export async function sessionClose( // Legacy active-session.json fallback removed — registry is the source of truth } + // GIT-89 AC#4: resolution can still come up empty — no session was ever + // started, or its directory is gone. Fail here with something actionable + // rather than letting an undefined id reach Supabase and surface as a + // "session not found" that reads like data loss. + if (!params.session_id && params.close_type !== "retroactive") { + const latencyMs = timer.stop(); + return { + success: false, + session_id: "", + close_compliance: { + close_type: params.close_type, + agent: "Unknown", + checklist_displayed: false, + questions_answered_by_agent: false, + human_asked_for_corrections: false, + learnings_stored: 0, + scars_applied: 0, + }, + validation_errors: [ + "No session_id provided and no active session could be resolved from disk. " + + "Run session_start first, or pass session_id explicitly.", + ], + performance: buildPerformanceData("session_close", latencyMs, 0), + }; + } + // 0a. File-based payload handoff: if .gitmem/closing-payload.json exists, // merge it with inline params (inline params take precedence). // This keeps the visible MCP tool call small: just session_id + close_type. diff --git a/tests/unit/schemas/registry.test.ts b/tests/unit/schemas/registry.test.ts index 88e7d1a..6eb693e 100644 --- a/tests/unit/schemas/registry.test.ts +++ b/tests/unit/schemas/registry.test.ts @@ -44,12 +44,19 @@ describe("validateToolArgs", () => { expect(error).toBeNull(); }); - it("returns error for missing required field", () => { + // GIT-89 AC#4: session_close must be callable without session_id so it can + // resolve the active session itself after an MCP restart. + it("accepts session_close without session_id", () => { const error = validateToolArgs("session_close", { close_type: "quick", }); + expect(error).toBeNull(); + }); + + it("returns error for missing required field", () => { + const error = validateToolArgs("session_close", {}); expect(error).not.toBeNull(); - expect(error).toContain("session_id"); + expect(error).toContain("close_type"); }); it("returns error for invalid close_type", () => { diff --git a/tests/unit/schemas/session-close.test.ts b/tests/unit/schemas/session-close.test.ts index 8fab060..afe7a8b 100644 --- a/tests/unit/schemas/session-close.test.ts +++ b/tests/unit/schemas/session-close.test.ts @@ -89,10 +89,23 @@ describe("SessionCloseParamsSchema", () => { }); describe("required params missing", () => { - it("rejects missing session_id", () => { + // GIT-89 AC#4: session_id is optional — sessionClose() resolves the active + // session from disk when it is omitted. This test previously asserted the + // opposite, which is what kept the recovery branch unreachable: the MCP + // layer rejected the call with "session_id: Required" before sessionClose() + // ran, so an agent that lost the id to a restart could not close at all. + it("accepts missing session_id (resolved from disk at close time)", () => { const result = SessionCloseParamsSchema.safeParse({ close_type: "quick", }); + expect(result.success).toBe(true); + }); + + it("still rejects a malformed session_id when one is supplied", () => { + const result = SessionCloseParamsSchema.safeParse({ + session_id: "not-a-uuid", + close_type: "quick", + }); expect(result.success).toBe(false); }); diff --git a/tests/unit/tools/session-close-identity.test.ts b/tests/unit/tools/session-close-identity.test.ts new file mode 100644 index 0000000..c62fb6c --- /dev/null +++ b/tests/unit/tools/session-close-identity.test.ts @@ -0,0 +1,114 @@ +/** + * GIT-89 AC#4: session_close must resolve the correct session after an MCP + * restart without the agent passing session_id. + * + * The runtime was always written for this — sessionClose() guards + * `params.session_id &&` before validating the format, and recovers identity + * when it is absent — but SessionCloseParamsSchema marked the field required. + * The MCP layer therefore rejected the call with "session_id: Required" before + * sessionClose() ever ran, so the recovery branch was unreachable in the only + * scenario it existed for. A restart (or a context compaction) takes the id + * away from the agent, which is precisely when close matters most: the failure + * lands after the reflection has been written and has nowhere to go. + * + * These tests pin both halves — the schema contract and the disk resolution — + * because fixing either one alone leaves the acceptance criterion unmet. + */ + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import * as fs from "fs"; +import * as path from "path"; +import * as os from "os"; +import { setGitmemDir, clearGitmemDirCache } from "../../../src/services/gitmem-dir.js"; +import { clearCurrentSession, resolveCurrentSession } from "../../../src/services/session-state.js"; +import { SessionCloseParamsSchema } from "../../../src/schemas/session-close.js"; + +const SESSION_ID = "7f3a9c21-4b5d-4e6f-8a90-1b2c3d4e5f60"; +let tmpRoot: string; + +/** A session left on disk by a process that no longer exists — i.e. a restart. */ +function seedOrphanedSession(sessionId: string, deadPid: number): void { + const dir = path.join(tmpRoot, "sessions", sessionId); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, "session.json"), + JSON.stringify({ + session_id: sessionId, + agent: "cli", + project: "gitmem", + started_at: new Date(Date.now() - 60 * 60 * 1000).toISOString(), + hostname: os.hostname(), + pid: deadPid, + host_pid: deadPid, + surfaced_scars: [{ scar_id: "535e0e42", title: "registry cannot rescue a lost registry" }], + recall_called: true, + }) + ); + // The registry is empty — the store that gets lost on restart. + fs.writeFileSync(path.join(tmpRoot, "active-sessions.json"), JSON.stringify({ sessions: [] })); +} + +describe("GIT-89 AC#4: session_close resolves identity after a restart", () => { + beforeEach(() => { + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), "gitmem-close-identity-")); + clearGitmemDirCache(); + setGitmemDir(tmpRoot); + clearCurrentSession(); + }); + + afterEach(() => { + clearCurrentSession(); + clearGitmemDirCache(); + fs.rmSync(tmpRoot, { recursive: true, force: true }); + }); + + describe("schema contract", () => { + it("accepts a close with no session_id, so the recovery branch is reachable", () => { + const result = SessionCloseParamsSchema.safeParse({ close_type: "quick" }); + expect(result.success).toBe(true); + }); + + it("still rejects a malformed session_id when one is supplied", () => { + const result = SessionCloseParamsSchema.safeParse({ + session_id: "../../etc/passwd", + close_type: "quick", + }); + expect(result.success).toBe(false); + }); + + it("accepts a well-formed session_id", () => { + const result = SessionCloseParamsSchema.safeParse({ + session_id: SESSION_ID, + close_type: "quick", + }); + expect(result.success).toBe(true); + }); + }); + + describe("disk resolution", () => { + it("recovers the orphaned session id that close would otherwise have to be told", () => { + seedOrphanedSession(SESSION_ID, 999_991); + + const resolved = resolveCurrentSession(); + + expect(resolved).not.toBeNull(); + expect(resolved?.sessionId).toBe(SESSION_ID); + }); + + it("carries the surfacing forward, so a close after a restart still reflects it", () => { + seedOrphanedSession(SESSION_ID, 999_991); + + const resolved = resolveCurrentSession(); + + expect(resolved?.surfacedScars).toHaveLength(1); + expect(resolved?.recallCalled).toBe(true); + }); + + it("returns null when there is genuinely no session, rather than inventing one", () => { + fs.mkdirSync(path.join(tmpRoot, "sessions"), { recursive: true }); + fs.writeFileSync(path.join(tmpRoot, "active-sessions.json"), JSON.stringify({ sessions: [] })); + + expect(resolveCurrentSession()).toBeNull(); + }); + }); +}); From 0e85a7f7ab344c8b595c734e8f1120a4e39896bf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:37:00 -0400 Subject: [PATCH 4/6] test: e2e coverage for session identity across a real MCP restart (GIT-89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/e2e/git-89-session-identity.test.ts | 208 ++++++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 tests/e2e/git-89-session-identity.test.ts diff --git a/tests/e2e/git-89-session-identity.test.ts b/tests/e2e/git-89-session-identity.test.ts new file mode 100644 index 0000000..eb8795d --- /dev/null +++ b/tests/e2e/git-89-session-identity.test.ts @@ -0,0 +1,208 @@ +/** + * GIT-89 E2E: a session must survive a real MCP server restart. + * + * The reported defect is not reproducible in-process. Session identity lives in + * a long-lived server process; what breaks it is that process dying and a new + * one taking over while the on-disk state stays put. Every assertion here + * therefore runs against a genuinely restarted server over the real MCP stdio + * protocol — the same surface an agent uses — rather than by calling the + * resolver directly. An in-process test of this bug proves nothing, because the + * thing that fails is the process boundary. + * + * Covers the acceptance criteria that can be checked without a scar corpus: + * AC#2 session survives restart with zero false "No active session" warnings + * AC#4 session_close resolves the session without being told its id + * AC#5 a genuinely absent session still warns (no invented sessions) + * + * AC#3 (surfacing continuity) is deliberately NOT asserted here. recall() + * surfaces nothing against an empty free-tier store, so an assertion would read + * `before=0, after=0` and pass without testing anything — a green result that + * means the instrument had no material to work with. It is covered at unit + * level in tests/unit/services/session-state-surfacing.test.ts, where the + * surfacing can be seeded. Restoring it here needs a store with real scars. + */ + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, rmSync, existsSync, readFileSync, mkdirSync, writeFileSync } from "fs"; +import { join } from "path"; +import { tmpdir } from "os"; +import { + createMcpClient, + callTool, + restartServer, + getToolResultText, + createTierEnv, + type McpTestClient, +} from "./mcp-client.js"; + +const NO_ACTIVE_SESSION = /No active session/i; +const UUID = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/i; + +/** Tools that must never report a missing session while one is live on disk. */ +const SESSION_REQUIRED_TOOLS = [ + { name: "recall", args: { plan: "continue the work after a restart", project: "gitmem" } }, + { + name: "create_learning", + args: { + title: "GIT-89 post-restart probe", + learning_type: "pattern", + description: "Written after a real MCP server restart.", + project: "gitmem", + }, + }, + { + name: "create_thread", + args: { title: "GIT-89 post-restart thread", description: "Written after a restart.", project: "gitmem" }, + }, + { + name: "create_decision", + args: { + title: "GIT-89 post-restart decision", + decision: "Resolve identity from disk.", + rationale: "The registry is the store that gets lost.", + project: "gitmem", + }, + }, +] as const; + +describe("GIT-89: session identity survives an MCP server restart", () => { + let root: string; + let env: Record; + let mcp: McpTestClient; + + beforeEach(async () => { + root = mkdtempSync(join(tmpdir(), "gitmem-git89-e2e-")); + // Isolate the store: this suite must never touch the developer's real + // .gitmem, and must not depend on what happens to be in it (GIT-92). + env = { ...createTierEnv("free"), GITMEM_DIR: root, HOME: root }; + mcp = await createMcpClient(env, { cwd: root }); + }); + + afterEach(async () => { + await mcp.cleanup(); + rmSync(root, { recursive: true, force: true }); + }); + + const readSessionFile = (id: string): Record | null => { + const p = join(root, "sessions", id, "session.json"); + return existsSync(p) ? JSON.parse(readFileSync(p, "utf-8")) : null; + }; + + const startSession = async (): Promise => { + const res = await callTool(mcp.client, "session_start", { project: "gitmem", agent_identity: "cli" }); + const id = getToolResultText(res).match(UUID)?.[0]; + expect(id, "session_start must return a session id").toBeTruthy(); + return id!; + }; + + // NOTE: assertions here read the PID out of the registry rather than off + // McpTestClient.process. That field is typed ChildProcess but createMcpClient + // never assigns it (`process: serverProcess!` — serverProcess stays null), so + // touching it throws. The registry PID is also the better witness: it is what + // the product actually wrote, not what the test harness happens to know. + const registryPidFor = (sessionId: string): number | undefined => { + const registry = JSON.parse(readFileSync(join(root, "active-sessions.json"), "utf-8")); + return registry.sessions?.find((s: { session_id: string }) => s.session_id === sessionId)?.pid; + }; + + const isAlive = (pid: number): boolean => { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } + }; + + it("keeps the same session id across a restart", async () => { + const sessionId = await startSession(); + const pidBefore = registryPidFor(sessionId); + + mcp = await restartServer(mcp, env, { cwd: root }); + + // Identity is re-derived by the new process, not carried in memory. + const after = await callTool(mcp.client, "recall", { plan: "resume", project: "gitmem" }); + expect(getToolResultText(after)).not.toMatch(NO_ACTIVE_SESSION); + expect(readSessionFile(sessionId)).not.toBeNull(); + expect(registryPidFor(sessionId)).not.toBe(pidBefore); + }); + + it.each(SESSION_REQUIRED_TOOLS.map((t) => [t.name, t.args] as const))( + "AC#2: %s does not warn about a missing session after a restart", + async (name, args) => { + await startSession(); + mcp = await restartServer(mcp, env, { cwd: root }); + + const text = getToolResultText(await callTool(mcp.client, name, args as Record)); + + expect(text).not.toMatch(NO_ACTIVE_SESSION); + } + ); + + it("AC#2: repairs the on-disk registry to the new, live process", async () => { + const sessionId = await startSession(); + const pidBefore = registryPidFor(sessionId); + expect(pidBefore, "session_start must register the session").toBeTruthy(); + + mcp = await restartServer(mcp, env, { cwd: root }); + await callTool(mcp.client, "recall", { plan: "resume", project: "gitmem" }); + + const pidAfter = registryPidFor(sessionId); + + // The entry must be reclaimed, not merely left behind: a stale PID here is + // the original defect — the registry pointing at a process that is gone. + expect(pidAfter, "the restarted process must reclaim the registry entry").toBeTruthy(); + expect(pidAfter).not.toBe(pidBefore); + expect(isAlive(pidAfter!), "the reclaimed PID must be a live process").toBe(true); + }); + + it("AC#2: restores recall_called, so writes are not told recall never ran", async () => { + const sessionId = await startSession(); + await callTool(mcp.client, "recall", { plan: "before the restart", project: "gitmem" }); + + mcp = await restartServer(mcp, env, { cwd: root }); + await callTool(mcp.client, "create_learning", { + title: "GIT-89 recall_called probe", + learning_type: "pattern", + description: "probe", + project: "gitmem", + }); + + expect(readSessionFile(sessionId)?.recall_called).toBe(true); + }); + + it("AC#4: session_close resolves the session without being passed its id", async () => { + await startSession(); + mcp = await restartServer(mcp, env, { cwd: root }); + + // No session_id — the agent has lost it to the restart, which is the point. + const res = await callTool(mcp.client, "session_close", { close_type: "quick" }); + const text = getToolResultText(res); + + // Assert on the failure shapes this specifically regressed through: a schema + // rejection ("session_id: Required") never reaches the resolution logic, and + // an {"error": ...} body is not a close. Checking only for absence of the + // words "No active session" passed this test while it was broken. + expect(res.isError ?? false).toBe(false); + expect(text).not.toMatch(/session_id.*Required/i); + expect(text).not.toMatch(/Invalid parameters/i); + expect(text).not.toMatch(NO_ACTIVE_SESSION); + }); + + it("AC#5: still warns when there is genuinely no session", async () => { + // Fresh store, no session_start — a real absence, not a lost identity. + mkdirSync(join(root, "sessions"), { recursive: true }); + writeFileSync(join(root, "active-sessions.json"), JSON.stringify({ sessions: [] })); + + const text = getToolResultText( + await callTool(mcp.client, "create_learning", { + title: "GIT-89 absent-session probe", + learning_type: "pattern", + description: "probe", + project: "gitmem", + }) + ); + + expect(text).toMatch(NO_ACTIVE_SESSION); + }); +}); From c8f316de0327c861af1536bf98c5f043a7046e83 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:37:46 -0400 Subject: [PATCH 5/6] ci: gate publish on the GIT-89 restart e2e (GIT-89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/ci.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 14b008f..f672779 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,15 @@ jobs: - name: Smoke test (free tier) run: npm run test:smoke:free + # GIT-89: session identity across a real MCP server restart. Runs the + # single e2e file rather than `npm run test:e2e`, because the Pro suites in + # that config gate on Docker/Supabase and hang rather than skip when + # neither is present (GIT-90). This file spawns the built server directly + # on the free tier — no Docker, no credentials — so it is safe in CI, and + # the publish job gates on this one passing. + - name: E2E — session identity survives MCP restart + run: npx vitest run --config vitest.e2e.config.ts tests/e2e/git-89-session-identity.test.ts + - name: Dependency audit (no unused deps) run: npx depcheck --ignores="@types/*" --json | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8')); const unused=d.dependencies||[]; if(unused.length){console.error('Unused dependencies:',unused);process.exit(1)}" From 9215fecb5d96b1a61464c94bd5d57258da785034 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:41:28 -0400 Subject: [PATCH 6/6] fix: unbreak the clean-room images (npm@latest vs node 20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- testing/clean-room/Dockerfile.claude-local | 7 +++++-- testing/clean-room/Dockerfile.claude-npm | 7 +++++-- testing/clean-room/Dockerfile.cursor-local | 7 +++++-- testing/clean-room/Dockerfile.cursor-npm | 7 +++++-- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/testing/clean-room/Dockerfile.claude-local b/testing/clean-room/Dockerfile.claude-local index b24f463..56424a2 100644 --- a/testing/clean-room/Dockerfile.claude-local +++ b/testing/clean-room/Dockerfile.claude-local @@ -18,8 +18,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ jq \ && rm -rf /var/lib/apt/lists/* -# Keep npm current (suppresses upgrade nag for users) -RUN npm install -g npm@latest +# NOTE: previously `RUN npm install -g npm@latest` here, to suppress the npm +# upgrade nag. It broke the image outright once npm@latest began requiring +# node >=22.22.2 while this base is node 20 — every clean-room build failed at +# this layer, which took out the pre-publish gate. The line was cosmetic; the +# npm bundled with the base image is what a real user on node 20 would have. # Install Claude Code (as root, before user switch) RUN npm install -g @anthropic-ai/claude-code diff --git a/testing/clean-room/Dockerfile.claude-npm b/testing/clean-room/Dockerfile.claude-npm index 9943a44..63dd721 100644 --- a/testing/clean-room/Dockerfile.claude-npm +++ b/testing/clean-room/Dockerfile.claude-npm @@ -18,8 +18,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ jq \ && rm -rf /var/lib/apt/lists/* -# Keep npm current (suppresses upgrade nag for users) -RUN npm install -g npm@latest +# NOTE: previously `RUN npm install -g npm@latest` here, to suppress the npm +# upgrade nag. It broke the image outright once npm@latest began requiring +# node >=22.22.2 while this base is node 20 — every clean-room build failed at +# this layer, which took out the pre-publish gate. The line was cosmetic; the +# npm bundled with the base image is what a real user on node 20 would have. # Install Claude Code (as root, before user switch) RUN npm install -g @anthropic-ai/claude-code diff --git a/testing/clean-room/Dockerfile.cursor-local b/testing/clean-room/Dockerfile.cursor-local index 35badeb..0e2b15b 100644 --- a/testing/clean-room/Dockerfile.cursor-local +++ b/testing/clean-room/Dockerfile.cursor-local @@ -22,8 +22,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ && rm -rf /var/lib/apt/lists/* -# Keep npm current (suppresses upgrade nag for users) -RUN npm install -g npm@latest +# NOTE: previously `RUN npm install -g npm@latest` here, to suppress the npm +# upgrade nag. It broke the image outright once npm@latest began requiring +# node >=22.22.2 while this base is node 20 — every clean-room build failed at +# this layer, which took out the pre-publish gate. The line was cosmetic; the +# npm bundled with the base image is what a real user on node 20 would have. # Install Cursor CLI (as root, before user switch) RUN curl https://cursor.com/install -fsS | bash diff --git a/testing/clean-room/Dockerfile.cursor-npm b/testing/clean-room/Dockerfile.cursor-npm index bb5f506..456213c 100644 --- a/testing/clean-room/Dockerfile.cursor-npm +++ b/testing/clean-room/Dockerfile.cursor-npm @@ -22,8 +22,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ && rm -rf /var/lib/apt/lists/* -# Keep npm current (suppresses upgrade nag for users) -RUN npm install -g npm@latest +# NOTE: previously `RUN npm install -g npm@latest` here, to suppress the npm +# upgrade nag. It broke the image outright once npm@latest began requiring +# node >=22.22.2 while this base is node 20 — every clean-room build failed at +# this layer, which took out the pre-publish gate. The line was cosmetic; the +# npm bundled with the base image is what a real user on node 20 would have. # Install Cursor CLI (as root, before user switch) RUN curl https://cursor.com/install -fsS | bash