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: needs maintainer review before merge. Reviewed September 27, 2026, 10:53 PM ET / September 28, 2026, 02:53 UTC (Revision 3). ClawSweeper reviewWhat this changesThe branch makes the shared config reader treat empty or JSON-whitespace-only files as absent, with tests and documentation for CLI usage, app settings, and later saves. Merge readiness✅ Ready for maintainer review Keep open: the linked failure remains on current main, and this PR is a focused, well-supported candidate fix. I found no actionable introduced defect. Priority: P2 Review scores
Verification
How this fits togetherThe shared config store reads config.json for both the macOS menu bar app and the CLI. Its result supplies provider settings and usage commands; settings edits write JSON back to the file. flowchart LR
A[Config file] --> B[Shared config reader]
B --> C{Blank or missing?}
C -->|Yes| D[Default configuration]
C -->|No| E[Decode JSON]
D --> F[App settings]
D --> G[CLI usage and edits]
E --> F
E --> G
E -->|Invalid| H[Config error]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Technical reviewBest possible solution: Land the shared-loader repair with the stated blank-file save policy and keep the malformed-file guard; resolve the linked issue after merge. Do we have a high-confidence way to reproduce the issue? Yes. Current main decodes every existing blank file, and the linked report supplies isolated Linux CLI steps; this read-only review did not execute them. Is this the best way to solve the issue? Yes. One guard in the shared loader repairs the app and CLI paths while the existing decoder continues to reject malformed non-empty files. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cda264c299ad. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
steipete
added a commit
that referenced
this pull request
Sep 28, 2026
A zero-byte or whitespace-only config file now behaves like a missing one (defaults, usage keeps working, next save writes valid JSON with 0600 permissions) instead of failing closed and blanking usage; malformed non-empty JSON keeps its decode error and protected writes. Fixes #4071. Thanks @kvnloo!
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Blank config files currently stop CLI usage with a decode error even though a missing file works. The shared config store now treats zero bytes and JSON whitespace as absent, so the existing default-backed paths work on macOS and Linux. Non-empty input still uses the existing decoder and error handling; config paths and credential storage are unchanged.
Regression coverage checks missing, empty, whitespace-only, and malformed files; read-only byte preservation; default creation and private settings saves; retained in-memory app settings; and CLI usage through an isolated local RPC stub. The configuration docs and 0.68.1 changelog describe the behavior.
Verification:
swift build -j 2 --product CodexBarCLI— passed (Build complete! (686.93 sec)).CodexBarCLI config validate --json— missing/empty/whitespace exit 0 without writing; malformed JSON exits 1 withkind: config. The same empty-file expectation failed on the baseline CLI with exit 1 before the fix.CodexBarCLI usage --provider codex --source cli --json --json-onlyusing a local synthetic RPC executable — returned the expected 1% usage for missing, empty, and whitespace files, leaving each file unchanged.CodexBarCLI config enable --provider grok --json— subsequent saves produced valid JSON with 0600 permissions for all three absence cases; malformed input remained unchanged and exited 1.make check— passed; SwiftLint reported 0 violations across 2,671 files.05f37ae903a1: both macOS test shards passed, and the Linux config-store suite passed all 11 parameter cases (3 tests). Lint and the musl build passed. Linux x64 ran 850 tests and failed only an unrelated Abacus timing assertion: 9.003251 seconds against a strict 9-second limit. The failed-job rerun (attempt 2) passed at the same head without code changes; CI run.The attempted local focused command was:
Production delta against the branch base: 1 file changed, 2 insertions(+), 2 deletions(-), net 0.
Fixes #4071