Add opt-in ccusage fallback for incomplete Codex history - #2817
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: found issues before merge. Reviewed August 10, 2026, 9:02 AM ET / 13:02 UTC. ClawSweeper reviewWhat this changesThis PR adds an opt-in ccusage executable fallback for incomplete Codex history scans, plus separate fallback-coverage signals for the dashboard and CLI and optional helper packaging. Merge readinessKeep open: the real-archive evidence supports the incomplete-history scenario, but the branch still accepts aggregate-only fallback comparisons that can regress individual days and needs a maintainer decision on the executable-fallback boundary. Likely related people: steipete (high confidence) and xx205 (medium confidence). Priority: P2 Review scores
Verification
How this fits togetherCodexBar reduces local Codex archive data into usage snapshots for the Spend Dashboard and flowchart LR
A[Codex archive] --> B[Native history scan]
B --> C{Native coverage complete?}
C -->|Yes| D[Usage snapshot]
C -->|No| E[Optional ccusage executable]
E --> F[Fallback validation]
F --> D
D --> G[Spend Dashboard]
D --> H[CLI cost JSON]
Decision needed
Why: This introduces a new environment/package surface and a deliberate local-code-execution boundary; source review cannot determine the preferred long-term product and security contract. Before merge
Findings
Agent review detailsSecurityNeeds attention: The deliberately opt-in helper execution path is a new security boundary that needs maintainer acceptance, although no hidden download or dependency-resolution path was found. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Choose whether CodexBar should support an executable fallback, then require per-day non-regression, exact architecture matching, and policy-compliant model fixtures before merging it. Do we have a high-confidence way to reproduce the issue? Yes for the remaining defect: a fallback whose total exceeds native totals while omitting or lowering one native day passes the aggregate check and replaces the report. The supplied real-archive smoke additionally demonstrates the broader fallback path. Is this the best way to solve the issue? No. The approach needs per-day non-regression and exact architecture validation, and maintainers must first choose whether the executable fallback is the desired product direction. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b093129a4f94. LabelsLabel justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
@clawsweeper re-review Updated the PR body with redacted real-archive smoke evidence and pushed the follow-up fixes for native coverage separation and version/SHA-256 helper provenance. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbcb0035ed
ℹ️ 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".
| "cacheCreationTokens": 5, | ||
| "totalTokens": 955, | ||
| "totalCost": 1.25, | ||
| "modelsUsed": ["gpt-5.6-sol"], |
There was a problem hiding this comment.
Use clearly fictitious test model names
This fixture introduces gpt-5.6-sol (and repeats it in the helper fixture below), which looks like a plausible future model identifier rather than an obviously fake placeholder; please switch these new test values to something clearly synthetic such as test-model so fixtures cannot be mistaken for unreleased model names.
Useful? React with 👍 / 👎.
Summary
Changes after review
historyCoverageIsEstablished=falsewhile exposinghistoryFallbackCoverageIsEstablished=true.Validation
Design notes
Related to #2815.