http.Server.WriteTimeout is an absolute deadline from when the request header was read, not an idle timeout. cmd/pansy/main.go:65 sets it to 30s; an agent turn is budgeted 4 minutes. Nothing cleared it for the SSE response, so every turn over 30 seconds was cut mid-stream.
Constant
Value
runTimeout (internal/agent/runtime.go:31)
4 minutes
keepAliveInterval (internal/api/agent.go:32)
20 seconds
WriteTimeout (cmd/pansy/main.go:65)
30 seconds
The 4-minute budget was unreachable. And the keep-alive from #73 — added so "a long silence while the model thinks doesn't look like a dead connection" — got exactly one tick before the connection it was pacing was destroyed underneath it. A mechanism built to survive long silences that was structurally incapable of surviving them.
Why it survived review
The failure is invisible from the handler. Writes made after the deadline return err == nil and their bytes are silently discarded.
This matters for how it was found and fixed. The sweep that flagged it predicted the writes would "fail with i/o timeout, silently (the error is discarded)" at agent.go:147 — pointing at the _, _ = io.WriteString(...). That diagnosis is wrong: there is no error at any layer, so logging the discarded write error would have caught nothing and the bug would have survived a fix aimed squarely at it. Verified against a standalone reproduction before writing the patch.
Only the client sees it — as unexpected EOF, which web/src/lib/agent.ts:171-176 reports as "The connection dropped partway through.", indistinguishable from a real network fault.
The fix
One line in openEventStream, with the error checked rather than ignored — a failure there means the stream will be cut and we can't prevent it, which is worth saying out loud.
Confirmed gin 1.10.1's responseWriter implements Unwrap() http.ResponseWriter (response_writer.go:54), so http.NewResponseController reaches the underlying conn. Without that this would silently no-op, so it was worth checking rather than assuming.
The test
Because the failure can't be observed server-side, TestEventStreamOutlivesWriteTimeout drives a real http.Server with a short WriteTimeout and asserts from the client side.
Against the unfixed code:
--- FAIL: TestEventStreamOutlivesWriteTimeout (0.81s)
client read error after 1/4 frames: unexpected EOF
client received 1 frames, want 4 — the stream was cut at WriteTimeout
That's the production symptom reproduced in a unit test. A test that inspected the return value of io.WriteString would have passed against the bug — which is the same failure mode as the undo-test fixture in #72, so it seemed worth being explicit about in the test's own comment.
Also passes under -race.
Not in scope
ReadTimeout: 15s has the same absolute-deadline shape and will matter for image upload in #80 (a phone photo over a slow connection). Flagged there rather than pre-emptively changed here.
GOWORK=off go test ./... green; gofmt -l internal/ clean.
Closes #78. Live bug — currently deployed.
## What was wrong
`http.Server.WriteTimeout` is an **absolute deadline from when the request header was read**, not an idle timeout. `cmd/pansy/main.go:65` sets it to 30s; an agent turn is budgeted 4 minutes. Nothing cleared it for the SSE response, so every turn over 30 seconds was cut mid-stream.
| Constant | Value |
|---|---|
| `runTimeout` (`internal/agent/runtime.go:31`) | 4 minutes |
| `keepAliveInterval` (`internal/api/agent.go:32`) | 20 seconds |
| **`WriteTimeout`** (`cmd/pansy/main.go:65`) | **30 seconds** |
The 4-minute budget was unreachable. And the keep-alive from #73 — added so "a long silence while the model thinks doesn't look like a dead connection" — got exactly one tick before the connection it was pacing was destroyed underneath it. A mechanism built to survive long silences that was structurally incapable of surviving them.
## Why it survived review
**The failure is invisible from the handler.** Writes made after the deadline return `err == nil` and their bytes are silently discarded.
This matters for how it was found and fixed. The sweep that flagged it predicted the writes would "fail with `i/o timeout`, silently (the error is discarded)" at `agent.go:147` — pointing at the `_, _ = io.WriteString(...)`. That diagnosis is wrong: there is no error at any layer, so logging the discarded write error would have caught **nothing** and the bug would have survived a fix aimed squarely at it. Verified against a standalone reproduction before writing the patch.
Only the client sees it — as `unexpected EOF`, which `web/src/lib/agent.ts:171-176` reports as *"The connection dropped partway through."*, indistinguishable from a real network fault.
## The fix
One line in `openEventStream`, with the error checked rather than ignored — a failure there means the stream *will* be cut and we can't prevent it, which is worth saying out loud.
Confirmed `gin` 1.10.1's `responseWriter` implements `Unwrap() http.ResponseWriter` (`response_writer.go:54`), so `http.NewResponseController` reaches the underlying conn. Without that this would silently no-op, so it was worth checking rather than assuming.
## The test
Because the failure can't be observed server-side, `TestEventStreamOutlivesWriteTimeout` drives a **real `http.Server`** with a short `WriteTimeout` and asserts from the **client** side.
Against the unfixed code:
```
--- FAIL: TestEventStreamOutlivesWriteTimeout (0.81s)
client read error after 1/4 frames: unexpected EOF
client received 1 frames, want 4 — the stream was cut at WriteTimeout
```
That's the production symptom reproduced in a unit test. A test that inspected the return value of `io.WriteString` would have passed against the bug — which is the same failure mode as the undo-test fixture in #72, so it seemed worth being explicit about in the test's own comment.
Also passes under `-race`.
## Not in scope
`ReadTimeout: 15s` has the same absolute-deadline shape and will matter for image upload in #80 (a phone photo over a slow connection). Flagged there rather than pre-emptively changed here.
`GOWORK=off go test ./...` green; `gofmt -l internal/` clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
http.Server.WriteTimeout is an ABSOLUTE deadline from when the request header
was read, not an idle timeout. pansy sets it to 30s (cmd/pansy/main.go:65)
while an agent turn is budgeted 4 minutes, so every turn over 30 seconds was
cut mid-stream. The 4-minute runTimeout was unreachable; the real ceiling was
30 seconds.
That also meant the keep-alive added in #73 could never do its job. It ticks
at 20s, so it got exactly one tick before the connection it was pacing was
destroyed underneath it — a mechanism built to survive long silences that was
structurally incapable of surviving them.
The failure is invisible from the handler: writes made after the deadline
return err == nil and their bytes are discarded. There is no error to check
and none to log, which is why this survived a review that specifically looked
at the discarded write error. Only the client sees it, as an unexpected EOF,
which web/src/lib/agent.ts reports as "The connection dropped partway
through." — indistinguishable from a real network fault.
Because it is invisible server-side, the test drives a real http.Server with a
short WriteTimeout and asserts from the CLIENT side. Against the unfixed code
it gets 1 of 4 frames and an unexpected EOF; a test that inspected the return
of io.WriteString would have passed against the bug.
gin 1.10.1's responseWriter implements Unwrap, so ResponseController reaches
the underlying conn. A failure to clear the deadline IS worth logging: it
means the stream will be cut and we cannot prevent it.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 01:53:10Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ⚠️ **security** — could not complete
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ⚠️ **performance** — could not complete
- ⚠️ **error-handling** — could not complete
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ⚠️ **performance** — could not complete
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Gadfly, security lens: clearing the write deadline outright traded the
truncation bug for an unbounded one. With no deadline, a client that stops
reading fills the socket buffer and blocks Write forever — pinning the agent
run goroutine and this stream's mutex, which also takes down the keep-alive
since it needs the same lock. The old 30s WriteTimeout at least bounded that.
Refresh the deadline per frame instead: the stream as a whole is unbounded,
but no single write is. That is the only shape that satisfies both ends, since
an absolute deadline cuts long turns and no deadline cannot be recovered from.
The probe stays in openEventStream so a writer that cannot take deadlines is
reported once rather than once per frame; the per-write call deliberately
ignores its error for the same reason.
Also widened the test's timing margins, per the second finding. tick is now
GREATER than writeTimeout, so every frame lands after the deadline has already
expired and the margin only ever grows — a loaded CI runner pushes this toward
passing rather than toward flaking. Sized the other way it would flake exactly
when CI is busiest. Passes 3/3 with -count=3.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Both findings taken, and the security one was a genuine catch — I'd traded one unbounded failure for another.
Clearing the deadline entirely removes the backstop. Correct. With no deadline, a client that stops reading fills the socket buffer and blocks Write forever, pinning the agent run goroutine and this stream's mutex — which takes the keep-alive down with it, since it needs the same lock. The old 30s WriteTimeout at least bounded that.
Fixed by refreshing the deadline per frame rather than removing it: the stream as a whole is unbounded, but no single write is. That's the only shape that satisfies both ends — an absolute deadline cuts long turns, and no deadline can't be recovered from. sseWriteTimeout is deliberately generous (30s) because it's a backstop against a stuck peer, not a pacing mechanism.
The probe stays in openEventStream so a writer that can't take deadlines is reported once instead of once per frame; the per-write call ignores its error for that reason, which the comment now states.
Test timing margin. Also correct. Restructured so tick (300ms) is greater than writeTimeout (200ms), which means every frame lands after the deadline has already expired and the first clears it by 100ms. The margin only grows: a slow or loaded runner delays frames further past a deadline that's already passed, so jitter pushes the test toward passing. Sized the other way — frames landing just before a deadline — it would flake exactly when CI is busiest. Verified with -count=3.
Re-verified the regression detection after both changes: with the deadline handling removed the test fails at 0/3 frames with unexpected EOF.
One thing I did not add: a test for the new bounded-write property. It needs a client that stalls mid-stream and then a 30-second wait, which isn't worth the suite time. Flagging it rather than pretending it's covered.
@gadfly review
Both findings taken, and the security one was a genuine catch — I'd traded one unbounded failure for another.
**Clearing the deadline entirely removes the backstop.** Correct. With no deadline, a client that stops reading fills the socket buffer and blocks `Write` forever, pinning the agent run goroutine *and* this stream's mutex — which takes the keep-alive down with it, since it needs the same lock. The old 30s `WriteTimeout` at least bounded that.
Fixed by refreshing the deadline **per frame** rather than removing it: the stream as a whole is unbounded, but no single write is. That's the only shape that satisfies both ends — an absolute deadline cuts long turns, and no deadline can't be recovered from. `sseWriteTimeout` is deliberately generous (30s) because it's a backstop against a stuck peer, not a pacing mechanism.
The probe stays in `openEventStream` so a writer that can't take deadlines is reported once instead of once per frame; the per-write call ignores its error for that reason, which the comment now states.
**Test timing margin.** Also correct. Restructured so `tick` (300ms) is *greater* than `writeTimeout` (200ms), which means every frame lands after the deadline has already expired and the first clears it by 100ms. The margin only grows: a slow or loaded runner delays frames further past a deadline that's already passed, so jitter pushes the test toward passing. Sized the other way — frames landing just before a deadline — it would flake exactly when CI is busiest. Verified with `-count=3`.
Re-verified the regression detection after both changes: with the deadline handling removed the test fails at 0/3 frames with `unexpected EOF`.
One thing I did **not** add: a test for the new bounded-write property. It needs a client that stalls mid-stream and then a 30-second wait, which isn't worth the suite time. Flagging it rather than pretending it's covered.
🪰Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡Regression test never approaches the 30s deadline boundary, so it can't catch a regression to a single set-once deadline instead of the required per-frame refresh
error-handling · flagged by 1 model
internal/api/sse_deadline_test.go:20-45 — The regression test doesn't exercise the "refreshed per frame" mechanism that is the actual fix for #78. openEventStream sets the write deadline via the hardcoded 30s sseWriteTimeout (internal/api/agent.go:120) once, at stream open, and that constant isn't overridable by the test. The test only overrides srv.Config.WriteTimeout to 200ms (internal/api/sse_deadline_test.go:46) but total runtime is frames(3) * tick(300ms) ≈ 900ms — three ord…
🪰 Gadfly · advisory
🟡 **Regression test never approaches the 30s deadline boundary, so it can't catch a regression to a single set-once deadline instead of the required per-frame refresh**
_error-handling · flagged by 1 model_
- `internal/api/sse_deadline_test.go:20-45` — The regression test doesn't exercise the "refreshed per frame" mechanism that is the actual fix for #78. `openEventStream` sets the write deadline via the hardcoded 30s `sseWriteTimeout` (`internal/api/agent.go:120`) once, at stream open, and that constant isn't overridable by the test. The test only overrides `srv.Config.WriteTimeout` to 200ms (`internal/api/sse_deadline_test.go:46`) but total runtime is `frames(3) * tick(300ms) ≈ 900ms` — three ord…
<sub>🪰 Gadfly · advisory</sub>
Verdict: No material issues found · 0 findings (0 with multi-model agreement)
No material issues found by consensus.
Per-model detailclaude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens (authn/authz, injection, SSRF, deserialization, secret leakage, input validation, unsafe handling of untrusted data, insecure defaults).
What I checked:
internal/api/agent.go:58-109 (agentChat) — mustActor(c) and the existing h.svc.AgentHistory permission check run beforeopenEventStream is called, and this diff doesn't touch that ordering, so authz gating is unaffected.
The per-frame SetWriteDeadline refresh (agent.go:189) and the one-time probe in openEventStream (agent.go:161-168) only touch a net.Conn deadline — no user-controlled data flows into either call. sseWriteTimeout is a fixed, unexported package var; the test mutates it in-package only (sse_deadline_test.go), not reachable from any request.
The slog.Error call at agent.go:167 logs only the error from SetWriteDeadline (an internal net/http error), no request data, no credentials.
Confirmed no new user input parsing, no new outbound requests (no SSRF surface), no new serialization/deserialization path — this is purely a connection-deadline change in an already-open, already-authorized stream.
Considered whether refreshing the deadline per-frame (vs. the previous absolute WriteTimeout) creates a resource-exhaustion angle: a client that stops reading can still only pin the write for sseWriteTimeout (30s) per attempt before the write times out, and the agent turn itself is independently capped by runTimeout (4 min, unchanged by this PR) — so worst case is the same order of magnitude as before, not a new unbounded hold. The author's own comments in agent.go:111-124 and 144-156 show this tradeoff was deliberately reasoned about, not overlooked.
Nothing in this diff introduces or weakens an authn/authz check, and there's no injection, SSRF, deserialization, or secret-leakage vector — it's a narrowly-scoped connection-deadline fix on an already-authenticated stream.
🎯 Correctness — No material issues found
Only one construction site, always fully initialized. I've verified the deadline math, the timeout constants against their real values, the gin/http.ResponseController wiring, the mutex serialization, and the two new tests' logic — no correctness issues surfaced.
Review
VERDICT: No material issues found
Findings, all verified against the checked-out code:
sseWriteTimeout (30s) matches the original WriteTimeout at cmd/pansy/main.go:65, and keepAliveInterval (20s, internal/api/agent.go:32) stays comfortably under it, so the keep-alive tick refreshes the deadline before it can expire during a quiet stretch — the numbers line up correctly.
runTimeout is confirmed at 4 minutes (internal/agent/runtime.go:31), consistent with the PR's claim that the per-frame refresh (not a single extended deadline) is what's needed to outlast the server's absolute WriteTimeout.
SetWriteDeadline is called immediately before every write while holding s.mu, and no write is ever in flight when a new deadline is set (writes are serialized by the mutex), so there's no race between refreshing the deadline and an in-progress write.
Go's net/http treats SetWriteDeadline as "last call wins" on the underlying conn (module is on go 1.26.2, well past the 1.20 minimum for http.ResponseController), so re-arming it per frame genuinely supersedes the server's original absolute deadline — the mechanism does what the PR claims.
No other middleware in the router (internal/api/api.go:39 only adds sloggin + gin.Recovery()) wraps c.Writer, so the Unwrap() chain the fix depends on isn't broken by anything else in this codebase.
eventStream has exactly one construction site (agent.go:161), always fully initializing rc, so there's no nil-ResponseController path.
The two new tests correctly distinguish the fixed/unfixed behavior: TestEventStreamOutlivesServerWriteTimeout leaves sseWriteTimeout at its 30s default against a 300ms server timeout (would fail pre-fix), and TestEventStreamRefreshesDeadlinePerFrame shrinks sseWriteTimeout to 400ms with 100ms-spaced frames against a generous 5s server timeout, which only passes if the deadline is refreshed per-write rather than once at open.
🧹 Code cleanliness & maintainability — No material issues found
Confirmed the 30s WriteTimeout referenced in the diff's comments matches cmd/pansy/main.go:65. The diff itself is unchanged there — this PR is scoped correctly to internal/api/agent.go and its test.
I've reviewed the full diff against the checked-out code. This is a clean, tightly-scoped change.
Verdict: No material issues found
Through the code cleanliness/maintainability lens specifically:
No duplication introduced: the two new tests share the streamFrames helper rather than copy-pasting server setup (internal/api/sse_deadline_test.go:18-49).
Struct/function sizes stay small: openEventStream and write remain short, single-purpose.
Naming and field additions fit the file's existing conventions (rc *http.ResponseController alongside c *gin.Context, mu sync.Mutex — verified against internal/api/agent.go:132-136).
The heavily narrative comment style (three separate blocks explaining the deadline rationale at the var, the function, and the write call) matches the pre-existing style already used throughout this same file (e.g. the keepAlive, chatErrorMessage, and agentChat doc comments), so it isn't a new maintainability wart introduced by this PR — it's consistent with the surrounding code.
Verified gofmt-relevant formatting by eye (tabs, brace placement) matches surrounding code; no stray whitespace or import churn (net/http was already imported).
No dead code, no leftover TODO, no copy-paste that should have been factored out.
Nothing in this diff, viewed purely for readability/structure/duplication/naming, rises to a reportable finding.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens only.
The change adds one SetWriteDeadline call per SSE frame (internal/api/agent.go:187) inside the existing write() critical section. I checked how often write()/send() actually fire on this path:
agentChat (internal/api/agent.go:60-108) sends one chatEvent per completed model round trip (a Step), bounded by maxSteps = 24 (internal/agent/runtime.go:27), plus one final Done/Warning event.
The keep-alive ticker (internal/api/agent.go:32, keepAliveInterval = 20s) adds at most ~12 more frames over the 4-minute runTimeout.
So this is at most a few dozen writes over a multi-minute stream — not a hot loop, not a per-token streaming path. http.ResponseController.SetWriteDeadline on a TCP conn is a cheap netpoller timer reset, not a syscall-heavy operation, and it's already inside the same mutex-guarded critical section as the write it protects, so it adds no new lock contention. The http.ResponseController itself is allocated once in openEventStream (internal/api/agent.go:160) and stored on eventStream, not reallocated per frame.
No N+1s, no unbounded growth, no missing pagination, no quadratic behavior, and no new blocking-call concerns beyond what the PR itself documents (the deadline is explicitly a bounded backstop, not a pacing mechanism). The new test spins up a real http.Server with sub-second timeouts and closes it via defer srv.Close() — bounded and self-contained.
Nothing to flag in this lane.
🧯 Error handling & edge cases — No material issues found
Verification
Read internal/api/agent.go in full against the diff. Every claim in the draft's reasoning checks out against the actual code:
sseWriteTimeout is a package-level var (30s), reassigned only in the test (sse_deadline_test.go), confirmed unused elsewhere in production code.
write() ignores the SetWriteDeadline error return (_ = s.rc.SetWriteDeadline(...)) while openEventStream logs it once via slog.Error — matches the stated "probe once, refresh silently" design.
The mutex (s.mu) is held only across a single write() call, so a stalled client blocks at most for one write, bounded by sseWriteTimeout (30s), not indefinitely — confirmed no other lock ordering issue with keepAlive's goroutine, which also goes through s.write() under the same lock.
keepAlive's stop is sync.Once-guarded and selects on both an explicit done channel and c.Request.Context().Done(), so no goroutine leak.
http.NewResponseController(c.Writer) against gin's ResponseWriter — gin has implemented Unwrap() http.ResponseWriter since v1.9.0 specifically to support http.ResponseController, and go.mod pins gin-gonic/gin v1.10.1, so this resolves correctly (not confirmable via local build due to sandbox restrictions outside the repo directory, but consistent with known gin API history).
http.Flusher.Flush() has no error return, so nothing is silently swallowed there beyond what's already documented.
The draft contains no bulleted findings to individually confirm or reject — it is a clean "no issues" verdict, and independent reading of the diff supports that verdict for the error-handling/edge-case lens.
VERDICT: No material issues found
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens. The diff replaces an absolute http.Server.WriteTimeout with a per-write deadline on the SSE response. Security-relevant observations:
No DoS regression / resource exhaustion. The change keeps a per-write deadline (sseWriteTimeout = 30s) rather than removing deadlines entirely. A client that stops reading will still have its writes time out within 30s once the socket buffer fills — the run goroutine and the stream mutex are not pinned indefinitely. The overall turn is still bounded by runTimeout (4 min). So this does not create a new vector for connection-goroutine exhaustion; if anything it makes the intended long-lived-stream case explicit while preserving the stuck-peer backstop.
No injection / untrusted-input path touched. The only new inputs are time.Now() and the package-level sseWriteTimeout constant — no request data flows into SetWriteDeadline or the response body. The frame content still goes through json.Marshal in send (agent.go:174), unchanged.
No authn/authz change.openEventStream is called after mustActor(c) and request binding in agentChat (agent.go:58-72); the auth surface is untouched.
sseWriteTimeout is a var, not const. It's package-level and reassignable, but only the test mutates it (under t.Cleanup restore). Production never reassigns it, and it's not exported, so no external tampering surface. Not a security concern.
http.NewResponseController(c.Writer) — no credential/secret handling involved; the PR description's verification of gin's Unwrap() is correctness, not security.
Nothing in my lens is materially wrong.
🎯 Correctness — No material issues found
VERDICT: No material issues found
Through the correctness lens I verified:
openEventStream (agent.go:157-171) sets the write deadline before the first flush, and write (agent.go:182-192) refreshes it beforeio.WriteString+Flush, so each individual write (write + flush) is bounded while the stream as a whole is not. Ordering is correct.
keepAlive's writes go through the same write path under mu, so the deadline is refreshed for keep-alive frames too and there's no concurrent unsynchronized SetWriteDeadline on the conn (and http.ResponseController is concurrency-safe regardless).
The diagnosis underpinning the fix is correct: http.Server.WriteTimeout is an absolute deadline from header read, and writes made after it on a Go HTTP server return nil with bytes silently dropped — the client sees EOF. That's why a client-side assertion is the only way to catch it, and why checking io.WriteString's return would have missed the bug. Matches the code at agent.go:190 (_, _ = io.WriteString).
cmd/pansy/main.go:65 confirms WriteTimeout: 30 * time.Second, and runTimeout/keepAliveInterval are as described in the PR, so the 4-min budget really was unreachable without this fix.
Test timing re-derived from first principles:
TestEventStreamOutlivesServerWriteTimeout: server WriteTimeout=300ms, frames at 300/600/900ms. Unfixed code cuts at 300ms (≈1 frame); fixed code, with sseWriteTimeout=30s default, refreshes per frame and delivers all 3. Correct.
TestEventStreamRefreshesDeadlinePerFrame: sseWriteTimeout=400ms, 8 frames at 100ms intervals spanning 800ms, server WriteTimeout=5s. Per-frame refresh delivers all 8; a set-once deadline set at open (t=0, expires 400ms) would cut mid-stream ≈400ms — exactly what the comment claims. The 100ms gap is a 4× margin under the 400ms deadline, so a correctly-refreshing implementation passes reliably even under CI jitter. Correct.
I could not read the vendored gin response_writer.go (no vendor/ dir) to independently confirm Unwrap() http.ResponseWriter, but go.mod pins gin v1.10.1 as the PR states, and this is a known gin feature; the author verified it against the source. Accepting.
No correctness defects found.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The change follows the surrounding file's conventions: slog.Error with "error" field matches the existing logging at agent.go:104,176; the doc-comment style (multi-paragraph // with deliberate emphasis) is consistent with the file's other comments; the new rc *http.ResponseController field slots into the existing struct cleanly.
sseWriteTimeout as a var (rather than const) purely for test overridability is a deliberate, explicitly-justified trade-off. It is the only mutable package-level global introduced, and neither test calls t.Parallel(), so the reassignment in TestEventStreamRefreshesDeadlinePerFrame (with t.Cleanup restore) is race-free under the default sequential test execution. Not a cleanliness smell worth flagging.
No dead or duplicated code: streamFrames is correctly shared by both tests; the per-frame _ = s.rc.SetWriteDeadline(...) in write and the one-shot probe in openEventStream are intentionally distinct (probe logs, hot path stays silent) and both are explained.
Names are clear (sseWriteTimeout, rc, streamFrames); no leaky abstraction or copy-paste.
Verified by reading internal/api/agent.go:1-220 and internal/api/sse_deadline_test.go in full; confirmed net/http import already present (agent.go:10) and time (agent.go:12), so no import churn was needed.
⚡ Performance — No material issues found
VERDICT: No material issues found
The new per-frame work in write (agent.go:189) is one time.Now().Add plus one SetWriteDeadline syscall per SSE frame. SSE frame rate here is low — a handful of step events per agent turn plus a keep-alive comment every keepAliveInterval (20s) — so the per-frame cost is negligible, not a hot-loop concern. Verified by reading keepAlive (agent.go:194-223) and the handler flow: frames are agent-step bursts and 20s tickers, not a tight loop.
http.NewResponseController(c.Writer) is constructed once per stream in openEventStream (agent.go:161), not per frame, so no per-frame allocation regression.
No new unbounded growth, N+1, or blocking added; the mutex in write already serialized keep-alive and run goroutines, and this change doesn't widen the critical section beyond a single non-blocking deadline call.
Verified via read_file on internal/api/agent.go:100-223 and grep over internal/api for write/keep-alive call sites.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 4: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 3/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
🎯 Correctness — No material issues found
No material issues found
The fix correctly moves from an absolute Server.WriteTimeout (which is measured from request header read) to a per-write deadline refreshed on every frame, which is the only way to keep an SSE stream alive across multi-minute agent turns.
http.NewResponseController(c.Writer) + SetWriteDeadline is the right mechanism for this.
The test accurately reproduces the client-visible symptom and validates the fix from the only vantage point where the bug is observable.
The keep-alive mechanism (20 s interval) and the per-write backstop (30 s) have a healthy margin and won't interfere with each other.
The sseWriteTimeout var mutation in tests is safe because t.Parallel() is not used.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The code is clean and well-maintained. The test helper streamFrames is focused and reusable. The sseWriteTimeout variable is explicitly and honestly documented as a var (not const) solely for test override — a defensible trade-off with a clear comment explaining why. Comments on openEventStream and the struct are extensive but proportionate to the subtlety of the invisible failure mode. No dead code, duplicated logic, or naming issues detected.
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 01:44:11.846)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 01:44:11.846)
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens. The change is a server-side timeout/robustness fix; it touches no authn/authz, input parsing, or data flow.
Verified authz is intact: POST /agent/chat is registered under v1.Group("/agent", h.requireAuth()) (internal/api/api.go:153), and the handler calls h.svc.AgentHistory which enforces requireGardenRole(..., roleEditor) (internal/service/agent.go:25). The diff doesn't alter the request path or any permission check.
Verified the DoS backstop is preserved: the per-write sseWriteTimeout = 30s (internal/api/agent.go:124) bounds each write, so a slow/stopped-reading client can't block a write forever. The overall run is still capped by runTimeout = 4 * time.Minute (internal/agent/runtime.go:31) via context.WithTimeout (runtime.go:96), so a malicious peer can't hold the goroutine/stream beyond that. Removing the server's absolute WriteTimeout for this stream does not open an unbounded-resource vector.
The var sseWriteTimeout is only mutated inside the test file (sse_deadline_test.go) with a t.Cleanup restore; production never reassigns it, so no concurrent-mutation/race concern in the served path.
No injection/path/template/SSRF surface is introduced: the deadline value is a fixed constant, not derived from request input.
🎯 Correctness — No material issues found
No material issues found
I verified the fix end-to-end against the checked-out code:
cmd/pansy/main.go:65 confirms WriteTimeout: 30 * time.Second, and internal/agent/runtime.go:31,96 confirms runTimeout = 4 * time.Minute applied via context.WithTimeout. The claim that the absolute 30s write deadline structurally truncates a 4-minute turn is correct — http.Server.WriteTimeout is set once on the conn at header-read time, not an idle timeout.
internal/api/agent.go:157-171 — http.NewResponseController(c.Writer) reaches the underlying conn via Unwrap. SetWriteDeadline(time.Now().Add(sseWriteTimeout)) at open replaces the conn's absolute write deadline with a per-write one; this is the correct mechanism to override WriteTimeout for a streaming response. I could not directly open gin's response_writer.go (no vendored copy / module cache in the workspace), so the Unwrap() availability for gin 1.10.1 is the one link I'm taking on the PR's stated verification rather than re-confirming myself — but if it were missing, SetWriteDeadline returns ErrUnsupported, the probe logs once, and behavior falls back to the pre-fix state, so the failure mode is the old bug, not a new one.
agent.go:182-192 — write() holds mu across the SetWriteDeadline + io.WriteString + Flush sequence. This is load-bearing for the per-frame deadline: the keep-alive goroutine (keepAlive, agent.go:198-223) and the run goroutine both call write(), so the mutex correctly serializes deadline-then-write as an atomic unit. A write and its deadline refresh cannot interleave with another frame's. Correct.
The probe in openEventStream sets the initial deadline but every actual frame refreshes it in write(); the probe's own deadline value is effectively redundant for writes (only there to detect unsupported writers), which matches the comment.
sse_deadline_test.go:62-74 — server WriteTimeout=300ms, tick=300ms, 3 frames. time.Sleep guarantees ≥ duration, so the first write lands at ≥300ms, i.e. at/after the absolute deadline; against the unfixed code the client gets <3 frames (the documented regression symptom). With the fix, openEventStream overrides the conn deadline to 30s before any sleep, so no interference. Sound.
sse_deadline_test.go:85-101 — sseWriteTimeout=400ms, server WriteTimeout=5s (not the limiter), 8 frames at 100ms tick spanning 800ms. A set-once deadline at 400ms would cut at ~frame 4-5; per-frame refresh keeps each write 100ms after its deadline was set, a 4× margin. This genuinely distinguishes "refresh per frame" from "set once" — the test's stated purpose. Correct.
The sseWriteTimeout var reassign in the test is goroutine-safe: it happens-before srv.Start()/handler invocation via goroutine-creation happens-before, and package tests run serially (no t.Parallel()). No -race concern.
On the semantic/deadline-math angle specifically: the per-frame time.Now().Add(sseWriteTimeout) gives each individual write 30s, the stream as a whole is unbounded, and a stuck reader is bounded to ≤30s per write — which is exactly the stated invariant. No off-by-one or wrong-unit issue in the deadline arithmetic.
🧹 Code cleanliness & maintainability — No material issues found
The code is clean. Let me verify the test's reliance on the keep-alive mutex comment and the comment claim about issue numbers.
The code is clean and well-structured. Comments are accurate, mutex claim verified. Test issue references are consistent with PR description (the second test guards the per-frame refresh which is the same fix's robustness, #87 referenced in comments only).
VERDICT: No material issues found
The change through the cleanliness/maintainability lens:
sseWriteTimeout as a var for testability: This is a pragmatic, well-documented choice. The comment at agent.go:121-123 explains exactly why it's not a const and that production never reassigns it. The alternative (injecting the duration) would add a parameter threading through openEventStream for no production benefit. Clean.
Comment density: The comments are long but load-bearing — each explains a non-obvious invariant (absolute vs. idle deadline, invisible failure mode, per-frame refresh rationale) that a future reader would otherwise re-derive incorrectly. They match the existing verbose-but-purposeful commenting style in this file (see keepAlive at agent.go:194-218).
Error handling split: openEventStream logs the probe failure once (agent.go:166-168); write deliberately ignores it (agent.go:185-189) with a comment explaining why per-frame logging would be noise. This is a considered, consistent split, not sloppy.
Struct field ordering: rc *http.ResponseController added between c and mu at agent.go:132-136. Slightly interrupts the existing grouping, but it's a minor stylistic preference and matches no violated convention in the file.
Test file: streamFrames is a well-factored helper shared by both tests. chatEvent{Error: "frame"} is a lightweight reuse of the production event type (verified at agent.go:41-51), not a fixture that asserts a mental model. Test names are descriptive; issue references in comments are consistent with the PR description.
I verified by reading agent.go in full and confirming chatEvent/openEventStream/write/keepAlive match the diff's context, and that the mutex claim in the sseWriteTimeout comment (keep-alive shares mu) holds at agent.go:212.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed internal/api/agent.go (the SSE deadline fix) and internal/api/sse_deadline_test.go through the performance lens.
SetWriteDeadline is now called per frame inside eventStream.write (agent.go:189), but frame frequency is low (step events plus a keep-alive every 20s per keepAliveInterval), and the call is a cheap deadline update under the already-held s.mu — no new allocation, no extra syscall of consequence on this path. No hot-loop regression.
http.NewResponseController(c.Writer) is constructed once per stream in openEventStream (agent.go:161), not per write. No per-frame allocation growth.
The sseWriteTimeout package-level var is read, never reassigns in production; the test reassigns it once with a cleanup. No contention concern.
Nothing materially wrong in the performance lens.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed the change through the error-handling & edge-case lens, verifying against the checked-out code.
Deliberately-ignored error in write() (agent.go:189) — _ = s.rc.SetWriteDeadline(...). This is intentional and correctly justified: openEventStream already probes once and logs if deadlines are unsupported, and SetWriteDeadline is deterministic per-conn (it fails the same way every time), so per-frame error checks would spam identical log lines. Verified by reading both call sites. Not a real issue.
openEventStream probe error path (agent.go:166-168) — failure is logged and the stream proceeds with degraded behavior (falls back to being cut at the server WriteTimeout). Honest degradation, not a swallowed error. The comment is explicit that the stream "still works" in that case.
Deadline- fires-mid-write edge case — a single io.WriteString exceeding sseWriteTimeout would be cut partway and produce a truncated SSE frame, with the write error still ignored at agent.go:190. This is preexisting behavior (the io.WriteString error was ignored before this PR too), and the failure mode (truncated stream seen by client) is no worse than the prior absolute-deadline behavior. Not introduced here.
Deadline gap before first keep-alive — openEventStream sets T0+30s; first keep-alive at T0+20s refreshes before expiry; every subsequent write (keep-alive or real frame) refreshes again. No window where the deadline can lapse silently. Verified via keepAlive at agent.go:198-223 routing through the same write() path.
sseWriteTimeout package-var mutation in tests — TestEventStreamRefreshesDeadlinePerFrame reassigns it with t.Cleanup restore. Neither test calls t.Parallel(), and Go runs non-parallel tests in a package sequentially, so no race on the var under -race. Would become a real race only if someone later adds t.Parallel() to either test — latent but not a current bug.
Test streamFrames cleanup ordering — t.Fatalf on Get error still runs the deferred srv.Close()/resp.Body.Close(); sc.Err() is the only error surfaced, which is the correct vantage point per the PR's rationale. No leaked goroutine/server on the unhappy path.
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Through the security lens, the change is clean:
The endpoint remains authenticated (mustActor in agentChat).
No new input surface, injection vector, or deserialization path is introduced.
http.NewResponseController and SetWriteDeadline are standard-library mechanisms with no known unsafe behavior.
The sseWriteTimeout variable is package-private; it cannot be mutated by untrusted code.
The per-frame write deadline is a defensive bound (30 s) rather than an unlimited removal of the deadline, which is the correct posture against a stalled peer.
Nothing in this diff creates an authn/authz gap, leaks credentials, or introduces unsafe handling of untrusted data.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The PR is clean and well-structured:
agent.go:124: sseWriteTimeout is a package-level var (not const) solely to allow tests to override it. This is a pragmatic, low-churn testing seam, and the comment explains exactly why.
Comments are high-signal: Every non-trivial decision — the invisible server-side failure mode, why the error is deliberately unchecked per-frame, why ResponseController is probed once up front — is documented without being verbose.
eventStream struct change is minimal: Adding rc *http.ResponseController alongside the existing c *gin.Context is the natural place for it.
Test structure is clean: streamFrames is a well-factored helper that returns (count, error) and lets both test cases express their specific assertions directly. The two test cases have distinct, non-overlapping responsibilities.
No dead code, no duplication, no confusing naming. The diff follows the surrounding patterns (e.g., slog.Error with structured keys, t.Cleanup for restoring mutated globals).
One thing I checked and confirmed is safe: TestEventStreamRefreshesDeadlinePerFrame mutates the package-level sseWriteTimeout without a mutex, but t.Parallel() is not used anywhere in this package, so there's no concurrent test risk. No other test touches it.
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🧯 Error handling & edge cases — No material issues found
No material issues found
The change handles its edge cases carefully and the error paths are well considered.
Probe-before-use pattern: openEventStream probes SetWriteDeadline once and logs with slog.Error if deadlines are unavailable. The stream continues to work in that degraded case — correct fallback behavior.
Ignored SetWriteDeadline error in write: Deliberately and correctly unchecked. The rc probes the same underlying capability on every call; an error here can only repeat what openEventStream already reported, so suppressing it avoids log spam without hiding new information.
Global var in test: sseWriteTimeout is a var (not a const) so TestEventStreamRefreshesDeadlinePerFrame can temporarily shrink it. The test uses t.Cleanup to restore the original value, and neither test in the file uses t.Parallel(), so there is no cross-test race.
Nil safety: eventStream is only ever constructed via openEventStream, so rc is always non-nil. http.NewResponseController never returns nil. c.Writer in a gin handler context is always initialized.
Deadline timing: keepAliveInterval (20s) is safely under sseWriteTimeout (30s), so the keep-alive ticker both paces proxies and refreshes the write deadline with a 10s margin.
Nothing in this diff introduces an unhandled nil, empty collection, boundary, overflow, or error-swallowing bug from the error-handling & edge-cases perspective.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: No material issues found** · 0 findings (0 with multi-model agreement)
No material issues found by consensus.
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed through the security lens (authn/authz, injection, SSRF, deserialization, secret leakage, input validation, unsafe handling of untrusted data, insecure defaults).
**What I checked:**
- `internal/api/agent.go:58-109` (`agentChat`) — `mustActor(c)` and the existing `h.svc.AgentHistory` permission check run *before* `openEventStream` is called, and this diff doesn't touch that ordering, so authz gating is unaffected.
- The per-frame `SetWriteDeadline` refresh (`agent.go:189`) and the one-time probe in `openEventStream` (`agent.go:161-168`) only touch a `net.Conn` deadline — no user-controlled data flows into either call. `sseWriteTimeout` is a fixed, unexported package var; the test mutates it in-package only (`sse_deadline_test.go`), not reachable from any request.
- The `slog.Error` call at `agent.go:167` logs only the `error` from `SetWriteDeadline` (an internal `net`/`http` error), no request data, no credentials.
- Confirmed no new user input parsing, no new outbound requests (no SSRF surface), no new serialization/deserialization path — this is purely a connection-deadline change in an already-open, already-authorized stream.
- Considered whether refreshing the deadline per-frame (vs. the previous absolute `WriteTimeout`) creates a resource-exhaustion angle: a client that stops reading can still only pin the write for `sseWriteTimeout` (30s) per attempt before the write times out, and the agent turn itself is independently capped by `runTimeout` (4 min, unchanged by this PR) — so worst case is the same order of magnitude as before, not a new unbounded hold. The author's own comments in `agent.go:111-124` and `144-156` show this tradeoff was deliberately reasoned about, not overlooked.
Nothing in this diff introduces or weakens an authn/authz check, and there's no injection, SSRF, deserialization, or secret-leakage vector — it's a narrowly-scoped connection-deadline fix on an already-authenticated stream.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Only one construction site, always fully initialized. I've verified the deadline math, the timeout constants against their real values, the gin/`http.ResponseController` wiring, the mutex serialization, and the two new tests' logic — no correctness issues surfaced.
## Review
**VERDICT: No material issues found**
Findings, all verified against the checked-out code:
- `sseWriteTimeout` (30s) matches the original `WriteTimeout` at `cmd/pansy/main.go:65`, and `keepAliveInterval` (20s, `internal/api/agent.go:32`) stays comfortably under it, so the keep-alive tick refreshes the deadline before it can expire during a quiet stretch — the numbers line up correctly.
- `runTimeout` is confirmed at 4 minutes (`internal/agent/runtime.go:31`), consistent with the PR's claim that the per-frame refresh (not a single extended deadline) is what's needed to outlast the server's absolute `WriteTimeout`.
- `SetWriteDeadline` is called immediately before every write while holding `s.mu`, and no write is ever in flight when a new deadline is set (writes are serialized by the mutex), so there's no race between refreshing the deadline and an in-progress write.
- Go's `net/http` treats `SetWriteDeadline` as "last call wins" on the underlying conn (module is on `go 1.26.2`, well past the 1.20 minimum for `http.ResponseController`), so re-arming it per frame genuinely supersedes the server's original absolute deadline — the mechanism does what the PR claims.
- No other middleware in the router (`internal/api/api.go:39` only adds `sloggin` + `gin.Recovery()`) wraps `c.Writer`, so the `Unwrap()` chain the fix depends on isn't broken by anything else in this codebase.
- `eventStream` has exactly one construction site (`agent.go:161`), always fully initializing `rc`, so there's no nil-`ResponseController` path.
- The two new tests correctly distinguish the fixed/unfixed behavior: `TestEventStreamOutlivesServerWriteTimeout` leaves `sseWriteTimeout` at its 30s default against a 300ms server timeout (would fail pre-fix), and `TestEventStreamRefreshesDeadlinePerFrame` shrinks `sseWriteTimeout` to 400ms with 100ms-spaced frames against a generous 5s server timeout, which only passes if the deadline is refreshed per-write rather than once at open.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
Confirmed the 30s `WriteTimeout` referenced in the diff's comments matches `cmd/pansy/main.go:65`. The diff itself is unchanged there — this PR is scoped correctly to `internal/api/agent.go` and its test.
I've reviewed the full diff against the checked-out code. This is a clean, tightly-scoped change.
## Verdict: No material issues found
Through the code cleanliness/maintainability lens specifically:
- **No duplication introduced**: the two new tests share the `streamFrames` helper rather than copy-pasting server setup (`internal/api/sse_deadline_test.go:18-49`).
- **Struct/function sizes stay small**: `openEventStream` and `write` remain short, single-purpose.
- **Naming and field additions fit the file's existing conventions** (`rc *http.ResponseController` alongside `c *gin.Context`, `mu sync.Mutex` — verified against `internal/api/agent.go:132-136`).
- **The heavily narrative comment style** (three separate blocks explaining the deadline rationale at the var, the function, and the write call) matches the pre-existing style already used throughout this same file (e.g. the `keepAlive`, `chatErrorMessage`, and `agentChat` doc comments), so it isn't a new maintainability wart introduced by this PR — it's consistent with the surrounding code.
- Verified `gofmt`-relevant formatting by eye (tabs, brace placement) matches surrounding code; no stray whitespace or import churn (`net/http` was already imported).
- No dead code, no leftover TODO, no copy-paste that should have been factored out.
Nothing in this diff, viewed purely for readability/structure/duplication/naming, rises to a reportable finding.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens only.
The change adds one `SetWriteDeadline` call per SSE frame (`internal/api/agent.go:187`) inside the existing `write()` critical section. I checked how often `write()`/`send()` actually fire on this path:
- `agentChat` (`internal/api/agent.go:60-108`) sends one `chatEvent` per completed **model round trip** (a `Step`), bounded by `maxSteps = 24` (`internal/agent/runtime.go:27`), plus one final `Done`/`Warning` event.
- The keep-alive ticker (`internal/api/agent.go:32`, `keepAliveInterval = 20s`) adds at most ~12 more frames over the 4-minute `runTimeout`.
So this is at most a few dozen writes over a multi-minute stream — not a hot loop, not a per-token streaming path. `http.ResponseController.SetWriteDeadline` on a TCP conn is a cheap netpoller timer reset, not a syscall-heavy operation, and it's already inside the same mutex-guarded critical section as the write it protects, so it adds no new lock contention. The `http.ResponseController` itself is allocated once in `openEventStream` (`internal/api/agent.go:160`) and stored on `eventStream`, not reallocated per frame.
No N+1s, no unbounded growth, no missing pagination, no quadratic behavior, and no new blocking-call concerns beyond what the PR itself documents (the deadline is explicitly a bounded backstop, not a pacing mechanism). The new test spins up a real `http.Server` with sub-second timeouts and closes it via `defer srv.Close()` — bounded and self-contained.
Nothing to flag in this lane.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## Verification
Read `internal/api/agent.go` in full against the diff. Every claim in the draft's reasoning checks out against the actual code:
- `sseWriteTimeout` is a package-level `var` (30s), reassigned only in the test (`sse_deadline_test.go`), confirmed unused elsewhere in production code.
- `write()` ignores the `SetWriteDeadline` error return (`_ = s.rc.SetWriteDeadline(...)`) while `openEventStream` logs it once via `slog.Error` — matches the stated "probe once, refresh silently" design.
- The mutex (`s.mu`) is held only across a single `write()` call, so a stalled client blocks at most for one write, bounded by `sseWriteTimeout` (30s), not indefinitely — confirmed no other lock ordering issue with `keepAlive`'s goroutine, which also goes through `s.write()` under the same lock.
- `keepAlive`'s stop is `sync.Once`-guarded and selects on both an explicit `done` channel and `c.Request.Context().Done()`, so no goroutine leak.
- `http.NewResponseController(c.Writer)` against gin's `ResponseWriter` — gin has implemented `Unwrap() http.ResponseWriter` since v1.9.0 specifically to support `http.ResponseController`, and `go.mod` pins `gin-gonic/gin v1.10.1`, so this resolves correctly (not confirmable via local build due to sandbox restrictions outside the repo directory, but consistent with known gin API history).
- `http.Flusher.Flush()` has no error return, so nothing is silently swallowed there beyond what's already documented.
The draft contains no bulleted findings to individually confirm or reject — it is a clean "no issues" verdict, and independent reading of the diff supports that verdict for the error-handling/edge-case lens.
VERDICT: No material issues found
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the security lens. The diff replaces an absolute `http.Server.WriteTimeout` with a per-write deadline on the SSE response. Security-relevant observations:
- **No DoS regression / resource exhaustion.** The change keeps a per-write deadline (`sseWriteTimeout = 30s`) rather than removing deadlines entirely. A client that stops reading will still have its writes time out within 30s once the socket buffer fills — the run goroutine and the stream mutex are not pinned indefinitely. The overall turn is still bounded by `runTimeout` (4 min). So this does not create a new vector for connection-goroutine exhaustion; if anything it makes the intended long-lived-stream case explicit while preserving the stuck-peer backstop.
- **No injection / untrusted-input path touched.** The only new inputs are `time.Now()` and the package-level `sseWriteTimeout` constant — no request data flows into `SetWriteDeadline` or the response body. The frame content still goes through `json.Marshal` in `send` (`agent.go:174`), unchanged.
- **No authn/authz change.** `openEventStream` is called after `mustActor(c)` and request binding in `agentChat` (`agent.go:58-72`); the auth surface is untouched.
- **`sseWriteTimeout` is a `var`, not `const`.** It's package-level and reassignable, but only the test mutates it (under `t.Cleanup` restore). Production never reassigns it, and it's not exported, so no external tampering surface. Not a security concern.
- **`http.NewResponseController(c.Writer)`** — no credential/secret handling involved; the PR description's verification of gin's `Unwrap()` is correctness, not security.
Nothing in my lens is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
VERDICT: No material issues found
Through the correctness lens I verified:
- `openEventStream` (agent.go:157-171) sets the write deadline before the first flush, and `write` (agent.go:182-192) refreshes it *before* `io.WriteString`+`Flush`, so each individual write (write + flush) is bounded while the stream as a whole is not. Ordering is correct.
- `keepAlive`'s writes go through the same `write` path under `mu`, so the deadline is refreshed for keep-alive frames too and there's no concurrent unsynchronized `SetWriteDeadline` on the conn (and `http.ResponseController` is concurrency-safe regardless).
- The diagnosis underpinning the fix is correct: `http.Server.WriteTimeout` is an absolute deadline from header read, and writes made after it on a Go HTTP server return `nil` with bytes silently dropped — the client sees EOF. That's why a client-side assertion is the only way to catch it, and why checking `io.WriteString`'s return would have missed the bug. Matches the code at agent.go:190 (`_, _ = io.WriteString`).
- `cmd/pansy/main.go:65` confirms `WriteTimeout: 30 * time.Second`, and `runTimeout`/`keepAliveInterval` are as described in the PR, so the 4-min budget really was unreachable without this fix.
Test timing re-derived from first principles:
- `TestEventStreamOutlivesServerWriteTimeout`: server `WriteTimeout=300ms`, frames at 300/600/900ms. Unfixed code cuts at 300ms (≈1 frame); fixed code, with `sseWriteTimeout=30s` default, refreshes per frame and delivers all 3. Correct.
- `TestEventStreamRefreshesDeadlinePerFrame`: `sseWriteTimeout=400ms`, 8 frames at 100ms intervals spanning 800ms, server `WriteTimeout=5s`. Per-frame refresh delivers all 8; a set-once deadline set at open (t=0, expires 400ms) would cut mid-stream ≈400ms — exactly what the comment claims. The 100ms gap is a 4× margin under the 400ms deadline, so a correctly-refreshing implementation passes reliably even under CI jitter. Correct.
I could not read the vendored gin `response_writer.go` (no vendor/ dir) to independently confirm `Unwrap() http.ResponseWriter`, but go.mod pins `gin v1.10.1` as the PR states, and this is a known gin feature; the author verified it against the source. Accepting.
No correctness defects found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found.**
- The change follows the surrounding file's conventions: `slog.Error` with `"error"` field matches the existing logging at `agent.go:104,176`; the doc-comment style (multi-paragraph `//` with deliberate emphasis) is consistent with the file's other comments; the new `rc *http.ResponseController` field slots into the existing struct cleanly.
- `sseWriteTimeout` as a `var` (rather than `const`) purely for test overridability is a deliberate, explicitly-justified trade-off. It is the only mutable package-level global introduced, and neither test calls `t.Parallel()`, so the reassignment in `TestEventStreamRefreshesDeadlinePerFrame` (with `t.Cleanup` restore) is race-free under the default sequential test execution. Not a cleanliness smell worth flagging.
- No dead or duplicated code: `streamFrames` is correctly shared by both tests; the per-frame `_ = s.rc.SetWriteDeadline(...)` in `write` and the one-shot probe in `openEventStream` are intentionally distinct (probe logs, hot path stays silent) and both are explained.
- Names are clear (`sseWriteTimeout`, `rc`, `streamFrames`); no leaky abstraction or copy-paste.
- Verified by reading `internal/api/agent.go:1-220` and `internal/api/sse_deadline_test.go` in full; confirmed `net/http` import already present (`agent.go:10`) and `time` (`agent.go:12`), so no import churn was needed.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
- The new per-frame work in `write` (`agent.go:189`) is one `time.Now().Add` plus one `SetWriteDeadline` syscall per SSE frame. SSE frame rate here is low — a handful of step events per agent turn plus a keep-alive comment every `keepAliveInterval` (20s) — so the per-frame cost is negligible, not a hot-loop concern. Verified by reading `keepAlive` (`agent.go:194-223`) and the handler flow: frames are agent-step bursts and 20s tickers, not a tight loop.
- `http.NewResponseController(c.Writer)` is constructed once per stream in `openEventStream` (`agent.go:161`), not per frame, so no per-frame allocation regression.
- No new unbounded growth, N+1, or blocking added; the mutex in `write` already serialized keep-alive and run goroutines, and this change doesn't widen the critical section beyond a single non-blocking deadline call.
Verified via `read_file` on `internal/api/agent.go:100-223` and grep over `internal/api` for write/keep-alive call sites.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 4: all chain targets failed
ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 3/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
- The fix correctly moves from an absolute `Server.WriteTimeout` (which is measured from request header read) to a per-write deadline refreshed on every frame, which is the only way to keep an SSE stream alive across multi-minute agent turns.
- `http.NewResponseController(c.Writer)` + `SetWriteDeadline` is the right mechanism for this.
- The test accurately reproduces the client-visible symptom and validates the fix from the only vantage point where the bug is observable.
- The keep-alive mechanism (20 s interval) and the per-write backstop (30 s) have a healthy margin and won't interfere with each other.
- The `sseWriteTimeout` var mutation in tests is safe because `t.Parallel()` is not used.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
The code is clean and well-maintained. The test helper `streamFrames` is focused and reusable. The `sseWriteTimeout` variable is explicitly and honestly documented as a `var` (not `const`) solely for test override — a defensible trade-off with a clear comment explaining why. Comments on `openEventStream` and the struct are extensive but proportionate to the subtlety of the invisible failure mode. No dead code, duplicated logic, or naming issues detected.
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 01:44:11.846)
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 01:44:11.846)
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed through the security lens. The change is a server-side timeout/robustness fix; it touches no authn/authz, input parsing, or data flow.
- Verified authz is intact: `POST /agent/chat` is registered under `v1.Group("/agent", h.requireAuth())` (`internal/api/api.go:153`), and the handler calls `h.svc.AgentHistory` which enforces `requireGardenRole(..., roleEditor)` (`internal/service/agent.go:25`). The diff doesn't alter the request path or any permission check.
- Verified the DoS backstop is preserved: the per-write `sseWriteTimeout = 30s` (`internal/api/agent.go:124`) bounds each write, so a slow/stopped-reading client can't block a write forever. The overall run is still capped by `runTimeout = 4 * time.Minute` (`internal/agent/runtime.go:31`) via `context.WithTimeout` (`runtime.go:96`), so a malicious peer can't hold the goroutine/stream beyond that. Removing the server's absolute `WriteTimeout` for this stream does not open an unbounded-resource vector.
- The `var sseWriteTimeout` is only mutated inside the test file (`sse_deadline_test.go`) with a `t.Cleanup` restore; production never reassigns it, so no concurrent-mutation/race concern in the served path.
- No injection/path/template/SSRF surface is introduced: the deadline value is a fixed constant, not derived from request input.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
I verified the fix end-to-end against the checked-out code:
- `cmd/pansy/main.go:65` confirms `WriteTimeout: 30 * time.Second`, and `internal/agent/runtime.go:31,96` confirms `runTimeout = 4 * time.Minute` applied via `context.WithTimeout`. The claim that the absolute 30s write deadline structurally truncates a 4-minute turn is correct — `http.Server.WriteTimeout` is set once on the conn at header-read time, not an idle timeout.
- `internal/api/agent.go:157-171` — `http.NewResponseController(c.Writer)` reaches the underlying conn via `Unwrap`. `SetWriteDeadline(time.Now().Add(sseWriteTimeout))` at open replaces the conn's absolute write deadline with a per-write one; this is the correct mechanism to override `WriteTimeout` for a streaming response. I could not directly open gin's `response_writer.go` (no vendored copy / module cache in the workspace), so the `Unwrap()` availability for gin 1.10.1 is the one link I'm taking on the PR's stated verification rather than re-confirming myself — but if it were missing, `SetWriteDeadline` returns `ErrUnsupported`, the probe logs once, and behavior falls back to the pre-fix state, so the failure mode is the old bug, not a new one.
- `agent.go:182-192` — `write()` holds `mu` across the `SetWriteDeadline` + `io.WriteString` + `Flush` sequence. This is load-bearing for the per-frame deadline: the keep-alive goroutine (`keepAlive`, `agent.go:198-223`) and the run goroutine both call `write()`, so the mutex correctly serializes deadline-then-write as an atomic unit. A write and its deadline refresh cannot interleave with another frame's. Correct.
- The probe in `openEventStream` sets the initial deadline but every actual frame refreshes it in `write()`; the probe's own deadline value is effectively redundant for writes (only there to detect unsupported writers), which matches the comment.
- `sse_deadline_test.go:62-74` — server `WriteTimeout=300ms`, tick=300ms, 3 frames. `time.Sleep` guarantees ≥ duration, so the first write lands at ≥300ms, i.e. at/after the absolute deadline; against the unfixed code the client gets <3 frames (the documented regression symptom). With the fix, `openEventStream` overrides the conn deadline to 30s before any sleep, so no interference. Sound.
- `sse_deadline_test.go:85-101` — `sseWriteTimeout=400ms`, server `WriteTimeout=5s` (not the limiter), 8 frames at 100ms tick spanning 800ms. A set-once deadline at 400ms would cut at ~frame 4-5; per-frame refresh keeps each write 100ms after its deadline was set, a 4× margin. This genuinely distinguishes "refresh per frame" from "set once" — the test's stated purpose. Correct.
- The `sseWriteTimeout` var reassign in the test is goroutine-safe: it happens-before `srv.Start()`/handler invocation via goroutine-creation happens-before, and package tests run serially (no `t.Parallel()`). No `-race` concern.
On the semantic/deadline-math angle specifically: the per-frame `time.Now().Add(sseWriteTimeout)` gives each individual write 30s, the stream as a whole is unbounded, and a stuck reader is bounded to ≤30s per write — which is exactly the stated invariant. No off-by-one or wrong-unit issue in the deadline arithmetic.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
The code is clean. Let me verify the test's reliance on the keep-alive mutex comment and the comment claim about issue numbers.
The code is clean and well-structured. Comments are accurate, mutex claim verified. Test issue references are consistent with PR description (the second test guards the per-frame refresh which is the same fix's robustness, #87 referenced in comments only).
**VERDICT: No material issues found**
The change through the cleanliness/maintainability lens:
- **`sseWriteTimeout` as a `var` for testability**: This is a pragmatic, well-documented choice. The comment at `agent.go:121-123` explains exactly why it's not a const and that production never reassigns it. The alternative (injecting the duration) would add a parameter threading through `openEventStream` for no production benefit. Clean.
- **Comment density**: The comments are long but load-bearing — each explains a non-obvious invariant (absolute vs. idle deadline, invisible failure mode, per-frame refresh rationale) that a future reader would otherwise re-derive incorrectly. They match the existing verbose-but-purposeful commenting style in this file (see `keepAlive` at `agent.go:194-218`).
- **Error handling split**: `openEventStream` logs the probe failure once (`agent.go:166-168`); `write` deliberately ignores it (`agent.go:185-189`) with a comment explaining why per-frame logging would be noise. This is a considered, consistent split, not sloppy.
- **Struct field ordering**: `rc *http.ResponseController` added between `c` and `mu` at `agent.go:132-136`. Slightly interrupts the existing grouping, but it's a minor stylistic preference and matches no violated convention in the file.
- **Test file**: `streamFrames` is a well-factored helper shared by both tests. `chatEvent{Error: "frame"}` is a lightweight reuse of the production event type (verified at `agent.go:41-51`), not a fixture that asserts a mental model. Test names are descriptive; issue references in comments are consistent with the PR description.
I verified by reading `agent.go` in full and confirming `chatEvent`/`openEventStream`/`write`/`keepAlive` match the diff's context, and that the mutex claim in the `sseWriteTimeout` comment (keep-alive shares `mu`) holds at `agent.go:212`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Reviewed `internal/api/agent.go` (the SSE deadline fix) and `internal/api/sse_deadline_test.go` through the performance lens.
- `SetWriteDeadline` is now called per frame inside `eventStream.write` (agent.go:189), but frame frequency is low (step events plus a keep-alive every 20s per `keepAliveInterval`), and the call is a cheap deadline update under the already-held `s.mu` — no new allocation, no extra syscall of consequence on this path. No hot-loop regression.
- `http.NewResponseController(c.Writer)` is constructed once per stream in `openEventStream` (agent.go:161), not per write. No per-frame allocation growth.
- The `sseWriteTimeout` package-level `var` is read, never reassigns in production; the test reassigns it once with a cleanup. No contention concern.
Nothing materially wrong in the performance lens.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
VERDICT: No material issues found
I reviewed the change through the error-handling & edge-case lens, verifying against the checked-out code.
- **Deliberately-ignored error in `write()` (`agent.go:189`)** — `_ = s.rc.SetWriteDeadline(...)`. This is intentional and correctly justified: `openEventStream` already probes once and logs if deadlines are unsupported, and `SetWriteDeadline` is deterministic per-conn (it fails the same way every time), so per-frame error checks would spam identical log lines. Verified by reading both call sites. Not a real issue.
- **`openEventStream` probe error path (`agent.go:166-168`)** — failure is logged and the stream proceeds with degraded behavior (falls back to being cut at the server `WriteTimeout`). Honest degradation, not a swallowed error. The comment is explicit that the stream "still works" in that case.
- **Deadline- fires-mid-write edge case** — a single `io.WriteString` exceeding `sseWriteTimeout` would be cut partway and produce a truncated SSE frame, with the write error still ignored at `agent.go:190`. This is preexisting behavior (the `io.WriteString` error was ignored before this PR too), and the failure mode (truncated stream seen by client) is no worse than the prior absolute-deadline behavior. Not introduced here.
- **Deadline gap before first keep-alive** — `openEventStream` sets T0+30s; first keep-alive at T0+20s refreshes before expiry; every subsequent write (keep-alive or real frame) refreshes again. No window where the deadline can lapse silently. Verified via `keepAlive` at `agent.go:198-223` routing through the same `write()` path.
- **`sseWriteTimeout` package-var mutation in tests** — `TestEventStreamRefreshesDeadlinePerFrame` reassigns it with `t.Cleanup` restore. Neither test calls `t.Parallel()`, and Go runs non-parallel tests in a package sequentially, so no race on the var under `-race`. Would become a real race only if someone later adds `t.Parallel()` to either test — latent but not a current bug.
- **Test `streamFrames` cleanup ordering** — `t.Fatalf` on `Get` error still runs the deferred `srv.Close()`/`resp.Body.Close()`; `sc.Err()` is the only error surfaced, which is the correct vantage point per the PR's rationale. No leaked goroutine/server on the unhappy path.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
Through the security lens, the change is clean:
- The endpoint remains authenticated (`mustActor` in `agentChat`).
- No new input surface, injection vector, or deserialization path is introduced.
- `http.NewResponseController` and `SetWriteDeadline` are standard-library mechanisms with no known unsafe behavior.
- The `sseWriteTimeout` variable is package-private; it cannot be mutated by untrusted code.
- The per-frame write deadline is a defensive bound (30 s) rather than an unlimited removal of the deadline, which is the correct posture against a stalled peer.
Nothing in this diff creates an authn/authz gap, leaks credentials, or introduces unsafe handling of untrusted data.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found.**
The PR is clean and well-structured:
- **`agent.go:124`**: `sseWriteTimeout` is a package-level `var` (not `const`) solely to allow tests to override it. This is a pragmatic, low-churn testing seam, and the comment explains exactly why.
- **Comments are high-signal**: Every non-trivial decision — the invisible server-side failure mode, why the error is deliberately unchecked per-frame, why `ResponseController` is probed once up front — is documented without being verbose.
- **`eventStream` struct change is minimal**: Adding `rc *http.ResponseController` alongside the existing `c *gin.Context` is the natural place for it.
- **Test structure is clean**: `streamFrames` is a well-factored helper that returns `(count, error)` and lets both test cases express their specific assertions directly. The two test cases have distinct, non-overlapping responsibilities.
- **No dead code, no duplication, no confusing naming**. The diff follows the surrounding patterns (e.g., `slog.Error` with structured keys, `t.Cleanup` for restoring mutated globals).
One thing I checked and confirmed is safe: `TestEventStreamRefreshesDeadlinePerFrame` mutates the package-level `sseWriteTimeout` without a mutex, but `t.Parallel()` is not used anywhere in this package, so there's no concurrent test risk. No other test touches it.
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
The change handles its edge cases carefully and the error paths are well considered.
- **Probe-before-use pattern**: `openEventStream` probes `SetWriteDeadline` once and logs with `slog.Error` if deadlines are unavailable. The stream continues to work in that degraded case — correct fallback behavior.
- **Ignored `SetWriteDeadline` error in `write`**: Deliberately and correctly unchecked. The `rc` probes the same underlying capability on every call; an error here can only repeat what `openEventStream` already reported, so suppressing it avoids log spam without hiding new information.
- **Global var in test**: `sseWriteTimeout` is a `var` (not a `const`) so `TestEventStreamRefreshesDeadlinePerFrame` can temporarily shrink it. The test uses `t.Cleanup` to restore the original value, and neither test in the file uses `t.Parallel()`, so there is no cross-test race.
- **Nil safety**: `eventStream` is only ever constructed via `openEventStream`, so `rc` is always non-nil. `http.NewResponseController` never returns nil. `c.Writer` in a gin handler context is always initialized.
- **Deadline timing**: `keepAliveInterval` (20s) is safely under `sseWriteTimeout` (30s), so the keep-alive ticker both paces proxies and refreshes the write deadline with a 10s margin.
Nothing in this diff introduces an unhandled nil, empty collection, boundary, overflow, or error-swallowing bug from the error-handling & edge-cases perspective.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Gadfly re-review: the regression test proved the stream outlives the server
WriteTimeout, but its whole run was under a second — far below the 30s
sseWriteTimeout — so it could NOT tell a per-frame refresh from a deadline set
once at open. A revert to set-once would reintroduce the unbounded-block risk
the per-frame refresh exists to prevent, and sail through the test.
Make sseWriteTimeout a var (production never reassigns it) so a test can shrink
it, and split into two focused cases sharing a streamFrames helper:
- TestEventStreamOutlivesServerWriteTimeout — server WriteTimeout 300ms, frames
straddling it, sseWriteTimeout left at 30s. Catches #78 (no deadline
management → server cuts the stream). Huge margin, so CI slowness only makes
it pass more surely — addresses the "timing margin tight" finding too.
- TestEventStreamRefreshesDeadlinePerFrame — sseWriteTimeout shrunk to 400ms,
8 frames at a 100ms tick (800ms total, 4× margin per gap). A per-frame refresh
delivers all 8; a set-once deadline expires mid-stream and cuts it.
Verified each fails against its own regression: removing the per-write refresh
gives 3/8 on the per-frame test (EOF at ~400ms); removing both deadline calls
gives 0/3 on the outlives test. Both pass on the real code, 3× under -count.
The "clear the deadline entirely" finding was already addressed by the previous
commit (per-frame refresh); this covers it against reintroduction.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The sharp one — "the test can't catch a regression to a set-once deadline" — was right, and it's now covered.
The old test ran entirely within ~900ms, far under the 30s sseWriteTimeout, so a deadline set once at open would have passed it just as well as a per-frame refresh — while reintroducing exactly the unbounded-block risk the per-frame refresh exists to prevent. Fixed by making sseWriteTimeout a var (production never reassigns it) and splitting into two cases:
TestEventStreamOutlivesServerWriteTimeout — catches #78. Server WriteTimeout 300ms, frames straddling it, sseWriteTimeout left at 30s so the margin is huge and CI slowness only helps it pass (addresses the timing-margin finding).
TestEventStreamRefreshesDeadlinePerFrame — catches the set-once regression. sseWriteTimeout shrunk to 400ms, 8 frames at a 100ms tick (800ms total). Per-frame refresh delivers all 8; a set-once deadline expires mid-stream.
Verified each fails against its own regression and nothing else:
set-once (drop per-write refresh) → RefreshesDeadlinePerFrame FAILS 3/8, EOF at ~400ms
OutlivesServerWriteTimeout still passes
no deadline mgmt (drop both calls) → OutlivesServerWriteTimeout FAILS 0/3
both present (real code) → both pass, 3× under -count
The "clear the deadline entirely" finding was already resolved by the previous commit (per-frame refresh); this now guards it against reintroduction rather than relying on review to catch it next time.
@gadfly review
The sharp one — "the test can't catch a regression to a set-once deadline" — was right, and it's now covered.
The old test ran entirely within ~900ms, far under the 30s `sseWriteTimeout`, so a deadline set once at open would have passed it just as well as a per-frame refresh — while reintroducing exactly the unbounded-block risk the per-frame refresh exists to prevent. Fixed by making `sseWriteTimeout` a `var` (production never reassigns it) and splitting into two cases:
- **`TestEventStreamOutlivesServerWriteTimeout`** — catches #78. Server WriteTimeout 300ms, frames straddling it, `sseWriteTimeout` left at 30s so the margin is huge and CI slowness only helps it pass (addresses the timing-margin finding).
- **`TestEventStreamRefreshesDeadlinePerFrame`** — catches the set-once regression. `sseWriteTimeout` shrunk to 400ms, 8 frames at a 100ms tick (800ms total). Per-frame refresh delivers all 8; a set-once deadline expires mid-stream.
Verified each fails against *its own* regression and nothing else:
```
set-once (drop per-write refresh) → RefreshesDeadlinePerFrame FAILS 3/8, EOF at ~400ms
OutlivesServerWriteTimeout still passes
no deadline mgmt (drop both calls) → OutlivesServerWriteTimeout FAILS 0/3
both present (real code) → both pass, 3× under -count
```
The "clear the deadline entirely" finding was already resolved by the previous commit (per-frame refresh); this now guards it against reintroduction rather than relying on review to catch it next time.
(CI infra recovered — the earlier review runs died on a docker.gitea.com outage, not anything in the diff. Re-requesting on the current head, which adds the per-frame-refresh regression test discussed above.)
@gadfly review
(CI infra recovered — the earlier review runs died on a `docker.gitea.com` outage, not anything in the diff. Re-requesting on the current head, which adds the per-frame-refresh regression test discussed above.)
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #78. Live bug — currently deployed.
What was wrong
http.Server.WriteTimeoutis an absolute deadline from when the request header was read, not an idle timeout.cmd/pansy/main.go:65sets it to 30s; an agent turn is budgeted 4 minutes. Nothing cleared it for the SSE response, so every turn over 30 seconds was cut mid-stream.runTimeout(internal/agent/runtime.go:31)keepAliveInterval(internal/api/agent.go:32)WriteTimeout(cmd/pansy/main.go:65)The 4-minute budget was unreachable. And the keep-alive from #73 — added so "a long silence while the model thinks doesn't look like a dead connection" — got exactly one tick before the connection it was pacing was destroyed underneath it. A mechanism built to survive long silences that was structurally incapable of surviving them.
Why it survived review
The failure is invisible from the handler. Writes made after the deadline return
err == niland their bytes are silently discarded.This matters for how it was found and fixed. The sweep that flagged it predicted the writes would "fail with
i/o timeout, silently (the error is discarded)" atagent.go:147— pointing at the_, _ = io.WriteString(...). That diagnosis is wrong: there is no error at any layer, so logging the discarded write error would have caught nothing and the bug would have survived a fix aimed squarely at it. Verified against a standalone reproduction before writing the patch.Only the client sees it — as
unexpected EOF, whichweb/src/lib/agent.ts:171-176reports as "The connection dropped partway through.", indistinguishable from a real network fault.The fix
One line in
openEventStream, with the error checked rather than ignored — a failure there means the stream will be cut and we can't prevent it, which is worth saying out loud.Confirmed
gin1.10.1'sresponseWriterimplementsUnwrap() http.ResponseWriter(response_writer.go:54), sohttp.NewResponseControllerreaches the underlying conn. Without that this would silently no-op, so it was worth checking rather than assuming.The test
Because the failure can't be observed server-side,
TestEventStreamOutlivesWriteTimeoutdrives a realhttp.Serverwith a shortWriteTimeoutand asserts from the client side.Against the unfixed code:
That's the production symptom reproduced in a unit test. A test that inspected the return value of
io.WriteStringwould have passed against the bug — which is the same failure mode as the undo-test fixture in #72, so it seemed worth being explicit about in the test's own comment.Also passes under
-race.Not in scope
ReadTimeout: 15shas the same absolute-deadline shape and will matter for image upload in #80 (a phone photo over a slow connection). Flagged there rather than pre-emptively changed here.GOWORK=off go test ./...green;gofmt -l internal/clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 01:53:10Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
@gadfly review
Both findings taken, and the security one was a genuine catch — I'd traded one unbounded failure for another.
Clearing the deadline entirely removes the backstop. Correct. With no deadline, a client that stops reading fills the socket buffer and blocks
Writeforever, pinning the agent run goroutine and this stream's mutex — which takes the keep-alive down with it, since it needs the same lock. The old 30sWriteTimeoutat least bounded that.Fixed by refreshing the deadline per frame rather than removing it: the stream as a whole is unbounded, but no single write is. That's the only shape that satisfies both ends — an absolute deadline cuts long turns, and no deadline can't be recovered from.
sseWriteTimeoutis deliberately generous (30s) because it's a backstop against a stuck peer, not a pacing mechanism.The probe stays in
openEventStreamso a writer that can't take deadlines is reported once instead of once per frame; the per-write call ignores its error for that reason, which the comment now states.Test timing margin. Also correct. Restructured so
tick(300ms) is greater thanwriteTimeout(200ms), which means every frame lands after the deadline has already expired and the first clears it by 100ms. The margin only grows: a slow or loaded runner delays frames further past a deadline that's already passed, so jitter pushes the test toward passing. Sized the other way — frames landing just before a deadline — it would flake exactly when CI is busiest. Verified with-count=3.Re-verified the regression detection after both changes: with the deadline handling removed the test fails at 0/3 frames with
unexpected EOF.One thing I did not add: a test for the new bounded-write property. It needs a client that stalls mid-stream and then a 30-second wait, which isn't worth the suite time. Flagging it rather than pretending it's covered.
🪰 Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +17,4 @@// once it passes, however recently the handler wrote. pansy sets it to 30s while// an agent turn may run for minutes.//// This has to be driven through a real http.Server and asserted from the CLIENT🟡 Regression test never approaches the 30s deadline boundary, so it can't catch a regression to a single set-once deadline instead of the required per-frame refresh
error-handling · flagged by 1 model
internal/api/sse_deadline_test.go:20-45— The regression test doesn't exercise the "refreshed per frame" mechanism that is the actual fix for #78.openEventStreamsets the write deadline via the hardcoded 30ssseWriteTimeout(internal/api/agent.go:120) once, at stream open, and that constant isn't overridable by the test. The test only overridessrv.Config.WriteTimeoutto 200ms (internal/api/sse_deadline_test.go:46) but total runtime isframes(3) * tick(300ms) ≈ 900ms— three ord…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: No material issues found · 0 findings (0 with multi-model agreement)
No material issues found by consensus.
Per-model detail
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens (authn/authz, injection, SSRF, deserialization, secret leakage, input validation, unsafe handling of untrusted data, insecure defaults).
What I checked:
internal/api/agent.go:58-109(agentChat) —mustActor(c)and the existingh.svc.AgentHistorypermission check run beforeopenEventStreamis called, and this diff doesn't touch that ordering, so authz gating is unaffected.SetWriteDeadlinerefresh (agent.go:189) and the one-time probe inopenEventStream(agent.go:161-168) only touch anet.Conndeadline — no user-controlled data flows into either call.sseWriteTimeoutis a fixed, unexported package var; the test mutates it in-package only (sse_deadline_test.go), not reachable from any request.slog.Errorcall atagent.go:167logs only theerrorfromSetWriteDeadline(an internalnet/httperror), no request data, no credentials.WriteTimeout) creates a resource-exhaustion angle: a client that stops reading can still only pin the write forsseWriteTimeout(30s) per attempt before the write times out, and the agent turn itself is independently capped byrunTimeout(4 min, unchanged by this PR) — so worst case is the same order of magnitude as before, not a new unbounded hold. The author's own comments inagent.go:111-124and144-156show this tradeoff was deliberately reasoned about, not overlooked.Nothing in this diff introduces or weakens an authn/authz check, and there's no injection, SSRF, deserialization, or secret-leakage vector — it's a narrowly-scoped connection-deadline fix on an already-authenticated stream.
🎯 Correctness — No material issues found
Only one construction site, always fully initialized. I've verified the deadline math, the timeout constants against their real values, the gin/
http.ResponseControllerwiring, the mutex serialization, and the two new tests' logic — no correctness issues surfaced.Review
VERDICT: No material issues found
Findings, all verified against the checked-out code:
sseWriteTimeout(30s) matches the originalWriteTimeoutatcmd/pansy/main.go:65, andkeepAliveInterval(20s,internal/api/agent.go:32) stays comfortably under it, so the keep-alive tick refreshes the deadline before it can expire during a quiet stretch — the numbers line up correctly.runTimeoutis confirmed at 4 minutes (internal/agent/runtime.go:31), consistent with the PR's claim that the per-frame refresh (not a single extended deadline) is what's needed to outlast the server's absoluteWriteTimeout.SetWriteDeadlineis called immediately before every write while holdings.mu, and no write is ever in flight when a new deadline is set (writes are serialized by the mutex), so there's no race between refreshing the deadline and an in-progress write.net/httptreatsSetWriteDeadlineas "last call wins" on the underlying conn (module is ongo 1.26.2, well past the 1.20 minimum forhttp.ResponseController), so re-arming it per frame genuinely supersedes the server's original absolute deadline — the mechanism does what the PR claims.internal/api/api.go:39only addssloggin+gin.Recovery()) wrapsc.Writer, so theUnwrap()chain the fix depends on isn't broken by anything else in this codebase.eventStreamhas exactly one construction site (agent.go:161), always fully initializingrc, so there's no nil-ResponseControllerpath.TestEventStreamOutlivesServerWriteTimeoutleavessseWriteTimeoutat its 30s default against a 300ms server timeout (would fail pre-fix), andTestEventStreamRefreshesDeadlinePerFrameshrinkssseWriteTimeoutto 400ms with 100ms-spaced frames against a generous 5s server timeout, which only passes if the deadline is refreshed per-write rather than once at open.🧹 Code cleanliness & maintainability — No material issues found
Confirmed the 30s
WriteTimeoutreferenced in the diff's comments matchescmd/pansy/main.go:65. The diff itself is unchanged there — this PR is scoped correctly tointernal/api/agent.goand its test.I've reviewed the full diff against the checked-out code. This is a clean, tightly-scoped change.
Verdict: No material issues found
Through the code cleanliness/maintainability lens specifically:
streamFrameshelper rather than copy-pasting server setup (internal/api/sse_deadline_test.go:18-49).openEventStreamandwriteremain short, single-purpose.rc *http.ResponseControlleralongsidec *gin.Context,mu sync.Mutex— verified againstinternal/api/agent.go:132-136).keepAlive,chatErrorMessage, andagentChatdoc comments), so it isn't a new maintainability wart introduced by this PR — it's consistent with the surrounding code.gofmt-relevant formatting by eye (tabs, brace placement) matches surrounding code; no stray whitespace or import churn (net/httpwas already imported).Nothing in this diff, viewed purely for readability/structure/duplication/naming, rises to a reportable finding.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens only.
The change adds one
SetWriteDeadlinecall per SSE frame (internal/api/agent.go:187) inside the existingwrite()critical section. I checked how oftenwrite()/send()actually fire on this path:agentChat(internal/api/agent.go:60-108) sends onechatEventper completed model round trip (aStep), bounded bymaxSteps = 24(internal/agent/runtime.go:27), plus one finalDone/Warningevent.internal/api/agent.go:32,keepAliveInterval = 20s) adds at most ~12 more frames over the 4-minuterunTimeout.So this is at most a few dozen writes over a multi-minute stream — not a hot loop, not a per-token streaming path.
http.ResponseController.SetWriteDeadlineon a TCP conn is a cheap netpoller timer reset, not a syscall-heavy operation, and it's already inside the same mutex-guarded critical section as the write it protects, so it adds no new lock contention. Thehttp.ResponseControlleritself is allocated once inopenEventStream(internal/api/agent.go:160) and stored oneventStream, not reallocated per frame.No N+1s, no unbounded growth, no missing pagination, no quadratic behavior, and no new blocking-call concerns beyond what the PR itself documents (the deadline is explicitly a bounded backstop, not a pacing mechanism). The new test spins up a real
http.Serverwith sub-second timeouts and closes it viadefer srv.Close()— bounded and self-contained.Nothing to flag in this lane.
🧯 Error handling & edge cases — No material issues found
Verification
Read
internal/api/agent.goin full against the diff. Every claim in the draft's reasoning checks out against the actual code:sseWriteTimeoutis a package-levelvar(30s), reassigned only in the test (sse_deadline_test.go), confirmed unused elsewhere in production code.write()ignores theSetWriteDeadlineerror return (_ = s.rc.SetWriteDeadline(...)) whileopenEventStreamlogs it once viaslog.Error— matches the stated "probe once, refresh silently" design.s.mu) is held only across a singlewrite()call, so a stalled client blocks at most for one write, bounded bysseWriteTimeout(30s), not indefinitely — confirmed no other lock ordering issue withkeepAlive's goroutine, which also goes throughs.write()under the same lock.keepAlive's stop issync.Once-guarded and selects on both an explicitdonechannel andc.Request.Context().Done(), so no goroutine leak.http.NewResponseController(c.Writer)against gin'sResponseWriter— gin has implementedUnwrap() http.ResponseWritersince v1.9.0 specifically to supporthttp.ResponseController, andgo.modpinsgin-gonic/gin v1.10.1, so this resolves correctly (not confirmable via local build due to sandbox restrictions outside the repo directory, but consistent with known gin API history).http.Flusher.Flush()has no error return, so nothing is silently swallowed there beyond what's already documented.The draft contains no bulleted findings to individually confirm or reject — it is a clean "no issues" verdict, and independent reading of the diff supports that verdict for the error-handling/edge-case lens.
VERDICT: No material issues found
glm-5.2:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens. The diff replaces an absolute
http.Server.WriteTimeoutwith a per-write deadline on the SSE response. Security-relevant observations:sseWriteTimeout = 30s) rather than removing deadlines entirely. A client that stops reading will still have its writes time out within 30s once the socket buffer fills — the run goroutine and the stream mutex are not pinned indefinitely. The overall turn is still bounded byrunTimeout(4 min). So this does not create a new vector for connection-goroutine exhaustion; if anything it makes the intended long-lived-stream case explicit while preserving the stuck-peer backstop.time.Now()and the package-levelsseWriteTimeoutconstant — no request data flows intoSetWriteDeadlineor the response body. The frame content still goes throughjson.Marshalinsend(agent.go:174), unchanged.openEventStreamis called aftermustActor(c)and request binding inagentChat(agent.go:58-72); the auth surface is untouched.sseWriteTimeoutis avar, notconst. It's package-level and reassignable, but only the test mutates it (undert.Cleanuprestore). Production never reassigns it, and it's not exported, so no external tampering surface. Not a security concern.http.NewResponseController(c.Writer)— no credential/secret handling involved; the PR description's verification of gin'sUnwrap()is correctness, not security.Nothing in my lens is materially wrong.
🎯 Correctness — No material issues found
VERDICT: No material issues found
Through the correctness lens I verified:
openEventStream(agent.go:157-171) sets the write deadline before the first flush, andwrite(agent.go:182-192) refreshes it beforeio.WriteString+Flush, so each individual write (write + flush) is bounded while the stream as a whole is not. Ordering is correct.keepAlive's writes go through the samewritepath undermu, so the deadline is refreshed for keep-alive frames too and there's no concurrent unsynchronizedSetWriteDeadlineon the conn (andhttp.ResponseControlleris concurrency-safe regardless).http.Server.WriteTimeoutis an absolute deadline from header read, and writes made after it on a Go HTTP server returnnilwith bytes silently dropped — the client sees EOF. That's why a client-side assertion is the only way to catch it, and why checkingio.WriteString's return would have missed the bug. Matches the code at agent.go:190 (_, _ = io.WriteString).cmd/pansy/main.go:65confirmsWriteTimeout: 30 * time.Second, andrunTimeout/keepAliveIntervalare as described in the PR, so the 4-min budget really was unreachable without this fix.Test timing re-derived from first principles:
TestEventStreamOutlivesServerWriteTimeout: serverWriteTimeout=300ms, frames at 300/600/900ms. Unfixed code cuts at 300ms (≈1 frame); fixed code, withsseWriteTimeout=30sdefault, refreshes per frame and delivers all 3. Correct.TestEventStreamRefreshesDeadlinePerFrame:sseWriteTimeout=400ms, 8 frames at 100ms intervals spanning 800ms, serverWriteTimeout=5s. Per-frame refresh delivers all 8; a set-once deadline set at open (t=0, expires 400ms) would cut mid-stream ≈400ms — exactly what the comment claims. The 100ms gap is a 4× margin under the 400ms deadline, so a correctly-refreshing implementation passes reliably even under CI jitter. Correct.I could not read the vendored gin
response_writer.go(no vendor/ dir) to independently confirmUnwrap() http.ResponseWriter, but go.mod pinsgin v1.10.1as the PR states, and this is a known gin feature; the author verified it against the source. Accepting.No correctness defects found.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
slog.Errorwith"error"field matches the existing logging atagent.go:104,176; the doc-comment style (multi-paragraph//with deliberate emphasis) is consistent with the file's other comments; the newrc *http.ResponseControllerfield slots into the existing struct cleanly.sseWriteTimeoutas avar(rather thanconst) purely for test overridability is a deliberate, explicitly-justified trade-off. It is the only mutable package-level global introduced, and neither test callst.Parallel(), so the reassignment inTestEventStreamRefreshesDeadlinePerFrame(witht.Cleanuprestore) is race-free under the default sequential test execution. Not a cleanliness smell worth flagging.streamFramesis correctly shared by both tests; the per-frame_ = s.rc.SetWriteDeadline(...)inwriteand the one-shot probe inopenEventStreamare intentionally distinct (probe logs, hot path stays silent) and both are explained.sseWriteTimeout,rc,streamFrames); no leaky abstraction or copy-paste.internal/api/agent.go:1-220andinternal/api/sse_deadline_test.goin full; confirmednet/httpimport already present (agent.go:10) andtime(agent.go:12), so no import churn was needed.⚡ Performance — No material issues found
VERDICT: No material issues found
write(agent.go:189) is onetime.Now().Addplus oneSetWriteDeadlinesyscall per SSE frame. SSE frame rate here is low — a handful of step events per agent turn plus a keep-alive comment everykeepAliveInterval(20s) — so the per-frame cost is negligible, not a hot-loop concern. Verified by readingkeepAlive(agent.go:194-223) and the handler flow: frames are agent-step bursts and 20s tickers, not a tight loop.http.NewResponseController(c.Writer)is constructed once per stream inopenEventStream(agent.go:161), not per frame, so no per-frame allocation regression.writealready serialized keep-alive and run goroutines, and this change doesn't widen the critical section beyond a single non-blocking deadline call.Verified via
read_fileoninternal/api/agent.go:100-223and grep overinternal/apifor write/keep-alive call sites.🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 4: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 3/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
🎯 Correctness — No material issues found
No material issues found
Server.WriteTimeout(which is measured from request header read) to a per-write deadline refreshed on every frame, which is the only way to keep an SSE stream alive across multi-minute agent turns.http.NewResponseController(c.Writer)+SetWriteDeadlineis the right mechanism for this.sseWriteTimeoutvar mutation in tests is safe becauset.Parallel()is not used.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The code is clean and well-maintained. The test helper
streamFramesis focused and reusable. ThesseWriteTimeoutvariable is explicitly and honestly documented as avar(notconst) solely for test override — a defensible trade-off with a clear comment explaining why. Comments onopenEventStreamand the struct are extensive but proportionate to the subtlety of the invisible failure mode. No dead code, duplicated logic, or naming issues detected.⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 01:44:11.846)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 01:44:11.846)
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the security lens. The change is a server-side timeout/robustness fix; it touches no authn/authz, input parsing, or data flow.
POST /agent/chatis registered underv1.Group("/agent", h.requireAuth())(internal/api/api.go:153), and the handler callsh.svc.AgentHistorywhich enforcesrequireGardenRole(..., roleEditor)(internal/service/agent.go:25). The diff doesn't alter the request path or any permission check.sseWriteTimeout = 30s(internal/api/agent.go:124) bounds each write, so a slow/stopped-reading client can't block a write forever. The overall run is still capped byrunTimeout = 4 * time.Minute(internal/agent/runtime.go:31) viacontext.WithTimeout(runtime.go:96), so a malicious peer can't hold the goroutine/stream beyond that. Removing the server's absoluteWriteTimeoutfor this stream does not open an unbounded-resource vector.var sseWriteTimeoutis only mutated inside the test file (sse_deadline_test.go) with at.Cleanuprestore; production never reassigns it, so no concurrent-mutation/race concern in the served path.🎯 Correctness — No material issues found
No material issues found
I verified the fix end-to-end against the checked-out code:
cmd/pansy/main.go:65confirmsWriteTimeout: 30 * time.Second, andinternal/agent/runtime.go:31,96confirmsrunTimeout = 4 * time.Minuteapplied viacontext.WithTimeout. The claim that the absolute 30s write deadline structurally truncates a 4-minute turn is correct —http.Server.WriteTimeoutis set once on the conn at header-read time, not an idle timeout.internal/api/agent.go:157-171—http.NewResponseController(c.Writer)reaches the underlying conn viaUnwrap.SetWriteDeadline(time.Now().Add(sseWriteTimeout))at open replaces the conn's absolute write deadline with a per-write one; this is the correct mechanism to overrideWriteTimeoutfor a streaming response. I could not directly open gin'sresponse_writer.go(no vendored copy / module cache in the workspace), so theUnwrap()availability for gin 1.10.1 is the one link I'm taking on the PR's stated verification rather than re-confirming myself — but if it were missing,SetWriteDeadlinereturnsErrUnsupported, the probe logs once, and behavior falls back to the pre-fix state, so the failure mode is the old bug, not a new one.agent.go:182-192—write()holdsmuacross theSetWriteDeadline+io.WriteString+Flushsequence. This is load-bearing for the per-frame deadline: the keep-alive goroutine (keepAlive,agent.go:198-223) and the run goroutine both callwrite(), so the mutex correctly serializes deadline-then-write as an atomic unit. A write and its deadline refresh cannot interleave with another frame's. Correct.openEventStreamsets the initial deadline but every actual frame refreshes it inwrite(); the probe's own deadline value is effectively redundant for writes (only there to detect unsupported writers), which matches the comment.sse_deadline_test.go:62-74— serverWriteTimeout=300ms, tick=300ms, 3 frames.time.Sleepguarantees ≥ duration, so the first write lands at ≥300ms, i.e. at/after the absolute deadline; against the unfixed code the client gets <3 frames (the documented regression symptom). With the fix,openEventStreamoverrides the conn deadline to 30s before any sleep, so no interference. Sound.sse_deadline_test.go:85-101—sseWriteTimeout=400ms, serverWriteTimeout=5s(not the limiter), 8 frames at 100ms tick spanning 800ms. A set-once deadline at 400ms would cut at ~frame 4-5; per-frame refresh keeps each write 100ms after its deadline was set, a 4× margin. This genuinely distinguishes "refresh per frame" from "set once" — the test's stated purpose. Correct.sseWriteTimeoutvar reassign in the test is goroutine-safe: it happens-beforesrv.Start()/handler invocation via goroutine-creation happens-before, and package tests run serially (not.Parallel()). No-raceconcern.On the semantic/deadline-math angle specifically: the per-frame
time.Now().Add(sseWriteTimeout)gives each individual write 30s, the stream as a whole is unbounded, and a stuck reader is bounded to ≤30s per write — which is exactly the stated invariant. No off-by-one or wrong-unit issue in the deadline arithmetic.🧹 Code cleanliness & maintainability — No material issues found
The code is clean. Let me verify the test's reliance on the keep-alive mutex comment and the comment claim about issue numbers.
The code is clean and well-structured. Comments are accurate, mutex claim verified. Test issue references are consistent with PR description (the second test guards the per-frame refresh which is the same fix's robustness, #87 referenced in comments only).
VERDICT: No material issues found
The change through the cleanliness/maintainability lens:
sseWriteTimeoutas avarfor testability: This is a pragmatic, well-documented choice. The comment atagent.go:121-123explains exactly why it's not a const and that production never reassigns it. The alternative (injecting the duration) would add a parameter threading throughopenEventStreamfor no production benefit. Clean.keepAliveatagent.go:194-218).openEventStreamlogs the probe failure once (agent.go:166-168);writedeliberately ignores it (agent.go:185-189) with a comment explaining why per-frame logging would be noise. This is a considered, consistent split, not sloppy.rc *http.ResponseControlleradded betweencandmuatagent.go:132-136. Slightly interrupts the existing grouping, but it's a minor stylistic preference and matches no violated convention in the file.streamFramesis a well-factored helper shared by both tests.chatEvent{Error: "frame"}is a lightweight reuse of the production event type (verified atagent.go:41-51), not a fixture that asserts a mental model. Test names are descriptive; issue references in comments are consistent with the PR description.I verified by reading
agent.goin full and confirmingchatEvent/openEventStream/write/keepAlivematch the diff's context, and that the mutex claim in thesseWriteTimeoutcomment (keep-alive sharesmu) holds atagent.go:212.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed
internal/api/agent.go(the SSE deadline fix) andinternal/api/sse_deadline_test.gothrough the performance lens.SetWriteDeadlineis now called per frame insideeventStream.write(agent.go:189), but frame frequency is low (step events plus a keep-alive every 20s perkeepAliveInterval), and the call is a cheap deadline update under the already-helds.mu— no new allocation, no extra syscall of consequence on this path. No hot-loop regression.http.NewResponseController(c.Writer)is constructed once per stream inopenEventStream(agent.go:161), not per write. No per-frame allocation growth.sseWriteTimeoutpackage-levelvaris read, never reassigns in production; the test reassigns it once with a cleanup. No contention concern.Nothing materially wrong in the performance lens.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed the change through the error-handling & edge-case lens, verifying against the checked-out code.
write()(agent.go:189) —_ = s.rc.SetWriteDeadline(...). This is intentional and correctly justified:openEventStreamalready probes once and logs if deadlines are unsupported, andSetWriteDeadlineis deterministic per-conn (it fails the same way every time), so per-frame error checks would spam identical log lines. Verified by reading both call sites. Not a real issue.openEventStreamprobe error path (agent.go:166-168) — failure is logged and the stream proceeds with degraded behavior (falls back to being cut at the serverWriteTimeout). Honest degradation, not a swallowed error. The comment is explicit that the stream "still works" in that case.io.WriteStringexceedingsseWriteTimeoutwould be cut partway and produce a truncated SSE frame, with the write error still ignored atagent.go:190. This is preexisting behavior (theio.WriteStringerror was ignored before this PR too), and the failure mode (truncated stream seen by client) is no worse than the prior absolute-deadline behavior. Not introduced here.openEventStreamsets T0+30s; first keep-alive at T0+20s refreshes before expiry; every subsequent write (keep-alive or real frame) refreshes again. No window where the deadline can lapse silently. Verified viakeepAliveatagent.go:198-223routing through the samewrite()path.sseWriteTimeoutpackage-var mutation in tests —TestEventStreamRefreshesDeadlinePerFramereassigns it witht.Cleanuprestore. Neither test callst.Parallel(), and Go runs non-parallel tests in a package sequentially, so no race on the var under-race. Would become a real race only if someone later addst.Parallel()to either test — latent but not a current bug.streamFramescleanup ordering —t.FatalfonGeterror still runs the deferredsrv.Close()/resp.Body.Close();sc.Err()is the only error surfaced, which is the correct vantage point per the PR's rationale. No leaked goroutine/server on the unhappy path.opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Through the security lens, the change is clean:
mustActorinagentChat).http.NewResponseControllerandSetWriteDeadlineare standard-library mechanisms with no known unsafe behavior.sseWriteTimeoutvariable is package-private; it cannot be mutated by untrusted code.Nothing in this diff creates an authn/authz gap, leaks credentials, or introduces unsafe handling of untrusted data.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The PR is clean and well-structured:
agent.go:124:sseWriteTimeoutis a package-levelvar(notconst) solely to allow tests to override it. This is a pragmatic, low-churn testing seam, and the comment explains exactly why.ResponseControlleris probed once up front — is documented without being verbose.eventStreamstruct change is minimal: Addingrc *http.ResponseControlleralongside the existingc *gin.Contextis the natural place for it.streamFramesis a well-factored helper that returns(count, error)and lets both test cases express their specific assertions directly. The two test cases have distinct, non-overlapping responsibilities.slog.Errorwith structured keys,t.Cleanupfor restoring mutated globals).One thing I checked and confirmed is safe:
TestEventStreamRefreshesDeadlinePerFramemutates the package-levelsseWriteTimeoutwithout a mutex, butt.Parallel()is not used anywhere in this package, so there's no concurrent test risk. No other test touches it.⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
🧯 Error handling & edge cases — No material issues found
No material issues found
The change handles its edge cases carefully and the error paths are well considered.
openEventStreamprobesSetWriteDeadlineonce and logs withslog.Errorif deadlines are unavailable. The stream continues to work in that degraded case — correct fallback behavior.SetWriteDeadlineerror inwrite: Deliberately and correctly unchecked. Thercprobes the same underlying capability on every call; an error here can only repeat whatopenEventStreamalready reported, so suppressing it avoids log spam without hiding new information.sseWriteTimeoutis avar(not aconst) soTestEventStreamRefreshesDeadlinePerFramecan temporarily shrink it. The test usest.Cleanupto restore the original value, and neither test in the file usest.Parallel(), so there is no cross-test race.eventStreamis only ever constructed viaopenEventStream, sorcis always non-nil.http.NewResponseControllernever returns nil.c.Writerin a gin handler context is always initialized.keepAliveInterval(20s) is safely undersseWriteTimeout(30s), so the keep-alive ticker both paces proxies and refreshes the write deadline with a 10s margin.Nothing in this diff introduces an unhandled nil, empty collection, boundary, overflow, or error-swallowing bug from the error-handling & edge-cases perspective.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
@gadfly review
The sharp one — "the test can't catch a regression to a set-once deadline" — was right, and it's now covered.
The old test ran entirely within ~900ms, far under the 30s
sseWriteTimeout, so a deadline set once at open would have passed it just as well as a per-frame refresh — while reintroducing exactly the unbounded-block risk the per-frame refresh exists to prevent. Fixed by makingsseWriteTimeoutavar(production never reassigns it) and splitting into two cases:TestEventStreamOutlivesServerWriteTimeout— catches #78. Server WriteTimeout 300ms, frames straddling it,sseWriteTimeoutleft at 30s so the margin is huge and CI slowness only helps it pass (addresses the timing-margin finding).TestEventStreamRefreshesDeadlinePerFrame— catches the set-once regression.sseWriteTimeoutshrunk to 400ms, 8 frames at a 100ms tick (800ms total). Per-frame refresh delivers all 8; a set-once deadline expires mid-stream.Verified each fails against its own regression and nothing else:
The "clear the deadline entirely" finding was already resolved by the previous commit (per-frame refresh); this now guards it against reintroduction rather than relying on review to catch it next time.
@gadfly review
(CI infra recovered — the earlier review runs died on a
docker.gitea.comoutage, not anything in the diff. Re-requesting on the current head, which adds the per-frame-refresh regression test discussed above.)