feat(server)!: replace PGlite with embedded Postgres; bundle pgvector; earthdistance geo#965
Merged
Conversation
Member
Author
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces PGlite with embedded Postgres 18, adds ChangesEmbedded Postgres and pgvector integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.github/workflows/build-pgvector-embedded.yml:
- Line 30: The workflow reads PGVECTOR_VERSION from the inputs context
(PGVECTOR_VERSION: ${{ inputs.pgvector_version || 'v0.8.1' }}) which only exists
for workflow_dispatch/workflow_call and is unavailable on push; change the
workflow to handle cross-event triggers by either restricting the trigger to
workflow_dispatch if an input is required, or guard usage with conditionals that
check github.event_name (e.g., only read inputs when github.event_name ==
'workflow_dispatch') and provide a reliable default for push; update any
jobs/steps that reference inputs or github.event.inputs accordingly so they run
only when inputs are present or fall back to the intended default behavior.
In `@packages/pgvector-embedded/package.json`:
- Line 3: Remove the hand-edited "version" property from the package.json
manifest so release-please can manage versions automatically; locate the
"version" key in packages/pgvector-embedded/package.json (the "version": "8.0.0"
entry) and delete that property entirely (do not replace it with a manual
placeholder), and ensure you do not add chore(release) commits — let the release
automation own version bumps.
In `@packages/server/src/__tests__/setup/embedded-postgres-backend.ts`:
- Around line 29-31: startEmbeddedBackend currently checks "if (active) return
active" which is racy under concurrent callers; add an in-flight promise guard
so only one startup runs. Introduce a module-scoped "startingPromise" (or
similar) that is checked/returned before spawning, set it to the startup Promise
when beginning initialization inside startEmbeddedBackend, await it, set
"active" on success and clear "startingPromise" on resolve/reject; ensure
subsequent callers return startingPromise while startup is in-flight and that
startingPromise is nulled on failure so retries are possible.
- Around line 34-47: The test creates a temporary dataDir with mkdtempSync and
starts EmbeddedPostgres via pg.initialise() and pg.start(), but if either call
throws the temp directory is leaked; wrap the startup sequence for the instance
(the calls to pg.initialise and pg.start) in a try/catch (or try/finally) that
on any error stops/cleans up the EmbeddedPostgres instance if created and
removes the dataDir (using fs.rm/fs.rmdir with recursive true or equivalent)
before rethrowing the error; reference dataDir, pg.initialise, pg.start, and the
EmbeddedPostgres instance in your change so the cleanup runs on all failure
paths.
In `@packages/server/src/__tests__/setup/global-setup.ts`:
- Around line 28-35: The logs in global-setup.ts currently print raw connection
URLs (databaseUrl and embedded.url) which may include credentials; update the
logging to redact credentials before printing by parsing the URL strings used in
the existing console.log calls (the variable names databaseUrl and embedded.url
returned by startEmbeddedBackend()) and replacing the userinfo portion
(username:password@) with a placeholder like <redacted_credentials> (or omit
userinfo entirely) so only host/port/db are logged; keep the same log messages
but call a small helper or inline logic to sanitize the URL before passing it to
console.log.
In `@packages/server/src/benchmarks/memory/adapters/lobu-inprocess.ts`:
- Around line 341-348: The dispose() implementation currently returns early when
embeddedBackend is falsy, skipping closeDbSingleton() and leaving open DB
handles for external DBs (e.g. when LOBU_BENCHMARK_DATABASE_URL is used); update
dispose() so it always awaits closeDbSingleton() regardless of embeddedBackend
(call await closeDbSingleton() before returning or remove the early-return),
then if embeddedBackend exists stop it and set embeddedBackend = null and
databaseReady = false — reference the dispose() function, embeddedBackend
variable, and closeDbSingleton() call in your changes.
In `@packages/server/src/lobu/stores/__tests__/postgres-secret-store.test.ts`:
- Around line 3-4: The comment string in the test docblock still uses "pnpm"
commands ("pnpm test" and "pnpm test:pg"); update that text to use bun instead
(e.g., "bun test" and "bun test:pg") while leaving the DATABASE_URL reference
unchanged so the sentence now advertises bun-based commands consistently.
🪄 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: 280ec41b-d00c-418b-9182-ab6cc0e32e03
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/pgvector-embedded/prebuilt/darwin-arm64/vector.dylibis excluded by!**/*.dylib
📒 Files selected for processing (74)
.github/workflows/build-pgvector-embedded.ymlMakefiledb/migrations/20260520120000_geo_earthdistance.sqlpackages/cli/package.jsonpackages/cli/src/__tests__/dev.test.tspackages/cli/src/commands/dev.tspackages/cli/src/commands/init.tspackages/cli/src/index.tspackages/cli/src/internal/credentials.tspackages/connectors/src/apple_photos.tspackages/pgvector-embedded/package.jsonpackages/pgvector-embedded/prebuilt/darwin-arm64/vector--0.8.1.sqlpackages/pgvector-embedded/prebuilt/darwin-arm64/vector.controlpackages/pgvector-embedded/scripts/build.shpackages/pgvector-embedded/src/index.tspackages/pgvector-embedded/tsconfig.jsonpackages/server/package.jsonpackages/server/scripts/build-server-bundle.mjspackages/server/src/__tests__/integration/auth/single-user-signup.test.tspackages/server/src/__tests__/integration/geo-enrichment.test.tspackages/server/src/__tests__/integration/identity/engine.test.tspackages/server/src/__tests__/integration/sandbox/namespace-dispatch.test.tspackages/server/src/__tests__/server-lifecycle.test.tspackages/server/src/__tests__/setup/embedded-postgres-backend.tspackages/server/src/__tests__/setup/global-setup.tspackages/server/src/__tests__/setup/pglite-backend.tspackages/server/src/__tests__/setup/test-db.tspackages/server/src/benchmarks/memory/adapters/lobu-inprocess.tspackages/server/src/db/client.tspackages/server/src/dev-vite.tspackages/server/src/gateway/__tests__/agent-history-routes.test.tspackages/server/src/gateway/__tests__/agent-routes.test.tspackages/server/src/gateway/__tests__/agent-settings-store.test.tspackages/server/src/gateway/__tests__/agent-transcript-snapshot.test.tspackages/server/src/gateway/__tests__/base-deployment-grants.test.tspackages/server/src/gateway/__tests__/chat-instance-manager-boot.test.tspackages/server/src/gateway/__tests__/chat-instance-manager-slack.test.tspackages/server/src/gateway/__tests__/connection-routes.test.tspackages/server/src/gateway/__tests__/core-services-store-selection.test.tspackages/server/src/gateway/__tests__/grant-store.test.tspackages/server/src/gateway/__tests__/helpers/db-setup.tspackages/server/src/gateway/__tests__/instruction-service.test.tspackages/server/src/gateway/__tests__/interaction-bridge-action-handlers.test.tspackages/server/src/gateway/__tests__/interaction-bridge-slack-webhook.test.tspackages/server/src/gateway/__tests__/mcp-proxy-edge-cases.test.tspackages/server/src/gateway/__tests__/mcp-proxy.test.tspackages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.tspackages/server/src/gateway/__tests__/oauth-state-store.test.tspackages/server/src/gateway/__tests__/pending-interaction-cleanup.test.tspackages/server/src/gateway/__tests__/pending-interaction-store.test.tspackages/server/src/gateway/__tests__/reserve-cap.test.tspackages/server/src/gateway/__tests__/rest-api-hardening.test.tspackages/server/src/gateway/__tests__/revoked-token-store.test.tspackages/server/src/gateway/__tests__/runs-queue-integration.test.tspackages/server/src/gateway/__tests__/slack-routes.test.tspackages/server/src/gateway/__tests__/smoke-dispatch.test.tspackages/server/src/gateway/__tests__/user-auth-profile-store.test.tspackages/server/src/gateway/infrastructure/queue/runs-queue.tspackages/server/src/gateway/orchestration/impl/embedded-deployment.tspackages/server/src/lobu/__tests__/agent-routes-apply.test.tspackages/server/src/lobu/stores/__tests__/postgres-secret-store.test.tspackages/server/src/scheduled/__tests__/stale-run-reaper.test.tspackages/server/src/scheduled/jobs.tspackages/server/src/server-lifecycle.tspackages/server/src/start-local.tspackages/server/src/tools/admin/manage_watchers.tspackages/server/src/tools/get_watchers.tspackages/server/src/tools/resolve_path.tspackages/server/src/utils/assert-node-version.tspackages/server/src/utils/insert-event.tsrelease-please-config.jsonscripts/dev-native.shscripts/e2e-lobu-apply.shscripts/publish-packages.mjs
💤 Files with no reviewable changes (1)
- packages/server/src/tests/setup/pglite-backend.ts
| pull-requests: write | ||
|
|
||
| env: | ||
| PGVECTOR_VERSION: ${{ inputs.pgvector_version || 'v0.8.1' }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Actions workflows triggered by both workflow_dispatchandpush, is ${{ inputs.foo }}valid on push events, or should${{ github.event.inputs.foo }} be used for cross-event safety?
💡 Result:
The ${{ inputs.foo }} context is not valid for push events [1][2]. The inputs context is only available in workflows triggered by workflow_dispatch or workflow_call [1]. When a workflow is configured to be triggered by both workflow_dispatch and push, the inputs context will only be populated when the workflow is manually triggered via workflow_dispatch [1][2]. During a push event, any reference to ${{ inputs.foo }} will resolve to an empty string (or null/undefined, depending on usage), as inputs are not defined for the push event [3]. For cross-event safety, you should not rely on ${{ inputs.foo }} for push triggers. If your workflow logic depends on values that need to be provided differently depending on the trigger, you should use conditional logic (e.g., using the if keyword or shell scripts) to handle the absence of these inputs when the workflow is triggered by a push [1][4]. Accessing ${{ github.event.inputs.foo }} is also restricted to workflow_dispatch events and will similarly be empty during a push event [1][2]. In summary: 1. Inputs are not supported for push events [3][5]. 2. ${{ inputs.foo }} and ${{ github.event.inputs.foo }} will be empty/null when the workflow is triggered by push [1][2]. 3. You must design your workflow to account for the absence of these inputs during non-dispatch triggers [4].
Citations:
- 1: https://docs.github.com/en/actions/reference/contexts-reference
- 2: https://docs.github.com/enterprise-cloud@latest/actions/using-workflows/workflow-syntax-for-github-actions
- 3: https://stackoverflow.com/questions/76211346/how-to-define-inputs-in-a-push-trigger-in-github-actions
- 4: https://www.aikido.dev/blog/checklist-github-actions
- 5: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
Design the workflow to handle missing inputs on push events.
Line 30 references ${{ inputs.pgvector_version }}, but the inputs context is only available on workflow_dispatch and workflow_call events, not on push events. Since this workflow triggers on both workflow_dispatch and push, the input will be unavailable during push, causing the expression to resolve to an empty string and fall back to the default 'v0.8.1'. While the fallback prevents failure, this design pattern is fragile if the workflow logic later depends on inputs being defined.
The suggested fix (using github.event.inputs) does not solve this issue—it has the same unavailability on push events. To properly handle cross-event triggers, either:
- Limit the workflow to
workflow_dispatchif inputs are required, or - Use conditional jobs or steps based on the trigger event (
github.event_name) to handle different input availability.
🤖 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 @.github/workflows/build-pgvector-embedded.yml at line 30, The workflow reads
PGVECTOR_VERSION from the inputs context (PGVECTOR_VERSION: ${{
inputs.pgvector_version || 'v0.8.1' }}) which only exists for
workflow_dispatch/workflow_call and is unavailable on push; change the workflow
to handle cross-event triggers by either restricting the trigger to
workflow_dispatch if an input is required, or guard usage with conditionals that
check github.event_name (e.g., only read inputs when github.event_name ==
'workflow_dispatch') and provide a reliable default for push; update any
jobs/steps that reference inputs or github.event.inputs accordingly so they run
only when inputs are present or fall back to the intended default behavior.
| @@ -0,0 +1,41 @@ | |||
| { | |||
| "name": "@lobu/pgvector-embedded", | |||
| "version": "8.0.0", | |||
There was a problem hiding this comment.
Remove manual package versioning in this manifest.
This introduces a hand-edited package version, which conflicts with your release flow ownership and can desync release-please automation.
As per coding guidelines, "Never hand-edit package.json versions and don't push chore(release) commits directly; release-please owns versioning".
🤖 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 `@packages/pgvector-embedded/package.json` at line 3, Remove the hand-edited
"version" property from the package.json manifest so release-please can manage
versions automatically; locate the "version" key in
packages/pgvector-embedded/package.json (the "version": "8.0.0" entry) and
delete that property entirely (do not replace it with a manual placeholder), and
ensure you do not add chore(release) commits — let the release automation own
version bumps.
Comment on lines
+29
to
+31
| export async function startEmbeddedBackend(): Promise<EmbeddedBackend> { | ||
| if (active) return active; | ||
|
|
There was a problem hiding this comment.
Concurrent calls can start multiple embedded instances despite “idempotent” contract.
if (active) return active is not enough under concurrent calls before active is set; two callers can both spawn. Add an in-flight promise guard.
Suggested fix
let active: EmbeddedBackend | null = null;
+let activeStart: Promise<EmbeddedBackend> | null = null;
export async function startEmbeddedBackend(): Promise<EmbeddedBackend> {
if (active) return active;
+ if (activeStart) return activeStart;
-
- injectPgvector(resolveEmbeddedNativeDir());
+ activeStart = (async () => {
+ injectPgvector(resolveEmbeddedNativeDir());
+ // ... existing startup logic ...
+ return active as EmbeddedBackend;
+ })().finally(() => {
+ activeStart = null;
+ });
+ return activeStart;
- // existing body...
}🤖 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 `@packages/server/src/__tests__/setup/embedded-postgres-backend.ts` around
lines 29 - 31, startEmbeddedBackend currently checks "if (active) return active"
which is racy under concurrent callers; add an in-flight promise guard so only
one startup runs. Introduce a module-scoped "startingPromise" (or similar) that
is checked/returned before spawning, set it to the startup Promise when
beginning initialization inside startEmbeddedBackend, await it, set "active" on
success and clear "startingPromise" on resolve/reject; ensure subsequent callers
return startingPromise while startup is in-flight and that startingPromise is
nulled on failure so retries are possible.
Comment on lines
+34
to
+47
| const dataDir = mkdtempSync(join(tmpdir(), 'lobu-test-pg-')); | ||
| // 0 lets the OS assign; embedded-postgres needs a concrete port, so pick a | ||
| // high random one and let a collision fail loudly rather than silently share. | ||
| const port = 50000 + Math.floor(Math.random() * 10000); | ||
| const pg = new EmbeddedPostgres({ | ||
| databaseDir: dataDir, | ||
| user: 'postgres', | ||
| password: 'postgres', | ||
| port, | ||
| persistent: false, | ||
| }); | ||
|
|
||
| await pg.initialise(); | ||
| await pg.start(); |
There was a problem hiding this comment.
Startup failure path leaks temporary datadirs.
If pg.initialise() or pg.start() throws, dataDir is never removed. Wrap startup in try/catch and cleanup on failure.
Suggested fix
const dataDir = mkdtempSync(join(tmpdir(), 'lobu-test-pg-'));
const port = 50000 + Math.floor(Math.random() * 10000);
const pg = new EmbeddedPostgres({
@@
- await pg.initialise();
- await pg.start();
+ try {
+ await pg.initialise();
+ await pg.start();
+ } catch (error) {
+ rmSync(dataDir, { recursive: true, force: true });
+ throw error;
+ }🤖 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 `@packages/server/src/__tests__/setup/embedded-postgres-backend.ts` around
lines 34 - 47, The test creates a temporary dataDir with mkdtempSync and starts
EmbeddedPostgres via pg.initialise() and pg.start(), but if either call throws
the temp directory is leaked; wrap the startup sequence for the instance (the
calls to pg.initialise and pg.start) in a try/catch (or try/finally) that on any
error stops/cleans up the EmbeddedPostgres instance if created and removes the
dataDir (using fs.rm/fs.rmdir with recursive true or equivalent) before
rethrowing the error; reference dataDir, pg.initialise, pg.start, and the
EmbeddedPostgres instance in your change so the cleanup runs on all failure
paths.
Comment on lines
+28
to
35
| console.log(`\n🗄️ Using Postgres at ${databaseUrl}`); | ||
| } else { | ||
| console.log('\n🐘 No DATABASE_URL — spawning ephemeral embedded Postgres...'); | ||
| embedded = await startEmbeddedBackend(); | ||
| process.env.DATABASE_URL = embedded.url; | ||
| process.env.PGSSLMODE = 'disable'; | ||
| console.log(`✅ Embedded Postgres ready at ${embedded.url}`); | ||
| } |
There was a problem hiding this comment.
Avoid logging full DATABASE_URL values (credential leak).
Line 28 and Line 34 print raw connection URLs, including potential passwords. Redact credentials before logging.
Suggested fix
+function redactDatabaseUrl(raw: string): string {
+ try {
+ const u = new URL(raw);
+ if (u.password) u.password = '***';
+ if (u.username) u.username = '***';
+ return u.toString();
+ } catch {
+ return '<invalid-database-url>';
+ }
+}
@@
- console.log(`\n🗄️ Using Postgres at ${databaseUrl}`);
+ console.log(`\n🗄️ Using Postgres at ${redactDatabaseUrl(databaseUrl)}`);
@@
- console.log(`✅ Embedded Postgres ready at ${embedded.url}`);
+ console.log(`✅ Embedded Postgres ready at ${redactDatabaseUrl(embedded.url)}`);📝 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.
Suggested change
| console.log(`\n🗄️ Using Postgres at ${databaseUrl}`); | |
| } else { | |
| console.log('\n🐘 No DATABASE_URL — spawning ephemeral embedded Postgres...'); | |
| embedded = await startEmbeddedBackend(); | |
| process.env.DATABASE_URL = embedded.url; | |
| process.env.PGSSLMODE = 'disable'; | |
| console.log(`✅ Embedded Postgres ready at ${embedded.url}`); | |
| } | |
| function redactDatabaseUrl(raw: string): string { | |
| try { | |
| const u = new URL(raw); | |
| if (u.password) u.password = '***'; | |
| if (u.username) u.username = '***'; | |
| return u.toString(); | |
| } catch { | |
| return '<invalid-database-url>'; | |
| } | |
| } | |
| console.log(`\n🗄️ Using Postgres at ${redactDatabaseUrl(databaseUrl)}`); | |
| } else { | |
| console.log('\n🐘 No DATABASE_URL — spawning ephemeral embedded Postgres...'); | |
| embedded = await startEmbeddedBackend(); | |
| process.env.DATABASE_URL = embedded.url; | |
| process.env.PGSSLMODE = 'disable'; | |
| console.log(`✅ Embedded Postgres ready at ${redactDatabaseUrl(embedded.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 `@packages/server/src/__tests__/setup/global-setup.ts` around lines 28 - 35,
The logs in global-setup.ts currently print raw connection URLs (databaseUrl and
embedded.url) which may include credentials; update the logging to redact
credentials before printing by parsing the URL strings used in the existing
console.log calls (the variable names databaseUrl and embedded.url returned by
startEmbeddedBackend()) and replacing the userinfo portion (username:password@)
with a placeholder like <redacted_credentials> (or omit userinfo entirely) so
only host/port/db are logged; keep the same log messages but call a small helper
or inline logic to sanitize the URL before passing it to console.log.
Comment on lines
+341
to
348
| if (!embeddedBackend) return; | ||
| // Close the postgres.js singleton pool BEFORE stopping the embedded | ||
| // Postgres. Otherwise idle connections in the pool outlive the server and | ||
| // any follow-up query rejects with ECONNREFUSED as an unhandled rejection. | ||
| await closeDbSingleton(); | ||
| await pgliteBackend.stop(); | ||
| pgliteBackend = null; | ||
| await embeddedBackend.stop(); | ||
| embeddedBackend = null; | ||
| databaseReady = false; |
There was a problem hiding this comment.
dispose() skips pool cleanup for external benchmark DBs.
When LOBU_BENCHMARK_DATABASE_URL is used, embeddedBackend is null and dispose() returns before closeDbSingleton(), leaving open DB handles.
Suggested fix
async dispose(): Promise<void> {
- if (!embeddedBackend) return;
- // Close the postgres.js singleton pool BEFORE stopping the embedded
- // Postgres. Otherwise idle connections in the pool outlive the server and
- // any follow-up query rejects with ECONNREFUSED as an unhandled rejection.
await closeDbSingleton();
- await embeddedBackend.stop();
- embeddedBackend = null;
+ if (embeddedBackend) {
+ // Close pool before stopping embedded Postgres.
+ await embeddedBackend.stop();
+ embeddedBackend = null;
+ }
databaseReady = false;
}🤖 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 `@packages/server/src/benchmarks/memory/adapters/lobu-inprocess.ts` around
lines 341 - 348, The dispose() implementation currently returns early when
embeddedBackend is falsy, skipping closeDbSingleton() and leaving open DB
handles for external DBs (e.g. when LOBU_BENCHMARK_DATABASE_URL is used); update
dispose() so it always awaits closeDbSingleton() regardless of embeddedBackend
(call await closeDbSingleton() before returning or remove the early-return),
then if embeddedBackend exists stop it and set embeddedBackend = null and
databaseReady = false — reference the dispose() function, embeddedBackend
variable, and closeDbSingleton() call in your changes.
Comment on lines
+3
to
4
| * selected. With `pnpm test` the backend is ephemeral embedded Postgres; with | ||
| * `pnpm test:pg` (or when DATABASE_URL is set explicitly) it's real |
There was a problem hiding this comment.
Use bun commands in this backend-selection test comment.
The updated docstring still advertises pnpm commands, which conflicts with repo command conventions.
Suggested edit
- * selected. With `pnpm test` the backend is ephemeral embedded Postgres; with
- * `pnpm test:pg` (or when DATABASE_URL is set explicitly) it's real
+ * selected. With `bun test` the backend is ephemeral embedded Postgres; with
+ * `bun run test:pg` (or when DATABASE_URL is set explicitly) it's realAs per coding guidelines **/*.{ts,tsx,js}: Always prefer bun over npm for package management and script execution.
📝 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.
Suggested change
| * selected. With `pnpm test` the backend is ephemeral embedded Postgres; with | |
| * `pnpm test:pg` (or when DATABASE_URL is set explicitly) it's real | |
| * selected. With `bun test` the backend is ephemeral embedded Postgres; with | |
| * `bun run test:pg` (or when DATABASE_URL is set explicitly) it's real |
🤖 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 `@packages/server/src/lobu/stores/__tests__/postgres-secret-store.test.ts`
around lines 3 - 4, The comment string in the test docblock still uses "pnpm"
commands ("pnpm test" and "pnpm test:pg"); update that text to use bun instead
(e.g., "bun test" and "bun test:pg") while leaving the DATABASE_URL reference
unchanged so the sentence now advertises bun-based commands consistently.
…; earthdistance geo Local `lobu run` and the test backend now run a real embedded PostgreSQL 18 (embedded-postgres) instead of PGlite, with pgvector injected from the new @lobu/pgvector-embedded package. This removes the entire dual-backend: the LOBU_DISABLE_PREPARE single-connection pool pin, the conversation-lock no-op, the PGlite test harness, and the @electric-sql/pglite* deps are all gone — one engine everywhere. - DATABASE_URL is the sole backend selector: postgres:// = external; a path (or file:) = embedded PG with its cluster at <path>/.lobu/pgdata. - geo_lookup reimplemented on core-contrib cube+earthdistance (no PostGIS, works on every backend); restores the geo schema lost in the migration squash. - pgvector ships as prebuilt per-platform artifacts; a CI workflow rebuilds them, and only the control + pinned install SQL are vendored. - Test backend auto-spawns an ephemeral embedded PG when DATABASE_URL is unset, so `make test` needs no external database. - review.sh now unsets DATABASE_URL so the suite runs against an isolated embedded PG instead of whatever (often shared/prod) DB .env points at. BREAKING CHANGE: the local `lobu run` / test database engine is now a real embedded PostgreSQL instead of PGlite. Existing ~/.lobu PGlite data dirs are not migrated — a fresh embedded PG cluster is created. Production (external Postgres via DATABASE_URL) is unchanged.
buremba
force-pushed
the
feat/embedded-pg-spike
branch
from
ae80cbc to
23080a4
Compare
buremba
changed the title
Member
Author
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
scripts/review.sh (1)
143-143: 💤 Low valueRemove vestigial
DATABASE_URLpass-through topi.
DATABASE_URLwas unset at line 73, so this expands to an empty string. Ifpidoesn't need it, remove the line to avoid confusion; if it does need a connection, this is a bug.Suggested fix
INTEGRATION_LOG="$INTEGRATION_LOG" INTEGRATION_EXIT="$INTEGRATION_EXIT" \ GH_TOKEN="$GH_TOKEN_VAL" \ - DATABASE_URL="$DATABASE_URL" \ pi --mode json --no-session -p "@${PROMPT_FILE}" "Review the diff. Emit only the JSON verdict." < /dev/null🤖 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 `@scripts/review.sh` at line 143, The script is exporting an empty DATABASE_URL into the environment for the pi command because DATABASE_URL was unset earlier; either remove the vestigial DATABASE_URL="$DATABASE_URL" entry or, if pi actually requires a DB connection, ensure DATABASE_URL is set before invoking pi. Locate the pi invocation in the script (the line containing DATABASE_URL="$DATABASE_URL" and the earlier unset of DATABASE_URL) and either delete that environment assignment or restore a valid DATABASE_URL value prior to calling the pi command.
🤖 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 @.github/workflows/build-pgvector-embedded.yml:
- Line 39: Replace the invalid runner label in the workflow entry "- { platform:
darwin-x64, runner: macos-13 }" by changing "macos-13" to "macos-15-intel" so
the runner label is valid for the x64 macOS build; update that runner string in
the same workflow step where the platform is "darwin-x64".
- Around line 44-45: The workflow uses mutable action tags (uses:
actions/checkout@v4 and uses: actions/setup-node@v4) and omits checkout
credential hardening; replace each mutable tag with the corresponding pinned
commit SHA for actions/checkout and actions/setup-node (every occurrence of
uses: actions/checkout@v4 and uses: actions/setup-node@v4) and add
persist-credentials: false to each checkout step (the steps using
actions/checkout) to prevent GitHub token persistence; ensure you update all
occurrences mentioned in the diff so every checkout and setup-node reference is
pinned and every checkout step includes persist-credentials: false.
- Around line 25-27: The workflow currently grants workflow-wide write perms;
change the top-level permissions mapping (the 'permissions' key) to "contents:
read" and then scope write permissions only on the job that needs them by adding
a job-level permissions block to the "open-pr" job with "contents: write" and
"pull-requests: write"; leave the "build" job with default/read-only
permissions. Update the entries for the 'permissions' key and add a per-job
permissions map under the "open-pr" job in the same YAML.
In `@packages/server/src/__tests__/setup/global-setup.ts`:
- Around line 25-35: The test setup currently accepts any inherited
DATABASE_URL; add a safety gate to fail fast when DATABASE_URL exists but points
to a non-local host to avoid running destructive migrations against shared/prod
DBs: after reading process.env.DATABASE_URL (the databaseUrl variable) validate
its hostname (e.g. allow localhost, 127.0.0.1, ::1, unix socket patterns or a
special in-repo marker) and if it does not match a local pattern call
processLogger.error or console.error and process.exit(1) before running
migrations or proceeding to startEmbeddedBackend; keep the existing embedded
fallback using startEmbeddedBackend and PGSSLMODE when DATABASE_URL is absent or
explicitly allowed.
In `@packages/server/src/__tests__/setup/test-db.ts`:
- Around line 63-64: cleanupTestDatabase currently runs session-level SET
commands on pooled connections which can apply on one connection and be reset on
another; wrap the cleanup logic in db.begin(async (tx) => { ... }) so it runs on
a single pinned connection and replace any SET session_replication_role ... with
SET LOCAL session_replication_role ... (use the transaction object tx for
queries) to ensure the role change is scoped to the transaction lifetime and
avoids leaving pooled connections in replica mode.
In `@packages/server/src/gateway/__tests__/helpers/db-setup.ts`:
- Around line 36-40: The test setup currently trusts any pre-set DATABASE_URL;
update ensureDbForGatewayTests (the block that calls startEmbeddedBackend and
checks process.env.DATABASE_URL) to fail fast when DATABASE_URL is present and
points to a non-loopback host unless an explicit test-only override env var is
set. Implement: when process.env.DATABASE_URL exists, parse it with new
URL(process.env.DATABASE_URL), inspect hostname and reject hosts not in
["localhost","127.0.0.1","::1"] (and consider unix socket formats) by throwing
an error or exiting; allow the connection only if a dedicated override like
process.env.ALLOW_NON_LOOPBACK_DATABASE_URL_FOR_TESTS === "true" is set. Keep
startEmbeddedBackend usage unchanged when DATABASE_URL is absent. Ensure the
error message references DATABASE_URL so it's clear why the test aborted.
---
Nitpick comments:
In `@scripts/review.sh`:
- Line 143: The script is exporting an empty DATABASE_URL into the environment
for the pi command because DATABASE_URL was unset earlier; either remove the
vestigial DATABASE_URL="$DATABASE_URL" entry or, if pi actually requires a DB
connection, ensure DATABASE_URL is set before invoking pi. Locate the pi
invocation in the script (the line containing DATABASE_URL="$DATABASE_URL" and
the earlier unset of DATABASE_URL) and either delete that environment assignment
or restore a valid DATABASE_URL value prior to calling the pi command.
🪄 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: 2fc4f9f4-2c06-4b6b-908e-fc884c9b1782
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/pgvector-embedded/prebuilt/darwin-arm64/vector.dylibis excluded by!**/*.dylib
📒 Files selected for processing (75)
.github/workflows/build-pgvector-embedded.ymlMakefiledb/migrations/20260520120000_geo_earthdistance.sqlpackages/cli/package.jsonpackages/cli/src/__tests__/dev.test.tspackages/cli/src/commands/dev.tspackages/cli/src/commands/init.tspackages/cli/src/index.tspackages/cli/src/internal/credentials.tspackages/connectors/src/apple_photos.tspackages/pgvector-embedded/package.jsonpackages/pgvector-embedded/prebuilt/darwin-arm64/vector--0.8.1.sqlpackages/pgvector-embedded/prebuilt/darwin-arm64/vector.controlpackages/pgvector-embedded/scripts/build.shpackages/pgvector-embedded/src/index.tspackages/pgvector-embedded/tsconfig.jsonpackages/server/package.jsonpackages/server/scripts/build-server-bundle.mjspackages/server/src/__tests__/integration/auth/single-user-signup.test.tspackages/server/src/__tests__/integration/geo-enrichment.test.tspackages/server/src/__tests__/integration/identity/engine.test.tspackages/server/src/__tests__/integration/sandbox/namespace-dispatch.test.tspackages/server/src/__tests__/server-lifecycle.test.tspackages/server/src/__tests__/setup/embedded-postgres-backend.tspackages/server/src/__tests__/setup/global-setup.tspackages/server/src/__tests__/setup/pglite-backend.tspackages/server/src/__tests__/setup/test-db.tspackages/server/src/benchmarks/memory/adapters/lobu-inprocess.tspackages/server/src/db/client.tspackages/server/src/dev-vite.tspackages/server/src/gateway/__tests__/agent-history-routes.test.tspackages/server/src/gateway/__tests__/agent-routes.test.tspackages/server/src/gateway/__tests__/agent-settings-store.test.tspackages/server/src/gateway/__tests__/agent-transcript-snapshot.test.tspackages/server/src/gateway/__tests__/base-deployment-grants.test.tspackages/server/src/gateway/__tests__/chat-instance-manager-boot.test.tspackages/server/src/gateway/__tests__/chat-instance-manager-slack.test.tspackages/server/src/gateway/__tests__/connection-routes.test.tspackages/server/src/gateway/__tests__/core-services-store-selection.test.tspackages/server/src/gateway/__tests__/grant-store.test.tspackages/server/src/gateway/__tests__/helpers/db-setup.tspackages/server/src/gateway/__tests__/instruction-service.test.tspackages/server/src/gateway/__tests__/interaction-bridge-action-handlers.test.tspackages/server/src/gateway/__tests__/interaction-bridge-slack-webhook.test.tspackages/server/src/gateway/__tests__/mcp-proxy-edge-cases.test.tspackages/server/src/gateway/__tests__/mcp-proxy.test.tspackages/server/src/gateway/__tests__/multi-tenant-isolation-reproducers.test.tspackages/server/src/gateway/__tests__/oauth-state-store.test.tspackages/server/src/gateway/__tests__/pending-interaction-cleanup.test.tspackages/server/src/gateway/__tests__/pending-interaction-store.test.tspackages/server/src/gateway/__tests__/reserve-cap.test.tspackages/server/src/gateway/__tests__/rest-api-hardening.test.tspackages/server/src/gateway/__tests__/revoked-token-store.test.tspackages/server/src/gateway/__tests__/runs-queue-integration.test.tspackages/server/src/gateway/__tests__/slack-routes.test.tspackages/server/src/gateway/__tests__/smoke-dispatch.test.tspackages/server/src/gateway/__tests__/user-auth-profile-store.test.tspackages/server/src/gateway/infrastructure/queue/runs-queue.tspackages/server/src/gateway/orchestration/impl/embedded-deployment.tspackages/server/src/lobu/__tests__/agent-routes-apply.test.tspackages/server/src/lobu/stores/__tests__/postgres-secret-store.test.tspackages/server/src/scheduled/__tests__/stale-run-reaper.test.tspackages/server/src/scheduled/jobs.tspackages/server/src/server-lifecycle.tspackages/server/src/start-local.tspackages/server/src/tools/admin/manage_watchers.tspackages/server/src/tools/get_watchers.tspackages/server/src/tools/resolve_path.tspackages/server/src/utils/assert-node-version.tspackages/server/src/utils/insert-event.tsrelease-please-config.jsonscripts/dev-native.shscripts/e2e-lobu-apply.shscripts/publish-packages.mjsscripts/review.sh
💤 Files with no reviewable changes (1)
- packages/server/src/tests/setup/pglite-backend.ts
✅ Files skipped from review due to trivial changes (20)
- packages/server/src/dev-vite.ts
- packages/server/src/tests/server-lifecycle.test.ts
- packages/cli/src/commands/init.ts
- packages/pgvector-embedded/tsconfig.json
- packages/server/src/scheduled/jobs.ts
- packages/cli/src/internal/credentials.ts
- packages/cli/src/index.ts
- packages/server/src/lobu/stores/tests/postgres-secret-store.test.ts
- packages/server/src/gateway/infrastructure/queue/runs-queue.ts
- packages/server/src/tools/admin/manage_watchers.ts
- scripts/dev-native.sh
- packages/server/src/tools/resolve_path.ts
- packages/connectors/src/apple_photos.ts
- packages/server/src/gateway/tests/base-deployment-grants.test.ts
- packages/server/src/gateway/tests/connection-routes.test.ts
- packages/server/src/tests/integration/auth/single-user-signup.test.ts
- packages/pgvector-embedded/prebuilt/darwin-arm64/vector.control
- packages/server/src/tools/get_watchers.ts
- packages/server/src/tests/integration/identity/engine.test.ts
- packages/server/src/tests/integration/geo-enrichment.test.ts
Comment on lines
+25
to
+27
| permissions: | ||
| contents: write | ||
| pull-requests: write |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/build-pgvector-embedded.ymlRepository: lobu-ai/lobu
Length of output: 4464
Scope permissions per job instead of workflow-wide write access.
The build job only performs checkout and artifact upload, requiring read-only access. The open-pr job requires write permissions for git operations and PR creation. Restrict workflow-level permissions to contents: read and add job-level write permissions only to open-pr.
Suggested hardening diff
permissions:
- contents: write
- pull-requests: write
+ contents: read
jobs:
build:
+ permissions:
+ contents: read
@@
open-pr:
+ permissions:
+ contents: write
+ pull-requests: write🧰 Tools
🪛 zizmor (1.25.2)
[error] 26-26: overly broad permissions (excessive-permissions): contents: write is overly broad at the workflow level
(excessive-permissions)
[error] 27-27: overly broad permissions (excessive-permissions): pull-requests: write is overly broad at the workflow level
(excessive-permissions)
🤖 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 @.github/workflows/build-pgvector-embedded.yml around lines 25 - 27, The
workflow currently grants workflow-wide write perms; change the top-level
permissions mapping (the 'permissions' key) to "contents: read" and then scope
write permissions only on the job that needs them by adding a job-level
permissions block to the "open-pr" job with "contents: write" and
"pull-requests: write"; leave the "build" job with default/read-only
permissions. Update the entries for the 'permissions' key and add a per-job
permissions map under the "open-pr" job in the same YAML.
Comment on lines
+44
to
+45
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/build-pgvector-embedded.ymlRepository: lobu-ai/lobu
Length of output: 5276
🌐 Web query:
GitHub Actions security best practices pin commit SHA versions persist-credentials
💡 Result:
Securing GitHub Actions workflows involves two critical practices: pinning actions to immutable commit SHAs and managing credential persistence during checkout [1][2][3]. Pinning Actions to Commit SHAs Referencing actions by version tags (e.g., v4) is insecure because tags are mutable and can be repointed to malicious code by compromised maintainer accounts [4][5][6]. To ensure supply chain integrity, you should pin all third-party actions to their full-length commit SHA [1][7][3]. - Implementation: Replace tag references (e.g., uses: actions/checkout@v4) with the specific SHA (e.g., uses: actions/checkout@b4ffde6) [5][7]. - Documentation: Always include a comment indicating the version tag the SHA corresponds to for easier maintenance (e.g., # v4.1.1) [4][5]. - Automation: Use tools like Dependabot, Renovate, or specialized scanners (e.g., zizmor, pinact) to automate and manage these updates [4][3]. Managing persist-credentials By default, the actions/checkout action configures Git to persist the GitHub token in the local.git/config file [2][8]. This can expose credentials to subsequent steps in the workflow or leak through artifacts if they are improperly managed [2][8]. - Best Practice: Explicitly set persist-credentials to false in your checkout step unless you specifically require the token for Git operations later in the job [1][2]. - Implementation Example: - name: Checkout uses: actions/checkout@v4 with: persist-credentials: false General Security Recommendations - Least Privilege: Grant the GITHUB_TOKEN only the minimum required permissions (default to read-only at the top level and elevate only where necessary) [9][10][11]. - Avoid Secrets Exposure: Do not store sensitive values in plain text; use GitHub Secrets and leverage OpenID Connect (OIDC) for cloud authentication instead of long-lived static secrets [3][12][10]. - Monitor Risks: Periodically audit workflows for unpinned actions, broad permissions, and insecure trigger usage (e.g., pull_request_target) [1][2][13].
Citations:
- 1: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 2: https://www.wiz.io/blog/github-actions-security-guide
- 3: https://www.aikido.dev/blog/checklist-github-actions
- 4: Pin all GitHub Actions to immutable SHA commits for supply chain security github/gh-aw#5691
- 5: Pin GitHub Actions to commit SHAs in release workflow github/gh-aw#10021
- 6: https://gbhackers.com/compromised-github-action/
- 7: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 8: Fix credential persistence vulnerability in release.md github/gh-aw#5308
- 9: https://docs.github.com/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions
- 10: https://docs.github.com/en/enterprise-server@3.13/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions?learn=getting_started&learnProduct=actions
- 11: https://docs.github.com/en/actions/reference/security/secure-use
- 12: https://dev.to/nkuziev-sec/why-fixed-container-image-versions-matter-lessons-from-the-trivy-supply-chain-attack-29ke
- 13: https://dev.to/duriantaco/github-actions-security-and-gitlab-ci-security-static-analysis-for-cicd-g9h
Pin actions to commit SHAs and disable checkout credential persistence for supply chain security.
Lines 44, 45, 72, 82, and 83 use mutable tag refs (@v4) instead of commit SHAs, exposing the workflow to supply chain attacks if maintainer accounts are compromised. Additionally, checkout steps at lines 44 and 82 should explicitly set persist-credentials: false to prevent GitHub token exposure in the job context and artifacts.
Also applies to: 72-72, 83-83
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 44-44: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 44-44: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 45-45: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/build-pgvector-embedded.yml around lines 44 - 45, The
workflow uses mutable action tags (uses: actions/checkout@v4 and uses:
actions/setup-node@v4) and omits checkout credential hardening; replace each
mutable tag with the corresponding pinned commit SHA for actions/checkout and
actions/setup-node (every occurrence of uses: actions/checkout@v4 and uses:
actions/setup-node@v4) and add persist-credentials: false to each checkout step
(the steps using actions/checkout) to prevent GitHub token persistence; ensure
you update all occurrences mentioned in the diff so every checkout and
setup-node reference is pinned and every checkout step includes
persist-credentials: false.
Comment on lines
+25
to
35
| const databaseUrl = process.env.DATABASE_URL?.trim(); | ||
| if (databaseUrl) { | ||
| process.env.DATABASE_URL = databaseUrl; | ||
| console.log(`\n🗄️ Using external Postgres at ${databaseUrl}`); | ||
| console.log(`\n🗄️ Using Postgres at ${databaseUrl}`); | ||
| } else { | ||
| console.log('\n🐘 No DATABASE_URL — spawning ephemeral embedded Postgres...'); | ||
| embedded = await startEmbeddedBackend(); | ||
| process.env.DATABASE_URL = embedded.url; | ||
| process.env.PGSSLMODE = 'disable'; | ||
| console.log(`✅ Embedded Postgres ready at ${embedded.url}`); | ||
| } |
There was a problem hiding this comment.
Fail fast on non-local DATABASE_URL in test setup to prevent destructive runs against shared/prod DBs.
Given this setup runs migrations/resets, accepting any inherited DATABASE_URL without a safety gate is high-risk and can target the wrong database.
Suggested guard
+function isLocalDatabaseUrl(url: string): boolean {
+ try {
+ const parsed = new URL(url);
+ const host = parsed.hostname;
+ return host === "127.0.0.1" || host === "localhost" || host === "::1";
+ } catch {
+ return false;
+ }
+}
+
export async function setup(): Promise<void> {
@@
const databaseUrl = process.env.DATABASE_URL?.trim();
if (databaseUrl) {
+ if (
+ process.env.LOBU_ALLOW_NONLOCAL_TEST_DB !== "1" &&
+ !isLocalDatabaseUrl(databaseUrl)
+ ) {
+ throw new Error(
+ "Refusing to run tests against non-local DATABASE_URL. Set LOBU_ALLOW_NONLOCAL_TEST_DB=1 to override."
+ );
+ }
process.env.DATABASE_URL = databaseUrl;
console.log(`\n🗄️ Using Postgres at ${databaseUrl}`);
} else {🤖 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 `@packages/server/src/__tests__/setup/global-setup.ts` around lines 25 - 35,
The test setup currently accepts any inherited DATABASE_URL; add a safety gate
to fail fast when DATABASE_URL exists but points to a non-local host to avoid
running destructive migrations against shared/prod DBs: after reading
process.env.DATABASE_URL (the databaseUrl variable) validate its hostname (e.g.
allow localhost, 127.0.0.1, ::1, unix socket patterns or a special in-repo
marker) and if it does not match a local pattern call processLogger.error or
console.error and process.exit(1) before running migrations or proceeding to
startEmbeddedBackend; keep the existing embedded fallback using
startEmbeddedBackend and PGSSLMODE when DATABASE_URL is absent or explicitly
allowed.
Comment on lines
+63
to
+64
| max: 5, | ||
| idle_timeout: 20, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/server/src/__tests__/setup/test-db.ts"
echo "== Pool config =="
nl -ba "$FILE" | sed -n '56,72p'
echo
echo "== session_replication_role usage =="
rg -n "session_replication_role|db\\.begin\\(" "$FILE"
echo
echo "== Cleanup block context =="
nl -ba "$FILE" | sed -n '232,258p'Repository: lobu-ai/lobu
Length of output: 135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/server/src/__tests__/setup/test-db.ts"
echo "== Pool config (lines 56-72) =="
sed -n '56,72p' "$FILE" | cat -n
echo
echo "== session_replication_role usage =="
rg -n "session_replication_role|db\.begin\(" "$FILE"
echo
echo "== Cleanup block context (lines 232-258) =="
sed -n '232,258p' "$FILE" | cat -nRepository: lobu-ai/lobu
Length of output: 1841
🏁 Script executed:
#!/bin/bash
FILE="packages/server/src/__tests__/setup/test-db.ts"
echo "== Top of file (imports) =="
head -30 "$FILE"
echo
echo "== db.begin usage elsewhere in codebase =="
rg -n "db\.begin\|tx\.begin" packages/server/src --type ts -A 2 | head -50
echo
echo "== postgres version in package.json =="
rg -i '"postgres"' packages/server/package.jsonRepository: lobu-ai/lobu
Length of output: 1336
🏁 Script executed:
#!/bin/bash
echo "== Transaction patterns (begin/transaction) =="
rg -n "\.begin\(|transaction\(" packages/server/src --type ts -B 1 -A 3 | head -80
echo
echo "== Check if db is postgres instance =="
rg -n "const db = postgres|let db = postgres" packages/server/src --type ts -A 1Repository: lobu-ai/lobu
Length of output: 6153
Use db.begin() transaction to pin cleanup to one connection.
With max: 5, cleanupTestDatabase() can run SET session_replication_role on one pooled connection and reset it on another, leaving a connection stuck in replica mode and causing cross-test integrity drift/flakes.
Wrap the cleanup logic in db.begin(async (tx) => { ... }) and use SET LOCAL instead of SET to scope the setting to the transaction lifetime:
Proposed fix
export async function cleanupTestDatabase(): Promise<void> {
- // Disable triggers temporarily for faster truncation
- await db`SET session_replication_role = 'replica'`;
-
- if (tables.length > 0) {
- const quotedTables = tables.map(({ tablename }) => `"${tablename}"`).join(', ');
- try {
- await db.unsafe(`TRUNCATE ${quotedTables} CASCADE`);
- } catch {
- // Ignore errors for tables that may not exist.
- }
- }
-
- // Re-enable triggers
- await db`SET session_replication_role = 'origin'`;
+ await db.begin(async (tx) => {
+ await tx`SET LOCAL session_replication_role = 'replica'`;
+ if (tables.length > 0) {
+ const quotedTables = tables.map(({ tablename }) => `"${tablename}"`).join(", ");
+ try {
+ await tx.unsafe(`TRUNCATE ${quotedTables} CASCADE`);
+ } catch {
+ // Ignore errors for tables that may not exist.
+ }
+ }
+ });🤖 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 `@packages/server/src/__tests__/setup/test-db.ts` around lines 63 - 64,
cleanupTestDatabase currently runs session-level SET commands on pooled
connections which can apply on one connection and be reset on another; wrap the
cleanup logic in db.begin(async (tx) => { ... }) so it runs on a single pinned
connection and replace any SET session_replication_role ... with SET LOCAL
session_replication_role ... (use the transaction object tx for queries) to
ensure the role change is scoped to the transaction lifetime and avoids leaving
pooled connections in replica mode.
Comment on lines
36
to
40
| if (!process.env.DATABASE_URL) { | ||
| backend = await startPgliteBackend(); | ||
| backend = await startEmbeddedBackend(); | ||
| process.env.DATABASE_URL = backend.url; | ||
| process.env.PGSSLMODE = "disable"; | ||
| process.env.LOBU_DISABLE_PREPARE = "1"; | ||
| } |
There was a problem hiding this comment.
Add a safety gate for inherited shared DATABASE_URL in gateway tests.
ensureDbForGatewayTests() will currently trust any pre-set DATABASE_URL, which can point test schema-reset logic at a shared/prod database. Please fail fast for non-loopback hosts unless an explicit test-only override is set.
Suggested patch
export function ensureDbForGatewayTests(): Promise<void> {
if (initPromise) return initPromise;
initPromise = (async () => {
+ const inheritedUrl = process.env.DATABASE_URL?.trim();
+ if (inheritedUrl) {
+ let host: string | null = null;
+ try {
+ host = new URL(inheritedUrl).hostname.toLowerCase().replace(/^\[|\]$/g, "");
+ } catch {
+ host = null;
+ }
+ const isLoopback =
+ host === "localhost" || host === "127.0.0.1" || host === "::1";
+ if (!isLoopback && process.env.LOBU_TEST_ALLOW_SHARED_DB !== "1") {
+ throw new Error(
+ "Refusing to run gateway test DB setup against non-loopback DATABASE_URL. " +
+ "Unset DATABASE_URL or set LOBU_TEST_ALLOW_SHARED_DB=1 explicitly."
+ );
+ }
+ }
+
if (!process.env.DATABASE_URL) {
backend = await startEmbeddedBackend();
process.env.DATABASE_URL = backend.url;
process.env.PGSSLMODE = "disable";
}🤖 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 `@packages/server/src/gateway/__tests__/helpers/db-setup.ts` around lines 36 -
40, The test setup currently trusts any pre-set DATABASE_URL; update
ensureDbForGatewayTests (the block that calls startEmbeddedBackend and checks
process.env.DATABASE_URL) to fail fast when DATABASE_URL is present and points
to a non-loopback host unless an explicit test-only override env var is set.
Implement: when process.env.DATABASE_URL exists, parse it with new
URL(process.env.DATABASE_URL), inspect hostname and reject hosts not in
["localhost","127.0.0.1","::1"] (and consider unix socket formats) by throwing
an error or exiting; allow the connection only if a dedicated override like
process.env.ALLOW_NON_LOOPBACK_DATABASE_URL_FOR_TESTS === "true" is set. Keep
startEmbeddedBackend usage unchanged when DATABASE_URL is absent. Ensure the
error message references DATABASE_URL so it's clear why the test aborted.
…SE_URL `lobu init` now prompts for the database backend and writes an explicit DATABASE_URL into the scaffolded .env: - "Local embedded Postgres" (default) → DATABASE_URL=file://. — an isolated per-project cluster at ./.lobu/pgdata (gitignored). - "Connect to an existing Postgres" → prompts for the postgres:// URL. Both already work at runtime (the DATABASE_URL path/URL routing); init is just the front-door UX. Modernized .env.tmpl off the PGlite/LOBU_DATA_DIR model onto the file:// vs postgres:// scheme.
server.ts is now the single entry for both backends, branching on DATABASE_URL: postgres:// connects to an external Postgres; a path/file:// spawns a local embedded Postgres. The embedded boot lives in the new embedded-runtime.ts and is loaded ONLY via `await import(...)` in the embedded branch, so the external/prod path never resolves or loads the embedded-postgres binary even though it sits in node_modules. - Deleted start-local.ts; build one bundle (server.bundle.mjs) instead of two. - dev.ts drops bundle-switching (one bundle, self-selects on DATABASE_URL) and maps LOBU_DATA_DIR → an embedded DATABASE_URL, so the Mac app (which spawns `lobu run` with LOBU_DATA_DIR) keeps working with no Swift change. - dev:local + e2e-lobu-apply.sh point at server.ts. Validated both modes from the one bundle: embedded (file://) spawns PG, runs migrations, all 4 extensions live; external (postgres://) connects, passes the schema-version check, and never triggers the lazy embedded-postgres import (0 spawn lines). typecheck 0, gateway tests 24/24.
…elector LOBU_DATA_DIR was a second, redundant way to point at the embedded DB's data location. It's now purely the embedded-PG root (nothing else reads it — the old bootstrap-pat/ephemeral-key persistence claims were stale), so it folds entirely into DATABASE_URL=file://<dir>. - embedded-runtime.ts requires DATABASE_URL (no LOBU_DATA_DIR fallback). - user-config `dataDir` and the CLI `--data-dir` now map to DATABASE_URL=file://. - dev.ts drops the LOBU_DATA_DIR fallback; dev-native.sh + e2e-lobu-apply.sh set DATABASE_URL=file://<dir>. Validated: embedded boots from DATABASE_URL=file:// alone (cluster at <dir>/.lobu/pgdata), typecheck 0. (Mac app LocalLobuRunner switch to DATABASE_URL=file:// lands via the owletto submodule bump.)
…rops LOBU_DATA_DIR)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/server/src/embedded-runtime.ts`:
- Around line 258-262: The fallback path passed to resolveExistingPath
mistakenly uses join(APP_ROOT, "packages", "embeddings", "src", "server.ts")
which resolves to packages/server/packages/embeddings/...; update the fallback
used when computing serverPath so it points to the sibling embeddings package
(e.g., join(APP_ROOT, "..", "embeddings", "src", "server.ts") or otherwise
construct a sibling path) instead of nesting under APP_ROOT, leaving
resolveExistingPath, serverPath and publishedServerPath logic intact.
In `@packages/server/src/server.ts`:
- Around line 82-85: The migrationsDir fallback should include the
bundle-relative "dist/db/migrations" before falling back to PACKAGE_REPO_ROOT so
external bundle runs find the shipped migrations; update the migrationsDir
assignment (the const migrationsDir used by assertSchemaUpToDate(getDb(), {
migrationsDir })) to try process.env.LOBU_MIGRATIONS_DIR, then a bundle-relative
path to "dist/db/migrations" (computed relative to the running bundle file), and
only then path.join(PACKAGE_REPO_ROOT, "db", "migrations"); ensure the new
bundle-relative path is constructed in a way compatible with the module format
used by server.ts so the schema check can find the bundled migrations.
In `@scripts/e2e-lobu-apply.sh`:
- Around line 70-77: The harness must prevent user config from leaking into
server.ts by launching the server with an isolated config/home; create a
temporary directory (mktemp -d) and start the bun run invocation with HOME and
XDG_CONFIG_HOME pointed at that temp dir (in addition to unsetting DATABASE_URL
and keeping LOBU_DATA_DIR, PORT, HOST), then remove the temp dir after the run;
update the env invocation that launches server.ts to include HOME and
XDG_CONFIG_HOME so only ${DATA_DIR} controls embedded DB selection.
🪄 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: 5b1a28cf-b63d-4b6b-84e0-df699a825585
📒 Files selected for processing (11)
AGENTS.mdpackages/cli/scripts/build.cjspackages/cli/src/__tests__/dev.test.tspackages/cli/src/commands/dev.tspackages/server/package.jsonpackages/server/scripts/build-server-bundle.mjspackages/server/src/embedded-runtime.tspackages/server/src/server.tspackages/server/src/start-local.tsscripts/dev-native.shscripts/e2e-lobu-apply.sh
💤 Files with no reviewable changes (1)
- packages/server/src/start-local.ts
✅ Files skipped from review due to trivial changes (2)
- scripts/dev-native.sh
- AGENTS.md
Comment on lines
+258
to
+262
| const serverPath = resolveExistingPath( | ||
| join(APP_ROOT, "packages", "embeddings", "src", "server.ts"), | ||
| join(process.cwd(), "packages", "embeddings", "src", "server.ts"), | ||
| ...(publishedServerPath ? [publishedServerPath] : []), | ||
| ); |
There was a problem hiding this comment.
Fix the monorepo embeddings source path fallback.
APP_ROOT is already packages/server, so join(APP_ROOT, "packages", "embeddings", ...) resolves to packages/server/packages/embeddings/.... If import.meta.resolve("@lobu/embeddings/server") misses, the embeddings child never starts and local embedding generation is silently disabled. Point this fallback at the sibling package path instead.
Suggested fix
const serverPath = resolveExistingPath(
- join(APP_ROOT, "packages", "embeddings", "src", "server.ts"),
+ join(APP_ROOT, "..", "embeddings", "src", "server.ts"),
join(process.cwd(), "packages", "embeddings", "src", "server.ts"),
...(publishedServerPath ? [publishedServerPath] : []),
);🤖 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 `@packages/server/src/embedded-runtime.ts` around lines 258 - 262, The fallback
path passed to resolveExistingPath mistakenly uses join(APP_ROOT, "packages",
"embeddings", "src", "server.ts") which resolves to
packages/server/packages/embeddings/...; update the fallback used when computing
serverPath so it points to the sibling embeddings package (e.g., join(APP_ROOT,
"..", "embeddings", "src", "server.ts") or otherwise construct a sibling path)
instead of nesting under APP_ROOT, leaving resolveExistingPath, serverPath and
publishedServerPath logic intact.
Comment on lines
+82
to
+85
| const migrationsDir = | ||
| process.env.LOBU_MIGRATIONS_DIR?.trim() || | ||
| path.join(PACKAGE_REPO_ROOT, "db", "migrations"); | ||
| await assertSchemaUpToDate(getDb(), { migrationsDir }); |
There was a problem hiding this comment.
Use the bundled migrations path in published external mode.
The CLI build already ships db/migrations next to server.bundle.mjs, but this branch only falls back to ${PACKAGE_REPO_ROOT}/db/migrations. Once the bundle is running from @lobu/cli/dist/server.bundle.mjs, PACKAGE_REPO_ROOT resolves to node_modules, so external lobu run misses the copied migrations and fails the boot-time schema check unless LOBU_MIGRATIONS_DIR or SKIP_SCHEMA_VERSION_CHECK is set. Add the bundle-relative dist/db/migrations fallback before the repo-root path.
🤖 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 `@packages/server/src/server.ts` around lines 82 - 85, The migrationsDir
fallback should include the bundle-relative "dist/db/migrations" before falling
back to PACKAGE_REPO_ROOT so external bundle runs find the shipped migrations;
update the migrationsDir assignment (the const migrationsDir used by
assertSchemaUpToDate(getDb(), { migrationsDir })) to try
process.env.LOBU_MIGRATIONS_DIR, then a bundle-relative path to
"dist/db/migrations" (computed relative to the running bundle file), and only
then path.join(PACKAGE_REPO_ROOT, "db", "migrations"); ensure the new
bundle-relative path is constructed in a way compatible with the module format
used by server.ts so the schema check can find the bundled migrations.
Comment on lines
+70
to
+77
| # Unset DATABASE_URL — with only LOBU_DATA_DIR set, server.ts boots an embedded | ||
| # Postgres rooted there and writes its own DATABASE_URL into process.env. | ||
| env \ | ||
| -u DATABASE_URL \ | ||
| LOBU_DATA_DIR="${DATA_DIR}" \ | ||
| PORT="${PORT}" \ | ||
| HOST=127.0.0.1 \ | ||
| PG_SOCKET_PORT=0 \ | ||
| bun run "${REPO_ROOT}/packages/server/src/start-local.ts" \ | ||
| bun run "${REPO_ROOT}/packages/server/src/server.ts" \ |
There was a problem hiding this comment.
Isolate the server from user-level config in this harness.
server.ts now loads ~/.config/lobu/config.json before it decides which database mode to boot. Unsetting only DATABASE_URL here still lets a saved databaseUrl / port leak into the process, so this e2e run can stop being hermetic and point back at a shared DB. Start the server with a temp HOME/XDG_CONFIG_HOME (and clean it up) so only ${DATA_DIR} drives the embedded path.
🤖 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 `@scripts/e2e-lobu-apply.sh` around lines 70 - 77, The harness must prevent
user config from leaking into server.ts by launching the server with an isolated
config/home; create a temporary directory (mktemp -d) and start the bun run
invocation with HOME and XDG_CONFIG_HOME pointed at that temp dir (in addition
to unsetting DATABASE_URL and keeping LOBU_DATA_DIR, PORT, HOST), then remove
the temp dir after the run; update the env invocation that launches server.ts to
include HOME and XDG_CONFIG_HOME so only ${DATA_DIR} controls embedded DB
selection.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/server/src/embedded-runtime.ts (1)
257-261:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmbeddings fallback path is incorrect.
APP_ROOTresolves topackages/server, so the first candidatejoin(APP_ROOT, "packages", "embeddings", ...)expands topackages/server/packages/embeddings/...which doesn't exist. This causes the monorepo fallback to fail silently, disabling local embedding generation. Point the fallback at the sibling package.🐛 Suggested fix
const serverPath = resolveExistingPath( - join(APP_ROOT, "packages", "embeddings", "src", "server.ts"), + join(APP_ROOT, "..", "embeddings", "src", "server.ts"), join(process.cwd(), "packages", "embeddings", "src", "server.ts"), ...(publishedServerPath ? [publishedServerPath] : []), );🤖 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 `@packages/server/src/embedded-runtime.ts` around lines 257 - 261, The fallback path for embeddings is wrong because APP_ROOT points to the server package, so the current resolveExistingPath call builds "packages/server/packages/embeddings/..."; update the candidates passed to resolveExistingPath in the serverPath assignment to reference the sibling embeddings package (e.g., join(APP_ROOT, "..", "embeddings", "src", "server.ts") or use path.resolve(APP_ROOT, "..", "embeddings", "src", "server.ts")) instead of join(APP_ROOT, "packages", "embeddings", ...); keep the existing process.cwd() candidate and publishedServerPath handling unchanged and ensure you update the serverPath assignment that calls resolveExistingPath.
🤖 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.
Duplicate comments:
In `@packages/server/src/embedded-runtime.ts`:
- Around line 257-261: The fallback path for embeddings is wrong because
APP_ROOT points to the server package, so the current resolveExistingPath call
builds "packages/server/packages/embeddings/..."; update the candidates passed
to resolveExistingPath in the serverPath assignment to reference the sibling
embeddings package (e.g., join(APP_ROOT, "..", "embeddings", "src", "server.ts")
or use path.resolve(APP_ROOT, "..", "embeddings", "src", "server.ts")) instead
of join(APP_ROOT, "packages", "embeddings", ...); keep the existing
process.cwd() candidate and publishedServerPath handling unchanged and ensure
you update the serverPath assignment that calls resolveExistingPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 960a1cf7-baa4-4908-ab53-550557794b6f
📒 Files selected for processing (7)
packages/cli/src/commands/dev.tspackages/cli/src/index.tspackages/owlettopackages/server/src/embedded-runtime.tspackages/server/src/utils/user-config.tsscripts/dev-native.shscripts/e2e-lobu-apply.sh
✅ Files skipped from review due to trivial changes (2)
- packages/owletto
- packages/cli/src/index.ts
…istence (0c54927)
…le binaries The build matrix never produced non-darwin artifacts: - build.sh copied vector.control / vector--<v>.sql from the OS Postgres SHAREDIR, but the workflow only runs `make` (not `make install`), so those files never land there — every fresh-PG cell died on `cp: cannot stat '.../extension/vector.control'`. Stage straight from the built pgvector tree instead (vector.control is in-repo, the SQL is make-generated). Validated locally: reproduces the committed darwin-arm64 artifact byte-for-bit. - Pass OPTFLAGS="" so the redistributed binary drops pgvector's default `-march=native`; otherwise it bakes in the CI runner's microarch and SIGILLs on older user CPUs. - Linux install used a hand-rolled curl|gpg --dearmor pipe that flaked with "gpg: cannot open '/dev/tty'"; switch to the official pgdg apt.postgresql.org.sh setup script.
simpleQuery(query) just returned query — a no-op left over from the removed PGlite `.simple()` prepared-statement path. Unwrapped all 30 call sites (resolve_path.ts, multi-tenant.ts) and removed the export.
Built by build-pgvector-embedded.yml (pgvector v0.8.1, PG18, OPTFLAGS="" for portability). darwin-x64 follows once the macos-13 cell clears the runner queue; darwin-arm64 already committed.
…ded PG from runtime server now depends on @lobu/pgvector-embedded (workspace:*), but the image never copied or stubbed it — bun install + the builder tsc would have failed the first time this hit main (build-images runs only on main). COPY its manifest + source and build its dist before the server type-check. Then prune embedded-postgres + @embedded-postgres/* (~145MB platform binary) from node_modules after the bundle: prod always uses an external DATABASE_URL, so the embedded path (the only loader, via await import) never runs. Validated with a full --target builder build: install + dist + tsc + bundle succeed and the runtime image no longer carries the 145MB binary.
…al.ts comment drift applyDevProjectPathDefault had zero callers (the LOBU_DEV_PROJECT_PATH fallback is inlined in server.ts main()). Removed it, and corrected the server-lifecycle.ts / dev-vite.ts headers that still described a two-entry-point design — start-local.ts was deleted in this PR.
Resolves conflicts from main's context-config flatten (#955) + shared-DB guard refactor against this branch's embedded-PG work: - cli/index.ts: context add adopts main's flat --url/--cwd/--lifecycle (drops the nested --port/--host/--database-url/--data-dir options). - cli/commands/dev.ts: keep embedded helpers (isExternalDatabaseUrl, resolveEmbeddedDataRoot, mode selection) + adopt main's shouldRefuseSharedDatabaseUrl guard; drop dead BackendBundleKind (pglite removed) and the userServerConfig.databaseUrl/dataDir block (flattened away). - server/utils/user-config.ts: drop the dataDir→DATABASE_URL mapping (dataDir no longer in the flattened config schema). - server/start-local.ts: stays deleted (consolidated into server.ts). - owletto: bumped to merged 872a6ea (Keychain key + main's flat-context). Verified: cli + server tsc clean, dev.test.ts 12/12, init still writes DATABASE_URL to .env (embedded-vs-external feature intact).
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The integration job (vitest under Node) failed with ERR_RESOLVE_PACKAGE_ENTRY_FAIL: the test backend imports @lobu/pgvector-embedded, so vite resolves its `import` condition (./dist/index.js) at transform time — but CI never built that package's dist (it's a new workspace dep in this PR). Build it alongside the other workspace packages, same as the Docker builder fix. Validated: full integration suite vs pg16 with dist freshly built → 0 resolve errors, 287/294 pass (the 5 fails are isolated-vm prebuild gaps local to the dev Mac; they load under CI's pinned Node 22). Also fix a stale comment: the test backend mirrors src/embedded-runtime.ts, not the deleted src/start-local.ts.
…os-13 GitHub retired the macos-13 (Intel) hosted runner in Dec 2025, so the darwin-x64 matrix cell queued indefinitely (no runner ever assigned). macos-15-intel is GitHub's current x86_64 macOS runner (Intel on macOS 15), the supported replacement until x86_64 macOS is dropped in 2027.
Built on macos-15-intel (x86_64 Mach-O bundle), completing all four platforms: darwin-arm64, darwin-x64, linux-x64, linux-arm64.
Member
Author
|
bug_free 55, simplicity 76, slop 8, bugs 1, 1 blockers Typecheck and integration passed. Unit suite failed as above. Explored by booting packages/server/dist/server.bundle.mjs with path DATABASE_URL on :8799; embedded Postgres migrated and /health returned 200. /api/docs returned 404. Blockers
Suggested fixes
Full verdict JSON{
"bug_free_confidence": 55,
"bugs": 1,
"slop": 8,
"simplicity": 76,
"blockers": [
"unit tests failed in touched CLI apply test path: packages/cli/src/commands/_lib/apply/__tests__/apply-cmd-dryrun.test.ts reported SyntaxError: Export named 'AutoCreateWhenRule' not found from packages/connector-sdk/dist/index.js"
],
"change_type": "feat",
"behavior_change_risk": "high",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/server/src/server.ts",
"line": 8,
"change": "Update this header comment: embedded-runtime is statically imported; only embedded-postgres/@lobu/pgvector-embedded and server-lifecycle are lazy-loaded."
},
{
"file": "packages/server/src/__tests__/setup/embedded-postgres-backend.ts",
"line": 35,
"change": "Replace random high-port selection with a bind-to-0/free-port helper to avoid rare embedded Postgres test port collisions."
}
],
"notes": "Typecheck and integration passed. Unit suite failed as above. Explored by booting packages/server/dist/server.bundle.mjs with path DATABASE_URL on :8799; embedded Postgres migrated and /health returned 200. /api/docs returned 404.",
"categories": {
"src": 2923,
"tests": 1516,
"docs": 90,
"config": 300,
"deps": 113,
"migrations": 100,
"ci": 128,
"generated": 3688
}
}Local review gate — branch protection can require the |
…ABASE_URL guard; trim apple_photos comment - turn-liveness.test.ts (added by #971) imported ensurePgliteForGatewayTests, which this PR's PGlite removal renamed to ensureDbForGatewayTests. The merge produced no textual conflict but a broken import that failed the gateway suite. Point it at the renamed helper. Validated: 9/9 pass vs Postgres. - review.sh: `unset DATABASE_URL` (run tests on isolated embedded PG) left a later `DATABASE_URL="$DATABASE_URL"` reference that tripped `set -u` (unbound variable), so pi never produced a verdict. Default to empty. - apple_photos.ts: trim the 13-line geo-tier TODO to a 4-line note (pi slop).
…cally imported The header said embedded-runtime loads via await import; it's actually a static import (the heavy embedded-postgres + pgvector injector are what load lazily, inside that module). Matches the dynamic-import allow-list in AGENTS.md.
buremba
added a commit
that referenced
this pull request
May 20, 2026
The `Security tests (vitest)` job has been red on main since the embedded- Postgres migration (#965): its build step only built core + connector-sdk, not @lobu/pgvector-embedded. vitest/vite resolve that package via its "import" export condition (dist/index.js), unlike bun:test which uses the "bun" condition (src/index.ts) — so the bun:test job stayed green while vitest failed collection with ERR_RESOLVE_PACKAGE_ENTRY_FAIL on the embedded- Postgres test backend's `import … from "@lobu/pgvector-embedded"`. Add the pgvector-embedded build to the vitest job's build step. Reproducer (exact CI command, packages/server): node ../../node_modules/.bin/vitest run src/__tests__/webhook-signatures.test.ts - without pgvector-embedded dist → ERR_RESOLVE_PACKAGE_ENTRY_FAIL, "no tests" - with core+connector-sdk+pgvector-embedded built → 7 passed
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
2 participants
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.
What
Replaces PGlite with a real embedded PostgreSQL 18 (
embedded-postgres) for the locallobu runruntime and the test backend, with pgvector injected from a new@lobu/pgvector-embeddedpackage. One DB engine everywhere — the entire PGlite/Postgres dual-backend is gone.Why
PGlite's WASM Postgres forced a parade of parity workarounds (the
LOBU_DISABLE_PREPAREsingle-connection pin, a conversation-lock no-op, simple-query-mode SQL quirks, a second test harness). A real embedded PG removes all of it: prepared statements, a multi-connection pool, andLISTEN/NOTIFYwork natively.Changes
DATABASE_URL:postgres://= external; a filesystem path (orfile:) = embedded PG with its cluster at<path>/.lobu/pgdata.lobu rundefaults to~/.lobu/pgdata.@lobu/pgvector-embedded); a CI workflow (build-pgvector-embedded.yml) rebuilds them. Onlyvector.control+ the pinned install SQL are vendored (not the upgrade chain).geo_lookupreverse-geocoding reimplemented on core-contribcube+earthdistance(works on every backend, zero extra deps); restores the geo schema lost in the migration squash. Accurate address/venue tiers are deferred to theapple.photosconnector (Apple frameworks) — noted in code.@electric-sql/pglite*), theLOBU_DISABLE_PREPAREmachinery (pool pin + lock no-op), and the dead embedded-mode tests. Test backend now auto-spawns an ephemeral embedded PG whenDATABASE_URLis unset, somake testneeds no external DB.Validation
lobu runboots end-to-end on embedded PG (migrations, vector + pg_trgm, multi-conn pool, clean teardown);CREATE EXTENSION vectorworks from the trimmed artifact set.Notes for review
~/.lobuPGlite data dirs are not migrated — a fresh embedded PG cluster is created. Prod (external Postgres) is unchanged.darwin-arm64,darwin-x64,linux-x64,linux-arm64) are now built bybuild-pgvector-embedded.ymland committed. Built withOPTFLAGS=""so the redistributed binary drops pgvector's default-march=native(which would SIGILL on older user CPUs); the build also stagesvector.control/SQL from the build tree rather than a non-existentmake installsharedir.simpleQueryno-op wrapper removed (30 call sites); the two server entry points are already collapsed into oneserver.tswith a lazy embedded-PG import.Follow-on changes in this PR (post-handover)
ENCRYPTION_KEY.LocalLobuRunnersetLOBU_ALLOW_EPHEMERAL_ENCRYPTION_KEY=1, so the embedded server minted a fresh key every launch — orphaning at-rest connection secrets and freezing the install-operator password on the first boot's now-lost key. Now reads-or-creates a per-context 32-byte key in the login Keychain and passes it asENCRYPTION_KEY(owletto submodule bumped). No server change needed.packages/servernow depends on@lobu/pgvector-embedded(workspace:*), but the Dockerfile never copied or stubbed it —bun install+ the buildertscwould have broken the first time this hitmain(build-imagesonly runs on main). Added the manifest/source COPY + dist build, and pruneembedded-postgres+@embedded-postgres/*(~145MB platform binary) from the runtime image, since prod always uses an externalDATABASE_URLand never loads the embedded path.Release coordination
feat(server)!→ major version bump.LOBU_DATA_DIR(now ignored); until users update it would boot to~/.lobu/pgdata. Land the Mac-app update with or before the CLI release and note it in the release notes.Summary by CodeRabbit
New Features
Chores