diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 10abf3c4..1225fdb6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed API key and header config resolution to treat plain strings as literals, support `$ENV_VAR` / `${ENV_VAR}` interpolation and `$!` bang escaping, and require explicit env syntax for config files, avoiding Windows case-insensitive env matches corrupting literal keys ([#5095](https://github.com/earendil-works/pi/issues/5095)). - Fixed session disposal to abort in-flight agent, compaction, branch summary, retry, and bash work ([#5029](https://github.com/earendil-works/pi/pull/5029) by [@TerminallyChilI](https://github.com/TerminallyChilI)). - Fixed `pi.getAllTools()` to expose each tool's `promptGuidelines` for extensions that need per-tool guideline attribution ([#4879](https://github.com/earendil-works/pi/issues/4879)). - Fixed follow-up messages queued by `agent_end` extension handlers to drain before the agent becomes idle ([#5115](https://github.com/earendil-works/pi-mono/pull/5115) by [@DanielThomas](https://github.com/DanielThomas)). diff --git a/packages/coding-agent/docs/custom-provider.md b/packages/coding-agent/docs/custom-provider.md index 70ede4c3..50cb1ef0 100644 --- a/packages/coding-agent/docs/custom-provider.md +++ b/packages/coding-agent/docs/custom-provider.md @@ -43,7 +43,7 @@ export default function (pi: ExtensionAPI) { pi.registerProvider("my-provider", { name: "My Provider", baseUrl: "https://api.example.com", - apiKey: "MY_API_KEY", + apiKey: "$MY_API_KEY", api: "openai-completions", models: [ { @@ -83,7 +83,7 @@ pi.registerProvider("openai", { pi.registerProvider("google", { baseUrl: "https://ai-gateway.corp.com/google", headers: { - "X-Corp-Auth": "CORP_AUTH_TOKEN" // env var or literal + "X-Corp-Auth": "$CORP_AUTH_TOKEN" // env var or literal } }); ``` @@ -112,7 +112,7 @@ export default async function (pi: ExtensionAPI) { pi.registerProvider("local-openai", { baseUrl: "http://localhost:1234/v1", - apiKey: "LOCAL_OPENAI_API_KEY", + apiKey: "$LOCAL_OPENAI_API_KEY", api: "openai-completions", models: payload.data.map((model) => ({ id: model.id, @@ -132,7 +132,7 @@ This registers the fetched models before startup finishes. ```typescript pi.registerProvider("my-llm", { baseUrl: "https://api.my-llm.com/v1", - apiKey: "MY_LLM_API_KEY", // env var name or literal value + apiKey: "$MY_LLM_API_KEY", // env var reference api: "openai-completions", // which streaming API to use models: [ { @@ -155,6 +155,8 @@ pi.registerProvider("my-llm", { When `models` is provided, it **replaces** all existing models for that provider. +`apiKey` and custom header values use the same config value syntax as `models.json`: `!command` at the start executes a command for the whole value, `$ENV_VAR` and `${ENV_VAR}` interpolate environment variables, `$$` emits a literal `$`, and `$!` emits a literal `!`. + ## Unregister Provider Use `pi.unregisterProvider(name)` to remove a provider that was previously registered via `pi.registerProvider(name, ...)`: @@ -163,7 +165,7 @@ Use `pi.unregisterProvider(name)` to remove a provider that was previously regis // Register pi.registerProvider("my-llm", { baseUrl: "https://api.my-llm.com/v1", - apiKey: "MY_LLM_API_KEY", + apiKey: "$MY_LLM_API_KEY", api: "openai-completions", models: [ { @@ -243,7 +245,7 @@ If your provider expects `Authorization: Bearer ` but doesn't use a standar ```typescript pi.registerProvider("custom-api", { baseUrl: "https://api.example.com", - apiKey: "MY_API_KEY", + apiKey: "$MY_API_KEY", authHeader: true, // adds Authorization: Bearer header api: "openai-completions", models: [...] @@ -592,7 +594,7 @@ Register your stream function: ```typescript pi.registerProvider("my-provider", { baseUrl: "https://api.example.com", - apiKey: "MY_API_KEY", + apiKey: "$MY_API_KEY", api: "my-custom-api", models: [...], streamSimple: streamMyProvider @@ -629,7 +631,7 @@ interface ProviderConfig { /** API endpoint URL. Required when defining models. */ baseUrl?: string; - /** API key or environment variable name. Required when defining models (unless oauth). */ + /** API key literal, env interpolation ($ENV_VAR or ${ENV_VAR}), or !command. Required when defining models (unless oauth). */ apiKey?: string; /** API type for streaming. Required at provider or model level when defining models. */ @@ -642,7 +644,7 @@ interface ProviderConfig { options?: SimpleStreamOptions ) => AssistantMessageEventStream; - /** Custom headers to include in requests. Values can be env var names. */ + /** Custom headers to include in requests. Values use the same resolution syntax as apiKey. */ headers?: Record; /** If true, adds Authorization: Bearer header with the resolved API key. */ diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index db856fc8..40781092 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -199,7 +199,7 @@ export default async function (pi: ExtensionAPI) { pi.registerProvider("local-openai", { baseUrl: "http://localhost:1234/v1", - apiKey: "LOCAL_OPENAI_API_KEY", + apiKey: "$LOCAL_OPENAI_API_KEY", api: "openai-completions", models: payload.data.map((model) => ({ id: model.id, @@ -1555,7 +1555,7 @@ If you need to discover models from a remote endpoint, prefer an async extension pi.registerProvider("my-proxy", { name: "My Proxy", baseUrl: "https://proxy.example.com", - apiKey: "PROXY_API_KEY", // env var name or literal + apiKey: "$PROXY_API_KEY", // env var reference api: "anthropic-messages", models: [ { @@ -1602,7 +1602,7 @@ pi.registerProvider("corporate-ai", { **Config options:** - `name` - Display name for the provider in UI such as `/login`. - `baseUrl` - API endpoint URL. Required when defining models. -- `apiKey` - API key or environment variable name. Required when defining models (unless `oauth` provided). +- `apiKey` - API key literal, environment interpolation (`$ENV_VAR` or `${ENV_VAR}`), or leading `!command`. Required when defining models (unless `oauth` provided). `$$` escapes `$`, and `$!` escapes a literal `!` without triggering command execution. - `api` - API type: `"anthropic-messages"`, `"openai-completions"`, `"openai-responses"`, etc. - `headers` - Custom headers to include in requests. - `authHeader` - If true, adds `Authorization: Bearer` header automatically. diff --git a/packages/coding-agent/docs/models.md b/packages/coding-agent/docs/models.md index 85fccaf3..c97a6cf3 100644 --- a/packages/coding-agent/docs/models.md +++ b/packages/coding-agent/docs/models.md @@ -101,7 +101,7 @@ Use `google-generative-ai` with a `baseUrl` to add models from Google AI Studio, "my-google": { "baseUrl": "https://generativelanguage.googleapis.com/v1beta", "api": "google-generative-ai", - "apiKey": "GEMINI_API_KEY", + "apiKey": "$GEMINI_API_KEY", "models": [ { "id": "gemma-4-31b-it", @@ -143,22 +143,31 @@ Set `api` at provider level (default for all models) or model level (override pe ### Value Resolution -The `apiKey` and `headers` fields support three formats: +The `apiKey` and `headers` fields support command execution, environment interpolation, and literals: -- **Shell command:** `"!command"` executes and uses stdout +- **Shell command:** `"!command"` at the start executes the whole value as a command and uses stdout ```json "apiKey": "!security find-generic-password -ws 'anthropic'" "apiKey": "!op read 'op://vault/item/credential'" ``` -- **Environment variable:** Uses the value of the named variable +- **Environment interpolation:** `"$ENV_VAR"` or `"${ENV_VAR}"` uses the value of the named variable. Interpolation works inside larger literals. ```json - "apiKey": "MY_API_KEY" + "apiKey": "$MY_API_KEY" + "apiKey": "${KEY_PREFIX}_${KEY_SUFFIX}" + ``` + `$FOO_BAR` is the variable `FOO_BAR`; use `${FOO}_BAR` when `BAR` is literal text. Missing environment variables make the value unresolved. +- **Escapes:** `"$$"` emits a literal `"$"`; `"$!"` emits a literal `"!"` without triggering command execution. + ```json + "apiKey": "$$literal-dollar-prefix" + "apiKey": "$!literal-bang-prefix" ``` - **Literal value:** Used directly ```json "apiKey": "sk-..." ``` +Legacy uppercase env-var-like values such as `MY_API_KEY` are migrated to `$MY_API_KEY` on startup. + For `models.json`, shell commands are resolved at request time. pi intentionally does not apply built-in TTL, stale reuse, or recovery logic for arbitrary commands. Different commands need different caching and failure strategies, and pi cannot infer the right one. If your command is slow, expensive, rate-limited, or should keep using a previous value on transient failures, wrap it in your own script or command that implements the caching or TTL behavior you want. @@ -172,10 +181,10 @@ If your command is slow, expensive, rate-limited, or should keep using a previou "providers": { "custom-proxy": { "baseUrl": "https://proxy.example.com/v1", - "apiKey": "MY_API_KEY", + "apiKey": "$MY_API_KEY", "api": "anthropic-messages", "headers": { - "x-portkey-api-key": "PORTKEY_API_KEY", + "x-portkey-api-key": "$PORTKEY_API_KEY", "x-secret": "!op read 'op://vault/item/secret'" }, "models": [...] @@ -268,7 +277,7 @@ To merge custom models into a built-in provider, include the `models` array: "providers": { "anthropic": { "baseUrl": "https://my-proxy.example.com/v1", - "apiKey": "ANTHROPIC_API_KEY", + "apiKey": "$ANTHROPIC_API_KEY", "api": "anthropic-messages", "models": [...] } @@ -327,7 +336,7 @@ Some Anthropic models require adaptive thinking (`thinking.type: "adaptive"` plu "anthropic-proxy": { "baseUrl": "https://proxy.example.com", "api": "anthropic-messages", - "apiKey": "ANTHROPIC_PROXY_KEY", + "apiKey": "$ANTHROPIC_PROXY_KEY", "compat": { "supportsEagerToolInputStreaming": false, "supportsLongCacheRetention": true, @@ -405,7 +414,7 @@ Example: "providers": { "openrouter": { "baseUrl": "https://openrouter.ai/api/v1", - "apiKey": "OPENROUTER_API_KEY", + "apiKey": "$OPENROUTER_API_KEY", "api": "openai-completions", "models": [ { @@ -455,7 +464,7 @@ Vercel AI Gateway example: "providers": { "vercel-ai-gateway": { "baseUrl": "https://ai-gateway.vercel.sh/v1", - "apiKey": "AI_GATEWAY_API_KEY", + "apiKey": "$AI_GATEWAY_API_KEY", "api": "openai-completions", "models": [ { diff --git a/packages/coding-agent/docs/providers.md b/packages/coding-agent/docs/providers.md index 7e80f931..07b6ba08 100644 --- a/packages/coding-agent/docs/providers.md +++ b/packages/coding-agent/docs/providers.md @@ -101,23 +101,31 @@ The file is created with `0600` permissions (user read/write only). Auth file cr ### Key Resolution -The `key` field supports three formats: +The `key` field supports command execution, environment interpolation, and literals: -- **Shell command:** `"!command"` executes and uses stdout (cached for process lifetime) +- **Shell command:** `"!command"` at the start executes the whole value as a command and uses stdout (cached for process lifetime) ```json { "type": "api_key", "key": "!security find-generic-password -ws 'anthropic'" } { "type": "api_key", "key": "!op read 'op://vault/item/credential'" } ``` -- **Environment variable:** Uses the value of the named variable +- **Environment interpolation:** `"$ENV_VAR"` or `"${ENV_VAR}"` uses the value of the named variable. Interpolation works inside larger literals. ```json - { "type": "api_key", "key": "MY_ANTHROPIC_KEY" } + { "type": "api_key", "key": "$MY_ANTHROPIC_KEY" } + { "type": "api_key", "key": "${KEY_PREFIX}_${KEY_SUFFIX}" } + ``` + `$FOO_BAR` is the variable `FOO_BAR`; use `${FOO}_BAR` when `BAR` is literal text. Missing environment variables make the value unresolved. +- **Escapes:** `"$$"` emits a literal `"$"`; `"$!"` emits a literal `"!"` without triggering command execution. + ```json + { "type": "api_key", "key": "$$literal-dollar-prefix" } + { "type": "api_key", "key": "$!literal-bang-prefix" } ``` - **Literal value:** Used directly ```json { "type": "api_key", "key": "sk-ant-..." } + { "type": "api_key", "key": "public" } ``` -OAuth credentials are also stored here after `/login` and managed automatically. +Legacy uppercase env-var-like values such as `MY_API_KEY` are migrated to `$MY_API_KEY` on startup. OAuth credentials are also stored here after `/login` and managed automatically. ## Cloud Providers diff --git a/packages/coding-agent/examples/extensions/custom-provider-anthropic/index.ts b/packages/coding-agent/examples/extensions/custom-provider-anthropic/index.ts index 69267f40..51426362 100644 --- a/packages/coding-agent/examples/extensions/custom-provider-anthropic/index.ts +++ b/packages/coding-agent/examples/extensions/custom-provider-anthropic/index.ts @@ -568,7 +568,7 @@ function streamCustomAnthropic( export default function (pi: ExtensionAPI) { pi.registerProvider("custom-anthropic", { baseUrl: "https://api.anthropic.com", - apiKey: "CUSTOM_ANTHROPIC_API_KEY", + apiKey: "$CUSTOM_ANTHROPIC_API_KEY", api: "custom-anthropic-api", models: [ diff --git a/packages/coding-agent/examples/extensions/custom-provider-gitlab-duo/index.ts b/packages/coding-agent/examples/extensions/custom-provider-gitlab-duo/index.ts index 23d0a40a..c23f0dac 100644 --- a/packages/coding-agent/examples/extensions/custom-provider-gitlab-duo/index.ts +++ b/packages/coding-agent/examples/extensions/custom-provider-gitlab-duo/index.ts @@ -327,7 +327,7 @@ export function streamGitLabDuo( export default function (pi: ExtensionAPI) { pi.registerProvider("gitlab-duo", { baseUrl: AI_GATEWAY_URL, - apiKey: "GITLAB_TOKEN", + apiKey: "$GITLAB_TOKEN", api: "gitlab-duo-api", models: MODELS.map(({ id, name, reasoning, input, cost, contextWindow, maxTokens }) => ({ id, diff --git a/packages/coding-agent/src/core/extensions/types.ts b/packages/coding-agent/src/core/extensions/types.ts index 0d5bb7af..7696c62e 100644 --- a/packages/coding-agent/src/core/extensions/types.ts +++ b/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; diff --git a/packages/coding-agent/src/core/model-registry.ts b/packages/coding-agent/src/core/model-registry.ts index 712874d8..455ab71f 100644 --- a/packages/coding-agent/src/core/model-registry.ts +++ b/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 | undefined, +): Record | undefined { + if (!headers) return undefined; + let migratedHeaders: Record | 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 = ( + 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): 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); } /** diff --git a/packages/coding-agent/src/core/resolve-config-value.ts b/packages/coding-agent/src/core/resolve-config-value.ts index 215914ba..119e379d 100644 --- a/packages/coding-agent/src/core/resolve-config-value.ts +++ b/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(); +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}`); diff --git a/packages/coding-agent/src/migrations.ts b/packages/coding-agent/src/migrations.ts index 39aeea04..eb07f220 100644 --- a/packages/coding-agent/src/migrations.ts +++ b/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, + 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; + 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; + + 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; + 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; + 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; + 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; + 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; + 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(); diff --git a/packages/coding-agent/src/utils/deprecation.ts b/packages/coding-agent/src/utils/deprecation.ts new file mode 100644 index 00000000..78a2f146 --- /dev/null +++ b/packages/coding-agent/src/utils/deprecation.ts @@ -0,0 +1,14 @@ +import chalk from "chalk"; + +const emittedDeprecationWarnings = new Set(); + +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(); +} diff --git a/packages/coding-agent/src/utils/json.ts b/packages/coding-agent/src/utils/json.ts new file mode 100644 index 00000000..9ee7b7b1 --- /dev/null +++ b/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 : "")); +} diff --git a/packages/coding-agent/test/auth-storage.test.ts b/packages/coding-agent/test/auth-storage.test.ts index 9ec97100..24242b6b 100644 --- a/packages/coding-agent/test/auth-storage.test.ts +++ b/packages/coding-agent/test/auth-storage.test.ts @@ -112,13 +112,13 @@ describe("AuthStorage", () => { expect(apiKey).toBeUndefined(); }); - test("apiKey as environment variable name resolves to env value", async () => { + test("apiKey with $ prefix resolves to env value", async () => { const originalEnv = process.env.TEST_AUTH_API_KEY_12345; process.env.TEST_AUTH_API_KEY_12345 = "env-api-key-value"; try { writeAuthJson({ - anthropic: { type: "api_key", key: "TEST_AUTH_API_KEY_12345" }, + anthropic: { type: "api_key", key: "$TEST_AUTH_API_KEY_12345" }, }); authStorage = AuthStorage.create(authJsonPath); @@ -134,6 +134,140 @@ describe("AuthStorage", () => { } }); + test("apiKey with braced env syntax resolves to env value", async () => { + const originalEnv = process.env.TEST_AUTH_BRACED_API_KEY_12345; + process.env.TEST_AUTH_BRACED_API_KEY_12345 = "braced-env-api-key-value"; + const bracedKey = "$" + "{TEST_AUTH_BRACED_API_KEY_12345}"; + + try { + writeAuthJson({ + anthropic: { type: "api_key", key: bracedKey }, + }); + + authStorage = AuthStorage.create(authJsonPath); + const apiKey = await authStorage.getApiKey("anthropic"); + + expect(apiKey).toBe("braced-env-api-key-value"); + } finally { + if (originalEnv === undefined) { + delete process.env.TEST_AUTH_BRACED_API_KEY_12345; + } else { + process.env.TEST_AUTH_BRACED_API_KEY_12345 = originalEnv; + } + } + }); + + test("apiKey interpolates braced env references inside literals", async () => { + const originalPartA = process.env.TEST_AUTH_INTERPOLATED_PART_A_12345; + const originalPartB = process.env.TEST_AUTH_INTERPOLATED_PART_B_12345; + process.env.TEST_AUTH_INTERPOLATED_PART_A_12345 = "left"; + process.env.TEST_AUTH_INTERPOLATED_PART_B_12345 = "right"; + const interpolatedKey = [ + "$", + "{TEST_AUTH_INTERPOLATED_PART_A_12345}_$", + "{TEST_AUTH_INTERPOLATED_PART_B_12345}", + ].join(""); + + try { + writeAuthJson({ + anthropic: { type: "api_key", key: interpolatedKey }, + }); + + authStorage = AuthStorage.create(authJsonPath); + const apiKey = await authStorage.getApiKey("anthropic"); + + expect(apiKey).toBe("left_right"); + } finally { + if (originalPartA === undefined) { + delete process.env.TEST_AUTH_INTERPOLATED_PART_A_12345; + } else { + process.env.TEST_AUTH_INTERPOLATED_PART_A_12345 = originalPartA; + } + if (originalPartB === undefined) { + delete process.env.TEST_AUTH_INTERPOLATED_PART_B_12345; + } else { + process.env.TEST_AUTH_INTERPOLATED_PART_B_12345 = originalPartB; + } + } + }); + + test("apiKey with $$ prefix escapes a leading dollar", async () => { + writeAuthJson({ + anthropic: { type: "api_key", key: "$$TEST_AUTH_API_KEY_12345" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + const apiKey = await authStorage.getApiKey("anthropic"); + + expect(apiKey).toBe("$TEST_AUTH_API_KEY_12345"); + }); + + test("apiKey with $! escapes a literal bang and still interpolates later env refs", async () => { + const originalEnv = process.env.TEST_AUTH_API_KEY_12345; + process.env.TEST_AUTH_API_KEY_12345 = "env-api-key-value"; + + try { + writeAuthJson({ + anthropic: { type: "api_key", key: "$!literal-$TEST_AUTH_API_KEY_12345" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + const apiKey = await authStorage.getApiKey("anthropic"); + + expect(apiKey).toBe("!literal-env-api-key-value"); + } finally { + if (originalEnv === undefined) { + delete process.env.TEST_AUTH_API_KEY_12345; + } else { + process.env.TEST_AUTH_API_KEY_12345 = originalEnv; + } + } + }); + + test("plain API key is used directly even when it matches an env var", async () => { + const originalEnv = process.env.TEST_AUTH_API_KEY_12345; + process.env.TEST_AUTH_API_KEY_12345 = "env-api-key-value"; + + try { + writeAuthJson({ + anthropic: { type: "api_key", key: "TEST_AUTH_API_KEY_12345" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + const apiKey = await authStorage.getApiKey("anthropic"); + + expect(apiKey).toBe("TEST_AUTH_API_KEY_12345"); + } finally { + if (originalEnv === undefined) { + delete process.env.TEST_AUTH_API_KEY_12345; + } else { + process.env.TEST_AUTH_API_KEY_12345 = originalEnv; + } + } + }); + + test("literal public API key is not corrupted by the Windows PUBLIC env var", async () => { + const originalPublic = process.env.PUBLIC; + process.env.PUBLIC = "C:\\Users\\Public"; + + try { + writeAuthJson({ + opencode: { type: "api_key", key: "public" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + const apiKey = await authStorage.getApiKey("opencode"); + + expect(apiKey).toBe("public"); + } finally { + if (originalPublic === undefined) { + delete process.env.PUBLIC; + } else { + process.env.PUBLIC = originalPublic; + } + } + }); + test("apiKey as literal value is used directly when not an env var", async () => { // Make sure this isn't an env var delete process.env.literal_api_key_value; @@ -274,7 +408,7 @@ describe("AuthStorage", () => { process.env[envVarName] = "first-value"; writeAuthJson({ - anthropic: { type: "api_key", key: envVarName }, + anthropic: { type: "api_key", key: `$${envVarName}` }, }); authStorage = AuthStorage.create(authJsonPath); diff --git a/packages/coding-agent/test/config-value-migration.test.ts b/packages/coding-agent/test/config-value-migration.test.ts new file mode 100644 index 00000000..35d4f155 --- /dev/null +++ b/packages/coding-agent/test/config-value-migration.test.ts @@ -0,0 +1,138 @@ +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ENV_AGENT_DIR } from "../src/config.ts"; +import { runMigrations } from "../src/migrations.ts"; + +describe("config value env var syntax migration", () => { + const tempDirs: string[] = []; + + afterEach(() => { + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + vi.restoreAllMocks(); + }); + + function createAgentDir(): string { + const agentDir = fs.mkdtempSync(path.join(os.tmpdir(), "pi-config-value-migration-test-")); + tempDirs.push(agentDir); + return agentDir; + } + + function withAgentDir(agentDir: string, fn: () => void): void { + const previousAgentDir = process.env[ENV_AGENT_DIR]; + process.env[ENV_AGENT_DIR] = agentDir; + try { + fn(); + } finally { + if (previousAgentDir === undefined) { + delete process.env[ENV_AGENT_DIR]; + } else { + process.env[ENV_AGENT_DIR] = previousAgentDir; + } + } + } + + it("rewrites legacy uppercase auth.json API key values to explicit env references", () => { + const agentDir = createAgentDir(); + fs.writeFileSync( + path.join(agentDir, "auth.json"), + `${JSON.stringify( + { + anthropic: { type: "api_key", key: "ANTHROPIC_API_KEY" }, + openai: { type: "api_key", key: "$OPENAI_API_KEY" }, + opencode: { type: "api_key", key: "public" }, + github: { type: "oauth", access: "ACCESS_TOKEN", refresh: "REFRESH_TOKEN", expires: 1 }, + }, + null, + 2, + )}\n`, + "utf-8", + ); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + withAgentDir(agentDir, () => runMigrations(agentDir)); + + const migrated = JSON.parse(fs.readFileSync(path.join(agentDir, "auth.json"), "utf-8")) as Record< + string, + Record + >; + expect(migrated.anthropic.key).toBe("$ANTHROPIC_API_KEY"); + expect(migrated.openai.key).toBe("$OPENAI_API_KEY"); + expect(migrated.opencode.key).toBe("public"); + expect(migrated.github.access).toBe("ACCESS_TOKEN"); + const logMessage = String(logSpy.mock.calls[0]?.[0] ?? ""); + expect(logMessage).toContain("explicit $ENV_VAR syntax"); + expect(logMessage).toContain('auth.json["anthropic"].key: ANTHROPIC_API_KEY -> $ANTHROPIC_API_KEY'); + }); + + it("rewrites legacy uppercase models.json API key and header values", () => { + const agentDir = createAgentDir(); + fs.writeFileSync( + path.join(agentDir, "models.json"), + `${JSON.stringify( + { + providers: { + "custom-provider": { + baseUrl: "https://example.com/v1", + apiKey: "CUSTOM_API_KEY", + api: "openai-completions", + headers: { + "x-api-key": "HEADER_API_KEY", + "x-literal": "literal", + }, + models: [ + { + id: "model-a", + headers: { "x-model-key": "MODEL_API_KEY" }, + }, + ], + modelOverrides: { + "model-b": { headers: { "x-override-key": "OVERRIDE_API_KEY" } }, + }, + }, + }, + }, + null, + 2, + )}\n`, + "utf-8", + ); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + withAgentDir(agentDir, () => runMigrations(agentDir)); + + const migrated = JSON.parse(fs.readFileSync(path.join(agentDir, "models.json"), "utf-8")) as { + providers: Record< + string, + { + apiKey?: string; + headers?: Record; + models?: Array<{ headers?: Record }>; + modelOverrides?: Record }>; + } + >; + }; + const provider = migrated.providers["custom-provider"]!; + expect(provider.apiKey).toBe("$CUSTOM_API_KEY"); + expect(provider.headers?.["x-api-key"]).toBe("$HEADER_API_KEY"); + expect(provider.headers?.["x-literal"]).toBe("literal"); + expect(provider.models?.[0]?.headers?.["x-model-key"]).toBe("$MODEL_API_KEY"); + expect(provider.modelOverrides?.["model-b"]?.headers?.["x-override-key"]).toBe("$OVERRIDE_API_KEY"); + const logMessage = String(logSpy.mock.calls[0]?.[0] ?? ""); + expect(logMessage).toContain( + 'models.json.providers["custom-provider"].apiKey: CUSTOM_API_KEY -> $CUSTOM_API_KEY', + ); + expect(logMessage).toContain( + 'models.json.providers["custom-provider"].headers["x-api-key"]: HEADER_API_KEY -> $HEADER_API_KEY', + ); + expect(logMessage).toContain( + 'models.json.providers["custom-provider"].models["model-a"].headers["x-model-key"]: MODEL_API_KEY -> $MODEL_API_KEY', + ); + expect(logMessage).toContain( + 'models.json.providers["custom-provider"].modelOverrides["model-b"].headers["x-override-key"]: OVERRIDE_API_KEY -> $OVERRIDE_API_KEY', + ); + }); +}); diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index 03de2d77..2e7b0c01 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -36,7 +36,7 @@ describe("ExtensionRunner", () => { const providerModelConfig: ProviderConfig = { baseUrl: "https://provider.test/v1", - apiKey: "PROVIDER_TEST_KEY", + apiKey: "provider-test-key", api: "openai-completions", models: [ { diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts index ad706ab5..95b70fbd 100644 --- a/packages/coding-agent/test/model-registry.test.ts +++ b/packages/coding-agent/test/model-registry.test.ts @@ -4,9 +4,10 @@ import { join } from "node:path"; import type { AnthropicMessagesCompat, Api, Context, Model, OpenAICompletionsCompat } from "@earendil-works/pi-ai"; import { getApiProvider } from "@earendil-works/pi-ai"; import { getOAuthProvider } from "@earendil-works/pi-ai/oauth"; -import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { AuthStorage } from "../src/core/auth-storage.ts"; import { clearApiKeyCache, ModelRegistry, type ProviderConfigInput } from "../src/core/model-registry.ts"; +import { clearDeprecationWarningsForTests } from "../src/utils/deprecation.ts"; describe("ModelRegistry", () => { let tempDir: string; @@ -18,6 +19,7 @@ describe("ModelRegistry", () => { mkdirSync(tempDir, { recursive: true }); modelsJsonPath = join(tempDir, "models.json"); authStorage = AuthStorage.create(join(tempDir, "auth.json")); + clearDeprecationWarningsForTests(); }); afterEach(() => { @@ -25,6 +27,8 @@ describe("ModelRegistry", () => { rmSync(tempDir, { recursive: true }); } clearApiKeyCache(); + clearDeprecationWarningsForTests(); + vi.restoreAllMocks(); }); /** Create minimal provider config */ @@ -35,7 +39,7 @@ describe("ModelRegistry", () => { ): ProviderConfigInput { return { baseUrl, - apiKey: "TEST_KEY", + apiKey: "test-key", api: api as Api, models: models.map((m) => ({ id: m.id, @@ -852,7 +856,7 @@ describe("ModelRegistry", () => { registry.registerProvider("named-provider", { name: "Named Provider", baseUrl: "https://provider.test/v1", - apiKey: "TEST_KEY", + apiKey: "test-key", api: "openai-completions", models: [ { @@ -892,6 +896,30 @@ describe("ModelRegistry", () => { expect(registry.getProviderDisplayName("oauth-provider")).toBe("OAuth Provider"); }); + test("registerProvider warns and temporarily treats uppercase apiKey as an env reference", async () => { + const originalEnv = process.env.CUSTOM_NAME; + process.env.CUSTOM_NAME = "legacy-env-key"; + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + try { + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + + registry.registerProvider("legacy-provider", { + ...providerConfig("https://provider.test/v1", [{ id: "demo-model" }], "openai-completions"), + apiKey: "CUSTOM_NAME", + }); + + expect(await registry.getApiKeyForProvider("legacy-provider")).toBe("legacy-env-key"); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Pass "$CUSTOM_NAME" instead')); + } finally { + if (originalEnv === undefined) { + delete process.env.CUSTOM_NAME; + } else { + process.env.CUSTOM_NAME = originalEnv; + } + } + }); + test("failed registerProvider does not persist invalid streamSimple config", () => { const registry = ModelRegistry.create(authStorage, modelsJsonPath); @@ -911,7 +939,7 @@ describe("ModelRegistry", () => { registry.registerProvider("demo-provider", { baseUrl: "https://provider.test/v1", - apiKey: "TEST_KEY", + apiKey: "test-key", api: "openai-completions", models: [ { @@ -931,7 +959,7 @@ describe("ModelRegistry", () => { expect(() => registry.registerProvider("demo-provider", { baseUrl: "https://provider.test/v2", - apiKey: "TEST_KEY", + apiKey: "test-key", models: [ { id: "broken-model", @@ -1187,7 +1215,117 @@ describe("ModelRegistry", () => { expect(apiKey).toBeUndefined(); }); - test("apiKey as environment variable name resolves to env value", async () => { + test("apiKey with $ prefix resolves to env value", async () => { + const originalEnv = process.env.TEST_API_KEY_12345; + process.env.TEST_API_KEY_12345 = "env-api-key-value"; + + try { + writeRawModelsJson({ + "custom-provider": providerWithApiKey("$TEST_API_KEY_12345"), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + const apiKey = await registry.getApiKeyForProvider("custom-provider"); + + expect(apiKey).toBe("env-api-key-value"); + } finally { + if (originalEnv === undefined) { + delete process.env.TEST_API_KEY_12345; + } else { + process.env.TEST_API_KEY_12345 = originalEnv; + } + } + }); + + test("apiKey with braced env syntax resolves to env value", async () => { + const originalEnv = process.env.TEST_BRACED_API_KEY_12345; + process.env.TEST_BRACED_API_KEY_12345 = "braced-env-api-key-value"; + const bracedKey = "$" + "{TEST_BRACED_API_KEY_12345}"; + + try { + writeRawModelsJson({ + "custom-provider": providerWithApiKey(bracedKey), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + const apiKey = await registry.getApiKeyForProvider("custom-provider"); + + expect(apiKey).toBe("braced-env-api-key-value"); + } finally { + if (originalEnv === undefined) { + delete process.env.TEST_BRACED_API_KEY_12345; + } else { + process.env.TEST_BRACED_API_KEY_12345 = originalEnv; + } + } + }); + + test("apiKey interpolates braced env references inside literals", async () => { + const originalPartA = process.env.TEST_INTERPOLATED_PART_A_12345; + const originalPartB = process.env.TEST_INTERPOLATED_PART_B_12345; + process.env.TEST_INTERPOLATED_PART_A_12345 = "left"; + process.env.TEST_INTERPOLATED_PART_B_12345 = "right"; + const interpolatedKey = ["$", "{TEST_INTERPOLATED_PART_A_12345}_$", "{TEST_INTERPOLATED_PART_B_12345}"].join( + "", + ); + + try { + writeRawModelsJson({ + "custom-provider": providerWithApiKey(interpolatedKey), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + const apiKey = await registry.getApiKeyForProvider("custom-provider"); + + expect(apiKey).toBe("left_right"); + } finally { + if (originalPartA === undefined) { + delete process.env.TEST_INTERPOLATED_PART_A_12345; + } else { + process.env.TEST_INTERPOLATED_PART_A_12345 = originalPartA; + } + if (originalPartB === undefined) { + delete process.env.TEST_INTERPOLATED_PART_B_12345; + } else { + process.env.TEST_INTERPOLATED_PART_B_12345 = originalPartB; + } + } + }); + + test("apiKey with $$ prefix escapes a leading dollar", async () => { + writeRawModelsJson({ + "custom-provider": providerWithApiKey("$$TEST_API_KEY_12345"), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + const apiKey = await registry.getApiKeyForProvider("custom-provider"); + + expect(apiKey).toBe("$TEST_API_KEY_12345"); + }); + + test("apiKey with $! escapes a literal bang and still interpolates later env refs", async () => { + const originalEnv = process.env.TEST_API_KEY_12345; + process.env.TEST_API_KEY_12345 = "env-api-key-value"; + + try { + writeRawModelsJson({ + "custom-provider": providerWithApiKey("$!literal-$TEST_API_KEY_12345"), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + const apiKey = await registry.getApiKeyForProvider("custom-provider"); + + expect(apiKey).toBe("!literal-env-api-key-value"); + } finally { + if (originalEnv === undefined) { + delete process.env.TEST_API_KEY_12345; + } else { + process.env.TEST_API_KEY_12345 = originalEnv; + } + } + }); + + test("plain apiKey is used directly even when it matches an env var", async () => { const originalEnv = process.env.TEST_API_KEY_12345; process.env.TEST_API_KEY_12345 = "env-api-key-value"; @@ -1199,7 +1337,7 @@ describe("ModelRegistry", () => { const registry = ModelRegistry.create(authStorage, modelsJsonPath); const apiKey = await registry.getApiKeyForProvider("custom-provider"); - expect(apiKey).toBe("env-api-key-value"); + expect(apiKey).toBe("TEST_API_KEY_12345"); } finally { if (originalEnv === undefined) { delete process.env.TEST_API_KEY_12345; @@ -1318,7 +1456,7 @@ describe("ModelRegistry", () => { process.env[envVarName] = "status-test-key"; writeRawModelsJson({ - "custom-provider": providerWithApiKey(envVarName), + "custom-provider": providerWithApiKey(`$${envVarName}`), }); const registry = ModelRegistry.create(authStorage, modelsJsonPath); @@ -1337,6 +1475,41 @@ describe("ModelRegistry", () => { } }); + test("provider auth status reports interpolated apiKey environment variables", () => { + const envVarNameA = "TEST_API_KEY_STATUS_PART_A_98765"; + const envVarNameB = "TEST_API_KEY_STATUS_PART_B_98765"; + const originalEnvA = process.env[envVarNameA]; + const originalEnvB = process.env[envVarNameB]; + process.env[envVarNameA] = "left"; + process.env[envVarNameB] = "right"; + const interpolatedKey = ["$", "{", envVarNameA, "}_$", "{", envVarNameB, "}"].join(""); + + try { + writeRawModelsJson({ + "custom-provider": providerWithApiKey(interpolatedKey), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + + expect(registry.getProviderAuthStatus("custom-provider")).toEqual({ + configured: true, + source: "environment", + label: `${envVarNameA}, ${envVarNameB}`, + }); + } finally { + if (originalEnvA === undefined) { + delete process.env[envVarNameA]; + } else { + process.env[envVarNameA] = originalEnvA; + } + if (originalEnvB === undefined) { + delete process.env[envVarNameB]; + } else { + process.env[envVarNameB] = originalEnvB; + } + } + }); + test("provider auth status reports non-env apiKey values from models.json as a config key", () => { writeRawModelsJson({ "custom-provider": providerWithApiKey("literal_api_key_value"), @@ -1350,6 +1523,29 @@ describe("ModelRegistry", () => { }); }); + test("missing explicit env apiKey keeps provider unavailable", () => { + const envVarName = "TEST_API_KEY_MISSING_TEST_98765"; + const originalEnv = process.env[envVarName]; + delete process.env[envVarName]; + + try { + writeRawModelsJson({ + "custom-provider": providerWithApiKey(`$${envVarName}`), + }); + + const registry = ModelRegistry.create(authStorage, modelsJsonPath); + + expect(registry.getProviderAuthStatus("custom-provider")).toEqual({ configured: false }); + expect(registry.getAvailable().some((model) => model.provider === "custom-provider")).toBe(false); + } finally { + if (originalEnv === undefined) { + delete process.env[envVarName]; + } else { + process.env[envVarName] = originalEnv; + } + } + }); + test("provider auth status reports command apiKey values from models.json without executing them", () => { const counterFile = join(tempDir, "status-counter"); writeFileSync(counterFile, "0"); @@ -1376,7 +1572,7 @@ describe("ModelRegistry", () => { process.env[envVarName] = "first-value"; writeRawModelsJson({ - "custom-provider": providerWithApiKey(envVarName), + "custom-provider": providerWithApiKey(`$${envVarName}`), }); const registry = ModelRegistry.create(authStorage, modelsJsonPath);