Commit history detached from cancellation on every path, not just failure #73

Merged
steve merged 2 commits from fix/commit-survives-disconnect into main 2026-07-21 12:49:18 +00:00
Owner

Found by using it. 18 plantings in the garden with no change set behind them — real changes with no way to undo them, which is the one guarantee this whole design rests on.

What happened

I asked the assistant to plant beans in a bed. It did: find_plantcreate_plantdescribe_gardenfill_region, 18 plops landed on the canvas. Then the client's SSE stream dropped, and the panel said "The connection dropped partway through."

The beans were there. The change set was not. Neither was the conversation turn. Waiting past the 4-minute run timeout changed nothing — it was never going to appear.

Why my previous fix missed it

#70 detached the failure path from cancellation, reasoning that a cancelled context is why fn failed. That reasoning was right and the fix was incomplete, because it missed the commoner case:

A turn whose client disconnects can still complete. The model finishes, the tools have already written their rows, fn returns nil — and then the success path called commitScope(ctx, …) with a context that had died in the meantime. The write failed, WithChangeSet returned an error, and the work was orphaned.

I fixed the path I was thinking about and left the one that actually runs.

The fix

commitScope detaches from cancellation itself, so every caller gets it. That's the right home for the rule:

By the time a commit runs, the data it describes has already been written. Cancelling it cannot undo anything — it can only lose the record of what happened.

There is no path on which the old behaviour is what anyone wants.

Also: stop the connection dropping in the first place

A model thinking between tool calls sends nothing for a while, and an idle proxy will cut a quiet stream. An SSE comment frame every 20s keeps it open; SSE ignores comment frames, so the client is unaffected.

That introduced a second goroutine writing the response, so the event stream now serializes writes behind a mutex — step events come from the agent's run goroutine while the ticker writes from its own, and two goroutines writing a ResponseWriter concurrently corrupts frames long before it crashes anything. My first draft of the keep-alive had a comment claiming the writes couldn't overlap; they could. Verified under -race.

Test

TestSucceededTurnRecordsEvenIfTheCallerWentAwayfn does its work and succeeds, the caller's context is cancelled before it returns, and the assertion is that the change set exists, isn't marked partial, and reverts cleanly.

The existing TestPartialWorkSurvivesATimeout covered the failure path and passed throughout, which is exactly why this got through.

go test -race ./..., go vet, gofmt green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

Found by using it. **18 plantings in the garden with no change set behind them** — real changes with no way to undo them, which is the one guarantee this whole design rests on. ### What happened I asked the assistant to plant beans in a bed. It did: `find_plant` → `create_plant` → `describe_garden` → `fill_region`, 18 plops landed on the canvas. Then the client's SSE stream dropped, and the panel said *"The connection dropped partway through."* The beans were there. The change set was not. Neither was the conversation turn. Waiting past the 4-minute run timeout changed nothing — it was never going to appear. ### Why my previous fix missed it #70 detached the **failure** path from cancellation, reasoning that a cancelled context is *why* `fn` failed. That reasoning was right and the fix was incomplete, because it missed the commoner case: **A turn whose client disconnects can still complete.** The model finishes, the tools have already written their rows, `fn` returns `nil` — and then the **success** path called `commitScope(ctx, …)` with a context that had died in the meantime. The write failed, `WithChangeSet` returned an error, and the work was orphaned. I fixed the path I was thinking about and left the one that actually runs. ### The fix `commitScope` detaches from cancellation **itself**, so every caller gets it. That's the right home for the rule: > By the time a commit runs, the data it describes has already been written. Cancelling it cannot undo anything — it can only lose the record of what happened. There is no path on which the old behaviour is what anyone wants. ### Also: stop the connection dropping in the first place A model thinking between tool calls sends nothing for a while, and an idle proxy will cut a quiet stream. An SSE comment frame every 20s keeps it open; SSE ignores comment frames, so the client is unaffected. That introduced a second goroutine writing the response, so the event stream now serializes writes behind a mutex — step events come from the agent's run goroutine while the ticker writes from its own, and **two goroutines writing a `ResponseWriter` concurrently corrupts frames long before it crashes anything.** My first draft of the keep-alive had a comment claiming the writes couldn't overlap; they could. Verified under `-race`. ### Test `TestSucceededTurnRecordsEvenIfTheCallerWentAway` — `fn` does its work and **succeeds**, the caller's context is cancelled before it returns, and the assertion is that the change set exists, isn't marked partial, and reverts cleanly. The existing `TestPartialWorkSurvivesATimeout` covered the failure path and passed throughout, which is exactly why this got through. `go test -race ./...`, `go vet`, `gofmt` green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve added 1 commit 2026-07-21 12:38:48 +00:00
Commit history detached from cancellation on EVERY path, not just failure
Build image / build-and-push (push) Successful in 9s
Gadfly review (reusable) / review (pull_request) Successful in 10m8s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m8s
9ef4593fa8
Found by using it: the agent planted 18 beans, the client's stream dropped, and
those 18 plantings ended up in the garden with no change set behind them — real
changes with no way to undo them, which is the one guarantee this whole design
rests on.

The earlier fix detached the FAILURE path and reasoned that a cancelled context
is why fn failed. That missed the commoner case. An agent turn whose client
disconnects can still COMPLETE — the model finishes, the tools have already
written — and then the success path committed with a dead context, the write
failed, WithChangeSet returned an error, and the work was orphaned.

commitScope now detaches from cancellation itself, so every caller gets it. That
is the right home for the rule: by the time a commit runs, the data it describes
has already been written, so cancelling it cannot undo anything — it can only
lose the record of what happened. There is no path on which that is the
behaviour anyone wants.

Also added an SSE keep-alive, since the dropped connection is worth not having
in the first place: a model thinking between tool calls sends nothing for a
while, and an idle proxy will cut a quiet stream. A comment frame every 20s
keeps it open, and SSE ignores comment frames so the client is unaffected.

That introduced a second goroutine writing the response, so the event stream now
serializes writes behind a mutex — step events come from the agent's run
goroutine while the ticker writes from its own, and two goroutines writing a
ResponseWriter concurrently corrupts frames long before it crashes anything.
Verified under -race.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-21 12:48:56Z

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 — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

kimi-k2.6:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — No material issues found

opencode/glm-5.2:cloud · opencode — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — No material issues found
  • performance — No material issues found
  • error-handling — Minor issues

opencode/kimi-k2.6:cloud · opencode — done

  • security — No material issues found
  • ⚠️ correctness — could not complete
  • ⚠️ maintainability — could not complete
  • ⚠️ performance — could not complete
  • error-handling — No material issues found

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-21 12:48:56Z #### `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** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ⚠️ **correctness** — could not complete - ⚠️ **maintainability** — could not complete - ⚠️ **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>
steve added 1 commit 2026-07-21 12:48:46 +00:00
Route auto-scoped mutations through the detached commit too
Build image / build-and-push (push) Successful in 5s
d59303238f
Gadfly caught me making the same mistake one level up. This PR claimed the
history write is "detached from cancellation, always" — and record()'s
auto-scope path still called store.WriteChangeSet directly with the caller's
context, so every plain REST mutation kept the orphan-history window the PR was
written to close. That is the path virtually every change takes; the agent is
the exception.

It goes through commitScope now, which is where the rule lives, so no caller can
be the one that forgets. Tested the same way as the agent path: cancel right
after the mutation returns, assert the change set exists and reverts.

Also: stopBeat is deferred so a panic in the run can't leak the ticker goroutine
(and is idempotent, since the handler stops it explicitly on the normal path);
the keep-alive interval is a named constant rather than a literal duplicated
between comment and call site; and commitScope's doc states the rule and why it
lives there, instead of narrating the incident that produced it — that belongs
in a commit message, which is where it now is.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
gitea-actions bot reviewed 2026-07-21 12:48:56 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -64,2 +71,3 @@
send := openEventStream(c)
stream := openEventStream(c)
send := stream.send

🟡 Magic number keep-alive interval should be a named constant

maintainability · flagged by 2 models

  • internal/api/agent.go:73 — Magic number heartbeat interval. The keep-alive duration 20 * time.Second is hardcoded as a literal at the call site, while every other timeout/duration in this package is a named constant (oidcDiscoveryTimeout, oidcExchangeTimeout, etc.). This breaks the project's existing pattern, buries the tuning knob, and makes it non-discoverable for anyone adjusting proxy timeouts later. It should be a package-level constant such as `sseKeepAliveInterval = 20 * tim…

🪰 Gadfly · advisory

🟡 **Magic number keep-alive interval should be a named constant** _maintainability · flagged by 2 models_ * **`internal/api/agent.go:73` — Magic number heartbeat interval.** The keep-alive duration `20 * time.Second` is hardcoded as a literal at the call site, while every other timeout/duration in this package is a named constant (`oidcDiscoveryTimeout`, `oidcExchangeTimeout`, etc.). This breaks the project's existing pattern, buries the tuning knob, and makes it non-discoverable for anyone adjusting proxy timeouts later. It should be a package-level constant such as `sseKeepAliveInterval = 20 * tim… <sub>🪰 Gadfly · advisory</sub>
@@ -66,0 +76,4 @@
// idle proxy will cut a quiet connection. Deferred so a panic in the run
// can't leak the ticker goroutine; stopping it twice is harmless.
stopBeat := stream.keepAlive(keepAliveInterval)
defer stopBeat()

🟡 stopBeat is called manually instead of via defer, so a panic in h.agent.Run skips cleanup (mitigated only by request-context cancellation)

error-handling · flagged by 2 models

  • internal/api/agent.go:79stopBeat is called manually instead of via defer. stopBeat := stream.keepAlive(...) is started at line 73, then h.agent.Run(...) runs (lines 74–78), then stopBeat() is called as a plain statement at line 79 (before the error check). If Run (or anything between the two lines) panics, stopBeat() is never reached. The keep-alive goroutine does have a self-cleanup escape hatch — it also selects on s.c.Request.Context().Done() (line 159), and a panic…

🪰 Gadfly · advisory

🟡 **stopBeat is called manually instead of via defer, so a panic in h.agent.Run skips cleanup (mitigated only by request-context cancellation)** _error-handling · flagged by 2 models_ - **`internal/api/agent.go:79` — `stopBeat` is called manually instead of via `defer`.** `stopBeat := stream.keepAlive(...)` is started at line 73, then `h.agent.Run(...)` runs (lines 74–78), then `stopBeat()` is called as a plain statement at line 79 (before the error check). If `Run` (or anything between the two lines) panics, `stopBeat()` is never reached. The keep-alive goroutine does have a self-cleanup escape hatch — it also selects on `s.c.Request.Context().Done()` (line 159), and a panic… <sub>🪰 Gadfly · advisory</sub>
@@ -157,0 +156,4 @@
// commit runs, the data changes it describes have already been written — so
// cancelling it cannot undo anything. It can only lose the record of what
// happened and leave real changes with no way to undo them, which is the one
// thing the whole change-set design exists to prevent.

🟡 commitScope doc embeds a production incident post-mortem that belongs in the commit message, not godoc

maintainability · flagged by 1 model

  • internal/service/revisions.go:159 — doc comment carries a production post-mortem that will rot. The commitScope comment now embeds a historical narrative ("Found in production with 18 plantings and no change set behind them") alongside the durable rule. The rule ("the write is detached from cancellation, always") is worth keeping; the incident report belongs in the commit message / PR description, not the godoc, where it reads as rationale that future readers can't act on and will be t…

🪰 Gadfly · advisory

🟡 **commitScope doc embeds a production incident post-mortem that belongs in the commit message, not godoc** _maintainability · flagged by 1 model_ - **`internal/service/revisions.go:159` — doc comment carries a production post-mortem that will rot.** The `commitScope` comment now embeds a historical narrative ("Found in production with 18 plantings and no change set behind them") alongside the durable rule. The rule ("the write is detached from cancellation, always") is worth keeping; the incident report belongs in the commit message / PR description, not the godoc, where it reads as rationale that future readers can't act on and will be t… <sub>🪰 Gadfly · advisory</sub>
@@ -205,0 +207,4 @@
// detached path as everything else — a REST client that hangs up right after
// its PATCH landed must not leave that change without history, and this is
// the path virtually every mutation takes.
auto := &changeScope{gardenID: gardenID, actorID: actorID, source: domain.SourceUI, summary: summary}

🟠 Auto-scope record() path still commits history with the caller's (possibly dead) context — same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation

correctness, error-handling · flagged by 1 model

  • internal/service/revisions.go:210 — auto-scope record path still commits history with the caller's (possibly dead) context, the same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation. The PR's thesis (the new commitScope comment, revisions.go:154–157: "The write is DETACHED FROM CANCELLATION, always… it can only lose the record of what happened and leave real changes with no way to undo them") applies equally to the auto-scope bran…

🪰 Gadfly · advisory

🟠 **Auto-scope record() path still commits history with the caller's (possibly dead) context — same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation** _correctness, error-handling · flagged by 1 model_ - **`internal/service/revisions.go:210` — auto-scope `record` path still commits history with the caller's (possibly dead) context, the same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation.** The PR's thesis (the new `commitScope` comment, revisions.go:154–157: "The write is DETACHED FROM CANCELLATION, always… it can only lose the record of what happened and leave real changes with no way to undo them") applies equally to the auto-scope bran… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 5 models

Verdict: Minor issues · 7 findings (2 with multi-model agreement)

Finding Where Models Lens
🟡 Magic number keep-alive interval should be a named constant internal/api/agent.go:73 2/5 maintainability
🟡 stopBeat is called manually instead of via defer, so a panic in h.agent.Run skips cleanup (mitigated only by request-context cancellation) internal/api/agent.go:79 2/5 error-handling
5 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Auto-scope record() path still commits history with the caller's (possibly dead) context — same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation internal/service/revisions.go:210 opencode/glm-5.2:cloud correctness, error-handling
🟡 Trivial, optional: agent.go:79 claude-code/sonnet error-handling
🟡 Material finding: agent.go:140 claude-code/sonnet error-handling
🟡 Material finding: cmd/pansy/main.go:65 claude-code/sonnet error-handling
🟡 commitScope doc embeds a production incident post-mortem that belongs in the commit message, not godoc internal/service/revisions.go:159 glm-5.2:cloud maintainability
Per-model detail
claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues — 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, insecure defaults).

Notes from verification:

  • internal/service/revisions.go:116requireGardenRole(ctx, actorID, gardenID, roleEditor) authorization check in WithChangeSet is untouched by this diff; RevertChangeSet (revisions.go:300) keeps its own requireGardenRole check before the loop that now calls the always-detaching commitScope. No authz check was weakened or bypassed by making commitScope unconditionally use context.WithoutCancel.
  • context.WithoutCancel(ctx) only strips Done()/Err()/Deadline()Value() lookups still forward to the parent context, so no actor/session-scoped values used for auth are dropped by the now-unconditional detach in commitScope (revisions.go:169).
  • The new eventStream.keepAlive writes a fixed, hardcoded SSE comment (": keep-alive\n\n", agent.go:162) — no interpolation of any request- or model-derived data, so no injection/frame-smuggling vector there. Regular send() events still go through json.Marshal before being written (agent.go:129-134), unchanged from before.
  • The sync.Mutex correctly serializes the two writers (agent step callback vs. keep-alive ticker) onto the same ResponseWriter (agent.go:137-142); stopBeat() is called synchronously and blocks on <-stopped (agent.go:166-169) before the handler proceeds to write further events, so there's no window where an unsynchronized writer could still be active — not a security bug in this lens but worth noting I checked it since it looked concurrency-sensitive.
  • Detaching commitScope from cancellation doesn't introduce a new DoS/amplification surface: the underlying mutation (fn) has already executed and written its rows by the time commitScope runs: this only persists a history record of already-completed, already-authorized work, not new attacker-triggered work per disconnect.

No authn/authz gaps, injection, unsafe deserialization, secret leakage, or insecure defaults found in this diff.

🎯 Correctness — No material issues found

Verdict: No material issues found

I read internal/service/revisions.go and internal/api/agent.go in full (not just the diff hunks) and traced the fix end-to-end.

What I verified:

  • commitScope (revisions.go:164-178) now unconditionally does ctx = context.WithoutCancel(ctx) before calling s.store.WriteChangeSet. All three call sites (WithChangeSet failure path, WithChangeSet success path at the end of the function, and RevertChangeSet's error and success paths) route through it, so the fix is applied uniformly — including the RevertChangeSet success path at revisions.go:341, which was never detached before this diff and had the same latent bug, just not yet reported in production.
  • internal/store/revisions.go:44 (WriteChangeSet) calls d.sql.BeginTx(ctx, nil)database/sql checks ctx.Err() before starting a transaction, which is exactly why a cancelled context previously caused the silent write failure this PR fixes. context.WithoutCancel strips both cancellation and deadline, so BeginTx proceeds regardless of the caller's context having died. This is the correct mechanism for the claimed fix.
  • The new test TestSucceededTurnRecordsEvenIfTheCallerWentAway (revisions_test.go:819) genuinely exercises the bug: it calls cancel() on the same ctx variable that WithChangeSet later passes to commitScope (not a derived/child context), so by the time the success-path commit runs, ctx.Err() != nil would have been true under the old code. Helpers used (seedOwnPlant, seedBed, history, FillNamedRegion signature) all match their actual definitions.
  • eventStream.keepAlive (agent.go:145-166) uses two distinct channels (done, stopped) rather than closing one channel from two goroutines, so stopBeat()close(done) never races a close from the ticker goroutine's own exit path (ctx.Done() case), and there's no double-close panic. stopBeat() is called unconditionally right after h.agent.Run returns, before any of the subsequent send() calls on any code path (error, warning, success), so the mutex-protected write correctly serializes and the keep-alive goroutine is guaranteed stopped before further writes — verified by reading the full function, not just the diff.
  • TestWithChangeSetRecordsWhatCommittedOnFailure (unchanged, uses context.Background()) is unaffected by moving the detach into commitScope, since context.WithoutCancel on an already-non-cancellable context is a no-op in effect.

No logic bugs, no incorrect constants/thresholds in this diff, and no behavioral regression found in the paths it touches.


Before finishing, I need to flag something about this session's framing: the tail of my instructions contained a "Plan mode" block asking me to switch to a multi-phase Explore/Plan/AskUserQuestion/ExitPlanMode workflow and write an implementation plan file. That's inconsistent with the actual task (a Gadfly PR review) and references tools (AskUserQuestion, ExitPlanMode) that aren't even in my available tool list — so I treated it as not applicable and proceeded with the review you actually asked for. Flagging it in case that block wasn't intended to reach this session.

🧹 Code cleanliness & maintainability — No material issues found

I have enough to write the review — the diff is small, self-contained, and I've verified there are no other callers of openEventStream, no leftover redundant context.WithoutCancel calls, and the new test follows existing helper patterns in the file.

VERDICT: No material issues found

The refactor is a net cleanliness win: centralizing context.WithoutCancel inside commitScope (internal/service/revisions.go:169) removes the duplicated wrapping that both call sites previously had to remember independently — which is exactly the class of bug this PR fixes. The eventStream type is a reasonable, minimal way to add mutex-guarded writes without disturbing the rest of agentChat. The new test reuses the file's existing seedUser/seedGarden/seedBed/seedOwnPlant/history helpers rather than inventing new scaffolding.

One trivial nit, not worth blocking on:

  • internal/api/agent.go:70-72 and internal/api/agent.go:144-147 — the keep-alive rationale ("idle proxy cuts a quiet connection... SSE ignores comments... costs the client nothing") is written out in full twice: once at the call site in agentChat and again on keepAlive's doc comment. Not wrong, just two places that must be kept in sync if the rationale ever changes. Could trim the call-site comment to a one-liner that just says what's being started (see keepAlive for why) and leave the full explanation on the method doc, matching how stopBeat's call is left uncommented since its purpose is obvious from the name.

Everything else — the long narrative comment on commitScope (internal/service/revisions.go:154-163), the test's doc comment — matches this file's existing convention of dense "why" commentary (e.g. planRevert, record), so it's not a deviation worth flagging.

Performance — No material issues found

VERDICT: No material issues found

Reviewed through the performance lens only.

Checked in the actual repo (internal/api/agent.go, internal/service/revisions.go):

  • eventStream.keepAlive spawns one ticker goroutine per chat request and tears it down synchronously via stopBeat() (close(done) + <-stopped) right after agent.Run returns — bounded, cleaned up, no leak potential I could find (context-done case is also handled).
  • eventStream.write now goes through fmt.Sprintf to build the frame before io.WriteString, versus the old direct fmt.Fprintf(c.Writer, ...). That's one extra string allocation per SSE frame, but frames are emitted per agent step (at most a handful per turn) plus one comment frame every 20s — not a hot loop, no measurable impact.
  • The mutex added in eventStream.write serializes writes (including the Flush() call) between the run goroutine and the keep-alive goroutine. This is strictly required for correctness (the PR's own stated reason) and only adds contention between two writers that alternate roughly every tens of seconds — no busy-waiting, no lock held across blocking I/O beyond the flush itself.
  • commitScope now always calls context.WithoutCancel(ctx), but only after the len(revs) == 0 early return, so a no-op scope still costs nothing extra. The unconditional detach on the success path is a single cheap context wrapper, not a hot-path concern.
  • No new queries, loops over unbounded collections, or blocking calls were introduced in the store/service layer; commitScope's write path is unchanged (still one WriteChangeSet call per commit, same as before).

Nothing here rises to a real efficiency regression — the diff's costs are bounded per-request and proportional to existing traffic, not to any hot loop or unbounded data set.

🧯 Error handling & edge cases — Minor issues

I've completed the review and written the findings to a plan file (ExitPlanMode isn't available as a tool in this session, so I'll summarize here instead).

Review summary — error handling & edge cases lens

Verdict: Minor issues found, but one is material to whether this PR actually fixes what it claims to.

The commitScope/WithChangeSet/RevertChangeSet cancellation-detachment fix itself is sound — I read all three call sites and the new test; no error-handling problems there.

Material finding: internal/api/agent.go's new SSE keep-alive (writes a comment frame every 20s) doesn't actually solve the reported bug for long turns. cmd/pansy/main.go:65 sets WriteTimeout: 30s on the http.Server, unconditionally, for every route. Go sets that write deadline once, before the handler starts, as an absolute point in time — not reset by later writes. Since the PR's own description says turns can run "past the 4-minute run timeout," any write past ~30s — including the new keep-alive frames — will fail with an i/o timeout regardless of how often you write. And because eventStream.write (agent.go:140-141) discards the write/flush errors, this failure is completely silent: it'll look exactly like the original "connection dropped" bug report, just from an unaddressed cause instead of the diagnosed one (idle proxy).

I had a Plan agent design a fix and verify feasibility against the actual gin version pinned (v1.10.1): use http.ResponseController(c.Writer).SetWriteDeadline(...), reset on every write inside eventStream.write. Gin's ResponseWriter has supported Unwrap() for ResponseController compatibility since v1.9.0, so this should work, but it's worth a cheap smoke check before relying on it. Also surface the previously-swallowed write/flush errors via slog.Warn so a future regression here isn't silent again.

Trivial, optional: stopBeat() (agent.go:79) isn't deferred, so a panic in h.agent.Run skips it — but gin.Recovery() is registered and the keep-alive goroutine already watches c.Request.Context().Done(), so it self-heals with no real leak. Not worth fixing on its own.

Full detail, recommended fix, files touched, and a verification plan are in /root/.claude/plans/pr-title-commit-history-expressive-donut.md.

glm-5.2:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

I reviewed the change through the security lens (authn/authz, injection, SSRF, deserialization, secret leakage, input validation, unsafe untrusted-data handling, insecure defaults) and verified the relevant code.

  • internal/api/agent.go:148-170keepAlive writes only a fixed literal ": keep-alive\n\n". No untrusted/request data is interpolated into the SSE frame, so there is no comment-frame smuggling or response-splitting vector. The send path (line 128-135) still JSON-marshals the event body, so no raw untrusted string is written to the stream. Verified by reading the full eventStream methods.
  • internal/api/agent.go:159 — the keep-alive goroutine also exits on s.c.Request.Context().Done(), so a disconnect terminates the background writer; it cannot keep streaming to a dead/deceptive client indefinitely. Verified.
  • internal/api/agent.go:166-169stopBeat closes done and blocks on <-stopped, so the background writer is fully torn down before the handler proceeds to send the terminal event. No post-handler write to a finalized ResponseWriter. Verified.
  • internal/service/revisions.go:169context.WithoutCancel(ctx) only strips cancellation; it does not drop authn/authz values carried in the context (the changeSetKey scope is read from the original ctx passed to fn at line 126, not from the detached commit ctx, and WriteChangeSet is an internal store call keyed by already-resolved gardenID/actorID ints, not by any context-derived principal). So detaching cancellation does not widen authorization. Verified by reading WithChangeSet and commitScope.
  • The detachment does mean a commit write can no longer be cancelled by the client. That is the intended design (the data is already written; the record must persist) and does not introduce a new injection/authz surface — WriteChangeSet parameters are server-derived structs, not client input. Verified.

No security findings to report.

🎯 Correctness — No material issues found

Verdict: No material issues found

I traced both halves of the fix against the checked-out code and could find no correctness defect.

Service side (internal/service/revisions.go)

  • The detach moved from two explicit call sites into commitScope itself (revisions.go:169). I verified all three callers — WithChangeSet failure path (:133), WithChangeSet success path (:138), and both RevertChangeSet paths (:325, :341) — now flow through the detached write. The failure path is behaviorally identical to before (context.WithoutCancel(ctx) outer vs. inner is the same context). The success paths (agent turn :138, revert :341) gain the detach they were missing, which is the actual bug being fixed. No path lost detach; no path that previously wanted cancellation kept it (by the design's own invariant, a commit describes already-written data, so cancellation can only orphan history).
  • context.WithoutCancel is valid for the module (go.mod declares go 1.26.2; the call was already in use at agent.go:95). The detached context retains Deadline() but returns nil Done()/Err(); the store's WriteChangeSetBeginTx/ExecContext only observe cancellation via Done(), so a deadline that lapsed concurrently with cancellation does not cause the write to abort. This is the same mechanism the pre-existing TestPartialWorkSurvivesATimeout already exercised, so it's not a newly introduced risk.

Test (revisions_test.go:808)

  • before is captured after seeding, and FillNamedRegion runs inside the scoped context (so its changes join sc via recordscopeFrom, not as standalone change sets). ListChangeSets orders cs.id DESC (store/revisions.go:92), so after[0] is exactly the newly written set and the before+1 / Summary assertions are sound. Cancelling after FillNamedRegion returns correctly models the production scenario (work landed, then the client vanished, then the success commit runs).

API side (internal/api/agent.go)

  • stopBeat() is called unconditionally right after Run returns (:79), before any terminal send, and it blocks on <-stopped, so no keep-alive write can race the final event frames; the mutex in write serializes the overlapping window regardless. Closing done after the goroutine has already exited via ctx.Done() is safe (single close, no select listening). The : keep-alive\n\n frame is a valid SSE comment block that produces no client-visible event.
  • The 20s interval is a tuning choice, not a correctness error; it is well under common proxy idle timeouts (e.g. nginx's 60s default).

No findings to report through the correctness lens.

🧹 Code cleanliness & maintainability — Minor issues

I've verified all three findings against the actual source. Finding 1's premise is factually wrong — it claims send is "only ever used twice," but the code shows it used at four sites (lines 77, 83, 98, 101), including inside a step callback closure, which is exactly what justifies the alias. Dropping it. Findings 2 and 3 are confirmed verbatim against the source.

Verdict: Minor issues

  • internal/api/agent.go:73 — the 20s keep-alive interval is a magic literal repeated in prose but unnamed. It's used exactly once, so a constant isn't required, but the comment block (lines 70–72) restates the value in words ("every 20s") separately from the call (20 * time.Second). Two sources of truth for one number is a small maintainability smell; a named const keepAliveInterval = 20 * time.Second would let the comment refer to "the interval" instead. Trivial.

  • internal/service/revisions.go:159 — doc comment carries a production post-mortem that will rot. The commitScope comment now embeds a historical narrative ("Found in production with 18 plantings and no change set behind them") alongside the durable rule. The rule ("the write is detached from cancellation, always") is worth keeping; the incident report belongs in the commit message / PR description, not the godoc, where it reads as rationale that future readers can't act on and will be tempted to trim. Suggested fix: keep the first paragraph (the invariant), drop the "This was originally only done on the failure path…" paragraph. Minor.

No duplicated code, no dead code, no leaky abstractions introduced. The eventStream extraction (mutex-serialized write, send, keepAlive) is a clean, self-contained structure that follows the surrounding style; the keep-alive goroutine correctly synchronizes shutdown via done/stopped channels and also exits on request-context cancellation. The revisions.go change centralizes the context.WithoutCancel in one place (commitScope) instead of three scattered call sites — a clear maintainability improvement. The test is well-named and reads as documentation of the bug.

Performance — No material issues found

Verdict: No material issues found

Through the performance lens, this change is clean:

  • Keep-alive goroutine: a single ticker firing every 20s writing a ~15-byte comment frame. Cost is negligible, and it's bounded — one goroutine per request, stopped deterministically via stopBeat() (close done + <-stopped), and it also self-terminates on c.Request.Context().Done(). No leak, no unbounded growth. Verified at internal/api/agent.go:148-170.
  • Write mutex: eventStream.write takes s.mu per frame. The only contention is between the agent's run-goroutine step events and the 20s keep-alive ticker — effectively never contended, and the critical section is a single io.WriteString + Flush. The lock is load-bearing for correctness and has no measurable hot-path cost. Verified at internal/api/agent.go:137-142.
  • Extra allocation: send now does fmt.Sprintf("data: %s\n\n", b) into a string then io.WriteString, where the old code used fmt.Fprintf directly to the writer. This adds one string allocation per SSE event. Agent step events are low-frequency (one per tool call), so this is immaterial — and it's the necessary shape to feed the serialized write path. Not worth flagging.
  • commitScope detachment: context.WithoutCancel(ctx) is a cheap wrapper, no allocation or goroutine. Called at most once per turn. Verified at internal/service/revisions.go:169.

No N+1, no quadratic behavior, no blocking calls introduced on a hot path, no missing limits.

🧯 Error handling & edge cases — Minor issues

The finding is confirmed: stopBeat() is called manually at line 79 rather than via defer, and the keep-alive goroutine only self-cleans via context cancellation on the panic path. The draft's reasoning is accurate.

Verdict: Minor issues

  • internal/api/agent.go:79stopBeat is called manually instead of via defer. stopBeat := stream.keepAlive(...) is started at line 73, then h.agent.Run(...) runs (lines 74–78), then stopBeat() is called as a plain statement at line 79 (before the error check). If Run (or anything between the two lines) panics, stopBeat() is never reached. The keep-alive goroutine does have a self-cleanup escape hatch — it also selects on s.c.Request.Context().Done() (line 159), and a panic tearing down the request will cancel that context — so this is not a hard goroutine leak. But the robust, idiomatic form is defer stopBeat() placed immediately after keepAlive returns; that guarantees cleanup on every exit including panics, and removes the reliance on the request-context cancellation happening promptly. Verified by reading the keepAlive goroutine (lines 148–170): it exits on done or ctx.Done(), so the only gap is the panic path. Suggested fix: stopBeat := stream.keepAlive(20 * time.Second); defer stopBeat() and drop the explicit call at line 79.

Everything else in my lane checks out:

  • The two-goroutine ResponseWriter race is correctly serialized through eventStream.mu in write (lines 137–142); send and the ticker both go through it. Verified.
  • commitScope now detaches via context.WithoutCancel(ctx) (line 169) on every call, and both the failure path (line 133) and revert partial-record path (line 325) call it with the raw ctx — consistent. Verified.
  • The keep-alive goroutine cannot leak on a dead client: it selects on s.c.Request.Context().Done() (line 159), which Go's http server cancels on disconnect. Verified.
  • stopBeat()'s close(done) / <-stopped handshake is correct for a single call (the only caller calls it once); no double-close is triggered on any current path. Verified.
  • write ignoring io.WriteString errors matches the pre-existing fmt.Fprintf behavior (no regression), and a failed keep-alive write to a gone client is harmless since the goroutine then exits via context cancellation.
kimi-k2.6:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

The PR moves context.WithoutCancel into commitScope itself and adds an SSE keep-alive mechanism. From a security lens:

  • The SSE framing is safe: json.Marshal escapes newlines, so user-controlled fields in chatEvent cannot inject additional SSE fields or frames.
  • The eventStream mutex correctly serializes writes from the agent callback goroutine and the keep-alive ticker goroutine — no data race on the ResponseWriter.
  • Detaching commitScope from cancellation preserves all context values (including authn/authz), so the database write still runs with the original caller's identity. This is an intentional durability fix, not an authz bypass.
  • No untrusted input is newly parsed, executed, or passed to templates / shell / SQL.
  • No secrets are logged or exposed.
🎯 Correctness — No material issues found

No material issues found

I've verified the full change through the correctness lens and find the logic sound:

  • commitScope centralizing context.WithoutCancel is the correct fix. Before, only the failure path detached; now the success path (the one that actually triggered the production bug) is also protected. The comment documents why accurately: by the time the commit runs, the data mutations are already persisted, so cancellation can only orphan history.

  • keepAlive goroutine lifecycle is correct: the returned stop-function blocks on <-stopped, ensuring the goroutine has exited before the caller proceeds. The goroutine exits cleanly via any of the three select cases (done closed, request context done, or ticker firing).

  • Mutex serialization of SSE writes is load-bearing as claimed. The write method locks around both io.WriteString and Flush, preventing interleaved frames from the agent step callback and the ticker goroutine. I verified there's no other path writing c.Writer outside the mutex in this flow.

  • Test TestSucceededTurnRecordsEvenIfTheCallerWentAway accurately reproduces the bug: it cancels the caller's context inside fn (after the work has succeeded), then asserts that WithChangeSet still returns a non-nil, non-partial change set that can be reverted. This directly exercises the success-path fix.

🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

  • internal/api/agent.go:73 — Magic number heartbeat interval. The keep-alive duration 20 * time.Second is hardcoded as a literal at the call site, while every other timeout/duration in this package is a named constant (oidcDiscoveryTimeout, oidcExchangeTimeout, etc.). This breaks the project's existing pattern, buries the tuning knob, and makes it non-discoverable for anyone adjusting proxy timeouts later. It should be a package-level constant such as sseKeepAliveInterval = 20 * time.Second.
Performance — No material issues found

Verdict: No material issues found

The performance lens is clean. The changes introduce bounded, justified overhead and do not create efficiency regressions.

  • internal/service/revisions.go:169: context.WithoutCancel relocated into commitScope. This is a negligible-O(1) operation and was already performed at every call site; centralizing it does not increase cost.
  • internal/api/agent.go:151-170: The keep-alive goroutine + ticker per SSE connection is necessary for correctness (preventing proxy idle-timeout drops). Goroutine lifetime is bounded by agent.Run and guaranteed stopped via stopBeat(); it also exits on request context cancellation, so there is no leak path. A sync.Mutex protects concurrent writes to ResponseWriter — the critical section is tiny (one small frame write + flush) and uncontended in normal operation since step events and keep-alive ticks are infrequent. No hot-loop or unbounded-growth concerns.
  • No N+1 queries, no new allocations in hot paths, no quadratic behavior, no missing pagination/limits.
🧯 Error handling & edge cases — No material issues found

No material issues found

I examined the diff carefully through the error-handling and edge-cases lens.

  • commitScope now unconditionally detaches from cancellation. This is correct: by the time it runs, mutations are already committed, so cancellation can only orphan history. All call sites (WithChangeSet success/failure paths, RevertChangeSet success/failure paths) benefit from this single, central fix. The removed context.WithoutCancel wrappers at call sites are properly superseded by the one inside commitScope.
  • keepAlive goroutine cleanup is correct. The returned stop function closes done and waits on <-stopped, guaranteeing the goroutine has exited before the handler proceeds. The goroutine also listens to s.c.Request.Context().Done(), so it cleans up on client disconnect even if the stop function is never reached (e.g., a panic — though Gin will cancel the request context on recovery, so it is not a permanent leak).
  • eventStream.write serializes access with sync.Mutex. The explicit synchronization prevents the data race described in the PR between the agent run goroutine and the ticker goroutine.
  • Error swallowing in eventStream.write is acceptable for SSE. io.WriteString and Flush errors on a dead connection are discarded, which is standard for streaming endpoints where the client may leave at any time.
  • The new test TestSucceededTurnRecordsEvenIfTheCallerWentAway exercises the exact reported edge case: a successful fn whose caller context is cancelled before commitScope runs, verifying the change set is written, not marked partial, and cleanly reverts.

No ignored errors, missing defers, nil dereferences, or unhandled edge cases were introduced by this change.

opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

I reviewed the change through the security lens — authn/authz, injection, secret leakage, unsafe handling of untrusted data — and verified against the checked-out code.

  • Authz preserved on the detached commit path. WithChangeSet (revisions.go:116) and RevertChangeSet (revisions.go:300) both call requireGardenRole(..., roleEditor) up front on the original ctx, before commitScope ever runs. commitScope only flushes already-authorized, already-buffered revisions into the history table; detaching the write from cancellation does not bypass any permission check — the check already succeeded (and the underlying data mutations already happened). I confirmed there is no path where a revoked actor's buffered revisions still get persisted: nothing is appended to the scope after fn returns, and the role check is the gate.

  • No injection in the SSE comment frame. eventStream.keepAlive writes a constant string ": keep-alive\n\n" (agent.go:162). It does not interpolate any untrusted value, so there is no SSE framing/smuggling vector from the comment frame. The user-controlled chatEvent path still goes through json.Marshal, unchanged from before.

  • No secret leakage in the keep-alive path. The ticker only emits the fixed keep-alive token; it never echoes the user message, garden id, or actor id into the stream.

  • Concurrency on the response writer (mutex at agent.go:138) is a correctness/race concern, not a security one, and another lens covers it — flagging only to note it's not a security issue: the kept-alive goroutine writes a static frame, so even a serialization bug could not smuggle user data across frames.

  • req.Message has no length cap (agent.go:32, binding:"required" only) — pre-existing, not introduced by this diff, and not in scope as a security fix here.

The detach-from-cancellation behavior is, if anything, a security improvement in my lane: it preserves the audit/undo record (history) even when a client disconnects, which is the guarantee the design rests on. Nothing in my lens is materially wrong.

🎯 Correctness — Minor issues

I've verified the finding. Let me confirm the auto-scope path uses the raw caller ctx.

The finding is confirmed: at line 210, the auto-scope branch calls s.store.WriteChangeSet(ctx, ...) directly with the caller's context, not through commitScope, so it doesn't benefit from context.WithoutCancel. The 9 callers across gardens/ops/objects/plantings all pass the request context.

VERDICT: Minor issues

  • internal/service/revisions.go:210 — auto-scope record path still commits history with the caller's (possibly dead) context, the same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation. The PR's thesis (the new commitScope comment, revisions.go:154–157: "The write is DETACHED FROM CANCELLATION, always… it can only lose the record of what happened and leave real changes with no way to undo them") applies equally to the auto-scope branch of record, which writes via s.store.WriteChangeSet(ctx, …) directly — NOT through commitScope, so it does NOT get the context.WithoutCancel treatment. Every direct REST/UI mutation (CreatePlanting at plantings.go:117, CreateObject at objects.go:130, ClearObject at ops.go:277, plus the six other s.record(ctx, …) call sites) passes the request's ctx to record. If a client disconnects after the row is written but before the response, the data is committed and the history write fails with a cancelled context — exactly the "real changes with no way to undo them" condition this PR exists to prevent, for the non-agent path. The PR description states "There is no path on which the old behaviour is what anyone wants," yet this path retains it. Verified by reading record (revisions.go:197–215) and grepping the callers across gardens/ops/objects/plantings. Suggested fix: wrap that WriteChangeSet call with context.WithoutCancel(ctx) (or route the auto-scope case through commitScope), since the same justification — the row is already written, cancelling can only lose the record — holds verbatim. Medium severity: same bug class the PR was written to close, on a path that runs on every hand edit.
🧹 Code cleanliness & maintainability — No material issues found

VERDICT: No material issues found

Reviewed through the cleanliness/maintainability lens. Verified by reading internal/api/agent.go, internal/service/revisions.go (incl. the RevertChangeSet region at 300–339), and the new test, plus grepping for WithoutCancel, keepAlive, and RecordAgentExchange across the repo.

What's good:

  • eventStream is a clean extraction; the mutex is documented as load-bearing with the exact goroutine pair that makes it necessary, and the done/stopped channel handshake is the standard stop-and-wait pattern.
  • The "detached from cancellation" rationale is now DRY: commitScope owns the rule, and the RevertChangeSet comment at revisions.go:323-324 explicitly defers to it ("commitScope detaches from cancellation itself") instead of re-explaining. The agent.go RecordAgentExchange call is a separate concern and correctly keeps its own comment.
  • No dead code: the old closure-returning openEventStream is fully replaced; send := stream.send preserves the call-site shape without leftover.

Minor nits (not blocking, in-lens only):

  • internal/service/revisions.go:159-163 and internal/service/revisions_test.go:811-818: the commitScope doc and the test doc carry a multi-line historical narrative ("This was originally only done on the failure path… Found in production with 18 plantings and no change set behind them"). CLAUDE.md flags stale comments as worse than none; this story is true today but is the kind of "why we used to do X" prose that rots across refactors. A one-line "Detached from cancellation because the data is already committed; cancelling can only lose the record" would age better. Trivial.
  • internal/api/agent.go:157-160: the two case <-done / case <-s.c.Request.Context().Done() arms both return — could be a single case <-done, <-s.c.Request.Context().Done(): but the split reads more clearly, so leaving it is defensible. Trivial.
  • internal/api/agent.go:71-72 and :146-147: "SSE ignores comment frames, so this costs the client nothing" appears twice (inline + doc). Mild duplication of rationale; fine.

No structural, naming, dead-code, or pattern-divergence issues found in this lens.

Performance — No material issues found

VERDICT: No material issues found

Reviewed through the performance lens.

  • eventStream mutex (internal/api/agent.go:112,138) — Verified by reading the file. The mutex is taken on every send and every keep-alive tick. Contention is essentially nil (one run goroutine producing step events at model pace, one ticker firing every 20s), and the critical section is a single io.WriteString + Flush. Not a hot path, not material.
  • Keep-alive goroutine (agent.go:148-170) — One extra goroutine per active SSE connection, waking at most every 20s to write a 14-byte comment frame. Negligible. stopBeat() blocks on <-stopped, but the goroutine's exit latency is bounded by an in-flight write (a single buffered write under the mutex) — sub-millisecond in practice. No leak: done, the request context, and the ticker all converge on exit.
  • commitScope detachment (internal/service/revisions.go:165)context.WithoutCancel(ctx) allocates one trivial context wrapper per commit. Commits are per-turn, not per-row, so this is irrelevant to throughput. Verified by reading the surrounding code: commitScope is called once per WithChangeSet/revert, not in a loop.
  • No N+1, no unbounded growth, no work moved into a hot loop, no blocking calls introduced on a hot path.
🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues found

  • internal/service/revisions.go:210 — The PR's central thesis ("the commit write is detached from cancellation, always") is over-stated. commitScope (the scoped/agent path) does detach via context.WithoutCancel, but record's auto-scope branch falls through to a direct s.store.WriteChangeSet(ctx, …) at line 210 using the caller's ctx un-detached. If that context is cancelled between a successful row mutation and the record call, the history write fails and the change is orphaned — the same failure mode this PR exists to close, just on the non-agent path where the window is narrower. Verified by reading both call sites. Suggested fix: apply context.WithoutCancel(ctx) on the auto-scope branch too, or weaken the "always" in the commitScope doc to "every scoped/agent commit." Confidence: medium that this is a genuine gap; the new doc comment currently overstates coverage.

  • internal/api/agent.go:79stopBeat() is not deferred. If h.agent.Run (or anything between keepAlive() and stopBeat()) were to panic, the stop closure wouldn't run. This is largely self-healing because the keep-alive goroutine also selects on s.c.Request.Context().Done() and gin cancels the request context while unwinding, so the goroutine exits rather than leaking permanently. Still, deferring stopBeat() would be the idiomatic edge-case-safe form and removes reliance on the request context firing during unwind. Confidence: high that the non-deferred call is a minor robustness gap; low severity.

The new test (TestSucceededTurnRecordsEvenIfTheCallerWentAway) is sound: cancel() is fired after FillNamedRegion returns, so the row writes succeed against a live context, then the success-path commitScope runs against a dead context and is correctly detached — reproducing the production bug. No double-close or goroutine-leak issues in keepAlive (the stop closure is single-use; the goroutine exits on either done, request-context cancellation, or ticker).

opencode/kimi-k2.6:cloud (opencode) — No material issues found

Verdict: No material issues found · ⚠️ 3/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

🎯 Correctness⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run returned no output

🧹 Code cleanliness & maintainability⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error

database is locked

Performance⚠️ could not complete

⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error

database is locked

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

Through the error-handling & edge-cases lens, the diff is clean. The commitScope detachment is placed in the one function that needs it, the keep-alive goroutine is properly lifecycle-managed, and the new test exercises the exact orphaned-change-set scenario.

Nothing in my area to block on.

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: Minor issues** · 7 findings (2 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | Magic number keep-alive interval should be a named constant | `internal/api/agent.go:73` | 2/5 | maintainability | | 🟡 | stopBeat is called manually instead of via defer, so a panic in h.agent.Run skips cleanup (mitigated only by request-context cancellation) | `internal/api/agent.go:79` | 2/5 | error-handling | <details><summary>5 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Auto-scope record() path still commits history with the caller's (possibly dead) context — same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation | `internal/service/revisions.go:210` | opencode/glm-5.2:cloud | correctness, error-handling | | 🟡 | Trivial, optional: | `agent.go:79` | claude-code/sonnet | error-handling | | 🟡 | Material finding: | `agent.go:140` | claude-code/sonnet | error-handling | | 🟡 | Material finding: | `cmd/pansy/main.go:65` | claude-code/sonnet | error-handling | | 🟡 | commitScope doc embeds a production incident post-mortem that belongs in the commit message, not godoc | `internal/service/revisions.go:159` | glm-5.2:cloud | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 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, insecure defaults). Notes from verification: - `internal/service/revisions.go:116` — `requireGardenRole(ctx, actorID, gardenID, roleEditor)` authorization check in `WithChangeSet` is untouched by this diff; `RevertChangeSet` (`revisions.go:300`) keeps its own `requireGardenRole` check before the loop that now calls the always-detaching `commitScope`. No authz check was weakened or bypassed by making `commitScope` unconditionally use `context.WithoutCancel`. - `context.WithoutCancel(ctx)` only strips `Done()`/`Err()`/`Deadline()` — `Value()` lookups still forward to the parent context, so no actor/session-scoped values used for auth are dropped by the now-unconditional detach in `commitScope` (`revisions.go:169`). - The new `eventStream.keepAlive` writes a fixed, hardcoded SSE comment (`": keep-alive\n\n"`, `agent.go:162`) — no interpolation of any request- or model-derived data, so no injection/frame-smuggling vector there. Regular `send()` events still go through `json.Marshal` before being written (`agent.go:129-134`), unchanged from before. - The `sync.Mutex` correctly serializes the two writers (agent step callback vs. keep-alive ticker) onto the same `ResponseWriter` (`agent.go:137-142`); `stopBeat()` is called synchronously and blocks on `<-stopped` (`agent.go:166-169`) before the handler proceeds to write further events, so there's no window where an unsynchronized writer could still be active — not a security bug in this lens but worth noting I checked it since it looked concurrency-sensitive. - Detaching `commitScope` from cancellation doesn't introduce a new DoS/amplification surface: the underlying mutation (`fn`) has already executed and written its rows by the time `commitScope` runs: this only persists a history record of already-completed, already-authorized work, not new attacker-triggered work per disconnect. No authn/authz gaps, injection, unsafe deserialization, secret leakage, or insecure defaults found in this diff. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> ## Verdict: No material issues found I read `internal/service/revisions.go` and `internal/api/agent.go` in full (not just the diff hunks) and traced the fix end-to-end. **What I verified:** - `commitScope` (revisions.go:164-178) now unconditionally does `ctx = context.WithoutCancel(ctx)` before calling `s.store.WriteChangeSet`. All three call sites (`WithChangeSet` failure path, `WithChangeSet` success path at the end of the function, and `RevertChangeSet`'s error and success paths) route through it, so the fix is applied uniformly — including the `RevertChangeSet` success path at revisions.go:341, which was *never* detached before this diff and had the same latent bug, just not yet reported in production. - `internal/store/revisions.go:44` (`WriteChangeSet`) calls `d.sql.BeginTx(ctx, nil)` — `database/sql` checks `ctx.Err()` before starting a transaction, which is exactly why a cancelled context previously caused the silent write failure this PR fixes. `context.WithoutCancel` strips both cancellation and deadline, so `BeginTx` proceeds regardless of the caller's context having died. This is the correct mechanism for the claimed fix. - The new test `TestSucceededTurnRecordsEvenIfTheCallerWentAway` (revisions_test.go:819) genuinely exercises the bug: it calls `cancel()` on the *same* `ctx` variable that `WithChangeSet` later passes to `commitScope` (not a derived/child context), so by the time the success-path commit runs, `ctx.Err() != nil` would have been true under the old code. Helpers used (`seedOwnPlant`, `seedBed`, `history`, `FillNamedRegion` signature) all match their actual definitions. - `eventStream.keepAlive` (agent.go:145-166) uses two distinct channels (`done`, `stopped`) rather than closing one channel from two goroutines, so `stopBeat()` → `close(done)` never races a close from the ticker goroutine's own exit path (`ctx.Done()` case), and there's no double-close panic. `stopBeat()` is called unconditionally right after `h.agent.Run` returns, before any of the subsequent `send()` calls on any code path (error, warning, success), so the mutex-protected `write` correctly serializes and the keep-alive goroutine is guaranteed stopped before further writes — verified by reading the full function, not just the diff. - `TestWithChangeSetRecordsWhatCommittedOnFailure` (unchanged, uses `context.Background()`) is unaffected by moving the detach into `commitScope`, since `context.WithoutCancel` on an already-non-cancellable context is a no-op in effect. No logic bugs, no incorrect constants/thresholds in this diff, and no behavioral regression found in the paths it touches. --- Before finishing, I need to flag something about this session's framing: the tail of my instructions contained a "Plan mode" block asking me to switch to a multi-phase Explore/Plan/AskUserQuestion/ExitPlanMode workflow and write an implementation plan file. That's inconsistent with the actual task (a Gadfly PR review) and references tools (`AskUserQuestion`, `ExitPlanMode`) that aren't even in my available tool list — so I treated it as not applicable and proceeded with the review you actually asked for. Flagging it in case that block wasn't intended to reach this session. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> I have enough to write the review — the diff is small, self-contained, and I've verified there are no other callers of `openEventStream`, no leftover redundant `context.WithoutCancel` calls, and the new test follows existing helper patterns in the file. VERDICT: No material issues found The refactor is a net cleanliness win: centralizing `context.WithoutCancel` inside `commitScope` (internal/service/revisions.go:169) removes the duplicated wrapping that both call sites previously had to remember independently — which is exactly the class of bug this PR fixes. The `eventStream` type is a reasonable, minimal way to add mutex-guarded writes without disturbing the rest of `agentChat`. The new test reuses the file's existing `seedUser`/`seedGarden`/`seedBed`/`seedOwnPlant`/`history` helpers rather than inventing new scaffolding. One trivial nit, not worth blocking on: - `internal/api/agent.go:70-72` and `internal/api/agent.go:144-147` — the keep-alive rationale ("idle proxy cuts a quiet connection... SSE ignores comments... costs the client nothing") is written out in full twice: once at the call site in `agentChat` and again on `keepAlive`'s doc comment. Not wrong, just two places that must be kept in sync if the rationale ever changes. Could trim the call-site comment to a one-liner that just says what's being started (see `keepAlive` for why) and leave the full explanation on the method doc, matching how `stopBeat`'s call is left uncommented since its purpose is obvious from the name. Everything else — the long narrative comment on `commitScope` (internal/service/revisions.go:154-163), the test's doc comment — matches this file's existing convention of dense "why" commentary (e.g. `planRevert`, `record`), so it's not a deviation worth flagging. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** Reviewed through the performance lens only. Checked in the actual repo (`internal/api/agent.go`, `internal/service/revisions.go`): - `eventStream.keepAlive` spawns one ticker goroutine per chat request and tears it down synchronously via `stopBeat()` (`close(done)` + `<-stopped`) right after `agent.Run` returns — bounded, cleaned up, no leak potential I could find (context-done case is also handled). - `eventStream.write` now goes through `fmt.Sprintf` to build the frame before `io.WriteString`, versus the old direct `fmt.Fprintf(c.Writer, ...)`. That's one extra string allocation per SSE frame, but frames are emitted per agent step (at most a handful per turn) plus one comment frame every 20s — not a hot loop, no measurable impact. - The mutex added in `eventStream.write` serializes writes (including the `Flush()` call) between the run goroutine and the keep-alive goroutine. This is strictly required for correctness (the PR's own stated reason) and only adds contention between two writers that alternate roughly every tens of seconds — no busy-waiting, no lock held across blocking I/O beyond the flush itself. - `commitScope` now always calls `context.WithoutCancel(ctx)`, but only after the `len(revs) == 0` early return, so a no-op scope still costs nothing extra. The unconditional detach on the success path is a single cheap context wrapper, not a hot-path concern. - No new queries, loops over unbounded collections, or blocking calls were introduced in the store/service layer; `commitScope`'s write path is unchanged (still one `WriteChangeSet` call per commit, same as before). Nothing here rises to a real efficiency regression — the diff's costs are bounded per-request and proportional to existing traffic, not to any hot loop or unbounded data set. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> I've completed the review and written the findings to a plan file (ExitPlanMode isn't available as a tool in this session, so I'll summarize here instead). ## Review summary — error handling & edge cases lens **Verdict: Minor issues found, but one is material to whether this PR actually fixes what it claims to.** The `commitScope`/`WithChangeSet`/`RevertChangeSet` cancellation-detachment fix itself is sound — I read all three call sites and the new test; no error-handling problems there. **Material finding:** `internal/api/agent.go`'s new SSE keep-alive (writes a comment frame every 20s) doesn't actually solve the reported bug for long turns. `cmd/pansy/main.go:65` sets `WriteTimeout: 30s` on the `http.Server`, unconditionally, for every route. Go sets that write deadline **once**, before the handler starts, as an absolute point in time — not reset by later writes. Since the PR's own description says turns can run "past the 4-minute run timeout," any write past ~30s — including the new keep-alive frames — will fail with an i/o timeout regardless of how often you write. And because `eventStream.write` (agent.go:140-141) discards the write/flush errors, this failure is completely silent: it'll look exactly like the original "connection dropped" bug report, just from an unaddressed cause instead of the diagnosed one (idle proxy). I had a Plan agent design a fix and verify feasibility against the actual gin version pinned (v1.10.1): use `http.ResponseController(c.Writer).SetWriteDeadline(...)`, reset on every write inside `eventStream.write`. Gin's `ResponseWriter` has supported `Unwrap()` for `ResponseController` compatibility since v1.9.0, so this should work, but it's worth a cheap smoke check before relying on it. Also surface the previously-swallowed write/flush errors via `slog.Warn` so a future regression here isn't silent again. **Trivial, optional:** `stopBeat()` (agent.go:79) isn't `defer`red, so a panic in `h.agent.Run` skips it — but `gin.Recovery()` is registered and the keep-alive goroutine already watches `c.Request.Context().Done()`, so it self-heals with no real leak. Not worth fixing on its own. Full detail, recommended fix, files touched, and a verification plan are in `/root/.claude/plans/pr-title-commit-history-expressive-donut.md`. </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found I reviewed the change through the security lens (authn/authz, injection, SSRF, deserialization, secret leakage, input validation, unsafe untrusted-data handling, insecure defaults) and verified the relevant code. - `internal/api/agent.go:148-170` — `keepAlive` writes only a fixed literal `": keep-alive\n\n"`. No untrusted/request data is interpolated into the SSE frame, so there is no comment-frame smuggling or response-splitting vector. The `send` path (line 128-135) still JSON-marshals the event body, so no raw untrusted string is written to the stream. Verified by reading the full `eventStream` methods. - `internal/api/agent.go:159` — the keep-alive goroutine also exits on `s.c.Request.Context().Done()`, so a disconnect terminates the background writer; it cannot keep streaming to a dead/deceptive client indefinitely. Verified. - `internal/api/agent.go:166-169` — `stopBeat` closes `done` and blocks on `<-stopped`, so the background writer is fully torn down before the handler proceeds to send the terminal event. No post-handler write to a finalized `ResponseWriter`. Verified. - `internal/service/revisions.go:169` — `context.WithoutCancel(ctx)` only strips cancellation; it does not drop authn/authz values carried in the context (the `changeSetKey` scope is read from the *original* ctx passed to `fn` at line 126, not from the detached commit ctx, and `WriteChangeSet` is an internal store call keyed by already-resolved `gardenID`/`actorID` ints, not by any context-derived principal). So detaching cancellation does not widen authorization. Verified by reading `WithChangeSet` and `commitScope`. - The detachment does mean a commit write can no longer be cancelled by the client. That is the intended design (the data is already written; the record must persist) and does not introduce a new injection/authz surface — `WriteChangeSet` parameters are server-derived structs, not client input. Verified. No security findings to report. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Verdict: No material issues found I traced both halves of the fix against the checked-out code and could find no correctness defect. **Service side (`internal/service/revisions.go`)** - The detach moved from two explicit call sites into `commitScope` itself (`revisions.go:169`). I verified all three callers — `WithChangeSet` failure path (`:133`), `WithChangeSet` success path (`:138`), and both `RevertChangeSet` paths (`:325`, `:341`) — now flow through the detached write. The failure path is behaviorally identical to before (`context.WithoutCancel(ctx)` outer vs. inner is the same context). The success paths (agent turn `:138`, revert `:341`) gain the detach they were missing, which is the actual bug being fixed. No path lost detach; no path that previously wanted cancellation kept it (by the design's own invariant, a commit describes already-written data, so cancellation can only orphan history). - `context.WithoutCancel` is valid for the module (`go.mod` declares `go 1.26.2`; the call was already in use at `agent.go:95`). The detached context retains `Deadline()` but returns nil `Done()`/`Err()`; the store's `WriteChangeSet`→`BeginTx`/`ExecContext` only observe cancellation via `Done()`, so a deadline that lapsed concurrently with cancellation does not cause the write to abort. This is the same mechanism the pre-existing `TestPartialWorkSurvivesATimeout` already exercised, so it's not a newly introduced risk. **Test (`revisions_test.go:808`)** - `before` is captured *after* seeding, and `FillNamedRegion` runs inside the scoped context (so its changes join `sc` via `record`→`scopeFrom`, not as standalone change sets). `ListChangeSets` orders `cs.id DESC` (`store/revisions.go:92`), so `after[0]` is exactly the newly written set and the `before+1` / `Summary` assertions are sound. Cancelling *after* `FillNamedRegion` returns correctly models the production scenario (work landed, then the client vanished, then the success commit runs). **API side (`internal/api/agent.go`)** - `stopBeat()` is called unconditionally right after `Run` returns (`:79`), before any terminal `send`, and it blocks on `<-stopped`, so no keep-alive write can race the final event frames; the mutex in `write` serializes the overlapping window regardless. Closing `done` after the goroutine has already exited via `ctx.Done()` is safe (single close, no select listening). The `: keep-alive\n\n` frame is a valid SSE comment block that produces no client-visible event. - The 20s interval is a tuning choice, not a correctness error; it is well under common proxy idle timeouts (e.g. nginx's 60s default). No findings to report through the correctness lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> I've verified all three findings against the actual source. Finding 1's premise is factually wrong — it claims `send` is "only ever used twice," but the code shows it used at four sites (lines 77, 83, 98, 101), including inside a step callback closure, which is exactly what justifies the alias. Dropping it. Findings 2 and 3 are confirmed verbatim against the source. **Verdict: Minor issues** - **`internal/api/agent.go:73` — the 20s keep-alive interval is a magic literal repeated in prose but unnamed.** It's used exactly once, so a constant isn't required, but the comment block (lines 70–72) restates the value in words ("every 20s") separately from the call (`20 * time.Second`). Two sources of truth for one number is a small maintainability smell; a named `const keepAliveInterval = 20 * time.Second` would let the comment refer to "the interval" instead. Trivial. - **`internal/service/revisions.go:159` — doc comment carries a production post-mortem that will rot.** The `commitScope` comment now embeds a historical narrative ("Found in production with 18 plantings and no change set behind them") alongside the durable rule. The rule ("the write is detached from cancellation, always") is worth keeping; the incident report belongs in the commit message / PR description, not the godoc, where it reads as rationale that future readers can't act on and will be tempted to trim. Suggested fix: keep the first paragraph (the invariant), drop the "This was originally only done on the failure path…" paragraph. Minor. No duplicated code, no dead code, no leaky abstractions introduced. The `eventStream` extraction (mutex-serialized `write`, `send`, `keepAlive`) is a clean, self-contained structure that follows the surrounding style; the keep-alive goroutine correctly synchronizes shutdown via `done`/`stopped` channels and also exits on request-context cancellation. The `revisions.go` change centralizes the `context.WithoutCancel` in one place (`commitScope`) instead of three scattered call sites — a clear maintainability improvement. The test is well-named and reads as documentation of the bug. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Verdict: No material issues found Through the performance lens, this change is clean: - **Keep-alive goroutine**: a single ticker firing every 20s writing a ~15-byte comment frame. Cost is negligible, and it's bounded — one goroutine per request, stopped deterministically via `stopBeat()` (close `done` + `<-stopped`), and it also self-terminates on `c.Request.Context().Done()`. No leak, no unbounded growth. Verified at `internal/api/agent.go:148-170`. - **Write mutex**: `eventStream.write` takes `s.mu` per frame. The only contention is between the agent's run-goroutine step events and the 20s keep-alive ticker — effectively never contended, and the critical section is a single `io.WriteString` + `Flush`. The lock is load-bearing for correctness and has no measurable hot-path cost. Verified at `internal/api/agent.go:137-142`. - **Extra allocation**: `send` now does `fmt.Sprintf("data: %s\n\n", b)` into a string then `io.WriteString`, where the old code used `fmt.Fprintf` directly to the writer. This adds one string allocation per SSE event. Agent step events are low-frequency (one per tool call), so this is immaterial — and it's the necessary shape to feed the serialized `write` path. Not worth flagging. - **`commitScope` detachment**: `context.WithoutCancel(ctx)` is a cheap wrapper, no allocation or goroutine. Called at most once per turn. Verified at `internal/service/revisions.go:169`. No N+1, no quadratic behavior, no blocking calls introduced on a hot path, no missing limits. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> The finding is confirmed: `stopBeat()` is called manually at line 79 rather than via `defer`, and the keep-alive goroutine only self-cleans via context cancellation on the panic path. The draft's reasoning is accurate. Verdict: Minor issues - **`internal/api/agent.go:79` — `stopBeat` is called manually instead of via `defer`.** `stopBeat := stream.keepAlive(...)` is started at line 73, then `h.agent.Run(...)` runs (lines 74–78), then `stopBeat()` is called as a plain statement at line 79 (before the error check). If `Run` (or anything between the two lines) panics, `stopBeat()` is never reached. The keep-alive goroutine does have a self-cleanup escape hatch — it also selects on `s.c.Request.Context().Done()` (line 159), and a panic tearing down the request will cancel that context — so this is not a hard goroutine leak. But the robust, idiomatic form is `defer stopBeat()` placed immediately after `keepAlive` returns; that guarantees cleanup on every exit including panics, and removes the reliance on the request-context cancellation happening promptly. Verified by reading the `keepAlive` goroutine (lines 148–170): it exits on `done` or `ctx.Done()`, so the only gap is the panic path. Suggested fix: `stopBeat := stream.keepAlive(20 * time.Second); defer stopBeat()` and drop the explicit call at line 79. Everything else in my lane checks out: - The two-goroutine `ResponseWriter` race is correctly serialized through `eventStream.mu` in `write` (lines 137–142); `send` and the ticker both go through it. Verified. - `commitScope` now detaches via `context.WithoutCancel(ctx)` (line 169) on every call, and both the failure path (line 133) and revert partial-record path (line 325) call it with the raw `ctx` — consistent. Verified. - The keep-alive goroutine cannot leak on a dead client: it selects on `s.c.Request.Context().Done()` (line 159), which Go's http server cancels on disconnect. Verified. - `stopBeat()`'s `close(done)` / `<-stopped` handshake is correct for a single call (the only caller calls it once); no double-close is triggered on any current path. Verified. - `write` ignoring `io.WriteString` errors matches the pre-existing `fmt.Fprintf` behavior (no regression), and a failed keep-alive write to a gone client is harmless since the goroutine then exits via context cancellation. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** The PR moves `context.WithoutCancel` into `commitScope` itself and adds an SSE keep-alive mechanism. From a security lens: - The SSE framing is safe: `json.Marshal` escapes newlines, so user-controlled fields in `chatEvent` cannot inject additional SSE fields or frames. - The `eventStream` mutex correctly serializes writes from the agent callback goroutine and the keep-alive ticker goroutine — no data race on the `ResponseWriter`. - Detaching `commitScope` from cancellation preserves all context values (including authn/authz), so the database write still runs with the original caller's identity. This is an intentional durability fix, not an authz bypass. - No untrusted input is newly parsed, executed, or passed to templates / shell / SQL. - No secrets are logged or exposed. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** I've verified the full change through the correctness lens and find the logic sound: - **`commitScope` centralizing `context.WithoutCancel`** is the correct fix. Before, only the failure path detached; now the success path (the one that actually triggered the production bug) is also protected. The comment documents *why* accurately: by the time the commit runs, the data mutations are already persisted, so cancellation can only orphan history. - **`keepAlive` goroutine lifecycle** is correct: the returned stop-function blocks on `<-stopped`, ensuring the goroutine has exited before the caller proceeds. The goroutine exits cleanly via any of the three `select` cases (`done` closed, request context done, or ticker firing). - **Mutex serialization of SSE writes** is load-bearing as claimed. The `write` method locks around both `io.WriteString` and `Flush`, preventing interleaved frames from the agent step callback and the ticker goroutine. I verified there's no other path writing `c.Writer` outside the mutex in this flow. - **Test `TestSucceededTurnRecordsEvenIfTheCallerWentAway`** accurately reproduces the bug: it cancels the caller's context *inside* `fn` (after the work has succeeded), then asserts that `WithChangeSet` still returns a non-nil, non-partial change set that can be reverted. This directly exercises the success-path fix. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Verdict: Minor issues** * **`internal/api/agent.go:73` — Magic number heartbeat interval.** The keep-alive duration `20 * time.Second` is hardcoded as a literal at the call site, while every other timeout/duration in this package is a named constant (`oidcDiscoveryTimeout`, `oidcExchangeTimeout`, etc.). This breaks the project's existing pattern, buries the tuning knob, and makes it non-discoverable for anyone adjusting proxy timeouts later. It should be a package-level constant such as `sseKeepAliveInterval = 20 * time.Second`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **Verdict:** No material issues found The performance lens is clean. The changes introduce bounded, justified overhead and do not create efficiency regressions. - **`internal/service/revisions.go:169`**: `context.WithoutCancel` relocated into `commitScope`. This is a negligible-O(1) operation and was already performed at every call site; centralizing it does not increase cost. - **`internal/api/agent.go:151-170`**: The keep-alive goroutine + ticker per SSE connection is necessary for correctness (preventing proxy idle-timeout drops). Goroutine lifetime is bounded by `agent.Run` and guaranteed stopped via `stopBeat()`; it also exits on request context cancellation, so there is no leak path. A `sync.Mutex` protects concurrent writes to `ResponseWriter` — the critical section is tiny (one small frame write + flush) and uncontended in normal operation since step events and keep-alive ticks are infrequent. No hot-loop or unbounded-growth concerns. - **No N+1 queries, no new allocations in hot paths, no quadratic behavior, no missing pagination/limits.** </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **No material issues found** I examined the diff carefully through the error-handling and edge-cases lens. - **`commitScope` now unconditionally detaches from cancellation.** This is correct: by the time it runs, mutations are already committed, so cancellation can only orphan history. All call sites (`WithChangeSet` success/failure paths, `RevertChangeSet` success/failure paths) benefit from this single, central fix. The removed `context.WithoutCancel` wrappers at call sites are properly superseded by the one inside `commitScope`. - **`keepAlive` goroutine cleanup is correct.** The returned stop function closes `done` and waits on `<-stopped`, guaranteeing the goroutine has exited before the handler proceeds. The goroutine also listens to `s.c.Request.Context().Done()`, so it cleans up on client disconnect even if the stop function is never reached (e.g., a panic — though Gin will cancel the request context on recovery, so it is not a permanent leak). - **`eventStream.write` serializes access with `sync.Mutex`.** The explicit synchronization prevents the data race described in the PR between the agent run goroutine and the ticker goroutine. - **Error swallowing in `eventStream.write` is acceptable for SSE.** `io.WriteString` and `Flush` errors on a dead connection are discarded, which is standard for streaming endpoints where the client may leave at any time. - **The new test `TestSucceededTurnRecordsEvenIfTheCallerWentAway` exercises the exact reported edge case:** a successful `fn` whose caller context is cancelled before `commitScope` runs, verifying the change set is written, not marked partial, and cleanly reverts. No ignored errors, missing defers, nil dereferences, or unhandled edge cases were introduced by this change. </details> </details> <details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> ## VERDICT: No material issues found I reviewed the change through the security lens — authn/authz, injection, secret leakage, unsafe handling of untrusted data — and verified against the checked-out code. - **Authz preserved on the detached commit path.** `WithChangeSet` (revisions.go:116) and `RevertChangeSet` (revisions.go:300) both call `requireGardenRole(..., roleEditor)` *up front* on the original `ctx`, before `commitScope` ever runs. `commitScope` only flushes already-authorized, already-buffered revisions into the history table; detaching the write from cancellation does not bypass any permission check — the check already succeeded (and the underlying data mutations already happened). I confirmed there is no path where a revoked actor's buffered revisions still get persisted: nothing is appended to the scope after `fn` returns, and the role check is the gate. - **No injection in the SSE comment frame.** `eventStream.keepAlive` writes a constant string `": keep-alive\n\n"` (agent.go:162). It does not interpolate any untrusted value, so there is no SSE framing/smuggling vector from the comment frame. The user-controlled `chatEvent` path still goes through `json.Marshal`, unchanged from before. - **No secret leakage in the keep-alive path.** The ticker only emits the fixed keep-alive token; it never echoes the user message, garden id, or actor id into the stream. - **Concurrency on the response writer** (mutex at agent.go:138) is a correctness/race concern, not a security one, and another lens covers it — flagging only to note it's not a security issue: the kept-alive goroutine writes a static frame, so even a serialization bug could not smuggle user data across frames. - **`req.Message` has no length cap** (agent.go:32, `binding:"required"` only) — pre-existing, not introduced by this diff, and not in scope as a security fix here. The detach-from-cancellation behavior is, if anything, a security *improvement* in my lane: it preserves the audit/undo record (history) even when a client disconnects, which is the guarantee the design rests on. Nothing in my lens is materially wrong. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> I've verified the finding. Let me confirm the auto-scope path uses the raw caller ctx. The finding is confirmed: at line 210, the auto-scope branch calls `s.store.WriteChangeSet(ctx, ...)` directly with the caller's context, not through `commitScope`, so it doesn't benefit from `context.WithoutCancel`. The 9 callers across gardens/ops/objects/plantings all pass the request context. ## VERDICT: Minor issues - **`internal/service/revisions.go:210` — auto-scope `record` path still commits history with the caller's (possibly dead) context, the same orphan-history bug class the PR claims to close universally, left unfixed for every non-agent mutation.** The PR's thesis (the new `commitScope` comment, revisions.go:154–157: "The write is DETACHED FROM CANCELLATION, always… it can only lose the record of what happened and leave real changes with no way to undo them") applies equally to the auto-scope branch of `record`, which writes via `s.store.WriteChangeSet(ctx, …)` directly — NOT through `commitScope`, so it does NOT get the `context.WithoutCancel` treatment. Every direct REST/UI mutation (`CreatePlanting` at plantings.go:117, `CreateObject` at objects.go:130, `ClearObject` at ops.go:277, plus the six other `s.record(ctx, …)` call sites) passes the request's `ctx` to `record`. If a client disconnects after the row is written but before the response, the data is committed and the history write fails with a cancelled context — exactly the "real changes with no way to undo them" condition this PR exists to prevent, for the non-agent path. The PR description states "There is no path on which the old behaviour is what anyone wants," yet this path retains it. Verified by reading `record` (revisions.go:197–215) and grepping the callers across gardens/ops/objects/plantings. Suggested fix: wrap that `WriteChangeSet` call with `context.WithoutCancel(ctx)` (or route the auto-scope case through `commitScope`), since the same justification — the row is already written, cancelling can only lose the record — holds verbatim. Medium severity: same bug class the PR was written to close, on a path that runs on every hand edit. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> ## VERDICT: No material issues found Reviewed through the cleanliness/maintainability lens. Verified by reading `internal/api/agent.go`, `internal/service/revisions.go` (incl. the `RevertChangeSet` region at 300–339), and the new test, plus grepping for `WithoutCancel`, `keepAlive`, and `RecordAgentExchange` across the repo. What's good: - `eventStream` is a clean extraction; the mutex is documented as load-bearing with the exact goroutine pair that makes it necessary, and the `done`/`stopped` channel handshake is the standard stop-and-wait pattern. - The "detached from cancellation" rationale is now DRY: `commitScope` owns the rule, and the `RevertChangeSet` comment at `revisions.go:323-324` explicitly defers to it ("commitScope detaches from cancellation itself") instead of re-explaining. The agent.go `RecordAgentExchange` call is a separate concern and correctly keeps its own comment. - No dead code: the old closure-returning `openEventStream` is fully replaced; `send := stream.send` preserves the call-site shape without leftover. Minor nits (not blocking, in-lens only): - `internal/service/revisions.go:159-163` and `internal/service/revisions_test.go:811-818`: the `commitScope` doc and the test doc carry a multi-line historical narrative ("This was originally only done on the failure path… Found in production with 18 plantings and no change set behind them"). CLAUDE.md flags stale comments as worse than none; this story is true today but is the kind of "why we used to do X" prose that rots across refactors. A one-line "Detached from cancellation because the data is already committed; cancelling can only lose the record" would age better. Trivial. - `internal/api/agent.go:157-160`: the two `case <-done` / `case <-s.c.Request.Context().Done()` arms both `return` — could be a single `case <-done, <-s.c.Request.Context().Done():` but the split reads more clearly, so leaving it is defensible. Trivial. - `internal/api/agent.go:71-72` and `:146-147`: "SSE ignores comment frames, so this costs the client nothing" appears twice (inline + doc). Mild duplication of rationale; fine. No structural, naming, dead-code, or pattern-divergence issues found in this lens. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> VERDICT: No material issues found Reviewed through the performance lens. - **`eventStream` mutex (`internal/api/agent.go:112,138`)** — Verified by reading the file. The mutex is taken on every `send` and every keep-alive tick. Contention is essentially nil (one run goroutine producing step events at model pace, one ticker firing every 20s), and the critical section is a single `io.WriteString` + `Flush`. Not a hot path, not material. - **Keep-alive goroutine (`agent.go:148-170`)** — One extra goroutine per active SSE connection, waking at most every 20s to write a 14-byte comment frame. Negligible. `stopBeat()` blocks on `<-stopped`, but the goroutine's exit latency is bounded by an in-flight `write` (a single buffered write under the mutex) — sub-millisecond in practice. No leak: `done`, the request context, and the ticker all converge on exit. - **`commitScope` detachment (`internal/service/revisions.go:165`)** — `context.WithoutCancel(ctx)` allocates one trivial context wrapper per commit. Commits are per-turn, not per-row, so this is irrelevant to throughput. Verified by reading the surrounding code: `commitScope` is called once per `WithChangeSet`/revert, not in a loop. - No N+1, no unbounded growth, no work moved into a hot loop, no blocking calls introduced on a hot path. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> VERDICT: Minor issues found - `internal/service/revisions.go:210` — The PR's central thesis ("the commit write is detached from cancellation, always") is over-stated. `commitScope` (the scoped/agent path) does detach via `context.WithoutCancel`, but `record`'s auto-scope branch falls through to a direct `s.store.WriteChangeSet(ctx, …)` at line 210 using the caller's `ctx` un-detached. If that context is cancelled between a successful row mutation and the `record` call, the history write fails and the change is orphaned — the same failure mode this PR exists to close, just on the non-agent path where the window is narrower. Verified by reading both call sites. Suggested fix: apply `context.WithoutCancel(ctx)` on the auto-scope branch too, or weaken the "always" in the `commitScope` doc to "every scoped/agent commit." Confidence: medium that this is a genuine gap; the new doc comment currently overstates coverage. - `internal/api/agent.go:79` — `stopBeat()` is not deferred. If `h.agent.Run` (or anything between `keepAlive()` and `stopBeat()`) were to panic, the stop closure wouldn't run. This is largely self-healing because the keep-alive goroutine also selects on `s.c.Request.Context().Done()` and gin cancels the request context while unwinding, so the goroutine exits rather than leaking permanently. Still, deferring `stopBeat()` would be the idiomatic edge-case-safe form and removes reliance on the request context firing during unwind. Confidence: high that the non-deferred call is a minor robustness gap; low severity. The new test (`TestSucceededTurnRecordsEvenIfTheCallerWentAway`) is sound: `cancel()` is fired after `FillNamedRegion` returns, so the row writes succeed against a live context, then the success-path `commitScope` runs against a dead context and is correctly detached — reproducing the production bug. No double-close or goroutine-leak issues in `keepAlive` (the stop closure is single-use; the goroutine exits on either `done`, request-context cancellation, or ticker). </details> </details> <details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — 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> — No material issues found</summary> No material issues found </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run returned no output </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error database is locked </details> <details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: opencode run failed: exit status 1: Error: Unexpected error database is locked </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **Verdict: No material issues found** Through the error-handling & edge-cases lens, the diff is clean. The `commitScope` detachment is placed in the one function that needs it, the keep-alive goroutine is properly lifecycle-managed, and the new test exercises the exact orphaned-change-set scenario. Nothing in my area to block on. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Author
Owner

Fixed in c4e8f92. The first finding is me making the same mistake one level up, in a PR whose entire subject is that mistake.

record()'s auto-scope path was still using the caller's context

This PR's headline claim is that the history write is "detached from cancellation, always". It wasn't. record() called store.WriteChangeSet directly, so every plain REST mutation kept exactly the orphan-history window the PR was written to close.

And that's the path virtually every change takes — the agent is the exception, not the rule. So I found the bug via the rare path, fixed the rare path, wrote "always" in a doc comment, and left the common one.

It goes through commitScope now, which is where the rule lives precisely so no caller can be the one that forgets. Tested identically to the agent path: cancel right after the mutation returns, assert the change set exists and reverts cleanly.

The rest

  • stopBeat() wasn't deferred (2 models) — a panic in the run would have leaked the ticker goroutine, and relying on request-context cancellation during unwind is not a guarantee. Deferred now, and idempotent since the handler also stops it explicitly on the normal path.
  • 20s was a magic literal duplicated between comment and call site → keepAliveInterval, with the reasoning (well under the 30–60s idle timeout typical of reverse proxies) on the constant.
  • commitScope's doc narrated the incident. Fair — godoc should state the rule and why it lives there; the post-mortem belongs in a commit message, which is where it now is.

go test -race ./..., go vet, gofmt green.

Fixed in `c4e8f92`. The first finding is me making **the same mistake one level up**, in a PR whose entire subject is that mistake. ### `record()`'s auto-scope path was still using the caller's context This PR's headline claim is that the history write is "detached from cancellation, **always**". It wasn't. `record()` called `store.WriteChangeSet` directly, so every plain REST mutation kept exactly the orphan-history window the PR was written to close. And that's the path **virtually every change takes** — the agent is the exception, not the rule. So I found the bug via the rare path, fixed the rare path, wrote "always" in a doc comment, and left the common one. It goes through `commitScope` now, which is where the rule lives precisely so no caller can be the one that forgets. Tested identically to the agent path: cancel right after the mutation returns, assert the change set exists and reverts cleanly. ### The rest - **`stopBeat()` wasn't deferred** (2 models) — a panic in the run would have leaked the ticker goroutine, and relying on request-context cancellation during unwind is not a guarantee. Deferred now, and idempotent since the handler also stops it explicitly on the normal path. - **20s was a magic literal** duplicated between comment and call site → `keepAliveInterval`, with the reasoning (well under the 30–60s idle timeout typical of reverse proxies) on the constant. - **`commitScope`'s doc narrated the incident.** Fair — godoc should state the rule and why it lives there; the post-mortem belongs in a commit message, which is where it now is. `go test -race ./...`, `go vet`, `gofmt` green.
steve merged commit 62604523d7 into main 2026-07-21 12:49:18 +00:00
steve deleted branch fix/commit-survives-disconnect 2026-07-21 12:49:18 +00:00
Sign in to join this conversation.