Repository navigation
fix(openai): preserve stateless Responses output items - #3419
strandly-the-agent wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Assessment: Comment Solid, well-tested cross-SDK bug fix with good parity between Python and TypeScript. The Review themes
Nice work keeping the two SDK implementations closely aligned and preserving the legacy string fallback. |
|
@strandly-the-agent can you rebase, re-test and re-review the PR? |
8a0fab0 to
fd73f96
Compare
|
@yonib05 Done. Rebased onto The one thing I'd want before merge: a live Mantle / non-reasoning-model check that DetailsRebase — 729 commits behind. Conflicts in Gates on Review — reviewer + adversarial pass, fresh context.
Logs, diff and probe scripts: |
|
Assessment: Comment (re-review after rebase + Thanks for the thorough rebase and the re-review ledger — the resolution looks coherent with main. Most of my first-pass themes are addressed: provider isolation is now documented in the PR body and hardened by the tool-call-coverage guard, Python no longer warns on foreign Still open
Nice job keeping the two SDKs in lock-step through the rebase. |
|
@strandly-the-agent can you review comments and failed codecov tests |
|
@yonib05 Pushed Two red checks I can't clear: DetailsCodecov — the 15 uncovered lines were all in Re-review comment items
Evidence: |
|
Assessment: Comment (re-review of Every actionable item from my last pass is now addressed — thanks for the quick turnaround. Verified resolved
Remaining before merge (neither is code-side / neither is yours to clear in the sandbox):
Nice, disciplined iteration across both SDKs here. |
|
@strandly-the-agent can you address comments? |
Head branch was pushed to by a user without write access
|
@yonib05 The one open item was the live Mantle check — this sandbox turned out to have a Bedrock role, so I ran it. Good thing: it exposed a real regression. On Still not mine to clear: DetailsWhat broke — wire capture shows the adapter sends a complete Fix — when Verified live on Review — one fresh-context reviewer: approve with nits, all folded in ( Evidence: |
|
@strandly-the-agent why are we always injecting |
|
@lizradway You're right, and it turns out we don't need to send it at all — removed in Why it was there: reasoning items only replay statelessly if they carry Would it break existing endpoints? That was the real risk — it went on every stateless request (the default), so any OpenAI-compatible endpoint behind a custom Verification
Evidence: |
|
@strandly-the-agent please rebase |
5b5a6d2 to
95ad33d
Compare
|
@yonib05 Rebased onto Details
|
|
@strandly-the-agent please rebase |
Request reasoning.encrypted_content, capture ordered terminal output items, persist them on assistant messages under additionalModelResponseFields, and replay validated items verbatim on stateless turns in both SDKs. Fixes strands-agents#3389
- Python: return quietly when history lacks openaiResponsesOutput instead of warning on every foreign additionalModelResponseFields - TS: allow the SDK-optional id on function_call / custom_tool_call items - cover the real-list response.incomplete capture path
Replay drops the toolUse blocks, so items missing an emitted function_call would orphan its tool result on the next request. Omit the field in that case and let the legacy history path rebuild the calls.
Close the codecov patch gaps (configured include, replay without text fallback, foreign response fields, non-dict and SDK-model items, stateful capture skip, recovery without reasoning items), add a session round-trip test per SDK, and lift the output-item capture/replay blocks out of stream()/_format_request_messages to pull their complexity back toward main.
Mantle's gateway strips id/status from a replayed message output item in streaming mode and then rejects the request (verified against openai.gpt-oss-120b; main's tests_integ/models/test_model_mantle.py test_reasoning_content_multi_turn failed on the branch). Reasoning and function_call items replay fine, so on Mantle keep those and send the assistant turn as the string-form message instead.
Stateless Responses already return encrypted reasoning by default and the include is documented as legacy/not required; on Bedrock Mantle it is a no-op. Injecting it on every stateless request risked rejections from OpenAI-compatible endpoints for no benefit. User-configured include values still pass through unchanged.
95ad33d to
2cbeabe
Compare
|
@yonib05 Rebased onto Details
|
Description
Preserve OpenAI Responses output items across stateless turns in both SDKs.
encrypted_contenton reasoning items by default (the legacyreasoning.encrypted_contentinclude is no longer injected; user-configuredincludepasses through)response.output_item.done/ terminal output itemsadditionalModelResponseFields.openaiResponsesOutput)messageitems are sent in that string form (Mantle rejects them verbatim) while reasoning/function_call items still replayNote:
stream_messagesnow forwardsadditionalModelResponseFieldsfor every provider (namespaced per provider), so Bedrock users withadditionalModelResponseFieldPathswill also see it persisted on assistant messages and in sessions.Related Issues
Fixes #3389
Related to #3388 and #3399
Documentation PR
No documentation change needed; this restores adapter round-trip behavior.
Type of Change
Bug fix
Testing
Rebased onto
main7939e67 on 2026-10-16 (squashed; conflicts inopenai_responses.py,streaming.pyand two Python test files resolved — main's #5074content_filterand #4139 cut-off-tool-call handling are kept as-is). Follow-ups:34c422e30/ff939261f(re-review fixes),6177fad9f(codecov gaps, session round-trip tests, complexity),d099713ec/c2b8746c3(Mantle replay fix found by the live run),5b5a6d208(stop injecting theinclude, perlizradway). Rebased again ontomain74b6790 on 2026-10-21 (one conflict: main's new_log_skipped_blockscall in_format_request_messages, kept alongside this PR's removal of the trailing empty-content filter) and ontomainbaf15f5 on 2026-10-28 (one conflict: main hoistedconst statefulin TS_streamResponses; kept it and passed the Mantle replay option). Gates on the current head2cbeabead:TypeScript
--check,tsc --noEmit(src) — passsrc/telemetry/__tests__/config.test.ts— the same 2 fail identically on cleanorigin/mainin this sandbox (unrelated)tscovertest/integreports 539 errors — identical count on cleanorigin/main(pre-existing / sandbox env)Python
ruff format --check,ruff check— pass./src ./tests_typing— pass, 366 files (run with--cache-dir /tmp --no-sqlite-cache; the NFS workspace locks mypy's sqlite cache)hatch test -py 3.13full unit suite — 8147 passed, 38 skippedLive Bedrock Mantle (
tests_integ/models/test_model_mantle.py, us-east-1, from the sandbox's Bedrock role)6177fad9f: 4 passed, 1 failed —test_reasoning_content_multi_turn(passes on cleanmain, so the PR regressed it). Wire capture showed the adapter sent a completemessageoutput item (id,status,annotations), but Mantle's streaming path stripsid/statusbefore its internal validation and rejects the request (SubmitRequestFailure, 219 pydantic errors, all on themessageitem).reasoningandfunction_callitems replay fine; the string-form assistant message is accepted.openai.gpt-5.4on Mantle accepts every shape; onlyopenai.gpt-oss-120b(the/v1path) fails. Seemantle-replay-shapes.log.d099713ec): whenbedrock_mantle_config/bedrockMantleConfigis set,messageitems are dropped from the replay and the existing string-form text fallback is used; reasoning and function_call items still replay. Non-Mantle behaviour is unchanged.c2b8746c3: 5 passed; two-turn tool-use probe passes onopenai.gpt-oss-120bandopenai.gpt-5.4(mantle-integ-fixed.log,mantle-integ-final.log).include: ["reasoning.encrypted_content"]was accepted by both Mantle models but is a no-op there: identical reasoning items with or without it (mantle-include.log). OpenAI's reasoning guide now states stateless mode returnsencrypted_contentby default and the include is "legacy … not required", so5b5a6d208removes the injection; Mantle suite still 5/5 without it (mantle-integ-no-include.log), and 5/5 again on each rebased head (round9-rebase-gates.log,round10-rebase-gates.log).Coverage (codecov patch gaps,
6177fad9f) — 9 new tests: configuredincludealready containingreasoning.encrypted_content, replay with no text fallback, foreignadditionalModelResponseFields(no warning), non-dict items, SDK-model → dict conversion + unsupported type, stateful mode skipping capture, recovery with non-list / no-reasoning / foreign fields (3 params), plus one session round-trip test per SDK (test_additional_model_response_fields_round_tripthroughSnapshotSessionManager; TStakeSnapshot→ JSON →loadSnapshot). Local branch coverage:_recover_message_on_max_tokens_reached.py100%;openai_responses.py97% — the remaining partials are pre-existing_streamevent-loop back-edges, not lines this PR adds.Complexity — the capture/replay blocks were lifted into
_capture_output_item,_capture_response_output,_persisted_output_fields,_replay_output_items. complexipy:stream()152 → 132 (main: 127);_format_request_messages67 → 57 (main: 55). Thecomplexity/highlabel reflects main's baseline forstream(), not this PR.Checks I cannot clear myself:
check-api-review-labelfails until a maintainer addsapi/review-completealongside the existingapi/needs-review;2 Approvers for Botsneeds two human approvals.Evidence (logs, diff, probe scripts):
s3://strandly-prod-artifacts-319426459652/strands-agents/harness-sdk/pr/3419/—mantle-integ.log(failure on6177fad9f),mantle-integ-main.log,mantle-replay-shapes.log,mantle-tools-branch.log,mantle-integ-fixed.log,mantle-integ-final.log,mantle_*.pyprobes,round5-full-gates.log,round6-gates.log.Response to the github-actions re-review (2026-10-16 / 2026-10-20)
api/needs-reviewis applied; the failingcheck-api-review-labelcheck wantsapi/review-complete, which only a designated API reviewer should add. Design questions for that review are in the ledger below.6177fad9f.stream()complexity — reduced as above.d099713ec. Still unverified:api.openai.com/ Azure / custombase_url(no provider key in the sandbox).Independent review ledger (reviewer + adversarial passes, fresh context)
No 🔴 from any review pass. Rebase resolution judged coherent with main; nothing in the PR is redundant with main.
Fixed in 34c422e
response.incompleteoutput list →test_stream_captures_incomplete_output_itemsadditionalModelResponseFieldsmerely lackedopenaiResponsesOutput(e.g. Bedrock-sourced) → returns quietly, matching TSidon every item; it is optional onfunction_call/custom_tool_callin openai 6.45.0 → made optional, test addedstreaming.pyrevertedFixed in ff93926 (adversarial, repro in
adv-probe-c.logC1 /adv-probe-b.logB4)toolUseblocks, so if the captured items lacked afunction_callfor an emitted call (terminal event with empty/absentoutputand nooutput_item.donefor it, or a stream ending without a terminal event) the next request carried an orphanedfunction_call_output— permanently, since the message is persisted. Both SDKs now only attach the field when capturedfunction_callids cover every emitted tool call; tests fail without the guard.Fixed in d099713 / c2b8746 (live Mantle run + one independent reviewer)
messageoutput items in streaming mode → Mantle-scoped string-form fallback formessageitems only. Reviewer verdict: approve with nits; Python/TS parity verified case-by-case; non-Mantle behaviour unchanged._mantleused!== undefined, so a JS caller passingbedrockMantleConfig: nullwould get the Mantle workaround on a plain OpenAI client →Boolean(bedrockMantleConfig).formatResponsesMessagesmerged.[reasoning, message, function_call]+ text/toolUse →[reasoning, function_call, string text]) added in both SDKs; the reviewer verified that order live on both Mantle models.messageindex instead of appending — the appended order is verified accepted and keeps a single fallback path. Rejected: applying the string-form fallback globally — verbatim replay is the point of OpenAI Responses adapter drops reasoning across multi-turn (no output-item round-trip) #3389, and the only endpoint it would hurt (api.openai.com) is the one we can't test.Open — maintainer call, deliberately not changed in a rebase
🟡— resolved ininclude: ["reasoning.encrypted_content"]on every stateless request5b5a6d208by not injecting it at all: OpenAI returnsencrypted_contentby default in stateless mode (include documented as legacy), and on Mantle it was a verified no-op. Removes the blast-radius risk on OpenAI-compatible endpoints; users on a backend that still needs the legacy include can set it inparams.function_callwhose result follows — design choice for maintainers.recover_message_on_max_tokens_reached(provider-agnostic) hardcodes the OpenAI field name; TS has no equivalent filtering. Layering question.reasoningitem followed by a plain assistant text message. Accepted by both Mantle models live (shape E/B inmantle-replay-shapes.log); unverified onapi.openai.com._normalize_messages'INVALID_TOOL_NAMErewrite ([BUG] Invalid tool names result in dangling toolUse blocks #1069) — only reachable when a model emits an invalid tool name; cheap follow-up guard possible.client_args={"base_url": …}/clientConfig.baseURLat Mantle by hand still gets verbatimmessageitems.Rejected
content_filtered: not a regression — main already replays an orphanedfunction_callfrom thetoolUseblock on a filtered turn; reasoning-only filtering there would be a main-side follow-up.Angles that found nothing: SlidingWindow (window 2–7, pinned prefixes) and Summarizing conversation managers,
output_item.donevs terminal-output ordering,incompletewithoutput=[]/None, FileSessionManager +SessionMessageround-trip, invalid-item fallback, redaction popping the field, Bedrock/other providers rebuilding{role, content}so the field never leaks upstream.hatch run prepare(equivalent commands run individually; see above)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.