Repository navigation
feat: single thread-scope resolver + wire all four surfaces (GIT-69 item 1) - #20
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One definition of "the thread store, scoped", per ruling R1/R6. The four
thread surfaces each computed their own view and disagreed in three ways
that were invisible from any single call site:
limit session_start 50 vs list_threads 100
status session_start excluded "dormant", list_threads did not
ordering the dedup loader had no ORDER BY at all
Those disagreements are the f02f74ca panel-vs-list defect and the
nondeterministic dedup candidate set. Both are closed here rather than in
three separate patches.
resolveThreadScope() (project, sessionId) from overrides > session > config
buildScopedThreadQuery() the one query shape: filters, order, limit
isInScope() same predicate for rows not from a scoped query
THREAD_SCOPE_ORDER total order — thread_id breaks ties, so the
candidate set is stable at the limit boundary
Dedup uses "own_session" visibility per R1: a match against another
session's thread is never a safe automatic merge. With no active session
the query is null, not project-wide — a sessionless caller must not
silently dedup against the whole namespace.
The resolver is downstream of session identity: it scopes correctly
relative to whatever (project, sessionId) it is handed. Deciding who the
caller is belongs to GIT-51. The project-disagreement fixture pins that
seam by name so the boundary is explicit rather than discovered.
Tests: +27 (parity, cross-project isolation, determinism, own-session
candidate set, status sets, recovery-seam fixture). Suite 1031 green,
typecheck clean. Contains zero lines from wip/git-51-session-recovery.
Consumer wiring (session_start / list_threads / resolve_thread / dedup)
follows in the next commit on this branch.
Refs GIT-69, GIT-63, GIT-67
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every surface that reads or resolves threads now imports one definition of "the thread store, scoped". The three disagreements are gone: session_start limit 50, excluded dormant, own query → resolver list_threads limit 100, included dormant, own query → resolver resolve_thread its own project lookup → resolver dedup no ORDER BY, project-wide candidates → resolver, own_session Per R1, dedup candidates are the caller's own session only; with no active session the query is null and dedup is skipped rather than widening to the namespace. The local-file fallback is held to the same scope, so cache and store cannot disagree. session_close's suggestion pass opts into "project" visibility explicitly — it proposes threads to a human rather than silently merging into one. That is a parameter, not a hardcoded default, so the distinction stays visible. Per R7, dormant is IN scope and filtered at display only, and the panel cap is annotation rather than scope: computePanelOmission/formatOmissionLine render "showing 5 of 47 in scope (31 dormant, 11 over cap) — list_threads for full view" whenever display < scope. A panel showing 5 of 47 with no indicator is a green-message pattern in miniature. Tests: +11 display-layer (7 reconciliation cases asserting shown + dormantHidden + overCap === totalInScope, plus disclosure and zero-term cases). Suite 1042 green, typecheck and build clean. Contains zero lines from wip/git-51-session-recovery. Refs GIT-69, GIT-63, GIT-67 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cache-health reported healthy on scar parity alone while two threads were demonstrably divergent (feedback ad5ca35a). "Healthy" asserted from one covered store is the same shape as a write reporting success from an unread row — a claim of completeness that was never checked. diffThreads() supplies the thread half as a pure, testable diff. Three buckets rather than two, because the stores are not partitioned the same way: remote rows carry a project, the local threads.json does not (ThreadObject has no project field, so one flat file holds every project's threads). A naive in-local/in-remote diff for project A would report every local project-B thread as drift. That is false drift, and a detector that cries wolf trains its reader to ignore it — which is how ad5ca35a's real divergence stayed invisible beside a healthy banner. So local-only IDs are reported as explicitly unattributable rather than folded into drift, and describeThreadDrift() always names the count even though it is not drift: an omitted number reads as zero. Tests: +13, including the equal-counts-but-divergent case a count-parity check calls healthy, and the false-drift case. Not yet wired into checkCacheHealth — one definitional question posted to GIT-69 first (whether unattributable locals degrade status). Refs GIT-69 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per R9a. cache-health reported healthy on scar parity alone while two threads were divergent (ad5ca35a). Threads are now a covered store, and "healthy" requires parity on every store checked. Status is three-valued because binary was the false choice: healthy full attribution AND no divergence — earned, never defaulted unverifiable no known divergence, but the check did not complete drift divergence is known "Healthy" beside "12 threads I could not classify" is a completeness claim over an incomplete check. "Drift" would be equally false — nothing is known divergent. The middle state exists so neither lie is available. Same shape as R4's local_only/durable:false, applied to reads instead of writes. A failed remote thread read reports unavailable and downgrades the overall verdict, rather than silently omitting the store it could not reach and still printing healthy — that omission IS ad5ca35a. checkCacheHealth's project parameter becomes live for the thread half and stays ignored for the scar half, deliberately: the local scar cache is unified cross-project, threads are project-scoped by schema. Documented at the call site rather than papered over. Tests: +18 total on thread-drift (5 new for three-valued status, including drift-beats-unverifiable and never-healthy-with-a-non-empty-bucket). Suite 1060 green across 64 files, typecheck clean. Refs GIT-69, ad5ca35a Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e (GIT-69 item 3)
Closes feedback 11c313d0 (non-atomic multi-record persistence). The
mechanism was sharper than "doesn't report a partial write":
syncThreadsToSupabase(...).catch(err =>
console.error("... (non-fatal):", err)); // outcome discarded
saveThreadsFile(openThreadsOnly); // prune runs regardless
A failed sync plus a successful prune dropped resolved threads locally
while remote still held them open — no error, green close. A destructive
local operation gated on a remote write whose result was never examined.
This is the purest instance of scar cd345431 (fire-and-forget hides
constraint violations) in the product: the swallow and the destruction sat
two lines apart, and the second depended on the first.
Changes:
- syncThreadsToSupabase returns ThreadSyncResult {attempted, synced,
failed[], skipped, all_synced} instead of void. It still completes a
partial sync — worth finishing — but now names every miss.
- session_close AWAITS it. Prune runs only when all_synced; otherwise
threads.json is retained in full, because it is the only remaining
record of the threads that did not land.
- Session files and the closing payload are retained on partial persist
too. Reporting failure while deleting the means to retry would be R4's
crime in reverse.
- Close returns success:false with validation_errors naming each missed
thread id and its error, per R4's fail-honest shape.
R3 — the gate is tier-relative. Free tier has no Supabase to await, so
sync reports skipped:true and the prune proceeds on local persistence
alone. Awaiting a sync that does not exist would fail-close every free
user's session close.
Adjacency: with the sync now awaited, GIT-70's unordered limit-200 dedup
window runs synchronously inside close, so its duplicate-creation
behaviour is close-path-critical rather than background.
Tests: +8 covering the outcome contract the prune gate depends on —
tier skip, empty-list success, single failure recorded not swallowed,
partial split, never-all_synced-with-failures, every missed id named.
Suite 1068 green across 65 files, typecheck and build clean.
Refs GIT-69, GIT-70, 11c313d0, cd345431
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per R9c. Answers feedback c1b65b66 (orchestrator cannot resolve a child
session's thread) without inventing a lineage model gitmem does not have.
The asymmetry that justifies this, and does NOT justify it for dedup:
resolve is an explicit, human-directed act on a NAMED thread — the caller
states a specific target — while dedup is automatic and silent over a
candidate set it never sees. R1's own-session caution therefore applies to
dedup and not here.
Condition (1) — cross-project resolve refused. getThreadFromSupabaseById
filters on thread_id alone with NO project filter, so before this change a
caller in project A could resolve project B's thread outright. Ownership is
now checked BEFORE hydrating by ID, because hydrating first would resolve
across projects silently.
Condition (2) — audit trail as the price of the widened power. Any resolve
of a thread this session does not own names the owning session in both the
response and the log. Silence here is exactly what project scope would
otherwise have bought us. No line when the caller owns the thread: an
always-on notice is noise, and noise is how a real signal gets ignored.
Condition (3) — a miss returns ownership information where it exists
("exists in project P, owned by session Y, status S") rather than a bare
not-found. Reporting absence for a thread that demonstrably exists is the
same lie as reporting success for a write that did not land.
Condition (4) — parent_session filed as GIT-72 against GIT-51's session
model. SessionChild is NOT a session: it has no session_id, it records
roles inside one session. No lineage is synthesized from it here.
New getThreadOwnership() is deliberately unscoped — the one lookup allowed
to see across projects, because its purpose is to explain a refusal rather
than grant access. Documented as such so it is not later mistaken for a
widening primitive.
Tests: +4 (cross-project refusal names the project and writes nothing;
miss carries ownership; cross-session resolve names the owner; owned
resolve stays quiet). Suite 1072 green across 65 files, typecheck and
build clean.
Refs GIT-69, GIT-72, c1b65b66
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The harness for the sprint's P0, built first so the fix has an honest way
to be verified. A stdio MCP harness already existed (tests/e2e/mcp-client.ts,
booting local dist/ headless with env overrides) — this extends it rather
than adding a parallel one.
Added:
createOutageEnv() pro tier with SUPABASE_URL at a closed port. The
f9fa0461 condition reproduced honestly — healthy
session, dead SOT — through the real write path, no
mocking. Port 1 refuses instantly, so failures are
immediate rather than 30s timeouts.
restartServer() close and respawn preserving GITMEM_DIR/HOME. Files
persist, process does not — the state session-identity
loss actually occurs in.
queryThreadRow() direct REST read, bypassing every gitmem code path.
THROWS on an unreachable store rather than returning
null: "cannot reach" must never read as "row absent",
or the harness reproduces the fail-open bug it exists
to catch.
hasLiveSupabase() so row-level tests skip loudly instead of vacuously.
Repro (tests/e2e/git-67-fail-open.test.ts) — 4 of 5 assertions FAIL against
this build, which is the point. A repro that passes before the fix is not a
repro. Confirmed defect shape, captured from the live probe:
Thread created: "..."
ID: t-02972e09 · 1 open threads
on a pro tier whose durable store is unreachable. Bare success, minted ID,
no caveat, no error.
Two lessons baked into the file as comments:
1. An early assertion keyed on "Thread created" PASSED while the defect was
live, because the response had taken the dedup shape instead. Assertions
are now shape-independent: no ID may reach the caller without a statement
about durability, whatever phrasing wraps it.
2. An early assertion for "admits non-durability" PASSED because the test's
own thread text contained the word "durable" and the tool echoes
submitted text back. Render-from-submission corrupted the test written to
catch render-from-submission. All thread text is now sentinel-free.
The surviving passing assertion is documented as weak — it is satisfied by
the enforcement banner alone while an ID is minted beside it.
No source changes. Fix follows.
Refs GIT-67, GIT-63, GIT-45, f9fa0461
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sprint's P0. Community feedback f9fa0461: create_thread returned a plausible t-xxxxxxxx for a row that never existed. Verified fixed against the harness from 436c7ff, which failed 4 of 5 assertions before this commit and passes 11 of 11 after. R5 — no active session, tier-relative. Pro refuses outright with stored:false, reason:"no_active_session", and NO id. An invented ID is strictly worse than an error: the caller records it and moves on. Free tier still writes, because there the local file IS the SOT and sessionless use is legitimate — refusing would fail-close every free user. Its response says "session: none" rather than implying an ownership it lacks. R2 — dedup refuses; it neither merges nor discards. The old path returned the matched thread's ID with the SUBMITTED text rendered beside it and success:true, while the stored row kept its own text and only updated_at moved. Now: stored:false, reason:"duplicate_candidate", the matched row's CURRENT text read back from the store, and both lengths so a discard is arithmetically visible. Two explicit ways through — resolve-then-create, or allow_duplicate:true. Critically there is no `thread` field on this response: returning a ThreadObject is exactly how the old code handed back another thread's ID as though it were the caller's. R4 — fail-honest on store failure. A Supabase write that does not land no longer falls through to a success with a minted ID. The local buffer survives (destroying data to signal an error repeats the crime in reverse), but the response is unmistakably non-success: stored_in:"local_only", durable:false, supabase_error, and prose saying it will not reach another session. R3 — every response names its store (stored_in: supabase | local | local_only), which also permanently closes the f104e10d silent-local trap: a write that quietly went local can no longer look identical to one that reached Supabase. The convention underneath all four: the response is rendered from a post-write READ of the stored row, never from the submitted payload. An ID appears in output only if the row backing it exists. Where neither store has the row, nothing is claimed at all. Tests: +6 e2e (dedup refusal states not-stored, shows both lengths, offers a way through, allow_duplicate works; free tier stores and names session:none). Suite 1072 unit + 11 e2e green, typecheck and build clean. One spec tension flagged to GIT-67, not resolved here: on free tier with no session the enforcement banner and a success payload still coexist, which GIT-67 criterion 3 words as "never both". They no longer contradict — the payload says session:none and the banner says the same — but the literal criterion is unmet and the wording needs a ruling. Refs GIT-67, GIT-63, f9fa0461, e39ec941 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Criterion 3 originally read "a response must never contain both a success
payload and an enforcement failure banner." That was written against the
old defect, where the two CONTRADICTED — success claimed while enforcement
asserted the write's precondition was absent. The contradiction is what
made the response a lie, not the co-presence.
R10 amends it: a response must never simultaneously claim success and
assert a condition that negates that success; banner and payload must
agree on session state and storage state.
This test encodes the amendment so it is mechanical rather than
remembered. It asserts CONSISTENCY, not absence:
- when the banner denies a session AND the payload reports a stored
thread, the payload must also say session:none and name its store —
either omission recreates the lie
- a payload may never both present an id and assert the write did not
store, in any tier
Closes the coverage gap disclosed in c0b577e: the dual-message test was
pro-only, so a free-tier violation would have passed unnoticed.
e2e 12/12 green.
Refs GIT-67
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
nTEG-dev
marked this pull request as ready for review
August 7, 2026 02:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — verification only. Do not merge. No release, no tag.
Opened per ruling R8b on GIT-69 to buy the full Node 18/20/22 CI matrix as free verification without a merge or publish surface.
What this does
Introduces
src/services/thread-scope.ts— one definition of "the thread store, scoped" — and routes every surface that reads or resolves threads through it.Four surfaces each computed their own view and disagreed in three ways that were invisible from any single call site:
session_startpaneldormantlist_threadsdormantresolve_thread(by text)ORDER BY, project-wideown_sessionThose disagreements are the
f02f74capanel-vs-list defect and the nondeterministic dedup candidate set.THREAD_SCOPE_ORDERandTHREAD_SCOPE_LIMITare single constants, and the order carries athread_idtiebreaker — without it,vitality_score: 1on most rows leaves the candidate set unstable exactly at the limit boundary.Rulings implemented
nulland dedup is skipped, rather than widening to the namespace. The local-file fallback is held to the same scope so cache and store cannot disagree.session_close's suggestion pass opts into"project"visibility as an explicit parameter, not a default — it proposes threads to a human rather than silently merging into one.Verification
npm run typechecknpm run buildnpm run test:unitNew coverage: resolver parity (query builder vs in-memory filter on shared fixtures), cross-project isolation including the
f02f74caleak as a guarded case, determinism, own-session candidate set, status sets, the recovery→scoping seam fixture, and 7 display-layer reconciliation cases assertingshown + dormantHidden + overCap === totalInScope.Scope boundaries
wip/git-51-session-recovery(acceptance check on the diff, per R6).syncThreadsToSupabaseis deliberately not wired — sync's criterion is reconciliation, not visibility. Filed separately as GIT-70.Refs GIT-69, GIT-63, GIT-67, GIT-70