Skip to content

Refactor duplicated Codex report accumulation and pricing - #3479

Merged
steipete merged 1 commit into
mainfrom
codex/cost-report-duplication-cleanup
Sep 7, 2026
Merged

Refactor duplicated Codex report accumulation and pricing#3479
steipete merged 1 commit into
mainfrom
codex/cost-report-duplication-cleanup

Conversation

@steipete

@steipete steipete commented Sep 7, 2026

Copy link
Copy Markdown
Owner

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.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 1:44 PM ET / 17:44 UTC.

ClawSweeper review

What this changes

Shares 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
Reviewed head: 4526f183be878e0af100375ff1b2d1c0d85aa3c8

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused consolidation with relevant regression coverage and no actionable defect found in source review.
Proof confidence 🌊 off-meta tidepool Not applicable: OWNER-authored internal cleanup is exempt from the external-contributor proof gate; reported focused tests cover report projection, pricing, catalog loading, and cache compatibility, without claiming a live runtime demonstration.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: OWNER-authored internal cleanup is exempt from the external-contributor proof gate; reported focused tests cover report projection, pricing, catalog loading, and cache compatibility, without claiming a live runtime demonstration.
Evidence reviewed 8 items Complete introduced diff inspected: Read the complete local base-to-head diff, filling the supplied patch truncation. Seven production files and seven test files change; the consolidation is absent from the pinned current main.
Report projection preserves existing semantics: The shared accumulator retains the six existing cost/token sums and their presence flags. Project/session summaries explicitly omit activity fields, and their callers retain existing sorting. Additional token-mix accumulation uses overflow-aware helpers.
Pricing paths remain equivalent: Compared both former pricing functions with the consolidated implementation: custom overlays remain first, aggregate-only threshold rejection follows resolution, and ordinary row pricing retains the default scalar behavior. The aggregate fallback call explicitly supplies aggregate: true.
Findings None None.
Security None None.

How this fits together

CodexBar 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]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC delta Production +33/-143 (net -110); tests +69/-12 (net +57) The cleanup reduces production duplication while expanding projection and overlay coverage.

Technical review

Best 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.

Labels

Label changes:

  • add P3: This is behavior-preserving internal cleanup with no established urgent user-facing defect.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: OWNER-authored internal cleanup is exempt from the external-contributor proof gate; reported focused tests cover report projection, pricing, catalog loading, and cache compatibility, without claiming a live runtime demonstration.

Label justifications:

  • P3: This is behavior-preserving internal cleanup with no established urgent user-facing defect.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: OWNER-authored internal cleanup is exempt from the external-contributor proof gate; reported focused tests cover report projection, pricing, catalog loading, and cache compatibility, without claiming a live runtime demonstration.

Evidence

What I checked:

  • Complete introduced diff inspected: Read the complete local base-to-head diff, filling the supplied patch truncation. Seven production files and seven test files change; the consolidation is absent from the pinned current main. (4526f183be87)
  • Report projection preserves existing semantics: The shared accumulator retains the six existing cost/token sums and their presence flags. Project/session summaries explicitly omit activity fields, and their callers retain existing sorting. Additional token-mix accumulation uses overflow-aware helpers. (Sources/CodexBarCore/CostUsageModels.swift:721, 4526f183be87)
  • Pricing paths remain equivalent: Compared both former pricing functions with the consolidated implementation: custom overlays remain first, aggregate-only threshold rejection follows resolution, and ordinary row pricing retains the default scalar behavior. The aggregate fallback call explicitly supplies aggregate: true. (Sources/CodexBarCore/Vendored/CostUsage/CostUsagePricing+Overlay.swift:4, ca3ad7851e93)
  • Released cache identity is preserved: The latest supplied release contains parser identity 9547dc9d7b7675f6. This PR explicitly adds that identity to compatible predecessors; existing adoption coverage checks preserved snapshots, usage rows, checkpoints, resumed parsing, and zero rebuilds. No stored payload or SQL schema is changed. (Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4, 6ef82690b4a7)
  • Upgrade and independent invalidation coverage: Read the predecessor-adoption test and Pi compatibility tests. The changed Pi fixture now isolates force-rescan behavior using the current parser identity; separate tests continue to verify predecessor invalidation and unchanged-cache reuse. (Tests/CodexBarTests/CostUsageStoreTests.swift:1028, 4526f183be87)
  • Related work is complementary: The supplied discussion links perf: reuse Codex pricing resolution across reports #3476, an open performance change that memoizes model resolution within report collections. This cleanup consolidates existing implementations without introducing that memoization; the other PR's runtime measurements are not proof for this head.

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Yuxin Qiao: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

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.

@steipete
steipete merged commit c526fe7 into main Sep 7, 2026
9 checks passed
steipete pushed a commit that referenced this pull request Sep 7, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant