fix(worktree): npm install only where npm is the package manager (VST-340) - #1451
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved 66c99d4 Bug fix with clear scope: prevents npm install from running in non-npm workspaces (pnpm/yarn/bun). Changes are well-tested with 11 test scenarios covering edge cases. The review comment about unreadable-pin handling was addressed in this commit. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates worktree create JS dependency installation to avoid running npm install in non-npm workspaces (pnpm/yarn/bun), and adds coverage to prevent regressions.
Changes:
- Skip background
npm installwhen a pnpm/yarn/bun lockfile exists orpackageManagerpins pnpm/yarn/bun - Improve failure visibility for
npm installby writing output to a temp log and printing its path on failure - Add a bash test to verify npm install is skipped/ran in the expected repository setups
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/size-ratchet-baseline.tsv | Updates the ratchet baseline for the worktree script size change. |
| skills/worktree/tests/worktree_js_dependency_install.sh | Adds test coverage for conditional npm installation behavior in worktree create. |
| skills/worktree/scripts/worktree | Implements conditional npm install logic and failure logging for JS worktrees. |
| CHANGELOG.md | Documents the updated worktree dependency-install behavior and failure visibility. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b176684310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Dismissing prior approval to re-evaluate c025a4a
Dismissing prior approval to re-evaluate b041561
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
skills/worktree/scripts/worktree:1745
- The npm
packageManagerdetection only matches"npm@", so"packageManager": "npm"(no version) won’t be treated as explicit npm evidence and can incorrectly skipnpm installwhen a foreign lockfile exists. Consider matching"npm"with an optional@...(and similarly anchoring the regex to the key position, e.g.^[[:space:]]*"packageManager"), so JSON strings elsewhere don’t accidentally satisfy the pattern.
if [[ ! -f "$wt/package-lock.json" ]] &&
! grep -Eq '"packageManager"[[:space:]]*:[[:space:]]*"npm@' "$wt/package.json" 2>/dev/null; then
if [[ -f "$wt/pnpm-lock.yaml" || -f "$wt/yarn.lock" || -f "$wt/bun.lockb" || -f "$wt/bun.lock" ]]; then
return 0
fi
if grep -Eq '"packageManager"[[:space:]]*:[[:space:]]*"(pnpm|yarn|bun)' "$wt/package.json" 2>/dev/null; then
return 0
skills/worktree/tests/worktree_js_dependency_install.sh:61
wait_for_installsalways returns success even on timeout, which makes failures harder to diagnose and forces callers to add extrasleepcalls plus ad-hoc greps later. Consider returning non-zero on timeout (and having callers fail with a clear message), or renaming the helper to reflect that it is a best-effort delay rather than a wait with a contract.
wait_for_installs() {
# The npm path is backgrounded; give it a moment to land.
local deadline=$((SECONDS + 5))
while [ "$SECONDS" -lt "$deadline" ]; do
[ -s "$NPM_CALL_LOG" ] && return 0
sleep 0.2
done
return 0
}
skills/worktree/tests/worktree_js_dependency_install.sh:6
- The opening comment is a bit ambiguous (“create runs …”) without naming the command/script under test. Suggest rephrasing to explicitly reference the
worktree createcommand (and optionally theinstall_worktree_dependenciesbehavior) so future readers immediately understand what “create” refers to.
# create runs npm install only where npm is genuinely the package manager.
# In a pnpm/yarn/bun workspace the historical unconditional `npm install`
# resolved the wrong tree and wrote a stray package-lock.json into the fresh
# checkout — dirty from birth, and `git add -A` would commit an npm lockfile
# into a pnpm monorepo.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
skills/worktree/scripts/worktree:1750
yarn.lockis treated as definitive evidence to skipnpm installunless there is an npm lockfile or an explicit npmpackageManagerpin. This will incorrectly skip installs for npm-based repos that don’t commitpackage-lock.jsonbut happen to include ayarn.lock(e.g., leftover or tooling-generated). Consider making the skip decision require positive evidence of yarn usage (e.g.,packageManager: "yarn@...",.yarnrc.yml,.yarn/, or similar) rather thanyarn.lockalone, or treatyarn.lockas “weak evidence” that doesn’t block npm unless other yarn indicators are present.
if [[ ! -f "$wt/package-lock.json" ]] &&
! grep -Eq '"packageManager"[[:space:]]*:[[:space:]]*"npm@' <<<"$pkg_flat"; then
if [[ -f "$wt/pnpm-lock.yaml" || -f "$wt/yarn.lock" || -f "$wt/bun.lockb" || -f "$wt/bun.lock" ]]; then
return 0
fi
skills/worktree/tests/worktree_js_dependency_install.sh:74
- The production
worktreescript backgrounds thenpm install, so limiting the “no npm invocation” assertion window to 2 seconds can produce false positives if the backgrounded call happens after the 2-second window. To make this test reliably detect unwanted npm usage, increase the observation window (e.g., 5–10 seconds) and/or add a second check after a short delay to catch late invocations.
assert_log_stays_empty() { # NAME — watch 2s and fail the moment npm logs
local name="$1" deadline=$((SECONDS + 2))
while [ "$SECONDS" -lt "$deadline" ]; do
if [ -s "$NPM_CALL_LOG" ]; then
bad "$name" "$(cat "$NPM_CALL_LOG")"
return 1
fi
sleep 0.2
done
ok "$name"
}
skills/worktree/tests/worktree_js_dependency_install.sh:61
wait_for_installsalways returns success, even after timing out without observing any npm invocation. This makes failures harder to diagnose because callers can’t distinguish “npm didn’t run yet” from “npm never ran.” Consider returning non-zero on timeout and having callers surface a clearer failure message when the expected npm invocation hasn’t happened within the deadline.
wait_for_installs() {
# The npm path is backgrounded; give it a moment to land.
local deadline=$((SECONDS + 5))
while [ "$SECONDS" -lt "$deadline" ]; do
[ -s "$NPM_CALL_LOG" ] && return 0
sleep 0.2
done
return 0
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0415610d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Merge queue ejected this PR ( Ejecting merge-group run: not identified Failing job(s): No same-named check comparison available for the PR head. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
Dismissing prior approval to re-evaluate f9b37c7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
skills/worktree/tests/worktree_js_dependency_install.sh:85
- The test reuses a single shared
$NPM_CALL_LOGacross multiple scenarios without clearing it between cases whereassert_log_stays_emptyis used. If a prior scenario logs unexpectedly, subsequent scenarios will fail immediately and can report earlier log lines, making failures harder to attribute to the specific case. Consider truncating: >\"$NPM_CALL_LOG\"before each scenario (right before invokingworktree create) so each assertion observes only the current run’s npm activity.
echo "=== a pnpm workspace never gets an npm install ==="
ROOT="$TMP_ROOT/pnpm"
make_repo "$ROOT" repo
printf '{ "name": "app", "devDependencies": {} }\n' >"$ROOT/repo/package.json"
printf 'lockfileVersion: "9.0"\n' >"$ROOT/repo/pnpm-lock.yaml"
git -C "$ROOT/repo" add package.json pnpm-lock.yaml
git -C "$ROOT/repo" commit -q -m "js: pnpm workspace"
git -C "$ROOT/repo" push -q origin main
(cd "$ROOT/repo" && "$WORKTREE_SCRIPT" create issue-pnpm >/dev/null)
assert_log_stays_empty "pnpm worktree skipped npm" || true
skills/worktree/tests/worktree_js_dependency_install.sh:90
- The test reuses a single shared
$NPM_CALL_LOGacross multiple scenarios without clearing it between cases whereassert_log_stays_emptyis used. If a prior scenario logs unexpectedly, subsequent scenarios will fail immediately and can report earlier log lines, making failures harder to attribute to the specific case. Consider truncating: >\"$NPM_CALL_LOG\"before each scenario (right before invokingworktree create) so each assertion observes only the current run’s npm activity.
echo "=== a packageManager pin skips npm even without a lockfile ==="
skills/worktree/tests/worktree_js_dependency_install.sh:100
- The test reuses a single shared
$NPM_CALL_LOGacross multiple scenarios without clearing it between cases whereassert_log_stays_emptyis used. If a prior scenario logs unexpectedly, subsequent scenarios will fail immediately and can report earlier log lines, making failures harder to attribute to the specific case. Consider truncating: >\"$NPM_CALL_LOG\"before each scenario (right before invokingworktree create) so each assertion observes only the current run’s npm activity.
(cd "$ROOT/repo" && "$WORKTREE_SCRIPT" create issue-pin >/dev/null)
assert_log_stays_empty "packageManager pin skipped npm" || true
echo "=== a pin formatted across lines is still a pin ==="
skills/worktree/tests/worktree_js_dependency_install.sh:108
- The test reuses a single shared
$NPM_CALL_LOGacross multiple scenarios without clearing it between cases whereassert_log_stays_emptyis used. If a prior scenario logs unexpectedly, subsequent scenarios will fail immediately and can report earlier log lines, making failures harder to attribute to the specific case. Consider truncating: >\"$NPM_CALL_LOG\"before each scenario (right before invokingworktree create) so each assertion observes only the current run’s npm activity.
(cd "$ROOT/repo" && "$WORKTREE_SCRIPT" create issue-multiline >/dev/null)
assert_log_stays_empty "a multiline packageManager pin skips npm" || true
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9b37c70a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
… stale lockfiles Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
…ing is disabled Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
…space.yaml is foreign evidence Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
15c47ae to
bc18fde
Compare
Dismissing prior approval to re-evaluate bc18fde
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
skills/worktree/tests/worktree_js_dependency_install.sh:61
wait_for_installsalways returns success (even when no log entry appears), and several assertions rely on a fixedsleep 1before grepping for a specific worktree name. This can make CI flaky on slower runners (backgrounded install/log write may land after the fixed sleep). Prefer a bounded poll that waits until either the expected marker appears in$NPM_CALL_LOGor a timeout is hit, and fail on timeout so the test outcome is deterministic.
wait_for_installs() {
# The npm path is backgrounded; give it a moment to land.
local deadline=$((SECONDS + 5))
while [ "$SECONDS" -lt "$deadline" ]; do
[ -s "$NPM_CALL_LOG" ] && return 0
sleep 0.2
done
return 0
}
skills/worktree/scripts/worktree:1744
- This function now combines (1) package-manager detection, (2) precedence rules, and (3) secure log-dir setup into one large block. To make future changes safer and easier to test/review, consider extracting small helpers (e.g.,
read_package_manager_pin,should_run_npm_install,npm_install_with_failure_log) and keepinginstall_worktree_dependenciesas orchestration.
[[ -f "$wt/package.json" ]] || return 0
# npm is wrong for a workspace that pins another package manager: it
# resolves a different tree and writes a stray package-lock.json into the
# fresh checkout (dirty from birth, and `git add -A` would commit an npm
# lockfile into a pnpm monorepo). Explicit npm evidence — its own lockfile
# or an npm packageManager pin — wins over an incidental foreign lockfile;
# otherwise another manager's lockfile or pin means that repo provisions
# its own dependencies, so skip.
# The TOP-LEVEL packageManager field only — a nested look-alike (e.g.
# config.packageManager) is not a pin. Structural readers in preference
# order: node (present wherever a package.json matters), then jq; a
# machine with neither reads no pin, and the only cost of that miss is an
# automatic install skipped — never a wrong-tree install.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc18fde184
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…ign-evidence gate npm tolerates a UTF-8 BOM, so the reader strips it before parsing; a manifest the reader cannot parse reads no pin, and any foreign evidence then skips the install regardless of a stale npm lockfile. Claude-Session: https://claude.ai/code/session_01EwxSRv8oy1NxoQm66WKa4J
Dismissing prior approval to re-evaluate 66c99d4
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
skills/worktree/tests/worktree_js_dependency_install.sh:124
- This test (and a few similar ones) implicitly requires that the system running the tests has either
nodeorjqavailable, because theworktreescript’s pin detection uses those tools; if neither is present, the script treats the manifest as unreadable and will skip installs unless it sees unambiguous npm evidence, making these assertions fail in minimal CI images. Recommendation: add an explicit preflight at the top of the test to skip with a clear message when neithernodenorjqis available, or adjust expectations in the specific cases where the behavior intentionally differs without a structural reader.
echo "=== a nested config.packageManager is not a pin ==="
ROOT="$TMP_ROOT/nested"
make_repo "$ROOT" repo
printf '{ "name": "app", "config": { "packageManager": "pnpm@10.33.2" } }\n' >"$ROOT/repo/package.json"
git -C "$ROOT/repo" add package.json
git -C "$ROOT/repo" commit -q -m "js: nested lookalike"
git -C "$ROOT/repo" push -q origin main
(cd "$ROOT/repo" && "$WORKTREE_SCRIPT" create issue-nested >/dev/null)
wait_for_installs
sleep 1
if grep -q "issue-nested" "$NPM_CALL_LOG" 2>/dev/null; then
ok "a nested config.packageManager still gets the npm install"
else
bad "nested lookalike not a pin" "log: $(cat "$NPM_CALL_LOG" 2>/dev/null)"
fi
CHANGELOG.md:18
- The updated gating logic also treats
pnpm-workspace.yamlas foreign evidence (skips npm even without a lockfile), and treatsnpm-shrinkwrap.jsonas explicit npm evidence. To avoid surprising users, it would help to mention the workspace-manifest case (and optionally shrinkwrap precedence) here so the changelog reflects the full set of signals the script uses.
- worktree: `create` installs npm dependencies only where npm is the
package manager — a pnpm/yarn/bun lockfile or `packageManager` pin skips
the step, so a fresh worktree in a pnpm workspace no longer starts dirty
with a stray `package-lock.json`; a failed npm install names its log
instead of vanishing (VST-340).
skills/worktree/scripts/worktree:1749
- This
node -eone-liner is doing several concerns at once (BOM stripping, parsing, top-level field selection, output normalization). It’s correct but difficult to audit/modify safely. Consider extracting this into a small helper function (still using node) written across multiple lines (with explicit error handling) so future changes to pin semantics are less error-prone.
pkg_manager="$(node -e 'const m=JSON.parse(require("fs").readFileSync(process.argv[1],"utf8").replace(/^\uFEFF/,"")).packageManager;process.stdout.write(typeof m==="string"?m:"")' "$wt/package.json" 2>/dev/null)" || pkg_manager_readable=0
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66c99d4d6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
skills/worktree/tests/worktree_js_dependency_install.sh:61
wait_for_installsreturns success even if it times out without any npm invocation being logged, which can make the 'should install' assertions flaky (false negatives). Consider returning non-zero on timeout and letting the caller record a failure (or converting the helper into anassert_install_happenedthat fails with a useful message when the log stays empty).
wait_for_installs() {
# The npm path is backgrounded; give it a moment to land.
local deadline=$((SECONDS + 5))
while [ "$SECONDS" -lt "$deadline" ]; do
[ -s "$NPM_CALL_LOG" ] && return 0
sleep 0.2
done
return 0
}
skills/worktree/scripts/worktree:1763
- This only treats
packageManagerpins as authoritative when they include an@(e.g.npm@...). If a repo uses\"packageManager\": \"npm\"(no version), it will fall through into the lockfile heuristics and may incorrectly skip the historical npm install when a foreign lockfile exists. To make behavior robust, handle the no-version forms too (e.g.npm|npm@*, and similarly forpnpm|pnpm@*,yarn|yarn@*,bun|bun@*).
case "$pkg_manager" in
pnpm@*|yarn@*|bun@*) return 0 ;;
npm@*) ;;
*)
skills/worktree/scripts/worktree:1763
- The new explicit
packageManagerrouting (especially thenpm@*branch) is behaviorally important but isn't directly asserted in the added test script (current 'npm evidence wins' coverage is via lockfiles/shrinkwrap). Add a test case wherepackageManageris explicitlynpm@...and a foreign lockfile (e.g.yarn.lock) is present, and assert that npm still runs.
case "$pkg_manager" in
pnpm@*|yarn@*|bun@*) return 0 ;;
npm@*) ;;
skills/worktree/README.md:15
- The doc says npm installs only when there's "no other manager's lockfile" (and no non-npm pin), but the implementation also treats
pnpm-workspace.yamlas foreign evidence even without a lockfile. To keep the README accurate, include workspace manifests (at leastpnpm-workspace.yaml) in the set of signals that suppress npm installs.
Worktrees are created under `<parent-of-checkout>/.worktrees/<checkout-name>/` — beside the checkout, not inside it, so editor file watchers never ingest worktree build outputs and sibling repos cannot collide. The default branch comes from `origin/HEAD` (fallback `main`). After creation the configured symlinks, copies, and scratch directories are applied. JS dependencies install automatically only where npm is the package manager (a `package.json` with no other manager's lockfile and no non-npm `packageManager` pin; explicit npm evidence wins over an incidental foreign lockfile) — pnpm/yarn/bun workspaces provision their own dependencies. The install runs in the background with every std fd detached (a `$(create)` capture returns at once); a failure leaves its full log under the user-private `${TMPDIR:-/tmp}/worktree-npm-install-<uid>/` dir, keyed by worktree name and path checksum.
Completes the PR #1451 review follow-up by deleting the shared-/tmp attack surface outright. The predictable `${TMPDIR:-/tmp}/worktree-npm-install-<uid>` scheme was raceable on a shared machine: another local user could pre-plant a symlink on the log file, and the directory's -L/-d/-O/chmod check sequence was non-atomic no matter how it was ordered. The log now lands at `<gitdir>/npm-install.log`. Each linked worktree has its own gitdir under the main checkout's `.git/worktrees/<name>/`, which is per-worktree and owner-controlled by construction — no shared directory, no predictable leaf, and no hardening dance to get wrong. An unresolvable or unwritable gitdir runs the install unlogged rather than failing, as before; the detached std fds, the flags, and the delete-on-success behaviour are unchanged. Two pins added: a failed install leaves its full output at that path, and a clean one leaves nothing behind. Claude-Session: https://claude.ai/code/session_01EwxSRv8oy1NxoQm66WKa4J
Fixes VST-340 (issue #1417):
createrannpm installunconditionally in any worktree with apackage.json, so a pnpm workspace (drovr, pnpm 10.33.2) got a wrong-tree install AND a straypackage-lock.json— every fresh worktree started dirty, trippinggit status --porcelaingates, andgit add -Awould have committed an npm lockfile into a pnpm monorepo.install_worktree_dependenciesnow detects the manager first: apnpm-lock.yaml/yarn.lock/bun.lockb/bun.lockbesidepackage.json, or apackageManagerpin naming pnpm/yarn/bun, skips the step (those repos provision their own dependencies). The historical npm path survives only where npm is genuinely the manager, and a failed install now names its log file instead of vanishing into a silenced background job.New
worktree_js_dependency_install.shsuite (4 pins, red-first: both skip pins fail against the old script; the plain-npm pin is the must-still-install control). Neighbor worktree suites pass.Closes VST-340.
Closes #1417.
https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5