feat(connectors): user-declared connector dependencies (npm bundled + nix native)#973
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds end-to-end native dependency support for connectors: CLI pre-compiles local connector source (ensuring project deps), the compile pipeline externalizes the SDK, connectors may declare ChangesConnector Native Dependencies: End-to-End Integration
Sequence Diagram(s)sequenceDiagram
Client->>Server: installConnector({ sourceCode|sourceUrl|compiled })
Server->>WorkerDaemon: job available via poll (includes nix_packages)
WorkerDaemon->>Executor: executeCompiledConnector(compiledCode, job, hooks, nixPackages)
Executor->>SubprocessExecutor: execute(..., options:{nixPackages})
alt nixPackages present
SubprocessExecutor->>NixShell: spawn nix-shell -p <packages> --run "exec node <child-runner>"
NixShell->>ChildProcess: runs connector in environment with native packages
else no nixPackages
SubprocessExecutor->>ChildProcess: fork child-runner directly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 62: Update the sentence describing what `lobu apply` runs so it
accurately states the exact install command: replace the mention of `bun
install` with `bun install --ignore-scripts` in the paragraph that explains
"Compile happens on the CLI" and the `lobu apply` behavior, ensuring the doc
clearly communicates the safety/behavior contract regarding script execution;
keep the rest of the explanation about bundling and externalizing
`@lobu/connector-sdk` unchanged.
In `@packages/cli/src/commands/_lib/apply/apply-cmd.ts`:
- Around line 380-397: The compile step currently always uses def.sourceFile
which ignores def.sourceCode (set when using source_url) and can compile the
wrong file; update the branch that checks def.sourceCode to prefer compiling
from the in-memory source (def.sourceCode) rather than def.sourceFile — either
by passing def.sourceCode into compileConnectorFromFile (or adding a new
compileConnectorFromSource API) or by writing def.sourceCode to a temporary file
and using that path; also ensure ensureProjectDepsInstalled is invoked with the
correct project entry (use the temp file path or adjust it to accept source
content) before calling client.installConnector so the uploaded bundle matches
the provided source.
In `@packages/cli/src/commands/init.ts`:
- Around line 666-699: The current init logic unconditionally overwrites
package.json and tsconfig.json via writeFile; change it to first check for
existing files (e.g., using fs.existsSync or fs.promises.access) and only
create/overwrite when absent or explicitly forced, and when a file exists read
and merge sensible defaults (for package.json merge devDependencies and preserve
existing fields; for tsconfig.json merge/extend existing compilerOptions and
include) before writing; update the writeFile calls that target join(projectDir,
"package.json") and join(projectDir, "tsconfig.json") to implement this
existence check + merge behavior and keep the mkdir(join(projectDir,
"connectors"), { recursive: true }) and .gitkeep creation unchanged.
In `@packages/server/src/worker-api.ts`:
- Line 492: The join on connector_definitions that supplies connector_runtime is
currently broad and can return nondeterministic rows; update the query that
selects cd.runtime AS connector_runtime (and the related joins at the same area)
to only pick the latest active connector_definition per (key, organization_id) —
e.g., replace the direct join with a subquery or LATERAL join that filters WHERE
status = 'active' and orders by a monotonic column (created_at or version) DESC
LIMIT 1, and use that subquery's runtime as connector_runtime so rows[0] is
deterministic and always returns the latest active runtime.nix.packages.
🪄 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: 1cf9c280-0bdd-46de-8727-9563121fb51c
📒 Files selected for processing (13)
AGENTS.mdpackages/cli/src/commands/_lib/apply/apply-cmd.tspackages/cli/src/commands/_lib/apply/client.tspackages/cli/src/commands/_lib/ensure-deps-installed.tspackages/cli/src/commands/init.tspackages/connector-sdk/src/connector-types.tspackages/connector-worker/src/compile/index.tspackages/connector-worker/src/daemon/client.tspackages/connector-worker/src/daemon/executor.tspackages/connector-worker/src/executor/interface.tspackages/connector-worker/src/executor/runtime.tspackages/connector-worker/src/executor/subprocess.tspackages/server/src/worker-api.ts
| conn.config AS connection_config, | ||
| conn.device_worker_id AS connection_device_worker_id, | ||
| cv.compiled_code, | ||
| cd.runtime AS connector_runtime, |
There was a problem hiding this comment.
Use latest active connector_definition when reading runtime metadata.
Line 504 introduces a broad join on connector_definitions without status/order constraints. If multiple rows exist for the same (key, organization_id), rows[0] becomes nondeterministic and can return stale/wrong runtime.nix.packages.
Suggested fix
- LEFT JOIN connector_definitions cd ON cd.key = r.connector_key
- AND cd.organization_id = r.organization_id
+ LEFT JOIN LATERAL (
+ SELECT runtime
+ FROM connector_definitions cd
+ WHERE cd.key = r.connector_key
+ AND cd.organization_id = r.organization_id
+ AND cd.status = 'active'
+ ORDER BY cd.updated_at DESC, cd.id DESC
+ LIMIT 1
+ ) cd ON trueAlso applies to: 504-506
🤖 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/worker-api.ts` at line 492, The join on
connector_definitions that supplies connector_runtime is currently broad and can
return nondeterministic rows; update the query that selects cd.runtime AS
connector_runtime (and the related joins at the same area) to only pick the
latest active connector_definition per (key, organization_id) — e.g., replace
the direct join with a subquery or LATERAL join that filters WHERE status =
'active' and orders by a monotonic column (created_at or version) DESC LIMIT 1,
and use that subquery's runtime as connector_runtime so rows[0] is deterministic
and always returns the latest active runtime.nix.packages.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/cli/src/commands/init.ts (1)
673-691:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t silently clobber an existing
package.jsonon read/parse failures.The current broad fallback can replace an existing file when parsing fails, which is destructive for
--hereinit. Treat only missing-file as “create new”, and surface parse/read errors for existing files.Suggested fix
- let pkgJson: Record<string, unknown>; - try { - pkgJson = JSON.parse(await readFile(pkgJsonPath, "utf-8")) as Record< - string, - unknown - >; - } catch { - pkgJson = { - name: projectName, - version: "0.0.0", - private: true, - type: "module", - }; - } + let pkgJson: Record<string, unknown>; + try { + const raw = await readFile(pkgJsonPath, "utf-8"); + pkgJson = JSON.parse(raw) as Record<string, unknown>; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + throw new Error( + `Found existing package.json but could not parse/read it. Please fix it and re-run init.` + ); + } + pkgJson = { + name: projectName, + version: "0.0.0", + private: true, + type: "module", + }; + }🤖 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/cli/src/commands/init.ts` around lines 673 - 691, The current try/catch around reading/parsing pkgJsonPath can silently overwrite an existing package.json on parse/read failure; change the logic in the init flow that sets pkgJson (variables: pkgJsonPath, pkgJson, projectName, cliVersion) so that you only build the default object when the file truly does not exist (e.g., detect ENOENT via fs.stat/fs.access or check the thrown error.code), but for any other read or JSON.parse error rethrow or surface the error to the user instead of falling back to the default; then continue to merge devDependencies and write the file as before.packages/server/src/worker-api.ts (1)
504-506:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake connector runtime join deterministic (latest active row).
status = 'active'helps, but this join can still return multiple rows per key/org and make runtime selection nondeterministic.Suggested fix
- LEFT JOIN connector_definitions cd ON cd.key = r.connector_key - AND cd.organization_id = r.organization_id - AND cd.status = 'active' + LEFT JOIN LATERAL ( + SELECT runtime + FROM connector_definitions cd + WHERE cd.key = r.connector_key + AND cd.organization_id = r.organization_id + AND cd.status = 'active' + ORDER BY cd.updated_at DESC, cd.id DESC + LIMIT 1 + ) cd ON TRUE🤖 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/worker-api.ts` around lines 504 - 506, The current LEFT JOIN to connector_definitions (alias cd) using cd.key = r.connector_key, cd.organization_id = r.organization_id and cd.status = 'active' can return multiple active rows and make runtime selection nondeterministic; change the join to target a single deterministic "latest" active row by joining cd to a subquery that selects the single id (or other unique PK) of the latest active definition per key/org (e.g., ORDER BY created_at or version DESC LIMIT 1) and join on cd.id = that id so functions referencing connector_definitions always get the latest active row for r.connector_key and r.organization_id.
🤖 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/cli/src/commands/init.ts`:
- Around line 673-691: The current try/catch around reading/parsing pkgJsonPath
can silently overwrite an existing package.json on parse/read failure; change
the logic in the init flow that sets pkgJson (variables: pkgJsonPath, pkgJson,
projectName, cliVersion) so that you only build the default object when the file
truly does not exist (e.g., detect ENOENT via fs.stat/fs.access or check the
thrown error.code), but for any other read or JSON.parse error rethrow or
surface the error to the user instead of falling back to the default; then
continue to merge devDependencies and write the file as before.
In `@packages/server/src/worker-api.ts`:
- Around line 504-506: The current LEFT JOIN to connector_definitions (alias cd)
using cd.key = r.connector_key, cd.organization_id = r.organization_id and
cd.status = 'active' can return multiple active rows and make runtime selection
nondeterministic; change the join to target a single deterministic "latest"
active row by joining cd to a subquery that selects the single id (or other
unique PK) of the latest active definition per key/org (e.g., ORDER BY
created_at or version DESC LIMIT 1) and join on cd.id = that id so functions
referencing connector_definitions always get the latest active row for
r.connector_key and r.organization_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa1fc511-b56f-4ebe-8ef2-8aa4a9905cc5
📒 Files selected for processing (4)
AGENTS.mdpackages/cli/src/commands/_lib/apply/apply-cmd.tspackages/cli/src/commands/init.tspackages/server/src/worker-api.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
Foundation for user-declared connector dependencies: - SDK: add `runtime.nix.packages` to ConnectorRuntimeInfo so a connector declares its native (nixpkgs) tools; rides the existing `runtime` JSONB, no migration. npm deps are bundled at compile time and don't go here. - Compile: externalize `@lobu/connector-sdk` (+ `lobu` alias) instead of bundling it. The SDK pulls a large infra graph (Sentry/OTel/grpc/git) transitively, inflating every connector to multiple MB; the runtime provides the SDK (it's a connector-worker dep), so the bundle leaves it as a runtime-resolved import — the standard externalize-the-framework pattern. Bundles drop from ~MB to the connector's own code + deps. Also emit an inline source map (sourcesContent:false) for stack traces. - Executor: when a connector declares nix packages, wrap the child in `nix-shell -p <pkgs> --run "exec node ..."` so the tools are on PATH; `exec` preserves the IPC channel and kill semantics. Plain fork() stays the path when no packages are declared. Fail with a clear actionable error when nix-shell is absent but packages are required.
Threads a connector's declared native packages from storage to the executor so they're on PATH during a run: - worker-api poll: join connector_definitions and surface its `runtime`, emitting `nix_packages` in the poll response (the `runtime.nix.packages` the SDK extraction already persists in the existing runtime JSONB — no storage change needed). - daemon: carry `nix_packages` on PollResponse and pass it through the three executeCompiledConnector call sites (sync/action/auth). - executeCompiledConnector: forward `nixPackages` to executor.execute, which wraps the child in nix-shell when non-empty. Connectors that declare no native deps are unaffected (plain fork path).
…ckaging Connector npm deps now work in the apply→cloud path: - apply: project-supplied connectors are compiled on the CLI (the only place the project's node_modules exists, so esbuild can bundle the connector's declared deps) and uploaded as a pre-compiled bundle (`compiled: true`); the server stores the artifact instead of recompiling source it can't resolve deps for. - ensure-deps-installed: runs `bun install --ignore-scripts` in the connector's project (resolved via the nearest `lobu.toml`, so a connector inside a monorepo never installs the wrong root); no-ops when the project declares no package.json. - client.installConnector gains a `compiled` flag. - lobu init scaffolds package.json (with @lobu/connector-sdk devDependency for editor types) + tsconfig.json + connectors/. - AGENTS.md documents the convention: npm = bundled at compile, native = nix at run time; SDK is runtime-provided/externalized.
Keep esbuild + connector-worker + SDK out of apply-cmd's module-load path (only `lobu apply` with local *.connector.ts needs it). Documented in the AGENTS.md dynamic-import allow-list. Matches connector-run-cmd's pattern.
…oin, init merge - apply: compile `def.sourcePath` (the actual `.ts`) for local connectors, not `def.sourceFile` (an error-message label that may point at a `type: connector` YAML). `source_url` connectors (source fetched into `sourceCode`, no local deps) upload raw for gateway compile instead of being compiled locally. - worker-api: filter the connector_definitions join to `status = 'active'` so it matches the partial unique index `idx_connector_defs_org_key` — archived/draft rows share `(key, org)` and made the runtime lookup nondeterministic. - init: merge into an existing `package.json` (preserve the user's fields, add the SDK devDependency) and never overwrite an existing `tsconfig.json`, so `--here` into an existing project is non-destructive. - docs: note `bun install --ignore-scripts` (the actual command).
18e953b to
203b82f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/init.ts (1)
295-336: ⚡ Quick winWire the database choice to a CLI flag (or remove the URL branch).
At Line 295,
flagis hardcoded toundefined, sodatabaseChoicecan only be'embedded'/'external'from the select prompt. That makes the URL branch at Line 329 effectively unreachable and blocks non-interactive selection of an external DB in--yesflows.💡 Suggested minimal patch
export interface InitOptions { yes?: boolean; here?: boolean; + database?: string; port?: string; publicUrl?: string; network?: string; @@ const databaseChoice = await promptOrDefault({ - flag: undefined, + flag: options.database, useDefaults, defaultValue: "embedded",🤖 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/cli/src/commands/init.ts` around lines 295 - 336, The select prompt's flag is set to undefined so a passed CLI value never populates databaseChoice, making the URL branch unreachable; set the select's flag (e.g., flag: "database") so CLI-supplied values are honored, keep the validate function that accepts "embedded"|"external"|/^(postgres(ql)?|file):/i so URLs validate, and ensure subsequent logic uses the resulting databaseChoice to populate databaseUrl (so the existing URL branch at the databaseUrl assignment becomes reachable in non-interactive --yes flows).
🤖 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.
Nitpick comments:
In `@packages/cli/src/commands/init.ts`:
- Around line 295-336: The select prompt's flag is set to undefined so a passed
CLI value never populates databaseChoice, making the URL branch unreachable; set
the select's flag (e.g., flag: "database") so CLI-supplied values are honored,
keep the validate function that accepts
"embedded"|"external"|/^(postgres(ql)?|file):/i so URLs validate, and ensure
subsequent logic uses the resulting databaseChoice to populate databaseUrl (so
the existing URL branch at the databaseUrl assignment becomes reachable in
non-interactive --yes flows).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff32e53c-0844-4b8c-8a4c-d1ff81197e12
📒 Files selected for processing (13)
AGENTS.mdpackages/cli/src/commands/_lib/apply/apply-cmd.tspackages/cli/src/commands/_lib/apply/client.tspackages/cli/src/commands/_lib/ensure-deps-installed.tspackages/cli/src/commands/init.tspackages/connector-sdk/src/connector-types.tspackages/connector-worker/src/compile/index.tspackages/connector-worker/src/daemon/client.tspackages/connector-worker/src/daemon/executor.tspackages/connector-worker/src/executor/interface.tspackages/connector-worker/src/executor/runtime.tspackages/connector-worker/src/executor/subprocess.tspackages/server/src/worker-api.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
|
bug_free 95, simplicity 95, slop 0, bugs 0, 0 blockers [env] Integration tests failed due to ENOSPC (no space left on device) during postgres DB initialization, unrelated to the diff. Full verdict JSON{
"bug_free_confidence": 95,
"bugs": 0,
"slop": 0,
"simplicity": 95,
"blockers": [],
"change_type": "feat",
"behavior_change_risk": "low",
"tests_adequate": true,
"suggested_fixes": [],
"notes": "[env] Integration tests failed due to ENOSPC (no space left on device) during postgres DB initialization, unrelated to the diff.",
"categories": {
"src": 376,
"tests": 0,
"docs": 6,
"config": 0,
"deps": 0,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
…ed-pg dylib symlinks
Two pre-existing macOS-only blockers that fail `make review` locally (both
already green on CI's Linux runners), surfaced while validating this PR:
- connector-sdk: a barrel re-export of the value+type dual name
`AutoCreateWhenRule` trips bun's cross-file module lexer in the test runner
("Export named ... not found"), failing the CLI test suite on macOS. Add a
`./identity-types` subpath export and import the value from it in the memory
CLI's schema (bypasses the barrel). CLI suite: 4 fail → 0 fail (308 pass).
- pgvector-embedded: the @embedded-postgres/darwin-* packages ship only
fully-versioned `lib*.<major>.<minor>.dylib` ICU libs; postgres/initdb link
against the `lib*.<major>.dylib` SONAME and the unversioned `lib*.dylib`
linker name, so initdb aborted with "Library not loaded: libicui18n.dylib".
injectPgvector now hydrates both symlink levels idempotently (macOS only).
Embedded Postgres initdb + start now succeed on darwin-arm64.
…976) (#984) * test: harden flaky port allocation + connector-sdk re-export barrel (#976) Two pre-existing test flakes from #976 (both intermittent, CI green; flake- reduction, not broken tests): - Port collisions: embedded-postgres-backend.ts and proxy-hardening.test.ts picked random high ports in overlapping ranges (50000-60000 vs 10000-60000) with no collision handling, racing to EADDRINUSE under concurrent test load. Add a shared free-port helper (bind :0, read assigned port, retry on EADDRINUSE) and route both sites through it. - connector-sdk AutoCreateWhenRule: re-export the identity-types schemas via import + a local export list instead of a transitive `export {…} from` barrel, so bun's ESM linker stops intermittently failing to resolve the name under concurrent load. Preventive — see PR body (could not reproduce on current main, likely already mitigated by #973 externalizing the SDK). * test: re-tag embedded-postgres bind failure as EADDRINUSE so retry fires Review finding (pi): embedded-postgres rejects start() with `undefined` on any early exit — a port collision included — so the OS-level EADDRINUSE never reaches withFreePortRetry's catch, leaving the retry dead for this caller. Capture stderr via `onLog` and re-tag a bind failure (`could not bind` / `address already in use`) as an Error with code='EADDRINUSE' so the wrapper retries; surface any other failure as a real Error instead of the original `undefined` reject. Validated red->green with a real PG-vs-PG collision: start() rejects with `undefined` (raw err type: undefined), onLog captures the bind error, and the catch re-tags it EADDRINUSE.
Summary
Lets connectors declare and use dependencies:
package.json(next tolobu.toml), bundled into the connector by esbuild at compile time. esbuild resolves a connector's imports relative to the connector file's directory, so the project'snode_modulesis used regardless of where thelobubinary is installed (global / npx / local).runtime.nix.packageson the connectordefinition; provisioned on PATH vianix-shellat execution time.lobu applyrunsbun installin the project, compiles each connector with the project's deps, and uploads the pre-compiled bundle (compiled: true) — the server can't bundle deps it never receives.@lobu/connector-sdkis externalized (runtime-provided, like Lambda'saws-sdk) instead of bundled, so a connector artifact is its own code + deps (KB) rather than the SDK's full infra graph (Sentry/OTel/grpc/git, ~6+ MB).lobu initscaffoldspackage.json(with@lobu/connector-sdkdevDependency for editor types) +tsconfig.json+connectors/.Commits
feat(connector-worker)— SDKruntime.nix.packagestype; compile externalizes the SDK + inline sourcemap; executor wraps the child innix-shell -p … --run "exec node …"(preserves IPC + kill) with a graceful "nix not installed" error.feat(server)— poll response surfacesnix_packages(read from the existingruntimeJSONB — no migration) → daemon → executor.feat(cli)— compile-on-apply +bun install --ignore-scripts(project root resolved via nearestlobu.toml);lobu initscaffolding; AGENTS.md docs.refactor(cli)— lazy-load the connector-compile graph in apply (allow-listed) to keep it off apply-cmd's module-load path.Validation
node_modules(not the CLI's). ✅make typecheck(strict, Dockerfile-equivalent): clean. ✅bun test packages/connector-worker: 52 pass / 0 fail. ✅packages/cli/.../apply) in isolation: 130 pass / 0 fail. ✅Test plan
lobu applya project whose connector declares an npm dep +runtime.nix.packages, on a gateway, and confirm a run succeeds with the tool on PATH.Notes
bun test packages/cli(full suite) shows 4AutoCreateWhenRuleerrors only on macOS/node-26/bun-1.3.5 — a pre-existing bun test-runner quirk on a value+type dual re-export in connector-sdk, reproduced identically on the base commit and unrelated to this PR. Each file passes in isolation; standalone import works; CI (Linux) is unaffected.Summary by CodeRabbit
New Features
Documentation
Reliability