diff --git a/devlog/_plan/260920_meaning_preservation_batch/030_lane_h2_lockout_dead_ends.md b/devlog/_plan/260920_meaning_preservation_batch/030_lane_h2_lockout_dead_ends.md new file mode 100644 index 00000000000..d1c6f7c1d94 --- /dev/null +++ b/devlog/_plan/260920_meaning_preservation_batch/030_lane_h2_lockout_dead_ends.md @@ -0,0 +1,90 @@ +# Lane H2 — the remaining Codex lockout dead ends (#5261) + +Status: OPEN. Base is `origin/dev` at `043aa435ff`, after lane H landed as `9880c3cad2`. +One branch, ordered commits, one pull request to `dev`. + +Lane H removed the dead end a locked-out user hits: injected routing now names its own undo, +a dead proxy is told to say so, and there is a troubleshooting page. This lane takes the four +paths INTO that state which lane H listed as not closed. + +## Why these four belong together + +None of them locks anyone out alone. Each one removes a signal, and the incident is what +happens when all of them are missing at once: a proxy that stopped, an autostart that failed +silently, a reboot that restarted nothing, a catalog pointer left naming a deleted file, and an +account flow that looked like it was working. Every individual step had an explanation; the +user had no way to reach any of them. + +## 1. The shim hid autostart failure, and could prevent Codex launching + +The wrapper ran `ocx ensure` with both streams discarded and `|| true`, then launched Codex +regardless (`src/codex/shim-templates.ts:139`). A failed start was invisible. + +Ensure's own streams stay discarded rather than being let through. It prints progress and +warnings on exit-zero runs too, so a wrapper that leaked them would put noise in front of every +ordinary launch, which is how a diagnostic gets ignored. The exit status is the signal. + +PowerShell had the opposite defect in the same place: a throwing `ensure` escaped a `try/finally` +with no `catch`, so Codex never launched at all. The autostart helper was producing the exact +lockout it exists to prevent, and the previous test asserted that propagation as correct +behaviour. It now catches, reports, and hands over. + +Not closed: the Unix revision marker moved to 3 so installed Unix shims regenerate, but Windows +shims carry no revision marker and are excluded from obsolete-shim refresh +(`src/codex/shim.ts:892`). Existing Windows wrappers keep the old text until reinstalled. Giving +Windows a refresh path is its own change. + +## 2. The catalog pointer — my lane H note was wrong + +Lane H recorded that nothing revalidates `model_catalog_json` after injection. That is not true. +The chooser refuses a missing owned path and the caller strips the stale line +(`src/codex/inject.ts:327`), with end-to-end coverage already in place. + +The real gap is narrower and worse: that repair only reaches someone who runs opencodex again, +and the difficulty of this state is that Codex is the thing that stopped working, so nothing +prompts them to. A `model_catalog_json` naming a file that is gone does not degrade Codex, it +stops Codex loading its configuration at all — the same blank wall as dead routing, from a +different cause. So this lane adds detection, not repair, and `ocx status` now names the file +and both ways out. + +## 3. Reboot: stated, not faked + +Applying the integration does not install a service, and the Windows scheduled task a separate +install would create is logon-triggered (`src/service/windows-taskxml.ts:225`). Setup ended on a +success line without ever saying routing outlives the proxy. + +A BootTrigger is deliberately NOT the fix. The task runs as the interactive user +(`LogonType: InteractiveToken`), so before logon there is no session for it to run in; the +trigger would read like a fix and change nothing. Genuine pre-logon start means a different +principal and a different service backend, which is larger than this lane and is left open +rather than half-done. What was cheap and true on every platform is saying the dependency +exists, reusing the health model `ocx status` and `ocx doctor` already report. + +## 4. Account pool: a silence, not an error + +The URL launcher swallowed its own failure and returned nothing, so the Codex login route +answered identically whether a browser opened, failed, or was skipped. The CLI printed the URL +and polled, and a user whose machine could not launch a browser watched something that looked +like it was working. + +Launch failure is now reported and never fatal — the URL is still worth opening by hand and the +flow stays live. The CLI names the fixed callback port 1455, because ChatGPT supplies the +redirect URI and the flow cannot move to a free port, so `--device` is the way around it. + +Not closed: the dashboard keeps last-good rows after a failed refresh +(`gui/src/hooks/useCodexAccountPool.ts:325`), which is why a newly added account can be absent +while older rows still show. That is a GUI change, and the pull-request gate requires a +screenshot of a UI change, which cannot be produced under this lane's no-build constraint. It +is left for a lane that can build the GUI. + +## Verification + +Static source review and exact-head hosted CI only. NOT RUN, by lane constraint: local suite, +individual tests, typecheck, build, install, `ocx` execution, service start or restart, and any +change to credentials or configuration on the machine. The incident being fixed is a +configuration change that locked a user out. + +Checked before push: the file-size ratchet (no tracked file over cap; `codex-shim.test.ts` +sits close to its cap, so the new shim tests went to a sibling file registered in both +layout maps), and the restated-constant class that broke two lanes in this batch — the shim +test's private literal copies of the marker constants now come from the source module. diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 74b27f8859c..860db7a1879 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -560,6 +560,7 @@ "codex-service-manager-probe-hardening.test.ts": "codex-integration", "codex-service-manager-probe.test.ts": "codex-integration", "codex-shim-autorestore.test.ts": "codex-integration", + "codex-shim-ensure-failure.test.ts": "codex-integration", "codex-shim-readiness.test.ts": "codex-integration", "codex-shim.test.ts": "codex-integration", "codex-signin-lockout.test.ts": "codex-integration", @@ -1018,6 +1019,7 @@ "oauth-login-summary.test.ts": "oauth", "oauth-manual-code.test.ts": "oauth", "oauth-open-browser-choice.test.ts": "oauth", + "oauth-open-url-result.test.ts": "oauth", "oauth-provider-reconcile.test.ts": "oauth", "oauth-public-surface.test.ts": "oauth", "oauth-reauth-bind.test.ts": "oauth", diff --git a/src/cli/account-auth.ts b/src/cli/account-auth.ts index 73838f9da8e..de6364eb05f 100644 --- a/src/cli/account-auth.ts +++ b/src/cli/account-auth.ts @@ -69,8 +69,22 @@ interface LoginStart { flowId?: string; instructions?: string; deviceCode?: string; + /** Whether the host actually opened a browser. Absent from older proxies. */ + browserLaunch?: "started" | "failed" | "skipped"; } +/** + * Said only when the host could not open a browser (#5261). + * + * Without it, a failed launch is indistinguishable from a successful one: the URL is printed + * 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. + */ +export const BROWSER_LAUNCH_FAILED_HINT = + "⚠️ No browser could be opened here — open the URL above yourself." + + "\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. */ const STDIN_SENTINEL = "-"; @@ -145,6 +159,7 @@ async function login(argv: string[], deps: RuntimeApiDeps): Promise { start.url ? `Open this URL to sign in:\n${start.url}` : "", start.deviceCode ? `Device code: ${start.deviceCode}` : "", start.instructions ?? "", + start.browserLaunch === "failed" ? BROWSER_LAUNCH_FAILED_HINT : "", start.flowId ? `Flow: ${start.flowId}` : "", ].filter(line => line !== "").join("\n"); if (block) writeStdoutFully(`${block}\n`); diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index 029336f66c9..b9b09420c87 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -615,7 +615,11 @@ const commandRunners: Record = { const guiUrl = selectDefaultGuiUrl(config, live, deps.probeHostname); console.log(`Opening ${guiUrl}`); const { openUrl } = await import("../lib/open-url"); - openUrl(guiUrl); + // Awaited so a launcher that never opened anything is said out loud (#5261). Still exit + // 0: the proxy is serving and the URL above is reachable, only the launch did not happen. + if ((await openUrl(guiUrl)).status === "failed") { + console.error("⚠️ No browser could be opened here; open the URL above yourself."); + } return 0; }, }); diff --git a/src/cli/index.ts b/src/cli/index.ts index e689bd26f36..c34ad2cf9b2 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -63,7 +63,7 @@ import { pendingTeardownsAreExactly, quarantinePendingTeardown, } from "../config/pending-teardown"; -import { collectStatus, deadProxyRoutingAdviceLines, hubStatusLines, remoteHubBannerLine, remoteHubStatusLines, unusedProxyWarningLines } from "./status"; +import { collectStatus, deadProxyRoutingAdviceLines, detectMissingCodexCatalogPath, hubStatusLines, missingCodexCatalogLines, remoteHubBannerLine, remoteHubStatusLines, unusedProxyWarningLines } from "./status"; import { endpointsToProve, everyEndpointProvenDown, sharedTeardownAuthorized, type UninstallObservation } from "./uninstall-plan"; import { takeFlag } from "./runtime-api"; import { parseStartOptions, StartArgsError } from "./start-args"; @@ -1614,6 +1614,12 @@ async function handleStatus() { console.log(` Codex autostart: ${status.json.codexAutostart ? "enabled" : "disabled"}${local}`); console.log(` Restart safety: ${startupHealthSummary(status.json.startup)}${local}`); console.log(` ${formatStartupRoutingDetail(status.json.startup)}${local}`); + // Independent of whether the proxy is up: a catalog pointer whose file is gone stops Codex + // loading its config at all, and presents as the same blank wall as dead routing (#5261). + // Tagged `(local)` on its header like every other local-state line, so a connected client + // cannot read a finding about its own Codex home as something the hub reported. + missingCodexCatalogLines(detectMissingCodexCatalogPath()) + .forEach((line, index) => console.log(` ${line}${index === 0 ? local : ""}`)); if (status.json.startup.routingKind === "native") { let retainedProviderTable = false; try { diff --git a/src/cli/init.ts b/src/cli/init.ts index f310b7f7966..e53b203415a 100644 --- a/src/cli/init.ts +++ b/src/cli/init.ts @@ -253,6 +253,14 @@ export async function runInit(): Promise { } console.log(`\n🚀 Setup complete! Run 'ocx start' to start the proxy.`); + // Said after the autostart choice, because the choice is what decides whether it applies. + // Setup otherwise ends on a success line while leaving a restart dependency unmentioned. + try { + const { collectStartupHealth, injectedRoutingRestartWarningLines } = await import("../codex/autostart-health"); + for (const line of injectedRoutingRestartWarningLines(collectStartupHealth(config))) console.log(line); + } catch { + // A diagnostic that cannot be computed must not fail a completed setup. + } for (const line of modelSelectionGuidance(providerName)) console.log(line); } catch (error) { if (error instanceof InitCancelledError) { diff --git a/src/cli/status.ts b/src/cli/status.ts index 22d1e25871c..9079a2b5f90 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -1,4 +1,5 @@ import { durableBunRuntime } from "../lib/bun-runtime"; +import { existsSync, readFileSync } from "node:fs"; import { codexAutoStartEnabled, getConfigPath, readConfigDiagnostics } from "../config"; import { getPidPath, readPid, readRuntimePort, type RuntimePortState } from "../config/process-state"; import { diagnoseCodexBundledPlugins, type CodexPluginsDiagnostic } from "../codex/plugins-doctor"; @@ -7,6 +8,8 @@ import type { OcxConfig } from "../types"; import { diagnoseService, serviceLogPath } from "../service"; import { collectStartupHealth, type StartupHealth } from "../codex/autostart-health"; import { getCodexRoutingKind } from "../codex/inject"; +import { missingOwnedCatalogPath } from "../codex/inject/config-toml"; +import { CODEX_CONFIG_PATH } from "../codex/paths"; import { diagnoseCodexShim } from "../codex/shim"; import { displayCodexRuntimePath, effortClampAppliesToRuntime, liveRemovedEfforts, loadLastEffortClamp, resolveCodexRuntime } from "../codex/runtime"; import { packageVersion } from "./help"; @@ -529,6 +532,38 @@ export function deadProxyRoutingAdviceLines(input: { ]; } +/** + * Read the live Codex config and report an opencodex catalog pointer whose file is gone. + * + * Unreadable or absent config is reported as no finding rather than as a problem: this is a + * diagnostic line, and inventing one from missing evidence is worse than staying quiet. + */ +export function detectMissingCodexCatalogPath(): string | null { + try { + if (!existsSync(CODEX_CONFIG_PATH)) return null; + return missingOwnedCatalogPath(readFileSync(CODEX_CONFIG_PATH, "utf8")); + } catch { + return null; + } +} + +/** + * The one state in this report where Codex is broken independently of the proxy (#5261). + * + * A `model_catalog_json` naming a file that is gone stops Codex loading its configuration at + * all, so it presents as the same blank wall as dead routing while having a different cause and + * a different fix. Both are named, because restarting the proxy rewrites the catalog and + * restoring removes the pointer, and which one the user wants is their choice, not ours. + */ +export function missingCodexCatalogLines(missingCatalogPath: string | null): string[] { + if (!missingCatalogPath) return []; + return [ + "⚠️ Codex is pointed at a model catalog that is no longer on disk, so Codex cannot load its config:", + ` ${missingCatalogPath}`, + " Regenerate it with 'ocx start', or remove opencodex from Codex with 'ocx restore'.", + ]; +} + export async function collectStatus(): Promise { const configDiagnostics = readConfigDiagnostics(); const config = configDiagnostics.config; diff --git a/src/codex/auth-api/login-flow.ts b/src/codex/auth-api/login-flow.ts index fa6d1c6fbf7..3a9111e2a4d 100644 --- a/src/codex/auth-api/login-flow.ts +++ b/src/codex/auth-api/login-flow.ts @@ -226,9 +226,12 @@ export async function handleCodexAuthLoginStart(req: Request, config: OcxConfig, // machine. Opening it on the hub host is useless at best, and on a // headless host it fails. `deviceCode` is the same signal the generic // OAuth login route uses to make this decision. + // Reported to the caller rather than discarded (#5261): a login whose browser never opened + // is indistinguishable from one that did, so it reads as success while nothing happens. + let browserLaunch: "started" | "failed" | "skipped" = "skipped"; if (result.url && !result.deviceCode && shouldOpenBrowserForLogin(body.openBrowser, runtimeConfig)) { const { openUrl } = await import("../../lib/open-url"); - openUrl(result.url); + browserLaunch = (await openUrl(result.url)).status === "started" ? "started" : "failed"; } (async () => { @@ -488,6 +491,8 @@ export async function handleCodexAuthLoginStart(req: Request, config: OcxConfig, flowId, url: result.url, instructions: result.instructions, + // Never fatal: the URL is still a valid thing to open by hand, and the flow stays live. + browserLaunch, // Dropped before #3366: every device-code surface renders this field, // so withholding it left the GUI and CLI with no code to show. ...(result.deviceCode ? { deviceCode: result.deviceCode } : {}), diff --git a/src/codex/autostart-health.ts b/src/codex/autostart-health.ts index 5fc990a5f6c..83b330ca3db 100644 --- a/src/codex/autostart-health.ts +++ b/src/codex/autostart-health.ts @@ -154,6 +154,34 @@ export function startupHealthSummary(health: StartupHealth): string { return action ? `${summary}; ${action}` : summary; } +/** + * What to say at the end of setup, once routing is on disk and the autostart choice is made. + * + * #5261: applying the Codex integration does not install a background service, and on Windows + * the scheduled task that a separate install would create is logon-triggered rather than + * boot-triggered. So "routing written, nothing listening" is an ordinary state after a restart + * rather than a corruption — and the user is never told, because setup ends on a success line. + * + * A boot trigger is not the missing piece and would be a false reassurance. The task runs as + * the interactive user, so before logon there is no session for it to run in; making it truly + * pre-logon means a different principal and a different service backend, not another trigger. + * What is actually missing is that nobody says the dependency exists, which is cheap to fix and + * true on every platform. + * + * Reuses the existing health model rather than re-deriving the condition, so this cannot drift + * from what `ocx status` and `ocx doctor` report about the same install. + */ +export function injectedRoutingRestartWarningLines(health: StartupHealth): string[] { + if (health.status !== "at-risk") return []; + return [ + // Deliberately not "nothing will restart the proxy": a healthy launcher shim does restart it, + // for CLI launches only, and is still at-risk. The summary line below says which case this is. + "⚠️ Codex routing is written to disk and survives a restart; keeping the proxy running is a separate matter.", + ` ${startupHealthSummary(health)}`, + " While the proxy is down Codex cannot sign in or reach a model. 'ocx restore' undoes the routing without needing it.", + ]; +} + function classifyStartupHealthSummary(health: StartupHealth): string { if (health.status === "native") return health.routingKind === "custom-remote" ? "custom remote Codex routing (no local restart dependency)" diff --git a/src/codex/inject/config-toml.ts b/src/codex/inject/config-toml.ts index 7577e2c6813..1e552dbbef3 100644 --- a/src/codex/inject/config-toml.ts +++ b/src/codex/inject/config-toml.ts @@ -609,3 +609,32 @@ export function chooseCatalogPathForInjection( return existsSync(DEFAULT_CATALOG_PATH) ? DEFAULT_CATALOG_PATH : null; } + +/** + * The effective `model_catalog_json` is one of ours and the file is gone. + * + * Codex does not degrade on this: a catalog path it cannot read stops it loading its + * configuration at all, which looks exactly like the routing lockout in #5261 and is what the + * reporter's machine was left in after the catalog file was deleted by hand. + * + * Injection already repairs it — the chooser refuses a missing owned path and the caller strips + * the stale line. That only helps someone who runs opencodex again, and the whole difficulty of + * this state is that Codex is the thing that stopped working, so nothing prompts them to. This + * predicate exists so the CLI can say it out loud. + * + * A user-owned catalog assignment wins here exactly as it does during injection: if they named + * the file, its absence is theirs to explain, and we do not claim it. + * + * Ownership is decided by basename, which is a weak test — a file the user happens to name + * `opencodex-catalog.json` is read as ours wherever it sits. That is deliberate rather than + * overlooked: it is the same test injection already applies, and a detector that drew the line + * somewhere else would report a state injection would then treat differently. Tightening it is a + * change to injection, not to this. + */ +export function missingOwnedCatalogPath(content: string): string | null { + const existing = readRootModelCatalogPath(content); + if (!existing) return null; + const resolved = resolveCodexConfigPath(existing); + if (!isOpencodexCatalogPath(resolved)) return null; + return existsSync(resolved) ? null : existing; +} diff --git a/src/codex/shim-templates.ts b/src/codex/shim-templates.ts index 5ed2cb6b4a1..0f5af00d022 100644 --- a/src/codex/shim-templates.ts +++ b/src/codex/shim-templates.ts @@ -4,11 +4,29 @@ import { serviceApiTokenFilePath } from "../lib/service-secrets"; import { windowsEnvIndirectBatchValue } from "../lib/win-paths"; const SHIM_MARKER = "opencodex codex autostart shim"; -const UNIX_SHIM_REVISION_MARKER = "opencodex unix codex shim revision 2"; +const UNIX_SHIM_REVISION_MARKER = "opencodex unix codex shim revision 3"; const CODEX_SHIM_REENTRY_EXIT_CODE = 126; const CODEX_SHIM_REENTRY_DIAGNOSTIC = "opencodex: saved Codex launcher resolved back to the autostart shim; run ocx codex-shim uninstall and reinstall Codex before enabling codexAutoStart."; +/** + * Said once, on stderr, when `ocx ensure` could not bring the proxy up (#5261). + * + * The shim used to discard both of ensure's streams and ignore its exit status, so a failed + * autostart was completely silent: Codex launched against injected routing pointing at a port + * nothing was listening on, and every request — sign-in included — failed with no mention of + * opencodex anywhere. + * + * Ensure's own streams stay discarded rather than being let through. Ensure prints progress and + * warnings on exit-zero runs too, and a wrapper that leaked those would put noise in front of + * every ordinary Codex launch, which is how a diagnostic gets ignored. The exit status is the + * signal; this line is the whole message. + * + * It names `ocx restore` because bringing the proxy back is only half the choice. A user who + * cannot sign in needs the way out that does not require the proxy at all. + */ +export const CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC = "opencodex: proxy autostart failed; launching Codex anyway. Run 'ocx doctor' for details, or 'ocx restore' to hand Codex back to its own account."; + const CODEX_INTERNAL_COMMANDS = [ "app-server", "archive", @@ -136,7 +154,9 @@ case "$ocx_subcommand" in ;; *) if [ -z "$OCX_SHIM_BYPASS" ]; then - ${BUN_RUNTIME_SOURCE_ENV}=${shQuote(bunRuntimeSource)} ${BUN_RUNTIME_PATH_ENV}=${shQuote(bunPath)} ${shQuote(bunPath)} ${shQuote(cliPath)} ensure >/dev/null 2>&1 || true + if ! ${BUN_RUNTIME_SOURCE_ENV}=${shQuote(bunRuntimeSource)} ${BUN_RUNTIME_PATH_ENV}=${shQuote(bunPath)} ${shQuote(bunPath)} ${shQuote(cliPath)} ensure >/dev/null 2>&1; then + printf '%s\\n' ${shQuote(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC)} >&2 + fi fi ;; esac @@ -197,6 +217,7 @@ setlocal\r ${windowsBatchSet(BUN_RUNTIME_SOURCE_ENV, bunRuntimeSource)}\r ${windowsBatchSet(BUN_RUNTIME_PATH_ENV, bunPath)}\r "%OCX_BUN%" "%OCX_CLI%" ensure >nul 2>nul\r +if errorlevel 1 echo ${CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC} 1>&2\r endlocal\r :run_codex\r "%OCX_REAL_CODEX%" %*\r @@ -239,13 +260,18 @@ if (-not $skipEnsure) { $priorRuntimePath = $env:${BUN_RUNTIME_PATH_ENV} $env:${BUN_RUNTIME_SOURCE_ENV} = ${psString(bunRuntimeSource)} $env:${BUN_RUNTIME_PATH_ENV} = ${psString(bunPath)} - try { & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null } + $ocxEnsureFailed = $false + # Caught, not propagated: a throwing ensure used to escape this wrapper and Codex never + # launched at all, which is a lockout produced by the autostart helper itself (#5261). + try { & ${psString(bunPath)} ${psString(cliPath)} ensure *> $null; if ($LASTEXITCODE -ne 0) { $ocxEnsureFailed = $true } } + catch { $ocxEnsureFailed = $true } finally { if ($null -eq $priorRuntimeSource) { Remove-Item Env:\\${BUN_RUNTIME_SOURCE_ENV} -ErrorAction SilentlyContinue } else { $env:${BUN_RUNTIME_SOURCE_ENV} = $priorRuntimeSource } if ($null -eq $priorRuntimePath) { Remove-Item Env:\\${BUN_RUNTIME_PATH_ENV} -ErrorAction SilentlyContinue } else { $env:${BUN_RUNTIME_PATH_ENV} = $priorRuntimePath } } + if ($ocxEnsureFailed) { [Console]::Error.WriteLine(${psString(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC)}) } } & ${psString(realCodexPath)} @args $codexExitCode = $LASTEXITCODE diff --git a/src/lib/open-url.ts b/src/lib/open-url.ts index ae6cb900362..a13287ccf6e 100644 --- a/src/lib/open-url.ts +++ b/src/lib/open-url.ts @@ -8,8 +8,33 @@ function windowsRundll32(): string { return existsSync(candidate) ? candidate : "rundll32"; } -export function openUrl(url: string): void { - if (!/^https?:\/\//i.test(url)) return; +/** + * Whether the OS launcher actually started. `started` does not prove a browser rendered the + * page — nothing observable from here can — but it does separate "we handed the URL off" from + * "there was nothing to hand it to", which is the distinction a caller needs (#5261). + */ +export type OpenUrlResult = + | { status: "started" } + | { status: "failed"; reason: "invalid-url" | "spawn-error" | "launcher-exit" }; + +/** + * How long to watch a launcher that did spawn before calling it started. + * + * `spawn` only proves a process began. `xdg-open` with no desktop handler, and rundll32 given a + * broken association, both spawn happily and exit nonzero a moment later without opening + * anything — so resolving on `spawn` alone would report a launch that did not happen. Those + * failures are immediate, and the only caller that awaits this is a login start, so a short + * window buys a true answer cheaply. A launcher still running when it elapses has started. + */ +const LAUNCHER_SETTLE_MS = 400; + +/** + * Never rejects. A browser that would not open is an inconvenience, not a login failure: the + * URL is still a valid thing to open by hand, so the caller decides what to say about it. + * Callers that genuinely do not care use `void openUrl(...)`. + */ +export function openUrl(url: string): Promise { + if (!/^https?:\/\//i.test(url)) return Promise.resolve({ status: "failed", reason: "invalid-url" }); const cmd = process.platform === "darwin" ? "open" : process.platform === "win32" ? windowsRundll32() @@ -17,9 +42,28 @@ export function openUrl(url: string): void { const args = process.platform === "win32" ? ["url.dll,FileProtocolHandler", url] : [url]; - const child = spawn(cmd, args, { detached: true, stdio: "ignore", shell: false }); - // Headless hosts (no xdg-open) emit ENOENT as an async 'error' event; without a - // listener that is an uncaught exception that kills the whole proxy/login flow. - child.on("error", () => {}); - child.unref(); + return new Promise(resolve => { + let settled = false; + let timer: ReturnType | undefined; + const settle = (result: OpenUrlResult) => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + resolve(result); + }; + const child = spawn(cmd, args, { detached: true, stdio: "ignore", shell: false }); + // Headless hosts (no xdg-open) emit ENOENT as an async 'error' event; without a + // listener that is an uncaught exception that kills the whole proxy/login flow. + // It is also the signal itself: on Windows this is how a missing rundll32 arrives. + child.on("error", () => settle({ status: "failed", reason: "spawn-error" })); + child.on("spawn", () => { + // Unref'd either way: this never keeps the process alive, it only decides what to report. + timer = setTimeout(() => settle({ status: "started" }), LAUNCHER_SETTLE_MS); + timer.unref?.(); + }); + child.on("exit", code => settle(code === 0 || code === null + ? { status: "started" } + : { status: "failed", reason: "launcher-exit" })); + child.unref(); + }); } diff --git a/src/oauth/login-cli.ts b/src/oauth/login-cli.ts index 45db1af0ecf..19ad8f833cd 100644 --- a/src/oauth/login-cli.ts +++ b/src/oauth/login-cli.ts @@ -106,7 +106,7 @@ async function handleOAuthLogin(name: string): Promise { onAuth: ({ url, instructions }) => { console.log(`\n🔐 Opening browser for ${name} login...\n${url}\n`); if (instructions) console.log(instructions); - openUrl(url); + void openUrl(url); }, onProgress: (m) => console.log(` ${m}`), onManualCodeInput: () => @@ -201,7 +201,7 @@ 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...`); - openUrl(def.dashboardUrl); + 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. diff --git a/src/server/management/oauth-account-routes.ts b/src/server/management/oauth-account-routes.ts index 0cb29ff8acd..0c03a96d3da 100644 --- a/src/server/management/oauth-account-routes.ts +++ b/src/server/management/oauth-account-routes.ts @@ -210,7 +210,7 @@ export async function handleOauthAccountRoutes(ctx: ManagementContext): Promise< const { shouldOpenBrowserForLogin } = await import("../../oauth/open-browser-choice"); if (authUrl && !deviceCode && shouldOpenBrowserForLogin(body.openBrowser, config)) { const { openUrl } = await import("../../lib/open-url"); - openUrl(authUrl); + void openUrl(authUrl); } return jsonResponse({ url: authUrl, instructions, deviceCode }); } catch (err) { diff --git a/tests/ci-workflows/cold-spawn-warmup.test.ts b/tests/ci-workflows/cold-spawn-warmup.test.ts index 7e969d8cf2a..29e86fdc944 100644 --- a/tests/ci-workflows/cold-spawn-warmup.test.ts +++ b/tests/ci-workflows/cold-spawn-warmup.test.ts @@ -78,6 +78,14 @@ const DISPOSITIONS: Readonly> = { warmed: true, why: "the first resetAt iteration loads src/codex/quota.ts and src/codex/main-account-cache.ts", }, + "tests/codex-integration/codex-shim-ensure-failure.test.ts": { + warmed: false, + why: + "Its children are two throwaway /bin/sh scripts standing in for ensure and for the real Codex " + + "launcher, so the cold cost is shell and process startup rather than a repository module " + + "graph, and an import scan has nothing to warm. The generated shim never loads a repository " + + "module in the child: the point of the file is what the shell does with an exit status.", + }, "tests/codex-integration/codex-shim.test.ts": { warmed: false, why: diff --git a/tests/codex-integration/codex-auth-api.test.ts b/tests/codex-integration/codex-auth-api.test.ts index ab750aa06d0..c17088cb15e 100644 --- a/tests/codex-integration/codex-auth-api.test.ts +++ b/tests/codex-integration/codex-auth-api.test.ts @@ -170,7 +170,7 @@ async function completeMockCodexOAuth(options: { done: true, loggedIn: true, } as ReturnType); - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(() => {}); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async () => ({ status: "started" as const })); // Mirrors the login-status poll delay in login-flow.ts; other timers are intentionally dropped. const CODEX_OAUTH_LOGIN_POLL_INTERVAL_MS = 2_000; const timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation((( @@ -4593,7 +4593,7 @@ describe("codex-auth API", () => { instructions: "Enter code: ABCD-EFGH", deviceCode: "ABCD-EFGH", })); - const openSpy = spyOn(openUrlModule, "openUrl").mockImplementation(() => {}); + const openSpy = spyOn(openUrlModule, "openUrl").mockImplementation(async () => ({ status: "started" as const })); try { const req = new Request("http://localhost/api/codex-auth/login", { method: "POST", @@ -4699,7 +4699,7 @@ describe("codex-auth API", () => { controller.onAuth({ url: "https://example.test/oauth" }); throw new Error("late failure at /home/alice/.opencodex/auth.json.ocx-tmp sk-secret-provider-key"); }; - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(() => {}); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async () => ({ status: "started" as const })); const timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation((( callback: (...args: unknown[]) => void, delay?: number, @@ -4747,7 +4747,7 @@ describe("codex-auth API", () => { const { OAuthMutationBusyError } = await import("../../src/oauth/store"); const openUrlMod = await import("../../src/lib/open-url"); const originalLogin = oauth.OAUTH_PROVIDERS.chatgpt.login; - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(() => {}); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async () => ({ status: "started" as const })); const timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation((( callback: (...args: unknown[]) => void, delay?: number, @@ -4834,7 +4834,7 @@ describe("codex-auth API", () => { done: false, loggedIn: false, } as ReturnType); - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(() => {}); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async () => ({ status: "started" as const })); const req = new Request("http://localhost/api/codex-auth/login", { method: "POST", headers: { "Content-Type": "application/json" }, @@ -5161,7 +5161,7 @@ describe("codex-auth API", () => { loggedIn: false, error: "test-stop", } as ReturnType); - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(() => {}); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async () => ({ status: "started" as const })); try { const req = new Request("http://localhost/api/codex-auth/login", { diff --git a/tests/codex-integration/codex-shim-ensure-failure.test.ts b/tests/codex-integration/codex-shim-ensure-failure.test.ts new file mode 100644 index 00000000000..ab652d64305 --- /dev/null +++ b/tests/codex-integration/codex-shim-ensure-failure.test.ts @@ -0,0 +1,117 @@ +/** + * #5261: the Codex autostart shim must never hide a failed autostart, and must never be the + * reason Codex does not launch. + * + * It used to do both. Both of `ocx ensure`'s streams were discarded and its exit status ignored + * with `|| true`, so a proxy that failed to come up was completely silent while Codex launched + * against injected routing pointing at a dead port. These run the real generated script against + * a stand-in `ensure` rather than asserting on template text, because the property under test is + * what the shell does with the exit status, not what the file says. + * + * Lives beside `codex-shim.test.ts` rather than inside it: that file sits close to its line cap, + * and the cap only moves down. + */ +import { describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { chmodSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { buildUnixCodexShim } from "../../src/codex/shim"; +import { CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC } from "../../src/codex/shim-templates"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { INTERNAL_DEADLINE_MS, SPAWN_BUDGET_MS } from "../helpers/test-budget"; + +/** The shim refuses to re-enter itself, so a nested test run must not inherit its guard state. */ +function cleanShimEnv(): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env, OCX_SHIM_BYPASS: "" }; + delete env.OCX_SHIM_ACTIVE_PID; + delete env.OCX_SHIM_ACTIVE_DEPTH; + delete env.OCX_SHIM_PROBE_ACTIVE; + return env; +} + +/** + * A shim whose `ensure` is `script`. The real Codex prints a marker and exits 7, so every case + * can tell whether Codex ran and whether its exit status survived the wrapper. + */ +function shimWithEnsure(dir: string, script: string): string { + const realCodex = join(dir, "codex-real"); + writeFileSync(realCodex, "#!/bin/sh\necho codex-ran\nexit 7\n"); + chmodSync(realCodex, 0o755); + const fakeBun = join(dir, "fake-bun"); + writeFileSync(fakeBun, script); + chmodSync(fakeBun, 0o755); + const wrapper = join(dir, "codex"); + writeFileSync(wrapper, buildUnixCodexShim(realCodex, fakeBun, join(dir, "cli.ts"), "process", join(dir, "absent-token"))); + chmodSync(wrapper, 0o755); + return wrapper; +} + +function runShim(wrapper: string) { + return spawnSync("/bin/sh", [wrapper, "exec", "hello"], { + encoding: "utf8", + env: cleanShimEnv(), + timeout: INTERNAL_DEADLINE_MS, + }); +} + +describe.skipIf(process.platform === "win32")("Codex shim autostart failure (#5261)", () => { + test("a failed ensure is reported once on stderr and Codex still launches", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-shim-ensure-fail-")); + try { + // Writes to both streams so the assertions below also prove ensure's own chatter + // stays discarded: leaking it would put noise in front of every Codex launch. + const wrapper = shimWithEnsure(dir, "#!/bin/sh\necho ensure-stdout\necho ensure-stderr >&2\nexit 1\n"); + const run = runShim(wrapper); + + expect(run.error).toBeUndefined(); + expect(run.stdout).toContain("codex-ran"); + expect(run.status).toBe(7); + expect(run.stderr).toContain(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC); + expect(run.stderr).not.toContain("ensure-stderr"); + expect(run.stdout).not.toContain("ensure-stdout"); + + // Once, not once per stream or per retry. + const occurrences = run.stderr.split(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC).length - 1; + expect(occurrences).toBe(1); + } finally { + removeTreeWithRetry(dir); + } + }, SPAWN_BUDGET_MS); + + test("a successful ensure says nothing at all", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-shim-ensure-ok-")); + try { + const wrapper = shimWithEnsure(dir, "#!/bin/sh\necho ensure-stdout\nexit 0\n"); + const run = runShim(wrapper); + + expect(run.stdout).toContain("codex-ran"); + expect(run.status).toBe(7); + expect(run.stderr).not.toContain(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC); + expect(run.stdout).not.toContain("ensure-stdout"); + } finally { + removeTreeWithRetry(dir); + } + }, SPAWN_BUDGET_MS); + + test("an ensure that cannot be executed at all is still not a lockout", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-shim-ensure-missing-")); + try { + // The launcher path does not exist. This is the headless/broken-install shape, and the + // wrapper must treat it exactly like a failed start rather than dying with it. + const realCodex = join(dir, "codex-real"); + writeFileSync(realCodex, "#!/bin/sh\necho codex-ran\nexit 7\n"); + chmodSync(realCodex, 0o755); + const wrapper = join(dir, "codex"); + writeFileSync(wrapper, buildUnixCodexShim(realCodex, join(dir, "no-such-bun"), join(dir, "cli.ts"), "process", join(dir, "absent-token"))); + chmodSync(wrapper, 0o755); + + const run = runShim(wrapper); + expect(run.stdout).toContain("codex-ran"); + expect(run.status).toBe(7); + expect(run.stderr).toContain(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC); + } finally { + removeTreeWithRetry(dir); + } + }, SPAWN_BUDGET_MS); +}); diff --git a/tests/codex-integration/codex-shim.test.ts b/tests/codex-integration/codex-shim.test.ts index f684fa8de91..efc971030b4 100644 --- a/tests/codex-integration/codex-shim.test.ts +++ b/tests/codex-integration/codex-shim.test.ts @@ -8,9 +8,8 @@ import { prependPath, withInstalledShim } from "../helpers/codex-shim-install-fi import { removeTreeWithRetry } from "../helpers/remove-tree"; import { repoPath, repoRoot } from "../helpers/repo-root"; import { INTERNAL_DEADLINE_MS, SPAWN_BUDGET_MS } from "../helpers/test-budget"; +import { CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC, SHIM_MARKER, UNIX_SHIM_REVISION_MARKER } from "../../src/codex/shim-templates"; -const SHIM_MARKER = "opencodex codex autostart shim"; -const UNIX_SHIM_REVISION_MARKER = "opencodex unix codex shim revision 2"; /** * A child environment with the shim's recursion-guard state stripped. @@ -1321,7 +1320,10 @@ printf '%s\\n' child-codex const driverPath = join(dir, "driver.ps1"); const realPath = join(dir, "codex-real.ps1"); writeFileSync(join(dir, "service-api-token"), "file-token\n"); - writeFileSync(ensurePath, failurePhase === "ensure" ? "throw 'fixture ensure failure'\n" : "exit 19\n"); + // The Codex phase must isolate a Codex failure, so its ensure has to SUCCEED. It used + // to exit 19, which the wrapper now correctly reports as a failed autostart (#5261), + // making both phases indistinguishable. + writeFileSync(ensurePath, failurePhase === "ensure" ? "throw 'fixture ensure failure'\n" : "exit 0\n"); writeFileSync(realPath, "throw 'fixture Codex failure'\n"); writeFileSync(wrapperPath, `\uFEFF${buildWindowsPowerShellCodexShim(realPath, ensurePath, "unused.ts", "process")}`); const emptyToken = callerToken === "" ? "$env:OPENCODEX_API_AUTH_TOKEN = ''\n" : ""; @@ -1333,9 +1335,14 @@ printf '%s\\n' child-codex }); expect(result.error).toBeUndefined(); expect(result.status, result.stderr).toBe(0); + // An ensure failure no longer stops Codex from launching (#5261). The wrapper + // reports it on stderr and hands over to the real launcher, so the error that + // reaches the caller is always Codex's own, in both phases. + const surfaced = "fixture Codex failure"; expect(result.stdout.trim().split(/\r?\n/)).toEqual([ - `error:fixture ${failurePhase} failure`, `after:${callerToken ?? ""}`, "presence-preserved:True", + `error:${surfaced}`, `after:${callerToken ?? ""}`, "presence-preserved:True", ]); + expect(result.stderr.includes(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC)).toBe(failurePhase === "ensure"); // A failed process must complete, rather than satisfy the check through a timeout. writeFileSync(driverPath, `\uFEFF$ErrorActionPreference = 'Stop'\n& '${wrapperPath.replace(/'/g, "''")}' exec\n`); @@ -1346,7 +1353,8 @@ printf '%s\\n' child-codex expect(uncaught.signal).toBeNull(); expect(typeof uncaught.status, uncaught.stderr).toBe("number"); expect(uncaught.status, uncaught.stderr).not.toBe(0); - expect(uncaught.stderr).toContain(`fixture ${failurePhase} failure`); + expect(uncaught.stderr).toContain(surfaced); + expect(uncaught.stderr.includes(CODEX_SHIM_ENSURE_FAILED_DIAGNOSTIC)).toBe(failurePhase === "ensure"); } finally { if (oldHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = oldHome; diff --git a/tests/codex-integration/codex-signin-lockout.test.ts b/tests/codex-integration/codex-signin-lockout.test.ts index 14452235647..a11f6ec5789 100644 --- a/tests/codex-integration/codex-signin-lockout.test.ts +++ b/tests/codex-integration/codex-signin-lockout.test.ts @@ -11,16 +11,20 @@ * test that needed one running would be testing the wrong state. */ import { describe, expect, test } from "bun:test"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { OCX_ROUTING_MARKER_LINE, OCX_SECTION_MARKER, hasInjectedCodexRouting, hasInjectedOpenaiBaseUrl, } from "../../src/codex/injected-marker"; -import { setRootOpenaiBaseUrl, setRootRealtimeWsBaseUrl } from "../../src/codex/inject/config-toml"; +import { missingOwnedCatalogPath, setRootOpenaiBaseUrl, setRootRealtimeWsBaseUrl } from "../../src/codex/inject/config-toml"; import { stripOpencodexConfig } from "../../src/codex/inject/remove"; -import { deadProxyRoutingAdviceLines } from "../../src/cli/status"; +import { deadProxyRoutingAdviceLines, missingCodexCatalogLines } from "../../src/cli/status"; import { findCommand } from "../../src/cli/registry"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; /** A Windows catalog path as TOML stores it: a basic string doubles the separators (#1798). */ const WINDOWS_CATALOG = JSON.stringify(String.raw`C:\Users\example\.codex\opencodex-catalog.json`); @@ -129,3 +133,53 @@ describe("Codex sign-in lockout behind a stopped proxy (#5261)", () => { .toContain(`ocx ${named}`); }); }); + +describe("a Codex catalog pointer whose file is gone (#5261)", () => { + /** Builds a root config naming `catalogPath`, in the escaped form TOML actually stores. */ + const configNaming = (catalogPath: string) => + `model = "gpt-5.5"\nmodel_catalog_json = ${JSON.stringify(catalogPath)}\n`; + + test("an owned catalog that is missing is reported, and one that exists is not", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-catalog-pointer-")); + try { + // Ownership is by basename, which is what injection already uses. Pinned here so the + // detector cannot start disagreeing with the code that writes and strips the same line. + const present = join(dir, "opencodex-catalog.json"); + writeFileSync(present, "{}"); + const absent = join(dir, "gone", "opencodex-catalog.json"); + + expect(missingOwnedCatalogPath(configNaming(absent))).toBe(absent); + expect(missingOwnedCatalogPath(configNaming(present))).toBeNull(); + } finally { + removeTreeWithRetry(dir); + } + }); + + test("a catalog the user named is theirs, present or not", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-catalog-user-")); + try { + // Same missing file, a name we never write. Claiming it would put opencodex's recovery + // advice in front of a problem that is not opencodex's to explain. + const userOwned = join(dir, "gone", "my-catalog.json"); + expect(missingOwnedCatalogPath(configNaming(userOwned))).toBeNull(); + expect(missingOwnedCatalogPath('model = "gpt-5.5"\n')).toBeNull(); + } finally { + removeTreeWithRetry(dir); + } + }); + + test("the report names the file and both ways out", () => { + const lines = missingCodexCatalogLines("/somewhere/opencodex-catalog.json"); + expect(lines.length).toBeGreaterThan(0); + const joined = lines.join(" "); + expect(joined).toContain("/somewhere/opencodex-catalog.json"); + // Regenerating and removing are different outcomes; the user picks, so both are offered. + expect(joined).toContain("ocx start"); + expect(joined).toContain("ocx restore"); + expect(missingCodexCatalogLines(null)).toEqual([]); + }); + + test("the commands it names are real", () => { + for (const name of ["start", "restore"]) expect(findCommand(name)).toBeDefined(); + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 646d7e75263..90591082831 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -391,6 +391,7 @@ "codex-service-manager-probe-hardening.test.ts": "codex-integration", "codex-service-manager-probe.test.ts": "codex-integration", "codex-shim-autorestore.test.ts": "codex-integration", + "codex-shim-ensure-failure.test.ts": "codex-integration", "codex-shim-readiness.test.ts": "codex-integration", "codex-shim.test.ts": "codex-integration", "codex-signin-lockout.test.ts": "codex-integration", @@ -844,6 +845,7 @@ "oauth-login-summary.test.ts": "oauth", "oauth-manual-code.test.ts": "oauth", "oauth-open-browser-choice.test.ts": "oauth", + "oauth-open-url-result.test.ts": "oauth", "oauth-provider-reconcile.test.ts": "oauth", "oauth-public-surface.test.ts": "oauth", "oauth-reauth-bind.test.ts": "oauth", diff --git a/tests/oauth/oauth-login-open-browser.test.ts b/tests/oauth/oauth-login-open-browser.test.ts index 0b7928d2195..676641a305e 100644 --- a/tests/oauth/oauth-login-open-browser.test.ts +++ b/tests/oauth/oauth-login-open-browser.test.ts @@ -33,7 +33,7 @@ async function startLogin(flow: { url: string; instructions?: string; deviceCode const openUrlMod = await import("../../src/lib/open-url"); const opened: string[] = []; const startSpy = spyOn(oauth, "startLoginFlow").mockResolvedValue(flow); - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation((url: string) => { opened.push(url); }); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async (url: string) => { opened.push(url); return { status: "started" as const }; }); try { const req = loginRequest("kimi"); const response = await handleOauthAccountRoutes({ diff --git a/tests/oauth/oauth-open-browser-choice.test.ts b/tests/oauth/oauth-open-browser-choice.test.ts index 48c7f542788..81c133bb028 100644 --- a/tests/oauth/oauth-open-browser-choice.test.ts +++ b/tests/oauth/oauth-open-browser-choice.test.ts @@ -49,7 +49,7 @@ async function startLogin( const openUrlMod = await import("../../src/lib/open-url"); const opened: string[] = []; const startSpy = spyOn(oauth, "startLoginFlow").mockResolvedValue({ url: "https://accounts.x.ai/oauth/authorize?code_challenge=x" }); - const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation((url: string) => { opened.push(url); }); + const openSpy = spyOn(openUrlMod, "openUrl").mockImplementation(async (url: string) => { opened.push(url); return { status: "started" as const }; }); try { const req = new Request("http://127.0.0.1/api/oauth/login", { method: "POST", diff --git a/tests/oauth/oauth-open-url-result.test.ts b/tests/oauth/oauth-open-url-result.test.ts new file mode 100644 index 00000000000..17ca2d97fdb --- /dev/null +++ b/tests/oauth/oauth-open-url-result.test.ts @@ -0,0 +1,55 @@ +/** + * #5261: a browser that never opened must not look like a login that is working. + * + * The launcher used to swallow its own failure and return nothing, which is what left the Codex + * login route unable to tell a browser that opened from one that never did. These cover the + * launcher itself and the recovery line it feeds; the route and the CLI block that consume the + * result are covered where those live. + * + * Nothing here opens a real browser. The started case is deliberately untested rather than + * faked: the launcher command is fixed per platform, so proving it would mean actually + * launching one on the machine running the suite. + */ +import { describe, expect, test } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { tmpdir } from "node:os"; +import { openUrl } from "../../src/lib/open-url"; +import { BROWSER_LAUNCH_FAILED_HINT } from "../../src/cli/account-auth"; + +describe("browser launch is reported, not swallowed (#5261)", () => { + test("a URL we will not hand to a launcher is reported as such", async () => { + // Refusing a non-http scheme is the one failure that never reaches spawn, so it is also + // the one that must not silently look like a launch. + for (const url of ["", "not-a-url", "file:///etc/passwd", "javascript:alert(1)"]) { + expect(await openUrl(url)).toEqual({ status: "failed", reason: "invalid-url" }); + } + }); + + test.skipIf(process.platform !== "linux")("a launcher that cannot be resolved reports a failure", async () => { + // Linux only, and on purpose: this empties PATH so the launcher cannot be found, and on a + // developer machine with a real browser any weaker setup risks actually opening one. + const priorPath = process.env.PATH; + let emptyDir: string | undefined; + try { + emptyDir = mkdtempSync(`${tmpdir()}/ocx-empty-path-`); + process.env.PATH = emptyDir; + expect(await openUrl("http://127.0.0.1:1455/auth/callback")).toEqual({ + status: "failed", + reason: "spawn-error", + }); + } finally { + if (priorPath === undefined) delete process.env.PATH; + else process.env.PATH = priorPath; + if (emptyDir) removeTreeWithRetry(emptyDir); + } + }); + + test("the CLI hint names the manual route and the reason the port cannot move", () => { + // ChatGPT supplies the redirect URI, so the callback cannot move to a free port. That is + // the part a user cannot work out alone, which is why the hint names it and --device. + expect(BROWSER_LAUNCH_FAILED_HINT).toContain("1455"); + expect(BROWSER_LAUNCH_FAILED_HINT).toContain("--device"); + expect(BROWSER_LAUNCH_FAILED_HINT.toLowerCase()).toContain("open the url"); + }); +}); diff --git a/tests/service/autostart-health.test.ts b/tests/service/autostart-health.test.ts index 293f21f4cec..b681fa7a775 100644 --- a/tests/service/autostart-health.test.ts +++ b/tests/service/autostart-health.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { collectStartupHealth, deriveStartupHealth, formatStartupRoutingDetail, startupHealthSummary } from "../../src/codex/autostart-health"; +import { collectStartupHealth, deriveStartupHealth, formatStartupRoutingDetail, injectedRoutingRestartWarningLines, startupHealthSummary } from "../../src/codex/autostart-health"; import { unusedProxyWarningLines } from "../../src/cli/status"; import { classifyCodexRouting, hasInjectedCodexRouting } from "../../src/codex/inject"; import { isCodexClientProcess, listCodexClientProcesses } from "../../src/codex/native-profile-processes"; @@ -396,6 +396,37 @@ describe("routing visibility (#2411)", () => { expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "custom-local" })).toEqual([]); expect(unusedProxyWarningLines({ proxyUp: true, routingKind: "unknown" })).toEqual([]); }); + + // #5261: setup writes routing that outlives the session and then ends on a success line. + // The warning reuses the health model rather than re-deriving the condition, so it cannot + // disagree with what status and doctor say about the same install. + test("injectedRoutingRestartWarningLines speaks exactly when the install is restart-unsafe", () => { + const atRisk = deriveStartupHealth(base); + expect(atRisk.status).toBe("at-risk"); + const lines = injectedRoutingRestartWarningLines(atRisk); + expect(lines.length).toBeGreaterThan(0); + const joined = lines.join(" "); + expect(joined).toContain("survives a restart"); + expect(joined).toContain(startupHealthSummary(atRisk)); + // The way out that does not require the proxy to come back first. + expect(joined).toContain("ocx restore"); + + // A CLI-only shim still leaves Codex Desktop uncovered, which is the reported shape. + const shimmed = deriveStartupHealth({ ...base, shimInstalled: true, shimHealthy: true }); + expect(shimmed.status).toBe("at-risk"); + expect(injectedRoutingRestartWarningLines(shimmed).length).toBeGreaterThan(0); + // ...and the warning must stay true in that case: a healthy shim DOES restart the proxy, for + // CLI launches. Claiming nothing will would contradict the summary line printed beneath it. + expect(injectedRoutingRestartWarningLines(shimmed).join(" ")).not.toContain("nothing here will restart"); + expect(injectedRoutingRestartWarningLines(shimmed).join(" ")).toContain(startupHealthSummary(shimmed)); + + // Native routing has no opencodex restart dependency, so there is nothing to warn about. + expect(injectedRoutingRestartWarningLines(deriveStartupHealth({ ...base, routingKind: "native" }))).toEqual([]); + // Neither does a viable service, which is the state the warning is steering toward. + const served = deriveStartupHealth({ ...base, serviceInstalled: true, serviceViable: true, serviceEnabled: true, serviceRunning: true }); + expect(served.status).toBe("protected"); + expect(injectedRoutingRestartWarningLines(served)).toEqual([]); + }); }); // #4550: configured routing is not adopted routing. A Codex client that started