From dc6021d92c702644ca42ee5ffa2d4e23189501c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:38:49 -0400 Subject: [PATCH 1/9] test: repeatable token-audit harness + 1.7.0 baseline (GIT-74 item 1 / GIT-50) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measures what gitmem injects into agent context, per surface, against the real corpus. Built as a re-runnable script rather than hand-collected numbers because the acceptance criterion is a before/after table — the "after" column has to be produced the same way as the "before". Method, stated because it qualifies every number: - o200k_base (GPT-4o BPE) via gpt-tokenizer, installed OUTSIDE the repo so no dependency lands in package.json days before a version bump. Not Claude's tokenizer; absolute counts carry modest error, deltas are sound. - Display strings only — structured fields that never render are not injection cost. - Read-only against live data. Write-path shapes are measured under the outage env, where nothing durable is produced. - Credentials read from the MCP config programmatically, never printed. Two false starts are baked in as guards, because both produced numbers that looked like data: 1. The first run reported recall at 133 tokens, identical for match_count 8 and 3. That was not a measurement — it was an RPC error string. The audit had called recall before background cache init finished and hit a cold-start fallback. Now gated on cache-status, and it says loudly when the cache never warms rather than publishing the fallback path. 2. An isolated GITMEM_DIR discards the warm scar cache; a cold load returns learnings with no embeddings, degrading recall to weak keyword matching. The live section now uses the real gitmem dir. Read-only tools only — no session is registered, no record written. DUMP=1 prints every measured string. A measurement you cannot read is a number you cannot defend. Recall is measured at two deliberately different yields. Cost is driven by how many scars clear the similarity threshold, not by match_count — with a narrow query only one scar clears and match_count never binds, so a single query would report one point and imply it was the cost. Refs GIT-74, GIT-50, GIT-49 Co-Authored-By: Claude Opus 5 --- scripts/token-audit.mts | 281 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 scripts/token-audit.mts diff --git a/scripts/token-audit.mts b/scripts/token-audit.mts new file mode 100644 index 0000000..44ebb0a --- /dev/null +++ b/scripts/token-audit.mts @@ -0,0 +1,281 @@ +/** + * Token-efficiency audit harness (GIT-74 item 1 / GIT-50 measurement spec) + * + * Measures the token cost of what gitmem injects into agent context, per + * surface, against the REAL corpus. Run before compaction for the baseline and + * again after for the delta — the acceptance criterion is a before/after table + * that is measured, not estimated, so this must be repeatable rather than + * hand-transcribed. + * + * Method notes, stated because they qualify every number below: + * + * - Tokenizer is o200k_base (GPT-4o BPE) via gpt-tokenizer, installed OUTSIDE + * the repo so no dependency is added to package.json days before a version + * bump. It is not Claude's tokenizer, so absolute counts carry a modest + * error; ratios and deltas — which is what the audit decides on — are sound. + * Set TOKENIZER_PATH to override. + * - Surfaces are measured on the DISPLAY string, because that is what reaches + * the model. Structured fields that never render are not injection cost. + * - Reads only. No tool that creates a thread, learning, decision or session is + * called against the live store. Write-path response shapes are measured from + * the outage-env harness instead, where nothing durable is produced. + * - Credentials are read from the MCP config programmatically and passed to the + * child process. They are never printed. (Four exposure sites are already on + * the record; this adds none.) + * + * Usage: npx tsx scripts/token-audit.mts [--json] + */ + +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import { readFileSync, mkdtempSync } from "fs"; +import { tmpdir, homedir } from "os"; +import { join, dirname } from "path"; +import { fileURLToPath } from "url"; +import { createRequire } from "module"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO = join(__dirname, ".."); +const SERVER = join(REPO, "dist", "index.js"); + +const TOKENIZER_PATH = + process.env.TOKENIZER_PATH ?? + "/private/tmp/claude-501/-Users-chriscrawford-nTEG-Labs-gitmem/484fb581-da80-40bd-a803-ed0f1afba43f/scratchpad/tok/node_modules/gpt-tokenizer/cjs/encoding/o200k_base.js"; + +const require = createRequire(import.meta.url); +const { encode } = require(TOKENIZER_PATH) as { encode: (s: string) => number[] }; + +function tokens(s: string): number { + return encode(s).length; +} + +// --- Credentials, read but never printed ----------------------------------- + +/** + * Pull the gitmem MCP server's env from the Claude config. Returns only what + * the server needs; values are passed straight to the child and never logged. + */ +function liveEnv(): Record { + const cfg = JSON.parse(readFileSync(join(homedir(), ".claude.json"), "utf-8")); + let found: Record | null = null; + + const walk = (node: unknown): void => { + if (!node || typeof node !== "object") return; + const obj = node as Record; + const servers = obj.mcpServers as Record }> | undefined; + if (servers?.gitmem?.env && !found) found = servers.gitmem.env; + for (const key of Object.keys(obj)) walk(obj[key]); + }; + walk(cfg); + + if (!found) throw new Error("gitmem MCP env not found in ~/.claude.json"); + + const env = found as Record; + const required = ["SUPABASE_URL", "SUPABASE_SERVICE_ROLE_KEY", "GITMEM_TABLE_PREFIX"]; + for (const k of required) { + if (!env[k]) throw new Error(`missing ${k} in gitmem MCP env — cannot measure against the real corpus`); + } + return env; +} + +// --- MCP plumbing ---------------------------------------------------------- + +async function connect(env: Record) { + const transport = new StdioClientTransport({ + command: "node", + args: [SERVER], + env: { ...process.env, ...env, NO_COLOR: "1", NODE_ENV: "test" } as Record, + }); + const client = new Client({ name: "gitmem-token-audit", version: "1.0.0" }, { capabilities: {} }); + await client.connect(transport); + return { client, close: async () => { try { await client.close(); } catch {} } }; +} + +function displayOf(result: unknown): string { + const content = (result as { content?: { type: string; text?: string }[] }).content ?? []; + return content.map((c) => c.text ?? "").join("\n"); +} + +// --- Measurement ----------------------------------------------------------- + +interface Row { + surface: string; + mode: "live-read" | "outage"; + chars: number; + tokens: number; + note: string; +} + +const rows: Row[] = []; + +/** + * DUMP=1 prints each measured string verbatim. + * + * Not a debug convenience — a measurement you cannot read is a number you + * cannot defend. The first run of this script reported identical token counts + * for match_count 8 and 3, and only dumping the text showed why. + */ +const DUMP = process.env.DUMP === "1"; + +function record(surface: string, mode: Row["mode"], text: string, note: string): void { + rows.push({ surface, mode, chars: text.length, tokens: tokens(text), note }); + if (DUMP) { + console.log(`\n===== ${surface} [${mode}] ${tokens(text)}t / ${text.length}c =====`); + console.log(text); + console.log(`===== end ${surface} =====`); + } +} + +async function main(): Promise { + const env = liveEnv(); + + // ---- Live, read-only surfaces against the real corpus ---- + // + // Deliberately uses the REAL gitmem dir rather than a temp one. An isolated + // dir discards the warm scar cache, and a cold load returns learnings with no + // embeddings — which degrades recall from semantic matching to weak keyword + // matching and produces a measurement of the degraded path. Only read-only + // tools are called here; nothing registers a session or writes a record. + const live = await connect(env); + + const call = async (name: string, args: Record) => + displayOf(await live.client.callTool({ name, arguments: args })); + + try { + // The scar cache loads in the background after the server reports ready. + // Measuring before it finishes measures the cold-start fallback path, not + // the rendering — the first run of this script did exactly that and + // produced a "133 token recall" that was actually an RPC error string. + // Gate on the cache reporting initialized, and say so if it never does. + const warmed = await (async () => { + for (let i = 0; i < 30; i++) { + const status = await call("gitmem-cache-status", { project: "gitmem" }); + if (/initialized["\s:]*true|scar_count["\s:]*[1-9]/i.test(status)) return true; + await new Promise((r) => setTimeout(r, 500)); + } + return false; + })(); + + if (!warmed) { + console.error( + "\n⚠️ Scar cache never reported initialized. Recall numbers below are the " + + "cold-start fallback path, NOT the rendering under measurement. Do not " + + "publish them as a baseline.\n", + ); + } + + // Recall cost is driven by how many scars clear the similarity threshold, + // not by match_count. Two queries of deliberately different yield bracket + // the real range — a single query would report one point and imply it was + // the cost. + const HIGH_YIELD_PLAN = + "Implement GIT-67 + GIT-63 jointly in gitmem-mcp: render write-tool responses from the " + + "stored row rather than from the submitted payload, across create_thread/create_learning/" + + "create_decision; fail loud with no_active_session instead of fabricating a success " + + "response and a synthetic ID when there is no active session; scope the dedup candidate " + + "set to the caller's session lineage so a cross-session semantic match hard-refuses."; + + const LOW_YIELD_PLAN = + "implement the thread scope resolver and wire the four thread surfaces to it"; + + record( + "recall — high yield (8 scars)", + "live-read", + await call("recall", { plan: HIGH_YIELD_PLAN, project: "gitmem", match_count: 8 }), + "PRIMARY INJECTION SURFACE — mixed full-body and stub rendering", + ); + + record( + "recall — low yield (1 stub)", + "live-read", + await call("recall", { plan: LOW_YIELD_PLAN, project: "gitmem", match_count: 8 }), + "same match_count; only 1 scar clears threshold, so match_count does not bind", + ); + + record( + "search (topic query)", + "live-read", + await call("search", { query: "fail-open write success without a stored row", project: "gitmem" }), + "", + ); + + record("list_threads", "live-read", await call("list_threads", { project: "gitmem" }), ""); + + record("log (recent learnings)", "live-read", await call("log", {}), ""); + + record("cache-health", "live-read", await call("gitmem-cache-health", { project: "gitmem" }), "post-GIT-69 three-valued status"); + + record("health (write-path)", "live-read", await call("health", {}), ""); + } finally { + await live.close(); + } + + // ---- Write-path response shapes, measured where nothing durable lands ---- + const outageHome = mkdtempSync(join(tmpdir(), "gitmem-audit-outage-")); + const outage = await connect({ + GITMEM_TIER: "pro", + SUPABASE_URL: "http://127.0.0.1:1", + SUPABASE_SERVICE_ROLE_KEY: "audit-not-a-real-key", + GITMEM_TABLE_PREFIX: "orchestra_", + GITMEM_DIR: outageHome, + HOME: outageHome, + }); + + try { + const oCall = async (name: string, args: Record) => + displayOf(await outage.client.callTool({ name, arguments: args })); + + record( + "create_thread — refusal (no session)", + "outage", + await oCall("create_thread", { text: "Audit probe alpha — cinnabar wren lattice" }), + "GIT-67 no_active_session refusal", + ); + + await oCall("session_start", { agent_identity: "cli", project: "gitmem" }); + + record( + "session_start panel (empty corpus)", + "outage", + await oCall("session_refresh", {}), + "LOWER BOUND only — no threads/decisions to render; live panel is larger", + ); + + const created = await oCall("create_thread", { text: "Audit probe bravo — quartz heron dulcimer" }); + record("create_thread — local_only (R4 fail-honest)", "outage", created, ""); + + record( + "create_thread — dedup refusal (R2)", + "outage", + await oCall("create_thread", { text: "Audit probe bravo — quartz heron dulcimer" }), + "quotes the stored row + both lengths", + ); + } finally { + await outage.close(); + } + + // ---- Report ---- + if (process.argv.includes("--json")) { + console.log(JSON.stringify({ tokenizer: "o200k_base", measured_at_utc: null, rows }, null, 2)); + return; + } + + const w = Math.max(...rows.map((r) => r.surface.length), 8); + console.log(`\nTokenizer: o200k_base (GPT-4o BPE). Display strings only.\n`); + console.log(`${"surface".padEnd(w)} ${"mode".padEnd(10)} ${"tokens".padStart(7)} ${"chars".padStart(7)} note`); + console.log("-".repeat(w + 40)); + for (const r of rows) { + console.log( + `${r.surface.padEnd(w)} ${r.mode.padEnd(10)} ${String(r.tokens).padStart(7)} ${String(r.chars).padStart(7)} ${r.note}`, + ); + } + const liveTotal = rows.filter((r) => r.mode === "live-read").reduce((a, r) => a + r.tokens, 0); + console.log("-".repeat(w + 40)); + console.log(`${"live-read subtotal".padEnd(w)} ${"".padEnd(10)} ${String(liveTotal).padStart(7)}`); + console.log(""); +} + +main().catch((err) => { + console.error("audit failed:", err instanceof Error ? err.message : err); + process.exit(1); +}); From bff6cc8940c780ce70dca550e5b8932f3bd48788 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 18:05:23 -0400 Subject: [PATCH 2/9] =?UTF-8?q?feat:=20tiered=20scar=20rendering=20+=20app?= =?UTF-8?q?lies=5Fwhen=20fix=20=E2=80=94=209=20tests=20RED=20pending=20R13?= =?UTF-8?q?a?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⚠️ THIS COMMIT IS HONESTLY RED. 9 tests fail in tests/unit/tools/provenance-citation.test.ts. They are named below and are the first task of the next session per R13a. Committed rather than left in a working tree overnight — the sprint preserved a stranger's 519 uncommitted lines on that principle. TIER RENDERING (GIT-74 / R11, R12b) Banded by outcome rate rather than a flat budget: stub <0.55 id + title + score compact 0.55–0.75 + why_this_matters + applies_when extended >=0.75 or top + first counter-argument full explicit fetch by id R12b: renders honest fields or nothing. Description-first-sentence is RETIRED — against the real corpus it produced provenance metadata ("Liberation Blueprint nugget, session 096e940d…") or the title echoed back by variant-enforcement text about as often as a lesson. A fragment that looks like a rendered judgment is worse than no line; it teaches the reader to skim the field. When why_this_matters and applies_when are both absent, the compact tier is the header alone. The per-scar "Full detail" affordance became one footer line — it was spending ~12 tokens × N to say one thing. MEASURED (o200k_base, net of a measured 35-token no-session banner that does not fire in normal use): high-yield recall 2565 → 487 (81%) low-yield recall 246 → 115 (53%) Both inside R12c's amended acceptance (≤750 / ≤115). Chrome cuts stopped at the citation line, the acknowledge line that drives confirm_scars, and one footer — trimming those buys a round number with honesty. APPLIES_WHEN (GIT-76) Assignment moved out of the `win` branch. It sat there since the monorepo extraction (d0530f7, 2026-02-03), so every scar and pattern this package ever wrote validated the field, reported success, and discarded it. Found because the compact tier had nothing to render — the audit failing honestly. NOT fixed here, absorbed into GIT-76 per R13b: problem_context and solution_approach have the identical branch-scoped defect at the identical site, and anti_pattern has no branch at all (so no severity default). They land together behind one parameterized test asserting every schema-accepted field persists for every learning_type — the guard that makes a fifth sibling structurally impossible. CITATION UNIFICATION (in scope, tests pending) One CITATION_LINE constant in display-protocol.ts, imported by recall, search and prepare_context. The consistency test that caught this was passing on a shared prefix over already-divergent wording. RED, pending R13a rewrite to invariants (not prose): - all paths use the same citation rule text - citation protocol uses consistent separator (separator removed by design) - citation protocol appears before results - display includes CITATION RULE ×3 (recall remote, recall free, prepare_context) - includes example with record ID format ([id:48ebca14] example removed) - renders the full body for >=0.55 scars ← encodes the pre-R11 contract - search citation text assertions Pre-authorized fallback if coupling exceeds these nine: revert recall's citation wording, ship low-yield ~133, unification becomes a follow-up. The 487 high-yield figure is unaffected either way. Refs GIT-74, GIT-76, GIT-50, GIT-49 Co-Authored-By: Claude Opus 5 --- scripts/token-audit.mts | 14 +++ src/services/display-protocol.ts | 28 ++++- src/tools/create-learning.ts | 17 ++- src/tools/prepare-context.ts | 7 +- src/tools/recall.ts | 111 +++++++++++++++---- src/tools/search.ts | 6 +- tests/unit/tools/provenance-citation.test.ts | 21 ++-- 7 files changed, 160 insertions(+), 44 deletions(-) diff --git a/scripts/token-audit.mts b/scripts/token-audit.mts index 44ebb0a..8cc0584 100644 --- a/scripts/token-audit.mts +++ b/scripts/token-audit.mts @@ -206,6 +206,20 @@ async function main(): Promise { record("cache-health", "live-read", await call("gitmem-cache-health", { project: "gitmem" }), "post-GIT-69 three-valued status"); record("health (write-path)", "live-read", await call("health", {}), ""); + + // The audit deliberately starts no session, so every live response above + // carries the no-active-session enforcement banner. It does NOT fire in + // normal use. Measured exactly rather than estimated away, so the recall + // figures can be reported both raw and net. + record( + "— enforcement banner (artifact, subtract)", + "live-read", + "--- gitmem enforcement ---\n" + + "No active session. Call session_start() first to initialize memory context.\n" + + "Without a session, scars won't be tracked and the closing ceremony can't run.\n" + + "---\n", + "present in every live row above; absent when a session is active", + ); } finally { await live.close(); } diff --git a/src/services/display-protocol.ts b/src/services/display-protocol.ts index 6b398d2..ec712e8 100644 --- a/src/services/display-protocol.ts +++ b/src/services/display-protocol.ts @@ -43,10 +43,32 @@ const { red, yellow, green, bold, dim, reset } = ANSI; // Display protocol suffix // --------------------------------------------------------------------------- +/** + * GIT-74/R11: collapsed from a 51-char rule plus three lines to a single line. + * + * This fires on EVERY tool response, so it was the largest cross-surface chrome + * item in the token audit — and the box-drawing rule alone tokenizes at roughly + * the cost of the sentence beneath it. The instruction is unchanged in force: + * echo verbatim, because the result is otherwise invisible to the user. Only + * the words spent saying it are fewer. + */ const DISPLAY_SUFFIX = - "\n───────────────────────────────────────────────────\n" + - "DISPLAY PROTOCOL: Output the content above verbatim as your response text.\n" + - "Tool results are collapsed in the CLI — the user cannot see them unless you echo them."; + "\nDISPLAY PROTOCOL: echo the above verbatim — tool results are collapsed and the user cannot see them otherwise."; + +/** + * The one citation rule (GIT-74/R12). + * + * It lived as three separate string literals in recall, search and + * prepare_context, which had already drifted — different wording, and only two + * of the three carried the worked example. A test asserted they were + * consistent and passed on a shared prefix. + * + * One definition, every surface imports it: the same discipline the thread + * scope resolver applied to thread visibility. Compacted per the token audit — + * the rule is unchanged in force, only shorter. + */ +export const CITATION_LINE = + 'Cite record IDs for claims; otherwise say "not in institutional memory."'; /** * Wrap formatted content with the display protocol suffix. diff --git a/src/tools/create-learning.ts b/src/tools/create-learning.ts index 1c8b85a..e97103b 100644 --- a/src/tools/create-learning.ts +++ b/src/tools/create-learning.ts @@ -120,6 +120,22 @@ export async function createLearning( ...(params.self_check_criteria && { self_check_criteria: params.self_check_criteria }), }; + // GIT-76: applies_when is universal, not win-specific. + // + // It sat inside the `win` branch below since the monorepo extraction + // (d0530f7, 2026-02-03), so every scar and pattern written by this package + // validated the field, reported success, and discarded it. The corpus shows + // the split cleanly: records from Dec 2025 carry values, everything written + // here since is []. + // + // It is also the single most decision-relevant field for an APPLYING/N_A + // call in confirm_scars, so its absence degraded scar utility and not merely + // fidelity — which is how the GIT-74 token audit found it: the compact tier + // had nothing to render. + if (params.applies_when !== undefined) { + learningData.applies_when = params.applies_when; + } + // Add type-specific fields if (params.learning_type === "scar") { learningData.severity = params.severity; @@ -130,7 +146,6 @@ export async function createLearning( if (params.learning_type === "win") { learningData.problem_context = params.problem_context || ""; learningData.solution_approach = params.solution_approach || ""; - learningData.applies_when = params.applies_when || []; learningData.severity = params.severity || "medium"; } diff --git a/src/tools/prepare-context.ts b/src/tools/prepare-context.ts index 31c0b10..3424698 100644 --- a/src/tools/prepare-context.ts +++ b/src/tools/prepare-context.ts @@ -28,7 +28,7 @@ import { buildComponentPerformance, } from "../services/metrics.js"; import { v4 as uuidv4 } from "uuid"; -import { wrapDisplay, productLine } from "../services/display-protocol.js"; +import { wrapDisplay, productLine, CITATION_LINE } from "../services/display-protocol.js"; import { formatNudgeHeader } from "../services/nudge-variants.js"; import type { Project, PerformanceBreakdown, PerformanceData } from "../types/index.js"; import { @@ -86,10 +86,7 @@ Proceed with caution — this may be new territory without documented lessons.`; // Citation protocol — provenance enforcement for any downstream claims // Placed BEFORE results so agents see it before processing scars - lines.push("───────────────────────────────────────────────────"); - lines.push("CITATION RULE: When referencing facts from these scars, cite the record ID."); - lines.push("Example: \"Edge improved to 3.07 [id:48ebca14]\" — not paraphrased numbers."); - lines.push("If you cannot cite a specific record for a claim, say \"not in institutional memory.\""); + lines.push(CITATION_LINE); lines.push(""); // Blocking verification requirements first diff --git a/src/tools/recall.ts b/src/tools/recall.ts index 1270ee9..bbdae21 100644 --- a/src/tools/recall.ts +++ b/src/tools/recall.ts @@ -38,7 +38,7 @@ 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 } from "../services/display-protocol.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"; import type { Project, RelevantScar, PerformanceData, PerformanceBreakdown, SurfacedScar } from "../types/index.js"; @@ -54,6 +54,28 @@ import type { Project, RelevantScar, PerformanceData, PerformanceBreakdown, Surf */ export const LOW_CONFIDENCE_THRESHOLD = 0.55; +/** + * Upper band boundary for tiered rendering (GIT-74/R11). + * + * At or above this similarity — or for the single top hit, whichever the query + * produced — a scar earns the extended render. Overall confirm-rate runs ~82% + * applied, and the high-similarity end is where that concentrates, so the + * likeliest-to-bind scar gets the deepest body. + */ +export const EXTENDED_THRESHOLD = 0.75; + +/** Per-field caps for compact/extended rendering. Chars, not tokens — the cap + * is enforced where the text is, and the token budget is verified by the audit + * script rather than guessed at per field. */ +const COMPACT_LESSON_CHARS = 180; +const APPLIES_WHEN_CHARS = 60; +const COUNTER_ARG_CHARS = 160; + +function truncateTo(text: string, cap: number): string { + const cleaned = text.replace(/\s+/g, " ").trim(); + return cleaned.length <= cap ? cleaned : cleaned.slice(0, cap - 1).trimEnd() + "…"; +} + /** * Parameters for recall tool */ @@ -158,6 +180,12 @@ No past lessons match this plan closely enough. Scars accumulate as you work — // Check if any scars have required_verification (blocking gates) const scarsWithVerification = scars.filter((s) => s.required_verification?.blocking); + // GIT-74/R11: the top hit earns the deepest render even if it does not clear + // EXTENDED_THRESHOLD — it is the likeliest to bind. + const topHitId = scars.length > 0 + ? scars.reduce((best, s) => (s.similarity > best.similarity ? s : best), scars[0]).id + : null; + const lines: string[] = [ formatNudgeHeader(scars.length), "", @@ -168,12 +196,11 @@ No past lessons match this plan closely enough. Scars accumulate as you work — lines.push(""); } - // Citation protocol — provenance enforcement for any downstream claims - // Placed BEFORE results so agents see it before processing scars - lines.push("───────────────────────────────────────────────────"); - lines.push("CITATION RULE: When referencing facts from these scars, cite the record ID."); - lines.push("Example: \"Edge improved to 3.07 [id:48ebca14]\" — not paraphrased numbers."); - lines.push("If you cannot cite a specific record for a claim, say \"not in institutional memory.\""); + // Citation protocol — provenance enforcement for any downstream claims. + // GIT-74/R11: collapsed from a 4-line block to one line. It fired ~200 tokens + // of chrome on every recall including single-stub responses, and nothing + // honest lives in it — the rule is the same rule in one sentence. + lines.push(dimText(CITATION_LINE)); lines.push(""); // Display blocking verification requirements FIRST and prominently @@ -221,13 +248,55 @@ No past lessons match this plan closely enough. Scars accumulate as you work — } } - // Stub low-confidence scars: render header only, skip the heavy body. - // ~66% of sub-threshold matches are N/A, so their full bodies are wasted - // tokens. Blocking-verification scars always render full regardless of score. - const isStub = - scar.similarity < LOW_CONFIDENCE_THRESHOLD && !scar.required_verification?.blocking; + // GIT-74/R11: tiered rendering, banded by outcome rate rather than a flat + // budget. The corpus says where tokens earn their keep — the sub-0.55 band + // runs ~66% N/A (GIT-49's basis) while overall confirm-rate runs ~82% + // applied, so render depth follows expected utility. + // + // stub < 0.55 id + title + score ~15 tok + // compact 0.55 – 0.75 + one-line lesson + applies_when ≤60 tok + // extended ≥ 0.75 or top hit + first counter-argument ≤150 tok + // full explicit fetch by id only + // + // The compact tier is the default and must stay sufficient for an honest + // APPLYING/N_A call — confirm_scars consumes this rendering, and degrading + // its decisions to save tokens would spend the product to buy efficiency. + // Blocking-verification scars render full regardless of score. + const forceFull = Boolean(scar.required_verification?.blocking); + const tier: "stub" | "compact" | "extended" | "full" = forceFull + ? "full" + : scar.similarity < LOW_CONFIDENCE_THRESHOLD + ? "stub" + : scar.similarity >= EXTENDED_THRESHOLD || scar.id === topHitId + ? "extended" + : "compact"; + + if (tier === "compact" || tier === "extended") { + // GIT-74/R12b: render honest fields, or render nothing. Never fragments. + // + // This previously used the description's first sentence as the one-line + // lesson. Against the real corpus that was provenance metadata + // ("Liberation Blueprint nugget, session 096e940d…"), or the title echoed + // back by variant-enforcement text, about as often as it was a lesson. A + // fragment that *looks* like a rendered judgment is worse than no line — + // it teaches the reader to skim the field. + // + // So: why_this_matters when present, applies_when when present, and when + // neither exists the compact tier is the header line alone. + if (scar.why_this_matters) { + lines.push(` ${truncateTo(scar.why_this_matters, COMPACT_LESSON_CHARS)}`); + } + + if (scar.applies_when.length > 0) { + lines.push(` ${dimText("Applies when:")} ${scar.applies_when.slice(0, 2).map((a) => truncateTo(a, APPLIES_WHEN_CHARS)).join("; ")}`); + } + + if (tier === "extended" && scar.counter_arguments.length > 0) { + lines.push(` ${dimText("You might think:")} ${truncateTo(scar.counter_arguments[0], COUNTER_ARG_CHARS)}`); + } + } - if (!isStub) { + if (tier === "full") { // Use variant enforcement text if available (blind to variant name) if (scar.variant_info?.has_variants && scar.variant_info.variant) { const variantText = formatVariantEnforcement(scar.variant_info.variant, scar.title); @@ -293,14 +362,14 @@ No past lessons match this plan closely enough. Scars accumulate as you work — lines.push("**Acknowledge these lessons before proceeding.**"); - // Pro: graph nudge when triples exist on any scar - if (hasProInsights() && scars.some(s => s.related_triples && s.related_triples.length > 0)) { - const firstScarWithTriples = scars.find(s => s.related_triples && s.related_triples.length > 0); - if (firstScarWithTriples) { - lines.push(""); - lines.push(dimText(`Pro: Use graph_traverse(lens: 'connected_to', node: '${firstScarWithTriples.title}') to explore deeper connections.`)); - } - } + // Two-phase lazy recall (GIT-50): the affordance is stated once, not once per + // scar. Repeating it on every entry cost ~12 tokens × N to say one thing. + lines.push(dimText("Full body: search(\"\")")); + + // GIT-74/R11: the graph_traverse nudge is removed from default rendering. + // It fired on every recall carrying triples and cost tokens on a discovery + // hint the caller rarely took. graph_traverse remains available as a tool; + // it just no longer advertises itself inside the injection path. return lines.join("\n"); } diff --git a/src/tools/search.ts b/src/tools/search.ts index 9b9b501..a74131d 100644 --- a/src/tools/search.ts +++ b/src/tools/search.ts @@ -23,7 +23,7 @@ import { buildComponentPerformance, } from "../services/metrics.js"; import { v4 as uuidv4 } from "uuid"; -import { wrapDisplay, truncate, SEV, TYPE, productLine, dimText } from "../services/display-protocol.js"; +import { wrapDisplay, truncate, SEV, TYPE, productLine, dimText, CITATION_LINE } from "../services/display-protocol.js"; import type { Project, PerformanceBreakdown, PerformanceData } from "../types/index.js"; // --- Types --- @@ -84,9 +84,7 @@ function buildSearchDisplay( // Citation protocol — provenance enforcement for any downstream claims // Placed BEFORE results so agents see it before processing results if (results.length > 0) { - lines.push("───────────────────────────────────────────────────"); - lines.push("CITATION RULE: When referencing facts from these results, cite the record ID."); - lines.push("If you cannot cite a specific record for a claim, say \"not in institutional memory.\""); + lines.push(dimText(CITATION_LINE)); lines.push(""); } diff --git a/tests/unit/tools/provenance-citation.test.ts b/tests/unit/tools/provenance-citation.test.ts index 745fed4..cf17155 100644 --- a/tests/unit/tools/provenance-citation.test.ts +++ b/tests/unit/tools/provenance-citation.test.ts @@ -13,6 +13,7 @@ */ import { describe, it, expect, vi, beforeEach } from "vitest"; +import { CITATION_LINE } from "../../../src/services/display-protocol.js"; // ============================================================ // Part 1: format-utils (compact format) — no mocking needed @@ -221,7 +222,7 @@ describe("search: citation protocol", () => { const result = await search({ query: "EPS strategy" }); - expect(result.display).toContain("CITATION RULE"); + expect(result.display).toContain(CITATION_LINE); expect(result.display).toContain("cite the record ID"); expect(result.display).toContain("not in institutional memory"); }); @@ -231,7 +232,7 @@ describe("search: citation protocol", () => { const result = await search({ query: "test query" }); - expect(result.display).toContain("CITATION RULE"); + expect(result.display).toContain(CITATION_LINE); expect(result.display).toContain("cite the record ID"); }); @@ -240,7 +241,7 @@ describe("search: citation protocol", () => { const result = await search({ query: "nonexistent topic" }); - expect(result.display).not.toContain("CITATION RULE"); + expect(result.display).not.toContain(CITATION_LINE); }); }); @@ -328,7 +329,7 @@ describe("prepare_context full: citation protocol", () => { format: "full", }); - expect(result.memory_payload).toContain("CITATION RULE"); + expect(result.memory_payload).toContain(CITATION_LINE); expect(result.memory_payload).toContain("cite the record ID"); expect(result.memory_payload).toContain("not in institutional memory"); }); @@ -354,7 +355,7 @@ describe("prepare_context full: citation protocol", () => { format: "full", }); - expect(result.memory_payload).not.toContain("CITATION RULE"); + expect(result.memory_payload).not.toContain(CITATION_LINE); expect(result.memory_payload).toContain("no relevant scars"); }); @@ -368,7 +369,7 @@ describe("prepare_context full: citation protocol", () => { const payload = result.memory_payload; const ackIdx = payload.indexOf("Acknowledge these lessons"); - const citationIdx = payload.indexOf("CITATION RULE"); + const citationIdx = payload.indexOf(CITATION_LINE); expect(ackIdx).toBeGreaterThan(-1); expect(citationIdx).toBeGreaterThan(-1); @@ -470,7 +471,7 @@ describe("recall: citation protocol", () => { const result = await recall({ plan: "deploy edge function" }); - expect(result.display).toContain("CITATION RULE"); + expect(result.display).toContain(CITATION_LINE); expect(result.display).toContain("cite the record ID"); expect(result.display).toContain("not in institutional memory"); }); @@ -489,7 +490,7 @@ describe("recall: citation protocol", () => { const result = await recall({ plan: "free tier recall test" }); - expect(result.display).toContain("CITATION RULE"); + expect(result.display).toContain(CITATION_LINE); }); it("display omits CITATION RULE when no scars found", async () => { @@ -497,7 +498,7 @@ describe("recall: citation protocol", () => { const result = await recall({ plan: "no matching scars here" }); - expect(result.display).not.toContain("CITATION RULE"); + expect(result.display).not.toContain(CITATION_LINE); }); it("citation protocol appears before results (before 'Acknowledge these lessons')", async () => { @@ -507,7 +508,7 @@ describe("recall: citation protocol", () => { const display = result.display!; const ackIdx = display.indexOf("Acknowledge these lessons"); - const citationIdx = display.indexOf("CITATION RULE"); + const citationIdx = display.indexOf(CITATION_LINE); expect(ackIdx).toBeGreaterThan(-1); expect(citationIdx).toBeGreaterThan(-1); From 0f42036699c859954a39021e00b02165dd853a88 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 21:12:38 -0400 Subject: [PATCH 3/9] test: rewrite 9 provenance-citation tests to invariants (GIT-74/R13a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Branch bff6cc8 was honestly red: 9 tests asserted the prose of the pre-audit citation chrome, which R12/R12b compacted and retired. The tests were pinning wording, not guarantees. Per R13a they now assert what has to hold: (a) all three retrieval surfaces emit the identical CITATION_LINE constant, exactly once, with no drifted second copy. The old "consistency" test compared a shared PREFIX and passed while the three literals had already diverged — that hole is closed. (b) the id-citation format is present: each surfaced row carries the 8-char id: form an agent is being told to cite, replacing the assertion on the retired worked example "[id:48ebca14]". (c) R11 tiering replaces the obsolete "full body at >=0.55". The four bands are asserted off LOW_CONFIDENCE_THRESHOLD/EXTENDED_THRESHOLD rather than restated as literals. The retired separator test is replaced rather than deleted: the box-drawing rule is gone from the retrieval surfaces (it survives only in server.ts/session-start.ts), so it is no longer a provenance guarantee, while single-constant identity is. Coverage: 31 -> 34 tests. Net new coverage is the compact/extended band split and the top-hit escalation rule — the top hit renders extended regardless of score, so a single-scar fixture can never exercise the compact band. Every previous tiering test used a single scar and was therefore testing extended while claiming to test the 0.55 boundary. Invariants shown to bind, not merely pass: - reword CITATION_LINE entirely -> 34/34 still green (prose-independent) - EXTENDED_THRESHOLD 0.75 -> 0.55 -> compact-band test fails - drop id: from recall's header -> id-citation test fails Source restored byte-identical after each mutation; git diff touches only the test file. Full suite: 65 files, 1096 tests, 0 failures. tsc --noEmit clean. Coupling did not extend beyond the 9, so the R13a fallback (revert recall's citation wording, ship low-yield ~133) was not needed. Co-Authored-By: Claude Opus 5 --- tests/unit/tools/provenance-citation.test.ts | 196 ++++++++++++++----- 1 file changed, 142 insertions(+), 54 deletions(-) diff --git a/tests/unit/tools/provenance-citation.test.ts b/tests/unit/tools/provenance-citation.test.ts index cf17155..579c91b 100644 --- a/tests/unit/tools/provenance-citation.test.ts +++ b/tests/unit/tools/provenance-citation.test.ts @@ -217,23 +217,36 @@ beforeEach(() => { }); describe("search: citation protocol", () => { - it("display includes CITATION RULE when results found (remote)", async () => { + it("emits the shared citation rule when results found (remote)", async () => { setupSearchRemote([makeSearchScar()]); const result = await search({ query: "EPS strategy" }); + // Invariant, not prose: whatever CITATION_LINE says, search emits it. expect(result.display).toContain(CITATION_LINE); - expect(result.display).toContain("cite the record ID"); - expect(result.display).toContain("not in institutional memory"); }); - it("display includes CITATION RULE when results found (free tier)", async () => { + it("emits the shared citation rule when results found (free tier)", async () => { setupSearchFreeTier([makeSearchScar()]); const result = await search({ query: "test query" }); expect(result.display).toContain(CITATION_LINE); - expect(result.display).toContain("cite the record ID"); + }); + + it("renders a citable record id for every result", async () => { + // The citation rule is only honourable if the ids it asks for are on the + // page. Assert the rendered id-form, not the worked example that used to + // sit inside the rule text. + setupSearchRemote([ + makeSearchScar({ id: "aaaaaaaa-1111-2222-3333-444444444444", title: "First" }), + makeSearchScar({ id: "bbbbbbbb-5555-6666-7777-888888888888", title: "Second" }), + ]); + + const result = await search({ query: "citable ids" }); + + expect(result.display).toContain("id:aaaaaaaa"); + expect(result.display).toContain("id:bbbbbbbb"); }); it("display omits CITATION RULE when no results", async () => { @@ -329,12 +342,11 @@ describe("prepare_context full: citation protocol", () => { format: "full", }); + // Invariant, not prose: the payload carries the shared constant verbatim. expect(result.memory_payload).toContain(CITATION_LINE); - expect(result.memory_payload).toContain("cite the record ID"); - expect(result.memory_payload).toContain("not in institutional memory"); }); - it("includes example with record ID format in full output", async () => { + it("emits the citation rule byte-identical to the other surfaces", async () => { setupPrepareRemote([makePrepareContextScar()]); const result = await prepareContext({ @@ -342,9 +354,13 @@ describe("prepare_context full: citation protocol", () => { format: "full", }); - // The example shows how to properly cite: [id:48ebca14] - expect(result.memory_payload).toContain("[id:48ebca14]"); - expect(result.memory_payload).toContain("not paraphrased numbers"); + // prepare_context is the sub-agent injection path, so it pushes the rule + // without ANSI dimming while recall/search dim it. The *text* must still be + // the one constant — this is the assertion that would have caught the + // original three-way drift, which the old shared-prefix check did not. + const payload = result.memory_payload; + const occurrences = payload.split(CITATION_LINE).length - 1; + expect(occurrences).toBe(1); }); it("omits CITATION RULE when no scars found (full format)", async () => { @@ -459,6 +475,7 @@ function makeRecallScar(overrides: Record = {}) { severity: overrides.severity ?? "medium", counter_arguments: overrides.counter_arguments ?? [], applies_when: overrides.applies_when ?? [], + why_this_matters: overrides.why_this_matters ?? undefined, similarity: overrides.similarity ?? 0.7, source_linear_issue: overrides.source_linear_issue ?? null, learning_type: overrides.learning_type ?? "scar", @@ -471,18 +488,23 @@ describe("recall: citation protocol", () => { const result = await recall({ plan: "deploy edge function" }); + // Invariant, not prose: whatever CITATION_LINE says, recall emits it. expect(result.display).toContain(CITATION_LINE); - expect(result.display).toContain("cite the record ID"); - expect(result.display).toContain("not in institutional memory"); }); - it("display includes example with [id:] format", async () => { - setupRecallRemote([makeRecallScar()]); + it("renders a citable record id for every scar", async () => { + // Replaces the assertion on the retired worked example ("[id:48ebca14]"). + // What actually has to hold is that each surfaced scar carries the id an + // agent is being told to cite, in the documented 8-char id: form. + setupRecallRemote([ + makeRecallScar({ id: "cccccccc-1111-2222-3333-444444444444", title: "First" }), + makeRecallScar({ id: "dddddddd-5555-6666-7777-888888888888", title: "Second" }), + ]); const result = await recall({ plan: "check metrics" }); - expect(result.display).toContain("[id:48ebca14]"); - expect(result.display).toContain("not paraphrased numbers"); + expect(result.display).toContain("id:cccccccc"); + expect(result.display).toContain("id:dddddddd"); }); it("display includes CITATION RULE (free tier)", async () => { @@ -569,26 +591,86 @@ describe("recall: confidence tiers", () => { expect(display).not.toContain("You might think"); }); - it("renders the full body for >=0.55 scars (description + counter_arguments present)", async () => { + // GIT-74/R11 replaced the flat "stub below 0.55, full body at or above it" + // split with four bands. The old assertion (full description at 0.7) now + // encodes a rule that no longer exists, so it is replaced by the band table + // itself, read off LOW_CONFIDENCE_THRESHOLD / EXTENDED_THRESHOLD rather than + // restated as literals. + // + // stub < 0.55 header only + // compact 0.55 – 0.75 + why_this_matters / applies_when + // extended >= 0.75 OR the top hit + first counter-argument + // full blocking verification + description + // + // NOTE: the top hit is escalated to extended regardless of score, so a + // single-scar fixture can never exercise the compact band. Each band test + // below therefore pins a decoy top hit above the scar under test. + it("compact band (0.55–0.75, not top hit): lesson fields render, counter-arguments do not", async () => { setupRecallRemote([ + makeRecallScar({ id: "top-hit-decoy", title: "Decoy top hit", similarity: 0.9 }), makeRecallScar({ - id: "strong-full", - title: "Strong full scar", + id: "compact-band", + title: "Compact band scar", similarity: 0.7, - description: "UNIQUE_FULL_BODY_should_render_for_high_confidence", - counter_arguments: ["UNIQUE_FULL_COUNTERARG_should_render"], + description: "UNIQUE_COMPACT_DESCRIPTION_must_not_render", + why_this_matters: "UNIQUE_COMPACT_WHY_must_render", + applies_when: ["UNIQUE_COMPACT_APPLIES_must_render"], + counter_arguments: ["UNIQUE_COMPACT_COUNTERARG_must_not_render"], }), ]); - const result = await recall({ plan: "full render test" }); - const display = result.display!; + const display = (await recall({ plan: "compact band test" })).display!; - expect(display).toContain("UNIQUE_FULL_BODY_should_render_for_high_confidence"); - expect(display).toContain("You might think"); - expect(display).toContain("UNIQUE_FULL_COUNTERARG_should_render"); + expect(display).toContain("Compact band scar"); + expect(display).toContain("UNIQUE_COMPACT_WHY_must_render"); + expect(display).toContain("UNIQUE_COMPACT_APPLIES_must_render"); + // The heavy fields belong to deeper bands only. + expect(display).not.toContain("UNIQUE_COMPACT_DESCRIPTION_must_not_render"); + expect(display).not.toContain("UNIQUE_COMPACT_COUNTERARG_must_not_render"); expect(display).not.toContain("[low confidence]"); }); + it("extended band (>=0.75): adds the first counter-argument", async () => { + setupRecallRemote([ + makeRecallScar({ id: "top-hit-decoy", title: "Decoy top hit", similarity: 0.95 }), + makeRecallScar({ + id: "extended-band", + title: "Extended band scar", + similarity: 0.8, + why_this_matters: "UNIQUE_EXTENDED_WHY_must_render", + counter_arguments: [ + "UNIQUE_EXTENDED_COUNTERARG_first_must_render", + "UNIQUE_EXTENDED_COUNTERARG_second_must_not_render", + ], + }), + ]); + + const display = (await recall({ plan: "extended band test" })).display!; + + expect(display).toContain("UNIQUE_EXTENDED_WHY_must_render"); + expect(display).toContain("You might think"); + expect(display).toContain("UNIQUE_EXTENDED_COUNTERARG_first_must_render"); + // "first counter-argument", singular — the band is capped, not unbounded. + expect(display).not.toContain("UNIQUE_EXTENDED_COUNTERARG_second_must_not_render"); + }); + + it("the top hit is escalated to extended even below the extended threshold", async () => { + // This is the rule that makes single-scar fixtures misleading, so it gets + // its own test rather than living as an implicit assumption. + setupRecallRemote([ + makeRecallScar({ + id: "sole-top-hit", + title: "Sole top hit", + similarity: 0.6, // compact band by score alone + counter_arguments: ["UNIQUE_TOPHIT_COUNTERARG_must_render"], + }), + ]); + + const display = (await recall({ plan: "top hit escalation test" })).display!; + + expect(display).toContain("UNIQUE_TOPHIT_COUNTERARG_must_render"); + }); + it("never stubs blocking-verification scars, even below threshold", async () => { setupRecallRemote([ { @@ -637,44 +719,50 @@ describe("recall: confidence tiers", () => { // Part 5: Cross-cutting provenance guarantees // ============================================================ -describe("provenance: separator consistency", () => { - it("citation protocol uses consistent separator across all full-format paths", async () => { - const SEPARATOR = "───────────────────────────────────────────────────"; - - // Search +describe("provenance: citation rule is one constant, not three copies", () => { + // The predecessors of these two tests asserted a box-drawing separator and a + // prose prefix ("CITATION RULE: When referencing facts from these"). The + // separator was retired from the retrieval surfaces by the GIT-74 token audit + // and the prose was compacted, so both tests were asserting chrome rather + // than the guarantee. Worse, the prefix check passed while the three literals + // had already drifted — a shared prefix is not shared text. + // + // The guarantee that actually matters: one exported constant, emitted whole + // by every surface. Assert that, and the tests survive the next rewording. + + it("all three retrieval surfaces emit the identical citation constant", async () => { setupSearchRemote([makeSearchScar()]); - const searchResult = await search({ query: "separator test" }); - expect(searchResult.display).toContain(SEPARATOR); + const searchDisplay = (await search({ query: "consistency" })).display!; - // Recall setupRecallRemote([makeRecallScar()]); - const recallResult = await recall({ plan: "separator test" }); - expect(recallResult.display).toContain(SEPARATOR); + const recallDisplay = (await recall({ plan: "consistency" })).display!; - // Prepare context full setupPrepareRemote([makePrepareContextScar()]); - const pcResult = await prepareContext({ plan: "separator test", format: "full" }); - expect(pcResult.memory_payload).toContain(SEPARATOR); - }); -}); + const pcPayload = (await prepareContext({ plan: "consistency", format: "full" })) + .memory_payload; -describe("provenance: citation text consistency", () => { - it("all paths use the same citation rule text", async () => { - const CITATION_TEXT = "CITATION RULE: When referencing facts from these"; + for (const surface of [searchDisplay, recallDisplay, pcPayload]) { + expect(surface).toContain(CITATION_LINE); + } + }); - // Search + it("no surface ships a second, drifted copy of the rule", async () => { + // Catches the original defect class directly: a surface that keeps its own + // literal alongside the import, or emits the rule twice. setupSearchRemote([makeSearchScar()]); - const searchResult = await search({ query: "consistency" }); - expect(searchResult.display).toContain(CITATION_TEXT); + const searchDisplay = (await search({ query: "duplication" })).display!; - // Recall setupRecallRemote([makeRecallScar()]); - const recallResult = await recall({ plan: "consistency" }); - expect(recallResult.display).toContain(CITATION_TEXT); + const recallDisplay = (await recall({ plan: "duplication" })).display!; - // Prepare context full setupPrepareRemote([makePrepareContextScar()]); - const pcResult = await prepareContext({ plan: "consistency", format: "full" }); - expect(pcResult.memory_payload).toContain(CITATION_TEXT); + const pcPayload = (await prepareContext({ plan: "duplication", format: "full" })) + .memory_payload; + + for (const surface of [searchDisplay, recallDisplay, pcPayload]) { + expect(surface.split(CITATION_LINE).length - 1).toBe(1); + // The retired prose must not reappear anywhere alongside the constant. + expect(surface).not.toContain("CITATION RULE: When referencing facts"); + } }); }); From 09083521a6db582b7356f35b954d3e916d67b4b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 21:22:10 -0400 Subject: [PATCH 4/9] fix: ids travel with the citation instruction on all four surfaces (GIT-74/R14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prepare_context (full) and formatCompact each emitted a citation instruction while rendering no record ids at all — they commanded the impossible, and the predictable failure mode is fabricated or absent citations from sub-agents. prepare_context is the sub-agent injection path, where provenance matters most downstream. Per R14, the fix is "add ids", not "drop the instruction": - prepare-context.ts renders id:<8-char> on each scar header - format-utils.ts renders id:<8-char> on each compact line - format-utils.ts:102's fourth un-unified literal now imports CITATION_LINE. R12 unified recall/search/prepare_context and left this one behind, so the drift R12 fixed could silently reopen here. src/ diff is the two renderers plus the import, per the ruling's constraint. Coupling did not extend further: the only test that broke was formatCompact's assertion on the retired literal's prose ("factual claims"), which was the prose-coupling R13a exists to remove. Invariant (b) now binds all four surfaces, including one class-killing test — "every surface that instructs citation also renders citable ids" — so a fifth id-less-but-instructing surface is structurally impossible rather than merely absent. Mutation-checked to bind, by name; source restored byte-identical: - drop id from prepare_context -> its id test + the class-killer fail - drop id from formatCompact -> its id test + the class-killer fail - restore the drifted literal -> 3 tests fail, incl. the class-killer MEASURED COST (o200k_base, same ruler as the audit), correcting the ruling's ~3-token estimate: 6 tok/record for prepare_context, 7 for formatCompact, and the literal unification COSTS +3 once per payload rather than saving — the shared constant is longer than the fourth literal it replaced. At 5 scars: +30 and +38 tokens respectively. Still affordable honesty, and the recall path is untouched, so the 487/115 high/low-yield figures stand. Suite: 65 files, 1099 tests, 0 failures. tsc --noEmit clean. Co-Authored-By: Claude Opus 5 --- src/hooks/format-utils.ts | 14 ++- src/tools/prepare-context.ts | 5 +- tests/unit/tools/provenance-citation.test.ts | 111 ++++++++++++++----- 3 files changed, 101 insertions(+), 29 deletions(-) diff --git a/src/hooks/format-utils.ts b/src/hooks/format-utils.ts index f4a17b2..f9d9c7d 100644 --- a/src/hooks/format-utils.ts +++ b/src/hooks/format-utils.ts @@ -6,6 +6,8 @@ * - quick-retrieve.ts (hook-invoked retrieval for auto-inject) */ +import { CITATION_LINE } from "../services/display-protocol.js"; + // --- Types --- export interface FormattableScar { @@ -85,7 +87,10 @@ export function formatCompact( const emoji = SEVERITY_EMOJI[scar.severity] || "[?]"; const label = SEVERITY_LABEL[scar.severity] || "UNKNOWN"; const firstSentence = scar.description.split(/\.\s/)[0].slice(0, 120); - const line = `${emoji} ${label}: ${scar.title} \u2014 ${firstSentence}`; + // GIT-74/R14: the id travels with the scar. An instruction ships only on + // surfaces that render the capability to obey it \u2014 this line asks the + // sub-agent to cite record IDs, so the record ID has to be on it. ~3 tokens. + const line = `${emoji} ${label}: ${scar.title} \u2014 ${firstSentence} id:${scar.id.slice(0, 8)}`; // Check token budget before adding (always include at least one) const candidate = [...lines, line].join("\n"); @@ -97,9 +102,12 @@ export function formatCompact( included++; } - // Citation reminder for sub-agent context (compact — one line) + // Citation reminder for sub-agent context (compact — one line). + // GIT-74/R14: this was the fourth un-unified literal. R12 unified recall, + // search and prepare_context and left this one behind, so the drift it fixed + // could reopen here. One constant, four surfaces. if (included > 0) { - lines.push("Cite record IDs for any factual claims from these scars."); + lines.push(CITATION_LINE); } return { payload: lines.join("\n"), included }; diff --git a/src/tools/prepare-context.ts b/src/tools/prepare-context.ts index 3424698..61ae965 100644 --- a/src/tools/prepare-context.ts +++ b/src/tools/prepare-context.ts @@ -117,7 +117,10 @@ Proceed with caution — this may be new territory without documented lessons.`; for (const scar of scars) { const emoji = SEVERITY_EMOJI[scar.severity] || "[?]"; - lines.push(`${emoji} **${scar.title}** (${scar.severity}, score: ${(scar.similarity || 0).toFixed(2)})`); + // GIT-74/R14: the id travels with the scar. This is the sub-agent injection + // path, where provenance matters most downstream — absorbed observations + // should carry citable ids home. ~3 tokens per record. + lines.push(`${emoji} **${scar.title}** (${scar.severity}, score: ${(scar.similarity || 0).toFixed(2)}) id:${scar.id.slice(0, 8)}`); lines.push(scar.description); if (scar.counter_arguments && scar.counter_arguments.length > 0) { diff --git a/tests/unit/tools/provenance-citation.test.ts b/tests/unit/tools/provenance-citation.test.ts index 579c91b..c9f67ba 100644 --- a/tests/unit/tools/provenance-citation.test.ts +++ b/tests/unit/tools/provenance-citation.test.ts @@ -38,19 +38,35 @@ function makeFormattableScar(overrides: Partial = {}): Formatta } describe("formatCompact: citation protocol", () => { - it("includes citation reminder when scars are present", () => { + it("emits the shared citation rule when scars are present", () => { const scars = [makeFormattableScar()]; const { payload } = formatCompact(scars, "test plan", 2000); - expect(payload).toContain("Cite record IDs"); - expect(payload).toContain("factual claims"); + // GIT-74/R14: this surface carried the fourth un-unified literal + // ("Cite record IDs for any factual claims from these scars."). It now + // imports the one constant like the other three, so the assertion moves + // off the prose and onto the constant. + expect(payload).toContain(CITATION_LINE); + }); + + it("renders a citable record id for every scar", () => { + // GIT-74/R14: instruction and capability travel together. This payload + // tells a sub-agent to cite record IDs, so it must carry them. + const scars = [ + makeFormattableScar({ id: "eeeeeeee-1111-2222-3333-444444444444", title: "First" }), + makeFormattableScar({ id: "ffffffff-5555-6666-7777-888888888888", title: "Second" }), + ]; + const { payload } = formatCompact(scars, "test plan", 2000); + + expect(payload).toContain("id:eeeeeeee"); + expect(payload).toContain("id:ffffffff"); }); it("does not include citation reminder when no scars match", () => { const { payload } = formatCompact([], "empty plan", 2000); // Empty scars = just the header, no citation line - expect(payload).not.toContain("Cite record IDs"); + expect(payload).not.toContain(CITATION_LINE); }); it("citation reminder appears after all scar lines", () => { @@ -346,7 +362,7 @@ describe("prepare_context full: citation protocol", () => { expect(result.memory_payload).toContain(CITATION_LINE); }); - it("emits the citation rule byte-identical to the other surfaces", async () => { + it("emits the citation rule exactly once, as the shared constant", async () => { setupPrepareRemote([makePrepareContextScar()]); const result = await prepareContext({ @@ -363,6 +379,22 @@ describe("prepare_context full: citation protocol", () => { expect(occurrences).toBe(1); }); + it("renders a citable record id for every scar", async () => { + // GIT-74/R14: this surface instructed citation while rendering no ids at + // all — it commanded the impossible, and the predictable failure mode is + // fabricated or absent citations from sub-agents. Ids now travel with the + // scars, and this is the test that keeps them there. + setupPrepareRemote([ + makePrepareContextScar({ id: "12345678-1111-2222-3333-444444444444", title: "First" }), + makePrepareContextScar({ id: "87654321-5555-6666-7777-888888888888", title: "Second" }), + ]); + + const result = await prepareContext({ plan: "citable ids", format: "full" }); + + expect(result.memory_payload).toContain("id:12345678"); + expect(result.memory_payload).toContain("id:87654321"); + }); + it("omits CITATION RULE when no scars found (full format)", async () => { setupPrepareRemote([]); @@ -719,7 +751,7 @@ describe("recall: confidence tiers", () => { // Part 5: Cross-cutting provenance guarantees // ============================================================ -describe("provenance: citation rule is one constant, not three copies", () => { +describe("provenance: citation rule is one constant across all four surfaces", () => { // The predecessors of these two tests asserted a box-drawing separator and a // prose prefix ("CITATION RULE: When referencing facts from these"). The // separator was retired from the retrieval surfaces by the GIT-74 token audit @@ -729,40 +761,69 @@ describe("provenance: citation rule is one constant, not three copies", () => { // // The guarantee that actually matters: one exported constant, emitted whole // by every surface. Assert that, and the tests survive the next rewording. - - it("all three retrieval surfaces emit the identical citation constant", async () => { - setupSearchRemote([makeSearchScar()]); + // + // R12 unified three surfaces; formatCompact kept a fourth literal, so the + // drift R12 fixed could silently reopen on the hook path. R14 folded it in, + // and these tests now span all four. + + // One id per surface, so each assertion names the exact string that surface + // was given rather than guessing at a character class. + const SURFACE_ID = { + search: "5ea12c00-1111-2222-3333-444444444444", + recall: "5eca1100-5555-6666-7777-888888888888", + prepare_context: "9cc07e00-9999-aaaa-bbbb-cccccccccccc", + formatCompact: "c0m9ac70-dddd-eeee-ffff-000000000000", + } as const; + + /** Every surface that emits the citation rule, rendered from one fixture set. */ + async function allSurfaces(): Promise> { + setupSearchRemote([makeSearchScar({ id: SURFACE_ID.search })]); const searchDisplay = (await search({ query: "consistency" })).display!; - setupRecallRemote([makeRecallScar()]); + setupRecallRemote([makeRecallScar({ id: SURFACE_ID.recall })]); const recallDisplay = (await recall({ plan: "consistency" })).display!; - setupPrepareRemote([makePrepareContextScar()]); + setupPrepareRemote([makePrepareContextScar({ id: SURFACE_ID.prepare_context })]); const pcPayload = (await prepareContext({ plan: "consistency", format: "full" })) .memory_payload; - for (const surface of [searchDisplay, recallDisplay, pcPayload]) { - expect(surface).toContain(CITATION_LINE); + const compactPayload = formatCompact( + [makeFormattableScar({ id: SURFACE_ID.formatCompact })], + "consistency", + 2000, + ).payload; + + return { search: searchDisplay, recall: recallDisplay, prepare_context: pcPayload, formatCompact: compactPayload }; + } + + it("all four surfaces emit the identical citation constant", async () => { + for (const [name, surface] of Object.entries(await allSurfaces())) { + expect(surface, `${name} must emit CITATION_LINE`).toContain(CITATION_LINE); } }); it("no surface ships a second, drifted copy of the rule", async () => { // Catches the original defect class directly: a surface that keeps its own // literal alongside the import, or emits the rule twice. - setupSearchRemote([makeSearchScar()]); - const searchDisplay = (await search({ query: "duplication" })).display!; - - setupRecallRemote([makeRecallScar()]); - const recallDisplay = (await recall({ plan: "duplication" })).display!; - - setupPrepareRemote([makePrepareContextScar()]); - const pcPayload = (await prepareContext({ plan: "duplication", format: "full" })) - .memory_payload; - - for (const surface of [searchDisplay, recallDisplay, pcPayload]) { - expect(surface.split(CITATION_LINE).length - 1).toBe(1); + for (const [name, surface] of Object.entries(await allSurfaces())) { + expect(surface.split(CITATION_LINE).length - 1, `${name} must emit it exactly once`).toBe(1); // The retired prose must not reappear anywhere alongside the constant. expect(surface).not.toContain("CITATION RULE: When referencing facts"); + expect(surface).not.toContain("factual claims from these scars"); + } + }); + + it("every surface that instructs citation also renders citable ids", async () => { + // GIT-74/R14, the invariant behind invariant (b): an instruction ships only + // on surfaces that render the capability to obey it. This is the test that + // makes a fifth id-less-but-instructing surface structurally impossible. + const surfaces = await allSurfaces(); + + for (const [name, surface] of Object.entries(surfaces)) { + const expectedId = `id:${SURFACE_ID[name as keyof typeof SURFACE_ID].slice(0, 8)}`; + expect(surface, `${name} instructs citation, so it must render ${expectedId}`).toContain( + expectedId, + ); } }); }); From cd9cb98cb66b4be279e8ccd0082ea877b3f802a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 21:27:58 -0400 Subject: [PATCH 5/9] fix: every schema-accepted field persists for every learning_type (GIT-76/R13b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit applies_when was fixed on bff6cc8. Its two siblings were not: problem_context and solution_approach had the identical defect at the identical site — assigned only inside the `win` branch, so a scar, pattern or anti_pattern that submitted them had them validated, acknowledged and discarded. anti_pattern had no branch at all and so reached the row with no severity: the same omission one level up. Per R13b all four are normalized together rather than fixing one field and shipping the same bug three more times beside its own fix: - applies_when / problem_context / solution_approach move to a universal block applied for every learning_type - anti_pattern gets a severity default ("medium", matching win's neutral default — a cheap correction if it should sit higher) - win keeps its empty-string floor for the two prose fields, so its stored shape is unchanged as the acceptance criterion requires The scope guard is R13b's single class-killing test, not four targeted ones: every schema-accepted field must persist to the stored row for every learning_type, asserted against the row handed to directUpsert rather than the tool's success message. A success message describing submitted content instead of stored content is the defect class 1.7.0 exists to close. counter_arguments is asserted scar-only, confirming the per-field intent R12a asked for rather than assuming it. Shown to FAIL against the pre-fix code, by name — the tests were run against stashed HEAD and 7 failed, exactly the siblings: scar/pattern/anti_pattern x problem_context, solution_approach (6) anti_pattern severity (1) applies_when passed pre-fix, correctly, because bff6cc8 already fixed it. +40 tests (1 new file). Suite: 66 files, 1139 tests, 0 failures. tsc --noEmit clean. Still open on GIT-76: the eleven-learning recovery (R13c). Co-Authored-By: Claude Opus 5 --- src/tools/create-learning.ts | 36 ++- .../create-learning-field-persistence.test.ts | 228 ++++++++++++++++++ 2 files changed, 260 insertions(+), 4 deletions(-) create mode 100644 tests/unit/tools/create-learning-field-persistence.test.ts diff --git a/src/tools/create-learning.ts b/src/tools/create-learning.ts index e97103b..9b3fbe9 100644 --- a/src/tools/create-learning.ts +++ b/src/tools/create-learning.ts @@ -132,8 +132,26 @@ export async function createLearning( // call in confirm_scars, so its absence degraded scar utility and not merely // fidelity — which is how the GIT-74 token audit found it: the compact tier // had nothing to render. - if (params.applies_when !== undefined) { - learningData.applies_when = params.applies_when; + // GIT-76/R13b: problem_context and solution_approach had the identical + // defect at the identical site — assigned only in the `win` branch, so a + // scar or pattern that submitted them had them validated, acknowledged and + // discarded. Fixing applies_when alone would have shipped the same bug twice + // beside its own fix, so all three are normalized together here. + // + // The rule this section now encodes: a field the schema accepts is a field + // the row stores, for every learning_type. Type-specific blocks below are for + // fields that are genuinely type-specific (counter_arguments on scars) or for + // per-type defaults — never for deciding whether a universal field survives. + const universalOptionalFields = [ + "applies_when", + "problem_context", + "solution_approach", + ] as const; + + for (const field of universalOptionalFields) { + if (params[field] !== undefined) { + learningData[field] = params[field]; + } } // Add type-specific fields @@ -144,8 +162,11 @@ export async function createLearning( } if (params.learning_type === "win") { - learningData.problem_context = params.problem_context || ""; - learningData.solution_approach = params.solution_approach || ""; + // Wins have always written these two as "" rather than leaving them absent. + // GIT-76's acceptance requires win behaviour unchanged, so the empty-string + // floor is preserved here rather than folded into the universal loop. + learningData.problem_context ??= ""; + learningData.solution_approach ??= ""; learningData.severity = params.severity || "medium"; } @@ -153,6 +174,13 @@ export async function createLearning( learningData.severity = params.severity || "low"; } + // GIT-76/R13b: anti_pattern had no branch at all, so it fell through with no + // severity — the same class of omission, one level up. "medium" matches win's + // neutral default; the value is a cheap correction if it should sit higher. + if (params.learning_type === "anti_pattern") { + learningData.severity = params.severity || "medium"; + } + try { let embeddingGenerated = false; const breakdown: PerformanceBreakdown = {}; diff --git a/tests/unit/tools/create-learning-field-persistence.test.ts b/tests/unit/tools/create-learning-field-persistence.test.ts new file mode 100644 index 0000000..451c0d7 --- /dev/null +++ b/tests/unit/tools/create-learning-field-persistence.test.ts @@ -0,0 +1,228 @@ +/** + * create_learning field persistence — the class-killing test (GIT-76 / R13b) + * + * The defect this guards is not "applies_when was dropped". It is the SHAPE + * that dropped it: a field assigned inside one learning_type's branch is + * silently discarded for every other type. Schema acceptance is not + * persistence — the write validated the field, reported success, and stored + * nothing. + * + * Three fields had that exact shape at the exact same site (applies_when, + * problem_context, solution_approach) and anti_pattern had no branch at all. + * R13b's scope guard is therefore ONE parameterized assertion rather than four + * targeted ones: every schema-accepted field persists to the stored row, for + * every learning_type. That is what makes a fifth sibling structurally + * impossible instead of merely absent. + * + * The assertion is made against the row handed to directUpsert — the stored + * row — never against the tool's own success message. A success message + * describing submitted content rather than stored content is the defect class + * 1.7.0 exists to close. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// --- Mock dependencies (mirrors create-learning-errors.test.ts) --- + +const mockDirectUpsert = vi.fn(() => Promise.resolve({ id: "test-learning-id" })); + +vi.mock("../../../src/services/supabase-client.js", () => ({ + isConfigured: vi.fn(() => true), + directUpsert: (...args: unknown[]) => mockDirectUpsert(...args), +})); + +vi.mock("../../../src/services/tier.js", () => ({ + hasSupabase: vi.fn(() => true), + getTableName: vi.fn((base: string) => `orchestra_${base}`), + hasProInsights: () => false, +})); + +vi.mock("../../../src/services/embedding.js", () => ({ + embed: vi.fn(() => Promise.resolve(null)), + isEmbeddingAvailable: vi.fn(() => false), +})); + +vi.mock("../../../src/services/agent-detection.js", () => ({ + getAgentIdentity: vi.fn(() => "CLI"), +})); + +vi.mock("../../../src/services/display-protocol.js", () => ({ + wrapDisplay: vi.fn((msg: string) => msg), + TYPE: { scar: "S", win: "W", pattern: "P", anti_pattern: "A" }, + SEV: { critical: "!", high: "H", medium: "M", low: "L" }, +})); + +vi.mock("../../../src/services/startup.js", () => ({ + flushCache: vi.fn(() => Promise.resolve()), +})); + +vi.mock("../../../src/services/triple-writer.js", () => ({ + writeTriplesForLearning: vi.fn(() => Promise.resolve()), +})); + +vi.mock("../../../src/services/variant-generation.js", () => ({ + generateVariantsForScar: vi.fn(() => Promise.resolve()), +})); + +vi.mock("../../../src/services/effect-tracker.js", () => ({ + getEffectTracker: vi.fn(() => ({ + track: vi.fn((_cat: string, _label: string, fn: () => Promise) => fn()), + })), +})); + +vi.mock("../../../src/services/storage.js", () => ({ + getStorage: vi.fn(() => ({ + upsert: vi.fn(() => Promise.resolve()), + })), +})); + +vi.mock("../../../src/services/session-state.js", () => ({ + getProject: vi.fn(() => "test-project"), +})); + +vi.mock("../../../src/services/metrics.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + recordMetrics: vi.fn(() => Promise.resolve()), + }; +}); + +vi.mock("uuid", () => ({ + v4: vi.fn(() => "test-uuid-learning"), +})); + +import { createLearning } from "../../../src/tools/create-learning.js"; +import type { CreateLearningParams, LearningType } from "../../../src/types/index.js"; + +beforeEach(() => { + vi.clearAllMocks(); + mockDirectUpsert.mockResolvedValue({ id: "test-learning-id" }); +}); + +/** The row actually handed to the store, not the tool's success message. */ +function storedRow(): Record { + expect(mockDirectUpsert, "the write must have been attempted").toHaveBeenCalled(); + return mockDirectUpsert.mock.calls[0][1] as unknown as Record; +} + +const ALL_TYPES: LearningType[] = ["scar", "win", "pattern", "anti_pattern"]; + +/** + * Every optional field the schema accepts, with a distinctive value and the + * check that proves it survived. Add a field to CreateLearningParams and it + * belongs here — that is the point of the table. + */ +const SCHEMA_FIELDS: Array<{ + field: keyof CreateLearningParams; + value: unknown; + /** Fields that are type-specific BY DESIGN, with the reason. */ + onlyFor?: LearningType[]; +}> = [ + { field: "applies_when", value: ["UNIQUE_APPLIES_WHEN"] }, + { field: "problem_context", value: "UNIQUE_PROBLEM_CONTEXT" }, + { field: "solution_approach", value: "UNIQUE_SOLUTION_APPROACH" }, + { field: "why_this_matters", value: "UNIQUE_WHY_THIS_MATTERS" }, + { field: "action_protocol", value: ["UNIQUE_ACTION_PROTOCOL"] }, + { field: "self_check_criteria", value: ["UNIQUE_SELF_CHECK"] }, + { field: "keywords", value: ["UNIQUE_KEYWORD"] }, + { field: "domain", value: ["UNIQUE_DOMAIN"] }, + // counter_arguments is scar-only by design: R12a called for the per-field + // intent to be confirmed rather than assumed, and this is that confirmation. + { field: "counter_arguments", value: ["UNIQUE_CA_ONE", "UNIQUE_CA_TWO"], onlyFor: ["scar"] }, +]; + +/** Minimum valid params for a type, so validation never masks a drop. */ +function baseParams(type: LearningType): CreateLearningParams { + return { + learning_type: type, + title: `Test ${type}`, + description: `Description for ${type}`, + ...(type === "scar" && { + severity: "high" as const, + counter_arguments: ["You might think X", "But Y"], + }), + }; +} + +describe("create_learning: every schema-accepted field persists, for every learning_type", () => { + // The class-killer. One assertion, every type × every universal field. + for (const type of ALL_TYPES) { + for (const { field, value, onlyFor } of SCHEMA_FIELDS) { + if (onlyFor && !onlyFor.includes(type)) continue; + + it(`${type}: persists ${String(field)} to the stored row`, async () => { + const result = await createLearning({ + ...baseParams(type), + [field]: value, + } as CreateLearningParams); + + expect(result.success, `create must succeed: ${JSON.stringify(result.errors)}`).toBe(true); + expect( + storedRow()[field as string], + `${String(field)} was accepted for ${type} but never reached the row`, + ).toEqual(value); + }); + } + } +}); + +describe("create_learning: type-specific fields stay type-specific", () => { + it("counter_arguments is written only for scars", async () => { + for (const type of ALL_TYPES.filter((t) => t !== "scar")) { + vi.clearAllMocks(); + mockDirectUpsert.mockResolvedValue({ id: "test-learning-id" }); + + await createLearning({ + ...baseParams(type), + counter_arguments: ["UNIQUE_SHOULD_NOT_PERSIST"], + } as CreateLearningParams); + + expect(storedRow().counter_arguments, `${type} must not carry counter_arguments`) + .toBeUndefined(); + } + }); +}); + +describe("create_learning: every learning_type gets a severity", () => { + // anti_pattern had no branch at all, so it fell through with severity + // undefined — the same omission one level up from the field drops. + for (const type of ALL_TYPES) { + it(`${type}: stored row carries a severity even when none is submitted`, async () => { + const params = baseParams(type); + if (type !== "scar") delete (params as { severity?: unknown }).severity; + + const result = await createLearning(params); + + expect(result.success).toBe(true); + expect(storedRow().severity, `${type} must not reach the row without a severity`) + .toBeDefined(); + }); + } +}); + +describe("create_learning: win behaviour unchanged (GIT-76 regression)", () => { + // R13b normalizes three fields into a universal block; win's long-standing + // empty-string floor for two of them must survive that refactor. + it("win still writes problem_context and solution_approach as '' when omitted", async () => { + const result = await createLearning({ + learning_type: "win", + title: "Test win", + description: "No context or approach submitted", + }); + + expect(result.success).toBe(true); + expect(storedRow().problem_context).toBe(""); + expect(storedRow().solution_approach).toBe(""); + }); + + it("win still defaults severity to medium", async () => { + await createLearning({ + learning_type: "win", + title: "Test win", + description: "No severity submitted", + }); + + expect(storedRow().severity).toBe("medium"); + }); +}); From 0771f74b7698b43b99b8db1176fd1fea54155683 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 21:40:26 -0400 Subject: [PATCH 6/9] test: re-runnable GIT-69 cross-project panel repro (acceptance, read-only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consumer verification for GIT-69's 2026-08-05 defect: session_start's panel showed a foreign-project thread while list_threads correctly excluded it. Runs both surfaces against the LOCAL BUILD over stdio — the session's own MCP tooling serves published 1.6.6 and would green-light unfixed code — then resolves every panel entry's true project by reading orchestra_threads directly, because a tool confirming itself is not confirmation. READ-ONLY against orchestra_threads per the acceptance-venue ruling (GIT-70 comment 10b75c26): production is evidence, never fixture. session_start does insert its own session row in orchestra_sessions — an ordinary operational write, not a seeded fixture — and the header says so rather than hiding it. Two instrument defects were found and fixed while building this, both of which produced FALSE FAILURES and either could have produced a false pass on a differently-shaped defect: - the panel renders truncated thread TEXT and no ids, so an id regex over it returns zero matches — which reads as "no leak" but means "not measured" - thread rows carry lifecycle status "emerging", not a literal "open", so status=eq.open excluded the very rows being matched against Both are the measurement-validity class (scar 7e8bda56). The script therefore carries an instrument self-test: it feeds the detector a thread that really does belong to another project and asserts a LEAK is reported. A repro that cannot fail is not a repro. Result on the branch build: panel claims 2 / prints 2, list_threads 2, parity exact, both gitmem, no weekend_warrior threads. Self-test detects a real orchestra_dev thread. Co-Authored-By: Claude Opus 5 --- scripts/git-69-panel-repro.mts | 222 +++++++++++++++++++++++++++++++++ 1 file changed, 222 insertions(+) create mode 100644 scripts/git-69-panel-repro.mts diff --git a/scripts/git-69-panel-repro.mts b/scripts/git-69-panel-repro.mts new file mode 100644 index 0000000..8257b41 --- /dev/null +++ b/scripts/git-69-panel-repro.mts @@ -0,0 +1,222 @@ +/** + * GIT-69 consumer verification — the 2026-08-05 cross-project leak repro. + * + * The original defect (desktop session 096e940d): session_start(project=gitmem) + * displayed a weekend_warrior thread (t-4120fdad) in its panel while + * list_threads(project=gitmem) correctly excluded it. Two code paths, two + * answers, and the panel was the one that leaked. + * + * This runs BOTH surfaces against the LOCAL BUILD over stdio — not through the + * session's own MCP tooling, which serves published 1.6.6 and would green-light + * unfixed code. It then resolves every surfaced thread's true project by + * reading orchestra_threads directly, because the acceptance criterion says + * "confirmed against orchestra_threads directly" and a tool confirming itself + * is not confirmation. + * + * READ-ONLY against orchestra_threads, per the acceptance-venue ruling + * (GIT-70 comment 10b75c26): production is evidence, never fixture. Nothing + * here writes a thread. session_start does insert its own session row in + * orchestra_sessions — an ordinary operational write, not a seeded fixture — + * and that is disclosed rather than hidden. + * + * Credentials are read from the MCP config and never printed. + * + * npx tsx scripts/git-69-panel-repro.mts + */ + +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import { readFileSync } from "fs"; +import { homedir, tmpdir } from "os"; +import { join, dirname } from "path"; +import { fileURLToPath } from "url"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const REPO = join(__dirname, ".."); +const SERVER = join(REPO, "dist", "index.js"); + +const TARGET_PROJECT = "gitmem"; +const FOREIGN_PROJECT = "weekend_warrior"; + +function liveEnv(): Record { + const cfg = JSON.parse(readFileSync(join(homedir(), ".claude.json"), "utf-8")); + let found: Record | null = null; + const walk = (node: unknown): void => { + if (!node || typeof node !== "object") return; + const obj = node as Record; + const servers = obj.mcpServers as Record }> | undefined; + if (servers?.gitmem?.env && !found) found = servers.gitmem.env; + for (const k of Object.keys(obj)) walk(obj[k]); + }; + walk(cfg); + if (!found) throw new Error("gitmem MCP env not found in ~/.claude.json"); + const env = found as Record; + for (const k of ["SUPABASE_URL", "SUPABASE_SERVICE_ROLE_KEY"]) { + if (!env[k]) throw new Error(`missing ${k}`); + } + return env; +} + +const env = liveEnv(); +const BASE = `${env.SUPABASE_URL.replace(/\/$/, "")}/rest/v1`; +const TABLE = `${env.GITMEM_TABLE_PREFIX || "orchestra_"}threads`; +const authHeaders = { + apikey: env.SUPABASE_SERVICE_ROLE_KEY, + Authorization: `Bearer ${env.SUPABASE_SERVICE_ROLE_KEY}`, +}; + +/** Every thread id mentioned in a surface's output. */ +function threadIds(text: string): Set { + return new Set(text.match(/\bt-[0-9a-f]{8}\b/g) ?? []); +} + +/** + * The panel renders truncated thread TEXT and no ids — verified by dumping the + * raw tool result, which carries `content` only and no structuredContent. So + * the panel's entries are matched back to rows by text prefix rather than by + * id. Stated explicitly because an id-regex over this surface silently returns + * zero matches and would read as "no leak" when it means "not measured". + */ +function panelThreadPrefixes(text: string): string[] { + const lines = text.split("\n"); + const header = lines.findIndex((l) => /^Threads \(\d+\)/.test(l.trim())); + if (header === -1) return []; + const out: string[] = []; + for (const line of lines.slice(header + 1)) { + if (!line.startsWith(" ") || !line.trim()) break; + out.push(line.trim().replace(/\.\.\.$/, "").trim()); + } + return out; +} + +/** The count the panel claims, independent of how many lines it prints. */ +function panelClaimedCount(text: string): number | null { + const m = text.match(/^Threads \((\d+)\)/m); + return m ? Number(m[1]) : null; +} + +function textOf(result: unknown): string { + const content = (result as { content?: { type: string; text?: string }[] }).content ?? []; + return content.map((c) => c.text ?? "").join("\n"); +} + +async function main() { + const transport = new StdioClientTransport({ + command: "node", + args: [SERVER], + env: { + ...process.env, + ...env, + NO_COLOR: "1", + GITMEM_HOME: join(tmpdir(), `git69-repro-${Date.now()}`), + } as Record, + }); + const client = new Client({ name: "git-69-repro", version: "1.0.0" }, { capabilities: {} }); + await client.connect(transport); + + console.log(`GIT-69 panel repro — local build, project=${TARGET_PROJECT}\n`); + + const started = await client.callTool({ + name: "session_start", + arguments: { project: TARGET_PROJECT, agent_identity: "cli" }, + }); + const panelText = textOf(started); + const prefixes = panelThreadPrefixes(panelText); + const claimed = panelClaimedCount(panelText); + + const listed = await client.callTool({ + name: "list_threads", + arguments: { project: TARGET_PROJECT, status: "open" }, + }); + const listText = textOf(listed); + const listIds = threadIds(listText); + + await client.close().catch(() => {}); + + console.log(`session_start panel : claims ${claimed}, prints ${prefixes.length} entries`); + console.log(`list_threads : ${listIds.size} thread ids\n`); + + const failures: string[] = []; + if (claimed !== null && claimed !== prefixes.length) { + failures.push(`panel claims ${claimed} threads but prints ${prefixes.length}`); + } + + // --- Resolve every panel entry against the table, ACROSS ALL PROJECTS --- + // Scoping this read to gitmem would assume the answer: a leaked thread would + // simply not be found and would look like a clean panel. + // No status filter: thread rows carry lifecycle values like "emerging", not + // a literal "open", so filtering on status=open silently returns the wrong + // set and every panel entry reads as unmatched. Unresolved is the criterion + // that matters here, and resolved threads simply will not appear in a panel. + const res = await fetch( + `${BASE}/${TABLE}?resolved_at=is.null&select=thread_id,project,text,status&limit=1000`, + { headers: authHeaders }, + ); + if (!res.ok) throw new Error(`read threads: ${res.status} ${await res.text()}`); + const rows = (await res.json()) as { thread_id: string; project: string; text: string; status: string }[]; + + console.log("panel contents, project resolved from orchestra_threads (all projects):"); + const matchedIds = new Set(); + for (const prefix of prefixes) { + const hits = rows.filter((r) => (r.text ?? "").startsWith(prefix)); + if (hits.length === 0) { + failures.push(`panel entry matched no open thread row: "${prefix.slice(0, 50)}"`); + console.log(` ?? (no row matched) "${prefix.slice(0, 46)}"`); + continue; + } + for (const h of hits) matchedIds.add(h.thread_id); + const projects = [...new Set(hits.map((h) => h.project))]; + const leaked = projects.filter((p) => p !== TARGET_PROJECT); + const flag = leaked.length ? "LEAK" : "ok "; + console.log(` ${flag} ${hits.map((h) => h.thread_id).join(",")} project=${projects.join(",")} "${prefix.slice(0, 40)}"`); + if (leaked.length) { + failures.push(`panel leaked thread from project(s) ${leaked.join(",")}: "${prefix.slice(0, 50)}"`); + } + if (projects.includes(FOREIGN_PROJECT)) { + failures.push(`panel contains a ${FOREIGN_PROJECT} thread: "${prefix.slice(0, 50)}"`); + } + } + + // Criterion 2: the two surfaces agree — the original defect was disagreement. + const onlyPanel = [...matchedIds].filter((id) => !listIds.has(id)); + const onlyList = [...listIds].filter((id) => !matchedIds.has(id)); + console.log(`\nparity: panel-only=${onlyPanel.length} list-only=${onlyList.length}`); + if (onlyPanel.length) failures.push(`in panel but not list_threads: ${onlyPanel.join(", ")}`); + if (onlyList.length) failures.push(`in list_threads but not panel: ${onlyList.join(", ")}`); + + // --- Instrument self-test ----------------------------------------------- + // A repro that cannot fail is not a repro. Two earlier versions of this + // script reported FAILED for instrument reasons (the panel renders no ids; + // thread rows carry status "emerging", not "open"), which is the same class + // in reverse — so the detector is now exercised against a thread that really + // does belong to another project, and must flag it. + const foreignRow = rows.find((r) => r.project && r.project !== TARGET_PROJECT); + if (!foreignRow) { + console.log("\nself-test SKIPPED — no foreign-project thread exists to detect with"); + failures.push("instrument unvalidated: no foreign-project row available"); + } else { + const syntheticPrefix = (foreignRow.text ?? "").slice(0, 45); + const hits = rows.filter((r) => (r.text ?? "").startsWith(syntheticPrefix)); + const detected = hits.some((h) => h.project !== TARGET_PROJECT); + console.log( + `\nself-test: feeding the detector a real ${foreignRow.project} thread ` + + `(${foreignRow.thread_id}) → ${detected ? "LEAK detected, instrument binds" : "NOT DETECTED"}`, + ); + if (!detected) failures.push("instrument does not detect a known foreign-project thread"); + } + + console.log(""); + if (failures.length) { + console.log("REPRO FAILED — the defect is still present:"); + for (const f of failures) console.log(` ${f}`); + process.exitCode = 1; + } else { + console.log(`PASS — panel contains no ${FOREIGN_PROJECT} threads and agrees with list_threads,`); + console.log(" both confirmed against orchestra_threads directly."); + } +} + +main().catch((e) => { + console.error(e); + process.exitCode = 1; +}); From 71581d54db67c5b877701e5aa8c7625d5a81093a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 21:42:08 -0400 Subject: [PATCH 7/9] =?UTF-8?q?fix:=20isolate=20the=20GIT-69=20repro's=20c?= =?UTF-8?q?wd=20=E2=80=94=20GITMEM=5FHOME=20alone=20does=20not=20contain?= =?UTF-8?q?=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first run of this harness registered its session in the REPO's .gitmem/active-sessions.json even though GITMEM_HOME pointed at a temp directory. The server resolves its state directory by walking UP from the working directory looking for .gitmem, and that walk-up beats the env var. Consequence, observed: five enforcement unit tests flipped to failing ("warns when recall called without session" and four siblings). They were not broken by any code change — they resolve the same real .gitmem, adopt whatever session they find there, and so stop seeing a no-session state. Clearing the orphaned entry returned the suite to 1139/1139 green with no source change. Two findings, both worth their own tickets: 1. GITMEM_HOME is not sufficient to relocate server state; the cwd walk-up wins. Any tool or test that sets it and assumes isolation does not have isolation. 2. tests/unit/services/enforcement.test.ts is not hermetic. Its result depends on whether a session happens to be registered on the developer's machine, so it can go red locally with clean code, and could go green against a real no-session defect if a stray session exists. Fixed here by pinning the child's cwd to the sandbox as well as GITMEM_HOME. Re-verified: repro passes, repo registry stays empty, suite 1139/1139. Co-Authored-By: Claude Opus 5 --- scripts/git-69-panel-repro.mts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/scripts/git-69-panel-repro.mts b/scripts/git-69-panel-repro.mts index 8257b41..c8bcbf4 100644 --- a/scripts/git-69-panel-repro.mts +++ b/scripts/git-69-panel-repro.mts @@ -26,7 +26,7 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; -import { readFileSync } from "fs"; +import { readFileSync, mkdirSync } from "fs"; import { homedir, tmpdir } from "os"; import { join, dirname } from "path"; import { fileURLToPath } from "url"; @@ -101,14 +101,28 @@ function textOf(result: unknown): string { } async function main() { + // The child MUST run with its cwd outside the repo. + // + // GITMEM_HOME alone is NOT sufficient: the server resolves its state + // directory by walking UP from the working directory looking for .gitmem, + // and that walk-up wins. An earlier version of this script set GITMEM_HOME + // to a temp dir and still registered its session in the repo's + // .gitmem/active-sessions.json — which then made five enforcement unit tests + // fail, because they resolve the same real directory and adopt whatever + // session they find there. A "read-only" harness that mutates repo state is + // not read-only, so the isolation is pinned here by cwd as well. + const sandbox = join(tmpdir(), `git69-repro-${Date.now()}`); + mkdirSync(sandbox, { recursive: true }); + const transport = new StdioClientTransport({ command: "node", args: [SERVER], + cwd: sandbox, env: { ...process.env, ...env, NO_COLOR: "1", - GITMEM_HOME: join(tmpdir(), `git69-repro-${Date.now()}`), + GITMEM_HOME: sandbox, } as Record, }); const client = new Client({ name: "git-69-repro", version: "1.0.0" }, { capabilities: {} }); From 7910760660b42ea489fd38e9b29be09cfdbbee09 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 22:11:52 -0400 Subject: [PATCH 8/9] =?UTF-8?q?test:=20repair=20the=20smoke-pro=20gate=20i?= =?UTF-8?q?nstrument=20(GIT-74/R16a)=20=E2=80=94=20tests=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate item (c) includes smoke-pro. The suite could not execute against a live store, and had been auto-skipping all sprint on describe.skipIf(!HAS_SUPABASE) because no credentials existed — so it was reporting green by never running. mcp-client.ts's own comment predicted exactly this: "a suite that silently skips its only real assertion is the same lie as a green message over an unwritten row." Bounds honoured: tests/ diff only, zero src/ changes. - parseToolResult: tools with a `display` field return rendered text and no machine-data blob (server.ts:417-430, deliberate). Calling it on those surfaced as a bare SyntaxError; it now names the contract instead. - querySessionRow: sibling of queryThreadRow. Since the response no longer carries a performance object, "did session_start reach Supabase" is answered by the row it wrote — a stronger assertion than the one it replaces, which could pass on a latency number with nothing persisted. - recall: asserts the render, and no longer assumes a populated corpus. The venue is a deliberately blank project; "returns semantic matches" was a fixture assumption. Now: a cited hit or an explicit miss, never neither. - smoke server runs with cwd + GITMEM_HOME in a sandbox. Without it the suite resolved the developer's real .gitmem (GIT-80) and session_close tried to sync local threads into the smoke target, producing foreign-key violations that were local state leaking into a test, not product defects. Result against gitmem-e2e: 4/5, up from 2/5, verified by reading the parsed output rather than the green count. The 5th failure is NOT instrument staleness and is NOT fixed here — it is a real product/schema defect the repaired instrument caught within minutes: session_close spreads the local session record into the Supabase upsert, and that record carries a `display` field no store defines. Confirmed absent from schema/setup.sql AND from production orchestra_sessions. It fails HONESTLY ("close · FAILED", "Failed to persist session"), which is 1.7.0's contract working as designed — on 1.6.6 this reported success. Fixing it needs src/ or a schema change, both outside R16a's bounds. Posted for ruling. Also observed, not fixed: analytics queries `public.scar_usage` unprefixed, bypassing GITMEM_TABLE_PREFIX — same class as GIT-73's hardcoded `gitmem_query_metrics`. Non-fatal. Unit suite 1139/1139, tsc clean. Co-Authored-By: Claude Opus 5 --- tests/e2e/mcp-client.ts | 49 ++++++++++++- tests/smoke/helpers.ts | 1 + tests/smoke/smoke-pro.test.ts | 132 +++++++++++++++++++--------------- 3 files changed, 123 insertions(+), 59 deletions(-) diff --git a/tests/e2e/mcp-client.ts b/tests/e2e/mcp-client.ts index 3f19790..a4d28c7 100644 --- a/tests/e2e/mcp-client.ts +++ b/tests/e2e/mcp-client.ts @@ -155,7 +155,54 @@ export function parseToolResult(result: ToolCallResult): T { text = text.slice(0, -3); } - return JSON.parse(text.trim()) as T; + // GIT-74/R16a: tools that return a `display` field respond with the rendered + // display text and NO machine-data blob (server.ts: "No separate machine-data + // blob: it bloats the response and causes CLI auto-collapse"). Calling this + // on such a tool used to surface as a bare `SyntaxError: Unexpected token '('` + // — a stack trace where a diagnosis belongs. Say what the contract is instead. + const trimmed = text.trim(); + if (!trimmed.startsWith("{") && !trimmed.startsWith("[")) { + throw new Error( + "parseToolResult: this tool returned display text, not JSON. Tools with a " + + "`display` field respond with rendered text only — assert over " + + "getToolResultText(), or verify the stored row directly. " + + `Got: ${trimmed.slice(0, 60)}…` + ); + } + + return JSON.parse(trimmed) as T; +} + +/** + * Read a session row straight from the store. + * + * Sibling of queryThreadRow, and the honest replacement for asserting Supabase + * connectivity from a response body: since GIT-74 the response is display text, + * so "did session_start reach Supabase" is answered by the row it wrote, not by + * a performance object the tool no longer returns. + */ +export async function querySessionRow( + sessionId: string, + config: { url: string; key: string; tablePrefix?: string } +): Promise | null> { + const table = `${config.tablePrefix ?? "orchestra_"}sessions`; + const endpoint = `${config.url}/rest/v1/${table}?id=eq.${encodeURIComponent(sessionId)}&select=id,agent,project`; + + const response = await fetch(endpoint, { + headers: { + apikey: config.key, + Authorization: `Bearer ${config.key}`, + }, + }); + + if (!response.ok) { + throw new Error( + `Row query failed (${response.status} ${response.statusText}) — store unreachable, NOT proof of absence` + ); + } + + const rows = (await response.json()) as Record[]; + return rows.length > 0 ? rows[0] : null; } /** diff --git a/tests/smoke/helpers.ts b/tests/smoke/helpers.ts index 5cef3a1..46308c1 100644 --- a/tests/smoke/helpers.ts +++ b/tests/smoke/helpers.ts @@ -18,6 +18,7 @@ export { createOutageEnv, restartServer, queryThreadRow, + querySessionRow, hasLiveSupabase, CORE_TOOLS, PRO_TOOLS, diff --git a/tests/smoke/smoke-pro.test.ts b/tests/smoke/smoke-pro.test.ts index 16797ef..1101d79 100644 --- a/tests/smoke/smoke-pro.test.ts +++ b/tests/smoke/smoke-pro.test.ts @@ -17,12 +17,15 @@ */ import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { mkdtempSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; import { createMcpClient, callTool, listTools, - parseToolResult, getToolResultText, + querySessionRow, isToolError, timedStep, EXPECTED_TOOL_COUNTS, @@ -40,10 +43,28 @@ describe.skipIf(!HAS_SUPABASE)("Smoke: Pro Tier", () => { let sessionId: string; beforeAll(async () => { + // GIT-74/R16a: the smoke server MUST run with its cwd outside the repo. + // + // Without this it resolves the developer's real .gitmem by walking up from + // the working directory (GIT-80 — GITMEM_HOME alone does not prevent it), + // and then session_close tries to sync the developer's actual threads and + // sessions into the smoke target. Against a blank store that surfaces as + // foreign-key violations: + // + // Key (source_session)=(057f7a50-…) is not present in table "gitmem_sessions" + // + // which is real local state leaking into a test, not a product defect. A + // smoke suite that reads the machine it runs on is measuring the machine. + const sandbox = mkdtempSync(join(tmpdir(), "gitmem-smoke-pro-")); + const { result } = await timedStep("Server starts (pro)", async () => { - return createMcpClient({ - GITMEM_TIER: "pro", - }); + return createMcpClient( + { + GITMEM_TIER: "pro", + GITMEM_HOME: sandbox, + }, + { cwd: sandbox } + ); }); mcp = result; }, 15_000); @@ -88,36 +109,27 @@ describe.skipIf(!HAS_SUPABASE)("Smoke: Pro Tier", () => { expect(isToolError(result)).toBe(false); - const data = parseToolResult<{ - session_id: string; - agent: string; - last_session: { id: string; title: string; date: string } | null; - recent_decisions?: Array<{ id: string; title: string }>; - performance: { - latency_ms: number; - total_latency_ms: number; - network_calls_made: number; - }; - }>(result); - - sessionId = data.session_id; - - // Valid UUID - expect(data.session_id).toMatch( - /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/ + // GIT-74/R16a: session_start returns display text, not a JSON body — the + // machine-data blob was removed deliberately. So the session id comes from + // the render, and "connected to Supabase" is proved by the row it wrote + // rather than by a performance object the tool no longer returns. That is + // a stronger assertion than the one it replaces: the old test could pass + // against a latency number with nothing persisted behind it. + const text = getToolResultText(result); + const uuid = text.match( + /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/ ); - expect(data.agent).toBe("CLI"); + expect(uuid, `no session id in session_start display:\n${text.slice(0, 200)}`).not.toBeNull(); + sessionId = uuid![0]; - // Performance breakdown proves Supabase connected - expect(data.performance).toBeDefined(); - expect(data.performance.total_latency_ms).toBeGreaterThan(0); - - // last_session field present (null is fine for first-ever session) - expect("last_session" in data).toBe(true); - - // relevant_scars no longer in session_start result - // Scars load on-demand via recall() - expect(data).not.toHaveProperty("relevant_scars"); + const row = await querySessionRow(sessionId, { + url: process.env.SUPABASE_URL!, + key: process.env.SUPABASE_SERVICE_ROLE_KEY!, + tablePrefix: process.env.GITMEM_TABLE_PREFIX, + }); + expect(row, "session_start reported a session id with no row behind it").not.toBeNull(); + expect(row!.id).toBe(sessionId); + expect(row!.project).toBe("test-project"); // session_start should complete under 5s with Supabase expect(latencyMs).toBeLessThan(5000); @@ -137,29 +149,23 @@ describe.skipIf(!HAS_SUPABASE)("Smoke: Pro Tier", () => { expect(isToolError(result)).toBe(false); - const data = parseToolResult<{ - activated: boolean; - plan: string; - scars: Array<{ - id: string; - title: string; - severity: string; - similarity: number; - description: string; - }>; - }>(result); - - expect(data.plan).toBe("deploy to production"); - expect(Array.isArray(data.scars)).toBe(true); - - // Validate scar structure if any returned - if (data.scars.length > 0) { - const scar = data.scars[0]; - expect(scar.id).toBeDefined(); - expect(scar.title.length).toBeGreaterThan(0); - expect(scar.similarity).toBeGreaterThan(0); - expect(scar.description.length).toBeGreaterThan(0); - } + // GIT-74/R16a: recall renders display text, so the assertion moves onto the + // render. It must also not assume a populated corpus — this suite's venue is + // a deliberately blank e2e project, and the old test's "returns semantic + // matches" expectation was a fixture assumption, not a contract. + // + // The contract that actually holds either way: recall answers, and it is + // unambiguous about which case it is in. When scars ARE present they carry + // the citable id form the citation rule demands (GIT-74/R14). + const text = getToolResultText(result); + expect(text.length).toBeGreaterThan(0); + + const foundScars = /\bid:[0-9a-z]{8}\b/.test(text); + const saidNone = /no relevant scars|no past lessons/i.test(text); + expect( + foundScars || saidNone, + `recall was neither a cited hit nor an explicit miss:\n${text.slice(0, 300)}` + ).toBe(true); // Recall should complete under 3s expect(latencyMs).toBeLessThan(3000); @@ -178,9 +184,19 @@ describe.skipIf(!HAS_SUPABASE)("Smoke: Pro Tier", () => { const text = getToolResultText(result); expect(text.length).toBeGreaterThan(0); - // Verify no error payload hidden in the response - const data = parseToolResult>(result); - expect(data).not.toHaveProperty("error"); + // GIT-74/R16a: the old check parsed the response as JSON and asserted no + // "error" key. Against display text that threw before it could assert + // anything — and the test is named "persists", so the honest check is the + // row, not the absence of a word in a string. + expect(text).not.toMatch(/\berror\b/i); + + const row = await querySessionRow(sessionId, { + url: process.env.SUPABASE_URL!, + key: process.env.SUPABASE_SERVICE_ROLE_KEY!, + tablePrefix: process.env.GITMEM_TABLE_PREFIX, + }); + expect(row, "session_close returned without the session row surviving").not.toBeNull(); + expect(row!.id).toBe(sessionId); }); it("cache_status reports initialized", async () => { From 6d6092bbe45e6086c0241807a5e4db35eef2ca2b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 22:18:17 -0400 Subject: [PATCH 9/9] fix: filter the session upsert to the table's known columns (GIT-74/R17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session_close builds its Supabase payload by spreading the existing session record. On the Supabase-miss path that record is the LOCAL file record — a different shape, carrying rendering fields like `display`. Postgres rejects the whole upsert on the first unknown key (PGRST204), so one local-only field failed the entire close: ((●)) gitmem ── close · FAILED Failed to persist session: Could not find the 'display' column of 'gitmem_sessions' in the schema cache A fresh Pro install provisioned from schema/setup.sql could not close its first session. Not a regression — 1.7.0 made a pre-existing silent failure loud. Fixes the CLASS, not the instance: filter to the known column set, so any future local-only field is inert rather than fatal. Adding a `display` column was rejected — a rendering field does not become persisted state to appease an upsert. THE COLUMN SET IS A UNION, DELIBERATELY. schema/setup.sql is the canonical source of truth, but it is STALE: the live production table carries ELEVEN columns it does not define (children, claude_code_session_id, compacted*, insights, metrics, task_observations, blocked_by, handover_linear_slug, pre_compaction_summary) and lacks updated_at, which setup.sql has. Filtering to setup.sql alone would have silently dropped real production data on every close — the same disease pointed the other way, and worse, because data loss reports nothing. Both origins are enumerated and commented in src/services/session-columns.ts. The setup.sql/production drift is itself a defect, tracked separately; do not trim this list until they agree. Shown to FAIL against pre-fix behaviour per scar d72bd8c0: with the filter mutated to a pass-through (which is what the code did before), 3 tests fail by name — the `display` drop, the any-unknown-key drop, and the realistic close payload. The 5 inverse-guard tests correctly still pass, since pass-through never dropped a real column. Verified at the persisted row, not the banner: session 1e5908a8 in gitmem-e2e carries close_compliance.close_type="quick". Every pre-fix run in that project (02:01–02:10) shows only "superseded" or none — no quick close had ever landed before this commit. smoke-pro against gitmem-e2e: 5/5 (was 2/5, then 4/5 after the instrument fix) full suite: 67 files, 1147 tests, 0 failures tsc --noEmit clean Not touched, files as 1.7.1: analytics queries public.scar_usage unprefixed, bypassing GITMEM_TABLE_PREFIX (GIT-73's family). Co-Authored-By: Claude Opus 5 --- src/services/session-columns.ts | 93 ++++++++++++++ src/tools/session-close.ts | 9 +- tests/unit/services/session-columns.test.ts | 129 ++++++++++++++++++++ 3 files changed, 230 insertions(+), 1 deletion(-) create mode 100644 src/services/session-columns.ts create mode 100644 tests/unit/services/session-columns.test.ts diff --git a/src/services/session-columns.ts b/src/services/session-columns.ts new file mode 100644 index 0000000..bf46439 --- /dev/null +++ b/src/services/session-columns.ts @@ -0,0 +1,93 @@ +/** + * The persisted column set for the sessions table (GIT-74/R17). + * + * WHY THIS EXISTS + * + * session_close builds its upsert payload by spreading the *existing* session + * record and layering the close on top: + * + * sessionData = { ...existingWithoutEmbedding, close_compliance } + * + * When the session is not found in Supabase it falls back to the LOCAL file + * record, whose shape is not the row's shape — it carries rendering fields like + * `display`. Postgres rejects the whole upsert on the first unknown key + * (PGRST204), so one local-only field fails the entire close. A fresh Pro + * install provisioned from schema/setup.sql could not close its first session. + * + * The fix filters to known columns rather than deleting `display` by name, + * because the defect is the shape-drift CLASS, not that one instance. Any + * future local-only field is now inert instead of fatal. + * + * SOURCE OF TRUTH — read this before editing. + * + * This list is the UNION of two things, and it has to be, because they differ: + * + * 1. `schema/setup.sql` — what a fresh install creates. The canonical + * definition, and the source of truth for anything provisioned today. + * 2. The live production table, which carries ELEVEN columns setup.sql does + * not define (children, claude_code_session_id, compacted*, insights, + * metrics, task_observations, blocked_by, handover_linear_slug, + * pre_compaction_summary) and lacks `updated_at`, which setup.sql has. + * + * Filtering to setup.sql ALONE would have silently dropped real production data + * on every close — the same disease this filter cures, pointed the other way. + * So the union is deliberate, and the drift between the two is itself a known + * defect: schema/setup.sql is stale and a fresh install does not reproduce + * production. That is tracked separately; do not "clean up" this list by + * trimming it to setup.sql until those two agree. + * + * When adding a column: add it to schema/setup.sql AND here, in the same + * change. A column missing here is silently dropped on write. + */ +export const SESSION_COLUMNS: ReadonlySet = new Set([ + // --- defined in schema/setup.sql --- + "id", + "session_title", + "session_date", + "agent", + "project", + "linear_issue", + "recording_path", + "transcript_path", + "decisions", + "open_threads", + "closing_reflection", + "close_compliance", + "rapport_summary", + "embedding", + "created_at", + "updated_at", + + // --- present in production, absent from setup.sql (see note above) --- + "blocked_by", + "children", + "claude_code_session_id", + "compacted", + "compacted_at", + "compacted_summary", + "handover_linear_slug", + "insights", + "metrics", + "pre_compaction_summary", + "task_observations", +]); + +/** + * Drop keys the sessions table does not have, so a local-only field cannot + * fail an otherwise valid close. + * + * Returns a new object; the input is not mutated. Undefined values are kept — + * an explicit undefined is a caller's business, and only *unknown keys* are the + * hazard being removed here. + */ +export function filterToSessionColumns( + data: Record +): Record { + const filtered: Record = {}; + for (const [key, value] of Object.entries(data)) { + if (SESSION_COLUMNS.has(key)) { + filtered[key] = value; + } + } + return filtered; +} diff --git a/src/tools/session-close.ts b/src/tools/session-close.ts index fb2ab92..4f988d2 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -13,6 +13,7 @@ import * as supabase from "../services/supabase-client.js"; import { embed, isEmbeddingAvailable } from "../services/embedding.js"; import { hasSupabase, hasProInsights, getTableName } from "../services/tier.js"; import { getStorage } from "../services/storage.js"; +import { filterToSessionColumns } from "../services/session-columns.js"; import { clearCurrentSession, resolveCurrentSession, getSurfacedScars, getConfirmations, getReflections, getObservations, getChildren, getThreads, getSessionActivity, isRecallCalled } from "../services/session-state.js"; import { normalizeThreads, mergeThreadStates, migrateStringThread, saveThreadsFile } from "../services/thread-manager.js"; // import { deduplicateThreadList } from "../services/thread-dedup.js"; @@ -1392,8 +1393,14 @@ export async function sessionClose( try { // Upsert session WITHOUT embedding (fast path) // Embedding + thread detection run fire-and-forget after + // + // GIT-74/R17: filter to the table's known columns. sessionData is built by + // spreading the existing record, which on the Supabase-miss path is the + // LOCAL file record — a different shape, carrying rendering fields like + // `display`. One unknown key fails the whole upsert (PGRST204) and the + // close reports FAILED, which is honest but fatal on a fresh install. await Promise.all([ - supabase.directUpsert(getTableName("sessions"), sessionData), + supabase.directUpsert(getTableName("sessions"), filterToSessionColumns(sessionData)), blindspotPromise, ]); diff --git a/tests/unit/services/session-columns.test.ts b/tests/unit/services/session-columns.test.ts new file mode 100644 index 0000000..081667a --- /dev/null +++ b/tests/unit/services/session-columns.test.ts @@ -0,0 +1,129 @@ +/** + * Session column filtering (GIT-74/R17) + * + * The defect: session_close builds its upsert by spreading the existing session + * record, and on the Supabase-miss path that record is the LOCAL file record — + * a different shape carrying rendering fields like `display`. Postgres rejects + * the entire upsert on the first unknown key (PGRST204), so one local-only + * field failed the whole close. A fresh Pro install could not close its first + * session. + * + * These tests guard the CLASS, not the `display` instance: any unknown key is + * dropped, and every known column survives. The second half matters as much as + * the first — a filter that drops real columns is the same bug pointed the + * other way, and filtering to schema/setup.sql alone would have done exactly + * that to eleven live production columns. + */ + +import { describe, it, expect } from "vitest"; +import { + SESSION_COLUMNS, + filterToSessionColumns, +} from "../../../src/services/session-columns.js"; + +describe("filterToSessionColumns: drops what the table does not have", () => { + it("drops `display` — the field that failed the close", () => { + const out = filterToSessionColumns({ + id: "s-1", + display: "((●)) gitmem ── close · FAILED", + }); + + expect(out).not.toHaveProperty("display"); + expect(out.id).toBe("s-1"); + }); + + it("drops any unknown key, not just the one that was reported", () => { + const out = filterToSessionColumns({ + id: "s-1", + display: "chrome", + performance: { latency_ms: 12 }, + some_future_local_field: true, + _internal: "x", + }); + + expect(Object.keys(out)).toEqual(["id"]); + }); + + it("returns a new object and does not mutate the input", () => { + const input = { id: "s-1", display: "chrome" }; + const out = filterToSessionColumns(input); + + expect(input).toHaveProperty("display"); + expect(out).not.toBe(input); + }); + + it("survives an empty payload", () => { + expect(filterToSessionColumns({})).toEqual({}); + }); +}); + +describe("filterToSessionColumns: keeps every column the table does have", () => { + // The inverse guard. A filter that silently drops real columns would be the + // same defect class as the one it fixes — data loss instead of a hard error, + // which is strictly worse because nothing reports it. + it("passes through every column in SESSION_COLUMNS", () => { + const payload: Record = {}; + for (const col of SESSION_COLUMNS) payload[col] = `value-of-${col}`; + + const out = filterToSessionColumns(payload); + + expect(Object.keys(out).sort()).toEqual([...SESSION_COLUMNS].sort()); + }); + + it("keeps the production-only columns that schema/setup.sql omits", () => { + // These eleven exist in the live table but not in setup.sql. Filtering to + // setup.sql alone would have dropped real data on every production close. + const productionOnly = [ + "blocked_by", + "children", + "claude_code_session_id", + "compacted", + "compacted_at", + "compacted_summary", + "handover_linear_slug", + "insights", + "metrics", + "pre_compaction_summary", + "task_observations", + ]; + + const payload = Object.fromEntries(productionOnly.map((c) => [c, "kept"])); + const out = filterToSessionColumns(payload); + + for (const col of productionOnly) { + expect(out, `${col} is a live production column and must survive`).toHaveProperty(col); + } + }); + + it("keeps a realistic close payload intact", () => { + const closePayload = { + id: "3f2a1b4c-0000-4000-8000-000000000000", + agent: "CLI", + project: "gitmem", + session_title: "Release session", + close_compliance: { valid: true }, + closing_reflection: { what_broke: "nothing" }, + decisions: ["ship it"], + open_threads: [{ id: "t-1" }], + rapport_summary: "good", + claude_code_session_id: "abc", + // the intruder + display: "((●)) gitmem ── close", + }; + + const out = filterToSessionColumns(closePayload); + + expect(out).not.toHaveProperty("display"); + expect(Object.keys(out).length).toBe(Object.keys(closePayload).length - 1); + expect(out.close_compliance).toEqual({ valid: true }); + expect(out.decisions).toEqual(["ship it"]); + }); + + it("preserves an explicitly-undefined known column", () => { + // Only unknown KEYS are the hazard. An explicit undefined on a real column + // is the caller's business and must not be quietly reinterpreted. + const out = filterToSessionColumns({ id: "s-1", linear_issue: undefined }); + + expect("linear_issue" in out).toBe(true); + }); +});