staging: integrate PR #413 fix/remove-account-pointer-dangle

# Conflicts:
#	lib/accounts.ts
#	test/accounts.test.ts

Author: ndycode

lib/accounts.ts

@@ -1289,12 +1289,40 @@ export class AccountManager {
 		return waitTimes.length > 0 ? Math.min(...waitTimes) : 0;
 	}
 
+	/**
+	 * Walks forward from `start` (inclusive) looking for the next enabled
+	 * account, wrapping around at most once. Returns -1 when every account in
+	 * the pool is disabled or the pool is empty.
+	 */
+	private findNextEnabled(start: number): number {
+		const count = this.accounts.length;
+		if (count === 0) return -1;
+		const base = ((start % count) + count) % count;
+		for (let step = 0; step < count; step++) {
+			const candidate = (base + step) % count;
+			const account = this.accounts[candidate];
+			if (account && account.enabled !== false) {
+				return candidate;
+			}
+		}
+		return -1;
+	}
+
 	removeAccount(account: ManagedAccount): boolean {
 		const idx = this.accounts.indexOf(account);
 		if (idx < 0) {
 			return false;
 		}
 
+		// Snapshot family pointers before splice so we can distinguish "was
+		// pointing at the removed account" from "was pointing past it".
+		const priorCursor: Record<ModelFamily, number> = {} as Record<ModelFamily, number>;
+		const priorActive: Record<ModelFamily, number> = {} as Record<ModelFamily, number>;
+		for (const family of MODEL_FAMILIES) {
+			priorCursor[family] = this.cursorByFamily[family];
+			priorActive[family] = this.currentAccountIndexByFamily[family];
+		}
+
 		this.accounts.splice(idx, 1);
 		this.accounts.forEach((acc, index) => {
 			acc.index = index;
@@ -1309,25 +1337,39 @@ export class AccountManager {
 		}
 
 		for (const family of MODEL_FAMILIES) {
-			if (this.cursorByFamily[family] > idx) {
-				this.cursorByFamily[family] = Math.max(
-					0,
-					this.cursorByFamily[family] - 1,
-				);
+			// Cursor: shift down if it was past the removed index, then normalize
+			// into [0, length). If the cursor was pointing AT the removed slot
+			// we keep the same numeric position which now references the
+			// successor account (post-splice).
+			let cursor = priorCursor[family];
+			if (cursor > idx) {
+				cursor = Math.max(0, cursor - 1);
 			}
-		}
-		for (const family of MODEL_FAMILIES) {
-			this.cursorByFamily[family] =
-				this.cursorByFamily[family] % this.accounts.length;
-		}
-
-		for (const family of MODEL_FAMILIES) {
-			if (this.currentAccountIndexByFamily[family] > idx) {
-				this.currentAccountIndexByFamily[family] -= 1;
+			if (cursor >= this.accounts.length) {
+				cursor = 0;
 			}
-			if (this.currentAccountIndexByFamily[family] >= this.accounts.length) {
-				this.currentAccountIndexByFamily[family] = -1;
+			if (cursor < 0) {
+				cursor = 0;
+			}
+			this.cursorByFamily[family] = cursor;
+
+			// Active pointer: preserve pre-PR behavior for pointers strictly
+			// past the removed index (just shift down). When the pointer was
+			// AT the removed slot (or is now dangling off the end after
+			// splice), advance to the next enabled account instead of
+			// defaulting to -1. Fall back to -1 only when every remaining
+			// account is disabled or the pool is empty.
+			let active = priorActive[family];
+			if (active > idx) {
+				active -= 1;
+			} else if (active === idx) {
+				// Same numeric position now hosts the successor account.
+				active = this.findNextEnabled(Math.min(idx, this.accounts.length - 1));
+			}
+			if (active >= this.accounts.length) {
+				active = this.findNextEnabled(0);
 			}
+			this.currentAccountIndexByFamily[family] = active;
 		}
 
 		return true;

test/accounts.test.ts

@@ -31,6 +31,7 @@ import {
 	setStoragePathState,
 } from "../lib/storage/path-state.js";
 import type { OAuthAuthDetails } from "../lib/types.js";
+import { MODEL_FAMILIES } from "../lib/prompts/codex.js";
 
 vi.mock("../lib/storage.js", async (importOriginal) => {
 	const actual = await importOriginal<typeof import("../lib/storage.js")>();
@@ -2737,6 +2738,157 @@ describe("AccountManager", () => {
 			expect(manager.getActiveIndexForFamily("codex")).toBe(-1);
 		});
 	});
+		describe("active-account pointer dangle (audit HIGH-3)", () => {
+			it("advances active pointer to next enabled account when active is at last slot", () => {
+				const now = Date.now();
+				const stored = {
+					version: 3 as const,
+					activeIndex: 2,
+					activeIndexByFamily: { codex: 2 },
+					accounts: [
+						{ refreshToken: "token-1", addedAt: now, lastUsed: now },
+						{ refreshToken: "token-2", addedAt: now, lastUsed: now },
+						{ refreshToken: "token-3", addedAt: now, lastUsed: now },
+					],
+				};
+
+				const manager = new AccountManager(undefined, stored as never);
+				const active = manager.getCurrentAccountForFamily("codex");
+				expect(active?.refreshToken).toBe("token-3");
+
+				// Remove the currently active account that lives at the last slot.
+				manager.removeAccount(active!);
+
+				expect(manager.getAccountCount()).toBe(2);
+				// Pointer must reference a valid enabled account, not -1.
+				const after = manager.getActiveIndexForFamily("codex");
+				expect(after).toBeGreaterThanOrEqual(0);
+				expect(after).toBeLessThan(2);
+				const newActive = manager.getCurrentAccountForFamily("codex");
+				expect(newActive).not.toBeNull();
+				expect(newActive?.enabled).not.toBe(false);
+			});
+
+			it("advances active pointer to next enabled when active is at middle slot", () => {
+				const now = Date.now();
+				const stored = {
+					version: 3 as const,
+					activeIndex: 1,
+					activeIndexByFamily: { codex: 1 },
+					accounts: [
+						{ refreshToken: "token-1", addedAt: now, lastUsed: now },
+						{ refreshToken: "token-2", addedAt: now, lastUsed: now },
+						{ refreshToken: "token-3", addedAt: now, lastUsed: now },
+					],
+				};
+
+				const manager = new AccountManager(undefined, stored as never);
+				const active = manager.getCurrentAccountForFamily("codex");
+				expect(active?.refreshToken).toBe("token-2");
+
+				manager.removeAccount(active!);
+
+				expect(manager.getAccountCount()).toBe(2);
+				// After removing token-2 from index 1, token-3 slides into index 1.
+				// The pointer should now reference token-3 (the successor).
+				const newActive = manager.getCurrentAccountForFamily("codex");
+				expect(newActive?.refreshToken).toBe("token-3");
+				expect(manager.getActiveIndexForFamily("codex")).toBe(1);
+			});
+
+			it("yields no routable account when every remaining account is disabled", () => {
+				const now = Date.now();
+				const stored = {
+					version: 3 as const,
+					activeIndex: 2,
+					activeIndexByFamily: { codex: 2 },
+					accounts: [
+						{ refreshToken: "token-1", addedAt: now, lastUsed: now, enabled: false },
+						{ refreshToken: "token-2", addedAt: now, lastUsed: now, enabled: false },
+						{ refreshToken: "token-3", addedAt: now, lastUsed: now, enabled: true },
+					],
+				};
+
+				const manager = new AccountManager(undefined, stored as never);
+				const active = manager.getCurrentAccountForFamily("codex");
+				expect(active?.refreshToken).toBe("token-3");
+
+				// Remove the last enabled account; remaining pool is all-disabled.
+				manager.removeAccount(active!);
+
+				expect(manager.getAccountCount()).toBe(2);
+				// getCurrentAccountForFamily returns null whenever the pointer
+				// references a disabled slot or is -1, which is the observable
+				// contract callers rely on for "no routable account".
+				expect(manager.getCurrentAccountForFamily("codex")).toBeNull();
+			});
+
+			it("sets active pointer to -1 when pool becomes empty", () => {
+				const now = Date.now();
+				const stored = {
+					version: 3 as const,
+					activeIndex: 0,
+					accounts: [
+						{ refreshToken: "token-1", addedAt: now, lastUsed: now },
+					],
+				};
+
+				const manager = new AccountManager(undefined, stored);
+				const account = manager.getCurrentAccount()!;
+				manager.removeAccount(account);
+
+				expect(manager.getAccountCount()).toBe(0);
+				expect(manager.getActiveIndexForFamily("codex")).toBe(-1);
+			});
+
+			it("does not perturb unrelated family pointers", () => {
+				const now = Date.now();
+				const stored = {
+					version: 3 as const,
+					activeIndex: 0,
+					activeIndexByFamily: { codex: 2 },
+					accounts: [
+						{ refreshToken: "token-1", addedAt: now, lastUsed: now },
+						{ refreshToken: "token-2", addedAt: now, lastUsed: now },
+						{ refreshToken: "token-3", addedAt: now, lastUsed: now },
+					],
+				};
+
+				const manager = new AccountManager(undefined, stored as never);
+				// Pick a family other than codex that exists in MODEL_FAMILIES.
+				const otherFamilies = MODEL_FAMILIES.filter((f) => f !== "codex");
+				if (otherFamilies.length === 0) {
+					// Defensive: single-family config means this test reduces to no-op.
+					return;
+				}
+				const otherFamily = otherFamilies[0]!;
+
+				// Drive the other-family pointer to index 0 via its own rotation.
+				// Directly manipulate via getActiveIndexForFamily/setActiveIndex since
+				// setActiveIndex mirrors all families; instead rotate the target
+				// family by calling getCurrentOrNextForFamily once.
+				const viaRotation = manager.getCurrentOrNextForFamily(otherFamily);
+				expect(viaRotation).not.toBeNull();
+				const otherBefore = manager.getActiveIndexForFamily(otherFamily);
+				expect(otherBefore).toBeGreaterThanOrEqual(0);
+				expect(otherBefore).toBeLessThan(3);
+
+				// Remove the codex-active account (last slot).
+				const codexActive = manager.getCurrentAccountForFamily("codex");
+				expect(codexActive?.refreshToken).toBe("token-3");
+				manager.removeAccount(codexActive!);
+
+				// The other family's pointer must still reference a valid enabled
+				// account in the new pool; it must not dangle to -1 just because we
+				// removed from codex.
+				const otherAfter = manager.getActiveIndexForFamily(otherFamily);
+				expect(otherAfter).toBeGreaterThanOrEqual(0);
+				expect(otherAfter).toBeLessThan(2);
+				const otherAccount = manager.getCurrentAccountForFamily(otherFamily);
+				expect(otherAccount).not.toBeNull();
+				expect(otherAccount?.enabled).not.toBe(false);
+			});
+		});
 
 	describe("flushPendingSave", () => {
 		it("flushes pending debounced save", async () => {