Repository navigation
fix(codex): count sessions archived as .jsonl.zst - #4534
BananaIIya wants to merge 8 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. What this changesThis PR decodes archived Codex rollouts into private, resumable cache mirrors for the existing cost scanner. Example: Run the supplied 30-day fixture containing plain sessions, compressed sessions, and an archived fork parent.
Review scores
ProductKind: Bug fix · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 1 item remains This PR addresses a current accounting bug, and the earlier findings are resolved; accepting the vendored decoder and transcript-mirror storage remains a maintainer choice. Priority: P2 Decision needed
Before merge
FindingsNone. Agent review detailsHow this fits togetherCodex local rollout files feed the shared cost scanner, which stores accounting progress in SQLite and supplies estimated usage to the CLI and app. flowchart LR
A[Codex rollout files] --> B[Archive inventory]
B --> C[Private resumable mirrors]
A --> D[Existing JSONL scanner]
C --> D
D --> E[SQLite cost store]
E --> F[CLI and app reports]
Technical reviewBest possible solution: Approve the portable decoder and mirror storage tradeoff, preserving the existing scanner’s accounting and offset ownership. Do we have a high-confidence way to reproduce the issue? Current-main discovery excludes compressed rollouts, corroborated by released-CLI comparisons in the linked report; no target code was executed during this read-only review. Is this the best way to solve the issue? Mirroring preserves the existing parser and logical offsets, with convincing accounting and upgrade evidence, subject to accepting its dependency and storage costs. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6480977b47f6. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (6 earlier review cycles)
Reviewed October 17, 2026, 6:14 PM ET / 22:14 UTC (Revision 7). |
|
@clawsweeper re-review Addressed the review in f8e339a: archive decoding now runs inside the refresh's scan budget with cancellation between chunks, failed archives keep catch-up pending (with a retry backoff), and forks with a missing baseline request their archived parent outside the report window. Synthetic before/after and upgrade proof are in |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
4e71293 to
890f64e
Compare
|
@clawsweeper re-review Addressed the remaining bounded-work finding (#2452) in the last commit: archives now decode in resumable slices that never exceed the refresh's byte allowance or deadline (checked before every decode step), with the paused decoder kept for the next refresh and a disk-skip resume after relaunch. Slices are written and published under an exclusive flock so the app and CLI cannot interleave. Rebased on 0.76.0; tests, |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
|
@clawsweeper re-review Addressed the transcript permission finding in d0993bb:
|
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0993bb2dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let session = reused, session.consumedOutput != onDisk { | ||
| reused = nil |
There was a problem hiding this comment.
When a process relaunches (or the parked session expires) with a large .resume file, the refresh deadline can fire while the fresh decoder is still replaying the on-disk prefix. park() then stores a valid session whose consumedOutput is less than onDisk, but this equality check discards it on the next refresh, so the app's 2-second catch-up passes (Sources/CodexBarCore/CostUsageFetcher.swift:32) repeatedly restart from byte zero and an archive whose prefix cannot be replayed in one pass never materializes. Retain sessions that have consumed a valid prefix rather than requiring them to have reached the end of the slice.
Useful? React with 👍 / 👎.
| return Set(cache.files.values.compactMap { usage in | ||
| guard let parentId = usage.forkedFromId?.lowercased(), | ||
| self.isUnresolvedMissingParentFork(usage), | ||
| !knownSessionIds.contains(parentId) |
There was a problem hiding this comment.
When a parent was indexed while plain, is later archived after a mirror root already exists, and is old enough to remain outside the current scan window, its now-missing plain path remains in cache.files. This set therefore treats the stale row as proof that the parent is available even though cachedCodexSessionIndex rejects missing paths, so the parent's archive is omitted from requiredSessionIds and every child fork stays unresolved and unmetered. Only suppress a parent ID when its cached source is still available to the file index.
Useful? React with 👍 / 👎.
| for sourceRoot in sourceRoots { | ||
| try checkCancellation?() | ||
| let mirrorRoot = self.mirrorRoot(for: sourceRoot, cacheRoot: cacheRoot) | ||
| let archives = self.archives(under: sourceRoot.standardizedFileURL, mirrorRoot: mirrorRoot) |
There was a problem hiding this comment.
CodexScanBudget is constructed immediately before prepare in CostUsageScanner.swift:6110-6127, but archives(under:) recursively drains each source tree without consulting that budget and cancellation is checked only once before the root. If inventory of a large CODEX_HOME consumes the app's 2-second automatic deadline, materialize defers every archive; the next catch-up pass repeats the same full walk and hits the deadline again, so archives can be starved forever while cancellation also cannot interrupt the traversal. Page or cache this inventory under the refresh budget so each pass makes progress.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
status: ⏳ waiting on author
Codex archives finished rollouts as rollout-*.jsonl.zst and later deletes the plain .jsonl. The cost scanner only lists .jsonl, so once the originals are removed their usage silently drops out of every cost window. Decode archived rollouts into a CodexBar-owned mirror under cost-usage/codex-archived-rollouts/, keeping the source layout and the archive's mtime, and add existing mirror roots to codexSessionsRoots. A rollout is mirrored only while its plain .jsonl sibling is absent, so it is never counted twice, and a mirror is pruned when its archive disappears. The parser fingerprint changes, so 0.75.0's 3f88178bf03ad704 is accepted as a compatible predecessor: stored rows and checkpoints are unchanged. Vendors the zstd v1.5.7 single-file decoder (BSD) as the CZstd target and registers it in the third-party acknowledgements. Fixes steipete#4523 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Address review on the archived-rollout mirror: - Decode inside the refresh's CodexScanBudget, after mirror roots are created ahead of root fingerprinting. Newest archives decode first; the rest are deferred, and cancellation is checked between input chunks. - Failed archives are kept in a per-mirror ledger, retried when they change or after an hour, and counted as pending files so catch-up stays pending and the previous report is preserved through completion reconciliation. - Forks whose baseline is missing request their archived parent by session id, regardless of the report window, so they settle like forks with older plain parents. Adds a synthetic before/after proof under .github/pr-proof. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Address the remaining review finding on bounded work (steipete#2452's intent): - An archive asks the refresh's budget for its estimated remaining decoded size, and for whatever the refresh has left once that estimate is used up (no content size in the header, or a frame it did not count), so slices never shrink. Output goes to a hidden .resume slice named for the archive's size and mtime, and decoding pauses at the allowance or at the refresh deadline, which is checked before every decode step. No archive is decoded past the budget, including the first one in a refresh. - The slice is written and published under an exclusive flock, so the app and the CLI cannot interleave; a busy or unavailable slice is deferred to the next refresh, and a slice longer than its archive is truncated and decoded again. - The paused decoder is kept per process (at most two, dropped after ten idle minutes), so the next refresh continues without re-decoding; after a relaunch the slice is resumed by skipping the bytes already on disk. Cancellation keeps the slice and is not recorded as a failure. - Archives are fully listed only when a refresh scans. Before roots are fingerprinted, a cheap probe creates a missing mirror root (stopping at the first archive, and remembering an empty result for ten minutes), so the first refresh, including a one-shot CLI run, scans archived usage. Also moves the changelog entry to 0.76.1 (Unreleased) after rebasing on 0.76.0, whose parser hash matches 0.75.0's compatible predecessor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mirrors hold whole decoded transcripts. Create slices 0600 and tighten resumed ones with fchmod, publish with .usingNewMetadataOnly so a replaced mirror does not inherit the old mode, write the failure ledger 0600, keep the mirror container and roots 0700, and tighten files an earlier build left readable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reuse a decoder paused while replaying a slice's prefix instead of restarting it, always make one decode step once an archive is admitted, check cancellation while walking for archives, and request an archived fork parent even when a stale row for its old plain file is cached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dline The first archive of each refresh is admitted past the deadline and makes at least one decode step, so a history large enough that listing it uses up every automatic refresh's deadline still converges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d0993bb to
b4bb662
Compare
|
@clawsweeper re-review Rebased on current main (0.77.0); the merge conflict is gone. Addressed the three findings in efce0ff and b4bb662:
Reverting any of the three fixes fails its test. 0.77.0's parser hash is registered as a compatible predecessor. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@clawsweeper re-review Addressed the remaining author items in 2914f1a:
The decoder dependency and mirror disk footprint still need your call, @steipete. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
This branch has not been deployed
1 participant
Fixes #4523 (thanks @kcharlan for the detailed report; also mentioned in #4300).
Codex archives finished rollouts as
rollout-*.jsonl.zstand later deletes the plain.jsonl. The cost scanner only reads.jsonl, so once the originals are gone that usage silently disappears from every cost window.Fix: archived rollouts are decoded into a mirror under CodexBar's cost cache and scanned like normal files. The byte-offset scanner and its logical offsets are unchanged.
.jsonlis absent (Codex keeps both for a while), so nothing is counted twice. Mirrors are removed when the archive goes away or the plain file comes back.CodexScanBudget, newest first. An archive asks for its estimated remaining size (or whatever the refresh has left once the header's estimate is used up), writes at most that to a hidden.resumeslice, and pauses at the allowance or the refresh deadline, which is checked before every decode step. No archive is decoded past the budget. The paused decoder is kept in memory (at most two, dropped after ten idle minutes) so the next refresh continues without re-decoding; after a relaunch the slice resumes by skipping the bytes already on disk, and a decoder paused while still replaying those bytes is kept, so a long replay spans refreshes instead of restarting. The first archive of each refresh is admitted, and makes at least one decode step, even when listing archives used up the deadline, so a large history cannot starve decoding. Both archive walks check cancellation every 256 entries.flock, so the app and the CLI cannot interleave. A busy or unavailable slice is deferred to the next refresh; a slice longer than its archive is truncated and decoded again.0600(and tightened withfchmodwhen resumed), published mirrors keep that mode (replaceItemAtwith.usingNewMetadataOnly, since the default keeps the old file's mode), the failure ledger is0600, and the mirror container and roots are0700. Files and folders an earlier build left readable are tightened on the next scan.Sources/CZstd: the decoder-only zstd v1.5.7 single-file build (BSD, ~1 MB), registered in the acknowledgements. macOS has no built-in zstd. Source and checksums are in its README. This is the new dependency the issue review flagged, so it needs your sign-off.Proof (synthetic fixture,
.github/pr-proof/codex-archived-rollouts/):run-proof.pybuilds a fictitiousCODEX_HOMEand runscodexbar cost --provider codex --days 30 --refreshwith an isolatedHOMEuntil totals repeat. Base is the notarized 0.75.0 CLI; the 0.77.0 upgrade uses the officialCodexBarCLI-v0.77.0-macos-arm64release (checksum verified) viarun-proof.py --upgrade-base 0.77.0=<cli>, against the fixture's clean variant (fixture dates are relative to the run day).a47b70abaf8ad86cadopted, same database inode, plain-file rows unchangedThe fixture has a plain rollout, archive-only rollouts in
sessions/andarchived_sessions/, a rollout present in both forms, a truncated archive, and a fork in the window whose parent is archived 60 days back.Under umask 022, every file and folder the patched CLI writes under the mirror is owner-only (
proof.json, "head mirror permissions"): directories0700, decoded transcripts and the failure ledger0600, none accessible to other users. No slice was pending when that run ended; paused slices are covered by the tests below.CodexArchivedRolloutMirrorTests(27 tests, seetests.log) covers: each refresh writing exactly its byte allowance to disk (nine refreshes for three archives); a multi-frame archive whose header declares only the first frame, including under an uncapped timed budget; pausing at the deadline; a replay paused by the deadline after a relaunch, an admitted archive past its deadline, and archives listed after every refresh's deadline has passed, each still completing over later refreshes; cancellation during the archive walks; resuming with and without the in-memory decoder; a changed archive mid-slice; a slice longer than its archive; a locked and an unavailable slice; the parked-decoder cap; cancellation; failure backoff; parent resolution, including a parent archived after its plain rollout was cached; owner-only modes for paused slices, published mirrors, the ledger and folders, including tightening files an earlier build left0644and replacing a loosened mirror; and the scanner slicing a large archive across refreshes and counting it exactly once (with a plain-rollout control). Reverting any of the three progress fixes (replay retention, first-archive admission, stale-parent check) fails its test. Disabling the parent request or the mirror-root probe makes the corresponding test fail, requesting an unbounded remainder traps in the uncapped-budget test, and dropping any of the mode fixes (creation mode,fchmod, folderchmod, tightening kept files,.usingNewMetadataOnly) fails a permission test.Tested with Xcode 27 on the 0.77.0-based head:
make test(all 1,720 test selections passed),make check(0 violations). The 0.75.0 scenarios inproof.jsonare from the previous head; the later changes only affect refreshes that hit their deadline, which the CLI--refreshfixture runs do not. The 0.77.0 upgrade was run on this head. On one real history (--period all --refresh, isolated cache), the totals match decompressing every archive in place without this patch.Coverage note:
historyCoverageIsEstablishedonly becomes true when the inventory has at mostcodexCatchUpScanCandidateLimit(512) rollouts, because that is the only path that clearscodexScanCatchUpPending. That is why the issue's column B (1,722 files) and the larger real history above stayfalse, with or without this patch. Mirroring archives pushes more installs over that limit; I left it unchanged for you to decide. Not covered: the Linux CLI build with the new C target.🤖 Generated with Claude Code