Merge pull request #1314 from corbitsdev/cl-9854-custom-provider-setup-lets-you-fill-both-temperature-and-top

fix(provider): skip top p when custom setup temperature is set

Author: TheGreatAxios

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();

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);
+  });
+});

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;
+}