-
Notifications
You must be signed in to change notification settings - Fork 100
feat(sdk): complete hooks implementation with additional context and stop hook #1547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(sdk): complete hooks implementation with additional context and stop hook #1547
Conversation
Coverage Report •
|
||||||||||||||||||||||||||||||
322a14d to
4c36ba1
Compare
…stop hook - Implement additional_context injection in UserPromptSubmit hooks - Hook context is appended to MessageEvent.extended_content - Context flows through condensation and is included in LLM messages - Implement stop hook integration in conversation run loop - Stop hooks can deny premature agent completion - Feedback from hooks is injected as user message with [Stop hook feedback] prefix - Agent continues running after stop hook denial - Add comprehensive tests for both features - Tests for context appearing in extended_content and to_llm_message() - Tests for stop hook denial with feedback injection - Integration tests for full conversation loop with stop hooks - Add advanced hooks example (34_hooks_advanced.py)
4c36ba1 to
bda858e
Compare
|
This likely solves #1527! |
| config = HookConfig.from_dict( | ||
| { | ||
| "hooks": { | ||
| "UserPromptSubmit": [ | ||
| { | ||
| "hooks": [{"type": "command", "command": str(context_script)}], | ||
| } | ||
| ], | ||
| "Stop": [ | ||
| { | ||
| "hooks": [{"type": "command", "command": str(stop_script)}], | ||
| } | ||
| ], | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: could we make this a HookConfig a pydantic class that contains UserPromptSubmit Stop, etc, which are other pydantic class, so we can have stricter type check instead of putting together a dict like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great idea. I’ll plan on implementing it in a follow-up PR so that we can keep this PR’s scope focused.
- Move 33_hooks.py to 33_hooks/ folder with separate scripts - Split into basic_hooks.py and advanced_hooks.py - Remove main() wrapper to match codebase convention (31/33 examples) - Extract shell scripts to scripts/ directory for reusability - Add README.md documenting hook types and usage Addresses review feedback: - Consolidate hook examples into folder structure - Simplify examples by removing main() wrapper
…with-additional-context-and-stop-hook-13
examples/01_standalone_sdk/33_hooks/hook_scripts/block_dangerous.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands please send a PR to OpenHands/docs repo to update the old 33_hooks.py to this new one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've successfully created a PR to the OpenHands/docs repo to update the hooks documentation with the new 33_hooks.py example.
Summary
PR Created: OpenHands/docs#230
Changes Made
Updated sdk/guides/hooks.mdx to reflect the new hooks example from the software-agent-sdk:
-
Updated the Python example code - Replaced the old inline script creation approach with the new structure that uses external hook scripts in a
hook_scripts/directory -
Added documentation for all four hook types:
- PreToolUse: Block dangerous commands before execution
- PostToolUse: Log tool usage after execution
- UserPromptSubmit: Inject context into user messages
- Stop: Enforce task completion criteria
-
Added expandable accordion sections showing each hook script:
block_dangerous.shlog_tools.shinject_git_context.shrequire_summary.sh
-
Added a Hook Types reference table documenting when each hook runs and whether it can block operations
-
Updated the GitHub link to point to the new directory structure (
examples/01_standalone_sdk/33_hooks/)
The PR cross-references the related SDK PR (#1547).
Update the hooks documentation to reflect the new 33_hooks.py example that demonstrates all four hook types: - PreToolUse: Block dangerous commands - PostToolUse: Log tool usage - UserPromptSubmit: Inject context into user messages - Stop: Enforce task completion criteria The new example uses external hook scripts in a hook_scripts/ directory and includes a Hook Types reference table. Related to: OpenHands/software-agent-sdk#1547 Co-authored-by: openhands <openhands@all-hands.dev>
| # Run UserPromptSubmit hooks for user messages | ||
| if isinstance(event, MessageEvent) and event.source == "user": | ||
| self._handle_user_prompt_submit(event) | ||
| callback_event = self._handle_user_prompt_submit(event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands, does this mean we will modify the event passed to the callback? Does it break the pattern we currently have for on_event? Ideally we should never mutate event passed to on_event, please investigate. Do not modify any code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answer to Your Question
Does this modify the event passed to the callback? Does it break the on_event pattern?
No Mutation Occurs
The original event is NOT mutated. Events are frozen Pydantic models (verified by test_event_immutability.py). When additional_context needs to be injected, a new MessageEvent is created:
# MessageEvent is frozen, so create a new one
event = MessageEvent(
source=event.source,
llm_message=event.llm_message,
...
extended_content=new_extended_content,
)Pattern Concern
The current on_event pattern is that all callbacks in a composed chain receive the same event (via compose_callbacks). The PR's design intends to pass a different (modified) event to downstream callbacks via original_callback.
However, there's an integration issue: in LocalConversation.__init__, create_hook_callback is called without passing original_callback, so it defaults to None. This means the modified event is created but never actually passed to downstream callbacks - it's lost.
Summary
- No mutation - Events are frozen; a new event is created instead
- Pattern concern is valid - If
original_callbackwere properly wired, downstream callbacks would receive a different event than what was originally emitted, which departs from the current "all callbacks see the same event" pattern - Current integration doesn't work as intended - The modified event with
additional_contextis created but lost becauseoriginal_callbackisNonein the actualLocalConversationsetup. The tests pass only because they explicitly provideoriginal_callback=capture_callback.
The design intent seems to be that the hook processor should intercept events, potentially modify them, and pass the modified version downstream - which is architecturally different from the current pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, there's an integration issue: in LocalConversation.init, create_hook_callback is called without passing original_callback, so it defaults to None. This means the modified event is created but never actually passed to downstream callbacks - it's lost.
@ak684 is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to pass to callback, also added test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xingyaoww I wonder WDYT about this solution?
@ak684
Please let me take a step back here and look at our options. We have the goal to inject some extra content in the user message, just like a keyword-skill.
- modify the event
- add a new event (this PR’s solution)
- intervene before the user message event is created
1 and 2 are after the event was created, and saved in EventLog (I think it was saved too?)
For 2, we then have a question:
a. do we notify listeners about the new event
b. or about both?
Also for 2, I think we have another question that I’m most interested in:
- do we send to the LLM only the second, or do we send both?
- (IMHO we should send only the second, since it seems to me it contains all
contentof the first, too; or make it have only theextended content)
Added tests verifying LocalConversation correctly wires hook callbacks to event persistence via original_callback parameter.
Use grep on raw JSON input instead of jq for detecting dangerous commands. This makes the example work out of the box without requiring jq installation.
|
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 |
follow up to: #1467
fixes #1527
Implement additional_context injection in UserPromptSubmit hooks
Implement stop hook integration in conversation run loop
Add comprehensive tests for both features
Add advanced hooks example (34_hooks_advanced.py)
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:ff9c2ef-pythonRun
All tags pushed for this build
About Multi-Architecture Support
ff9c2ef-python) is a multi-arch manifest supporting both amd64 and arm64ff9c2ef-python-amd64) are also available if needed