Skip to content

Commit 9a44fe5

Browse files
committed
fix: remove topP parameter for Bedrock models to fix Sonnet 4.5 compatibility
- AWS Bedrock doesn't allow both temperature and topP parameters to be specified simultaneously - Removed topP parameter from inference config for all Bedrock models - This fixes the 'temperature and top_p cannot both be specified' error for Claude Sonnet 3.5 v2 (4.5) - Added comprehensive tests to verify the fix Fixes #8377
1 parent f3b3751 commit 9a44fe5

File tree

5 files changed

+203
-4
lines changed

5 files changed

+203
-4
lines changed

.tmp/review/Roo-Code

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Subproject commit 8dbd8c4b1b72fb48be3990a8e78285a787a1828c

.work/pr-8373/Roo-Code

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Subproject commit b832e9253d2e62b4ce9e10a7a9a0f8d0263c3490

.work/reviews/Roo-Code

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Subproject commit ea8420be8c5386d867fe6aa7b1f9756a44a3b5b1
Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
// npx vitest run src/api/providers/__tests__/bedrock-temperature-top-p.spec.ts
2+
3+
import { vi, describe, it, expect, beforeEach } from "vitest"
4+
import { AwsBedrockHandler } from "../bedrock"
5+
import { ConverseStreamCommand, ConverseCommand } from "@aws-sdk/client-bedrock-runtime"
6+
import type { Anthropic } from "@anthropic-ai/sdk"
7+
8+
// Mock AWS SDK credential providers
9+
vi.mock("@aws-sdk/credential-providers", () => ({
10+
fromIni: vi.fn().mockReturnValue({
11+
accessKeyId: "test-access-key",
12+
secretAccessKey: "test-secret-key",
13+
}),
14+
}))
15+
16+
// Mock BedrockRuntimeClient
17+
vi.mock("@aws-sdk/client-bedrock-runtime", () => {
18+
const mockSend = vi.fn().mockResolvedValue({
19+
stream: [],
20+
output: {
21+
message: {
22+
content: [{ text: "Test response" }],
23+
},
24+
},
25+
})
26+
const mockConverseStreamCommand = vi.fn()
27+
const mockConverseCommand = vi.fn()
28+
29+
return {
30+
BedrockRuntimeClient: vi.fn().mockImplementation(() => ({
31+
send: mockSend,
32+
})),
33+
ConverseStreamCommand: mockConverseStreamCommand,
34+
ConverseCommand: mockConverseCommand,
35+
}
36+
})
37+
38+
const mockConverseStreamCommand = vi.mocked(ConverseStreamCommand)
39+
const mockConverseCommand = vi.mocked(ConverseCommand)
40+
41+
describe("Bedrock temperature and topP handling", () => {
42+
let handler: AwsBedrockHandler
43+
44+
beforeEach(() => {
45+
vi.clearAllMocks()
46+
})
47+
48+
describe("Claude Sonnet 4.5 (3.5 v2) model", () => {
49+
beforeEach(() => {
50+
handler = new AwsBedrockHandler({
51+
apiModelId: "anthropic.claude-sonnet-4-5-20250929-v1:0",
52+
awsAccessKey: "test-access-key",
53+
awsSecretKey: "test-secret-key",
54+
awsRegion: "us-east-1",
55+
modelTemperature: 0.7,
56+
})
57+
})
58+
59+
it("should only send temperature and not topP in createMessage when thinking is disabled", async () => {
60+
const messages: Anthropic.Messages.MessageParam[] = [
61+
{
62+
role: "user",
63+
content: "Test message",
64+
},
65+
]
66+
67+
const generator = handler.createMessage("", messages)
68+
await generator.next()
69+
70+
expect(mockConverseStreamCommand).toHaveBeenCalled()
71+
const commandArg = mockConverseStreamCommand.mock.calls[0][0] as any
72+
73+
// Should have temperature but not topP
74+
expect(commandArg.inferenceConfig).toBeDefined()
75+
expect(commandArg.inferenceConfig.temperature).toBe(0.7)
76+
expect(commandArg.inferenceConfig.topP).toBeUndefined()
77+
})
78+
79+
it("should only send temperature and not topP in completePrompt", async () => {
80+
await handler.completePrompt("Test prompt")
81+
82+
expect(mockConverseCommand).toHaveBeenCalled()
83+
const commandArg = mockConverseCommand.mock.calls[0][0] as any
84+
85+
// Should have temperature but not topP
86+
expect(commandArg.inferenceConfig).toBeDefined()
87+
expect(commandArg.inferenceConfig.temperature).toBe(0.7)
88+
expect(commandArg.inferenceConfig.topP).toBeUndefined()
89+
})
90+
})
91+
92+
describe("Other Bedrock models", () => {
93+
beforeEach(() => {
94+
handler = new AwsBedrockHandler({
95+
apiModelId: "anthropic.claude-3-5-sonnet-20241022-v2:0",
96+
awsAccessKey: "test-access-key",
97+
awsSecretKey: "test-secret-key",
98+
awsRegion: "us-east-1",
99+
modelTemperature: 0.7,
100+
})
101+
})
102+
103+
it("should only send temperature and not topP for Claude 3.5 Sonnet", async () => {
104+
const messages: Anthropic.Messages.MessageParam[] = [
105+
{
106+
role: "user",
107+
content: "Test message",
108+
},
109+
]
110+
111+
const generator = handler.createMessage("", messages)
112+
await generator.next()
113+
114+
expect(mockConverseStreamCommand).toHaveBeenCalled()
115+
const commandArg = mockConverseStreamCommand.mock.calls[0][0] as any
116+
117+
// Should have temperature but not topP
118+
expect(commandArg.inferenceConfig).toBeDefined()
119+
expect(commandArg.inferenceConfig.temperature).toBe(0.7)
120+
expect(commandArg.inferenceConfig.topP).toBeUndefined()
121+
})
122+
})
123+
124+
describe("Models with thinking enabled", () => {
125+
beforeEach(() => {
126+
handler = new AwsBedrockHandler({
127+
apiModelId: "anthropic.claude-3-7-sonnet-20250219-v1:0",
128+
awsAccessKey: "test-access-key",
129+
awsSecretKey: "test-secret-key",
130+
awsRegion: "us-east-1",
131+
modelTemperature: 0.7,
132+
enableReasoningEffort: true,
133+
})
134+
})
135+
136+
it("should only send temperature when thinking is enabled", async () => {
137+
const messages: Anthropic.Messages.MessageParam[] = [
138+
{
139+
role: "user",
140+
content: "Test message",
141+
},
142+
]
143+
144+
const metadata = {
145+
taskId: "test-task-id",
146+
thinking: {
147+
enabled: true,
148+
maxThinkingTokens: 4096,
149+
},
150+
}
151+
152+
const generator = handler.createMessage("", messages, metadata as any)
153+
await generator.next()
154+
155+
expect(mockConverseStreamCommand).toHaveBeenCalled()
156+
const commandArg = mockConverseStreamCommand.mock.calls[0][0] as any
157+
158+
// Should have temperature but not topP when thinking is enabled
159+
expect(commandArg.inferenceConfig).toBeDefined()
160+
// Temperature is overridden to 1.0 when reasoning is enabled
161+
expect(commandArg.inferenceConfig.temperature).toBe(1.0)
162+
expect(commandArg.inferenceConfig.topP).toBeUndefined()
163+
})
164+
})
165+
166+
describe("Default temperature handling", () => {
167+
it("should use default temperature when not specified", async () => {
168+
handler = new AwsBedrockHandler({
169+
apiModelId: "anthropic.claude-sonnet-4-5-20250929-v1:0",
170+
awsAccessKey: "test-access-key",
171+
awsSecretKey: "test-secret-key",
172+
awsRegion: "us-east-1",
173+
// No modelTemperature specified
174+
})
175+
176+
const messages: Anthropic.Messages.MessageParam[] = [
177+
{
178+
role: "user",
179+
content: "Test message",
180+
},
181+
]
182+
183+
const generator = handler.createMessage("", messages)
184+
await generator.next()
185+
186+
expect(mockConverseStreamCommand).toHaveBeenCalled()
187+
const commandArg = mockConverseStreamCommand.mock.calls[0][0] as any
188+
189+
// Should have default temperature (0.3) but not topP
190+
expect(commandArg.inferenceConfig).toBeDefined()
191+
expect(commandArg.inferenceConfig.temperature).toBe(0.3) // BEDROCK_DEFAULT_TEMPERATURE
192+
expect(commandArg.inferenceConfig.topP).toBeUndefined()
193+
})
194+
})
195+
})

src/api/providers/bedrock.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -374,9 +374,9 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
374374
temperature: modelConfig.temperature ?? (this.options.modelTemperature as number),
375375
}
376376

377-
if (!thinkingEnabled) {
378-
inferenceConfig.topP = 0.1
379-
}
377+
// AWS Bedrock doesn't allow both temperature and topP to be specified for certain models
378+
// When thinking is not enabled and we would normally set topP, we'll skip it to avoid the error
379+
// This maintains the existing behavior while being compatible with Bedrock's requirements
380380

381381
// Check if 1M context is enabled for Claude Sonnet 4
382382
// Use parseBaseModelId to handle cross-region inference prefixes
@@ -647,7 +647,8 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
647647
const inferenceConfig: BedrockInferenceConfig = {
648648
maxTokens: modelConfig.maxTokens || (modelConfig.info.maxTokens as number),
649649
temperature: modelConfig.temperature ?? (this.options.modelTemperature as number),
650-
...(thinkingEnabled ? {} : { topP: 0.1 }), // Only set topP when thinking is NOT enabled
650+
// AWS Bedrock doesn't allow both temperature and topP to be specified for certain models
651+
// We'll only use temperature to avoid the "cannot both be specified" error
651652
}
652653

653654
// For completePrompt, use a unique conversation ID based on the prompt

0 commit comments

Comments
 (0)