refactor(examples): use .from_dict() for hooks example #1746
Conversation
…yped approach - Changed hooks example to use HookConfig.from_dict() with untyped dictionary - Commented out the typed approach (HookMatcher/HookDefinition) for reference - Both approaches are valid, but .from_dict() demonstrates backward compatibility Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This PR has some pedagogical and usability concerns. The main issue is that it demonstrates the non-recommended approach as the primary example, which contradicts best practices for educational code. See inline comments for details.
|
|
||
| from openhands.sdk import LLM, Conversation | ||
| from openhands.sdk.hooks import HookConfig, HookDefinition, HookMatcher | ||
| from openhands.sdk.hooks import HookConfig |
There was a problem hiding this comment.
🟠 Important: The imports for HookMatcher and HookDefinition have been removed, but they are still referenced in the commented code (lines 108-153). If a user tries to uncomment the "recommended" typed approach, they will get NameError exceptions.
Consider keeping these imports or adding a comment at line 104 instructing users to uncomment the imports if they want to use the typed approach:
| from openhands.sdk.hooks import HookConfig | |
| from openhands.sdk.hooks import HookConfig, HookDefinition, HookMatcher |
| # Configure ALL hook types using .from_dict() with untyped dictionary | ||
| hook_config = HookConfig.from_dict( | ||
| { | ||
| "hooks": { | ||
| "PreToolUse": [ | ||
| { | ||
| "matcher": "terminal", | ||
| "hooks": [ | ||
| { | ||
| "command": str(SCRIPT_DIR / "block_dangerous.sh"), | ||
| "timeout": 10, | ||
| } | ||
| ], | ||
| } | ||
| ], | ||
| ) | ||
| ], | ||
| post_tool_use=[ | ||
| HookMatcher( | ||
| matcher="*", | ||
| hooks=[ | ||
| HookDefinition( | ||
| command=(f"LOG_FILE={log_file} {SCRIPT_DIR / 'log_tools.sh'}"), | ||
| timeout=5, | ||
| ) | ||
| "PostToolUse": [ | ||
| { | ||
| "matcher": "*", | ||
| "hooks": [ | ||
| { | ||
| "command": ( | ||
| f"LOG_FILE={log_file} {SCRIPT_DIR / 'log_tools.sh'}" | ||
| ), | ||
| "timeout": 5, | ||
| } | ||
| ], | ||
| } | ||
| ], | ||
| ) | ||
| ], | ||
| user_prompt_submit=[ | ||
| HookMatcher( | ||
| hooks=[ | ||
| HookDefinition( | ||
| command=str(SCRIPT_DIR / "inject_git_context.sh"), | ||
| ) | ||
| "UserPromptSubmit": [ | ||
| { | ||
| "hooks": [ | ||
| { | ||
| "command": str(SCRIPT_DIR / "inject_git_context.sh"), | ||
| } | ||
| ], | ||
| } | ||
| ], | ||
| ) | ||
| ], | ||
| stop=[ | ||
| HookMatcher( | ||
| hooks=[ | ||
| HookDefinition( | ||
| command=( | ||
| f"SUMMARY_FILE={summary_file} " | ||
| f"{SCRIPT_DIR / 'require_summary.sh'}" | ||
| ), | ||
| ) | ||
| "Stop": [ | ||
| { | ||
| "hooks": [ | ||
| { | ||
| "command": ( | ||
| f"SUMMARY_FILE={summary_file} " | ||
| f"{SCRIPT_DIR / 'require_summary.sh'}" | ||
| ), | ||
| } | ||
| ], | ||
| } | ||
| ], | ||
| ) | ||
| ], | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| # Alternative: Typed approach (commented out) | ||
| # This is the recommended approach for better type safety and IDE support |
There was a problem hiding this comment.
🟠 Important: This creates a confusing learning experience. The active example (lines 51-102) uses the untyped .from_dict() approach, while the comment at line 105 states the typed approach is "recommended for better type safety and IDE support."
Examples should demonstrate best practices, not legacy patterns. Users learning the SDK will naturally copy the active code, not the commented-out version.
Recommendation: Reverse this - make the typed approach the primary example and either:
- Remove the
.from_dict()version entirely, or - Show it as a brief commented alternative with a note like "For backward compatibility with JSON config files, you can also use
HookConfig.from_dict({...})"
This aligns with the PR description which states "the typed approach is the recommended one for better type safety" - so it should be the one actually demonstrated.
| # Alternative: Typed approach (commented out) | ||
| # This is the recommended approach for better type safety and IDE support | ||
| # hook_config = HookConfig( | ||
| # pre_tool_use=[ | ||
| # HookMatcher( | ||
| # matcher="terminal", | ||
| # hooks=[ | ||
| # HookDefinition( | ||
| # command=str(SCRIPT_DIR / "block_dangerous.sh"), | ||
| # timeout=10, | ||
| # ) | ||
| # ], | ||
| # ) | ||
| # ], | ||
| # post_tool_use=[ | ||
| # HookMatcher( | ||
| # matcher="*", | ||
| # hooks=[ | ||
| # HookDefinition( | ||
| # command=( | ||
| # f"LOG_FILE={log_file} " | ||
| # f"{SCRIPT_DIR / 'log_tools.sh'}" | ||
| # ), | ||
| # timeout=5, | ||
| # ) | ||
| # ], | ||
| # ) | ||
| # ], | ||
| # user_prompt_submit=[ | ||
| # HookMatcher( | ||
| # hooks=[ | ||
| # HookDefinition( | ||
| # command=str(SCRIPT_DIR / "inject_git_context.sh"), | ||
| # ) | ||
| # ], | ||
| # ) | ||
| # ], | ||
| # stop=[ | ||
| # HookMatcher( | ||
| # hooks=[ | ||
| # HookDefinition( | ||
| # command=( | ||
| # f"SUMMARY_FILE={summary_file} " | ||
| # f"{SCRIPT_DIR / 'require_summary.sh'}" | ||
| # ), | ||
| # ) | ||
| # ], | ||
| # ) | ||
| # ], | ||
| # ) |
There was a problem hiding this comment.
🟡 Suggestion: Having two complete versions of the same configuration (50 lines each) creates significant maintenance burden and makes the example unnecessarily long. If the hook API changes, both versions need updating.
Consider these alternatives:
- Preferred: Show only the typed approach as the primary example (since it's recommended)
- Add a brief comment or docstring explaining that
.from_dict()exists for JSON config file compatibility - If both patterns must be shown in full detail, consider separate example files (e.g.,
33_hooks_typed.pyand33_hooks_from_dict.py)
There was a problem hiding this comment.
@OpenHands please do show only the typed approach as the primary example (since it's recommended)
AND add a brief comment explaining that .from_dict() (maybe just use one dict with only ONE hook matcher as example.
There was a problem hiding this comment.
Uh oh! There was an unexpected error starting the job :(
| } | ||
| ) | ||
|
|
||
| # Alternative: Typed approach (commented out) |
There was a problem hiding this comment.
🟢 Nit: This comment is contradictory. "Alternative" suggests it's a secondary option, but line 105 says it's "the recommended approach." Consider clearer labeling:
| # Alternative: Typed approach (commented out) | |
| # Typed approach (RECOMMENDED for type safety and IDE support) |
Though ideally, the recommended approach should be the active code, not commented out.
Summary
This PR refactors the hooks example (#33_hooks) to use the untyped
HookConfig.from_dict()approach instead of the typed Pydantic fields, while preserving the typed approach in comments for reference.Changes
HookConfig.from_dict()with an untyped dictionaryHookMatcherandHookDefinition) for referenceContext
This is a follow-up to PR #1726, which added typed Pydantic fields to
HookConfig. While the typed approach is recommended for better type safety and IDE support, this example demonstrates the untyped.from_dict()method which:{"hooks": {...}}wrapper formatBoth approaches are valid and fully supported. The typed approach remains in the code as commented-out reference.
Related
@xingyaoww can click here to continue refining the PR
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:b07dae2-pythonRun
All tags pushed for this build
About Multi-Architecture Support
b07dae2-python) is a multi-arch manifest supporting both amd64 and arm64b07dae2-python-amd64) are also available if needed