fix(build): detect orphaned compiled tests before building - #5647
seekskyworld wants to merge 2 commits into
Conversation
Generated-by: OpenAI Codex Signed-off-by: seekskyworld <djh1813553759@gmail.com>
Generated-by: OpenAI Codex Signed-off-by: seekskyworld <djh1813553759@gmail.com>
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current six-file root prebuild scanner for orphan compiled test artifacts and its tests. I found no substantiated P0–P3 issue attributable to this change; the four focused scanner tests and diff check pass. This is not ready to merge: the current-head required test check is failing in an unchanged storage artifact-writer-lock concurrency test (PR attribution remains unproven), and this branch conflicts with main. Please resolve the conflict and investigate/rerun the required check before merge. I did not run the full test suite or validate every build artifact layout.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Adds a root prebuild guard that fails the build when dist/**/*.test.{js,jsx,mjs,cjs} has no matching source under src/, with docs and a CI lane. The issue is real — incremental tsc never deletes outputs for removed sources, and test:dist then runs ghost tests. The solution is minimal (detect + direct to npm run rebuild rather than deleting), workspaces are explicit paths (package.json:16-30), Desktop's nested dist/main/__tests__ layout and .d.ts/.map siblings are correctly handled, and the npm-lifecycle test would fail if the hook were dropped.
Findings
- [P2] The
testlane is red on the current head (run 35902091783): storage testbundle export excludes a mutation queued behind the same child-held writer lock. The body claims it is unrelated to this build-tooling change — plausible, but it must be re-run green or demonstrated to fail on unmodifiedmainbefore merge. - No code-level findings. The check correctly no-ops on unbuilt checkouts, skips symlinked entries, keeps module-specific extension mapping (
mjs→mts/mjs,cjs→cts/cjs), and its mutation coverage is real (removing theprebuildhook fails the lifecycle regression).
Verdict
needs-discussion — code is merge-ready, but the failing test lane on head needs a green re-run or proof of pre-existing failure.
Summary
After a test source is removed or renamed, incremental TypeScript builds leave its compiled copy in
dist, andtest:diststill runs it. Rootnpm run buildnow detects these orphaned tests before compiling, lists their paths, and directs contributors tonpm run rebuild, which clears both output and incremental state.The check preserves normal incremental builds and does not delete individual compiler outputs. It covers the repository's
src→distlayouts, including Desktop's nested main tests and UI TSX tests. Contributor guidance and CI regression coverage are included.Fixes #5641
Verification
npm run check:releasepasses all 203 release-contract tests; 19 focused guard/file-policy tests pass. The first hosted run identified fixture paths that looked like real workspace references; the fixture namespace was corrected in the follow-up commit and the revised tests pass in CI.Hosted check
On head
0e34d7ee5, Windows/Linux packaging, audits, and both owner checks pass. The main test run 35902091783 fails in the unchanged storage testbundle export excludes a mutation queued behind the same child-held writer lock: the export containedafter-exportas well asseed. This is a separate failure from the corrected fixture-path issue; CI is not green. The new build-guard tests and release contracts passed in this run.AI use
Tool(s) and scope: OpenAI Codex inspected the issue, implemented and tested the build guard, and drafted this description under human direction.
Checklist
Does this PR entail a change in behavior?