Hook Engine + Chained Command Rewriting (PR #131 Part 1)#156
Hook Engine + Chained Command Rewriting (PR #131 Part 1)#156ahundt wants to merge 5 commits intortk-ai:masterfrom
Conversation
Rust binary replaces 204-line bash script as Claude Code PreToolUse hook. Adds rtk hook claude, rtk run -c, and Windows support via cfg!(windows). Closes rtk-ai#112 (chained commands missed). Based on updated master (70c3786) which includes: - Hook audit mode (rtk-ai#151) - Claude Code agents and skills (d8f4659) - tee raw output feature (rtk-ai#134) Migrated from feat/rust-hooks (571bd86) with conflict resolution for: - src/main.rs: Commands enum (preserved both hook audit + our hook commands) - src/init.rs: Hook registration (integrated both approaches) New files (src/cmd/ module): - mod.rs: Module declarations (10 modules, excluding safety/trash/gemini for PR 1) - hook.rs: Shared hook decision logic (21 tests, 3 safety tests removed for PR 2) - claude_hook.rs: Claude Code JSON protocol handler (18 tests) - lexer.rs: Quote-aware tokenizer (28 tests) - analysis.rs: Chain parsing and shellism detection (10 tests) - builtins.rs: cd/export/pwd/echo/true/false (8 tests) - exec.rs: Command executor with recursion guard (22 tests, safety dispatch removed for PR 2) - filters.rs: Output filter registry (5 tests) - predicates.rs: Context predicates (4 tests) - test_helpers.rs: Test utilities Modified files: - src/main.rs: Added Commands::Run, Commands::Hook, HookCommands enum, routing - src/init.rs: Changed patch_settings_json to use rtk hook claude binary command - hooks/rtk-rewrite.sh: Replaced 204-line bash script with 4-line shim (exec rtk hook claude) - Cargo.toml: Added which = 7 for PATH resolution - INSTALL.md: Added Windows installation section Windows support: - exec.rs:175-176: cfg!(windows) selects cmd /C vs sh -c for shell passthrough - predicates.rs:26: USERPROFILE fallback for Windows home directory - No bash, node, or bun dependency - rtk hook claude is a compiled Rust binary Tests: All 541 tests pass
|
Thanks Andrew for the clean split from #131, and the architecture is genuinely well thought out — the lexer, fail-open design, RAII guard, and deny(clippy::print_stdout) are all However, I have a critical concern that I think is a regression: The hook wraps everything in rtk run -c '...' instead of routing to specialized filters. The current bash hook does: This PR does: The entire value of RTK is the specialized filters per command. rtk cargo test shows only failures (90% token reduction). rtk run -c 'cargo test' just runs the command and strips ANSI Did you test this with real commands and compare token savings? I'd expect rtk run -c 'cargo test' to produce significantly more output than rtk cargo test. What I'd expect instead: A few other items:
The foundation here is solid. The lexer, chain parsing, and fail-open protocol handling are exactly what we need. But the command routing needs to preserve the specialized filters — Happy to discuss the best approach. Would it make sense to have rtk run -c detect known commands and dispatch to their specialized modules internally? |
|
I tested the PR locally and found a critical issue: the hook routes everything through rtk run -c, bypassing all specialized filters. Here's what I measured:
|
…commands Replaces stub check_for_hook_inner with full tokenize+native-path dispatch. Adds route_native_command() with replace_first_word/route_pnpm/route_npx helpers to route single parsed commands to optimized RTK subcommands. Chains (&&/||/;) and shellisms still use rtk run -c. No safety integration (PR rtk-ai#157 adds that). Mirrors ~/.claude/hooks/rtk-rewrite.sh routing table. Corrects shell script vitest double-run bug for pnpm vitest run flags.
|
Thanks @pszymkowiak, i found that too independently and just pushed a fix, hopefully it should work now! also there is one potential dep that could be added optionally to robustify things and improve token reduction but i left it out in favor of a small regex at this time to minimize deps, strip-ansi-escapes strips ANSI escape sequences from byte sequences using the vte terminal parser — handles the full escape sequence space (CSI, OSC, private-mode params, two-byte sequences) that a hand-rolled regex misses. https://crates.io/crates/strip-ansi-escapes |
rtk has no `tail` subcommand — routing to "rtk tail" was silently broken (rtk would error "unrecognized subcommand"). Remove the Route entry so the command falls through to `rtk run -c '...'` correctly. Move the log-tailing test cases from test_routing_native_commands (which asserted the broken path) into test_routing_fallbacks_to_rtk_run where they correctly verify the rtk-run-c fallback behavior.
Port tests added during the ROUTES table integration that were missing from the v2 worktree: registry.rs: - 12 classify tests for Python/Go commands (pytest, go×4, ruff×2, pip×3, golangci-lint) that verify PATTERNS/RULES and ROUTES alignment - 11 lookup tests (test_lookup_*, test_no_duplicate_binaries_in_routes, test_lookup_is_o1_consistent) that verify O(1) HashMap routing hook.rs: - Extend test_routing_native_commands from 20 to 47 cases covering all ROUTES entries: docker, kubectl, curl, eslint, tsc, prettier, playwright, prisma, pytest, golangci-lint, ruff, pip, gh variants - Add test_routing_subcommand_filter_fallback (14 cases) verifying that Only[] subcommand filters correctly reject unmatched subcommands Total: 545 → 569 tests (+24)
|
ok i made it a much better router that goes in registry.rs so all the "add a new tool" code is in one place, it should be much more efficient than the previous two versions and easier to extend to better address that bug you mentioned. |
Three integration tests that simulate the full hook pipeline from scratch: raw command → check_for_hook (lexer + router) → rewritten rtk cmd → execute both → assert rtk output has fewer tokens than raw Tests: - test_e2e_git_status_saves_tokens: verifies ≥40% savings vs raw git status - test_e2e_ls_saves_tokens: verifies ≥40% savings vs raw ls -la - test_e2e_git_log_saves_tokens: verifies ≤5% overhead (already-compact input) Each test first asserts the lexer+router produced the correct rewrite, then executes both commands and compares whitespace-delimited token counts. Run with: cargo test e2e -- --ignored Requires: cargo install --path . (rtk on PATH) + git repo
|
@pszymkowiak I also added a few small e2e tests to confirm the behavior actually uses the rtk internals and those pass. |
PR 131 Part 1: Hook Engine + Chained Command Rewriting
Branch:
feat/rust-hooks-v2| Base:master| Tests: 541 passCloses: #112 | Split from: PR #131
New dep:
which = "7"PR: #156
Context
FlorianBruniaux requested splitting PR #131 (52 files, 8K+ additions) into separate PRs:
This PR combines items 3 + 4 because they are architecturally inseparable: the hook protocol handler calls
lexer::tokenize()thenanalysis::parse_chain()to process chained commands. Separating them would require duplicating the lexer.Coordination with PR #141: FlorianBruniaux noted overlap with #141's JS-based hook for Windows. This PR achieves Windows support via compiled Rust binary instead -- no bash, node, or bun required. CI/CD already builds Windows binaries.
exec.rsusescfg!(windows)for shell selection.Merge Sequence
Summary
Replaces the 204-line bash hook with a native Rust binary that provides quote-aware chained command rewriting. Closes #112 where
cd /path && git statusonly rewrotecd.Impact: Captures ~12-20M tokens/month in previously-missed optimizations across chained commands.
Why Rust over bash:
cd && git statusrewrites both)rtk hook checkshows exact rewrites)rtk hook claude-- Claude Code PreToolUse handlerReads JSON from stdin, applies rewriting, outputs JSON to stdout. Fail-open: malformed input exits 0 with no output so Claude proceeds unchanged.
Chained command rewriting (closes #112)
Before:
cd /tmp && git status-- hook only sawcd, missedgit statusAfter: lexer splits on
&&/||/;respecting quotes, each command wrapped independentlygit commit -m "Fix && Bug"is NOT split (quote-aware).rtk run -c <command>-- Command executorParses chains, detects shellisms (globs/pipes/subshells -> passthrough to sh/cmd), handles builtins (
cd/export/pwd), applies output filters, prevents recursion viaRTK_ACTIVEenv guard.rtk hook check-- DebuggerChanges
16 files changed (+2969, -221)
New (
src/cmd/):mod.rs,hook.rs,claude_hook.rs,lexer.rs,analysis.rs,builtins.rs,exec.rs,filters.rs,predicates.rs,test_helpers.rsModified:
src/main.rs(+Commands::Run, +Commands::Hook),src/init.rs(register binary hook),hooks/rtk-rewrite.sh(204-line script -> 4-line shim),Cargo.toml(+which),INSTALL.md(+Windows section)Intentionally excluded (stacked PRs):
feat/data-safety-rules-v2)feat/gemini-support-v2)Review Guide
Focus areas:
src/cmd/lexer.rs+analysis.rs-- Chain parsing correctness (quote handling)src/cmd/claude_hook.rs-- Protocol compliance, fail-open designsrc/cmd/exec.rs-- Builtin handling, Windows shell selection (cfg!(windows))src/cmd/hook.rs-- Shared decision logic (used by Parts 2 and 3)Implementation Notes
Binary size: Compiled with LTO + stripping. Size increase from
whichdependency minimal (<0.1 MB). Full size impact measurable after all 3 parts merge (PR #131 reported 5.1 MB total, +0.3 MB from combined deps).Backward compatible: All existing RTK features work unchanged. Legacy bash hook becomes 4-line shim forwarding to
rtk hook claude.Test Plan
cargo test-- 541 tests pass (hook:22, claude_hook:18, lexer:28, analysis:10, builtins:8, exec:22, filters:5, predicates:4)echo '{"tool_input":{"command":"git status"}}' | cargo run -- hook claude-- JSON rewrite worksecho '{"tool_input":{"command":"cd /tmp && git status"}}' | cargo run -- hook claude-- chain split workscargo run -- hook check "git status"-- text debugger workscargo run -- run -c "echo hello"-- executor worksgrep 'cfg!(windows)' src/cmd/exec.rs-- Windows shell selection presentRelated PRs (Split from PR #131)
Merge order: Part 1 first → retarget Parts 2 & 3 to
master→ merge in any order