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