diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f7100b579..668b8cbfa 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,9 @@ - Added privacy-safe pseudonymous product analytics for onboarding, command use, execution modes, run outcomes, TTFT, latency, usage, tools, retries, and compactions, with disclosure and opt-out controls ([ENG-4682](https://linear.app/primeintellect/issue/ENG-4682/add-privacy-safe-posthog-analytics-to-prime-agent)). - Changed sent agent messages in the IPython cell UI to show only the message text with a `╰─` gutter when expanded, matching received messages, and hid the raw `agent_message.send` receipt dictionary. - Fixed Homebrew installs attempting to self-update their versioned Cellar keg instead of directing users to `brew upgrade prime-agent` ([#844](https://github.com/PrimeIntellect-ai/prime-agent/issues/844)) +- Fixed concurrent first-time `settings.json` writes losing each other's changes, and crash-interrupted writes leaving a truncated file that silently reset all settings; updates now hold the file lock before reading and write through a temp file and rename ([#983](https://github.com/PrimeIntellect-ai/prime-agent/issues/983)) +- Fixed crash-interrupted `auth.json` writes risking loss of all stored API keys and OAuth tokens; writes now go through a `0600` temp file and atomic rename ([#983](https://github.com/PrimeIntellect-ai/prime-agent/issues/983)) +- Fixed the legacy credential migration removing `oauth.json` and `settings.json` apiKeys before `auth.json` was safely written, which let a crash mid-migration destroy all credentials ([#983](https://github.com/PrimeIntellect-ai/prime-agent/issues/983)) ## [0.7.1] - 2026-08-07 diff --git a/packages/coding-agent/src/core/auth-storage.ts b/packages/coding-agent/src/core/auth-storage.ts index 493c9c964..a97adf08d 100644 --- a/packages/coding-agent/src/core/auth-storage.ts +++ b/packages/coding-agent/src/core/auth-storage.ts @@ -15,8 +15,18 @@ import { type OAuthProviderId, } from "@earendil-works/pi-ai"; import { getOAuthApiKey, getOAuthProvider, getOAuthProviders } from "@earendil-works/pi-ai/oauth"; -import { chmodSync, existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; -import { dirname, join } from "path"; +import { + chmodSync, + closeSync, + existsSync, + mkdirSync, + openSync, + readFileSync, + renameSync, + rmSync, + writeFileSync, +} from "fs"; +import { basename, dirname, join } from "path"; import lockfile from "proper-lockfile"; import { getAgentDir } from "../config.js"; import { @@ -122,6 +132,29 @@ export class FileAuthStorageBackend implements AuthStorageBackend { } } + private writeAtomically(content: string): void { + const tempPath = join( + dirname(this.authPath), + `.${basename(this.authPath)}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`, + ); + let fd: number | undefined = openSync(tempPath, "wx", 0o600); + try { + writeFileSync(fd, content, "utf-8"); + closeSync(fd); + fd = undefined; + chmodSync(tempPath, 0o600); + renameSync(tempPath, this.authPath); + chmodSync(this.authPath, 0o600); + } finally { + if (fd !== undefined) { + closeSync(fd); + } + if (existsSync(tempPath)) { + rmSync(tempPath, { force: true }); + } + } + } + private acquireLockSyncWithRetry(path: string): () => void { const maxAttempts = 10; const delayMs = 20; @@ -159,8 +192,7 @@ export class FileAuthStorageBackend implements AuthStorageBackend { 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); + this.writeAtomically(next); } return result; } finally { @@ -204,8 +236,7 @@ export class FileAuthStorageBackend implements AuthStorageBackend { const { result, next } = await fn(current); throwIfCompromised(); if (next !== undefined) { - writeFileSync(this.authPath, next, "utf-8"); - chmodSync(this.authPath, 0o600); + this.writeAtomically(next); } throwIfCompromised(); return result; diff --git a/packages/coding-agent/src/core/settings-manager.ts b/packages/coding-agent/src/core/settings-manager.ts index ab42f5e93..69ea808a0 100644 --- a/packages/coding-agent/src/core/settings-manager.ts +++ b/packages/coding-agent/src/core/settings-manager.ts @@ -1,7 +1,7 @@ import type { ServiceTier, Transport } from "@earendil-works/pi-ai"; -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; +import { existsSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "fs"; import { homedir } from "os"; -import { dirname, join } from "path"; +import { basename, dirname, join } from "path"; import lockfile from "proper-lockfile"; import { CONFIG_DIR_NAME, getAgentDir } from "../config.js"; @@ -208,6 +208,22 @@ function deepMergeSettings(base: Settings, overrides: Settings): Settings { export type SettingsScope = "global" | "project"; +function writeSettingsFileAtomically(path: string, content: string): void { + const dir = dirname(path); + const tempPath = join( + dir, + `.${basename(path)}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`, + ); + try { + writeFileSync(tempPath, content, "utf-8"); + renameSync(tempPath, path); + } finally { + if (existsSync(tempPath)) { + rmSync(tempPath, { force: true }); + } + } +} + export interface SettingsStorage { withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void; } @@ -264,17 +280,22 @@ export class FileSettingsStorage implements SettingsStorage { if (fileExists) { release = this.acquireLockSyncWithRetry(path); } - const current = fileExists ? readFileSync(path, "utf-8") : undefined; - const next = fn(current); + let current = fileExists ? readFileSync(path, "utf-8") : undefined; + let next = fn(current); if (next !== undefined) { - // Only create directory when we actually need to write - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } if (!release) { + // Only create directory when we actually need to write + if (!existsSync(dir)) { + mkdirSync(dir, { recursive: true }); + } release = this.acquireLockSyncWithRetry(path); + // Re-read under the lock so a concurrent first-time write is not lost. + current = existsSync(path) ? readFileSync(path, "utf-8") : undefined; + next = fn(current); + } + if (next !== undefined) { + writeSettingsFileAtomically(path, next); } - writeFileSync(path, next, "utf-8"); } } finally { if (release) { diff --git a/packages/coding-agent/src/migrations.ts b/packages/coding-agent/src/migrations.ts index 0085d7b48..0234fb894 100644 --- a/packages/coding-agent/src/migrations.ts +++ b/packages/coding-agent/src/migrations.ts @@ -4,6 +4,7 @@ import chalk from "chalk"; import { + chmodSync, type Dirent, existsSync, mkdirSync, @@ -25,6 +26,24 @@ const MIGRATION_GUIDE_URL = const EXTENSIONS_DOC_URL = "https://github.com/earendil-works/pi-mono/blob/main/packages/coding-agent/docs/extensions.md"; +function writeJsonFileAtomically(path: string, value: unknown, mode?: number): void { + const tempPath = join( + dirname(path), + `.${basename(path)}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`, + ); + try { + writeFileSync(tempPath, JSON.stringify(value, null, 2), mode === undefined ? undefined : { mode }); + if (mode !== undefined) { + chmodSync(tempPath, mode); + } + renameSync(tempPath, path); + } finally { + if (existsSync(tempPath)) { + rmSync(tempPath, { force: true }); + } + } +} + /** * Migrate legacy oauth.json and settings.json apiKeys to auth.json. * @@ -42,7 +61,8 @@ export function migrateAuthToAuthJson(): string[] { const migrated: Record = {}; const providers: string[] = []; - // Migrate oauth.json + // Read oauth.json; it is renamed to .migrated only after auth.json is durable. + let hasOAuth = false; if (existsSync(oauthPath)) { try { const oauth = JSON.parse(readFileSync(oauthPath, "utf-8")); @@ -50,35 +70,53 @@ export function migrateAuthToAuthJson(): string[] { migrated[provider] = { type: "oauth", ...(cred as object) }; providers.push(provider); } - renameSync(oauthPath, `${oauthPath}.migrated`); + hasOAuth = true; } catch { // Skip on error } } - // Migrate settings.json apiKeys + // Read settings.json apiKeys; the file is rewritten only after auth.json is durable. + let settings: Record | undefined; if (existsSync(settingsPath)) { try { const content = readFileSync(settingsPath, "utf-8"); - const settings = JSON.parse(content); - if (settings.apiKeys && typeof settings.apiKeys === "object") { - for (const [provider, key] of Object.entries(settings.apiKeys)) { + const parsed = JSON.parse(content) as Record; + if (parsed.apiKeys && typeof parsed.apiKeys === "object") { + for (const [provider, key] of Object.entries(parsed.apiKeys)) { if (!migrated[provider] && typeof key === "string") { migrated[provider] = { type: "api_key", key }; providers.push(provider); } } - delete settings.apiKeys; - writeFileSync(settingsPath, JSON.stringify(settings, null, 2)); + settings = parsed; } } catch { // Skip on error } } + // Write auth.json first so credentials survive a crash before the old locations are cleaned up. if (Object.keys(migrated).length > 0) { mkdirSync(dirname(authPath), { recursive: true }); - writeFileSync(authPath, JSON.stringify(migrated, null, 2), { mode: 0o600 }); + writeJsonFileAtomically(authPath, migrated, 0o600); + } + + if (settings) { + delete settings.apiKeys; + try { + writeJsonFileAtomically(settingsPath, settings); + } catch { + // Skip on error + } + } + + if (hasOAuth) { + try { + renameSync(oauthPath, `${oauthPath}.migrated`); + } catch { + // Skip on error + } } return providers; diff --git a/packages/coding-agent/test/suite/regressions/983-credential-file-durability.test.ts b/packages/coding-agent/test/suite/regressions/983-credential-file-durability.test.ts new file mode 100644 index 000000000..3c0d47f7a --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/983-credential-file-durability.test.ts @@ -0,0 +1,203 @@ +import type { PathLike, PathOrFileDescriptor, WriteFileOptions } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { registerOAuthProvider } from "@earendil-works/pi-ai/oauth"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const fsEvents = vi.hoisted(() => ({ events: [] as string[] })); + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeFileSync(path: PathOrFileDescriptor, data: string | NodeJS.ArrayBufferView, options?: WriteFileOptions) { + if (typeof path !== "number") { + fsEvents.events.push(`write:${String(path)}`); + } + return actual.writeFileSync(path, data, options); + }, + renameSync(oldPath: PathLike, newPath: PathLike) { + fsEvents.events.push(`rename:${String(newPath)}`); + return actual.renameSync(oldPath, newPath); + }, + }; +}); + +import { ENV_AGENT_DIR } from "../../../src/config.js"; +import { AuthStorage } from "../../../src/core/auth-storage.js"; +import { FileSettingsStorage } from "../../../src/core/settings-manager.js"; +import { migrateAuthToAuthJson } from "../../../src/migrations.js"; + +describe("regression #983: credential file durability", () => { + let tempDir: string; + let agentDir: string; + const previousAgentDir = process.env[ENV_AGENT_DIR]; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "prime-agent-983-")); + agentDir = join(tempDir, "agent"); + mkdirSync(agentDir, { recursive: true }); + fsEvents.events.length = 0; + }); + + afterEach(() => { + if (previousAgentDir === undefined) { + delete process.env[ENV_AGENT_DIR]; + } else { + process.env[ENV_AGENT_DIR] = previousAgentDir; + } + if (tempDir && existsSync(tempDir)) { + rmSync(tempDir, { recursive: true, force: true }); + } + }); + + function expectNoLeftoverTempFiles(dir: string) { + expect(readdirSync(dir).filter((f) => f.endsWith(".tmp"))).toEqual([]); + } + + function expectRestrictiveMode(path: string) { + // Windows cannot express POSIX modes; a writable file stats as 0o666 there. + const expected = process.platform === "win32" ? 0o666 : 0o600; + expect(statSync(path).mode & 0o777).toBe(expected); + } + + it("migrates credentials to auth.json before touching oauth.json or settings.json", () => { + process.env[ENV_AGENT_DIR] = agentDir; + const authPath = join(agentDir, "auth.json"); + const oauthPath = join(agentDir, "oauth.json"); + const settingsPath = join(agentDir, "settings.json"); + writeFileSync(oauthPath, JSON.stringify({ anthropic: { access: "a", refresh: "r", expires: 1 } })); + writeFileSync(settingsPath, JSON.stringify({ theme: "dark", apiKeys: { openai: "sk-test" } })); + + fsEvents.events.length = 0; + const providers = migrateAuthToAuthJson(); + + expect(providers.sort()).toEqual(["anthropic", "openai"]); + const auth = JSON.parse(readFileSync(authPath, "utf-8")) as Record; + expect(auth.anthropic.type).toBe("oauth"); + expect(auth.openai).toEqual({ type: "api_key", key: "sk-test" }); + expectRestrictiveMode(authPath); + expect(existsSync(oauthPath)).toBe(false); + expect(existsSync(`${oauthPath}.migrated`)).toBe(true); + const settings = JSON.parse(readFileSync(settingsPath, "utf-8")) as Record; + expect(settings.apiKeys).toBeUndefined(); + expect(settings.theme).toBe("dark"); + expectNoLeftoverTempFiles(agentDir); + + // auth.json must be durable before the old locations are modified. + const authWriteIndex = fsEvents.events.indexOf(`rename:${authPath}`); + const settingsWriteIndex = fsEvents.events.indexOf(`rename:${settingsPath}`); + const oauthRenameIndex = fsEvents.events.indexOf(`rename:${oauthPath}.migrated`); + expect(authWriteIndex).toBeGreaterThanOrEqual(0); + expect(settingsWriteIndex).toBeGreaterThan(authWriteIndex); + expect(oauthRenameIndex).toBeGreaterThan(authWriteIndex); + }); + + it("re-reads settings under the lock before first-time creation", () => { + const settingsPath = join(agentDir, "settings.json"); + const storage = new FileSettingsStorage(tempDir, agentDir); + + const seen: (string | undefined)[] = []; + storage.withLock("global", (current) => { + seen.push(current); + if (current === undefined) { + // Simulate a concurrent process creating the file first. + writeFileSync(settingsPath, JSON.stringify({ theme: "dark" })); + return JSON.stringify({ theme: "light" }); + } + return JSON.stringify({ ...JSON.parse(current), defaultModel: "claude-sonnet" }); + }); + + expect(seen).toHaveLength(2); + expect(seen[0]).toBeUndefined(); + expect(JSON.parse(seen[1]!)).toEqual({ theme: "dark" }); + expect(JSON.parse(readFileSync(settingsPath, "utf-8"))).toEqual({ + theme: "dark", + defaultModel: "claude-sonnet", + }); + }); + + it("writes settings.json via temp file and rename", () => { + const settingsPath = join(agentDir, "settings.json"); + writeFileSync(settingsPath, JSON.stringify({ theme: "dark" })); + const storage = new FileSettingsStorage(tempDir, agentDir); + + fsEvents.events.length = 0; + storage.withLock("global", () => JSON.stringify({ theme: "light" })); + + expect(fsEvents.events).toContain(`rename:${settingsPath}`); + expect(fsEvents.events).not.toContain(`write:${settingsPath}`); + expect(JSON.parse(readFileSync(settingsPath, "utf-8"))).toEqual({ theme: "light" }); + expectNoLeftoverTempFiles(agentDir); + }); + + it("writes auth.json via temp file and rename with 0600 permissions on set", () => { + const authPath = join(agentDir, "auth.json"); + const storage = AuthStorage.create(authPath, { usePrimeCliConfig: false }); + + fsEvents.events.length = 0; + storage.set("openai", { type: "api_key", key: "sk-test" }); + + expect(fsEvents.events).toContain(`rename:${authPath}`); + expect(fsEvents.events).not.toContain(`write:${authPath}`); + expectRestrictiveMode(authPath); + expect(JSON.parse(readFileSync(authPath, "utf-8"))).toEqual({ + openai: { type: "api_key", key: "sk-test" }, + }); + expectNoLeftoverTempFiles(agentDir); + }); + + it("writes auth.json via temp file and rename with 0600 permissions on OAuth refresh", async () => { + const providerId = `test-oauth-983-${Math.random().toString(36).slice(2)}`; + registerOAuthProvider({ + id: providerId, + name: "Test OAuth Provider", + async login() { + throw new Error("Not used in this test"); + }, + async refreshToken(credentials) { + return { + ...credentials, + access: "refreshed-access-token", + expires: Date.now() + 60_000, + }; + }, + getApiKey(credentials) { + return `Bearer ${credentials.access}`; + }, + }); + + const authPath = join(agentDir, "auth.json"); + writeFileSync( + authPath, + JSON.stringify({ + [providerId]: { + type: "oauth", + refresh: "refresh-token", + access: "expired-access-token", + expires: Date.now() - 10_000, + }, + }), + ); + const storage = AuthStorage.create(authPath, { usePrimeCliConfig: false }); + + fsEvents.events.length = 0; + const apiKey = await storage.getApiKey(providerId); + + expect(apiKey).toBe("Bearer refreshed-access-token"); + expect(fsEvents.events).toContain(`rename:${authPath}`); + expect(fsEvents.events).not.toContain(`write:${authPath}`); + expectRestrictiveMode(authPath); + expectNoLeftoverTempFiles(agentDir); + }); +});