Python: Preserve structured output on streaming harness tool-approval path (#7418)

[Shivani . (Shivani767)]

Motivation & Context

When a harness agent uses structured output (response_format) together with tool approval, the streaming path lost the parsed value. ToolApprovalMiddleware._process_stream returned ResponseStream(..., finalizer=AgentResponse.from_updates) without forwarding output_format_type. The non-streaming path forwarded it, so AgentResponse.value was populated only when streaming was on (#7418).

This is the successor to #7800, which was closed while I consolidated on a single open PR; westey (@westey-m) then correctly noted that #7798 did not cover this change, so the fix was left without an active PR. This PR carries it as a fresh, non-draft PR. This supersedes #7800 — please close #7800 in favour of this one.

Description & Review Guide

What are the major changes?

• Added _structured_response_format(context): resolves the effective response format, preferring run options["response_format"] and falling back to agent.default_options.
• The streaming finalizer now forwards that format to AgentResponse.from_updates, so the streaming path parses value the same way the non-streaming path does.
• The finalizer rebuilds the response from the streamed updates (preserving the middleware's approval / user-input handling), then forwards the terminal inner response's parsed structured value through AgentResponse.from_updates(..., value=...) — the public value argument on from_updates, not a private-field mutation (Python: [Bug]: Harness Agent drops structured value on the streaming path with tool approval #7418). Without this, an auto-approved preamble that from_updates coalesces into the JSON message masks the parsed value.

What is the impact of these changes?

• AgentResponse.value is now populated for streaming runs that use structured output plus tool approval, including the auto-approved-tool and coalesced-preamble cases.
• No change to non-streaming behaviour.
• The finalizer resolves final.value (which also completes the terminal response's lazy parse) and passes the result to from_updates(value=...). If the terminal parse fails, the value is left unset so the outer response still raises lazily on value access, matching the error timing of a middleware-free streaming run.

What do you want reviewers to focus on?

• The combination of "rebuild from updates, then forward the parsed value" versus simply returning the inner response. Returning the inner response directly was the earlier attempt, but current main relies on the finalizer rebuilding from updates so buffered approval requests are re-emitted (_tool_approval.py:526), and that broke test_tool_approval_middleware_auto_approves_with_host_pause_and_cached_safe_call[streaming].
• The finalizer now uses the public AgentResponse.from_updates(..., value=...) contract rather than touching the private _value / _value_parsed fields, so the lazy-parse case no longer reparses preamble + JSON as one invalid string.

Related Issue

Fixes #7418

Supersedes #7800 (closed, no replacement PR). #7798 addressed a separate issue and did not include this fix. No other open PR addresses #7418.

Validation

Branch is up to date with current main (6362719, merge of main into the branch on 2026-09-21).

uv run pytest packages/core/tests/core/test_harness_tool_approval.py -q
71 passed

uv run pytest packages/core/tests/core/ -q -k 'approval or streaming'
all passed (EXIT=0)

New regression tests: test_streaming_tool_approval_preserves_structured_value, test_streaming_auto_approved_tool_preserves_structured_value, test_streaming_auto_approved_tool_preserves_value_when_preamble_text_coalesces, test_non_streaming_tool_approval_preserves_structured_value, test_streaming_tool_approval_defers_structured_parse_error_to_value_access.

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

/cc westey (@westey-m) — you flagged that #7800's structured-output fix was not covered by #7798; this is that fix, rebased and green.

[Copilot]

Copilot review overview

🟡 Changes recommended

Lazy parsing prevents the terminal structured value from being copied in the preamble case.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Preserve terminal JSON value before checking lazy parse state · New

What changed in this PR

Fixes structured-output preservation in streaming tool-approval middleware.

Changes:

• Propagates the effective response format into stream finalization.
• Attempts to retain parsed terminal values.
• Adds structured-output regression tests.

File	Description
_tool_approval.py	Adds response-format resolution and value carry-over.
test_harness_tool_approval.py	Adds streaming and non-streaming regression coverage.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Evan Mattson (moonbox3)]

Please also fix the CI/CD failures.

[Shivani . (Shivani767)]

Sure, I will update with 1-2 hour time period

[Eduard van Valkenburg (eavanvalkenburg)]

Shivani . (@Shivani767), can this use the existing public finalizer contract instead of copying private fields? AgentResponse.from_updates(..., value=...) already accepts a pre-parsed structured value. Resolving final.value from the terminal inner response and passing it as value would keep the outer update aggregation while avoiding _value and _value_parsed mutation. The current _value_parsed guard skips the normal lazy case, so the preamble and terminal JSON can still be reparsed as one invalid string. Package Checks, Test Typing Checks, and Merge Gatekeeper are also currently failing.

[Shivani . (Shivani767)]

Sure I will

[Shivani . (Shivani767)]

Thanks Eduard van Valkenburg (@eavanvalkenburg) — done in f2e874f. The finalizer now uses the public contract and no longer touches private fields:

value: Any = None
if final is not None:
    # ``ValidationError`` subclasses ``ValueError``; ``TypeError`` covers a
    # malformed ``response_format`` in the terminal inner response.
    with contextlib.suppress(ValueError, TypeError):
        value = final.value
return AgentResponse.from_updates(updates, output_format_type=response_format, value=value)

Reading final.value completes the terminal response's lazy parse, so:

• the outer response is still aggregated from the buffered updates (the middleware's approval / user-input handling is preserved), and
• the already-parsed structured value is carried over via from_updates(..., value=...), so an auto-approved preamble coalesced into the JSON message can't mask it.

The _value_parsed guard is gone, so the normal lazy case no longer reparses preamble + JSON as one invalid string. When the terminal parse fails, the value is left unset so it still raises lazily on value access, matching the error timing of a middleware-free streaming run (covered by test_streaming_tool_approval_defers_structured_parse_error_to_value_access).

CI status, verified locally on Python 3.11:

• Package Checks ✅ — cast("Mapping[str, Any]", default_options) in _structured_response_format removes the strict-pyright reportUnknownMemberType / reportUnknownVariableType at _tool_approval.py:94; pyright on packages/core is now 0 errors.
• Test Typing Checks ✅ — the parametrized test's option dicts are kept out of Agent's generic client/options inference; mypy, pyrefly, ty, zuban, and pyright all pass.
• pytest packages/core/tests/core/test_harness_tool_approval.py → 71 passed.

[Shivani . (Shivani767)]

Evan Mattson (@moonbox3) the CI/CD failures are fixed and verified locally on Python 3.11.

• Package Checks: pyright on packages/core reports 0 errors. The committed code failed here with reportUnknownMemberType and reportUnknownVariableType at _tool_approval.py:94, because default_options.get(...) was partially unknown. That lookup now goes through an explicit cast("Mapping[str, Any]", ...).
• Test Typing Checks: mypy, pyrefly, ty, zuban and pyright all pass over the packages/core tests. The failure was in the parametrized regression test, where the option dicts clashed with Agent's generic OptionsCoT inference against the fixture's MockBaseChatClient[ChatOptions[None]].
• Tests: pytest packages/core/tests/core/test_harness_tool_approval.py passes 71 tests, and -k 'approval or streaming' over packages/core/tests/core also passes.
• ruff check and ruff format --check are clean.

The finalizer also now follows Eduard van Valkenburg (@eavanvalkenburg)'s suggestion and passes the parsed value through the public AgentResponse.from_updates(..., value=...) contract, instead of mutating the private _value and _value_parsed fields.

Update: I merged main into the branch to bring it up to date. The head is now 6362719 and the PR's net diff is unchanged (_harness/_tool_approval.py plus the harness tool-approval tests). Re-verified locally after the merge: pyright 0 errors, all five test type checkers pass, and the tool-approval test file passes 71 tests.

[Eduard van Valkenburg (eavanvalkenburg)]

Shivani . (@Shivani767) Thanks for updating the implementation. The PR body still describes copying _value / _value_parsed and asks reviewers to evaluate that private-field approach, but the current head uses the public AgentResponse.from_updates(..., value=...) contract. Could you update the Description & Review Guide to describe the implementation that is actually in the diff?

[Shivani . (Shivani767)]

Eduard van Valkenburg (@eavanvalkenburg) done — thanks for catching that. The Description & Review Guide now describes the implementation that is actually in the diff: the finalizer rebuilds the outer response from the streamed updates and forwards the terminal inner response's parsed value through the public AgentResponse.from_updates(..., value=...) contract, with no mention of the old _value / _value_parsed copying. The reviewer-focus bullet now reads as confirming the public-contract approach rather than asking reviewers to evaluate the private-field mutation. Also updated the Validation section to the current head (6362719) and current test counts (71 passed), including the deferred-parse-error regression test.