diff --git a/packages/opencode/src/cli.ts b/packages/opencode/src/cli.ts index 8f06560..983e443 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) { + // `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 preReadSawIt = rawRoster ? rawRoster.has(targetId) : 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) + mutatorSplicedIt = true + return current + }, + configPath, + { allowDrop: [targetId] }, + ) + + 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 29e19ed..c08b20b 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' @@ -268,14 +269,38 @@ async function executeAccountCommand( // 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) + // + // `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) + mutatorSplicedIt = true + return current + }, + ctx.accountStoragePath, + { allowDrop: [targetId] }, + ) + + const removed = mutatorSplicedIt || preReadSawIt if (!removed) { return { diff --git a/packages/opencode/src/core/accounts.ts b/packages/opencode/src/core/accounts.ts index 4e61e48..d373327 100644 --- a/packages/opencode/src/core/accounts.ts +++ b/packages/opencode/src/core/accounts.ts @@ -525,8 +525,69 @@ 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 + // 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', { + droppedIds, + }) +} + 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) + } + emitRosterDropWarning(dropped) + } + return { version: 1, main: { type: 'opencode', provider: 'openai' }, @@ -544,9 +605,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 +644,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 +659,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 +1035,50 @@ 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 — shared pipeline with mutateAccounts + // (no allowDrop seam here; this writer has no caller-driven removal + // 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, + accounts: [ + ...(Array.isArray(baseConfig.accounts) ? baseConfig.accounts : []), + ...additions, + ], + } await writeJsonAtomic(path, nextConfig) await writeJsonAtomic(statePath, stateFromStorage(merged)) } finally { @@ -981,6 +1089,143 @@ 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. + * + * 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, + 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 } +} + +/** + * 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 +} + +/** + * 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` 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 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, + 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 +} + /** * Read-modify-write the account store atomically under the save lock. * @@ -996,10 +1241,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 +1275,71 @@ 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. + // 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 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 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, ...configFromStorage(next) } + const nextConfig = { + ...existing, + ...baseConfig, + accounts: [ + ...(Array.isArray(baseConfig.accounts) ? baseConfig.accounts : []), + ...additions, + ], + } 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: nextConfig.accounts.length, + accountIds: collectStringIds(nextConfig.accounts), + }) return next } finally { await stateLock.release() diff --git a/packages/opencode/src/core/provider.ts b/packages/opencode/src/core/provider.ts index 3877b3e..d13e106 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,27 @@ export async function codexRefreshFn(input: { isRefreshError: true, }) as ProviderHttpError } + // 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 + : 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..7bf9d6a 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,691 @@ 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') + }) + + // 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) + }) +}) + +// --------------------------------------------------------------------------- +// 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' + }) + 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') + }) + + // 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) + }) + + // 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`} (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') + + 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`) + } + + // 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: healthyId, refresh: `r-${healthyId}` }, + { id: collapsedId, refresh: `r-${collapsedId}` }, + ]) + const stateRaw1 = readFileSync(statePath, 'utf8') + const stateObj1 = JSON.parse(stateRaw1) + delete stateObj1.accounts[collapsedId] + writeFileSync(statePath, JSON.stringify(stateObj1)) + await loadAccounts(cfgPath) // load 1: drops = [collapsedId] + + writeConfigAndState([ + { 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[split1Id] + delete stateObj2.accounts[split2Id] + writeFileSync(statePath, JSON.stringify(stateObj2)) + 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: `${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) + }) +}) diff --git a/packages/opencode/src/tests/commands.test.ts b/packages/opencode/src/tests/commands.test.ts index 390d93a..045f9b4 100644 --- a/packages/opencode/src/tests/commands.test.ts +++ b/packages/opencode/src/tests/commands.test.ts @@ -1294,6 +1294,101 @@ 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']) + }) + + // 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 42da348..c52d6e0 100644 --- a/packages/opencode/src/tests/error-contract.test.ts +++ b/packages/opencode/src/tests/error-contract.test.ts @@ -64,6 +64,123 @@ 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') + } + }) + + // 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 // ------------------------------------------------------------------- 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 () => {