feat(hooks): Support file-based hook scripts in .openhands directory#1719
feat(hooks): Support file-based hook scripts in .openhands directory#1719
Conversation
|
[Automatic Post]: It has been a while since there was any activity on this PR. @xingyaoww, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
…very - Create utils.py with load_project_hooks() function similar to load_project_skills() - Add discover_hook_scripts() to find hook scripts in .openhands directory - Support standard naming (stop.sh, pre_tool_use.sh, etc.) and legacy (agent_finish.sh) - Support scripts in both .openhands/ and .openhands/hooks/ directories - Update HookConfig.load() to merge JSON config with discovered scripts - Add comprehensive tests for the new functionality Co-authored-by: openhands <openhands@all-hands.dev>
950b2ba to
a26c365
Compare
| @@ -216,34 +216,60 @@ def load( | |||
There was a problem hiding this comment.
@OpenHands can we also support passing a list of working_dir in case we have multiple dirs to load hooks from?
There was a problem hiding this comment.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
I've implemented support for passing a list of working_dir to HookConfig.load(). The changes have been pushed to the PR branch.
Summary of Changes
openhands-sdk/openhands/sdk/hooks/config.py
- Updated
HookConfig.load()method signature to acceptworking_dir: str | Path | list[str | Path] | None - When a list is provided, hooks from all directories are discovered and merged together
- Backward compatible: single directory or
Nonestill works as before
tests/sdk/hooks/test_config.py
- Added new
TestMultipleWorkingDirstest class with 6 test cases:test_load_with_list_of_working_dirs- verifies loading from multiple directoriestest_load_merges_hooks_from_multiple_dirs- verifies hooks are merged when same event type exists in multiple dirstest_load_with_json_and_scripts_from_multiple_dirs- verifies JSON and scripts from different dirs are mergedtest_load_with_empty_list_returns_empty_config- edge case handlingtest_load_with_single_item_list- single-item list works correctlytest_load_with_nonexistent_dirs_in_list- gracefully handles nonexistent directories
All 33 tests pass.
Checklist
- ✅
working_dirparameter now accepts a list of directories - ✅ Hooks from all directories are merged together
- ✅ Backward compatible with existing single directory usage
- ✅ Comprehensive test coverage added
- ✅ Changes pushed to PR branch
feature/agent-finish-sh-hook
| "session_start.sh": HookEventType.SESSION_START, | ||
| "session_end.sh": HookEventType.SESSION_END, | ||
| # Legacy naming for backward compatibility | ||
| "agent_finish.sh": HookEventType.STOP, |
There was a problem hiding this comment.
@OpenHands let's remove the agent_finish.sh, we should just keep stop.sh. Please update OpenHands/docs#259 to reflect this updated implementation
There was a problem hiding this comment.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
I've completed the requested changes to remove agent_finish.sh and keep only stop.sh. Here's a summary:
Changes Made
1. software-agent-sdk PR #1719 (OpenHands/software-agent-sdk)
Commit: f9b4cbe6 - "refactor(hooks): remove agent_finish.sh, keep only stop.sh"
openhands-sdk/openhands/sdk/hooks/utils.py: Removedagent_finish.shfromHOOK_SCRIPT_MAPPINGand updated commentsopenhands-sdk/openhands/sdk/hooks/config.py: Updated docstring to remove mention ofagent_finish.shtests/sdk/hooks/test_config.py: Removed 2 test functions related toagent_finish.sh:test_load_discovers_agent_finish_scripttest_discover_hook_scripts_finds_legacy_agent_finish
All 25 tests pass after the changes.
2. docs PR #259 (OpenHands/docs)
Commit: 340eebf - "docs: rename agent_finish.sh to stop.sh"
openhands/usage/customization/repository.mdx: Renamed section from "Agent Finish Script" to "Stop Script" and updated the filename from.openhands/agent_finish.shto.openhands/stop.sh- Updated PR title from "Document .openhands/agent_finish.sh repository hook" to "Document .openhands/stop.sh repository hook"
- Updated PR description accordingly
Both PRs have been pushed to their respective remote branches.
Allow HookConfig.load() to accept a list of directories for loading hooks from multiple locations. When a list is provided, hooks from all directories are discovered and merged together. This is useful for scenarios where hooks need to be loaded from multiple project directories or when combining project-level hooks with shared hooks. Co-authored-by: openhands <openhands@all-hands.dev>
Remove legacy agent_finish.sh hook script support. Users should use stop.sh instead, which follows the standard event-based naming convention. Co-authored-by: openhands <openhands@all-hands.dev>
| if config_path is None: | ||
| # Search for hooks.json in standard locations | ||
| search_paths = [ | ||
| base_dir / ".openhands" / "hooks.json", | ||
| Path.home() / ".openhands" / "hooks.json", | ||
| ] | ||
| for search_path in search_paths: | ||
| if search_path.exists(): | ||
| config_path = search_path | ||
| break |
There was a problem hiding this comment.
@OpenHands /codereview-roasted, will we load Path.home() / ".openhands" / "hooks.json" as duplicate in the loop? Can we just create a helper function _load_hook_dir, and it will return loaded json config when hooks.json exists there, and return None if not? This should greatly simplify the code here. We basically need to loop through all working dirs AND ~/.openhands, and then merge ALL hooks together.
Can we also check to make sure the hooks.json are refer to scripts with absolute path? I worry about relative path could make things a bit weird here - Please read the code and evaluate the risk there
There was a problem hiding this comment.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
I've addressed the code review comments and pushed the changes to the PR. Here's a summary of what was done:
Changes Made
1. Created _load_hook_dir() helper function
- Extracts the logic to load hooks from a single directory (both
hooks.jsonand hook scripts) - Returns
HookConfig | None- the loaded config or None if no hooks found - Accepts a
load_scriptsparameter to control whether to discover hook scripts (set to False for~/.openhandsto avoid loading scripts from there)
2. Refactored HookConfig.load() to avoid duplicate loading
- Fixed the bug: Previously,
~/.openhands/hooks.jsonwould be loaded multiple times when multiple working directories were provided (once per working directory in the loop) - Now the method:
- Loads hooks from each working directory (both
hooks.jsonand scripts) - Loads
~/.openhands/hooks.jsonexactly once at the end (only if not already loaded via working_dirs) - Merges all discovered hooks together
- Loads hooks from each working directory (both
3. Added _warn_relative_paths() function
- Logs warnings when
hooks.jsoncommands use relative paths (./script.shor../script.sh) - These paths can be problematic because the working directory when the hook runs may differ from where
hooks.jsonwas loaded
4. Added comprehensive tests
TestRelativePathWarning: Tests for relative path warning behaviorTestHomeDirDeduplication: Tests that~/.openhands/hooks.jsonis only loaded once with multiple working dirs, and that home dir hooks are properly merged with working dir hooks
Checklist
- Created
_load_hook_dirhelper function as requested - Fixed duplicate loading of
~/.openhands/hooks.json - Added validation/warning for relative paths in
hooks.json - All 36 tests pass
- Pre-commit hooks pass (ruff format, ruff lint, pyright, etc.)
- Changes pushed to
feature/agent-finish-sh-hookbranch
The PR has been updated at: #1719
…h warnings - Create _load_hook_dir() helper function to simplify hook loading from a single directory - Refactor HookConfig.load() to avoid duplicate loading of ~/.openhands/hooks.json when multiple working directories are provided - Add _warn_relative_paths() to log warnings when hooks.json commands use relative paths (./script.sh or ../script.sh) which could be problematic when the working directory changes - Add comprehensive tests for: - Relative path warning behavior - Home directory deduplication with multiple working dirs - Home directory hooks merged with working dir hooks Co-authored-by: openhands <openhands@all-hands.dev>
|
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 |
- Remove dead code: empty TYPE_CHECKING block in config.py - Fix DRY violation: remove duplicate _pascal_to_snake from utils.py and import from config.py instead - Improve silent failure pattern: hook script commands now log failures to stderr before returning true, making debugging easier Co-authored-by: openhands <openhands@all-hands.dev>
|
Looks like this is already support. @OpenHands can you review diff, and just KEEP the support to load hooks from multiple workspaces, and then revert all unncessary changes in this PR. I want it to be really succint |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
…ple workspaces Remove file-based hook script discovery (stop.sh, pre_tool_use.sh, etc.) and keep only the minimal change to support loading hooks.json from multiple working directories. Changes: - HookConfig.load() now accepts working_dir as a list of paths - Hooks from all directories are merged together - ~/.openhands/hooks.json is still loaded as global config - Removed utils.py (script discovery functionality) - Reverted __init__.py to main branch state - Simplified tests to only cover multiple working directories
|
I've simplified PR #1719 to only keep the support for loading hooks from multiple workspaces, reverting all unnecessary changes. Summary of ChangesFiles Changed (2 files, +253/-26 lines):
Reverted/Removed:
Kept Feature:
Usage Example: config = HookConfig.load(working_dir=['/project1', '/project2'])All 23 tests pass. Changes have been pushed to the |
Summary
Adds support for file-based hook scripts in the
.openhands/directory. This enables backward-compatible, convention-based hook configuration without requiringhooks.json.Features
Hook Script Discovery
HookConfig.load()now automatically discovers and registers shell scripts in.openhands/or.openhands/hooks/:stop.shpre_tool_use.shpost_tool_use.shuser_prompt_submit.shsession_start.shsession_end.shMultiple Working Directories
HookConfig.load()now accepts a list of working directories, merging hooks from all sources:Implementation Details
bash {script_path} || trueto prevent failures from blocking events~/.openhands/are NOT auto-loaded (onlyhooks.jsonfrom home dir)hooks.jsoncommands using./or../Files Changed
openhands/sdk/hooks/config.py: Extendedload()method with script discovery and multi-dir supportopenhands/sdk/hooks/utils.py: New module withdiscover_hook_scripts()andload_project_hooks()openhands/sdk/hooks/__init__.py: Exportload_project_hookstests/sdk/hooks/test_config.py: Comprehensive test coverageUsage Example
The hook will be automatically discovered and executed when the agent stops.
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:da9f72f-pythonRun
All tags pushed for this build
About Multi-Architecture Support
da9f72f-python) is a multi-arch manifest supporting both amd64 and arm64da9f72f-python-amd64) are also available if needed