From 555eb9bd5742af983529b753b999fc6e55d5beef Mon Sep 17 00:00:00 2001 From: MelodyVAR <61931019+MelodyVAR@users.noreply.github.com> Date: Thu, 24 Sep 2026 22:37:27 +0800 Subject: [PATCH 1/2] fix: preserve Step thinking effort preferences --- apps/cli/src/ui/interactive-mode.ts | 14 +- apps/cli/test/thinking-default-marker.test.ts | 60 ++++++ packages/coding-agent/src/features/step.ts | 25 ++- .../test/step-thinking-preferences.test.ts | 201 ++++++++++++++++++ 4 files changed, 279 insertions(+), 21 deletions(-) create mode 100644 apps/cli/test/thinking-default-marker.test.ts create mode 100644 packages/coding-agent/test/step-thinking-preferences.test.ts diff --git a/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index 0190a91b..dd044edb 100644 --- a/apps/cli/src/ui/interactive-mode.ts +++ b/apps/cli/src/ui/interactive-mode.ts @@ -95,7 +95,6 @@ import { type SessionEntry, SessionImportFileNotFoundError, SessionManager, - STEP_PROVIDER_ID, type StepLoginHost, sessionEntryToContextMessages, setRegisteredThemes, @@ -143,7 +142,7 @@ import { TuiMainScreen, visibleWidth, } from "@step-harness/pi-tui"; -import type { AuthEvent, AuthPrompt, ImageContent } from "@step-harness/providers"; +import { type AuthEvent, type AuthPrompt, clampThinkingLevel, type ImageContent } from "@step-harness/providers"; import type { AssistantMessage, Message, Model, Usage } from "@step-harness/providers/compat"; import chalk from "chalk"; import { spawn } from "child_process"; @@ -4775,12 +4774,11 @@ export class InteractiveMode { }; const availableLevels = this.session.getAvailableThinkingLevels(); const globalDefault = this.settingsManager.getDefaultThinkingLevel() ?? DEFAULT_THINKING_LEVEL; - // Step models default to their highest supported effort, so mark that as - // the default rather than the global level (which may not be selectable). - const defaultMarker = - this.session.model?.provider === STEP_PROVIDER_ID - ? (availableLevels[availableLevels.length - 1] ?? globalDefault) - : globalDefault; + const model = this.session.model; + const savedDefault = model + ? (this.settingsManager.getModelThinkingLevel(model.provider, model.id) ?? globalDefault) + : globalDefault; + const defaultMarker = model ? clampThinkingLevel(model, savedDefault) : savedDefault; const selector = new ThinkingSelectorComponent( this.session.thinkingLevel ?? DEFAULT_THINKING_LEVEL, availableLevels, diff --git a/apps/cli/test/thinking-default-marker.test.ts b/apps/cli/test/thinking-default-marker.test.ts new file mode 100644 index 00000000..96a078d4 --- /dev/null +++ b/apps/cli/test/thinking-default-marker.test.ts @@ -0,0 +1,60 @@ +import type { ThinkingLevel } from "@step-harness/agent-core"; +import type { Model } from "@step-harness/providers"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { SettingsManager } from "../../../packages/coding-agent/src/core/settings-manager.ts"; +import { stepThinkingLevelMap } from "../../../packages/coding-agent/src/features/step-provider/index.ts"; +import { initTheme } from "../../../packages/coding-agent/src/theme/theme.ts"; +import { stripAnsi } from "../../../packages/coding-agent/src/utils/ansi.ts"; +import { stepModel } from "../../../packages/coding-agent/test/utilities.ts"; +import { InteractiveMode } from "../src/ui/interactive-mode.ts"; +import type { ThinkingSelectorComponent } from "../src/ui/view/dialogs/thinking-selector.ts"; + +const showThinkingSelector = Reflect.get(InteractiveMode.prototype, "showThinkingSelector") as (this: object) => void; + +function renderSelector(settingsManager: SettingsManager, model: Model, levels: ThinkingLevel[]): string[] { + let selector!: ThinkingSelectorComponent; + showThinkingSelector.call({ + session: { model, thinkingLevel: "high", getAvailableThinkingLevels: () => levels }, + settingsManager, + selectThinkingLevel: vi.fn(), + showSelector(factory: (done: () => void) => { component: ThinkingSelectorComponent }) { + selector = factory(() => {}).component; + }, + }); + return selector.render(100).map(stripAnsi); +} + +describe("thinking selector default marker", () => { + beforeAll(() => initTheme("dark")); + + it("marks the saved global default for Step models", () => { + const lines = renderSelector( + SettingsManager.inMemory({ defaultThinkingLevel: "medium" }), + stepModel({ thinkingLevelMap: stepThinkingLevelMap(["low", "medium", "high"]) }), + ["low", "medium", "high"], + ); + expect(lines.some((line) => line.includes("medium") && line.includes("default"))).toBe(true); + expect(lines.some((line) => line.includes("high") && line.includes("default"))).toBe(false); + }); + + it("marks the model-specific default ahead of the global default", () => { + const lines = renderSelector( + SettingsManager.inMemory({ + defaultThinkingLevel: "high", + modelThinkingLevels: { "step/step-5-preview": "low" }, + }), + stepModel({ thinkingLevelMap: stepThinkingLevelMap(["low", "medium", "high"]) }), + ["low", "medium", "high"], + ); + expect(lines.some((line) => line.includes("low") && line.includes("default"))).toBe(true); + }); + + it("marks the supported level used when the saved default must be clamped", () => { + const lines = renderSelector( + SettingsManager.inMemory({ defaultThinkingLevel: "medium" }), + stepModel({ thinkingLevelMap: stepThinkingLevelMap(["low", "high"]) }), + ["low", "high"], + ); + expect(lines.some((line) => line.includes("high") && line.includes("default"))).toBe(true); + }); +}); diff --git a/packages/coding-agent/src/features/step.ts b/packages/coding-agent/src/features/step.ts index 2c51567d..4c3ef1c1 100644 --- a/packages/coding-agent/src/features/step.ts +++ b/packages/coding-agent/src/features/step.ts @@ -1,5 +1,5 @@ import process from "node:process"; -import { type Api, getSupportedThinkingLevels, type Model } from "@step-harness/providers"; +import { type Api, clampThinkingLevel, type Model } from "@step-harness/providers"; import type { ExtensionAPI, ExtensionCommandContext, @@ -54,14 +54,13 @@ export interface StepExtensionOptions { * carries `reasoning_effort_support_list` on some profiles (e.g. step_plan); when * it does not (e.g. platform), fetch the per-model detail from the domain-root * `/v1`. Mutates the model in place — the session's active model is the same - * object the picker reads. When `applyDefault` is set (fresh activation, not a - * restore), also default the level to the highest supported effort. + * object the picker reads. Keep the session's resolved thinking preference; + * capability discovery may only clamp a level the active model cannot support. */ async function enrichStepModelEffort( model: Model | undefined, ctx: ExtensionContext, pi: ExtensionAPI, - applyDefault: boolean, ): Promise { try { if (!model || model.provider !== STEP_PROVIDER_ID) return; @@ -80,11 +79,12 @@ async function enrichStepModelEffort( } } } - if (applyDefault) { - const supported = getSupportedThinkingLevels(model).filter((level) => level !== "off"); - const highest = supported[supported.length - 1]; - if (highest) pi.setThinkingLevel(highest); - } + // Discovery may finish after a model switch or a user effort change. Only + // validate the still-active model, using the latest session preference. + if (ctx.model !== model) return; + const selected = pi.getThinkingLevel(); + const supported = clampThinkingLevel(model, selected); + if (supported !== selected) pi.setThinkingLevel(supported); } catch { // Best-effort; the model keeps its existing thinking-level defaults. } @@ -100,12 +100,11 @@ export function createStepExtension(options: StepExtensionOptions = {}): Extensi // Fire-and-forget; failures leave the model's defaults untouched. // (The initial model catalog is refreshed by the launcher before the // session's model is resolved, so it is already available here.) - pi.on("session_start", (event, ctx) => { - const fresh = event.reason === "startup" || event.reason === "new"; - void enrichStepModelEffort(ctx.model, ctx, pi, fresh); + pi.on("session_start", (_event, ctx) => { + void enrichStepModelEffort(ctx.model, ctx, pi); }); pi.on("model_select", (event, ctx) => { - void enrichStepModelEffort(event.model, ctx, pi, event.source !== "restore"); + void enrichStepModelEffort(event.model, ctx, pi); }); // Declarative plugins (including StepPage) contribute MCP servers. The // bridge is loaded as part of the Step product extension so ordinary Pi diff --git a/packages/coding-agent/test/step-thinking-preferences.test.ts b/packages/coding-agent/test/step-thinking-preferences.test.ts new file mode 100644 index 00000000..e54819a6 --- /dev/null +++ b/packages/coding-agent/test/step-thinking-preferences.test.ts @@ -0,0 +1,201 @@ +import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { ThinkingLevel } from "@step-harness/agent-core"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { parseArgs } from "../src/cli/args.ts"; +import type { AgentSession } from "../src/core/agent-session.ts"; +import { AuthStorage } from "../src/core/auth-storage.ts"; +import type { ExtensionAPI, ExtensionContext } from "../src/core/extensions/types.ts"; +import { createAgentSession } from "../src/core/sdk.ts"; +import { SessionManager } from "../src/core/session-manager.ts"; +import { SettingsManager } from "../src/core/settings-manager.ts"; +import { createStepExtension } from "../src/features/step.ts"; +import { stepThinkingLevelMap } from "../src/features/step-provider/index.ts"; +import { createStepSettingsManager } from "../src/step/settings-manager.ts"; +import { createInMemoryModelRegistry, getModelRuntime } from "./model-runtime-test-utils.ts"; +import { createTestResourceLoader, stepModel } from "./utilities.ts"; + +const roots: string[] = []; +const sessions: AgentSession[] = []; +function fixture() { + const root = mkdtempSync(join(tmpdir(), "step-thinking-preferences-")); + roots.push(root); + const cwd = join(root, "project"); + const agentDir = join(root, "agent"); + mkdirSync(cwd, { recursive: true }); + mkdirSync(agentDir, { recursive: true }); + return { root, cwd, agentDir }; +} +async function makeSession(f: ReturnType, settings: SettingsManager, thinkingLevel?: ThinkingLevel) { + const registry = await createInMemoryModelRegistry(AuthStorage.inMemory()); + const result = await createAgentSession({ + cwd: f.cwd, + agentDir: f.agentDir, + settingsManager: settings, + sessionManager: SessionManager.inMemory(f.cwd), + resourceLoader: createTestResourceLoader(), + modelRuntime: getModelRuntime(registry), + model: stepModel({ thinkingLevelMap: stepThinkingLevelMap(["low", "medium", "high"]) }), + thinkingLevel, + noTools: "all", + }); + sessions.push(result.session); + return result.session; +} +function stepEffortHooks(session: AgentSession) { + const on = vi.fn(); + createStepExtension({ permission: { env: {} } })({ + registerProvider: vi.fn(), + registerCommand: vi.fn(), + sendUserMessage: vi.fn(), + on, + setThinkingLevel: (level: ThinkingLevel) => session.setThinkingLevel(level), + getThinkingLevel: () => session.thinkingLevel, + } as unknown as ExtensionAPI); + const ctx = { + get model() { + return session.model; + }, + get thinkingLevel() { + return session.thinkingLevel; + }, + modelRegistry: { + getApiKeyAndHeaders: async () => ({ ok: true, apiKey: "test-key" }), + }, + } as unknown as ExtensionContext; + return { + async start(reason = "startup") { + // The first registered session_start handler is the real effort enrichment hook. + await on.mock.calls.find(([name]) => name === "session_start")![1]({ type: "session_start", reason }, ctx); + }, + async select() { + await on.mock.calls.find(([name]) => name === "model_select")![1]( + { type: "model_select", model: session.model, source: "set" }, + ctx, + ); + }, + }; +} +afterEach(() => { + vi.restoreAllMocks(); + for (const session of sessions.splice(0)) session.dispose(); + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); +}); + +describe("Step thinking preferences", () => { + it("CLI parsing and the core SDK honor --thinking medium before the Step hook", async () => { + const f = fixture(); + const parsed = parseArgs(["--thinking", "medium"]); + expect(parsed.thinking).toBe("medium"); + const session = await makeSession(f, SettingsManager.inMemory({ defaultThinkingLevel: "high" }), parsed.thinking); + expect(session.thinkingLevel).toBe("medium"); + }); + + it("Step startup must preserve the explicit --thinking medium option", async () => { + const f = fixture(); + const parsed = parseArgs(["--thinking", "medium"]); + const session = await makeSession(f, SettingsManager.inMemory({ defaultThinkingLevel: "high" }), parsed.thinking); + expect(session.thinkingLevel).toBe("medium"); + await stepEffortHooks(session).start(); + expect(session.thinkingLevel).toBe("medium"); + }); + + it("a saved default survives disk roundtrip and must still apply after startup", async () => { + const f = fixture(); + const saved = createStepSettingsManager(f.cwd, f.agentDir); + saved.setDefaultThinkingLevel("medium"); + await saved.flush(); + const reloaded = createStepSettingsManager(f.cwd, f.agentDir); + expect(reloaded.getDefaultThinkingLevel()).toBe("medium"); + const session = await makeSession(f, reloaded); + expect(session.thinkingLevel).toBe("medium"); + await stepEffortHooks(session).start(); + expect(reloaded.getDefaultThinkingLevel()).toBe("medium"); + expect(session.thinkingLevel).toBe("medium"); + }); + + it("Step startup must preserve a saved per-model thinking preference", async () => { + const f = fixture(); + const saved = createStepSettingsManager(f.cwd, f.agentDir); + saved.setDefaultThinkingLevel("high"); + saved.setModelThinkingLevel("step", "step-5-preview", "low"); + await saved.flush(); + const reloaded = createStepSettingsManager(f.cwd, f.agentDir); + const session = await makeSession(f, reloaded); + expect(session.thinkingLevel).toBe("low"); + await stepEffortHooks(session).start(); + expect(session.thinkingLevel).toBe("low"); + }); + + it("model_select must preserve the effort already resolved by the session", async () => { + const f = fixture(); + const session = await makeSession(f, SettingsManager.inMemory(), "medium"); + await stepEffortHooks(session).select(); + expect(session.thinkingLevel).toBe("medium"); + }); + + it("an explicit resume lifecycle does not trigger the override", async () => { + const f = fixture(); + const session = await makeSession(f, SettingsManager.inMemory(), "medium"); + await stepEffortHooks(session).start("resume"); + expect(session.thinkingLevel).toBe("medium"); + }); + + it("preserves a newer user choice while capability discovery is pending", async () => { + const f = fixture(); + const session = await makeSession(f, SettingsManager.inMemory(), "medium"); + const model = session.model!; + delete model.thinkingLevelMap; + let respond!: (response: Response) => void; + const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementation( + () => + new Promise((resolve) => { + respond = resolve; + }), + ); + await stepEffortHooks(session).start(); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledOnce()); + session.setThinkingLevel("low"); + respond(new Response(JSON.stringify({ reasoning_effort_support_list: ["low", "medium", "high"] }))); + await vi.waitFor(() => expect(model.thinkingLevelMap).toBeDefined()); + expect(session.thinkingLevel).toBe("low"); + }); + + it("does not change the active model's effort when an earlier model's discovery finishes", async () => { + const f = fixture(); + const session = await makeSession(f, SettingsManager.inMemory(), "medium"); + const previousModel = session.model!; + delete previousModel.thinkingLevelMap; + let respond!: (response: Response) => void; + const fetchMock = vi.spyOn(globalThis, "fetch").mockImplementation( + () => + new Promise((resolve) => { + respond = resolve; + }), + ); + await stepEffortHooks(session).start(); + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledOnce()); + session.agent.state.model = stepModel({ + id: "other-model", + thinkingLevelMap: stepThinkingLevelMap(["low", "medium"]), + }); + session.setThinkingLevel("low"); + respond(new Response(JSON.stringify({ reasoning_effort_support_list: ["high"] }))); + await vi.waitFor(() => expect(previousModel.thinkingLevelMap).toBeDefined()); + expect(session.thinkingLevel).toBe("low"); + }); + + it("clamps only unsupported choices after discovering the active model's capabilities", async () => { + const f = fixture(); + const session = await makeSession(f, SettingsManager.inMemory(), "medium"); + const model = session.model!; + delete model.thinkingLevelMap; + vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({ reasoning_effort_support_list: ["low", "high"] })), + ); + await stepEffortHooks(session).start(); + await vi.waitFor(() => expect(model.thinkingLevelMap).toBeDefined()); + expect(session.thinkingLevel).toBe("high"); + }); +}); From 7b94ebea504d7edad3f249ec06ac0ebf6fbd45c7 Mon Sep 17 00:00:00 2001 From: longyongshen Date: Mon, 28 Sep 2026 17:50:28 +0800 Subject: [PATCH 2/2] fix: match the still-active Step model by identity, drop stepHighestEffort Review follow-up on the previous commit. The stale-discovery guard compared model objects by reference, while the session emits model_select through modelsAreEqual. When a provider is registered or unregistered, _refreshCurrentModelFromRegistry swaps in an equal but new model object, and the reference check then skipped a clamp that should run. Use modelsAreEqual so the guard agrees with the session. stepHighestEffort was referenced only by its own test, and its doc comment ("used as the default level for a model") describes the default this branch removes. Delete it together with that test case. --- packages/coding-agent/src/features/step.ts | 4 ++-- packages/coding-agent/test/step-provider.test.ts | 7 ------- packages/providers/src/step-provider/index.ts | 10 ---------- 3 files changed, 2 insertions(+), 19 deletions(-) diff --git a/packages/coding-agent/src/features/step.ts b/packages/coding-agent/src/features/step.ts index 4c3ef1c1..7a3a4357 100644 --- a/packages/coding-agent/src/features/step.ts +++ b/packages/coding-agent/src/features/step.ts @@ -1,5 +1,5 @@ import process from "node:process"; -import { type Api, clampThinkingLevel, type Model } from "@step-harness/providers"; +import { type Api, clampThinkingLevel, type Model, modelsAreEqual } from "@step-harness/providers"; import type { ExtensionAPI, ExtensionCommandContext, @@ -81,7 +81,7 @@ async function enrichStepModelEffort( } // Discovery may finish after a model switch or a user effort change. Only // validate the still-active model, using the latest session preference. - if (ctx.model !== model) return; + if (!modelsAreEqual(ctx.model, model)) return; const selected = pi.getThinkingLevel(); const supported = clampThinkingLevel(model, selected); if (supported !== selected) pi.setThinkingLevel(supported); diff --git a/packages/coding-agent/test/step-provider.test.ts b/packages/coding-agent/test/step-provider.test.ts index 322e6346..59814833 100644 --- a/packages/coding-agent/test/step-provider.test.ts +++ b/packages/coding-agent/test/step-provider.test.ts @@ -22,7 +22,6 @@ import { STEP_PROVIDER_ID, STEP_STATIC_REFRESH_TOKEN, startStepCallbackServer, - stepHighestEffort, stepModelsDetailBaseUrl, stepOpenAiBaseUrl, stepProviderInlineExtension, @@ -674,12 +673,6 @@ describe("Step dynamic model discovery", () => { }); }); - it("reports the highest supported effort", () => { - expect(stepHighestEffort(["low", "medium", "high"])).toBe("high"); - expect(stepHighestEffort(["low", "xhigh", "medium"])).toBe("xhigh"); - expect(stepHighestEffort([])).toBeUndefined(); - }); - it("fetches a model's supported efforts from /v1/models/{id}", async () => { let requestUrl: string | undefined; let authorization: string | null | undefined; diff --git a/packages/providers/src/step-provider/index.ts b/packages/providers/src/step-provider/index.ts index c814a2ce..d5e622f3 100644 --- a/packages/providers/src/step-provider/index.ts +++ b/packages/providers/src/step-provider/index.ts @@ -723,16 +723,6 @@ export function stepThinkingLevelMap(efforts: readonly string[]): ThinkingLevelM return map; } -/** Highest supported reasoning effort (used as the default level for a model). */ -export function stepHighestEffort(efforts: readonly string[]): ThinkingLevel | undefined { - const supported = new Set(efforts.map((effort) => effort.trim().toLowerCase())); - for (let index = STEP_EFFORT_LEVELS.length - 1; index >= 0; index -= 1) { - const level = STEP_EFFORT_LEVELS[index]!; - if (supported.has(level)) return level; - } - return undefined; -} - export interface FetchStepModelEffortsInput { /** OpenAI-style base (`.../v1`); the request targets `{baseUrl}/models/{modelId}`. */ readonly baseUrl: string;