fix(build): clean stale outputs before tsc --build to prevent TS5055#4453
Merged
Conversation
Run `tsc --build --clean` before `tsc --build` in build_package.js so a stale tsconfig.tsbuildinfo (e.g. after a version bump, branch switch, or a prior `npm ci` prepare) cannot collide with composite project references emitting back into packages/core/dist. Closes #4447
Contributor
📋 Review SummaryThis PR addresses a real and painful build issue where stale TypeScript build outputs cause TS5055 errors after version bumps or branch switches. The fix is minimal, targeted, and follows TypeScript's recommended approach for cleaning composite project builds. The implementation is correct and well-documented. 🔍 General Feedback
🎯 Specific FeedbackNo specific issues identified in this review. ✅ Highlights
RecommendationApprove - This is a solid fix for a documented build reliability issue. The approach is minimal, uses TypeScript's intended workflow, and includes proper documentation of the rationale. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses TS5055 build failures caused by stale TypeScript build artifacts by adding an automatic clean step before each per-package tsc --build invocation in the workspace build flow (closes #4447).
Changes:
- Run
tsc --build --cleanbeforetsc --buildinscripts/build_package.jsto remove stale outputs and prevent TS5055.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+33
| // Clean stale outputs first to avoid TS5055 when tsbuildinfo is out of sync | ||
| // with sources (e.g. after a version bump or branch switch) under composite | ||
| // project references. | ||
| execSync('tsc --build --clean', { stdio: 'inherit' }); | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
Verified the concern: from packages/cli, tsc -b --clean does walk references and wipes packages/core/dist/. In scripts/build.js's dependency-ordered build, every downstream package would clean and force a rebuild of upstream outputs — real perf regression.
Fixed in 109a092: replaced tsc -b --clean with direct rmSync('dist') + rmSync('tsconfig.tsbuildinfo'), scoped strictly to the current package. Same outcome for the TS5055 case, no reference-graph blast radius. Thanks for the catch.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
This fix works but unconditionally running A less expensive alternative: try incremental first, fall back to clean only on failure: try {
execSync('tsc --build', { stdio: 'inherit' });
} catch {
execSync('tsc --build --clean', { stdio: 'inherit' });
execSync('tsc --build', { stdio: 'inherit' });
}This preserves fast incremental builds in the 99% case (normal development) while still auto-recovering from stale tsbuildinfo after version bumps or branch switches. |
wenshao
requested changes
May 23, 2026
|
|
||
| // Clean stale outputs first to avoid TS5055 when tsbuildinfo is out of sync | ||
| // with sources (e.g. after a version bump or branch switch) under composite | ||
| // project references. |
Collaborator
There was a problem hiding this comment.
[Critical] tsc --build --clean traverses project references and destroys the dist/ output of ALL referenced packages, not just the current package.
In the sequential build (scripts/build.js: core → acp-bridge → cli), when build_package.js runs for cli (which references 6 packages via tsconfig.json references), --clean wipes the already-built outputs of core/, acp-bridge/, and all channel packages. This means:
coregets compiled 2-3 times in a singlenpm run build- After a full build, intermediate packages (core, acp-bridge, channels/*) have no valid
dist/— onlycliends up with usable output - Incremental compilation is eliminated entirely
- Non-TypeScript assets copied by
copy_files.js(.md,.jsonskill definitions,.last_buildmarker) are also destroyed and never restored for dependency packages
Suggested change
| // project references. | |
| import { existsSync, rmSync } from 'node:fs'; | |
| import { join } from 'node:path'; | |
| // Clean stale outputs first to avoid TS5055 when tsbuildinfo is out of sync | |
| // with sources (e.g. after a version bump or branch switch). | |
| const distDir = join(process.cwd(), 'dist'); | |
| if (existsSync(distDir)) { | |
| rmSync(distDir, { recursive: true, force: true }); | |
| } | |
| const tsBuildInfo = join(process.cwd(), 'tsconfig.tsbuildinfo'); | |
| if (existsSync(tsBuildInfo)) { | |
| rmSync(tsBuildInfo, { force: true }); | |
| } |
This scopes cleanup to only the current package, matching the existing scripts/clean.js approach.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
This thread is on the original commit 376f2ad6e (which used tsc -b --clean). Already addressed in follow-up commit 109a09294 — replaced with package-scoped rmSync('dist') + rmSync('tsconfig.tsbuildinfo'), matching the suggested code modulo the existsSync wrappers (rmSync with { force: true } already tolerates missing paths, so the existsSync check is redundant).
Maintainer validation at head 109a09294 confirmed all gates green.
|
|
||
| // Clean stale outputs first to avoid TS5055 when tsbuildinfo is out of sync | ||
| // with sources (e.g. after a version bump or branch switch) under composite | ||
| // project references. |
Collaborator
There was a problem hiding this comment.
[Suggestion] Two related concerns with the unconditional --clean-before---build pattern:
-
No atomicity / rollback: The destructive
--cleanruns before--build. Iftsc --buildfails (type error, OOM, missing dependency), the olddist/is already gone — the package is left unusable until a successful rebuild. Before this PR, a failedtsc --buildleft the previous outputs intact. -
Unconditional incremental state loss:
tsc --build --cleanruns on every invocation, not just whentsbuildinfois actually stale. The project configures"incremental": true+"composite": truespecifically for fast incremental rebuilds, but this change forces a full cold build every time — even when no source files changed.
Consider either: (a) checking whether tsbuildinfo is actually stale before cleaning, or (b) moving cleanup to only run on the first package in the build sequence from the root orchestrator (scripts/build.js).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Both concerns are real, but I think the proposed alternatives are worse than the trade-off as shipped:
On atomicity: A failed tsc --build already implies the package is in a broken state (type errors, missing deps, OOM). Preserving the previous dist/ doesn't make it usable — downstream consumers would still trip on stale interfaces vs current source. Build scripts aren't expected to be transactional; both CI and dev iteration want a clear failure signal, not a silently-stale artifact.
On unconditional incremental loss: Real cost, but:
- Staleness detection (option a) is brittle — mtime comparisons race under parallel writes, behave differently in linked worktrees, and don't catch all triggers (e.g.
version.jsbump touchespackage.jsonbut TS doesn't track that file). - Moving cleanup to the root
scripts/build.js(option b) doesn't covernpm run build --workspace=...invocations or thenpm cipreparelifecycle, which are exactly some of the documented TS5055 triggers in bug: npm run build fails with TS5055 due to stale dist artifacts #4447. - The TS5055 triggers (version bump, branch switch,
npm ciprepare) typically invalidate large portions of the reference graph anyway, so the incremental win in the avoidable-cleanup case is small even when applicable. - For fast inner-loop iteration,
tsc -b --watch/npm run devare the right tools;npm run buildis for fresh trees / CI / pre-publish, where a full rebuild is the expected cost.
Happy to revisit if a specific workflow regresses meaningfully in practice.
Replace `tsc --build --clean` with direct `rmSync` of `dist` and `tsconfig.tsbuildinfo`. `tsc -b --clean` walks project references, so when scripts/build.js builds packages in dependency order, cleaning from a downstream package (e.g. cli) would also wipe upstream outputs (core, acp-bridge, channels) that were just built — a major perf regression. Spotted by Copilot review on #4453.
Collaborator
Local maintainer validation — all gates green ✅Reviewed at head Environment
Results
TS5055 reproduction — verified fixedThe PR's stated bug is that after a Simulated by placing dummy stale Then ran This confirms the Triage of the 5 core failures (NOT caused by PR 4453)
The CLI suite is completely clean (6783 tests, 0 failures across 377 files) — this is the primary regression check for any build-script change, as the CLI package depends on the core build output. Scope / risk
Reviewer recommendationSafe to merge. This is a minimal build-hygiene fix:
— Maintainer local validation, run on |
wenshao
reviewed
May 23, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
LGTM on the code changes. The switch from tsc --build --clean to rmSync correctly addresses the reference-walking concern. One suggestion below.
| // using `tsc --build --clean`, because the latter walks project references | ||
| // and would wipe upstream packages already built by scripts/build.js. | ||
| rmSync('dist', { recursive: true, force: true }); | ||
| rmSync('tsconfig.tsbuildinfo', { force: true }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Unconditionally deleting tsconfig.tsbuildinfo before every build eliminates TypeScript's incremental compilation entirely. The composite: true + incremental: true setup in this monorepo exists specifically to enable tsc --build to skip unchanged projects. Every npm run build will now do a full recompile regardless of what changed.
Consider making the cleanup conditional — only delete tsbuildinfo when it's genuinely stale (e.g., after a version bump, branch switch, or when source mtime is newer than tsbuildinfo mtime).
Suggested change
| rmSync('tsconfig.tsbuildinfo', { force: true }); | |
| // Only clean tsbuildinfo when sources are newer (version bump / branch switch) | |
| if (existsSync('tsconfig.tsbuildinfo')) { | |
| const tsInfoStat = statSync('tsconfig.tsbuildinfo'); | |
| const srcStat = statSync('src'); | |
| if (srcStat.mtimeMs > tsInfoStat.mtimeMs) { | |
| rmSync('tsconfig.tsbuildinfo', { force: true }); | |
| } | |
| } else { | |
| // No tsbuildinfo yet — first build, nothing to clean | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
|
@yiliang114 the try-then-clean-on-failure pattern has two problems I'd want to address before adopting it:
The fix as shipped trades incremental compilation in |
wenshao
approved these changes
May 23, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅ The switch from tsc --build --clean to package-scoped rmSync correctly addresses the reference-walking concern, and the fix is minimal and well-motivated. — qwen3.7-max via Qwen Code /review
yiliang114
approved these changes
May 23, 2026
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The package-scoped rmSync approach is correct and simple — only cleans the current package's own outputs without affecting upstream dependencies in the build order. The incremental compilation trade-off is acceptable given the real-world usage patterns of npm run build.
5 tasks
This was referenced May 27, 2026
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
Co-authored-by: Jacob Richman <jacob314@gmail.com>
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
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.
Summary
Closes #4447.
scripts/build_package.jsrunstsc --buildwithout first cleaning previous outputs. Whenpackages/core/dist/contains stale.d.tsfiles andtsconfig.tsbuildinfois out of sync with sources — typically after aversion.jsbump, a branch switch, or a priornpm ciprepare— composite project references resolve those stale.d.tsfiles as inputs to dependent packages, and TypeScript then fails to emit back into the same paths withTS5055: Cannot write file ... because it would overwrite input file.This prepends
tsc --build --cleanso each per-package build starts from a known-clean state. Usingtsc -b --clean(rather thanrmSync) keeps the cleanup scoped to files TypeScript actually emitted and reads naturally to anyone scanning the script.Reproduction (before fix)
Verification
After the patch, the same sequence completes cleanly:
Test plan
mainviabuild → version bump → buildnpm run buildstill passes from a clean tree🤖 Generated with Qwen Code