From 2f3007ff5145070fe646e4dc6f89ad38e4478e42 Mon Sep 17 00:00:00 2001 From: ethernet Date: Wed, 15 Jul 2026 12:14:10 -0400 Subject: [PATCH] test: port JS/package.json invariant tests from Python to vitest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI change classifier routes package.json / package-lock.json / .ts/.tsx changes to the frontend lane, not the Python lane. Four Python tests asserted about these JS-side artifacts, so a PR touching only those files would skip the Python suite — the regression goes green on the PR and red on main (where the classifier fails open). Ported all four to vitest so they run in the correct CI lane: tests/test_package_json_lazy_deps.py → apps/desktop/electron/package-json-lazy-deps.test.ts (camofox is lazy, agent-browser is eager, lockfile clean) tests/test_desktop_electron_pin.py → apps/desktop/electron/desktop-electron-pin.test.ts (electron dep is exact, matches build.electronVersion, lockfile agrees) tests/test_assistant_ui_tap_compat.py → apps/desktop/electron/assistant-ui-tap-compat.test.ts (@assistant-ui cluster shares one tap version + semver helper) tests/test_dashboard_sidecar_close_on_disconnect.py → web/src/lib/chat-sidebar-session-params.test.ts (sidecar session.create opts into close_on_disconnect + profile) The ChatSidebar test was regex-matching .tsx source text (the source-reading anti-pattern). Extracted sidecarSessionCreateParams() from the component's effect into an exported pure function so the test calls real code instead of pattern-matching a string. Verified: 8 electron tests + 76 web tests pass; both typecheck clean. --- .../electron/assistant-ui-tap-compat.test.ts | 154 +++++++++++++++++ .../electron/desktop-electron-pin.test.ts | 98 +++++++++++ .../electron/package-json-lazy-deps.test.ts | 88 ++++++++++ tests/test_assistant_ui_tap_compat.py | 159 ------------------ ...t_dashboard_sidecar_close_on_disconnect.py | 25 --- tests/test_desktop_electron_pin.py | 96 ----------- tests/test_package_json_lazy_deps.py | 85 ---------- web/src/components/ChatSidebar.tsx | 21 ++- .../lib/chat-sidebar-session-params.test.ts | 35 ++++ 9 files changed, 391 insertions(+), 370 deletions(-) create mode 100644 apps/desktop/electron/assistant-ui-tap-compat.test.ts create mode 100644 apps/desktop/electron/desktop-electron-pin.test.ts create mode 100644 apps/desktop/electron/package-json-lazy-deps.test.ts delete mode 100644 tests/test_assistant_ui_tap_compat.py delete mode 100644 tests/test_dashboard_sidecar_close_on_disconnect.py delete mode 100644 tests/test_desktop_electron_pin.py delete mode 100644 tests/test_package_json_lazy_deps.py create mode 100644 web/src/lib/chat-sidebar-session-params.test.ts diff --git a/apps/desktop/electron/assistant-ui-tap-compat.test.ts b/apps/desktop/electron/assistant-ui-tap-compat.test.ts new file mode 100644 index 000000000..afbca904e --- /dev/null +++ b/apps/desktop/electron/assistant-ui-tap-compat.test.ts @@ -0,0 +1,154 @@ +/** + * Invariant: the @assistant-ui dependency cluster agrees on one tap version. + * + * The Hermes desktop app (``apps/desktop``) is built from source on every + * install/update via ``scripts/install.ps1`` → ``npm ci``/``npm install`` → + * ``tsc -b && vite build``. The ``@assistant-ui`` packages share an internal + * reactivity lib, ``@assistant-ui/tap``, and they only interoperate when they + * all resolve the *same* tap version: + * + * - ``@assistant-ui/react@0.12.28`` and ``@assistant-ui/core`` pin + * ``@assistant-ui/tap@^0.5.x`` (which exports ``.`` and ``./react``). + * - ``@assistant-ui/store@0.2.18`` bumped its tap peer to ``^0.9.0`` and started + * importing ``@assistant-ui/tap/react-shim`` — an entry point that only exists + * in the tap ``0.9.x`` line. + * + * Because ``react@0.12.28`` requests ``store@^0.2.9`` (a caret range), a fresh + * install silently floated ``store`` up to ``0.2.18``, which then could not find + * ``./react-shim`` in the hoisted ``tap@0.5.x`` and crashed ``vite build`` with: + * + * "./react-shim" is not exported ... from package @assistant-ui/tap + * + * i.e. the opaque "apps/desktop build failed (exit 1)" every user hit when + * updating. The fix pins ``@assistant-ui/store`` (via root ``overrides``) to the + * last release that targets ``tap@^0.5.x``. + * + * This is a *contract* test, not a snapshot: it does not assert specific version + * numbers, only that the cluster resolves a single shared tap (wherever npm + * places it — hoisted to root, or nested under the ``apps/desktop`` workspace + * since the 0.14 bump dropped the ``store`` override) and that this tap satisfies + * every ``@assistant-ui/*`` package's declared requirement. It fails if any + * future bump reintroduces a split tap version or requirement across the cluster. + */ + +import assert from 'node:assert/strict' +import fs from 'node:fs' +import path from 'node:path' + +import { test } from 'vitest' + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..') +const LOCK_PATH = path.join(REPO_ROOT, 'package-lock.json') +const TAP = '@assistant-ui/tap' + +/** + * Minimal npm semver check for the ranges this cluster actually uses. + * + * Supports exact versions, ``^x.y.z`` (with correct 0.x semantics), and + * ``||`` unions. Pre-release tags are ignored (none are used here). + */ +function caretSatisfies(version: string, spec: string): boolean { + function parse(v: string): [number, number, number] { + const core = v.replace(/^[^0-9]+/, '').split('-')[0].split('+')[0] + const parts = core.split('.').slice(0, 3) + while (parts.length < 3) parts.push('0') + return [parseInt(parts[0], 10), parseInt(parts[1], 10), parseInt(parts[2], 10)] + } + + const ver = parse(version) + for (const clause of spec.split('||')) { + const trimmed = clause.trim() + if (!trimmed) continue + if (trimmed.startsWith('^')) { + const lo = parse(trimmed) + if (ver[0] < lo[0] || (ver[0] === lo[0] && ver[1] < lo[1]) || (ver[0] === lo[0] && ver[1] === lo[1] && ver[2] < lo[2])) continue + let hi: [number, number, number] + if (lo[0] > 0) { + hi = [lo[0] + 1, 0, 0] + } else if (lo[1] > 0) { + hi = [0, lo[1] + 1, 0] + } else { + hi = [0, 0, lo[2] + 1] + } + if ( + (ver[0] < hi[0]) || + (ver[0] === hi[0] && ver[1] < hi[1]) || + (ver[0] === hi[0] && ver[1] === hi[1] && ver[2] < hi[2]) + ) { + return true + } + } else if (trimmed[0].match(/\d/) || trimmed.startsWith('v')) { + if (ver[0] === parse(trimmed)[0] && ver[1] === parse(trimmed)[1] && ver[2] === parse(trimmed)[2]) { + return true + } + } + } + return false +} + +interface LockPackage { + version?: string + dependencies?: Record + peerDependencies?: Record + peerDependenciesMeta?: Record +} + +function lockPackages(): Record { + const lock = JSON.parse(fs.readFileSync(LOCK_PATH, 'utf-8')) + return (lock.packages ?? {}) as Record +} + +function sharedTapVersion(packages: Record): string { + /** The one tap version every install site resolves to. */ + const versions = new Set() + for (const [key, meta] of Object.entries(packages)) { + const idx = key.lastIndexOf('node_modules/') + const name = idx >= 0 ? key.slice(idx + 'node_modules/'.length) : key + if (name === TAP) { + if (meta.version) versions.add(meta.version) + } + } + assert.ok(versions.size > 0, 'package-lock.json has no @assistant-ui/tap entry — the @assistant-ui cluster should resolve a single shared tap version.') + assert.ok(versions.size === 1, `@assistant-ui/tap resolves to multiple versions ${[...versions].sort()} — the cluster must share one tap line (see this test's docstring).`) + return [...versions][0]! +} + +test('every @assistant-ui/* package\'s tap requirement is satisfiable', () => { + const packages = lockPackages() + + const tapVersion = sharedTapVersion(packages) + + const offenders: string[] = [] + for (const [key, meta] of Object.entries(packages)) { + const idx = key.lastIndexOf('node_modules/') + const name = idx >= 0 ? key.slice(idx + 'node_modules/'.length) : key + if (!name.startsWith('@assistant-ui/') || name === TAP) continue + const peerMeta = (meta.peerDependenciesMeta ?? {})[TAP] + if (peerMeta?.optional) continue + const spec = (meta.dependencies ?? {})[TAP] || (meta.peerDependencies ?? {})[TAP] + if (!spec) continue + if (!caretSatisfies(tapVersion, spec)) { + offenders.push(`${name} requires ${TAP}"${spec}"`) + } + } + + assert.deepEqual( + offenders, + [], + `Hoisted ${TAP}@${tapVersion} does not satisfy: ` + + offenders.join('; ') + + '. The @assistant-ui cluster has split tap requirements — pin the ' + + 'offending package (e.g. via root package.json `overrides`) so the ' + + 'whole cluster shares one tap line. See this test\'s module docstring.' + ) +}) + +test('caretSatisfies helper', () => { + assert.ok(caretSatisfies('0.5.14', '^0.5.10')) + assert.ok(caretSatisfies('0.5.14', '^0.5.14')) + assert.ok(!caretSatisfies('0.5.14', '^0.9.0')) + assert.ok(!caretSatisfies('0.5.14', '^0.6.0')) + assert.ok(caretSatisfies('1.2.5', '^1.2.0')) + assert.ok(!caretSatisfies('2.0.0', '^1.2.0')) + assert.ok(caretSatisfies('0.5.14', '^0.5.0 || ^0.9.0')) +}) diff --git a/apps/desktop/electron/desktop-electron-pin.test.ts b/apps/desktop/electron/desktop-electron-pin.test.ts new file mode 100644 index 000000000..d94ab570c --- /dev/null +++ b/apps/desktop/electron/desktop-electron-pin.test.ts @@ -0,0 +1,98 @@ +/** + * Regression: the desktop Electron dependency must be an exact, consistent pin. + * + * The Windows desktop install failed at "Building desktop app" because Electron + * changed its install mechanism mid patch-series: + * + * electron 40.9.3 .. 40.10.2 -> @electron/get@^2 + extract-zip@^2 (pure JS) + * electron 40.10.3 / 40.10.4 -> @electron/get@^5 + + * @electron-internal/extract-zip@^1 (native napi) + * + * ``apps/desktop/package.json`` declared ``electronVersion: 40.9.3`` (the tested, + * JS-extract build) but pinned the dependency loosely as ``electron: ^40.9.3``. + * ``npm ci`` then resolved 40.10.3/40.10.4 — the new *native* extract-zip whose + * win32-x64 binding fails to ``dlopen`` on some Windows hosts + * (``ERR_DLOPEN_FAILED loading index.win32-x64-msvc.node``). + * + * These tests lock the contract that prevents that drift, without hard-coding the + * specific version (which is allowed to move): + * + * 1. the Electron dependency is an *exact* version (Electron Builder needs the + * installed binary to match ``electronVersion`` / ``electronDist``), and + * 2. the dependency, ``build.electronVersion``, and the resolved lockfile entry + * all agree — so ``npm ci`` installs exactly what the build packages. + */ + +import assert from 'node:assert/strict' +import fs from 'node:fs' +import path from 'node:path' + +import { test } from 'vitest' + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..') +const DESKTOP_PKG = path.join(REPO_ROOT, 'apps', 'desktop', 'package.json') +const ROOT_LOCK = path.join(REPO_ROOT, 'package-lock.json') + +// An exact semver: digits.digits.digits with an optional prerelease/build tag, +// but NO range operators (^ ~ > < = * x || spaces || -range). +const EXACT_SEMVER = /^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/ + +function desktopPkg(): Record { + assert.ok(fs.existsSync(DESKTOP_PKG), `missing ${DESKTOP_PKG}`) + return JSON.parse(fs.readFileSync(DESKTOP_PKG, 'utf-8')) +} + +function electronSpec(pkg: Record): string { + for (const section of ['dependencies', 'devDependencies'] as const) { + const deps = (pkg[section] ?? {}) as Record + const spec = deps['electron'] + if (spec) return spec + } + assert.fail('electron is not listed in apps/desktop dependencies') +} + +test('electron dependency is exactly pinned', () => { + const spec = electronSpec(desktopPkg()) + assert.match( + spec, + EXACT_SEMVER, + `electron must be pinned to an exact version, got "${spec}". ` + + 'A range (^/~) lets npm ci resolve a newer Electron whose postinstall ' + + 'may differ from the one the build was validated against.' + ) +}) + +test('electron dependency matches build.electronVersion', () => { + const pkg = desktopPkg() + const spec = electronSpec(pkg) + const build = (pkg.build ?? {}) as Record + const builderVersion = build.electronVersion as string | undefined + assert.ok(builderVersion, 'build.electronVersion is missing') + assert.equal( + spec, + builderVersion, + `electron dependency ("${spec}") must equal build.electronVersion ` + + `("${builderVersion}"); otherwise electron-builder packages a different ` + + 'version than npm installs into electronDist.' + ) +}) + +test('lockfile resolves the pinned electron', () => { + if (!fs.existsSync(ROOT_LOCK)) return // skip if lockfile not present + const spec = electronSpec(desktopPkg()) + const lock = JSON.parse(fs.readFileSync(ROOT_LOCK, 'utf-8')) + const packages = (lock.packages ?? {}) as Record + const resolved = Object.entries(packages) + .filter(([key]) => key.endsWith('node_modules/electron')) + .map(([, meta]) => meta.version) + .filter((v): v is string => !!v) + assert.ok(resolved.length > 0, 'no electron entry found in package-lock.json') + for (const v of resolved) { + assert.equal( + v, + spec, + `package-lock.json resolves electron to ${v}, but the pin is "${spec}"; ` + + 'run `npm install --package-lock-only` so `npm ci` stays consistent.' + ) + } +}) diff --git a/apps/desktop/electron/package-json-lazy-deps.test.ts b/apps/desktop/electron/package-json-lazy-deps.test.ts new file mode 100644 index 000000000..b0c80c77e --- /dev/null +++ b/apps/desktop/electron/package-json-lazy-deps.test.ts @@ -0,0 +1,88 @@ +/** + * Invariants for what is eager vs lazy in the root ``package.json``. + * + * The root ``package.json`` is installed by ``hermes update`` on every user, + * including users who never opted into a given browser backend. Anything + * listed in ``dependencies`` therefore runs its npm postinstall script for + * everyone — including binary-fetching backends, on every update. + * + * The contract: + * + * - ``agent-browser`` IS eager. It is the default Chromium-driving backend + * used whenever the agent makes a browser call without a cloud provider + * configured, so it must already be installed before any session starts. + * Its postinstall is also small. + * + * - ``@askjo/camofox-browser`` is NOT eager. It is an explicit opt-in + * alternative browser backend, selected by the user via + * ``hermes tools`` → Browser Automation → Camofox, and only used at + * runtime when ``CAMOFOX_URL`` is set. Its postinstall fetches a ~300MB + * Firefox-fork binary, which silently blocked ``hermes update`` for + * multi-minute stretches on slow / network-restricted connections + * (notably users in China running through a VPN). The package is + * installed on demand by ``tools_config.py`` ``post_setup_key == + * "camofox"`` when the user actually selects Camofox. + * + * If a future PR re-adds Camofox (or any other binary-postinstall package) + * to root ``dependencies``, this test fails — read the lazy-install + * guidance in the ``hermes-agent-dev`` skill before changing the + * expectations. + */ + +import assert from 'node:assert/strict' +import fs from 'node:fs' +import path from 'node:path' + +import { test } from 'vitest' + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..') +const ROOT_PKG = path.join(REPO_ROOT, 'package.json') +const ROOT_LOCK = path.join(REPO_ROOT, 'package-lock.json') + +function rootPackageJson(): Record { + return JSON.parse(fs.readFileSync(ROOT_PKG, 'utf-8')) +} + +test('camofox is not in root dependencies (must stay opt-in)', () => { + const deps = (rootPackageJson().dependencies ?? {}) as Record + assert.ok( + !('@askjo/camofox-browser' in deps), + 'Camofox is a ~300MB binary-postinstall backend that must stay ' + + 'out of root package.json dependencies. It belongs in the ' + + 'Camofox post_setup handler in hermes_cli/tools_config.py so it ' + + 'only installs when the user explicitly selects Camofox via ' + + '`hermes tools` → Browser Automation → Camofox.' + ) +}) + +test('agent-browser stays eager (default backend)', () => { + const deps = (rootPackageJson().dependencies ?? {}) as Record + assert.ok( + 'agent-browser' in deps, + 'agent-browser is the default browser-tool backend used by every ' + + 'session that doesn\'t have a cloud browser provider configured. ' + + 'It must stay in root package.json dependencies so it is present ' + + 'after `hermes setup` / `hermes update` without an explicit ' + + 'post_setup step.' + ) +}) + +test('root lockfile has no camofox entries', () => { + if (!fs.existsSync(ROOT_LOCK)) { + // Some CI matrix shards skip lockfile materialization. + return + } + const text = fs.readFileSync(ROOT_LOCK, 'utf-8') + assert.ok( + !text.includes('@askjo/camofox-browser'), + 'package-lock.json still references @askjo/camofox-browser. ' + + 'Regenerate the lockfile after removing the dep: ' + + '`rm package-lock.json && npm install --package-lock-only ' + + '--ignore-scripts --no-fund --no-audit`.' + ) + assert.ok( + !text.includes('camoufox-js'), + 'package-lock.json still references camoufox-js (transitive of ' + + '@askjo/camofox-browser). Regenerate the lockfile.' + ) +}) diff --git a/tests/test_assistant_ui_tap_compat.py b/tests/test_assistant_ui_tap_compat.py deleted file mode 100644 index 4be17141c..000000000 --- a/tests/test_assistant_ui_tap_compat.py +++ /dev/null @@ -1,159 +0,0 @@ -"""Invariant: the @assistant-ui dependency cluster agrees on one tap version. - -The Hermes desktop app (``apps/desktop``) is built from source on every -install/update via ``scripts/install.ps1`` → ``npm ci``/``npm install`` → -``tsc -b && vite build``. The ``@assistant-ui`` packages share an internal -reactivity lib, ``@assistant-ui/tap``, and they only interoperate when they -all resolve the *same* tap version: - -* ``@assistant-ui/react@0.12.28`` and ``@assistant-ui/core`` pin - ``@assistant-ui/tap@^0.5.x`` (which exports ``.`` and ``./react``). -* ``@assistant-ui/store@0.2.18`` bumped its tap peer to ``^0.9.0`` and started - importing ``@assistant-ui/tap/react-shim`` — an entry point that only exists - in the tap ``0.9.x`` line. - -Because ``react@0.12.28`` requests ``store@^0.2.9`` (a caret range), a fresh -install silently floated ``store`` up to ``0.2.18``, which then could not find -``./react-shim`` in the hoisted ``tap@0.5.x`` and crashed ``vite build`` with:: - - "./react-shim" is not exported ... from package @assistant-ui/tap - -i.e. the opaque "apps/desktop build failed (exit 1)" every user hit when -updating. The fix pins ``@assistant-ui/store`` (via root ``overrides``) to the -last release that targets ``tap@^0.5.x``. - -This is a *contract* test, not a snapshot: it does not assert specific version -numbers, only that the cluster resolves a single shared tap (wherever npm -places it — hoisted to root, or nested under the ``apps/desktop`` workspace -since the 0.14 bump dropped the ``store`` override) and that this tap satisfies -every ``@assistant-ui/*`` package's declared requirement. It fails if any -future bump reintroduces a split tap version or requirement across the cluster. -""" - -from __future__ import annotations - -import json -from pathlib import Path - -import pytest - - -REPO_ROOT = Path(__file__).resolve().parent.parent -TAP = "@assistant-ui/tap" - - -def _caret_satisfies(version: str, spec: str) -> bool: - """Minimal npm semver check for the ranges this cluster actually uses. - - Supports exact versions, ``^x.y.z`` (with correct 0.x semantics), and - ``||`` unions. Pre-release tags are ignored (none are used here). - """ - - def parse(v: str) -> tuple[int, int, int]: - core = v.lstrip("^~>= 0: - hi = (major + 1, 0, 0) - elif minor > 0: - hi = (0, minor + 1, 0) - else: - hi = (0, 0, lo[2] + 1) - if ver < hi: - return True - elif clause[0].isdigit() or clause.startswith("v"): - if ver == parse(clause): - return True - return False - - -def _lock_packages() -> dict: - lock_path = REPO_ROOT / "package-lock.json" - if not lock_path.exists(): - pytest.skip("package-lock.json not materialized in this CI shard") - with lock_path.open("r", encoding="utf-8") as fh: - return json.load(fh).get("packages", {}) - - -def _shared_tap_version(packages: dict) -> str: - """The one tap version every install site resolves to. - - npm hoists tap to root ``node_modules`` when the cluster lives at the top - level, but nests the whole cluster under a workspace - (``apps/desktop/node_modules``) when only that workspace depends on it — as - it does since the 0.14 bump dropped the ``@assistant-ui/store`` override. - Either layout is fine; the invariant is that a single tap version is shared - across every install site, so ``vite build`` never hits a split API. - """ - versions = { - meta["version"] - for key, meta in packages.items() - if key.rsplit("node_modules/", 1)[-1] == TAP - } - assert versions, ( - "package-lock.json has no @assistant-ui/tap entry — the " - "@assistant-ui cluster should resolve a single shared tap version." - ) - assert len(versions) == 1, ( - f"@assistant-ui/tap resolves to multiple versions {sorted(versions)} — " - "the cluster must share one tap line (see this test's docstring)." - ) - return versions.pop() - - -def test_assistant_ui_cluster_agrees_on_one_tap() -> None: - """Every @assistant-ui/* package's tap requirement must be satisfiable. - - Encodes the contract that broke the desktop build: a single hoisted - @assistant-ui/tap must satisfy the tap range declared by react, core, - store, and any sibling — otherwise the missing ``./react-shim`` export - (or a similar API split) breaks ``vite build``. - """ - packages = _lock_packages() - tap_version = _shared_tap_version(packages) - - offenders: list[str] = [] - for key, meta in packages.items(): - name = key.rsplit("node_modules/", 1)[-1] - if not name.startswith("@assistant-ui/") or name == TAP: - continue - peer_meta = meta.get("peerDependenciesMeta", {}).get(TAP, {}) - if peer_meta.get("optional"): - continue - spec = meta.get("dependencies", {}).get(TAP) or meta.get( - "peerDependencies", {} - ).get(TAP) - if not spec: - continue - if not _caret_satisfies(tap_version, spec): - offenders.append(f"{name} requires {TAP}{spec!r}") - - assert not offenders, ( - f"Hoisted {TAP}@{tap_version} does not satisfy: " - + "; ".join(offenders) - + ". The @assistant-ui cluster has split tap requirements — pin the " - "offending package (e.g. via root package.json `overrides`) so the " - "whole cluster shares one tap line. See this test's module docstring." - ) - - -def test_caret_satisfies_helper() -> None: - """Guard the tiny semver helper the invariant relies on.""" - assert _caret_satisfies("0.5.14", "^0.5.10") - assert _caret_satisfies("0.5.14", "^0.5.14") - assert not _caret_satisfies("0.5.14", "^0.9.0") - assert not _caret_satisfies("0.5.14", "^0.6.0") - assert _caret_satisfies("1.2.5", "^1.2.0") - assert not _caret_satisfies("2.0.0", "^1.2.0") - assert _caret_satisfies("0.5.14", "^0.5.0 || ^0.9.0") diff --git a/tests/test_dashboard_sidecar_close_on_disconnect.py b/tests/test_dashboard_sidecar_close_on_disconnect.py deleted file mode 100644 index b2eb33645..000000000 --- a/tests/test_dashboard_sidecar_close_on_disconnect.py +++ /dev/null @@ -1,25 +0,0 @@ -import re -from pathlib import Path - -CHAT_SIDEBAR = Path(__file__).resolve().parent.parent / "web/src/components/ChatSidebar.tsx" - - -def test_sidecar_session_create_requests_close_on_disconnect(): - """The sidecar must opt its session into close_on_disconnect so the gateway - reaps the slash_worker on WS disconnect (the #21370/#21467 leak).""" - source = CHAT_SIDEBAR.read_text(encoding="utf-8") - call = re.search(r'"session\.create",\s*\{(.*?)\}', source, re.DOTALL) - assert call, "sidecar session.create call not found" - assert re.search(r"close_on_disconnect:\s*true", call.group(1)) - - -def test_sidecar_session_create_scopes_profile(): - """The sidecar must pass the dashboard's selected profile so model/credential - info matches the PTY child under profile-scoped chat.""" - source = CHAT_SIDEBAR.read_text(encoding="utf-8") - call = re.search(r'"session\.create",\s*\{(.*?)\}\);', source, re.DOTALL) - assert call, "sidecar session.create call not found" - body = call.group(1) - assert re.search(r"close_on_disconnect:\s*true", body) - assert re.search(r'source:\s*"tool"', body) - assert re.search(r"\.\.\.\(profile\s*\?\s*\{\s*profile\s*\}\s*:\s*\{\}\)", body) diff --git a/tests/test_desktop_electron_pin.py b/tests/test_desktop_electron_pin.py deleted file mode 100644 index ff9f1554e..000000000 --- a/tests/test_desktop_electron_pin.py +++ /dev/null @@ -1,96 +0,0 @@ -"""Regression: the desktop Electron dependency must be an exact, consistent pin. - -The Windows desktop install failed at "Building desktop app" because Electron -changed its install mechanism mid patch-series: - - electron 40.9.3 .. 40.10.2 -> @electron/get@^2 + extract-zip@^2 (pure JS) - electron 40.10.3 / 40.10.4 -> @electron/get@^5 + - @electron-internal/extract-zip@^1 (native napi) - -``apps/desktop/package.json`` declared ``electronVersion: 40.9.3`` (the tested, -JS-extract build) but pinned the dependency loosely as ``electron: ^40.9.3``. -``npm ci`` then resolved 40.10.3/40.10.4 — the new *native* extract-zip whose -win32-x64 binding fails to ``dlopen`` on some Windows hosts -(``ERR_DLOPEN_FAILED loading index.win32-x64-msvc.node``). - -These tests lock the contract that prevents that drift, without hard-coding the -specific version (which is allowed to move): - -1. the Electron dependency is an *exact* version (Electron Builder needs the - installed binary to match ``electronVersion`` / ``electronDist``), and -2. the dependency, ``build.electronVersion``, and the resolved lockfile entry - all agree — so ``npm ci`` installs exactly what the build packages. -""" - -from __future__ import annotations - -import json -import re -from pathlib import Path - -import pytest - - -REPO_ROOT = Path(__file__).resolve().parent.parent -DESKTOP_PKG = REPO_ROOT / "apps" / "desktop" / "package.json" -ROOT_LOCK = REPO_ROOT / "package-lock.json" - -# An exact semver: digits.digits.digits with an optional prerelease/build tag, -# but NO range operators (^ ~ > < = * x || spaces || -range). -_EXACT_SEMVER = re.compile(r"^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$") - - -def _desktop_pkg() -> dict: - assert DESKTOP_PKG.is_file(), f"missing {DESKTOP_PKG}" - return json.loads(DESKTOP_PKG.read_text(encoding="utf-8")) - - -def _electron_spec(pkg: dict) -> str: - for section in ("dependencies", "devDependencies"): - spec = pkg.get(section, {}).get("electron") - if spec: - return spec - pytest.fail("electron is not listed in apps/desktop dependencies") - - -def test_electron_dependency_is_exactly_pinned(): - """A loose range lets npm drift onto an Electron with a different installer.""" - spec = _electron_spec(_desktop_pkg()) - assert _EXACT_SEMVER.match(spec), ( - f"electron must be pinned to an exact version, got {spec!r}. " - "A range (^/~) lets npm ci resolve a newer Electron whose postinstall " - "may differ from the one the build was validated against." - ) - - -def test_electron_dependency_matches_electron_version(): - """electron-builder packages build.electronVersion against the installed binary.""" - pkg = _desktop_pkg() - spec = _electron_spec(pkg) - builder_version = pkg.get("build", {}).get("electronVersion") - assert builder_version, "build.electronVersion is missing" - assert spec == builder_version, ( - f"electron dependency ({spec!r}) must equal build.electronVersion " - f"({builder_version!r}); otherwise electron-builder packages a different " - "version than npm installs into electronDist." - ) - - -def test_lockfile_resolves_the_pinned_electron(): - """npm ci installs from the lockfile, so it must agree with the pin.""" - if not ROOT_LOCK.is_file(): - pytest.skip("root package-lock.json not present") - spec = _electron_spec(_desktop_pkg()) - lock = json.loads(ROOT_LOCK.read_text(encoding="utf-8")) - packages = lock.get("packages", {}) - resolved = [ - meta.get("version") - for path, meta in packages.items() - if path.endswith("node_modules/electron") and meta.get("version") - ] - assert resolved, "no electron entry found in package-lock.json" - assert all(v == spec for v in resolved), ( - f"package-lock.json resolves electron to {sorted(set(resolved))}, " - f"but the pin is {spec!r}; run `npm install --package-lock-only` so " - "`npm ci` stays consistent." - ) \ No newline at end of file diff --git a/tests/test_package_json_lazy_deps.py b/tests/test_package_json_lazy_deps.py deleted file mode 100644 index 0e2456dba..000000000 --- a/tests/test_package_json_lazy_deps.py +++ /dev/null @@ -1,85 +0,0 @@ -"""Invariants for what is eager vs lazy in the root ``package.json``. - -The root ``package.json`` is installed by ``hermes update`` on every user, -including users who never opted into a given browser backend. Anything -listed in ``dependencies`` therefore runs its npm postinstall script for -everyone — including binary-fetching backends, on every update. - -The contract: - -* ``agent-browser`` IS eager. It is the default Chromium-driving backend - used whenever the agent makes a browser call without a cloud provider - configured, so it must already be installed before any session starts. - Its postinstall is also small. - -* ``@askjo/camofox-browser`` is NOT eager. It is an explicit opt-in - alternative browser backend, selected by the user via - ``hermes tools`` → Browser Automation → Camofox, and only used at - runtime when ``CAMOFOX_URL`` is set. Its postinstall fetches a ~300MB - Firefox-fork binary, which silently blocked ``hermes update`` for - multi-minute stretches on slow / network-restricted connections - (notably users in China running through a VPN). The package is - installed on demand by ``tools_config.py`` ``post_setup_key == - "camofox"`` when the user actually selects Camofox. - -If a future PR re-adds Camofox (or any other binary-postinstall package) -to root ``dependencies``, this test fails — read the lazy-install -guidance in the ``hermes-agent-dev`` skill before changing the -expectations. -""" - -from __future__ import annotations - -import json -from pathlib import Path - - -REPO_ROOT = Path(__file__).resolve().parent.parent - - -def _root_package_json() -> dict: - with (REPO_ROOT / "package.json").open("r", encoding="utf-8") as fh: - return json.load(fh) - - -def test_camofox_is_not_in_root_dependencies() -> None: - """Camofox must be opt-in, installed lazily by its post_setup handler.""" - deps = _root_package_json().get("dependencies", {}) - assert "@askjo/camofox-browser" not in deps, ( - "Camofox is a ~300MB binary-postinstall backend that must stay " - "out of root package.json dependencies. It belongs in the " - "Camofox post_setup handler in hermes_cli/tools_config.py so it " - "only installs when the user explicitly selects Camofox via " - "`hermes tools` → Browser Automation → Camofox." - ) - - -def test_agent_browser_stays_eager() -> None: - """agent-browser is the default backend; it must remain eager.""" - deps = _root_package_json().get("dependencies", {}) - assert "agent-browser" in deps, ( - "agent-browser is the default browser-tool backend used by every " - "session that doesn't have a cloud browser provider configured. " - "It must stay in root package.json dependencies so it is present " - "after `hermes setup` / `hermes update` without an explicit " - "post_setup step." - ) - - -def test_root_lockfile_has_no_camofox_entries() -> None: - """Regenerated lockfiles should not contain Camofox tree entries.""" - lock_path = REPO_ROOT / "package-lock.json" - if not lock_path.exists(): - # Some CI matrix shards skip lockfile materialization. - return - text = lock_path.read_text(encoding="utf-8") - assert "@askjo/camofox-browser" not in text, ( - "package-lock.json still references @askjo/camofox-browser. " - "Regenerate the lockfile after removing the dep: " - "`rm package-lock.json && npm install --package-lock-only " - "--ignore-scripts --no-fund --no-audit`." - ) - assert "camoufox-js" not in text, ( - "package-lock.json still references camoufox-js (transitive of " - "@askjo/camofox-browser). Regenerate the lockfile." - ) diff --git a/web/src/components/ChatSidebar.tsx b/web/src/components/ChatSidebar.tsx index e6c6d09e0..57236e782 100644 --- a/web/src/components/ChatSidebar.tsx +++ b/web/src/components/ChatSidebar.tsx @@ -79,6 +79,21 @@ interface ChatSidebarProps { onSessionTitleChange?: (title: string | null) => void; } +/** Build the ``session.create`` params for the sidecar session. + * + * Extracted from the effect below so the invariant — close_on_disconnect + * is set, source is "tool", and the profile is forwarded when present — + * can be tested without reading component source text. See + * ``chat-sidebar-session-params.test.ts``. + */ +export function sidecarSessionCreateParams(profile?: string): Record { + return { + close_on_disconnect: true, + source: "tool", + ...(profile ? { profile } : {}), + }; +} + export function ChatSidebar({ channel, profile, @@ -189,11 +204,7 @@ export function ChatSidebar({ } // close_on_disconnect: the gateway reaps this sidecar session (and its // slash_worker subprocess) when the WS drops, instead of leaking it. - return gw.request<{ session_id: string }>("session.create", { - close_on_disconnect: true, - source: "tool", - ...(profile ? { profile } : {}), - }); + return gw.request<{ session_id: string }>("session.create", sidecarSessionCreateParams(profile)); }) .catch((e: Error) => { if (!cancelled) { diff --git a/web/src/lib/chat-sidebar-session-params.test.ts b/web/src/lib/chat-sidebar-session-params.test.ts new file mode 100644 index 000000000..212360d37 --- /dev/null +++ b/web/src/lib/chat-sidebar-session-params.test.ts @@ -0,0 +1,35 @@ +/** + * Tests for the sidecar ``session.create`` params built by ChatSidebar. + * + * The sidecar must opt its session into close_on_disconnect so the gateway + * reaps the slash_worker on WS disconnect (the #21370/#21467 leak), and it + * must pass the dashboard's selected profile so model/credential info + * matches the PTY child under profile-scoped chat. + * + */ + +import { describe, expect, it } from "vitest"; + +import { sidecarSessionCreateParams } from "@/components/ChatSidebar"; + +describe("sidecarSessionCreateParams", () => { + it("opts into close_on_disconnect", () => { + const params = sidecarSessionCreateParams(); + expect(params.close_on_disconnect).toBe(true); + }); + + it("sets source to 'tool'", () => { + const params = sidecarSessionCreateParams(); + expect(params.source).toBe("tool"); + }); + + it("forwards the profile when present", () => { + const params = sidecarSessionCreateParams("work"); + expect(params.profile).toBe("work"); + }); + + it("omits profile when undefined", () => { + const params = sidecarSessionCreateParams(); + expect(params).not.toHaveProperty("profile"); + }); +});