-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Account for last token count on resume #8677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
be68e48
account for last token count on resume
aibrahim-oai b73bfb7
account for last token count on resume
aibrahim-oai 16aebc4
integration
aibrahim-oai 064b423
tests
aibrahim-oai f1e423e
event
aibrahim-oai 2fd86d9
Merge branch 'main' into resume-token-count
aibrahim-oai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1228,6 +1228,93 @@ async fn auto_compact_runs_after_token_limit_hit() { | |
| ); | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn auto_compact_runs_after_resume_when_token_usage_is_over_limit() { | ||
| skip_if_no_network!(); | ||
|
|
||
| let server = start_mock_server().await; | ||
|
|
||
| let limit = 200_000; | ||
| let over_limit_tokens = 250_000; | ||
| let remote_summary = "REMOTE_COMPACT_SUMMARY"; | ||
|
|
||
| let compacted_history = vec![ | ||
| codex_protocol::models::ResponseItem::Message { | ||
| id: None, | ||
| role: "assistant".to_string(), | ||
| content: vec![codex_protocol::models::ContentItem::OutputText { | ||
| text: remote_summary.to_string(), | ||
| }], | ||
| }, | ||
| codex_protocol::models::ResponseItem::Compaction { | ||
| encrypted_content: "ENCRYPTED_COMPACTION_SUMMARY".to_string(), | ||
| }, | ||
| ]; | ||
| let compact_mock = | ||
| mount_compact_json_once(&server, serde_json::json!({ "output": compacted_history })).await; | ||
|
|
||
| let mut builder = test_codex().with_config(move |config| { | ||
| set_test_compact_prompt(config); | ||
| config.model_auto_compact_token_limit = Some(limit); | ||
| config.features.enable(Feature::RemoteCompaction); | ||
| }); | ||
| let initial = builder.build(&server).await.unwrap(); | ||
| let home = initial.home.clone(); | ||
| let rollout_path = initial.session_configured.rollout_path.clone(); | ||
|
|
||
| // A single over-limit completion should not auto-compact until the next user message. | ||
| mount_sse_once( | ||
| &server, | ||
| sse(vec![ | ||
| ev_assistant_message("m1", FIRST_REPLY), | ||
| ev_completed_with_tokens("r1", over_limit_tokens), | ||
| ]), | ||
| ) | ||
| .await; | ||
| initial.submit_turn("OVER_LIMIT_TURN").await.unwrap(); | ||
|
|
||
| assert!( | ||
| compact_mock.requests().is_empty(), | ||
| "remote compaction should not run before the next user message" | ||
| ); | ||
|
|
||
| let mut resume_builder = test_codex().with_config(move |config| { | ||
| set_test_compact_prompt(config); | ||
| config.model_auto_compact_token_limit = Some(limit); | ||
| config.features.enable(Feature::RemoteCompaction); | ||
| }); | ||
| let resumed = resume_builder | ||
| .resume(&server, home, rollout_path) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let follow_up_user = "AFTER_RESUME_USER"; | ||
| let sse_follow_up = sse(vec![ | ||
| ev_assistant_message("m2", FINAL_REPLY), | ||
| ev_completed("r2"), | ||
| ]); | ||
|
|
||
| let follow_up_matcher = move |req: &wiremock::Request| { | ||
| let body = std::str::from_utf8(&req.body).unwrap_or(""); | ||
| body.contains(follow_up_user) && body.contains(remote_summary) | ||
| }; | ||
| mount_sse_once_match(&server, follow_up_matcher, sse_follow_up).await; | ||
|
|
||
| resumed.submit_turn(follow_up_user).await.unwrap(); | ||
|
|
||
| let compact_requests = compact_mock.requests(); | ||
| assert_eq!( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want to assert the event as well? |
||
| compact_requests.len(), | ||
| 1, | ||
| "remote compaction should run once after resume" | ||
| ); | ||
| assert_eq!( | ||
| compact_requests[0].path(), | ||
| "/v1/responses/compact", | ||
| "remote compaction should hit the compact endpoint" | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn auto_compact_persists_rollout_entries() { | ||
| skip_if_no_network!(); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper blindly picks the last
EventMsg::TokenCountin the rollout, but review sub-agent events are forwarded into the parent session (seeprocess_review_eventsincore/src/tasks/review.rs, which passes through all “other” events) and are persisted to the rollout. If the last TokenCount came from a/reviewrun, this will seedtoken_infowith 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 👍 / 👎.