feat(hooks): add direct Codex command rewriting - #3552
Conversation
|
|
|
Hi @aeppling @KuSh @TaKO8Ki, Codex has been gaining a lot of traction lately, and I think this integration could be really useful for its users. It lets RTK work transparently in Codex, so command output takes up less context without adding an extra model round trip. Whenever you have some bandwidth, I would really appreciate a review. I am happy to respond quickly and keep improving anything that needs work. Thanks! |
|
AFAIK this is already open in #2557, sadly not yet merged. |
Thanks! I checked #2557. There is an important behavioral difference between the two implementations. #2557 intentionally skips rewriting in default and ask modes, and only applies Our PR targets the current Codex hook semantics. The official Codex hooks documentation now explicitly specifies This means RTK can transparently rewrite the command in normal modes while still preserving Codex's native approval and sandbox flow. I also verified this end to end with Codex CLI So while the two PRs overlap in purpose, the behavior is meaningfully different. Happy to consolidate with #2557 if the maintainers prefer. |
|
Thanks! One thought after looking more closely at #2557 and the review discussion: Our PR may be better considered as an alternative implementation rather than necessarily a follow-up. It keeps RTK responsible only for I’m happy to follow whichever direction you prefer, whether that means reviewing Our PR directly or rebasing it on top of #2557 and incorporating the relevant parts. |
|
Replying to #3552 (comment) — On "alternative implementation vs follow-up": I think that framing is fair on the protocol point specifically. I traced Codex's own source ( That said, digging further turned up a real gap in that argument, plus some independent bugs — I'd want these addressed regardless of which PR becomes the reference implementation: 1. 2. Wrapping through 3. Local install has no working uninstall. 4. Two hook-detection parser regressions in On duplication ( Happy to help verify any of the above against a specific approach if useful. |
2d34e13 to
e85985e
Compare
|
Thanks for the detailed review. I rebased onto the latest
I added regression tests for each case. I kept the agent-specific orchestration separate as suggested. I did not include the broader low-level scaffolding extraction in this update, but I am happy to follow up with that if you would prefer it in this PR. Would appreciate another look when you have time. Happy to keep iterating. |
|
Rechecked against the updated branch — the four issues from the previous round are genuinely fixed, verified against source rather than just the diff:
Three minor things left, none of them merge blockers:
Nice work turning the previous round around — this is in good shape. |
e85985e to
cff625a
Compare
|
Thanks for checking this again. I rebased onto the latest
I also added regression coverage for missing and empty JSON files, path-aware parse and read errors, successful backups, backup failures, and preservation of the original file when a backup fails. Local verification is green: formatting, Clippy, 2,634 unit tests with 8 expected ignores, all 78 integration tests, the real Codex install and uninstall flow, and a workflow-equivalent Semgrep scan with zero findings. The new CI run is currently waiting for maintainer approval before GitHub will start any jobs. One unrelated note: the benchmark job on the current Happy to address anything else that comes up after the CI run is approved. |
|
Re-verified
One more pass at high effort surfaced one actionable item, posted inline below. Two other things it turned up are not asks for this PR:
|
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Requesting one small change (inline below) — the rest of the previous review round is resolved, see the follow-up comment.
| let quote = match command.as_bytes().first() { | ||
| Some(b'"') => Some('"'), | ||
| Some(b'\'') => Some('\''), | ||
| Some(_) => return command.split_whitespace().next(), | ||
| None => return None, | ||
| }?; |
There was a problem hiding this comment.
Two notes on raw_first_token, both "can't happen" rather than breakage — worth pinning down so a future reader doesn't have to re-derive them.
The single-quote branch (line 24) can never change the outcome. shell_split doesn't process escapes inside '...', so a single-quoted first token already survives intact through the primary path:
shell_split(r"'C:\Users\jane\rtk.exe' hook claude")
→ ["C:\\Users\\jane\\rtk.exe", "hook", "claude"]
If that token is an rtk path, is_rtk_binary(parsed_binary) matched and the || short-circuited before the fallback ran; if it isn't, the fallback returns the same string and still doesn't match. The backslash-eating this fallback exists to work around is confined to unquoted text and "...", so only the " branch and the unquoted split_whitespace() branch are load-bearing.
The ? on line 27 is a no-op. Every arm either yields Some(_) or returns early, so it can never short-circuit. Dropping the Option round-trip says the same thing:
let quote = match command.as_bytes().first() {
Some(b'"') => '"',
Some(b'\'') => '\'',
Some(_) => return command.split_whitespace().next(),
None => return None,
};Neither needs fixing for this PR to be correct. One thing explicitly not worth adding here, since it looks like a gap: quoted.find(quote) is blind to a backslash-escaped quote, but there's no input that reaches it with one — Windows forbids " in a path outright, and any POSIX string with a legitimately escaped quote is parsed correctly by shell_split, so the fallback is never consulted for it.
There was a problem hiding this comment.
Thanks, that makes sense. I will leave raw_first_token unchanged in this PR. The single-quote branch and the ? are non-behavioral, and #3704 is the right place to simplify this once the shared lexer consolidation lands.
cff625a to
e4b74fd
Compare
KuSh
left a comment
KuSh
left a comment
There was a problem hiding this comment.
Two real regressions from the read_json_file/backup_and_atomic_write refactor in 53fc1b61 (inline below), plus a handful of non-blocking cleanup notes on where the duplication this PR set out to reduce is still present.
| let serialized = serde_json::to_string_pretty(&root) | ||
| .context("Failed to serialize hooks.json")?; | ||
| atomic_write(&hooks_json_path, &serialized)?; | ||
| if let Some(mut root) = read_json_file(&hooks_json_path)? { |
There was a problem hiding this comment.
This is a behavioral regression from the read_json_file/backup_and_atomic_write refactor in 53fc1b61. Before that commit, a hooks.json parse error was silently swallowed here (if let Ok(mut root) = serde_json::from_str(...) { ... }), so remove_cursor_hooks always reported success. Now read_json_file(&hooks_json_path)? propagates the parse error as a hard Err. Step 1 above (deleting the Cursor hook script) has already run and committed to disk by the time this executes, so a corrupted hooks.json now leaves partial, inconsistent state (script gone, stale RTK entry still in hooks.json) and reports failure where it used to succeed cleanly. Could this step catch/log a parse error the way the old code did, rather than propagating it, so uninstall stays best-effort here?
There was a problem hiding this comment.
Fixed in e260715. remove_cursor_hooks_at() now catches only serde_json::Error, logs a warning, leaves the malformed hooks.json unchanged, and continues the best-effort cleanup. Read and permission errors still propagate normally.
I added test_remove_cursor_hooks_keeps_malformed_json_best_effort and test_remove_cursor_hooks_still_propagates_read_errors to cover both boundaries.
| let serialized = | ||
| serde_json::to_string_pretty(&root).context("Failed to serialize Droid hook file")?; | ||
| atomic_write(path, &serialized)?; | ||
| backup_and_atomic_write(path, &serialized)?; |
There was a problem hiding this comment.
This ? means a backup failure on this file aborts uninstall_droid_at's whole loop over droid_hook_file_candidates before the remaining candidate files are ever touched — even though test_remove_droid_hook_propagates_backup_failure_and_preserves_file covers the single-file fail-closed behavior, this loop-level consequence (one file's backup problem, e.g. a stray <file>.json.bak directory, blocking cleanup of unrelated files) isn't tested or documented. Worth collecting errors per-candidate in uninstall_droid_at and continuing the loop, surfacing a combined error/warning at the end, so an unrelated file's backup issue doesn't block cleanup of the others?
There was a problem hiding this comment.
Fixed in e260715. uninstall_droid_at() now attempts every candidate file, collects per-file failures, and returns a combined error after the loop.
The new regression test verifies that a backup failure leaves the affected file unchanged without preventing a later candidate from being cleaned and backed up.
| Ok(()) | ||
| } | ||
|
|
||
| fn codex_hook_already_present(root: &serde_json::Value) -> bool { |
There was a problem hiding this comment.
codex_hook_already_present/patch_codex_hooks_json/remove_codex_hook_from_json (this block, ~130 lines) reimplement the same read/check/patch/remove-hooks.json pattern already present for Claude (hook_already_present/insert_hook_entry near line 1147) and Cursor. This PR did unify the low-level file I/O via read_json_file/backup_and_atomic_write, but the higher-level presence-check/insert/remove logic is now a 3rd near-identical implementation — a future fix to that logic has to be found and applied in 3+ places. Not blocking, just flagging as a follow-up worth extracting at some point (could be a separate PR).
There was a problem hiding this comment.
Agreed. This is worth a separate follow-up. I kept this PR scoped to the Codex integration and the concrete behavioral regressions, so I did not expand this round into a higher-level hooks.json traversal refactor.
| println!("[--] Global RTK.md: not found"); | ||
| } | ||
|
|
||
| if global_hooks_json.exists() { |
There was a problem hiding this comment.
This global-hooks.json status block and its local counterpart further down are copy-pasted 16-line blocks differing only in the path variable and the "Global"/"Local" label. A print_codex_hook_status(label, path) helper would collapse both and avoid the wording/error-handling drifting apart if one copy gets updated and the other doesn't. Not blocking.
There was a problem hiding this comment.
Agreed. A print_codex_hook_status(label, path) helper would be a straightforward cleanup. I kept it as a follow-up since it is non-blocking and does not affect the current integration behavior.
| Ignore, | ||
| } | ||
|
|
||
| fn pre_tool_use_rewrite_output( |
There was a problem hiding this comment.
vscode_response_from_decision (further down, around the Copilot/VS Code handlers) still hand-rolls the hookSpecificOutput/updatedInput JSON that this new helper exists to consolidate. Since handle_vscode/vscode_response only thread a bare cmd: &str rather than the full payload, that path also can't preserve extra tool_input fields (timeout, description) the way this helper does for Codex/Claude/Droid — worth migrating it too, either in this PR or a quick follow-up, so the consolidation is complete.
There was a problem hiding this comment.
Agreed. A proper migration needs to thread the full payload through handle_vscode and vscode_response, rather than only replacing the envelope builder, so that extra tool_input fields are preserved.
I kept this out of the current PR and would cover it with dedicated field-preservation tests in a follow-up.
| } | ||
|
|
||
| /// Back up an existing JSON file before replacing it atomically. | ||
| fn backup_and_atomic_write(path: &Path, content: &str) -> Result<Option<PathBuf>> { |
There was a problem hiding this comment.
Claude's own settings.json installer, patch_settings_json_command (further down, ~line 1014), is the most-used path in this file but wasn't migrated to read_json_file/backup_and_atomic_write — Droid and Cursor were. That leaves two divergent failure-handling styles (that function's older ad hoc read/backup vs. the new hard-fail-on-backup-error everywhere else) between Claude's install path and everyone else's, undermining the consistency this refactor otherwise establishes. Worth a follow-up to bring it in line too.
There was a problem hiding this comment.
Agreed. The Claude installer should use the shared JSON I/O helpers as well. Since it is the most-used path, I would handle that in a focused follow-up with coverage for existing settings, backup failures, dry-run behavior, and idempotency. I left its current behavior unchanged in this PR.
|
Thanks for the detailed follow-up. I pushed
I added regression coverage for the malformed Cursor JSON path, Cursor I/O error propagation, and the Droid multi-candidate failure case. Local verification is green: formatting, Clippy, 2,675 unit tests with 8 expected ignores, all 79 integration tests, a workflow-equivalent Semgrep scan with zero findings, and a real project-scoped Droid install and uninstall round trip. I kept the non-blocking cleanup suggestions out of this change so this round stays focused on the two behavioral regressions. The new CI run is waiting for maintainer approval before GitHub will start the jobs. |
Thanks
Would you mind tackling these points in a follow-up PR? |
4 participants
Summary
PreToolUseintegration that transparently rewrites supported shell commands tortk ...throughupdatedInput.Implementation
rtk hook codexwith fail-open parsing and preservation of other tool input fields.rtk init --codexand update Codex guidance and integration docs.Benefits
Codex users get transparent RTK token savings without relying on prompt compliance or paying for another model inference, while retaining Codex's native approval and sandbox checks.
This integration is now mature because the required
PreToolUse.updatedInputcapability is available in Codex and has been verified end to end against Codex CLI0.147.0-alpha.6.6.I would appreciate review and merge if this direction aligns with the project.
Test plan
cargo fmt --all && cargo clippy --all-targets && cargo testls -laexecuted asrtk ls -laand returned compact outputRelated issues
Closes #1003
Closes #1812
Addresses #2921