TUI: add FollowupBehavior setting to choose between steer and follow-up dispatch#2477
TUI: add FollowupBehavior setting to choose between steer and follow-up dispatch#2477trungutt wants to merge 4 commits intodocker:mainfrom
Conversation
Instead of holding messages in a client-side queue and replaying them as separate turns after the stream stops, the TUI now calls runtime.Steer() to inject them mid-turn. The agent sees steered messages at the next tool-round boundary via <system-reminder> tags, enabling real-time course corrections without waiting for the full turn to complete.
Address review feedback: pass attachments through App.Steer so images/PDFs are not silently dropped, add ClearSteerQueue to the Runtime interface so Ctrl+X actually drains the runtime queue, and drain orphaned steer messages at the start of RunStream to prevent cross-run leakage on cancellation.
PR docker#2405 made the TUI unconditionally use runtime.Steer for messages sent while the agent is working, injecting them mid-turn via <system-reminder> at the next tool-round boundary. Some users prefer the previous follow-up semantics where each queued message runs as its own undivided turn after the current one completes. Add a user-config setting (Settings.FollowupBehavior, "steer" | "followup", default "steer") that selects between the two modes. The runtime already supports both paths (Steer / FollowUp); only the TUI side needed rewiring. A new /followup-behavior slash command toggles and persists it at runtime, mirroring the existing /split-diff pattern. Changes: - userconfig: add FollowupBehavior field + GetFollowupBehavior() normalizer + constants. Default is "steer". - runtime: add ClearFollowUpQueue symmetric to ClearSteerQueue on the Runtime interface, LocalRuntime (drains the in-memory queue), and RemoteRuntime (no-op, matching the steer no-op). - app: add App.FollowUp and App.ClearFollowUpQueue mirroring their steer counterparts. Extract buildQueuedMessage so both paths share the same attachment handling (files, images/PDFs, pasted inline text). - TUI chat: branch handleSendMsg on the configured behavior; clear both runtime queues in handleClearQueue so a mode switch can't leak messages. - TUI tui.go: make the working-indicator label reflect the active mode ("N steered" vs "N queued") instead of hardcoding "steered". - Slash command: /followup-behavior steer|followup with a plain argument shows, sets, or rejects invalid values via toast notifications. - Tests: unit tests for GetFollowupBehavior normalization + TUI tests for the follow-up dispatch path, follow-up queue-full handling, attachment pass-through, and unconditional dual-queue clearing. This PR is stacked on the two commits from docker#2405 to provide an integrated diff. It will rebase cleanly once docker#2405 merges.
|
/review |
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
Two confirmed high-severity bugs allow follow-up messages from a cancelled session to contaminate the next session's conversation. A medium-severity race condition means the mode switch is not in effect immediately after the command is issued.
| go func() { | ||
| // Drain any orphaned steer messages from a previous cancelled run | ||
| // so they don't leak into this new stream. | ||
| r.steerQueue.Drain(context.Background()) |
There was a problem hiding this comment.
HIGH: followUpQueue not drained at RunStream start — orphaned follow-ups leak into the next session
RunStream explicitly drains steerQueue at startup (line 80) to clear orphaned messages from a previous cancelled session, but followUpQueue receives no equivalent drain. If the user cancels a session while follow-up messages are pending (in followup mode), those messages remain in followUpQueue. The next RunStream call — for a completely different conversation — will dequeue and process them at the end of the first agent turn (loop.go:454: r.followUpQueue.Dequeue(ctx)), silently injecting stale messages into the new session as if the user sent them there. This works in tandem with Finding 2 below.
Fix: Add r.followUpQueue.Drain(context.Background()) immediately after the steer drain on line 80.
There was a problem hiding this comment.
Good catch — fixed in 4f823e4. RunStream now drains both queues at entry, so a follow-up enqueued in a previously cancelled run can't leak into the next session.
| } | ||
| // Clear the display-only queue; steered messages that the runtime | ||
| // hasn't consumed yet are lost when the stream is cancelled. | ||
| p.messageQueue = nil |
There was a problem hiding this comment.
HIGH: StreamCancelledMsg handler clears the TUI display queue but not the runtime follow-up queue
When a stream is cancelled, the handler sets p.messageQueue = nil (clearing the sidebar display) but never calls p.app.ClearFollowUpQueue(). In followup mode, any messages the user dispatched via FollowUp() before hitting Cancel are still sitting in the runtime's internal followUpQueue. They survive the cancel boundary and — combined with the missing drain in RunStream — will be dequeued and executed in the user's next session.
By contrast, handleClearQueue (the explicit clear-queue action) correctly calls both p.app.ClearSteerQueue() and p.app.ClearFollowUpQueue().
Fix: Call p.app.ClearFollowUpQueue() (and defensively p.app.ClearSteerQueue()) in the StreamCancelledMsg handler, just as handleClearQueue does.
There was a problem hiding this comment.
Fixed in 4f823e4. I extracted the display-queue-drop + ClearSteerQueue + ClearFollowUpQueue logic into a clearPendingQueues helper and called it from both handleClearQueue and the StreamCancelledMsg handler, so the explicit clear action and the cancel path have identical semantics. Added TestClearPendingQueues_StreamCancelled to pin the cancel path's drain behaviour.
| } | ||
|
|
||
| // Persist to global userconfig (fire-and-forget, matching the split-diff pattern). | ||
| go func() { |
There was a problem hiding this comment.
MEDIUM: /followup-behavior setting is persisted asynchronously — the new mode is not active until the file write completes
handleSetFollowupBehavior fires a goroutine to write the new value to disk, but userconfig.Get() calls Load() → os.ReadFile on every invocation with no in-memory cache. Between when the goroutine starts and when the file write commits, every call to userconfig.Get().GetFollowupBehavior() — including the dispatch switch in handleSendMsg and the queueLabelForMode() label in the working indicator — still returns the old value.
A user who types /followup-behavior followup and immediately sends a message while the agent is busy will see the success toast but have their message incorrectly steered (old behavior) rather than queued as a follow-up. The split-diff toggle uses the same fire-and-forget pattern, but its state is driven by an in-memory field (sessionState.ToggleSplitDiffView()), so the race doesn't affect it.
Fix: Apply the change to an in-memory field (e.g. update a package-level variable via a Set function) synchronously before launching the goroutine, so the in-flight setting is visible immediately.
There was a problem hiding this comment.
Fixed in 4f823e4. Added userconfig.SetFollowupBehaviorOverride, a mutex-guarded package-level override that Get applies on top of whatever it reads from disk. handleSetFollowupBehavior now calls it synchronously before launching the persistence goroutine, so the dispatch switch in handleSendMsg and the queueLabelForMode label see the new value immediately — no race against the file write. Covered by TestSetFollowupBehaviorOverride.
…mmediately Three review findings on the FollowupBehavior setting change: - HIGH: `LocalRuntime.RunStream` only drained the steer queue at startup, so a follow-up message enqueued in the previous (cancelled) session survived the cancel boundary and was dequeued as the first action of the next `RunStream` call. Drain both queues on entry. - HIGH: The TUI `StreamCancelledMsg` handler cleared the sidebar display queue but did not call `ClearSteerQueue`/`ClearFollowUpQueue` on the app, so pending messages sat in the runtime queues until the next run. Extract the drain into a `clearPendingQueues` helper and call it from both the explicit clear-queue action and the cancel handler. - MEDIUM: `/followup-behavior` persisted the new mode in a goroutine while `userconfig.Get` reads from disk on every call. Until the write committed, the dispatch switch and working-indicator label still saw the old value. Add `SetFollowupBehaviorOverride` as an in-memory override that `Get` consults, and set it synchronously in the handler before the background save, so the new mode is visible immediately.
1 participant
Summary
PR #2405 made the TUI unconditionally use mid-turn steering (
runtime.Steer,<system-reminder>injection) for messages submitted while the agent is working. Some users prefer the previous follow-up semantics, where each queued message runs as its own undivided turn after the current one completes.This PR restores the choice via a persisted user-config setting and a slash command. Default is
steer(no behavior change for existing users).User-facing changes
New setting
settings.followup_behaviorin~/.config/cagent/config.yaml(under the existingSettingsstruct). Values:steer(default) |followup.New slash command
/followup-behavior [steer|followup]:Working-indicator wording now reflects the active mode —
Working… (N steered)in steer mode,Working… (N queued)in follow-up mode — instead of always saying "steered".