refactor(connectors): consolidate V1 IPC, prune V0 taxonomy, unify compile pipeline#931
Merged
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (45)
💤 Files with no reviewable changes (21)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughThis PR centralizes connector compilation in a shared compile module, changes the executor contract from context/contents to mode-discriminated job/result with event streaming, updates CLI/server/daemon/subprocess/child-runner to the new contract, makes ConnectorRuntime.execute() provide a default, and prunes SDK exports and redundant connector action stubs. ChangesExecutor Contract Refactor & Shared Compilation Pipeline
ConnectorRuntime Default Execute & SDK Export Pruning
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/connector-worker/src/executor/child-runner.ts (1)
128-174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the streamed event count for the
items_foundfallback.When
syncResult.metadata?.items_foundis absent, this falls back totrailingEvents.length. Any connector that only usesemitEvents()and returns noeventsarray will therefore report0items found despite having streamed events successfully.Suggested fix
// mode === 'sync' + let emittedEventCount = 0; const emitEvents = async (events: EventEnvelope[]) => { + emittedEventCount += events.length; for (let index = 0; index < events.length; index += EVENT_CHUNK_SIZE) { await sendIPC({ type: 'event_chunk', events: events.slice(index, index + EVENT_CHUNK_SIZE), }); @@ metadata: { items_found: typeof syncResult?.metadata?.items_found === 'number' ? syncResult.metadata.items_found - : trailingEvents.length, + : emittedEventCount, items_skipped: typeof syncResult?.metadata?.items_skipped === 'number' ? syncResult.metadata.items_skipped : 0,🤖 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/connector-worker/src/executor/child-runner.ts` around lines 128 - 174, The fallback for metadata.items_found currently uses trailingEvents.length which misses events streamed via emitEvents; declare a counter (e.g., streamedEventsCount = 0) in the same scope, increment it inside emitEvents by events.length before sending chunks, and then change the items_found fallback to use streamedEventsCount (or streamedEventsCount || trailingEvents.length) when syncResult?.metadata?.items_found is absent; reference emitEvents, trailingEvents, syncResult, and metadata in your change.
🤖 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/connector-worker/src/executor/subprocess.ts`:
- Around line 318-328: The handler currently trusts msg.result after a bare cast
and directly calls settle(() => resolve(result)); add explicit validation of the
child result object (the msg.type === 'result' branch) before resolving: ensure
the object has a valid mode string (e.g., 'sync' or 'async') and that required
fields for each mode are present (for example, when result.mode === 'sync'
validate result.checkpoint exists and is well-formed; validate any auth/token
fields if expected), and if validation fails, call settle(() => reject(new
Error(...))) or otherwise handle the malformed payload (log and ignore) instead
of resolving; implement this as a small type-guard/validator used right before
the code that sets latestCheckpoint and calls settle(() => resolve(result)) so
only verified ExecutorResult objects are accepted.
In `@packages/server/src/tools/admin/manage_operations.ts`:
- Around line 215-223: The job config currently builds only from envStrings and
connectionCredentials, which drops saved per-connection settings; update the job
config creation in manage_operations.ts (the job: { config: ... } block) to
merge connection.config into job.config using the same helper/merge/precedence
you use for feed execution so connection.config values are preserved (e.g.,
combine connection.config, envStrings, and connectionCredentials in the same
order/merge function you use elsewhere).
In `@packages/server/src/utils/connector-catalog.ts`:
- Around line 5-8: The imports createConnectorCompiler and
findBundledConnectorFile (aliased as findInDirs) from
'`@lobu/connector-worker/compile`' fail because the compile subpath dist artifacts
are missing; before merging, build the connector-worker package so that
packages/connector-worker/dist/compile exists by running the repo build target
(e.g., make build-packages) to compile TypeScript from src → dist, then verify
the compiled export map resolves so imports used in connector-catalog.ts (and
other consumers like connector-compiler.ts, server.ts, CLI) can be resolved at
runtime.
---
Outside diff comments:
In `@packages/connector-worker/src/executor/child-runner.ts`:
- Around line 128-174: The fallback for metadata.items_found currently uses
trailingEvents.length which misses events streamed via emitEvents; declare a
counter (e.g., streamedEventsCount = 0) in the same scope, increment it inside
emitEvents by events.length before sending chunks, and then change the
items_found fallback to use streamedEventsCount (or streamedEventsCount ||
trailingEvents.length) when syncResult?.metadata?.items_found is absent;
reference emitEvents, trailingEvents, syncResult, and metadata in your change.
🪄 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: 2fdeb497-de7c-437a-81bd-304eb60cf50b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
packages/cli/src/commands/_lib/connector-loader.tspackages/cli/src/commands/_lib/connector-run-cmd.tspackages/connector-sdk/src/connector-runtime.tspackages/connector-sdk/src/connector-types.tspackages/connector-sdk/src/event-taxonomy.tspackages/connector-sdk/src/index.tspackages/connector-sdk/src/types.tspackages/connector-worker/integration-tests/subprocess-source-mode.test.tspackages/connector-worker/integration-tests/subprocess.test.tspackages/connector-worker/package.jsonpackages/connector-worker/src/__tests__/executor-heartbeat.test.tspackages/connector-worker/src/compile-connector.tspackages/connector-worker/src/compile/index.tspackages/connector-worker/src/daemon/executor.tspackages/connector-worker/src/executor/child-runner.tspackages/connector-worker/src/executor/interface.tspackages/connector-worker/src/executor/runtime.tspackages/connector-worker/src/executor/subprocess.tspackages/connectors/src/README.mdpackages/connectors/src/capterra.tspackages/connectors/src/g2.tspackages/connectors/src/glassdoor.tspackages/connectors/src/gmaps.tspackages/connectors/src/google_play.tspackages/connectors/src/hackernews.tspackages/connectors/src/ios_appstore.tspackages/connectors/src/linkedin.tspackages/connectors/src/microsoft_outlook.tspackages/connectors/src/producthunt.tspackages/connectors/src/reddit.tspackages/connectors/src/revolut.tspackages/connectors/src/rss.tspackages/connectors/src/spotify.tspackages/connectors/src/trustpilot.tspackages/connectors/src/website.tspackages/connectors/src/whatsapp.tspackages/connectors/src/x.tspackages/connectors/src/youtube.tspackages/server/package.jsonpackages/server/src/lib/feed-sync.tspackages/server/src/server.tspackages/server/src/tools/admin/manage_operations.tspackages/server/src/utils/connector-catalog.tspackages/server/src/utils/connector-compiler.ts
💤 Files with no reviewable changes (21)
- packages/connector-sdk/src/event-taxonomy.ts
- packages/connectors/src/glassdoor.ts
- packages/connectors/src/revolut.ts
- packages/connectors/src/whatsapp.ts
- packages/connectors/src/youtube.ts
- packages/connectors/src/rss.ts
- packages/connectors/src/google_play.ts
- packages/connectors/src/capterra.ts
- packages/connectors/src/hackernews.ts
- packages/connector-sdk/src/connector-types.ts
- packages/connectors/src/g2.ts
- packages/connectors/src/ios_appstore.ts
- packages/connectors/src/microsoft_outlook.ts
- packages/connectors/src/trustpilot.ts
- packages/connectors/src/spotify.ts
- packages/connectors/src/x.ts
- packages/connectors/src/producthunt.ts
- packages/connectors/src/linkedin.ts
- packages/connectors/src/reddit.ts
- packages/connectors/src/gmaps.ts
- packages/connectors/src/website.ts
Comment on lines
318
to
+328
| if (msg.type === 'result') { | ||
| terminalMessageReceived = true; | ||
| const result = msg.result as FeedSyncResult; | ||
| const result = msg.result as ExecutorResult; | ||
| queueTask(async () => { | ||
| finalMetadata = result.metadata; | ||
| finalAuthUpdate = result.auth_update; | ||
| finalAuthResult = result.auth_result; | ||
| latestCheckpoint = result.checkpoint; | ||
|
|
||
| if (Array.isArray(result.contents) && result.contents.length > 0) { | ||
| if (collectContents) { | ||
| collectedContents.push(...result.contents); | ||
| } | ||
| await hooks?.onContentChunk?.(result.contents); | ||
| // For sync results, surface the trailing checkpoint to callers | ||
| // through the result; in-flight `checkpoint_update` messages have | ||
| // already been forwarded via the hook. | ||
| if (result.mode === 'sync') { | ||
| latestCheckpoint = result.checkpoint; | ||
| } | ||
|
|
||
| settle(() => | ||
| resolve({ | ||
| contents: collectedContents, | ||
| checkpoint: latestCheckpoint, | ||
| metadata: finalMetadata, | ||
| auth_update: finalAuthUpdate, | ||
| auth_result: finalAuthResult, | ||
| }) | ||
| ); | ||
| settle(() => resolve(result)); |
There was a problem hiding this comment.
Validate child result messages before resolving them.
This path now trusts msg.result after a bare cast. Since connector code shares the child process, it can send an arbitrary { type: 'result' } payload and hand the parent a malformed ExecutorResult or bogus checkpoint/auth data. Please hard-validate mode and its required fields at this boundary before calling resolve(...).
Suggested guard
if (msg.type === 'result') {
terminalMessageReceived = true;
- const result = msg.result as ExecutorResult;
+ const result = msg.result;
queueTask(async () => {
- // For sync results, surface the trailing checkpoint to callers
- // through the result; in-flight `checkpoint_update` messages have
- // already been forwarded via the hook.
- if (result.mode === 'sync') {
- latestCheckpoint = result.checkpoint;
- }
- settle(() => resolve(result));
+ if (!result || typeof result !== 'object' || typeof result.mode !== 'string') {
+ throw new Error('Child returned an invalid executor result');
+ }
+ if (result.mode === 'sync') {
+ latestCheckpoint =
+ typeof result.checkpoint === 'object' || result.checkpoint === null
+ ? result.checkpoint
+ : null;
+ } else if (result.mode === 'action') {
+ if (!result.output || typeof result.output !== 'object') {
+ throw new Error('Child returned an invalid action result');
+ }
+ } else if (result.mode === 'authenticate') {
+ if (!result.auth || typeof result.auth !== 'object') {
+ throw new Error('Child returned an invalid auth result');
+ }
+ } else {
+ throw new Error(`Child returned unknown mode: ${String(result.mode)}`);
+ }
+ settle(() => resolve(result as ExecutorResult));
});
return;
}📝 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
| if (msg.type === 'result') { | |
| terminalMessageReceived = true; | |
| const result = msg.result as FeedSyncResult; | |
| const result = msg.result as ExecutorResult; | |
| queueTask(async () => { | |
| finalMetadata = result.metadata; | |
| finalAuthUpdate = result.auth_update; | |
| finalAuthResult = result.auth_result; | |
| latestCheckpoint = result.checkpoint; | |
| if (Array.isArray(result.contents) && result.contents.length > 0) { | |
| if (collectContents) { | |
| collectedContents.push(...result.contents); | |
| } | |
| await hooks?.onContentChunk?.(result.contents); | |
| // For sync results, surface the trailing checkpoint to callers | |
| // through the result; in-flight `checkpoint_update` messages have | |
| // already been forwarded via the hook. | |
| if (result.mode === 'sync') { | |
| latestCheckpoint = result.checkpoint; | |
| } | |
| settle(() => | |
| resolve({ | |
| contents: collectedContents, | |
| checkpoint: latestCheckpoint, | |
| metadata: finalMetadata, | |
| auth_update: finalAuthUpdate, | |
| auth_result: finalAuthResult, | |
| }) | |
| ); | |
| settle(() => resolve(result)); | |
| if (msg.type === 'result') { | |
| terminalMessageReceived = true; | |
| const result = msg.result; | |
| queueTask(async () => { | |
| if (!result || typeof result !== 'object' || typeof result.mode !== 'string') { | |
| throw new Error('Child returned an invalid executor result'); | |
| } | |
| if (result.mode === 'sync') { | |
| latestCheckpoint = | |
| typeof result.checkpoint === 'object' || result.checkpoint === null | |
| ? result.checkpoint | |
| : null; | |
| } else if (result.mode === 'action') { | |
| if (!result.output || typeof result.output !== 'object') { | |
| throw new Error('Child returned an invalid action result'); | |
| } | |
| } else if (result.mode === 'authenticate') { | |
| if (!result.auth || typeof result.auth !== 'object') { | |
| throw new Error('Child returned an invalid auth result'); | |
| } | |
| } else { | |
| throw new Error(`Child returned unknown mode: ${String(result.mode)}`); | |
| } | |
| settle(() => resolve(result as ExecutorResult)); | |
| }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/connector-worker/src/executor/subprocess.ts` around lines 318 - 328,
The handler currently trusts msg.result after a bare cast and directly calls
settle(() => resolve(result)); add explicit validation of the child result
object (the msg.type === 'result' branch) before resolving: ensure the object
has a valid mode string (e.g., 'sync' or 'async') and that required fields for
each mode are present (for example, when result.mode === 'sync' validate
result.checkpoint exists and is well-formed; validate any auth/token fields if
expected), and if validation fails, call settle(() => reject(new Error(...))) or
otherwise handle the malformed payload (log and ignore) instead of resolving;
implement this as a small type-guard/validator used right before the code that
sets latestCheckpoint and calls settle(() => resolve(result)) so only verified
ExecutorResult objects are accepted.
Comment on lines
+215
to
+223
| job: { | ||
| mode: 'action', | ||
| actionKey: | ||
| operation.backend_config.backend === 'local_action' | ||
| ? operation.backend_config.actionKey | ||
| : operation.operation_key, | ||
| actionInput, | ||
| config: { ...envStrings, ...connectionCredentials }, | ||
| env: envStrings, |
There was a problem hiding this comment.
Keep connection.config in the action job config.
This path now sends only env strings plus resolved credentials. Any local action that reads saved per-connection settings from context.config will start running as if the connection were unconfigured. Merge connection.config into job.config here, ideally with the same helper/precedence you use for feed execution.
🤖 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/tools/admin/manage_operations.ts` around lines 215 - 223,
The job config currently builds only from envStrings and connectionCredentials,
which drops saved per-connection settings; update the job config creation in
manage_operations.ts (the job: { config: ... } block) to merge connection.config
into job.config using the same helper/merge/precedence you use for feed
execution so connection.config values are preserved (e.g., combine
connection.config, envStrings, and connectionCredentials in the same order/merge
function you use elsewhere).
Removes re-exports/types with zero consumers across packages/, examples/, scripts/, config/ outside of the SDK itself: - FeedMode enum (`'sync' | 'virtual'`) — only ever lived in the SDK - MatchStrategy — used internally inside identity-types but never imported externally; keep the value, drop the re-export - isSourceNativeEventType / SOURCE_NATIVE_EVENT_TYPES + event-taxonomy.ts — whole file existed only to be re-exported, no consumers anywhere - ReactionEntity — embedded in ReactionContext; nothing imports it directly - Stealth/CDP browser re-exports (humanWait, randomScroll, getRandomDelay, testBotDetection, StealthBrowser, launchStealthBrowser, withErrorCapture, withHttpRetry, BrowserAuthCascadeError, BrowserNetworkConfig, discoverChromeListeningPorts, discoverChromeProcessCdpUrls, normalizeCdpUrl, tryWebSocketCdp) — kept internal to the SDK where launcher/acquire use them; the index just stops claiming they're public surface. Rationale: SDK is the public surface third-party connectors consume. Every re-export is a backwards-compat obligation. Verified zero external uses with `rg -l "\bSYM\b" packages/ --type=ts | grep -v connector-sdk/src` per symbol before deletion. Typecheck clean: packages/connector-sdk, packages/connectors (modulo pre-existing whatsapp baileys-typings drift), packages/connector-worker, packages/cli, packages/server (modulo pre-existing guardrails/aggregator.ts SkillPreToolGuardrail breakage on main).
…r IPC
The connector-worker executor stack was a multi-hop adapter that
repackaged the V1 ConnectorRuntime contract (SyncContext/ActionContext/
AuthContext + EventEnvelope) into a legacy "feed/options" shape with
magic underscore-prefixed keys (__action_key, __auth_mode, __feed_key,
__entity_ids, __auth_config, __auth_previous_credentials), serialised
that over IPC, unpacked it back to V1 in the child runner, then packed
the result into a legacy `FeedSyncResult { contents: Content[] }`,
serialised that, unpacked it in the daemon executor, and re-normalised
each item a second time inside `processContent`.
Concretely deleted:
- `Content`, `Checkpoint`, `FeedOptions`, `FeedSyncResult`, `SessionState`
from `packages/connector-sdk/src/types.ts` (only `Env` remains — it has
20+ downstream consumers in the gateway). The SDK now exposes one
taxonomy: `SyncContext` / `ActionContext` / `AuthContext` /
`EventEnvelope` from `connector-types.ts`.
- `buildConnectorExecutionContext`, `mergeExecutionSessionState`,
`mergeExecutionEnv`, `normalizeEventEnvelope`, `getActionOutput` from
`connector-worker/src/executor/runtime.ts` (~120 LOC of pack/unpack).
- The `__action_key` / `__auth_mode` / `__feed_key` / `__entity_ids` /
`__auth_config` / `__auth_previous_credentials` / `stripInternalOptions`
protocol on the IPC boundary.
- The double `normalizeEventEnvelope` call (child runner already
normalises, daemon's `processContent` then normalised again on the
resulting `Content` — a no-op pass on already-normalised fields).
The new contract: `ExecutorJob` is a discriminated union (`mode: 'sync' |
'action' | 'authenticate'`) carrying the V1 SDK shapes verbatim, and
`ExecutorResult` mirrors `SyncResult` / `ActionResult` / `AuthResult`
the same way. `child-runner.ts` reads the job and dispatches; the
parent's `subprocess.ts` is mode-agnostic (it just plumbs events,
checkpoints, and auth artifacts through hooks).
Net diff: -242 LOC across the in-scope packages (sdk + worker), and the
result/return paths from `executeSyncRun`, `executeActionRun`,
`executeAuthRun` collapse from "consume legacy FeedSyncResult and unpack
the right slot" to mode-narrowed direct access.
Typecheck status:
- packages/connector-sdk: clean
- packages/connector-worker: clean
- packages/connectors: clean (pre-existing whatsapp baileys-typings
drift in src/whatsapp.ts only)
- packages/cli: clean
- packages/server: two BREAKING call-sites under
packages/server (out of spike scope):
src/lib/feed-sync.ts (executeCompiledConnector
still passing the flat legacy params)
src/tools/admin/manage_operations.ts
(getActionOutput re-export gone)
These reach across the package boundary
into connector-worker internals via
relative path / dynamic import — flagged
as architectural leak in REPORT.md.
Updating them is a one-screen rewrite to
the new `{job: {mode, ...}}` signature plus
`result.output` / `result.events` mode
narrowing. Left for a focused follow-up
PR so the spike stays within the read-only
constraint on out-of-scope packages.
The legacy test (`executor-heartbeat.test.ts`) was updated to the new
shape; the underlying `bun test` integration cannot run in this worktree
(unrelated `@lobu/embeddings` module-resolution issue that reproduces on
the pre-spike commit too, recorded in REPORT.md).
…sites
Updates the two server callers that reached into `connector-worker`'s
executor internals via relative path + dynamic import (flagged as an
abstraction leak in REPORT.md spike 2):
- `packages/server/src/lib/feed-sync.ts:7` was importing from
`'../../../connector-worker/src/executor/runtime'` — now goes through
the proper `@lobu/connector-worker/executor/runtime` package export.
The sync call switches to the new `{ compiledCode, job: { mode: 'sync',
... }, hooks }` signature; item-count tracking moves from
`result.contents.length` to an `onEventChunk` counter (events are
streamed via the hook now, not collected in the result).
- `packages/server/src/tools/admin/manage_operations.ts:207` used a
dynamic `await import("../../../../connector-worker/...")` to grab
`executeCompiledConnector` + `getActionOutput`. The latter no longer
exists (spike 2 deleted it), so this call-site goes through
`@lobu/connector-worker/executor/runtime` for `executeCompiledConnector`
alone and mode-narrows directly on `result.output`.
Typecheck status:
- packages/connector-sdk: clean
- packages/connector-worker: clean
- packages/connectors: clean (modulo pre-existing whatsapp/baileys
drift on main)
- packages/cli: clean
- packages/server: clean (modulo pre-existing
gateway/guardrails/aggregator.ts errors on
main)
`make build-packages` was run to refresh the connector-worker .d.ts so
the new exports resolve through the package boundary instead of through
the workspace .ts sources.
…te stubs
Most connectors don't expose actions — `rg -c "Actions not supported"
packages/connectors/src` returned 19 identical paste-jobs of
async execute(_ctx: ActionContext): Promise<ActionResult> {
return { success: false, error: 'Actions not supported' };
}
(one minor variant: whatsapp's `'Actions not supported in v1.'`, and
revolut's tab-indented double-quoted version). Parallel to how
`authenticate()` already had a default-throws implementation, this turns
`execute()` from `abstract` into a default-rejecting method on the base
class. Connectors that don't declare any `actions` in their
`ConnectorDefinition` no longer need to write the stub.
Concrete deltas:
- `packages/connector-sdk/src/connector-runtime.ts`: `execute` becomes a
non-abstract method returning `{ success: false, error: 'Actions not
supported' }`. Doc comment updated to call out the new optionality.
- 19 connectors lose the 3-line stub and the dead
`type ActionContext, type ActionResult` imports it required:
capterra, g2, glassdoor, gmaps, google_play, hackernews,
ios_appstore, linkedin, microsoft_outlook, producthunt, reddit,
revolut, rss, spotify, trustpilot, website, whatsapp (kept Auth
imports for `authenticate`), x, youtube.
Net: -114 LOC across packages/connectors, +10 LOC in the SDK base class.
Compatibility checks:
- `child-runner.ts findRuntimeClass` checks `prototype?.sync` AND
`prototype?.execute`. The inherited default on `ConnectorRuntime.prototype`
still satisfies `Foo.prototype.execute` access (prototype-chain lookup),
so the discovery heuristic continues to accept these subclasses.
- The 8 device-only "throws BRIDGE_ONLY from execute()" stubs in
apple_health, apple_photos, apple_screen_time, chrome,
chrome_bookmarks, chrome_downloads, chrome_history, local_directory,
whatsapp_local are NOT touched — those communicate "this connector
only runs on a device worker" (a different semantic from "actions not
supported") and the user's brief was specifically the 16+ "Actions not
supported" pastes.
Typecheck status (after `bun run build` in connector-sdk to regenerate
.d.ts so connector-package consumers see the new default):
- packages/connector-sdk: clean
- packages/connector-worker: clean
- packages/connectors: 126 errors — IDENTICAL count to pre-spike
baseline (verified via git stash + recount).
All are the pre-existing whatsapp baileys
typings drift plus the dom-lib / .ts-import-
extension / unknown-type drift in capterra,
g2, glassdoor, etc. on main. None are caused
by this spike.
- packages/cli: clean
- packages/server: clean (modulo pre-existing
gateway/guardrails/aggregator.ts errors).
Three packages each shipped their own near-identical copy of
`findBundledConnectorFile` + `compileConnectorFromFile` + the `npm:`
specifier esbuild plugin + the `lobu`/`@lobu/connector-sdk` aliasing +
the mtime-keyed LRU compile cache:
- packages/connector-worker/src/compile-connector.ts (~150 LOC)
- packages/cli/src/commands/_lib/connector-loader.ts (~120 LOC)
- packages/server/src/utils/connector-catalog.ts (~150 LOC of the
file, the rest
stays — metadata
extraction, URI
normalisation,
catalog iteration)
Comments in two of the three already said things like "single source of
truth lives in @lobu/connector-worker. Duplicated here as a flat string
array to keep this file self-contained" and "Mirror the worker-side
resolver in compile-connector.ts: try the subdir layout first … Keep
these in sync if either side changes." That's the drift surface this
spike closes.
New shared module: `packages/connector-worker/src/compile/index.ts`,
exported as `@lobu/connector-worker/compile`. (Originally `./build` but
the repo root `.gitignore` has `**/build/` which silently ate the new
directory; renamed to `./compile` so it tracks.)
Surface:
- `EXTERNAL_RUNTIME_DEPS` — re-exported from `runtime-deps.ts`.
- `findBundledConnectorFile(key, candidateDirs)` — pure helper; the
per-environment candidate-dir list is the parameter that genuinely
differs (worker image vs CLI dist-bundle vs gateway pod). No
internal cache; callers that need one (`server/connector-catalog.ts`,
`cli/connector-loader.ts`) wrap their own thin Map<key, path|null>.
- `createNpmSpecifierPlugin({ onUnresolved })` — esbuild plugin
parametrised on the warn hook (server wants to log via pino, CLI/
worker stay silent).
- `createConnectorCompiler({ cacheMax?, sdkEntry?, onUnresolvedNpm? })`
→ `{ compileConnectorFromFile }`. Each instance owns its own
8-entry LRU; callers that want per-process isolation create their
own.
Net diff: -381 / +296 (with the new 229-LOC shared module) = -85 LOC
across the three packages plus zero risk of the three implementations
drifting from each other.
Server now declares `@lobu/connector-worker: workspace:*` as a real dep
(it was already importing from it after the prior round-2 spike, but
without a corresponding package.json entry).
Typecheck status (`make typecheck` from worktree root): **clean.** The
pre-existing `gateway/guardrails/aggregator.ts` `SkillPreToolGuardrail`
errors that REPORT.md called out turned out to be stale dist artifacts
in this worktree — once `make build-packages` re-emitted `@lobu/core`'s
.d.ts, those resolved too. Whole repo typecheck is green.
Round-3 follow-up. Spike 9232088 deleted the legacy executor adapter in `@lobu/connector-worker` and migrated `daemon/executor.ts` plus the two `packages/server` call-sites. The CLI's `lobu connector run` path (`packages/cli/src/commands/_lib/connector-run-cmd.ts:434`) was missed and would have crashed at runtime on every invocation. The slip was masked by an `(executeCompiledConnector as any)` cast on the call site — typecheck couldn't see the contract mismatch. `make build-packages` is bundler-only and didn't catch it either. Concrete fixes: - Remove the `as any` cast on the call site so future contract drift is a typecheck error. - Replace the flat-shape call (`{ mode: 'sync', compiledCode, config, checkpoint, env, connectionCredentials, sessionState, credentials, feedKey, entityIds, apiType, hooks: { onContentChunk, collectContents } }`) with the V1 shape: `{ compiledCode, job: { mode: 'sync', config, checkpoint, env, sessionState, credentials, feedKey, entityIds }, hooks: { onEventChunk } }`. - Switch `onContentChunk` → `onEventChunk` and accumulate events into a local `EventEnvelope[]` (the executor used to collect them into `result.contents` for the caller via `collectContents: true`; that path is gone, sync is streaming-only now). - Replace all `result.contents.*` reads with `collectedEvents.*` for the artifact JSON, the summary line, and the `summarizeEvents` call. - Mode-narrow on `result.mode === 'sync'` before reading `result.checkpoint` / `result.metadata`. Verification (typecheck-alone-is-insufficient per the round-3 brief): 1. `bunx tsc --noEmit` in `packages/cli` — clean. 2. `make build-packages` — clean. 3. Runtime smoke test in `packages/cli`: - load `dist/commands/_lib/connector-run-cmd.js` and confirm the `connectorRun` export resolves; - dynamic-import `@lobu/connector-worker/executor/runtime` the same way the CLI does at runtime, confirm `executeCompiledConnector` resolves; - feed it a synthetic 5-line connector whose `sync()` throws `'intentional smoke-test failure'`, with the new V1 job shape; - assert the executor forks a subprocess, the subprocess loads the bundle, the connector throws, and the error round-trips back with the expected payload. Result: `OK: V1 contract end-to-end (subprocess threw expected payload)`. The OLD shape would have died with TypeError on `result.contents.length` or shape-validation in `buildConnectorExecutionContext`.
… comment
Round-3 follow-up. The `sync` arm of `ExecutorResult` advertised
`events: EventEnvelope[]` but `child-runner.ts` always returned
`events: []` because the V1 contract streams events via
`hooks.onEventChunk` and the executor doesn't collect on the result
side. The shape was lying. Picked the option the code already
implements: sync is streaming-only.
Concrete changes:
- `packages/connector-worker/src/executor/interface.ts`:
- `ExecutorResult` sync arm loses the `events` field. New doc-block
spells out the contract ("events leave via `hooks.onEventChunk`,
never collected onto the result — callers that need a list build
it themselves in the hook") and points at the CLI as the reference
consumer.
- Top-of-file comment no longer mentions the long-dead
`__action_key` / `__feed_key` / `__auth_mode` magic-key protocol.
`git grep` for those identifiers across the monorepo now returns
zero hits (verified post-commit).
- `packages/connector-worker/src/executor/child-runner.ts`:
- The post-`sync()` `events: []` field is gone from the returned
`ExecutorResult`.
- Renamed the local `events` variable to `trailingEvents` to make it
obvious it captures the *return-value* path (connectors that build
the full list before returning) as distinct from incremental
`emitEvents()` calls during sync. Both paths still funnel through
the same `event_chunk` IPC so the parent sees one uniform stream.
Verification:
- Monorepo grep `rg -n '__action_key|__feed_key|__auth_mode' packages/
examples/ scripts/ config/`: zero hits.
- `rg -n 'result\.events' packages/connector-worker/src packages/cli/src
packages/server/src`: the remaining hits are all unrelated (chat SSE
`eventsUrl`, embedding-fetch JSON response, the child-runner's own
read of `syncResult.events` from the SDK type — which is the
connector's `SyncResult.events`, not the executor's). No consumer
reads sync `result.events` from `ExecutorResult` anywhere.
- Typecheck: `connector-sdk`, `connector-worker`, `cli`, `server`
clean; `connectors` has only pre-existing whatsapp/dom-lib drift on
main.
Round-3 follow-up. Two server files still reached into
`packages/connector-worker/src/runtime-deps.ts` via relative path even
after round-2 spike `ad4e92d6` added `@lobu/connector-worker/compile`
and declared the package dep. Cross-package leaks like these silently
break the moment the worker's source layout shifts (dist-only consumer
runtimes, esm-vs-cjs flips, file relocations) and they're the exact
pattern round-3 was about closing.
- `packages/server/src/server.ts:44`: `import { assertExternalDepsResolvable }
from '../../connector-worker/src/runtime-deps'`
- `packages/server/src/utils/connector-compiler.ts:8`: `import {
EXTERNAL_RUNTIME_DEPS } from '../../../connector-worker/src/runtime-deps'`
Both now go through `@lobu/connector-worker/compile`. The compile module
already re-exported `EXTERNAL_RUNTIME_DEPS` (round-2 spike `ad4e92d6`);
this spike adds `assertExternalDepsResolvable` to the same export so
the boot-time helper has a public-export path too. Both belong in
`connector-worker` (not `@lobu/core`) because they describe deps the
worker's compile pipeline externalises — moving them out would split
that concern across two packages.
Verification:
- `rg -n "from ['\"]\\.\\./\\.\\./.*connector-worker|from ['\"]\\.\\./\\.\\./\\.\\./connector-worker"
packages/server packages/cli --type=ts`: zero hits.
- `make build-packages`: clean.
- `bunx tsc --noEmit` in `packages/server`: clean.
Round-3 follow-up covering targets 4 and 5.
- `packages/connector-sdk/src/connector-runtime.ts:72`: dropped the `_`
prefix on the `ctx` parameter of the base `execute()`. The repo
convention (AGENTS.md: "When fixing unused-parameter errors, delete
the parameter rather than prefixing with `_`") doesn't apply
literally here because the parameter is contractually required:
subclasses overriding `execute(ctx: ActionContext)` rely on the base
signature accepting an `ActionContext`. Verified by deleting the
param and watching every action-supporting connector
(`google_calendar`, `google_gmail`, `chrome`, `revolut`) fail
typecheck with `Target signature provides too few arguments.
Expected 1 or more, but got 0.`. Restored the param without the
underscore and added a single `// biome-ignore
lint/correctness/noUnusedFunctionParameters` line with the rationale
("contract signature — subclasses receive the full ActionContext")
so future readers see why the param looks unused but stays.
- `packages/connectors/src/README.md:353`: deleted the paragraph that
told new connector authors to paste the 3-line "actions not
supported" stub. Replaced with one sentence pointing at the new
default impl on `ConnectorRuntime` ("do nothing — the base class
ships a default `execute()` that returns `{ success: false, error:
'Actions not supported' }`. Omit the method entirely.").
Verification:
- `make typecheck` from the worktree root: clean.
- 19 connectors that already lost the stub in spike 02f990f still
compile against the new base.
Round-4 doc polish.
- `packages/connectors/src/README.md`: the Quick Start (top of file)
still showed the deleted `execute(_ctx: ActionContext): Promise<ActionResult>
{ return { success: false, error: 'Actions not supported' }; }` stub
on every new connector — contradicting the actions-not-supported
guidance later in the same file ("omit the method entirely"). Removed
the stub from the Quick Start class body and dropped the now-unused
`ActionContext` / `ActionResult` imports from its import block. The
later section that does show actions (the `github`-style example
around line 331) still imports them locally and is unchanged.
- `packages/cli/src/commands/_lib/connector-run-cmd.ts:344`: comment
said "Build the SyncContext shape that executeCompiledConnector
expects" — but as of spike `76a4677c` we build an `ExecutorJob`, not
a `SyncContext`. One-word edit.
Verification:
- `rg -n "execute\(_ctx|ActionContext|ActionResult" packages/connectors/src/README.md`
returns one hit (line 331 — the actions-supported example), not the
Quick Start or the no-actions guidance.
- `make typecheck`: clean.
Round-5 polish — final doc/fixture drift after the V1 IPC migration.
- `packages/connectors/src/README.md`: rewrote the "How connector code
runs" section. Pre-spike it described the gateway shipping
~13 MB `compiled_code` inline in every worker-poll response and the
worker loading it blindly. After spikes `ad4e92d6` (compile pipeline
consolidation) and `92320888` (V1 IPC), fleet workers receive only
`connector_key` and compile locally via the shared
`@lobu/connector-worker/compile` module; the gateway only inlines
`compiled_code` for device/DB-only workers without source on disk.
Also updated the IPC step to name the live wire format
(`ExecutorJob` / `ExecutorResult`, `event_chunk` for streaming).
- `packages/connector-worker/integration-tests/subprocess.test.ts` +
`subprocess-source-mode.test.ts`: fixtures still constructed a
`BASE_CONTEXT: SyncContext` shaped as the deleted V0
`{ options, checkpoint, env, apiType }`. Tests still passed because
the fixture was passed positionally and the diagnostic-capture tests
trigger crash/throw paths before the executor inspects most fields,
but the comment + type alias misled future readers. Migrated to
`BASE_JOB: ExecutorJob` with the V1 shape (`mode: 'sync'`, `feedKey`,
`config`, `checkpoint`, `entityIds`, `credentials`, `sessionState`,
`env`). Also fixed the synthetic-connector `compiled()` template: the
inline `execute()` returned `{ contents: [], checkpoint: null }`
(V0 `FeedSyncResult` shape) — replaced with V1 `ActionResult`
(`{ success: false, error: 'no actions' }`) so the fixture is
internally consistent with what `findRuntimeClass` expects.
Verification:
- `bun test packages/connector-worker/integration-tests`: 8 pass, 0 fail.
- `rg -n "\\bSyncContext\\b" packages/connector-worker packages/cli/src`:
remaining hits are all live SDK-type references (the executor
interface comment names the SDK type, the integration test's comment
explains the synthetic `sync(_ctx)` receives a V1 `SyncContext`) —
no stale fixture references. README `SyncContext` references all
document the live SDK type.
- `make typecheck`: clean.
Round-6 one-liner. compile-connector.ts:17 referenced './build' but the shared module was renamed to './compile' in round-2 (the gitignore gotcha — '\*\*/build/' silently swallowed the original dir name). Comment now matches the actual import path.
The integration job built only `core` + `connector-sdk` before invoking the server's vitest suite. After spike ad4e92d the server's `packages/server/src/utils/connector-catalog.ts` imports `@lobu/connector-worker/compile` — Node's package resolver follows the `exports` field to `./dist/compile/index.js` and fails when dist is absent. vitest's vite-node loader propagates the failure as a load error, breaking every integration test whose import graph transitively touches `queue-helpers` / `worker-api` / `connector-catalog` — all 30 files in the suite. The unit job already builds connector-worker (line 57-67); the integration job had a smaller list that pre-dated the shared-compile-module refactor. Root cause verification: 1. Reproduced locally by `rm -rf packages/connector-worker/dist` and running `LOBU_TEST_BACKEND=pglite vitest run` from packages/server: `Error: Failed to load url @lobu/connector-worker/compile … Does the file exist?` on every integration file — same symptom as the CI run #26117699107 attached to PR #931. 2. Rebuilt connector-worker (`cd packages/connector-worker && bun run build`), re-ran the same vitest invocation — 73 of 76 files load and pass. The 2 remaining failures are pre-existing on main (`run-script-runtime.test.ts` needs Node 22's isolated-vm abi127 prebuild; `install-operator.test.ts` fails identically on origin/main when checked out and re-run, both unrelated to this fix). Considered alternatives: - `"bun": "./src/compile/index.ts"` conditional export — rejected: the integration job runs Node (line 142 of ci.yml: "We run them under Node (not bun) … the sandbox runtime requires isolated-vm which is a V8 native addon that bun cannot load"), so the `bun` condition wouldn't activate. - `"node": "./src/compile/index.ts"` source-mapped export — rejected: the source is .ts; Node can't load .ts without a loader, and installing one for CI tests would also slow every Node consumer in production. The build-dist approach matches every other workspace package. - Switching to relative imports — rejected: that's the leak round 3 closed. The fix belongs in CI, not by undoing the package boundary. The added embeddings build is required because connector-worker's src/embeddings.ts imports `@lobu/embeddings` and resolves it via the workspace symlink → dist. Same dependency order the unit job already honors (line 66 of the unit job builds both in the same order).
buremba
force-pushed
the
feat/simplify-connector-stack
branch
from
a3c18d3 to
8563dc3
Compare
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.
Summary
Multi-round simplification on
packages/connector-sdk+packages/connector-worker+packages/connectors. Cumulative ~450 LOC net deletion across 45 files. Includes one runtime-break fix thatmake build-packageswould have shipped to prod.Key changes
options.__action_key/options.__auth_mode/options.__feed_key/options.__entity_ids/options.__auth_config; child-runner unpacked viastripInternalOptions. Invisible to every type. Now:ExecutorJobis a discriminated union (mode: 'sync' | 'action' | 'authenticate') carrying V1 SDK shapes verbatim.connector-sdk/src/types.tspreviously shipped a V0Content/Checkpoint/FeedSyncResult/FeedOptions/SessionStatefamily that nothing underpackages/connectors/actually used (31/31 connectors are V1). Deleted.connector-run-cmd.tspreviously passed flatmode/config/checkpoint/...+ oldonContentChunk/collectContentsand readresult.contents— would have failed in production after the IPC migration.make build-packageswas bundler-only and didn't catch it. Migrated to V1 contract + verified by built-dist round-trip test (synthetic 5-line connector with throwingsync(); asserted error round-trips throughonEventChunk).connector-worker/src/compile-connector.ts,cli/src/commands/_lib/connector-loader.ts,server/src/utils/connector-catalog.tspreviously each had their own LRU caches + esbuild config. Consolidated behind@lobu/connector-worker/compile.execute()stubs deleted frompackages/connectors/.ConnectorRuntime.execute()defaults to{ success: false, error: 'Actions not supported' }.ExecutorResultadvertisedeventsfor sync butchild-runneralways returnedevents: []. Removed.server.ts+connector-compiler.tspreviously reached into worker source via relative imports; now go through@lobu/connector-worker/compile.Process
Built across 6 rounds of spike commits driven by codex review. Final codex verdict: ENOUGH. All
wip(spike):commit history preserved on branch — squash on merge.Test plan
make typecheckcleanbun test packages/connector-worker/integration-testsclean (8/8)lobu connector run <key>against a compiled connector works (CLI runtime regression coverage)Summary by CodeRabbit
Refactor
Documentation