Skip to content

Retain priced Codex models when Auto Review is unpriced - #2643

Merged
steipete merged 6 commits into
steipete:mainfrom
akshayprabhu200:codex/fix-partial-codex-models
Aug 5, 2026
Merged

Retain priced Codex models when Auto Review is unpriced#2643
steipete merged 6 commits into
steipete:mainfrom
akshayprabhu200:codex/fix-partial-codex-models

Conversation

@akshayprabhu200

@akshayprabhu200 akshayprabhu200 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Keep priced Codex model rows visible when the same local history also contains an unpriced codex-auto-review routing row. The dashboard labels this as a partial model breakdown and removes ranking; it does not invent a price for Auto Review.

Root cause

Codex local token_count records do not contain a billable model. The surrounding context can name codex-auto-review, which is a routing label with no pricing entry. The scanner therefore leaves that row unpriced. The spend dashboard previously treated a single unpriced model row as reason to discard every Codex model row, even when the daily stored cost exactly matched the sum of the other priced rows.

Behavior

  • Retains the directly priced Codex rows when their sum exactly matches the daily stored cost.
  • Shows the routing row without a fabricated cost.
  • Keeps the group partial and unranked.
  • Continues to fail closed for malformed or inconsistent histories, including named rows with a present but invalid cost.

Validation

  • make check
  • swift test --filter SpendDashboardModelTests (30 tests passed)
  • make test (complete suite passed with the App Store Xcode toolchain before the final focused guard)
  • git diff --check
  • The partial-attribution fixtures use clearly fictional model names; they do not expose non-public model identifiers.
  • A local codexbar cost --provider codex --format json --days 30 structural scan found priced rows alongside unpriced codex-auto-review rows, with a valid aggregate cost.

Native runtime proof

Native Usage & Spend proof — model names retained, values redacted

This native Usage & Spend capture shows priced Codex model rows, the partial-breakdown warning, dashed unranked markers, and codex-auto-review with an em dash instead of a fabricated cost. Model names remain visible; financial and usage quantities are redacted.

The capture was made at signed parent 39610a6cb8ae6e75761d64d5760222321762069f. Follow-up signed head 063fc189fed45a0f6f544a53d0d1d301c9f7d246 adds only a fail-closed guard and regression test for malformed named costs; it does not alter the displayed valid-data behavior.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Aug 4, 2026
@clawsweeper

clawsweeper Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 4, 2026, 11:35 PM ET / August 5, 2026, 03:35 UTC.

ClawSweeper review

What this changes

The PR displays priced Codex model rows beside an unpriced Auto Review routing row when their daily costs reconcile, marking the breakdown partial and unranked.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep this PR open for a maintainer product decision: the implementation is focused, fail-closed for invalid data, and backed by native UI proof, but it deliberately changes the dashboard from withholding incomplete model attribution to showing reconciled partial rows.

Priority: P2
Reviewed head: 124714647b9d81b2e950497c33b6505b0a09326e
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Strong native proof and focused fail-closed regression coverage support a mergeable implementation once the attribution-policy decision is made.
Proof confidence 🦞 diamond lobster (5/6) ✨ media proof bonus Sufficient (screenshot): A native Usage & Spend screenshot directly shows the priced rows, partial warning, unranked markers, and unpriced Auto Review row; the follow-up guard does not alter that valid-data display.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (screenshot): A native Usage & Spend screenshot directly shows the priced rows, partial warning, unranked markers, and unpriced Auto Review row; the follow-up guard does not alter that valid-data display.
Evidence reviewed 5 items Current main withholds incomplete attribution: Current main only includes a provider's model rows when its model history is complete, so it does not yet implement this PR's partial-attribution behavior.
Proposed reconciliation guard: The PR admits partial Codex history only when every daily entry is either fully attributed or satisfies the dedicated partial-cost reconciliation guard.
Regression coverage: The added tests cover reconciled priced-plus-unpriced rows, fully unpriced history remaining unavailable, and malformed named costs being rejected.
Findings None None.
Security None None.

How this fits together

CodexBar converts local Codex usage records into provider and model spending summaries for the Usage & Spend dashboard. This change sits in the model-attribution step between scanned daily costs and the dashboard’s ranked model rows.

flowchart LR
  A[Local Codex usage records] --> B[Daily cost and model breakdowns]
  B --> C[Spend attribution]
  C --> D{Costs reconcile?}
  D -->|Complete| E[Ranked model rows]
  D -->|Partial but reconciled| F[Partial unranked model rows]
  D -->|Invalid or inconsistent| G[Withhold model breakdown]
Loading

Decision needed

Question Recommendation
Should exact daily cost reconciliation permit a partial, unranked Codex model breakdown when Auto Review has no billable model cost? Approve reconciled partial attribution: Accept the PR's bounded rule: show directly priced rows, retain the unpriced routing row without a fabricated cost, and keep the group partial and unranked.

Why: Current main intentionally requires complete model-cost coverage, so source evidence cannot determine whether this broader attribution contract is desired.

Before merge

  • Resolve merge risk (P1) - Merging changes an existing dashboard contract: mixed Codex history that currently hides all model rows will instead show partial, unranked rows based on daily exact-cost reconciliation.
  • Complete next step (P2) - A maintainer must choose the dashboard attribution contract before this otherwise-correct PR can proceed.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Implementation and regression coverage production +53, tests +112, 2 files affected The focused tests cover the new valid partial case and the principal fail-closed boundaries.

Merge-risk options

Maintainer options:

  1. Approve the partial-attribution contract (recommended)
    Accept the visible compatibility change because the PR labels the data partial, removes ranking, and rejects malformed or unreconciled costs.
  2. Pause on the current complete-only policy
    Do not merge if maintainers require model spending rows to remain hidden unless every row has a fully attributed cost.

Technical review

Best possible solution:

Adopt the reconciled partial-attribution display only if maintainers want exact daily cost matching to be sufficient evidence for a clearly labeled, unranked model breakdown.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR proposes a dashboard attribution policy rather than repairing a specified broken contract; it includes native after-fix UI proof of the proposed behavior.

Is this the best way to solve the issue?

Unclear: the implementation is narrowly scoped and safely rejects invalid data, but maintainers must decide whether daily reconciliation is sufficient attribution evidence.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9cc993f310cb.

Labels

Label justifications:

  • P2: This is a bounded, user-visible spend-dashboard attribution change without evidence of an urgent runtime failure.
  • merge-risk: 🚨 compatibility: Existing mixed Codex histories will present partial model rows where the current dashboard presents none.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (screenshot): A native Usage & Spend screenshot directly shows the priced rows, partial warning, unranked markers, and unpriced Auto Review row; the follow-up guard does not alter that valid-data display.
  • proof: sufficient: Contributor real behavior proof is sufficient. A native Usage & Spend screenshot directly shows the priced rows, partial warning, unranked markers, and unpriced Auto Review row; the follow-up guard does not alter that valid-data display.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. A native Usage & Spend screenshot directly shows the priced rows, partial warning, unranked markers, and unpriced Auto Review row; the follow-up guard does not alter that valid-data display.

Evidence

What I checked:

Likely related people:

  • steipete: Commit 27a314f introduced the complete-only model-row behavior that remains on current main. (role: introduced the current attribution policy; confidence: high; commits: 27a314f3a3a8; files: Sources/CodexBar/SpendDashboardModel.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain explicit maintainer approval for showing reconciled partial model attribution to existing users.

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.

History

Review history (11 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-04T07:49:19.486Z sha 39610a6 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-04T14:40:30.497Z sha 39610a6 :: needs real behavior proof before merge. :: [P2] Reject invalid costs instead of treating them as unpriced
  • reviewed 2026-08-04T14:59:41.434Z sha 39610a6 :: needs changes before merge. :: [P2] Reject invalid costs instead of treating them as unpriced
  • reviewed 2026-08-04T15:07:18.055Z sha 063fc18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-04T16:03:20.630Z sha 063fc18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-04T19:59:21.707Z sha 063fc18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T02:38:17.143Z sha f1c6fba :: needs maintainer review before merge. :: none
  • reviewed 2026-08-05T02:49:17.264Z sha 1247146 :: needs maintainer review before merge. :: none

@akshayprabhu200
akshayprabhu200 marked this pull request as ready for review August 4, 2026 14:36
@clawsweeper clawsweeper Bot added merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. proof: sufficient Contributor real behavior proof is sufficient. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Native Usage & Spend proof with model names retained:

Native Usage & Spend proof

The capture shows the priced Codex model rows, partial-breakdown warning, dashed unranked markers, and codex-auto-review with an em dash rather than a fabricated cost. Financial and usage quantities are redacted; model identifiers remain visible.

Follow-up signed commit 063fc189fed45a0f6f544a53d0d1d301c9f7d246 closes the malformed-cost fail-closed case identified in the review, with a focused regression test.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 4, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Aug 4, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Aug 5, 2026
@steipete
steipete merged commit 8870914 into steipete:main Aug 5, 2026
1 check passed
@steipete

steipete commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Landed. Verification before merge (independent maintainer-agent review):

  • Retention logic verified strict: gated to the Codex provider; named priced rows must sum exactly to the entry's stored cost, unpriced rows must have an absent (not malformed) cost plus valid tokens, unnamed rows must be proven-zero, and at least one priced row must exist. A present-but-invalid cost fails closed (test-pinned).
  • "No ranking on partial" confirmed in the presentation layer: retained-partial groups get .incomplete completeness, rendering circle.dashed + warning label instead of a rank, em dash for the unpriced row's nil cost. Non-Codex behavior untouched (pre-existing Claude partial tests pass unchanged).
  • Added a guard test (f1c6fbac9) pinning that genuinely-unpriced-everything (routing rows only, no priced rows) stays .unavailable — previously only true by unstated invariant.
  • Post-merge-with-main housekeeping: extracted model-breakdown helpers to SpendDashboardModel+ModelBreakdown.swift (2a3150801) to stay under the SwiftLint type-body cap after OpenCode Go: per-model cost breakdown by day #2649 landed; byte-for-byte mechanical move, autoreview clean.
  • Local: swift test --filter SpendDashboardModelTests — 32/32; make check — 0 violations. CI green at merge.

@akshayprabhu200
akshayprabhu200 deleted the codex/fix-partial-codex-models branch August 6, 2026 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient Contributor real behavior proof is sufficient. 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.

2 participants