Skip to content

Fix Codex fork identity and cache dependencies - #2120

Closed
hhh2210 wants to merge 3 commits into
steipete:mainfrom
hhh2210:codex/fix-codex-fork-identity-cache
Closed

Fix Codex fork identity and cache dependencies#2120
hhh2210 wants to merge 3 commits into
steipete:mainfrom
hhh2210:codex/fix-codex-fork-identity-cache

Conversation

@hhh2210

@hhh2210 hhh2210 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Status

Closed as a duplicate landing candidate after #2118 remained active and incorporated the same cache-gate ordering repair. #2118 is the original implementation by @xx205. This branch remains available as an audit trail for the independent runtime validation and review work below.

What this PR established

  • The implementation carried here prefers the fork leaf payload.id over parent-oriented session_id aliases and invalidates a cached fork when its parent baseline file changes. The substantive implementation came from Fix Codex fork parent cache invalidation #2118, with its Git authorship preserved.
  • An independent frozen-data run reproduced both the fork overcount and Unknown model symptom, then verified the fix on the same immutable input.
  • The cache boundary from refactor: derive Codex cost cache producer key from parser source hash #1042 remains explicit: parser and attribution changes rotate CodexParserHash; codex-vN changes only for an incompatible on-disk schema or cache layout.
  • Copilot's cache fast-path ordering finding was fixed and verified before this PR was closed.

Problem

A fork rollout can contain both:

  • payload.id: the child/leaf session identity
  • payload.session_id: the parent-oriented session identity

Choosing session_id first aliases the child to its parent. That breaks parent lookup and can keep a child cache entry fresh after its inherited parent baseline has changed. In the UI, the same identity error can surface as fork usage attributed to Unknown model:

CodexBar showing 84M tokens attributed to Unknown model

Independent frozen-data proof

I cloned my local Codex session tree into an isolated frozen corpus of 1,692 JSONL files and scanned it with isolated home and cache directories. This evidence is independent of the 578-file proof supplied by @xx205 on #2118.

Build Jul 13 total tokens Unknown model gpt-5.6-sol codex-auto-review
main@c852c135 705,781,187 27,654,293 663,872,726 14,254,168
Fixed implementation 394,500,055 0 380,245,887 14,254,168

A separate model-inheritance experiment produced no further change once leaf identity and parent dependency invalidation were correct, so that patch was excluded.

Validation

  • swift test --filter 'CostUsageCacheTests|CostUsageScannerBreakdownTests': 89 tests passed
  • make check: passed; SwiftFormat clean and SwiftLint reported 0 violations
  • Frozen 1,692-file A/B scan: 0 Unknown-model tokens after the fix

Canonical routing

Implementation credit and runtime-evidence credit are separate: the core code belongs to #2118/@xx205, while each contributor's frozen-corpus results retain their own provenance.

Copilot AI review requested due to automatic review settings July 13, 2026 09:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes Codex fork session identity selection and makes warm-cache correctness depend on the parent baseline file used for inherited fork totals, preventing forked child sessions from being aliased to their parent and avoiding stale fork accounting when the parent file changes.

Changes:

  • Prefer payload.id over payload.session_id when parsing Codex session identifiers (leaf identity wins).
  • Persist and validate a fork parent “baseline dependency key” so cached fork results invalidate when the parent file appears/changes.
  • Add focused scanner tests covering both leaf-identity selection and warm-cache parent dependency invalidation; regenerate CodexParserHash.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Tests/CodexBarTests/CostUsageScannerBreakdownTests.swift Adds regression coverage for fork leaf-ID preference and parent-baseline cache invalidation.
Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CacheHelpers.swift Stores a parent baseline dependency key in cache entries and uses it to decide cached-fork freshness.
Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift Updates fast/slow session-id parsing order to prefer leaf id; adds a dependency-key helper for parent baseline tracking.
Sources/CodexBarCore/Vendored/CostUsage/CostUsageCache.swift Documents Codex cache artifact versioning invariants and adds a new optional cached field for fork dependency tracking.
Sources/CodexBarCore/Generated/CodexParserHash.generated.swift Updates the tracked parser hash to reflect scanner/parser source changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 899 to 905
if let parentSessionId = cached.forkedFromId {
guard let cachedDependencyKey = cached.forkBaselineDependencyKey,
try cachedDependencyKey == (context.resources.inheritedResolver.dependencyKey(for: parentSessionId))
else { return false }
}

guard !Self.cachedCodexFileNeedsPriorityRescan(cached, context: context) else { return false }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0571f21. The priority-rescan, identity-rescan, and empty-row checks now run before the parent dependency lookup. The throwing lookup is also assigned to currentDependencyKey before comparison. Validation: 89 focused scanner/cache tests passed, and make check passed.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels Jul 13, 2026
@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 5:41 AM ET / 09:41 UTC.

Summary
The PR prefers leaf Codex fork identities, invalidates child cache entries when their inherited parent baseline changes, documents the parser-hash cache invariant, and adds focused scanner regressions.

Reproducibility: yes. The PR supplies a controlled current-main versus branch scan over the same frozen 1,692-file Codex corpus, reproducing Unknown-model attribution and fork overcount before the fix and eliminating them afterward.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 5 files; 208 additions, 11 deletions. The patch remains focused on the Codex scanner/cache model, generated parser hash, and regression coverage.
  • Frozen-corpus proof: 1,692 files; Unknown attribution reduced from 27,654,293 tokens to 0. The immutable-input comparison directly demonstrates the claimed accounting correction.
  • Focused validation: 89 scanner/cache tests passed. The affected identity and warm-cache paths have targeted regression coverage in addition to runtime proof.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #2037
Summary: The open issue is the canonical user report; this PR and two related PRs are candidate or partial fixes for overlapping fork-accounting paths, with this PR and #2118 requiring a human canonical-branch choice.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

Maintainer options:

  1. Decide the mitigation before merge
    Land one canonical leaf-identity and parent-cache-dependency implementation with preserved authorship and both proof records, then separately evaluate the non-overlapping parent-resolution behavior from Partially fix inherited token accounting across Codex session forks #2113 against the remaining overcount corpus.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

Maintainer decision needed

  • Question: Should this PR or Fix Codex fork parent cache invalidation #2118 be the canonical landing branch for the shared fork identity and cache invalidation implementation?
  • Rationale: Both branches remain open, mergeable, independently proven, and substantially identical, while the contributors explicitly disagree about supersession; branch selection and attribution require maintainer intent.
  • Likely owner: xx205 — They authored the shared substantive implementation and maintain the competing original branch, so their input is central to a fair landing-path decision.
  • Options:
    • Use original branch (recommended): Make Fix Codex fork parent cache invalidation #2118 canonical to preserve the original implementation path, while carrying this PR’s independent frozen-corpus proof into the final review record.
    • Use this branch: Make this PR canonical because its prior review finding is resolved and it provides a larger independent frozen-corpus comparison with explicit implementation attribution.
    • Consolidate first: Ask the contributors to agree on one branch and a combined attribution and proof record before either PR lands.

Security
Cleared: The diff is limited to local Codable cache metadata, scanner logic, a generated parser hash, documentation comments, and tests, with no new dependency, secret, permission, download, or supply-chain surface.

Review details

Best possible solution:

Land one canonical leaf-identity and parent-cache-dependency implementation with preserved authorship and both proof records, then separately evaluate the non-overlapping parent-resolution behavior from #2113 against the remaining overcount corpus.

Do we have a high-confidence way to reproduce the issue?

Yes. The PR supplies a controlled current-main versus branch scan over the same frozen 1,692-file Codex corpus, reproducing Unknown-model attribution and fork overcount before the fix and eliminating them afterward.

Is this the best way to solve the issue?

Yes for the demonstrated identity and warm-cache dependency bug: preferring the leaf ID and tracking the parent-file fingerprint is narrow and regression-tested. The unresolved question is which duplicate branch should land, not whether this implementation approach is appropriate.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c852c135e10c.

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: This is a bounded, normal-priority correctness fix for materially inflated Codex usage attribution.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The isolated 1,692-file frozen-corpus A/B scan convincingly shows the after-fix scanner result on identical real data, supplemented by focused tests and lint without exposing sensitive session details.
  • proof: sufficient: Contributor real behavior proof is sufficient. The isolated 1,692-file frozen-corpus A/B scan convincingly shows the after-fix scanner result on identical real data, supplemented by focused tests and lint without exposing sensitive session details.
Evidence reviewed

What I checked:

Likely related people:

  • xx205: Authored the substantive fork identity and cache invalidation implementation cherry-picked here and maintains the parallel original implementation in Fix Codex fork parent cache invalidation #2118. (role: introduced proposed behavior; confidence: high; commits: 6780ca2391cf, 8152bcae8f6b; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CacheHelpers.swift, Tests/CodexBarTests/CostUsageScannerBreakdownTests.swift)
  • hhh2210: Prepared this independently proven branch, documented the Codex cache-versioning invariant, and resolved the prior cache fast-path review concern. (role: integration contributor; confidence: high; commits: a55b34acf16d, 0571f21dc3d3; files: Sources/CodexBarCore/Generated/CodexParserHash.generated.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageCache.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner+CacheHelpers.swift)
  • iam-brain: Authored the narrower parent-resolution work in Partially fix inherited token accounting across Codex session forks #2113 and reported the broader current fork-overcount behavior this patch partially addresses. (role: adjacent fork-accounting contributor; confidence: medium; commits: 845c8b61636c; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift, Tests/CodexBarTests/CostUsageScannerBreakdownTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-13T09:24:06.670Z sha a55b34a :: needs changes before merge. :: [P3] Run cheap rescan gates before indexing the parent

@hhh2210

hhh2210 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the cache fast-path ordering finding in 0571f21. Parent indexing now runs only after the cheap priority, identity, and empty-row rejection gates.

Validation:

  • 89 focused scanner/cache tests passed
  • make check passed; SwiftFormat clean, SwiftLint 0 violations

@clawsweeper

clawsweeper Bot commented Jul 13, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@xx205

xx205 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Provenance clarification: the substantive fork identity and cache invalidation implementation here is a cherry-pick of the original implementation in #2118. #2118 remains active, now includes the cache-gate ordering optimization and its own controlled local proof, and has not been abandoned or agreed to be superseded. The runtime evidence in #2120 is independent and should remain separately attributed. Please leave any close or supersede decision to maintainers.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 13, 2026
@hhh2210

hhh2210 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

The previous claim that #2120 superseded #2118 was incorrect. #2118 was not abandoned or agreed to be superseded, and @xx205 has kept the original implementation active.

I compared the latest trees after #2118 added its cache-gate optimization. There is no remaining behavioral reason to keep two landing candidates. I am closing #2120 as a duplicate, while retaining the branch and its independent 1,692-file proof as an audit trail.

The technical conclusions do not change: leaf-ID precedence and parent-dependent cache invalidation fix the reproduced issue, parser changes should use CodexParserHash rather than codex-v10, and #2037 should remain open for broader accounting work.

@clawsweeper cluster #2120 as duplicate/independent validation under canonical candidate #2118. Keep implementation attribution and the two independent proof corpora separate.

@hhh2210 hhh2210 closed this Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants