Repository navigation
feat(ast-grep): add ast-grep filter (rtk ast-grep) - #3857
Conversation
Groups matches by file, caps at 5/file and 50 total. --json passes through untouched. ~85% savings on a repo-wide test search. Closes rtk-ai#3856
pszymkowiak
left a comment
There was a problem hiding this comment.
Tested end-to-end with the real ast-grep binary (0.45.3, installed fresh) against this repo — no stub needed.
Plain mode: `ast-grep run -p 'LazyLock::new($$$)' -l rust .` — 334 raw lines → 57 filtered, correctly grouped by file with "N more matches in X" / "N more file(s) not shown" for overflow, matching the code exactly.
`--json`: confirmed untouched passthrough of the structured output (verified byte-for-byte via a captured-fixture replay; two live runs differ only because ast-grep itself doesn't guarantee traversal order between invocations — confirmed that's inherent to the tool, not something this wrapper introduces, by diffing two raw ast-grep --json runs with no rtk involved at all).
No-match case: exit code 1 on both raw and rtk ast-grep, no spurious output.
Also checked: guard::never_worse() gives a real automatic fallback (compares filtered vs raw, not just "on error"), the real captured fixture contains zero ANSI escape bytes so no strip_ansi() gap, and the fixture/synthetic test split follows the established pattern (inline strings for logic, real fixture for the savings assertion). cargo test/clippy/fmt all clean, 3048 tests green.
One non-blocking nit: result.stdout.clone() in the --json branch (line 95) is needed as written since result.stdout is borrowed again later for tracking, but could be restructured to avoid the copy — worth a look given that's exactly the path with the largest payloads.
Approving — nice addition, closes a real gap.
Review nit: --json is the largest-payload path, so borrow rather than clone result.stdout when skipping the filter.
|
Good catch, thanks. Fixed — borrows |
Adds the ast-grep row/section to README.md, what-rtk-covers.md, and FEATURES.md, matching how rtk grep/rg are documented there.
|
Added the missing docs the CONTRIBUTING checklist calls for: README.md, |
`RtkRule::pipeline_final_safe` was replaced by the `PipelineSafety` enum on develop (rtk-ai#3171). The ast-grep rule still set the old field, so the branch merged cleanly but failed to compile (E0560). `ProducerOnly` keeps the rule's original intent: safe as a pipeline's first stage, never as its final stage, since run() execs with stdin null. Adding a producer-safe rule also requires listing it in `test_pipeline_producer_safe_rule_set`, which pins the exact set. `savings_pct` was left at the 60.0 default; the repo-wide measurement is 84.6%, which is what README and what-rtk-covers.md already claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…output Two defects in `filter_ast_grep`, both verified against ast-grep 0.45.3. `ast-grep scan` diagnostics parse only on their locator line, so filtering line by line kept ` ┌─ a.rs:2:13` and dropped the rule id, severity, message and source line — at exit code 0, so it read as success. The whole-output fallback never fired because `order` was not empty. `unparsed_signal()` now passes any shape through untouched when a single non-blank line fails to parse, which is what `search.rs` already does for grep/rg. This also covers `--heading` mode and Windows drive-letter paths, where `[^:]+` cannot match `C:\src\a.rs`. The per-file overflow hint was computed as `entries.len() - max_per_file`, which ignores `max_total` cutting a file short: a file under its own cap lost its remainder with no hint at all, and a file over it under-reported the drop. Both now count against what was actually printed. The hint said "matches" while counting lines. ast-grep prints one line per matched source line and a structural match spans several, so a repo search reported "19 more matches" where five matches remained. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Round 2 of 3 — APPROVED, follow-ups filed
Claim: rtk ast-grep compacts ast-grep output by grouping matches per file and capping them, the way rtk grep/rtk rg do, leaving --json untouched.
Scope: accept. (Stated here for the first time — round 1 predates the current review template.)
Ran: ast-grep 0.45.3 in Docker (node:22-trixie-slim, LC_ALL=C) — plain run, --json and --json=compact, --heading always, --color always, -C 2, scan against a real sgconfig.yml, lsp, new, scan --interactive, no-match, missing binary, absolute paths, the bare -p form, and the final-pipeline-stage case. Repo-wide savings 84% against the 20% floor (CONTRIBUTING.md).
CI @3d2d244: all 11 checks green — Linux, macOS, Windows, clippy, fmt, benchmark. Worth knowing: this PR had never actually been built before today. Every run since Sept 3 sat at action_required waiting on a maintainer to release it, so nothing here had ever seen a runner.
Thanks for this — the shape is right, --json is handled exactly as it should be, and the pipeline_final_safe: false reasoning was correct (I confirmed stdin really is dropped as a final stage). Three things came out of the run matrix that I've fixed directly on your branch rather than sending back as a list.
Pushed to your branch
f64d1d4 — compiles again against develop. The branch merged cleanly but did not build: RtkRule::pipeline_final_safe was replaced by the PipelineSafety enum while this PR was open (#3171), so the new rule hit E0560. ProducerOnly is the faithful translation of what you wrote — safe first in a pipeline, never last. Adding a producer-safe rule also needs an entry in test_pipeline_producer_safe_rule_set, which pins the exact set. I also set savings_pct to 85, since it was still on the 60.0 struct default while your own README and what-rtk-covers.md claim 85 — the repo-wide measurement is 84.6%.
3d2d244 — two correctness fixes. This one is larger than a maintainer fixup normally is.
ast-grep scan output was being destroyed. MATCH_LINE_RE is applied per line with continue, and the whole-output fallback only fires when zero lines match. In a scan diagnostic exactly one line parses — the locator — so the filter kept it and dropped everything carrying the meaning, at exit code 0:
raw before this fix
warning[no-unwrap]: avoid unwrap ┌─ a.rs:2:13
┌─ a.rs:2:13 (exit 0)
│
2 │ let x = foo().unwrap();
│ ^^^^^^^^^^^^^^
never_worse() cannot catch this — the wreckage is smaller than the raw. search.rs:369 already guards grep/rg against exactly this shape with unparsed_signal(), so I mirrored it: one unparseable non-blank line and the output is returned untouched. That covers --heading mode and Windows drive-letter paths too, where [^:]+ cannot match C:\src\a.rs.
The caps dropped lines with no hint. The per-file hint was entries.len() - max_per_file, which ignores max_total cutting a file short. A file under its own cap lost its remainder silently, and skipped_files was not incremented either. Twenty files of three matches: 60 raw lines → 50 shown + 3 skipped files hinted = 59 accounted for, one match simply gone. The hint now counts what was actually printed.
The hint said "matches" while counting lines. ast-grep prints one line per matched source line and a structural match spans several — that is the premise of this PR. On a real search it reported … 19 more matches where five matches remained. It now reads match line(s); test_groups_and_caps_by_file pinned the old wording, so that assertion moved with it, and FEATURES.md says which unit it is.
Two regression tests come with it: test_scan_diagnostic_shape_passes_through and test_total_cap_hints_lines_it_cut. Savings after the change are still 84%, so the guard costs nothing on the path that matters.
Filed as follow-ups
- #4015 —
-C/-A/-Bcontext lines are indistinguishable from match lines (grep separates them with-, ast-grep uses:), so they eat the per-file cap and inflate the count. Related to #1161 / #1313. - #4016 — the per-file cap cuts inside a structural match, so the kept output can be a truncated fragment. Capping by match rather than by line is a design call, not a fix for this PR.
- #4017 — with the guard above, a Windows absolute-path run now passes through safely instead of losing lines, but that means no savings there at all.
Checked and correct — no need to re-verify
--json passthrough is byte-identical (including --json=compact); exit codes match raw ast-grep (0, 1 on no match, 1 on missing binary); --heading always and --color always behave; the docs commit ba802ac is accurate.
Hypotheses built and dropped
- That the
^ast-grep\s+rule breakslsp,newandscan --interactivethrough the stdin-null capture. Raw ast-grep behaves identically under a non-TTY (rc 0, 1 and 101), so the rewrite makes nothing worse; and with the parse guard,scannow passes through intact. - That clap eats
--before it reaches ast-grep, the waysearch.rscompensates for withrestore_double_dash.rtk ast-grep run … -- ./-weird.rsmatched raw byte for byte.
Next
Nothing — approving. mergeable: CLEAN against develop.
Rounds: 2/3. Threads: 0 open, round 1's --json clone nit resolved by 905ca3a.
Closes #3856
What
Adds
rtk ast-grep, filtering ast-grep's structural search output the same wayrtk grep/rtk rgalready do — groups matches by file, caps per-file/total, collapses overflow to a count hint.Why
ast-grep matches span multiple lines per hit (whole statement/block), so raw output on a repo-wide search gets big fast. No filter covered it yet.
Numbers
Real repo-wide test search: 1528 tokens raw -> 235 filtered (~85% savings).
--jsonstays untouched since that's an explicit structured-output ask.Scope
src/cmds/system/ast_grep_cmd.rsmain.rs(Commands::AstGrep,is_operational_command,PASSTHROUGH)src/discover/rules.rs—pipeline_safety: ProducerOnly(safe as a pipeline's first stage, never its last: no piped-stdin support yet)run(scandiagnostics,--heading, paths thepath:line:parser can't read) pass through untouched rather than being groupedtests/fixtures/ast_grep_lazylock_raw.*)Test plan
cargo fmt --allcargo clippy --all-targetscargo test(new module + touched rule/classification tests pass)