Repository navigation
fix(npm): recognize extended subcommands - #3087
Conversation
cbd7f2f to
56519c4
Compare
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — APPROVED, follow-ups filed
Claim: 15 official npm subcommands (query, sbom, ll, run-script, …) must reach npm as themselves instead of being turned into npm run <name>.
Scope: accept. The change is one const and one test, it does exactly what #2663 asks, and no other open PR or local branch touches npm_cmd.rs beyond the rustfmt style-edition reflow.
Ran: npm 10.8.2 (node:20-alpine), 10.9.8 (node:22-alpine), 11.19.0 (node:24-alpine) and 12.0.2 (local), all LC_ALL=C; rtk built from merge-base f9d8c775 and from head 56519c49 with a shared target dir; 7 before/after cases in a fixture project that deliberately declares build, query, hook and edit scripts, so shadowing would show.
Savings: not applicable — this is routing, not filtering. I checked instead that the newly routed paths stay faithful: npm ll differs from raw npm by two trailing blank lines, and npm sbom --sbom-format=spdx is byte-identical modulo its UUID and timestamp and still parses as JSON.
CI @56519c49: all 11 checks green (fmt, clippy, test on ubuntu/macos/windows, benchmark, Security Scan, semgrep, doc review, test presence, CLA).
Blocks merge (0)
None.
Before / after
f9d8c775 (base) |
56519c49 (head) |
|
|---|---|---|
rtk npm query |
ran the query script |
[] — npm's own query output |
rtk npm sbom |
Missing script: "sbom" |
npm's Must specify --sbom-format usage |
rtk npm ll |
Missing script: "ll" |
the dependency tree |
rtk npm run-script build |
Missing script: "run-script" |
runs build |
rtk npm build |
runs build |
runs build — injection preserved |
query and edit now shadow same-named scripts, which is correct: plain npm query never runs your script either, so rtk is matching npm rather than diverging from it.
Optional — will not hold merge
test_extended_official_subcommands_do_not_inject_runis named for injection but asserts membership, so a failure reports "must be routed as a native subcommand" when the real cause is that a name left the array.test_extended_official_subcommands_are_allowlistedwould point the reader at the right thing. Purely the name — see below for why the test itself earns its place.
Filed as follow-ups
- #4011 —
NPM_SUBCOMMANDSdrifts from npm's command set in both directions.binis in the list and was removed from npm in v9;trustandundeprecateare real npm 11/12 commands and are missing; the alias sweep #3411 asked for is still open. All pre-existing or outside this PR's hunks. - #3411 — its second defect (
npm run-script <s>→npm run run-script <s>) is fixed by this PR; comment added there with the transcript rather than a duplicate issue.
Checked and correct — no need to re-verify
- 14 of the 15 names resolve on npm 11.19.0 and 12.0.2; all 15 on npm 10.x.
run-scriptresolves through npm's alias table. - The new test is not redundant. Mutation: deleting
"query",from the const makestest_extended_official_subcommands_do_not_inject_runFAIL whiletest_npm_subcommand_routingstill passes — the older test iterates the const, so it cannot see a removal. The new test is the only thing pinning these 15 names. - Single registration point.
exec/exec_with(npx, bunx) bypass injection by design — npx takes a package name, not a script.pnpm,bunanddenohave no run-injection at all. The hook rewrite layer prefixesrtkgenerically with no second npm list, anddiscover/rules.rsonly matches an already-explicitrun/run-script. - Merges cleanly onto
develop@79b96a44; on the merged treecargo fmt --all -- --check,cargo clippy --all-targets --all-features -- -D warningsandcargo test --all-features(3537 + 155 passed, 0 failed) are all green.
Hypotheses built and dropped
- "
hookis not an npm command, so this PR regressesnpm run hook." Half true and still the right call as written.npm hookexists on npm 10.8.2 and 10.9.8 — Node 20/22 LTS — where dropping it would leave exactly the bug #2663 reports. On npm ≥11rtk npm hooknow prints npm's ownUnknown command: "hook" / Did you mean this? npm run hook, so rtk reproduces npm instead of diverging from it, and npm itself tells the user the fix. Including it never makes rtk disagree with npm; excluding it would, on npm 10. Recorded in the follow-up issue so nobody "fixes" it blindly. - "Routing
sbomthrough the npm line filter corrupts its JSON." First measurement showed 2229 B → 1418 B. That was an artifact of my own shell fallback comparing two different commands; the real comparison is byte-identical andJSON.parse-clean.
Your questions
None posted since the PR opened.
Next
Nothing — approving. Rounds: 1/3. Threads: 0 open, 0 resolved this round.
Summary
npm query,npm sbom, andnpm completionfrom becomingnpm run ...runinjection for project script names such asbuild,dev, andlintTesting
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features(2,496 passed, 8 ignored)Closes #2663