Repository navigation
fix(ruff): preserve analyze graph JSON output - #3942
Conversation
Signed-off-by: AxelRay <110029405+axelray-dev@users.noreply.github.com>
aa13c7e to
4c51f02
Compare
Protocol-zero-0
left a comment
There was a problem hiding this comment.
[P2] Recognize analyze graph after Ruff global options
Verified head 4c51f02f with real Ruff 0.17.0 on Linux. The bare ruff analyze graph big case is fixed: a 301-file package produces 12,628 bytes of valid JSON, identical through native Ruff and this head; the base truncates it to 2,001 bytes.
However, these supported forms still return the same invalid, truncated JSON on this head:
rtk ruff --isolated analyze graph big
rtk ruff --color never analyze graph big
rtk ruff --config ruff.toml analyze graph big
rtk ruff --config=ruff.toml analyze graph bigNative Ruff succeeds and emits the full graph for each. Both native and RTK exit 0, so a caller only discovers the corruption when parsing stdout. Moving --isolated after analyze graph makes the head preserve the graph correctly. A small graph below the cap also passes, which explains why routing-only/small-output tests would miss this.
Reproducer in an empty directory:
from pathlib import Path
p = Path("big")
p.mkdir(exist_ok=True)
(p / "__init__.py").write_text("")
for i in range(300):
(p / f"m{i:03d}.py").write_text(
f"from big import m{(i + 1) % 300:03d}\n"
)
Path("ruff.toml").write_text("line-length = 88\n")ruff --isolated analyze graph big | python3 -m json.tool >/dev/null
rtk ruff --isolated analyze graph big | python3 -m json.tool >/dev/nullThe first parses successfully; the second fails. is_analyze_graph_invocation only matches arguments 0 and 1, so the new exemption is bypassed by global options. Please identify the subcommand after global options, including value-taking --config/--color and their equals forms, using the shared argument-tokenization approach, and add a large-graph regression for those forms.
This is an inherited corruption case that the new fix still leaves open, rather than a regression introduced by this patch. Independent validation covered nine scenarios / 27 native-base-head executions: the forms above, a global option after the action, a small graph, a directory named format, and an invalid option. Exit statuses 0/2 matched native Ruff throughout. Focused Ruff tests, fmt and warnings-denied clippy passed. Full-suite, current-develop integration and other platforms were not tested.
Replay real `ruff analyze graph` and `ruff rule E501` output (ruff 0.16.6) through `rtk ruff`: the graph comes back whole and valid JSON, other passthrough subcommands keep the cap. The unit tests now use the same captured graph instead of a hand-built one, and pin which invocations the classifier exempts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
KuSh
left a comment
There was a problem hiding this comment.
Round 1 — APPROVED, follow-ups filed
Claim: rtk ruff analyze graph returns ruff's complete JSON instead of cutting it at the 2000-char passthrough cap.
Scope: accept (round 1, frozen)
Ran: ruff 0.16.6 on a 301-file package, 20 argument forms run under native ruff, merge-base f749546, this head, develop 83fea35 and this head merged on develop; invariants: non-default targets (--direction dependents, . default with a 27 KB graph, a package dir named format), over-cap input byte-compared with native, under the hook, every decision; savings 0% on analyze graph by design ("Correctness VS Token Savings" in CONTRIBUTING.md), every other path unchanged vs develop
Pins: 3 decisions (the analyze and graph conjuncts, the exemption limited to the graph) plus the run() wiring; on 4c51f02 6 mutated, 4 survived (both conjuncts, always exempt, wiring). With the fixup, 9 mutated (adding an uncapped else arm, an extra forwarded argument and the arguments forwarded as one), 1 survives: dropping the analyze conjunct, which no real invocation can tell apart (every ruff <X> graph with X ≠ analyze exits 2 with empty stdout)
Matrix: executed this round; pin check: executed this round
CI @5917f7ab5: all green (15 checks, test on ubuntu, macos and both windows targets)
Blocks merge (0)
Filed as follow-ups
- #4654
rtk ruffsubcommand detection ignores ruff's global options — outside this PR's claim, same on develop.--isolated,--config X,--color X,-qbeforeanalyze, or-v/--isolated/--config Xbetweenanalyzeandgraph, still return 2001 B of invalid JSON (8 forms). Same class as #3927 and #3928; the fix is one tokenized subcommand finder for all three detectors. - #4655
rtk ruffcuts SARIF, JUnit, GitLab, JSON Lines andrule --output-format=jsonat the passthrough cap — outside this PR's claim, same on develop.rule E501from #3929's body is listed there too.
Checked and correct — no need to re-verify
analyze graph big,analyze graph(27 KB),--direction dependents,-vafter the files: byte-identical to native and valid JSON; develop returns 2001 B of invalid JSON.- Small graph, missing directory,
--bogus(exit 2),analyze --help,analyze graph format,rule,check: same output and exit codes as develop. Exit codes match native on every row. - Hook:
rtk rewriteleavesruff analyze …unrewritten, as on develop, so the path is reached by an explicitrtk ruff. - Head merges cleanly onto develop 83fea35; fmt and clippy clean, tests green on the head and on the merge.
Fixup
Two commits on the branch:
- a merge of develop (clean, no conflicts): the branch base predates
tests/common/, which the new integration test needs; test(ruff): pin the analyze graph exemption on captured output: replays capturedruff analyze graphandruff rule E501output throughrun()with a stub ruff that checks the exact argv it receives; the unit test uses the captured graph instead of a hand-built one, and a table pins which invocations are exempt. Therule E501test pins where the exemption stops today; #4655 would change that deliberately.
Your questions
None from the author. The other review on this PR: its global-options finding reproduces (and extends to options between analyze and graph). It is the same on develop, so it is tracked in #4654 rather than held here.
Next
fixup 5917f7a pushed, code-reviewed: merges develop and adds tests pinning the exemption and the run() path on captured ruff output — approving.
Threads: 0 open, 0 resolved this round.
|
@Protocol-zero-0 thank you for the global-options repro. It reproduces exactly on 4c51f02, and the same miss also covers Every one of them returns the same 2001 bytes on develop, so this PR leaves those forms as they were rather than breaking them. |
Summary
ruff analyze graphinstead of applying the text passthrough cap.Fixes #3929
Test plan
cargo fmt --all -- --checkcargo clippy --all-targetscargo test --allanalyze graphoutput through rtk remained 17,403 bytes and parsed as JSON.