fix(storage): token-source duplicate must not disable the real org account on merge (#171) (#180)

* fix(storage): token-source duplicate must not disable the real org account on merge (#171)

Closes the last #171 recovery gap, found by testing the REAL persisted path:
when storage dedup collapses a disabled accountIdSource:"token" duplicate (a
plugin re-login artifact) into the matching org-source account by email, the
survivor inherited enabled:false. Combined with codex-doctor/codex-health
skipping disabled accounts, a single-real-account pool could end up dark and
UNRECOVERABLE — the user had no automatic path back.

Root cause: lib/storage/identity.ts mergeAccountRecords had no `enabled`
resolution — the flag fell through the `{...older, ...newest}` spread, and the
disabled token-dup could be the newest record.

Fix: add resolveMergedEnabled(target, source). When exactly one side is
org-like (organizationId present OR accountIdSource==="org") and the other is a
token-source duplicate, the org account's own enabled state governs. Every
other shape stays fail-closed (either disabled => disabled), preserving the
user-disable intent and the same-identity merge semantics from #64/#85.
Added `enabled` to the permissive AccountLike type.

Verified (architect-reviewed): identity.ts is the ONLY home for this
cross-source bug — login-runner's mergeStoredAccountPair only merges
exact-identity collisions (different key namespaces, can't hit the org<->token
shape) and is backstopped by this same fixed dedup on every persist.

Tests:
- test/storage.test.ts: token-dup does not disable org account (fresh token
  carried); fail-closed preserved for user-disabled same-identity; two distinct
  real org accounts both stay enabled (no #64/#85 regression).
- test/chaos/doctor-recovery-171-stress.test.ts: single-account + disabled dup
  recovers end-to-end through real saveAccounts/loadAccounts + recovery helpers.
- Mutation teeth-check: reverting resolveMergedEnabled fails the merge test.

Full suite green (2542 passed, 1 skipped), tsc + eslint + build clean,
npm audit 0 vulnerabilities.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: route #171 composite managers through liveManagers (PR #180 review)

Greptile P2: the new single-account composite describe created AccountManager
instances directly and disposed them inline, so a mid-test assertion failure
could leak a shutdown listener. Now uses the module-level makeManager() (which
registers in liveManagers) and drains liveManagers in this describe's afterEach,
matching the leak-safe pattern in the rest of the file.

Full suite green (2542 passed, 1 skipped), tsc + eslint clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: neil <neil@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 3 people

lib/storage/identity.ts

@@ -23,6 +23,7 @@ export type AccountLike = {
   refreshToken: string;
   addedAt?: number;
   lastUsed?: number;
+  enabled?: boolean;
 };
 
 const normalizeWorkspaceIdentityPart = (value: unknown): string | undefined =>
@@ -136,6 +137,37 @@ function pickNewestAccountIndex<T extends AccountLike>(
   return newest === candidate ? candidateIndex : existingIndex;
 }
 
+function isOrgLikeAccount<T extends AccountLike>(account: T): boolean {
+  return (
+    !!normalizeWorkspaceIdentityPart(account.organizationId) ||
+    account.accountIdSource === "org"
+  );
+}
+
+/**
+ * Resolve the merged `enabled` flag.
+ *
+ * Default is fail-closed: if either side is explicitly disabled, the merge is
+ * disabled. The ONE exception (issue #171): when exactly one side is an
+ * org-backed real account and the other is a token-source duplicate (a plugin
+ * re-login artifact), the org account's own `enabled` state governs. A disabled
+ * token duplicate must never silently disable the real account it collapses
+ * into — otherwise the canonical account becomes unroutable and unrecoverable.
+ */
+function resolveMergedEnabled<T extends AccountLike>(target: T, source: T): boolean | undefined {
+  const targetOrgLike = isOrgLikeAccount(target);
+  const sourceOrgLike = isOrgLikeAccount(source);
+  const targetTokenDup = !targetOrgLike && target.accountIdSource === "token";
+  const sourceTokenDup = !sourceOrgLike && source.accountIdSource === "token";
+
+  if (targetOrgLike && sourceTokenDup) return target.enabled;
+  if (sourceOrgLike && targetTokenDup) return source.enabled;
+
+  // Fail-closed for every other shape (same-identity, two real accounts, etc.).
+  if (target.enabled === false || source.enabled === false) return false;
+  return target.enabled ?? source.enabled;
+}
+
 function mergeAccountRecords<T extends AccountLike>(target: T, source: T): T {
   const newest = selectNewestAccount(target, source);
   const older = newest === target ? source : target;
@@ -147,6 +179,7 @@ function mergeAccountRecords<T extends AccountLike>(target: T, source: T): T {
     accountIdSource: target.accountIdSource ?? source.accountIdSource,
     accountLabel: target.accountLabel ?? source.accountLabel,
     email: target.email ?? source.email,
+    enabled: resolveMergedEnabled(target, source),
   };
 }
 

test/chaos/doctor-recovery-171-stress.test.ts

@@ -28,6 +28,14 @@ import {
 	clearRefreshedAccountsStaleState,
 	findDisabledTokenSourceDuplicates,
 } from "../../lib/accounts/stale-state.js";
+import {
+	loadAccounts,
+	saveAccounts,
+	setStoragePathDirect,
+} from "../../lib/storage.js";
+import { promises as fsp } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
 
 const FAMILY: ModelFamily = "codex";
 const NOW = 1_700_000_000_000;
@@ -334,3 +342,81 @@ describe("chaos/doctor-recovery — real manager + real recovery helpers (issue
 	});
 });
 
+describe("chaos/doctor-recovery — single-account + disabled dup, real save/load (issue #171)", () => {
+	let dir: string;
+
+	afterEach(async () => {
+		while (liveManagers.length > 0) {
+			liveManagers.pop()?.disposeShutdownHandler();
+		}
+		setStoragePathDirect(null);
+		if (dir) await fsp.rm(dir, { recursive: true, force: true });
+	});
+
+	it("recovers a lone org account whose disabled token-dup merged in", async () => {
+		dir = await fsp.mkdtemp(join(tmpdir(), "ralph171-"));
+		setStoragePathDirect(join(dir, "oc-codex-multi-auth-accounts.json"));
+		const FUTURE = Date.now() + 3_600_000;
+
+		// Reporter shape reduced to a single real account: an org account in
+		// auth-failure cooldown, plus a disabled token-source duplicate (same
+		// email) minted by a fresh re-login. Before the merge fix this collapsed
+		// to ONE disabled account that neither doctor nor health could recover.
+		await saveAccounts({
+			version: 3,
+			activeIndex: 0,
+			accounts: [
+				{
+					accountId: "org-AAA",
+					organizationId: "org-AAA",
+					accountIdSource: "org",
+					email: "user@example.com",
+					refreshToken: "OLD-refresh",
+					addedAt: 100,
+					lastUsed: 200,
+					coolingDownUntil: FUTURE,
+					cooldownReason: "auth-failure",
+				},
+				{
+					accountId: "uuid-fresh",
+					accountIdSource: "token",
+					email: "user@example.com",
+					refreshToken: "FRESH-refresh",
+					addedAt: 999,
+					lastUsed: 999,
+					enabled: false,
+				},
+			],
+		});
+
+		// Merge on load: ONE enabled org account, dark (cooldown active).
+		const loaded = await loadAccounts();
+		expect(loaded?.accounts).toHaveLength(1);
+		const acct = loaded!.accounts[0]!;
+		expect(acct.enabled).not.toBe(false);
+		const before = makeManager(loaded!);
+		expect(
+			before
+				.getSelectionExplainability(FAMILY, "gpt-5.4-mini", Date.now())
+				.filter((e) => e.eligible).length,
+		).toBe(0);
+
+		// codex-doctor --fix: refresh enabled accounts, clear stale state, persist.
+		const refreshable = loaded!.accounts.filter((a) => a.enabled !== false);
+		expect(refreshable).toHaveLength(1);
+		for (const a of refreshable) a.refreshToken = "NEW-" + a.refreshToken;
+		clearRefreshedAccountsStaleState(refreshable);
+		await saveAccounts(loaded!);
+
+		// After restart: pool is eligible again.
+		const after = await loadAccounts();
+		const mgr = makeManager(after!);
+		const eligible = mgr
+			.getSelectionExplainability(FAMILY, "gpt-5.4-mini", Date.now())
+			.filter((e) => e.eligible).length;
+		expect(eligible).toBe(1);
+		const selected = mgr.getCurrentOrNextForFamilyHybrid(FAMILY, "gpt-5.4-mini");
+		expect(selected?.organizationId).toBe("org-AAA");
+		expect(mgr.isAccountCoolingDown(selected as NonNullable<Active>)).toBe(false);
+	});
+});

test/storage.test.ts

@@ -316,6 +316,75 @@ describe("storage", () => {
       expect(loaded?.activeIndexByFamily?.codex).toBe(0);
     });
 
+    // Issue #171: a disabled token-source duplicate (a plugin re-login artifact)
+    // must NOT disable the real org account it collapses into during dedup.
+    it("keeps the org account enabled when a disabled token-source duplicate merges in (#171)", async () => {
+      const now = Date.now();
+      await saveAccounts({
+        version: 3,
+        activeIndex: 0,
+        accounts: [
+          {
+            accountId: "org-AAA",
+            organizationId: "org-AAA",
+            accountIdSource: "org",
+            email: "user@example.com",
+            refreshToken: "OLD-refresh",
+            addedAt: 100,
+            lastUsed: 200,
+            coolingDownUntil: now + 3_600_000,
+            cooldownReason: "auth-failure",
+          },
+          {
+            accountId: "uuid-fresh",
+            accountIdSource: "token",
+            email: "user@example.com",
+            refreshToken: "FRESH-refresh",
+            addedAt: 999,
+            lastUsed: 999,
+            enabled: false,
+          },
+        ],
+      });
+      const loaded = await loadAccounts();
+      expect(loaded?.accounts).toHaveLength(1);
+      const survivor = loaded!.accounts[0]!;
+      expect(survivor.organizationId).toBe("org-AAA");
+      // The real account must remain routable: NOT disabled by the dup.
+      expect(survivor.enabled).not.toBe(false);
+      // And it carries the fresh token from the newer (dup) record.
+      expect(survivor.refreshToken).toBe("FRESH-refresh");
+    });
+
+    it("preserves fail-closed when a same-identity record was user-disabled (#171 guard)", async () => {
+      await saveAccounts({
+        version: 3,
+        activeIndex: 0,
+        accounts: [
+          { accountId: "org-X", organizationId: "org-X", accountIdSource: "org", email: "u@e.com", refreshToken: "RT", addedAt: 100, lastUsed: 100, enabled: false },
+          { accountId: "org-X", organizationId: "org-X", accountIdSource: "org", email: "u@e.com", refreshToken: "RT", addedAt: 200, lastUsed: 200, enabled: true },
+        ],
+      });
+      const loaded = await loadAccounts();
+      expect(loaded?.accounts).toHaveLength(1);
+      // User intent wins: a genuinely disabled real account stays disabled.
+      expect(loaded!.accounts[0]!.enabled).toBe(false);
+    });
+
+    it("keeps two distinct real org accounts both enabled (no merge regression)", async () => {
+      await saveAccounts({
+        version: 3,
+        activeIndex: 0,
+        accounts: [
+          { accountId: "org-1", organizationId: "org-1", accountIdSource: "org", email: "a@e.com", refreshToken: "R1", addedAt: 100, lastUsed: 100, enabled: true },
+          { accountId: "org-2", organizationId: "org-2", accountIdSource: "org", email: "b@e.com", refreshToken: "R2", addedAt: 100, lastUsed: 100, enabled: true },
+        ],
+      });
+      const loaded = await loadAccounts();
+      expect(loaded?.accounts).toHaveLength(2);
+      expect(loaded!.accounts.every((a) => a.enabled !== false)).toBe(true);
+    });
+
     it("retains per-account rate-limit and cooldown metadata through save/load round-trip", async () => {
       await saveAccounts({
         version: 3,