fix: isolate embedding in a Node sidecar - #23
Conversation
📝 WalkthroughWalkthroughThe PR moves default semantic embedding into a persistent Node 20+ sidecar. It adds NDJSON lifecycle and error handling, explicit inline mode, lexical-search bypasses, integration coverage, doctor checks, packaging validation, and updated documentation. ChangesEmbedding sidecar architecture
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Host as Bun host
participant Client as embed.ts
participant Sidecar as embed-sidecar.mjs
participant Model as Transformers.js model
Host->>Client: request embeddings
Client->>Sidecar: send NDJSON request
Sidecar->>Model: run inference
Model-->>Sidecar: return normalized vectors
Sidecar-->>Client: send NDJSON response
Client-->>Host: return embeddings
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
src/embed.ts (1)
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
fileURLToPathinstead ofURL.pathname.
URL.pathnamereturns/C:/path/...on Windows, and manualdecodeURIComponentdoes not repair the leading slash or the drive letter.fileURLToPathhandles percent-decoding and platform path shape in one call.♻️ Proposed refactor
Add the import:
+import { fileURLToPath } from "node:url";Then:
- const sidecarPath = decodeURIComponent(new URL("./embed-sidecar.mjs", import.meta.url).pathname); + const sidecarPath = fileURLToPath(new URL("./embed-sidecar.mjs", import.meta.url));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/embed.ts` at line 174, Update the sidecarPath initialization in embed.ts to use Node’s fileURLToPath conversion on the module URL instead of URL.pathname with manual decodeURIComponent, ensuring percent-decoding and Windows path handling are performed correctly.spikes/pack-smoke.sh (1)
39-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the sidecar actually ran and that the
.mjsis in the artifact.The message at Line 47 claims a sidecar embed, but nothing in the block verifies it.
EPISODIC_EMBED_MODEis unset, so the default applies. If a future change flipped the default to inline, this check would still produce 768 dims and a unit norm, and it would still print "sidecar embed ok". The gate would pass while the packaged sidecar was broken or absent.Add two cheap assertions: confirm
src/embed-sidecar.mjsexists in the installed tree, and pin the mode explicitly.♻️ Proposed refactor
echo "== packaged Node sidecar embed smoke (downloads ~110MB model on first run) ==" +SIDECAR="$WORK/cache/node_modules/opencode-episodic-memory/src/embed-sidecar.mjs" +[ -f "$SIDECAR" ] || { echo "missing packaged sidecar: $SIDECAR"; exit 1; } +echo "packaged sidecar present: $SIDECAR" +export EPISODIC_EMBED_MODE=sidecar (cd "$WORK/cache" && bun -e "As per coding guidelines: "Use
bash spikes/pack-smoke.shas the release artifact gate, including a clean-install import and real Node-sidecar embedding check; ensuresrc/embed-sidecar.mjsis included in the npm artifact."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spikes/pack-smoke.sh` around lines 39 - 47, Update the packaged embedding smoke-test block around embedQuery to explicitly set EPISODIC_EMBED_MODE to the Node-sidecar mode and verify src/embed-sidecar.mjs exists in the installed tree before running the embedding assertion. Keep the existing dimension and normalization checks and success output unchanged.Source: Coding guidelines
src/embed.test.ts (1)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed sleeps with condition polling.
Lines 77, 81, 90, and 136 wait for a fixed number of milliseconds. On a loaded CI runner a 10 ms or 20 ms wait can elapse before the sidecar has restarted or before the process has exited, which makes the suite flaky. The comment at Lines 88-89 states the intent to avoid relying on scheduling, but a fixed 20 ms still relies on it.
Add a small poll helper and wait for the observable condition.
♻️ Proposed refactor
const sleep = (milliseconds: number) => new Promise((resolve) => setTimeout(resolve, milliseconds)); + +async function waitFor(predicate: () => boolean, timeout = 2_000): Promise<void> { + const deadline = Date.now() + timeout; + while (Date.now() < deadline) { + if (predicate()) return; + await sleep(5); + } + throw new Error("condition not met before the timeout"); +}Then, for the restart assertion:
const startsBefore = events().filter(({ event }) => event === "start").length; expect(await embed(["exit-once"])).toEqual([new Float32Array([9, 0, 1])]); - await sleep(20); - expect(events().filter(({ event }) => event === "start").length).toBeGreaterThan(startsBefore); + await waitFor(() => events().filter(({ event }) => event === "start").length > startsBefore);Also applies to: 88-91, 136-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/embed.test.ts` around lines 76 - 82, Replace the fixed sleep calls in the embed restart/error tests with a small polling helper that repeatedly checks the relevant observable condition until it is satisfied or a timeout is reached. Update the waits around embed(["after-bad-count"]), embed(["after-bad-dimensions"]), the lines 88-91 flow, and line 136 to poll for sidecar restart or process exit rather than elapsed time, while preserving the existing assertions.plugin/episodic-memory.ts (1)
80-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated empty-result block.
Lines 82-85 and Lines 97-100 contain the same two-branch empty-result logic. Extract one helper and call it from both places.
♻️ Proposed refactor
+ const noHits = () => + isIndexEmpty(index) + ? "No matching past conversations found. The index is empty — run `bun run src/cli.ts sync` to index conversations." + : "No matching past conversations found."; if (args.mode === "text") { const hits = textSearch(index, args.query, opts); - if (hits.length === 0) { - if (isIndexEmpty(index)) return "No matching past conversations found. The index is empty — run `bun run src/cli.ts sync` to index conversations."; - return "No matching past conversations found."; - } + if (hits.length === 0) return noHits(); return formatHits(hits, 400, "score"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/episodic-memory.ts` around lines 80 - 100, Extract the duplicated empty-result handling into a shared helper near the search logic, preserving the isIndexEmpty(index) check and both existing messages. Replace the inline blocks after textSearch and hybrid/vector search with calls to this helper, leaving the surrounding search behavior unchanged.ARCHITECTURE.md (1)
265-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
markdownlint reports MD040 at Line 265. Use
textfor the directory listing.📝 Proposed fix
-``` +```text src/ core library (reader, parser, embed host/client, inline backend, Node sidecar, store, indexer, format, cli) + tests🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ARCHITECTURE.md` at line 265, Update the fenced code block containing the directory listing in ARCHITECTURE.md to specify the text language, changing its opening fence to ```text while preserving the listing content.Source: Linters/SAST tools
src/embed-sidecar.mjs (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe default model literal is duplicated and can drift silently.
src/embed.tsLine 5 declaresDEFAULT_MODEL = "Snowflake/snowflake-arctic-embed-m-v1.5", andsrc/embed-inline.tsLine 13 imports it. This file repeats the literal because a plain Node.mjsmodule cannot import the.tsmodule. If the two values ever diverge, documents and queries are embedded by different models, and cosine scores become meaningless without any error.Two options:
- Move the literal into a shared
.mjsor.jsonmodule that bothsrc/embed.tsand this file import.- Keep the literal here and add a test that asserts the two values match.
At minimum, add a comment that points at
src/embed.tsLine 5.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/embed-sidecar.mjs` at line 12, The default embedding model is duplicated between the sidecar and embed implementation, risking silent divergence. Update the model definition near process.env.EPISODIC_EMBED_MODEL and src/embed.ts’s DEFAULT_MODEL to use a shared .mjs or .json value; if that is not feasible, retain the literal and add a matching-value test plus a comment referencing DEFAULT_MODEL in src/embed.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 102-108: Update AGENTS.md lines 102-108 to describe Node 20+ and
the persistent sidecar as the default embedding path, while stating that
EPISODIC_EMBED_MODE=inline lazily loads the backend on the first embedding
request in Bun and remains unsafe in affected OpenCode/Bun versions. Update
README.md lines 55-59 to state that Node 20+ is required for sidecar mode and
that inline mode is available without Node as an unsafe alternative.
In `@ARCHITECTURE.md`:
- Around line 48-57: Correct the architecture diagrams so the sidecar process
routes vectors through the Bun host and indexing flow rather than directly to
store.ts or scoreVector. Update the system overview’s ES --> S relationship and
the read-path diagram around N --> V to reflect the existing embed.ts/indexer.ts
and read-path sequence, keeping them consistent with the sequence diagram.
In `@plugin/episodic-memory.ts`:
- Line 92: Update the semantic-search embedding failure handling near the
returned message to log the detailed error through the plugin logger, then
return only a short stable error message without interpolating e.message or
other exception text. Preserve the existing guidance to use text mode or run the
doctor command.
In `@spikes/fake-embed-sidecar.mjs`:
- Around line 5-6: The sidecar and tests derive temporary paths inconsistently.
In spikes/fake-embed-sidecar.mjs lines 5-6, use tmpdir() and join() from node:os
and node:path for both logPath and exitOncePath; in src/embed.test.ts lines 7-8,
define and set both environment variables before the dynamic import, then
forward those values to the spawned host at line 125 so the inner script uses
them instead of rebuilding paths from process.ppid.
In `@src/embed-sidecar.mjs`:
- Around line 29-40: Bound embedding batch sizes using the existing EPISODIC_*
configuration from the synchronousSession/indexer flow, ensuring texts is split
into capped chunks before invoking embedder and each chunk’s vectors are
returned in order. Preserve validation and output behavior while preventing a
single sidecar response from processing an unbounded session.
- Around line 74-80: Chain the request queue in the stdin handler onto the
initialization promise before invoking embed, so requests received before
initialize() completes wait for readiness instead of calling an undefined
embedder. Update the queue setup around the existing queue.then callback and
preserve the current requestError handling and sequential processing behavior.
In `@src/embed.test.ts`:
- Around line 128-137: Update the PID validation in the spawned-host test before
the process.kill call to require pid to be a positive safe integer, so a missing
or invalid output producing 0 fails immediately and cannot be treated as a valid
sidecar process.
- Around line 101-112: Wrap each test’s EPISODIC_EMBED_MODE assignment and
assertion in a try/finally block, including the invalid-mode test and the lazy
inline-mode test. Move deletion of the environment variable into finally so
cleanup always runs when embed rejects or the assertion fails.
- Around line 11-12: Remove the chmodSync call from the test setup and its
node:fs import, and commit spikes/fake-embed-sidecar.mjs with executable mode
100755 so EPISODIC_NODE_BINARY can run it without mutating the repository during
tests.
In `@src/embed.ts`:
- Around line 205-209: Update requestSidecar to bound both await child.ready and
the per-request promise so callers cannot hang indefinitely. Use separate,
configurable EPISODIC_* timeout values: a generous readiness timeout that
accommodates first-run model downloads and a shorter request timeout applied
only after readiness succeeds; preserve the existing retry and lexical-fallback
behavior when either timeout expires.
- Around line 106-110: Update the ready:false branch in the sidecar startup
handling to invoke the existing teardown path (sidecarGone) before or while
rejecting readiness, rather than leaving the failed child and module-level
sidecar reference alive. Ensure the resulting rejection uses
SidecarUnavailableError so the retry logic around startSidecar is applied, while
preserving the existing error detail and avoiding duplicate readiness rejection.
- Around line 211-221: Update the request flow after registering the pending
entry in the embedding method around child.pending.set to recheck sidecar
liveness and identity before writing. If the sidecar has exited or has already
been replaced, remove the newly registered request and reject it with the
existing SidecarUnavailableError path; otherwise preserve the normal stdin write
behavior.
---
Nitpick comments:
In `@ARCHITECTURE.md`:
- Line 265: Update the fenced code block containing the directory listing in
ARCHITECTURE.md to specify the text language, changing its opening fence to
```text while preserving the listing content.
In `@plugin/episodic-memory.ts`:
- Around line 80-100: Extract the duplicated empty-result handling into a shared
helper near the search logic, preserving the isIndexEmpty(index) check and both
existing messages. Replace the inline blocks after textSearch and hybrid/vector
search with calls to this helper, leaving the surrounding search behavior
unchanged.
In `@spikes/pack-smoke.sh`:
- Around line 39-47: Update the packaged embedding smoke-test block around
embedQuery to explicitly set EPISODIC_EMBED_MODE to the Node-sidecar mode and
verify src/embed-sidecar.mjs exists in the installed tree before running the
embedding assertion. Keep the existing dimension and normalization checks and
success output unchanged.
In `@src/embed-sidecar.mjs`:
- Line 12: The default embedding model is duplicated between the sidecar and
embed implementation, risking silent divergence. Update the model definition
near process.env.EPISODIC_EMBED_MODEL and src/embed.ts’s DEFAULT_MODEL to use a
shared .mjs or .json value; if that is not feasible, retain the literal and add
a matching-value test plus a comment referencing DEFAULT_MODEL in src/embed.ts.
In `@src/embed.test.ts`:
- Around line 76-82: Replace the fixed sleep calls in the embed restart/error
tests with a small polling helper that repeatedly checks the relevant observable
condition until it is satisfied or a timeout is reached. Update the waits around
embed(["after-bad-count"]), embed(["after-bad-dimensions"]), the lines 88-91
flow, and line 136 to poll for sidecar restart or process exit rather than
elapsed time, while preserving the existing assertions.
In `@src/embed.ts`:
- Line 174: Update the sidecarPath initialization in embed.ts to use Node’s
fileURLToPath conversion on the module URL instead of URL.pathname with manual
decodeURIComponent, ensuring percent-decoding and Windows path handling are
performed correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e4c53b7-15ab-498a-969a-25c4ba712c9c
📒 Files selected for processing (12)
AGENTS.mdARCHITECTURE.mdREADME.mddocs/RELEASE.mdplugin/episodic-memory.tsspikes/fake-embed-sidecar.mjsspikes/pack-smoke.shsrc/cli.tssrc/embed-inline.tssrc/embed-sidecar.mjssrc/embed.test.tssrc/embed.ts
| - Plugin runs inside OpenCode's Bun runtime. Transformers.js and its native ML | ||
| addons (`onnxruntime-node`, `sharp`) MUST load only in the persistent Node 20+ | ||
| sidecar, never on plugin import. The host defaults to `EPISODIC_EMBED_MODE=sidecar`; | ||
| `inline` dynamically imports the backend only at first embedding request and is | ||
| unsafe in affected OpenCode/Bun versions. Text search and transcript reads do | ||
| not embed and therefore work without Node. `onnxruntime-node` ships platform | ||
| binaries in its npm tarball; `trustedDependencies` only affects contributors. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the documentation with the supported inline mode.
Both sections describe the sidecar or Node runtime as the only embedding path. The PR also supports explicit EPISODIC_EMBED_MODE=inline. Qualify these statements as default sidecar behavior and retain the inline safety warning.
AGENTS.md#L102-L108: qualify the sidecar-only requirement and state that inline mode loads lazily in Bun.README.md#L55-L59: state that Node 20+ is required for sidecar mode, while inline mode is available without Node as an unsafe alternative.
📍 Affects 2 files
AGENTS.md#L102-L108(this comment)README.md#L55-L59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 102 - 108, Update AGENTS.md lines 102-108 to describe
Node 20+ and the persistent sidecar as the default embedding path, while stating
that EPISODIC_EMBED_MODE=inline lazily loads the backend on the first embedding
request in Bun and remains unsafe in affected OpenCode/Bun versions. Update
README.md lines 55-59 to state that Node 20+ is required for sidecar mode and
that inline mode is available without Node as an unsafe alternative.
| OCDB --> R --> P --> E | ||
| E <-->|NDJSON stdin/stdout| ES | ||
| ES --> S | ||
| S <--> IDB | ||
| PLG --> S | ||
| CLI --> S | ||
| PLG -.->|session.idle| R | ||
| SK -.when-to-recall.-> PLG | ||
|
|
||
| HF[(Hugging Face<br/>model hub)] -.one-time ~100 MB<br/>download, then cached.-> E | ||
| HF[(Hugging Face<br/>model hub)] -.one-time ~100 MB<br/>download, then cached.-> ES |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The flowchart routes the sidecar directly to store.ts, which the code does not do.
Line 50 draws ES --> S. In the implementation the sidecar returns vectors to embed.ts in the Bun host over stdout, and indexer.ts then writes them through store.ts. The Node process never touches index.db. The sequence diagram at Lines 174-176 already shows the correct round trip, so the two diagrams disagree.
The same shape appears at Lines 205-207 in the read path, where N --> V implies the sidecar feeds scoreVector directly.
📝 Proposed fix
OCDB --> R --> P --> E
E <-->|NDJSON stdin/stdout| ES
- ES --> S
+ E --> S
S <--> IDBApply the equivalent correction in the read-path diagram:
Q[query text] --> EQ["embedQuery in Bun<br/>prepend retrieval prefix,<br/>truncate at 2000 chars"]
- EQ --> N["Node sidecar<br/>embed 768-dim"]
- N --> V["scoreVector<br/>brute-force cosine over all<br/>candidate embedding blobs<br/>+ time/text filters + minScore"]
+ EQ <-->|NDJSON| N["Node sidecar<br/>embed 768-dim"]
+ EQ --> V["scoreVector<br/>brute-force cosine over all<br/>candidate embedding blobs<br/>+ time/text filters + minScore"]As per coding guidelines: "Keep ARCHITECTURE.md synchronized with pipeline changes, including its system overview, data model, read/write paths, and design decisions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ARCHITECTURE.md` around lines 48 - 57, Correct the architecture diagrams so
the sidecar process routes vectors through the Bun host and indexing flow rather
than directly to store.ts or scoreVector. Update the system overview’s ES --> S
relationship and the read-path diagram around N --> V to reflect the existing
embed.ts/indexer.ts and read-path sequence, keeping them consistent with the
sequence diagram.
Source: Coding guidelines
| try { | ||
| vector = (await embedQuery(args.query))[0]; | ||
| } catch (e) { | ||
| return `Semantic search unavailable: embedding failed (${e instanceof Error ? e.message : e}). Use mode: "text" for embedding-free lexical search, or run \`bun run src/cli.ts doctor\`.`; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
The error text can carry sidecar stderr, including local filesystem paths.
src/embed.ts builds sidecar errors with sidecarError at Lines 49-52, which appends up to 8 KiB of captured child stderr to the message. That stderr comes from Node and from Transformers.js, so it can include the model cache path under the user's home directory and the Node binary path. Line 92 returns that text as the tool result, which goes to the model provider.
Return a short, stable message to the agent, and send the detailed text to the plugin log instead.
🛡️ Proposed fix
} catch (e) {
- return `Semantic search unavailable: embedding failed (${e instanceof Error ? e.message : e}). Use mode: "text" for embedding-free lexical search, or run \`bun run src/cli.ts doctor\`.`;
+ await log("warn", `episodic_search embedding failed: ${e instanceof Error ? e.message : e}`);
+ return 'Semantic search unavailable: the embedding backend failed. Use mode: "text" for embedding-free lexical search, or run `bun run src/cli.ts doctor` for details.';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return `Semantic search unavailable: embedding failed (${e instanceof Error ? e.message : e}). Use mode: "text" for embedding-free lexical search, or run \`bun run src/cli.ts doctor\`.`; | |
| } catch (e) { | |
| await log("warn", `episodic_search embedding failed: ${e instanceof Error ? e.message : e}`); | |
| return 'Semantic search unavailable: the embedding backend failed. Use mode: "text" for embedding-free lexical search, or run `bun run src/cli.ts doctor` for details.'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/episodic-memory.ts` at line 92, Update the semantic-search embedding
failure handling near the returned message to log the detailed error through the
plugin logger, then return only a short stable error message without
interpolating e.message or other exception text. Preserve the existing guidance
to use text mode or run the doctor command.
| const logPath = process.env.EPISODIC_TEST_SIDECAR_LOG ?? `/tmp/episodic-fake-sidecar-${process.ppid}.log`; | ||
| const exitOncePath = process.env.EPISODIC_TEST_SIDECAR_EXIT_ONCE ?? `/tmp/episodic-fake-sidecar-exit-${process.ppid}`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The fixture and the test derive the temp directory differently, so the offline sidecar tests fail on macOS. The fixture hardcodes /tmp while the test uses os.tmpdir(). The two agree only on Linux, where tmpdir() is /tmp. On macOS tmpdir() resolves under /var/folders/..., the fixture writes its log somewhere the test never reads, events() returns [], and the assertion at src/embed.test.ts Line 46 fails. The PR states that macOS verification is still outstanding.
spikes/fake-embed-sidecar.mjs#L5-L6: replace the hardcoded/tmpprefix withjoin(tmpdir(), ...)fromnode:osandnode:pathfor bothlogPathandexitOncePath.src/embed.test.ts#L7-L8: setEPISODIC_TEST_SIDECAR_LOGandEPISODIC_TEST_SIDECAR_EXIT_ONCEfrom these values before the dynamic import at Line 14, and forward them to the spawned host at Line 125 so the inner script at Line 121 stops rebuilding the path itself. That removes both the directory assumption and the implicitprocess.ppidcoupling.
📍 Affects 2 files
spikes/fake-embed-sidecar.mjs#L5-L6(this comment)src/embed.test.ts#L7-L8
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spikes/fake-embed-sidecar.mjs` around lines 5 - 6, The sidecar and tests
derive temporary paths inconsistently. In spikes/fake-embed-sidecar.mjs lines
5-6, use tmpdir() and join() from node:os and node:path for both logPath and
exitOncePath; in src/embed.test.ts lines 7-8, define and set both environment
variables before the dynamic import, then forward those values to the spawned
host at line 125 so the inner script uses them instead of rebuilding paths from
process.ppid.
| async function embed(texts) { | ||
| const output = await embedder(texts, { pooling: "cls", normalize: true }); | ||
| const dimensions = output.dims.at(-1); | ||
| if (!Number.isSafeInteger(dimensions) || dimensions <= 0) throw new Error("model returned invalid embedding dimensions"); | ||
| const data = output.data; | ||
| if (data.length !== texts.length * dimensions) throw new Error("model returned an invalid embedding batch"); | ||
| const vectors = []; | ||
| for (let index = 0; index < texts.length; index++) { | ||
| vectors.push(Array.from(data.slice(index * dimensions, (index + 1) * dimensions))); | ||
| } | ||
| return vectors; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every call site of embed() and any batching/chunking constants.
rg -nP --type=ts -C6 '\bembed\s*\(' -g '!**/*.test.ts'
echo "=== batching constants ==="
rg -nP --type=ts -C3 '(BATCH|batchSize|chunkSize|CHUNK_)'Repository: robertn702/opencode-episodic-memory
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -u
echo "=== tracked files likely relevant ==="
git ls-files | rg '(^|/)(src/embed-sidecar\.mjs|src/indexer\.ts|src/embed\.ts|package\.json|pnpm-lock\.yaml|package-lock\.json|yarn\.lock)$|(-test\.ts|\.test\.ts)$' | sed -n '1,200p'
echo
echo "=== find embed call sites (type-agnostic) ==="
rg -n --glob '!**/*.test.ts' --glob '!node_modules' --glob '!dist' --glob '!build' '\bembed\s*\(' . || true
echo
echo "=== find batching/chunking constants ==="
rg -n --glob '!**/*.test.ts' --glob '!node_modules' --glob '!dist' --glob '!build' '(\$?[A-Z_]*(?:BATCH|BATCH_|BATCH_SIZE|SIZE|CHUNK|CHUNK_SIZE|MAX_|LIMIT|WINDOW))\b|batchSize|chunkSize|chunkSize|maxBatch|batchSize' . || true
echo
echo "=== file sizes ==="
for f in src/embed-sidecar.mjs src/indexer.ts src/embed.ts; do
[ -f "$f" ] && wc -l "$f"
doneRepository: robertn702/opencode-episodic-memory
Length of output: 1652
🏁 Script executed:
#!/bin/bash
set -u
echo "=== src/indexer.ts ==="
cat -n src/indexer.ts
echo
echo "=== embedding constants/config env refs ==="
rg -n --glob '!**/*.test.ts' --glob '!node_modules' --glob '!dist' --glob '!build' 'EPISODIC|max|MAX|maximal|maximum|embedding|embeddings|DIMENSIONS|dims|8192|500|32|text|texts' src package.json .Repository: robertn702/opencode-episodic-memory
Length of output: 50391
Bound the embedding request size from src/indexer.ts.
synchronousSession batches every exchange in a SourceSession into one embed(texts) call, while sidecar output is one JSON line per batch. Use an EPISODIC_* batch-size cap and split texts before calling embed, or cap it in the sidecar request path so large sessions do not allocate multi-megabytes per single response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/embed-sidecar.mjs` around lines 29 - 40, Bound embedding batch sizes
using the existing EPISODIC_* configuration from the synchronousSession/indexer
flow, ensuring texts is split into capped chunks before invoking embedder and
each chunk’s vectors are returned in order. Preserve validation and output
behavior while preventing a single sidecar response from processing an unbounded
session.
| process.env.EPISODIC_EMBED_MODE = "not-a-mode"; | ||
| await expect(embed(["invalid-mode"])).rejects.toThrow('Invalid EPISODIC_EMBED_MODE "not-a-mode"'); | ||
| delete process.env.EPISODIC_EMBED_MODE; | ||
| }); | ||
|
|
||
| test("keeps inline mode lazy for an empty batch (no Transformers.js/model load)", async () => { | ||
| const startsBefore = events().filter(({ event }) => event === "start").length; | ||
| process.env.EPISODIC_EMBED_MODE = "inline"; | ||
| expect(await embed([])).toEqual([]); | ||
| delete process.env.EPISODIC_EMBED_MODE; | ||
| expect(events().filter(({ event }) => event === "start").length).toBe(startsBefore); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
A failed assertion leaks EPISODIC_EMBED_MODE into later tests.
Line 101 sets EPISODIC_EMBED_MODE, and Line 103 deletes it. If the assertion at Line 102 rejects, Line 103 never runs and the variable stays set for the rest of the file. The same pattern appears at Lines 108-110.
getEmbedMode reads that variable on every call in src/embed.ts Line 40, so one failure cascades into unrelated failures and hides the original cause.
Use try/finally.
🐛 Proposed fix
- process.env.EPISODIC_EMBED_MODE = "not-a-mode";
- await expect(embed(["invalid-mode"])).rejects.toThrow('Invalid EPISODIC_EMBED_MODE "not-a-mode"');
- delete process.env.EPISODIC_EMBED_MODE;
+ process.env.EPISODIC_EMBED_MODE = "not-a-mode";
+ try {
+ await expect(embed(["invalid-mode"])).rejects.toThrow('Invalid EPISODIC_EMBED_MODE "not-a-mode"');
+ } finally {
+ delete process.env.EPISODIC_EMBED_MODE;
+ }Apply the same structure to the inline-mode test at Lines 108-110.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/embed.test.ts` around lines 101 - 112, Wrap each test’s
EPISODIC_EMBED_MODE assignment and assertion in a try/finally block, including
the invalid-mode test and the lazy inline-mode test. Move deletion of the
environment variable into finally so cleanup always runs when embed rejects or
the assertion fails.
| const pid = Number(output.trim()); | ||
| expect(Number.isSafeInteger(pid)).toBe(true); | ||
| for (let attempt = 0; attempt < 20; attempt++) { | ||
| try { | ||
| process.kill(pid, 0); | ||
| } catch { | ||
| return; | ||
| } | ||
| await sleep(10); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against a pid of 0 before calling process.kill.
If the spawned host fails, output is empty, Number("".trim()) is 0, and Number.isSafeInteger(0) is true, so the assertion at Line 129 passes. Line 132 then calls process.kill(0, 0). On POSIX, pid 0 addresses the entire process group of the caller. Signal 0 performs only a permission check, so nothing is terminated, but the call succeeds, the loop runs all 20 iterations, and the failure message at Line 138 reports a nonexistent sidecar instead of the real cause.
Assert that the pid is positive.
🐛 Proposed fix
const pid = Number(output.trim());
- expect(Number.isSafeInteger(pid)).toBe(true);
+ expect(Number.isSafeInteger(pid) && pid > 0).toBe(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pid = Number(output.trim()); | |
| expect(Number.isSafeInteger(pid)).toBe(true); | |
| for (let attempt = 0; attempt < 20; attempt++) { | |
| try { | |
| process.kill(pid, 0); | |
| } catch { | |
| return; | |
| } | |
| await sleep(10); | |
| } | |
| const pid = Number(output.trim()); | |
| expect(Number.isSafeInteger(pid) && pid > 0).toBe(true); | |
| for (let attempt = 0; attempt < 20; attempt++) { | |
| try { | |
| process.kill(pid, 0); | |
| } catch { | |
| return; | |
| } | |
| await sleep(10); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/embed.test.ts` around lines 128 - 137, Update the PID validation in the
spawned-host test before the process.kill call to require pid to be a positive
safe integer, so a missing or invalid output producing 0 fails immediately and
cannot be treated as a valid sidecar process.
| if ("ready" in record) { | ||
| if (record.ready === true) child.resolveReady(); | ||
| else child.rejectReady(new Error(`Embedding sidecar failed to start: ${typeof record.error === "string" ? record.error : "unknown error"}`)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Tear down the sidecar when startup reports ready:false.
Line 108 rejects the ready promise but leaves the module-level sidecar reference set and leaves the child process alive. Two consequences follow:
- The next call reaches
startSidecar()at Line 172, gets the same failed child, and awaits the already-rejectedready. Recovery then depends on the child exiting first and clearingsidecarthrough theexitedhandler at Line 196. That is a race, not a deterministic path. - The rejection is a plain
Error, not aSidecarUnavailableError, so the single retry at Line 223 never applies to a startup failure.
Route the failure through the existing teardown so the state is cleared and the error type is consistent.
🐛 Proposed fix
if ("ready" in record) {
if (record.ready === true) child.resolveReady();
- else child.rejectReady(new Error(`Embedding sidecar failed to start: ${typeof record.error === "string" ? record.error : "unknown error"}`));
+ else {
+ sidecarGone(child, new SidecarUnavailableError(`Embedding sidecar failed to start: ${typeof record.error === "string" ? record.error : "unknown error"}`));
+ }
return;
}Note that sidecarGone calls rejectAll, which calls rejectReady, so the ready promise is still rejected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("ready" in record) { | |
| if (record.ready === true) child.resolveReady(); | |
| else child.rejectReady(new Error(`Embedding sidecar failed to start: ${typeof record.error === "string" ? record.error : "unknown error"}`)); | |
| return; | |
| } | |
| if ("ready" in record) { | |
| if (record.ready === true) child.resolveReady(); | |
| else { | |
| sidecarGone(child, new SidecarUnavailableError(`Embedding sidecar failed to start: ${typeof record.error === "string" ? record.error : "unknown error"}`)); | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/embed.ts` around lines 106 - 110, Update the ready:false branch in the
sidecar startup handling to invoke the existing teardown path (sidecarGone)
before or while rejecting readiness, rather than leaving the failed child and
module-level sidecar reference alive. Ensure the resulting rejection uses
SidecarUnavailableError so the retry logic around startSidecar is applied, while
preserving the existing error detail and avoiding duplicate readiness rejection.
| async function requestSidecar(texts: string[], retried = false): Promise<Float32Array[]> { | ||
| let child: Sidecar; | ||
| try { | ||
| child = startSidecar(); | ||
| await child.ready; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add a bounded wait for sidecar readiness and for each request.
Neither await child.ready at Line 209 nor the request promise has a timeout. If the Node process stalls, for example during the one-time model download described in ARCHITECTURE.md Line 57, or if it stops writing to stdout without exiting, every caller waits indefinitely. src/cli.ts doctor and plugin/episodic-memory.ts episodic_search both await these promises directly, so the user sees a hang rather than the documented lexical fallback.
A single fixed timeout is not correct here, because the first-run model download is legitimately slow. Consider one of these:
- Have
src/embed-sidecar.mjsemit periodic progress messages during model load, and time out only on silence. - Apply a generous readiness timeout controlled by an
EPISODIC_*environment variable, plus a shorter per-request timeout that applies after readiness resolves.
Do you want me to open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/embed.ts` around lines 205 - 209, Update requestSidecar to bound both
await child.ready and the per-request promise so callers cannot hang
indefinitely. Use separate, configurable EPISODIC_* timeout values: a generous
readiness timeout that accommodates first-run model downloads and a shorter
request timeout applied only after readiness succeeds; preserve the existing
retry and lexical-fallback behavior when either timeout expires.
| return await new Promise<Float32Array[]>((resolve, reject) => { | ||
| child.pending.set(id, { resolve, reject, count: texts.length }); | ||
| try { | ||
| child.process.stdin.write(`${JSON.stringify({ id, texts })}\n`); | ||
| } catch (error) { | ||
| child.pending.delete(id); | ||
| const unavailable = new SidecarUnavailableError(`Could not write to embedding sidecar: ${String(error)}`); | ||
| sidecarGone(child, unavailable); | ||
| reject(unavailable); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
A request registered after the sidecar dies can hang forever.
startSidecar() returns child at Line 208 and the code awaits child.ready at Line 209. The child can then exit before Line 212 runs. In that window:
- The
exitedhandler at Line 196 already ran and already calledrejectAll, which clearedchild.pending. - Line 212 inserts a new entry into that now-orphaned map.
- No further code path rejects it.
drainStdouthas ended andexiteddoes not fire twice. - The write at Line 214 targets a closed stdin. A synchronous throw is not guaranteed, so the
catchat Line 215 may not run.
The caller then awaits a promise that never settles. plugin/episodic-memory.ts Line 90 awaits embedQuery with no timeout, so episodic_search would hang instead of returning the lexical-fallback message.
Check liveness after registration, and reject if the sidecar was already replaced.
🐛 Proposed fix
return await new Promise<Float32Array[]>((resolve, reject) => {
+ if (sidecar !== child) {
+ reject(new SidecarUnavailableError("Embedding sidecar became unavailable before the request was sent"));
+ return;
+ }
child.pending.set(id, { resolve, reject, count: texts.length });
try {
child.process.stdin.write(`${JSON.stringify({ id, texts })}\n`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/embed.ts` around lines 211 - 221, Update the request flow after
registering the pending entry in the embedding method around child.pending.set
to recheck sidecar liveness and identity before writing. If the sidecar has
exited or has already been replaced, remove the newly registered request and
reject it with the existing SidecarUnavailableError path; otherwise preserve the
normal stdin write behavior.
1 participant
Summary
EPISODIC_EMBED_MODE=inlineas an explicit lazy escape hatch; it is never selected automatically after a sidecar failure.Corrected Root Cause
plugin/episodic-memory.tsimportssrc/embed.ts, which previously statically imported Transformers.js. Its Node export loadsonnxruntime-nodeandsharpat module-import time, so plugin import itself exposed OpenCode's embedded Bun process to native addon teardown defects.episodic_readdoes not embed and is not an embedding trigger. This change does not attribute the shutdown failure specifically toonnxruntime-web; the Node ONNX Runtime build may itself expose WebGPU symbols.Configuration
EPISODIC_EMBED_MODE=sidecar, using Node 20+ fromEPISODIC_NODE_BINARY(defaultnode).EPISODIC_EMBED_MODE=inline; unsafe on affected OpenCode/Bun versions because it loads native addons in Bun.episodic_readremain embedding-free when Node is unavailable.Verification
bun run typecheckbun test(40 passing; fake-sidecar lifecycle/protocol coverage)bun run spikes/plugin-harness.tsbash spikes/pack-smoke.sh(clean packaged install; real Node-sidecar embed: 768 dimensions, norm 1.0000)doctordiagnostic and short-lived Bun-host sidecar cleanup test.Remaining Local Check
The macOS-only real OpenCode
/exitshutdown regression cannot run on this Linux host. Please verify locally that OpenCode exits cleanly after semantic search or indexing using the default sidecar mode.Summary by CodeRabbit
New Features
Documentation
Tests