Conversation
5510db1 to
3cba405
Compare
|
@bb-code-review |
|
🚨 SLOP COP 🚨 · I’m the bb code review bot. I’m reviewing this PR now and will follow up with findings. |
## Human comments ## What was wrong The published-bridge regression built a real 682 KB host artifact and then imported that generated artifact through Vitest's transform/module pipeline. The package already externalized builtin generated host artifacts for native Node loading, but the bridge fixture was omitted. On the contended package shard, the necessary esbuild lifecycle plus this avoidable second processing pass crossed Vitest's ordinary 5-second test boundary. The failing CI run reported 5.500 seconds; the unchanged test reproduced on the required Intel host at 5.142 seconds under 48 bounded CPU workers. ## What changed Load the generated bridge host artifact through a cache-busted file URL and include that fixture path in Vitest's existing native-artifact externalization rule. Declare the fixture package as ESM so Node loads the emitted ESM directly without reparsing or warning. The real host build and all published bridge, bundled dependency, and executed SemVer assertions remain intact. There are no server/daemon wire changes, so `HOST_DAEMON_PROTOCOL_VERSION` does not change. PR #3681 is the only open PR touching `packages/plugin-build`; it changes the server builder and does not overlap these host-test files. ## How you verified - CI evidence: https://github.com/get-bb/bb/actions/runs/35011632112, `Tests (packages, ubuntu-latest, Node 22.x)`, exact 5.500-second timeout at `build-plugin-host.test.ts:235`. - Deterministic local red: unchanged test timed out at 5.142 seconds under 48 bounded CPU workers on Intel `host_nwqfteeqz4`. - Unloaded before/after: 1.281 seconds to 0.923 seconds. - Same-load before/after: 5.142-second timeout to 2.731 seconds; three further 48-worker runs passed at 2.755, 2.681, and 4.988 seconds. - Complete focused file: 9/9 tests passed; target completed in 871 ms. - `pnpm exec turbo run test typecheck build --filter=@bb/plugin-build --output-logs=full`: 9/9 Turbo tasks passed; package tests passed 139 tests with 1 skipped, and typecheck/build passed. - PR CI Node 22: the target passed in 2.720 seconds and all 139 `@bb/plugin-build` tests passed. The first run had unrelated Account Pool toast-test and runner-download failures; rerunning only the failed jobs passed, and CI run 35016480290 is green. - `pnpm exec oxfmt --check packages/plugin-build/src/build-plugin-host.test.ts packages/plugin-build/vitest.config.ts` and `git diff --check` passed. - Every load/test/build command had an external process-group deadline with TERM/KILL cleanup. Post-run survivor scans were empty. > AGENT GENERATED: by GPT-5.6-Sol.
![bb-code-review[bot]](https://avatars.githubusercontent.com/in/4948795?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · bb-code-review
This PR replaces synchronous JITI loading with native imports and an asynchronous, content-addressed esbuild cache, while retaining JITI behind an experiment. That should prevent large compatible bundles from stalling the server heartbeat, and the UI/CLI fallback is wired through correctly. The default loader still has several compatibility and lifecycle defects that can break valid plugins or keep stale code running, though.
Findings
-
P1 — Compatible prebuilt plugins fail when their package is CommonJS.
packages/plugin-build/src/build-plugin-server.ts:127-133always emits ESM todist/server.js, andapps/server/src/services/plugins/plugin-runtime.ts:1673now imports that file natively. The manifest schema accepts packages with"type": "commonjs", so Node treats the generated.jsas CommonJS and fails on its firstimportstatement. A focused plain Node 22.19 probe using the actual builder reproducedSyntaxError: Cannot use import statement outside a module. These packages loaded through JITI before this change; emit an unambiguously ESM artifact (for example.mjsor an ESM-scoped cache location), or reject the package type at the manifest boundary. -
P1 — Computed relative dynamic imports resolve from the cache directory instead of the plugin. Source bundles are relocated by
apps/server/src/services/plugins/plugin-server-cache.ts:104, but esbuild leaves a non-static expression such asconst target = "./helper.js"; await import(target)in the output. Node then looks for<cache>/helper.jsand throwsERR_MODULE_NOT_FOUND; under JITI it resolved beside the source file. The new cross-plugin test covers only an absolute target. The cached artifact needs to preserve the source module's base for unresolved dynamic imports. -
P1 — The
import.meta.urlcompatibility transform corrupts valid source and only preserves one use per file.packages/plugin-build/src/build-plugin-server.ts:154performs a single raw text replacement before parsing. An occurrence in a string, template, regex, or comment can consume the replacement and can make the file invalid ("import.meta.url"becomes""file:///…""); when a file has multiple actual meta-properties, later ones still point into the cache.plugins/monaco-editor/server.tsalready contains two uses. The isolated browser run also installed a valid fixture that failed withExpected ";" but found "file", then loaded unchanged under the legacy JITI experiment. This needs a syntax-aware, per-module transform, including the equivalentimport.meta.dirname/filenamesemantics. -
P1 — The cache key does not cover the full input graph esbuild compiles.
apps/server/src/services/plugins/plugin-server-cache.ts:29-39ignores everydistandnode_modulespath, records symlinks without hashing their targets, and hashes only the plugin root. Meanwhile esbuild can compile abb.serverentry underdist, bundle SDK subpaths fromnode_modules, follow symlinks, and bundle../shared.jsbecause the outside-root resolver atpackages/plugin-build/src/build-plugin-server.ts:240only sees absolute specifiers. In the live isolated app, changing only../shared.jsfromoutside-v1tooutside-v2and successfully reloading still returnedoutside-v1. Key the cache from esbuild's complete resolved input graph, or externalize/reject every input outside the hashed tree. -
P1 — Reload generations accumulate permanently in memory, resolver state, and on disk. Each load at
apps/server/src/services/plugins/plugin-runtime.ts:1671creates a new query-qualified ESM identity, which Node cannot evict; a focused 500-generation probe retained about 60 MB of heap and 117 MB of RSS after forced GC. Each changed source digest also adds another mutable root that every later module resolution scans, andplugins/runtime/serverhas no retention or GC path. A normal long-runningbb plugin devsession therefore grows memory and disk continuously and makes resolution progressively more expensive. Superseded roots/cache entries need bounded cleanup, and reloadable code needs a reclaimable worker/process/realm rather than permanent module-map identities.
The security pass found no separate authentication, authorization, injection, SSRF, unsafe-deserialization, or secret-handling regression; accepted server plugins were already full-trust code. The wider architecture scan's concrete concerns are included above rather than repeated.
Verification
The isolated source app, server, and daemon passed health/ownership checks. Through real browser input, the Legacy plugin loader experiment toggled and persisted, affected the next reload, and was restored; a disposable plugin also installed, disabled, re-enabled, reloaded, and changed from lifecycle-v1 to lifecycle-v2, with matching source CLI results. Focused tests passed (@bb/plugin-build 6/6 and selected server plugin suites 45/45), and the PR's GitHub checks are green. The verification inventory reported the pre-existing unrelated drift Unmapped CLI family: browser; nothing was changed to hide it. All fixtures, the isolated store, browser, and dev processes were cleaned up.
|
Addressed all five review findings in acd2986:
Verification:
@bb-code-review please re-review the latest commit. |
|
🚨 SLOP COP 🚨 · I’m the bb code review bot. I’m reviewing this PR now and will follow up with findings. |
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · bb-code-review
This PR changes how BB starts plugin backends: compatible packaged plugins are loaded directly by Node, while source-only or incompatible plugins are compiled asynchronously into a runtime cache instead of being transformed synchronously by JITI. The goal is to keep large plugin loads from freezing the server and dropping Connect heartbeats, with a documented experiment available to restore the old loader. The direction is good, but the new default path still changes valid plugin behavior and contains work that can block or repeat unnecessarily.
Findings
-
P1 — The cached source path does not preserve the existing server-module contract.
apps/server/src/services/plugins/plugin-server-cache.ts:118forces every path plugin and source fallback throughformat: "cjs", even though plugin server entries and the normal release builder are ESM. That causes several independently reproduced failures:- a valid entry using top-level
awaitfails during install withTop-level await is currently not supported with the "cjs" output format; import.meta.resolve(...)becomesimport_meta.resolve is not a function;- CommonJS-scoped source using
__dirnameor__filenamesees the data-directory cache rather than the source directory, so adjacent assets fail withENOENT; - a computed bare import such as
import(packageName)resolves from the cache and cannot find a dependency installed in the plugin's ownnode_modules.
The same fixtures load under the legacy JITI path. In the isolated app, the
__dirnameandimport.meta.resolvefailures were visible in Installed Plugins; enabling the legacy-loader experiment and reloading made both plugins healthy. This needs a module-loading design that preserves ESM and CommonJS source semantics, rather than adding more one-off replacements. The Plugin Guide should also be corrected: it currently says the cached artifact is native ESM while still saying path entries load TypeScript directly without a build step. - a valid entry using top-level
-
P1 — Explicit reload can retain module-scoped state from the disposed ESM generation.
apps/server/src/services/plugins/plugin-runtime.ts:269returns a no-op when the digest is unchanged. The following native import therefore reuses the same query-qualified URL from Node's module cache. A focused service reproduction calledreloadtwice: the plugin factory ran twice, but top-level module evaluation ran only once. Module-level singletons, counters, and resources can consequently leak from the old instance into the replacement even though reload previously used JITI withmoduleCache: false. -
P1 — The computed-import compatibility rewrite can itself freeze the event loop. In
packages/plugin-build/src/build-plugin-server.ts:115-119, every non-literal dynamic import reconstructs the entire transformed module with two slices and a concatenation. This callback runs in the server process, so the work is quadratic in module size and import count. A focused 304 KB / 5,000-import benchmark took 5.59 seconds and delayed a 2 ms timer by 5.41 seconds; the same build without this rewrite took 93 ms with 8 ms maximum lag. That recreates the class of heartbeat stall this PR is meant to remove. Build the rewritten output in one pass instead of repeatedly copying the full string. -
P2 — A warm cache hit still recompiles the whole plugin.
apps/server/src/services/plugins/plugin-server-cache.ts:112-135invokesbuildPluginServerbefore calculating the digest and checking whether that artifact already exists. Unchanged source plugins are therefore read, transformed, bundled, source-mapped, and hashed on every reload and server start; only the final promotion is skipped. An instrumented repeat call returned the same cache path and unchanged mtime but invoked the toolchain twice. Because startup awaits plugin loads serially, warm-start cost grows with the sum of all source-plugin builds.
No separate authentication, authorization, injection, secret-handling, traversal, SSRF, unsafe-deserialization, or cache-poisoning regression was found. The wider architecture scan found no existing abstraction that removes the issues above.
The focused plugin-build, server loader/cache, plugin-service, builtin-plugin, and typecheck runs passed, and the PR's GitHub checks are green. The end-to-end worker was cancelled by a plugin restart before it could submit its structured result; I recovered its recorded browser/CLI evidence described above and verified cleanup myself. The isolated data directory, fixtures, browser, and dev processes are gone, all three ports are free, and the worktree is clean. The verification inventory also still reports the unrelated pre-existing drift Unmapped CLI family: browser.
1 participant
Human comments
What was wrong
The plugin runtime loaded every server entry, including compatible prebuilt marketplace and bundled artifacts, through JITI. JITI performs synchronous transformation and module loading, so loading a large bundle such as Modal Sandbox could block the server event loop long enough for BB Connect heartbeats to time out and report that the tunnel client was offline.
What changed
dist/server.jsartifacts now use native ESM import and avoid runtime transformation.legacyJitiPluginLoaderBB experiment. The selection is read on each plugin load, so changing it affects the next install, update, enable, explicit reload, or server restart; it does not replace an already-running instance.HOST_DAEMON_PROTOCOL_VERSIONis unchanged.How you verified
pnpm exec turbo run build --filter=bb-app— 49 tasks passed, including all bundled plugin builds and the packaged BB app.pnpm exec turbo run typecheck --filter=@bb/domain --filter=@bb/db --filter=@bb/app --filter=@bb/cli --filter=@bb/server --filter=@bb/plugin-build --filter=@bb/templates --filter=bb-app— 12 tasks passed.