ci: connector runtime parity smoke gate#1037
Merged
Merged
Conversation
Guards against the packaging/parity drift that broke prod in #1035: the worker Docker image shipped missing `COPY packages/core`, so @lobu/connector-sdk's transitive @lobu/core import dangled and every connector feed sync crashed at runtime — yet all CI was green because the workspace tests resolve @lobu/core and build-images.yml builds + pushes images but never RUNS them. Add a single SHARED runtime self-check (packages/connector-worker/src/self-check) executed from BOTH the built CLI runtime (`lobu connector runtime-self-check`, hidden) and the worker image (`bun src/bin.ts self-check`). Both entrypoints call the SAME runConnectorRuntimeSelfCheck() over the same compile + default SubprocessExecutor path (the parity invariant; only the connector-source discovery roots differ). It asserts artifact/packaging facts only — @lobu/connector-sdk + @lobu/core resolve+import, EXTERNAL_RUNTIME_DEPS (playwright/sharp/jimp) resolve, the bundled connector source dir is discoverable, all discovered connectors instantiate with unique key/name/version, and a synthetic no-op connector compiles + runs through the default SubprocessExecutor. No network/DB/gateway/OAuth; passes under docker run --network=none. CI wiring: - ci.yml PR gate (path-filtered like mac-build-smoke): build worker image amd64 (load, no push), run the image self-check with --network=none, and run the CLI self-check on the host. - build-images.yml main/deploy gate: same amd64 image self-check as a job that build-worker (and transitively build-app) depends on, so a failing smoke blocks BOTH worker and app pushes — Flux can't roll the shared tag to a broken worker. Shares the buildx cache scope with build-worker.
|
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:
📝 WalkthroughWalkthroughAdds a connector runtime parity self-check module, exposes it from connector-worker, integrates it into worker and CLI entrypoints, and gates build/CI workflows (selectively via a filter) by running an amd64 worker image self-check and a host-side CLI self-check. ChangesConnector runtime parity validation
Sequence DiagramssequenceDiagram
participant WorkerBin as worker bin.ts
participant CLI as cli index.ts
participant ConnectorCmd as connector.ts
participant SelfCheckMod as self-check module
WorkerBin->>WorkerBin: Check argv for 'self-check'
WorkerBin->>SelfCheckMod: runConnectorRuntimeSelfCheck({ surface: 'worker' })
SelfCheckMod-->>WorkerBin: SelfCheckResult
WorkerBin->>WorkerBin: Output JSON or formatted result
CLI->>CLI: Parse 'connector runtime-self-check'
CLI->>ConnectorCmd: connectorRuntimeSelfCheckCommand({ json })
ConnectorCmd->>SelfCheckMod: runConnectorRuntimeSelfCheck({ surface: 'cli' })
SelfCheckMod-->>ConnectorCmd: SelfCheckResult
ConnectorCmd->>ConnectorCmd: Output JSON or formatted result
sequenceDiagram
participant Event as Workflow Trigger
participant Filter as Filter Job
participant Smoke as Smoke Job
participant BuildWorker as build-worker Job
participant BuildApp as build-app Job
Event->>Filter: Check for relevant path changes
Filter-->>Smoke: run=true or run=false
Smoke->>Smoke: If enabled: build worker image + self-check
Smoke-->>BuildWorker: Parity check result
BuildWorker->>BuildWorker: Waits for smoke job
BuildWorker-->>BuildApp: Worker image ready
BuildApp->>BuildApp: Build app image (depends on worker)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+546
to
+581
| runs-on: ubuntu-latest | ||
| outputs: | ||
| run: ${{ steps.filter.outputs.run }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Decide whether to run the connector parity smoke | ||
| id: filter | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| run: | | ||
| set -euo pipefail | ||
| if [ -z "${BASE_SHA:-}" ]; then | ||
| echo "run=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| changed=$(git diff --name-only "$BASE_SHA"...HEAD) | ||
| if echo "$changed" | grep -E '^(docker/|packages/connector-worker/|packages/connector-sdk/|packages/connectors/|packages/core/|packages/embeddings/|packages/cli/|package\.json$|bun\.lock$|\.github/workflows/ci\.yml$|\.github/workflows/build-images\.yml$)' >/dev/null; then | ||
| echo "run=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "run=false" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::No paths affecting the connector runtime changed — skipping connector-parity-smoke." | ||
| fi | ||
|
|
||
| # Connector-runtime parity smoke gate. Guards against the packaging/parity | ||
| # drift that broke prod: the worker image shipped missing `COPY packages/core`, | ||
| # so `@lobu/connector-sdk`'s transitive `@lobu/core` import dangled and every | ||
| # feed sync crashed at runtime — yet CI was green because the workspace tests | ||
| # resolve `@lobu/core` and build-images.yml builds+pushes images but never RUNS | ||
| # them. This job RUNS the built artifact's self-check on BOTH surfaces (the | ||
| # worker image and the host CLI), which both call the SAME | ||
| # runConnectorRuntimeSelfCheck() over the same compile + default | ||
| # SubprocessExecutor path. | ||
| connector-parity-smoke: |
Comment on lines
+582
to
+640
| needs: connector-parity-smoke-filter | ||
| if: needs.connector-parity-smoke-filter.outputs.run == 'true' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 25 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| # Submodule needed so `bun install` keeps packages/owletto in the lockfile | ||
| # graph (a bare install without it can prune the entry). The CLI self-check | ||
| # itself doesn't need the web UI; the worker image build below doesn't use | ||
| # the submodule at all. | ||
| - uses: ./.github/actions/setup-submodule | ||
| with: | ||
| deploy-key: ${{ secrets.OWLETTO_WEB_DEPLOY_KEY }} | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| # Build the worker image for amd64 only (no QEMU/arm64 — this is a smoke | ||
| # gate, not a release) and load it into the local daemon instead of | ||
| # pushing. Reuse the GHA buildx cache shared with build-images.yml so this | ||
| # leg doesn't pay a cold worker build. | ||
| - name: Build worker image (amd64, load, no push) | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: . | ||
| platforms: linux/amd64 | ||
| file: ./docker/worker/Dockerfile | ||
| load: true | ||
| push: false | ||
| tags: lobu-worker:parity-smoke | ||
| cache-from: type=gha,scope=worker | ||
| cache-to: type=gha,mode=max,scope=worker | ||
|
|
||
| # The real prod runtime contract: run the built image's self-check with NO | ||
| # network, so a green result can't depend on reaching anything external. | ||
| - name: Worker image self-check (docker run --network=none) | ||
| run: | | ||
| docker run --rm --network=none lobu-worker:parity-smoke \ | ||
| bun src/bin.ts self-check --json | ||
|
|
||
| # Host (CLI) leg: same self-check function, exercised from the built CLI | ||
| # runtime. This PR touches the gated paths, so this leg dogfoods the gate. | ||
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: 1.3.5 | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "22" | ||
|
|
||
| - name: Install dependencies | ||
| run: bun install | ||
|
|
||
| - name: Build workspace packages (CLI + deps) | ||
| run: make build-packages | ||
|
|
||
| - name: CLI runtime self-check (host) | ||
| run: node packages/cli/bin/lobu.js connector runtime-self-check --json |
|
Codecov Report❌ Patch coverage is
📢 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: 5
🤖 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-images.yml:
- Around line 47-55: The workflow uses floating tags for third-party actions and
leaves checkout credentials persistent; update the three action references
(actions/checkout@v4, docker/setup-buildx-action@v3,
docker/build-push-action@v6) to fixed commit SHAs and add persist-credentials:
false under the actions/checkout step to disable credential persistence; ensure
you replace each `@v`* tag with the corresponding commit SHA from the action's
repo and keep the same inputs otherwise.
In @.github/workflows/ci.yml:
- Around line 545-586: Add explicit least-privilege GitHub Actions permissions
for the connector parity jobs: modify the connector-parity-smoke-filter and
connector-parity-smoke job definitions (and the other parity job instance
referenced in the comment) to include a permissions block granting at minimum
contents: read (and no broader token scopes). Place the permissions entry under
each job key so the jobs do not inherit default token permissions.
- Around line 550-552: The checkout steps use unpinned action refs and leave
credentials persisted; update each actions/checkout usage (currently "uses:
actions/checkout@v4" with "fetch-depth: 0") to SHA-pin the action ref (use the
specific commit SHA instead of `@v4`) and add "persist-credentials: false" under
the checkout step; apply the same changes to the other occurrences noted (lines
referencing the same checkout step at the other job locations) so all parity CI
jobs are SHA-pinned and do not persist credentials.
In `@packages/connector-worker/src/self-check/index.ts`:
- Around line 200-213: The discovery code currently only accepts .ts files in
the two places where extname(entry.name) and extname(s.name) are checked; update
those filters to also allow compiled connector outputs (e.g., .js, .mjs, and
optionally .cjs) while still excluding type declaration files and maps (e.g.,
.d.ts, .map). Modify the checks in the branches that push into paths (the ones
referencing entry.name and s.name, and the paths array populated after
readdir/resolve) so they accept the compiled extensions when scanning dist-style
directories produced by builds.
- Around line 261-264: The dynamic runtime import in withCwdTempFile (callsite:
await import(pathToFileURL(tmpFile).href) in
packages/connector-worker/src/self-check/index.ts) lacks a measured-cost
justification in CLAUDE.md; add an entry to CLAUDE.md that documents the
measured-cost rationale for this specific dynamic-import pattern (reference the
same pattern/matching rationale used by child-runner.ts), include evidence of
controlled input (tmpFile is produced by withCwdTempFile),
security/attack-surface assessment, performance cost measurements or estimates,
and an explicit statement allowing this dynamic import for the
self-check/runtime-compiled bundle scenario so reviewers can validate it against
the dynamic-import allow-list.
🪄 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: 49bddf4f-8bcf-4e77-ab8d-ed5f4a073ec1
📒 Files selected for processing (7)
.github/workflows/build-images.yml.github/workflows/ci.ymlpackages/cli/src/commands/connector.tspackages/cli/src/index.tspackages/connector-worker/package.jsonpackages/connector-worker/src/bin.tspackages/connector-worker/src/self-check/index.ts
Comment on lines
+47
to
+55
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Build worker image (amd64, load, no push) | ||
| uses: docker/build-push-action@v6 | ||
| with: |
There was a problem hiding this comment.
Pin newly added Actions and disable checkout credential persistence.
Line 48, Line 51, and Line 54 use tag refs (@v*) instead of commit SHAs, and Line 48 leaves persist-credentials at default. This weakens CI supply-chain posture in the new gate job.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 47-48: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 51-51: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 54-54: 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-images.yml around lines 47 - 55, The workflow uses
floating tags for third-party actions and leaves checkout credentials
persistent; update the three action references (actions/checkout@v4,
docker/setup-buildx-action@v3, docker/build-push-action@v6) to fixed commit SHAs
and add persist-credentials: false under the actions/checkout step to disable
credential persistence; ensure you replace each `@v`* tag with the corresponding
commit SHA from the action's repo and keep the same inputs otherwise.
Comment on lines
+545
to
+586
| connector-parity-smoke-filter: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| run: ${{ steps.filter.outputs.run }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Decide whether to run the connector parity smoke | ||
| id: filter | ||
| env: | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| run: | | ||
| set -euo pipefail | ||
| if [ -z "${BASE_SHA:-}" ]; then | ||
| echo "run=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| changed=$(git diff --name-only "$BASE_SHA"...HEAD) | ||
| if echo "$changed" | grep -E '^(docker/|packages/connector-worker/|packages/connector-sdk/|packages/connectors/|packages/core/|packages/embeddings/|packages/cli/|package\.json$|bun\.lock$|\.github/workflows/ci\.yml$|\.github/workflows/build-images\.yml$)' >/dev/null; then | ||
| echo "run=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "run=false" >> "$GITHUB_OUTPUT" | ||
| echo "::notice::No paths affecting the connector runtime changed — skipping connector-parity-smoke." | ||
| fi | ||
|
|
||
| # Connector-runtime parity smoke gate. Guards against the packaging/parity | ||
| # drift that broke prod: the worker image shipped missing `COPY packages/core`, | ||
| # so `@lobu/connector-sdk`'s transitive `@lobu/core` import dangled and every | ||
| # feed sync crashed at runtime — yet CI was green because the workspace tests | ||
| # resolve `@lobu/core` and build-images.yml builds+pushes images but never RUNS | ||
| # them. This job RUNS the built artifact's self-check on BOTH surfaces (the | ||
| # worker image and the host CLI), which both call the SAME | ||
| # runConnectorRuntimeSelfCheck() over the same compile + default | ||
| # SubprocessExecutor path. | ||
| connector-parity-smoke: | ||
| needs: connector-parity-smoke-filter | ||
| if: needs.connector-parity-smoke-filter.outputs.run == 'true' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 25 | ||
| steps: |
There was a problem hiding this comment.
Set explicit minimal permissions for the new parity jobs.
The new connector-parity-smoke-filter and connector-parity-smoke jobs inherit default token permissions. Please set explicit least-privilege permissions (at minimum contents: read) for these jobs.
Also applies to: 582-640
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 546-581: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🪛 zizmor (1.25.2)
[warning] 550-552: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 545-580: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 550-550: 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/ci.yml around lines 545 - 586, Add explicit
least-privilege GitHub Actions permissions for the connector parity jobs: modify
the connector-parity-smoke-filter and connector-parity-smoke job definitions
(and the other parity job instance referenced in the comment) to include a
permissions block granting at minimum contents: read (and no broader token
scopes). Place the permissions entry under each job key so the jobs do not
inherit default token permissions.
Comment on lines
+550
to
+552
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Harden new CI job steps: SHA-pin actions and set persist-credentials: false on checkout.
The newly added parity jobs introduce unpinned third-party action refs and checkout steps with credential persistence enabled. Tightening both is a low-cost hardening win.
Also applies to: 587-587, 598-599, 605-606, 625-626, 629-630
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 550-552: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 550-550: 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/ci.yml around lines 550 - 552, The checkout steps use
unpinned action refs and leave credentials persisted; update each
actions/checkout usage (currently "uses: actions/checkout@v4" with "fetch-depth:
0") to SHA-pin the action ref (use the specific commit SHA instead of `@v4`) and
add "persist-credentials: false" under the checkout step; apply the same changes
to the other occurrences noted (lines referencing the same checkout step at the
other job locations) so all parity CI jobs are SHA-pinned and do not persist
credentials.
Comment on lines
+200
to
+213
| if (extname(entry.name) !== '.ts' || entry.name.endsWith('.d.ts')) | ||
| continue; | ||
| paths.push(entryPath); | ||
| continue; | ||
| } | ||
| if (entry.isDirectory()) { | ||
| if (entry.name === '__tests__' || entry.name.startsWith('_')) continue; | ||
| try { | ||
| const sub = await readdir(entryPath, { withFileTypes: true }); | ||
| for (const s of sub.sort((a, b) => a.name.localeCompare(b.name))) { | ||
| if (!s.isFile()) continue; | ||
| if (extname(s.name) !== '.ts' || s.name.endsWith('.d.ts')) continue; | ||
| paths.push(resolve(entryPath, s.name)); | ||
| } |
There was a problem hiding this comment.
Connector discovery currently ignores built JS connectors.
Line 200 and Line 211 only accept .ts, but your candidate roots include dist-style directories. In packaged/built layouts, connector files are typically .js/.mjs, so this can incorrectly fail connectors-instantiate.
Suggested fix
- if (extname(entry.name) !== '.ts' || entry.name.endsWith('.d.ts'))
+ const topExt = extname(entry.name);
+ if (!['.ts', '.js', '.mjs', '.cjs'].includes(topExt) || entry.name.endsWith('.d.ts'))
continue;
@@
- if (extname(s.name) !== '.ts' || s.name.endsWith('.d.ts')) continue;
+ const subExt = extname(s.name);
+ if (!['.ts', '.js', '.mjs', '.cjs'].includes(subExt) || s.name.endsWith('.d.ts')) continue;📝 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 (extname(entry.name) !== '.ts' || entry.name.endsWith('.d.ts')) | |
| continue; | |
| paths.push(entryPath); | |
| continue; | |
| } | |
| if (entry.isDirectory()) { | |
| if (entry.name === '__tests__' || entry.name.startsWith('_')) continue; | |
| try { | |
| const sub = await readdir(entryPath, { withFileTypes: true }); | |
| for (const s of sub.sort((a, b) => a.name.localeCompare(b.name))) { | |
| if (!s.isFile()) continue; | |
| if (extname(s.name) !== '.ts' || s.name.endsWith('.d.ts')) continue; | |
| paths.push(resolve(entryPath, s.name)); | |
| } | |
| const topExt = extname(entry.name); | |
| if (!['.ts', '.js', '.mjs', '.cjs'].includes(topExt) || entry.name.endsWith('.d.ts')) | |
| continue; | |
| paths.push(entryPath); | |
| continue; | |
| } | |
| if (entry.isDirectory()) { | |
| if (entry.name === '__tests__' || entry.name.startsWith('_')) continue; | |
| try { | |
| const sub = await readdir(entryPath, { withFileTypes: true }); | |
| for (const s of sub.sort((a, b) => a.name.localeCompare(b.name))) { | |
| if (!s.isFile()) continue; | |
| const subExt = extname(s.name); | |
| if (!['.ts', '.js', '.mjs', '.cjs'].includes(subExt) || s.name.endsWith('.d.ts')) continue; | |
| paths.push(resolve(entryPath, s.name)); | |
| } |
🤖 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/self-check/index.ts` around lines 200 - 213,
The discovery code currently only accepts .ts files in the two places where
extname(entry.name) and extname(s.name) are checked; update those filters to
also allow compiled connector outputs (e.g., .js, .mjs, and optionally .cjs)
while still excluding type declaration files and maps (e.g., .d.ts, .map).
Modify the checks in the branches that push into paths (the ones referencing
entry.name and s.name, and the paths array populated after readdir/resolve) so
they accept the compiled extensions when scanning dist-style directories
produced by builds.
connector-worker has no direct @lobu/core dependency — it's consumed transitively by @lobu/connector-sdk (logger/retry). The self-check resolved @lobu/core from connector-worker's own location, which resolves in the hoisted dev workspace but fails in the isolated-linker worker image: a false negative (caught by the gate's own docker leg on #1037). Resolve/import @lobu/core anchored at @lobu/connector-sdk (its real consumer), reproducing the exact prod edge that dangled in #1035: .../connector-sdk/node_modules/@lobu/core.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/self-check/index.ts`:
- Around line 48-50: There is a duplicate import of pathToFileURL from
'node:url' causing a module/typecheck error; update the import statements in
packages/connector-worker/src/self-check/index.ts by removing the redundant
import and consolidating the node:url imports so that fileURLToPath and
pathToFileURL are imported together (keeping extname, join, resolve imports from
'node:path' unchanged), i.e., ensure only one import line brings in
pathToFileURL and fileURLToPath.
🪄 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: c9a9d0fc-24c2-4294-9db2-968596c903a2
📒 Files selected for processing (1)
packages/connector-worker/src/self-check/index.ts
The previous commit added a pathToFileURL import that already existed (line 50), tripping TS2300 in make build-packages. pathToFileURL is already imported; remove the duplicate.
Collapse the four hand-written async try/catch IIFEs into one async check() helper (records returned detail or error); fold connector discovery and key-uniqueness into the same pattern; trim duplicated narration while keeping the non-obvious rationale (SDK-anchored core resolution, cwd temp file, dynamic-import probes). No behavior change: 534 -> 413 lines, all 11 checks still pass and the dangling-@lobu/core failure path still exits 1.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/self-check/index.ts`:
- Around line 167-175: The current try/catch around readdir in the connector
scan swallows errors so unreadable connector subdirectories (readdir failures)
are silently skipped; update the block in the self-check scan that calls
readdir(entryPath, { withFileTypes: true }) so that any error is propagated
(throw) or logged and causes the scan to fail instead of being ignored: remove
the empty catch, or rethrow the caught error, and ensure the caller of this loop
(or the function performing the scan) handles the thrown error so that
unreadable directories are reported as a failure of the connector discovery
check; keep using isConnectorFile, resolve and paths as-is when iterating
successful directory entries.
- Around line 297-340: The self-check currently resolves EXTERNAL_RUNTIME_DEPS
using createRequire(import.meta.url) (see require_ and the loop over
EXTERNAL_RUNTIME_DEPS) which anchors resolution to the self-check module
location; instead resolve externals the same way the child process will by
creating a require anchored at the cwd-staged tmp module used by
executor/child-runner.ts (i.e., build a staged module URL/path using
process.cwd() like the child-runner does and call
createRequire(stagedModuleUrl).resolve(dep) for each dep) so the check uses the
same resolution root as the forked importer.
🪄 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: 1e005fe9-6434-4467-9cd1-80479ead4693
📒 Files selected for processing (1)
packages/connector-worker/src/self-check/index.ts
…e connector subdir Address CodeRabbit review: - Resolve EXTERNAL_RUNTIME_DEPS from a cwd-rooted require, matching how child-runner stages and resolves the compiled bundle, so the probe fails where a real connector would instead of off connector-worker's hoisted tree. - Propagate readdir errors on connector subdirs (an unreadable subdir is a packaging defect for a parity gate) instead of silently skipping. - Document the self-check + child-runner runtime-compiled-bundle imports in the AGENTS.md dynamic-import allow-list. - Add least-privilege contents:read to the connector-parity-smoke job.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/connector-worker/src/self-check/index.ts (1)
154-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDiscover compiled connector artifacts too.
firstExistingDir()can select.../connectors/distor a packagedconnectorsdirectory, butisConnectorFile()still only accepts.ts. In those layouts the scan will miss.js/.mjs/.cjsconnectors and failconnectors-instantiateeven when the bundled artifacts are present.Suggested fix
async function collectConnectorSourceFiles(dirPath: string): Promise<string[]> { const paths: string[] = []; const isConnectorFile = (name: string) => - extname(name) === ".ts" && !name.endsWith(".d.ts"); + [".ts", ".js", ".mjs", ".cjs"].includes(extname(name)) && + !name.endsWith(".d.ts") && + !name.endsWith(".map"); for (const entry of await readdir(dirPath, { withFileTypes: 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/connector-worker/src/self-check/index.ts` around lines 154 - 177, collectConnectorSourceFiles currently only treats files with extname ".ts" as connector files (isConnectorFile), so when firstExistingDir selects a compiled layout (e.g., connectors/dist or packaged connectors) .js/.mjs/.cjs artifacts are ignored and the scan misses bundled connectors; update isConnectorFile inside collectConnectorSourceFiles to accept compiled extensions (e.g., ".ts", ".js", ".mjs", ".cjs") while still excluding TypeScript declaration files (names ending with ".d.ts"), so both the top-level file checks and the nested readdir loop use the new predicate and return the resolved paths as before.
🤖 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/connector-worker/src/self-check/index.ts`:
- Around line 154-177: collectConnectorSourceFiles currently only treats files
with extname ".ts" as connector files (isConnectorFile), so when
firstExistingDir selects a compiled layout (e.g., connectors/dist or packaged
connectors) .js/.mjs/.cjs artifacts are ignored and the scan misses bundled
connectors; update isConnectorFile inside collectConnectorSourceFiles to accept
compiled extensions (e.g., ".ts", ".js", ".mjs", ".cjs") while still excluding
TypeScript declaration files (names ending with ".d.ts"), so both the top-level
file checks and the nested readdir loop use the new predicate and return the
resolved paths as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 113b286c-7507-439a-9993-dbff4541154b
📒 Files selected for processing (3)
.github/workflows/build-images.ymlAGENTS.mdpackages/connector-worker/src/self-check/index.ts
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
Member
Author
|
Addressed the CodeRabbit review in Fixed (valid findings):
Intentionally skipped (with reasoning):
Validation (Node 22): per-package |
Member
Author
|
bug_free 88, simplicity 84, slop 0, bugs 0, 0 blockers Typecheck/unit passed. [env] Integration failed because DATABASE_URL points at database "postgres" and the harness refused destructive setup; failing paths are untouched server tests. Explored CLI self-check and worker self-check from package cwd; both returned ok with 31 connectors. Full verdict JSON{
"bug_free_confidence": 88,
"bugs": 0,
"slop": 0,
"simplicity": 84,
"blockers": [],
"change_type": "test",
"behavior_change_risk": "low",
"tests_adequate": true,
"suggested_fixes": [],
"notes": "Typecheck/unit passed. [env] Integration failed because DATABASE_URL points at database \"postgres\" and the harness refused destructive setup; failing paths are untouched server tests. Explored CLI self-check and worker self-check from package cwd; both returned ok with 31 connectors.",
"categories": {
"src": 487,
"tests": 0,
"docs": 2,
"config": 0,
"deps": 4,
"migrations": 0,
"ci": 155,
"generated": 0
}
}Local review gate — branch protection can require the |
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
3 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.
Incident this prevents
The worker Docker image (
docker/worker/Dockerfile) shipped to prod missingCOPY packages/core, so@lobu/connector-sdk's transitive@lobu/coreimport dangled and every connector feed sync crashed withENOENT reading .../connector-sdk/node_modules/@lobu/core. All CI was green because:ci.ymltests the monorepo workspace, where@lobu/coreresolves fine, andbuild-images.ymlbuilds + pushes images but never runs them.Fixed by a one-line COPY in #1035. This PR adds a gate that prevents that whole class of packaging/parity drift by running the built artifact and asserting its resolution/compile/execute graph actually holds.
Design: one shared self-check, two entrypoints (the parity invariant)
A single shared function
runConnectorRuntimeSelfCheck()lives inpackages/connector-worker/src/self-check/. Both the worker image and the built CLI call the same function over the same compile + defaultSubprocessExecutorpath. The only permitted difference between the two surfaces is the set of connector-source discovery roots (genuinely environment-specific: monorepopackages/connectors/srcvs the worker image layout vs an npm-installed CLI's bundleddist/connectors).packages/connector-worker/src/bin.tsgains aself-checksubcommand (alongsidedaemon). Worker image runsbun src/bin.ts self-check --json.lobu connector runtime-self-checksubcommand that statically imports the same function via a new./self-checkexport on@lobu/connector-worker.What the self-check asserts (artifact/packaging facts ONLY)
@lobu/connector-sdkresolves and imports.@lobu/coreresolves and imports through the same runtime resolver — the exact dangling edge that broke prod.EXTERNAL_RUNTIME_DEPSpackage (playwright/patchright,sharp,jimp) is resolvable.key/name/version, and keys are unique (31 connectors today).compileConnectorFromFilepath and executes throughexecuteCompiledConnectorusing the defaultSubprocessExecutor(no custom executor) — proving compile + fork + child-runner + SDK-externalized runtime resolution all line up.What it explicitly does NOT do
No network, DB, gateway, or OAuth. It asserts no event content and no checkpoint semantics, and re-runs no business logic. It passes under
docker run --network=none.CI wiring
PR gate (
.github/workflows/ci.yml) — path-filtered job modeled onmac-build-smoke+ its filter job (paths:docker/**,packages/{connector-worker,connector-sdk,connectors,core,embeddings,cli}/**, rootpackage.json,bun.lock, and the two workflow files). The job builds the worker imagelinux/amd64only (load: true,push: false, GHA buildx cachescope=worker), runsdocker run --rm --network=none <img> bun src/bin.ts self-check --json, then runs the CLI self-check on the host (make build-packages→node packages/cli/bin/lobu.js connector runtime-self-check --json).Main/deploy gate (
.github/workflows/build-images.yml) — the same amd64 image self-check as aconnector-parity-smokejob thatbuild-workerdepends on. Sincebuild-appalreadyneeds: build-worker, a failing smoke blocks both the worker and app pushes. This matters because the chart applies one shared tag to app + worker and Flux's ImageUpdateAutomation would otherwise roll the app to a tag whose worker is broken. amd64-only, and it shares the buildx cache scope withbuild-workerso only the arm64 leg is new work at push time.Validation
make build-packages(Node 22) — green.bun src/bin.ts self-check— PASS, exit 0 (same 11 checks,surface: worker).packages/core/distaside reproduces the prod bug — the self-check exits 1 withimport:@lobu/connector-sdk,resolve:@lobu/core,import:@lobu/core,connectors-instantiate, andsynthetic-connector-subprocess-executeall failing on the dangling@lobu/core. Restored.tsc --noEmitforconnector-workerandcli— clean. Pre-commit biome + root tsc — clean.Docker is not available on this host, so the image leg was not run locally. That leg is validated by the new CI job itself — good dogfooding, since this PR touches the gated paths and the gate will run on this very PR.
Summary by CodeRabbit
Chores
New Features
Documentation