fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability)

[kyledef]

Problem

When a thinking model's config changes mid-conversation, the request history still carries the original signed thinking / redacted_thinking blocks. Anthropic (including via Databricks/Bedrock) rejects them with:

400 BAD_REQUEST — messages.N.content.M: `thinking` or `redacted_thinking` blocks in the
latest assistant message cannot be modified. These blocks must remain as they were in the
original response.

should_retry returned !config.transient_only for all RequestFailed errors, and RetryConfig::default() has transient_only = false. So this identical, never-recoverable payload was retried 3× (with 1s/1.6s/3.4s backoff) before the agent turn failed anyway — wasting ~6s and obscuring the real cause. Observed in the wild ending an agent session unexpectedly.

Fix

Detect this class of deterministically-permanent 4xx by message marker and never retry it, regardless of transient_only. The broad RequestFailed retry behavior is intentionally unchanged (the existing default_config_retries_request_failed contract still holds), so this is a narrow, non-regressing fix.

ProviderError::RequestFailed(message) if is_permanent_request_failure(message) => false,
ProviderError::RequestFailed(_) => !config.transient_only,

Tests

• never_retries_permanent_thinking_block_400 — the exact 400 payload is not retried even under default (retrying) config.
• permanent_request_failure_marker_detection — marker matching, and that ordinary 400s (e.g. "model not found") are unaffected.
• All existing retry tests still pass.

Notes / follow-up

This is the safe half of a two-part issue. The deeper root cause — stale signed thinking blocks being sent at all after a mid-conversation config change — lives in crates/goose/src/providers/formats/anthropic.rs (message_to_anthropic) and is tracked separately. This PR stops the wasteful retries and lets the failure surface immediately.

Internal tracking: BOT-1019.

[DOsinga]

Pushed a small commit trimming a few redundant comments (the per-marker doc and the inline comment in should_retry were restating the constant's doc; the test had a comment restating its own name). Kept the one comment that earns its place — the thinking/redacted_thinking immutability explanation on the constant.

On the substring matching: that's the right call here. By the time should_retry sees a RequestFailed, it's just a String — the status code and structured payload are already gone. And this matches the existing convention in http_status.rs (is_context_length_exceeded_message), which classifies 400s by message content the same way. Also merged main in. Approving.

[DOsinga]

Tidy, well-tested narrow fix. Comments trimmed, merged main, tests + clippy green. LGTM.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c194e76cd

ℹ️ 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".

[chatgpt-codex-connector]

Handle Bedrock validation errors too

Restricting the permanent-marker bypass to RequestFailed misses the Bedrock streaming path I checked: ConverseStreamError::ValidationException values that are not context-length errors fall through to ProviderError::ServerError in crates/goose/src/providers/bedrock.rs:481-496, and should_retry returns true for all ServerErrors before reaching this branch. When Bedrock returns the same thinking-block immutability validation message during request setup, it will still be retried with the identical payload, leaving that documented scenario unfixed.

Useful? React with 👍 / 👎.