Conversation
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
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
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
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
`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
`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
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
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-<timestamp>-<nonce>.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
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
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Review limit reachedNext included review available in 15 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (22)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 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); |
There was a problem hiding this comment.
after a successful token exchange, the provider has already invalidated the old refresh token before this snapshot runs. the snapshot therefore preserves the consumed token rather than the newly issued live token. repeated refreshes can fill and eventually prune the ring until every retained snapshot contains dead credentials, defeating the recovery guarantee. the current vitest coverage directly saves changed data and does not cover the provider invalidation order.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage/load-save.ts
Line: 557-562
Comment:
**snapshots preserve dead tokens**
after a successful token exchange, the provider has already invalidated the old refresh token before this snapshot runs. the snapshot therefore preserves the consumed token rather than the newly issued live token. repeated refreshes can fill and eventually prune the ring until every retained snapshot contains dead credentials, defeating the recovery guarantee. the current vitest coverage directly saves changed data and does not cover the provider invalidation order.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
The observation is right; the conclusion that it defeats the recovery guarantee is not. I have corrected the documentation rather than the behaviour (d72710f).
The mechanism is real. Refresh tokens are single-use, so a snapshot taken immediately before a refresh preserves the token that refresh consumed. That is inherent to a point-in-time snapshot and cannot be fixed by reordering: snapshotting after the write would preserve the new token but defeat the feature's entire purpose, which is surviving a write that replaces the store with something wrong. A clobber snapshotted after the fact is just a snapshot of the clobber.
But the staleness is bounded to one account per write, not the whole ring. A refresh rotates the token of the account being refreshed. Every other account in the pool is snapshotted with the token that is live at that moment. So the newest snapshot restores a pool where at most one account needs a fresh opencode auth login, and the rest come back working. Repeated refreshes do not converge on "every retained snapshot contains dead credentials" — they converge on "each snapshot has one stale entry", and the newest snapshot is stale in exactly one account.
That is a test I was missing, and I have added it: after rotating account 0's credentials, the snapshot holds rt-1 (superseded) for account 0 and rt-2 (still live on disk) for account 1.
The alternative is the failure this feature exists to answer. The incident that motivated it: the account store was overwritten by a test run that had not isolated HOME, and the only copy available was 15 days old. Restoring it brought back 7 accounts, 5 of which had rotated their refresh tokens in the interim and returned invalid_refresh_token. One account needing re-login is the bounded case; seven accounts and a 15-day gap is the unbounded one. Snapshotting on refreshes is what keeps the gap small — that is why credential fields are deliberately absent from the ignore list.
On coverage. You are right that the suite drives saveAccounts directly rather than exercising a real token exchange, so it does not model provider-side invalidation ordering. It cannot: the exchange is network-side, and the storage layer's contract is "preserve the bytes that were on disk before this write", which is what is asserted. The new rotation-semantics test pins the property that actually matters here — which accounts in a snapshot are stale and which are not.
The docs previously claimed the newest snapshot "holds refresh tokens that still work". That was an overstatement, and your comment is what caught it. Both README.md and docs/configuration.md now state the bound instead of the guarantee, including that the rotated account needs a re-login.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds pre-write snapshots of the previous JSON credential store, retaining recent token-bearing copies for recovery after destructive or corrupting writes. The PR also adds configuration/documentation, test-home isolation, and related retry/live-reload changes included from the stacked base.
Changes:
- Snapshot significant storage changes atomically with restricted permissions and prefix-scoped retention.
- Add snapshot settings, tests, documentation, and a guard against writes into the real home during Vitest runs.
- Update retry-budget accounting and account-manager reload behavior, with corresponding tests.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.ts |
Configures a per-run isolated test home and longer test timeout. |
test/test-home-isolation.test.ts |
Tests home isolation and storage write protection. |
test/storage-credential-snapshots.test.ts |
Covers snapshot significance, retention, permissions, and failure behavior. |
test/retry-budget.test.ts |
Tests proportional retry wait-budget consumption. |
test/plugin-config.test.ts |
Updates expected configuration defaults. |
test/paths.test.ts |
Makes path-root tests independent of the real HOME and temp directory. |
test/login-runner.test.ts |
Excludes snapshot renames from account-file rename assertions. |
test/accounts-live-reload.test.ts |
Tests upstream quota re-probing and safer account reloads. |
lib/storage/test-home-guard.ts |
Provides the shared real-home write guard. |
lib/storage/paths.ts |
Exports directory-containment checking for the guard. |
lib/storage/load-save.ts |
Invokes snapshots before JSON writes and deletion. |
lib/storage/credential-snapshots.ts |
Implements significance comparison, snapshot creation, and retention. |
lib/storage/backup.ts |
Adds raw-content backup writing and explicit backup-directory/path helpers. |
lib/schemas.ts |
Adds credential snapshot configuration fields. |
lib/request/retry-budget.ts |
Adds time-proportional retry wait accounting. |
lib/config.ts |
Adds snapshot defaults and configuration resolvers. |
lib/AGENTS.md |
Documents the new storage modules. |
index.ts |
Adds safer empty reload handling and upstream quota re-probing. |
docs/development/CONFIG_FIELDS.md |
Documents snapshot defaults and bounds. |
docs/configuration.md |
Documents snapshot configuration, retention, and file locations. |
README.md |
Documents snapshot environment variables and behavior. |
AGENTS.md |
Adds credential snapshots to the storage map. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // 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); |
There was a problem hiding this comment.
Correct, and fixed in cc742c1.
The exclusion was only ever enforced by the absence of a snapshot call inside the keychain branch of saveAccountsUnlocked, which covers nothing outside that branch. Two paths escaped it:
clearAccounts, as you point out.- The JSON fallback after a failed keychain write, which drops through to
writeAccountsToPathUnlockedand snapshots there.
Both copied the whole pool, refresh tokens included, into a plaintext file in backups/.
The check now lives inside snapshotCredentialStoreBeforeWrite, next to the config gate, so it applies to every caller rather than to the ones that happen to sit in the right branch:
const config = loadPluginConfig();
if (!getCredentialSnapshots(config)) return;
if (isKeychainOptInEnabled()) return;clearAccounts still deletes the JSON artefact and the keychain entry in the documented order; only the snapshot is skipped.
Two tests, both control-run against deliberately broken code (gate removed → both fail):
- clearing a keychain-backed store writes no snapshot. The assertion is not vacuous — the test also asserts the JSON really did contain the plaintext token, so a regression would have something to leak.
- a keychain write failure that falls back to JSON writes no snapshot either, while still persisting the pool.
| try { | ||
| const path = getStoragePath(); | ||
| assertTestRunNeverTouchesRealHome(path); |
There was a problem hiding this comment.
Correct, and fixed in cc742c1.
The guard is documented as the one error that must propagate, and trySnapshotCredentialStoreBeforeWrite re-throws it for exactly that reason — but clearAccounts then caught it. A run that escaped its sandbox got a warning and a successful return from a deletion the guard had refused to allow: fail-closed downgraded to fail-open on the single path that destroys the store.
The catch now re-throws that code before the generic handling:
} catch (error) {
if (error instanceof StorageError && error.code === TEST_HOME_ESCAPE_CODE) {
throw error;
}
const code = (error as NodeJS.ErrnoException).code;
if (code !== "ENOENT") { ... }
}I kept the guard call inside the try rather than hoisting it out, because getStoragePath() on the line above can itself throw and that failure does belong to the best-effort contract. Re-throwing by code keeps both behaviours exact. Every other error still returns normally, and the guard remains inert outside vitest.
The literal is now a shared TEST_HOME_ESCAPE_CODE constant rather than a string repeated at three sites, since a typo in any re-throw would silently reinstate the swallow.
Covered by a test that points storage at a non-existent path under the real home and asserts clearAccounts() rejects with that code and creates nothing on disk. Control-run: restoring the old catch makes it fail.
|
|
||
| setStoragePath(null); | ||
| expect(isUnder(real, getStoragePath())).toBe(false); | ||
|
|
||
| setStoragePath(process.cwd()); |
There was a problem hiding this comment.
Two things here.
This file is not part of this PR. test/test-home-isolation.test.ts was introduced by #258 (854a25d), which this PR is stacked on; none of my four commits touch it. It shows in the file list only because the stack shows #258's commits until that PR merges. I have left it alone deliberately so #258 and this PR are not both editing it — its owner has the finding.
The stated premise does not hold, though. The assertion does not depend on the checkout being outside the real home, because the resolved storage path is never derived from process.cwd()'s location. setStoragePath(cwd) resolves the project root, then namespaces storage under getProjectGlobalConfigDir() — that is homedir()/.opencode/projects/<key> — specifically so account files are never written into user repositories. With HOME redirected, that is inside the sandbox no matter where the checkout sits.
This machine is already the layout you describe:
realHome : /home/nowaker
cwd : /home/nowaker/projekty/forks/oc-codex-multi-auth-cred-snapshots <-- under realHome
homedir() : /var/tmp/oc-snap-probe-gfg0tc <-- sandbox
cwd is under realUserHome(), and all 4 tests in the file pass. Same on a /home/runner/work/... layout, for the same reason.
The scenario that would break isolation is a different one: if os.tmpdir() itself resolved inside the real home, the sandbox home created by vitest.config.ts would land under realUserHome() and it is the line 27 assertion — isUnder(realUserHome(), homedir()) — that would fail, not this one. On GitHub-hosted Linux runners os.tmpdir() is /tmp, so that does not currently apply. Flagging it for #258's owner rather than acting on it here.
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
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
Why
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.
That is not hypothetical. On this machine,
~/.opencode/oc-codex-multi-auth-accounts.jsonwas recently overwritten by anpm testrun from a worktree that had not isolatedHOME: five real ChatGPT accounts replaced by two test fixtures. The only copy to recover from was 15 days old. It restored 7 accounts - and 5 of them came back with refresh tokens that had rotated in the meantime and no longer authenticated. Five accounts had to be re-authorized by hand.#258 fixes that particular cause. This PR is the safety net for the next one, whatever it turns out to be: if the store is ever clobbered again, there must be a recent snapshot holding tokens that still work.
Age is half the problem. Refresh tokens are single-use and rotate constantly, so a backup predating the last few refreshes restores a file whose credentials are already dead.
codex-exportproduces a backup on demand andimportAccountswrites one before it applies - a user who has never run either has nothing at all.What it does
Before a significant write, the document currently on disk is copied into the existing
backups/directory ascodex-credential-snapshot-<timestamp>-<nonce>.json.The snapshot is of the previous state, not the incoming one. It is taken before the replacement lands, inside the storage lock the write already holds. Snapshotting the new document would be useless for the case this exists for - a clobber would simply be recorded as a clobber. What is worth keeping is the last good state.
Significance is a denylist, not an allowlist. 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, which is recoverable. An allowlist fails the other way, and that failure is not.
Ignored, because it is scheduling churn that upstream re-derives on the next request and that holds nothing worth restoring:
lastUsed,lastSwitchReasonrateLimitResetTimes,coolingDownUntil,cooldownReasonquotaExhaustedUntil,quotaExhaustedStampAt,quotaExhaustedClearedAtactiveIndexandactiveIndexByFamilyThe rotation 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. In particular every token refresh produces a snapshot -
refreshToken,accessToken,expiresAtandtokenRotatedAtare all deliberately absent from the ignore list. That is what keeps the newest snapshot holding tokens that still authenticate, which is the difference between a recovery that works and the one described above. Account added or removed, identity changed (accountId,accountUserId,email,accountIdSource), user-set metadata (accountLabel,accountTags,accountNote,enabled),planType,oauthScope, and the storageversionall count too.Comparison runs against the normalized payload, through a canonical projection that sorts keys and drops
undefined-valued ones, so a difference that normalization orJSON.stringifyerases 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 copying.Retention
Keeps the newest
credentialSnapshotsMaxCount(default 10) and prunes strictly by the snapshot filename prefix.backups/is shared. It also holdscodex-pre-import-backup-*,codex-backup-*, the*.migrated-to-keychain.*artefacts thatfindMigrationBackupsreads, andpre-global-migration-*directories. Deleting any of those would be a data-loss bug inside a feature whose only purpose is preventing data loss, so retention never considers a file it did not write. A test seeds one of each kind and asserts they all survive a prune.0for the count means keep every snapshot. Disabling is the boolean's job - overloading the count with an off switch would make0the one value a user can set by accident that silently removes their safety net.Failure behaviour
clearAccountssnapshots unconditionally before unlinking. Deleting the store outright is the most significant event there is.0600into a0700directory through the shared backup writer (temp + rename, bounded write time), because they hold live refresh tokens.TEST_HOME_ESCAPEguard, which now also covers the snapshot path. That guard exists to stop a test run writing over real credentials, so swallowing it would disarm it.Keychain: out of scope, deliberately
When
CODEX_KEYCHAIN=1the keychain holds the authoritative blob and the on-disk JSON is only a post-migration rollback artefact. Snapshotting there would write the whole pool, refresh tokens and all, into a plaintext file inbackups/- the exact thing a user opting into the OS keychain asked the plugin not to do - and snapshotting the leftover JSON instead would archive a document that is already stale. The keychain branch insaveAccountsUnlockedcarries a comment saying so. Keychain users' recovery path remainscodex-exportplus the keychain's own backing store.Config
credentialSnapshotstrueCODEX_AUTH_CREDENTIAL_SNAPSHOTScredentialSnapshotsMaxCount10CODEX_AUTH_CREDENTIAL_SNAPSHOTS_MAX_COUNT0keeps allDocumented in
docs/configuration.md(options table, env table, file locations) anddocs/development/CONFIG_FIELDS.md(defaults table, numeric bounds), plus the README credential-storage section and the twoAGENTS.mdmaps.Tests
test/storage-credential-snapshots.test.ts, 35 tests. Full suite green: 3605 passed, 1 skipped, 143 files.npm run typecheckandnpm run lintboth clean.Covered: no snapshot on first write; no snapshot for a
lastUsed-only,lastSwitchReason-only, rate-limit-only, cooldown-only, quota-stamp-only,activeIndex-only oractiveIndexByFamily-only change (7 cases); a snapshot for each of account added, account removed, refresh-token rotation, access-token replacement, expiry move, label, tags, note, disable, email,accountUserId,accountIdSource, plan type and scope (14 cases); the snapshot holds the previous document and not the incoming one; an unparseable store is snapshotted; a schema-version change is significant; key order andundefinedvalues are not;clearAccountssnapshots before deleting and writes nothing when there is nothing to delete; retention keeps exactly N and deletes the oldest; foreign files inbackups/survive both retention paths;0keeps everything; the filename predicate rejects the other backup kinds; mode is0600in a0700directory; a snapshot write failure does not fail the save; the disable switch writes nothing at all.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 afterwards. 10 of 10 failed as required - including snapshotting the new content instead of the previous, un-ignoring
lastUsed, un-ignoringactiveIndex, ignoring the credential fields, pruning by directory instead of by prefix, skipping theclearAccountssnapshot, letting a snapshot failure propagate, ignoring the disable switch, writing0644, and keeping one too many.Base
This stacks on #258. It is branched off that PR's head rather than
main, for two reasons: #258 is what stops the test suite writing to the real~/.opencode/oc-codex-multi-auth-accounts.json(running the suite without it is what caused the incident above), and this feature reuses theassertTestRunNeverTouchesRealHomeguard #258 introduces, extending it to cover the snapshot path.It targets
mainon top of #258's base, so once #258 merges this diff narrows to just the four commits of this feature. Until then the file list shows #258's five commits as well. There are no fork-only commits in either.Commits
refactor(storage): let a backup writer take raw bytes and a named path-writeBackupFileContentso a caller preserving a file's existing bytes does not re-serialize (which would normalize away the corruption worth keeping) or hand-roll a second writer with weaker guarantees;createTimestampedBackupPathForfor a backup of a path other than the active one;getBackupDirectoryso the shared directory has a name.feat(config): add credential-snapshot settingsfeat(storage): snapshot the credential store before a significant write- the feature, the test suite, and theassertTestRunNeverTouchesRealHomeextraction into its own module (importing it fromload-save.tswould be a cycle, since that is what triggers snapshots). Behaviour of the guard is unchanged.docs(storage): document credential-store snapshotsnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.the pr is not yet safe to merge because the prior token-recovery finding remains outstanding.
Findings
Fix with agent prompt
Summary
this pr adds pre-write credential snapshots with significance filtering, bounded retention, restrictive permissions, and json/keychain safety boundaries. changes since the previous review also harden account reloads and long rate-limit waits.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[significant account-store mutation] --> B{keychain enabled?} B -- yes --> C[write authoritative keychain blob] C --> D{keychain write succeeded?} D -- yes --> E[preserve or refresh rollback artifact] D -- no --> F[fall back to json without plaintext snapshot] B -- no --> G[hold storage lock] G --> H[read current json bytes] H --> I{significant change or deletion?} I -- no --> J[write normalized json] I -- yes --> K[write 0600 previous-state snapshot] K --> L[prune owned prefix to retention limit] L --> JReviews (2) · Last reviewed commit: "docs(storage): bound what a credential s..."