fix: type the agent SSE payload — render event content, end the stream on a terminal event - #121
Open
ambiorix2099 wants to merge 3 commits into
Open
fix: type the agent SSE payload — render event content, end the stream on a terminal event#121ambiorix2099 wants to merge 3 commits into
ambiorix2099 wants to merge 3 commits into
Conversation
Streaming an already-terminal execution hung forever: the parse loop returned only on body EOF, and the server holds that connection open emitting nothing but heartbeat comments, which the parser discards. StreamExecution now stops once a done or error event has reached the sink, by cancelling the stream context — the client's documented shutdown path, which the service already treats as a clean stop, rather than a second shutdown mechanism. The CLI no longer depends on the server closing the connection. Cancellation is also what releases the producer: its send onto the event channel now selects on the context, so a consumer that stops reading while the buffer is full can no longer strand that goroutine. Introduces the typed EventPayload mirroring the server's AgentSSEEvent. It is what makes the terminal types checkable here, and what the renderer reads in place of untyped map lookups in the following commit. Refs #102, #116 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sink looked payload keys up by string literal, and several of those keys are not in what the server sends. thinking and error both read "message" where the server sends "content", so a failed run printed "[error]" with no reason at all. Tool-call arguments arrive as "args", not "input", and a handoff names its destination "target", not "agentName" — both confirmed against the server's AgentSSEEvent. guardrail_fail has no "reason" field either; the server puts the failure detail in "content", so that is what renders, and a payload without one leaves the guardrail name standing rather than trailing an empty separator. Reading the typed payload means the next server-side rename is a compile error instead of a blank line. terminalSink writes to an io.Writer so each event type's rendering is covered by a test. Refs #116 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard for #102 now passes: streaming a just-completed execution returns as soon as the done event arrives. run_bounded needed fixing to make the assertion meaningful. Under bats' errexit, an unguarded `wait` on the child it had just killed aborted the test with the signal status before the helper could return 124, and the `rc=$?` at the call site was equally unreachable — so the timeout the test checks for could never be observed. Refs #102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
propp-orkes
reviewed
Aug 13, 2026
There was a problem hiding this comment.
Automated review:
This PR fixes a regression where the agent stream would hang indefinitely if the server kept the connection open after a terminal event (issue #102). It implements several key improvements:
- Terminal Event Detection: The
servicelayer now detectsEventDoneandEventErrorand terminates the stream, preventing hangs on servers that keep connections open for heartbeats. - Robust Parsing: Replaced generic
map[string]anyparsing in the CLI with a strongly-typedEventPayloadstruct. This prevents silent failures if the server's JSON schema changes slightly and provides better type safety. - Goroutine Leak Prevention: Updated the SSE parser to use
context.Contextin itsselectstatements. This ensures that if a consumer stops reading (e.g., due to an error or user interruption), the producer goroutine is not stranded on a full channel. - Improved UX: The CLI now handles guardrail failures more gracefully, avoiding trailing separators if no failure reason is provided.
- Comprehensive Testing: Added extensive unit and integration tests to verify the fix and prevent future regressions in stream handling and payload parsing.
propp-orkes
reviewed
Aug 13, 2026
| w io.Writer | ||
| } | ||
|
|
||
| func newTerminalSink() terminalSink { |
There was a problem hiding this comment.
[Nit] Could we have a more clear name for this? I feel like new is a touch ambiguous and using a verb like createTerminalSink() might be better.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SSE layer read payload keys the server doesn't send, and waited for a connection close the server doesn't perform. This replaces the untyped map with a typed payload mirroring
AgentSSEEvent, so a renamed field breaks the build instead of printing blank.Rendering (#116).
thinkinganderrorreadmessage; the server sendscontent— so every thinking line was blank and a failed run printed[error]with no reason. Alsoargsnotinput, andtargetnotagentName. The issue's table was wrong on one row: guardrail-fail detail does exist, incontent.Termination (#102) — partial. The stream now ends on a terminal event rather than waiting for a close that never comes. Also closes a leak where the producer goroutine could be stranded on a full channel.
This does not close #102. The server buffers events for 5 minutes; past that it replays nothing and holds the connection open, so no terminal event ever arrives and the client still hangs. Verified: an execution that ended 41s ago now exits in ~1s, while two from ~18 minutes earlier still hang. The remaining fix is server-side:
AgentService.openStreamalready looks up the execution status for an existence check and discards the result. Completing the emitter when that status is terminal would close this at any age, without depending on the buffer.Also fixed:
run_boundedin the e2e helpers could never return 124 under bats' errexit, so the regression guard would have passed or failed for the wrong reason.Tests:
go test ./...andgo test ./internal/agent -racepass. The #102 guard is unskipped — it fails before this change and passes after, against a real server.Closes #116
Part of #102