Conversation
Three independent bugs bundled in issue rtk-ai#3401, each an over-loose match_command regex that activated a filter on commands it should never touch: - spring-boot: java -jar *.jar matched ANY jar invocation, not just Spring Boot apps, silently collapsing unrelated tool output to the filter's Spring-specific keep-list. Now requires "spring" in the jar filename (mvn/gradle alternatives were already Spring-specific). - liquibase: unanchored substring match fired on any command containing "/liquibase" anywhere, e.g. `rm -rf /opt/liquibase`. Anchored to the invoked program name instead. - ssh: `\b` can't match between "ssh" and a following "-", so ssh-keygen/ssh-add/ssh-copy-id incorrectly activated the plain-ssh connection filter. Replaced with an explicit (\s|$) boundary. TOML [[tests.*]] blocks only exercise each filter's output pipeline, never match_command routing, so added a Rust unit test per filter (colocated in toml_filter.rs) covering both the excluded false positive and the still-matching true positive. Closes rtk-ai#3401
Code review caught a case-sensitivity gap: the jar-filename check only matched a lowercase "spring" substring, so PascalCase/CamelCase artifact names (e.g. MySpringApp.jar) silently failed to activate the filter. Scope the case-insensitive flag to just that group so the mvn/gradle alternatives are unaffected.
Code review caught that \S* spans '/', so the "spring in jar name" check actually matched "spring" anywhere in the full path argument (e.g. a directory literally named spring-cache), not just the jar's own filename as intended. Split the path prefix from the filename match so "spring" must appear in the final path segment. Also closes a coverage gap: the ssh fix's end-of-string branch (bare `ssh` with no args) had no test.
KuSh
left a comment
There was a problem hiding this comment.
Reviewed at high effort against ef6eef5, in a clean worktree of this branch.
Verification: cargo test --bin rtk toml_filter → 69 passed; cargo fmt --all -- --check → clean; cargo clippy --all-targets → zero warnings. I also probed the three new match_command regexes against ~24 real-world command strings.
The ssh and liquibase tightenings behave correctly. One blocking issue on spring-boot, three non-blocking notes inline.
Blocking — the new jar-name heuristic only treats / as a path separator, so the exact false positive this PR closes still fires on Windows paths (java -jar C:\spring-cache\other.jar matches spring-boot). Details and a validated suggestion are inline on src/filters/spring-boot.toml.
Non-blocking — the new regression guards can pass vacuously through find_filter_in's first-match semantics; liquibase's (?:\S*/)? prefix branch is unreachable in production; the PR description's ssh rationale is inverted; and the acknowledged spring-boot false-negative trade-off deserves a line in the filter's description. All four inline.
- spring-boot: treat backslash as a path separator alongside forward slash in the jar-name heuristic, closing the Windows false positive where "spring" in a directory segment (not the filename) still activated the filter (e.g. java -jar C:\spring-cache\other.jar) - spring-boot: document the accepted false-negative tradeoff in the filter description (default Maven/Gradle jar naming won't match) - liquibase: drop the dead (?:\S*/)? path-prefix branch from match_command — both production callers already basename argv[0] before the regex runs, so the branch was unreachable - toml_filter tests: rewrite the spring-boot/liquibase/ssh negative assertions to check each filter's own match_regex directly instead of relying on find_filter_in's first-match-in-order semantics, which could pass vacuously if a future filter shadowed the target - toml_filter tests: add Windows-path negative/positive cases for spring-boot, and replace the liquibase "path-qualified" test with one that reflects actual (basenamed) caller behavior Addresses reviewer feedback from rtk-ai#3560.
Records the KTDs and validation behind the spring-boot/liquibase/ssh fixes applied in the previous commit.
|
@KuSh Thanks for the thorough review — all five findings addressed in cab2283 (+ e2e6753 for the planning doc):
Full gate re-run clean: |
KuSh
left a comment
There was a problem hiding this comment.
Thanks for turning this around so quickly — the Windows-separator round is spot on, and switching the negatives to assert against each filter's own match_regex rather than find_filter_in is exactly the right call. Approving.
I pushed one maintainer-edit commit on top (f0a2863) rather than send this back for another round, since everything left was small and self-contained. Details below so nothing about that push is a surprise.
What I verified
All five threads are genuinely addressed. I round-tripped the three match_command values through tomllib and ran them against the full matrix from the last review: C:\spring-cache\other.jar and C:\dev\spring-workspace\build\other-tool.jar now correctly do not match, while mvn spring-boot:run, build/libs/my-spring-app.jar, build\libs\my-spring-app.jar, MySpringApp.jar, C:\dev\my-spring-app.jar and gradle bootRun all still do.
The liquibase narrowing is safe: both callers basename argv[0] first (Path::file_name in run_fallback, strip_absolute_path in discover/registry.rs). strip_absolute_path splits only on /, but the old \S*/ prefix didn't handle \ either — so dropping it is not a Windows regression.
Gate on your head commit: cargo fmt --all -- --check clean, cargo clippy --all-targets zero warnings, cargo test --all 2663 passed / 0 failed.
What f0a2863 changes
1. Removed docs/plans/2026-08-30-001-...-plan.md. It's the only commit in repo history to add anything under docs/plans/, and .gitignore already excludes claudedocs/ for this class of generated output.
2. Corrected the new description. It credited mvn spring-boot:run to this filter, but mvn is a Clap subcommand (src/main.rs:823) dispatched to mvn_cmd::run, so it never reaches run_fallback's TOML lookup and this filter cannot activate for it. The mvn alternation in match_command is pre-existing dead code — I left it alone rather than widen the diff.
3. Named the second false negative. argv reaches the regex as one space-joined string, so java -jar "C:\Program Files\app\my-spring-app.jar" — quotes already gone by match time — no longer matches, where the old broad pattern did.
On (3) I deliberately documented rather than widened the regex. Rust's regex crate has no lookarounds, and any whitespace-spanning prefix lets a later spring-named jar argument re-trigger the filter (java -jar other.jar -Dpath=/tmp/spring-x.jar), recreating exactly the false-positive class this PR exists to close. Full passthrough for those jars is a missed optimization, not data loss — the same trade-off you already accepted for <artifactId>-<version>.jar naming. It's now stated in the description and pinned by an assertion so it doesn't get "fixed" back into a false positive later.
The description credited `mvn spring-boot:run` to this filter, but RTK routes `mvn` through its own Clap subcommand into the dedicated Maven filter, so that alternation never reaches the TOML registry. It also omitted the second false negative the jar-name heuristic introduces: argv reaches the regex as one space-joined string, so a jar under a path containing spaces (`C:\Program Files\...`) no longer matches. Widening the prefix across whitespace would let a later `spring`-named jar argument re-trigger the filter, so those jars stay on full passthrough — now stated in the description and pinned by an assertion. Drops the `~/.ssh/` path from the ssh-add assertion, which tripped the `sensitive-path-reference` semgrep rule. The argument plays no part in what the assertion checks: that `^ssh(?:\s|$)` rejects `ssh-add`. Also removes the planning document committed alongside the review fixes; `docs/plans/` has no precedent in the repo and `.gitignore` already excludes `claudedocs/` for this kind of generated output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkpWfkyELX5ZbYABg7SQqv
f0a2863 to
3853c71
Compare
Summary
Three independent bugs bundled in #3401, each an overly broad
match_commandregex that activated a filter on commands it should never touch, silently discarding output that had nothing to do with the tool the filter thought it was compacting.spring-boot— anyjava -jar <file>.jarinvocation activated Spring-specific compaction, collapsing an unrelated Java tool's output down to an 11-pattern Spring-only keep-list. Since a command-line regex can't inspect a jar's manifest, the filter now requires "spring" (case-insensitively) to appear in the jar's own filename — not merely somewhere in its path, which a first-pass version of this fix got wrong (caught in review; a directory like/opt/spring-cache/other-tool.jarno longer false-positives). A second review pass caught a further gap: the filename/path split only recognized/, so a Windows path likejava -jar C:\spring-cache\other.jarstill let aspringdirectory segment leak into the match — fixed by treating\as a separator alongside/.liquibase— an unanchored substring match fired on any command containing/liquibaseanywhere, e.g.rm -rf /opt/liquibase. Anchored to the invoked program name instead; the review also flagged that the optional path-qualifying prefix in the anchored regex was dead code (both production callers already basenameargv[0]before the regex runs), so it was dropped in favor of the plain anchored form.ssh—\bdid form a boundary betweensshand a following-(his a word character,-is not) — and that boundary is exactly why^ssh\bincorrectly matchedssh-keygen,ssh-add, andssh-copy-id:\bonly asserts a word/non-word transition exists, not which side is the "real" token boundary. Replaced with an explicit(?:\s|$)boundary, which requires whitespace or end-of-string and correctly excludes thessh-*utilities — the same shape already applied togcc.tomlin a sibling PR for this issue family.Validation
TOML
[[tests.*]]blocks only exercise each filter's output pipeline, nevermatch_commandrouting, so a Rust unit test was added per filter (src/core/toml_filter.rs) asserting both the excluded false positive and the still-matching true positive — each confirmed to fail before its fix and pass after. A follow-up review round added Windows-path coverage for spring-boot and rewrote the three negative assertions to check each filter's own compiled regex directly (filter.match_regex.is_match(...)) rather than relying onfind_filter_in's first-match-in-order semantics, which could otherwise pass vacuously if an unrelated, alphabetically-earlier filter happened to shadow the one under test. Full gate:cargo fmt --all -- --check(clean),cargo clippy --all-targets(zero warnings), targetedcargo test --bin rtk toml_filter(69/69 passed),cargo test --all(2584 passed; 2 pre-existing failures inhooks::rewrite_cmd::tests::unattestable_passthroughconfirmed present on a clean checkout of this branch before these changes too — they depend on local permission settings, unrelated to this change).Session-settled decisions carried from planning: narrow spring-boot's
java -jarmatch by requiring "spring" in the jar filename rather than inspecting runtime output (user-directed, over leaving unconditional — a regex only sees the command line); anchor liquibase to the invoked program name rather than an unanchored substring (user-directed, over the current pattern —rm -rf /opt/liquibasemust never activate the filter); replace ssh's\b, which formed a boundary in the wrong place, with an explicit(?:\s|$)boundary (user-directed, over leaving it —ssh-keygen/ssh-addare extremely common).One known, accepted trade-off: the spring-boot heuristic will miss a genuine Spring Boot jar that has no "spring" in its filename (common with Maven's default
<artifactId>-<version>.jarnaming) — that jar simply won't get compaction (full passthrough), which is a missed optimization, not a data-loss bug, and consistent with this repo's fallback-pattern philosophy of preferring unfiltered output over incorrect filtering when uncertain. This tradeoff is now also documented directly in the spring-boot filter'sdescriptionfield.Related: Fixes #3401