fix: P0+P1 fixes from pre-merge review of hook engine

[aeppling]

Fixes 8 issues (3 P0, 5 P1) identified during pre-merge review of the hook engine (feat-hook-engine branch).

Summary

P0 — Blockers

P0.1: Silent output drop on command failure (runner.rs:81)

When the underlying command fails (non-zero exit), the skip_filter_on_failure path returned the exit code without printing any output. Affects 14 call sites (ls, tree, psql, kubectl, docker, gh).

$ echo '{"tool_name":"Bash","tool_input":{"command":"ls /nonexistent"}}' | rtk hook claude
→ hook rewrites to: rtk ls /nonexistent

$ rtk ls /nonexistent
(blank output, exit 2)          ← BEFORE
/usr/bin/ls: cannot access '/nonexistent': No such file or directory  ← AFTER

P0.2: hook and pipe missing from META_COMMANDS (main.rs:1078)

On Clap parse failure (version mismatch, typo in settings.json), RTK fell through to shell execution instead of showing a Clap error.

$ rtk hook claud
sh: 1: hook: not found (exit 127)   ← BEFORE (shell fallback)

$ rtk hook claud
error: unrecognized subcommand 'claud'
  tip: a similar subcommand exists: 'claude'  ← AFTER (Clap error)

P0.3: store_hash never called — integrity system inert (init.rs)

Gemini bash script was installed without storing a SHA-256 baseline. Tamper detection was dead on arrival.

P1 — Important

P1.1: Audit log only covers Claude Code path (hook_cmd.rs)

RTK_HOOK_AUDIT=1 produced no output for Copilot, Gemini, or Cursor hooks.

$ RTK_HOOK_AUDIT=1 echo '{"tool_name":"runTerminalCommand","tool_input":{"command":"git status"}}' | rtk hook copilot
$ tail -1 ~/.local/share/rtk/hook-audit.log
2026-04-16T15:23:06 | rewrite | git status | rtk git status   ← now logged

P1.2: Cursor always emits "permission": "allow" (hook_cmd.rs:437)

Permission system bypassed — every rewritten command got "allow" regardless of deny/ask rules.

$ echo '{"tool_input":{"command":"git status"}}' | rtk hook cursor
{"permission":"allow",...}   ← BEFORE (hardcoded)
{"permission":"ask",...}     ← AFTER (respects permission system)

$ echo '{"tool_input":{"command":"cargo test"}}' | rtk hook cursor
{"permission":"allow",...}   ← cargo test IS in allow list, correctly gets "allow"

P1.5: failure_lines collected but never used (runner.rs:189)

extract_test_summary collected cargo test failure details (assertion messages, expected vs actual) but never included them in output. Agent only saw failure names, not diagnostics.

P1.7: libc declared unconditionally (Cargo.toml)

libc = "0.2" in [dependencies] pulled it in on all platforms. Only needed under [target.'cfg(unix)'.dependencies].

P1.8: Migration leaves stale settings.json entry (init.rs:969)

migrate_old_hook_script() deleted ~/.claude/hooks/rtk-rewrite.sh but left the stale entry in settings.json. hook_already_present() matched the stale entry and blocked the new rtk hook claude from being registered. Hooks silently broken after upgrade. Same issue for Cursor hooks.json.

# Old settings.json has: "command": "/home/user/.claude/hooks/rtk-rewrite.sh"
# After rtk init --global:
#   BEFORE: script deleted, stale entry stays, new entry blocked → hooks broken
#   AFTER:  script deleted, stale entry cleaned, new entry added → hooks work

Noted (no fix needed)

• P1.3: Cargo streaming vs buffered dual impl — by design (different subcommands need different modes)
• P1.6: Pipe filter savings ~47% vs 60% target — TODO, needs filter algorithm improvement
• P1.9: FilterMode::Buffered only in tests — valid API surface

Test plan

• cargo fmt --all && cargo clippy --all-targets && cargo test --all — 1590 passed, 0 failed, no new clippy warnings
• P0.1: rtk ls /nonexistent prints error message to stderr, exits 2
• P0.2: rtk hook claud shows Clap error with suggestion, not shell fallback
• P0.3: store_hash called after Gemini script install (clippy dead-code warning gone)
• P1.1: RTK_HOOK_AUDIT=1 + Copilot hook → audit log entry written to ~/.local/share/rtk/hook-audit.log
• P1.2: Cursor hook returns "permission":"ask" for non-allowed commands, "permission":"allow" for allowed ones
• P1.8: 6 new unit tests for legacy migration — old entries stripped, new entries preserved, third-party hooks untouched (Claude + Cursor)
• All reproductions use realistic agent payloads (JSON on stdin via hook protocol), not manual CLI invocations

[pszymkowiak]

Review — PR #1346 (P0/P1 fixes)

Good progress. 5 out of 8 fixes are correct. 3 issues need attention before merge.

Fix status

• P0.1 (output drop): PARTIAL — eprint! writes to stderr instead of stdout. Breaks piping: rtk ls /path | grep foo won't work because the output goes to stderr. Fix: change eprint!("{}", raw) to print!("{}", raw) at runner.rs:83.

• P0.2 (META_COMMANDS): FIXED — "hook" and "pipe" added, completeness test covers them.

• P0.3 (store_hash): PARTIAL — wired for Gemini at init.rs:2438 but not for the Claude native hook path. If the Claude path is intentionally excluded (binary, not script), document it. Otherwise wire it.

• P1.1 (audit log): FIXED, except Gemini deny path at hook_cmd.rs:206-211 — writes {"decision":"deny"} but never calls audit_log("deny", cmd, ""). All other deny paths now call it.

• P1.2 (Cursor "allow"): FIXED — verdict now checked, maps to "allow"/"ask".

• P1.3 (failure_lines): FIXED — now consumed in output.

• P1.4 (40% threshold): NOT FIXED — still >= 40.0 with a TODO comment. A comment is not a fix. Either bump to 60% or document the exception.

• P1.5 (libc duplicate): FIXED.

Blockers (2)

1. runner.rs:83 — eprint! → print!. One character change.
2. hook_cmd.rs:211 — add audit_log("deny", cmd, ""); in the Gemini deny branch.

Minor

• run_cursor and run_cursor_inner duplicate permission mapping logic. Extract a shared helper.
• failure_lines block has no visual separator from the failures list above it.

[aeppling]

P0.1 : look good to me, for error we use eprint , to be transparent , returning in stderr as real err
P0.3: store_hash only Gemini: By design — Claude/Cursor use binary commands, no script file on disk to hash.
P1.4: 60% not possible for the moment without loosing signal, need rework the filter itself

[aeppling]

wait, my bad sorry , for p0.1 we should split stream for stderr and use eprint & print respectivly, working on it, was too fast on my answer

[aeppling]

fixed :) thanks for the review

[FlorianBruniaux]

Local build + test run. 1 test failing.

Failing test — test_cursor_rewrite_flat_format

thread '...test_cursor_rewrite_flat_format' panicked at src/hooks/hook_cmd.rs:765:9:
assertion `left == right` failed
  left: String("allow")
 right: "ask"

Root cause: run_cursor_inner calls permissions::check_command() which reads the real settings.json/settings.local.json from disk. The test assumes git status has no allow rule (verdict = Default → "ask"), but on machines with Bash(git:*) in their local settings that command gets Allow → "allow".

You already fixed test_cursor_no_hook_specific_output for this — you made the assertion permissive (perm == "allow" || perm == "ask"). Same fix needed on line 765, or better: use check_command_with_rules with empty rules in run_cursor_inner to guarantee isolation regardless of what's in the developer's settings files. The second option is cleaner and tests the actual default-permission code path explicitly.

Typo from #1277 still open

pipe_cmd.rs:70: "{} matches in {}F:\n\n" — the F: artifact was in my review of #1277 but wasn't addressed here. Quick fix: "{} matches in {} files:\n\n".

---

Everything else looks correct. The P0.1 fd-separation fix (stdout → print!, stderr → eprint!) and the raw_stderr field addition are solid. P1.8 migration logic in init.rs is a meaningful fix for the stale-entry upgrade bug.

Fix the test isolation issue and the typo, then good to go.

[aeppling]

For the typo, make sense for RTK to compress "files" into "F"

[FlorianBruniaux]

ok