fix: remove topP parameter from Bedrock inference config #8388
Conversation
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
dosubot
bot
added
size:S
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found some issues that need attention:
- Removing topP from Bedrock inference config is correct for Bedrock models that reject temperature + top_p together.
- Please ensure tests and any call sites that asserted/relied on topP are updated; see inline comments.
- Note: GitHub shows merge conflicts; a rebase may be required before merge.
| @@ -44,7 +44,6 @@ import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from ". | |||
| interface BedrockInferenceConfig { | |||
| maxTokens: number | |||
| temperature?: number | |||
| topP?: number | |||
| } | |||
Contributor
There was a problem hiding this comment.
[P1] Contract change removes topP from BedrockInferenceConfig. Ensure related tests and any call sites that asserted/relied on topP are updated (e.g., src/api/providers/tests/bedrock-reasoning.spec.ts expectations when thinking is disabled).
| @@ -647,7 +642,6 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH | |||
| const inferenceConfig: BedrockInferenceConfig = { | |||
| maxTokens: modelConfig.maxTokens || (modelConfig.info.maxTokens as number), | |||
| temperature: modelConfig.temperature ?? (this.options.modelTemperature as number), | |||
| ...(thinkingEnabled ? {} : { topP: 0.1 }), // Only set topP when thinking is NOT enabled | |||
| } | |||
Contributor
There was a problem hiding this comment.
[P2] Non-streaming path mirrors streaming config without topP. Consider adding/adjusting a unit test for completePrompt to assert that inferenceConfig does not include topP, to prevent regressions.
mrubens
approved these changes
Sep 29, 2025
daniel-lxs
force-pushed
the
fix/bedrock-sonnet-4.5-temperature-top-p
branch
from
7755b21 to
cc94b2c
Compare
Fixes #8377 - Removed topP from BedrockInferenceConfig interface - Removed topP assignment in createMessage() method - Removed topP assignment in completePrompt() method - All 107 Bedrock tests pass including reasoning tests
…ter model renames
daniel-lxs
force-pushed
the
fix/bedrock-sonnet-4.5-temperature-top-p
branch
from
cc94b2c to
cdc8452
Compare
hannesrudolph
added
PR - Needs Review
and removed
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
mini2s
added a commit
to zgsm-ai/costrict
that referenced
this pull request
Sep 30, 2025
* fix: identify mcp and slash command config path in multiple folder workspace (RooCodeInc#6904) * fix: resolve CI e2e test ETIMEDOUT errors when downloading VS Code (RooCodeInc#7583) * fix: Tackling Race/State condition issue by Changing the Code Design for Gemini Grounding Sources (RooCodeInc#7434) Co-authored-by: daniel-lxs <[email protected]> Co-authored-by: Matt Rubens <[email protected]> * fix: preserve context by retrying with full conversation on invalid previous_response_id (RooCodeInc#7714) * chore: add changeset for v3.26.8 (RooCodeInc#7715) * feat(checkpoints): create checkpoint when user sends a message (RooCodeInc#7713) * feat(checkpoints): create checkpoint on user message send * fix(checkpoints): suppress implicit user-message checkpoint row; keep current checkpoint updated without a chat row * Fix checkpoint suppression for user messages - Propagate suppressMessage flag through event chain properly - Update ChatView to check checkpoint metadata for suppressMessage flag - Ensure checkpoint messages are created but not rendered when suppressed - Fix bug where checkpointSave(false) should have been checkpointSave(true) * fix: only create checkpoint on user message when files have changed - Changed allowEmpty from true to false in checkpointSave call - Checkpoints will now only be created when there are actual file changes - This avoids creating empty commits in the shadow git repository * test: update checkpoint test to include suppressMessage parameter - Fixed test expectation to match the new function signature - saveCheckpoint now expects both allowEmpty and suppressMessage parameters --------- Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: Daniel Riccio <[email protected]> * Bump to 3.27.0 (RooCodeInc#7719) * Changeset version bump (RooCodeInc#7716) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt Rubens <[email protected]> * fix: update DeepSeek pricing to new unified rates effective Sept 5, 2025 (RooCodeInc#7687) - Updated deepseek-chat pricing: $0.56 input (cache miss), $0.07 (cache hit), $1.68 output - Updated deepseek-reasoner pricing: same unified rates as deepseek-chat - Both models now have identical pricing as per DeepSeek announcement - Pricing takes effect at 16:00 UTC, Sept 5th, 2025 Fixes RooCodeInc#7685 Co-authored-by: Roo Code <[email protected]> * feat: replace cloud waitlist ad with direct Cloud link in navigation (RooCodeInc#7742) Co-authored-by: Roo Code <[email protected]> * feat: show dash instead of zero for missing data on evals page (RooCodeInc#7748) Co-authored-by: Roo Code <[email protected]> * Feature/update vertex ai models and regions (RooCodeInc#7727) * Add model info to eval runs table (RooCodeInc#7749) * chore(deps): update dependency eslint-config-prettier to v10.1.8 (RooCodeInc#6464) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency esbuild to v0.25.9 (RooCodeInc#5455) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency @changesets/cli to v2.29.6 (RooCodeInc#7376) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency nock to v14.0.10 (RooCodeInc#6465) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update dependency eslint-plugin-turbo to v2.5.6 (RooCodeInc#7764) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * refactor(utils): simplify shell detection logic * Merge remote-tracking branch 'upstream/main' into roo-to-main * test: update shell detection test expectation and mocks * test: add mock cleanup in shell detection tests * test: update shell detection tests to prioritize PowerShell 7 * fix(workspace-event-monitor): increase max retries and improve retry logic * refactor(codebase): improve error handling and workspace validation * Revert PR RooCodeInc#7188 - Restore temperature parameter to fix TabbyApi/ExLlamaV2 crashes (RooCodeInc#7594) * fix: reduce CodeBlock button z-index to prevent overlap with popovers (RooCodeInc#7783) Fixes RooCodeInc#7703 - CodeBlock language dropdown and copy button were appearing above popovers due to z-index: 100. Reduced to z-index: 40 to maintain proper layering hierarchy while keeping buttons functional. * Make ollama models info transport work like lmstudio (RooCodeInc#7679) * feat: add click-to-edit, ESC-to-cancel, and fix padding consistency for chat messages (RooCodeInc#7790) * feat: add click-to-edit, ESC-to-cancel, and fix padding consistency - Enable click-to-edit for past messages by making message text clickable - Add ESC key handler to cancel edit mode in ChatTextArea - Fix padding consistency between past and queued message editors - Adjust right padding for edit mode to accommodate cancel button Fixes RooCodeInc#7788 * fix: adjust padding and layout for ChatTextArea in edit mode * refactor: replace hardcoded pr-[72px] with standard Tailwind pr-20 class --------- Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: daniel-lxs <[email protected]> * Let people paste in the auth redirect url (RooCodeInc#7805) Co-authored-by: roomote[bot] <219738659+roomote[bot]@users.noreply.github.com> Co-authored-by: Bruno Bergher <[email protected]> * test: change console.error to console.warn in tests * fix: resolve chat message edit/delete duplication issues (RooCodeInc#7793) * fix: add GIT_EDITOR env var to merge-resolver mode for non-interactive rebase (RooCodeInc#7819) * UI: Render reasoning as plain italic (match <thinking>) (RooCodeInc#7752) Co-authored-by: Roo Code <[email protected]> Co-authored-by: Hannes Rudolph <[email protected]> Co-authored-by: daniel-lxs <[email protected]> * Add taskSyncEnabled to userSettingsConfigSchema (
Fixes #8377
Changes
topPfromBedrockInferenceConfiginterfacetopPassignment increateMessage()methodtopPassignment incompletePrompt()methodTesting
✅ All 107 Bedrock tests pass including reasoning tests
✅ Type checking passed
✅ Linting passed
Description
The
topPparameter was being sent to AWS Bedrock even when extended thinking was enabled, which could cause issues. This PR completely removes thetopPparameter from the inference configuration, allowing the provider to rely solely onmaxTokensandtemperatureparameters for both streaming and non-streaming requests.Important
Remove
topPparameter from Bedrock inference configuration to prevent issues with extended thinking, updatingbedrock.tsand related tests.topPparameter fromBedrockInferenceConfiginbedrock.ts.topPassignment increateMessage()andcompletePrompt()methods inbedrock.ts.bedrock-reasoning.spec.tsto verifytopPis not present in payloads.This description was created by
for b5e18c6. You can customize this summary. It will automatically update as commits are pushed.