The conversational surface, in the editor rather than on its own page. The reason is the feedback loop: watching the canvas change as the agent works IS the confirmation — which is exactly what makes "act freely without asking first" tolerable. It also means the agent never has to guess which garden you mean.
Tool calls in the app's own vocabulary
"Clearing a bed", "Looking up a plant" — not raw tool names or JSON. That's the difference between the panel feeling like it's doing something and feeling like it's hung, which matters because a replant makes a dozen calls over tens of seconds.
An unknown tool degrades to readable words (prune_orchard → "prune orchard") rather than showing snake_case at the user, so the client can lag the server by a tool without looking broken.
The canvas refreshes as each step lands
Not just at the end. Refreshing only on completion would put the whole point of siting the chat here — watching it work — behind the same wait that streaming exists to remove.
Undo on the turn itself
So the common case never involves opening the History panel. It uses #49's useUndo, not a second implementation — which is what the issue asked for, and which forced a small honest change: the chat knows a turn's change set id but not its tally, so useUndo now takes an UndoTarget with optional counts. Fabricating counts to satisfy the type would have produced a confidently wrong "1 of 1 changes undone"; describeUndo says "Partly undone" when it has no denominator rather than inventing one.
Only offered when it works
New GET /capabilities. The agent routes 404 without a key, so without this the client would have to probe for a 404 to find out — and a tab that opens onto an apology is worse than no tab. With no key configured the editor looks exactly as it does today.
Streaming details worth knowing
Hand-rolled over fetch rather than EventSource, which can only issue GETs and this needs a POST body. The wire format is still SSE, so a proxy that understands it doesn't buffer and the server wouldn't change if EventSource became viable. Partial frames are buffered across chunks; a malformed frame is skipped rather than killing a working stream; aborting on unmount doesn't report an error, because navigating away isn't a failure.
Errors read as sentences, and as different sentences
A permission refusal, a timeout and a model failure want different reactions. Every failure path refreshes — something may have landed before it failed — and says where to look for it.
Verification
86 tests. New ones cover describeStep (vocabulary, repeat collapsing, unknown-tool fallback, empty step) and the UndoTarget change (no invented denominator, clean undo unchanged). tsc --noEmit, npm run build, go test ./... green.
With this, every issue in #58 is merged. I'll exercise the whole thing against the live deploy once it's up and report back.
Closes #57. **Completes the v2 epic (#58).**
The conversational surface, in the editor rather than on its own page. The reason is the feedback loop: **watching the canvas change as the agent works IS the confirmation** — which is exactly what makes "act freely without asking first" tolerable. It also means the agent never has to guess which garden you mean.
### Tool calls in the app's own vocabulary
*"Clearing a bed"*, *"Looking up a plant"* — not raw tool names or JSON. That's the difference between the panel feeling like it's doing something and feeling like it's hung, which matters because a replant makes a dozen calls over tens of seconds.
An unknown tool degrades to readable words (`prune_orchard` → "prune orchard") rather than showing snake_case at the user, so the client can lag the server by a tool without looking broken.
### The canvas refreshes as each step lands
Not just at the end. Refreshing only on completion would put the whole point of siting the chat here — *watching it work* — behind the same wait that streaming exists to remove.
### Undo on the turn itself
So the common case never involves opening the History panel. It uses **#49's `useUndo`, not a second implementation** — which is what the issue asked for, and which forced a small honest change: the chat knows a turn's change set id but not its tally, so `useUndo` now takes an `UndoTarget` with optional counts. Fabricating counts to satisfy the type would have produced a confidently wrong *"1 of 1 changes undone"*; `describeUndo` says **"Partly undone"** when it has no denominator rather than inventing one.
### Only offered when it works
New `GET /capabilities`. The agent routes 404 without a key, so without this the client would have to probe for a 404 to find out — and a tab that opens onto an apology is worse than no tab. With no key configured the editor looks exactly as it does today.
### Streaming details worth knowing
Hand-rolled over `fetch` rather than `EventSource`, which can only issue GETs and this needs a POST body. The wire format is still SSE, so a proxy that understands it doesn't buffer and the server wouldn't change if `EventSource` became viable. Partial frames are buffered across chunks; a malformed frame is skipped rather than killing a working stream; aborting on unmount doesn't report an error, because navigating away isn't a failure.
### Errors read as sentences, and as *different* sentences
A permission refusal, a timeout and a model failure want different reactions. Every failure path refreshes — something may have landed before it failed — and says where to look for it.
### Verification
86 tests. New ones cover `describeStep` (vocabulary, repeat collapsing, unknown-tool fallback, empty step) and the `UndoTarget` change (no invented denominator, clean undo unchanged). `tsc --noEmit`, `npm run build`, `go test ./...` green.
---
With this, every issue in #58 is merged. I'll exercise the whole thing against the live deploy once it's up and report back.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The conversational surface, in the editor rather than on its own page. The
reason is the feedback loop: watching the canvas change as the agent works IS
the confirmation, which is exactly what makes "act freely without asking first"
tolerable. It also means the agent never has to guess which garden you mean.
Tool calls surface as they happen, in the app's own vocabulary — "Clearing a
bed", "Looking up a plant" — not raw tool names or JSON. That is the difference
between the panel feeling like it's doing something and feeling like it's hung,
which matters because a replant makes a dozen calls over tens of seconds. An
unknown tool degrades to readable words rather than showing snake_case at the
user, so the client can lag the server by a tool without looking broken.
The canvas refreshes as each step lands, not just at the end. Refreshing only on
completion would put the whole point of siting the chat here — watching it work
— behind the same wait that streaming exists to remove.
Undo sits on the turn itself, so the common case never involves opening the
History panel. It uses #49's useUndo, not a second implementation, which meant
giving that hook an UndoTarget: the chat knows a turn's change set id but not
its tally, and fabricating counts to satisfy the type would have produced a
confidently wrong "1 of 1 changes undone". describeUndo now says "Partly undone"
when it has no denominator rather than inventing one.
The panel is only offered when the instance actually has the assistant
configured, via a new /capabilities read. The routes 404 without a key, so
without this the client would have to probe for a 404 to find out — and a tab
that opens onto an apology is worse than no tab.
Streaming is hand-rolled over fetch rather than EventSource, which can only
issue GETs and this needs a POST body. The wire format is still SSE, so a proxy
that understands it doesn't buffer and the server wouldn't change if EventSource
became viable. Partial frames are buffered across chunks and a malformed frame
is skipped rather than killing a working stream.
Errors read as sentences, and as different sentences: a permission refusal, a
timeout and a model failure want different reactions. Every failure path
refreshes, because something may have landed before it failed — and says where
to look for it.
Closes#57
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 06:33:21Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **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** — Minor issues
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ⚠️ **maintainability** — could not complete
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 10 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** — 10 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠Capabilities endpoint reports agent ready when routes may not be registered
correctness, maintainability · flagged by 4 models
/capabilities can falsely advertise a working assistant (internal/api/api.go:59-61). The endpoint returns cfg.Agent.Ready(), but the actual chat routes are only registered if agent.NewRunner succeeds. If the model spec is unresolvable (the exact failure case noted at line 137), Ready() is still true yet POST /agent/chat 404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.
🪰 Gadfly · advisory
🟠 **Capabilities endpoint reports agent ready when routes may not be registered**
_correctness, maintainability · flagged by 4 models_
- **`/capabilities` can falsely advertise a working assistant** (`internal/api/api.go:59-61`). The endpoint returns `cfg.Agent.Ready()`, but the actual chat routes are only registered if `agent.NewRunner` succeeds. If the model spec is unresolvable (the exact failure case noted at line 137), `Ready()` is still true yet POST `/agent/chat` 404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.
<sub>🪰 Gadfly · advisory</sub>
🟡history.isError is never surfaced; a failed history load silently renders as an empty conversation
error-handling · flagged by 1 model
web/src/editor/ChatPanel.tsx:27,86,111 — confirmed history.isError is never referenced anywhere in the file (only history.data and history.isSuccess are used). If GET /gardens/:id/agent/history fails, messages falls back to [] via history.data ?? [] (line 86), and since isSuccess stays false the placeholder "Ask for what you want…" text (gated on isSuccess, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state…
🪰 Gadfly · advisory
🟡 **history.isError is never surfaced; a failed history load silently renders as an empty conversation**
_error-handling · flagged by 1 model_
- `web/src/editor/ChatPanel.tsx:27,86,111` — confirmed `history.isError` is never referenced anywhere in the file (only `history.data` and `history.isSuccess` are used). If `GET /gardens/:id/agent/history` fails, `messages` falls back to `[]` via `history.data ?? []` (line 86), and since `isSuccess` stays false the placeholder "Ask for what you want…" text (gated on `isSuccess`, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state…
<sub>🪰 Gadfly · advisory</sub>
🔴Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown
correctness · flagged by 1 model
web/src/editor/ChatPanel.tsx:41 — Unmounting ChatPanel (via useEffect(() => () => abort.current?.abort(), [])) silently aborts an in-flight agent turn. EditorRail renders only the active tab lazily (web/src/editor/EditorRail.tsx:27,85), and GardenEditorPage.tsx:150-153 unconditionally calls setRailTab('inspector') on every object/plop selection — with no guard for a pending chat turn — overriding an open 'chat' tab and unmounting ChatPanel mid-stream. Server-side, `intern…
🪰 Gadfly · advisory
🔴 **Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown**
_correctness · flagged by 1 model_
- **`web/src/editor/ChatPanel.tsx:41`** — Unmounting `ChatPanel` (via `useEffect(() => () => abort.current?.abort(), [])`) silently aborts an in-flight agent turn. `EditorRail` renders only the active tab lazily (`web/src/editor/EditorRail.tsx:27,85`), and `GardenEditorPage.tsx:150-153` unconditionally calls `setRailTab('inspector')` on every object/plop selection — with no guard for a pending chat turn — overriding an open `'chat'` tab and unmounting `ChatPanel` mid-stream. Server-side, `intern…
<sub>🪰 Gadfly · advisory</sub>
web/src/editor/ChatPanel.tsx:93-100 — "Start over" errors are silent. useClearAgentHistory has no onError handler, and the component never reads clear.isError. If the DELETE fails, the button simply exits its pending state with no feedback. Fix: check clear.isError and render an alert, or at least disable the button and show the error text.
🪰 Gadfly · advisory
🟡 **Clear history mutation errors silently ignored**
_error-handling · flagged by 2 models_
- `web/src/editor/ChatPanel.tsx:93-100` — \"Start over\" errors are silent. `useClearAgentHistory` has no `onError` handler, and the component never reads `clear.isError`. If the DELETE fails, the button simply exits its pending state with no feedback. **Fix:** check `clear.isError` and render an alert, or at least disable the button and show the error text.
<sub>🪰 Gadfly · advisory</sub>
🟠History loading and error states silently swallowed
error-handling · flagged by 1 model
web/src/editor/ChatPanel.tsx:111-128 — The conversation pane only renders content when history.isSuccess is true. If the history query fails (history.isError) or is still loading (history.isLoading / history.isPending), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space. - Fix: Add explicit branches for history.isPending and history.isError inside the scrol…
🪰 Gadfly · advisory
🟠 **History loading and error states silently swallowed**
_error-handling · flagged by 1 model_
- **`web/src/editor/ChatPanel.tsx:111-128`** — The conversation pane only renders content when `history.isSuccess` is true. If the history query fails (`history.isError`) or is still loading (`history.isLoading` / `history.isPending`), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space. - *Fix:* Add explicit branches for `history.isPending` and `history.isError` inside the scrol…
<sub>🪰 Gadfly · advisory</sub>
🟡Uses React.ReactNode without importing React, unlike every other component's import type { ReactNode } convention
maintainability · flagged by 2 models
web/src/editor/ChatPanel.tsx:202 — children?: React.ReactNode uses the React namespace without importing it anywhere in the file (only { useEffect, useRef, useState } is imported from 'react'). Every other component in the codebase types children as ReactNode via import type { ReactNode } from 'react'. Match that convention.
🪰 Gadfly · advisory
🟡 **Uses React.ReactNode without importing React, unlike every other component's `import type { ReactNode }` convention**
_maintainability · flagged by 2 models_
- `web/src/editor/ChatPanel.tsx:202` — `children?: React.ReactNode` uses the `React` namespace without importing it anywhere in the file (only `{ useEffect, useRef, useState }` is imported from `'react'`). Every other component in the codebase types children as `ReactNode` via `import type { ReactNode } from 'react'`. Match that convention.
<sub>🪰 Gadfly · advisory</sub>
🔴Abort during fetch falsely reported as network error
correctness, error-handling · flagged by 5 models
web/src/lib/agent.ts:128 — The fetch catch block reports "Could not reach the server" for every thrown error, but it never checks signal?.aborted. If the user clicks Stop (or the panel unmounts) while the fetch is still in flight, the intentional AbortError is misreported as a network failure. The reader.read() phase later in the same function correctly guards with if (signal?.aborted) return (line 151); the fetch phase needs the same guard.
🪰 Gadfly · advisory
🔴 **Abort during fetch falsely reported as network error**
_correctness, error-handling · flagged by 5 models_
- **`web/src/lib/agent.ts:128`** — The `fetch` catch block reports *"Could not reach the server"* for every thrown error, but it never checks `signal?.aborted`. If the user clicks **Stop** (or the panel unmounts) while the `fetch` is still in flight, the intentional `AbortError` is misreported as a network failure. The `reader.read()` phase later in the same function correctly guards with `if (signal?.aborted) return` (line 151); the `fetch` phase needs the same guard.
<sub>🪰 Gadfly · advisory</sub>
🔴Server Warning field silently dropped in SSE stream
correctness, error-handling, maintainability · flagged by 4 models
web/src/lib/agent.ts:171-174 — The client parses SSE events into { step?, done?, error? } but the server also sends a warning field (see internal/api/agent.go:34-43 and agent.go:90). When the turn succeeds but the transcript can't be saved, the server emits {"done": {...}, "warning": "I couldn't save this exchange..."} so the user knows the conversation will disappear on reload. The client ignores warning entirely, producing the exact "quiet inconsistency" the server comment…
🪰 Gadfly · advisory
🔴 **Server Warning field silently dropped in SSE stream**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`web/src/lib/agent.ts:171-174`** — The client parses SSE events into `{ step?, done?, error? }` but the server also sends a `warning` field (see `internal/api/agent.go:34-43` and `agent.go:90`). When the turn succeeds but the transcript can't be saved, the server emits `{"done": {...}, "warning": "I couldn't save this exchange..."}` so the user knows the conversation will disappear on reload. The client ignores `warning` entirely, producing the exact "quiet inconsistency" the server comment…
<sub>🪰 Gadfly · advisory</sub>
🟠useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn
performance · flagged by 2 models
web/src/lib/agent.ts:184-186 — useAgentRefresh invalidates the conversation history and change-set history on every step, but neither changes mid-turn.onStep calls refresh(), which invalidates gardenFullKey, historyKey, and agentHistoryKey all at once. I verified the server side: the conversation transcript is only written after the run completes via RecordAgentExchange (internal/api/agent.go:87), and the change set is only committed after the run function returns vi…
🪰 Gadfly · advisory
🟠 **useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn**
_performance · flagged by 2 models_
- **`web/src/lib/agent.ts:184-186` — `useAgentRefresh` invalidates the conversation history and change-set history on every step, but neither changes mid-turn.** `onStep` calls `refresh()`, which invalidates `gardenFullKey`, `historyKey`, and `agentHistoryKey` all at once. I verified the server side: the conversation transcript is only written *after* the run completes via `RecordAgentExchange` (`internal/api/agent.go:87`), and the change set is only committed *after* the run function returns vi…
<sub>🪰 Gadfly · advisory</sub>
🟡describeUndo duplicates totalChanges' summation logic instead of sharing a helper
maintainability · flagged by 4 models
web/src/lib/history.ts:239 — describeUndo reimplements the exact summation target.counts.reduce((sum, c) => sum + c.n, 0) that totalChanges (line 117-119) already does for ChangeSet. Since UndoTarget.counts and ChangeSet.counts are both ChangeCount[], factor the reduce into a small shared helper (e.g. sumCounts(counts?: ChangeCount[])) and have both call it.
🪰 Gadfly · advisory
🟡 **describeUndo duplicates totalChanges' summation logic instead of sharing a helper**
_maintainability · flagged by 4 models_
- `web/src/lib/history.ts:239` — `describeUndo` reimplements the exact summation `target.counts.reduce((sum, c) => sum + c.n, 0)` that `totalChanges` (line 117-119) already does for `ChangeSet`. Since `UndoTarget.counts` and `ChangeSet.counts` are both `ChangeCount[]`, factor the reduce into a small shared helper (e.g. `sumCounts(counts?: ChangeCount[])`) and have both call it.
<sub>🪰 Gadfly · advisory</sub>
Verdict: Blocking issues found · 10 findings (7 with multi-model agreement)
Finding
Where
Models
Lens
🔴
Abort during fetch falsely reported as network error
web/src/lib/agent.ts:128
5/5
correctness, error-handling
🔴
Server Warning field silently dropped in SSE stream
web/src/lib/agent.ts:171
4/5
correctness, error-handling, maintainability
🟠
Capabilities endpoint reports agent ready when routes may not be registered
internal/api/api.go:59
4/5
correctness, maintainability
🟡
describeUndo duplicates totalChanges' summation logic instead of sharing a helper
web/src/lib/history.ts:239
4/5
maintainability
🟠
useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn
web/src/lib/agent.ts:184
2/5
performance
🟡
Clear history mutation errors silently ignored
web/src/editor/ChatPanel.tsx:93
2/5
error-handling
🟡
Uses React.ReactNode without importing React, unlike every other component's 'import type { ReactNode }' convention
web/src/editor/ChatPanel.tsx:202
2/5
maintainability
3 single-model findings (lower confidence)
Finding
Where
Model
Lens
🔴
Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown
web/src/editor/ChatPanel.tsx:41
claude-code/sonnet
correctness
🟠
History loading and error states silently swallowed
web/src/editor/ChatPanel.tsx:111
kimi-k2.6:cloud
error-handling
🟡
history.isError is never surfaced; a failed history load silently renders as an empty conversation
web/src/editor/ChatPanel.tsx:27
claude-code/sonnet
error-handling
Per-model detailclaude-code/sonnet (claude-code) — Blocking issues found
Good, no dangerouslySetInnerHTML anywhere in the panel. Everything checks out server-side. My review is done.
VERDICT: No material issues found
Reviewed the diff through the security lens, with server-side verification (not just the client code):
Authz on the new chat surface is enforced server-side, not just client-side.ChatPanel's canEdit prop only hides the Undo button in the UI (web/src/editor/ChatPanel.tsx:97); the actual gate is service.AgentHistory/RecordAgentExchange/ClearAgentHistory (internal/service/agent.go:25,38,58) and Runner.Run → svc.GetGarden/svc.WithChangeSet (internal/agent/runtime.go:99,115), all of which call requireGardenRole(..., roleEditor). A viewer can't get the assistant to act regardless of what the client sends. Verified by reading the service layer, not assumed.
New GET /capabilities (internal/api/api.go:58) is intentionally unauthenticated and only returns a boolean ({"agent": bool}) — no secret, model name, or config detail leaks. Consistent with the stated design ("a dead button is worse than no button") and low-sensitivity by nature.
CSRF: /agent/chat is a POST and sits under v1.Use(h.csrfGuard()) (internal/api/api.go:54), which rejects cross-origin Origin headers for state-changing methods once PANSY_BASE_URL is set. streamChat's hand-rolled fetch (web/src/lib/agent.ts) doesn't bypass this — the guard reads the browser-set Origin header, not anything the client has to opt into.
XSS: message bodies from the model/user are rendered as plain JSX text in Bubble (web/src/editor/ChatPanel.tsx:198-215), no dangerouslySetInnerHTML anywhere in the new files — confirmed by grep.
SSE parsing: malformed frames are skipped, not fatal (web/src/lib/agent.ts, the catch { continue } around JSON.parse), and the stream only ever comes from same-origin /api/v1/agent/chat over credentials: 'same-origin' — not attacker-controlled input being deserialized unsafely.
Unknown-tool fallback (describeStep, TOOL_LABELS[t] ?? t.replace(/_/g, ' ')) only ever formats a string for display via JSX text, so a hypothetical malicious tool name still can't inject markup.
Nothing here introduces an authn/authz gap, injection, SSRF, unsafe deserialization, or credential leak.
🎯 Correctness — Blocking issues found
Both findings are independently confirmed against the actual source. The plumbing (lazy tab rendering, unconditional auto-switch to inspector, unconditional abort-on-unmount, request-scoped context passed into Run, and the asymmetric fetch vs reader.read() abort handling) all match the draft's claims exactly, with no guard code anywhere that would prevent the described failure.
VERDICT: Blocking issues found
web/src/editor/ChatPanel.tsx:41 — Unmounting ChatPanel (via useEffect(() => () => abort.current?.abort(), [])) silently aborts an in-flight agent turn. EditorRail renders only the active tab lazily (web/src/editor/EditorRail.tsx:27,85), and GardenEditorPage.tsx:150-153 unconditionally calls setRailTab('inspector') on every object/plop selection — with no guard for a pending chat turn — overriding an open 'chat' tab and unmounting ChatPanel mid-stream. Server-side, internal/api/agent.go:67 passes the request-scoped c.Request.Context() into h.agent.Run (internal/agent/runtime.go:90), so the aborted client fetch cancels the run before it returns a Turn, meaning RecordAgentExchange (internal/api/agent.go:87) is never reached — the turn is silently lost with no error surfaced, since the component that would display one is already gone. This directly undermines the PR's stated design premise of watching the canvas while the agent works.
Fix: don't unconditionally abort on unmount, and/or guard the auto-switch-to-inspector effect while a turn is pending.
web/src/lib/agent.ts:129 — The catch around the initial fetch() call in streamChat unconditionally calls handlers.onError('Could not reach the server.') with no signal?.aborted check, unlike the reader.read() catch a few lines later (agent.ts:151) which correctly special-cases if (signal?.aborted) return. Clicking Stop, or the unmount-abort from the first finding, while the POST/SSE handshake is still in flight throws an AbortError here and surfaces a misleading "Could not reach the server." message instead of silently doing nothing, contradicting the intended behavior that aborting shouldn't report an error.
Fix: check signal?.aborted in the fetch() catch too, matching the reader.read() catch.
🧹 Code cleanliness & maintainability — Minor issues
All three findings check out against the source. Details:
api.go:59-61 — confirmed: /capabilities is registered as an inline anonymous closure while healthz (the adjacent route) is a named package function and nearly everything else uses named h.xxx methods. Checked spa.go:28 — r.NoRoute(func(c *gin.Context) {...}) is the only other inline closure in the package, and it's structurally different (NoRoute has no equivalent grouped-route pattern to match).
history.ts:239 — confirmed: describeUndo does target.counts.reduce((sum, c) => sum + c.n, 0) (guarded by target.counts ?), which is the identical reduce totalChanges (line 117-119) performs on ChangeSet.counts. Both operate on ChangeCount[], so a shared sumCounts(counts?: ChangeCount[]) helper is a legitimate dedup opportunity.
ChatPanel.tsx:202 — confirmed: only { useEffect, useRef, useState } is imported from 'react'; React.ReactNode at line 202 relies on the global React namespace ambient-declared by @types/react (export as namespace React), which compiles fine under jsx: "react-jsx" but is inconsistent with every other component in the codebase (EditorRail.tsx, PageStub.tsx, Alert.tsx, Modal.tsx, AuthCard.tsx, Divider.tsx, EditorHint.tsx — all grep-confirmed), which import ReactNode explicitly via import type { ReactNode } from 'react'.
All three are real, minor, verified.
VERDICT: Minor issues
internal/api/api.go:59-61 — The new /capabilities handler is a bare anonymous closure registered inline in New, while every other route in this file (including healthz right next to it) is a named handler function or h.method. Pull capabilities out into a named function or method on handlers to match the established convention.
web/src/lib/history.ts:239 — describeUndo reimplements the exact summation target.counts.reduce((sum, c) => sum + c.n, 0) that totalChanges (line 117-119) already does for ChangeSet. Since UndoTarget.counts and ChangeSet.counts are both ChangeCount[], factor the reduce into a small shared helper (e.g. sumCounts(counts?: ChangeCount[])) and have both call it.
web/src/editor/ChatPanel.tsx:202 — children?: React.ReactNode uses the React namespace without importing it anywhere in the file (only { useEffect, useRef, useState } is imported from 'react'). Every other component in the codebase types children as ReactNode via import type { ReactNode } from 'react'. Match that convention.
⚡ Performance — Minor issues
Everything checks out exactly as claimed. All three sub-claims verified: refresh() invalidates all three keys per step (agent.ts:181-187), invoked in onStep (ChatPanel.tsx:62-67), WithChangeSet only commits history after fn returns (revisions.go:114-141), and RecordAgentExchange is called only once after h.agent.Run returns (agent.go:67-93).
VERDICT: Minor issues
web/src/lib/agent.ts:181-187 (useAgentRefresh), invoked from web/src/editor/ChatPanel.tsx:62-67 on every onStep — refresh() invalidates three query keys per step (gardenFullKey, historyKey, agentHistoryKey), but two of the three cannot have changed mid-turn, so refetching them per step is pure waste.
The change-set history entry is only written once the whole turn finishes: internal/service/revisions.go:114-141 (WithChangeSet) buffers revisions and calls commitScope (which the GET /gardens/:id/history read reflects) only after fn — the entire agent run — returns. The agent conversation messages are likewise written only after the turn: internal/api/agent.go:87-92 calls RecordAgentExchange only once, after h.agent.Run(...) returns, not per step. Meanwhile gardenFullKey refetching per step is legitimate, since garden mutations commit immediately per tool call.
So for a turn with N steps, this fires N redundant /gardens/:id/history fetches and N redundant /gardens/:id/agent/history fetches guaranteed to return unchanged data, on top of the (justified) N garden-full refetches. Low severity — wasted round trips and re-renders, not wrong behavior — but it scales with step count.
Fix: only invalidate gardenFullKey in the onStep handler; keep historyKey/agentHistoryKey invalidation in onDone/onError, where refresh() is already called (ChatPanel.tsx:70,79).
🧯 Error handling & edge cases — Minor issues
Verified Review
VERDICT: Minor issues
web/src/lib/agent.ts:119-131 — the initial fetch() call's catch block unconditionally calls handlers.onError('Could not reach the server.'), without checking signal?.aborted the way the later reader.read() catch does (agent.ts:148-153, which correctly returns silently on abort). Confirmed both call sites that can trigger an abort before headers arrive: ChatPanel.tsx:41 (useEffect cleanup, abort.current?.abort() on unmount) and ChatPanel.tsx:177-181 (Stop button). The server only flushes SSE headers after loading agent history and calling openEventStream (internal/api/agent.go:59,65,101-105), so an abort issued before that point makes fetch() reject with AbortError, hits this unguarded catch, and surfaces a false "Could not reach the server." message — most visibly when the user clicks Stop, since the panel is still mounted to show it. Fix: mirror the if (signal?.aborted) return guard used a few lines down.
web/src/editor/ChatPanel.tsx:27,86,111 — confirmed history.isError is never referenced anywhere in the file (only history.data and history.isSuccess are used). If GET /gardens/:id/agent/history fails, messages falls back to [] via history.data ?? [] (line 86), and since isSuccess stays false the placeholder "Ask for what you want…" text (gated on isSuccess, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state, and no error is surfaced to the user.
web/src/editor/ChatPanel.tsx:95 — confirmed onClick={() => clear.mutate()} passes no onError callback, and useClearAgentHistory (agent.ts:53-61) only wires onSuccess. If the DELETE fails, clear.isPending returns to false and the button simply re-enables with no feedback that "Start over" didn't actually clear anything.
Through the security lens, I verified the new surface and the existing guards it relies on:
/capabilities (internal/api/api.go:59) is registered on v1beforerequireAuth, so it is unauthenticated. I confirmed it returns only {"agent": cfg.Agent.Ready()} and that Ready() (internal/config/config.go:82) is a boolean of Enabled && key != "" && model != "" — it does not leak the API key or model spec. This is intentional info disclosure (a single readiness bit) and matches the DESIGN.md note. Trivial at most; no secret leakage.
Authz on the agent routes is preserved: agentChat, getAgentHistory, deleteAgentHistory are all under h.requireAuth() (internal/api/api.go:144-147), and the service layer re-checks roleEditor (internal/service/agent.go:25,38,58). A viewer opening the Assistant tab (the tab isn't gated by canEdit in GardenEditorPage) will be refused at the service layer — no privilege bypass. (UX, not security.)
CSRF: streamChat's raw fetch POST is same-origin (API_BASE='/api/v1', credentials:'same-origin') and inherits csrfGuard (internal/api/auth.go:136), which rejects cross-origin state-changing requests when PANSY_BASE_URL is set. Consistent with the rest of the API.
XSS / injection: ChatPanel renders all message bodies and describeStep output as React text ({body}, {describeStep(s)}), no dangerouslySetInnerHTML. Stream error strings are server-authored and rendered as text. No untrusted-data interpolation into HTML, URLs, or SQL on this side.
Stream parsing (web/src/lib/agent.ts): malformed SSE frames are skipped, and events come from the trusted same-origin server response, so there's no untrusted-event injection path; the missing zod validation there is a robustness concern, not a security one.
No blocking or material security issues in this change.
🎯 Correctness — No material issues found
Verdict: No material issues found
Through the correctness lens I verified the load-bearing logic:
describeUndo / UndoTarget change (web/src/lib/history.ts:174,208,224-241): The new total = target.counts ? target.counts.reduce(...) : 0 reproduces totalChanges exactly (read at history.ts:117-119), so the HistoryPanel path (which passes a full ChangeSet with counts) keeps producing "N of M changes undone". When the chat panel passes { id } with no counts, total=0 → "Partly undone" — no invented denominator. The applied === 0 early-return at line 234 means the zero-denominator branch is only reached when something was actually applied, so the message is never misleading. Verified the test expectations at history.test.ts:116-141 match this behavior.
HistoryPanel still compiles against the widened type (web/src/editor/HistoryPanel.tsx:77,117): ChangeSet is structurally assignable to UndoTarget (id: number, counts: ChangeCount[] satisfies counts?: ChangeCount[]), and totalChanges(changeSet) > 0 at line 115 gates the button so the empty-counts edge case is unreachable from the history list. No behavior change there.
describeStep (web/src/lib/agent.ts): new Set(labels) dedups repeats; unknown-tool fallback t.replace(/_/g, ' ') yields "prune orchard" from "prune_orchard". Tool labels match the server-defined tool names (internal/agent/tools.go:21-61). Verified.
SSE parsing (streamChat): server emits data: %s\n\n (internal/api/agent.go:113); client splits on \n\n, takes the data: line, slice(5).trim(). JSON produced by the server contains no literal newlines, so single-line frames parse correctly. Malformed frames are skipped without aborting. Aborted reads return silently. Verified.
/capabilities (internal/api/api.go, internal/config/config.go:82-84): Ready() requires Enabled && key != "" && Model != "", matching the "agent 404s when unconfigured" premise.
ChatPanel state transitions: onError/onDone/Stop all clear pending and refresh; the unmount effect aborts. No state left dangling.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/lib/history.ts:239 — describeUndo re-implements the counts summation inline (target.counts.reduce((sum, c) => sum + c.n, 0)), byte-for-byte the same reduction already factored out as totalChanges at history.ts:117-119. The only reason it isn't reused is that totalChanges is typed to take a full ChangeSet rather than just { counts } (or an UndoTarget). This is duplicated logic in the one module that exists to share undo behavior; a future change to how counts are summed would have to be made in two places. Suggested fix: widen totalChanges to accept Pick<ChangeSet, 'counts'> (or an UndoTarget with counts), or extract a sumCounts(counts: ChangeCount[]): number helper that both call. Verified by reading both functions.
internal/api/api.go:59-61 — the /capabilities handler is an inline anonymous closure, while every other route in this file dispatches to a method on handlers (e.g. h.register, h.me, h.listGardens). The comment justifies the route's existence, not the style break. For consistency with the surrounding "handlers stay thin" pattern (and the file's own doc comment), this should be a h.capabilities(c *gin.Context) method. Trivial, but it's the one route that doesn't follow the file's own structure.
web/src/editor/ChatPanel.tsx:202 — children?: React.ReactNode reaches for the global React namespace while the rest of the file (and the imports at the top) use named imports from 'react' (useEffect, useRef, useState). It compiles (via @types/react's export as namespace React), but it's inconsistent with the file's own import style; import type { ReactNode } from 'react' would match. Trivial.
⚡ Performance — Minor issues
The finding is confirmed against the actual code:
useAgentRefresh (agent.ts:181-187) invalidates all three keys together.
onStep in ChatPanel.tsx calls refresh(), so all three are invalidated per step.
RecordAgentExchange (agent.go:87) runs only after h.agent.Run returns — conversation history is not written mid-turn.
commitScope (revisions.go:141/156-168) writes the change set only after the run's fn returns — change-set history is not committed mid-turn.
The canvas data (gardenFullKey) does mutate per step (tools write rows immediately; only the revisions are buffered), so invalidating it per step is intended.
So invalidating agentHistoryKey and historyKey on every step is redundant until the turn completes.
Verdict: Minor issues
web/src/lib/agent.ts:184-186 — useAgentRefresh invalidates the conversation history and change-set history on every step, but neither changes mid-turn.onStep calls refresh(), which invalidates gardenFullKey, historyKey, and agentHistoryKey all at once. I verified the server side: the conversation transcript is only written after the run completes via RecordAgentExchange (internal/api/agent.go:87), and the change set is only committed after the run function returns via commitScope (internal/service/revisions.go:141) — neither produces new rows during the step stream. The canvas data (gardenFullKey) does change per step as tools write rows immediately, so invalidating it per step is the intended "watch it work" behavior. But for a ~12-step turn, this fires ~11 redundant refetches of /gardens/:id/agent/history and ~11 of /gardens/:id/history, each a network round-trip plus DB queries returning identical data. Suggested fix: split the refresh — invalidate gardenFullKey in onStep, and invalidate agentHistoryKey/historyKey only in onDone/onError (where they already call refresh()). TanStack Query cancels overlapping refetches so this isn't catastrophic, but it's still ~22 wasted requests per turn on the feature's headline path. (Verified by reading internal/api/agent.go, internal/service/revisions.go, internal/agent/runtime.go, and internal/service/agent.go.)
🧯 Error handling & edge cases — Minor issues
Both findings are confirmed against the actual source.
Finding 1 verified: agent.ts:128-131 — the fetchcatch block unconditionally calls handlers.onError('Could not reach the server.') with no signal?.aborted check, unlike the read-loop catch at lines 148-153 which does check. The Stop handler (ChatPanel.tsx:177-178) and the unmount cleanup (ChatPanel.tsx:41) both call abort.current?.abort(), so an abort during the pending POST is misreported as a network failure.
Finding 2 verified: internal/api/agent.go:43 defines Warning string on chatEvent, and line 90 sends chatEvent{Done: turn, Warning: "I couldn't save this exchange…"}. The client's AgentTurn (agent.ts:64-69) has no warning field, and the dispatch at agent.ts:171-174 only checks error/step/done, so the warning is dropped.
VERDICT: Minor issues
web/src/lib/agent.ts:128 — intentional abort during the initial fetch is misreported as a network failure. The read-loop catch (line 148) correctly checks if (signal?.aborted) return and stays silent, but the fetchcatch at line 128 does not. When the user clicks Stop (ChatPanel.tsx:177-178 calls abort.current?.abort()) before the response headers arrive — i.e. while the initial POST is still pending — fetch rejects with an AbortError, this catch runs unconditionally, and handlers.onError('Could not reach the server.') fires. The onError handler then does setPending(null); setError('Could not reach the server.'); refresh(), so a user-initiated cancellation is shown to the user as "Could not reach the server." — the exact "navigating away isn't a failure" case the code claims to handle, just one layer earlier. The same path is hit on unmount (useEffect cleanup at ChatPanel.tsx:41 aborts), causing a state update after unmount. Fix: mirror the read-loop check, e.g. catch { if (signal?.aborted) return; handlers.onError('Could not reach the server.') }.
web/src/lib/agent.ts:171-174 — a done event carrying a server warning is silently dropped. The server's chatEvent (internal/api/agent.go:33-44) can send Warning alongside Done (e.g. "I couldn't save this exchange, so it won't be here after a reload…", sent at agent.go:90). The client's AgentTurn type (agent.ts:64-69) has no warning field, and the event dispatch (agent.ts:171-174) only inspects error/step/done. So when the transcript save fails, the user gets a clean "done" with no indication that the exchange won't persist after reload — the server explicitly designed that warning to avoid "the quieter lie." The onDone handler in ChatPanel.tsx:68-74 only surfaces truncated. Fix: add warning?: string to AgentTurn and the event shape, and surface it (e.g. setError/an Alert) in onDone.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
web/src/lib/agent.ts:128 — The fetch catch block reports "Could not reach the server" for every thrown error, but it never checks signal?.aborted. If the user clicks Stop (or the panel unmounts) while the fetch is still in flight, the intentional AbortError is misreported as a network failure. The reader.read() phase later in the same function correctly guards with if (signal?.aborted) return (line 151); the fetch phase needs the same guard.
web/src/lib/agent.ts:171-174 — The client parses SSE events into { step?, done?, error? } but the server also sends a warning field (see internal/api/agent.go:34-43 and agent.go:90). When the turn succeeds but the transcript can't be saved, the server emits {"done": {...}, "warning": "I couldn't save this exchange..."} so the user knows the conversation will disappear on reload. The client ignores warning entirely, producing the exact "quiet inconsistency" the server comment explicitly warns against.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/lib/agent.ts:171 — The streaming response events use a raw as cast (const e = event as { step?: AgentStep; … }) after JSON.parse. Every other external payload in this module validates with zod (capabilitiesSchema, agentMessageSchema, historySchema), and the file even imports z for that purpose. Skipping validation here breaks the module's own pattern; it also means only JSON-parse failures are skipped, while structurally malformed frames can still propagate to handlers. Fix: Add a small zod schema for the SSE event envelope and safeParse it in the frame loop, skipping frames that don't match.
web/src/lib/history.ts:239 — describeUndo manually reimplements the summation that totalChanges already performs (target.counts?.reduce((sum, c) => sum + c.n, 0)). The UndoTarget type was introduced precisely so callers don't need to fabricate counts; widening totalChanges to accept UndoTarget would remove the duplication and keep the tally logic in one place. Fix: Change totalChanges parameter from ChangeSet to UndoTarget (with optional chaining / nullish coalescing for the optional counts) and call it from describeUndo.
⚡ Performance — No material issues found
No material issues found
web/src/editor/ChatPanel.tsx:44-46 — scrollIntoView({ behavior: 'smooth' }) fires on every pending change, which happens on every agent step. A replant makes "a dozen calls over tens of seconds" (per the PR description), so a single turn triggers ~12 overlapping smooth-scroll animations. On lower-end devices this causes frame drops and visual jank. Fix: use 'auto' instead of 'smooth' for mid-turn updates (or debounce the scroll), reserving smooth scrolling only when the turn completes.
web/src/lib/agent.ts:143-161 — The SSE buffer (buffer += decoder.decode(...)) has no size ceiling. If the server sends data without a terminating \n\n (buggy proxy, malformed stream, or deliberate abuse), buffer grows unbounded until the tab runs out of memory. Fix: cap the buffer (e.g., at 1 MB) and discard/abort if it exceeds the limit without producing a complete frame.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/lib/agent.ts:129 — The fetch catch block reports "Could not reach the server." for every error, including an intentional AbortError when the user clicks Stop. The reader.read() catch two lines later correctly checks signal?.aborted and returns silently, but the initial fetch catch omits that guard. If the user aborts while the request is still opening, they get a false network-error message instead of a clean cancellation.
Fix: Add if (signal?.aborted) return before handlers.onError(...) in the fetch catch.
web/src/editor/ChatPanel.tsx:111-128 — The conversation pane only renders content when history.isSuccess is true. If the history query fails (history.isError) or is still loading (history.isLoading / history.isPending), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space.
Fix: Add explicit branches for history.isPending and history.isError inside the scroll container, e.g. a "Loading history…" / "Could not load conversation" message.
opencode/glm-5.2:cloud (opencode) — Blocking issues found
I've examined the diff and verified the security-relevant paths against the checked-out code. Let me trace the key authn/authz and input-validation paths.
Verified:
/capabilities (api.go:59) is unauthenticated but only reveals {"agent": bool} — equivalent to the existing /auth/providers pattern, no sensitive data.
POST /agent/chat (agent.go:51) calls h.svc.AgentHistory which requires roleEditor (service/agent.go:25) before opening the SSE stream — a viewer is rejected before any work runs. gardenId from the body is checked against the actor's role, so no IDOR.
GET/DELETE /gardens/:id/agent/history route through AgentHistory/ClearAgentHistory, both roleEditor-gated (service/agent.go:24, 57).
csrfGuard (auth.go:136) applies to the v1 group including /agent/chat; the client's fetch uses credentials: 'same-origin' and browsers emit an Origin header for same-origin POSTs, so the guard passes legitimately.
Agent reply body (m.body) and step labels render as React text children inside Bubble/<li>/<Alert> — auto-escaped, no dangerouslySetInnerHTML. No XSS vector from model output or error strings.
SSE events are parsed as unknown then narrowed by field; no eval/template injection. Server is trusted; no zod validation is a robustness nit, not a security hole.
RecordAgentExchange uses context.WithoutCancel so a client aborting mid-stream can't suppress the persisted exchange/change-set — the audit trail survives.
One non-security observation (outside my lens): viewers see the Assistant tab and an enabled Send box (GardenEditorPage pushes the tab on capabilities.data?.agent alone, and ChatPanel only disables input on pending, not !canEdit). The server correctly rejects them, so this is UX, not an authz bypass.
VERDICT: No material issues found
🎯 Correctness — Minor issues
VERDICT: Minor issues
web/src/lib/agent.ts:171 — server's warning event field is silently dropped. The server's chatEvent (internal/api/agent.go:43) carries a Warning string, and at internal/api/agent.go:90 it is sent alongside Done when RecordAgentExchange fails — deliberately, per the comment, to avoid "a quiet inconsistency" where a reload silently forgets the exchange. The client's streamChat parses each event as { step?; done?; error? } (agent.ts:171) with no warning field, and AgentTurn (agent.ts:64-69) has no warning member. onDone in ChatPanel.tsx only checks turn.truncated. The warning is never surfaced, so the user is told nothing despite the server having gone out of its way to say something. Fix: add warning?: string to AgentTurn, read it in streamChat, and surface it in onDone (e.g. setError(turn.warning) alongside the truncated check).
internal/api/api.go:59-61 — /capabilities can advertise the agent while the chat route is absent. The endpoint reports cfg.Agent.Ready(), but the /agent/chat route is only registered when agent.NewRunneralso succeeds (api.go:134-150: routes live inside the else of the NewRunner error branch). In the case where Ready() is true but NewRunner fails (an unresolvable model spec, which the code explicitly accounts for at api.go:137-141), /capabilities returns {agent: true}, the client renders the Assistant tab, and the first POST 404s — surfacing as "This instance doesn't have the assistant configured," which is misleading (it is configured, just broken). The stated purpose of the endpoint (api.go:56-58) is to avoid exactly this dead-button case. Fix: gate the reported value on h.agent != nil (move the handler registration after the agent block, or capture readiness in the closure).
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual code.
Finding 1: history.ts:239 does target.counts.reduce((sum, c) => sum + c.n, 0); totalChanges at line 117-118 does cs.counts.reduce((sum, c) => sum + c.n, 0). Identical reduction logic, confirmed.
Finding 2: api.go:59 registers /capabilities as an inline closure reading cfg from the enclosing scope. Every other route in the package (confirmed via grep across internal/api) registers a named function (healthz package-level, or h.xxx methods). Confirmed.
VERDICT: Minor issues
web/src/lib/history.ts:239 — duplicated change tally. describeUndo now does target.counts.reduce((sum, c) => sum + c.n, 0) inline, which is the exact computation totalChanges (line 117-118) already owns. The only reason it can't reuse it is that totalChanges takes a ChangeSet while the new target is UndoTarget with optional counts. Extract sumCounts(counts: ChangeCount[]): number and have both call it, so the count is computed one way. (Verified by reading both call sites.)
internal/api/api.go:59 — inline closure is the only handler in the package not following the named-handler convention. Every other route registers a named function (healthz package-level, or func (h *handlers) xxx). The closure also reads cfg from the enclosing scope rather than h.cfg. Suggested fix: add func (h *handlers) capabilities(c *gin.Context) and register h.capabilities, matching the surrounding code and keeping the handler testable in isolation. (Verified via grep — every other v1.GET/POST/PATCH/DELETE in the package uses a named function.)
⚡ Performance — No material issues found
VERDICT: No material issues found
I checked the per-step refresh() call in ChatPanel.tsx:62-67 for a hot-path regression. useAgentRefresh invalidates three queries (gardenFullKey, historyKey, agentHistoryKey) on every onStep, and a replant can fire a dozen steps over tens of seconds. The /gardens/:id/full payload is the whole garden, so naively this is a dozen full refetches. However: TanStack Query dedupes concurrent same-key refetches, and within a single SSE chunk the for (const frame of frames) loop at agent.ts:162 processes all frames synchronously, so multiple steps in one chunk coalesce into one scheduled refetch (react-query batches refetches queued in the same tick). Across chunks arriving milliseconds apart you get roughly one refetch per chunk, which is exactly the "watch the canvas update as it works" intent. Not a regression.
pending.steps uses [...p.steps, step] (ChatPanel.tsx:63), an O(n) copy per step, but n is bounded by a single turn (~dozens). Not material.
streamChat's buffer.split('\n\n') + frame.split('\n').find(...) (agent.ts:160-163) allocates per chunk, but frames are small and the stream is short-lived. Not material.
scrollIntoView({ behavior: 'smooth' }) re-fires on every step via the [history.data, pending] effect (ChatPanel.tsx:44-46); repeated smooth-scroll calls can queue against each other and cause minor jank, but this is cosmetic, not a performance regression worth flagging.
Verified by reading web/src/lib/agent.ts, web/src/editor/ChatPanel.tsx, and web/src/lib/objects.ts (for gardenFullKey and the default staleTime in queryClient.ts).
🧯 Error handling & edge cases — Blocking issues found
Both findings confirmed against the actual code. Let me verify the ChatPanel Stop handler.
Both findings verified against the actual code.
Finding 1 (fetch abort):agent.ts:128-130 — the fetch catch block calls handlers.onError('Could not reach the server.') without checking signal?.aborted. The read-side catch at line 148-151 correctly guards with if (signal?.aborted) return. ChatPanel Stop button (line 178) and unmount effect (line 41) both call abort(), so user-initiated aborts surface as spurious errors. Confirmed.
Finding 2 (warning field dropped):agent.go:43 defines Warning string in chatEvent, and agent.go:90 emits it when the exchange couldn't be persisted. agent.ts:171 only destructures { step?, done?, error? } — no warning. The AgentTurn interface (agent.ts:64-69) has no warning field. Confirmed.
Review
VERDICT: Blocking issues found
web/src/lib/agent.ts:128-130 — fetch abort reports a spurious error to the user. The catch around fetch doesn't check signal?.aborted before calling handlers.onError('Could not reach the server.'). When the user clicks Stop (ChatPanel.tsx:178) or the panel unmounts (ChatPanel.tsx:41), both of which call abort.current?.abort(), the in-flight fetch rejects with an AbortError and the panel shows "Could not reach the server." The read-side catch at lines 148-151 guards the abort case correctly ("an aborted read is the caller navigating away, not a failure worth reporting"), but the fetch-side catch is inconsistent with it and reintroduces exactly the spurious error the comment two lines below claims to avoid.
Fix: in the fetch catch, check if (signal?.aborted) return before calling onError, mirroring the read-side guard:
}catch{if(signal?.aborted)returnhandlers.onError('Could not reach the server.')return}
web/src/lib/agent.ts:171-174 — the server's warning field is silently dropped, hiding a partial-failure message. The server's chatEvent carries a Warning string that rides alongside Done (internal/api/agent.go:41-43), emitted when the turn succeeded but the exchange couldn't be persisted: "I couldn't save this exchange, so it won't be here after a reload. Anything I changed is still on the canvas, and in History." (agent.go:90). The client's event handler only destructures { step?, done?, error? } and never reads warning, so this deliberately-surfaced partial-failure message never reaches the user. The user reloads to check whether the change landed, finds the conversation gone, and the panel never told them why. Verified the server emits it and the client's AgentTurn interface (agent.ts:64-69) has no warning field either.
Fix: add warning?: string to the event type and surface it — e.g. set it as the error/banner string on done, or extend AgentTurn/onDone to carry it:
AuthZ on agent endpoints: Verified. agentChat calls AgentHistory first, which gates on roleEditor via requireGardenRole. The agent tools themselves (internal/agent/tools.go) funnel through service methods that enforce garden-level permissions. Viewers cannot reach the run loop, and the frontend's canEdit gating is consistent with the backend.
CSRF: The new POST /agent/chat route sits under /api/v1 where csrfGuard() is applied. The streamChat client (web/src/lib/agent.ts:121) sends credentials: 'same-origin', so the session cookie is included, and the existing Origin-header check will reject cross-origin state-changing requests when PANSY_BASE_URL is configured.
Injection / XSS: User and LLM text is rendered via React JSX ({body}, {error}, {describeStep(...)}), which escapes markup automatically. No dangerouslySetInnerHTML is introduced. SSE events are JSON-marshaled on the server before being written, so no newline injection into the SSE wire format is possible.
Info leak via /capabilities: The endpoint returns only a boolean agent flag. This same fact was already discoverable by probing /agent/chat for a 404, so the dedicated endpoint does not leak new information.
Undo button security: The changeSetId used for undo comes from the authenticated API response. The revertChangeSet handler resolves the garden from the change set and enforces roleEditor. A viewer (canEdit=false) sees no undo button, and the backend would reject any revert attempt anyway.
Outside my lens: None.
🎯 Correctness — Minor issues
Corrected review
Verdict: Minor issues
/capabilities can falsely advertise a working assistant (internal/api/api.go:59-61). The endpoint returns cfg.Agent.Ready(), but the actual chat routes are only registered if agent.NewRunner succeeds. If the model spec is unresolvable (the exact failure case noted at line 137), Ready() is still true yet POST /agent/chat 404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.
Client silently drops server-sent warning events (web/src/lib/agent.ts:171-174). The server's chatEvent carries a Warning field (e.g., when RecordAgentExchange fails: "I couldn't save this exchange, so it won't be here after a reload" — internal/api/agent.go:34-44,90). The client's SSE parser only looks at step, done, and error; the warning is ignored. The user sees a normal "Done" turn with no indication that the conversation will disappear on reload.
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run returned no output
⚡ Performance — No material issues found
No material issues found
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/lib/agent.ts:128-129 — Abort during fetch misreported as network failure.
The fetch catch block calls handlers.onError('Could not reach the server.') without checking signal.aborted. If the user clicks Stop before the request finishes (or the component unmounts), the AbortError is reported as a server failure, and setError fires on an unmounting component. Fix: check signal?.aborted and return silently before invoking onError, matching the reader.read() catch at line 151.
web/src/lib/agent.ts:171-174 — Server warning field swallowed.
When RecordAgentExchange fails, the server emits {\"done\": {...}, \"warning\": \"I couldn't save this exchange...\"} so the user knows the turn worked but the transcript won't persist. The client parses { step?, done?, error? } and ignores warning; the user sees a clean completion. Fix: include warning?: string in the parsed event shape and surface it to onDone (e.g., append it to the reply or call onError).
web/src/editor/ChatPanel.tsx:93-100 — "Start over" errors are silent. useClearAgentHistory has no onError handler, and the component never reads clear.isError. If the DELETE fails, the button simply exits its pending state with no feedback. Fix: check clear.isError and render an alert, or at least disable the button and show the error text.
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: Blocking issues found** · 10 findings (7 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | Abort during fetch falsely reported as network error | `web/src/lib/agent.ts:128` | 5/5 | correctness, error-handling |
| 🔴 | Server Warning field silently dropped in SSE stream | `web/src/lib/agent.ts:171` | 4/5 | correctness, error-handling, maintainability |
| 🟠 | Capabilities endpoint reports agent ready when routes may not be registered | `internal/api/api.go:59` | 4/5 | correctness, maintainability |
| 🟡 | describeUndo duplicates totalChanges' summation logic instead of sharing a helper | `web/src/lib/history.ts:239` | 4/5 | maintainability |
| 🟠 | useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn | `web/src/lib/agent.ts:184` | 2/5 | performance |
| 🟡 | Clear history mutation errors silently ignored | `web/src/editor/ChatPanel.tsx:93` | 2/5 | error-handling |
| 🟡 | Uses React.ReactNode without importing React, unlike every other component's 'import type { ReactNode }' convention | `web/src/editor/ChatPanel.tsx:202` | 2/5 | maintainability |
<details><summary>3 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🔴 | Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown | `web/src/editor/ChatPanel.tsx:41` | claude-code/sonnet | correctness |
| 🟠 | History loading and error states silently swallowed | `web/src/editor/ChatPanel.tsx:111` | kimi-k2.6:cloud | error-handling |
| 🟡 | history.isError is never surfaced; a failed history load silently renders as an empty conversation | `web/src/editor/ChatPanel.tsx:27` | claude-code/sonnet | error-handling |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Good, no dangerouslySetInnerHTML anywhere in the panel. Everything checks out server-side. My review is done.
## VERDICT: No material issues found
Reviewed the diff through the security lens, with server-side verification (not just the client code):
- **Authz on the new chat surface is enforced server-side, not just client-side.** `ChatPanel`'s `canEdit` prop only hides the Undo button in the UI (`web/src/editor/ChatPanel.tsx:97`); the actual gate is `service.AgentHistory`/`RecordAgentExchange`/`ClearAgentHistory` (`internal/service/agent.go:25,38,58`) and `Runner.Run` → `svc.GetGarden`/`svc.WithChangeSet` (`internal/agent/runtime.go:99,115`), all of which call `requireGardenRole(..., roleEditor)`. A viewer can't get the assistant to act regardless of what the client sends. Verified by reading the service layer, not assumed.
- **New `GET /capabilities` (`internal/api/api.go:58`) is intentionally unauthenticated** and only returns a boolean (`{"agent": bool}`) — no secret, model name, or config detail leaks. Consistent with the stated design ("a dead button is worse than no button") and low-sensitivity by nature.
- **CSRF**: `/agent/chat` is a POST and sits under `v1.Use(h.csrfGuard())` (`internal/api/api.go:54`), which rejects cross-origin `Origin` headers for state-changing methods once `PANSY_BASE_URL` is set. `streamChat`'s hand-rolled `fetch` (`web/src/lib/agent.ts`) doesn't bypass this — the guard reads the browser-set `Origin` header, not anything the client has to opt into.
- **XSS**: message bodies from the model/user are rendered as plain JSX text in `Bubble` (`web/src/editor/ChatPanel.tsx:198-215`), no `dangerouslySetInnerHTML` anywhere in the new files — confirmed by grep.
- **SSE parsing**: malformed frames are skipped, not fatal (`web/src/lib/agent.ts`, the `catch { continue }` around `JSON.parse`), and the stream only ever comes from same-origin `/api/v1/agent/chat` over `credentials: 'same-origin'` — not attacker-controlled input being deserialized unsafely.
- **Unknown-tool fallback** (`describeStep`, `TOOL_LABELS[t] ?? t.replace(/_/g, ' ')`) only ever formats a string for display via JSX text, so a hypothetical malicious tool name still can't inject markup.
Nothing here introduces an authn/authz gap, injection, SSRF, unsafe deserialization, or credential leak.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
Both findings are independently confirmed against the actual source. The plumbing (lazy tab rendering, unconditional auto-switch to inspector, unconditional abort-on-unmount, request-scoped context passed into `Run`, and the asymmetric `fetch` vs `reader.read()` abort handling) all match the draft's claims exactly, with no guard code anywhere that would prevent the described failure.
**VERDICT: Blocking issues found**
- **`web/src/editor/ChatPanel.tsx:41`** — Unmounting `ChatPanel` (via `useEffect(() => () => abort.current?.abort(), [])`) silently aborts an in-flight agent turn. `EditorRail` renders only the active tab lazily (`web/src/editor/EditorRail.tsx:27,85`), and `GardenEditorPage.tsx:150-153` unconditionally calls `setRailTab('inspector')` on every object/plop selection — with no guard for a pending chat turn — overriding an open `'chat'` tab and unmounting `ChatPanel` mid-stream. Server-side, `internal/api/agent.go:67` passes the request-scoped `c.Request.Context()` into `h.agent.Run` (`internal/agent/runtime.go:90`), so the aborted client fetch cancels the run before it returns a `Turn`, meaning `RecordAgentExchange` (`internal/api/agent.go:87`) is never reached — the turn is silently lost with no error surfaced, since the component that would display one is already gone. This directly undermines the PR's stated design premise of watching the canvas while the agent works.
Fix: don't unconditionally abort on unmount, and/or guard the auto-switch-to-inspector effect while a turn is pending.
- **`web/src/lib/agent.ts:129`** — The `catch` around the initial `fetch()` call in `streamChat` unconditionally calls `handlers.onError('Could not reach the server.')` with no `signal?.aborted` check, unlike the `reader.read()` catch a few lines later (`agent.ts:151`) which correctly special-cases `if (signal?.aborted) return`. Clicking **Stop**, or the unmount-abort from the first finding, while the POST/SSE handshake is still in flight throws an `AbortError` here and surfaces a misleading "Could not reach the server." message instead of silently doing nothing, contradicting the intended behavior that aborting shouldn't report an error.
Fix: check `signal?.aborted` in the `fetch()` catch too, matching the `reader.read()` catch.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings check out against the source. Details:
- **api.go:59-61** — confirmed: `/capabilities` is registered as an inline anonymous closure while `healthz` (the adjacent route) is a named package function and nearly everything else uses named `h.xxx` methods. Checked `spa.go:28` — `r.NoRoute(func(c *gin.Context) {...})` is the only other inline closure in the package, and it's structurally different (NoRoute has no equivalent grouped-route pattern to match).
- **history.ts:239** — confirmed: `describeUndo` does `target.counts.reduce((sum, c) => sum + c.n, 0)` (guarded by `target.counts ?`), which is the identical reduce `totalChanges` (line 117-119) performs on `ChangeSet.counts`. Both operate on `ChangeCount[]`, so a shared `sumCounts(counts?: ChangeCount[])` helper is a legitimate dedup opportunity.
- **ChatPanel.tsx:202** — confirmed: only `{ useEffect, useRef, useState }` is imported from `'react'`; `React.ReactNode` at line 202 relies on the global `React` namespace ambient-declared by `@types/react` (`export as namespace React`), which compiles fine under `jsx: "react-jsx"` but is inconsistent with every other component in the codebase (`EditorRail.tsx`, `PageStub.tsx`, `Alert.tsx`, `Modal.tsx`, `AuthCard.tsx`, `Divider.tsx`, `EditorHint.tsx` — all grep-confirmed), which import `ReactNode` explicitly via `import type { ReactNode } from 'react'`.
All three are real, minor, verified.
VERDICT: Minor issues
- `internal/api/api.go:59-61` — The new `/capabilities` handler is a bare anonymous closure registered inline in `New`, while every other route in this file (including `healthz` right next to it) is a named handler function or `h.method`. Pull `capabilities` out into a named function or method on `handlers` to match the established convention.
- `web/src/lib/history.ts:239` — `describeUndo` reimplements the exact summation `target.counts.reduce((sum, c) => sum + c.n, 0)` that `totalChanges` (line 117-119) already does for `ChangeSet`. Since `UndoTarget.counts` and `ChangeSet.counts` are both `ChangeCount[]`, factor the reduce into a small shared helper (e.g. `sumCounts(counts?: ChangeCount[])`) and have both call it.
- `web/src/editor/ChatPanel.tsx:202` — `children?: React.ReactNode` uses the `React` namespace without importing it anywhere in the file (only `{ useEffect, useRef, useState }` is imported from `'react'`). Every other component in the codebase types children as `ReactNode` via `import type { ReactNode } from 'react'`. Match that convention.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
Everything checks out exactly as claimed. All three sub-claims verified: `refresh()` invalidates all three keys per step (agent.ts:181-187), invoked in `onStep` (ChatPanel.tsx:62-67), `WithChangeSet` only commits history after `fn` returns (revisions.go:114-141), and `RecordAgentExchange` is called only once after `h.agent.Run` returns (agent.go:67-93).
VERDICT: Minor issues
- **`web/src/lib/agent.ts:181-187`** (`useAgentRefresh`), invoked from **`web/src/editor/ChatPanel.tsx:62-67`** on every `onStep` — `refresh()` invalidates three query keys per step (`gardenFullKey`, `historyKey`, `agentHistoryKey`), but two of the three cannot have changed mid-turn, so refetching them per step is pure waste.
The change-set history entry is only written once the whole turn finishes: `internal/service/revisions.go:114-141` (`WithChangeSet`) buffers revisions and calls `commitScope` (which the `GET /gardens/:id/history` read reflects) only after `fn` — the entire agent run — returns. The agent conversation messages are likewise written only after the turn: `internal/api/agent.go:87-92` calls `RecordAgentExchange` only once, after `h.agent.Run(...)` returns, not per step. Meanwhile `gardenFullKey` refetching per step is legitimate, since garden mutations commit immediately per tool call.
So for a turn with N steps, this fires N redundant `/gardens/:id/history` fetches and N redundant `/gardens/:id/agent/history` fetches guaranteed to return unchanged data, on top of the (justified) N `garden-full` refetches. Low severity — wasted round trips and re-renders, not wrong behavior — but it scales with step count.
Fix: only invalidate `gardenFullKey` in the `onStep` handler; keep `historyKey`/`agentHistoryKey` invalidation in `onDone`/`onError`, where `refresh()` is already called (`ChatPanel.tsx:70,79`).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## Verified Review
**VERDICT: Minor issues**
- `web/src/lib/agent.ts:119-131` — the initial `fetch()` call's `catch` block unconditionally calls `handlers.onError('Could not reach the server.')`, without checking `signal?.aborted` the way the later `reader.read()` catch does (agent.ts:148-153, which correctly returns silently on abort). Confirmed both call sites that can trigger an abort before headers arrive: `ChatPanel.tsx:41` (`useEffect` cleanup, `abort.current?.abort()` on unmount) and `ChatPanel.tsx:177-181` (Stop button). The server only flushes SSE headers after loading agent history and calling `openEventStream` (`internal/api/agent.go:59,65,101-105`), so an abort issued before that point makes `fetch()` reject with `AbortError`, hits this unguarded catch, and surfaces a false "Could not reach the server." message — most visibly when the user clicks Stop, since the panel is still mounted to show it. Fix: mirror the `if (signal?.aborted) return` guard used a few lines down.
- `web/src/editor/ChatPanel.tsx:27,86,111` — confirmed `history.isError` is never referenced anywhere in the file (only `history.data` and `history.isSuccess` are used). If `GET /gardens/:id/agent/history` fails, `messages` falls back to `[]` via `history.data ?? []` (line 86), and since `isSuccess` stays false the placeholder "Ask for what you want…" text (gated on `isSuccess`, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state, and no error is surfaced to the user.
- `web/src/editor/ChatPanel.tsx:95` — confirmed `onClick={() => clear.mutate()}` passes no `onError` callback, and `useClearAgentHistory` (`agent.ts:53-61`) only wires `onSuccess`. If the DELETE fails, `clear.isPending` returns to false and the button simply re-enables with no feedback that "Start over" didn't actually clear anything.
</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
Through the security lens, I verified the new surface and the existing guards it relies on:
- **`/capabilities` (internal/api/api.go:59)** is registered on `v1` *before* `requireAuth`, so it is unauthenticated. I confirmed it returns only `{"agent": cfg.Agent.Ready()}` and that `Ready()` (internal/config/config.go:82) is a boolean of `Enabled && key != "" && model != ""` — it does **not** leak the API key or model spec. This is intentional info disclosure (a single readiness bit) and matches the DESIGN.md note. Trivial at most; no secret leakage.
- **Authz on the agent routes** is preserved: `agentChat`, `getAgentHistory`, `deleteAgentHistory` are all under `h.requireAuth()` (internal/api/api.go:144-147), and the service layer re-checks `roleEditor` (internal/service/agent.go:25,38,58). A viewer opening the Assistant tab (the tab isn't gated by `canEdit` in GardenEditorPage) will be refused at the service layer — no privilege bypass. (UX, not security.)
- **CSRF**: `streamChat`'s raw `fetch` POST is same-origin (`API_BASE='/api/v1'`, `credentials:'same-origin'`) and inherits `csrfGuard` (internal/api/auth.go:136), which rejects cross-origin state-changing requests when `PANSY_BASE_URL` is set. Consistent with the rest of the API.
- **XSS / injection**: `ChatPanel` renders all message bodies and `describeStep` output as React text (`{body}`, `{describeStep(s)}`), no `dangerouslySetInnerHTML`. Stream `error` strings are server-authored and rendered as text. No untrusted-data interpolation into HTML, URLs, or SQL on this side.
- **Stream parsing** (web/src/lib/agent.ts): malformed SSE frames are skipped, and events come from the trusted same-origin server response, so there's no untrusted-event injection path; the missing zod validation there is a robustness concern, not a security one.
No blocking or material security issues in this change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the correctness lens I verified the load-bearing logic:
- **`describeUndo` / `UndoTarget` change** (`web/src/lib/history.ts:174,208,224-241`): The new `total = target.counts ? target.counts.reduce(...) : 0` reproduces `totalChanges` exactly (read at `history.ts:117-119`), so the HistoryPanel path (which passes a full `ChangeSet` with `counts`) keeps producing "N of M changes undone". When the chat panel passes `{ id }` with no `counts`, `total=0` → "Partly undone" — no invented denominator. The `applied === 0` early-return at line 234 means the zero-denominator branch is only reached when something was actually applied, so the message is never misleading. Verified the test expectations at `history.test.ts:116-141` match this behavior.
- **HistoryPanel still compiles against the widened type** (`web/src/editor/HistoryPanel.tsx:77,117`): `ChangeSet` is structurally assignable to `UndoTarget` (`id: number`, `counts: ChangeCount[]` satisfies `counts?: ChangeCount[]`), and `totalChanges(changeSet) > 0` at line 115 gates the button so the empty-counts edge case is unreachable from the history list. No behavior change there.
- **`describeStep`** (`web/src/lib/agent.ts`): `new Set(labels)` dedups repeats; unknown-tool fallback `t.replace(/_/g, ' ')` yields "prune orchard" from "prune_orchard". Tool labels match the server-defined tool names (`internal/agent/tools.go:21-61`). Verified.
- **SSE parsing** (`streamChat`): server emits `data: %s\n\n` (`internal/api/agent.go:113`); client splits on `\n\n`, takes the `data:` line, `slice(5).trim()`. JSON produced by the server contains no literal newlines, so single-line frames parse correctly. Malformed frames are skipped without aborting. Aborted reads return silently. Verified.
- **`/capabilities`** (`internal/api/api.go`, `internal/config/config.go:82-84`): `Ready()` requires `Enabled && key != "" && Model != ""`, matching the "agent 404s when unconfigured" premise.
- **ChatPanel state transitions**: `onError`/`onDone`/Stop all clear `pending` and refresh; the unmount effect aborts. No state left dangling.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- `web/src/lib/history.ts:239` — `describeUndo` re-implements the `counts` summation inline (`target.counts.reduce((sum, c) => sum + c.n, 0)`), byte-for-byte the same reduction already factored out as `totalChanges` at `history.ts:117-119`. The only reason it isn't reused is that `totalChanges` is typed to take a full `ChangeSet` rather than just `{ counts }` (or an `UndoTarget`). This is duplicated logic in the one module that exists to share undo behavior; a future change to how counts are summed would have to be made in two places. Suggested fix: widen `totalChanges` to accept `Pick<ChangeSet, 'counts'>` (or an `UndoTarget` with counts), or extract a `sumCounts(counts: ChangeCount[]): number` helper that both call. Verified by reading both functions.
- `internal/api/api.go:59-61` — the `/capabilities` handler is an inline anonymous closure, while every other route in this file dispatches to a method on `handlers` (e.g. `h.register`, `h.me`, `h.listGardens`). The comment justifies the route's existence, not the style break. For consistency with the surrounding "handlers stay thin" pattern (and the file's own doc comment), this should be a `h.capabilities(c *gin.Context)` method. Trivial, but it's the one route that doesn't follow the file's own structure.
- `web/src/editor/ChatPanel.tsx:202` — `children?: React.ReactNode` reaches for the global `React` namespace while the rest of the file (and the imports at the top) use named imports from `'react'` (`useEffect`, `useRef`, `useState`). It compiles (via `@types/react`'s `export as namespace React`), but it's inconsistent with the file's own import style; `import type { ReactNode } from 'react'` would match. Trivial.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
The finding is confirmed against the actual code:
- `useAgentRefresh` (agent.ts:181-187) invalidates all three keys together.
- `onStep` in ChatPanel.tsx calls `refresh()`, so all three are invalidated per step.
- `RecordAgentExchange` (agent.go:87) runs only after `h.agent.Run` returns — conversation history is not written mid-turn.
- `commitScope` (revisions.go:141/156-168) writes the change set only after the run's `fn` returns — change-set history is not committed mid-turn.
- The canvas data (`gardenFullKey`) does mutate per step (tools write rows immediately; only the *revisions* are buffered), so invalidating it per step is intended.
So invalidating `agentHistoryKey` and `historyKey` on every step is redundant until the turn completes.
**Verdict: Minor issues**
- **`web/src/lib/agent.ts:184-186` — `useAgentRefresh` invalidates the conversation history and change-set history on every step, but neither changes mid-turn.** `onStep` calls `refresh()`, which invalidates `gardenFullKey`, `historyKey`, and `agentHistoryKey` all at once. I verified the server side: the conversation transcript is only written *after* the run completes via `RecordAgentExchange` (`internal/api/agent.go:87`), and the change set is only committed *after* the run function returns via `commitScope` (`internal/service/revisions.go:141`) — neither produces new rows during the step stream. The canvas data (`gardenFullKey`) does change per step as tools write rows immediately, so invalidating it per step is the intended "watch it work" behavior. But for a ~12-step turn, this fires ~11 redundant refetches of `/gardens/:id/agent/history` and ~11 of `/gardens/:id/history`, each a network round-trip plus DB queries returning identical data. **Suggested fix:** split the refresh — invalidate `gardenFullKey` in `onStep`, and invalidate `agentHistoryKey`/`historyKey` only in `onDone`/`onError` (where they already call `refresh()`). TanStack Query cancels overlapping refetches so this isn't catastrophic, but it's still ~22 wasted requests per turn on the feature's headline path. (Verified by reading `internal/api/agent.go`, `internal/service/revisions.go`, `internal/agent/runtime.go`, and `internal/service/agent.go`.)
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings are confirmed against the actual source.
Finding 1 verified: `agent.ts:128-131` — the `fetch` `catch` block unconditionally calls `handlers.onError('Could not reach the server.')` with no `signal?.aborted` check, unlike the read-loop `catch` at lines 148-153 which does check. The Stop handler (`ChatPanel.tsx:177-178`) and the unmount cleanup (`ChatPanel.tsx:41`) both call `abort.current?.abort()`, so an abort during the pending POST is misreported as a network failure.
Finding 2 verified: `internal/api/agent.go:43` defines `Warning string` on `chatEvent`, and line 90 sends `chatEvent{Done: turn, Warning: "I couldn't save this exchange…"}`. The client's `AgentTurn` (`agent.ts:64-69`) has no `warning` field, and the dispatch at `agent.ts:171-174` only checks `error`/`step`/`done`, so the warning is dropped.
VERDICT: Minor issues
- **`web/src/lib/agent.ts:128` — intentional abort during the initial `fetch` is misreported as a network failure.** The read-loop `catch` (line 148) correctly checks `if (signal?.aborted) return` and stays silent, but the `fetch` `catch` at line 128 does not. When the user clicks **Stop** (`ChatPanel.tsx:177-178` calls `abort.current?.abort()`) *before* the response headers arrive — i.e. while the initial POST is still pending — `fetch` rejects with an `AbortError`, this `catch` runs unconditionally, and `handlers.onError('Could not reach the server.')` fires. The `onError` handler then does `setPending(null); setError('Could not reach the server.'); refresh()`, so a user-initiated cancellation is shown to the user as "Could not reach the server." — the exact "navigating away isn't a failure" case the code claims to handle, just one layer earlier. The same path is hit on unmount (`useEffect` cleanup at `ChatPanel.tsx:41` aborts), causing a state update after unmount. Fix: mirror the read-loop check, e.g. `catch { if (signal?.aborted) return; handlers.onError('Could not reach the server.') }`.
- **`web/src/lib/agent.ts:171-174` — a `done` event carrying a server `warning` is silently dropped.** The server's `chatEvent` (`internal/api/agent.go:33-44`) can send `Warning` alongside `Done` (e.g. "I couldn't save this exchange, so it won't be here after a reload…", sent at `agent.go:90`). The client's `AgentTurn` type (`agent.ts:64-69`) has no `warning` field, and the event dispatch (`agent.ts:171-174`) only inspects `error`/`step`/`done`. So when the transcript save fails, the user gets a clean "done" with no indication that the exchange won't persist after reload — the server explicitly designed that warning to avoid "the quieter lie." The `onDone` handler in `ChatPanel.tsx:68-74` only surfaces `truncated`. Fix: add `warning?: string` to `AgentTurn` and the event shape, and surface it (e.g. `setError`/an `Alert`) in `onDone`.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 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> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/lib/agent.ts:128`** — The `fetch` catch block reports *"Could not reach the server"* for every thrown error, but it never checks `signal?.aborted`. If the user clicks **Stop** (or the panel unmounts) while the `fetch` is still in flight, the intentional `AbortError` is misreported as a network failure. The `reader.read()` phase later in the same function correctly guards with `if (signal?.aborted) return` (line 151); the `fetch` phase needs the same guard.
- **`web/src/lib/agent.ts:171-174`** — The client parses SSE events into `{ step?, done?, error? }` but the server also sends a `warning` field (see `internal/api/agent.go:34-43` and `agent.go:90`). When the turn succeeds but the transcript can't be saved, the server emits `{"done": {...}, "warning": "I couldn't save this exchange..."}` so the user knows the conversation will disappear on reload. The client ignores `warning` entirely, producing the exact "quiet inconsistency" the server comment explicitly warns against.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/lib/agent.ts:171`** — The streaming response events use a raw `as` cast (`const e = event as { step?: AgentStep; … }`) after `JSON.parse`. Every other external payload in this module validates with zod (`capabilitiesSchema`, `agentMessageSchema`, `historySchema`), and the file even imports `z` for that purpose. Skipping validation here breaks the module's own pattern; it also means only JSON-parse failures are skipped, while structurally malformed frames can still propagate to handlers.
**Fix:** Add a small zod schema for the SSE event envelope and `safeParse` it in the frame loop, skipping frames that don't match.
- **`web/src/lib/history.ts:239`** — `describeUndo` manually reimplements the summation that `totalChanges` already performs (`target.counts?.reduce((sum, c) => sum + c.n, 0)`). The `UndoTarget` type was introduced precisely so callers don't need to fabricate counts; widening `totalChanges` to accept `UndoTarget` would remove the duplication and keep the tally logic in one place.
**Fix:** Change `totalChanges` parameter from `ChangeSet` to `UndoTarget` (with optional chaining / nullish coalescing for the optional `counts`) and call it from `describeUndo`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- **`web/src/editor/ChatPanel.tsx:44-46`** — `scrollIntoView({ behavior: 'smooth' })` fires on every `pending` change, which happens on every agent step. A replant makes "a dozen calls over tens of seconds" (per the PR description), so a single turn triggers ~12 overlapping smooth-scroll animations. On lower-end devices this causes frame drops and visual jank. **Fix:** use `'auto'` instead of `'smooth'` for mid-turn updates (or debounce the scroll), reserving smooth scrolling only when the turn completes.
- **`web/src/lib/agent.ts:143-161`** — The SSE buffer (`buffer += decoder.decode(...)`) has no size ceiling. If the server sends data without a terminating `\n\n` (buggy proxy, malformed stream, or deliberate abuse), `buffer` grows unbounded until the tab runs out of memory. **Fix:** cap the buffer (e.g., at 1 MB) and discard/abort if it exceeds the limit without producing a complete frame.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`web/src/lib/agent.ts:129`** — The `fetch` catch block reports "Could not reach the server." for **every** error, including an intentional `AbortError` when the user clicks **Stop**. The `reader.read()` catch two lines later correctly checks `signal?.aborted` and returns silently, but the initial `fetch` catch omits that guard. If the user aborts while the request is still opening, they get a false network-error message instead of a clean cancellation.
- *Fix:* Add `if (signal?.aborted) return` before `handlers.onError(...)` in the `fetch` catch.
- **`web/src/editor/ChatPanel.tsx:111-128`** — The conversation pane only renders content when `history.isSuccess` is true. If the history query fails (`history.isError`) or is still loading (`history.isLoading` / `history.isPending`), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space.
- *Fix:* Add explicit branches for `history.isPending` and `history.isError` inside the scroll container, e.g. a "Loading history…" / "Could not load conversation" message.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've examined the diff and verified the security-relevant paths against the checked-out code. Let me trace the key authn/authz and input-validation paths.
**Verified:**
- `/capabilities` (api.go:59) is unauthenticated but only reveals `{"agent": bool}` — equivalent to the existing `/auth/providers` pattern, no sensitive data.
- `POST /agent/chat` (agent.go:51) calls `h.svc.AgentHistory` which requires `roleEditor` (service/agent.go:25) *before* opening the SSE stream — a viewer is rejected before any work runs. `gardenId` from the body is checked against the actor's role, so no IDOR.
- `GET/DELETE /gardens/:id/agent/history` route through `AgentHistory`/`ClearAgentHistory`, both `roleEditor`-gated (service/agent.go:24, 57).
- `csrfGuard` (auth.go:136) applies to the v1 group including `/agent/chat`; the client's `fetch` uses `credentials: 'same-origin'` and browsers emit an `Origin` header for same-origin POSTs, so the guard passes legitimately.
- Agent reply body (`m.body`) and step labels render as React text children inside `Bubble`/`<li>`/`<Alert>` — auto-escaped, no `dangerouslySetInnerHTML`. No XSS vector from model output or error strings.
- SSE events are parsed as `unknown` then narrowed by field; no `eval`/template injection. Server is trusted; no zod validation is a robustness nit, not a security hole.
- `RecordAgentExchange` uses `context.WithoutCancel` so a client aborting mid-stream can't suppress the persisted exchange/change-set — the audit trail survives.
**One non-security observation (outside my lens):** viewers see the Assistant tab and an enabled Send box (GardenEditorPage pushes the tab on `capabilities.data?.agent` alone, and `ChatPanel` only disables input on `pending`, not `!canEdit`). The server correctly rejects them, so this is UX, not an authz bypass.
VERDICT: No material issues found
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## VERDICT: Minor issues
- **`web/src/lib/agent.ts:171` — server's `warning` event field is silently dropped.** The server's `chatEvent` (`internal/api/agent.go:43`) carries a `Warning` string, and at `internal/api/agent.go:90` it is sent alongside `Done` when `RecordAgentExchange` fails — deliberately, per the comment, to avoid "a quiet inconsistency" where a reload silently forgets the exchange. The client's `streamChat` parses each event as `{ step?; done?; error? }` (`agent.ts:171`) with no `warning` field, and `AgentTurn` (`agent.ts:64-69`) has no `warning` member. `onDone` in `ChatPanel.tsx` only checks `turn.truncated`. The `warning` is never surfaced, so the user is told nothing despite the server having gone out of its way to say something. Fix: add `warning?: string` to `AgentTurn`, read it in `streamChat`, and surface it in `onDone` (e.g. `setError(turn.warning)` alongside the truncated check).
- **`internal/api/api.go:59-61` — `/capabilities` can advertise the agent while the chat route is absent.** The endpoint reports `cfg.Agent.Ready()`, but the `/agent/chat` route is only registered when `agent.NewRunner` *also* succeeds (`api.go:134-150`: routes live inside the `else` of the NewRunner error branch). In the case where `Ready()` is true but `NewRunner` fails (an unresolvable model spec, which the code explicitly accounts for at `api.go:137-141`), `/capabilities` returns `{agent: true}`, the client renders the Assistant tab, and the first POST 404s — surfacing as "This instance doesn't have the assistant configured," which is misleading (it *is* configured, just broken). The stated purpose of the endpoint (`api.go:56-58`) is to avoid exactly this dead-button case. Fix: gate the reported value on `h.agent != nil` (move the handler registration after the agent block, or capture readiness in the closure).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified against the actual code.
- Finding 1: `history.ts:239` does `target.counts.reduce((sum, c) => sum + c.n, 0)`; `totalChanges` at line 117-118 does `cs.counts.reduce((sum, c) => sum + c.n, 0)`. Identical reduction logic, confirmed.
- Finding 2: `api.go:59` registers `/capabilities` as an inline closure reading `cfg` from the enclosing scope. Every other route in the package (confirmed via grep across `internal/api`) registers a named function (`healthz` package-level, or `h.xxx` methods). Confirmed.
VERDICT: Minor issues
- `web/src/lib/history.ts:239` — duplicated change tally. `describeUndo` now does `target.counts.reduce((sum, c) => sum + c.n, 0)` inline, which is the exact computation `totalChanges` (line 117-118) already owns. The only reason it can't reuse it is that `totalChanges` takes a `ChangeSet` while the new target is `UndoTarget` with optional counts. Extract `sumCounts(counts: ChangeCount[]): number` and have both call it, so the count is computed one way. (Verified by reading both call sites.)
- `internal/api/api.go:59` — inline closure is the only handler in the package not following the named-handler convention. Every other route registers a named function (`healthz` package-level, or `func (h *handlers) xxx`). The closure also reads `cfg` from the enclosing scope rather than `h.cfg`. Suggested fix: add `func (h *handlers) capabilities(c *gin.Context)` and register `h.capabilities`, matching the surrounding code and keeping the handler testable in isolation. (Verified via grep — every other `v1.GET`/`POST`/`PATCH`/`DELETE` in the package uses a named function.)
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
- I checked the per-step `refresh()` call in `ChatPanel.tsx:62-67` for a hot-path regression. `useAgentRefresh` invalidates three queries (`gardenFullKey`, `historyKey`, `agentHistoryKey`) on every `onStep`, and a replant can fire a dozen steps over tens of seconds. The `/gardens/:id/full` payload is the whole garden, so naively this is a dozen full refetches. However: TanStack Query dedupes concurrent same-key refetches, and within a single SSE chunk the `for (const frame of frames)` loop at `agent.ts:162` processes all frames synchronously, so multiple steps in one chunk coalesce into one scheduled refetch (react-query batches refetches queued in the same tick). Across chunks arriving milliseconds apart you get roughly one refetch per chunk, which is exactly the "watch the canvas update as it works" intent. Not a regression.
- `pending.steps` uses `[...p.steps, step]` (`ChatPanel.tsx:63`), an O(n) copy per step, but n is bounded by a single turn (~dozens). Not material.
- `streamChat`'s `buffer.split('\n\n')` + `frame.split('\n').find(...)` (`agent.ts:160-163`) allocates per chunk, but frames are small and the stream is short-lived. Not material.
- `scrollIntoView({ behavior: 'smooth' })` re-fires on every step via the `[history.data, pending]` effect (`ChatPanel.tsx:44-46`); repeated smooth-scroll calls can queue against each other and cause minor jank, but this is cosmetic, not a performance regression worth flagging.
Verified by reading `web/src/lib/agent.ts`, `web/src/editor/ChatPanel.tsx`, and `web/src/lib/objects.ts` (for `gardenFullKey` and the default `staleTime` in `queryClient.ts`).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
Both findings confirmed against the actual code. Let me verify the ChatPanel Stop handler.
Both findings verified against the actual code.
**Finding 1 (fetch abort):** `agent.ts:128-130` — the fetch `catch` block calls `handlers.onError('Could not reach the server.')` without checking `signal?.aborted`. The read-side catch at line 148-151 correctly guards with `if (signal?.aborted) return`. ChatPanel Stop button (line 178) and unmount effect (line 41) both call `abort()`, so user-initiated aborts surface as spurious errors. Confirmed.
**Finding 2 (warning field dropped):** `agent.go:43` defines `Warning string` in `chatEvent`, and `agent.go:90` emits it when the exchange couldn't be persisted. `agent.ts:171` only destructures `{ step?, done?, error? }` — no `warning`. The `AgentTurn` interface (agent.ts:64-69) has no `warning` field. Confirmed.
## Review
**VERDICT: Blocking issues found**
- **`web/src/lib/agent.ts:128-130` — fetch abort reports a spurious error to the user.** The `catch` around `fetch` doesn't check `signal?.aborted` before calling `handlers.onError('Could not reach the server.')`. When the user clicks **Stop** (`ChatPanel.tsx:178`) or the panel unmounts (`ChatPanel.tsx:41`), both of which call `abort.current?.abort()`, the in-flight `fetch` rejects with an `AbortError` and the panel shows "Could not reach the server." The read-side `catch` at lines 148-151 guards the abort case correctly ("an aborted read is the caller navigating away, not a failure worth reporting"), but the fetch-side catch is inconsistent with it and reintroduces exactly the spurious error the comment two lines below claims to avoid.
Fix: in the fetch `catch`, check `if (signal?.aborted) return` before calling `onError`, mirroring the read-side guard:
```ts
} catch {
if (signal?.aborted) return
handlers.onError('Could not reach the server.')
return
}
```
- **`web/src/lib/agent.ts:171-174` — the server's `warning` field is silently dropped, hiding a partial-failure message.** The server's `chatEvent` carries a `Warning` string that rides alongside `Done` (`internal/api/agent.go:41-43`), emitted when the turn succeeded but the exchange couldn't be persisted: *"I couldn't save this exchange, so it won't be here after a reload. Anything I changed is still on the canvas, and in History."* (`agent.go:90`). The client's event handler only destructures `{ step?, done?, error? }` and never reads `warning`, so this deliberately-surfaced partial-failure message never reaches the user. The user reloads to check whether the change landed, finds the conversation gone, and the panel never told them why. Verified the server emits it and the client's `AgentTurn` interface (`agent.ts:64-69`) has no `warning` field either.
Fix: add `warning?: string` to the event type and surface it — e.g. set it as the error/banner string on done, or extend `AgentTurn`/`onDone` to carry it:
```ts
const e = event as { step?: AgentStep; done?: AgentTurn; error?: string; warning?: string }
if (e.error) handlers.onError(e.error)
else if (e.step) handlers.onStep(e.step)
else if (e.done) handlers.onDone(e.done, e.warning)
```
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/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.
**Security lens review:**
- **AuthZ on agent endpoints:** Verified. `agentChat` calls `AgentHistory` first, which gates on `roleEditor` via `requireGardenRole`. The agent tools themselves (`internal/agent/tools.go`) funnel through service methods that enforce garden-level permissions. Viewers cannot reach the run loop, and the frontend's `canEdit` gating is consistent with the backend.
- **CSRF:** The new `POST /agent/chat` route sits under `/api/v1` where `csrfGuard()` is applied. The `streamChat` client (`web/src/lib/agent.ts:121`) sends `credentials: 'same-origin'`, so the session cookie is included, and the existing Origin-header check will reject cross-origin state-changing requests when `PANSY_BASE_URL` is configured.
- **Injection / XSS:** User and LLM text is rendered via React JSX (`{body}`, `{error}`, `{describeStep(...)}`), which escapes markup automatically. No `dangerouslySetInnerHTML` is introduced. SSE events are JSON-marshaled on the server before being written, so no newline injection into the SSE wire format is possible.
- **Info leak via `/capabilities`:** The endpoint returns only a boolean `agent` flag. This same fact was already discoverable by probing `/agent/chat` for a 404, so the dedicated endpoint does not leak new information.
- **Undo button security:** The `changeSetId` used for undo comes from the authenticated API response. The `revertChangeSet` handler resolves the garden from the change set and enforces `roleEditor`. A viewer (`canEdit=false`) sees no undo button, and the backend would reject any revert attempt anyway.
Outside my lens: None.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Corrected review
**Verdict: Minor issues**
- **`/capabilities` can falsely advertise a working assistant** (`internal/api/api.go:59-61`). The endpoint returns `cfg.Agent.Ready()`, but the actual chat routes are only registered if `agent.NewRunner` succeeds. If the model spec is unresolvable (the exact failure case noted at line 137), `Ready()` is still true yet POST `/agent/chat` 404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.
- **Client silently drops server-sent `warning` events** (`web/src/lib/agent.ts:171-174`). The server's `chatEvent` carries a `Warning` field (e.g., when `RecordAgentExchange` fails: *"I couldn't save this exchange, so it won't be here after a reload"* — `internal/api/agent.go:34-44,90`). The client's SSE parser only looks at `step`, `done`, and `error`; the `warning` is ignored. The user sees a normal "Done" turn with no indication that the conversation will disappear on reload.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run returned no output
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- `web/src/lib/agent.ts:128-129` — Abort during fetch misreported as network failure.
The `fetch` catch block calls `handlers.onError('Could not reach the server.')` without checking `signal.aborted`. If the user clicks **Stop** before the request finishes (or the component unmounts), the `AbortError` is reported as a server failure, and `setError` fires on an unmounting component.
**Fix:** check `signal?.aborted` and return silently before invoking `onError`, matching the `reader.read()` catch at line 151.
- `web/src/lib/agent.ts:171-174` — Server `warning` field swallowed.
When `RecordAgentExchange` fails, the server emits `{\"done\": {...}, \"warning\": \"I couldn't save this exchange...\"}` so the user knows the turn worked but the transcript won't persist. The client parses `{ step?, done?, error? }` and ignores `warning`; the user sees a clean completion.
**Fix:** include `warning?: string` in the parsed event shape and surface it to `onDone` (e.g., append it to the reply or call `onError`).
- `web/src/editor/ChatPanel.tsx:93-100` — \"Start over\" errors are silent.
`useClearAgentHistory` has no `onError` handler, and the component never reads `clear.isError`. If the DELETE fails, the button simply exits its pending state with no feedback.
**Fix:** check `clear.isError` and render an alert, or at least disable the button and show the error text.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Verifying the live deploy, the agent routes stayed 404 while every other route
from the same build answered — so the binary was current and Agent.Ready() was
simply false. Working out which of the three conditions failed meant reading the
source, because pansy logged the enabled case and said nothing at all about the
disabled one.
It now logs which condition failed, with the hint that actually matters: an
orchestrator's stack-level environment is not the container's environment, and
a key set in one but not passed through to the other looks exactly like a key
that was never set. README says the same thing next to the compose example.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The worst finding is one I created two commits ago: the server's `warning` field
— added precisely because a failed save was being swallowed — was never read by
the client. The "I couldn't save this exchange" message went nowhere, which
recreated the exact silence the warning existed to break. It's wired now, and
SSE frames are validated with zod rather than type-asserted, so the next field
added server-side fails loudly instead of vanishing.
Aborting during the initial fetch reported "Could not reach the server." The
read loop already knew an abort was the caller's own doing; the request path did
not, so pressing Stop looked like the network had failed.
Unmount no longer aborts an in-flight turn, and that one is a design bug of my
own making: selecting an object auto-switches the rail to the inspector, so
clicking the canvas mid-turn silently killed the turn — while the canvas is
exactly what you're meant to be watching. The request continues, the exchange is
persisted server-side, and coming back to the tab shows it. Stop still aborts,
but only our READ: the turn keeps running server-side either way, which is why
its work still lands in History.
/capabilities reported cfg.Agent.Ready() while the routes require NewRunner to
have succeeded. A configured-but-unresolvable model would therefore advertise a
chat tab whose first message 404s. It reports the runner's actual state now, and
is a named handler like everything else in that file.
A failed history load rendered as an empty conversation, which looks like the
thread was lost — a much worse thing to believe than "it didn't load". Both the
loading and error states are shown, and a failed "Start over" says so.
Per-step refresh refetched the change history and the conversation, neither of
which can move until the turn commits: up to 2×(N−1) requests per turn for data
that cannot have changed. Split into a canvas-only refresh for steps and a full
one for the end.
describeUndo had its own copy of totalChanges' summation; it uses the helper,
which now accepts an optional counts list. React.ReactNode became an imported
ReactNode, matching every other component.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
All fixed in f1c9e2b. Two of these are mine in an embarrassing way.
I left the warning field unwired
Two commits ago I added a server-side warning — specifically because a failed save was being swallowed and the user deserved to know. The client never read it. So "I couldn't save this exchange" went nowhere, recreating the exact silence the warning existed to break.
Wired now. And SSE frames are validated with zod rather than type-asserted, so the next field added server-side fails loudly instead of vanishing the same way.
Clicking the canvas killed the turn
Unmount aborted the in-flight stream — and selecting an object auto-switches the rail to the inspector, which unmounts the chat. So clicking the canvas mid-turn silently killed the turn, while the canvas is exactly what you're meant to be watching. A design bug I introduced by connecting two of my own decisions without noticing they met.
Unmount no longer aborts. The request continues, the exchange persists server-side, and returning to the tab shows it. Stop still aborts, but only our read — the turn keeps running server-side either way, which is why its work lands in History regardless. That's now stated in the code, because "Stop" implying cancellation would be its own small lie.
The rest
Abort during the initial fetch reported "Could not reach the server." (5 models). The read loop already knew an abort was the caller's own doing; the request path didn't.
/capabilities reported cfg.Agent.Ready() while the routes require NewRunner to have succeeded — so a configured-but-unresolvable model would advertise a chat tab whose first message 404s. It reports the runner's real state now, as a named handler like everything else in that file.
A failed history load rendered as an empty conversation, which looks like the thread was lost — a much worse thing to believe than "it didn't load". Loading and error states both shown; a failed "Start over" says so.
Per-step refresh refetched change history and the conversation, neither of which can move until the turn commits — up to 2×(N−1) requests per turn for data that cannot have changed. Split into a canvas-only refresh for steps and a full one at the end.
86 tests, tsc --noEmit, npm run build, go test ./..., go vet, gofmt green.
Separately, while verifying the live deploy I found the assistant isn't coming up on pansy.orgrimmar.dudenhoeffer.casa — the agent routes 404 while every other route from the same build answers, so the binary is current and Agent.Ready() is false. Details in a comment on #58; this PR adds a startup log line naming which condition failed, because working that out currently means reading the source.
All fixed in `f1c9e2b`. Two of these are mine in an embarrassing way.
### I left the `warning` field unwired
Two commits ago I added a server-side `warning` — *specifically* because a failed save was being swallowed and the user deserved to know. The client never read it. So "I couldn't save this exchange" went nowhere, **recreating the exact silence the warning existed to break.**
Wired now. And SSE frames are validated with zod rather than type-asserted, so the next field added server-side fails loudly instead of vanishing the same way.
### Clicking the canvas killed the turn
Unmount aborted the in-flight stream — and selecting an object **auto-switches the rail to the inspector**, which unmounts the chat. So clicking the canvas mid-turn silently killed the turn, while the canvas is exactly what you're meant to be watching. A design bug I introduced by connecting two of my own decisions without noticing they met.
Unmount no longer aborts. The request continues, the exchange persists server-side, and returning to the tab shows it. Stop still aborts, but only our *read* — the turn keeps running server-side either way, which is why its work lands in History regardless. That's now stated in the code, because "Stop" implying cancellation would be its own small lie.
### The rest
- **Abort during the initial `fetch` reported "Could not reach the server."** (5 models). The read loop already knew an abort was the caller's own doing; the request path didn't.
- **`/capabilities` reported `cfg.Agent.Ready()`** while the routes require `NewRunner` to have *succeeded* — so a configured-but-unresolvable model would advertise a chat tab whose first message 404s. It reports the runner's real state now, as a named handler like everything else in that file.
- **A failed history load rendered as an empty conversation**, which looks like the thread was *lost* — a much worse thing to believe than "it didn't load". Loading and error states both shown; a failed "Start over" says so.
- **Per-step refresh** refetched change history and the conversation, neither of which can move until the turn commits — up to 2×(N−1) requests per turn for data that cannot have changed. Split into a canvas-only refresh for steps and a full one at the end.
- `describeUndo` reuses `totalChanges`; `React.ReactNode` → imported `ReactNode`.
86 tests, `tsc --noEmit`, `npm run build`, `go test ./...`, `go vet`, `gofmt` green.
---
Separately, while verifying the live deploy I found the assistant isn't coming up on `pansy.orgrimmar.dudenhoeffer.casa` — the agent routes 404 while every other route from the same build answers, so the binary is current and `Agent.Ready()` is false. Details in a comment on #58; this PR adds a startup log line naming which condition failed, because working that out currently means reading the source.
steve
merged commit 8dbbc5439d into main2026-07-21 06:43:22 +00:00
steve
deleted branch feat/chat-panel2026-07-21 06:43:22 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #57. Completes the v2 epic (#58).
The conversational surface, in the editor rather than on its own page. The reason is the feedback loop: watching the canvas change as the agent works IS the confirmation — which is exactly what makes "act freely without asking first" tolerable. It also means the agent never has to guess which garden you mean.
Tool calls in the app's own vocabulary
"Clearing a bed", "Looking up a plant" — not raw tool names or JSON. That's the difference between the panel feeling like it's doing something and feeling like it's hung, which matters because a replant makes a dozen calls over tens of seconds.
An unknown tool degrades to readable words (
prune_orchard→ "prune orchard") rather than showing snake_case at the user, so the client can lag the server by a tool without looking broken.The canvas refreshes as each step lands
Not just at the end. Refreshing only on completion would put the whole point of siting the chat here — watching it work — behind the same wait that streaming exists to remove.
Undo on the turn itself
So the common case never involves opening the History panel. It uses #49's
useUndo, not a second implementation — which is what the issue asked for, and which forced a small honest change: the chat knows a turn's change set id but not its tally, souseUndonow takes anUndoTargetwith optional counts. Fabricating counts to satisfy the type would have produced a confidently wrong "1 of 1 changes undone";describeUndosays "Partly undone" when it has no denominator rather than inventing one.Only offered when it works
New
GET /capabilities. The agent routes 404 without a key, so without this the client would have to probe for a 404 to find out — and a tab that opens onto an apology is worse than no tab. With no key configured the editor looks exactly as it does today.Streaming details worth knowing
Hand-rolled over
fetchrather thanEventSource, which can only issue GETs and this needs a POST body. The wire format is still SSE, so a proxy that understands it doesn't buffer and the server wouldn't change ifEventSourcebecame viable. Partial frames are buffered across chunks; a malformed frame is skipped rather than killing a working stream; aborting on unmount doesn't report an error, because navigating away isn't a failure.Errors read as sentences, and as different sentences
A permission refusal, a timeout and a model failure want different reactions. Every failure path refreshes — something may have landed before it failed — and says where to look for it.
Verification
86 tests. New ones cover
describeStep(vocabulary, repeat collapsing, unknown-tool fallback, empty step) and theUndoTargetchange (no invented denominator, clean undo unchanged).tsc --noEmit,npm run build,go test ./...green.With this, every issue in #58 is merged. I'll exercise the whole thing against the live deploy once it's up and report back.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-21 06:33:21Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 10 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -56,0 +56,4 @@// What this instance can actually do, so the UI offers only what works. The// agent routes 404 when unconfigured; without this the client would have to// probe for a 404 to find that out, and a dead button is worse than no button.v1.GET("/capabilities", func(c *gin.Context) {🟠 Capabilities endpoint reports agent ready when routes may not be registered
correctness, maintainability · flagged by 4 models
/capabilitiescan falsely advertise a working assistant (internal/api/api.go:59-61). The endpoint returnscfg.Agent.Ready(), but the actual chat routes are only registered ifagent.NewRunnersucceeds. If the model spec is unresolvable (the exact failure case noted at line 137),Ready()is still true yet POST/agent/chat404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.🪰 Gadfly · advisory
@@ -0,0 +24,4 @@* mean.*/export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: boolean }) {const history = useAgentHistory(gardenId, true)🟡 history.isError is never surfaced; a failed history load silently renders as an empty conversation
error-handling · flagged by 1 model
web/src/editor/ChatPanel.tsx:27,86,111— confirmedhistory.isErroris never referenced anywhere in the file (onlyhistory.dataandhistory.isSuccessare used). IfGET /gardens/:id/agent/historyfails,messagesfalls back to[]viahistory.data ?? [](line 86), and sinceisSuccessstays false the placeholder "Ask for what you want…" text (gated onisSuccess, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state…🪰 Gadfly · advisory
@@ -0,0 +38,4 @@// Abort an in-flight turn when the panel goes away, so a closed tab doesn't// leave a read hanging.useEffect(() => () => abort.current?.abort(), [])🔴 Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown
correctness · flagged by 1 model
web/src/editor/ChatPanel.tsx:41— UnmountingChatPanel(viauseEffect(() => () => abort.current?.abort(), [])) silently aborts an in-flight agent turn.EditorRailrenders only the active tab lazily (web/src/editor/EditorRail.tsx:27,85), andGardenEditorPage.tsx:150-153unconditionally callssetRailTab('inspector')on every object/plop selection — with no guard for a pending chat turn — overriding an open'chat'tab and unmountingChatPanelmid-stream. Server-side, `intern…🪰 Gadfly · advisory
@@ -0,0 +90,4 @@<div className="flex items-center justify-between gap-2"><h2 className="text-sm font-semibold text-fg">Assistant</h2>{messages.length > 0 && (<button🟡 Clear history mutation errors silently ignored
error-handling · flagged by 2 models
web/src/editor/ChatPanel.tsx:93-100— "Start over" errors are silent.useClearAgentHistoryhas noonErrorhandler, and the component never readsclear.isError. If the DELETE fails, the button simply exits its pending state with no feedback. Fix: checkclear.isErrorand render an alert, or at least disable the button and show the error text.🪰 Gadfly · advisory
@@ -0,0 +108,4 @@)}<div className="flex min-h-0 flex-1 flex-col gap-2 overflow-y-auto">{history.isSuccess && messages.length === 0 && !pending && (🟠 History loading and error states silently swallowed
error-handling · flagged by 1 model
web/src/editor/ChatPanel.tsx:111-128— The conversation pane only renders content whenhistory.isSuccessis true. If the history query fails (history.isError) or is still loading (history.isLoading/history.isPending), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space. - Fix: Add explicit branches forhistory.isPendingandhistory.isErrorinside the scrol…🪰 Gadfly · advisory
@@ -0,0 +199,4 @@}: {role: 'user' | 'assistant'body: stringchildren?: React.ReactNode🟡 Uses React.ReactNode without importing React, unlike every other component's
import type { ReactNode }conventionmaintainability · flagged by 2 models
web/src/editor/ChatPanel.tsx:202—children?: React.ReactNodeuses theReactnamespace without importing it anywhere in the file (only{ useEffect, useRef, useState }is imported from'react'). Every other component in the codebase types children asReactNodeviaimport type { ReactNode } from 'react'. Match that convention.🪰 Gadfly · advisory
@@ -0,0 +125,4 @@credentials: 'same-origin',signal,})} catch {🔴 Abort during fetch falsely reported as network error
correctness, error-handling · flagged by 5 models
web/src/lib/agent.ts:128— Thefetchcatch block reports "Could not reach the server" for every thrown error, but it never checkssignal?.aborted. If the user clicks Stop (or the panel unmounts) while thefetchis still in flight, the intentionalAbortErroris misreported as a network failure. Thereader.read()phase later in the same function correctly guards withif (signal?.aborted) return(line 151); thefetchphase needs the same guard.🪰 Gadfly · advisory
@@ -0,0 +168,4 @@} catch {continue // a malformed frame shouldn't kill a working stream}const e = event as { step?: AgentStep; done?: AgentTurn; error?: string }🔴 Server Warning field silently dropped in SSE stream
correctness, error-handling, maintainability · flagged by 4 models
web/src/lib/agent.ts:171-174— The client parses SSE events into{ step?, done?, error? }but the server also sends awarningfield (seeinternal/api/agent.go:34-43andagent.go:90). When the turn succeeds but the transcript can't be saved, the server emits{"done": {...}, "warning": "I couldn't save this exchange..."}so the user knows the conversation will disappear on reload. The client ignoreswarningentirely, producing the exact "quiet inconsistency" the server comment…🪰 Gadfly · advisory
@@ -0,0 +181,4 @@export function useAgentRefresh(gardenId: number) {const qc = useQueryClient()return () => {void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) })🟠 useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn
performance · flagged by 2 models
web/src/lib/agent.ts:184-186—useAgentRefreshinvalidates the conversation history and change-set history on every step, but neither changes mid-turn.onStepcallsrefresh(), which invalidatesgardenFullKey,historyKey, andagentHistoryKeyall at once. I verified the server side: the conversation transcript is only written after the run completes viaRecordAgentExchange(internal/api/agent.go:87), and the change set is only committed after the run function returns vi…🪰 Gadfly · advisory
@@ -225,1 +237,3 @@return { tone: 'partial', message: `${applied} of ${total} changes undone — ${skipped}.` }// Only claim a denominator when we have one. "2 of 3" from a caller that// never knew the total would be a number invented to fill a sentence.const total = target.counts ? target.counts.reduce((sum, c) => sum + c.n, 0) : 0🟡 describeUndo duplicates totalChanges' summation logic instead of sharing a helper
maintainability · flagged by 4 models
web/src/lib/history.ts:239—describeUndoreimplements the exact summationtarget.counts.reduce((sum, c) => sum + c.n, 0)thattotalChanges(line 117-119) already does forChangeSet. SinceUndoTarget.countsandChangeSet.countsare bothChangeCount[], factor the reduce into a small shared helper (e.g.sumCounts(counts?: ChangeCount[])) and have both call it.🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 10 findings (7 with multi-model agreement)
web/src/lib/agent.ts:128web/src/lib/agent.ts:171internal/api/api.go:59web/src/lib/history.ts:239web/src/lib/agent.ts:184web/src/editor/ChatPanel.tsx:93web/src/editor/ChatPanel.tsx:2023 single-model findings (lower confidence)
web/src/editor/ChatPanel.tsx:41web/src/editor/ChatPanel.tsx:111web/src/editor/ChatPanel.tsx:27Per-model detail
claude-code/sonnet (claude-code) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Good, no dangerouslySetInnerHTML anywhere in the panel. Everything checks out server-side. My review is done.
VERDICT: No material issues found
Reviewed the diff through the security lens, with server-side verification (not just the client code):
ChatPanel'scanEditprop only hides the Undo button in the UI (web/src/editor/ChatPanel.tsx:97); the actual gate isservice.AgentHistory/RecordAgentExchange/ClearAgentHistory(internal/service/agent.go:25,38,58) andRunner.Run→svc.GetGarden/svc.WithChangeSet(internal/agent/runtime.go:99,115), all of which callrequireGardenRole(..., roleEditor). A viewer can't get the assistant to act regardless of what the client sends. Verified by reading the service layer, not assumed.GET /capabilities(internal/api/api.go:58) is intentionally unauthenticated and only returns a boolean ({"agent": bool}) — no secret, model name, or config detail leaks. Consistent with the stated design ("a dead button is worse than no button") and low-sensitivity by nature./agent/chatis a POST and sits underv1.Use(h.csrfGuard())(internal/api/api.go:54), which rejects cross-originOriginheaders for state-changing methods oncePANSY_BASE_URLis set.streamChat's hand-rolledfetch(web/src/lib/agent.ts) doesn't bypass this — the guard reads the browser-setOriginheader, not anything the client has to opt into.Bubble(web/src/editor/ChatPanel.tsx:198-215), nodangerouslySetInnerHTMLanywhere in the new files — confirmed by grep.web/src/lib/agent.ts, thecatch { continue }aroundJSON.parse), and the stream only ever comes from same-origin/api/v1/agent/chatovercredentials: 'same-origin'— not attacker-controlled input being deserialized unsafely.describeStep,TOOL_LABELS[t] ?? t.replace(/_/g, ' ')) only ever formats a string for display via JSX text, so a hypothetical malicious tool name still can't inject markup.Nothing here introduces an authn/authz gap, injection, SSRF, unsafe deserialization, or credential leak.
🎯 Correctness — Blocking issues found
Both findings are independently confirmed against the actual source. The plumbing (lazy tab rendering, unconditional auto-switch to inspector, unconditional abort-on-unmount, request-scoped context passed into
Run, and the asymmetricfetchvsreader.read()abort handling) all match the draft's claims exactly, with no guard code anywhere that would prevent the described failure.VERDICT: Blocking issues found
web/src/editor/ChatPanel.tsx:41— UnmountingChatPanel(viauseEffect(() => () => abort.current?.abort(), [])) silently aborts an in-flight agent turn.EditorRailrenders only the active tab lazily (web/src/editor/EditorRail.tsx:27,85), andGardenEditorPage.tsx:150-153unconditionally callssetRailTab('inspector')on every object/plop selection — with no guard for a pending chat turn — overriding an open'chat'tab and unmountingChatPanelmid-stream. Server-side,internal/api/agent.go:67passes the request-scopedc.Request.Context()intoh.agent.Run(internal/agent/runtime.go:90), so the aborted client fetch cancels the run before it returns aTurn, meaningRecordAgentExchange(internal/api/agent.go:87) is never reached — the turn is silently lost with no error surfaced, since the component that would display one is already gone. This directly undermines the PR's stated design premise of watching the canvas while the agent works.Fix: don't unconditionally abort on unmount, and/or guard the auto-switch-to-inspector effect while a turn is pending.
web/src/lib/agent.ts:129— Thecatcharound the initialfetch()call instreamChatunconditionally callshandlers.onError('Could not reach the server.')with nosignal?.abortedcheck, unlike thereader.read()catch a few lines later (agent.ts:151) which correctly special-casesif (signal?.aborted) return. Clicking Stop, or the unmount-abort from the first finding, while the POST/SSE handshake is still in flight throws anAbortErrorhere and surfaces a misleading "Could not reach the server." message instead of silently doing nothing, contradicting the intended behavior that aborting shouldn't report an error.Fix: check
signal?.abortedin thefetch()catch too, matching thereader.read()catch.🧹 Code cleanliness & maintainability — Minor issues
All three findings check out against the source. Details:
/capabilitiesis registered as an inline anonymous closure whilehealthz(the adjacent route) is a named package function and nearly everything else uses namedh.xxxmethods. Checkedspa.go:28—r.NoRoute(func(c *gin.Context) {...})is the only other inline closure in the package, and it's structurally different (NoRoute has no equivalent grouped-route pattern to match).describeUndodoestarget.counts.reduce((sum, c) => sum + c.n, 0)(guarded bytarget.counts ?), which is the identical reducetotalChanges(line 117-119) performs onChangeSet.counts. Both operate onChangeCount[], so a sharedsumCounts(counts?: ChangeCount[])helper is a legitimate dedup opportunity.{ useEffect, useRef, useState }is imported from'react';React.ReactNodeat line 202 relies on the globalReactnamespace ambient-declared by@types/react(export as namespace React), which compiles fine underjsx: "react-jsx"but is inconsistent with every other component in the codebase (EditorRail.tsx,PageStub.tsx,Alert.tsx,Modal.tsx,AuthCard.tsx,Divider.tsx,EditorHint.tsx— all grep-confirmed), which importReactNodeexplicitly viaimport type { ReactNode } from 'react'.All three are real, minor, verified.
VERDICT: Minor issues
internal/api/api.go:59-61— The new/capabilitieshandler is a bare anonymous closure registered inline inNew, while every other route in this file (includinghealthzright next to it) is a named handler function orh.method. Pullcapabilitiesout into a named function or method onhandlersto match the established convention.web/src/lib/history.ts:239—describeUndoreimplements the exact summationtarget.counts.reduce((sum, c) => sum + c.n, 0)thattotalChanges(line 117-119) already does forChangeSet. SinceUndoTarget.countsandChangeSet.countsare bothChangeCount[], factor the reduce into a small shared helper (e.g.sumCounts(counts?: ChangeCount[])) and have both call it.web/src/editor/ChatPanel.tsx:202—children?: React.ReactNodeuses theReactnamespace without importing it anywhere in the file (only{ useEffect, useRef, useState }is imported from'react'). Every other component in the codebase types children asReactNodeviaimport type { ReactNode } from 'react'. Match that convention.⚡ Performance — Minor issues
Everything checks out exactly as claimed. All three sub-claims verified:
refresh()invalidates all three keys per step (agent.ts:181-187), invoked inonStep(ChatPanel.tsx:62-67),WithChangeSetonly commits history afterfnreturns (revisions.go:114-141), andRecordAgentExchangeis called only once afterh.agent.Runreturns (agent.go:67-93).VERDICT: Minor issues
web/src/lib/agent.ts:181-187(useAgentRefresh), invoked fromweb/src/editor/ChatPanel.tsx:62-67on everyonStep—refresh()invalidates three query keys per step (gardenFullKey,historyKey,agentHistoryKey), but two of the three cannot have changed mid-turn, so refetching them per step is pure waste.The change-set history entry is only written once the whole turn finishes:
internal/service/revisions.go:114-141(WithChangeSet) buffers revisions and callscommitScope(which theGET /gardens/:id/historyread reflects) only afterfn— the entire agent run — returns. The agent conversation messages are likewise written only after the turn:internal/api/agent.go:87-92callsRecordAgentExchangeonly once, afterh.agent.Run(...)returns, not per step. MeanwhilegardenFullKeyrefetching per step is legitimate, since garden mutations commit immediately per tool call.So for a turn with N steps, this fires N redundant
/gardens/:id/historyfetches and N redundant/gardens/:id/agent/historyfetches guaranteed to return unchanged data, on top of the (justified) Ngarden-fullrefetches. Low severity — wasted round trips and re-renders, not wrong behavior — but it scales with step count.Fix: only invalidate
gardenFullKeyin theonStephandler; keephistoryKey/agentHistoryKeyinvalidation inonDone/onError, whererefresh()is already called (ChatPanel.tsx:70,79).🧯 Error handling & edge cases — Minor issues
Verified Review
VERDICT: Minor issues
web/src/lib/agent.ts:119-131— the initialfetch()call'scatchblock unconditionally callshandlers.onError('Could not reach the server.'), without checkingsignal?.abortedthe way the laterreader.read()catch does (agent.ts:148-153, which correctly returns silently on abort). Confirmed both call sites that can trigger an abort before headers arrive:ChatPanel.tsx:41(useEffectcleanup,abort.current?.abort()on unmount) andChatPanel.tsx:177-181(Stop button). The server only flushes SSE headers after loading agent history and callingopenEventStream(internal/api/agent.go:59,65,101-105), so an abort issued before that point makesfetch()reject withAbortError, hits this unguarded catch, and surfaces a false "Could not reach the server." message — most visibly when the user clicks Stop, since the panel is still mounted to show it. Fix: mirror theif (signal?.aborted) returnguard used a few lines down.web/src/editor/ChatPanel.tsx:27,86,111— confirmedhistory.isErroris never referenced anywhere in the file (onlyhistory.dataandhistory.isSuccessare used). IfGET /gardens/:id/agent/historyfails,messagesfalls back to[]viahistory.data ?? [](line 86), and sinceisSuccessstays false the placeholder "Ask for what you want…" text (gated onisSuccess, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state, and no error is surfaced to the user.web/src/editor/ChatPanel.tsx:95— confirmedonClick={() => clear.mutate()}passes noonErrorcallback, anduseClearAgentHistory(agent.ts:53-61) only wiresonSuccess. If the DELETE fails,clear.isPendingreturns to false and the button simply re-enables with no feedback that "Start over" didn't actually clear anything.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
Through the security lens, I verified the new surface and the existing guards it relies on:
/capabilities(internal/api/api.go:59) is registered onv1beforerequireAuth, so it is unauthenticated. I confirmed it returns only{"agent": cfg.Agent.Ready()}and thatReady()(internal/config/config.go:82) is a boolean ofEnabled && key != "" && model != ""— it does not leak the API key or model spec. This is intentional info disclosure (a single readiness bit) and matches the DESIGN.md note. Trivial at most; no secret leakage.agentChat,getAgentHistory,deleteAgentHistoryare all underh.requireAuth()(internal/api/api.go:144-147), and the service layer re-checksroleEditor(internal/service/agent.go:25,38,58). A viewer opening the Assistant tab (the tab isn't gated bycanEditin GardenEditorPage) will be refused at the service layer — no privilege bypass. (UX, not security.)streamChat's rawfetchPOST is same-origin (API_BASE='/api/v1',credentials:'same-origin') and inheritscsrfGuard(internal/api/auth.go:136), which rejects cross-origin state-changing requests whenPANSY_BASE_URLis set. Consistent with the rest of the API.ChatPanelrenders all message bodies anddescribeStepoutput as React text ({body},{describeStep(s)}), nodangerouslySetInnerHTML. Streamerrorstrings are server-authored and rendered as text. No untrusted-data interpolation into HTML, URLs, or SQL on this side.No blocking or material security issues in this change.
🎯 Correctness — No material issues found
Verdict: No material issues found
Through the correctness lens I verified the load-bearing logic:
describeUndo/UndoTargetchange (web/src/lib/history.ts:174,208,224-241): The newtotal = target.counts ? target.counts.reduce(...) : 0reproducestotalChangesexactly (read athistory.ts:117-119), so the HistoryPanel path (which passes a fullChangeSetwithcounts) keeps producing "N of M changes undone". When the chat panel passes{ id }with nocounts,total=0→ "Partly undone" — no invented denominator. Theapplied === 0early-return at line 234 means the zero-denominator branch is only reached when something was actually applied, so the message is never misleading. Verified the test expectations athistory.test.ts:116-141match this behavior.web/src/editor/HistoryPanel.tsx:77,117):ChangeSetis structurally assignable toUndoTarget(id: number,counts: ChangeCount[]satisfiescounts?: ChangeCount[]), andtotalChanges(changeSet) > 0at line 115 gates the button so the empty-counts edge case is unreachable from the history list. No behavior change there.describeStep(web/src/lib/agent.ts):new Set(labels)dedups repeats; unknown-tool fallbackt.replace(/_/g, ' ')yields "prune orchard" from "prune_orchard". Tool labels match the server-defined tool names (internal/agent/tools.go:21-61). Verified.streamChat): server emitsdata: %s\n\n(internal/api/agent.go:113); client splits on\n\n, takes thedata:line,slice(5).trim(). JSON produced by the server contains no literal newlines, so single-line frames parse correctly. Malformed frames are skipped without aborting. Aborted reads return silently. Verified./capabilities(internal/api/api.go,internal/config/config.go:82-84):Ready()requiresEnabled && key != "" && Model != "", matching the "agent 404s when unconfigured" premise.onError/onDone/Stop all clearpendingand refresh; the unmount effect aborts. No state left dangling.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/lib/history.ts:239—describeUndore-implements thecountssummation inline (target.counts.reduce((sum, c) => sum + c.n, 0)), byte-for-byte the same reduction already factored out astotalChangesathistory.ts:117-119. The only reason it isn't reused is thattotalChangesis typed to take a fullChangeSetrather than just{ counts }(or anUndoTarget). This is duplicated logic in the one module that exists to share undo behavior; a future change to how counts are summed would have to be made in two places. Suggested fix: widentotalChangesto acceptPick<ChangeSet, 'counts'>(or anUndoTargetwith counts), or extract asumCounts(counts: ChangeCount[]): numberhelper that both call. Verified by reading both functions.internal/api/api.go:59-61— the/capabilitieshandler is an inline anonymous closure, while every other route in this file dispatches to a method onhandlers(e.g.h.register,h.me,h.listGardens). The comment justifies the route's existence, not the style break. For consistency with the surrounding "handlers stay thin" pattern (and the file's own doc comment), this should be ah.capabilities(c *gin.Context)method. Trivial, but it's the one route that doesn't follow the file's own structure.web/src/editor/ChatPanel.tsx:202—children?: React.ReactNodereaches for the globalReactnamespace while the rest of the file (and the imports at the top) use named imports from'react'(useEffect,useRef,useState). It compiles (via@types/react'sexport as namespace React), but it's inconsistent with the file's own import style;import type { ReactNode } from 'react'would match. Trivial.⚡ Performance — Minor issues
The finding is confirmed against the actual code:
useAgentRefresh(agent.ts:181-187) invalidates all three keys together.onStepin ChatPanel.tsx callsrefresh(), so all three are invalidated per step.RecordAgentExchange(agent.go:87) runs only afterh.agent.Runreturns — conversation history is not written mid-turn.commitScope(revisions.go:141/156-168) writes the change set only after the run'sfnreturns — change-set history is not committed mid-turn.gardenFullKey) does mutate per step (tools write rows immediately; only the revisions are buffered), so invalidating it per step is intended.So invalidating
agentHistoryKeyandhistoryKeyon every step is redundant until the turn completes.Verdict: Minor issues
web/src/lib/agent.ts:184-186—useAgentRefreshinvalidates the conversation history and change-set history on every step, but neither changes mid-turn.onStepcallsrefresh(), which invalidatesgardenFullKey,historyKey, andagentHistoryKeyall at once. I verified the server side: the conversation transcript is only written after the run completes viaRecordAgentExchange(internal/api/agent.go:87), and the change set is only committed after the run function returns viacommitScope(internal/service/revisions.go:141) — neither produces new rows during the step stream. The canvas data (gardenFullKey) does change per step as tools write rows immediately, so invalidating it per step is the intended "watch it work" behavior. But for a ~12-step turn, this fires ~11 redundant refetches of/gardens/:id/agent/historyand ~11 of/gardens/:id/history, each a network round-trip plus DB queries returning identical data. Suggested fix: split the refresh — invalidategardenFullKeyinonStep, and invalidateagentHistoryKey/historyKeyonly inonDone/onError(where they already callrefresh()). TanStack Query cancels overlapping refetches so this isn't catastrophic, but it's still ~22 wasted requests per turn on the feature's headline path. (Verified by readinginternal/api/agent.go,internal/service/revisions.go,internal/agent/runtime.go, andinternal/service/agent.go.)🧯 Error handling & edge cases — Minor issues
Both findings are confirmed against the actual source.
Finding 1 verified:
agent.ts:128-131— thefetchcatchblock unconditionally callshandlers.onError('Could not reach the server.')with nosignal?.abortedcheck, unlike the read-loopcatchat lines 148-153 which does check. The Stop handler (ChatPanel.tsx:177-178) and the unmount cleanup (ChatPanel.tsx:41) both callabort.current?.abort(), so an abort during the pending POST is misreported as a network failure.Finding 2 verified:
internal/api/agent.go:43definesWarning stringonchatEvent, and line 90 sendschatEvent{Done: turn, Warning: "I couldn't save this exchange…"}. The client'sAgentTurn(agent.ts:64-69) has nowarningfield, and the dispatch atagent.ts:171-174only checkserror/step/done, so the warning is dropped.VERDICT: Minor issues
web/src/lib/agent.ts:128— intentional abort during the initialfetchis misreported as a network failure. The read-loopcatch(line 148) correctly checksif (signal?.aborted) returnand stays silent, but thefetchcatchat line 128 does not. When the user clicks Stop (ChatPanel.tsx:177-178callsabort.current?.abort()) before the response headers arrive — i.e. while the initial POST is still pending —fetchrejects with anAbortError, thiscatchruns unconditionally, andhandlers.onError('Could not reach the server.')fires. TheonErrorhandler then doessetPending(null); setError('Could not reach the server.'); refresh(), so a user-initiated cancellation is shown to the user as "Could not reach the server." — the exact "navigating away isn't a failure" case the code claims to handle, just one layer earlier. The same path is hit on unmount (useEffectcleanup atChatPanel.tsx:41aborts), causing a state update after unmount. Fix: mirror the read-loop check, e.g.catch { if (signal?.aborted) return; handlers.onError('Could not reach the server.') }.web/src/lib/agent.ts:171-174— adoneevent carrying a serverwarningis silently dropped. The server'schatEvent(internal/api/agent.go:33-44) can sendWarningalongsideDone(e.g. "I couldn't save this exchange, so it won't be here after a reload…", sent atagent.go:90). The client'sAgentTurntype (agent.ts:64-69) has nowarningfield, and the event dispatch (agent.ts:171-174) only inspectserror/step/done. So when the transcript save fails, the user gets a clean "done" with no indication that the exchange won't persist after reload — the server explicitly designed that warning to avoid "the quieter lie." TheonDonehandler inChatPanel.tsx:68-74only surfacestruncated. Fix: addwarning?: stringtoAgentTurnand the event shape, and surface it (e.g.setError/anAlert) inonDone.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
🎯 Correctness — Blocking issues found
Blocking issues found
web/src/lib/agent.ts:128— Thefetchcatch block reports "Could not reach the server" for every thrown error, but it never checkssignal?.aborted. If the user clicks Stop (or the panel unmounts) while thefetchis still in flight, the intentionalAbortErroris misreported as a network failure. Thereader.read()phase later in the same function correctly guards withif (signal?.aborted) return(line 151); thefetchphase needs the same guard.web/src/lib/agent.ts:171-174— The client parses SSE events into{ step?, done?, error? }but the server also sends awarningfield (seeinternal/api/agent.go:34-43andagent.go:90). When the turn succeeds but the transcript can't be saved, the server emits{"done": {...}, "warning": "I couldn't save this exchange..."}so the user knows the conversation will disappear on reload. The client ignoreswarningentirely, producing the exact "quiet inconsistency" the server comment explicitly warns against.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/lib/agent.ts:171— The streaming response events use a rawascast (const e = event as { step?: AgentStep; … }) afterJSON.parse. Every other external payload in this module validates with zod (capabilitiesSchema,agentMessageSchema,historySchema), and the file even importszfor that purpose. Skipping validation here breaks the module's own pattern; it also means only JSON-parse failures are skipped, while structurally malformed frames can still propagate to handlers.Fix: Add a small zod schema for the SSE event envelope and
safeParseit in the frame loop, skipping frames that don't match.web/src/lib/history.ts:239—describeUndomanually reimplements the summation thattotalChangesalready performs (target.counts?.reduce((sum, c) => sum + c.n, 0)). TheUndoTargettype was introduced precisely so callers don't need to fabricate counts; wideningtotalChangesto acceptUndoTargetwould remove the duplication and keep the tally logic in one place.Fix: Change
totalChangesparameter fromChangeSettoUndoTarget(with optional chaining / nullish coalescing for the optionalcounts) and call it fromdescribeUndo.⚡ Performance — No material issues found
No material issues found
web/src/editor/ChatPanel.tsx:44-46—scrollIntoView({ behavior: 'smooth' })fires on everypendingchange, which happens on every agent step. A replant makes "a dozen calls over tens of seconds" (per the PR description), so a single turn triggers ~12 overlapping smooth-scroll animations. On lower-end devices this causes frame drops and visual jank. Fix: use'auto'instead of'smooth'for mid-turn updates (or debounce the scroll), reserving smooth scrolling only when the turn completes.web/src/lib/agent.ts:143-161— The SSE buffer (buffer += decoder.decode(...)) has no size ceiling. If the server sends data without a terminating\n\n(buggy proxy, malformed stream, or deliberate abuse),buffergrows unbounded until the tab runs out of memory. Fix: cap the buffer (e.g., at 1 MB) and discard/abort if it exceeds the limit without producing a complete frame.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/lib/agent.ts:129— Thefetchcatch block reports "Could not reach the server." for every error, including an intentionalAbortErrorwhen the user clicks Stop. Thereader.read()catch two lines later correctly checkssignal?.abortedand returns silently, but the initialfetchcatch omits that guard. If the user aborts while the request is still opening, they get a false network-error message instead of a clean cancellation.if (signal?.aborted) returnbeforehandlers.onError(...)in thefetchcatch.web/src/editor/ChatPanel.tsx:111-128— The conversation pane only renders content whenhistory.isSuccessis true. If the history query fails (history.isError) or is still loading (history.isLoading/history.isPending), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space.history.isPendingandhistory.isErrorinside the scroll container, e.g. a "Loading history…" / "Could not load conversation" message.opencode/glm-5.2:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've examined the diff and verified the security-relevant paths against the checked-out code. Let me trace the key authn/authz and input-validation paths.
Verified:
/capabilities(api.go:59) is unauthenticated but only reveals{"agent": bool}— equivalent to the existing/auth/providerspattern, no sensitive data.POST /agent/chat(agent.go:51) callsh.svc.AgentHistorywhich requiresroleEditor(service/agent.go:25) before opening the SSE stream — a viewer is rejected before any work runs.gardenIdfrom the body is checked against the actor's role, so no IDOR.GET/DELETE /gardens/:id/agent/historyroute throughAgentHistory/ClearAgentHistory, bothroleEditor-gated (service/agent.go:24, 57).csrfGuard(auth.go:136) applies to the v1 group including/agent/chat; the client'sfetchusescredentials: 'same-origin'and browsers emit anOriginheader for same-origin POSTs, so the guard passes legitimately.m.body) and step labels render as React text children insideBubble/<li>/<Alert>— auto-escaped, nodangerouslySetInnerHTML. No XSS vector from model output or error strings.unknownthen narrowed by field; noeval/template injection. Server is trusted; no zod validation is a robustness nit, not a security hole.RecordAgentExchangeusescontext.WithoutCancelso a client aborting mid-stream can't suppress the persisted exchange/change-set — the audit trail survives.One non-security observation (outside my lens): viewers see the Assistant tab and an enabled Send box (GardenEditorPage pushes the tab on
capabilities.data?.agentalone, andChatPanelonly disables input onpending, not!canEdit). The server correctly rejects them, so this is UX, not an authz bypass.VERDICT: No material issues found
🎯 Correctness — Minor issues
VERDICT: Minor issues
web/src/lib/agent.ts:171— server'swarningevent field is silently dropped. The server'schatEvent(internal/api/agent.go:43) carries aWarningstring, and atinternal/api/agent.go:90it is sent alongsideDonewhenRecordAgentExchangefails — deliberately, per the comment, to avoid "a quiet inconsistency" where a reload silently forgets the exchange. The client'sstreamChatparses each event as{ step?; done?; error? }(agent.ts:171) with nowarningfield, andAgentTurn(agent.ts:64-69) has nowarningmember.onDoneinChatPanel.tsxonly checksturn.truncated. Thewarningis never surfaced, so the user is told nothing despite the server having gone out of its way to say something. Fix: addwarning?: stringtoAgentTurn, read it instreamChat, and surface it inonDone(e.g.setError(turn.warning)alongside the truncated check).internal/api/api.go:59-61—/capabilitiescan advertise the agent while the chat route is absent. The endpoint reportscfg.Agent.Ready(), but the/agent/chatroute is only registered whenagent.NewRunneralso succeeds (api.go:134-150: routes live inside theelseof the NewRunner error branch). In the case whereReady()is true butNewRunnerfails (an unresolvable model spec, which the code explicitly accounts for atapi.go:137-141),/capabilitiesreturns{agent: true}, the client renders the Assistant tab, and the first POST 404s — surfacing as "This instance doesn't have the assistant configured," which is misleading (it is configured, just broken). The stated purpose of the endpoint (api.go:56-58) is to avoid exactly this dead-button case. Fix: gate the reported value onh.agent != nil(move the handler registration after the agent block, or capture readiness in the closure).🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual code.
history.ts:239doestarget.counts.reduce((sum, c) => sum + c.n, 0);totalChangesat line 117-118 doescs.counts.reduce((sum, c) => sum + c.n, 0). Identical reduction logic, confirmed.api.go:59registers/capabilitiesas an inline closure readingcfgfrom the enclosing scope. Every other route in the package (confirmed via grep acrossinternal/api) registers a named function (healthzpackage-level, orh.xxxmethods). Confirmed.VERDICT: Minor issues
web/src/lib/history.ts:239— duplicated change tally.describeUndonow doestarget.counts.reduce((sum, c) => sum + c.n, 0)inline, which is the exact computationtotalChanges(line 117-118) already owns. The only reason it can't reuse it is thattotalChangestakes aChangeSetwhile the new target isUndoTargetwith optional counts. ExtractsumCounts(counts: ChangeCount[]): numberand have both call it, so the count is computed one way. (Verified by reading both call sites.)internal/api/api.go:59— inline closure is the only handler in the package not following the named-handler convention. Every other route registers a named function (healthzpackage-level, orfunc (h *handlers) xxx). The closure also readscfgfrom the enclosing scope rather thanh.cfg. Suggested fix: addfunc (h *handlers) capabilities(c *gin.Context)and registerh.capabilities, matching the surrounding code and keeping the handler testable in isolation. (Verified via grep — every otherv1.GET/POST/PATCH/DELETEin the package uses a named function.)⚡ Performance — No material issues found
VERDICT: No material issues found
refresh()call inChatPanel.tsx:62-67for a hot-path regression.useAgentRefreshinvalidates three queries (gardenFullKey,historyKey,agentHistoryKey) on everyonStep, and a replant can fire a dozen steps over tens of seconds. The/gardens/:id/fullpayload is the whole garden, so naively this is a dozen full refetches. However: TanStack Query dedupes concurrent same-key refetches, and within a single SSE chunk thefor (const frame of frames)loop atagent.ts:162processes all frames synchronously, so multiple steps in one chunk coalesce into one scheduled refetch (react-query batches refetches queued in the same tick). Across chunks arriving milliseconds apart you get roughly one refetch per chunk, which is exactly the "watch the canvas update as it works" intent. Not a regression.pending.stepsuses[...p.steps, step](ChatPanel.tsx:63), an O(n) copy per step, but n is bounded by a single turn (~dozens). Not material.streamChat'sbuffer.split('\n\n')+frame.split('\n').find(...)(agent.ts:160-163) allocates per chunk, but frames are small and the stream is short-lived. Not material.scrollIntoView({ behavior: 'smooth' })re-fires on every step via the[history.data, pending]effect (ChatPanel.tsx:44-46); repeated smooth-scroll calls can queue against each other and cause minor jank, but this is cosmetic, not a performance regression worth flagging.Verified by reading
web/src/lib/agent.ts,web/src/editor/ChatPanel.tsx, andweb/src/lib/objects.ts(forgardenFullKeyand the defaultstaleTimeinqueryClient.ts).🧯 Error handling & edge cases — Blocking issues found
Both findings confirmed against the actual code. Let me verify the ChatPanel Stop handler.
Both findings verified against the actual code.
Finding 1 (fetch abort):
agent.ts:128-130— the fetchcatchblock callshandlers.onError('Could not reach the server.')without checkingsignal?.aborted. The read-side catch at line 148-151 correctly guards withif (signal?.aborted) return. ChatPanel Stop button (line 178) and unmount effect (line 41) both callabort(), so user-initiated aborts surface as spurious errors. Confirmed.Finding 2 (warning field dropped):
agent.go:43definesWarning stringinchatEvent, andagent.go:90emits it when the exchange couldn't be persisted.agent.ts:171only destructures{ step?, done?, error? }— nowarning. TheAgentTurninterface (agent.ts:64-69) has nowarningfield. Confirmed.Review
VERDICT: Blocking issues found
web/src/lib/agent.ts:128-130— fetch abort reports a spurious error to the user. Thecatcharoundfetchdoesn't checksignal?.abortedbefore callinghandlers.onError('Could not reach the server.'). When the user clicks Stop (ChatPanel.tsx:178) or the panel unmounts (ChatPanel.tsx:41), both of which callabort.current?.abort(), the in-flightfetchrejects with anAbortErrorand the panel shows "Could not reach the server." The read-sidecatchat lines 148-151 guards the abort case correctly ("an aborted read is the caller navigating away, not a failure worth reporting"), but the fetch-side catch is inconsistent with it and reintroduces exactly the spurious error the comment two lines below claims to avoid.Fix: in the fetch
catch, checkif (signal?.aborted) returnbefore callingonError, mirroring the read-side guard:web/src/lib/agent.ts:171-174— the server'swarningfield is silently dropped, hiding a partial-failure message. The server'schatEventcarries aWarningstring that rides alongsideDone(internal/api/agent.go:41-43), emitted when the turn succeeded but the exchange couldn't be persisted: "I couldn't save this exchange, so it won't be here after a reload. Anything I changed is still on the canvas, and in History." (agent.go:90). The client's event handler only destructures{ step?, done?, error? }and never readswarning, so this deliberately-surfaced partial-failure message never reaches the user. The user reloads to check whether the change landed, finds the conversation gone, and the panel never told them why. Verified the server emits it and the client'sAgentTurninterface (agent.ts:64-69) has nowarningfield either.Fix: add
warning?: stringto the event type and surface it — e.g. set it as the error/banner string on done, or extendAgentTurn/onDoneto carry it:opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
Security lens review:
AuthZ on agent endpoints: Verified.
agentChatcallsAgentHistoryfirst, which gates onroleEditorviarequireGardenRole. The agent tools themselves (internal/agent/tools.go) funnel through service methods that enforce garden-level permissions. Viewers cannot reach the run loop, and the frontend'scanEditgating is consistent with the backend.CSRF: The new
POST /agent/chatroute sits under/api/v1wherecsrfGuard()is applied. ThestreamChatclient (web/src/lib/agent.ts:121) sendscredentials: 'same-origin', so the session cookie is included, and the existing Origin-header check will reject cross-origin state-changing requests whenPANSY_BASE_URLis configured.Injection / XSS: User and LLM text is rendered via React JSX (
{body},{error},{describeStep(...)}), which escapes markup automatically. NodangerouslySetInnerHTMLis introduced. SSE events are JSON-marshaled on the server before being written, so no newline injection into the SSE wire format is possible.Info leak via
/capabilities: The endpoint returns only a booleanagentflag. This same fact was already discoverable by probing/agent/chatfor a 404, so the dedicated endpoint does not leak new information.Undo button security: The
changeSetIdused for undo comes from the authenticated API response. TherevertChangeSethandler resolves the garden from the change set and enforcesroleEditor. A viewer (canEdit=false) sees no undo button, and the backend would reject any revert attempt anyway.Outside my lens: None.
🎯 Correctness — Minor issues
Corrected review
Verdict: Minor issues
/capabilitiescan falsely advertise a working assistant (internal/api/api.go:59-61). The endpoint returnscfg.Agent.Ready(), but the actual chat routes are only registered ifagent.NewRunnersucceeds. If the model spec is unresolvable (the exact failure case noted at line 137),Ready()is still true yet POST/agent/chat404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.Client silently drops server-sent
warningevents (web/src/lib/agent.ts:171-174). The server'schatEventcarries aWarningfield (e.g., whenRecordAgentExchangefails: "I couldn't save this exchange, so it won't be here after a reload" —internal/api/agent.go:34-44,90). The client's SSE parser only looks atstep,done, anderror; thewarningis ignored. The user sees a normal "Done" turn with no indication that the conversation will disappear on reload.🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run returned no output
⚡ Performance — No material issues found
No material issues found
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/lib/agent.ts:128-129— Abort during fetch misreported as network failure.The
fetchcatch block callshandlers.onError('Could not reach the server.')without checkingsignal.aborted. If the user clicks Stop before the request finishes (or the component unmounts), theAbortErroris reported as a server failure, andsetErrorfires on an unmounting component.Fix: check
signal?.abortedand return silently before invokingonError, matching thereader.read()catch at line 151.web/src/lib/agent.ts:171-174— Serverwarningfield swallowed.When
RecordAgentExchangefails, the server emits{\"done\": {...}, \"warning\": \"I couldn't save this exchange...\"}so the user knows the turn worked but the transcript won't persist. The client parses{ step?, done?, error? }and ignoreswarning; the user sees a clean completion.Fix: include
warning?: stringin the parsed event shape and surface it toonDone(e.g., append it to the reply or callonError).web/src/editor/ChatPanel.tsx:93-100— "Start over" errors are silent.useClearAgentHistoryhas noonErrorhandler, and the component never readsclear.isError. If the DELETE fails, the button simply exits its pending state with no feedback.Fix: check
clear.isErrorand render an alert, or at least disable the button and show the error text.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
All fixed in
f1c9e2b. Two of these are mine in an embarrassing way.I left the
warningfield unwiredTwo commits ago I added a server-side
warning— specifically because a failed save was being swallowed and the user deserved to know. The client never read it. So "I couldn't save this exchange" went nowhere, recreating the exact silence the warning existed to break.Wired now. And SSE frames are validated with zod rather than type-asserted, so the next field added server-side fails loudly instead of vanishing the same way.
Clicking the canvas killed the turn
Unmount aborted the in-flight stream — and selecting an object auto-switches the rail to the inspector, which unmounts the chat. So clicking the canvas mid-turn silently killed the turn, while the canvas is exactly what you're meant to be watching. A design bug I introduced by connecting two of my own decisions without noticing they met.
Unmount no longer aborts. The request continues, the exchange persists server-side, and returning to the tab shows it. Stop still aborts, but only our read — the turn keeps running server-side either way, which is why its work lands in History regardless. That's now stated in the code, because "Stop" implying cancellation would be its own small lie.
The rest
fetchreported "Could not reach the server." (5 models). The read loop already knew an abort was the caller's own doing; the request path didn't./capabilitiesreportedcfg.Agent.Ready()while the routes requireNewRunnerto have succeeded — so a configured-but-unresolvable model would advertise a chat tab whose first message 404s. It reports the runner's real state now, as a named handler like everything else in that file.describeUndoreusestotalChanges;React.ReactNode→ importedReactNode.86 tests,
tsc --noEmit,npm run build,go test ./...,go vet,gofmtgreen.Separately, while verifying the live deploy I found the assistant isn't coming up on
pansy.orgrimmar.dudenhoeffer.casa— the agent routes 404 while every other route from the same build answers, so the binary is current andAgent.Ready()is false. Details in a comment on #58; this PR adds a startup log line naming which condition failed, because working that out currently means reading the source.