Refactor duplicated Codex report accumulation and pricing - #3479
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. |
|
Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 1:44 PM ET / 17:44 UTC. ClawSweeper reviewWhat this changesShares Codex cost-report accumulation and pricing helpers while preserving report fields, aggregate pricing safeguards, and existing cache compatibility. Merge readiness✅ Ready for maintainer review Keep open: this is useful, bounded cleanup that remains distinct from the related performance PR. No actionable correctness defect was found; repository policy and OWNER authorship also preclude automatic closure. Priority: P3 Review scores
Verification
How this fits togetherCodexBar builds spending reports from cached local session usage and model pricing. These helpers calculate daily costs and summarize them for project and session displays. flowchart LR
A[Cached session usage] --> C[Daily report builder]
B[Model rates and custom prices] --> C
C --> D[Row or aggregate pricing]
D --> E[Shared model summaries]
E --> F[Project and session displays]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep one implementation of report summation and pricing while retaining existing projections, overlay precedence, and compatible cache adoption. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR proposes internal consolidation rather than a reported malfunction; source comparison found no introduced failing path. Is this the best way to solve the issue? Yes: reusing the existing accumulator and pricing implementation is a narrow approach, with explicit preservation of activity-field omission, aggregate safeguards, and cache adoption. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against ca3ad7851e93. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: Refactor duplicated Codex report accumulation and pricing This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Reuse bounded model resolution within each Codex daily, project, and session report while preserving scalar pricing and persisted cache compatibility. Refs #3247, #3411. Cleanup #3479 funds this fix; the pair removes nine production lines. Changelog and contributor proof retained. Validation: 267 focused tests, full 1,031-selection suite, make check, independent P2 review, and exact-head CI 34155321823 pass. Co-authored-by: Nikolai Berezovskii <nikolai@brzv.sk>
Codex report construction duplicates model-cost accumulation and scalar/aggregate pricing, and wraps a single catalog load in a stateful resolver. Share those paths while preserving their existing behavior.
Project/session model summaries retain exactly their six cost/token fields, including nil-versus-zero semantics; activity detail remains omitted. Traversal, addition order, and caller sorting stay unchanged. Aggregate pricing still applies custom overlays before rejecting unresolved long-context aggregates. Catalog loading stays at the same point with the same supplied/loaded/empty precedence. The single-use project accumulator method becomes its two existing assignments.
This removes 110 production lines and funds the separate report-pricing CPU fix in #3476. The generated parser identity changes mechanically, so the released 9547dc9d7b7675f6 identity is explicitly adopted without rebuilding native rows or checkpoints. Pi's force-rescan fixture uses the current parser identity; independent predecessor-invalidation coverage remains.
The focused run passed 287 tests across 11 suites covering pricing, overlays, catalog-loading work, model projection, cache adoption, Pi compatibility, fork accounting, project/session behavior and architecture. The final patch additionally checks that a custom zero-price overlay remains authoritative above the aggregate context threshold.
make checkpasses and independent review found no actionable P0–P2 findings. The full local suite passed all 1,029 selections across 86 groups on the first attempt, with no retries or timeouts, in 1,276.5 seconds. Exact-head CI run 34148708994 passed both macOS shards and all Linux builds. No live accounts, credentials or user preferences were accessed by these tests. Production output is unchanged, so no user-facing changelog entry is needed for this cleanup.