From a67da14e4e1bef3b37b3e742e5a815ff803d230a Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 9 Aug 2026 18:40:24 +0000 Subject: [PATCH] fix(cli): surface actionable error when auth broker is unreachable runRootCommand called discoverAuthStorage without a try/catch, so a configured-but-unreachable broker with no cached snapshot re-threw AuthBrokerError as a raw uncaught exception at startup, unlike the other startup paths that print a clean stderr message and exit non-zero. Wrap the startup auth discovery: broker failures now report an actionable message naming the broker URL and the recovery options (start it with `omp auth-broker serve`, or reset `auth.broker.url`/`auth.broker.token`) and exit 1. Unrelated errors still propagate. The broker still replaces the local store when configured; no silent fallback to local credentials. Fixes #8096 --- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/src/main.ts | 15 ++- .../src/session/auth-broker-config.ts | 38 +++++++ ...ue-8096-broker-unreachable-startup.test.ts | 103 ++++++++++++++++++ 4 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/issue-8096-broker-unreachable-startup.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a4d66e8d2..b51e09b55 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the CLI crashing at startup with a raw uncaught `AuthBrokerError` when a configured auth broker (`auth.broker.url` / `OMP_AUTH_BROKER_URL`) is unreachable and no fresh cached snapshot exists. Startup auth discovery now fails with an actionable message naming the broker URL and the recovery options (`omp auth-broker serve`, or resetting `auth.broker.url` / `auth.broker.token`) and exits non-zero, instead of dumping a stack trace ([#8096](https://github.com/can1357/oh-my-pi/issues/8096)). + ## [17.2.12] - 2026-08-08 ### Fixed diff --git a/packages/coding-agent/src/main.ts b/packages/coding-agent/src/main.ts index c9de644e9..306a7403a 100644 --- a/packages/coding-agent/src/main.ts +++ b/packages/coding-agent/src/main.ts @@ -74,6 +74,7 @@ import { loadSessionExtensions, } from "./sdk"; import type { AgentSession } from "./session/agent-session"; +import { describeAuthBrokerStartupError } from "./session/auth-broker-config"; import type { AuthStorage } from "./session/auth-storage"; import { describePendingToolCalls } from "./session/exit-diagnostics"; import { @@ -1276,8 +1277,18 @@ export async function runRootCommand( // tree; declare it so headless subagent optimizations (e.g. skipping replan // title refresh) can tell a focusable process from a print/RPC/eval one. setInteractiveHost(isInteractive); - // Create AuthStorage and ModelRegistry upfront - const authStorage = await logger.time("discoverAuthStorage", deps.discoverAuthStorage ?? discoverAuthStorage); + // Create AuthStorage and ModelRegistry upfront. A configured-but-unreachable + // auth broker throws here; convert it to an actionable stderr message + clean + // exit instead of a raw uncaught stack trace (issue #8096). + let authStorage: AuthStorage; + try { + authStorage = await logger.time("discoverAuthStorage", deps.discoverAuthStorage ?? discoverAuthStorage); + } catch (error) { + const message = await describeAuthBrokerStartupError(error); + if (message === null) throw error; + process.stderr.write(`${chalk.red(`Error: ${message}`)}\n`); + process.exit(1); + } const modelRegistry = logger.time("modelRegistry:init", () => new ModelRegistry(authStorage)); const settingsInstance = diff --git a/packages/coding-agent/src/session/auth-broker-config.ts b/packages/coding-agent/src/session/auth-broker-config.ts index 184c8ef65..860c4bd47 100644 --- a/packages/coding-agent/src/session/auth-broker-config.ts +++ b/packages/coding-agent/src/session/auth-broker-config.ts @@ -21,6 +21,7 @@ * boot without forcing a startup reorder. */ +import { AuthBrokerError } from "@oh-my-pi/pi-ai/auth-broker"; import { type AuthBrokerClientConfig, type DiscoverAuthStorageOptions, @@ -28,6 +29,7 @@ import { getAuthBrokerTokenFilePath, resolveAuthBrokerConfig as resolveAuthBrokerConfigShared, } from "@oh-my-pi/pi-ai/auth-broker/discover"; +import { MissingApiKeyError } from "@oh-my-pi/pi-ai/error"; import { getAgentDir } from "@oh-my-pi/pi-utils"; import { resolveConfigValue } from "../config/resolve-config-value"; import type { AuthStorage } from "./auth-storage"; @@ -90,3 +92,39 @@ export function discoverAuthStorage( configValueResolver: resolveConfigValue, }); } + +/** + * Turn an auth-storage discovery failure raised at CLI startup into a clean, + * actionable message, or return `null` when the error is unrelated to the + * broker (so the caller rethrows it unchanged). + * + * A configured broker deliberately *replaces* the local credential store — + * {@link discoverAuthStorage} never silently falls back to local SQLite once + * `auth.broker.url` is set — so an unreachable broker is fatal. Without this, + * the underlying `AuthBrokerError` (or missing-token `MissingApiKeyError`) + * propagates as a raw uncaught exception and the CLI dies with a stack dump + * instead of recovery guidance (issue #8096). + */ +export async function describeAuthBrokerStartupError(error: unknown): Promise { + if (error instanceof MissingApiKeyError) { + // resolveAuthBrokerConfig already built an actionable message naming the + // env var / config key / token-file path to set. + return error.message; + } + if (!(error instanceof AuthBrokerError)) return null; + let url: string | undefined; + try { + url = (await resolveAuthBrokerConfig())?.url; + } catch { + // Config resolution itself failed (e.g. token vanished); fall back to a + // URL-less message rather than masking the original broker failure. + } + const target = url ? ` at ${url}` : ""; + return ( + `Auth broker${target} is unreachable (${error.message}). ` + + "omp is configured to use this broker for credentials and will not fall back to local credentials automatically.\n" + + "Start the broker with `omp auth-broker serve`, or disable it with " + + "`omp config reset auth.broker.url` and `omp config reset auth.broker.token` " + + "(or unset OMP_AUTH_BROKER_URL / OMP_AUTH_BROKER_TOKEN)." + ); +} diff --git a/packages/coding-agent/test/issue-8096-broker-unreachable-startup.test.ts b/packages/coding-agent/test/issue-8096-broker-unreachable-startup.test.ts new file mode 100644 index 000000000..0615f2584 --- /dev/null +++ b/packages/coding-agent/test/issue-8096-broker-unreachable-startup.test.ts @@ -0,0 +1,103 @@ +/** + * Regression (issue #8096): a configured-but-unreachable auth broker must not + * crash startup with a raw uncaught `AuthBrokerError` stack trace. Startup auth + * discovery is wrapped so the failure surfaces as an actionable stderr message + * and a clean `process.exit(1)`, mirroring the other startup error paths + * (session resolution, model resolution, export). + * + * The broker deliberately replaces the local credential store when configured, + * so an unreachable broker stays fatal — the fix is the recovery guidance, not + * a silent fallback to local credentials. + */ +import { describe, expect, it, vi } from "bun:test"; +import { AuthBrokerError } from "@oh-my-pi/pi-ai/auth-broker"; +import { MissingApiKeyError } from "@oh-my-pi/pi-ai/error"; +import { parseArgs } from "@oh-my-pi/pi-coding-agent/cli/args"; +import { runRootCommand } from "@oh-my-pi/pi-coding-agent/main"; +import { describeAuthBrokerStartupError } from "@oh-my-pi/pi-coding-agent/session/auth-broker-config"; +import { setInteractiveHost } from "@oh-my-pi/pi-utils"; + +class ProcessExitSignal extends Error { + constructor(readonly code: number) { + super(`process.exit(${code})`); + this.name = "ProcessExitSignal"; + } +} + +describe("describeAuthBrokerStartupError", () => { + it("turns a broker connection failure into recovery guidance", async () => { + const message = await describeAuthBrokerStartupError( + new AuthBrokerError("Auth broker request failed after 2 attempt(s)"), + ); + expect(message).not.toBeNull(); + expect(message).toContain("Auth broker request failed after 2 attempt(s)"); + // Both recovery routes the reporter asked for: start it, or disable it. + expect(message).toContain("omp auth-broker serve"); + expect(message).toContain("omp config reset auth.broker.url"); + expect(message).toContain("OMP_AUTH_BROKER_URL"); + }); + + it("names the configured broker URL when it can be resolved", async () => { + const prevUrl = process.env.OMP_AUTH_BROKER_URL; + const prevToken = process.env.OMP_AUTH_BROKER_TOKEN; + process.env.OMP_AUTH_BROKER_URL = "http://127.0.0.1:8765"; + process.env.OMP_AUTH_BROKER_TOKEN = "test-token"; + try { + const message = await describeAuthBrokerStartupError(new AuthBrokerError("connection refused")); + expect(message).toContain("http://127.0.0.1:8765"); + } finally { + if (prevUrl === undefined) delete process.env.OMP_AUTH_BROKER_URL; + else process.env.OMP_AUTH_BROKER_URL = prevUrl; + if (prevToken === undefined) delete process.env.OMP_AUTH_BROKER_TOKEN; + else process.env.OMP_AUTH_BROKER_TOKEN = prevToken; + } + }); + + it("passes through a missing-token message unchanged", async () => { + const err = new MissingApiKeyError(undefined, "OMP_AUTH_BROKER_URL is set but no bearer token is available."); + expect(await describeAuthBrokerStartupError(err)).toBe(err.message); + }); + + it("returns null for unrelated errors so the caller rethrows them", async () => { + expect(await describeAuthBrokerStartupError(new Error("boom"))).toBeNull(); + expect(await describeAuthBrokerStartupError(new TypeError("nope"))).toBeNull(); + }); +}); + +describe("runRootCommand — unreachable auth broker at startup", () => { + it("exits 1 with an actionable message instead of an uncaught AuthBrokerError", async () => { + const previous = setInteractiveHost(false); + const parsed = parseArgs([]); + parsed.noExtensions = true; + + const exitCodes: number[] = []; + let stderr = ""; + vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + exitCodes.push(code ?? 0); + throw new ProcessExitSignal(code ?? 0); + }) as typeof process.exit); + vi.spyOn(process.stderr, "write").mockImplementation((chunk: unknown) => { + stderr += String(chunk); + return true; + }); + + let thrown: unknown; + try { + await runRootCommand(parsed, [], { + discoverAuthStorage: async () => { + throw new AuthBrokerError("Auth broker request failed after 2 attempt(s)"); + }, + }); + } catch (err) { + thrown = err; + } finally { + vi.restoreAllMocks(); + setInteractiveHost(previous); + } + + expect(thrown).toBeInstanceOf(ProcessExitSignal); + expect(exitCodes).toEqual([1]); + expect(stderr).toContain("Auth broker request failed after 2 attempt(s)"); + expect(stderr).toContain("omp auth-broker serve"); + }, 15_000); +});