feat(copilot): read per-request input/cache from session-store.db

[kelchm]

Rebased onto current main (da3d903) as a single commit. All five blockers from the ec4ed4f review are addressed, plus both follow-up findings and the #927 collision — the table below maps each to its fix and its regression. The precedence mechanism changed since the last reviewed head: rollup-vs-store precedence is now per-(session, model) serve-time reconciliation with per-leg residuals (Design, below).

Why

The Copilot CLI and the GitHub Copilot desktop app both write ~/.copilot/session-store.db unconditionally: one assistant_usage_events row per API request, written as it happens with real timestamps. Until now, input/cache tokens for these surfaces came only from the session.shutdown rollups in events.jsonl. Validation against two machines' real stores showed that the rollup is lossy in three independent ways. First, it is written only on clean shutdown, so a session SIGKILL'd mid-turn keeps its DB rows but loses the entire leg's input/cache accounting. Second, its counters reset at in-session compaction: a clean, single-process, 107-request session's sole rollup covered exactly its five post-compaction requests and omitted the prior 102. Any long session that compacts is therefore silently truncated, crash or not. Third, it stamps everything on the shutdown day: a session run on Aug 6 but closed the next morning landed entirely on Aug 7.

On a long-history validation machine with 4,826 sessions, 1,360 store rows, and 8 models, reading the store recovered about 35% of real Copilot spend: $165 → $256. The entire delta reconciles exactly to the three gaps above.

Response to review

Finding	Fix	Pinned by
1 — store rows fabricate calls/turns	Supplementary weight: rollups/residuals are always weightless; rows pair with per-turn calls by timestamp adjacency, and only unpaired rows count. One shared helper carries the rule into sealed daily counters and every user-visible surface (models, audit, TUI, compare, session/PR/branch rows, plan windows, savings counts).	(s), (y), aggregator test, one regression per surface
2 — deferred store read reports hydration complete	A deferred changed source (busy, EACCES, unreadable fingerprint) marks the pass incomplete. The mechanism is generic parser machinery, scoped here to this PR's deferral path (#916 keeps the cross-provider boundary). The verdict is memoized with each result, and the daily watermark holds.	(t), (t2), (z)
3 — absence epoch inflates sealed history	Reconciliation reads only cached contents. Deleting or resetting the store changes nothing served.	(o) — now asserts identity where it asserted the doubling
4 — same-path DB reset reuses dedup keys	Content-discriminated keys: copilot-store:<sid>:<rowId>:<fnv1a64(created_at|tokens|model)>.	(v) + parse-level pins
5 — version sequencing	Daily cache 17 → 19, deliberately skipping the burned 18 (details below). Parse version appends -session-store-v2.	—
Partial coverage drops the uncovered tail	Per-leg residuals: each rollup leg subtracts only the rows in its own interval and serves the remainder once, on its own day.	(p), (w), (x)
Late-learned project splits the session	One label per session, resolved at serve time, in both directions (rows cached before the jsonl existed; jsonl orphaned after a prune).	(u), NULL-cwd unit pin
#927 collision (iamtoruk)	The env declaration is withdrawn; the read is allowlisted with its own reason string, and the map comment documents why repointing needs no fingerprint under serve-time reconciliation. This is an explicit decision in the diff.	env-guard tests

Design

Source. There is one session-store source per DB file, mirroring the OTel pattern: lazy openDatabase, iterate, then close in finally. The env override is CODEBURN_COPILOT_SESSION_STORE_DB, with a default of ~/.copilot/session-store.db. Each billable row becomes a supplementary call carrying input, cache-read, cache-write, and reasoning tokens with outputTokens: 0; per-turn output, tools, and userMessage remain on the events.jsonl per-turn calls. A row with an empty model (TEXT NOT NULL still admits '') is priced as unknown rather than dropped. input_tokens is cache-inclusive (input + cache_read + cache_write), using the same convention as the rollups, and the parser emits the uncached remainder. This was verified three ways: per-row token_details_json, footer reconciliation on machine 1, and an 18-comparison rollup reconciliation on machine 2 (16/18 exact; both divergences were the rollup gaps above, always DB > rollup). Across 1,380+ real rows, there were zero violations. Each row's recorded billing charge (total_nano_aiu, request_multiplier) is captured on the cached call as opaque metadata, schema-adaptively so that older stores without those columns parse identically. Nothing prices or displays it; billing-grade cost stays with #890.

Reconciliation. Both representations always parse and cache. At serve time, parseProviderSources aggregates cached calls per (session, model). Where rows exist for the pair, the rollup calls are dropped and the rows serve per request. Each rollup leg subtracts only the rows in its own interval. Rows commit strictly before their leg's shutdown line, so a leg at time T covers exactly the rows in (previous leg's T, T]. Any remainder serves once as a residual call at that leg's timestamp, per token component, floored at 0. Where no rows exist, the rollup serves unchanged. The serve set is one coherent snapshot: nothing a writer does between discovery and a parse can change what one serve pass sees. It is also always the complete cached history: copilot is a durable provider, and the sharded cache loads durable providers in full even for ranged queries — enforced by provider name as well as the persisted flag — so pairing and residual subtraction can never become range-dependent.

Three properties follow. First, partial coverage never drops the uncovered tail. The prior head suppressed the whole session's rollups on any served row, under-serving stores adopted mid-session or pruned before reading. The residual is exactly the tail, and it shrinks monotonically to zero at full coverage (test (p)). Second, a crash tail cannot cancel a covered leg's missing rows. The lifetime max(0, rollupSum − allRowsSum) was wrong exactly there, because a row after the leg must never join its subtraction (test (w), the reviewers' verbatim arithmetic). Per-leg anchoring also keeps each gap on its own day (test (x)). Third, served totals are independent of store presence, so no absence epoch exists for a sealed day to capture. The prior head's documented ≤90d doubling could be sealed into daily history forever (test (o), which previously pinned that doubling and now pins identity). Cached rows from a deleted store remain the record until the 90-day orphan age-out. At that point, the still-cached rollups take over, still exactly once.

Behavioral weight. One real request served both ways previously counted apiCalls=2, turns=2, modelCalls=2. Now, a rollup or residual is aggregate accounting, never a request: it has zero call/turn/model-call/category weight, while retaining tokens and cost. A store row is one real request, but when its per-turn call is served, the row is supplementary too. Rows pair with same-model per-turn calls by monotone timestamp adjacency over the FULL cached serve set. The two carry no shared request id, so pairing happens inside a deliberately tight 2-minute window. A request's row and its assistant.message are written at the same completion moment, while a crash-only row sits minutes from any unrelated call. A wide window would let it pair against a neighbor whose own row is missing and hide the crash request's weight. Both external reviewers found this independently, and a crash row 5 minutes out is pinned to keep it.

Full-set pairing means a date-range boundary cannot double a request across adjacent day queries (test (y)), and a subagent's haiku row can never pair against the parent's sonnet call (test (s)). Supplementary-only turns fold into the nearest behavioral turn within 30 minutes, never across a local-day boundary. With no behavioral turn (a rollup-only events.jsonl, or a range slice holding only residuals), they remain separate weightless turns, and the session serves its usage with apiCalls: 0; the emission gate admits usage-bearing zero-call sessions. The rule lives in one helper module (behavioral-weight.ts) consumed by every counter: session summaries, aggregateProjectsIntoDays and the counters it seals, exports, the TUI, models/audit/compare reports, plan windows, and savings counts. This keeps every surface consistent. records.csv retains every supplementary row, because dropping them would hide recovered tokens from the ledger, and marks each with a supplementary column. Test (s) pins the four contract scenarios verbatim: JSONL + matching row = 1 call/turn; JSONL + rollup = the JSONL count; store-only crashed request = 1 call; rollup-only = 0 calls with full usage. The marker (supplementaryAccounting) is transient, assigned at serve time, and never cached, so there is no schema change.

Hydration fence. A changed source whose read defers on the busy shape (locked, EACCES, corrupt mid-replace) continues serving its cached rows, but the pass records the deferral and isSessionHydrationComplete() reports false. The daily backfill therefore holds its watermark instead of finalizing a day the deferred rows never reached, and the fence lifts on the next successful read (test (t)). The verdict travels with its result: the 180s memo and serve burst-reuse restore the hydration verdict under which their data was parsed, so a memoized partial parse cannot inherit a later parse's complete verdict. An incomplete parse also rides only the short TTL — the resident process's validated-reuse extension never prolongs a deferral past its promised retry — and that resident watcher can genuinely see copilot change: probeRoots() covers every discovery root, the store contributing its parent directory so SQLite WAL appends are visible. An unreadable fingerprint defers too; previously, it was skipped before any parser could throw. A genuinely deleted file remains a silent skip. Both cases are covered in suite (z).

Test (t2) wires the real flag into ensureCacheHydrated with no stub. A seeded watermark is held exactly, neither advanced nor reset, while the store's re-read defers. The same persisted incomplete cache then heals in place, with the deferred row's tokens included in the sealed day. An unchanged unreadable store defers nothing; one that changed and remains unreadable holds the fence until the read lands, by design. This covers this PR's own deferral path; the cross-provider boundary stays with issue #916.

Identity. Every call in a session serves under one project label, resolved at serve time: the session-state-derived project (workspace.yaml cwd) when the serve set knows it, otherwise the store rows' own label. Project is part of the session grouping key, so this prevents one real session from splitting in two. It works in both directions. Rows cached before the session's events.jsonl existed are relabeled when the serve set learns better, including the NULL cwd/repository shape, which is unit-pinned. Conversely, a jsonl orphaned by a session-state prune adopts the surviving rows' label instead of the generic copilot bucket (test (u), both ways). The store's own sessions.cwd → repository → sessionId chain labels only sessions with no session-state at all.

Dedup keys include a 64-bit FNV-1a content hash of the raw created_at, token counts, and model. A same-path DB reset that reuses AUTOINCREMENT ids therefore mints new keys, admitting the recreated row's usage while the original remains as durable history. A byte-identical re-insert (backup restore, VACUUM INTO) still collapses (test (v); 32-bit collisions between plausible token tuples are constructible, hence 64).

Reasoning tokens. These are metadata, never a cost line, and that is now true end to end. They are a subset of output_tokens because the store's token_details_json prices input/cache/output only, and the per-turn calls already bill the full output. The query-time recompute previously re-added them for non-claude providers. On machine 2's ~533K priced reasoning tokens, that produced about $13 of phantom cost, demonstrated on a real row ($0.00520 → $0.00379). Copilot now joins claude in the reasoning-inside-output case.

Failure semantics — discovery is schema validation; everything non-absent defers

Discovery uses stat() plus prepare-validation of the parser's exact query (LIMIT 1, shared verbatim with the parser so the two cannot diverge on schema).

Condition	Behavior
True absence — stat() says ENOENT/ENOTDIR, no sqlite driver, or no such table/column (CLI builds predating the store, or a future migration)	No source. Rollups rule.
Anything else — stat() EACCES/EIO, locked, corrupt, mid-replace, or the store failing at open or mid-parse after a validated probe	The source is emitted anyway. Its parse raises the busy shape; parseProviderSources skips and retries; no cache write; hydration reports incomplete until the read lands. Previously cached rows keep serving, and reconciliation keeps holding from the cache.

Session-state files never wait on the store: a locked or corrupt store cannot stall or defer events.jsonl parsing.

Versions — parse `-session-store-v2`; daily cache 17 → 19, skipping the burned 18

• PROVIDER_PARSE_VERSIONS.copilot appends -session-store-v2 (v1 never shipped). The bump re-parses pre-store caches so rows land, and it re-keys any v1-era cache to the content-discriminated keys. It carries the same one-time drop-and-reparse cost copilot provider: cache hit rate is always 0.0% when using GitHub Copilot CLI (sessions misidentified as VS Code transcripts) #944 took: the fix(cache): declare the provider env overrides that must invalidate the cache #927-documented hazard that rows pruned from a still-existing DB before first parse are lost. That hazard is stated here explicitly.
• DAILY_CACHE_VERSION 17 → 19, deliberately skipping 18. Main is at 17, but an earlier public head of this PR claimed v18 under different accounting (whole-session suppression, no supplementary weight). isMigratableCache/adoptOlderDailyCaches would carry those days forward as finalized without re-deriving them. Skipping the number is what makes "existing v18 data cannot be accepted under two meanings" true in practice. If anything else claims 19 first, this PR takes the next free number before merge.
• The fix(cache): declare the provider env overrides that must invalidate the cache #927 hazard explains why: any copilot fingerprint change drops cached entries whose source path still exists, destroying provider-pruned history that only the cache holds. The env question is resolved in the diff itself. The PROVIDER_ENV_VARS declaration is withdrawn, the read is allowlisted with its own reason string, and the map comment documents why serve-time reconciliation makes repointing safe without a fingerprint (a new path parses on sight; the old path's rows persist as durable orphans). Completing the copilot map stays sequenced behind fix(cache): declare the provider env overrides that must invalidate the cache #927's carry-forward follow-up.

Sharp edges

There are three accepted residuals and one designed asymmetry. All are documented in code and bounded:

• Corrupted or reset inputs can defeat interval subtraction — always over-serve, never lose. A same-path reset that re-inserts rows with in-interval timestamps can cancel that interval's missing usage. A rollup with an unparseable stamp serves weightless on a stable fallback: the file's preceding valid timestamp, or else the session's earliest. This is pinned in (w2), including the resume case. A timestamp-less shutdown anchors on the last event seen, which is not a guaranteed upper bound for a store-only leg. Each case requires multiple independent failures to stack; none occurred in 4,826 real sessions.
• Pairing is ambiguous inside the 2-minute window. A crash-only row within two minutes of a request whose own row is missing can pair against it, under-counting apiCalls by one. Tokens remain exact. This requires two independent record losses within two minutes.
• Model-identity corners serve both representations. A row whose model string cannot match its rollup's (the empty-model unknown key, or a named mismatch — never observed across 1,380+ rows and 18 reconciliations, where names matched exactly) is invisible to per-model reconciliation, so both representations serve: over-serve, never lose.
• A session idle >90 days serves store input/cache without journal output. The durable age-out is scoped: only the store DB — the durable record itself, whose crash-only rows have no rollup to fall back to — is exempt while present (retainWhilePresent); journal-style sources keep the released schedule. When an idle session's journal ages out, its store rows lose their pairing partner and become behavioral. The call count therefore stays ≈ the request count, and the served set is strictly more than the release serves (which is nothing). Whether journal-style sources should get the same retention is Durable providers delete cached history older than 90 days even when the source files still exist #987.

One accuracy gap is an open decision, not a silent residual: store-only (crash-recovered) requests serve measured input/cache but not their output tokens. The trade-offs and a proposed default are in the review thread, alongside three more decisions (session-count weight, empty-model handling, sequencing).

This PR does not change the following pre-existing behavior, noted because the store makes it more visible: project labels are live-computed while sealed days keep their sealed label (every provider); a read-only run with an unfetchable network provider can still report complete hydration (network providers fetch only in write mode; the fingerprint fence deliberately exempts them); a durable orphan aging out at 90 days steps lifetime totals once as the rollups become the record — the same dynamic OTel prunes have always had.

Out of scope

• Billing-grade cost from total_nano_aiu / request_multiplier, and throughput from the latency columns (upstream Cost modes: reconcile displayed cost with the source's own reported cost (auto/calculate/display) #890)
• The cross-provider hydration boundary for every SQLite provider's busy-skip (issue Session-cumulative providers (hermes) lose post-finalization usage from the daily history #916) — this PR fences its own deferral path
• Sync/OTLP exports the serve set as-is, as it always has for rollup calls; a receiver-safe copilot contract (supplementary marking plus append-once-compatible identities for reconciliation output, which is mutable while spans are send-once) is Sync can double-count copilot usage at the receiver — spans are sent once, but reconciliation output changes #988
• mergeDayEntries partial-slice shadowing (src/daily-cache.ts): pre-existing on main, exercised by every version bump including this one; a fresh non-empty provider slice suppresses the carried slice for that (day, provider), so a partially re-derivable day loses its source-expired remainder. It is called out because the bump makes it reachable, not because this PR introduces it
• The rollup's compaction reset likely deserves an upstream report to the Copilot CLI

Validation

Store-writing CLI builds 1.0.70, 1.0.78, 1.0.78-2, and 1.0.79 all reconcile across the two machines; the sole divergence is the 1.0.78 compaction reset above. The store first appears between 1.0.67 and 1.0.70, and older sessions retain the rollup path. npx tsc --noEmit clean; npm run build:cli clean; npm test 2,680 passed; npm run test:locks 26/26. docs/providers/copilot.md documents the source, reconciliation, weight, failure behavior, and dedup keys. On the second machine's full history, four validation rounds ran on the final head: branch output tokens equal the release exactly; every headline delta reconciles to store recovery minus the removed rollup-reasoning pricing; per-session AI-credit calibration matched GitHub's charged dollars to floating-point precision on 21 of 24 store-covered sessions; repeated runs are hash-identical.

Evidence — A/B runs, live crash test, machine-2 reconciliation, test inventory

A/B (real-store snapshot, fresh caches). Serve-time and parse-time heads produced identical totals: $0.565 / 537,154 tokens. Pre-store main: $0.503 / 512,423. The serve-time head also healed a cache warmed by pre-store main to the same totals with no double-count. This revision changes totals only where a rollup exceeded its session's rows (previously truncated to the rows, now topped up by the residual) and where reasoning was double-billed.

Machine 1, live. SIGKILL crash recovery was exact (+2 input / +24,729 cache-write); warm-refresh deltas matched new DB rows to the token; concurrent overview against a streaming CLI session read cleanly; upgrade healing worked in place.

Machine 2. 4,826 sessions, schema v6, 1,360/1,360 ISO-Z timestamps, 0 cache-inclusive violations, and 16/18 exact reconciliations, with both divergences root-caused. The 4,826 → 4,825 headline traced to the per-day reattribution that the daily-cache bump exists to handle.

Serve-level regressions, one per finding: behavioral weight across all four contract scenarios, rollup-only sessions, and per-model subagent pairing (s); fence held and lifted with totals unchanged during the outage (t); real-flag watermark held and healed in place, with the JSONL unchanged throughout so only the store's own deferral can reach the fence (t2); project unification in both directions (u); same-path reset (v); mixed coverage — crash tail vs covered-leg gap (w); equal-timestamp coalescing, unparseable stamps, and out-of-window crash weight (w2); multi-leg residual day attribution (x); range-invariant pairing (y); memo verdicts and the fingerprint fence (z); absence-epoch identity (o, inverted from the prior head's pinned doubling); progressive row landing with residual retirement (p); the growing-store merge (i); sharded-cache integration — durable full-load under a scoped ranged query (mutation-verified by-name pin), age-out deletions persisting to shards, retention re-bucketing on older appends without duplicate shard copies, incomplete-memo retry before the validated cap, and billing metadata through the per-shard validator (sc). Parse-level: key stability, reset splitting, byte-identical collapse. Aggregator-level: supplementary weight in sealed counters. Unit-level: virtual-suffix stat candidates shared with the fingerprint fallbacks. The one flake: a CLI-spawn test can exceed its 5s timeout under full worker-pool load (1.0s isolated on this head, 0.99s on base — not a regression).

History — how the design got here

Rounds 1–4 of external adversarial review (grok-4.5, Claude Opus, gpt-5.6-sol at max effort, GitHub Copilot's reviewer) hardened a parse-time suppression design. Three reviewers independently converged on its races: probe-vs-parse, atomic replacement, and absence epochs. Round 5 then replaced it with serve-time precedence. This revision replaced whole-session suppression with per-(session, model) reconciliation and per-leg residuals, added the weight/fence/key/project fixes, and absorbed the #927 rebase. Further adversarial review of the finished head (grok-4.5 and gpt-5.6-sol, independently) surfaced the residual-cancellation arithmetic, its edge shapes, and a timestamp-stability re-break. Every finding was fixed with its own regression. Every maintainer finding across five exact-head review rounds is either fixed with a verbatim-repro regression on this head or explicitly documented as an accepted residual above.

[Copilot]

Pull request overview

This PR adds support for reading Copilot per-request input/cache usage from ~/.copilot/session-store.db (SQLite) and uses that as the authoritative source for input/cache/reasoning when available, suppressing redundant session.shutdown rollups to avoid double counting. This improves correctness for crash scenarios (no clean shutdown rollup) and improves timestamp/day attribution by using per-request timestamps.

Changes:

• Add discovery + parsing for a new Copilot source type session-store that reads assistant_usage_events rows and emits supplementary calls (input/cache/reasoning only; output excluded).
• Tag JSONL session-state sources as shutdownCovered (or defer parsing when the DB is locked) so shutdown rollups are suppressed only when the DB coverage is known and usable.
• Bump Copilot parse version and daily-cache version to force re-derivation under the new per-request accounting; add/expand unit + integration tests and update changelog.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/providers/copilot.ts	Adds session-store discovery/parsing, coverage-based suppression/deferral for JSONL shutdown rollups, and wiring for new sourceTypes.
src/session-cache.ts	Fingerprints CODEBURN_COPILOT_SESSION_STORE_DB and bumps Copilot parse version to heal cached sessions under the new source.
src/daily-cache.ts	Bumps daily cache schema/version to re-derive day attribution with per-request timestamps.
tests/providers/copilot.test.ts	Adds hermetic env stubbing plus extensive unit coverage for session-store parsing/suppression/locking behavior.
tests/parser.test.ts	Adds end-to-end durable-merge integration tests for growing JSONL legs and growing session-store DB rows.
CHANGELOG.md	Documents the new session-store source behavior and associated cache/version bumps.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kelchm]

Validation update, worth reviewer attention because it reframes the impact: tracing a reconciliation outlier on a second machine showed the session.shutdown rollup resets its counters at in-session compaction. A clean, single-process, 107-request CLI 1.0.78 session's sole rollup matched its five post-compaction requests exactly (per model, to the token) and omitted the prior 102 requests. So rollup-only accounting truncates any session that compacts — not just crashed ones — and on that machine the store recovered ~35% of real Copilot spend overall. The delta-with-reset-detection logic from #944 remains correct for multi-rollup files; the reset trigger is now confirmed and documented at the code site. Full details in the updated description.

[ozymandiashh]

Blocking changes required on this exact head:

1. This stack inherits both blockers from #945, and commit 1dc69f3 adds another prohibited Claude co-author trailer. #945 must be corrected and merged or closed first; then this PR needs an updated base and fresh review.

2. Coverage is captured during discovery (src/providers/copilot.ts:2281-2295), store rows are read later, and JSONL suppression uses only the stale coverage set (:2747-2753). If Copilot commits a DB row and appends session.shutdown between those phases, both the DB call and JSONL rollup are emitted. A production-shaped fixture doubled input 100, cache-read 8,000, and cache-write 2,000, and durable union can persist the duplication. Coverage and emitted rows need one coherent snapshot/transaction or equivalent parser-side suppression derived from the same rows.

3. The stat() path catches every error as absence. EACCES/EIO and other unknown failures must defer rather than admit potentially duplicate JSONL rollups; distinguish expected absence/schema cases from transient or permission errors.

Targeted tests (110/110), typecheck, CLI build, and diff check passed locally, but this head has no remote checks and remains stacked on the unmergeable #945.

[ozymandiashh]

Blocking race remains on exact head a55ad07. The parse-time store recheck is enabled only when discovery saw an uncovered events.jsonl mtime within 60 seconds of the probe (src/providers/copilot.ts:2847-2849; consumed at 864-869). For an active file older than 60 seconds, Copilot can commit its DB row and append session.shutdown after discovery stat but before parsing. The store parser emits the row while JSONL emits the same rollup; durable union preserves both incompatible keys. A production-shaped repro sets mtime 10 minutes old, discovers it with no recheckStore, inserts the covered DB row, appends shutdown, then parses both sources: expected zero shutdown calls, actual one, duplicating input/cache. The existing fresh-file test always falls inside the 60-second heuristic and misses this window. Every uncovered session capable of shutdown emission needs a parse-time coverage recheck, or coverage/emission must share one coherent snapshot. Prior trailer and stat-error blockers are fixed; 205 targeted tests, typecheck, CLI build, and diff check pass, but this adversarial case fails. Rebase/update after #945 lands and request fresh review.

[ozymandiashh]

Blocking findings on exact head bcab2f0aec88fecf33e336ecbeccc8c1c746a218:

1. A normal JSONL assistant call and its matching session-store row are both counted as API/model calls and separate turns. The DB call at src/providers/copilot.ts:2105-2125 is supplementary accounting, but the downstream paths at src/parser.ts:2493-2501,1683,1717,1729 give it ordinary behavioral weight. An end-to-end fixture produces 2 calls/turns where one real request occurred. Store accounting calls need zero call/turn weight while retaining input/cache/reasoning usage.

2. A store row with NULL cwd/repository falls back to session_id as project (src/providers/copilot.ts:2073-2080), while the matching JSONL source uses workspace.yaml (:2242-2255). Because the session grouping key includes project (src/parser.ts:3126-3128), one real session splits into two projects/sessions. Resolve the store row to the same workspace/session project identity, with a NULL-cwd regression.

3. The stale-mtime double-count race is fixed, but the opposite same-refresh race remains. Discovery/parse orders the store before JSONL (:2835, :2870); the store snapshot reads at :2020-2038. If a row commits after that read but before JSONL reaches shutdown, the later live recheck (:858-868) sees coverage and suppresses the rollup, while the already-read store emitted no row. That refresh loses all input/cache for the request. Coverage and emitted rows need one coherent snapshot or a retry/defer contract that cannot suppress against rows absent from the emitted store snapshot.

4. New SQLITE_BUSY/retryable deferrals are still reported as complete in write mode (src/parser.ts:3008-3012,3837-3857), so daily hydration can seal a day missing both store and rollup usage (src/daily-cache.ts:756-790,806-827). The #916 branch fixes this global hydration fence, but both branches independently claim daily-cache v18. Merge #916 first; then rebase this PR and use a distinct cache version.

The exact-head targeted suite (189 tests), typecheck, CLI build, diff check, and all five remote workflows pass, but they do not cover these invariants. Three production-shaped adversarial fixtures reproduce findings 1-3. Please keep this draft until all four are addressed and request a fresh review.

[ozymandiashh]

One important refinement to blocker 4: rebasing onto #916 is necessary but not sufficient for the current storeProbeBusy sentinel. #916 marks hydration incomplete when discovery reports a typed retryable failure or when a changed source parser throws. This branch currently converts busy store discovery to sessionStore === 'busy', tags JSONL sources storeProbeBusy, and throws only when their parser runs. On a warm cache where those JSONLs are unchanged, createSessionParser is never invoked, so the retryable failure still never reaches the hydration fence. Please add a production-path regression for: busy store + unchanged cached JSONL => hydration incomplete and daily watermark held, and propagate the busy discovery result directly into the fence (or an equivalent source-independent signal).

[kelchm]

@ozymandiashh All four findings on bcab2f0 are addressed or sequenced. Head: ec4ed4f. Description rewritten (Precedence / Failure semantics / Validation) for the design change below.

2 — fixed. Store rows now take the session's jsonl-derived project (workspace.yaml cwd — the same label its per-turn calls carry) via a sessionId→project map attached to the store source at discovery; sessions.cwd → repository → sessionId applies only to sessions with no jsonl. Regression covers your verbatim NULL-cwd/repository shape plus the differing-cwd shape.

3 — fixed structurally. Precedence moved to serve time, taking "one coherent snapshot" literally: parsers always emit and cache both representations, and parseProviderSources drops a session's shutdown calls only when the serve set holds that session's store rows from a still-discovered store. No probe snapshot and no live re-check remain, so suppression cannot outrun the emitted rows by construction. Your repro is pinned at serve level: test (p) — row commits after the store read → the rollup counts, nothing zeroes; next refresh swaps to the row, once. The rest of the transition family rides alongside: (m) rows-then-shutdown counted once, (k) stale-cache healing, (n) atomic replacement / zero-usage rows, (o) absence epochs. The parse-time machinery (coverage probe, shutdownCovered, re-check, session-state busy-deferral) is deleted — net −96 source lines — and a locked/corrupt store now defers only its own re-read while cached rows keep serving. Bounded residual documented in the description: orphaned rows of a deleted store stop suppressing, so overlap legs can double-count ≤90d until age-out, replacing the old indefinite under-count.

1 — agreed; next head. Store accounting calls will carry zero call/turn weight with usage/cost/tokens retained. That threads a field through the cached-call schema and the stats/daily paths, so I'll land it on the post-#916 rebase rather than churn this head twice. One question: should the shutdown-rollup calls — same supplementary contract — go zero-weight too, or store rows only?

4 — agreed with your sequencing. After #916 lands: rebase, next distinct daily-cache version, finding 1 on that head, fresh review requested then.

--

Refactor A/B on a real-store snapshot: the serve-time and parse-time heads produce identical fresh-cache totals; pre-store main reads $0.503 / 512,423 and this head heals that same cache in place to $0.565 / 537,154. tsc clean; 122 targeted tests; full suite green modulo known environmental failures.

[ozymandiashh]

Blocking findings on exact head ec4ed4f2bc371ae7a8c76b90e22b5d2f0afa9c48:

1. Supplementary session-store rows still fabricate behavioral activity. src/providers/copilot.ts:2111-2131 emits each row as a normal call; calls without turnId become separate turns at src/parser.ts:2493-2500, then increment turns/API/model calls at :1683, :1717, :1729. A production-path fixture with one normal JSONL assistant call plus its matching store row produced apiCalls=2, turns=2, modelCalls=2; expected 1/1/1. Store rows need accounting-only/zero behavioral weight while retaining tokens and cost.

2. A deferred changed-store read still reports hydration complete. The retryable catch at src/parser.ts:3008-3012 continues without recording incompleteness, and :3852-3872 can therefore let daily history advance. A warm-cache fixture changed the discovered store into an unreadable file: isSessionHydrationComplete() was true, expected false.

3. The accepted absence epoch is not bounded once daily history seals. The committed test at tests/parser.test.ts:1113-1144 intentionally doubles live input from 600 to 1,200 while the store is absent. In an end-to-end daily-cache fixture, restoring the store made live usage converge to 600, but the finalized day remained permanently at 1,200. ENOENT is normal absence, so a retryable-only fence does not repair this.

4. Same-path DB reset/replacement can reuse durable dedup keys and lose new usage. The key at src/providers/copilot.ts:2080-2085 is only copilot-store:<sessionId>:<rowId>; SQLite AUTOINCREMENT prevents reuse only within one database lifetime. Recreating the DB at the same path with the same session and a new row id=1 left the old 100-input call in the durable union instead of the new 200-input call (src/parser.ts:2980-2993 rejects the reused key).

5. Sequencing remains required: this head and the pending hydration/accounting fix both claim daily-cache v18. Land the hydration/accounting boundary first, then rebase this PR and use a distinct version so existing v18 data cannot be accepted under two meanings.

The Round-5 serve-time refactor does fix the prior project-alignment and two snapshot-order races, and all five remote workflows are green. Targeted session-store/serve tests, typecheck, builds, and diff-check also pass. The reproduced invariants above remain merge blockers; please add end-to-end regressions for each and request a fresh review.

[ozymandiashh]

Two additional exact-head blockers from the independent pass on ec4ed4f:

• Partial store coverage drops the uncovered tail. Any served store row marks the whole session covered, and src/parser.ts:3073-3106 removes every shutdown rollup for that session. A fixture with DB request 1 plus a shutdown rollup for requests 1+2 served only request 1 while hydration remained complete. Coverage must be granular enough to preserve usage not represented by store rows, or the replacement contract must prove completeness.

• A project learned after the store row was cached can split one session. The JSONL-derived project map is attached only during fresh discovery (src/providers/copilot.ts:2773-2784), while an unchanged store reuses its cached calls (src/parser.ts:2903-2912); project is part of the grouping key (:3141-3143). The regression fixture produced the same session under both testproj and actual-project. Cached store rows need project identity reconciliation/update when the session-state source appears.

The same audit also confirmed that accepting #916's independently-defined v18 through this PR strips its reasoningTokens and webSearchRequests fields while skipping re-derivation, strengthening the distinct-version sequencing requirement in the submitted review.

[ozymandiashh]

On the call-weight question: do not make only store rows unconditionally zero-weight.

• A session.shutdown per-model rollup is always supplementary aggregate accounting, never one physical request. It should always carry zero call/turn/session/model-call/category weight while retaining its token/cost contribution.
• A session-store row is one real request. It should carry behavioral weight only when that request has no served JSONL assistant/request representation; otherwise its token fields supplement the JSONL call and its behavioral weight is zero. Blanket weight 1 duplicates normal completed requests; blanket weight 0 hides crash/store-only requests.

This likely needs an explicit count weight or a logical-request reconciliation rule, not just a source-wide boolean. Please pin at least: normal JSONL+matching row = 1 call/turn; JSONL+shutdown aggregate = JSONL call count only; store-only crashed request = 1 call; and partial store coverage + rollup preserves the uncovered tail exactly once.

[iamtoruk]

Heads up before the rebase: #927 merged today and it collides with this branch in a way that goes beyond the textual conflict in session-cache.ts.

What #927 changed on main

1. PROVIDER_ENV_VARS now has no copilot entry at all, deliberately. The reasoning is documented at the map in src/session-cache.ts: on a fingerprint change, getOrCreateProviderSection (src/parser.ts:2650) keeps only cached entries whose source path is gone, and copilot's OTel discovery returns one source per still-existing DB file, so any copilot fingerprint change drops the cached entry and re-parses, losing conversations Copilot has since pruned from the DB.

2. That decision is pinned by tests, so the rebase will hit red before it hits review:

   • tests/session-cache.test.ts:371 asserts PROVIDER_ENV_VARS['copilot'] is undefined
   • tests/session-cache.test.ts:378 onward asserts the copilot fingerprint does not move for each of the nine deferred vars
   • tests/provider-env-declarations.test.ts is a new static guard that scans every process.env read in src/providers/ and fails on any read not declared or allowlisted. Your new CODEBURN_COPILOT_SESSION_STORE_DB read in copilot.ts will trip it. The existing copilot reads are allowlisted at tests/provider-env-declarations.test.ts:87 with a shared reason string.

Where that leaves this branch

This PR declares copilot: ['CODEBURN_COPILOT_SESSION_STORE_DB'], which contradicts the pinned test directly. Your argument for fingerprinting the store var still holds (repointing it changes which sessions' rollups are suppressed, a cross-file effect the fingerprint has to see), but #927 documents why any copilot fingerprint entry is currently a durable-history hazard. So the rebase needs one of:

• declare the store var, update the pinned tests, and document in the map comment why this specific var is worth the bounded loss fix(cache): declare the provider env overrides that must invalidate the cache #927 describes, or
• allowlist the read for now and sequence the declaration behind the carry-forward change (merge instead of drop), which fix(cache): declare the provider env overrides that must invalidate the cache #927 lists as the follow-up that unblocks completing the copilot map.

Either way it should be an explicit decision in the diff, not a conflict resolution artifact.

Two smaller rebase notes

• The parse-version bump in this PR (session-store-v1) triggers the same drop-and-reparse path fix(cache): declare the provider env overrides that must invalidate the cache #927 documents, independent of the env question. copilot provider: cache hit rate is always 0.0% when using GitHub Copilot CLI (sessions misidentified as VS Code transcripts) #944 already took that hit once, so it may be acceptable, but worth stating in the PR body.
• Main is still at DAILY_CACHE_VERSION = 17 as of today. The v18 collision concern from review stands; whichever change lands second should take the next free version.

[kelchm]

@ozymandiashh All five blockers on ec4ed4f have been addressed. Both follow-up findings and the call-weight contract have also been addressed.

Head: 5d13600559b528d49a6191002fd8a831db91c77c, rebased onto 0.9.20 main.

@iamtoruk — your #927 points have been resolved per your option 2. The declaration has been withdrawn. The read is allowlisted with its own reason string, and the map comment documents the decision in the diff.

Your version rule is honored (decision 4). Both smaller rebase notes are stated in the description, which now reflects the revised mechanism.

1. Call weight: fixed per your contract. Rollups and residuals always retain tokens and cost. They always carry zero call, turn, model-call, and category weight. The residual behavior is described in the follow-ups section below.

Store rows are paired with same-model, per-turn calls based on timestamp adjacency across the full serve set. Only unpaired rows count. These represent store-only requests.

Your four pins are included verbatim as regressions in test (s):

• JSONL+row = 1/1/1.
• JSONL+rollup = JSONL count.
• Store-only = 1.
• Rollup-only = 0 calls with full usage.

The weight propagates into aggregateProjectsIntoDays. Sealed v19 days therefore agree with live summaries.

2. Hydration: fixed here, source-independently. A changed source may defer on the busy shape. When that happens, the pass is marked incomplete. isSessionHydrationComplete() returns false, and the watermark holds. This also applies when the source fingerprint is unreadable.

The verdict is stored with each memoized result. A stale partial parse therefore cannot inherit the complete verdict of a later parse. The flag is generic parser machinery. It is not copilot-scoped.

Tests (t), (t2), and (z) cover this. Sequencing is covered in decision 4 below.

Test (t2) connects the real isSessionHydrationComplete to ensureCacheHydrated. It asserts that the daily cache remains incomplete and that its watermark does not move. This covers the part of your request that a boolean assertion alone did not.

3. Absence epoch: fixed structurally. Reconciliation reads only cached contents. It never performs discovery. Deleting or resetting the store does not change what is served.

A sealed day therefore has no epoch to capture. Test (o) now pins identity. Previously, it pinned the documented doubling.

4. Dedup keys: fixed. The key is copilot-store:<sid>:<rowId>:<fnv1a64(created_at|tokens|model)>.

A same-path reset may reuse ids, but it still creates new keys. The recreated row's usage is admitted, while the original remains durable history.

A byte-identical reinsert collapses. Test (v) and the parse-level pins cover both cases.

5. Versioning: This head uses daily-cache 19, skipping 18. Main is at 17. An earlier public head of this PR claimed v18 under different accounting.

The carry-forward path is isMigratableCache → migratedFrom. It preserves complete. Reusing 18 would therefore adopt those days as finalized without re-deriving them. Version 18 is burned, not free.

The parse version appends -session-store-v2. Version 1 never shipped.

Partial coverage / project identity (follow-ups): Whole-session suppression is gone. Precedence is evaluated per (session, model).

Residuals are calculated per leg. Each rollup leg subtracts only the rows in its own interval, then serves the remainder once using that leg's timestamp.

An uncovered tail is preserved on its own day. A crash-tail row can never cancel a covered leg's missing rows. Tests (p), (w), and (x) cover these cases.

Project identity resolves at serve time in both directions. This covers rows cached before events.jsonl existed and events.jsonl orphaned after a prune. One session therefore cannot split. Test (u) covers this.

Open decisions

Four decisions are yours. Each ships with a default unless you object.

1. Output on crash-recovered store rows stays unbilled. Store rows contain measured output_tokens. A paired row's output is already billed through its per-turn assistant.message call.

Billing unpaired rows would break an invariant: pairing ambiguity may affect call counts, but never tokens. A mispair within the 2-minute window would double-bill output dollars into finalized daily history. That history would not converge. This is the class of problem blocked by your sealed-history findings.

The counter-pull is real. #762 accepted a documented, bounded over-count rather than lose usage. Here, exact vendor-measured output from a crashed request goes unbilled. The probed row serves $0.0058 instead of $0.0133 when output is included.

The clean recovery is the reported-cost path from #890, and this head stages it: every row's total_nano_aiu and request_multiplier are captured into the cache — schema-adaptively, so older-schema stores parse identically and the discovery probe still validates only the base select. Nothing prices or displays them yet.

The unit is also no longer uncalibrated: GitHub documents 1 credit = $0.01, and against one machine's full store the computed cost matched the recorded charges to floating-point precision on 21 of 24 covered sessions — API list rates, exactly. Serving reported cost from the already-captured charge is a small, well-evidenced follow-up.

Default: Unpaired output stays unbilled. This remains a documented accepted edge; the #890 follow-up serves reported cost from the captured charge. Is that acceptable, or do you want unpaired rows to bill row output now, accepting the mispair double-bill exposure?

2. A rollup-only session counts as exactly one session. Your weight contract lists five zero dimensions: call, turn, session, model-call, and category. Four are implemented everywhere, in both live summaries and sealed days.

A literal zero for the fifth would count spend under zero sessions. Per-session averages would inflate, and session counts would diverge from the CLI's own list. That conflicts with CONTRIBUTING's "session counts match what you see in the tool".

Your #916 design sketch points the other way: "a sealed day remains … 1 call / 1 session; a later observation records only … 0 calls and 0 sessions". Supplementary accounting never creates an additional session, but the session itself counts once.

That matches #791's counted-exactly-once rule. It also matches the anchors precedent. Anchors are excluded from sessions only because they carry zero in-range spend. A rollup does not.

Default: One session and zero everything else. "Session weight" means that a rollup never creates an additional session. Can you confirm that reading, or should it contribute zero sessions everywhere despite serving real tokens/cost?

3. Empty-model rows keep the dead unknown key. A row with model '' is priced as unknown/$0. It is not dropped. The never-drop behavior is pinned. The coverage predicate does not know models.

Reconciliation operates per (session, model). An empty-model row can therefore never match its rollup. If this corner case occurs, both entries are served. The result is a permanent token and call-weight over-count, with $0 of extra cost.

There were zero occurrences across 1,380+ probed rows. The behavior still conflicts with your permanence standard and needs explicit acceptance.

The alternatives look worse. Adopting the rollup's model would infer identity in the manner warned against by #968. Repricing would also be unsafe.

Even model-alias unknown <model> would turn the $0 corner into a real-dollar double-count. Aliases affect pricing lookup, while reconciliation retains the raw model name.

Default: Accept and document this behavior, with an explicit do-not-alias caveat. Adopt-when-unambiguous (the row takes the session's single rollup model) is specced and ready as a follow-up if you want the corner closed.

4. Sequencing: proceeding without #916, at daily-cache 19. The facts first. #916 is an open issue with no implementing PR or branch. The design comment there is awaiting confirmation, so the timeline is unclear.

Your two original reasons for boundary-first no longer bind. The fence this PR needed now ships in this PR. Your comment allowed "an equivalent source-independent signal", and tests (t)/(t2)/(z) are the regressions you specified.

The v18 two-meanings risk is also gone. This head takes 19. Version 18 was burned by this branch's earlier public head. The precedent for skipping a never-shipped number is CACHE_VERSION 6→7 in #791.

Your version rule is order-independent. The eventual #916 PR simply takes the next free number. Nothing here preempts it.

The pre-existing mergeDayEntries partial-slice shadowing surfaced during this work. It goes to a disclosed issue rather than widening this diff.

Default: proceed with this head at v19, fence in scope. If there is a remaining reason to hold for the boundary that I'm not seeing, say so, and this waits and renumbers.

This head also addresses the following cases, each with its own regression:

• Residual cancellation under mixed coverage.
• Daily-cache weight propagation.
• Memo-scoped hydration verdicts.
• A fingerprint-failure fence.
• Range-invariant pairing.
• Behavioral weight at every user-visible counter — models, audit, TUI daily, compare, session/PR/branch rows, plan windows, savings counts — through one shared helper, one regression per surface.
• The durable age-out scoped so only the session-store DB is retained while present; journal-style sources keep the released schedule.
• A reasoning-only session no longer vanishes from emission.
• Each store row's recorded AI-credit charge (total_nano_aiu, request_multiplier) captured into the cache, schema-adaptively; pricing/display stays Cost modes: reconcile displayed cost with the source's own reported cost (auto/calculate/display) #890.
• records.csv keeps accounting-only rows and marks them (supplementary).
• Their edge shapes.

The description documents the provably unreconstructable remainders.

docs/providers/copilot.md documents the new source. Typecheck, CLI build, npm test (2,680), and npm run test:locks (26) all pass locally.

[vidoluco]

Independent validation from a third machine (Copilot CLI, mixed gpt-5.6-terra / gpt-5.4-mini / claude-opus-5 sessions), in case a second confirmation is useful before merge.

Credit parity holds here too. Across 76 CLI sessions carrying a session.shutdown rollup, comparing CodeBurn's computed session cost against total_nano_aiu / 1e9 × $0.01: median ratio 1.001. Same conclusion as @kelchm's 21/24 — GitHub prices credits at the models' list rates.

The outputForCost change is right, and the billed total adjudicates it exactly. I hit this independently before finding this PR. On one session:

rollup reasoningTokens	20,115
per-turn output + rollup, reasoning excluded	$0.4100 + $1.1472 = $1.5572
what main reports today	$1.7986
calculateCost(model, 0, 20115, 0, 0, 0)	$0.2414 — exactly the delta
billed	155.72 credits = $1.5572

So excluding reasoning reproduces the billed figure to four decimal places, and main is 15.5% high on that session. That is the whole delta, nothing else moves.

One field that may be worth a look, since it does not appear in this diff: events.jsonl also carries session.usage_checkpoint with totalNanoAiu, written during the run. On my sessions it is cumulative for the whole session and strictly monotonic across resume legs (checked on sessions with 2 and 3 shutdown legs — no reset at the leg boundary, unlike the rollup counters). It is a coarser signal than the store's per-request rows, so it is no substitute, but it is a JSONL-side live billed total if a fallback is ever wanted for surfaces or CLI versions where session-store.db is not available. It also covers a fourth rollup loss mode next to the three in the PR body: a session that simply has not exited yet has no rollup at all. Measured mid-work on an open session, CodeBurn showed $0.19 against 128.89 credits ($1.29) already billed — since today is normally read while a session is open, that is the common case rather than the crash case.

Happy to run a calibration branch against this machine if that is useful.

[kelchm]

@vidoluco — thank you, this is excellent confirmation.

On the open-session case (your fourth mode), this branch is designed to handle it: rows are written per request, so today reads an open session’s input/cache from the store without waiting for a rollup. Could you confirm that against the current head? Mid-session, today should match the store’s row sum, missing only not-yet-journaled output. If you still see a gap like your $0.19-vs-$1.29 one, that’s a bug we want—the surface (one-shot CLI vs the resident desktop/menubar process) and whether the store was mid-write would narrow it quickly.

session.usage_checkpoint is a real find: a billed total for store-less sessions and a cross-check on the residuals this PR synthesizes. It seems like it would pair well with #890 rather than widening this diff. If you can verify that the checkpoint stays monotonic on your multi-leg sessions against sum(total_nano_aiu) up to each checkpoint’s timestamp, the follow-up gets much easier.

And yes to the calibration offer—the branch is now rebased onto current main; a release-vs-branch A/B on your history, plus the per-session ratio table you already produced, would be excellent.

[ozymandiashh]

Thanks for the detailed follow-up here. I checked the current head (a5a0ddae) and the main path is in much better shape. I still hit three cases that I don't think we can safely ship yet.

1. The cache-version bump can drop old Copilot history.

The new session-store-v2 fingerprint in src/session-cache.ts forces a reparse. On a fingerprint change, though, getOrCreateProviderSection only carries forward durable entries whose source path has disappeared. Copilot's DB paths normally keep existing, even after old rows have been pruned.

I reproduced this by seeding the old cache with one already-pruned OTel call and leaving an existing, valid, but empty agent-traces.db in place. After parseAllSessions(undefined, 'copilot'), the call count went from 1 to 0 in both the served result and the persisted cache. At that point the history is gone for good because it is no longer in the DB.

The PR body already mentions this upgrade hazard, but losing historical usage on upgrade feels too risky to accept. Could we either avoid the fingerprint bump or make the migration reparse the live DB and union it with the durable cached entries? An extant-but-pruned DB regression would pin this nicely.

2. A partial store snapshot can undercount a compacted session, and the bad result can be sealed.

The residual code subtracts every store row between two shutdown timestamps from that shutdown leg. That works only if all those rows belong to the leg, but the rollup resets at in-session compaction.

Here's the small repro I used: row A has 100 tokens before compaction, the only shutdown rollup has 200 tokens for post-compaction request B, and B's row is not yet present in the first store snapshot. The current code reports 200; the real total is 300. Once B's row appears, the next refresh reports 300.

The worrying part is the daily cache. In a cross-day version of the same test, the first hydration was considered complete and sealed {Aug 10: 500, Aug 11: 100}. After the missing row arrived, the live parser correctly had {Aug 10: 600}, but the persisted daily result stayed unchanged.

This looks like a snapshot-ordering/fencing gap between the store and JSONL sources. I think the store needs to be read after the shutdown snapshot, or re-read/fenced against it. Please add regressions for compaction plus progressive row arrival, including the real ensureCacheHydrated path across a day boundary.

3. Sync can permanently overcount as reconciliation changes.

Residual values change as store coverage grows, but their dedup key does not. The sync ledger sends a key once and suppresses it forever, while OTLP derives the span ID from that same key.

In my repro, pass one sends 10k tokens as a 4k row plus a 6k residual. On pass two, the local total is still 10k (4k + a new 3k row + a residual that shrank to 3k), but the old residual is suppressed and the new row is appended. The receiver now has 13k. A pairing-state change has the same kind of problem: one request locally can become two ordinary spans remotely because the supplementary weight is not serialized.

I realize this is tracked in #988, but sync exports this serve set today, so leaving it out of scope still means shipping receiver totals that can become permanently wrong. Either the sync contract needs update/replacement semantics here, or these mutable residual/supplementary calls should stay out of sync until #988 lands.

CI and the targeted suites are green; these are cross-source transition cases that the current tests don't cover. Once these three paths are handled, I'm happy to take another look.

[ozymandiashh]

Would you like us to take this PR over and finish the remaining fixes? We’re happy to handle the three blockers above and add the missing regression coverage if that would help.