P3 [DEREM-1] On chatloop retry, the client accumulates duplicate advisor deltas with no reset signal.

RunAdvisor sets PublishMessagePart to forward text deltas via OnAdviceDelta, but never sets OnRetry on the chatloop options. When chatretry.Retry fires (rate limit, provider error), result is reset server-side (chatloop.go:465) and the step function re-runs, but no signal reaches the client. The second attempt's deltas append to the first attempt's accumulated text, producing garbled output (e.g. "Use small s...Use small steps.") in the advisor card during the retry window.

"The final tool result overwrites the accumulated text when it arrives, because mergeStreamPayload takes the value path when part.result is defined. So this is a transient visual glitch, not data corruption. But during the retry window (which includes a backoff delay), the user stares at garbled advisor text." (Hisoka)

Fix: wire OnRetry in RunAdvisor to either publish a reset event the client can use to clear accumulated delta text, or track and reset accumulated state inside the PublishMessagePart closure. (Hisoka)

🤖

P3 [DEREM-2] No test exercises the error-after-partial-streaming-deltas path.

If the advisor streams text deltas then the provider errors, the frontend receives result_delta events followed by a final tool-result with is_error: true. The backend path (partial OnAdviceDelta calls followed by chatloop.Run returning an error, which becomes a ResultTypeError JSON result) and the frontend transition (mergeStreamPayload overrides accumulated deltas, isStreaming clears) are both correct by design but neither has a test. A regression in value-override-delta precedence or isStreaming cleanup would be invisible.

"A regression in the value-override-delta precedence or the isStreaming cleanup would cause partial advice text to render alongside the error UI." (Chopper)

A sibling test that streams partial deltas then delivers an error result (both here and in runner_test.go) would close this gap. (Chopper)

🤖

P4 [DEREM-3] isStreaming is only cleared when part.result is set; a bare is_error: true part without result would leave the tool stuck in "running" status.

"A tool-result part with is_error: true but no result and no result_delta, arriving after prior deltas set isStreaming: true, would leave the tool in 'running' status forever." (Mafuuu)

No current tool hits this (the advisor always sends a final result alongside errors), so this is about future-proofing. If a future streaming tool terminates with a bare error part, this will surface. Clearing isStreaming when part.is_error is true would close the gap. (Mafuuu)

🤖

P4 [DEREM-4] Variadic opts ...RunAdvisorOptions silently discards all but the first element.

Single internal caller today (tool.go), standard Go optional-parameter pattern, and the practical risk is near zero. Four reviewers flagged it independently; the concern is that the variadic signature implies composability that doesn't exist.

A *RunAdvisorOptions pointer would make the single-config contract explicit and cause a compile error on misuse: func (rt *Runtime) RunAdvisor(ctx, question, snapshot, opts *RunAdvisorOptions), nil check instead of length check. (Kite P3, Pariston Note, Ryosuke Note, Knov Note)

🤖

Nit [DEREM-5] RunningWithStreamedAdvice asserts streamed text is visible and the empty-advice fallback is absent, but doesn't assert the initial placeholder ("Reviewing context and preparing guidance.") disappears.

The component uses a ternary so both can't render simultaneously today, but a refactor that splits the ternary into independent conditionals would pass the story while rendering both. Adding expect(canvas.queryByText("Reviewing context and preparing guidance.")).not.toBeInTheDocument() would guard the transition. (Bisky)

🤖

Nit [DEREM-7] isStreaming on StreamToolResult is undocumented. Its semantics are non-obvious: the field distinguishes "accumulating deltas" from "final result received." Other boolean fields like isError are self-explanatory; this one requires knowing that result entries can exist in a pre-completion state.

Suggestion: /** True while result deltas are still accumulating before the final result. */ (Gon)

🤖

Nit [DEREM-8] RunAdvisorOptions configures one advisor invocation. doesn't mention streaming or callbacks, which is the struct's sole purpose.

Suggestion:

// RunAdvisorOptions carries optional streaming callbacks for a
// single RunAdvisor invocation.

(Leorio)

🤖

Nit [DEREM-9] The conditional spread ...(isStreaming ? { isStreaming: true } : {}) relies on the entire object being reconstructed from scratch each time so the old isStreaming: true doesn't survive. A future refactor that merges instead of replaces would silently reintroduce the flag. The test at streamState.test.ts:255 guards this with toBeUndefined(), so it's safe today.

Six reviewers independently noted this pattern. An explicit isStreaming: isStreaming || undefined would make the intent clearer. (Kite Nit, Meruem Note, Nami Note, Pariston Note, Knov Note, Ryosuke Note)

🤖

P3 [DEREM-10] No test for PublishAdviceReset wiring at the tool layer.

TestAdvisorToolPublishesAdviceDeltasWithToolCallID proves call.ID is threaded through the PublishAdviceDelta closure (lines 60-62). This structurally identical PublishAdviceReset closure has no corresponding test. If someone removes the call.ID binding here, the chatd.go guard (toolCallID == "") silently suppresses the reset event, and the frontend retains stale accumulated text on retry. The runner test (TestAdvisorRunResetsAdviceDeltasOnRetry) exercises OnAdviceReset directly but does not test the tool layer's call.ID binding.

Sketch: mirror TestAdvisorToolPublishesAdviceDeltasWithToolCallID in tool_test.go but use a StreamFn that returns a 429 on the first call and succeeds on the second. Wire both PublishAdviceDelta and PublishAdviceReset into the tool options. Assert the reset callback was called with "call-1" between the stale and fresh delta callbacks. (Bisky)

🤖