Repository navigation
refactor(hooks): consolidate hook config I/O - #4151
Merged
Merged
Conversation
Contributor
Author
📊 Automated PR Analysis
SummaryThis PR consolidates hook configuration I/O logic across Claude, Codex, Cursor and Trae, sharing presence-checking, insertion and removal code while keeping host-specific matching separate. It also standardizes JSON I/O handling for empty/BOM-prefixed configs and backup failures, and routes VS Code/Copilot rewrites through a shared function that preserves input fields like timeout and description. Review Checklist
Linked issues: #4002 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
Contributor
Author
|
@KuSh I’ve also built and tested a WorkBuddy hook on top of this refactor. I’m waiting for this PR to merge before opening that one so its diff stays focused on WorkBuddy. Happy to address any feedback here in the meantime! |
… test A hook group whose `matcher` is JSON `null` was counted as not covering the tool, so `rtk init` appended a second RTK group next to an existing one. Treat `null` like an omitted matcher, as develop did. The Copilot integration test ran `rtk hook copilot` from a temp dir whose project-root walk could reach a `.claude/settings.json` on the test machine, letting its permission rules change the response. Give the temp dir its own `.claude` so the walk stops there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
KuSh
approved these changes
Sep 24, 2026
KuSh
left a comment
KuSh
left a comment
Collaborator
There was a problem hiding this comment.
Round 1 of 3 — APPROVED
Claim: Claude, Codex, Cursor and Trae share one presence / insert / remove implementation (host matcher and ownership rules kept per host); Claude's settings.json and the Codex status move onto read_json_file / backup_and_atomic_write; VS Code/Copilot rewrites go through pre_tool_use_rewrite_output and keep the other tool_input fields. Closes #4002.
Scope: accept (round 1, frozen). Overlaps on src/hooks/init.rs with #3878 (split into per-agent submodules) and #4186 (symlinked backup destinations); whichever of this and #3878 lands second rebases, which is mechanical for this one.
Ran: base 727ee6e × head f6ef006 × head merged on develop 005eb9c, one target dir each (md5 differ). 19 rows: Claude install / reinstall / --show / uninstall on a settings file with user hooks, a user hook sharing RTK's group, 7 matcher forms, legacy rtk-rewrite.sh migration, BOM, empty, dry-run, backup slot as a directory, symlinked settings.json; Codex --show global+local × missing / empty / BOM / malformed / directory / unreadable, install / reinstall / uninstall with user hooks, inactive matcher; Cursor and Trae install / reinstall / uninstall over user entries incl. a prompt entry; rtk hook copilot × extra fields, Copilot-CLI Bash schema, not rewritable, BOM, CRLF, stdin /dev/null, deny rule, ask rule, camelCase schema. Invariants: every decision (allow / ask / deny / skip), user content survives outside the patched block, symlink target, idempotence. Savings: n/a (no filter).
Matrix: never → executed this round
CI @f6ef0067: all green; @37ec9595 (fixup): all green (fmt, clippy, test ubuntu/macos/windows, semgrep, security scan, benchmark, doc review, CLA)
Blocks merge (1, frozen at round 1; closed by fixup 37ec959)
src/hooks/init.rs:1342— a present-but-nullmatcher no longer counts as covering the tool (1 site;claude_group_covers_bashfalls through to it, Codex and Cursor call it directly). Evidence:{"matcher":null,"hooks":[{"type":"command","command":"rtk hook claude"}]}thenrtk init -g --auto-patch: develop →hook already present, 1 group; head →hook added, groups[null, "Bash"]. Closed by treatingnulllike an omitted matcher. Test:test_hook_presence_respects_host_matcher_forms(extended; fails with the arm removed).
Optional — will not hold merge
tests/hook_config_io_test.rs:9—copilot_cli_rewrite_preserves_vscode_tool_inputrunsrtk hook copilotfrom a temp dir whose project-root walk reaches the test machine's.claude/settings.json. With an ancestor allow rule forBash(git status)it fails at line 42 (permissionDecision: "allow"), and passes without it. That happens when TMPDIR sits under a directory holding.claude, as on Windows%TEMP%. Folded into the fixup: the temp dir gets its own.claude. The test name also sayscopilot_clibut exercises the VS Code schema.patch_settings_json_commandignores theOption<PathBuf>thatbackup_and_atomic_writereturns and re-checkssettings.json.bakwith.exists(). A stale.bakfrom an earlier install gets printed as this run's backup, and with-vit is printed twice. Same as develop; use the returned value once.
Follow-ups (outside this PR; reproduced on develop f5e104e, identical on this head)
- Gemini install/uninstall decide ownership by "first hook's command contains
rtk": uninstall deletes a user hook that shares RTK'sBeforeToolgroup, and a user hook such as/opt/smartkit/check.shis taken for RTK's (install skips the real one, uninstall deletes the user's). The shared helpers here are the natural fix; filed as #4227. - The startup banner (
hook_check::binary_hook_registered) still ignores matcher and hook type, so with RTK's hook undermatcher: "Read"or as apromptentry the banner stays silent whilertk init --shownow says "not configured". Same banner behaviour as develop; added to #4211, which is the same detector failing the other way. - A legacy
rtk-rewrite.shentry still counts as installed: with the script deleted by hand, or underCLAUDE_CONFIG_DIR,rtk init -greports "already present" and never registersrtk hook claude. Same as develop; added to #3465, the same shape for Cursor. - The remaining hand-rolled JSON readers next to the changed code (
show_claude_config, the Cursor status in--show,remove_hook_from_settings,remove_legacy_settings_entries, the Trae backup block) could move onto the same helpers later; not needed for this PR.
Checked and correct — no need to re-verify
- Claude matcher rule matches the Claude Code docs: letters, digits,
_ - space , |form an exact list; anything else is an unanchored regex.Read, Bash/Read|Bash/Bash/*/^Bacount as covering, whileReadandBado not and now get a workingBashgroup appended (develop said "already present" and left the hook inert). - Uninstall with RTK's hook hand-merged into the user's Bash group: develop deleted the whole group along with
echo user-bash, while head keeps the user hook. That is a data-loss fix on develop's behaviour. - Legacy migration with a mixed group: develop left
rtk-rewrite.shin place and reported "already present" (sortk hook claudewas never registered); head removes only the legacy hook, keeps the user's, and registers the new one. - Codex
--show: empty → "exists but RTK hook is not configured" (was "invalid JSON"); BOM →[ok](was "invalid JSON"); malformed →[!!] invalid JSON; directory / unreadable → error exit, path-aware message. Global and local identical. settings.jsoninstall: BOM tolerated, empty file →{}, dry-run byte-identical,.baktaken, backup failure (slot is a directory) leaves the original intact and exits 1, symlinkedsettings.jsonstays a symlink and the target is updated.- Cursor and Trae: install / reinstall idempotent, uninstall preserves user and
promptentries and unrelated events; a Cursor RTK entry undermatcher: "Read"is no longer counted as installed. - Copilot:
explanation,isBackground,timeout,descriptionpreserved on allow and ask; nopermissionDecisionon ask; deny and non-rewritable emit nothing, exit 0; BOM, CRLF,/dev/nulland camelCase unchanged vs develop. - Mutation: forcing
is_command_hookto ignoretypefails 4 tests, pruning every touched group fails 7, and a Claude matcher that always covers failstest_hook_presence_respects_host_matcher_forms. - Fixup 37ec959 merged onto develop f5e104e: clean; fmt / clippy (0 warnings) / 3935 tests pass. Fixup matrix identical to f6ef006 on every row; its before/after was rerun independently.
Hypotheses built and dropped
- "Codex/Cursor matchers treated as an unanchored regex report a non-firing group as installed": develop ignored the matcher entirely for both, so every case the regex accepts was already "installed" on develop. The new check only narrows it; no input goes from correct to wrong.
rtk init --showfor Claude and Cursor with BOM files:[ok]on head, same as develop.- Droid counting a
prompt-type entry that carriescommand: "rtk hook droid"as installed: reproduces, but prompt hooks carryprompt, notcommand, so no real config reaches it. Gemini writing through a symlinkedsettings.json: the symlink survives.
Your questions
- "@KuSh … Would appreciate a review when you have time. Happy to make any adjustments!" → this is round 1.
- "I've also built and tested a WorkBuddy hook on top of this refactor. I'm waiting for this PR to merge before opening that one…" → fine; nothing here changes the helpers' shape, so building on
hook_present/remove_hook_entriesis safe.
Next
fixup 37ec959 pushed: a null matcher counts as match-all again, and the Copilot integration test gets its own .claude so the test machine's permission rules cannot decide it. Nothing else from you — approving.
Rounds: 1/3. Threads: 0 open, 0 resolved this round.
Merged
Contributor
Author
|
Thanks @KuSh for the thorough review and the fixes! I have picked them up by rebasing the WorkBuddy branch onto develop and am preparing its separate PR. |
4 tasks done
KuSh
added a commit
to jpds/rtk
that referenced
this pull request
Sep 25, 2026
…ew modules develop changed src/hooks/init.rs after this branch's base (rtk-ai#4151: consolidate hook config I/O, and the null-matcher follow-up). Resolving the modify/delete conflict by keeping the split alone would drop those changes: the merged tree builds and its test suite passes, yet `rtk init -g --uninstall` removes a user's own hook from a group it shares with RTK's, where develop prunes only the RTK entry. Carry each of develop's changes to the file its item now lives in: the sixteen rewritten helpers in mod.rs, codex.rs, cursor.rs and trae.rs; the shared hook-entry helpers (is_command_hook, group_covers_tool, claude_group_covers_bash, HookEntries, hook_present, append_hook_entry, remove_hook_entries) after clean_double_blanks in mod.rs; is_cursor_hook_entry in cursor.rs; print_codex_hook_status beside its only caller in codex.rs; and the six new tests after test_global_default_mode_creates_artifacts in mod.rs. Restore the 26 `///` doc comments the split dropped -- read_json_file, patch_claude_md, patch_agents_md and 23 Codex tests -- verbatim from develop, and move the `upsert_rtk_block tests` banner to agents_md.rs, where that test now lives. Narrow the module's visibility to what is used. The split had re-exported eight submodules with `pub(crate) use x::*` and marked 154 moved items `pub(crate)`, although nothing outside src/hooks/init/ uses any of them except copilot_user_dir and COPILOT_HOOK_JSON (hook_cmd.rs). Submodules see mod.rs's private items and its private glob imports through `use super::*`, so the globs become private, the helpers shared between submodules become pub(super), and the rest return to private. Four functions init.rs had exported -- run_omp_mode, uninstall_omp, uninstall_omp_with_patch_mode and run_pi_mode -- were left `pub` inside private modules with no re-export; in a binary crate a re-export nobody calls is an unused import, so the three that only tests (or nothing) call are private with their `allow(dead_code)` comment rewritten to say so, and the one mod.rs calls is pub(super). Put code where its callers are. print_instructions_agents_awareness_note is named for instructions_agents.rs and called only from there; GEMINI_MD is used only by gemini.rs; read_json_file, backup_path_for and backup_and_atomic_write had drifted from beside atomic_write to the bottom of mod.rs; is_codex_hook_command and is_trae_hook_command each have a single consumer, so they leave hooks/mod.rs for codex.rs and trae.rs with their four tests (is_claude_hook_command stays: four modules use it). run_pi_mode and run_omp_mode are called only by their files' tests, so they live in those test modules; uninstall_omp has had no caller since OMP uninstall started going through uninstall_omp_with_patch_mode, and is gone. The one test that exercised cursor.rs helpers from mod.rs moves to cursor.rs, which lets insert_cursor_hook_entry and remove_cursor_hook_from_json be private again. The remaining pub(super) items are entry points the mod.rs dispatcher calls, the shared block helpers in agents_md.rs, the OpenCode plugin helpers the Claude flow installs alongside its own hook, and eight helpers pinned by three tests that deliberately exercise several agents' hook registration side by side. Pi and Oh My Pi share one extension file: OMP loads Pi's `rtk.ts` through its legacy-pi-compat layer, and 170 of omp.rs's 253 lines were calls into the Pi machinery that lived in opencode.rs beside the unrelated OpenCode plugin. pi.rs now holds both agents and the shared install, uninstall, stock-content and ownership-tracking code; opencode.rs keeps the OpenCode plugin alone. The Claude settings.json flow -- run_default_mode, patch_settings_json_command and the migration helpers, some 490 lines and 26 tests -- moves from mod.rs to claude.rs, which until now held only the CLAUDE.md patching its header did not describe. canonicalize_path_for_comparison is a filesystem helper codex.rs depends on, so it joins read_json_file in mod.rs; the two Pi/OMP test fixtures follow their tests into pi.rs. Each file imports the constants it uses instead of receiving all fifty through mod.rs, and mod.rs glob-imports only the two modules whose helpers siblings share. Three copies of the same logic, all older than the split, collapse onto the helpers that already existed. Four of the five "config directory from an env override, else under home" resolvers become calls to resolve_config_dir (Droid's stays: its override replaces the home directory itself, and now says so). remove_hook_from_settings re-implemented read_json_file and backup_and_atomic_write inline; it and the six other per-agent "serialise, print under --dry-run, back up, write atomically, report under -v" envelopes in claude.rs, codex.rs, cursor.rs and droid.rs go through update_json_file, which takes each site's messages as arguments so every line they print stays the same (Trae's all-or-nothing batch is left as it is). One behaviour changes on purpose: copilot_user_dir treated an empty COPILOT_HOME as a path and wrote Copilot's files relative to the working directory; like the other agents it now ignores an empty override and falls back to home. instructions_agents.rs opened with `///` above `use super::*;`, which documents the import; make it the `//!` module header the other files use. The last three `// --- section ---` banners go the way of the ones the split already removed, and remove_rtk_block's doc no longer names CLAUDE.md -- it strips the block from any instructions file. Apart from the deletion, the moves and the thirteen de-duplicated functions named above, every top-level item in develop's init.rs is byte-identical to its copy in src/hooks/init/, doc comments included, modulo visibility, rustfmt layout, the include_str! paths one directory deeper, and the split's rename of print_rules_only_awareness_note to print_instructions_agents_awareness_note; develop's 297 tests all match by name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #3552. Closes #4002.
pre_tool_use_rewrite_output, preservingtimeout,descriptionand other input fields without changing permission decisions.Rebased onto
develop(727ee6e6) before implementation. This finishes the consolidation requested in #4002 and fixes the remaining field-preservation gap.Test plan
cargo fmt --all --check,cargo clippy --all-targets,cargo test,git diff --check