Skip to content

fix(npx): dispatch unknown tools to npx instead of npm#1458

Merged
KuSh merged 4 commits intortk-ai:developfrom
tmchow:fix/npx-unknown-tools-to-npx
Apr 26, 2026
Merged

fix(npx): dispatch unknown tools to npx instead of npm#1458
KuSh merged 4 commits intortk-ai:developfrom
tmchow:fix/npx-unknown-tools-to-npx

Conversation

@tmchow
Copy link
Copy Markdown
Contributor

@tmchow tmchow commented Apr 22, 2026 •
edited
Loading

Summary

  • Fixes rtk npx dispatching to npm instead of npx for unrouted tools. The fallback arm of Commands::Npx called npm_cmd::run, so rtk npx cowsay hello tried to run npm cowsay hello and failed with "Missing script".
  • Replaces the fallback with a plain npx passthrough modeled on the existing prisma generic-subcommand arm a few lines up in the same file.
  • Preserves --skip-env (sets SKIP_ENV_VALIDATION=1 on the spawned process, matching what npm_cmd::run did).

Evidence

Demo: rtk npx cowsay hello before vs after the fix

Test plan

  • cargo fmt --all --check && cargo clippy --all-targets && cargo test pass locally (1679 tests, 0 failed). Pre-existing clippy warnings are unrelated to this change.
  • Manual: installed rtk 0.37.2 still reproduces with npm error code ENOENT. Rebuilt target/debug/rtk from this branch routes through npx and prints the cowsay output (see demo above).
  • Added test_npx_unknown_tool_passthrough next to the existing clap parsing tests to pin the parse-level contract.

Notes

Fixes #815

Compound Engineering

@tmchow
fix(npx): dispatch unknown tools to npx instead of npm
739df94 
The generic fallback arm of `Commands::Npx` called `npm_cmd::run`, which
executes `npm` instead of `npx`. This broke every non-routed npx
invocation. `rtk npx cowsay hello` failed with `npm error Missing
script: "cowsay"` because the rewrite hook sends `npx ...` through as
`rtk npx ...`, and `rtk npx` was then running npm.

Mirror the existing prisma generic-subcommand passthrough pattern:
resolve npx, forward all args, record a tracked passthrough, and
propagate the exit status.

Fixes rtk-ai#815
@tmchow tmchow mentioned this pull request Apr 22, 2026
Closed
@tmchow
Copy link
Copy Markdown
Contributor Author

tmchow commented Apr 22, 2026

Note: the failing benchmark test is pre-existing

KuSh
KuSh reviewed Apr 22, 2026
View reviewed changes
Comment thread src/main.rs
@tmchow
refactor(npx): use shared runner::run_passthrough_cmd helper
09aaf55 
Per @KuSh's review on rtk-ai#1458, the generic npx passthrough arm built
its own Command + tracking::TimedExecution inline, duplicating the
logic already covered by core::runner::run_passthrough. The sticking
point was `SKIP_ENV_VALIDATION`: the existing helper takes tool +
args and builds the Command itself, so there was no seam for a
caller-supplied env var.

Add a `run_passthrough_cmd` variant that accepts a pre-built Command
and refactor the existing `run_passthrough` to delegate to it. The
npx arm now builds a Command, sets SKIP_ENV_VALIDATION when
requested, and hands it to the shared helper. This also upgrades the
npx passthrough from a raw `cmd.status()` to the shared streaming +
tracking path that pnpm_cmd already uses, so verbose logging and
telemetry stay consistent across tools.
@tmchow
Copy link
Copy Markdown
Contributor Author

tmchow commented Apr 23, 2026

@KuSh all fixed up

KuSh
KuSh reviewed Apr 23, 2026
View reviewed changes
Comment thread src/main.rs Outdated
@tmchow
refactor(npx): consolidate npm/npx execution in npm_cmd
a5e6bf5 
Add `npm_cmd::exec` for npx and an internal `run_filtered` helper that
both `run` (npm) and `exec` (npx) share. The helper resolves the
command, applies args, honors SKIP_ENV_VALIDATION, and routes through
runner::run_filtered with the npm output filter. The npx fallback in
main.rs collapses to `npm_cmd::exec(&args, cli.verbose, cli.skip_env)?`.

Per @KuSh review on rtk-ai#1458.
@tmchow
Copy link
Copy Markdown
Contributor Author

tmchow commented Apr 25, 2026

Pushed a5e6bf5: moved everything into npm_cmd. Added a private run_filtered(name, args, verbose, skip_env) helper that builds the command, applies SKIP_ENV_VALIDATION, emits the verbose log, and routes through runner::run_filtered with filter_npm_output. pub fn run keeps its existing effective_args building (run-injection logic) and calls run_filtered("npm", ...). New pub fn exec just calls run_filtered("npx", args, verbose, skip_env). The fallback in main.rs is now npm_cmd::exec(&args, cli.verbose, cli.skip_env)?.

One call-out: I kept exec routing through runner::run_filtered (matching npm) instead of runner::run_passthrough_cmd, so both share streaming + tracking and the same output filter. If you want raw passthrough on npx, swap the runner call in run_filtered and pass a passthrough flag, happy to do that.

Verified: cargo test 1679 passing (no change), cargo clippy clean for the changed files. Manual: rebuilt target/release/rtk, rtk npx cowsay "ok" still works, -v shows Running: npx cowsay ok.

KuSh
KuSh requested changes Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@KuSh KuSh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one unneeded change left, otherwise LGTM!

Comment thread src/core/runner.rs
@KuSh KuSh self-assigned this Apr 25, 2026
This was referenced Apr 25, 2026
Closed
Closed
@tmchow
refactor(runner): drop unused run_passthrough_cmd helper
d8bfca8 
Restores src/core/runner.rs to the develop state. After a5e6bf5 moved
npx execution into npm_cmd::exec (routing through runner::run_filtered),
nothing calls run_passthrough_cmd anymore.

Addresses review feedback on PR rtk-ai#1458.
@KuSh KuSh merged commit 2ad1579 into rtk-ai:develop Apr 26, 2026
11 checks passed
KuSh pushed a commit that referenced this pull request Apr 26, 2026
@tmchow @KuSh
refactor(npx): use shared runner::run_passthrough_cmd helper
4d5c2fc 
Per @KuSh's review on #1458, the generic npx passthrough arm built
its own Command + tracking::TimedExecution inline, duplicating the
logic already covered by core::runner::run_passthrough. The sticking
point was `SKIP_ENV_VALIDATION`: the existing helper takes tool +
args and builds the Command itself, so there was no seam for a
caller-supplied env var.

Add a `run_passthrough_cmd` variant that accepts a pre-built Command
and refactor the existing `run_passthrough` to delegate to it. The
npx arm now builds a Command, sets SKIP_ENV_VALIDATION when
requested, and hands it to the shared helper. This also upgrades the
npx passthrough from a raw `cmd.status()` to the shared streaming +
tracking path that pnpm_cmd already uses, so verbose logging and
telemetry stay consistent across tools.
KuSh pushed a commit that referenced this pull request Apr 26, 2026
@tmchow @KuSh
refactor(npx): consolidate npm/npx execution in npm_cmd
614e562 
Add `npm_cmd::exec` for npx and an internal `run_filtered` helper that
both `run` (npm) and `exec` (npx) share. The helper resolves the
command, applies args, honors SKIP_ENV_VALIDATION, and routes through
runner::run_filtered with the npm output filter. The npx fallback in
main.rs collapses to `npm_cmd::exec(&args, cli.verbose, cli.skip_env)?`.

Per @KuSh review on #1458.
This was referenced Apr 26, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KuSh KuSh KuSh approved these changes

Assignees

@KuSh KuSh

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tmchow @KuSh