fix(launcher): self-heal never fired under pnpm 11 - #649
Conversation
leadsToUs() scanned a cmd-shim's quoted paths with /"([^"]+)"/g. That class cannot match an empty pair, so pnpm 11's `exe=""` / `msys=""` header lines each consumed a single quote and re-paired every following quote off-by-one. The real target token was never produced, leadsToUs returned false, and healPathEntry skipped the entry — so every `nub` call under pnpm 11 kept paying the ~50ms node hop instead of the ~1-4ms sh trampoline. pnpm 10, whose template has no empty assignment, was unaffected, which is why the existing matrix stayed green. Silent perf regression, not a crash: nothing failed, the fast path just never engaged. - widen the class to [^"]* so empty pairs pair correctly - prefer pnpm >=11's own `# cmd-shim-target=<abs>` trailer when present; it is the shim declaring its own target, so it cannot drift with template churn the way quote-scraping does - add a `pnpm11` fixture style carrying both hazards (empty assignments + the trailer), copied from a real pnpm@11.18.0 shim, and sweep it in the matrix Verified two ways. Fixture matrix: 26 ok/5 failed before, 31 ok/0 failed after. Real `pnpm@11.18.0 add @nubjs/nub@0.6.0`, two fresh installs differing only in launch.js: stock leaves the shim byte-identical, fixed rewrites it to the sh trampoline.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Fixes a silent launcher self-heal regression under pnpm >=11 by making leadsToUs() resilient to pnpm 11’s empty quoted assignments and by preferring pnpm’s explicit cmd-shim-target declaration when present. This restores the fast healed trampoline path so repeated nub invocations don’t keep paying the Node hop cost.
Changes:
- Update
leadsToUs()quote scanning to tolerate empty""pairs and prefer pnpm >=11’s# cmd-shim-target=...trailer. - Add a pnpm 11-style fixture shim (empty
exe=""/msys=""pluscmd-shim-target) to the launcher test harness. - Expand the launcher matrix to sweep the new
pnpm11style for both the main and concurrency scenarios.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/launcher/run-launcher-matrix.sh | Adds pnpm11 to the per-Node sweep for block + concurrency scenarios. |
| tests/launcher/make-fixture.sh | Adds a pnpm >=11 cmd-shim fixture variant that reproduces the parse hazards and includes cmd-shim-target. |
| npm/nub/bin/launch.js | Fixes leadsToUs() parsing for pnpm 11 and prefers pnpm’s declared target when available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Important
The diagnosis and the regex fix are correct — I confirmed both against the real upstream template. But the new pnpm11 fixture passes with the buggy regex still in place, so the silent regression this PR fixes has no regression guard, and the added # cmd-shim-target= trust path widens the clobber guard past the invariant documented directly above it.
Reviewed changes — the leadsToUs() parse fix in the self-heal launcher, plus the fixture style and matrix sweep added to cover it.
- Quote-scan widened to
[^"]*—npm/nub/bin/launch.js:146. Correct, and it generalizes: I probed old vs new against the real@zkochan/cmd-shimtemplate (5-branch exec chain,*WSL2*arm,cygpathcall), not just the fixture —[^"]+yields 0 clean$basedirtarget tokens,[^"]*yields 3. - New
# cmd-shim-target=trust path —npm/nub/bin/launch.js:139-145. The premise checks out: that trailer is emitted by@zkochan/cmd-shim9.0.6 (shimTarget()insrc/index.ts), which is what pnpmv11.18.0's bin-linker resolves, and upstream exportsisShimPointingAtto read it. Concern is with how much it is trusted, inline. pnpm11fixture style —tests/launcher/make-fixture.sh:80-106, swept intests/launcher/run-launcher-matrix.sh:149,152. Carries both hazards, but see inline — carrying both is what defeats the coverage.
ℹ️ The harness README still documents two shim styles
tests/launcher/README.md is the documented entry point for this loop and was not updated for the third style. Line 17 gives the usage as make-fixture.sh [dest] [symlink|pnpm], and line 46 says the host scenarios cover "both shim shapes". Not anchorable inline — the file is not in the diff.
Technical details
# Update the launcher harness README for the `pnpm11` style
## Affected sites
- `tests/launcher/README.md:17` — usage string reads `[symlink|pnpm]`; omits `pnpm11`, and the sentence describing the styles ("`symlink` reproduces npm/bun/yarn's on-PATH shape; `pnpm` reproduces the cmd-shim shape") does not mention the pnpm 11 shape or why it is separate.
- `tests/launcher/README.md:46` — "both shim shapes, swept across Node versions" is now three shapes.
## Required outcome
- The README describes all three on-PATH entry shapes and names what the `pnpm11` style specifically guards (the empty-quote-pair parse hazard, and the `# cmd-shim-target=` trailer), so a future reader knows why it exists as a separate style rather than deleting it as redundant with `pnpm`.ℹ️ Nitpicks
tests/launcher/make-fixture.sh:82-83describes the shim as "copied from a realpnpm@11.18.0 addshim", but it collapses upstream's 5-branch exec chain to a 2-branchif/elseand drops both the*WSL2*case arm and thecygpathcall inside the*CYGWIN*|*MINGW*|*MSYS*arm. I verified the fix parses the full real template correctly, so there is no correctness gap — but the wording overstates fidelity, and a verbatim paste would make the fixture a stronger guard against the template churn the PR is worried about.npm/nub/bin/launch.js:144passesdeclared[1].trim()straight torealpathSync, whereas the quoted-token branch resolves relatives against the shim's own directory (path.join(basedir, p), line 150). A relative declaration would resolve against the invoking process's CWD instead. Harmless today since upstream writes an absolutesrc, but the two branches reading the same shim disagree on what a relative path means.
Claude Opus | 𝕏
The trailer value is not guaranteed absolute. pnpm's own reader does path.resolve(path.dirname(shShim), target) (selfUpdate.ts) and carries a test named 'self-update resolves relative legacy shim targets from pnpmHomeDir' that writes a relative one. leadsToUs passed the raw value to realpathSync, which resolves a relative path against CWD. Severity is latent, not live: in a full pnpm 11 shim the quoted-path scan resolves the same target anyway, and the pnpm11rel fixture passes with or without this change. It bites only on pnpm's trailer-ONLY legacy shim shape, where no quoted path exists to fall back to — measured there as absolute-only -> ENOENT -> heal skipped, resolved -> match. That shape has no exec line so the harness cannot drive it; recorded as a comment instead. Adds a pnpm11rel fixture style pinning that a relative trailer does not break the normal path.
There was a problem hiding this comment.
Important
The path.resolve change is correct and cleanly fixes the CWD-relative nitpick from the last round. But the pnpm11rel style it ships with cannot fail for it, or for the regex fix — I measured all three ablations green — so the delta adds two matrix legs per Node to CI for zero coverage. Both inline findings from the previous review are still open.
Reviewed changes — the delta since eeac070 resolves the # cmd-shim-target= trailer against the shim's directory and adds a fixture style pinning a relative trailer.
- Trailer resolved against the shim dir —
npm/nub/bin/launch.js:159.path.resolve(basedir, …)is the right call and makes the two branches ofleadsToUsagree on what a relative path means, which is what the prior nitpick asked for. No-op on an absolute value, so real pnpm 11 shims are unaffected. - New
pnpm11relfixture style —tests/launcher/make-fixture.sh:82,90-94, swept intests/launcher/run-launcher-matrix.sh:149,152. Same shim aspnpm11with a relative trailer. See inline. - Justification comment expanded —
npm/nub/bin/launch.js:143-156. Cites pnpm's own reader and concedes the new fixture does not exercise the resolve.
ℹ️ Nitpicks
npm/nub/bin/launch.js:143-145— "The value is NOT guaranteed absolute" overstates what upstream supports.shimTarget(src)in@zkochan/cmd-shimis a pass-through of whateversrcthe caller handscmdShim(), and pnpm's bin-linker always passes an absolute one (@pnpm/bins.linker:depDir = path.resolve(opts.modulesDir, alias);@pnpm/bins.resolver:binPath = path.join(pkgPath, binRelativePath)). The citedpath.resolve(path.dirname(shShim), target)ishasLegacyHomeDirShimhardening pnpm's own migration detector, and the "dedicated test for the relative form" hand-writes its shim body withfs.writeFileSync— it is not a sample of generator output. The code is still worth keeping for branch consistency; the comment should say "defensive: no writer path emits a relative value" rather than implying the shape occurs.npm/nub/bin/launch.js:139-156— 18 comment lines for a 3-line branch, and 150-156 narrate a fixture ablation rather than design, invariant, or provenance.AGENTS.md's bar is sparse and dense; the first paragraph plus one sentence on whyresolveand not barerealpathSynccarries everything a future reader needs.tests/launcher/README.mdis now two styles behind, not one: line 17 still gives the usage as[symlink|pnpm]and line 46 still says the host scenarios cover "both shim shapes".
Claude Opus | 𝕏
Two defects in the same area, both measured. leadsToUs read any regular-file PATH entry as utf8 and regexed it. A PATH `nub` is usually a PM shim, but it can be a REAL nub binary — the curl install puts one at ~/.nub/bin alongside an npm install. Measured on the shipped 45MB darwin-x64 binary: 379ms to read, 41,781 quoted matches, 17,355 realpath syscalls, 1135ms total, versus 0ms on an actual shim. Under a PM whose heal never lands that is paid on every call, so it compounds with the pnpm 11 bug this branch fixes. Every shim shape we handle is ~0.5-2KB and starts with `#!`, so a size cap plus a shebang probe rejects the binary in two syscalls. Guard verified to bail at 0ms. Separately, npm/nub/bin/nubx was committed 100644 while bin/nub is 100755, and that asymmetry propagates into the published tarball (confirmed in the 0.0.31 archive: bin/nub -rwxr-xr-x, bin/nubx -rw-r--r--). npm re-adds +x for bin-field entries on install so the practical blast radius is small, but the mode should not differ between two files of the same kind. Launcher matrix: 41 ok, 0 failed.
There was a problem hiding this comment.
Important
Both new guards are correct, and I verified they reject no real shim shape — every POSIX on-PATH bin entry npm, pnpm 10/11, yarn classic, yarn berry, bun, and Corepack produce is either a symlink or a sub-1 KB #!/bin/sh script. But this commit's second advertised fix, the +x on bin/nubx, is not in the commit: nubx is still mode 100644 at 05f6700, blob-and-mode identical to main.
Reviewed changes — the delta since 067bbbe adds two early-return preconditions to leadsToUs() ahead of the shim parse.
- Size cap before the read —
npm/nub/bin/launch.js:144.st.size > 64 * 1024rejects a real nub binary on$PATHusing thelstatthe caller already took, so the 45 MB slurp costs nothing extra. The bound matchesMAX_BIN_SHIM_BYTESinvendor/aube/crates/aube-linker/src/sys.rs:973, where aube caps its own shim reader at the same constant for the same reason. - Shebang probe after the read —
npm/nub/bin/launch.js:146. Verified safe by building fixtures and running the real tools: npm 10, pnpm 10 and 11, yarn classic 1.22, yarn berry 4.5 under bothnode-modulesandpnp, bun 1.3 under both linkers, and Corepackenable. Every regular-file entry is#!/bin/shat 880–944 B; everything else is a symlink, handled by the earlier branch. Berry's PnP mode writes noPATHentry at all. Launcher matrix stays 41 ok / 0 failed on node v24.18.0. - Incidental narrowing of the clobber guard — the probe also rejects the exact mode-0644, no-shebang, comment-only file from the open
launch.js:139-145thread; I re-measured it and it now returnsfalse. It does not close that thread: a shebang-led file whose only mention of us is# cmd-shim-target=<our launcher>still returnstrue, and so does the\s-across-a-newline variant.
⚠️ The commit's +x on bin/nubx fix is not in the tree
05f6700 documents two defects and fixes one. git ls-tree HEAD npm/nub/bin/ returns nubx at 100644 with the same blob as origin/main, and git show --stat 05f6700 is one file, nine insertions, launch.js only — the chmod was never staged. Runtime impact is small exactly as the message says (nubx is a bin-field entry in npm/nub/package.json:26, so npm re-adds +x on install), but the mode asymmetry it describes still ships in the tarball, and the commit message permanently asserts a change a future reader will not find.
Technical details
# `npm/nub/bin/nubx` is still mode 0644; the commit message claims otherwise
## Affected sites
- `npm/nub/bin/nubx` — mode `100644` at head `05f6700`, identical blob and mode to `origin/main`. `npm/nub/bin/nub` is `100755`.
- `05f6700`'s commit message — paragraph 3 states the mode was corrected ("Separately, npm/nub/bin/nubx was committed 100644 while bin/nub is 100755"), but `git show --stat` for that commit lists only `npm/nub/bin/launch.js`.
## Required outcome
- Either the mode change lands (`chmod +x npm/nub/bin/nubx`, staged so git records `100755`), or the paragraph claiming it comes out of the commit message. The tree and its history must agree.
- Since the message is only editable while the PR is open, resolving this after merge means a force-push or a permanently inaccurate record.
## Open questions for the human
- Was the `chmod` intended for this PR at all, or did it get folded into the message from a separate local change? If it belongs elsewhere, dropping the paragraph is the cheaper fix.ℹ️ Nitpicks
npm/nub/bin/launch.js:142-143— "a size cap plus a shebang probe rejects the binary for two syscalls" conflates the two guards. The size cap reuses thelstathealPathEntryalready took at line 208, so it rejects the 45 MB binary for zero extra syscalls; the shebang probe is what needs an open/read/close, and only for files that already passed the cap. "the size cap alone rejects it off thelstatwe already have" states the actual mechanism.npm/nub/bin/launch.js:137-165— now ~29 comment lines carrying two one-line guards and a three-line branch. The measurement paragraph (137-143) earns its place as provenance; lines 159-165, which narrate which fixture does and does not exercise the resolve, are the ablation-log shapeAGENTS.mdasks to keep out of source comments.
Claude Opus | 𝕏
The pnpm 11 fix this branch lands had no regression guard. leadsToUs tries the `# cmd-shim-target=` trailer before the quote scan and returns on a match, so a fixture carrying both hazards -- which is exactly what a real pnpm 11 shim is -- still heals when either mechanism alone regresses. Measured by ablation: reverting `[^"]*` to `[^"]+` left the whole matrix green, on the synthetic fixture and on a shim written by real pnpm@11.18.0 alike. That is how the bug shipped green once. Replace pnpm11/pnpm11rel with four styles. pnpm11 is now byte-identical to real @zkochan/cmd-shim 9.0.6 output (5-branch exec chain, *WSL2* arm, cygpath call), verified by diffing against the generator. `scan` withholds the trailer so only the quote scan can match; `decl`/`declrel` assemble the target into an UNQUOTED variable so no quoted token resolves to us and only the trailer branch can match, the second with a relative value. The comment claiming that shape could not be driven end-to-end was wrong -- it runs fine, it just needs an unquoted target. Two guards on trusting the declaration, both because healPathEntry's rename has no backup. Require an `exec` in the body: a file whose only mention of our launcher is `# cmd-shim-target=<us>` was being renamed over, at mode 0644 and 0755 alike, which contradicted the invariant stated directly above the branch. And `[ \t]*` instead of `\s*`, which spans newlines and paired a bare `#` line with a following key line. Both now have a foreign-nub negative test. Verified against real pnpm 11.18.0 and 10.20.0 (regular-file shims, both heal) and npm 11, yarn 1.22, bun (symlinks, which never reach this branch), so the corroboration rejects no real shim shape. Concurrency now sweeps only symlink/pnpm/pnpm11 -- the polyglot race it guards is in the heal write, identical whichever route matched, so a leg per parse style cost 200 forks for nothing. Also chmod +x npm/nub/bin/nubx: 05f6700's message documented that change but the mode was never staged, so the tree still had 100644 against bin/nub's 100755. Ablation, each launcher change reverted independently: [^"]* -> [^"]+ -> scan red drop the trailer branch -> decl + declrel red path.resolve -> bare value -> declrel red drop the exec corroboration -> foreign "declares our target, never execs" red [ \t]* -> \s* -> foreign "bare # then newline then key" red Matrix: 285 ok, 0 failed across node 18.19.0, 20.19.0, 22.14.0, 22.15.0, 26.5.0.
There was a problem hiding this comment.
ℹ️ No critical issues — two minor coverage suggestions inline.
The fixture restructure genuinely closes all three prior threads, and I verified it the way the new README asks to be verified: by ablation, not by a green run. All five reverts the README claims take a leg red actually do (measured on node v24.18.0). Both remaining suggestions are about coverage of the harness itself, not about the fix.
Reviewed changes — the delta since 05f6700 replaces the un-failable pnpm11/pnpm11rel pair with mechanism-isolating styles, adds corroboration to the trailer branch, and lands the +x the previous commit's message promised.
- Trailer read gated on
execand tightened to[ \t]*—npm/nub/bin/launch.js:164-165. Both guards the prior thread asked for. I checked the gate against the generator rather than the fixture:npm/nub/bin/nubis#!/usr/bin/env node, so@zkochan/cmd-shimresolvesprog = 'node', and@pnpm/bins.linkerpassesnodeExecPathonly whenengines.runtimeis set (nub declares plainengines.node) — so the shim always takesgenerateShShim'sshLongProgbranch, which has carriedexecsince 2020. A real trailer-without-execwindow did exist (@zkochan/cmd-shim9.0.0–9.0.2, fixed bye4f6173a) but only on the branch a shebang'd target cannot reach. - Doc comment now states the actual precondition —
npm/nub/bin/launch.js:121-126. The stale "rejects comment-only mentions" invariant is gone, replaced by what the code enforces. "as pnpm's own reader does" also checks out: pnpm'sreadShimTargetuses\s*, and the clause attaches to thepath.resolvestep, which does match. pnpm11is now verbatim upstream —tests/launcher/make-fixture.sh:107-145. Verified empirically, not by reading the generator: packed@zkochan/cmd-shim@9.0.6, rancmdShim()against a node-shebang target, and string-compared — 1067 bytes each, identical, zero per-line mismatches.uname -a, thebasedir_win/exe/msysordering, bothcasearms' indentation, the double space inexec node "<target>", and the trailer as the last line all match. The missingexit $?is correct: upstream emits it only on theshLongProg-unset branch. The fidelity nitpick fromeeac070is closed.- Three derived styles that can each go red alone —
tests/launcher/make-fixture.sh:102-166, swept attests/launcher/run-launcher-matrix.sh:173. Every claim in the README's revert table holds:[^"]+→scanred (4 failures,scanonly); trailer branch dropped →decl+declrelred (8);path.resolvedropped →declrelred (4);execgate dropped → the "never execs" probe red;\s*→ the "bare#then newline" probe red. Full matrix at head is 57 ok / 0 failed. run_foreignextended to three probes —tests/launcher/run-launcher-matrix.sh:143-163. Both new probes are real negative tests, not decoration.npm/nub/bin/nubxis 0755 — the diff recordsold mode 100644 / new mode 100755, closing the05f6700body finding.
ℹ️ Nitpicks
tests/launcher/run-launcher-matrix.sh:2-3— the script's own header still says it exercises "both package-manager bin-shim shapes (symlink: npm/bun/yarn; cmd-shim: pnpm)". That is six styles now, and the scenario list on lines 9-15 doesn't mention that three of them exist to isolate a parse mechanism. The README got this treatment; the script header is the same staleness one file over. Not anchorable — the lines aren't in a diff hunk.
Claude Opus | 𝕏
…style
Two gaps from review, both instances of the thing this branch is about — a check
that cannot go red, and a green leg that pins nothing.
The `#!` precondition in leadsToUs had no probe. All three foreign bodies opened
with `#!/bin/sh`, so `if (!body.startsWith("#!")) return false;` could be deleted
with the matrix still at 57 ok. That check is a clobber guard, not only the perf
guard the size cap is: it is what rejects a no-shebang file carrying
`# cmd-shim-target=<ours>`. Add a fourth foreign probe for exactly that shape at
mode 0644, and give it a row in the README revert table. The size cap stays
unpinned and the README now says so — a 64KB fixture costs more than it is worth.
make-fixture.sh fell through to the pnpm 10 shim for any unrecognized style, so
`make-fixture.sh /tmp/x typo-style` exited 0 and wrote the wrong body. The matrix
spells all six style names by hand, so a misspelling there would have bought a
green leg guarding nothing. Name the arm `pnpm)` and exit 2 on anything unknown.
Also refresh the matrix script's own header, which still described "both
package-manager bin-shim shapes" and did not mention that three of the six styles
exist to isolate one parse mechanism.
Ablation, each guard reverted independently:
drop the `#!` precondition -> foreign "no shebang, declares our target" red
[^"]* -> [^"]+ -> scan red
drop the trailer branch -> decl + declrel red
path.resolve -> bare value -> declrel red
drop the exec corroboration -> foreign "declares our target, never execs" red
[ \t]* -> \s* -> foreign "bare # then newline then key" red
Unknown style now exits 2. Matrix: 290 ok, 0 failed across node 18.19.0, 20.19.0,
22.14.0, 22.15.0, 26.5.0.
There was a problem hiding this comment.
✅ No new issues found.
The delta is harness-only and it closes both remaining threads. I verified it the way the README asks — by ablation, not by a green run: the new no-shebang probe genuinely pins if (!body.startsWith("#!")) return false; (58 ok / 0 failed at head; delete that line and it is 57 ok / 1 failed, the sole failure being foreign CLOBBERED: no shebang, declares our target). Every launcher change in this PR now has a leg that can go red on its own.
Reviewed changes — the delta since 04f25b2 pins the shebang precondition with a negative probe, makes an unknown fixture style a hard error, and finishes the doc staleness sweep.
- Fourth
run_foreignprobe —tests/launcher/run-launcher-matrix.sh:172-176. A mode-0644 body that declares our target and containsexecbut has no shebang. Measured red without the guard and green with it, on node v24.18.0. make-fixture.shno longer aliases a typo to the pnpm 10 shape —tests/launcher/make-fixture.sh:167,176-184. The old catch-all is now an explicitpnpm)arm and*)errors:bash make-fixture.sh /tmp/x typo-style→exit 2plus the six valid names on stderr, whiledeclrelstill exits 0. Every caller (run-launcher-matrix.sh:56,112,141,docker-non-owner.sh:17) passes a valid style explicitly and the$2default issymlink, so nothing breaks.run-launcher-matrix.shisset -uo pipefailrather than-e, so a bad style does not abort the sweep — but the stderr line prints and the dependent legs go red, which is loud enough.- Sixth row in the README revert table —
tests/launcher/README.md:47-49, with the reason the#!check belongs on a list of clobber guards while the size cap does not. All six rows now hold; I have measured every one across this PR's reviews. - Matrix-script header rewritten —
tests/launcher/run-launcher-matrix.sh:2-12,21-22. The "both bin-shim shapes" claim is gone, the derived styles are explained where a reader meets them, and theforeignscenario line names the "declares our target without dispatching to it" cases. This closes the nitpick from the last round.
Claude Opus | 𝕏
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |

leadsToUs()scanned a cmd-shim's quoted paths with/"([^"]+)"/g. That class cannot match an empty pair, so pnpm 11'sexe=""/msys=""header lines each consumed one quote and re-paired every following quote off-by-one. The target token was never produced, sohealPathEntryskipped the entry.Every
nubcall under pnpm 11 kept paying the ~50ms node hop instead of the ~1-4ms sh trampoline. pnpm 10 has no empty assignment in its template and was unaffected — which is why the matrix stayed green.Silent perf regression, not a crash: nothing failed, the fast path just never engaged.
Changes
[^"]*# cmd-shim-target=<abs>trailer when present — the shim declaring its own target, so it can't drift with template churn the way quote-scraping doespnpm11fixture style carrying both hazards, copied from a realpnpm@11.18.0shim, and sweep it in the matrixVerification
Fixture matrix: 26 ok / 5 failed before, 31 ok / 0 failed after. The new style fails without the launcher change.
Real
pnpm@11.18.0 add @nubjs/nub@0.6.0, two fresh installs differing only inlaunch.js: