Fix Codex fork parent cache invalidation#2118
Conversation
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 5:49 AM ET / 09:49 UTC. Summary Reproducibility: yes. at source level: current main selects a parent-oriented session alias before the child leaf ID, and the supplied frozen-corpus A/B run shows the resulting current-main overcount and stable after-fix cold/warm totals. 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. Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land one canonical patch that preserves leaf-ID precedence, parent-fingerprint cache invalidation, parser-hash rotation, and all compatible ancestry improvements, with focused synthetic tests plus stable cold/warm frozen-corpus output. Do we have a high-confidence way to reproduce the issue? Yes at source level: current main selects a parent-oriented session alias before the child leaf ID, and the supplied frozen-corpus A/B run shows the resulting current-main overcount and stable after-fix cold/warm totals. Is this the best way to solve the issue? Yes for the code direction: leaf identity plus explicit parent-cache dependency tracking is narrower and safer than global fork suppression. The unresolved question is which overlapping branch should become the canonical landing vehicle. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c852c135e10c. 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 (4 earlier review cycles)
|
|
@clawsweeper re-review I reproduced the current-release Release screenshot:
The corpus contains 1,692 frozen JSONL files. Each run used a fresh isolated
This PR already fixes the observed The cache boundary should remain explicit: parser and attribution changes must regenerate Please use this run as real-data proof for #2118. For clustering, #2118 is the complete candidate for the current-release fork identity/cache and |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
Superseded by ready PR #2120, which preserves @xx205 attribution, includes the same valid implementation, adds the #1042 cache-versioning invariant, and carries frozen-corpus proof for the Unknown-model symptom. @clawsweeper cluster #2118 under #2120 and close this draft; review #2120 as the independently mergeable candidate. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Provenance correction: the runtime evidence claimed by this PR is limited to the controlled local 578-file experiment now documented in the PR body. The same frozen input was scanned with current main and this PR using separate empty caches, followed by two warm-cache scans. Any third-party validation elsewhere in the discussion is independent and must not be attributed to the contributor-provided proof for this PR. Please evaluate the redacted local table in the PR body for the proof gate. |
|
@clawsweeper re-review |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
|
Status and provenance clarification: #2118 is the original implementation PR and remains actively maintained. It now includes a controlled local A/B plus cold/warm proof, incorporates the cache-gate ordering optimization, and is ready for maintainer review. #2120 independently cherry-picks the substantive implementation and adds separate documentation and evidence; there has been no contributor agreement that it supersedes or closes #2118. Evidence from the two PRs should remain separately attributed. Please leave the landing decision to maintainers. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Routing correction: I closed #2120 as a duplicate landing candidate after comparing it with the latest #2118 tree. #2118 is the original, actively maintained implementation and should be treated as the canonical identity/cache candidate. My 1,692-file frozen-corpus A/B remains independent supporting evidence for #2118; @xx205's 578-file cold/warm run remains the contributor proof for this PR. Please preserve that evidence provenance, keep #2113 classified as partial ancestry overlap, and leave #2037 open for the broader accounting problem. |
devYRPauli
left a comment
devYRPauli
left a comment
There was a problem hiding this comment.
Traced the root bug - it's real on main: the session_meta parse resolved identity as payload["session_id"] ?? payload["sessionId"] ?? payload["id"], so a forked child whose session_id alias carries the parent's id got filed under the parent's identity, corrupting the session->file index and row-dedup keys. Preferring the leaf id is right, and good that this updates all three parse sites (~1604/1742/2213), not just one. The 578-file before/after corpus (3.37B -> 522M tokens, stable across cold/warm rescans) is convincing.
One coordination note: this overlaps #2113 and #2121 - all three touch the identity / keepCachedCodexFileIfFresh path and will conflict. Since this one carries the real-corpus proof, landing it first and rebasing the other two on top would avoid the churn.
|
I like this simple fix for the 5.6 multiagents v2 bloat. My successive PRs #2155 #2156 #2157 #2158 aim to tackle more of the larger problem and should be complementary your fix but with added performance, lineage tracking, and containment improvements to overall increase token accounting accuracy. So far, it's strongly backed by my comparisons between the reported OpenAI token usage on my codex account vs the results of the current "legacy" scanner without this specific fix. #2118 falls short of #2158's overhaul and improvements (with many more changes, of course):
|
Co-authored-by: Xu Xiang <xx205@outlook.com>
8152bca to
45dfa3f
Compare
steipete
changed the title
![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: 45dfa3f959
ℹ️ 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 false | ||
| } | ||
| if let parentSessionId = cached.forkedFromId { | ||
| guard let cachedDependencyKey = cached.forkBaselineDependencyKey else { return false } |
There was a problem hiding this comment.
When the cached file is a Codex subagent with forkedFromId, the parser intentionally treats the parent id as lineage rather than a baseline and never records a parent dependency key, so forkBaselineDependencyKey remains nil. This guard then rejects an otherwise fresh cache entry on every refresh, causing unchanged subagent logs to be fully reparsed repeatedly; in subagent-heavy histories this defeats the warm cache and makes refresh cost scale with old logs. Consider skipping this dependency check for independent/subagent forks or storing a non-parent sentinel.
Useful? React with 👍 / 👎.
|
Exact-head maintainer proof for
|
5 participants
Summary
Why
Fork accounting depends on the parent totals at the fork timestamp, but cache freshness previously tracked only the child file. A child cached while its parent was missing—or before the parent changed—could keep stale inherited totals indefinitely.
Current
mainalready contains the leaf-session identity correction from overlapping work. This branch is intentionally narrowed to the remaining parent-cache dependency bug.Safety
The cache field is optional. Existing Codex caches rotate through the generated parser producer key; the on-disk schema and artifact layout remain compatible. Parent snapshots are fingerprinted before and after parsing. If the parent changes during both read attempts, the unresolved result is deliberately not reusable on the next refresh.
Testing
swift test --filter 'CostUsageScannerBreakdownTests|CostUsageCacheTests'— 98 tests passedmake check— passedmake test— 651 selections in 55 groups passed; zero retries or failuresCo-authored-by: Xu Xiang xx205@outlook.com