Act on reasoning-included per turn #9402
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: a44959ad27
ℹ️ 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".
| pub(crate) fn set_server_reasoning_included(&mut self, included: bool) { | ||
| self.server_reasoning_included |= included; | ||
| } |
There was a problem hiding this comment.
The new set_server_reasoning_included uses |= so once any response sets the flag true it can never return to false. Because the header is only emitted when present, a later response that does not include X-Reasoning-Included (e.g., mixed backend versions during a rollout or different servers per SSE request) will still be treated as “included,” causing get_total_token_usage to skip estimating past reasoning tokens and undercount usage. That can suppress auto‑compaction decisions. Consider setting the flag to the current response’s value (not OR‑ing), or explicitly clearing it when the header is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Reset the session reasoning-included flag to reflect the latest response header rather than remaining sticky across responses. codex-rs/core/src/state/session.rsL88-L93
Testing
- ✅
just fix -p codex-core - ❌
cargo test -p codex-core(failed: models_manager::manager::tests::{refresh_available_models_*}, shell_snapshot::tests::linux_sh_snapshot_includes_sections, exec::tests::kill_child_process_group_kills_grandchildren_on_timeout)
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de6149c4bc
ℹ️ 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".
aibrahim-oai
changed the title
aibrahim-oai
changed the title
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51a67b00f8
ℹ️ 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".
| tokio::spawn(async move { | ||
| if server_reasoning_included { | ||
| let _ = tx_event | ||
| .send(Ok(ResponseEvent::ServerReasoningIncluded(true))) | ||
| .await; |
There was a problem hiding this comment.
ServerReasoningIncluded is described as being emitted when the header is present on that response, but here the websocket path captures the header once at connect time and reuses it for every stream_request. Because the same websocket connection is reused across turns, the flag can never clear on later turns if the server omits X-Reasoning-Included for a subsequent response, so server_reasoning_included stays true and get_total_token_usage will undercount (skipping the extra reasoning tokens) until the connection is torn down. Consider re-checking per request (or reconnecting per turn) so the flag can clear when the header is absent.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.