feat: add structured task outcomes to preset finish tool - #334
feat: add structured task outcomes to preset finish tool#334malhotra5 wants to merge 15 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
1 similar comment
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste rating: 🟢 Good taste — Approving.
The change is small, focused, and well-tested. Two-layer design (in-sandbox finish-tool wiring + server-side outcome extraction) is the right shape; each piece is independently testable.
What I verified locally:
- Pinned SDK commit
d99c1efd796d1188bbed7f4fec1f495d17505a85resolves to a working install ofopenhands.tools.preset.TaskOutcome(subdiropenhands-tools, same VCS pin asopenhands-sdk). First-party package → 7-day supply-chain rule does not apply. - Full test suite: 132 passed. The one error in
tests/test_router.pyisDockerExceptionfrom a missing Docker daemon in this environment (unrelated to this PR). - Migration 016 is minimal (one nullable JSON column, no backfill, no FK, no data loss risk on downgrade).
- Both
presets/{prompt,plugin}/sdk_main.pyapply the samefinish_tool_response_schema=TaskOutcomewiring — the duplicated change is consistent across files. setup.shparses the new/sdk-versionJSON shape identically in all three locations (scripts/test_tarball/,presets/prompt/,presets/plugin/).
Minor suggestions (non-blocking):
[IMPROVEMENT OPPORTUNITIES]
- [
openhands/automation/utils/version.py] Dead code:_sdk_direct_url_install_spec()exists only to be tested — production code now uses_direct_url_install_spec(distribution, SDK_PACKAGE_NAME)directly. The one-line wrapper has a single caller (tests/test_version.py) and could be inlined into the test or dropped. - [
openhands/automation/utils/version.py,get_tools_install_spec] String surgery: the tools install spec is built by substring-replacingopenhands-sdk→openhands-toolsandsubdirectory=openhands-sdk→subdirectory=openhands-toolson the SDK install spec. Today this is safe because the SDK pin guarantees both subdirs live at the same commit, but it's an implicit coupling to the subdirectory naming convention. A cleaner shape: parse the SDK'sdirect_url.json, swap the package name andsubdirectoryfield, and re-serialize — same logic, no string surgery, no silent breakage if the subdir name ever changes. - [
openhands/automation/utils/conversation_outcome.py] Uncovered branch:fetch_latest_finish_tool_response_for_run(the wrapper that catches all exceptions and handles local-vs-remote backend) is only exercised intests/test_router.pyvia monkeypatch. Neither thebackend.is_local_modetrue branch nor the remote httpx branch inside this wrapper has a direct test. The innerfetch_latest_finish_tool_responseIS covered withhttpx.MockTransport(good), so the bug surface is small, but a 5-line direct test of the wrapper would close the gap. - [
openhands/automation/presets/{prompt,plugin}/sdk_main.py] Duplication: The 4-lineget_default_agent(...)change is duplicated across two files. Not avoidable given the preset structure, but worth noting that any future change to the finish-tool wiring must touch both — a comment pointing to its sibling would help reviewers of future PRs.
[TESTING GAPS]
- Same point as the third item above — a direct unit test for
fetch_latest_finish_tool_response_for_run(with bothis_local_mode=Trueand the remote-httpx path) would make the outcome pipeline provably correct end-to-end without depending on the integration test intest_router.py.
[RISK ASSESSMENT]
⚠️ Risk Assessment: 🟢 LOW- Breaking change: None — only additive schema (
run_metadatanullable JSON column, new field on response, new utility, new key in/sdk-versionresponse). Existing API consumers see the same shape plus one extra optional key. - Security: None —
run_metadatais server-internal storage; the finish-tool response is parsed from a sandbox we already trust to run arbitrary code. - Supply chain: SDK pinned to a commit, but it's a first-party OpenHands package — the 7-day freshness rule does not apply per the reviewer guidelines, and the commit is referenced as required by this PR.
- Failure mode: If outcome extraction fails, the wrapper silently returns
Noneand the run still completes normally — graceful degradation, no user-visible regression.
- Breaking change: None — only additive schema (
VERDICT: ✅ Worth merging. Solid PR; minor cleanup suggestions above are optional follow-ups.
KEY INSIGHT: Server-side outcome extraction is intentionally a best-effort post-step (try/except → None) rather than a hard dependency on the finish-tool schema — this is the right call, because it keeps run completion decoupled from agent schema evolution.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Summary
TaskOutcome/TaskOutcomeBlockerPydantic response models to prompt and plugin preset SDK templates.FinishToolwithresponse_schema=TaskOutcome.outcome_summaryas the schema field alias for the model'ssummaryattribute becausesummaryis reserved by the SDK structured-output tool schema.Testing
uv run pytest tests/test_preset_router.py -q -k 'PresetFileSyntax or GenerateTarball or GeneratePluginTarball'uv run pre-commit run --files openhands/automation/presets/prompt/sdk_main.py openhands/automation/presets/plugin/sdk_main.py tests/test_preset_router.py --show-diff-on-failureNote: I also attempted
uv run pytest tests/ -q --ignore=tests/integration; it failed during setup because Docker/testcontainers is unavailable in this environment (DockerException: Error while fetching server API version).This PR was created by an AI agent (OpenHands) on behalf of the user.
@malhotra5 can click here to continue refining the PR