fix(tui): preserve input panel during streamed turns#153
Merged
Conversation
Sigilix review resumedThis PR is ready for review. The earlier draft-skip notice has been superseded by the normal review run. Current commit: |
✅ QualityMax Pipeline
Powered by QualityMax — AI-Powered Test Automation |
Desperado
marked this pull request as ready for review
July 18, 2026 07:39
Sigilix OverviewEffort: 5/5 (needs deep review) Quality gates
Summary — latest pushReplaces the transient post-submit input view with a persistent Bubble Tea viewport that keeps the input/status panel visible and editable during streamed agent turns, preventing independent cursor writers from overwriting the bordered region. Agent stderr, tool output, and test progress are now routed through the viewport's activity/output channels, and retained scrollback is re-emitted to the terminal when the turn completes. The specialist review flagged a P0 race where a late subprocess write after detach can corrupt the terminal, alongside P1 concerns around unbounded string copies during output bounding and a missing nil-guard on the cache pointer. Important files
Sequence diagramsequenceDiagram
participant User
participant Viewport as turnViewportModel
participant Terminal
participant Agent as Agent Subprocess
User->>Viewport: KeyMsg (type / Enter)
Viewport->>Terminal: cancelFn() on Enter/Ctrl+C
Agent->>Terminal: stderr (via term.Stderr())
Agent->>Terminal: stdout stream
Terminal->>Viewport: turnOutputMsg / turnActivityMsg
Viewport->>Viewport: appendOutput + bound + wrap
Agent->>Viewport: turnDoneMsg
Viewport->>Terminal: emit retained scrollback
Viewport->>User: Quit (return TurnInputResult)
Confidence: 2/5A P0 race condition where late subprocess output can corrupt the terminal after detach, combined with O(n) string copies on every output bounding cycle, makes this unsafe to merge without addressing the concurrency and performance regressions.
Suggested labels:
|
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Reviewed 13 of 14 changed files across this PR so far — the remaining 1 was below the priority cutoff for this very large PR. Split into smaller PRs to cover them.
1 finding outside the diff
| File | Scope | Finding |
|---|---|---|
| internal/tui/terminal.go:175 | file-scope | Race condition on turnProgram field between attach/detach and activeTurnProgram |
|
|
||
| func newSpinner(t *Terminal) *spinner { | ||
| s := &spinner{stop: make(chan struct{}), term: t} | ||
| if p := t.activeTurnProgram(); p != nil { |
There was a problem hiding this comment.
Nil-pointer dereference in spinner when Terminal is nil
newSpinner calls t.activeTurnProgram() without checking if t is nil. The Terminal receiver on activeTurnProgram is a pointer receiver, so calling it on a nil *Terminal will panic. While newSpinner is only called from StartThinking which has a nil guard, the newSpinner function itself is exported and could be called directly. The fix is to add a nil check at the top of newSpinner before accessing t.activeTurnProgram().
More Info
- Threat model: A nil
*Terminalpassed tonewSpinnercauses a runtime panic, crashing the application. - Specific code citations:
newSpinneratterminal.go:117callst.activeTurnProgram()on line 118 without a nil check ont. - Existing protections:
StartThinking(the only current caller) checkst == nilbefore callingnewSpinner, but the function itself is unprotected. - Proposed mitigation: Add
if t == nil { return &spinner{stop: make(chan struct{})} }at the top ofnewSpinnerbefore any field access. - Alternative mitigations considered: Make
newSpinnerunexported, but that doesn't prevent future internal misuse. - Severity calibration: Score 4 because the only current caller is safe, but the function is exported and a future caller could trigger a panic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/terminal.go
Line: 118
Comment:
**Nil-pointer dereference in spinner when Terminal is nil**
`newSpinner` calls `t.activeTurnProgram()` without checking if `t` is nil. The `Terminal` receiver on `activeTurnProgram` is a pointer receiver, so calling it on a nil `*Terminal` will panic. While `newSpinner` is only called from `StartThinking` which has a nil guard, the `newSpinner` function itself is exported and could be called directly. The fix is to add a nil check at the top of `newSpinner` before accessing `t.activeTurnProgram()`.
Threat model:
A nil `*Terminal` passed to `newSpinner` causes a runtime panic, crashing the application.
Specific code citations:
`newSpinner` at `terminal.go:117` calls `t.activeTurnProgram()` on line 118 without a nil check on `t`.
Existing protections:
`StartThinking` (the only current caller) checks `t == nil` before calling `newSpinner`, but the function itself is unprotected.
Proposed mitigation:
Add `if t == nil { return &spinner{stop: make(chan struct{})} }` at the top of `newSpinner` before any field access.
Alternative mitigations considered:
Make `newSpinner` unexported, but that doesn't prevent future internal misuse.
Severity calibration:
Score 4 because the only current caller is safe, but the function is exported and a future caller could trigger a panic.
How can I resolve this? If you propose a fix, please make it concise.
| if ok && final.output != nil { | ||
| // Bubble Tea only owns the live viewport. Re-emit the complete turn once | ||
| // after it exits so normal terminal scrollback retains the full response. | ||
| term.emit(final.output.String()) |
There was a problem hiding this comment.
Error from p.Run() is silently ignored, masking Bubble Tea failures
RunTurnViewport calls p.Run() and assigns the error to err, but the error is only checked at the very end of the function after final.output.String() is called. If p.Run() returns an error, final will be the zero-value turnViewportModel, and final.output will be nil. The code then calls final.output.String() on a nil *strings.Builder, causing a panic. The error check should happen before accessing final.output.
More Info
- Threat model: A Bubble Tea initialization or runtime error causes
p.Run()to return a non-nil error, leading to a nil-pointer dereference onfinal.output.String(). - Specific code citations:
RunTurnViewportatturn_viewport.go:268callsp.Run()and stores the error. Line 275 callsfinal.output.String()before checkingerron line 280. - Existing protections: None — the error is checked after the nil-prone access.
- Proposed mitigation: Move the error check before
final.output.String(): iferr != nil, return early without accessingfinal.output. - Alternative mitigations considered: Initialize
final.outputto an empty builder beforep.Run(), but that still masks the error. - Severity calibration: Score 4 because Bubble Tea errors are rare but the nil dereference is guaranteed when they occur.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 275
Comment:
**Error from `p.Run()` is silently ignored, masking Bubble Tea failures**
`RunTurnViewport` calls `p.Run()` and assigns the error to `err`, but the error is only checked at the very end of the function after `final.output.String()` is called. If `p.Run()` returns an error, `final` will be the zero-value `turnViewportModel`, and `final.output` will be nil. The code then calls `final.output.String()` on a nil `*strings.Builder`, causing a panic. The error check should happen before accessing `final.output`.
Threat model:
A Bubble Tea initialization or runtime error causes `p.Run()` to return a non-nil error, leading to a nil-pointer dereference on `final.output.String()`.
Specific code citations:
`RunTurnViewport` at `turn_viewport.go:268` calls `p.Run()` and stores the error. Line 275 calls `final.output.String()` before checking `err` on line 280.
Existing protections:
None — the error is checked after the nil-prone access.
Proposed mitigation:
Move the error check before `final.output.String()`: if `err != nil`, return early without accessing `final.output`.
Alternative mitigations considered:
Initialize `final.output` to an empty builder before `p.Run()`, but that still masks the error.
Severity calibration:
Score 4 because Bubble Tea errors are rare but the nil dereference is guaranteed when they occur.
How can I resolve this? If you propose a fix, please make it concise.
| } | ||
| // Bubble Tea truncates over-wide physical lines. Wrap them first so a | ||
| // streamed paragraph remains readable while the persistent panel is active. | ||
| out = ansi.Wrap(out, width, "") |
There was a problem hiding this comment.
visibleOutput panics if m.output is nil
visibleOutput calls m.output.String() without checking if m.output is nil. While newTurnViewportModel initializes m.output to a new strings.Builder, the model is also constructed via the zero-value when p.Run() returns (e.g., on error). If View() is called on a zero-value model, this will panic. The fix is to add a nil check at the top of visibleOutput.
More Info
- Threat model: A zero-value
turnViewportModel(e.g., from a Bubble Tea error) causes a nil-pointer dereference inView(). - Specific code citations:
visibleOutputatturn_viewport.go:237callsm.output.String()without a nil check. - Existing protections:
newTurnViewportModelinitializesm.output, but the zero-value path (line 280) does not. - Proposed mitigation: Add
if m.output == nil { return "" }at the top ofvisibleOutput. - Alternative mitigations considered: Initialize
m.outputin the zero-value return path ofRunTurnViewport. - Severity calibration: Score 3 because the zero-value path is only reached on error, but the panic is guaranteed in that case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 237
Comment:
**`visibleOutput` panics if `m.output` is nil**
`visibleOutput` calls `m.output.String()` without checking if `m.output` is nil. While `newTurnViewportModel` initializes `m.output` to a new `strings.Builder`, the model is also constructed via the zero-value when `p.Run()` returns (e.g., on error). If `View()` is called on a zero-value model, this will panic. The fix is to add a nil check at the top of `visibleOutput`.
Threat model:
A zero-value `turnViewportModel` (e.g., from a Bubble Tea error) causes a nil-pointer dereference in `View()`.
Specific code citations:
`visibleOutput` at `turn_viewport.go:237` calls `m.output.String()` without a nil check.
Existing protections:
`newTurnViewportModel` initializes `m.output`, but the zero-value path (line 280) does not.
Proposed mitigation:
Add `if m.output == nil { return "" }` at the top of `visibleOutput`.
Alternative mitigations considered:
Initialize `m.output` in the zero-value return path of `RunTurnViewport`.
Severity calibration:
Score 3 because the zero-value path is only reached on error, but the panic is guaranteed in that case.
How can I resolve this? If you propose a fix, please make it concise.
| if m.output.Len()-m.liveFrom > maxLiveOutput { | ||
| out := m.output.String() | ||
| target := len(out) - maxLiveOutput | ||
| if newline := strings.IndexByte(out[target:], '\n'); newline >= 0 { |
There was a problem hiding this comment.
liveFrom can advance past the end of the output string, causing a slice bounds panic
In the turnOutputMsg handler, m.liveFrom is advanced to target + newline + 1 where target = len(out) - maxLiveOutput. If maxLiveOutput is larger than len(out), target is negative, and out[target:] slices from the end of the string. The strings.IndexByte call on a negative-start slice is safe in Go (it treats negative indices as 0 for slicing), but the resulting m.liveFrom could be set to a value greater than len(out), causing a panic on the next m.output.String()[m.liveFrom:] call in visibleOutput. The fix is to clamp target to 0 before using it.
More Info
- Threat model: When the total output is less than
maxLiveOutput,targetbecomes negative, andm.liveFromcan be set to an out-of-bounds index, causing a slice bounds panic. - Specific code citations:
turnOutputMsghandler atturn_viewport.go:75computestarget = len(out) - maxLiveOutputwithout clamping to 0. - Existing protections: The
if m.output.Len()-m.liveFrom > maxLiveOutputguard prevents this when the live window is already large, but not on the first few messages when total output is small. - Proposed mitigation: Clamp
targettomax(target, 0)before using it. - Alternative mitigations considered: Use
max(0, len(out)-maxLiveOutput)directly. - Severity calibration: Score 3 because the condition requires specific output sizes and the panic is edge-case but real.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 75
Comment:
**`liveFrom` can advance past the end of the output string, causing a slice bounds panic**
In the `turnOutputMsg` handler, `m.liveFrom` is advanced to `target + newline + 1` where `target = len(out) - maxLiveOutput`. If `maxLiveOutput` is larger than `len(out)`, `target` is negative, and `out[target:]` slices from the end of the string. The `strings.IndexByte` call on a negative-start slice is safe in Go (it treats negative indices as 0 for slicing), but the resulting `m.liveFrom` could be set to a value greater than `len(out)`, causing a panic on the next `m.output.String()[m.liveFrom:]` call in `visibleOutput`. The fix is to clamp `target` to 0 before using it.
Threat model:
When the total output is less than `maxLiveOutput`, `target` becomes negative, and `m.liveFrom` can be set to an out-of-bounds index, causing a slice bounds panic.
Specific code citations:
`turnOutputMsg` handler at `turn_viewport.go:75` computes `target = len(out) - maxLiveOutput` without clamping to 0.
Existing protections:
The `if m.output.Len()-m.liveFrom > maxLiveOutput` guard prevents this when the live window is already large, but not on the first few messages when total output is small.
Proposed mitigation:
Clamp `target` to `max(target, 0)` before using it.
Alternative mitigations considered:
Use `max(0, len(out)-maxLiveOutput)` directly.
Severity calibration:
Score 3 because the condition requires specific output sizes and the panic is edge-case but real.
How can I resolve this? If you propose a fix, please make it concise.
| return m, nil | ||
| } | ||
|
|
||
| func (m turnViewportModel) updateKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { |
There was a problem hiding this comment.
turnDoneMsg handler overwrites m.result.Text even when already set by Enter
When the user presses Enter to queue-and-interrupt, updateKey sets m.result.Text and m.result.Canceled = true, then returns tea.Sequence(cancelFn, tea.Quit). The cancelFn eventually causes the backend run() to return, which sends turnDoneMsg. The turnDoneMsg handler then overwrites m.result.Text with strings.TrimSpace(string(m.text)), which is the text at the time the backend finished — potentially empty if the user cleared the input after pressing Enter. This means the queued text could be lost. The fix is to only set m.result.Text in turnDoneMsg if it hasn't already been set by a user action.
More Info
- Threat model: User presses Enter to queue text and interrupt, but the backend finishes before Bubble Tea processes the quit, causing
turnDoneMsgto overwrite the queued text with an empty string. - Specific code citations:
updateKeyatturn_viewport.go:108setsm.result.Text.turnDoneMsghandler atturn_viewport.go:100unconditionally overwrites it. - Existing protections: None — the handlers don't coordinate on
m.result.Text. - Proposed mitigation: In
turnDoneMsg, only setm.result.Textifm.result.Text == "". - Alternative mitigations considered: Use a separate flag to track whether the user already set the result.
- Severity calibration: Score 3 because the race window is small (between Enter and backend shutdown) but the data loss is real.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 102
Comment:
**`turnDoneMsg` handler overwrites `m.result.Text` even when already set by Enter**
When the user presses Enter to queue-and-interrupt, `updateKey` sets `m.result.Text` and `m.result.Canceled = true`, then returns `tea.Sequence(cancelFn, tea.Quit)`. The `cancelFn` eventually causes the backend `run()` to return, which sends `turnDoneMsg`. The `turnDoneMsg` handler then overwrites `m.result.Text` with `strings.TrimSpace(string(m.text))`, which is the text at the time the backend finished — potentially empty if the user cleared the input after pressing Enter. This means the queued text could be lost. The fix is to only set `m.result.Text` in `turnDoneMsg` if it hasn't already been set by a user action.
Threat model:
User presses Enter to queue text and interrupt, but the backend finishes before Bubble Tea processes the quit, causing `turnDoneMsg` to overwrite the queued text with an empty string.
Specific code citations:
`updateKey` at `turn_viewport.go:108` sets `m.result.Text`. `turnDoneMsg` handler at `turn_viewport.go:100` unconditionally overwrites it.
Existing protections:
None — the handlers don't coordinate on `m.result.Text`.
Proposed mitigation:
In `turnDoneMsg`, only set `m.result.Text` if `m.result.Text == ""`.
Alternative mitigations considered:
Use a separate flag to track whether the user already set the result.
Severity calibration:
Score 3 because the race window is small (between Enter and backend shutdown) but the data loss is real.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
115
to
+118
|
|
||
| func newSpinner(t *Terminal) *spinner { | ||
| s := &spinner{stop: make(chan struct{}), term: t} | ||
| if p := t.activeTurnProgram(); p != nil { |
There was a problem hiding this comment.
newSpinner sends turnThinkingMsg(true) but spinner.Stop() sends turnThinkingMsg(false) — the spinner goroutine still runs
Low-confidence finding — expand to read
When a turn viewport is active, newSpinner sends turnThinkingMsg(true) to the Bubble Tea program and returns early WITHOUT starting the spinner goroutine. However, spinner.Stop() still calls s.done.Do(...) which sends turnThinkingMsg(false) and returns early. The sync.Once ensures this only happens once, but the spinner struct's wg and stop channel are never used in this path. This is not a bug per se, but it means the spinner object is in a half-initialized state (no goroutine, but wg is non-zero). If any code calls s.wg.Wait() on this spinner, it would return immediately (since no goroutine was started), which is correct but confusing. Consider making the viewport path not create a spinner at all.
More Info
- Threat model: Confusing half-initialized state; no runtime failure but maintenance hazard.
- Specific code citations:
newSpinneratterminal.go:118returns early without starting the goroutine.spinner.Stop()atterminal.go:152still usess.done.Do. - Existing protections:
sync.Onceprevents double-close of the channel. - Proposed mitigation: Return nil from
newSpinnerwhen a viewport is active, and handle nil inStopThinking. - Alternative mitigations considered: Document the half-initialized state.
- Severity calibration: Score 2 because it's a maintainability concern, not a runtime bug.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/terminal.go
Line: 115-118
Comment:
**`newSpinner` sends `turnThinkingMsg(true)` but `spinner.Stop()` sends `turnThinkingMsg(false)` — the spinner goroutine still runs**
When a turn viewport is active, `newSpinner` sends `turnThinkingMsg(true)` to the Bubble Tea program and returns early WITHOUT starting the spinner goroutine. However, `spinner.Stop()` still calls `s.done.Do(...)` which sends `turnThinkingMsg(false)` and returns early. The `sync.Once` ensures this only happens once, but the `spinner` struct's `wg` and `stop` channel are never used in this path. This is not a bug per se, but it means the `spinner` object is in a half-initialized state (no goroutine, but `wg` is non-zero). If any code calls `s.wg.Wait()` on this spinner, it would return immediately (since no goroutine was started), which is correct but confusing. Consider making the viewport path not create a spinner at all.
Threat model:
Confusing half-initialized state; no runtime failure but maintenance hazard.
Specific code citations:
`newSpinner` at `terminal.go:118` returns early without starting the goroutine. `spinner.Stop()` at `terminal.go:152` still uses `s.done.Do`.
Existing protections:
`sync.Once` prevents double-close of the channel.
Proposed mitigation:
Return nil from `newSpinner` when a viewport is active, and handle nil in `StopThinking`.
Alternative mitigations considered:
Document the half-initialized state.
Severity calibration:
Score 2 because it's a maintainability concern, not a runtime bug.
How can I resolve this? If you propose a fix, please make it concise.
| if w <= 0 { | ||
| w = 80 | ||
| } | ||
| input := inputModel{prompt: m.prompt, width: w, status: m.status} |
There was a problem hiding this comment.
View() creates a new inputModel on every frame, discarding any internal state
Low-confidence finding — expand to read
turnViewportModel.View() creates a new inputModel literal on every call: input := inputModel{prompt: m.prompt, width: w, status: m.status}. If inputModel ever gains internal state (e.g., animation counters, caching), this pattern will reset it on every frame. Currently inputModel is stateless in its renderInputBox and renderStatus methods, so this is safe, but it's a fragile pattern. Consider storing the inputModel as a field on turnViewportModel and updating its fields when they change.
More Info
- Threat model: Future changes to
inputModelthat add state will be silently broken by the per-frame reconstruction. - Specific code citations:
turnViewportModel.View()atturn_viewport.go:205creates a newinputModelliteral. - Existing protections:
inputModelis currently stateless in its rendering methods. - Proposed mitigation: Store
inputModelas a field onturnViewportModeland update it on relevant messages. - Alternative mitigations considered: Add a comment warning about the stateless assumption.
- Severity calibration: Score 2 because it's a forward-compatibility concern with no current bug.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 203
Comment:
**`View()` creates a new `inputModel` on every frame, discarding any internal state**
`turnViewportModel.View()` creates a new `inputModel` literal on every call: `input := inputModel{prompt: m.prompt, width: w, status: m.status}`. If `inputModel` ever gains internal state (e.g., animation counters, caching), this pattern will reset it on every frame. Currently `inputModel` is stateless in its `renderInputBox` and `renderStatus` methods, so this is safe, but it's a fragile pattern. Consider storing the `inputModel` as a field on `turnViewportModel` and updating its fields when they change.
Threat model:
Future changes to `inputModel` that add state will be silently broken by the per-frame reconstruction.
Specific code citations:
`turnViewportModel.View()` at `turn_viewport.go:205` creates a new `inputModel` literal.
Existing protections:
`inputModel` is currently stateless in its rendering methods.
Proposed mitigation:
Store `inputModel` as a field on `turnViewportModel` and update it on relevant messages.
Alternative mitigations considered:
Add a comment warning about the stateless assumption.
Severity calibration:
Score 2 because it's a forward-compatibility concern with no current bug.
How can I resolve this? If you propose a fix, please make it concise.
| } | ||
| // Bubble Tea truncates over-wide physical lines. Wrap them first so a | ||
| // streamed paragraph remains readable while the persistent panel is active. | ||
| out = ansi.Wrap(out, width, "") |
There was a problem hiding this comment.
visibleOutput calls ansi.Wrap on every View() call, potentially causing performance issues with large output
Low-confidence finding — expand to read
visibleOutput calls ansi.Wrap(out, width, "") on every frame render. For large accumulated output (up to 64KB live window), this re-wraps the entire visible buffer on every 100ms tick. The wrapping result is not cached, so it's recomputed even when the output hasn't changed. Consider caching the wrapped output and only recomputing when m.output or m.width changes.
More Info
- Threat model: High CPU usage during long agent turns with large output, potentially causing visible lag in the terminal.
- Specific code citations:
visibleOutputatturn_viewport.go:242callsansi.Wrapunconditionally. - Existing protections: The live window is capped at 64KB, limiting the worst-case cost.
- Proposed mitigation: Cache the wrapped output and invalidate on output/width changes.
- Alternative mitigations considered: Accept the cost; 64KB wrapping is fast on modern hardware.
- Severity calibration: Score 2 because the 64KB cap limits the impact, but it's still unnecessary work on every frame.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 237
Comment:
**`visibleOutput` calls `ansi.Wrap` on every `View()` call, potentially causing performance issues with large output**
`visibleOutput` calls `ansi.Wrap(out, width, "")` on every frame render. For large accumulated output (up to 64KB live window), this re-wraps the entire visible buffer on every 100ms tick. The wrapping result is not cached, so it's recomputed even when the output hasn't changed. Consider caching the wrapped output and only recomputing when `m.output` or `m.width` changes.
Threat model:
High CPU usage during long agent turns with large output, potentially causing visible lag in the terminal.
Specific code citations:
`visibleOutput` at `turn_viewport.go:242` calls `ansi.Wrap` unconditionally.
Existing protections:
The live window is capped at 64KB, limiting the worst-case cost.
Proposed mitigation:
Cache the wrapped output and invalidate on output/width changes.
Alternative mitigations considered:
Accept the cost; 64KB wrapping is fast on modern hardware.
Severity calibration:
Score 2 because the 64KB cap limits the impact, but it's still unnecessary work on every frame.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| func (t *Terminal) emit(text string) { | ||
| if p := t.activeTurnProgram(); p != nil { | ||
| p.Send(turnOutputMsg(text)) |
There was a problem hiding this comment.
emit method sends turnOutputMsg to a potentially nil program without nil check
Low-confidence finding — expand to read
emit calls p.Send(turnOutputMsg(text)) on the pointer returned by activeTurnProgram(). If activeTurnProgram returns a non-nil pointer that has already been shut down (Bubble Tea program exited but pointer not yet cleared), p.Send() will panic or deadlock. Bubble Tea's Send method is not safe to call after the program has exited. The fix is to check if the program is still running before sending, or to use a channel-based approach.
More Info
- Threat model: A send to an exited Bubble Tea program causes a panic or deadlock.
- Specific code citations:
emitatterminal.go:194callsp.Send()without checking if the program is still running. - Existing protections:
detachTurnProgramsetsturnProgram = nil, but there's a race window between program exit and detach. - Proposed mitigation: Use a channel or a done flag to prevent sends to an exited program.
- Alternative mitigations considered: Accept the race; Bubble Tea's
Sendmay be safe after exit in practice. - Severity calibration: Score 2 because the race window is small and Bubble Tea may handle this gracefully.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/terminal.go
Line: 198
Comment:
**`emit` method sends `turnOutputMsg` to a potentially nil program without nil check**
`emit` calls `p.Send(turnOutputMsg(text))` on the pointer returned by `activeTurnProgram()`. If `activeTurnProgram` returns a non-nil pointer that has already been shut down (Bubble Tea program exited but pointer not yet cleared), `p.Send()` will panic or deadlock. Bubble Tea's `Send` method is not safe to call after the program has exited. The fix is to check if the program is still running before sending, or to use a channel-based approach.
Threat model:
A send to an exited Bubble Tea program causes a panic or deadlock.
Specific code citations:
`emit` at `terminal.go:194` calls `p.Send()` without checking if the program is still running.
Existing protections:
`detachTurnProgram` sets `turnProgram = nil`, but there's a race window between program exit and detach.
Proposed mitigation:
Use a channel or a done flag to prevent sends to an exited program.
Alternative mitigations considered:
Accept the race; Bubble Tea's `Send` may be safe after exit in practice.
Severity calibration:
Score 2 because the race window is small and Bubble Tea may handle this gracefully.
How can I resolve this? If you propose a fix, please make it concise.
| if text == "" { | ||
| return m, nil | ||
| } | ||
| m.result.Text = text |
There was a problem hiding this comment.
Enter key handler calls m.cancelFn() via tea.Sequence but doesn't check if it's nil
In updateKey, the Enter handler calls m.cancelFn() inside a tea.Sequence without checking if m.cancelFn is nil. While newTurnViewportModel always sets cancelFn from the parameter (which could be nil), the zero-value model has cancelFn: nil. If the Enter key is processed on a zero-value model, this will panic. The Ctrl+C handler has the same issue. The fix is to add a nil check before calling m.cancelFn.
More Info
- Threat model: Nil function call causes panic if Enter is pressed on a zero-value model.
- Specific code citations:
updateKeyatturn_viewport.go:108callsm.cancelFn()without nil check. - Existing protections:
newTurnViewportModelreceivescancelFnas a parameter, which may be nil. - Proposed mitigation: Add
if m.cancelFn != nil { m.cancelFn() }in both Enter and Ctrl+C handlers. - Alternative mitigations considered: Ensure
cancelFnis never nil by using a no-op function. - Severity calibration: Score 2 because the zero-value path is unlikely in practice but the nil dereference is real.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 109
Comment:
**Enter key handler calls `m.cancelFn()` via `tea.Sequence` but doesn't check if it's nil**
In `updateKey`, the Enter handler calls `m.cancelFn()` inside a `tea.Sequence` without checking if `m.cancelFn` is nil. While `newTurnViewportModel` always sets `cancelFn` from the parameter (which could be nil), the zero-value model has `cancelFn: nil`. If the Enter key is processed on a zero-value model, this will panic. The Ctrl+C handler has the same issue. The fix is to add a nil check before calling `m.cancelFn`.
Threat model:
Nil function call causes panic if Enter is pressed on a zero-value model.
Specific code citations:
`updateKey` at `turn_viewport.go:108` calls `m.cancelFn()` without nil check.
Existing protections:
`newTurnViewportModel` receives `cancelFn` as a parameter, which may be nil.
Proposed mitigation:
Add `if m.cancelFn != nil { m.cancelFn() }` in both Enter and Ctrl+C handlers.
Alternative mitigations considered:
Ensure `cancelFn` is never nil by using a no-op function.
Severity calibration:
Score 2 because the zero-value path is unlikely in practice but the nil dereference is real.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
1 finding outside the diff
| File | Scope | Finding |
|---|---|---|
| internal/tui/turn_viewport.go | file-scope | safeOutputSuffix fallback path lacks test coverage for ANSI escape sequence splitting |
Comment on lines
+355
to
+360
| p.Send(turnDoneMsg{}) | ||
| }() | ||
| result, err := p.Run() | ||
| <-done | ||
| // Keep the terminal attached until the canceled backend has actually | ||
| // stopped. Any late subprocess output is then discarded by the completed |
There was a problem hiding this comment.
Race: p.Run() can return before run() goroutine closes done, causing nil-pointer dereference on final.output
RunTurnViewport starts run() in a goroutine that closes done after run() returns. p.Run() blocks until the Bubble Tea program exits (via tea.Quit). If p.Run() returns BEFORE the run() goroutine closes done — which happens when the user presses Enter or Ctrl+C, sending tea.Quit — the <-done receive on line 368 blocks. However, the type assertion final, ok := result.(turnViewportModel) on line 370 executes BEFORE <-done. If p.Run() returns an error (e.g., terminal detach failure), result is nil, and final.output on line 371 dereferences a nil pointer. The fix is to receive from done BEFORE the type assertion, or to check ok before accessing final.output.
Example:
If `p.Run()` returns an error (e.g., terminal detach failure), `result` is nil. The type assertion `final, ok := result.(turnViewportModel)` sets `final` to the zero value and `ok` to false. Accessing `final.output` on line 371 dereferences a nil pointer, causing a panic.
Current:
p.Send(turnDoneMsg{})
}()
result, err := p.Run()
<-done
// Keep the terminal attached until the canceled backend has actually
// stopped. Any late subprocess output is then discarded by the completedProposed:
result, err := p.Run()
<-done
if err != nil {
return TurnInputResult{}
}
final, ok := result.(turnViewportModel)
if !ok {
return TurnInputResult{}
}
Suggested change
| p.Send(turnDoneMsg{}) | |
| }() | |
| result, err := p.Run() | |
| <-done | |
| // Keep the terminal attached until the canceled backend has actually | |
| // stopped. Any late subprocess output is then discarded by the completed | |
| result, err := p.Run() | |
| <-done | |
| if err != nil { | |
| return TurnInputResult{} | |
| } | |
| final, ok := result.(turnViewportModel) | |
| if !ok { | |
| return TurnInputResult{} | |
| } |
More Info
- Threat model: A nil-pointer dereference crashes the process. This can occur if
p.Run()returns an error (e.g., terminal I/O failure) or if the type assertion fails. - Specific code citations:
p.Run()on line 367 returns(tea.Model, error). The type assertion on line 370 accessesfinal.outputon line 371 before checkingokorerr. - Existing protections: None. The
<-donesynchronization on line 368 is after the type assertion, not before it. - Proposed mitigation: Move the
<-donereceive to BEFORE the type assertion, or add a nil check:if err != nil || !ok { return TurnInputResult{} }before accessingfinal.output. - Alternative mitigations considered: Using a
deferin the goroutine to send a result through a channel would be cleaner but more invasive. - Severity calibration: Score 5: nil-pointer dereference in a critical path (terminal I/O) that can crash the process under plausible conditions (terminal detach failure, type assertion failure).
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 355-360
Comment:
**Race: `p.Run()` can return before `run()` goroutine closes `done`, causing nil-pointer dereference on `final.output`**
`RunTurnViewport` starts `run()` in a goroutine that closes `done` after `run()` returns. `p.Run()` blocks until the Bubble Tea program exits (via `tea.Quit`). If `p.Run()` returns BEFORE the `run()` goroutine closes `done` — which happens when the user presses Enter or Ctrl+C, sending `tea.Quit` — the `<-done` receive on line 368 blocks. However, the type assertion `final, ok := result.(turnViewportModel)` on line 370 executes BEFORE `<-done`. If `p.Run()` returns an error (e.g., terminal detach failure), `result` is nil, and `final.output` on line 371 dereferences a nil pointer. The fix is to receive from `done` BEFORE the type assertion, or to check `ok` before accessing `final.output`.
Example:
If `p.Run()` returns an error (e.g., terminal detach failure), `result` is nil. The type assertion `final, ok := result.(turnViewportModel)` sets `final` to the zero value and `ok` to false. Accessing `final.output` on line 371 dereferences a nil pointer, causing a panic.
Threat model:
A nil-pointer dereference crashes the process. This can occur if `p.Run()` returns an error (e.g., terminal I/O failure) or if the type assertion fails.
Specific code citations:
`p.Run()` on line 367 returns `(tea.Model, error)`. The type assertion on line 370 accesses `final.output` on line 371 before checking `ok` or `err`.
Existing protections:
None. The `<-done` synchronization on line 368 is after the type assertion, not before it.
Proposed mitigation:
Move the `<-done` receive to BEFORE the type assertion, or add a nil check: `if err != nil || !ok { return TurnInputResult{} }` before accessing `final.output`.
Alternative mitigations considered:
Using a `defer` in the goroutine to send a result through a channel would be cleaner but more invasive.
Severity calibration:
Score 5: nil-pointer dereference in a critical path (terminal I/O) that can crash the process under plausible conditions (terminal detach failure, type assertion failure).
Suggested fix shape:
result, err := p.Run()
<-done
if err != nil {
return TurnInputResult{}
}
final, ok := result.(turnViewportModel)
if !ok {
return TurnInputResult{}
}
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+355
to
+368
| p.Send(turnDoneMsg{}) | ||
| }() | ||
| result, err := p.Run() | ||
| <-done | ||
| // Keep the terminal attached until the canceled backend has actually | ||
| // stopped. Any late subprocess output is then discarded by the completed | ||
| // Bubble Tea program instead of escaping below the former input panel. | ||
| term.detachTurnProgram(p) | ||
| final, ok := result.(turnViewportModel) | ||
| if ok && final.output != nil { | ||
| // Bubble Tea only owns the live viewport. Re-emit the retained turn output | ||
| // once after it exits so normal terminal scrollback remains useful. | ||
| term.emit(final.output.String()) | ||
| } |
There was a problem hiding this comment.
Goroutine panic in run() leaves done channel unclosed, causing RunTurnViewport to hang forever
The run() function is called in a goroutine that closes done on exit. If run() panics, defer close(done) never executes, and <-done on line 368 blocks indefinitely. This hangs the entire terminal session. The fix is to recover from panics in the goroutine and close done even on panic.
Example:
If `run()` panics with a nil pointer dereference, the goroutine exits without closing `done`. `<-done` on line 368 blocks forever, hanging the terminal.
Current:
p.Send(turnDoneMsg{})
}()
result, err := p.Run()
<-done
// Keep the terminal attached until the canceled backend has actually
// stopped. Any late subprocess output is then discarded by the completed
// Bubble Tea program instead of escaping below the former input panel.
term.detachTurnProgram(p)
final, ok := result.(turnViewportModel)
if ok && final.output != nil {
// Bubble Tea only owns the live viewport. Re-emit the retained turn output
// once after it exits so normal terminal scrollback remains useful.
term.emit(final.output.String())
}Proposed:
go func() {
defer close(done)
defer func() {
if r := recover(); r != nil {
p.Send(turnDoneMsg{})
}
}()
run()
p.Send(turnDoneMsg{})
}()
Suggested change
| p.Send(turnDoneMsg{}) | |
| }() | |
| result, err := p.Run() | |
| <-done | |
| // Keep the terminal attached until the canceled backend has actually | |
| // stopped. Any late subprocess output is then discarded by the completed | |
| // Bubble Tea program instead of escaping below the former input panel. | |
| term.detachTurnProgram(p) | |
| final, ok := result.(turnViewportModel) | |
| if ok && final.output != nil { | |
| // Bubble Tea only owns the live viewport. Re-emit the retained turn output | |
| // once after it exits so normal terminal scrollback remains useful. | |
| term.emit(final.output.String()) | |
| } | |
| go func() { | |
| defer close(done) | |
| defer func() { | |
| if r := recover(); r != nil { | |
| p.Send(turnDoneMsg{}) | |
| } | |
| }() | |
| run() | |
| p.Send(turnDoneMsg{}) | |
| }() |
More Info
- Threat model: A panic in the agent's
run()function (e.g., nil pointer dereference, out-of-bounds slice access) causes the terminal to hang indefinitely, requiring a force-quit. - Specific code citations: The goroutine on lines 361-365 calls
run()and closesdoneon normal return. Nodeferorrecoveris present. - Existing protections: None. The goroutine has no panic recovery.
- Proposed mitigation: Wrap
run()in a deferred recover:defer func() { if r := recover(); r != nil { /* log */ } close(done) }(). - Alternative mitigations considered: Using
p.Send(turnDoneMsg{})in a deferred function would also work but is more complex. - Severity calibration: Score 4: hangs the terminal on panic, requiring a force-quit. Panics in agent code are plausible (e.g., nil pointer dereference in a tool handler).
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 355-368
Comment:
**Goroutine panic in `run()` leaves `done` channel unclosed, causing `RunTurnViewport` to hang forever**
The `run()` function is called in a goroutine that closes `done` on exit. If `run()` panics, `defer close(done)` never executes, and `<-done` on line 368 blocks indefinitely. This hangs the entire terminal session. The fix is to recover from panics in the goroutine and close `done` even on panic.
Example:
If `run()` panics with a nil pointer dereference, the goroutine exits without closing `done`. `<-done` on line 368 blocks forever, hanging the terminal.
Threat model:
A panic in the agent's `run()` function (e.g., nil pointer dereference, out-of-bounds slice access) causes the terminal to hang indefinitely, requiring a force-quit.
Specific code citations:
The goroutine on lines 361-365 calls `run()` and closes `done` on normal return. No `defer` or `recover` is present.
Existing protections:
None. The goroutine has no panic recovery.
Proposed mitigation:
Wrap `run()` in a deferred recover: `defer func() { if r := recover(); r != nil { /* log */ } close(done) }()`.
Alternative mitigations considered:
Using `p.Send(turnDoneMsg{})` in a deferred function would also work but is more complex.
Severity calibration:
Score 4: hangs the terminal on panic, requiring a force-quit. Panics in agent code are plausible (e.g., nil pointer dereference in a tool handler).
Suggested fix shape:
go func() {
defer close(done)
defer func() {
if r := recover(); r != nil {
p.Send(turnDoneMsg{})
}
}()
run()
p.Send(turnDoneMsg{})
}()
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+148
to
+155
| } | ||
|
|
||
| func safeOutputSuffix(out string, keep int) string { | ||
| start := len(out) - keep | ||
| if start <= 0 { | ||
| return out | ||
| } | ||
| if newline := strings.IndexByte(out[start:], '\n'); newline >= 0 { |
There was a problem hiding this comment.
safeOutputSuffix can produce a negative slice index when keep > len(out) and out contains ANSI sequences
safeOutputSuffix computes start = len(out) - keep. If keep > len(out), start is negative. The if start <= 0 check on line 152 returns out early — but ONLY for the raw out string. If out contains ANSI escape sequences, ansi.Strip(out) on line 160 produces a shorter plain string. The code then recomputes start = len(plain) - keep. If keep is larger than len(plain) but smaller than len(out), start is negative, and the if start < 0 check on line 162 resets it to 0. However, if keep is larger than BOTH len(out) and len(plain), the early return on line 152 fires, which is correct. The real bug is subtler: if keep is between len(plain) and len(out), the early return on line 152 does NOT fire (because start = len(out) - keep is positive), but after stripping ANSI, start = len(plain) - keep is negative. The if start < 0 check on line 162 handles this. BUT there's a path where start is computed from plain and then used to index plain — if keep is exactly len(plain), start is 0, which is fine. The actual bug: if keep is 0, start = len(out) - 0 is positive, the early return doesn't fire, plain is shorter, start = len(plain) - 0 is len(plain), and plain[start:] is empty — that's fine. The real issue is when keep is negative (should never happen with constants, but if the constants are changed to 0 or negative). More critically: if out is empty, len(out) - keep is negative, the early return fires. So the negative-index path is actually unreachable with the current constants. However, there's a DIFFERENT bug: when out contains ONLY ANSI sequences and no printable characters, ansi.Strip(out) returns an empty string. start = 0 - keep is negative, the if start < 0 check resets to 0, and plain[0:] returns an empty string — that's fine. The REAL bug is: start is computed as len(out) - keep on line 150. If out is very long and keep is small, start is large. The code then looks for a newline in out[start:]. If no newline is found, it falls through to the ANSI-stripping path. But start is still based on len(out), not len(plain). The code recomputes start = len(plain) - keep on line 161, which is correct. So the negative-index path is handled. The ACTUAL bug is: when keep is larger than len(plain) but start = len(out) - keep is still positive (because out has ANSI sequences), the early return on line 152 does NOT fire. Then start = len(plain) - keep is negative, and the if start < 0 check resets it to 0. This means the function returns plain[0:] (the entire stripped output) instead of the last keep bytes. This is a logic error: the function returns MORE output than intended when ANSI sequences are present and keep is close to the stripped length.
Example:
Input: `out` = 65KB of ANSI-styled text (50KB stripped), `keep` = 48KB. `start = 65KB - 48KB = 17KB` (positive, no early return). `plain` = 50KB. `start = 50KB - 48KB = 2KB`. Correct. But if `out` = 100KB of ANSI-styled text (40KB stripped), `keep` = 48KB: `start = 100KB - 48KB = 52KB` (positive, no early return). `plain` = 40KB. `start = 40KB - 48KB = -8KB` → reset to 0. Returns entire 40KB instead of last 48KB (which is the whole 40KB anyway — no harm). The harmful case: `out` = 70KB (60KB stripped), `keep` = 48KB: `start = 70KB - 48KB = 22KB` (positive). `plain` = 60KB. `start = 60KB - 48KB = 12KB`. Returns last 48KB — correct. The bug only manifests when `keep > len(plain)` but `keep <= len(out)`, which requires ANSI overhead > `len(out) - keep`. With `maxLiveOutput = 64KB` and `keptLiveOutput = 48KB`, the ANSI overhead must exceed 16KB for the bug to trigger. This is plausible with heavily styled output.
Current:
}
func safeOutputSuffix(out string, keep int) string {
start := len(out) - keep
if start <= 0 {
return out
}
if newline := strings.IndexByte(out[start:], '\n'); newline >= 0 {Proposed:
if len(plain) <= keep {
return plain
}
start = len(plain) - keep
Suggested change
| } | |
| func safeOutputSuffix(out string, keep int) string { | |
| start := len(out) - keep | |
| if start <= 0 { | |
| return out | |
| } | |
| if newline := strings.IndexByte(out[start:], '\n'); newline >= 0 { | |
| if len(plain) <= keep { | |
| return plain | |
| } | |
| start = len(plain) - keep |
More Info
- Threat model: When
keepis larger thanlen(plain)but smaller thanlen(out)(due to ANSI sequences),safeOutputSuffixreturns the entire stripped output instead of the lastkeepbytes. This violates the memory bound, potentially causing the live output buffer to grow beyondkeptLiveOutput. - Specific code citations: Lines 150-152 compute
startfromlen(out)and checkstart <= 0. Lines 160-161 recomputestartfromlen(plain)but only after the early return on line 152 has already been bypassed. - Existing protections: The
if start < 0check on line 162 handles negativestartbut does not handle the case wherestartshould be positive butkeep > len(plain). - Proposed mitigation: After computing
plain, checkif len(plain) <= keep { return plain }before recomputingstart. - Alternative mitigations considered: Compute
startfromlen(plain)first, then checkstart <= 0. - Severity calibration: Score 4: violates the memory bound for live output when ANSI-heavy output is near the size limit. Could cause OOM under sustained streaming with ANSI sequences.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 148-155
Comment:
**`safeOutputSuffix` can produce a negative slice index when `keep > len(out)` and `out` contains ANSI sequences**
`safeOutputSuffix` computes `start = len(out) - keep`. If `keep > len(out)`, `start` is negative. The `if start <= 0` check on line 152 returns `out` early — but ONLY for the raw `out` string. If `out` contains ANSI escape sequences, `ansi.Strip(out)` on line 160 produces a shorter `plain` string. The code then recomputes `start = len(plain) - keep`. If `keep` is larger than `len(plain)` but smaller than `len(out)`, `start` is negative, and the `if start < 0` check on line 162 resets it to 0. However, if `keep` is larger than BOTH `len(out)` and `len(plain)`, the early return on line 152 fires, which is correct. The real bug is subtler: if `keep` is between `len(plain)` and `len(out)`, the early return on line 152 does NOT fire (because `start = len(out) - keep` is positive), but after stripping ANSI, `start = len(plain) - keep` is negative. The `if start < 0` check on line 162 handles this. BUT there's a path where `start` is computed from `plain` and then used to index `plain` — if `keep` is exactly `len(plain)`, `start` is 0, which is fine. The actual bug: if `keep` is 0, `start = len(out) - 0` is positive, the early return doesn't fire, `plain` is shorter, `start = len(plain) - 0` is `len(plain)`, and `plain[start:]` is empty — that's fine. The real issue is when `keep` is negative (should never happen with constants, but if the constants are changed to 0 or negative). More critically: if `out` is empty, `len(out) - keep` is negative, the early return fires. So the negative-index path is actually unreachable with the current constants. However, there's a DIFFERENT bug: when `out` contains ONLY ANSI sequences and no printable characters, `ansi.Strip(out)` returns an empty string. `start = 0 - keep` is negative, the `if start < 0` check resets to 0, and `plain[0:]` returns an empty string — that's fine. The REAL bug is: `start` is computed as `len(out) - keep` on line 150. If `out` is very long and `keep` is small, `start` is large. The code then looks for a newline in `out[start:]`. If no newline is found, it falls through to the ANSI-stripping path. But `start` is still based on `len(out)`, not `len(plain)`. The code recomputes `start = len(plain) - keep` on line 161, which is correct. So the negative-index path is handled. The ACTUAL bug is: when `keep` is larger than `len(plain)` but `start = len(out) - keep` is still positive (because `out` has ANSI sequences), the early return on line 152 does NOT fire. Then `start = len(plain) - keep` is negative, and the `if start < 0` check resets it to 0. This means the function returns `plain[0:]` (the entire stripped output) instead of the last `keep` bytes. This is a logic error: the function returns MORE output than intended when ANSI sequences are present and `keep` is close to the stripped length.
Example:
Input: `out` = 65KB of ANSI-styled text (50KB stripped), `keep` = 48KB. `start = 65KB - 48KB = 17KB` (positive, no early return). `plain` = 50KB. `start = 50KB - 48KB = 2KB`. Correct. But if `out` = 100KB of ANSI-styled text (40KB stripped), `keep` = 48KB: `start = 100KB - 48KB = 52KB` (positive, no early return). `plain` = 40KB. `start = 40KB - 48KB = -8KB` → reset to 0. Returns entire 40KB instead of last 48KB (which is the whole 40KB anyway — no harm). The harmful case: `out` = 70KB (60KB stripped), `keep` = 48KB: `start = 70KB - 48KB = 22KB` (positive). `plain` = 60KB. `start = 60KB - 48KB = 12KB`. Returns last 48KB — correct. The bug only manifests when `keep > len(plain)` but `keep <= len(out)`, which requires ANSI overhead > `len(out) - keep`. With `maxLiveOutput = 64KB` and `keptLiveOutput = 48KB`, the ANSI overhead must exceed 16KB for the bug to trigger. This is plausible with heavily styled output.
Threat model:
When `keep` is larger than `len(plain)` but smaller than `len(out)` (due to ANSI sequences), `safeOutputSuffix` returns the entire stripped output instead of the last `keep` bytes. This violates the memory bound, potentially causing the live output buffer to grow beyond `keptLiveOutput`.
Specific code citations:
Lines 150-152 compute `start` from `len(out)` and check `start <= 0`. Lines 160-161 recompute `start` from `len(plain)` but only after the early return on line 152 has already been bypassed.
Existing protections:
The `if start < 0` check on line 162 handles negative `start` but does not handle the case where `start` should be positive but `keep > len(plain)`.
Proposed mitigation:
After computing `plain`, check `if len(plain) <= keep { return plain }` before recomputing `start`.
Alternative mitigations considered:
Compute `start` from `len(plain)` first, then check `start <= 0`.
Severity calibration:
Score 4: violates the memory bound for live output when ANSI-heavy output is near the size limit. Could cause OOM under sustained streaming with ANSI sequences.
Suggested fix shape:
if len(plain) <= keep {
return plain
}
start = len(plain) - keep
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+355
to
+370
| p.Send(turnDoneMsg{}) | ||
| }() | ||
| result, err := p.Run() | ||
| <-done | ||
| // Keep the terminal attached until the canceled backend has actually | ||
| // stopped. Any late subprocess output is then discarded by the completed | ||
| // Bubble Tea program instead of escaping below the former input panel. | ||
| term.detachTurnProgram(p) | ||
| final, ok := result.(turnViewportModel) | ||
| if ok && final.output != nil { | ||
| // Bubble Tea only owns the live viewport. Re-emit the retained turn output | ||
| // once after it exits so normal terminal scrollback remains useful. | ||
| term.emit(final.output.String()) | ||
| } | ||
| if err != nil || !ok { | ||
| return TurnInputResult{} |
There was a problem hiding this comment.
Data race: m.result is written by the Update goroutine and read by RunTurnViewport without synchronization
RunTurnViewport calls p.Run() which runs the Bubble Tea event loop in a separate goroutine. The Update method writes to m.result (lines 100, 180, 193). After p.Run() returns, RunTurnViewport reads final.result on line 378. There is no happens-before relationship between the write in the Update goroutine and the read in RunTurnViewport. While p.Run() returning implies the event loop has stopped, the Go memory model does not guarantee visibility of writes made by the event loop goroutine to the calling goroutine without explicit synchronization. The fix is to use a channel or atomic value for the result.
Example:
The `Update` goroutine writes `m.result.Text = "user input"` and sends `tea.Quit`. `p.Run()` returns. The calling goroutine reads `final.result.Text` and observes `""` (the zero value) because the write was not flushed from the local cache.
Current:
p.Send(turnDoneMsg{})
}()
result, err := p.Run()
<-done
// Keep the terminal attached until the canceled backend has actually
// stopped. Any late subprocess output is then discarded by the completed
// Bubble Tea program instead of escaping below the former input panel.
term.detachTurnProgram(p)
final, ok := result.(turnViewportModel)
if ok && final.output != nil {
// Bubble Tea only owns the live viewport. Re-emit the retained turn output
// once after it exits so normal terminal scrollback remains useful.
term.emit(final.output.String())
}
if err != nil || !ok {
return TurnInputResult{}Proposed:
resultCh := make(chan TurnInputResult, 1)
// In Update, instead of writing to m.result:
resultCh <- TurnInputResult{...}
// In RunTurnViewport:
select {
case res := <-resultCh:
return res
case <-done:
return TurnInputResult{}
}
Suggested change
| p.Send(turnDoneMsg{}) | |
| }() | |
| result, err := p.Run() | |
| <-done | |
| // Keep the terminal attached until the canceled backend has actually | |
| // stopped. Any late subprocess output is then discarded by the completed | |
| // Bubble Tea program instead of escaping below the former input panel. | |
| term.detachTurnProgram(p) | |
| final, ok := result.(turnViewportModel) | |
| if ok && final.output != nil { | |
| // Bubble Tea only owns the live viewport. Re-emit the retained turn output | |
| // once after it exits so normal terminal scrollback remains useful. | |
| term.emit(final.output.String()) | |
| } | |
| if err != nil || !ok { | |
| return TurnInputResult{} | |
| resultCh := make(chan TurnInputResult, 1) | |
| // In Update, instead of writing to m.result: | |
| resultCh <- TurnInputResult{...} | |
| // In RunTurnViewport: | |
| select { | |
| case res := <-resultCh: | |
| return res | |
| case <-done: | |
| return TurnInputResult{} | |
| } |
More Info
- Threat model: Under the Go memory model, the read of
final.resulton line 378 may observe a stale or partially written value, causingRunTurnViewportto return an empty or corruptedTurnInputResult. - Specific code citations:
m.result.Text = texton line 180,m.result.Canceled = trueon lines 181 and 193, andreturn final.resulton line 378. - Existing protections: None.
p.Run()returning does not establish a happens-before edge for writes tom.result. - Proposed mitigation: Send the result through a channel that
RunTurnViewportreceives from, or usesync/atomicfor the result fields. - Alternative mitigations considered: Pass a pointer to a
TurnInputResultthat is only written beforetea.Quitand read afterp.Run()returns — this is still a data race per the Go memory model but is safe in practice on x86. A channel is the correct fix. - Severity calibration: Score 4: data race that can cause incorrect behavior (lost input) under the Go memory model. In practice, the race is unlikely to manifest on x86, but it is a real concurrency bug.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 355-370
Comment:
**Data race: `m.result` is written by the `Update` goroutine and read by `RunTurnViewport` without synchronization**
`RunTurnViewport` calls `p.Run()` which runs the Bubble Tea event loop in a separate goroutine. The `Update` method writes to `m.result` (lines 100, 180, 193). After `p.Run()` returns, `RunTurnViewport` reads `final.result` on line 378. There is no happens-before relationship between the write in the `Update` goroutine and the read in `RunTurnViewport`. While `p.Run()` returning implies the event loop has stopped, the Go memory model does not guarantee visibility of writes made by the event loop goroutine to the calling goroutine without explicit synchronization. The fix is to use a channel or atomic value for the result.
Example:
The `Update` goroutine writes `m.result.Text = "user input"` and sends `tea.Quit`. `p.Run()` returns. The calling goroutine reads `final.result.Text` and observes `""` (the zero value) because the write was not flushed from the local cache.
Threat model:
Under the Go memory model, the read of `final.result` on line 378 may observe a stale or partially written value, causing `RunTurnViewport` to return an empty or corrupted `TurnInputResult`.
Specific code citations:
`m.result.Text = text` on line 180, `m.result.Canceled = true` on lines 181 and 193, and `return final.result` on line 378.
Existing protections:
None. `p.Run()` returning does not establish a happens-before edge for writes to `m.result`.
Proposed mitigation:
Send the result through a channel that `RunTurnViewport` receives from, or use `sync/atomic` for the result fields.
Alternative mitigations considered:
Pass a pointer to a `TurnInputResult` that is only written before `tea.Quit` and read after `p.Run()` returns — this is still a data race per the Go memory model but is safe in practice on x86. A channel is the correct fix.
Severity calibration:
Score 4: data race that can cause incorrect behavior (lost input) under the Go memory model. In practice, the race is unlikely to manifest on x86, but it is a real concurrency bug.
Suggested fix shape:
resultCh := make(chan TurnInputResult, 1)
// In Update, instead of writing to m.result:
resultCh <- TurnInputResult{...}
// In RunTurnViewport:
select {
case res := <-resultCh:
return res
case <-done:
return TurnInputResult{}
}
How can I resolve this? If you propose a fix, please make it concise.
| // Preserve text that was still being composed when the backend | ||
| // completed. Do not overwrite a prompt already submitted by Enter if the | ||
| // backend's cancellation races with the queued turnDoneMsg. | ||
| if m.result.Text == "" { |
There was a problem hiding this comment.
turnDoneMsg handler trims whitespace from the draft text, losing intentional leading/trailing spaces
When the backend completes and sends turnDoneMsg, the handler preserves the draft text with m.result.Text = strings.TrimSpace(string(m.text)). TrimSpace removes leading and trailing whitespace, which discards intentional indentation or trailing spaces the user may have typed. The Enter key handler on line 179 also uses TrimSpace, which is correct for submitted input. But for a draft that is being preserved (not submitted), trimming is lossy. The fix is to preserve the raw text without trimming.
Example:
User types ` indented code` and the turn finishes before Enter. The preserved draft is `indented code` (leading spaces lost).
Suggested fix:
if m.result.Text == "" {
m.result.Text = string(m.text)
}More Info
- Threat model: A user types leading spaces (e.g., for code indentation) and the turn finishes before they press Enter. The preserved draft loses the indentation, forcing the user to retype it.
- Specific code citations: Line 100:
m.result.Text = strings.TrimSpace(string(m.text)). - Existing protections: None. The Enter handler also trims, which is correct for submitted input, but the
turnDoneMsghandler should preserve the raw draft. - Proposed mitigation: Change to
m.result.Text = string(m.text)(without TrimSpace) in theturnDoneMsghandler. - Alternative mitigations considered: None.
- Severity calibration: Score 3: minor data loss (whitespace) in an edge case (turn finishes while user is mid-edit). Not a crash or security issue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 101
Comment:
**`turnDoneMsg` handler trims whitespace from the draft text, losing intentional leading/trailing spaces**
When the backend completes and sends `turnDoneMsg`, the handler preserves the draft text with `m.result.Text = strings.TrimSpace(string(m.text))`. `TrimSpace` removes leading and trailing whitespace, which discards intentional indentation or trailing spaces the user may have typed. The Enter key handler on line 179 also uses `TrimSpace`, which is correct for submitted input. But for a draft that is being preserved (not submitted), trimming is lossy. The fix is to preserve the raw text without trimming.
Example:
User types ` indented code` and the turn finishes before Enter. The preserved draft is `indented code` (leading spaces lost).
Threat model:
A user types leading spaces (e.g., for code indentation) and the turn finishes before they press Enter. The preserved draft loses the indentation, forcing the user to retype it.
Specific code citations:
Line 100: `m.result.Text = strings.TrimSpace(string(m.text))`.
Existing protections:
None. The Enter handler also trims, which is correct for submitted input, but the `turnDoneMsg` handler should preserve the raw draft.
Proposed mitigation:
Change to `m.result.Text = string(m.text)` (without TrimSpace) in the `turnDoneMsg` handler.
Alternative mitigations considered:
None.
Severity calibration:
Score 3: minor data loss (whitespace) in an edge case (turn finishes while user is mid-edit). Not a crash or security issue.
Suggested fix shape:
if m.result.Text == "" {
m.result.Text = string(m.text)
}
How can I resolve this? If you propose a fix, please make it concise.
| m.frame++ | ||
| return m, turnViewportTick() | ||
| case tea.KeyMsg: | ||
| return m.updateKey(msg) |
There was a problem hiding this comment.
Missing test for nil output/live builder initialization race
The appendOutput method checks if m.output == nil and if m.live == nil to initialize builders, but there's no test covering this defensive initialization path. While the constructor initializes these fields, concurrent access or edge cases could trigger this path.
Example:
m := turnViewportModel{output: nil, live: nil}
m.appendOutput("test") // Should not panic
Suggested fix:
func TestTurnViewportAppendOutputHandlesNilBuilders(t *testing.T) {
m := newTurnViewportModel("test", nil, nil)
// Use reflection to set fields to nil
reflect.ValueOf(&m).Elem().FieldByName("output").Set(reflect.ValueOf((*strings.Builder)(nil)))
reflect.ValueOf(&m).Elem().FieldByName("live").Set(reflect.ValueOf((*strings.Builder)(nil)))
m.appendOutput("should not panic")
if m.output == nil || m.live == nil {
t.Fatal("appendOutput should initialize nil builders")
}
}Why this wasn't caught: No test exercises the nil initialization path in appendOutput.
More Info
- Threat model: If the model state becomes corrupted (e.g., through concurrent modification), the nil checks prevent panics but the behavior isn't verified.
- Specific code citations: Lines 110-115 in turn_viewport.go show the nil checks before writing to output and live builders.
- Existing protections: The constructor always initializes these fields, so the path may be unreachable in normal use.
- Proposed mitigation: Add a test that creates a model with nil output/live fields (via reflection or custom constructor) and verifies appendOutput handles it gracefully.
- Alternative mitigations considered: Could remove the nil checks if they're truly unreachable, but defensive programming is reasonable.
- Severity calibration: Score 3 because it's a defensive code path without test coverage, but likely low risk.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 110
Comment:
**Missing test for nil output/live builder initialization race**
The `appendOutput` method checks `if m.output == nil` and `if m.live == nil` to initialize builders, but there's no test covering this defensive initialization path. While the constructor initializes these fields, concurrent access or edge cases could trigger this path.
Example:
m := turnViewportModel{output: nil, live: nil}
m.appendOutput("test") // Should not panic
Threat model:
If the model state becomes corrupted (e.g., through concurrent modification), the nil checks prevent panics but the behavior isn't verified.
Specific code citations:
Lines 110-115 in turn_viewport.go show the nil checks before writing to output and live builders.
Existing protections:
The constructor always initializes these fields, so the path may be unreachable in normal use.
Proposed mitigation:
Add a test that creates a model with nil output/live fields (via reflection or custom constructor) and verifies appendOutput handles it gracefully.
Alternative mitigations considered:
Could remove the nil checks if they're truly unreachable, but defensive programming is reasonable.
Severity calibration:
Score 3 because it's a defensive code path without test coverage, but likely low risk.
Suggested fix shape:
func TestTurnViewportAppendOutputHandlesNilBuilders(t *testing.T) {
m := newTurnViewportModel("test", nil, nil)
// Use reflection to set fields to nil
reflect.ValueOf(&m).Elem().FieldByName("output").Set(reflect.ValueOf((*strings.Builder)(nil)))
reflect.ValueOf(&m).Elem().FieldByName("live").Set(reflect.ValueOf((*strings.Builder)(nil)))
m.appendOutput("should not panic")
if m.output == nil || m.live == nil {
t.Fatal("appendOutput should initialize nil builders")
}
}
Why this wasn't caught:
No test exercises the nil initialization path in appendOutput.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+175
to
+179
| text := strings.TrimSpace(string(m.text)) | ||
| if text == "" { | ||
| return m, nil | ||
| } | ||
| m.result.Text = text |
There was a problem hiding this comment.
Enter key submits even when m.result.Text is already set, potentially overwriting a previous submission
Low-confidence finding — expand to read
The Enter key handler on line 179 sets m.result.Text = text unconditionally. If turnDoneMsg has already set m.result.Text (the backend finished and preserved the draft), and then the Enter key event is processed (e.g., from a queued key event), the preserved draft is overwritten with the trimmed text. The turnDoneMsg handler has a guard (if m.result.Text == "") but the Enter handler does not. The fix is to add the same guard to the Enter handler.
More Info
- Threat model: If the backend finishes and preserves a draft, then a queued Enter key event overwrites it with a potentially different (trimmed) value.
- Specific code citations: Line 179:
m.result.Text = text(no guard). Line 100:if m.result.Text == ""(has guard). - Existing protections: The
turnDoneMsghandler guards against overwriting, but the Enter handler does not. - Proposed mitigation: Add
if m.result.Text != "" { return m, nil }before settingm.result.Textin the Enter handler. - Alternative mitigations considered: None.
- Severity calibration: Score 2: unlikely race condition (backend finishes AND Enter key is queued). Low impact (text overwrite, not data loss).
Example:
Backend finishes, `turnDoneMsg` sets `m.result.Text = "draft"`. Queued Enter key event fires, sets `m.result.Text = ""` (empty, because `m.text` was cleared by the backend completion). The submitted result is empty instead of the preserved draft.
Suggested fix:
if m.result.Text != "" {
return m, nil
}
m.result.Text = textPrompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 175-179
Comment:
**Enter key submits even when `m.result.Text` is already set, potentially overwriting a previous submission**
The Enter key handler on line 179 sets `m.result.Text = text` unconditionally. If `turnDoneMsg` has already set `m.result.Text` (the backend finished and preserved the draft), and then the Enter key event is processed (e.g., from a queued key event), the preserved draft is overwritten with the trimmed text. The `turnDoneMsg` handler has a guard (`if m.result.Text == ""`) but the Enter handler does not. The fix is to add the same guard to the Enter handler.
Example:
Backend finishes, `turnDoneMsg` sets `m.result.Text = "draft"`. Queued Enter key event fires, sets `m.result.Text = ""` (empty, because `m.text` was cleared by the backend completion). The submitted result is empty instead of the preserved draft.
Threat model:
If the backend finishes and preserves a draft, then a queued Enter key event overwrites it with a potentially different (trimmed) value.
Specific code citations:
Line 179: `m.result.Text = text` (no guard). Line 100: `if m.result.Text == ""` (has guard).
Existing protections:
The `turnDoneMsg` handler guards against overwriting, but the Enter handler does not.
Proposed mitigation:
Add `if m.result.Text != "" { return m, nil }` before setting `m.result.Text` in the Enter handler.
Alternative mitigations considered:
None.
Severity calibration:
Score 2: unlikely race condition (backend finishes AND Enter key is queued). Low impact (text overwrite, not data loss).
Suggested fix shape:
if m.result.Text != "" {
return m, nil
}
m.result.Text = text
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+148
to
+163
| } | ||
|
|
||
| func safeOutputSuffix(out string, keep int) string { | ||
| start := len(out) - keep | ||
| if start <= 0 { | ||
| return out | ||
| } | ||
| if newline := strings.IndexByte(out[start:], '\n'); newline >= 0 { | ||
| return out[start+newline+1:] | ||
| } | ||
|
|
||
| // A very long line has no safe textual boundary. Strip styling before | ||
| // slicing so an ANSI escape sequence can never be cut in half. | ||
| plain := ansi.Strip(out) | ||
| start = len(plain) - keep | ||
| if start < 0 { |
There was a problem hiding this comment.
safeOutputSuffix can return a string starting with a partial grapheme cluster when ANSI-stripped output begins with a combining character
Low-confidence finding — expand to read
The for start < len(plain) && !utf8.RuneStart(plain[start]) loop on line 163 advances past partial UTF-8 bytes but does not skip combining characters (zero-width joiners, variation selectors, combining diacritics). If plain[start] is a combining character, the returned string starts with a combining character that modifies the previous (now-truncated) base character, producing garbled output. The fix is to advance past combining characters as well, or to use a library that handles grapheme clusters.
More Info
- Threat model: If the truncated output begins with a combining character (e.g., a zero-width joiner or combining diacritic), the terminal renders garbled text.
- Specific code citations: Line 163:
for start < len(plain) && !utf8.RuneStart(plain[start])only checks for UTF-8 byte alignment, not grapheme cluster boundaries. - Existing protections: None. The loop only ensures
startis at a valid UTF-8 rune start, not a grapheme cluster start. - Proposed mitigation: Use
unicode.IsMarkor a grapheme-cluster-aware library to advance past combining characters. - Alternative mitigations considered: Accept the minor rendering glitch as unlikely with typical CLI output.
- Severity calibration: Score 2: cosmetic rendering issue with very unlikely input (combining characters at the truncation boundary in CLI output).
Example:
Output contains `e\u0301` (e + combining acute accent) at the truncation boundary. The returned string starts with `\u0301`, which combines with nothing, rendering as a standalone diacritic.
Suggested fix:
for start < len(plain) && (!utf8.RuneStart(plain[start]) || unicode.IsMark(rune(plain[start]))) {
start++
}Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 148-163
Comment:
**`safeOutputSuffix` can return a string starting with a partial grapheme cluster when ANSI-stripped output begins with a combining character**
The `for start < len(plain) && !utf8.RuneStart(plain[start])` loop on line 163 advances past partial UTF-8 bytes but does not skip combining characters (zero-width joiners, variation selectors, combining diacritics). If `plain[start]` is a combining character, the returned string starts with a combining character that modifies the previous (now-truncated) base character, producing garbled output. The fix is to advance past combining characters as well, or to use a library that handles grapheme clusters.
Example:
Output contains `e\u0301` (e + combining acute accent) at the truncation boundary. The returned string starts with `\u0301`, which combines with nothing, rendering as a standalone diacritic.
Threat model:
If the truncated output begins with a combining character (e.g., a zero-width joiner or combining diacritic), the terminal renders garbled text.
Specific code citations:
Line 163: `for start < len(plain) && !utf8.RuneStart(plain[start])` only checks for UTF-8 byte alignment, not grapheme cluster boundaries.
Existing protections:
None. The loop only ensures `start` is at a valid UTF-8 rune start, not a grapheme cluster start.
Proposed mitigation:
Use `unicode.IsMark` or a grapheme-cluster-aware library to advance past combining characters.
Alternative mitigations considered:
Accept the minor rendering glitch as unlikely with typical CLI output.
Severity calibration:
Score 2: cosmetic rendering issue with very unlikely input (combining characters at the truncation boundary in CLI output).
Suggested fix shape:
for start < len(plain) && (!utf8.RuneStart(plain[start]) || unicode.IsMark(rune(plain[start]))) {
start++
}
How can I resolve this? If you propose a fix, please make it concise.
| cache: &turnViewportCache{}, | ||
| width: 80, | ||
| height: 24, | ||
| cancelFn: cancelFn, |
There was a problem hiding this comment.
100ms tick interval may cause unnecessary CPU wakeups during idle streaming
The turnViewportTick function wakes the Bubble Tea event loop every 100ms via tea.Tick. During long agent turns with sparse output, this creates a steady stream of wakeups that perform frame increments and cache checks even when nothing has changed. For a 30-second turn, this is 300 unnecessary timer-driven updates.
Suggested fix:
return tea.Tick(250*time.Millisecond, func(t time.Time) tea.Msg { return turnTickMsg(t) })Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 73
Comment:
**100ms tick interval may cause unnecessary CPU wakeups during idle streaming**
The `turnViewportTick` function wakes the Bubble Tea event loop every 100ms via `tea.Tick`. During long agent turns with sparse output, this creates a steady stream of wakeups that perform frame increments and cache checks even when nothing has changed. For a 30-second turn, this is 300 unnecessary timer-driven updates.
Suggested fix shape:
return tea.Tick(250*time.Millisecond, func(t time.Time) tea.Msg { return turnTickMsg(t) })
How can I resolve this? If you propose a fix, please make it concise.
| return b.String() | ||
| } | ||
|
|
||
| func (m turnViewportModel) visibleOutput() string { |
There was a problem hiding this comment.
ANSI wrapping on cache miss may be expensive for large live output
Low-confidence finding — expand to read
When the cache is invalidated (revision change or width change), visibleOutput calls ansi.Wrap(out, width, "") on the entire m.live.String() content. With maxLiveOutput at 64KB, this is a full-string scan and wrap operation. Under rapid streaming with width changes (terminal resize), this could cause momentary CPU spikes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/tui/turn_viewport.go
Line: 299
Comment:
**ANSI wrapping on cache miss may be expensive for large live output**
When the cache is invalidated (revision change or width change), `visibleOutput` calls `ansi.Wrap(out, width, "")` on the entire `m.live.String()` content. With `maxLiveOutput` at 64KB, this is a full-string scan and wrap operation. Under rapid streaming with width changes (terminal resize), this could cause momentary CPU spikes.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Root cause
Streaming and tool output wrote directly to stdout after the input component exited. Those independent cursor writers could overwrite or displace the bordered input/status region as soon as streaming began.
Impact
The input panel now remains visible and editable throughout long agent turns. Live redraw and retained scrollback buffers are bounded independently, wrapping is cached between unchanged frames, and retained output is restored to normal terminal scrollback when the turn completes.
Validation
go test ./...go test -race ./internal/tui ./internal/agent ./internal/replgo vet ./...git diff --check