fix(oauth): drop legacy credential backup on destructive mutations

src/oauth/store.ts

@@ -4,8 +4,10 @@
  * Multiauth shape (260706): each provider value is a ProviderAccountSet
  * `{ activeAccountId, accounts: [{ id, credential, needsReauth?, addedAt? }] }`.
  * Legacy single-credential values (`{ access, refresh, expires, ... }`) normalize on load,
- * and the first new-shape persist writes a one-time `auth.json.pre-multiauth` backup so a
- * downgraded loader (which silently drops unknown shapes) cannot destroy refresh tokens.
+ * and the first non-destructive new-shape persist writes a one-time
+ * `auth.json.pre-multiauth` backup so a downgraded loader (which silently drops unknown
+ * shapes) cannot destroy refresh tokens. Destructive mutations remove that backup so
+ * logout and account deletion do not retain the deleted credentials.
  *
  * Exceptions:
  * - `chatgpt` stays single-slot (always replaced): codex-auth-api uses it as a scratch slot
@@ -448,6 +450,24 @@ function backupLegacyOnce(): void {
   } catch { /* best-effort */ }
 }
 
+/**
+ * Destructive mutations (logout, account deletion) also drop the downgrade backup: it
+ * holds a copy of the very credentials the user removed, so keeping it would retain
+ * tokens the user asked to destroy. Best-effort like the create path — the removal runs
+ * after persist, so a failed unlink must not report a failed logout for an account that
+ * is already gone. A stale uninstall-manifest entry is harmless: removeOwnedConfigState
+ * skips paths that no longer exist.
+ */
+function removeLegacyBackup(): void {
+  try {
+    unlinkSync(`${getAuthStorePath()}.pre-multiauth`);
+  } catch (error) {
+    if (errorCode(error) !== "ENOENT") {
+      console.warn(`[oauth] could not remove legacy credential backup: ${error instanceof Error ? error.message : String(error)}`);
+    }
+  }
+}
+
 function isCredentialSource(value: unknown): value is OAuthCredentialSource {
   return value === "oauth" || value === "local-cli" || value === "credential-file" || value === "environment" || value === "manual";
 }
@@ -712,16 +732,24 @@ function serializeMutation<T>(work: () => Promise<T>, retainedValues: readonly u
   drainOAuthMutations();
   return result;
 }
-export function mutateStore<T>(fn:(store:AuthStore)=>T|Promise<T>, retainedValues: readonly unknown[] = [], options?: { waitMs?: number; assertBeforePersist?: () => void }):Promise<T>{return serializeMutation(async()=>{const guard=await createOAuthFileLock({path:getAuthStoreLockPath(),staleAfterMs:30000}).acquire();try{
+export function mutateStore<T>(fn:(store:AuthStore)=>T|Promise<T>, retainedValues: readonly unknown[] = [], options?: { waitMs?: number; assertBeforePersist?: () => void; removeLegacyBackup?: boolean | ((result: T) => boolean) }):Promise<T>{return serializeMutation(async()=>{const guard=await createOAuthFileLock({path:getAuthStoreLockPath(),staleAfterMs:30000}).acquire();try{
     const { store, hadLegacy } = loadAuthStoreInternal();
-    if (hadLegacy) backupLegacyOnce();
     const selections = new Map(Object.entries(store).map(([provider, set]) => [provider, {
       set,
       accountId: set.activeAccountId,
       revision: set.selectionRevision,
       accountIds: set.accounts.map(account => account.id),
     }]));
     const result = await fn(store);
+    // A destructive mutation that removed nothing must not drop the downgrade
+    // backup: the request was a no-op, so there is no deleted credential to
+    // stop retaining, and the backup is a whole-store copy for every provider.
+    // The decision needs the mutation's result, so it is taken here; `fn` only
+    // edits the in-memory store, and nothing has touched disk yet.
+    const dropLegacyBackup = typeof options?.removeLegacyBackup === "function"
+      ? options.removeLegacyBackup(result)
+      : options?.removeLegacyBackup === true;
+    if (hadLegacy && !dropLegacyBackup) backupLegacyOnce();
     options?.assertBeforePersist?.();
     const changedProviders: string[] = [];
     for (const provider of new Set([...selections.keys(), ...Object.keys(store)])) {
@@ -743,6 +771,7 @@ export function mutateStore<T>(fn:(store:AuthStore)=>T|Promise<T>, retainedValue
       }
     }
     persist(store);
+    if (dropLegacyBackup) removeLegacyBackup();
     for (const provider of changedProviders) publishAccountSelection(provider, "oauth");
     return result;
   }finally{guard.release();}}, retainedValues, options?.waitMs);
@@ -886,7 +915,7 @@ export async function removeCredential(provider: string): Promise<"removed" | "n
     }
     set.activeAccountId = set.accounts[0]!.id;
     return "removed" as const;
-  }, [provider]);
+  }, [provider], { removeLegacyBackup: result => result === "removed" });
 }
 
 // ---------------------------------------------------------------------------
@@ -1029,7 +1058,7 @@ export async function removeAccount(provider: string, accountId: string): Promis
     }
     if (set.activeAccountId === accountId) set.activeAccountId = set.accounts[0]!.id;
     return true;
-  }, [provider, accountId]);
+  }, [provider, accountId], { removeLegacyBackup: removed => removed });
   return removed;
 }
 

structure/overview.md

@@ -66,7 +66,7 @@ opencodex state root does not undo those writes. Putting native Codex back is th
 | Path | Owner | Notes |
 | --- | --- | --- |
 | `~/.opencodex/config.json` | opencodex | Init creates via private temp plus no-replace hard link; dashboard and explicit updates use atomic replacement. |
-| `~/.opencodex/auth.json` | opencodex | OAuth tokens; not committed. Multiauth shape: `provider -> { activeAccountId, accounts[] }` (legacy single-credential values normalize on load; a one-time `auth.json.pre-multiauth` backup guards downgrades). ChatGPT scratch OAuth stays separate from the Codex account store. For multi-slot providers, credentials without `accountId`/email replace the active slot on a normal login; an explicit add-account login preserves the prior slot and appends a distinct one. Single-slot providers such as ChatGPT remain replacement-only. |
+| `~/.opencodex/auth.json` | opencodex | OAuth tokens; not committed. Multiauth shape: `provider -> { activeAccountId, accounts[] }` (legacy single-credential values normalize on load; a one-time `auth.json.pre-multiauth` backup guards downgrades and is removed by destructive mutations such as logout or account deletion). ChatGPT scratch OAuth stays separate from the Codex account store. For multi-slot providers, credentials without `accountId`/email replace the active slot on a normal login; an explicit add-account login preserves the prior slot and appends a distinct one. Single-slot providers such as ChatGPT remain replacement-only. |
 | `~/.opencodex/codex-accounts.json` | opencodex | Hardened main-plus-added credential store used by `openai` in Pool mode. |
 | `~/.opencodex/catalog-backup.json` | opencodex | One-time pristine Codex catalog backup for restore; per-catalog copies are hashed variants (see [`catalog.md`](catalog.md)). |
 | `~/.opencodex/usage.jsonl` | opencodex | Append-only request usage log (0o600); request metadata + token counts only, never prompts or auth. |

tests/oauth/oauth-store-multi.test.ts

@@ -213,6 +213,71 @@ describe("multi-account auth store", () => {
     }
   });
 
+  test("logout migrates a legacy store without retaining its credential backup", async () => {
+    const authPath = join(TEST_DIR, "auth.json");
+    mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 });
+    writeFileSync(authPath, JSON.stringify({
+      xai: { access: "legacy-access", refresh: "legacy-refresh", expires: Date.now() + 1000 },
+    }));
+
+    expect(await removeCredential("xai")).toBe("removed");
+
+    expect(JSON.parse(readFileSync(authPath, "utf-8"))).toEqual({});
+    expect(existsSync(`${authPath}.pre-multiauth`)).toBe(false);
+  });
+
+  test("account deletion removes an existing legacy credential backup", async () => {
+    const authPath = join(TEST_DIR, "auth.json");
+    mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 });
+    const legacy = {
+      xai: { access: "legacy-access", refresh: "legacy-refresh", expires: Date.now() + 1000 },
+    };
+    writeFileSync(authPath, JSON.stringify(legacy));
+    writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify(legacy));
+    const accountId = getAccountSet("xai")!.activeAccountId;
+
+    expect(await removeAccount("xai", accountId)).toBe(true);
+
+    expect(JSON.parse(readFileSync(authPath, "utf-8"))).toEqual({});
+    expect(existsSync(`${authPath}.pre-multiauth`)).toBe(false);
+  });
+
+  test("logout on a migrated store still removes a stale credential backup", async () => {
+    const authPath = join(TEST_DIR, "auth.json");
+    mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 });
+    await saveCredential("xai", cred({ email: "a@example.test" }));
+    // A backup left over from an earlier migration holds copies of removed credentials.
+    writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify({ xai: { access: "stale", refresh: "stale", expires: 1 } }));
+
+    expect(await removeCredential("xai")).toBe("removed");
+
+    expect(existsSync(`${authPath}.pre-multiauth`)).toBe(false);
+  });
+
+  test("a logout that removed nothing keeps the downgrade backup", async () => {
+    const authPath = join(TEST_DIR, "auth.json");
+    mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 });
+    await saveCredential("xai", cred({ email: "a@example.test" }));
+    // The backup is a whole-store copy, so a no-op removal for one provider must
+    // not destroy downgrade recovery for every other provider in it.
+    writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify({ xai: { access: "stale", refresh: "stale", expires: 1 } }));
+
+    expect(await removeCredential("anthropic")).toBe("not-found");
+
+    expect(existsSync(`${authPath}.pre-multiauth`)).toBe(true);
+  });
+
+  test("an account deletion that matched nothing keeps the downgrade backup", async () => {
+    const authPath = join(TEST_DIR, "auth.json");
+    mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 });
+    await saveCredential("xai", cred({ email: "a@example.test" }));
+    writeFileSync(`${authPath}.pre-multiauth`, JSON.stringify({ xai: { access: "stale", refresh: "stale", expires: 1 } }));
+
+    expect(await removeAccount("xai", "no-such-account")).toBe(false);
+
+    expect(existsSync(`${authPath}.pre-multiauth`)).toBe(true);
+  });
+
   test("legacy credential WITHOUT identity gets a deterministic account id across loads", async () => {
     // Legacy stores are re-normalized on EVERY load without being persisted, so the
     // derived id must be stable: a time-salted id would make getAccountSet and