diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 5e64524fc..7d59269e4 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -14,6 +14,7 @@ - Updated internal `coerceServiceTierByFamily` helper to facilitate migration from legacy settings ### Fixed +- Changed API-key resolution precedence so an explicit environment variable (e.g. `GEMINI_API_KEY`) overrides a stored/broker-migrated static API key; a deliberate OAuth login still takes precedence over the env var. - Improved Vertex AI reliability by automatically falling back to global endpoints on 404 errors diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index 6060fcb76..d6d91c9f0 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -1736,8 +1736,8 @@ export class AuthStorage { /** * Classify where a provider's auth comes from, following the same precedence * as {@link AuthStorage.getApiKey}: runtime override → config override → - * stored credential (api_key before oauth, matching getApiKey) → env var → - * fallback resolver. Returns undefined when no auth is configured. + * stored OAuth → env var → stored api_key → fallback resolver. Returns + * undefined when no auth is configured. * * Compact, structured counterpart to {@link describeCredentialSource}. */ @@ -1745,10 +1745,9 @@ export class AuthStorage { if (this.#runtimeOverrides.has(provider)) return { kind: "runtime" }; if (this.#configOverrides.has(provider)) return { kind: "config" }; const stored = this.#getCredentialsForProvider(provider); - if (stored.length > 0) { - return { kind: stored.some(credential => credential.type === "api_key") ? "api_key" : "oauth" }; - } + if (stored.some(credential => credential.type === "oauth")) return { kind: "oauth" }; if (getEnvApiKey(provider)) return { kind: "env", envVar: getEnvApiKeyName(provider) }; + if (stored.some(credential => credential.type === "api_key")) return { kind: "api_key" }; if (this.#fallbackResolver?.(provider)) return { kind: "fallback" }; return undefined; } @@ -3760,12 +3759,8 @@ export class AuthStorage { return configKey; } - const apiKeySelection = this.#selectCredentialByType(provider, "api_key"); - if (apiKeySelection) { - return this.#configValueResolver(apiKeySelection.credential.key); - } - - // Return current OAuth access token only if it is not already expired. + // Precedence: a deliberate OAuth login wins, then an explicit env var, then a stored + // static api_key (which may be a stale broker-migrated copy) as a last resort. const oauthSelection = this.#selectCredentialByType(provider, "oauth"); if (oauthSelection) { const expiresAt = oauthSelection.credential.expires; @@ -3784,17 +3779,22 @@ export class AuthStorage { const envKey = getEnvApiKey(provider); if (envKey) return envKey; + const apiKeySelection = this.#selectCredentialByType(provider, "api_key"); + if (apiKeySelection) { + return this.#configValueResolver(apiKeySelection.credential.key); + } + return this.#fallbackResolver?.(provider) ?? undefined; } /** * Get API key for a provider. - * Priority: + * Priority (first match wins): * 1. Runtime override (CLI --api-key) * 2. Config override (models.yml `providers..apiKey`) - * 3. API key from storage - * 4. OAuth token from storage (auto-refreshed) - * 5. Environment variable + * 3. OAuth token from storage (auto-refreshed) + * 4. Environment variable + * 5. Stored API key (e.g. a broker-migrated copy) — last resort, so an explicit env var wins * 6. Fallback resolver (models.yml custom providers, last-resort) */ async getApiKey(provider: string, sessionId?: string, options?: AuthApiKeyOptions): Promise { @@ -3814,25 +3814,27 @@ export class AuthStorage { return configKey; } - const apiKeySelection = this.#selectCredentialByType(provider, "api_key", sessionId); - if (apiKeySelection) { - this.#recordSessionCredential(provider, sessionId, "api_key", apiKeySelection.index); - return this.#configValueResolver(apiKeySelection.credential.key); - } - + // Precedence: a deliberate OAuth login wins, then an explicit env var, then a stored + // static api_key (which may be a stale broker-migrated copy) as a last resort. const oauthResolved = await this.#resolveOAuthSelection(provider, sessionId, options); if (oauthResolved) { return oauthResolved.apiKey; } - // Fall back to environment variable or custom resolver. If we reach here after - // an OAuth miss, the session sticky (if any) is stale — the request will - // authenticate via env/fallback, not OAuth, so clear the sticky now so that - // getOAuthAccountId() correctly suppresses account_uuid for this session. + // Past OAuth: the session sticky (if any) is stale — the request authenticates via + // env/api_key/fallback, not OAuth, so clear it now so getOAuthAccountId() correctly + // suppresses account_uuid for this session. if (sessionId) this.#sessionLastCredential.get(provider)?.delete(sessionId); + const envKey = getEnvApiKey(provider); if (envKey) return envKey; + const apiKeySelection = this.#selectCredentialByType(provider, "api_key", sessionId); + if (apiKeySelection) { + this.#recordSessionCredential(provider, sessionId, "api_key", apiKeySelection.index); + return this.#configValueResolver(apiKeySelection.credential.key); + } + // Fall back to custom resolver (e.g., models.json custom providers) return this.#fallbackResolver?.(provider) ?? undefined; } @@ -4486,12 +4488,13 @@ export class AuthStorage { /** * Describe where the active credential for a provider came from. * - * Surfaces four layers, highest precedence first: + * Mirrors {@link AuthStorage.getApiKey} precedence, highest first: * 1. Runtime override (`--api-key`). * 2. Config override (`models.yml` `providers..apiKey`). - * 3. Stored credential (the one this session is currently sticky to, or the - * one round-robin would pick next when no session id is supplied). - * 4. Env var / fallback resolver — when no stored credential exists. + * 3. Stored OAuth credential. + * 4. Env var — overrides a stored static api_key (e.g. a stale broker copy). + * 5. Stored api_key credential. + * 6. Fallback resolver. * * The string is purely informational; consumers must not parse it. */ @@ -4505,30 +4508,31 @@ export class AuthStorage { const baseLabel = this.#sourceLabel ?? "local store"; const stored = this.#getStoredCredentials(provider); - if (stored.length === 0) { - if (getEnvApiKey(provider)) return `env ${baseLabel ? `(fallback over ${baseLabel})` : ""}`.trim(); - if (this.#fallbackResolver?.(provider) !== undefined) return `fallback resolver`; - return undefined; - } - const session = sessionId ? this.#sessionLastCredential.get(provider)?.get(sessionId) : undefined; - // Same selection logic as #selectCredentialByType for "no session" lookups: prefer - // the type with stored credentials, lean OAuth before api_key. We don't run the - // full round-robin here because describing the source shouldn't advance the index. - const preferredType: AuthCredential["type"] = - session?.type ?? (stored.some(entry => entry.credential.type === "oauth") ? "oauth" : "api_key"); - const typed = stored - .map((entry, index) => ({ entry, index })) - .filter(({ entry }) => entry.credential.type === preferredType); - if (typed.length === 0) return baseLabel; - const index = session?.index ?? typed[0].index; - const chosen = stored[index] ?? typed[0].entry; - const credential = chosen.credential; - const identity = - credential.type === "oauth" - ? (credential.email ?? credential.accountId ?? credential.projectId ?? `cred ${chosen.id}`) - : `cred ${chosen.id}`; - return `${baseLabel} · ${preferredType} #${chosen.id} (${identity})`; + // Describe the stored credential of a given type, honoring the session sticky index. + const describeStored = (type: AuthCredential["type"]): string | undefined => { + const typed = stored + .map((entry, index) => ({ entry, index })) + .filter(({ entry }) => entry.credential.type === type); + if (typed.length === 0) return undefined; + const index = session?.type === type ? session.index : typed[0].index; + const chosen = stored[index] ?? typed[0].entry; + const credential = chosen.credential; + const identity = + credential.type === "oauth" + ? (credential.email ?? credential.accountId ?? credential.projectId ?? `cred ${chosen.id}`) + : `cred ${chosen.id}`; + return `${baseLabel} · ${type} #${chosen.id} (${identity})`; + }; + + // A deliberate OAuth login wins; then an explicit env var; then a stored static api_key. + const oauthSource = describeStored("oauth"); + if (oauthSource) return oauthSource; + if (getEnvApiKey(provider)) return `env (over ${baseLabel})`; + const apiKeySource = describeStored("api_key"); + if (apiKeySource) return apiKeySource; + if (this.#fallbackResolver?.(provider) !== undefined) return "fallback resolver"; + return undefined; } } diff --git a/packages/ai/test/auth-storage-api-key-login.test.ts b/packages/ai/test/auth-storage-api-key-login.test.ts index e50e0fd4f..879a8a9ec 100644 --- a/packages/ai/test/auth-storage-api-key-login.test.ts +++ b/packages/ai/test/auth-storage-api-key-login.test.ts @@ -8,6 +8,7 @@ import { AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-sto import * as deepseekModule from "@oh-my-pi/pi-ai/registry/deepseek"; import * as kagiModule from "@oh-my-pi/pi-ai/registry/kagi"; import * as ollamaCloudModule from "@oh-my-pi/pi-ai/registry/ollama-cloud"; +import * as aiStream from "@oh-my-pi/pi-ai/stream"; import { removeWithRetries } from "../../utils/src/temp"; function countCredentialRows(dbPath: string, provider: string): number { @@ -38,6 +39,9 @@ function countCredentialRowsByDisabledState(dbPath: string, provider: string, di } describe("AuthStorage api-key login upsert", () => { + // A live env var now (correctly) overrides a stored static api_key. These tests verify that a + // freshly stored api_key resolves through AuthStorage.getApiKey, so neutralize the env leg + // entirely — this ignores every provider's ambient env key, not just the few set locally. let tempDir = ""; let dbPath = ""; let store: SqliteAuthCredentialStore | null = null; @@ -47,6 +51,7 @@ describe("AuthStorage api-key login upsert", () => { let loginOllamaCloudSpy: Mock; beforeEach(async () => { + vi.spyOn(aiStream, "getEnvApiKey").mockReturnValue(undefined); tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "pi-ai-auth-api-key-login-")); dbPath = path.join(tempDir, "agent.db"); store = await SqliteAuthCredentialStore.open(dbPath); diff --git a/packages/ai/test/auth-storage-credential-origin.test.ts b/packages/ai/test/auth-storage-credential-origin.test.ts index 822ffeff4..e3d7b2d10 100644 --- a/packages/ai/test/auth-storage-credential-origin.test.ts +++ b/packages/ai/test/auth-storage-credential-origin.test.ts @@ -68,14 +68,24 @@ describe("AuthStorage.getCredentialOrigin", () => { }); }); - test("a stored api key reports api_key and outranks a co-stored OAuth credential", async () => { + test("a stored OAuth credential outranks a co-stored api key", async () => { await withEnv(SUPPRESS_ENV, async () => { - // getApiKey() prefers api_key before oauth, so the origin must match. + // getApiKey() resolves stored OAuth before a stored api_key, so the origin must match. await auth?.set("openai", [ { type: "oauth", access: "a", refresh: "r", expires: Date.now() + 60_000 }, { type: "api_key", key: "sk-stored" }, ]); - expect(auth?.getCredentialOrigin("openai")).toEqual({ kind: "api_key" }); + expect(auth?.getCredentialOrigin("openai")).toEqual({ kind: "oauth" }); + }); + }); + + test("an explicit env var outranks a stored api key", async () => { + // Regression: a live env var is the user's current choice and must win over a stored + // static api_key (e.g. a stale broker-migrated copy) so `GEMINI_API_KEY` etc. take effect. + await withEnv({ ...SUPPRESS_ENV, OPENAI_API_KEY: "sk-env" }, async () => { + await auth?.set("openai", [{ type: "api_key", key: "sk-stored" }]); + expect(auth?.getCredentialOrigin("openai")).toEqual({ kind: "env", envVar: "OPENAI_API_KEY" }); + expect(await auth?.getApiKey("openai")).toBe("sk-env"); }); });