rtk-ai / rtk-ai/rtk

rtk init -g --uninstall removes foreign PreToolUse hooks sharing a matcher group

Open Beginner friendly
#3,036 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

area:cli area:security bug help wanted priority:high
Dominant language
Rust
Stars
81.1k
Forks
5.1k
Avg merge
4d 21h
Merged PRs (30d)
35

Description

Summary

rtk init -g --uninstall removes the entire PreToolUse matcher group that rtk's hook lives in, instead of removing only rtk's own hook entry. If a user's rtk hook shares a matcher group with any foreign hook, that foreign hook is silently deleted too.

This is a data-loss bug for anything security-relevant: in my case the shared group held a PreToolUse: Bash guard that blocks commands from dumping secrets to stdout. Uninstalling rtk would silently delete it.

Version: 0.43.0 (Homebrew bottle, matches current master).

Repro

~/.claude/settings.json with rtk's hook in the same matcher group as a foreign hook:

{
  "hooks": {
    "PreToolUse": [
      {
        "matcher": "Bash",
        "hooks": [
          { "type": "command", "command": "node ~/.claude/hooks/my-security-guard.js" },
          { "type": "command", "command": "rtk hook claude" }
        ]
      }
    ]
  }
}

Run:

rtk init -g --uninstall

Expected: only rtk hook claude is removed; my-security-guard.js survives.
Actual: the whole "matcher": "Bash" entry is gone, including my-security-guard.js.

Root cause

src/hooks/init.rsremove_hook_from_json (line 529), the retain at lines 544-556:

pre_tool_use_array.retain(|entry| {
    if let Some(hooks_array) = entry.get("hooks").and_then(|h| h.as_array()) {
        for hook in hooks_array {
            if let Some(command) = hook.get("command").and_then(|c| c.as_str()) {
                // Match both legacy script path and new binary command
                if command.contains(REWRITE_HOOK_FILE) || command == CLAUDE_HOOK_COMMAND {
                    return false;
                }
            }
        }
    }
    true
});

retain iterates over entries (matcher groups), but the predicate returns false as soon as any hook inside the entry matches rtk. So one matching hook discards the entry and every sibling hook in it.

This looks unintended, not by design

Two things in the codebase point the same way:

  1. remove_legacy_hook_entries_from_json (line 1322) already gets this right — it only drops an entry when all of its hooks are rtk hooks:

    let dominated_by_legacy = entry
        .get("hooks")
        .and_then(|h| h.as_array())
        .map(|hooks| {
            hooks.iter().all(|hook| { /* ... contains(REWRITE_HOOK_FILE) ... */ })
        })
        .unwrap_or(false);
    

    So remove_hook_from_json is inconsistent with its sibling.

  2. The existing tests assert that foreign hooks must survivetest_remove_hook_from_json (line 5496) ends with:

    // Should have only one hook left
    assert_eq!(pre_tool_use.len(), 1);
    // Check it's the other hook
    assert_eq!(command, "/some/other/hook.sh");
    

    The intent is clearly "don't touch foreign hooks". The bug survives only because both existing tests (5496, 5531) put rtk in its own entry, where dropping the entry happens to be equivalent to dropping the hook. The mixed-entry case is untested.

Note this is easy to hit in practice: install_hook_into_json appends rtk as a new "matcher": "Bash" group, so a user who already has a Bash group ends up with two. Consolidating them by hand — which looks like harmless tidying — is what arms the bug.

Proposed fix

Strip rtk's own hooks first, then drop only the entries that were emptied:

    let mut removed_any = false;

    // Remove rtk's own hooks, so foreign hooks sharing an entry survive.
    for entry in pre_tool_use_array.iter_mut() {
        if let Some(hooks_array) = entry.get_mut("hooks").and_then(|h| h.as_array_mut()) {
            let before = hooks_array.len();
            hooks_array.retain(|hook| {
                !hook
                    .get("command")
                    .and_then(|c| c.as_str())
                    .is_some_and(|cmd| {
                        cmd.contains(REWRITE_HOOK_FILE) || cmd == CLAUDE_HOOK_COMMAND
                    })
            });
            if hooks_array.len() < before {
                removed_any = true;
            }
        }
    }

    // Drop only entries that rtk emptied out.
    pre_tool_use_array.retain(|entry| {
        entry
            .get("hooks")
            .and_then(|h| h.as_array())
            .map_or(true, |hooks| !hooks.is_empty())
    });

    removed_any

Returning removed_any rather than pre_tool_use_array.len() < original_len matters: when rtk's hook shared an entry, the array length is unchanged even though a hook was removed, and the old return value would report false.

The existing tests at 5496/5531 still pass — rtk's solo entry ends up empty and is dropped, so len() goes 2 → 1 as asserted.

Suggested regression test

#[test]
fn test_remove_hook_from_json_preserves_foreign_hook_in_shared_entry() {
    let mut json_content = serde_json::json!({
        "hooks": {
            "PreToolUse": [
                {
                    "matcher": "Bash",
                    "hooks": [
                        { "type": "command", "command": "/some/other/hook.sh" },
                        { "type": "command", "command": CLAUDE_HOOK_COMMAND }
                    ]
                }
            ]
        }
    });

    let removed = remove_hook_from_json(&mut json_content);
    assert!(removed);

    let pre_tool_use = json_content["hooks"]["PreToolUse"].as_array().unwrap();
    assert_eq!(pre_tool_use.len(), 1, "entry must survive: it still holds a foreign hook");

    let hooks = pre_tool_use[0]["hooks"].as_array().unwrap();
    assert_eq!(hooks.len(), 1, "only rtk's hook should be removed");
    assert_eq!(hooks[0]["command"].as_str().unwrap(), "/some/other/hook.sh");
}

Notes

The proposed patch is untested locally — I don't have a Rust toolchain on this machine, so I'm filing this as an issue with a diff rather than a drive-by PR. Happy to open one if you'd prefer.

Same shape may be worth a look in the Codex/Cursor uninstall paths (lines ~3199, ~3281), though those entries appear to carry command directly rather than a nested hooks array, so the bug likely doesn't apply there.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/hooks/init.rs at remove_hook_from_json and review the existing tests test_remove_hook_from_json around lines 5496 and 5531. Add the shared-entry regression test described in the issue, then run the hook initialization tests and verify that only rtk's hook is removed while the foreign hook and its matcher entry remain.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
cli, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.