From dac99b89f6b2c269d5b4438a7c5d4cbe9b318f3b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 23:08:28 -0400 Subject: [PATCH] fix: confirm_scars must not green-light a failed retrieval (GIT-93 step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty scar set had two causes and one answer. recall() ran, reached the store and matched nothing — proceeding is correct. Or recall() never reached the store — nothing was checked and any warning that applies is still unseen. Both produced: ok No recall-surfaced scars to confirm. Proceed freely. This is why step 1 could hide for as long as it did. The *_scar_search RPC 404'd on every call from the day it was written, and the protocol reported success for that failure, so nothing accumulated into a signal anyone could notice. Fixing the RPC name without fixing the signal would leave the next such break equally well hidden — which is the actual defect worth removing. recall now records a failure marker on the session when the store cannot be reached, and clears it when a search succeeds. Cleared on success rather than on attempt, and independent of how many scars came back: a search that ran and matched nothing is a real answer; a failed one is not an answer at all. confirm_scars reads that marker before answering an empty set. On a failure it returns valid: false, names the underlying error, and says plainly that this is not the same as no relevant scars. The marker persists to session.json and is restored on recovery, following recall_called. A restart is exactly the event this state exists to survive, so it must not be the thing that launders a broken store into a clean slate. Verified end to end: with a prefix whose match_ function does not exist, recall 404s and confirm_scars returns REJECTED quoting PGRST202, where it previously returned "Proceed freely". +6 tests (1185 -> 1191), including the restart case, and confirmed to fail against the pre-fix behaviour (2 of 6). provenance-citation.test.ts mocks session-state and needed the two new exports added to its mock. Co-Authored-By: Claude Opus 5 --- src/services/session-state.ts | 82 ++++++++++ src/tools/confirm-scars.ts | 34 +++++ src/tools/recall.ts | 16 +- .../confirm-scars-retrieval-failure.test.ts | 143 ++++++++++++++++++ tests/unit/tools/provenance-citation.test.ts | 4 + 5 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 tests/unit/tools/confirm-scars-retrieval-failure.test.ts diff --git a/src/services/session-state.ts b/src/services/session-state.ts index 9fca222..7d342de 100644 --- a/src/services/session-state.ts +++ b/src/services/session-state.ts @@ -38,6 +38,18 @@ interface SessionContext { * conflict rather than on agreement. */ recoveryConflict?: boolean; + /** + * GIT-93: set when a recall() attempt failed to reach the store, cleared when + * one succeeds. + * + * "No scars surfaced" has two causes that used to be indistinguishable: + * retrieval ran and found nothing relevant, or retrieval never ran. Only the + * first is safe to proceed on. Without this, confirm_scars answered both with + * "Proceed freely" — a green result for a broken store, which is how a + * retrieval path that had never worked survived indefinitely (the *_scar_search + * RPC 404'd on every call since it was written and nothing downstream said so). + */ + recallFailure?: { message: string; at: string } | null; } // Global session state (single active session per MCP server instance) @@ -165,6 +177,13 @@ function recoverSessionFromDisk(): SessionContext | null { currentSession.recallCalled = true; } + // GIT-93: restore an unresolved retrieval failure too. A restart must not + // launder a broken store into a clean slate — otherwise the very event this + // session state exists to survive becomes a way to lose the warning. + if (currentSession && data.recall_failure) { + currentSession.recallFailure = data.recall_failure; + } + console.error( `[session-state] Recovered session ${data.session_id.slice(0, 8)} from disk after MCP restart ` + `(${currentSession?.surfacedScars.length ?? 0} surfaced scars)` @@ -265,6 +284,69 @@ export function setRecallCalled(): void { } } +/** + * GIT-93: record that a recall() attempt could not reach the store. + * + * Persisted alongside recall_called so it survives an MCP restart, for the same + * reason: the failure outlives the process that observed it, and a restart must + * not turn a broken store into an apparently clean one. + */ +export function setRecallFailure(message: string): void { + const session = resolveCurrentSession(); + if (!session) return; + + const failure = { message, at: new Date().toISOString() }; + session.recallFailure = failure; + console.error(`[session-state] recall() failure recorded: ${message.slice(0, 120)}`); + + try { + const sessionFilePath = getSessionPath(session.sessionId, "session.json"); + if (!fs.existsSync(sessionFilePath)) return; + const data = JSON.parse(fs.readFileSync(sessionFilePath, "utf-8")); + data.recall_failure = failure; + 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_failure:", error); + } +} + +/** + * GIT-93: clear the failure marker after a recall() that reached the store. + * + * Cleared on success rather than on attempt, and regardless of how many scars + * came back: a successful search returning nothing is a real answer, while a + * failed one is not an answer at all. Only the former should let confirm_scars + * say "proceed". + */ +export function clearRecallFailure(): void { + const session = resolveCurrentSession(); + if (!session || !session.recallFailure) return; + + session.recallFailure = null; + + 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_failure) return; // nothing recorded — no write + delete data.recall_failure; + fs.writeFileSync(sessionFilePath, JSON.stringify(data, null, 2)); + } catch (error) { + console.warn("[session-state] Failed to clear recall_failure:", error); + } +} + +/** + * GIT-93: the unresolved retrieval failure for this session, if any. + * + * Returns null when retrieval last succeeded — which is the only state in which + * an empty scar set means "nothing relevant" rather than "nothing was asked". + */ +export function getRecallFailure(): { message: string; at: string } | null { + return resolveCurrentSession()?.recallFailure ?? null; +} + /** * Check if recall() was called this session. * Used by enforcement to avoid false positives when recall returns 0 scars. diff --git a/src/tools/confirm-scars.ts b/src/tools/confirm-scars.ts index cc0f5c5..d9a468d 100644 --- a/src/tools/confirm-scars.ts +++ b/src/tools/confirm-scars.ts @@ -21,6 +21,7 @@ import { getSurfacedScars, addConfirmations, getConfirmations, + getRecallFailure, } from "../services/session-state.js"; import { Timer, buildPerformanceData } from "../services/metrics.js"; import { getSessionPath } from "../services/gitmem-dir.js"; @@ -185,6 +186,39 @@ export async function confirmScars(params: ConfirmScarsParams): Promise { const rawScars = await getStorage().search(plan, matchCount); const searchLatencyMs = searchTimer.stop(); + // GIT-93: free-tier local search also answered — same rule as the Pro path. + clearRecallFailure(); + const scars: FormattedScar[] = rawScars .map((scar) => ({ id: scar.id, @@ -563,6 +566,11 @@ export async function recall(params: RecallParams): Promise { const searchLatencyMs = searchTimer.stop(); + // GIT-93: the store answered. Clear any earlier failure regardless of how + // many scars came back — a search that ran and matched nothing is a real + // answer, and only that state may license confirm_scars to say "proceed". + clearRecallFailure(); + // Assign variants for A/B testing (dev tier only) // Agent identity is always available, so variants are always assigned const variantTimer = new Timer(); @@ -746,6 +754,12 @@ export async function recall(params: RecallParams): Promise { const message = error instanceof Error ? error.message : String(error); console.error("[recall] Search failed:", message); + // GIT-93: record the failure on the session, not just in stderr. This + // returns scars: [] with a warning banner, and a later confirm_scars sees + // only "no scars surfaced" — which it used to answer with "Proceed freely". + // The marker is what lets it tell an empty answer from no answer. + setRecallFailure(message); + const latencyMs = timer.stop(); const perfData = buildPerformanceData("recall", latencyMs, 0); const mainErrMsg = `⚠️ Error querying institutional memory: ${message}`; diff --git a/tests/unit/tools/confirm-scars-retrieval-failure.test.ts b/tests/unit/tools/confirm-scars-retrieval-failure.test.ts new file mode 100644 index 0000000..a4835cc --- /dev/null +++ b/tests/unit/tools/confirm-scars-retrieval-failure.test.ts @@ -0,0 +1,143 @@ +/** + * GIT-93 step 2: a failed retrieval must not read as a clean check. + * + * confirm_scars answered an empty scar set with "No recall-surfaced scars to + * confirm. Proceed freely." — regardless of why the set was empty. Two very + * different states produced that identical green result: + * + * 1. recall() ran, reached the store, matched nothing. Proceeding is correct. + * 2. recall() never reached the store. Nothing was checked, and any warning + * that applies is still unseen. Proceeding is a guess. + * + * The second is not hypothetical: the *_scar_search RPC 404'd on every call from + * the day it was written, and the reason it survived that long is that the + * system reported success for the failure. Fixing the RPC name without fixing + * this signal would leave the next such break just as well hidden. + * + * These tests drive real session state on disk — the marker has to survive a + * process restart, so an in-memory-only assertion would not prove much. + */ + +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, + setRecallFailure, + clearRecallFailure, + getRecallFailure, + addSurfacedScars, + resolveCurrentSession, +} from "../../../src/services/session-state.js"; +import { confirmScars } from "../../../src/tools/confirm-scars.js"; + +const SESSION_ID = "3c9f21ab-77de-4a10-9f31-2b8c4d5e6f70"; +let root: string; + +function startSession(): void { + fs.mkdirSync(path.join(root, "sessions", SESSION_ID), { recursive: true }); + fs.writeFileSync( + path.join(root, "sessions", SESSION_ID, "session.json"), + JSON.stringify({ + session_id: SESSION_ID, + agent: "cli", + project: "gitmem", + started_at: new Date().toISOString(), + hostname: os.hostname(), + pid: process.pid, + host_pid: process.pid, + }) + ); + fs.writeFileSync(path.join(root, "active-sessions.json"), JSON.stringify({ sessions: [] })); + setCurrentSession({ + sessionId: SESSION_ID, + agent: "cli", + project: "gitmem", + startedAt: new Date(), + }); +} + +describe("GIT-93: confirm_scars distinguishes an empty answer from no answer", () => { + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), "gitmem-git93-")); + clearGitmemDirCache(); + setGitmemDir(root); + clearCurrentSession(); + startSession(); + }); + + afterEach(() => { + clearCurrentSession(); + clearGitmemDirCache(); + fs.rmSync(root, { recursive: true, force: true }); + }); + + it("still says proceed when retrieval succeeded and matched nothing", async () => { + // No failure recorded — recall ran and simply found nothing relevant. + const result = await confirmScars({ confirmations: [] }); + + expect(result.valid).toBe(true); + expect(result.formatted_response).toContain("Proceed freely"); + }); + + it("refuses to say proceed when retrieval never reached the store", async () => { + setRecallFailure("Supabase RPC error: 404 - PGRST202"); + + const result = await confirmScars({ confirmations: [] }); + + expect(result.valid).toBe(false); + expect(result.formatted_response).not.toContain("Proceed freely"); + expect(result.errors.join(" ")).toMatch(/recall\(\) failed|unavailable/i); + }); + + it("names the underlying failure rather than reporting a generic error", async () => { + setRecallFailure("Supabase RPC error: 404 - PGRST202"); + + const result = await confirmScars({ confirmations: [] }); + + // A diagnosable message is the difference between this being noticed in a + // day and being noticed never. + expect(result.formatted_response).toContain("PGRST202"); + }); + + it("says proceed again once a later recall reaches the store", async () => { + setRecallFailure("transient network error"); + clearRecallFailure(); + + const result = await confirmScars({ confirmations: [] }); + + expect(result.valid).toBe(true); + expect(result.formatted_response).toContain("Proceed freely"); + }); + + it("keeps the failure marker across an MCP restart", () => { + setRecallFailure("Supabase RPC error: 404 - PGRST202"); + + // Simulate the restart: in-memory state dies, disk survives. + clearCurrentSession(); + const recovered = resolveCurrentSession(); + + expect(recovered?.sessionId).toBe(SESSION_ID); + // A restart must not launder a broken store into a clean slate. + expect(getRecallFailure()?.message).toContain("PGRST202"); + }); + + it("does not suppress real surfaced scars when a stale failure is present", async () => { + // A failure followed by a successful recall that surfaced scars: the scars + // are what matter, and the normal confirmation path must still run. + setRecallFailure("earlier transient failure"); + clearRecallFailure(); + addSurfacedScars([ + { scar_id: "abc12345", title: "some scar", severity: "high", source: "recall" }, + ] as never); + + const result = await confirmScars({ confirmations: [] }); + + // Missing confirmations for a surfaced scar is its own rejection — the + // point here is that it is NOT the retrieval-failure path. + expect(result.formatted_response).not.toContain("institutional memory was not reached"); + }); +}); diff --git a/tests/unit/tools/provenance-citation.test.ts b/tests/unit/tools/provenance-citation.test.ts index c9f67ba..184551b 100644 --- a/tests/unit/tools/provenance-citation.test.ts +++ b/tests/unit/tools/provenance-citation.test.ts @@ -156,6 +156,10 @@ vi.mock("../../../src/services/session-state.js", () => ({ getCurrentSession: vi.fn(() => null), addSurfacedScars: vi.fn(), setRecallCalled: vi.fn(), + // GIT-93: recall records/clears a retrieval-failure marker so confirm_scars + // can tell "found nothing" from "never reached the store". + setRecallFailure: vi.fn(), + clearRecallFailure: vi.fn(), })); vi.mock("../../../src/services/agent-detection.js", () => ({