Contain Codex interleaved overcount and add fork provenance fixtures (#2037)#2082
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2433b97a36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| billingSuppressedTokenOrdinals: context.resources | ||
| .billingSuppressedTokenOrdinalsByFilePath[input.fileURL.path] ?? [], |
There was a problem hiding this comment.
Because scanCodexFile tries the fresh-cache path before this rescan path, an unchanged sibling that was already cached never sees billingSuppressedTokenOrdinalsByFilePath when a new earlier sibling for the same missing parent appears. In that scenario the old file keeps its cached full-prefix rows while the new owner also bills the shared prefix, so the copied prefix is double-counted until a force rescan or cache bust; files with newly computed suppressions need to bypass the fresh-cache path or persist the suppression state in the cache.
Useful? React with 👍 / 👎.
| // Match inheritance discovery against cache + in-scan files only. Avoid `indexRoots()` | ||
| // crawls on every refresh for confirmed-missing parents; previously scanned parents are | ||
| // already seeded into `fileIndex` via `cachedCodexSessionIndex`. | ||
| if Self.codexCacheContainsSession(cache, sessionId: parentId) { |
There was a problem hiding this comment.
When a parent session is still in cache.files but its JSONL has since been deleted or moved out of the active roots, this raw cache lookup still returns true before stale entries are pruned later in loadCodexDaily. The resolver/index will not be able to read that parent, so the children are parsed as unresolved while this block has skipped missing-parent prefix suppression, billing the siblings' copied prefix more than once for that refresh and saving the overcount; validate the cached parent's path exists under the roots or rely on the file index instead of the raw cache.
Useful? React with 👍 / 👎.
|
Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 7:42 PM ET / 23:42 UTC. Summary Reproducibility: yes. at high confidence from source and fixtures: sanitized interleaved and fork-family inputs have hand-calculated scanner oracles, and the repository owner reports exact after-fix results from the signed application and bundled CLI. The original private Ultra corpus itself is not committed, so the classification remains source-reproducible rather than a direct current-main live reproduction. Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the exact proven head after required checks pass, close the incorporated and independently unmergeable #2066 path, and keep #2037 open for the fixture-gated cross-file provenance ledger and exact smaller-lineage accounting. Do we have a high-confidence way to reproduce the issue? Yes at high confidence from source and fixtures: sanitized interleaved and fork-family inputs have hand-calculated scanner oracles, and the repository owner reports exact after-fix results from the signed application and bundled CLI. The original private Ultra corpus itself is not committed, so the classification remains source-reproducible rather than a direct current-main live reproduction. Is this the best way to solve the issue? Yes for the scoped Phase 1 objective. The bounded replay and watermark policy prevents multiplicative overcount without relying on unsafe token-value identity, while explicitly leaving exact cross-file provenance to the canonical follow-up. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7465c6a11674. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (5 earlier review cycles)
|
Zihao-Qi
force-pushed
the
codex/fix-2037-fork-family-provenance
branch
from
2433b97 to
df637ec
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🦪 silver shellfish
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1816116e5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return [ | ||
| last.input, | ||
| last.cached, | ||
| last.output, | ||
| total.input, |
There was a problem hiding this comment.
When two children of the same unresolved parent have distinct token events at the same ordinal but coincidentally identical last/total counts (for example different prompts/models or timestamps that produce the same token totals), this fingerprint makes buildProvisionalPrefixBillingSuppressions treat them as a copied prefix and suppress all but the first sibling. Because the key omits copy-stable identity such as timestamp/model/line identity and is only token values, legitimate sibling work can be undercounted; the new provenance spec also calls out that token-value equality alone must not drive dedupe.
Useful? React with 👍 / 👎.
Zihao-Qi
changed the title
Stop gap-recount inflation with post-latch watermark containment and a min(last, containedTotalDelta) cap, while documenting the intentional smaller-lineage undercount deferred to Phase 2. Co-authored-by: Cursor <cursoragent@cursor.com>
Lock ordinary Codex fork copy identity with sanitized goldens, suppress shared missing-parent prefixes once, and keep suppression ordinals aligned with the scan parser without claiming steipete#2037 closed. Co-authored-by: Cursor <cursoragent@cursor.com>
Propagate refresh cancellation through fingerprint pre-scans, ignore stale cached parents, and force-rescan suppressed siblings when the fork family grows. Co-authored-by: Cursor <cursoragent@cursor.com>
steipete
force-pushed
the
codex/fix-2037-fork-family-provenance
branch
from
5441ff7 to
be56cc9
Compare
|
Maintainer pass complete on the rebased head Changes from the original PR:
Proof:
Live package proof used the Developer ID-signed
Host and guest results matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be56cc9aff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func component(water: Int, counted: Int, current: Int) -> Int { | ||
| if current >= water { | ||
| return max(0, current - max(water, counted)) | ||
| } | ||
| return max(0, current - counted) |
There was a problem hiding this comment.
When an interleaved row drops in one component but rises in another, this component-wise branch still bills the rising component from the lower lineage. For example, totals-only snapshots (input: 1000, output: 1) followed by (input: 10, output: 1000) latch interleaving because input drops, but then add 999 output tokens, so the file reports 2000 tokens even though no observed cumulative snapshot exceeds 1010. That reintroduces overcounting for mixed input/output lineages; after any component indicates a lineage flip, the event should be contained as a whole rather than combining per-component watermarks from different snapshots.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
Contain interleaved cumulative counters with bounded replay tracking, retain conservative fail-open behavior for ambiguous missing-parent siblings, and add sanitized fork-family oracles plus regression coverage. Co-authored-by: Zihaoqi <Zihao-Qi@users.noreply.github.com>
2 participants
Summary
33ce→3869,4d90→52bf, and missing-parent siblings).#1164accounting with scanner integration tests.Missing-parent safety decision
5441ff75.last_token_usage/total_token_usagevectors at the same ordinal. Treating those counters as event identity can silently undercount legitimate work.token_count, and adding more token fields only reduces collision probability. Therefore missing-parent siblings fail open: retain possible copied-prefix overcount rather than destructively suppress ambiguous rows.Deferred provenance work
Relationship to #2066
Fixes #2037.Test plan
swift test --filter Issue2037swift test --filter 'codex interleaved'swift test --filter 'codex forked child'make checkmake test— 618 selections, 52/52 groups passed