DRAFT: Critic agent that forks generator settings for PR-style review #1706
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
xingyaoww
commented
Jan 13, 2026
xingyaoww
commented
Jan 13, 2026
Collaborator
Author
|
@OpenHands do a /codereview-roasted and help me address all review comments in this PR. When you are done, you should help me write a script in examples/0 folder that demonstrate this critic work in action. You can refer to the existing delagation example, as well as the example introduced in #1269 |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
Final summary (double-checked)✅ Checklist against your request
🔍 Conciseness / extraneous changes check
📌 What changed (since the last state on main)
🚀 Push status
|
|
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 |
xingyaoww
commented
Jan 14, 2026
Collaborator
Author
There was a problem hiding this comment.
We should put it under https://github.com/OpenHands/software-agent-sdk/tree/main/examples/01_standalone_sdk
xingyaoww
commented
Jan 14, 2026
| if __name__ == "__main__": | ||
| raise SystemExit(main()) | ||
| """ |
Collaborator
Author
There was a problem hiding this comment.
This seems not ideal. Should we make a hook ideally accepting a callback function as well?
xingyaoww
commented
Jan 14, 2026
Comment on lines
61
to
76
| def _extract_llm(self, events: Sequence[LLMConvertibleEvent]) -> LLM | None: | ||
| for event in events: | ||
| agent = getattr(event, "agent", None) | ||
| llm = getattr(agent, "llm", None) | ||
| if isinstance(llm, LLM): | ||
| return llm | ||
|
|
||
| for event in events: | ||
| if not isinstance(event, SystemPromptEvent): | ||
| continue | ||
| agent = getattr(event, "agent", None) | ||
| llm = getattr(agent, "llm", None) | ||
| if isinstance(llm, LLM): | ||
| return llm | ||
|
|
||
| return None |
Collaborator
Author
There was a problem hiding this comment.
We don't need this, since self.llm is already there
xingyaoww
commented
Jan 14, 2026
| `not_pass`. | ||
| """ | ||
|
|
||
| llm: LLM | None = None |
Collaborator
Author
There was a problem hiding this comment.
This should be llm: LLM
It is a required arg
xingyaoww
commented
Jan 14, 2026
Comment on lines
78
to
91
| def _extract_agent(self, events: Sequence[LLMConvertibleEvent]) -> Agent | None: | ||
| for event in events: | ||
| agent = getattr(event, "agent", None) | ||
| if isinstance(agent, Agent): | ||
| return agent | ||
|
|
||
| for event in events: | ||
| if not isinstance(event, SystemPromptEvent): | ||
| continue | ||
| agent = getattr(event, "agent", None) | ||
| if isinstance(agent, Agent): | ||
| return agent | ||
|
|
||
| return None |
Collaborator
Author
There was a problem hiding this comment.
Similar to the get planning agent function in the openhands.tools.preset - we should reconstruct an Agent instance ourselves
Collaborator
Author
|
@OpenHands review all current PR reviews and modify the code to address all the review comments. If you check examples/01_standalone_sdk/33_hooks/33_hooks.py example, you can see how we can set a Hook at Finish event. Can you extend the hook system, so we can pass in custom callback function python for the finish event hook, then we should modify the critic agent's implementation so critic agent runs in a callback function in a finish hook? |
|
I'm on it! xingyaoww can track my progress at all-hands.dev |
Address PR review comments: - Make llm field required in AgentReviewCritic (not optional) - Remove _extract_llm and _extract_agent methods - Add agent_factory parameter to allow custom agent creation - Add create_critic_stop_hook() helper for callback-based stop hooks - Create get_critic_agent in openhands.tools.preset.critic Extend hook system: - Add HookType.CALLBACK for Python callback functions - Update HookDefinition to support callback field - Update HookExecutor to execute callback hooks - Add validation to ensure callback is provided for CALLBACK type Move example: - Move from examples/00_critic/ to examples/01_standalone_sdk/34_critic_agent_hook.py - Update example to use callback-based hook with agent_factory Add tests: - Add tests for callback hook functionality - Update critic tests to use mock LLM fixture Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've successfully addressed all the PR review comments and implemented the requested features for PR #1706. The changes have been pushed to the Changes Made1. PR Review Comments Addressed ✅
2. Hook System Extended for Python Callbacks ✅
3. Critic Stop Hook Helper ✅
4. Tests Added ✅
Files Changed
Example Usagefrom openhands.sdk.critic.impl.agent_review import (
AgentReviewCritic,
create_critic_stop_hook,
)
from openhands.tools.preset.critic import get_critic_agent
# Create critic with custom agent factory
critic = AgentReviewCritic(
llm=llm,
agent_factory=get_critic_agent, # Optional: uses default if not provided
review_style="roasted",
)
# Create callback-based stop hook
hook_config = HookConfig(
stop=[
HookMatcher(
hooks=[create_critic_stop_hook(critic, str(workspace))]
)
]
)The PR is now updated at: #1706 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| from openhands.sdk.hooks.executor import HookResult |
|
|
||
| if TYPE_CHECKING: | ||
| from openhands.sdk.hooks.executor import HookResult | ||
| from openhands.sdk.hooks.types import HookEvent |
xingyaoww
commented
Jan 20, 2026
| return subprocess.check_output(["git", "diff"], cwd=workspace, text=True) | ||
|
|
||
|
|
||
| def main() -> None: |
Collaborator
Author
There was a problem hiding this comment.
@OpenHands please remove the main function and write it just as a script. /remember this in AGENTS.md when you finish modifying this script
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 and pushed them to the PR branch. Here's a summary:
Changes Made
1. Refactored examples/01_standalone_sdk/34_critic_agent_hook.py
- ✅ Removed the
main()function wrapper - ✅ Converted the example to run as a direct script at module level
- ✅ Kept helper functions (
_git,_git_patch) as they are reusable utilities - ✅ Consistent with other examples in the
01_standalone_sdk/folder (e.g.,01_hello_world.py)
2. Updated AGENTS.md with the convention
- ✅ Added new
<EXAMPLES_STYLE>section documenting that:- Examples in
examples/01_standalone_sdk/should be written as direct scripts - NOT wrapped in a
main()function - Script code should run at module level after imports and helper function definitions
- Examples in
Commits Pushed
bce1736a- Refactor critic agent hook example to script style4f2ad9af- Document examples script style convention in AGENTS.md
Both commits have been pushed to the openhands/critic-agent-reviewer branch and the PR has been updated.
Remove the main() function and write the example as a direct script, consistent with other examples in the 01_standalone_sdk folder. Co-authored-by: openhands <openhands@all-hands.dev>
Add EXAMPLES_STYLE section noting that examples in 01_standalone_sdk should be written as direct scripts without a main() function wrapper. Co-authored-by: openhands <openhands@all-hands.dev>
xingyaoww
force-pushed
the
openhands/critic-agent-reviewer
branch
from
4f2ad9a to
4475dfc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
AgentReviewCritic, aCriticBaseimplementation that spawns a separate OpenHands agent to review the current git diff.Fixes #1704
Checklist
@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:8010218-pythonRun
All tags pushed for this build
About Multi-Architecture Support
8010218-python) is a multi-arch manifest supporting both amd64 and arm648010218-python-amd64) are also available if needed