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.* 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 new file mode 100644 index 0000000..30a4df2 --- /dev/null +++ b/src/services/thread-drift.ts @@ -0,0 +1,195 @@ +/** + * 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; +} + +/** + * 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; + /** 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. Not the same as `healthy`. */ + in_sync: boolean; + /** Three-valued verdict — see ThreadDriftStatus. */ + status: ThreadDriftStatus; +} + +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; + + // 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, + only_remote, + unattributable_local, + drift_count, + in_sync: drift_count === 0, + status, + }; +} + +/** + * 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.status === "healthy") { + return `threads healthy (${drift.in_sync_count} compared, all attributed)`; + } + + 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`); + } + // 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} unattributable (local cache has no project field, so these may belong to another project)` + ); + } + + return `threads ${drift.status}: ${parts.join(", ")}; ${drift.in_sync_count} verified in sync`; +} diff --git a/src/services/thread-scope.ts b/src/services/thread-scope.ts new file mode 100644 index 0000000..fa98413 --- /dev/null +++ b/src/services/thread-scope.ts @@ -0,0 +1,256 @@ +/** + * 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; + +/** + * 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; + +// --- 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; +} + +// --- 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" + * 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/src/services/thread-supabase.ts b/src/services/thread-supabase.ts index b830788..88d0da3 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,35 @@ export interface ThreadDisplayInfo { days_since_touch: number; } +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. + */ +/** 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[]; + failed: { id: string; error: string }[]; + skipped: boolean; + all_synced: boolean; +} + +export interface ActiveThreadsResult { + open: ThreadObject[]; + displayInfo: ThreadDisplayInfo[]; + scope: ThreadScopeCounts; +} + // ---------- Mapping Helpers ---------- /** @@ -225,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 { @@ -252,7 +330,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 +340,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 +389,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 +429,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 +462,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; @@ -444,11 +551,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. @@ -515,11 +636,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) ---------- @@ -603,27 +742,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..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,9 +25,10 @@ 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"; import { embed, isEmbeddingAvailable } from "../services/embedding.js"; import { writeTriplesForThreadCreation } from "../services/triple-writer.js"; import { getEffectTracker } from "../services/effect-tracker.js"; @@ -51,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; @@ -67,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; } @@ -83,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`), }; } @@ -96,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()) { @@ -106,14 +182,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, @@ -124,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, @@ -156,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), }; } @@ -185,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 @@ -238,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: { @@ -250,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/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..6e0e0fb 100644 --- a/src/tools/resolve-thread.ts +++ b/src/tools/resolve-thread.ts @@ -27,6 +27,14 @@ import { getThreadFromSupabaseById, 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"; @@ -104,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), }; } @@ -219,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 { @@ -251,8 +305,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..cb70bec 100644 --- a/src/tools/session-close.ts +++ b/src/tools/session-close.ts @@ -17,6 +17,9 @@ 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 { validateSessionClose, buildCloseCompliance, @@ -1256,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); } @@ -1375,7 +1413,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( @@ -1471,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/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/e2e/git-67-fail-open.test.ts b/tests/e2e/git-67-fail-open.test.ts new file mode 100644 index 0000000..b9bc828 --- /dev/null +++ b/tests/e2e/git-67-fail-open.test.ts @@ -0,0 +1,329 @@ +/** + * 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", + 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", + freeConsistency: "Harness case india — walnut beacon fulcrum", +} 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/); + }); +}); + +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"); + }); + + 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); + }); +}); diff --git a/tests/e2e/mcp-client.ts b/tests/e2e/mcp-client.ts index 5a97e38..3f19790 100644 --- a/tests/e2e/mcp-client.ts +++ b/tests/e2e/mcp-client.ts @@ -226,6 +226,101 @@ 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. + * + * 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, + 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, diff --git a/tests/unit/services/thread-drift.test.ts b/tests/unit/services/thread-drift.test.ts new file mode 100644 index 0000000..7ec1edb --- /dev/null +++ b/tests/unit/services/thread-drift.test.ts @@ -0,0 +1,246 @@ +/** + * 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("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 healthy (1 compared, all attributed)"); + }); + + 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); + // 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", () => { + 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"); + }); +}); diff --git a/tests/unit/services/thread-scope.test.ts b/tests/unit/services/thread-scope.test.ts new file mode 100644 index 0000000..b0618da --- /dev/null +++ b/tests/unit/services/thread-scope.test.ts @@ -0,0 +1,364 @@ +/** + * 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, + computePanelOmission, + formatOmissionLine, + 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"); + }); +}); + +// ---------- 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(); + }); +}); 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(); + } + }); +}); 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 }]);