Merge pull request #4072 from jeremymcs/claude/fix-unconfigured-models-uXM3A
Fix unconfigured model selection in defaults and scoped models
This commit is contained in:
commit
9b892a86e5
7 changed files with 181 additions and 14 deletions
85
packages/pi-coding-agent/src/core/model-resolver.test.ts
Normal file
85
packages/pi-coding-agent/src/core/model-resolver.test.ts
Normal file
|
|
@ -0,0 +1,85 @@
|
|||
/**
|
||||
* Regression test for the #unconfigured-models fix: findInitialModel() must
|
||||
* skip the saved default when its provider has no working auth, rather than
|
||||
* returning an unusable model that every selector surface would display as
|
||||
* "current".
|
||||
*/
|
||||
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import { findInitialModel } from "./model-resolver.js";
|
||||
|
||||
function fakeRegistry(options: {
|
||||
models: Array<{ provider: string; id: string }>;
|
||||
readyProviders: Set<string>;
|
||||
}) {
|
||||
const fullModels = options.models.map((m) => ({
|
||||
...m,
|
||||
name: m.id,
|
||||
api: "anthropic-messages",
|
||||
baseUrl: "",
|
||||
reasoning: false,
|
||||
input: ["text"],
|
||||
cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
|
||||
contextWindow: 128_000,
|
||||
maxTokens: 4096,
|
||||
}));
|
||||
const available = fullModels.filter((m) => options.readyProviders.has(m.provider));
|
||||
return {
|
||||
find(provider: string, id: string) {
|
||||
return fullModels.find((m) => m.provider === provider && m.id === id);
|
||||
},
|
||||
getAvailable() {
|
||||
return available;
|
||||
},
|
||||
isProviderRequestReady(provider: string) {
|
||||
return options.readyProviders.has(provider);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
test("findInitialModel skips saved default when provider has no auth", async () => {
|
||||
// User saved xai/grok-4 as default, but XAI_API_KEY is unset so xai is
|
||||
// in the registry but not ready. Previously findInitialModel() step 3
|
||||
// returned xai anyway — now it must fall through to step 4 and pick
|
||||
// an available model.
|
||||
const registry = fakeRegistry({
|
||||
models: [
|
||||
{ provider: "xai", id: "grok-4-fast-non-reasoning" },
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
],
|
||||
readyProviders: new Set(["anthropic"]),
|
||||
});
|
||||
|
||||
const result = await findInitialModel({
|
||||
scopedModels: [],
|
||||
isContinuing: false,
|
||||
defaultProvider: "xai",
|
||||
defaultModelId: "grok-4-fast-non-reasoning",
|
||||
modelRegistry: registry as any,
|
||||
});
|
||||
|
||||
assert.ok(result.model, "a model must be returned");
|
||||
assert.equal(result.model!.provider, "anthropic", "unauth'd saved default must be skipped");
|
||||
});
|
||||
|
||||
test("findInitialModel keeps saved default when provider has auth", async () => {
|
||||
const registry = fakeRegistry({
|
||||
models: [
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
{ provider: "openai", id: "gpt-5.4" },
|
||||
],
|
||||
readyProviders: new Set(["anthropic", "openai"]),
|
||||
});
|
||||
|
||||
const result = await findInitialModel({
|
||||
scopedModels: [],
|
||||
isContinuing: false,
|
||||
defaultProvider: "openai",
|
||||
defaultModelId: "gpt-5.4",
|
||||
modelRegistry: registry as any,
|
||||
});
|
||||
|
||||
assert.equal(result.model?.provider, "openai");
|
||||
assert.equal(result.model?.id, "gpt-5.4");
|
||||
});
|
||||
|
|
@ -120,7 +120,12 @@ export class ModelSelectorComponent extends Container implements Focusable {
|
|||
this.settingsManager = settingsManager;
|
||||
this.modelRegistry = modelRegistry;
|
||||
this.scopedModels = scopedModels;
|
||||
this.scope = scopedModels.length > 0 ? "scoped" : "all";
|
||||
// Only land in "scoped" view when at least one scoped model has working
|
||||
// auth — otherwise the user would see an empty picker (#unconfigured-models).
|
||||
const hasReadyScopedModel = scopedModels.some((scoped) =>
|
||||
modelRegistry.isProviderRequestReady(scoped.model.provider),
|
||||
);
|
||||
this.scope = hasReadyScopedModel ? "scoped" : "all";
|
||||
this.onSelectCallback = onSelect;
|
||||
this.onCancelCallback = onCancel;
|
||||
|
||||
|
|
@ -215,12 +220,16 @@ export class ModelSelectorComponent extends Container implements Focusable {
|
|||
}
|
||||
|
||||
this.allModels = this.sortModelsWithinProvider(models);
|
||||
// Scoped models must also be filtered by provider readiness so users
|
||||
// can't pick a scoped model whose provider has no API key / OAuth.
|
||||
this.scopedModelItems = this.sortModelsWithinProvider(
|
||||
this.scopedModels.map((scoped) => ({
|
||||
provider: scoped.model.provider,
|
||||
id: scoped.model.id,
|
||||
model: scoped.model,
|
||||
})),
|
||||
this.scopedModels
|
||||
.filter((scoped) => this.modelRegistry.isProviderRequestReady(scoped.model.provider))
|
||||
.map((scoped) => ({
|
||||
provider: scoped.model.provider,
|
||||
id: scoped.model.id,
|
||||
model: scoped.model,
|
||||
})),
|
||||
);
|
||||
this.activeModels = this.scope === "scoped" ? this.scopedModelItems : this.allModels;
|
||||
this.filteredModels = this.activeModels;
|
||||
|
|
|
|||
|
|
@ -52,7 +52,12 @@ export async function findExactModelMatch(host: any, searchTerm: string): Promis
|
|||
|
||||
export async function getModelCandidates(host: any): Promise<Model<any>[]> {
|
||||
if (host.session.scopedModels.length > 0) {
|
||||
return host.session.scopedModels.map((scoped: any) => scoped.model);
|
||||
// Filter scoped models by provider auth readiness so callers like
|
||||
// findExactModelMatch can't resolve a scoped-but-unconfigured model.
|
||||
const registry = host.session.modelRegistry;
|
||||
return host.session.scopedModels
|
||||
.filter((scoped: any) => registry.isProviderRequestReady(scoped.model.provider))
|
||||
.map((scoped: any) => scoped.model);
|
||||
}
|
||||
|
||||
host.session.modelRegistry.refresh();
|
||||
|
|
|
|||
|
|
@ -269,16 +269,40 @@ export async function bootstrapAutoSession(
|
|||
//
|
||||
// Precedence:
|
||||
// 1) Explicit session override via /gsd model (this session)
|
||||
// 2) GSD model preferences from PREFERENCES.md
|
||||
// 3) Current session model from settings/session restore
|
||||
// 2) GSD model preferences from PREFERENCES.md (validated against live auth)
|
||||
// 3) Current session model from settings/session restore (if provider ready)
|
||||
//
|
||||
// This preserves #3517 defaults while honoring explicit runtime model
|
||||
// selection for subsequent /gsd runs in the same session.
|
||||
const manualSessionOverride = getSessionModelOverride(ctx.sessionManager.getSessionId());
|
||||
const preferredModel = resolveDefaultSessionModel(ctx.model?.provider);
|
||||
// Validate the preferred model against the live registry + provider auth so
|
||||
// an unconfigured PREFERENCES.md entry (no API key / OAuth) can't become the
|
||||
// start-model snapshot. Without this, every subsequent unit would try to
|
||||
// fall back to an unusable model.
|
||||
let validatedPreferredModel: { provider: string; id: string } | undefined;
|
||||
if (preferredModel) {
|
||||
const { resolveModelId } = await import("./auto-model-selection.js");
|
||||
const available = ctx.modelRegistry.getAvailable();
|
||||
const match = resolveModelId(
|
||||
`${preferredModel.provider}/${preferredModel.id}`,
|
||||
available,
|
||||
ctx.model?.provider,
|
||||
);
|
||||
if (match) {
|
||||
validatedPreferredModel = { provider: match.provider, id: match.id };
|
||||
} else {
|
||||
ctx.ui.notify(
|
||||
`Preferred model ${preferredModel.provider}/${preferredModel.id} from PREFERENCES.md is not configured; falling back to session default.`,
|
||||
"warning",
|
||||
);
|
||||
}
|
||||
}
|
||||
const sessionModelReady =
|
||||
ctx.model && ctx.modelRegistry.isProviderRequestReady(ctx.model.provider);
|
||||
const startModelSnapshot = manualSessionOverride
|
||||
?? preferredModel
|
||||
?? (ctx.model
|
||||
?? validatedPreferredModel
|
||||
?? (sessionModelReady && ctx.model
|
||||
? { provider: ctx.model.provider, id: ctx.model.id }
|
||||
: null);
|
||||
|
||||
|
|
|
|||
|
|
@ -48,3 +48,17 @@ test("bootstrapAutoSession checks manual session override before preferences", (
|
|||
"manual override and preference fallback must be resolved before building startModelSnapshot",
|
||||
);
|
||||
});
|
||||
|
||||
test("bootstrapAutoSession validates preferred model against live registry auth (#unconfigured-models)", () => {
|
||||
// The raw PREFERENCES.md value must be validated against getAvailable()
|
||||
// before being captured as the snapshot, so an unconfigured provider
|
||||
// (no API key / OAuth) can't become autoModeStartModel.
|
||||
const validationIdx = source.indexOf("ctx.modelRegistry.getAvailable()");
|
||||
assert.ok(validationIdx > -1, "auto-start.ts should validate preferred model against getAvailable()");
|
||||
|
||||
const resolveModelIdIdx = source.indexOf("resolveModelId");
|
||||
assert.ok(resolveModelIdIdx > -1, "auto-start.ts should resolve preferred model against the registry");
|
||||
|
||||
const warningIdx = source.indexOf("is not configured; falling back to session default");
|
||||
assert.ok(warningIdx > -1, "auto-start.ts should warn when preferred model is unconfigured");
|
||||
});
|
||||
|
|
|
|||
|
|
@ -17,7 +17,6 @@ interface MinimalModel {
|
|||
}
|
||||
|
||||
interface MinimalModelRegistry {
|
||||
getAll(): MinimalModel[]
|
||||
getAvailable(): MinimalModel[]
|
||||
}
|
||||
|
||||
|
|
@ -48,10 +47,14 @@ export function validateConfiguredModel(
|
|||
): void {
|
||||
const configuredProvider = settingsManager.getDefaultProvider()
|
||||
const configuredModel = settingsManager.getDefaultModel()
|
||||
const allModels = modelRegistry.getAll()
|
||||
const availableModels = modelRegistry.getAvailable()
|
||||
// Check against availableModels (configured + auth'd) rather than getAll()
|
||||
// so a stale default pointing at an unconfigured provider triggers the
|
||||
// fallback. Previously a model present in the registry but missing API
|
||||
// key / OAuth would satisfy configuredExists and survive startup, ending
|
||||
// up as ctx.model even though it couldn't actually be used.
|
||||
const configuredExists = configuredProvider && configuredModel &&
|
||||
allModels.some((m) => m.provider === configuredProvider && m.id === configuredModel)
|
||||
availableModels.some((m) => m.provider === configuredProvider && m.id === configuredModel)
|
||||
|
||||
if (!configuredModel || !configuredExists) {
|
||||
// Model not configured at all, or removed from registry — pick a fallback.
|
||||
|
|
|
|||
|
|
@ -121,4 +121,31 @@ describe("validateConfiguredModel — regression #3534", () => {
|
|||
assert.ok(settings._provider);
|
||||
assert.ok(settings._model);
|
||||
});
|
||||
|
||||
it("falls back when configured model exists in registry but provider has no auth", () => {
|
||||
// Simulate: user configured xai/grok-4 but XAI_API_KEY is unset, so
|
||||
// xai is in getAll() but not getAvailable(). Previously this slipped
|
||||
// through configuredExists and left an unusable default in place.
|
||||
const allModels = [
|
||||
{ provider: "xai", id: "grok-4-fast-non-reasoning" },
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
];
|
||||
const availableModels = [
|
||||
{ provider: "anthropic", id: "claude-opus-4-6" },
|
||||
];
|
||||
const registry = createMockRegistry(allModels, availableModels);
|
||||
const settings = createMockSettings({
|
||||
provider: "xai",
|
||||
model: "grok-4-fast-non-reasoning",
|
||||
thinking: "high",
|
||||
});
|
||||
|
||||
validateConfiguredModel(registry, settings);
|
||||
|
||||
// Should have replaced with an authenticated fallback
|
||||
assert.equal(settings._provider, "anthropic");
|
||||
assert.equal(settings._model, "claude-opus-4-6");
|
||||
// Thinking level resets because the original model was replaced
|
||||
assert.equal(settings._thinking, "off");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue