Merge pull request #3537 from jeremymcs/fix/model-fallback-race
fix(pi-coding-agent): resolve model fallback race that ignores configured provider
This commit is contained in:
commit
8eba02f59f
4 changed files with 178 additions and 5 deletions
|
|
@ -13,7 +13,7 @@ import type { ModelRegistry } from "./model-registry.js";
|
|||
/** Default model IDs for each known provider */
|
||||
const defaultModelPerProvider: Record<KnownProvider, string> = {
|
||||
"amazon-bedrock": "us.anthropic.claude-opus-4-6-v1",
|
||||
anthropic: "claude-opus-4-6[1m]",
|
||||
anthropic: "claude-opus-4-6",
|
||||
"anthropic-vertex": "claude-sonnet-4-6",
|
||||
openai: "gpt-5.4",
|
||||
"azure-openai-responses": "gpt-5.2",
|
||||
|
|
@ -24,7 +24,7 @@ const defaultModelPerProvider: Record<KnownProvider, string> = {
|
|||
"google-vertex": "gemini-3-pro-preview",
|
||||
"github-copilot": "gpt-4o",
|
||||
openrouter: "openai/gpt-5.1-codex",
|
||||
"vercel-ai-gateway": "anthropic/claude-opus-4-6[1m]",
|
||||
"vercel-ai-gateway": "anthropic/claude-opus-4-6",
|
||||
xai: "grok-4-fast-non-reasoning",
|
||||
groq: "openai/gpt-oss-120b",
|
||||
cerebras: "zai-glm-4.6",
|
||||
|
|
@ -507,7 +507,7 @@ export async function findInitialModel(options: {
|
|||
const found = modelRegistry.find(defaultProvider, defaultModelId);
|
||||
if (found) {
|
||||
// Check if the provider's recommended default is a higher-capability variant
|
||||
// of the saved model (e.g. saved "claude-opus-4-6" vs recommended "claude-opus-4-6[1m]").
|
||||
// of the saved model (e.g. saved "claude-opus-4-6" vs recommended "claude-opus-4-6-extended").
|
||||
// If so, prefer the recommended variant to avoid using a smaller context window (#1125).
|
||||
const recommendedId = defaultModelPerProvider[defaultProvider as KnownProvider];
|
||||
if (recommendedId && recommendedId !== defaultModelId && recommendedId.startsWith(defaultModelId)) {
|
||||
|
|
|
|||
|
|
@ -214,6 +214,17 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
|
|||
}
|
||||
}
|
||||
|
||||
// Flush extension provider registrations so extension-provided models (e.g. claude-code/*)
|
||||
// are available in the registry before model resolution. Without this, findInitialModel()
|
||||
// cannot find extension models and falls back to built-in providers (#3534).
|
||||
const extensionsForModelResolution = resourceLoader.getExtensions();
|
||||
for (const { name, config } of extensionsForModelResolution.runtime.pendingProviderRegistrations) {
|
||||
modelRegistry.registerProvider(name, config);
|
||||
}
|
||||
// Note: we do NOT clear pendingProviderRegistrations here — bindCore() will iterate
|
||||
// an empty array harmlessly, and clearing here would require the runtime to track
|
||||
// whether the flush already happened.
|
||||
|
||||
// If still no model, use findInitialModel (checks settings default, then provider defaults)
|
||||
if (!model) {
|
||||
const result = await findInitialModel({
|
||||
|
|
|
|||
42
src/cli.ts
42
src/cli.ts
|
|
@ -453,7 +453,7 @@ if (isPrintMode) {
|
|||
await resourceLoader.reload()
|
||||
markStartup('resourceLoader.reload')
|
||||
|
||||
const { session, extensionsResult } = await createAgentSession({
|
||||
const { session, extensionsResult, modelFallbackMessage } = await createAgentSession({
|
||||
authStorage,
|
||||
modelRegistry,
|
||||
settingsManager,
|
||||
|
|
@ -467,6 +467,25 @@ if (isPrintMode) {
|
|||
// registry, causing the user's valid choice to be silently overwritten.
|
||||
validateConfiguredModel(modelRegistry, settingsManager)
|
||||
|
||||
// Re-apply the validated model to the session only when findInitialModel() used a
|
||||
// fallback (not when restoring an existing session's model). This prevents silently
|
||||
// overriding the persisted model of resumed conversations (#3534).
|
||||
if (modelFallbackMessage) {
|
||||
const validatedProvider = settingsManager.getDefaultProvider()
|
||||
const validatedModelId = settingsManager.getDefaultModel()
|
||||
if (validatedProvider && validatedModelId) {
|
||||
const correctModel = modelRegistry.getAvailable()
|
||||
.find((m) => m.provider === validatedProvider && m.id === validatedModelId)
|
||||
if (correctModel) {
|
||||
try {
|
||||
await session.setModel(correctModel)
|
||||
} catch {
|
||||
// Provider not ready — leave session on its current model
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (extensionsResult.errors.length > 0) {
|
||||
for (const err of extensionsResult.errors) {
|
||||
// Downgrade conflicts with built-in tools to warnings (#1347)
|
||||
|
|
@ -606,7 +625,7 @@ const resourceLoadPromise = resourceLoader.reload()
|
|||
await resourceLoadPromise
|
||||
markStartup('resourceLoader.reload')
|
||||
|
||||
const { session, extensionsResult } = await createAgentSession({
|
||||
const { session, extensionsResult, modelFallbackMessage: interactiveFallbackMsg } = await createAgentSession({
|
||||
authStorage,
|
||||
modelRegistry,
|
||||
settingsManager,
|
||||
|
|
@ -620,6 +639,25 @@ markStartup('createAgentSession')
|
|||
// registry, causing the user's valid choice to be silently overwritten.
|
||||
validateConfiguredModel(modelRegistry, settingsManager)
|
||||
|
||||
// Re-apply the validated model to the session only when findInitialModel() used a
|
||||
// fallback (not when restoring an existing session's model). This prevents silently
|
||||
// overriding the persisted model of resumed conversations (#3534).
|
||||
if (interactiveFallbackMsg) {
|
||||
const validatedProvider = settingsManager.getDefaultProvider()
|
||||
const validatedModelId = settingsManager.getDefaultModel()
|
||||
if (validatedProvider && validatedModelId) {
|
||||
const correctModel = modelRegistry.getAvailable()
|
||||
.find((m) => m.provider === validatedProvider && m.id === validatedModelId)
|
||||
if (correctModel) {
|
||||
try {
|
||||
await session.setModel(correctModel)
|
||||
} catch {
|
||||
// Provider not ready — leave session on its current model
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (extensionsResult.errors.length > 0) {
|
||||
for (const err of extensionsResult.errors) {
|
||||
const isConflict = err.error.includes("supersedes") || err.error.includes("conflicts with");
|
||||
|
|
|
|||
124
src/tests/startup-model-validation.test.ts
Normal file
124
src/tests/startup-model-validation.test.ts
Normal file
|
|
@ -0,0 +1,124 @@
|
|||
/**
|
||||
* GSD-2 — Regression tests for startup model validation (#3534)
|
||||
*
|
||||
* Verifies that validateConfiguredModel() correctly handles extension-provided
|
||||
* models and that stale model IDs (e.g. claude-opus-4-6[1m]) trigger fallback.
|
||||
*/
|
||||
|
||||
import { describe, it, beforeEach } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { validateConfiguredModel } from "../startup-model-validation.js";
|
||||
|
||||
// ─── Helpers ────────────────────────────────────────────────────────────────
|
||||
|
||||
interface MockModel {
|
||||
provider: string;
|
||||
id: string;
|
||||
}
|
||||
|
||||
function createMockRegistry(allModels: MockModel[], availableModels?: MockModel[]) {
|
||||
return {
|
||||
getAll: () => allModels,
|
||||
getAvailable: () => availableModels ?? allModels,
|
||||
};
|
||||
}
|
||||
|
||||
function createMockSettings(defaults: { provider?: string; model?: string; thinking?: "off" | "high" }) {
|
||||
let currentProvider = defaults.provider;
|
||||
let currentModel = defaults.model;
|
||||
let currentThinking: "off" | "minimal" | "low" | "medium" | "high" | "xhigh" = defaults.thinking ?? "off";
|
||||
|
||||
return {
|
||||
getDefaultProvider: () => currentProvider,
|
||||
getDefaultModel: () => currentModel,
|
||||
getDefaultThinkingLevel: () => currentThinking,
|
||||
setDefaultModelAndProvider: (provider: string, modelId: string) => {
|
||||
currentProvider = provider;
|
||||
currentModel = modelId;
|
||||
},
|
||||
setDefaultThinkingLevel: (level: "off" | "minimal" | "low" | "medium" | "high" | "xhigh") => {
|
||||
currentThinking = level;
|
||||
},
|
||||
// Expose for assertions
|
||||
get _provider() { return currentProvider; },
|
||||
get _model() { return currentModel; },
|
||||
get _thinking() { return currentThinking; },
|
||||
};
|
||||
}
|
||||
|
||||
// ─── Tests ──────────────────────────────────────────────────────────────────
|
||||
|
||||
describe("validateConfiguredModel — regression #3534", () => {
|
||||
it("preserves valid extension-provided model without overwriting", () => {
|
||||
// Simulate: user configured claude-code/claude-opus-4-6, extension has registered it
|
||||
const registry = createMockRegistry([
|
||||
{ provider: "claude-code", id: "claude-opus-4-6" },
|
||||
{ provider: "google", id: "gemini-2.5-pro" },
|
||||
]);
|
||||
const settings = createMockSettings({ provider: "claude-code", model: "claude-opus-4-6" });
|
||||
|
||||
validateConfiguredModel(registry, settings);
|
||||
|
||||
// Should NOT have changed the settings — the model is valid
|
||||
assert.equal(settings._provider, "claude-code");
|
||||
assert.equal(settings._model, "claude-opus-4-6");
|
||||
});
|
||||
|
||||
it("falls back when configured model ID does not exist in registry", () => {
|
||||
// Simulate: user configured claude-opus-4-6[1m] but registry only has claude-opus-4-6
|
||||
const registry = createMockRegistry([
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
{ provider: "google", id: "gemini-2.5-pro" },
|
||||
]);
|
||||
const settings = createMockSettings({ provider: "anthropic", model: "claude-opus-4-6[1m]" });
|
||||
|
||||
validateConfiguredModel(registry, settings);
|
||||
|
||||
// Should have replaced with a fallback — the [1m] variant doesn't exist
|
||||
assert.notEqual(settings._model, "claude-opus-4-6[1m]");
|
||||
});
|
||||
|
||||
it("does not fall back to google when anthropic models are available", () => {
|
||||
// Simulate: stale setting triggers fallback, anthropic should be preferred over google
|
||||
const registry = createMockRegistry([
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
{ provider: "google", id: "gemini-2.5-pro" },
|
||||
]);
|
||||
const settings = createMockSettings({ provider: "anthropic", model: "nonexistent-model" });
|
||||
|
||||
validateConfiguredModel(registry, settings);
|
||||
|
||||
// Should pick anthropic fallback, not google
|
||||
assert.equal(settings._provider, "anthropic");
|
||||
assert.equal(settings._model, "claude-opus-4-6");
|
||||
});
|
||||
|
||||
it("resets thinking level when model is replaced", () => {
|
||||
const registry = createMockRegistry([
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
]);
|
||||
const settings = createMockSettings({
|
||||
provider: "anthropic",
|
||||
model: "nonexistent-model",
|
||||
thinking: "high",
|
||||
});
|
||||
|
||||
validateConfiguredModel(registry, settings);
|
||||
|
||||
assert.equal(settings._thinking, "off");
|
||||
});
|
||||
|
||||
it("is a no-op when no model is configured at all", () => {
|
||||
const registry = createMockRegistry([
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
{ provider: "google", id: "gemini-2.5-pro" },
|
||||
]);
|
||||
const settings = createMockSettings({ provider: undefined, model: undefined });
|
||||
|
||||
validateConfiguredModel(registry, settings);
|
||||
|
||||
// Should pick a fallback since nothing was configured
|
||||
assert.ok(settings._provider);
|
||||
assert.ok(settings._model);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue