Skip to content

Make typecheck self-generate workspace declarations (Fixes #3536) - #3683

Merged
acoliver merged 1 commit into
mainfrom
issue3536
Sep 16, 2026
Merged

acoliver merged 1 commit into
mainfrom
issue3536

Conversation

@acoliver

@acoliver acoliver commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

TLDR

npm run typecheck now regenerates workspace declarations itself (npm run build:types runs 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 .tsbuildinfo would otherwise make tsc silently skip. fixes #3536

Dive Deeper

Root cause. Three workspace tsconfigs map cross-workspace imports at dist declarations (cli -> tools/dist, core -> mcp/dist, a2a-server -> settings/storage/tools dist). Whenever packages/*/dist/*.d.ts exists 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, bare npm run typecheck could not pass without an undocumented prior npm run build, which is exactly the order-dependence #3536 reports (repros in #3536 and #3670).

Fix, in three parts.

  1. Root package.json typecheck script prepends npm run build:types && ; the original three segments (workspace checks, tsconfig.scripts.json, evals/tsconfig.json) are unchanged and still fail fast.
  2. scripts/build_package.ts declaration-only mode (from Remove the build from the test path; keep it for lint/typecheck only and emit declarations, not JS #2983):
    • skips the tsc --build --clean pre-step. That clean deletes the compiled JavaScript of a prior full build, leaving a partial dist, and a partial dist breaks subsequent bun test runs 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 as Cannot find module './src/index.js' from packages/mcp/dist/mcp/index.d.ts).
    • passes --force alongside --emitDeclarationOnly. On TypeScript 5.8.3, a plain declaration build silently skips emission (exit 0, no files) when a surviving .tsbuildinfo claims outputs that were deleted (.tsbuildinfo lives outside dist, so it survives dist wipes). --force makes 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 typecheck now succeeds and leaves declaration-only dist output (no JavaScript); a subsequent npm run test fails on dist-mapped imports until npm run build runs. 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:types before lint:ci (type-aware ESLint needs declarations and runs before typecheck), so the typecheck step now runs it a second time. Harmless (--force re-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:

  1. npm run typecheck with no prior build: passes (this is the headline behavior; on main it fails with unresolved/stale declaration errors).
  2. With stale declarations: mv packages/tools/dist /tmp/bak && npm run typecheck passes and regenerates packages/tools/dist declarations.
  3. Non-destruction: on a fully built checkout, run npm run typecheck, then confirm packages/tools/dist/index.js, packages/mcp/dist/mcp/index.js, packages/storage/dist/index.js still exist, and a previously dist-sensitive test (e.g. cd packages/cli && bun test src/ui/commands/permissionsCommand.test.ts) still passes.
  4. Full builds unchanged: npm run build then npm run test behave as before.
  5. Focused suites: bun test scripts/tests/issue-3536-typecheck-declaration-order.bun.test.ts scripts/tests/issue-2983-declaration-build.test.ts.

Testing Matrix

🍏 🪟 🐧
npm run ✅ (full verification cycle twice: test, lint, typecheck, format, build, smoke) ❓ (CI workflows)
npx ✅ (eslint, prettier on changed files)
Docker ❓ (not exercised; change is platform-neutral build scripting)
Podman - - -
Seatbelt - - -

Linked issues / bugs

fixes #3536 (also covers the #3670 duplicate repro: TS6305 on settings/dist declarations and missing OutputObject members, 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

    • Improved type-checking reliability by generating updated type declarations before validation.
    • Declaration-only builds now refresh declarations reliably, including when previous build metadata is stale or compiled output is missing.
    • Declaration-only builds preserve existing compiled JavaScript, while full builds continue to refresh compiled artifacts.
  • Tests

    • Added regression coverage for type-checking order, declaration generation, and build behavior.

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.
@github-actions github-actions Bot added the maintainer:e2e:ok Trusted contributor; maintainer-approved E2E run label Sep 15, 2026
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 347fd02d-b938-4566-a3fa-5a02a0a503f2

📥 Commits

Reviewing files that changed from the base of the PR and between 8921d84 and 5ca2f0d.

⛔ Files ignored due to path filters (2)
  • dev-docs/cherrypicking-runbook.md is excluded by !dev-docs/**
  • project-plans/20260914-issue3536-typecheck-declarations/PLAN.md is excluded by !project-plans/**
📒 Files selected for processing (4)
  • package.json
  • scripts/build_package.ts
  • scripts/tests/issue-2983-declaration-build.test.ts
  • scripts/tests/issue-3536-typecheck-declaration-order.bun.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The root typecheck script now generates declarations before running workspace and project checks. Declaration-only builds force declaration regeneration without cleaning existing JavaScript. Tests cover command ordering and full versus declaration-only build behavior.

Changes

Declaration-first typecheck flow

Layer / File(s) Summary
Declaration build behavior
scripts/build_package.ts, scripts/tests/issue-2983-declaration-build.test.ts
Declaration-only builds pass --force to TypeScript and skip the pre-build clean. The existing declaration-build test checks the new argument.
Typecheck command ordering
package.json, scripts/tests/issue-3536-typecheck-declaration-order.bun.test.ts
typecheck runs build:types first, followed by four fail-fast checks. Tests validate the command order and declaration-only build configuration.
Build regression coverage
scripts/tests/issue-3536-typecheck-declaration-order.bun.test.ts
Temporary fixtures verify declaration regeneration, JavaScript preservation during declaration-only builds, and stale JavaScript replacement during full builds.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 5ca2f

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: making typecheck regenerate workspace declarations. It also references the fixed issue.
Description check ✅ Passed The description covers the required sections, explains the root cause and implementation, provides a reviewer test plan, includes a testing matrix, and links the relevant issues.
Linked Issues check ✅ Passed Issue #3536 requires typecheck to generate current workspace declarations before downstream checks. package.json now runs npm run build:types first. scripts/build_package.ts uses declaration-onl…
Out of Scope Changes check ✅ Passed The changed files support issue #3536. The package script implements the required order, the build script implements safe declaration regeneration, and the tests verify the required behavior. The avai…
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue3536

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview — PR #3683

  • Reviewed head SHA: 5ca2f0dba4c425ab0ffda18fd81e26c8080a63d4
  • Merge base: 8921d849d8f5e67629b38064648b7dec2deb23b2
  • Range: full from 8921d849d8f5e67629b38064648b7dec2deb23b2
  • Range fallback: none
  • Scope: selected 6 file(s), +373/-9; cumulative 6 file(s), +373/-9
  • Tokens: 0 total (0 input, 0 output, 0 cache)
  • OCR version: open-code-review v1.8.4 (e78474478) linux/amd64 built at: 2026-08-01T03:27:37Z https://github.com/alibaba/open-code-review
  • Phase: llm-preflight
  • Exit code: 1
  • Run: https://github.com/vybestack/llxprt-code/actions/runs/34921698658
  • OCR failed to run or parse output.
  • Artifacts: ocr-review-output contains raw JSON, stdout, stderr, preview, phase, and exit-code diagnostics.
  • Infrastructure diagnostic: phase=llm-preflight; reason=OCR LLM connectivity check failed (model=zai-5.3-flash)

OCR stderr excerpt

Set llm.extra_body = {"thinking": {"type": "disabled"}}
Set language = English
model=zai-5.3-flash
provider-url=configured
Error: llm request failed: POST "[REDACTED]/v1/messages": 401 Unauthorized {"error":{"message":"token expired or incorrect","type":"401"}}

OCR preflight excerpt

model=zai-5.3-flash
provider-url=configured
Error: llm request failed: POST "[REDACTED]/v1/messages": 401 Unauthorized {"error":{"message":"token expired or incorrect","type":"401"}}

OCR preview stderr excerpt

(empty)

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This PR changes 6 file(s).

  • project-plans/20260914-issue3536-typecheck-declarations/PLAN.md: New planning document for issue npm run typecheck fails before build when workspace declaration output is stale #3536. It records the verified root cause (workspace typechecks resolving cross-workspace imports at generated dist declarations, failing when missing or stale), the root reproduction, and five acceptance criteria: prepend npm run build:types to the root typecheck script with &&-only chaining, no changes elsewhere, runbook update, and --force no-clean declaration builds. It also lists boundary cases, planned tests (a new Bun test pinning script ordering), files to modify, explicit out-of-scope items, and the verification cycle.
  • package.json: Modifies the root "typecheck" npm script to first run "npm run build:types" before executing workspace typechecks and the two tsc project checks (tsconfig.scripts.json and evals/tsconfig.json). This makes typecheck self-generate workspace declaration files, so downstream typechecks consume freshly built .d.ts outputs instead of potentially stale or missing declarations (fixes npm run typecheck fails before build when workspace declaration output is stale #3536). No dependencies or other scripts are changed.
  • scripts/tests/issue-3536-typecheck-declaration-order.bun.test.ts: Adds a new Bun test suite for issue npm run typecheck fails before build when workspace declaration output is stale #3536, verifying the typecheck pipeline self-generates workspace declarations before type-checking. Unit tests assert the root package.json typecheck script chains exactly four fail-fast commands with npm run build:types first (workspaces, scripts, evals after) and that build:types uses DECLARATIONS_ONLY_ENV. Integration tests spawn build_package.ts against a temp fixture package, confirming declarations-only builds preserve prior JS output, regenerate declarations when dist is missing, and full builds clean/re-emit stale dist artifacts.
  • dev-docs/cherrypicking-runbook.md: Rewrites the Troubleshooting Notes entry for stale workspace types. Removes the manual workaround (rebuilding @vybestack/llxprt-code-core then rerunning typecheck) and documents that npm run typecheck now self-regenerates workspace declarations via npm run build:types before workspace checks while preserving compiled JavaScript (issue npm run typecheck fails before build when workspace declaration output is stale #3536), so rerunning typecheck clears stale-declaration failures and persistent failures reflect actual source. Adds a warning that on a never-built checkout typecheck emits declaration-only packages/*/dist output, making a following npm run test fail on dist-mapped imports until npm run build runs.
  • scripts/build_package.ts: Fixes stale-declaration builds (issue npm run typecheck fails before build when workspace declaration output is stale #3536). buildTscArgs now adds '--force' to the declaration-only tsc --build invocation, forcing re-emission of .d.ts files when a surviving .tsbuildinfo claims outputs are current but dist was wiped or partially deleted. In main(), the 'tsc --build --clean' step is now skipped for declaration-only builds so compiled JavaScript from a prior full build is preserved; --force refreshes declarations in place instead. Full builds still clean before rebuilding. Comments document why force and no-clean pair together.
  • scripts/tests/issue-2983-declaration-build.test.ts: Updates the declaration-only emit test to expect the new --force flag in the tsc argument list when buildTscArgs is called with declaration-only mode enabled, matching the PR's change that makes typecheck builds self-generate workspace declarations (issue npm run typecheck fails before build when workspace declaration output is stale #3536). Adds a comment noting --force lets declaration builds self-heal a wiped dist directory. The non-declaration-mode expectation is unchanged.

Changes

Layer File(s) Summary
... ... ...

Magnitude

🎯 2 (M)
373 additions, 9 deletions, 6 changed files across 0 packages, 10 acceptance criteria

Related

No related items found.


Walkthrough generated by LLxprt PR Review. Planner issue: #2256

@acoliver

Copy link
Copy Markdown
Collaborator Author

CodeRabbit review disposition (no actionable comments were generated; one pre-merge advisory):

  • Docstring Coverage 20% < 80% (warning) — Dismissed as out of scope for this PR. The five touched functions are small build-script helpers and test helpers in scripts/build_package.ts and the two test files; they already carry the repo's convention of sparse, why-focused comments (the buildTscArgs doc comment explains the --emitDeclarationOnly/--force pairing, and the clean-skip rationale is documented at the guard). Adding JSDoc blocks to satisfy a coverage metric would be unrelated churn for a build-ordering fix, and the repo does not use docstring coverage as a gate. No source change made.
  • The OCR action comment on this PR is a pre-existing CI integration failure (expired token, 401 at LLM preflight), not a review finding; no code action belongs in this PR.

@acoliver
acoliver merged commit 211d186 into main Sep 16, 2026
44 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer:e2e:ok Trusted contributor; maintainer-approved E2E run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npm run typecheck fails before build when workspace declaration output is stale

1 participant