Repository navigation
fix(ruff): restore a leading -- before routing - #4727
Conversation
clap's trailing_var_arg strips a `--` that leads `args`, so `rtk ruff -- check` reached run() as `["check"]`, was routed as the check subcommand with no path, and linted the whole working directory. Restore it the way golangci and dotnet do, so the line reaches ruff as typed. Fixes rtk-ai#4702
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Independently checked b1701fb5 against base 5ab62913 with real Ruff 0.17.0 on Linux. This fixes the leading-separator routing issue from #4702 without duplicating a separator already retained by clap.
- 21 scenarios / 63 native, base and head processes.
ruff -- check,-- format,-- bad.py,-- --helpand the other leading-separator usage/error cases now match native exit 2 and both output streams byte-for-byte. In particular, base invokes formatting for-- formatand handles-- --helpas help; head correctly preserves their native errors. - Explicit
check -- check, an ordinary failing file, a file namedformat, a path containing a space,format --check -- <file>, normal check and implicit-check controls retain the expected exits and diagnostics. The PR's three argv-capture tests also pass. - 18 Ruff unit tests, 3 CLI integration tests, fmt and warnings-denied all-target clippy pass. No full-suite, cross-platform or pending-PR integration run is claimed.
Separate existing issue: check --isolated -- --statistics and check --isolated -- -- still append . and scan the directory (native exit 0, both base/head exit 1 in the fixture). This PR does not change that heuristic; I am keeping it separate from this leading--- fix and the routing work in #3968/#4654.
Approved for the scoped fix.
|
Thanks for the thorough check, especially the |
KuSh
left a comment
There was a problem hiding this comment.
Round 1 — APPROVED, follow-ups filed
Claim: rtk ruff restores a leading -- that clap strips, so rtk ruff -- <arg> reaches ruff as typed instead of being routed as a subcommand.
Scope: accept. #3933 and #3968 edit the same function and conflict with this by one line; this lands first and they rebase over it.
Ran: real ruff 0.16.6 (LC_ALL=C) × 15 argument forms × native / develop 5ab6291 / this head, on a 61-module project with files named check, format, -- and sub/; plus rtk -v/-vv before the subcommand, rtk rewrite on 4 forms, v1.0.1 on 4 rows. Invariants: under the hook, split streams (stderr byte-identical to ruff's on the 6 error rows), every decision. Savings unchanged (filter untouched): 86% on the 122-issue run, 28% on the 2-issue run, vs the 20% floor (CONTRIBUTING.md).
Pins: 1 decision (restore before routing), 2 mutated (restore removed; routing on the stripped args with the restored ones forwarded), 0 survived — both caught by ruff_leading_double_dash_reaches_ruff.
Matrix: executed this round; pin check: executed this round.
CI @b1701fb5: all green (15 pass, check-target skipped; run approved this round)
Blocks merge (0)
Filed as follow-ups
- #4752
rtk ruff checkappends.when every path is dash-prefixed or contains=(rtk ruff check a=b.pylints the whole tree) — outside this PR's hunks, same on v1.0.1 (also noted in the review above); filed. - #4753 A leading
--is dropped for 19 other passthrough commands (rtk pytest -- -k.pyruns the wrong tests) — the same bug class, outside this PR; filed. - #3025 — with this change,
rtk ruffalso reaches thestd::env::args()panic when rtk is invoked by an absolute path from a non-UTF-8 directory (same asrtk giton develop; invoked by name it does not happen). Comment added; #3224 covers it.
Checked and correct — no need to re-verify
rtk ruff -- check,-- format,-- sub,-- -- check,-- --version,--alone: exit code and stderr identical toruff. On develop-- checklinted 61 files and-- formatreformatted 61 files (also on v1.0.1), where ruff refuses both.check -- check,check -- format,check -- sub,format -- check,format --check -- check,--isolated -- check: same output and exit code as develop; clap already kept a--that follows an argument.rtk -v ruff -- checkandrtk -vv ruff -- check: the restore counts only the user region,Running: ruff -- check.ruff_cmd::runhas a single caller (Commands::Ruff), so the read of the process argv cannot see another command's arguments.cargo fmt,clippy --all-targets(0 warnings),cargo test --all(5033 passed) on this head, which already contains develop 5ab6291.
Next
Nothing — approving.
Rounds: 1. Threads: 0 open.
Summary
Commands::Ruffhanded its args toruff_cmd::runwithoutrestore_double_dash, so clap's leading--was gone before routing.rtk ruff -- checkbecame["check"], got routed as thechecksubcommand with no path, and linted the whole directory.ruff_cmd::runnow restores it first, the same waygolangci_cmdanddotnet_cmddo, so the line reaches ruff as typed.Fixes #4702
ruff -- checkruff check -- checkAll checks passed!, exit 0Ruff: No issues found, exit 0Ruff: No issues found, exit 0Test plan
cargo fmt --all && cargo clippy --all-targets && cargo testrtk <command>output inspected (table above, real ruff 0.16.6 on macOS)New
tests/ruff_double_dash_test.rsstubsruffon PATH and checks the exact argv it receives for-- check,check -- checkand--isolated -- check. The first one fails ondevelop(["check", "--output-format=json", "."]) and passes with the fix. The other two pin that a--already kept by clap isn't duplicated by the restore.One unrelated lib test failed once in five local runs of
cargo test --liband passed on rerun. It doesn't touchruff_cmd::run.