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.
This commit is contained in:
parent
1fc62582ed
commit
828c5edf62
5 changed files with 177 additions and 8 deletions
|
|
@ -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 <provider>/<id> "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",
|
||||
);
|
||||
});
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
25
src/cli.ts
25
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 })
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue