From 8d05b454ba934c3d77c179baee2db38f955f864b Mon Sep 17 00:00:00 2001 From: Tom Riglar Date: Mon, 26 Oct 2026 09:03:20 +0000 Subject: [PATCH] fix(device): don't crash when no browser opener is installed spawn reports a missing opener (xdg-open on a headless box) as an 'error' event, not a throw, and with no listener it crashed the process after dcd device open had printed "Opened the monitor". openBrowser now waits for 'spawn' or 'error' and returns false, so callers print the URL instead. Also corrects CLAUDE.md and the bridge.ts header: the bridge keeps one connection and re-reads auth on every request; it no longer remakes the connection when a session is renewed. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- src/commands/device.ts | 2 +- src/commands/login.ts | 2 +- src/mcp/bridge.ts | 4 ++-- src/utils/browser.ts | 32 +++++++++++++++++++++++--------- test/unit/browser.test.ts | 15 ++++++++++----- 6 files changed, 38 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4ea6ff6..3cef23c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,7 +53,7 @@ Top-level `defineCommand` in `src/index.ts` wires thirteen subcommands (`cloud`, **MCP server.** `src/mcp/` is a third front-end onto the same service layer (a sibling to `src/commands/`), shipped as the `dcd-mcp` bin over stdio transport (`@modelcontextprotocol/sdk`, `zod` schemas). `index.ts` boots the server; `server.ts` registers tools; `context.ts` resolves auth + API URL **lazily**, and again when a `dcd login` session nears expiry (so `tools/list` works unauthenticated, auth errors surface as tool errors rather than a boot crash, and a long-lived server outlasts the one-hour access token) via the same `resolveAuth`/`resolveApiUrl` as the CLI — `DEVICE_CLOUD_API_KEY` env or stored `dcd login` session, with `DCD_API_URL` to override. **Critical invariant: stdout is the JSON-RPC channel** — tools must never call the `src/commands/*` layer or `utils/cli` `logger` (both write to stdout / can `process.exit`); they call services/gateways directly with `logStderr` and return data via `helpers.ts` `jsonResult`/`errorResult`. The `runTool` wrapper records `mcp tool …` telemetry and converts thrown errors to `isError` results. Tools: `dcd_list_devices`, `dcd_list_runs`, `dcd_get_status`, `dcd_download_artifacts` (read-only), and `dcd_run_cloud_test` (billable — gated out by `--read-only` / `DCD_MCP_READONLY=1`, annotated destructive, async-by-default). `dcd_run_cloud_test` reuses `computeCommonRoot`/`buildTestMetadataMap` from `src/services/flow-paths.ts` (extracted from `cloud.ts` so both build identical server-side paths). Registry manifest: `server.json` at repo root. -**`dcd mcp` (the bridge).** A second stdio entry point, `src/mcp/bridge.ts`, run by the `dcd mcp` command. It is a low-level `Server` that routes between two MCP clients: the `dcd_*` tools from the same in-process `createServer()` (over an in-memory transport), and the device tools proxied from the hosted `/mcp`, so those are defined and contract-tested once, in the dcd API. It adds the inputs only a local process can serve: `install_app` takes `app_path` (uploaded with `uploadLocalApp`) and `run` takes `path` (a file is sent as `yaml`, a directory is uploaded as a flow bundle). Credentials are the CLI's (`getContext`): the hosted `/mcp` accepts an API key or a `dcd login` session with `x-dcd-org`, and the remote connection is remade when a session is renewed. If the hosted endpoint can't be reached, it serves the `dcd_*` tools only and says why on stderr. Read-only mode keeps only device tools annotated `readOnlyHint`. `dcd-mcp` itself is unchanged. +**`dcd mcp` (the bridge).** A second stdio entry point, `src/mcp/bridge.ts`, run by the `dcd mcp` command. It is a low-level `Server` that routes between two MCP clients: the `dcd_*` tools from the same in-process `createServer()` (over an in-memory transport), and the device tools proxied from the hosted `/mcp`, so those are defined and contract-tested once, in the dcd API. It adds the inputs only a local process can serve: `install_app` takes `app_path` (uploaded with `uploadLocalApp`) and `run` takes `path` (a file is sent as `yaml`, a directory is uploaded as a flow bundle). Credentials are the CLI's (`getContext`): the hosted `/mcp` accepts an API key or a `dcd login` session with `x-dcd-org`. One connection is shared, and its `fetch` re-reads the auth headers from `getContext()` on every request, so a renewed session is picked up without remaking the connection (which would close it under a `run` still going). If the hosted endpoint can't be reached, it serves the `dcd_*` tools only and says why on stderr. Read-only mode keeps only device tools annotated `readOnlyHint`. `dcd-mcp` itself is unchanged. ## Contributing diff --git a/src/commands/device.ts b/src/commands/device.ts index 1e7f265..43a371a 100644 --- a/src/commands/device.ts +++ b/src/commands/device.ts @@ -1041,7 +1041,7 @@ const openSub = defineCommand({ const id = await resolveSessionId(ctx.apiUrl, ctx.auth, args.id as string | undefined); const { session } = await DeviceGateway.status(ctx.apiUrl, ctx.auth, id); // Only a person at a terminal gets a browser; a script gets the URL. - const opened = interactive(json) && openBrowser(session.monitor_url); + const opened = interactive(json) && (await openBrowser(session.monitor_url)); return { session, data: { url: session.monitor_url, opened }, diff --git a/src/commands/login.ts b/src/commands/login.ts index 611eef6..553cf2b 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -171,7 +171,7 @@ export const loginCommand = defineCommand({ if (noBrowser) { rows.push(ui.note('Open this URL in a browser to finish login:'), colors.highlight(loginUrl)); } else { - const opened = openBrowser(loginUrl); + const opened = await openBrowser(loginUrl); rows.push( ui.note( opened diff --git a/src/mcp/bridge.ts b/src/mcp/bridge.ts index cd34889..636e475 100644 --- a/src/mcp/bridge.ts +++ b/src/mcp/bridge.ts @@ -10,8 +10,8 @@ * a bundle). * * Auth is the CLI's: an API key or a `dcd login` session (getContext). The - * hosted endpoint accepts both. A session is renewed before it expires, and - * the remote connection is remade with it. + * hosted endpoint accepts both. A session is renewed before it expires; the + * one shared connection re-reads the auth on every request (RemoteDevices). * * stdout is the JSON-RPC channel: everything else goes to stderr. */ diff --git a/src/utils/browser.ts b/src/utils/browser.ts index 627cae9..93f51c1 100644 --- a/src/utils/browser.ts +++ b/src/utils/browser.ts @@ -2,14 +2,14 @@ import { spawn } from 'node:child_process'; /** * Open `url` in the default browser. False if it isn't an http(s) URL or no - * opener could be started. + * opener could be started (none installed, as on a headless box). * * Only http(s) is opened, so a URL can't name a local file or start with `-` * and be read as an option. No opener goes through a shell: Windows uses * rundll32, because `cmd /c start` parses the URL again and treats its `&` * as a command separator. */ -export function openBrowser(url: string): boolean { +export async function openBrowser(url: string): Promise { let href: string; try { const parsed = new URL(url); @@ -25,11 +25,25 @@ export function openBrowser(url: string): boolean { : process.platform === 'win32' ? { bin: 'rundll32', args: ['url.dll,FileProtocolHandler', href] } : { bin: 'xdg-open', args: [href] }; - try { - const child = spawn(cmd.bin, cmd.args, { detached: true, stdio: 'ignore' }); - child.unref(); - return true; - } catch { - return false; - } + return startOpener(cmd.bin, cmd.args); +} + +/** + * Start `bin` detached. False if it couldn't be started. A missing program is + * reported as an 'error' event, not thrown, and with no listener it would + * crash the process, so this waits for 'spawn' or 'error' to know. + */ +export function startOpener(bin: string, args: string[]): Promise { + return new Promise((resolve) => { + try { + const child = spawn(bin, args, { detached: true, stdio: 'ignore' }); + child.once('spawn', () => { + child.unref(); + resolve(true); + }); + child.once('error', () => resolve(false)); + } catch { + resolve(false); + } + }); } diff --git a/test/unit/browser.test.ts b/test/unit/browser.test.ts index 0934735..e13e000 100644 --- a/test/unit/browser.test.ts +++ b/test/unit/browser.test.ts @@ -1,11 +1,16 @@ import { expect } from 'chai'; -import { openBrowser } from '../../src/utils/browser.js'; +import { openBrowser, startOpener } from '../../src/utils/browser.js'; describe('openBrowser', () => { - it('opens nothing that is not an http(s) URL', () => { - expect(openBrowser('file:///etc/passwd')).to.equal(false); - expect(openBrowser('--help')).to.equal(false); - expect(openBrowser('not a url')).to.equal(false); + it('opens nothing that is not an http(s) URL', async () => { + expect(await openBrowser('file:///etc/passwd')).to.equal(false); + expect(await openBrowser('--help')).to.equal(false); + expect(await openBrowser('not a url')).to.equal(false); + }); + + it('returns false, rather than crashing, when the opener is not installed', async () => { + // As with xdg-open on a headless box: spawn emits ENOENT as an event. + expect(await startOpener('dcd-test-no-such-opener', ['https://example.com'])).to.equal(false); }); });