[tayfuryldz]

Fixes #4296.

Recognize the legacy RTK Codex hook forms that older setups left behind: rtk hook claude and rtk-rewrite.sh. This lets init detect them instead of adding a second RTK hook, and lets uninstall remove them through the same existing entry-removal path.

Adds coverage for both legacy forms.

Validation: cargo test --locked codex_hook_command_ --bin rtk; git diff --check.

[CLAassistant]

All committers have signed the CLA.

[rtk-wshm-sync-bot]

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type	bug-fix
🟢 Risk	low

Summary

Updates is_codex_hook_command in src/hooks/init/codex.rs to also recognize legacy RTK Codex hook forms ("rtk hook claude" and rtk-rewrite.sh) left behind by older setups. This lets init detect these legacy hooks instead of duplicating them, and lets uninstall remove them via the existing entry-removal path.

Review Checklist

• Tests present
• Breaking change
• Docs updated

Linked issues: #4296

---

Analyzed automatically by wshm · This is an automated analysis, not a human review.

[yeahjack]

Tested 1df6391b against develop 45b0b4c3 using the actual CLI with isolated configuration. I found two issues that need addressing before this resolves #4296.

1. Recognizing a legacy entry prevents its migration

Starting with this .codex/hooks.json:

{
  "hooks": {
    "PreToolUse": [{
      "matcher": "Bash",
      "hooks": [{"type": "command", "command": "rtk hook claude"}]
    }]
  }
}

rtk init --codex reports already present, but leaves the legacy command unchanged and does not add rtk hook codex. Repeating init has the same result. When both legacy and native entries already exist, init leaves both in place. A legacy script registration behaves the same way.

codex_hook_already_present uses the expanded predicate, so patch_codex_hooks_json returns before performing any migration. Please distinguish a valid native registration from an owned legacy entry, clean up the latter, and ensure a native Codex registration exists. Add installer-level regression tests for legacy-only and mixed configurations; predicate tests alone do not cover this path.

2. The script substring match can delete unrelated user hooks

With command.contains("rtk-rewrite.sh"), rtk init --codex --uninstall deletes command hooks such as:

echo rtk-rewrite.sh
/custom/rtk-rewrite.sh.backup
python3 /custom/audit.py --label rtk-rewrite.sh

I reproduced all three. Init also treats these entries as an existing RTK installation and skips adding the native hook.

Please restrict legacy recognition to supported, parsed invocations whose command/path establishes RTK ownership, and preserve ambiguous entries. These examples should be negative tests for both installation detection and uninstall. Ordinary user hooks sharing the group and prompt-type entries were preserved in the fixtures I tested.

[tayfuryldz]

Addressed both review findings. Codex installation now distinguishes a native rtk hook codex registration from RTK-owned legacy registrations, removes owned legacy entries before inserting the native hook, and no longer treats arbitrary commands that merely mention rtk-rewrite.sh as owned. Added negative coverage for the three ambiguous examples from review. Focused command-recognition and installer idempotency tests pass; cargo fmt --check and git diff --check are clean.

[tayfuryldz]

Both requested installer-level cases are addressed on the current head: native Codex registration is distinguished from RTK-owned legacy registrations so legacy-only and mixed configurations migrate to one native hook, and legacy script recognition no longer uses an arbitrary substring match. The unrelated command examples from review are covered as negative cases so install detection/uninstall preserve them. Current checks are green.

[tayfuryldz]

Rebased onto current develop and reran the Codex-focused Rust tests after the rebase. The filtered run is green, including the installer/migration coverage, and git diff --check is clean.

[yeahjack]

@tayfuryldz Thanks for the rebase. One mixed-configuration case remains on c6059a0: patch_codex_hooks_json still returns early when a native hook is present, before removing legacy registrations. This leaves the legacy entries in place when native and legacy hooks coexist.

#4352 contains a supplemental fix and installer regression tests in commit 16c5c400. Could you consider incorporating that additional commit into this PR? I'll keep #4352 open until the fix is incorporated.

[tayfuryldz]

Incorporated the mixed-registration fix and installer regression coverage from #4352. Native + legacy configurations now remove the owned legacy entry without replacing the existing native hook. The Codex init test group is 54/54, Clippy with -D warnings passes, cargo fmt --check passes, and git diff --check is clean.

[yeahjack]

@tayfuryldz Thanks for incorporating the fix and regression coverage. I checked 0c94e43, and it addresses the mixed-registration issue I reported.

[KuSh]

Round 1 of 3 — CHANGES REQUESTED

Claim: rtk init --codex replaces a legacy RTK Codex hook (rtk hook claude, or the rtk-rewrite.sh script) with rtk hook codex, and --uninstall removes it, leaving the user's own hooks alone.
Scope: accept (round 1, frozen). This lands before #4269, which also rewrites patch_codex_hooks_json and will rebase onto it. It composes with #4265: both arms go through is_rtk_hook_command.
Ran: init --codex, --show, init again, --uninstall --dry-run and --uninstall, on merge-base ff9bd8e and on 0c94e43, against 24 .codex/hooks.json fixtures: fresh, native only, 7 legacy spellings, 3 mixed layouts, foreign only, 5 chained or argument forms, prompt type, non-Bash group, shared group, symlinked .codex. Plus -g with a redirected HOME, and the legacy predicate diffed against bash's own parse (declare -f) over 2,431 generated commands. Invariants: non-default targets (-g, non-Bash matcher, split groups), every decision, foreign content. No savings figure applies: this is not a filter.
Pins: 14 decisions, 14 mutated, 3 survived (the claude literal in the legacy arm; presence check counting legacy entries; uninstall falling back to the native-only predicate)
Matrix: never — running now; pin check: never — running now
CI @0c94e43a: all green (11/11)

Blocks merge (3, frozen at round 1)

1. src/hooks/init/codex.rs:990 — legacy script matched on its first word only (1 site, and it must change). rtk init --codex now deletes any hook entry that starts with an rtk-rewrite.sh path, together with whatever the user chained after it. Develop leaves these entries alone, and the rtk hook claude arm of the same function already requires the exact argv. Evidence, each with one entry in .codex/hooks.json:

   • /home/u/.claude/hooks/rtk-rewrite.sh && logger -t audit done: develop keeps it and adds rtk hook codex; 0c94e43 deletes it on init.
   • /home/u/.claude/hooks/rtk-rewrite.sh | tee -a /tmp/audit.log: same.
   • /home/u/bin/rtk-rewrite.sh --my-flag: same.

   Sweep: of 2,431 generated commands, 90 are claimed without being the script on its own. All 90 are the 9 path spellings × 10 suffixes (an argument, &&, ||, |, ;, &, a redirect, a second line), and 63 of them drop a second command. Every prefix form (echo, bash, env, python3 … --label) and every lookalike name is rejected correctly.
   Test must: assert is_codex_hook_command is false on …/rtk-rewrite.sh && logger -t audit done, … | tee -a /tmp/audit.log, … followed by \nmy-audit, and … --flag. The installer-level migration test must keep a …/rtk-rewrite.sh && logger -t audit done entry through init. Putting back the first-word match must turn both red. After the fix: 0 claimed on the sweep, the three entries above survive init and uninstall, and the legacy rows still migrate.
   The same predicate has an unpinned decision: mutation is_rtk_hook_command(command, "claude") → is_rtk_hook_command(command, "claude") || is_rtk_hook_command(command, "cursor") passes 310/310. A negative row for rtk hook cursor kills it.

2. src/hooks/init/codex.rs:911 — unpinned behaviour (the presence check counts only the native hook). Evidence: mutation is_native_codex_hook_command → is_codex_hook_command in codex_hook_already_present passes 310/310. On install the mutation changes nothing, because legacy entries are removed first. Its other reader is --show: on a legacy-only hooks.json it prints [--] Local hooks.json exists but RTK hook is not configured, as develop does, and the mutation would make it print [ok]. Test must: codex_hook_already_present is false on a root whose Bash group holds only rtk hook claude and …/rtk-rewrite.sh.

3. src/hooks/init/codex.rs:1001 — unpinned behaviour (uninstall removes legacy entries, the second half of #4296). Evidence: mutation is_command_hook(hook, is_codex_hook_command) → is_command_hook(hook, is_native_codex_hook_command) in remove_codex_hook_from_json passes 310/310. The predicate test checks is_codex_hook_command, which uninstall then stops calling. The behaviour itself is correct on 0c94e43: every legacy row ends with no RTK entry after --uninstall, and user hooks in the same group are kept. Test must: remove_codex_hook_from_json on a Bash group holding rtk hook claude, …/rtk-rewrite.sh and my-guard returns true and leaves exactly my-guard.

Optional — will not hold merge

• When init only removes a legacy entry next to an existing native hook, the summary says Hook: … (registered), but the hook was already there. Uninstall likewise reports hooks.json: removed rtk hook codex entry when it removed only a legacy entry.
• crate::hooks::is_rtk_hook_command(command, "claude") is crate::hooks::is_claude_hook_command, and "rtk-rewrite.sh" is crate::hooks::constants::REWRITE_HOOK_FILE.

Filed as follow-ups

• #4427 rtk init claims any Claude or Cursor hook command that mentions rtk-rewrite.sh — outside this PR's hunks: the substring match yeahjack flagged here also exists in claude.rs and cursor.rs, where it blocks rtk init -g and deletes foreign hooks on uninstall; filed.

Checked and correct — no need to re-verify

• Both findings from yeahjack's review of 1df6391 are fixed. Legacy-only and mixed layouts (same group, separate groups, -g) end with exactly one rtk hook codex, and an existing native entry is kept as is (its timeout field included). echo rtk-rewrite.sh, /custom/rtk-rewrite.sh.backup, python3 /custom/audit.py --label rtk-rewrite.sh and echo rtk hook claude come out of init and uninstall exactly as on develop.
• A second init is a no-op (file and .bak unchanged). --dry-run writes nothing, and the uninstall dry-run reports without removing.
• User hooks that share a group with a legacy entry survive. A prompt-type rtk hook claude is kept. A legacy entry in a non-Bash group is removed, and a Bash native hook is added.
• A symlinked .codex that points outside the project is refused with exit 1 and nothing written (existing guard).
• bash …/rtk-rewrite.sh, a Windows-path script and rtk hook claude # comment are not claimed: they stay in place, which is the safe miss.
• Merged onto develop 6f5503d: clean merge; fmt and clippy clean (0 warnings); cargo test --all passes 4,101 tests.

Your questions

• No questions from you since the PR opened. yeahjack's request to fold in 16c5c40 is done, and #4352 is closed as a duplicate of this PR.

Next

Match the rtk-rewrite.sh arm only when the script runs on its own (one word, the shape is_rtk_hook_command already requires), and add the negative, presence and uninstall rows above. When you push, please also add Co-authored-by: Yijie Xu <48432972+yeahjack@users.noreply.github.com> to the mixed-registration commit: its fix and tests come from 16c5c40. After that push, this approves.
Rounds: 1/3. Threads: 0 open (waiting on the three items above), 0 resolved this round.

[tayfuryldz]

Addressed the three round-one blockers on current develop. Legacy rtk-rewrite.sh ownership now requires the script to be the only argv entry, with negative coverage for chained, piped, newline, argument, and rtk hook cursor forms. Added direct coverage that legacy-only configuration does not count as native presence, and that uninstall removes both legacy forms while preserving the user hook. The installer regression also keeps a chained script entry intact. Codex-filtered tests are 70/70, Clippy with -D warnings passes, fmt and diff-check are clean. I also added the requested Yijie Xu co-author trailer to the mixed-registration commit.

[yeahjack]

@KuSh When you have a chance, could you revisit the updates here and in #4308? Thank you!

[KuSh]

Round 2 of 3 — CHANGES REQUESTED

Claim: rtk init --codex replaces a legacy RTK Codex hook (rtk hook claude, or the rtk-rewrite.sh script) with rtk hook codex, and --uninstall removes it, leaving the user's own hooks alone.
Scope: accept (round 1, frozen).
Ran: init --codex, --show before and after, init again, --uninstall --dry-run and --uninstall, on 0c94e43 (round 1's head) and 2a8f439, against 25 .codex/hooks.json fixtures, plus -g with a redirected HOME and a symlinked .codex. The legacy predicate was diffed against bash's own parse (declare -f) over 2,431 generated commands, then over 1,188 more with an operator glued to the RTK invocation. Develop 16a20a2 was run on the glued rows. Invariants: non-default targets (-g, non-Bash matcher, split groups), every decision, foreign content. No savings figure applies: this is not a filter.
Pins: 13 decisions, 13 mutated, 0 survived
Matrix: executed this round (round 1 executed @0c94e43a); pin check: executed this round
CI @2a8f4390: all green (11/11). The branch now conflicts with develop (see Next).

Blocks merge (round 1's three are closed; 1 reopened below)

Round 1's blockers 2 and 3 are closed: codex_hook_presence_ignores_legacy_only_configuration and remove_codex_hook_removes_legacy_entries_and_preserves_user_hook each fail under their mutation. Blocker 1's stated rows pass: 0 wrong claims on the 2,431-command sweep, and the &&, |, ;, newline and --flag entries survive. The co-author trailer is in.

1. src/hooks/init/codex.rs:994 and :928 — an entry that runs RTK but is not a plain legacy line is either deleted or doubled. Round 1 asked for such entries to be kept with rtk hook codex added next to them. That ask was mine and it was wrong: both hooks then rewrite every command, which is the #4296 symptom the PR is fixing. Only a plain legacy line should be migrated. Any other entry that still runs RTK's hook should be kept, rtk hook codex should not be added, and init should say so.

   • Deleted. shell_split joins the words an operator touches, so my-audit&&/home/u/.claude/hooks/rtk-rewrite.sh comes back as one word ending in the script, and my-audit;/usr/local/bin/rtk hook claude passes the rtk hook claude check. rtk init --codex deletes both, my-audit included; develop keeps them. On 1,188 such glued commands, 63 are claimed. crate::discover::lexer::tokenize (the lex shell_split uses) marks the operator as a non-Arg token, which tells them apart. This is my miss as well: round 1's sweep never glued an operator to the RTK part.
   • Doubled. …/rtk-rewrite.sh && logger -t audit done, … | tee -a /tmp/audit.log and bash /home/u/.claude/hooks/rtk-rewrite.sh are kept, and rtk hook codex is added next to them.

   Rows (.codex/hooks.json entry → rtk init --codex):

   Entry	2a8f439	Required
   rtk hook claude, /home/u/.claude/hooks/rtk-rewrite.sh, ~/.claude/hooks/rtk-rewrite.sh, "/home/u/My Hooks/rtk-rewrite.sh", $HOME/.claude/hooks/rtk-rewrite.sh	replaced by rtk hook codex	unchanged
   /home/u/.claude/hooks/rtk-rewrite.sh && logger -t audit done	kept + rtk hook codex added	kept, rtk hook codex not added, a warning naming the entry, exit 0
   /home/u/.claude/hooks/rtk-rewrite.sh | tee -a /tmp/audit.log	kept + added	same as above
   bash /home/u/.claude/hooks/rtk-rewrite.sh	kept + added	same as above
   my-audit&&/home/u/.claude/hooks/rtk-rewrite.sh	deleted, rtk hook codex added	same as above
   my-audit;/usr/local/bin/rtk hook claude	deleted, rtk hook codex added	same as above
   echo rtk-rewrite.sh, /custom/rtk-rewrite.sh.backup, python3 /custom/audit.py --label rtk-rewrite.sh	kept + added	unchanged: these do not run the hook, no warning
   /opt/bin/rtk hook codex plus one of the warned entries	both kept	both kept, with the warning

   The summary line must not say registered when nothing was added. --uninstall keeps the warned entries and repeats the warning. Test must: for each warned row, assert the file after init (the entry kept, no rtk hook codex added) and that the warning names the entry; for each glued row, assert is_codex_hook_command is false. Adding the native hook back on that path, or dropping the operator check, must turn a test red.

Optional

• When init only removes a legacy entry next to an existing rtk hook codex, it prints Hook: … (registered). Uninstall prints removed rtk hook codex entry when it removed only a legacy entry. You are touching the summary anyway.
• crate::hooks::is_claude_hook_command and crate::hooks::constants::REWRITE_HOOK_FILE already exist for is_rtk_hook_command(command, "claude") and "rtk-rewrite.sh".

Filed as follow-ups

• #4617 rtk init claims a user's hook command when an operator glues it to rtk hook <agent> — pre-existing in the shared is_rtk_hook_command (my-audit;/usr/local/bin/rtk hook codex and the Claude equivalent on develop); filed.

Checked and correct — no need to re-verify

• Legacy-only and mixed layouts (same group, separate groups, -g) end with exactly one rtk hook codex, and an existing native entry is kept as is. rtk hook cursor, prompt-type entries and user hooks sharing a group are left alone.
• --show reports a legacy-only file as not configured, as develop does. A second init is a no-op, --dry-run writes nothing, and a symlinked .codex is refused with exit 1.

Next

1. Rebase onto develop. There is one conflict: develop's 5d8102a changed insert_hook_entry to (root, event, matcher, command). Keep your else-branch insert in that form (PRE_TOOL_USE_KEY, CLAUDE_PRE_MATCHER, CODEX_HOOK_COMMAND) and drop develop's unconditional insert.
2. Migrate only plain legacy lines. Keep and warn on any other entry that still runs RTK, without adding rtk hook codex, per the rows above.

After that push, this approves.
Rounds: 2/3. Threads: 1 open (above); round 1's three resolved (two fixed by 2a8f439, the third superseded by this one).

[KuSh]

Blocks merge (1): an entry that runs RTK but is not a plain legacy line is either deleted here (my-audit&&/home/u/.claude/hooks/rtk-rewrite.sh and my-audit;/usr/local/bin/rtk hook claude come back from shell_split as RTK's) or kept with rtk hook codex added at :928, so two hooks rewrite every command. Migrate plain lines only; keep and warn on the rest without adding the native hook. Rows in the review body.