From eaefe146725d7af8c618474885357630a953045c Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:33:35 +0200 Subject: [PATCH 1/4] fix(accounts): stop a load-time drop from deleting an account An OAuth account whose state entry lacks a usable refresh token was removed from the config file without any deletion code running. normalizeAccount returns null for such an entry, normalizeStorage filters it out silently, and mutateAccounts - re-reading fresh under its lock and behaving exactly as designed - then writes the roster it just loaded. The account is gone, no deletion path executed, and nothing was logged, so the loss leaves no trace to diagnose from. Reproduced end to end: three accounts in, two accounts out. mutateAccounts and saveAccounts now carry a dropped entry's raw config record through to the write verbatim, so the account survives on disk until something removes it deliberately. Refusing to write instead was the obvious shape and the wrong one: updateMainRefreshState persists the main account's refresh lease through mutateAccounts on this same file, so one broken fallback entry would have taken down main token refresh, and the remove and re-login paths that could repair the account run through it too - the operator would have been trapped by the fix. Deliberate removal of a dropped account is expressible through a new allowDrop option, which the two remove paths pass. They previously could not remove such an account at all: it is absent from the loaded roster, so the mutator reported it missing. normalizeStorage now warns with the dropped ids on every load, and a config write is logged with the resulting roster, because the absence of both is what made the original loss undiagnosable. Also: a refresh response that omits refresh_token no longer fails. The grant rotates the token single-use and an absent value means the current one stands, but it was treated as a malformed response, which would arm refresh backoff on every account at once the first time the server declined to rotate. Not observed in the wild - the contract is documented by an independent implementation of the same API. --- packages/opencode/src/cli.ts | 39 +- packages/opencode/src/commands.ts | 31 +- packages/opencode/src/core/accounts.ts | 231 ++++++++++- packages/opencode/src/core/provider.ts | 15 +- .../opencode/src/tests/accounts-store.test.ts | 366 +++++++++++++++++- packages/opencode/src/tests/commands.test.ts | 54 +++ .../opencode/src/tests/error-contract.test.ts | 54 +++ .../src/tests/provider-backoff.test.ts | 30 +- 8 files changed, 773 insertions(+), 47 deletions(-) diff --git a/packages/opencode/src/cli.ts b/packages/opencode/src/cli.ts index 8f06560..263a308 100644 --- a/packages/opencode/src/cli.ts +++ b/packages/opencode/src/cli.ts @@ -5,6 +5,7 @@ import { loadAccounts, mutateAccounts, type OAuthAccount, + readConfigRosterIds, } from './core/accounts' import { assertFallbackAccountIdAllowed, @@ -141,17 +142,33 @@ async function main() { process.exit(1) } - // Structural edit: route through mutateAccounts so the deletion is written - // authoritatively rather than union-merged back in by saveAccounts. - let found = false - await mutateAccounts((current) => { - const idx = current.accounts.findIndex((a) => a.id === targetId) - if (idx === -1) return current - found = true - current.accounts.splice(idx, 1) - return current - }) - if (!found) { + // A load-dropped entry sits in the raw config but is absent from the + // mutator's `current.accounts`; the mutator's splice would no-op and + // the load-time preservation pass would resurrect it. Pre-check the + // raw roster and pass `allowDrop` so the entry is gone end-to-end. + const configPath = getAccountStoragePath() + const rawRoster = await readConfigRosterIds(configPath) + const existedOnDisk = rawRoster ? rawRoster.has(targetId) : false + + let removed = false + await mutateAccounts( + (current) => { + const idx = current.accounts.findIndex((a) => a.id === targetId) + if (idx === -1) return current + current.accounts.splice(idx, 1) + removed = true + return current + }, + configPath, + existedOnDisk ? { allowDrop: [targetId] } : undefined, + ) + + // The mutator could not find the id in current.accounts because it was + // load-dropped; allowDrop prevented preservation, so the on-disk entry + // is gone — count it as a successful removal. + if (!removed && existedOnDisk) removed = true + + if (!removed) { console.error(`No account with id "${targetId}".`) process.exit(1) } diff --git a/packages/opencode/src/commands.ts b/packages/opencode/src/commands.ts index 29e19ed..7f00b17 100644 --- a/packages/opencode/src/commands.ts +++ b/packages/opencode/src/commands.ts @@ -7,6 +7,7 @@ import { mutateAccounts, type OAuthAccount, type RoutingMode, + readConfigRosterIds, } from './core/accounts' import type { FallbackAccount } from './core/accounts.ts' import type { CacheKeepManager, CacheKeepWindow } from './core/cachekeep' @@ -265,17 +266,33 @@ async function executeAccountCommand( if (tokens[0] === 'remove' && tokens[1]) { const targetId = tokens[1] + // A load-dropped entry sits in the raw config but is absent from the + // mutator's `current.accounts`; the mutator's splice would no-op and + // the load-time preservation pass would resurrect it. Pre-check the + // raw roster and pass `allowDrop` so the entry is gone end-to-end. + const rawRoster = await readConfigRosterIds(ctx.accountStoragePath) + const existedOnDisk = rawRoster ? rawRoster.has(targetId) : false + // Structural edit: route through mutateAccounts so the deletion is written // authoritatively. saveAccounts union-merges latest ∪ incoming by id, which // would resurrect the removed account from the on-disk `latest` set. let removed = false - const next = await mutateAccounts((current) => { - const idx = current.accounts.findIndex((a) => a.id === targetId) - if (idx === -1) return current - removed = true - current.accounts.splice(idx, 1) - return current - }, ctx.accountStoragePath) + const next = await mutateAccounts( + (current) => { + const idx = current.accounts.findIndex((a) => a.id === targetId) + if (idx === -1) return current + current.accounts.splice(idx, 1) + removed = true + return current + }, + ctx.accountStoragePath, + existedOnDisk ? { allowDrop: [targetId] } : undefined, + ) + + // The mutator could not find the id in current.accounts because it was + // load-dropped; allowDrop prevented preservation, so the on-disk entry + // is gone — count it as a successful removal. + if (!removed && existedOnDisk) removed = true if (!removed) { return { diff --git a/packages/opencode/src/core/accounts.ts b/packages/opencode/src/core/accounts.ts index 4e61e48..b468b5c 100644 --- a/packages/opencode/src/core/accounts.ts +++ b/packages/opencode/src/core/accounts.ts @@ -527,6 +527,42 @@ function normalizeResetState(value: unknown): ResetStateByAccount | undefined { function normalizeStorage(value: unknown): AccountStorage | null { if (!isRecord(value) || !Array.isArray(value.accounts)) return null + const inputAccounts = value.accounts + const normalizedAccounts = inputAccounts + .map(normalizeAccount) + .filter((account): account is FallbackAccount => account != null) + + // A silent drop here is what ate a real account: normalizeAccount rejects + // an oauth entry whose state-side refresh is missing, the next mutateAccounts + // call writes the filtered roster, and the account is gone with no log. Emit + // a WARN so any plain read path (loadAccounts) surfaces the same signal a + // guard on the mutator would. Compare against pre-normalize string-id + // records only, so an entry that survives normalization with a synthesized + // id is never flagged as a drop. + if (normalizedAccounts.length < inputAccounts.length) { + const inputIds = new Set() + for (const candidate of inputAccounts) { + if ( + isRecord(candidate) && + typeof candidate.id === 'string' && + candidate.id.trim() + ) { + inputIds.add(candidate.id.trim()) + } + } + const loadedIds = new Set(normalizedAccounts.map((account) => account.id)) + const dropped: string[] = [] + for (const id of inputIds) { + if (!loadedIds.has(id)) dropped.push(id) + } + if (dropped.length > 0) { + logA.warn( + 'account dropped during load (state entry missing or unusable)', + { droppedIds: dropped }, + ) + } + } + return { version: 1, main: { type: 'opencode', provider: 'openai' }, @@ -544,9 +580,7 @@ function normalizeStorage(value: unknown): AccountStorage | null { cachekeep: isRecord(value.cachekeep) ? value.cachekeep : undefined, mainAccountId: typeof value.mainAccountId === 'string' ? value.mainAccountId : undefined, - accounts: value.accounts - .map(normalizeAccount) - .filter((account): account is FallbackAccount => account != null), + accounts: normalizedAccounts, } } @@ -585,8 +619,12 @@ function objectWithDefinedEntries(value: Record) { * state. The config never holds secrets (see accountConfig), so reading it here * is safe. Reads are lock-free but the file is written atomically, so a * concurrent write is seen as either the complete old or complete new file. + * + * Trims and skips blank ids per the rule in collectConfigRosterIds above. */ -async function readConfigRosterIds(path: string): Promise | null> { +export async function readConfigRosterIds( + path: string, +): Promise | null> { let value: unknown try { value = (await readJsonIfPresent(path)).value @@ -596,9 +634,11 @@ async function readConfigRosterIds(path: string): Promise | null> { if (!isRecord(value) || !Array.isArray(value.accounts)) return null const ids = new Set() for (const account of value.accounts) { - if (isRecord(account) && typeof account.id === 'string') { - ids.add(account.id) - } + if (!isRecord(account)) continue + if (typeof account.id !== 'string') continue + const trimmed = account.id.trim() + if (!trimmed) continue + ids.add(trimmed) } return ids } @@ -970,7 +1010,42 @@ export async function saveAccounts( : null const merged = mergeStorageForSave(latest, storage) const existing = isRecord(configJson.value) ? configJson.value : {} - const nextConfig = { ...existing, ...configFromStorage(merged) } + + // Preserve load-dropped raw entries (parallel to mutateAccounts). This + // function is currently called only by tests, but it is exported and + // could be reached by an external consumer; the same roster invariant + // applies — a load-dropped id stays on disk verbatim until explicitly + // removed. + const rawIds = collectConfigRosterIds(configJson.value) + const latestAccountIds = new Set(merged.accounts.map((a) => a.id)) + const { preservedRawEntries, preservedIds } = + pickRawRosterEntriesForPreservation( + configJson.value, + rawIds ?? new Set(), + latestAccountIds, + new Set(), + ) + if (preservedIds.length > 0) { + logA.warn('account load-dropped, preserved on disk', { + preservedIds, + }) + } + + const baseConfig = configFromStorage(merged) + const additions = preservedRawEntries.filter((raw) => { + if (!isRecord(raw)) return false + if (typeof raw.id !== 'string') return false + return !latestAccountIds.has(raw.id.trim()) + }) + + const nextConfig = { + ...existing, + ...baseConfig, + accounts: [ + ...(Array.isArray(baseConfig.accounts) ? baseConfig.accounts : []), + ...additions, + ], + } await writeJsonAtomic(path, nextConfig) await writeJsonAtomic(statePath, stateFromStorage(merged)) } finally { @@ -981,6 +1056,78 @@ export async function saveAccounts( } } +/** + * Collects account ids from a parsed config value. Returns null when the + * value is not a record or has no accounts array — callers should treat null + * as "no roster to compare against" rather than an empty roster, since a + * missing array is structurally different from an empty one (it implies the + * user has never written a roster, not that they wrote an empty one). + * + * Roster rule (aligned with normalizeAccountBase in core/accounts.ts): an + * entry counts as a roster member only when its `id` is a string with at + * least one non-whitespace character. Non-record entries, entries whose id + * is not a string, entries with a non-string id (number, boolean, null), + * and entries with a blank/whitespace-only id are NOT in the roster. + * `normalizeAccountBase` would synthesize a `randomUUID()` for any of those + * cases — those synthesized ids are not authoritative and must never be + * treated as load-dropped. Storing the trimmed form (rather than the raw + * bytes) means downstream comparisons against `current.accounts.map(a=>a.id)` + * (whose ids are already trimmed by normalizeAccountBase) match. + */ +function collectConfigRosterIds(value: unknown): Set | null { + if (!isRecord(value) || !Array.isArray(value.accounts)) return null + const ids = new Set() + for (const account of value.accounts) { + if (!isRecord(account)) continue + if (typeof account.id !== 'string') continue + const trimmed = account.id.trim() + if (!trimmed) continue + ids.add(trimmed) + } + return ids +} + +/** + * Picks the raw entries that should be preserved on a config write because + * normalizeAccount rejected them. A load-dropped raw entry (its id is in + * `rawIds` but not in `currentAccountIds`) is preserved verbatim from + * `rawValue.accounts` so the next write cannot silently erase it. Ids in + * `allowDrop` are not preserved — the caller is deliberately removing them. + * The comparison is against `currentAccountIds` (the pre-mutator loaded + * roster) so a legitimate removal by a mutator is NOT preserved back. + * + * Returns the raw entries in raw-file order (the order they appear in + * `rawValue.accounts`); preserved entries are appended to the end of + * nextConfig.accounts after the normalized ones. + */ +function pickRawRosterEntriesForPreservation( + rawValue: unknown, + rawIds: Set, + currentAccountIds: Set, + allowDrop: Set, +): { + preservedRawEntries: Array> + preservedIds: string[] +} { + if (!isRecord(rawValue) || !Array.isArray(rawValue.accounts)) { + return { preservedRawEntries: [], preservedIds: [] } + } + const preservedRawEntries: Array> = [] + const preservedIds: string[] = [] + for (const id of rawIds) { + if (currentAccountIds.has(id)) continue + if (allowDrop.has(id)) continue + const rawEntry = rawValue.accounts.find( + (acc): acc is Record => + isRecord(acc) && typeof acc.id === 'string' && acc.id.trim() === id, + ) + if (!rawEntry) continue + preservedRawEntries.push(rawEntry) + preservedIds.push(id) + } + return { preservedRawEntries, preservedIds } +} + /** * Read-modify-write the account store atomically under the save lock. * @@ -996,10 +1143,26 @@ export async function saveAccounts( * The mutator may edit `current` in place and return it, or return a new * storage object. Returning undefined means "no change" and still rewrites the * freshly-read state (a harmless idempotent write). + * + * Load-time drop preservation: if normalizeAccount (called inside + * normalizeStorage) rejects an account whose id IS in the raw config roster, + * the previous behavior would erase that id silently on the next write. This + * function now carries the dropped raw entry through to the written config + * verbatim, so the on-disk state always matches the operator's intent (an + * account they added, even if temporarily un-loadable, stays in their list + * until they deliberately remove it). + * + * Removal seam: when the caller knows they are removing an id (e.g. the CLI + * `remove` command) and that id may be load-dropped — in which case the + * mutator cannot find it in `current.accounts` to splice it — the caller can + * pass `options.allowDrop: [id]`. Ids in `allowDrop` are NOT preserved; the + * mutator's splice still no-ops on a dropped id, but the absence of + * preservation completes the removal end-to-end. */ export async function mutateAccounts( mutate: (current: AccountStorage) => AccountStorage | undefined, path = getAccountStoragePath(), + options: { allowDrop?: readonly string[] } = {}, ): Promise { const statePath = getAccountStatePath(path) const lock = await acquireSaveAccountsLock(path) @@ -1014,11 +1177,61 @@ export async function mutateAccounts( mergeConfigAndState(configJson.value, stateJson.value), ) : null) ?? emptyAccountStorage() + + // Snapshot the pre-mutator account ids BEFORE running the mutator: + // the mutator may edit `current.accounts` in place, so reading + // current.accounts afterwards would observe the mutated set, not the + // loaded one — and a legitimate removal by the mutator would look + // identical to a load-time drop. + const currentAccountIds = new Set(current.accounts.map((a) => a.id)) const next = mutate(current) ?? current + + // Preserve load-dropped raw entries. The comparison uses + // `currentAccountIds` (pre-mutator) so a legitimate removal by the + // mutator is NOT preserved back onto disk — if it was in + // `current.accounts`, normalizeAccount accepted it, and there is no + // load-time drop to preserve. + const rawIds = collectConfigRosterIds(configJson.value) + const allowDrop = new Set(options.allowDrop ?? []) + const { preservedRawEntries, preservedIds } = + pickRawRosterEntriesForPreservation( + configJson.value, + rawIds ?? new Set(), + currentAccountIds, + allowDrop, + ) + if (preservedIds.length > 0) { + logA.warn('account load-dropped, preserved on disk', { + preservedIds, + }) + } + + const baseConfig = configFromStorage(next) + // Skip a raw entry whose id the mutator already added back, to avoid + // duplicating it on disk. (The mutator sees normalized FallbackAccounts; + // a raw entry has the same id since normalizeAccountBase trims.) + const nextAccountIds = new Set(next.accounts.map((a) => a.id)) + const additions = preservedRawEntries.filter((raw) => { + if (!isRecord(raw)) return false + if (typeof raw.id !== 'string') return false + return !nextAccountIds.has(raw.id.trim()) + }) + const existing = isRecord(configJson.value) ? configJson.value : {} - const nextConfig = { ...existing, ...configFromStorage(next) } + const nextConfig = { + ...existing, + ...baseConfig, + accounts: [ + ...(Array.isArray(baseConfig.accounts) ? baseConfig.accounts : []), + ...additions, + ], + } await writeJsonAtomic(path, nextConfig) await writeJsonAtomic(statePath, stateFromStorage(next)) + logA.debug('account config written', { + accountCount: next.accounts.length, + accountIds: next.accounts.map((a) => a.id), + }) return next } finally { await stateLock.release() diff --git a/packages/opencode/src/core/provider.ts b/packages/opencode/src/core/provider.ts index 3877b3e..6789044 100644 --- a/packages/opencode/src/core/provider.ts +++ b/packages/opencode/src/core/provider.ts @@ -67,7 +67,10 @@ export const CODEX_ISSUER = 'https://auth.openai.com' interface TokenResponse { id_token: string access_token: string - refresh_token: string + // OpenAI's refresh grant rotates the refresh token single-use; an absent + // refresh_token means "keep using the current one". Treat both missing + // and empty-string as the same signal rather than a malformed response. + refresh_token?: string expires_in?: number } @@ -118,8 +121,6 @@ export async function codexRefreshFn(input: { !tokens || typeof tokens.access_token !== 'string' || !tokens.access_token || - typeof tokens.refresh_token !== 'string' || - !tokens.refresh_token || typeof tokens.expires_in !== 'number' ) { throw Object.assign(new Error('Token refresh failed: malformed response'), { @@ -127,9 +128,15 @@ export async function codexRefreshFn(input: { isRefreshError: true, }) as ProviderHttpError } + // An omitted/empty refresh_token is not malformed: it means the server + // declined to rotate, and we keep the current one. + const rotatedRefresh = + typeof tokens.refresh_token === 'string' && tokens.refresh_token + ? tokens.refresh_token + : input.refreshToken return { access: tokens.access_token, - refresh: tokens.refresh_token, + refresh: rotatedRefresh, expires: input.now() + tokens.expires_in * 1000, expiresIn: tokens.expires_in, } diff --git a/packages/opencode/src/tests/accounts-store.test.ts b/packages/opencode/src/tests/accounts-store.test.ts index 6afcf39..420b67f 100644 --- a/packages/opencode/src/tests/accounts-store.test.ts +++ b/packages/opencode/src/tests/accounts-store.test.ts @@ -16,7 +16,11 @@ import type { OAuthAccount, } from '../core/accounts.ts' import { acquireRefreshFileLock } from '../core/refresh-file-lock.ts' -import { FLOOR_AUTH_FILE, FLOOR_STATE_FILE } from './setup-env.ts' +import { + FLOOR_AUTH_FILE, + FLOOR_LOG_FILE, + FLOOR_STATE_FILE, +} from './setup-env.ts' let dir: string let cfgPath: string @@ -1150,3 +1154,363 @@ describe('mutateAccounts (authoritative structural edits)', () => { expect(loaded?.accounts.map((a) => a.id).sort()).toEqual(['concurrent']) }) }) + +// --------------------------------------------------------------------------- +// Roster-drop preservation — a load-dropped entry (raw config roster has it, +// but normalizeAccount rejected the merged record so it is absent from +// current.accounts) is carried through to the written config verbatim. The +// alternative — refusing to write — breaks any code path that shares the +// writer with a load-dropped entry (e.g. updateMainRefreshState) and blocks +// the only paths that could repair the account (remove, re-add). Preserve +// is the right primitive: the dropped entry survives the write, the +// operator gets a WARN, and a deliberate removal uses the allowDrop option +// to override preservation for a single id. +// --------------------------------------------------------------------------- + +describe('mutateAccounts load-time roster preservation', () => { + // The mutator is free to write whatever it likes; preserve is a wrapper + // around the disk write that re-inserts raw entries whose ids normalize + // rejected, so they cannot be silently erased by the next config write. + function writeConfigWithMixedEntries(accounts: unknown[]) { + writeFileSync(cfgPath, `${JSON.stringify({ version: 1, accounts })}\n`) + writeFileSync( + statePath, + `${JSON.stringify({ version: 1, accounts: {} })}\n`, + ) + } + + // Anti-regression for the original incident: a load-dropped entry survives + // an unrelated mutateAccounts call. The mutator never even touches 'b'; + // preservation is what keeps it on disk. + it('PRESERVES a load-dropped entry after an unrelated mutateAccounts call (writes through)', async () => { + const { saveAccounts, mutateAccounts } = await import('../core/accounts.ts') + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('a'), oauthAccount('b')], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts.b + writeFileSync(statePath, JSON.stringify(stateObj)) + + // Mutator only touches the refresh metadata, never the accounts list — + // mirrors updateMainRefreshState's shape. + await mutateAccounts((current) => { + current.refresh = current.refresh ?? {} + current.refresh.intervalMinutes = 7 + return current + }, cfgPath) + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + // 'b' is still on disk, verbatim from the original raw entry. + expect(cfg.accounts.map((a: { id: string }) => a.id).sort()).toEqual([ + 'a', + 'b', + ]) + const preservedB = cfg.accounts.find((a: { id: string }) => a.id === 'b') + expect(preservedB).toBeDefined() + // Mutator's refresh change persisted too — preservation does not block + // legitimate mutator writes. + expect(cfg.refresh?.intervalMinutes).toBe(7) + }) + + // updateMainRefreshState goes through mutateAccounts on the same config + // path. A broken FALLBACK state entry (raw config has the id but state + // has no refresh for it) must not break MAIN refresh. This test pins + // that failure mode directly. + it('updateMainRefreshState-shaped mutation succeeds while a broken fallback exists (main refresh must not break)', async () => { + const { saveAccounts, mutateAccounts } = await import('../core/accounts.ts') + const main = oauthAccount('main') + const broken = oauthAccount('broken') + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [main, broken], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts.broken + writeFileSync(statePath, JSON.stringify(stateObj)) + + // Same shape as updateMainRefreshState in src/index.ts: touches only + // refresh/main lease metadata. Must not throw. + let resolved = false + let rejected: unknown + try { + await mutateAccounts((current) => { + current.refresh = current.refresh ?? {} + current.refresh.mainRefreshLeaseId = 'lease-1' + current.refresh.mainRefreshLeaseUntil = Date.now() + 60_000 + return current + }, cfgPath) + resolved = true + } catch (error) { + rejected = error + } + + expect(resolved).toBe(true) + expect(rejected).toBeUndefined() + + // Both accounts still on disk. + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id).sort()).toEqual([ + 'broken', + 'main', + ]) + // The mutator's refresh metadata went to the state file (where + // configFromStorage routes refresh lease fields) — the test only + // asserts the call did not throw, which is the MUST invariant. + expect(typeof cfg.refresh).toBe('object') + }) + + // Removal of a load-dropped account via allowDrop: the entry is in raw + // config (so the operator could see it via cli list, which reads raw), + // absent from current.accounts (load-dropped), and the allowDrop option + // suppresses preservation so it is gone from disk after the call. + it('removes a load-dropped account end to end when allowDrop is set', async () => { + const { saveAccounts, mutateAccounts } = await import('../core/accounts.ts') + const a = oauthAccount('a') + const broken = oauthAccount('broken') + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [a, broken], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts.broken + writeFileSync(statePath, JSON.stringify(stateObj)) + + // The mutator cannot find 'broken' in current.accounts (it was + // load-dropped). Without allowDrop, preserve would put it back. With + // allowDrop set, preservation is skipped and the entry is gone. + await mutateAccounts( + (current) => { + const idx = current.accounts.findIndex( + (candidate) => candidate.id === 'broken', + ) + if (idx !== -1) current.accounts.splice(idx, 1) + return current + }, + cfgPath, + { allowDrop: ['broken'] }, + ) + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id)).toEqual(['a']) + }) + + // A normal removal (target id is in current.accounts) still works + // without allowDrop. + it('allows a normal removal through the mutator', async () => { + const { loadAccounts, saveAccounts, mutateAccounts } = await import( + '../core/accounts.ts' + ) + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('a'), oauthAccount('b'), oauthAccount('c')], + }, + cfgPath, + ) + + await mutateAccounts((current) => { + current.accounts = current.accounts.filter( + (account) => account.id !== 'b', + ) + return current + }, cfgPath) + + const loaded = await loadAccounts(cfgPath) + expect(loaded?.accounts.map((a) => a.id)).toEqual(['a', 'c']) + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id)).toEqual(['a', 'c']) + }) + + // First-run with no config file: no raw roster to preserve from; the + // mutator runs as before. + it('does not throw on first run with no config file', async () => { + const { mutateAccounts } = await import('../core/accounts.ts') + expect(existsSync(cfgPath)).toBe(false) + expect(existsSync(statePath)).toBe(false) + + await expect( + mutateAccounts((current) => { + current.accounts.push(oauthAccount('first')) + return current + }, cfgPath), + ).resolves.toBeDefined() + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id)).toEqual(['first']) + }) + + // The roster predicate aligns with normalizeAccountBase: trim, skip + // blank/whitespace-only. A garbage entry that synthesize-a-uuid on load + // must not be treated as "dropped" and preserved spuriously. + it('blank and whitespace-padded raw ids do not trigger spurious preservation', async () => { + const { mutateAccounts } = await import('../core/accounts.ts') + writeConfigWithMixedEntries([ + { id: 'real-a', type: 'oauth', enabled: true }, + { id: ' ', type: 'oauth', enabled: true }, // whitespace only → not in roster + { id: '', type: 'oauth', enabled: true }, // empty → not in roster + { id: 7, type: 'oauth', enabled: true }, // non-string id → not in roster + { id: ' padded ', type: 'oauth', enabled: true }, // padded with content → trimmed to 'padded', in roster + {}, // no id → not in roster + null, // not a record → not in roster + 'string', // not a record → not in roster + ]) + + await mutateAccounts((current) => current, cfgPath) + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + const ids = cfg.accounts.map((a: { id: string }) => a.id) + // 'real-a' and ' padded ' are the only records whose id (after + // trim) was a non-empty string; both are load-dropped in this scenario + // (state is empty) so both are preserved verbatim. The blank / + // non-string / non-record entries were never in the roster. + expect(ids.sort()).toEqual([' padded ', 'real-a'].sort()) + }) + + // An api-type account rejected for a bad baseURL is also load-dropped, and + // the raw entry is preserved verbatim so re-add or re-login can fix it. + it('also preserves a load-dropped api-type account (verbatim from raw)', async () => { + const { mutateAccounts } = await import('../core/accounts.ts') + writeFileSync( + cfgPath, + `${JSON.stringify({ + version: 1, + accounts: [ + { id: 'good-api', type: 'api', baseURL: 'https://example.test' }, + { id: 'bad-api', type: 'api', baseURL: 'not-a-url' }, + ], + })}\n`, + ) + writeFileSync( + statePath, + `${JSON.stringify({ version: 1, accounts: {} })}\n`, + ) + + await mutateAccounts((current) => current, cfgPath) + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id).sort()).toEqual([ + 'bad-api', + 'good-api', + ]) + const preserved = cfg.accounts.find( + (a: { id: string }) => a.id === 'bad-api', + ) + // Verbatim: the bad baseURL is kept so the operator can repair it via + // re-add or fix the URL — the loader does not silently swallow it. + expect(preserved?.baseURL).toBe('not-a-url') + }) +}) + +// --------------------------------------------------------------------------- +// saveAccounts is exported but currently has no production callers. The same +// roster invariant applies (a load-dropped id stays on disk verbatim), so the +// preserve logic from mutateAccounts is mirrored here. This test pins that +// parallelism so a future regression to either writer is caught. +// --------------------------------------------------------------------------- + +describe('saveAccounts load-time roster preservation', () => { + it('preserves a load-dropped entry on a re-save even when the caller passes a storage without it (parallel to mutateAccounts)', async () => { + const { saveAccounts } = await import('../core/accounts.ts') + // Seed config + state for [a, broken]. + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('a'), oauthAccount('broken')], + }, + cfgPath, + ) + // Strip 'broken' from the state file so the next saveAccounts call + // re-reads a config where 'broken' is load-dropped — and the caller + // passes a storage that does NOT mention 'broken' (the typical + // caller-side view, since they cannot see load-dropped ids via + // loadAccounts either). + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts.broken + writeFileSync(statePath, JSON.stringify(stateObj)) + + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('a')], + }, + cfgPath, + ) + + // 'broken' must come back to disk verbatim because saveAccounts + // preserves load-dropped entries — the caller-side storage doesn't + // know about it, but it was in the original raw roster. + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id).sort()).toEqual([ + 'a', + 'broken', + ]) + }) +}) + +describe('normalizeStorage roster drop is loud on every load', () => { + // The same drop path is hit by plain loadAccounts — not just by mutations — + // and the previous version was silent, which is why the original incident + // had no log trail. The accounts-channel WARN names the dropped ids. + let logFile: string + + beforeEach(() => { + logFile = join(dir, 'drop.log') + process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = logFile + process.env.OPENCODE_OPENAI_AUTH_LOG_LEVEL = 'info' + process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = logFile + }) + afterEach(() => { + process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = FLOOR_LOG_FILE + delete process.env.OPENCODE_OPENAI_AUTH_LOG_LEVEL + }) + + it('emits a WARN naming the dropped ids when loadAccounts filters them out', async () => { + const { saveAccounts, loadAccounts } = await import('../core/accounts.ts') + const { flushForTest } = await import('../logger.ts') + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('keep-a'), oauthAccount('silent-drop')], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts['silent-drop'] + writeFileSync(statePath, JSON.stringify(stateObj)) + + const loaded = await loadAccounts(cfgPath) + await flushForTest() + // The dropped id must not have survived the load. + expect(loaded?.accounts.map((a) => a.id)).toEqual(['keep-a']) + + const logTxt = readFileSync(logFile, 'utf8') + // WARN on the accounts channel naming the dropped id. + expect(logTxt).toMatch(/WARN \[accounts\]/) + expect(logTxt).toContain('silent-drop') + // No token values are leaked through the WARN. + expect(logTxt).not.toContain('ref-silent-drop') + expect(logTxt).not.toContain('acc-silent-drop') + }) +}) diff --git a/packages/opencode/src/tests/commands.test.ts b/packages/opencode/src/tests/commands.test.ts index 390d93a..225dd2b 100644 --- a/packages/opencode/src/tests/commands.test.ts +++ b/packages/opencode/src/tests/commands.test.ts @@ -1294,6 +1294,60 @@ describe('commands', () => { expect(refreshCalls.length).toBe(1) }) + // Pins the allowDrop wiring at the command level. The mutateAccounts + // primitive is already unit-tested; this test drives the real + // openai-account remove path so a future refactor that drops allowDrop + // from the call site would leave a load-dropped account preserved on + // disk after the remove command — and this test catches it. + test('openai-account remove of a load-dropped account actually removes it from disk', async () => { + const healthy = makeAccount('healthy') + const broken = makeAccount('broken') + const qm = new QuotaManager({ + storage: { version: 1 as const, accounts: [healthy, broken] }, + }) + const ctx: CommandContext = { + accountStoragePath: configPath, + quotaManager: qm, + loadAccounts, + client: makeClient(), + } + + // Seed two oauth accounts, then strip 'broken' from the state file. + // On the next read the merge yields a record with no refresh, + // normalizeAccount rejects it, and 'broken' is load-dropped — the + // exact condition under which preserve would silently resurrect it + // without allowDrop. + await saveAccounts( + { + version: 1 as const, + main: { type: 'opencode', provider: 'openai' }, + accounts: [healthy, broken], + }, + configPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts.broken + writeFileSync(statePath, JSON.stringify(stateObj)) + + const payload = await buildDialogPayload( + 'openai-account', + 'remove broken', + ctx, + ) + + // The response must say the account was removed (not "Not Found" — + // which is what the mutator's missing-in-current.accounts check would + // report when allowDrop is missing). + expect(payload.text).toContain('Removed account `broken`') + expect(payload.text).not.toContain('Not Found') + + // And it must actually be gone from disk. This is the assertion that + // reddens when allowDrop is stripped from the call site. + const cfg = JSON.parse(readFileSync(configPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id)).toEqual(['healthy']) + }) + test('refreshSidebar called after order', async () => { const account = makeAccount('acct-1') const acct2 = makeAccount('acct-2') diff --git a/packages/opencode/src/tests/error-contract.test.ts b/packages/opencode/src/tests/error-contract.test.ts index 42da348..ef96dd0 100644 --- a/packages/opencode/src/tests/error-contract.test.ts +++ b/packages/opencode/src/tests/error-contract.test.ts @@ -64,6 +64,60 @@ describe('error contract', () => { } }) + // OpenAI's refresh grant rotates the refresh token single-use. When the + // server returns no refresh_token (or an empty one) on a successful + // exchange, the caller MUST keep using the current refresh token rather + // than throw a malformed-response error — otherwise refresh backoff would + // arm across every account on the first non-rotating response. + + it('reuses the input refresh token when the response omits refresh_token', async () => { + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers(), + json: async () => ({ + access_token: 'rotated-access', + // refresh_token intentionally absent + expires_in: 3600, + }), + }) + + const result = await codexRefreshFn({ + refreshToken: 'keep-this-refresh', + fetchImpl: mockFetch as unknown as typeof fetch, + now: () => 1_700_000_000_000, + }) + + // The refresh token in the result is the INPUT token, unchanged. + expect(result.refresh).toBe('keep-this-refresh') + expect(result.access).toBe('rotated-access') + expect(result.expiresIn).toBe(3600) + }) + + it('still throws when access_token is missing on a 200 response', async () => { + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers(), + json: async () => ({ + // access_token intentionally absent + refresh_token: 'irrelevant', + expires_in: 3600, + }), + }) + + try { + await codexRefreshFn({ + refreshToken: 'test-refresh', + fetchImpl: mockFetch as unknown as typeof fetch, + now: () => Date.now(), + }) + expect.unreachable('should have thrown') + } catch (error) { + expect((error as Error).message.toLowerCase()).toContain('malformed') + } + }) + // ------------------------------------------------------------------- // isTransientRefreshError duck-types .status // ------------------------------------------------------------------- diff --git a/packages/opencode/src/tests/provider-backoff.test.ts b/packages/opencode/src/tests/provider-backoff.test.ts index e918bea..6ea0e97 100644 --- a/packages/opencode/src/tests/provider-backoff.test.ts +++ b/packages/opencode/src/tests/provider-backoff.test.ts @@ -110,7 +110,12 @@ describe('codexRefreshFn token validation', () => { expect(thrown?.isRefreshError).toBe(true) }) - it('throws structured refresh error when refresh_token is missing', async () => { + it('keeps the input refresh token when the response omits refresh_token', async () => { + // OpenAI's refresh grant rotates the refresh token single-use. An absent + // refresh_token on a successful exchange therefore means "keep using + // the current refresh token" — throwing 'malformed response' here would + // arm refresh backoff across every account on the first non-rotating + // response. const mockFetch = mock(async () => { return new Response( JSON.stringify({ @@ -121,21 +126,16 @@ describe('codexRefreshFn token validation', () => { ) }) - let thrown: ProviderHttpError | undefined - try { - await codexRefreshFn({ - refreshToken: 'some-refresh', - fetchImpl: mockFetch as unknown as typeof fetch, - now: mockNow, - }) - } catch (e) { - thrown = e as ProviderHttpError - } + const result = await codexRefreshFn({ + refreshToken: 'kept-input-refresh', + fetchImpl: mockFetch as unknown as typeof fetch, + now: mockNow, + }) - expect(thrown).toBeDefined() - expect(thrown?.message).toContain('malformed response') - expect(thrown?.status).toBe(200) - expect(thrown?.isRefreshError).toBe(true) + expect(result.access).toBe('valid-access') + expect(result.refresh).toBe('kept-input-refresh') + expect(result.expiresIn).toBe(3600) + expect(result.expires).toBe(mockNow() + 3600 * 1000) }) it('throws structured refresh error when expires_in is missing or not a number', async () => { From 563492c4cffcc11e3962cf88f1e5dfd9415e4cb3 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 15 Aug 2026 16:35:39 +0200 Subject: [PATCH 2/4] fix(accounts): tighten the load-drop preserve path Five follow-ups from review. A refresh response carrying a defined non-string refresh_token was treated as if the field were absent and reported as a successful refresh. Absent means the server declined to rotate and the current token stands; a non-string is malformed wire data, and now throws. The remove paths read the raw roster outside mutateAccounts' lock to decide whether to pass allowDrop, so a concurrent add could leave it off and preserve the entry instead of removing it. allowDrop is now passed unconditionally, which is a no-op for a healthy account because preservation checks the loaded roster before it consults allowDrop. The pre-read survives only to answer whether the id was on disk, OR'd with whether the mutator spliced it, so the reported outcome stays honest: removing an id that never existed reports Not Found rather than claiming a removal. That second arm covers a concurrent add racing the pre-read and is not pinned by a test - reaching it needs a race the suite cannot deterministically produce. The preserve pipeline was duplicated across mutateAccounts and saveAccounts and had already drifted apart in one place, so it is now one helper both call. Extracting it surfaced that each writer was also emitting the dropped-roster warning independently. The config-write log reported the mutator's account list, which excludes the preserved entries, so the record added to make this failure diagnosable described a roster that differed from the file. It now logs what was written. The dropped-roster warning fired on every load and every write. Since a preserved entry persists until something removes it deliberately, that condition never clears and the warning repeated indefinitely. It is now deduped on the set of dropped ids, so a newly dropped account still warns. Also documents why a preserved entry keeps its credentials: state is spread over config on load, so a config-inline refresh token is load-bearing, and stripping it would turn a recoverable account into a dead one. --- packages/opencode/src/cli.ts | 26 +-- packages/opencode/src/commands.ts | 36 ++-- packages/opencode/src/core/accounts.ts | 183 ++++++++++++------ packages/opencode/src/core/provider.ts | 16 +- .../opencode/src/tests/accounts-store.test.ts | 136 +++++++++++++ packages/opencode/src/tests/commands.test.ts | 41 ++++ .../opencode/src/tests/error-contract.test.ts | 63 ++++++ 7 files changed, 412 insertions(+), 89 deletions(-) diff --git a/packages/opencode/src/cli.ts b/packages/opencode/src/cli.ts index 263a308..983e443 100644 --- a/packages/opencode/src/cli.ts +++ b/packages/opencode/src/cli.ts @@ -142,32 +142,32 @@ async function main() { process.exit(1) } - // A load-dropped entry sits in the raw config but is absent from the - // mutator's `current.accounts`; the mutator's splice would no-op and - // the load-time preservation pass would resurrect it. Pre-check the - // raw roster and pass `allowDrop` so the entry is gone end-to-end. + // `allowDrop` is unconditional — for a healthy entry it is a no-op, + // for a load-dropped entry it suppresses the preservation pass that + // would otherwise resurrect the raw entry. The user-facing message + // comes from two signals OR'd together: the mutator's splice and a + // pre-read of the raw roster that the mutator's current.accounts + // cannot see. The pre-read is purely diagnostic — a stale read can + // only change the message when another writer races us, and the + // mutator signal covers exactly that case. const configPath = getAccountStoragePath() const rawRoster = await readConfigRosterIds(configPath) - const existedOnDisk = rawRoster ? rawRoster.has(targetId) : false + const preReadSawIt = rawRoster ? rawRoster.has(targetId) : false - let removed = false + let mutatorSplicedIt = false await mutateAccounts( (current) => { const idx = current.accounts.findIndex((a) => a.id === targetId) if (idx === -1) return current current.accounts.splice(idx, 1) - removed = true + mutatorSplicedIt = true return current }, configPath, - existedOnDisk ? { allowDrop: [targetId] } : undefined, + { allowDrop: [targetId] }, ) - // The mutator could not find the id in current.accounts because it was - // load-dropped; allowDrop prevented preservation, so the on-disk entry - // is gone — count it as a successful removal. - if (!removed && existedOnDisk) removed = true - + const removed = mutatorSplicedIt || preReadSawIt if (!removed) { console.error(`No account with id "${targetId}".`) process.exit(1) diff --git a/packages/opencode/src/commands.ts b/packages/opencode/src/commands.ts index 7f00b17..c08b20b 100644 --- a/packages/opencode/src/commands.ts +++ b/packages/opencode/src/commands.ts @@ -266,33 +266,41 @@ async function executeAccountCommand( if (tokens[0] === 'remove' && tokens[1]) { const targetId = tokens[1] - // A load-dropped entry sits in the raw config but is absent from the - // mutator's `current.accounts`; the mutator's splice would no-op and - // the load-time preservation pass would resurrect it. Pre-check the - // raw roster and pass `allowDrop` so the entry is gone end-to-end. - const rawRoster = await readConfigRosterIds(ctx.accountStoragePath) - const existedOnDisk = rawRoster ? rawRoster.has(targetId) : false - // Structural edit: route through mutateAccounts so the deletion is written // authoritatively. saveAccounts union-merges latest ∪ incoming by id, which // would resurrect the removed account from the on-disk `latest` set. - let removed = false + // + // `allowDrop` is unconditional for the target id. For a healthy entry it + // is a no-op (the mutator splices, preservation already wouldn't fire for + // a loaded id). For a load-dropped entry the mutator's splice no-ops, + // but preservation would resurrect the raw entry — allowDrop suppresses + // it. Behaviour (disk state) is therefore race-free inside the lock. + // + // The user-facing message comes from two signals OR'd together: + // - the mutator's splice (authoritative for healthy ids) + // - a pre-read of the raw roster that the mutator's current.accounts + // cannot see (load-dropped ids, which normalize rejected). + // The pre-read is purely diagnostic — its staleness can only change the + // message when another writer races us between read and lock, and the + // mutator signal covers exactly that case. It is NOT load-bearing for + // disk behaviour; that is `allowDrop`'s job now. + const rawRoster = await readConfigRosterIds(ctx.accountStoragePath) + const preReadSawIt = rawRoster ? rawRoster.has(targetId) : false + + let mutatorSplicedIt = false const next = await mutateAccounts( (current) => { const idx = current.accounts.findIndex((a) => a.id === targetId) if (idx === -1) return current current.accounts.splice(idx, 1) - removed = true + mutatorSplicedIt = true return current }, ctx.accountStoragePath, - existedOnDisk ? { allowDrop: [targetId] } : undefined, + { allowDrop: [targetId] }, ) - // The mutator could not find the id in current.accounts because it was - // load-dropped; allowDrop prevented preservation, so the on-disk entry - // is gone — count it as a successful removal. - if (!removed && existedOnDisk) removed = true + const removed = mutatorSplicedIt || preReadSawIt if (!removed) { return { diff --git a/packages/opencode/src/core/accounts.ts b/packages/opencode/src/core/accounts.ts index b468b5c..78017a9 100644 --- a/packages/opencode/src/core/accounts.ts +++ b/packages/opencode/src/core/accounts.ts @@ -525,6 +525,32 @@ function normalizeResetState(value: unknown): ResetStateByAccount | undefined { return Object.keys(normalized).length > 0 ? normalized : undefined } +// Module-level dedup of the load-dropped-entries WARN. The preserve +// invariant keeps broken entries on disk indefinitely — every +// loadAccounts, every main-refresh tick — so logging the same drop set on +// every call is spam by construction. The first occurrence stays loud; +// identical repeats are suppressed; a change in the drop set (any new +// id appearing, an id being fixed and gone) yields a new key and re-warns. +// +// Dedup key: sorted, comma-joined ids. The set grows monotonically within +// the process; there is no reset. If the same logical drop recurs after +// a process restart it will re-warn — which is the desired behaviour +// because the operator opens a fresh log file on restart anyway. +// +// Keyed on the SORTED form so that id-ordering differences (e.g. drop +// set [a,b] vs [b,a] in different writes) deduplicate as the same event. +const warnedRosterDrops = new Set() + +function emitRosterDropWarning(droppedIds: readonly string[]): void { + if (droppedIds.length === 0) return + const key = [...droppedIds].sort().join(',') + if (warnedRosterDrops.has(key)) return + warnedRosterDrops.add(key) + logA.warn('account load-dropped, preserved on disk', { + droppedIds, + }) +} + function normalizeStorage(value: unknown): AccountStorage | null { if (!isRecord(value) || !Array.isArray(value.accounts)) return null const inputAccounts = value.accounts @@ -555,12 +581,7 @@ function normalizeStorage(value: unknown): AccountStorage | null { for (const id of inputIds) { if (!loadedIds.has(id)) dropped.push(id) } - if (dropped.length > 0) { - logA.warn( - 'account dropped during load (state entry missing or unusable)', - { droppedIds: dropped }, - ) - } + emitRosterDropWarning(dropped) } return { @@ -1011,33 +1032,18 @@ export async function saveAccounts( const merged = mergeStorageForSave(latest, storage) const existing = isRecord(configJson.value) ? configJson.value : {} - // Preserve load-dropped raw entries (parallel to mutateAccounts). This - // function is currently called only by tests, but it is exported and - // could be reached by an external consumer; the same roster invariant - // applies — a load-dropped id stays on disk verbatim until explicitly - // removed. - const rawIds = collectConfigRosterIds(configJson.value) + // Preserve load-dropped raw entries — shared pipeline with mutateAccounts + // (no allowDrop seam here; this writer has no caller-driven removal + // intent). The WARN and dedup live on the shared helper, so both + // writers stay in sync. const latestAccountIds = new Set(merged.accounts.map((a) => a.id)) - const { preservedRawEntries, preservedIds } = - pickRawRosterEntriesForPreservation( - configJson.value, - rawIds ?? new Set(), - latestAccountIds, - new Set(), - ) - if (preservedIds.length > 0) { - logA.warn('account load-dropped, preserved on disk', { - preservedIds, - }) - } + const additions = buildPreservedAdditions( + configJson.value, + latestAccountIds, + new Set(), + ) const baseConfig = configFromStorage(merged) - const additions = preservedRawEntries.filter((raw) => { - if (!isRecord(raw)) return false - if (typeof raw.id !== 'string') return false - return !latestAccountIds.has(raw.id.trim()) - }) - const nextConfig = { ...existing, ...baseConfig, @@ -1099,6 +1105,14 @@ function collectConfigRosterIds(value: unknown): Set | null { * Returns the raw entries in raw-file order (the order they appear in * `rawValue.accounts`); preserved entries are appended to the end of * nextConfig.accounts after the normalized ones. + * + * Credentials (e.g. a config-inline `refresh`) on a preserved raw entry + * are NOT stripped before the write. Why: mergeConfigAndState spreads the + * state entry over the config entry, so a refresh living only in the + * config file is load-bearing — an account whose only token copy sits in + * the config loads fine today. Stripping credentials here would convert a + * recoverable account into a permanently dead one — the same argument that + * makes preserve beat refuse-to-write, one layer down. */ function pickRawRosterEntriesForPreservation( rawValue: unknown, @@ -1128,6 +1142,64 @@ function pickRawRosterEntriesForPreservation( return { preservedRawEntries, preservedIds } } +/** + * Shared roster-preservation pipeline used by both `mutateAccounts` and + * `saveAccounts`. Returns the raw entries that should be appended to the + * writer's outgoing accounts list so a load-dropped entry survives the + * write. The WARN is emitted here (dedup'd by emitRosterDropWarning) so + * both writers stay in sync — the alternative (each writer owning its + * own copy of the iteration logic) is drift-prone by construction: the + * next invariant change would land on one writer only. + * + * `loadedIds` is the writer's outgoing account-id set: in mutateAccounts + * it is the pre-mutator loaded roster; in saveAccounts it is the merged + * candidate set. Either side of that distinction gets the same + * preservation behaviour. + * + * Any id already in `loadedIds` is filtered out of `additions` so the + * writer does not duplicate a raw entry the caller/merger already + * accounted for. + */ +/** + * Walks a list of mixed-shape config entries (some are normalized account + * configs, some are raw preserved entries passed through verbatim) and + * collects the string ids. Used for the write-debug log so the + * diagnostic surface reflects what landed on disk rather than what the + * mutator returned. + */ +function collectStringIds(entries: unknown): string[] { + if (!Array.isArray(entries)) return [] + const ids: string[] = [] + for (const entry of entries) { + if (!isRecord(entry)) continue + if (typeof entry.id !== 'string') continue + ids.push(entry.id) + } + return ids +} + +function buildPreservedAdditions( + rawConfigValue: unknown, + loadedIds: Set, + allowDrop: Set, +): Array> { + const rawIds = collectConfigRosterIds(rawConfigValue) + if (rawIds === null) return [] + const { preservedRawEntries, preservedIds } = + pickRawRosterEntriesForPreservation( + rawConfigValue, + rawIds, + loadedIds, + allowDrop, + ) + emitRosterDropWarning(preservedIds) + return preservedRawEntries.filter((raw) => { + if (!isRecord(raw)) return false + if (typeof raw.id !== 'string') return false + return !loadedIds.has(raw.id.trim()) + }) +} + /** * Read-modify-write the account store atomically under the save lock. * @@ -1178,6 +1250,11 @@ export async function mutateAccounts( ) : null) ?? emptyAccountStorage() + // Snapshot the pre-mutator account ids BEFORE running the mutator: + // the mutator may edit `current.accounts` in place, so reading + // current.accounts afterwards would observe the mutated set, not the + // loaded one — and a legitimate removal by the mutator would look + // identical to a load-time drop. // Snapshot the pre-mutator account ids BEFORE running the mutator: // the mutator may edit `current.accounts` in place, so reading // current.accounts afterwards would observe the mutated set, not the @@ -1186,37 +1263,19 @@ export async function mutateAccounts( const currentAccountIds = new Set(current.accounts.map((a) => a.id)) const next = mutate(current) ?? current - // Preserve load-dropped raw entries. The comparison uses - // `currentAccountIds` (pre-mutator) so a legitimate removal by the - // mutator is NOT preserved back onto disk — if it was in - // `current.accounts`, normalizeAccount accepted it, and there is no - // load-time drop to preserve. - const rawIds = collectConfigRosterIds(configJson.value) + // Preserve load-dropped raw entries via the shared pipeline. The + // comparison is against `currentAccountIds` (pre-mutator) so a + // legitimate removal by the mutator is NOT preserved back onto disk + // — if it was in current.accounts, normalizeAccount accepted it and + // there is no load-time drop to preserve. const allowDrop = new Set(options.allowDrop ?? []) - const { preservedRawEntries, preservedIds } = - pickRawRosterEntriesForPreservation( - configJson.value, - rawIds ?? new Set(), - currentAccountIds, - allowDrop, - ) - if (preservedIds.length > 0) { - logA.warn('account load-dropped, preserved on disk', { - preservedIds, - }) - } + const additions = buildPreservedAdditions( + configJson.value, + currentAccountIds, + allowDrop, + ) const baseConfig = configFromStorage(next) - // Skip a raw entry whose id the mutator already added back, to avoid - // duplicating it on disk. (The mutator sees normalized FallbackAccounts; - // a raw entry has the same id since normalizeAccountBase trims.) - const nextAccountIds = new Set(next.accounts.map((a) => a.id)) - const additions = preservedRawEntries.filter((raw) => { - if (!isRecord(raw)) return false - if (typeof raw.id !== 'string') return false - return !nextAccountIds.has(raw.id.trim()) - }) - const existing = isRecord(configJson.value) ? configJson.value : {} const nextConfig = { ...existing, @@ -1228,9 +1287,13 @@ export async function mutateAccounts( } await writeJsonAtomic(path, nextConfig) await writeJsonAtomic(statePath, stateFromStorage(next)) + // Log the actual written roster (nextConfig.accounts), not the + // mutator's output (next.accounts). next.accounts omits preserved + // entries — which this log was created specifically to surface — so + // logging it here would defeat the post-incident forensic use. logA.debug('account config written', { - accountCount: next.accounts.length, - accountIds: next.accounts.map((a) => a.id), + accountCount: nextConfig.accounts.length, + accountIds: collectStringIds(nextConfig.accounts), }) return next } finally { diff --git a/packages/opencode/src/core/provider.ts b/packages/opencode/src/core/provider.ts index 6789044..d13e106 100644 --- a/packages/opencode/src/core/provider.ts +++ b/packages/opencode/src/core/provider.ts @@ -128,8 +128,20 @@ export async function codexRefreshFn(input: { isRefreshError: true, }) as ProviderHttpError } - // An omitted/empty refresh_token is not malformed: it means the server - // declined to rotate, and we keep the current one. + // A missing or empty-string refresh_token legitimately means "no rotation" + // (the server declined to rotate; keep the current one). A *defined* non- + // string value (number, boolean, null, object) is a genuinely malformed + // response — swallow-and-reuse here would turn a wire-shape regression + // into a successful refresh report. + if ( + tokens.refresh_token !== undefined && + typeof tokens.refresh_token !== 'string' + ) { + throw Object.assign(new Error('Token refresh failed: malformed response'), { + status: response.status, + isRefreshError: true, + }) as ProviderHttpError + } const rotatedRefresh = typeof tokens.refresh_token === 'string' && tokens.refresh_token ? tokens.refresh_token diff --git a/packages/opencode/src/tests/accounts-store.test.ts b/packages/opencode/src/tests/accounts-store.test.ts index 420b67f..6474795 100644 --- a/packages/opencode/src/tests/accounts-store.test.ts +++ b/packages/opencode/src/tests/accounts-store.test.ts @@ -1513,4 +1513,140 @@ describe('normalizeStorage roster drop is loud on every load', () => { expect(logTxt).not.toContain('ref-silent-drop') expect(logTxt).not.toContain('acc-silent-drop') }) + + // The write-debug log exists specifically so the post-incident forensic + // trail reflects what landed on disk — not what the mutator returned. + // Preserved entries are appended to nextConfig.accounts after the + // mutator runs, so logging next.accounts would under-report. + it('write-debug log lists the actual written roster (preserved entry included)', async () => { + const { saveAccounts, mutateAccounts } = await import('../core/accounts.ts') + const { flushForTest } = await import('../logger.ts') + process.env.OPENCODE_OPENAI_AUTH_LOG_LEVEL = 'debug' + // Use a UUID so the dropped-id dedup state does not collide with + // any other test in this process. + const preservedId = `preserve-${randomUUID()}` + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('healthy'), oauthAccount(preservedId)], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts[preservedId] + writeFileSync(statePath, JSON.stringify(stateObj)) + + // Mutator touches only refresh metadata — mirrors updateMainRefreshState. + await mutateAccounts((current) => { + current.refresh = current.refresh ?? {} + current.refresh.intervalMinutes = 11 + return current + }, cfgPath) + await flushForTest() + + const logTxt = readFileSync(logFile, 'utf8') + // Pull only the DEBUG line for the config write — the WARN line also + // contains the preserved id (as preservedIds), so a flat toContain() + // would be ambiguous between DEBUG and WARN. + const debugLine = logTxt + .split('\n') + .find((line) => line.includes('account config written')) + expect(debugLine).toBeDefined() + // The DEBUG payload must list BOTH the healthy entry and the preserved + // entry — exactly what landed on disk. This is the assertion that + // reddens when the log uses next.accounts (only the healthy entry). + expect(debugLine).toContain(preservedId) + expect(debugLine).toContain('healthy') + }) +}) + +// The preserve invariant keeps a load-dropped entry on disk indefinitely. +// That guarantees the WARN condition persists forever — every main-refresh +// tick, every loadAccounts — which the original fix would spam. The dedup +// keeps the first occurrence loud and suppresses identical repeats; a +// change in the dropped-id set (any new id appearing) still re-warns. Each +// test uses UUID ids so dedup state does not leak between tests. +describe('roster-drop WARN dedupes identical repeats, re-warns on set change', () => { + let logFile: string + + beforeEach(() => { + logFile = join(dir, 'dedup.log') + process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = logFile + process.env.OPENCODE_OPENAI_AUTH_LOG_LEVEL = 'info' + }) + afterEach(() => { + process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = FLOOR_LOG_FILE + delete process.env.OPENCODE_OPENAI_AUTH_LOG_LEVEL + }) + + it('two consecutive loads with the same dropped id emit exactly one WARN', async () => { + const { saveAccounts, loadAccounts } = await import('../core/accounts.ts') + const { flushForTest } = await import('../logger.ts') + const droppedId = `dedup-${randomUUID()}` + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('keep'), oauthAccount(droppedId)], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts[droppedId] + writeFileSync(statePath, JSON.stringify(stateObj)) + + await loadAccounts(cfgPath) + await loadAccounts(cfgPath) // identical dropped set — should be deduped + await flushForTest() + + const logTxt = readFileSync(logFile, 'utf8') + const warns = logTxt.match(/WARN \[accounts\]/g) ?? [] + expect(warns.length).toBe(1) + expect(logTxt).toContain(droppedId) + }) + + it('a new dropped id (different from the previously-warned set) re-warns', async () => { + const { saveAccounts, loadAccounts } = await import('../core/accounts.ts') + const { flushForTest } = await import('../logger.ts') + const firstId = `dedup-A-${randomUUID()}` + const secondId = `dedup-B-${randomUUID()}` + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('keep'), oauthAccount(firstId)], + }, + cfgPath, + ) + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts[firstId] + writeFileSync(statePath, JSON.stringify(stateObj)) + await loadAccounts(cfgPath) + + // Different broken id — warn again. + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('keep'), oauthAccount(secondId)], + }, + cfgPath, + ) + const stateRaw2 = readFileSync(statePath, 'utf8') + const stateObj2 = JSON.parse(stateRaw2) + delete stateObj2.accounts[secondId] + writeFileSync(statePath, JSON.stringify(stateObj2)) + await loadAccounts(cfgPath) + await flushForTest() + + const logTxt = readFileSync(logFile, 'utf8') + const warns = logTxt.match(/WARN \[accounts\]/g) ?? [] + expect(warns.length).toBe(2) + expect(logTxt).toContain(firstId) + expect(logTxt).toContain(secondId) + }) }) diff --git a/packages/opencode/src/tests/commands.test.ts b/packages/opencode/src/tests/commands.test.ts index 225dd2b..045f9b4 100644 --- a/packages/opencode/src/tests/commands.test.ts +++ b/packages/opencode/src/tests/commands.test.ts @@ -1348,6 +1348,47 @@ describe('commands', () => { expect(cfg.accounts.map((a: { id: string }) => a.id)).toEqual(['healthy']) }) + // An id that was never on disk (fat-finger typo) must report Not + // Found — lying "Removed" would silently mask the operator's typo and + // leave their real account untouched. The message comes from a closure + // flag OR'd with a pre-read saw-it: a never-existed id has neither + // signal, so it reports Not Found. The unrelated healthy account is NOT + // collateral damage. + test('openai-account remove of a nonexistent id reports Not Found', async () => { + const account = makeAccount('acct-present') + const qm = new QuotaManager({ + storage: { version: 1 as const, accounts: [account] }, + }) + const ctx: CommandContext = { + accountStoragePath: configPath, + quotaManager: qm, + loadAccounts, + client: makeClient(), + } + await saveAccounts( + { + version: 1 as const, + main: { type: 'opencode', provider: 'openai' }, + accounts: [account], + }, + configPath, + ) + + const payload = await buildDialogPayload( + 'openai-account', + 'remove ghost-id', + ctx, + ) + expect(payload.text).toContain('Not Found') + expect(payload.text).toContain('ghost-id') + expect(payload.text).not.toContain('Removed') + // The healthy account must NOT have been removed by an unrelated typo. + const cfg = JSON.parse(readFileSync(configPath, 'utf8')) + expect(cfg.accounts.map((a: { id: string }) => a.id)).toEqual([ + 'acct-present', + ]) + }) + test('refreshSidebar called after order', async () => { const account = makeAccount('acct-1') const acct2 = makeAccount('acct-2') diff --git a/packages/opencode/src/tests/error-contract.test.ts b/packages/opencode/src/tests/error-contract.test.ts index ef96dd0..c52d6e0 100644 --- a/packages/opencode/src/tests/error-contract.test.ts +++ b/packages/opencode/src/tests/error-contract.test.ts @@ -118,6 +118,69 @@ describe('error contract', () => { } }) + // A missing refresh_token or an empty-string refresh_token legitimately + // means "no rotation" (the server declined to rotate; keep using the + // current one). A DEFINED non-string value — a number, null, an object, + // etc. — is a genuinely malformed response and must throw, not be + // silently swallowed as a successful refresh. + it.each([ + ['number', 12345], + ['null', null], + ['object', { malformed: true }], + ['boolean', true], + ['array', ['refresh']], + ])( + 'throws malformed response when refresh_token is a defined non-string (%s)', + async (_label, refreshValue) => { + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers(), + json: async () => ({ + access_token: 'valid-access', + refresh_token: refreshValue, + expires_in: 3600, + }), + }) + + try { + await codexRefreshFn({ + refreshToken: 'kept-input-refresh', + fetchImpl: mockFetch as unknown as typeof fetch, + now: () => Date.now(), + }) + expect.unreachable('should have thrown') + } catch (error) { + const e = error as ProviderHttpError + expect(e.message.toLowerCase()).toContain('malformed') + expect(e.status).toBe(200) + expect(e.isRefreshError).toBe(true) + } + }, + ) + + it('reuses the input refresh token when refresh_token is an empty string', async () => { + const mockFetch = jest.fn().mockResolvedValue({ + ok: true, + status: 200, + headers: new Headers(), + json: async () => ({ + access_token: 'rotated-access', + refresh_token: '', + expires_in: 3600, + }), + }) + + const result = await codexRefreshFn({ + refreshToken: 'kept-input-refresh', + fetchImpl: mockFetch as unknown as typeof fetch, + now: () => Date.now(), + }) + + expect(result.refresh).toBe('kept-input-refresh') + expect(result.access).toBe('rotated-access') + }) + // ------------------------------------------------------------------- // isTransientRefreshError duck-types .status // ------------------------------------------------------------------- From 11491570372c31fbb2ca11169334cfdd693a71c7 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 15 Aug 2026 21:41:50 +0200 Subject: [PATCH 3/4] fix(accounts): stop re-login duplicating a preserved account Extracting the preserve pipeline collapsed two sets that happened to hold the same value at the call site but answer different questions. One decides WHAT to preserve - an id the load dropped, as opposed to one the mutator deliberately removed - and must be read before the mutator runs. The other decides whether the writer is already emitting that id, and can only be read after. Using the first for both meant an id the mutator re-added was preserved alongside itself. Re-login is exactly that case. A load-dropped account is by definition absent from the roster upsert is handed, so upsert finds no match and pushes a fresh entry; the preserve pass then appended the stale raw one next to it. The duplicate survived into the loaded roster, so routing saw two candidates under one id, the sidebar listed it twice, and remove spliced only the first - on the path an operator walks to recover from the failure this whole change exists to prevent. The two sets are separate again: the helper takes the pre-mutator set and decides preservation, and each writer filters the result against the ids it is about to serialize. Both sides compare trimmed ids, matching the roster collector, so a padded id cannot slip past the comparison and reintroduce the duplicate. Also: the warning dedup key joined ids with a comma, so one id containing a comma collided with two ids that did not and silently suppressed a real warning. It now encodes the sorted array. --- packages/opencode/src/core/accounts.ts | 121 +++++++++--- .../opencode/src/tests/accounts-store.test.ts | 177 +++++++++++++++++- 2 files changed, 276 insertions(+), 22 deletions(-) diff --git a/packages/opencode/src/core/accounts.ts b/packages/opencode/src/core/accounts.ts index 78017a9..d6ac9c4 100644 --- a/packages/opencode/src/core/accounts.ts +++ b/packages/opencode/src/core/accounts.ts @@ -543,7 +543,11 @@ const warnedRosterDrops = new Set() function emitRosterDropWarning(droppedIds: readonly string[]): void { if (droppedIds.length === 0) return - const key = [...droppedIds].sort().join(',') + // JSON.stringify so id strings that contain a comma can't hash to the + // same key as a comma-less split of the same characters — e.g. + // {`a,b`} would join to `a,b`, colliding with {`a`, `b`} on join and + // silently suppressing the second WARN. + const key = JSON.stringify([...droppedIds].sort()) if (warnedRosterDrops.has(key)) return warnedRosterDrops.add(key) logA.warn('account load-dropped, preserved on disk', { @@ -1034,16 +1038,39 @@ export async function saveAccounts( // Preserve load-dropped raw entries — shared pipeline with mutateAccounts // (no allowDrop seam here; this writer has no caller-driven removal - // intent). The WARN and dedup live on the shared helper, so both - // writers stay in sync. - const latestAccountIds = new Set(merged.accounts.map((a) => a.id)) - const additions = buildPreservedAdditions( + // intent). The WARN lives on the shared helper. `latestAccountIds` + // is the loaded (post-normalize) set: an id that survived the load + // is NOT actually load-dropped and must not be re-appended as a raw + // entry. `latest` can be null (no config file); in that case the + // loaded set is empty and the emitted set is whatever the caller's + // `storage` arg carries — which is fine because there is no raw + // config to preserve from. + const latestAccountIds = latest + ? new Set(latest.accounts.map((a) => a.id)) + : new Set() + const preserved = buildPreservedAdditions( configJson.value, latestAccountIds, new Set(), ) - + // Drop preserved entries whose ids the writer is already emitting via + // the serialized output. trim() on both sides matches collectConfigRosterIds + // and guards against whitespace-padded raw entries duplicating + // already-trimmed serialized ids. const baseConfig = configFromStorage(merged) + const writtenIds = new Set( + (Array.isArray(baseConfig.accounts) ? baseConfig.accounts : []) + .map((e) => + isRecord(e) && typeof e.id === 'string' ? e.id.trim() : '', + ) + .filter(Boolean), + ) + const additions = preserved.filter((raw) => { + if (!isRecord(raw)) return false + if (typeof raw.id !== 'string') return false + return !writtenIds.has(raw.id.trim()) + }) + const nextConfig = { ...existing, ...baseConfig, @@ -1151,14 +1178,28 @@ function pickRawRosterEntriesForPreservation( * own copy of the iteration logic) is drift-prone by construction: the * next invariant change would land on one writer only. * - * `loadedIds` is the writer's outgoing account-id set: in mutateAccounts - * it is the pre-mutator loaded roster; in saveAccounts it is the merged - * candidate set. Either side of that distinction gets the same - * preservation behaviour. - * - * Any id already in `loadedIds` is filtered out of `additions` so the - * writer does not duplicate a raw entry the caller/merger already - * accounted for. + * Takes two distinct id sets because they answer two distinct questions; + * collapsing them back into one (as an earlier refactor did) is wrong + * because an id the caller is actively emitting must NOT be appended a + * second time as a stale raw entry. A re-login path is the live example: + * the mutator pushes a fresh entry for an id whose state entry was + * missing, the load-time preserver sees the same id in raw config and + * would otherwise append the stale raw entry alongside the fresh one. + * - `loadedIds`: what normalizeStorage produced for the merged + * config+state — the PRE-mutator / post-normalize set. Drives the + * pickRawRosterEntriesForPreservation decision (an id that survived + * normalization is not actually load-dropped). + * - `emittedIds`: what the writer is about to write — POST-mutator + * for mutateAccounts (the ids in `next.accounts`), POST-union for + * saveAccounts (the ids in `merged.accounts`). Drives the dedup + * filter on the raw entries the writer appends. + */ +/** + * Walks a list of mixed-shape config entries (some are normalized account + * configs, some are raw preserved entries passed through verbatim) and + * collects the string ids. Used for the write-debug log so the + * diagnostic surface reflects what landed on disk rather than what the + * mutator returned. */ /** * Walks a list of mixed-shape config entries (some are normalized account @@ -1178,6 +1219,29 @@ function collectStringIds(entries: unknown): string[] { return ids } +/** + * Returns the raw config entries that survived load-time rejection (i.e. + * they exist on disk as raw entries but normalizeAccount could not accept + * them — state entry missing, baseURL malformed, etc.) so a subsequent + * write can carry them back to disk verbatim instead of silently erasing + * the operator's account. The WARN is emitted here (dedup'd by + * emitRosterDropWarning) so the load-drop signal is centralized. + * + * `loadedIds` is the set of ids that DID survive normalization (pre- + * mutator / post-normalize) — used by pickRawRosterEntriesForPreservation + * to distinguish a real load-time drop from an id the writer is about + * to refresh via re-login. Wrong `loadedIds` here turns "deliberate + * removal" into "preservation resurrection" because both look the same + * at the load boundary. + * + * The dedup against the writer's *output* (a stale raw entry must not + * be appended alongside a fresh entry the writer just re-added) is the + * call site's responsibility because it needs the writer's serialized + * output (`configFromStorage(next)` for mutateAccounts, the same against + * `merged` for saveAccounts) and a trim on BOTH sides (raw and + * serialized ids may differ in whitespace — see collectionConfigRosterIds + * for the matching trim on the load side). + */ function buildPreservedAdditions( rawConfigValue: unknown, loadedIds: Set, @@ -1193,11 +1257,7 @@ function buildPreservedAdditions( allowDrop, ) emitRosterDropWarning(preservedIds) - return preservedRawEntries.filter((raw) => { - if (!isRecord(raw)) return false - if (typeof raw.id !== 'string') return false - return !loadedIds.has(raw.id.trim()) - }) + return preservedRawEntries } /** @@ -1269,13 +1329,32 @@ export async function mutateAccounts( // — if it was in current.accounts, normalizeAccount accepted it and // there is no load-time drop to preserve. const allowDrop = new Set(options.allowDrop ?? []) - const additions = buildPreservedAdditions( + const preserved = buildPreservedAdditions( configJson.value, currentAccountIds, allowDrop, ) - + // Drop preserved entries whose ids the mutator is already emitting in + // normalized form via baseConfig.accounts — the live example is a + // re-login: the mutator pushes a fresh entry for an id whose state + // entry was missing, and a stale raw entry for the same id must + // NOT be appended alongside it (round-4 had this regression). trim() + // on both sides matches collectConfigRosterIds so a whitespace- + // padded raw id doesn't sneak past the comparison. const baseConfig = configFromStorage(next) + const writtenIds = new Set( + (Array.isArray(baseConfig.accounts) ? baseConfig.accounts : []) + .map((e) => + isRecord(e) && typeof e.id === 'string' ? e.id.trim() : '', + ) + .filter(Boolean), + ) + const additions = preserved.filter((raw) => { + if (!isRecord(raw)) return false + if (typeof raw.id !== 'string') return false + return !writtenIds.has(raw.id.trim()) + }) + const existing = isRecord(configJson.value) ? configJson.value : {} const nextConfig = { ...existing, diff --git a/packages/opencode/src/tests/accounts-store.test.ts b/packages/opencode/src/tests/accounts-store.test.ts index 6474795..90e6497 100644 --- a/packages/opencode/src/tests/accounts-store.test.ts +++ b/packages/opencode/src/tests/accounts-store.test.ts @@ -1416,6 +1416,106 @@ describe('mutateAccounts load-time roster preservation', () => { // re-add or fix the URL — the loader does not silently swallow it. expect(preserved?.baseURL).toBe('not-a-url') }) + + // N1 regression: when the mutator re-adds a load-dropped entry (the + // exact shape of `re-login` for an account whose state entry is missing), + // the preserved raw entry must not be appended alongside the mutator's + // fresh version. The two id sets the helper accepts answer distinct + // questions — `loadedIds` (pre-mutator) decides what to preserve, and + // `emittedIds` (post-mutator) decides whether the writer is already + // emitting that id — and the F3 extraction dropped the second one. + it('mutator re-add of a load-dropped entry does NOT append a duplicate raw entry', async () => { + const { saveAccounts, mutateAccounts } = await import('../core/accounts.ts') + const healthy = oauthAccount('healthy') + const broken = oauthAccount('broken') + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [healthy, broken], + }, + cfgPath, + ) + // Strip 'broken' from state — it is now load-dropped. + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts.broken + writeFileSync(statePath, JSON.stringify(stateObj)) + + // Mutator re-adds 'broken' with fresh tokens — the shape of re-login. + await mutateAccounts((current) => { + current.accounts.push({ + ...oauthAccount('broken'), + access: 'fresh-access-broken', + refresh: 'fresh-refresh-broken', + expires: Date.now() + 3600_000, + }) + return current + }, cfgPath) + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + const brokenEntries = cfg.accounts.filter( + (a: { id: string }) => a.id === 'broken', + ) + // Exactly ONE entry for 'broken' — the mutator's fresh one. A second + // (stale raw) entry here is the F3-extraction regression. + expect(brokenEntries.length).toBe(1) + // Pin where the survivor lives: the state file carries the fresh + // tokens because accountConfig strips them from config, so the test + // asserts on state.broken.refresh rather than the config-side + // accountConfig projection. + const stateAfter = JSON.parse(readFileSync(statePath, 'utf8')) + expect(stateAfter.accounts.broken.refresh).toBe('fresh-refresh-broken') + }) + + // Padded id variant of the same regression: a load-dropped entry whose + // raw id is ` padded ` (whitespace) loads with no state record + // and would re-append against the mutator's trimmed `padded` entry + // unless the writer's "already emitting" set is built with trim() on + // both sides. collectConfigRosterIds trims on the load side; this + // test pins the trim on the writer side. + it('mutator re-add of a load-dropped WHITESPACE-PADDED id does NOT duplicate', async () => { + const { saveAccounts, mutateAccounts } = await import('../core/accounts.ts') + const rawId = ' padded ' + const trimmedId = 'padded' + await saveAccounts( + { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: [oauthAccount('healthy'), oauthAccount(rawId)], + }, + cfgPath, + ) + // Strip the state entry for the padded id so it is load-dropped. + const stateRaw = readFileSync(statePath, 'utf8') + const stateObj = JSON.parse(stateRaw) + delete stateObj.accounts[rawId] + writeFileSync(statePath, JSON.stringify(stateObj)) + + // Mutator re-adds under the trimmed id (normalizeAccountBase trims + // the raw whitespace from any id the caller passes). + await mutateAccounts((current) => { + current.accounts.push({ + ...oauthAccount(trimmedId), + access: 'fresh-access', + refresh: 'fresh-refresh', + expires: Date.now() + 3600_000, + }) + return current + }, cfgPath) + + const cfg = JSON.parse(readFileSync(cfgPath, 'utf8')) + const paddedEntries = cfg.accounts.filter( + (a: { id: string }) => a.id === trimmedId, + ) + expect(paddedEntries.length).toBe(1) + // The raw padded entry must NOT be present (its trimmed form is the + // fresh one already on disk; the raw is what would duplicate). + const rawEntries = cfg.accounts.filter( + (a: { id: string }) => a.id === rawId, + ) + expect(rawEntries.length).toBe(0) + }) }) // --------------------------------------------------------------------------- @@ -1477,7 +1577,6 @@ describe('normalizeStorage roster drop is loud on every load', () => { logFile = join(dir, 'drop.log') process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = logFile process.env.OPENCODE_OPENAI_AUTH_LOG_LEVEL = 'info' - process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = logFile }) afterEach(() => { process.env.OPENCODE_OPENAI_AUTH_LOG_FILE = FLOOR_LOG_FILE @@ -1649,4 +1748,80 @@ describe('roster-drop WARN dedupes identical repeats, re-warns on set change', ( expect(logTxt).toContain(firstId) expect(logTxt).toContain(secondId) }) + + // The dedup key must not allow a single id containing a comma to hash + // to the same value as two ids whose join-string is identical. Set A + // = {`a,b`} and Set B = {`a`, `b`} have different ids but the same + // `[...droppedIds].sort().join(',')` output (`a,b`). The bug suppresses + // the second WARN silently. Use JSON.stringify so the two sets map to + // distinct keys and both WARN. + it('warn dedup key resists comma-collision in id strings', async () => { + const { loadAccounts } = await import('../core/accounts.ts') + const { flushForTest } = await import('../logger.ts') + + function writeConfigAndState( + accounts: Array<{ id: string; refresh: string }>, + ) { + const cfg = { + version: 1, + main: { type: 'opencode', provider: 'openai' }, + accounts: accounts.map((a) => ({ + id: a.id, + type: 'oauth', + enabled: true, + })), + } + const state = { + version: 1, + accounts: Object.fromEntries( + accounts.map((a) => [ + a.id, + { + access: `acc-${a.id}`, + refresh: a.refresh, + expires: Date.now() + 3600_000, + }, + ]), + ), + } + writeFileSync(cfgPath, `${JSON.stringify(cfg)}\n`) + writeFileSync(statePath, `${JSON.stringify(state)}\n`) + } + + // Setup 1: a single load-dropped entry with id 'a,b' (literal comma) + // — strip its refresh from state so normalize cannot accept it. + writeConfigAndState([ + { id: 'h-n2-comma', refresh: 'r-hn2-comma' }, + { id: 'a,b', refresh: 'r-abc' }, + ]) + // Strip 'a,b' from state at the field level (state value is keyed by id). + const stateRaw1 = readFileSync(statePath, 'utf8') + const stateObj1 = JSON.parse(stateRaw1) + delete stateObj1.accounts['a,b'] + writeFileSync(statePath, JSON.stringify(stateObj1)) + await loadAccounts(cfgPath) // load 1: drops = ['a,b'] + + // Setup 2: write fresh config + state with two load-dropped entries + // 'a' and 'b' (no commas in their ids). Direct file writes avoid any + // saveAccounts-side append that would skew the drop set. + writeConfigAndState([ + { id: 'h-n2-comma', refresh: 'r-hn2-comma' }, + { id: 'a', refresh: 'r-a' }, + { id: 'b', refresh: 'r-b' }, + ]) + const stateRaw2 = readFileSync(statePath, 'utf8') + const stateObj2 = JSON.parse(stateRaw2) + delete stateObj2.accounts.a + delete stateObj2.accounts.b + writeFileSync(statePath, JSON.stringify(stateObj2)) + await loadAccounts(cfgPath) // load 2: drops = ['a','b'] + await flushForTest() + + const logTxt = readFileSync(logFile, 'utf8') + const warns = logTxt.match(/WARN \[accounts\]/g) ?? [] + // Buggy join(',') dedup: 'a,b' (load 1) and 'a,b' (load 2) collide; + // second WARN suppressed → 1. JSON dedup: keys differ + // ('["a,b"]' vs '["a","b"]') → 2. + expect(warns.length).toBe(2) + }) }) From 2ad77e5018d370dfaa1811021cb288152a5075a8 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 15 Aug 2026 22:07:15 +0200 Subject: [PATCH 4/4] docs(accounts): correct the preserve helper's contract, isolate its test The doc block on buildPreservedAdditions still described an emittedIds parameter that moved to the call sites, so the comment explaining the distinction that has now broken twice described a signature the function does not have. It documents the real one: the helper decides what to preserve from the load-side view, and each writer separately filters that against the ids it is about to serialize, because only the writer knows its own output. Two stranded duplicate doc blocks from the same extraction are gone, and a misspelled reference to collectConfigRosterIds is fixed. The dedup-collision test used fixed ids against a module-level set that is never reset and is shared across the whole process, while its siblings deliberately namespace theirs. It now namespaces too. The collision it exercises needs the two id sets to join to the same string, which constrains the prefix to sort before the unprefixed id - recorded in the test, since it otherwise reads as an arbitrary choice. --- packages/opencode/src/core/accounts.ts | 68 +++++-------------- .../opencode/src/tests/accounts-store.test.ts | 59 ++++++++++------ 2 files changed, 55 insertions(+), 72 deletions(-) diff --git a/packages/opencode/src/core/accounts.ts b/packages/opencode/src/core/accounts.ts index d6ac9c4..d373327 100644 --- a/packages/opencode/src/core/accounts.ts +++ b/packages/opencode/src/core/accounts.ts @@ -1169,38 +1169,6 @@ function pickRawRosterEntriesForPreservation( return { preservedRawEntries, preservedIds } } -/** - * Shared roster-preservation pipeline used by both `mutateAccounts` and - * `saveAccounts`. Returns the raw entries that should be appended to the - * writer's outgoing accounts list so a load-dropped entry survives the - * write. The WARN is emitted here (dedup'd by emitRosterDropWarning) so - * both writers stay in sync — the alternative (each writer owning its - * own copy of the iteration logic) is drift-prone by construction: the - * next invariant change would land on one writer only. - * - * Takes two distinct id sets because they answer two distinct questions; - * collapsing them back into one (as an earlier refactor did) is wrong - * because an id the caller is actively emitting must NOT be appended a - * second time as a stale raw entry. A re-login path is the live example: - * the mutator pushes a fresh entry for an id whose state entry was - * missing, the load-time preserver sees the same id in raw config and - * would otherwise append the stale raw entry alongside the fresh one. - * - `loadedIds`: what normalizeStorage produced for the merged - * config+state — the PRE-mutator / post-normalize set. Drives the - * pickRawRosterEntriesForPreservation decision (an id that survived - * normalization is not actually load-dropped). - * - `emittedIds`: what the writer is about to write — POST-mutator - * for mutateAccounts (the ids in `next.accounts`), POST-union for - * saveAccounts (the ids in `merged.accounts`). Drives the dedup - * filter on the raw entries the writer appends. - */ -/** - * Walks a list of mixed-shape config entries (some are normalized account - * configs, some are raw preserved entries passed through verbatim) and - * collects the string ids. Used for the write-debug log so the - * diagnostic surface reflects what landed on disk rather than what the - * mutator returned. - */ /** * Walks a list of mixed-shape config entries (some are normalized account * configs, some are raw preserved entries passed through verbatim) and @@ -1220,27 +1188,25 @@ function collectStringIds(entries: unknown): string[] { } /** - * Returns the raw config entries that survived load-time rejection (i.e. - * they exist on disk as raw entries but normalizeAccount could not accept - * them — state entry missing, baseURL malformed, etc.) so a subsequent - * write can carry them back to disk verbatim instead of silently erasing - * the operator's account. The WARN is emitted here (dedup'd by - * emitRosterDropWarning) so the load-drop signal is centralized. + * Returns the raw config entries that survived load-time rejection so a + * subsequent writer can carry them back to disk verbatim instead of + * silently erasing them. The WARN is emitted here (dedup'd by + * emitRosterDropWarning) so the load-drop signal lives in one place — + * the alternative (each writer owning its own iteration logic) is + * drift-prone by construction: the next invariant change would land + * on one writer only. * - * `loadedIds` is the set of ids that DID survive normalization (pre- - * mutator / post-normalize) — used by pickRawRosterEntriesForPreservation - * to distinguish a real load-time drop from an id the writer is about - * to refresh via re-login. Wrong `loadedIds` here turns "deliberate - * removal" into "preservation resurrection" because both look the same - * at the load boundary. + * `loadedIds` answers the load-side question: what ids survived + * normalization? An id in that set is not actually load-dropped and + * must not be preserved as a raw entry. * - * The dedup against the writer's *output* (a stale raw entry must not - * be appended alongside a fresh entry the writer just re-added) is the - * call site's responsibility because it needs the writer's serialized - * output (`configFromStorage(next)` for mutateAccounts, the same against - * `merged` for saveAccounts) and a trim on BOTH sides (raw and - * serialized ids may differ in whitespace — see collectionConfigRosterIds - * for the matching trim on the load side). + * The writer-side question — is this id already being emitted in the + * caller's serialized output? — is the call site's responsibility. It + * sees its own output (`configFromStorage(next)` for mutateAccounts, + * `configFromStorage(merged)` for saveAccounts) and applies the dedup + * filter there, with `id.trim()` on both sides. The split exists + * because the helper has no view into the writer's specific output + * and the trim must match `collectConfigRosterIds` on the load side. */ function buildPreservedAdditions( rawConfigValue: unknown, diff --git a/packages/opencode/src/tests/accounts-store.test.ts b/packages/opencode/src/tests/accounts-store.test.ts index 90e6497..7bf9d6a 100644 --- a/packages/opencode/src/tests/accounts-store.test.ts +++ b/packages/opencode/src/tests/accounts-store.test.ts @@ -1751,10 +1751,21 @@ describe('roster-drop WARN dedupes identical repeats, re-warns on set change', ( // The dedup key must not allow a single id containing a comma to hash // to the same value as two ids whose join-string is identical. Set A - // = {`a,b`} and Set B = {`a`, `b`} have different ids but the same - // `[...droppedIds].sort().join(',')` output (`a,b`). The bug suppresses + // = {`:a,b`} and Set B = {`:a`, `b`} (where the prefix + // is constructed to start with a char < `b` so the sort order lines up + // both setups to the same joined string) have different ids but the + // same `[...droppedIds].sort().join(',')` output. The bug suppresses // the second WARN silently. Use JSON.stringify so the two sets map to // distinct keys and both WARN. + // + // `warnedRosterDrops` is module-level and never reset, so the dropped-id + // sets here are built from a unique randomUUID() prefix to keep them + // from colliding with any earlier test in the process. The prefix is + // itself load-bearing for the bug-trigger: the two setups' sort+join + // characters must be byte-identical, which is impossible to arrange + // when both setups use the same prefix-segmented ids on each side, so + // Setup 2's second id is left unprefixed (just `b`) and the prefix's + // leading character is forced below `b` in lex order. it('warn dedup key resists comma-collision in id strings', async () => { const { loadAccounts } = await import('../core/accounts.ts') const { flushForTest } = await import('../logger.ts') @@ -1788,40 +1799,46 @@ describe('roster-drop WARN dedupes identical repeats, re-warns on set change', ( writeFileSync(statePath, `${JSON.stringify(state)}\n`) } - // Setup 1: a single load-dropped entry with id 'a,b' (literal comma) - // — strip its refresh from state so normalize cannot accept it. + // Prefix starts with `a` so `${prefix}:a` < `b` lexicographically; + // that ordering is what makes Setup 2's two-id join + // (`${prefix}:a,b`) equal Setup 1's one-id join. randomUUID() makes + // the prefix unique within the process so the dedup set cannot be + // polluted by prior tests. + const prefix = `a${randomUUID().slice(0, 8)}` + const healthyId = `${prefix}:healthy` + const collapsedId = `${prefix}:a,b` + const split1Id = `${prefix}:a` + const split2Id = 'b' + writeConfigAndState([ - { id: 'h-n2-comma', refresh: 'r-hn2-comma' }, - { id: 'a,b', refresh: 'r-abc' }, + { id: healthyId, refresh: `r-${healthyId}` }, + { id: collapsedId, refresh: `r-${collapsedId}` }, ]) - // Strip 'a,b' from state at the field level (state value is keyed by id). const stateRaw1 = readFileSync(statePath, 'utf8') const stateObj1 = JSON.parse(stateRaw1) - delete stateObj1.accounts['a,b'] + delete stateObj1.accounts[collapsedId] writeFileSync(statePath, JSON.stringify(stateObj1)) - await loadAccounts(cfgPath) // load 1: drops = ['a,b'] + await loadAccounts(cfgPath) // load 1: drops = [collapsedId] - // Setup 2: write fresh config + state with two load-dropped entries - // 'a' and 'b' (no commas in their ids). Direct file writes avoid any - // saveAccounts-side append that would skew the drop set. writeConfigAndState([ - { id: 'h-n2-comma', refresh: 'r-hn2-comma' }, - { id: 'a', refresh: 'r-a' }, - { id: 'b', refresh: 'r-b' }, + { id: healthyId, refresh: `r-${healthyId}` }, + { id: split1Id, refresh: `r-${split1Id}` }, + { id: split2Id, refresh: `r-${split2Id}` }, ]) const stateRaw2 = readFileSync(statePath, 'utf8') const stateObj2 = JSON.parse(stateRaw2) - delete stateObj2.accounts.a - delete stateObj2.accounts.b + delete stateObj2.accounts[split1Id] + delete stateObj2.accounts[split2Id] writeFileSync(statePath, JSON.stringify(stateObj2)) - await loadAccounts(cfgPath) // load 2: drops = ['a','b'] + await loadAccounts(cfgPath) // load 2: drops = [split1Id, split2Id] await flushForTest() const logTxt = readFileSync(logFile, 'utf8') const warns = logTxt.match(/WARN \[accounts\]/g) ?? [] - // Buggy join(',') dedup: 'a,b' (load 1) and 'a,b' (load 2) collide; - // second WARN suppressed → 1. JSON dedup: keys differ - // ('["a,b"]' vs '["a","b"]') → 2. + // Buggy join(',') dedup: `${prefix}:a,b` (load 1) and `${prefix}:a,b` + // (load 2, after sort) collide; second WARN suppressed → 1. + // JSON dedup: keys differ (`["${prefix}:a,b"]` vs + // `["${prefix}:a","b"]`) → 2. expect(warns.length).toBe(2) }) })