From c064487dbebe1fd4080c3b3a840d854b02faed00 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 22 Jul 2026 10:27:50 -0500 Subject: [PATCH] fix(desktop): guard against compiled .js shadowing .ts renderer sources A stray tsc run can emit foo.js next to foo.ts under apps/shared/src or apps/desktop/src. .gitignore hides the artifact from git status, but Vite resolves extensionless imports .js-before-.ts, so the renderer silently runs the stale compiled copy. This bit for real: a Jul 16 artifact of websocket-url.js predated the #68250 getGatewayWsUrl contract change ({ ok, wsUrl } IPC result), so its old 'if (fresh) return fresh' handed the whole result object to new WebSocket(), dialing ws://127.0.0.1:5174/[object%20Object] on every boot. The desktop app could never connect, and the failure survived reboots and cache wipes because the poison lived in src/. Two layers: - scripts/assert-no-stale-js.mjs runs before vite in dev:renderer and at the head of build; it fails loud with the exact shadowing files and the rm command when a .js sits next to a sibling .ts/.tsx in either src tree. - JsonRpcGatewayClient.connect() now rejects non-ws:// URLs with a readable error instead of letting new WebSocket() coerce an object into [object%20Object], so any future contract skew fails diagnosably. --- apps/desktop/package.json | 4 +- apps/desktop/scripts/assert-no-stale-js.mjs | 91 +++++++++++++++++++ .../scripts/assert-no-stale-js.test.mjs | 85 +++++++++++++++++ .../lib/json-rpc-gateway-url-guard.test.ts | 65 +++++++++++++ apps/shared/src/json-rpc-gateway.ts | 12 +++ 5 files changed, 255 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/scripts/assert-no-stale-js.mjs create mode 100644 apps/desktop/scripts/assert-no-stale-js.test.mjs create mode 100644 apps/desktop/src/lib/json-rpc-gateway-url-guard.test.ts diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 743269c10476..78ccbdb8aec3 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -14,13 +14,13 @@ "dev": "concurrently -k \"npm:dev:renderer\" \"npm:dev:electron\"", "dev:fake-boot": "cross-env HERMES_DESKTOP_BOOT_FAKE=1 HERMES_DESKTOP_BOOT_FAKE_STEP_MS=650 npm run dev", "dev:mock": "node scripts/dev-mock.mjs", - "dev:renderer": "node scripts/assert-root-install.mjs && vite --host 127.0.0.1 --port 5174", + "dev:renderer": "node scripts/assert-root-install.mjs && node scripts/assert-no-stale-js.mjs && vite --host 127.0.0.1 --port 5174", "dev:electron": "wait-on http://127.0.0.1:5174 && node scripts/bundle-electron-main.mjs --dev && cross-env XCURSOR_SIZE=24 HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron .", "profile:main": "wait-on http://127.0.0.1:5174 && node scripts/bundle-electron-main.mjs --dev && cross-env XCURSOR_SIZE=24 HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron --inspect=9229 .", "profile:main:cpu": "wait-on http://127.0.0.1:5174 && node scripts/bundle-electron-main.mjs --dev && cross-env XCURSOR_SIZE=24 NODE_OPTIONS=--cpu-prof HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron .", "start": "npm run build && electron .", "prebuild": "tsc -b . --clean", - "build": "node scripts/assert-root-install.mjs && node scripts/write-build-stamp.mjs && vite build && node scripts/bundle-electron-main.mjs && node scripts/stage-native-deps.mjs", + "build": "node scripts/assert-root-install.mjs && node scripts/assert-no-stale-js.mjs && node scripts/write-build-stamp.mjs && vite build && node scripts/bundle-electron-main.mjs && node scripts/stage-native-deps.mjs", "postbuild": "node scripts/assert-dist-built.mjs", "prebuilder": "node scripts/patch-electron-builder-mac-binary.mjs", "builder": "cross-env NODE_OPTIONS=--max-old-space-size=16384 node scripts/run-electron-builder.mjs", diff --git a/apps/desktop/scripts/assert-no-stale-js.mjs b/apps/desktop/scripts/assert-no-stale-js.mjs new file mode 100644 index 000000000000..19e1f22fd23a --- /dev/null +++ b/apps/desktop/scripts/assert-no-stale-js.mjs @@ -0,0 +1,91 @@ +// Dev/build-time guard: refuse to run against compiled .js files shadowing +// .ts/.tsx sources in the renderer source trees. +// +// A stray `tsc` invocation (no project file, editor compile-on-save, etc.) +// can emit `foo.js` next to `foo.ts` under apps/shared/src or +// apps/desktop/src. `.gitignore` already hides those artifacts from git, +// which makes them invisible in `git status` — but Vite resolves +// extensionless imports with `.js` BEFORE `.ts`, so the renderer silently +// runs the stale compiled copy instead of the real source. +// +// This bit hard in July 2026: a Jul 16 artifact of websocket-url.js predated +// the #68250 getGatewayWsUrl contract change, so every boot dialed +// "ws:///[object%20Object]" and the desktop app could never connect, +// surviving reboots and cache wipes because the poison lived in src/. +// +// Runs before `vite` in dev and at the head of `build`. Fails loud with the +// exact files and the fix instead of letting the app boot on stale code. + +import { existsSync, readdirSync, statSync } from "fs" +import { join, resolve } from "path" +import { isMain } from "./utils.mjs" + +// Pure scan — returns { ok: true } or { ok: false, stale: ["/abs/foo.js", ...] }. +// A .js file is stale when a sibling .ts or .tsx source exists. Standalone +// .js files (fixtures, configs) are left alone. +export function checkNoStaleJs(srcDirs) { + const stale = [] + + const walk = dir => { + if (!existsSync(dir) || !statSync(dir).isDirectory()) { + return + } + + for (const name of readdirSync(dir)) { + const full = join(dir, name) + + if (statSync(full).isDirectory()) { + if (name !== "node_modules") { + walk(full) + } + + continue + } + + if (!name.endsWith(".js")) { + continue + } + + const base = full.slice(0, -".js".length) + + if (existsSync(`${base}.ts`) || existsSync(`${base}.tsx`)) { + stale.push(full) + } + } + } + + for (const dir of srcDirs) { + walk(dir) + } + + return stale.length ? { ok: false, stale } : { ok: true } +} + +function main() { + const desktopRoot = resolve(import.meta.dirname, "..") + const result = checkNoStaleJs([ + join(desktopRoot, "src"), + resolve(desktopRoot, "..", "shared", "src") + ]) + + if (!result.ok) { + console.error("\n✗ assert-no-stale-js: compiled .js artifacts are shadowing .ts sources:") + + for (const file of result.stale) { + console.error(` ${file}`) + } + + console.error(" Vite resolves .js before .ts, so the app would run these stale") + console.error(" compiled copies instead of the real sources. Delete them:") + console.error(` rm ${result.stale.join(" ")}\n`) + process.exit(1) + } + + console.log("✓ assert-no-stale-js: no compiled .js shadowing .ts sources") +} + +if (isMain(import.meta.url)) { + main() +} + +export default { checkNoStaleJs } diff --git a/apps/desktop/scripts/assert-no-stale-js.test.mjs b/apps/desktop/scripts/assert-no-stale-js.test.mjs new file mode 100644 index 000000000000..5acbe6c56809 --- /dev/null +++ b/apps/desktop/scripts/assert-no-stale-js.test.mjs @@ -0,0 +1,85 @@ +import assert from 'node:assert/strict' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { test } from 'vitest' + +import { checkNoStaleJs } from '../scripts/assert-no-stale-js.mjs' + +function makeSrcTree(build) { + const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-stale-js-')) + const srcDir = path.join(tempRoot, 'src') + fs.mkdirSync(srcDir, { recursive: true }) + if (build) build(srcDir) + return { tempRoot, srcDir } +} + +test('passes on a clean tree of .ts sources', () => { + const { tempRoot, srcDir } = makeSrcTree(d => { + fs.writeFileSync(path.join(d, 'websocket-url.ts'), 'export {}', 'utf8') + fs.writeFileSync(path.join(d, 'index.ts'), 'export {}', 'utf8') + }) + try { + assert.deepEqual(checkNoStaleJs([srcDir]), { ok: true }) + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }) + } +}) + +test('flags a compiled .js shadowing a sibling .ts (the #68250 incident shape)', () => { + const { tempRoot, srcDir } = makeSrcTree(d => { + fs.writeFileSync(path.join(d, 'websocket-url.ts'), 'export {}', 'utf8') + fs.writeFileSync(path.join(d, 'websocket-url.js'), 'export {}', 'utf8') + }) + try { + const result = checkNoStaleJs([srcDir]) + assert.equal(result.ok, false) + assert.equal(result.stale.length, 1) + assert.match(result.stale[0], /websocket-url\.js$/) + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }) + } +}) + +test('flags a .js shadowing a .tsx source and recurses into subdirectories', () => { + const { tempRoot, srcDir } = makeSrcTree(d => { + const nested = path.join(d, 'app', 'chat') + fs.mkdirSync(nested, { recursive: true }) + fs.writeFileSync(path.join(nested, 'view.tsx'), 'export {}', 'utf8') + fs.writeFileSync(path.join(nested, 'view.js'), 'export {}', 'utf8') + }) + try { + const result = checkNoStaleJs([srcDir]) + assert.equal(result.ok, false) + assert.match(result.stale[0], /app[/\\]chat[/\\]view\.js$/) + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }) + } +}) + +test('leaves standalone .js files (no sibling source) alone', () => { + const { tempRoot, srcDir } = makeSrcTree(d => { + fs.writeFileSync(path.join(d, 'fixture-data.js'), 'export {}', 'utf8') + fs.writeFileSync(path.join(d, 'index.ts'), 'export {}', 'utf8') + }) + try { + assert.deepEqual(checkNoStaleJs([srcDir]), { ok: true }) + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }) + } +}) + +test('skips node_modules and tolerates missing directories', () => { + const { tempRoot, srcDir } = makeSrcTree(d => { + const nm = path.join(d, 'node_modules', 'pkg') + fs.mkdirSync(nm, { recursive: true }) + fs.writeFileSync(path.join(nm, 'mod.ts'), 'export {}', 'utf8') + fs.writeFileSync(path.join(nm, 'mod.js'), 'export {}', 'utf8') + }) + try { + assert.deepEqual(checkNoStaleJs([srcDir, path.join(tempRoot, 'does-not-exist')]), { ok: true }) + } finally { + fs.rmSync(tempRoot, { recursive: true, force: true }) + } +}) diff --git a/apps/desktop/src/lib/json-rpc-gateway-url-guard.test.ts b/apps/desktop/src/lib/json-rpc-gateway-url-guard.test.ts new file mode 100644 index 000000000000..16c2a141fc01 --- /dev/null +++ b/apps/desktop/src/lib/json-rpc-gateway-url-guard.test.ts @@ -0,0 +1,65 @@ +// connect() must reject malformed WS URLs loudly instead of letting +// `new WebSocket()` coerce them. The failure this guards against is real: +// a stale compiled websocket-url.js (predating the #68250 { ok, wsUrl } +// IPC contract) returned the whole result object, and the browser dialed +// "ws:///[object%20Object]" — an opaque, unfixable-looking +// "Could not connect to Hermes gateway" boot loop. + +import { JsonRpcGatewayClient } from '@hermes/shared' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +class FakeSocket { + static OPEN = 1 + readyState = 0 + addEventListener = vi.fn((type: string, handler: () => void) => { + if (type === 'open') { + // Land the handshake asynchronously like a real socket. + setTimeout(() => { + this.readyState = FakeSocket.OPEN + handler() + }, 0) + } + }) + removeEventListener = vi.fn() + close = vi.fn() + send = vi.fn() +} + +describe('JsonRpcGatewayClient connect() URL guard', () => { + beforeEach(() => { + // jsdom has no WebSocket; the class reads WebSocket.OPEN when a socket exists. + vi.stubGlobal('WebSocket', FakeSocket) + }) + + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('rejects a non-string (an IPC result object passed through whole)', async () => { + const client = new JsonRpcGatewayClient() + await expect( + client.connect({ ok: true, wsUrl: 'ws://127.0.0.1:1/api/ws' } as unknown as string) + ).rejects.toThrow(/requires a ws:\/\/ or wss:\/\/ URL string, got type "object"/) + }) + + it('rejects a string that is not a ws:// or wss:// URL', async () => { + const client = new JsonRpcGatewayClient() + await expect(client.connect('http://127.0.0.1:1234/api/ws')).rejects.toThrow( + /requires a ws:\/\/ or wss:\/\/ URL string/ + ) + }) + + it('does not flip connection state when rejecting a malformed URL', async () => { + const client = new JsonRpcGatewayClient() + await client.connect(undefined as unknown as string).catch(() => undefined) + expect(client.connectionState).toBe('idle') + }) + + it('accepts ws:// and wss:// URL strings', async () => { + for (const url of ['ws://127.0.0.1:1234/api/ws?token=t', 'wss://gw.example.com/api/ws?ticket=t']) { + const client = new JsonRpcGatewayClient({ socketFactory: () => new FakeSocket() as unknown as WebSocket }) + await client.connect(url) + expect(client.connectionState).toBe('open') + } + }) +}) diff --git a/apps/shared/src/json-rpc-gateway.ts b/apps/shared/src/json-rpc-gateway.ts index cff91305f27f..67e48ba98398 100644 --- a/apps/shared/src/json-rpc-gateway.ts +++ b/apps/shared/src/json-rpc-gateway.ts @@ -95,6 +95,18 @@ export class JsonRpcGatewayClient { } async connect(wsUrl: string): Promise { + // Fail loudly on a malformed URL instead of letting `new WebSocket()` + // coerce it. A non-string here means an upstream contract skew (e.g. an + // IPC result object passed through whole), which otherwise surfaces as an + // opaque dial to "ws:///[object%20Object]" — a real incident: + // a stale compiled copy of websocket-url.js shadowed the .ts source and + // returned the raw `{ ok, wsUrl }` IPC result after #68250 changed the + // getGatewayWsUrl contract. + if (typeof wsUrl !== 'string' || !/^wss?:\/\//i.test(wsUrl)) { + const got = typeof wsUrl === 'string' ? JSON.stringify(wsUrl) : `type "${typeof wsUrl}"` + throw new Error(`gateway connect() requires a ws:// or wss:// URL string, got ${got}`) + } + if (this.socket?.readyState === WebSocket.OPEN || this.state === 'connecting') { return }