feat: add structured task outcomes to preset finish tool - #334
Merged
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
|
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>
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
Contributor
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
approved these changes
Aug 20, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
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>
Contributor
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
VascoSch92
added a commit
that referenced
this pull request
Aug 24, 2026
#334 landed revision 017 on main after this branch was cut, so both files declared revision 017 revising 016. Alembic refused to run at all — two heads, and "Revision 017 is present more than once". Chain this one after it instead.
Merged
5 tasks
VascoSch92
added a commit
that referenced
this pull request
Aug 24, 2026
…or and verifier registry (#378) * feat: replace the parse-only source registry with a provider descriptor and verifier registry A provider was described in three places: BUILTIN_SOURCES mapped a slug to a secret extractor, the event_schemas _PARSERS registry mapped the same slug to a parser, and RESERVED_SOURCES listed the same slugs by hand. Adding one meant editing all three. Verification was not a registry at all: verify_signature() was hard-wired to hex HMAC-SHA256 over the raw body, and a custom webhook could configure only the header name. Standard Webhooks and Slack could not be onboarded. Add openhands/automation/providers.py holding both registries. Provider carries the parser, verifier name, signature header, secret accessor, capabilities, and the subject/handshake hooks reserved for later phases. VERIFIERS resolves a scheme by name: hmac_sha256_hex (unchanged), standard_webhooks (from PR #247) and slack_v0, the last two rejecting deliveries outside a 5-minute window. event_router resolves a verifier instead of calling verify_signature directly, parse_event reads the descriptor, and RESERVED_SOURCES is derived. Add custom_webhooks.signature_scheme as a nullable column; NULL reads as hmac_sha256_hex, the behaviour the row was created with. slack is deliberately not registered as a provider: the OSS VM delivers to that source through a CustomWebhook row, and reserving the name would break it. Slack becomes a provider in #360. Also fix two order-dependent tests in test_router.py that asserted on a browser-supplied telemetry distinct id without forcing local mode; they passed only while an earlier test file left a local-mode config in the cache. * refactor: address review comments on the provider registry Delete the `# ====` banner comments, in providers.py, event_schemas and utils/webhook.py (where the banner predated this branch) and in the tests. Hoist every constant in providers.py to the top of the file and type them Final[str]/Final[int]. Drop `from __future__ import annotations`. It was carrying nothing: the two TYPE_CHECKING names are reached through string forward references inside `Callable[...]` type aliases, which are runtime expressions the future import does not touch. Import, pyright and the suite are unaffected without it. Correct the lazy-import comment in `parse_event()`. The import is genuinely required, but the cycle it breaks is not the one the comment named: moving it to module level fails on providers.py's own `from ...event_schemas import WebhookEvent`, not on the parsers the submodules define. Drop the issue and PR numbers from code comments and test docstrings. Trim the docstrings throughout to a single line where the signature already says the rest, including the Args/Returns block `verify_signature()` carried over from utils/webhook.py, and cut the migration docstring down to the one-line summary the other migrations use. No behaviour change: 1422 passed, pre-commit clean. * fix: refuse non-ASCII signature headers and hoist both registries Move VERIFIERS to the top of providers.py alongside PROVIDERS. Neither can hold its entries there -- the values are instances of classes defined further down, and a module-level annotation is evaluated at runtime -- so both are declared empty next to the type contracts and filled at the bottom, which is what PROVIDERS already did. Fix a crash on a non-ASCII signature header. `hmac.compare_digest` raises TypeError rather than returning False when either str argument is non-ASCII, and header bytes reach an app latin-1 decoded, so a signature header of b"\xff" took the request out through an unhandled exception -- a 500 where a 401 belongs. Reachable on every scheme: it was already true of `verify_signature` before this branch, and the two new verifiers inherited it. All three now compare bytes. Correct three comments and a migration docstring that claimed rows predating the column read as NULL. They do not: the ALTER TABLE carries a server_default, so existing rows are backfilled to "hmac_sha256_hex". NULL is reachable, but through a PATCH that clears the field, so the fallback stays and the reason it exists is now stated accurately. The test that covered this is renamed to match what it actually sets up. New tests: a non-ASCII signature is refused by each verifier and returns 401 end to end; PATCHing signature_scheme persists and changes which signature the delivery path accepts, which nothing covered; and standard_webhooks_key pins that a literal secret which happens to parse as base64 is still decoded. 1428 passed, pre-commit clean. * refactor: drop dead code and the duplicated freshness check Remove the module logger and its import: nothing in providers.py ever logged. `except (binascii.Error, ValueError)` around b64decode is one clause written twice -- binascii.Error subclasses ValueError -- so catch ValueError and drop the binascii import. `except (TypeError, ValueError)` around int(timestamp) had a dead arm. The guard above it rules out None, and int() of a str raises only ValueError. Both timestamped verifiers carried the same six lines of parse-and-compare. They now share `_within_tolerance()`, which also names the concept the two schemes have in common. `get_header` tried an exact hit, then a lowercased hit, then a full scan. The scan already subsumes the lowercased hit, so that middle branch was three lines buying nothing. 1428 passed, pre-commit clean. * fix: renumber the signature_scheme migration to 018 #334 landed revision 017 on main after this branch was cut, so both files declared revision 017 revising 016. Alembic refused to run at all — two heads, and "Revision 017 is present more than once". Chain this one after it instead.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
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