diff --git a/apps/desktop/electron/dev-cdp.test.ts b/apps/desktop/electron/dev-cdp.test.ts index bb86a57a1287..f86aa9db3041 100644 --- a/apps/desktop/electron/dev-cdp.test.ts +++ b/apps/desktop/electron/dev-cdp.test.ts @@ -8,73 +8,87 @@ import assert from 'node:assert/strict' import { test } from 'vitest' -import { describeDevCdpDecision, resolveDevCdpPort } from './dev-cdp' +import { DEFAULT_PORT, describeDevCdpDecision, resolveDevCdpPort } from './dev-cdp' const DEV_SERVER = 'http://127.0.0.1:5174' -/** A dev-server run that asked for a port — the one combination that opens it. */ -const opted = { env: { HERMES_DESKTOP_CDP_PORT: '9222' }, isPackaged: false, devServer: DEV_SERVER } +/** The ordinary `npm run dev` / `hgui` run. */ +const devRun = { env: {}, isPackaged: false, devServer: DEV_SERVER } -test('opens the requested port for an opted-in dev-server run', () => { - assert.deepEqual(resolveDevCdpPort(opted), { port: 9222, reason: null }) +test('a dev-server run opens the default port with no opt-in', () => { + assert.deepEqual(resolveDevCdpPort(devRun), { port: DEFAULT_PORT, reason: null }) }) -test('stays closed unless the developer asks for it', () => { - // The default `npm run dev` / `hgui` path: dev server, no opt-in. - assert.deepEqual(resolveDevCdpPort({ ...opted, env: {} }), { port: null, reason: 'not-requested' }) +test('the default matches what the scripts/ tooling reaches for', () => { + // scripts/eval.mjs and scripts/perf/lib/cdp.mjs both default here; if this + // drifts, `node scripts/eval.mjs ...` stops finding a live renderer. + assert.equal(DEFAULT_PORT, 9222) }) test('a packaged build never opens the port, however loudly the env asks', () => { - assert.deepEqual(resolveDevCdpPort({ ...opted, isPackaged: true }), { port: null, reason: 'packaged' }) + const decision = resolveDevCdpPort({ ...devRun, env: { HERMES_DESKTOP_CDP_PORT: '9222' }, isPackaged: true }) + + assert.deepEqual(decision, { port: null, reason: 'packaged' }) }) -test('packaged wins over every other gate', () => { - // Belt-and-suspenders: even with a dev server present and a valid port - // requested, packaged is checked first and short-circuits. - const decision = resolveDevCdpPort({ - env: { HERMES_DESKTOP_CDP_PORT: '9222' }, - isPackaged: true, - devServer: DEV_SERVER - }) +test('packaged is checked before every other gate', () => { + // Belt-and-suspenders: dev server present, valid port requested, still shut. + for (const value of ['9222', '', 'off', 'garbage']) { + const decision = resolveDevCdpPort({ + env: { HERMES_DESKTOP_CDP_PORT: value }, + isPackaged: true, + devServer: DEV_SERVER + }) - assert.equal(decision.port, null) - assert.equal(decision.reason, 'packaged') + assert.equal(decision.port, null, `expected packaged to refuse ${JSON.stringify(value)}`) + assert.equal(decision.reason, 'packaged') + } }) test('an unpackaged dist run (no dev server) does not qualify', () => { // `electron .` against dist/ is how the packaged app gets smoke tested; it // should behave like the packaged app, not like a source-tree dev run. - assert.deepEqual(resolveDevCdpPort({ ...opted, devServer: undefined }), { port: null, reason: 'no-dev-server' }) + assert.deepEqual(resolveDevCdpPort({ ...devRun, devServer: undefined }), { port: null, reason: 'no-dev-server' }) }) -test('rejects ports that are not usable integers', () => { - for (const value of ['0', '80', '-1', '70000', 'yes', '9222.5', '92 22', '']) { - const decision = resolveDevCdpPort({ ...opted, env: { HERMES_DESKTOP_CDP_PORT: value } }) +test('the port is overridable', () => { + assert.equal(resolveDevCdpPort({ ...devRun, env: { HERMES_DESKTOP_CDP_PORT: '9333' } }).port, 9333) +}) + +test('tolerates surrounding whitespace on the override', () => { + assert.equal(resolveDevCdpPort({ ...devRun, env: { HERMES_DESKTOP_CDP_PORT: ' 9333 ' } }).port, 9333) +}) + +test('can be switched off on a dev run', () => { + for (const value of ['0', 'off', 'OFF', 'false', 'no']) { + const decision = resolveDevCdpPort({ ...devRun, env: { HERMES_DESKTOP_CDP_PORT: value } }) + + assert.equal(decision.port, null, `expected ${JSON.stringify(value)} to close the port`) + assert.equal(decision.reason, 'opted-out') + } +}) + +test('refuses ports that are not usable integers', () => { + for (const value of ['80', '-1', '70000', 'yes', '9222.5', '92 22']) { + const decision = resolveDevCdpPort({ ...devRun, env: { HERMES_DESKTOP_CDP_PORT: value } }) assert.equal(decision.port, null, `expected ${JSON.stringify(value)} to be refused`) + assert.equal(decision.reason, 'invalid-port') } }) -test('tolerates surrounding whitespace on the requested port', () => { - assert.equal(resolveDevCdpPort({ ...opted, env: { HERMES_DESKTOP_CDP_PORT: ' 9333 ' } }).port, 9333) -}) +test('explains itself when an explicit setting was not honoured', () => { + // A typo'd port or a deliberate opt-out should say so — silently doing + // something other than what the env asked for is the bad failure mode. + for (const value of ['garbage', 'off']) { + const decision = resolveDevCdpPort({ ...devRun, env: { HERMES_DESKTOP_CDP_PORT: value } }) -test('every refusal that followed an explicit request explains itself', () => { - // An opt-in that gets ignored must say why — a silent no-op is the failure - // mode where someone burns an hour wondering why nothing is listening. - const refusals = [ - resolveDevCdpPort({ ...opted, isPackaged: true }), - resolveDevCdpPort({ ...opted, devServer: undefined }), - resolveDevCdpPort({ ...opted, env: { HERMES_DESKTOP_CDP_PORT: 'nope' } }) - ] - - for (const decision of refusals) { - assert.equal(decision.port, null) - assert.ok(describeDevCdpDecision(decision), `expected an explanation for ${decision.reason}`) + assert.ok(describeDevCdpDecision(decision), `expected an explanation for ${JSON.stringify(value)}`) } }) -test('says nothing when the port opened, or when it was never requested', () => { - assert.equal(describeDevCdpDecision(resolveDevCdpPort(opted)), null) - assert.equal(describeDevCdpDecision(resolveDevCdpPort({ ...opted, env: {} })), null) +test('stays quiet when the port opened, or is closed by design', () => { + assert.equal(describeDevCdpDecision(resolveDevCdpPort(devRun)), null) + assert.equal(describeDevCdpDecision(resolveDevCdpPort({ ...devRun, isPackaged: true })), null) + assert.equal(describeDevCdpDecision(resolveDevCdpPort({ ...devRun, devServer: undefined })), null) }) diff --git a/apps/desktop/electron/dev-cdp.ts b/apps/desktop/electron/dev-cdp.ts index dc4495fcd796..c5c4e3ef3534 100644 --- a/apps/desktop/electron/dev-cdp.ts +++ b/apps/desktop/electron/dev-cdp.ts @@ -1,21 +1,26 @@ /** - * Dev-only Chrome DevTools Protocol exposure for the desktop renderer. + * Dev Chrome DevTools Protocol exposure for the desktop renderer. * * The renderer is a Chromium page, so `--remote-debugging-port` turns it into * something the repo's existing CDP tooling (`scripts/eval.mjs`, * `scripts/perf/lib/cdp.mjs`, the `diag-*` / `probe-*` family) can attach to - * and read the live DOM from. That is genuinely useful while iterating on the - * UI — and it is also arbitrary code execution against whatever the running - * app can reach, so it stays off unless three independent conditions all hold: + * and read the live DOM from. Every one of those scripts already defaults to + * 9222, so a dev-server run opens 9222 and they just work. * - * 1. The build is NOT packaged. A shipped app never opens this port, whatever - * the environment says. - * 2. A dev server is wired up (`HERMES_DESKTOP_DEV_SERVER`). That is the - * signature of `npm run dev` / `hgui`; a packaged or `dist`-loading run - * has no dev server and does not qualify. - * 3. The developer opted in explicitly with a valid `HERMES_DESKTOP_CDP_PORT`. - * Absent that, a plain `npm run dev` behaves exactly as it does today — - * nobody gets a debugging port they did not ask for. + * If you are running a dev server you are already executing arbitrary local + * JS — vite's module graph and every postinstall in node_modules — so a + * loopback debugging port does not meaningfully widen that. `perf:serve` + * already opens one unconditionally. What must never happen is a *packaged* + * app exposing it, which is the one hard gate here. + * + * - packaged build → always closed, whatever the env says. + * - no HERMES_DESKTOP_DEV_SERVER → closed (an unpackaged `electron .` against + * dist/ is how the packaged app gets smoke tested; it should behave like + * the packaged app). + * - otherwise → open on 9222, or HERMES_DESKTOP_CDP_PORT. + * + * `HERMES_DESKTOP_CDP_PORT=off` (or `0` / `false`) opts out for anyone who + * wants the port closed on a dev run. * * The port binds to loopback (Chromium's default) and the address is * deliberately not configurable: there is no reason to expose a renderer @@ -23,7 +28,7 @@ */ /** Why the port is closed, for a one-line log the developer can act on. */ -type ClosedReason = 'packaged' | 'no-dev-server' | 'not-requested' | 'invalid-port' +type ClosedReason = 'packaged' | 'no-dev-server' | 'opted-out' | 'invalid-port' type DevCdpDecision = { port: number; reason: null } | { port: null; reason: ClosedReason } @@ -33,11 +38,15 @@ type DevCdpInput = { devServer: string | undefined } -// Below 1024 needs privileges; the ephemeral range is fair game but the -// well-known CDP port (9222) is what every script in scripts/ defaults to. +/** What every script under scripts/ already reaches for. */ +const DEFAULT_PORT = 9222 + +// Below 1024 needs privileges on most platforms; 65535 is the ceiling. const MIN_PORT = 1024 const MAX_PORT = 65535 +const OPT_OUT = new Set(['0', 'off', 'false', 'no']) + /** * Decide whether this run may expose a renderer debugging port, and on which * port. Pure: every input is passed in, so the gate is testable without an @@ -50,17 +59,19 @@ function resolveDevCdpPort({ env, isPackaged, devServer }: DevCdpInput): DevCdpD return { port: null, reason: 'packaged' } } + // A dev server means a source-tree run (`npm run dev` / `hgui`). + if (!devServer) { + return { port: null, reason: 'no-dev-server' } + } + const requested = (env.HERMES_DESKTOP_CDP_PORT ?? '').trim() if (!requested) { - return { port: null, reason: 'not-requested' } + return { port: DEFAULT_PORT, reason: null } } - // A dev server means a source-tree run (`npm run dev` / `hgui`). An - // unpackaged `electron .` against dist/ is how the packaged app is smoke - // tested, and it should behave like the packaged app here. - if (!devServer) { - return { port: null, reason: 'no-dev-server' } + if (OPT_OUT.has(requested.toLowerCase())) { + return { port: null, reason: 'opted-out' } } const port = Number(requested) @@ -78,19 +89,20 @@ function describeDevCdpDecision(decision: DevCdpDecision): string | null { case null: return null + case 'invalid-port': + return `HERMES_DESKTOP_CDP_PORT is not a valid port (expected an integer ${MIN_PORT}-${MAX_PORT}, or "off"); renderer debugging is disabled.` + + case 'opted-out': + return 'renderer debugging disabled by HERMES_DESKTOP_CDP_PORT.' + + // Packaged and dist-run builds are closed by design — the common case, not + // worth a line of startup noise. case 'packaged': - return 'HERMES_DESKTOP_CDP_PORT ignored: renderer debugging is dev-only and this is a packaged build.' case 'no-dev-server': - return 'HERMES_DESKTOP_CDP_PORT ignored: no HERMES_DESKTOP_DEV_SERVER, so this is not a dev-server run.' - - case 'invalid-port': - return `HERMES_DESKTOP_CDP_PORT ignored: not a valid port (expected an integer ${MIN_PORT}-${MAX_PORT}).` - - case 'not-requested': return null } } -export { describeDevCdpDecision, resolveDevCdpPort } +export { DEFAULT_PORT, describeDevCdpDecision, resolveDevCdpPort } export type { DevCdpDecision } diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index c0194c9437e3..aad243ad188e 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -273,9 +273,10 @@ if (REMOTE_DISPLAY_REASON) { ) } -// Renderer debugging port for `hgui` / `npm run dev`. Opt-in, dev-server-only, -// never packaged — see electron/dev-cdp.ts for the gate. Must run before app -// `ready` like the switches above; Chromium binds it at launch. +// Renderer debugging port. On for dev-server runs (`hgui` / `npm run dev`) so +// the CDP tooling in scripts/ can attach; never for a packaged build — see +// electron/dev-cdp.ts. Must run before app `ready` like the switches above; +// Chromium binds it at launch. const DEV_CDP = resolveDevCdpPort({ env: process.env, isPackaged: IS_PACKAGED, devServer: DEV_SERVER }) if (DEV_CDP.port) { @@ -284,8 +285,8 @@ if (DEV_CDP.port) { // so a future edit can't widen it by omission. app.commandLine.appendSwitch('remote-debugging-address', '127.0.0.1') console.log( - `[hermes] renderer debugging on http://127.0.0.1:${DEV_CDP.port} (dev only; HERMES_DESKTOP_CDP_PORT). ` + - 'Anything that can reach this port can run code in the renderer.' + `[hermes] renderer debugging on http://127.0.0.1:${DEV_CDP.port} — anything that can reach it ` + + 'can run code in the renderer. HERMES_DESKTOP_CDP_PORT=off to disable.' ) } else { const why = describeDevCdpDecision(DEV_CDP) diff --git a/apps/desktop/scripts/eval.mjs b/apps/desktop/scripts/eval.mjs index 5e1ff95f3c7b..5f9fb282ec94 100644 --- a/apps/desktop/scripts/eval.mjs +++ b/apps/desktop/scripts/eval.mjs @@ -13,7 +13,8 @@ try { } catch { console.error( `no renderer debugging port on 127.0.0.1:${port}. ` + - 'Relaunch the app with HERMES_DESKTOP_CDP_PORT set, or point this script at the right port.' + 'Dev-server runs (`npm run dev` / `hgui`) open one automatically — check the app is running, ' + + 'and that HERMES_DESKTOP_CDP_PORT is not set to "off" or another port.' ) process.exit(1) } diff --git a/website/docs/reference/environment-variables.md b/website/docs/reference/environment-variables.md index 1a72ca10e749..948a118c97f8 100644 --- a/website/docs/reference/environment-variables.md +++ b/website/docs/reference/environment-variables.md @@ -530,7 +530,7 @@ Three dashboard-auth providers ship in the box. For a remote Hermes Desktop conn | `HERMES_DESKTOP_CWD` | Initial project directory for Desktop chat sessions. Set by `hermes desktop --cwd`. | | `HERMES_DESKTOP_PYTHON` | Absolute path to a Python interpreter for the backend, checked before Electron auto-resolves one for the source checkout. Used by worktree dev helpers (see [TUI & Desktop from Worktrees](../developer-guide/worktree-ui-dev.md)) to reuse a shared venv. | | `HERMES_DESKTOP_DEV_SERVER` | Vite dev-server URL the Electron shell loads instead of the packaged bundle (e.g. `http://127.0.0.1:5174`). Set automatically by `npm run dev`; only relevant when hacking on the app. | -| `HERMES_DESKTOP_CDP_PORT` | Opens a Chrome DevTools Protocol port on `127.0.0.1` for the renderer, so DOM/CSS inspection tooling can attach (see [TUI & Desktop from Worktrees](../developer-guide/worktree-ui-dev.md)). Ignored unless the build is unpackaged **and** `HERMES_DESKTOP_DEV_SERVER` is set — a packaged app never opens it. Anything that can reach the port can execute code in the renderer, so leave it unset unless you're actively debugging. | +| `HERMES_DESKTOP_CDP_PORT` | Overrides the Chrome DevTools Protocol port the renderer exposes on `127.0.0.1` for DOM/CSS inspection tooling (default `9222`). Dev-server runs (`npm run dev`, `hgui`) open it automatically; a packaged app never does, and no value here changes that. Set to `off` to disable it on a dev run. Anything that can reach the port can execute code in the renderer. | ### Microsoft Graph (Teams Meetings)