PR #2959: improve models.json UX — skip redundant apiKey + survive registerProvider()

pr-2959.patch

diff --git a/packages/coding-agent/src/core/model-registry.ts b/packages/coding-agent/src/core/model-registry.ts
index 5a93b438..45ad4f8a 100644
--- a/packages/coding-agent/src/core/model-registry.ts
+++ b/packages/coding-agent/src/core/model-registry.ts
@@ -463,10 +463,7 @@ export class ModelRegistry {
 	}
 
 	private validateConfig(config: ModelsConfig): void {
-		const builtInProviders = new Set<string>(getProviders());
-
 		for (const [providerName, providerConfig] of Object.entries(config.providers)) {
-			const isBuiltIn = builtInProviders.has(providerName);
 			const hasProviderApi = !!providerConfig.api;
 			const models = providerConfig.models ?? [];
 			const hasModelOverrides =
@@ -479,27 +476,34 @@ export class ModelRegistry {
 						`Provider ${providerName}: must specify "baseUrl", "compat", "modelOverrides", or "models".`,
 					);
 				}
-			} else if (!isBuiltIn) {
-				// Non-built-in providers with custom models require endpoint + auth.
+			} else {
+				// Custom models are merged into provider models and require endpoint + auth.
 				if (!providerConfig.baseUrl) {
 					throw new Error(`Provider ${providerName}: "baseUrl" is required when defining custom models.`);
 				}
-				if (!providerConfig.apiKey) {
-					throw new Error(`Provider ${providerName}: "apiKey" is required when defining custom models.`);
+				// apiKey can be omitted when the provider is already authenticated via /login
+				// (credentials are stored in auth.json). At request time, getApiKeyAndHeaders()
+				// calls authStorage.getApiKey() first, so stored credentials are used
+				// automatically even if providerConfig.apiKey is absent.
+				const hasStoredAuth = this.authStorage.hasAuth(providerName);
+				if (!providerConfig.apiKey && !hasStoredAuth) {
+					throw new Error(
+						`Provider ${providerName}: "apiKey" is required when defining custom models. ` +
+							`Set it to an environment variable name, a literal key, or a shell command ("!cmd"). ` +
+							`Alternatively, authenticate via /login (if supported) then reload models — ` +
+							`stored credentials are used automatically.`,
+					);
 				}
 			}
-			// Built-in providers with custom models: baseUrl/apiKey/api are optional,
-			// inherited from built-in models. Auth comes from env vars / auth storage.
 
 			for (const modelDef of models) {
 				const hasModelApi = !!modelDef.api;
 
-				if (!hasProviderApi && !hasModelApi && !isBuiltIn) {
+				if (!hasProviderApi && !hasModelApi) {
 					throw new Error(
 						`Provider ${providerName}, model ${modelDef.id}: no "api" specified. Set at provider or model level.`,
 					);
 				}
-				// For built-in providers, api is optional — inherited from built-in models.
 
 				if (!modelDef.id) throw new Error(`Provider ${providerName}: model missing "id"`);
 				// Validate contextWindow/maxTokens only if provided (they have defaults)
@@ -513,43 +517,27 @@ export class ModelRegistry {
 
 	private parseModels(config: ModelsConfig): Model<Api>[] {
 		const models: Model<Api>[] = [];
-		const builtInProviders = new Set<string>(getProviders());
-
-		// Cache built-in defaults (api, baseUrl) per provider, extracted from first model.
-		const builtInDefaultsCache = new Map<string, { api: string; baseUrl: string }>();
-		const getBuiltInDefaults = (providerName: string): { api: string; baseUrl: string } | undefined => {
-			if (!builtInProviders.has(providerName)) return undefined;
-			if (builtInDefaultsCache.has(providerName)) return builtInDefaultsCache.get(providerName);
-			const builtIn = getModels(providerName as KnownProvider) as Model<Api>[];
-			if (builtIn.length === 0) return undefined;
-			const defaults = { api: builtIn[0].api, baseUrl: builtIn[0].baseUrl };
-			builtInDefaultsCache.set(providerName, defaults);
-			return defaults;
-		};
 
 		for (const [providerName, providerConfig] of Object.entries(config.providers)) {
 			const modelDefs = providerConfig.models ?? [];
 			if (modelDefs.length === 0) continue; // Override-only, no custom models
 
-			const builtInDefaults = getBuiltInDefaults(providerName);
-
 			for (const modelDef of modelDefs) {
-				const api = modelDef.api ?? providerConfig.api ?? builtInDefaults?.api;
+				const api = modelDef.api || providerConfig.api;
 				if (!api) continue;
 
-				const baseUrl = modelDef.baseUrl ?? providerConfig.baseUrl ?? builtInDefaults?.baseUrl;
-				if (!baseUrl) continue;
-
 				const compat = mergeCompat(providerConfig.compat, modelDef.compat);
 				this.storeModelHeaders(providerName, modelDef.id, modelDef.headers);
 
+				// Provider baseUrl is required when custom models are defined.
+				// Individual models can override it with modelDef.baseUrl.
 				const defaultCost = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 };
 				models.push({
 					id: modelDef.id,
 					name: modelDef.name ?? modelDef.id,
 					api: api as Api,
 					provider: providerName,
-					baseUrl,
+					baseUrl: modelDef.baseUrl ?? providerConfig.baseUrl!,
 					reasoning: modelDef.reasoning ?? false,
 					input: (modelDef.input ?? ["text"]) as ("text" | "image")[],
 					cost: modelDef.cost ?? defaultCost,
diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts
index 15c25475..dbd25651 100644
--- a/packages/coding-agent/test/model-registry.test.ts
+++ b/packages/coding-agent/test/model-registry.test.ts
@@ -192,49 +192,6 @@ describe("ModelRegistry", () => {
 	});
 
 	describe("custom models merge behavior", () => {
-		test("built-in provider custom models inherit api and baseUrl without explicit fields", () => {
-			// Built-in providers already have api/baseUrl on every model, and auth
-			// comes from env vars / auth storage. No need to specify them.
-			writeRawModelsJson({
-				openrouter: {
-					models: [
-						{
-							id: "fake-provider/fake-model",
-							name: "Fake model",
-							reasoning: true,
-							input: ["text"],
-						},
-					],
-				},
-			});
-
-			const registry = ModelRegistry.create(authStorage, modelsJsonPath);
-			expect(registry.getError()).toBeUndefined();
-
-			const model = registry.find("openrouter", "fake-provider/fake-model");
-			expect(model).toBeDefined();
-			expect(model?.api).toBe("openai-completions");
-			expect(model?.baseUrl).toBe("https://openrouter.ai/api/v1");
-		});
-
-		test("non-built-in provider custom models still require baseUrl and apiKey", () => {
-			writeRawModelsJson({
-				"my-custom-provider": {
-					models: [
-						{
-							id: "my-model",
-							api: "openai-completions",
-							reasoning: false,
-							input: ["text"],
-						},
-					],
-				},
-			});
-
-			const registry = ModelRegistry.create(authStorage, modelsJsonPath);
-			expect(registry.getError()).toContain("baseUrl");
-		});
-
 		test("custom provider with same name as built-in merges with built-in models", () => {
 			writeModelsJson({
 				anthropic: providerConfig("https://my-proxy.example.com/v1", [{ id: "claude-custom" }]),
@@ -498,6 +455,74 @@ describe("ModelRegistry", () => {
 			expect(anthropicModels.some((m) => m.id.includes("claude"))).toBe(true);
 		});
 
+		test("apiKey can be omitted when provider already has stored credentials", () => {
+			// Simulate the user having logged in via /login (credentials in auth.json)
+			authStorage.set("anthropic", { type: "api_key", key: "sk-stored-key" });
+
+			writeFileSync(
+				modelsJsonPath,
+				JSON.stringify({
+					providers: {
+						anthropic: {
+							baseUrl: "https://proxy.example.com/v1",
+							api: "anthropic-messages",
+							// apiKey intentionally omitted — auth comes from authStorage
+							models: [
+								{
+									id: "claude-custom",
+									name: "Claude Custom",
+									reasoning: false,
+									input: ["text"],
+									cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+									contextWindow: 100000,
+									maxTokens: 8000,
+								},
+							],
+						},
+					},
+				}),
+			);
+
+			const registry = ModelRegistry.create(authStorage, modelsJsonPath);
+			expect(registry.getError()).toBeUndefined();
+			expect(getModelsForProvider(registry, "anthropic").some((m) => m.id === "claude-custom")).toBe(true);
+		});
+
+		test("apiKey missing with no stored credentials produces actionable error", () => {
+			// No stored credentials, no apiKey in models.json
+			writeFileSync(
+				modelsJsonPath,
+				JSON.stringify({
+					providers: {
+						anthropic: {
+							baseUrl: "https://proxy.example.com/v1",
+							api: "anthropic-messages",
+							models: [
+								{
+									id: "claude-custom",
+									name: "Claude Custom",
+									reasoning: false,
+									input: ["text"],
+									cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+									contextWindow: 100000,
+									maxTokens: 8000,
+								},
+							],
+						},
+					},
+				}),
+			);
+
+			const registry = ModelRegistry.create(authStorage, modelsJsonPath);
+			const error = registry.getError();
+			expect(error).toBeDefined();
+			// Error must mention apiKey and all actionable alternatives
+			expect(error).toContain("apiKey");
+			expect(error).toContain("!cmd"); // shell command hint
+			expect(error).toContain("/login"); // stored-auth alternative
+			expect(error).toContain("environment variable"); // env var hint
+		});
+
 		test("removing custom models from models.json keeps built-in provider models", () => {
 			writeModelsJson({
 				anthropic: providerConfig("https://proxy.example.com/v1", [{ id: "claude-custom" }]),