Skip to content

Partially fix inherited token accounting across Codex session forks#2113

Closed
iam-brain wants to merge 1 commit into
steipete:mainfrom
iam-brain:iam-brain/partial-fix-codex-fork-token-accounting
Closed

Partially fix inherited token accounting across Codex session forks#2113
iam-brain wants to merge 1 commit into
steipete:mainfrom
iam-brain:iam-brain/partial-fix-codex-fork-token-accounting

Conversation

@iam-brain

Copy link
Copy Markdown
Contributor

Summary

Partially fixes #2037 by improving how the usage scanner resolves parent sessions and subtracts token totals inherited by forked sessions.

Forked rollout files can retain copied session_meta from an ancestor, which made metadata-only lookup unreliable and allowed the same inherited usage to be counted repeatedly. This change:

  • treats the session UUID in a canonical rollout filename as authoritative
  • prevents copied metadata from impersonating a missing ancestor
  • fails open when legacy metadata ownership is ambiguous
  • lazily searches outside the normal 30-day window when resolving a referenced long-lived parent
  • keeps the broader historical search limited to ancestry resolution rather than expanding the standard scan

Testing against local session data from #2037 reduced the calculated total from roughly 2.23B tokens to 1.24B, compared with approximately 618M reported by OpenAI for the same period. This is still not an exact match, and more work is needed to understand the remaining counter behavior across related and unrelated sessions. However, it closes a substantial part of the gap without introducing speculative family-wide or global suppression that could undercount valid usage.

Testing

  • swift test --filter CodexSessionFileIndexTests\|Issue2037
  • make test
  • make check
  • git diff --check

All checks pass.

@clawsweeper clawsweeper Bot added 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 13, 2026
@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 12:12 AM ET / 04:12 UTC.

Summary
The PR replaces single-file session lookup with ambiguity-aware candidate resolution, recognizes canonical rollout filenames as session ownership, and searches older files only when resolving parent ancestry.

Reproducibility: yes. at source level: copied session_meta can map multiple rollout files to one identifier under current main, and the added focused tests exercise that path. The original private session dataset was not independently rerun here, so confidence in the exact real-world magnitude is medium.

Review metrics: 2 noteworthy metrics.

  • Patch size: 4 files; 217 added, 29 removed. The implementation is bounded but substantial enough that real-session compatibility deserves explicit review.
  • Regression surface: 2 test files affected. The PR adds both index-level and scanner-integration coverage for its new resolution rules.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #2037
Summary: This PR explicitly implements a partial candidate fix for the canonical token-overcount issue and does not claim to resolve its remaining discrepancy.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Have a maintainer explicitly accept the incremental-fix boundary and keep the linked issue open for the remaining accounting work.

Risk before merge

  • [P1] The demonstrated result remains materially inaccurate—roughly 1.24B calculated tokens versus about 618M reported—so users may still see significantly inflated usage and cost after this patch.
  • [P1] The broader on-demand ancestry search and ambiguity rules alter which historical rollout is selected; synthetic tests cannot fully establish behavior across all legacy filename and metadata combinations.

Maintainer options:

  1. Land incrementally with follow-up (recommended)
    Merge the bounded ownership correction while retaining the linked issue as the canonical tracker for unresolved overcounting.
  2. Require more representative proof
    Pause merge until redacted results cover additional forked, legacy, and ordinary sessions and show that the new resolver does not introduce undercounting.
  3. Wait for a complete model
    Close or supersede this PR only if maintainers prefer a single comprehensive lineage-accounting change over an incremental correction.

Next step before merge

  • [P2] A maintainer should decide whether the documented partial accuracy improvement is acceptable to merge now; there is no concrete mechanical defect for an automated repair lane.

Maintainer decision needed

  • Question: Should this bounded ancestry fix merge now despite the remaining roughly twofold discrepancy against OpenAI’s reported total?
  • Rationale: The implementation appears internally consistent and improves the supplied dataset substantially, but deciding whether an intentionally incomplete accounting correction is preferable to waiting for a complete model requires maintainer judgment.
  • Likely owner: steipete — Repository ownership and prior review history make this the best available owner for accepting or rejecting the partial-fix boundary.
  • Options:
    • Merge the bounded correction (recommended): Accept the safer ancestry-resolution improvement now and keep the linked issue open for the remaining counter-lineage work.
    • Hold for complete accounting: Delay merging until another patch explains enough of the remaining discrepancy to establish trustworthy totals on the supplied dataset.
    • Request broader real-data sampling: Ask for redacted before-and-after totals from additional forked and non-forked sessions before choosing whether to land the partial fix.

Security
Cleared: The diff changes local parsing, lookup, generated hash metadata, and tests without adding dependencies, external execution, credential handling, or broader permissions.

Review details

Best possible solution:

Land the narrow ownership and ancestry correction only if maintainers accept it as an incremental accuracy improvement, while keeping #2037 open for the remaining cumulative-counter discrepancy and additional real-session validation.

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

Yes at source level: copied session_meta can map multiple rollout files to one identifier under current main, and the added focused tests exercise that path. The original private session dataset was not independently rerun here, so confidence in the exact real-world magnitude is medium.

Is this the best way to solve the issue?

Yes as a narrow partial correction: canonical filename ownership and fail-open ambiguity handling are safer than speculative global suppression. It is not a complete solution to the linked issue’s remaining cumulative-counter overcount.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal-priority accuracy bug affecting displayed usage and cost, with a bounded patch and no evidence of data loss, security bypass, or runtime unavailability.
  • add merge-risk: 🚨 other: Merging changes user-visible token and cost accounting while the supplied real dataset still differs materially from the provider-reported total.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body reports an after-fix run against the reporter’s real session data, including concrete before, after, and provider totals; this proves improvement while transparently showing the remaining gap.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body reports an after-fix run against the reporter’s real session data, including concrete before, after, and provider totals; this proves improvement while transparently showing the remaining gap.

Label justifications:

  • P2: This is a normal-priority accuracy bug affecting displayed usage and cost, with a bounded patch and no evidence of data loss, security bypass, or runtime unavailability.
  • merge-risk: 🚨 other: Merging changes user-visible token and cost accounting while the supplied real dataset still differs materially from the provider-reported total.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body reports an after-fix run against the reporter’s real session data, including concrete before, after, and provider totals; this proves improvement while transparently showing the remaining gap.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body reports an after-fix run against the reporter’s real session data, including concrete before, after, and provider totals; this proves improvement while transparently showing the remaining gap.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Repository ownership and the prior fork-accounting PR history make this the strongest available route for deciding whether the bounded partial correction should land. (role: likely reviewer and decision owner; confidence: medium; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift)
  • iam-brain: The linked issue attributes earlier accounting work to this contributor, who also supplied the current real-session investigation and focused follow-up patch. (role: recent usage-accounting contributor; confidence: medium; commits: 845c8b61636c; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Tests/CodexBarTests/CodexSessionFileIndexTests.swift, Tests/CodexBarTests/Issue2037ScannerIntegrationTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@steipete

Copy link
Copy Markdown
Owner

Thank you for identifying the inherited-token direction early; this was directionally right and helped frame the later scanner work.

This implementation now predates and conflicts with #2118, #2208, and the copied-prefix fix in #2228, so I’m closing it as superseded. If the post-#2228 measurement leaves a specific, reproducible residual, a freshly rebased and narrowly scoped PR for that remaining case would be very welcome.

@steipete steipete closed this Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. 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.

Ultra-mode Terra and Sol sessions can overcount forked context

2 participants