fix(codex): validate rollouts structurally, guard the parse path - #926
Open
ozymandiashh wants to merge 1 commit into
Open
fix(codex): validate rollouts structurally, guard the parse path#926ozymandiashh wants to merge 1 commit into
ozymandiashh wants to merge 1 commit into
Conversation
Ports two upstream fixes this branch never received (eece4cf, 4ff3497). **Discovery gated on a client identity string.** `isValidCodexSession` required `payload.originator` to start with "codex". But `originator` is a free-form client identity, not a format marker: anything driving `codex app-server` writes structurally identical rollouts with its own value — "t3code_desktop", "JetBrains.IntelliJ IDEA", whatever ships next. Every third-party frontend was silently dropped, and each one needed a new allowlist entry (getagentseal#626, getagentseal#873). Validation is now structural. Be clear about what that gate was and was not. It was never a security boundary — anyone able to write into the sessions directory could write `"originator":"codex-cli"` and pass it. It was accidental integrity protection, and removing it widens what gets ingested from those directories to any well-formed `session_meta` line. The trust boundary is unchanged: write access to the Codex home, which is itself configurable via `CODEX_HOME`. A crafted rollout can inflate cost or impersonate a project path, exactly as it could before by spelling the originator correctly. **Non-string fields on the parse path.** A garbage `timestamp` threw RangeError out of `toISOString()` and zeroed the session. Guarding only that one would have been the smaller half of the problem: the timestamps that reach emitted calls were unguarded too, and a numeric one produces `NaN-NaN-NaN` day buckets that the daily cache then keeps for ten years — silent, and persistent. Token counts could go NaN and slip past a `=== 0` check into reported cost. `session_id` and `forked_from_id` could coerce an object into a dedup key. All of those are now guarded; the fields still read through a raw cast are listed nowhere, because there are none left on this path. **The cache is bumped to 16.** Rollouts rejected before they were ever parsed now contribute usage, and nothing downstream can notice: the aggregator serves every day before today from this cache, with ten-year retention, so an upgrading user would keep pre-fix history forever while today disagreed with it. Reviewers split on this bump and both arguments are worth having. For it: cache versions are per-branch lineage, and landing unbumped leaves a user on this branch with stale history that merging main cannot repair retroactively. Against: most users are unaffected and pay a full re-derivation for nothing, and main is already at 16 for an unrelated timezone migration, so the numbers collide. It is bumped here because the change is correct on its own terms — whoever merges main should resolve the two meanings, probably as 17.
This was referenced Aug 5, 2026
fix(parser): keep both halves of a midnight-straddling turn, and stop --provider leaking claude
#929
Draft
ozymandiashh
added a commit
to ozymandiashh/codeburn
that referenced
this pull request
Aug 5, 2026
… invalidate cached history decodeKiroChatFile estimated input tokens from pendingUserMessage - the last human turn sliced to 500 chars - while output summed every bot char, so any multi-turn session or final prompt over 500 chars under-reported input tokens (and therefore costUSD) severalfold. The estimate is now the sum of every human turn's full character count, with the 500-char cap kept for the display userMessage only. That closes most of the gap but does NOT reach parity with the modern-execution, CLI-session and V2 arms: those count tool and system records as input - their code comments state tool results are fed back to the model - while the chat arm still counts only human records. Tool content demonstrably exists in the format (the G2 fixture carries a tool record), so the chat arm still under-reports; it just under-reports far less than before. The blast radius is the chat-file arm alone: the IDE-file dispatcher routes any record carrying a chat array plus metadata to decodeKiroChatFile, so this is chat-shaped Kiro IDE files, not every Kiro prompt. The identity-preamble exclusion now trims leading whitespace before its startsWith match. Pre-fix, a near miss (a leading newline, a BOM, a wrapper) was nearly harmless, because the preamble only mattered if it happened to be the last human turn; post-fix, every unmatched system-injected human record adds its FULL length to input, and preambles are large - a missed match is a silent multi-thousand-token inflation on every affected session. Leading whitespace tolerance is cheap (a genuine prompt never starts with whitespace plus an identity tag) and the failure asymmetry favours exclusion: a false negative inflates tokens, a false positive only skips a preamble. A renamed preamble remains a residual risk, noted in the near-miss regression test. Cached history is affected, which is what a user actually sees. session-cache serves unchanged files without invoking the provider parser, so bump kiro's PROVIDER_PARSE_VERSIONS fingerprint (ide-parsing-v1 -> v2) to force one re-parse of every already-cached kiro session; without it the pre-fix token and cost numbers would be served forever. The daily rollup ALSO needs invalidating for this fix to be fully visible: days finalized before the fix keep their pre-fix kiro cost in the daily cache, and ensureCacheHydrated re-derives them only on a version bump, a savings-config change, a timezone change, or an incomplete cache - the session-cache re-parse alone leaves finalized day totals untouched. No DAILY_CACHE_VERSION bump ships here, and the deferral rationale is honestly weak: the three PRs editing 15 -> 16 on the same line are the most trivially resolvable conflict class there is, and the sibling PR the deferral leans on (getagentseal#926) says itself the number is contested - main may already be at 16 for an unrelated reason, and the reconcile is "most likely as 17" - so this dependency may not survive conflict resolution. Nothing here fails if it does not, which is why the dependency now lives where a releaser actually looks: the CHANGELOG Unreleased/Fixed entry for this fix states that finalized-day totals refresh with the next daily-cache bump only for days whose kiro chat files still exist — sourceless days carry forward with their pre-fix totals under the v14 NEVER-LOSE rule — and that its absence only delays that refresh. A releaser reconciling getagentseal#926 against main's 16 will see the dependency in the changelog, not buried in this body. Update the G2 parity golden: A1 was pinned at 125 tokens for a 3000-char prompt (the 500-char slice / 4); the corrected value is 750 (3000 / 4), with a comment marking 125 as a pre-fix value so it is not restored. 3000, 2400 and 1000+1000 are all exact multiples of four, so add G2b pinning the estimator's rounding with an odd length (3001 chars -> 751 tokens; round and floor would both give 750). Add a money-path regression test (2400-char prompt -> 600 tokens, userMessage still 500-capped for display), a multi-turn accumulation test (an identical resubmitted prompt counts again - a real second model input; identity messages stay excluded), a near-miss identity test (leading-newline and BOM preambles stay excluded), and extend the kiro cache-invalidation test to pin the v1 -> v2 fingerprint bump.
ozymandiashh
added a commit
to ozymandiashh/codeburn
that referenced
this pull request
Aug 5, 2026
… invalidate cached history decodeKiroChatFile estimated input tokens from pendingUserMessage - the last human turn sliced to 500 chars - while output summed every bot char, so any multi-turn session or final prompt over 500 chars under-reported input tokens (and therefore costUSD) severalfold. The estimate is now the sum of every human turn's full character count, with the 500-char cap kept for the display userMessage only. That closes most of the gap but does NOT reach parity with the modern-execution, CLI-session and V2 arms: those count tool and system records as input - their code comments state tool results are fed back to the model - while the chat arm still counts only human records. Tool content demonstrably exists in the format (the G2 fixture carries a tool record), so the chat arm still under-reports; it just under-reports far less than before. The blast radius is the chat-file arm alone: the IDE-file dispatcher routes any record carrying a chat array plus metadata to decodeKiroChatFile, so this is chat-shaped Kiro IDE files, not every Kiro prompt. The identity-preamble exclusion now trims leading whitespace before its startsWith match. Pre-fix, a near miss (a leading newline, a BOM, a wrapper) was nearly harmless, because the preamble only mattered if it happened to be the last human turn; post-fix, every unmatched system-injected human record adds its FULL length to input, and preambles are large - a missed match is a silent multi-thousand-token inflation on every affected session. Leading whitespace tolerance is cheap (a genuine prompt never starts with whitespace plus an identity tag) and the failure asymmetry favours exclusion: a false negative inflates tokens, a false positive only skips a preamble. A renamed preamble remains a residual risk, noted in the near-miss regression test. Cached history is affected, which is what a user actually sees. session-cache serves unchanged files without invoking the provider parser, so bump kiro's PROVIDER_PARSE_VERSIONS fingerprint (ide-parsing-v1 -> v2) to force one re-parse of every already-cached kiro session; without it the pre-fix token and cost numbers would be served forever. The daily rollup ALSO needs invalidating for this fix to be fully visible: days finalized before the fix keep their pre-fix kiro cost in the daily cache, and ensureCacheHydrated re-derives them only on a version bump, a savings-config change, a timezone change, or an incomplete cache - the session-cache re-parse alone leaves finalized day totals untouched. No DAILY_CACHE_VERSION bump ships here, and the deferral rationale is honestly weak: the three PRs editing 15 -> 16 on the same line are the most trivially resolvable conflict class there is, and the sibling PR the deferral leans on (getagentseal#926) says itself the number is contested - main may already be at 16 for an unrelated reason, and the reconcile is "most likely as 17" - so this dependency may not survive conflict resolution. Nothing here fails if it does not, which is why the dependency now lives where a releaser actually looks: the CHANGELOG Unreleased/Fixed entry for this fix states that finalized-day totals refresh with the next daily-cache bump only for days whose kiro chat files still exist — sourceless days carry forward with their pre-fix totals under the v14 NEVER-LOSE rule — and that its absence only delays that refresh. A releaser reconciling getagentseal#926 against main's 16 will see the dependency in the changelog, not buried in this body. Update the G2 parity golden: A1 was pinned at 125 tokens for a 3000-char prompt (the 500-char slice / 4); the corrected value is 750 (3000 / 4), with a comment marking 125 as a pre-fix value so it is not restored. 3000, 2400 and 1000+1000 are all exact multiples of four, so add G2b pinning the estimator's rounding with an odd length (3001 chars -> 751 tokens; round and floor would both give 750). Add a money-path regression test (2400-char prompt -> 600 tokens, userMessage still 500-capped for display), a multi-turn accumulation test (an identical resubmitted prompt counts again - a real second model input; identity messages stay excluded), a near-miss identity test (leading-newline and BOM preambles stay excluded), and extend the kiro cache-invalidation test to pin the v1 -> v2 fingerprint bump.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports two upstream fixes this branch never received:
eece4cfand4ff3497.Discovery gated on a client identity string
isValidCodexSessionrequiredpayload.originatorto start with"codex". Butoriginatoris a free-form client identity, not a format marker: anything drivingcodex app-serverwrites structurally identical rollouts with its own value —t3code_desktop,JetBrains.IntelliJ IDEA, whatever ships next. Every third-party frontend was silently dropped, and each new one needed an allowlist entry (#626, #873).Validation is now structural.
Being straight about what that gate was. It was never a security boundary — anyone able to write into the sessions directory could write
"originator":"codex-cli"and sail through. It was accidental integrity protection, and removing it does widen ingestion from those directories to any well-formedsession_metaline. The trust boundary is unchanged: write access to the Codex home, which is itself configurable viaCODEX_HOME. A crafted rollout can inflate cost or impersonate a project path — exactly as it could before, by spelling the originator correctly.Non-string fields on the parse path
The upstream fix is about a garbage
timestampthrowingRangeErrorout oftoISOString()and zeroing the session. Porting only that would have been the smaller half:NaN-NaN-NaNday buckets that the daily cache then keeps for ten years — silent and persistent, worse than the crash;NaNand slip past a=== 0check straight into reported cost;session_idandforked_from_idcould coerce an object into a dedup key.All guarded. A reviewer caught that the first version of this PR covered two of roughly six unchecked fields while the message claimed the path was hardened — that gap is closed and the message no longer overstates it.
The cache bump, and the disagreement about it
Bumped to 16. Rollouts that were rejected before they were ever parsed now contribute usage, and nothing downstream can notice on its own: the aggregator serves every day before today from this cache, retention is ten years, so an upgrading user keeps pre-fix history forever while today's numbers disagree with it.
Two reviewers split on this, and both arguments are worth putting in front of you:
It is bumped because the change is correct on its own terms. Whoever merges main will need to reconcile the two definitions, most likely as 17.
Verification
Core 519, CLI codex 29, daily-cache 24 — all green. The new discovery tests are discriminating: the
t3code, JetBrains, no-originator and malformed-cwdcases all fail against the old gate, and the malformed-cwdone fails for a reason unrelated to the originator string.