feat(hook): PreToolUse redirect classifier, ported from lean-ctx - #170
Merged
Conversation
New src/hook_redirect.rs: fail-open timeout wrapper (decide_with_timeout) and a classify() step targeting agentflare-backend's own tools instead of lean-ctx's compression handlers, which don't apply here. Two redirect rules: TodoWrite -> nudge toward the item MCP tool, Write/Edit to a specs/*.md path -> nudge toward artifact_publish (enforces the existing CLAUDE.md 'specs live in agentflare artifacts' rule instead of just hoping the model remembers it). Wired into claude-code's existing PreToolUse hook (src/hook.rs) with zero new host wiring needed there. Extended coverage to two more hosts that have a matching PreToolUse-equivalent event (Windsurf does not - it only hooks MCP-tool-use and shell commands, not native Write/TodoWrite): - cursor: src/init.rs's wire_cursor() gained a preToolUse entry (matcher Write, Cursor has no Glob/Edit/TodoWrite tools) via the same granular per-event idempotent backfill wire_claude_code already used, so existing installs pick it up on the next init instead of only fresh ones. - codex: new wire_codex_hooks(), writing ~/.codex/hooks.json (same shape as Claude Code's) plus upserting the [features] codex_hooks = true flag Codex requires in config.toml. Corrects a stale comment claiming Codex hooks need a plugin marketplace - verified against OpenAI's own docs and lean-ctx's shipped installer that hooks.json works directly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds PreToolUse redirect classification with timeout-based fail-open behavior, integrates it into hook processing, and wires Cursor and Codex configurations while preserving existing Cursor hooks. ChangesPreToolUse redirect flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PreToolUseHook
participant redirect_decision
participant classify
participant HookOutput
PreToolUseHook->>redirect_decision: tool name and full tool input
redirect_decision->>classify: classify tool call with timeout
classify-->>redirect_decision: deny reason or no decision
redirect_decision-->>PreToolUseHook: decision or None
PreToolUseHook->>HookOutput: print deny decision and exit early
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…lish artifact_publish is retired for this kind of content (see the handoff rework) — specs/design docs/plans now attach to the relevant item as an asset instead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/hook.rs (1)
135-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct test for the new early-return path.
pre_tool_usenow short-circuits on a redirect decision before touching session/runtime state. A test feeding aTodoWrite(or spec-pathWrite) payload throughpre_tool_useand asserting no session mutation / correct stdout would pin this integration contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hook.rs` around lines 135 - 141, Add a focused integration test for the early-return branch in pre_tool_use, using a TodoWrite or spec-path Write payload that produces a redirect_decision. Assert the redirect output is correct and session/runtime state remains unchanged, covering the path before normal state handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hook_redirect.rs`:
- Around line 32-35: Update is_spec_like_path to recognize repo-root-relative
specs/*.md paths as well as nested paths, while preserving backslash
normalization and the existing .md requirement. Add coverage for a bare path
such as specs/design.md.
In `@src/init.rs`:
- Line 1: Update wire_cursor and wire_codex_hooks to distinguish JSON parse
failures from ownership checks: warn and skip rewriting when existing hook
content is invalid instead of replacing it with an empty object. Replace unsafe
nested as_object_mut/as_array_mut unwraps with shape validation matching the
existing top-level checks, and skip gracefully with a warning for atypical
structures.
- Around line 442-461: Update wire_codex_hooks to validate the existing
hooks.json content and parsed JSON shape before merging hooks, matching the
established validation behavior in wire_cursor. Do not silently treat malformed
or incompatible existing configuration as an empty object; preserve valid object
settings while handling invalid input through the same error/fallback policy
used by wire_cursor.
- Around line 365-423: Update wire_cursor to validate the existing hooks.json
JSON and required object/array shapes before merging entries. Handle parse
failures or incompatible hooks/preToolUse values safely instead of defaulting
into malformed data or calling unwrap, while preserving valid existing
configuration and adding agentflare hooks only when the structure is compatible.
- Around line 486-501: Update the config-writing logic in the init flow to avoid
appending a duplicate [features] table. Parse the existing config.toml as TOML,
ensure the features table exists, and set codex_hooks to true regardless of its
current value before serializing and writing it back; preserve unrelated
configuration entries.
---
Nitpick comments:
In `@src/hook.rs`:
- Around line 135-141: Add a focused integration test for the early-return
branch in pre_tool_use, using a TodoWrite or spec-path Write payload that
produces a redirect_decision. Assert the redirect output is correct and
session/runtime state remains unchanged, covering the path before normal state
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40385003-e190-469e-b790-711009d962e5
📒 Files selected for processing (4)
src/hook.rssrc/hook_redirect.rssrc/init.rssrc/main.rs
Comment on lines
+32
to
+35
| fn is_spec_like_path(path: &str) -> bool { | ||
| let normalized = path.replace('\\', "/"); | ||
| normalized.contains("/specs/") && normalized.ends_with(".md") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
is_spec_like_path misses repo-root-relative specs/*.md paths.
normalized.contains("/specs/") requires a leading path segment before specs. A bare relative path like specs/design.md (no parent directory) won't match, even though it's exactly the pattern the PR objective describes ("Write/Edit operations targeting specs/*.md"). No test covers this case either.
🐛 Proposed fix
fn is_spec_like_path(path: &str) -> bool {
let normalized = path.replace('\\', "/");
- normalized.contains("/specs/") && normalized.ends_with(".md")
+ (normalized.starts_with("specs/") || normalized.contains("/specs/")) && normalized.ends_with(".md")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn is_spec_like_path(path: &str) -> bool { | |
| let normalized = path.replace('\\', "/"); | |
| normalized.contains("/specs/") && normalized.ends_with(".md") | |
| } | |
| fn is_spec_like_path(path: &str) -> bool { | |
| let normalized = path.replace('\\', "/"); | |
| (normalized.starts_with("specs/") || normalized.contains("/specs/")) && normalized.ends_with(".md") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hook_redirect.rs` around lines 32 - 35, Update is_spec_like_path to
recognize repo-root-relative specs/*.md paths as well as nested paths, while
preserving backslash normalization and the existing .md requirement. Add
coverage for a bare path such as specs/design.md.
| @@ -1,10 +1,10 @@ | |||
| // `agentflare init --agent X` — the one explicit, consent-is-the-invocation | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fragile handling of pre-existing hooks JSON in both wire_cursor and wire_codex_hooks. Both functions parse an existing hooks file and, on parse failure, silently fall back to an empty object — discarding the user's entire prior hook configuration without warning once the file is rewritten. Both also assume specific nested shapes (hooks as object, preToolUse as array) via .as_object_mut().unwrap()/.as_array_mut().unwrap() without the same defensive shape-check already applied to the top-level value, so an atypical or hand-edited file panics agentflare init instead of failing gracefully.
src/init.rs#L365-423: inwire_cursor, detect JSON parse failure onexistingseparately from the "not agentflare's" ownership check and skip with a warning instead of silently resetting to{}; guardhooks.entry("hooks")andhooks.entry("preToolUse")results with a shape check (mirroring theif !content.is_object()pattern) instead of unwrapping.src/init.rs#L442-461: inwire_codex_hooks, apply the same parse-failure warning-and-skip behavior, and guardhooks.as_object_mut()before unwrapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/init.rs` at line 1, Update wire_cursor and wire_codex_hooks to
distinguish JSON parse failures from ownership checks: warn and skip rewriting
when existing hook content is invalid instead of replacing it with an empty
object. Replace unsafe nested as_object_mut/as_array_mut unwraps with shape
validation matching the existing top-level checks, and skip gracefully with a
warning for atypical structures.
Comment on lines
365
to
+423
| fn wire_cursor() { | ||
| let path = cwd().join(".cursor").join("hooks.json"); | ||
| if path.exists() { | ||
| let existing = fs::read_to_string(&path).unwrap_or_default(); | ||
| if existing.contains("agentflare") { | ||
| println!(" skip .cursor/hooks.json (already wired)"); | ||
| return; | ||
| } | ||
| let existing = fs::read_to_string(&path).unwrap_or_default(); | ||
| if !existing.is_empty() && !existing.contains("agentflare") { | ||
| println!(" skip .cursor/hooks.json (exists, not agentflare's — not overwriting)"); | ||
| return; | ||
| } | ||
|
|
||
| let mut content: Value = serde_json::from_str(&existing).unwrap_or_else(|_| json!({})); | ||
| if !content.is_object() { | ||
| content = json!({}); | ||
| } | ||
| let bin = agentflare_binary(); | ||
| let content = json!({ | ||
| "version": 1, | ||
| "hooks": { | ||
| "sessionStart": [{ "command": format!("\"{bin}\" hook session-start"), "type": "command", "timeout": 30 }], | ||
| "beforeSubmitPrompt": [{ "command": format!("\"{bin}\" hook prompt-submit"), "type": "command", "timeout": 10 }] | ||
| } | ||
| }); | ||
| let obj = content.as_object_mut().unwrap(); | ||
| obj.entry("version").or_insert_with(|| json!(1)); | ||
| let hooks = obj | ||
| .entry("hooks") | ||
| .or_insert_with(|| json!({})) | ||
| .as_object_mut() | ||
| .unwrap(); | ||
|
|
||
| let mut added = false; | ||
| if !hooks | ||
| .get("sessionStart") | ||
| .is_some_and(|v| v.to_string().contains("hook session-start")) | ||
| { | ||
| hooks.insert( | ||
| "sessionStart".to_string(), | ||
| json!([{ "command": format!("\"{bin}\" hook session-start"), "type": "command", "timeout": 30 }]), | ||
| ); | ||
| added = true; | ||
| } | ||
| if !hooks | ||
| .get("beforeSubmitPrompt") | ||
| .is_some_and(|v| v.to_string().contains("hook prompt-submit")) | ||
| { | ||
| hooks.insert( | ||
| "beforeSubmitPrompt".to_string(), | ||
| json!([{ "command": format!("\"{bin}\" hook prompt-submit"), "type": "command", "timeout": 10 }]), | ||
| ); | ||
| added = true; | ||
| } | ||
| // preToolUse entries carry a `matcher` instead of `type`/`timeout` — Cursor's | ||
| // own hook schema for this event (Shell|Read|Write|Grep|Delete|Task|MCP:* are | ||
| // the only valid matchers; Cursor has no Glob/Edit/TodoWrite tools, so `Write` | ||
| // is the only matcher agentflare's redirect classifier needs here). | ||
| let pre_arr = hooks | ||
| .entry("preToolUse") | ||
| .or_insert_with(|| json!([])) | ||
| .as_array_mut() | ||
| .unwrap(); | ||
| if !pre_arr | ||
| .iter() | ||
| .any(|v| v.to_string().contains("hook pre-tool-use")) | ||
| { | ||
| pre_arr | ||
| .push(json!({ "matcher": "Write", "command": format!("\"{bin}\" hook pre-tool-use") })); | ||
| added = true; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
wire_cursor merge logic doesn't validate pre-existing file shape/parseability.
See consolidated comment (shared root cause with wire_codex_hooks).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/init.rs` around lines 365 - 423, Update wire_cursor to validate the
existing hooks.json JSON and required object/array shapes before merging
entries. Handle parse failures or incompatible hooks/preToolUse values safely
instead of defaulting into malformed data or calling unwrap, while preserving
valid existing configuration and adding agentflare hooks only when the structure
is compatible.
Comment on lines
+442
to
+461
| /// Codex hooks (`~/.codex/hooks.json`, same shape as Claude Code's | ||
| /// settings.json hooks) are gated behind an experimental feature flag Codex | ||
| /// itself requires. Source: https://learn.chatgpt.com/docs/extend/mcp?surface=cli | ||
| /// (verified 2026-07-13) — the flag lives in `config.toml`, not hooks.json. | ||
| fn wire_codex_hooks() { | ||
| let codex_dir = home().join(".codex"); | ||
| let hooks_path = codex_dir.join("hooks.json"); | ||
| let mut settings: Value = fs::read_to_string(&hooks_path) | ||
| .ok() | ||
| .and_then(|s| serde_json::from_str(&s).ok()) | ||
| .unwrap_or_else(|| json!({})); | ||
| if !settings.is_object() { | ||
| settings = json!({}); | ||
| } | ||
| let bin = agentflare_binary(); | ||
|
|
||
| let obj = settings.as_object_mut().unwrap(); | ||
| let hooks = obj.entry("hooks").or_insert_with(|| json!({})); | ||
| let hooks_obj = hooks.as_object_mut().unwrap(); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
wire_codex_hooks merge logic doesn't validate pre-existing file shape/parseability.
See consolidated comment (shared root cause with wire_cursor).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/init.rs` around lines 442 - 461, Update wire_codex_hooks to validate the
existing hooks.json content and parsed JSON shape before merging hooks, matching
the established validation behavior in wire_cursor. Do not silently treat
malformed or incompatible existing configuration as an empty object; preserve
valid object settings while handling invalid input through the same
error/fallback policy used by wire_cursor.
Comment on lines
+486
to
+501
| let config_path = codex_dir.join("config.toml"); | ||
| let config_content = fs::read_to_string(&config_path).unwrap_or_default(); | ||
| if config_content.contains("codex_hooks") { | ||
| println!(" skip ~/.codex/config.toml (codex_hooks flag already present)"); | ||
| return; | ||
| } | ||
| let mut updated = config_content.clone(); | ||
| if !updated.is_empty() && !updated.ends_with('\n') { | ||
| updated.push('\n'); | ||
| } | ||
| updated.push_str("[features]\ncodex_hooks = true\n"); | ||
| match fs::write(&config_path, updated) { | ||
| Ok(_) => println!(" ok ~/.codex/config.toml: codex_hooks feature flag enabled"), | ||
| Err(e) => println!(" fail writing ~/.codex/config.toml: {e}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of src/init.rs with line numbers and nearby context.
sed -n '430,540p' src/init.rs | cat -n
# Find other references to config.toml / codex_hooks / features handling in the repo.
rg -n "codex_hooks|config\.toml|\[features\]" src tests . -g '!target' -g '!node_modules'Repository: getappz/agentflare
Length of output: 1959
Avoid duplicating [features]
If config.toml already has a [features] table, this appends another one and makes the file invalid TOML. The contains("codex_hooks") check also misses codex_hooks = false, so the flag can stay disabled. Consider updating the existing table or parsing TOML instead of string appending.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/init.rs` around lines 486 - 501, Update the config-writing logic in the
init flow to avoid appending a duplicate [features] table. Parse the existing
config.toml as TOML, ensure the features table exists, and set codex_hooks to
true regardless of its current value before serializing and writing it back;
preserve unrelated configuration entries.
4 tasks
This was referenced Jul 14, 2026
6 tasks
This was referenced Aug 22, 2026
getappz
added a commit
that referenced
this pull request
Aug 27, 2026
…ning free text (#624) Item #170's false-positive class hit twice more in one session (items #192, #173): a description that merely mentions "design-spec" (e.g. referencing another item's spec) forces the review-only prompt even for a genuine implementation task, because nothing ever set the structured metadata.task_type signal detect_review_only already knows how to trust. handoff now accepts an optional task_type and merges it into the item's existing metadata (without clobbering other keys) both when targeting an existing item_id and when creating a new one. Agentflare-Agent: claude-code Agentflare-Branch: task/task-type-metadata-review-only-fix Agentflare-Session: e77fc32e-33d0-4884-ab55-fdda48fe45fd Co-authored-by: shiva <shiva@gosysinfo.tech>
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
1 participant
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.
Ports lean-ctx's fail-open timeout redirect flow (
decide_with_timeout,classify) — NOT its compression-specific handlers (dedup/read_dedup/search_rewrite/edit_health don't apply; agentflare has no read-compression tool to redirect into).Two rules:
TodoWrite-> nudge toward theitemMCP tool (action=create) for anything that should survive past the sessionWrite/Edit-> nudge towardartifact_publishinstead of committing specs/design docs/plans to the repoWired for:
pre_tool_usewire_cursor()with a preToolUse/Write matcher entry + granular backfillwire_codex_hooks(), hooks.json + config.tomlcodex_hooksfeature flag, corrects a stale comment claiming Codex needs a plugin marketplaceWindsurf excluded: its hooks only fire on MCP-tool-use/shell commands, not native Write/TodoWrite.
Rebased onto current master (which now already includes the tool consolidation) from its original fork point.
Verification
cargo fmt --checkcleancargo clippy --workspace --all-targets -- -D warnings -A unsafe_code -A clippy::pedanticcleanSummary by CodeRabbit