From 2977c14917becf826f3e4c01ecf4c03f0db528e3 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Tue, 17 Feb 2026 19:57:21 +0100 Subject: [PATCH] refactor(coding-agent): move auth storage to backend abstraction --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/docs/sdk.md | 10 +- .../examples/sdk/02-custom-model.ts | 2 +- .../examples/sdk/09-api-keys-and-oauth.ts | 4 +- .../examples/sdk/12-full-control.ts | 2 +- packages/coding-agent/examples/sdk/README.md | 6 +- .../coding-agent/src/core/auth-storage.ts | 306 ++++++++++++------ packages/coding-agent/src/core/sdk.ts | 4 +- packages/coding-agent/src/index.ts | 10 +- packages/coding-agent/src/main.ts | 2 +- ...gent-session-auto-compaction-queue.test.ts | 2 +- .../test/agent-session-branching.test.ts | 2 +- .../test/agent-session-compaction.test.ts | 2 +- .../test/agent-session-concurrent.test.ts | 4 +- .../coding-agent/test/auth-storage.test.ts | 124 +++++-- .../test/compaction-extensions.test.ts | 2 +- .../test/extensions-input-event.test.ts | 2 +- .../test/extensions-runner.test.ts | 2 +- .../coding-agent/test/model-registry.test.ts | 2 +- packages/coding-agent/test/utilities.ts | 4 +- packages/mom/src/agent.ts | 2 +- 21 files changed, 355 insertions(+), 143 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d87bf471..1e3a54f0 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,10 +5,12 @@ ### Breaking Changes - `SettingsManager` persistence semantics changed for SDK consumers. Setters now update in-memory state immediately and queue disk writes. Code that requires durable on-disk settings must call `await settingsManager.flush()`. +- `AuthStorage` no longer uses direct constructor path convenience in SDK-facing usage. Use static factories (`AuthStorage.create(...)`, `AuthStorage.fromStorage(...)`, `AuthStorage.inMemory(...)`). ### Added - Added `SettingsManager.drainErrors()` for caller-controlled settings I/O error handling without manager-side console output. +- Added auth storage backends (`FileAuthStorageBackend`, `InMemoryAuthStorageBackend`) and `AuthStorage.fromStorage(...)` for storage-first auth persistence wiring. ### Changed @@ -17,6 +19,8 @@ ### Fixed - Fixed project settings persistence to preserve unrelated external edits via merge-on-write, while still applying in-memory changes for modified keys. +- Fixed auth credential persistence to preserve unrelated external edits to `auth.json` via locked read/merge/write updates. +- Fixed auth load/persist error surfacing by buffering errors and exposing them via `AuthStorage.drainErrors()`. ## [0.52.12] - 2026-02-13 diff --git a/packages/coding-agent/docs/sdk.md b/packages/coding-agent/docs/sdk.md index 0437f777..db756330 100644 --- a/packages/coding-agent/docs/sdk.md +++ b/packages/coding-agent/docs/sdk.md @@ -19,7 +19,7 @@ See [examples/sdk/](../examples/sdk/) for working examples from minimal to full import { AuthStorage, createAgentSession, ModelRegistry, SessionManager } from "@mariozechner/pi-coding-agent"; // Set up credential storage and model registry -const authStorage = new AuthStorage(); +const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage); const { session } = await createAgentSession({ @@ -281,7 +281,7 @@ When you pass a custom `ResourceLoader`, `cwd` and `agentDir` no longer control import { getModel } from "@mariozechner/pi-ai"; import { AuthStorage, ModelRegistry } from "@mariozechner/pi-coding-agent"; -const authStorage = new AuthStorage(); +const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage); // Find specific built-in model (doesn't check if API key exists) @@ -329,7 +329,7 @@ API key resolution priority (handled by AuthStorage): import { AuthStorage, ModelRegistry } from "@mariozechner/pi-coding-agent"; // Default: uses ~/.pi/agent/auth.json and ~/.pi/agent/models.json -const authStorage = new AuthStorage(); +const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage); const { session } = await createAgentSession({ @@ -342,7 +342,7 @@ const { session } = await createAgentSession({ authStorage.setRuntimeApiKey("anthropic", "sk-my-temp-key"); // Custom auth storage location -const customAuth = new AuthStorage("/my/app/auth.json"); +const customAuth = AuthStorage.create("/my/app/auth.json"); const customRegistry = new ModelRegistry(customAuth, "/my/app/models.json"); const { session } = await createAgentSession({ @@ -773,7 +773,7 @@ import { } from "@mariozechner/pi-coding-agent"; // Set up auth storage (custom location) -const authStorage = new AuthStorage("/custom/agent/auth.json"); +const authStorage = AuthStorage.create("/custom/agent/auth.json"); // Runtime API key override (not persisted) if (process.env.MY_KEY) { diff --git a/packages/coding-agent/examples/sdk/02-custom-model.ts b/packages/coding-agent/examples/sdk/02-custom-model.ts index 25da4461..ccac7bce 100644 --- a/packages/coding-agent/examples/sdk/02-custom-model.ts +++ b/packages/coding-agent/examples/sdk/02-custom-model.ts @@ -8,7 +8,7 @@ import { getModel } from "@mariozechner/pi-ai"; import { AuthStorage, createAgentSession, ModelRegistry } from "@mariozechner/pi-coding-agent"; // Set up auth storage and model registry -const authStorage = new AuthStorage(); +const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage); // Option 1: Find a specific built-in model by provider/id diff --git a/packages/coding-agent/examples/sdk/09-api-keys-and-oauth.ts b/packages/coding-agent/examples/sdk/09-api-keys-and-oauth.ts index 2e074d08..a41f1e8b 100644 --- a/packages/coding-agent/examples/sdk/09-api-keys-and-oauth.ts +++ b/packages/coding-agent/examples/sdk/09-api-keys-and-oauth.ts @@ -8,7 +8,7 @@ import { AuthStorage, createAgentSession, ModelRegistry, SessionManager } from " // Default: AuthStorage uses ~/.pi/agent/auth.json // ModelRegistry loads built-in + custom models from ~/.pi/agent/models.json -const authStorage = new AuthStorage(); +const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage); await createAgentSession({ @@ -19,7 +19,7 @@ await createAgentSession({ console.log("Session with default auth storage and model registry"); // Custom auth storage location -const customAuthStorage = new AuthStorage("/tmp/my-app/auth.json"); +const customAuthStorage = AuthStorage.create("/tmp/my-app/auth.json"); const customModelRegistry = new ModelRegistry(customAuthStorage, "/tmp/my-app/models.json"); await createAgentSession({ diff --git a/packages/coding-agent/examples/sdk/12-full-control.ts b/packages/coding-agent/examples/sdk/12-full-control.ts index 0e9941a4..3d161dc9 100644 --- a/packages/coding-agent/examples/sdk/12-full-control.ts +++ b/packages/coding-agent/examples/sdk/12-full-control.ts @@ -22,7 +22,7 @@ import { } from "@mariozechner/pi-coding-agent"; // Custom auth storage location -const authStorage = new AuthStorage("/tmp/my-agent/auth.json"); +const authStorage = AuthStorage.create("/tmp/my-agent/auth.json"); // Runtime API key override (not persisted) if (process.env.MY_ANTHROPIC_KEY) { diff --git a/packages/coding-agent/examples/sdk/README.md b/packages/coding-agent/examples/sdk/README.md index 7de0e00e..4a300518 100644 --- a/packages/coding-agent/examples/sdk/README.md +++ b/packages/coding-agent/examples/sdk/README.md @@ -43,7 +43,7 @@ import { } from "@mariozechner/pi-coding-agent"; // Auth and models setup -const authStorage = new AuthStorage(); +const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage); // Minimal @@ -71,7 +71,7 @@ const { session } = await createAgentSession({ }); // Full control -const customAuth = new AuthStorage("/my/app/auth.json"); +const customAuth = AuthStorage.create("/my/app/auth.json"); customAuth.setRuntimeApiKey("anthropic", process.env.MY_KEY!); const customRegistry = new ModelRegistry(customAuth); @@ -108,7 +108,7 @@ await session.prompt("Hello"); | Option | Default | Description | |--------|---------|-------------| -| `authStorage` | `new AuthStorage()` | Credential storage | +| `authStorage` | `AuthStorage.create()` | Credential storage | | `modelRegistry` | `new ModelRegistry(authStorage)` | Model registry | | `cwd` | `process.cwd()` | Working directory | | `agentDir` | `~/.pi/agent` | Config directory | diff --git a/packages/coding-agent/src/core/auth-storage.ts b/packages/coding-agent/src/core/auth-storage.ts index c4dbaa73..9724422b 100644 --- a/packages/coding-agent/src/core/auth-storage.ts +++ b/packages/coding-agent/src/core/auth-storage.ts @@ -34,6 +34,125 @@ export type AuthCredential = ApiKeyCredential | OAuthCredential; export type AuthStorageData = Record; +type LockResult = { + result: T; + next?: string; +}; + +export interface AuthStorageBackend { + withLock(fn: (current: string | undefined) => LockResult): T; + withLockAsync(fn: (current: string | undefined) => Promise>): Promise; +} + +export class FileAuthStorageBackend implements AuthStorageBackend { + constructor(private authPath: string = join(getAgentDir(), "auth.json")) {} + + private ensureParentDir(): void { + const dir = dirname(this.authPath); + if (!existsSync(dir)) { + mkdirSync(dir, { recursive: true, mode: 0o700 }); + } + } + + private ensureFileExists(): void { + if (!existsSync(this.authPath)) { + writeFileSync(this.authPath, "{}", "utf-8"); + chmodSync(this.authPath, 0o600); + } + } + + withLock(fn: (current: string | undefined) => LockResult): T { + this.ensureParentDir(); + this.ensureFileExists(); + + let release: (() => void) | undefined; + try { + release = lockfile.lockSync(this.authPath, { realpath: false }); + const current = existsSync(this.authPath) ? readFileSync(this.authPath, "utf-8") : undefined; + const { result, next } = fn(current); + if (next !== undefined) { + writeFileSync(this.authPath, next, "utf-8"); + chmodSync(this.authPath, 0o600); + } + return result; + } finally { + if (release) { + release(); + } + } + } + + async withLockAsync(fn: (current: string | undefined) => Promise>): Promise { + this.ensureParentDir(); + this.ensureFileExists(); + + let release: (() => Promise) | undefined; + let lockCompromised = false; + let lockCompromisedError: Error | undefined; + const throwIfCompromised = () => { + if (lockCompromised) { + throw lockCompromisedError ?? new Error("Auth storage lock was compromised"); + } + }; + + try { + release = await lockfile.lock(this.authPath, { + retries: { + retries: 10, + factor: 2, + minTimeout: 100, + maxTimeout: 10000, + randomize: true, + }, + stale: 30000, + onCompromised: (err) => { + lockCompromised = true; + lockCompromisedError = err; + }, + }); + + throwIfCompromised(); + const current = existsSync(this.authPath) ? readFileSync(this.authPath, "utf-8") : undefined; + const { result, next } = await fn(current); + throwIfCompromised(); + if (next !== undefined) { + writeFileSync(this.authPath, next, "utf-8"); + chmodSync(this.authPath, 0o600); + } + throwIfCompromised(); + return result; + } finally { + if (release) { + try { + await release(); + } catch { + // Ignore unlock errors when lock is compromised. + } + } + } + } +} + +export class InMemoryAuthStorageBackend implements AuthStorageBackend { + private value: string | undefined; + + withLock(fn: (current: string | undefined) => LockResult): T { + const { result, next } = fn(this.value); + if (next !== undefined) { + this.value = next; + } + return result; + } + + async withLockAsync(fn: (current: string | undefined) => Promise>): Promise { + const { result, next } = await fn(this.value); + if (next !== undefined) { + this.value = next; + } + return result; + } +} + /** * Credential storage backed by a JSON file. */ @@ -41,11 +160,27 @@ export class AuthStorage { private data: AuthStorageData = {}; private runtimeOverrides: Map = new Map(); private fallbackResolver?: (provider: string) => string | undefined; + private loadError: Error | null = null; + private errors: Error[] = []; - constructor(private authPath: string = join(getAgentDir(), "auth.json")) { + private constructor(private storage: AuthStorageBackend) { this.reload(); } + static create(authPath?: string): AuthStorage { + return new AuthStorage(new FileAuthStorageBackend(authPath ?? join(getAgentDir(), "auth.json"))); + } + + static fromStorage(storage: AuthStorageBackend): AuthStorage { + return new AuthStorage(storage); + } + + static inMemory(data: AuthStorageData = {}): AuthStorage { + const storage = new InMemoryAuthStorageBackend(); + storage.withLock(() => ({ result: undefined, next: JSON.stringify(data, null, 2) })); + return AuthStorage.fromStorage(storage); + } + /** * Set a runtime API key override (not persisted to disk). * Used for CLI --api-key flag. @@ -69,31 +204,55 @@ export class AuthStorage { this.fallbackResolver = resolver; } - /** - * Reload credentials from disk. - */ - reload(): void { - if (!existsSync(this.authPath)) { - this.data = {}; - return; - } - try { - this.data = JSON.parse(readFileSync(this.authPath, "utf-8")); - } catch { - this.data = {}; + private recordError(error: unknown): void { + const normalizedError = error instanceof Error ? error : new Error(String(error)); + this.errors.push(normalizedError); + } + + private parseStorageData(content: string | undefined): AuthStorageData { + if (!content) { + return {}; } + return JSON.parse(content) as AuthStorageData; } /** - * Save credentials to disk. + * Reload credentials from storage. */ - private save(): void { - const dir = dirname(this.authPath); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true, mode: 0o700 }); + reload(): void { + let content: string | undefined; + try { + this.storage.withLock((current) => { + content = current; + return { result: undefined }; + }); + this.data = this.parseStorageData(content); + this.loadError = null; + } catch (error) { + this.loadError = error as Error; + this.recordError(error); + } + } + + private persistProviderChange(provider: string, credential: AuthCredential | undefined): void { + if (this.loadError) { + return; + } + + try { + this.storage.withLock((current) => { + const currentData = this.parseStorageData(current); + const merged: AuthStorageData = { ...currentData }; + if (credential) { + merged[provider] = credential; + } else { + delete merged[provider]; + } + return { result: undefined, next: JSON.stringify(merged, null, 2) }; + }); + } catch (error) { + this.recordError(error); } - writeFileSync(this.authPath, JSON.stringify(this.data, null, 2), "utf-8"); - chmodSync(this.authPath, 0o600); } /** @@ -108,7 +267,7 @@ export class AuthStorage { */ set(provider: string, credential: AuthCredential): void { this.data[provider] = credential; - this.save(); + this.persistProviderChange(provider, credential); } /** @@ -116,7 +275,7 @@ export class AuthStorage { */ remove(provider: string): void { delete this.data[provider]; - this.save(); + this.persistProviderChange(provider, undefined); } /** @@ -152,6 +311,12 @@ export class AuthStorage { return { ...this.data }; } + drainErrors(): Error[] { + const drained = [...this.errors]; + this.errors = []; + return drained; + } + /** * Login to an OAuth provider. */ @@ -173,9 +338,8 @@ export class AuthStorage { } /** - * Refresh OAuth token with file locking to prevent race conditions. + * Refresh OAuth token with backend locking to prevent race conditions. * Multiple pi instances may try to refresh simultaneously when tokens expire. - * This ensures only one instance refreshes while others wait and use the result. */ private async refreshOAuthTokenWithLock( providerId: OAuthProviderId, @@ -185,91 +349,42 @@ export class AuthStorage { return null; } - // Ensure auth file exists for locking - if (!existsSync(this.authPath)) { - const dir = dirname(this.authPath); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true, mode: 0o700 }); - } - writeFileSync(this.authPath, "{}", "utf-8"); - chmodSync(this.authPath, 0o600); - } + const result = await this.storage.withLockAsync(async (current) => { + const currentData = this.parseStorageData(current); + this.data = currentData; + this.loadError = null; - let release: (() => Promise) | undefined; - let lockCompromised = false; - let lockCompromisedError: Error | undefined; - const throwIfLockCompromised = () => { - if (lockCompromised) { - throw lockCompromisedError ?? new Error("OAuth refresh lock was compromised"); - } - }; - - try { - // Acquire exclusive lock with retry and timeout - // Use generous retry window to handle slow token endpoints - release = await lockfile.lock(this.authPath, { - retries: { - retries: 10, - factor: 2, - minTimeout: 100, - maxTimeout: 10000, - randomize: true, - }, - stale: 30000, // Consider lock stale after 30 seconds - onCompromised: (err) => { - lockCompromised = true; - lockCompromisedError = err; - }, - }); - - throwIfLockCompromised(); - - // Re-read file after acquiring lock - another instance may have refreshed - this.reload(); - - const cred = this.data[providerId]; + const cred = currentData[providerId]; if (cred?.type !== "oauth") { - return null; + return { result: null }; } - // Check if token is still expired after re-reading - // (another instance may have already refreshed it) if (Date.now() < cred.expires) { - // Token is now valid - another instance refreshed it - throwIfLockCompromised(); - const apiKey = provider.getApiKey(cred); - return { apiKey, newCredentials: cred }; + return { result: { apiKey: provider.getApiKey(cred), newCredentials: cred } }; } - // Token still expired, we need to refresh const oauthCreds: Record = {}; - for (const [key, value] of Object.entries(this.data)) { + for (const [key, value] of Object.entries(currentData)) { if (value.type === "oauth") { oauthCreds[key] = value; } } - const result = await getOAuthApiKey(providerId, oauthCreds); - if (result) { - throwIfLockCompromised(); - this.data[providerId] = { type: "oauth", ...result.newCredentials }; - this.save(); - throwIfLockCompromised(); - return result; + const refreshed = await getOAuthApiKey(providerId, oauthCreds); + if (!refreshed) { + return { result: null }; } - throwIfLockCompromised(); - return null; - } finally { - // Always release the lock - if (release) { - try { - await release(); - } catch { - // Ignore unlock errors (lock may have been compromised) - } - } - } + const merged: AuthStorageData = { + ...currentData, + [providerId]: { type: "oauth", ...refreshed.newCredentials }, + }; + this.data = merged; + this.loadError = null; + return { result: refreshed, next: JSON.stringify(merged, null, 2) }; + }); + + return result; } /** @@ -311,7 +426,8 @@ export class AuthStorage { if (result) { return result.apiKey; } - } catch { + } catch (error) { + this.recordError(error); // Refresh failed - re-read file to check if another instance succeeded this.reload(); const updatedCred = this.data[providerId]; diff --git a/packages/coding-agent/src/core/sdk.ts b/packages/coding-agent/src/core/sdk.ts index cadc676d..103a9b91 100644 --- a/packages/coding-agent/src/core/sdk.ts +++ b/packages/coding-agent/src/core/sdk.ts @@ -44,7 +44,7 @@ export interface CreateAgentSessionOptions { /** Global config directory. Default: ~/.pi/agent */ agentDir?: string; - /** Auth storage for credentials. Default: new AuthStorage(agentDir/auth.json) */ + /** Auth storage for credentials. Default: AuthStorage.create(agentDir/auth.json) */ authStorage?: AuthStorage; /** Model registry. Default: new ModelRegistry(authStorage, agentDir/models.json) */ modelRegistry?: ModelRegistry; @@ -170,7 +170,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} // Use provided or create AuthStorage and ModelRegistry const authPath = options.agentDir ? join(agentDir, "auth.json") : undefined; const modelsPath = options.agentDir ? join(agentDir, "models.json") : undefined; - const authStorage = options.authStorage ?? new AuthStorage(authPath); + const authStorage = options.authStorage ?? AuthStorage.create(authPath); const modelRegistry = options.modelRegistry ?? new ModelRegistry(authStorage, modelsPath); const settingsManager = options.settingsManager ?? SettingsManager.create(cwd, agentDir); diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index b5bf79ba..7a15cc82 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -14,7 +14,15 @@ export { type SessionStats, } from "./core/agent-session.js"; // Auth and model registry -export { type ApiKeyCredential, type AuthCredential, AuthStorage, type OAuthCredential } from "./core/auth-storage.js"; +export { + type ApiKeyCredential, + type AuthCredential, + AuthStorage, + type AuthStorageBackend, + FileAuthStorageBackend, + InMemoryAuthStorageBackend, + type OAuthCredential, +} from "./core/auth-storage.js"; // Compaction export { type BranchPreparation, diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index c7d79630..e090931a 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -554,7 +554,7 @@ export async function main(args: string[]) { const agentDir = getAgentDir(); const settingsManager = SettingsManager.create(cwd, agentDir); reportSettingsErrors(settingsManager, "startup"); - const authStorage = new AuthStorage(); + const authStorage = AuthStorage.create(); const modelRegistry = new ModelRegistry(authStorage, getModelsPath()); const resourceLoader = new DefaultResourceLoader({ diff --git a/packages/coding-agent/test/agent-session-auto-compaction-queue.test.ts b/packages/coding-agent/test/agent-session-auto-compaction-queue.test.ts index 0c99307f..972f2615 100644 --- a/packages/coding-agent/test/agent-session-auto-compaction-queue.test.ts +++ b/packages/coding-agent/test/agent-session-auto-compaction-queue.test.ts @@ -46,7 +46,7 @@ describe("AgentSession auto-compaction queue resume", () => { const sessionManager = SessionManager.inMemory(); const settingsManager = SettingsManager.create(tempDir, tempDir); - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); authStorage.setRuntimeApiKey("anthropic", "test-key"); const modelRegistry = new ModelRegistry(authStorage, tempDir); diff --git a/packages/coding-agent/test/agent-session-branching.test.ts b/packages/coding-agent/test/agent-session-branching.test.ts index a6f161f4..2c89b4bd 100644 --- a/packages/coding-agent/test/agent-session-branching.test.ts +++ b/packages/coding-agent/test/agent-session-branching.test.ts @@ -54,7 +54,7 @@ describe.skipIf(!API_KEY)("AgentSession forking", () => { sessionManager = noSession ? SessionManager.inMemory() : SessionManager.create(tempDir); const settingsManager = SettingsManager.create(tempDir, tempDir); - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = new ModelRegistry(authStorage, tempDir); session = new AgentSession({ diff --git a/packages/coding-agent/test/agent-session-compaction.test.ts b/packages/coding-agent/test/agent-session-compaction.test.ts index 5a03a2a7..b4873bdb 100644 --- a/packages/coding-agent/test/agent-session-compaction.test.ts +++ b/packages/coding-agent/test/agent-session-compaction.test.ts @@ -60,7 +60,7 @@ describe.skipIf(!API_KEY)("AgentSession compaction e2e", () => { const settingsManager = SettingsManager.create(tempDir, tempDir); // Use minimal keepRecentTokens so small test conversations have something to summarize settingsManager.applyOverrides({ compaction: { keepRecentTokens: 1 } }); - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = new ModelRegistry(authStorage); session = new AgentSession({ diff --git a/packages/coding-agent/test/agent-session-concurrent.test.ts b/packages/coding-agent/test/agent-session-concurrent.test.ts index 0115af77..7e9fc17d 100644 --- a/packages/coding-agent/test/agent-session-concurrent.test.ts +++ b/packages/coding-agent/test/agent-session-concurrent.test.ts @@ -99,7 +99,7 @@ describe("AgentSession concurrent prompt guard", () => { const sessionManager = SessionManager.inMemory(); const settingsManager = SettingsManager.create(tempDir, tempDir); - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = new ModelRegistry(authStorage, tempDir); // Set a runtime API key so validation passes authStorage.setRuntimeApiKey("anthropic", "test-key"); @@ -192,7 +192,7 @@ describe("AgentSession concurrent prompt guard", () => { const sessionManager = SessionManager.inMemory(); const settingsManager = SettingsManager.create(tempDir, tempDir); - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = new ModelRegistry(authStorage, tempDir); authStorage.setRuntimeApiKey("anthropic", "test-key"); diff --git a/packages/coding-agent/test/auth-storage.test.ts b/packages/coding-agent/test/auth-storage.test.ts index 0b395538..a91102f6 100644 --- a/packages/coding-agent/test/auth-storage.test.ts +++ b/packages/coding-agent/test/auth-storage.test.ts @@ -36,7 +36,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "sk-ant-literal-key" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("sk-ant-literal-key"); @@ -47,7 +47,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!echo test-api-key-from-command" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("test-api-key-from-command"); @@ -58,7 +58,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!echo ' spaced-key '" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("spaced-key"); @@ -69,7 +69,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!printf 'line1\\nline2'" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("line1\nline2"); @@ -80,7 +80,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!exit 1" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBeUndefined(); @@ -91,7 +91,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!nonexistent-command-12345" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBeUndefined(); @@ -102,7 +102,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!printf ''" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBeUndefined(); @@ -117,7 +117,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "TEST_AUTH_API_KEY_12345" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("env-api-key-value"); @@ -138,7 +138,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "literal_api_key_value" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("literal_api_key_value"); @@ -149,7 +149,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!echo 'hello world' | tr ' ' '-'" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const apiKey = await authStorage.getApiKey("anthropic"); expect(apiKey).toBe("hello-world"); @@ -166,7 +166,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: command }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); // Call multiple times await authStorage.getApiKey("anthropic"); @@ -188,10 +188,10 @@ describe("AuthStorage", () => { }); // Create multiple AuthStorage instances - const storage1 = new AuthStorage(authJsonPath); + const storage1 = AuthStorage.create(authJsonPath); await storage1.getApiKey("anthropic"); - const storage2 = new AuthStorage(authJsonPath); + const storage2 = AuthStorage.create(authJsonPath); await storage2.getApiKey("anthropic"); // Command should still have only run once @@ -208,7 +208,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: command }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); await authStorage.getApiKey("anthropic"); // Clear cache and call again @@ -226,7 +226,7 @@ describe("AuthStorage", () => { openai: { type: "api_key", key: "!echo key-openai" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const keyA = await authStorage.getApiKey("anthropic"); const keyB = await authStorage.getApiKey("openai"); @@ -244,7 +244,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: command }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); // Call multiple times - all should return undefined const key1 = await authStorage.getApiKey("anthropic"); @@ -269,7 +269,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: envVarName }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const key1 = await authStorage.getApiKey("anthropic"); expect(key1).toBe("first-value"); @@ -320,7 +320,7 @@ describe("AuthStorage", () => { }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); const realLock = lockfile.lock.bind(lockfile); const lockSpy = vi.spyOn(lockfile, "lock"); @@ -339,13 +339,97 @@ describe("AuthStorage", () => { }); }); + describe("persistence semantics", () => { + test("set preserves unrelated external edits", () => { + writeAuthJson({ + anthropic: { type: "api_key", key: "old-anthropic" }, + openai: { type: "api_key", key: "openai-key" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + + // Simulate external edit while process is running + writeAuthJson({ + anthropic: { type: "api_key", key: "old-anthropic" }, + openai: { type: "api_key", key: "openai-key" }, + google: { type: "api_key", key: "google-key" }, + }); + + authStorage.set("anthropic", { type: "api_key", key: "new-anthropic" }); + + const updated = JSON.parse(readFileSync(authJsonPath, "utf-8")) as Record; + expect(updated.anthropic.key).toBe("new-anthropic"); + expect(updated.openai.key).toBe("openai-key"); + expect(updated.google.key).toBe("google-key"); + }); + + test("remove preserves unrelated external edits", () => { + writeAuthJson({ + anthropic: { type: "api_key", key: "anthropic-key" }, + openai: { type: "api_key", key: "openai-key" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + + // Simulate external edit while process is running + writeAuthJson({ + anthropic: { type: "api_key", key: "anthropic-key" }, + openai: { type: "api_key", key: "openai-key" }, + google: { type: "api_key", key: "google-key" }, + }); + + authStorage.remove("anthropic"); + + const updated = JSON.parse(readFileSync(authJsonPath, "utf-8")) as Record; + expect(updated.anthropic).toBeUndefined(); + expect(updated.openai.key).toBe("openai-key"); + expect(updated.google.key).toBe("google-key"); + }); + + test("does not overwrite malformed auth file after load error", () => { + writeAuthJson({ + anthropic: { type: "api_key", key: "anthropic-key" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + writeFileSync(authJsonPath, "{invalid-json", "utf-8"); + + authStorage.reload(); + authStorage.set("openai", { type: "api_key", key: "openai-key" }); + + const raw = readFileSync(authJsonPath, "utf-8"); + expect(raw).toBe("{invalid-json"); + }); + + test("reload records parse errors and drainErrors clears buffer", () => { + writeAuthJson({ + anthropic: { type: "api_key", key: "anthropic-key" }, + }); + + authStorage = AuthStorage.create(authJsonPath); + writeFileSync(authJsonPath, "{invalid-json", "utf-8"); + + authStorage.reload(); + + // Keeps previous in-memory data on reload failure + expect(authStorage.get("anthropic")).toEqual({ type: "api_key", key: "anthropic-key" }); + + const firstDrain = authStorage.drainErrors(); + expect(firstDrain.length).toBeGreaterThan(0); + expect(firstDrain[0]).toBeInstanceOf(Error); + + const secondDrain = authStorage.drainErrors(); + expect(secondDrain).toHaveLength(0); + }); + }); + describe("runtime overrides", () => { test("runtime override takes priority over auth.json", async () => { writeAuthJson({ anthropic: { type: "api_key", key: "!echo stored-key" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); authStorage.setRuntimeApiKey("anthropic", "runtime-key"); const apiKey = await authStorage.getApiKey("anthropic"); @@ -358,7 +442,7 @@ describe("AuthStorage", () => { anthropic: { type: "api_key", key: "!echo stored-key" }, }); - authStorage = new AuthStorage(authJsonPath); + authStorage = AuthStorage.create(authJsonPath); authStorage.setRuntimeApiKey("anthropic", "runtime-key"); authStorage.removeRuntimeApiKey("anthropic"); diff --git a/packages/coding-agent/test/compaction-extensions.test.ts b/packages/coding-agent/test/compaction-extensions.test.ts index 9b7bf8be..15d075a8 100644 --- a/packages/coding-agent/test/compaction-extensions.test.ts +++ b/packages/coding-agent/test/compaction-extensions.test.ts @@ -96,7 +96,7 @@ describe.skipIf(!API_KEY)("Compaction extensions", () => { const sessionManager = SessionManager.create(tempDir); const settingsManager = SettingsManager.create(tempDir, tempDir); - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = new ModelRegistry(authStorage); const runtime = createExtensionRuntime(); diff --git a/packages/coding-agent/test/extensions-input-event.test.ts b/packages/coding-agent/test/extensions-input-event.test.ts index c990b7ef..56237861 100644 --- a/packages/coding-agent/test/extensions-input-event.test.ts +++ b/packages/coding-agent/test/extensions-input-event.test.ts @@ -29,7 +29,7 @@ describe("Input Event", () => { for (let i = 0; i < extensions.length; i++) fs.writeFileSync(path.join(extensionsDir, `e${i}.ts`), extensions[i]); const result = await discoverAndLoadExtensions([], tempDir, tempDir); const sm = SessionManager.inMemory(); - const mr = new ModelRegistry(new AuthStorage(path.join(tempDir, "auth.json"))); + const mr = new ModelRegistry(AuthStorage.create(path.join(tempDir, "auth.json"))); return new ExtensionRunner(result.extensions, result.runtime, tempDir, sm, mr); } diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index df2f2036..4538698f 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -24,7 +24,7 @@ describe("ExtensionRunner", () => { extensionsDir = path.join(tempDir, "extensions"); fs.mkdirSync(extensionsDir); sessionManager = SessionManager.inMemory(); - const authStorage = new AuthStorage(path.join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(path.join(tempDir, "auth.json")); modelRegistry = new ModelRegistry(authStorage); }); diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts index bb084b13..01e0bafc 100644 --- a/packages/coding-agent/test/model-registry.test.ts +++ b/packages/coding-agent/test/model-registry.test.ts @@ -15,7 +15,7 @@ describe("ModelRegistry", () => { tempDir = join(tmpdir(), `pi-test-model-registry-${Date.now()}-${Math.random().toString(36).slice(2)}`); mkdirSync(tempDir, { recursive: true }); modelsJsonPath = join(tempDir, "models.json"); - authStorage = new AuthStorage(join(tempDir, "auth.json")); + authStorage = AuthStorage.create(join(tempDir, "auth.json")); }); afterEach(() => { diff --git a/packages/coding-agent/test/utilities.ts b/packages/coding-agent/test/utilities.ts index ce28b07b..ff82173b 100644 --- a/packages/coding-agent/test/utilities.ts +++ b/packages/coding-agent/test/utilities.ts @@ -119,7 +119,7 @@ export const PI_AGENT_DIR = join(homedir(), ".pi", "agent"); * Use this for tests that need real OAuth credentials. */ export function getRealAuthStorage(): AuthStorage { - return new AuthStorage(AUTH_PATH); + return AuthStorage.create(AUTH_PATH); } /** @@ -214,7 +214,7 @@ export function createTestSession(options: TestSessionOptions = {}): TestSession settingsManager.applyOverrides(options.settingsOverrides); } - const authStorage = new AuthStorage(join(tempDir, "auth.json")); + const authStorage = AuthStorage.create(join(tempDir, "auth.json")); const modelRegistry = new ModelRegistry(authStorage, tempDir); const session = new AgentSession({ diff --git a/packages/mom/src/agent.ts b/packages/mom/src/agent.ts index 19feb0b0..01cf5561 100644 --- a/packages/mom/src/agent.ts +++ b/packages/mom/src/agent.ts @@ -428,7 +428,7 @@ function createRunner(sandboxConfig: SandboxConfig, channelId: string, channelDi // Create AuthStorage and ModelRegistry // Auth stored outside workspace so agent can't access it - const authStorage = new AuthStorage(join(homedir(), ".pi", "mom", "auth.json")); + const authStorage = AuthStorage.create(join(homedir(), ".pi", "mom", "auth.json")); const modelRegistry = new ModelRegistry(authStorage); // Create agent