fix(discover): register ssh in the rewrite rule table so hook routes it#2004
Open
YOMXXX wants to merge 1 commit into
Open
fix(discover): register ssh in the rewrite rule table so hook routes it#2004YOMXXX wants to merge 1 commit into
YOMXXX wants to merge 1 commit into
Conversation
`rtk verify --filter ssh` reports 3/3 tests passing because the TOML filter at `src/filters/ssh.toml` is valid, but `rtk rewrite "ssh ..."` returned exit 1 for every shape ‑- the Claude Code hook's rewrite path therefore never produced `rtk ssh ...` and the ssh filter was effectively dark in production, despite passing verify. Root cause: `rewrite_command` ➜ `rewrite_segment_inner` looks up the base command in `RULES` (`src/discover/rules.rs`). `RULES` is the source of truth for the hook rewrite path, but it didn't list ssh. Verify and rewrite consult two different tables, so a TOML-only filter with no RULES entry passes verify yet is invisible to the hook. Fix: add a single `RtkRule` for ssh next to the existing curl/wget entries (same `Network` category, same `\s+` pattern shape). Once rewritten, main.rs's TOML-filter dispatch picks up `src/filters/ssh.toml` unchanged ‑- no other code paths needed. Behaviour: - `ssh user@host uptime` → `rtk ssh user@host uptime` (Allow + filter) - `ssh -o ConnectTimeout=5 h cmd` → `rtk ssh -o ConnectTimeout=5 h cmd` - `ssh host` → `rtk ssh host` (login banners still benefit from strip_lines_matching) - bare `ssh` (no args) → unchanged (`\s+` guard, nothing to filter) Tests: - test_classify_ssh_with_remote_command pins the Supported / Network classification so future tweaks to RULES order can't silently undo this. - test_rewrite_ssh_shapes locks in the three reproduction shapes from the issue verbatim. - test_rewrite_ssh_with_host_only covers the no-remote-command shape that the issue's repro script also exits 1 for today. - test_rewrite_bare_ssh_not_rewritten guards the `\s+` boundary so a future loosened pattern doesn't start rewriting argv-less `ssh`. cargo fmt / clippy --all-targets / test --bin rtk: 1913 passed, 0 warnings. Fixes rtk-ai#1654
Contributor
Author
|
Review note: I would not merge this as-is because it rewrites interactive SSH shapes. The tests intentionally rewrite Please narrow the rewrite rule to remote-command forms only, or explicitly exclude interactive/forwarding shapes. Also, the new comment in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
`rtk verify --filter ssh` reports 3/3 passing because the TOML filter at `src/filters/ssh.toml` is valid, but `rtk rewrite "ssh …"` exits 1 for every shape — the Claude Code hook's rewrite path therefore never produces `rtk ssh …` and the ssh filter is effectively dark in production, even though verify passes.
Reporter (#1654) saw ssh as the #1 unhandled command in 30 days of `rtk discover`, with ~50K–100K tokens/month of savings missed on a single user.
Root cause
`registry::rewrite_command` → `rewrite_segment_inner` looks the base command up in `RULES` (`src/discover/rules.rs`). `RULES` is the source of truth for the hook rewrite path, but it did not list ssh. `rtk verify` consults the TOML filter registry; `rtk rewrite` consults the hard-coded RULES table — so a TOML-only filter with no RULES entry passes verify yet is invisible to the hook.
This split is the same shape as the historical `ps` / `ansible-playbook` setup that the reporter mentions still working — those have RULES entries; ssh did not.
Fix
Add one `RtkRule` for ssh next to curl/wget (same `Network` category, same `^cmd\s+` pattern shape). Once `rtk rewrite` produces `rtk ssh …`, main.rs's TOML-filter dispatch picks up `src/filters/ssh.toml` unchanged — no other code paths needed.
```rust
RtkRule {
pattern: r"^ssh\s+",
rtk_cmd: "rtk ssh",
rewrite_prefixes: &["ssh"],
category: "Network",
savings_pct: 65.0,
...
}
```
Behaviour
`GIT_SSH_COMMAND="ssh -o …" git push` continues to env-strip → `rtk git push` exactly as today; covered by existing `test_classify_env_prefix_stripped`.
Test plan
Notes
Fixes #1654