fix(docs): the fresh-worktree node_modules remedy produces a broken worktree - #1288
Conversation
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
The ws diagnosis holds up and the script does what it says on the tin, but the framing was too strong in a way that matters.
Linking makes the suite RUNNABLE, not self-testing. node_modules/@webjsdev/core is a relative symlink into the primary's packages/core, so through the linked root every bare @webjsdev/* specifier resolves into the PRIMARY checkout. A worktree editing framework source therefore runs the primary's copy through those paths; relative imports and the browser suite, which wtr serves from the worktree, do use its own files. Verified directly. That also means my headline evidence, that the residual failures match the primary's, is consistent with the setup working AND with it simply executing the primary's code, so it never distinguished the two. Now stated plainly in both the script header and the doc.
Knock-on: the sentence I kept about linking a subset of @webjsdev/* packages became unactionable, because after linking, the worktree's node_modules IS the primary's, so following it writes into the primary. Retired.
The webjs doctor and webjs dev remedy string still suggests the root-only symlink this doc now warns against. It is correct for a scaffolded app worktree, which has no nested trees and no built dist, so I noted the distinction rather than changing the runtime message.
Two smaller ones: the "discovers both" claim was only true of the node_modules half, since the dist entry is a literal one-element list; and the defaultPrimary comment described --git-dir arithmetic rather than the --git-common-dir the code uses, in the one branch no test covered.
…orktree AGENTS.md told you to symlink the primary checkout's root node_modules. Doing exactly that leaves a worktree that looks set up and then fails hundreds of assertions for reasons that point nowhere near the cause. The root tree carries ws@7, hoisted for another dependent, while packages/server declares ^8.20.0 and keeps ws@8 nested. Link only the root and WebSocketServer, a ws@8-only named export, resolves up to ws@7 and throws at module load, reddening the server, integration, and smoke suites without ever naming ws. packages/core/dist is the second gap: it is built rather than committed, so every test importing the built bundle fails to resolve it. npm run worktree:link discovers both from the primary rather than hardcoding a list, since the set changes whenever a package gains a nested tree. Measured on a virgin worktree: 936 failures before, 29 after, and that residual set is the same one the primary checkout fails locally. The safety rules are not hypothetical. The naive version of this script created a dangling node_modules/node_modules and deleted a git-tracked directory that happened to be named node_modules, so it now refuses to overwrite any existing path, refuses to create a dangling link, and never descends into node_modules.
Through the linked root, every bare @webjsdev/* specifier resolves into the PRIMARY checkout, because node_modules/@webjsdev/core is a relative symlink into the primary's packages/core. So a worktree editing framework source runs the suite against the primary's copy through those paths, while relative imports and the browser suite do use its own. Linking makes the suite runnable, not self-testing, and presenting it as a fix without saying so was misleading. That also retires the retained subset-linking sentence, which after linking would have written into the primary's own node_modules. The doctor and dev remedy message still suggests the root-only symlink. It is correct for a scaffolded app worktree, which has no nested trees and no built dist, so it is noted rather than changed. Also corrects the defaultPrimary comment, which described --git-dir arithmetic rather than the --git-common-dir the code actually uses, and covers that branch with a test; it was the only one nothing exercised.
0ba1f22 to
14015da
Compare
1 participant
Closes #1287
Summary
AGENTS.mdtold you to set up a fresh worktree by symlinking the primary checkout's rootnode_modules. Following that exactly produces a worktree that looks set up and then fails hundreds of assertions for reasons that point nowhere near the real cause.The root tree carries
ws@7(hoisted for another dependent) whilepackages/serverdeclares^8.20.0and keepsws@8nested. Link only the root andWebSocketServer, a ws@8-only named export, resolves up to ws@7 and throws at module load. That single miss reddens the server, integration, and smoke suites without ever naming ws, so the natural response is to go hunting in your own diff.packages/core/distis the second gap: it is built rather than committed, so every test importing the built bundle fails to resolve it in a fresh worktree.npm run worktree:linkdiscovers both from the primary checkout rather than hardcoding a list, because the set changes whenever a package gains a nested tree. It found one I did not know existed (packages/server/test/node_modules), which is the argument for discovery over a written-down list.Measured
On a virgin worktree cut from
origin/main:npm testfailuresnode_modulessymlink only (what AGENTS.md said)node_moduleslinkspackages/core/distlinkThe residual 29 is the same set the PRIMARY checkout fails locally (
differential-elision,blog-smoke,blog-http, and twotest/bun/listener*files), so the worktree-specific gap is closed.The safety rules are not hypothetical
The naive version of this script, written while investigating, created a dangling
node_modules/node_modulesand deleted a git-tracked directory that happened to be namednode_modules(packages/editors/nvim/vendor/node_modules). So the shipped script never overwrites an existing path, never creates a dangling link, and never descends intonode_moduleshunting for more. Each of those has a test.Deliberately not done
The
webjs doctorversion-skew check floated in the issue is not here. It is a genuinely bigger change (a new check, a stable code,--jsonshape, fixtures) and the doc correction plus the script already remove the failure. Happy to do it separately if you want the diagnosis surfaced at runtime too.Test plan
test/repo-health/link-worktree-deps.test.mjs, 7 tests, all green. Covers nested links, thecore/distlink, idempotence, refusing to clobber a real install, refusing a dangling link, refusing to link a checkout to itself, and not descending intonode_modules.scripts/helper, a root npm script,AGENTS.md, and a new test).packages/*/srcis untouched.AGENTS.md(the corrected remedy, which is the deliverable). Not the docs site or the scaffold skill: this is framework-monorepo tooling and a scaffolded app has no primary checkout to link from.