Skip to content

test(desktop): cover SDK namespace cycles and production plugin loading - #107405

Open
shiwandehongbai wants to merge 2 commits into
NousResearch:mainfrom
shiwandehongbai:fix/desktop-sdk-production-regression
Open

shiwandehongbai wants to merge 2 commits into
NousResearch:mainfrom
shiwandehongbai:fix/desktop-sdk-production-regression

Conversation

@shiwandehongbai

Copy link
Copy Markdown

What does this PR do?

Adds executable regression coverage for the production-only runtime/disk plugin failure (Cannot convert undefined or null to object) reported in #107288 and #107304.

This is a test companion stacked on #107303, not a competing implementation. The first commit preserves @g3org3yo's lazy-namespace fix from 6d719a11b6f9860cef6315a4fcda74cab29a5844 unchanged and with its original authorship. Our incremental commit is 5b90ac4e56 (tests/probe only). Maintainers can cherry-pick that test commit into #107303 instead of merging this PR. If #107303 merges first, this can be rebased to leave only the regression coverage.

Related Issue

Refs #107288, #107304, #107303.

Type of Change

Why this coverage?

We independently reproduced the failure on Windows after updating to 67764dc0863349a384c16425e73ee8571f3a94b7 (Desktop package version 0.17.2). Two unchanged disk plugins failed in the common SDK shim setup, before their code could run. The eager module-level map captured a not-yet-initialized SDK namespace in the generated bundle. Null-guarding Object.keys would not restore the missing named exports.

The existing loader/UI/legacy compatibility tests passed with the fix, but exercising the production module cycle and generated blob shims is important for this regression.

Changes Made

  • apps/desktop/scripts/tests/plugin-sdk-runtime.test.mjs: uses Vite's actual Rolldown dependency to bundle a cyclic module fixture; the positive path loads the real src/sdk/runtime.ts. Executes generated shim modules and checks default/named export identities and cache reuse. An isolated, portable eager-control implementation reproduces the old failure without needing Git history.
  • apps/desktop/scripts/check-plugin-sdk-production.mjs: builds the real desktop application graph with the existing production Vite config and a second smoke entry sharing its chunks, then loads that entry in a headless browser. No SDK/React/loader mocks in this production smoke. It tests no-import, SDK-named-import and React/JSX plugins, reload/disposal, stable shim caching, namespace export identities, actual React rendering, and rejection of unsupported/missing imports.
  • The probe uses a fresh temporary build/preview, blocks nonlocal browser requests and WebSockets, and cleans only its own temporary directory. It does not open personal plugin folders or connect to a Hermes backend. No new dependencies.

These are standalone commands, not silently added to the default Vitest suite or CI. The small cycle regression and the opt-in production/browser probe are separate so the normal targeted check does not require a browser/full build. Maintainer guidance on the preferred CI placement is welcome.

How to Test

From repository root with the existing workspace dependencies installed:

# GREEN: 2 passed, no skipped tests
node --test apps/desktop/scripts/tests/plugin-sdk-runtime.test.mjs

# Deliberate RED: exits 1 with Object.keys(undefined)
node apps/desktop/scripts/tests/plugin-sdk-runtime.test.mjs --baseline

# Production/browser GREEN
node apps/desktop/scripts/check-plugin-sdk-production.mjs

# Deliberate production RED: exits 1 before the no-import plugin registers
node apps/desktop/scripts/check-plugin-sdk-production.mjs --baseline

The browser commands use Playwright's Chromium by default. On this Windows machine its headless-shell was absent, so both controls were actually run with the explicit --channel=chrome option (installed Chrome); the probe does not download browsers. --channel=msedge is also supported. Optional --screenshot emits the synthetic smoke page as a data URL; all temporary files are still cleaned up.

Actual local results

  • Fast bundled-cycle suite: 2 passed, exit 0.
  • Old eager cycle control: exit 1 at Object.keys(undefined) (expected RED).
  • Old eager production control: exit 1, plugin load failed: Cannot convert undefined or null to object (expected RED).
  • Current production graph: 4,087 modules, Chrome 152.0.7977.83, exit 0, pageErrors: []:
    • no-import, SDK named import and React JSX plugins registered;
    • reload disposes previous contributions; shim map is stable;
    • SDK, React and both JSX namespaces retain all export identities;
    • unsupported package and missing named export remain errors;
    • React JSX rendered in Chromium.
  • Existing targeted Vitest suites: 16 passed across runtime-loader.test.ts, desktop-plugins-section.test.tsx, and legacy-sdk-compat.test.ts.
  • npm run typecheck --workspace apps/desktop: exit 0 (renderer, Electron, E2E types).
  • npm run build --workspace apps/desktop: exit 0.
  • npm run builder --workspace apps/desktop -- --dir --publish never: exit 0 (Windows unpacked artifact; no release publishing).
  • After backing up and installing the complete rebuilt renderer asset generation in the local packaged Desktop, a renderer reload restored both previously failing plugins without modifying either plugin. Desktop main/backend were not restarted. This is a local recovery observation, not a claim about all supported operating systems.

Existing Vite config/deprecated chunk-option and ineffective-dynamic-import warnings remain. The full Python suite and macOS/Linux runtime checks were not run for this frontend-only contribution.

Checklist

  • Read the contributing guide.
  • Conventional commit messages; scope limited to this regression.
  • Searched existing issues/PRs and explicitly reused fix(desktop): resolve plugin SDK namespaces lazily so disk plugins load in production builds #107303 with attribution.
  • Added executable regression coverage and independently reran the production positive check.
  • Tested on Windows; no machine paths, private logs, credentials, account data or theme assets included.
  • Full pytest tests/ -q (not run; frontend-only scope).
  • macOS/Linux runtime verification (not run).
  • No config keys, tool schemas, permissions or authentication changes.

Implementation was AI-assisted; the local test/build outputs and installed renderer recovery were independently checked before submission.

g3org3yo and others added 2 commits September 10, 2026 21:46
runtime.ts captured the SDK namespaces (the plugin SDK, React and the two jsx
runtimes) in a module-scope object literal, and that module sits in an import
cycle: sdk/index -> @/contrib/* -> contrib/runtime-loader -> sdk/runtime ->
sdk/index.

In an unbundled (dev) graph the namespace object is live, so the capture works.
In a production bundle the bundler emits the SDK namespace as a hoisted `var`
whose assignment lands AFTER the literal that reads it, so the captured value
is `undefined` (no TDZ error) and `Object.keys(GLOBALS[globalKey])` in
`shimUrl()` throws "Cannot convert undefined or null to object".

That throw happens inside `loadRuntimePlugin()` -- via `unsupportedImports()`
-> `sdkImportMap()` -> `shimUrl()`, which run for every source before it is
evaluated -- so it is content-independent: EVERY plugin loaded from
$HERMES_HOME/desktop-plugins/<id>/plugin.js (and the desktop/plugin.js half of
a unified package) fails to load, showing status "failed" in
Capabilities -> Plugins.

Resolve the namespaces at call time instead: `installPluginSdk()` and the shim
builder only ever run once the app is up, so reading them there is always safe
and statement ordering can no longer matter.

Repro (production build only): build apps/desktop for production, drop any
plugin.js into ~/.hermes/desktop-plugins/<id>/, start the app ->
[plugins] runtime load failed (<id>) TypeError: Cannot convert undefined or
null to object (.../assets/sdk-<hash>.js:5).

Note: a vitest unit test cannot catch this (dev module graph keeps the
namespace live); the faithful guard is a production-build smoke test that
loads a fixture plugin through loadRuntimePlugin().
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) comp/plugins Plugin system and bundled plugins labels Sep 10, 2026
QAbsolut added a commit to QAbsolut/hermes-agent that referenced this pull request Sep 10, 2026
…ad in production builds

Root cause: a top-level  capturing the  namespace import
was emitted by the Rolldown bundler before the import's namespace variable was
populated in the output bundle. This left
as , causing every runtime (disk) plugin's shim blob to crash with
 in Object.keys().

Fix: replace the eager  with a  function
that resolves the bindings at call time, after all imports are initialized.
Both installPluginSdk() and shimUrl() now call this function lazily.

Upstream tracking: NousResearch/hermes-agent issues NousResearch#107312, NousResearch#107304, NousResearch#107336,
NousResearch#107288, NousResearch#107291, NousResearch#107352; PRs NousResearch#107303, NousResearch#107301, NousResearch#107309, NousResearch#107338, NousResearch#107405.
This commit is a local fix — REMOVE when upstream merges one of those PRs.
QAbsolut added a commit to QAbsolut/hermes-agent that referenced this pull request Sep 10, 2026
…ad in production builds

Root cause: a top-level  capturing the  namespace import
was emitted by the Rolldown bundler before the import's namespace variable was
populated in the output bundle. This left
as , causing every runtime (disk) plugin's shim blob to crash with
 in Object.keys().

Fix: replace the eager  with a  function
that resolves the bindings at call time, after all imports are initialized.
Both installPluginSdk() and shimUrl() now call this function lazily.

Upstream tracking: NousResearch/hermes-agent issues NousResearch#107312, NousResearch#107304, NousResearch#107336,
NousResearch#107288, NousResearch#107291, NousResearch#107352; PRs NousResearch#107303, NousResearch#107301, NousResearch#107309, NousResearch#107338, NousResearch#107405.
This commit is a local fix — REMOVE when upstream merges one of those PRs.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants