Conversation
ozymandiashh
left a comment
There was a problem hiding this comment.
Thanks for the fix and the cache bump. Two blockers.
-
tests/providers/codex.test.ts:1225fails (still dedups consecutive zero-cumulative duplicates: expected 1 call, got 2), on this head and on the head merged with currentmain.docs/providers/codex.md:164asks for exactly that suite plustests/parser-filter.test.tsto be run when the dedup key shape changes, and the PR template requiresnpm testto pass. -
The failing test is not stale; it is #257's deliberate decision. #257 changed the guard from
cumulativeTotal > 0toprevCumulativeTotal !== nullspecifically to stop double-counting consecutive zero-cumulativetoken_countevents. Your newtests/codex-missing-cumulative.test.tsasserts 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 atoken_countevent for the same request? If you have one, attach a redacted excerpt (perdocs/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.
…ns still collapse (getagentseal#257) Real-data answer to the review's question: Codex does re-emit byte-identical token_count events. Scanned 53 public rollout sessions (codeset-ai/ codeset-release-evals, 1313 token_count events): 603 (46%) are byte-identical repeats of their predecessor (identical total AND last usage), 0 cumulative monotonicity violations, 54 events carry no total_token_usage at all. The missing-cumulative path now collapses a record whose info payload is byte-identical to the previous event's (getagentseal#257 semantics restored); only records with differing payloads count as distinct requests. prevInfoIdentity is persisted in the resume state. The getagentseal#1088 invariant note is restored at the dedupKey in adapted form; docs/providers/codex.md documents the three-layer dedup and the accepted fork-cutoff trade-off.
|
You asked for a real Codex rollout to settle it, so I scanned one: codeset-ai/codeset-release-evals (public eval corpus) — 53 sessions, 1313
Verbatim excerpt (lines 14/17 vs 24 of {"timestamp":"…T16:22:18.697Z","type":"event_msg","payload":{"type":"token_count","info":{"total_token_usage":{"input_tokens":11923,"cached_input_tokens":0,"output_tokens":208,"reasoning_output_tokens":54,"total_tokens":12131},"last_token_usage":{"input_tokens":11923,…identical…},"model_context_window":258400}}}
{"timestamp":"…T16:22:19.467Z","type":"event_msg","payload":{"type":"token_count","info":{…identical to the line above…}}}
{"timestamp":"…T16:22:22.661Z","type":"event_msg","payload":{"type":"token_count","info":{"total_token_usage":{"input_tokens":26873,…},"last_token_usage":{"input_tokens":14950,…}}}} ← next request, payload differsThat lands on your "it does repeat" branch, so 4c3a2b6 narrows the fix exactly as you specified:
|
Dismissed: both blockers verified as addressed on 4c3a2b6 — tests/providers/codex.test.ts (incl. the #257 zero-cumulative dedup test at :1201) and tests/parser-filter.test.ts pass; the fix was narrowed per your 'it does repeat' branch (byte-identical consecutive records collapse via info-payload identity, rate_limits excluded as payload sibling); #1088 invariant note restored at dedupKey; docs/providers/codex.md dedup + Quirks updated. Independent spot-check of codeset-ai/codeset-release-evals confirms the re-emission data (cited file: 9/20 byte-identical repeats, 0 monotonicity violations). Full suite green locally; CI never ran on the PR (action_required first-contributor gate).
ozymandiashh
left a comment
There was a problem hiding this comment.
Re-reviewed 4c3a2b6 — both blockers addressed exactly as specified.
- The #257 suite passes again: tests/providers/codex.test.ts 48/48 (incl. zero-cumulative dedup at :1201), tests/codex-missing-cumulative.test.ts 6/6 (both halves: identical ×3 → 1 call, differing ×3 → 3 calls), tests/parser-filter.test.ts 11/11. tsc --noEmit clean. Full npm test: 3744 passed / 0 failed (one lock-timing flake on the first run, passes in isolation on both main and this head).
- The real-data answer checks out: independently scanned the cited file from codeset-ai/codeset-release-evals — 20 token_count events, 9 byte-identical repeats (45%), 0 monotonicity violations, plus one null-info event (handled by the est path). The byte-identity collapse on info (rate_limits correctly excluded as a payload sibling) is the right narrowing.
Also verified: #1088 invariant note restored at dedupKey with the fork-cutoff trade-off stated; docs dedup + Quirks updated; prevInfoIdentity persisted in resume state; both cache layers invalidated (v16 + parse-version suffix). My previous review is dismissed as addressed.
ozymandiashh
left a comment
There was a problem hiding this comment.
Reviewed the full diff and validated it beyond CI: the patch applies cleanly on current main, and applied on a temp branch the codex suites (new missing-cumulative tests plus providers/codex and pricing-1075) pass 61/61 locally. The three-layer dedup is the right shape — byte-identity collapse for re-emissions with real-world measurements behind it (603/1313 events), equal-cumulative guard only when a finite nonnegative total is actually reported, and physical record position as identity when cumulative is absent. The weakened cross-file replay protection without cumulative is a documented, reasoned trade-off. Cache bumps (v16 + missing-cumulative-v1 parse version) follow the house convention so stale caches reprice.
iamtoruk
left a comment
There was a problem hiding this comment.
Ran this against my own corpus: 1,399 rollouts, 136,145 token_count events. The shape this fixes (info present, total_token_usage missing) shows up 0 times. All 685 no-cumulative events are info: null, one per session, the opening rate-limit ping, and those already take the estimate path at codex.ts:1108.
Checked your corpus too. Spread-sampled 30 sessions from codeset-release-evals: 1,308 events, 30 null-info, 0 of the target shape. The "54 of 1313" figure is those null-info pings. The re-emission half of your data holds up (624 of 1308 byte-identical repeats), but that half is already collapsed by the cumulative guard.
Built both sides. audit --provider codex --period lifetime is byte-identical: 51,399 calls, $5,226.17. Delta is $0.00.
Tests and tsc are green, rebase onto main is clean. So this is harmless, but the cache bump makes every user cold-reparse their whole Codex history to get the same numbers back. Drop CODEX_CACHE_VERSION back to 15 and drop the -missing-cumulative-v1 suffix.
Also fix the Quirks line in docs/providers/codex.md. It says those 54 events key on record position. They do not, they are null-info and go to the estimate path.
One thing worth noting: the codex:record:: fallback throws away the forkedFromId identity #1383 just added for sub-agent parents. Nothing hits it today, but if the missing-cumulative shape ever appears in a sub-agent rollout it double counts the replayed parent history.
Fix the docs and the version bumps and I will take it.
When
total_token_usageis 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:
total_tokens;Validation: 37 tests pass across the relevant Codex test files, including the new three-case regression and cache invalidation coverage; TypeScript
--noEmitpasses. The three missing-cumulative regressions fail on the previous PR head and pass with this revision. Fixtures are synthetic.