Browse Source

revert out of scope changes from #9252 (#9258)

Hannes Rudolph 1 month ago
parent
commit
161345c6fa

+ 6 - 6
src/api/providers/__tests__/roo.spec.ts

@@ -511,17 +511,16 @@ describe("RooHandler", () => {
 				// Consume stream
 			}
 
-			const firstCallBody = mockCreate.mock.calls[0][0]
-			expect(firstCallBody).toEqual(
+			expect(mockCreate).toHaveBeenCalledWith(
 				expect.objectContaining({
 					model: mockOptions.apiModelId,
 					messages: expect.any(Array),
 					stream: true,
 					stream_options: { include_usage: true },
+					reasoning: { enabled: false },
 				}),
+				undefined,
 			)
-			expect(firstCallBody.reasoning).toBeUndefined()
-			expect(mockCreate.mock.calls[0][1]).toBeUndefined()
 		})
 
 		it("should include reasoning with enabled: false when explicitly disabled", async () => {
@@ -596,7 +595,7 @@ describe("RooHandler", () => {
 			)
 		})
 
-		it("should include reasoning for minimal", async () => {
+		it("should not include reasoning for minimal (treated as none)", async () => {
 			handler = new RooHandler({
 				...mockOptions,
 				reasoningEffort: "minimal",
@@ -606,8 +605,9 @@ describe("RooHandler", () => {
 				// Consume stream
 			}
 
+			// minimal should result in no reasoning parameter
 			const callArgs = mockCreate.mock.calls[0][0]
-			expect(callArgs.reasoning).toEqual({ enabled: true, effort: "minimal" })
+			expect(callArgs.reasoning).toBeUndefined()
 		})
 
 		it("should handle enableReasoningEffort: false overriding reasoningEffort setting", async () => {

+ 4 - 4
src/api/transform/__tests__/reasoning.spec.ts

@@ -826,10 +826,10 @@ describe("reasoning.ts", () => {
 			}
 
 			const result = getRooReasoning(options)
-			expect(result).toBeUndefined()
+			expect(result).toEqual({ enabled: false })
 		})
 
-		it("should include reasoning params for minimal effort", () => {
+		it("should omit reasoning params for minimal effort", () => {
 			const modelWithSupported: ModelInfo = {
 				...baseModel,
 				supportsReasoningEffort: true,
@@ -847,7 +847,7 @@ describe("reasoning.ts", () => {
 			}
 
 			const result = getRooReasoning(options)
-			expect(result).toEqual({ enabled: true, effort: "minimal" })
+			expect(result).toBeUndefined()
 		})
 
 		it("should handle all valid reasoning effort values", () => {
@@ -889,7 +889,7 @@ describe("reasoning.ts", () => {
 			}
 
 			const result = getRooReasoning(options)
-			expect(result).toBeUndefined()
+			expect(result).toEqual({ enabled: false })
 		})
 	})
 })

+ 18 - 7
src/api/transform/reasoning.ts

@@ -52,23 +52,34 @@ export const getRooReasoning = ({
 	// Check if model supports reasoning effort
 	if (!model.supportsReasoningEffort) return undefined
 
-	// If disabled via toggle, send explicit disabled flag for back-compat
+	// Explicit off switch from settings: always send disabled for back-compat and to
+	// prevent automatic reasoning when the toggle is turned off.
 	if (settings.enableReasoningEffort === false) {
 		return { enabled: false }
 	}
 
-	// If the selection is "disable", omit the field entirely (no reasoning param)
+	// For Roo models that support reasoning effort, absence of a selection should be
+	// treated as an explicit "off" signal so that the backend does not auto-enable
+	// reasoning. This aligns with the default behavior in tests.
+	if (!reasoningEffort) {
+		return { enabled: false }
+	}
+
+	// "disable" is a legacy sentinel that means "omit the reasoning field entirely"
+	// and let the server decide any defaults.
 	if (reasoningEffort === "disable") {
 		return undefined
 	}
 
-	// When an effort is provided (including "none" and "minimal"), enable with effort
-	if (reasoningEffort) {
-		return { enabled: true, effort: reasoningEffort as ReasoningEffortExtended }
+	// For Roo, "minimal" is treated as "none" for effort-based reasoning – we omit
+	// the reasoning field entirely instead of sending an explicit effort.
+	if (reasoningEffort === "minimal") {
+		return undefined
 	}
 
-	// No explicit selection -> omit field
-	return undefined
+	// When an effort is provided (e.g. "low" | "medium" | "high" | "none"), enable
+	// with the selected effort.
+	return { enabled: true, effort: reasoningEffort as ReasoningEffortExtended }
 }
 
 export const getAnthropicReasoning = ({

+ 58 - 73
webview-ui/src/components/settings/SimpleThinkingBudget.tsx

@@ -1,36 +1,3 @@
-/*
-Semantics for Reasoning Effort (SimpleThinkingBudget)
-
-Capability surface:
-- modelInfo.supportsReasoningEffort: boolean | Array<"disable" | "none" | "minimal" | "low" | "medium" | "high">
-  - true  → UI shows ["low","medium","high"]
-  - array → UI shows exactly the provided values
-
-Selection behavior:
-- "disable":
-  - Label: t("settings:providers.reasoningEffort.none")
-  - set enableReasoningEffort = false
-  - persist reasoningEffort = "disable"
-  - request builders omit any reasoning parameter/body sections
-- "none":
-  - Label: t("settings:providers.reasoningEffort.none")
-  - set enableReasoningEffort = true
-  - persist reasoningEffort = "none"
-  - request builders include reasoning with value "none"
-- "minimal" | "low" | "medium" | "high":
-  - set enableReasoningEffort = true
-  - persist the selected value
-  - request builders include reasoning with the selected effort
-
-Required:
-- If modelInfo.requiredReasoningEffort is true, do not synthesize a "None" choice. Only show values from the capability.
-- On mount, if unset and a default exists, set enableReasoningEffort = true and use modelInfo.reasoningEffort.
-
-Notes:
-- Current selection is normalized to the capability: unsupported persisted values are not shown.
-- Both "disable" and "none" display as the "None" label per UX, but are wired differently as above.
-- "minimal" uses t("settings:providers.reasoningEffort.minimal").
-*/
 import { useEffect } from "react"
 
 import { type ProviderSettings, type ModelInfo, type ReasoningEffort, reasoningEfforts } from "@roo-code/types"
@@ -48,8 +15,8 @@ interface SimpleThinkingBudgetProps {
 	modelInfo?: ModelInfo
 }
 
-// Reasoning selection values including control values
-type ReasoningSelectValue = "disable" | "none" | "minimal" | ReasoningEffort
+// Extended type to include "none" option
+type ReasoningEffortWithNone = ReasoningEffort | "none"
 
 export const SimpleThinkingBudget = ({
 	apiConfiguration,
@@ -58,46 +25,57 @@ export const SimpleThinkingBudget = ({
 }: SimpleThinkingBudgetProps) => {
 	const { t } = useAppTranslation()
 
-	const isSupported = !!modelInfo?.supportsReasoningEffort
+	// Check model capabilities
+	const isReasoningEffortSupported = !!modelInfo && modelInfo.supportsReasoningEffort
+	const isReasoningEffortRequired = !!modelInfo && modelInfo.requiredReasoningEffort
 
-	const isReasoningEffortRequired = !!modelInfo?.requiredReasoningEffort
+	// Build available reasoning efforts list
+	// Include "none" option unless reasoning effort is required
+	const baseEfforts = [...reasoningEfforts] as ReasoningEffort[]
+	const availableReasoningEfforts: ReadonlyArray<ReasoningEffortWithNone> = isReasoningEffortRequired
+		? baseEfforts
+		: (["none", ...baseEfforts] as ReasoningEffortWithNone[])
 
-	// Compute available options from capability
-	const supports = modelInfo?.supportsReasoningEffort
-	const availableOptions: readonly ReasoningSelectValue[] =
-		supports === true ? ([...reasoningEfforts] as const) : (supports as any)
+	// Default reasoning effort - use model's default if available, otherwise "medium"
+	const modelDefaultReasoningEffort = modelInfo?.reasoningEffort as ReasoningEffort | undefined
+	const defaultReasoningEffort: ReasoningEffortWithNone = isReasoningEffortRequired
+		? modelDefaultReasoningEffort || "medium"
+		: "none"
 
-	// Helper for labels
-	const labelFor = (v: ReasoningSelectValue) => {
-		if (v === "disable" || v === "none") return t("settings:providers.reasoningEffort.none")
-		if (v === "minimal") return t("settings:providers.reasoningEffort.minimal")
-		return t(`settings:providers.reasoningEffort.${v}`)
-	}
-
-	// Determine current selection (normalize to capability)
-	let current: ReasoningSelectValue | undefined = apiConfiguration.reasoningEffort as ReasoningSelectValue | undefined
-	if (!current && isReasoningEffortRequired && modelInfo.reasoningEffort) {
-		current = modelInfo.reasoningEffort as ReasoningSelectValue
-	}
-	// If persisted value isn't supported by capability (e.g., "minimal" while supports=true), don't show it
-	const normalizedCurrent: ReasoningSelectValue | undefined =
-		current && (availableOptions as readonly any[]).includes(current) ? current : undefined
+	// Current reasoning effort - treat undefined/null as "none"
+	const currentReasoningEffort: ReasoningEffortWithNone =
+		(apiConfiguration.reasoningEffort as ReasoningEffort | undefined) || defaultReasoningEffort
 
-	// Default when required: set to model default on mount (no synthetic "None")
+	// Set default reasoning effort when model supports it and no value is set
 	useEffect(() => {
-		if (!isReasoningEffortRequired) return
-		if (!apiConfiguration.reasoningEffort && modelInfo?.reasoningEffort) {
-			setApiConfigurationField("enableReasoningEffort", true, false)
-			setApiConfigurationField("reasoningEffort", modelInfo?.reasoningEffort as any, false)
+		if (isReasoningEffortSupported && !apiConfiguration.reasoningEffort) {
+			// Only set a default if reasoning is required, otherwise leave as undefined (which maps to "none")
+			if (isReasoningEffortRequired && defaultReasoningEffort !== "none") {
+				setApiConfigurationField("reasoningEffort", defaultReasoningEffort as ReasoningEffort, false)
+			}
 		}
 	}, [
+		isReasoningEffortSupported,
 		isReasoningEffortRequired,
 		apiConfiguration.reasoningEffort,
-		modelInfo?.reasoningEffort,
+		defaultReasoningEffort,
 		setApiConfigurationField,
 	])
 
-	if (!isSupported) {
+	useEffect(() => {
+		if (!isReasoningEffortSupported) return
+		const shouldEnable = isReasoningEffortRequired || currentReasoningEffort !== "none"
+		if (shouldEnable && apiConfiguration.enableReasoningEffort !== true) {
+			setApiConfigurationField("enableReasoningEffort", true, false)
+		}
+	}, [
+		isReasoningEffortSupported,
+		isReasoningEffortRequired,
+		currentReasoningEffort,
+		apiConfiguration.enableReasoningEffort,
+		setApiConfigurationField,
+	])
+	if (!modelInfo || !isReasoningEffortSupported) {
 		return null
 	}
 
@@ -107,25 +85,32 @@ export const SimpleThinkingBudget = ({
 				<label className="block font-medium mb-1">{t("settings:providers.reasoningEffort.label")}</label>
 			</div>
 			<Select
-				value={normalizedCurrent}
-				onValueChange={(value: ReasoningSelectValue) => {
-				if (value === "disable") {
-					setApiConfigurationField("enableReasoningEffort", false, true)
-					setApiConfigurationField("reasoningEffort", "disable" as any, true)
+				value={currentReasoningEffort}
+				onValueChange={(value: ReasoningEffortWithNone) => {
+					// If "none" is selected, clear the reasoningEffort field
+					if (value === "none") {
+						setApiConfigurationField("reasoningEffort", undefined)
 					} else {
-						setApiConfigurationField("enableReasoningEffort", true, true)
-						setApiConfigurationField("reasoningEffort", value as any, true)
+						setApiConfigurationField("reasoningEffort", value as ReasoningEffort)
 					}
 				}}>
 				<SelectTrigger className="w-full">
 					<SelectValue
-						placeholder={normalizedCurrent ? labelFor(normalizedCurrent) : t("settings:common.select")}
+						placeholder={
+							currentReasoningEffort
+								? currentReasoningEffort === "none"
+									? t("settings:providers.reasoningEffort.none")
+									: t(`settings:providers.reasoningEffort.${currentReasoningEffort}`)
+								: t("settings:common.select")
+						}
 					/>
 				</SelectTrigger>
 				<SelectContent>
-					{availableOptions.map((value) => (
+					{availableReasoningEfforts.map((value) => (
 						<SelectItem key={value} value={value}>
-							{labelFor(value)}
+							{value === "none"
+								? t("settings:providers.reasoningEffort.none")
+								: t(`settings:providers.reasoningEffort.${value}`)}
 						</SelectItem>
 					))}
 				</SelectContent>

+ 34 - 0
webview-ui/src/components/settings/ThinkingBudget.tsx

@@ -1,3 +1,37 @@
+/*
+Semantics for Reasoning Effort (ThinkingBudget)
+
+Capability surface:
+- modelInfo.supportsReasoningEffort: boolean | Array&lt;"disable" | "none" | "minimal" | "low" | "medium" | "high"&gt;
+  - true  → UI shows ["low","medium","high"]
+  - array → UI shows exactly the provided values
+
+Selection behavior:
+- "disable":
+  - Label: t("settings:providers.reasoningEffort.none")
+  - set enableReasoningEffort = false
+  - persist reasoningEffort = "disable"
+  - request builders omit any reasoning parameter/body sections
+- "none":
+  - Label: t("settings:providers.reasoningEffort.none")
+  - set enableReasoningEffort = true
+  - persist reasoningEffort = "none"
+  - request builders include reasoning with value "none"
+- "minimal" | "low" | "medium" | "high":
+  - set enableReasoningEffort = true
+  - persist the selected value
+  - request builders include reasoning with the selected effort
+
+Required:
+- If modelInfo.requiredReasoningEffort is true, do not synthesize a "None" choice. Only show values from the capability.
+- On mount, if unset and a default exists, set enableReasoningEffort = true and use modelInfo.reasoningEffort.
+
+Notes:
+- Current selection is normalized to the capability: unsupported persisted values are not shown.
+- Both "disable" and "none" display as the "None" label per UX, but are wired differently as above.
+- "minimal" uses t("settings:providers.reasoningEffort.minimal").
+*/
+
 import { useEffect } from "react"
 import { Checkbox } from "vscrui"
 

+ 1 - 2
webview-ui/src/components/settings/__tests__/SimpleThinkingBudget.spec.tsx

@@ -10,7 +10,6 @@ vi.mock("@src/i18n/TranslationContext", () => ({
 			const translations: Record<string, string> = {
 				"settings:providers.reasoningEffort.label": "Model Reasoning Effort",
 				"settings:providers.reasoningEffort.none": "None",
-				"settings:providers.reasoningEffort.minimal": "Minimal",
 				"settings:providers.reasoningEffort.low": "Low",
 				"settings:providers.reasoningEffort.medium": "Medium",
 				"settings:providers.reasoningEffort.high": "High",
@@ -190,7 +189,7 @@ describe("SimpleThinkingBudget", () => {
 			/>,
 		)
 
-		expect(screen.getByText("Select")).toBeInTheDocument()
+		expect(screen.getByText("None")).toBeInTheDocument()
 	})
 
 	it("should use model default reasoning effort when required and available", () => {