Repository navigation
feat(discover): route pnpm's bare script forms - #3676
dwin-gharibi wants to merge 1 commit into
Conversation
pnpm runs a bare script name directly -- pnpm test is pnpm run test -- and that shorter form is what people type, but only the run/run-script spelling was routed. Adds test, build, typecheck and format, plus the (\s|$) word boundary the rule was missing: without it pnpm installer matched on the install alternative. dev, start, serve and watch are deliberately excluded. They never terminate, and rtk captures child output rather than streaming it, so routing them would leave the agent staring at nothing. lint is excluded too: pnpm lint already reaches the dedicated eslint/biome filter, which parses the tool's output instead of just stripping package-manager boilerplate. A test pins that routing so it is not accidentally taken over later.
There was a problem hiding this comment.
Round 1 of 3 — CHANGES REQUESTED
Claim: Route pnpm's bare script forms (test, build, typecheck, format) through rtk pnpm, and add the missing (\s|$) word boundary.
Scope: accept if narrowed to "route pnpm's bare test, build and format, and add the missing (\s|$) word boundary" — typecheck must come out. (round 1, frozen)
Ran: real pnpm 9 and 12.4.1 (the Rust rewrite) against two real projects; rtk rewrite × 28 command forms × develop 79b96a4 vs head f0f6042; flag forms (--silent, --prod, -- --watch=false, --help); a streaming probe; two mutation tests; fmt/clippy/cargo test --all on the head (2630 pass) and on the head merged into latest develop (3541 pass, clean merge). Savings on the newly-routed forms: 0%, byte-identical — which your PR body states honestly.
CI @f0f6042c: never run. Only license/cla is green; check-target is SKIPPED because the fork's workflow is waiting for maintainer approval. I'll approve the run once the blocker is addressed.
Thank you for the PR description — the lint exclusion, the long-running exclusion and the honest 0%-savings note all made this much faster to review, and the "what the tests caught" section is exactly the right instinct.
Blocks merge (1, frozen at round 1)
src/discover/rules.rs:89— routing a script name into a non-passthrough rtk subcommand.typecheckis the one added name that collides with a real clap variant, sortk pnpm typecheckrunstscinstead of the project's script, drops--noEmit, and writesdist/index.jsinto the working tree. Verified on pnpm 9 and 12.4.1. Regression againstdevelop, and it makes #2809 fire by default. Details and a one-word fix in the line comment.
Optional — will not hold merge
-
rules.rs:93—test/build/formatreport 80% savings but deliver 0%. All three reachrun_passthrough, which inherits stdio and filters nothing. Withsubcmd_status: &[]they classify asRtkStatus::Existing("dedicated handler with filtering") anddiscovercredits them the rule's 80%. They also leave the unsupported list — erasing the exact signal that #3876 depends on. The fix mirrors the cargo rule one entry above:subcmd_status: &[("test", Passthrough), ("build", Passthrough), ("format", Passthrough)], which zeroes the savings automatically. -
rules.rs:84— the stated rationale for excluding dev/start/serve/watch is not accurate. "rtk captures rather than streams, so the agent would see nothing and hang" isn't true of this path:run_passthroughinherits all three stdio handles. Verified — a script printing then sleeping 10s, killed at 3s, showed its early line. Excluding those four is still a fine conservative call; it's the reason that would mislead the next maintainer. The same text is repeated in the test doc atregistry.rs:1517. -
rules.rs:88— changelog text in a comment. "The trailing(\s|$)is new -- without itpnpm installermatched" describes a prior revision; house style is to state the invariant instead, e.g. "anchors the subcommand at a word boundary sopnpm installerdoes not match". -
registry.rs:1544—existing_subcommands_still_rewriteasserts onlyis_some(), so it can't distinguish "rewrote correctly" from "rewrote to the wrong rtk command". The sibling test two functions up already usesassert_eq!(…, Some("rtk …")).
Checked and correct — no need to re-verify
test,build,format→PnpmCommands::Other→ passthrough. Byte-identical to raw pnpm on both pnpm 9 and 12.4.1, exit codes propagated,--silent/--prod/-- --watch=false/--helpall forwarded verbatim with no clap interception.- The
(\s|$)fix is real:pnpm installerrewrote tortk pnpm installerondevelop, no longer does. pnpm lint→rtk lintholds. The mechanism ismatches.last()atregistry.rs:169("last match wins"), with the eslint/biome rule at a higher index — your reasoning is right, though note it's the index and not the alternation that decides it.- No later rule shadows the new names (
next/prettier/vitest/jestall require their literal tool name). - Capture-group numbering is safe:
(\s|$)is group 2;subcmd_savings/subcmd_statusread group 1. - Both new tests are genuine, proven by mutation: adding
devto the pattern failslong_running_scripts_are_left_alone; removing(\s|$)failssimilar_names_are_not_captured. - pnpm 12 specifics:
testandstartare genuine built-ins there, whilebuild/typecheck/formatresolve through the bare-script fallback, which v12 still supports — so your premise holds on the Rust rewrite too. (Unrelated to this PR:run-scriptno longer exists in pnpm 12, so that alternative is dead on v12.) - Merges cleanly into latest
develop; full suite green after merge.
Filed as follow-ups
- #2809 — absorbed: comment added with the
dist/index.jsemission transcript and the dropped---noEmitmechanism there rather than opening a duplicate. - #4009 — filed: 23 rules on
developend in a bare subcommand alternation with no boundary; this PR fixes the pnpm one, 22 remain, with 16 demonstrated over-matches includinggit branchless(a real tool) →rtk git branchless. Untouched code, so an issue rather than a finding here — nice catch surfacing the class. pnpm --filter app test/-Fstay unrouted — absorbed by existing #3858, not refiled.
Hypotheses built and dropped
- The 20% savings floor blocks these 0%-savings forms. Dropped — the floor governs filters; these are passthroughs that add tracking.
pnpm formatis shadowed by the prettier rule. Dropped — that rule requires the literalprettier.pnpm test:unitshould route. Dropped —:correctly falls outside(\s|$).long_running_scripts_are_left_aloneis vacuous because it also passes on the parent commit. Dropped — mutation shows it does catch a future addition ofdev, which is what it claims to guard.
Next
Drop typecheck from the alternation and from bare_script_names_are_rewritten; optionally take items 2–5 in the same push. Then I'll approve the CI run and, once it's green, approve the PR. That is the whole remaining set.
Rounds: 1/3. Threads: 1 open (the typecheck blocker), 0 resolved this round.
| // `lint` is absent on purpose: `pnpm lint` already routes to the dedicated | ||
| // eslint/biome filter (`rtk lint`), which knows the tool's output format. | ||
| // The trailing (\s|$) is new -- without it `pnpm installer` matched. | ||
| pattern: r"^pnpm\s+(exec|i|install|list|ls|outdated|run|run-script|test|build|typecheck|format)(\s|$)", |
There was a problem hiding this comment.
Blocking: typecheck is the one added name that is not a passthrough, and routing it writes files into the project.
PnpmCommands::Typecheck is a real clap variant. On current develop, main.rs:2096 dispatches it to tsc_cmd::run(Some("pnpm"), &args, …), which runs tsc with only the args after typecheck. For a bare pnpm typecheck that arg list is empty, so the project's typecheck script never runs and any flags it carried are dropped.
Verified on the head merged into latest develop, real project, "typecheck": "tsc --noEmit", tsconfig.json with outDir: "./dist":
# pnpm 12.4.1
pnpm typecheck → $ tsc --noEmit exit 0, no dist/
rtk pnpm typecheck → Running: pnpm tsc exit 0, dist/index.js WRITTEN
# pnpm 9
pnpm typecheck → exit 0, no dist/
rtk pnpm typecheck → Running: tsc exit 0, dist/index.js WRITTEN
--noEmit is dropped, so a read-only type check acquires a write side effect and emits compiled JavaScript into the working tree. On a project whose typecheck script is not tsc at all, the same path turned exit 0 with 1392 bytes of real output into exit 1 with a fabricated TypeScript: No errors found.
Broken by the same mechanism: vue-tsc --noEmit, svelte-check, tsc -b, turbo run typecheck, and tsc -p tsconfig.app.json (checks the wrong file set, so green is a false pass). With tool_exec(…, MissingTool::Fetch), a project without tsc also gets a network fetch.
This is a regression against develop: today pnpm typecheck is not rewritten, so the hook leaves it alone. This change makes the hook do it automatically for everyone, turning the already-filed #2809 from opt-in into default.
The other three names are fine — test, build and format all reach PnpmCommands::Other → run_passthrough, byte-identical to raw pnpm with exit codes propagated, verified on pnpm 9 and 12.4.1.
| pattern: r"^pnpm\s+(exec|i|install|list|ls|outdated|run|run-script|test|build|typecheck|format)(\s|$)", | |
| pattern: r"^pnpm\s+(exec|i|install|list|ls|outdated|run|run-script|test|build|format)(\s|$)", |
"pnpm typecheck" also needs dropping from bare_script_names_are_rewritten, which currently asserts the broken routing is correct. With both changes the suite is green and pnpm typecheck goes back to being left alone.
…4100) Several places decided where a word ends, and none of them used the definition bash uses. `strip_word_prefix` accepted exactly one byte as the boundary between a prefix and the command behind it: an ASCII space. Bash separates words on space, tab and newline alike, and the rule patterns match `\s+`, so a tab-separated command failed to rewrite: ls\t-la no rewrite cargo\tbuild no rewrite noglob\tls -la no rewrite The first two also classified as Supported, so `rtk discover` counted coverage the hook never delivered — the two sides disagreeing about what separates a command from its prefix. The third did not: no rule names `noglob`, so `classify_command` calls it Unsupported either way. Its rewrite is repaired here too, but nothing was over-reported for it. `strip_absolute_path` split on a literal space, so `cat\t/etc/passwd` normalised to `passwd`. `extract_base_command` and `normalize_php_tool_command_with_dirs` used `char::is_whitespace`, which is Unicode and counts NBSP — the lexer rejects exactly that in `is_word_boundary_whitespace`, whose comment says why. All of them now use it, so `cat\u{a0}/etc/passwd` stays one word as bash reads it, and the separator is carried through rather than replaced, so `noglob\tls -la` keeps its tab. The remaining `char::is_whitespace` in `join_redirect_suffix` tests whether a suffix is already separated rather than splitting a word, so it stays. Two table-wide invariants the `RULES` table has never had come with it. **Anchoring, both ends.** A pattern matches against a whole command line, so one that does not start with `^` fires on a path component, and one that ends in a bare alternation matches any command whose subcommand merely *starts* with a listed one — `git branchless` routes into `rtk git` (#4009). The 22 patterns in that state are #4014's and #3676's to fix and are named in a `PENDING` list, not exempted: an entry that no longer matches a rule fails the test, so the list can only shrink. **`rtk_cmd` does not identify a rule.** `classify_command` picks a rule by index and `rewrite_segment_inner` then looks one up again by `rtk_cmd` with `find`, whose comment claims the value is unique. It is not — `rtk php` and `rtk uv` are two rules each, so that lookup already returns a sibling. It is harmless only because every field it reads matches across each pair, and that is now asserted rather than assumed. Forcing uniqueness is the better end state but not a test-only change: the siblings are worded differently (`\b` against `(?:\s|$)`), so merging means choosing one terminator and re-checking every command each pattern currently claims. The mechanical fix is elsewhere anyway — `classify_command` already holds the matched rule's index and could pass it along instead of having the rewrite re-derive a rule from `rtk_cmd`. Left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…4100) Several places decided where a word ends, and none of them used the definition bash uses. `strip_word_prefix` accepted exactly one byte as the boundary between a prefix and the command behind it: an ASCII space. Bash separates words on space, tab and newline alike, and the rule patterns match `\s+`, so a tab-separated command failed to rewrite: ls\t-la no rewrite cargo\tbuild no rewrite noglob\tls -la no rewrite The first two also classified as Supported, so `rtk discover` counted coverage the hook never delivered — the two sides disagreeing about what separates a command from its prefix. The third did not: no rule names `noglob`, so `classify_command` calls it Unsupported either way. Its rewrite is repaired here too, but nothing was over-reported for it. `strip_absolute_path` split on a literal space, so `cat\t/etc/passwd` normalised to `passwd`. `extract_base_command` and `normalize_php_tool_command_with_dirs` used `char::is_whitespace`, which is Unicode and counts NBSP — the lexer rejects exactly that in `is_word_boundary_whitespace`, whose comment says why. All of them now use it, so `cat\u{a0}/etc/passwd` stays one word as bash reads it, and the separator is carried through rather than replaced, so `noglob\tls -la` keeps its tab. The remaining `char::is_whitespace` in `join_redirect_suffix` tests whether a suffix is already separated rather than splitting a word, so it stays. Two table-wide invariants the `RULES` table has never had come with it. **Anchoring, both ends.** A pattern matches against a whole command line, so one that does not start with `^` fires on a path component, and one that ends in a bare alternation matches any command whose subcommand merely *starts* with a listed one — `git branchless` routes into `rtk git` (#4009). The 22 patterns in that state are #4014's and #3676's to fix and are named in a `PENDING` list, not exempted: an entry that no longer matches a rule fails the test, so the list can only shrink. **`rtk_cmd` does not identify a rule.** `classify_command` picks a rule by index and `rewrite_segment_inner` then looks one up again by `rtk_cmd` with `find`, whose comment claims the value is unique. It is not — `rtk php` and `rtk uv` are two rules each, so that lookup already returns a sibling. It is harmless only because every field it reads matches across each pair, and that is now asserted rather than assumed. Forcing uniqueness is the better end state but not a test-only change: the siblings are worded differently (`\b` against `(?:\s|$)`), so merging means choosing one terminator and re-checking every command each pattern currently claims. The mechanical fix is elsewhere anyway — `classify_command` already holds the matched rule's index and could pass it along instead of having the rewrite re-derive a rule from `rtk_cmd`. Left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…4100) Several places decided where a word ends, and none of them used the definition bash uses. `strip_word_prefix` accepted exactly one byte as the boundary between a prefix and the command behind it: an ASCII space. Bash separates words on space, tab and newline alike, and the rule patterns match `\s+`, so a tab-separated command failed to rewrite: ls\t-la no rewrite cargo\tbuild no rewrite noglob\tls -la no rewrite The first two also classified as Supported, so `rtk discover` counted coverage the hook never delivered — the two sides disagreeing about what separates a command from its prefix. The third did not: no rule names `noglob`, so `classify_command` calls it Unsupported either way. Its rewrite is repaired here too, but nothing was over-reported for it. `strip_absolute_path` split on a literal space, so `cat\t/etc/passwd` normalised to `passwd`. `extract_base_command` and `normalize_php_tool_command_with_dirs` used `char::is_whitespace`, which is Unicode and counts NBSP — the lexer rejects exactly that in `is_word_boundary_whitespace`, whose comment says why. All of them now use it, so `cat\u{a0}/etc/passwd` stays one word as bash reads it, and the separator is carried through rather than replaced, so `noglob\tls -la` keeps its tab. The remaining `char::is_whitespace` in `join_redirect_suffix` tests whether a suffix is already separated rather than splitting a word, so it stays. Two table-wide invariants the `RULES` table has never had come with it. **Anchoring, both ends.** A pattern matches against a whole command line, so one that does not start with `^` fires on a path component, and one that ends in a bare alternation matches any command whose subcommand merely *starts* with a listed one — `git branchless` routes into `rtk git` (#4009). The 22 patterns in that state are #4014's and #3676's to fix and are named in a `PENDING` list, not exempted: an entry that no longer matches a rule fails the test, so the list can only shrink. **`rtk_cmd` does not identify a rule.** `classify_command` picks a rule by index and `rewrite_segment_inner` then looks one up again by `rtk_cmd` with `find`, whose comment claims the value is unique. It is not — `rtk php` and `rtk uv` are two rules each, so that lookup already returns a sibling. It is harmless only because every field it reads matches across each pair, and that is now asserted rather than assumed. Forcing uniqueness is the better end state but not a test-only change: the siblings are worded differently (`\b` against `(?:\s|$)`), so merging means choosing one terminator and re-checking every command each pattern currently claims. The mechanical fix is elsewhere anyway — `classify_command` already holds the matched rule's index and could pass it along instead of having the rewrite re-derive a rule from `rtk_cmd`. Left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…4100) Several places decided where a word ends, and none of them used the definition bash uses. `strip_word_prefix` accepted exactly one byte as the boundary between a prefix and the command behind it: an ASCII space. Bash separates words on space, tab and newline alike, and the rule patterns match `\s+`, so a tab-separated command failed to rewrite: ls\t-la no rewrite cargo\tbuild no rewrite noglob\tls -la no rewrite The first two also classified as Supported, so `rtk discover` counted coverage the hook never delivered — the two sides disagreeing about what separates a command from its prefix. The third did not: no rule names `noglob`, so `classify_command` calls it Unsupported either way. Its rewrite is repaired here too, but nothing was over-reported for it. `strip_absolute_path` split on a literal space, so `cat\t/etc/passwd` normalised to `passwd`. `extract_base_command` and `normalize_php_tool_command_with_dirs` used `char::is_whitespace`, which is Unicode and counts NBSP — the lexer rejects exactly that in `is_word_boundary_whitespace`, whose comment says why. All of them now use it, so `cat\u{a0}/etc/passwd` stays one word as bash reads it, and the separator is carried through rather than replaced, so `noglob\tls -la` keeps its tab. The remaining `char::is_whitespace` in `join_redirect_suffix` tests whether a suffix is already separated rather than splitting a word, so it stays. Two table-wide invariants the `RULES` table has never had come with it. **Anchoring, both ends.** A pattern matches against a whole command line, so one that does not start with `^` fires on a path component, and one that ends in a bare alternation matches any command whose subcommand merely *starts* with a listed one — `git branchless` routes into `rtk git` (#4009). The 22 patterns in that state are #4014's and #3676's to fix and are named in a `PENDING` list, not exempted: an entry that no longer matches a rule fails the test, so the list can only shrink. **`rtk_cmd` does not identify a rule.** `classify_command` picks a rule by index and `rewrite_segment_inner` then looks one up again by `rtk_cmd` with `find`, whose comment claims the value is unique. It is not — `rtk php` and `rtk uv` are two rules each, so that lookup already returns a sibling. It is harmless only because every field it reads matches across each pair, and that is now asserted rather than assumed. Forcing uniqueness is the better end state but not a test-only change: the siblings are worded differently (`\b` against `(?:\s|$)`), so merging means choosing one terminator and re-checking every command each pattern currently claims. The mechanical fix is elsewhere anyway — `classify_command` already holds the matched rule's index and could pass it along instead of having the rewrite re-derive a rule from `rtk_cmd`. Left as a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #3675.
Summary
test,build,typecheck,format.(\s|$)word boundary —pnpm installerused to match theinstallalternative and get rewritten.
lintis deliberately excluded:pnpm lintalready routes to the dedicated eslint/biomefilter (
rtk lint), which parses the tool's output rather than just stripping package-managerboilerplate. Sending it to
rtk pnpmwould have been a downgrade.dev,start,serve,watch.Those never terminate, and rtkcaptures rather than streams, so routing them would hang the agent with no output. This is the
one case where not adding coverage is the correct call.
Test plan
cargo fmt --all && cargo clippy --all-targets && cargo testrtk <command>output inspectedUnit tests (added)
src/discover/registry.rs, modulepnpm_bare_scripts:bare_script_names_are_rewritten— the five safe scripts.long_running_scripts_are_left_alone—dev,start,serve,watchmust returnNone.This is the test that keeps the hazard closed.
lint_keeps_its_dedicated_filter— assertspnpm lintstill resolves tortk lint.existing_subcommands_still_rewrite—install,run build,outdated,ls.similar_names_are_not_captured—pnpm installer,pnpm testify,pnpm buildkite.Behavioural check
Fake
pnpmonPATH; failing output preserved and exit codes propagated:pnpm testkeepsa.test.ts+1 failed(exit 1);pnpm buildkeepsdist/index.js(exit 0).Savings on these samples are ~0%: pnpm's own output is already terse, and the filter strips
package-manager boilerplate that small fixtures don't have. The value here is coverage
consistency and tracking, not compression — flagging that honestly rather than claiming a win.
What the tests caught
The first draft included
lintin the rule. The unit test failed withleft: Some("rtk lint"),right: Some("rtk pnpm lint")— the dedicated linter filter was alreadywinning, and adding
linthere would only have created ambiguity between two rules. Theexpectation was wrong, not the code;
lintwas dropped and a test now pins the existing routing.