fix(kiro): stop Kiro CLI token totals inflating on every sync (#65)#70
Merged
Conversation
added 5 commits
July 20, 2026 05:25
`readSqliteJsonRows` returns `[]` for five materially different outcomes:
bad arguments, an unstattable path, an absent database, a database that
read cleanly and holds no rows, and a database that exists but could not
be read at all. Callers that only ADD what they find may treat all five
alike — the worst case is doing nothing.
A caller that RECONCILES cannot. Applying the delta between what it reads
now and what it recorded before turns "I could not read it" into "every
row was deleted", and subtracts real accounting data. The upcoming Kiro CLI
ledger reconciles, so it needs to tell those apart and bail out.
Adds `readSqliteJsonRowsWithStatus` returning `{ ok, reason, rows }`.
`ok` is true only when `rows` faithfully represents the database.
`readSqliteJsonRows` becomes a one-line wrapper over it, so the seven
existing call sites keep their exact prior behavior — the six pre-existing
tests pass untouched, and a new test pins that the rows-only API still
cannot distinguish empty from missing from unreadable.
`unreadable` (no sqlite backend at all) and `query-failed` (a readable
database, an unsatisfiable query) are separate reasons. The module already
made that distinction internally to decide whether to warn; collapsing it
would have discarded it. Every consumer today treats both as "do not
reconcile", but they call for different remedies.
No production behavior changes in this commit.
Independent QA (Codex, xhigh) found three defects in the first cut. `stat-failed` could never occur. `fs.existsSync` swallows every error and answers false, so a directory we lack +x on was indistinguishable from a path that was never there — and the `catch` guarding it was unreachable. Verified by running it: number, object, boolean, symbol, function and an ENOTDIR path all return false, none throw. Switched to `statSync` with `throwIfNoEntry: false`, mapping ENOTDIR to `missing` (a path component is a file, so the target genuinely cannot exist) and everything else to `stat-failed`. The reason is now reachable, and a test drives it through a real chmod-000 parent directory. `query-failed` was assigned to every failure that was not a missing backend — so a corrupt image, `SQLITE_BUSY`, `SQLITE_CANTOPEN` or a timeout were all reported as "your statement did not apply", contradicting the comment justifying the split. Classification is now positive: only an identifiable statement error (no such table/column/function, syntax or parse error) is `query-failed`; anything unrecognized stays `unreadable`. Warning eligibility is untouched and remains independent of this. The header comment still said an empty array meant five situations. It means six. Test gaps QA identified, now closed: invalid-args asserts the whole result object rather than just `reason`; the warn-once test exercises two distinct label/path pairs, so collapsing the keyed cache into one global flag would now fail it; corrupt-or-locked is pinned to `unreadable` with stderr asserted silent. 16/16 in this file. Full suite 791/795, failing names byte-identical to main.
`readKiroCliRequests` reads the whole `conversations_v2` table with no age
filter, while `cursors.kiroCli.requests` was pruned at 90 days AND capped at
20,000 entries. The memory window was strictly smaller than the read window,
so a request evicted by either cap was re-ingested on every subsequent sync,
forever. Four syncs of one 91-day-old conversation gave 100 -> 200 -> 300 -> 400.
Fix: keep that same per-request record, stop pruning it, and only ever add the
growth — `delta = max(0, current - recorded)`, per request.
Two rules carry the whole design, and every defect found while getting here
came from breaking one of them:
1. Never prune. Anything dropped from the map is re-added in full next sync;
that IS this bug. Dropping only the age cap while keeping the count cap
just converts an age-triggered recount into a volume-triggered one, which
is why the issue's "rejected options" list did not actually cover this.
2. The delta is clamped at zero — this parser never subtracts. Attribution is
only ever needed to justify a DECREASE, so never decreasing removes the
need for it:
- a shrinking read (deleted session, corrupt file, unreadable or vanished
database, partial parse) yields a zero delta, so no health gate is needed;
- `source="kiro"` buckets are shared with the Kiro IDE parser, and since we
only add we never need to know the IDE's share, so `cursors.hourly` —
shared by 11 parsers — is untouched.
Keying on `message_id` rather than `request_id` keeps identity stable when
kiro-cli flushes a live session into SQLite under a new request_id. That is
what the TASK-007 cross-source retraction pass used to reconcile by hand; it
walked the age-capped cursor whose eviction caused this bug, and is removed.
Behavior changes worth knowing:
- A genuine downward correction never propagates.
- A request that moves to another bucket leaves its earlier contribution in
the old one (a mislabel, not a double count).
- Upgrading freezes any inflation already accumulated rather than repairing
it; repair would need the CLI's share of a shared kiro bucket, which the
hourly state does not record.
- State grows with Kiro's own history instead of being bounded. That is the
deliberate trade for correctness and belongs in the release notes.
- A degraded SQLite read now emits an ungated stderr line. There is no gate —
a degraded read is already safe — but "no gate needed" is not "no signal
needed", and `warnSqliteUnavailable` fires only for a missing backend.
Tests: 803 pass. Eight boundary tests were added and six were verified to fail
against the 6c04040 baseline; the two that also pass on baseline are labelled
in-code as regression guards, not as proof, because they pin behavior that a
rejected subtracting design broke rather than behavior this change introduces.
Two earlier approaches were built and rejected under review before this one;
the second is archived on `fix/65-kiro-ledger` with its findings. Green gates
were not evidence for either: one was 796/796 with ci:local passing while
carrying a CRITICAL and three HIGH defects.
Refs #65
Review of 955ea5c reproduced three defects, two of them permanent data loss. All three share one root: the code INFERRED whether a request had already been counted instead of recording it. 1. Adoption was a single latch for the whole sync, so it froze every request it saw — including ones that had never reached a bucket. Everything created between the last pre-upgrade sync and the first post-upgrade data sync was lost permanently. It had no choice: the pre-watermark cursor keys `request_id || message_id` while this code keys `message_id || request_id`, so `prior` missed essentially every legacy entry and the blanket freeze was the fallback. Now decided per request, probing BOTH key schemes, and the old cursor's own prune policy resolves the ambiguous case: no record and a bucket older than the legacy 90-day cap means it was pruned (counted but forgotten) so freeze it; no record and a newer bucket means the old cursor would still be holding it, so it is genuinely new and is counted in full. 2. `conversation_count` drifted permanently below the token totals. A request first seen with zero tokens recorded no conversation, and when Kiro later rewrote it with real data `prior` existed, so it never contributed one. The pre-#65 comment documented that exact sequence as normal Kiro behavior. `counted` is now stored on the record rather than inferred from `!prior`. 3. The main path stamped `watermarkVersion` unconditionally while the early-return path deliberately refused to, with a comment naming the hazard. One degraded read — a locked database while a live session file keeps the main path running — therefore ended the migration early, and the next healthy sync re-counted the entire corpus and emitted it as an absolute total. Stamping now requires the migration to have actually completed. Also removes `clampAndCapKiroCliState` and its two cap constants, which had no callers left. No test asserted capping, so nothing was encoding the bug. Tests: 806 pass. Three regression tests added, each verified RED against 955ea5c. The third initially passed against the commit it was meant to catch because it had no session file and so took the empty-flat early return instead of the main path — it now carries one, and a comment saying why. Known and unresolved, for the PR body: keying on `message_id` collapses two requests that share one, which would silently halve them. The mechanism is proven; whether real kiro-cli emits such rows is not, and the repo has no schema documentation. Verify against a real database before merge. Refs #65
Closes the one open question this PR was carrying. `user_turn_metadata.requests` is an ARRAY per turn while `message_id` plausibly identifies the turn, so two distinct requests can share one identity. Last-write-wins silently halved them, and the loss was unrecoverable because the watermark then kept only the survivor: 3000/1500 of real usage counted as 2000/1000. It could not be settled by inspection — the repo has no schema documentation, every fixture is one-request-per-turn, and there is no Kiro CLI install to query. So rather than ship a documented maybe, this accumulates per-identity while keeping each underlying request's own share, so a repeated row replaces rather than adds. Cross-tier duplicates of the same turn are already removed by the turn-granular filter, so anything still sharing an identity here is genuinely distinct work. Safe under both hypotheses: with one request per turn it reduces to exactly the previous single-entry behavior, which is why the other 806 tests are unchanged. Regression test verified RED against deaa292 (2000/1000 vs 3000/1500) and also asserts stability on re-sync, so accumulation cannot itself become a re-add. Refs #65
This was referenced Jul 20, 2026
pitimon
pushed a commit
that referenced
this pull request
Jul 20, 2026
The fact file records evidence as file:line, so merging #70 shifted the line numbers this branch's snapshot was pinned to and docs:openwiki:check failed on a gate unrelated to this branch's change. Regenerated; no source change.
pitimon
added a commit
that referenced
this pull request
Jul 20, 2026
* perf: cache Codex context parses per file instead of per scan The Codex Context Breakdown cache keyed on the GLOBAL maximum session-file mtime, so appending to whichever session is currently active invalidated the aggregate for every unchanged historical file and forced a full rescan. The reporter measured 2.19s cold against 2,066 files, and 9.1s under concurrent dashboard load, for what was a single-line append. Adds a per-file parse cache consulted when the aggregate cache misses, so that miss now costs one file rather than all of them. The aggregate cache is deliberately kept as the fast path: replacing it outright would turn every no-change request into "stat every file and recombine", regressing the case that matters most under dashboard load. `top` is deliberately absent from the per-file key. Truncation to the top N happens at merge time, so one cached parse serves every `top` value; a test pins this so it does not get "fixed" into the key later. File identity reuses the predicate rollout.js already relies on in 19 places — inode, size, mtimeMs, plus a sha1 of the first 256 bytes to catch a rotator that reuses the inode. Moved to src/lib/file-identity.js and imported back into rollout.js under the same name, so those 19 call sites are untouched. The comparison is equality, never ordering: a clock moving backwards or a restored mtime must read as changed. Verified the merge helpers build fresh targets and only read from their source rows, so a cached parse cannot be mutated by a later merge — the aliasing hazard that per-file caching would otherwise introduce. Tests were negative-controlled: with the cache lookup disabled, the two behavioral tests fail and the three correctness tests still pass, which is the correct split. Refs #62 * perf: cache Claude category parses per file instead of per scan Same fix as the Codex side, but this one had a real obstacle: the scan kept a single `seenHashes` dedup Set shared across every file, so whether a message in one file counted depended on what another file had already contributed. Summing independent per-file results would double count every cross-file duplicate — and #62's own acceptance criterion is that the aggregate stays identical to a clean full scan. categorizeSessionFile now parses into its own accumulators and returns them, with a file-local dedup set plus the list of hashes it claimed. The caller merges in `files` order and inserts every hash into one global Set; on any collision it discards the merge and re-runs a full sequential scan with a shared dedup set, which is exactly the old behavior. A scan of 6,529 real session files found zero cross-file duplicates, so the guard is expected to be dead code — it exists so correctness does not depend on that measurement being true of everyone's corpus. A test forces the collision and asserts the duplicate is counted once. Merging in `files` order rather than worker-completion order also makes the result deterministic, which the old shared-accumulator scan was not. Measured on a copy of 800 real session files (108 MB): cold scan 292 ms -> 347 ms no change (L1) 2 ms -> 2 ms after one append 274 ms -> 16 ms Cold is ~19% slower — one extra stat and 256-byte head read per file, plus building per-file structures. That is the trade: the append path, which is what #62 reports and what the dashboard actually hits while a session is live, gets 17x faster. Verified the refactor changes no real-world number: the full breakdown over ~6,500 local session files is byte-identical to main across a fixed date range. An earlier comparison appeared to differ, but that was the live corpus being appended to between the two runs, not the code. Parse instrumentation is opt-in — left always-on it would grow one Map entry per session file forever on a long-running server, purely to serve tests. Fixes #62 * chore: refresh openwiki source facts after #70 The fact file records evidence as file:line, so merging #70 shifted the line numbers this branch's snapshot was pinned to and docs:openwiki:check failed on a gate unrelated to this branch's change. Regenerated; no source change. --------- Co-authored-by: itarun.p <itarun.p@somapait.com>
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.
Fixes the unbounded inflation in #65.
The bug
readKiroCliRequestsreads the wholeconversations_v2table with no age filter, whilecursors.kiroCli.requestswas pruned at 90 days and capped at 20,000 entries. The memory window was strictly smaller than the read window, so a request evicted by either cap was re-ingested on every subsequent sync, forever. Four syncs of one 91-day-old conversation:100 → 200 → 300 → 400.The fix
Keep that same per-request record, stop pruning it, and only ever add the growth —
delta = max(0, current - recorded), per request.Two rules carry the design:
source="kiro"bucket never needs to be known, socursors.hourly— shared by 11 parsers — is untouched.Identity is
message_id || request_id, which stays stable when kiro-cli flushes a live session into SQLite under a newrequest_id. That is what the TASK-007 cross-source retraction pass reconciled by hand; it walked the age-capped cursor whose eviction caused this bug, and is removed.Behavior changes worth knowing
cursors.jsonis read and rewritten whole on every sync, so that is parse+serialize cost on every sync, not just resting footprint. This is the deliberate trade for correctness — pruning is the bug.warnSqliteUnavailablefires only for a missing backend.Open question — please check before merging
Keying on
message_idsilently halves two requests that share one (reproduced: 3000/1500/2 counted as 2000/1000/1, unrecoverable). The mechanism is proven; whether real kiro-cli emits multiple entries per turn sharing amessage_idis not — the repo has no schema doc and every fixture is one-request-per-turn.request_id-first is collapse-safe but migration-unsafe;message_id-first is the reverse. Worth one query against a realconversations_v2before merge.Test plan
npm test→ 806 pass, 0 failnpm run ci:local→ exit 0npm run docs:openwiki:check→ 0 findings6c04040baseline or against the previous commit in this branch; the two that also pass on baseline are labelled in-code as regression guards, not proof — they pin behavior that a rejected earlier design broke.message_idquestion above against a real database.package.json, bothproject.ymlMARKETING_VERSION,TokenTrackerWin.csproj) is deliberately not bumped, pending the release decision — same as the other open PRs.How this was arrived at
Three designs were built for this. Two were rejected under review and the second is archived on
fix/65-kiro-ledgerwith its findings, because it is useful evidence rather than a useful solution.Green gates were not evidence for any of them. The rejected ledger was 796/796 with
ci:localpassing while carrying a CRITICAL and three HIGH defects, because its new branches had no coverage. Across the whole effort, four tests were caught passing for the wrong reason — one asserted>=where inflation had to be caught, one asserted<=where erasure had to be caught, one stopped discriminating its own fix after a later refactor, and one took an early-return path that skipped the hazard it targeted. Every behavioral claim in this PR was re-verified fail-before after the final design change; the earlier evidence was discarded.