Conversation
The TypeScript build exists to serve type-aware lint and `tsc --noEmit`. Nothing else reads it: every workspace declares a `bun` export condition resolving to source, packages/cli/vitest.config.ts aliases cross-workspace imports at source, and the published CLI ships raw TypeScript plus the separate publish-time bundle from #2999/#3013. Remove the build from four of six test shard legs and give the retained lint/typecheck build a declaration-only mode. Emit mode - scripts/build_package.ts appends --emitDeclarationOnly to `tsc --build` when LLXPRT_EMIT_DECLARATIONS_ONLY=1, and guards top-level execution behind import.meta.main so the mode helpers are testable. - Root `build:types` sets that variable; `build` and `build:packages` are untouched, so the release path keeps full JavaScript emit for the workspaces that publish `main: dist/index.js`. - scripts/build.ts skips the VS Code companion in declaration-only mode: its esbuild step resolves workspace deps at `dist/*.js`, and it is built on its own track by `npm run build:vscode`. - scripts/chmod_executable.ts treats an absent target as a no-op only in declaration-only mode, and still fails loudly otherwise. Consumers of compiled output that the removal surfaced - scripts/start.ts imported parseBootstrapArgs from packages/cli/dist/src/config/profileBootstrap.js and discarded the result. The call was added by the JS-to-TS script migration and duplicates what the spawned CLI already does; drop it. - Three CLI integration files spawned `node packages/cli/dist/index.js`. Their own comments already described the child as booting from TypeScript source; spawn packages/cli/index.ts with the Bun binary running the suite instead. - packages/auth's package-boundary test asserted dist/index.js and dist/index.d.ts exist on disk, which only held because the shard built first. The published contract stays pinned by the main/types/exports assertions. CI - lint_javascript builds declarations only. - test_shard builds only on the `agents` and `scripts` legs, which run the agents API-surface guard. That guard resolves telemetry, mcp, and several storage subpaths through node_modules to `dist/*.d.ts` because packages/agents/tsconfig.json has no source mapping for them; repointing those is #2618. - Those legs need the FULL build. Bun applies tsconfig `paths` at runtime, so a declaration-only dist makes core's mcp mapping resolve to a `.d.ts` with no JavaScript behind it: 178 of 340 agents test files then die at import. A complete dist or no dist both work; a partial one does not. - The per-shard agents API-surface guard step is removed as redundant with lint_javascript and the agents package pretest hook. - The stale comment citing "storage, settings" now names the real dependents: cli to tools, core to mcp, a2a-server to settings/storage/tools. Refs #2983
Review remediation for #2983. - scripts/build.ts: skip packages/lsp in declaration-only mode. Its build is a bare `tsc -p tsconfig.json` that never sees --emitDeclarationOnly, so it emitted JavaScript into a build whose purpose is not to. Nothing imports the package or maps it in a tsconfig — it is reached by module resolution from a spawned process — so no declarations are lost. Guard the script's top-level execution behind import.meta.main so the selector is importable. A clean `npm run build:types` now leaves 2771 declarations and zero compiler-emitted JavaScript. - scripts/tests/issue-2983-declaration-build.test.ts: add an end-to-end run of the real build_package.ts against a throwaway workspace laid out like a real one, asserting the emitted file set in both modes. This is the coverage that replaces the auth build-artifact assertions: it proves a full build writes index.js, index.d.ts, staged assets and .last_build, and that a declaration build writes no .js or .js.map. Also pin the lsp skip as safe by asserting no tsconfig references the package. - Correct the claim that nothing consumes packages/*/dist/*.js. Published library workspaces do, through main/import, whenever a consumer resolves without the `bun` export condition. Only the PR path does not. Refs #2983
Second review pass for #2983. - packages/cli/src/integration-tests: the spawned-CLI helpers listened for 'close' but not 'error'. A spawn failure (a moved or deleted CLI entry) emits only 'error', so the promise never settled and the case hung until the runner's own timeout with no cause reported. Settle with exit code -1 and the spawn message on stderr, matching the existing timeout path. - scripts/build.ts: name the workspace when reading or parsing its package.json fails. A bare SyntaxError left the reader to guess which of sixteen manifests was malformed. - scripts/tests/issue-2983-declaration-build.test.ts: guard the two indexOf-based lookups. A renamed ci.yml marker returned -1, which sliced most of the file and still contained every expected phrase, so the test would have passed on stale content; and an empty build-step list produced a misleading assertion message instead of naming the real problem. Refs #2983
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds declaration-only build support, narrows full builds in CI, updates CLI integration tests to execute TypeScript entry points, and adds coverage for build behavior, workspace selection, executable handling, and workflow wiring. ChangesDeclaration-only build pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
WalkthroughThis PR changes 12 file(s).
Changes
Magnitude🎯 3 (L) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
github-actions
Bot
added
the
maintainer:e2e:ok
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
OpenCodeReview — PR #3102
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
PR review remediation for #2983. - scripts/build.ts: the workspace-name pattern rejected uppercase. It exists to keep names shell-safe where they are interpolated into the npm command line, not to enforce npm naming policy — and npm still resolves legacy names containing uppercase, so the stricter rule could have thrown on a valid workspace. Accept uppercase in both scope and name segments and say in the comment what the pattern is for. - scripts/tests/issue-2983-declaration-build.test.ts: pin the entry-point contract that the import.meta.main guards introduced. The case spawns build_package.ts from outside packages/ and asserts it exits 1 with "must be invoked from a package directory" — the first statement inside the guarded main(), so reaching it proves entry-point execution survived. Nothing in the repo imports either build script for side effects; the only importer is this test, and it takes the workspace selector alone. Refs #2983
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/tests/issue-2983-declaration-build.test.ts (1)
445-476: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated build-step filter.
Three tests repeat the same filter over
test_shardsteps at Lines 446, 453, and 470. A single helper removes the repetition and keeps the matcher string in one place.♻️ Proposed helper
function shardBuildSteps(): WorkflowStep[] { return jobSteps(ciJobs()['test_shard']).filter((step) => String(step.run ?? '').includes('npm run build'), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/issue-2983-declaration-build.test.ts` around lines 445 - 476, Extract the repeated build-step filtering logic into a helper such as shardBuildSteps(), using the existing test_shard workflow and “npm run build” matcher. Replace the inline filters in all three tests with calls to that helper while preserving their existing assertions.
🤖 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/cli/src/integration-tests/cli-args-test-helpers.ts`:
- Around line 135-146: Update the spawn-error lifecycle comments in the error
handlers of both packages/cli/src/integration-tests/cli-args-test-helpers.ts
(lines 135-146) and
packages/cli/src/integration-tests/loadbalancer.integration.test.ts (lines
96-107): state that a spawn failure emits error and later close, while the error
handler preserves the diagnostic and exitCode -1 and the later close handler
cannot alter the already-settled promise. No behavioral code change is needed.
In `@scripts/build.ts`:
- Around line 99-106: Update workspaceBuildSelector to detect when
declarationBuildWorkspaces(readWorkspacePackageNames()) produces no workspaces
and throw an explicit error before mapping or returning the selector. Preserve
the existing non-declarations path and normal workspace flag generation for
non-empty results.
---
Nitpick comments:
In `@scripts/tests/issue-2983-declaration-build.test.ts`:
- Around line 445-476: Extract the repeated build-step filtering logic into a
helper such as shardBuildSteps(), using the existing test_shard workflow and
“npm run build” matcher. Replace the inline filters in all three tests with
calls to that helper while preserving their existing assertions.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 099267af-0e86-48b9-a3d2-3ec96cd82612
⛔ Files ignored due to path filters (1)
project-plans/issue2983/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (11)
.github/workflows/ci.ymlpackage.jsonpackages/auth/src/__tests__/package-boundary.test.tspackages/cli/src/integration-tests/cli-args-test-helpers.tspackages/cli/src/integration-tests/loadbalancer.integration.test.tsscripts/build.tsscripts/build_package.tsscripts/chmod_executable.tsscripts/start.tsscripts/tests/issue-2983-declaration-build.test.tstsconfig.scripts.json
💤 Files with no reviewable changes (1)
- scripts/start.ts
Third review pass for #2983. - scripts/build.ts: throw when the declaration build selects no workspaces. An empty selector left a bare `npm run build`, which re-enters this script instead of fanning out — the declaration build would report success while building nothing it intended. Unreachable today, reachable the moment the exclusion set grows, and silent either way. - scripts/tests/issue-2983-declaration-build.test.ts: add the parallel entry-point case for build.ts. It has no cwd guard — it anchors on import.meta.url — so spawning it from a temp directory would run a real build. Instead the case empties PATH, which makes its first guarded action (`npm run generate`) fail immediately; reaching that failure proves main() ran while doing no work. Also pin the empty-selector guard. - packages/cli/src/integration-tests: correct the spawn-failure comments. A failed spawn emits 'error' and then 'close' with a null exit code; the earlier text claimed 'close' never fires. The behaviour was already right (the first settle wins) but the stated mechanism was not. Refs #2983
Scope note: one acceptance criterion is intentionally not fully metThe issue asks that Why those two legs. Both run the agents API-surface guard — Why the guard needs it. Its docblock claims it is clean-CI safe, but that is not true today. Its temp tsconfig extends Why it is not a small fix. The failing subpaths are not expressible as wildcard Each needs an explicit per-subpath mapping across four packages. That is repointing cross-workspace mappings at source, which the issue lists under Out of scope and which #2618 owns. It also carries real risk: the guard compares against a checked-in API-surface snapshot, so changing how it resolves types can shift the parsed surface. Second constraint, discovered while implementing. Those two legs must run the FULL build, not What was delivered. Four of six shard legs stop building, the retained lint/typecheck build stops transpiling, and four genuine |
1 participant
TLDR
The TypeScript build exists to serve type-aware lint and
tsc --noEmit. This PR confines it to that job.test_shardlegs stop building. Tests never read compiled output: every workspace declares abunexport condition resolving to TypeScript source, andpackages/cli/vitest.config.tsaliases cross-workspace imports at source.npm run build:typesrunstsc --build --emitDeclarationOnly, producing 2771.d.tsfiles and zero compiler-emitted JavaScript.release.ymlstill runsnpm run build:packageswith full emit, because every published library workspace declaresmain: dist/index.jsand shipsdist.Removing the build surfaced four places that quietly depended on compiled output. All four are fixed at the import/spawn site rather than by restoring the build.
Reviewers should look hardest at two things: the decision to keep a build on the
agentsandscriptslegs (explained below, with measurements), and the fact that a partially built workspace is worse than an unbuilt one under Bun.Dive Deeper
What actually consumes
packages/*/distdist?tsc --noEmitcli -> tools,core -> mcp,a2a-server -> settings/storage/toolsmap atdist/*.d.tsmain/importpoint atdist/*.jswhen a consumer resolves without thebunconditionpackages/cli/bundle/llxprt.js(#2999 / #3013), whichprepackbuilds and this PR never touchesHidden consumers the change surfaced
Measured by deleting
packages/*/distand running every shard.scripts/start.tsimportedparseBootstrapArgsfrompackages/cli/dist/src/config/profileBootstrap.jsand threw the result away. The call was added by the JS-to-TS script migration and duplicates what the spawned CLI already does. Removed.node packages/cli/dist/index.js(34 failing cases). Their own comments already described the child as booting from TypeScript source, so they now spawnpackages/cli/index.tswith the Bun binary running the suite — which is what the shipped launcher does. They are also noticeably faster.packages/auth's package-boundary test asserteddist/index.jsanddist/index.d.tsexist on disk. That only held because the shard built first, so it measured the CI harness. Its coverage is replaced by an end-to-end build test (below); the published contract stays pinned by themain/types/exportsassertions in the same file.packages/cli's build chainschmod_executable.ts dist/index.js, which does not exist in declaration-only mode. The script now treats an absent target as a no-op only in that mode and still fails loudly otherwise.Why the
agentsandscriptslegs still buildBoth run the agents API-surface guard —
agentsthrough its packagepretesthook,scriptsthroughscripts/tests/check-agents-api-surface.test.ts. Despite its docblock, that guard needs dependency declarations: its temp tsconfig resolves@vybestack/llxprt-code-{telemetry,mcp}and severalstorage/*subpaths throughnode_modulestodist/*.d.ts, becausepackages/agents/tsconfig.jsonhas no source mapping for them. Those subpaths are not expressible as wildcardpathsentries —@vybestack/llxprt-code-storage/storage/secure-store.jsresolves tosrc/secure-store/secure-store.ts, and@vybestack/llxprt-code-tools/doubleEscapeUtils.jstosrc/formatters/doubleEscapeUtils.ts. Repointing them is #2618, and the issue lists that work under Out of scope.Everything else on those legs is fine without a build: with
distdeleted,bun scripts/test.ts --shard agents --skip-pretestpasses 337/340 files, and the three stragglers pass in isolation (they are load-sensitive 30s timeouts, not resolution failures).The redundant per-shard
Run agents API-surface guardstep is removed:lint_javascriptalready runs it after its own build, and theagentsshard covers it through the packagepretesthook.Why those legs need the FULL build, not
build:typesBun applies tsconfig
pathsat runtime.packages/core/tsconfig.jsonmaps@vybestack/llxprt-code-mcpto../mcp/dist/mcp/index.d.ts, and that mapping wins over the package'sbunexport condition whenever the file exists. With a declaration-onlydist, cross-package imports therefore resolve to a.d.tswhose relative re-export has no JavaScript behind it.Measured: 178 of 340 agents test files die at import with
Cannot find module './src/index.js' from packages/mcp/dist/mcp/index.d.ts, andbun scripts/start.tsfails the same way. With nodist, resolution falls through to thebuncondition and everything passes.A complete
distworks. Nodistworks. A partial one does not.build:typesis therefore confined tolint_javascript, which executes no application code, and bothci.ymland the test suite pin that constraint so nobody moves it.Build plumbing
scripts/build_package.tsappends--emitDeclarationOnlywhenLLXPRT_EMIT_DECLARATIONS_ONLY=1. Build mode accepts the flag, so no parallel tsconfig tree is needed — one pipeline, two emit modes. Only the exact value1enables it, so a stray0cannot change what the release build emits.build:typessets that variable and delegates tobuild.buildandbuild:packagesare untouched.scripts/build.tsskips two workspaces in declaration-only mode, because neither contributes declarations that any tsconfig maps:vscode-ide-companionends its build in esbuild (which resolves workspace deps atdist/*.js), andlspbuilds with a baretsc -pthat never sees the flag. Both still build normally on the release path, andlspis reached at runtime by module resolution from a spawned process, never imported.Reviewer Test Plan
Confirm the emit contract:
Only two files should appear, both example-extension assets copied verbatim by
copy_files.ts(examples/mcp-server/example.js,examples/hooks/scripts/on-start.js). No compiler output. Then checkfind packages/*/dist -name '*.d.ts' | wc -lis in the thousands, and that lint and typecheck still pass against it:Confirm the shards no longer need a build:
Confirm the release path is intact:
And the focused suite:
Note: run the CLI integration files through
bun scripts/test.ts --shard cli, not a single combinedbun test a.ts b.ts c.ts. The CLI runner spawns one process per file and the suite relies on that isolation forLLXPRT_CONFIG_HOME; sharing one process leaks settings between files. That is pre-existing and unrelated to this PR.Testing Matrix
Verified on macOS:
npm run test,npm run lint:ci,npm run lint:eslint-guard,npm run typecheck,npm run format:check,npm run build,npm run build:types,npm run bundle:cli, and the CLI smoke run. Windows and Linux are covered by the CI matrix; the two platform-sensitive spots arechmod_executable.ts(already a no-op on Windows) andbuild:types, which usescross-env.Linked issues / bugs
Closes #2983
Contributes to #2702 (CI execution optimization) and #2578 (all-Bun).
Coordinates with #2999 / #3013: the publish-time CLI bundle stays decoupled from the declaration build, and no build path deletes
packages/cli/bundle/.Blocked on #2618 for the last two shard builds.
Summary by CodeRabbit
Build & CI
Bug Fixes
Tests