fix: npm install in a linked worktree corrupts the shared node_modules - #1447
fix: npm install in a linked worktree corrupts the shared node_modules#1447vivek7405 wants to merge 11 commits into
Conversation
`npm run worktree:link` symlinks a worktree's node_modules at the primary checkout's, so an install run in the worktree acts on the primary. Measured on npm 11.19.0 and bun 1.3.14: `npm ci` deletes the primary's whole node_modules through the link, `bun install` writes packages into it, and `npm install` silently replaces the link with a real tree and detaches the worktree. The damage lands on a session that did nothing wrong. No package-manager lifecycle hook can prevent this. npm removes the symlink before `preinstall` runs, `npm ci` has already emptied the primary by then, and Bun runs `preinstall` in time but ignores a non-zero exit. A PreToolUse hook is the only layer that sees the state before the manager starts.
The PreToolUse hook covers Claude Code sessions. Every other tool needs a diagnosis after the fact, because the three damaging shapes are otherwise silent: `npm install` detaches the worktree, `npm ci` empties the primary's node_modules, and `bun install` writes into it through the link. The script always exits 0. A non-zero preinstall blocks `npm ci` and would red every CI job, and it cannot prevent the damage anyway, so making it a guard would be a guard in name only. It returns immediately unless `.git` is a FILE, so a normal clone and CI never see it.
The primary's `node_modules/@webjsdev/` accumulates three defect classes when an install runs inside a linked worktree: links that dangle into a removed worktree, links resolving into a live foreign checkout (the worst case, since it resolves fine and silently runs another branch's source), and links that resolve inside the primary but absolutely where every sibling is relative. All three are repaired to the relative form, atomically, since `npm test` resolves these from many processes at once. A LIVE `.name-HASH` staging entry is left alone: most resolve to real in-repo directories, nothing imports them, one predates a package rename, and deleting one risks racing a live install. Only a staging entry that is BOTH staging-shaped and dangling is removed. `--check` reports without writing, mirroring `npm run check:git`.
The cleanup hook removes merged worktrees and had no idea the primary could hold `@webjsdev/*` links pointing INTO the tree it is deleting. It now repoints them at the primary's own packages first, while the target is still identifiable, and drops an npm staging entry that would be left dangling. Scoped to links targeting THIS worktree, and conservative: a link it cannot correct is kept and reported rather than repointed at a missing path, and a worktree that is KEPT has its links left pointing at it.
`FRAMEWORK_RESOLVE` is a bare `require.resolve` in a try/catch, so it reports only resolvable versus not. A link into a live FOREIGN checkout passes it while being exactly the corrupted state, and silently runs another branch's framework source. Its remedy also recommended a bare `npm install`, which is the command that causes this when node_modules is a symlink. The new `framework-links` check judges the link against the tree that PHYSICALLY owns its node_modules. That owner rule is what keeps a healthy linked worktree passing, since there node_modules is itself a symlink at the primary's, so the primary is the owner. The target is resolved against the directory the link physically sits in for the same reason: resolving `../../packages/core` lexically through a symlinked node_modules lands under the worktree and reports every correctly linked worktree as foreign.
|
Design rationale: why prevention moved off I filed this against a mechanism that turned out to be wrong, so it is worth writing down what actually happens before anyone tries to "simplify" this back. The filing said npm's workspace linker rewrites I never identified which npm or Bun version wrote the absolute worktree-anchored targets sitting in my primary. I stopped looking once it was clear the fix does not depend on the answer: all three reproducible behaviours plus the historical debris are covered by the same set of changes. The bigger correction is that my proposed fix could not have worked. I wanted a That is why prevention is a One thing I want to flag as a deliberate non-fix. The structurally correct answer is to stop sharing one |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through the whole diff. The five layers hang together and the measurement work behind the design is the right call, but there are four real defects here and one doc line the PR falsified.
The one that worries me most is the doctor remedies. I rewrote them thinking of this monorepo and forgot they ship in the published CLI, where webjs dev prints them verbatim to someone whose scaffolded app has no worktree:link script. That is the #954 path, which is exactly the audience those messages exist for, so I made that case worse while fixing mine.
The --check bug is the same shape of mistake: I put the early exit inside the else, so the one flag combination my own docs promise is safe is the one that mutates. Reproduced it directly, and it created the symlink.
The install gate leaking bun i matters because Bun is the manager that writes THROUGH the link into the primary rather than replacing it, so the alias that gets past the gate is the one that does the damage the gate exists to stop.
Also AGENTS.md:71 still says the doctor remedy suggests the root-only symlink. This PR deleted that suggestion, so the line is now false.
The PR body the review read was the stale WIP one; I have since replaced it with the real test plan, so that finding is already closed.
The doctor remedies are the serious one. They ship in the PUBLISHED CLI and `bin/webjs.js` prints them verbatim as the `webjs dev` / `webjs start` preflight failure, so naming `npm run worktree:link` unconditionally sent the #954 audience, a scaffolded APP worktree with no such script, to run something that does not exist. They are app-generic again, and name the monorepo script only where a package.json actually declares it. `--check` was not read-only: its exit sat inside the repair `else`, so with `WEBJS_NO_WORKTREE_REPAIR=1` it fell through to the linking loop and the seed step. The one flag pair documented as changing nothing was the pair that wrote. The install gate admitted `npm i` but leaked `bun i`, `pnpm i`, bare `yarn`, `npm clean-install`, `npm ic` and `npm in`. `bun i` is the consequential leak, since Bun writes THROUGH the link into the primary rather than replacing it. The teardown repoint shadowed the script-global `base`, which holds the merge base ref `is_merged()` reads. Latent today, and bash's dynamic scoping makes it a silent worktree leak the day anything calls a helper from that function. Each defect gains the regression test that would have caught it.
The bare-`yarn` branch was the worst of these. It lived inside the generic VERBS pattern, whose prefix any space satisfies, so it matched the token ANYWHERE and blocked `which yarn`, `rm -rf /tmp/yarn` and `git switch -c feat/yarn`. That fires on ordinary commands in a linked worktree, which is the mandated working state here, and a gate that cries wolf gets turned off. It is now its own anchored pattern requiring command position. The hyphenated npm verbs needed spelling out. The trailing word boundary excludes `-`, so listing `install` never reached `install-test`, `install-ci-test` or `clean-install-test`, and the short aliases do not cover them. The remove verbs are in the table too: `npm rm` in a linked worktree deletes from the checkout that owns the tree. `--check` under WEBJS_NO_WORKTREE_REPAIR=1 exited 0 while inspecting nothing, because `touched` kept its initializer when the repair block was skipped. The hatch suppresses the repair WRITE, and `--check` never writes, so it now always inspects. My previous test asserted the exit 0 and locked the defect in. The `entry_name` rename gets the source guard the last commit claimed for it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, scoped to the fix commit and its blast radius. Five more, and the fix commit introduced two of them, which is the part worth dwelling on.
The bare-yarn branch is the one that would have hurt. I bolted it onto the generic VERBS pattern, and that pattern's prefix is satisfied by any space, so it matched the token anywhere in the line rather than in command position. which yarn, rm -rf /tmp/yarn, git switch -c feat/yarn all blocked with an install-corruption message. Every worktree here is a linked worktree, so that fires constantly, and a gate that cries wolf is a gate someone turns off. It is its own anchored pattern now, requiring command position.
The other self-inflicted one: moving the --check exit out of the else fixed the write, but touched keeps its initializer when the repair block is skipped, so --check under the documented hatch exited 0 reporting a clean tree it never inspected. Worse, the test I added asserted that exit 0, so it locked the defect in rather than catching it. The hatch suppresses the repair WRITE, and --check never writes, so it always inspects now.
On the hyphenated verbs: the trailing word boundary excludes -, so listing install never reached install-test, and adding it as an alias did not add the command it aliases. Spelled out now, and I folded in the remove verbs while I was there, since npm rm in a linked worktree deletes from the primary just as effectively.
Fair hit on the missing test for the entry_name rename. It is a source assertion rather than a behavioural one, because is_merged runs before the repoint so the defect is unreachable until someone adds that call, and I checked it goes red when the shadow is put back.
Fixed in f5cfde9c. The gate now covers 35 install and remove spellings across the four managers with no false positive across 24 ordinary commands.
The false-positive class fixed for bare `yarn` last round was left standing for the other three managers, and the remove verbs widened it. The gate matched a manager+verb pair ANYWHERE in the line, so it blocked this PR's own commit subject, `grep -rn "npm ci" AGENTS.md` and `git log --grep "npm install"`. A linked worktree is the mandated working state here, so that fires on ordinary commands constantly, and a gate that cries wolf gets turned off. The command is now split on `&&`, `||`, `;`, `|`, `(` and `)`, and each segment is judged only by what it STARTS with, after leading env assignments and wrappers like `sudo` are stripped. That kills the class structurally rather than by adding another anchored special case per manager. A GLOBAL install is never blocked either. `-g` writes to the npm prefix, never through the link, and `npm update -g webjsdev` is this repo's own documented post-release step, so the gate was refusing a workflow the repo requires over a corruption that cannot occur. Docs updated for both, plus the remove verbs the last commit added silently.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, scoped to the previous fix commit. This one is the most useful of the three, because it caught that I fixed a bug narrowly instead of structurally.
Last round I re-anchored bare yarn because it matched a token anywhere rather than a command. I fixed exactly that one branch and left the identical defect standing for npm, bun and pnpm, then widened it by adding the remove verbs. So the gate blocked git commit -m "fix: npm install in a linked worktree ...", which is this PR's own commit subject, along with grep -rn "npm ci" AGENTS.md and git log --grep "npm install". My reply last round said I had verified no false positives, and that verification only covered yarn-shaped inputs, so the claim was broader than the evidence.
The reason it never bit me while working is worth writing down: the hook was added to .claude/settings.json during this session, so it is not loaded yet. It would have started firing on the next session, on ordinary commands, in the mandated working state.
Fixed structurally rather than with another per-manager anchor. The command is split on &&, ||, ;, |, ( and ), and each segment is judged only by what it STARTS with, after stripping leading env assignments and wrappers like sudo. There is no token matching left anywhere in the hook.
The global-install finding is the one I would have been most annoyed to hit later. npm update -g webjsdev is this repo's documented post-release step, and the gate was refusing it over a corruption that cannot happen, since -g writes to the npm prefix and never through the link. -g and --global are now exempt, and the exemption is narrow: the same verb without -g still blocks.
Fixed in 4c6b0945, with tests for both classes. 25 block cases and 22 allow cases now, and the allow list is the false-positive class itself rather than only the commands I expected to pass.
The env-assignment strip used an unanchored `[A-Za-z_]*=*` case glob, which matches the WHOLE segment whenever any LATER token carries an `=`. It then ate the leading words, so `npm install --omit=dev`, `npm ci --loglevel=error` and `npm install --workspace=packages/core` were all ALLOWED. That is the one direction that matters, since the gate exists to stop a write, and the last is an ordinary command in this monorepo. It also made the `--prefix=` branch of the regexes dead while the space-form test stayed green over it. The strip is token-wise now, and the assignment test is anchored to the first token alone. Walking past arbitrary tokens after a wrapper turned out to re-create the token-anywhere class one level in: with `command` and `bash` treated as wrappers, `command -v yarn` blocked and `bash -c "echo yarn"` would have. So the walk covers a wrapper's own flags and their values only, and `command`, `exec`, `bash` and `sh` are not wrappers. `bash -c "npm ci"` is a documented accepted gap rather than a parser for nested shells.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth pass, scoped to the segment-splitting rewrite, and asked specifically for false NEGATIVES. It found one, and it is the worst kind: the gate failed OPEN.
The env-assignment strip used a case glob [A-Za-z_]*=*, which I read as "first token is VAR=value". It is not anchored to a token at all: it matches the whole segment whenever ANY later token carries an =. So it ate the leading words and npm install --omit=dev, npm ci --loglevel=error and npm install --workspace=packages/core were all allowed. That last one is an ordinary command in this monorepo, and it corrupts the primary. Twelve commands that must block were passing.
It also made the --prefix= half of the regexes dead code, and the pre-existing prefix test used only the space form, so it stayed green over the dead branch. That is the second time in this PR a test has passed over the thing it was named for, which is a pattern I should have been watching for after the --check one.
Fixing it surfaced a second-order problem worth recording. My post-wrapper rule walked past ARBITRARY tokens, which re-creates the token-anywhere class one level in: with command and bash in the wrapper list, command -v yarn blocked, and bash -c "echo yarn" would have reached the bare-yarn branch too. The walk now covers a wrapper's own flags and their values only, and command, exec, bash and sh are not wrappers. bash -c "npm ci" is a documented accepted gap; parsing a nested shell is not something this hook should try to do.
Fixed in 0017635f. Both directions are now pinned by tests: the =-in-a-flag set, the --prefix= equals form, and the wrapper cases including the two that must stay allowed.
Five review rounds each found a defect in this one file, in both directions, so this replaces the accreted matcher rather than patching case seven. Recognising a package-manager invocation inside an arbitrary shell command was the whole problem. Every false positive came from quoted text and every false negative from an over-narrow token scan, so the matcher now runs in stages: quoted spans are removed FIRST, the remainder is split on separators, a segment is judged by its FIRST token, and only inside a manager-led segment are the remaining tokens scanned for the first recognised verb. Removing quoted spans first is what kills the false-positive class at the root. A manager never has its own name inside quotes, while ordinary commands carry shell metacharacters there constantly, so `git commit -m "fix: the link; npm install now blocks"` no longer reads as two commands. Scanning for the first RECOGNISED verb, with an explicit safe-verb list to stop on, is what kills the false-negative class: `npm --silent install` and `npm -w packages/core install` now block, while `npm run test -- --grep add` still does not. The scan no longer stops at the verb either, since a trailing `--prefix` decides which directory is judged. Also fixes `cd --` / `cd ~` / `pushd`, makes `yarnpkg` real rather than a token listed in one place and matched nowhere, and drops an inert quote strip. The two branches that were holding real blocks with no coverage, the manager carve-out and the git-toplevel escalation, now have tests.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fifth pass. Eight findings, and taken together they are a verdict on the approach rather than on any one of them, so I stopped patching and rebuilt the matcher.
The pattern across all five rounds is that every defect has been in this one file, and every one has been a shell-quoting or tokenising edge case. The other four layers have been clean since round 1. That is the signal: I was recognising a package-manager invocation inside an arbitrary shell command with an accreted pile of regexes, and each fix bought one more case rather than closing the class.
Both classes had a single root cause each. Every false positive came from quoted text, because splitting the raw command makes a ; or && inside a commit message look like a command boundary. Every false negative came from an over-narrow token scan. So the matcher now runs in explicit stages: quoted spans are removed FIRST, the remainder is split on separators, each segment is judged by its FIRST token, and only inside a manager-led segment are the remaining tokens scanned for the first RECOGNISED verb, with an explicit safe-verb list to stop on.
Writing the tests for this found one more myself: the scan stopped at the verb, so npm install --prefix <worktree> run from the primary was judged against the primary's own real node_modules and allowed. The scan now continues past the verb to collect the prefix.
Fixed in 92e82957. Both directions are pinned by tests, and the two branches that were holding real blocks with no coverage, the manager carve-out and the git-toplevel escalation, now have their own.
The previous commit's quote handling DELETED quoted spans, which took the path with them. `cd "<worktree>" && npm ci` left `cd` with no argument, so the install was judged against the session cwd and allowed. That is the arrival shape this hook exists for, and quoting a path is the ordinary spelling, so the rewrite regressed the headline case. Quoted spans are now neutralised rather than removed: the quote characters go, the content stays, and only the separators inside them are defused. It is a character-by-character state machine because quote nesting has to be tracked, and a single-quote sed pass running first paired the apostrophe in `can't` with the next quote in the line and swallowed a real install between them. Heredoc bodies are dropped. This repo's docs are full of `npm install` lines and a newline is a command separator here. The merged verb table is per manager again: `a` is a BUN alias, so merging it blocked `npm --workspace a run build`, and `bun upgrade` upgrades the Bun binary rather than node_modules. `npm audit fix` joins the block list on the same rationale as `link`, `rebuild` and `prune`, `--cwd` and `--dir` are mined as target directories like `--prefix`, and an informational `yarn --version` is no longer read as a bare install.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Sixth pass, and the last one this cycle runs. Ten findings, including a regression my own rewrite introduced, so I am stopping here rather than starting a seventh round.
The regression is the one that matters. My quote handling DELETED quoted spans, which took the path with them: cd "<worktree>" && npm ci left cd with no argument, so the install was judged against the session cwd and allowed. That is the arrival shape the hook exists for, and quoting a path is the ordinary spelling. I fixed a false-positive class by introducing a false negative on the headline case, which is the worse of the two directions.
Quoted spans are neutralised now rather than removed: the quote characters go, the content stays, and only the separators inside them are defused. It has to be a character-by-character state machine, because a single-quote pass running first pairs the apostrophe in can't with the next quote in the line and swallows whatever sits between, which is finding 4 and was also a live fail-open.
The rest are fixed too: heredoc bodies are dropped (this repo's docs are full of npm install lines and a newline is a separator here), the verb tables are per manager again so bun upgrade and npm --workspace a run build stop blocking, npm audit fix joins the block list on the same rationale as link / rebuild / prune, --cwd and --dir are mined as target directories, yarn --version is not read as a bare install, the test title claiming tilde coverage is corrected, and framework-dev.md now describes the matcher that actually ships.
Two deliberate calls worth stating plainly. npm r <pkg> and npm a <pkg> are NOT blocked: those one-letter aliases are rare as commands and common as flag values, and admitting them blocks npm -w a run build, so I took the false negative on the rarer spelling and wrote the gap into the hook. And bash -c "npm ci" remains unhandled, because parsing a nested shell is not something this hook should attempt.
Fixed in 41c562ca. 194 tests green, 51 command shapes verified in both directions.
I am not running another round on this. Six rounds have each found real defects in this one file while the other four layers have been clean since the first, and that is a verdict on the approach: recognising a package-manager invocation inside arbitrary shell is not converging in a bash matcher. The fixes in this commit are unreviewed, and the PR stays draft on that basis.
|
Status: stopping the review cycle unfinished, and why Six review rounds, every one finding real defects, all of them in That distribution is the finding. The prevention layer has to recognise a package-manager invocation inside an arbitrary shell command, and I have been doing it with regexes and globs in bash. Each round I fixed the case in front of me and the next round found another: quoted separators, flags before the verb, an I am not confident a seventh round would be the last, and that is the honest reason to stop rather than a rule I am following. What I think the options are, in the order I would pick them:
The branch, the commits and the card stay exactly as they are until you decide. The last commit's fixes are unreviewed. |
1 participant
Closes #1442
npm run worktree:linksymlinks a worktree'snode_modulesat the primary checkout's, which makes the worktree runnable and also puts a single writable path from every worktree into the primary's dependency tree. An install run inside a linked worktree therefore acts on the PRIMARY, and the damage lands on a session that did nothing wrong, in a place and at a time remote from the cause.What the measurements changed
The issue was filed against one mechanism (npm's workspace linker rewriting
@webjsdev/*to absolute worktree paths). That specific rewrite does not reproduce on npm 11.19.0. Three other behaviours do, and one is worse:npm installnode_modulessymlink with a real tree, silently detaching the worktreenpm ci<primary>/node_modulesbun install/bun addThe filing's headline fix, a blocking
preinstallguard, cannot work: npm removes the symlink beforepreinstallruns,npm cihas already emptied the primary by then, and Bun runs it in time but ignores a non-zero exit (measured:bun installexited 0 with apreinstallthat exited 2). So prevention moves up to aPreToolUsehook, the only layer that sees the state before the package manager starts, and thepreinstallscript stays a non-blocking reporter for every other tool. Blockingnpm installthere would also leave the worktree with no symlink and a half-built tree, which is worse than what it prevented.Layers
.claude/hooks/block-install-in-linked-worktree.shrefuses an install verb whose target directory has a symlinkednode_modules. It stays narrow:npm test,npm run <script>,npx ...,npm lsall pass, and acdout of the worktree before the verb supersedes the session cwd, so installing elsewhere from a worktree shell is not blocked. Escape hatchWEBJS_NO_WORKTREE_INSTALL_GATE=1.preinstallrunsscripts/warn-worktree-install.mjs, which ALWAYS exits 0 and returns immediately unless.gitis a FILE, so a normal clone and CI never see it.cleanup-merged-worktree.shrepoints primary@webjsdev/*links into a worktree before removing it, and drops a staging entry that would be left dangling.link-worktree-deps.mjsgains a repair pass and a read-only--check(npm run check:worktree-links). Escape hatchWEBJS_NO_WORKTREE_REPAIR=1.framework-linksdoctor check.FRAMEWORK_RESOLVEcannot see a link into a live FOREIGN checkout, because it resolves perfectly while silently running another branch's framework source.Verified against the real corruption
The primary checkout carried three dangling entries dated 1 Aug plus an absolute-but-in-primary
server.npm run worktree:linkrepaired exactly those four and left all eight LIVE.name-HASHstaging entries untouched, which is the behaviour the issue's acceptance criteria ask for.Test plan
test/hooks/block-install-in-linked-worktree.test.mjs, 9 tests. Counterfactual: a realnode_modulesdirectory allows the identical command.test/repo-health/warn-worktree-install.test.mjs, 5 tests. Counterfactual: exit 0 even on a broken gitdir pointer, so the reporter can never block an install.test/hooks/cleanup-merged-worktree.test.mjs, 5 new tests. Counterfactual run through git at0cdbc2a6: removing therepoint_primary_linkscall reds exactly the 3 discriminating tests and leaves the other 9 green.test/repo-health/link-worktree-deps.test.mjs, 13 new tests covering all three defect classes, the live-staging carve-out,--check, and the escape hatch.test/cli/doctor.test.mjs, 8 new tests. The foreign-link test assertsframeworkResolves()returns TRUE whileframework-linkswarns, which is the gap the new check fills.test/bun/listener.test.mjsfails, and it is a pre-existing linked-worktree artifact, not this branch: it passes in the primary checkout and fails identically in three unrelated worktrees on other branches.webjs checkclean onwebsite,gallery,examples/blog.webjs doctoronwebsitereports[pass] framework-links.websiteboots in prod mode with 200 on/,/docs/configuration,/ui,/ui/buttonand no broken modulepreload hints.scriptsblock literally andpackages/cli/templates/ships nopackage.json, so no app inherits the rootpreinstall).test/bun/**surface. This is repo tooling, andrequire-bun-parity-with-runtime-src.shmatches none of these paths. The one Bun-relevant behaviour, thatpreinstallfires before Bun's write, is asserted in the Node test by building the filesystem shapes directly; the reporter was also run under bun 1.3.14 directly and exits 0.Docs
AGENTS.md(the install advice at L59 and L67, a new never-install-through-the-link paragraph, the cleanup-hook paragraph, and thewebjs doctorCLI line),framework-dev.md(a new section with the measurement table and the layer map),packages/cli/AGENTS.md,.agents/skills/webjs/references/built-ins.md,website/app/docs/configuration/page.ts, and thelink-worktree-deps.mjsdocblock, which carried the advice that caused this.