Conversation
The documented verification cycle runs typecheck before build, but three workspace tsconfigs resolve cross-workspace imports at dist declarations (cli -> tools, core -> mcp, a2a-server -> settings/storage/tools), so the result depended on declaration output left by earlier work in the checkout. On a clean tree with stale or absent dist, typecheck failed with missing-export errors that a full build then healed (#3536, #3670). The root typecheck script now runs npm run build:types first, and the declaration-only build mode skips tsc --build --clean, which deletes the compiled JavaScript of a prior full build and leaves a partial dist that breaks bun test (per #2983), and passes --force so declaration emit self-heals a wiped dist despite a surviving .tsbuildinfo, which TypeScript 5.8.3 otherwise silently skips. Full builds are unchanged. Boundary: on a never-built checkout, typecheck leaves declaration-only output; npm run test needs a prior full build there. Documented in the cherrypicking runbook.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe root ChangesDeclaration-first typecheck flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The updated typecheck flow regenerates declarations before validation without requiring a prior full build. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
OpenCodeReview — PR #3683
OCR stderr excerptOCR preflight excerptOCR preview stderr excerpt |
WalkthroughThis PR changes 6 file(s).
Changes
Magnitude🎯 2 (M) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
|
CodeRabbit review disposition (no actionable comments were generated; one pre-merge advisory):
|
TLDR
npm run typechecknow regenerates workspace declarations itself (npm run build:typesruns first), so its result no longer depends on declaration output left behind by earlier builds in the checkout. The declaration-only build mode was also made non-destructive and self-healing: it no longer wipes compiled JavaScript from a prior full build, and it re-emits declarations even when a stale.tsbuildinfowould otherwise maketscsilently skip. fixes #3536Dive Deeper
Root cause. Three workspace tsconfigs map cross-workspace imports at dist declarations (cli ->
tools/dist, core ->mcp/dist, a2a-server ->settings/storage/toolsdist). Wheneverpackages/*/dist/*.d.tsexists and predates current source, the workspace type checks read the stale declarations and report missing exports/properties that exist in source. On a clean tree with no dist at all, the same mapped paths fail to resolve. Either way, barenpm run typecheckcould not pass without an undocumented priornpm run build, which is exactly the order-dependence #3536 reports (repros in #3536 and #3670).Fix, in three parts.
package.jsontypecheckscript prependsnpm run build:types &&; the original three segments (workspace checks,tsconfig.scripts.json,evals/tsconfig.json) are unchanged and still fail fast.scripts/build_package.tsdeclaration-only mode (from Remove the build from the test path; keep it for lint/typecheck only and emit declarations, not JS #2983):tsc --build --cleanpre-step. That clean deletes the compiled JavaScript of a prior full build, leaving a partial dist, and a partial dist breaks subsequentbun testruns on dist-mapped imports (Remove the build from the test path; keep it for lint/typecheck only and emit declarations, not JS #2983's documented rule; observed locally asCannot find module './src/index.js' from packages/mcp/dist/mcp/index.d.ts).--forcealongside--emitDeclarationOnly. On TypeScript 5.8.3, a plain declaration build silently skips emission (exit 0, no files) when a surviving.tsbuildinfoclaims outputs that were deleted (.tsbuildinfolives outsidedist, so it survives dist wipes).--forcemakes declaration emit self-heal a wiped or partial dist while preserving existing dist JavaScript. Full (release) builds are completely unchanged: clean + build still run.Known boundary (documented in the cherrypicking runbook). On a checkout that has never been built,
npm run typechecknow succeeds and leaves declaration-only dist output (no JavaScript); a subsequentnpm run testfails on dist-mapped imports untilnpm run buildruns. Before this change the same sequence failed earlier, at typecheck itself. Repointing the dist-mapped tsconfig paths at source (#2618) removes this class entirely and remains the accepted future effort.CI note (accepted redundancy). The lint job already runs
npm run build:typesbeforelint:ci(type-aware ESLint needs declarations and runs before typecheck), so the typecheck step now runs it a second time. Harmless (--forcere-emits idempotently), at the cost of some minutes on that job. Removing the explicit step would break lint, so it stays.Reviewer Test Plan
On a clean checkout at this PR head:
npm run typecheckwith no prior build: passes (this is the headline behavior; onmainit fails with unresolved/stale declaration errors).mv packages/tools/dist /tmp/bak && npm run typecheckpasses and regeneratespackages/tools/distdeclarations.npm run typecheck, then confirmpackages/tools/dist/index.js,packages/mcp/dist/mcp/index.js,packages/storage/dist/index.jsstill exist, and a previously dist-sensitive test (e.g.cd packages/cli && bun test src/ui/commands/permissionsCommand.test.ts) still passes.npm run buildthennpm run testbehave as before.bun test scripts/tests/issue-3536-typecheck-declaration-order.bun.test.ts scripts/tests/issue-2983-declaration-build.test.ts.Testing Matrix
Linked issues / bugs
fixes #3536 (also covers the #3670 duplicate repro: TS6305 on
settings/distdeclarations and missingOutputObjectmembers, healed by the same declaration regeneration). Related: #2983 (declaration-only build mode this builds on), #2618 (accepted future effort to repoint dist-mapped paths at source), #3480 (verification session where this was observed).Summary by CodeRabbit
Bug Fixes
Tests