From 828c5edf623c822793923d92d4d3525b4d7bb92f Mon Sep 17 00:00:00 2001 From: ace-pm Date: Wed, 15 Apr 2026 10:12:32 +0200 Subject: [PATCH] fix(cli): don't persist --model override in print mode (#4251) `gsd -p --model X "msg"` was silently overwriting defaultProvider/ defaultModel in settings.json. One-shot verification runs must use the model for that invocation only. Adds an AgentSessionConfig.persistModelChanges flag (default true so interactive behavior is unchanged), forwards it through createAgentSession, and sets it false in main.ts when !isInteractive and in src/cli.ts print mode. The gsd wrapper also skips validateConfiguredModel when --model is explicitly passed, so a CLI-provided model can't trigger a fallback repair that writes the wrong default back. Three settings.json write sinks audited: agent-session._applyModelChange (gated on flag), model-selector.ts (interactive only, unreachable in print), startup-model-validation (gated by !cliFlags.model in print). Regression: 8 source-assertion tests in agent-session-print-mode-persist.test.ts. --- .../agent-session-print-mode-persist.test.ts | 137 ++++++++++++++++++ .../pi-coding-agent/src/core/agent-session.ts | 12 +- packages/pi-coding-agent/src/core/sdk.ts | 5 + packages/pi-coding-agent/src/main.ts | 6 + src/cli.ts | 25 +++- 5 files changed, 177 insertions(+), 8 deletions(-) create mode 100644 packages/pi-coding-agent/src/core/agent-session-print-mode-persist.test.ts diff --git a/packages/pi-coding-agent/src/core/agent-session-print-mode-persist.test.ts b/packages/pi-coding-agent/src/core/agent-session-print-mode-persist.test.ts new file mode 100644 index 000000000..2c1000a9a --- /dev/null +++ b/packages/pi-coding-agent/src/core/agent-session-print-mode-persist.test.ts @@ -0,0 +1,137 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** + * Regression #4251: `gsd -p --model / "msg"` must never mutate + * the persisted defaultProvider/defaultModel in settings.json. The one-shot + * print invocation used to verify a provider (e.g. Bearer-auth smoke test) + * was silently overwriting the global default. + * + * Fix: thread a `persistModelChanges` flag from main.ts (false when not + * interactive) through CreateAgentSessionOptions → AgentSessionConfig → + * AgentSession, and gate `_applyModelChange`'s call to + * setDefaultModelAndProvider on it. + */ + +const repoRoot = process.cwd(); +const agentSessionSource = readFileSync( + join(repoRoot, "packages/pi-coding-agent/src/core/agent-session.ts"), + "utf-8", +); +const sdkSource = readFileSync( + join(repoRoot, "packages/pi-coding-agent/src/core/sdk.ts"), + "utf-8", +); +const mainSource = readFileSync( + join(repoRoot, "packages/pi-coding-agent/src/main.ts"), + "utf-8", +); +const gsdCliSource = readFileSync( + join(repoRoot, "src/cli.ts"), + "utf-8", +); + +test("AgentSessionConfig exposes persistModelChanges flag (#4251)", () => { + const configStart = agentSessionSource.indexOf("export interface AgentSessionConfig"); + assert.ok(configStart >= 0, "missing AgentSessionConfig"); + const configEnd = agentSessionSource.indexOf("\n}", configStart); + const configBlock = agentSessionSource.slice(configStart, configEnd); + assert.ok( + configBlock.includes("persistModelChanges?: boolean"), + "AgentSessionConfig should declare optional persistModelChanges flag", + ); +}); + +test("AgentSession stores persistModelChanges and defaults it to true (#4251)", () => { + assert.ok( + agentSessionSource.includes("private _persistModelChanges: boolean"), + "AgentSession should store _persistModelChanges", + ); + assert.ok( + agentSessionSource.includes("this._persistModelChanges = config.persistModelChanges ?? true"), + "constructor must default persistModelChanges to true so interactive behavior is preserved", + ); +}); + +test("_applyModelChange gates settings persistence on _persistModelChanges (#4251)", () => { + const start = agentSessionSource.indexOf("private async _applyModelChange("); + assert.ok(start >= 0, "missing _applyModelChange"); + const window = agentSessionSource.slice(start, start + 1500); + assert.ok( + window.includes("options?.persist !== false && this._persistModelChanges"), + "_applyModelChange must check _persistModelChanges before writing settings", + ); +}); + +test("CreateAgentSessionOptions forwards persistModelChanges to AgentSession (#4251)", () => { + const optionsStart = sdkSource.indexOf("export interface CreateAgentSessionOptions"); + assert.ok(optionsStart >= 0, "missing CreateAgentSessionOptions"); + const optionsEnd = sdkSource.indexOf("\n}", optionsStart); + const optionsBlock = sdkSource.slice(optionsStart, optionsEnd); + assert.ok( + optionsBlock.includes("persistModelChanges?: boolean"), + "CreateAgentSessionOptions should expose persistModelChanges", + ); + assert.ok( + sdkSource.includes("persistModelChanges: options.persistModelChanges"), + "createAgentSession must forward options.persistModelChanges into AgentSessionConfig", + ); +}); + +test("main.ts disables model-change persistence in one-shot / print mode (#4251)", () => { + assert.ok( + mainSource.includes("sessionOptions.persistModelChanges = false"), + "main.ts should set persistModelChanges=false when not interactive", + ); + const gateIdx = mainSource.indexOf("sessionOptions.persistModelChanges = false"); + const guardIdx = mainSource.lastIndexOf("if (!isInteractive)", gateIdx); + assert.ok( + guardIdx >= 0 && guardIdx < gateIdx, + "persistModelChanges=false must be guarded by !isInteractive so interactive mode still persists user model choice", + ); +}); + +test("gsd src/cli.ts print-mode createAgentSession passes persistModelChanges: false (#4251)", () => { + const printGuardIdx = gsdCliSource.indexOf("if (isPrintMode)"); + assert.ok(printGuardIdx >= 0, "missing isPrintMode branch in src/cli.ts"); + const createIdx = gsdCliSource.indexOf("createAgentSession({", printGuardIdx); + assert.ok(createIdx >= 0, "missing createAgentSession call in print-mode branch"); + const createBlock = gsdCliSource.slice(createIdx, createIdx + 800); + assert.ok( + createBlock.includes("persistModelChanges: false"), + "print-mode createAgentSession must pass persistModelChanges: false so --model overrides cannot mutate settings.json", + ); +}); + +test("gsd src/cli.ts print-mode --model override calls setModel with persist: false (#4251)", () => { + const printGuardIdx = gsdCliSource.indexOf("if (isPrintMode)"); + const overrideIdx = gsdCliSource.indexOf("if (cliFlags.model)", printGuardIdx); + assert.ok(overrideIdx >= 0, "missing --model override block in print-mode branch"); + const overrideBlock = gsdCliSource.slice(overrideIdx, overrideIdx + 500); + assert.ok( + overrideBlock.includes("session.setModel(match, { persist: false })"), + "print-mode --model override must pass { persist: false } explicitly so the intent is visible at the call site", + ); +}); + +test("gsd src/cli.ts print-mode skips validateConfiguredModel when --model is set (#4251)", () => { + const printGuardIdx = gsdCliSource.indexOf("if (isPrintMode)"); + const validateIdx = gsdCliSource.indexOf("validateConfiguredModel(", printGuardIdx); + assert.ok(validateIdx >= 0, "missing validateConfiguredModel call in print-mode branch"); + // Walk backward to find the nearest enclosing `if (!cliFlags.model)` guard. + const guardIdx = gsdCliSource.lastIndexOf("if (!cliFlags.model)", validateIdx); + assert.ok( + guardIdx >= 0 && guardIdx > printGuardIdx, + "validateConfiguredModel must be guarded by `if (!cliFlags.model)` in print mode so a CLI-provided model never triggers fallback repair that overwrites settings.json", + ); + // reapplyValidatedModelOnFallback must be inside the same guard block. + const reapplyIdx = gsdCliSource.indexOf("reapplyValidatedModelOnFallback(", validateIdx); + assert.ok(reapplyIdx >= 0, "missing reapplyValidatedModelOnFallback call"); + const blockEnd = gsdCliSource.indexOf("\n }\n", guardIdx); + assert.ok( + reapplyIdx < blockEnd, + "reapplyValidatedModelOnFallback must be inside the same `if (!cliFlags.model)` block as validateConfiguredModel", + ); +}); diff --git a/packages/pi-coding-agent/src/core/agent-session.ts b/packages/pi-coding-agent/src/core/agent-session.ts index f5d6c3c35..8cbc430ae 100644 --- a/packages/pi-coding-agent/src/core/agent-session.ts +++ b/packages/pi-coding-agent/src/core/agent-session.ts @@ -169,6 +169,10 @@ export interface AgentSessionConfig { /** Optional: check if the claude-code CLI provider is ready (installed + authed). * Passed through to RetryHandler for third-party block recovery (#3772). */ isClaudeCodeReady?: () => boolean; + /** When false, model changes (via setModel/cycleModel/extension setModel) do NOT + * write defaultProvider/defaultModel back to settings.json. Used by print/one-shot + * mode so that `gsd -p --model X "msg"` never mutates the persisted default (#4251). */ + persistModelChanges?: boolean; } export interface ExtensionBindings { @@ -299,10 +303,16 @@ export class AgentSession { // Base system prompt (without extension appends) - used to apply fresh appends each turn private _baseSystemPrompt = ""; + // Whether model changes should write defaultProvider/defaultModel to settings.json. + // Set to false for one-shot/print mode so `gsd -p --model X` never mutates the + // persisted default (#4251). Default true preserves interactive behavior. + private _persistModelChanges: boolean; + constructor(config: AgentSessionConfig) { this.agent = config.agent; this.sessionManager = config.sessionManager; this.settingsManager = config.settingsManager; + this._persistModelChanges = config.persistModelChanges ?? true; this._scopedModels = config.scopedModels ?? []; this._resourceLoader = config.resourceLoader; this._customTools = config.customTools ?? []; @@ -1654,7 +1664,7 @@ export class AgentSession { this._retryHandler.abortRetry(); this.agent.setModel(model); this.sessionManager.appendModelChange(model.provider, model.id); - if (options?.persist !== false) { + if (options?.persist !== false && this._persistModelChanges) { this.settingsManager.setDefaultModelAndProvider(model.provider, model.id); } this.setThinkingLevel(thinkingLevel); diff --git a/packages/pi-coding-agent/src/core/sdk.ts b/packages/pi-coding-agent/src/core/sdk.ts index 1558b10b2..75eb11c4f 100644 --- a/packages/pi-coding-agent/src/core/sdk.ts +++ b/packages/pi-coding-agent/src/core/sdk.ts @@ -99,6 +99,10 @@ export interface CreateAgentSessionOptions { /** Optional: check if the claude-code CLI provider is ready (installed + authed). * Passed to RetryHandler for third-party block recovery (#3772). */ isClaudeCodeReady?: () => boolean; + /** When false, model changes do NOT write defaultProvider/defaultModel back to + * settings.json. main.ts sets this to false for print/one-shot mode so + * `gsd -p --model X "msg"` cannot mutate the persisted default (#4251). */ + persistModelChanges?: boolean; } /** Result from createAgentSession */ @@ -491,6 +495,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} initialActiveToolNames, extensionRunnerRef, isClaudeCodeReady: options.isClaudeCodeReady, + persistModelChanges: options.persistModelChanges, }); const extensionsResult = resourceLoader.getExtensions(); diff --git a/packages/pi-coding-agent/src/main.ts b/packages/pi-coding-agent/src/main.ts index 4416043cc..bc1acc879 100644 --- a/packages/pi-coding-agent/src/main.ts +++ b/packages/pi-coding-agent/src/main.ts @@ -569,6 +569,12 @@ export async function main(args: string[]) { sessionOptions.authStorage = authStorage; sessionOptions.modelRegistry = modelRegistry; sessionOptions.resourceLoader = resourceLoader; + // One-shot/print mode must never mutate the persisted defaultProvider/defaultModel: + // a verification run like `gsd -p --model longcat/X "reply ok"` should use model X + // for that invocation only, not silently change the global default (#4251). + if (!isInteractive) { + sessionOptions.persistModelChanges = false; + } // Handle CLI --api-key as runtime override (not persisted) if (parsed.apiKey) { diff --git a/src/cli.ts b/src/cli.ts index 7353e780a..61ee32b0c 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -552,6 +552,11 @@ if (isPrintMode) { await resourceLoader.reload() markStartup('resourceLoader.reload') + // Print mode is a one-shot invocation. The --model flag is a transient + // override (e.g. verification smoke tests like `gsd -p --model longcat/X "reply ok"`) + // and MUST NOT mutate the persisted defaultProvider/defaultModel in settings.json (#4251). + // We disable persistence at session construction so every downstream path + // (setModel override, fallback reapply, validation repair) is gated in one place. const { session, extensionsResult, modelFallbackMessage } = await createAgentSession({ authStorage, modelRegistry, @@ -559,24 +564,30 @@ if (isPrintMode) { sessionManager, resourceLoader, isClaudeCodeReady: () => modelRegistry.isProviderRequestReady('claude-code'), + persistModelChanges: false, }) markStartup('createAgentSession') - // Validate configured model AFTER extensions have registered their models (#2626). - // Before this, extension-provided models (e.g. claude-code/*) were not yet in the - // registry, causing the user's valid choice to be silently overwritten. - validateConfiguredModel(modelRegistry, settingsManager) - await reapplyValidatedModelOnFallback(session, modelRegistry, settingsManager, modelFallbackMessage) + // In print mode we still repair a genuinely stale default so the session has a + // usable model, BUT when the caller explicitly passed --model we skip validation + // entirely — the CLI already said which model to use, and repairing the default + // would overwrite settings.json with a fallback the user didn't ask for (#4251). + if (!cliFlags.model) { + validateConfiguredModel(modelRegistry, settingsManager) + await reapplyValidatedModelOnFallback(session, modelRegistry, settingsManager, modelFallbackMessage) + } printExtensionErrors(extensionsResult.errors) - // Apply --model override if specified + // Apply --model override if specified. persist: false is redundant given + // persistModelChanges above, but we pass it explicitly so the intent is + // visible at the call site and survives future refactors. if (cliFlags.model) { const available = modelRegistry.getAvailable() const match = available.find((m) => m.id === cliFlags.model) || available.find((m) => `${m.provider}/${m.id}` === cliFlags.model) if (match) { - session.setModel(match) + session.setModel(match, { persist: false }) } }