Merge PR #8097: fix(cli): surface actionable error when auth broker is unreachable (@roboomp)

2026-08-13 01:14:47 +02:00
parent 99e8fe968c a67da14e4e
commit e8782235e2
4 changed files with 155 additions and 2 deletions
@@ -103,6 +103,7 @@
 - Fixed retry-fallback selection switching a live session from a large-context primary onto a smaller-context fallback and immediately sending a predictably oversized request; candidate selection now skips any fallback whose usable window cannot hold the current context and advances to the first configured candidate that fits ([#8065](https://github.com/can1357/oh-my-pi/issues/8065)).
 - Fixed advisor recovery selecting another role's fallback chain when both roles use the same model. ([#8075](https://github.com/can1357/oh-my-pi/issues/8075))
 - Fixed `retry_fallback_applied` and `retry_fallback_succeeded` not being forwarded to extensions: `AgentSession.#emitExtensionEvent` had no branch for either event and `ExtensionAPI.on(...)` lacked overloads, so extension handlers could not observe model/advisor fallback transitions or successes that the TUI and RPC paths already received ([#8079](https://github.com/can1357/oh-my-pi/issues/8079)).
+- 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

@@ -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 =
@@ -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<string | null> {
+	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)."
+	);
+}
@@ -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);
+});