Skip to content

feat(hook): native cross-platform (Windows & more) hook-rewrite command - #150

Closed
ayoub-khemissi wants to merge 9 commits into
rtk-ai:masterfrom
ayoub-khemissi:feat/native-hook-rewrite
Closed

ayoub-khemissi wants to merge 9 commits into
rtk-ai:masterfrom
ayoub-khemissi:feat/native-hook-rewrite

Conversation

@ayoub-khemissi

@ayoub-khemissi ayoub-khemissi commented Feb 16, 2026 •

Copy link
Copy Markdown

Summary

Replace the bash-based rtk-rewrite.sh hook with a native Rust rtk hook-rewrite subcommand, eliminating the bash/jq dependency and enabling cross-platform support (Windows, macOS, Linux).

  • New src/hook_cmd.rs (1126 lines): Pure Rust command rewriter that reads Claude Code PreToolUse JSON from stdin, rewrites known commands to their rtk equivalents, and outputs updated JSON. Supports command chains (&&, ;), env var prefixes, git/cargo/docker/kubectl global flags, and 30+ command patterns (git, cargo, gh, pnpm, vitest, tsc, eslint, prettier, playwright, prisma, pytest, ruff, pip, go, golangci-lint, curl, wget, cat, grep, find, ls, tree, head, diff).
  • Updated src/init.rs: rtk init now installs the native rtk hook-rewrite command instead of the bash script. Includes migration logic to clean up old .sh hook files. Both old and new formats are detected during --uninstall.
  • Updated src/main.rs: Added hidden hook-rewrite subcommand routing to hook_cmd::run().
  • New build.rs: Set 8 MB stack size on Windows to prevent debug build stack overflow (Windows defaults to 1 MB vs 8 MB on Unix).
  • Minor src/gain.rs: Formatting fix (line wrapping for table_width calculation).

Why native?

Bash hook (rtk-rewrite.sh) Native hook (rtk hook-rewrite)
Dependencies bash + jq required None (single binary)
Windows Requires Git Bash/WSL Works natively
Performance Shell startup + jq parsing Direct Rust execution
Security Shell script attack surface Pure string manipulation, no shell spawning
Maintenance Separate script file to distribute Built into rtk binary

Security review

Per SECURITY.md checklist:

  • No Command::new("sh") — pure string rewriting, no shell spawning
  • No unsafe {} blocks
  • No network operations (reqwest::, std::net::)
  • No environment manipulation (.env("LD_PRELOAD"))
  • No .unwrap() in production paths (only in lazy_static! regex compilation)
  • Input validation via allowlists (specific subcommands matched, not denylists)
  • Graceful failure: any parse error → exit(0) (no rewrite, original command passes through)
  • init.rs changes reduce attack surface by removing bash script dependency

Test plan

  • 56 unit tests covering all rewrite patterns, edge cases, command chains, env prefixes, JSON roundtrip
  • cargo fmt --all --check && cargo clippy --all-targets && cargo test --all passes
  • Manual test: rtk init --auto-patch installs native hook command in settings.json
  • Manual test: rtk init --uninstall removes both old .sh and new native hook entries
  • Manual test: In Claude Code session, raw git status is rewritten to rtk git status
  • Cross-platform: Verify on Windows (no bash dependency needed)

Verification results (Windows 11, 2026-02-16)

Quality pipeline

Check Result
cargo fmt --all --check PASS — no diff
cargo clippy --all-targets PASS — 45 pre-existing warnings, 0 errors
cargo test --all PASS — 413 tests, 0 failures

Uninstall coverage

Scenario Result
Remove native hook (rtk hook-rewrite) from settings.json OK
Remove old bash hook (rtk-rewrite.sh) file OK
Remove RTK.md OK
Remove @RTK.md reference from CLAUDE.md OK
Migration .sh → native then uninstall OK — both cleaned up
Idempotent uninstall (nothing installed) OK — "RTK was not installed"

Hook-rewrite manual verification

Raw command Rewritten to
git status rtk git status
cargo test -- --nocapture rtk cargo test -- --nocapture
cd /project && git status && cargo test cd /project && rtk git status && rtk cargo test
TEST_ID=123 git status TEST_ID=123 rtk git status
cat src/main.rs rtk read src/main.rs
python -m pytest tests/ rtk pytest tests/
rtk git status (already prefixed) No double-rewrite
Non-Bash tool (Read) Silently ignored (exit 0)

Cross-platform (Windows)

  • Binary: Native PE64 executable, 5.4 MB stripped
  • Zero bash dependency: No bash, sh, jq, sed, grep calls in hook_cmd.rs
  • hook-rewrite: Pure Rust stdin/stdout JSON, works natively on Windows
  • Stack overflow fix: build.rs sets /STACK:8388608 (8 MB) on Windows via linker flag — zero runtime overhead, matches Unix defaults

🤖 Generated with Claude Code

@ayoub-khemissi ayoub-khemissi changed the title feat(hook): native cross-platform hook-rewrite command feat(hook): native cross-platform (Windows & more) hook-rewrite command Feb 17, 2026
@aeppling aeppling added the P1-critical Bloque des utilisateurs, fix ASAP label Feb 18, 2026
@aeppling

Copy link
Copy Markdown
Contributor

Hello,

Approved.

Code is correct for me, but for futures PR please split files into respective components to avoid having 1000+ lines files.

We should refactor the code structure of the project for a clean architecture that will help for easy reviewing and extensible code.

@pszymkowiak

Copy link
Copy Markdown
Collaborator

Thanks for this massive PR — the native hook-rewrite is a great step forward for cross-platform support!

I did an exhaustive test of 313 commands across all 12 categories, simulating the real Claude Code hook JSON format (tool_input.command). Here are the full results:

Test Results: 312/313 passed (99.7%)

Category Tested Passed Notes
Git (supported) 29 29 status, diff, log, add, commit, push, pull, branch, fetch, stash, show, worktree
Git (blocked) 15 15 checkout, merge, rebase, reset, cherry-pick, tag, bisect, reflog, remote, clean, blame, init, clone
Git (global flags) 7 7 -C, --no-pager, --bare, --no-optional-locks, -c key=val
GitHub CLI (supported) 17 17 pr, issue, run, api, release
GitHub CLI (blocked) 5 5 repo, gist, workflow, auth, config
Cargo (supported) 19 19 build, test, clippy, check, fmt, install, +toolchain
Cargo (blocked) 7 7 run, new, publish, doc, bench, update, add
Files 21 21 cat→read, head→read --max-lines, ls, tree, find, diff
Grep/Search 8 8 grep, rg → rtk grep
Network 5 5 curl, wget
Docker (supported) 18 18 ps, images, logs, run, build, exec, compose
Docker (blocked) 7 7 rm, rmi, stop, push, pull, network, volume
Kubectl (supported) 15 15 get, logs, describe, apply + global flags
Kubectl (blocked) 7 7 create, delete, edit, scale, exec, port-forward, rollout
JS/TS — vitest 9 9 vitest, npx vitest, pnpm vitest
JS/TS — tsc 11 11 tsc, npx tsc, pnpm tsc, vue-tsc, npx vue-tsc
JS/TS — eslint 9 8 pnpm eslint . not rewritten (see below)
JS/TS — prettier 5 5 prettier, npx prettier
JS/TS — playwright 5 5 playwright, npx playwright, pnpm playwright
JS/TS — prisma 7 7 prisma, npx prisma
NPM 6 6 npm test, npm run build/dev/lint/start
PNPM (supported) 3 3 list, ls, outdated
PNPM (blocked) 5 5 install, add, remove, run, dev
Python — pytest 8 8 pytest, python -m pytest + flags
Python — ruff (supported) 5 5 check, format
Python — ruff (blocked) 2 2 rule, linter
Python — pip/uv (supported) 8 8 list, outdated, install, show + uv variants
Python — pip/uv (blocked) 3 3 freeze, download
Go (supported) 9 9 test, build, vet + flags
Go — golangci-lint 3 3 run + flags
Go (blocked) 5 5 run, mod, install, get, generate
Env prefix 4 4 NODE_ENV=, RUST_LOG=, CI=, multi-var
Chained commands (&&) 4 4 git&&git, cargo&&cargo, cd&&ls, mkdir&&cd
Skip (no rewrite) 21 21 rtk *, echo, mkdir, cd, rm, mv, cp, chmod, which, env, export, source, python, node, make, cmake, gradle, mvn, dotnet
Heredoc 1 1 cat <<EOF skipped

Only issue: pnpm eslint . not rewritten

pnpm eslint . → no rewrite (expected rtk lint .).

The hook handles pnpm lint (script name) but not pnpm eslint (direct binary). Minor — most projects use pnpm lint in practice.

Notes on kubectl / docker compose syntax

The hook correctly mirrors real CLI syntax:

  • kubectl get pods → rtk kubectl get pods
  • docker compose logs web → rtk docker compose logs web

RTK currently uses shorter aliases (rtk kubectl pods, rtk docker ps/images/logs only), but that's a design issue on our side — we'll update RTK to accept the full natural syntax. No action needed on this PR.

Verdict

Excellent work. 312/313 commands rewrite correctly across 35 test categories. The chain splitting (&&, ;), env prefix preservation (NODE_ENV=test npm test), global flag stripping (git -C, --no-pager; docker -H; kubectl --namespace, -n), heredoc detection, and quote-aware parsing all work correctly.

The only fix needed is adding pnpm eslint support alongside the existing pnpm lint handling. Thanks!

I'm giving @aeppling the green light to merge once the pnpm eslint fix is in.

@ayoub-khemissi

Copy link
Copy Markdown
Author

Fixes from review feedback

1. Split hook_cmd.rs into modules

The monolithic 1127-line file has been split into 9 files under src/hook/:

File Responsibility
mod.rs Entry point, chain parsing, orchestrator
git.rs Git + GitHub CLI
cargo.rs Cargo
files.rs cat, grep, ls, tree, find, diff, head, curl, wget
js_ts.rs vitest, tsc, eslint, prettier, playwright, prisma, pnpm
containers.rs Docker + kubectl
python.rs pytest, ruff, pip/uv
go.rs go test/build/vet, golangci-lint
helpers.rs Shared utilities (replace_prefix, starts_with_any)

All 71 hook tests preserved and passing.

2. Fix pnpm eslint (the 1/313 missing)

Added pnpm eslint . → rtk lint . support in js_ts.rs, with 2 dedicated tests (test_pnpm_eslint, test_pnpm_eslint_bare).

→ 313/313 commands now covered.

3. Fix CI validate-docs

  • Updated version 0.18.1 → 0.19.0 in README.md, CLAUDE.md, ARCHITECTURE.md
  • Fixed module count 47 → 49 in ARCHITECTURE.md
  • Updated workflow and validate-docs script to check src/hook/ (native hook) instead of .claude/hooks/rtk-rewrite.sh (legacy bash hook)

@pszymkowiak

pszymkowiak commented Feb 20, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up: Real Execution Testing + Migration Warning

Following up on my earlier 313-command protocol test — I went deeper and tested every RTK filter with real toolchains (not just the JSON rewrite protocol). Also found critical issues with the auto-migration behavior.

Real Execution: 98/98 pass

Built the PR branch (merged master, zero conflicts, 491 tests pass) and tested against actual projects in /tmp/:

Ecosystem Commands Pass Tools
Rust 9 9 cargo build/test/check/clippy/fmt
Node.js 15 15 vitest, eslint, prettier, playwright, tsc, pnpm, npm, prisma
Python 11 11 pytest, ruff check/format, pip list/outdated/show
Go 8 8 go test/build/vet, golangci-lint
Git 11 11 status, log, diff, branch, show, fetch, stash, etc.
GitHub CLI 9 9 pr, issue, run, release (list/view/diff)
Docker/K8s 4 4 ps, images, pods, services
Files 18 18 ls, tree, find, read, grep, diff, json, log, smart, wc, env, deps
Network 3 3 curl, wget
Meta 8 8 gain, version, init, err, summary, proxy
Total 98 98

All RTK filters work perfectly with real command output. Zero regressions vs master.


Critical: Don't Auto-Migrate to Native Hook

init.rs hardcodes HOOK_COMMAND = "rtk hook-rewrite" and migrate_old_hook() silently replaces the bash hook on rtk init.

This is breaking. Deeper testing (beyond the 313 protocol commands) revealed 3 P0 bugs that would silently corrupt user workflows:

P0 — Must fix before making native hook default

# Bug Example Impact
1 gh --json/--jq/--template rewritten gh pr list --json number → rtk gh pr list --json number Breaks JSON parsing for scripts/jq/Claude Code (#196)
2 Git global flags crash RTK git --no-pager log → rtk git --no-pager log → error Breaks routinely used --no-pager
3 cat file1 file2 multi-file → crash cat f1.txt f2.txt → rtk read f1.txt f2.txt → error rtk read only accepts single file

These aren't edge cases — gh --json and git --no-pager are used in almost every Claude Code session.

Suggested approach:

  1. Keep bash hook as default when merging
  2. Add opt-in: rtk init --native-hook for early adopters
  3. Fix P0s → then switch default in a future release

P1 — Should fix

# Bug Example Fix
4 head -n N not handled head -n 20 file → not rewritten Add regex for head -n (\d+) in hook/files.rs
5 Pipes not split git log | head -5 → rewritten as one command Split on | like &&/;

P2 — Nice to have (follow-up PRs)

pnpm install/pnpm run, env/printenv, bun commands, wc, tail -n N — all have RTK filters but no hook rewrite routing yet.


Roadmap: Toward an Extensible Hook Engine

This PR provides excellent rewrite coverage (hardcoded in Rust modules). Long term, we want the community to add rewrite rules without modifying Rust source code.

We plan to also leverage ideas from PR #156 (Hook Engine by @ahundt) which brings:

  • A lexer + chain analysis engine for quote-aware command parsing
  • A rtk run -c executor that routes commands to the right filter at runtime
  • A rtk hook check debugger for testing rewrites without execution

Combined with the declarative TOML plugin system (#139), this enables:

# ~/.config/rtk/plugins/terraform.toml
[filter]
command = "terraform"
subcommands = ["plan", "apply", "init"]
strategy = "strip"
patterns = ["^Acquiring", "^- Installed", "^Terraform has been"]

Community contributors would add support for new tools by dropping a .toml file — no Rust compilation needed.

Why not JS hooks? (ref: closed PR #141)

  • Performance: Node.js/Bun adds 30-80ms startup overhead per hook invocation — RTK targets <10ms. Every command triggers the hook; this latency compounds fast
  • Runtime dependency: Requires Node.js or Bun — breaks the single static binary philosophy
  • Maintenance burden: Duplicates regex logic across two codebases (bash + JS) that must stay in sync
  • Not extensible: Hardcoded patterns, no plugin mechanism
  • This PR's native Rust binary already solves Windows support (CI builds Windows binaries)

Bottom Line

RTK filters are rock solid (98/98 real execution). The hook architecture is clean and well-modularized.

Recommendation: Merge but keep bash hook as default. Fix the 3 P0s in follow-ups, then switch. Long term, combine with PR #156's engine + TOML plugins for community-extensible rewrite rules.

Happy to discuss any of these points.

@pszymkowiak

Copy link
Copy Markdown
Collaborator

Please rebase on master — there are conflicts in ARCHITECTURE.md and src/main.rs. Also please address the P0 bugs raised in the earlier review before we can merge.

@ayoub-khemissi

Copy link
Copy Markdown
Author

Fixes from review feedback (round 2)

1. Rebased on upstream/master

Merged 10 upstream commits (up to 0.23.0). Conflict in ARCHITECTURE.md resolved (module count updated to 52). src/main.rs auto-merged cleanly. 524 tests pass.

2. P0 bug fixes — all 3 resolved

# Bug Root cause Fix Tests added
P0-1 gh pr list --json number → rtk gh pr list --json number breaks JSON parsing try_rewrite_gh didn't check for structured output flags Skip rewrite when --json, --jq, or --template present in command (hook/git.rs) 5 tests
P0-2 git --no-pager log → rtk git --no-pager log → RTK parse error Hook stripped global flags for subcommand detection but preserved them in the rewritten output Strip global flags (--no-pager, -C, --bare, --no-optional-locks, -c key=val) from both match and output (hook/git.rs) 6 tests
P0-3 cat f1.txt f2.txt → rtk read f1.txt f2.txt → crash try_rewrite_file_cmd blindly replaced cat with rtk read regardless of arg count Count non-flag arguments; skip rewrite if != 1 file, also skip cat - (stdin) (hook/files.rs) 5 tests

Consistency with existing RTK patterns:

  • P0-1: gh_cmd.rs already has should_passthrough_run_view() checking --json — same pattern
  • P0-2: git.rs receives args via clap trailing_var_arg and passes to Command::new("git") — RTK never sees global flags, so stripping is correct
  • P0-3: read.rs and main.rs define file: PathBuf (singular) — rtk read only accepts one file

3. Keep bash hook as default, add --native-hook opt-in

Per @pszymkowiak's recommendation: the native hook needs more battle-testing before becoming the default.

Before (this PR): rtk init -g → native hook (rtk hook-rewrite), auto-migrates bash hook
After: rtk init -g → bash hook (stable default), rtk init -g --native-hook → native hook (opt-in)

Scenario Behavior
rtk init -g (Unix) Installs bash hook (~/.claude/hooks/rtk-rewrite.sh) — same as upstream
rtk init -g --native-hook Installs native hook (rtk hook-rewrite), migrates old bash hook
rtk init -g (Windows, no --native-hook) Falls back to --claude-md mode with message
rtk init -g --native-hook (Windows) Installs native hook (cross-platform)
rtk init --show Displays hook type (bash or native) neutrally
rtk init -g --uninstall Removes both hook types

4. Code quality fixes

  • Renamed _match_cmd → match_cmd (variable is used, underscore was misleading)
  • Used strip_prefix instead of manual slice indexing for cat arg parsing
  • Simplified redundant hook_already_present logic (3 conditions → same coverage)
  • Added edge case test: cat - (stdin) → no rewrite
  • Added documentation test: unknown git global flags → safe fallback (no rewrite)

Quality gate

Check Result
cargo fmt --all --check PASS
cargo clippy --all-targets PASS — 0 new warnings
cargo test --all PASS — 524 tests, 0 failures
Hook tests specifically 88 tests (was 71 before, +17 new)

@pszymkowiak

Copy link
Copy Markdown
Collaborator

Thanks for fixing the previous P0 bugs — all three confirmed fixed, and the architecture is clean.

However, two new P1 bugs were introduced:

P1-1: git -C flag stripped silently
git -C /other/repo status is rewritten to rtk git status, which runs in the wrong directory with no warning. Fix: return no rewrite when -C is present.

P1-2: grep -r rewrite breaks at runtime
grep -r TODO . → rtk grep -r TODO . fails immediately with a clap error — rtk grep expects the pattern first, not flags. This is a copy of the same bug that exists in the bash hook.

This is the 3rd review cycle on this PR. Before submitting the next update, could you please run the full hook test suite manually?

HOOK=hooks/rtk-rewrite.sh bash hooks/test-rtk-rewrite.sh

It covers 57 commands and would have caught both issues above. We can't merge a hook that rewrites 500+ commands per session without that baseline passing.

@ayoub-khemissi
ayoub-khemissi force-pushed the feat/native-hook-rewrite branch from 6d7592d to 27958f5 Compare March 2, 2026 10:21
@ayoub-khemissi

Copy link
Copy Markdown
Author

P1 Fixes — Verification Report

Summary

Both P1 regressions from the previous review are now resolved in commit 27958f5.


P1-1: git -C /path status — silent wrong directory

Root cause: GIT_GLOBAL_FLAGS_RE regex was stripping -C <path> from the rewritten command. The -C flag tells git which directory to operate in — removing it silently runs the command in the wrong directory.

Fix (src/hook/git.rs): Early-return None when -C is detected. The command runs unmodified through native git (no rewrite = no risk of wrong directory). Other cosmetic flags (--no-pager, --bare, --no-optional-locks) are still stripped safely.

// Before: -C silently stripped → wrong directory
"git -C /path status" → "rtk git status"  // ❌ operates on CWD, not /path

// After: skip rewrite entirely when -C present
"git -C /path status" → None (no rewrite)  // ✅ runs original git command

Tests added (3 new):

  • test_git_dash_c_path_no_rewrite — status
  • test_git_dash_c_path_log_no_rewrite — log
  • test_git_dash_c_path_diff_no_rewrite — diff with relative path

Note: lowercase -c key=val (config override) is still stripped — it's cosmetic and doesn't change directory context.


P1-2: grep -r TODO . — clap argument ordering error

Root cause: The hook did a naive prefix replacement (grep → rtk grep) without considering argument order. RTK's clap definition expects PATTERN PATH [-- extra_args...] but real-world grep usage puts flags first: grep -r -i PATTERN .. Clap sees -r where it expects a positional arg and fails.

Fix (src/hook/files.rs): Added reorder_grep_args() function that:

  1. Parses tokens into flags vs positionals
  2. Handles flags with values (-A 3, --include *.rs) — consumes next token
  3. Handles boolean flags (-r, -i, -rn)
  4. Handles -- separator
  5. Reconstructs as rtk grep PATTERN PATH -- FLAGS
// Before: clap error (flag before positional)
"grep -r TODO ."     → "rtk grep -r TODO ."      // ❌ clap: unexpected argument '-r'

// After: args reordered for clap compatibility
"grep -r TODO ."     → "rtk grep TODO . -- -r"    // ✅
"grep -r -i pat src" → "rtk grep pat src -- -r -i" // ✅
"grep -A 3 pat ."   → "rtk grep pat . -- -A 3"    // ✅
"grep TODO src/"     → "rtk grep TODO src/"         // ✅ (no flags = no --)
"rg -i pattern ."   → "rtk grep pattern . -- -i"   // ✅ (rg also handled)

Tests added (7 new):

  • test_grep_flags_reordered — -r flag
  • test_grep_multiple_flags_reordered — -r -i combined
  • test_grep_combined_flags_reordered — -rn combined flag
  • test_rg_flags_reordered — rg with -i
  • test_grep_flag_with_value — -A 3 (value-consuming flag)
  • test_grep_no_flags_no_separator — no flags case
  • test_rg_no_flags_no_separator — rg no flags case

Quality Gates

Check Result
cargo fmt --all --check ✅ Clean
cargo clippy --all-targets (hook/) ✅ Zero warnings
cargo test --all ✅ 535 passed, 0 failed
HOOK=hooks/rtk-rewrite.sh bash hooks/test-rtk-rewrite.sh ✅ 52/61 passed (9 pre-existing failures¹)

¹ Pre-existing failures unrelated to P1 changes:

  • 3× find/tree/wget: bash hook rewrites them but test expects no rewrite (test/hook mismatch)
  • 6× audit logging: feature not implemented in bash hook

Files Changed

File Change
src/hook/git.rs Early-return None for -C flag + 3 tests
src/hook/files.rs reorder_grep_args() parser + 7 tests

ahundt added a commit to ahundt/rtk that referenced this pull request Mar 4, 2026
Move protocol-specific modules into src/cmd/hook/ to group LLM adapters
together and mirror the structure of PR rtk-ai#150's src/hook/ directory:
- hook.rs → hook/mod.rs (dispatch engine becomes module root)
- claude_hook.rs → hook/claude.rs (no redundant _hook suffix)
- gemini_hook.rs → hook/gemini.rs (symmetric with claude.rs)
- trash_cmd.rs → trash.rs (no redundant _cmd suffix; consistent with builtins.rs)

safety.rs stays flat in src/cmd/ — shared by exec.rs and hook/mod.rs.

Update module declarations (cmd/mod.rs), dispatch (main.rs:1672-1677),
exec.rs import (trash_cmd→trash), and internal import paths in hook/{claude,gemini}.rs
(super::hook:: → super::).

906 tests pass.
ahundt added a commit to ahundt/rtk that referenced this pull request Mar 4, 2026
…parison notes (Part 4)

Add tests and fixes identified during PR comparison review with rtk-ai#150 and rtk-ai#241:

analysis.rs:
- test_cargo_test_pipe_grep_is_not_safe_suffix: regression guard — "cargo test | grep FAILED"
  must not be classified as a safe suffix (pipe target is format-sensitive)
- test_nohup_background_strips_ampersand: edge case — "nohup cargo build &" strips
  trailing & as safe suffix; core [nohup, cargo, build] routes to passthrough (correct)

builtins.rs:
- is_valid_env_name(): POSIX identifier validation [A-Za-z_][A-Za-z0-9_]*
- builtin_export(): rejects invalid identifiers (e.g. "123=x") fail-open (silent skip)
- tests: test_export_invalid_identifier_ignored, test_export_empty_name_ignored,
  test_is_valid_env_name (7 assertions for valid/invalid identifier edge cases)

hook/mod.rs:
- test_cat_multi_file_is_blocked: cat is blocked by src/rules/rtk.safety.block-cat.md;
  defensive cat→rtk-read fallback in route_native_command() exercises only with
  RTK_BLOCK_TOKEN_WASTE=0; verified actual runtime behavior is Blocked
- test_cat_single_file_is_blocked: same blocking rule applies regardless of arity

.claude/notes/hook-pr-comparison.md:
- Mark export identifier validation as fixed (with test names)
- Correct audit logging comparison: rtk-ai#156 has equivalent via RTK_AUDIT_DIR /
  hook_audit_cmd.rs (always-on, dir-based) vs rtk-ai#241's RTK_HOOK_AUDIT=1 (flag-gated)
- Update "What rtk-ai#241 brings" section: oracle model is rtk-ai#156's gap; audit logging is parity
- Update "What rtk-ai#241 has over both" with accurate parity note for audit logging

All 913 tests pass.
ahundt added a commit to ahundt/rtk that referenced this pull request Mar 4, 2026
Move protocol-specific modules into src/cmd/hook/ to group LLM adapters
together and mirror the structure of PR rtk-ai#150's src/hook/ directory:
- hook.rs → hook/mod.rs (dispatch engine becomes module root)
- claude_hook.rs → hook/claude.rs (protocol adapter, no _hook suffix)
- trash_cmd.rs → trash.rs (no _cmd suffix, consistent with builtins.rs)

Update module declarations (cmd/mod.rs), dispatch (main.rs:1523), and
internal import path in hook/claude.rs (super::hook:: → super::).

818 tests pass; pre-existing warnings unchanged.
ahundt added a commit to ahundt/rtk that referenced this pull request Mar 4, 2026
…ation (Part 4)

Add tests and fixes identified during PR comparison review with rtk-ai#150 and rtk-ai#241:

analysis.rs:
- test_cargo_test_pipe_grep_is_not_safe_suffix: regression guard for pipe-to-grep
  routing — verifies "cargo test | grep FAILED" is not classified as a safe suffix
  and triggers shell passthrough (pipe target is format-sensitive)
- test_nohup_background_strips_ampersand: edge case — "nohup cargo build &" strips
  trailing & as safe suffix; core [nohup, cargo, build] does not require shell

builtins.rs:
- is_valid_env_name(): POSIX shell identifier validation [A-Za-z_][A-Za-z0-9_]*
- builtin_export(): rejects invalid identifiers (e.g. "123=x") with fail-open
  behavior (silent skip, never error — preserves RTK's fail-open principle)
- tests: test_export_invalid_identifier_ignored, test_export_empty_name_ignored,
  test_is_valid_env_name (7 assertions covering valid/invalid identifier patterns)

hook/mod.rs:
- test_cat_multi_file_rewrites_to_rtk_read: documents that on this branch (without
  data-safety rules), cat → rtk read for all arities via defensive fallback in
  route_native_command(); contrasts with feat/multi-platform-hooks where cat is
  Blocked by src/rules/rtk.safety.block-cat.md
- test_cat_single_file_rewrites_to_rtk_read: same fallback path, single-file case

All 825 tests pass.
@FlorianBruniaux

Copy link
Copy Markdown
Contributor

Hi @ayoub-khemissi! PR #241 (rtk rewrite) just merged and touched the hook system significantly — this will likely create conflicts with your native cross-platform hooks PR. Could you rebase and check the conflict areas? The feature is valuable and we want to move it forward.

ayoub-khemissi and others added 9 commits March 5, 2026 20:36
Windows default stack is 1 MB vs 8 MB on Unix. The large Commands enum
(30+ Clap variants) overflows in unoptimized debug builds. Add build.rs
with /STACK linker flag for Windows only, zero runtime overhead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split the monolithic 1127-line hook_cmd.rs into 8 focused modules under
src/hook/ for better maintainability and code review:

- mod.rs: entry point, chain parsing, orchestrator
- git.rs: git + GitHub CLI rewriters
- cargo.rs: cargo rewriter
- files.rs: file ops (cat, grep, ls, tree, find, diff, head) + network
- js_ts.rs: JS/TS tooling (vitest, tsc, eslint, prettier, playwright, prisma, pnpm)
- containers.rs: Docker + kubectl rewriters
- python.rs: pytest, ruff, pip/uv rewriters
- go.rs: go test/build/vet + golangci-lint rewriters
- helpers.rs: shared utility functions (replace_prefix, starts_with_any)

All 71 hook tests passing. No behavioral changes.
- Update version references from 0.18.1 to 0.19.0 in README.md,
  CLAUDE.md, and ARCHITECTURE.md
- Update module count in ARCHITECTURE.md (47 → 49)
- Update validate-docs workflow and script to check native hook module
  (src/hook/) instead of deprecated bash script (.claude/hooks/rtk-rewrite.sh)
- P0-1: gh --json/--jq/--template no longer rewritten (preserves
  structured output for scripts and Claude Code JSON parsing)
- P0-2: git global flags (--no-pager, -C, --bare, etc.) stripped from
  rewritten command (RTK doesn't understand these flags)
- P0-3: cat with multiple files no longer rewritten to rtk read
  (rtk read only accepts a single file argument)

Adds 15 new tests covering all three bug scenarios.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per reviewer feedback: the native hook (rtk hook-rewrite) has 3 P0 bugs
that need battle-testing before becoming the default. This commit:

- Restores bash hook as default on Unix (rtk init -g)
- Adds --native-hook flag for early adopters who want cross-platform
  support (Windows) or want to opt out of bash/jq dependency
- Windows without --native-hook falls back to --claude-md mode
- Auto-migration from bash→native only happens with --native-hook
- show_config displays both hook types neutrally

Usage:
  rtk init -g                  # bash hook (default, stable)
  rtk init -g --native-hook    # native hook (opt-in, cross-platform)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename _match_cmd → match_cmd (variable is used, underscore misleading)
- Use strip_prefix instead of manual slice for cat arg parsing
- Skip cat - (stdin) rewrite — rtk read doesn't support stdin
- Add test for unknown git global flags (safe fallback to no-rewrite)
- Simplify redundant hook_already_present logic

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P1-1: git -C /path now skips rewrite (was silently stripping directory change)
P1-2: grep/rg with flags before pattern now skips rewrite (was causing clap error)

Bash hook test suite: 52/61 pass (9 expected: 3 design diffs + 6 Windows audit)
Native hook tests: 98/98 pass, full suite: 534/534 pass

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The REWRITE_HOOK constant is now #[cfg(unix)] after the
--native-hook opt-in refactor. The test_hook_has_guards test
must also be gated to avoid compilation errors on Windows.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ayoub-khemissi
ayoub-khemissi force-pushed the feat/native-hook-rewrite branch from 27958f5 to ea3ceb1 Compare March 5, 2026 20:15
@ayoub-khemissi

Copy link
Copy Markdown
Author

Rebased on upstream/master (includes PR #241 rtk rewrite)

Rebased on latest upstream/master as requested by @FlorianBruniaux. All conflicts resolved cleanly.

Conflict areas resolved

File Resolution
src/main.rs Kept HookAudit + Rewrite (upstream) alongside HookRewrite (native)
src/init.rs Merged bash hook (default) + native hook (--native-hook opt-in), kept integrity checks
src/discover/registry.rs Removed inlined PATTERNS/RULES (already in rules.rs since PR #241)
Docs (ARCHITECTURE, CLAUDE, README) Kept upstream versions (0.24.0, 55 modules)
.github/workflows/validate-docs.yml Kept thin delegator check from PR #241

Post-rebase fix

  • Added #[cfg(unix)] on test_hook_has_guards — REWRITE_HOOK constant is now Unix-only after the --native-hook refactor

Quality gates

Check Result
cargo fmt --all --check ✅ Clean
cargo clippy --all-targets ✅ 0 errors
cargo test --all ✅ 712 passed, 0 failed, 2 ignored
cargo test --all -- --ignored ✅ 2 integration tests pass
cargo build --release ✅ 0 errors

Ready for review.

@ala-dojo

ala-dojo commented Mar 18, 2026 •

Copy link
Copy Markdown

Hi, I opened a discussion documenting this limitation:

@CLAassistant

CLAassistant commented Mar 20, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Warrio111

Copy link
Copy Markdown

Looking for this PR 🧐, @ayoub-khemissi

@pszymkowiak

Copy link
Copy Markdown
Collaborator

Hi! Two things needed before we can review:

  1. Retarget to develop — this PR targets master, but all PRs should target develop. You can change the base branch in the PR settings (right sidebar).
  2. Sign the CLA — if not already done, please sign at https://cla-assistant.io/rtk-ai/rtk

Thanks!

@aeppling

Copy link
Copy Markdown
Contributor

Hey

We are cleaning up the codebase and improving the project structure for better onboarding. As part of this effort, PR #826 reorganizes src/ from a flat layout into subfolders.

No logic changes — only file moves and import path updates.

What you need to do

Rebase your branch on develop when receiving this comment:

git fetch origin && git rebase origin/develop

Git detects renames automatically. If you get import conflicts, update the paths:

use crate::git;        // now: use crate::cmds::git::git;
use crate::tracking;   // now: use crate::core::tracking;
use crate::config;     // now: use crate::core::config;
use crate::init;       // now: use crate::hooks::init;
use crate::gain;       // now: use crate::analytics::gain;

Need help rebasing? Tag @aeppling

@ayoub-khemissi

Copy link
Copy Markdown
Author

This PR has been superseded by #1090, which takes a cleaner approach based on the review feedback here.

Why a new PR instead of rebasing: upstream develop has changed significantly since this PR was opened (registry-based rewrite system in discover/registry.rs, codebase reorganization into subfolders, native hook processing via rtk hook copilot). The 8 per-language modules in src/hook/ are now fully superseded by the registry. Rebasing 9 commits over this produced modify/delete conflicts on the first commit — not viable.

What #1090 does: fresh branch from develop, reuses the existing rtk hook copilot command, adds only the --native-hook flag to rtk init for cross-platform (Windows) support. 3 files, 500 LOC vs 14 files, 1905 LOC here.

I'll leave this PR open for you to close at your discretion.

aeppling added a commit that referenced this pull request Apr 19, 2026
Removing old guards, windows can now just use the binary hook engine from 0.37

Related issues:
- Fixes #502 : rtk init --global falls back to --claude-md on Windows
- Fixes #1353 : Feature request: hook-based mode on Windows
- Partially addresses #330 : Add hooks support for Windows
- Partially addresses #913 :  Persistent "No hook installed" warning on Windows
- Partially addresses #1373 : Suppress "No hook installed" warning on Windows
- Partially addresses #682 : Config to suppress hook warning
- Related to #1248 : Windows PowerShell compatibility gaps

Supersedes community PRs:
- #1123 fix(init): enable hook installation on Windows
- #1027 fix(init): enable hook-based mode on Windows
- #809 feat: enable hook-based mode on Windows
- #452 feat: add Windows hook support for rtk init --global
- #551 feat: native cross-platform hook for Windows support
- #150 feat(hook): native cross-platform hook-rewrite command
- #1063 Feat/windows hooks
@aeppling

Copy link
Copy Markdown
Contributor

Thanks for contributing @ayoub-khemissi — this was the original comprehensive native hook implementation (1900+ LOC) and it pioneered the approach that master eventually adopted.

This is now superseded by the v0.37.x releases: master ships src/hooks/hook_cmd.rs with multi-agent support (Claude, Cursor, Copilot, Gemini) and registry-based rewriting. Closing as resolved.

@aeppling aeppling closed this Apr 24, 2026
thehoff referenced this pull request in thehoff/contextcrawler May 14, 2026
Removing old guards, windows can now just use the binary hook engine from 0.37

Related issues:
- Fixes rtk-ai#502 : rtk init --global falls back to --claude-md on Windows
- Fixes rtk-ai#1353 : Feature request: hook-based mode on Windows
- Partially addresses rtk-ai#330 : Add hooks support for Windows
- Partially addresses rtk-ai#913 :  Persistent "No hook installed" warning on Windows
- Partially addresses rtk-ai#1373 : Suppress "No hook installed" warning on Windows
- Partially addresses rtk-ai#682 : Config to suppress hook warning
- Related to rtk-ai#1248 : Windows PowerShell compatibility gaps

Supersedes community PRs:
- rtk-ai#1123 fix(init): enable hook installation on Windows
- rtk-ai#1027 fix(init): enable hook-based mode on Windows
- rtk-ai#809 feat: enable hook-based mode on Windows
- rtk-ai#452 feat: add Windows hook support for rtk init --global
- rtk-ai#551 feat: native cross-platform hook for Windows support
- #150 feat(hook): native cross-platform hook-rewrite command
- rtk-ai#1063 Feat/windows hooks
thehoff referenced this pull request in thehoff/contextcrawler May 14, 2026
Removing old guards, windows can now just use the binary hook engine from 0.37

Related issues:
- Fixes rtk-ai#502 : rtk init --global falls back to --claude-md on Windows
- Fixes rtk-ai#1353 : Feature request: hook-based mode on Windows
- Partially addresses rtk-ai#330 : Add hooks support for Windows
- Partially addresses rtk-ai#913 :  Persistent "No hook installed" warning on Windows
- Partially addresses rtk-ai#1373 : Suppress "No hook installed" warning on Windows
- Partially addresses rtk-ai#682 : Config to suppress hook warning
- Related to rtk-ai#1248 : Windows PowerShell compatibility gaps

Supersedes community PRs:
- rtk-ai#1123 fix(init): enable hook installation on Windows
- rtk-ai#1027 fix(init): enable hook-based mode on Windows
- rtk-ai#809 feat: enable hook-based mode on Windows
- rtk-ai#452 feat: add Windows hook support for rtk init --global
- rtk-ai#551 feat: native cross-platform hook for Windows support
- #150 feat(hook): native cross-platform hook-rewrite command
- rtk-ai#1063 Feat/windows hooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-changes P1-critical Bloque des utilisateurs, fix ASAP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants