diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index f69223915..9a358b46f 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -52,6 +52,7 @@ import { installPackageAssets, getPackageAssetOwner, hasPackageAssetOwnerInstall import { THINKING_LEVELS, normalizeModelConfig, + isThinkingLevel, normalizeModelId, normalizeRoutingEntry, readSavedModelConfig as readModelRoutingAuthority, @@ -4166,7 +4167,7 @@ function reportProfilesDrops(ctx: ExtensionContext, path: string, drops: Profile } /** Pi's own live-session controls: the ExtensionAPI's setModel/setThinkingLevel. */ -type LiveSession = Pick; +type LiveSession = Pick; /** * Switch the running session to the profile's orchestrator. `settings.json` @@ -4348,19 +4349,58 @@ async function runProfilesPanelAction( const orchestratorEffects = orchestratorEntry !== undefined ? ", set the configured orchestrator entry in settings.json, and attempt to switch this session to that orchestrator model" : ""; - let confirmMessage: string; - if (savedRouting.status !== "valid") { - const modelsPath = sanitizeTerminalText(modelConfigPath(ctx.cwd)); - confirmMessage = `Profile "${result.name}" has agent routing entries, but the current global routing in ${modelsPath} could not be read, so existing routes are not listed. Applying it will replace global routing in ${modelsPath} with this profile's routes, so every existing agent route may be replaced or cleared back to inherit${orchestratorEffects}. Continue?`; + // A profile whose agent routes already match the effective current routing + // and that moves no orchestrator changes nothing: re-selecting the active + // profile or verifying state would otherwise train users to approve a + // dialog without reading it, weakening the guard on the destructive cases + // (issue #1683). Any routing change, any orchestrator change, or an + // unreadable routing authority (the diff above cannot prove a no-op) + // keeps the confirmation exactly as #1349/#1384 defined it. + // `applyOrchestratorSettings` treats an entry without a model as "leave + // settings.json alone", so such an entry is a no-op too, not a change. + // The live session is a fourth surface: applying re-asserts the profile's + // orchestrator on it, so a session already moved to another model or + // thinking level mid-session is a real change the user must approve, + // even when settings.json and the profile agree. + const liveOrchestrator = (() => { + if (ctx.model === undefined || typeof ctx.model.provider !== "string" || typeof ctx.model.id !== "string") return undefined; + let thinking: unknown; + try { thinking = live.getThinkingLevel(); } catch { return undefined; } + return { model: `${ctx.model.provider}/${ctx.model.id}`, thinking: isThinkingLevel(thinking) ? thinking : undefined }; + })(); + const orchestratorUnchanged = (orchestratorEntry === undefined || orchestratorEntry.model === undefined) || (() => { + const current = readOrchestratorSettings(orchestratorSettingsPath()); + // An invalid stored defaultThinkingLevel is dropped from the entry but + // applyOrchestratorSettings would delete the key, so the file would + // change: a no-op cannot be proven and the dialog must stay. + if (current.status === "valid" && "defaultThinkingLevel" in current.value && current.entry?.thinking === undefined) return false; + return current.status === "valid" && current.entry !== undefined + && current.entry.model === orchestratorEntry.model + && current.entry.thinking === orchestratorEntry.thinking + && liveOrchestrator !== undefined + && liveOrchestrator.model === orchestratorEntry.model + && liveOrchestrator.thinking === orchestratorEntry.thinking; + })(); + if (savedRouting.status === "valid" && replacedRoutes.length === 0 && clearedRoutes.length === 0 && addedRoutes.length === 0 && orchestratorUnchanged) { + ctx.ui.notify( + `Profile "${result.name}" already matches the current global routing${orchestratorEntry !== undefined ? " and orchestrator" : ""}; applying it changed nothing.`, + "info", + ); } else { - const changes = [...replacedRoutes, ...clearedRoutes, ...addedRoutes]; - const changeSummary = changes.length > 0 - ? changes.join("; ") - : "its agent routes already match the current global routing"; - confirmMessage = `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}${orchestratorEffects}. Continue?`; + let confirmMessage: string; + if (savedRouting.status !== "valid") { + const modelsPath = sanitizeTerminalText(modelConfigPath(ctx.cwd)); + confirmMessage = `Profile "${result.name}" has agent routing entries, but the current global routing in ${modelsPath} could not be read, so existing routes are not listed. Applying it will replace global routing in ${modelsPath} with this profile's routes, so every existing agent route may be replaced or cleared back to inherit${orchestratorEffects}. Continue?`; + } else { + const changes = [...replacedRoutes, ...clearedRoutes, ...addedRoutes]; + const changeSummary = changes.length > 0 + ? changes.join("; ") + : "its agent routes already match the current global routing"; + confirmMessage = `Profile "${result.name}" has agent routing entries. Applying it will replace global routing in ${sanitizeTerminalText(modelConfigPath(ctx.cwd))} with this profile's routes: ${changeSummary}${orchestratorEffects}. Continue?`; + } + const approved = await ctx.ui.confirm(`Apply profile "${result.name}"?`, confirmMessage); + if (!approved) return file; } - const approved = await ctx.ui.confirm(`Apply profile "${result.name}"?`, confirmMessage); - if (!approved) return file; } // Applying spans three files — the store, models.json, and Pi's global // settings.json — and there is no cross-file rename, so order the writes to diff --git a/tests/gentle-ai.test.ts b/tests/gentle-ai.test.ts index d53c4302f..06b7eceb4 100644 --- a/tests/gentle-ai.test.ts +++ b/tests/gentle-ai.test.ts @@ -17,8 +17,8 @@ import type { import { __testing, applyModelConfig, applyModelConfigAsync, createGentleAiExtension } from "../extensions/gentle-ai.ts"; import { PROFILES_KIND, PROFILES_VERSION, readProfilesFileResult } from "../lib/agent-profiles.ts"; import { readSessionProfileBinding, resetSessionProfileBindingsForTesting } from "../lib/session-profile-binding.ts"; -import type { AgentRoutingEntry } from "../lib/model-routing-authority.ts"; -type LiveSession = Pick; +import type { AgentRoutingEntry, ThinkingLevel } from "../lib/model-routing-authority.ts"; +type LiveSession = Pick; import { PROFILE_PIN_KIND, PROFILE_PIN_VERSION, setProfilePinWorktreeResolverForTesting, writeProfilePinSync } from "../lib/agent-profile-pin.ts"; import { NATIVE_REVIEW_ERROR_CODE, NativeReviewCliError, type NativeReviewCli } from "../lib/native-review-cli.ts"; import { CandidateViewError, type CandidateViewRegistry } from "../lib/review-candidate-view.ts"; @@ -2344,6 +2344,149 @@ test("applying a populated profile asks for confirmation naming the diff and abo assert.deepEqual(fixture.liveSwitches, [], "declined apply must not switch the live session"); }); +test("applying a populated profile whose routes already match skips the confirmation and applies", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + // The profile's agent routes are identical to the effective current routing + // and the profile carries no orchestrator entry, so applying changes nothing: + // the dialog is pure friction and the apply must proceed without it. + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { worker: { model: "openai/alpha" } } }); + const settingsBefore = readFileSync(settingsPath, "utf8"); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 0, "a no-op populated apply must not ask for confirmation"); + assert.ok( + fixture.notifications.some((entry) => entry.severity === "info" && /already matches the current global routing/.test(entry.message)), + `the no-op apply is disclosed with an informational notice: ${JSON.stringify(fixture.notifications)}`, + ); + const store = JSON.parse(readFileSync(storePath, "utf8")); + assert.equal(store.active, "team", "the apply still claims the profile as active"); + assert.deepEqual(JSON.parse(readFileSync(fixture.globalPath, "utf8")), { worker: { model: "openai/alpha" } }, "the routing is unchanged"); + assert.equal(readFileSync(settingsPath, "utf8"), settingsBefore, "settings.json is untouched"); +}); + +test("applying a populated profile whose routes match and whose orchestrator is already set skips the confirmation", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + // writeSettings defaults to nan/deepseek-v4-flash · high; the profile's + // orchestrator entry says the same, and the live session still runs on it, + // so nothing changes anywhere. + writeSettings(); + fixture.setLiveModel("nan", "deepseek-v4-flash", "high"); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { orchestrator: { model: "nan/deepseek-v4-flash", thinking: "high" }, worker: { model: "openai/alpha" } } }); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 0, "an already-active profile must not ask for confirmation"); + assert.deepEqual(fixture.liveSwitches, [], "the no-op apply performs no live switch: the orchestrator is already there"); + assert.equal(JSON.parse(readFileSync(storePath, "utf8")).active, "team"); +}); + +test("applying a populated profile keeps the confirmation when the live session runs a different model", async (t) => { + const { fixture, storePath, writeStore, writeSettings } = profilesStoreFixture(t); + // settings.json and the profile agree on openai/alpha · high, but the live + // session was switched to openai/beta mid-session: applying would move the + // live session, so the no-op skip must not fire and the dialog must stay. + writeSettings({ defaultProvider: "openai", defaultModel: "alpha", defaultThinkingLevel: "high" }); + fixture.setLiveModel("openai", "beta", "high"); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { orchestrator: { model: "openai/alpha", thinking: "high" }, worker: { model: "openai/alpha" } } }); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "a live-session orchestrator move is a real change and must confirm"); + assert.deepEqual(fixture.liveSwitches, [], "declining must not switch the live session"); + assert.notEqual(JSON.parse(readFileSync(storePath, "utf8")).active, "team"); +}); + +test("applying a populated profile keeps the confirmation when settings.json holds an invalid thinking level", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + // The stored defaultThinkingLevel is not a valid level: readOrchestratorSettings + // drops it from the entry, but applyOrchestratorSettings would delete the key + // and rewrite the file, so "unchanged" cannot be proven and the dialog stays. + writeSettings({ defaultThinkingLevel: "banana" }); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { orchestrator: { model: "nan/deepseek-v4-flash" }, worker: { model: "openai/alpha" } } }); + const settingsBefore = readFileSync(settingsPath, "utf8"); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "an invalid stored thinking level cannot prove a no-op"); + assert.equal(readFileSync(settingsPath, "utf8"), settingsBefore, "declining preserves the invalid key byte-identically"); + assert.notEqual(JSON.parse(readFileSync(storePath, "utf8")).active, "team"); +}); + +test("applying a populated profile keeps the confirmation when settings.json is unreadable and the profile has an orchestrator", async (t) => { + const { fixture, storePath, writeStore, settingsPath } = profilesStoreFixture(t); + writeFileSync(settingsPath, "{ not json\n"); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { orchestrator: { model: "nan/glm5.3" }, worker: { model: "openai/alpha" } } }); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "an unreadable settings.json cannot prove an orchestrator no-op"); + assert.notEqual(JSON.parse(readFileSync(storePath, "utf8")).active, "team"); +}); + +test("applying a populated profile with a thinking-only orchestrator entry skips the confirmation", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + // An orchestrator entry without a model is a no-op for the orchestrator: + // applyOrchestratorSettings treats a missing model as "leave settings.json + // alone", so the apply moves nothing and must not confirm (issue #1683). + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { orchestrator: { thinking: "max" }, worker: { model: "openai/alpha" } } }); + const settingsBefore = readFileSync(settingsPath, "utf8"); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 0, "a thinking-only orchestrator entry changes nothing and must not confirm"); + assert.ok( + fixture.notifications.some((entry) => entry.severity === "info" && /already matches the current global routing/.test(entry.message)), + `the no-op apply is disclosed with an informational notice: ${JSON.stringify(fixture.notifications)}`, + ); + assert.equal(readFileSync(settingsPath, "utf8"), settingsBefore, "settings.json is untouched"); + assert.deepEqual(fixture.liveSwitches, [], "a thinking-only orchestrator entry never switches the live session"); + assert.equal(JSON.parse(readFileSync(storePath, "utf8")).active, "team"); +}); + +test("applying a populated profile with matching routes but a different orchestrator still confirms", async (t) => { + const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); + writeSettings(); + mkdirSync(fixture.configHome, { recursive: true }); + writeFileSync(fixture.globalPath, `${JSON.stringify({ worker: { model: "openai/alpha" } }, null, 2)}\n`); + writeStore({ team: { orchestrator: { model: "nan/glm5.3", thinking: "max" }, worker: { model: "openai/alpha" } } }); + + fixture.onConfirm(async () => false); + + applyOnce(fixture); + await fixture.run("gentle:profiles"); + + assert.equal(fixture.confirmCalls.length, 1, "an orchestrator change is a real change and must confirm"); + assert.match(fixture.confirmCalls[0]?.[0] ?? "", /Apply profile/); + assert.notEqual(JSON.parse(readFileSync(storePath, "utf8")).active, "team", "declining leaves the store untouched"); +}); + test("applying a populated profile names materialized-only routes it would clear before asking", async (t) => { const { fixture, storePath, writeStore, writeSettings, settingsPath } = profilesStoreFixture(t); writeSettings(); @@ -3475,7 +3618,7 @@ test("Enter binds the selected profile to the parent session and writes nothing" ui: { notify(message: string, severity: string) { notifications.push({ message, severity }); } }, sessionManager: { getSessionId: () => "session-panel" }, } as unknown as ExtensionContext; - const live = { setModel: async () => true, setThinkingLevel() {} }; + const live = { setModel: async () => true, setThinkingLevel() {}, getThinkingLevel(): ThinkingLevel { return "medium"; } }; const file = readValidProfilesStore(storePath); await __testing.runProfilesPanelAction(ctx, live, storePath, file, { type: "apply", name: "team" }, {}); const binding = readSessionProfileBinding("session-panel"); @@ -3505,7 +3648,7 @@ test("a keeps the legacy global apply semantics", async (t) => { ui: { notify() {}, confirm: async () => true }, sessionManager: { getSessionId: () => "session-panel" }, } as unknown as ExtensionContext; - const live = { setModel: async () => true, setThinkingLevel() {} }; + const live = { setModel: async () => true, setThinkingLevel() {}, getThinkingLevel(): ThinkingLevel { return "medium"; } }; const file = readValidProfilesStore(storePath); await __testing.runProfilesPanelAction(ctx, live, storePath, file, { type: "apply-global", name: "team" }, {}); assert.equal(JSON.parse(readFileSync(storePath, "utf8")).active, "team", "the global store claims the profile"); @@ -3526,7 +3669,7 @@ test("Enter with a winning pin binds the session and never touches the pin layer ui: { notify() {} }, sessionManager: { getSessionId: () => "session-panel" }, } as unknown as ExtensionContext; - const live = { setModel: async () => true, setThinkingLevel() {} }; + const live = { setModel: async () => true, setThinkingLevel() {}, getThinkingLevel(): ThinkingLevel { return "medium"; } }; const file = readValidProfilesStore(storePath); await __testing.runProfilesPanelAction(ctx, live, storePath, file, { type: "apply", name: "team" }, {}); assert.equal(readSessionProfileBinding("session-panel")?.name, "team"); @@ -3546,7 +3689,7 @@ test("Enter without a parent session id fails loud and writes nothing", async (t hasUI: true, ui: { notify(message: string, severity: string) { notifications.push({ message, severity }); } }, } as unknown as ExtensionContext; - const live = { setModel: async () => true, setThinkingLevel() {} }; + const live = { setModel: async () => true, setThinkingLevel() {}, getThinkingLevel(): ThinkingLevel { return "medium"; } }; const file = readValidProfilesStore(storePath); await __testing.runProfilesPanelAction(ctx, live, storePath, file, { type: "apply", name: "team" }, {}); assert.equal(readSessionProfileBinding(undefined), undefined);