feat: add cross-platform Node.js and Windows support#6
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds cross-runtime support (Bun + Node.js ≥20) with a platform-agnostic HTTP server and a pluggable SQLite abstraction; tightens database typing across services; introduces Node-focused build/test tooling and Vitest migration; updates tests, scripts, and README accordingly. ChangesCross-Platform Runtime, SQLite Pluggability, Tooling & Test Migration
sequenceDiagram 🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 777123f7d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| constructor(path: string) { | ||
| // Dynamic require to avoid hard dependency when Bun is available | ||
| const BetterSqlite3 = require("better-sqlite3") as typeof import("better-sqlite3"); |
There was a problem hiding this comment.
Load better-sqlite3 without bare require
In the new Node fallback path, the package still runs as ESM (package.json has "type": "module"), so a bare require is not defined at runtime. After getDatabase() catches the missing bun:sqlite module and selects BetterSqlite3Database, constructing the first SQLite connection reaches this line and throws ReferenceError: require is not defined, preventing the plugin from starting on Node. Use createRequire(import.meta.url) or an ESM-compatible import path for the fallback.
Useful? React with 👍 / 👎.
| process.exit(1); | ||
| } | ||
|
|
||
| const result = spawnSync(process.execPath, [tscPath], { stdio: "inherit" }); |
There was a problem hiding this comment.
Invoke the TypeScript compiler via a JS entrypoint
On Windows installs produced by npm, node_modules/.bin/tsc is a command shim rather than the TypeScript JavaScript entrypoint, so spawnSync(process.execPath, [tscPath]) makes Node try to parse the shim and the cross-platform build fails before copying assets. This affects the Windows support this script is meant to add; invoke the package entrypoint such as node_modules/typescript/bin/tsc, or spawn the shim directly with the appropriate shell/platform handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the plugin to run cross-platform (including Windows) by adding runtime abstractions for SQLite and HTTP serving, migrating the test suite to Vitest, and replacing shell-based build steps with Node.js APIs.
Changes:
- Added runtime abstraction layers for HTTP serving (Bun.serve → Node http) and SQLite (bun:sqlite → better-sqlite3).
- Migrated the test suite from
bun:testtovitestand introduced a Vitest config. - Replaced the build script with a Node.js-based implementation and updated docs/package metadata for Node.js support.
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds Vitest runner configuration for Node-based tests. |
| tests/windows-path.test.ts | Migrates test imports to Vitest. |
| tests/vector-search-backend-integration.test.ts | Migrates test imports to Vitest. |
| tests/vector-backends/usearch-backend.test.ts | Migrates test imports to Vitest. |
| tests/vector-backends/migration-fallback.test.ts | Migrates test imports to Vitest. |
| tests/vector-backends/exact-scan-backend.test.ts | Migrates test imports to Vitest. |
| tests/vector-backends/backend-factory.test.ts | Migrates test imports to Vitest. |
| tests/tool-scope.test.ts | Reworks mocking strategy for Vitest module mocking. |
| tests/tags.test.ts | Migrates test imports to Vitest. |
| tests/project-scope.test.ts | Migrates test imports to Vitest. |
| tests/profile-write.test.ts | Migrates test imports to Vitest. |
| tests/profile-tool-runtime.test.ts | Migrates runtime mocking from Bun’s mock API to Vitest (vi). |
| tests/privacy.test.ts | Migrates test imports to Vitest. |
| tests/plugin-loader-contract.test.ts | Migrates to Vitest and adds an init timeout guard. |
| tests/opencode-provider.test.ts | Migrates to Vitest and replaces spyOn with a module-mock approach. |
| tests/openai-chat-completion-provider.test.ts | Migrates test imports to Vitest. |
| tests/memory-scope.test.ts | Migrates module mocks from Bun to Vitest. |
| tests/memory-engine.test.ts | Migrates module mocks from Bun to Vitest. |
| tests/language-detector.test.ts | Migrates test imports to Vitest. |
| tests/config.test.ts | Migrates test imports to Vitest. |
| tests/config-resolution.test.ts | Migrates filesystem spies to Vitest mocking. |
| tests/anthropic-provider.test.ts | Migrates test imports to Vitest. |
| tests/ai-provider-config.test.ts | Migrates test imports to Vitest. |
| src/services/web-server.ts | Switches server startup from Bun.serve to a platform serve() abstraction. |
| src/services/web-server-worker.ts | Switches worker server startup to the platform serve() abstraction. |
| src/services/user-prompt/user-prompt-manager.ts | Updates SQLite typing to use the new Database abstraction. |
| src/services/user-profile/user-profile-manager.ts | Updates SQLite typing and localizes some utility logic. |
| src/services/sqlite/vector-search.ts | Updates SQLite typing to use the new Database abstraction. |
| src/services/sqlite/transcript-manager.ts | Updates SQLite typing to use the new Database abstraction. |
| src/services/sqlite/sqlite-bootstrap.ts | Introduces a Bun/better-sqlite3 compatibility layer and Database/Statement wrappers. |
| src/services/sqlite/shard-manager.ts | Updates SQLite typing to use the new Database abstraction. |
| src/services/sqlite/connection-manager.ts | Updates connection manager typing to use the new Database abstraction. |
| src/services/platform-server.ts | Adds Bun/Node HTTP server abstraction (Bun.serve fallback to Node http). |
| src/services/memory-scoring-service.ts | Adjusts SQLite bootstrap typing in the scoring service. |
| src/services/memory-conflicts.ts | Updates SQLite typing and adds a new import related to scoring. |
| src/services/ai/session/ai-session-manager.ts | Updates SQLite typing to use the new Database abstraction. |
| scripts/migrate-tests.mjs | Adds a migration helper script for bun:test → vitest conversions. |
| scripts/build.mjs | Adds a Node-based build script for compiling + copying web assets. |
| README.md | Updates documentation to reflect Node.js support and platform requirements. |
| package.json | Updates scripts, engines, and dependencies for Node.js + Vitest support. |
| package-lock.json | Locks new dependencies (better-sqlite3, vitest, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const tscPath = resolve("./node_modules/.bin/tsc"); | ||
| if (!existsSync(tscPath)) { | ||
| console.error("Error: tsc not found. Run npm install or bun install first."); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const result = spawnSync(process.execPath, [tscPath], { stdio: "inherit" }); | ||
| if (result.status !== 0) { | ||
| process.exit(result.status ?? 1); |
| constructor(path: string) { | ||
| // Dynamic require to avoid hard dependency when Bun is available | ||
| const BetterSqlite3 = require("better-sqlite3") as typeof import("better-sqlite3"); | ||
| this.db = new BetterSqlite3(path); | ||
| } |
| export function getDatabase(): new (path: string) => Database { | ||
| if (!DatabaseImpl) { | ||
| try { | ||
| const bunSqlite = require("bun:sqlite") as typeof import("bun:sqlite"); | ||
| DatabaseImpl = bunSqlite.Database as unknown as new (path: string) => Database; | ||
| } catch { | ||
| DatabaseImpl = BetterSqlite3Database; | ||
| } | ||
| } |
| const nodeServer = createServer(async (req: IncomingMessage, res: ServerResponse) => { | ||
| try { | ||
| const url = `http://${req.headers.host}${req.url}`; | ||
|
|
||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of req) { | ||
| chunks.push(chunk); | ||
| } | ||
| const body = chunks.length > 0 ? Buffer.concat(chunks) : undefined; | ||
|
|
||
| const request = new Request(url, { | ||
| method: req.method, | ||
| headers: new Headers(req.headers as Record<string, string>), | ||
| body: body && body.length > 0 ? body : undefined, | ||
| }); |
| async function createNodeServer(options: ServeOptions): Promise<PlatformServer> { | ||
| const requestIPs = new WeakMap<Request, string>(); | ||
|
|
||
| const nodeServer = createServer(async (req: IncomingMessage, res: ServerResponse) => { | ||
| try { | ||
| const url = `http://${req.headers.host}${req.url}`; |
| import { log } from "./logger.js"; | ||
| import { CONFIG } from "../config.js"; | ||
| import type { MemoryConflict } from "./sqlite/types.js"; | ||
| import { calculateInterference } from "./memory-scoring.js"; |
| @@ -1,4 +1,4 @@ | |||
| import { getDatabase } from "./sqlite-bootstrap.js"; | |||
| import { getDatabase, type Database } from "./sqlite-bootstrap.js"; | |||
| @@ -1,4 +1,4 @@ | |||
| import { getDatabase } from "./sqlite-bootstrap.js"; | |||
| import { getDatabase, type Database } from "./sqlite-bootstrap.js"; | |||
| import { getDatabase, type Database } from "./sqlite-bootstrap.js"; | ||
| import { existsSync, mkdirSync } from "node:fs"; | ||
| import { join, dirname } from "node:path"; | ||
| import { log } from "../logger.js"; | ||
| import { CONFIG } from "../../config.js"; | ||
| import { connectionManager } from "./connection-manager.js"; |
| function migrateFile(filePath: string): void { | ||
| let content = readFileSync(filePath, "utf-8"); | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/memory-engine.test.ts (1)
260-280:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset the mocked
CONFIGbetween tests.This mock returns a shared object, but the suite mutates
CONFIG.transcriptStorage.enabledlater. If that test exits before manual cleanup, the mutated flag bleeds into the rest of the file and makes follow-up failures noisy.Proposed fix
-beforeEach(() => { +beforeEach(async () => { dbByPath.clear(); contextTracker.clear(); + const { CONFIG } = await import("../src/config.js"); + CONFIG.transcriptStorage.enabled = true; });Also applies to: 301-304
🤖 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 `@tests/memory-engine.test.ts` around lines 260 - 280, The test mock currently returns a shared CONFIG object (used by vi.mock) so mutations like CONFIG.transcriptStorage.enabled leak between tests; change the mock to produce a fresh CONFIG per test or reset the mock between tests — e.g., replace the static object factory with one that constructs and returns a new CONFIG object each time (referencing CONFIG and transcriptStorage.enabled), or call vi.resetModules()/re-mock in a beforeEach/afterEach so each test gets an unmutated CONFIG instance.
🧹 Nitpick comments (2)
src/services/platform-server.ts (1)
18-21: 💤 Low valueConsider handling missing Host header.
req.headers.hostcan beundefined(e.g., HTTP/1.0 clients), which would result in a URL likehttp://undefined/path. Consider falling back to the server's hostname from options.💡 Suggested fallback
- const url = `http://${req.headers.host}${req.url}`; + const host = req.headers.host || `${options.hostname}:${options.port}`; + const url = `http://${host}${req.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/services/platform-server.ts` around lines 18 - 21, The URL construction inside the createServer callback (nodeServer) currently uses req.headers.host which can be undefined; update the handler to guard against a missing Host header by deriving host = req.headers.host ?? options.hostname ?? 'localhost' (or another safe default) and then build the full URL from that host and req.url; modify the code that constructs url (`http://${req.headers.host}${req.url}`) to use this fallback host so the URL is valid for HTTP/1.0 clients and when Host is absent.src/services/sqlite/sqlite-bootstrap.ts (1)
64-74: ⚡ Quick winConsider creating a wrapper for bun:sqlite.Database to maintain type safety instead of using
as unknown ascast.The
as unknown ascast on line 68 bypasses type checking without evidence of necessity. Whilebun:sqlite.Databaselikely supports the standard SQLite APIs your code relies on (prepare(),run(),exec(),close()), this cast removes compile-time type verification for future maintenance. Following the same pattern asBetterSqlite3Database, wrappingbun:sqlite.Databaseto explicitly implement yourDatabaseinterface would provide both type safety and self-documenting API compatibility expectations.🤖 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/services/sqlite/sqlite-bootstrap.ts` around lines 64 - 74, Replace the unchecked cast of bun:sqlite.Database in getDatabase by creating a thin wrapper class that implements our Database interface (similar to BetterSqlite3Database) and delegates methods prepare/run/exec/close to an inner bun:sqlite.Database instance; instantiate and assign that wrapper to DatabaseImpl instead of using "as unknown as" so type safety is preserved while still using bun:sqlite when available (refer to getDatabase, DatabaseImpl, BetterSqlite3Database and the bun:sqlite.Database usage).
🤖 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 `@README.md`:
- Around line 344-347: The README currently lists the Node commands (npm run
build, npm test) only under the "### Build" section, which makes the Node test
workflow easy to miss; update the "Testing" section to mirror the Node commands
by adding the same npm commands (npm run build and npm test) alongside the
existing Bun instructions so readers see both Bun and Node test workflows;
locate the "Testing" heading in README.md and append a short Node subsection or
inline note showing the exact npm commands to run the tests.
In `@scripts/build.mjs`:
- Around line 7-15: The Windows failure comes from checking for
"./node_modules/.bin/tsc" which is a platform-specific shim; instead resolve the
actual TypeScript JS entry and spawn Node with that file. Use
createRequire(import.meta.url) to call require.resolve('typescript/bin/tsc') (or
'typescript/bin/tsc.js') to get the real script path, replace the current
tscPath/existsSync check with that resolved path, and keep using
spawnSync(process.execPath, [tscPath], { stdio: "inherit" }) and the existing
result.status handling so the build works cross-platform.
In `@scripts/migrate-tests.mjs`:
- Line 4: The .mjs script contains TypeScript type annotations causing a runtime
parse error; remove the TypeScript signatures (e.g., change the function
declaration "function migrateFile(filePath: string): void {" to "function
migrateFile(filePath) {" and likewise strip any other ": type" or return type
annotations), or alternatively rename the file to a .ts and convert it to a
proper TypeScript module; ensure you apply the same change where the same
annotation appears (the second occurrence noted around the other function at the
referenced location).
- Around line 39-43: The current automatic regex that replaces "Bun.spawnSync"
with "spawnSync(" is unsafe because Bun.spawnSync and
node:child_process.spawnSync have incompatible signatures; update
scripts/migrate-tests.mjs so that instead of performing content =
content.replace(/Bun\.spawnSync\s*\(\s*\{/g, "spawnSync("), the script detects
any occurrence of "Bun.spawnSync" (or matches the regex /Bun\.spawnSync\s*\(/)
and injects a clear manual-migration marker/comment (e.g., "//
MANUAL_MIGRATION_REQUIRED: Bun.spawnSync usage — convert to spawnSync(command,
args, options) and add appropriate import") or throw an explicit error listing
the file, so the call sites (Bun.spawnSync) are flagged for human review; do not
attempt to auto-rewrite the call shape or add imports.
In `@src/services/ai/session/ai-session-manager.ts`:
- Around line 2-4: The constructor in ai-session-manager.ts opens
"ai-sessions.db" before ensuring CONFIG.storagePath exists; modify the
AISessionManager constructor (or the init code that calls open on
ai-sessions.db) to compute the dbDir = dirname(join(CONFIG.storagePath,
"ai-sessions.db")) and call existsSync(dbDir) || mkdirSync(dbDir, { recursive:
true }) (using join, dirname, existsSync, mkdirSync) before
opening/instantiating the database, and apply the same pre-check to the other
place referenced (the block around lines 24-28) so the storage directory is
always created prior to opening the DB.
In `@src/services/platform-server.ts`:
- Around line 45-48: The catch block currently sends String(error) to the client
which can leak internals; change the handler around the catch that uses res (the
response object in this module) to log the full error server-side (e.g., using
console.error or the module's logger) and respond with a generic, non-sensitive
message and appropriate status (500) — for example send a fixed plain-text or
JSON error payload like "Internal server error" instead of String(error) while
preserving the existing res.statusCode = 500 assignment.
- Around line 28-32: The current creation of the Request uses new
Headers(req.headers as Record<string,string>) which can lose multi-value headers
(e.g., Set-Cookie). Replace the direct cast by building a Headers instance and
iterating over req.headers: for each header key, if the value is an array append
each item with Headers.append, if it's a string set or append once; then pass
that Headers object into the Request constructor (references: the request
variable, req.headers, Headers, Request).
In `@tests/tool-scope.test.ts`:
- Around line 29-31: Remove the vi.resetModules() calls used in the tests (e.g.,
inside createPlugin) and switch to a mutable mock-state pattern: hoist the mock
objects to the top of the test scope (so they persist across imports) and in
beforeEach/afterEach mutate their properties to the desired state for each test
instead of resetting the module cache; update the tests that referenced
vi.resetModules() to reinitialize mock fields on the hoisted mock objects (and
optionally call vi.clearAllMocks()/vi.resetAllMocks() for call history) so tests
remain compatible with Bun’s partial vi API.
---
Outside diff comments:
In `@tests/memory-engine.test.ts`:
- Around line 260-280: The test mock currently returns a shared CONFIG object
(used by vi.mock) so mutations like CONFIG.transcriptStorage.enabled leak
between tests; change the mock to produce a fresh CONFIG per test or reset the
mock between tests — e.g., replace the static object factory with one that
constructs and returns a new CONFIG object each time (referencing CONFIG and
transcriptStorage.enabled), or call vi.resetModules()/re-mock in a
beforeEach/afterEach so each test gets an unmutated CONFIG instance.
---
Nitpick comments:
In `@src/services/platform-server.ts`:
- Around line 18-21: The URL construction inside the createServer callback
(nodeServer) currently uses req.headers.host which can be undefined; update the
handler to guard against a missing Host header by deriving host =
req.headers.host ?? options.hostname ?? 'localhost' (or another safe default)
and then build the full URL from that host and req.url; modify the code that
constructs url (`http://${req.headers.host}${req.url}`) to use this fallback
host so the URL is valid for HTTP/1.0 clients and when Host is absent.
In `@src/services/sqlite/sqlite-bootstrap.ts`:
- Around line 64-74: Replace the unchecked cast of bun:sqlite.Database in
getDatabase by creating a thin wrapper class that implements our Database
interface (similar to BetterSqlite3Database) and delegates methods
prepare/run/exec/close to an inner bun:sqlite.Database instance; instantiate and
assign that wrapper to DatabaseImpl instead of using "as unknown as" so type
safety is preserved while still using bun:sqlite when available (refer to
getDatabase, DatabaseImpl, BetterSqlite3Database and the bun:sqlite.Database
usage).
🪄 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: 014a4ff0-a064-432e-91ce-eabb91d47dec
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
README.mdpackage.jsonscripts/build.mjsscripts/migrate-tests.mjssrc/services/ai/session/ai-session-manager.tssrc/services/memory-conflicts.tssrc/services/memory-scoring-service.tssrc/services/platform-server.tssrc/services/sqlite/connection-manager.tssrc/services/sqlite/shard-manager.tssrc/services/sqlite/sqlite-bootstrap.tssrc/services/sqlite/transcript-manager.tssrc/services/sqlite/vector-search.tssrc/services/user-profile/user-profile-manager.tssrc/services/user-prompt/user-prompt-manager.tssrc/services/web-server-worker.tssrc/services/web-server.tstests/ai-provider-config.test.tstests/anthropic-provider.test.tstests/config-resolution.test.tstests/config.test.tstests/language-detector.test.tstests/memory-engine.test.tstests/memory-scope.test.tstests/openai-chat-completion-provider.test.tstests/opencode-provider.test.tstests/plugin-loader-contract.test.tstests/privacy.test.tstests/profile-tool-runtime.test.tstests/profile-write.test.tstests/project-scope.test.tstests/tags.test.tstests/tool-scope.test.tstests/vector-backends/backend-factory.test.tstests/vector-backends/exact-scan-backend.test.tstests/vector-backends/migration-fallback.test.tstests/vector-backends/usearch-backend.test.tstests/vector-search-backend-integration.test.tstests/windows-path.test.tsvitest.config.ts
…ocs, ESM compatibility
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…resetModules and reset CONFIG mock state
Fixes GitHub Code Scanning alert #6. Adds permissions: contents: read to restrict GITHUB_TOKEN scope following principle of least privilege.
Summary
Adds full cross-platform support so the plugin runs on Node.js 20+ and Windows, not just Bun on Linux/macOS.
Changes
bun:sqliteat runtime, falls back tobetter-sqlite3with compatibleDatabase/StatementwrappersBun.serve()when available, otherwise falls back to Node.jshttp.createServer()cpSync,mkdirSync)bun:testtovitestwith proper ESM mockingbetter-sqlite3andvitestdependencies, updatedenginesto allow"node": ">=20.0.0"Testing
All 173 tests pass on both Bun and Node.js runtimes.
Backward Compatibility
test:bunscript for existing Bun test workflowsSummary by CodeRabbit
New Features
Documentation
Chores / Tests