From 9e837ada63ed4d6d7826a7b3af6d29cf95e4a09b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 11:57:40 -0400 Subject: [PATCH 01/10] =?UTF-8?q?docs:=20add=20positioning=20memo=20v2=20?= =?UTF-8?q?=E2=80=94=20wedge/moat=20(strategy)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 --- ...ng-memo-v2-memory-wedge-governance-moat.md | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 docs/strategy/positioning-memo-v2-memory-wedge-governance-moat.md diff --git a/docs/strategy/positioning-memo-v2-memory-wedge-governance-moat.md b/docs/strategy/positioning-memo-v2-memory-wedge-governance-moat.md new file mode 100644 index 0000000..2ff6260 --- /dev/null +++ b/docs/strategy/positioning-memo-v2-memory-wedge-governance-moat.md @@ -0,0 +1,82 @@ +# Positioning Memo v2 — Memory Is the Wedge, Governance Is the Moat + +**Status:** Draft v1 · 2026-08-02 · Author: Brain (strategist), at the founder's direction +**Supersedes in part:** the implicit positioning of the Jan-2026 seed deck and `GitMem_vs_Mem0_Positioning.docx` (Jan 23, 2026) — both remain correct about the market; this memo revises what the *product* is. +**Provenance:** grounded in seven months of production use at the Weekend Warrior lab (the reference deployment), the Agent Working Protocol v1.0→v1.2 (authored 2026-08-02, `weekend-warrior-dashboard/docs/architecture/awp-core.md`), and two independent external reviews of that protocol received the same day. n=1 caveats are stated where they apply — this memo practices the honest-labeling it recommends. + +--- + +## 1. The one-sentence revision + +Today GitMem sells **memory** ("we compete with forgetting"). It should sell **memory that enforces** ("we compete with *repeating*"). Mem0 remembers preferences; GitMem remembers — and *governs*. + +## 2. Why: the 65% problem + +GitMem's founding philosophy document (Jan 3, 2026) already contained the confession: *"write-only is not enough... enforcement happens through triggers."* Seven months of production use sharpened it into a measurable claim: **a recalled scar is a suggestion, and suggestions get ignored.** An external reviewer of the Agent Working Protocol reported (their telemetry, unverified in our record — labeled per our own R-16): instructional rules failed in ~65% of their transcripts. Our own scar ledger agrees directionally: every scar in it is a rule that was surfaced-and-ignorable until someone built a gate. + +This is not a criticism of GitMem — it is GitMem's **roadmap**. The value curve of institutional memory is an enforcement ladder: + +``` +I instructional — the scar is recalled; the agent is asked (today's product) +M mechanical — a gate detects violation and blocks/flags (the upgrade) +S structural — the violation has no code path (the moat) +``` + +Retrieval quality (Mem0's benchmark game — latency, token reduction) optimizes tier I. Nobody on the competitive slide climbs the ladder. The company that turns remembered failures into *enforced* protections owns a different category than the one that recalls them fastest. + +## 3. The feature map (from AWP, production-derived) + +Every item below was extracted from a rule exercised in production at the reference deployment, then hardened by two external reviews. Effort tiers are rough. + +| Feature | AWP source | What it is | Effort | +|---|---|---|---| +| **Scar→Gate compiler** | R-03/R-07 + tier ladder | One command promotes a Hardened scar into an enforced hook / CI check / pre-commit gate. GitMem already ships hooks — this is the flagship motion, and no competitor does it. | M | +| **Scar lifecycle** | R-22 | Active → Hardened (fired productively ≥N) → Retired. Retirement is a governed act with evidence, not housekeeping — an agent may never retire a scar that constrains it. Builds on existing decay (process/incident/context). | S–M | +| **Supersession primitive** | R-05 | `superseded_by` as a first-class field; search/recall refuses to serve a superseded entry without its head. Kills the stale-pointer failure class. | S | +| **Capability declaration** | R-17 | Sessions open with machine-readable model/context/tooling declaration; handoffs become capability-aware; near-limit agents stop at clean commits. | S | +| **Compliance artifacts** | INV-4 / R-23 | "The agent followed the rules" is only claimable with an artifact the claimant did not author (cold-start diff, planted-defect run, audit report). This is the enterprise AI-governance checkbox, generated automatically. | M | +| **Self-audit cadence** | R-20 | Every N sessions, a cold agent audits the memory/rules corpus for self-violations; findings become scars. Recurring, schedulable, demo-able. | M | +| **Cold-start harness** | R-13/R-14 | Dispatch a fresh agent against a handoff; diff its first actions vs. intent; divergences are handoff defects. Tests the memory's *sufficiency*, not just its recall. | M | +| **Attention/compute ledger** | R-15/R-19 | Per-session log of human words, escalations, token/wall-clock per decision class. Measure first; price only after a baseline (no fake unit costs). Feeds the "30-60-90: Attention + Trust" project directly. | S | + +*(Effort: S = small/one sprint-equivalent · M = medium/multi-sprint.)* + +## 4. Marketplace upgrade: memory packs → protocol packs + +The pitch's marketplace flywheel gets stronger when a pack is not a pile of scars but a **protocol pack**: scars + their compiled gates + adoption observables ("you have this when…"). Buyers install enforcement, not reading material. **AWP itself is SKU #1** — the single-file distribution edition exists today, with a lineage table, a 13-row scar table, and two external reviews as its independent compliance artifacts. The npm/PyPI analogy in the Jan positioning doc becomes exact: packages ship *executable* protection, not documentation. + +## 5. GTM: the book is the content engine, the lab is the proof + +The pitch promises content-led growth. The content now exists as a governed pipeline: **The Liberation Blueprint** (book outline v1.0, 15 chapters, one drafted) is precisely the "practitioners, not theorists" narrative the deck promises — and the Weekend Warrior lab is the flagship case study with auditable numbers: seventeen ratifications on ~two hundred founder words in one night; five pre-contamination catches in a single research batch; a full protocol version (v1.0→v1.2) evolved in one day under two independent reviews; every claim citable to an immutable record. "Built in production, not theory" stops being a slide and becomes a bibliography. + +Separately: the two protocol reviewers evidently operate agent fleets with their own telemetry. They are not reviewers; they are **design partners** and plausibly the first protocol-pack customers. Worth pursuing by name. + +## 6. Pricing logic: the adoption ladder is the freemium ladder + +AWP §V (non-goals) is the guard: full governance is **pure tax** on casual work, and the product must never impose it by default. The graduated shape: + +- **Free:** memory (recall/learn/close) — the wedge, zero-config, `npx init`, unchanged. +- **Pro ($19):** the enforcement ladder — scar lifecycle, scar→gate compiler, supersession, capability declarations, self-audit cadence. (This gives Pro the identity it currently lacks: today Pro is "more memory"; tomorrow Pro is "memory that acts.") +- **Marketplace:** protocol packs at 15%, AWP as the reference SKU. + +AWP §IV's adoption observables are literally the onboarding checklist: each step has a "you have this when…" test, which converts activation from a metric into a product feature. + +## 7. Honest labeling (the section an associate will try to puncture first) + +- **n = 1.** The protocol is proven at one shop, one founder, one domain, one season. The pitch claim is "proven wedge + production-derived governance hypothesis," not "proven governance." The Maturity Declaration discipline applies to this memo and to the deck. +- **The 65% figure is external and unverified here.** Use it as a reviewer's observation with our directional agreement, never as our benchmark. (Building our own instructional-vs-mechanical compliance telemetry is itself a small, high-value feature — R-19's ledger yields it.) +- **Scope-creep is the real product risk.** If governance leaks into the free tier's first-run experience, the wedge dies. The §V non-goals boundary ships *in the product*, not just in this memo. +- **Linear/record hygiene:** the GitMem Linear workspace's newest activity predates this memo by ~5 months while the repo shipped continuously — the project currently violates its own record-primacy thesis. First workstream of the next session. + +## 8. Recommended next actions (for the GitMem-side session to convert into issues) + +1. Reconcile Linear to reality: audit the 11 GitMem projects against the repo's actual state; close/merge the dead, restate the live. +2. Adopt AWP internally for GitMem development itself (dogfooding the governance tier before selling it) — version-pinned per R-21. +3. Ship the two S-tier features first (supersession primitive, capability declaration) — smallest surface, largest credibility. +4. Prototype the scar→gate compiler against GitMem's existing hooks as the Pro-tier flagship. +5. Revise the deck's category line: from "we compete with forgetting" to "we compete with repeating," with the enforcement ladder as the category-creation slide. +6. Contact the two protocol reviewers as design partners. + +--- + +*This memo is uncommitted at authorship; it enters the record via the GitMem repo's own conventions. If a claim here contradicts a scar earned later, the scar wins — write the amendment.* From 753e9c3d44f10a55959102c590f8daf7da9d25d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 22:30:37 -0400 Subject: [PATCH 02/10] feat: single thread-scope resolver (GIT-69 item 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One definition of "the thread store, scoped", per ruling R1/R6. The four thread surfaces each computed their own view and disagreed in three ways that were invisible from any single call site: limit session_start 50 vs list_threads 100 status session_start excluded "dormant", list_threads did not ordering the dedup loader had no ORDER BY at all Those disagreements are the f02f74ca panel-vs-list defect and the nondeterministic dedup candidate set. Both are closed here rather than in three separate patches. resolveThreadScope() (project, sessionId) from overrides > session > config buildScopedThreadQuery() the one query shape: filters, order, limit isInScope() same predicate for rows not from a scoped query THREAD_SCOPE_ORDER total order — thread_id breaks ties, so the candidate set is stable at the limit boundary Dedup uses "own_session" visibility per R1: a match against another session's thread is never a safe automatic merge. With no active session the query is null, not project-wide — a sessionless caller must not silently dedup against the whole namespace. The resolver is downstream of session identity: it scopes correctly relative to whatever (project, sessionId) it is handed. Deciding who the caller is belongs to GIT-51. The project-disagreement fixture pins that seam by name so the boundary is explicit rather than discovered. Tests: +27 (parity, cross-project isolation, determinism, own-session candidate set, status sets, recovery-seam fixture). Suite 1031 green, typecheck clean. Contains zero lines from wip/git-51-session-recovery. Consumer wiring (session_start / list_threads / resolve_thread / dedup) follows in the next commit on this branch. Refs GIT-69, GIT-63, GIT-67 Co-Authored-By: Claude Opus 5 --- src/services/thread-scope.ts | 187 +++++++++++++++ tests/unit/services/thread-scope.test.ts | 291 +++++++++++++++++++++++ 2 files changed, 478 insertions(+) create mode 100644 src/services/thread-scope.ts create mode 100644 tests/unit/services/thread-scope.test.ts diff --git a/src/services/thread-scope.ts b/src/services/thread-scope.ts new file mode 100644 index 0000000..9ef34de --- /dev/null +++ b/src/services/thread-scope.ts @@ -0,0 +1,187 @@ +/** + * Thread Scope Resolver (GIT-69) + * + * ONE definition of "the thread store, scoped". Every surface that reads or + * resolves threads imports from here: session_start's panel, list_threads, + * resolve_thread, and create_thread's dedup candidate selection. + * + * Before this module each of those four computed its own view, and they + * disagreed in three ways that were invisible from any single call site: + * + * - limit session_start 50 vs list_threads 100 + * - status set session_start excluded "dormant", list_threads did not + * - ordering the dedup loader had no ORDER BY at all + * + * Those disagreements are the f02f74ca panel-vs-list defect and the + * nondeterministic dedup candidate set. Both die here rather than in three + * separate patches. + * + * Scope has two axes: + * project — which namespace's threads are in view + * visibility — "project" (everything in the namespace) or "own_session" + * (only threads this session created) + * + * Per ruling R1, dedup uses "own_session": automatic dedup against another + * session's thread is never safe, because a match there means silently binding + * this session's content to a thread it does not own. Widening to a + * parent/descendant lineage arrives here later (GIT-69 item 4) and every + * consumer inherits it by import. + * + * This resolver is downstream of session identity: it consumes whatever + * (project, sessionId) it is given and scopes correctly relative to them. + * Deciding *who the caller is* belongs to session recovery (GIT-51), not here. + */ + +import { getCurrentSession, getProject } from "./session-state.js"; +import type { Project } from "../types/index.js"; + +// --- Types --- + +/** Which threads a caller may see. */ +export type ThreadVisibility = "project" | "own_session"; + +export interface ThreadScope { + /** Namespace the caller is operating in. */ + project: Project; + /** Session the caller belongs to, or null when there is no active session. */ + sessionId: string | null; +} + +export interface ThreadQuery { + filters: Record; + order: string; + limit: number; +} + +export interface ThreadQueryOptions { + visibility: ThreadVisibility; + /** Terminal statuses to exclude. Defaults to the active-thread set. */ + exclude?: readonly string[]; + limit?: number; +} + +// --- Constants --- + +/** + * Deterministic total order for every scoped thread query. + * + * vitality and recency rank the rows; thread_id breaks ties. The tiebreaker is + * not cosmetic — vitality_score is 1 for most rows, so without it the ordering + * is unstable exactly at the limit boundary, and two identical calls can return + * different candidate sets. Determinism is an acceptance criterion (R6). + */ +export const THREAD_SCOPE_ORDER = + "vitality_score.desc,last_touched_at.desc,thread_id.asc"; + +/** Row cap for scoped queries. One number, so no two surfaces can disagree. */ +export const THREAD_SCOPE_LIMIT = 100; + +/** Statuses that mean "this thread is no longer live work". */ +export const TERMINAL_STATUSES = ["resolved", "archived"] as const; + +/** Statuses excluded from the active-thread view. */ +export const INACTIVE_STATUSES = ["resolved", "archived", "dormant"] as const; + +// --- Core --- + +/** + * Resolve the caller's thread scope. + * + * Explicit overrides win, then the active session, then the configured + * project. sessionId is null when there is no active session — callers decide + * what that means for them; "own_session" visibility treats it as "no thread + * is in scope" rather than silently widening to the whole project. + */ +export function resolveThreadScope(overrides?: { + project?: Project; + sessionId?: string | null; +}): ThreadScope { + const session = getCurrentSession(); + + const project = + overrides?.project ?? getProject() ?? ("default" as Project); + + const sessionId = + overrides?.sessionId !== undefined + ? overrides.sessionId + : session?.sessionId ?? null; + + return { project, sessionId }; +} + +/** + * Build the query for a scope. Every scoped read goes through this, so the + * limit, ordering, and status set cannot drift between surfaces again. + * + * Returns null when the scope can select nothing — "own_session" visibility + * with no active session. That is a real empty set, not an error, and it is + * deliberately distinct from an unscoped query: callers must not fall back to + * project-wide results when session identity is missing. + */ +export function buildScopedThreadQuery( + scope: ThreadScope, + options: ThreadQueryOptions +): ThreadQuery | null { + if (options.visibility === "own_session" && !scope.sessionId) { + return null; + } + + const exclude = options.exclude ?? INACTIVE_STATUSES; + + const filters: Record = { + project: scope.project, + }; + + if (exclude.length > 0) { + filters.status = `not.in.(${exclude.join(",")})`; + } + + if (options.visibility === "own_session") { + filters.source_session = scope.sessionId as string; + } + + return { + filters, + order: THREAD_SCOPE_ORDER, + limit: options.limit ?? THREAD_SCOPE_LIMIT, + }; +} + +/** + * Whether a thread is inside a scope, for in-memory filtering of rows that did + * not come from a scoped query (local file cache, session aggregation). + * + * Mirrors buildScopedThreadQuery exactly. If the two ever disagree, the store + * and the cache disagree — which is the divergence class this module exists to + * close — so they are tested against the same fixtures. + */ +export function isInScope( + thread: { project?: string | null; source_session?: string | null }, + scope: ThreadScope, + visibility: ThreadVisibility +): boolean { + if (thread.project != null && thread.project !== scope.project) { + return false; + } + + if (visibility === "own_session") { + if (!scope.sessionId) return false; + if (thread.source_session !== scope.sessionId) return false; + } + + return true; +} + +/** + * Human-readable scope description, for tool responses that need to say what + * they searched. Naming the scope in-band is how a caller can tell "no results" + * from "wrong scope" without reading the source. + */ +export function describeScope( + scope: ThreadScope, + visibility: ThreadVisibility +): string { + return visibility === "own_session" + ? `project ${scope.project}, session ${scope.sessionId?.slice(0, 8) ?? "none"}` + : `project ${scope.project}, all sessions`; +} diff --git a/tests/unit/services/thread-scope.test.ts b/tests/unit/services/thread-scope.test.ts new file mode 100644 index 0000000..a82666d --- /dev/null +++ b/tests/unit/services/thread-scope.test.ts @@ -0,0 +1,291 @@ +/** + * Unit tests for the Thread Scope Resolver (GIT-69) + * + * Covers the three acceptance criteria from ruling R6: + * - parity query builder and in-memory filter agree on one fixture set + * - isolation a project's threads never appear in another project's scope + * - determinism ordering is a total order, stable across identical calls + * + * Plus the project-disagreement fixture, which documents the recovery→scoping + * seam by name so the boundary is explicit rather than discovered. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockGetCurrentSession = vi.fn(); +const mockGetProject = vi.fn(); + +vi.mock("../../../src/services/session-state.js", () => ({ + getCurrentSession: () => mockGetCurrentSession(), + getProject: () => mockGetProject(), +})); + +const { + resolveThreadScope, + buildScopedThreadQuery, + isInScope, + describeScope, + THREAD_SCOPE_ORDER, + THREAD_SCOPE_LIMIT, + INACTIVE_STATUSES, + TERMINAL_STATUSES, +} = await import("../../../src/services/thread-scope.js"); + +type Scope = ReturnType; + +const SESSION_A = "84765490-da8b-45d3-9f59-cc30bb99cbde"; +const SESSION_B = "096e940d-af35-4bac-bc30-a73c9d0de79e"; + +beforeEach(() => { + mockGetCurrentSession.mockReset(); + mockGetProject.mockReset(); + mockGetCurrentSession.mockReturnValue(null); + mockGetProject.mockReturnValue(undefined); +}); + +// ---------- Scope resolution ---------- + +describe("resolveThreadScope()", () => { + it("takes project and session from the active session", () => { + mockGetCurrentSession.mockReturnValue({ sessionId: SESSION_A }); + mockGetProject.mockReturnValue("gitmem"); + + expect(resolveThreadScope()).toEqual({ + project: "gitmem", + sessionId: SESSION_A, + }); + }); + + it("returns a null sessionId when no session is active", () => { + mockGetProject.mockReturnValue("gitmem"); + + expect(resolveThreadScope().sessionId).toBeNull(); + }); + + it("falls back to the default project when none is configured", () => { + expect(resolveThreadScope().project).toBe("default"); + }); + + it("lets explicit overrides win over session state", () => { + mockGetCurrentSession.mockReturnValue({ sessionId: SESSION_A }); + mockGetProject.mockReturnValue("gitmem"); + + expect( + resolveThreadScope({ project: "weekend_warrior", sessionId: SESSION_B }) + ).toEqual({ project: "weekend_warrior", sessionId: SESSION_B }); + }); + + it("distinguishes an explicit null sessionId from an omitted one", () => { + mockGetCurrentSession.mockReturnValue({ sessionId: SESSION_A }); + + // Omitted → inherit the session. + expect(resolveThreadScope({}).sessionId).toBe(SESSION_A); + // Explicit null → caller is deliberately sessionless. + expect(resolveThreadScope({ sessionId: null }).sessionId).toBeNull(); + }); +}); + +// ---------- Determinism ---------- + +describe("ordering is deterministic", () => { + it("breaks ties on thread_id so the order is total", () => { + // vitality_score is 1 for most rows, so without a tiebreaker the ordering + // is unstable exactly at the limit boundary and two identical calls can + // return different candidate sets. + expect(THREAD_SCOPE_ORDER).toContain("thread_id"); + }); + + it("applies the same order and limit to every visibility", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + + const projectWide = buildScopedThreadQuery(scope, { visibility: "project" }); + const ownSession = buildScopedThreadQuery(scope, { visibility: "own_session" }); + + expect(projectWide?.order).toBe(THREAD_SCOPE_ORDER); + expect(ownSession?.order).toBe(THREAD_SCOPE_ORDER); + expect(projectWide?.limit).toBe(THREAD_SCOPE_LIMIT); + expect(ownSession?.limit).toBe(THREAD_SCOPE_LIMIT); + }); + + it("returns an identical query for identical inputs", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + const first = buildScopedThreadQuery(scope, { visibility: "project" }); + const second = buildScopedThreadQuery(scope, { visibility: "project" }); + + expect(first).toEqual(second); + }); +}); + +// ---------- Cross-project isolation ---------- + +describe("cross-project isolation", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + + it("always constrains the query to one project", () => { + for (const visibility of ["project", "own_session"] as const) { + const query = buildScopedThreadQuery(scope, { visibility }); + expect(query?.filters.project).toBe("gitmem"); + } + }); + + it("excludes a thread belonging to another project", () => { + const foreign = { project: "weekend_warrior", source_session: SESSION_A }; + + expect(isInScope(foreign, scope, "project")).toBe(false); + expect(isInScope(foreign, scope, "own_session")).toBe(false); + }); + + it("admits a same-project thread from another session at project visibility", () => { + const sibling = { project: "gitmem", source_session: SESSION_B }; + + expect(isInScope(sibling, scope, "project")).toBe(true); + }); + + it("reproduces the f02f74ca leak as a guarded case", () => { + // A weekend_warrior thread reaching a gitmem session_start panel is the + // 2026-08-05 repro. It must be excluded on every axis. + const leaked = { project: "weekend_warrior", source_session: SESSION_B }; + + expect(isInScope(leaked, { project: "gitmem", sessionId: SESSION_A }, "project")).toBe(false); + }); +}); + +// ---------- own_session visibility ---------- + +describe("own_session visibility (dedup candidate set, R1)", () => { + it("restricts candidates to the caller's own session", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + const query = buildScopedThreadQuery(scope, { visibility: "own_session" }); + + expect(query?.filters.source_session).toBe(SESSION_A); + }); + + it("excludes another session's thread from the candidate set", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + const otherSession = { project: "gitmem", source_session: SESSION_B }; + + expect(isInScope(otherSession, scope, "own_session")).toBe(false); + }); + + it("selects nothing rather than widening when there is no session", () => { + const scope: Scope = { project: "gitmem", sessionId: null }; + + // null, not a project-wide query. A sessionless caller must not silently + // dedup against every thread in the namespace. + expect(buildScopedThreadQuery(scope, { visibility: "own_session" })).toBeNull(); + }); + + it("still permits a project-wide read when there is no session", () => { + const scope: Scope = { project: "gitmem", sessionId: null }; + const query = buildScopedThreadQuery(scope, { visibility: "project" }); + + expect(query).not.toBeNull(); + expect(query?.filters.source_session).toBeUndefined(); + }); +}); + +// ---------- Status sets ---------- + +describe("status filtering is single-sourced", () => { + it("excludes the inactive set by default", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + const query = buildScopedThreadQuery(scope, { visibility: "project" }); + + for (const status of INACTIVE_STATUSES) { + expect(query?.filters.status).toContain(status); + } + }); + + it("honours a narrower terminal-only exclusion", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + const query = buildScopedThreadQuery(scope, { + visibility: "project", + exclude: TERMINAL_STATUSES, + }); + + expect(query?.filters.status).not.toContain("dormant"); + }); + + it("omits the status filter when nothing is excluded", () => { + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + const query = buildScopedThreadQuery(scope, { + visibility: "project", + exclude: [], + }); + + expect(query?.filters.status).toBeUndefined(); + }); +}); + +// ---------- Parity ---------- + +describe("parity between the query builder and the in-memory filter", () => { + // The store path and the cache path must agree, or they reproduce the + // divergence this module exists to close. + const scope: Scope = { project: "gitmem", sessionId: SESSION_A }; + + const fixtures = [ + { name: "own session, same project", row: { project: "gitmem", source_session: SESSION_A }, project: true, own: true }, + { name: "other session, same project", row: { project: "gitmem", source_session: SESSION_B }, project: true, own: false }, + { name: "own session, other project", row: { project: "weekend_warrior", source_session: SESSION_A }, project: false, own: false }, + { name: "other session, other project", row: { project: "weekend_warrior", source_session: SESSION_B }, project: false, own: false }, + ]; + + for (const f of fixtures) { + it(`agrees on: ${f.name}`, () => { + expect(isInScope(f.row, scope, "project")).toBe(f.project); + expect(isInScope(f.row, scope, "own_session")).toBe(f.own); + }); + } + + it("treats a project-less local row as in scope", () => { + // Local cache rows predate project tagging; excluding them would hide a + // user's own threads behind a schema detail. + expect(isInScope({ source_session: SESSION_A }, scope, "project")).toBe(true); + }); +}); + +// ---------- Recovery seam (R6 #1) ---------- + +describe("project-disagreement fixture — the recovery→scoping seam", () => { + // GIT-51's resolveCurrentSession() builds a recovered context as + // project: data.project || entry.project + // where data is sessions//session.json and entry is the + // active-sessions.json registry row. When those two disagree, session.json + // silently wins, and the resolver is handed a project the registry never + // agreed to. + // + // The resolver's contract is deliberately narrow: it scopes correctly + // relative to whatever identity it is given. Detecting the disagreement is + // GIT-51's job at its rebase phase (explicit precedence + recovery_conflict), + // not this module's. These tests pin that boundary so a later reader does not + // mistake the silence for an oversight. + + it("scopes to exactly the project it was handed", () => { + mockGetCurrentSession.mockReturnValue({ sessionId: SESSION_A }); + mockGetProject.mockReturnValue("gitmem"); // registry view + + // Recovery resolved a different project from session.json and passes it in. + const scope = resolveThreadScope({ project: "weekend_warrior" }); + const query = buildScopedThreadQuery(scope, { visibility: "project" }); + + expect(query?.filters.project).toBe("weekend_warrior"); + }); + + it("does not silently merge the two candidate projects", () => { + const recovered: Scope = { project: "weekend_warrior", sessionId: SESSION_A }; + + // The registry's project must not leak into a scope built from the + // recovered one — a union here would be the cross-project leak arriving + // through a new door. + expect(isInScope({ project: "gitmem", source_session: SESSION_A }, recovered, "project")).toBe(false); + }); + + it("names its scope so a caller can tell empty from mis-scoped", () => { + const scope: Scope = { project: "weekend_warrior", sessionId: SESSION_A }; + + expect(describeScope(scope, "project")).toContain("weekend_warrior"); + expect(describeScope(scope, "own_session")).toContain(SESSION_A.slice(0, 8)); + expect(describeScope({ project: "gitmem", sessionId: null }, "own_session")).toContain("none"); + }); +}); From 00fa2d293a81d66c0d4c24c74a0001162efdb061 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 22:40:56 -0400 Subject: [PATCH 03/10] feat: wire all four thread surfaces to the scope resolver (GIT-69) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every surface that reads or resolves threads now imports one definition of "the thread store, scoped". The three disagreements are gone: session_start limit 50, excluded dormant, own query → resolver list_threads limit 100, included dormant, own query → resolver resolve_thread its own project lookup → resolver dedup no ORDER BY, project-wide candidates → resolver, own_session Per R1, dedup candidates are the caller's own session only; with no active session the query is null and dedup is skipped rather than widening to the namespace. The local-file fallback is held to the same scope, so cache and store cannot disagree. session_close's suggestion pass opts into "project" visibility explicitly — it proposes threads to a human rather than silently merging into one. That is a parameter, not a hardcoded default, so the distinction stays visible. Per R7, dormant is IN scope and filtered at display only, and the panel cap is annotation rather than scope: computePanelOmission/formatOmissionLine render "showing 5 of 47 in scope (31 dormant, 11 over cap) — list_threads for full view" whenever display < scope. A panel showing 5 of 47 with no indicator is a green-message pattern in miniature. Tests: +11 display-layer (7 reconciliation cases asserting shown + dormantHidden + overCap === totalInScope, plus disclosure and zero-term cases). Suite 1042 green, typecheck and build clean. Contains zero lines from wip/git-51-session-recovery. Refs GIT-69, GIT-63, GIT-67 Co-Authored-By: Claude Opus 5 --- src/services/thread-scope.ts | 69 ++++++++++++++ src/services/thread-supabase.ts | 116 +++++++++++++++++------ src/tools/create-thread.ts | 18 +++- src/tools/list-threads.ts | 6 +- src/tools/resolve-thread.ts | 7 +- src/tools/session-close.ts | 9 +- src/tools/session-start.ts | 28 ++++-- tests/unit/services/thread-scope.test.ts | 73 ++++++++++++++ 8 files changed, 280 insertions(+), 46 deletions(-) diff --git a/src/services/thread-scope.ts b/src/services/thread-scope.ts index 9ef34de..fa98413 100644 --- a/src/services/thread-scope.ts +++ b/src/services/thread-scope.ts @@ -79,6 +79,13 @@ export const THREAD_SCOPE_LIMIT = 100; /** Statuses that mean "this thread is no longer live work". */ export const TERMINAL_STATUSES = ["resolved", "archived"] as const; +/** + * In scope but excluded from a default listing (R7). Dormant threads exist — + * hiding them from scope would make the panel and the dedup candidate set + * disagree about what exists. They are filtered at display and counted. + */ +export const DORMANT_STATUSES: readonly string[] = ["dormant"]; + /** Statuses excluded from the active-thread view. */ export const INACTIVE_STATUSES = ["resolved", "archived", "dormant"] as const; @@ -172,6 +179,68 @@ export function isInScope( return true; } +// --- Honest truncation (R7) --- + +/** + * What the scope actually contained. The three counts must reconcile: + * listable + dormantHidden === totalInScope. + */ +export interface ThreadScopeCounts { + /** In scope and eligible for the default listing. */ + listable: number; + /** In scope, withheld from the default listing because dormant. */ + dormantHidden: number; + /** Everything the scope selected. */ + totalInScope: number; +} + +export interface PanelOmission { + shown: number; + dormantHidden: number; + overCap: number; + totalInScope: number; +} + +/** + * Split a scope into what a capped panel shows and what it withholds. + * + * By construction shown + dormantHidden + overCap === totalInScope. That + * identity is the point: a panel showing 5 of 47 with no indicator is a + * green-message pattern in miniature — a display asserting a completeness it + * does not have. + */ +export function computePanelOmission( + counts: ThreadScopeCounts, + maxShow: number +): PanelOmission { + const shown = Math.max(0, Math.min(counts.listable, maxShow)); + return { + shown, + dormantHidden: counts.dormantHidden, + overCap: Math.max(0, counts.listable - shown), + totalInScope: counts.totalInScope, + }; +} + +/** + * Render the omission line, or null when the view is complete and there is + * nothing to disclose. Any capped or filtered view must state what it omitted + * and where the rest lives (R7). + */ +export function formatOmissionLine( + omission: PanelOmission, + fullViewHint = "list_threads for full view" +): string | null { + const withheld = omission.dormantHidden + omission.overCap; + if (withheld === 0) return null; + + const parts: string[] = []; + if (omission.dormantHidden > 0) parts.push(`${omission.dormantHidden} dormant`); + if (omission.overCap > 0) parts.push(`${omission.overCap} over cap`); + + return `showing ${omission.shown} of ${omission.totalInScope} in scope (${parts.join(", ")}) — ${fullViewHint}`; +} + /** * Human-readable scope description, for tool responses that need to say what * they searched. Naming the scope in-band is how a caller can tell "no results" diff --git a/src/services/thread-supabase.ts b/src/services/thread-supabase.ts index b830788..166d8a3 100644 --- a/src/services/thread-supabase.ts +++ b/src/services/thread-supabase.ts @@ -15,6 +15,12 @@ import { computeVitality, computeLifecycleStatus, detectThreadClass } from "./th import type { ThreadClass, LifecycleStatus } from "./thread-vitality.js"; import { normalizeText, deduplicateThreadList } from "./thread-dedup.js"; import type { ThreadWithEmbedding } from "./thread-dedup.js"; +import { + buildScopedThreadQuery, + TERMINAL_STATUSES, + DORMANT_STATUSES, +} from "./thread-scope.js"; +import type { ThreadScope, ThreadVisibility, ThreadScopeCounts } from "./thread-scope.js"; import type { ThreadObject, Project } from "../types/index.js"; // ---------- Supabase Row Types ---------- @@ -50,6 +56,14 @@ export interface ThreadDisplayInfo { days_since_touch: number; } +export type { ThreadScopeCounts } from "./thread-scope.js"; + +export interface ActiveThreadsResult { + open: ThreadObject[]; + displayInfo: ThreadDisplayInfo[]; + scope: ThreadScopeCounts; +} + // ---------- Mapping Helpers ---------- /** @@ -252,7 +266,7 @@ export async function getThreadFromSupabaseById( * Returns null if Supabase is unavailable (caller should fall back to local). */ export async function listThreadsFromSupabase( - project: Project = "default", + scope: ThreadScope, options: { statusFilter?: string; includeResolved?: boolean; @@ -262,32 +276,38 @@ export async function listThreadsFromSupabase( return null; } + // GIT-69: project, ordering, and limit come from the one scope resolver. + // Only the status axis varies here, because list_threads is the surface that + // can deliberately ask for resolved threads. + const query = buildScopedThreadQuery(scope, { + visibility: "project", + exclude: TERMINAL_STATUSES, + }); + if (!query) return null; + try { - const filters: Record = { - project, - }; + const filters = { ...query.filters }; // Apply status filter unless including all if (!options.includeResolved && options.statusFilter) { if (options.statusFilter === "open") { // "open" in local status = all non-terminal Supabase statuses - filters.status = "not.in.(resolved,archived)"; + filters.status = `not.in.(${TERMINAL_STATUSES.join(",")})`; } else if (options.statusFilter === "resolved") { filters.status = "resolved"; } else { // Pass through any Supabase-native status filters.status = options.statusFilter; } - } else if (!options.includeResolved) { - // Default: exclude resolved and archived - filters.status = "not.in.(resolved,archived)"; + } else if (options.includeResolved) { + delete filters.status; } const rows = await supabase.directQuery(getTableName("threads_lite"), { select: "*", filters, - order: "vitality_score.desc,last_touched_at.desc", - limit: 100, + order: query.order, + limit: query.limit, }); const threads = deduplicateThreadList(rows.map(rowToThreadObject)); @@ -305,27 +325,35 @@ export async function listThreadsFromSupabase( * Returns null if Supabase is unavailable. */ export async function loadActiveThreadsFromSupabase( - project: Project = "default" -): Promise<{ open: ThreadObject[]; displayInfo: ThreadDisplayInfo[] } | null> { + scope: ThreadScope +): Promise { if (!hasSupabase() || !supabase.isConfigured()) { return null; } + // GIT-69/R7: the panel is a VIEW of the scope, not a second definition of + // it. Dormant threads are IN scope — they exist, and hiding them here would + // make the panel and the dedup candidate set disagree about what exists, + // which is this ticket's original sin. They are filtered at display, and the + // count is reported so the panel can say what it omitted. + const query = buildScopedThreadQuery(scope, { + visibility: "project", + exclude: TERMINAL_STATUSES, + }); + if (!query) return null; + try { - // Get only non-resolved, non-archived threads (open/active only) const rows = await supabase.directQuery(getTableName("threads_lite"), { select: "*", - filters: { - project, - status: "not.in.(archived,dormant,resolved)", - }, - order: "vitality_score.desc,last_touched_at.desc", - limit: 50, + filters: query.filters, + order: query.order, + limit: query.limit, }); const now = new Date(); const open: ThreadObject[] = []; const displayInfo: ThreadDisplayInfo[] = []; + let dormantHidden = 0; // Deduplicate by text content (mirrors aggregateThreads logic) const seenText = new Set(); @@ -337,6 +365,13 @@ export async function loadActiveThreadsFromSupabase( seenIds.add(row.id); if (key) seenText.add(key); + // In scope, excluded from the default listing — counted, never dropped + // silently. + if (DORMANT_STATUSES.includes(row.status)) { + dormantHidden++; + continue; + } + open.push(rowToThreadObject(row)); // Phase 6: Compute lifecycle display info for open threads @@ -363,9 +398,17 @@ export async function loadActiveThreadsFromSupabase( }); } - const dupsRemoved = rows.length - open.length; - console.error(`[thread-supabase] Loaded ${open.length} open threads from Supabase (${dupsRemoved} duplicates removed)`); - return { open, displayInfo }; + const dupsRemoved = rows.length - open.length - dormantHidden; + console.error(`[thread-supabase] Loaded ${open.length} open threads from Supabase (${dormantHidden} dormant in scope, ${dupsRemoved} duplicates removed)`); + return { + open, + displayInfo, + scope: { + listable: open.length, + dormantHidden, + totalInScope: open.length + dormantHidden, + }, + }; } catch (error) { console.error("[thread-supabase] Failed to load active threads:", error instanceof Error ? error.message : error); return null; @@ -603,27 +646,40 @@ function parseEmbedding(raw: string | number[] | null | undefined): number[] | n * Returns null if Supabase is unavailable. */ export async function loadOpenThreadEmbeddings( - project: Project = "default" + scope: ThreadScope, + visibility: ThreadVisibility = "own_session" ): Promise { if (!hasSupabase() || !supabase.isConfigured()) { return null; } + // GIT-69/R1: dedup candidates default to the caller's OWN session. A match + // against another session's thread is never a safe automatic merge — it + // binds this session's content to a thread it does not own. With no active + // session the query is null and dedup is skipped entirely, rather than + // widening to every thread in the namespace. + // + // session_close's suggestion pass opts into "project" deliberately: it is + // proposing threads to a human, not silently merging into one. + const query = buildScopedThreadQuery(scope, { visibility }); + if (!query) { + console.error("[thread-supabase] No session in scope — skipping dedup candidates"); + return []; + } + try { const rows = await supabase.directQuery<{ thread_id: string; text: string; embedding: string | number[] | null; }>(getTableName("threads"), { - select: "thread_id,text,embedding", - filters: { - project, - status: "not.in.(resolved,archived)", - }, - limit: 100, + select: "thread_id,text,embedding,source_session", + filters: query.filters, + order: query.order, + limit: query.limit, }); - console.error(`[thread-supabase] Loaded ${rows.length} thread embeddings for dedup`); + console.error(`[thread-supabase] Loaded ${rows.length} thread embeddings for dedup (own session)`); return rows.map((row) => ({ thread_id: row.thread_id, text: row.text, diff --git a/src/tools/create-thread.ts b/src/tools/create-thread.ts index 5094fb6..a804f5d 100644 --- a/src/tools/create-thread.ts +++ b/src/tools/create-thread.ts @@ -28,6 +28,7 @@ import { touchThreadsInSupabase, } from "../services/thread-supabase.js"; import { checkDuplicate } from "../services/thread-dedup.js"; +import { resolveThreadScope } from "../services/thread-scope.js"; import { embed, isEmbeddingAvailable } from "../services/embedding.js"; import { writeTriplesForThreadCreation } from "../services/triple-writer.js"; import { getEffectTracker } from "../services/effect-tracker.js"; @@ -106,14 +107,21 @@ export async function createThread( } } - // Phase 3: Load existing open threads with embeddings for dedup check + // Phase 3 / GIT-69: Load dedup candidates through the one scope resolver. + // Candidates are this session's own threads in this project — never another + // session's, which cannot be safely auto-merged (R1). + const scope = resolveThreadScope({ project, sessionId: sessionId ?? null }); + let existingThreads: ThreadWithEmbedding[] = []; - const loadedFromSupabase = await loadOpenThreadEmbeddings(project); + const loadedFromSupabase = await loadOpenThreadEmbeddings(scope); if (loadedFromSupabase) { existingThreads = loadedFromSupabase; - } else { - // Supabase unavailable: use local threads for text-only fallback - const localThreads = loadThreadsFile().filter((t) => t.status === "open"); + } else if (scope.sessionId) { + // Supabase unavailable: use local threads for text-only fallback, held to + // the same scope the query would have applied. + const localThreads = loadThreadsFile().filter( + (t) => t.status === "open" && t.source_session === scope.sessionId + ); existingThreads = localThreads.map((t) => ({ thread_id: t.id, text: t.text, diff --git a/src/tools/list-threads.ts b/src/tools/list-threads.ts index 72eedbb..49953fb 100644 --- a/src/tools/list-threads.ts +++ b/src/tools/list-threads.ts @@ -17,6 +17,7 @@ import { getThreads, getProject } from "../services/session-state.js"; import { aggregateThreads, loadThreadsFile, mergeThreadStates } from "../services/thread-manager.js"; import { deduplicateThreadList } from "../services/thread-dedup.js"; import { listThreadsFromSupabase } from "../services/thread-supabase.js"; +import { resolveThreadScope } from "../services/thread-scope.js"; import * as supabase from "../services/supabase-client.js"; import { Timer, @@ -92,8 +93,9 @@ export async function listThreads( let allThreads: ThreadObject[] | null = null; let source: "supabase" | "aggregation" | "memory" | "file" = "file"; - // Try Supabase first (source of truth) - const supabaseThreads = await listThreadsFromSupabase(project, { + // Try Supabase first (source of truth) — scoped by the one resolver (GIT-69) + const scope = resolveThreadScope({ project }); + const supabaseThreads = await listThreadsFromSupabase(scope, { statusFilter: includeResolved ? undefined : statusFilter, includeResolved, }); diff --git a/src/tools/resolve-thread.ts b/src/tools/resolve-thread.ts index 683e0e8..dac1cc3 100644 --- a/src/tools/resolve-thread.ts +++ b/src/tools/resolve-thread.ts @@ -27,6 +27,7 @@ import { getThreadFromSupabaseById, listThreadsFromSupabase, } from "../services/thread-supabase.js"; +import { resolveThreadScope } from "../services/thread-scope.js"; import { writeTriplesForThreadResolution } from "../services/triple-writer.js"; import { getEffectTracker } from "../services/effect-tracker.js"; import { getAgentIdentity } from "../services/agent-detection.js"; @@ -251,8 +252,10 @@ async function hydrateThreadFromSupabase( return await getThreadFromSupabaseById(threadId); } if (textMatch) { - const project = getProject() || "default"; - const openThreads = await listThreadsFromSupabase(project, { statusFilter: "open" }); + // GIT-69: same scope resolver as list_threads, so a thread the user can + // see is a thread they can resolve by text. + const scope = resolveThreadScope(); + const openThreads = await listThreadsFromSupabase(scope, { statusFilter: "open" }); if (openThreads) { return findThreadByText(openThreads, textMatch); } diff --git a/src/tools/session-close.ts b/src/tools/session-close.ts index 13e28d6..66b5728 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -17,6 +17,8 @@ import { clearCurrentSession, getSurfacedScars, getConfirmations, getReflections import { normalizeThreads, mergeThreadStates, migrateStringThread, saveThreadsFile } from "../services/thread-manager.js"; // import { deduplicateThreadList } from "../services/thread-dedup.js"; import { syncThreadsToSupabase, loadOpenThreadEmbeddings } from "../services/thread-supabase.js"; +import { resolveThreadScope } from "../services/thread-scope.js"; +import type { Project } from "../types/index.js"; import { validateSessionClose, buildCloseCompliance, @@ -1375,7 +1377,12 @@ export async function sessionClose( // Phase 5: Implicit thread detection (chained after embedding) const suggestProject: string = (existingSession?.project as string) || "default"; const recentSessions = await loadRecentSessionEmbeddings(suggestProject, 30, 20); - const threadEmbs = await loadOpenThreadEmbeddings(suggestProject); + // GIT-69: project visibility is deliberate here — the suggestion + // pass proposes threads to a human rather than silently merging. + const threadEmbs = await loadOpenThreadEmbeddings( + resolveThreadScope({ project: suggestProject as Project, sessionId }), + "project" + ); if (recentSessions && threadEmbs) { const existing = loadSuggestions(); const updated = detectSuggestedThreads( diff --git a/src/tools/session-start.ts b/src/tools/session-start.ts index 07da7d7..637ca31 100644 --- a/src/tools/session-start.ts +++ b/src/tools/session-start.ts @@ -33,6 +33,8 @@ import { setCurrentSession, getCurrentSession, addSurfacedScars, getSurfacedScar import { aggregateThreads, saveThreadsFile, loadThreadsFile, mergeThreadStates } from "../services/thread-manager.js"; // import { deduplicateThreadList } from "../services/thread-dedup.js"; import { loadActiveThreadsFromSupabase, archiveDormantThreads } from "../services/thread-supabase.js"; +import { resolveThreadScope, computePanelOmission, formatOmissionLine } from "../services/thread-scope.js"; +import type { ThreadScopeCounts } from "../services/thread-scope.js"; import type { ThreadDisplayInfo } from "../services/thread-supabase.js"; import { setGitmemDir, getGitmemDir, getSessionPath, getConfigProject } from "../services/gitmem-dir.js"; import { registerSession, findSessionByHostPid, pruneStale, migrateFromLegacy } from "../services/active-sessions.js"; @@ -157,6 +159,8 @@ async function loadLastSession( latency_ms: number; network_call: boolean; threadsFromSupabase: boolean; + /** What the scope held, so the panel can say what it omitted (GIT-69/R7). */ + scopeCounts?: ThreadScopeCounts | null; }> { const timer = new Timer(); @@ -183,17 +187,19 @@ async function loadLastSession( limit: 10, orderBy: { column: "created_at", ascending: false }, }), - loadActiveThreadsFromSupabase(project), + loadActiveThreadsFromSupabase(resolveThreadScope({ project })), ]); let aggregated_open_threads: ThreadObject[]; let displayInfo: ThreadDisplayInfo[] = []; let threadsFromSupabase = false; + let scopeCounts: ThreadScopeCounts | null = null; if (supabaseThreads !== null) { // Supabase is source of truth for threads aggregated_open_threads = supabaseThreads.open; displayInfo = supabaseThreads.displayInfo; + scopeCounts = supabaseThreads.scope; threadsFromSupabase = true; console.error(`[session_start] Loaded ${aggregated_open_threads.length} open threads from Supabase`); @@ -209,7 +215,7 @@ async function loadLastSession( const latency_ms = timer.stop(); if (sessions.length === 0) { - return { session: null, aggregated_open_threads, displayInfo, latency_ms, network_call: true, threadsFromSupabase }; + return { session: null, aggregated_open_threads, displayInfo, latency_ms, network_call: true, threadsFromSupabase, scopeCounts }; } // Find the most recent session that was properly closed @@ -248,6 +254,7 @@ async function loadLastSession( latency_ms, network_call: true, threadsFromSupabase, + scopeCounts, }; } catch (error) { console.error("[session_start] Failed to load last session:", error); @@ -864,7 +871,7 @@ function stripThreadPrefix(text: string): string { return text.replace(/^t-[a-f0-9]+:\s*/i, ""); } -function formatStartDisplay(result: SessionStartResult, displayInfoMap?: Map, isFirstSession?: boolean, analytics?: LightweightSummary | null): string { +function formatStartDisplay(result: SessionStartResult, displayInfoMap?: Map, isFirstSession?: boolean, analytics?: LightweightSummary | null, scopeCounts?: ThreadScopeCounts | null): string { const visual: string[] = []; // Line 1: branded product line + session state @@ -912,8 +919,17 @@ function formatStartDisplay(result: SessionStartResult, displayInfoMap?: Map 60 ? text.slice(0, 57) + "..." : text; visual.push(` ${truncated}`); } - if (result.open_threads!.length > maxShow) { - visual.push(` +${result.open_threads!.length - maxShow} more`); + + // GIT-69/R7: no silent truncation. A capped or filtered view states what + // it omitted and where the rest lives. + const counts: ThreadScopeCounts = scopeCounts ?? { + listable: result.open_threads!.length, + dormantHidden: 0, + totalInScope: result.open_threads!.length, + }; + const omissionLine = formatOmissionLine(computePanelOmission(counts, maxShow)); + if (omissionLine) { + visual.push(dimText(` ${omissionLine}`)); } } @@ -1184,7 +1200,7 @@ export async function sessionStart( displayInfoMap.set(info.thread.id, info); } const isFirstSession = !isResuming && !slimLastSession; - result.display = formatStartDisplay(result, displayInfoMap, isFirstSession, analyticsResult); + result.display = formatStartDisplay(result, displayInfoMap, isFirstSession, analyticsResult, lastSessionResult.scopeCounts); // Write display to per-session dir try { diff --git a/tests/unit/services/thread-scope.test.ts b/tests/unit/services/thread-scope.test.ts index a82666d..b0618da 100644 --- a/tests/unit/services/thread-scope.test.ts +++ b/tests/unit/services/thread-scope.test.ts @@ -25,6 +25,8 @@ const { buildScopedThreadQuery, isInScope, describeScope, + computePanelOmission, + formatOmissionLine, THREAD_SCOPE_ORDER, THREAD_SCOPE_LIMIT, INACTIVE_STATUSES, @@ -289,3 +291,74 @@ describe("project-disagreement fixture — the recovery→scoping seam", () => { expect(describeScope({ project: "gitmem", sessionId: null }, "own_session")).toContain("none"); }); }); + +// ---------- Display layer: no silent truncation (R7) ---------- + +describe("panel omission accounting", () => { + // The cap is annotation, not scope. These assert the panel cannot claim a + // completeness it does not have, and that its numbers reconcile exactly to + // the scope set: shown + dormantHidden + overCap === totalInScope. + + const cases = [ + { name: "everything fits", listable: 3, dormantHidden: 0, maxShow: 5 }, + { name: "over cap only", listable: 12, dormantHidden: 0, maxShow: 5 }, + { name: "dormant only", listable: 4, dormantHidden: 31, maxShow: 5 }, + { name: "both", listable: 16, dormantHidden: 31, maxShow: 5 }, + { name: "exactly at cap", listable: 5, dormantHidden: 0, maxShow: 5 }, + { name: "empty scope", listable: 0, dormantHidden: 0, maxShow: 5 }, + { name: "all dormant", listable: 0, dormantHidden: 9, maxShow: 5 }, + ]; + + for (const c of cases) { + it(`reconciles: ${c.name}`, () => { + const counts = { + listable: c.listable, + dormantHidden: c.dormantHidden, + totalInScope: c.listable + c.dormantHidden, + }; + const o = computePanelOmission(counts, c.maxShow); + + // The invariant R7 exists to protect. + expect(o.shown + o.dormantHidden + o.overCap).toBe(counts.totalInScope); + expect(o.shown).toBeLessThanOrEqual(c.maxShow); + expect(o.overCap).toBeGreaterThanOrEqual(0); + }); + } + + it("says nothing when the view is complete", () => { + const o = computePanelOmission({ listable: 3, dormantHidden: 0, totalInScope: 3 }, 5); + expect(formatOmissionLine(o)).toBeNull(); + }); + + it("discloses both omission kinds and where the rest lives", () => { + const o = computePanelOmission({ listable: 16, dormantHidden: 31, totalInScope: 47 }, 5); + const line = formatOmissionLine(o); + + expect(line).toContain("showing 5 of 47 in scope"); + expect(line).toContain("31 dormant"); + expect(line).toContain("11 over cap"); + expect(line).toContain("list_threads"); + }); + + it("omits a zero term rather than printing '0 dormant'", () => { + const overCapOnly = formatOmissionLine( + computePanelOmission({ listable: 12, dormantHidden: 0, totalInScope: 12 }, 5) + ); + expect(overCapOnly).toContain("7 over cap"); + expect(overCapOnly).not.toContain("dormant"); + + const dormantOnly = formatOmissionLine( + computePanelOmission({ listable: 4, dormantHidden: 31, totalInScope: 35 }, 5) + ); + expect(dormantOnly).toContain("31 dormant"); + expect(dormantOnly).not.toContain("over cap"); + }); + + it("discloses dormant-only omission even when nothing is over cap", () => { + // The panel looks complete — 4 threads listed, 4 threads shown — while 31 + // in-scope threads are withheld. Exactly the case a naive "+N more" misses. + const o = computePanelOmission({ listable: 4, dormantHidden: 31, totalInScope: 35 }, 5); + expect(o.overCap).toBe(0); + expect(formatOmissionLine(o)).not.toBeNull(); + }); +}); From 7aa7af3489fdd84d7a632c453a67b73f7e757847 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 22:51:02 -0400 Subject: [PATCH 04/10] feat: thread drift detection, pure diff (GIT-69 item 2, part 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cache-health reported healthy on scar parity alone while two threads were demonstrably divergent (feedback ad5ca35a). "Healthy" asserted from one covered store is the same shape as a write reporting success from an unread row — a claim of completeness that was never checked. diffThreads() supplies the thread half as a pure, testable diff. Three buckets rather than two, because the stores are not partitioned the same way: remote rows carry a project, the local threads.json does not (ThreadObject has no project field, so one flat file holds every project's threads). A naive in-local/in-remote diff for project A would report every local project-B thread as drift. That is false drift, and a detector that cries wolf trains its reader to ignore it — which is how ad5ca35a's real divergence stayed invisible beside a healthy banner. So local-only IDs are reported as explicitly unattributable rather than folded into drift, and describeThreadDrift() always names the count even though it is not drift: an omitted number reads as zero. Tests: +13, including the equal-counts-but-divergent case a count-parity check calls healthy, and the false-drift case. Not yet wired into checkCacheHealth — one definitional question posted to GIT-69 first (whether unattributable locals degrade status). Refs GIT-69 Co-Authored-By: Claude Opus 5 --- src/services/thread-drift.ts | 166 ++++++++++++++++++++ tests/unit/services/thread-drift.test.ts | 186 +++++++++++++++++++++++ 2 files changed, 352 insertions(+) create mode 100644 src/services/thread-drift.ts create mode 100644 tests/unit/services/thread-drift.test.ts diff --git a/src/services/thread-drift.ts b/src/services/thread-drift.ts new file mode 100644 index 0000000..e7b40c4 --- /dev/null +++ b/src/services/thread-drift.ts @@ -0,0 +1,166 @@ +/** + * Thread Drift Detection (GIT-69 item 2) + * + * cache-health reported `status: healthy` on scar parity alone while two + * threads were demonstrably divergent between stores (feedback ad5ca35a). + * "Healthy" asserted from one covered store is the same shape as a write + * reporting success from an unread row: a claim of a completeness that was + * never checked. + * + * This module supplies the thread half. It is a pure diff — no I/O — so the + * comparison can be tested against fixtures without a database. + * + * ## Why there are three buckets and not two + * + * The two stores are not partitioned the same way. Remote `threads` rows carry + * a `project`; the local `threads.json` cache does not — `ThreadObject` has no + * project field, so one flat file holds every project's threads. + * + * A naive two-bucket diff (in-local / in-remote) for project A would therefore + * report every local project-B thread as drift. That is false drift, and a + * drift detector that cries wolf is worse than none: it trains its reader to + * ignore it, which is how ad5ca35a's real divergence stayed invisible next to + * a healthy banner. + * + * So local-only IDs are reported in their own bucket, explicitly unattributable + * rather than folded into drift. The honest statement is "these exist locally + * and I cannot tell whose they are", not "these are missing remotely". + * + * The durable fix is to give the local cache a project field so attribution is + * possible; until then this module reports what is actually knowable. + */ + +// --- Types --- + +/** Minimal shape needed from either store. */ +export interface DriftThread { + id: string; + status?: string; +} + +export interface ThreadDrift { + /** Present in both stores, states agree. */ + in_sync_count: number; + /** Present in both stores, states disagree. Unambiguous drift. */ + divergent: DriftDivergence[]; + /** In the remote scope, absent locally. Unambiguous drift. */ + only_remote: string[]; + /** + * Present locally, not in this project's remote scope. NOT counted as drift: + * the local cache is not project-partitioned, so these may legitimately + * belong to another project. + */ + unattributable_local: string[]; + /** Divergent + only_remote. The count that can be asserted. */ + drift_count: number; + /** True when nothing attributable diverges. */ + in_sync: boolean; +} + +export interface DriftDivergence { + id: string; + local_status?: string; + remote_status?: string; +} + +// --- Core --- + +/** + * Diff local cache threads against the remote scope's threads. + * + * `remote` must already be scoped (project, status) by the caller via the + * thread-scope resolver — this function does not decide what is in scope, only + * whether the two views of that scope agree. + */ +export function diffThreads( + local: readonly DriftThread[], + remote: readonly DriftThread[] +): ThreadDrift { + const remoteById = new Map(); + for (const t of remote) { + if (t.id) remoteById.set(t.id, t); + } + + const localById = new Map(); + for (const t of local) { + if (t.id) localById.set(t.id, t); + } + + const divergent: DriftDivergence[] = []; + const unattributable_local: string[] = []; + let in_sync_count = 0; + + for (const [id, localThread] of localById) { + const remoteThread = remoteById.get(id); + + if (!remoteThread) { + unattributable_local.push(id); + continue; + } + + // Both stores hold it — do they agree? + if ( + localThread.status !== undefined && + remoteThread.status !== undefined && + localThread.status !== remoteThread.status + ) { + divergent.push({ + id, + local_status: localThread.status, + remote_status: remoteThread.status, + }); + } else { + in_sync_count++; + } + } + + const only_remote: string[] = []; + for (const id of remoteById.keys()) { + if (!localById.has(id)) only_remote.push(id); + } + + // Deterministic output — a drift report that reorders between identical + // runs cannot be diffed by a human or a test. + divergent.sort((a, b) => a.id.localeCompare(b.id)); + only_remote.sort(); + unattributable_local.sort(); + + const drift_count = divergent.length + only_remote.length; + + return { + in_sync_count, + divergent, + only_remote, + unattributable_local, + drift_count, + in_sync: drift_count === 0, + }; +} + +/** + * One-line summary for cache-health's details field. + * + * Always states the unattributable count when non-zero, even though it is not + * drift — an omitted number reads as zero, and the reader cannot otherwise + * tell "no local extras" from "local extras I declined to classify". + */ +export function describeThreadDrift(drift: ThreadDrift): string { + if (drift.in_sync && drift.unattributable_local.length === 0) { + return `threads in sync (${drift.in_sync_count})`; + } + + const parts: string[] = []; + if (drift.divergent.length > 0) { + parts.push(`${drift.divergent.length} divergent`); + } + if (drift.only_remote.length > 0) { + parts.push(`${drift.only_remote.length} missing locally`); + } + if (drift.unattributable_local.length > 0) { + parts.push( + `${drift.unattributable_local.length} local-only (project unattributable — local cache is not project-partitioned)` + ); + } + + return `threads: ${parts.join(", ")} of ${drift.in_sync_count + drift.drift_count} compared`; +} diff --git a/tests/unit/services/thread-drift.test.ts b/tests/unit/services/thread-drift.test.ts new file mode 100644 index 0000000..754921e --- /dev/null +++ b/tests/unit/services/thread-drift.test.ts @@ -0,0 +1,186 @@ +/** + * Unit tests for Thread Drift Detection (GIT-69 item 2) + * + * Pure function tests — no mocks, no I/O. + * + * The governing case is ad5ca35a: cache-health reported healthy while two + * threads were divergent. These assert that a divergence of any kind is + * detectable, and — equally — that the detector does not manufacture drift + * from the local cache's lack of project partitioning. + */ + +import { describe, it, expect } from "vitest"; +import { + diffThreads, + describeThreadDrift, +} from "../../../src/services/thread-drift.js"; + +describe("diffThreads()", () => { + it("reports in sync when both stores agree", () => { + const both = [ + { id: "t-aaaa1111", status: "open" }, + { id: "t-bbbb2222", status: "open" }, + ]; + const drift = diffThreads(both, both); + + expect(drift.in_sync).toBe(true); + expect(drift.drift_count).toBe(0); + expect(drift.in_sync_count).toBe(2); + }); + + it("detects a status disagreement on a thread both stores hold", () => { + // The ad5ca35a shape: same thread, different state, counts identical. + const local = [{ id: "t-aaaa1111", status: "open" }]; + const remote = [{ id: "t-aaaa1111", status: "resolved" }]; + + const drift = diffThreads(local, remote); + + expect(drift.in_sync).toBe(false); + expect(drift.drift_count).toBe(1); + expect(drift.divergent).toEqual([ + { id: "t-aaaa1111", local_status: "open", remote_status: "resolved" }, + ]); + }); + + it("catches divergence that equal counts would hide", () => { + // Both stores hold exactly 2 threads. A count-parity check calls this + // healthy — which is precisely the reported defect. + const local = [ + { id: "t-aaaa1111", status: "open" }, + { id: "t-bbbb2222", status: "open" }, + ]; + const remote = [ + { id: "t-aaaa1111", status: "resolved" }, + { id: "t-bbbb2222", status: "open" }, + ]; + + expect(local.length).toBe(remote.length); + expect(diffThreads(local, remote).in_sync).toBe(false); + }); + + it("reports a remote thread missing locally as drift", () => { + const drift = diffThreads([], [{ id: "t-cccc3333", status: "open" }]); + + expect(drift.only_remote).toEqual(["t-cccc3333"]); + expect(drift.drift_count).toBe(1); + expect(drift.in_sync).toBe(false); + }); +}); + +describe("local cache is not project-partitioned", () => { + it("does NOT count a local-only thread as drift", () => { + // ThreadObject has no project field, so a local thread absent from this + // project's remote scope may simply belong to another project. Calling it + // drift would be false drift, and a detector that cries wolf trains its + // reader to ignore the real signal. + const local = [ + { id: "t-aaaa1111", status: "open" }, + { id: "t-wwww9999", status: "open" }, // weekend_warrior, say + ]; + const remote = [{ id: "t-aaaa1111", status: "open" }]; + + const drift = diffThreads(local, remote); + + expect(drift.unattributable_local).toEqual(["t-wwww9999"]); + expect(drift.drift_count).toBe(0); + expect(drift.in_sync).toBe(true); + }); + + it("still reports genuine drift alongside unattributable locals", () => { + const local = [ + { id: "t-aaaa1111", status: "open" }, + { id: "t-wwww9999", status: "open" }, + ]; + const remote = [ + { id: "t-aaaa1111", status: "resolved" }, + { id: "t-cccc3333", status: "open" }, + ]; + + const drift = diffThreads(local, remote); + + expect(drift.divergent.map((d) => d.id)).toEqual(["t-aaaa1111"]); + expect(drift.only_remote).toEqual(["t-cccc3333"]); + expect(drift.unattributable_local).toEqual(["t-wwww9999"]); + expect(drift.drift_count).toBe(2); // unattributable excluded + }); +}); + +describe("edge cases", () => { + it("treats both stores empty as in sync", () => { + const drift = diffThreads([], []); + expect(drift.in_sync).toBe(true); + expect(drift.in_sync_count).toBe(0); + }); + + it("does not flag divergence when a status is unknown on either side", () => { + // Absent status means "not compared", not "disagrees". + expect(diffThreads([{ id: "t-a" }], [{ id: "t-a", status: "open" }]).in_sync).toBe(true); + expect(diffThreads([{ id: "t-a", status: "open" }], [{ id: "t-a" }]).in_sync).toBe(true); + }); + + it("ignores entries with no id rather than crashing", () => { + const drift = diffThreads([{ id: "", status: "open" }], []); + expect(drift.drift_count).toBe(0); + expect(drift.unattributable_local).toEqual([]); + }); + + it("produces deterministic ordering", () => { + const local = [ + { id: "t-cccc", status: "open" }, + { id: "t-aaaa", status: "open" }, + { id: "t-bbbb", status: "open" }, + ]; + const remote = [ + { id: "t-cccc", status: "resolved" }, + { id: "t-aaaa", status: "resolved" }, + { id: "t-bbbb", status: "resolved" }, + ]; + + const first = diffThreads(local, remote); + const second = diffThreads([...local].reverse(), [...remote].reverse()); + + expect(first.divergent.map((d) => d.id)).toEqual(["t-aaaa", "t-bbbb", "t-cccc"]); + expect(first).toEqual(second); + }); +}); + +describe("describeThreadDrift()", () => { + it("states plain agreement when there is nothing to disclose", () => { + const drift = diffThreads( + [{ id: "t-a", status: "open" }], + [{ id: "t-a", status: "open" }] + ); + expect(describeThreadDrift(drift)).toBe("threads in sync (1)"); + }); + + it("names the unattributable count even though it is not drift", () => { + // An omitted number reads as zero. The reader cannot otherwise tell + // "no local extras" from "local extras I declined to classify". + const drift = diffThreads( + [ + { id: "t-a", status: "open" }, + { id: "t-w", status: "open" }, + ], + [{ id: "t-a", status: "open" }] + ); + + expect(drift.in_sync).toBe(true); + const text = describeThreadDrift(drift); + expect(text).toContain("1 local-only"); + expect(text).toContain("unattributable"); + }); + + it("names each drift kind separately", () => { + const drift = diffThreads( + [{ id: "t-a", status: "open" }], + [ + { id: "t-a", status: "resolved" }, + { id: "t-c", status: "open" }, + ] + ); + const text = describeThreadDrift(drift); + + expect(text).toContain("1 divergent"); + expect(text).toContain("1 missing locally"); + }); +}); From 646c0ef9886d71fe493fc6e756072d7fb03f6ac6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 22:56:07 -0400 Subject: [PATCH 05/10] feat: cache-health covers threads, three-valued status (GIT-69 item 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per R9a. cache-health reported healthy on scar parity alone while two threads were divergent (ad5ca35a). Threads are now a covered store, and "healthy" requires parity on every store checked. Status is three-valued because binary was the false choice: healthy full attribution AND no divergence — earned, never defaulted unverifiable no known divergence, but the check did not complete drift divergence is known "Healthy" beside "12 threads I could not classify" is a completeness claim over an incomplete check. "Drift" would be equally false — nothing is known divergent. The middle state exists so neither lie is available. Same shape as R4's local_only/durable:false, applied to reads instead of writes. A failed remote thread read reports unavailable and downgrades the overall verdict, rather than silently omitting the store it could not reach and still printing healthy — that omission IS ad5ca35a. checkCacheHealth's project parameter becomes live for the thread half and stays ignored for the scar half, deliberately: the local scar cache is unified cross-project, threads are project-scoped by schema. Documented at the call site rather than papered over. Tests: +18 total on thread-drift (5 new for three-valued status, including drift-beats-unverifiable and never-healthy-with-a-non-empty-bucket). Suite 1060 green across 64 files, typecheck clean. Refs GIT-69, ad5ca35a Co-Authored-By: Claude Opus 5 --- src/services/startup.ts | 111 ++++++++++++++++++++++- src/services/thread-drift.ts | 39 +++++++- tests/unit/services/thread-drift.test.ts | 66 +++++++++++++- 3 files changed, 206 insertions(+), 10 deletions(-) diff --git a/src/services/startup.ts b/src/services/startup.ts index 783112f..ff08ae6 100644 --- a/src/services/startup.ts +++ b/src/services/startup.ts @@ -28,6 +28,11 @@ import { clearLocalSearch, } from "./local-vector-search.js"; import { getConfig, shouldUseLocalSearch } from "./config.js"; +import { diffThreads, describeThreadDrift } from "./thread-drift.js"; +import type { ThreadDriftStatus } from "./thread-drift.js"; +import { loadThreadsFile } from "./thread-manager.js"; +import { listThreadsFromSupabase } from "./thread-supabase.js"; +import { resolveThreadScope } from "./thread-scope.js"; import type { Project } from "../types/index.js"; // Track startup state — unified cache, no per-project partitioning @@ -318,13 +323,35 @@ export interface CacheStatus { } export interface CacheHealth { - status: "healthy" | "stale" | "out_of_sync" | "unavailable"; + /** + * GIT-69 item 2 / R9a: `unverifiable` is the honest middle state — no known + * divergence, but the check did not complete over every covered store. + * `healthy` is earned, never defaulted. + */ + status: "healthy" | "unverifiable" | "stale" | "out_of_sync" | "unavailable"; local_scar_count: number; remote_scar_count: number; local_latest_updated_at: string | null; remote_latest_updated_at: string | null; needs_refresh: boolean; details: string; + /** + * Thread-store parity. `ad5ca35a`: cache-health reported healthy on scar + * parity alone while two threads were demonstrably divergent — "healthy" + * asserted from one covered store is a claim of completeness never checked. + */ + threads?: ThreadCacheHealth; +} + +export interface ThreadCacheHealth { + status: ThreadDriftStatus | "unavailable"; + local_count: number; + remote_count: number; + drift_count: number; + unattributable_count: number; + /** Bounded sample — full lists can be large; see details for totals. */ + divergent_ids: string[]; + details: string; } export interface CacheFlushResult { @@ -383,7 +410,7 @@ export function getCacheStatus(_project?: Project): CacheStatus { /** * Check cache health against remote Supabase */ -export async function checkCacheHealth(_project?: Project): Promise { +export async function checkCacheHealth(project?: Project): Promise { const config = getConfig(); if (config.resolvedSearchMode === "remote") { @@ -438,6 +465,31 @@ export async function checkCacheHealth(_project?: Project): Promise details = `Timestamp mismatch: local=${localLatest}, remote=${remoteLatest}`; } + // GIT-69 item 2: threads are a covered store too. "Healthy" now requires + // parity on every store checked, not scars alone (ad5ca35a). + // + // Note the asymmetry, deliberate: scars are compared cross-project because + // the local cache is unified, while threads are project-scoped by schema. + // So `project` is load-bearing for this half of the check and ignored for + // the other — which is why it was `_project` until now. + const threads = await checkThreadHealth(project); + + if (threads.status === "drift") { + status = "out_of_sync"; + needsRefresh = true; + details = `${details}; ${threads.details}`; + } else if ( + (threads.status === "unverifiable" || threads.status === "unavailable") && + status === "healthy" + ) { + // R9a: no known divergence, but the check did not complete. Neither lie + // is available — not green, not drift. + status = "unverifiable"; + details = threads.details; + } else if (threads.status !== "healthy") { + details = `${details}; ${threads.details}`; + } + return { status, local_scar_count: localCount, @@ -446,6 +498,61 @@ export async function checkCacheHealth(_project?: Project): Promise remote_latest_updated_at: remoteLatest, needs_refresh: needsRefresh, details, + threads, + }; +} + +/** + * Diff the local thread cache against the project's remote thread scope. + * + * A failed remote read reports `unavailable` rather than falling through to a + * scar-only verdict — silently omitting a store you could not reach and still + * printing "healthy" is precisely the ad5ca35a defect. R7's no-silent- + * truncation invariant, applied to a read. + */ +async function checkThreadHealth(project?: Project): Promise { + const scope = resolveThreadScope(project ? { project } : undefined); + + let remote: { id: string; status?: string }[] | null = null; + try { + const rows = await listThreadsFromSupabase(scope, { statusFilter: "open" }); + remote = rows ? rows.map((t) => ({ id: t.id, status: t.status })) : null; + } catch (error) { + console.error( + "[startup] Thread health check failed:", + error instanceof Error ? error.message : error + ); + } + + if (remote === null) { + return { + status: "unavailable", + local_count: 0, + remote_count: -1, + drift_count: 0, + unattributable_count: 0, + divergent_ids: [], + details: `threads unavailable: could not read the remote store for project ${scope.project} — thread parity NOT checked`, + }; + } + + const local = loadThreadsFile() + .filter((t) => t.status === "open") + .map((t) => ({ id: t.id, status: t.status })); + + const drift = diffThreads(local, remote); + + return { + status: drift.status, + local_count: local.length, + remote_count: remote.length, + drift_count: drift.drift_count, + unattributable_count: drift.unattributable_local.length, + divergent_ids: [ + ...drift.divergent.map((d) => d.id), + ...drift.only_remote, + ].slice(0, 20), + details: describeThreadDrift(drift), }; } diff --git a/src/services/thread-drift.ts b/src/services/thread-drift.ts index e7b40c4..30a4df2 100644 --- a/src/services/thread-drift.ts +++ b/src/services/thread-drift.ts @@ -38,6 +38,20 @@ export interface DriftThread { status?: string; } +/** + * Three-valued, because binary is the false choice (R9a). + * + * healthy full attribution AND no divergence — earned, never defaulted + * unverifiable nothing known divergent, but some rows could not be attributed + * drift divergence is known + * + * `healthy` beside "12 threads I could not classify" would be a completeness + * claim over an incomplete check. `drift` would be equally false — nothing is + * known divergent. The honest middle state exists so neither lie is available, + * and the green banner is reserved for checks that actually completed. + */ +export type ThreadDriftStatus = "healthy" | "unverifiable" | "drift"; + export interface ThreadDrift { /** Present in both stores, states agree. */ in_sync_count: number; @@ -53,8 +67,10 @@ export interface ThreadDrift { unattributable_local: string[]; /** Divergent + only_remote. The count that can be asserted. */ drift_count: number; - /** True when nothing attributable diverges. */ + /** True when nothing attributable diverges. Not the same as `healthy`. */ in_sync: boolean; + /** Three-valued verdict — see ThreadDriftStatus. */ + status: ThreadDriftStatus; } export interface DriftDivergence { @@ -127,6 +143,15 @@ export function diffThreads( const drift_count = divergent.length + only_remote.length; + // R9a: healthy is earned — full attribution AND no divergence. Anything + // unattributable downgrades to unverifiable rather than passing as green. + const status: ThreadDriftStatus = + drift_count > 0 + ? "drift" + : unattributable_local.length > 0 + ? "unverifiable" + : "healthy"; + return { in_sync_count, divergent, @@ -134,6 +159,7 @@ export function diffThreads( unattributable_local, drift_count, in_sync: drift_count === 0, + status, }; } @@ -145,8 +171,8 @@ export function diffThreads( * tell "no local extras" from "local extras I declined to classify". */ export function describeThreadDrift(drift: ThreadDrift): string { - if (drift.in_sync && drift.unattributable_local.length === 0) { - return `threads in sync (${drift.in_sync_count})`; + if (drift.status === "healthy") { + return `threads healthy (${drift.in_sync_count} compared, all attributed)`; } const parts: string[] = []; @@ -156,11 +182,14 @@ export function describeThreadDrift(drift: ThreadDrift): string { if (drift.only_remote.length > 0) { parts.push(`${drift.only_remote.length} missing locally`); } + // R9a: the count AND the reason belong in the status line itself, not in a + // footnote — an omitted number reads as zero, and an unexplained one reads + // as a defect. if (drift.unattributable_local.length > 0) { parts.push( - `${drift.unattributable_local.length} local-only (project unattributable — local cache is not project-partitioned)` + `${drift.unattributable_local.length} unattributable (local cache has no project field, so these may belong to another project)` ); } - return `threads: ${parts.join(", ")} of ${drift.in_sync_count + drift.drift_count} compared`; + return `threads ${drift.status}: ${parts.join(", ")}; ${drift.in_sync_count} verified in sync`; } diff --git a/tests/unit/services/thread-drift.test.ts b/tests/unit/services/thread-drift.test.ts index 754921e..7ec1edb 100644 --- a/tests/unit/services/thread-drift.test.ts +++ b/tests/unit/services/thread-drift.test.ts @@ -144,13 +144,71 @@ describe("edge cases", () => { }); }); +describe("three-valued status (R9a) — healthy is earned, never defaulted", () => { + it("is healthy only with full attribution AND no divergence", () => { + const both = [{ id: "t-a", status: "open" }]; + expect(diffThreads(both, both).status).toBe("healthy"); + }); + + it("is unverifiable when nothing diverges but something is unattributable", () => { + // The state binary status has no room for: no known divergence, but the + // check did not complete. Green here would be a completeness claim over an + // incomplete check. + const drift = diffThreads( + [ + { id: "t-a", status: "open" }, + { id: "t-w", status: "open" }, + ], + [{ id: "t-a", status: "open" }] + ); + + expect(drift.drift_count).toBe(0); + expect(drift.in_sync).toBe(true); + expect(drift.status).toBe("unverifiable"); + }); + + it("is drift when divergence is known", () => { + const drift = diffThreads( + [{ id: "t-a", status: "open" }], + [{ id: "t-a", status: "resolved" }] + ); + expect(drift.status).toBe("drift"); + }); + + it("reports drift over unverifiable when both apply", () => { + // Known divergence is the stronger claim — it must not be masked by the + // weaker one. + const drift = diffThreads( + [ + { id: "t-a", status: "open" }, + { id: "t-w", status: "open" }, + ], + [{ id: "t-a", status: "resolved" }] + ); + + expect(drift.unattributable_local).toEqual(["t-w"]); + expect(drift.status).toBe("drift"); + }); + + it("never returns healthy while any bucket is non-empty", () => { + const cases = [ + diffThreads([{ id: "t-a", status: "open" }], [{ id: "t-a", status: "resolved" }]), + diffThreads([], [{ id: "t-c", status: "open" }]), + diffThreads([{ id: "t-w", status: "open" }], []), + ]; + for (const drift of cases) { + expect(drift.status).not.toBe("healthy"); + } + }); +}); + describe("describeThreadDrift()", () => { it("states plain agreement when there is nothing to disclose", () => { const drift = diffThreads( [{ id: "t-a", status: "open" }], [{ id: "t-a", status: "open" }] ); - expect(describeThreadDrift(drift)).toBe("threads in sync (1)"); + expect(describeThreadDrift(drift)).toBe("threads healthy (1 compared, all attributed)"); }); it("names the unattributable count even though it is not drift", () => { @@ -166,8 +224,10 @@ describe("describeThreadDrift()", () => { expect(drift.in_sync).toBe(true); const text = describeThreadDrift(drift); - expect(text).toContain("1 local-only"); - expect(text).toContain("unattributable"); + // R9a: count AND reason in the status line itself. + expect(text).toContain("unverifiable"); + expect(text).toContain("1 unattributable"); + expect(text).toContain("no project field"); }); it("names each drift kind separately", () => { From 87768eaa842dcf9b6f677029b3282e7c31f99e6d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:02:29 -0400 Subject: [PATCH 06/10] =?UTF-8?q?fix:=20atomic=20session=5Fclose=20?= =?UTF-8?q?=E2=80=94=20gate=20destructive=20local=20ops=20on=20sync=20outc?= =?UTF-8?q?ome=20(GIT-69=20item=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes feedback 11c313d0 (non-atomic multi-record persistence). The mechanism was sharper than "doesn't report a partial write": syncThreadsToSupabase(...).catch(err => console.error("... (non-fatal):", err)); // outcome discarded saveThreadsFile(openThreadsOnly); // prune runs regardless A failed sync plus a successful prune dropped resolved threads locally while remote still held them open — no error, green close. A destructive local operation gated on a remote write whose result was never examined. This is the purest instance of scar cd345431 (fire-and-forget hides constraint violations) in the product: the swallow and the destruction sat two lines apart, and the second depended on the first. Changes: - syncThreadsToSupabase returns ThreadSyncResult {attempted, synced, failed[], skipped, all_synced} instead of void. It still completes a partial sync — worth finishing — but now names every miss. - session_close AWAITS it. Prune runs only when all_synced; otherwise threads.json is retained in full, because it is the only remaining record of the threads that did not land. - Session files and the closing payload are retained on partial persist too. Reporting failure while deleting the means to retry would be R4's crime in reverse. - Close returns success:false with validation_errors naming each missed thread id and its error, per R4's fail-honest shape. R3 — the gate is tier-relative. Free tier has no Supabase to await, so sync reports skipped:true and the prune proceeds on local persistence alone. Awaiting a sync that does not exist would fail-close every free user's session close. Adjacency: with the sync now awaited, GIT-70's unordered limit-200 dedup window runs synchronously inside close, so its duplicate-creation behaviour is close-path-critical rather than background. Tests: +8 covering the outcome contract the prune gate depends on — tier skip, empty-list success, single failure recorded not swallowed, partial split, never-all_synced-with-failures, every missed id named. Suite 1068 green across 65 files, typecheck and build clean. Refs GIT-69, GIT-70, 11c313d0, cd345431 Co-Authored-By: Claude Opus 5 --- src/services/thread-supabase.ts | 55 +++++- src/tools/session-close.ts | 89 ++++++++-- .../unit/services/thread-sync-result.test.ts | 167 ++++++++++++++++++ 3 files changed, 290 insertions(+), 21 deletions(-) create mode 100644 tests/unit/services/thread-sync-result.test.ts diff --git a/src/services/thread-supabase.ts b/src/services/thread-supabase.ts index 166d8a3..02ab5d5 100644 --- a/src/services/thread-supabase.ts +++ b/src/services/thread-supabase.ts @@ -58,6 +58,19 @@ export interface ThreadDisplayInfo { export type { ThreadScopeCounts } from "./thread-scope.js"; +/** + * Outcome of a thread sync, so a caller can decide whether it is safe to + * discard local state (GIT-69 item 3). `skipped` means Supabase is not this + * tier's SOT — nothing to sync, which is success, not failure. + */ +export interface ThreadSyncResult { + attempted: number; + synced: string[]; + failed: { id: string; error: string }[]; + skipped: boolean; + all_synced: boolean; +} + export interface ActiveThreadsResult { open: ThreadObject[]; displayInfo: ThreadDisplayInfo[]; @@ -487,11 +500,25 @@ export async function syncThreadsToSupabase( threads: ThreadObject[], project: Project = "default", sessionId?: string -): Promise { - if (!hasSupabase() || !supabase.isConfigured() || threads.length === 0) { - return; +): Promise { + // GIT-69 item 3: this used to return void and swallow every per-thread + // error to a console line marked "non-fatal", which made the outcome + // unknowable to the caller. session_close then pruned the local cache + // regardless. A destructive local operation cannot be gated on a remote + // write whose result was never examined (scar cd345431). + if (!hasSupabase() || !supabase.isConfigured()) { + // Not a failure: on free tier Supabase is not the SOT, so there is + // nothing to sync and nothing to gate on. + return { attempted: 0, synced: [], failed: [], skipped: true, all_synced: true }; + } + + if (threads.length === 0) { + return { attempted: 0, synced: [], failed: [], skipped: false, all_synced: true }; } + const synced: string[] = []; + const failed: { id: string; error: string }[] = []; + // Load existing open threads once upfront for text-based dedup. // Prevents duplicate creation when closing ceremony generates new thread IDs // for threads that already exist with the same (or similar) text. @@ -558,11 +585,29 @@ export async function syncThreadsToSupabase( // Existing thread, just touch it await touchThreadsInSupabase([thread.id]); } + synced.push(thread.id); } catch (error) { - console.error(`[thread-supabase] Failed to sync thread ${thread.id}:`, error instanceof Error ? error.message : error); - // Continue with other threads + const message = error instanceof Error ? error.message : String(error); + console.error(`[thread-supabase] Failed to sync thread ${thread.id}:`, message); + // Continue with the other threads — a partial sync is still worth + // completing — but record the miss so the caller can refuse to prune. + failed.push({ id: thread.id, error: message }); } } + + if (failed.length > 0) { + console.error( + `[thread-supabase] Thread sync incomplete: ${synced.length} synced, ${failed.length} failed (${failed.map((f) => f.id).join(", ")})` + ); + } + + return { + attempted: threads.length, + synced, + failed, + skipped: false, + all_synced: failed.length === 0, + }; } // ---------- Archival (Phase 6) ---------- diff --git a/src/tools/session-close.ts b/src/tools/session-close.ts index 66b5728..cb70bec 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -17,6 +17,7 @@ import { clearCurrentSession, getSurfacedScars, getConfirmations, getReflections import { normalizeThreads, mergeThreadStates, migrateStringThread, saveThreadsFile } from "../services/thread-manager.js"; // import { deduplicateThreadList } from "../services/thread-dedup.js"; import { syncThreadsToSupabase, loadOpenThreadEmbeddings } from "../services/thread-supabase.js"; +import type { ThreadSyncResult } from "../services/thread-supabase.js"; import { resolveThreadScope } from "../services/thread-scope.js"; import type { Project } from "../types/index.js"; import { @@ -1258,20 +1259,55 @@ export async function sessionClose( params, existingSession, isRetroactive, agentIdentity, closeCompliance, sessionId ); - // Sync threads to Supabase (fire-and-forget, non-blocking) + // GIT-69 item 3: sync threads and AWAIT the outcome. + // + // This was fire-and-forget with the error swallowed to a console line marked + // "non-fatal", and the local prune below ran unconditionally right after. A + // failed sync plus a successful prune drops resolved threads locally while + // remote still holds them open — no error, green close. The purest instance + // of scar cd345431 in the product: fire-and-forget hiding a failed write, + // with a destructive local operation gated on its unexamined result. + // + // R3: the gate is tier-relative. On free tier the local file IS the SOT and + // there is no sync to await — `skipped: true`, and the prune proceeds on + // local persistence alone. Awaiting a sync that does not exist would + // fail-close every free user's session close. const closeThreads = (sessionData.open_threads || []) as ThreadObject[]; + let threadSync: ThreadSyncResult = { + attempted: 0, synced: [], failed: [], skipped: true, all_synced: true, + }; + if (closeThreads.length > 0) { const closeProject = isRetroactive ? "default" : (existingSession?.project as string | undefined) || "default"; - syncThreadsToSupabase(closeThreads, closeProject, sessionId).catch((err) => { - console.error("[session_close] Thread Supabase sync failed (non-fatal):", err); - }); + try { + threadSync = await syncThreadsToSupabase(closeThreads, closeProject, sessionId); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error("[session_close] Thread sync threw:", message); + threadSync = { + attempted: closeThreads.length, + synced: [], + failed: closeThreads.map((t) => ({ id: t.id, error: message })), + skipped: false, + all_synced: false, + }; + } } - // Prune threads.json: only keep open threads + // Prune threads.json: only keep open threads — ONLY if the durable store + // agrees. A partial sync means the local file is the only record of the + // threads that did not land; discarding it would destroy them. try { - const openThreadsOnly = closeThreads.filter(t => t.status === "open" || !t.status); - saveThreadsFile(openThreadsOnly); - console.error(`[session_close] Pruned threads.json: ${openThreadsOnly.length} open threads (removed ${closeThreads.length - openThreadsOnly.length} resolved/archived)`); + if (!threadSync.all_synced) { + console.error( + `[session_close] Local thread prune SKIPPED — ${threadSync.failed.length} thread(s) did not reach Supabase (${threadSync.failed.map((f) => f.id).join(", ")}). ` + + `threads.json retained in full; it is the only record of the unsynced threads.` + ); + } else { + const openThreadsOnly = closeThreads.filter(t => t.status === "open" || !t.status); + saveThreadsFile(openThreadsOnly); + console.error(`[session_close] Pruned threads.json: ${openThreadsOnly.length} open threads (removed ${closeThreads.length - openThreadsOnly.length} resolved/archived)`); + } } catch (err) { console.error("[session_close] Failed to prune threads.json (non-fatal):", err); } @@ -1478,21 +1514,42 @@ export async function sessionClose( const decisionsCount = params.decisions?.length || 0; await writeAgentBriefing(learningsCount, decisionsCount); - // GIT-21: Clean up session files (registry, per-session dir, legacy file) - cleanupSessionFiles(sessionId); + // GIT-69 item 3 / R4: a partial persist is not a close. Report it + // honestly AND keep the state needed to retry — cleaning up the session + // files and payload here would destroy the recovery path while announcing + // the failure, which is the crime in reverse. + const partialPersist = !threadSync.all_synced; + + if (!partialPersist) { + // GIT-21: Clean up session files (registry, per-session dir, legacy file) + cleanupSessionFiles(sessionId); - // Clean up payload file AFTER successful close (not before — crash safety) - if (payloadConsumed) { - try { fs.unlinkSync(payloadPath); } catch { /* already gone */ } + // Clean up payload file AFTER successful close (not before — crash safety) + if (payloadConsumed) { + try { fs.unlinkSync(payloadPath); } catch { /* already gone */ } + } + } else { + console.error( + `[session_close] Partial persist — retaining session files and payload for ${sessionId} so the close can be retried` + ); } - const display = formatCloseDisplay(sessionId, closeCompliance, params, learningsCount, true, validation.warnings.length > 0 ? validation.warnings : undefined, transcriptStatus, blindspotSnippet); + const persistErrors = partialPersist + ? [ + `Thread sync incomplete: ${threadSync.failed.length} of ${threadSync.attempted} thread(s) did not reach Supabase — ${threadSync.failed.map((f) => `${f.id} (${f.error})`).join("; ")}. ` + + `threads.json was NOT pruned and session files were retained; re-run session_close to retry.`, + ] + : []; + + const allErrors = [...persistErrors, ...validation.warnings]; + + const display = formatCloseDisplay(sessionId, closeCompliance, params, learningsCount, !partialPersist, allErrors.length > 0 ? allErrors : undefined, transcriptStatus, blindspotSnippet); return { - success: true, + success: !partialPersist, session_id: sessionId, close_compliance: closeCompliance, - validation_errors: validation.warnings.length > 0 ? validation.warnings : undefined, + validation_errors: allErrors.length > 0 ? allErrors : undefined, performance: perfData, display, }; diff --git a/tests/unit/services/thread-sync-result.test.ts b/tests/unit/services/thread-sync-result.test.ts new file mode 100644 index 0000000..99e0d6a --- /dev/null +++ b/tests/unit/services/thread-sync-result.test.ts @@ -0,0 +1,167 @@ +/** + * Unit tests for syncThreadsToSupabase's outcome contract (GIT-69 item 3) + * + * syncThreadsToSupabase used to return void and swallow every per-thread error + * to a console line marked "non-fatal". session_close then pruned the local + * thread cache unconditionally — a destructive local operation gated on a + * remote write whose result was never examined (scar cd345431, feedback + * 11c313d0). + * + * These tests pin the outcome contract the prune gate depends on. If sync can + * fail without saying so, the gate above it is decorative. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const mockDirectQuery = vi.fn(); +const mockDirectUpsert = vi.fn(); +const mockIsConfigured = vi.fn(() => true); +const mockHasSupabase = vi.fn(() => true); + +vi.mock("../../../src/services/supabase-client.js", () => ({ + isConfigured: (...a: unknown[]) => mockIsConfigured(...a), + directQuery: (...a: unknown[]) => mockDirectQuery(...a), + directUpsert: (...a: unknown[]) => mockDirectUpsert(...a), + upsertRecord: vi.fn(), + listRecords: vi.fn(), +})); + +vi.mock("../../../src/services/tier.js", () => ({ + hasSupabase: (...a: unknown[]) => mockHasSupabase(...a), + getTableName: (base: string) => `orchestra_${base}`, + hasEmbeddings: () => false, +})); + +const { syncThreadsToSupabase } = await import( + "../../../src/services/thread-supabase.js" +); + +const thread = (id: string, status = "open") => ({ + id, + text: `thread ${id}`, + status: status as "open" | "resolved", + created_at: "2026-08-06T00:00:00.000Z", +}); + +beforeEach(() => { + mockDirectQuery.mockReset(); + mockDirectUpsert.mockReset(); + mockIsConfigured.mockReturnValue(true); + mockHasSupabase.mockReturnValue(true); + mockDirectUpsert.mockResolvedValue([{ id: "row" }]); +}); + +describe("tier awareness (R3)", () => { + it("reports skipped, not failed, when Supabase is not this tier's store", () => { + // Free tier: the local file IS the SOT. There is nothing to sync, and + // treating that as a failure would fail-close every free user's close. + mockHasSupabase.mockReturnValue(false); + + return syncThreadsToSupabase([thread("t-a")], "gitmem", "s-1").then((r) => { + expect(r.skipped).toBe(true); + expect(r.all_synced).toBe(true); + expect(r.failed).toEqual([]); + }); + }); + + it("reports skipped when the client is unconfigured", async () => { + mockIsConfigured.mockReturnValue(false); + const r = await syncThreadsToSupabase([thread("t-a")], "gitmem", "s-1"); + expect(r.skipped).toBe(true); + expect(r.all_synced).toBe(true); + }); + + it("treats an empty thread list as success, not as skipped", async () => { + const r = await syncThreadsToSupabase([], "gitmem", "s-1"); + expect(r.skipped).toBe(false); + expect(r.all_synced).toBe(true); + expect(r.attempted).toBe(0); + }); +}); + +describe("outcome reporting", () => { + it("reports all_synced when every thread lands", async () => { + // First call is the candidate load; the rest are per-thread lookups. + mockDirectQuery.mockResolvedValue([]); + + const r = await syncThreadsToSupabase( + [thread("t-a"), thread("t-b")], + "gitmem", + "s-1" + ); + + expect(r.all_synced).toBe(true); + expect(r.attempted).toBe(2); + expect(r.synced).toEqual(["t-a", "t-b"]); + expect(r.failed).toEqual([]); + }); + + it("records a failed thread instead of swallowing it", async () => { + mockDirectQuery + .mockResolvedValueOnce([]) // candidate load + .mockRejectedValueOnce(new Error("boom")); // t-a lookup + + const r = await syncThreadsToSupabase([thread("t-a")], "gitmem", "s-1"); + + expect(r.all_synced).toBe(false); + expect(r.failed).toHaveLength(1); + expect(r.failed[0].id).toBe("t-a"); + expect(r.failed[0].error).toContain("boom"); + }); + + it("continues past a failure and reports the partial split", async () => { + // A partial sync is still worth completing — but the miss must be named, + // because the local file is the only remaining record of it. + mockDirectQuery + .mockResolvedValueOnce([]) // candidate load + .mockRejectedValueOnce(new Error("network")) // t-a fails + .mockResolvedValueOnce([]); // t-b succeeds + + const r = await syncThreadsToSupabase( + [thread("t-a"), thread("t-b")], + "gitmem", + "s-1" + ); + + expect(r.attempted).toBe(2); + expect(r.synced).toEqual(["t-b"]); + expect(r.failed.map((f) => f.id)).toEqual(["t-a"]); + expect(r.all_synced).toBe(false); + }); + + it("never reports all_synced while any thread failed", async () => { + mockDirectQuery + .mockResolvedValueOnce([]) + .mockRejectedValue(new Error("down")); + + const r = await syncThreadsToSupabase( + [thread("t-a"), thread("t-b"), thread("t-c")], + "gitmem", + "s-1" + ); + + expect(r.failed).toHaveLength(3); + expect(r.all_synced).toBe(false); + // The prune gate reads exactly this field. + expect(r.synced).toEqual([]); + }); + + it("names every missed id so the caller can report them", async () => { + mockDirectQuery + .mockResolvedValueOnce([]) + .mockRejectedValue(new Error("timeout")); + + const r = await syncThreadsToSupabase( + [thread("t-aaa"), thread("t-bbb")], + "gitmem", + "s-1" + ); + + const ids = r.failed.map((f) => f.id); + expect(ids).toContain("t-aaa"); + expect(ids).toContain("t-bbb"); + for (const f of r.failed) { + expect(f.error).toBeTruthy(); + } + }); +}); From 9db579eb07567c7e8d398203ba9ac00d521affd3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:07:06 -0400 Subject: [PATCH 07/10] feat: project-scoped resolve with audit trail (GIT-69 item 4 v1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per R9c. Answers feedback c1b65b66 (orchestrator cannot resolve a child session's thread) without inventing a lineage model gitmem does not have. The asymmetry that justifies this, and does NOT justify it for dedup: resolve is an explicit, human-directed act on a NAMED thread — the caller states a specific target — while dedup is automatic and silent over a candidate set it never sees. R1's own-session caution therefore applies to dedup and not here. Condition (1) — cross-project resolve refused. getThreadFromSupabaseById filters on thread_id alone with NO project filter, so before this change a caller in project A could resolve project B's thread outright. Ownership is now checked BEFORE hydrating by ID, because hydrating first would resolve across projects silently. Condition (2) — audit trail as the price of the widened power. Any resolve of a thread this session does not own names the owning session in both the response and the log. Silence here is exactly what project scope would otherwise have bought us. No line when the caller owns the thread: an always-on notice is noise, and noise is how a real signal gets ignored. Condition (3) — a miss returns ownership information where it exists ("exists in project P, owned by session Y, status S") rather than a bare not-found. Reporting absence for a thread that demonstrably exists is the same lie as reporting success for a write that did not land. Condition (4) — parent_session filed as GIT-72 against GIT-51's session model. SessionChild is NOT a session: it has no session_id, it records roles inside one session. No lineage is synthesized from it here. New getThreadOwnership() is deliberately unscoped — the one lookup allowed to see across projects, because its purpose is to explain a refusal rather than grant access. Documented as such so it is not later mistaken for a widening primitive. Tests: +4 (cross-project refusal names the project and writes nothing; miss carries ownership; cross-session resolve names the owner; owned resolve stays quiet). Suite 1072 green across 65 files, typecheck and build clean. Refs GIT-69, GIT-72, c1b65b66 Co-Authored-By: Claude Opus 5 --- src/services/thread-supabase.ts | 51 +++++++++++++++++ src/tools/resolve-thread.ts | 75 ++++++++++++++++++++---- tests/unit/tools/resolve-thread.test.ts | 76 +++++++++++++++++++++++++ 3 files changed, 191 insertions(+), 11 deletions(-) diff --git a/src/services/thread-supabase.ts b/src/services/thread-supabase.ts index 02ab5d5..88d0da3 100644 --- a/src/services/thread-supabase.ts +++ b/src/services/thread-supabase.ts @@ -63,6 +63,14 @@ export type { ThreadScopeCounts } from "./thread-scope.js"; * discard local state (GIT-69 item 3). `skipped` means Supabase is not this * tier's SOT — nothing to sync, which is success, not failure. */ +/** Ownership facts about a thread, used to explain refusals (GIT-69 item 4). */ +export interface ThreadOwnership { + thread_id: string; + project: string; + source_session: string | null; + status: string; +} + export interface ThreadSyncResult { attempted: number; synced: string[]; @@ -252,6 +260,49 @@ export async function resolveThreadInSupabase( * * Returns the mapped ThreadObject, or null if not found / Supabase unavailable. */ +/** + * Who owns a thread, regardless of the caller's scope (GIT-69 item 4). + * + * Deliberately UNSCOPED — this is the one lookup allowed to see across + * projects, because its purpose is to explain a refusal rather than to grant + * access. Per R9c: a miss must return ownership information where it exists + * ("owned by session Y in project P") rather than a bare not-found, and a + * cross-project resolve must be refused by name rather than silently missing. + * + * Callers must never use this to widen what they resolve — only to say why + * they did not. + */ +export async function getThreadOwnership( + threadId: string +): Promise { + if (!hasSupabase() || !supabase.isConfigured()) { + return null; + } + + try { + const rows = await supabase.directQuery(getTableName("threads"), { + select: "thread_id,project,source_session,status", + filters: { thread_id: threadId }, + limit: 1, + }); + + if (rows.length === 0) return null; + + return { + thread_id: rows[0].thread_id, + project: rows[0].project, + source_session: rows[0].source_session, + status: rows[0].status, + }; + } catch (error) { + console.error( + "[thread-supabase] Ownership lookup failed:", + error instanceof Error ? error.message : error + ); + return null; + } +} + export async function getThreadFromSupabaseById( threadId: string ): Promise { diff --git a/src/tools/resolve-thread.ts b/src/tools/resolve-thread.ts index dac1cc3..6e0e0fb 100644 --- a/src/tools/resolve-thread.ts +++ b/src/tools/resolve-thread.ts @@ -28,6 +28,13 @@ import { listThreadsFromSupabase, } from "../services/thread-supabase.js"; import { resolveThreadScope } from "../services/thread-scope.js"; +import { getThreadOwnership } from "../services/thread-supabase.js"; +import type { ThreadOwnership } from "../services/thread-supabase.js"; + +/** Short form for log/response readability; "unknown" when unrecorded. */ +function shortId(id: string | null | undefined): string { + return id ? id.slice(0, 8) : "unknown"; +} import { writeTriplesForThreadResolution } from "../services/triple-writer.js"; import { getEffectTracker } from "../services/effect-tracker.js"; import { getAgentIdentity } from "../services/agent-detection.js"; @@ -105,28 +112,60 @@ export async function resolveThread( // Supabase, splice it into the working set, and retry. The downstream flow // (local cache write + Supabase resolve + metrics/triples) then proceeds // unchanged, syncing the previously-diverged local cache back to the SOT. + // GIT-69 item 4 v1 (R9c): resolve is project-scoped, not own-session-scoped. + // Resolve is an explicit, human-directed act on a NAMED thread — the caller + // states a specific target — whereas dedup is automatic and silent over a + // candidate set. R1's own-session caution therefore does not transfer here. + // Cross-PROJECT resolve stays refused, and the widened power is paid for with + // an audit trail: any resolve of a thread this session does not own names the + // owning session. + const scope = resolveThreadScope(); + let ownership: ThreadOwnership | null = null; + if (!resolved) { - const hydrated = await hydrateThreadFromSupabase(effectiveThreadId, effectiveTextMatch); - if (hydrated) { - if (!findThreadById(threads, hydrated.id)) { - threads.push(hydrated); + // Ownership is checked BEFORE hydrating by ID: getThreadFromSupabaseById + // filters on thread_id alone with no project filter, so hydrating first + // would resolve across projects — condition (1) violated silently. + ownership = effectiveThreadId + ? await getThreadOwnership(effectiveThreadId) + : null; + + const crossProject = ownership !== null && ownership.project !== scope.project; + + if (!crossProject) { + const hydrated = await hydrateThreadFromSupabase(effectiveThreadId, effectiveTextMatch); + if (hydrated) { + if (!findThreadById(threads, hydrated.id)) { + threads.push(hydrated); + } + resolved = resolveThreadInList(threads, { + threadId: hydrated.id, + sessionId, + resolutionNote: params.resolution_note, + }); } - resolved = resolveThreadInList(threads, { - threadId: hydrated.id, - sessionId, - resolutionNote: params.resolution_note, - }); } } if (!resolved) { const latencyMs = timer.stop(); const searchKey = params.thread_id || params.text_match; + + // R9c(3): a miss returns ownership information where it exists, rather + // than a bare not-found. "Not found" for a thread that demonstrably exists + // in another project is the same lie as a success for a write that didn't + // land — it reports absence when the truth is refusal. + const message = ownership + ? ownership.project !== scope.project + ? `Refused: thread ${ownership.thread_id} belongs to project "${ownership.project}", not "${scope.project}". Cross-project resolve is not permitted.` + : `Thread not found in scope: "${searchKey}" (exists in project "${ownership.project}", owned by session ${shortId(ownership.source_session)}, status ${ownership.status})` + : `Thread not found: "${searchKey}"`; + return { success: false, - error: `Thread not found: "${searchKey}"`, + error: message, performance: buildPerformanceData("resolve_thread", latencyMs, 0), - display: wrapDisplay(`Thread not found: "${searchKey}"`), + display: wrapDisplay(message), }; } @@ -220,6 +259,20 @@ export async function resolveThread( } let resolveMsg = `Thread resolved: "${resolved.text?.slice(0, 60) || resolved.id}"`; + + // R9c(2): the audit trail is the price of the widened power. Resolving a + // thread this session did not create is now permitted inside the project, so + // every such resolve states who owned it and who closed it. A silent + // cross-session write is the thing the project-wide scope would otherwise + // have bought us. + const owner = resolved.source_session ?? ownership?.source_session ?? null; + if (owner && sessionId && owner !== sessionId) { + resolveMsg += `\nCross-session resolve: ${resolved.id} owned by session ${shortId(owner)}, resolved by session ${shortId(sessionId)} (project ${scope.project})`; + console.error( + `[resolve_thread] Cross-session resolve: ${resolved.id} owned by ${owner}, resolved by ${sessionId} in project ${scope.project}` + ); + } + if (alsoResolved.length > 0) resolveMsg += `\nAlso resolved: ${alsoResolved.map(t => t.id).join(", ")}`; return { diff --git a/tests/unit/tools/resolve-thread.test.ts b/tests/unit/tools/resolve-thread.test.ts index 0351308..d4ae590 100644 --- a/tests/unit/tools/resolve-thread.test.ts +++ b/tests/unit/tools/resolve-thread.test.ts @@ -23,6 +23,10 @@ vi.mock("../../../src/services/thread-supabase.js", () => ({ // tests override these per-case. getThreadFromSupabaseById: vi.fn(() => Promise.resolve(null)), listThreadsFromSupabase: vi.fn(() => Promise.resolve(null)), + // GIT-69 item 4: ownership lookup used to explain refusals. Defaults to + // "no ownership record", which keeps the bare not-found path for existing + // tests; the scoping tests override it per-case. + getThreadOwnership: vi.fn(() => Promise.resolve(null)), })); vi.mock("../../../src/services/triple-writer.js", () => ({ @@ -66,6 +70,7 @@ vi.mock("../../../src/services/thread-manager.js", async (importOriginal) => { import { getThreads } from "../../../src/services/session-state.js"; import { resolveThreadInSupabase, + getThreadOwnership, getThreadFromSupabaseById, listThreadsFromSupabase, } from "../../../src/services/thread-supabase.js"; @@ -262,6 +267,77 @@ describe("resolve_thread — cross-session resolution (GIT-46)", () => { expect(vi.mocked(resolveThreadInSupabase).mock.calls[0][0]).toBe("t-cafe0001"); }); + // ---- GIT-69 item 4 v1: project scope + audit trail (R9c) ---- + + it("refuses a cross-project resolve and names the project", async () => { + // Condition (1). getThreadFromSupabaseById filters on thread_id alone with + // no project filter, so without this gate a caller in project A resolves + // project B's thread outright. + vi.mocked(getThreadOwnership).mockResolvedValue({ + thread_id: "t-cafe0001", + project: "weekend_warrior", + source_session: "other-session", + status: "open", + }); + vi.mocked(getThreadFromSupabaseById).mockResolvedValue({ ...remoteThread }); + + const result = await resolveThread({ thread_id: "t-cafe0001" }); + + expect(result.success).toBe(false); + expect(result.error).toContain("weekend_warrior"); + expect(result.error).toContain("Cross-project resolve is not permitted"); + // The refusal must be a refusal, not a silent no-op that still wrote. + expect(resolveThreadInSupabase).not.toHaveBeenCalled(); + }); + + it("returns ownership information on a miss rather than a bare not-found", async () => { + // Condition (3). "Not found" for a thread that demonstrably exists reports + // absence when the truth is that it wasn't reachable in this scope. + vi.mocked(getThreadOwnership).mockResolvedValue({ + thread_id: "t-cafe0001", + project: "default", + source_session: "abcdef12-3456-7890-abcd-ef1234567890", + status: "open", + }); + vi.mocked(getThreadFromSupabaseById).mockResolvedValue(null); + + const result = await resolveThread({ thread_id: "t-cafe0001" }); + + expect(result.success).toBe(false); + expect(result.error).toContain("abcdef12"); + expect(result.error).toContain("owned by session"); + expect(result.error).not.toBe('Thread not found: "t-cafe0001"'); + }); + + it("names the owning session when resolving a thread this session does not own", async () => { + // Condition (2). The audit trail is the price of the widened power — a + // silent cross-session write is what project scope would otherwise buy. + vi.mocked(getThreadFromSupabaseById).mockResolvedValue({ + ...remoteThread, + source_session: "99887766-5544-3322-1100-aabbccddeeff", + }); + + const result = await resolveThread({ thread_id: "t-cafe0001" }); + + expect(result.success).toBe(true); + expect(result.display).toContain("Cross-session resolve"); + expect(result.display).toContain("99887766"); + }); + + it("stays quiet when the caller owns the thread", async () => { + // No audit line for the ordinary case — an always-on notice is noise, and + // noise is how a real signal gets ignored. + vi.mocked(getThreadFromSupabaseById).mockResolvedValue({ + ...remoteThread, + source_session: "test-session", + }); + + const result = await resolveThread({ thread_id: "t-cafe0001" }); + + expect(result.success).toBe(true); + expect(result.display).not.toContain("Cross-session resolve"); + }); + it("case 3: resolves a cross-session thread by text_match too", async () => { vi.mocked(listThreadsFromSupabase).mockResolvedValue([{ ...remoteThread }]); From 436c7ff8d43bb8f072761f1fa5cf0504b4d19374 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:16:41 -0400 Subject: [PATCH 08/10] test: outage-injection harness + failing GIT-67/63 repro (P0 groundwork) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The harness for the sprint's P0, built first so the fix has an honest way to be verified. A stdio MCP harness already existed (tests/e2e/mcp-client.ts, booting local dist/ headless with env overrides) — this extends it rather than adding a parallel one. Added: createOutageEnv() pro tier with SUPABASE_URL at a closed port. The f9fa0461 condition reproduced honestly — healthy session, dead SOT — through the real write path, no mocking. Port 1 refuses instantly, so failures are immediate rather than 30s timeouts. restartServer() close and respawn preserving GITMEM_DIR/HOME. Files persist, process does not — the state session-identity loss actually occurs in. queryThreadRow() direct REST read, bypassing every gitmem code path. THROWS on an unreachable store rather than returning null: "cannot reach" must never read as "row absent", or the harness reproduces the fail-open bug it exists to catch. hasLiveSupabase() so row-level tests skip loudly instead of vacuously. Repro (tests/e2e/git-67-fail-open.test.ts) — 4 of 5 assertions FAIL against this build, which is the point. A repro that passes before the fix is not a repro. Confirmed defect shape, captured from the live probe: Thread created: "..." ID: t-02972e09 · 1 open threads on a pro tier whose durable store is unreachable. Bare success, minted ID, no caveat, no error. Two lessons baked into the file as comments: 1. An early assertion keyed on "Thread created" PASSED while the defect was live, because the response had taken the dedup shape instead. Assertions are now shape-independent: no ID may reach the caller without a statement about durability, whatever phrasing wraps it. 2. An early assertion for "admits non-durability" PASSED because the test's own thread text contained the word "durable" and the tool echoes submitted text back. Render-from-submission corrupted the test written to catch render-from-submission. All thread text is now sentinel-free. The surviving passing assertion is documented as weak — it is satisfied by the enforcement banner alone while an ID is minted beside it. No source changes. Fix follows. Refs GIT-67, GIT-63, GIT-45, f9fa0461 Co-Authored-By: Claude Opus 5 --- tests/e2e/git-67-fail-open.test.ts | 181 +++++++++++++++++++++++++++++ tests/e2e/mcp-client.ts | 87 ++++++++++++++ tests/smoke/helpers.ts | 4 + 3 files changed, 272 insertions(+) create mode 100644 tests/e2e/git-67-fail-open.test.ts diff --git a/tests/e2e/git-67-fail-open.test.ts b/tests/e2e/git-67-fail-open.test.ts new file mode 100644 index 0000000..5a7cfb3 --- /dev/null +++ b/tests/e2e/git-67-fail-open.test.ts @@ -0,0 +1,181 @@ +/** + * GIT-67 / GIT-63 — fail-open write repro (E2E, through the MCP protocol) + * + * These run against the LOCAL build over stdio, not against the published + * package. That distinction is load-bearing: the sprint's own MCP tooling + * serves gitmem-mcp 1.6.6 from npm, so verifying the fix through it would + * green-light unfixed code. + * + * Two conditions, both from community feedback f9fa0461: + * + * 1. No active session. Every write tool must refuse with no ID. An ID that + * corresponds to no stored row is worse than an error, because the caller + * records it and moves on. + * + * 2. Healthy session, dead durable store. This is the condition the original + * report most likely hit — it needs no restart, and session loss and store + * failure are indistinguishable from outside. The response must be + * unmistakably non-success (R4: local_only, durable:false), never a bare + * success string with a minted ID. + * + * Written to fail against the pre-fix build. A repro that passes before the + * fix is not a repro. + */ + +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { + createMcpClient, + callTool, + getToolResultText, + createOutageEnv, + type McpTestClient, +} from "./mcp-client.js"; +import { mkdirSync, rmSync, existsSync } from "fs"; +import { join } from "path"; +import { tmpdir } from "os"; + +/** A minted thread id: "t-" + 8 hex. The thing that must not appear. */ +const THREAD_ID_PATTERN = /\bt-[0-9a-f]{8}\b/; + +/** + * Thread text for these tests must contain NONE of the words the assertions + * look for. + * + * The tools echo submitted text back in their response, so an assertion keyed + * on a word the submission can contain matches the input rather than the + * behaviour. An earlier draft used "…the durable store unreachable" as the + * thread text and its own "admits non-durability" check passed on that echo, + * while the defect was live and a bare ID was being minted. + * + * That is render-from-submission corrupting the test written to catch + * render-from-submission. Neutral, distinctive, sentinel-free strings only — + * and distinct enough from each other not to trip semantic dedup. + */ +const SAFE_TEXT = { + noSessionId: "Harness case alpha — quiet river marmalade", + noSessionLegible: "Harness case bravo — copper lantern zither", + dualMessage: "Harness case charlie — velvet compass gherkin", + deadSotId: "Harness case delta — brass otter samovar", + deadSotNaming: "Harness case echo — plum trellis oscilloscope", +} as const; + +const TEST_DIR = join(tmpdir(), `gitmem-git67-${process.pid}`); + +function freshDir(name: string): string { + const dir = join(TEST_DIR, name); + if (existsSync(dir)) rmSync(dir, { recursive: true, force: true }); + mkdirSync(dir, { recursive: true }); + return dir; +} + +afterAll(() => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true, force: true }); +}); + +describe("GIT-67: no active session", () => { + let mcp: McpTestClient; + + beforeAll(async () => { + // A pristine HOME/GITMEM_DIR means there is no session file to recover + // from, so "no active session" is unambiguous regardless of what the + // recovery path does. + const dir = freshDir("no-session"); + mcp = await createMcpClient({ + ...createOutageEnv(), + GITMEM_DIR: dir, + HOME: dir, + }); + }); + + afterAll(async () => { + await mcp?.cleanup(); + }); + + it("create_thread does not mint a thread ID when there is no session", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.noSessionId, + }); + const text = getToolResultText(result); + + // The core assertion. An ID in this output is a promise of durability the + // tool cannot keep. + expect(text).not.toMatch(THREAD_ID_PATTERN); + }); + + it("create_thread says plainly that nothing was stored", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.noSessionLegible, + }); + const text = getToolResultText(result).toLowerCase(); + + // Note: on the pre-fix build this passes on the enforcement banner alone, + // while an ID is minted alongside it. It is kept because the refusal must + // remain legible after the fix, but it is NOT the assertion that catches + // the defect — the ID and dual-message tests are. A test that can be + // satisfied by a banner is not evidence the write refused. + expect(text).toMatch(/no active session|not stored|stored: ?false|no_active_session/); + }); + + it("never emits a success payload and a failure banner in one response", async () => { + // The reported symptom: "Thread created ... ID: t-xxxxxxxx" carrying + // "--- gitmem enforcement --- No active session." in the same string. + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.dualMessage, + }); + const text = getToolResultText(result); + + const claimsSuccess = /thread created/i.test(text); + const admitsFailure = /no active session/i.test(text); + + expect(claimsSuccess && admitsFailure).toBe(false); + }); +}); + +describe("GIT-67 / R4: healthy session, unreachable durable store", () => { + let mcp: McpTestClient; + + beforeAll(async () => { + const dir = freshDir("dead-sot"); + mcp = await createMcpClient({ + ...createOutageEnv(), + GITMEM_DIR: dir, + HOME: dir, + }); + + // A genuinely healthy session. Only the store is dead. + await callTool(mcp.client, "session_start", { + agent_identity: "cli", + project: "gitmem", + }); + }); + + afterAll(async () => { + await mcp?.cleanup(); + }); + + it("does not surface a thread ID without a durability caveat", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.deadSotId, + }); + const text = getToolResultText(result); + + // Deliberately shape-independent. An earlier version of this assertion + // keyed on "Thread created" and passed while the defect was live, because + // the response had taken the dedup shape ("Dedup: matched existing thread + // … ID: t-xxxxxxxx") instead. The invariant is not about a phrase — it is + // that no ID reaches the caller without a statement about durability. + const presentsId = THREAD_ID_PATTERN.test(text); + const admitsNonDurability = /durable|local[_ ]only|not stored|failed|unavailable/i.test(text); + + expect(presentsId && !admitsNonDurability).toBe(false); + }); + + it("names the store and its non-durability (R3 + R4)", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.deadSotNaming, + }); + const text = getToolResultText(result).toLowerCase(); + + expect(text).toMatch(/durable|local[_ ]only|not stored/); + }); +}); diff --git a/tests/e2e/mcp-client.ts b/tests/e2e/mcp-client.ts index 5a97e38..be92f6c 100644 --- a/tests/e2e/mcp-client.ts +++ b/tests/e2e/mcp-client.ts @@ -226,6 +226,93 @@ export function createTierEnv(tier: "free" | "pro" | "dev"): Record = {} +): Record { + return { + GITMEM_TIER: "pro", + SUPABASE_URL: "http://127.0.0.1:1", + SUPABASE_SERVICE_ROLE_KEY: "outage-harness-not-a-real-key", + GITMEM_TABLE_PREFIX: "orchestra_", + ...overrides, + }; +} + +/** + * Restart the MCP server while on-disk session state survives (GIT-51/GIT-67). + * + * The server is a separate long-lived process holding session identity in + * memory; restarting it is what orphans that identity. Reusing the same + * GITMEM_DIR/HOME is the whole point — the files persist, the process does + * not, which is precisely the state the reported defect occurs in. + * + * Returns a fresh client. The caller must use it; the old one is closed. + */ +export async function restartServer( + mcp: McpTestClient, + env: Record = {}, + options: { cwd?: string } = {} +): Promise { + await mcp.cleanup(); + return createMcpClient(env, options); +} + +/** + * Read a thread row straight from Supabase, bypassing every gitmem code path. + * + * The acceptance standard for this sprint is the row, not the tool message, so + * assertions need a channel the code under test cannot influence. Returns null + * when the row is absent, and throws when the store is unreachable — an + * unreachable store must never read as "row absent", or the harness reproduces + * the fail-open bug it exists to catch. + */ +export async function queryThreadRow( + threadId: string, + config: { url: string; key: string; tablePrefix?: string } +): Promise | null> { + const table = `${config.tablePrefix ?? "orchestra_"}threads`; + const endpoint = `${config.url}/rest/v1/${table}?thread_id=eq.${encodeURIComponent(threadId)}&select=*`; + + 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; +} + +/** + * Whether real Supabase credentials are present. + * + * Row-level assertions need a live store. Tests gate on this and skip loudly + * rather than passing vacuously — a suite that silently skips its only + * real assertion is the same lie as a green message over an unwritten row. + */ +export function hasLiveSupabase(): boolean { + return Boolean(process.env.SUPABASE_URL && process.env.SUPABASE_SERVICE_ROLE_KEY); +} + /** * Core tools available in all tiers */ diff --git a/tests/smoke/helpers.ts b/tests/smoke/helpers.ts index c7389cd..5cef3a1 100644 --- a/tests/smoke/helpers.ts +++ b/tests/smoke/helpers.ts @@ -15,6 +15,10 @@ export { isToolError, waitFor, createTierEnv, + createOutageEnv, + restartServer, + queryThreadRow, + hasLiveSupabase, CORE_TOOLS, PRO_TOOLS, EXPECTED_TOOL_COUNTS, From c0b577e2fea39ed96bd7a0d1ceb09b7b42f7b22e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:22:49 -0400 Subject: [PATCH 09/10] fix: render-from-stored-row across create_thread (GIT-67 + GIT-63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sprint's P0. Community feedback f9fa0461: create_thread returned a plausible t-xxxxxxxx for a row that never existed. Verified fixed against the harness from 436c7ff, which failed 4 of 5 assertions before this commit and passes 11 of 11 after. R5 — no active session, tier-relative. Pro refuses outright with stored:false, reason:"no_active_session", and NO id. An invented ID is strictly worse than an error: the caller records it and moves on. Free tier still writes, because there the local file IS the SOT and sessionless use is legitimate — refusing would fail-close every free user. Its response says "session: none" rather than implying an ownership it lacks. R2 — dedup refuses; it neither merges nor discards. The old path returned the matched thread's ID with the SUBMITTED text rendered beside it and success:true, while the stored row kept its own text and only updated_at moved. Now: stored:false, reason:"duplicate_candidate", the matched row's CURRENT text read back from the store, and both lengths so a discard is arithmetically visible. Two explicit ways through — resolve-then-create, or allow_duplicate:true. Critically there is no `thread` field on this response: returning a ThreadObject is exactly how the old code handed back another thread's ID as though it were the caller's. R4 — fail-honest on store failure. A Supabase write that does not land no longer falls through to a success with a minted ID. The local buffer survives (destroying data to signal an error repeats the crime in reverse), but the response is unmistakably non-success: stored_in:"local_only", durable:false, supabase_error, and prose saying it will not reach another session. R3 — every response names its store (stored_in: supabase | local | local_only), which also permanently closes the f104e10d silent-local trap: a write that quietly went local can no longer look identical to one that reached Supabase. The convention underneath all four: the response is rendered from a post-write READ of the stored row, never from the submitted payload. An ID appears in output only if the row backing it exists. Where neither store has the row, nothing is claimed at all. Tests: +6 e2e (dedup refusal states not-stored, shows both lengths, offers a way through, allow_duplicate works; free tier stores and names session:none). Suite 1072 unit + 11 e2e green, typecheck and build clean. One spec tension flagged to GIT-67, not resolved here: on free tier with no session the enforcement banner and a success payload still coexist, which GIT-67 criterion 3 words as "never both". They no longer contradict — the payload says session:none and the banner says the same — but the literal criterion is unmet and the wording needs a ruling. Refs GIT-67, GIT-63, f9fa0461, e39ec941 Co-Authored-By: Claude Opus 5 --- src/tools/create-thread.ts | 227 +++++++++++++++++++++++++---- tests/e2e/git-67-fail-open.test.ts | 113 ++++++++++++++ tests/e2e/mcp-client.ts | 16 +- 3 files changed, 320 insertions(+), 36 deletions(-) diff --git a/src/tools/create-thread.ts b/src/tools/create-thread.ts index a804f5d..691de05 100644 --- a/src/tools/create-thread.ts +++ b/src/tools/create-thread.ts @@ -15,7 +15,7 @@ */ import { v4 as uuidv4 } from "uuid"; -import { getTableName } from "../services/tier.js"; +import { getTableName, getTier, hasSupabase } from "../services/tier.js"; import { getThreads, setThreads, getCurrentSession, getProject } from "../services/session-state.js"; import { generateThreadId, @@ -25,7 +25,7 @@ import { import { createThreadInSupabase, loadOpenThreadEmbeddings, - touchThreadsInSupabase, + getThreadFromSupabaseById, } from "../services/thread-supabase.js"; import { checkDuplicate } from "../services/thread-dedup.js"; import { resolveThreadScope } from "../services/thread-scope.js"; @@ -52,14 +52,48 @@ export interface CreateThreadParams { linear_issue?: string; /** Project namespace (default: default) */ project?: Project; + /** + * R2: force a genuinely new thread past a dedup match. Dedup refuses rather + * than merging, so this is the caller's explicit way through — never a + * default, because the whole point is that the decision is stated. + */ + allow_duplicate?: boolean; } +/** + * Why a write did not store, when it did not (GIT-67/GIT-63). + * Absent on success. + */ +export type NotStoredReason = + | "no_active_session" + | "duplicate_candidate" + | "empty_text" + | "store_unavailable"; + +/** + * Where a thread actually landed (R3: every response names its store). + * `local_only` is the honest middle state from R4 — written somewhere, but + * not somewhere durable. + */ +export type StoredIn = "supabase" | "local" | "local_only" | null; + export interface CreateThreadResult { success: boolean; + /** + * Whether a row exists. Distinct from `success` on purpose: a dedup refusal + * is a well-formed answer to a well-formed request, not a crash. + */ + stored: boolean; + /** Whether what was stored survives this machine. */ + durable: boolean; + /** R3: names the store in-band, which also kills the f104e10d silent-local trap. */ + stored_in: StoredIn; + reason?: NotStoredReason; thread?: ThreadObject; error?: string; total_open: number; supabase_synced: boolean; + supabase_error?: string; performance: PerformanceData; /** Phase 3: true when dedup gate found an existing duplicate */ deduplicated?: boolean; @@ -68,6 +102,11 @@ export interface CreateThreadResult { method: "embedding" | "token_overlap" | "text_normalization" | "skipped"; similarity: number | null; matched_thread_id: string | null; + /** R2: the STORED text of the match, so the caller sees what it collided with. */ + matched_text?: string; + /** R2: both lengths, so a silent discard is arithmetically visible. */ + stored_length?: number; + submitted_length?: number; }; display?: string; } @@ -84,11 +123,15 @@ export async function createThread( const latencyMs = timer.stop(); return { success: false, + stored: false, + durable: false, + stored_in: null, + reason: "empty_text", error: "Thread text is required", total_open: 0, supabase_synced: false, performance: buildPerformanceData("create_thread" as any, latencyMs, 0), - display: wrapDisplay(`Failed: thread text is required`), + display: wrapDisplay(`Not stored: thread text is required`), }; } @@ -97,6 +140,38 @@ export async function createThread( const project = params.project || getProject() || "default"; const trimmedText = params.text.trim(); + // GIT-67 / R5: fail loud with no ID when there is no session to own the + // write. The reported defect was a success payload carrying a plausible + // t-xxxxxxxx for a row that never existed — the caller records that ID and + // moves on, so an invented ID is strictly worse than an error. + // + // Tier-relative per R5: on pro the write is refused outright, because the + // durable store records source_session and an unowned row is a defect. On + // free the local file is the SOT and sessionless use is legitimate, so the + // write proceeds and the response says `session: none` rather than implying + // an ownership it does not have. + const tier = getTier(); + if (!sessionId && tier !== "free") { + const latencyMs = timer.stop(); + const message = + "Not stored: no active session (reason: no_active_session). " + + "No thread ID has been issued because no row was written. " + + "Call session_start() first, then retry — or fall back to another record (e.g. the issue tracker) if the session cannot be restored."; + + return { + success: false, + stored: false, + durable: false, + stored_in: null, + reason: "no_active_session", + error: message, + total_open: 0, + supabase_synced: false, + performance: buildPerformanceData("create_thread" as any, latencyMs, 0), + display: wrapDisplay(message), + }; + } + // Phase 3: Generate embedding for new text (best-effort) let newEmbedding: number[] | null = null; if (isEmbeddingAvailable()) { @@ -132,30 +207,33 @@ export async function createThread( // Phase 3: Run dedup check const dedupResult = checkDuplicate(trimmedText, newEmbedding, existingThreads); - // If duplicate found, touch existing thread and return it - if (dedupResult.is_duplicate && dedupResult.matched_thread_id) { - // Touch the existing thread to keep it vital - await touchThreadsInSupabase([dedupResult.matched_thread_id]); + // GIT-63 / R2: a dedup match REFUSES. It does not merge, and it does not + // discard. + // + // The old behaviour returned the matched thread's ID with the SUBMITTED text + // rendered beside it and success:true, while the stored row kept its own + // text and only updated_at moved. The caller believed a handoff was written; + // the database held the previous session's. Nothing anywhere said the + // content had been dropped. + // + // Everything below renders from the STORED row. The submitted text appears + // only as a length, never as the thing that got an ID. + if (dedupResult.is_duplicate && dedupResult.matched_thread_id && !params.allow_duplicate) { + const matchedId = dedupResult.matched_thread_id; + + // Read the stored row back rather than trusting the dedup candidate cache + // — the refusal quotes the database, not our copy of it. + const storedRow = await getThreadFromSupabaseById(matchedId); + const storedText = storedRow?.text ?? dedupResult.matched_text ?? ""; const fileThreads = loadThreadsFile(); const totalOpen = fileThreads.filter((t) => t.status === "open").length; - - // Find the existing thread to return - const existingThread: ThreadObject = fileThreads.find( - (t) => t.id === dedupResult.matched_thread_id - ) || { - id: dedupResult.matched_thread_id, - text: dedupResult.matched_text || trimmedText, - status: "open", - created_at: new Date().toISOString(), - }; - const latencyMs = timer.stop(); recordMetrics({ id: metricsId, tool_name: "create_thread" as any, - query_text: `dedup:${dedupResult.matched_thread_id}`, + query_text: `dedup_refused:${matchedId}`, tables_searched: [getTableName("threads")], latency_ms: latencyMs, result_count: 0, @@ -164,23 +242,45 @@ export async function createThread( dedup_blocked: true, dedup_method: dedupResult.method, dedup_similarity: dedupResult.similarity, - matched_thread_id: dedupResult.matched_thread_id, + matched_thread_id: matchedId, + submitted_length: trimmedText.length, + stored_length: storedText.length, }, }).catch(() => {}); + const message = + `NOT STORED: your text was not written (reason: duplicate_candidate).\n` + + `It resembles an existing thread in this session${dedupResult.similarity !== null ? ` (similarity ${dedupResult.similarity})` : ""}.\n\n` + + `Existing thread ${matchedId} — ${storedText.length} chars, unchanged:\n` + + ` "${truncate(storedText, 120)}"\n\n` + + `Your submission — ${trimmedText.length} chars, NOT persisted anywhere:\n` + + ` "${truncate(trimmedText, 120)}"\n\n` + + `To supersede: resolve_thread("${matchedId}") then create_thread again.\n` + + `To keep both: create_thread with allow_duplicate: true.`; + return { - success: true, - thread: formatThreadForDisplay(existingThread), + success: false, + stored: false, + durable: false, + stored_in: null, + reason: "duplicate_candidate", + // Deliberately NO `thread` field. Returning a ThreadObject here is how + // the old code handed back another thread's ID as if it were the + // caller's; there is no thread to return, because nothing was created. total_open: totalOpen, - supabase_synced: true, + supabase_synced: false, performance: buildPerformanceData("create_thread" as any, latencyMs, 0), deduplicated: true, dedup: { method: dedupResult.method, similarity: dedupResult.similarity, - matched_thread_id: dedupResult.matched_thread_id, + matched_thread_id: matchedId, + matched_text: storedText, + stored_length: storedText.length, + submitted_length: trimmedText.length, }, - display: wrapDisplay(`Dedup: matched existing thread "${truncate(trimmedText, 60)}"\nID: ${dedupResult.matched_thread_id}`), + error: message, + display: wrapDisplay(message), }; } @@ -193,12 +293,21 @@ export async function createThread( ...(sessionId && { source_session: sessionId }), }; - // Write to Supabase (source of truth) with embedding — non-blocking on failure + // Write to Supabase (source of truth) with embedding. let supabaseSynced = false; + let supabaseError: string | undefined; const embeddingJson = newEmbedding ? JSON.stringify(newEmbedding) : null; - const supabaseResult = await createThreadInSupabase(thread, project, embeddingJson); - if (supabaseResult) { - supabaseSynced = true; + + if (hasSupabase()) { + try { + const supabaseResult = await createThreadInSupabase(thread, project, embeddingJson); + supabaseSynced = Boolean(supabaseResult); + if (!supabaseSynced) { + supabaseError = "write returned no row"; + } + } catch (err) { + supabaseError = err instanceof Error ? err.message : String(err); + } } // Update in-memory session state if active @@ -246,11 +355,65 @@ export async function createThread( }) ); + // GIT-63/GIT-67 core convention: render from a post-write READ of the stored + // row, never from the submitted payload. An ID appears in output only if the + // row backing it exists. + // + // Tier-relative per R3: pro's SOT is the Supabase row, free's is the local + // record. Each reads back its own store; neither claims the other's. + const storedRow = supabaseSynced ? await getThreadFromSupabaseById(thread.id) : null; + const localRecord = loadThreadsFile().find((t) => t.id === thread.id) ?? null; + const rendered = storedRow ?? localRecord; + + const durable = supabaseSynced && storedRow !== null; + const storedIn: StoredIn = durable + ? "supabase" + : hasSupabase() + ? "local_only" + : "local"; + + if (!rendered) { + // Neither store has it. Nothing to render from, so nothing is claimed. + const message = + `NOT STORED: the write could not be confirmed in any store (reason: store_unavailable).` + + (supabaseError ? `\nDurable store: ${supabaseError}` : ""); + return { + success: false, + stored: false, + durable: false, + stored_in: null, + reason: "store_unavailable", + error: message, + total_open: totalOpen, + supabase_synced: false, + supabase_error: supabaseError, + performance: perfData, + display: wrapDisplay(message), + }; + } + + // R4: fail-honest. The local buffer survives — destroying it to signal an + // error would repeat the crime in reverse — but the response is + // unmistakably non-success, and no ID is presented as durable. + const message = durable + ? `Thread created: "${truncate(rendered.text, 60)}"\nID: ${rendered.id} · ${totalOpen} open threads · stored in supabase` + : hasSupabase() + ? `SAVED LOCALLY ONLY — not durable (stored_in: local_only, durable: false).\n` + + `Thread ${rendered.id}: "${truncate(rendered.text, 60)}"\n` + + `The durable store did not accept this write${supabaseError ? ` (${supabaseError})` : ""}, so it exists only on this machine and will not reach another session.\n` + + `Retry when the store is reachable, or record it somewhere durable.` + : `Thread created: "${truncate(rendered.text, 60)}"\nID: ${rendered.id} · ${totalOpen} open threads · stored in local file` + + (sessionId ? "" : " · session: none"); + return { - success: true, - thread: formatThreadForDisplay(thread), + success: durable || !hasSupabase(), + stored: true, + durable, + stored_in: storedIn, + thread: formatThreadForDisplay(rendered), total_open: totalOpen, supabase_synced: supabaseSynced, + ...(supabaseError ? { supabase_error: supabaseError } : {}), performance: perfData, deduplicated: false, dedup: { @@ -258,6 +421,6 @@ export async function createThread( similarity: dedupResult.similarity, matched_thread_id: null, }, - display: wrapDisplay(`Thread created: "${truncate(trimmedText, 60)}"\nID: ${thread.id} · ${totalOpen} open threads`), + display: wrapDisplay(message), }; } diff --git a/tests/e2e/git-67-fail-open.test.ts b/tests/e2e/git-67-fail-open.test.ts index 5a7cfb3..bcc0d14 100644 --- a/tests/e2e/git-67-fail-open.test.ts +++ b/tests/e2e/git-67-fail-open.test.ts @@ -57,6 +57,11 @@ const SAFE_TEXT = { dualMessage: "Harness case charlie — velvet compass gherkin", deadSotId: "Harness case delta — brass otter samovar", deadSotNaming: "Harness case echo — plum trellis oscilloscope", + dedupSeed: "Harness case foxtrot — juniper anvil quasar", + dedupLonger: + "Harness case foxtrot — juniper anvil quasar, extended with additional wording so the two submissions differ in length while still colliding", + freeNoSession: "Harness case golf — tin sparrow obelisk", + freeNoSessionNaming: "Harness case hotel — moss lantern pendulum", } as const; const TEST_DIR = join(tmpdir(), `gitmem-git67-${process.pid}`); @@ -179,3 +184,111 @@ describe("GIT-67 / R4: healthy session, unreachable durable store", () => { expect(text).toMatch(/durable|local[_ ]only|not stored/); }); }); + +describe("GIT-63 / R2: dedup refuses rather than discarding", () => { + let mcp: McpTestClient; + + beforeAll(async () => { + const dir = freshDir("dedup"); + mcp = await createMcpClient({ + ...createOutageEnv(), + GITMEM_DIR: dir, + HOME: dir, + }); + await callTool(mcp.client, "session_start", { + agent_identity: "cli", + project: "gitmem", + }); + // Seed the thread the next call will collide with. + await callTool(mcp.client, "create_thread", { text: SAFE_TEXT.dedupSeed }); + }); + + afterAll(async () => { + await mcp?.cleanup(); + }); + + it("states that the submitted text was NOT stored", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.dedupSeed, + }); + const text = getToolResultText(result).toLowerCase(); + + // The old behaviour read as "created and merged" while the row kept its + // previous text and only updated_at moved. + expect(text).toMatch(/not stored|duplicate_candidate/); + expect(text).not.toMatch(/thread created/i); + }); + + it("shows both lengths so a discard is arithmetically visible", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.dedupLonger, + }); + const text = getToolResultText(result); + + // Two char counts must appear — the stored row's and the submission's. + const lengths = text.match(/\b\d+ chars\b/g) ?? []; + expect(lengths.length).toBeGreaterThanOrEqual(2); + }); + + it("offers an explicit way through rather than a dead end", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.dedupSeed, + }); + const text = getToolResultText(result); + + expect(text).toMatch(/allow_duplicate/); + expect(text).toMatch(/resolve_thread/); + }); + + it("allow_duplicate: true actually creates a distinct thread", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.dedupSeed, + allow_duplicate: true, + }); + const text = getToolResultText(result); + + expect(text).not.toMatch(/not stored/i); + expect(text).toMatch(THREAD_ID_PATTERN); + }); +}); + +describe("R5: free tier writes without a session, and says so", () => { + let mcp: McpTestClient; + + beforeAll(async () => { + const dir = freshDir("free-no-session"); + mcp = await createMcpClient({ + GITMEM_TIER: "free", + SUPABASE_URL: "", + SUPABASE_SERVICE_ROLE_KEY: "", + GITMEM_DIR: dir, + HOME: dir, + }); + }); + + afterAll(async () => { + await mcp?.cleanup(); + }); + + it("stores the thread — the local file IS the SOT here", async () => { + // Refusing this would fail-close every sessionless free user. R5 makes the + // guard tier-relative precisely to avoid that. + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.freeNoSession, + }); + const text = getToolResultText(result); + + expect(text).toMatch(THREAD_ID_PATTERN); + expect(text).not.toMatch(/not stored/i); + }); + + it("names the store and the absent session rather than implying ownership", async () => { + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.freeNoSessionNaming, + }); + const text = getToolResultText(result).toLowerCase(); + + expect(text).toContain("local file"); + expect(text).toContain("session: none"); + }); +}); diff --git a/tests/e2e/mcp-client.ts b/tests/e2e/mcp-client.ts index be92f6c..3f19790 100644 --- a/tests/e2e/mcp-client.ts +++ b/tests/e2e/mcp-client.ts @@ -273,10 +273,18 @@ export async function restartServer( * Read a thread row straight from Supabase, bypassing every gitmem code path. * * The acceptance standard for this sprint is the row, not the tool message, so - * assertions need a channel the code under test cannot influence. Returns null - * when the row is absent, and throws when the store is unreachable — an - * unreachable store must never read as "row absent", or the harness reproduces - * the fail-open bug it exists to catch. + * assertions need a channel the code under test cannot influence. + * + * STANDING INVARIANT — do not soften this to return null on error. + * + * Absent row → null. Unreachable store → throw. Those are different facts and + * must stay different values. Collapsing them would make every row-absence + * assertion pass whenever the store is merely down, which is the fail-open + * class rebuilt inside the instrument that exists to detect it: a test suite + * reporting "verified, no row written" when it verified nothing at all. + * + * If a caller finds the throw inconvenient, the correct fix is to handle the + * unreachable case explicitly, never to erase the distinction here. */ export async function queryThreadRow( threadId: string, From f86fe3cdff8327bb0e023802152c7ab3966e7be5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:24:53 -0400 Subject: [PATCH 10/10] test: encode amended criterion 3 as a consistency assertion (R10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Criterion 3 originally read "a response must never contain both a success payload and an enforcement failure banner." That was written against the old defect, where the two CONTRADICTED — success claimed while enforcement asserted the write's precondition was absent. The contradiction is what made the response a lie, not the co-presence. R10 amends it: a response must never simultaneously claim success and assert a condition that negates that success; banner and payload must agree on session state and storage state. This test encodes the amendment so it is mechanical rather than remembered. It asserts CONSISTENCY, not absence: - when the banner denies a session AND the payload reports a stored thread, the payload must also say session:none and name its store — either omission recreates the lie - a payload may never both present an id and assert the write did not store, in any tier Closes the coverage gap disclosed in c0b577e: the dual-message test was pro-only, so a free-tier violation would have passed unnoticed. e2e 12/12 green. Refs GIT-67 Co-Authored-By: Claude Opus 5 --- tests/e2e/git-67-fail-open.test.ts | 35 ++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/e2e/git-67-fail-open.test.ts b/tests/e2e/git-67-fail-open.test.ts index bcc0d14..b9bc828 100644 --- a/tests/e2e/git-67-fail-open.test.ts +++ b/tests/e2e/git-67-fail-open.test.ts @@ -62,6 +62,7 @@ const SAFE_TEXT = { "Harness case foxtrot — juniper anvil quasar, extended with additional wording so the two submissions differ in length while still colliding", freeNoSession: "Harness case golf — tin sparrow obelisk", freeNoSessionNaming: "Harness case hotel — moss lantern pendulum", + freeConsistency: "Harness case india — walnut beacon fulcrum", } as const; const TEST_DIR = join(tmpdir(), `gitmem-git67-${process.pid}`); @@ -291,4 +292,38 @@ describe("R5: free tier writes without a session, and says so", () => { expect(text).toContain("local file"); expect(text).toContain("session: none"); }); + + it("banner and payload AGREE — criterion 3 as amended by R10", async () => { + // The original criterion said a response must never contain both a success + // payload and an enforcement banner. That was written against the old + // defect, where the two CONTRADICTED: success claimed while enforcement + // asserted the write's precondition was absent. The contradiction is what + // made the response a lie. + // + // Amended: a response must never simultaneously claim success and assert a + // condition that negates that success. Co-presence is fine when both are + // true. So this asserts CONSISTENCY, not absence — and it is deliberately + // written to fail if a future change makes the payload claim a session + // while the banner denies one. + const result = await callTool(mcp.client, "create_thread", { + text: SAFE_TEXT.freeConsistency, + }); + const text = getToolResultText(result); + const lower = text.toLowerCase(); + + const bannerDeniesSession = /no active session/i.test(text); + const payloadClaimsStored = THREAD_ID_PATTERN.test(text) && !/not stored/i.test(text); + + if (bannerDeniesSession && payloadClaimsStored) { + // Permitted only because the payload agrees about the session and names + // where it actually landed. Either omission would recreate the lie. + expect(lower).toContain("session: none"); + expect(lower).toMatch(/local file|local_only|supabase/); + } + + // The negating combination itself: a stored claim that also asserts the + // write did not store. Must never occur in any tier. + const selfNegating = payloadClaimsStored && /not stored|no thread id has been issued/i.test(text); + expect(selfNegating).toBe(false); + }); });