Skip to content

Commit d00c525

Browse files
committed
fix: address PR review feedback (#4481)
- Simplify ThinkingBudget ternary logic since component only renders when reasoning budget supported - Break down complex thinking enabled condition with clear documentation - Replace 'as any' usage with proper TypeScript interfaces for AWS SDK events - Add comprehensive documentation for multiple stream structures explaining AWS SDK compatibility
1 parent d37cec9 commit d00c525

File tree

2 files changed

+68
-68
lines changed

2 files changed

+68
-68
lines changed

src/api/providers/bedrock.ts

Lines changed: 62 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,42 @@ interface BedrockPayload {
6565
additionalModelRequestFields?: BedrockThinkingConfig
6666
}
6767

68+
// Define specific types for content block events to avoid 'as any' usage
69+
// These handle the multiple possible structures returned by AWS SDK
70+
interface ContentBlockStartEvent {
71+
start?: {
72+
text?: string
73+
thinking?: string
74+
}
75+
contentBlockIndex?: number
76+
// Alternative structure used by some AWS SDK versions
77+
content_block?: {
78+
type?: string
79+
thinking?: string
80+
}
81+
// Official AWS SDK structure for reasoning (as documented)
82+
contentBlock?: {
83+
type?: string
84+
thinking?: string
85+
reasoningContent?: {
86+
text?: string
87+
}
88+
}
89+
}
90+
91+
interface ContentBlockDeltaEvent {
92+
delta?: {
93+
text?: string
94+
thinking?: string
95+
type?: string
96+
// AWS SDK structure for reasoning content deltas
97+
reasoningContent?: {
98+
text?: string
99+
}
100+
}
101+
contentBlockIndex?: number
102+
}
103+
68104
// Define types for stream events based on AWS SDK
69105
export interface StreamEvent {
70106
messageStart?: {
@@ -74,38 +110,8 @@ export interface StreamEvent {
74110
stopReason?: "end_turn" | "tool_use" | "max_tokens" | "stop_sequence"
75111
additionalModelResponseFields?: Record<string, unknown>
76112
}
77-
contentBlockStart?: {
78-
start?: {
79-
text?: string
80-
thinking?: string
81-
}
82-
contentBlockIndex?: number
83-
content_block?: {
84-
type?: string
85-
thinking?: string
86-
}
87-
// Alternative structure that might be used
88-
contentBlock?: {
89-
type?: string
90-
thinking?: string
91-
// AWS SDK structure for reasoning
92-
reasoningContent?: {
93-
text?: string
94-
}
95-
}
96-
}
97-
contentBlockDelta?: {
98-
delta?: {
99-
text?: string
100-
thinking?: string
101-
type?: string
102-
// Based on AWS docs, reasoning might come through reasoningContent
103-
reasoningContent?: {
104-
text?: string
105-
}
106-
}
107-
contentBlockIndex?: number
108-
}
113+
contentBlockStart?: ContentBlockStartEvent
114+
contentBlockDelta?: ContentBlockDeltaEvent
109115
metadata?: {
110116
usage?: {
111117
inputTokens: number
@@ -327,13 +333,16 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
327333
let additionalModelRequestFields: BedrockThinkingConfig | undefined
328334
let thinkingEnabled = false
329335

330-
if (
331-
(metadata?.thinking?.enabled ||
332-
(shouldUseReasoningBudget({ model: modelConfig.info, settings: this.options }) &&
333-
modelConfig.reasoning &&
334-
modelConfig.reasoningBudget)) &&
335-
modelConfig.info.supportsReasoningBudget
336-
) {
336+
// Determine if thinking should be enabled
337+
// metadata?.thinking?.enabled: Explicitly enabled through API metadata (direct request)
338+
// shouldUseReasoningBudget(): Enabled through user settings (enableReasoningEffort = true)
339+
const isThinkingExplicitlyEnabled = metadata?.thinking?.enabled
340+
const isThinkingEnabledBySettings =
341+
shouldUseReasoningBudget({ model: modelConfig.info, settings: this.options }) &&
342+
modelConfig.reasoning &&
343+
modelConfig.reasoningBudget
344+
345+
if ((isThinkingExplicitlyEnabled || isThinkingEnabledBySettings) && modelConfig.info.supportsReasoningBudget) {
337346
thinkingEnabled = true
338347
additionalModelRequestFields = {
339348
thinking: {
@@ -471,10 +480,9 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
471480

472481
// Handle content blocks
473482
if (streamEvent.contentBlockStart) {
474-
// The actual AWS SDK structure based on the documentation
475-
const cbStart = streamEvent.contentBlockStart as any
483+
const cbStart = streamEvent.contentBlockStart
476484

477-
// Check if this is a reasoning block
485+
// Check if this is a reasoning block (official AWS SDK structure)
478486
if (cbStart.contentBlock?.reasoningContent) {
479487
if (cbStart.contentBlockIndex && cbStart.contentBlockIndex > 0) {
480488
yield { type: "reasoning", text: "\n" }
@@ -484,13 +492,15 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
484492
text: cbStart.contentBlock.reasoningContent.text || "",
485493
}
486494
}
487-
// Check for thinking block - handle both possible structures
495+
// Check for thinking block - handle both possible AWS SDK structures
496+
// cbStart.contentBlock: newer/official structure
497+
// cbStart.content_block: alternative structure seen in some AWS SDK versions
488498
else if (cbStart.contentBlock?.type === "thinking" || cbStart.content_block?.type === "thinking") {
489499
const contentBlock = cbStart.contentBlock || cbStart.content_block
490500
if (cbStart.contentBlockIndex && cbStart.contentBlockIndex > 0) {
491501
yield { type: "reasoning", text: "\n" }
492502
}
493-
if (contentBlock.thinking) {
503+
if (contentBlock?.thinking) {
494504
yield {
495505
type: "reasoning",
496506
text: contentBlock.thinking,
@@ -507,12 +517,16 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
507517

508518
// Handle content deltas
509519
if (streamEvent.contentBlockDelta) {
510-
const cbDelta = streamEvent.contentBlockDelta as any
520+
const cbDelta = streamEvent.contentBlockDelta
511521
const delta = cbDelta.delta
512522

513-
// Check for reasoning content - handle multiple possible structures
523+
// Process reasoning and text content deltas
524+
// Multiple structures are supported for AWS SDK compatibility:
525+
// - delta.reasoningContent.text: official AWS docs structure for reasoning
526+
// - delta.thinking: alternative structure for thinking content
527+
// - delta.text: standard text content
514528
if (delta) {
515-
// Check for reasoningContent property (as shown in AWS docs)
529+
// Check for reasoningContent property (official AWS SDK structure)
516530
if (delta.reasoningContent?.text) {
517531
yield {
518532
type: "reasoning",
@@ -521,7 +535,7 @@ export class AwsBedrockHandler extends BaseProvider implements SingleCompletionH
521535
continue
522536
}
523537

524-
// Fallback to original structure
538+
// Handle alternative thinking structure (fallback for older SDK versions)
525539
if (delta.type === "thinking_delta" && delta.thinking) {
526540
yield {
527541
type: "reasoning",

webview-ui/src/components/settings/ThinkingBudget.tsx

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,7 @@ import { Checkbox } from "vscrui"
33

44
import { type ProviderSettings, type ModelInfo, type ReasoningEffort, reasoningEfforts } from "@roo-code/types"
55

6-
import {
7-
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS,
8-
DEFAULT_HYBRID_REASONING_MODEL_THINKING_TOKENS,
9-
getModelMaxOutputTokens,
10-
} from "@roo/api"
6+
import { DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, DEFAULT_HYBRID_REASONING_MODEL_THINKING_TOKENS } from "@roo/api"
117

128
import { useAppTranslation } from "@src/i18n/TranslationContext"
139
import { Slider, Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@src/components/ui"
@@ -69,21 +65,11 @@ export const ThinkingBudget = ({ apiConfiguration, setApiConfigurationField, mod
6965
<div className="flex items-center gap-1">
7066
<Slider
7167
min={8192}
72-
max={
73-
apiConfiguration.apiModelId && isReasoningBudgetSupported
74-
? Math.max(
75-
modelInfo.maxTokens || 8192,
76-
customMaxOutputTokens,
77-
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS,
78-
)
79-
: apiConfiguration.apiModelId
80-
? getModelMaxOutputTokens({
81-
modelId: apiConfiguration.apiModelId,
82-
model: modelInfo,
83-
settings: apiConfiguration,
84-
})
85-
: modelInfo.maxTokens
86-
}
68+
max={Math.max(
69+
modelInfo.maxTokens || 8192,
70+
customMaxOutputTokens,
71+
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS,
72+
)}
8773
step={1024}
8874
value={[customMaxOutputTokens]}
8975
onValueChange={([value]) => setApiConfigurationField("modelMaxTokens", value)}

0 commit comments

Comments
 (0)