From a26c365600e1cae2ab06c5f50e7ade66425aa688 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 30 Jan 2026 15:58:37 +0000 Subject: [PATCH 1/6] refactor(hooks): add load_project_hooks for general hook script discovery - 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-sdk/openhands/sdk/hooks/__init__.py | 2 + openhands-sdk/openhands/sdk/hooks/config.py | 56 ++++-- openhands-sdk/openhands/sdk/hooks/utils.py | 151 +++++++++++++++ tests/sdk/hooks/test_config.py | 183 ++++++++++++++++++ 4 files changed, 377 insertions(+), 15 deletions(-) create mode 100644 openhands-sdk/openhands/sdk/hooks/utils.py diff --git a/openhands-sdk/openhands/sdk/hooks/__init__.py b/openhands-sdk/openhands/sdk/hooks/__init__.py index f76b8d38af..d81c8f990e 100644 --- a/openhands-sdk/openhands/sdk/hooks/__init__.py +++ b/openhands-sdk/openhands/sdk/hooks/__init__.py @@ -19,6 +19,7 @@ from openhands.sdk.hooks.executor import HookExecutor, HookResult from openhands.sdk.hooks.manager import HookManager from openhands.sdk.hooks.types import HookDecision, HookEvent, HookEventType +from openhands.sdk.hooks.utils import load_project_hooks __all__ = [ @@ -35,4 +36,5 @@ "HookDecision", "HookEventProcessor", "create_hook_callback", + "load_project_hooks", ] diff --git a/openhands-sdk/openhands/sdk/hooks/config.py b/openhands-sdk/openhands/sdk/hooks/config.py index 1d5a8bdd92..7c0abe99df 100644 --- a/openhands-sdk/openhands/sdk/hooks/config.py +++ b/openhands-sdk/openhands/sdk/hooks/config.py @@ -216,34 +216,60 @@ def load( ) -> "HookConfig": """Load config from path or search .openhands/hooks.json locations. + Also discovers hook scripts in the .openhands directory (e.g., stop.sh, + agent_finish.sh) and merges them with the JSON config. + Args: - path: Explicit path to hooks.json file. If provided, working_dir is ignored. - working_dir: Project directory for discovering .openhands/hooks.json. - Falls back to cwd if not provided. + path: Explicit path to hooks.json file. If provided, working_dir is ignored + for JSON loading but still used for script discovery. + working_dir: Project directory for discovering .openhands/hooks.json + and hook scripts. Falls back to cwd if not provided. """ - if path is None: + # Import here to avoid circular dependency + from openhands.sdk.hooks.utils import load_project_hooks + + base_dir = Path(working_dir) if working_dir else Path.cwd() + + # Load JSON config + json_config: HookConfig | None = None + config_path = path + + if config_path is None: # Search for hooks.json in standard locations - base_dir = Path(working_dir) if working_dir else Path.cwd() search_paths = [ base_dir / ".openhands" / "hooks.json", Path.home() / ".openhands" / "hooks.json", ] for search_path in search_paths: if search_path.exists(): - path = search_path + config_path = search_path break - if path is None: + if config_path is not None: + config_path = Path(config_path) + if config_path.exists(): + try: + with open(config_path) as f: + data = json.load(f) + json_config = cls.model_validate(data) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to load hooks from {config_path}: {e}") + + # Load hook scripts from .openhands directory + script_config = load_project_hooks(base_dir) + + # Merge configs: JSON config takes precedence, script hooks are appended + configs_to_merge = [] + if json_config is not None and not json_config.is_empty(): + configs_to_merge.append(json_config) + if not script_config.is_empty(): + configs_to_merge.append(script_config) + + if not configs_to_merge: return cls() - path = Path(path) - if not path.exists(): - return cls() - - with open(path) as f: - data = json.load(f) - # Use model_validate which triggers the model_validator - return cls.model_validate(data) + merged = cls.merge(configs_to_merge) + return merged if merged is not None else cls() @classmethod def from_dict(cls, data: dict[str, Any]) -> "HookConfig": diff --git a/openhands-sdk/openhands/sdk/hooks/utils.py b/openhands-sdk/openhands/sdk/hooks/utils.py new file mode 100644 index 0000000000..022b91fea2 --- /dev/null +++ b/openhands-sdk/openhands/sdk/hooks/utils.py @@ -0,0 +1,151 @@ +"""Utility functions for hook loading and management.""" + +from __future__ import annotations + +import logging +from pathlib import Path +from typing import TYPE_CHECKING + +from openhands.sdk.hooks.types import HookEventType + + +if TYPE_CHECKING: + from openhands.sdk.hooks.config import HookConfig + +logger = logging.getLogger(__name__) + + +# Mapping of script filenames to hook event types. +# Scripts can be named either by event type (e.g., stop.sh) or by legacy names. +HOOK_SCRIPT_MAPPING: dict[str, HookEventType] = { + # Standard event-based naming (snake_case) + "stop.sh": HookEventType.STOP, + "pre_tool_use.sh": HookEventType.PRE_TOOL_USE, + "post_tool_use.sh": HookEventType.POST_TOOL_USE, + "user_prompt_submit.sh": HookEventType.USER_PROMPT_SUBMIT, + "session_start.sh": HookEventType.SESSION_START, + "session_end.sh": HookEventType.SESSION_END, + # Legacy naming for backward compatibility + "agent_finish.sh": HookEventType.STOP, +} + +# Default timeout for hook scripts (10 minutes) +DEFAULT_HOOK_SCRIPT_TIMEOUT = 600 + + +def _pascal_to_snake(name: str) -> str: + """Convert PascalCase to snake_case.""" + import re + + result = re.sub(r"(? dict[HookEventType, list[Path]]: + """Discover hook scripts in the .openhands directory. + + Searches for executable shell scripts that match known hook event types. + Scripts can be placed directly in .openhands/ or in .openhands/hooks/. + + Args: + openhands_dir: Path to the .openhands directory. + + Returns: + Dictionary mapping HookEventType to list of script paths found. + """ + discovered: dict[HookEventType, list[Path]] = {} + + if not openhands_dir.exists() or not openhands_dir.is_dir(): + return discovered + + # Search locations: .openhands/ and .openhands/hooks/ + search_dirs = [openhands_dir] + hooks_subdir = openhands_dir / "hooks" + if hooks_subdir.exists() and hooks_subdir.is_dir(): + search_dirs.append(hooks_subdir) + + for search_dir in search_dirs: + for script_name, event_type in HOOK_SCRIPT_MAPPING.items(): + script_path = search_dir / script_name + if script_path.exists() and script_path.is_file(): + if event_type not in discovered: + discovered[event_type] = [] + # Avoid duplicates (same script found in multiple locations) + if script_path not in discovered[event_type]: + discovered[event_type].append(script_path) + logger.debug( + f"Discovered hook script: {script_path} -> {event_type.value}" + ) + + return discovered + + +def load_project_hooks(work_dir: str | Path) -> HookConfig: + """Load hooks from project-specific files in the .openhands directory. + + Discovers hook scripts in {work_dir}/.openhands/ and creates a HookConfig + with the appropriate hook definitions. This is similar to load_project_skills + but for hooks. + + Supported script locations: + - {work_dir}/.openhands/{event_type}.sh (e.g., stop.sh, pre_tool_use.sh) + - {work_dir}/.openhands/hooks/{event_type}.sh + - {work_dir}/.openhands/agent_finish.sh (legacy, maps to STOP event) + + Args: + work_dir: Path to the project/working directory. + + Returns: + HookConfig with hooks discovered from script files. + Returns empty HookConfig if no hooks found. + """ + # Import here to avoid circular dependency + from openhands.sdk.hooks.config import ( + HookConfig, + HookDefinition, + HookMatcher, + HookType, + ) + + if isinstance(work_dir, str): + work_dir = Path(work_dir) + + openhands_dir = work_dir / ".openhands" + discovered_scripts = discover_hook_scripts(openhands_dir) + + if not discovered_scripts: + logger.debug(f"No hook scripts found in {openhands_dir}") + return HookConfig() + + # Build hook config from discovered scripts + hook_data: dict[str, list[HookMatcher]] = {} + + for event_type, script_paths in discovered_scripts.items(): + field_name = _pascal_to_snake(event_type.value) + matchers: list[HookMatcher] = [] + + for script_path in script_paths: + # Use relative path from work_dir for the command + relative_path = script_path.relative_to(work_dir) + # Use || true to prevent hook failures from blocking the event + command = f"bash {relative_path} || true" + + matchers.append( + HookMatcher( + matcher="*", + hooks=[ + HookDefinition( + type=HookType.COMMAND, + command=command, + timeout=DEFAULT_HOOK_SCRIPT_TIMEOUT, + ) + ], + ) + ) + + if matchers: + hook_data[field_name] = matchers + + num_scripts = sum(len(m) for m in hook_data.values()) + logger.debug(f"Loaded {num_scripts} hook scripts from {openhands_dir}") + return HookConfig(**hook_data) diff --git a/tests/sdk/hooks/test_config.py b/tests/sdk/hooks/test_config.py index dc97cf65b6..2e62828b04 100644 --- a/tests/sdk/hooks/test_config.py +++ b/tests/sdk/hooks/test_config.py @@ -1,10 +1,12 @@ """Tests for hook configuration loading and management.""" import json +import os import tempfile from openhands.sdk.hooks.config import HookConfig, HookDefinition, HookMatcher from openhands.sdk.hooks.types import HookEventType +from openhands.sdk.hooks.utils import discover_hook_scripts, load_project_hooks class TestHookMatcher: @@ -229,3 +231,184 @@ def test_unknown_event_type_raises_error(self): HookConfig.from_dict( {"PreToolExecute": [{"hooks": [{"command": "test.sh"}]}]} ) + + def test_load_discovers_agent_finish_script(self): + """Test that load() discovers .openhands/agent_finish.sh (legacy naming).""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + finish_script = os.path.join(openhands_dir, "agent_finish.sh") + with open(finish_script, "w") as f: + f.write("#!/bin/bash\necho done\n") + + config = HookConfig.load(working_dir=tmpdir) + + assert config.has_hooks_for_event(HookEventType.STOP) + hooks = config.get_hooks_for_event(HookEventType.STOP, None) + assert len(hooks) == 1 + assert hooks[0].command == "bash .openhands/agent_finish.sh || true" + assert hooks[0].timeout == 600 + + def test_load_discovers_stop_script(self): + """Test that load() discovers .openhands/stop.sh (standard naming).""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + stop_script = os.path.join(openhands_dir, "stop.sh") + with open(stop_script, "w") as f: + f.write("#!/bin/bash\necho stopping\n") + + config = HookConfig.load(working_dir=tmpdir) + + assert config.has_hooks_for_event(HookEventType.STOP) + hooks = config.get_hooks_for_event(HookEventType.STOP, None) + assert len(hooks) == 1 + assert hooks[0].command == "bash .openhands/stop.sh || true" + + def test_load_discovers_scripts_in_hooks_subdir(self): + """Test that load() discovers scripts in .openhands/hooks/ subdirectory.""" + with tempfile.TemporaryDirectory() as tmpdir: + hooks_dir = os.path.join(tmpdir, ".openhands", "hooks") + os.makedirs(hooks_dir) + pre_tool_script = os.path.join(hooks_dir, "pre_tool_use.sh") + with open(pre_tool_script, "w") as f: + f.write("#!/bin/bash\necho pre-tool\n") + + config = HookConfig.load(working_dir=tmpdir) + + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) + hooks = config.get_hooks_for_event(HookEventType.PRE_TOOL_USE, "AnyTool") + assert len(hooks) == 1 + assert hooks[0].command == "bash .openhands/hooks/pre_tool_use.sh || true" + + def test_load_merges_json_and_scripts(self): + """Test that load() merges hooks.json with discovered scripts.""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + + # Create hooks.json with pre_tool_use hook + hooks_json = os.path.join(openhands_dir, "hooks.json") + data = {"PreToolUse": [{"hooks": [{"command": "json-hook.sh"}]}]} + with open(hooks_json, "w") as f: + json.dump(data, f) + + # Create stop.sh script + stop_script = os.path.join(openhands_dir, "stop.sh") + with open(stop_script, "w") as f: + f.write("#!/bin/bash\necho stop\n") + + config = HookConfig.load(working_dir=tmpdir) + + # Should have both hooks + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) + assert config.has_hooks_for_event(HookEventType.STOP) + + pre_hooks = config.get_hooks_for_event( + HookEventType.PRE_TOOL_USE, "AnyTool" + ) + assert len(pre_hooks) == 1 + assert pre_hooks[0].command == "json-hook.sh" + + stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) + assert len(stop_hooks) == 1 + assert stop_hooks[0].command == "bash .openhands/stop.sh || true" + + +class TestLoadProjectHooks: + """Tests for load_project_hooks utility function.""" + + def test_load_project_hooks_no_openhands_dir(self): + """Test load_project_hooks returns empty when .openhands doesn't exist.""" + with tempfile.TemporaryDirectory() as tmpdir: + config = load_project_hooks(tmpdir) + assert config.is_empty() + + def test_load_project_hooks_empty_openhands_dir(self): + """Test load_project_hooks returns empty config when no scripts found.""" + with tempfile.TemporaryDirectory() as tmpdir: + os.makedirs(os.path.join(tmpdir, ".openhands")) + config = load_project_hooks(tmpdir) + assert config.is_empty() + + def test_load_project_hooks_discovers_all_event_types(self): + """Test load_project_hooks discovers scripts for all event types.""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + + # Create scripts for multiple event types + scripts = [ + "stop.sh", + "pre_tool_use.sh", + "post_tool_use.sh", + "session_start.sh", + "session_end.sh", + "user_prompt_submit.sh", + ] + for script in scripts: + with open(os.path.join(openhands_dir, script), "w") as f: + f.write(f"#!/bin/bash\necho {script}\n") + + config = load_project_hooks(tmpdir) + + assert config.has_hooks_for_event(HookEventType.STOP) + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) + assert config.has_hooks_for_event(HookEventType.POST_TOOL_USE) + assert config.has_hooks_for_event(HookEventType.SESSION_START) + assert config.has_hooks_for_event(HookEventType.SESSION_END) + assert config.has_hooks_for_event(HookEventType.USER_PROMPT_SUBMIT) + + def test_load_project_hooks_accepts_string_path(self): + """Test load_project_hooks accepts string paths.""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + with open(os.path.join(openhands_dir, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop\n") + + # Pass string path instead of Path object + config = load_project_hooks(str(tmpdir)) + assert config.has_hooks_for_event(HookEventType.STOP) + + +class TestDiscoverHookScripts: + """Tests for discover_hook_scripts utility function.""" + + def test_discover_hook_scripts_nonexistent_dir(self): + """Test discover_hook_scripts returns empty dict for nonexistent directory.""" + from pathlib import Path + + result = discover_hook_scripts(Path("/nonexistent/path")) + assert result == {} + + def test_discover_hook_scripts_finds_legacy_agent_finish(self): + """Test discover_hook_scripts finds agent_finish.sh and maps to STOP.""" + from pathlib import Path + + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = Path(tmpdir) / ".openhands" + openhands_dir.mkdir() + (openhands_dir / "agent_finish.sh").write_text("#!/bin/bash\necho done\n") + + result = discover_hook_scripts(openhands_dir) + + assert HookEventType.STOP in result + assert len(result[HookEventType.STOP]) == 1 + assert result[HookEventType.STOP][0].name == "agent_finish.sh" + + def test_discover_hook_scripts_finds_scripts_in_hooks_subdir(self): + """Test discover_hook_scripts searches .openhands/hooks/ subdirectory.""" + from pathlib import Path + + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = Path(tmpdir) / ".openhands" + hooks_subdir = openhands_dir / "hooks" + hooks_subdir.mkdir(parents=True) + (hooks_subdir / "pre_tool_use.sh").write_text("#!/bin/bash\necho pre\n") + + result = discover_hook_scripts(openhands_dir) + + assert HookEventType.PRE_TOOL_USE in result + assert len(result[HookEventType.PRE_TOOL_USE]) == 1 + assert result[HookEventType.PRE_TOOL_USE][0].name == "pre_tool_use.sh" From e6c9c6bc139e3b8749e3afe1a1e195c3c069be20 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 2 Feb 2026 13:19:55 +0000 Subject: [PATCH 2/6] Support passing a list of working_dir to HookConfig.load() 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-sdk/openhands/sdk/hooks/config.py | 93 ++++++++------ tests/sdk/hooks/test_config.py | 129 ++++++++++++++++++++ 2 files changed, 183 insertions(+), 39 deletions(-) diff --git a/openhands-sdk/openhands/sdk/hooks/config.py b/openhands-sdk/openhands/sdk/hooks/config.py index 7c0abe99df..a1dea4d026 100644 --- a/openhands-sdk/openhands/sdk/hooks/config.py +++ b/openhands-sdk/openhands/sdk/hooks/config.py @@ -212,7 +212,9 @@ def _normalize_hooks_input(cls, data: Any) -> Any: @classmethod def load( - cls, path: str | Path | None = None, working_dir: str | Path | None = None + cls, + path: str | Path | None = None, + working_dir: str | Path | list[str | Path] | None = None, ) -> "HookConfig": """Load config from path or search .openhands/hooks.json locations. @@ -222,48 +224,61 @@ def load( Args: path: Explicit path to hooks.json file. If provided, working_dir is ignored for JSON loading but still used for script discovery. - working_dir: Project directory for discovering .openhands/hooks.json - and hook scripts. Falls back to cwd if not provided. + working_dir: Project directory (or list of directories) for discovering + .openhands/hooks.json and hook scripts. When a list is provided, + hooks from all directories are merged. Falls back to cwd if not + provided. """ # Import here to avoid circular dependency from openhands.sdk.hooks.utils import load_project_hooks - base_dir = Path(working_dir) if working_dir else Path.cwd() - - # Load JSON config - json_config: HookConfig | None = None - config_path = path - - 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 - - if config_path is not None: - config_path = Path(config_path) - if config_path.exists(): - try: - with open(config_path) as f: - data = json.load(f) - json_config = cls.model_validate(data) - except (json.JSONDecodeError, OSError) as e: - logger.warning(f"Failed to load hooks from {config_path}: {e}") - - # Load hook scripts from .openhands directory - script_config = load_project_hooks(base_dir) - - # Merge configs: JSON config takes precedence, script hooks are appended - configs_to_merge = [] - if json_config is not None and not json_config.is_empty(): - configs_to_merge.append(json_config) - if not script_config.is_empty(): - configs_to_merge.append(script_config) + # Normalize working_dir to a list of paths + if working_dir is None: + working_dirs = [Path.cwd()] + elif isinstance(working_dir, list): + working_dirs = [Path(d) for d in working_dir] + else: + working_dirs = [Path(working_dir)] + + configs_to_merge: list[HookConfig] = [] + + for base_dir in working_dirs: + # Load JSON config for this directory + json_config: HookConfig | None = None + config_path = path + + 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 + + if config_path is not None: + config_path = Path(config_path) + if config_path.exists(): + try: + with open(config_path) as f: + data = json.load(f) + json_config = cls.model_validate(data) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to load hooks from {config_path}: {e}") + + # Load hook scripts from .openhands directory + script_config = load_project_hooks(base_dir) + + # Add configs to merge list + if json_config is not None and not json_config.is_empty(): + configs_to_merge.append(json_config) + if not script_config.is_empty(): + configs_to_merge.append(script_config) + + # Reset path for next iteration (only use explicit path for first dir) + path = None if not configs_to_merge: return cls() diff --git a/tests/sdk/hooks/test_config.py b/tests/sdk/hooks/test_config.py index 2e62828b04..418c106c69 100644 --- a/tests/sdk/hooks/test_config.py +++ b/tests/sdk/hooks/test_config.py @@ -412,3 +412,132 @@ def test_discover_hook_scripts_finds_scripts_in_hooks_subdir(self): assert HookEventType.PRE_TOOL_USE in result assert len(result[HookEventType.PRE_TOOL_USE]) == 1 assert result[HookEventType.PRE_TOOL_USE][0].name == "pre_tool_use.sh" + + +class TestMultipleWorkingDirs: + """Tests for loading hooks from multiple working directories.""" + + def test_load_with_list_of_working_dirs(self): + """Test that load() accepts a list of working directories.""" + with ( + tempfile.TemporaryDirectory() as tmpdir1, + tempfile.TemporaryDirectory() as tmpdir2, + ): + # Create hooks in first directory + openhands_dir1 = os.path.join(tmpdir1, ".openhands") + os.makedirs(openhands_dir1) + with open(os.path.join(openhands_dir1, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop1\n") + + # Create hooks in second directory + openhands_dir2 = os.path.join(tmpdir2, ".openhands") + os.makedirs(openhands_dir2) + with open(os.path.join(openhands_dir2, "pre_tool_use.sh"), "w") as f: + f.write("#!/bin/bash\necho pre\n") + + # Load from both directories + config = HookConfig.load(working_dir=[tmpdir1, tmpdir2]) + + # Should have hooks from both directories + assert config.has_hooks_for_event(HookEventType.STOP) + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) + + stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) + assert len(stop_hooks) == 1 + assert "stop.sh" in stop_hooks[0].command + + pre_hooks = config.get_hooks_for_event( + HookEventType.PRE_TOOL_USE, "AnyTool" + ) + assert len(pre_hooks) == 1 + assert "pre_tool_use.sh" in pre_hooks[0].command + + def test_load_merges_hooks_from_multiple_dirs(self): + """Test that hooks from multiple directories are merged.""" + with ( + tempfile.TemporaryDirectory() as tmpdir1, + tempfile.TemporaryDirectory() as tmpdir2, + ): + # Create stop hook in first directory + openhands_dir1 = os.path.join(tmpdir1, ".openhands") + os.makedirs(openhands_dir1) + with open(os.path.join(openhands_dir1, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop1\n") + + # Create another stop hook in second directory + openhands_dir2 = os.path.join(tmpdir2, ".openhands") + os.makedirs(openhands_dir2) + with open(os.path.join(openhands_dir2, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop2\n") + + # Load from both directories + config = HookConfig.load(working_dir=[tmpdir1, tmpdir2]) + + # Should have both stop hooks merged + stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) + assert len(stop_hooks) == 2 + + def test_load_with_json_and_scripts_from_multiple_dirs(self): + """Test loading JSON config and scripts from multiple directories.""" + with ( + tempfile.TemporaryDirectory() as tmpdir1, + tempfile.TemporaryDirectory() as tmpdir2, + ): + # Create hooks.json in first directory + openhands_dir1 = os.path.join(tmpdir1, ".openhands") + os.makedirs(openhands_dir1) + hooks_json = os.path.join(openhands_dir1, "hooks.json") + data = {"PreToolUse": [{"hooks": [{"command": "json-hook.sh"}]}]} + with open(hooks_json, "w") as f: + json.dump(data, f) + + # Create script in second directory + openhands_dir2 = os.path.join(tmpdir2, ".openhands") + os.makedirs(openhands_dir2) + with open(os.path.join(openhands_dir2, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop\n") + + # Load from both directories + config = HookConfig.load(working_dir=[tmpdir1, tmpdir2]) + + # Should have hooks from both sources + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) + assert config.has_hooks_for_event(HookEventType.STOP) + + pre_hooks = config.get_hooks_for_event( + HookEventType.PRE_TOOL_USE, "AnyTool" + ) + assert len(pre_hooks) == 1 + assert pre_hooks[0].command == "json-hook.sh" + + def test_load_with_empty_list_returns_empty_config(self): + """Test that load() with empty list returns empty config.""" + config = HookConfig.load(working_dir=[]) + assert config.is_empty() + + def test_load_with_single_item_list(self): + """Test that load() works with a single-item list.""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + with open(os.path.join(openhands_dir, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop\n") + + # Load with single-item list + config = HookConfig.load(working_dir=[tmpdir]) + + assert config.has_hooks_for_event(HookEventType.STOP) + + def test_load_with_nonexistent_dirs_in_list(self): + """Test that load() handles nonexistent directories in the list gracefully.""" + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + with open(os.path.join(openhands_dir, "stop.sh"), "w") as f: + f.write("#!/bin/bash\necho stop\n") + + # Load with mix of existing and nonexistent directories + config = HookConfig.load(working_dir=["/nonexistent/path", tmpdir]) + + # Should still load hooks from the existing directory + assert config.has_hooks_for_event(HookEventType.STOP) From f9b4cbe6c91f580483060d894ed6e6bfeb11caee Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 2 Feb 2026 13:21:06 +0000 Subject: [PATCH 3/6] refactor(hooks): remove agent_finish.sh, keep only stop.sh 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-sdk/openhands/sdk/hooks/config.py | 4 +-- openhands-sdk/openhands/sdk/hooks/utils.py | 6 +--- tests/sdk/hooks/test_config.py | 32 --------------------- 3 files changed, 3 insertions(+), 39 deletions(-) diff --git a/openhands-sdk/openhands/sdk/hooks/config.py b/openhands-sdk/openhands/sdk/hooks/config.py index a1dea4d026..2465cf8ed0 100644 --- a/openhands-sdk/openhands/sdk/hooks/config.py +++ b/openhands-sdk/openhands/sdk/hooks/config.py @@ -218,8 +218,8 @@ def load( ) -> "HookConfig": """Load config from path or search .openhands/hooks.json locations. - Also discovers hook scripts in the .openhands directory (e.g., stop.sh, - agent_finish.sh) and merges them with the JSON config. + Also discovers hook scripts in the .openhands directory (e.g., stop.sh) + and merges them with the JSON config. Args: path: Explicit path to hooks.json file. If provided, working_dir is ignored diff --git a/openhands-sdk/openhands/sdk/hooks/utils.py b/openhands-sdk/openhands/sdk/hooks/utils.py index 022b91fea2..dad0574a1a 100644 --- a/openhands-sdk/openhands/sdk/hooks/utils.py +++ b/openhands-sdk/openhands/sdk/hooks/utils.py @@ -16,17 +16,14 @@ # Mapping of script filenames to hook event types. -# Scripts can be named either by event type (e.g., stop.sh) or by legacy names. +# Scripts are named by event type (e.g., stop.sh). HOOK_SCRIPT_MAPPING: dict[str, HookEventType] = { - # Standard event-based naming (snake_case) "stop.sh": HookEventType.STOP, "pre_tool_use.sh": HookEventType.PRE_TOOL_USE, "post_tool_use.sh": HookEventType.POST_TOOL_USE, "user_prompt_submit.sh": HookEventType.USER_PROMPT_SUBMIT, "session_start.sh": HookEventType.SESSION_START, "session_end.sh": HookEventType.SESSION_END, - # Legacy naming for backward compatibility - "agent_finish.sh": HookEventType.STOP, } # Default timeout for hook scripts (10 minutes) @@ -90,7 +87,6 @@ def load_project_hooks(work_dir: str | Path) -> HookConfig: Supported script locations: - {work_dir}/.openhands/{event_type}.sh (e.g., stop.sh, pre_tool_use.sh) - {work_dir}/.openhands/hooks/{event_type}.sh - - {work_dir}/.openhands/agent_finish.sh (legacy, maps to STOP event) Args: work_dir: Path to the project/working directory. diff --git a/tests/sdk/hooks/test_config.py b/tests/sdk/hooks/test_config.py index 418c106c69..a578a0846e 100644 --- a/tests/sdk/hooks/test_config.py +++ b/tests/sdk/hooks/test_config.py @@ -232,23 +232,6 @@ def test_unknown_event_type_raises_error(self): {"PreToolExecute": [{"hooks": [{"command": "test.sh"}]}]} ) - def test_load_discovers_agent_finish_script(self): - """Test that load() discovers .openhands/agent_finish.sh (legacy naming).""" - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - finish_script = os.path.join(openhands_dir, "agent_finish.sh") - with open(finish_script, "w") as f: - f.write("#!/bin/bash\necho done\n") - - config = HookConfig.load(working_dir=tmpdir) - - assert config.has_hooks_for_event(HookEventType.STOP) - hooks = config.get_hooks_for_event(HookEventType.STOP, None) - assert len(hooks) == 1 - assert hooks[0].command == "bash .openhands/agent_finish.sh || true" - assert hooks[0].timeout == 600 - def test_load_discovers_stop_script(self): """Test that load() discovers .openhands/stop.sh (standard naming).""" with tempfile.TemporaryDirectory() as tmpdir: @@ -382,21 +365,6 @@ def test_discover_hook_scripts_nonexistent_dir(self): result = discover_hook_scripts(Path("/nonexistent/path")) assert result == {} - def test_discover_hook_scripts_finds_legacy_agent_finish(self): - """Test discover_hook_scripts finds agent_finish.sh and maps to STOP.""" - from pathlib import Path - - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = Path(tmpdir) / ".openhands" - openhands_dir.mkdir() - (openhands_dir / "agent_finish.sh").write_text("#!/bin/bash\necho done\n") - - result = discover_hook_scripts(openhands_dir) - - assert HookEventType.STOP in result - assert len(result[HookEventType.STOP]) == 1 - assert result[HookEventType.STOP][0].name == "agent_finish.sh" - def test_discover_hook_scripts_finds_scripts_in_hooks_subdir(self): """Test discover_hook_scripts searches .openhands/hooks/ subdirectory.""" from pathlib import Path From d8ab9be7e3d67e53e19a8f8dab59d38afaa32f72 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 2 Feb 2026 13:43:21 +0000 Subject: [PATCH 4/6] Refactor hook loading with _load_hook_dir helper and add relative path 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-sdk/openhands/sdk/hooks/config.py | 187 +++++++++++++++----- tests/sdk/hooks/test_config.py | 138 +++++++++++++++ 2 files changed, 279 insertions(+), 46 deletions(-) diff --git a/openhands-sdk/openhands/sdk/hooks/config.py b/openhands-sdk/openhands/sdk/hooks/config.py index 2465cf8ed0..35261801c9 100644 --- a/openhands-sdk/openhands/sdk/hooks/config.py +++ b/openhands-sdk/openhands/sdk/hooks/config.py @@ -1,17 +1,22 @@ """Hook configuration loading and management.""" +from __future__ import annotations + import json import logging import re from enum import Enum from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any from pydantic import BaseModel, Field, model_validator from openhands.sdk.hooks.types import HookEventType +if TYPE_CHECKING: + pass + logger = logging.getLogger(__name__) @@ -43,6 +48,96 @@ class HookType(str, Enum): PROMPT = "prompt" # LLM-based evaluation (future) +def _load_hook_dir( + directory: Path, + load_scripts: bool = True, +) -> HookConfig | None: + """Load hooks from a single directory. + + Loads hooks.json if it exists in the directory, and optionally discovers + hook scripts (e.g., stop.sh, pre_tool_use.sh). + + Args: + directory: Path to the directory containing .openhands/ or being the + .openhands directory itself. + load_scripts: Whether to also discover and load hook scripts. + Set to False for ~/.openhands to avoid loading scripts from there. + + Returns: + HookConfig with loaded hooks, or None if no hooks found. + """ + # Import here to avoid circular dependency + from openhands.sdk.hooks.utils import load_project_hooks + + configs_to_merge: list[HookConfig] = [] + + # Determine the hooks.json path + # If directory is already .openhands, look for hooks.json directly + # Otherwise, look in directory/.openhands/hooks.json + if directory.name == ".openhands": + hooks_json_path = directory / "hooks.json" + openhands_dir = directory + else: + hooks_json_path = directory / ".openhands" / "hooks.json" + openhands_dir = directory / ".openhands" + + # Load hooks.json if it exists + if hooks_json_path.exists(): + try: + with open(hooks_json_path) as f: + data = json.load(f) + json_config = HookConfig.model_validate(data) + if not json_config.is_empty(): + # Validate commands for potential path issues + _warn_relative_paths(json_config, hooks_json_path) + configs_to_merge.append(json_config) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to load hooks from {hooks_json_path}: {e}") + + # Load hook scripts if requested + if load_scripts and openhands_dir.exists(): + # For script loading, we need the parent directory (working dir) + work_dir = openhands_dir.parent if directory.name == ".openhands" else directory + script_config = load_project_hooks(work_dir) + if not script_config.is_empty(): + configs_to_merge.append(script_config) + + if not configs_to_merge: + return None + + if len(configs_to_merge) == 1: + return configs_to_merge[0] + + return HookConfig.merge(configs_to_merge) + + +def _warn_relative_paths(config: HookConfig, config_path: Path) -> None: + """Log warnings for commands that use relative paths. + + Relative paths in hooks.json can be problematic because the working + directory when the hook runs may not be the same as where hooks.json + was loaded from. + + Args: + config: The HookConfig to check. + config_path: Path to the hooks.json file (for logging context). + """ + for field_name in HOOK_EVENT_FIELDS: + matchers = getattr(config, field_name, []) + for matcher in matchers: + for hook in matcher.hooks: + command = hook.command + # Check if command starts with a relative path indicator + # Common patterns: ./script.sh, ../script.sh, script.sh (no path) + # We warn about ./ and ../ as they're explicitly relative + if command.startswith("./") or command.startswith("../"): + logger.warning( + f"Hook command in {config_path} uses relative path: " + f"'{command}'. Consider using absolute paths to avoid " + f"issues when the working directory changes." + ) + + class HookDefinition(BaseModel): """A single hook definition.""" @@ -215,22 +310,41 @@ def load( cls, path: str | Path | None = None, working_dir: str | Path | list[str | Path] | None = None, - ) -> "HookConfig": + ) -> HookConfig: """Load config from path or search .openhands/hooks.json locations. Also discovers hook scripts in the .openhands directory (e.g., stop.sh) and merges them with the JSON config. Args: - path: Explicit path to hooks.json file. If provided, working_dir is ignored - for JSON loading but still used for script discovery. + path: Explicit path to hooks.json file. If provided, this file is + loaded directly and working_dir is still used for script discovery. working_dir: Project directory (or list of directories) for discovering .openhands/hooks.json and hook scripts. When a list is provided, hooks from all directories are merged. Falls back to cwd if not provided. + + The method loads hooks from: + 1. Each working directory's .openhands/ (both hooks.json and scripts) + 2. ~/.openhands/hooks.json (global user config, no scripts) + + All discovered hooks are merged together. """ - # Import here to avoid circular dependency - from openhands.sdk.hooks.utils import load_project_hooks + configs_to_merge: list[HookConfig] = [] + + # Handle explicit path if provided + if path is not None: + path = Path(path) + if path.exists(): + try: + with open(path) as f: + data = json.load(f) + json_config = cls.model_validate(data) + if not json_config.is_empty(): + _warn_relative_paths(json_config, path) + configs_to_merge.append(json_config) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to load hooks from {path}: {e}") # Normalize working_dir to a list of paths if working_dir is None: @@ -240,45 +354,26 @@ def load( else: working_dirs = [Path(working_dir)] - configs_to_merge: list[HookConfig] = [] - + # Load hooks from each working directory (both hooks.json and scripts) for base_dir in working_dirs: - # Load JSON config for this directory - json_config: HookConfig | None = None - config_path = path - - 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 - - if config_path is not None: - config_path = Path(config_path) - if config_path.exists(): - try: - with open(config_path) as f: - data = json.load(f) - json_config = cls.model_validate(data) - except (json.JSONDecodeError, OSError) as e: - logger.warning(f"Failed to load hooks from {config_path}: {e}") - - # Load hook scripts from .openhands directory - script_config = load_project_hooks(base_dir) - - # Add configs to merge list - if json_config is not None and not json_config.is_empty(): - configs_to_merge.append(json_config) - if not script_config.is_empty(): - configs_to_merge.append(script_config) - - # Reset path for next iteration (only use explicit path for first dir) - path = None + dir_config = _load_hook_dir(base_dir, load_scripts=True) + if dir_config is not None: + configs_to_merge.append(dir_config) + + # Load global user config from ~/.openhands/hooks.json (no scripts) + # Only load if no explicit path was provided + if path is None: + home_openhands = Path.home() / ".openhands" + # Check if we already loaded from home directory via working_dirs + home_already_loaded = any( + wd.resolve() == home_openhands.resolve() + or wd.resolve() == home_openhands.parent.resolve() + for wd in working_dirs + ) + if not home_already_loaded: + home_config = _load_hook_dir(home_openhands, load_scripts=False) + if home_config is not None: + configs_to_merge.append(home_config) if not configs_to_merge: return cls() @@ -287,7 +382,7 @@ def load( return merged if merged is not None else cls() @classmethod - def from_dict(cls, data: dict[str, Any]) -> "HookConfig": + def from_dict(cls, data: dict[str, Any]) -> HookConfig: """Create HookConfig from a dictionary. Supports both legacy format with "hooks" wrapper and direct format: @@ -331,7 +426,7 @@ def save(self, path: str | Path) -> None: json.dump(self.model_dump(mode="json", exclude_defaults=True), f, indent=2) @classmethod - def merge(cls, configs: list["HookConfig"]) -> "HookConfig | None": + def merge(cls, configs: list[HookConfig]) -> HookConfig | None: """Merge multiple hook configs by concatenating handlers per event type. Each hook config may have multiple event types (pre_tool_use, diff --git a/tests/sdk/hooks/test_config.py b/tests/sdk/hooks/test_config.py index a578a0846e..fc416efab5 100644 --- a/tests/sdk/hooks/test_config.py +++ b/tests/sdk/hooks/test_config.py @@ -509,3 +509,141 @@ def test_load_with_nonexistent_dirs_in_list(self): # Should still load hooks from the existing directory assert config.has_hooks_for_event(HookEventType.STOP) + + +class TestRelativePathWarning: + """Tests for relative path warnings in hooks.json.""" + + def test_relative_path_warning_logged(self, caplog): + """Test that relative paths in hooks.json trigger a warning.""" + import logging + + data = { + "PreToolUse": [ + {"hooks": [{"command": "./scripts/hook.sh"}]}, + {"hooks": [{"command": "../other/hook.sh"}]}, + ] + } + + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + hooks_file = os.path.join(openhands_dir, "hooks.json") + with open(hooks_file, "w") as f: + json.dump(data, f) + + with caplog.at_level(logging.WARNING): + HookConfig.load(working_dir=tmpdir) + + # Should have warnings for both relative paths + assert "./scripts/hook.sh" in caplog.text + assert "../other/hook.sh" in caplog.text + assert "relative path" in caplog.text.lower() + + def test_absolute_path_no_warning(self, caplog): + """Test that absolute paths in hooks.json don't trigger warnings.""" + import logging + + data = {"PreToolUse": [{"hooks": [{"command": "/usr/local/bin/hook.sh"}]}]} + + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + hooks_file = os.path.join(openhands_dir, "hooks.json") + with open(hooks_file, "w") as f: + json.dump(data, f) + + with caplog.at_level(logging.WARNING): + HookConfig.load(working_dir=tmpdir) + + # Should not have any relative path warnings + assert "relative path" not in caplog.text.lower() + + def test_simple_command_no_warning(self, caplog): + """Test that simple commands (no path) don't trigger warnings.""" + import logging + + data = {"PreToolUse": [{"hooks": [{"command": "echo hello"}]}]} + + with tempfile.TemporaryDirectory() as tmpdir: + openhands_dir = os.path.join(tmpdir, ".openhands") + os.makedirs(openhands_dir) + hooks_file = os.path.join(openhands_dir, "hooks.json") + with open(hooks_file, "w") as f: + json.dump(data, f) + + with caplog.at_level(logging.WARNING): + HookConfig.load(working_dir=tmpdir) + + # Should not have any relative path warnings + assert "relative path" not in caplog.text.lower() + + +class TestHomeDirDeduplication: + """Tests for preventing duplicate loading of ~/.openhands/hooks.json.""" + + def test_home_dir_not_loaded_twice_with_multiple_working_dirs( + self, tmp_path, monkeypatch + ): + """Test ~/.openhands/hooks.json is only loaded once with multiple dirs.""" + # Create a fake home directory with hooks.json + fake_home = tmp_path / "fake_home" + fake_home.mkdir() + fake_openhands = fake_home / ".openhands" + fake_openhands.mkdir() + hooks_file = fake_openhands / "hooks.json" + hooks_file.write_text( + json.dumps({"PreToolUse": [{"hooks": [{"command": "home-hook.sh"}]}]}) + ) + + # Monkeypatch Path.home() to return our fake home + monkeypatch.setattr("pathlib.Path.home", lambda: fake_home) + + # Create two working directories without hooks + work_dir1 = tmp_path / "work1" + work_dir1.mkdir() + work_dir2 = tmp_path / "work2" + work_dir2.mkdir() + + # Load from both working directories + config = HookConfig.load(working_dir=[str(work_dir1), str(work_dir2)]) + + # Should have exactly one hook from home directory (not duplicated) + hooks = config.get_hooks_for_event(HookEventType.PRE_TOOL_USE, "AnyTool") + assert len(hooks) == 1 + assert hooks[0].command == "home-hook.sh" + + def test_home_dir_merged_with_working_dir_hooks(self, tmp_path, monkeypatch): + """Test that home dir hooks are merged with working dir hooks.""" + # Create a fake home directory with hooks.json + fake_home = tmp_path / "fake_home" + fake_home.mkdir() + fake_openhands = fake_home / ".openhands" + fake_openhands.mkdir() + home_hooks_file = fake_openhands / "hooks.json" + home_hooks_file.write_text( + json.dumps({"PreToolUse": [{"hooks": [{"command": "home-hook.sh"}]}]}) + ) + + # Monkeypatch Path.home() to return our fake home + monkeypatch.setattr("pathlib.Path.home", lambda: fake_home) + + # Create a working directory with its own hooks + work_dir = tmp_path / "work" + work_dir.mkdir() + work_openhands = work_dir / ".openhands" + work_openhands.mkdir() + work_hooks_file = work_openhands / "hooks.json" + work_hooks_file.write_text( + json.dumps({"PreToolUse": [{"hooks": [{"command": "work-hook.sh"}]}]}) + ) + + # Load from working directory + config = HookConfig.load(working_dir=str(work_dir)) + + # Should have both hooks merged + hooks = config.get_hooks_for_event(HookEventType.PRE_TOOL_USE, "AnyTool") + assert len(hooks) == 2 + commands = [h.command for h in hooks] + assert "work-hook.sh" in commands + assert "home-hook.sh" in commands From 26b8c48cd57f428dcde08f4732fd4ac1c1bf115e Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 2 Feb 2026 14:09:05 +0000 Subject: [PATCH 5/6] fix: address code review issues in hooks module - 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-sdk/openhands/sdk/hooks/config.py | 5 +---- openhands-sdk/openhands/sdk/hooks/utils.py | 16 ++++++---------- tests/sdk/hooks/test_config.py | 14 +++++++++++--- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/openhands-sdk/openhands/sdk/hooks/config.py b/openhands-sdk/openhands/sdk/hooks/config.py index 35261801c9..c89e246351 100644 --- a/openhands-sdk/openhands/sdk/hooks/config.py +++ b/openhands-sdk/openhands/sdk/hooks/config.py @@ -7,16 +7,13 @@ import re from enum import Enum from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import Any from pydantic import BaseModel, Field, model_validator from openhands.sdk.hooks.types import HookEventType -if TYPE_CHECKING: - pass - logger = logging.getLogger(__name__) diff --git a/openhands-sdk/openhands/sdk/hooks/utils.py b/openhands-sdk/openhands/sdk/hooks/utils.py index dad0574a1a..1616abefd7 100644 --- a/openhands-sdk/openhands/sdk/hooks/utils.py +++ b/openhands-sdk/openhands/sdk/hooks/utils.py @@ -30,14 +30,6 @@ DEFAULT_HOOK_SCRIPT_TIMEOUT = 600 -def _pascal_to_snake(name: str) -> str: - """Convert PascalCase to snake_case.""" - import re - - result = re.sub(r"(? dict[HookEventType, list[Path]]: """Discover hook scripts in the .openhands directory. @@ -101,6 +93,7 @@ def load_project_hooks(work_dir: str | Path) -> HookConfig: HookDefinition, HookMatcher, HookType, + _pascal_to_snake, ) if isinstance(work_dir, str): @@ -123,8 +116,11 @@ def load_project_hooks(work_dir: str | Path) -> HookConfig: for script_path in script_paths: # Use relative path from work_dir for the command relative_path = script_path.relative_to(work_dir) - # Use || true to prevent hook failures from blocking the event - command = f"bash {relative_path} || true" + # Log failures to stderr but don't block the event + command = ( + f'bash {relative_path} || {{ echo "Hook script {relative_path} ' + f'failed with exit code $?" >&2; true; }}' + ) matchers.append( HookMatcher( diff --git a/tests/sdk/hooks/test_config.py b/tests/sdk/hooks/test_config.py index fc416efab5..a5df83f87e 100644 --- a/tests/sdk/hooks/test_config.py +++ b/tests/sdk/hooks/test_config.py @@ -246,7 +246,9 @@ def test_load_discovers_stop_script(self): assert config.has_hooks_for_event(HookEventType.STOP) hooks = config.get_hooks_for_event(HookEventType.STOP, None) assert len(hooks) == 1 - assert hooks[0].command == "bash .openhands/stop.sh || true" + # Command logs failures to stderr before returning true + assert hooks[0].command.startswith("bash .openhands/stop.sh || {") + assert "failed with exit code" in hooks[0].command def test_load_discovers_scripts_in_hooks_subdir(self): """Test that load() discovers scripts in .openhands/hooks/ subdirectory.""" @@ -262,7 +264,11 @@ def test_load_discovers_scripts_in_hooks_subdir(self): assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) hooks = config.get_hooks_for_event(HookEventType.PRE_TOOL_USE, "AnyTool") assert len(hooks) == 1 - assert hooks[0].command == "bash .openhands/hooks/pre_tool_use.sh || true" + # Command logs failures to stderr before returning true + assert hooks[0].command.startswith( + "bash .openhands/hooks/pre_tool_use.sh || {" + ) + assert "failed with exit code" in hooks[0].command def test_load_merges_json_and_scripts(self): """Test that load() merges hooks.json with discovered scripts.""" @@ -295,7 +301,9 @@ def test_load_merges_json_and_scripts(self): stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) assert len(stop_hooks) == 1 - assert stop_hooks[0].command == "bash .openhands/stop.sh || true" + # Command logs failures to stderr before returning true + assert stop_hooks[0].command.startswith("bash .openhands/stop.sh || {") + assert "failed with exit code" in stop_hooks[0].command class TestLoadProjectHooks: From 7003fdff589b011a8a308e2ec3b622286b5bfe22 Mon Sep 17 00:00:00 2001 From: openhands Date: Fri, 6 Feb 2026 03:38:19 +0000 Subject: [PATCH 6/6] refactor(hooks): simplify PR to only support loading hooks from multiple 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 --- openhands-sdk/openhands/sdk/hooks/__init__.py | 2 - openhands-sdk/openhands/sdk/hooks/config.py | 148 ++------ openhands-sdk/openhands/sdk/hooks/utils.py | 143 -------- tests/sdk/hooks/test_config.py | 318 ++---------------- 4 files changed, 67 insertions(+), 544 deletions(-) delete mode 100644 openhands-sdk/openhands/sdk/hooks/utils.py diff --git a/openhands-sdk/openhands/sdk/hooks/__init__.py b/openhands-sdk/openhands/sdk/hooks/__init__.py index d81c8f990e..f76b8d38af 100644 --- a/openhands-sdk/openhands/sdk/hooks/__init__.py +++ b/openhands-sdk/openhands/sdk/hooks/__init__.py @@ -19,7 +19,6 @@ from openhands.sdk.hooks.executor import HookExecutor, HookResult from openhands.sdk.hooks.manager import HookManager from openhands.sdk.hooks.types import HookDecision, HookEvent, HookEventType -from openhands.sdk.hooks.utils import load_project_hooks __all__ = [ @@ -36,5 +35,4 @@ "HookDecision", "HookEventProcessor", "create_hook_callback", - "load_project_hooks", ] diff --git a/openhands-sdk/openhands/sdk/hooks/config.py b/openhands-sdk/openhands/sdk/hooks/config.py index c89e246351..18d26cfbec 100644 --- a/openhands-sdk/openhands/sdk/hooks/config.py +++ b/openhands-sdk/openhands/sdk/hooks/config.py @@ -45,96 +45,6 @@ class HookType(str, Enum): PROMPT = "prompt" # LLM-based evaluation (future) -def _load_hook_dir( - directory: Path, - load_scripts: bool = True, -) -> HookConfig | None: - """Load hooks from a single directory. - - Loads hooks.json if it exists in the directory, and optionally discovers - hook scripts (e.g., stop.sh, pre_tool_use.sh). - - Args: - directory: Path to the directory containing .openhands/ or being the - .openhands directory itself. - load_scripts: Whether to also discover and load hook scripts. - Set to False for ~/.openhands to avoid loading scripts from there. - - Returns: - HookConfig with loaded hooks, or None if no hooks found. - """ - # Import here to avoid circular dependency - from openhands.sdk.hooks.utils import load_project_hooks - - configs_to_merge: list[HookConfig] = [] - - # Determine the hooks.json path - # If directory is already .openhands, look for hooks.json directly - # Otherwise, look in directory/.openhands/hooks.json - if directory.name == ".openhands": - hooks_json_path = directory / "hooks.json" - openhands_dir = directory - else: - hooks_json_path = directory / ".openhands" / "hooks.json" - openhands_dir = directory / ".openhands" - - # Load hooks.json if it exists - if hooks_json_path.exists(): - try: - with open(hooks_json_path) as f: - data = json.load(f) - json_config = HookConfig.model_validate(data) - if not json_config.is_empty(): - # Validate commands for potential path issues - _warn_relative_paths(json_config, hooks_json_path) - configs_to_merge.append(json_config) - except (json.JSONDecodeError, OSError) as e: - logger.warning(f"Failed to load hooks from {hooks_json_path}: {e}") - - # Load hook scripts if requested - if load_scripts and openhands_dir.exists(): - # For script loading, we need the parent directory (working dir) - work_dir = openhands_dir.parent if directory.name == ".openhands" else directory - script_config = load_project_hooks(work_dir) - if not script_config.is_empty(): - configs_to_merge.append(script_config) - - if not configs_to_merge: - return None - - if len(configs_to_merge) == 1: - return configs_to_merge[0] - - return HookConfig.merge(configs_to_merge) - - -def _warn_relative_paths(config: HookConfig, config_path: Path) -> None: - """Log warnings for commands that use relative paths. - - Relative paths in hooks.json can be problematic because the working - directory when the hook runs may not be the same as where hooks.json - was loaded from. - - Args: - config: The HookConfig to check. - config_path: Path to the hooks.json file (for logging context). - """ - for field_name in HOOK_EVENT_FIELDS: - matchers = getattr(config, field_name, []) - for matcher in matchers: - for hook in matcher.hooks: - command = hook.command - # Check if command starts with a relative path indicator - # Common patterns: ./script.sh, ../script.sh, script.sh (no path) - # We warn about ./ and ../ as they're explicitly relative - if command.startswith("./") or command.startswith("../"): - logger.warning( - f"Hook command in {config_path} uses relative path: " - f"'{command}'. Consider using absolute paths to avoid " - f"issues when the working directory changes." - ) - - class HookDefinition(BaseModel): """A single hook definition.""" @@ -310,20 +220,17 @@ def load( ) -> HookConfig: """Load config from path or search .openhands/hooks.json locations. - Also discovers hook scripts in the .openhands directory (e.g., stop.sh) - and merges them with the JSON config. - Args: - path: Explicit path to hooks.json file. If provided, this file is - loaded directly and working_dir is still used for script discovery. + path: Explicit path to hooks.json file. If provided, only this file + is loaded (working_dir is still used for additional configs). working_dir: Project directory (or list of directories) for discovering - .openhands/hooks.json and hook scripts. When a list is provided, - hooks from all directories are merged. Falls back to cwd if not - provided. + .openhands/hooks.json. When a list is provided, hooks from all + directories are merged. Falls back to cwd if not provided. The method loads hooks from: - 1. Each working directory's .openhands/ (both hooks.json and scripts) - 2. ~/.openhands/hooks.json (global user config, no scripts) + 1. The explicit path if provided + 2. Each working directory's .openhands/hooks.json + 3. ~/.openhands/hooks.json (global user config) All discovered hooks are merged together. """ @@ -338,7 +245,6 @@ def load( data = json.load(f) json_config = cls.model_validate(data) if not json_config.is_empty(): - _warn_relative_paths(json_config, path) configs_to_merge.append(json_config) except (json.JSONDecodeError, OSError) as e: logger.warning(f"Failed to load hooks from {path}: {e}") @@ -351,30 +257,46 @@ def load( else: working_dirs = [Path(working_dir)] - # Load hooks from each working directory (both hooks.json and scripts) + # Load hooks.json from each working directory for base_dir in working_dirs: - dir_config = _load_hook_dir(base_dir, load_scripts=True) - if dir_config is not None: - configs_to_merge.append(dir_config) + hooks_json_path = base_dir / ".openhands" / "hooks.json" + if hooks_json_path.exists(): + try: + with open(hooks_json_path) as f: + data = json.load(f) + dir_config = cls.model_validate(data) + if not dir_config.is_empty(): + configs_to_merge.append(dir_config) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to load hooks from {hooks_json_path}: {e}") - # Load global user config from ~/.openhands/hooks.json (no scripts) + # Load global user config from ~/.openhands/hooks.json # Only load if no explicit path was provided if path is None: - home_openhands = Path.home() / ".openhands" + home_hooks_path = Path.home() / ".openhands" / "hooks.json" # Check if we already loaded from home directory via working_dirs home_already_loaded = any( - wd.resolve() == home_openhands.resolve() - or wd.resolve() == home_openhands.parent.resolve() + (wd / ".openhands" / "hooks.json").resolve() + == home_hooks_path.resolve() for wd in working_dirs + if (wd / ".openhands" / "hooks.json").exists() ) - if not home_already_loaded: - home_config = _load_hook_dir(home_openhands, load_scripts=False) - if home_config is not None: - configs_to_merge.append(home_config) + if not home_already_loaded and home_hooks_path.exists(): + try: + with open(home_hooks_path) as f: + data = json.load(f) + home_config = cls.model_validate(data) + if not home_config.is_empty(): + configs_to_merge.append(home_config) + except (json.JSONDecodeError, OSError) as e: + logger.warning(f"Failed to load hooks from {home_hooks_path}: {e}") if not configs_to_merge: return cls() + if len(configs_to_merge) == 1: + return configs_to_merge[0] + merged = cls.merge(configs_to_merge) return merged if merged is not None else cls() diff --git a/openhands-sdk/openhands/sdk/hooks/utils.py b/openhands-sdk/openhands/sdk/hooks/utils.py deleted file mode 100644 index 1616abefd7..0000000000 --- a/openhands-sdk/openhands/sdk/hooks/utils.py +++ /dev/null @@ -1,143 +0,0 @@ -"""Utility functions for hook loading and management.""" - -from __future__ import annotations - -import logging -from pathlib import Path -from typing import TYPE_CHECKING - -from openhands.sdk.hooks.types import HookEventType - - -if TYPE_CHECKING: - from openhands.sdk.hooks.config import HookConfig - -logger = logging.getLogger(__name__) - - -# Mapping of script filenames to hook event types. -# Scripts are named by event type (e.g., stop.sh). -HOOK_SCRIPT_MAPPING: dict[str, HookEventType] = { - "stop.sh": HookEventType.STOP, - "pre_tool_use.sh": HookEventType.PRE_TOOL_USE, - "post_tool_use.sh": HookEventType.POST_TOOL_USE, - "user_prompt_submit.sh": HookEventType.USER_PROMPT_SUBMIT, - "session_start.sh": HookEventType.SESSION_START, - "session_end.sh": HookEventType.SESSION_END, -} - -# Default timeout for hook scripts (10 minutes) -DEFAULT_HOOK_SCRIPT_TIMEOUT = 600 - - -def discover_hook_scripts(openhands_dir: Path) -> dict[HookEventType, list[Path]]: - """Discover hook scripts in the .openhands directory. - - Searches for executable shell scripts that match known hook event types. - Scripts can be placed directly in .openhands/ or in .openhands/hooks/. - - Args: - openhands_dir: Path to the .openhands directory. - - Returns: - Dictionary mapping HookEventType to list of script paths found. - """ - discovered: dict[HookEventType, list[Path]] = {} - - if not openhands_dir.exists() or not openhands_dir.is_dir(): - return discovered - - # Search locations: .openhands/ and .openhands/hooks/ - search_dirs = [openhands_dir] - hooks_subdir = openhands_dir / "hooks" - if hooks_subdir.exists() and hooks_subdir.is_dir(): - search_dirs.append(hooks_subdir) - - for search_dir in search_dirs: - for script_name, event_type in HOOK_SCRIPT_MAPPING.items(): - script_path = search_dir / script_name - if script_path.exists() and script_path.is_file(): - if event_type not in discovered: - discovered[event_type] = [] - # Avoid duplicates (same script found in multiple locations) - if script_path not in discovered[event_type]: - discovered[event_type].append(script_path) - logger.debug( - f"Discovered hook script: {script_path} -> {event_type.value}" - ) - - return discovered - - -def load_project_hooks(work_dir: str | Path) -> HookConfig: - """Load hooks from project-specific files in the .openhands directory. - - Discovers hook scripts in {work_dir}/.openhands/ and creates a HookConfig - with the appropriate hook definitions. This is similar to load_project_skills - but for hooks. - - Supported script locations: - - {work_dir}/.openhands/{event_type}.sh (e.g., stop.sh, pre_tool_use.sh) - - {work_dir}/.openhands/hooks/{event_type}.sh - - Args: - work_dir: Path to the project/working directory. - - Returns: - HookConfig with hooks discovered from script files. - Returns empty HookConfig if no hooks found. - """ - # Import here to avoid circular dependency - from openhands.sdk.hooks.config import ( - HookConfig, - HookDefinition, - HookMatcher, - HookType, - _pascal_to_snake, - ) - - if isinstance(work_dir, str): - work_dir = Path(work_dir) - - openhands_dir = work_dir / ".openhands" - discovered_scripts = discover_hook_scripts(openhands_dir) - - if not discovered_scripts: - logger.debug(f"No hook scripts found in {openhands_dir}") - return HookConfig() - - # Build hook config from discovered scripts - hook_data: dict[str, list[HookMatcher]] = {} - - for event_type, script_paths in discovered_scripts.items(): - field_name = _pascal_to_snake(event_type.value) - matchers: list[HookMatcher] = [] - - for script_path in script_paths: - # Use relative path from work_dir for the command - relative_path = script_path.relative_to(work_dir) - # Log failures to stderr but don't block the event - command = ( - f'bash {relative_path} || {{ echo "Hook script {relative_path} ' - f'failed with exit code $?" >&2; true; }}' - ) - - matchers.append( - HookMatcher( - matcher="*", - hooks=[ - HookDefinition( - type=HookType.COMMAND, - command=command, - timeout=DEFAULT_HOOK_SCRIPT_TIMEOUT, - ) - ], - ) - ) - - if matchers: - hook_data[field_name] = matchers - - num_scripts = sum(len(m) for m in hook_data.values()) - logger.debug(f"Loaded {num_scripts} hook scripts from {openhands_dir}") - return HookConfig(**hook_data) diff --git a/tests/sdk/hooks/test_config.py b/tests/sdk/hooks/test_config.py index a5df83f87e..f744fb7f21 100644 --- a/tests/sdk/hooks/test_config.py +++ b/tests/sdk/hooks/test_config.py @@ -6,7 +6,6 @@ from openhands.sdk.hooks.config import HookConfig, HookDefinition, HookMatcher from openhands.sdk.hooks.types import HookEventType -from openhands.sdk.hooks.utils import discover_hook_scripts, load_project_hooks class TestHookMatcher: @@ -232,163 +231,6 @@ def test_unknown_event_type_raises_error(self): {"PreToolExecute": [{"hooks": [{"command": "test.sh"}]}]} ) - def test_load_discovers_stop_script(self): - """Test that load() discovers .openhands/stop.sh (standard naming).""" - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - stop_script = os.path.join(openhands_dir, "stop.sh") - with open(stop_script, "w") as f: - f.write("#!/bin/bash\necho stopping\n") - - config = HookConfig.load(working_dir=tmpdir) - - assert config.has_hooks_for_event(HookEventType.STOP) - hooks = config.get_hooks_for_event(HookEventType.STOP, None) - assert len(hooks) == 1 - # Command logs failures to stderr before returning true - assert hooks[0].command.startswith("bash .openhands/stop.sh || {") - assert "failed with exit code" in hooks[0].command - - def test_load_discovers_scripts_in_hooks_subdir(self): - """Test that load() discovers scripts in .openhands/hooks/ subdirectory.""" - with tempfile.TemporaryDirectory() as tmpdir: - hooks_dir = os.path.join(tmpdir, ".openhands", "hooks") - os.makedirs(hooks_dir) - pre_tool_script = os.path.join(hooks_dir, "pre_tool_use.sh") - with open(pre_tool_script, "w") as f: - f.write("#!/bin/bash\necho pre-tool\n") - - config = HookConfig.load(working_dir=tmpdir) - - assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) - hooks = config.get_hooks_for_event(HookEventType.PRE_TOOL_USE, "AnyTool") - assert len(hooks) == 1 - # Command logs failures to stderr before returning true - assert hooks[0].command.startswith( - "bash .openhands/hooks/pre_tool_use.sh || {" - ) - assert "failed with exit code" in hooks[0].command - - def test_load_merges_json_and_scripts(self): - """Test that load() merges hooks.json with discovered scripts.""" - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - - # Create hooks.json with pre_tool_use hook - hooks_json = os.path.join(openhands_dir, "hooks.json") - data = {"PreToolUse": [{"hooks": [{"command": "json-hook.sh"}]}]} - with open(hooks_json, "w") as f: - json.dump(data, f) - - # Create stop.sh script - stop_script = os.path.join(openhands_dir, "stop.sh") - with open(stop_script, "w") as f: - f.write("#!/bin/bash\necho stop\n") - - config = HookConfig.load(working_dir=tmpdir) - - # Should have both hooks - assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) - assert config.has_hooks_for_event(HookEventType.STOP) - - pre_hooks = config.get_hooks_for_event( - HookEventType.PRE_TOOL_USE, "AnyTool" - ) - assert len(pre_hooks) == 1 - assert pre_hooks[0].command == "json-hook.sh" - - stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) - assert len(stop_hooks) == 1 - # Command logs failures to stderr before returning true - assert stop_hooks[0].command.startswith("bash .openhands/stop.sh || {") - assert "failed with exit code" in stop_hooks[0].command - - -class TestLoadProjectHooks: - """Tests for load_project_hooks utility function.""" - - def test_load_project_hooks_no_openhands_dir(self): - """Test load_project_hooks returns empty when .openhands doesn't exist.""" - with tempfile.TemporaryDirectory() as tmpdir: - config = load_project_hooks(tmpdir) - assert config.is_empty() - - def test_load_project_hooks_empty_openhands_dir(self): - """Test load_project_hooks returns empty config when no scripts found.""" - with tempfile.TemporaryDirectory() as tmpdir: - os.makedirs(os.path.join(tmpdir, ".openhands")) - config = load_project_hooks(tmpdir) - assert config.is_empty() - - def test_load_project_hooks_discovers_all_event_types(self): - """Test load_project_hooks discovers scripts for all event types.""" - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - - # Create scripts for multiple event types - scripts = [ - "stop.sh", - "pre_tool_use.sh", - "post_tool_use.sh", - "session_start.sh", - "session_end.sh", - "user_prompt_submit.sh", - ] - for script in scripts: - with open(os.path.join(openhands_dir, script), "w") as f: - f.write(f"#!/bin/bash\necho {script}\n") - - config = load_project_hooks(tmpdir) - - assert config.has_hooks_for_event(HookEventType.STOP) - assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) - assert config.has_hooks_for_event(HookEventType.POST_TOOL_USE) - assert config.has_hooks_for_event(HookEventType.SESSION_START) - assert config.has_hooks_for_event(HookEventType.SESSION_END) - assert config.has_hooks_for_event(HookEventType.USER_PROMPT_SUBMIT) - - def test_load_project_hooks_accepts_string_path(self): - """Test load_project_hooks accepts string paths.""" - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - with open(os.path.join(openhands_dir, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop\n") - - # Pass string path instead of Path object - config = load_project_hooks(str(tmpdir)) - assert config.has_hooks_for_event(HookEventType.STOP) - - -class TestDiscoverHookScripts: - """Tests for discover_hook_scripts utility function.""" - - def test_discover_hook_scripts_nonexistent_dir(self): - """Test discover_hook_scripts returns empty dict for nonexistent directory.""" - from pathlib import Path - - result = discover_hook_scripts(Path("/nonexistent/path")) - assert result == {} - - def test_discover_hook_scripts_finds_scripts_in_hooks_subdir(self): - """Test discover_hook_scripts searches .openhands/hooks/ subdirectory.""" - from pathlib import Path - - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = Path(tmpdir) / ".openhands" - hooks_subdir = openhands_dir / "hooks" - hooks_subdir.mkdir(parents=True) - (hooks_subdir / "pre_tool_use.sh").write_text("#!/bin/bash\necho pre\n") - - result = discover_hook_scripts(openhands_dir) - - assert HookEventType.PRE_TOOL_USE in result - assert len(result[HookEventType.PRE_TOOL_USE]) == 1 - assert result[HookEventType.PRE_TOOL_USE][0].name == "pre_tool_use.sh" - class TestMultipleWorkingDirs: """Tests for loading hooks from multiple working directories.""" @@ -399,34 +241,36 @@ def test_load_with_list_of_working_dirs(self): tempfile.TemporaryDirectory() as tmpdir1, tempfile.TemporaryDirectory() as tmpdir2, ): - # Create hooks in first directory + # Create hooks.json in first directory openhands_dir1 = os.path.join(tmpdir1, ".openhands") os.makedirs(openhands_dir1) - with open(os.path.join(openhands_dir1, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop1\n") + with open(os.path.join(openhands_dir1, "hooks.json"), "w") as f: + json.dump({"PreToolUse": [{"hooks": [{"command": "hook1.sh"}]}]}, f) - # Create hooks in second directory + # Create hooks.json in second directory openhands_dir2 = os.path.join(tmpdir2, ".openhands") os.makedirs(openhands_dir2) - with open(os.path.join(openhands_dir2, "pre_tool_use.sh"), "w") as f: - f.write("#!/bin/bash\necho pre\n") + with open(os.path.join(openhands_dir2, "hooks.json"), "w") as f: + json.dump({"PostToolUse": [{"hooks": [{"command": "hook2.sh"}]}]}, f) # Load from both directories config = HookConfig.load(working_dir=[tmpdir1, tmpdir2]) # Should have hooks from both directories - assert config.has_hooks_for_event(HookEventType.STOP) assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) - - stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) - assert len(stop_hooks) == 1 - assert "stop.sh" in stop_hooks[0].command + assert config.has_hooks_for_event(HookEventType.POST_TOOL_USE) pre_hooks = config.get_hooks_for_event( HookEventType.PRE_TOOL_USE, "AnyTool" ) assert len(pre_hooks) == 1 - assert "pre_tool_use.sh" in pre_hooks[0].command + assert pre_hooks[0].command == "hook1.sh" + + post_hooks = config.get_hooks_for_event( + HookEventType.POST_TOOL_USE, "AnyTool" + ) + assert len(post_hooks) == 1 + assert post_hooks[0].command == "hook2.sh" def test_load_merges_hooks_from_multiple_dirs(self): """Test that hooks from multiple directories are merged.""" @@ -434,57 +278,27 @@ def test_load_merges_hooks_from_multiple_dirs(self): tempfile.TemporaryDirectory() as tmpdir1, tempfile.TemporaryDirectory() as tmpdir2, ): - # Create stop hook in first directory + # Create PreToolUse hook in first directory openhands_dir1 = os.path.join(tmpdir1, ".openhands") os.makedirs(openhands_dir1) - with open(os.path.join(openhands_dir1, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop1\n") + with open(os.path.join(openhands_dir1, "hooks.json"), "w") as f: + json.dump({"PreToolUse": [{"hooks": [{"command": "hook1.sh"}]}]}, f) - # Create another stop hook in second directory + # Create another PreToolUse hook in second directory openhands_dir2 = os.path.join(tmpdir2, ".openhands") os.makedirs(openhands_dir2) - with open(os.path.join(openhands_dir2, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop2\n") + with open(os.path.join(openhands_dir2, "hooks.json"), "w") as f: + json.dump({"PreToolUse": [{"hooks": [{"command": "hook2.sh"}]}]}, f) # Load from both directories config = HookConfig.load(working_dir=[tmpdir1, tmpdir2]) - # Should have both stop hooks merged - stop_hooks = config.get_hooks_for_event(HookEventType.STOP, None) - assert len(stop_hooks) == 2 - - def test_load_with_json_and_scripts_from_multiple_dirs(self): - """Test loading JSON config and scripts from multiple directories.""" - with ( - tempfile.TemporaryDirectory() as tmpdir1, - tempfile.TemporaryDirectory() as tmpdir2, - ): - # Create hooks.json in first directory - openhands_dir1 = os.path.join(tmpdir1, ".openhands") - os.makedirs(openhands_dir1) - hooks_json = os.path.join(openhands_dir1, "hooks.json") - data = {"PreToolUse": [{"hooks": [{"command": "json-hook.sh"}]}]} - with open(hooks_json, "w") as f: - json.dump(data, f) - - # Create script in second directory - openhands_dir2 = os.path.join(tmpdir2, ".openhands") - os.makedirs(openhands_dir2) - with open(os.path.join(openhands_dir2, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop\n") - - # Load from both directories - config = HookConfig.load(working_dir=[tmpdir1, tmpdir2]) - - # Should have hooks from both sources - assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) - assert config.has_hooks_for_event(HookEventType.STOP) - - pre_hooks = config.get_hooks_for_event( - HookEventType.PRE_TOOL_USE, "AnyTool" - ) - assert len(pre_hooks) == 1 - assert pre_hooks[0].command == "json-hook.sh" + # Should have both hooks merged + hooks = config.get_hooks_for_event(HookEventType.PRE_TOOL_USE, "AnyTool") + assert len(hooks) == 2 + commands = [h.command for h in hooks] + assert "hook1.sh" in commands + assert "hook2.sh" in commands def test_load_with_empty_list_returns_empty_config(self): """Test that load() with empty list returns empty config.""" @@ -496,95 +310,27 @@ def test_load_with_single_item_list(self): with tempfile.TemporaryDirectory() as tmpdir: openhands_dir = os.path.join(tmpdir, ".openhands") os.makedirs(openhands_dir) - with open(os.path.join(openhands_dir, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop\n") + with open(os.path.join(openhands_dir, "hooks.json"), "w") as f: + json.dump({"PreToolUse": [{"hooks": [{"command": "hook.sh"}]}]}, f) # Load with single-item list config = HookConfig.load(working_dir=[tmpdir]) - assert config.has_hooks_for_event(HookEventType.STOP) + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) def test_load_with_nonexistent_dirs_in_list(self): """Test that load() handles nonexistent directories in the list gracefully.""" with tempfile.TemporaryDirectory() as tmpdir: openhands_dir = os.path.join(tmpdir, ".openhands") os.makedirs(openhands_dir) - with open(os.path.join(openhands_dir, "stop.sh"), "w") as f: - f.write("#!/bin/bash\necho stop\n") + with open(os.path.join(openhands_dir, "hooks.json"), "w") as f: + json.dump({"PreToolUse": [{"hooks": [{"command": "hook.sh"}]}]}, f) # Load with mix of existing and nonexistent directories config = HookConfig.load(working_dir=["/nonexistent/path", tmpdir]) # Should still load hooks from the existing directory - assert config.has_hooks_for_event(HookEventType.STOP) - - -class TestRelativePathWarning: - """Tests for relative path warnings in hooks.json.""" - - def test_relative_path_warning_logged(self, caplog): - """Test that relative paths in hooks.json trigger a warning.""" - import logging - - data = { - "PreToolUse": [ - {"hooks": [{"command": "./scripts/hook.sh"}]}, - {"hooks": [{"command": "../other/hook.sh"}]}, - ] - } - - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - hooks_file = os.path.join(openhands_dir, "hooks.json") - with open(hooks_file, "w") as f: - json.dump(data, f) - - with caplog.at_level(logging.WARNING): - HookConfig.load(working_dir=tmpdir) - - # Should have warnings for both relative paths - assert "./scripts/hook.sh" in caplog.text - assert "../other/hook.sh" in caplog.text - assert "relative path" in caplog.text.lower() - - def test_absolute_path_no_warning(self, caplog): - """Test that absolute paths in hooks.json don't trigger warnings.""" - import logging - - data = {"PreToolUse": [{"hooks": [{"command": "/usr/local/bin/hook.sh"}]}]} - - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - hooks_file = os.path.join(openhands_dir, "hooks.json") - with open(hooks_file, "w") as f: - json.dump(data, f) - - with caplog.at_level(logging.WARNING): - HookConfig.load(working_dir=tmpdir) - - # Should not have any relative path warnings - assert "relative path" not in caplog.text.lower() - - def test_simple_command_no_warning(self, caplog): - """Test that simple commands (no path) don't trigger warnings.""" - import logging - - data = {"PreToolUse": [{"hooks": [{"command": "echo hello"}]}]} - - with tempfile.TemporaryDirectory() as tmpdir: - openhands_dir = os.path.join(tmpdir, ".openhands") - os.makedirs(openhands_dir) - hooks_file = os.path.join(openhands_dir, "hooks.json") - with open(hooks_file, "w") as f: - json.dump(data, f) - - with caplog.at_level(logging.WARNING): - HookConfig.load(working_dir=tmpdir) - - # Should not have any relative path warnings - assert "relative path" not in caplog.text.lower() + assert config.has_hooks_for_event(HookEventType.PRE_TOOL_USE) class TestHomeDirDeduplication: