Account for last token count on resume #8677
Conversation
![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: be68e48b9b
ℹ️ 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".
| fn last_token_info_from_rollout(rollout_items: &[RolloutItem]) -> Option<TokenUsageInfo> { | ||
| rollout_items.iter().rev().find_map(|item| match item { | ||
| RolloutItem::EventMsg(EventMsg::TokenCount(ev)) => ev.info.clone(), | ||
| _ => None, |
There was a problem hiding this comment.
This helper blindly picks the last EventMsg::TokenCount in the rollout, but review sub-agent events are forwarded into the parent session (see process_review_events in core/src/tasks/review.rs, which passes through all “other” events) and are persisted to the rollout. If the last TokenCount came from a /review run, this will seed token_info with the reviewer model’s usage instead of the main conversation’s context, so the resumed session’s auto-compaction decisions and UI token indicators can be wrong. Consider scoping to the main session’s turn context or excluding sub-agent sources when choosing the last token info.
Useful? React with 👍 / 👎.
|
@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". |
| resumed.submit_turn(follow_up_user).await.unwrap(); | ||
|
|
||
| let compact_requests = compact_mock.requests(); | ||
| assert_eq!( |
There was a problem hiding this comment.
do you want to assert the event as well?
last token count in context manager is initialized to 0. Gets populated only on events from server.
This PR populates it on resume so we can decide if we need to compact or not.