Hook config I/O: finish the consolidation started in #3552 (4 leftover call sites)
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 81.1k
- Forks
- 5.1k
- Avg merge
- 4d 21h
- Merged PRs (30d)
- 35
Description
Four related cleanups surfaced while reviewing #3552 (Codex hook integration). Each is non-blocking on its own — filing them together so they don't get lost, as agreed with @yeahjack, who offered to take them in a focused follow-up PR.
All four are the same shape: #3552 successfully unified the low-level hook-config file I/O behind read_json_file / backup_and_atomic_write, but several call sites and the layer above them were left on their own implementations.
1. Per-agent hooks.json patch/presence/remove keep getting reimplemented
codex_hook_already_present / patch_codex_hooks_json / remove_codex_hook_from_json in src/hooks/init.rs reimplement the read → check-presence → patch → remove pattern that already exists for Claude (hook_already_present / insert_hook_entry) and Cursor. #3008 adds Trae as a fourth — read_trae_hooks_json / trae_hook_already_present / patch_trae_hooks_json_paths / remove_trae_hook_from_json — hand-rolling read_json_file (14 other callers) and backup_and_atomic_write (10 other callers) along with it. A fix to the presence-check or insert/remove logic now has to be found and applied in four places.
Trae is worth recording in detail, because two of the defects its review turned up are ones the shared helpers already prevent:
read_json_file strips a leading BOM; the hand-rolled reader did not. Given the same UTF-8-BOM'd hooks.json:
rtk init -g --agent cursor -> exit 0, installed
rtk init -g --agent trae -> exit 1, "Failed to parse Trae hooks file ... as JSON"
Install and uninstall both failed, and a BOM'd PreToolUse payload made the hook skip the rewrite entirely. The fix was routing the two readers through read_json_file.
A private "is this registration ours" rule drifted, in both directions. It first required matcher == "RunCommand" and timeout == 30, so a user who had edited either got RTK Trae support was not installed (nothing to remove) while the hook stayed registered and live, and a reinstall appended a duplicate. Matching on the command alone fixed that and went too far the other way: a registration under a matcher excluding RunCommand then counted as installed (so install skipped adding one that would fire), and a user-authored type: "prompt" entry was deleted along with its group. Each end cost a separate review round to find.
Neither could have happened behind a shared presence/patch/remove implementation, which is the argument for this item. Trae's rule after the fixes (trae_group_covers_run_command + is_trae_hook_entry) is the most worked-out of the four and is probably the right shape to lift into the shared one, rather than a fourth thing to migrate onto it.
2. show_codex_config's global and local blocks are copy-pasted
The global-hooks.json and local-hooks.json status blocks differ only in the path variable and the Global/Local label. A print_codex_hook_status(label, path) helper collapses both and stops the wording and error handling drifting apart.
These two blocks also still parse with raw fs::read_to_string + from_json_str rather than read_json_file, so an existing-but-empty hooks.json is reported as [!!] ... is invalid JSON, while every other Codex path in the same file treats an empty file as a benign {}.
3. vscode_response_from_decision doesn't use pre_tool_use_rewrite_output
src/hooks/hook_cmd.rs — #3552 added pre_tool_use_rewrite_output and migrated the Claude, Codex and Droid paths onto it. The VS Code / Copilot path still hand-rolls the same hookSpecificOutput / updatedInput envelope. Because handle_vscode / vscode_response only thread a bare cmd: &str rather than the full payload, that path also cannot preserve extra tool_input fields (timeout, description) the way the shared helper does — so this is a small behaviour gap, not only duplication. Worth field-preservation coverage when it moves.
4. patch_settings_json_command isn't on the shared JSON I/O helpers
src/hooks/init.rs — Claude's own settings.json installer is the most-used path in the file, but Droid and Cursor were migrated to read_json_file / backup_and_atomic_write and it wasn't. That leaves two different failure-handling styles side by side (its older ad hoc read/backup vs. the newer propagate-on-backup-failure), which is the inconsistency the refactor was meant to remove. Worth coverage for existing settings, backup failure, dry-run and idempotency when it moves.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/hooks/init.rs and src/hooks/hook_cmd.rs, tracing the named per-agent hook operations, show_codex_config, vscode_response_from_decision, and patch_settings_json_command. Compare these paths with read_json_file, backup_and_atomic_write, and pre_tool_use_rewrite_output before running the relevant hook coverage. Done means the four call-site groups share the intended helpers, preserve required fields and matching behavior, and cover the noted error, dry-run, backup, and idempotency cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cli, tooling
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100