feat: handle hook rejection display using rejection_source and HookExecutionEvent#488
feat: handle hook rejection display using rejection_source and HookExecutionEvent#488xingyaoww wants to merge 14 commits into
Conversation
Update the CLI to use the new rejection_source field from the SDK to distinguish hook rejections from user rejections, instead of pattern matching on rejection_reason strings. This provides a cleaner, more reliable way to detect hook rejections. TUI changes (richlog_visualizer.py): - Add _is_hook_rejection() helper using getattr() for SDK compatibility - Add _get_rejection_title() and _get_rejection_icon() helpers - Show 'Hook Blocked Action' title and '⚡ ✗' icon for hook rejections - Falls back to 'User Rejected Action' and '✗' for user rejections ACP changes (shared_event_handler.py): - Add _is_hook_rejection() helper using getattr() for SDK compatibility - Prepend '**⚡ Hook Blocked Action**' header for hook rejections Tests: - Add 8 new tests for TUI hook rejection detection - Add 5 new tests for ACP hook rejection detection - Tests use model_copy() for SDK version compatibility Depends on: OpenHands/software-agent-sdk#1995 Co-authored-by: openhands <openhands@all-hands.dev>
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
@OpenHands can you help me fix pre-commit run --all-files on this PR |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
|
@OpenHands review diff from main, and can you help me create a snapshot test for the rejection event feature? |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
|
I've fixed the pre-commit issues on PR #488. The changes have been pushed to the What was fixed:
Verification:
The fix has been pushed to the PR branch and should now pass the pre-commit CI checks. |
Add visual snapshot tests to verify the display of user and hook rejection events: - test_user_rejection_display: verifies 'User Rejected Action' title - test_hook_rejection_display: verifies 'Hook Blocked Action' title with ⚡ icon - test_user_and_hook_rejections_comparison: shows both types side by side These tests ensure the visual appearance of rejection events is correct and provides regression protection for the rejection_source field feature.
SummaryI've reviewed the diff from main and created snapshot tests for the rejection event feature as requested. The changes have been pushed to the Changes MadeAdded to
Generated snapshot files:
Verification
The snapshot tests provide visual regression protection for the rejection event feature, ensuring that:
|
|
This PR is stale because it has been open for 40 days with no activity. Remove the stale label or leave a comment, otherwise it will be closed in 10 days. |
|
@OpenHands Please merge from main, resolve all conflicts, and review all the PR review comments. Tell me if the implementation is correct. I think this PR we were basically waiting for a new SDK release so we can import stuff from the upstream SDK. Right Now, I think the latest main already updates to use the latest version of the SDK. So can you update this PR so to get it ready for me |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
- Merge from main (SDK 1.16.0 with rejection_source field) - Replace getattr() workaround with direct event.rejection_source access - Integrate hook rejection detection with main's refactored visualizer - Update tests to use rejection_source parameter directly (no model_copy) - Keep both hook rejection tests and new agent prefix tests Co-authored-by: openhands <openhands@all-hands.dev>
Summary of Remaining IssueThe push triggered pre-commit checks, and two lint errors remain that need to be fixed: Ruff Lint Failures (E501 - Line too long)Both are the same issue — a docstring exceeding the 88-character line limit:
Additionally, ruff-format reformatted 1 file (likely auto-fixable formatting), which means the commit needs a follow-up fix. What Was Completed ✅
What Still Needs Fixing ❌
These are trivial one-line fixes — the docstrings just need to be shortened or wrapped. I should push a follow-up commit to fix these two lint errors so CI passes. |
Co-authored-by: openhands <openhands@all-hands.dev>
|
horray! 85ff6bf <- this was due to the hook :D |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean, simple solution that replaces fragile string parsing with proper field access.
Key Insight: Using a dedicated rejection_source field from the SDK instead of pattern matching is exactly the right data structure choice. The helper functions are simple, single-purpose, and eliminate special cases. Testing is thorough with real objects (not just mocks) and includes visual regression coverage.
✅ Worth merging: Core logic is sound, implementation is clean, and tests are comprehensive.
Optional improvement: Consider adding a screenshot to the PR description showing the actual TUI displaying a hook rejection with the ⚡ icon (nice-to-have, not blocking).
Remove HOOK_ICON constant and _get_rejection_icon() function. All rejection events now use the standard ERROR_ICON (✗). Remove ⚡ from ACP HOOK_BLOCKED_HEADER. Title distinction (Hook Blocked Action vs User Rejected Action) is preserved. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
The stop hook path uses HookExecutionEvent (not UserRejectObservation),
so 'Hook Blocked Action' was never shown for stop hook denials.
TUI: Import HookExecutionEvent, show 'Hook Blocked Action' collapsible
for blocked HookExecutionEvents, hide successful ones.
ACP: Add handle_hook_execution() to SharedEventHandler, dispatch in
event.py and token_streamer.py.
Tests: Add HookExecutionEvent tests for both TUI and ACP helpers.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Show all hook executions in TUI (as 'Hook Executed') and ACP, not just blocked ones. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
Finally worked! @enyst
|
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean solution that replaces fragile pattern matching with proper field access. The helper functions eliminate special cases, tests use real objects (not mocks), and the implementation is straightforward. Worth merging.

Summary
Update the CLI to properly display hook rejections by using both
UserRejectObservation.rejection_sourceandHookExecutionEventfrom the SDK. This covers both hook paths:UserRejectObservation(rejection_source="hook")HookExecutionEvent(blocked=true)All rejections use the standard error icon (✗) — no special icon.
Changes
TUI (
openhands_cli/tui/widgets/richlog_visualizer.py)_is_hook_rejection()helper: checksUserRejectObservation.rejection_source == "hook"orHookExecutionEvent.blocked_get_rejection_title()helper for dynamic titlesHookExecutionEvent: show "Hook Blocked Action" when blocked, "Hook Executed" when successfulHookExecutionEventwith error themeACP (
openhands_cli/acp_impl/events/)_is_hook_rejection()helper handling bothHookExecutionEventandUserRejectObservationhandle_hook_execution()toSharedEventHandler— prepends "Hook Blocked Action" header for blocked hooks, shows all hook executionsHookExecutionEventin bothevent.pyandtoken_streamer.pyTests
TestHookRejectionDetection) — coversUserRejectObservation,HookExecutionEvent, andAgentErrorEventTestACPHookRejectionDetection)TestRejectionEventSnapshots)Docs
AGENTS.mdVisual Example
Stop hook blocked (HookExecutionEvent):
Stop hook passed (HookExecutionEvent):
PreToolUse hook blocked (UserRejectObservation):
User rejection:
Verification
make lintpassedmake testpassed (1293 tests)make test-snapshotspassedHookExecutionEventwithblocked: true🚀 Try this PR