fix(codex): detect structural fork replay boundaries - #1457

This PR was auto-closed. Only contributors approved with lgtm can open PRs. Open an issue first.

Maintainers review auto-closed issues and reopen worthwhile ones. Issues that do not meet the quality bar in CONTRIBUTING.md may not be reopened or receive a reply.

If a maintainer replies lgtmi, your future issues will stay open. If a maintainer replies lgtm, your future issues and PRs will stay open.

See CONTRIBUTING.md.

sequenceDiagram
  participant CodexLog
  participant visit_codex_session_file
  participant CodexReplayBoundary
  participant UsageAccounting
  CodexLog->>visit_codex_session_file: read session metadata and token_count rows
  visit_codex_session_file->>CodexReplayBoundary: check line index and token totals
  CodexReplayBoundary-->>visit_codex_session_file: classify replay token row
  visit_codex_session_file->>UsageAccounting: replace previous totals or emit usage

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

@pullfrog review it
@codesmith review it
@cubic review it
@coderabbitai review it

Got it, I'm reviewing the PR changes now.

@pullfrog review it
@codesmith review it
@cubic review it
@coderabbitai review it

@ryoppippi I have started the AI code review. It will take a few minutes to complete.

@ryoppippi, I’ll review the changes in #1457.

@ryoppippi

@pullfrog review it
@codesmith review it
@cubic review it
@coderabbitai review it

Reviewed the PR: pulled the branch, read the full parser.rs rewrite against the base, traced all four fixtures plus the pre-existing loader.rs replay tests by hand, and ran the suite. Built clean on 1.96.0 (no warnings), and all 48 codex-adapter tests pass, including the 4 new fixtures and the #1218/#1369 regressions.

Verdict: solid and well-tested. The structural approach is a real improvement over the 1-second heuristic, and the modern/legacy × multisecond/fork-local matrix is good coverage. A few things worth confirming before merge, none blocking.

1. The forked_from_id path hard-requires the ancestor session_meta at exactly line index 1. In detect_replay_boundary, when line 0 carries forked_from_id but no thread_spawn, replay_candidate stays false, so detection hinges entirely on the 1 => arm finding an adjacent ancestor (parser.rs:159-172):

let has_adjacent_parent = value.entry_type.as_deref() == Some("session_meta")
    && ancestor_id == parent_id.as_deref();
if !has_adjacent_parent && !explicit_thread_spawn {
    return None; // -> copied history is NOT skipped -> double count
}

If Codex ever emits any record between the leaf session_meta (line 0) and the ancestor session_meta (e.g. a git_info/turn_context line), has_adjacent_parent is false and, absent thread_spawn, the whole prefix gets counted again. This is stricter than the old scan-first-16KB detection. Can you confirm real fork rollouts always place the ancestor session_meta immediately at line 1? If a preceding record is possible, this path needs to scan forward for the ancestor rather than pin it to index 1.

2. LegacyCreationSecond still carries the exact multi-second limitation this PR fixes. The multi-second skip only kicks in via the NativeLine boundary; streams with no native task_started fall back to LegacyCreationSecond (parser.rs:106-108, :190), which only skips events sharing the creation second. That's the documented compatibility case and not a regression, but the PR framing ("skip inherited prefixes even when they span multiple seconds") is only true for streams that emit a native task_started; legacy-without-task_started multi-second replays still under/over-count. Worth stating explicitly.

3. The started_at vs record-clock signal is an assumption about Codex's replay writer. The native-boundary detection (codex_is_native_task_start, parser.rs:219-230) relies on replayed task_started markers retaining their original started_at while the record timestamp is rewritten to fork-creation time, so they diverge for copied turns and coincide for the live turn. It's a clean signal and the fixtures encode it, but it's load-bearing; if a future Codex version rewrites started_at on replay too, the boundary silently collapses to the first turn.

Minor:

• source: Option<Value> (parser.rs:86) breaks the otherwise fully-borrowed struct and allocates a serde_json::Value per session_meta. Only lines 0-1 are parsed this way so the cost is negligible, but given the repo's zero-copy/min-size posture you could reach parent_thread_id with a small borrowed struct instead.
• The line_index counters in detect_replay_boundary and visit_codex_session_file must stay in lockstep for NativeLine(idx) to line up. I verified each path increments exactly once per line read, but it's an implicit cross-function invariant; a one-line comment noting "must count lines identically to visit_codex_session_file" would protect it from future edits.

No code changes made; this is review-only.

@pullfrog read all reviews from them and fix them

I addressed both review findings in commit 4a7d161d on the local pr-1457 branch:

• Added a lockstep invariant comment on CodexReplayBoundary::NativeLine documenting that its physical line index must stay aligned with the counter in visit_codex_session_file.
• Added replay-no-task-started-legacy.jsonl and a test falls_back_to_leaf_creation_second_without_task_started covering the LegacyCreationSecond fallback path, including that events at the leaf creation second are skipped and their cumulative total becomes the baseline.

Validation passed: cargo +1.96.0 test --manifest-path rust/Cargo.toml --workspace (all green).

However, pushing the commit failed because the PR branch lives on the fork MaxGhenis/ccusage, and pullfrog[bot] does not have push access there:

remote: Permission to MaxGhenis/ccusage.git denied to pullfrog[bot].
fatal: unable to access 'https://github.com/MaxGhenis/ccusage.git/': The requested URL returned error: 403

I did not reply to or resolve the review threads because the fixes are not yet live on the remote. If you grant the bot push access to the fork, or if a maintainer can push this commit manually, the review comments are ready to be resolved.

  ^{｜ ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ via Pullfrog ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

Run failed. View the logs →

  ^{｜ ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot ｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏}

Thanks for digging into this. The underlying bug was real, but this PR has been superseded — the one-second heuristic it replaces no longer exists on main.

#1435 and #1438 replaced it with CodexReplayPlan in rust/adapters/codex/src/replay.rs, which reads the parent session log and subtracts the exact replayed prefix instead of guessing a boundary from a single file. #1428 also moved the adapter to rust/adapters/codex/, so the parser.rs code this PR rewrites is no longer there in that form.

I checked your replay-multisecond-modern.jsonl scenario against current main rather than relying on the diff:

So the common case is already fixed. What remains is narrower than this PR assumes: when the referenced parent log is unavailable, replay_prefix returns an empty slice and the parser falls back to the old rewritten-second heuristic, which still misses an inherited prefix spanning multiple seconds. That is a genuine gap and worth closing.

It is better closed inside the replay.rs fallback than by reinstating a single-file boundary scan, though. This PR's boundary signal — task_started.started_at matching the record second — is itself a heuristic, and a weaker one than reading the parent log, so layering it back over the current design would trade a verified mechanism for an inferred one.

Closing this as superseded. A small PR scoped to just that fallback path would be very welcome, and I would review it happily.

One process note for next time: the verification links point at docs/reviews/ in your own repository, where the fixtures and the expected values are both authored by the same pass. That is not independent confirmation, and it is what let the multisecond claim stand against a tree that had already moved on. For #1458 I was able to confirm the invariant against 21,815 real token_count records, which is the kind of evidence that settles it.

Following up: I closed the fallback gap myself in #1501, so you are not left holding it.

The measurements that shaped it, in case they are useful for your accounting work:

• The burst Codex rewrites to the fork instant spans 10 to 40ms across the 9 fork sessions with one in a real log directory, while the child's own first turn follows a pause of 5.8 to 15.3 seconds. Following the run rather than the recorded second separates those two populations by two orders of magnitude, and it is why the multi-second case you found was reachable at all.
• I did not adopt the task_started.started_at signal. Of 1,833 real task_started records in fork sessions, 135 have started_at differing from their record second, and some of those differences are exactly 1 second on native records that merely crossed a tick — so strict equality misclassifies them. That is the kind of thing only real logs surface.
• Before the change, 1 of 210 real fork sessions over-counted by 27,354,644 tokens (2.4x) through the fallback. After it, all 210 agree with exact prefix subtraction.

Thanks for pointing at the right place even though the patch had moved on.

Historical audit: this pull request was auto-closed by the legacy contributor gate. That closure did not assess technical importance.

Audit result: needs review. The current state does not prove resolution, but a fresh technical or product check is required before deciding whether the underlying request is still relevant. This closed PR will not be revived as-is; create a new PR only after reviewing the related issue and current main.