fix(accounts): harden removeAccount pointer normalization

Addresses oracle audit HIGH-3 finding (flagged as pre-existing during
PR #399 review). When removing the currently active account while
other accounts remain in the pool, pointers now advance to the next
enabled account instead of defaulting to -1.

Normalizes activeIndex, currentAccountIndexByFamily, and cursorByFamily
consistently via a shared findNextEnabled helper. The helper walks
forward (with modulo wrap) from a search origin and only returns -1
when every remaining account is disabled or the pool is empty.

Tests cover: remove-at-last, remove-at-middle, remove-with-all-others-disabled,
remove-until-empty, and multi-family isolation.

Source: .sisyphus/notepads/phase1-audit/reports/pr399.json HIGH-3

Author: ndycode

lib/accounts.ts

@@ -1094,12 +1094,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;
@@ -1114,21 +1142,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

@@ -27,6 +27,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")>();
@@ -2618,6 +2619,162 @@ describe("AccountManager", () => {
       expect(manager.getAccountCount()).toBe(0);
       expect(manager.getActiveIndexForFamily("codex")).toBe(-1);
     });
+
+    // Regression suite for audit finding HIGH-3 (PR #399 audit report):
+    // removeAccount previously set activeIndex to -1 whenever the active
+    // account was removed from the last array slot (even when other enabled
+    // accounts remained). Pointer must advance to the next enabled account.
+    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", () => {