fix: ship sqlite runtime modules + packaging guard + Chroma error logging#1
Merged
Merged
Conversation
…ace MODULE_NOT_FOUND errors
parseFileList is pure (only imports logger, no bun:sqlite), so make it a
static import inlined into the worker bundle instead of a lazy createRequire
loader — the more robust fix for the observation hot path. SessionStore still
requires the lazy loader (it pulls bun:sqlite), so short-circuit it behind the
injected store override. Make MODULE_NOT_FOUND-class errors visible: Bun's
ResolveMessage/BuildMessage are not instanceof Error and serialize as {}, so
treat any object with a string message as error-like, and normalize the
fire-and-forget catch sites that swallowed them.
Co-authored-by: bionicbutterfly13 <bionicbutterfly13@users.noreply.github.com>
Co-authored-by: KaizenPrompt <KaizenPrompt@users.noreply.github.com>
Co-authored-by: Garcia6l20 <Garcia6l20@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
…n/sqlite, add relative-require guards
The worker reaches SessionStore via a runtime createRequire('../sqlite/
SessionStore.js') that esbuild leaves as a bare string literal, so the build
must emit SessionStore.js as a loose sibling of the worker bundle (thedotmack#3091/
thedotmack#3092/thedotmack#3107). Allowlist plugin/sqlite in the npm tarball so it actually ships.
Add two static guards that assert every relative require target in
plugin/scripts/*.cjs resolves to an emitted sibling: a fast build-time check
(build-hooks) and a tarball check (smoke:clean-room PART 3).
Co-authored-by: bionicbutterfly13 <bionicbutterfly13@users.noreply.github.com>
Co-authored-by: KaizenPrompt <KaizenPrompt@users.noreply.github.com>
Co-authored-by: Garcia6l20 <Garcia6l20@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
… require Loose sibling of the worker bundle that ChromaSync's lazy loader resolves at runtime. bun:sqlite kept external so it stays out of the SDK bundle. Co-authored-by: bionicbutterfly13 <bionicbutterfly13@users.noreply.github.com> Co-authored-by: KaizenPrompt <KaizenPrompt@users.noreply.github.com> Co-authored-by: Garcia6l20 <Garcia6l20@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
…e resolution Asserts SessionStore.js is emitted next to the worker bundle and resolves the way ChromaSync requires it, and that parseFileList is inlined (no ../sqlite/observations/files.js require survives, and no loose files.js ships). Co-authored-by: bionicbutterfly13 <bionicbutterfly13@users.noreply.github.com> Co-authored-by: KaizenPrompt <KaizenPrompt@users.noreply.github.com> Co-authored-by: Garcia6l20 <Garcia6l20@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
…st + logger fix
worker-service.cjs now inlines parseFileList (Task 1) and no longer emits a lazy
createRequire('../sqlite/observations/files.js') — leaving the old bundle
committed would reference a module the build deliberately stops shipping. The
other bundles pick up the logger isErrorLike hardening (Task 6). External require
sets are unchanged across all five (no dependency drift). Repo convention ships
rebuilt bundles alongside source changes (cf. 29af028).
Co-authored-by: bionicbutterfly13 <bionicbutterfly13@users.noreply.github.com>
Co-authored-by: KaizenPrompt <KaizenPrompt@users.noreply.github.com>
Co-authored-by: Garcia6l20 <Garcia6l20@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
…es build Pre-merge review found the build-time guard ran right after emitting SessionStore.js but before server-service/mcp-server/context-generator/ transcript-watcher were rebuilt, so it scanned stale copies of 4 of the 5 bundles — defeating the "just-built" intent for those targets (smoke:clean-room PART 3 still backstopped the real ship gate). Move the single guard call to after all five plugin/scripts esbuild outputs are written so it scans fresh output for every bundle. Bundle contents are unchanged by this move. Co-authored-by: bionicbutterfly13 <bionicbutterfly13@users.noreply.github.com> Co-authored-by: KaizenPrompt <KaizenPrompt@users.noreply.github.com> Co-authored-by: Garcia6l20 <Garcia6l20@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
plugin/sqlite/SessionStore.js shipped under plugin/package.json's "type":"module": it works under Bun (the real worker runtime) but Node refuses to require() a .js file inside an ESM package. Emitting it as .cjs removes that latent Node-consumer foot-gun — cheap and strictly safer; nothing about the Bun path changes. - scripts/build-hooks.js: emit plugin/sqlite/SessionStore.cjs (same esbuild opts: external ['bun:sqlite'], CJS format). - src/services/sync/ChromaSync.ts: loadSessionStoreCtor() runtime createRequire specifier "../sqlite/SessionStore.js" -> ".cjs" (the type-only import stays ".js" — TS resolves it to the .ts source). - Delete the stale committed plugin/sqlite/SessionStore.js (renamed to .cjs) so we never ship both. - Regenerate plugin/scripts/*.cjs; only worker-service.cjs changes (ChromaSync is reachable only from the worker bundle). - Regression test: resolve ../sqlite/SessionStore.cjs; negative control re-verified (remove file -> MODULE_NOT_FOUND). Gates green: typecheck, build (relative-require guard resolves ../sqlite/SessionStore.cjs), check:sdk-bundle (no bun:sqlite leak), smoke:clean-room PART 3 (tarball scan finds SessionStore.cjs), npm pack (.cjs in tarball, .js absent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WSGNPpdcNkTexMTXf9y2f
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where the worker bundle relied on runtime createRequire('../sqlite/...') for SQLite modules that were not emitted/shipped, breaking Chroma vector sync with MODULE_NOT_FOUND errors that were effectively invisible due to {}-style serialization.
Changes:
- Switches
parseFileListto a static import inChromaSync.ts(so it’s inlined into the worker bundle) while keepingSessionStoreas a lazily-required, separately emittedplugin/sqlite/SessionStore.cjs. - Adds build- and pack-time “relative require closure” guards to prevent shipping bundles with unresolved sibling requires.
- Improves error logging/normalization so Bun’s non-
Errorresolve/build failures produce useful messages (and adds a regression test).
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/worker-service-lazy-sqlite-modules.test.ts | Regression test ensuring SessionStore.cjs is present/resolvable and parseFileList is inlined (no loose files emitted). |
| src/utils/logger.ts | Adds isErrorLike() and updates error formatting/routing behavior for Bun non-Error thrown types. |
| src/services/worker/http/routes/SessionRoutes.ts | Normalizes caught values into Error when logging Chroma prompt sync failures. |
| src/services/worker/agents/ResponseProcessor.ts | Normalizes caught values into Error when logging Chroma sync failures. |
| src/services/worker-service.ts | Normalizes caught values into Error for background/backfill error logging. |
| src/services/sync/ChromaSync.ts | Uses static parseFileList import; updates lazy require target to SessionStore.cjs. |
| scripts/smoke-clean-room.cjs | Adds PART 3 tarball-time guard to validate relative require targets in packed plugin/scripts/*.cjs. |
| scripts/build-hooks.js | Emits plugin/sqlite/SessionStore.cjs and adds build-time relative-require guard over plugin/scripts/*.cjs. |
| plugin/sqlite/SessionStore.cjs | Adds the emitted CJS runtime SQLite module to be required by the worker at runtime. |
| plugin/scripts/context-generator.cjs | Rebuilt bundle incorporating updated logger behavior (artifact update). |
| package.json | Ensures plugin/sqlite is included in the published package files allowlist. |
Comment on lines
341
to
350
| private routeErrorToSink(message: string, context?: LogContext, data?: any): void { | ||
| try { | ||
| if (!errorSink || !(data instanceof Error)) return; | ||
| if (!errorSink || !isErrorLike(data)) return; | ||
| // Pass the message as context so the sink can fingerprint on it too; the | ||
| // sink (captureException) scrubs everything through error-scrub / | ||
| // scrubProperties, so an unsafe message here cannot leak — but `message` | ||
| // is not whitelisted, so it is dropped by scrubProperties anyway. We pass | ||
| // only the Error itself; context is intentionally minimal. | ||
| errorSink(data); | ||
| errorSink(data instanceof Error ? data : new Error(data.message)); | ||
| } catch { |
Comment on lines
+344
to
+372
| // those parts cannot catch a missing sibling. Statically assert every relative | ||
| // require target inside the PACKED plugin/scripts/*.cjs resolves inside the | ||
| // tarball. This is the guard that would have caught 13.9.x shipping without | ||
| // plugin/sqlite/. | ||
| function checkPluginRelativeRequires(failures, pkgRoot) { | ||
| log('\nPART 3 — plugin relative-require closure (#3091/#3092/#3107 guard)'); | ||
|
|
||
| const scriptsDir = path.join(pkgRoot, 'plugin', 'scripts'); | ||
| if (!fs.existsSync(scriptsDir)) { | ||
| failures.push(`packed tarball is missing plugin/scripts (${scriptsDir})`); | ||
| return; | ||
| } | ||
|
|
||
| const RELATIVE_SPECIFIER = /["'](\.\.?\/[A-Za-z0-9_./-]+\.(?:c?js))["']/g; | ||
| let checked = 0; | ||
| for (const entry of fs.readdirSync(scriptsDir)) { | ||
| if (!entry.endsWith('.cjs')) continue; | ||
| const filePath = path.join(scriptsDir, entry); | ||
| const content = fs.readFileSync(filePath, 'utf8'); | ||
| const seen = new Set(); | ||
| let m; | ||
| while ((m = RELATIVE_SPECIFIER.exec(content)) !== null) { | ||
| const spec = m[1]; | ||
| if (seen.has(spec)) continue; | ||
| seen.add(spec); | ||
| checked++; | ||
| const resolved = path.resolve(path.dirname(filePath), spec); | ||
| if (!fs.existsSync(resolved)) { | ||
| failures.push( |
Comment on lines
+67
to
+83
| function assertPluginRelativeRequiresResolve(scriptsDir) { | ||
| const RELATIVE_SPECIFIER = /["'](\.\.?\/[A-Za-z0-9_./-]+\.(?:c?js))["']/g; | ||
| const failures = []; | ||
| for (const entry of fs.readdirSync(scriptsDir)) { | ||
| if (!entry.endsWith('.cjs')) continue; | ||
| const filePath = path.join(scriptsDir, entry); | ||
| const content = fs.readFileSync(filePath, 'utf-8'); | ||
| const seen = new Set(); | ||
| let m; | ||
| while ((m = RELATIVE_SPECIFIER.exec(content)) !== null) { | ||
| const spec = m[1]; | ||
| if (seen.has(spec)) continue; | ||
| seen.add(spec); | ||
| const resolved = path.resolve(path.dirname(filePath), spec); | ||
| if (!fs.existsSync(resolved)) { | ||
| failures.push(`${entry} requires "${spec}" but ${path.relative(scriptsDir, resolved)} was not emitted`); | ||
| } |
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
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.
Fixes the 13.9.x regression where observation → Chroma vector sync was 100% broken:
worker-service.cjslazy-requires../sqlite/*modules the build never emitted / the npm allowlist never shipped, so every observation threwMODULE_NOT_FOUND(swallowed to{}).Changes (hybrid A/B)
parseFileList→ static import inChromaSync.ts(inlined into the worker bundle; kills the runtime-require foot-gun on the hot path; SDK bundle stays free ofbun:sqlite).SessionStore→ emitted toplugin/sqlite/SessionStore.cjs+ added topackage.json#files; backfill short-circuits to the injected store.build-hooks.js+ clean-roomnpm packPART 3 — every relativerequire("../…")inplugin/scripts/*.cjsmust resolve inside the tarball, or the build/pack fails.isErrorLike()inlogger.tsso Bun's non-ErrorResolveMessageserializes its.message/.stackinstead of{}; catch sites normalized inworker-service.ts/ResponseProcessor.ts/SessionRoutes.ts.Gates
typecheck ✅ · build (guard) ✅ · check:sdk-bundle (no bun:sqlite leak) ✅ · smoke:clean-room PART 3 ✅ ·
npm packassertsSessionStore.cjsin /.jsout ✅ · Bun runtime resolve ✅ · regression 4/4 ✅. Single-user local mode unaffected. Live-Chroma UAT validated separately (observation flow + backfill init); the only unrun step was DNS-env-blocked, not a defect.Consolidates the approaches from community PRs thedotmack#3102 / thedotmack#3108 / thedotmack#3116.
Co-authored-by: bionicbutterfly13 bionicbutterfly13@users.noreply.github.com
Co-authored-by: KaizenPrompt KaizenPrompt@users.noreply.github.com
Co-authored-by: Garcia6l20 Garcia6l20@users.noreply.github.com