From dbb244a8d9d73253d5b0509e265bac98bae3ec86 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Sat, 3 Oct 2026 12:47:04 -0700 Subject: [PATCH] fix(provider): skip top p when custom setup temperature is set The schema already refuses both sampling fields on the wire, but the wizard still walked temperature then top p and only failed at save. --- src/tui/provider/setup.ts | 31 ++++++++- src/tui/provider/steps.test.ts | 118 +++++++++++++++++++++++++++++++++ src/tui/provider/steps.ts | 43 ++++++++++++ 3 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 src/tui/provider/steps.test.ts diff --git a/src/tui/provider/setup.ts b/src/tui/provider/setup.ts index 1ba59812e..cb4f0594b 100644 --- a/src/tui/provider/setup.ts +++ b/src/tui/provider/setup.ts @@ -69,7 +69,14 @@ import { stopRamp, teardownSurface, } from "./surface.js"; -import { stepsFor, type ProviderField, type SetupStep } from "./steps.js"; +import { + nextSamplingStepIndex, + previousSamplingStepIndex, + stepsFor, + topPAfterTemperatureAdvance, + type ProviderField, + type SetupStep, +} from "./steps.js"; import type { ProviderPreset, ProviderSetupConfig, @@ -308,6 +315,10 @@ export async function runProviderSetup( state.submitting = true; state.submitPhase = "testing"; clearError(); + state.values.topP = topPAfterTemperatureAdvance( + state.values.temperature, + state.values.topP, + ); surface.paint(); stopRamp(state); state.rampTimer = setInterval(() => surface.paintStatus(), RAMP_TICK_MS); @@ -449,7 +460,17 @@ export async function runProviderSetup( if (!stepReady(field, state.values[field])) return; if (state.stepIndex < steps().length - 1) { - state.stepIndex += 1; + if (field === "temperature") { + state.values.topP = topPAfterTemperatureAdvance( + state.values.temperature, + state.values.topP, + ); + } + state.stepIndex = nextSamplingStepIndex( + steps(), + state.stepIndex, + state.values.temperature, + ); clearError(); if (isListStep()) enterModelList(); showStep(); @@ -466,7 +487,11 @@ export async function runProviderSetup( } if (isGoModelListStep()) discovery.abandonGoPrefetch(); if (isZenModelListStep()) discovery.abandonZenPrefetch(); - state.stepIndex -= 1; + state.stepIndex = previousSamplingStepIndex( + steps(), + state.stepIndex, + state.values.temperature, + ); clearError(); if (currentStep() === "provider") enterProviderList(); else if (isListStep()) enterModelList(); diff --git a/src/tui/provider/steps.test.ts b/src/tui/provider/steps.test.ts new file mode 100644 index 000000000..f4bfc0cbc --- /dev/null +++ b/src/tui/provider/steps.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, test } from "bun:test"; + +import { + CUSTOM_STEPS, + OAUTH_STEPS, + OLLAMA_STEPS, + PRESET_STEPS, + nextSamplingStepIndex, + previousSamplingStepIndex, + skipTopPWhenTemperatureSet, + topPAfterTemperatureAdvance, +} from "./steps.js"; + +const temperatureIndex = CUSTOM_STEPS.indexOf("temperature"); +const topPIndex = CUSTOM_STEPS.indexOf("topP"); +const effortsIndex = CUSTOM_STEPS.indexOf("efforts"); +const maxTokensIndex = CUSTOM_STEPS.indexOf("maxTokens"); +const defaultEffortIndex = CUSTOM_STEPS.indexOf("defaultEffort"); + +describe("custom provider sampling-step skip", () => { + test("CUSTOM_STEPS includes sampling and effort steps", () => { + expect(temperatureIndex).toBeGreaterThanOrEqual(0); + expect(topPIndex).toBeGreaterThanOrEqual(0); + expect(effortsIndex).toBeGreaterThanOrEqual(0); + expect(maxTokensIndex).toBeGreaterThanOrEqual(0); + expect(defaultEffortIndex).toBeGreaterThanOrEqual(0); + }); + + test("nextSamplingStepIndex skips topP when temperature is set", () => { + expect(nextSamplingStepIndex(CUSTOM_STEPS, temperatureIndex, "0.7")).toBe( + effortsIndex, + ); + expect(nextSamplingStepIndex(CUSTOM_STEPS, temperatureIndex, "0")).toBe( + effortsIndex, + ); + }); + + test("nextSamplingStepIndex does not skip topP when temperature is blank", () => { + expect(nextSamplingStepIndex(CUSTOM_STEPS, temperatureIndex, "")).toBe( + topPIndex, + ); + expect(nextSamplingStepIndex(CUSTOM_STEPS, temperatureIndex, " ")).toBe( + topPIndex, + ); + }); + + test("nextSamplingStepIndex advances from topP to efforts", () => { + expect(nextSamplingStepIndex(CUSTOM_STEPS, topPIndex, "")).toBe( + effortsIndex, + ); + expect(nextSamplingStepIndex(CUSTOM_STEPS, topPIndex, "0.7")).toBe( + effortsIndex, + ); + }); + + test("nextSamplingStepIndex does not skip the temperature step", () => { + expect(nextSamplingStepIndex(CUSTOM_STEPS, maxTokensIndex, "0.7")).toBe( + temperatureIndex, + ); + }); + + test("nextSamplingStepIndex is +1 on lists without sampling steps", () => { + expect(nextSamplingStepIndex(PRESET_STEPS, 0, "0.7")).toBe(1); + expect(nextSamplingStepIndex(OAUTH_STEPS, 0, "0.7")).toBe(1); + expect(nextSamplingStepIndex(OLLAMA_STEPS, 0, "0.7")).toBe(1); + }); + + test("previousSamplingStepIndex skips topP when temperature is set", () => { + expect(previousSamplingStepIndex(CUSTOM_STEPS, effortsIndex, "0.7")).toBe( + temperatureIndex, + ); + expect(previousSamplingStepIndex(CUSTOM_STEPS, effortsIndex, "0")).toBe( + temperatureIndex, + ); + }); + + test("previousSamplingStepIndex does not skip topP when temperature is blank", () => { + expect(previousSamplingStepIndex(CUSTOM_STEPS, effortsIndex, "")).toBe( + topPIndex, + ); + expect(previousSamplingStepIndex(CUSTOM_STEPS, effortsIndex, " ")).toBe( + topPIndex, + ); + }); + + test("previousSamplingStepIndex from topP lands on temperature", () => { + expect(previousSamplingStepIndex(CUSTOM_STEPS, topPIndex, "0.7")).toBe( + temperatureIndex, + ); + expect(previousSamplingStepIndex(CUSTOM_STEPS, topPIndex, "")).toBe( + temperatureIndex, + ); + }); + + test("previousSamplingStepIndex does not skip two steps", () => { + expect( + previousSamplingStepIndex(CUSTOM_STEPS, defaultEffortIndex, "0.7"), + ).toBe(effortsIndex); + }); + + test("previousSamplingStepIndex floors at 0", () => { + expect(previousSamplingStepIndex(CUSTOM_STEPS, 0, "0.7")).toBe(0); + }); + + test("topPAfterTemperatureAdvance clears topP only when temperature is set", () => { + expect(topPAfterTemperatureAdvance("0.7", "0.9")).toBe(""); + expect(topPAfterTemperatureAdvance("0", "0.9")).toBe(""); + expect(topPAfterTemperatureAdvance("", "0.9")).toBe("0.9"); + expect(topPAfterTemperatureAdvance(" ", "0.9")).toBe("0.9"); + }); + + test("skipTopPWhenTemperatureSet treats 0 as set and whitespace as unset", () => { + expect(skipTopPWhenTemperatureSet("0")).toBe(true); + expect(skipTopPWhenTemperatureSet("0.7")).toBe(true); + expect(skipTopPWhenTemperatureSet("")).toBe(false); + expect(skipTopPWhenTemperatureSet(" ")).toBe(false); + }); +}); diff --git a/src/tui/provider/steps.ts b/src/tui/provider/steps.ts index 66de3788c..0d8b44cb3 100644 --- a/src/tui/provider/steps.ts +++ b/src/tui/provider/steps.ts @@ -132,3 +132,46 @@ export function stepsFor(choice: ProviderChoice | null): readonly SetupStep[] { if (isOllamaProviderId(choice.id)) return OLLAMA_STEPS; return choice.oauth !== null ? OAUTH_STEPS : PRESET_STEPS; } + +// Inference options XOR temperature and topP on the wire. The wizard still +// lists both steps so either can be filled; skip/clear is navigation, not +// a range check (stepReady stays range-only). +export function skipTopPWhenTemperatureSet(temperature: string): boolean { + return temperature.trim().length > 0; +} + +export function nextSamplingStepIndex( + steps: readonly SetupStep[], + currentIndex: number, + temperature: string, +): number { + const next = currentIndex + 1; + if ( + skipTopPWhenTemperatureSet(temperature) && + steps[currentIndex] === "temperature" && + steps[next] === "topP" + ) { + return next + 1; + } + return next; +} + +export function previousSamplingStepIndex( + steps: readonly SetupStep[], + currentIndex: number, + temperature: string, +): number { + if (currentIndex <= 0) return 0; + const prev = currentIndex - 1; + if (skipTopPWhenTemperatureSet(temperature) && steps[prev] === "topP") { + return Math.max(0, prev - 1); + } + return prev; +} + +export function topPAfterTemperatureAdvance( + temperature: string, + topP: string, +): string { + return skipTopPWhenTemperatureSet(temperature) ? "" : topP; +}