fix(tools): materialize generator transform_fn results in TransformedTool.run - #5672

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8bea70ff76

Review: TransformedTool.run generator materialization (#5668)

Bug / intent

TransformedTool.run already routes through without_injected_parameters(self.fn), but that helper returns the raw callable when there is nothing to inject (dependencies.py early if not exclude: return fn). The direct dispatch branch then only handled coroutine vs plain-sync (is_coroutine_function → await, else call_sync_fn_in_threadpool), and handed the raw return to convert_result. A sync generator therefore reached serialization unconsumed → empty content / structured_content={'result': []}; an async generator function is not a coroutine function, so it took the sync path and the async-gen object was stringified as its repr, failing the structured-content check. That matches #5668 and the ctx-injected vs no-inject asymmetry.

Fix soundness

The insertion in tool_transform.py (after await / threadpool return, still inside the _current_tool try, before InputRequired* / ToolResult / convert_result) is the right place:

if inspect.isasyncgen(result):
    result = [item async for item in result]
if inspect.isgenerator(result):
    result = list(result)

That is the same contract as FunctionTool._materialize_generator (function_tool.py) and the consume block inside the injected-params wrapper — including two separate ifs rather than elif, which is what _materialize_generator uses. Keeping materialization under _current_tool so a generator body that calls forward() / forward_raw() still sees the ContextVar set is an important detail the comment calls out correctly.

The regression tests (test_transform_fn_sync_generator_yields_values / test_transform_fn_async_generator_yields_values) are stronger than a single combined smoke: they exercise both shapes via Client.call_tool and assert both data and structured_content. That covers the silent-empty and hard-fail modes from the issue.

Relationship to #5670

#5670 patches the same TransformedTool.run site for the same #5668 root cause (I left a COMMENTED review there earlier). This PR is effectively the companion/duplicate path with clearer _current_tool rationale and more thorough tests. Maintainers will want to land one and close the other (or fold) to avoid conflicting diffs on the same hunk.

Residual gaps / suggestions

1. Sync generator vs threadpool. Non-async transform_fn still constructs the generator inside call_sync_fn_in_threadpool, then list(result) runs on the event loop. FunctionTool deliberately materializes sync generators inside _wrap_body_errors(..., materialize_generators=True) so body iteration stays under the same invocation / worker-thread boundary. Not a correctness bug for the empty-result / repr failure this PR fixes, but worth aligning if a blocking sync generator transform should not stall the loop (Codex flagged the same placement concern).
2. DRY. This is a third copy of the four-line materialize (alongside FunctionTool._materialize_generator and without_injected_parameters). Reusing the static helper (or a shared util) would keep empty-gen / exception-wrapping edge cases in one place.
3. Tests (optional follow-ups). Empty generator → []; a ctx-injected transform that already worked via the deps wrapper (guards against a future double-path regression); yielding ToolResult / InputRequiredResult from a generator is exotic and non-blocking.

Verdict: Sound fix, mirrors the intended FunctionTool / deps pattern, and the tests are the right smoke for #5668. Leaving COMMENTED rather than approve only for the threadpool-parity residual, the DRY nit, and the overlap with #5670 — happy to approve once those are acknowledged or one of the two PRs is chosen as the landing vehicle.