fix: make the plugin load and run outside Bun (#60) - #97
Merged
Conversation
The artifact index imported bun:sqlite statically, and it sits in the eager chain through hooks/artifact-auto-index. Node and Electron reject the bun: scheme while resolving the module graph, so the whole plugin failed to register on OpenCode Desktop and none of its commands ran. Move the specifier behind an await import() at call time and keep the type import, which is erased. The build now emits no static bun: specifier, guarded by a bundle test. Also stop caching a half-built index when initialize() rejects. That path was unreachable while the import could not fail; now it can, and without this every later call reports "Database not initialized" instead of the real cause. Refs #60
ast-grep and btca imported spawn and which from the bun builtin, which made a node-target build impossible. Both used the same which-then-spawn shape, so extract one runtime-neutral helper over node:child_process and port them onto it. Also swap the remaining Bun globals outside octto's server: Bun.file stat and text, Bun.write, and the browser opener's Bun.spawn all have direct node: counterparts. Refs #60
Bun.serve was the last runtime dependency keeping the plugin from working under Electron. Replace it with node:http plus ws, which both Bun and Node run, and switch the shipped build to --target node so the bundle stops emitting the Bun-only import.meta.require shim. Octto only ever needed send() from a socket and stop()/port/hostname from a server, so express that as SessionSocket and SessionServer rather than leaking a runtime's types through the session layer. jsonc-parser joins bun-pty as external: its UMD build resolves nested requires that do not survive node-target bundling. Adds the first coverage for this module, driving a real server with real client sockets over loopback. Closes #60
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Bun and Node do not honour the spawn timeout option identically, so the timeout test passed locally and hung for the full sleep on CI. Drive the kill from our own timer instead of trusting the runtime's.
Killing the shell does not necessarily close the pipes: an orphaned grandchild keeps them open, so "close" only arrived once the original command would have finished anyway. CI hit Bun's five second per-test limit as a result. Settle from the timer instead, and assert the elapsed time in the test so the delay cannot come back unnoticed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/artifact-index/index.ts">
<violation number="1" location="src/tools/artifact-index/index.ts:408">
P2: Concurrent callers can create separate SQLite connections and receive different “singleton” indexes, which can race initialization and leave one connection unreachable by `close()`. Cache a shared initialization promise (clearing it on failure) so every caller awaits the same instance.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| if (globalIndex) return globalIndex; | ||
|
|
||
| const index = createArtifactIndex(); |
There was a problem hiding this comment.
P2: Concurrent callers can create separate SQLite connections and receive different “singleton” indexes, which can race initialization and leave one connection unreachable by close(). Cache a shared initialization promise (clearing it on failure) so every caller awaits the same instance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tools/artifact-index/index.ts, line 408:
<comment>Concurrent callers can create separate SQLite connections and receive different “singleton” indexes, which can race initialization and leave one connection unreachable by `close()`. Cache a shared initialization promise (clearing it on failure) so every caller awaits the same instance.</comment>
<file context>
@@ -386,9 +403,12 @@ export function createArtifactIndex(dbDir: string = DEFAULT_DB_DIR): ArtifactInd
- }
+ if (globalIndex) return globalIndex;
+
+ const index = createArtifactIndex();
+ // Only cache once initialization succeeds, otherwise every later call gets a
+ // half-built index reporting "not initialized" instead of the real failure.
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Externalise ws. Under --target node the bundler inlined the real npm package, and real ws never completes a handshake against Bun's node:http, so octto's socket hung in CONNECTING for every CLI user. Loading the bundle was not enough to catch that; the handshake now runs on both runtimes. Register the server error handler on the WebSocketServer. ws attaches its own listener to the HTTP server and re-emits on itself, so a handler on the HTTP server is registered too late to ever run: a bind failure crashed Node outright and left Bun waiting on a promise that never settled. Report a signal death as 128 + signum instead of collapsing it to 0. Both callers branch on exitCode, so a killed command read as a clean empty run. Reach schema.sql through import.meta.dirname; import.meta.path is Bun-only and left this file half-ported. Guard closeAllConnections, absent before Node 18.2, which would strand stop(). Pin the frame limit to Bun's 16 MB rather than inheriting ws's 100 MB. Narrow the server's dependency to SocketRouter, which removes the double cast from its tests. The tests earn their keep now. The timeout guard used a command that execs, so no grandchild held the pipes and reverting either fix still passed. The bundle test greps text it hardcodes rather than reading the build script, and never ran the artifact; it now loads it under both runtimes. loadSqlite's failure and the no-cache-on-failure path, the point of the whole change, had no coverage at all. Capturing logs had become blanket suppression, hiding stray output from every test that did not assert; unread lines now fail the test.
There was a problem hiding this comment.
2 issues found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/octto/session/server.ts">
<violation number="1" location="src/octto/session/server.ts:118">
P2: Stopping a session can hang on older Electron/Node runtimes after an HTTP client has used keep-alive. The optional call avoids the throw but leaves no fallback to destroy those HTTP sockets, so `http.close()` may never invoke its callback; track HTTP connections and destroy them when this API is unavailable.</violation>
</file>
<file name="tests/mindmodel/loader.test.ts">
<violation number="1" location="tests/mindmodel/loader.test.ts:22">
P3: Move `rmSync(testDir, ...)` before `expect(stray).toEqual([])` in this afterEach hook so that failed log-gate assertions don't leave tmp directories behind.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| wss.close(() => { | ||
| // Absent before Node 18.2, which older Electron builds still ship. An | ||
| // unguarded call throws inside ws's close callback and strands stop(). | ||
| http.closeAllConnections?.(); |
There was a problem hiding this comment.
P2: Stopping a session can hang on older Electron/Node runtimes after an HTTP client has used keep-alive. The optional call avoids the throw but leaves no fallback to destroy those HTTP sockets, so http.close() may never invoke its callback; track HTTP connections and destroy them when this API is unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/octto/session/server.ts, line 118:
<comment>Stopping a session can hang on older Electron/Node runtimes after an HTTP client has used keep-alive. The optional call avoids the throw but leaves no fallback to destroy those HTTP sockets, so `http.close()` may never invoke its callback; track HTTP connections and destroy them when this API is unavailable.</comment>
<file context>
@@ -93,7 +113,9 @@ function stop(http: Server, wss: WebSocketServer): Promise<void> {
- http.closeAllConnections();
+ // Absent before Node 18.2, which older Electron builds still ship. An
+ // unguarded call throws inside ws's close callback and strands stop().
+ http.closeAllConnections?.();
http.close(() => {
resolve();
</file context>
| afterEach(() => { | ||
| const stray = logs.unread(); | ||
| logs.restore(); | ||
| expect(stray).toEqual([]); |
There was a problem hiding this comment.
P3: Move rmSync(testDir, ...) before expect(stray).toEqual([]) in this afterEach hook so that failed log-gate assertions don't leave tmp directories behind.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/mindmodel/loader.test.ts, line 22:
<comment>Move `rmSync(testDir, ...)` before `expect(stray).toEqual([])` in this afterEach hook so that failed log-gate assertions don't leave tmp directories behind.</comment>
<file context>
@@ -17,7 +17,9 @@ describe("mindmodel loader", () => {
afterEach(() => {
+ const stray = logs.unread();
logs.restore();
+ expect(stray).toEqual([]);
rmSync(testDir, { recursive: true, force: true });
});
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #60.
Problem
On OpenCode Desktop the plugin appeared in the UI but none of its commands worked, because it never loaded:
The reporter's diagnosis was correct, but
bun:sqlitewas only the first of several blockers. Fixing it just moved the failure:Received protocol 'bun:' in ESM loader__require is not a functionWhat was actually in the way
bun:sqliteimported statically intools/artifact-index, which sits in the eager chain viahooks/artifact-auto-index. Node rejects the scheme while resolving the graph, so the whole plugin dies before registering. Now deferred to call time; the type import is erased.spawn/whichfrom thebunbuiltin intools/ast-grepandtools/btca. These made a node-target build impossible. Both used the same which-then-spawn shape, so they now share onenode:child_processhelper.Bun.file/Bun.write/Bun.spawnin the ledger loader, octto persistence, and the browser opener. Directnode:counterparts.Bun.servein octto's websocket server. Replaced withnode:http+ws.--target bunemittedvar __require = import.meta.require, which is undefined on Node. Now--target node, which Bun also runs.jsonc-parserresolves to a UMD build whose nestedrequire("./impl/format")calls do not survive node-target bundling. Externalized, likebun-pty. It is already a runtime dependency.Design note
Octto only ever used
send()from a socket andstop()/port/hostnamefrom a server. Rather than swap one runtime's types for another's, the session layer now depends onSessionSocketandSessionServer, so neither Bun's nor Node's types leak through it.New dependency:
ws(+@types/ws).Verification
Both runtimes load the shipped bundle:
Bun stays the primary runtime and is unaffected, which was the main regression risk in changing the build target.
The rewritten server was also exercised end to end under real Node v22, not just Bun:
{"httpStatus":200,"htmlBytes":52837,"notFound":404, "connects":1,"messages":[{"type":"response","id":"q1","answer":{"value":"yes"}}],"disconnects":1}469 tests pass, up from 450. Octto had zero coverage before this; the new suite drives a real server with real client sockets over loopback rather than mocking the transport. A bundle test guards the two constructs that caused this bug, and I mutation-checked it: restoring the static import fails it.