fix(hook): stdin timeout + stderr logging + bare /agentflare report - #80
Conversation
- Add read_stdin_timeout(ms) helper using thread+channel+recv_timeout - Replace blocking stdin().read_to_string() in pre_tool_use and prompt_submit - 1s timeout, fail-open: timeout → log to stderr, skip gracefully - Bare /agentflare or /agentflare status reports active state - Closes ponytail audit PR tickets #71, #70, #75
📝 WalkthroughWalkthroughAdds a ChangesHook stdin timeout and status command
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hook.rs (2)
97-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated timeout-handling boilerplate across both call sites.
The
match read_stdin_timeout(1000) { Some(s) if !s.is_empty() => s, _ => { eprintln!(...); return; } }block is repeated verbatim (only the log label differs) inpre_tool_useandprompt_submit, and the1000ms timeout is a duplicated magic literal. Consider extracting a small helper to reduce duplication and centralize the timeout constant.♻️ Suggested refactor
+const STDIN_TIMEOUT_MS: u64 = 1000; + +fn read_stdin_or_skip(label: &str) -> Option<String> { + match read_stdin_timeout(STDIN_TIMEOUT_MS) { + Some(s) if !s.is_empty() => Some(s), + _ => { + eprintln!("[agentflare] {label}: stdin timeout or empty — skipping"); + None + } + } +}Then in
pre_tool_use:- let input = match read_stdin_timeout(1000) { - Some(s) if !s.is_empty() => s, - _ => { - eprintln!("[agentflare] PreToolUse: stdin timeout or empty — skipping"); - return; - } - }; + let Some(input) = read_stdin_or_skip("PreToolUse") else { return };And similarly in
prompt_submitwith the"UserPromptSubmit"label.Also applies to: 155-161
🤖 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 97 - 103, Extract the repeated stdin timeout handling in pre_tool_use and prompt_submit into a small shared helper that wraps read_stdin_timeout and the empty-input check, so both call sites just pass their log label. Replace the duplicated 1000 ms magic literal with a central timeout constant used by that helper, and keep the existing eprintln!/return behavior for the "PreToolUse" and "UserPromptSubmit" paths.
269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest doesn't verify meaningful behavior.
read_stdin_timeout_does_not_paniconly checks the call doesn't panic; it exercises neither theNone(timeout) branch nor a populated-input branch. Given this is the core fix for tickets#71/#70, consider parameterizing the read logic (e.g., accept a genericReador an injected timeout duration for testing) so the timeout path can be asserted deterministically instead of relying on real stdin timing in tests.🤖 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 269 - 273, The read_stdin_timeout test currently only proves the call does not panic and does not validate either the timeout path or the successful input path. Refactor the stdin-reading logic in read_stdin_timeout to be testable with an injected reader or configurable timeout so you can deterministically assert the None branch and a populated-input branch without relying on real stdin timing. Update read_stdin_timeout and its tests, including read_stdin_timeout_does_not_panic, to cover the actual behavior of the timeout 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.
Nitpick comments:
In `@src/hook.rs`:
- Around line 97-103: Extract the repeated stdin timeout handling in
pre_tool_use and prompt_submit into a small shared helper that wraps
read_stdin_timeout and the empty-input check, so both call sites just pass their
log label. Replace the duplicated 1000 ms magic literal with a central timeout
constant used by that helper, and keep the existing eprintln!/return behavior
for the "PreToolUse" and "UserPromptSubmit" paths.
- Around line 269-273: The read_stdin_timeout test currently only proves the
call does not panic and does not validate either the timeout path or the
successful input path. Refactor the stdin-reading logic in read_stdin_timeout to
be testable with an injected reader or configurable timeout so you can
deterministically assert the None branch and a populated-input branch without
relying on real stdin timing. Update read_stdin_timeout and its tests, including
read_stdin_timeout_does_not_panic, to cover the actual behavior of the timeout
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5666e382-0309-421e-adc5-5aaff5dc3eac
📒 Files selected for processing (1)
src/hook.rs
Changes
ead_stdin_timeout(ms)\ helper using thread+channel+recv_timeout (stdlib only)
Tickets
References
Test
Summary by CodeRabbit
New Features
/agentflareand/agentflare statusto report whether the hook is currently ACTIVE or off.Bug Fixes
Tests