Skip to content

Commit 2bd49d7

Browse files
committed
fix(bedrock): omit temperature in completePrompt for adaptive-thinking models
Addresses CodeRabbit review on #386. completePrompt was unconditionally sending temperature in its inferenceConfig, which causes a 400 error for Claude Opus/Sonnet 4.7 and 4.8 (sampling parameters were removed by Anthropic for these models). createMessage already guarded this, but the non-stream path did not. - Extract the adaptive-thinking detection into a private isAdaptiveThinkingModel(modelId) method (parseBaseModelId-aware, so cross-region/global prefixes are handled). - Reuse it in both createMessage and completePrompt so the two request paths stay consistent. - Add two regression tests: completePrompt omits temperature for opus-4-8 and still sends it for opus-4-6. 64 bedrock tests pass, check-types clean.
1 parent e2d630a commit 2bd49d7

2 files changed

Lines changed: 77 additions & 13 deletions

File tree

src/api/providers/__tests__/bedrock.spec.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,5 +1456,48 @@ describe("AwsBedrockHandler", () => {
14561456
})
14571457
expect(commandArg.inferenceConfig?.temperature).toBeUndefined()
14581458
})
1459+
1460+
it("completePrompt should omit temperature for Claude Opus 4.8 (non-stream path)", async () => {
1461+
// Regression guard for the non-stream path: completePrompt must guard
1462+
// temperature the same way createMessage does, otherwise adaptive-thinking
1463+
// models (4.7/4.8) return a 400 from Bedrock.
1464+
const mockConverseCommand = vi.mocked(ConverseCommand)
1465+
1466+
const opus48Handler = new AwsBedrockHandler({
1467+
apiModelId: "anthropic.claude-opus-4-8",
1468+
awsAccessKey: "test-access-key",
1469+
awsSecretKey: "test-secret-key",
1470+
awsRegion: "us-east-1",
1471+
})
1472+
1473+
await opus48Handler.completePrompt("Test prompt")
1474+
1475+
expect(mockConverseCommand).toHaveBeenCalled()
1476+
const commandArg = mockConverseCommand.mock.calls[0][0] as any
1477+
1478+
// 4.8 must NOT receive temperature in the non-stream inferenceConfig.
1479+
expect(commandArg.inferenceConfig?.temperature).toBeUndefined()
1480+
})
1481+
1482+
it("completePrompt should still send temperature for older Claude Opus 4.6 (non-stream path)", async () => {
1483+
// 4.6 and earlier still accept sampling parameters, so completePrompt must
1484+
// continue to send temperature for them.
1485+
const mockConverseCommand = vi.mocked(ConverseCommand)
1486+
1487+
const opus46Handler = new AwsBedrockHandler({
1488+
apiModelId: "anthropic.claude-opus-4-6-v1",
1489+
awsAccessKey: "test-access-key",
1490+
awsSecretKey: "test-secret-key",
1491+
awsRegion: "us-east-1",
1492+
})
1493+
1494+
await opus46Handler.completePrompt("Test prompt")
1495+
1496+
expect(mockConverseCommand).toHaveBeenCalled()
1497+
const commandArg = mockConverseCommand.mock.calls[0][0] as any
1498+
1499+
// 4.6 must still receive temperature.
1500+
expect(commandArg.inferenceConfig?.temperature).toBeDefined()
1501+
})
14591502
})
14601503
})

src/api/providers/bedrock.ts

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,30 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
297297
this.client = new BedrockRuntimeClient(clientConfig)
298298
}
299299

300+
/**
301+
* Detect models that require the adaptive-thinking API contract.
302+
*
303+
* Starting with Claude Opus 4.7 (and the matching Sonnet 4.7), and continuing
304+
* in Opus 4.8 / Sonnet 4.8, Anthropic removed sampling parameters
305+
* (temperature/top_p/top_k) and replaced budget_tokens-based thinking with
306+
* `thinking.type: "adaptive"` plus `output_config.effort`. The migration guide
307+
* from 4.7 → 4.8 confirms there are no further breaking API changes, so a single
308+
* guard matches both generations. Shared by createMessage and completePrompt so
309+
* both request paths omit temperature for these models (sending it causes a 400).
310+
*
311+
* Accepts a model ID (with or without a cross-region/global prefix) and strips
312+
* the prefix via parseBaseModelId before matching.
313+
*/
314+
private isAdaptiveThinkingModel(modelId: string): boolean {
315+
const baseModelId = this.parseBaseModelId(modelId)
316+
return (
317+
baseModelId.includes("opus-4-7") ||
318+
baseModelId.includes("opus-4-8") ||
319+
baseModelId.includes("sonnet-4-7") ||
320+
baseModelId.includes("sonnet-4-8")
321+
)
322+
}
323+
300324
// Helper to guess model info from custom modelId string if not in bedrockModels
301325
private guessModelInfoFromId(modelId: string): Partial<ModelInfo> {
302326
// Define a mapping for model ID patterns and their configurations
@@ -392,19 +416,11 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
392416
let additionalModelRequestFields: BedrockAdditionalModelFields | undefined
393417
let thinkingEnabled = false
394418

395-
// Detect models that require the adaptive-thinking API contract.
396-
// Starting with Claude Opus 4.7 (and the matching Sonnet 4.7), and continuing
397-
// in Opus 4.8 / Sonnet 4.8, Anthropic removed sampling parameters
398-
// (temperature/top_p/top_k) and replaced budget_tokens-based thinking with
399-
// `thinking.type: "adaptive"` plus `output_config.effort`. The migration guide
400-
// from 4.7 → 4.8 confirms there are no further breaking API changes, so we
401-
// keep a single guard here that matches both generations.
419+
// Detect models that require the adaptive-thinking API contract (Opus/Sonnet
420+
// 4.7 and 4.8). See isAdaptiveThinkingModel for details. The same guard is
421+
// reused in completePrompt so both request paths stay consistent.
402422
const baseModelId = this.parseBaseModelId(modelConfig.id)
403-
const isAdaptiveThinkingModel =
404-
baseModelId.includes("opus-4-7") ||
405-
baseModelId.includes("opus-4-8") ||
406-
baseModelId.includes("sonnet-4-7") ||
407-
baseModelId.includes("sonnet-4-8")
423+
const isAdaptiveThinkingModel = this.isAdaptiveThinkingModel(modelConfig.id)
408424

409425
// Determine if thinking should be enabled
410426
// metadata?.thinking?.enabled: Explicitly enabled through API metadata (direct request)
@@ -788,7 +804,12 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
788804

789805
const inferenceConfig: BedrockInferenceConfig = {
790806
maxTokens: modelConfig.maxTokens || (modelConfig.info.maxTokens as number),
791-
temperature: modelConfig.temperature ?? (this.options.modelTemperature as number),
807+
// Claude 4.7+ (including 4.8) removed sampling parameters entirely —
808+
// sending temperature causes a 400 error. Guard the non-stream path the
809+
// same way createMessage does so completePrompt also works for these models.
810+
...(this.isAdaptiveThinkingModel(modelConfig.id)
811+
? {}
812+
: { temperature: modelConfig.temperature ?? (this.options.modelTemperature as number) }),
792813
}
793814

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

0 commit comments

Comments
 (0)