fix(client): SSE spec compliance, BOM tolerance, bounded decode, accumulator guards

[Emasoft]

Summary

Four pre-existing robustness bugs in the client/executor streaming and decode paths, found by a 4-subsystem audit. All four fail silently or catastrophically on malformed-but-legal server output.

Bugs and evidence

1. SSE data: lines without a space were silently dropped. The stream loop matched strings.HasPrefix(line, "data: ") (colon+space) only. The SSE spec makes the single space after the colon optional, so a provider or proxy emitting spec-legal data:{...} had every such chunk vanish — no error, no log, no retry: the scanner just continued past them. A turn could commit an arbitrarily truncated response, and none of the retry tiers ever saw the loss.

2. A leading UTF-8 BOM broke the first chunk. Gateways and proxies sometimes glue \ufeff to the first line of an SSE stream or to a JSON body. Unstripped, the BOM broke the data: prefix match (chunk silently dropped, as above) and made encoding/json reject an otherwise healthy non-streaming 200 body with a confusing parse error.

3. Non-streaming 200 bodies were decoded unbounded (memory DoS). ChatCompletion and Completion decoded resp.Body directly. The error path was already bounded (maxErrorBodyBytes), but a broken or hostile server streaming an unbounded success body could exhaust memory through the decoder.

4. StreamAccumulator.Append panicked on a negative tool-call delta index and cross-merged out-of-order deltas. The merge condition if index < len(a.ToolCalls) indexed the slice for ANY index below the length: a malformed index: -1 panicked with index-out-of-range, killing the whole CLI; an out-of-order delta (e.g. index 5 arriving before index 0) spliced its arguments into another tool call slot, corrupting both entries.

Fixes

• Match on "data:" and strip one optional space: strings.TrimPrefix(strings.TrimPrefix(line, "data:"), " ").
• Strip the UTF-8 BOM from the first stream line and from non-streaming bodies before decoding.
• Read non-streaming 200 bodies through io.LimitReader(resp.Body, maxCompletionBodyBytes) (8 MiB) before json.Unmarshal, mirroring the existing error-path bound.
• Merge a delta into a slot only when index >= 0 && index < len(a.ToolCalls) && a.ToolCalls[index].Index == index; deltas matching no slot are appended as new entries, preserving the data.

StreamAccumulator exists on this base (internal/executor/executor.go), so the accumulator guard is included here rather than riding with a future PR.

Testing

• gofmt -l . — no findings in touched files
• go build ./... — pass
• go vet ./internal/client ./internal/executor — pass
• go test -race -count=1 ./... — all 15 packages ok
• golangci-lint run ./internal/client/... ./internal/executor/... (v2.13.2) — 0 issues

Regression tests added: internal/client/stream_parse_test.go (SSE data-line variants: colon+space, colon-no-space, mixed shapes, BOM on first line, BOM+no-space; BOM body decode; oversized-body bound; normal-body sanity) and internal/executor/stream_accumulator_guard_test.go (negative-index no-panic, out-of-order no-cross-merge).

Same fixes landed on these open PRs

The same client/executor hunks were ported to the open PR branches that carried the buggy code: #129 (feat/otp-gate), #130 (fix/subagent-hang), #131 (feat/subagent-control), #132 (fix/fleet-pacing), #136 (feat/todos-pane-default-open), #141 (feat/config-strict-parsing), #142 (feat/config-cli-parity), #146 (feat/subagent-persistence), #149 (feat/agent-resume). #139 (feat/jev-compaction) already carried them.

Scope note (per review)

The parseRetryAfterAt tests at the end of internal/client/stream_parse_test.go (TestParseRetryAfterAt_ExtremeValues, TestParseRetryAfterAt_HugeButValidSecondsDoesNotPanic) are not related to this PR's four fixes: they came with the Retry-After / retry-hardening changes this PR builds on and were carried into the file when it was created. They are called out here for scope instead of being split into a separate PR, as suggested in review. (Review fixes themselves landed in 6b7158d: index-field accumulator slot lookup, bare data: keepalives skipped, and a clear oversized-body error.)

[mlhher]

@Emasoft

Thank you, the SSE data: handling, BOM stripping, body bound and negative-index guard all look good, and the tests cover them well. A few things to look at before merging:

1. Accumulator still mis-handles non-zero-based indices. If a provider numbers tool calls from 1 and interleaves fragments (e.g. {Index:1, ID:"a", name, args}, {Index:2, ID:"b", ...}, then fragment {Index:1, args}, fragment {Index:2, args}), the fragments end up as extra entries with no ID or name. Before this PR they were merged into the wrong call. The .Index == index check avoids the panic, but the lookup is still by slice position. Finding the slot where ToolCalls[i].Index == index would handle it properly.

2. Bare data: lines now end the stream. The existing data == "" check breaks the loop, and a bare data: line used to be skipped. A server using one as a keepalive would now end the turn early. Skipping empty-data lines instead of breaking would avoid that, or please note the change in the description.

3. Oversized-body error (minor). The LimitReader truncates silently, so an oversized body fails with "unexpected end of JSON input". Reading max+1 bytes and returning a "response body too large" error would be clearer.

4. Scope. The test file also adds parseRetryAfterAt tests that aren't related to this PR. Please split them out or mention them in the description.

One more thing: unlike #127, this PR doesn't include the CLA checkbox from the template. I'm assuming that's because your statement on #127 ("my contributions to this project") is meant to cover your contributions generally, and I've treated it that way. If that's not what you intended, please say so here, and the CLA would then need to be confirmed per PR. Otherwise there's nothing to do on this point.

[Emasoft]

Thank you for the careful review — all four points are addressed in 6b7158d.

1. Accumulator non-zero-based indices — fixed. StreamAccumulator.Append now locates the slot by its Index field (the first ToolCalls[i].Index == index) and appends a new slot only when no slot carries that index, so a provider numbering calls from 1 and interleaving fragments (create 1, create 2, fragment 1, fragment 2) merges each fragment into its own call. The lookup only ever ranges over the slice, so the no-panic guarantee is unchanged; a repeated negative index now merges into its own (broken) slot, which the updated guard test pins. New regression test: TestStreamAccumulator_AppendOneBasedInterleavedFragmentsMergeByIndex (your exact example), plus the in-order and out-of-order guard tests still passing.

2. Bare data: lines — fixed. Empty data lines are now skipped (continue) instead of ending the loop: they are keepalives/heartbeats, and breaking on them ended the turn early with a truncated response no retry tier ever saw. New test TestChatCompletionStream_EmptyDataKeepaliveDoesNotEndStream sends a keepalive between two real chunks and asserts the stream completes with both.

3. Oversized body — fixed. Both non-streaming reads (ChatCompletion, Completion) now read maxCompletionBodyBytes+1 bytes through the LimitReader; the presence of the +1 byte returns the new errors.As-able *BodyTooLargeError — "response body too large (> 8388608 bytes)" — instead of the silently truncated read failing decode with unexpected end of JSON input. Tests pin the exact max+1 boundary, the at-limit body still decoding, and the oversized case.

4. Scope — noted in the description. A "Scope note (per review)" section now explains that the parseRetryAfterAt tests in stream_parse_test.go came with the Retry-After / retry-hardening changes this PR builds on, rather than splitting them into a separate PR.

CLA: yes — my statement on #127 ("my contributions to this project") is meant to cover my contributions generally, including this PR; I just missed the template checkbox here. If you'd rather have it confirmed per PR going forward, say so and I'll include it on each one.

Gates on 6b7158d: gofmt clean on touched files, go build ./..., full go test -race -count=1 ./... (all packages ok), golangci-lint run ./internal/client/... ./internal/executor/... (v2.13.2) — 0 issues.

[mlhher]

Thanks!