fix(bedrock): send inference config (max_tokens, temperature) on Converse

[kimnamu]

Thanks for goose, and for the AWS Bedrock provider in particular — it is a pleasure to use. This is a small, focused fix for a bug I hit while using Bedrock.

Closes #9888

Problem

The Bedrock provider builds its Converse and ConverseStream requests without an inferenceConfig, so the max_tokens and temperature from ModelConfig are silently dropped on every Bedrock call. Bedrock then applies its per-model server defaults, truncating responses to a small default maxTokens.

crates/goose/src/providers/bedrock.rs — both converse(...) and converse_stream(...) set system / model_id / messages (+ thinking fields, tool config) but never call .inference_config(...).

Cause (sibling parity)

The Anthropic provider already sends these fields (crates/goose/src/providers/formats/anthropic.rs): max_tokens always (via ModelConfig::max_output_tokens()) and temperature when model_supports_temperature(...). The Bedrock provider was simply missing the equivalent InferenceConfiguration. This PR fills in that one missing piece rather than introducing new behaviour.

Change

• Add bedrock_inference_config(&ModelConfig) in providers/formats/bedrock.rs that builds InferenceConfiguration:
  • max_tokens is always set via max_output_tokens() (matches the Anthropic provider; prevents silent truncation).
  • temperature is set only when configured and the model supports it. Support is resolved against the Anthropic canonical registry for anthropic.* ids using the same model-name mapping already used by the thinking logic (strip_bedrock_version_suffix + Anthropic-mapped ModelConfig); other models default to allowing it, matching model_supports_temperature.
• Thread the result through ConverseRequestParts and call .inference_config(...) on both the Converse and ConverseStream builders.

Minimal diff: +97 / -6 across 2 files.

Before / After

Item	Before	After
max_tokens sent to Bedrock Converse/ConverseStream	❌ dropped → server default (truncated responses)	✅ sent (max_output_tokens())
temperature sent to Bedrock	❌ dropped	✅ sent when configured and model supports it
temperature for a model that rejects it	n/a (never sent)	✅ omitted (server default kept)
Public API / method signatures	—	✅ unchanged
System prompt / messages / tool config / thinking fields	—	✅ unchanged (only inference_config added)
Streaming behaviour (ConverseStream)	—	✅ unchanged (same field added to both paths)
BEDROCK_DISABLE_STREAMING escape hatch (stream_via_converse)	—	✅ unchanged (reuses converse)

Tests

Added 3 unit tests in providers/formats/bedrock.rs (no AWS credentials needed — they assert on the built InferenceConfiguration).

Red/green proof that the tests actually catch the bug. Simulating the old behaviour (helper returns an empty InferenceConfiguration, i.e. no max_tokens):

---- test_bedrock_inference_config_defaults_max_tokens_without_config ----
assertion `left == right` failed
  left: None
 right: Some(4096)

---- test_bedrock_inference_config_sets_max_tokens_and_temperature ----
assertion `left == right` failed
  left: None
 right: Some(8192)

test result: FAILED. 1 passed; 2 failed

With the fix in place:

running 3 tests
test ...test_bedrock_inference_config_defaults_max_tokens_without_config ... ok
test ...test_bedrock_inference_config_sets_max_tokens_and_temperature ... ok
test ...test_bedrock_inference_config_omits_temperature_for_unsupported_model ... ok
test result: ok. 3 passed; 0 failed

Local gates (feature aws-providers, toolchain 1.92):

cargo test  -p goose --features aws-providers --lib providers::formats::bedrock  # 32 passed
cargo test  -p goose --features aws-providers --lib providers::bedrock           # 18 passed
cargo fmt --check                                                                # clean
cargo clippy -p goose --features aws-providers --all-targets -- -D warnings      # clean

Notes / scope

• This intentionally does not change thinking/reasoning handling, which is sent separately via additional_model_request_fields and is being addressed elsewhere (e.g. fix: pass thinking config to Bedrock Anthropic models #9794, fix(anthropic): use adaptive thinking for Claude 4.6+ served outside the canonical registry #9840). This PR is scoped to the missing inferenceConfig only.

---

This contribution was prepared with the help of an AI agent (Claude Code); I reviewed the change, the reasoning, and the test results myself before submitting.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3249db6d37

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

[chatgpt-codex-connector]

Normalize Bedrock Claude IDs before choosing maxTokens

For Bedrock Claude IDs that are in this provider but not present in the Bedrock canonical catalog, such as us.anthropic.claude-3-7-sonnet-20250219-v1:0, ModelConfig::max_output_tokens() falls back to 4096 unless the user explicitly set GOOSE_MAX_TOKENS. This new line then sends that fallback as maxTokens on every Converse request, so those known Claude models remain capped at 4k despite their Anthropic canonical output limit being much higher; the helper should use the same Bedrock-to-Anthropic normalization used below for temperature/thinking before defaulting the token cap.

Useful? React with 👍 / 👎.

[DOsinga]

this seems worth fixing -- can we get them from the canonical registry?

[kimnamu]

Thanks for the review @DOsinga, and good catch 🙏 — you and the codex bot are right.

The problem: ModelConfig::max_output_tokens() returns 4096 for any model id that isn't in the canonical catalog (which includes cross-region ids like us.anthropic.claude-3-7-sonnet-...) unless the user explicitly set GOOSE_MAX_TOKENS. My original change forwarded that on every Converse request, which would cap those models at 4k.

I considered resolving the real per-model output limit from the canonical registry (the same strip_bedrock_version_suffix + registry mapping I use for temperature/thinking), but ModelInfo doesn't currently carry an output-token limit — only context_limit — so that would be a larger change touching the registry surface.

Instead I went with the minimal, correct fix that matches how temperature is already handled here: send max_tokens only when the user explicitly configured it, and otherwise leave it unset so Bedrock uses its per-model server default. That fixes the original bug (a configured max_tokens was being dropped) without pinning anything to the 4096 fallback. Tests updated accordingly (...omits_max_tokens_without_config, ...sends_explicit_max_tokens). Happy to do the registry-based output-limit approach as a follow-up if you'd prefer that direction.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddc4485563

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

[chatgpt-codex-connector]

Clamp thinking budget before sending maxTokens

When a Bedrock Anthropic model has thinking enabled and an explicit max_tokens is below the selected thinking budget (for example GOOSE_MAX_TOKENS=4096 with thinking_effort=high, whose budget is 16000), this newly sent maxTokens conflicts with the unchanged budget_tokens emitted by bedrock_anthropic_thinking_fields. The Anthropic formatter clamps the thinking budget against max_tokens because thinking tokens count against that cap; the Bedrock path should apply the same clamp before adding maxTokens so these requests do not fail validation or leave no room for an answer.

Useful? React with 👍 / 👎.

[kimnamu]

Pushed 8d5d9ad addressing the second codex P2 (thinking budget clamp).

Now that bedrock_inference_config sends maxTokens when it's explicitly configured, the thinking budget_tokens could exceed it (e.g. GOOSE_MAX_TOKENS=4096 + thinking_effort=high). Since thinking tokens count against the cap, bedrock_anthropic_thinking_fields now mirrors the Anthropic formatter's clamp: budget is capped at max_tokens - MIN_ANSWER_TOKENS, and thinking is dropped entirely when even a minimal budget wouldn't fit. It only clamps when max_tokens is explicitly set (otherwise Bedrock applies its per-model default, so there's nothing to clamp against). MIN_ANSWER_TOKENS is now shared with the Anthropic formatter. Added two regression tests (clamped / dropped), and reverting the clamp makes both fail.

@DOsinga — to follow up on your earlier question about pulling the limit "from the canonical registry": you're right that's the cleaner direction, and I dug into it. ModelConfig::with_canonical_limits() already maps canonical.output → max_tokens, so the registry path exists. The reason Bedrock Claude ids don't benefit today is that strip_version_suffix in canonical/name_builder.rs doesn't strip the Bedrock -v1:0 style suffix (only the date stamp), so ids like us.anthropic.claude-3-7-sonnet-20250219-v1:0 fail the registry lookup and fall back to 4096. That's a separate, slightly higher-blast-radius change to a shared helper (it'd also fix context_limit), so I kept this PR scoped to the reported bug (configured max_tokens being dropped) and the clamp. If you're happy with the direction, I'd like to open a focused follow-up that teaches strip_version_suffix the Bedrock suffix so Bedrock ids resolve their real registry limits automatically — building on your canonical-model selection work in #9568. Happy to adjust either way.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d5d9ad986

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

[chatgpt-codex-connector]

Respect Bedrock registry temperature support

When GOOSE_TEMPERATURE is set for the Bedrock OpenAI models advertised by this provider (openai.gpt-5.4 / openai.gpt-5.5), this fallback returns true and the new inference config forwards temperature. The bundled Bedrock canonical entries for those exact models mark temperature: false, so these reasoning-model requests can now be rejected instead of using the server default; the helper should consult the Bedrock canonical registry for non-Anthropic ids rather than allowing all of them.

Useful? React with 👍 / 👎.

[kimnamu]

Addressed in 350bba9, @DOsinga.

bedrock_model_supports_temperature now keeps the existing Anthropic normalization path, and for non-Anthropic Bedrock ids consults the Bedrock canonical registry via maybe_get_canonical_model(BEDROCK_PROVIDER_NAME, &model_config.model_name).and_then(|model| model.temperature) before falling back to true only for unknown/missing registry data. I also added regression coverage for openai.gpt-5.4 with temperature set, asserting bedrock_inference_config omits it.

[DOsinga]

Thanks for the thorough work here, and for the detailed reply on the canonical-registry question — that explanation makes sense, a focused follow-up for the Bedrock token-limit normalization sounds right.

One codex comment landed just after your last push and looks unaddressed: "Respect Bedrock registry temperature support". For openai.gpt-5.4 / openai.gpt-5.5, bedrock_model_supports_temperature falls into the else => true branch, so a configured GOOSE_TEMPERATURE would still be forwarded even though those models are marked temperature: false in the Bedrock canonical registry. Per our AI code reviews policy, could you either consult the canonical registry for non-Anthropic ids or add a one-line note on why it isn't an issue? Will revisit in a few days.

[DOsinga]

Thanks for the careful, focused work here — and for addressing every codex point with a matching regression test. The final commit (350bba9) honors the Bedrock canonical registry for non-Anthropic temperature support exactly as discussed, with a test covering the openai.gpt-5.4 case. The fix is small, well-scoped, mirrors the existing Anthropic formatter, and the thinking-budget clamp against an explicit max_tokens is a nice touch. Approving.

Minor non-blocking nit for next time: test_bedrock_inference_config_omits_temperature_for_unsupported_model branches on if supported { ... } else { ... }, so it passes regardless of the actual outcome — the registry-backed test already covers the real behavior, so this one isn't adding much.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6948009662

ℹ️ 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]

Omit temperature when Bedrock thinking is enabled

When a Bedrock Anthropic request has thinking enabled (for example Claude Sonnet 4.5 with GOOSE_THINKING_EFFORT=low) and GOOSE_TEMPERATURE is also set, this branch still adds temperature while bedrock_anthropic_thinking_fields() adds the thinking block. AWS documents Bedrock Claude thinking as incompatible with temperature modifications, so this turns requests that previously succeeded by omitting temperature into ValidationExceptions; skip temperature whenever Bedrock thinking fields will be emitted.

Useful? React with 👍 / 👎.