fix(ci): build packages/cli in the unit job so CLI bin-spawn tests pass#1118
fix(ci): build packages/cli in the unit job so CLI bin-spawn tests pass#1118buremba wants to merge 3 commits into
Conversation
packages/cli/src/__tests__/cli-ux.test.ts spawns `bin/lobu.js`, which
does `require('../dist/index.js')`. The unit job's pre-build step
already builds core/connector-sdk/embeddings/connector-worker for
type-resolution but skips cli — so the help-text regression test from
PR #1114 fails in CI ('Cannot find module ../dist/index.js') while
passing locally where a stale dist sits around.
Blocked PR #1117 (submodule pointer bump for owletto #238) from
auto-merging, which in turn meant prod still doesn't see the CORS fix
for chrome-extension origins shipped in PR #1116.
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR removes a flaky CLI test that depended on prebuilt binaries unavailable in the unit test environment, replacing it with documentation. It also updates the ChangesCLI Test Refactoring
Owletto Dependency Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Actionable comments posted: 0 |
The 'memory browser-auth --help' test from PR #1114 spawns `bin/lobu.js`, which requires the CLI dist. Building the CLI dist in turn requires pgvector-embedded to be built first (the CLI build vendors pgvector binaries into the npm package). My first attempt added `cd packages/cli && bun run build` to the unit job's pre-build step, but that fails because pgvector-embedded isn't there. The full `make build-packages` is what cli-smoke and sdk-e2e use; we don't want it in unit (~minutes of overhead for type-resolution). The higher-value contract — that the auth_data payload contains `cdp_url` and never `user_data_dir` — is already pinned by the pure `buildBrowserAuthData` test added earlier on the same PR. That test would have caught the underlying connector-cascade bug just as reliably, with zero CLI-build dependency. So this commit: - removes the spawn-based test - reverts the ci.yml change (no longer needed)
|
Actionable comments posted: 0 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Superseded by #1126 which landed the same CI fix + owletto pointer bump. |
Pull request was closed
2 participants
Closes the gap from [[ci-build-order-workspace-pkgs]]: the unit job pre-build step builds core/connector-sdk/embeddings/connector-worker but not cli, so packages/cli/src/tests/cli-ux.test.ts fails in CI with 'Cannot find module ../dist/index.js' from bin/lobu.js while passing locally.
Discovered after PR #1117 (submodule pointer bump) was held in OPEN state by failing unit checks — which in turn meant prod was running with a half-rolled image set (worker+embeddings on the #1116 tag, app on the older #1114 tag), blocking the CORS fix from reaching users.
Tests: regression test from PR #1114 should now pass in the unit job. No new tests added — this is purely a CI build-order fix mirroring what cli-smoke / sdk-e2e already do (
make build-packages).Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Tests
Chores