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
This commit is contained in:
roboomp
@@ -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
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
Reference in New Issue
Block a user