Skip to content

feat: add structured task outcomes to preset finish tool - #334

Open
malhotra5 wants to merge 15 commits into
mainfrom
add-task-outcome-structured-output
Open

feat: add structured task outcomes to preset finish tool#334
malhotra5 wants to merge 15 commits into
mainfrom
add-task-outcome-structured-output

Conversation

@malhotra5

Copy link
Copy Markdown
Member

Summary

  • Add TaskOutcome/TaskOutcomeBlocker Pydantic response models to prompt and plugin preset SDK templates.
  • Configure preset agents to use an explicit FinishTool with response_schema=TaskOutcome.
  • Use outcome_summary as the schema field alias for the model's summary attribute because summary is reserved by the SDK structured-output tool schema.
  • Add preset tests that assert both generated templates include the structured finish-tool wiring.

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-failure

Note: 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

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions github-actions Bot added the type: feat A new feature label Aug 12, 2026
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Coverage

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>

Copy link
Copy Markdown
Contributor

🔍 Review in progress…

We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here.

1 similar comment

Copy link
Copy Markdown
Contributor

🔍 Review in progress…

We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here.

all-hands-bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 d99c1efd796d1188bbed7f4fec1f495d17505a85 resolves to a working install of openhands.tools.preset.TaskOutcome (subdir openhands-tools, same VCS pin as openhands-sdk). First-party package → 7-day supply-chain rule does not apply.
  • Full test suite: 132 passed. The one error in tests/test_router.py is DockerException from 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.py apply the same finish_tool_response_schema=TaskOutcome wiring — the duplicated change is consistent across files.
  • setup.sh parses the new /sdk-version JSON 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-replacing openhands-sdkopenhands-tools and subdirectory=openhands-sdksubdirectory=openhands-tools on 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's direct_url.json, swap the package name and subdirectory field, 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 in tests/test_router.py via monkeypatch. Neither the backend.is_local_mode true branch nor the remote httpx branch inside this wrapper has a direct test. The inner fetch_latest_finish_tool_response IS covered with httpx.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-line get_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 both is_local_mode=True and the remote-httpx path) would make the outcome pipeline provably correct end-to-end without depending on the integration test in test_router.py.

[RISK ASSESSMENT]

  • ⚠️ Risk Assessment: 🟢 LOW
    • Breaking change: None — only additive schema (run_metadata nullable JSON column, new field on response, new utility, new key in /sdk-version response). Existing API consumers see the same shape plus one extra optional key.
    • Security: None — run_metadata is 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 None and the run still completes normally — graceful degradation, no user-visible regression.

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

Comment thread openhands/automation/utils/version.py Outdated
Comment thread openhands/automation/utils/version.py Outdated
Comment thread openhands/automation/utils/conversation_outcome.py
Comment thread openhands/automation/presets/plugin/sdk_main.py
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: feat A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants