Avoid rewriting unchanged plan history files - #2878
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@clawsweeper review |
|
@codex review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Codex review: needs maintainer review before merge. Reviewed August 12, 2026, 2:31 AM ET / 06:31 UTC. ClawSweeper reviewWhat this changesThe branch skips atomic replacement of a provider’s deterministic plan-history JSON when the existing bytes are unchanged, and adds filesystem-state regression tests. Merge readinessThis owner-authored PR remains necessary: current main still atomically replaces every non-empty provider history file, while the branch adds a narrow exact-byte guard with focused regression coverage. Priority: P2 Review scores
Verification
How this fits togetherCodexBar saves per-provider plan-utilization history as local JSON files. Provider refreshes feed the history store, which removes empty histories or persists changed data for later reload and menu-history rendering. flowchart LR
A[Provider refresh] --> B[Plan history store]
B --> C[Encode canonical JSON]
C --> D{Existing bytes identical?}
D -->|Yes| E[Preserve existing file]
D -->|No| F[Atomically write file]
B -->|Empty history| G[Remove stale file]
E --> H[Later history reload]
F --> H
G --> H
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the exact-byte persistence guard after required checks pass, retaining the established deletion and repair paths for non-identical histories. Do we have a high-confidence way to reproduce the issue? Yes—current main’s save loop deterministically encodes non-empty provider histories and always reaches atomic write; the branch’s focused tests exercise the unchanged-file path. Is this the best way to solve the issue? Yes—the byte-equality comparison is the narrowest solution because it preserves existing behavior for changed, missing, unreadable, malformed, and empty histories. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6e0a50001f98. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
clawsweeper
Bot
added
P2
|
Landed and verified. Behavior:
Proof:
Merged as No release, tag, appcast, package, or artifact publication was performed. |
1 participant
Summary
Fixes #2495
Root cause
PlanUtilizationHistoryStore.savedeterministically encoded every known provider and then always called atomicData.write, even when the encoded bytes exactly matched the existing provider file. Because an update to one provider saves the complete history dictionary, unchanged sibling files such ashistory/claude.jsonandhistory/codex.jsonwere repeatedly replaced as well.Implementation
After encoding the canonical sorted payload, the store reads the current file and returns early only when the bytes are identical. A missing or failed read falls through to the existing atomic write. Empty histories still remove stale files, malformed files are replaced with valid canonical JSON, and changed providers still round-trip normally.
Validation
UsageStorePlanUtilizationTests: 92 tests passedmake check: passedmake test: 840 selections across 70 groups passed with no retries or timeoutsOne earlier full run hit host-load flakiness in the unrelated
BoundedChildProcessProofTestslane: the PTY latency assertion took 25 seconds and the synthetic Grok handshake timed out. That exact three-test suite then passed in isolation in 5.3 seconds, and the complete matrix rerun passed all 70 groups without retries.