fix: attach task outcome to finish tool - #4465
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
we do have structured output for that
Co-authored-by: openhands <openhands@all-hands.dev>
malhotra5
changed the title
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>
|
📁 PR Artifacts Notice This PR contains a |
Co-authored-by: openhands <openhands@all-hands.dev>
2ea3b55 to
54fdb2b
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
✅ 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
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable - The core implementation is simple and the targeted tests pass, but this is not merge-ready because it changes the default FinishTool contract without the repo-required eval evidence and commits noisy prompt transcripts as source artifacts.
[CRITICAL ISSUES]
- [openhands-sdk/openhands/sdk/tool/builtins/finish.py, Line 104] Testing / Regression Proof: Attaching a default structured response schema to FinishTool is a global agent-behavior change. Per the repository review guide, eval-impacting changes need an eval monitor link or explicit maintainer confirmation before approval. I could not find that in the PR description or comments.
[IMPROVEMENT OPPORTUNITIES]
- [.pr/task_outcome_missing_mcp_result.md, Line 23] Artifact Hygiene: The committed evidence artifacts include full system/developer prompt transcripts. Keep the evidence concise and sanitized, or move transient local run logs out of the source tree.
[TESTING GAPS]
- Targeted tests I ran locally passed:
uv run pytest tests/sdk/tool/test_finish_task_outcome.py -q(18 passed)uv run pytest tests/sdk/tool/test_response_schema.py -q(36 passed)
- These unit tests are useful, but they do not replace the eval evidence required for a default agent tool-contract change.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This changes the default schema/instructions exposed to every SDK agent using FinishTool and alters automation callback payload behavior, so regressions could show up as changed agent completion behavior rather than isolated API failures. The implementation itself is localized and covered by targeted tests, and there are no dependency changes.
VERDICT:
❌ Needs rework: Add the required eval evidence/maintainer confirmation and remove or sanitize the committed prompt transcript artifacts before merge.
KEY INSIGHT:
The data model change is sensible, but because FinishTool is global agent plumbing, proving behavior did not regress matters more than adding more unit-level assertions.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| openWorldHint=False, | ||
| ), | ||
| ) | ||
| return [tool.set_response_schema(FinishTaskOutcomeResponse)] |
There was a problem hiding this comment.
🔴 Critical: Attaching a default structured response schema to FinishTool changes the prompt/tool contract for every SDK agent, so this is eval-impacting under the repository's custom review guide. I couldn't find an eval monitor link or maintainer confirmation in the PR description/comments. Please add that evidence before merge; the targeted unit tests are useful but they don't show the global agent-behavior impact.
There was a problem hiding this comment.
Will wait for maintainer confirming on whether we'd like to commit this as default behavior. IMHO its probably a good idea, considering the fact that background agents need a more principled mechanism by which they get specify that they're blocked at a given task.
Wrt to automations specifically, we want to wire this ability by default without having to rely on the user to configure the additional response schema. The automation service will also parse the TaskOutcome as we've defined it.
Will run a subset of eval for regression check
| {"asctime": "2026-08-11 22:47:14,152", "levelname": "WARNING", "name": "openhands.sdk.conversation.state", "filename": "state.py", "lineno": 516, "message": "No persistence_dir provided; falling back to InMemoryFileStore. EventLog data will not persist across requests."} | ||
| {"asctime": "2026-08-11 22:47:14,153", "levelname": "INFO", "name": "openhands.sdk.conversation.state", "filename": "state.py", "lineno": 587, "message": "Created new conversation 34b6287e-1232-4d2d-8200-7c9725207372"} | ||
| {"asctime": "2026-08-11 22:47:14,155", "levelname": "INFO", "name": "openhands.sdk.agent.base", "filename": "base.py", "lineno": 568, "message": "Loaded 0 tools from spec"} | ||
| System Prompt ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
🟠 Important: This committed evidence file includes the full agent system/developer prompt, and .pr/example_result.txt has the same problem. That is transient local run output, not SDK source: it bloats the tree and publishes internal operational instructions in a public repository. Please remove these transcript files from the branch, or replace them with a short sanitized artifact that only shows the command and final task outcome/status lines.
There was a problem hiding this comment.
.pr folder is automatically removed from the PR prior to merge. the folder is way to publish assets/evidence as part of the PR
|
Evaluation Triggered
|
❌ Evaluation FailedEvaluation: Log Tail: |
There was a problem hiding this comment.
🐾 (smolpaws, for Engel) — flagging a stale-task_outcome window on conversations that continue past the first finish (e.g. a Slack automation that replies, then the user sends another message and the same conversation resumes).
_task_outcome_recording only ever writes task_outcome (on FinishAction or ConversationErrorEvent) and it's persisted on ConversationState. It's never reset on resume. So:
- run 1 finishes → outcome stored
- user pings again → run 2 resumes;
task_outcomestill holds run 1's until run 2 sets its own - if run 2 ends without a
finish(a chat-style reply often does),close()ships run 1's outcome — and the automation#329 watchdog fallback reads that same persisted value
reported_at/terminal_reason let a reader detect staleness but nothing enforces it. Cheap fix: clear (or mark in-progress) at run start / on the new user message.
juanmichelini
left a comment
juanmichelini
left a comment
There was a problem hiding this comment.
Love the purpose of the PR. Is this backwards compatible or does it introduce breaking changes? Could we document that on the PR
6 participants
HUMAN:
Tested that this works via the example script as part of this PR.
AGENT:
Why
Automation runs need a first-class, automatically integrated way for agents to report outcome metadata such as success, blockers, and whether user action is needed. This should not require each automation author to configure a custom contract, and it should avoid a separate LLM call or standalone reporting tool when the agent already has a terminal
finishaction.Summary
TaskOutcomereporting to the defaultFinishTool, storing the latest outcome on conversation state for SDK, remote conversation, and agent-server consumers.finish.report_task_outcometool, limitsTaskOutcome.sourcetoagentorsystem, and updates examples/tests for the new flow.Issue Number
Links OpenHands/automation#323.
How to Test
Reviewer steps:
Run targeted task outcome tests:
Run lint/type checks for touched test files:
Run the standalone example after configuring an LLM environment:
Evidence from this PR:
54 passed, 5 warningsfortests/sdk/tool/test_finish_task_outcome.pyandtests/sdk/tool/test_response_schema.py.python .github/scripts/check_duplicate_example_numbers.py..pr/example_result.txtand.pr/task_outcome_missing_mcp_result.md.35 successful,0 failing,0 pending,2 skipped.Video/Screenshots
Design Doc
Type
Notes
OpenHands/docsbranchtask-outcome-reporting, documentingexamples/01_standalone_sdk/57_task_outcome_missing_mcp.pyso the documented examples check passes.TaskOutcome.sourceis intentionally limited toagentandsystem.🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang: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:be17960-pythonRun
All tags pushed for this build
About Multi-Architecture Support
be17960-python) is a multi-arch manifest supporting both amd64 and arm64be17960-python-amd64) are also available if needed