Feat: structured output - #4207
Conversation
Port follow-ups on top of OpenHands#2808: - reject response_schema fields named 'summary' or 'security_risk': the SDK injects meta-fields with those names into every action schema after the response-schema merge, so a user field would be silently shadowed on the way out (risk) or double as the event summary on the way back - renumber the example to 56_ (48_ was taken by conversation_fork) - update the example NOTE: the reserved names are now enforced, not a convention
Co-authored-by: Vasco Schiavo <115561717+VascoSch92@users.noreply.github.com>
|
Address review: collapse the reverse-scan loop into next() over a generator expression. Co-authored-by: Vasco Schiavo <115561717+VascoSch92@users.noreply.github.com>
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
Moreover, could you make the comments and docstring minimal and not verbose. Thanks
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Requested reviewer: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🔴 Needs improvement
[CRITICAL ISSUES]
- The new persisted
Action.structured_outputfield is not readable by older SDKs. I reproduced a structuredFinishActionevent from this head and loaded it withopenhands-sdk==1.36.0;Event.model_validate_json()fails withaction.structured_output Extra inputs are not permitted. This contradicts the PR's compatibility claim and creates a version-skew/resume break for older readers. - The LLM-facing response schema drops valid constraints before advertising it, so outputs can satisfy the advertised contract and still fail runtime validation. See the inline finding.
[TESTING / VALIDATION GAPS]
check-examplescurrently fails becauseexamples/01_standalone_sdk/56_structured_output.pyis undocumented. The SDK package guidance requires a corresponding docs PR and cross-reference; none is linked here.- This changes core tool-calling and action-event behavior, but there is no completed eval-monitor run with human confirmation. Per repository policy, a human maintainer should decide after lightweight evals. The focused structured-output/adjacent suite passed locally (58 tests); the current
sdk-testsfailure is an MCP stdioConnection closedfailure rather than a structured-output assertion.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. The design addresses a real need and the revised static action kind fixes the earlier persistence blocker, but this still changes the serialized event contract and every structured tool-call validation path. The compatibility and schema-contract defects should be fixed before merge, followed by lightweight eval validation.
VERDICT:
❌ Needs rework: Fix event compatibility and preserve the advertised response-schema contract; add/link the required docs update.
KEY INSIGHT:
Structured output must preserve one identical contract across LLM schema generation, runtime validation, persistence, and version-skewed readers.
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 created by an AI agent (OpenHands) on behalf of the repository reviewer.
Co-authored-by: openhands <openhands@all-hands.dev>
|
Implemented the latest review fixes in d7c3b53 and resolved both threads. Validation:
Companion docs: OpenHands/docs#668. The optional check-examples job cannot see a fork branch (it only checks OpenHands/docs/) and will remain red until that docs PR is merged or the branch is created upstream; this account cannot push to OpenHands/docs. The sdk-tests CI failure is unrelated timing flakiness in tests/sdk/io/test_filestore_cache.py::test_cache_performance_repeated_reads (5627 other tests passed). I do not have repository permission to rerun that job or apply the review-this label. A human maintainer still needs to run/confirm the requested lightweight eval. |
Co-authored-by: openhands <openhands@all-hands.dev>
|
Current status on a7a662e: all CI checks pass, including sdk-tests, tools-tests, Windows tests, all three agent-server binary builds, coverage, pre-commit, and the review-thread gate. The runnable example was removed from the SDK diff to avoid the fork-only docs branch limitation; the companion documentation remains in OpenHands/docs#668. Ready for human review and the requested eval confirmation. |
|
Conflicts resolved and merged The only conflicts were in dependency files:
No source-code conflicts — the structured-output changes merged cleanly. Validation:
The PR now reports This comment was posted by an AI agent (OpenHands) on behalf of the user. |
|
Done. I've resolved the merge conflicts on PR #4207. SummaryThe branch What conflicted: Only two dependency files — no source-code conflicts. The structured-output implementation merged cleanly.
Validation:
(The broader Result: Merge commit The remaining "blocked" state just reflects pending required reviews/checks, not conflicts. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: Feat: structured output (head 78f9ecd)
I reviewed the latest head against the findings from the three prior reviews (two all-hands-bot, one enyst). All previously identified blocking issues are resolved on this head:
- Persistence compatibility:
_structured_outputis aPrivateAttronActionthat never crosses the serialization boundary. I confirmed viatest_action_event_roundtrips_with_static_kindthat serialized events contain neitherstructured_outputnor a synthetic action subclass, and thatEvent.model_validate_jsonrestores a cleanFinishActionwithstructured_output is None. additionalProperties: falsepropagation:_merge_response_schemanow copiesadditionalProperties: falsefrom the response schema into the merged LLM-facing schema. Verified withtest_response_schema_preserves_additional_properties_false.parse_last_responseside-effect-free: it callsevent.action.model_copy()before setting_structured_output, so the caller's event is not mutated. The round-trip test assertsrestored.action.structured_output is Noneboth before and after the call.- Unsupported top-level schema keywords:
_response_tool_schemauses an explicit allowlist (_SUPPORTED_RESPONSE_SCHEMA_KEYS) and rejects anything outside it (e.g.dependentRequired), so the advertised and validated schema contracts cannot diverge via unsupported object-level semantics. - Dynamic fields:
additionalPropertiesvalues other thanNone/Falseare rejected, and schemas without named properties are rejected. - Reserved meta-field names (
kind,security_risk,structured_output,summary) are rejected atset_response_schematime using the expanded property set. - Field collision is checked in both
set_response_schemaand_merge_response_schema.
I verified the alias edge case directly: a Pydantic model with Field(alias="myFoo") advertises the alias in the LLM-facing schema, _split_response_arguments validates the alias keys (default model_validate), dumps to field names via model_dump(mode="json"), and parse_response correctly re-validates with by_name=True to match the field-name keys. The by_name=True is essential here — without it, parse_response would fail on aliased models because model_validate defaults to expecting alias keys.
All 214 tests/sdk/tool/ tests pass, including the 32 in test_response_schema.py.
No blocking correctness or security defects found
Remaining items (non-blocking)
-
Missing
integration-testlabel. This PR modifies agent dispatch (_get_action_eventnow runsfix_malformed_tool_argumentsagainsttool.response_schemabefore tool execution) and tool schema generation — the path of every tool call. PerAGENTS.mdTESTING guidance, changes to tool descriptions or agent decision logic should add theintegration-testlabel so benchmark impact is verified. This has been flagged in all three prior reviews and remains unaddressed. Please add the label before merge. -
Stale PR description. The "How to Test" and "Video/Screenshots" sections reference
examples/01_standalone_sdk/56_structured_output.py, but that file does not exist in the diff (deferred to the companion docs PR). Update the PR description to avoid pointing reviewers at a non-existent file. -
_response_schema_jsonis recomputed on every tool call (tool.py:518). For a Pydantic model, this callsmodel_json_schema()on each invocation of_split_response_arguments, which is not free. The no-schema fast path is unaffected, but for tools with aresponse_schemathis runs on every action. Consider caching the normalized schema on the tool instance (e.g. a one-time normalization inset_response_schema).
Risk assessment
MEDIUM. The design is sound and all prior blocking issues are resolved. Risk remains medium because this touches the path of every tool call (action_from_arguments / _get_tool_schema / to_mcp_tool), and the repository-mandated benchmark signal (integration-test) has not been recorded.
Verdict: No blocking correctness or security defects. Recommend adding the integration-test label and running the benchmark gate before merge.
_response_schema_json() was called on every action_from_arguments() invocation, regenerating the Pydantic JSON schema each time. The no-schema fast path was unaffected, but tools with a response_schema paid the cost on every call. Now the normalized schema is computed once in set_response_schema and stored in a PrivateAttr, with a fallback for older instances. Co-authored-by: openhands <openhands@all-hands.dev>
…teAttr
The previous per-instance PrivateAttr cache went stale after model_copy:
- set_response_schema(None) left a non-empty cache on the no-schema copy
- model_copy(update={"response_schema": X}) bypassed set_response_schema
and reused the OLD schema, silently routing fields to the wrong model
Move the cache to a module-level dict keyed by the immutable Pydantic class
(matching the existing _action_types_with_* pattern) inside _response_schema_json.
Pydantic model_json_schema() now runs at most once per class with zero
staleness risk; dict schemas keep the cheap deepcopy path. Drop the
PrivateAttr field entirely, so the cache can never diverge from
response_schema.
Co-authored-by: openhands <openhands@all-hands.dev>
VascoSch92
added
the
integration-test
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR implements first-class structured output by attaching a Pydantic model (or JSON Schema dict) to any tool spec via response_schema. The model's fields are merged into the LLM-facing action schema, validated on receipt in action_from_arguments, and recoverable as a typed object through parse_response() / parse_last_response(). The design is clean and aligns well with the SDK's stateless-Pydantic-model principle: response_schema and structured_output are both runtime-only (SkipJsonSchema, exclude=True / PrivateAttr), so persisted events and tool specs are unchanged and older readers are unaffected. The zero-cost-when-unused guards (if self.response_schema is None) keep the hot path untouched, and parse_last_response() reconstructs structured output from the already-persisted tool_call.arguments rather than relying on a new persisted field, which is the right call for backward compatibility.
I verified the core paths by exercising them directly: the $ref expansion handles circular references, the reserved-field check correctly catches aliased collisions (e.g. Field(alias="summary") is rejected because the advertised schema property key is summary), fix_malformed_tool_arguments works correctly with both Pydantic-model and dict-schema response schemas, and optional response fields omitted by the LLM are filled with defaults. The by_name=True in parse_response is safe because it accepts both field-name and alias keys.
Risk Assessment: Low-Medium
The PR touches action_from_arguments / to_mcp_tool / _get_tool_schema, i.e. the path of every tool call, so the integration-test label is appropriate. The no-schema fast path is guarded and covered by test_finish_tool_without_schema_is_unchanged. The new jsonschema dependency is a well-known, stable package.
Actionable Finding: Merge drift reverts unrelated work
The diff between the PR head and the current main tip (06a7d72) includes changes that are not part of this feature and appear to be artifacts of the branch not being rebased onto the latest main. Inline comments could not be attached because the affected files are deletions/reversions, so the findings are listed here:
- Version downgrades in all four
pyproject.tomlfiles (openhands-sdk,openhands-tools,openhands-workspace,openhands-agent-server) from1.40.1to1.40.0. This would regress the published version if merged as-is. - Deletion of
openhands-agent-server/openhands/agent_server/canvas_extensions/(manifest module +__init__, ~178 lines) and the corresponding tests undertests/agent_server/canvas_extensions/. These were added in #4361 ("Canvas Extensions manifest and containment [1/4]"). - Reversion of the accumulated-LLM-cost completion callback (#4311):
BaseWorkspace.register_cost,accumulated_cost,_send_completion_callback,LocalWorkspace.__exit__/RemoteWorkspace.__exit__overrides, and the associated tests undertests/sdk/workspace/,tests/conversation/, andtests/workspace/are removed.
These reverts are almost certainly unintentional. Before merging, rebase the branch onto the current main tip so only the structured-output changes remain in the diff. The structured-output implementation itself looks correct and complete.
…566-structured-output
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
Reviewed the structured-output feature: a Pydantic model or JSON Schema dict attached to a tool via params={"response_schema": ...}. The schema fields are merged into the LLM-facing action schema, split out on receipt into Action._structured_output (a PrivateAttr), validated, and recovered typed via parse_response() / parse_last_response().
Overall this is a clean, well-scoped implementation. I verified the test suite (34 tests pass), pre-commit (ruff/pyright/pycodestyle/import-rules/tool-registration all clean on changed files), and several edge cases by hand (aliased fields, nested models, dict-schema required-field enforcement, $ref expansion, round-trip through persisted specs). The design choices are sound:
- Zero cost when unused — every new hook is behind
if response_schema is None, and_merge_response_schema/to_mcp_toolearly-return unchanged on the no-schema path, so the hot path for plain tools is unaffected. - Persistence/wire shape unchanged —
response_schemaisSkipJsonSchema+exclude=TrueonToolDefinition, and_structured_outputis aPrivateAttr, so persisted events/specs are byte-for-byte unchanged and older SDKs keep reading them.parse_last_responsereconstructs structured output from the already-persistedtool_call.arguments, which I confirmed still carries the response fields (it is serialized before_extract_security_risk/_extract_summarypop meta fields). - Reserved-field guard is thorough — rejecting
kind,security_risk,structured_output, andsummaryprevents the silent absorption the PR calls out. The action-field collision check and theadditionalProperties/named-properties/whitelist validation on dict schemas are appropriately defensive.
Risk assessment: Low
The change is opt-in and backward-compatible. The residual risk is test coverage on the composed agent path (see below) and the eval impact the author already flagged — the integration-test label is appropriate.
Findings
Inline
tool.py: the per-class JSON-schema cache is unguarded while its sibling caches use_action_type_lock, and the comment's "at most once per class" guarantee isn't actually enforced under concurrency. Low severity (GIL-safe + idempotent in CPython), but the comment/implementation mismatch is worth a one-line fix.
In the body (not tied to a single changed line)
-
No integration test for the agent
_get_action_eventpath. The new conditional secondfix_malformed_tool_arguments(arguments, tool.response_schema)call inagent.py— and its interaction with_extract_security_risk/_extract_summarypoppingsecurity_risk/summarybeforeaction_from_argumentssplits the response fields — is on the hot path of every tool call but is only covered indirectly. The unit tests exerciseaction_from_argumentsandfix_malformed_tool_argumentsin isolation. A behavior-style test that drives aresponse_schema-equipped tool through a realAgent.step(and assertsstructured_outputsurvives on the emittedActionEvent) would lock in the composition and guard the reconstruction contract. This is the most material gap. -
ClientTool._get_tool_schemamerges the response schema twice.super()._get_tool_schema(...)(line 285) already callsself._merge_response_schema(...)internally, then line 301 callsself._merge_response_schema(merged)again. Not a correctness bug (idempotent), just redundant work on the schema-build path. Worth collapsing to a single merge. -
PR description inaccuracy (non-blocking). The description references
_create_action_type_with_schema"cached per(action_type, schema)pair," but no such function exists — the implementation merges at the JSON-schema level via_merge_response_schema/_response_tool_schema. The code is correct; only the description is stale. Flagging so reviewers aren't confused when cross-referencing.
Verification notes
tests/sdk/tool/test_response_schema.py: 34 passed.tests/sdk/tool: all pass.tests/crossfailures observed (test_remote_conversation_live_server.py,test_event_loss_repro.py) are environmental — they fail on agent-server config/env parsing (IndexErrorinenv_parser.py) and live-server startup, unrelated to this PR's changes.- Pre-commit clean on all 8 changed SDK files.
No blocking issues. The inline cache note and the agent-path test gap are the two items I'd suggest addressing before merge; the rest are optional.
Address review: the per-class cache was read and written unguarded while its sibling action-type caches use _action_type_lock, so the promised 'at most once per class' build was not actually enforced. Build under a dedicated lock (the two never nest, so schema building stays off the action-type critical section) and hand back a private copy so callers cannot mutate the cached entry. Adds a concurrency test.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
@VascoSch92 can you take a look ? |
|
I'm looking inside the tests but it seems an infrastructure problem |
|
thanks |
|
It was a pleasure working with you on this. I already have a few new ideas 😉. Thanks again! |
* docs(sdk): add structured output guide Documents the response_schema mechanism landed in OpenHands/software-agent-sdk#4207: attaching a Pydantic model or JSON Schema to any tool spec, reading typed results via parse_response / parse_last_response, the raw JSON Schema form, and the constraints (reserved field names, one tool per spec, round-trip to dict). * docs(sdk): address review on the structured output guide - fix the persistence claim: structured_output is a PrivateAttr excluded from event serialization, so it is None after a reload; parse_last_response re-reads the tool call and does survive - note that a schema field clashing with the tool's own field also raises at resolution time, not just the reserved meta names - follow the other guides: add a Ready-to-run Example block backed by examples/01_standalone_sdk/56_structured_output.py - trim the prose throughout (124 -> 50 lines) * docs(sdk): note that response_schema fields are scoped to their tool Observed while running the example: the model may attempt to send the schema fields when calling other tools, which are rejected as unexpected arguments before the agent retries.
5 participants
HUMAN:
Implemented a general version of #4116 after @VascoSch92 suggested it. Credit also to his original design idea #2808 remained unfinished.
AGENT:
Why
#2566 asks for first-class structured output. Today, getting reliably
formatted responses out of an agent means manual prompting plus brittle
post-processing. @VascoSch92 implemented the mechanism in #2808 — reviewed
favorably, never landed for lack of time — and offered the final pass to me.
This PR completes that work: his three commits rebased onto current
main(authorship preserved), plus a fix for the known meta-field collision issue.
Summary
Tool(name="FinishTool", params={"response_schema": ProjectFacts}).The model's fields are merged into the action schema sent to the LLM
(
_create_action_type_with_schema, cached per(action_type, schema)pair),validated on receipt in
action_from_arguments, and recoverable typed viaparse_response()/parse_last_response().response_schemafrom spec params before calling thetool's
create()(factories with fixed signatures, e.g.FinishTool.create,never see it) and applies it via
set_response_schema()— amodel_copy,consistent with
set_executor().response_schemais runtime-only (SkipJsonSchema,exclude=True): it nevercrosses the serialization boundary, so persisted events/specs are unchanged
and older readers are unaffected.
Tool.paramsdrops class values on dump.summaryorsecurity_riskare rejected with an explicitValueError. The SDK injectsmeta-fields with those names into every action schema after the merge: a
user field would be silently absorbed as the event summary (
summary) orredefined and swallowed by the risk-analyzer flow (
security_risk).if response_schema is None.Issue Number
Closes #2566. Supersedes / completes #2808.
How to Test
19 tests cover: schema extension, payload validation (accept/reject), nested
Pydantic models, class-creation caching, spec serialization dropping class
values,
parse_last_responseacross multiple tools, executor unchanged,tool-without-schema unchanged, action/schema field collision, and the two
reserved meta-field names. Full
tests/sdk/tool+tests/crosssuites pass(verified on two machines);
tests/sdk/agentpasses except one failure(
test_acp_agent.py::…::test_gemini_046_uses_set_session_model) reproduced onunmodified
main, i.e. pre-existing.pre-commithook set (ruff, pyright,pycodestyle, import rules, tool registration) clean on all changed files.
See OpenHands/docs#668 for the runnable end-to-end example.
Video/Screenshots
Both directions of the mechanism:
Schema attached to
TerminalTool— the model must justify every command:Schema attached to
FinishTool— the final answer comes back as a typedobject via
parse_last_response()(Pydantic-validated, no text parsing):Type
Notes
authorship preserved; they applied onto current
mainwithout conflicts.Commit 4 adds the reserved-name guard, renumbers the example to
56_(48 was taken), and updates its NOTE.
action_from_arguments/to_mcp_tool, i.e.the path of every tool call. The no-schema fast path is covered by
test_finish_tool_without_schema_is_unchanged; happy to have the eval suiterun before merge if maintainers want the extra confidence — guidance on how
to trigger it welcome.
Documentation
Companion docs PR: OpenHands/docs#668.
Review update (d7c3b53)
Structured output is now runtime-only on actions and is reconstructed from the existing persisted
tool_call.argumentsfield, keeping the event wire shape readable by older SDKs. Response schema merging also preserves validation constraints and rejects schemas without named properties.Review update (529e292)
Preserved
additionalProperties: falsein the advertised schema and madeparse_last_response()side-effect free. Validation: 214 SDK tool tests and pre-commit passed.