Skip to content

fix(sdk): keep the completion a Responses stream yielded - #4772

Open
alanhuangyoo wants to merge 3 commits into
OpenHands:mainfrom
alanhuangyoo:fix/responses-stream-keeps-yielded-completion
Open

alanhuangyoo wants to merge 3 commits into
OpenHands:mainfrom
alanhuangyoo:fix/responses-stream-keeps-yielded-completion

Conversation

@alanhuangyoo

@alanhuangyoo alanhuangyoo commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

HUMAN:

Ran the streaming path and confirmed the completion the Responses stream yielded is the one kept, on both the sync and async call sites.


AGENT:

Why

responses() and aresponses() capture the ResponseCompletedEvent while draining the stream, then read it back off the wrapper:

completed_response = getattr(ret, "completed_response", completed_response)

getattr's default only applies when the attribute is absent. A wrapper that exposes completed_response and leaves it None therefore overwrites the event iteration just found, and the call fails with

LLMNoResponseError: Responses stream finished without a completed response

even though the stream completed normally. Both the sync and async paths carried the same line.

Summary

  • Take the wrapper's value only when it has one. A wrapper that sets the attribute late can still supply the completion; a stale None no longer clobbers what iteration already found.
  • Two tests, sync and async, driving a wrapper that yields the completion and also exposes completed_response = None.

Issue Number

Closes #4769

How to Test

uv run pytest tests/sdk/llm/ -q
# 981 passed

uv run ruff check && uv run ruff format --check
# clean

test_responses_streaming_keeps_the_yielded_completion and test_aresponses_streaming_keeps_the_yielded_completion both fail on main with exactly the reported error:

openhands.sdk.llm.exceptions.types.LLMNoResponseError: Responses stream finished without a completed response

and pass here, with response.raw_response is completed_response.

The test double is the smallest thing that reproduces it:

class _StaleCompletedResponseStream:
    def __init__(self, events):
        self._events = events
        self.completed_response = None      # present, and still None after iteration

    def __iter__(self):
        return iter(self._events)

Video/Screenshots

Not applicable — no GUI surface; the test output above is the evidence.

Design Doc

Not added; the change is one conditional, applied to the two paths that shared the line.

Both responses() and aresponses() capture the ResponseCompletedEvent as
they drain the stream, then read it back off the wrapper:

    completed_response = getattr(ret, "completed_response", completed_response)

getattr's default only applies when the attribute is absent. A wrapper
that exposes `completed_response` and leaves it None overwrites the
event the stream just yielded, and the call fails with

    LLMNoResponseError: Responses stream finished without a completed response

even though the stream completed normally.

Take the wrapper's value only when it has one, so it can still supply the
completion for wrappers that set it late, without a stale None clobbering
what iteration already found. Both the sync and async paths had the same
line.

Closes OpenHands#4769
@all-hands-bot

Copy link
Copy Markdown
Collaborator

👋 This PR needs a couple of things fixed before OpenHands can review it:

  • the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

@all-hands-bot

all-hands-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

🚦 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 @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

The HUMAN note is filled in; that check only re-runs on a push.
@alanhuangyoo

Copy link
Copy Markdown
Contributor Author

The only red here is Validate PR description, and it is not something I can clear from the PR side:

Linked issue(s) (#4769) carry neither `ready-for-dev` nor a pre-rollout creation date.

#4769 was filed by @yifanxiong272 on 2026-08-30, after the ready-for-dev rollout date the checker grandfathers against (READY_FOR_DEV_ROLLOUT_ISO = 2026-08-13), and it carries bug, llm, priority:medium, sdk but not ready-for-dev. Dropping the link is not a way out either — validate_linked_issue_ready errors on a PR with no linked issue at all.

So this needs a maintainer to add ready-for-dev to #4769, after which the check re-runs on the next event. Everything else on this PR is green.

Content-wise nothing has changed since the last push: the regression test fails on main with the stale wrapper state overwriting the yielded completion event, and passes here.

@alanhuangyoo

Copy link
Copy Markdown
Contributor Author

@neubig #4976 names this PR as the fix for the yielded-versus-wrapper completion bug, so here is where it stands.

I re-checked it against main at c370074, which includes #4968's llm.py changes:

  • The branch still merges into main with no conflicts.
  • With main's llm.py, both regression tests fail with LLMNoResponseError: Responses stream finished without a completed response.
  • With this change, the whole file passes (20/20).

So the bug is still on main, and the fix still applies.

The only red check is Validate PR description. It fails because #4769 has no ready-for-dev label, and I can't add that from here. Could you add it to #4769, or tell me who triages it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Responses streaming overwrites a yielded completion event with stale wrapper state

2 participants