Repository navigation
[Fix] Remove a switch's leftover route when TypeScript build info sits beside its install - #532
Conversation
…s beside its install Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe leftover-install check now treats
Assessment against linked issues — out-of-scope changes: No out-of-scope changes were identified. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change allows stale route cleanup when only build-info files remain, while preserving directories with non-ignored files. The supplied evidence identifies no remaining merge-blocking risk. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is limited to cleanup of leftover route directories. Existing checks still protect routes in the destination branch and files that the destination does not ignore. No new production entrypoint or material security issue was established, though deletion remains a consequential operation. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
Manual pass on Windows (UTM VM), Buildkite artifact for c7ec033, on a new Gutenberg site created on trunk (25 Sep):
The earlier site most likely got its |
|
@coderabbitai review |
✅ Action performedReview finished.
|
## Why `v1.2.0-beta.2` shipped on 23 September. Five commits reach trunk since then: #524, #528, #530 and #532 fix what the beta.2 testing turned up (files another branch ignored counted as changes after a switch, `NODE_OPTIONS` lost in the macOS shims, and a trunk-only Gutenberg route left behind by a switch to an older pull request, which broke `wp-build`), and #531 makes the manual **Real WordPress setup** workflow pass again, green on both platforms and both targets on trunk's app code. Milestone `v1.2.0` holds only the umbrella #251. `electron-builder` derives artifact names from `package.json`, so the version has to move before the tag and the artifacts exist. This is a stable release: it takes `releases/latest` from `v1.1.0`. ## What changes Only the version. All three package-version fields in `package.json` and `package-lock.json` move together from `1.2.0-beta.2` to `1.2.0`. No dependency versions change, and there is no application behaviour change in this PR. ## How to test this Platforms: any for the suite; macOS and Windows for the artifact checks that follow the merge. 1. `npm run lint` is clean and `npm test` passes, from the repository root. 2. `node -e "console.log(require('./package.json').version)"` prints `1.2.0`. 3. `python3 -c "import json;d=json.load(open('package-lock.json'));print(d['version'], d['packages']['']['version'])"` prints `1.2.0 1.2.0`. **What must not have happened:** no dependency version may change. `git diff` must show exactly three changed lines, all of them the root package version. After merge, the release build must produce artifacts named `wordpress-contributor-toolkit-1.2.0-*`; the asar root must be exactly `node_modules`, `package.json`, `src`, with npm 11 under `node_modules/npm` and no `.codesigning` directory; on macOS `CFBundleShortVersionString` reads `1.2.0`. ## Risks and limitations No UI change. The risk is a mis-scoped edit in `package-lock.json`, where dependencies also carry version fields; the replacement was confined to the first 2000 bytes of the file, which hold the two root entries and nothing else, and both files were re-parsed as JSON afterwards. Manual passes on what reached trunk after beta.2: #528 and #530 on macOS in their PRs; #530 plus #532 on Windows on #532's artifact (c7ec033), including the case that failed on e0a3faa. ## Related Part of #251. Release notes follow on the release; docs publish for the tag per #487. --- <details> <summary>Review outcome (required, see AGENTS.md)</summary> 0 [fix here] · 0 [follow-up]. Deterministic layer only: lint clean, 1667 tests pass, both files valid JSON, diff confined to the three root version fields. No judgement pass was run: this change has no logic to review. - **Review:** completed, deterministic layer, in session; head is the single bump commit / base `4c3b05a` (trunk); evidence above; outcome: nothing to fix. - **Since review:** none. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Why
#530 does not fix #529 on a real site. A Windows pass on the trunk artifact (e0a3faa) of the v1.2.0 candidate, on a Gutenberg site switching from its issue branch to
pr/76292, still failed Apply and rebuild withENOENT ... routes\dashboard\package.json. The leftoverroutes\dashboard\heldnode_modulesandtsconfig.tsbuildinfoandtsconfig.test.tsbuildinfo, which trunk'stsc --buildwrites there and Gutenberg ignores. #530 removes the directory only when it holds nothing butnode_modules, so it kept it. Nothing was running during the switch. Any site whose trunk types were built has these files, on either platform; the macOS pass for #530 most likely ran on a site that did not.What changes
Root cause: the "holds only an install" rule was narrower than what a build leaves in a route.
holdsOnlyInstallsinsrc/ticket-branches.jsalso accepts*.tsbuildinfofiles. They are TypeScript's incremental cache, rewritten by the next build, and the name is TypeScript's convention, not Gutenberg's. Every other guard stays: the directory must be missing from the new tree, andotherPathsstill refuses anything untracked and not ignored, so a.tsbuildinfothe destination does not ignore keeps the directory (and #521 excludes it).Deliberately not: accepting any ignored file. Ignored files include a contributor's
.envor local config, and this code deletes.How to test this
Platforms: Windows is where it was seen; either reproduces it. Build: the Buildkite artifact for this PR's head.
Starting state: a Gutenberg site on trunk, installed and built, no pull request checked out, no changes;
routes\dashboard\holdsnode_modulesand the two.tsbuildinfofiles. A site already onpr/76292with the leftover is not healed: go back to trunk first, or deleteroutes\dashboard\.76292(by number, or through a linked issue). Let the install finish.Get-ChildItem routes(PowerShell) orls routes. Expected: nodashboard.routes/dashboard/node_modules/is back and the site reports no changes.What must not have happened: other
routes/*directories touched; the rootnode_modulesrebuilt by the switch itself.On the old code,
TypeScript build info beside the install goes with it (issue #529)intests/unit/ticket-branches.integration.test.cjsfails (checked:removedis[]) and passes here.TypeScript build info the destination does not ignore keeps the directory (issue #529)pins the guard.Risks and limitations
build/beside the install) still keeps the directory, as before. Not seen; widen only on evidence.Related
Follow-up to #530. Fixes #529 for sites with built types.
Review outcome (required, see AGENTS.md)
.github/instructions/code-review.instructions.mdstep 3; head c7ec033 / base e0a3faa;npm run lintclean,npm test1667 pass 0 fail. 0 [fix here] · 0 [follow-up]. Checked:otherPathsstill gates non-ignored build info; readdirDirent.isFile()is false for symlinks and junctions on Windows, so a link named*.tsbuildinfokeeps the directory; both tests fail if their half of the rule breaks. Nit not taken:holdsOnlyInstallsno longer names everything it accepts.🤖 Generated with Claude Code