fix(coding-agent): make config env references explicit

closes #5095

packages/coding-agent/src/core/extensions/types.ts

@@ -1256,7 +1256,7 @@ export interface ExtensionAPI {
 	 * // Register a new provider with custom models
 	 * pi.registerProvider("my-proxy", {
 	 *   baseUrl: "https://proxy.example.com",
-	 *   apiKey: "PROXY_API_KEY",
+	 *   apiKey: "$PROXY_API_KEY",
 	 *   api: "anthropic-messages",
 	 *   models: [
 	 *     {
@@ -1322,7 +1322,7 @@ export interface ProviderConfig {
 	name?: string;
 	/** Base URL for the API endpoint. Required when defining models. */
 	baseUrl?: string;
-	/** API key or environment variable name. Required when defining models (unless oauth provided). */
+	/** API key literal, env interpolation ($ENV_VAR or ${ENV_VAR}), or leading !command. Required when defining models (unless oauth provided). */
 	apiKey?: string;
 	/** API type. Required at provider or model level when defining models. */
 	api?: Api;

packages/coding-agent/src/core/model-registry.ts

@@ -25,11 +25,17 @@ import { type Static, Type } from "typebox";
 import { Compile } from "typebox/compile";
 import type { TLocalizedValidationError } from "typebox/error";
 import { getAgentDir } from "../config.ts";
+import { warnDeprecation } from "../utils/deprecation.ts";
+import { stripJsonComments } from "../utils/json.ts";
 import { normalizePath } from "../utils/paths.ts";
 import type { AuthStatus, AuthStorage } from "./auth-storage.ts";
 import { BUILT_IN_PROVIDER_DISPLAY_NAMES } from "./provider-display-names.ts";
 import {
 	clearConfigValueCache,
+	getConfigValueEnvVarNames,
+	isCommandConfigValue,
+	isConfigValueConfigured,
+	isLegacyEnvVarNameConfigValue,
 	resolveConfigValueOrThrow,
 	resolveConfigValueUncached,
 	resolveHeadersOrThrow,
@@ -218,13 +224,6 @@ function formatValidationPath(error: TLocalizedValidationError): string {
 	return path || "root";
 }
 
-/** Strip `//` line comments and trailing commas from JSON, leaving string literals untouched. */
-function stripJsonComments(input: string): string {
-	return input
-		.replace(/"(?:\\.|[^"\\])*"|\/\/[^\n]*/g, (m) => (m[0] === '"' ? m : ""))
-		.replace(/"(?:\\.|[^"\\])*"|,(\s*[}\]])/g, (m, tail) => tail ?? (m[0] === '"' ? m : ""));
-}
-
 /** Provider override config (baseUrl, compat) without request auth/headers */
 interface ProviderOverride {
 	baseUrl?: string;
@@ -237,6 +236,77 @@ interface ProviderRequestConfig {
 	authHeader?: boolean;
 }
 
+function migrateLegacyRegisterProviderConfigValue(providerName: string, field: string, value: string): string {
+	if (!isLegacyEnvVarNameConfigValue(value)) return value;
+	warnDeprecation(
+		`registerProvider("${providerName}") ${field} value "${value}" is treated as a legacy environment variable reference. This will no longer be detected as an environment variable reference in a future release. Pass "$${value}" instead.`,
+	);
+	return `$${value}`;
+}
+
+function migrateLegacyRegisterProviderHeaders(
+	providerName: string,
+	field: string,
+	headers: Record<string, string> | undefined,
+): Record<string, string> | undefined {
+	if (!headers) return undefined;
+	let migratedHeaders: Record<string, string> | undefined;
+	for (const [key, value] of Object.entries(headers)) {
+		const migratedValue = migrateLegacyRegisterProviderConfigValue(providerName, `${field} header "${key}"`, value);
+		if (migratedValue === value) continue;
+		migratedHeaders ??= { ...headers };
+		migratedHeaders[key] = migratedValue;
+	}
+	return migratedHeaders ?? headers;
+}
+
+function migrateLegacyRegisterProviderConfigValues(
+	providerName: string,
+	config: ProviderConfigInput,
+): ProviderConfigInput {
+	let migratedConfig: ProviderConfigInput | undefined;
+
+	const setMigratedConfigValue = <TKey extends keyof ProviderConfigInput>(
+		key: TKey,
+		value: ProviderConfigInput[TKey],
+	) => {
+		migratedConfig ??= { ...config };
+		migratedConfig[key] = value;
+	};
+
+	if (config.apiKey) {
+		const apiKey = migrateLegacyRegisterProviderConfigValue(providerName, "apiKey", config.apiKey);
+		if (apiKey !== config.apiKey) {
+			setMigratedConfigValue("apiKey", apiKey);
+		}
+	}
+
+	const headers = migrateLegacyRegisterProviderHeaders(providerName, "headers", config.headers);
+	if (headers !== config.headers) {
+		setMigratedConfigValue("headers", headers);
+	}
+
+	if (config.models) {
+		let models: ProviderConfigInput["models"] | undefined;
+		for (let index = 0; index < config.models.length; index++) {
+			const model = config.models[index];
+			const modelHeaders = migrateLegacyRegisterProviderHeaders(
+				providerName,
+				`model "${model.id}" headers`,
+				model.headers,
+			);
+			if (modelHeaders === model.headers) continue;
+			models ??= [...config.models];
+			models[index] = { ...model, headers: modelHeaders };
+		}
+		if (models) {
+			setMigratedConfigValue("models", models);
+		}
+	}
+
+	return migratedConfig ?? config;
+}
+
 export type ResolvedRequestAuth =
 	| {
 			ok: true;
@@ -641,9 +711,10 @@ export class ModelRegistry {
 	 * Get API key for a model.
 	 */
 	hasConfiguredAuth(model: Model<Api>): boolean {
+		const providerApiKey = this.providerRequestConfigs.get(model.provider)?.apiKey;
 		return (
 			this.authStorage.hasAuth(model.provider) ||
-			this.providerRequestConfigs.get(model.provider)?.apiKey !== undefined
+			(providerApiKey !== undefined && isConfigValueConfigured(providerApiKey))
 		);
 	}
 
@@ -738,12 +809,15 @@ export class ModelRegistry {
 			return authStatus;
 		}
 
-		if (providerApiKey.startsWith("!")) {
+		if (isCommandConfigValue(providerApiKey)) {
 			return { configured: true, source: "models_json_command" };
 		}
 
-		if (process.env[providerApiKey]) {
-			return { configured: true, source: "environment", label: providerApiKey };
+		const envVarNames = getConfigValueEnvVarNames(providerApiKey);
+		if (envVarNames.length > 0) {
+			return isConfigValueConfigured(providerApiKey)
+				? { configured: true, source: "environment", label: envVarNames.join(", ") }
+				: { configured: false };
 		}
 
 		return { configured: true, source: "models_json_key" };
@@ -794,9 +868,10 @@ export class ModelRegistry {
 	 * If provider has oauth: registers OAuth provider for /login support.
 	 */
 	registerProvider(providerName: string, config: ProviderConfigInput): void {
-		this.validateProviderConfig(providerName, config);
-		this.applyProviderConfig(providerName, config);
-		this.upsertRegisteredProvider(providerName, config);
+		const migratedConfig = migrateLegacyRegisterProviderConfigValues(providerName, config);
+		this.validateProviderConfig(providerName, migratedConfig);
+		this.applyProviderConfig(providerName, migratedConfig);
+		this.upsertRegisteredProvider(providerName, migratedConfig);
 	}
 
 	/**

packages/coding-agent/src/core/resolve-config-value.ts

@@ -8,18 +8,151 @@ import { getShellConfig } from "../utils/shell.ts";
 
 // Cache for shell command results (persists for process lifetime)
 const commandResultCache = new Map<string, string | undefined>();
+const ENV_VAR_NAME_RE = /^[A-Za-z_][A-Za-z0-9_]*$/;
+const ENV_VAR_NAME_PREFIX_RE = /^[A-Za-z_][A-Za-z0-9_]*/;
+const LEGACY_ENV_VAR_NAME_RE = /^[A-Z_][A-Z0-9_]*$/;
+
+type TemplatePart = { type: "literal"; value: string } | { type: "env"; name: string };
+
+type ConfigValueReference = { type: "command"; config: string } | { type: "template"; parts: TemplatePart[] };
+
+function appendLiteral(parts: TemplatePart[], value: string): void {
+	if (!value) return;
+	const previousPart = parts[parts.length - 1];
+	if (previousPart?.type === "literal") {
+		previousPart.value += value;
+		return;
+	}
+	parts.push({ type: "literal", value });
+}
+
+function parseConfigValueTemplate(config: string): TemplatePart[] {
+	const parts: TemplatePart[] = [];
+	let index = 0;
+
+	while (index < config.length) {
+		const dollarIndex = config.indexOf("$", index);
+		if (dollarIndex < 0) {
+			appendLiteral(parts, config.slice(index));
+			break;
+		}
+
+		appendLiteral(parts, config.slice(index, dollarIndex));
+		const nextChar = config[dollarIndex + 1];
+
+		if (nextChar === "$" || nextChar === "!") {
+			appendLiteral(parts, nextChar);
+			index = dollarIndex + 2;
+			continue;
+		}
+
+		if (nextChar === "{") {
+			const endIndex = config.indexOf("}", dollarIndex + 2);
+			if (endIndex < 0) {
+				appendLiteral(parts, "$");
+				index = dollarIndex + 1;
+				continue;
+			}
+
+			const name = config.slice(dollarIndex + 2, endIndex);
+			if (ENV_VAR_NAME_RE.test(name)) {
+				parts.push({ type: "env", name });
+			} else {
+				appendLiteral(parts, config.slice(dollarIndex, endIndex + 1));
+			}
+			index = endIndex + 1;
+			continue;
+		}
+
+		const match = config.slice(dollarIndex + 1).match(ENV_VAR_NAME_PREFIX_RE);
+		if (match) {
+			parts.push({ type: "env", name: match[0] });
+			index = dollarIndex + 1 + match[0].length;
+			continue;
+		}
+
+		appendLiteral(parts, "$");
+		index = dollarIndex + 1;
+	}
+
+	return parts;
+}
+
+function parseConfigValueReference(config: string): ConfigValueReference {
+	if (config.startsWith("!")) {
+		return { type: "command", config };
+	}
+
+	return { type: "template", parts: parseConfigValueTemplate(config) };
+}
+
+function resolveEnvConfigValue(name: string): string | undefined {
+	return process.env[name] || undefined;
+}
+
+function getTemplateEnvVarNames(parts: TemplatePart[]): string[] {
+	const names: string[] = [];
+	for (const part of parts) {
+		if (part.type !== "env" || names.includes(part.name)) continue;
+		names.push(part.name);
+	}
+	return names;
+}
+
+function resolveTemplate(parts: TemplatePart[]): string | undefined {
+	let resolved = "";
+	for (const part of parts) {
+		if (part.type === "literal") {
+			resolved += part.value;
+			continue;
+		}
+		const envValue = resolveEnvConfigValue(part.name);
+		if (envValue === undefined) return undefined;
+		resolved += envValue;
+	}
+	return resolved;
+}
+
+export function getConfigValueEnvVarName(config: string): string | undefined {
+	const reference = parseConfigValueReference(config);
+	if (reference.type !== "template") return undefined;
+	return reference.parts.length === 1 && reference.parts[0]?.type === "env" ? reference.parts[0].name : undefined;
+}
+
+export function getConfigValueEnvVarNames(config: string): string[] {
+	const reference = parseConfigValueReference(config);
+	return reference.type === "template" ? getTemplateEnvVarNames(reference.parts) : [];
+}
+
+export function getMissingConfigValueEnvVarNames(config: string): string[] {
+	return getConfigValueEnvVarNames(config).filter((name) => resolveEnvConfigValue(name) === undefined);
+}
+
+export function isCommandConfigValue(config: string): boolean {
+	return parseConfigValueReference(config).type === "command";
+}
+
+export function isConfigValueConfigured(config: string): boolean {
+	return getMissingConfigValueEnvVarNames(config).length === 0;
+}
+
+export function isLegacyEnvVarNameConfigValue(config: string): boolean {
+	return LEGACY_ENV_VAR_NAME_RE.test(config);
+}
 
 /**
  * Resolve a config value (API key, header value, etc.) to an actual value.
  * - If starts with "!", executes the rest as a shell command and uses stdout (cached)
- * - Otherwise checks environment variable first, then treats as literal (not cached)
+ * - Interpolates "$ENV_VAR" or "${ENV_VAR}" references with the named environment variable
+ * - In non-command values, "$$" escapes a literal "$" and "$!" escapes a literal "!"
+ * - Otherwise treats the value as a literal
  */
 export function resolveConfigValue(config: string): string | undefined {
-	if (config.startsWith("!")) {
-		return executeCommand(config);
+	const reference = parseConfigValueReference(config);
+	if (reference.type === "command") {
+		return executeCommand(reference.config);
 	}
-	const envValue = process.env[config];
-	return envValue || config;
+	return resolveTemplate(reference.parts);
 }
 
 function executeWithConfiguredShell(command: string): { executed: boolean; value: string | undefined } {
@@ -89,11 +222,11 @@ function executeCommand(commandConfig: string): string | undefined {
  * Resolve all header values using the same resolution logic as API keys.
  */
 export function resolveConfigValueUncached(config: string): string | undefined {
-	if (config.startsWith("!")) {
-		return executeCommandUncached(config);
+	const reference = parseConfigValueReference(config);
+	if (reference.type === "command") {
+		return executeCommandUncached(reference.config);
 	}
-	const envValue = process.env[config];
-	return envValue || config;
+	return resolveTemplate(reference.parts);
 }
 
 export function resolveConfigValueOrThrow(config: string, description: string): string {
@@ -102,8 +235,19 @@ export function resolveConfigValueOrThrow(config: string, description: string):
 		return resolvedValue;
 	}
 
-	if (config.startsWith("!")) {
-		throw new Error(`Failed to resolve ${description} from shell command: ${config.slice(1)}`);
+	const reference = parseConfigValueReference(config);
+	if (reference.type === "command") {
+		throw new Error(`Failed to resolve ${description} from shell command: ${reference.config.slice(1)}`);
+	}
+
+	if (reference.type === "template") {
+		const missingEnvVars = getMissingConfigValueEnvVarNames(config);
+		if (missingEnvVars.length === 1) {
+			throw new Error(`Failed to resolve ${description} from environment variable: ${missingEnvVars[0]}`);
+		}
+		if (missingEnvVars.length > 1) {
+			throw new Error(`Failed to resolve ${description} from environment variables: ${missingEnvVars.join(", ")}`);
+		}
 	}
 
 	throw new Error(`Failed to resolve ${description}`);

packages/coding-agent/src/migrations.ts

@@ -3,10 +3,12 @@
  */
 
 import chalk from "chalk";
-import { existsSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "fs";
+import { chmodSync, existsSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "fs";
 import { dirname, join } from "path";
 import { CONFIG_DIR_NAME, getAgentDir, getBinDir } from "./config.ts";
 import { migrateKeybindingsConfig } from "./core/keybindings.ts";
+import { isLegacyEnvVarNameConfigValue } from "./core/resolve-config-value.ts";
+import { stripJsonComments } from "./utils/json.ts";
 
 const MIGRATION_GUIDE_URL =
 	"https://github.com/earendil-works/pi-mono/blob/main/packages/coding-agent/CHANGELOG.md#extensions-migration";
@@ -72,6 +74,135 @@ export function migrateAuthToAuthJson(): string[] {
 	return providers;
 }
 
+interface ConfigValueMigration {
+	location: string;
+	from: string;
+	to: string;
+}
+
+function migrateLegacyEnvVarString(value: string): string | undefined {
+	return isLegacyEnvVarNameConfigValue(value) ? `$${value}` : undefined;
+}
+
+function migrateStringProperty(
+	record: Record<string, unknown>,
+	key: string,
+	location: string,
+	migrations: ConfigValueMigration[],
+): boolean {
+	const value = record[key];
+	if (typeof value !== "string") return false;
+	const migrated = migrateLegacyEnvVarString(value);
+	if (migrated === undefined) return false;
+	record[key] = migrated;
+	migrations.push({ location, from: value, to: migrated });
+	return true;
+}
+
+function migrateHeadersConfig(headers: unknown, location: string, migrations: ConfigValueMigration[]): boolean {
+	if (typeof headers !== "object" || headers === null || Array.isArray(headers)) return false;
+	const headerRecord = headers as Record<string, unknown>;
+	let migrated = false;
+	for (const [key, value] of Object.entries(headerRecord)) {
+		if (typeof value !== "string") continue;
+		const migratedValue = migrateLegacyEnvVarString(value);
+		if (migratedValue === undefined) continue;
+		headerRecord[key] = migratedValue;
+		migrations.push({ location: `${location}[${JSON.stringify(key)}]`, from: value, to: migratedValue });
+		migrated = true;
+	}
+	return migrated;
+}
+
+function migrateAuthJsonConfigValues(agentDir: string): ConfigValueMigration[] {
+	const authPath = join(agentDir, "auth.json");
+	if (!existsSync(authPath)) return [];
+
+	try {
+		const parsed = JSON.parse(readFileSync(authPath, "utf-8")) as unknown;
+		if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) return [];
+		const authData = parsed as Record<string, unknown>;
+
+		const migrations: ConfigValueMigration[] = [];
+		for (const [provider, credential] of Object.entries(authData)) {
+			if (typeof credential !== "object" || credential === null || Array.isArray(credential)) continue;
+			const credentialRecord = credential as Record<string, unknown>;
+			if (credentialRecord.type !== "api_key") continue;
+			migrateStringProperty(credentialRecord, "key", `auth.json[${JSON.stringify(provider)}].key`, migrations);
+		}
+
+		if (migrations.length === 0) return [];
+		writeFileSync(authPath, `${JSON.stringify(parsed, null, 2)}\n`, "utf-8");
+		chmodSync(authPath, 0o600);
+		return migrations;
+	} catch {
+		return [];
+	}
+}
+
+function migrateModelsJsonConfigValues(agentDir: string): ConfigValueMigration[] {
+	const modelsPath = join(agentDir, "models.json");
+	if (!existsSync(modelsPath)) return [];
+
+	const parsed = JSON.parse(stripJsonComments(readFileSync(modelsPath, "utf-8"))) as unknown;
+	if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) return [];
+	const modelsData = parsed as Record<string, unknown>;
+	const providers = modelsData.providers;
+	if (typeof providers !== "object" || providers === null || Array.isArray(providers)) return [];
+
+	const migrations: ConfigValueMigration[] = [];
+	for (const [provider, providerConfig] of Object.entries(providers)) {
+		if (typeof providerConfig !== "object" || providerConfig === null || Array.isArray(providerConfig)) continue;
+		const providerRecord = providerConfig as Record<string, unknown>;
+		const providerLocation = `models.json.providers[${JSON.stringify(provider)}]`;
+		migrateStringProperty(providerRecord, "apiKey", `${providerLocation}.apiKey`, migrations);
+		migrateHeadersConfig(providerRecord.headers, `${providerLocation}.headers`, migrations);
+
+		if (Array.isArray(providerRecord.models)) {
+			for (let index = 0; index < providerRecord.models.length; index++) {
+				const modelConfig = providerRecord.models[index];
+				if (typeof modelConfig !== "object" || modelConfig === null || Array.isArray(modelConfig)) continue;
+				const modelRecord = modelConfig as Record<string, unknown>;
+				const modelKey = typeof modelRecord.id === "string" ? JSON.stringify(modelRecord.id) : String(index);
+				migrateHeadersConfig(modelRecord.headers, `${providerLocation}.models[${modelKey}].headers`, migrations);
+			}
+		}
+
+		const modelOverrides = providerRecord.modelOverrides;
+		if (typeof modelOverrides === "object" && modelOverrides !== null && !Array.isArray(modelOverrides)) {
+			for (const [modelId, modelOverride] of Object.entries(modelOverrides)) {
+				if (typeof modelOverride !== "object" || modelOverride === null || Array.isArray(modelOverride)) continue;
+				const modelOverrideRecord = modelOverride as Record<string, unknown>;
+				migrateHeadersConfig(
+					modelOverrideRecord.headers,
+					`${providerLocation}.modelOverrides[${JSON.stringify(modelId)}].headers`,
+					migrations,
+				);
+			}
+		}
+	}
+
+	if (migrations.length === 0) return [];
+	writeFileSync(modelsPath, `${JSON.stringify(parsed, null, 2)}\n`, "utf-8");
+	return migrations;
+}
+
+function migrateExplicitEnvVarConfigValues(): void {
+	const agentDir = getAgentDir();
+	const migrations = [...migrateAuthJsonConfigValues(agentDir), ...migrateModelsJsonConfigValues(agentDir)];
+	if (migrations.length === 0) return;
+
+	const details = migrations.map((migration) => `  - ${migration.location}: ${migration.from} -> ${migration.to}`);
+	console.log(
+		chalk.yellow(
+			[
+				"Warning: Migrated API key/header environment references to explicit $ENV_VAR syntax. Plain strings will be treated as literals.",
+				...details,
+			].join("\n"),
+		),
+	);
+}
+
 /**
  * Migrate sessions from ~/.pi/agent/*.jsonl to proper session directories.
  *
@@ -307,6 +438,7 @@ export function runMigrations(cwd: string): {
 	deprecationWarnings: string[];
 } {
 	const migratedAuthProviders = migrateAuthToAuthJson();
+	migrateExplicitEnvVarConfigValues();
 	migrateSessionsFromAgentRoot();
 	migrateToolsToBin();
 	migrateKeybindingsConfigFile();

packages/coding-agent/src/utils/deprecation.ts

@@ -0,0 +1,14 @@
+import chalk from "chalk";
+
+const emittedDeprecationWarnings = new Set<string>();
+
+export function warnDeprecation(message: string): void {
+	if (emittedDeprecationWarnings.has(message)) return;
+	emittedDeprecationWarnings.add(message);
+	console.warn(chalk.yellow(`Deprecation warning: ${message}`));
+}
+
+/** Clear deprecation warning state. Exported for tests. */
+export function clearDeprecationWarningsForTests(): void {
+	emittedDeprecationWarnings.clear();
+}

packages/coding-agent/src/utils/json.ts

@@ -0,0 +1,6 @@
+/** Strip `//` line comments and trailing commas from JSON, leaving string literals untouched. */
+export function stripJsonComments(input: string): string {
+	return input
+		.replace(/"(?:\\.|[^"\\])*"|\/\/[^\n]*/g, (m) => (m[0] === '"' ? m : ""))
+		.replace(/"(?:\\.|[^"\\])*"|,(\s*[}\]])/g, (m, tail) => tail ?? (m[0] === '"' ? m : ""));
+}