Repository navigation
Session History: preserve exact metadata while limiting closing previews - #28925
brennanb2025 wants to merge 5 commits into
Conversation
…ts that accept partial rows A list row only needs the opening and closing records of a transcript. A new partial read mode feeds the existing per-agent fold the first 256 KB and last 64 KB, marks the row partial, and never publishes to transcript consumers. Hosts serve such rows only to clients advertising aiVault.list.partial-rows.v1; nothing advertises it yet, so behaviour is unchanged.
… gap into their own modules
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🟠 High · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 23 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly summarizes the main change: preserving exact session metadata while limiting closing preview collection. Description check The description is detailed and covers the user impact, implementation, rationale, measurements, testing, compatibility, and current validation limits. It identifies that the PR remains draft and that…
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
brennanb2025/scan-bounded-read
- Resolve merge conflict in branch
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
brennanb2025
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
39702b7f-2843-4bb9-acce-47df02d23c32
📒 Files selected for processing: 16
src/main/ai-vault/session-parse-cache-persistence.test.tssrc/main/ai-vault/session-parse-cache-snapshot-serialization.tssrc/main/ai-vault/session-parse-cache-store.tssrc/main/ai-vault/session-partial-transcript-read.test.tssrc/main/ai-vault/session-scanner-accumulator.tssrc/main/ai-vault/session-scanner-codex-parser.tssrc/main/ai-vault/session-scanner-first-user-prompt.tssrc/main/ai-vault/session-scanner-jsonl-reader.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner-primary-parsers.tssrc/main/ai-vault/session-scanner-types.tssrc/main/ai-vault/session-transcript-reader.tssrc/main/ipc/ai-vault-scan-coalescing.test.tssrc/main/ipc/ai-vault.tssrc/shared/ai-vault-partial-rows-capability.tssrc/shared/ai-vault-types.ts
🚧 Files skipped from review as they are similar to previous changes: 4
- src/shared/ai-vault-partial-rows-capability.ts
- src/main/ai-vault/session-parse-cache-store.ts
- src/shared/ai-vault-types.ts
- src/main/ai-vault/session-scanner-parse-cache.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| if ( | ||
| role === 'user' && | ||
| seedFirstUserPrompt !== false && | ||
| !accumulator.firstUserPrompt && | ||
| seedFullFirstUserPrompt() | ||
| ) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the self-call with shouldCaptureFullFirstUserPrompt().
At Line 128, the guard calls seedFullFirstUserPrompt() with no arguments. This recursive call is wrong in two ways:
- Inside the call,
roleisundefined, so the function returnsvoid. The outer condition is then always falsy. As a result,firstUserPromptis never set, even in'full'capture mode. The copyable first prompt is lost for every parser that uses this helper. - TypeScript does not allow a missing required argument. It also does not allow a truthiness test on a
voidexpression (TS1345). The node typecheck should fail.
The old accumulator code gated on the capture mode. shouldCaptureFullFirstUserPrompt is defined in this file for that purpose.
🐛 Proposed fix
!accumulator.firstUserPrompt &&
- seedFullFirstUserPrompt()
+ shouldCaptureFullFirstUserPrompt()
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| role === 'user' && | |
| seedFirstUserPrompt !== false && | |
| !accumulator.firstUserPrompt && | |
| seedFullFirstUserPrompt() | |
| ) { | |
| if ( | |
| role === 'user' && | |
| seedFirstUserPrompt !== false && | |
| !accumulator.firstUserPrompt && | |
| shouldCaptureFullFirstUserPrompt() | |
| ) { |
|
Session History rebuilds spend CPU extracting previews from large transcripts. Skipping the middle of a transcript can also lose a renamed title or the latest working folder. This revision keeps the complete metadata parser and limits only closing-preview extraction for explicitly capable clients; bundled clients keep exact rows. Not ready; remaining in draft at
No broader benchmark or weaker metadata behavior is being added to rescue the result. The earlier bounded-byte performance claims are superseded. Runtime QA, a useful measured result and final conflict/CI validation remain required before this PR can leave draft. |
|
Closing this implementation under the performance PR guidance: free performance wins, or tradeoffs justified by a substantial benefit. I reviewed the full production diff at The corrected five-run comparison already documented in this PR establishes the tradeoff, rather than merely leaving it unmeasured:
Given those costs and that result, this approach does not meet the bar to land. Faster Session History remains a useful objective for a different implementation. This is not a closure for merge conflicts, missing QA or draft status, and the earlier two-window measurements should not be used to justify its client follow-ups. |
ELI5
Session History can lose a renamed title or the latest working folder when a large transcript is sampled from its opening and closing records. This host change reads every record with the existing parser and limits only the closing preview work, so session identity, location, model, counts, prompts and timestamps stay exact. Bundled clients do not yet request shortened previews; preserving their existing behavior is required before this PR leaves draft.
What Changed
Large Claude and Codex transcripts can produce a capability-gated row with previews collected only from complete records starting in the final 64 KiB. The existing parser still sees the entire transcript; appends resume from its validated cursor. These list reads stay separate from exact search consumers, and an ordinary caller upgrades a shortened-preview cache entry through a complete read.
The read requirement is included in both the list cache and outer request-coalescing keys. The existing parse-cache schema advances to 5 so this implementation rejects previously sampled rows whose metadata may be incomplete. The wire capability stays optional, preserving mixed client and host versions.
Why
Titles, directories and other mutable fields can be recorded anywhere in a transcript. A newer closing timestamp cannot prove the skipped middle contains no relevant update. Reusing the complete parser removes that failure class without another metadata parser or field-specific fallback rules.
The intended difference from the common pattern is preserving the existing complete derivation for imported CLI histories: no verified alternate metadata source covers all supported fields on every execution host. Preview work alone is bounded; byte reads remain complete. The earlier two-window speed and byte measurements are superseded and must not be used as evidence for this implementation. Corrected measurement failed the agreed usefulness requirement, so this PR remains draft.
Measurement result: not useful enough to land
Five alternating fresh-process runs on the original measurement host, with a 250-row scan limit and identical synthetic data, produced these cold medians:
All 30 cold/warm/append row comparisons match, excluding only the intended preview/coverage fields for capable callers. The elapsed saving is 220.165 ms, about 47% of the earlier 470 ms gain, and CPU does not improve. This fails the agreed usefulness requirement. These are process/cache-cold scans, not cold physical storage; all transcript bytes are still read. The earlier bounded-byte claims are superseded.
A search-enabled sample ran for about 10 seconds but both indexes were still working (694/1200 and 817/1200 files). It does not prove completed search timing or total-work parity. No broader corpus or weaker metadata behavior was added to rescue the result.
Linked Issue
N/A (internal performance work).
Visual Proof
Blocked: none of the available remote QA hosts had safe resources for an Electron launch; no rendered comparison or screenshots were produced.
Testing
The normal pre-commit hook passed on bd6c391. Focused regression cases cover whole-row parity except preview coverage, aligned and crossing cutoff records, unfinished records completed by append, consumer silence and exact upgrades, cache reload and request coalescing. Windows targeted execution on bd6c391 collected 67 tests: 65 passed, including all partial-read, persistence, IPC coalescing and full-prompt tests and the RPC capability case. Two existing RPC assertions expect POSIX separators and failed on Windows; the same two failures reproduce from unchanged main test source. Remote Electron QA and current-head full CI remain unverified; prior-head results do not validate this rewrite. GitHub currently reports mergeability as unknown, and no current-head checks have registered. The final main synchronization is deferred because the PR is otherwise not ready.
Windows, WSL, SSH and folder scopes retain the existing execution-host stream path. Runtime evidence for those paths remains pending. No saved field, setting, new wire operation or client default is added.
Review
Round one found incomplete metadata derivation and record-boundary loss. A design review selected the complete fold with bounded preview collection, and a fresh pre-code verifier approved it. Independent round-two source reviews confirmed the metadata, cache and request-isolation fixes, then found a wrong predicate in the moved full-prompt capture function and a concurrency fixture using the wrong scanner boundary. A fresh independent design review unanimously kept the mechanism and authorized only those two narrow corrections. Both narrow corrections are pushed in bd6c391. Fresh independent source verification agrees both corrections are fixed. The benchmark below fails the agreed usefulness requirement. Remote rendered validation and exact-head CI remain unverified; this PR is not ready.
Agent skill upstream boundary
Checklist