Skip to content

Commit 92e8295

Browse files
committed
fix: rebuild the install matcher in explicit stages
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.
1 parent 0017635 commit 92e8295

2 files changed

Lines changed: 187 additions & 75 deletions

File tree

.claude/hooks/block-install-in-linked-worktree.sh

Lines changed: 108 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,28 @@
1515
# manager starts. `scripts/warn-worktree-install.mjs` covers every other tool by
1616
# reporting rather than blocking.
1717
#
18-
# ## It matches a COMMAND, never a token
18+
# ## How the matching works, and why it is shaped this way
1919
#
20-
# The command is split on `&&`, `||`, `;`, `|`, `(` and `)`, and each segment is
21-
# judged only by what it STARTS with. Matching the manager token anywhere in the
22-
# line instead is the obvious shortcut and it is badly wrong: it blocks
23-
# `git commit -m "fix: npm install ..."`, `grep -rn "npm ci" AGENTS.md` and
24-
# `git log --grep "npm install"`. Every worktree here is a linked worktree, so
25-
# that fires on ordinary commands constantly, and a gate that cries wolf is a
26-
# gate someone turns off.
20+
# Recognising a package-manager invocation inside an arbitrary shell command is
21+
# the hard part of this hook, and getting it wrong in either direction is
22+
# expensive: a false NEGATIVE lets the corruption through, and a false POSITIVE
23+
# fires on ordinary commands in a linked worktree, which is the mandated working
24+
# state here, until someone turns the gate off. Four passes of review found a
25+
# defect in each direction, so the matcher is built in explicit stages:
2726
#
28-
# The predicate runs against the COMMAND's target directory, not just the session
29-
# cwd: the harness resets cwd to the primary checkout between commands, so a real
30-
# install in a worktree arrives as `cd /path/to/worktree && npm ci`.
27+
# 1. QUOTED SPANS ARE REMOVED FIRST. A manager invocation never has its own
28+
# name inside quotes, while ordinary commands carry shell metacharacters
29+
# there constantly. Splitting the raw string instead makes
30+
# `git commit -m "fix: the link; npm install now blocks"` look like two
31+
# commands, the second an install.
32+
# 2. The remainder is split on `&&`, `||`, `;`, `|`, `(`, `)` and newlines.
33+
# 3. Each segment is judged by its FIRST token, after leading env assignments
34+
# and wrappers are stripped. A segment that does not START with a package
35+
# manager is not an install, whatever else it contains.
36+
# 4. Only inside a manager-led segment are the remaining tokens scanned, and
37+
# the FIRST one recognised as either an install verb or a known safe verb
38+
# decides. Anything unrecognised is skipped rather than assumed, so a flag
39+
# VALUE (`npm -w packages/core install`) does not hide the verb behind it.
3140
#
3241
# Contract: exit 0 = allow, exit 2 = block (message on stderr).
3342
# Escape hatch: WEBJS_NO_WORKTREE_INSTALL_GATE=1.
@@ -39,53 +48,35 @@ input=$(cat)
3948
cmd=$(printf '%s' "$input" | jq -r '.tool_input.command // empty' 2>/dev/null || true)
4049
if [ -z "$cmd" ]; then exit 0; fi
4150

42-
# Every manager's documented install ALIASES, not just its canonical spelling.
43-
# The gate is worthless if `bun i` walks past it, and Bun is the manager that
44-
# writes THROUGH the symlink into the primary rather than replacing it.
45-
#
46-
# HYPHENATED verbs are spelled out: the trailing word boundary excludes `-`, so
47-
# `install-test` is NOT reached by listing `install`, and the short aliases
48-
# (`it`, `cit`, `sit`) do not cover the long spellings.
49-
#
50-
# The REMOVE verbs are here too. `npm rm <pkg>` in a linked worktree deletes from
51-
# the checkout that owns the tree, the same corruption in the other direction.
52-
NPM_VERBS='install-ci-test|clean-install-test|install-clean|clean-install|install-test|install|isntall|isntal|isnta|isnt|instal|insta|inst|ins|in|i|add|ci|cit|sit|it|ic|update|upgrade|udpate|up|dedupe|ddp|uninstall|unlink|un|remove|rm|r'
53-
BUN_VERBS='install|i|add|a|update|up|remove|rm'
54-
PNPM_VERBS='install|i|add|update|upgrade|up|dedupe|remove|rm|uninstall|un'
55-
YARN_VERBS='install|add|upgrade|up|dedupe|remove'
56-
# `npm --prefix <dir> install` puts flags BEFORE the verb. Only the two flags that
57-
# themselves name a target directory are admitted, so this stays targeted rather
58-
# than swallowing a token run and matching `npm run install`.
59-
SEG_VERBS="^(npm[[:space:]]+(${NPM_VERBS})|bun[[:space:]]+(${BUN_VERBS})|pnpm[[:space:]]+(${PNPM_VERBS})|yarn[[:space:]]+(${YARN_VERBS})|(npm|pnpm|yarn)[[:space:]]+(--prefix|-C)[[:space:]=]+[^[:space:]]+[[:space:]]+(${NPM_VERBS}))([^[:alnum:]_-]|\$)"
60-
# Bare `yarn` IS an install in yarn classic, but only when it is the whole
61-
# command: `yarn test` is not one.
62-
SEG_BARE_YARN='^yarn([[:space:]]+-[^[:space:]]*)*[[:space:]]*$'
51+
# Verbs that WRITE to node_modules. Every manager's documented aliases, because
52+
# a gate `bun i` walks past is worthless and Bun is the manager that writes
53+
# THROUGH the link rather than replacing it. `link`, `rebuild` and `prune` are
54+
# here for the same reason as the remove verbs: they all mutate the tree that
55+
# the symlink points at.
56+
INSTALL_VERBS='install-ci-test|clean-install-test|install-clean|clean-install|install-test|install|isntall|isntal|isnta|isnt|instal|insta|inst|ins|in|i|add|a|ci|cit|sit|it|ic|update|upgrade|udpate|up|dedupe|ddp|uninstall|unlink|un|remove|rm|r|link|ln|rebuild|rb|prune'
57+
# Verbs that do NOT touch node_modules. Listed explicitly so the scan can STOP:
58+
# without them, `npm run test -- --grep add` would keep scanning and hit `add`.
59+
SAFE_VERBS='run|run-script|rum|urn|test|tst|t|start|stop|restart|exec|x|ls|list|la|ll|init|innit|create|publish|pack|version|view|v|info|show|why|ping|config|c|get|set|docs|home|repo|bugs|audit|fund|outdated|prefix|root|bin|whoami|token|team|org|access|star|unstar|search|s|se|find|help|doctor|explain|edit|deprecate|dist-tag|hook|login|logout|adduser|owner|profile|shrinkwrap|unpublish|completion|diff|query|sbom'
6360
# A GLOBAL install writes to the npm prefix, never through the local link, and
6461
# `npm update -g webjsdev` is this repo's documented post-release step.
6562
GLOBAL='(^|[[:space:]])(-g|--global)([[:space:]]|$)'
6663

67-
# Walk the segments in order so a `cd` earlier in the line moves the target the
68-
# way the shell would.
64+
# STAGE 1: remove quoted spans. See the docblock; this is what keeps a shell
65+
# metacharacter inside a commit message from being read as a command boundary.
66+
scrubbed=$(printf '%s' "$cmd" | sed -e "s/'[^']*'/ /g" -e 's/"[^"]*"/ /g')
67+
6968
eff="$PWD"
7069
target=""
70+
# STAGE 2: split into segments. `cd` in an earlier segment moves the target the
71+
# way the shell would.
7172
while IFS= read -r seg; do
72-
# Quotes are dropped so a quoted command body tokenizes (`bash -c "npm ci"`).
73-
# This is safe: what keeps `git commit -m "npm ci"` out is the command-position
74-
# rule below, never the quoting.
75-
seg=$(printf '%s' "$seg" | tr -d '"'"'"'')
7673
seg="${seg#"${seg%%[![:space:]]*}"}"
7774
[ -z "$seg" ] && continue
7875

79-
# Strip leading env assignments and benign wrappers, so `FOO=1 npm ci`,
80-
# `sudo npm ci` and `bash -c "npm ci"` are still judged on the manager that
81-
# follows them.
82-
#
83-
# This is done TOKEN BY TOKEN, and the env-assignment test is anchored to the
84-
# first token alone. An unanchored `[A-Za-z_]*=*` case glob matches the WHOLE
85-
# segment whenever any LATER token contains `=`, so it ate leading words and
86-
# `npm install --omit=dev`, `npm ci --loglevel=error` and
87-
# `npm install --workspace=packages/core` all failed OPEN. Failing open is the
88-
# one direction that matters here, since the whole point is to stop a write.
76+
# STAGE 3: strip leading env assignments and benign wrappers, token by token.
77+
# The assignment test is anchored to the FIRST token: an unanchored
78+
# `[A-Za-z_]*=*` glob matches the whole segment whenever any LATER token
79+
# carries an `=`, which silently disabled the gate for `npm install --omit=dev`.
8980
wrapper_seen=0
9081
prev_flag=0
9182
for _ in 1 2 3 4 5 6 7 8 9 10; do
@@ -94,25 +85,21 @@ while IFS= read -r seg; do
9485
strip=0
9586
case "$first" in
9687
*=*)
97-
# A real env assignment: NAME=..., NAME being a valid shell identifier.
9888
name="${first%%=*}"
9989
case "$name" in
10090
''|*[!A-Za-z0-9_]*|[0-9]*) ;;
10191
*) strip=1 ;;
10292
esac ;;
10393
sudo|env|time|nice) wrapper_seen=1; prev_flag=0; strip=1 ;;
10494
npm|bun|pnpm|yarn|yarnpkg) ;;
105-
-*)
106-
# A wrapper's OWN flag, e.g. `sudo -u foo npm ci`.
107-
[ "$wrapper_seen" = "1" ] && { prev_flag=1; strip=1; } ;;
95+
-*) [ "$wrapper_seen" = "1" ] && { prev_flag=1; strip=1; } ;;
10896
*)
109-
# The VALUE of the wrapper flag just stripped, e.g. the `foo` in `-u foo`
110-
# or the `10` in `nice -n 10`. Deliberately narrow: walking past ARBITRARY
111-
# tokens after a wrapper re-creates the token-anywhere class one level in,
112-
# where `bash -c "echo yarn"` would reach the bare-yarn branch and block.
113-
# `command`, `exec`, `bash` and `sh` are NOT wrappers here for that reason,
114-
# so `command -v yarn` stays allowed and `bash -c "npm ci"` is a known,
115-
# accepted gap rather than a parser for nested shells.
97+
# The VALUE of a wrapper flag (`sudo -u foo`, `nice -n 10`). Deliberately
98+
# narrow: walking past ARBITRARY tokens after a wrapper re-creates the
99+
# token-anywhere class one level in, where `bash -c "echo yarn"` reaches
100+
# the bare-yarn branch. `command`, `exec`, `bash` and `sh` are NOT
101+
# wrappers for that reason, so `command -v yarn` stays allowed and
102+
# `bash -c "npm ci"` is an accepted gap rather than a nested-shell parser.
116103
if [ "$wrapper_seen" = "1" ] && [ "$prev_flag" = "1" ]; then prev_flag=0; strip=1; fi ;;
117104
esac
118105
[ "$strip" = "1" ] || break
@@ -122,31 +109,78 @@ while IFS= read -r seg; do
122109
done
123110
[ -n "$seg" ] || continue
124111

125-
case "$seg" in
126-
cd|cd\ *)
127-
d="${seg#cd}"; d="${d#"${d%%[![:space:]]*}"}"; d="${d%% *}"
128-
d=$(printf '%s' "$d" | tr -d "\"'")
112+
set -- $seg
113+
head_tok="$1"
114+
115+
# `cd` / `pushd` move the effective directory.
116+
case "$head_tok" in
117+
cd|pushd)
118+
shift
119+
# Skip `--` and any option, so `cd -- <dir>` and `cd -P <dir>` both work.
120+
while [ $# -gt 0 ]; do
121+
case "$1" in --) shift; break ;; -*) shift ;; *) break ;; esac
122+
done
123+
d="${1:-}"
129124
case "$d" in
130125
'') ;;
126+
'~') eff="$HOME" ;;
127+
'~/'*) eff="$HOME/${d#'~/'}" ;;
131128
/*) eff="$d" ;;
132-
'~'*) ;;
133129
*) eff="$eff/$d" ;;
134130
esac
135131
continue ;;
136132
esac
137133

138-
if printf '%s' "$seg" | grep -Eq "$SEG_VERBS" || printf '%s' "$seg" | grep -Eq "$SEG_BARE_YARN"; then
139-
printf '%s' "$seg" | grep -Eq "$GLOBAL" && continue
140-
target="$eff"
141-
# An explicit --prefix / -C on the install itself wins over the cwd.
142-
p=$(printf '%s' "$seg" | grep -oE '(^|[[:space:]])(-C|--prefix)[[:space:]=]+[^[:space:]]+' | sed -E 's/.*(-C|--prefix)[[:space:]=]+//' | tr -d "\"'" | head -1)
143-
if [ -n "$p" ]; then
144-
case "$p" in /*) target="$p" ;; '~'*) ;; *) target="$eff/$p" ;; esac
145-
fi
146-
break
134+
# STAGE 4: only a manager-led segment can be an install.
135+
case "$head_tok" in
136+
npm|bun|pnpm|yarn|yarnpkg) ;;
137+
*) continue ;;
138+
esac
139+
shift
140+
141+
# A global install never touches this tree.
142+
printf '%s' "$seg" | grep -Eq "$GLOBAL" && continue
143+
144+
verdict=""
145+
prefix_dir=""
146+
pending_prefix=0
147+
while [ $# -gt 0 ]; do
148+
tok="$1"; shift
149+
case "$tok" in
150+
--prefix=*|-C=*) prefix_dir="${tok#*=}"; continue ;;
151+
--prefix|-C) pending_prefix=1; continue ;;
152+
-*) continue ;;
153+
esac
154+
if [ "$pending_prefix" = "1" ]; then prefix_dir="$tok"; pending_prefix=0; continue; fi
155+
# The first token recognised either way decides; anything else is a flag
156+
# value or a package name and is skipped rather than assumed.
157+
# Do NOT stop at the verb: `--prefix` may still be ahead of us, and
158+
# `npm install --prefix <worktree>` run from the primary would otherwise be
159+
# judged against the primary's own real node_modules and allowed. The FIRST
160+
# verdict wins; later tokens are only mined for the prefix.
161+
[ -n "$verdict" ] && continue
162+
if printf '%s' "$tok" | grep -Eq "^(${INSTALL_VERBS})$"; then verdict="install"; continue; fi
163+
if printf '%s' "$tok" | grep -Eq "^(${SAFE_VERBS})$"; then verdict="safe"; continue; fi
164+
done
165+
166+
# A bare `yarn` (only flags, no verb) IS an install in yarn classic.
167+
if [ -z "$verdict" ]; then
168+
case "$head_tok" in yarn|yarnpkg) verdict="install" ;; esac
169+
fi
170+
[ "$verdict" = "install" ] || continue
171+
172+
target="$eff"
173+
if [ -n "$prefix_dir" ]; then
174+
case "$prefix_dir" in
175+
'~') target="$HOME" ;;
176+
'~/'*) target="$HOME/${prefix_dir#'~/'}" ;;
177+
/*) target="$prefix_dir" ;;
178+
*) target="$eff/$prefix_dir" ;;
179+
esac
147180
fi
181+
break
148182
done <<EOF
149-
$(printf '%s' "$cmd" | tr '&|;()' '\n\n\n\n\n')
183+
$(printf '%s' "$scrubbed" | tr '&|;()' '\n\n\n\n\n')
150184
EOF
151185

152186
[ -n "$target" ] || exit 0

test/hooks/block-install-in-linked-worktree.test.mjs

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// (`npm test`, `npm run <script>`, `npx ...`) have to pass.
66
import { test } from 'node:test';
77
import assert from 'node:assert/strict';
8-
import { spawnSync } from 'node:child_process';
8+
import { spawnSync, execSync } from 'node:child_process';
99
import { mkdtempSync, mkdirSync, rmSync, symlinkSync, writeFileSync } from 'node:fs';
1010
import { join, dirname, resolve } from 'node:path';
1111
import { tmpdir } from 'node:os';
@@ -131,6 +131,14 @@ test('judges a COMMAND, never a token that merely appears in the line', () => {
131131
'echo "the hook blocks npm rm x too"',
132132
'rg "bun add" .',
133133
'cat README | grep yarn',
134+
// A shell metacharacter INSIDE the quoted text. Splitting the raw command
135+
// makes the tail look like its own command, so each of these read as an
136+
// install. Quoted spans are removed before the split for exactly this.
137+
'git commit -m "fix: guard the link; npm install now blocks"',
138+
'git commit -m "fix: cd wt && npm ci corrupts the primary"',
139+
'gh pr create --body "| npm install | replaces the symlink |"',
140+
'echo "run (npm ci) to reproduce"',
141+
"git log --grep 'npm install'",
134142
]) {
135143
assert.equal(runHook(cmd, worktree).status, 0, `expected allow for \`${cmd}\``);
136144
}
@@ -279,3 +287,73 @@ test('honours WEBJS_NO_WORKTREE_INSTALL_GATE=1', () => {
279287
assert.equal(r.status, 0, `expected allow, got ${r.status}: ${r.stderr}`);
280288
} finally { rmSync(root, { recursive: true, force: true }); }
281289
});
290+
291+
test('sees the verb through flags that sit BEFORE it', () => {
292+
// `npm --silent install` and `npm -w packages/core install` are ordinary
293+
// spellings. Only `--prefix` / `-C` used to be admitted in the pre-verb
294+
// position, so every other flag hid the verb and the gate failed open.
295+
const { root, worktree } = makeLinkedPair();
296+
try {
297+
for (const cmd of [
298+
'npm --silent install', 'npm -s ci', 'npm --ignore-scripts ci',
299+
'npm -w packages/core install', 'npm --workspace packages/core install',
300+
'npm --no-audit install', 'npm --prefer-offline ci',
301+
'pnpm -r install', 'pnpm --filter core install', 'bun --cwd . install',
302+
]) {
303+
assert.equal(runHook(cmd, worktree).status, 2, `expected block for \`${cmd}\``);
304+
}
305+
} finally { rmSync(root, { recursive: true, force: true }); }
306+
});
307+
308+
test('a flag VALUE never hides the verb, and a safe verb still stops the scan', () => {
309+
const { root, worktree } = makeLinkedPair();
310+
try {
311+
// `packages/core` is neither an install verb nor a safe one, so the scan
312+
// continues and finds `install`.
313+
assert.equal(runHook('npm -w packages/core install', worktree).status, 2);
314+
// ...but `run` IS a safe verb, so the scan stops there and never reaches
315+
// the `add` further along.
316+
assert.equal(runHook('npm run test -- --grep add', worktree).status, 0);
317+
} finally { rmSync(root, { recursive: true, force: true }); }
318+
});
319+
320+
test('follows every `cd` spelling, including ~, --, and pushd', () => {
321+
const { root, worktree } = makeLinkedPair();
322+
try {
323+
for (const cmd of [`cd -- ${worktree} && npm ci`, `pushd ${worktree} && npm ci`, `cd -P ${worktree} && npm ci`]) {
324+
assert.equal(runHook(cmd, root).status, 2, `expected block for \`${cmd}\``);
325+
}
326+
} finally { rmSync(root, { recursive: true, force: true }); }
327+
});
328+
329+
test('treats `yarnpkg` as yarn, since it is declared a manager token', () => {
330+
const { root, worktree } = makeLinkedPair();
331+
try {
332+
for (const cmd of ['yarnpkg install', 'yarnpkg add x', 'yarnpkg']) {
333+
assert.equal(runHook(cmd, worktree).status, 2, `expected block for \`${cmd}\``);
334+
}
335+
} finally { rmSync(root, { recursive: true, force: true }); }
336+
});
337+
338+
test('the manager carve-out in the strip loop is load-bearing', () => {
339+
// A BOOLEAN wrapper flag followed directly by the manager. Without the
340+
// `npm|bun|pnpm|yarn|yarnpkg)` case in the strip loop, `npm` is taken for the
341+
// value of `-E` and stripped, leaving `ci` as the head token and no block.
342+
const { root, worktree } = makeLinkedPair();
343+
try {
344+
assert.equal(runHook('sudo -E npm ci', worktree).status, 2);
345+
assert.equal(runHook('sudo -n npm install', worktree).status, 2);
346+
} finally { rmSync(root, { recursive: true, force: true }); }
347+
});
348+
349+
test('escalates to the git toplevel, so an install from a SUBDIRECTORY blocks', () => {
350+
// The install lands at the package root. `makeLinkedPair` builds no git repo,
351+
// so the toplevel term was never exercised by any other test here.
352+
const { root, worktree } = makeLinkedPair();
353+
try {
354+
execSync('git init -q -b main', { cwd: worktree, stdio: 'pipe' });
355+
const sub = join(worktree, 'packages', 'core');
356+
mkdirSync(sub, { recursive: true });
357+
assert.equal(runHook('npm install', sub).status, 2, 'a subdirectory install still reaches the linked root');
358+
} finally { rmSync(root, { recursive: true, force: true }); }
359+
});

0 commit comments

Comments
 (0)