From 1cc2247794d84e318496c30782bdf51267aa1096 Mon Sep 17 00:00:00 2001 From: Kevin Thomas Date: Wed, 9 Sep 2026 16:11:09 -0700 Subject: [PATCH] fix(ai): gate live test credentials behind an explicit opt-in The packages/ai test OAuth helper read ~/.pi/agent/auth.json, refreshed expired OAuth entries against the real provider, and wrote the refreshed tokens back, at import time in every live test module. It now returns undefined without touching the filesystem or network unless PI_LIVE_TESTS=1 is set and PI_TEST_AUTH_FILE names a dedicated test credential file; the real stores under ~/.prime/agent and ~/.pi/agent are refused. test.sh moves both stores aside and clears the opt-in. Linear: ENG-5347 --- CONTRIBUTING.md | 2 +- .../eng-5347-test-credential-isolation.md | 1 + packages/ai/README.md | 23 ++ packages/ai/test/oauth-test-helper.test.ts | 200 ++++++++++++++++++ packages/ai/test/oauth.ts | 66 +++++- packages/coding-agent/docs/development.md | 2 + test.sh | 37 ++-- 7 files changed, 307 insertions(+), 24 deletions(-) create mode 100644 packages/ai/.changes/eng-5347-test-credential-isolation.md create mode 100644 packages/ai/test/oauth-test-helper.test.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cb693bd65b..739f3cb6aa 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,7 +35,7 @@ If a maintainer has invited a pull request: 1. Keep the change focused on the accepted Issue or Discussion. 2. Follow the repository's development rules and existing conventions. 3. Add or update tests for behavioral changes. -4. Run the relevant checks locally and describe the validation in the pull request. +4. Run the relevant checks locally and describe the validation in the pull request. Tests must not read or modify real credential stores; live provider tests stay behind the `PI_LIVE_TESTS=1` / `PI_TEST_AUTH_FILE` opt-in described in `packages/ai/README.md`. 5. Avoid unrelated refactors or dependency changes. Development setup and commands are documented in the [development guide](packages/coding-agent/docs/development.md). diff --git a/packages/ai/.changes/eng-5347-test-credential-isolation.md b/packages/ai/.changes/eng-5347-test-credential-isolation.md new file mode 100644 index 0000000000..a5948d32ae --- /dev/null +++ b/packages/ai/.changes/eng-5347-test-credential-isolation.md @@ -0,0 +1 @@ +- Changed the live provider tests to require the `PI_LIVE_TESTS=1` opt-in and a dedicated `PI_TEST_AUTH_FILE`, so running the test suite no longer reads, refreshes, or rewrites credentials in `~/.prime/agent` or `~/.pi/agent`. diff --git a/packages/ai/README.md b/packages/ai/README.md index a35c679302..949a6b3e92 100644 --- a/packages/ai/README.md +++ b/packages/ai/README.md @@ -1240,6 +1240,29 @@ const response = await complete(model, { ## Development +### Running tests + +Run focused tests from the package root: + +```bash +cd packages/ai +npx tsx ../../node_modules/vitest/dist/cli.js --run test/specific.test.ts +``` + +Tests that talk to live providers are opt-in and skip by default. Without the opt-in the suite never reads, refreshes, or rewrites the credentials in `~/.prime/agent/auth.json` or the legacy `~/.pi/agent/auth.json`. + +To run the live suites against OAuth-backed providers (Anthropic Claude Pro/Max, GitHub Copilot, OpenAI Codex), copy the entries you want to test into a dedicated file and point the tests at it: + +```bash +# Same JSON shape as the agent's auth.json: +# { "anthropic": { "type": "oauth", "access": "...", "refresh": "...", "expires": 0 }, +# "openai": { "type": "api_key", "key": "sk-..." } } +export PI_LIVE_TESTS=1 +export PI_TEST_AUTH_FILE=/path/to/test-auth.json +``` + +`PI_TEST_AUTH_FILE` must be a separate copy: the helper refuses to use the real credential stores. Refreshed OAuth tokens are written back to that test file only, so keep it if a provider rotates refresh tokens. Suites gated on provider environment variables (`ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, ...) run whenever the variable is set; the root `test.sh` unsets them, moves both credential stores aside, and clears the live-test opt-in before running the full suite. + ### Adding a New Provider Adding a new LLM provider requires changes across multiple files. This checklist covers all necessary steps: diff --git a/packages/ai/test/oauth-test-helper.test.ts b/packages/ai/test/oauth-test-helper.test.ts new file mode 100644 index 0000000000..bb2e063f32 --- /dev/null +++ b/packages/ai/test/oauth-test-helper.test.ts @@ -0,0 +1,200 @@ +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { LIVE_TESTS_ENV, resolveApiKey, TEST_AUTH_FILE_ENV } from "./oauth.js"; + +vi.mock("fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + existsSync: vi.fn(actual.existsSync), + readFileSync: vi.fn(actual.readFileSync), + writeFileSync: vi.fn(actual.writeFileSync), + }; +}); + +import type * as FsModule from "fs"; + +const LEGACY_REFRESH = "SENTINEL-LEGACY-PI-REFRESH-5347"; +const CURRENT_REFRESH = "SENTINEL-CURRENT-PRIME-REFRESH-5347"; +const TEST_REFRESH = "SENTINEL-TEST-FILE-REFRESH-5347"; + +type FetchCall = { url: string; body: string | undefined }; + +let home: string; +let legacyStore: string; +let currentStore: string; +let testAuthFile: string; +let fetchCalls: FetchCall[]; + +function pathOf(arg: unknown): string { + return typeof arg === "string" ? arg : String(arg); +} + +function fsPaths(fn: typeof existsSync | typeof readFileSync | typeof writeFileSync): string[] { + return vi.mocked(fn).mock.calls.map((call) => pathOf(call[0])); +} + +function storePaths(paths: string[]): string[] { + return paths.filter((p) => p.startsWith(join(home, ".pi")) || p.startsWith(join(home, ".prime"))); +} + +function clearFsCalls(): void { + vi.mocked(existsSync).mockClear(); + vi.mocked(readFileSync).mockClear(); + vi.mocked(writeFileSync).mockClear(); +} + +function oauthEntry(refresh: string, expires: number) { + return { type: "oauth", access: `${refresh}-ACCESS`, refresh, expires }; +} + +function snapshot(path: string) { + return { content: readFileSync(path, "utf-8"), mode: statSync(path).mode & 0o777 }; +} + +describe("test OAuth credential helper", () => { + beforeEach(() => { + home = mkdtempSync(join(tmpdir(), "eng5347-home-")); + vi.stubEnv("HOME", home); + vi.stubEnv("USERPROFILE", home); + vi.stubEnv(LIVE_TESTS_ENV, ""); + vi.stubEnv(TEST_AUTH_FILE_ENV, ""); + delete process.env[LIVE_TESTS_ENV]; + delete process.env[TEST_AUTH_FILE_ENV]; + + legacyStore = join(home, ".pi", "agent", "auth.json"); + currentStore = join(home, ".prime", "agent", "auth.json"); + testAuthFile = join(home, "test-fixtures", "auth.json"); + for (const path of [legacyStore, currentStore, testAuthFile]) { + mkdirSync(join(path, ".."), { recursive: true }); + } + writeFileSync(legacyStore, JSON.stringify({ anthropic: oauthEntry(LEGACY_REFRESH, 1) })); + chmodSync(legacyStore, 0o600); + writeFileSync(currentStore, JSON.stringify({ anthropic: oauthEntry(CURRENT_REFRESH, 1) })); + chmodSync(currentStore, 0o600); + writeFileSync( + testAuthFile, + JSON.stringify({ + anthropic: oauthEntry(TEST_REFRESH, 1), + openai: { type: "api_key", key: "sk-test-file-openai-5347" }, + }), + ); + + fetchCalls = []; + vi.stubGlobal("fetch", async (input: string | URL | Request, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + fetchCalls.push({ url, body: typeof init?.body === "string" ? init.body : undefined }); + return new Response( + JSON.stringify({ + access_token: "REFRESHED-ACCESS-5347", + refresh_token: "REFRESHED-REFRESH-5347", + expires_in: 3600, + }), + { status: 200, headers: { "Content-Type": "application/json" } }, + ); + }); + + clearFsCalls(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + rmSync(home, { recursive: true, force: true }); + }); + + it("does nothing without the opt-in, even when a test auth file is configured", async () => { + vi.stubEnv(TEST_AUTH_FILE_ENV, testAuthFile); + const legacyBefore = snapshot(legacyStore); + const currentBefore = snapshot(currentStore); + const testBefore = snapshot(testAuthFile); + clearFsCalls(); + + await expect(resolveApiKey("anthropic")).resolves.toBeUndefined(); + await expect(resolveApiKey("openai")).resolves.toBeUndefined(); + + expect(fetchCalls).toEqual([]); + expect(fsPaths(existsSync)).toEqual([]); + expect(fsPaths(readFileSync)).toEqual([]); + expect(fsPaths(writeFileSync)).toEqual([]); + expect(snapshot(legacyStore)).toEqual(legacyBefore); + expect(snapshot(currentStore)).toEqual(currentBefore); + expect(snapshot(testAuthFile)).toEqual(testBefore); + }); + + it("does nothing with the opt-in when no test auth file is configured", async () => { + vi.stubEnv(LIVE_TESTS_ENV, "1"); + + await expect(resolveApiKey("anthropic")).resolves.toBeUndefined(); + + expect(fetchCalls).toEqual([]); + expect(fsPaths(existsSync)).toEqual([]); + expect(fsPaths(readFileSync)).toEqual([]); + expect(fsPaths(writeFileSync)).toEqual([]); + expect(readFileSync(legacyStore, "utf-8")).toContain(LEGACY_REFRESH); + expect(readFileSync(currentStore, "utf-8")).toContain(CURRENT_REFRESH); + }); + + it("reads only the test auth file with the opt-in and writes refreshed tokens back there only", async () => { + vi.stubEnv(LIVE_TESTS_ENV, "1"); + vi.stubEnv(TEST_AUTH_FILE_ENV, testAuthFile); + const legacyBefore = snapshot(legacyStore); + const currentBefore = snapshot(currentStore); + clearFsCalls(); + + await expect(resolveApiKey("openai")).resolves.toBe("sk-test-file-openai-5347"); + expect(fetchCalls).toEqual([]); + + await expect(resolveApiKey("anthropic")).resolves.toBe("REFRESHED-ACCESS-5347"); + + expect(fetchCalls).toHaveLength(1); + expect(new URL(fetchCalls[0].url).host).toBe("platform.claude.com"); + expect(fetchCalls[0].body).toContain(TEST_REFRESH); + expect(fetchCalls[0].body).not.toContain(LEGACY_REFRESH); + expect(fetchCalls[0].body).not.toContain(CURRENT_REFRESH); + + expect(storePaths(fsPaths(existsSync))).toEqual([]); + expect(storePaths(fsPaths(readFileSync))).toEqual([]); + expect(fsPaths(readFileSync)).toEqual([testAuthFile, testAuthFile]); + expect(fsPaths(writeFileSync)).toEqual([testAuthFile]); + + const testAfter = JSON.parse(readFileSync(testAuthFile, "utf-8")); + expect(testAfter.anthropic.refresh).toBe("REFRESHED-REFRESH-5347"); + expect(testAfter.anthropic.access).toBe("REFRESHED-ACCESS-5347"); + expect(testAfter.openai).toEqual({ type: "api_key", key: "sk-test-file-openai-5347" }); + expect(statSync(testAuthFile).mode & 0o777).toBe(0o600); + expect(snapshot(legacyStore)).toEqual(legacyBefore); + expect(snapshot(currentStore)).toEqual(currentBefore); + }); + + it("refuses a test auth file that points at a real credential store", async () => { + vi.stubEnv(LIVE_TESTS_ENV, "1"); + + for (const store of [currentStore, legacyStore]) { + vi.stubEnv(TEST_AUTH_FILE_ENV, store); + await expect(resolveApiKey("anthropic")).rejects.toThrow(/dedicated test credential file/); + } + + expect(fetchCalls).toEqual([]); + expect(fsPaths(existsSync)).toEqual([]); + expect(fsPaths(readFileSync)).toEqual([]); + expect(fsPaths(writeFileSync)).toEqual([]); + expect(readFileSync(legacyStore, "utf-8")).toContain(LEGACY_REFRESH); + expect(readFileSync(currentStore, "utf-8")).toContain(CURRENT_REFRESH); + }); + + it("returns undefined for a missing test auth file without touching the real stores", async () => { + vi.stubEnv(LIVE_TESTS_ENV, "1"); + const missing = join(home, "test-fixtures", "missing.json"); + vi.stubEnv(TEST_AUTH_FILE_ENV, missing); + + await expect(resolveApiKey("anthropic")).resolves.toBeUndefined(); + + expect(fetchCalls).toEqual([]); + expect(fsPaths(existsSync)).toEqual([missing]); + expect(fsPaths(readFileSync)).toEqual([]); + expect(fsPaths(writeFileSync)).toEqual([]); + }); +}); diff --git a/packages/ai/test/oauth.ts b/packages/ai/test/oauth.ts index ade53377cc..fd1566d25a 100644 --- a/packages/ai/test/oauth.ts +++ b/packages/ai/test/oauth.ts @@ -1,10 +1,22 @@ import { chmodSync, existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; import { homedir } from "os"; -import { dirname, join } from "path"; +import { dirname, join, resolve } from "path"; import { getOAuthApiKey } from "../src/utils/oauth/index.js"; import type { OAuthCredentials, OAuthProvider } from "../src/utils/oauth/types.js"; -const AUTH_PATH = join(homedir(), ".pi", "agent", "auth.json"); +/** + * Credential helper for the live provider tests in this package. + * + * Live tests are opt-in. Without `PI_LIVE_TESTS=1` this helper returns `undefined` + * without touching the filesystem or the network, so every `describe.skipIf(!token)` + * gate skips. With the opt-in, credentials are read from the file named by + * `PI_TEST_AUTH_FILE` (same JSON shape as the agent's `auth.json`), never from the + * developer's real credential stores under `~/.prime/agent` or `~/.pi/agent`. + * Refreshed OAuth tokens are written back to the test file only. + */ + +export const LIVE_TESTS_ENV = "PI_LIVE_TESTS"; +export const TEST_AUTH_FILE_ENV = "PI_TEST_AUTH_FILE"; type ApiKeyCredential = { type: "api_key"; @@ -19,29 +31,61 @@ type AuthCredential = ApiKeyCredential | OAuthCredentialEntry; type AuthStorage = Record; -function loadAuthStorage(): AuthStorage { - if (!existsSync(AUTH_PATH)) { +export function liveTestsEnabled(): boolean { + return process.env[LIVE_TESTS_ENV] === "1"; +} + +function realCredentialStores(): string[] { + const home = homedir(); + return [join(home, ".prime", "agent", "auth.json"), join(home, ".pi", "agent", "auth.json")].map((p) => resolve(p)); +} + +/** + * Path of the test-designated credential file, or `undefined` when live tests are + * not enabled or no file is configured. Throws if the configured file is one of the + * real credential stores, so a misconfiguration fails loudly instead of rewriting + * the developer's credentials. + */ +export function getTestAuthFile(): string | undefined { + if (!liveTestsEnabled()) return undefined; + const configured = process.env[TEST_AUTH_FILE_ENV]?.trim(); + if (!configured) return undefined; + const path = resolve(configured); + if (realCredentialStores().includes(path)) { + throw new Error( + `${TEST_AUTH_FILE_ENV} must point at a dedicated test credential file, not the real credential store at ${path}. ` + + `Copy the entries you want to test into a separate file.`, + ); + } + return path; +} + +function loadAuthStorage(path: string): AuthStorage { + if (!existsSync(path)) { return {}; } try { - const content = readFileSync(AUTH_PATH, "utf-8"); + const content = readFileSync(path, "utf-8"); return JSON.parse(content); } catch { return {}; } } -function saveAuthStorage(storage: AuthStorage): void { - const configDir = dirname(AUTH_PATH); +function saveAuthStorage(path: string, storage: AuthStorage): void { + const configDir = dirname(path); if (!existsSync(configDir)) { mkdirSync(configDir, { recursive: true, mode: 0o700 }); } - writeFileSync(AUTH_PATH, JSON.stringify(storage, null, 2), "utf-8"); - chmodSync(AUTH_PATH, 0o600); + writeFileSync(path, JSON.stringify(storage, null, 2), "utf-8"); + chmodSync(path, 0o600); } export async function resolveApiKey(provider: string): Promise { - const storage = loadAuthStorage(); + const path = getTestAuthFile(); + if (!path) return undefined; + + const storage = loadAuthStorage(path); const entry = storage[provider]; if (!entry) return undefined; @@ -63,7 +107,7 @@ export async function resolveApiKey(provider: string): Promise