Repository navigation
Fix unbounded Vault JSONL history scans - #4536
austinywang wants to merge 20 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace unbounded forward JSONL scanning with bounded, directional streaming that returns summaries and stop reasons; apply bounded reverse scans with computed byte/line budgets to antigravity history and preview loading; add tests verifying limit enforcement and stop reasons. ChangesBounded JSONL streaming for antigravity history and preview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Greptile SummaryThis PR replaces unbounded
Confidence Score: 4/5Safe to merge for active sessions; dormant sessions in large history files may now show an empty preview pane with only a truncation banner. The streaming primitive and session-listing path are correct and well-tested. The one real concern is loadAntigravityHistorySynchronously: it now reads tail-first with a hard 12 000-line / 24 MB cap, so any session whose most-recent record sits beyond that window delivers zero turns plus 'Preview truncated' — previously the forward scan would have found those records regardless of file size. This is an intentional performance trade-off, but it introduces a user-visible regression for dormant sessions in vaults with heavy ongoing activity. Sources/SessionIndexView.swift — specifically loadAntigravityHistorySynchronously and whether the shared antigravityHistoryMaximumScanLines constant is the right cap for the preview path. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[forEachJSONLine\nmaxBytes · maxLines · direction] --> B{direction}
B -- forward --> C[streamJSONLinesForward\nread chunks left to right\nflush trailing line at EOF]
B -- reverse --> D[streamJSONLinesReverse\nseek from fileSize backward\nbuffer = chunk + carry]
C --> E[processJSONLineData\ncheck maxLines\n++ linesVisited\nautoreleasepool → body]
D --> E
E -- stoppedByBody --> F[return .stoppedByBody]
E -- maxLines hit --> G[return .maxLines]
E -- nil continue --> H{more data within budget?}
H -- yes --> C
H -- yes --> D
H -- no or EOF --> I{reachedEOF or didReadEntireFile?}
I -- yes leftover --> E
I -- no --> J[return .maxBytes]
I -- yes no leftover --> K[return .completed]
D --> L[shouldProcessReverseCarry\nposition==0 flush\nposition>0 peek byte before window]
L -- is newline flush --> E
L -- not newline skip --> M[return .maxBytes or .completed]
subgraph Antigravity Listing
N[loadAntigravityHistoryEntries\noffset+limit budget] --> A
A --> O[merge metadata\nstable-target early-stop\nbreak across roots]
O --> P[sort by modified\ntiebreak reverseOrderIndex]
P --> Q[dropFirst prefix page]
end
subgraph Antigravity Preview
R[loadAntigravityHistorySynchronously\n24MB 12000 lines reverse] --> A
A --> S[collect matching turns\nreverse order]
S --> T[reverse re-index\ninsert truncation marker at 0]
end
Reviews (11): Last reviewed commit: "fix: preserve antigravity history orderi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/SessionIndexRegisteredAgents.swift`:
- Around line 533-540: The current reverse-scan returns early when
latestBySessionID[sessionId] is non-nil, causing newer-but-partial metadata to
block filling missing fields from older records; update the logic in the loop
that handles latestBySessionID and sessionIDsInReverseHistoryOrder so that when
latestBySessionID[sessionId] exists you merge missing fields (e.g., title, cwd)
from the current metadata into the stored metadata instead of immediately
returning false, and only append sessionId and increment the count when you
first create the entry; ensure the merge stops once all required fields are
populated or after processing older records so the entry is complete before
counting toward the target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2a5d4cd-f041-4aef-8941-486d9df37da8
📒 Files selected for processing (5)
Sources/SessionIndexRegisteredAgents.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftcmuxTests/PiVaultAgentPersistenceTests.swiftcmuxTests/SessionIndexJSONLStreamTests.swift
Dismissed after the requested metadata-backfill fix landed in e6f2e9f, the review thread was resolved, and CodeRabbit reported success on the latest commit.
|
Non-blocking review notes from a follow-up Codex pass — all small, no need to block on them:
Not requesting changes — flagging for awareness. |
|
Empirical confirmation of the codex-review finding above: ran Same result at the red commit I added a one-shot lint that catches this class of bug in seconds. Reproducer from any cmux checkout: ./skills/regression-hunt/scripts/lint-pbxproj-test-wiring.sh --repo-root .On this branch it currently reports: (The latter two are pre-existing on main and unrelated to this PR — filing https://github.com/manaflow-ai/cmux/issues/ separately for those.) The lint script and the broader |
#4562) * ci: lint that every cmuxTests Swift file is wired into pbxproj Catches the class of bug surfaced during the #4529 investigation: a test file added to the worktree without a matching entry in cmux.xcodeproj/project.pbxproj is silently ignored by Xcode and never compiles or runs on CI. Both bot reviews and `xcodebuild test -only-testing:cmuxTests/<TestClass>` pass with "Executed 0 tests" — so the missing wiring is indistinguishable from a clean two-commit red/green regression test until a real user hits the bug the test was supposed to catch. This is the RED commit of a two-commit pattern: introducing the lint immediately flags two pre-existing orphans on main (SessionIndexViewTests.swift and SidebarMarkdownRendererTests.swift) and the new `workflow-guard-tests` step turns red. The follow-up commit wires those two files into the cmuxTests target so the lint goes green. Refs #4559 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: wire SessionIndexViewTests and SidebarMarkdownRendererTests + document pitfalls GREEN commit of the two-commit pattern started in the previous commit. `scripts/lint-pbxproj-test-wiring.sh` (added previously) flagged two pre-existing test files on `main` that have never compiled or run on CI because they were never added to the cmuxTests target: - cmuxTests/SessionIndexViewTests.swift - cmuxTests/SidebarMarkdownRendererTests.swift Both contain real-looking XCTest coverage (Claude local-command-caveat title formatting, markdown inline-attribute preservation). Wiring them into `cmux.xcodeproj/project.pbxproj` so they actually run, which also closes #4559 and lets the new `workflow-guard-tests` lint step go green on this PR. Also adds two CLAUDE.md "Pitfalls" entries based on what fell out of the #4529 investigation: - Foundation/SwiftUI/AttributeGraph/WebKit semantics change silently between macOS versions (concrete `URL.deletingLastPathComponent` example from #4529). Recommends AWS M4 Pro builders for empirical repro and points to the `regression-hunt` skill. - Test files in cmuxTests/ must be wired into project.pbxproj or they're silently skipped. References the new lint script and the PR #4536 incident that surfaced the class of bug. Self-test of `tests/test_ci_pbxproj_test_wiring.sh` also got a small hardening: the synthetic sandbox pbxproj no longer mentions the orphan test file by name in a comment, which used to produce a spurious `hits=1` and mask the failure detection inside the wrapper. Closes #4559 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: handle .registered SessionAgent case in test helper to compile cmuxTests/SessionIndexViewTests.swift was not in the test target until the previous commit wired it into project.pbxproj. While it was orphan, the SessionAgent enum gained a `.registered(RegisteredSessionAgent)` case (Sources/SessionIndexModels.swift:44) that the test's `defaultSpecificsForTesting` switch never accounted for. With the test file now actually compiling on CI, the switch fails: SessionIndexViewTests.swift:391:9: error: switch must be exhaustive note: add missing case: '.registered(_)' The test's call sites only ever pass built-in agents (`.claude`, `.grok`). Adding a `fatalError` on `.registered` keeps the switch exhaustive without inventing fake `CmuxVaultAgentRegistration` data that future readers would have to reconcile; if anyone extends the suite to cover Vault-registered agents, the fatalError points them at the missing helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: tighten pbxproj lint to require target membership, not just file reference Earlier version of the lint counted any pbxproj line that mentioned the test file basename. That's too permissive: a file can have a PBXFileReference and a group children entry but still not be a member of the cmuxTests target's PBXSourcesBuildPhase, in which case Xcode silently skips it — the exact failure mode that lets a regression test land green without ever running. Switch to counting only lines that end with "<basename>.swift in Sources */", which appear in: 1. the PBXBuildFile entry, and 2. the cmuxTests target's PBXSourcesBuildPhase files list. A target member has hits >= 2 in both. A "group-only" file (referenced in the project tree but not part of the test target) has hits = 0, which is exactly the silent-skip case the lint must catch. Also adds a new (c) sandbox case in `tests/test_ci_pbxproj_test_wiring.sh` that drops a file with a PBXFileReference + group child but no PBXBuildFile / SourcesBuildPhase entry, and asserts the lint flags it. Without this case the wrapper would still pass against the looser bare-filename check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: scope pbxproj test-wiring check to cmuxTests target's Sources phase Earlier iterations counted matches in the whole pbxproj. That accepted two silent-skip cases: 1. A file with PBXFileReference + group child but no PBXBuildFile / SourcesBuildPhase entry (filename appears 2x globally, but the file is not a member of any target — Xcode does not compile it). 2. A file wired into the wrong target (e.g. cmuxUITests instead of cmuxTests). `<file>.swift in Sources` appears 2x in the pbxproj — once in PBXBuildFile, once inside cmuxUITests' Sources phase — but Xcode still does not compile it into the cmuxTests bundle, so the regression test never runs. Resolve the cmuxTests PBXNativeTarget and its Sources build phase UUID, slice that phase block, and look for the file's `in Sources` entry only inside that block. Threshold becomes 1 hit (membership) rather than a global count, which is exactly what determines whether Xcode compiles the file into cmuxTests. Tighten the test wrapper to exercise all three failure modes against synthetic pbxprojs that include a real cmuxTests PBXNativeTarget + Sources phase stub. Verified by inverse: with this change reverted on a scratch copy of the real pbxproj, the lint misses the wrong-target case; with it applied, the lint flags it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: anchor pbxproj membership match against suffix-overlap false negatives The previous check looked for `<base> in Sources` inside the cmuxTests Sources phase. That's vulnerable to filename-suffix overlap: if the lint target is a substring of another wired file, the longer match still satisfies the grep. The repo already contains an overlapping pair — `SearchIndexTests.swift` is a suffix of `SettingsSearchIndexTests.swift` — so accidentally removing `SearchIndexTests.swift` from the cmuxTests Sources phase would pass the lint. Switch to a fixed-string match against the full PBX comment `/* <base> in Sources */`. The leading `/* ` and trailing ` */` disambiguate the basename. Verified by inverse: scratch-removing `SearchIndexTests.swift` from the real cmuxTests Sources phase now flags the file specifically, while `SettingsSearchIndexTests.swift` is unaffected. Add a fifth sandbox case (e) in the wrapper that wires `PrefixFooTests.swift` but leaves `FooTests.swift` orphan, and asserts the lint flags only the orphan. Without this fixture a later loosening of the grep would not be caught. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6edf4c3. Configure here.
…unded-scans # Conflicts: # Sources/SessionIndexView.swift # cmux.xcodeproj/project.pbxproj
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
SessionIndexStore.forEachJSONLinestreaming primitive with strictmaxBytes, optionalmaxLines, forward/reverse direction, stop summaries, and a per-recordautoreleasepoolaroundData.subdata, JSON decoding, and the caller body.history.jsonl, so the sorted/deduped set is capped by a page-sized byte/line budget instead of the whole file.Reproduction / verified code shape
Sources/SessionIndexRegisteredAgents.swift:466-549on current main:loadAntigravityHistoryEntriesusedforEachJSONLine(url: historyURL, maxBytes: Int.max), built the fulllatestBySessionIDdictionary, sorted alllatestBySessionID.values, then sliced withdropFirst(offset).prefix(limit).Sources/SessionIndexStore.swift:1010-1045on current main: the JSONL loop read chunks, createdlineDatawithleftover.subdata(in:), decoded withJSONSerialization.jsonObject(with:), and invokedbody(obj)without a per-lineautoreleasepool.Sources/SessionIndexView.swift:1284-1317on current main: Antigravity preview calledSessionIndexStore.forEachJSONLine(url: url, maxBytes: Int.max)and only stopped after collectingmaxPreviewTurnsmatching records, so nonmatching history rows could force a full-file scan.Design decision
Shared primitive. The bug class was duplicated scan policy, so the JSONL reader now owns byte limits, line limits, traversal direction, and autorelease pressure; callers choose budgets instead of open-coding unbounded loops.
Verification
SessionIndexJSONLStreamTests; the first commit adds a failing regression wheremaxLines: 3should visit only three JSONL records.Perf notes
No RSS benchmark was collected. The deterministic before/after signal is line/byte boundedness: before the regression's
maxLines: 3path visited all 10 records; after the fix it visits 3 and reports.maxLines. Reverse byte-cap coverage asserts the reader does not read beyond the supplied byte budget.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes session list ordering/pagination and preview truncation for Antigravity history; behavior is bounded and tested but affects how large histories are indexed and displayed.
Overview
Replaces unbounded Vault
history.jsonlreads with a sharedSessionIndexStore.forEachJSONLinehelper that enforcesmaxBytes, optionalmaxLines, forward/reverse traversal, per-lineautoreleasepool, and a stop summary (bytes, lines, reason).Antigravity session listing now scans newest-first with a byte budget tied to
offset + limit(full max window when search/cwd filters apply), keeps reverse discovery order instead of sorting the whole deduped set, merges sparse rows to backfill title/cwd, and stops early once the page has enough stable metadata—so duplicate lines do not burn the whole file budget.Antigravity transcript preview uses the same bounded reverse reader (fixed byte/line caps) and inserts a localized “Preview truncated” event when limits or turn caps are hit. Adds
SessionIndexJSONLStreamTestsand Antigravity pagination/metadata tests; expandssessionIndex.preview.truncatedlocalizations.Reviewed by Cursor Bugbot for commit 94a6a9c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes unbounded Vault JSONL history scans by adding a capped streaming reader and moving Antigravity listing/preview to reverse, budgeted scans that stop once page metadata stabilizes. Preserves newest-first history ordering, makes pagination deterministic, and prepends a localized “Preview truncated” when limits are hit. Addresses Linear 4535.
Bug Fixes
SessionIndexStore.forEachJSONLinewithmaxBytes, optionalmaxLines, forward/reverse traversal, per-recordautoreleasepool, and a summary (bytes read, lines visited, stop reason).history.jsonlwith byte budgets scaled tooffset + limit(uses max window for search/cwd filters), preserves reverse discovery order with a discovery-index tiebreaker, backfillstitle/cwdfrom older rows, stops once target count and metadata are stable, and skips duplicate rows.SessionIndexJSONLStreamTests; expandedPiVaultAgentPersistenceTestsfor pagination/backfill/sparse-search/duplicate-row/ordering cases; broadenedsessionIndex.preview.truncatedlocalizations.Refactors
CLAUDE.mdseparators to avoid conflict-marker false positives.Written for commit 4b73d17. Summary will update on new commits.
Summary by CodeRabbit