fix(pnpm): rewrite pnpm commands with global flags before the subcommand

[breisnerlopez]

Problem

The hook rewriter matches pnpm via a subcommand allowlist anchored right after pnpm (^pnpm\s+(exec|i|install|list|ls|outdated|run|run-script)), with no stripping of pnpm global options. So flag-first monorepo forms are never rewritten and stream raw:

command	before
pnpm run build	✅ rewritten
pnpm -r install	❌ raw
pnpm --filter @app list	❌ raw
pnpm -w install	❌ raw

Fix

Add strip_pnpm_global_opts — a direct mirror of the existing strip_git_global_opts (#163) — that strips a fixed set of pnpm global options (-r/--recursive, -w/--workspace-root, --filter/-F) for classification only, in the same spot git is normalized in classify_command.

The rewrite step still operates on the original command text via strip_word_prefix, so:

• an unknown flag (pnpm -x build) is not stripped → no match → no rewrite;
• a flag-first form whose original doesn't match a rewrite prefix (pnpm -r lint) is a safe no-op, never a malformed rewrite.

rtk pnpm now accepts -r/-w as global flags and forwards them. They are appended after the subcommand (pnpm install -r), matching the established --filter handling — behavior-preserving, since these are root-level pnpm options accepted in either position (verified against pnpm 9.15.4: pnpm install --help lists them, and pnpm <flag> <sub> vs pnpm <sub> <flag> produce identical output and install scope for -r, -w, --filter).

Tests

16 new tests: flag-first rewrite forms (-r, --filter, -F, --filter=, -w, combos), no-regression on bare forms (pnpm install, pnpm run build, pnpm build still None), false-positive guards (unknown flag not stripped, --filter without subcommand, recursive-lint safe no-op), CLI parse tests, and the merge ordering.

cargo fmt --check, cargo clippy --all-targets, and cargo test all clean.

Note

The glued short filter form -F@app (no space) is not stripped; it falls back to the existing safe passthrough (no scope change), consistent with pre-existing behavior.

[breisnerlopez]

Rebased onto latest develop to clear the merge conflict (it was in the PnpmCommands match arms: develop had changed the tsc_cmd::run signature for Typecheck while this PR extends merge_pnpm_args_os for Other — kept both). No behavior change from the original diff; fmt + clippy -D warnings + the pnpm tests are green locally. Ready for a first review whenever you have a moment.

[KuSh]

Round 1 of 3 — CHANGES REQUESTED

Claim: pnpm commands whose global options precede the subcommand (pnpm -r install, pnpm --filter @app list, pnpm -w install) are never rewritten, because the rule is anchored right after pnpm — strip a fixed set of global options for classification only, and forward -r/-w through rtk pnpm.

Scope: accept. The claim is right, the design is right, and the hook behaviour it promises is correct in execution. One implementation over-reach has to be narrowed first — see below. Not a duplicate of #3676 (bare script forms): complementary, though they will textually conflict in rules.rs/registry.rs, so whichever lands second rebases. (round 1, frozen)

Ran: pnpm 9.15.4 against a real 3-package workspace (root + @app/app + @app/lib, real registry installs), LC_ALL=C; merge-base 79347d5e vs head 0fe63b39, shared target dir. 30 rewrite forms diffed base vs head; every rewritten spelling then executed end to end; list/outdated/install/passthrough compared against raw pnpm; both flag positions compared byte for byte; classify_command probed directly on both binaries; 4 mutations; startup timing.
Savings on the newly-routed paths: pnpm -r outdated 2740 → 78 bytes (97%), pnpm -r list 348 → 115 bytes (67%) — both clear the 20% floor (CONTRIBUTING.md).

CI @0fe63b39: all green — fmt, clippy, doc review, test presence, benchmark, Security Scan, semgrep, and test on ubuntu / macos / windows. CLA signed.

Blocks merge (1, frozen at round 1)

1. src/discover/registry.rs:155 — classify/rewrite divergence hands rtk discover and rtk session a confidently wrong answer.

The strip runs inside classify_command, but the rewrite matches rewrite_prefixes against the original text. For the rtk pnpm rule (prefix "pnpm") those agree, and that is the case your tests cover. For every tool rule the stripped form can now reach they don't: the prefix is "pnpm exec vitest", "pnpm lint", … which the flag-first original never matches. You call that a safe no-op, and for the hook it genuinely is. But Classification::Supported has two other consumers, and for them the no-op is a wrong answer.

Probed classify_command directly on both binaries, same inputs:

command	develop	this head	rtk rewrite on this head
pnpm -r lint	Unsupported(pnpm)	Supported(rtk lint, 84%)	(nothing)
pnpm -r exec eslint .	Unsupported(pnpm)	Supported(rtk lint, 84%)	(nothing)
pnpm --filter @app exec vitest run	Unsupported(pnpm)	Supported(rtk vitest, 99%)	(nothing)
pnpm -F web exec playwright test	Unsupported(pnpm)	Supported(rtk playwright, 94%)	(nothing)
pnpm -r exec tsc --noEmit	Unsupported(pnpm)	Supported(rtk tsc, 83%)	(nothing)
pnpm --filter @app exec prettier --write .	Unsupported(pnpm)	Supported(rtk prettier, 70%)	(nothing)
pnpm -w exec next build	Unsupported(pnpm)	Supported(rtk next, 87%)	(nothing)

Seven for seven: newly Supported, advertising 70–99% savings, and structurally un-rewritable. Unsupported was the honest answer and this replaces it with a number that can never be delivered. Consequences:

• src/analytics/session_cmd.rs:46 — count_rtk_commands counts anything Supported as adopted RTK usage. A monorepo session that runs pnpm --filter @app exec vitest run fifty times now reports those fifty as RTK-covered while the hook streamed all fifty raw.
• src/discover/mod.rs:421 — files them as Supported-but-uncovered, i.e. as missed opportunities carrying estimated_savings_pct of up to 99%. rtk discover exists to tell people what to route; this makes it recommend seven command families it cannot route.

The narrow fix keeps 100% of what this PR claims: only use the stripped form when the rule it matches is the rtk pnpm rule — every form in your own tables (install, list, ls, i, outdated, run) is that rule, so nothing you set out to fix is lost, and the tool rules go back to classifying exactly as they do on develop. Making the rewrite side apply the same normalization would also close it, but that's a bigger change and it isn't what your claim needs. Your call which — hence sending it back rather than patching it myself.

Optional — will not hold merge

1. test_rewrite_pnpm_unknown_flag_not_stripped passes for the wrong reason. It asserts on pnpm -x build, but build is not a routed subcommand, so it returns None whether or not -x was stripped. I widened PNPM_GLOBAL_OPT to strip any -[a-z] — the exact false-positive the fixed-set design exists to prevent — and the whole suite stayed green (3423 passed, 0 failed). So the guard your "Fix" section rests on is currently unasserted. pnpm -x install is the load-bearing case: under that mutation it yields Some("rtk pnpm -x install"), which reaches clap and dies (ERROR Unknown option: 'x', exit 1). Verified the one-liner below passes on your code and fails on the mutation:

   assert_eq!(rewrite_command_no_prefixes("pnpm -x install", &[]), None);

   Yours to fold in with the blocker fix — not worth a separate commit from me.

2. Whitespace tolerance differs between the bare and flag-first forms. &cmd[5..] after starts_with("pnpm ") leaves the extra space at the front of the slice, and PNPM_GLOBAL_OPT is ^-anchored, so pnpm -r install (two spaces) doesn't rewrite while pnpm install does. cmd.strip_prefix("pnpm").map(str::trim_start) closes it. Safe no-op, lost savings only. Worth noting the mechanism is shared with strip_git_global_opts but the outcome isn't — git -C /tmp status still rewrites on develop, via a different rule — so this isn't simply inherited behaviour.

Checked and correct — no need to re-verify

• Flag position really is immaterial. pnpm -r outdated --format json vs pnpm outdated --format json -r, and the two list --json orders, are byte-identical on 9.15.4. Appending after the subcommand is safe; the pnpm_global_flags doc comment is accurate.
• Recursive JSON shape doesn't break the parsers. pnpm -r list --json returns an array of n projects where the non-recursive form returns one. PnpmListParser flattens all of it — rtk pnpm -r list reports all 5 packages across 3 projects, nothing dropped. The likeliest spot for a silent wrong answer, and it is clean.
• Every rewritten spelling parses and runs: -r, -w, --recursive, --workspace-root, -r -w, -F @app, --filter=@app, --filter @app run build, -r install --frozen-lockfile, -r i, -r ls.
• --filter scoping is real: --filter @app/app list → only @app/app + is-odd; -F @app/lib list → only @app/lib + is-number.
• Unstripped forms degrade safely, never wrongly: -rw, -F@app, -C /tmp, --dir /tmp all → raw pnpm.
• Bare forms byte-identical to develop.
• Gates: fmt, clippy (0 warnings), cargo test --all (3423 passed) on the head; and on the head merged onto latest develop (79b96a44) — merges cleanly, 3707 passed, 0 clippy warnings.
• Startup 7 ms, unchanged; the new LazyLock regex is guarded by starts_with("pnpm ").
• Mutation-tested: dropping -w → test_rewrite_pnpm_workspace_root_install fails; stripping any -\S+ → 3 filter tests fail; swapping -r/-w order → test_merge_recursive_workspace_root_ordering fails.

Filed as follow-ups

• #2658 (fix(pnpm): propagate exit code from pnpm outdated) — evidence comment, not a duplicate. run_outdated ends in a hard Ok(0). Invisible until now because non-recursive pnpm outdated itself exits 0 even with outdated deps — but pnpm -r outdated exits 1, and this PR is what first routes that command into rtk. Root cause is in src/cmds/js/pnpm_cmd.rs, untouched here, so not this PR's to fix.
• #3838 (exclude_commands is matched on the raw command, so git -C <path> escapes it) — evidence comment. exclude_commands = ["pnpm install"] will not cover pnpm -r install once this PR routes it, for exactly the reason #3838 describes for git. Confirmed the same gap on develop for git -C /tmp status; the fix is central, not here.
• #4004 — npm and bun have the identical flag-first gap this PR closes for pnpm: npm -w @app run build, npm --workspace @app run build, npm --workspaces run build, bun --filter '*' test, bun --cwd packages/app test, all unrewritten on develop and on this head. Searched open and closed under two wordings, nothing tracked it; filed as #4004.

Hypotheses built and dropped

• Recursive list/outdated JSON shape change produces a silent wrong answer — dropped, parsers handle the array.
• Appending -r/-w after the subcommand changes install scope — dropped, byte-identical output in both positions.
• pnpm -r i / pnpm -r ls fall to unfiltered passthrough — true, but pnpm i / pnpm ls do the same on develop; pre-existing (#3516 territory).
• A doc or awareness file enumerates pnpm invocation forms — dropped, no doc lists --filter or the flag forms.

Your questions

• "Ready for a first review whenever you have a moment." → Done. The rebase is clean: I re-merged your head onto latest develop (79b96a44) independently, no conflict, 3707 tests green — your Typecheck / merge_pnpm_args_os resolution is correct.

Next

One change: gate the strip so it only applies when the match is the rtk pnpm rule, so pnpm -r lint and the pnpm <flags> exec <tool> forms classify as they do on develop. Fold in the pnpm -x install assertion while you're there. Everything else here is verified and stands.

Rounds: 1/3. Threads: 1 open (blocking item above), 0 resolved this round.

[breisnerlopez]

Round 1 addressed in 6c84b82.

Blocker — classify/rewrite divergence (registry.rs). Took the narrow route you preferred: the strip is now adopted only when the stripped form routes to the rtk pnpm rule itself (matches_pnpm_rule, keyed on rtk_cmd == "rtk pnpm"). Every form in the tables (install/list/ls/i/outdated/run) is that rule, so nothing the PR claims is lost; the tool rules reachable via pnpm exec/pnpm run go back to classifying exactly as on develop. Probed the seven from your table on the built binary — all seven rtk rewrite to nothing and now classify Unsupported, matching develop; the install/list/outdated forms still rewrite. Added test_classify_pnpm_flag_first_tool_stays_unsupported (load-bearing: fails without the gate) plus test_classify_pnpm_flag_first_install_still_supported.

Optional 1 — pnpm -x install. Folded in: test_rewrite_pnpm_unknown_flag_not_stripped now also asserts rewrite_command_no_prefixes("pnpm -x install", &[]) == None. Confirmed it fails under the widen-to--\S+ mutation and passes here.

Optional 2 — whitespace tolerance. Folded, but with a catch worth flagging: str::trim_start / char::is_whitespace tolerates any whitespace, and that reintroduces your exact divergence for a tab separator — strip_pnpm_global_opts would strip pnpm\t-r install (→ classify Supported(rtk pnpm)) while the rewrite's strip_word_prefix only accepts a literal ASCII space (→ None). So I kept the boundary at a single ASCII space (starts_with("pnpm ")) and only trim_start the remainder, which closes the pnpm -r install case you cited without widening the separator. Added test_pnpm_tab_separator_no_classify_rewrite_divergence, which asserts both sides agree (Unsupported + None) for the tab form.

Gates on the head: fmt, clippy -D warnings, cargo test --bins 3427 passed / 0 failed. Not a duplicate of #3676 (bare script forms) — complementary as you noted; whichever lands second rebases.

[KuSh]

Round 2 of 3 — APPROVED, follow-ups filed

Claim: pnpm commands whose global options precede the subcommand (pnpm -r install, pnpm --filter @app list, pnpm -w install) are never rewritten, because the rule is anchored right after pnpm — strip a fixed set of global options for classification only, and forward -r/-w through rtk pnpm.

Scope: accept. (round 1, frozen — not reopened)

Ran: reference 0fe63b39 (round-1 head) and current develop 5e0f92cd, each built in its own CARGO_TARGET_DIR, all binaries md5-distinct. Round-1's 31-form rewrite matrix rerun; classify_command probed directly on develop vs this head; a 6528-case differential fuzz over pnpm spellings; both new tests mutation-checked; real pnpm 9.15.4 workspace end to end; LC_ALL=C.
Savings unchanged: pnpm -r outdated 97%, pnpm -r list 66% — both clear the 20% floor (CONTRIBUTING.md). Startup 6 ms.

CI @6c84b823: all green — fmt, clippy, doc review, test presence, benchmark, Security Scan, semgrep, test on ubuntu / macos / windows. CLA signed.

Blocks merge (0 — round-1 blocker verified fixed)

Round 1's blocker is closed. matches_pnpm_rule is the right narrow gate, and it holds under measurement rather than by construction. Probing classify_command directly, develop vs this head:

command	develop 5e0f92cd	head 6c84b823
pnpm -r lint	Unsupported(pnpm)	Unsupported(pnpm)
pnpm -r exec eslint .	Unsupported(pnpm)	Unsupported(pnpm)
pnpm --filter @app exec vitest run	Unsupported(pnpm)	Unsupported(pnpm)
pnpm -F web exec playwright test	Unsupported(pnpm)	Unsupported(pnpm)
pnpm -r exec tsc --noEmit	Unsupported(pnpm)	Unsupported(pnpm)
pnpm --filter @app exec prettier --write .	Unsupported(pnpm)	Unsupported(pnpm)
pnpm -w exec next build	Unsupported(pnpm)	Unsupported(pnpm)
pnpm -r install / --filter @app list / -w install / -r outdated	Unsupported(pnpm)	Supported(rtk pnpm, 80%)

Exact parity with develop on all seven, and the four forms the PR exists for are Supported. Nothing the PR claims was lost.

I did not take the class on trust. Generated 6528 pnpm spellings (6 separators × 16 flag forms × 17 subcommands × 4 tails) and asserted both directions of the property — never Supported(rtk pnpm) with a None rewrite, and never an rtk pnpm … rewrite without the matching classification. This head introduces zero new divergences: the 64 that trip the property are byte-identical to the 64 on develop (all tab-separated bare forms, see follow-up below).

Both new tests are load-bearing, confirmed by mutation:

• removing the gate (let cmd_normalized = cmd_pnpm_stripped;) → test_classify_pnpm_flag_first_tool_stays_unsupported fails with exactly the round-1 symptom: Supported { rtk_equivalent: "rtk lint", …, estimated_savings_pct: 84.0 }.
• widening the boundary to char::is_whitespace → test_pnpm_tab_separator_no_classify_rewrite_divergence fails with Supported { rtk_equivalent: "rtk pnpm", …, 80.0 }.

Optional — none outstanding

Both round-1 optionals were folded in and verified. The pnpm -x install assertion is present and load-bearing (round 1 showed the whole suite stayed green without it). The whitespace fix works: pnpm -r install → rtk pnpm -r install, which executes correctly (ok, exit 0); pnpm install and every bare form are unchanged.

Checked and correct — no need to re-verify

• No regression against round 1: all 31 matrix forms identical except the one intended change (pnpm -r install now rewrites).
• Real producer still correct: -r list reports all 5 packages across 3 projects; --filter @app/lib list scopes to 2; -r outdated reports both; -r install clean. Exit codes unchanged.
• Gates: fmt, clippy --all-targets (0 warnings), cargo test --all 3537 passed on the head; and on the head merged onto latest develop 5e0f92cd — merges cleanly (including across #3782's edition-2024 bump), 0 clippy warnings, 3801 passed, 0 failed.
• Build hygiene: reference, head and merge each had their own CARGO_TARGET_DIR; md5s distinct, so the before/after above is real evidence and not one binary shown twice.

Filed as follow-ups

• #4100 — strip_word_prefix accepts only a literal ASCII space (cmd.as_bytes()[prefix.len()] == b' ', registry.rs:1930) while the rule patterns use \s+, so a tab-separated command classifies Supported and never rewrites. Pre-existing and not pnpm-specific: git\tstatus, cargo\tbuild, npm\trun build all return nothing on develop today. Same class as #3995 (closed 2026-09-13, which fixed the absolute-path arm); this is the whitespace arm. 64 spellings in my fuzz, identical count on develop and on this head. Out of scope here — this PR neither adds to it nor can anchor a fix on an added line, and your decision to keep the boundary at a single ASCII space is what kept it from growing. Filed as #4100, with the note that a tab is valid shell (bash -xc $'git\t--version' traces as + git --version), so the classify side is the correct one and strip_word_prefix is the narrow one — and that in practice the frequency is low.

Hypotheses built and dropped

• The trim_start on the remainder reintroduces the divergence for pnpm \t-r install (space then tab) — dropped. It classifies Supported(rtk pnpm) and rewrites to rtk pnpm -r install, so the two agree. Your tab reasoning holds for the separator you guarded, and this adjacent case is consistent too.
• matches_pnpm_rule's next_back() might disagree with classify_command's own matches.last() — dropped, same index on the same input; the fuzz would have caught a mismatch.
• Gating on the rule could strand a form the PR claims — dropped, all four claimed families stay Supported(rtk pnpm).

Your questions

• "Optional 2 — … str::trim_start / char::is_whitespace tolerates any whitespace, and that reintroduces your exact divergence for a tab separator … So I kept the boundary at a single ASCII space." → Right call, and I verified it rather than taking it: under the widened boundary your new test fails with Supported(rtk pnpm, 80%) on pnpm\t-r install. Keeping the strip's boundary identical to strip_word_prefix's is the property that matters, and it is now asserted. The pre-existing tab gap it exposed is filed separately, not charged to this PR.
• "Not a duplicate of #3676 (bare script forms) — complementary as you noted; whichever lands second rebases." → Agreed, unchanged from round 1.

Next

Nothing — approving. The tab-separator gap goes to its own issue; it is develop's, not this PR's.

Rounds: 2/3. Threads: 0 open, 1 resolved this round (the round-1 blocker).