fix(session-cache): stop /clear from listing a phantom session - #157
Conversation
/clear does not stay in the current transcript: the CLI opens a NEW jsonl and writes only bookkeeping into it — a local-command caveat, the <command-name>/clear</command-name> record, empty stdout. The indexer took that record as the session summary, so every /clear added a "2 msgs" sidebar entry titled "/clear clear </com", and every session actually started by /clear kept that title instead of its first prompt (53 of the 99 transcripts in one project folder here). The first-prompt scan now classifies user records. Local-command bookkeeping is skipped, <local-command-stdout> included — the CLI writes a command's own output back as a user record, which titled every session where /model preceded the first prompt. A bare slash-command record is only a fallback title, used when the transcript also holds an assistant turn (a headless "/code-review high" run); with no real turn both readers return null and nothing is indexed, the same way a brand-new session stays out of the sidebar until its first prompt. Migration v9 purges the rows the old parser wrote, plus the cache_meta gate of their folders so the next reconcile re-reads exactly those files. Neither kind of row can heal on its own: a phantom sits on a file that never changes again, and a mistitled one keeps its summary because the header-only refresh path only overwrites a summary it can re-derive.
devsuitup
left a comment
There was a problem hiding this comment.
Reviewed against main @ 9ae4b9a in a dedicated worktree. Full suite run locally: 586 pass / 0 fail / 7 skipped (the skips are all linux only sandbox-wrapper tests, unrelated). The reported bug is real, the fix works, and I measured it on real data: replaying both parsers over the 346 transcripts in this machine's ~/.claude/projects gives 0 sessions lost, 0 gained, 1 retitled — and that one retitling is an improvement (<command-name>/model… → the actual prompt). The migration is surgical rather than a full reindex, and I confirmed every caller of the two readers handles the new null return (4 call sites for readSessionFile, 1 for readSessionDisplayHeader, all guarded).
Also checked, since the branch is 19 commits behind main: git diff --name-only <merge-base> main touches none of the files in this PR, and the caller set is identical on both branches. The merge is clean semantically, not just textually.
Three things I'd like fixed before merge.
1. db.js:244 — v9 leaves orphaned session_metrics rows, permanently
The migration deletes from session_cache and the three search tables, but not from session_metrics. The repo's own convention disagrees: deleteCachedSession() (db.js:587) starts with stmts.metricsDeleteBySession.run(sessionId).
For real-but-mistitled sessions this is harmless — they get reindexed and replaceSessionMetrics() does delete-before-insert. For the phantom rows it is not: readSessionFile() now returns null for those files forever, so nothing ever passes over them again. getDailyMetrics() (COUNT(DISTINCT sessionId), SUM(messageCount)) and getTotalCounts() keep counting those sessions and their messages indefinitely. On the reporting machine that's ~53 sessions.
Reproduced on a throwaway DB (v8 seeded with a phantom row + its metrics, then loaded):
{"cache": [], "metrics": [{"sessionId":"phantom","messageCount":2}], "version": "9"}This is the one finding with a closing window: once db_version = 9 is written, only a v10 can clean it up. It's one line, inside the existing inner try (since session_metrics is created further down at db.js:308 and may not exist on a fresh DB).
2. read-session-file.js:120 — the fix covers only one of the two tag layouts the CLI emits
SLASH_COMMAND_RE is anchored on <command-name> being the first tag. The CLI produces both orders. From real transcripts on this machine:
<command-name>/compact</command-name> <command-message>compact</command-message>…
→ {"kind":"command","text":"/compact"} ✅
<command-message>auto-compact</command-message> <command-name>/auto-compact</command-name>
→ {"kind":"prompt","text":"<command-message>auto-compact</command-message>\n<command-name>…"} ❌
For the <command-message>-first layout, classifyUserText() returns prompt carrying raw XML — which is exactly the defect this PR removes for the other layout: it becomes the summary, and cleanDisplayName() truncates it to the same kind of fragment (/pre-compact pre-compact </comm). The v9 purge doesn't catch them either, since summary LIKE '<command-name>%' can't match a string starting with <command-message>.
In fairness: no transcript on this machine starts with a record in that layout (I scanned the first user record of all 346), and the commands involved are hook commands, not /clear. But the premise of the fix is a tag order, and there are two.
Suggested: identify the command record by the presence of <command-name> together with <command-message>/<command-args> rather than by position, and widen the v9 LIKE clause to match.
3. read-session-file.js:119 — LOCAL_COMMAND_RE is unanchored
The regex matches the marker anywhere in the text, and this PR adds <local-command-stdout> to the alternation. A user prompt that merely quotes that string — pasting a transcript excerpt, which happens in this repo — is classified skip. With no other user turn, both readers return null and the session is never indexed: invisible in the sidebar, absent from search.
To be clear, the PR doesn't create this: <bash-input> / <bash-stdout> / <local-command-caveat> are already unanchored on main, and I confirmed main already loses such a file. The PR widens it by one token.
Worth closing anyway, because the near-miss is real: across 1707 real user records, exactly one matches the unanchored form without matching an anchored one — and it's a prose prompt, not bookkeeping. Anchoring costs nothing and closes the pre-existing hole too:
const LOCAL_COMMAND_RE = /^\s*<(bash-input|bash-stdout|local-command-caveat|local-command-stdout)>/;Measured: anchored vs unanchored differ on exactly that 1 record out of 1707. No real bookkeeping record is missed by anchoring.
Minor
db.js:241— half the purge condition is untested. DroppingOR summary LIKE '<local-command-stdout>%'leavestest/db-purge-command-summaries.test.jsgreen (2 pass / 0 fail), because it only seeds<command-name>summaries. Worth one seeded row. For what it's worth, the parser tests are mutation-resistant — three separate mutations each turned them red.db.js:243— the migration isn't transactional, and better-sqlite3 autocommits each.run(). A crash between thesession_cachedelete and the search-table purge leaves orphanedsearch_map/search_content/search_ftsentries forever, since the replay hitsif (bad.length === 0) return;. Reproduced with a simulated crash — the FTS query still returns the ghost id. Low impact in practice (the renderer only uses search results as a filter set over rendered sessions, so an orphan id matches nothing and shows no error), so it's dead weight in the index rather than a visible bug. Adb.transaction(...)around the v9 body would close it..ai/contexts/session-cache.md:54— "/clearand/modeldo not stay in the current transcript" is not true for/model. In real transcripts it lands mid-file (user record #51, #104, #2 in three different sessions). The code is still correct — a/modelat position #51 is never a summary candidate, a real prompt precedes it — but the written rationale is wrong, and that's the kind of thing a later reader takes as settled.test/db-initial-scan-marker.test.js:125— comment still says "migration v8 must not run again" while the line above was bumped to 9.
The two modified existing tests are pure expected-version bumps ('8' → '9'); no assertion was loosened or removed — checked line by line.
Nice catch on the /model variant while you were in there, and thanks for including the migration rather than leaving the already-written rows to rot.
The CLI writes both tag orders: <command-name> first for /clear, but <command-message> first for /auto-compact and /pre-compact. Requiring <command-name> to open the record left the second form classified as a prompt, so the raw XML became the summary and cleanDisplayName truncated it to "/pre-compact pre-compact </comm" -- the very defect this branch removes. A record is now recognised by a <command-name> tag sitting next to one of its siblings, wherever it sits. The bookkeeping test is anchored for the opposite reason: matching anywhere, it skipped a prompt that merely quotes <local-command-stdout> (a pasted transcript excerpt), and with no other user turn the whole session went unindexed -- invisible in the sidebar and absent from search. Over 1707 real user records the anchored and unanchored forms differ on exactly one record, and that one is prose.
Three gaps in the purge of the rows the pre-fix parser wrote: - session_metrics kept its rows. deleteCachedSession() drops them first for good reason: readSessionFile now returns null on a phantom's file forever, so nothing ever revisits it and its metrics keep inflating getDailyMetrics() and getTotalCounts() for the life of the database. - summaries opening on <command-message> went unmatched, although the CLI writes that tag order too (/auto-compact, /pre-compact). - the migration runner wraps nothing in a transaction and better-sqlite3 autocommits every statement, so an interruption between the session_cache deletes and the search-table purge was permanent: the relaunch is already at db_version 9 and its SELECT no longer matches the rows it dropped, leaving those search entries orphaned for good. The purge test only seeded <command-name> summaries, so dropping either of the other two LIKE branches left it green; it now seeds all three shapes plus their metrics, and a second test interrupts the purge midway and asserts no search entry outlives its session_cache row.
/model does not open a new jsonl. It is written into the transcript already open -- user records devsuitup#51, devsuitup#104 and devsuitup#2 in three transcripts on this machine. The code is right either way, since a /model at devsuitup#51 is never a summary candidate, but the written justification was not. Also records the two tag orders a command envelope comes in, why the bookkeeping test is anchored, and what migration v9 now purges. The relaunch comment in db-initial-scan-marker.test.js had its version number bumped to 9 without saying what that implies: at 9 no migration runs again, the v8 marker backfill included.
|
I've pushed the review fixes onto this branch directly (
The same commit anchors
The purge test only seeded
Two things I deliberately left alone:
State after the push: 598 tests, 591 pass, 0 fail, 7 skipped (the skips are Worth adding: replaying both parsers over the 346 transcripts in my |
devsuitup
left a comment
There was a problem hiding this comment.
Review fixes pushed to this branch and verified: 598 tests, 591 pass, 0 fail; eslint 0 errors. Each functional fix has a test that fails when the fix is reverted. Also replayed both parsers over 346 real transcripts: 0 sessions lost, 1 retitled for the better.
Symptom
Clearing a session adds a session to the sidebar. Reported with this view — two entries titled
/clear clear </com, the top one appearing the instant/clearran:Cause
/cleardoes not stay in the current transcript — the CLI opens a new.jsonland writes only bookkeeping into it:{"type":"user","isMeta":true,"message":{"content":"<local-command-caveat>Caveat: …</local-command-caveat>"}} {"type":"user","message":{"content":"<command-name>/clear</command-name>\n<command-message>clear</command-message>\n<command-args></command-args>"}} {"type":"system","subtype":"local_command","content":"<local-command-stdout></local-command-stdout>"}readSessionFile()skipped the caveat but accepted the<command-name>record as the summary. Two consequences:messageCount2, so it is indexed and listed — a session the user never started./clearis titled from that same record;cleanDisplayName()strips the tags and leaves the truncated fragment/clear clear </com. 53 of the 99 transcripts in one project folder on the reporter's machine are titled this way.Same class, found while fixing it:
<local-command-stdout>also arrives as atype: "user"record, so a/modelbefore the first prompt titled the session<local-command-stdout>Set model to Sonnet 5….Fix
read-session-file.jsnow classifies each user record (classifyUserText()), shared by both readers:skip<bash-input>,<bash-stdout>,<local-command-caveat>,<local-command-stdout>command<command-name>/x</command-name>+<command-args>promptA
commandrecord becomes the title only when the transcript also holds an assistant turn — so a headless/code-review highrun still reads as/code-review highinstead of raw XML. With no real turn, bothreadSessionFile()andreadSessionDisplayHeader()returnnulland nothing is indexed, exactly how a brand-new session stays out of the sidebar until its first prompt.Migration v9
Rows written by the old parser cannot heal on their own:
h.summary || cachedEntry.summary, and the header read now declines to re-derive a bookkeeping summary.v9 therefore purges rows whose summary starts with
<command-name>or<local-command-stdout>(plus theirsearch_map/search_content/search_ftsentries) and drops thecache_metagate for their folders, so the nextreconcileCacheFromFilesystem()re-reads exactly those files — every other file in the folder still hits thefileMtimefast path. No full re-index, and a no-op on installs that never hit the bug.Verification
Against the reporter's real
~/.claude/projectstree: the phantom (21e03eba, 2 KB) is no longer indexed, and every other session in that folder now shows its own first prompt instead of/clear …or<local-command-stdout>….test/read-session-file-slash-command.test.js— the bookkeeping-only transcript,/clear+ real prompt, command-as-fallback title,/modeloutput,<scheduled-task>precedencetest/db-purge-command-summaries.test.js— v9 purges only the affected rows, clears only the affected folder gate, leaves the search tables clean, and is a no-op otherwise.ai/contexts/session-cache.md— records the rule and why the migration existsTakes effect on next launch; the affected folders re-index once at that startup.