fix(providers): drop stale signed thinking blocks after a mid-conversation model switch - #10007
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: 67f46226ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -641,7 +654,7 @@ pub fn create_request_for_provider( | |||
| tool_call_id: None, | |||
| }; | |||
|
|
|||
| let messages_spec = format_messages(messages, image_format); | |||
| let messages_spec = format_messages(messages, image_format, Some(&model_config.model_name)); | |||
There was a problem hiding this comment.
Passing model_config.model_name here makes the new stale check compare the current endpoint name with the prior message's resolved_model; for Databricks, model_info_from_endpoint stores upstream_model_name in that field. For any Claude serving endpoint whose endpoint name differs from its upstream model, a same-endpoint follow-up is treated as stale and valid reasoning blocks are stripped even though no model switch occurred. Please compare against the current resolved upstream model, or fall back to the requested endpoint only when no resolved model is known.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the check now compares against requested_model (the endpoint-name identity space callers pass as current_model) rather than resolved_model, so a same-endpoint Databricks follow-up is no longer flagged stale despite its upstream name differing. Added regression test keeps_reasoning_when_endpoint_matches_despite_upstream_resolved_name to lock this in.
| let Some(inference) = message.metadata.inference.as_ref() else { | ||
| return false; |
There was a problem hiding this comment.
The new check returns false whenever metadata.inference is absent, but direct Anthropic assistant messages do not get this metadata today: Agent::reply_internal only builds InferenceMetadata after fetch_model_info() returns a resolved_model, and the default Anthropic model info leaves that field unset. In a direct Anthropic extended-thinking session, switching models still replays the old signed thinking block and can hit the same 400, so the formatter needs requested-model provenance recorded or another source before treating it as unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: Agent::reply_internal no longer gates InferenceMetadata on resolved_model being present — it now always records at least requested_model, so direct Anthropic extended-thinking sessions carry the provenance this formatter needs. The "no inference metadata" early-return now only affects genuinely pre-existing/legacy messages.
|
Thanks @codex — both P2 findings were valid and are now addressed in
141 |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8313966e72
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let origin_model = inference.requested_model.as_str(); | ||
| !origin_model.is_empty() && origin_model != current_model |
There was a problem hiding this comment.
When a Databricks serving endpoint keeps the same endpoint name but is repointed to a different upstream Claude model, this compares only the prior requested_model with the current endpoint name, so the old signed reasoning is treated as fresh and replayed to a different upstream model; those signatures are model-specific and can still trigger the 400 this change is trying to avoid. Fresh evidence in this revision is that Agent::reply_internal now records resolved_model, but this helper ignores it instead of comparing in resolved-model space when both sides are known and falling back to requested names only when they are not.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the direct counterpoint to the earlier P2 on line 657, and the two can't both be satisfied with one comparison: comparing in resolved-model space fixes the rare repointed-endpoint case but reintroduces false-positive stripping for every Databricks endpoint whose name differs from its upstream, which is the common case. I opted for the endpoint-name (requested_model) comparison as the safe default and rely on the permanent-400 retry guard in #10005 as the backstop for the repointed-endpoint edge case; happy to switch to resolved-space if you'd prefer that tradeoff.
8313966 to
766979b
Compare
|
Rebased onto latest |
|
Thanks @kyledef — the fix itself looks right and the codex threads are well handled. Two cleanups before we land it: Comments — the model-signature/400 rationale is currently explained in several places: the doc comment on
Tests — |
Root cause of the 400 "thinking/redacted_thinking blocks in the latest assistant message cannot be modified" that ends extended-thinking sessions. A thinking block's signature is issued by — and only valid for — the model that produced it. When a conversation switches models or thinking-effort mid-stream (e.g. via set_config_option), the stored history still carries the previous model's signed `thinking` / `redacted_thinking` (Databricks `reasoning`) blocks. Replaying those signatures against a different model makes Anthropic (direct and via Databricks/Bedrock) reject the whole request with a 400, which is unrecoverable and kills the agent turn. Fix: when serializing history, drop signed thinking/redacted blocks whose originating model (from `message.metadata.inference`) differs from the model this request targets. The turn's text and tool content are still sent. When provenance is unknown (older stored messages, or no inference metadata) the prior behavior is kept, so single-model conversations are unaffected. - anthropic formatter: `AnthropicFormatOptions.current_model` + shared `thinking_block_is_stale()`; skip stale signed `thinking` and all `redacted_thinking`. - databricks formatter (the path in the observed incident): same staleness check applied to its `reasoning` summary blocks, threading the target model into `format_messages`. Tests: stale-from-other-model is dropped (text preserved); same-model is kept; unknown-provenance is kept — for both formatters. Pairs with the retry-side fix (PR aaif-goose#10005). Refs BOT-1019.
…king detection - Compare current_model against both requested_model and resolved_model so a Databricks Claude-backed endpoint that rewrites the model name upstream doesn't get mistaken for a genuine model switch (Codex P2s). - Always record InferenceMetadata even when resolved_model can't be fetched, so direct Anthropic sessions carry provenance too (Codex P2). - Consolidate the model-signature/400 rationale into the thinking_block_is_stale doc comment; drop the comments elsewhere that just restated it. - Split the combined databricks same-model/different-model test into two named tests, and assert the exact bare-string content shape instead of hedging between String/Array. Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
766979b to
b1355a6
Compare
|
Rebased onto the current Addressed both review-comment cleanups from @DOsinga in
|
|
Ran an automated review pass (
Not fixing it in this PR: I haven't confirmed live whether Anthropic's signature check is actually keyed to effort/budget_tokens vs. purely model identity, this PR already resolves the reported incident, and #10005's retry backstop still fails fast on any residual 400 of this shape rather than retrying 3x. Suggest a fast follow-up PR that records the originating |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a841ed000e
ℹ️ 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 fn thinking_block_is_stale(message: &Message, current_model: Option<&str>) -> bool { |
There was a problem hiding this comment.
With aws_bedrock Claude sessions, the provenance now recorded by the agent is never consulted when serializing history: BedrockProvider::prepare_request still sends visible messages through to_bedrock_message_with_caching, and to_bedrock_message_content unconditionally turns Thinking/RedactedThinking into Bedrock ReasoningContent. After a Bedrock session switches Claude models, signed reasoning from the prior model is still replayed and can hit the same validation 400 this change is meant to avoid; please thread this staleness check/current model into the Bedrock formatter too.
Useful? React with 👍 / 👎.
# Conflicts: # crates/goose-provider-types/src/formats/anthropic.rs # crates/goose-provider-types/src/formats/databricks.rs # crates/goose-providers/src/anthropic.rs # crates/goose/src/agents/agent.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5b9e9d355
ℹ️ 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".
| .filter(|content| { | ||
| if !thinking_is_stale { | ||
| return true; | ||
| } | ||
| match content { | ||
| MessageContent::Thinking(thinking) => thinking.signature.is_empty(), | ||
| MessageContent::RedactedThinking(_) => false, |
There was a problem hiding this comment.
When a stale assistant turn contains only signed Thinking or RedactedThinking—for example, a response that exhausted its output budget during reasoning—this filter removes every content block, but the function still builds the message with content: [] and prepare_request includes it in the Converse history. The agent's empty-turn guard cannot prevent this because the original reasoning counted as provider-produced content, and Bedrock rejects empty conversation turns, so the model-switch follow-up still fails; omit or normalize emptied turns and their neighboring roles before sending the history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87469c2df9
ℹ️ 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".
| if requested.is_empty() && resolved.is_empty() { | ||
| return false; | ||
| } | ||
| current_model != requested && current_model != resolved |
There was a problem hiding this comment.
When a session switches between two Anthropic-compatible providers that expose the same requested model string, this returns false because it compares only model names and ignores inference.provider. If those providers route that name to different deployments and fetch_model_info did not resolve the underlying model, the old provider's signed thinking is replayed to the new deployment and can still trigger the validation error this change is intended to prevent; pass the current provider identity into this check and require it to match before retaining signed content.
Useful? React with 👍 / 👎.
# Conflicts: # crates/goose-provider-types/src/formats/databricks.rs # crates/goose/src/agents/agent.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff27b4f9e
ℹ️ 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::test] | ||
| async fn requested_model_is_recorded_without_resolved_model() -> Result<()> { |
There was a problem hiding this comment.
This regression test exercises only the state-machine pipeline, while the equivalent change in Agent::reply_internal remains untested. Because stale-thinking filtering now depends on every direct-provider assistant turn recording requested_model even when resolved_model is absent, the default legacy path could diverge without detection; add an equivalent legacy-loop test asserting the persisted inference metadata. The repository explicitly requires agent-loop behavior changes to be implemented and tested in both paths.
AGENTS.md reference: AGENTS.md:L19-L23
Useful? React with 👍 / 👎.
* origin/main: fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
* origin/main: fix(mcp): prune dead notification subscribers (#11032) chore: remove the extension and tool count suggestion (#10869) feat: compaction in the GDK (#11042) fix(provider): retry transient errors on first stream item before ending turn (#10968) feat(cli): add /new to start a fresh session without restarting (#10767) feat(acp): title new sessions from _meta.sessionTitle (#10712) fix: adjust rmcp::model::Meta ref (#11107) Skip hook loading and lifecycle events for subagents (#10596) Sanitize Unicode tags in Responses output (#10745) fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
* main: fix(mcp): prune dead notification subscribers (#11032) chore: remove the extension and tool count suggestion (#10869) feat: compaction in the GDK (#11042) fix(provider): retry transient errors on first stream item before ending turn (#10968) feat(cli): add /new to start a fresh session without restarting (#10767) feat(acp): title new sessions from _meta.sessionTitle (#10712) fix: adjust rmcp::model::Meta ref (#11107) Skip hook loading and lifecycle events for subagents (#10596) Sanitize Unicode tags in Responses output (#10745) fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
4 participants
Problem (root cause)
Extended-thinking agent sessions die with an unrecoverable provider 400:
A thinking block's signature is issued by — and only valid for — the model that produced it. When a conversation switches models or thinking-effort mid-stream (e.g. via
set_config_option), the stored history still carries the previous model's signedthinking/redacted_thinkingblocks (Databricks calls themreasoningblocks). Replaying those signatures against a different model makes Anthropic — direct and via Databricks/Bedrock — reject the whole request. The agent turn then ends unexpectedly.This was observed in the wild: two
set_config_optioncalls, then an immediate 400 on the next request (at message index 3, so not a compaction artifact).Fix
When serializing history, drop signed thinking / redacted-thinking blocks whose originating model (from
message.metadata.inference) differs from the model the current request targets. The turn's text and tool content are still sent.When provenance is unknown — older stored messages, or no inference metadata — the prior behavior is kept, so single-model conversations are completely unaffected (no regression).
Applied in both serialization paths:
formats/anthropic.rs): newAnthropicFormatOptions.current_model+ sharedthinking_block_is_stale(); skips stale signedthinkingand all staleredacted_thinking.formats/databricks.rs) — the path in the observed incident: same staleness check applied to itsreasoningsummary blocks, threading the target model intoformat_messages.Tests
For both formatters:
All 140
providers::formatstests pass;cargo fmtand clippy clean (no new warnings in touched files).Related
Pairs with the retry-side fix (#10005), which stops the identical 400 payload from being retried 3×. This PR removes the cause; that PR makes any remaining permanent 400 fail fast instead of after wasteful retries.
Internal tracking: BOT-1019.