Conversation
| && token_limit_reached | ||
| && needs_follow_up | ||
| { | ||
| if turn_context.config.features.enabled(Feature::TokenBudget) |
There was a problem hiding this comment.
we should probably drop this
| && needs_follow_up | ||
| { | ||
| && needs_follow_up; | ||
| if needs_follow_up && (new_context_window_requested || auto_compact_needed) { |
There was a problem hiding this comment.
more complicated than needed
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a00ab1bb6e
ℹ️ 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".
| crate::compact_token_budget::run_inline_auto_compact_task( | ||
| Arc::clone(sess), | ||
| Arc::clone(turn_context), | ||
| initial_context_injection, | ||
| ) |
There was a problem hiding this comment.
When pre-turn compaction is triggered by a comp-hash change or model downshift, maybe_run_previous_model_inline_compact passes a StepContext built from the previous model. This token-budget shortcut then starts the new window with that previous TurnContext and persists it as the reference context, so the actual current turn sees a non-None reference and only appends diffs instead of rebuilding full initial context for the new model. In TokenBudget sessions that switch/downshift models, the first request after the reset can therefore keep stale full-context metadata from the old model (for example context/skill budgets or other initial-context-only sections).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
true, we should take as a followup.
| sess.start_new_context_window(turn_context.as_ref(), world_state) | ||
| .await; |
There was a problem hiding this comment.
When this path is used by standalone /compact, it writes a replacement history that already contains full initial context, but rollout reconstruction only adopts TurnContext baselines from real user-turn segments. After resuming a thread that ended with token-budget /compact (especially before any user turn), the replacement history is restored while reference_context_item remains missing or stale, so the next turn reinjects full initial context on top of the compacted window and duplicates token-budget/world-state context.
AGENTS.md reference: AGENTS.md:L103-L111
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
true, we should take as a followup.
Why
When
Feature::TokenBudgetis enabled, compaction should behave likenew_context: start a fresh context window with the standard injected context, without asking the server to summarize old history and without carrying prior user or assistant messages into the next model request.This is still a compaction operation from the client lifecycle perspective. Manual
/compactand auto-compaction should keep the same observable side effects that clients and hooks expect, including compact hooks andTurnItem::ContextCompaction.What changed
compact_token_budgetto run token-budget manual and inline auto-compaction through a shared compaction lifecycle.new_contextrequests from forced context-window startup:take_new_context_window_request()consumes pending requests, andstart_new_context_window()installs a fresh context window./compactand inline auto-compaction to install a fresh context window locally instead of calling server/local summarization.ContextCompactionitem start/completion events.Testing
just test -p codex-core token_budget_context_uses_new_window_after_compactionjust test -p codex-core token_budget_compaction_runs_compact_hooksjust test -p codex-core token_budget_mid_turn_auto_compaction_resets_before_active_follow_up