Unify rollout reconstruction with resume/fork TurnContext hydration#12612
Unify rollout reconstruction with resume/fork TurnContext hydration#12612charley-oai merged 64 commits intomainfrom
Conversation
|
@codex review this |
![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: 859ea16a31
ℹ️ 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".
|
Security review completed. No security issues were found in this pull request. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d4efe495b
ℹ️ 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".
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ab8c7d01
ℹ️ 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".
|
@codex review this |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9506d61000
ℹ️ 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".
|
@codex review this |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
codex-rs/core/src/codex.rs
Outdated
| } | ||
| RolloutItem::EventMsg(EventMsg::ThreadRolledBack(rollback)) => { | ||
| history.drop_last_n_user_turns(rollback.num_turns); | ||
| let mut turns_to_drop = |
There was a problem hiding this comment.
a bit strange to run two version of this logic in parallel, one to drop user messages, another to drop turns
There was a problem hiding this comment.
I think we can't unify them because history only contains model-visible data whereas our stack of user turns contains metadata needed to fetch previous_model and reference_context_item
| let push_replayed_turn = |replayed_segments: &mut Vec<RolloutReplayMetaSegment>, | ||
| active_turn: ActiveRolloutTurn| { | ||
| replayed_segments.push(RolloutReplayMetaSegment::Turn(Box::new( | ||
| ReplayedRolloutTurn { |
There was a problem hiding this comment.
we are copying the struct into almost the same struct ActiveRolloutTurn -- >ReplayedRolloutTurn.
There was a problem hiding this comment.
The conversion serves to eliminate dead/unused fields
- dropping turn_id, which is only needed while matching lifecycle events
- dropping saw_user_message, which is only needed to decide whether the span counts as a user turn at all
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acec761d88
ℹ️ 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".
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06f8e6b316
ℹ️ 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".
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37cacb2dab
ℹ️ 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".
|
@codex review this |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
charley-oai
force-pushed
the
cc/unify-rollout-reconstruction-resume-hydration
branch
2 times, most recently
from
288f66a to
44b3006
Compare
|
@codex review this |
charley-oai
force-pushed
the
cc/unify-rollout-reconstruction-resume-hydration
branch
2 times, most recently
from
1198055 to
ff0efdb
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff0efdbbee
ℹ️ 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".
| // Materialize exact history semantics from the replay-derived suffix. The eventual lazy | ||
| // design should keep this same replay shape, but drive it from a resumable reverse source | ||
| // instead of an eagerly loaded `&[RolloutItem]`. | ||
| for item in rollout_suffix { |
There was a problem hiding this comment.
Considering we have a separate loop anyway (ideally we won't), can we drop rollout_suffix and add break to RolloutItem::Compacted branch?
There was a problem hiding this comment.
Ah, is it because the history is append to the end only?
There was a problem hiding this comment.
I think it's hard to do the second as part of the first because of None replacement_history compacts which want the whole history so far (when going left to right)
|
|
||
| let initial_context = self.build_initial_context(turn_context, None).await; | ||
| let mut history = ContextManager::new(); | ||
| if let Some(base_replacement_history) = base_replacement_history { |
There was a problem hiding this comment.
Why do we have base_replacement_history? Wouldn't the loop below replace what's needed?
There was a problem hiding this comment.
That's needed to keep track of whether we can stop consuming backwards, and then at the beginning of the forward pass we use it to start off the history we build
| // Thread rollback always targets the newest surviving user turns, so consume that | ||
| // skip budget before letting this segment contribute metadata or a compaction base. | ||
| if *pending_rollback_turns > 0 { | ||
| if active_segment.counts_as_user_turn { |
There was a problem hiding this comment.
the drop_last_n_user_turns is base on user messages, this logic is based on turns. Do they align?
There was a problem hiding this comment.
I think this logic is based on user messages too, counts_as_user_turn is carefully designed to align with drop_last_n_user_turns logic (and I added a bunch of tests to ensure they align)
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
Is rollback behavior aligned?
Can we simplify by removing support for non-replacement_history compaction?
If we are keeping two loops, can we make them independent and bail compact item when loading ResponseItems - that will make me sleep a bit better at night.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25e8ea4acc
ℹ️ 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".
charley-oai
deleted the
cc/unify-rollout-reconstruction-resume-hydration
branch
2 participants
Summary
This PR unifies rollout history reconstruction and resume/fork metadata hydration under a single
Session::reconstruct_history_from_rolloutimplementation.The key change from main is that replay metadata now comes from the same reconstruction pass that rebuilds model-visible history, instead of doing a second bespoke rollout scan to recover
previous_model/reference_context_item.What Changed
Unified reconstruction output
reconstruct_history_from_rolloutnow returns a singleRolloutReconstructionbundle containing:historyprevious_modelreference_context_itemResume and fork both consume that shared output directly.
Reverse replay core
The reconstruction logic moved into
codex-rs/core/src/codex/rollout_reconstruction.rsand now scans rollout items newest-to-oldest.That reverse pass:
previous_modelreference_context_itemis preserved or clearedreplacement_historycheckpointHistory materialization is still bridged eagerly for now by replaying only the surviving suffix forward, which keeps the history result stable while moving the control flow toward the future lazy reverse loader design.
Removed bespoke context lookup
This deletes
last_rollout_regular_turn_context_lookupand its separate compaction-aware scan.The previous model / baseline metadata is now computed from the same replay state that rebuilds history, so resume/fork cannot drift from the reconstructed transcript view.
TurnContextItempersistence contractTurnContextItemis now treated as the replay source of truth for durable model-visible baselines.This PR keeps the following contract explicit:
TurnContextItemfor the first real user turn so resume can recoverprevious_modelTurnContextItemafterCompactedso resume/fork can re-establish the baseline from the rewritten historyBehavior Preserved
drop_last_n_user_turnsreference_context_itemuntil a laterTurnContextItemre-establishes itprevious_modelstill comes from the newest surviving user turn that established oneTests
Targeted validation run for the current branch shape:
cd codex-rs && cargo test -p codex-core --lib codex::rollout_reconstruction_tests -- --nocapturecd codex-rs && just fmtThe branch also extracts the rollout reconstruction tests into
codex-rs/core/src/codex/rollout_reconstruction_tests.rsso this logic has a dedicated home instead of living inline incodex.rs.