From 854a25d6303c1b549a2d8bb2832a354d2d67986d Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:00:49 -0500 Subject: [PATCH 01/15] test(isolation): stop the suite writing to the real account store The suite drives real AccountManager instances and calls loadAccounts / saveAccounts without overriding storage, so `npm test` resolved ~/.opencode/oc-codex-multi-auth-accounts.json against the developer's own home and wrote fixtures over it. On 2026-09-17 that replaced a live five-account ChatGPT pool with two test records (accountId "test-account" and "new-import", addedAt 1ms and 2ms past the epoch) and took the running opencode fleet down for roughly 40 minutes: live processes reported "No Codex accounts configured. Run `opencode auth login`." and "All 2 account(s) are rate-limited". Recovery needed a 15-day-old backup, and five of the seven accounts it restored came back with dead refresh tokens. The redirect has to be `test.env` rather than a setupFiles entry. vitest applies test.env in the worker before it imports any test module, while a setup file runs once the module graph is already loading, which is too late for lib/config.ts, lib/accounts/recovery.ts, lib/logger.ts, lib/prompts/codex.ts, lib/prompts/opencode-codex.ts and lib/auto-update-checker.ts: each captures homedir() at module scope. Verified empirically rather than assumed. With the real HOME inherited on the command line, LOG_DIR still resolves inside the sandbox. A redirect alone is one refactor away from lapsing silently, so the storage layer also fails closed. Under VITEST, any account-storage write, unlink, or lock-sidecar probe that resolves inside the real home throws TEST_HOME_ESCAPE instead of proceeding. The check compares against os.userInfo().homedir, which reads the passwd entry rather than $HOME and so still names the real home after the redirect; the sandbox cannot spoof it. It is inert outside vitest. The guard runs before `acquireOrDetectLock`, not inside the try that wraps it, because that probe writes a lock sidecar next to the accounts file and would therefore touch the real store even on a pure read, and because the surrounding catch would swallow the refusal. test/paths.test.ts needed fixing as a consequence rather than by coincidence. Its two lookalike-prefix cases build a sibling of an allowed root and require it to be outside all three roots; with HOME under tmpdir(), every sibling of home is a child of tmpdir(), which resolvePath legitimately allows, so the assertions stopped throwing. Mocking homedir() and tmpdir() to fixed unrelated roots makes them independent of where the real HOME points. They pass with HOME both inside and outside tmpdir(), which also means the isolated home can keep living under tmpdir(). AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- lib/storage/load-save.ts | 38 +++++++++++++++++++- lib/storage/paths.ts | 2 +- test/paths.test.ts | 19 ++++++++++ test/test-home-isolation.test.ts | 62 ++++++++++++++++++++++++++++++++ vitest.config.ts | 24 +++++++++++++ 5 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 test/test-home-isolation.test.ts diff --git a/lib/storage/load-save.ts b/lib/storage/load-save.ts index fddcbe71..ef20c727 100644 --- a/lib/storage/load-save.ts +++ b/lib/storage/load-save.ts @@ -25,7 +25,7 @@ import { AnyAccountStorageSchema, getValidationErrors } from "../schemas.js"; import { renameWithWindowsRetry } from "./atomic-write.js"; import { formatStorageErrorHint, StorageError } from "./errors.js"; import { normalizeAccountStorage } from "./normalize.js"; -import { getConfigDir } from "./paths.js"; +import { getConfigDir, isWithinDirectory } from "./paths.js"; import { getCurrentLegacyProjectStoragePath, getCurrentProjectRoot, @@ -142,6 +142,10 @@ async function checkWorktreeLockForCurrentStorage( }); return; } + // Before the probe, not inside the try: `acquireOrDetectLock` writes a lock + // sidecar next to the accounts file, so a leaked HOME would touch the real + // store here even on a pure read, and this function's catch would hide it. + assertTestRunNeverTouchesRealHome(path); try { const result = await acquireOrDetectLock(path); if (!result.acquired && result.foreign) { @@ -171,6 +175,36 @@ async function checkWorktreeLockForCurrentStorage( } } +/** + * Refuse to mutate account storage inside the developer's real home while the + * test suite is running. + * + * `vitest.config.ts` redirects HOME to a sandbox before any module loads, but a + * test that restores the captured real HOME, or a future regression in that + * config, would otherwise write fixtures straight over live ChatGPT + * credentials. `os.userInfo()` reads the passwd entry instead of `$HOME`, so it + * still names the real home after the redirect and gives the check something + * the sandbox cannot spoof. Inert outside vitest. + */ +function assertTestRunNeverTouchesRealHome(path: string): void { + if (!process.env.VITEST) return; + + let realHome: string; + try { + realHome = os.userInfo().homedir; + } catch { + return; + } + if (!realHome || !isWithinDirectory(realHome, path)) return; + + throw new StorageError( + `Refusing to write account storage inside the real home directory during a test run: ${path}`, + "TEST_HOME_ESCAPE", + path, + "A test resolved account storage against the developer's real home. Point HOME at a temp directory for the whole vitest process (see vitest.config.ts) instead of overriding it per test.", + ); +} + async function ensureGitignore(storagePath: string): Promise { if (!getCurrentStoragePath()) return; @@ -537,6 +571,7 @@ async function loadAccountsInternal( * Callers must already be inside withStorageLock when using this helper directly. */ async function writeAccountsToPathUnlocked(path: string, storage: AccountStorageV3): Promise { + assertTestRunNeverTouchesRealHome(path); const uniqueSuffix = `${Date.now()}.${Math.random().toString(36).slice(2, 8)}`; const tempPath = `${path}.${uniqueSuffix}.tmp`; @@ -759,6 +794,7 @@ export async function clearAccounts(): Promise { let jsonCleared = true; try { const path = getStoragePath(); + assertTestRunNeverTouchesRealHome(path); await fs.unlink(path); } catch (error) { const code = (error as NodeJS.ErrnoException).code; diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index 1dc45bc6..ad2e11ce 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -81,7 +81,7 @@ function normalizePathForComparison(filePath: string): string { return process.platform === "win32" ? resolvedPath.toLowerCase() : resolvedPath; } -function isWithinDirectory(baseDir: string, targetPath: string): boolean { +export function isWithinDirectory(baseDir: string, targetPath: string): boolean { const normalizedBase = normalizePathForComparison(baseDir); const normalizedTarget = normalizePathForComparison(targetPath); const rel = relative(normalizedBase, normalizedTarget); diff --git a/test/paths.test.ts b/test/paths.test.ts index ca215ec8..c3a88ceb 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -2,10 +2,29 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { homedir, tmpdir } from "node:os"; import path from "node:path"; +// `resolvePath` guards against a path that merely shares a string prefix with +// an allowed root, so the lookalike cases below build a sibling of one root and +// require it to be outside all three. Against the real layout that only holds +// by accident: the suite runs with HOME redirected under the temp dir, which +// makes every sibling of home a child of tmpdir() and turns the assertion into +// a no-op. Fixed roots keep these cases meaningful wherever HOME points. +// +// This mock and the isolated HOME in `vitest.config.ts` are load-bearing +// together. Removing the mock as redundant silently turns both lookalike +// assertions into no-ops rather than failing them. +const FAKE_HOME = path.resolve("/fake-storage-paths-home"); +const FAKE_TMPDIR = path.resolve("/fake-storage-paths-tmp"); + vi.mock("node:fs", () => ({ existsSync: vi.fn(), })); +vi.mock("node:os", async (importOriginal) => ({ + ...(await importOriginal()), + homedir: () => FAKE_HOME, + tmpdir: () => FAKE_TMPDIR, +})); + import { existsSync } from "node:fs"; import { getConfigDir, diff --git a/test/test-home-isolation.test.ts b/test/test-home-isolation.test.ts new file mode 100644 index 00000000..ec58263b --- /dev/null +++ b/test/test-home-isolation.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect } from "vitest"; +import { homedir, userInfo } from "node:os"; +import { isAbsolute, relative, resolve } from "node:path"; +import { LOG_DIR } from "../lib/logger.js"; +import { ACCOUNTS_FILE_NAME } from "../lib/constants.js"; +import { saveAccounts } from "../lib/storage/load-save.js"; +import { getConfigDir } from "../lib/storage/paths.js"; +import { + getStoragePath, + setStoragePath, + setStoragePathDirect, +} from "../lib/storage/state.js"; + +function realUserHome(): string { + return userInfo().homedir; +} + +function isUnder(baseDir: string, targetPath: string): boolean { + const rel = relative(resolve(baseDir), resolve(targetPath)); + return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); +} + +describe("test home isolation", () => { + it("redirects homedir() away from the real user home", () => { + expect(process.env.OC_CODEX_TEST_HOME).toBeTruthy(); + expect(homedir()).toBe(process.env.OC_CODEX_TEST_HOME); + expect(isUnder(realUserHome(), homedir())).toBe(false); + }); + + it("keeps every resolved account storage path out of the real user home", () => { + const real = realUserHome(); + + setStoragePath(null); + expect(isUnder(real, getStoragePath())).toBe(false); + + setStoragePath(process.cwd()); + expect(isUnder(real, getStoragePath())).toBe(false); + + setStoragePath(null); + expect(isUnder(real, getConfigDir())).toBe(false); + }); + + // LOG_DIR is captured at module scope from homedir(), so it only lands in + // the sandbox if the override beat the import. That is the property a + // setupFiles entry cannot provide and this whole mechanism exists for. + it("beats module-scope homedir() capture", () => { + expect(isUnder(realUserHome(), LOG_DIR)).toBe(false); + expect(isUnder(process.env.OC_CODEX_TEST_HOME as string, LOG_DIR)).toBe(true); + }); + + it("refuses to write account storage that escapes into the real home", async () => { + const escaped = resolve(realUserHome(), ".opencode", ACCOUNTS_FILE_NAME); + setStoragePathDirect(escaped); + try { + await expect( + saveAccounts({ version: 3, accounts: [], activeIndex: 0 }), + ).rejects.toMatchObject({ code: "TEST_HOME_ESCAPE" }); + } finally { + setStoragePathDirect(null); + } + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 5499527f..4240487d 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,9 +1,33 @@ import { defineConfig } from 'vitest/config'; +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +/** + * Per-run throwaway home. The suite writes real account storage, so without + * this a `npm test` resolves `~/.opencode/oc-codex-multi-auth-accounts.json` + * against the developer's actual home and overwrites live ChatGPT credentials + * with fixtures. + * + * This must be `test.env`, not a `setupFiles` entry: vitest applies `test.env` + * before the worker imports any test module, and `lib/config.ts`, + * `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module + * scope, so anything later than import time is too late for them. + */ +const isolatedHome = + process.env.OC_CODEX_TEST_HOME ?? + mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); +process.env.OC_CODEX_TEST_HOME = isolatedHome; export default defineConfig({ test: { globals: true, environment: 'node', + env: { + HOME: isolatedHome, + USERPROFILE: isolatedHome, + OC_CODEX_TEST_HOME: isolatedHome, + }, include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From d9f5f81db8fff99c99104897d83d71a6a42fac2d Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:06:32 -0500 Subject: [PATCH 02/15] test(vitest): stop index.ts suites timing out on their own import test/index-retry.test.ts fails two of its six cases on a 5s timeout under full-suite load, and has done so on a clean checkout of upstream main. Nothing in those cases is slow: they already run on fake timers, and the whole file finishes its assertions in well under a second once loaded. What exceeds the timeout is the import. Four suites import the real `index.ts`, and the first one scheduled pays the transform of a 4900-line entry plus its dependency graph. Measured on an idle machine: 3.2s-6.7s for the cold import, ~400ms for a warm re-import after `vi.resetModules()`. With vitest's 5s default that is a coin flip before any test body runs, and CPU contention from the rest of the suite decides it. The floor is raised for the whole run rather than for one file, because a per-file timeout only moves the hazard to whichever of the four suites is scheduled first next time. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- vitest.config.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vitest.config.ts b/vitest.config.ts index 4240487d..41c0fe84 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -28,6 +28,12 @@ export default defineConfig({ USERPROFILE: isolatedHome, OC_CODEX_TEST_HOME: isolatedHome, }, + // Four suites import the real `index.ts`, and the first one scheduled pays + // the transform of a 4900-line entry plus its dependency graph: measured at + // 3.2s-6.7s on an idle machine, against a 5s default. Whichever suite loses + // that race times out under full-suite CPU contention, which is flakiness in + // the harness rather than in any assertion (a warm re-import costs ~400ms). + testTimeout: 15_000, include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From e131d69fd80e60743efb33655fa0e43f2d306e30 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:08:03 -0500 Subject: [PATCH 03/15] fix(retry): charge the rate-limit budget by wait length, not by attempt When every account is rate-limited the request waits and retries, and that loop is gated on `consumeRetryBudget("rateLimitGlobal", ...)`. The budget is tiny - 1 conservative, 3 balanced, 10 aggressive - and a wait cost one unit however briefly it blocked. Three consecutive 400ms waits therefore exhausted the default and the request hard-failed with "All N account(s) are rate-limited", when one more second of waiting would have served it. Nothing else in that gate can end the loop. `retryAllAccountsMaxRetries` defaults to Infinity, and `retryAllAccountsMaxWaitMs` defaults to 0, which the gate reads as uncapped - so the budget is the only term that can go false. The tracker is constructed per request, so this is not budget carried over from an earlier one either. Observed in production. Three independent sessions on a healthy 7-account pool died after roughly nine minutes each, reporting a true reset four hours out: `All 7 account(s) are rate-limited. Try again in 4h 0m 0s`. Three units against a real 4h wait should have been about twelve hours of sleeping. The accounts had just had their quota stamps cleared, so each attempt looked viable, went out, took a real 429, slept briefly, and repeated until the budget was gone. A unit now measures blocking time rather than attempts. `consumeWait` charges a full unit for a wait at or above RETRY_WAIT_BUDGET_UNIT_MS (5s), so a multi-hour block stays governed exactly as it was, and accumulates shorter waits on a per-bucket carry so a burst of sub-second probes is effectively free. An exhausted bucket still refuses a free wait: the carry is what bounds how long short waits can loop, and without that check the loop would never terminate. `consume` is unchanged and remains the default, so every other retry class keeps counting attempts. Only a caller that passes a wait is charged by duration, and the metrics counter follows the tracker's own usage rather than assuming one unit per call, so a free wait no longer reports budget it never spent. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 16 +++++++- lib/request/retry-budget.ts | 42 ++++++++++++++++++++ test/retry-budget.test.ts | 79 +++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 2 deletions(-) diff --git a/index.ts b/index.ts index 1cb71259..99b35279 100644 --- a/index.ts +++ b/index.ts @@ -2396,9 +2396,20 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const consumeRetryBudget = ( bucket: RetryBudgetClass, reason: string, + waitMs?: number, ): boolean => { - if (retryBudget.consume(bucket)) { - runtimeMetrics.retryBudgetUsage[bucket] += 1; + // Pass the wait so the charge scales with how long the retry + // blocks. Metrics follow the tracker's own counter rather than + // assuming one unit, or a free sub-second wait would report + // budget it never spent. + const usedBefore = retryBudget.getUsage()[bucket]; + const granted = + waitMs === undefined + ? retryBudget.consume(bucket) + : retryBudget.consumeWait(bucket, waitMs); + if (granted) { + runtimeMetrics.retryBudgetUsage[bucket] += + retryBudget.getUsage()[bucket] - usedBefore; return true; } runtimeMetrics.retryBudgetExhaustions += 1; @@ -3795,6 +3806,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { consumeRetryBudget( "rateLimitGlobal", `All accounts rate-limited wait ${waitMs}ms`, + waitMs, ) ) { const countdownMessage = `All ${count} account(s) rate-limited. Waiting`; diff --git a/lib/request/retry-budget.ts b/lib/request/retry-budget.ts index d1ea2beb..a86d01bf 100644 --- a/lib/request/retry-budget.ts +++ b/lib/request/retry-budget.ts @@ -43,6 +43,18 @@ const PROFILE_LIMITS: Record = { }, }; +/** + * How much blocking one budget unit buys when a retry is charged through + * {@link RetryBudgetTracker.consumeWait}. + * + * The budgets are small (1/3/10) and were being charged one unit per wait + * regardless of length, so three consecutive sub-second waits exhausted the + * default and hard-failed a request that one more second would have served. + * Waiting is only expensive in proportion to the time it costs the caller, so + * that is what a unit now measures. + */ +export const RETRY_WAIT_BUDGET_UNIT_MS = 5_000; + const RETRY_BUDGET_CLASSES: RetryBudgetClass[] = [ "authRefresh", "network", @@ -87,12 +99,42 @@ function createUsedCounters(): RetryBudgetLimits { export class RetryBudgetTracker { private readonly used: RetryBudgetLimits = createUsedCounters(); + private readonly waitCarryMs: RetryBudgetLimits = createUsedCounters(); private readonly limits: RetryBudgetLimits; constructor(limits: RetryBudgetLimits) { this.limits = { ...limits }; } + /** + * Charge a retry that blocks for `waitMs` against a bucket, in proportion to + * how long it blocks. + * + * A wait of {@link RETRY_WAIT_BUDGET_UNIT_MS} or longer costs a full unit, + * so a multi-hour block stays governed exactly as before. Shorter waits + * accumulate on a per-bucket carry and only cost a unit once they have added + * up to one, so a burst of sub-second waits is effectively free. + * + * An exhausted bucket refuses even a free wait: the carry bounds how long + * short waits can loop, and without that check they would loop forever once + * the budget ran out. + */ + consumeWait(bucket: RetryBudgetClass, waitMs: number): boolean { + if (this.getRemaining(bucket) <= 0) return false; + + const wait = Number.isFinite(waitMs) && waitMs > 0 ? waitMs : 0; + if (wait >= RETRY_WAIT_BUDGET_UNIT_MS) return this.consume(bucket); + + const carried = this.waitCarryMs[bucket] + wait; + if (carried < RETRY_WAIT_BUDGET_UNIT_MS) { + this.waitCarryMs[bucket] = carried; + return true; + } + + this.waitCarryMs[bucket] = carried - RETRY_WAIT_BUDGET_UNIT_MS; + return this.consume(bucket); + } + consume(bucket: RetryBudgetClass): boolean { const limit = this.limits[bucket]; if (!Number.isFinite(limit)) { diff --git a/test/retry-budget.test.ts b/test/retry-budget.test.ts index 8f3e62fe..8d67dba7 100644 --- a/test/retry-budget.test.ts +++ b/test/retry-budget.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { RetryBudgetTracker, resolveRetryBudgetLimits, + RETRY_WAIT_BUDGET_UNIT_MS, type RetryBudgetLimits, } from "../lib/request/retry-budget.js"; @@ -72,6 +73,84 @@ describe("retry-budget", () => { expect(tracker.getUsage().authRefresh).toBe(1); }); + describe("consumeWait", () => { + const balanced = () => new RetryBudgetTracker(resolveRetryBudgetLimits("balanced")); + + it("does not charge the budget for a burst of sub-second waits", () => { + const tracker = balanced(); + + // The production regression: three consecutive 400ms waits spent the + // whole default budget and hard-failed the request. + for (let i = 0; i < 3; i++) { + expect(tracker.consumeWait("rateLimitGlobal", 400)).toBe(true); + } + expect(tracker.getUsage().rateLimitGlobal).toBe(0); + expect(tracker.getRemaining("rateLimitGlobal")).toBe(3); + + for (let i = 0; i < 27; i++) { + expect(tracker.consumeWait("rateLimitGlobal", 400)).toBe(true); + } + }); + + it("charges a full unit for a wait at or above the unit length", () => { + const tracker = balanced(); + + expect(tracker.consumeWait("rateLimitGlobal", RETRY_WAIT_BUDGET_UNIT_MS)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(true); + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(3); + + // A long wait stays governed: the fourth exceeds the balanced budget. + expect(tracker.consumeWait("rateLimitGlobal", 6 * 60 * 60 * 1000)).toBe(false); + }); + + it("accumulates short waits into whole units", () => { + const tracker = balanced(); + const waitMs = RETRY_WAIT_BUDGET_UNIT_MS / 10; + + for (let i = 0; i < 10; i++) { + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + } + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + }); + + it("refuses free short waits once the bucket is exhausted", () => { + const tracker = balanced(); + + for (let i = 0; i < 3; i++) { + expect(tracker.consumeWait("rateLimitGlobal", RETRY_WAIT_BUDGET_UNIT_MS)).toBe(true); + } + + // Without this the carry would grant sub-unit waits forever and the + // retry loop could never terminate. + expect(tracker.consumeWait("rateLimitGlobal", 1)).toBe(false); + expect(tracker.consumeWait("rateLimitGlobal", 0)).toBe(false); + }); + + it("keeps per-bucket carries independent", () => { + const tracker = balanced(); + const waitMs = RETRY_WAIT_BUDGET_UNIT_MS / 2; + + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + expect(tracker.consumeWait("rateLimitShort", waitMs)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(0); + expect(tracker.getUsage().rateLimitShort).toBe(0); + + expect(tracker.consumeWait("rateLimitGlobal", waitMs)).toBe(true); + expect(tracker.getUsage().rateLimitGlobal).toBe(1); + expect(tracker.getUsage().rateLimitShort).toBe(0); + }); + + it("treats a zero-limit bucket as immediately exhausted", () => { + const tracker = new RetryBudgetTracker( + resolveRetryBudgetLimits("balanced", { rateLimitGlobal: 0 }), + ); + expect(tracker.consumeWait("rateLimitGlobal", 1)).toBe(false); + }); + }); + it("clones constructor limits to avoid external mutation", () => { const limits: RetryBudgetLimits = { authRefresh: 1, From db13030a8fc0a366edb00101d7653ec41da50be4 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:09:51 -0500 Subject: [PATCH 04/15] fix(rate-limit): re-probe upstream during a long all-accounts wait With the retry budget no longer spent on short waits, a request that finds every account blocked sleeps out the real reset, which can be hours or - `retryAllAccountsMaxRetries` defaults to Infinity - days. Its only wake-up was the accounts file changing on disk and the watcher swapping the cached manager. That covers a peer process clearing a block, and it covers `opencode auth login` adding an account mid-sleep, since both write that file. It does not cover a reset granted server-side: the backend restoring quota changes nothing locally, so the sleeper keeps sleeping against capacity that has already come back. A wait of a minute or more now re-probes upstream as well. The probe is the quota monitor's own `runNow`, which refreshes `/wham/usage` for every account and persists what it finds, a recovery included - and persisting drops the cached manager, which is what makes the enclosing retry loop re-resolve one that no longer reports a block. It starts a minute in and doubles to a quarter-hour ceiling, so a multi-day sleep costs a handful of usage requests rather than one per five-second countdown tick. A probe that throws is logged at debug and the wait continues. The next probe is scheduled from the moment a probe returns rather than from when it was due, so a slow usage request cannot leave `nextProbeAt` in the past and collapse the countdown sleep to zero. Both wake paths are covered end to end against a real request: one where usage reports the quota back with no file write at all, and one where a login adds a second account while the first stays blocked on disk. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 69 +++++++++++++++++++++++++-- test/accounts-live-reload.test.ts | 79 ++++++++++++++++++++++++++++++- 2 files changed, 142 insertions(+), 6 deletions(-) diff --git a/index.ts b/index.ts index 99b35279..c005291d 100644 --- a/index.ts +++ b/index.ts @@ -394,6 +394,15 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { let startupPreflightShown = false; let beginnerSafeModeEnabled = false; const MIN_BACKOFF_MS = 100; + // An all-accounts rate-limit wait can run for days, and the local accounts + // file is its only wake-up. A quota reset granted server-side leaves that file + // untouched, so such a wait would be slept straight through. Long waits + // therefore re-probe upstream: first after a minute, doubling to a quarter + // hour, so a multi-day sleep costs a handful of usage requests rather than one + // per countdown tick. + const UPSTREAM_REPROBE_MIN_WAIT_MS = 60_000; + const UPSTREAM_REPROBE_FIRST_DELAY_MS = 60_000; + const UPSTREAM_REPROBE_MAX_DELAY_MS = 15 * 60_000; const runtimeMetrics: RuntimeMetrics = { startedAt: Date.now(), @@ -2461,16 +2470,35 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { totalMs: number, message: string, intervalMs: number = 5000, + probeUpstream?: () => Promise, ): Promise => { const startTime = Date.now(); const endTime = startTime + totalMs; - + let probeDelayMs = UPSTREAM_REPROBE_FIRST_DELAY_MS; + let nextProbeAt = + probeUpstream && totalMs >= UPSTREAM_REPROBE_MIN_WAIT_MS + ? startTime + probeDelayMs + : Number.POSITIVE_INFINITY; + while (Date.now() < endTime) { if (cachedAccountManager !== accountManager) return; if (abortSignal?.aborted) { throw abortError(); } - + + if (probeUpstream && Date.now() >= nextProbeAt) { + if (await probeUpstream()) return; + if (cachedAccountManager !== accountManager) return; + if (abortSignal?.aborted) { + throw abortError(); + } + probeDelayMs = Math.min(probeDelayMs * 2, UPSTREAM_REPROBE_MAX_DELAY_MS); + // Measured from the end of the probe, so a slow usage + // request cannot schedule the next one in the past and + // collapse the countdown sleep below to zero. + nextProbeAt = Date.now() + probeDelayMs; + } + const remaining = Math.max(0, endTime - Date.now()); const waitLabel = formatWaitTime(remaining); await showToast( @@ -2478,8 +2506,8 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { "warning", { duration: Math.min(intervalMs + 1000, toastDurationMs) }, ); - - const sleepTime = Math.min(intervalMs, remaining); + + const sleepTime = Math.min(intervalMs, remaining, nextProbeAt - Date.now()); if (sleepTime > 0) { await sleep(sleepTime); } else { @@ -2488,6 +2516,32 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } }; + /** + * True when an all-accounts wait can stop early. + * + * `runNow` refreshes `/wham/usage` for every account and persists + * whatever it finds, so a reset that never touched local disk + * becomes visible here. Persisting a recovery also drops the cached + * manager, which is what makes the enclosing retry loop re-resolve + * one that no longer reports a block. + */ + const probeUpstreamBlockLifted = async (): Promise => { + try { + await quotaMonitor.runNow(); + } catch (error) { + logDebug( + `[${PLUGIN_NAME}] Upstream quota re-probe failed: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return false; + } + if (cachedAccountManager !== accountManager) return true; + const manager = accountManager; + if (!manager) return false; + return manager.getMinWaitTimeForFamily(modelFamily, model) === 0; + }; + let allRateLimitedRetries = 0; let emptyResponseRetries = 0; const attemptedUnsupportedFallbackModels = new Set(); @@ -3810,7 +3864,12 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { ) ) { const countdownMessage = `All ${count} account(s) rate-limited. Waiting`; - await sleepWithCountdown(addJitter(waitMs, 0.2), countdownMessage); + await sleepWithCountdown( + addJitter(waitMs, 0.2), + countdownMessage, + undefined, + probeUpstreamBlockLifted, + ); allRateLimitedRetries++; continue; } diff --git a/test/accounts-live-reload.test.ts b/test/accounts-live-reload.test.ts index 1998ded9..d903336a 100644 --- a/test/accounts-live-reload.test.ts +++ b/test/accounts-live-reload.test.ts @@ -9,6 +9,7 @@ const captured = vi.hoisted((): { context?: ToolContext; listener?: () => void; onWatch?: () => void; + onQuotaProbe?: () => Promise; reads: Promise[]; maxRetries?: number; } => ({ reads: [] })); @@ -32,7 +33,11 @@ vi.mock("../lib/tools/index.js", () => ({ createToolRegistry: (context: ToolContext) => { captured.context = context; return {}; }, })); vi.mock("../lib/quota-notifications.js", () => ({ - createQuotaMonitor: () => ({ start() {}, dispose() {} }), + createQuotaMonitor: () => ({ + start() {}, + dispose() {}, + runNow: async () => { await captured.onQuotaProbe?.(); }, + }), })); vi.mock("../lib/auto-update-checker.js", () => ({ checkAndNotify: vi.fn(async () => {}) })); vi.mock("../lib/config.js", async (original) => ({ @@ -91,6 +96,7 @@ describe("accounts live reload", () => { await fs.writeFile(path, JSON.stringify(storage(true))); captured.listener = undefined; captured.onWatch = undefined; + captured.onQuotaProbe = undefined; captured.reads.length = 0; captured.maxRetries = undefined; plugin = await Reflect.apply(OpenAIOAuthPlugin, undefined, [{ @@ -285,6 +291,77 @@ describe("accounts live reload", () => { await vi.advanceTimersByTimeAsync(5000); expect((await response).status).toBe(200); }); + it("wakes a long wait when an upstream re-probe finds the block lifted", async () => { + const manager = captured.context?.cachedAccountManagerRef.current; + if (!manager) throw new Error("Missing manager"); + const account = manager.getCurrentAccount(); + if (!account) throw new Error("Missing account"); + manager.markQuotaExhausted(account, Date.now() + 86_400_000, "gpt-5.1"); + vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("data: [DONE]\n\n", { + status: 200, headers: { "content-type": "text/event-stream" }, + })); + let enteredWait: () => void = () => {}; + const waiting = new Promise((resolve) => { enteredWait = resolve; }); + const minWait = manager.getMinWaitTimeForFamily.bind(manager); + vi.spyOn(manager, "getMinWaitTimeForFamily").mockImplementation((...args) => { + enteredWait(); + return minWait(...args); + }); + let probes = 0; + captured.onQuotaProbe = async () => { + probes += 1; + // What a server-side grant looks like: usage reports the quota back, + // the recovery is persisted, and the cached manager is dropped. The + // accounts file is never written by another process, so the watcher + // has nothing to fire on - this is the wake-up it cannot provide. + captured.context?.invalidateAccountManagerCache(); + }; + const response = request("https://api.openai.com/v1/responses", { + method: "POST", body: JSON.stringify({ model: "gpt-5.1", stream: true, input: [] }), + }); + await waiting; + for (let elapsed = 0; elapsed < 120_000 && probes === 0; elapsed += 5000) { + await vi.advanceTimersByTimeAsync(5000); + } + expect(probes).toBe(1); + await vi.advanceTimersByTimeAsync(5000); + expect((await response).status).toBe(200); + }); + it("wakes a waiting request when a login adds an account mid-sleep", async () => { + const manager = captured.context?.cachedAccountManagerRef.current; + if (!manager) throw new Error("Missing manager"); + const account = manager.getCurrentAccount(); + if (!account) throw new Error("Missing account"); + const blockedUntil = Date.now() + 86_400_000; + manager.markQuotaExhausted(account, blockedUntil, "gpt-5.1"); + vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("data: [DONE]\n\n", { + status: 200, headers: { "content-type": "text/event-stream" }, + })); + let enteredWait: () => void = () => {}; + const waiting = new Promise((resolve) => { enteredWait = resolve; }); + const minWait = manager.getMinWaitTimeForFamily.bind(manager); + vi.spyOn(manager, "getMinWaitTimeForFamily").mockImplementation((...args) => { + enteredWait(); + return minWait(...args); + }); + const response = request("https://api.openai.com/v1/responses", { + method: "POST", body: JSON.stringify({ model: "gpt-5.1", stream: true, input: [] }), + }); + await waiting; + const reloaded = nextReload(); + // The incumbent account stays blocked on disk, so the only thing that can + // end this wait is the account the login added. + await fs.writeFile(path, JSON.stringify({ ...storage(true), accounts: [ + { ...storage(true).accounts[0], quotaExhaustedUntil: blockedUntil }, + { accountId: "fresh-login", refreshToken: "fresh-refresh", accessToken: "fresh-access", + expiresAt: Date.now() + 86_400_000, enabled: true, addedAt: 2, lastUsed: 2 }, + ] })); + await tick(); + await settle(); + await reloaded; + await vi.advanceTimersByTimeAsync(5000); + expect((await response).status).toBe(200); + }); it("keeps externally cleared blocks cleared despite queued and late saves from the old manager", async () => { const previous = captured.context?.cachedAccountManagerRef.current; if (!previous) throw new Error("Missing manager"); From 6c4c355508b68d88c72d63d8ba1a9d5e8f860c58 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 04:12:13 -0500 Subject: [PATCH 05/15] fix(accounts): never let a failed load empty a working account pool `loadAccounts()` reports a read or parse failure exactly as it reports an absent file - by returning null. `AccountState.initializeFromStorage()` turns that null into an AccountManager holding zero accounts, and both reload paths installed it unconditionally. The process then answered No Codex accounts configured. Run `opencode auth login`. while the accounts file on disk was intact and every other process on the machine was serving requests from it. Cross-process lock contention makes the failing read reachable: several opencode instances share one accounts file, and a read that loses a race against another process's atomic temp-file rename surfaces as exactly this empty result. Two guards, one per install site: - `reloadCachedAccountManager` compares the fresh manager against the incumbent it is replacing. A fresh manager with no accounts replacing an incumbent that has some is refused, the incumbent keeps serving, and a bounded retry (3 attempts, 2s apart) runs in case the next read succeeds. - `reloadForExternalAccountsChange` cannot compare against the incumbent, because an invalidation may legitimately have retired it and left the cache null. It compares against the file instead: the watcher already reads and hashes the changed file, so counting its `accounts` array costs nothing and says directly whether the accounts went away or the read failed. A file that carries accounts but loads as empty is refused and retried through the existing bounded retry path. The file-based comparison is what makes a genuine deletion still work. An external writer that really does remove the last account leaves an empty array on disk, the observed count is 0, the guard does not fire, and the empty pool is adopted as it should be. Both directions are covered by tests. Emptying the pool through the plugin's own surfaces (`codex-remove`, logout, a storage-mode switch) installs a manager directly rather than arriving on either of these paths, so neither guard can block a user-initiated removal. `readAccountsDigest` becomes `readAccountsFileState` and returns the count alongside the digest. The count is taken off the raw parsed document rather than the schema-validated union, so it reads the same for a V1, V2, or V3 file. The retry timers are unref'd and cancelled on watcher disposal, so a process shutting down mid-retry is not held open. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- index.ts | 115 +++++++++++++++++++++++++----- test/accounts-live-reload.test.ts | 29 ++++++++ 2 files changed, 128 insertions(+), 16 deletions(-) diff --git a/index.ts b/index.ts index c005291d..aa6ea961 100644 --- a/index.ts +++ b/index.ts @@ -1684,6 +1684,50 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } }; + /** + * `loadAccounts()` reports a read or parse failure the same way it reports + * an absent file - by returning null - and a null load builds a manager + * holding zero accounts. Installing that over a working pool makes this + * process answer "No Codex accounts configured" while the accounts file on + * disk is intact, which cross-process lock contention makes reachable. + * + * Emptying the pool for real always goes through an explicit action + * (`codex-remove`, logout, a storage-mode switch); each installs its own + * manager rather than arriving here, so refusing the shrink costs a genuine + * deletion nothing. + */ + const isUntrustworthyEmptyReload = ( + incumbent: AccountManager | null, + reloaded: AccountManager, + ): boolean => + incumbent !== null && + incumbent !== reloaded && + reloaded.getAccountCount() === 0 && + incumbent.getAccountCount() > 0; + + const EMPTY_RELOAD_RETRY_DELAY_MS = 2000; + const EMPTY_RELOAD_MAX_RETRIES = 3; + let emptyReloadRetries = 0; + let emptyReloadRetryTimer: ReturnType | undefined; + const cancelEmptyReloadRetry = (): void => { + clearTimeout(emptyReloadRetryTimer); + emptyReloadRetryTimer = undefined; + emptyReloadRetries = 0; + }; + const scheduleEmptyReloadRetry = (retry: () => Promise): void => { + if (emptyReloadRetries >= EMPTY_RELOAD_MAX_RETRIES) { + emptyReloadRetries = 0; + return; + } + emptyReloadRetries += 1; + clearTimeout(emptyReloadRetryTimer); + emptyReloadRetryTimer = setTimeout(() => { + emptyReloadRetryTimer = undefined; + void retry(); + }, EMPTY_RELOAD_RETRY_DELAY_MS); + emptyReloadRetryTimer.unref(); + }; + const reloadCachedAccountManager = async (): Promise => { if (!cachedAccountManager) return; const previous = cachedAccountManager; @@ -1702,6 +1746,15 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } try { const reloadedManager = await AccountManager.loadFromDisk(); + if (isUntrustworthyEmptyReload(previous, reloadedManager)) { + reloadedManager.disposeShutdownHandler(); + logWarn( + `[${PLUGIN_NAME}] Account reload returned no accounts while ${previous.getAccountCount()} are held; keeping the loaded pool and retrying`, + ); + scheduleEmptyReloadRetry(reloadCachedAccountManager); + return; + } + cancelEmptyReloadRetry(); cachedAccountManager = reloadedManager; accountManagerPromise = Promise.resolve(reloadedManager); // Dispose only after the replacement is installed so we never leak @@ -1734,21 +1787,41 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { accountsWatcherDisposed = true; unsubscribeAccountsPath?.(); stopAccountsWatcher(); + cancelEmptyReloadRetry(); unregisterCleanup(disposeAccountsWatcher); }; - const readAccountsDigest = async (path: string): Promise => { + const readAccountsFileState = async ( + path: string, + ): Promise<{ digest: string; accountCount: number } | undefined> => { try { const content = await readFile(path, "utf8"); - if (!AnyAccountStorageSchema.safeParse(JSON.parse(content)).success) return; - return createHash("sha256").update(content).digest("hex"); + const data = JSON.parse(content) as unknown; + if (!AnyAccountStorageSchema.safeParse(data).success) return; + // Counted off the raw document rather than the parsed union so the + // count is the same for every storage version. + const accounts = (data as { accounts?: unknown }).accounts; + return { + digest: createHash("sha256").update(content).digest("hex"), + accountCount: Array.isArray(accounts) ? accounts.length : 0, + }; } catch { return; } }; const reloadForExternalAccountsChange = async (path: string, generation: number, attempt = 0, retired?: AccountManager): Promise => { - const digest = await readAccountsDigest(path); - if (generation !== accountsWatchGeneration || !digest || path !== getStoragePath()) return; + const observed = await readAccountsFileState(path); + if (generation !== accountsWatchGeneration || !observed || path !== getStoragePath()) return; + const digest = observed.digest; if (digest === consumeLastWrittenAccountsDigest(path)) return; + const retryLater = (): void => { + if (attempt < 2 && generation === accountsWatchGeneration && !accountsWatcherDisposed) { + accountsReloadTimer = setTimeout(() => { + accountsReloadTimer = undefined; + void reloadForExternalAccountsChange(path, generation, attempt + 1, retired); + }, 1500); + accountsReloadTimer.unref(); + } + }; const previous = cachedAccountManager; try { // A null cache means an invalidation retired the incumbent; the @@ -1769,6 +1842,21 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { reloaded.disposeShutdownHandler(); return; } + // The file this reload observed carried accounts but the load + // produced none, so `loadAccounts()` failed to read it rather than + // the accounts having gone away - a failure it reports as an empty + // result, never as a throw, so the catch below cannot see it. + // Adopting it would answer "No Codex accounts configured" against an + // intact file; the retired incumbent still serves its accounts until + // a retry lands a real one. + if (observed.accountCount > 0 && reloaded.getAccountCount() === 0) { + reloaded.disposeShutdownHandler(); + logWarn( + `[${PLUGIN_NAME}] Externally changed accounts file holds ${observed.accountCount} account(s) but loaded as empty; keeping the current pool and retrying`, + ); + retryLater(); + return; + } const outgoing = cachedAccountManager; if (outgoing && outgoing !== retired) { // Another actor replaced the cached manager while this reload @@ -1784,13 +1872,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { observedAccountsDigest = digest; } catch { logWarn("Could not reload externally updated account storage"); - if (attempt < 2 && generation === accountsWatchGeneration && !accountsWatcherDisposed) { - accountsReloadTimer = setTimeout(() => { - accountsReloadTimer = undefined; - void reloadForExternalAccountsChange(path, generation, attempt + 1, retired); - }, 1500); - accountsReloadTimer.unref(); - } + retryLater(); return; } logDebug("Reloaded cached account manager after external accounts file change"); @@ -1804,8 +1886,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const path = watchedAccountsPath; if (!path) return; const generation = accountsWatchGeneration; - const digest = await readAccountsDigest(path); - if (generation !== accountsWatchGeneration || !digest || digest === observedAccountsDigest) return; + const observed = await readAccountsFileState(path); + if (generation !== accountsWatchGeneration || !observed || observed.digest === observedAccountsDigest) return; + const digest = observed.digest; observedAccountsDigest = digest; clearTimeout(accountsReloadTimer); accountsReloadTimer = undefined; @@ -1830,9 +1913,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { }); watchedAccountsPath = path; const generation = accountsWatchGeneration; - const initialDigest = await readAccountsDigest(path); + const initial = await readAccountsFileState(path); if (generation !== accountsWatchGeneration) return; - observedAccountsDigest = initialDigest; + observedAccountsDigest = initial?.digest; // Stat polling follows the path across the storage writer's temp-file rename. watchFile(path, { interval: 1500, persistent: false }, onAccountsStatChanged); unregisterCleanup(disposeAccountsWatcher); diff --git a/test/accounts-live-reload.test.ts b/test/accounts-live-reload.test.ts index d903336a..3612f87a 100644 --- a/test/accounts-live-reload.test.ts +++ b/test/accounts-live-reload.test.ts @@ -362,6 +362,35 @@ describe("accounts live reload", () => { await vi.advanceTimersByTimeAsync(5000); expect((await response).status).toBe(200); }); + it("keeps the loaded pool when an external change loads as empty", async () => { + const previous = captured.context?.cachedAccountManagerRef.current; + if (!previous) throw new Error("Missing manager"); + expect(previous.getAccountCount()).toBe(1); + const empty = new AccountManager(undefined, { ...storage(true), accounts: [] }); + const load = vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValueOnce(empty); + await fs.writeFile(path, JSON.stringify(storage(false))); + await tick(); + await settle(); + expect(load).toHaveBeenCalledTimes(1); + expect(captured.context?.cachedAccountManagerRef.current).toBe(previous); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountCount()).toBe(1); + const reloaded = nextReload(); + await vi.advanceTimersByTimeAsync(1500); + await drainReads(); + await reloaded; + expect(load).toHaveBeenCalledTimes(2); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountsSnapshot()[0]?.enabled).toBe(false); + }); + it("adopts an external change that genuinely removes the last account", async () => { + const previous = captured.context?.cachedAccountManagerRef.current; + const reloaded = nextReload(); + await fs.writeFile(path, JSON.stringify({ ...storage(true), accounts: [] })); + await tick(); + await settle(); + await reloaded; + expect(captured.context?.cachedAccountManagerRef.current).not.toBe(previous); + expect(captured.context?.cachedAccountManagerRef.current?.getAccountCount()).toBe(0); + }); it("keeps externally cleared blocks cleared despite queued and late saves from the old manager", async () => { const previous = captured.context?.cachedAccountManagerRef.current; if (!previous) throw new Error("Missing manager"); From 2825200c1e619c16f8635e74b78de43401c02696 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 05:15:04 -0500 Subject: [PATCH 06/15] test(vitest): remove the temp home each run mints The per-run home added alongside the HOME redirect was never removed, so every `npm test` left one behind. On a tmpfs `/tmp` that accumulates: 24 of them had collected on this machine, `/tmp` reached 98%, and the resulting ENOSPC killed a `vitest run` outright with `ENOSPC: no space left on device` on a pure unit-test file. A test harness that degrades the machine it runs on is the harness's own bug, not the operator's. A `globalSetup` teardown is the right hook: it runs once, in the main process, after every worker is finished, so it cannot race a suite that is still writing. The HOME redirect stays in `test.env` exactly where it was - that placement is load-bearing, because `lib/config.ts`, `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module scope and `test.env` is the only hook that lands before the worker imports them. Deleting a directory unattended deserves more care than deleting one by hand, so three conditions gate it and a path failing any of them is left alone rather than guessed at: - the config must have minted the directory itself. A home handed in through `OC_CODEX_TEST_HOME` belongs to whoever set it, and a CI harness that points the suite at a directory it manages must get that directory back. The config records ownership when it mints, so an inherited path and a minted one are distinguishable even when they look identical. - the resolved path must still sit under `tmpdir()`. - it must carry the prefix `mkdtempSync` was given. `force: true` keeps an already-removed directory from throwing, so an interrupted run cannot leave a failure that outlives it. The tests drive `teardown` against directories they create themselves, never against the live run's own home, so a future regression in the gate can only destroy scratch. Two of them are deliberately near identical - same path shape, opposite ownership - because that pins the ownership flag as the only thing deciding the delete. One more asserts that the prefix this module exports still matches the one the config minted with: the two are spelled in separate files, and were they to drift apart teardown would quietly stop matching and the leak would return with nothing failing. Every guard was control-run: each was broken in turn and the matching test confirmed failing before being restored. Verified end to end by counting `/tmp` before and after a full run - 20 before, 20 after, so the run minted a home and took it away again. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 5611cee --- test/global-setup.ts | 30 ++++++++++ test/test-home-isolation.test.ts | 95 +++++++++++++++++++++++++++++++- vitest.config.ts | 8 ++- 3 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 test/global-setup.ts diff --git a/test/global-setup.ts b/test/global-setup.ts new file mode 100644 index 00000000..2ad8ab64 --- /dev/null +++ b/test/global-setup.ts @@ -0,0 +1,30 @@ +import { rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { resolve } from "node:path"; + +export const MINTED_HOME_PREFIX = "oc-codex-multi-auth-test-home-"; + +/** + * Remove the throwaway home `vitest.config.ts` minted for this run. + * + * Every run otherwise leaves one behind, and on a tmpfs `/tmp` they accumulate + * until a run dies of ENOSPC. + * + * Three conditions gate the delete, because this is an unattended `rm -rf`: + * the config must have minted the directory itself rather than been handed one + * through `OC_CODEX_TEST_HOME`, the resolved path must still sit directly under + * `tmpdir()`, and it must carry the prefix `mkdtempSync` was given. A path that + * fails any of them is left alone rather than guessed at. + */ +export async function teardown(): Promise { + if (process.env.OC_CODEX_TEST_HOME_OWNED !== "1") return; + + const home = process.env.OC_CODEX_TEST_HOME; + if (!home) return; + + const resolved = resolve(home); + const expectedPrefix = resolve(tmpdir(), MINTED_HOME_PREFIX); + if (resolved === expectedPrefix || !resolved.startsWith(expectedPrefix)) return; + + await rm(resolved, { recursive: true, force: true }); +} diff --git a/test/test-home-isolation.test.ts b/test/test-home-isolation.test.ts index ec58263b..3f2618d8 100644 --- a/test/test-home-isolation.test.ts +++ b/test/test-home-isolation.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from "vitest"; -import { homedir, userInfo } from "node:os"; -import { isAbsolute, relative, resolve } from "node:path"; +import { existsSync, mkdtempSync, mkdirSync, rmSync } from "node:fs"; +import { homedir, tmpdir, userInfo } from "node:os"; +import { isAbsolute, join, relative, resolve } from "node:path"; import { LOG_DIR } from "../lib/logger.js"; import { ACCOUNTS_FILE_NAME } from "../lib/constants.js"; import { saveAccounts } from "../lib/storage/load-save.js"; @@ -10,6 +11,7 @@ import { setStoragePath, setStoragePathDirect, } from "../lib/storage/state.js"; +import { MINTED_HOME_PREFIX, teardown } from "./global-setup.js"; function realUserHome(): string { return userInfo().homedir; @@ -60,3 +62,92 @@ describe("test home isolation", () => { } }); }); + +describe("test home teardown", () => { + // Every case drives `teardown` against a directory this test made, never + // against the live run's own home, so a broken gate can only destroy scratch. + const runTeardown = async ( + home: string | undefined, + owned: boolean, + ): Promise => { + const previousHome = process.env.OC_CODEX_TEST_HOME; + const previousOwned = process.env.OC_CODEX_TEST_HOME_OWNED; + if (home === undefined) delete process.env.OC_CODEX_TEST_HOME; + else process.env.OC_CODEX_TEST_HOME = home; + if (owned) process.env.OC_CODEX_TEST_HOME_OWNED = "1"; + else delete process.env.OC_CODEX_TEST_HOME_OWNED; + try { + await teardown(); + } finally { + if (previousHome === undefined) delete process.env.OC_CODEX_TEST_HOME; + else process.env.OC_CODEX_TEST_HOME = previousHome; + if (previousOwned === undefined) delete process.env.OC_CODEX_TEST_HOME_OWNED; + else process.env.OC_CODEX_TEST_HOME_OWNED = previousOwned; + } + }; + + const mintedLookalike = (): string => + mkdtempSync(join(tmpdir(), MINTED_HOME_PREFIX)); + + // `vitest.config.ts` spells the prefix as a literal and this module exports + // it as a constant. Were the two to drift apart, teardown would simply stop + // matching the home the config minted and the leak would return silently. + it("agrees with the prefix the config actually minted", () => { + if (process.env.OC_CODEX_TEST_HOME_OWNED !== "1") return; + const home = process.env.OC_CODEX_TEST_HOME as string; + expect(resolve(home).startsWith(resolve(tmpdir(), MINTED_HOME_PREFIX))).toBe(true); + }); + + it("removes a home it minted itself", async () => { + const home = mintedLookalike(); + expect(existsSync(home)).toBe(true); + await runTeardown(home, true); + expect(existsSync(home)).toBe(false); + }); + + // Identical path shape to the case above, so the ownership flag is the only + // thing left deciding it. A home handed in through the environment belongs + // to whoever set it. + it("keeps an inherited home that looks exactly like a minted one", async () => { + const home = mintedLookalike(); + try { + await runTeardown(home, false); + expect(existsSync(home)).toBe(true); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it("keeps a path outside the temp directory", async () => { + const scratchRoot = join(process.cwd(), "tmp"); + mkdirSync(scratchRoot, { recursive: true }); + const outside = mkdtempSync(join(scratchRoot, MINTED_HOME_PREFIX)); + try { + await runTeardown(outside, true); + expect(existsSync(outside)).toBe(true); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + it("keeps a temp path that does not carry the minted prefix", async () => { + const foreign = mkdtempSync(join(tmpdir(), "oc-codex-unrelated-")); + try { + await runTeardown(foreign, true); + expect(existsSync(foreign)).toBe(true); + } finally { + rmSync(foreign, { recursive: true, force: true }); + } + }); + + // An interrupted run can leave the env half-set or the directory already + // gone. Neither may throw, or the failure outlives the run it came from. + it("tolerates a missing home and an already-removed one", async () => { + await expect(runTeardown(undefined, true)).resolves.toBeUndefined(); + + const home = mintedLookalike(); + await runTeardown(home, true); + expect(existsSync(home)).toBe(false); + await expect(runTeardown(home, true)).resolves.toBeUndefined(); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 41c0fe84..d28c7049 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -14,10 +14,13 @@ import { join } from 'node:path'; * `lib/accounts/recovery.ts` and `lib/logger.ts` capture `homedir()` at module * scope, so anything later than import time is too late for them. */ +const inheritedHome = process.env.OC_CODEX_TEST_HOME; const isolatedHome = - process.env.OC_CODEX_TEST_HOME ?? - mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); + inheritedHome ?? mkdtempSync(join(tmpdir(), 'oc-codex-multi-auth-test-home-')); process.env.OC_CODEX_TEST_HOME = isolatedHome; +// Only a home this config minted may be removed once the run ends. One handed +// in through the environment belongs to whoever set it. +if (!inheritedHome) process.env.OC_CODEX_TEST_HOME_OWNED = '1'; export default defineConfig({ test: { @@ -34,6 +37,7 @@ export default defineConfig({ // that race times out under full-suite CPU contention, which is flakiness in // the harness rather than in any assertion (a warm re-import costs ~400ms). testTimeout: 15_000, + globalSetup: ['./test/global-setup.ts'], include: ['test/**/*.test.ts'], exclude: [ 'node_modules/**', From ae02060dc5487ab86fe3ddfeba025c3d03c822b9 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 12:30:09 -0500 Subject: [PATCH 07/15] fix(accounts): name a seat by its seat, not by its workspace An account id names a ChatGPT workspace, and every member of a Business workspace shares it. Rendering it alone therefore gave distinct members of one workspace an identical identity string: Account 7 (one@example.com, id:4f10cc...3ab921) Account 8 (two@example.com, id:4f10cc...3ab921) Those are two different seats. Upstream meters each separately - its own quota, its own weekly reset - and the store holds them as separate records. Only the display collapsed them, which reads as one account duplicated and sends whoever reads it hunting a dedup bug that is not there. `accountUserId` is the member's own id and the only stored field that tells two seats of one workspace apart, and no surface rendered it. Every account-identity renderer now appends its last 6 characters as `seat:`, beside the 6 of `accountId` those surfaces already print: Account 7 (one@example.com, id:3ab921, seat:111111) Account 8 (two@example.com, id:3ab921, seat:222222) Six characters rather than the whole uuid keeps the rows one line, and the `seat:` prefix pairs with the `id:` already beside it so neither suffix has to be guessed at. `formatSeatSuffix` is shared so the five renderers that carried this independently cannot drift apart again: `formatAccountLabel`, the `formatCommandAccountLabel` closure behind every `codex-*` tool, the interactive auth menu, the fallback login menu, and the standalone CLI's account summary. The `auth login --deep` probe line prints both ids read off the probed token, so the pair names the seat that actually answered rather than the workspace it belongs to. A record with no member id renders byte-for-byte as it did before, which is what leaves token-only records untouched. The standalone CLI puts the seat through the same mask and suffix pair as `accountId`, so a printed `seat:` never discloses more than the field beside it. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm c5ebcc8-dirty --- README.md | 2 +- index.ts | 22 +++++++-- lib/account-display.ts | 25 ++++++++++ lib/accounts.ts | 36 +++++++++----- lib/cli.ts | 5 +- lib/ui/auth-menu.ts | 5 +- scripts/install-oc-codex-multi-auth-core.js | 15 +++++- test/accounts.test.ts | 36 ++++++++++++++ test/auth-menu.test.ts | 55 +++++++++++++++++++++ test/cli.test.ts | 18 +++++++ 10 files changed, 199 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 11b4e230..8bd7d83c 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ Most of these also run as a **direct CLI** with no agent or model involvement, s - unsupported-model handling is strict by default, with opt-in fallback controls - TUI quota status follows the account/workspace used by the latest request - Business workspace memberships and Personal accounts keep separate usage and quota windows. Business members sharing one workspace are distinguished by their member/seat identity, so their usage is not collapsed into one row. -- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. +- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints the last 6 characters of that as `seat:`. Two seats in one workspace are therefore told apart on screen instead of rendering one identical `id:`. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. - The ChatGPT plan (`Free`, `Plus`, `Pro`, `Business`, `Business Premium`, `Enterprise`) is read from the access token, refreshed on every token refresh, and shown by `codex-list` and `codex-status`. `codex-limits` and the TUI read the plan live from the usage endpoint and name it the same way. An unrecognized plan is reported verbatim rather than renamed. --- diff --git a/index.ts b/index.ts index aa6ea961..8780bc84 100644 --- a/index.ts +++ b/index.ts @@ -144,7 +144,8 @@ import { matchesModelPoolAccountKey, type ModelPoolAccount, } from "./lib/accounts/pool-identity.js"; -import { resolveDisplayEmail } from "./lib/account-display.js"; +import { formatSeatSuffix, resolveDisplayEmail } from "./lib/account-display.js"; +import { extractAccountUserId } from "./lib/auth/token-utils.js"; import { CodexAuthError } from "./lib/errors.js"; import { getStoragePath, @@ -1273,6 +1274,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { account: { email?: string; accountId?: string; + accountUserId?: string; accountLabel?: string; accountTags?: string[]; accountNote?: string; @@ -1283,6 +1285,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const email = resolveDisplayEmail(account?.email, options.maskEmail ?? false); const workspace = account?.accountLabel?.trim(); const accountId = formatAccountIdForDisplay(account?.accountId); + const seat = formatSeatSuffix(account?.accountUserId); const tags = Array.isArray(account?.accountTags) ? account.accountTags @@ -1294,6 +1297,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { if (email) details.push(email); if (workspace) details.push(`workspace:${workspace}`); if (accountId) details.push(`id:${accountId}`); + if (seat) details.push(`seat:${seat}`); if (tags.length > 0) details.push(`tags:${tags.join(",")}`); if (details.length === 0) { @@ -4290,9 +4294,20 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { if (deepProbe) { ok += 1; + // Both read from the probed token, so the pair is the seat + // the credential actually belongs to. The workspace id + // alone repeats across every member of a Business + // workspace and cannot confirm which seat answered. + const tokenSeat = formatSeatSuffix( + extractAccountUserId(accessToken), + ); + const identity = [ + tokenAccountId ? `id:${tokenAccountId.slice(-6)}` : undefined, + tokenSeat ? `seat:${tokenSeat}` : undefined, + ].filter((part): part is string => part !== undefined); const detail = - tokenAccountId - ? `${authDetail} (id:${tokenAccountId.slice(-6)})` + identity.length > 0 + ? `${authDetail} (${identity.join(", ")})` : authDetail; console.log(`[${i + 1}/${total}] ${label}: ${detail}`); continue; @@ -4644,6 +4659,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { } return { accountId: account.accountId, + accountUserId: account.accountUserId, accountLabel: account.accountLabel, email: account.email, index, diff --git a/lib/account-display.ts b/lib/account-display.ts index a9d9f4ef..d845dde9 100644 --- a/lib/account-display.ts +++ b/lib/account-display.ts @@ -47,3 +47,28 @@ export function resolveDisplayEmail( if (!trimmed) return undefined; return maskEmail ? maskEmailForDisplay(trimmed) : trimmed; } + +/** + * Render the short seat suffix that separates two accounts sharing one + * workspace `accountId`. + * + * A ChatGPT Business workspace is a single `accountId` shared by every member + * of it; `accountUserId` is that member's own id and the only stored field + * that tells their seats apart. Upstream meters each seat separately - its own + * quota, its own weekly reset - so seats sharing a workspace are distinct + * accounts, not copies of one. + * + * Every display surface used to render `accountId` alone, so four members of + * one Business workspace printed an identical `id:` string and read as the + * same account duplicated four times. Appending this suffix is what makes the + * rendered rows match the accounts they describe. + * + * Six characters, matching what the surfaces already print for `accountId`. + * Returns `undefined` when there is no member id, so a token-only record + * renders exactly as it did before. + */ +export function formatSeatSuffix(accountUserId: string | undefined): string | undefined { + const trimmed = accountUserId?.trim(); + if (!trimmed) return undefined; + return trimmed.length > 6 ? trimmed.slice(-6) : trimmed; +} diff --git a/lib/accounts.ts b/lib/accounts.ts index c07c2a0a..c1991eee 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -31,7 +31,7 @@ import { import { formatWaitTime, type RateLimitReason } from "./accounts/rate-limits.js"; import { nowMs } from "./utils.js"; import { logWarn } from "./logger.js"; -import { resolveDisplayEmail } from "./account-display.js"; +import { formatSeatSuffix, resolveDisplayEmail } from "./account-display.js"; export type { AccountSelectionExplainability, ManagedAccount } from "./accounts/state.js"; @@ -424,7 +424,9 @@ export class AccountManager { } export function formatAccountLabel( - account: { email?: string; accountId?: string; accountLabel?: string } | undefined, + account: + | { email?: string; accountId?: string; accountUserId?: string; accountLabel?: string } + | undefined, index: number, options: { maskEmail?: boolean } = {}, ): string { @@ -436,17 +438,25 @@ export function formatAccountLabel( ? accountId.slice(-6) : accountId : null; - - if (accountLabel && email && idSuffix) { - return `Account ${index + 1} (${accountLabel}, ${email}, id:${idSuffix})`; - } - if (accountLabel && email) return `Account ${index + 1} (${accountLabel}, ${email})`; - if (accountLabel && idSuffix) return `Account ${index + 1} (${accountLabel}, id:${idSuffix})`; - if (accountLabel) return `Account ${index + 1} (${accountLabel})`; - if (email && idSuffix) return `Account ${index + 1} (${email}, id:${idSuffix})`; - if (email) return `Account ${index + 1} (${email})`; - if (idSuffix) return `Account ${index + 1} (${idSuffix})`; - return `Account ${index + 1}`; + // `accountId` names the workspace, which every member of a Business + // workspace shares. Without the seat, four distinct members printed one + // identical `id:` string (see `formatSeatSuffix`). + const seatSuffix = formatSeatSuffix(account?.accountUserId); + + const details: string[] = []; + if (accountLabel) details.push(accountLabel); + if (email) details.push(email); + if (idSuffix) { + // The id has always rendered bare when it is the only thing known about + // an account. Once a seat sits beside it, an unprefixed pair of suffixes + // would not say which is which, so the prefix goes back on. + const idIsOnlyDetail = !accountLabel && !email && !seatSuffix; + details.push(idIsOnlyDetail ? idSuffix : `id:${idSuffix}`); + } + if (seatSuffix) details.push(`seat:${seatSuffix}`); + + if (details.length === 0) return `Account ${index + 1}`; + return `Account ${index + 1} (${details.join(", ")})`; } export function formatCooldown( diff --git a/lib/cli.ts b/lib/cli.ts index 680ec70c..d8049db4 100644 --- a/lib/cli.ts +++ b/lib/cli.ts @@ -1,7 +1,7 @@ import { createInterface } from "node:readline/promises"; import { stdin as input, stdout as output } from "node:process"; import type { AccountIdSource } from "./types.js"; -import { resolveDisplayEmail } from "./account-display.js"; +import { formatSeatSuffix, resolveDisplayEmail } from "./account-display.js"; import { showAuthMenu, showAccountDetails, @@ -51,6 +51,7 @@ export type LoginMode = export interface ExistingAccountInfo { accountId?: string; + accountUserId?: string; accountLabel?: string; email?: string; index: number; @@ -87,10 +88,12 @@ function formatAccountLabel( accountId && accountId.length > 14 ? `${accountId.slice(0, 8)}...${accountId.slice(-6)}` : accountId; + const seatSuffix = formatSeatSuffix(account.accountUserId); const details: string[] = []; if (email) details.push(email); if (label) details.push(`workspace:${label}`); if (accountIdDisplay) details.push(`id:${accountIdDisplay}`); + if (seatSuffix) details.push(`seat:${seatSuffix}`); if (details.length > 0) { return `${num}. ${details.join(" | ")}`; } diff --git a/lib/ui/auth-menu.ts b/lib/ui/auth-menu.ts index f463f2b6..38760f95 100644 --- a/lib/ui/auth-menu.ts +++ b/lib/ui/auth-menu.ts @@ -3,7 +3,7 @@ import { confirm } from "./confirm.js"; import { getUiRuntimeOptions } from "./runtime.js"; import { select, type MenuItem } from "./select.js"; import { paintUiText, formatUiBadge } from "./format.js"; -import { resolveDisplayEmail } from "../account-display.js"; +import { formatSeatSuffix, resolveDisplayEmail } from "../account-display.js"; export type AccountStatus = | "active" @@ -18,6 +18,7 @@ export type AccountStatus = export interface AccountInfo { index: number; accountId?: string; + accountUserId?: string; accountLabel?: string; email?: string; addedAt?: number; @@ -121,6 +122,8 @@ function accountTitle(account: AccountInfo, maskEmail = false): string { if (accountIdSuffix && (!label || !label.includes(accountIdSuffix))) { details.push(`id:${accountIdSuffix}`); } + const seatSuffix = formatSeatSuffix(account.accountUserId); + if (seatSuffix) details.push(`seat:${seatSuffix}`); if (details.length === 0) { return `${account.index + 1}. Account`; diff --git a/scripts/install-oc-codex-multi-auth-core.js b/scripts/install-oc-codex-multi-auth-core.js index ff5fb911..b5e26cd2 100644 --- a/scripts/install-oc-codex-multi-auth-core.js +++ b/scripts/install-oc-codex-multi-auth-core.js @@ -422,12 +422,21 @@ function summarizeStandaloneAccounts(storage, includeSensitive, tag) { const trimmedId = typeof account?.accountId === "string" ? account.accountId.trim() : ""; const accountId = trimmedId || undefined; + // Members of one Business workspace share `accountId`, so the seat is + // what tells them apart. It is carried masked next to its suffix for + // the same reason `accountId` is: so the printed `seat:` never + // discloses more than the field beside it. + const trimmedUserId = + typeof account?.accountUserId === "string" ? account.accountUserId.trim() : ""; + const accountUserId = trimmedUserId || undefined; return { index, label: account?.accountLabel ?? `Account ${index + 1}`, email: maskValue(account?.email, includeSensitive), accountId: maskValue(accountId, includeSensitive), idSuffix: accountIdSuffix(accountId, includeSensitive), + accountUserId: maskValue(accountUserId, includeSensitive), + seatSuffix: accountIdSuffix(accountUserId, includeSensitive), accountIdSource: account?.accountIdSource, enabled: account?.enabled !== false, hasRefreshToken: typeof account?.refreshToken === "string" && account.refreshToken.length > 0, @@ -453,7 +462,11 @@ function printStandaloneResult(command, payload, json) { console.log(`Accounts: ${payload.totalAccounts}`); if (Array.isArray(payload.accounts)) { for (const account of payload.accounts) { - const identity = [account.email, account.idSuffix ? `id:${account.idSuffix}` : undefined] + const identity = [ + account.email, + account.idSuffix ? `id:${account.idSuffix}` : undefined, + account.seatSuffix ? `seat:${account.seatSuffix}` : undefined, + ] .filter(Boolean) .join(", "); const name = identity ? `${account.label} (${identity})` : account.label; diff --git a/test/accounts.test.ts b/test/accounts.test.ts index 24a44be2..1704b6e9 100644 --- a/test/accounts.test.ts +++ b/test/accounts.test.ts @@ -820,6 +820,42 @@ describe("AccountManager", () => { expect(formatAccountLabel({ accountId: "123456" }, 0)).toBe("Account 1 (123456)"); }); + // Same index on both sides: the rendered label may only differ by seat, so + // the assertion cannot pass on "Account 7" vs "Account 8" alone. + it("renders distinct labels for two seats sharing one workspace accountId", () => { + const workspace = { + email: "shared@example.com", + accountId: "05cd9f040000000000989a40", + }; + + const first = formatAccountLabel( + { ...workspace, accountUserId: "user_aaaaaa111111" }, + 6, + ); + const second = formatAccountLabel( + { ...workspace, accountUserId: "user_bbbbbb222222" }, + 6, + ); + + expect(first).not.toBe(second); + expect(first).toBe("Account 7 (shared@example.com, id:989a40, seat:111111)"); + expect(second).toBe("Account 7 (shared@example.com, id:989a40, seat:222222)"); + }); + + it("renders an account with no accountUserId exactly as before", () => { + expect( + formatAccountLabel({ email: "user@example.com", accountId: "abcdef123456" }, 0), + ).toBe("Account 1 (user@example.com, id:123456)"); + expect(formatAccountLabel({ accountId: "abcdef123456" }, 2)).toBe("Account 3 (123456)"); + expect( + formatAccountLabel( + { accountLabel: "Work", email: "work@co.com", accountId: "abcdef123456" }, + 0, + ), + ).toBe("Account 1 (Work, work@co.com, id:123456)"); + expect(formatAccountLabel({ accountUserId: "" }, 3)).toBe("Account 4"); + }); + it("performs true round-robin rotation across multiple requests", () => { const now = Date.now(); const stored = { diff --git a/test/auth-menu.test.ts b/test/auth-menu.test.ts index 6449803f..57554973 100644 --- a/test/auth-menu.test.ts +++ b/test/auth-menu.test.ts @@ -56,6 +56,61 @@ describe("auth-menu", () => { expect(accountRows[1]?.label).toContain("id:org-cccc...dd3333"); }); + it("renders distinct rows for two seats sharing one workspace accountId", async () => { + vi.mocked(select).mockResolvedValueOnce({ type: "cancel" }); + + const workspaceId = "org-aaaa1111bbbb2222"; + const accounts: AccountInfo[] = [ + { + index: 0, + email: "shared@example.com", + accountId: workspaceId, + accountUserId: "user_aaaaaa111111", + }, + { + index: 1, + email: "shared@example.com", + accountId: workspaceId, + accountUserId: "user_bbbbbb222222", + }, + ]; + + await showAuthMenu(accounts); + + const items = vi.mocked(select).mock.calls[0]?.[0] as Array<{ + label: string; + value?: { type?: string }; + }>; + const accountRows = items.filter((item) => item.value?.type === "select-account"); + expect(accountRows).toHaveLength(2); + expect(accountRows[0]?.label).toContain("seat:111111"); + expect(accountRows[1]?.label).toContain("seat:222222"); + // The rows carry the same email and the same workspace id, so dropping + // the seat collapses them into one indistinguishable string. + expect(accountRows[0]?.label.replace(/^1\. /, "")).not.toBe( + accountRows[1]?.label.replace(/^2\. /, ""), + ); + }); + + it("omits the seat for an account with no accountUserId", async () => { + vi.mocked(select).mockResolvedValueOnce({ type: "cancel" }); + + await showAuthMenu([ + { + index: 0, + email: "solo@example.com", + accountId: "org-aaaa1111bbbb2222", + }, + ]); + + const items = vi.mocked(select).mock.calls[0]?.[0] as Array<{ + label: string; + value?: { type?: string }; + }>; + const row = items.find((item) => item.value?.type === "select-account"); + expect(row?.label).toBe("1. solo@example.com | id:org-aaaa...bb2222"); + }); + it("uses detailed account title in delete confirmation", async () => { vi.mocked(select).mockResolvedValueOnce("delete"); vi.mocked(confirm).mockResolvedValueOnce(true); diff --git a/test/cli.test.ts b/test/cli.test.ts index 971902ed..92171a04 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -189,6 +189,24 @@ describe("CLI Module", () => { expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining("1. id:acc_1234567890")); }); + it("displays a seat suffix that separates two members of one workspace", async () => { + mockRl.question.mockResolvedValueOnce("a"); + const consoleSpy = vi.spyOn(console, "log"); + + const { promptLoginMode } = await import("../lib/cli.js"); + await promptLoginMode([ + { index: 0, email: "shared@example.com", accountId: "acc_1234567890", accountUserId: "user_aaaaaa111111" }, + { index: 1, email: "shared@example.com", accountId: "acc_1234567890", accountUserId: "user_bbbbbb222222" }, + ]); + + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining("1. shared@example.com | id:acc_1234567890 | seat:111111"), + ); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining("2. shared@example.com | id:acc_1234567890 | seat:222222"), + ); + }); + it("displays plain Account N when no email or accountId", async () => { mockRl.question.mockResolvedValueOnce("f"); const consoleSpy = vi.spyOn(console, "log"); From ca34f609376949817c96960c95271c5f7d63c4d6 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 12:43:36 -0500 Subject: [PATCH 08/15] fix(auth): say whether a login repaired a seat or added one `opencode auth login` reported nothing about what it did to the store. A login that lands on a seat already held and a login that appends a seat never held before produce the same silence, and the two outcomes are opposite: one refreshes the credentials of an account you already have, the other leaves that account exactly as exhausted as it was and puts a new one beside it. That silence is how a pool grows without anyone deciding it should. Three logins run to repair three spent accounts landed on three seats the store had never held; the count went 6 to 9 and nothing said so. Every one of those seats shares a workspace `accountId` with an account already in the pool, so `codex-list` afterwards showed what looked like duplicates. After persistence settles, each login result now reports its outcome through `logInfo`, the channel this file already uses for the `CODEX_AUTH_ACCOUNT_ID` override: Login updated Account 4 (id:3ab921, seat:111111) in place - an account already in the store. Login added Account 9 (id:3ab921, seat:222222) as a NEW account - it was not in the store, so it repaired no existing account. Same workspace id as Account 4, Account 7. The neighbour line is the one that answers the question actually being asked: an addition that shares a workspace id or an email with accounts already stored names those slots, so "this did not repair account 4" is visible at the moment it happens rather than inferred from a count three steps later. Reported after `pruneRefreshTokenCollisions` rather than inside the persist loop, because a slot number is only true once the prune has run. The outcome is recorded in the loop, where add-vs-update is known, and keyed by the refresh token the login just wrote; a merge keeps the newest record's token, so the key still resolves the row that survived. Slots only. The line has no access to the `maskEmail` setting every other identity surface honors, so it names `Account N` and prints the same 6-character `id:`/`seat:` suffixes those surfaces already show, never an address. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm c5ebcc8-dirty --- lib/auth/login-runner.ts | 72 +++++++++++++++++++++++++++++++++++++ test/login-runner.test.ts | 76 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/lib/auth/login-runner.ts b/lib/auth/login-runner.ts index 50b5ab3a..c9aa2a40 100644 --- a/lib/auth/login-runner.ts +++ b/lib/auth/login-runner.ts @@ -433,6 +433,12 @@ export async function resolveAndPersistAccountSelection( return persistResolvedAccountSelection(selection, options); } +function formatIdentitySuffix(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + if (!trimmed) return undefined; + return trimmed.length > 6 ? trimmed.slice(-6) : trimmed; +} + /** * Persists login results through the shared storage transaction so overlapping * login retries serialize their read-modify-write cycle instead of racing stale @@ -743,6 +749,13 @@ export async function persistAccountPool( let identityIndexes = buildIdentityIndexes(); + // Whether a login landed on an existing record or appended a new one is + // only knowable here, but the slot it ends up in is only final after the + // prune below, so the decision is recorded now and reported there. Keyed + // by refresh token: the login just wrote it, and a merge keeps the newest + // record's token, so the key still finds the row that survived. + const loginOutcomes: { refreshToken: string; added: boolean }[] = []; + for (const result of results) { const accountId = result.accountIdOverride ?? extractAccountId(result.access); const normalizedAccountId = accountId?.trim() || undefined; @@ -855,6 +868,7 @@ export async function persistAccountPool( addedAt: now, lastUsed: now, }); + loginOutcomes.push({ refreshToken: result.refresh, added: true }); identityIndexes = buildIdentityIndexes(); continue; } @@ -902,6 +916,7 @@ export async function persistAccountPool( oauthScope: normalizedScope ?? existing.oauthScope, lastUsed: now, }; + loginOutcomes.push({ refreshToken: result.refresh, added: false }); identityIndexes = buildIdentityIndexes(); } @@ -1001,6 +1016,63 @@ export async function persistAccountPool( if (accounts.length === 0) return; + // A login that lands on a seat the store never held is indistinguishable + // from one that repaired an existing seat unless it says which it did. + // The workspace/email neighbours are named because that is the line that + // distinguishes "this replaced your exhausted account" from "this added a + // ninth account beside it". Slots only - an email is never printed here, + // matching every other identity surface. + const describeSlots = (indexes: number[]): string => + indexes.map((slot) => `Account ${slot + 1}`).join(", "); + + for (const outcome of loginOutcomes) { + const index = accounts.findIndex( + (account) => account?.refreshToken === outcome.refreshToken, + ); + if (index < 0) continue; + const account = accounts[index]; + if (!account) continue; + + const identityParts: string[] = []; + const idSuffix = formatIdentitySuffix(account.accountId); + const seatSuffix = formatIdentitySuffix(account.accountUserId); + if (idSuffix) identityParts.push(`id:${idSuffix}`); + if (seatSuffix) identityParts.push(`seat:${seatSuffix}`); + const identity = identityParts.length > 0 ? ` (${identityParts.join(", ")})` : ""; + + if (!outcome.added) { + logInfo( + `Login updated Account ${index + 1}${identity} in place - an account already in the store.`, + ); + continue; + } + + const workspaceId = account.accountId?.trim(); + const email = sanitizeEmail(account.email); + const sameWorkspace: number[] = []; + const sameEmail: number[] = []; + for (let i = 0; i < accounts.length; i += 1) { + if (i === index) continue; + const other = accounts[i]; + if (!other) continue; + if (workspaceId && other.accountId?.trim() === workspaceId) sameWorkspace.push(i); + if (email && sanitizeEmail(other.email) === email) sameEmail.push(i); + } + + const notes: string[] = []; + if (sameWorkspace.length > 0) { + notes.push(`Same workspace id as ${describeSlots(sameWorkspace)}.`); + } + if (sameEmail.length > 0) { + notes.push(`Same email as ${describeSlots(sameEmail)}.`); + } + logInfo( + `Login added Account ${index + 1}${identity} as a NEW account - it was not in the store, so it repaired no existing account.${ + notes.length > 0 ? ` ${notes.join(" ")}` : "" + }`, + ); + } + const resolveIndexByIdentityKeys = (identityKeys: string[] | undefined): number | undefined => { if (!identityKeys || identityKeys.length === 0) return undefined; for (const identityKey of identityKeys) { diff --git a/test/login-runner.test.ts b/test/login-runner.test.ts index b305d025..6d057788 100644 --- a/test/login-runner.test.ts +++ b/test/login-runner.test.ts @@ -14,6 +14,7 @@ import { } from "../lib/auth/login-runner.js"; import { JWT_CLAIM_PATH } from "../lib/constants.js"; import { loadAccounts, setStoragePathDirect } from "../lib/storage.js"; +import * as loggerModule from "../lib/logger.js"; import { JWT_CLAIM_PATH } from "../lib/constants.js"; function createTokenResult( @@ -605,6 +606,81 @@ describe("login-runner account and quota identities", () => { ]); }); + const loginAs = async ( + workspaceId: string, + memberId: string, + email: string, + refresh: string, + ): Promise => { + await persistAccountPool( + [ + { + type: "success", + access: businessAccessTokenFor(workspaceId, memberId, email), + refresh, + expires: Date.now() + 60_000, + }, + ], + false, + ); + }; + + it("reports a re-login of a stored seat as an in-place update", async () => { + const infoSpy = vi.spyOn(loggerModule, "logInfo"); + await loginAs("workspace-a", "member-a", "a@example.com", "refresh-a"); + infoSpy.mockClear(); + + await loginAs("workspace-a", "member-a", "a@example.com", "refresh-a-new"); + + expect(await loadAccounts().then((stored) => stored?.accounts)).toHaveLength(1); + expect(infoSpy).toHaveBeenCalledWith( + expect.stringContaining("Login updated Account 1"), + ); + expect(infoSpy).toHaveBeenCalledWith(expect.stringContaining("in place")); + expect(infoSpy).not.toHaveBeenCalledWith( + expect.stringContaining("as a NEW account"), + ); + }); + + it("reports a new seat in a stored workspace as an addition, naming the slot it did not repair", async () => { + const infoSpy = vi.spyOn(loggerModule, "logInfo"); + await loginAs("workspace-a", "member-a", "first@example.com", "refresh-a"); + infoSpy.mockClear(); + + await loginAs("workspace-a", "member-b", "second@example.com", "refresh-b"); + + expect(await loadAccounts().then((stored) => stored?.accounts)).toHaveLength(2); + expect(infoSpy).toHaveBeenCalledWith( + expect.stringContaining("Login added Account 2"), + ); + expect(infoSpy).toHaveBeenCalledWith( + expect.stringContaining("as a NEW account"), + ); + expect(infoSpy).toHaveBeenCalledWith( + expect.stringContaining("Same workspace id as Account 1."), + ); + // Only slots are named. An email on this line would be the one identity + // surface that ignores `maskEmail`. + const messages = infoSpy.mock.calls.map(([message]) => String(message)); + expect(messages.join("\n")).not.toContain("example.com"); + }); + + it("names the slot sharing an email when a new seat is added under a different workspace", async () => { + const infoSpy = vi.spyOn(loggerModule, "logInfo"); + await loginAs("workspace-a", "member-a", "shared@example.com", "refresh-a"); + infoSpy.mockClear(); + + await loginAs("workspace-b", "member-b", "shared@example.com", "refresh-b"); + + expect(await loadAccounts().then((stored) => stored?.accounts)).toHaveLength(2); + expect(infoSpy).toHaveBeenCalledWith( + expect.stringContaining("Same email as Account 1."), + ); + expect(infoSpy).not.toHaveBeenCalledWith( + expect.stringContaining("Same workspace id as"), + ); + }); + /** An access token that names one ChatGPT account, the unit the backend meters. */ const accessTokenFor = (chatgptAccountId: string): string => encodeJwt({ [JWT_CLAIM_PATH]: { chatgpt_account_id: chatgptAccountId } }); From 8800101bd717882f3036cd763f148dc4d3fe2d92 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 12:53:47 -0500 Subject: [PATCH 09/15] fix(auth): let the login prune actually collide with itself `pruneRefreshTokenCollisions` exists to collapse two stored records that are the same account. It keyed them on org:|account:|member:|refresh: with the refresh token inside the key, so two records collided only when their tokens were byte-identical. A re-login mints a new refresh token - which is precisely how a second record of one seat comes to exist - so the one case this prune is for was the one case it could never see. It merged only records that were already identical in every field it compared, which is no merge at all. org+account+member is a seat, and a seat is one account: same workspace, same member, therefore one quota pool upstream. Two records carrying it are that account twice, and the newer supersedes the older. So the seat key drops the token, and `pickNewestAccountIndex` + `mergeStoredAccountPair` keep the live credential. The token stays in both keys that do NOT name a seat. Two records under one workspace id with no member id, exactly like two sharing only an email, can be two different members whose seat was never recorded - Business workspaces are shared by construction. Merging those would delete a working account, so there they keep the token that tells them apart. That is why this is two branches and not one. This is latent. It did not cause any account to be duplicated or lost: `normalizeAccountStorage` already dedupes on the same org|account|member seat key on every load and every save, so a record this prune should have merged is merged before it reaches disk. The fix removes a dead branch's dead-ness, it does not repair damage. Because the storage layer normalizes on write, the prune's effect cannot be read back off disk - so the tests stub `withAccountStorageTransaction` and assert on the array the runner hands to `persist`, covering both directions: one seat with two tokens merges to the newest, two email-only records with two tokens stay separate. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm c5ebcc8-dirty --- lib/auth/login-runner.ts | 15 +++++- test/login-runner.test.ts | 96 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/lib/auth/login-runner.ts b/lib/auth/login-runner.ts index c9aa2a40..7367145e 100644 --- a/lib/auth/login-runner.ts +++ b/lib/auth/login-runner.ts @@ -938,7 +938,20 @@ export async function persistAccountPool( const accountUserId = account?.accountUserId?.trim() ?? ""; const email = account?.email?.trim().toLowerCase() ?? ""; const refreshToken = account?.refreshToken?.trim() ?? ""; - if (organizationId || accountId || accountUserId) { + // A member id pins one seat of one workspace, so two records + // carrying it are the same seat and the newer one supersedes the + // older. The refresh token is left out because a re-login mints a + // new one: keying on it meant the single case this prune exists + // to collapse was the one case that could never collide. + if (accountUserId) { + return `org:${organizationId}|account:${accountId}|member:${accountUserId}`; + } + // No member id, so the seat is unknown. Two records under one + // workspace id, like two sharing only an email, can be two + // different members whose seat was never recorded - so both keep + // the token that tells them apart rather than risk merging two + // live accounts into one. + if (organizationId || accountId) { return `org:${organizationId}|account:${accountId}|member:${accountUserId}|refresh:${refreshToken}`; } return `email:${email}|refresh:${refreshToken}`; diff --git a/test/login-runner.test.ts b/test/login-runner.test.ts index 6d057788..91ec2dec 100644 --- a/test/login-runner.test.ts +++ b/test/login-runner.test.ts @@ -14,6 +14,8 @@ import { } from "../lib/auth/login-runner.js"; import { JWT_CLAIM_PATH } from "../lib/constants.js"; import { loadAccounts, setStoragePathDirect } from "../lib/storage.js"; +import type { AccountMetadataV3, AccountStorageV3 } from "../lib/storage.js"; +import * as loadSaveModule from "../lib/storage/load-save.js"; import * as loggerModule from "../lib/logger.js"; import { JWT_CLAIM_PATH } from "../lib/constants.js"; @@ -681,6 +683,100 @@ describe("login-runner account and quota identities", () => { ); }); + // Reads the array the runner hands to `persist`, because reading it back + // through the storage layer cannot see this: `saveAccounts` normalizes on + // write using the same org|account|member seat key, so it merges same-seat + // records itself and hides whether the prune did anything. + const prunedAccountsFor = async ( + stored: AccountMetadataV3[], + result: TokenSuccessWithAccount, + ): Promise => { + let persisted: AccountStorageV3 | undefined; + const transaction = vi + .spyOn(loadSaveModule, "withAccountStorageTransaction") + .mockImplementation(( + handler: ( + current: AccountStorageV3 | null, + persist: (storage: AccountStorageV3) => Promise, + ) => Promise, + ): Promise => + handler( + { version: 3, accounts: stored, activeIndex: 0, activeIndexByFamily: {} }, + async (storage) => { + persisted = storage; + }, + )); + try { + await persistAccountPool([result], false); + } finally { + transaction.mockRestore(); + } + return persisted?.accounts ?? []; + }; + + /** A login for a seat none of the seeded records hold, so only the prune acts on them. */ + const unrelatedSeatLogin = (): TokenSuccessWithAccount => ({ + type: "success", + access: businessAccessTokenFor("workspace-z", "member-z", "z@example.com"), + refresh: "refresh-z", + expires: Date.now() + 60_000, + }); + + it("merges two records of one seat that carry different refresh tokens", async () => { + const accounts = await prunedAccountsFor( + [ + { + accountId: "workspace-a", + accountUserId: "member-a", + email: "a@example.com", + refreshToken: "refresh-stale", + addedAt: 1_000, + lastUsed: 1_000, + }, + { + accountId: "workspace-a", + accountUserId: "member-a", + email: "a@example.com", + refreshToken: "refresh-current", + addedAt: 2_000, + lastUsed: 2_000, + }, + ], + unrelatedSeatLogin(), + ); + + const seat = accounts.filter((account) => account.accountUserId === "member-a"); + expect(seat).toHaveLength(1); + expect(seat[0]?.refreshToken).toBe("refresh-current"); + expect(accounts).toHaveLength(2); + }); + + it("keeps two email-only records with different refresh tokens apart", async () => { + const accounts = await prunedAccountsFor( + [ + { + email: "shared@example.com", + refreshToken: "refresh-1", + addedAt: 1_000, + lastUsed: 1_000, + }, + { + email: "shared@example.com", + refreshToken: "refresh-2", + addedAt: 2_000, + lastUsed: 2_000, + }, + ], + unrelatedSeatLogin(), + ); + + expect(accounts.map((account) => account.refreshToken)).toEqual([ + "refresh-1", + "refresh-2", + "refresh-z", + ]); + }); + /** An access token that names one ChatGPT account, the unit the backend meters. */ const accessTokenFor = (chatgptAccountId: string): string => encodeJwt({ [JWT_CLAIM_PATH]: { chatgpt_account_id: chatgptAccountId } }); From 7f1f870665326829f8966a569a14632b947861b2 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 13:09:46 -0500 Subject: [PATCH 10/15] fix(codex-list): stop the account table cutting off the seat it prints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `codex-list` renders two ways. Its default v2 output prints the account label in full, so the `seat:` suffix reaches the screen. Its plain-table output - the `CODEX_TUI_V2=0` path - pins the Label column at 42 characters and truncates the cell to fit. A full Business-seat identity is 66: Account 10 (name@example.com, id:05cd9f04...989a40, seat:989a40) so that cell was cut mid-`id:` and the seat never appeared at all: 1 Account 1 (shared@example.com, id:05cd9f0… unknown active 2 Account 2 (shared@example.com, id:05cd9f0… unknown ok Two members of one workspace still rendered as one identical string, which is the exact symptom the seat suffix exists to remove - the column was simply too narrow to show the field that distinguishes them. It is now 68, which fits the whole identity and leaves the four-column row at 112 characters. `codex-status` keeps its 42-wide Label. That table carries seven columns, so widening it the same way would produce a 149-character row: a readability cost paid on a surface that is not the one that lists accounts, and its default v2 output already prints the label untruncated. The regression test drives the real `codex-list` tool, and therefore the real `formatCommandAccountLabel` closure rather than one of the hand-written stand-ins in the tool suites. That is why it sees a truncation the unit-level label tests cannot: they assert on the formatter's return value, which was already correct. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- lib/tools/codex-list.ts | 7 +++++- test/index.test.ts | 52 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/lib/tools/codex-list.ts b/lib/tools/codex-list.ts index a8922be0..61ffda38 100644 --- a/lib/tools/codex-list.ts +++ b/lib/tools/codex-list.ts @@ -289,7 +289,12 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { const listTableOptions: TableOptions = { columns: [ { header: "#", width: 3 }, - { header: "Label", width: 42 }, + // Wide enough for a full Business-seat identity - "Account 10 + // (name@example.com, id:05cd9f04...989a40, seat:989a40)" is 66 + // characters. At 42 the cell truncated mid-`id:`, so two members + // of one workspace rendered as the same cut-off string and the + // seat that tells them apart never reached the screen. + { header: "Label", width: 68 }, { header: "Plan", width: 18 }, { header: "Status", width: 20 }, ], diff --git a/test/index.test.ts b/test/index.test.ts index 454cab49..7f932225 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -3916,6 +3916,58 @@ describe("OpenAIOAuthPlugin", () => { } }); }); + + // Every member of a ChatGPT Business workspace shares its `accountId`, so + // rendering that alone gave distinct members one identical `id:` and read as + // a single account duplicated. These drive the REAL `codex-list`, and so the + // real `formatCommandAccountLabel` closure behind every `codex-*` tool. + describe("seat identity across account-display surfaces", () => { + const setMaskEmail = async (value: boolean) => { + const configModule = await import("../lib/config.js"); + vi.mocked(configModule.getCodexTuiMaskEmail).mockReturnValue(value); + }; + + it("codex-list: distinguishes two seats sharing one workspace account id", async () => { + await setMaskEmail(false); + // Same email AND same workspace id on both rows, so the seat is the + // only thing that can tell them apart. + mockStorage.accounts = [ + { + refreshToken: "r1", + email: "shared@example.com", + accountId: "05cd9f040000000000989a40", + accountUserId: "user_aaaaaa111111", + }, + { + refreshToken: "r2", + email: "shared@example.com", + accountId: "05cd9f040000000000989a40", + accountUserId: "user_bbbbbb222222", + }, + ]; + + const output = (await plugin.tool["codex-list"].execute()) as string; + + expect(output).toContain("seat:111111"); + expect(output).toContain("seat:222222"); + }); + + it("codex-list: renders no seat for an account with no member id", async () => { + await setMaskEmail(false); + mockStorage.accounts = [ + { + refreshToken: "r1", + email: "solo@example.com", + accountId: "05cd9f040000000000989a40", + }, + ]; + + const output = (await plugin.tool["codex-list"].execute()) as string; + + expect(output).toContain("solo@example.com"); + expect(output).not.toContain("seat:"); + }); + }); }); describe("OpenAIOAuthPlugin edge cases", () => { From 40a0ee3d7f4814aa2e90cde80e812b39dbca4165 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 14:03:09 -0500 Subject: [PATCH 11/15] fix(accounts): size the seat suffix to tell the listed seats apart The seat suffix was a fixed 6-character tail of `accountUserId`, which is not an identity. Two members of one workspace whose ids end the same way rendered identically: member-000001 -> seat:000001 other-000001 -> seat:000001 So the display could still claim two accounts are one - the exact false reading this suffix was added to prevent, reintroduced one layer down. This is not hypothetical: a diagnostic written against these same 6-character tails reported that nine distinct seats "collapse to five identities", which was wrong, and the ids it collapsed were real. `formatSeatSuffix` now takes the other accounts being rendered beside this one and returns the shortest tail, at least 6 characters, that renders every distinct member id in that set differently. `member-000001` and `other-000001` become `ber-000001` and `her-000001`; ids that already differ at 6 stay at 6, so the common case is unchanged. The rendered id joins the measured set itself, so the guarantee holds whether a caller passes all the accounts or only the other ones. The search terminates: it stops at the longest id present, and at that length every id is rendered whole, which is distinct by definition. `resolveSeatSuffixes` gives a whole list one shared length so rows line up, and returns `undefined` in place for records with no member id. Every surface that renders an account identity now passes its peers - `formatAccountLabel`, the `formatCommandAccountLabel` closure behind all 24 `codex-*` tools, `buildJsonAccountIdentity`, the interactive auth menu, the fallback login menu, and the standalone CLI summary. A surface that rendered one account without its peers would fall back to 6 characters and could still collide, which is why the threading is exhaustive rather than only where a collision was observed. The standalone CLI keeps its own masking rule: the seat is disclosed no more than `accountId` beside it, except where a longer tail is what tells two seats apart. An account with no `accountUserId` renders byte-for-byte as before. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- README.md | 2 +- index.ts | 30 ++++++-- lib/account-display.ts | 82 ++++++++++++++++++++- lib/accounts.ts | 10 ++- lib/cli.ts | 19 ++++- lib/tools/codex-dashboard.ts | 5 +- lib/tools/codex-health.ts | 9 ++- lib/tools/codex-label.ts | 1 + lib/tools/codex-limits.ts | 3 +- lib/tools/codex-list.ts | 11 ++- lib/tools/codex-note.ts | 5 +- lib/tools/codex-pool.ts | 7 +- lib/tools/codex-refresh.ts | 5 +- lib/tools/codex-remove.ts | 1 + lib/tools/codex-reset.ts | 5 +- lib/tools/codex-status.ts | 12 ++- lib/tools/codex-switch.ts | 1 + lib/tools/codex-tag.ts | 5 +- lib/tools/codex-warm.ts | 1 + lib/tools/index.ts | 8 +- lib/ui/auth-menu.ts | 22 ++++-- scripts/install-oc-codex-multi-auth-core.js | 56 ++++++++++++-- test/account-display.test.ts | 65 ++++++++++++++++ test/accounts.test.ts | 22 ++++++ test/index.test.ts | 4 +- 25 files changed, 346 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 8bd7d83c..63e36674 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ Most of these also run as a **direct CLI** with no agent or model involvement, s - unsupported-model handling is strict by default, with opt-in fallback controls - TUI quota status follows the account/workspace used by the latest request - Business workspace memberships and Personal accounts keep separate usage and quota windows. Business members sharing one workspace are distinguished by their member/seat identity, so their usage is not collapsed into one row. -- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints the last 6 characters of that as `seat:`. Two seats in one workspace are therefore told apart on screen instead of rendering one identical `id:`. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. +- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints a tail of that as `seat:`. The tail is 6 characters where that is enough to tell the listed accounts apart and grows to the shortest length that does when it is not, so two distinct seats never render the same `seat:`. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. - The ChatGPT plan (`Free`, `Plus`, `Pro`, `Business`, `Business Premium`, `Enterprise`) is read from the access token, refreshed on every token refresh, and shown by `codex-list` and `codex-status`. `codex-limits` and the TUI read the plan live from the usage endpoint and name it the same way. An unrecognized plan is reported verbatim rather than renamed. --- diff --git a/index.ts b/index.ts index 8780bc84..812c6a57 100644 --- a/index.ts +++ b/index.ts @@ -479,11 +479,13 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { account?: { email?: string; accountId?: string; + accountUserId?: string; accountLabel?: string; accountTags?: string[]; accountNote?: string; }; label?: string; + peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; } = {}, ): Record => ({ index: index + 1, @@ -491,7 +493,10 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { ...(options.includeSensitive ? { label: - options.label ?? formatCommandAccountLabel(options.account, index), + options.label ?? + formatCommandAccountLabel(options.account, index, { + peerAccounts: options.peerAccounts, + }), email: options.account?.email ?? null, accountId: options.account?.accountId ?? null, } @@ -1280,12 +1285,18 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { accountNote?: string; } | undefined, index: number, - options: { maskEmail?: boolean } = {}, + options: { + maskEmail?: boolean; + peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; + } = {}, ): string => { const email = resolveDisplayEmail(account?.email, options.maskEmail ?? false); const workspace = account?.accountLabel?.trim(); const accountId = formatAccountIdForDisplay(account?.accountId); - const seat = formatSeatSuffix(account?.accountUserId); + const seat = formatSeatSuffix( + account?.accountUserId, + options.peerAccounts?.map((peer) => peer?.accountUserId), + ); const tags = Array.isArray(account?.accountTags) ? account.accountTags @@ -1339,7 +1350,10 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const maskEmail = resolveMaskEmail(); const selected = await select( storage.accounts.map((account, index) => ({ - label: formatCommandAccountLabel(account, index, { maskEmail }), + label: formatCommandAccountLabel(account, index, { + maskEmail, + peerAccounts: storage.accounts, + }), value: index, })), { @@ -1364,7 +1378,9 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { ): BeginnerAccountSnapshot[] => { return storage.accounts.map((account, index) => ({ index, - label: formatCommandAccountLabel(account, index), + label: formatCommandAccountLabel(account, index, { + peerAccounts: storage.accounts, + }), accountLabel: account.accountLabel, enabled: account.enabled !== false, isActive: index === activeIndex, @@ -2929,6 +2945,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { const failures = await accountManager.incrementAuthFailures(account); const accountLabel = formatAccountLabel(account, account.index, { maskEmail: maskEmailEnabled, + peerAccounts: accountManager.getAccountsSnapshot(), }); if (failures >= ACCOUNT_LIMITS.MAX_AUTH_FAILURES_BEFORE_REMOVAL) { @@ -3011,6 +3028,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { ) { const accountLabel = formatAccountLabel(account, account.index, { maskEmail: maskEmailEnabled, + peerAccounts: accountManager.getAccountsSnapshot(), }); await showToast( `Using ${accountLabel} (${account.index + 1}/${accountCount})`, @@ -3271,6 +3289,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { if (workspaceDeactivated) { const accountLabel = formatAccountLabel(account, account.index, { maskEmail: maskEmailEnabled, + peerAccounts: accountManager.getAccountsSnapshot(), }); accountManager.refundToken(account, modelFamily, model); accountManager.recordFailure(account, modelFamily, model); @@ -3586,6 +3605,7 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { if (isInvalidatedAuthTokenError(errorBody, response.status)) { const accountLabel = formatAccountLabel(account, account.index, { maskEmail: maskEmailEnabled, + peerAccounts: accountManager.getAccountsSnapshot(), }); accountManager.refundToken(account, modelFamily, model); accountManager.recordFailure(account, modelFamily, model); diff --git a/lib/account-display.ts b/lib/account-display.ts index d845dde9..fb772030 100644 --- a/lib/account-display.ts +++ b/lib/account-display.ts @@ -48,6 +48,47 @@ export function resolveDisplayEmail( return maskEmail ? maskEmailForDisplay(trimmed) : trimmed; } +const SEAT_SUFFIX_MIN_LENGTH = 6; + +function normalizeSeatIdentity(accountUserId: string | undefined): string | undefined { + const trimmed = accountUserId?.trim(); + return trimmed && trimmed.length > 0 ? trimmed : undefined; +} + +function sliceSeatSuffix(accountUserId: string, length: number): string { + return accountUserId.length > length ? accountUserId.slice(-length) : accountUserId; +} + +/** + * Shortest tail, at least six characters, that renders every distinct member + * id in `accountUserIds` as a different string. + * + * It terminates because the search stops at the longest id present, and at + * that length every id is rendered whole - distinct strings by definition. So + * a length always exists, and the first one found is the shortest. + */ +function resolveSeatSuffixLength(accountUserIds: readonly (string | undefined)[]): number { + const distinct = new Set(); + for (const accountUserId of accountUserIds) { + const normalized = normalizeSeatIdentity(accountUserId); + if (normalized) distinct.add(normalized); + } + if (distinct.size <= 1) return SEAT_SUFFIX_MIN_LENGTH; + + let longest = SEAT_SUFFIX_MIN_LENGTH; + for (const accountUserId of distinct) { + longest = Math.max(longest, accountUserId.length); + } + for (let length = SEAT_SUFFIX_MIN_LENGTH; length < longest; length += 1) { + const rendered = new Set(); + for (const accountUserId of distinct) { + rendered.add(sliceSeatSuffix(accountUserId, length)); + } + if (rendered.size === distinct.size) return length; + } + return longest; +} + /** * Render the short seat suffix that separates two accounts sharing one * workspace `accountId`. @@ -63,12 +104,45 @@ export function resolveDisplayEmail( * same account duplicated four times. Appending this suffix is what makes the * rendered rows match the accounts they describe. * - * Six characters, matching what the surfaces already print for `accountId`. + * Six characters by default, matching what the surfaces already print for + * `accountId`. Six is not unique on its own - real member ids were observed + * sharing a six-character tail, which is the same false "these are duplicates" + * reading this suffix exists to prevent - so pass `peerAccountUserIds` (the + * other accounts rendered alongside this one) and the suffix grows to whatever + * length tells them all apart. + * * Returns `undefined` when there is no member id, so a token-only record * renders exactly as it did before. */ -export function formatSeatSuffix(accountUserId: string | undefined): string | undefined { - const trimmed = accountUserId?.trim(); +export function formatSeatSuffix( + accountUserId: string | undefined, + peerAccountUserIds?: readonly (string | undefined)[], +): string | undefined { + const trimmed = normalizeSeatIdentity(accountUserId); if (!trimmed) return undefined; - return trimmed.length > 6 ? trimmed.slice(-6) : trimmed; + return sliceSeatSuffix( + trimmed, + peerAccountUserIds + // This id joins the set the length is measured against, so the + // guarantee holds even for a caller whose peer list is the OTHER + // accounts rather than all of them. A set makes the common case, + // where it is already there, a no-op. + ? resolveSeatSuffixLength([...peerAccountUserIds, trimmed]) + : SEAT_SUFFIX_MIN_LENGTH, + ); +} + +/** + * Seat suffixes for a whole rendered set, all cut to one length so the rows + * line up and no two distinct member ids share a rendering. Entries without a + * member id come back `undefined`, holding their position. + */ +export function resolveSeatSuffixes( + accountUserIds: readonly (string | undefined)[], +): (string | undefined)[] { + const length = resolveSeatSuffixLength(accountUserIds); + return accountUserIds.map((accountUserId) => { + const trimmed = normalizeSeatIdentity(accountUserId); + return trimmed ? sliceSeatSuffix(trimmed, length) : undefined; + }); } diff --git a/lib/accounts.ts b/lib/accounts.ts index c1991eee..70e27b68 100644 --- a/lib/accounts.ts +++ b/lib/accounts.ts @@ -428,7 +428,10 @@ export function formatAccountLabel( | { email?: string; accountId?: string; accountUserId?: string; accountLabel?: string } | undefined, index: number, - options: { maskEmail?: boolean } = {}, + options: { + maskEmail?: boolean; + peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; + } = {}, ): string { const accountLabel = account?.accountLabel?.trim(); const email = resolveDisplayEmail(account?.email, options.maskEmail ?? false); @@ -441,7 +444,10 @@ export function formatAccountLabel( // `accountId` names the workspace, which every member of a Business // workspace shares. Without the seat, four distinct members printed one // identical `id:` string (see `formatSeatSuffix`). - const seatSuffix = formatSeatSuffix(account?.accountUserId); + const seatSuffix = formatSeatSuffix( + account?.accountUserId, + options.peerAccounts?.map((peer) => peer?.accountUserId), + ); const details: string[] = []; if (accountLabel) details.push(accountLabel); diff --git a/lib/cli.ts b/lib/cli.ts index d8049db4..9b251cce 100644 --- a/lib/cli.ts +++ b/lib/cli.ts @@ -78,7 +78,10 @@ export interface LoginMenuResult { function formatAccountLabel( account: ExistingAccountInfo, index: number, - options: { maskEmail?: boolean } = {}, + options: { + maskEmail?: boolean; + peerAccounts?: readonly ExistingAccountInfo[]; + } = {}, ): string { const num = index + 1; const label = account.accountLabel?.trim(); @@ -88,7 +91,10 @@ function formatAccountLabel( accountId && accountId.length > 14 ? `${accountId.slice(0, 8)}...${accountId.slice(-6)}` : accountId; - const seatSuffix = formatSeatSuffix(account.accountUserId); + const seatSuffix = formatSeatSuffix( + account.accountUserId, + options.peerAccounts?.map((peer) => peer.accountUserId), + ); const details: string[] = []; if (email) details.push(email); if (label) details.push(`workspace:${label}`); @@ -119,7 +125,9 @@ async function promptLoginModeFallback( if (existingAccounts.length > 0) { console.log(`\n${existingAccounts.length} account(s) saved:`); for (const account of existingAccounts) { - console.log(` ${formatAccountLabel(account, account.index, { maskEmail })}`); + console.log( + ` ${formatAccountLabel(account, account.index, { maskEmail, peerAccounts: existingAccounts })}`, + ); } console.log(""); } @@ -176,7 +184,10 @@ export async function promptLoginMode( case "verify-flagged": return { mode: "verify-flagged" }; case "select-account": { - const accountAction = await showAccountDetails(action.account, { maskEmail }); + const accountAction = await showAccountDetails(action.account, { + maskEmail, + peerAccounts: existingAccounts, + }); if (accountAction === "delete") { return { mode: "manage", deleteAccountIndex: action.account.index }; } diff --git a/lib/tools/codex-dashboard.ts b/lib/tools/codex-dashboard.ts index 71b4db2f..b349d7ee 100644 --- a/lib/tools/codex-dashboard.ts +++ b/lib/tools/codex-dashboard.ts @@ -140,6 +140,7 @@ export function createCodexDashboardTool(ctx: ToolContext): ToolDefinition { ...buildJsonAccountIdentity(entry.index, { includeSensitive: includeSensitiveOutput, account: storage.accounts[entry.index], + peerAccounts: storage.accounts, }), eligible: entry.eligible, healthScore: entry.healthScore, @@ -196,7 +197,7 @@ export function createCodexDashboardTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel( storage.accounts[entry.index], entry.index, - { maskEmail }, + { maskEmail, peerAccounts: storage.accounts }, ); const state = entry.eligible ? formatUiBadge(ui, "eligible", "success") @@ -252,7 +253,7 @@ export function createCodexDashboardTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel( storage.accounts[entry.index], entry.index, - { maskEmail }, + { maskEmail, peerAccounts: storage.accounts }, ); lines.push( ` - ${label}: ${entry.eligible ? "eligible" : "blocked"} | health=${Math.round(entry.healthScore)} | tokens=${entry.tokensAvailable.toFixed(1)} | reasons=${entry.reasons.join(", ")}`, diff --git a/lib/tools/codex-health.ts b/lib/tools/codex-health.ts index 05820175..c3658bb4 100644 --- a/lib/tools/codex-health.ts +++ b/lib/tools/codex-health.ts @@ -94,8 +94,13 @@ export function createCodexHealthTool(ctx: ToolContext): ToolDefinition { const account = storage.accounts[i]; if (!input || !account) continue; - const label = formatCommandAccountLabel(account, i); - const displayLabel = formatCommandAccountLabel(account, i, { maskEmail }); + const label = formatCommandAccountLabel(account, i, { + peerAccounts: storage.accounts, + }); + const displayLabel = formatCommandAccountLabel(account, i, { + maskEmail, + peerAccounts: storage.accounts, + }); const outcome = await refreshAndPersistAccount(input); if (outcome.status === "refreshed") { diff --git a/lib/tools/codex-label.ts b/lib/tools/codex-label.ts index 588f199e..38dad374 100644 --- a/lib/tools/codex-label.ts +++ b/lib/tools/codex-label.ts @@ -163,6 +163,7 @@ export function createCodexLabelTool(ctx: ToolContext): ToolDefinition { const accountLabel = formatCommandAccountLabel(account, targetIndex, { maskEmail, + peerAccounts: accounts, }); return { kind: "ok", accountLabel, previousLabel }; }, diff --git a/lib/tools/codex-limits.ts b/lib/tools/codex-limits.ts index 62daa5e0..bb61f803 100644 --- a/lib/tools/codex-limits.ts +++ b/lib/tools/codex-limits.ts @@ -174,11 +174,12 @@ export function createCodexLimitsTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel( effectiveDisplayAccount, displayIndex, + { peerAccounts: storage.accounts }, ); const displayLabel = formatCommandAccountLabel( effectiveDisplayAccount, displayIndex, - { maskEmail }, + { maskEmail, peerAccounts: storage.accounts }, ); const isActive = i === activeIndex || sharesActiveCredential; const activeSuffix = isActive diff --git a/lib/tools/codex-list.ts b/lib/tools/codex-list.ts index 61ffda38..ed59d562 100644 --- a/lib/tools/codex-list.ts +++ b/lib/tools/codex-list.ts @@ -179,6 +179,7 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { ...buildJsonAccountIdentity(index, { includeSensitive: includeSensitiveOutput, account, + peerAccounts: storage.accounts, }), enabled: account.enabled !== false, isActive: index === activeIndex, @@ -210,7 +211,10 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { ]; filteredEntries.forEach(({ account, index }) => { - const label = formatCommandAccountLabel(account, index, { maskEmail }); + const label = formatCommandAccountLabel(account, index, { + maskEmail, + peerAccounts: storage.accounts, + }); const badges: string[] = []; if (index === activeIndex) badges.push(formatUiBadge(ui, "current", "accent")); @@ -307,7 +311,10 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { ]; filteredEntries.forEach(({ account, index }) => { - const label = formatCommandAccountLabel(account, index, { maskEmail }); + const label = formatCommandAccountLabel(account, index, { + maskEmail, + peerAccounts: storage.accounts, + }); const statuses: string[] = []; const rateLimit = formatRateLimitEntry(account, now); const quotaExhausted = formatQuotaExhaustionEntry(account, now); diff --git a/lib/tools/codex-note.ts b/lib/tools/codex-note.ts index 80e10826..173e02e3 100644 --- a/lib/tools/codex-note.ts +++ b/lib/tools/codex-note.ts @@ -104,7 +104,10 @@ export function createCodexNoteTool(ctx: ToolContext): ToolDefinition { accountManagerPromiseRef.current = Promise.resolve(reloadedManager); } - const accountLabel = formatCommandAccountLabel(persistedAccount, targetIndex, { maskEmail }); + const accountLabel = formatCommandAccountLabel(persistedAccount, targetIndex, { + maskEmail, + peerAccounts: storage.accounts, + }); if (normalizedNote.length === 0) { return `Cleared note for ${accountLabel}`; } diff --git a/lib/tools/codex-pool.ts b/lib/tools/codex-pool.ts index 2b49f686..1e01297b 100644 --- a/lib/tools/codex-pool.ts +++ b/lib/tools/codex-pool.ts @@ -164,7 +164,10 @@ function buildPoolSnapshot( ...ctx.buildJsonAccountIdentity(index, { includeSensitive, account, - label: ctx.formatCommandAccountLabel(account, index, { maskEmail }), + label: ctx.formatCommandAccountLabel(account, index, { + maskEmail, + peerAccounts: storedAccounts, + }), }), enabled: account.enabled !== false, }); @@ -197,7 +200,7 @@ function renderPoolStatusText( const account = storage?.accounts[index]; if (account) { lines.push( - ` ${ctx.formatCommandAccountLabel(account, index, { maskEmail })}${ + ` ${ctx.formatCommandAccountLabel(account, index, { maskEmail, peerAccounts: storage?.accounts })}${ account.enabled === false ? " [disabled]" : "" }`, ); diff --git a/lib/tools/codex-refresh.ts b/lib/tools/codex-refresh.ts index a9872809..541f7502 100644 --- a/lib/tools/codex-refresh.ts +++ b/lib/tools/codex-refresh.ts @@ -53,7 +53,10 @@ export function createCodexRefreshTool(ctx: ToolContext): ToolDefinition { const input = inputs[i]; const account = storage.accounts[i]; if (!input || !account) continue; - const label = formatCommandAccountLabel(account, i, { maskEmail }); + const label = formatCommandAccountLabel(account, i, { + maskEmail, + peerAccounts: storage.accounts, + }); const outcome = await refreshAndPersistAccount(input); if (outcome.status === "refreshed") { diff --git a/lib/tools/codex-remove.ts b/lib/tools/codex-remove.ts index 38e06855..d8ac1572 100644 --- a/lib/tools/codex-remove.ts +++ b/lib/tools/codex-remove.ts @@ -152,6 +152,7 @@ export function createCodexRemoveTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel(account, targetIndex, { maskEmail, + peerAccounts: accounts, }); storage.accounts.splice(targetIndex, 1); diff --git a/lib/tools/codex-reset.ts b/lib/tools/codex-reset.ts index 7c235fa1..a5497fdc 100644 --- a/lib/tools/codex-reset.ts +++ b/lib/tools/codex-reset.ts @@ -211,9 +211,12 @@ export function createCodexResetTool(ctx: ToolContext): ToolDefinition { if (!target) { throw new Error(`No account at position ${index + 1}.`); } - const label = formatCommandAccountLabel(target, index); + const label = formatCommandAccountLabel(target, index, { + peerAccounts: storage.accounts, + }); const displayLabel = formatCommandAccountLabel(target, index, { maskEmail, + peerAccounts: storage.accounts, }); const identity = buildJsonAccountIdentity(index, { includeSensitive: includeSensitiveOutput, diff --git a/lib/tools/codex-status.ts b/lib/tools/codex-status.ts index e2b680c7..0fce45b8 100644 --- a/lib/tools/codex-status.ts +++ b/lib/tools/codex-status.ts @@ -141,6 +141,7 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { ...buildJsonAccountIdentity(index, { includeSensitive: includeSensitiveOutput, account, + peerAccounts: storage.accounts, }), enabled: account.enabled !== false, isActive: index === activeIndex, @@ -166,6 +167,7 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { ...buildJsonAccountIdentity(index, { includeSensitive: includeSensitiveOutput, account, + peerAccounts: storage.accounts, }), families: Object.fromEntries( MODEL_FAMILIES.map((family) => { @@ -207,7 +209,10 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { ]; storage.accounts.forEach((account, index) => { - const label = formatCommandAccountLabel(account, index, { maskEmail }); + const label = formatCommandAccountLabel(account, index, { + maskEmail, + peerAccounts: storage.accounts, + }); const badges: string[] = []; if (index === activeIndex) badges.push(formatUiBadge(ui, "active", "accent")); @@ -319,7 +324,10 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { ]; storage.accounts.forEach((account, index) => { - const label = formatCommandAccountLabel(account, index, { maskEmail }); + const label = formatCommandAccountLabel(account, index, { + maskEmail, + peerAccounts: storage.accounts, + }); const active = index === activeIndex ? "Yes" : "No"; const rateLimit = formatRateLimitEntry(account, now) ?? "None"; const cooldown = formatCooldown(account, now) ?? "No"; diff --git a/lib/tools/codex-switch.ts b/lib/tools/codex-switch.ts index 54c805df..71d74c45 100644 --- a/lib/tools/codex-switch.ts +++ b/lib/tools/codex-switch.ts @@ -130,6 +130,7 @@ export function createCodexSwitchTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel(account, targetIndex, { maskEmail, + peerAccounts: accounts, }); try { await persist(storage); diff --git a/lib/tools/codex-tag.ts b/lib/tools/codex-tag.ts index efd7db34..9cf5819d 100644 --- a/lib/tools/codex-tag.ts +++ b/lib/tools/codex-tag.ts @@ -126,7 +126,10 @@ export function createCodexTagTool(ctx: ToolContext): ToolDefinition { accountManagerPromiseRef.current = Promise.resolve(reloadedManager); } - const accountLabel = formatCommandAccountLabel(persistedAccount, targetIndex, { maskEmail }); + const accountLabel = formatCommandAccountLabel(persistedAccount, targetIndex, { + maskEmail, + peerAccounts: storage.accounts, + }); const previousText = previousTags.length > 0 ? previousTags.join(", ") : "none"; const nextText = diff --git a/lib/tools/codex-warm.ts b/lib/tools/codex-warm.ts index d2cf632e..05dcb84c 100644 --- a/lib/tools/codex-warm.ts +++ b/lib/tools/codex-warm.ts @@ -144,6 +144,7 @@ export function createCodexWarmTool(ctx: ToolContext): ToolDefinition { const account = storage.accounts[result.index]; const label = formatCommandAccountLabel(account, result.index, { maskEmail, + peerAccounts: storage.accounts, }); if (result.status === "warmed") { lines.push(` ${getStatusMarker(ui, "ok")} ${label}: Window started`); diff --git a/lib/tools/index.ts b/lib/tools/index.ts index 6fa83d8e..d50c0edb 100644 --- a/lib/tools/index.ts +++ b/lib/tools/index.ts @@ -109,13 +109,17 @@ export interface ToolContext { | { email?: string; accountId?: string; + accountUserId?: string; accountLabel?: string; accountTags?: string[]; accountNote?: string; } | undefined, index: number, - options?: { maskEmail?: boolean }, + options?: { + maskEmail?: boolean; + peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; + }, ) => string; resolveMaskEmail: () => boolean; normalizeAccountTags: (raw: string) => string[]; @@ -154,11 +158,13 @@ export interface ToolContext { account?: { email?: string; accountId?: string; + accountUserId?: string; accountLabel?: string; accountTags?: string[]; accountNote?: string; }; label?: string; + peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; }, ) => Record; buildRoutingVisibilitySnapshot: (overrides?: { diff --git a/lib/ui/auth-menu.ts b/lib/ui/auth-menu.ts index 38760f95..b1b17457 100644 --- a/lib/ui/auth-menu.ts +++ b/lib/ui/auth-menu.ts @@ -111,7 +111,11 @@ function formatAccountIdSuffix(accountId: string | undefined): string | undefine : trimmed; } -function accountTitle(account: AccountInfo, maskEmail = false): string { +function accountTitle( + account: AccountInfo, + maskEmail = false, + peerAccounts?: readonly AccountInfo[], +): string { const email = resolveDisplayEmail(account.email, maskEmail); const label = account.accountLabel?.trim(); const accountIdSuffix = formatAccountIdSuffix(account.accountId); @@ -122,7 +126,10 @@ function accountTitle(account: AccountInfo, maskEmail = false): string { if (accountIdSuffix && (!label || !label.includes(accountIdSuffix))) { details.push(`id:${accountIdSuffix}`); } - const seatSuffix = formatSeatSuffix(account.accountUserId); + const seatSuffix = formatSeatSuffix( + account.accountUserId, + peerAccounts?.map((peer) => peer.accountUserId), + ); if (seatSuffix) details.push(`seat:${seatSuffix}`); if (details.length === 0) { @@ -162,7 +169,7 @@ export async function showAuthMenu( ? (ui.v2Enabled ? ` ${formatUiBadge(ui, "disabled", "danger")}` : ` ${ANSI.red}[disabled]${ANSI.reset}`) : ""; const statusSuffix = badge ? ` ${badge}` : ""; - const label = `${accountTitle(account, maskEmail)}${currentBadge}${statusSuffix}${disabledBadge}`; + const label = `${accountTitle(account, maskEmail, accounts)}${currentBadge}${statusSuffix}${disabledBadge}`; return { label: ui.v2Enabled ? paintUiText(ui, label, "heading") : label, hint: `used ${formatRelativeTime(account.lastUsed)}`, @@ -194,12 +201,13 @@ export async function showAuthMenu( export async function showAccountDetails( account: AccountInfo, - options: { maskEmail?: boolean } = {}, + options: { maskEmail?: boolean; peerAccounts?: readonly AccountInfo[] } = {}, ): Promise { const ui = getUiRuntimeOptions(); const maskEmail = options.maskEmail ?? false; + const peerAccounts = options.peerAccounts; const header = - `${accountTitle(account, maskEmail)} ${statusBadge(account.status)}` + + `${accountTitle(account, maskEmail, peerAccounts)} ${statusBadge(account.status)}` + (account.enabled === false ? (ui.v2Enabled ? ` ${formatUiBadge(ui, "disabled", "danger")}` @@ -230,11 +238,11 @@ export async function showAccountDetails( if (!action) return "cancel"; if (action === "delete") { - const confirmed = await confirm(`Delete ${accountTitle(account, maskEmail)}?`); + const confirmed = await confirm(`Delete ${accountTitle(account, maskEmail, peerAccounts)}?`); if (!confirmed) continue; } if (action === "refresh") { - const confirmed = await confirm(`Re-authenticate ${accountTitle(account, maskEmail)}?`); + const confirmed = await confirm(`Re-authenticate ${accountTitle(account, maskEmail, peerAccounts)}?`); if (!confirmed) continue; } return action; diff --git a/scripts/install-oc-codex-multi-auth-core.js b/scripts/install-oc-codex-multi-auth-core.js index b5e26cd2..16a99221 100644 --- a/scripts/install-oc-codex-multi-auth-core.js +++ b/scripts/install-oc-codex-multi-auth-core.js @@ -410,22 +410,68 @@ function accountIdSuffix(accountId, includeSensitive) { return accountId.slice(-4); } +// A member id is what tells two seats of one Business workspace apart, and a +// fixed-length tail does not always do it: real member ids were observed +// sharing a six-character tail, which prints two different seats as one - the +// exact misreading the seat exists to prevent. So the length grows until every +// seat printed in this run is distinct, mirroring `resolveSeatSuffixes` in +// lib/account-display.ts. It starts at the length the mask above allows, so +// masked output lengthens only when leaving it short would print a lie. +function seatSuffixAtLength(accountUserId, includeSensitive, length) { + if (!accountUserId) return undefined; + if (!includeSensitive && accountUserId.length < MASK_MIN_LENGTH) return undefined; + return accountUserId.length > length ? accountUserId.slice(-length) : accountUserId; +} + +function resolveStandaloneSeatLength(accountUserIds, includeSensitive) { + const base = includeSensitive ? 6 : 4; + const distinct = new Set( + accountUserIds.filter( + (accountUserId) => + seatSuffixAtLength(accountUserId, includeSensitive, base) !== undefined, + ), + ); + if (distinct.size <= 1) return base; + + let longest = base; + for (const accountUserId of distinct) { + longest = Math.max(longest, accountUserId.length); + } + for (let length = base; length < longest; length += 1) { + const rendered = new Set( + [...distinct].map((accountUserId) => + seatSuffixAtLength(accountUserId, includeSensitive, length), + ), + ); + if (rendered.size === distinct.size) return length; + } + return longest; +} + function summarizeStandaloneAccounts(storage, includeSensitive, tag) { const accounts = Array.isArray(storage?.accounts) ? storage.accounts : []; const normalizedTag = typeof tag === "string" ? tag.trim().toLowerCase() : ""; - return accounts + const entries = accounts .map((account, index) => ({ account, index })) .filter(({ account }) => !normalizedTag || (Array.isArray(account?.accountTags) && - account.accountTags.some((entry) => String(entry).toLowerCase() === normalizedTag))) + account.accountTags.some((entry) => String(entry).toLowerCase() === normalizedTag))); + const seatLength = resolveStandaloneSeatLength( + entries.map(({ account }) => + (typeof account?.accountUserId === "string" ? account.accountUserId.trim() : "") || undefined, + ), + includeSensitive, + ); + return entries .map(({ account, index }) => { const trimmedId = typeof account?.accountId === "string" ? account.accountId.trim() : ""; const accountId = trimmedId || undefined; // Members of one Business workspace share `accountId`, so the seat is // what tells them apart. It is carried masked next to its suffix for - // the same reason `accountId` is: so the printed `seat:` never - // discloses more than the field beside it. + // the same reason `accountId` is: so the printed `seat:` discloses no + // more of an id than the field beside it unless telling two seats + // apart requires it. const trimmedUserId = typeof account?.accountUserId === "string" ? account.accountUserId.trim() : ""; const accountUserId = trimmedUserId || undefined; @@ -436,7 +482,7 @@ function summarizeStandaloneAccounts(storage, includeSensitive, tag) { accountId: maskValue(accountId, includeSensitive), idSuffix: accountIdSuffix(accountId, includeSensitive), accountUserId: maskValue(accountUserId, includeSensitive), - seatSuffix: accountIdSuffix(accountUserId, includeSensitive), + seatSuffix: seatSuffixAtLength(accountUserId, includeSensitive, seatLength), accountIdSource: account?.accountIdSource, enabled: account?.enabled !== false, hasRefreshToken: typeof account?.refreshToken === "string" && account.refreshToken.length > 0, diff --git a/test/account-display.test.ts b/test/account-display.test.ts index 20173383..3a21e346 100644 --- a/test/account-display.test.ts +++ b/test/account-display.test.ts @@ -1,6 +1,8 @@ import { + formatSeatSuffix, maskEmailForDisplay, resolveDisplayEmail, + resolveSeatSuffixes, } from "../lib/account-display.js"; describe("account-display", () => { @@ -80,3 +82,66 @@ describe("email masking edge cases", () => { } }); }); + +describe("seat suffix", () => { + it("uses six characters when nothing else needs telling apart", () => { + expect(formatSeatSuffix("user_aaaaaa111111")).toBe("111111"); + expect(formatSeatSuffix("abc")).toBe("abc"); + expect(formatSeatSuffix(undefined)).toBeUndefined(); + expect(formatSeatSuffix(" ")).toBeUndefined(); + }); + + // Six characters is a tail, not an identity. These two member ids are + // different seats - different quota, different weekly reset - that happen + // to end the same way, so a fixed six-character suffix renders both as + // `000001` and reports two accounts as one. + it("grows past six characters when distinct ids share a six-character tail", () => { + const ids = ["member-000001", "other-000001"]; + + expect(resolveSeatSuffixes(ids)).toEqual(["ber-000001", "her-000001"]); + expect(formatSeatSuffix("member-000001", ids)).toBe("ber-000001"); + expect(formatSeatSuffix("other-000001", ids)).toBe("her-000001"); + }); + + it("never renders two distinct member ids the same way", () => { + const cases: string[][] = [ + ["member-000001", "other-000001"], + ["aaaaaa", "bbbbbb"], + ["prefix-a-xxxxxx", "prefix-b-xxxxxx", "prefix-c-xxxxxx"], + ["short", "a-very-long-member-identifier-short"], + ["x".repeat(40), `y${"x".repeat(39)}`], + ]; + + for (const ids of cases) { + const rendered = resolveSeatSuffixes(ids).filter( + (seat): seat is string => seat !== undefined, + ); + expect(new Set(rendered).size, `rendering ${ids.join(" / ")}`).toBe( + new Set(ids).size, + ); + } + }); + + it("holds the position of entries that have no member id", () => { + expect(resolveSeatSuffixes(["member-000001", undefined, "other-000001"])).toEqual([ + "ber-000001", + undefined, + "her-000001", + ]); + }); + + // Repeats of one id are one seat listed twice, not a collision to resolve, + // so they must not push every row into a longer rendering. + it("keeps six characters when the only repeats are the same id", () => { + expect(resolveSeatSuffixes(["user_aaaaaa111111", "user_aaaaaa111111"])).toEqual([ + "111111", + "111111", + ]); + }); + + // A caller passing the OTHER accounts rather than all of them still gets a + // suffix that separates this id from them. + it("counts the rendered id itself even when the peer list omits it", () => { + expect(formatSeatSuffix("member-000001", ["other-000001"])).toBe("ber-000001"); + }); +}); diff --git a/test/accounts.test.ts b/test/accounts.test.ts index 1704b6e9..98368ad5 100644 --- a/test/accounts.test.ts +++ b/test/accounts.test.ts @@ -842,6 +842,28 @@ describe("AccountManager", () => { expect(second).toBe("Account 7 (shared@example.com, id:989a40, seat:222222)"); }); + // Same index on both sides again, and now the member ids end identically: + // six characters renders both seats `000001`, so the label is only distinct + // if the suffix grows. `peerAccounts` is what tells the formatter which + // other accounts it has to stay distinguishable from. + it("renders distinct labels for two seats whose member ids share a six-character tail", () => { + const workspace = { + email: "shared@example.com", + accountId: "05cd9f040000000000989a40", + }; + const peerAccounts = [ + { ...workspace, accountUserId: "member-000001" }, + { ...workspace, accountUserId: "other-000001" }, + ]; + + const first = formatAccountLabel(peerAccounts[0], 6, { peerAccounts }); + const second = formatAccountLabel(peerAccounts[1], 6, { peerAccounts }); + + expect(first).not.toBe(second); + expect(first).toBe("Account 7 (shared@example.com, id:989a40, seat:ber-000001)"); + expect(second).toBe("Account 7 (shared@example.com, id:989a40, seat:her-000001)"); + }); + it("renders an account with no accountUserId exactly as before", () => { expect( formatAccountLabel({ email: "user@example.com", accountId: "abcdef123456" }, 0), diff --git a/test/index.test.ts b/test/index.test.ts index 7f932225..45be271e 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -5323,10 +5323,12 @@ describe("OpenAIOAuthPlugin fetch handler", () => { // The runtime label must be built with masking enabled. If the // `{ maskEmail }` option is dropped from this call site, this fails. + // Matched by containment so a later option added alongside it - the + // seat-disambiguating `peerAccounts` - does not read as a regression. expect(vi.mocked(accountsModule.formatAccountLabel)).toHaveBeenCalledWith( expect.anything(), expect.any(Number), - { maskEmail: true }, + expect.objectContaining({ maskEmail: true }), ); }); From 5d0510373c846bad5030675523a94333ea8366a2 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 14:05:01 -0500 Subject: [PATCH 12/15] fix(codex-list): give the seat a column a long email cannot push it out of MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plain-table output of `codex-list` and `codex-status` renders the account identity into one fixed-width cell that truncates from the right, and the seat sat at the end of it behind the email and the workspace label. Neither of those has a length bound, so any sufficiently long one pushes the seat past the cell's right edge and two members of one workspace go back to rendering as the same truncated string: 1 Account 1 (extremely.long.account.display.name@very-long-corp… 2 Account 2 (extremely.long.account.display.name@very-long-corp… Widening the cell does not fix this - it only moves the length at which it happens, which is what the previous 42 -> 68 widening did. A cell shared with an unbounded field cannot hold anything reliably. So the seat leaves the label and gets a column of its own, sized to the widest seat actually rendered. A column cannot be pushed out of by its neighbours, and one sized to its own contents never truncates what it holds - which matters because the suffix length is now variable, so a fixed seat width would clip exactly the ids that needed the extra characters. Accounts with no member id show `-`. The label keeps its own width and may still truncate an email or a label; that is cosmetic now rather than a loss of identity, which is why it is left alone. `formatCommandAccountLabel` takes `omitSeat` so these two callers do not print the seat twice. Every other surface - the v2 lists, the auth menus, the JSON output, runtime log lines - renders free-form text with no fixed-width cell, so the seat cannot be truncated there and they are unchanged. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- index.ts | 14 +++-- lib/tools/codex-list.ts | 29 ++++++++-- lib/tools/codex-status.ts | 16 ++++++ lib/tools/index.ts | 1 + test/index.test.ts | 116 ++++++++++++++++++++++++++++++++++++-- 5 files changed, 162 insertions(+), 14 deletions(-) diff --git a/index.ts b/index.ts index 812c6a57..d5bdaa87 100644 --- a/index.ts +++ b/index.ts @@ -1288,15 +1288,21 @@ export const OpenAIOAuthPlugin: Plugin = async ({ client }: PluginInput) => { options: { maskEmail?: boolean; peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; + omitSeat?: boolean; } = {}, ): string => { const email = resolveDisplayEmail(account?.email, options.maskEmail ?? false); const workspace = account?.accountLabel?.trim(); const accountId = formatAccountIdForDisplay(account?.accountId); - const seat = formatSeatSuffix( - account?.accountUserId, - options.peerAccounts?.map((peer) => peer?.accountUserId), - ); + // `omitSeat` is for a caller that renders the seat itself in a place + // a long email cannot push it out of - a table column of its own. + // Leaving it in the label too would print the seat twice. + const seat = options.omitSeat + ? undefined + : formatSeatSuffix( + account?.accountUserId, + options.peerAccounts?.map((peer) => peer?.accountUserId), + ); const tags = Array.isArray(account?.accountTags) ? account.accountTags diff --git a/lib/tools/codex-list.ts b/lib/tools/codex-list.ts index ed59d562..65a485ca 100644 --- a/lib/tools/codex-list.ts +++ b/lib/tools/codex-list.ts @@ -6,6 +6,7 @@ import { tool, type ToolDefinition } from "@opencode-ai/plugin/tool"; import { getStoragePath, loadAccounts } from "../storage.js"; import { formatCooldown } from "../accounts.js"; +import { resolveSeatSuffixes } from "../account-display.js"; import { buildTableHeader, buildTableRow, type TableOptions } from "../table-formatter.js"; import { formatUiBadge, @@ -290,15 +291,31 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { return lines.join("\n"); } + // The seat gets a column of its own, sized to the widest seat actually + // rendered. Kept inside the label it sat behind an email and a + // workspace label, neither of which has a length bound, so any long + // one pushed it past the cell's right edge and two members of one + // workspace went back to rendering as the same truncated string. A + // column cannot be pushed out of by its neighbours, and one sized to + // its own contents never truncates what it holds. + const seatSuffixes = resolveSeatSuffixes( + storage.accounts.map((entry) => entry.accountUserId), + ); + const seatHeader = "Seat"; + const seatWidth = filteredEntries.reduce( + (widest, { index }) => + Math.max(widest, seatSuffixes[index]?.length ?? 0), + seatHeader.length, + ); const listTableOptions: TableOptions = { columns: [ { header: "#", width: 3 }, - // Wide enough for a full Business-seat identity - "Account 10 - // (name@example.com, id:05cd9f04...989a40, seat:989a40)" is 66 - // characters. At 42 the cell truncated mid-`id:`, so two members - // of one workspace rendered as the same cut-off string and the - // seat that tells them apart never reached the screen. + // Wide enough for "Account 10 (name@example.com, + // id:05cd9f04...989a40)" at 57 characters. Longer emails and + // labels still truncate here, which is why the seat is no longer + // one of them. { header: "Label", width: 68 }, + { header: seatHeader, width: seatWidth }, { header: "Plan", width: 18 }, { header: "Status", width: 20 }, ], @@ -314,6 +331,7 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel(account, index, { maskEmail, peerAccounts: storage.accounts, + omitSeat: true, }); const statuses: string[] = []; const rateLimit = formatRateLimitEntry(account, now); @@ -334,6 +352,7 @@ export function createCodexListTool(ctx: ToolContext): ToolDefinition { [ String(index + 1), label, + seatSuffixes[index] ?? "-", formatPlanType(account.planType) ?? "unknown", statusText, ], diff --git a/lib/tools/codex-status.ts b/lib/tools/codex-status.ts index 0fce45b8..5567140f 100644 --- a/lib/tools/codex-status.ts +++ b/lib/tools/codex-status.ts @@ -6,6 +6,7 @@ import { tool, type ToolDefinition } from "@opencode-ai/plugin/tool"; import { loadAccounts } from "../storage.js"; import { AccountManager, formatCooldown, formatWaitTime } from "../accounts.js"; +import { resolveSeatSuffixes } from "../account-display.js"; import { MODEL_FAMILIES } from "../prompts/codex.js"; import { recommendBeginnerNextAction } from "../ui/beginner.js"; import { @@ -305,10 +306,23 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { return lines.join("\n"); } + // A column of its own, sized to what it holds, for the same reason as + // in `codex-list`: behind an unbounded email this 42-wide Label + // truncates, and a seat that does not reach the screen cannot tell + // two members of one workspace apart. + const seatSuffixes = resolveSeatSuffixes( + storage.accounts.map((entry) => entry.accountUserId), + ); + const seatHeader = "Seat"; + const seatWidth = seatSuffixes.reduce( + (widest, seat) => Math.max(widest, seat?.length ?? 0), + seatHeader.length, + ); const statusTableOptions: TableOptions = { columns: [ { header: "#", width: 3 }, { header: "Label", width: 42 }, + { header: seatHeader, width: seatWidth }, { header: "Plan", width: 18 }, { header: "Active", width: 6 }, { header: "Rate Limit", width: 16 }, @@ -327,6 +341,7 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { const label = formatCommandAccountLabel(account, index, { maskEmail, peerAccounts: storage.accounts, + omitSeat: true, }); const active = index === activeIndex ? "Yes" : "No"; const rateLimit = formatRateLimitEntry(account, now) ?? "None"; @@ -341,6 +356,7 @@ export function createCodexStatusTool(ctx: ToolContext): ToolDefinition { [ String(index + 1), label, + seatSuffixes[index] ?? "-", formatPlanType(account.planType) ?? "unknown", active, rateLimit, diff --git a/lib/tools/index.ts b/lib/tools/index.ts index d50c0edb..1f7198d7 100644 --- a/lib/tools/index.ts +++ b/lib/tools/index.ts @@ -119,6 +119,7 @@ export interface ToolContext { options?: { maskEmail?: boolean; peerAccounts?: readonly ({ accountUserId?: string } | undefined)[]; + omitSeat?: boolean; }, ) => string; resolveMaskEmail: () => boolean; diff --git a/test/index.test.ts b/test/index.test.ts index 45be271e..9fcb1fd0 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -3927,6 +3927,17 @@ describe("OpenAIOAuthPlugin", () => { vi.mocked(configModule.getCodexTuiMaskEmail).mockReturnValue(value); }; + const WORKSPACE_ID = "05cd9f040000000000989a40"; + + // The rendered account rows, stripped of their leading number. The + // number alone always differs, so comparing whole rows would pass even + // when every identity on them is identical - which is the bug. + const identityRows = (output: string): string[] => + output + .split("\n") + .filter((line) => /^\d+ /.test(line)) + .map((line) => line.replace(/^\d+ +/, "").trim()); + it("codex-list: distinguishes two seats sharing one workspace account id", async () => { await setMaskEmail(false); // Same email AND same workspace id on both rows, so the seat is the @@ -3935,21 +3946,116 @@ describe("OpenAIOAuthPlugin", () => { { refreshToken: "r1", email: "shared@example.com", - accountId: "05cd9f040000000000989a40", + accountId: WORKSPACE_ID, accountUserId: "user_aaaaaa111111", }, { refreshToken: "r2", email: "shared@example.com", - accountId: "05cd9f040000000000989a40", + accountId: WORKSPACE_ID, + accountUserId: "user_bbbbbb222222", + }, + ]; + + const output = (await plugin.tool["codex-list"].execute()) as string; + + expect(output).toContain("111111"); + expect(output).toContain("222222"); + const [first, second] = identityRows(output); + expect(first).not.toBe(second); + }); + + // The reported case, with the reviewer's own example ids. Six characters + // is a tail, not an identity: `member-000001` and `other-000001` are + // different seats that end the same way. A fixed six-character seat + // renders both as `000001` and puts the display back to claiming two + // accounts are one - the exact false reading this suffix exists to stop. + it("codex-list: distinguishes member ids that share a six-character tail", async () => { + await setMaskEmail(false); + mockStorage.accounts = [ + { + refreshToken: "r1", + email: "shared@example.com", + accountId: WORKSPACE_ID, + accountUserId: "member-000001", + }, + { + refreshToken: "r2", + email: "shared@example.com", + accountId: WORKSPACE_ID, + accountUserId: "other-000001", + }, + ]; + + const output = (await plugin.tool["codex-list"].execute()) as string; + + const [first, second] = identityRows(output); + expect(first).not.toBe(second); + // Grown past six to the shortest tail that separates them. + expect(output).toContain("ber-000001"); + expect(output).toContain("her-000001"); + }); + + // Email and workspace label have no length bound, so anything that + // shares a fixed-width cell with them can be pushed off its right edge. + // The seat must survive an email long enough to truncate the label. + it("codex-list: keeps both seats legible when a long email truncates the label", async () => { + await setMaskEmail(false); + const longEmail = + "extremely.long.account.display.name@very-long-corporate-subdomain.example.com"; + mockStorage.accounts = [ + { + refreshToken: "r1", + email: longEmail, + accountId: WORKSPACE_ID, + accountUserId: "user_aaaaaa111111", + }, + { + refreshToken: "r2", + email: longEmail, + accountId: WORKSPACE_ID, accountUserId: "user_bbbbbb222222", }, ]; const output = (await plugin.tool["codex-list"].execute()) as string; - expect(output).toContain("seat:111111"); - expect(output).toContain("seat:222222"); + // The label really is truncated here, so the assertions below are + // exercising the overflow case rather than a comfortable fit. + expect(output).toContain("…"); + expect(output).toContain("111111"); + expect(output).toContain("222222"); + const [first, second] = identityRows(output); + expect(first).not.toBe(second); + }); + + // The seat column is sized to the seats it holds rather than to a fixed + // number. These two seats are the same length and differ only in their + // last character, so any column narrower than they are truncates both to + // the same string - a seat that is present but no longer distinguishing. + it("codex-list: sizes the seat column so it never truncates a seat", async () => { + await setMaskEmail(false); + mockStorage.accounts = [ + { + refreshToken: "r1", + email: "shared@example.com", + accountId: WORKSPACE_ID, + accountUserId: "xAAAAAB", + }, + { + refreshToken: "r2", + email: "shared@example.com", + accountId: WORKSPACE_ID, + accountUserId: "yAAAAAC", + }, + ]; + + const output = (await plugin.tool["codex-list"].execute()) as string; + + expect(output).toContain("AAAAAB"); + expect(output).toContain("AAAAAC"); + const [first, second] = identityRows(output); + expect(first).not.toBe(second); }); it("codex-list: renders no seat for an account with no member id", async () => { @@ -3958,7 +4064,7 @@ describe("OpenAIOAuthPlugin", () => { { refreshToken: "r1", email: "solo@example.com", - accountId: "05cd9f040000000000989a40", + accountId: WORKSPACE_ID, }, ]; From 8e3c139e7fb03bfdf22161c7072cecd6cc17487e Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 14:31:23 -0500 Subject: [PATCH 13/15] fix(accounts): bound the seat so a head-only difference stays readable The seat renderer searched for the shortest TAIL that told the listed member ids apart. That is the wrong primitive for the ids this backend actually issues: __ The character that names the seat is at index 0, and everything after it is the workspace id repeated verbatim. No tail shorter than the whole string reaches index 0, so the search ran to its termination bound and returned all 39 characters for every account: #2 9__05cd9f04-d56a-4256-9934-9cb827989a40 #3 X__05cd9f04-d56a-4256-9934-9cb827989a40 #7 E__05cd9f04-d56a-4256-9934-9cb827989a40 #8 W__05cd9f04-d56a-4256-9934-9cb827989a40 Correct - those are four distinct strings - and unusable. The Seat column is sized to what it holds, so a real 9-account pool produced a ~150-char row, and 38 of the 39 characters spent were the workspace id already printed in the Label cell beside it. The one character that names the seat was the one a tail window is guaranteed to drop until it takes everything. `resolveSeatRenderer` now picks a rendering rather than a length, trying three capped strategies in order: 1. A tail, so ids that differ near their end keep rendering exactly as before and stay consistent with the `accountId` suffix beside them. 2. A window anchored at the first position where the ids diverge, which is what keeps the real head-differing shape short: `9__05c`, `X__05c`. 3. A SHA-256 prefix, for ids no capped window separates - one id being another with a prefix bolted on. A backend does not produce that; a fixture can. Returning the id whole survives as the final fallback, so two distinct ids still never render alike. Reaching it needs a 128-bit SHA-256 prefix collision. The properties this holds: - two records with different accountUserId never render the same string - a rendered seat is at most 32 characters, whatever the id length - ids differing early still render at 6 The standalone CLI keeps its own copy of the renderer - it reads the pool without the compiled lib - so it gets the same three strategies under the same cap, still starting at the length its mask allows so masked output widens only when staying short would print a lie. The new tests are built from the exact live shape, N ids of `__`, because every previous fixture differed near the tail and so could not reach this. The bound is asserted separately from distinctness: one without the other is how a renderer that is technically correct becomes unreadable. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- README.md | 2 +- lib/account-display.ts | 147 +++++++++++++++----- scripts/install-oc-codex-multi-auth-core.js | 104 +++++++++----- test/account-display.test.ts | 52 +++++++ test/index.test.ts | 29 ++++ test/standalone-cli.test.ts | 39 ++++++ 6 files changed, 303 insertions(+), 70 deletions(-) diff --git a/README.md b/README.md index 63e36674..31e71cdc 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ Most of these also run as a **direct CLI** with no agent or model involvement, s - unsupported-model handling is strict by default, with opt-in fallback controls - TUI quota status follows the account/workspace used by the latest request - Business workspace memberships and Personal accounts keep separate usage and quota windows. Business members sharing one workspace are distinguished by their member/seat identity, so their usage is not collapsed into one row. -- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints a tail of that as `seat:`. The tail is 6 characters where that is enough to tell the listed accounts apart and grows to the shortest length that does when it is not, so two distinct seats never render the same `seat:`. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. +- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints a short excerpt of that as `seat:`. The excerpt is a 6-character tail where that is enough to tell the listed accounts apart. Where it is not, it widens, or moves to where those ids first differ - a member id can carry its distinguishing character at the head, followed by the workspace id repeated - so two distinct seats never render the same `seat:` and a `seat:` is never longer than 32 characters. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. - The ChatGPT plan (`Free`, `Plus`, `Pro`, `Business`, `Business Premium`, `Enterprise`) is read from the access token, refreshed on every token refresh, and shown by `codex-list` and `codex-status`. `codex-limits` and the TUI read the plan live from the usage endpoint and name it the same way. An unrecognized plan is reported verbatim rather than renamed. --- diff --git a/lib/account-display.ts b/lib/account-display.ts index fb772030..ca1c91ab 100644 --- a/lib/account-display.ts +++ b/lib/account-display.ts @@ -12,6 +12,8 @@ * user-defined account label when one exists. */ +import { createHash } from "node:crypto"; + /** * Mask an email for display while preserving the domain so collisions between * accounts on the same provider remain distinguishable. @@ -49,6 +51,23 @@ export function resolveDisplayEmail( } const SEAT_SUFFIX_MIN_LENGTH = 6; +/** + * Hard ceiling on a rendered seat, independent of how long the member id is. + * + * Without it the search below returns whatever length separates the ids, and + * for a real ChatGPT member id - `__` - that is the whole 39-character string, because the one + * character that names the seat sits at the head and no tail short of the + * entire id reaches it. Every seat then renders as its own workspace id, which + * is both unreadable and the field already printed beside it. + */ +const SEAT_RENDER_MAX_LENGTH = 12; +/** + * Hash prefix lengths for the last-resort renderer. 32 hex characters is 128 + * bits of SHA-256, so the list is exhausted only by a collision that cannot be + * reached with ids a backend hands out. + */ +const SEAT_HASH_LENGTHS: readonly number[] = [8, 12, 16, 24, 32]; function normalizeSeatIdentity(accountUserId: string | undefined): string | undefined { const trimmed = accountUserId?.trim(); @@ -60,33 +79,89 @@ function sliceSeatSuffix(accountUserId: string, length: number): string { } /** - * Shortest tail, at least six characters, that renders every distinct member - * id in `accountUserIds` as a different string. + * `length` characters starting at `start`, slid left when the id is too short + * to hold that window whole. Never padded: a short id renders as itself. + */ +function sliceSeatWindow(accountUserId: string, start: number, length: number): string { + if (accountUserId.length <= length) return accountUserId; + const begin = Math.max(0, Math.min(start, accountUserId.length - length)); + return accountUserId.slice(begin, begin + length); +} + +function hashSeatIdentity(accountUserId: string, length: number): string { + return createHash("sha256").update(accountUserId).digest("hex").slice(0, length); +} + +/** Index of the first character at which the given ids are not all equal. */ +function commonPrefixLength(values: readonly string[]): number { + const [first] = values; + if (first === undefined) return 0; + let shared = first.length; + for (const value of values) { + let index = 0; + while (index < shared && index < value.length && first[index] === value[index]) { + index += 1; + } + shared = index; + if (shared === 0) break; + } + return shared; +} + +/** + * A renderer that gives every distinct member id in `accountUserIds` a + * different string, short enough to sit in a column beside the account. * - * It terminates because the search stops at the longest id present, and at - * that length every id is rendered whole - distinct strings by definition. So - * a length always exists, and the first one found is the shortest. + * Three strategies, each capped, tried in order: + * + * 1. A tail. This is what the surfaces already print for `accountId`, so it + * is preferred wherever it works, which is wherever the ids differ near + * their end. + * 2. A window anchored where the ids first diverge. Real member ids carry + * their distinguishing character at the HEAD followed by a long shared + * tail, so no tail separates them and only an anchored window stays short. + * 3. A SHA-256 prefix, for ids that no capped window separates - one id being + * another with a prefix bolted on, which a backend does not produce but a + * fixture can. + * + * Returning the id whole is kept as the final fallback so two distinct ids can + * never render alike; reaching it needs a 128-bit SHA-256 prefix collision. */ -function resolveSeatSuffixLength(accountUserIds: readonly (string | undefined)[]): number { - const distinct = new Set(); +function resolveSeatRenderer( + accountUserIds: readonly (string | undefined)[], +): (accountUserId: string) => string { + const distinct: string[] = []; + const seen = new Set(); for (const accountUserId of accountUserIds) { const normalized = normalizeSeatIdentity(accountUserId); - if (normalized) distinct.add(normalized); + if (!normalized || seen.has(normalized)) continue; + seen.add(normalized); + distinct.push(normalized); } - if (distinct.size <= 1) return SEAT_SUFFIX_MIN_LENGTH; + const tailAtMinLength = (accountUserId: string) => + sliceSeatSuffix(accountUserId, SEAT_SUFFIX_MIN_LENGTH); + if (distinct.length <= 1) return tailAtMinLength; - let longest = SEAT_SUFFIX_MIN_LENGTH; - for (const accountUserId of distinct) { - longest = Math.max(longest, accountUserId.length); + const separates = (render: (accountUserId: string) => string): boolean => + new Set(distinct.map(render)).size === distinct.length; + + for (let length = SEAT_SUFFIX_MIN_LENGTH; length <= SEAT_RENDER_MAX_LENGTH; length += 1) { + const render = (accountUserId: string) => sliceSeatSuffix(accountUserId, length); + if (separates(render)) return render; } - for (let length = SEAT_SUFFIX_MIN_LENGTH; length < longest; length += 1) { - const rendered = new Set(); - for (const accountUserId of distinct) { - rendered.add(sliceSeatSuffix(accountUserId, length)); - } - if (rendered.size === distinct.size) return length; + + const start = commonPrefixLength(distinct); + for (let length = SEAT_SUFFIX_MIN_LENGTH; length <= SEAT_RENDER_MAX_LENGTH; length += 1) { + const render = (accountUserId: string) => sliceSeatWindow(accountUserId, start, length); + if (separates(render)) return render; } - return longest; + + for (const length of SEAT_HASH_LENGTHS) { + const render = (accountUserId: string) => hashSeatIdentity(accountUserId, length); + if (separates(render)) return render; + } + + return (accountUserId: string) => accountUserId; } /** @@ -104,12 +179,13 @@ function resolveSeatSuffixLength(accountUserIds: readonly (string | undefined)[] * same account duplicated four times. Appending this suffix is what makes the * rendered rows match the accounts they describe. * - * Six characters by default, matching what the surfaces already print for - * `accountId`. Six is not unique on its own - real member ids were observed - * sharing a six-character tail, which is the same false "these are duplicates" - * reading this suffix exists to prevent - so pass `peerAccountUserIds` (the - * other accounts rendered alongside this one) and the suffix grows to whatever - * length tells them all apart. + * A six-character tail by default, matching what the surfaces already print + * for `accountId`. Six characters are not an identity on their own - member + * ids sharing a six-character tail were observed, which is the same false + * "these are duplicates" reading this suffix exists to prevent - so pass + * `peerAccountUserIds` (the other accounts rendered alongside this one) and + * the rendering widens or moves until it tells them all apart, within + * {@link SEAT_RENDER_MAX_LENGTH}. * * Returns `undefined` when there is no member id, so a token-only record * renders exactly as it did before. @@ -120,29 +196,24 @@ export function formatSeatSuffix( ): string | undefined { const trimmed = normalizeSeatIdentity(accountUserId); if (!trimmed) return undefined; - return sliceSeatSuffix( - trimmed, - peerAccountUserIds - // This id joins the set the length is measured against, so the - // guarantee holds even for a caller whose peer list is the OTHER - // accounts rather than all of them. A set makes the common case, - // where it is already there, a no-op. - ? resolveSeatSuffixLength([...peerAccountUserIds, trimmed]) - : SEAT_SUFFIX_MIN_LENGTH, - ); + if (!peerAccountUserIds) return sliceSeatSuffix(trimmed, SEAT_SUFFIX_MIN_LENGTH); + // This id joins the set the rendering is chosen against, so the guarantee + // holds even for a caller whose peer list is the OTHER accounts rather than + // all of them. Already being there makes it a no-op. + return resolveSeatRenderer([...peerAccountUserIds, trimmed])(trimmed); } /** - * Seat suffixes for a whole rendered set, all cut to one length so the rows + * Seat suffixes for a whole rendered set, all built the same way so the rows * line up and no two distinct member ids share a rendering. Entries without a * member id come back `undefined`, holding their position. */ export function resolveSeatSuffixes( accountUserIds: readonly (string | undefined)[], ): (string | undefined)[] { - const length = resolveSeatSuffixLength(accountUserIds); + const render = resolveSeatRenderer(accountUserIds); return accountUserIds.map((accountUserId) => { const trimmed = normalizeSeatIdentity(accountUserId); - return trimmed ? sliceSeatSuffix(trimmed, length) : undefined; + return trimmed ? render(trimmed) : undefined; }); } diff --git a/scripts/install-oc-codex-multi-auth-core.js b/scripts/install-oc-codex-multi-auth-core.js index 16a99221..7b77b3fe 100644 --- a/scripts/install-oc-codex-multi-auth-core.js +++ b/scripts/install-oc-codex-multi-auth-core.js @@ -1,3 +1,4 @@ +import { createHash } from "node:crypto"; import { existsSync, realpathSync } from "node:fs"; import { copyFile, mkdir, readFile, rename, rm, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; @@ -410,42 +411,81 @@ function accountIdSuffix(accountId, includeSensitive) { return accountId.slice(-4); } -// A member id is what tells two seats of one Business workspace apart, and a -// fixed-length tail does not always do it: real member ids were observed -// sharing a six-character tail, which prints two different seats as one - the -// exact misreading the seat exists to prevent. So the length grows until every -// seat printed in this run is distinct, mirroring `resolveSeatSuffixes` in -// lib/account-display.ts. It starts at the length the mask above allows, so -// masked output lengthens only when leaving it short would print a lie. -function seatSuffixAtLength(accountUserId, includeSensitive, length) { - if (!accountUserId) return undefined; - if (!includeSensitive && accountUserId.length < MASK_MIN_LENGTH) return undefined; +// A member id is what tells two seats of one Business workspace apart, and no +// fixed-length tail always does it. Real member ids were observed sharing a +// six-character tail, and the ones this backend issues are +// `__` - so the +// character that names the seat is at the head and NO tail short of the whole +// 39-character id reaches it. Growing a tail until it separates them therefore +// prints every seat as its own workspace id, which is the field already beside +// it. The renderer below mirrors `resolveSeatRenderer` in +// lib/account-display.ts: a tail, else a window anchored where the ids first +// diverge, else a hash prefix - each capped - and the id whole only if none of +// those separate them, which needs a 128-bit SHA-256 collision. +const STANDALONE_SEAT_MAX_LENGTH = 12; +const STANDALONE_SEAT_HASH_LENGTHS = [8, 12, 16, 24, 32]; + +function seatIsDisclosable(accountUserId, includeSensitive) { + if (!accountUserId) return false; + return includeSensitive || accountUserId.length >= MASK_MIN_LENGTH; +} + +function seatTail(accountUserId, length) { return accountUserId.length > length ? accountUserId.slice(-length) : accountUserId; } -function resolveStandaloneSeatLength(accountUserIds, includeSensitive) { +function seatWindow(accountUserId, start, length) { + if (accountUserId.length <= length) return accountUserId; + const begin = Math.max(0, Math.min(start, accountUserId.length - length)); + return accountUserId.slice(begin, begin + length); +} + +function seatCommonPrefixLength(values) { + const [first] = values; + if (first === undefined) return 0; + let shared = first.length; + for (const value of values) { + let index = 0; + while (index < shared && index < value.length && first[index] === value[index]) { + index += 1; + } + shared = index; + if (shared === 0) break; + } + return shared; +} + +function resolveStandaloneSeatRenderer(accountUserIds, includeSensitive) { + // Starts at the length the mask above allows, so masked output widens only + // when leaving it short would print a lie. const base = includeSensitive ? 6 : 4; - const distinct = new Set( - accountUserIds.filter( - (accountUserId) => - seatSuffixAtLength(accountUserId, includeSensitive, base) !== undefined, - ), - ); - if (distinct.size <= 1) return base; + const distinct = []; + const seen = new Set(); + for (const accountUserId of accountUserIds) { + if (!seatIsDisclosable(accountUserId, includeSensitive)) continue; + if (seen.has(accountUserId)) continue; + seen.add(accountUserId); + distinct.push(accountUserId); + } + const atBase = (accountUserId) => seatTail(accountUserId, base); + if (distinct.length <= 1) return atBase; - let longest = base; - for (const accountUserId of distinct) { - longest = Math.max(longest, accountUserId.length); + const separates = (render) => new Set(distinct.map(render)).size === distinct.length; + + for (let length = base; length <= STANDALONE_SEAT_MAX_LENGTH; length += 1) { + const render = (accountUserId) => seatTail(accountUserId, length); + if (separates(render)) return render; } - for (let length = base; length < longest; length += 1) { - const rendered = new Set( - [...distinct].map((accountUserId) => - seatSuffixAtLength(accountUserId, includeSensitive, length), - ), - ); - if (rendered.size === distinct.size) return length; + const start = seatCommonPrefixLength(distinct); + for (let length = base; length <= STANDALONE_SEAT_MAX_LENGTH; length += 1) { + const render = (accountUserId) => seatWindow(accountUserId, start, length); + if (separates(render)) return render; + } + for (const length of STANDALONE_SEAT_HASH_LENGTHS) { + const render = (accountUserId) => createHash("sha256").update(accountUserId).digest("hex").slice(0, length); + if (separates(render)) return render; } - return longest; + return (accountUserId) => accountUserId; } function summarizeStandaloneAccounts(storage, includeSensitive, tag) { @@ -456,7 +496,7 @@ function summarizeStandaloneAccounts(storage, includeSensitive, tag) { .filter(({ account }) => !normalizedTag || (Array.isArray(account?.accountTags) && account.accountTags.some((entry) => String(entry).toLowerCase() === normalizedTag))); - const seatLength = resolveStandaloneSeatLength( + const renderSeat = resolveStandaloneSeatRenderer( entries.map(({ account }) => (typeof account?.accountUserId === "string" ? account.accountUserId.trim() : "") || undefined, ), @@ -482,7 +522,9 @@ function summarizeStandaloneAccounts(storage, includeSensitive, tag) { accountId: maskValue(accountId, includeSensitive), idSuffix: accountIdSuffix(accountId, includeSensitive), accountUserId: maskValue(accountUserId, includeSensitive), - seatSuffix: seatSuffixAtLength(accountUserId, includeSensitive, seatLength), + seatSuffix: seatIsDisclosable(accountUserId, includeSensitive) + ? renderSeat(accountUserId) + : undefined, accountIdSource: account?.accountIdSource, enabled: account?.enabled !== false, hasRefreshToken: typeof account?.refreshToken === "string" && account.refreshToken.length > 0, diff --git a/test/account-display.test.ts b/test/account-display.test.ts index 3a21e346..71c9124b 100644 --- a/test/account-display.test.ts +++ b/test/account-display.test.ts @@ -144,4 +144,56 @@ describe("seat suffix", () => { it("counts the rendered id itself even when the peer list omits it", () => { expect(formatSeatSuffix("member-000001", ["other-000001"])).toBe("ber-000001"); }); + + // The shape this backend actually issues: one distinguishing character, + // then the workspace uuid repeated verbatim. The ids are 39 characters and + // differ ONLY at index 0, so no tail shorter than the whole string reaches + // the character that names the seat. A tail search therefore returns all 39 + // for every account - unreadable, and 38 of those characters are the + // workspace id already printed beside it. + it("stays short for ids that differ only at their first character", () => { + const workspace = "05cd9f04-d56a-4256-9934-9cb827989a40"; + const ids = ["9", "X", "E", "W"].map((seat) => `${seat}__${workspace}`); + + expect(resolveSeatSuffixes(ids)).toEqual([ + "9__05c", + "X__05c", + "E__05c", + "W__05c", + ]); + expect(formatSeatSuffix(`9__${workspace}`, ids)).toBe("9__05c"); + }); + + // A rendered seat sits in a table column, so its width may not be a + // function of how long the member id happens to be. 32 is the contract: + // the longest hash prefix the last-resort renderer can reach. + it("bounds every rendering regardless of id length", () => { + const workspace = "05cd9f04-d56a-4256-9934-9cb827989a40"; + const otherWorkspace = "0ce0db3a-1111-2222-3333-444444ff8839"; + const cases: string[][] = [ + ["9", "X", "E", "W"].map((seat) => `${seat}__${workspace}`), + [ + ...["9", "X"].map((seat) => `${seat}__${workspace}`), + ...["Q", "R"].map((seat) => `${seat}__${otherWorkspace}`), + ], + ["member-000001", "other-000001"], + ["a".repeat(200), `b${"a".repeat(199)}`], + // No capped window separates these - one id is another with a + // character bolted on the front - so they reach the hash. Long + // enough that returning them whole would breach the bound. + [`${"A".repeat(50)}X`, `${"A".repeat(50)}Y`, `B${"A".repeat(50)}X`], + ]; + + for (const ids of cases) { + const rendered = resolveSeatSuffixes(ids).filter( + (seat): seat is string => seat !== undefined, + ); + expect(new Set(rendered).size, `rendering ${ids.join(" / ")}`).toBe( + new Set(ids).size, + ); + for (const seat of rendered) { + expect(seat.length, `"${seat}" from ${ids.join(" / ")}`).toBeLessThanOrEqual(32); + } + } + }); }); diff --git a/test/index.test.ts b/test/index.test.ts index 9fcb1fd0..cd675ce7 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -4073,6 +4073,35 @@ describe("OpenAIOAuthPlugin", () => { expect(output).toContain("solo@example.com"); expect(output).not.toContain("seat:"); }); + + // The member ids this backend issues are `__`, so four seats of one workspace + // differ only at index 0 across a 38-character shared tail. A tail-based + // seat renders all 39 of those characters, and the column is sized to + // what it holds, so the row balloons past 150 characters and spends 38 + // of them repeating the workspace id that is already in the Label cell. + it("codex-list: keeps the seat column narrow for ids that differ only at the head", async () => { + await setMaskEmail(false); + const workspaceUuid = "05cd9f04-d56a-4256-9934-9cb827989a40"; + mockStorage.accounts = ["9", "X", "E", "W"].map((seat, position) => ({ + refreshToken: `r${position}`, + email: "shared@example.com", + accountId: workspaceUuid, + accountUserId: `${seat}__${workspaceUuid}`, + })); + + const output = (await plugin.tool["codex-list"].execute()) as string; + + const rows = identityRows(output); + expect(rows).toHaveLength(4); + expect(new Set(rows).size).toBe(4); + // The seat is an excerpt, not the id repeated into a second column. + expect(output).not.toContain(`9__${workspaceUuid}`); + // 119 characters with a 6-wide seat column, 152 with a 39-wide one. + for (const line of output.split("\n").filter((line) => /^\d+ /.test(line))) { + expect(line.length, line).toBeLessThan(130); + } + }); }); }); diff --git a/test/standalone-cli.test.ts b/test/standalone-cli.test.ts index 562faa46..ecf6accb 100644 --- a/test/standalone-cli.test.ts +++ b/test/standalone-cli.test.ts @@ -199,6 +199,45 @@ describe("standalone oc-codex-multi-auth CLI commands", () => { expect(identities[1]).toBe("(dup@....com, id:bbbb)"); }); + // This CLI keeps its own copy of the seat renderer, so it can drift from + // `lib/account-display.ts` silently. The ids here are the shape the backend + // issues - one distinguishing character, then the workspace uuid - which a + // tail-based renderer cannot separate with fewer than all 39 characters. + it("list: keeps the seat short for member ids that differ only at the head", async () => { + vi.resetModules(); + tempHome = await createTempHome(); + const workspaceUuid = "05cd9f04-d56a-4256-9934-9cb827989a40"; + await seedPool( + tempHome, + ["9", "X"].map((seat, position) => ({ + email: "shared@example.com", + accountId: workspaceUuid, + accountUserId: `${seat}__${workspaceUuid}`, + accountIdSource: "token", + refreshToken: `refresh-${position}`, + addedAt: 1000, + lastUsed: 2000, + })), + ); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const { runInstaller } = await import("../scripts/install-oc-codex-multi-auth-core.js"); + + await expect(runInstaller(["list", "--include-sensitive"], { + env: { ...process.env, HOME: tempHome, USERPROFILE: tempHome }, + })).resolves.toMatchObject({ exitCode: 0 }); + + const identities = logSpy.mock.calls + .map((call) => String(call[0])) + .filter((line) => line.startsWith("- [")) + .map(extractIdentity); + + expect(identities).toHaveLength(2); + // Matched exactly, not by prefix: the whole 39-character id starts with + // the short rendering, so `toContain` would pass on the defect. + const seats = identities.map((identity) => identity.match(/seat:([^,)]+)/)?.[1]); + expect(seats).toEqual(["9__05c", "X__05c"]); + }); + it("list: drops the org-derived label the plugin no longer generates", async () => { // The standalone CLI reads the pool through its own normalizer, so // without a mirror of the drop it keeps printing the wrong From 2ff281f40da44cf41c29d244d2d5029fef7ed6fc Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 15:00:26 -0500 Subject: [PATCH 14/15] fix(accounts): anchor the seat where the real ids actually diverge The previous commit fixed the right defect for the wrong reason, and said so in the code, the tests and the README. It was written against a description of the backend's member ids - `__`, 39 characters, differing at index 0 - and a fixture of exactly that shape. Measured structurally against a real nine-seat Business pool, the ids are 67 characters, share five leading characters, share NO tail, and their pairwise first divergences fall at three positions, 26 characters apart. Nothing in the old fixture reaches the case the live data is in. So the shipped code took a path nothing tested: with the divergences that far apart no single capped window separates the nine ids, and the hash fallback fired. Every seat rendered as an opaque 8-character prefix. That is bounded and distinct - the outcome was correct - but it was reached by the branch the code described as unreachable outside a fixture, and the README described a rendering the user would never see. Two changes. A fourth strategy, between the single window and the hash: short excerpts at each position where some pair of ids first differs, joined by `..`. The measurement is what makes this sound rather than speculative - a pair is told apart by any excerpt spanning its first divergence, so an excerpt spanning all of those positions tells every pair apart, and on the real profile that is three anchors and a 6-character rendering. It is anchored at each pair's FIRST divergence rather than at every index where the ids disagree: across ids that share only a prefix the latter is most of the tail, which localizes nothing and overflows the cap. The join is capped like everything else - once one window set exceeds `SEAT_RENDER_MAX_LENGTH` no wider set can fit, so the search ends there and the hash takes over. The hash is now documented as what it is. It is not a branch kept for tidiness against inputs a backend does not produce: it is what remains when the divergences are too many or too spread out to excerpt inside the cap, and what it prints cannot be matched against the member id by eye. README says so in those terms, with an example, because a user opening `codex-list` and seeing `719f78b5` deserves a sentence that describes it. The fixtures that encoded the wrong description are relabelled synthetic rather than deleted - a single divergence at the head is exactly what the single anchored window exists for, so it is still worth covering, just not worth calling real. The real profile is reproduced rather than paraphrased: one test asserts the fixture's own structure (67 characters, divergences at 5/31/32, and that neither of the first two strategies separates it inside the cap), so the fixture cannot drift into an easier shape the way its predecessor did. What the rendering tests assert on that profile is distinct, bounded, and DERIVED - every piece of the seat lifted from the id it names - but never a literal window. Distinct-and-bounded alone is satisfied by the hash, so on its own it would let the joined-excerpt strategy be deleted silently; pinning an exact string is how the last fixture came to assert a rendering the real data never produces. The standalone CLI keeps its own copy of the renderer, so it gets the same strategy and the same coverage. Its real-profile test asserts derived-from-id for the same reason the lib's does. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- README.md | 2 +- lib/account-display.ts | 93 +++++++++++--- scripts/install-oc-codex-multi-auth-core.js | 65 ++++++++-- test/account-display.test.ts | 131 ++++++++++++++++++-- test/index.test.ts | 53 +++++++- test/standalone-cli.test.ts | 77 +++++++++++- 6 files changed, 377 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 31e71cdc..7ee602ef 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ Most of these also run as a **direct CLI** with no agent or model involvement, s - unsupported-model handling is strict by default, with opt-in fallback controls - TUI quota status follows the account/workspace used by the latest request - Business workspace memberships and Personal accounts keep separate usage and quota windows. Business members sharing one workspace are distinguished by their member/seat identity, so their usage is not collapsed into one row. -- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints a short excerpt of that as `seat:`. The excerpt is a 6-character tail where that is enough to tell the listed accounts apart. Where it is not, it widens, or moves to where those ids first differ - a member id can carry its distinguishing character at the head, followed by the workspace id repeated - so two distinct seats never render the same `seat:` and a `seat:` is never longer than 32 characters. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. +- An account identifies itself by its own ChatGPT email and the last 6 characters of its account id, with the email masked when `maskEmail` is on. An account id names a ChatGPT workspace and every member of a Business workspace shares it, so a record that also carries a member/seat id prints a short excerpt of that as `seat:`. The excerpt is a 6-character tail where that is enough to tell the listed accounts apart. Where it is not, it widens, moves to where those ids first differ, or joins two short excerpts with `..` - real member ids are long, share a leading prefix, and differ in more than one place, so a tail alone often cannot separate them. Where no excerpt that short can separate them, `seat:` is instead an **opaque hash prefix** such as `719f78b5`: it identifies the seat and stays stable, but it is not part of the member id and cannot be matched against anything ChatGPT shows you. Whichever form it takes, two distinct seats never render the same `seat:` and a `seat:` is never longer than 32 characters. A record with no member id renders exactly as before. The OAuth id_token also lists the API-platform organizations the login belongs to; those are not ChatGPT workspaces and are never used to name an account, so logging in clears a label left behind by one. A label you set with `codex-label` is always kept. - The ChatGPT plan (`Free`, `Plus`, `Pro`, `Business`, `Business Premium`, `Enterprise`) is read from the access token, refreshed on every token refresh, and shown by `codex-list` and `codex-status`. `codex-limits` and the TUI read the plan live from the usage endpoint and name it the same way. An unrecognized plan is reported verbatim rather than renamed. --- diff --git a/lib/account-display.ts b/lib/account-display.ts index ca1c91ab..afdd445b 100644 --- a/lib/account-display.ts +++ b/lib/account-display.ts @@ -54,12 +54,11 @@ const SEAT_SUFFIX_MIN_LENGTH = 6; /** * Hard ceiling on a rendered seat, independent of how long the member id is. * - * Without it the search below returns whatever length separates the ids, and - * for a real ChatGPT member id - `__` - that is the whole 39-character string, because the one - * character that names the seat sits at the head and no tail short of the - * entire id reaches it. Every seat then renders as its own workspace id, which - * is both unreadable and the field already printed beside it. + * A seat sits in a table column beside the account, so its width may not be a + * function of how long the backend's ids happen to be. Member ids measured in + * a real multi-seat Business pool are 67 characters with no shared tail, so a + * search that stops when the ids are separated rather than when it runs out of + * room prints most of the id in every row. */ const SEAT_RENDER_MAX_LENGTH = 12; /** @@ -68,6 +67,8 @@ const SEAT_RENDER_MAX_LENGTH = 12; * reached with ids a backend hands out. */ const SEAT_HASH_LENGTHS: readonly number[] = [8, 12, 16, 24, 32]; +/** Marks the gap between two excerpts, as `accountId` already elides with `...`. */ +const SEAT_WINDOW_SEPARATOR = ".."; function normalizeSeatIdentity(accountUserId: string | undefined): string | undefined { const trimmed = accountUserId?.trim(); @@ -108,21 +109,67 @@ function commonPrefixLength(values: readonly string[]): number { return shared; } +/** First index at which two ids differ, or their shared length if one prefixes the other. */ +function firstDivergence(left: string, right: string): number { + const limit = Math.min(left.length, right.length); + let index = 0; + while (index < limit && left[index] === right[index]) index += 1; + return index; +} + +/** + * For every pair of ids, the first index at which that pair differs. + * + * Deliberately not "every index where the ids disagree": across a handful of + * random-looking ids that is nearly every index, which localizes nothing. A + * pair is told apart by any excerpt covering its first divergence, so an + * excerpt covering all of these positions tells every pair apart. + */ +function divergenceAnchors(values: readonly string[]): number[] { + const anchors = new Set(); + for (let left = 0; left < values.length; left += 1) { + for (let right = left + 1; right < values.length; right += 1) { + const leftValue = values[left]; + const rightValue = values[right]; + if (leftValue === undefined || rightValue === undefined) continue; + anchors.add(firstDivergence(leftValue, rightValue)); + } + } + return [...anchors].sort((left, right) => left - right); +} + +/** Starts of `width`-wide windows covering `anchors`, dropping those an earlier window already spans. */ +function anchorWindowStarts(anchors: readonly number[], width: number): number[] { + const starts: number[] = []; + for (const anchor of anchors) { + const last = starts[starts.length - 1]; + if (last !== undefined && anchor < last + width) continue; + starts.push(anchor); + } + return starts; +} + /** * A renderer that gives every distinct member id in `accountUserIds` a * different string, short enough to sit in a column beside the account. * - * Three strategies, each capped, tried in order: + * Four strategies, each capped, tried in order: * * 1. A tail. This is what the surfaces already print for `accountId`, so it * is preferred wherever it works, which is wherever the ids differ near * their end. - * 2. A window anchored where the ids first diverge. Real member ids carry - * their distinguishing character at the HEAD followed by a long shared - * tail, so no tail separates them and only an anchored window stays short. - * 3. A SHA-256 prefix, for ids that no capped window separates - one id being - * another with a prefix bolted on, which a backend does not produce but a - * fixture can. + * 2. One window anchored where the ids first diverge, for ids that share a + * long tail. + * 3. Short windows at each position where some pair first differs, joined by + * `..`. Member ids in the one real multi-seat Business pool this was + * measured against diverge in more than one place - clusters 26 characters + * apart - so no single capped window separates them, and joining excerpts + * is what keeps the seat both bounded and readable off the id. + * 4. A SHA-256 prefix. This is NOT an unreachable branch kept for tidiness: + * it is what remains when the divergences are too many or too spread out + * for (3) to cover inside the cap. What it prints is opaque - it cannot be + * matched against the id by eye - so any surface documenting the seat has + * to say this outcome exists. * * Returning the id whole is kept as the final fallback so two distinct ids can * never render alike; reaching it needs a 128-bit SHA-256 prefix collision. @@ -156,6 +203,21 @@ function resolveSeatRenderer( if (separates(render)) return render; } + const anchors = divergenceAnchors(distinct); + for (let width = 2; width <= SEAT_RENDER_MAX_LENGTH; width += 1) { + const starts = anchorWindowStarts(anchors, width); + const rendered = + starts.length * width + (starts.length - 1) * SEAT_WINDOW_SEPARATOR.length; + // Wider windows only ever cost more, so once one set overflows the cap + // no later width can fit and the search is over. + if (rendered > SEAT_RENDER_MAX_LENGTH) break; + const render = (accountUserId: string) => + starts + .map((windowStart) => accountUserId.slice(windowStart, windowStart + width)) + .join(SEAT_WINDOW_SEPARATOR); + if (separates(render)) return render; + } + for (const length of SEAT_HASH_LENGTHS) { const render = (accountUserId: string) => hashSeatIdentity(accountUserId, length); if (separates(render)) return render; @@ -184,8 +246,9 @@ function resolveSeatRenderer( * ids sharing a six-character tail were observed, which is the same false * "these are duplicates" reading this suffix exists to prevent - so pass * `peerAccountUserIds` (the other accounts rendered alongside this one) and - * the rendering widens or moves until it tells them all apart, within - * {@link SEAT_RENDER_MAX_LENGTH}. + * {@link resolveSeatRenderer} picks a rendering that separates them inside + * {@link SEAT_RENDER_MAX_LENGTH}: a wider or relocated excerpt of the id + * where one fits, and an opaque hash prefix where none does. * * Returns `undefined` when there is no member id, so a token-only record * renders exactly as it did before. diff --git a/scripts/install-oc-codex-multi-auth-core.js b/scripts/install-oc-codex-multi-auth-core.js index 7b77b3fe..18c7a748 100644 --- a/scripts/install-oc-codex-multi-auth-core.js +++ b/scripts/install-oc-codex-multi-auth-core.js @@ -412,18 +412,19 @@ function accountIdSuffix(accountId, includeSensitive) { } // A member id is what tells two seats of one Business workspace apart, and no -// fixed-length tail always does it. Real member ids were observed sharing a -// six-character tail, and the ones this backend issues are -// `__` - so the -// character that names the seat is at the head and NO tail short of the whole -// 39-character id reaches it. Growing a tail until it separates them therefore -// prints every seat as its own workspace id, which is the field already beside -// it. The renderer below mirrors `resolveSeatRenderer` in -// lib/account-display.ts: a tail, else a window anchored where the ids first -// diverge, else a hash prefix - each capped - and the id whole only if none of -// those separate them, which needs a 128-bit SHA-256 collision. +// fixed-length tail always does it: member ids sharing a six-character tail +// were observed, and in a real nine-seat pool the ids are 67 characters with +// no shared tail at all, so growing a tail until it separates them prints most +// of the id in every row. The renderer below mirrors `resolveSeatRenderer` in +// lib/account-display.ts - a tail, else one window anchored where the ids first +// diverge, else short windows at each position where a pair first differs +// joined by `..`, else a hash prefix, each capped - and the id whole only if +// none of those separate them, which needs a 128-bit SHA-256 collision. The +// hash outcome is reachable, and it prints a value that cannot be matched +// against the id by eye. const STANDALONE_SEAT_MAX_LENGTH = 12; const STANDALONE_SEAT_HASH_LENGTHS = [8, 12, 16, 24, 32]; +const STANDALONE_SEAT_WINDOW_SEPARATOR = ".."; function seatIsDisclosable(accountUserId, includeSensitive) { if (!accountUserId) return false; @@ -455,6 +456,36 @@ function seatCommonPrefixLength(values) { return shared; } +function seatFirstDivergence(left, right) { + const limit = Math.min(left.length, right.length); + let index = 0; + while (index < limit && left[index] === right[index]) index += 1; + return index; +} + +// For every pair, the first index at which that pair differs - not every index +// where the ids disagree, which across a handful of random-looking ids is +// nearly all of them and localizes nothing. +function seatDivergenceAnchors(values) { + const anchors = new Set(); + for (let left = 0; left < values.length; left += 1) { + for (let right = left + 1; right < values.length; right += 1) { + anchors.add(seatFirstDivergence(values[left], values[right])); + } + } + return [...anchors].sort((left, right) => left - right); +} + +function seatAnchorWindowStarts(anchors, width) { + const starts = []; + for (const anchor of anchors) { + const last = starts[starts.length - 1]; + if (last !== undefined && anchor < last + width) continue; + starts.push(anchor); + } + return starts; +} + function resolveStandaloneSeatRenderer(accountUserIds, includeSensitive) { // Starts at the length the mask above allows, so masked output widens only // when leaving it short would print a lie. @@ -481,6 +512,20 @@ function resolveStandaloneSeatRenderer(accountUserIds, includeSensitive) { const render = (accountUserId) => seatWindow(accountUserId, start, length); if (separates(render)) return render; } + const anchors = seatDivergenceAnchors(distinct); + for (let width = 2; width <= STANDALONE_SEAT_MAX_LENGTH; width += 1) { + const starts = seatAnchorWindowStarts(anchors, width); + const rendered = + starts.length * width + (starts.length - 1) * STANDALONE_SEAT_WINDOW_SEPARATOR.length; + // Wider windows only ever cost more, so once one set overflows the cap + // no later width can fit. + if (rendered > STANDALONE_SEAT_MAX_LENGTH) break; + const render = (accountUserId) => + starts + .map((windowStart) => accountUserId.slice(windowStart, windowStart + width)) + .join(STANDALONE_SEAT_WINDOW_SEPARATOR); + if (separates(render)) return render; + } for (const length of STANDALONE_SEAT_HASH_LENGTHS) { const render = (accountUserId) => createHash("sha256").update(accountUserId).digest("hex").slice(0, length); if (separates(render)) return render; diff --git a/test/account-display.test.ts b/test/account-display.test.ts index 71c9124b..2be7234d 100644 --- a/test/account-display.test.ts +++ b/test/account-display.test.ts @@ -145,12 +145,11 @@ describe("seat suffix", () => { expect(formatSeatSuffix("member-000001", ["other-000001"])).toBe("ber-000001"); }); - // The shape this backend actually issues: one distinguishing character, - // then the workspace uuid repeated verbatim. The ids are 39 characters and - // differ ONLY at index 0, so no tail shorter than the whole string reaches - // the character that names the seat. A tail search therefore returns all 39 - // for every account - unreadable, and 38 of those characters are the - // workspace id already printed beside it. + // A SYNTHETIC single-divergence shape - one distinguishing character, then + // 38 identical ones - not what this backend issues; see the measured + // profile below. No tail reaches a difference at the head, so a tail search + // returns all 39 characters for every account. This is the case the single + // anchored window exists for. it("stays short for ids that differ only at their first character", () => { const workspace = "05cd9f04-d56a-4256-9934-9cb827989a40"; const ids = ["9", "X", "E", "W"].map((seat) => `${seat}__${workspace}`); @@ -164,6 +163,115 @@ describe("seat suffix", () => { expect(formatSeatSuffix(`9__${workspace}`, ids)).toBe("9__05c"); }); + /** + * The profile measured structurally against a real nine-seat Business pool: + * 67 characters, five shared leading characters, no shared tail, and + * pairwise first divergences at three positions 26 apart. Reproduced rather + * than described, because the fixture it replaces encoded a description of + * that data that turned out to be wrong. + */ + const realPoolProfileIds = (): string[] => { + const member = (head: string) => `${head}0123456789abcdefghijklmno`; + const seat = (head: string, workspace: string) => `user_${member(head)}${workspace}`; + const w1 = "05cd9f04-d56a-4256-9934-9cb827989a40"; + const w2 = "0ce0db3a-1111-2222-3333-444444ff8839"; + const w3 = "15aaaaaa-2222-3333-4444-555555aa1111"; + const w4 = "25bbbbbb-3333-4444-5555-666666bb2222"; + const w5 = "35cccccc-4444-5555-6666-777777cc3333"; + return [ + seat("A", w1), + seat("B", w1), + seat("C", w1), + seat("D", w1), + seat("E", w2), + seat("A", w2), + seat("B", w3), + seat("F", w4), + seat("C", w5), + ]; + }; + + const firstDivergence = (left: string, right: string): number => { + let index = 0; + while (index < left.length && left[index] === right[index]) index += 1; + return index; + }; + + it("carries the structure measured on the real pool, not a paraphrase of it", () => { + const ids = realPoolProfileIds(); + const divergences = new Set(); + for (let left = 0; left < ids.length; left += 1) { + for (let right = left + 1; right < ids.length; right += 1) { + divergences.add(firstDivergence(ids[left]!, ids[right]!)); + } + } + + expect([...new Set(ids.map((id) => id.length))]).toEqual([67]); + expect(new Set(ids).size).toBe(9); + expect([...divergences].sort((left, right) => left - right)).toEqual([5, 31, 32]); + // Neither of the first two strategies can separate these inside the cap, + // which is what makes this shape worth a fixture at all. + expect(new Set(ids.map((id) => id.slice(-32))).size).toBe(5); + expect(new Set(ids.map((id) => id.slice(5, 5 + 12))).size).toBe(6); + }); + + // Distinct and bounded is the entire contract on this shape. Deliberately + // not a specific string: which strategy reaches it is an implementation + // detail, and pinning one is how the previous fixture came to assert a + // rendering the real data never produced. + it("renders the real-pool profile distinct and bounded", () => { + const rendered = resolveSeatSuffixes(realPoolProfileIds()).filter( + (seat): seat is string => seat !== undefined, + ); + + expect(rendered).toHaveLength(9); + expect(new Set(rendered).size).toBe(9); + for (const seat of rendered) { + expect(seat.length, seat).toBeLessThanOrEqual(32); + } + }); + + // Distinct and bounded is satisfied by a hash too, so on its own it leaves + // the joined-excerpt strategy unpinned - delete that strategy and this + // profile silently falls through to opaque output. This asserts the part + // that is worth having: every piece of the rendered seat is lifted from the + // id, so a human can find it there. Still no literal window. + it("keeps the real-pool seat derived from the id rather than hashed", () => { + const ids = realPoolProfileIds(); + const rendered = resolveSeatSuffixes(ids); + + ids.forEach((id, index) => { + const seat = rendered[index]; + expect(seat, id).toBeDefined(); + for (const piece of String(seat).split("..")) { + expect(piece.length, `"${piece}" of "${seat}"`).toBeGreaterThan(0); + expect(id, `"${piece}" of "${seat}"`).toContain(piece); + } + }); + }); + + // The hash is not a branch kept for tidiness. Divergences too many or too + // far apart for joined excerpts to cover inside the cap land here, and what + // it prints cannot be matched against the id by eye - which is why the + // surfaces that print it have to say so. + it("falls back to a bounded hash when the divergences are too scattered to excerpt", () => { + const ids = [ + "A".repeat(60), + ...[0, 3, 6, 9, 12, 15, 18, 21].map( + (at) => `${"A".repeat(at)}B${"A".repeat(59 - at)}`, + ), + ]; + + const rendered = resolveSeatSuffixes(ids).filter( + (seat): seat is string => seat !== undefined, + ); + + expect(new Set(rendered).size).toBe(ids.length); + for (const seat of rendered) { + expect(seat).toMatch(/^[0-9a-f]{8}$/); + } + }); + // A rendered seat sits in a table column, so its width may not be a // function of how long the member id happens to be. 32 is the contract: // the longest hash prefix the last-resort renderer can reach. @@ -178,10 +286,15 @@ describe("seat suffix", () => { ], ["member-000001", "other-000001"], ["a".repeat(200), `b${"a".repeat(199)}`], - // No capped window separates these - one id is another with a - // character bolted on the front - so they reach the hash. Long - // enough that returning them whole would breach the bound. + // Long enough that returning any of them whole would breach the bound. [`${"A".repeat(50)}X`, `${"A".repeat(50)}Y`, `B${"A".repeat(50)}X`], + realPoolProfileIds(), + [ + "A".repeat(60), + ...[0, 3, 6, 9, 12, 15, 18, 21].map( + (at) => `${"A".repeat(at)}B${"A".repeat(59 - at)}`, + ), + ], ]; for (const ids of cases) { diff --git a/test/index.test.ts b/test/index.test.ts index cd675ce7..b9d995b3 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -4074,12 +4074,12 @@ describe("OpenAIOAuthPlugin", () => { expect(output).not.toContain("seat:"); }); - // The member ids this backend issues are `__`, so four seats of one workspace - // differ only at index 0 across a 38-character shared tail. A tail-based - // seat renders all 39 of those characters, and the column is sized to - // what it holds, so the row balloons past 150 characters and spends 38 - // of them repeating the workspace id that is already in the Label cell. + // A SYNTHETIC single-divergence shape - see the measured profile below + // for what the backend actually issues. Four seats differing only at + // index 0 across a 38-character shared tail: a tail-based seat renders + // all 39 of those characters, and the column is sized to what it holds, + // so the row balloons past 150 characters and spends 38 of them + // repeating the workspace id already in the Label cell. it("codex-list: keeps the seat column narrow for ids that differ only at the head", async () => { await setMaskEmail(false); const workspaceUuid = "05cd9f04-d56a-4256-9934-9cb827989a40"; @@ -4102,6 +4102,47 @@ describe("OpenAIOAuthPlugin", () => { expect(line.length, line).toBeLessThan(130); } }); + + // The profile measured structurally against a real nine-seat Business + // pool: 67-character ids, five shared leading characters, no shared + // tail, and pairwise first divergences in clusters 26 characters apart. + // Asserted as distinct and narrow rather than as a particular excerpt - + // which strategy reaches it is an implementation detail, and pinning one + // is how the fixture above came to encode a wrong reading of the data. + it("codex-list: keeps rows distinct and narrow on the measured real-pool id shape", async () => { + await setMaskEmail(false); + const w1 = "05cd9f04-d56a-4256-9934-9cb827989a40"; + const w2 = "0ce0db3a-1111-2222-3333-444444ff8839"; + const w3 = "15aaaaaa-2222-3333-4444-555555aa1111"; + const w4 = "25bbbbbb-3333-4444-5555-666666bb2222"; + const w5 = "35cccccc-4444-5555-6666-777777cc3333"; + const pool: Array<[string, string]> = [ + ["A", w1], + ["B", w1], + ["C", w1], + ["D", w1], + ["E", w2], + ["A", w2], + ["B", w3], + ["F", w4], + ["C", w5], + ]; + mockStorage.accounts = pool.map(([head, workspace], position) => ({ + refreshToken: `r${position}`, + email: "shared@example.com", + accountId: workspace, + accountUserId: `user_${head}0123456789abcdefghijklmno${workspace}`, + })); + + const output = (await plugin.tool["codex-list"].execute()) as string; + + const rows = identityRows(output); + expect(rows).toHaveLength(9); + expect(new Set(rows).size).toBe(9); + for (const line of output.split("\n").filter((line) => /^\d+ /.test(line))) { + expect(line.length, line).toBeLessThan(130); + } + }); }); }); diff --git a/test/standalone-cli.test.ts b/test/standalone-cli.test.ts index ecf6accb..a84ed6ef 100644 --- a/test/standalone-cli.test.ts +++ b/test/standalone-cli.test.ts @@ -200,9 +200,11 @@ describe("standalone oc-codex-multi-auth CLI commands", () => { }); // This CLI keeps its own copy of the seat renderer, so it can drift from - // `lib/account-display.ts` silently. The ids here are the shape the backend - // issues - one distinguishing character, then the workspace uuid - which a - // tail-based renderer cannot separate with fewer than all 39 characters. + // `lib/account-display.ts` silently. The ids here are a SYNTHETIC + // single-divergence shape - one distinguishing character, then the + // workspace uuid - not what the backend issues; see the measured profile + // below. No tail reaches a difference at the head, so a tail-based renderer + // cannot separate them with fewer than all 39 characters. it("list: keeps the seat short for member ids that differ only at the head", async () => { vi.resetModules(); tempHome = await createTempHome(); @@ -238,6 +240,75 @@ describe("standalone oc-codex-multi-auth CLI commands", () => { expect(seats).toEqual(["9__05c", "X__05c"]); }); + // The profile measured structurally against a real nine-seat Business pool: + // 67-character ids, five shared leading characters, no shared tail, and + // pairwise first divergences in clusters 26 characters apart. Asserted as + // distinct and bounded rather than as a particular excerpt - which strategy + // reaches it is an implementation detail, and pinning one is how the + // fixture above came to encode a wrong reading of the data. + it("list: keeps the seat distinct and bounded on the measured real-pool id shape", async () => { + vi.resetModules(); + tempHome = await createTempHome(); + const workspaces = [ + "05cd9f04-d56a-4256-9934-9cb827989a40", + "0ce0db3a-1111-2222-3333-444444ff8839", + "15aaaaaa-2222-3333-4444-555555aa1111", + "25bbbbbb-3333-4444-5555-666666bb2222", + "35cccccc-4444-5555-6666-777777cc3333", + ]; + const pool: Array<[string, number]> = [ + ["A", 0], + ["B", 0], + ["C", 0], + ["D", 0], + ["E", 1], + ["A", 1], + ["B", 2], + ["F", 3], + ["C", 4], + ]; + await seedPool( + tempHome, + pool.map(([head, workspaceIndex], position) => ({ + email: "shared@example.com", + accountId: workspaces[workspaceIndex], + accountUserId: `user_${head}0123456789abcdefghijklmno${workspaces[workspaceIndex]}`, + accountIdSource: "token", + refreshToken: `refresh-${position}`, + addedAt: 1000, + lastUsed: 2000, + })), + ); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const { runInstaller } = await import("../scripts/install-oc-codex-multi-auth-core.js"); + + await expect(runInstaller(["list", "--include-sensitive"], { + env: { ...process.env, HOME: tempHome, USERPROFILE: tempHome }, + })).resolves.toMatchObject({ exitCode: 0 }); + + const seats = logSpy.mock.calls + .map((call) => String(call[0])) + .filter((line) => line.startsWith("- [")) + .map((line) => extractIdentity(line).match(/seat:([^,)]+)/)?.[1]); + + expect(seats).toHaveLength(9); + expect(new Set(seats).size).toBe(9); + for (const seat of seats) { + expect(seat, String(seat)).toBeDefined(); + expect(String(seat).length, String(seat)).toBeLessThanOrEqual(32); + } + // Distinct and bounded is satisfied by a hash too, so it alone would not + // notice this copy losing the joined-excerpt strategy the lib has. Every + // piece has to be lifted from the id it names. + pool.forEach(([head, workspaceIndex], position) => { + const id = `user_${head}0123456789abcdefghijklmno${workspaces[workspaceIndex]}`; + for (const piece of String(seats[position]).split("..")) { + expect(piece.length, `"${piece}" of "${seats[position]}"`).toBeGreaterThan(0); + expect(id, `"${piece}" of "${seats[position]}"`).toContain(piece); + } + }); + }); + it("list: drops the org-derived label the plugin no longer generates", async () => { // The standalone CLI reads the pool through its own normalizer, so // without a mirror of the drop it keeps printing the wrong From 0a555d7b7195c516381e8a9df0db69554b646c42 Mon Sep 17 00:00:00 2001 From: Nowaker Date: Thu, 17 Sep 2026 15:19:04 -0500 Subject: [PATCH 15/15] fix(accounts): keep widening the seat window past an overflowing width The joined-excerpt search abandoned the remaining widths as soon as one window set exceeded the cap, on the stated premise that "wider windows only ever cost more". That premise is false. A window set costs windows * width + (windows - 1) * 2 which grows with `width` only while `windows` holds still, and it does not: two anchors closer together than the window merge into one window, so the count drops and the total can fall. Measured on anchors at {5,6,7,31,32,33} - two clusters of three adjacent positions, 26 apart: width 2 -> 4 windows, cost 14 over the 12-character cap width 3 -> 2 windows, cost 8 fits, and separates width 4 -> 2 windows, cost 10 width 5 -> 2 windows, cost 12 width 6 -> 2 windows, cost 14 over again Stopping at the first overflow stopped at width 2 and fell through to the hash, so seven accounts that a three-character window renders as `012..qrs` / `Z12..qrs` / `0Z2..qrs` printed as opaque SHA-256 prefixes instead. The rendering was correct - distinct and bounded - and unusable for the reason the whole excerpt strategy exists: nothing on screen could be found in the id it names. So the overflow skips that width rather than ending the search. The cap is untouched: a width whose set exceeds it is still rejected, the loop still stops at the cap, and nothing wider than 12 characters is ever rendered from a window. At most eleven widths are tried. This is not hypothetical clustering. The real nine-seat pool diverges at {5, 31, 32}, where 31 and 32 are adjacent - the same shape, one member per cluster short of reaching the overflow. It renders identically before and after this commit. The hash branch stays reachable: divergences too many or too far apart for any capped window set still land there, and keep their own test. The standalone CLI carries its own copy of the renderer, so it carries the same fix. A divergence between the two is its own bug. The new fixtures assert the window arithmetic in the test rather than describing it - the anchor positions, the four windows costing 14 at width 2, the two costing 8 at width 3 - because that arithmetic is what decides the outcome. Each also asserts DERIVED: every `..`-joined piece is a substring of the id it names. Distinct-and-bounded alone is satisfied by a hash, which is precisely what this shape used to produce, so on its own it would not have noticed. AI-Tool: opencode AI-Model: anthropic/claude-opus-5 AI-Platform: linux AI-Harness: Vibeterm 4e488d6 --- lib/account-display.ts | 9 ++- scripts/install-oc-codex-multi-auth-core.js | 8 ++- test/account-display.test.ts | 74 +++++++++++++++++++++ test/standalone-cli.test.ts | 49 ++++++++++++++ 4 files changed, 134 insertions(+), 6 deletions(-) diff --git a/lib/account-display.ts b/lib/account-display.ts index afdd445b..193afa24 100644 --- a/lib/account-display.ts +++ b/lib/account-display.ts @@ -208,9 +208,12 @@ function resolveSeatRenderer( const starts = anchorWindowStarts(anchors, width); const rendered = starts.length * width + (starts.length - 1) * SEAT_WINDOW_SEPARATOR.length; - // Wider windows only ever cost more, so once one set overflows the cap - // no later width can fit and the search is over. - if (rendered > SEAT_RENDER_MAX_LENGTH) break; + // Skipped, not abandoned: a wider window can span two nearby anchors + // that needed one window each, so the cost falls as the window count + // does. Anchors at {5,6,7,31,32,33} cost 14 at width 2 (four windows) + // and 8 at width 3 (two), so giving up at the first overflow loses a + // rendering that fits comfortably. + if (rendered > SEAT_RENDER_MAX_LENGTH) continue; const render = (accountUserId: string) => starts .map((windowStart) => accountUserId.slice(windowStart, windowStart + width)) diff --git a/scripts/install-oc-codex-multi-auth-core.js b/scripts/install-oc-codex-multi-auth-core.js index 18c7a748..1dcf5252 100644 --- a/scripts/install-oc-codex-multi-auth-core.js +++ b/scripts/install-oc-codex-multi-auth-core.js @@ -517,9 +517,11 @@ function resolveStandaloneSeatRenderer(accountUserIds, includeSensitive) { const starts = seatAnchorWindowStarts(anchors, width); const rendered = starts.length * width + (starts.length - 1) * STANDALONE_SEAT_WINDOW_SEPARATOR.length; - // Wider windows only ever cost more, so once one set overflows the cap - // no later width can fit. - if (rendered > STANDALONE_SEAT_MAX_LENGTH) break; + // Skipped, not abandoned: a wider window can span two nearby anchors + // that needed one window each, so the cost falls as the window count + // does. Mirrors `resolveSeatRenderer` in lib/account-display.ts, where + // the measured counter-example is written out. + if (rendered > STANDALONE_SEAT_MAX_LENGTH) continue; const render = (accountUserId) => starts .map((windowStart) => accountUserId.slice(windowStart, windowStart + width)) diff --git a/test/account-display.test.ts b/test/account-display.test.ts index 2be7234d..89454729 100644 --- a/test/account-display.test.ts +++ b/test/account-display.test.ts @@ -250,6 +250,79 @@ describe("seat suffix", () => { }); }); + /** + * Two clusters of three adjacent divergences, 26 apart - the real pool's + * clustering with one more member per cluster. Every id differs from every + * other at exactly one marker position, so an excerpt separates a pair + * only by covering a marker, which is what makes the window arithmetic + * below decide the outcome rather than merely describe it. + */ + const clusteredAnchorIds = (): string[] => { + const base = "user_0123456789abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnop"; + const flip = (at: number) => `${base.slice(0, at)}Z${base.slice(at + 1)}`; + return [base, ...[5, 6, 7, 31, 32, 33].map(flip)]; + }; + + /** The renderer's own window placement, to measure the fixture rather than trust it. */ + const windowStarts = (anchors: readonly number[], width: number): number[] => { + const starts: number[] = []; + for (const anchor of anchors) { + const last = starts[starts.length - 1]; + if (last !== undefined && anchor < last + width) continue; + starts.push(anchor); + } + return starts; + }; + + const joinedCost = (windows: number, width: number): number => + windows * width + (windows - 1) * "..".length; + + it("carries a divergence structure whose window cost falls as the window widens", () => { + const ids = clusteredAnchorIds(); + const divergences = new Set(); + for (let left = 0; left < ids.length; left += 1) { + for (let right = left + 1; right < ids.length; right += 1) { + divergences.add(firstDivergence(ids[left]!, ids[right]!)); + } + } + const anchors = [...divergences].sort((left, right) => left - right); + + expect([...new Set(ids.map((id) => id.length))]).toEqual([67]); + expect(new Set(ids).size).toBe(7); + expect(anchors).toEqual([5, 6, 7, 31, 32, 33]); + // Neither earlier strategy can separate these inside the cap. + expect(new Set(ids.map((id) => id.slice(-32))).size).toBe(1); + expect(new Set(ids.map((id) => id.slice(5, 5 + 12))).size).toBe(4); + + // The non-monotonicity itself: at width 2 the adjacent anchors need a + // window each and the join costs 14, over the 12-character cap. At + // width 3 each cluster collapses into ONE window and the same join + // costs 8. A search that stops at the first overflow never sees it. + expect(windowStarts(anchors, 2)).toEqual([5, 7, 31, 33]); + expect(joinedCost(4, 2)).toBeGreaterThan(12); + expect(windowStarts(anchors, 3)).toEqual([5, 31]); + expect(joinedCost(2, 3)).toBeLessThanOrEqual(12); + }); + + // Distinct and bounded would pass on a hash, which is exactly what this + // shape used to produce, so the assertion that matters is DERIVED: every + // piece lifted from the id it names. + it("excerpts clustered divergences instead of giving up at the first overflow", () => { + const ids = clusteredAnchorIds(); + const rendered = resolveSeatSuffixes(ids); + + expect(new Set(rendered).size).toBe(ids.length); + ids.forEach((id, index) => { + const seat = rendered[index]; + expect(seat, id).toBeDefined(); + expect(String(seat).length, String(seat)).toBeLessThanOrEqual(32); + for (const piece of String(seat).split("..")) { + expect(piece.length, `"${piece}" of "${seat}"`).toBeGreaterThan(0); + expect(id, `"${piece}" of "${seat}"`).toContain(piece); + } + }); + }); + // The hash is not a branch kept for tidiness. Divergences too many or too // far apart for joined excerpts to cover inside the cap land here, and what // it prints cannot be matched against the id by eye - which is why the @@ -289,6 +362,7 @@ describe("seat suffix", () => { // Long enough that returning any of them whole would breach the bound. [`${"A".repeat(50)}X`, `${"A".repeat(50)}Y`, `B${"A".repeat(50)}X`], realPoolProfileIds(), + clusteredAnchorIds(), [ "A".repeat(60), ...[0, 3, 6, 9, 12, 15, 18, 21].map( diff --git a/test/standalone-cli.test.ts b/test/standalone-cli.test.ts index a84ed6ef..0e835640 100644 --- a/test/standalone-cli.test.ts +++ b/test/standalone-cli.test.ts @@ -309,6 +309,55 @@ describe("standalone oc-codex-multi-auth CLI commands", () => { }); }); + // Two clusters of three adjacent divergences, 26 apart. At a two-character + // window each cluster needs a window of its own and the join overflows the + // cap; at three characters each cluster collapses into one window and the + // join fits. A search that abandons the widths after the first overflow + // prints a hash here, so this is where this copy would drift from the lib. + it("list: excerpts clustered divergences rather than giving up on them", async () => { + vi.resetModules(); + tempHome = await createTempHome(); + const base = "user_0123456789abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnop"; + const memberIds = [ + base, + ...[5, 6, 7, 31, 32, 33].map((at) => `${base.slice(0, at)}Z${base.slice(at + 1)}`), + ]; + await seedPool( + tempHome, + memberIds.map((accountUserId, position) => ({ + email: "shared@example.com", + accountId: "05cd9f04-d56a-4256-9934-9cb827989a40", + accountUserId, + accountIdSource: "token", + refreshToken: `refresh-${position}`, + addedAt: 1000, + lastUsed: 2000, + })), + ); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + const { runInstaller } = await import("../scripts/install-oc-codex-multi-auth-core.js"); + + await expect(runInstaller(["list", "--include-sensitive"], { + env: { ...process.env, HOME: tempHome, USERPROFILE: tempHome }, + })).resolves.toMatchObject({ exitCode: 0 }); + + const seats = logSpy.mock.calls + .map((call) => String(call[0])) + .filter((line) => line.startsWith("- [")) + .map((line) => extractIdentity(line).match(/seat:([^,)]+)/)?.[1]); + + expect(seats).toHaveLength(memberIds.length); + expect(new Set(seats).size).toBe(memberIds.length); + memberIds.forEach((id, position) => { + const seat = String(seats[position]); + expect(seat.length, seat).toBeLessThanOrEqual(32); + for (const piece of seat.split("..")) { + expect(piece.length, `"${piece}" of "${seat}"`).toBeGreaterThan(0); + expect(id, `"${piece}" of "${seat}"`).toContain(piece); + } + }); + }); + it("list: drops the org-derived label the plugin no longer generates", async () => { // The standalone CLI reads the pool through its own normalizer, so // without a mirror of the drop it keeps printing the wrong