Skip to content

fix(codex): preserve distinct requests when cumulative usage is missing - #1264

Open
roy-tong wants to merge 2 commits into
getagentseal:mainfrom
roy-tong:fix/codex-no-total-events
Open

fix(codex): preserve distinct requests when cumulative usage is missing#1264
roy-tong wants to merge 2 commits into
getagentseal:mainfrom
roy-tong:fix/codex-no-total-events

Conversation

@roy-tong

@roy-tong roy-tong commented Sep 6, 2026

Copy link
Copy Markdown

When total_token_usage is absent, the old cumulative guard and all-zero deduplication key collapse subsequent requests. Keying only on last usage also loses distinct requests with equal token amounts.

This revision:

  • applies the cumulative guard only to a finite, nonnegative reported total_tokens;
  • treats absent, null and partial totals as missing identity rather than zero;
  • uses source path and physical record offset for fallback identity, preserving distinct equal-usage rows and stable same-file rereads;
  • deliberately makes no cross-file replay-deduplication claim when cumulative identity is unavailable;
  • invalidates both Codex result and provider/session caches so existing aggregates are reparsed.

Validation: 37 tests pass across the relevant Codex test files, including the new three-case regression and cache invalidation coverage; TypeScript --noEmit passes. The three missing-cumulative regressions fail on the previous PR head and pass with this revision. Fixtures are synthetic.

@roy-tong roy-tong changed the title fix(codex): keep token_count events in sessions that never write total_token_usage fix(codex): preserve distinct requests when cumulative usage is missing Sep 7, 2026

@ozymandiashh ozymandiashh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and the cache bump. Two blockers.

  1. tests/providers/codex.test.ts:1225 fails (still dedups consecutive zero-cumulative duplicates: expected 1 call, got 2), on this head and on the head merged with current main. docs/providers/codex.md:164 asks for exactly that suite plus tests/parser-filter.test.ts to be run when the dedup key shape changes, and the PR template requires npm test to pass.

  2. The failing test is not stale; it is #257's deliberate decision. #257 changed the guard from cumulativeTotal > 0 to prevCumulativeTotal !== null specifically to stop double-counting consecutive zero-cumulative token_count events. Your new tests/codex-missing-cumulative.test.ts asserts three identical consecutive events produce three calls, which reinstates exactly what #257 removed. Both sides rest on synthetic fixtures. The only thing that settles it is a real Codex rollout JSONL: does Codex ever repeat a token_count event for the same request? If you have one, attach a redacted excerpt (per docs/providers/codex.md "Real data first"). If it does not repeat, delete the old test with that justification in the commit message; if it does, narrow the fix so byte-identical consecutive records still collapse.

On your question about double-counting: I probed it and all-cumulative and mixed sessions still behave like main thanks to the seenKeys backstop. The one weakened path is a forked session lacking cumulative totals outside the 5 s cutoff, where the forkedFromId replay protection no longer applies. You acknowledge the trade-off; please state it in the code comment you replaced rather than deleting the #1088 invariant note outright.

Also update docs/providers/codex.md:40-42 (dedup key shape) and the Quirks note on prevCumulativeTotal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants