fix: resolve Codex fork duplication when parent is unresolved (#2108)#2121
fix: resolve Codex fork duplication when parent is unresolved (#2108)#2121Yuxin-Qiao wants to merge 9 commits into
Conversation
7cce315 to
0a9acd4
Compare
![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: 7cce3156bc
ℹ️ 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".
| Self.rememberCodexRows( | ||
| rows, | ||
| sessionId: session.id, | ||
| sessionId: forkedFromId ?? session.id, |
There was a problem hiding this comment.
Because this changes the Codex row de-duplication semantics, the producer key needs to change as well; otherwise existing caches written with the old child-scoped row keys are still loaded as valid and unchanged forked files keep their previously double-counted days until a manual force rescan. I checked Scripts/regenerate-codex-parser-hash.sh --check, and it reports Sources/CodexBarCore/Generated/CodexParserHash.generated.swift is stale, so normal refresh will not invalidate those old cache entries.
Useful? React with 👍 / 👎.
| let uniqueRows = Self.uniqueCodexRows( | ||
| rows: parsed.rows, | ||
| sessionId: sessionId, | ||
| sessionId: forkedFromId ?? sessionId, |
There was a problem hiding this comment.
When two forked child sessions from the same unresolved parent have real child token_count rows without turn_id and the same model/day/token delta at the same event index, this parent-scoped key treats the second child's real usage as a duplicate and drops it. The replayed parent prefix needs cross-child de-duping, but applying forkedFromId to every parsed row also collapses legitimate child work in last-usage-only logs that lack turn IDs.
Useful? React with 👍 / 👎.
| Self.rememberScannedCodexFile( | ||
| input: input, | ||
| session: CodexScannedSession(id: current.sessionId, days: current.days), | ||
| forkedFromId: current.forkedFromId, |
There was a problem hiding this comment.
If the child file that previously owned the replayed parent prefix is later deleted or falls out of the scan roots, this fresh-cache path keeps the remaining forked siblings exactly as previously filtered and only records their parent-scoped keys after accepting their cached days. Removing the old owner then subtracts the sole cached copy of the replay, and the remaining sibling is never reparsed to reclaim those rows, so the report undercounts until a force rescan.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 14, 2026, 2:57 AM ET / 06:57 UTC. Summary Reproducibility: yes. at source level: the redacted fork-family fixture and issues #2037 and #2108 establish replay overcount, but this exact head has not been run against the affected frozen corpus. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Adopt one provenance contract for the canonical fork-accounting issue, fix the two cache/turn-state defects, integrate complementary leaf-identity and parent-fingerprint work from the proof-positive related PR, and verify identical redacted cold and repeated warm totals on the frozen affected corpus. Do we have a high-confidence way to reproduce the issue? Yes at source level: the redacted fork-family fixture and issues #2037 and #2108 establish replay overcount, but this exact head has not been run against the affected frozen corpus. Is this the best way to solve the issue? Unclear: turn provenance can support safe deduplication, but the current patch still mishandles parent removal outside the scan window and stale turn IDs, and maintainers must consolidate it with the competing identity and cache design. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b41715f3e3fb. Label changesLabel 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 (16 earlier review cycles; latest 8 shown)
|
0a9acd4 to
a27cdf4
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
a27cdf4 to
be123f1
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be123f1e2e
ℹ️ 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".
| Self.rememberCodexRows( | ||
| rows, | ||
| sessionId: session.id, | ||
| sessionId: forkedFromId ?? session.id, |
There was a problem hiding this comment.
When a forked session is kept from a fresh cache and the same session also exists as another copied file/root, child-only token_count rows that lack turn_id are remembered here under forkedFromId instead of the child session.id. The duplicate file's uniqueCodexRows checks the child session namespace for those untagged rows, does not see the first cached copy, and counts the child work again; pass the child session id through with the separate forkedFromId argument so only turn-tagged replay rows use the parent namespace.
Useful? React with 👍 / 👎.
| if let forkedFromId = cached.forkedFromId { | ||
| let parentIsResolved = (try? context.resources.fileIndex.fileURL(for: forkedFromId)) != nil | ||
| if !parentIsResolved { |
There was a problem hiding this comment.
When a child fork was cached while its parent was unresolved, its cached days can include the replayed parent prefix. If that parent file later becomes available and sorts before the child in the scan, this branch now treats the child cache as fresh because parentIsResolved is true, so the parent file contributes the same rows and the stale child totals are kept without row-key filtering; that double-counts the replay until a force rescan, so cache freshness needs to account for the parent-resolution transition.
Useful? React with 👍 / 👎.
|
Addressed the cache-ownership review finding.
@codex review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afcb524161
ℹ️ 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".
| if let forkedFromId = cached.forkedFromId { | ||
| let parentIsResolved = (try? context.resources.fileIndex.fileURL(for: forkedFromId)) != nil | ||
| if !parentIsResolved { | ||
| return false |
There was a problem hiding this comment.
When a cached child still points at a deleted/missing parent, this block returns false before the fresh-cache path can reuse the repaired codexRows, so the same unchanged fork files are reparsed on every refresh for as long as the parent log remains absent. The sibling invalidation below already repairs ownership after a disappearance, and cachedCodexRowsNeedIdentityRescan already rejects entries without stable rows, so this turns the missing-parent case into a persistent cache miss and can make menu refreshes expensive for large fork families.
Useful? React with 👍 / 👎.
| Self.rememberCodexRows( | ||
| rows, | ||
| sessionId: session.id, | ||
| sessionId: forkedFromId ?? session.id, |
There was a problem hiding this comment.
This call passes the parent ID as sessionId and leaves the new forkedFromId argument nil, so rememberCodexRows stores even fork rows with turnID == nil in the parent namespace. uniqueCodexRows only parent-scopes rows that actually have a turn ID; when a fresh cached forked file is kept and another copy of the same session is encountered later, untagged rows are looked up under the child session and do not match the remembered keys, allowing duplicate usage to be counted. Pass session.id plus forkedFromId to keep the remembered keys consistent with the uniqueness check.
Useful? React with 👍 / 👎.
| let siblingPaths = cache.files.compactMap { path, usage in | ||
| forkFamiliesNeedingRescan.contains(usage.forkedFromId ?? "") ? path : nil |
There was a problem hiding this comment.
When the cache retains a wider window, this selects every cached file in the fork family, including siblings whose days are outside the current scan window and therefore are not present in files. Those paths are removed from cache.files, but only siblingFiles from the current files list are rescanned; the code then keeps the old retained since/until keys, so a later report inside that supposedly retained range can be served from cache with those historical sibling days missing until another refresh happens to rescan them.
Useful? React with 👍 / 👎.
| forkedFromId: current.forkedFromId, | ||
| rows: cachedRows, |
There was a problem hiding this comment.
This fresh-cache path only remembers cachedRows; it never re-runs uniqueCodexRows unless the same child session already contributed. If a real parent file is later restored, or an earlier sibling is parsed before this cached child, the already-seen parent-scoped turn keys are ignored and current.days can keep counting replayed parent rows that should now be removed. Cached fork files need the same row-level filtering whenever their parent-scoped keys collide with state, not just when sessionAlreadyContributed is true.
Useful? React with 👍 / 👎.
|
Follow-up pushed as c15ef6e: regenerated |
clawsweeper
Bot
removed
rating: 🧂 unranked krab
|
Follow-up pushed as f233116: regenerated the parser hash again after the explicit lint annotation changed the hashed parser source. The latest CI log identified only this generated-artifact mismatch. @clawsweeper re-review |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Rechecked the current PR head The tests cover the unresolved-parent and fork-family cache transitions in the production scanner/cache path. I also checked the local machine for a real fork-transition corpus, but it does not contain a usable fork metadata sample, so I am not claiming a live corpus reproduction or attaching private session data. The remaining proof boundary is specifically a public/redacted unresolved-parent corpus supplied by the maintainer or CI environment. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Fixed the four cache-transition findings on
Validation: |
|
Follow-up: commit Verified locally:
The fixture is the committed, redacted 4d90→52bf fork family with a documented scanner oracle (deduped 52,185,847 vs naive 100,916,095 units); it is executable evidence, not UI imagery. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 827cbfbb96
ℹ️ 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".
| } | ||
| } | ||
| let siblingFiles = files.filter { siblingPaths.contains($0.path) } | ||
| scanState = CodexScanState() |
There was a problem hiding this comment.
When a deleted cached fork causes this sibling-rescan path to run, resetting scanState here drops all row keys already remembered from the first pass (including a real parent file or earlier contributor whose days remain in cache). For fork logs whose parent baseline is still unresolved (for example missing/unparseable fork timestamps), the sibling reparse then no longer sees those parent-scoped keys and can count the replayed parent rows again on top of the retained parent/earlier-file totals. Rebuild the state from retained cache entries or avoid discarding the earlier scan state before rescanning just the sibling files.
Useful? React with 👍 / 👎.
| if let forkedFromId = old.forkedFromId { | ||
| forkFamiliesNeedingRescan.insert(forkedFromId) | ||
| } |
There was a problem hiding this comment.
When an unscanned cached file is removed, this only records the removed file's own forkedFromId. If the removed file is the real parent of cached fork children (old.sessionId == P and old.forkedFromId == nil), those children were previously parsed with a resolved baseline and their cached rows omit the replayed parent prefix; after the parent file is deleted or leaves the roots, no family rescan is scheduled for children with forkedFromId == P, so the parent's usage is subtracted from the cache and never reclaimed until a force rescan.
Useful? React with 👍 / 👎.
| let useParentScope = forkedFromId != nil && row.turnID != nil | ||
| let activeSessionId = useParentScope ? forkedFromId : sessionId |
There was a problem hiding this comment.
This treats any non-nil row turn ID as parent provenance, but currentTurnID is only updated from task_started; turn_context IDs are ignored. In fork logs where child work starts with a turn_context carrying the child ID but no intervening task_started, the child token rows keep the replayed parent turn ID, so sibling children with matching event index/model/delta are deduped under the parent even though they are legitimate child usage.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Current-head follow-up: Validated locally: Please review the current head together with the prior redacted fork-corpus evidence. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Follow-up pushed in 2aec564 for the remaining fork-cache correctness threads. Addressed behavior:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aec564a49
ℹ️ 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".
|
|
||
| if !forkFamiliesNeedingRescan.isEmpty { | ||
| let siblingPaths = cache.files.compactMap { path, usage in | ||
| forkFamiliesNeedingRescan.contains(usage.forkedFromId ?? "") && filePathsInScan.contains(path) |
There was a problem hiding this comment.
Fresh evidence in this revision is the new filePathsInScan.contains(path) filter: when a cached parent file is deleted during a narrow refresh for the parent day, fork children parsed while that parent was resolved usually have only child-day days, so they are not in files and are left with cached rows that omitted the replayed parent prefix. Because the cache then retains the wider window, a later child-day report can reuse those stale child entries after the parent contribution was removed, undercounting until a force rescan; invalidate or rescan all cached children in the fork family, not only paths in the current scan list.
Useful? React with 👍 / 👎.
| if let turnID { | ||
| currentTurnID = turnID |
There was a problem hiding this comment.
Fresh evidence in this revision is the new non-nil-only assignment here: when a fork replay leaves currentTurnID set to a parent task_started and the following child turn_context has no turn ID, this branch keeps the parent ID active. Subsequent token_count rows without their own turn_id are then keyed under forkedFromId with the parent turn and can be dropped as replay duplicates, so a new turn context should clear the active turn before applying a non-nil ID (with the same behavior mirrored in the JSON fallback).
Useful? React with 👍 / 👎.
|
Closing this duplicate candidate to reduce competing Codex fork-accounting contracts. The underlying replay-overcount report remains important in #2108 / canonical #2037.\n\nThe safer merge-ready landing vehicle is #2118: it uses canonical leaf-session identity plus parent-file cache dependencies, includes focused cold/warm cache regressions and frozen-corpus evidence, and avoids this branch's unresolved missing-parent and stale-turn-ID undercount risks.\n\nPlease consolidate any unique coverage only if it is demonstrably absent from #2118; otherwise maintaining one canonical implementation will make review and release validation substantially clearer. |
2 participants
This PR resolves #2108.
Problem
When a child session is forked from a parent, the child session logs often replay the parent session's token usage events at the beginning of the file.
If the parent session is unresolved/missing, and the logs only contain
last_token_usage(withouttotal_token_usage), CodexBar previously had no watermark and no way to filter out the replayed parent events. Sibling child sessions would thus repeat-count the parent prefix, leading to duplicate token usage counting.Solution
Use the parent's session ID (
forkedFromId) instead of the child'ssessionIdas the key-namespace for row deduplication keys whenforkedFromIdis present.This ensures that any replayed parent turns (which have the same turn ID in all sibling child sessions) are recognized as duplicates of the parent session and are counted exactly once across all sibling sessions, rather than being multiplied for each child session.