From ad8548e3abacc87292bd3519363b8a0f28a4435e Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 21:04:35 +0900 Subject: [PATCH 1/3] fix(oauth): say when a CLI login's browser never opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openUrl` reports whether the OS launcher actually started, and the Codex account login prints it. The two generic CLI logins still called `void openUrl(...)`, so on a host with no browser both announced that they were opening one, printed a URL, and then asked a question that assumes it opened. Nothing distinguishes that from a login that is working, which is the whole of #5261. Reporting it is an ordering problem, not a message problem. The OAuth controller does not await `onAuth` — every provider calls it as `ctrl.onAuth?.(...)` and moves on — so the launcher's answer arrives after the flow has continued, and on a callback-server provider it has already drawn a readline prompt by then. A warning written at that moment lands on the line the user is typing on. So the launch reports itself as soon as it settles, and the manual-code prompt waits on that report before asking; the key login awaits it before it creates a reader at all. The sentence itself is now stated once. `BROWSER_LAUNCH_FAILED_HINT` keeps its ChatGPT-specific second line about the fixed callback port and `--device`, but derives its first line from the shared notice instead of repeating it. The handlers take an optional deps object because the contract worth holding is an order, and an order is only observable from something that records both events. Production passes none of them. --- scripts/test-layout/layout.json | 1 + src/cli/account-auth.ts | 6 +- src/lib/browser-launch-notice.ts | 59 +++++ src/oauth/login-cli.ts | 109 +++++--- tests/fixtures/test-layout-expected.json | 1 + .../oauth-login-cli-browser-launch.test.ts | 242 ++++++++++++++++++ 6 files changed, 388 insertions(+), 30 deletions(-) create mode 100644 src/lib/browser-launch-notice.ts create mode 100644 tests/oauth/oauth-login-cli-browser-launch.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index a8d43bc015c..d5466e31887 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1031,6 +1031,7 @@ "oauth-first-add-hint.test.ts": "gui", "oauth-health.test.ts": "oauth", "oauth-log.test.ts": "oauth", + "oauth-login-cli-browser-launch.test.ts": "oauth", "oauth-login-cli-live-update.test.ts": "oauth", "oauth-login-open-browser.test.ts": "oauth", "oauth-login-summary.test.ts": "oauth", diff --git a/src/cli/account-auth.ts b/src/cli/account-auth.ts index de6364eb05f..bf233f0462c 100644 --- a/src/cli/account-auth.ts +++ b/src/cli/account-auth.ts @@ -2,6 +2,7 @@ import { writeSync } from "node:fs"; import { modelSelectionGuidance, modelSelectionNextSteps } from "./model-selection-guidance"; import { warnIfCodexCatalogRefreshPending } from "./account-catalog-refresh"; import { isCodexResetCreditOperationId } from "../codex/reset-credit-recovery"; +import { BROWSER_LAUNCH_FAILED_NOTICE } from "../lib/browser-launch-notice"; import { CliUsageError, printData, @@ -80,9 +81,12 @@ interface LoginStart { * either way, so the user waits at a terminal that looks like it is working. Names the fixed * callback port because that is the part people cannot guess — ChatGPT supplies the redirect * URI, so the flow cannot move to a free port, and `--device` is the way around it. + * + * Extends the shared notice rather than repeating it: only the second line is specific to this + * flow, and the first is the sentence every other login prints for the same failure. */ export const BROWSER_LAUNCH_FAILED_HINT = - "⚠️ No browser could be opened here — open the URL above yourself." + BROWSER_LAUNCH_FAILED_NOTICE + "\n If nothing on this machine can reach http://localhost:1455, rerun with --device instead."; /** `-` means "read it from stdin", the documented way to pass a code silently. */ diff --git a/src/lib/browser-launch-notice.ts b/src/lib/browser-launch-notice.ts new file mode 100644 index 00000000000..fb39c0193e1 --- /dev/null +++ b/src/lib/browser-launch-notice.ts @@ -0,0 +1,59 @@ +import type { OpenUrlResult } from "./open-url"; + +/** + * The one sentence a terminal login says when nothing opened. + * + * Stated once because it is now said from three places — the Codex account login, the generic + * OAuth login and the key login — and a sentence restated three times drifts three ways. Callers + * that have something more specific to add append to it rather than rewriting it, so the part a + * user learns to recognize stays identical everywhere. + */ +export const BROWSER_LAUNCH_FAILED_NOTICE = + "⚠️ No browser could be opened here — open the URL above yourself."; + +/** + * Report a browser launch whose answer arrives after the code that started it has moved on. + * + * The OAuth controller does not await `onAuth`, so a CLI login cannot simply await the launcher + * there: the flow continues, and on a callback-server provider the very next thing it does is + * draw a readline prompt. A warning written at that moment lands on the line the user is typing + * on, which is worse than not warning at all. + * + * So the launch reports itself as soon as it settles, and anything that would collide with it + * waits on {@link BrowserLaunchReport.settled} first. The launcher answers within its own settle + * window, so the wait costs a fraction of a second and buys a deterministic order. + */ +export interface BrowserLaunchReport { + /** Adopt a launch already in flight. Its failure is reported once, when it settles. */ + track(launch: Promise): void; + /** + * Resolves once every launch tracked BEFORE this call has been reported, and immediately when + * none was. A login publishes one URL, so "before this call" and "at all" are the same set + * here; the narrower promise is the one this actually keeps. + */ + settled(): Promise; +} + +export function createBrowserLaunchReport( + warn: (message: string) => void = message => { console.warn(message); }, +): BrowserLaunchReport { + let pending: Promise = Promise.resolve(); + return { + track(launch) { + pending = pending + .then(() => launch) + .then( + result => { + if (result.status !== "started") warn(`\n${BROWSER_LAUNCH_FAILED_NOTICE}`); + }, + // openUrl is documented never to reject, and a launcher that did would mean the same + // thing as one that failed. Swallowing it here is not politeness: this chain is what + // `settled()` hands to a prompt, so a rejection would propagate out of a login that + // is still perfectly able to continue, and would go unhandled in the polling flows + // that do not reach that await until minutes later. + () => { warn(`\n${BROWSER_LAUNCH_FAILED_NOTICE}`); }, + ); + }, + settled: () => pending, + }; +} diff --git a/src/oauth/login-cli.ts b/src/oauth/login-cli.ts index 19ad8f833cd..a842affc9ca 100644 --- a/src/oauth/login-cli.ts +++ b/src/oauth/login-cli.ts @@ -2,6 +2,7 @@ import * as readline from "node:readline"; import { modelSelectionGuidance } from "../cli/model-selection-guidance"; import { initializeProviderModelSelection } from "../providers/initial-model-selection"; import { openUrl } from "../lib/open-url"; +import { createBrowserLaunchReport } from "../lib/browser-launch-notice"; import { loadConfig, saveConfig } from "../config"; import { findLiveProxy } from "../server/proxy-liveness"; import { @@ -14,6 +15,41 @@ import type { OcxConfig, OcxProviderConfig } from "../types"; import { configuredAdminToken } from "../lib/admin-secrets"; import { codexAccountNamespaceProviderCollisionError } from "../codex/account-namespace-match"; +/** + * Seams a test drives in place of a browser, a terminal and a real provider. Production passes + * none of them. + * + * They exist because the thing worth proving here is an ORDER — that a browser which did not + * open is on screen before the question that assumes it did — and an order is only observable + * from something that records both events. Spawning a launcher and attaching to stdin to find + * that out would test the operating system instead. + */ +export interface LoginCliDeps { + runLogin?: typeof runLogin; + openUrl?: typeof openUrl; + warn?: (message: string) => void; + /** Ask one question, read one line. Defaults to a readline prompt that owns its own lifetime. */ + ask?: (question: string) => Promise; +} + +/** + * Run `body` with a line reader, creating and closing a real one only when the caller did not + * supply its own. A readline interface attaches to stdin, so building one that nothing will ask + * a question keeps the process alive for no reason. + */ +async function withPrompt( + supplied: ((question: string) => Promise) | undefined, + body: (ask: (question: string) => Promise) => Promise, +): Promise { + if (supplied) return await body(supplied); + const rl = readline.createInterface({ input: process.stdin, output: process.stdout }); + try { + return await body(question => new Promise(resolve => rl.question(question, resolve))); + } finally { + rl.close(); + } +} + const LIVE_RELOAD_PROVIDERS = new Set([ ...listOAuthProviders(), ...Object.keys(KEY_LOGIN_PROVIDERS), @@ -83,7 +119,7 @@ export function loginUsageMessage(): string { + ` API-key login: ${Object.keys(KEY_LOGIN_PROVIDERS).join(", ")}`; } -export async function handleLogin(provider?: string): Promise { +export async function handleLogin(provider?: string, deps: LoginCliDeps = {}): Promise { const name = (provider ?? "").trim().toLowerCase(); // A removed provider id reached through its alias still logs in — the merged // successor owns the flow. Warn rather than silently reroute so scripts and @@ -91,30 +127,40 @@ export async function handleLogin(provider?: string): Promise { const alias = DEPRECATED_OAUTH_PROVIDER_ALIASES[name]; if (alias) { console.error(`${name} is deprecated; logging in as ${alias}`); - return handleOAuthLogin(alias); + return handleOAuthLogin(alias, deps); } - if (isPublicOAuthProvider(name)) return handleOAuthLogin(name); - if (isKeyLoginProvider(name)) return handleKeyLogin(name); + if (isPublicOAuthProvider(name)) return handleOAuthLogin(name, deps); + if (isKeyLoginProvider(name)) return handleKeyLogin(name, deps); console.error(loginUsageMessage()); process.exit(1); } -async function handleOAuthLogin(name: string): Promise { - const rl = readline.createInterface({ input: process.stdin, output: process.stdout }); - try { - await runLogin(name, { +export async function handleOAuthLogin(name: string, deps: LoginCliDeps = {}): Promise { + const login = deps.runLogin ?? runLogin; + const launch = deps.openUrl ?? openUrl; + const browser = createBrowserLaunchReport(deps.warn); + await withPrompt(deps.ask, async (ask) => { + await login(name, { onAuth: ({ url, instructions }) => { console.log(`\n🔐 Opening browser for ${name} login...\n${url}\n`); if (instructions) console.log(instructions); - void openUrl(url); + // The controller does not await onAuth, so the launcher's answer cannot be reported from + // here — this returns long before it arrives. It reports itself instead, and the one + // thing that could collide with it waits below (#5261). + browser.track(launch(url)); }, onProgress: (m) => console.log(` ${m}`), - onManualCodeInput: () => - new Promise((res) => rl.question("Paste redirect URL or code (or wait for browser): ", res)), + onManualCodeInput: async () => { + // "or wait for browser" is a lie if nothing opened, and a warning printed after readline + // has drawn the prompt lands on the line the user is typing on. + await browser.settled(); + return await ask("Paste redirect URL or code (or wait for browser): "); + }, }); - } finally { - rl.close(); - } + }); + // A device or polling provider never prompts, so nothing above waited on the launcher. It is + // still owed an answer before this claims the login worked. + await browser.settled(); const reload = await notifyRunningProxyAfterOAuthLogin(name); console.log(`\n✅ Logged in to ${name}. Try: ocx sync`); for (const line of modelSelectionGuidance(name)) console.log(line); @@ -192,8 +238,9 @@ export async function commitKeyLoginProvider( return mergedProvider; } -async function handleKeyLogin(name: string): Promise { +export async function handleKeyLogin(name: string, deps: LoginCliDeps = {}): Promise { const def = KEY_LOGIN_PROVIDERS[name]; + const launch = deps.openUrl ?? openUrl; const preflightConfig = loadConfig(); const namespaceCollision = codexAccountNamespaceProviderCollisionError(preflightConfig.codexAccountNamespaces, name); if (namespaceCollision) { @@ -201,21 +248,25 @@ async function handleKeyLogin(name: string): Promise { process.exit(1); } console.log(`\n🔑 ${def.label} — opening ${def.dashboardUrl} so you can create/copy an API key...`); - void openUrl(def.dashboardUrl); - const rl = readline.createInterface({ input: process.stdin, output: process.stdout }); - const key = (await new Promise((res) => rl.question(`Paste your ${def.label} API key: `, res))).trim(); - // Template URL with placeholders needs resolution before saving. - let baseUrl = def.baseUrl; - if (/\{[^}]*\}/.test(baseUrl)) { - const resolved = (await new Promise((res) => rl.question(`Your endpoint URL (${baseUrl}): `, res))).trim(); - if (!resolved) { - rl.close(); - console.error("A resolved URL is required — replace the {placeholder} with your actual value."); - process.exit(1); + const browser = createBrowserLaunchReport(deps.warn); + browser.track(launch(def.dashboardUrl)); + // The next question asks for a key the user gets FROM that page, so a page that never opened + // has to be on screen before the question rather than underneath it (#5261). + await browser.settled(); + const { key, baseUrl } = await withPrompt(deps.ask, async (ask) => { + const entered = (await ask(`Paste your ${def.label} API key: `)).trim(); + // Template URL with placeholders needs resolution before saving. + let resolvedBaseUrl = def.baseUrl; + if (/\{[^}]*\}/.test(resolvedBaseUrl)) { + const resolved = (await ask(`Your endpoint URL (${resolvedBaseUrl}): `)).trim(); + if (!resolved) { + console.error("A resolved URL is required — replace the {placeholder} with your actual value."); + process.exit(1); + } + resolvedBaseUrl = resolved; } - baseUrl = resolved; - } - rl.close(); + return { key: entered, baseUrl: resolvedBaseUrl }; + }); if (!key) { console.error("No key entered."); process.exit(1); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 53fc4cbab3d..a34abfa4bbc 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -857,6 +857,7 @@ "oauth-first-add-hint.test.ts": "gui", "oauth-health.test.ts": "oauth", "oauth-log.test.ts": "oauth", + "oauth-login-cli-browser-launch.test.ts": "oauth", "oauth-login-cli-live-update.test.ts": "oauth", "oauth-login-open-browser.test.ts": "oauth", "oauth-login-summary.test.ts": "oauth", diff --git a/tests/oauth/oauth-login-cli-browser-launch.test.ts b/tests/oauth/oauth-login-cli-browser-launch.test.ts new file mode 100644 index 00000000000..1efad4b987b --- /dev/null +++ b/tests/oauth/oauth-login-cli-browser-launch.test.ts @@ -0,0 +1,242 @@ +/** + * #5261 remainder: the two CLI logins that still discarded the launcher's answer. + * + * The landed half taught `openUrl` to say whether anything started, and the Codex account login + * to print it. `ocx login ` and `ocx login ` kept calling + * `void openUrl(...)`, so on a host with no browser both printed a URL, claimed to be opening + * it, and then asked a question that assumes it opened. The user waits at a prompt that looks + * like progress. + * + * What these assert is the ORDER, not merely the presence of a warning. The OAuth controller + * does not await `onAuth`, so the launcher answers after the login flow has moved on — on a + * callback-server provider, after it has already drawn a readline prompt. A warning that lands + * there is written over the line the user is typing on, which is why "it warns eventually" is + * not the contract. + * + * Nothing here opens a browser or attaches to stdin: both are injected, which is the only way + * two events can be observed in sequence at all. + */ +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { saveConfig } from "../../src/config"; +import { handleKeyLogin, handleOAuthLogin, type LoginCliDeps } from "../../src/oauth/login-cli"; +import { KEY_LOGIN_PROVIDERS } from "../../src/oauth/key-providers"; +import { listOAuthProviders } from "../../src/oauth"; +import { BROWSER_LAUNCH_FAILED_NOTICE, createBrowserLaunchReport } from "../../src/lib/browser-launch-notice"; +import { BROWSER_LAUNCH_FAILED_HINT } from "../../src/cli/account-auth"; +import { repoPath } from "../helpers/repo-root"; +import { readFileSync } from "node:fs"; +import type { OpenUrlResult } from "../../src/lib/open-url"; +import type { OAuthCredentials } from "../../src/oauth/types"; +import type { OcxConfig } from "../../src/types"; +import { installIsolatedCodexHome, type IsolatedCodexHome } from "../helpers/isolated-codex-home"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +let testDir = ""; +let previousHome: string | undefined; +let isolatedCodexHome: IsolatedCodexHome | null = null; +let logSpy: { mockRestore(): void } | null = null; + +type FakeRunLogin = NonNullable; + +/** Named from the roster rather than typed in, so a renamed provider cannot leave this passing. */ +function anyOAuthProvider(): string { + const [first] = listOAuthProviders(); + if (!first) throw new Error("no OAuth providers are registered"); + return first; +} + +/** A key provider whose baseUrl needs no placeholder resolution, so one prompt ends the flow. */ +function anyDirectKeyProvider(): string { + const found = Object.entries(KEY_LOGIN_PROVIDERS).find(([, def]) => !/\{[^}]*\}/.test(def.baseUrl)); + if (!found) throw new Error("no key-login provider has a resolved baseUrl"); + return found[0]; +} + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + isolatedCodexHome = installIsolatedCodexHome("ocx-login-launch-codex-"); + testDir = mkdtempSync(join(tmpdir(), "ocx-login-launch-")); + process.env.OPENCODEX_HOME = testDir; + // The one row exists so the config validates; it is deliberately not the provider under test, + // which keeps the post-login live-reload notify from looking for a proxy. An empty table would + // fail validation and be silently replaced by the packaged default, whose contents this has no + // reason to depend on. + saveConfig({ + port: 0, + hostname: "127.0.0.1", + defaultProvider: "login-launch-stub", + providers: { + "login-launch-stub": { + adapter: "openai-chat", + baseUrl: "https://stub.invalid/v1", + apiKey: "sk-login-launch-stub", + }, + }, + } as OcxConfig); + logSpy = spyOn(console, "log").mockImplementation(() => {}); +}); + +afterEach(() => { + logSpy?.mockRestore(); + logSpy = null; + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + isolatedCodexHome?.restore(); + isolatedCodexHome = null; + if (testDir) removeTreeWithRetry(testDir); + testDir = ""; +}); + +describe("CLI OAuth login reports the browser launch (#5261)", () => { + test("a browser that never opened is on screen before the paste prompt", async () => { + const events: string[] = []; + let settleLaunch: (result: OpenUrlResult) => void = () => {}; + const launch = new Promise(resolve => { settleLaunch = resolve; }); + const login: FakeRunLogin = async (_provider, ctrl) => { + ctrl.onAuth?.({ url: "https://accounts.example.test/authorize?state=1" }); + // The launcher answers only after onAuth has returned. That gap is the defect: the old + // code had already discarded the promise by this point. + settleLaunch({ status: "failed", reason: "launcher-exit" }); + expect(await ctrl.onManualCodeInput?.()).toBe("pasted-code"); + return {} as OAuthCredentials; + }; + + await handleOAuthLogin(anyOAuthProvider(), { + runLogin: login, + openUrl: () => launch, + warn: message => { events.push(`warn:${message}`); }, + ask: async () => { events.push("ask"); return "pasted-code"; }, + }); + + expect(events).toEqual([`warn:\n${BROWSER_LAUNCH_FAILED_NOTICE}`, "ask"]); + }); + + test("a browser that did open adds nothing to the prompt", async () => { + // A non-regression guard rather than proof of the fix: it passed before this change too, and + // it is here so the new warning cannot start firing on a launch that worked. + const events: string[] = []; + const login: FakeRunLogin = async (_provider, ctrl) => { + ctrl.onAuth?.({ url: "https://accounts.example.test/authorize?state=2" }); + await ctrl.onManualCodeInput?.(); + return {} as OAuthCredentials; + }; + + await handleOAuthLogin(anyOAuthProvider(), { + runLogin: login, + openUrl: async () => ({ status: "started" }), + warn: message => { events.push(`warn:${message}`); }, + ask: async () => { events.push("ask"); return "pasted-code"; }, + }); + + expect(events).toEqual(["ask"]); + }); + + test("a polling flow that never prompts is still told the launch failed", async () => { + // Device and polling providers publish a URL and then wait. Nothing asks a question, so + // nothing there would have waited on the launcher; the answer is still owed before the + // login claims to have worked. + const events: string[] = []; + const login: FakeRunLogin = async (_provider, ctrl) => { + ctrl.onAuth?.({ url: "https://device.example.test/activate", deviceCode: "WDJB-MJHT" }); + ctrl.onProgress?.("Waiting for approval..."); + return {} as OAuthCredentials; + }; + + await handleOAuthLogin(anyOAuthProvider(), { + runLogin: login, + openUrl: async () => ({ status: "failed", reason: "spawn-error" }), + warn: () => { events.push("warn"); }, + ask: async () => { events.push("ask"); return ""; }, + }); + + expect(events).toEqual(["warn"]); + }); + + test("a launcher that throws is reported, not turned into a failed login", async () => { + // openUrl documents that it never rejects, but this seam accepts any launcher. A rejection + // here used to travel out through settled(), which a polling flow does not await until the + // whole login has finished — so it would surface as an unhandled rejection and take down a + // login that could still have completed by hand. + const events: string[] = []; + const login: FakeRunLogin = async (_provider, ctrl) => { + ctrl.onAuth?.({ url: "https://accounts.example.test/authorize?state=3" }); + ctrl.onProgress?.("Waiting for browser authentication..."); + return {} as OAuthCredentials; + }; + + await handleOAuthLogin(anyOAuthProvider(), { + runLogin: login, + openUrl: async () => { throw new Error("launcher blew up"); }, + warn: () => { events.push("warn"); }, + ask: async () => { events.push("ask"); return ""; }, + }); + + expect(events).toEqual(["warn"]); + }); +}); + +describe("CLI key login reports the dashboard launch (#5261)", () => { + test("a dashboard that never opened is on screen before the key prompt", async () => { + const events: string[] = []; + const errors: string[] = []; + const errorSpy = spyOn(console, "error").mockImplementation((...parts: unknown[]) => { + errors.push(parts.join(" ")); + }); + const exitSpy = spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit:${code}`); + }) as never); + + try { + await expect(handleKeyLogin(anyDirectKeyProvider(), { + openUrl: async () => ({ status: "failed", reason: "launcher-exit" }), + warn: () => { events.push("warn"); }, + // An empty key ends the flow immediately after the prompt. The key path beyond it is + // already covered where it lives; what this case is about is what precedes the question. + ask: async () => { events.push("ask"); return ""; }, + })).rejects.toThrow("process.exit:1"); + } finally { + exitSpy.mockRestore(); + errorSpy.mockRestore(); + } + + expect(events).toEqual(["warn", "ask"]); + expect(errors).toContain("No key entered."); + }); +}); + +describe("the launch report is one sentence and one order", () => { + test("settled() cannot resolve before the warning is written", async () => { + const events: string[] = []; + let settleLaunch: (result: OpenUrlResult) => void = () => {}; + const launch = new Promise(resolve => { settleLaunch = resolve; }); + const report = createBrowserLaunchReport(() => { events.push("warn"); }); + + report.track(launch); + const waited = report.settled().then(() => { events.push("prompt"); }); + settleLaunch({ status: "failed", reason: "invalid-url" }); + await waited; + + expect(events).toEqual(["warn", "prompt"]); + }); + + test("a report with nothing tracked resolves rather than hanging", async () => { + const events: string[] = []; + await createBrowserLaunchReport(() => { events.push("warn"); }).settled(); + expect(events).toEqual([]); + }); + + test("the Codex account hint extends the shared notice instead of holding its own copy", () => { + // Asserting only that the strings agree would pass on a second copy that happens to match + // today, which is the state this replaced. The source is read as well, so the sentence has + // exactly one home and a later edit to it cannot reach two thirds of the logins. + expect(BROWSER_LAUNCH_FAILED_HINT.startsWith(BROWSER_LAUNCH_FAILED_NOTICE)).toBe(true); + expect(BROWSER_LAUNCH_FAILED_HINT.length).toBeGreaterThan(BROWSER_LAUNCH_FAILED_NOTICE.length); + + const accountAuth = readFileSync(repoPath("src", "cli", "account-auth.ts"), "utf8"); + expect(accountAuth).toContain("BROWSER_LAUNCH_FAILED_NOTICE"); + expect(accountAuth).not.toContain(BROWSER_LAUNCH_FAILED_NOTICE); + }); +}); From cd8c52829063bc5f46d097a57b6ddb697fed55ec Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 21:11:39 +0900 Subject: [PATCH 2/3] fix(gui): stop a failed account refresh from reading as a current roster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a successful load, a failed account read left the Codex pool reporting `ready` with the rows it had before. Keeping those rows is deliberate — blanking a populated pool because one 30s poll missed is its own defect — but nothing distinguished a list the server had just confirmed from one that predated a failure. The shape a user hits: add an account, the read that would bring it over fails, and the dashboard shows the older accounts with the new one simply absent and no indication that anything went wrong (#5261). The controller now carries `refreshFailed` alongside `loadState`, for the same reason `refreshing` already lives there: `loadState` answers what the surface can draw, and a warm failure does not change that answer. Folding the failure into `loadState` would mean either flashing the cold skeleton over good data or saying nothing, and saying nothing is what this fixes. A cold failure still replaces the surface with its existing error. The pool renders a non-destructive amber status above the rows it is qualifying, with the retry the cold error already offers. It appears only when rows survived, so an empty cold failure still shows its own message rather than a banner describing nothing. The load-states prop also stops restating the load-state union and derives it from the controller instead. --- gui/src/components/CodexAccountPool.tsx | 3 +- .../codex-account-pool-main-card.tsx | 17 +- .../components/codex-account-pool-types.ts | 2 +- gui/src/hooks/useCodexAccountPool.ts | 23 +- gui/src/i18n/de.ts | 1 + gui/src/i18n/en.ts | 1 + gui/src/i18n/fr.ts | 1 + gui/src/i18n/ja.ts | 1 + gui/src/i18n/ko.ts | 1 + gui/src/i18n/ru.ts | 1 + gui/src/i18n/tr.ts | 1 + gui/src/i18n/vi.ts | 1 + gui/src/i18n/zh-TW.ts | 1 + gui/src/i18n/zh.ts | 1 + .../styles/provider-workspace-settings.css | 1 + .../codex-account-pool-controller.test.ts | 4 + .../codex-account-pool-stale-refresh.test.tsx | 270 ++++++++++++++++++ .../codex-account-pool-toast-tone.test.tsx | 1 + 18 files changed, 325 insertions(+), 6 deletions(-) create mode 100644 gui/tests/codex-account-pool-stale-refresh.test.tsx diff --git a/gui/src/components/CodexAccountPool.tsx b/gui/src/components/CodexAccountPool.tsx index e9be8f96970..20e5532d1da 100644 --- a/gui/src/components/CodexAccountPool.tsx +++ b/gui/src/components/CodexAccountPool.tsx @@ -72,7 +72,7 @@ export default function CodexAccountPool({ apiBase, accountModeState = null, ban // but stays inert (no load, no polling) whenever a shared controller was injected. const ownController = useCodexAccountPool(apiBase, !injectedController); const controller = injectedController ?? ownController; - const { accounts, activeId, loadState, switchingId, pauseUpdatingId, priorityUpdatingId, pausingExhausted, activePinnedId, load } = controller; + const { accounts, activeId, loadState, refreshFailed, switchingId, pauseUpdatingId, priorityUpdatingId, pausingExhausted, activePinnedId, load } = controller; // #3898: the native-main device reauth drives the dedicated namespace; a // completed flow refreshes the account list so the card leaves reauth state. const mainReauth = useMainDeviceReauth(apiBase, () => { void load(); }); @@ -476,6 +476,7 @@ export default function CodexAccountPool({ apiBase, accountModeState = null, ban { void load(); }} /> diff --git a/gui/src/components/codex-account-pool-main-card.tsx b/gui/src/components/codex-account-pool-main-card.tsx index 25af1eb49f9..051424184f5 100644 --- a/gui/src/components/codex-account-pool-main-card.tsx +++ b/gui/src/components/codex-account-pool-main-card.tsx @@ -3,7 +3,7 @@ import { IconLock, IconPause, IconPlay, IconPlus, IconRefresh, IconTicket } from import AccountPriorityControl, { AccountPriorityBadge } from "./AccountPriorityControl"; import QuotaBars from "./QuotaBars"; import { CodexPauseToggleLabel, CodexTicketBadge } from "./codex-account-pool-helpers"; -import type { CodexAccountEntry } from "./codex-account-pool-types"; +import type { CodexAccountEntry, CodexAccountLoadState } from "./codex-account-pool-types"; import type { CodexAccountModeState } from "../codex-multi-state"; import type { TFn } from "../i18n/shared"; import type { MainDeviceReauthState } from "./use-main-device-reauth"; @@ -359,11 +359,13 @@ export function CodexAccountPoolActions(props: { export function CodexAccountPoolLoadStates({ t, loadState, + refreshFailed, accountsCount, onRetry, }: { t: TFn; - loadState: "loading" | "ready" | "error"; + loadState: CodexAccountLoadState; + refreshFailed: boolean; accountsCount: number; onRetry: () => void; }): ReactNode { @@ -419,5 +421,16 @@ export function CodexAccountPoolLoadStates({ ); } + // Rows survived a failed refresh, so they are still worth showing — but they are the ones from + // before it, and an account added since is simply not among them. A status rather than an alert: + // nothing on screen is wrong, it is just older than it looks. + if (refreshFailed && accountsCount > 0) { + return ( +
+ {t("codexAuth.accountsRefreshFailed")} + +
+ ); + } return null; } diff --git a/gui/src/components/codex-account-pool-types.ts b/gui/src/components/codex-account-pool-types.ts index 679350fa23f..0e16acca880 100644 --- a/gui/src/components/codex-account-pool-types.ts +++ b/gui/src/components/codex-account-pool-types.ts @@ -1 +1 @@ -export type { CodexAccountEntry } from "../hooks/useCodexAccountPool"; +export type { CodexAccountEntry, CodexAccountLoadState } from "../hooks/useCodexAccountPool"; diff --git a/gui/src/hooks/useCodexAccountPool.ts b/gui/src/hooks/useCodexAccountPool.ts index cb5fce92968..fe4cdaa155b 100644 --- a/gui/src/hooks/useCodexAccountPool.ts +++ b/gui/src/hooks/useCodexAccountPool.ts @@ -97,6 +97,11 @@ export interface CodexAccountPoolController { * `ready` during a refresh so rows survive; this is what makes that wait visible. */ refreshing: boolean; + /** + * The most recent account read failed. The rows it could not replace are still on screen, so + * this is the only thing that tells a surface they are no longer known to be current. + */ + refreshFailed: boolean; /** True until the first load attempt settles, whether it succeeds or fails. */ initialLoading: boolean; switchingId: string | null; @@ -160,6 +165,13 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou ); const [activeId, setActiveId] = useState(() => seed?.activeId ?? null); const [loadState, setLoadState] = useState(() => (seed != null ? "ready" : "loading")); + // Deliberately beside `loadState` rather than inside it. `loadState` answers what the surface + // can draw, and a warm refresh failure keeps the rows drawable — folding the failure in would + // mean either flashing the cold skeleton over good data or, as before, saying nothing at all. + // Saying nothing is the defect: the rows on screen are the ones from before the refresh, so an + // account the user has just added is simply absent while the older ones look current (#5261). + // `refreshing` already set the precedent that a fact about the read lives next to loadState. + const [refreshFailed, setRefreshFailed] = useState(false); const [switchingId, setSwitchingId] = useState(null); const [pauseUpdatingId, setPauseUpdatingId] = useState(null); const [priorityUpdatingId, setPriorityUpdatingId] = useState(null); @@ -277,6 +289,10 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou hasLoadedRef.current = true; // Progressive: paint account/quota boxes as soon as /accounts returns. setLoadState("ready"); + // Cleared here rather than at the settle below, because the rows it qualifies are + // painted here. Waiting for /active to finish would leave the just-replaced rows + // labelled as pre-refresh ones for as long as that read's budget allows. + setRefreshFailed(false); } return true; } catch { @@ -329,9 +345,11 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou }); return activeOk; } - // Cold failure only: after a successful load (including empty), keep rows and stay ready - // so a soft poll miss does not flash the skeleton / wipe the pool. + // A cold failure has nothing to show, so it replaces the surface. A warm one keeps its rows + // — flashing the skeleton on a soft poll miss is its own defect — and says so instead of + // continuing to present them as current. if (!hasLoadedRef.current) setLoadState("error"); + setRefreshFailed(true); return false; } finally { bounded.clear(); @@ -623,6 +641,7 @@ export function useCodexAccountPool(apiBase: string, enabled = true): CodexAccou activeId, loadState, refreshing: inflightCount > 0, + refreshFailed, initialLoading: !firstAttemptSettled, switchingId, pauseUpdatingId, diff --git a/gui/src/i18n/de.ts b/gui/src/i18n/de.ts index a46c0f89fa0..baf6776b8ee 100644 --- a/gui/src/i18n/de.ts +++ b/gui/src/i18n/de.ts @@ -1670,6 +1670,7 @@ export const de: Record = { "accountPool.priorityUpdateFailed": "Die Auswahlreihenfolge für {email} konnte nicht gespeichert werden. Der zuletzt bestätigte Wert wird angezeigt.", "codexAuth.switched": "{email} ist für die nächste Anfrage ausgewählt", "codexAuth.loadFailed": "Die Codex-Kontoeinstellungen konnten nicht geladen werden.", + "codexAuth.accountsRefreshFailed": "Die letzte Kontoaktualisierung ist fehlgeschlagen. Unten stehen die zuletzt bestätigten Konten.", "codexAuth.switchFailed": "Das Konto konnte nicht gewechselt werden. Die vorherige Auswahl bleibt erhalten.", "codexAuth.removeConfirm": "{id} entfernen?", "codexAuth.removeFailed": "Das Konto konnte nicht entfernt werden. Es wurde nichts geändert.", diff --git a/gui/src/i18n/en.ts b/gui/src/i18n/en.ts index 061019df6c3..0a67cb6b217 100644 --- a/gui/src/i18n/en.ts +++ b/gui/src/i18n/en.ts @@ -2264,6 +2264,7 @@ export const en = { "codexAuth.switched": "{email} is selected for the next request", "codexAuth.loadFailed": "Codex account settings could not be loaded.", + "codexAuth.accountsRefreshFailed": "The latest account refresh failed. The accounts below are the last ones confirmed.", "codexAuth.switchFailed": "The account could not be switched. Your previous selection is unchanged.", "codexAuth.removeConfirm": "Remove {id}?", "codexAuth.removeFailed": "The account could not be removed. Nothing was changed.", diff --git a/gui/src/i18n/fr.ts b/gui/src/i18n/fr.ts index b2e00370208..8e705317f8e 100644 --- a/gui/src/i18n/fr.ts +++ b/gui/src/i18n/fr.ts @@ -2186,6 +2186,7 @@ export const fr: Record = { "accountPool.priorityUpdateFailed": "Impossible d’enregistrer l’ordre de sélection de {email}. La dernière valeur confirmée est affichée.", "codexAuth.switched": "{email} est sélectionné pour la prochaine requête", "codexAuth.loadFailed": "Impossible de charger les paramètres des comptes Codex.", + "codexAuth.accountsRefreshFailed": "La dernière actualisation des comptes a échoué. Les comptes ci-dessous sont les derniers confirmés.", "codexAuth.switchFailed": "Impossible de changer de compte. Votre sélection précédente reste inchangée.", "codexAuth.removeConfirm": "Supprimer {id} ?", "codexAuth.removeFailed": "Impossible de supprimer le compte. Aucune modification apportée.", diff --git a/gui/src/i18n/ja.ts b/gui/src/i18n/ja.ts index 7ab8054d2ed..29dd05ef173 100644 --- a/gui/src/i18n/ja.ts +++ b/gui/src/i18n/ja.ts @@ -2114,6 +2114,7 @@ export const ja: Record = { "accountPool.priorityUpdateFailed": "{email} の選択順序を保存できませんでした。最後に確認された値を表示しています。", "codexAuth.switched": "次のリクエストでは {email} を使用します", "codexAuth.loadFailed": "Codex アカウント設定を読み込めませんでした。", + "codexAuth.accountsRefreshFailed": "最新のアカウント更新に失敗しました。以下は最後に確認されたアカウントです。", "codexAuth.switchFailed": "アカウントを切り替えられませんでした。以前の選択はそのままです。", "codexAuth.removeConfirm": "{id} を削除しますか?", "codexAuth.removeFailed": "アカウントを削除できませんでした。何も変更されていません。", diff --git a/gui/src/i18n/ko.ts b/gui/src/i18n/ko.ts index 31d5c4af19c..d965ca260db 100644 --- a/gui/src/i18n/ko.ts +++ b/gui/src/i18n/ko.ts @@ -1706,6 +1706,7 @@ export const ko: Record = { "accountPool.priorityUpdateFailed": "{email}의 선택 순서를 저장하지 못했습니다. 마지막으로 확인된 값을 표시합니다.", "codexAuth.switched": "다음 요청에 {email}을(를) 사용합니다", "codexAuth.loadFailed": "Codex 계정 설정을 불러오지 못했습니다.", + "codexAuth.accountsRefreshFailed": "최신 계정 새로고침에 실패했습니다. 아래 목록은 마지막으로 확인된 계정입니다.", "codexAuth.switchFailed": "계정을 전환하지 못했습니다. 이전 선택은 그대로 유지됩니다.", "codexAuth.removeConfirm": "{id}을(를) 삭제하시겠습니까?", "codexAuth.removeFailed": "계정을 제거하지 못했습니다. 변경된 내용은 없습니다.", diff --git a/gui/src/i18n/ru.ts b/gui/src/i18n/ru.ts index 9a0f4fc0920..63b27cc5eff 100644 --- a/gui/src/i18n/ru.ts +++ b/gui/src/i18n/ru.ts @@ -2185,6 +2185,7 @@ export const ru: Record = { "accountPool.priorityUpdateFailed": "Не удалось сохранить порядок выбора для {email}. Показано последнее подтверждённое значение.", "codexAuth.switched": "{email} выбран для следующего запроса", "codexAuth.loadFailed": "Не удалось загрузить настройки аккаунтов Codex.", + "codexAuth.accountsRefreshFailed": "Последнее обновление аккаунтов не удалось. Ниже показаны последние подтверждённые аккаунты.", "codexAuth.switchFailed": "Не удалось переключить аккаунт. Ваш предыдущий выбор не изменён.", "codexAuth.removeConfirm": "Удалить {id}?", "codexAuth.removeFailed": "Не удалось удалить аккаунт. Ничего не изменено.", diff --git a/gui/src/i18n/tr.ts b/gui/src/i18n/tr.ts index f8a086ac3ab..2fb46067b86 100644 --- a/gui/src/i18n/tr.ts +++ b/gui/src/i18n/tr.ts @@ -2205,6 +2205,7 @@ export const tr: Record = { "codexAuth.switched": "Sonraki istek için {email} seçildi", "codexAuth.loadFailed": "Codex hesap ayarları yüklenemedi.", + "codexAuth.accountsRefreshFailed": "Son hesap yenilemesi başarısız oldu. Aşağıda son doğrulanan hesaplar gösteriliyor.", "codexAuth.switchFailed": "Hesap değiştirilemedi.", "codexAuth.removeConfirm": "{id} kaldırılsın mı?", "codexAuth.removeFailed": "Hesap kaldırılamadı.", diff --git a/gui/src/i18n/vi.ts b/gui/src/i18n/vi.ts index f9eabd994c7..f14875f0824 100644 --- a/gui/src/i18n/vi.ts +++ b/gui/src/i18n/vi.ts @@ -2202,6 +2202,7 @@ export const vi: Record = { "accountPool.priorityUpdateFailed": "Không thể lưu thứ tự lựa chọn cho {email}. Giá trị được xác nhận gần nhất đang được hiển thị.", "codexAuth.switched": "Tài khoản được chọn cho request tiếp theo là {email}", "codexAuth.loadFailed": "Không thể tải cài đặt tài khoản Codex.", + "codexAuth.accountsRefreshFailed": "Lần làm mới tài khoản gần nhất thất bại. Dưới đây là các tài khoản được xác nhận gần nhất.", "codexAuth.switchExceedsThresholdWarning": "Mức sử dụng của tài khoản này đã đạt hoặc vượt ngưỡng chuyển đổi ({threshold}%). Lựa chọn đã ghim sẽ được giải phóng nếu không còn hạn ngạch khả dụng.", "codexAuth.switchFailed": "Không thể chuyển đổi tài khoản. Tùy chọn trước đó của bạn vẫn được giữ nguyên.", "codexAuth.removeConfirm": "Gỡ bỏ {id}?", diff --git a/gui/src/i18n/zh-TW.ts b/gui/src/i18n/zh-TW.ts index 25abb6135b4..c95bb6765e2 100644 --- a/gui/src/i18n/zh-TW.ts +++ b/gui/src/i18n/zh-TW.ts @@ -1669,6 +1669,7 @@ export const zhTW: Record = { "accountPool.quotaWindowInert": "只有配額策略，或門檻大於 0 的填滿優先策略，才會依用量計分；在目前的輪換策略下，這項設定不會有任何作用。", "codexAuth.switched": "下一次請求將使用 {email}", "codexAuth.loadFailed": "無法載入 Codex 帳號設定。", + "codexAuth.accountsRefreshFailed": "最近一次帳號重新整理失敗。以下是最後一次確認的帳號。", "codexAuth.switchFailed": "無法切換帳號。之前的選擇保持不變。", "codexAuth.removeConfirm": "刪除 {id}？", "codexAuth.removeFailed": "無法移除帳號。未進行任何更改。", diff --git a/gui/src/i18n/zh.ts b/gui/src/i18n/zh.ts index cf4b6e94055..9e41810935c 100644 --- a/gui/src/i18n/zh.ts +++ b/gui/src/i18n/zh.ts @@ -1687,6 +1687,7 @@ export const zh: Record = { "accountPool.priorityUpdateFailed": "无法保存 {email} 的选择顺序。当前显示最后一次确认的值。", "codexAuth.switched": "下一次请求将使用 {email}", "codexAuth.loadFailed": "无法加载 Codex 账号设置。", + "codexAuth.accountsRefreshFailed": "最近一次账号刷新失败。以下是最后一次确认的账号。", "codexAuth.switchFailed": "无法切换账户。之前的选择保持不变。", "codexAuth.removeConfirm": "删除 {id}？", "codexAuth.removeFailed": "无法移除账户。未进行任何更改。", diff --git a/gui/src/styles/provider-workspace-settings.css b/gui/src/styles/provider-workspace-settings.css index 34a9e808971..5dc15873e1f 100644 --- a/gui/src/styles/provider-workspace-settings.css +++ b/gui/src/styles/provider-workspace-settings.css @@ -29,6 +29,7 @@ background: var(--raised); border-radius: var(--radius-xs); } .pwi-auth-state--error { color: var(--red); background: var(--red-soft); justify-content: space-between; } +.pwi-auth-state--stale { color: var(--amber); background: var(--amber-soft); justify-content: space-between; } .pwi-auth-state--empty { justify-content: center; } .pwi-auth-actions { display: flex; align-items: center; gap: 8px; flex-wrap: wrap; margin-top: 8px; } diff --git a/gui/tests/codex-account-pool-controller.test.ts b/gui/tests/codex-account-pool-controller.test.ts index 044b8156a01..3f1d5c848d4 100644 --- a/gui/tests/codex-account-pool-controller.test.ts +++ b/gui/tests/codex-account-pool-controller.test.ts @@ -21,6 +21,10 @@ test("the controller is the single data owner and exposes the agreed contract", // WP2 (260730_gui_hydration_loading_unify/010): progress is part of the contract, because a // forced quota refresh keeps `loadState` at "ready" and would otherwise be invisible. "refreshing", "initialLoading", + // #5261: for the same reason in the other direction. A warm refresh failure keeps the rows + // and keeps `loadState` at "ready", so without this the surface has no way to say that what + // it is showing predates a failed read. + "refreshFailed", ]) { expect(hook).toContain(member); } diff --git a/gui/tests/codex-account-pool-stale-refresh.test.tsx b/gui/tests/codex-account-pool-stale-refresh.test.tsx new file mode 100644 index 00000000000..0f164c71a72 --- /dev/null +++ b/gui/tests/codex-account-pool-stale-refresh.test.tsx @@ -0,0 +1,270 @@ +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { Window } from "happy-dom"; +import { act } from "react"; +import type { Root } from "react-dom/client"; +import CodexAccountPool from "../src/components/CodexAccountPool"; +import { clearClientResourceStoresForTests } from "../src/client-resource"; +import { useCodexAccountPool, type CodexAccountEntry, type CodexAccountPoolController } from "../src/hooks/useCodexAccountPool"; +import { en } from "../src/i18n/en"; +import { LanguageProvider } from "../src/i18n/provider"; + +/** + * #5261: a failed account refresh used to leave the roster looking current. + * + * Keeping the rows is deliberate — blanking a populated pool on a soft poll miss is its own + * defect — but the controller also went on reporting `ready`, so nothing distinguished a list + * the server had just confirmed from one that predated a failure. The case that surfaced it: + * add an account, the read that would bring it over fails, and the dashboard shows the older + * accounts with the new one simply absent. + * + * Both halves are held here because either alone is satisfiable without the other: a flag the + * surface never reads changes nothing a user sees, and a banner with no flag behind it never + * appears. + */ + +const globals = ["document", "window", "navigator", "localStorage", "IS_REACT_ACT_ENVIRONMENT"] as const; +let previous: Record<(typeof globals)[number], unknown>; +let win: Window; +let host: HTMLElement; +let root: Root | null = null; +let originalFetch: typeof globalThis.fetch; +let accountsOk = true; +let serverAccounts: unknown[] = []; +let baseCounter = 0; +let activeResponseGate: Promise | null = null; + +function row(id: string, email: string, isMain = false) { + return { id, email, isMain, paused: false, priority: 0, hasCredential: true, quota: null }; +} + +const mainAccount: CodexAccountEntry = { + id: "main", + email: "main@example.test", + isMain: true, + paused: false, + priority: 0, + hasCredential: true, + quota: null, + quotaAutoRefresh: { + fiveHourAvailable: false, + weeklyAvailable: false, + fiveHourEnabled: false, + weeklyEnabled: false, + }, +}; + +function makeController(overrides: Partial = {}): CodexAccountPoolController { + return { + accounts: [mainAccount], + activeId: null, + loadState: "ready", + refreshing: false, + refreshFailed: false, + initialLoading: false, + switchingId: null, + pauseUpdatingId: null, + priorityUpdatingId: null, + pausingExhausted: false, + activeNeedsReauth: false, + activePinnedId: null, + load: async () => true, + switchAccount: async () => ({ ok: true, activeId: null }), + setAccountPaused: async () => ({ ok: true }), + setAccountPriority: async () => ({ ok: true }), + pauseExhaustedAccounts: async () => ({ ok: true, pausedCount: 0 }), + saveAlias: async () => ({ ok: true }), + removeAccount: async () => ({ ok: true }), + syncAfterAccountAdded: async () => ({ ok: true }), + pauseRefresh: () => ({ __brand: "codex-pool-pause" }) as never, + resumeRefresh: () => {}, + subscribeLoadObserver: () => () => {}, + readLastThreshold: () => undefined, + readLastActive: () => undefined, + ...overrides, + }; +} + +beforeEach(() => { + previous = Object.fromEntries(globals.map((k) => [k, Reflect.get(globalThis, k)])) as typeof previous; + win = new Window({ url: "http://localhost/" }); + Object.defineProperty(win.navigator, "language", { configurable: true, value: "en-US" }); + Object.defineProperties(globalThis, { + document: { configurable: true, value: win.document }, + window: { configurable: true, value: win }, + navigator: { configurable: true, value: win.navigator }, + localStorage: { configurable: true, value: win.localStorage }, + }); + (globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + + originalFetch = globalThis.fetch; + accountsOk = true; + activeResponseGate = null; + serverAccounts = [row("a1", "account-one", true)]; + Object.defineProperty(globalThis, "fetch", { + configurable: true, + value: async (url: string) => { + const path = String(url).split("/api/")[1] ?? String(url); + if (path.startsWith("usage?")) { + return { ok: true, json: async () => ({ accounts: [] }) } as unknown as Response; + } + if (path.startsWith("codex-auth/accounts")) { + if (!accountsOk) return { ok: false, status: 503 } as unknown as Response; + return { ok: true, json: async () => ({ accounts: serverAccounts }) } as unknown as Response; + } + if (path.startsWith("codex-auth/active")) { + const gate = activeResponseGate; + activeResponseGate = null; + if (gate) await gate; + return { + ok: true, + json: async () => ({ activeCodexAccountId: null, autoSwitchThreshold: 80 }), + } as unknown as Response; + } + return { ok: true, json: async () => ({}) } as unknown as Response; + }, + }); + + host = win.document.createElement("div") as unknown as HTMLElement; + win.document.body.appendChild(host as never); +}); + +afterEach(async () => { + if (root) { + const current = root; + await act(async () => { current.unmount(); }); + root = null; + } + await act(async () => { await new Promise((r) => setTimeout(r, 0)); }); + clearClientResourceStoresForTests(); + for (const key of globals) { + Object.defineProperty(globalThis, key, { configurable: true, value: previous[key] }); + } + Object.defineProperty(globalThis, "fetch", { configurable: true, value: originalFetch }); + await win.happyDOM?.close?.(); +}); + +/** A fresh apiBase each time: the controller's last-good snapshot is keyed by it. */ +async function mountController() { + baseCounter += 1; + const apiBase = `stale-${Date.now()}-${baseCounter}`; + const seen: { current: CodexAccountPoolController | null } = { current: null }; + function Probe() { + seen.current = useCodexAccountPool(apiBase, true); + return null; + } + const { createRoot } = await import("react-dom/client"); + await act(async () => { + root = createRoot(host); + root.render(); + }); + await act(async () => { await new Promise((r) => setTimeout(r, 30)); }); + return seen; +} + +async function mountPool(controller: CodexAccountPoolController) { + const { createRoot } = await import("react-dom/client"); + await act(async () => { + root = createRoot(host); + root.render( + + + , + ); + }); + await act(async () => { await new Promise((r) => setTimeout(r, 40)); }); +} + +function staleBanner(): Element | null { + return host.querySelector(".pwi-auth-state--stale"); +} + +test("a failed refresh keeps the rows and stops reporting them as current", async () => { + const seen = await mountController(); + expect(seen.current!.loadState).toBe("ready"); + expect(seen.current!.refreshFailed).toBe(false); + expect(seen.current!.accounts.map(a => a.id)).toEqual(["a1"]); + + // The server now has an account this client has never seen, and the read that would have + // brought it over fails. This is the reported shape of the defect, not a synthetic one. + serverAccounts = [row("a1", "account-one", true), row("a2", "account-two")]; + accountsOk = false; + await act(async () => { await seen.current!.load(); }); + + expect(seen.current!.refreshFailed).toBe(true); + // Still ready, and still holding the rows: blanking a populated pool on a miss is its own + // defect, so the fix is that the surface now has something to say, not that it shows less. + expect(seen.current!.loadState).toBe("ready"); + expect(seen.current!.accounts.map(a => a.id)).toEqual(["a1"]); + + accountsOk = true; + await act(async () => { await seen.current!.load(); }); + + expect(seen.current!.refreshFailed).toBe(false); + expect(seen.current!.accounts.map(a => a.id)).toEqual(["a1", "a2"]); +}); + +test("the banner clears with the rows it qualifies, not with the whole load", async () => { + // The rows are painted the moment /accounts returns, while /active can still be running on + // its own much longer budget. Clearing the flag at the settle instead would leave the rows + // that just replaced the stale ones labelled as the stale ones for that whole window. + const seen = await mountController(); + + accountsOk = false; + await act(async () => { await seen.current!.load(); }); + expect(seen.current!.refreshFailed).toBe(true); + + accountsOk = true; + serverAccounts = [row("a1", "account-one", true), row("a2", "account-two")]; + let releaseActive!: () => void; + activeResponseGate = new Promise(resolve => { releaseActive = resolve; }); + + let pending: Promise; + await act(async () => { + pending = seen.current!.load(); + await new Promise((r) => setTimeout(r, 10)); + }); + + // /accounts has landed; /active has not. + expect(seen.current!.accounts.map(a => a.id)).toEqual(["a1", "a2"]); + expect(seen.current!.refreshFailed).toBe(false); + + await act(async () => { releaseActive(); await pending!; }); + expect(seen.current!.refreshFailed).toBe(false); +}); + +test("a cold failure still replaces the surface rather than annotating an empty one", async () => { + // Non-regression: the cold path is unchanged, and this holds it there now that a second + // failure signal exists that must not take it over. + accountsOk = false; + const seen = await mountController(); + + expect(seen.current!.loadState).toBe("error"); + expect(seen.current!.accounts).toEqual([]); +}); + +test("the roster says so on screen when the rows it shows are the pre-refresh ones", async () => { + await mountPool(makeController({ refreshFailed: true })); + + const banner = staleBanner(); + expect(banner).not.toBeNull(); + expect(banner!.textContent).toContain(en["codexAuth.accountsRefreshFailed"]); + // Non-destructive: the accounts it is qualifying are still rendered underneath it. + expect(host.textContent).toContain("main@example.test"); +}); + +test("a roster whose refresh succeeded carries no banner", async () => { + // Non-regression: passes before this change too, and is here so the new banner cannot start + // appearing over a roster the server has just confirmed. + await mountPool(makeController({ refreshFailed: false })); + + expect(staleBanner()).toBeNull(); +}); + +test("a cold failure shows its own error instead of the stale banner", async () => { + // Precedence, not regression: nothing survived to qualify, so the banner would be describing + // an empty list. The cold error has to win even though both conditions hold. + await mountPool(makeController({ accounts: [], loadState: "error", refreshFailed: true })); + + expect(staleBanner()).toBeNull(); + expect(host.textContent).toContain(en["codexAuth.loadFailed"]); +}); diff --git a/gui/tests/codex-account-pool-toast-tone.test.tsx b/gui/tests/codex-account-pool-toast-tone.test.tsx index 5e64bed40a3..c730876f670 100644 --- a/gui/tests/codex-account-pool-toast-tone.test.tsx +++ b/gui/tests/codex-account-pool-toast-tone.test.tsx @@ -70,6 +70,7 @@ function makeController(overrides: Partial = {}): Co activeNeedsReauth: false, activePinnedId: null, refreshing: false, + refreshFailed: false, initialLoading: false, load: async () => true, switchAccount: async () => ({ ok: true, activeId: null }), From 4674e944af5794a4b3f8420cf6213e97b65cffdf Mon Sep 17 00:00:00 2001 From: JUN Date: Sun, 20 Sep 2026 21:12:48 +0900 Subject: [PATCH 3/3] docs(devlog): record lane R3 and correct the stale #5292 row #5292's GUI half landed in #5300 two hours before the plan was written, so the table's description of it is a snapshot, not open work. The lane note says what is already on dev and what holds it there. The two #5261 remainders are recorded with the reason each fix has the shape it does: the OAuth controller does not await onAuth, and the account roster keeps its rows on purpose. --- .../260920_round2_followups/030_lane_r3.md | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 devlog/_plan/260920_round2_followups/030_lane_r3.md diff --git a/devlog/_plan/260920_round2_followups/030_lane_r3.md b/devlog/_plan/260920_round2_followups/030_lane_r3.md new file mode 100644 index 00000000000..d38a851a40f --- /dev/null +++ b/devlog/_plan/260920_round2_followups/030_lane_r3.md @@ -0,0 +1,90 @@ +# R3 — the roster and login remainders + +Status: implemented, awaiting review. Scope was #5292 and the two #5261 remainders. + +## #5292 was already closed before this lane opened + +The plan's table says `gui/src/pages/Logs.tsx` restates the recovery-kind union with nine of +thirteen members. That was true when the table was written and stopped being true two hours +earlier: `555f0cacdf` (#5300, 18:41) replaced the copy with the durable roster, and the plan +commit landed at 20:48 from a snapshot taken before it. + +Current `dev` already has all of it. `Logs.tsx` imports `AttemptRecoveryKind` from +`src/usage/telemetry-contract.ts` and its label map closes with +`satisfies Record`, so a fourteenth kind is a typecheck failure +there rather than an "Unknown recovery reason". All ten catalogs carry all thirteen labels plus +the fallback, and `tests/usage/request-outcome-agreement.test.ts` holds both: the label map has +to cover every member of `ATTEMPT_RECOVERY_KIND_ROSTER`, and every key it names has to exist in +every catalog. Verified by reading the tree, not by rerunning the suite. + +Nothing was changed for it. The row is stale, not open. + +## #5261, remainder one: the two CLI logins that discarded the launch + +`src/oauth/login-cli.ts` called `void openUrl(...)` in both `handleOAuthLogin` and +`handleKeyLogin`. Each printed a URL, said it was opening a browser, and asked a question that +assumes it opened — indistinguishable from a login that is working. + +The part that made this more than a missing `console.warn`: `OAuthController.onAuth` returns +`void` and every one of the thirteen provider call sites invokes it as `ctrl.onAuth?.(...)` and +moves on. The launcher's answer therefore arrives after the flow has continued, and on a +callback-server provider `#waitForCallback` has already called `onManualCodeInput` by then. A +warning written at that moment lands on the line the user is typing on. + +Making `onAuth` awaitable would mean changing the controller contract and all thirteen call +sites, which is a much larger change than the defect deserves. Instead the launch reports itself +when it settles, and the two things that could collide with it wait on that report: the +manual-code prompt awaits it before asking, and the key login awaits it before it constructs a +reader at all. A polling provider that never prompts is still told before the login claims to +have worked. + +`BROWSER_LAUNCH_FAILED_HINT` in `src/cli/account-auth.ts` kept its ChatGPT-specific second line +and now derives its first from `BROWSER_LAUNCH_FAILED_NOTICE`, so the sentence has one home +across all three logins. + +The handlers took an optional deps object. The contract worth holding is an order, and an order +is only observable from something that records both events; spawning a launcher and attaching to +stdin to find that out would test the operating system. Production passes none of them. + +## #5261, remainder two: the roster that kept last-good rows silently + +`useCodexAccountPool` kept its rows after a failed read and also kept reporting `ready`. Keeping +the rows is right — blanking a populated pool because one 30s poll missed is its own defect — but +the surface then could not tell a list the server had just confirmed from one that predated a +failure. The reported shape: add an account, the read that would bring it over fails, and the +older accounts are on screen with the new one absent. + +`refreshFailed` sits beside `loadState` rather than inside it, for the same reason `refreshing` +already does. `loadState` answers what the surface can draw and a warm failure does not change +that answer; folding it in would mean either flashing the cold skeleton over good data or saying +nothing. A cold failure still replaces the surface with the error it already had, and the banner +only renders when rows survived, so an empty cold failure is never annotated instead of explained. + +## Verification + +Static review and hosted CI at the exact head. The lane ran no local suite, no individual test, +no typecheck, no build, no install, no `ocx`, and changed no credential or configuration — +recorded as NOT RUN. + +Checked by reading rather than running, because the ratchets are what a merge breaks: + +- No file this lane touches appears in `tests/fixtures/file-size-baseline.json`. The ten i18n + catalogs are in its `exempt` list. +- `tests/oauth/oauth-login-cli-browser-launch.test.ts` is registered in both + `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json`. The gui + suite has no layout guard. +- The one new i18n key is in all ten catalogs, which `gui/tests/locale-parity.test.ts` and + `gui/tests/claude-desktop-locale.test.ts` both require. +- `CodexAccountLoadState` gained no member. `CodexAccountPoolController` gained one, and the + source-oracle roster in `gui/tests/codex-account-pool-controller.test.ts` names it. +- `CodexAccountPoolLoadStates` stopped restating the load-state union and derives it. + +## The GUI screenshot gate + +`enforce-target` requires a screenshot for a PR that touches `gui`. Producing one needs +`bun run build:gui` and a running proxy, both of which this lane is forbidden to do, so the pull +request says so and offers what can be checked instead: the rendered markup is asserted against a +mounted DOM in `gui/tests/codex-account-pool-stale-refresh.test.tsx` — the banner appears with +surviving rows, carries the catalog string, does not appear on a successful refresh, and does not +replace the cold error — and the new class reuses the existing `.pwi-auth-state` block with the +`--amber` pair already used elsewhere in the theme.