Repository navigation
fix(ast-grep): account for every match line and stop capturing other subcommands - #4121
Conversation
…subcommands The caps counted the lines they held back inside a shown file, but a file the total cap skipped outright was reported as a file and nothing else: 255 of 387 lines on the module's own fixture were in no tally at all, with no way to read them. Count those lines too, and point at the full output with the same `[full output: …]` handle every other capping filter emits -- decided on the text that handle is part of, so a search that ends up printing raw never leaves a stored file with nothing pointing at it. Only `run` produces the `path:line:content` shape this module parses. `scan --report-style short` happens to parse as well and was being capped as if it were a match list, and `lsp` speaks a protocol over the stdin that capturing closes, so it served an editor an empty document. The subcommand is now read in two passes -- the options that may precede one, then `run`'s own grammar once the line is known to be a `run` -- and a line that names any other subcommand, or a `--stdin` run, goes through the passthrough, which inherits stdin. A line this module cannot identify is not filtered: `--color always lsp` reading as a `run` is what closed that stdin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 Automated PR Analysis
SummaryFixes three regressions in the ast-grep command filter introduced by a prior PR: match lines dropped by the total cap were no longer counted or pointed to via a recovery hint, Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
pszymkowiak
left a comment
There was a problem hiding this comment.
Reviewed by building the branch and running both binaries against the module's fixture (387 match lines, 32 files) through a stub ast-grep, plus real ast-grep 0.45.3 on the worktree.
Verified
- Accounting: develop shows 50 lines + per-file notes for 82, trailer names 20 files and zero lines, no archive: 255 lines exist nowhere. This PR: same 50 + 82, trailer
… 255 more match line(s) in 20 more file(s) not shown,[full output: …]whose archive is byte-identical to the raw (cmp). 50 + 82 + 255 = 387, 12 + 20 = 32, from the actual output. Real ast-grep: 50 + 30 + 118 = 198 over 34 files,rtk recallcontent matches a fresh run. scan --report-style short: 60 lines capped to 51 on develop, 60 through untouched on this PR.- stdin: develop runs everything through
exec_capture(Stdio::null()); this PR routes non-runshapes torun_passthroughwith inherited stdin.lsp,-c cfg lsp,-p X --stdin,outline --stdinall receive the piped bytes now. - Near-band case (6 lines, compaction cannot pay for its hint) prints raw and writes no archive; 60-line case writes exactly one. Exit 3 and stderr propagate on both paths. Fixture savings 84 % with the hint included.
Follow-ups, none blocking
OTHER_SUBCOMMANDS(ast_grep_cmd.rs:38) was not checked against the binary:docsis not an ast-grep subcommand (0.45.3: "unrecognized subcommand 'docs'"), whileoutlineandhelpare missing. Cost is compression only:rtk ast-grep -p X docs(a directory nameddocs) goes unfiltered, 387 lines instead of 60.outlinewithout--stdinis captured and rescued byunparsed_signal. FEATURES.md documents the phantom entry too.global_takes_valuelists--coloras a pre-subcommand global option; perast-grep --helponly-c/--config,-h,-Vare. Harmless (moves toward passthrough).- The description says the store decision is "taken on the text the hint is already part of"; the code reserves a fixed
HINT_RESERVE= 256 B before the hint exists. Fine in practice (hints are 40–70 B), but a customtee_directoryover ~230 chars plus a search within 256 B of the never_worse band could still orphan an archive. Either check the rendered hint length or make the description match. ast_grep_cmd.rs:266-282: the same comment paragraph appears twice.
Approving. CI green on all three OSes.
Fixes three regressions from the release review on #3979, all introduced by #3857, which added
src/cmds/system/ast_grep_cmd.rs.Part of a set of six, one per originating PR: #3681, #3552, #3265, #3772, #3857 (this), #3941.
1 — 87 % of matches dropped with no way to read them
The caps counted the lines they held back inside a shown file, but a file the total cap skipped outright was reported as a file and nothing else. On the module's own fixture (387 match lines across 32 files):
Now those lines are counted too, and the full output goes behind the
[full output: …]handle thatsrc/cmds/README.mdrequires of any capping filter:50 + 82 + 255 = 387.
filter_ast_grepreturns ahidden_linescount so the I/O stays inrun().The decision to store is taken on the text the hint is already part of. Deciding before appending it left a band where the hint tipped
never_worseback to raw: the archive was written, raw was printed, and the file sat there with nothing pointing at it — having evicted an archive another caller still needed.2 —
scan --report-style shortwas captured despite the docsThat style emits
path:line:col: message, which matches the module'spath:line:contentregex, so a 60-finding lint report was grouped and capped to 18 lines — while the module header andFEATURES.mdboth saidscanpasses through.3 —
lspran with stdin closedexec_capturehands the child a null stdin, soast-grep lspserved an editor an empty document:The routing fix
Only
runproduces the shape this module parses. Every other subcommand, and arun --stdin, now goes throughrunner::run_passthrough, which inherits stdin.Telling a subcommand from a flag's value needs a grammar, read in two passes, because which options take a value depends on the subcommand that has not been identified yet:
--config,--color,-c) — the first free positional this finds is the subcommandrun,run's own grammar, re-readWithout the second pass,
run -p scan src/took its pattern for thescansubcommand and gave up the filter entirely.A line this module cannot identify is not filtered: if the first positional is neither
runnor a known subcommand, an option whose arity is unknown may have claimed the subcommand's place, so any of the other names anywhere on the line disqualifies it.--color always lspreading as arunis what closed that stdin. The cost is a pattern or path spelled exactly like a subcommand going unfiltered — compression, not correctness.--jsonis now read from tokens too, so a path named--jsonpast--no longer disables the filter.Verification
15 unit tests, including: every line printed or counted on the real fixture; the trailer's line and file counts; a subcommand behind
--color/--heading/--no-ignore/--globs/--threads; a run flag's value not read as a subcommand; paths past--;--jsonfrom tokens; and a capped output always carrying a route to the rest.Verified end to end against a stub:
lspgets the editor's bytes with and without preceding flags,scan --report-style shortpasses 60 lines through untouched,runstill filters and still emits the handle, and a small search writes no archive at all.cargo fmt --all,cargo clippy --all-targetsandcargo test --allare green, rebased on currentdevelop.🤖 Generated with Claude Code