From 854a25d6303c1b549a2d8bb2832a354d2d67986d Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:00:49 -0500 Subject: [PATCH 01/11] test(isolation): stop the suite writing to the real account store The suite drives real AccountManager instances and calls loadAccounts / saveAccounts without overriding storage, so `npm test` resolved ~/.opencode/oc-codex-multi-auth-accounts.json against the developer's own home and wrote fixtures over it. On 2026-09-17 that replaced a live five-account ChatGPT pool with two test records (accountId "test-account" and "new-import", addedAt 1ms and 2ms past the epoch) and took the running opencode fleet down for roughly 40 minutes: live processes reported "No Codex accounts configured. Run `opencode auth login`." and "All 2 account(s) are rate-limited". Recovery needed a 15-day-old backup, and five of the seven accounts it restored came back with dead refresh tokens. The redirect has to be `test.env` rather than a setupFiles entry. vitest applies test.env in the worker before it imports any test module, while a setup file runs once the module graph is already loading, which is too late for lib/config.ts, lib/accounts/recovery.ts, lib/logger.ts, lib/prompts/codex.ts, lib/prompts/opencode-codex.ts and lib/auto-update-checker.ts: each captures homedir() at module scope. Verified empirically rather than assumed. With the real HOME inherited on the command line, LOG_DIR still resolves inside the sandbox. A redirect alone is one refactor away from lapsing silently, so the storage layer also fails closed. Under VITEST, any account-storage write, unlink, or lock-sidecar probe that resolves inside the real home throws TEST_HOME_ESCAPE instead of proceeding. The check compares against os.userInfo().homedir, which reads the passwd entry rather than $HOME and so still names the real home after the redirect; the sandbox cannot spoof it. It is inert outside vitest. The guard runs before `acquireOrDetectLock`, not inside the try that wraps it, because that probe writes a lock sidecar next to the accounts file and would therefore touch the real store even on a pure read, and because the surrounding catch would swallow the refusal. test/paths.test.ts needed fixing as a consequence rather than by coincidence. Its two lookalike-prefix cases build a sibling of an allowed root and require it to be outside all three roots; with HOME under tmpdir(), every sibling of home is a child of tmpdir(), which resolvePath legitimately allows, so the assertions stopped throwing. Mocking homedir() and tmpdir() to fixed unrelated roots makes them independent of where the real HOME points. They pass with HOME both inside and outside tmpdir(), which also means the isolated home can keep living under tmpdir(). AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- lib/storage/load-save.ts | 38 +++++++++++++++++++- lib/storage/paths.ts | 2 +- test/paths.test.ts | 19 ++++++++++ test/test-home-isolation.test.ts | 62 ++++++++++++++++++++++++++++++++ vitest.config.ts | 24 +++++++++++++ 5 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 test/test-home-isolation.test.ts diff --git a/lib/storage/load-save.ts b/lib/storage/load-save.ts index fddcbe71..ef20c727 100644 --- a/lib/storage/load-save.ts +++ b/lib/storage/load-save.ts @@ -25,7 +25,7 @@ import { AnyAccountStorageSchema, getValidationErrors } from "../schemas.js"; import { renameWithWindowsRetry } from "./atomic-write.js"; import { formatStorageErrorHint, StorageError } from "./errors.js"; import { normalizeAccountStorage } from "./normalize.js"; -import { getConfigDir } from "./paths.js"; +import { getConfigDir, isWithinDirectory } from "./paths.js"; import { getCurrentLegacyProjectStoragePath, getCurrentProjectRoot, @@ -142,6 +142,10 @@ async function checkWorktreeLockForCurrentStorage( }); return; } + // Before the probe, not inside the try: `acquireOrDetectLock` writes a lock + // sidecar next to the accounts file, so a leaked HOME would touch the real + // store here even on a pure read, and this function's catch would hide it. + assertTestRunNeverTouchesRealHome(path); try { const result = await acquireOrDetectLock(path); if (!result.acquired && result.foreign) { @@ -171,6 +175,36 @@ async function checkWorktreeLockForCurrentStorage( } } +/** + * Refuse to mutate account storage inside the developer's real home while the + * test suite is running. + * + * `vitest.config.ts` redirects HOME to a sandbox before any module loads, but a + * test that restores the captured real HOME, or a future regression in that + * config, would otherwise write fixtures straight over live ChatGPT + * credentials. `os.userInfo()` reads the passwd entry instead of `$HOME`, so it + * still names the real home after the redirect and gives the check something + * the sandbox cannot spoof. Inert outside vitest. + */ +function assertTestRunNeverTouchesRealHome(path: string): void { + if (!process.env.VITEST) return; + + let realHome: string; + try { + realHome = os.userInfo().homedir; + } catch { + return; + } + if (!realHome || !isWithinDirectory(realHome, path)) return; + + throw new StorageError( + `Refusing to write account storage inside the real home directory during a test run: ${path}`, + "TEST_HOME_ESCAPE", + path, + "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", + ); +} + async function ensureGitignore(storagePath: string): Promise { if (!getCurrentStoragePath()) return; @@ -537,6 +571,7 @@ async function loadAccountsInternal( * Callers must already be inside withStorageLock when using this helper directly. */ async function writeAccountsToPathUnlocked(path: string, storage: AccountStorageV3): Promise { + assertTestRunNeverTouchesRealHome(path); const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; @@ -759,6 +794,7 @@ export async function clearAccounts(): Promise { let jsonCleared = true; try { const path = getStoragePath(); + assertTestRunNeverTouchesRealHome(path); await fs.unlink(path); } catch (error) { const code = (error as NodeJS.ErrnoException).code; diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index 1dc45bc6..ad2e11ce 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -81,7 +81,7 @@ function normalizePathForComparison(filePath: string): string { return process.platform === "win32" ? resolvedPath.toLowerCase() : resolvedPath; } -function isWithinDirectory(baseDir: string, targetPath: string): boolean { +export function isWithinDirectory(baseDir: string, targetPath: string): boolean { const normalizedBase = normalizePathForComparison(baseDir); const normalizedTarget = normalizePathForComparison(targetPath); const rel = relative(normalizedBase, normalizedTarget); diff --git a/test/paths.test.ts b/test/paths.test.ts index ca215ec8..c3a88ceb 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -2,10 +2,29 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { homedir, tmpdir } from "node:os"; import path from "node:path"; +// `resolvePath` guards against a path that merely shares a string prefix with +// an allowed root, so the lookalike cases below build a sibling of one root and +// require it to be outside all three. Against the real layout that only holds +// by accident: the suite runs with HOME redirected under the temp dir, which +// makes every sibling of home a child of tmpdir() and turns the assertion into +// a no-op. Fixed roots keep these cases meaningful wherever HOME points. +// +// This mock and the isolated HOME in `vitest.config.ts` are load-bearing +// together. Removing the mock as redundant silently turns both lookalike +// assertions into no-ops rather than failing them. +const FAKE_HOME = path.resolve("/fake-storage-paths-home"); +const FAKE_TMPDIR = path.resolve("/fake-storage-paths-tmp"); + vi.mock("node:fs", () => ({ existsSync: vi.fn(), })); +vi.mock("node:os", async (importOriginal) => ({ + ...(await importOriginal()), + homedir: () => FAKE_HOME, + tmpdir: () => FAKE_TMPDIR, +})); + import { existsSync } from "node:fs"; import { getConfigDir, diff --git a/test/test-home-isolation.test.ts b/test/test-home-isolation.test.ts new file mode 100644 index 00000000..ec58263b --- /dev/null +++ b/test/test-home-isolation.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect } from "vitest"; +import { homedir, userInfo } from "node:os"; +import { isAbsolute, relative, resolve } from "node:path"; +import { LOG_DIR } from "../lib/logger.js"; +import { ACCOUNTS_FILE_NAME } from "../lib/constants.js"; +import { saveAccounts } from "../lib/storage/load-save.js"; +import { getConfigDir } from "../lib/storage/paths.js"; +import { + getStoragePath, + setStoragePath, + setStoragePathDirect, +} from "../lib/storage/state.js"; + +function realUserHome(): string { + return userInfo().homedir; +} + +function isUnder(baseDir: string, targetPath: string): boolean { + const rel = relative(resolve(baseDir), resolve(targetPath)); + return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); +} + +describe("test home isolation", () => { + it("redirects homedir() away from the real user home", () => { + expect(process.env.OC_CODEX_TEST_HOME).toBeTruthy(); + expect(homedir()).toBe(process.env.OC_CODEX_TEST_HOME); + expect(isUnder(realUserHome(), homedir())).toBe(false); + }); + + it("keeps every resolved account storage path out of the real user home", () => { + const real = realUserHome(); + + setStoragePath(null); + expect(isUnder(real, getStoragePath())).toBe(false); + + setStoragePath(process.cwd()); + expect(isUnder(real, getStoragePath())).toBe(false); + + setStoragePath(null); + expect(isUnder(real, getConfigDir())).toBe(false); + }); + + // LOG_DIR is captured at module scope from homedir(), so it only lands in + // the sandbox if the override beat the import. That is the property a + // setupFiles entry cannot provide and this whole mechanism exists for. + it("beats module-scope homedir() capture", () => { + expect(isUnder(realUserHome(), LOG_DIR)).toBe(false); + expect(isUnder(process.env.OC_CODEX_TEST_HOME as string, LOG_DIR)).toBe(true); + }); + + it("refuses to write account storage that escapes into the real home", async () => { + const escaped = resolve(realUserHome(), ".opencode", ACCOUNTS_FILE_NAME); + setStoragePathDirect(escaped); + try { + await expect( + saveAccounts({ version: 3, accounts: [], activeIndex: 0 }), + ).rejects.toMatchObject({ code: "TEST_HOME_ESCAPE" }); + } finally { + setStoragePathDirect(null); + } + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 5499527f..4240487d 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,9 +1,33 @@ import { defineConfig } from 'vitest/config'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +/** + * Per-run throwaway home. The suite writes real account storage, so without + * this a `npm test` resolves `~/.opencode/oc-codex-multi-auth-accounts.json` + * against the developer's actual home and overwrites live ChatGPT credentials + * with fixtures. + * + * This must be `test.env`, not a `setupFiles` entry: vitest applies `test.env` + * before the worker imports any test module, and `lib/config.ts`, + * `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module + * scope, so anything later than import time is too late for them. + */ +const isolatedHome = + process.env.OC_CODEX_TEST_HOME ?? + mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); +process.env.OC_CODEX_TEST_HOME = isolatedHome; export default defineConfig({ test: { globals: true, environment: 'node', + env: { + HOME: isolatedHome, + USERPROFILE: isolatedHome, + OC_CODEX_TEST_HOME: isolatedHome, + }, include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From d9f5f81db8fff99c99104897d83d71a6a42fac2d Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:06:32 -0500 Subject: [PATCH 02/11] test(vitest): stop index.ts suites timing out on their own import test/index-retry.test.ts fails two of its six cases on a 5s timeout under full-suite load, and has done so on a clean checkout of upstream main. Nothing in those cases is slow: they already run on fake timers, and the whole file finishes its assertions in well under a second once loaded. What exceeds the timeout is the import. Four suites import the real `index.ts`, and the first one scheduled pays the transform of a 4900-line entry plus its dependency graph. Measured on an idle machine: 3.2s-6.7s for the cold import, ~400ms for a warm re-import after `vi.resetModules()`. With vitest's 5s default that is a coin flip before any test body runs, and CPU contention from the rest of the suite decides it. The floor is raised for the whole run rather than for one file, because a per-file timeout only moves the hazard to whichever of the four suites is scheduled first next time. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- vitest.config.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vitest.config.ts b/vitest.config.ts index 4240487d..41c0fe84 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -28,6 +28,12 @@ export default defineConfig({ USERPROFILE: isolatedHome, OC_CODEX_TEST_HOME: isolatedHome, }, + // Four suites import the real `index.ts`, and the first one scheduled pays + // the transform of a 4900-line entry plus its dependency graph: measured at + // 3.2s-6.7s on an idle machine, against a 5s default. Whichever suite loses + // that race times out under full-suite CPU contention, which is flakiness in + // the harness rather than in any assertion (a warm re-import costs ~400ms). + testTimeout: 15_000, include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From e131d69fd80e60743efb33655fa0e43f2d306e30 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:08:03 -0500 Subject: [PATCH 03/11] fix(retry): charge the rate-limit budget by wait length, not by attempt When every account is rate-limited the request waits and retries, and that loop is gated on `consumeRetryBudget("rateLimitGlobal", ...)`. The budget is tiny - 1 conservative, 3 balanced, 10 aggressive - and a wait cost one unit however briefly it blocked. Three consecutive 400ms waits therefore exhausted the default and the request hard-failed with "All N account(s) are rate-limited", when one more second of waiting would have served it. Nothing else in that gate can end the loop. `retryAllAccountsMaxRetries` defaults to Infinity, and `retryAllAccountsMaxWaitMs` defaults to 0, which the gate reads as uncapped - so the budget is the only term that can go false. The tracker is constructed per request, so this is not budget carried over from an earlier one either. Observed in production. Three independent sessions on a healthy 7-account pool died after roughly nine minutes each, reporting a true reset four hours out: `All 7 account(s) are rate-limited. Try again in 4h 0m 0s`. Three units against a real 4h wait should have been about twelve hours of sleeping. The accounts had just had their quota stamps cleared, so each attempt looked viable, went out, took a real 429, slept briefly, and repeated until the budget was gone. A unit now measures blocking time rather than attempts. `consumeWait` charges a full unit for a wait at or above RETRY_WAIT_BUDGET_UNIT_MS (5s), so a multi-hour block stays governed exactly as it was, and accumulates shorter waits on a per-bucket carry so a burst of sub-second probes is effectively free. An exhausted bucket still refuses a free wait: the carry is what bounds how long short waits can loop, and without that check the loop would never terminate. `consume` is unchanged and remains the default, so every other retry class keeps counting attempts. Only a caller that passes a wait is charged by duration, and the metrics counter follows the tracker's own usage rather than assuming one unit per call, so a free wait no longer reports budget it never spent. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 16 +++++++- lib/request/retry-budget.ts | 42 ++++++++++++++++++++ test/retry-budget.test.ts | 79 +++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 2 deletions(-) diff --git a/index.ts b/index.ts index 1cb71259..99b35279 100644 --- a/index.ts +++ b/index.ts @@ -2396,9 +2396,20 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const consumeRetryBudget = ( bucket: RetryBudgetClass, reason: string, + waitMs?: number, ): boolean => { - if (retryBudget.consume(bucket)) { - runtimeMetrics.retryBudgetUsage[bucket] += 1; + // Pass the wait so the charge scales with how long the retry + // blocks. Metrics follow the tracker's own counter rather than + // assuming one unit, or a free sub-second wait would report + // budget it never spent. + const usedBefore = retryBudget.getUsage()[bucket]; + const granted = + waitMs === undefined + ? retryBudget.consume(bucket) + : retryBudget.consumeWait(bucket, waitMs); + if (granted) { + runtimeMetrics.retryBudgetUsage[bucket] += + retryBudget.getUsage()[bucket] - usedBefore; return true; } runtimeMetrics.retryBudgetExhaustions += 1; @@ -3795,6 +3806,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { consumeRetryBudget( "rateLimitGlobal", `All accounts rate-limited wait ${waitMs}ms`, + waitMs, ) ) { const countdownMessage = `All ${count} account(s) rate-limited. Waiting`; diff --git a/lib/request/retry-budget.ts b/lib/request/retry-budget.ts index d1ea2beb..a86d01bf 100644 --- a/lib/request/retry-budget.ts +++ b/lib/request/retry-budget.ts @@ -43,6 +43,18 @@ const PROFILE_LIMITS: Record = { }, }; +/** + * How much blocking one budget unit buys when a retry is charged through + * {@link RetryBudgetTracker.consumeWait}. + * + * The budgets are small (1/3/10) and were being charged one unit per wait + * regardless of length, so three consecutive sub-second waits exhausted the + * default and hard-failed a request that one more second would have served. + * Waiting is only expensive in proportion to the time it costs the caller, so + * that is what a unit now measures. + */ +export const RETRY_WAIT_BUDGET_UNIT_MS = 5_000; + const RETRY_BUDGET_CLASSES: RetryBudgetClass[] = [ "authRefresh", "network", @@ -87,12 +99,42 @@ function createUsedCounters(): RetryBudgetLimits { export class RetryBudgetTracker { private readonly used: RetryBudgetLimits = createUsedCounters(); + private readonly waitCarryMs: RetryBudgetLimits = createUsedCounters(); private readonly limits: RetryBudgetLimits; constructor(limits: RetryBudgetLimits) { this.limits = { ...limits }; } + /** + * Charge a retry that blocks for `waitMs` against a bucket, in proportion to + * how long it blocks. + * + * A wait of {@link RETRY_WAIT_BUDGET_UNIT_MS} or longer costs a full unit, + * so a multi-hour block stays governed exactly as before. Shorter waits + * accumulate on a per-bucket carry and only cost a unit once they have added + * up to one, so a burst of sub-second waits is effectively free. + * + * An exhausted bucket refuses even a free wait: the carry bounds how long + * short waits can loop, and without that check they would loop forever once + * the budget ran out. + */ + consumeWait(bucket: RetryBudgetClass, waitMs: number): boolean { + if (this.getRemaining(bucket) <= 0) return false; + + const wait = Number.isFinite(waitMs) && waitMs > 0 ? waitMs : 0; + if (wait >= RETRY_WAIT_BUDGET_UNIT_MS) return this.consume(bucket); + + const carried = this.waitCarryMs[bucket] + wait; + if (carried < RETRY_WAIT_BUDGET_UNIT_MS) { + this.waitCarryMs[bucket] = carried; + return true; + } + + this.waitCarryMs[bucket] = carried - RETRY_WAIT_BUDGET_UNIT_MS; + return this.consume(bucket); + } + consume(bucket: RetryBudgetClass): boolean { const limit = this.limits[bucket]; if (!Number.isFinite(limit)) { diff --git a/test/retry-budget.test.ts b/test/retry-budget.test.ts index 8f3e62fe..8d67dba7 100644 --- a/test/retry-budget.test.ts +++ b/test/retry-budget.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { RetryBudgetTracker, resolveRetryBudgetLimits, + RETRY_WAIT_BUDGET_UNIT_MS, type RetryBudgetLimits, } from "../lib/request/retry-budget.js"; @@ -72,6 +73,84 @@ describe("retry-budget", () => { expect(tracker.getUsage().authRefresh).toBe(1); }); + describe("consumeWait", () => { + const balanced = () => new RetryBudgetTracker(resolveRetryBudgetLimits("balanced")); + + it("does not charge the budget for a burst of sub-second waits", () => { + const tracker = balanced(); + + // The production regression: three consecutive 400ms waits spent the + // whole default budget and hard-failed the request. + for (let i = 0; i < 3; i++) { + expect(tracker.consumeWait("rateLimitGlobal", 400)).toBe(true); + } + expect(tracker.getUsage().rateLimitGlobal).toBe(0); + expect(tracker.getRemaining("rateLimitGlobal")).toBe(3); + + for (let i = 0; i < 27; i++) { + expect(tracker.consumeWait("rateLimitGlobal", 400)).toBe(true); + } + }); + + it("charges a full unit for a wait at or above the unit length", () => { + const tracker = balanced(); + + expect(tracker.consumeWait("rateLimitGlobal", RETRY_WAIT_BUDGET_UNIT_MS)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(true); + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(3); + + // A long wait stays governed: the fourth exceeds the balanced budget. + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(false); + }); + + it("accumulates short waits into whole units", () => { + const tracker = balanced(); + const waitMs = RETRY_WAIT_BUDGET_UNIT_MS / 10; + + for (let i = 0; i < 10; i++) { + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + } + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + }); + + it("refuses free short waits once the bucket is exhausted", () => { + const tracker = balanced(); + + for (let i = 0; i < 3; i++) { + expect(tracker.consumeWait("rateLimitGlobal", RETRY_WAIT_BUDGET_UNIT_MS)).toBe(true); + } + + // Without this the carry would grant sub-unit waits forever and the + // retry loop could never terminate. + expect(tracker.consumeWait("rateLimitGlobal", 1)).toBe(false); + expect(tracker.consumeWait("rateLimitGlobal", 0)).toBe(false); + }); + + it("keeps per-bucket carries independent", () => { + const tracker = balanced(); + const waitMs = RETRY_WAIT_BUDGET_UNIT_MS / 2; + + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + expect(tracker.consumeWait("rateLimitShort", waitMs)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(0); + expect(tracker.getUsage().rateLimitShort).toBe(0); + + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + expect(tracker.getUsage().rateLimitShort).toBe(0); + }); + + it("treats a zero-limit bucket as immediately exhausted", () => { + const tracker = new RetryBudgetTracker( + resolveRetryBudgetLimits("balanced", { rateLimitGlobal: 0 }), + ); + expect(tracker.consumeWait("rateLimitGlobal", 1)).toBe(false); + }); + }); + it("clones constructor limits to avoid external mutation", () => { const limits: RetryBudgetLimits = { authRefresh: 1, From db13030a8fc0a366edb00101d7653ec41da50be4 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:09:51 -0500 Subject: [PATCH 04/11] fix(rate-limit): re-probe upstream during a long all-accounts wait With the retry budget no longer spent on short waits, a request that finds every account blocked sleeps out the real reset, which can be hours or - `retryAllAccountsMaxRetries` defaults to Infinity - days. Its only wake-up was the accounts file changing on disk and the watcher swapping the cached manager. That covers a peer process clearing a block, and it covers `opencode auth login` adding an account mid-sleep, since both write that file. It does not cover a reset granted server-side: the backend restoring quota changes nothing locally, so the sleeper keeps sleeping against capacity that has already come back. A wait of a minute or more now re-probes upstream as well. The probe is the quota monitor's own `runNow`, which refreshes `/wham/usage` for every account and persists what it finds, a recovery included - and persisting drops the cached manager, which is what makes the enclosing retry loop re-resolve one that no longer reports a block. It starts a minute in and doubles to a quarter-hour ceiling, so a multi-day sleep costs a handful of usage requests rather than one per five-second countdown tick. A probe that throws is logged at debug and the wait continues. The next probe is scheduled from the moment a probe returns rather than from when it was due, so a slow usage request cannot leave `nextProbeAt` in the past and collapse the countdown sleep to zero. Both wake paths are covered end to end against a real request: one where usage reports the quota back with no file write at all, and one where a login adds a second account while the first stays blocked on disk. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 69 +++++++++++++++++++++++++-- test/accounts-live-reload.test.ts | 79 ++++++++++++++++++++++++++++++- 2 files changed, 142 insertions(+), 6 deletions(-) diff --git a/index.ts b/index.ts index 99b35279..c005291d 100644 --- a/index.ts +++ b/index.ts @@ -394,6 +394,15 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { let startupPreflightShown = false; let beginnerSafeModeEnabled = false; const MIN_BACKOFF_MS = 100; + // An all-accounts rate-limit wait can run for days, and the local accounts + // file is its only wake-up. A quota reset granted server-side leaves that file + // untouched, so such a wait would be slept straight through. Long waits + // therefore re-probe upstream: first after a minute, doubling to a quarter + // hour, so a multi-day sleep costs a handful of usage requests rather than one + // per countdown tick. + const UPSTREAM_REPROBE_MIN_WAIT_MS = 60_000; + const UPSTREAM_REPROBE_FIRST_DELAY_MS = 60_000; + const UPSTREAM_REPROBE_MAX_DELAY_MS = 15 * 60_000; const runtimeMetrics: RuntimeMetrics = { startedAt: Date.now(), @@ -2461,16 +2470,35 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { totalMs: number, message: string, intervalMs: number = 5000, + probeUpstream?: () => Promise, ): Promise => { const startTime = Date.now(); const endTime = startTime + totalMs; - + let probeDelayMs = UPSTREAM_REPROBE_FIRST_DELAY_MS; + let nextProbeAt = + probeUpstream && totalMs >= UPSTREAM_REPROBE_MIN_WAIT_MS + ? startTime + probeDelayMs + : Number.POSITIVE_INFINITY; + while (Date.now() < endTime) { if (cachedAccountManager !== accountManager) return; if (abortSignal?.aborted) { throw abortError(); } - + + if (probeUpstream && Date.now() >= nextProbeAt) { + if (await probeUpstream()) return; + if (cachedAccountManager !== accountManager) return; + if (abortSignal?.aborted) { + throw abortError(); + } + probeDelayMs = Math.min(probeDelayMs * 2, UPSTREAM_REPROBE_MAX_DELAY_MS); + // Measured from the end of the probe, so a slow usage + // request cannot schedule the next one in the past and + // collapse the countdown sleep below to zero. + nextProbeAt = Date.now() + probeDelayMs; + } + const remaining = Math.max(0, endTime - Date.now()); const waitLabel = formatWaitTime(remaining); await showToast( @@ -2478,8 +2506,8 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { "warning", { duration: Math.min(intervalMs + 1000, toastDurationMs) }, ); - - const sleepTime = Math.min(intervalMs, remaining); + + const sleepTime = Math.min(intervalMs, remaining, nextProbeAt - Date.now()); if (sleepTime > 0) { await sleep(sleepTime); } else { @@ -2488,6 +2516,32 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } }; + /** + * True when an all-accounts wait can stop early. + * + * `runNow` refreshes `/wham/usage` for every account and persists + * whatever it finds, so a reset that never touched local disk + * becomes visible here. Persisting a recovery also drops the cached + * manager, which is what makes the enclosing retry loop re-resolve + * one that no longer reports a block. + */ + const probeUpstreamBlockLifted = async (): Promise => { + try { + await quotaMonitor.runNow(); + } catch (error) { + logDebug( + `[${PLUGIN_NAME}] Upstream quota re-probe failed: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return false; + } + if (cachedAccountManager !== accountManager) return true; + const manager = accountManager; + if (!manager) return false; + return manager.getMinWaitTimeForFamily(modelFamily, model) === 0; + }; + let allRateLimitedRetries = 0; let emptyResponseRetries = 0; const attemptedUnsupportedFallbackModels = new Set(); @@ -3810,7 +3864,12 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { ) ) { const countdownMessage = `All ${count} account(s) rate-limited. Waiting`; - await sleepWithCountdown(addJitter(waitMs, 0.2), countdownMessage); + await sleepWithCountdown( + addJitter(waitMs, 0.2), + countdownMessage, + undefined, + probeUpstreamBlockLifted, + ); allRateLimitedRetries++; continue; } diff --git a/test/accounts-live-reload.test.ts b/test/accounts-live-reload.test.ts index 1998ded9..d903336a 100644 --- a/test/accounts-live-reload.test.ts +++ b/test/accounts-live-reload.test.ts @@ -9,6 +9,7 @@ const captured = vi.hoisted((): { context?: ToolContext; listener?: () => void; onWatch?: () => void; + onQuotaProbe?: () => Promise; reads: Promise[]; maxRetries?: number; } => ({ reads: [] })); @@ -32,7 +33,11 @@ vi.mock("../lib/tools/index.js", () => ({ createToolRegistry: (context: ToolContext) => { captured.context = context; return {}; }, })); vi.mock("../lib/quota-notifications.js", () => ({ - createQuotaMonitor: () => ({ start() {}, dispose() {} }), + createQuotaMonitor: () => ({ + start() {}, + dispose() {}, + runNow: async () => { await captured.onQuotaProbe?.(); }, + }), })); vi.mock("../lib/auto-update-checker.js", () => ({ checkAndNotify: vi.fn(async () => {}) })); vi.mock("../lib/config.js", async (original) => ({ @@ -91,6 +96,7 @@ describe("accounts live reload", () => { await fs.writeFile(path, JSON.stringify(storage(true))); captured.listener = undefined; captured.onWatch = undefined; + captured.onQuotaProbe = undefined; captured.reads.length = 0; captured.maxRetries = undefined; plugin = await Reflect.apply(OpenAIOAuthPlugin, undefined, [{ @@ -285,6 +291,77 @@ describe("accounts live reload", () => { await vi.advanceTimersByTimeAsync(5000); expect((await response).status).toBe(200); }); + it("wakes a long wait when an upstream re-probe finds the block lifted", async () => { + const manager = captured.context?.cachedAccountManagerRef.current; + if (!manager) throw new Error("Missing manager"); + const account = manager.getCurrentAccount(); + if (!account) throw new Error("Missing account"); + manager.markQuotaExhausted(account, Date.now() + 86_400_000, "gpt-5.1"); + vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("data: [DONE]\n\n", { + status: 200, headers: { "content-type": "text/event-stream" }, + })); + let enteredWait: () => void = () => {}; + const waiting = new Promise((resolve) => { enteredWait = resolve; }); + const minWait = manager.getMinWaitTimeForFamily.bind(manager); + vi.spyOn(manager, "getMinWaitTimeForFamily").mockImplementation((...args) => { + enteredWait(); + return minWait(...args); + }); + let probes = 0; + captured.onQuotaProbe = async () => { + probes += 1; + // What a server-side grant looks like: usage reports the quota back, + // the recovery is persisted, and the cached manager is dropped. The + // accounts file is never written by another process, so the watcher + // has nothing to fire on - this is the wake-up it cannot provide. + captured.context?.invalidateAccountManagerCache(); + }; + const response = request("https://api.openai.com/v1/responses", { + method: "POST", body: JSON.stringify({ model: "gpt-5.1", stream: true, input: [] }), + }); + await waiting; + for (let elapsed = 0; elapsed < 120_000 && probes === 0; elapsed += 5000) { + await vi.advanceTimersByTimeAsync(5000); + } + expect(probes).toBe(1); + await vi.advanceTimersByTimeAsync(5000); + expect((await response).status).toBe(200); + }); + it("wakes a waiting request when a login adds an account mid-sleep", async () => { + const manager = captured.context?.cachedAccountManagerRef.current; + if (!manager) throw new Error("Missing manager"); + const account = manager.getCurrentAccount(); + if (!account) throw new Error("Missing account"); + const blockedUntil = Date.now() + 86_400_000; + manager.markQuotaExhausted(account, blockedUntil, "gpt-5.1"); + vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("data: [DONE]\n\n", { + status: 200, headers: { "content-type": "text/event-stream" }, + })); + let enteredWait: () => void = () => {}; + const waiting = new Promise((resolve) => { enteredWait = resolve; }); + const minWait = manager.getMinWaitTimeForFamily.bind(manager); + vi.spyOn(manager, "getMinWaitTimeForFamily").mockImplementation((...args) => { + enteredWait(); + return minWait(...args); + }); + const response = request("https://api.openai.com/v1/responses", { + method: "POST", body: JSON.stringify({ model: "gpt-5.1", stream: true, input: [] }), + }); + await waiting; + const reloaded = nextReload(); + // The incumbent account stays blocked on disk, so the only thing that can + // end this wait is the account the login added. + await fs.writeFile(path, JSON.stringify({ ...storage(true), accounts: [ + { ...storage(true).accounts[0], quotaExhaustedUntil: blockedUntil }, + { accountId: "fresh-login", refreshToken: "fresh-refresh", accessToken: "fresh-access", + expiresAt: Date.now() + 86_400_000, enabled: true, addedAt: 2, lastUsed: 2 }, + ] })); + await tick(); + await settle(); + await reloaded; + await vi.advanceTimersByTimeAsync(5000); + expect((await response).status).toBe(200); + }); it("keeps externally cleared blocks cleared despite queued and late saves from the old manager", async () => { const previous = captured.context?.cachedAccountManagerRef.current; if (!previous) throw new Error("Missing manager"); From 6c4c355508b68d88c72d63d8ba1a9d5e8f860c58 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:12:13 -0500 Subject: [PATCH 05/11] fix(accounts): never let a failed load empty a working account pool `loadAccounts()` reports a read or parse failure exactly as it reports an absent file - by returning null. `AccountState.initializeFromStorage()` turns that null into an AccountManager holding zero accounts, and both reload paths installed it unconditionally. The process then answered No Codex accounts configured. Run `opencode auth login`. while the accounts file on disk was intact and every other process on the machine was serving requests from it. Cross-process lock contention makes the failing read reachable: several opencode instances share one accounts file, and a read that loses a race against another process's atomic temp-file rename surfaces as exactly this empty result. Two guards, one per install site: - `reloadCachedAccountManager` compares the fresh manager against the incumbent it is replacing. A fresh manager with no accounts replacing an incumbent that has some is refused, the incumbent keeps serving, and a bounded retry (3 attempts, 2s apart) runs in case the next read succeeds. - `reloadForExternalAccountsChange` cannot compare against the incumbent, because an invalidation may legitimately have retired it and left the cache null. It compares against the file instead: the watcher already reads and hashes the changed file, so counting its `accounts` array costs nothing and says directly whether the accounts went away or the read failed. A file that carries accounts but loads as empty is refused and retried through the existing bounded retry path. The file-based comparison is what makes a genuine deletion still work. An external writer that really does remove the last account leaves an empty array on disk, the observed count is 0, the guard does not fire, and the empty pool is adopted as it should be. Both directions are covered by tests. Emptying the pool through the plugin's own surfaces (`codex-remove`, logout, a storage-mode switch) installs a manager directly rather than arriving on either of these paths, so neither guard can block a user-initiated removal. `readAccountsDigest` becomes `readAccountsFileState` and returns the count alongside the digest. The count is taken off the raw parsed document rather than the schema-validated union, so it reads the same for a V1, V2, or V3 file. The retry timers are unref'd and cancelled on watcher disposal, so a process shutting down mid-retry is not held open. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 115 +++++++++++++++++++++++++----- test/accounts-live-reload.test.ts | 29 ++++++++ 2 files changed, 128 insertions(+), 16 deletions(-) diff --git a/index.ts b/index.ts index c005291d..aa6ea961 100644 --- a/index.ts +++ b/index.ts @@ -1684,6 +1684,50 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } }; + /** + * `loadAccounts()` reports a read or parse failure the same way it reports + * an absent file - by returning null - and a null load builds a manager + * holding zero accounts. Installing that over a working pool makes this + * process answer "No Codex accounts configured" while the accounts file on + * disk is intact, which cross-process lock contention makes reachable. + * + * Emptying the pool for real always goes through an explicit action + * (`codex-remove`, logout, a storage-mode switch); each installs its own + * manager rather than arriving here, so refusing the shrink costs a genuine + * deletion nothing. + */ + const isUntrustworthyEmptyReload = ( + incumbent: AccountManager | null, + reloaded: AccountManager, + ): boolean => + incumbent !== null && + incumbent !== reloaded && + reloaded.getAccountCount() === 0 && + incumbent.getAccountCount() > 0; + + const EMPTY_RELOAD_RETRY_DELAY_MS = 2000; + const EMPTY_RELOAD_MAX_RETRIES = 3; + let emptyReloadRetries = 0; + let emptyReloadRetryTimer: ReturnType | undefined; + const cancelEmptyReloadRetry = (): void => { + clearTimeout(emptyReloadRetryTimer); + emptyReloadRetryTimer = undefined; + emptyReloadRetries = 0; + }; + const scheduleEmptyReloadRetry = (retry: () => Promise): void => { + if (emptyReloadRetries >= EMPTY_RELOAD_MAX_RETRIES) { + emptyReloadRetries = 0; + return; + } + emptyReloadRetries += 1; + clearTimeout(emptyReloadRetryTimer); + emptyReloadRetryTimer = setTimeout(() => { + emptyReloadRetryTimer = undefined; + void retry(); + }, EMPTY_RELOAD_RETRY_DELAY_MS); + emptyReloadRetryTimer.unref(); + }; + const reloadCachedAccountManager = async (): Promise => { if (!cachedAccountManager) return; const previous = cachedAccountManager; @@ -1702,6 +1746,15 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } try { const reloadedManager = await AccountManager.loadFromDisk(); + if (isUntrustworthyEmptyReload(previous, reloadedManager)) { + reloadedManager.disposeShutdownHandler(); + logWarn( + `[${PLUGIN_NAME}] Account reload returned no accounts while ${previous.getAccountCount()} are held; keeping the loaded pool and retrying`, + ); + scheduleEmptyReloadRetry(reloadCachedAccountManager); + return; + } + cancelEmptyReloadRetry(); cachedAccountManager = reloadedManager; accountManagerPromise = Promise.resolve(reloadedManager); // Dispose only after the replacement is installed so we never leak @@ -1734,21 +1787,41 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { accountsWatcherDisposed = true; unsubscribeAccountsPath?.(); stopAccountsWatcher(); + cancelEmptyReloadRetry(); unregisterCleanup(disposeAccountsWatcher); }; - const readAccountsDigest = async (path: string): Promise => { + const readAccountsFileState = async ( + path: string, + ): Promise<{ digest: string; accountCount: number } | undefined> => { try { const content = await readFile(path, "utf8"); - if (!AnyAccountStorageSchema.safeParse(JSON.parse(content)).success) return; - return createHash("sha256").update(content).digest("hex"); + const data = JSON.parse(content) as unknown; + if (!AnyAccountStorageSchema.safeParse(data).success) return; + // Counted off the raw document rather than the parsed union so the + // count is the same for every storage version. + const accounts = (data as { accounts?: unknown }).accounts; + return { + digest: createHash("sha256").update(content).digest("hex"), + accountCount: Array.isArray(accounts) ? accounts.length : 0, + }; } catch { return; } }; const reloadForExternalAccountsChange = async (path: string, generation: number, attempt = 0, retired?: AccountManager): Promise => { - const digest = await readAccountsDigest(path); - if (generation !== accountsWatchGeneration || !digest || path !== getStoragePath()) return; + const observed = await readAccountsFileState(path); + if (generation !== accountsWatchGeneration || !observed || path !== getStoragePath()) return; + const digest = observed.digest; if (digest === consumeLastWrittenAccountsDigest(path)) return; + const retryLater = (): void => { + if (attempt < 2 && generation === accountsWatchGeneration && !accountsWatcherDisposed) { + accountsReloadTimer = setTimeout(() => { + accountsReloadTimer = undefined; + void reloadForExternalAccountsChange(path, generation, attempt + 1, retired); + }, 1500); + accountsReloadTimer.unref(); + } + }; const previous = cachedAccountManager; try { // A null cache means an invalidation retired the incumbent; the @@ -1769,6 +1842,21 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { reloaded.disposeShutdownHandler(); return; } + // The file this reload observed carried accounts but the load + // produced none, so `loadAccounts()` failed to read it rather than + // the accounts having gone away - a failure it reports as an empty + // result, never as a throw, so the catch below cannot see it. + // Adopting it would answer "No Codex accounts configured" against an + // intact file; the retired incumbent still serves its accounts until + // a retry lands a real one. + if (observed.accountCount > 0 && reloaded.getAccountCount() === 0) { + reloaded.disposeShutdownHandler(); + logWarn( + `[${PLUGIN_NAME}] Externally changed accounts file holds ${observed.accountCount} account(s) but loaded as empty; keeping the current pool and retrying`, + ); + retryLater(); + return; + } const outgoing = cachedAccountManager; if (outgoing && outgoing !== retired) { // Another actor replaced the cached manager while this reload @@ -1784,13 +1872,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { observedAccountsDigest = digest; } catch { logWarn("Could not reload externally updated account storage"); - if (attempt < 2 && generation === accountsWatchGeneration && !accountsWatcherDisposed) { - accountsReloadTimer = setTimeout(() => { - accountsReloadTimer = undefined; - void reloadForExternalAccountsChange(path, generation, attempt + 1, retired); - }, 1500); - accountsReloadTimer.unref(); - } + retryLater(); return; } logDebug("Reloaded cached account manager after external accounts file change"); @@ -1804,8 +1886,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const path = watchedAccountsPath; if (!path) return; const generation = accountsWatchGeneration; - const digest = await readAccountsDigest(path); - if (generation !== accountsWatchGeneration || !digest || digest === observedAccountsDigest) return; + const observed = await readAccountsFileState(path); + if (generation !== accountsWatchGeneration || !observed || observed.digest === observedAccountsDigest) return; + const digest = observed.digest; observedAccountsDigest = digest; clearTimeout(accountsReloadTimer); accountsReloadTimer = undefined; @@ -1830,9 +1913,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { }); watchedAccountsPath = path; const generation = accountsWatchGeneration; - const initialDigest = await readAccountsDigest(path); + const initial = await readAccountsFileState(path); if (generation !== accountsWatchGeneration) return; - observedAccountsDigest = initialDigest; + observedAccountsDigest = initial?.digest; // Stat polling follows the path across the storage writer's temp-file rename. watchFile(path, { interval: 1500, persistent: false }, onAccountsStatChanged); unregisterCleanup(disposeAccountsWatcher); diff --git a/test/accounts-live-reload.test.ts b/test/accounts-live-reload.test.ts index d903336a..3612f87a 100644 --- a/test/accounts-live-reload.test.ts +++ b/test/accounts-live-reload.test.ts @@ -362,6 +362,35 @@ describe("accounts live reload", () => { await vi.advanceTimersByTimeAsync(5000); expect((await response).status).toBe(200); }); + it("keeps the loaded pool when an external change loads as empty", async () => { + const previous = captured.context?.cachedAccountManagerRef.current; + if (!previous) throw new Error("Missing manager"); + expect(previous.getAccountCount()).toBe(1); + const empty = new AccountManager(undefined, { ...storage(true), accounts: [] }); + const load = vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValueOnce(empty); + await fs.writeFile(path, JSON.stringify(storage(false))); + await tick(); + await settle(); + expect(load).toHaveBeenCalledTimes(1); + expect(captured.context?.cachedAccountManagerRef.current).toBe(previous); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountCount()).toBe(1); + const reloaded = nextReload(); + await vi.advanceTimersByTimeAsync(1500); + await drainReads(); + await reloaded; + expect(load).toHaveBeenCalledTimes(2); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountsSnapshot()[0]?.enabled).toBe(false); + }); + it("adopts an external change that genuinely removes the last account", async () => { + const previous = captured.context?.cachedAccountManagerRef.current; + const reloaded = nextReload(); + await fs.writeFile(path, JSON.stringify({ ...storage(true), accounts: [] })); + await tick(); + await settle(); + await reloaded; + expect(captured.context?.cachedAccountManagerRef.current).not.toBe(previous); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountCount()).toBe(0); + }); it("keeps externally cleared blocks cleared despite queued and late saves from the old manager", async () => { const previous = captured.context?.cachedAccountManagerRef.current; if (!previous) throw new Error("Missing manager"); From d9062ee0dc9e883c82204b146f2a82f2f723760e Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 05:13:05 -0500 Subject: [PATCH 06/11] refactor(storage): let a backup writer take raw bytes and a named path `writePreImportBackupFile` is the only bounded-time, mode-0600, temp-then-rename writer in the storage layer, and it is reachable only by handing it an `AccountStorageV3` to re-serialize. A caller that wants to preserve the bytes a file already holds cannot use it: re-serializing a parsed document silently normalizes it, and a file that no longer parses cannot be handed over at all - which is exactly the file most worth preserving. `writeBackupFileContent` takes the content verbatim and owns the write guarantees; `writePreImportBackupFile` becomes the one-line serializing wrapper over it, so both paths keep the same 0600 mode, the same bounded write time, and the same atomic swap rather than growing a second writer with weaker promises. `createTimestampedBackupPathFor` takes the storage path explicitly. `createTimestampedBackupPath` resolves the *currently active* path, which is wrong for any caller writing a backup of some other file - the global-storage migration does exactly that while a project path is active, and would land its backup beside the wrong accounts file. `getBackupDirectory` names the directory the path builders were computing inline. It is shared by every backup kind - pre-import backups, keychain migration artefacts, pre-global-migration directories - so a caller that prunes has to scope its deletes by its own filename prefix. Naming the directory is what lets that requirement be stated somewhere. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- lib/storage/backup.ts | 59 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/lib/storage/backup.ts b/lib/storage/backup.ts index a8e38939..1ebfeab8 100644 --- a/lib/storage/backup.ts +++ b/lib/storage/backup.ts @@ -6,6 +6,13 @@ * dependency on the import pipeline. `writePreImportBackupFile` is the * bounded-time writer used inside `importAccounts` to snapshot the existing * accounts file before apply. + * + * Every file written here holds live refresh tokens, so the writer is the one + * place that owns the 0600 mode, the bounded write time, and the temp+rename + * swap. `writeBackupFileContent` exists so a caller that already has the exact + * bytes it wants preserved — the pre-write credential snapshotter, which + * copies the previous file verbatim rather than re-serializing it — does not + * hand-roll a second writer with weaker guarantees. */ import { promises as fs } from "node:fs"; @@ -39,22 +46,56 @@ function sanitizeBackupPrefix(prefix: string): string { return safe.length > 0 ? safe : "codex-backup"; } -export function createTimestampedBackupPath(prefix = "codex-backup"): string { - const storagePath = getStoragePath(); - const backupDir = join(dirname(storagePath), "backups"); +/** + * The `backups/` directory that belongs to one accounts file. + * + * Every backup kind shares it: pre-import backups, keychain migration + * artefacts, pre-global-migration directories, and credential snapshots. A + * caller that prunes must therefore scope its deletes by its own filename + * prefix rather than by this directory. + */ +export function getBackupDirectory(storagePath: string): string { + return join(dirname(storagePath), "backups"); +} + +/** + * Timestamped backup path beside an explicitly named accounts file. + * + * Callers that write a backup for a path other than the currently active one — + * the global-storage migration writes to the global file while a project path + * is active — must use this rather than {@link createTimestampedBackupPath}, + * so the backup lands next to the file it describes. + */ +export function createTimestampedBackupPathFor( + storagePath: string, + prefix = "codex-backup", +): string { const safePrefix = sanitizeBackupPrefix(prefix); const nonce = randomBytes(3).toString("hex"); - return join(backupDir, `${safePrefix}-${formatBackupTimestamp()}-${nonce}.json`); + return join( + getBackupDirectory(storagePath), + `${safePrefix}-${formatBackupTimestamp()}-${nonce}.json`, + ); } -export async function writePreImportBackupFile(backupPath: string, snapshot: AccountStorageV3): Promise { +export function createTimestampedBackupPath(prefix = "codex-backup"): string { + return createTimestampedBackupPathFor(getStoragePath(), prefix); +} + +/** + * Write one backup file atomically, with a bounded write time and mode 0600. + * + * `content` is written verbatim. Temp+rename keeps a half-written backup from + * ever being visible under its final name, and the timeout keeps a stuck disk + * from blocking the operation the backup precedes. + */ +export async function writeBackupFileContent(backupPath: string, content: string): Promise { const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${backupPath}.${uniqueSuffix}.tmp`; try { await fs.mkdir(dirname(backupPath), { recursive: true }); - const backupContent = JSON.stringify(snapshot, null, 2); - await writeFileWithTimeout(tempPath, backupContent, PRE_IMPORT_BACKUP_WRITE_TIMEOUT_MS); + await writeFileWithTimeout(tempPath, content, PRE_IMPORT_BACKUP_WRITE_TIMEOUT_MS); await renameWithWindowsRetry(tempPath, backupPath); } catch (error) { try { @@ -65,3 +106,7 @@ export async function writePreImportBackupFile(backupPath: string, snapshot: Acc throw error; } } + +export async function writePreImportBackupFile(backupPath: string, snapshot: AccountStorageV3): Promise { + await writeBackupFileContent(backupPath, JSON.stringify(snapshot, null, 2)); +} From d650513b0782daff98552a3a7d94f738d2670058 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 05:13:39 -0500 Subject: [PATCH 07/11] feat(config): add credential-snapshot settings Two keys for the pre-write credential-store snapshots the next commit adds: - `credentialSnapshots` (default `true`, env `CODEX_AUTH_CREDENTIAL_SNAPSHOTS`). On by default because a snapshot is worth little unless it is recent: restoring from a copy that predates the last few token refreshes brings back accounts whose refresh tokens have since rotated and no longer authenticate. - `credentialSnapshotsMaxCount` (default `10`, env `CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT`). `0` means keep every snapshot rather than keep none - turning the feature off is the boolean's job, and overloading the count with an off switch would make `0` the one value a user can set by accident that silently disables their safety net. The default-config fixtures in `plugin-config.test.ts` assert the whole merged object, so they move with the defaults. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- lib/config.ts | 30 ++++++++++++++++++++++++++++++ lib/schemas.ts | 2 ++ test/plugin-config.test.ts | 10 ++++++++++ 3 files changed, 42 insertions(+) diff --git a/lib/config.ts b/lib/config.ts index 11cc1bb7..70f04724 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -91,6 +91,8 @@ const DEFAULT_CONFIG: PluginConfig = { toastDurationMs: 5_000, accountToasts: true, perProjectAccounts: true, + credentialSnapshots: true, + credentialSnapshotsMaxCount: 10, sessionRecovery: true, autoResume: true, autoUpdate: true, @@ -1013,6 +1015,34 @@ export function getPerProjectAccounts(pluginConfig: PluginConfig): boolean { ); } +/** + * Whether the credential store is snapshotted before a significant write. + * + * On by default: the snapshots are the only recourse if the accounts file is + * ever replaced wholesale, and they are worth little unless they are recent + * enough to hold refresh tokens that still work. + */ +export function getCredentialSnapshots(pluginConfig: PluginConfig): boolean { + return resolveBooleanSetting( + "CODEX_AUTH_CREDENTIAL_SNAPSHOTS", + pluginConfig.credentialSnapshots, + true, + ); +} + +/** + * How many credential snapshots to keep. `0` keeps every snapshot; turning the + * feature off is {@link getCredentialSnapshots}' job, not a magic zero. + */ +export function getCredentialSnapshotsMaxCount(pluginConfig: PluginConfig): number { + return resolveNumberSetting( + "CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT", + pluginConfig.credentialSnapshotsMaxCount, + 10, + { min: 0 }, + ); +} + export function getParallelProbing(pluginConfig: PluginConfig): boolean { return resolveBooleanSetting( "CODEX_AUTH_PARALLEL_PROBING", diff --git a/lib/schemas.ts b/lib/schemas.ts index f66ff438..36c99612 100644 --- a/lib/schemas.ts +++ b/lib/schemas.ts @@ -55,6 +55,8 @@ export const PluginConfigSchema = z.object({ toastDurationMs: z.number().min(1000).optional(), accountToasts: z.boolean().optional(), perProjectAccounts: z.boolean().optional(), + credentialSnapshots: z.boolean().optional(), + credentialSnapshotsMaxCount: z.number().int().min(0).optional(), sessionRecovery: z.boolean().optional(), autoResume: z.boolean().optional(), autoUpdate: z.boolean().optional(), diff --git a/test/plugin-config.test.ts b/test/plugin-config.test.ts index a74b02e5..fe660950 100644 --- a/test/plugin-config.test.ts +++ b/test/plugin-config.test.ts @@ -141,6 +141,8 @@ describe('Plugin Configuration', () => { toastDurationMs: 5_000, accountToasts: true, perProjectAccounts: true, + credentialSnapshots: true, + credentialSnapshotsMaxCount: 10, sessionRecovery: true, autoResume: true, autoUpdate: true, @@ -190,6 +192,8 @@ describe('Plugin Configuration', () => { toastDurationMs: 5_000, accountToasts: true, perProjectAccounts: true, + credentialSnapshots: true, + credentialSnapshotsMaxCount: 10, sessionRecovery: true, autoResume: true, autoUpdate: true, @@ -236,6 +240,8 @@ describe('Plugin Configuration', () => { toastDurationMs: 5_000, accountToasts: true, perProjectAccounts: true, + credentialSnapshots: true, + credentialSnapshotsMaxCount: 10, sessionRecovery: true, autoResume: true, autoUpdate: true, @@ -293,6 +299,8 @@ describe('Plugin Configuration', () => { toastDurationMs: 5_000, accountToasts: true, perProjectAccounts: true, + credentialSnapshots: true, + credentialSnapshotsMaxCount: 10, sessionRecovery: true, autoResume: true, autoUpdate: true, @@ -344,6 +352,8 @@ describe('Plugin Configuration', () => { toastDurationMs: 5_000, accountToasts: true, perProjectAccounts: true, + credentialSnapshots: true, + credentialSnapshotsMaxCount: 10, sessionRecovery: true, autoResume: true, autoUpdate: true, From 81b2d30a940a9621707feaa9eb74eb86c57107a9 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 05:14:39 -0500 Subject: [PATCH 08/11] feat(storage): snapshot the credential store before a significant write The account store is one JSON file holding every account's live refresh token, and nothing stands between it and a process that replaces it wholesale. A test run that has not isolated `HOME`, a bad merge, a half-finished restore: any of them overwrite the pool, and the only way back is whatever backup happens to exist. `codex-export` produces one on demand and `importAccounts` writes one before it applies, so a user who has never run either has nothing at all. Age is the second half of the problem. Refresh tokens are single-use and rotate constantly, so a backup that predates the last few refreshes restores accounts that can no longer authenticate - the file comes back and the credentials in it are already dead. A recovery that restores seven accounts and finds five of them unusable is the case this exists to prevent. So: before a significant write, copy the document currently on disk into `backups/` as `codex-credential-snapshot--.json`. Two decisions carry the design. **The snapshot is of the previous state, not the new one.** It is taken before the replacement lands. Snapshotting the incoming document would be useless for the case this exists for - a clobber would simply be recorded as a clobber - and what is worth keeping is the last good state. **Significance is a denylist.** Every difference counts unless the field is explicitly ignored. A field added to the schema in six months therefore cannot silently switch snapshots off; the worst it can do is cost one extra snapshot. An allowlist fails the other way, and that failure is unrecoverable. Ignored, because it is scheduling churn that is re-derived from upstream on the next request and holds nothing worth restoring: `lastUsed`, `lastSwitchReason`, `rateLimitResetTimes`, `coolingDownUntil`, `cooldownReason`, the `quotaExhausted*` stamps, and the `activeIndex` / `activeIndexByFamily` rotation cursor. The cursor is the load-bearing one: under the default hybrid strategy it moves on essentially every request, so snapshotting on it would churn the whole ring away within minutes and leave nothing but cursor movements to restore from. Everything else is significant, including - deliberately - every token refresh, since `refreshToken`, `accessToken`, `expiresAt` and `tokenRotatedAt` are all absent from the ignore list. That is what keeps the newest snapshot holding tokens that still work. Comparison is against the normalized payload and through a canonical projection that sorts keys and drops `undefined`-valued ones, so a difference that normalization or `JSON.stringify` erases never costs a snapshot. A previous file that no longer parses is treated as changed: the write is about to destroy it, and a file too corrupt to read is precisely the one worth keeping a copy of. Placement and failure behaviour: - Both call sites are already inside `withStorageLock`, so the captured document is exactly the state the write supersedes. - `clearAccounts` snapshots unconditionally before unlinking. Deleting the store outright is the most significant event there is. - A snapshot failure never fails the save. A transient disk error blocking a token refresh would break the user's live sessions, which is strictly worse than a missing snapshot. The one error that does propagate is `TEST_HOME_ESCAPE`: that guard exists to stop a test run writing over real credentials, so swallowing it would disarm it. - No file on disk yet is the ordinary first-write case, not an error. Retention keeps the newest `credentialSnapshotsMaxCount` and prunes **strictly by the snapshot filename prefix**. `backups/` is shared with `codex-pre-import-backup-*`, `codex-backup-*`, `*.migrated-to-keychain.*` and `pre-global-migration-*` directories; deleting one of those would be a data-loss bug inside a feature whose only purpose is preventing data loss. A test seeds one of each and asserts they all survive. Files are written 0600 into a 0700 directory through the shared backup writer, because they hold live refresh tokens. Keychain backend: **out of scope, deliberately.** When `CODEX_KEYCHAIN=1` the keychain holds the authoritative blob and the JSON file is only a post-migration rollback artefact. Snapshotting there would write the whole pool, refresh tokens and all, into a plaintext file in `backups/` - the exact thing a user opting into the OS keychain asked us not to do - and snapshotting the leftover JSON instead would archive a document that is already stale. The code comment at the keychain branch says so. `assertTestRunNeverTouchesRealHome` moves to its own module so the snapshot writer can apply the same guard; importing it from `load-save.ts` would be a cycle, since `load-save.ts` is what triggers snapshots. Behaviour is unchanged. `login-runner.test.ts` counted raw `fs.rename` calls to prove two overlapping persists both landed. The snapshot writer swaps through the same `fs.rename`, so the count now filters on the accounts file as the rename destination, which is what the assertion meant all along. Every new guard was control-run: the production code was broken in the matching way, the test was confirmed to fail, and the source restored byte-identical. Ten of ten failed as required - including that snapshotting the new content instead of the previous content, ignoring credential fields, pruning foreign files, or writing 0644 each trip a test. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- lib/storage/credential-snapshots.ts | 310 ++++++++++++++++ lib/storage/load-save.ts | 53 ++- lib/storage/test-home-guard.ts | 41 +++ test/login-runner.test.ts | 8 +- test/storage-credential-snapshots.test.ts | 421 ++++++++++++++++++++++ 5 files changed, 801 insertions(+), 32 deletions(-) create mode 100644 lib/storage/credential-snapshots.ts create mode 100644 lib/storage/test-home-guard.ts create mode 100644 test/storage-credential-snapshots.test.ts diff --git a/lib/storage/credential-snapshots.ts b/lib/storage/credential-snapshots.ts new file mode 100644 index 00000000..9f261d2b --- /dev/null +++ b/lib/storage/credential-snapshots.ts @@ -0,0 +1,310 @@ +/** + * Pre-write snapshots of the credential store. + * + * The store is a single JSON file holding every account's live refresh token. + * Anything that replaces it wholesale — a bad merge, a test run that escaped + * its sandbox, a partial restore — takes the tokens with it, and a backup old + * enough to predate the last few refreshes restores accounts whose refresh + * tokens have since been rotated and are therefore dead. This module keeps a + * bounded ring of recent snapshots so there is always a *live-token* copy to + * restore from. + * + * Two decisions carry the design: + * + * 1. The snapshot captures the document already on disk, taken before the + * new one replaces it. Snapshotting the incoming document would be + * useless for the case this exists for: a clobber would simply be + * snapshotted as a clobber. What is worth keeping is the last good state. + * + * 2. Significance is a denylist, not an allowlist. Everything counts as a + * significant change unless it is explicitly ignored below. A field added + * to the schema later therefore cannot silently switch snapshots off; the + * worst it can do is cost one extra snapshot, which is recoverable, where + * a missing snapshot is not. + */ + +import { promises as fs } from "node:fs"; +import { join } from "node:path"; +import { loadPluginConfig, getCredentialSnapshots, getCredentialSnapshotsMaxCount } from "../config.js"; +import { createLogger } from "../logger.js"; +import { + createTimestampedBackupPathFor, + getBackupDirectory, + writeBackupFileContent, +} from "./backup.js"; +import { StorageError } from "./errors.js"; +import { assertTestRunNeverTouchesRealHome } from "./test-home-guard.js"; +import type { AccountStorageV3 } from "./migrations.js"; + +const log = createLogger("credential-snapshots"); + +/** + * Filename prefix owned exclusively by this module. + * + * `backups/` is shared with `codex-pre-import-backup-*`, `codex-backup-*`, + * `*.migrated-to-keychain.*` and `pre-global-migration-*`. Retention deletes + * strictly by this prefix, because deleting one of those would be a + * data-loss bug inside a feature whose only purpose is preventing data loss. + */ +export const CREDENTIAL_SNAPSHOT_PREFIX = "codex-credential-snapshot"; + +/** + * Document-level fields that never, on their own, justify a snapshot. + * + * The rotation cursor moves on essentially every request under the default + * hybrid strategy. Snapshotting on it would churn the whole ring away within + * minutes and leave nothing but cursor movements to restore from. + */ +const IGNORED_ROOT_FIELDS: ReadonlySet = new Set([ + "activeIndex", + "activeIndexByFamily", +]); + +/** + * Per-account fields that never, on their own, justify a snapshot. + * + * All of it is scheduling churn: it changes constantly, it is re-derived from + * upstream on the next request, and none of it is recoverable state. Note what + * is deliberately absent — `refreshToken`, `accessToken`, `expiresAt`, + * `tokenRotatedAt` — so every token refresh produces a snapshot. That is the + * case that matters most: a snapshot whose tokens are stale restores accounts + * that cannot authenticate. + */ +const IGNORED_ACCOUNT_FIELDS: ReadonlySet = new Set([ + "lastUsed", + "lastSwitchReason", + "rateLimitResetTimes", + "rateLimitResetTime", + "coolingDownUntil", + "cooldownReason", + "quotaExhaustedUntil", + "quotaExhaustedStampAt", + "quotaExhaustedClearedAt", +]); + +export function isCredentialSnapshotFileName(name: string): boolean { + return name.startsWith(`${CREDENTIAL_SNAPSHOT_PREFIX}-`) && name.endsWith(".json"); +} + +/** + * Sort keys and drop `undefined`-valued ones so two documents with identical + * content compare equal as strings. + * + * This is load-bearing rather than tidiness: the previous document comes from + * `JSON.parse`, so its key order is the file's, while the incoming one is + * built in code. Comparing their raw serializations would report a difference + * on every single write. Dropping `undefined` matches `JSON.stringify`, which + * omits those keys when the document is written, so an in-memory + * `{ email: undefined }` and an on-disk absent `email` are the same state. + */ +function canonicalize(value: unknown): unknown { + if (Array.isArray(value)) return value.map(canonicalize); + if (value === null || typeof value !== "object") return value; + + const source = value as Record; + const result: Record = {}; + for (const key of Object.keys(source).sort()) { + const entry = source[key]; + if (entry === undefined) continue; + result[key] = canonicalize(entry); + } + return result; +} + +function projectAccount(account: unknown): unknown { + if (account === null || typeof account !== "object" || Array.isArray(account)) { + return account; + } + const source = account as Record; + const result: Record = {}; + for (const key of Object.keys(source)) { + if (IGNORED_ACCOUNT_FIELDS.has(key)) continue; + result[key] = source[key]; + } + return result; +} + +/** + * The document reduced to the parts a snapshot exists to preserve. + * + * Accounts are compared position-wise, so a reorder reads as significant. That + * is the intended bias: an extra snapshot costs one file, a missed one costs + * the credentials. + */ +function significantProjection(document: unknown): string { + if (document === null || typeof document !== "object" || Array.isArray(document)) { + return JSON.stringify(canonicalize(document)) ?? "null"; + } + + const source = document as Record; + const projected: Record = {}; + for (const key of Object.keys(source)) { + if (IGNORED_ROOT_FIELDS.has(key) || key === "accounts") continue; + projected[key] = source[key]; + } + + const accounts = source.accounts; + projected.accounts = Array.isArray(accounts) ? accounts.map(projectAccount) : accounts; + + return JSON.stringify(canonicalize(projected)) ?? "null"; +} + +export function isSignificantStorageChange( + previousContent: string, + next: AccountStorageV3, +): boolean { + let previous: unknown; + try { + previous = JSON.parse(previousContent) as unknown; + } catch { + // A file that no longer parses is precisely the state worth preserving: + // the write about to happen replaces it, and whatever it held is then gone + // for good. Treat it as changed so it is captured before that happens. + return true; + } + return significantProjection(previous) !== significantProjection(next); +} + +async function restrictDirectoryMode(directory: string): Promise { + if (process.platform === "win32") return; + try { + await fs.chmod(directory, 0o700); + } catch (error) { + log.warn("Failed to restrict credential snapshot directory to 0700", { + path: directory, + error: String(error), + }); + } +} + +/** + * Delete all but the newest `maxCount` snapshots. + * + * `maxCount <= 0` keeps every snapshot; turning the feature off is the + * boolean setting's job, not a magic zero. + */ +export async function pruneCredentialSnapshots( + backupDirectory: string, + maxCount: number, +): Promise { + if (!Number.isFinite(maxCount) || maxCount <= 0) return; + + let entries: string[]; + try { + entries = await fs.readdir(backupDirectory); + } catch { + return; + } + + const candidates = entries.filter(isCredentialSnapshotFileName); + if (candidates.length <= maxCount) return; + + const dated = await Promise.all( + candidates.map(async (name) => { + const full = join(backupDirectory, name); + let mtimeMs = Number.NEGATIVE_INFINITY; + let isFile = false; + try { + const stats = await fs.stat(full); + mtimeMs = stats.mtimeMs; + isFile = stats.isFile(); + } catch { + // Vanished between readdir and stat: leave it out rather than racing + // another process for the unlink. + } + return { full, name, mtimeMs, isFile }; + }), + ); + + // Newest first. The embedded timestamp breaks mtime ties, which happens when + // several snapshots land inside one filesystem timestamp granule. + const ordered = dated + .filter((entry) => entry.isFile) + .sort((a, b) => { + if (b.mtimeMs !== a.mtimeMs) return b.mtimeMs - a.mtimeMs; + return a.name < b.name ? 1 : a.name > b.name ? -1 : 0; + }); + + for (const stale of ordered.slice(maxCount)) { + try { + await fs.unlink(stale.full); + } catch (error) { + log.warn("Failed to prune credential snapshot", { + path: stale.full, + error: String(error), + }); + } + } +} + +/** + * Preserve the document currently at `storagePath` before it is replaced. + * + * `next` is the document about to be written, or `null` when the store is + * about to be deleted outright — deletion is unconditionally significant. + * + * Callers must already hold the storage lock, so the snapshot is consistent + * with the write it precedes. + */ +export async function snapshotCredentialStoreBeforeWrite( + storagePath: string, + next: AccountStorageV3 | null, +): Promise { + const config = loadPluginConfig(); + if (!getCredentialSnapshots(config)) return; + + const backupDirectory = getBackupDirectory(storagePath); + assertTestRunNeverTouchesRealHome(backupDirectory); + + let previousContent: string; + try { + previousContent = await fs.readFile(storagePath, "utf-8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + log.warn("Skipping credential snapshot: existing store is unreadable", { + path: storagePath, + error: String(error), + }); + } + // No file yet is the ordinary first-write case: there is no previous state + // to preserve, which is not an error. + return; + } + + if (next !== null && !isSignificantStorageChange(previousContent, next)) return; + + const snapshotPath = createTimestampedBackupPathFor(storagePath, CREDENTIAL_SNAPSHOT_PREFIX); + await fs.mkdir(backupDirectory, { recursive: true, mode: 0o700 }); + await restrictDirectoryMode(backupDirectory); + await writeBackupFileContent(snapshotPath, previousContent); + log.info("Captured credential store snapshot", { path: snapshotPath }); + + await pruneCredentialSnapshots(backupDirectory, getCredentialSnapshotsMaxCount(config)); +} + +/** + * {@link snapshotCredentialStoreBeforeWrite}, with every failure downgraded to + * a warning. + * + * A snapshot is a safety net, never a precondition. Letting a transient disk + * error fail the write it precedes would break a token refresh, and therefore + * the user's live sessions, to protect a copy of the file — strictly worse + * than having no snapshot. The one exception is the test-home guard: that + * exists to stop a test run writing over real credentials, so swallowing it + * would disarm it. + */ +export async function trySnapshotCredentialStoreBeforeWrite( + storagePath: string, + next: AccountStorageV3 | null, +): Promise { + try { + await snapshotCredentialStoreBeforeWrite(storagePath, next); + } catch (error) { + if (error instanceof StorageError && error.code === "TEST_HOME_ESCAPE") throw error; + log.warn("Credential snapshot failed; continuing with the write", { + path: storagePath, + error: String(error), + }); + } +} diff --git a/lib/storage/load-save.ts b/lib/storage/load-save.ts index ef20c727..3fedc278 100644 --- a/lib/storage/load-save.ts +++ b/lib/storage/load-save.ts @@ -25,7 +25,9 @@ import { AnyAccountStorageSchema, getValidationErrors } from "../schemas.js"; import { renameWithWindowsRetry } from "./atomic-write.js"; import { formatStorageErrorHint, StorageError } from "./errors.js"; import { normalizeAccountStorage } from "./normalize.js"; -import { getConfigDir, isWithinDirectory } from "./paths.js"; +import { getConfigDir } from "./paths.js"; +import { assertTestRunNeverTouchesRealHome } from "./test-home-guard.js"; +import { trySnapshotCredentialStoreBeforeWrite } from "./credential-snapshots.js"; import { getCurrentLegacyProjectStoragePath, getCurrentProjectRoot, @@ -175,36 +177,6 @@ async function checkWorktreeLockForCurrentStorage( } } -/** - * Refuse to mutate account storage inside the developer's real home while the - * test suite is running. - * - * `vitest.config.ts` redirects HOME to a sandbox before any module loads, but a - * test that restores the captured real HOME, or a future regression in that - * config, would otherwise write fixtures straight over live ChatGPT - * credentials. `os.userInfo()` reads the passwd entry instead of `$HOME`, so it - * still names the real home after the redirect and gives the check something - * the sandbox cannot spoof. Inert outside vitest. - */ -function assertTestRunNeverTouchesRealHome(path: string): void { - if (!process.env.VITEST) return; - - let realHome: string; - try { - realHome = os.userInfo().homedir; - } catch { - return; - } - if (!realHome || !isWithinDirectory(realHome, path)) return; - - throw new StorageError( - `Refusing to write account storage inside the real home directory during a test run: ${path}`, - "TEST_HOME_ESCAPE", - path, - "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", - ); -} - async function ensureGitignore(storagePath: string): Promise { if (!getCurrentStoragePath()) return; @@ -582,6 +554,12 @@ async function writeAccountsToPathUnlocked(path: string, storage: AccountStorage // Normalize before persisting so every write path enforces dedup semantics // (exact identity dedupe plus legacy email dedupe for identity-less records). const normalizedStorage = normalizeAccountStorage(storage) ?? storage; + // Preserve what is on disk now, before it is replaced. Compared against + // the normalized payload rather than the caller's, so a difference + // normalization erases never costs a snapshot. We are already inside + // `withStorageLock`, so the captured state is exactly the state this write + // supersedes. + await trySnapshotCredentialStoreBeforeWrite(path, normalizedStorage); const content = JSON.stringify(normalizedStorage, null, 2); await fs.writeFile(tempPath, content, { encoding: "utf-8", mode: 0o600 }); @@ -707,6 +685,15 @@ async function saveAccountsUnlocked(storage: AccountStorageV3): Promise { await checkWorktreeLockForCurrentStorage("save"); if (isKeychainOptInEnabled()) { + // Credential snapshots are deliberately scoped to the JSON backend and do + // not cover this branch. Snapshotting here would mean writing the account + // pool, refresh tokens and all, into a plaintext file in `backups/` — the + // exact thing a user opting into the OS keychain asked us not to do. The + // on-disk JSON that remains is a rollback artefact, not the live store, so + // snapshotting it instead would archive a document that is already stale. + // Keychain users' recovery path stays `codex-export` plus the keychain's + // own backing store. + // // Normalize before serializing so the keychain receives the same shape // the JSON backend would have written. Using the same JSON format keeps // migration and rollback symmetric: a rolled-back JSON file is valid @@ -795,6 +782,10 @@ export async function clearAccounts(): Promise { try { const path = getStoragePath(); assertTestRunNeverTouchesRealHome(path); + // Deleting the store outright is the most significant event there is, so + // this snapshot is unconditional; `null` says there is no successor + // document to compare against. + await trySnapshotCredentialStoreBeforeWrite(path, null); await fs.unlink(path); } catch (error) { const code = (error as NodeJS.ErrnoException).code; diff --git a/lib/storage/test-home-guard.ts b/lib/storage/test-home-guard.ts new file mode 100644 index 00000000..9379559d --- /dev/null +++ b/lib/storage/test-home-guard.ts @@ -0,0 +1,41 @@ +/** + * The test-run write guard, shared by every storage writer. + * + * Extracted from `lib/storage/load-save.ts` so the credential-snapshot writer + * can apply the same check. Importing it from `load-save.ts` directly would be + * a cycle: `load-save.ts` is what triggers snapshots in the first place. + */ + +import os from "node:os"; +import { StorageError } from "./errors.js"; +import { isWithinDirectory } from "./paths.js"; + +/** + * Refuse to mutate account storage inside the developer's real home while the + * test suite is running. + * + * `vitest.config.ts` redirects HOME to a sandbox before any module loads, but a + * test that restores the captured real HOME, or a future regression in that + * config, would otherwise write fixtures straight over live ChatGPT + * credentials. `os.userInfo()` reads the passwd entry instead of `$HOME`, so it + * still names the real home after the redirect and gives the check something + * the sandbox cannot spoof. Inert outside vitest. + */ +export function assertTestRunNeverTouchesRealHome(path: string): void { + if (!process.env.VITEST) return; + + let realHome: string; + try { + realHome = os.userInfo().homedir; + } catch { + return; + } + if (!realHome || !isWithinDirectory(realHome, path)) return; + + throw new StorageError( + `Refusing to write account storage inside the real home directory during a test run: ${path}`, + "TEST_HOME_ESCAPE", + path, + "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", + ); +} diff --git a/test/login-runner.test.ts b/test/login-runner.test.ts index b305d025..b57e12e0 100644 --- a/test/login-runner.test.ts +++ b/test/login-runner.test.ts @@ -218,7 +218,13 @@ describe("login-runner persistAccountPool", () => { resolveFirstRename?.(); await Promise.all([firstPersist, secondPersist]); - expect(renameSpy).toHaveBeenCalledTimes(2); + // Count only the renames that publish the accounts file. The + // pre-write credential snapshotter swaps its own file through the + // same `fs.rename`, so a raw call count also counts snapshots. + const accountFileRenames = renameSpy.mock.calls.filter( + ([, destinationPath]) => destinationPath === storagePath, + ); + expect(accountFileRenames).toHaveLength(2); const loaded = await loadAccounts(); expect(loaded?.accounts).toHaveLength(2); expect( diff --git a/test/storage-credential-snapshots.test.ts b/test/storage-credential-snapshots.test.ts new file mode 100644 index 00000000..683e906f --- /dev/null +++ b/test/storage-credential-snapshots.test.ts @@ -0,0 +1,421 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { promises as fs } from "node:fs"; +import { join } from "node:path"; +import { mkdtemp } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { + clearAccounts, + saveAccounts, + setStoragePathDirect, + type AccountStorageV3, +} from "../lib/storage.js"; +import { + CREDENTIAL_SNAPSHOT_PREFIX, + isCredentialSnapshotFileName, + isSignificantStorageChange, + pruneCredentialSnapshots, +} from "../lib/storage/credential-snapshots.js"; +import { MODEL_FAMILIES } from "../lib/prompts/codex.js"; + +type StoredAccount = AccountStorageV3["accounts"][number]; + +let testDir: string; +let storagePath: string; +let backupsDir: string; + +function makeAccount(overrides: Partial = {}): StoredAccount { + return { + accountId: "acct-1", + email: "one@example.com", + refreshToken: "rt-1", + accessToken: "at-1", + expiresAt: 1_900_000_000_000, + addedAt: 1_000, + lastUsed: 2_000, + ...overrides, + }; +} + +function makeStorage(): AccountStorageV3 { + return { + version: 3, + accounts: [ + makeAccount(), + makeAccount({ + accountId: "acct-2", + email: "two@example.com", + refreshToken: "rt-2", + accessToken: "at-2", + }), + ], + activeIndex: 0, + }; +} + +function withAccount( + storage: AccountStorageV3, + index: number, + mutate: (account: StoredAccount) => StoredAccount, +): AccountStorageV3 { + return { + ...storage, + accounts: storage.accounts.map((account, i) => (i === index ? mutate({ ...account }) : account)), + }; +} + +async function listSnapshotNames(): Promise { + try { + return (await fs.readdir(backupsDir)).filter(isCredentialSnapshotFileName).sort(); + } catch { + return []; + } +} + +async function readSnapshotContents(): Promise { + const names = await listSnapshotNames(); + return Promise.all(names.map((name) => fs.readFile(join(backupsDir, name), "utf-8"))); +} + +async function readLiveStore(): Promise { + return JSON.parse(await fs.readFile(storagePath, "utf-8")) as AccountStorageV3; +} + +async function exists(path: string): Promise { + try { + await fs.stat(path); + return true; + } catch { + return false; + } +} + +beforeEach(async () => { + testDir = await mkdtemp(join(tmpdir(), "oc-codex-credential-snapshots-")); + storagePath = join(testDir, "oc-codex-multi-auth-accounts.json"); + backupsDir = join(testDir, "backups"); + setStoragePathDirect(storagePath); +}); + +afterEach(async () => { + setStoragePathDirect(null); + vi.unstubAllEnvs(); + await fs.rm(testDir, { recursive: true, force: true }); +}); + +describe("credential snapshots: significance", () => { + it("writes no snapshot on the first save because there is no previous state", async () => { + await saveAccounts(makeStorage()); + + expect(await listSnapshotNames()).toEqual([]); + expect(await exists(backupsDir)).toBe(false); + }); + + const ignoredChanges: Array<[string, (storage: AccountStorageV3) => AccountStorageV3]> = [ + [ + "lastUsed", + (storage) => withAccount(storage, 0, (a) => ({ ...a, lastUsed: a.lastUsed + 90_000 })), + ], + [ + "lastSwitchReason", + (storage) => withAccount(storage, 0, (a) => ({ ...a, lastSwitchReason: "rotation" })), + ], + [ + "rateLimitResetTimes", + (storage) => + withAccount(storage, 0, (a) => ({ + ...a, + rateLimitResetTimes: { [MODEL_FAMILIES[0]]: Date.now() + 60_000 }, + })), + ], + [ + "coolingDownUntil and cooldownReason", + (storage) => + withAccount(storage, 0, (a) => ({ + ...a, + coolingDownUntil: Date.now() + 60_000, + cooldownReason: "network-error", + })), + ], + [ + "quota exhaustion stamps", + (storage) => + withAccount(storage, 0, (a) => ({ + ...a, + quotaExhaustedUntil: Date.now() + 60_000, + quotaExhaustedStampAt: Date.now(), + })), + ], + ["activeIndex", (storage) => ({ ...storage, activeIndex: 1 })], + [ + "activeIndexByFamily", + (storage) => ({ ...storage, activeIndexByFamily: { [MODEL_FAMILIES[0]]: 1 } }), + ], + ]; + + it.each(ignoredChanges)("takes no snapshot for a %s-only change", async (_label, mutate) => { + const base = makeStorage(); + await saveAccounts(base); + + await saveAccounts(mutate(base)); + + expect(await listSnapshotNames()).toEqual([]); + }); + + const significantChanges: Array<[string, (storage: AccountStorageV3) => AccountStorageV3]> = [ + [ + "account added", + (storage) => ({ + ...storage, + accounts: [ + ...storage.accounts, + makeAccount({ accountId: "acct-3", email: "three@example.com", refreshToken: "rt-3" }), + ], + }), + ], + ["account removed", (storage) => ({ ...storage, accounts: storage.accounts.slice(0, 1) })], + [ + "refresh token rotated", + (storage) => + withAccount(storage, 0, (a) => ({ + ...a, + refreshToken: "rt-rotated", + accessToken: "at-rotated", + expiresAt: (a.expiresAt ?? 0) + 3_600_000, + tokenRotatedAt: 1_800_000_000_000, + })), + ], + [ + "access token replaced", + (storage) => withAccount(storage, 0, (a) => ({ ...a, accessToken: "at-fresh" })), + ], + [ + "expiry moved", + (storage) => + withAccount(storage, 0, (a) => ({ ...a, expiresAt: (a.expiresAt ?? 0) + 600_000 })), + ], + ["label set", (storage) => withAccount(storage, 0, (a) => ({ ...a, accountLabel: "Work" }))], + ["tags set", (storage) => withAccount(storage, 0, (a) => ({ ...a, accountTags: ["work"] }))], + ["note set", (storage) => withAccount(storage, 0, (a) => ({ ...a, accountNote: "primary" }))], + ["disabled", (storage) => withAccount(storage, 0, (a) => ({ ...a, enabled: false }))], + [ + "email changed", + (storage) => withAccount(storage, 0, (a) => ({ ...a, email: "renamed@example.com" })), + ], + [ + "accountUserId set", + (storage) => withAccount(storage, 0, (a) => ({ ...a, accountUserId: "member-9" })), + ], + [ + "accountIdSource changed", + (storage) => withAccount(storage, 0, (a) => ({ ...a, accountIdSource: "manual" })), + ], + ["plan type changed", (storage) => withAccount(storage, 0, (a) => ({ ...a, planType: "pro" }))], + [ + "oauth scope changed", + (storage) => withAccount(storage, 0, (a) => ({ ...a, oauthScope: "openid profile" })), + ], + ]; + + it.each(significantChanges)("snapshots a %s", async (_label, mutate) => { + const base = makeStorage(); + await saveAccounts(base); + const before = await fs.readFile(storagePath, "utf-8"); + + await saveAccounts(mutate(base)); + + expect(await readSnapshotContents()).toEqual([before]); + }); + + it("preserves the previous document rather than the incoming one", async () => { + const base = makeStorage(); + await saveAccounts(base); + + await saveAccounts( + withAccount(base, 0, (a) => ({ ...a, refreshToken: "rt-clobbered-by-a-bad-write" })), + ); + + const [snapshot] = await readSnapshotContents(); + const snapshotDoc = JSON.parse(snapshot) as AccountStorageV3; + expect(snapshotDoc.accounts[0].refreshToken).toBe("rt-1"); + expect((await readLiveStore()).accounts[0].refreshToken).toBe("rt-clobbered-by-a-bad-write"); + }); + + it("snapshots a store that no longer parses", async () => { + await fs.writeFile(storagePath, "{ this is not json", "utf-8"); + + await saveAccounts(makeStorage()); + + expect(await readSnapshotContents()).toEqual(["{ this is not json"]); + }); + + it("treats a storage schema version change as significant", () => { + const previous = JSON.stringify({ + version: 1, + accounts: [{ refreshToken: "rt-1", addedAt: 1, lastUsed: 2 }], + activeIndex: 0, + }); + + expect( + isSignificantStorageChange(previous, { + version: 3, + accounts: [{ refreshToken: "rt-1", addedAt: 1, lastUsed: 2 }], + activeIndex: 0, + }), + ).toBe(true); + }); + + it("ignores key order and undefined-valued keys in the incoming document", () => { + const previous = JSON.stringify({ + version: 3, + activeIndex: 0, + accounts: [{ addedAt: 1, lastUsed: 2, refreshToken: "rt-1" }], + }); + + expect( + isSignificantStorageChange(previous, { + version: 3, + accounts: [{ refreshToken: "rt-1", addedAt: 1, lastUsed: 2, email: undefined }], + activeIndex: 0, + }), + ).toBe(false); + }); +}); + +describe("credential snapshots: clearAccounts", () => { + it("snapshots the store before deleting it", async () => { + await saveAccounts(makeStorage()); + const before = await fs.readFile(storagePath, "utf-8"); + + await clearAccounts(); + + expect(await exists(storagePath)).toBe(false); + expect(await readSnapshotContents()).toEqual([before]); + }); + + it("writes nothing when there is no store to delete", async () => { + await clearAccounts(); + + expect(await listSnapshotNames()).toEqual([]); + }); +}); + +describe("credential snapshots: retention", () => { + async function seedForeignBackups(): Promise { + await fs.mkdir(backupsDir, { recursive: true }); + const foreignFiles = [ + join(backupsDir, "codex-pre-import-backup-20250101-000000000-aaaaaa.json"), + join(backupsDir, "codex-backup-20250101-000000000-bbbbbb.json"), + join(backupsDir, "oc-codex-multi-auth-accounts.json.migrated-to-keychain.2025-01-01T00-00-00-000Z"), + ]; + for (const file of foreignFiles) { + await fs.writeFile(file, "foreign", "utf-8"); + } + const foreignDir = join(backupsDir, "pre-global-migration-20250101-000000000"); + await fs.mkdir(foreignDir, { recursive: true }); + return [...foreignFiles, foreignDir]; + } + + it("keeps only the configured number of snapshots across repeated saves", async () => { + vi.stubEnv("CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT", "3"); + const foreign = await seedForeignBackups(); + + const base = makeStorage(); + await saveAccounts(base); + for (let generation = 1; generation <= 5; generation += 1) { + await saveAccounts( + withAccount(base, 0, (a) => ({ ...a, accountLabel: `generation-${generation}` })), + ); + } + + expect(await listSnapshotNames()).toHaveLength(3); + for (const path of foreign) { + expect(await exists(path), `${path} must survive pruning`).toBe(true); + } + }); + + it("deletes the oldest snapshots and never a foreign file", async () => { + const foreign = await seedForeignBackups(); + const snapshots = ["oldest", "older", "old", "newer", "newest"]; + for (const [index, marker] of snapshots.entries()) { + const path = join( + backupsDir, + `${CREDENTIAL_SNAPSHOT_PREFIX}-2025010${index + 1}-000000000-abcdef.json`, + ); + await fs.writeFile(path, marker, "utf-8"); + const stamp = new Date(1_700_000_000_000 + index * 60_000); + await fs.utimes(path, stamp, stamp); + } + + await pruneCredentialSnapshots(backupsDir, 2); + + expect((await readSnapshotContents()).sort()).toEqual(["newer", "newest"].sort()); + for (const path of foreign) { + expect(await exists(path), `${path} must survive pruning`).toBe(true); + } + }); + + it("keeps every snapshot when the max count is zero", async () => { + await fs.mkdir(backupsDir, { recursive: true }); + for (let index = 0; index < 4; index += 1) { + await fs.writeFile( + join(backupsDir, `${CREDENTIAL_SNAPSHOT_PREFIX}-2025010${index + 1}-000000000-abcdef.json`), + `snapshot-${index}`, + "utf-8", + ); + } + + await pruneCredentialSnapshots(backupsDir, 0); + + expect(await listSnapshotNames()).toHaveLength(4); + }); + + it("recognizes only its own filenames", () => { + expect(isCredentialSnapshotFileName(`${CREDENTIAL_SNAPSHOT_PREFIX}-20250101-000000000-ab.json`)) + .toBe(true); + expect(isCredentialSnapshotFileName("codex-pre-import-backup-20250101-000000000-ab.json")) + .toBe(false); + expect(isCredentialSnapshotFileName("codex-backup-20250101-000000000-ab.json")).toBe(false); + expect(isCredentialSnapshotFileName(`${CREDENTIAL_SNAPSHOT_PREFIX}.json`)).toBe(false); + }); +}); + +describe("credential snapshots: safety", () => { + it.skipIf(process.platform === "win32")( + "writes snapshots as 0600 inside a 0700 directory", + async () => { + const base = makeStorage(); + await saveAccounts(base); + await saveAccounts(withAccount(base, 0, (a) => ({ ...a, refreshToken: "rt-rotated" }))); + + const [name] = await listSnapshotNames(); + const fileStats = await fs.stat(join(backupsDir, name)); + const dirStats = await fs.stat(backupsDir); + expect(fileStats.mode & 0o777).toBe(0o600); + expect(dirStats.mode & 0o777).toBe(0o700); + }, + ); + + it("does not fail the save when the snapshot cannot be written", async () => { + const base = makeStorage(); + await saveAccounts(base); + await fs.writeFile(backupsDir, "not a directory", "utf-8"); + + await expect( + saveAccounts(withAccount(base, 0, (a) => ({ ...a, refreshToken: "rt-rotated" }))), + ).resolves.toBeUndefined(); + + expect((await readLiveStore()).accounts[0].refreshToken).toBe("rt-rotated"); + expect(await fs.readFile(backupsDir, "utf-8")).toBe("not a directory"); + }); + + it("writes nothing at all when the feature is disabled", async () => { + vi.stubEnv("CODEX_AUTH_CREDENTIAL_SNAPSHOTS", "0"); + + const base = makeStorage(); + await saveAccounts(base); + await saveAccounts(withAccount(base, 0, (a) => ({ ...a, refreshToken: "rt-rotated" }))); + await clearAccounts(); + + expect(await exists(backupsDir)).toBe(false); + }); +}); From 5e0cb60f97c83ffb80001f2475009cfe4542daae Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 05:15:16 -0500 Subject: [PATCH 09/11] docs(storage): document credential-store snapshots Both config keys land in `docs/configuration.md` and `docs/development/CONFIG_FIELDS.md` with their defaults, their env overrides, and their numeric bounds, alongside the existing keys. The parts a user needs in order to rely on the feature, rather than just know it exists: - the snapshot holds the state being *replaced*, not the state replacing it, which is what makes it useful after a wholesale overwrite; - token refreshes count as significant, so the newest snapshot holds tokens that still authenticate; - rotation bookkeeping does not, so ordinary traffic cannot churn the kept snapshots away; - retention prunes strictly by the snapshot filename prefix, so nothing else in `backups/` is at risk; - `0` for the count means keep every snapshot, and `credentialSnapshots: false` is how you turn it off; - a snapshot failure never fails the write it precedes; - `CODEX_KEYCHAIN=1` is not covered. The file-locations tables gain the snapshot path and its permissions, so someone recovering from a clobber can find the files without reading the source. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- AGENTS.md | 1 + README.md | 4 ++++ docs/configuration.md | 7 +++++++ docs/development/CONFIG_FIELDS.md | 3 +++ lib/AGENTS.md | 2 ++ 5 files changed, 17 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index b2981888..a97313a2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,6 +112,7 @@ oc-codex-multi-auth doctor - Per-project accounts: `~/.opencode/projects//oc-codex-multi-auth-accounts.json`. - Global accounts: `~/.opencode/oc-codex-multi-auth-accounts.json`. - Flagged accounts: `oc-codex-multi-auth-flagged-accounts.json`, written beside the active accounts file (per project when `perProjectAccounts` is on). +- Credential snapshots: `backups/codex-credential-snapshot-*.json`, written beside the active accounts file. Holds the previous store content, captured before a significant write; retention prunes strictly by that prefix so it never deletes another backup kind. - Quota notification state: `oc-codex-multi-auth-quota-notifications.json`, written beside the active accounts file (per project when `perProjectAccounts` is on). - Request logs: `~/.opencode/logs/codex-plugin/` when logging is enabled. - Model catalog: 13 modern bases / 59 variants; legacy 59 explicit. diff --git a/README.md b/README.md index 11b4e230..f87e09e6 100644 --- a/README.md +++ b/README.md @@ -421,6 +421,8 @@ Selected runtime/environment overrides: | `CODEX_TUI_MASK_EMAIL=0/1` | Mask account emails across account-display surfaces (list/status/limits/health/dashboard/menus + TUI quota status) | | `CODEX_TUI_MASK_EMAIL_DETAILS=0/1` | Also hide account email in quota details when prompt masking is enabled | | `CODEX_AUTH_PER_PROJECT_ACCOUNTS=0/1` | Disable/enable per-project account pools | +| `CODEX_AUTH_CREDENTIAL_SNAPSHOTS=0/1` | Disable/enable pre-write snapshots of the credential store (default on) | +| `CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT=` | How many credential snapshots to keep (`0` keeps all of them) | | `CODEX_AUTH_AUTO_UPDATE=0/1` | Disable/enable daily npm update check and cache refresh | | `CODEX_AUTH_ROTATION_STRATEGY=hybrid\|sticky\|round-robin` | Account selection strategy | | `CODEX_AUTH_UNSUPPORTED_MODEL_POLICY=strict\|fallback` | Control unsupported-model retry behavior | @@ -456,6 +458,8 @@ By default, account pools are stored locally as V3 JSON files. File permissions Use JSON storage when you want predictable, inspectable local files and easy backup/export behavior. +Before the store is changed in a way that matters, the plugin copies the previous version of the file into `backups/` as `codex-credential-snapshot-*.json`, mode `0600` in a `0700` directory. The snapshot holds the state being replaced, not the state replacing it, which is what makes it useful if the file is ever overwritten wholesale. Token refreshes count as significant, so the newest snapshot holds refresh tokens that still work; a snapshot old enough to predate the last few refreshes restores accounts that can no longer authenticate. Rotation bookkeeping - `lastUsed`, rate-limit and cooldown state, quota stamps, and the rotation cursor - never triggers one on its own, so the kept snapshots are not churned away by ordinary traffic. The plugin keeps the 10 most recent and prunes strictly by that filename prefix, so nothing else in `backups/` is touched. Set `credentialSnapshots: false` to turn it off, or `credentialSnapshotsMaxCount` to keep a different number (`0` keeps all of them). +
diff --git a/docs/configuration.md b/docs/configuration.md index f438246c..0c50c0ba 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -224,6 +224,8 @@ a restart to change their configuration. "server": 2 }, "perProjectAccounts": true, + "credentialSnapshots": true, + "credentialSnapshotsMaxCount": 10, "autoUpdate": true, "toastDurationMs": 5000, "accountToasts": true, @@ -281,6 +283,8 @@ The sample above intentionally sets `"retryAllAccountsMaxRetries": 3` as a bound | `retryProfile` | `balanced` | retry budget profile for request classes (`conservative`, `balanced`, `aggressive`) | | `retryBudgetOverrides` | `{}` | optional per-class budget overrides (`authRefresh`, `network`, `server`, `rateLimitShort`, `rateLimitGlobal`, `emptyResponse`) | | `perProjectAccounts` | `true` | each project gets its own account storage | +| `credentialSnapshots` | `true` | before a significant change to the account store, copy the previous on-disk version into `backups/` so there is always a recent copy holding refresh tokens that still work. Snapshots are taken for account additions and removals, token refreshes, identity changes, label/tag/note/enabled changes, plan changes, schema-version changes, and deletion of the store. Rotation bookkeeping never triggers one on its own: `lastUsed`, `lastSwitchReason`, rate-limit and cooldown state, quota-exhaustion stamps, and the `activeIndex` / `activeIndexByFamily` rotation cursor. A snapshot failure is logged and never fails the write it precedes. Snapshots cover the default JSON backend only, not `CODEX_KEYCHAIN=1` | +| `credentialSnapshotsMaxCount` | `10` | how many credential snapshots to keep. Pruning deletes strictly by the snapshot filename prefix, so other files in `backups/` are never touched. `0` means keep every snapshot; use `credentialSnapshots: false` to turn the feature off | | `autoUpdate` | `true` | check npm daily and clear the OpenCode-managed plugin cache on exit when a newer version is available; restart OpenCode to install it | | `toastDurationMs` | `5000` | how long toast notifications stay visible (ms) | | `accountToasts` | `true` | show the transient `Using (N/N)` account-selection toast; set `false` to hide only this informational toast (rate-limit/auth/recovery warnings and errors still show) | @@ -469,6 +473,8 @@ override any config with env vars (boolean values are truthy only for `"1"`): | `CODEX_AUTH_BEGINNER_SAFE_MODE=1` | enable beginner-safe retry behavior | | `CODEX_AUTH_RETRY_PROFILE=aggressive` | override retry profile (`conservative`, `balanced`, `aggressive`) | | `CODEX_AUTH_PER_PROJECT_ACCOUNTS=0` | disable per-project accounts | +| `CODEX_AUTH_CREDENTIAL_SNAPSHOTS=0` | disable pre-write credential-store snapshots (enabled by default) | +| `CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT=25` | how many credential snapshots to keep (`0` keeps all of them) | | `CODEX_AUTH_PARALLEL_PROBING=1` | enable concurrent account health probes | | `CODEX_AUTH_PARALLEL_PROBING_MAX_CONCURRENCY=3` | max concurrent probes (1–5) | | `CODEX_AUTH_EMPTY_RESPONSE_MAX_RETRIES=3` | override empty-response retry count | @@ -616,6 +622,7 @@ opencode run "task" --model=openai/gpt-5.6-sol-high | `~/.opencode/oc-codex-multi-auth-accounts.json` | global V3 account pool | | `~/.opencode/projects//oc-codex-multi-auth-accounts.json` | per-project account pool | | `~/.opencode/projects//oc-codex-multi-auth-flagged-accounts.json` | flagged/deactivated account metadata, written beside the active accounts file. With the default `perProjectAccounts` this is the per-project path; with project storage off it is `~/.opencode/oc-codex-multi-auth-flagged-accounts.json` | +| `~/.opencode/backups/codex-credential-snapshot-*.json` | pre-write credential-store snapshots, written beside the accounts file they belong to (so the per-project `backups/` directory when `perProjectAccounts` is on). Mode `0600` in a `0700` directory, because they hold live refresh tokens | | `~/.opencode/logs/codex-plugin/` | request/debug logs when enabled | | `~/.opencode/cache/` | instruction/catalog and auto-update caches | | `~/.local/state/opencode/oc-codex-multi-auth-tui-quota.json` | TUI quota snapshot cache shared by the provider and TUI plugins; `$OPENCODE_STATE_DIR` overrides the directory when set | diff --git a/docs/development/CONFIG_FIELDS.md b/docs/development/CONFIG_FIELDS.md index 5bbe403b..14e52733 100644 --- a/docs/development/CONFIG_FIELDS.md +++ b/docs/development/CONFIG_FIELDS.md @@ -248,6 +248,8 @@ Defaults come from `lib/config.ts` / `lib/schemas.ts`. Environment overrides win | `toastDurationMs` | `5000` | `CODEX_AUTH_TOAST_DURATION_MS` | Toast visibility duration | | `accountToasts` | `true` | `CODEX_AUTH_ACCOUNT_TOASTS` | Gates only the informational "Using \ (N/N)" selection toast; warning/error toasts are unaffected | | `perProjectAccounts` | `true` | `CODEX_AUTH_PER_PROJECT_ACCOUNTS` | Project-scoped account pools | +| `credentialSnapshots` | `true` | `CODEX_AUTH_CREDENTIAL_SNAPSHOTS` | Copy the previous account store into `backups/` before a significant change | +| `credentialSnapshotsMaxCount` | `10` | `CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT` | Snapshots kept; `0` keeps all of them, and disabling is `credentialSnapshots`' job | | `sessionRecovery` | `true` | `CODEX_AUTH_SESSION_RECOVERY` | Auto-recover common API errors | | `autoResume` | `true` | `CODEX_AUTH_AUTO_RESUME` | Auto-resume after thinking-block recovery | | `autoUpdate` | `true` | `CODEX_AUTH_AUTO_UPDATE` | Daily npm update check + cache refresh | @@ -277,6 +279,7 @@ Defaults come from `lib/config.ts` / `lib/schemas.ts`. Environment overrides win | `streamStallTimeoutMs` | at least 1000 | 1000 | | `quotaNotifications.intervalMs` | at least 30000 | clamped up to 30000 | | `retryBudgetOverrides.*` | integer, at least 0 | (file only) | +| `credentialSnapshotsMaxCount` | integer, at least 0 | 0, no ceiling | So `parallelProbingMaxConcurrency: 9` in the file falls back to the default `2`, while `CODEX_AUTH_PARALLEL_PROBING_MAX_CONCURRENCY=9` is accepted with no ceiling. diff --git a/lib/AGENTS.md b/lib/AGENTS.md index 1729e4b6..8e9c0aa5 100644 --- a/lib/AGENTS.md +++ b/lib/AGENTS.md @@ -78,6 +78,8 @@ lib/ | Storage keychain | `storage/keychain.ts` | optional native keychain backend | | Storage migrations | `storage/migrations.ts` | V1 → V3 upgrade; V2 files throw a StorageError with code UNKNOWN_V2_FORMAT | | Backups/import/export | `storage/backup.ts`, `storage/export-import.ts` | timestamped backups and dry-run import preview | +| Credential snapshots | `storage/credential-snapshots.ts` | pre-write copy of the previous account store, denylist significance check, prefix-scoped retention | +| Test-home write guard | `storage/test-home-guard.ts` | refuses storage writes inside the real home during a vitest run | | Tool registry | `tools/index.ts` | `ToolContext`, `createToolRegistry` | | TUI quota status | `tui-status.ts`, `tui-quota-cache.ts`, `codex-usage.ts` | prompt quota display and usage cache | | Error types | `errors.ts`, `error-sentinels.ts` | StorageError and structured sentinel errors | From cc742c174f1248d3b82cc147cb380f6accb3dc40 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 13:36:29 -0500 Subject: [PATCH 10/11] fix(storage): enforce the snapshot exclusions the clear path ignored Credential snapshots ship with two stated exclusions. Neither was actually enforced on `clearAccounts`, so both held for ordinary saves and silently lapsed on the one path that destroys the store. Keychain mode was scoped out by the absence of a snapshot call in the keychain branch of `saveAccountsUnlocked`, which is not a mechanism - it only covers the call sites that happen to sit inside that branch. `clearAccounts` calls the snapshotter unconditionally, and the JSON fallback taken after a failed keychain write reaches `writeAccountsToPathUnlocked`, which calls it too. Under `CODEX_KEYCHAIN=1` either one copied the whole account pool, refresh tokens included, into a plaintext file in `backups/` - precisely the artefact a user who opted into the OS keychain asked the plugin not to create, and a stale one at that, since the authoritative pool lives in the keychain and the remaining JSON is a pre-migration leftover. The check moves into `snapshotCredentialStoreBeforeWrite`, beside the config gate, so every present and future caller inherits it instead of each one having to remember. The test-home guard was documented as the single error that must propagate, and `trySnapshotCredentialStoreBeforeWrite` re-throws it for that reason. `clearAccounts` then caught it: the assertion sits inside a `try` whose `catch` absorbs everything except ENOENT, so a run that escaped its sandbox got a warning and a *successful* return from a deletion the guard had refused to let happen. Fail-closed became fail-open on the path that deletes credentials. The catch now re-throws that code before the generic handling; every other failure keeps the best-effort contract, and the guard is inert outside vitest either way. The code is now a shared `TEST_HOME_ESCAPE_CODE` constant rather than a string literal repeated at three sites, since a typo in any re-throw would quietly restore the swallow. Four tests, each control-run against deliberately broken production code: clearing a keychain-backed store writes no snapshot (and the store really did hold a plaintext pool, so the assertion is not vacuous); a keychain write failure that falls back to JSON writes none either; a clear refused by the guard rejects rather than resolving, and creates nothing on disk; and a rotation snapshot holds the superseded token only for the account that write rotated, with every other account's live token intact. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- lib/storage/credential-snapshots.ts | 19 +++- lib/storage/load-save.ts | 40 +++++--- lib/storage/test-home-guard.ts | 11 ++- test/storage-credential-snapshots.test.ts | 112 +++++++++++++++++++++- 4 files changed, 165 insertions(+), 17 deletions(-) diff --git a/lib/storage/credential-snapshots.ts b/lib/storage/credential-snapshots.ts index 9f261d2b..c1233265 100644 --- a/lib/storage/credential-snapshots.ts +++ b/lib/storage/credential-snapshots.ts @@ -33,7 +33,11 @@ import { writeBackupFileContent, } from "./backup.js"; import { StorageError } from "./errors.js"; -import { assertTestRunNeverTouchesRealHome } from "./test-home-guard.js"; +import { isKeychainOptInEnabled } from "./keychain.js"; +import { + assertTestRunNeverTouchesRealHome, + TEST_HOME_ESCAPE_CODE, +} from "./test-home-guard.js"; import type { AccountStorageV3 } from "./migrations.js"; const log = createLogger("credential-snapshots"); @@ -253,6 +257,17 @@ export async function snapshotCredentialStoreBeforeWrite( const config = loadPluginConfig(); if (!getCredentialSnapshots(config)) return; + // Scoped to the JSON backend, enforced here rather than at each call site. + // Under the keychain opt-in the authoritative pool lives in the OS keychain, + // and whatever JSON remains at `storagePath` is a pre-migration or + // write-fallback artefact. Copying it into `backups/` would put the whole + // token set in a plaintext file that a user who opted into the keychain + // asked us not to create, and it would archive a document that is already + // stale. Every write path - ordinary save, the JSON fallback after a failed + // keychain write, and `clearAccounts` - goes through here, so one check + // covers all of them. + if (isKeychainOptInEnabled()) return; + const backupDirectory = getBackupDirectory(storagePath); assertTestRunNeverTouchesRealHome(backupDirectory); @@ -301,7 +316,7 @@ export async function trySnapshotCredentialStoreBeforeWrite( try { await snapshotCredentialStoreBeforeWrite(storagePath, next); } catch (error) { - if (error instanceof StorageError && error.code === "TEST_HOME_ESCAPE") throw error; + if (error instanceof StorageError && error.code === TEST_HOME_ESCAPE_CODE) throw error; log.warn("Credential snapshot failed; continuing with the write", { path: storagePath, error: String(error), diff --git a/lib/storage/load-save.ts b/lib/storage/load-save.ts index 3fedc278..d18271f9 100644 --- a/lib/storage/load-save.ts +++ b/lib/storage/load-save.ts @@ -26,7 +26,10 @@ import { renameWithWindowsRetry } from "./atomic-write.js"; import { formatStorageErrorHint, StorageError } from "./errors.js"; import { normalizeAccountStorage } from "./normalize.js"; import { getConfigDir } from "./paths.js"; -import { assertTestRunNeverTouchesRealHome } from "./test-home-guard.js"; +import { + assertTestRunNeverTouchesRealHome, + TEST_HOME_ESCAPE_CODE, +} from "./test-home-guard.js"; import { trySnapshotCredentialStoreBeforeWrite } from "./credential-snapshots.js"; import { getCurrentLegacyProjectStoragePath, @@ -685,14 +688,12 @@ async function saveAccountsUnlocked(storage: AccountStorageV3): Promise { await checkWorktreeLockForCurrentStorage("save"); if (isKeychainOptInEnabled()) { - // Credential snapshots are deliberately scoped to the JSON backend and do - // not cover this branch. Snapshotting here would mean writing the account - // pool, refresh tokens and all, into a plaintext file in `backups/` — the - // exact thing a user opting into the OS keychain asked us not to do. The - // on-disk JSON that remains is a rollback artefact, not the live store, so - // snapshotting it instead would archive a document that is already stale. - // Keychain users' recovery path stays `codex-export` plus the keychain's - // own backing store. + // Credential snapshots are scoped to the JSON backend and do not cover + // keychain mode. That is enforced inside the snapshotter itself rather + // than by the absence of a call here, so neither the JSON fallback below + // nor `clearAccounts` can reintroduce a plaintext copy of the token set - + // see `snapshotCredentialStoreBeforeWrite`. Keychain users' recovery path + // stays `codex-export` plus the keychain's own backing store. // // Normalize before serializing so the keychain receives the same shape // the JSON backend would have written. Using the same JSON format keeps @@ -775,6 +776,10 @@ export async function saveAccounts(storage: AccountStorageV3): Promise { * keychain delete and log at `error`. Both copies remain in sync so the * caller can retry safely. The operation is still best-effort (never * throws) to preserve the existing contract above the storage layer. + * + * @throws StorageError (code `TEST_HOME_ESCAPE`) - the single exception to + * best-effort, and inert outside vitest. The guard refuses the deletion, so + * absorbing it would return success for a clear that never happened. */ export async function clearAccounts(): Promise { return withStorageLock(async () => { @@ -782,12 +787,23 @@ export async function clearAccounts(): Promise { try { const path = getStoragePath(); assertTestRunNeverTouchesRealHome(path); - // Deleting the store outright is the most significant event there is, so - // this snapshot is unconditional; `null` says there is no successor - // document to compare against. + // Deleting the store outright needs no significance test - `null` says + // there is no successor document to compare against. The snapshotter + // still applies its own config and keychain gates. await trySnapshotCredentialStoreBeforeWrite(path, null); await fs.unlink(path); } catch (error) { + // The test-home guard is not a storage failure to absorb. It fires only + // under vitest, and it exists to fail a run that escaped its sandbox; it + // throws before the unlink, so swallowing it here would report a + // successful clear for a deletion that deliberately did not happen - + // fail-closed downgraded to fail-open on the one path that destroys the + // store. The same re-throw covers the snapshotter, which surfaces this + // code through `trySnapshotCredentialStoreBeforeWrite` for the same + // reason. + if (error instanceof StorageError && error.code === TEST_HOME_ESCAPE_CODE) { + throw error; + } const code = (error as NodeJS.ErrnoException).code; if (code !== "ENOENT") { jsonCleared = false; diff --git a/lib/storage/test-home-guard.ts b/lib/storage/test-home-guard.ts index 9379559d..b6c222d1 100644 --- a/lib/storage/test-home-guard.ts +++ b/lib/storage/test-home-guard.ts @@ -10,6 +10,15 @@ import os from "node:os"; import { StorageError } from "./errors.js"; import { isWithinDirectory } from "./paths.js"; +/** + * StorageError code for a write refused inside the developer's real home. + * + * Shared rather than spelled out at each site: several callers deliberately + * absorb storage failures and have to re-throw this one, and a typo in any of + * those copies would silently turn the guard back into a swallowed warning. + */ +export const TEST_HOME_ESCAPE_CODE = "TEST_HOME_ESCAPE"; + /** * Refuse to mutate account storage inside the developer's real home while the * test suite is running. @@ -34,7 +43,7 @@ export function assertTestRunNeverTouchesRealHome(path: string): void { throw new StorageError( `Refusing to write account storage inside the real home directory during a test run: ${path}`, - "TEST_HOME_ESCAPE", + TEST_HOME_ESCAPE_CODE, path, "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", ); diff --git a/test/storage-credential-snapshots.test.ts b/test/storage-credential-snapshots.test.ts index 683e906f..21f984b1 100644 --- a/test/storage-credential-snapshots.test.ts +++ b/test/storage-credential-snapshots.test.ts @@ -1,8 +1,14 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { promises as fs } from "node:fs"; -import { join } from "node:path"; +import { join, resolve } from "node:path"; import { mkdtemp } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { tmpdir, userInfo } from "node:os"; +import { + _resetBackendForTests, + _setBackendForTests, + type KeychainBackend, +} from "../lib/storage/keychain.js"; +import { TEST_HOME_ESCAPE_CODE } from "../lib/storage/test-home-guard.js"; import { clearAccounts, saveAccounts, @@ -418,4 +424,106 @@ describe("credential snapshots: safety", () => { expect(await exists(backupsDir)).toBe(false); }); + + it("propagates a clear refused by the test-home guard instead of reporting success", async () => { + // A path under the real home that does not exist and is not a storage + // location, so a regression in the guard still cannot unlink a real + // account store. The guard runs before any filesystem call, so nothing + // here is created either. + const escaped = resolve( + userInfo().homedir, + ".oc-codex-credential-snapshot-guard-probe", + "oc-codex-multi-auth-accounts.json", + ); + setStoragePathDirect(escaped); + + await expect(clearAccounts()).rejects.toMatchObject({ + code: TEST_HOME_ESCAPE_CODE, + }); + expect(await exists(escaped)).toBe(false); + }); +}); + +describe("credential snapshots: keychain opt-in", () => { + function createMockKeychain(): KeychainBackend & { failWrites: boolean } { + const store = new Map(); + const backend = { + failWrites: false, + async get(service: string, account: string) { + return store.get(`${service}::${account}`) ?? null; + }, + async set(service: string, account: string, secret: string) { + if (backend.failWrites) throw new Error("simulated keychain failure"); + store.set(`${service}::${account}`, secret); + }, + async delete(service: string, account: string) { + return store.delete(`${service}::${account}`); + }, + async isAvailable() { + return true; + }, + }; + return backend; + } + + afterEach(() => { + _resetBackendForTests(); + }); + + it("writes no snapshot when clearing a keychain-backed store", async () => { + await saveAccounts(makeStorage()); + const before = await fs.readFile(storagePath, "utf-8"); + vi.stubEnv("CODEX_KEYCHAIN", "1"); + _setBackendForTests(createMockKeychain()); + + await clearAccounts(); + + expect(await listSnapshotNames()).toEqual([]); + expect(await exists(storagePath)).toBe(false); + // The store really did hold a plaintext pool, so a snapshot here would + // have copied live tokens into backups/ rather than been a no-op. + expect(before).toContain("rt-1"); + }); + + it("writes no snapshot when a failed keychain write falls back to JSON", async () => { + const base = makeStorage(); + await saveAccounts(base); + vi.stubEnv("CODEX_KEYCHAIN", "1"); + const backend = createMockKeychain(); + backend.failWrites = true; + _setBackendForTests(backend); + + await saveAccounts(withAccount(base, 0, (a) => ({ ...a, refreshToken: "rt-rotated" }))); + + // The fallback wrote the pool to JSON, which is the path that snapshots. + expect((await readLiveStore()).accounts[0].refreshToken).toBe("rt-rotated"); + expect(await listSnapshotNames()).toEqual([]); + }); +}); + +describe("credential snapshots: token rotation semantics", () => { + it("keeps the superseded token for the refreshed account and live tokens for the rest", async () => { + const base = makeStorage(); + await saveAccounts(base); + + await saveAccounts( + withAccount(base, 0, (a) => ({ + ...a, + refreshToken: "rt-1-rotated", + accessToken: "at-1-rotated", + tokenRotatedAt: 1_800_000_000_000, + })), + ); + + const [snapshot] = await readSnapshotContents(); + const snapshotDoc = JSON.parse(snapshot) as AccountStorageV3; + // A refresh consumes one account's token, so that one account's + // snapshotted token is the superseded one... + expect(snapshotDoc.accounts[0].refreshToken).toBe("rt-1"); + // ...while every other account in the pool is snapshotted with the + // token that is still live on disk. That bounds the staleness of a + // snapshot to the accounts a single write actually rotated. + expect(snapshotDoc.accounts[1].refreshToken).toBe("rt-2"); + expect((await readLiveStore()).accounts[1].refreshToken).toBe("rt-2"); + }); }); From d72710f106795db418aa74cc002e633ebc55a3e0 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 13:37:21 -0500 Subject: [PATCH 11/11] docs(storage): bound what a credential snapshot can restore Both docs claimed the newest snapshot "holds refresh tokens that still work". Review pointed out that cannot be true of the account a write just refreshed: refresh tokens are single-use, so by the time the snapshot is taken the provider has already invalidated the token it preserves for that one account. The claim was overstated rather than wrong in kind, and the correction is the reason the feature snapshots on refreshes at all. A snapshot restores the pool as it stood an instant before one write. For the single account that write rotated, the restored refresh token is the consumed one and that account needs a fresh login; every other account in the pool comes back with the token that was live at that moment. That bound is the whole point - the incident this feature answers restored a 15-day-old backup in which every account's token had rotated away, and five of seven came back dead. Both files now state the bound instead of the guarantee. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- README.md | 2 +- docs/configuration.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f87e09e6..b011ff8f 100644 --- a/README.md +++ b/README.md @@ -458,7 +458,7 @@ By default, account pools are stored locally as V3 JSON files. File permissions Use JSON storage when you want predictable, inspectable local files and easy backup/export behavior. -Before the store is changed in a way that matters, the plugin copies the previous version of the file into `backups/` as `codex-credential-snapshot-*.json`, mode `0600` in a `0700` directory. The snapshot holds the state being replaced, not the state replacing it, which is what makes it useful if the file is ever overwritten wholesale. Token refreshes count as significant, so the newest snapshot holds refresh tokens that still work; a snapshot old enough to predate the last few refreshes restores accounts that can no longer authenticate. Rotation bookkeeping - `lastUsed`, rate-limit and cooldown state, quota stamps, and the rotation cursor - never triggers one on its own, so the kept snapshots are not churned away by ordinary traffic. The plugin keeps the 10 most recent and prunes strictly by that filename prefix, so nothing else in `backups/` is touched. Set `credentialSnapshots: false` to turn it off, or `credentialSnapshotsMaxCount` to keep a different number (`0` keeps all of them). +Before the store is changed in a way that matters, the plugin copies the previous version of the file into `backups/` as `codex-credential-snapshot-*.json`, mode `0600` in a `0700` directory. The snapshot holds the state being replaced, not the state replacing it, which is what makes it useful if the file is ever overwritten wholesale. Token refreshes count as significant, which bounds how stale a restore can be. Refresh tokens are single-use, so a snapshot taken just before a refresh holds the consumed token for the one account that refresh rotated - that account needs a fresh `opencode auth login` - while every other account in the pool comes back with the token that was live at that moment. A snapshot old enough to predate many refreshes restores a pool where most or all accounts can no longer authenticate, which is the failure this bounding exists to avoid. Rotation bookkeeping - `lastUsed`, rate-limit and cooldown state, quota stamps, and the rotation cursor - never triggers one on its own, so the kept snapshots are not churned away by ordinary traffic. The plugin keeps the 10 most recent and prunes strictly by that filename prefix, so nothing else in `backups/` is touched. Set `credentialSnapshots: false` to turn it off, or `credentialSnapshotsMaxCount` to keep a different number (`0` keeps all of them).
diff --git a/docs/configuration.md b/docs/configuration.md index 0c50c0ba..6bede8e3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -283,7 +283,7 @@ The sample above intentionally sets `"retryAllAccountsMaxRetries": 3` as a bound | `retryProfile` | `balanced` | retry budget profile for request classes (`conservative`, `balanced`, `aggressive`) | | `retryBudgetOverrides` | `{}` | optional per-class budget overrides (`authRefresh`, `network`, `server`, `rateLimitShort`, `rateLimitGlobal`, `emptyResponse`) | | `perProjectAccounts` | `true` | each project gets its own account storage | -| `credentialSnapshots` | `true` | before a significant change to the account store, copy the previous on-disk version into `backups/` so there is always a recent copy holding refresh tokens that still work. Snapshots are taken for account additions and removals, token refreshes, identity changes, label/tag/note/enabled changes, plan changes, schema-version changes, and deletion of the store. Rotation bookkeeping never triggers one on its own: `lastUsed`, `lastSwitchReason`, rate-limit and cooldown state, quota-exhaustion stamps, and the `activeIndex` / `activeIndexByFamily` rotation cursor. A snapshot failure is logged and never fails the write it precedes. Snapshots cover the default JSON backend only, not `CODEX_KEYCHAIN=1` | +| `credentialSnapshots` | `true` | before a significant change to the account store, copy the previous on-disk version into `backups/` so a clobbered store can be restored to a recent state. Refresh tokens are single-use: a snapshot taken just before a refresh holds the consumed token for the one account that refresh rotated, and the live token for every other account, so restoring costs at most a re-login for that one account rather than the whole pool. Snapshots are taken for account additions and removals, token refreshes, identity changes, label/tag/note/enabled changes, plan changes, schema-version changes, and deletion of the store. Rotation bookkeeping never triggers one on its own: `lastUsed`, `lastSwitchReason`, rate-limit and cooldown state, quota-exhaustion stamps, and the `activeIndex` / `activeIndexByFamily` rotation cursor. A snapshot failure is logged and never fails the write it precedes. Snapshots cover the default JSON backend only, not `CODEX_KEYCHAIN=1` | | `credentialSnapshotsMaxCount` | `10` | how many credential snapshots to keep. Pruning deletes strictly by the snapshot filename prefix, so other files in `backups/` are never touched. `0` means keep every snapshot; use `credentialSnapshots: false` to turn the feature off | | `autoUpdate` | `true` | check npm daily and clear the OpenCode-managed plugin cache on exit when a newer version is available; restart OpenCode to install it | | `toastDurationMs` | `5000` | how long toast notifications stay visible (ms) |