Repository navigation
fix(ruff): route flag-first implicit check invocations to check (#3927) - #3968
anujsingh-cse wants to merge 5 commits into
Conversation
📊 Automated PR Analysis
SummaryFixes rtk's argument routing so flag-first implicit Review Checklist
Linked issues: #3927 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
f263b9f to
ae272dc
Compare
|
Rebased onto latest \develop\ so it's up to date and mergeable. Happy to adjust anything reviewers would like. |
ae272dc to
bfd38ff
Compare
|
Rebased onto current develop and fixed the CI failure. The branch was 300+ commits behind a develop that has since repaired broken rustdoc intra-doc links (the rustdoc gate failed on the stale base); rustfmt also rewrapped one test literal to current style. All local gates pass against current develop: |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Verified bfd38ff with real Ruff 0.17.0 on Linux: nine scenarios, 27 raw/parent/head executions. The intended implicit --select F401 bad.py and -q clean.py routes are fixed, and ordinary check, help format, and --version still work. However, supported global-option-before-subcommand commands regress (see inline reproduction), so I recommend preserving those routes before merging.
Validation: 12 focused Ruff unit tests, cargo fmt --all -- --check, and cargo clippy --all-targets -- -D warnings passed. This is focused local verification, not full-suite or cross-platform validation. Related #3933 also covers global-option routing; please coordinate the shared routing changes rather than integrate competing parsers independently.
|
Fixed in 14c891c. The routing now tokenizes the pre-subcommand arguments instead of finding the first token without a
The boolean-predicate tests are replaced by complete CLI cases asserting the full forwarded argument vector — the gap you pointed out is exactly what let the forwarding regressions through. Verified with real Ruff 0.17.0 through the built rtk binary (your three reproductions first):
17 ruff_cmd unit tests (six of them the new pinned CLI plans), |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Re-tested exact head 14c891c with Ruff 0.17.0 on Linux: 12 scenarios / 36 raw-previous-head executions. The earlier --isolated check, --config ... format --check, and --config ... --version forwarding regressions are fixed. Implicit checks, split --color, equals-form config and the -- path separator also keep the expected exit codes.
One new output regression remains: rtk ruff help format is no longer complete. Raw Ruff and the previous reviewed head bfd38ff both return the same 4,884 bytes; this head returns 2,001 bytes ending in ..., losing --range, file selection and the remaining global options. The new effective-subcommand decision sends help through the existing generic truncation branch. Please preserve the complete non-check/help output and pin the stdout bytes, not only the forwarded argument vector. This is the output half of the #3933 coordination point.
The new routing parser should also use the shared arg_tokenizer API, as required by .claude/rules/rust-patterns.md rule 6, rather than its own flag/value scanner. The narrow fix should retain the now-correct forwarding cases above.
17 focused Ruff unit tests, fmt and clippy with warnings denied pass. No full-suite or cross-platform run in this review.
|
Both points addressed in d39d176. Shared tokenizer. Help output regression. The complete passthrough for Verified against real Ruff 0.17.0 through the built binary:
20 ruff_cmd unit tests, On #3933: with the output half now here and the routing half subsuming the first-argument rule (including its |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Re-reviewed d39d176 with real Ruff 0.17.0 on Linux. The prior global-option forwarding and full-help blockers are cleared, and routing now uses the shared tokenizer.
Sixteen CLI scenarios / 48 raw-previous-head comparisons cover implicit checks, explicit check after global flags, config/color value forms, format checks, complete help/version output, separators and a 55-diagnostic run. Full help format stdout now matches raw Ruff, including the previously missing --range section; the previous head still truncates it. Twenty Ruff unit tests, fmt and clippy (all targets, warnings denied) pass.
One pre-existing CLI boundary gap remains on both previous and current heads: with a clean file named check, rtk ruff -- check checks the entire directory and exits 1 where ruff check -- check exits 0. The leading separator is lost before the routing helper receives the arguments. This is outside the changed router and does not hold this fix; the direct-router separator tests do not establish that end-to-end case.
The inline encoding note is a small cleanup, not a remaining blocker for this routing/help fix. I did not run the full repository suite, rustdoc or other platforms. #3933 overlaps this effective-subcommand routing; please coordinate integration rather than landing the two independently.
|
Restored in 6345475 — the ellipsis was mangled by a Windows-1252 round-trip in my brace-cleanup edit, which also corrupted the test expectation to match. Both now carry the proper U+2026; verified no mis-encoded bytes remain in the file and all 20 ruff_cmd tests, fmt, and clippy still pass. Thanks for the thorough re-review across the sixteen scenarios. |
KuSh
left a comment
There was a problem hiding this comment.
Round 1 — CHANGES REQUESTED
Claim: rtk ruff finds the subcommand through the shared tokenizer, so flag-first implicit checks (rtk ruff --select F401 src, #3927) and global options before an explicit subcommand route to the right ruff subcommand, and help/version output stays complete.
Scope: accept. This lands before #3933 (its help format case is covered here, and #3928 closes with this PR) and before #3912 (rename only, rebases on top).
Ran: real ruff 0.16.6, LC_ALL=C, a 64-module package plus a 301-module package for analyze graph. 39 invocations × native ruff / develop 5ab6291 / this head merged onto develop, comparing exit codes and stdout bytes. 586 generated invocations (global options × subcommands; global + check options + paths) checked against what native ruff accepts, with the forwarded argv captured: 0 mismatches (develop: 367). Invariants: over-cap + accounting (50 shown + 192 counted = 242 issues; 50 + 252 = 302), every decision, under the hook (the rewrite rule matches ruff check|format only and is unchanged). Savings on check runs: 95–96% (65177 → 3339 B, 87599 → 3229 B) vs the 20% floor (CONTRIBUTING.md).
Pins: 16 decisions, 25 mutations, 8 survived: 4 router decisions (item 2 below), 2 equivalent, and 2 that only move the 2000-char passthrough cap (#4655), which I am not holding this PR on.
Matrix: executed this round; pin check: executed this round
CI @6345475: no run. The branch conflicts with develop in src/cmds/python/ruff_cmd.rs (against #3942), so there is no merge ref to build, and the runs on the last four heads ended action_required. CI runs after the rebase.
Blocks merge (2)
-
src/cmds/python/ruff_cmd.rs:143— the router reads check-only options as if they were global (3 sites: removing the explicitcheck, first-positional subcommand detection, the help/version scan intop_level_request; the first is the one that must change). The grammar knows--config/--coloras value options. In the flag-first form this PR adds, a check option's value therefore becomes a free positional. When that value ischeck, it is taken for the explicit subcommand and removed, and the option takes the next argument instead:rtk ruff --output-file check keep.pyrunsruff check --output-format=json --output-file keep.py:keep.pyis overwritten with the JSON report for the whole directory. develop: exit 2, file intact. Nativeruff check --output-file check keep.pywrites the report tocheck.rtk ruff --extend-exclude check src/pkg/bad.pyreports 601 issues in 361 files, withbad.pyexcluded. Native: bad.py's 2 issues. develop: exit 2.rtk ruff -qhprints check's help behindRuff check (JSON parse failed: …). develop and native print the top-level help (1977 B).top_level_requestcompares raw strings, so it misses a cluster (.claude/rules/rust-patterns.mdrule 6).
A correct fix: when an option outside ruff's global grammar (
--config,--color,--isolated,-v/--verbose,-q/--quiet,-s/--silent,-h/--help,-V/--version) appears before the first free positional, the call is the implicit-check form. No later word is the subcommand and nothing is removed. Help and version are read from the tokens. Rows it must pass: the three invocations above givekeep.pyintact with the report incheck, bad.py's 2 issues, and the 1977 B top-level help.--extend-exclude config src/pkg/bad.py(exit 2 on both sides today) then works too. The 39 rows and 586 cases above stay unchanged. Tests: plan assertions for these inputs. -
src/cmds/python/ruff_cmd.rs:164— unpinned behaviour (router decisions). Each of these edits passes all 28 ruff tests:- drop
"color"fromRUFF_GLOBAL_VALUE_OPTIONS: no test uses--color. Test must assert:--color never check a.py→check --output-format=json --color never a.py;--color never format --check a.pyforwarded verbatim. - replace the
return RuffRoute::Check { explicit_at: None }after a non-subcommand positional withcontinue(a later subcommand-named word wins). Test must assert:src format→check --output-format=json src format. - iterate
tokens.iter()instead ofbefore_dashdash(&tokens). Test must assert:--isolated -- format→check --output-format=json --isolated -- format. A--in the middle reaches the router; a leading one does not (follow-ups). false &&in front of the post-loopdashdash_index(..)check. It only differs from the flags-only fallback when-h/--help/-V/--versionis also present: pin it with such an input, or drop it.
Each mutation must turn a test red.
- drop
Optional
run(),plan_ruff_commandandfilter_ruff_outputeach re-route and re-scan for--output-format. Computing the route once inrun()and moving it into the closure keeps the three in step.-vprintsRunning: ruff <user args>, not the argv rtk actually runs (unchanged from develop). With the plan in hand, printing the plan would show the injectedcheck.
Filed as follow-ups
- #4701
rtk ruff check --statisticsprints statistics JSON where ruff prints a table, larger than native output. Pre-existing forcheck --statistics; this PR adds the--statisticsspelling. - #4702
rtk ruff -- <path>drops the leading--:rtk ruff -- checklints the whole directory, whereruff check -- checklints the filecheck. Pre-existing. - #4655, comment added: with this PR,
rtk ruff --output-format concise srcreaches the same 2000-char cap asrtk ruff check --output-format concise src(byte-identical output);generate-shell-completion bashis cut from 311361 B to 2001 B.
Checked and correct — no need to re-verify
- #3927's forms:
--select F401 src(summary, exit 1 = native),--fix src,--statistics src,-q src,-s src.--fixor--statisticsalone append.. - Global option before the subcommand:
--isolated check srcand--config ruff.toml check srcgive the JSON summary.--config ruff.toml format --check srcand--config ruff.toml --versionare forwarded verbatim, with exit codes equal to native. help format(4889 B),help check(6297 B),--help,-h,-V,version: byte-for-byte equal to native.… +192 morerenders as UTF-8.- Over-cap check output: lines shown + the count in the notice = total.
Your questions
None open since the last push; the 13 Oct comment closed the ellipsis note.
Next
Rebase onto develop. The conflict with #3942 resolves by keeping its exemption: in ruff_output_is_complete, the Subcommand arm also returns true for is_analyze_graph_invocation(args), and #3942's two tests call filter_ruff_output. With that, analyze graph big returns all 14816 B, as native does, and the full suite passes on the merge. Then item 1 and the tests in items 1 and 2. After that, this merges. Global options before analyze graph are #4654, outside this claim.
Rounds: 1. Threads: 2 open (items 1–2, waiting on the author), 3 resolved this round (routing, help output and ellipsis, fixed in 14c891c, d39d176 and 6345475).
| if RUFF_SUBCOMMANDS.contains(&token.text) { | ||
| if token.text == "check" { | ||
| return RuffRoute::Check { | ||
| explicit_at: Some(token.source_index), |
There was a problem hiding this comment.
Blocks merge (1): a check option's value is taken for the explicit check. --output-file, --extend-exclude, --select and so on are unknown to the grammar, so their value is a free positional. When that value is check, it is taken for the subcommand and removed:
rtk ruff --output-file check keep.pyrunsruff check --output-format=json --output-file keep.pyand overwriteskeep.pywith the report. develop: exit 2, file intact.rtk ruff --extend-exclude check src/pkg/bad.pylints 361 files withbad.pyexcluded. Native: bad.py's 2 issues.
An option outside ruff's global grammar before the first free positional should make this the implicit-check form: no later word is the subcommand, nothing is removed. The top_level_request raw scan belongs to the same fix, read from tokens: rtk ruff -qh today prints check's help behind a JSON-parse banner, where develop and native print the top-level help. The rows are in the review body.
| /// (`ruff --help`, 0.17: `--config` and `--color`). Every other global token | ||
| /// is a flag; rule-selection options belong to the subcommand and only | ||
| /// appear once the routing decision has been made. | ||
| const RUFF_GLOBAL_VALUE_OPTIONS: &[&str] = &["config", "color"]; |
There was a problem hiding this comment.
Blocks merge (2): router decisions no test pins. Each of these passes all 28 ruff tests:
- drop
"color"here → needs--color never check a.py→check --output-format=json --color never a.py, and--color never format --check a.pyforwarded verbatim continueinstead of returningCheckafter a non-subcommand positional → needssrc format→check --output-format=json src formattokens.iter()instead ofbefore_dashdash(&tokens)→ needs--isolated -- format→check --output-format=json --isolated -- formatfalse &&before the post-loopdashdash_indexcheck → pin with-h/--versionpresent, or drop it
Each mutation must turn a test red.
Finding the first token without a `-` misrouted invocations that put a global option before the subcommand: `--config ruff.toml` fed its value to the routing decision, so `--config ruff.toml format --check` was treated as an implicit check and rebuilt as one, and an explicit `check` after `--isolated` was forwarded as a path. Route on a real pre-subcommand scan instead. Only `--config` and `--color` consume a value (ruff 0.17 `--help`); every other global token is a flag, and the first bare token decides the route — a known subcommand routes, anything else is a path and the run is an implicit check. An explicit `check` is consumed at its actual position, and flag-only runs keep the top-level requests (`--version`, `--help`) while others stay implicit checks. `is_format` now keys on the effective subcommand, so `help format` no longer reaches the format filter. The boolean-predicate tests are replaced by complete CLI cases that pin the full forwarded argument vector, which is what let the forwarding regressions slip through before.
Route through the shared `arg_tokenizer` (rust-patterns rule 6) instead of the hand-rolled scan: `tokenize_grammar` with a `Dialect::Posix` grammar and a `ruff_takes_value` predicate for the two value-taking global options, so an option value is consumed rather than read as the subcommand or a path. The first free positional before the `--` boundary decides the route, and a path after the boundary keeps the run an implicit check. Help and version output regressed to the bounded passthrough once the format filter keyed on the effective subcommand. Extract the output choice into `filter_ruff_output` and keep `help`/`version` requests and top-level flags complete — raw Ruff returns every byte, and so does rtk — while other subcommands keep the bounded passthrough. Pin the stdout bytes for the complete routes instead of only the forwarded argument vector.
A Windows-1252 round-trip mangled the ellipsis in the '+N more' cap line and in its test expectation to the mojibake byte sequence. Restore the proper U+2026 in both.
A check-only option's value looked like a free positional to the router, so `--output-file check keep.py` removed that value as the explicit subcommand and `--output-file` consumed `keep.py`. Route the whole invocation as an implicit check whenever an option outside ruff's global grammar appears before the first free positional: nothing is removed and no later word is treated as the subcommand. Read help and version from tokenized flags so a cluster such as `-qh` stays top-level, and drop the redundant post-loop `--` fallback. Add plan pins for every router decision named in the review.
6345475 to
211f7cf
Compare
|
Rebased onto develop ( #3942 conflict. Resolved as you specified: the Item 1 — check-only options. The router now scans the tokens before the first free positional against ruff's global grammar ( Item 2 — pins. Added plan assertions for every row. Mutation check, each applied locally and each observed red:
Verification (Windows, ruff 0.16.2):
|
|
The completion-only supplement below is now available as a reviewable PR against this PR's source branch. Rechecked revised head The completion-output follow-up from #4655 still needs an adaptation to this new head: all five shell generators return only 2,001 bytes. The attached additive patch applies directly to Validation:
This is the completion-only adaptation, not the full #4654/#4655 selected-format integration. It neither opens that dependent PR before this one lands nor changes the outstanding maintainer review. Full Completion-only supplement for revised head 211f7cfdiff --git a/src/cmds/python/ruff_cmd.rs b/src/cmds/python/ruff_cmd.rs
index 6d63126b..753e230d 100644
--- a/src/cmds/python/ruff_cmd.rs
+++ b/src/cmds/python/ruff_cmd.rs
@@ -78,6 +78,18 @@ pub fn run(args: &[String], verbose: u8) -> Result<i32> {
eprintln!("Running: ruff {}", args.join(" "));
}
+ if ruff_output_is_complete(route, args) {
+ // Complete documentation can exceed the capture limit and may contain
+ // shell syntax whose bytes must be preserved exactly.
+ return runner::run(
+ cmd,
+ "ruff",
+ &display_args(args),
+ runner::RunMode::Passthrough,
+ runner::RunOptions::default(),
+ );
+ }
+
let owned_args = args.to_vec();
runner::run_filtered(
cmd,
@@ -123,12 +135,15 @@ fn filter_ruff_output(args: &[String], stdout: &str) -> String {
}
}
-/// Help and version requests must keep their full stdout.
+/// Help, version, graph JSON and generated shell completions must keep full stdout.
fn ruff_output_is_complete(route: RuffRoute, args: &[String]) -> bool {
match route {
RuffRoute::TopLevel => true,
RuffRoute::Subcommand { at } => {
- matches!(args[at].as_str(), "help" | "version") || is_analyze_graph_invocation(args)
+ matches!(
+ args[at].as_str(),
+ "help" | "version" | "generate-shell-completion"
+ ) || is_analyze_graph_invocation(args)
}
RuffRoute::Check { .. } => false,
}
@@ -533,6 +548,31 @@ fn compact_path(path: &str) -> String {
mod tests {
use super::*;
+ #[test]
+ fn completion_requests_are_complete_documents_only_when_routed() {
+ let args = |values: &[&str]| {
+ values
+ .iter()
+ .map(|value| value.to_string())
+ .collect::<Vec<_>>()
+ };
+ for args in [
+ args(&["generate-shell-completion", "bash"]),
+ args(&["--isolated", "generate-shell-completion", "bash"]),
+ args(&["--config", "ruff.toml", "generate-shell-completion", "bash"]),
+ ] {
+ let route = route_ruff_args(&args);
+ assert!(ruff_output_is_complete(route, &args));
+ }
+ for args in [
+ args(&["--config", "generate-shell-completion", "check", "src"]),
+ args(&["check", "--", "generate-shell-completion"]),
+ ] {
+ let route = route_ruff_args(&args);
+ assert!(!ruff_output_is_complete(route, &args));
+ }
+ }
+
#[test]
fn known_ruff_subcommands_do_not_route_through_check() {
for subcommand in RUFF_SUBCOMMANDS {
diff --git a/tests/ruff_passthrough_test.rs b/tests/ruff_passthrough_test.rs
index 7938faeb..14e5a368 100644
--- a/tests/ruff_passthrough_test.rs
+++ b/tests/ruff_passthrough_test.rs
@@ -88,3 +88,18 @@ fn other_passthrough_subcommands_keep_the_cap() {
context(&out)
);
}
+
+#[test]
+fn shell_completion_output_is_untruncated_and_byte_exact() {
+ let dir = common::tempdir();
+ let document = "complete -F _ruff ruff\n".repeat(512);
+ fake_ruff(
+ dir.path(),
+ &["generate-shell-completion", "bash"],
+ &document,
+ );
+
+ let out = rtk_ruff(dir.path(), &["generate-shell-completion", "bash"]);
+ assert_eq!(out.status.code(), Some(0), "{}", context(&out));
+ assert_eq!(out.stdout, document.as_bytes(), "{}", context(&out));
+} |
Closes #3927
Problem
rtk ruff --select F401 src(a flag-first implicitcheck) exits 2 with no output becauseis_check_invocationbails on any leading-. The flag is forwarded verbatim toruff, which rejects it because--selectbelongs toruff check, notruff. The error lands on stderr (dropped by the ruff filter), so the user sees nothing.Repro (ruff 0.16.6, LC_ALL=C):
Same for
--fix src,--statistics src,-q src.Fix
Change
is_check_invocationto find the first non-flag argument (skipping leading global flags) and route on that:check, or is not a known subcommand → implicitcheckWhen there is no positional argument at all, only the genuinely top-level flags (
--version/-V/--help/-h) stay top-level; every other flag-only invocation (rtk ruff --fix) is treated as an implicit check.Tests
top_level_flags_are_not_misclassified_as_pathswith-V/-hflag_first_implicit_check_routes_to_check—--select F401 src,--fix src,-q srcflag_only_implicit_check_routes_to_check—--fix,--statisticsExisting routing tests (
known_ruff_subcommands_do_not_route_through_check,paths_and_explicit_check_route_through_check) are unchanged and still pass.cargo fmt --checkandcargo clippy --all-targetsclean; 19 ruff tests pass.Verified end-to-end:
rtk ruff --select F401 <dir>now emits the ruled/filed summary (was empty exit 2), andrtk ruff --versionstill prints the version top-level.