perf: reuse Codex pricing resolution across reports - #3476
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 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. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 3:32 PM ET / 19:32 UTC. ClawSweeper reviewWhat this changesReuse bounded model-pricing lookups within each Codex daily, project, or session report collection while preserving individual cost calculations. Merge readiness✅ Ready for maintainer review This remains a useful, well-supported optimization after the merged cleanup. No actionable defects remain; the disclosed benchmark baseline does not create an additional merge blocker. Priority: P2 Review scores
Verification
How this fits togetherCodexBar builds local spend reports from cached session usage and pricing catalogs. These calculations feed daily totals and project/session breakdowns in its spend surfaces. flowchart LR
A[Cached session usage] --> C[Report collection]
B[Pricing catalog] --> D[Bounded model lookup memo]
C --> D
D --> E[Per-row cost calculation]
F[Dates and custom rates] --> E
E --> G[Daily project and session reports]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep lookup reuse scoped to each report collection, with bounded memory and unchanged per-row pricing and cache compatibility. Do we have a high-confidence way to reproduce the issue? Yes: main repeatedly resolves the same model inside report-row processing, and the contributor's real-cache measurements demonstrate the cost; this review verified the path without executing it. Is this the best way to solve the issue? Yes: collection-scoped memoization reuses the existing pricing lookup while leaving request-dependent prices uncached, following the repository's established resolver pattern. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c526fe71721e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
![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: 29fd82887c
ℹ️ 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".
|
@codex review Head 35a0de6 addresses the P2 oversized-key finding with a 512-byte UTF-8 admission limit before key allocation in both memos. The red-before-green test covers retained bytes, positive/negative lookup correctness, multibyte inputs, and the exact boundary. 272 focused tests, make check, ordinary release build, and the repeated real-corpus candidate proof pass (CPU -58.2%, byte-identical complete outputs). The post-change full local suite is running alongside exact-head CI; the PR body marks that gate in progress rather than reusing the prior head result. |
|
@clawsweeper re-review Please review exact head 35a0de6. The independent P2 retained-memory finding is addressed: both memo admission paths reject identifiers over 512 UTF-8 bytes before array allocation, retaining the uncached scalar behavior. A deterministic regression proves both the byte bound and preserved resolution. The updated real-corpus proof has identical complete outputs, CPU -58.2%, and RSS +4.5%. Focused tests (272), make check, and the ordinary release build pass. The post-change full local suite and exact-head CI are running in parallel; the PR body distinguishes those pending gates from completed evidence. Please call out any concrete remaining patch-quality gap rather than inferring that the pending gates have passed. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
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 check` passes 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.
2 participants
Codex cost reports repeatedly normalized and matched the same model even after the pricing catalog was loaded. Reuse bounded model-resolution results within one synchronous daily, project, or session report collection, reducing redundant CPU work without caching final prices or costs.
The resolver retains exact UTF-8 input keys, including positive and negative results, with 1,024 entries per memo and a 512-byte admission check before allocation. Oversized identifiers remain correct and uncached. Historical dates, custom zero/partial overrides, provider-qualified routes, context thresholds, authoritative source costs, and priority multipliers continue through scalar pricing. There is no global cache or cross-refresh invalidation policy.
The separate cleanup #3479 is landed and removes 110 production lines. This integrated fix adds 101 lines relative to that cleanup, leaving the pair net negative by nine lines. Project/session summaries retain their existing field projection. Native cache rows and checkpoints remain compatible with both the released parser identity 9547dc9d7b7675f6 and the cleanup identity ba2eca901de4c53d. Pi/OMP retains its existing parser-key invalidation policy and separate predecessor coverage.
Current integration validation
make checkpasses. Independent review found no actionable P0–P2 findings.Contributor's real-corpus proof
Nikolai Berezovskii (@brzvsk) measured the original resolver implementation at 35a0de6 against base 170a4d4 using a private frozen backup of an existing cost cache and pricing catalog: 2,169 files and 243,617 exact usage rows. Optimized core harnesses loaded SQLite and built daily, project, and session reports for fixed 30-day and 365-day ranges. The original A/B used three fresh processes per variant, reversing order for the middle pair; the post-review candidate was rebuilt and measured three more times against the same input. No concurrent build ran during those measurements.
All six complete structural report receipts were byte-identical, including token/cost/priority fields. Range timings include SQLite loading; process totals also include receipt serialization. Both variants used the same frozen catalog, existing persisted parser identity, and empty custom overlay. Override behavior is covered by scalar-parity tests. These are contributor-recorded measurements of the original implementation, not newly measured percentages for the maintainer integration. Raw cache data, reports, identities, and paths remain private.
This fixes repeated Codex report model resolution, complementing #3297's deferred raw-history decoding and #3319's separate Claude resolver. It does not claim to solve all eager usage-row hydration, scheduling, pending-state transitions, or whole-app battery use. Refs #3247; related follow-up evidence appears in #3411. Changelog credit is retained. Thanks @brzvsk for the root-cause investigation, implementation, memory-bound correction, and detailed proof.