Repository navigation
fix(hooks): recognize configured agent hooks in warning check (#913) - #4343
Aayushyaash wants to merge 4 commits into
Conversation
|
A quick fix for #913 would have been to add an OpenCode path check directly to 1. Keep configuration knowledge with the agentEach agent under If an agent changes its config format or install path later, its detection logic can be updated in the same module. 2. Avoid the hook masking issueThere was a previous attempt to solve this in This refactor keeps 3. Clean up old code while making the changeThere were also a couple of leftovers in
I removed these along with the related test code. This also removes the need for the 4. Make adding agents simplerWith this structure, adding support for another agent means implementing |
📊 Automated PR Analysis
SummaryFixes a false '[warn] No hook installed' message in Review Checklist
Linked issues: #913 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
…ules - Move agent configuration detection into corresponding init submodules with a common `is_configured() -> bool` interface. - Replace ad-hoc path checks with self-contained probes owned by each agent's initialization module for consistent hook detection. - Expose init submodules with `pub(crate)` visibility for use by hook-check logic.
828922d to
7e2279c
Compare
…#913) - Fix false `[warn] No hook installed` in `rtk gain` and command interception when a supported agent is configured (especially on Windows with binary hooks). - Use per-agent configuration probes to detect whether any supported agent has an active RTK hook or plugin. - Evaluate outdated-hook detection ahead of missing-hook suppression so upgrade warnings are not hidden. - Keep `hook_check::status()` tied to Claude's probe to preserve `rtk discover` transcript analysis fidelity. - Add integration test coverage for Antigravity, Cursor, OpenCode, and Gemini.
7e2279c to
9bc2f39
Compare
- Remove dead configured_agents() utility and its consistency test. - Remove duplicate binary_hook_registered() helper and test (superseded by claude::hook_status). - Remove obsolete other_integration_installed() test helper and its 7 unit tests. - Clean up unused constants and imports across hook_check. - Add explanatory doc comment on hook_check::status() documenting Claude delegation for rtk discover.
9bc2f39 to
fabc51e
Compare
Resolve merge conflict in src/hooks/init/gemini.rs by adopting #[cfg(all(test, feature = engine))], and adapt claude hook_already_present call to new signature.
3cd876f to
079bb3c
Compare
KuSh
left a comment
There was a problem hiding this comment.
Round 1 — CHANGES REQUESTED
Claim: silence the false No hook installed warning in rtk gain and in the daily check when another agent's RTK hook is configured, keep Claude's outdated warning first, and leave hook_check::status() (used by rtk discover and the init footer) unchanged.
Scope: accept if narrowed to "silence the missing-hook warning when another agent's RTK hook is registered, with Claude's outdated warning still first". The new Cursor "outdated" check is cut (blocker 2).
Ran: 66 home/workspace states, each run under the merge-base binary and under this head merged onto develop: rtk ls . (the daily check, which is what the hook's rewritten commands reach) and rtk gain, with stderr captured separately, plus rtk init --show. The states are 45 real rtk init installs and uninstalls from this head (--agent × 13, --opencode, --gemini, --codex, --copilot, global and project scope), 9 hand-built Claude settings shapes (legacy script v2/v4, registered/dangling/unreadable, absolute path, BOM, PostToolUse only), 6 Cursor legacy shapes (including the hooks.json that pre-binary installs wrote, and following the advice from each) and 6 controls. Invariants: every decision, split streams. Savings: n/a (no filter).
Pins: 34 decisions, 34 mutated, 21 survived the suite (listed in blockers 1–3); 4 equivalent mutants
Matrix: executed this round; pin check: executed this round
CI @079bb3c6e: all green (14 checks). Local gate on the merge with develop is green too, apart from one test that fails identically on the merge-base on this machine.
What works: on develop, an OpenCode plugin with a ~/.claude that has no hook warns on every path; on this head it is silent. Global installs of Gemini, Cursor, Trae, Antigravity, Pi, OMP, Hermes, Droid and Vibe, plus project-scope Trae, Antigravity, Pi, OMP and Hermes, go from No hook installed to silent. Every uninstall brings the warning back, and suppress_hook_warning still wins.
Blocks merge (3, frozen at round 1)
-
src/hooks/init/claude.rs:990—status()is no longer develop's function (one cause, four shapes). The removedbinary_hook_registeredaccepted onlyrtk hook claude(any rtk path) underPreToolUse.hook_already_presentalso accepts any command containingrtk-rewrite.sh, and the new code checksPostToolUsetoo:- settings.json points at
~/.claude/hooks/rtk-rewrite.sh, script deleted: developNo hook installed, head silent (a broken hook stops being reported) - settings.json points at the v4 script (the version
hooks/claude/rtk-rewrite.shships): develop silent, headHook outdatedevery day rtk hook claudeonly underPostToolUse: developNo hook installed, head silent- script exists but is unreadable: develop
Outdated, headMissing
The same value feeds
rtk discoverand thertk initfooter, which the PR says stay unchanged. A fix passes when these rows match develop, and the absolute-path case (/opt/homebrew/bin/rtk hook claude→ silent) is pinned again, since this PR deletes its test. Keeping develop's matcher insideclaude.rssatisfies all of them. - settings.json points at
-
src/hooks/init/cursor.rs:268— the Cursor "outdated" arm only fires on files nothing runs (cut by the narrowed claim).binary_registeredusescursor_hook_already_present, which also matchesrtk-rewrite.shcommands. On a real legacy install (hooks.json →./hooks/rtk-rewrite.shplus the script), the arm stays quiet. It fires only for a stray script that hooks.json does not reference. A Cursor-only user with no~/.claudethen getsHook outdated — run rtk init -g to updateon every command, once a day. Following that advice creates~/.claudeand installs the Claude hook. Making the arm fire on real legacy installs would not help either:rtk init -gdeletes the script and leaves hooks.json pointing at it (#3465, #3693). Please removecursor::is_outdated. Once it is gone,is_any_hook_outdated()equalsstatus() == Outdated, so it and its three call sites can go, andwarning_textbecomes a pure function again. -
src/hooks/hook_check.rs:91and the probes — unpinned behaviour (mutations that pass the whole suite). The mutations added an env-var guard.only_user_dirs_resolves_user_locationswent red on each one, but that test is a lint about env access, not these decisions.- Daily check, the
[rtk] /!\line that #913's "command interception" refers to:|| is_any_agent_configured()dropped fromsuppress_missingpasses. The only tests runrtk gain. Test must:rtk ls .with~/.claudeand an OpenCode plugin → stderr has noNo hook installed. The same command without the plugin must still show it. - Probes with no test at all, each replaced by
return false:codex,droid,trae,hermes,pi,vibe. Arms that also survive: Trae and Pi project scope (loop reduced to[true]), OMP extension, Hermes plugin file, Hermesconfig.yaml, the Gemini matcher (any matcher accepted), Gemini's script-file fallback, and Cursor's legacy-file arm. Test must: for each agent, the filesrtk init [-g] --agent <a>writes → silent, and the same files after--uninstall→ warning back. Each mutation above must turn one of them red. - Gemini's script-file fallback and Cursor's legacy-file arm count an unregistered file as configured, so a script whose settings entry is gone silences the warning. Dropping those two arms is simpler than pinning them.
- Daily check, the
Optional — will not hold merge
- Copilot (
rtk init -g --copilotand project scope), project-scope Codex (.codex/hooks.json) and project-scope Droid (.factory/hooks.json) still warn: their probes read only the global directory, or there is no probe. On develop they warn as well, so this is not a regression. If you would rather keep this PR to what it has, I will file it as an issue on approval. - The
claudeentry inAGENT_PROBESnever decides anything: it is true only whenstatus()isOk, and both callers run it only whenstatus()isMissing.claude::is_configured()is also true when~/.claudedoes not exist. Removing the entry is simpler than explaining it. test_agent_probes_covers_all_supported_agentscompares the list with a copy of itself.strip_leading_bombeforefrom_json_stris redundant: develop has no strip, and a BOM settings file reads the same on both sides.copilotbecamepub(crate)without a caller, and the comment under themodblock ininit/mod.rs("every other submodule is used only from here") is now stale.
Checked and correct — no need to re-verify
- The warning returns after
--uninstallfor every agent with an uninstall path (Cursor, Antigravity, Pi, Hermes, Droid, Vibe, Trae, Gemini, Codex). - Claude outdated still beats another configured agent (Antigravity + v2 script →
Hook outdated). hooks.suppress_hook_warningandRTK_SUPPRESS_HOOK_WARNING=1still silence Missing, and Outdated still shows under them.rtk init --showoutput is identical on both sides for all 66 states.- There is no measurable cost on the hooked commands:
rtk ls .takes 5.23 ms vs 5.45 ms with OpenCode only, and 5.25 ms vs 5.09 ms with Cursor. - Antigravity's hard-coded paths match what
run_antigravity_mode_atwrites, in both scopes.
Your questions
- None asked since the PR opened.
Next
Blockers 1–3: develop's Claude matcher, remove the Cursor outdated arm, and tests that kill the mutations in 3. After that, this merges. The Optional items are your call.
Threads: 3 open (inline, waiting on the author), 0 resolved.
| if let Ok(content) = fs::read_to_string(&settings_path) { | ||
| let content = strip_leading_bom(&content); | ||
| if let Ok(root) = from_json_str::<serde_json::Value>(content.trim()) | ||
| && [PRE_TOOL_USE_KEY, POST_TOOL_USE_KEY] |
There was a problem hiding this comment.
Blocker 1. hook_already_present is not the removed binary_hook_registered: it also accepts any command containing rtk-rewrite.sh, and this checks PostToolUse as well. Merge-base vs this head: a settings entry for a deleted ~/.claude/hooks/rtk-rewrite.sh goes from No hook installed to silent, a registered v4 script goes from silent to Hook outdated, a PostToolUse-only entry goes from No hook installed to silent, and an unreadable script goes from Outdated to Missing. rtk discover and the init footer read the same value. Keeping develop's PreToolUse + is_claude_hook_command matcher here makes all four rows match develop.
| } | ||
|
|
||
| /// Returns true if Cursor hook is installed via legacy script instead of binary command. | ||
| pub fn is_outdated() -> bool { |
There was a problem hiding this comment.
Blocker 2 (cut by the narrowed scope). cursor_hook_already_present matches rtk-rewrite.sh commands too, so on a real legacy install (hooks.json → ./hooks/rtk-rewrite.sh plus the script) this returns false. It is true only for a script nothing references. A Cursor-only user then gets Hook outdated — run rtk init -g daily, and following it installs the Claude hook. A version that fires on real legacy installs would lead into #3465/#3693. Please remove it. is_any_hook_outdated() then equals status() == Outdated and can go with it.
| let suppress_missing = | ||
| status == HookStatus::Missing && crate::core::config::hook_warning_suppressed(); | ||
| let suppress_missing = status == HookStatus::Missing | ||
| && (crate::core::config::hook_warning_suppressed() || is_any_agent_configured()); |
There was a problem hiding this comment.
Blocker 3 (unpinned). Dropping || is_any_agent_configured() here passes the whole suite: every new test runs rtk gain, none runs the daily check. Test must: rtk ls . with ~/.claude and an OpenCode plugin → stderr without No hook installed, and the same command without the plugin still shows it. The probe mutations in the review body need the same treatment.
Summary
Fixes #913
[warn] No hook installedmessage inrtk gainand command interception when any supported agent is configured, including OpenCode, Cursor, Antigravity, Gemini, and Codex.src/hooks/init/<agent>.rsusing a consistentis_configured() -> boolfunction instead of doing path checks directly inhook_check.rs.hook_check::status()tied to Claude Code sortk discovercontinues to use Claude's hook status.hook_check.rs.Why This Change Is Needed
hook_check.rsandrtk gainoriginally assumed Claude Code was the only configured agent. If Claude was not configured,status()returnedHookStatus::Missing, which caused[warn] No hook installedto be shown on every run.This became especially confusing on Windows after running
rtk init -g --opencode. The OpenCode plugin was active and saving tokens, butrtk gainstill reported that no hook was installed.How It Works
Per-Agent Detection: Each agent module under
src/hooks/init/<agent>.rsnow exposespub fn is_configured() -> bool. The agent module already knows where its configuration lives, such assettings.json,hooks.json, or its plugin directory, so the detection stays with the agent instead of being duplicated inhook_check.rs.Check All Supported Agents:
hook_check.rskeeps a list of the supported agent probes and checks whether any of them is configured.Outdated Hooks Take Priority:
is_any_hook_outdated()is checked beforeis_any_agent_configured()in bothcheck_and_warn()andgain.rs. For example, if OpenCode is configured but the Claude hook is outdated, the user still gets the upgrade warning.Keep
rtk discoverBehavior:hook_check::status()still checks Claude Code specifically.rtk discoveruses Claude's hook status when working with Claude session logs, so this keeps that behavior unchanged.Test Plan
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test --bin rtk hooks::(582 passed)tests/hook_detection_multi_agent_test.rs:rtk.ts), Cursor (hooks.json), Gemini (settings.json), or Antigravity is configured.rtk gainreports token savings without the false warning.