Feat/tui2 interface - #191
Conversation
… pane redesign Extract inline code from tui2.go into component packages: - help.Show(app,pages,bindings,onDismiss), command.ShowList(app,pages,entries,onSelect,onDismiss) - messages.Format(items,thinkingText,ctx), infopane.Format(state,theme) - contextinfo.Format(tokens,window,th), sessioninfo.Format(info,th) - Add task/sub-agent panes with autohide and dynamic fixed-height sizing - Add modal.Wrap() for consistent modal sizing across command/question/model picker Eighties neon theme: - Single-source theme.go: all colors, borders, tviewmd styling centralized - Heading (#00ffff), Detail (#cc99ff), Secondary (#9988bb), Dim (#888888) - 30 tool colors + 11 role colors brightened to neon equivalents - Pane borders: info=cyan, tasks=yellow, sub-agents=lavender, input=pink - Removed dead rolecolors.go, infobar, statusbar, mcp, tool, separator, messages sub-packages - Zero hardcoded hex colors outside theme.go Sub-agent lifecycle: - BackgroundJobs.CancelPending() for explicit bulk cancellation - Sub-agent blocks survive turns; hooks re-wired per-loop for cross-turn end events - Sub-agents pane shows running agents with name, elapsed time, task description - Question modal: fix Enter key in multiselect mode Info pane redesign: - Config section: Sub-agents (provider/model/concurrency), Embedding (active/inactive) - Middleware pipeline shown as vertical arrow list - Consistent Label: value format across all sections (no alignment padding) Build, vet, staticcheck, gofmt, and all tests pass.
… from TUI - Add Agent section to info pane showing active/idle status - Remove --mcp/--mcp-http from tui2 (use yaah serve for MCP) - Change Connected theme color to cyan for consistency - Fix background jobs pane height calculation
…lushed flushPendingTokens now returns bool. DoneEvent handler uses e.Response as a fallback when pendingTokens is empty — matching the old TUI's DoneEvent handler that catches non-streamed responses.
Split 753-line monolithic tui2.go into 20 focused files: tui2.go (140) — struct, constructor, config view.go (99) — layout assembly run.go (63) — lifecycle, ticker events.go (151) — agent event handling control.go (122) — control plane + right pane rendering blocks.go (113) — block operations + toggles commands.go (79) — command palette keymap.go (81) — keybindings input.go (53) — global key routing scroll.go (62) — conversation rendering markdown.go (48) — markdown rendering followup.go (41) — input submit + clear helpers_subagent.go (38) — sub-agent helpers modals.go (37) — modal wrappers panes.go (34) — right-pane updates state.go (30) — focus state + search thinking.go (27) — thinking indicator banner.go (24) — banner toggle helpers_tool.go (24) — tool helpers helpers_msg.go (22) — message helpers Removed: plainMessages (dead state, never read) Removed: 20+ duplicate methods across split files Deleted: stale theme.go (duplicate of colors/theme.go)
- Delete docs/otel-setup.md (OpenObserve-specific guide) - Update docker-compose.yml: remove openobserve service, wire yaah to SigNoz collector (host.docker.internal:4318) - Update README.md, docs/features.md, docs/configuration.md: replace all OpenObserve references with SigNoz - Add SigNoz Docker install link (signoz.io/docs/install/docker/) - Add foundry casting.yaml + pours/ for SigNoz infra-as-code
…reshes Remove the animated spinner ticker that drove full conversation rebuilds at 5-15 Hz even when nothing changed. Replace with event-driven dirty-flag debounce (markDirty/flushRefresh) that coalesces rapid event bursts into a single render pass. TUI2 (tview): - Remove startSpinnerTicker() goroutine (was 5 QueueUpdateDraw/sec) - Add startDebounceTimer() — 100ms tick flushes needsRefresh flag - Add markDirty()/flushRefresh() to scroll.go, wired to all callers - Replace 15 refreshMessages() call sites with markDirty() - Switch Compaction/Escalation events from QueueUpdateDraw to lighter QueueUpdate - Remove dead conversationCache field TUI1 (bubbletea): - Spinner tick no longer calls refreshViewport() unconditionally - Add markDirty() with renderGeneration tracking for cache invalidation - All event handlers use markDirty() instead of direct refresh/scroll - Add renderMessages() cache: returns cached output when UI is idle and no state has changed (generation+msgCount+expandState key) Shared: - Increase BrokerView subscriber buffer 256 → 4096 (match broker default) Before: ~30 full O(n) conversation rebuilds per complex agent turn. After: ~1-2 rebuilds (coalesced by 100ms debounce + spinner tick).
14 new test files, 170+ test cases covering: - colors/theme.go: Tag, TagBold, Detect, ToolHex, RoleHex, NoColor - thinking: Indicator lifecycle, Render, Advance, Spinner - reasoning: Block toggle, expand/collapse, RenderCtx - toolblock: Icon catalog, state transitions, duration, Render per state - subagent: lifecycle, blink, spinner, Render states, elapsed - messages: Format with text/tool/subagent/reasoning blocks - infopane: Format all State field combinations - todo: Format/FormatList, all statuses and priorities - backgroundjobs: empty, running only, mixed states - command: Parse all 18 commands + edge cases - sessioninfo: Format, shortVersion - contextinfo: window/no-window, percentage cap - mcpinfo: connected/disconnected, multiple servers - keymap: Translate, Match, DefaultBindings - markdown: renderMarkdown, messageWidth All tests pass. gofmt clean. staticcheck clean.
Phase 1 code reorganization: - Split monolithic tui2.go into focused files (proxy.go, usage.go, blocks.go) - Eliminate duplicated state (conversationLog as single source of truth) - Rename events.go to proxy.go (agent.View bridge) - Delete helpers_tool.go and helpers_subagent.go (moved to blocks.go) New features: - usage.go: cumulative token/cost tracking with model pricing table - components/error: error overlay modal with auto-dismiss Bug fixes: - usage.go: fix map iteration randomness by using longest prefix match - blocks.go: AddToolEnd always calls Complete (empty results are valid) - proxy.go: route errors through AddToolError, not AddToolEnd - control.go: label cost estimate as 'at current model rates' - error/error.go: fix goroutine leak with time.AfterFunc and timer cleanup Unit tests: - usage_test.go: 6 tests for cost calculation and usage tracking - blocks_test.go: 14 tests for block operations - error/error_test.go: 14 tests for error overlay component Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
…racking - Mark Phase 1, 2, 3, and 7 as complete - Add progress table showing status of each phase - Add latest commit reference - Update current state description Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Add short-lived OTel spans for turn responses and tool dispatch in the agent loop, ensuring they are exported even if the parent turn span is lost due to a crash. This provides visibility into agent execution during failures. In the TUI2 event proxy, most event handlers now run in goroutines to avoid blocking the event forwarder, while token delta handling remains synchronous to preserve ordering. This improves UI responsiveness. Instrument refreshMessages with timing and OTel spans, and log a warning when a refresh takes over 50ms to aid performance diagnostics. Also include a new plan file for loop refactoring under .agents/plans/loop-refactoring/.
Verify that HandleEvent returns without blocking for all async event types (Thinking, Flush, ToolStart/End, SubAgentStart/End, Escalation, Compaction, Done). Confirm that TokenDeltaEvent remains synchronous for token ordering preservation. Six tests guard against regression of the go QueueUpdate fix that was required because tview QueueUpdate blocks the forwarder goroutine until the main thread completes draw().
…agnostics - Wire SetApproveFn in tui2 session (was never called) so bash approval shows a modal instead of blocking on stdin forever. Tview captures stdin so the fallback scanner.Scan() in approveTool hung the agent goroutine indefinitely. - Write TokenDeltaEvent stream directly to pendingTokens from the forwarder goroutine via mutex, eliminating all QueueUpdate calls during streaming. This prevents the 100-slot tview updates channel from flooding and the main thread from being pinned in draw() between callbacks. - Add RecordToolGoroutine diagnostic spans inside the tool dispatch goroutine with explicit span.End() (no defer) so checkpoints are exported even when the goroutine blocks. - Add ShowApprovalFn override field for testing modal display without a running tview app. - Tests: 3 HandleControlMsg approval/continue tests, 4 approveTool integration tests, updated async TokenDelta tests.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change expands TUI2 feature parity, restructures rendering and event handling, adds SigNoz deployment and telemetry support, documents loop refactoring plans, and optimizes refresh behavior in the legacy TUI. ChangesTUI2 feature parity
SigNoz observability
Legacy TUI refresh optimization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui2/proxy.go (1)
26-101: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSerialize lifecycle UI callbacks.
QueueUpdateis FIFO only in callback submission order. Separate goroutines can submitToolEndEventbeforeToolStartEvent, orSubAgentEndEventbeforeSubAgentStartEvent. The end operation then silently finds no block, and the later start leaves an active block visible. Route lifecycle events through one FIFO dispatcher that submits callbacks inHandleEventorder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/proxy.go` around lines 26 - 101, The lifecycle callbacks in HandleEvent are submitted from separate goroutines, allowing ToolStartEvent/ToolEndEvent and SubAgentStartEvent/SubAgentEndEvent to arrive out of order. Route these lifecycle event callbacks through a single FIFO dispatcher that submits them in HandleEvent order, while preserving the existing callback behavior and ensuring end events cannot precede their corresponding start events.
🟡 Minor comments (25)
internal/agent/loop.go-203-204 (1)
203-204: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRecord the actual result count.
Line 204 passes the requested tool-call count as
resultCount.RecordToolDispatchDoneemits this value asdispatch.results. IfexecuteToolPhasestops before it creates a result for every call, the span reports results that do not exist.Return the number of produced results from
executeToolPhaseand pass it here. Otherwise, rename the parameter and telemetry attribute to represent attempted tool calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop.go` around lines 203 - 204, Update executeToolPhase and its caller in the loop to return and propagate the actual number of produced tool results, then pass that count to observability.RecordToolDispatchDone instead of len(msg.ToolCalls). Ensure the telemetry dispatch.results value reflects results created when execution stops early.internal/tui/render.go-336-338 (2)
336-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude terminal dimensions in the render cache key.
renderMessagesusesm.widthandm.viewport.Height(), but the cache does not store or compare them. After a resize,WindowSizeMsgrefreshes the viewport without changingrenderGeneration, so completed content can remain formatted for the previous dimensions. Store and compare both dimensions, and add a resize regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/render.go` around lines 336 - 338, Update renderMessages and its cache state to include both m.width and m.viewport.Height() in the cache key, comparing them before returning cachedMessages so resized terminals rerender completed content. Persist the current dimensions when storing the cache, and add a regression test covering a resize via WindowSizeMsg.
336-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalidate the render cache when expansion state changes.
refreshViewport()does not callmarkDirty(), andcachedExpandStateuses only map lengths. Toggling an existing zone can therefore return stale cached output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/render.go` around lines 336 - 338, Update the render-cache validation in the rendering method containing expandState so cachedExpandState reflects which expansion zones are open, not only the lengths of subagentExpanded, toolExpanded, and reasoningExpanded. Ensure toggling an existing zone changes the computed state and invalidates the cached output, including when refreshViewport() does not call markDirty()..agents/plans/tui2-feature-parity/PLAN.md-236-239 (1)
236-239: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn keymap conflicts as errors.
The plan says
Panic at startup if a conflict is found. Make validation return an error and let the startup path report it. This avoids process-level crashes and keeps conflict details testable.As per coding guidelines, Go code must use errors as values rather than panicking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 236 - 239, Update keymap validation in keymap.go init to return a descriptive error when multiple actions share a key combination instead of panicking. Propagate that error through the startup path and report it there, preserving conflict details so callers and tests can inspect the returned error.Source: Coding guidelines
.agents/plans/tui2-feature-parity/PLAN.md-333-342 (1)
333-342: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required Go quality gates.
The plan lists only
go build ./...after each phase. Addgofmt -l .,go vet ./..., and Staticcheck to the phase exit criteria. A successful build does not cover these checks.As per coding guidelines, Go changes must pass
gofmt -l .,go vet ./..., and Staticcheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 333 - 342, Add gofmt -l ., go vet ./..., and Staticcheck to the phase exit criteria alongside go build ./.... Update the quality-gate guidance in the plan so every phase requires all four checks to pass.Source: Coding guidelines
.agents/plans/tui2-feature-parity/PLAN.md-187-199 (1)
187-199: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not make
modelPricesa package global.The proposed
var modelPricesconflicts with the repository rule that permits only approved global registries and metric instruments. Store pricing in the usage tracker or expose a lookup function backed by local immutable data.As per coding guidelines, globals are limited to build-time, serve-mode, role-registry, and initialized OTel metric instruments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 187 - 199, The proposed modelPrices package global violates the repository’s global-variable restrictions. Revise the usage pricing design so pricing is stored within the usage tracker or accessed through a lookup function backed by local immutable data, while preserving the model-to-input/output price mappings and estimate behavior.Source: Coding guidelines
.agents/plans/tui2-feature-parity/PLAN.md-1-4 (1)
1-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove task tracking out of this Markdown plan.
The document stores work state in
status: in-progress,TODOentries, andFile a follow-up issue. The repository rule requires Beads for task tracking. Keep architectural decisions here, but track phase status and follow-up work withbdissue IDs.As per coding guidelines,
*.mdfiles must use Beads for task tracking and must not use Markdown TODO lists.Also applies to: 31-42, 282-286
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 1 - 4, Remove task-tracking state from the tui2-feature-parity plan, including the frontmatter status, Markdown TODO entries, and “File a follow-up issue” sections. Preserve architectural decisions and phase descriptions, and reference the corresponding Beads issue IDs for tracked work instead of maintaining task status in the Markdown file.Source: Coding guidelines
.agents/plans/tui2-feature-parity/PLAN.md-19-24 (1)
19-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse one name for the canonical conversation state.
The plan calls the source
conversationLog, but the proposedAppstruct usesconversation.items. Phase 5 also describesconversationLogas separate state. Choose one identifier and use it in every section to avoid reintroducing duplicate state.Also applies to: 155-162, 243-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 19 - 24, Standardize the plan on a single canonical conversation-state identifier, preferably conversationLog, and replace references to conversation.items throughout the App struct, Phase 5, and the additional affected sections. Describe conversationLog as the sole source of truth, not separate state, and remove wording that implies duplicate conversation storage..agents/plans/tui2-feature-parity/PLAN.md-45-47 (1)
45-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to all fenced blocks.
These three fences trigger Markdownlint MD040. Use
textfor the commit, tree, and execution-order diagrams.Also applies to: 63-101, 317-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 45 - 47, Add the text language identifier to every fenced code block in PLAN.md referenced by this change, including the commit, tree, and execution-order diagram blocks around the visible commit entry and the additional ranges. Preserve each block’s contents while ensuring all fences satisfy Markdownlint MD040.Source: Linters/SAST tools
README.md-94-105 (1)
94-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMap
host.docker.internalon native Linux.
docker-compose.ymlsetsOTEL_EXPORTER_OTLP_ENDPOINT=host.docker.internal:4318, but native Docker Engine on Linux does not provide this hostname by default. Add thehost-gatewaymapping, or state that Docker Desktop is required.Proposed Compose change
services: yaah: + extra_hosts: + - "host.docker.internal:host-gateway"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 94 - 105, Update the README’s SigNoz Docker setup instructions to address native Linux resolution of host.docker.internal: either document the required host-gateway mapping in docker-compose.yml or explicitly state that Docker Desktop is required, while keeping the existing compose commands and tracing setup guidance intact.pours/deployment/ingester/ingester.yaml-17-17 (1)
17-17: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDefine
LOW_CARDINAL_EXCEPTION_GROUPINGforingester.The unset variable provides no boolean default and can cause exporter configuration validation to fail. Set it to the intended
trueorfalsevalue inpours/deployment/compose.yaml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pours/deployment/ingester/ingester.yaml` at line 17, Define LOW_CARDINAL_EXCEPTION_GROUPING in the ingester service environment within compose.yaml with the intended explicit true or false value, so the ingester configuration receives a valid boolean instead of an unset variable.internal/tui2/commands.go-55-66 (1)
55-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPalette entries that need an argument silently do nothing.
showCommandListalways callsHandleCommand(cmd, "").command.DefaultEntriesincludessearchandsteer, and both are labelled "requires arg".HandleCommandskipsCmdSteerwhenargis empty, andsearchMessagesreturns immediately on an empty query. A user who selects either entry from the palette sees no result and no message.Prompt for the argument, or prefill the input with the command text.
🐛 Minimal fix: prefill the input instead of dispatching
command.ShowList(t.App, t.Pages, entries, func(cmd command.Cmd) { t.App.SetFocus(t.Input) t.focus = focusNormal + if cmd == command.CmdSearch || cmd == command.CmdSteer { + t.SetEphemeral("This command needs an argument. Type it in the input.") + return + } t.HandleCommand(cmd, "") }, func() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/commands.go` around lines 55 - 66, Update showCommandList so selecting an argument-requiring command such as search or steer does not dispatch HandleCommand with an empty argument; prefill the input with the selected command text and return focus to the input, while preserving the existing immediate dispatch behavior for commands that do not require arguments.internal/tui2/view.go-27-33 (1)
27-33: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winCount wrapped display rows before clearing
userScrolled.When a long message line wraps,
GetScrollOffsetcounts display rows, butstrings.Splitandstrings.Countcount only newline-delimited lines. UseGetWrappedLineCount()or maintain an equivalent cached count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/view.go` around lines 27 - 33, Update the MouseScrollDown handling to calculate totalLines using t.Messages.GetWrappedLineCount() (or an equivalent display-row count) instead of splitting GetText(true) on newlines, then retain the existing row+h boundary check before clearing t.userScrolled.internal/tui2/components/toolblock/toolblock_test.go-176-184 (1)
176-184: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe seconds assertion cannot fail for a millisecond value.
strings.Contains(b.durationStr(), "s")matches"10ms"as well as"2.0s". The test passes even if the seconds branch ofdurationStris broken or removed. Assert the exact expected string, or assert that the output does not contain"ms".💚 Proposed fix
func TestDurationStr_Second(t *testing.T) { th := colors.NewDarkTheme() b := New("t1", "bash", `{}`, &th) b.startTime = time.Now().Add(-2 * time.Second) b.Complete("done", "") - if !strings.Contains(b.durationStr(), "s") { - t.Errorf("second duration should show s, got %q", b.durationStr()) - } + got := b.durationStr() + if strings.Contains(got, "ms") { + t.Errorf("second duration should not use ms units, got %q", got) + } + if !strings.HasSuffix(got, "s") { + t.Errorf("second duration should end with s, got %q", got) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/toolblock/toolblock_test.go` around lines 176 - 184, Strengthen TestDurationStr_Second by asserting the seconds-format output from durationStr rather than merely checking for “s”, which also matches millisecond values. Use an exact expected value if timing is deterministic, or at minimum verify the result excludes “ms” while retaining the seconds assertion.internal/tui2/colors/theme_test.go-40-47 (1)
40-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the environment after the test.
os.Unsetenvchanges process state for the remaining tests in this package and never restores it. Callt.Setenvfirst for each variable, then unset.t.Setenvregisters a cleanup that restores the original value.♻️ Proposed fix
func TestDetectTheme_DefaultDark(t *testing.T) { - os.Unsetenv("YAARH_THEME") - os.Unsetenv("NO_COLOR") + t.Setenv("YAARH_THEME", "") + t.Setenv("NO_COLOR", "") + os.Unsetenv("YAARH_THEME") + os.Unsetenv("NO_COLOR") th := DetectTheme()Note that
DetectThemeusesos.LookupEnv("NO_COLOR"), so an empty value still setsNoColor. The explicit unset aftert.Setenvis required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/colors/theme_test.go` around lines 40 - 47, Update TestDetectTheme_DefaultDark to call t.Setenv for both YAARH_THEME and NO_COLOR before unsetting them, then retain the explicit os.Unsetenv calls so empty values do not affect DetectTheme. This ensures t.Setenv restores each variable’s original process state after the test.internal/tui2/components/modelpicker/modelpicker.go-103-112 (1)
103-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the duplicate border and title.
listalready sets a border and the title" Model Picker "at lines 50-52.innernow sets the same border and title around the list and the filter. The result renders two nested boxes with the same title. Keep the border and title oninneronly, and drop them fromlist.🐛 Proposed fix (applies outside the selected range)
// Lines 50-52: remove the border and title from the list. list.SetInputCapture(func(ev *tcell.EventKey) *tcell.EventKey { // ... })- list.SetBorder(true). - SetTitle(" Model Picker "). - SetTitleColor(tcell.ColorYellow) -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/modelpicker/modelpicker.go` around lines 103 - 112, Remove the border and “ Model Picker ” title setup from the list initialization, while retaining the border and title configured on inner before adding it to pages. Keep list’s input capture and other behavior unchanged.internal/tui2/scroll.go-92-95 (1)
92-95: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRedirect the default logger during TUI2 startup.
runTUI2leaves the default logger pointed atos.Stderr, so slow-refresh messages can corrupt the tview screen. Apply the/dev/nullredirection used byrunTUIand restore it on exit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/scroll.go` around lines 92 - 95, Update runTUI2 startup to redirect the default logger’s output to /dev/null, matching the existing runTUI behavior, and restore the original logger destination when runTUI2 exits. Keep the slow-refresh logging in refreshMessages unchanged.internal/tui2/components/help/help.go-31-42 (1)
31-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winList all supported commands in the help overlay.
The reference omits
:followup,:roles,:login,:logout,:session, and:mcp.command.Parseaccepts these commands, so users cannot discover them through:help.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/help/help.go` around lines 31 - 42, Update the help overlay command list in the help rendering code to include the supported :followup, :roles, :login, :logout, :session, and :mcp commands, matching the commands accepted by command.Parse and preserving the existing formatting.internal/tui2/components/command/command_test.go-7-14 (1)
7-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap each input case in a named subtest. Use
t.Run(input, func(t *testing.T) { ... })inTestParse_Quit,TestParse_Help, andTestParse_Modelso failures identify the command variant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/command/command_test.go` around lines 7 - 14, Update TestParse_Quit, TestParse_Help, and TestParse_Model to wrap each input iteration in a named t.Run(input, func(t *testing.T) { ... }) subtest, keeping each existing Parse assertion inside its corresponding subtest.Source: Coding guidelines
internal/tui2/usage.go-79-80 (1)
79-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a model-version boundary for fallback pricing.
The current prefix check prices
gpt-4.1asgpt-4, even thoughgpt-4.1is not a datedgpt-4version. Only accept an exact name or aname + "-"version suffix.Proposed fix
+import "strings" + - if modelName[:len(name)] == name && len(name) > len(bestMatch) { + if strings.HasPrefix(modelName, name+"-") && len(name) > len(bestMatch) { bestMatch = name }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/usage.go` around lines 79 - 80, Update the model-name matching condition around bestMatch so fallback pricing accepts only an exact model name or a modelName beginning with name followed by “-”. Replace the unrestricted prefix check while preserving the existing longest-match selection and avoiding false matches such as gpt-4.1 for gpt-4.internal/tui2/panes.go-24-33 (1)
24-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the newest ephemeral message until its timer expires.
Each call starts an independent clear timer. If a second message replaces the first after one second, the first timer clears the second message after two more seconds.
Track a message generation and clear only when the generation still matches.
Proposed fix
+// Add to TUI2. Access this field only through QueueUpdateDraw-owned updates. +ephemeralGeneration uint64 + func (t *TUI2) SetEphemeral(msg string) { t.ephemeralMsg = msg + t.ephemeralGeneration++ + generation := t.ephemeralGeneration t.renderInfoPane() go func() { time.Sleep(3 * time.Second) t.App.QueueUpdateDraw(func() { + if generation != t.ephemeralGeneration { + return + } t.ephemeralMsg = "" t.renderInfoPane() }) }() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/panes.go` around lines 24 - 33, The SetEphemeral method starts independent timers that can clear newer messages prematurely. Add a message-generation or equivalent identity check in SetEphemeral, capture the generation for each scheduled timer, and clear ephemeralMsg only when that generation still matches the current one; otherwise leave the newest message and its timer intact.internal/tui2/components/thinking/thinking_test.go-126-136 (1)
126-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the
seedtransition.This test only checks that rendering is non-empty. It passes if
Advancestops incrementingseed. Record the initial value and assert the expected increment after three calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/thinking/thinking_test.go` around lines 126 - 136, Update TestAdvance_SeedIncrements to capture the indicator’s initial seed before calling Advance, then assert that three Advance calls produce the expected seed increment. Retain the existing render assertion only if still relevant, and use the indicator’s established seed symbol or accessor.internal/tui2/components/infopane/infopane.go-111-117 (1)
111-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender
EmbeddingInfo.Modelwhen embeddings are enabled.
EmbeddingInfostoresModelfor display, but this branch only rendersactive. Show the model when it is non-empty.Proposed fix
if s.Embedding.Enabled { b.WriteString(th.TagBold(th.Heading, "Embedding\n")) b.WriteString(fmt.Sprintf(" Status: %s\n", th.Tag(th.Detail, "active"))) + if s.Embedding.Model != "" { + b.WriteString(fmt.Sprintf(" Model: %s\n", th.Tag(th.Detail, s.Embedding.Model))) + } } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/infopane/infopane.go` around lines 111 - 117, Update the enabled branch of the embedding display in EmbeddingInfo so it continues to show the active status and also renders EmbeddingInfo.Model when the model value is non-empty. Leave the inactive branch unchanged.internal/tui2/components/command/command.go-99-101 (1)
99-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle argument commands from the command palette.
showCommandListpasses an empty argument toHandleCommand, so selectingsteerdoes nothing and selectingsearchreturns without searching. Add an argument-input flow or remove these entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/command/command.go` around lines 99 - 101, Update showCommandList and its command-palette handling so argument-requiring commands steer and search prompt for and pass a user-provided argument to HandleCommand; alternatively remove the steer and search entries from the palette if argument input cannot be supported. Keep no-argument commands unchanged.internal/tui2/components/banner/banner.go-37-37 (1)
37-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the
WriteString(fmt.Sprintf(...))call withfmt.Fprintf.This expression triggers Staticcheck QF1012.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/banner/banner.go` at line 37, Update the banner rendering code around the WriteString call to use fmt.Fprintf with the builder as the destination, preserving the existing "[%s::d]%s[-]" formatting and dimColor/tagline values. Remove the redundant fmt.Sprintf construction so the Staticcheck QF1012 warning is resolved.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (17)
internal/tui/model.go (1)
421-431: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCoalesce the two refresh paths.
If
needsRefreshandpendingRefreshare both true, this tick callsrefreshViewporttwice. ClearpendingRefreshafter the streaming or thinking refresh, because that refresh already includes the pending state.Proposed change
- if m.thinking && m.needsRefresh { - m.refreshViewport() - m.scrollToBottom() - m.needsRefresh = false - } - if m.needsRefresh && m.streaming { + if m.needsRefresh && (m.thinking || m.streaming) { m.refreshViewport() m.scrollToBottom() m.needsRefresh = false + m.pendingRefresh = false } m.flushPendingRefresh()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 421 - 431, Coalesce the refresh handling in the model update flow by clearing pendingRefresh whenever the thinking or streaming refresh path runs, since refreshViewport already incorporates that state. Update the adjacent conditions around m.thinking, m.streaming, and m.needsRefresh so flushPendingRefresh does not trigger a second refresh in the same tick.internal/tui2/tui2.go (2)
61-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing group markers for the atomic fields.
The doc comment at lines 20-23 declares three field categories, and line 42 marks the start of the
QueueUpdateDrawgroup. The block from line 61 onward mixesQueueUpdateDraw-only fields with atomic fields (tokensRx,charsWritten,charsRendered,isStreaming,needsRefresh) and no marker separates them. A reader cannot tell which fields are safe to touch from a non-UI goroutine.♻️ Suggested grouping markers
userScrolled bool + // --- Atomic / immutable fields below — safe from any goroutine --- + isStreaming atomic.Bool needsRefresh atomic.BoolAlso move
tokensRx,charsWritten, andcharsRenderedinto that group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/tui2.go` around lines 61 - 96, Add the missing group markers in the TUI state struct to distinguish QueueUpdateDraw-only fields from atomic fields. Move tokensRx, charsWritten, and charsRendered into the existing atomic-field group with isStreaming and needsRefresh, and place the appropriate marker before that group while preserving the existing field behavior.
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the seven positional parameters with a config struct.
SetConfigtakes adjacent parameters of the same type, for examplesubAgentsProviderandsubAgentsModel. A caller can transpose them and the compiler will not report an error. A named struct makes each call site self-documenting and prevents that class of bug.♻️ Proposed refactor
+// Config carries the display-only configuration shown in the info pane. +type Config struct { + SubAgentsEnabled bool + SubAgentsProvider string + SubAgentsConcurrency int + SubAgentsModel string + EmbeddingEnabled bool + EmbeddingModel string + MiddlewarePipeline []string +} + -func (t *TUI2) SetConfig(subAgentsEnabled bool, subAgentsProvider string, subAgentsConcurrency int, subAgentsModel string, embeddingEnabled bool, embeddingModel string, pipeline []string) { - t.subAgentsEnabled = subAgentsEnabled - t.subAgentsProvider = subAgentsProvider - t.subAgentsConcurrency = subAgentsConcurrency - t.subAgentsModel = subAgentsModel - t.embeddingEnabled = embeddingEnabled - t.embeddingModel = embeddingModel - t.middlewarePipeline = pipeline +func (t *TUI2) SetConfig(cfg Config) { + t.subAgentsEnabled = cfg.SubAgentsEnabled + t.subAgentsProvider = cfg.SubAgentsProvider + t.subAgentsConcurrency = cfg.SubAgentsConcurrency + t.subAgentsModel = cfg.SubAgentsModel + t.embeddingEnabled = cfg.EmbeddingEnabled + t.embeddingModel = cfg.EmbeddingModel + t.middlewarePipeline = cfg.MiddlewarePipeline t.renderInfoPane() }This changes the call site in
cmd/yaah/tui2.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/tui2.go` around lines 131 - 140, Replace the positional arguments of TUI2.SetConfig with a named configuration struct containing all seven settings, then update SetConfig to read from that struct and update every caller, including cmd/yaah/tui2.go, to construct it with named fields.internal/tui2/components/error/error.go (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the package so it does not reuse the
erroridentifier.The package name
errormatches the predeclared Go type. Every importer must either alias the import or writeerror.Error{...}, which reads as iferrorwere the builtin. A name such aserroruiorerrmodalremoves that ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/error/error.go` around lines 1 - 2, Rename the package declaration from error to a non-conflicting name such as errorui, and update all references to its exported symbols accordingly so importers no longer need to use the predeclared error identifier as a package name.internal/tui2/followup.go (1)
4-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared submit logic.
submitInputandsubmitFollowUpdiffer only in the callback they invoke. Two copies of the guard, flush, and clear sequence will drift.♻️ Proposed refactor
-// submitInput sends the current input text as a new prompt, if any. -func (t *TUI2) submitInput() { - if t.OnSubmit != nil { - text := t.Input.GetText() - if text != "" { - t.flushRefresh() - t.Input.SetText("", false) - t.OnSubmit(text) - } - } -} - -// submitFollowUp sends the current input text as a follow-up, if any. -func (t *TUI2) submitFollowUp() { - if t.OnFollowUp != nil { - text := t.Input.GetText() - if text != "" { - t.flushRefresh() - t.Input.SetText("", false) - t.OnFollowUp(text) - } - } -} +// submitInput sends the current input text as a new prompt, if any. +func (t *TUI2) submitInput() { t.submitTo(t.OnSubmit) } + +// submitFollowUp sends the current input text as a follow-up, if any. +func (t *TUI2) submitFollowUp() { t.submitTo(t.OnFollowUp) } + +// submitTo drains the input and passes it to cb, if both are non-empty. +func (t *TUI2) submitTo(cb func(string)) { + if cb == nil { + return + } + text := t.Input.GetText() + if text == "" { + return + } + t.flushRefresh() + t.Input.SetText("", false) + cb(text) +}This also replaces the nested
ifblocks with early returns.As per coding guidelines: "Prefer early returns over nested
if/elsestatements."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/followup.go` around lines 4 - 25, Extract the duplicated guard, flush, input-clearing, and callback invocation from TUI2.submitInput and TUI2.submitFollowUp into a shared helper that accepts the appropriate callback. Refactor both methods to use early returns for nil callbacks and empty input while preserving their existing OnSubmit and OnFollowUp behavior.Source: Coding guidelines
internal/tui2/colors/theme.go (2)
216-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelegate
SecondaryTagtoColorTag.
SecondaryTagrepeats theNoColorcheck and the tag construction thatColorTagalready performs. Delegation keeps one implementation of the tag format.♻️ Proposed refactor
func (th *Theme) SecondaryTag() string { - if th.NoColor { - return "" - } - return "[" + th.Secondary + "]" + return th.ColorTag(th.Secondary) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/colors/theme.go` around lines 216 - 221, Update Theme.SecondaryTag to delegate tag generation to the existing ColorTag method, passing th.Secondary, and remove its duplicated NoColor check and string construction while preserving the current output.
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the field comments with the values.
The comment on
Connectedstates green, but the dark theme sets#00ffff(cyan) and the light theme sets#005faf(blue). The comment onUserstates hot pink, but the dark theme sets#ff00ff(magenta). The struct doc claims it is the single source of truth for the look, so correct comments matter here.Also applies to: 109-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/colors/theme.go` around lines 22 - 23, Update the `Connected` and `User` field comments in the theme struct to match the actual dark and light theme colors: describe `Connected` as cyan/blue rather than green, and `User` as magenta rather than hot pink. Leave the theme values unchanged and ensure the comments accurately reflect both theme variants where they differ.internal/tui2/components/toolblock/toolblock_test.go (2)
166-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet
startTimeinstead of sleeping.
time.Sleep(10 * time.Millisecond)adds wall-clock dependency to the test.TestDurationStr_Secondat line 179 already setsb.startTimedirectly. Use the same deterministic approach here.♻️ Proposed refactor
b := New("t1", "bash", `{}`, &th) - time.Sleep(10 * time.Millisecond) + b.startTime = time.Now().Add(-10 * time.Millisecond) b.Complete("done", "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/toolblock/toolblock_test.go` around lines 166 - 174, Update TestDurationStr_SubSecond to assign b.startTime directly, matching the deterministic setup used by TestDurationStr_Second, and remove the time.Sleep call while preserving the millisecond-duration assertion.
11-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
t.Runfor the table cases.The loop runs the table without subtests. A failure reports only the parent test name. Wrap each case in
t.Run(tc.name, ...).♻️ Proposed refactor
for _, tc := range tests { - if got := Icon(tc.name); got != tc.want { - t.Errorf("Icon(%q) = %q, want %q", tc.name, got, tc.want) - } + t.Run(tc.name, func(t *testing.T) { + if got := Icon(tc.name); got != tc.want { + t.Errorf("Icon(%q) = %q, want %q", tc.name, got, tc.want) + } + }) }As per coding guidelines: "Place tests next to the code they test and use
t.Run("name", func(t *testing.T) { ... })for subtests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/toolblock/toolblock_test.go` around lines 11 - 25, Update TestIcon_Known to wrap each table-driven case in t.Run(tc.name, func(t *testing.T) { ... }), keeping the existing Icon assertion inside the subtest.Source: Coding guidelines
internal/tui2/components/contextinfo/contextinfo_test.go (1)
32-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a positive assertion for the capped percentage.
The test only asserts that
200.0%is absent. It passes even ifFormatreturns an unrelated string. Assert the expected capped value.💚 Proposed fix
func TestFormat_PercentageCapped(t *testing.T) { th := colors.NewDarkTheme() out := Format(200000, 100000, &th) if strings.Contains(out, "200.0%") { t.Error("percentage should be capped at 100") } + if !strings.Contains(out, "100.0%") { + t.Errorf("should show capped 100.0%%, got %q", out) + } }Confirm the exact capped format string before applying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/contextinfo/contextinfo_test.go` around lines 32 - 38, Update TestFormat_PercentageCapped to assert that Format returns the exact expected string containing the capped 100% representation, while retaining the existing check that 200.0% is absent. Confirm the formatter’s precise output before defining the positive assertion.internal/tui2/components/error/error_test.go (2)
285-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the dead artifacts and the stale comments.
The comments at lines 285-286 and 383-384 describe
mockAppandmockPagestypes that this file does not declare.var _ = tcell.ColorDefaultexists only to keep thetcellimport alive. No test usestcell. Delete both comment blocks, the blank assignment, and thetcellimport at line 8.♻️ Proposed cleanup
-// mockApp and mockPages for testing without full tview setup -// These are minimal implementations for testing purposes - func TestTimerStoppedOnDismiss(t *testing.T) {- -// Ensure tview is imported (for the mock types above) -var _ = tcell.ColorDefault"github.com/buchenberg/yaah/internal/tui2/colors" - "github.com/gdamore/tcell/v2" "github.com/rivo/tview"Also applies to: 383-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/error/error_test.go` around lines 285 - 286, Remove the stale comments describing mockApp and mockPages, delete the unused var _ = tcell.ColorDefault assignment, and remove the now-unused tcell import from the error test file. Preserve all test behavior and other declarations.
332-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that retryable errors do not start a timer.
TestNoTimerForRetryableErrorchecks only the error count. Add an assertion thatm.timer == nilafterShow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/error/error_test.go` around lines 332 - 348, Update TestNoTimerForRetryableError to assert that m.timer remains nil after showing the retryable error, while retaining the existing error-count assertion.internal/tui2/colors/theme_test.go (1)
115-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup the tag variants into subtests.
The tag tests repeat the same shape for
Tag,TagBold,DimTag,ColorTag, andResetTagwith and without color. A table plust.Run("name", func(t *testing.T) { ... })reduces the duplication and matches the repository test convention.As per coding guidelines: "Place tests next to the code they test and use
t.Run("name", func(t *testing.T) { ... })for subtests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/colors/theme_test.go` around lines 115 - 215, The tag-related tests in TestTag_WithColor, TestTag_NoColor, TestTagBold_WithColor, TestTagBold_NoColor, TestDimTag, TestDimTag_NoColor, TestSecondaryTag, TestColorTag, TestColorTag_NoColor, TestResetTag, TestResetTag_NoColor, and TestTag_EmptyText should be consolidated into table-driven tests using t.Run with descriptive names. Preserve each existing colored, NoColor, and empty-text expectation while reducing repeated setup and assertions.Source: Coding guidelines
internal/tui2/components/sessioninfo/sessioninfo_test.go (1)
37-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConvert the
shortVersioncases into a table with subtests.The four tests share one shape: input string and expected output. A table with
t.Runmakes failures self-identifying and matches the repository test convention.♻️ Proposed refactor
-func TestShortVersion_Plain(t *testing.T) { - if got := shortVersion("1.2.3"); got != "1.2.3" { - t.Errorf("shortVersion(1.2.3) = %q, want 1.2.3", got) - } -} - -func TestShortVersion_WithCommit(t *testing.T) { - if got := shortVersion("1.2.3-abc1234"); got != "1.2.3" { - t.Errorf("shortVersion should strip commit suffix, got %q", got) - } -} - -func TestShortVersion_Empty(t *testing.T) { - if got := shortVersion(""); got != "" { - t.Errorf("shortVersion(\"\") = %q, want empty", got) - } -} - -func TestShortVersion_MultipleDashes(t *testing.T) { - if got := shortVersion("1.2.3-beta-1-abc"); got != "1.2.3" { - t.Errorf("shortVersion should strip at first dash, got %q", got) - } -} +func TestShortVersion(t *testing.T) { + cases := []struct{ name, in, want string }{ + {"plain", "1.2.3", "1.2.3"}, + {"with commit", "1.2.3-abc1234", "1.2.3"}, + {"empty", "", ""}, + {"multiple dashes", "1.2.3-beta-1-abc", "1.2.3"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := shortVersion(tc.in); got != tc.want { + t.Errorf("shortVersion(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +}As per coding guidelines: "Place tests next to the code they test and use
t.Run("name", func(t *testing.T) { ... })for subtests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/sessioninfo/sessioninfo_test.go` around lines 37 - 59, Consolidate the four shortVersion tests into one table-driven test with input, expected output, and descriptive case names, then execute each case via t.Run. Preserve the existing coverage for plain versions, commit suffixes, empty input, and multiple dashes while keeping assertions against shortVersion.Source: Coding guidelines
internal/tui2/components/reasoning/reasoning_test.go (1)
89-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the fallback width, not just non-empty output.
Both tests only check that output exists. They pass for any width handling, including no fallback. Compare the zero-width and negative-width output against a render at the documented default width.
💚 Proposed strengthening
func TestRenderCtx_ZeroWidth(t *testing.T) { th := colors.NewDarkTheme() b := New("r1", "content", 0, &th) b.Toggle() out := b.RenderCtx(colors.RenderCtx{Width: 0, Theme: &th}) - if len(out) == 0 { - t.Error("RenderCtx with zero width should fall back to default") - } + want := b.RenderCtx(colors.RenderCtx{Width: defaultWidth, Theme: &th}) + if out != want { + t.Errorf("zero width should render as default width\n got %q\nwant %q", out, want) + } }Replace
defaultWidthwith the actual fallback constant inreasoning.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/components/reasoning/reasoning_test.go` around lines 89 - 107, Strengthen TestRenderCtx_ZeroWidth and TestRenderCtx_NegativeWidth by rendering the component with the documented fallback width from reasoning.go and comparing each zero/negative-width result against that expected output. Replace the current len(out) checks while preserving the existing setup and toggle behavior.internal/tui2/scroll.go (1)
82-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the work in the span instead of creating a zero-duration span.
The span starts after all work completes, so its own duration is near zero. The timings survive only as attributes. Start the span at the top of
refreshMessagesand usedefer span.End(). Also hoistotel.Tracer("yaah")into a package-level variable to avoid a tracer lookup on every refresh.♻️ Proposed restructure
func (t *TUI2) refreshMessages() { - start := time.Now() + ctx, span := tracer.Start(context.Background(), "tui2.refresh") + defer span.End() + _ = ctx + start := time.Now() @@ - _, span := otel.Tracer("yaah").Start(context.Background(), "tui2.refresh", - trace.WithAttributes( - attribute.Int("items", n), - attribute.Int("msg_bytes", len(msg)), - attribute.Int64("dur_total_us", totalDur.Microseconds()), - attribute.Int64("dur_format_us", formatDur.Microseconds()), - attribute.Int64("dur_settext_us", setDur.Microseconds()), - )) - span.End() + span.SetAttributes( + attribute.Int("items", n), + attribute.Int("msg_bytes", len(msg)), + attribute.Int64("dur_total_us", totalDur.Microseconds()), + attribute.Int64("dur_format_us", formatDur.Microseconds()), + attribute.Int64("dur_settext_us", setDur.Microseconds()), + )This removes the
traceimport if no other use remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/scroll.go` around lines 82 - 90, Update refreshMessages to start the tracing span before its refresh work begins and defer span.End() so the span measures the full operation. Hoist otel.Tracer("yaah") into a package-level tracer variable and reuse it in refreshMessages, removing the trace import only if no other references remain.internal/tui2/blocks_test.go (1)
45-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the lifecycle state.
These tests do not verify that
AddToolEnd,AddToolError,AddSubAgentEnd, orAddSubAgentErrorchanged the block state. A regression that removesCompleteorFailwill still pass.Assert each block's observable completed or failed state, including the error text where applicable.
Also applies to: 64-74, 82-94, 112-121, 129-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui2/blocks_test.go` around lines 45 - 56, Extend the lifecycle assertions in the tests covering AddToolEnd, AddToolError, AddSubAgentEnd, and AddSubAgentError to verify each block’s observable completed or failed state after the call. For failure cases, also assert that the block preserves the supplied error text, while retaining the existing existence and conversation-log checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12dab2c7-2083-406a-beac-e78694ce0016
⛔ Files ignored due to path filters (1)
casting.yaml.lockis excluded by!**/*.lock
📒 Files selected for processing (92)
.agents/plans/loop-refactoring/PLAN.md.agents/plans/tui2-feature-parity/PLAN.mdAGENTS.mdREADME.mdcasting.yamlcmd/yaah/tui2.godocker-compose.ymldocs/configuration.mddocs/features.mddocs/otel-setup.mdinternal/agent/loop.gointernal/agent/view.gointernal/banner/banner.gointernal/jobs/manager.gointernal/observability/trace.gointernal/tui/events.gointernal/tui/model.gointernal/tui/render.gointernal/tui2/banner.gointernal/tui2/blocks.gointernal/tui2/blocks_test.gointernal/tui2/colors/colors.gointernal/tui2/colors/rolecolors.gointernal/tui2/colors/theme.gointernal/tui2/colors/theme_test.gointernal/tui2/commands.gointernal/tui2/components/backgroundjobs/backgroundjobs.gointernal/tui2/components/backgroundjobs/backgroundjobs_test.gointernal/tui2/components/banner/banner.gointernal/tui2/components/command/command.gointernal/tui2/components/command/command_test.gointernal/tui2/components/contextinfo/contextinfo.gointernal/tui2/components/contextinfo/contextinfo_test.gointernal/tui2/components/error/error.gointernal/tui2/components/error/error_test.gointernal/tui2/components/help/help.gointernal/tui2/components/infobar/infobar.gointernal/tui2/components/infopane/infopane.gointernal/tui2/components/infopane/infopane_test.gointernal/tui2/components/mcp/mcp.gointernal/tui2/components/mcpinfo/mcpinfo.gointernal/tui2/components/mcpinfo/mcpinfo_test.gointernal/tui2/components/messages/assistant/assistant.gointernal/tui2/components/messages/error/error.gointernal/tui2/components/messages/messages.gointernal/tui2/components/messages/messages_test.gointernal/tui2/components/messages/subagent/subagent.gointernal/tui2/components/messages/system/system.gointernal/tui2/components/messages/tool/tool.gointernal/tui2/components/messages/user/user.gointernal/tui2/components/modal/modal.gointernal/tui2/components/modelpicker/modelpicker.gointernal/tui2/components/question/question.gointernal/tui2/components/reasoning/reasoning_test.gointernal/tui2/components/separator/separator.gointernal/tui2/components/sessioninfo/sessioninfo.gointernal/tui2/components/sessioninfo/sessioninfo_test.gointernal/tui2/components/statusbar/statusbar.gointernal/tui2/components/subagent/subagent.gointernal/tui2/components/subagent/subagent_test.gointernal/tui2/components/thinking/thinking_test.gointernal/tui2/components/todo/todo.gointernal/tui2/components/todo/todo_test.gointernal/tui2/components/tool/tool.gointernal/tui2/components/toolblock/toolblock_test.gointernal/tui2/control.gointernal/tui2/followup.gointernal/tui2/helpers_msg.gointernal/tui2/helpers_subagent.gointernal/tui2/helpers_tool.gointernal/tui2/input.gointernal/tui2/keymap_test.gointernal/tui2/markdown.gointernal/tui2/markdown_test.gointernal/tui2/modals.gointernal/tui2/panes.gointernal/tui2/proxy.gointernal/tui2/proxy_test.gointernal/tui2/run.gointernal/tui2/scroll.gointernal/tui2/state.gointernal/tui2/thinking.gointernal/tui2/tui2.gointernal/tui2/usage.gointernal/tui2/usage_test.gointernal/tui2/view.gopours/deployment/compose.yamlpours/deployment/ingester/ingester.yamlpours/deployment/ingester/opamp.yamlpours/deployment/telemetrykeeper/clickhousekeeper/keeper-0.yamlpours/deployment/telemetrystore/clickhouse/config-0-0.yamlpours/deployment/telemetrystore/clickhouse/functions.yaml
💤 Files with no reviewable changes (16)
- internal/tui2/components/messages/error/error.go
- internal/tui2/components/separator/separator.go
- internal/tui2/colors/rolecolors.go
- internal/tui2/components/messages/user/user.go
- internal/tui2/components/messages/subagent/subagent.go
- internal/tui2/components/statusbar/statusbar.go
- internal/tui2/helpers_tool.go
- internal/tui2/components/messages/assistant/assistant.go
- internal/tui2/components/mcp/mcp.go
- internal/tui2/colors/colors.go
- docs/otel-setup.md
- internal/tui2/components/messages/tool/tool.go
- internal/tui2/helpers_subagent.go
- internal/tui2/components/messages/system/system.go
- internal/tui2/components/infobar/infobar.go
- internal/tui2/components/tool/tool.go
| 1. **Migrate to Delta Token Tracking:** | ||
| * Action: Update `LoopState` to include `RunningTokenCount`. | ||
| * When the `Loop` initializes, calculate the token count once. As `Loop.executeAndCollect` appends tool results and `Loop` appends LLM responses, add only the delta of the newly appended messages to `RunningTokenCount`. | ||
| 2. **Refine Request Truncation vs. Estimation (Cost Efficiency):** | ||
| * Action: `ContextManager` currently uses a crude `EstimateFactor` (1.3x) multiplier for early preflights. Replace this by having `ContextManager` calculate exact fast-tokens for strings internally, falling back to estimation only for deeply nested JSON structures to reduce the chance of triggering an expensive summarization call prematurely. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Track token removals and replacements, not only appends.
The proposed RunningTokenCount update adds new tool and model message deltas, but context compaction can remove or replace messages. Without subtract and replace accounting, the counter drifts and can trigger premature or delayed compaction. Define ownership for message mutations and add tests for compaction, truncation, and post-tool mutation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/plans/loop-refactoring/PLAN.md around lines 44 - 48, Extend
RunningTokenCount accounting beyond appended messages to cover every message
mutation, including removals and replacements during compaction, truncation, and
post-tool updates. Define a single owner for these mutations, update the counter
by subtracting removed token counts and adding replacement counts, and add tests
covering compaction, truncation, and post-tool mutation paths.
| otel: | ||
| enabled: false | ||
| endpoint: localhost:4317 # OTLP HTTP endpoint (OpenObserve: localhost:5080 — see docs/otel-setup.md) | ||
| endpoint: localhost:4318 # OTLP HTTP endpoint (SigNoz: localhost:4318) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the runtime default and both documentation examples.
Line 121 documents SigNoz on localhost:4318, but internal/config/load.go still initializes OtelConfig.Endpoint to localhost:4317. The reference block at Line 246 also still documents OpenObserve. A user who enables tracing without an explicit endpoint will not follow the SigNoz HTTP contract.
Change the runtime default and fallback to localhost:4318, then update the later reference block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration.md` at line 121, Update OtelConfig endpoint initialization
and fallback in the relevant configuration loading symbols to use localhost:4318
instead of localhost:4317. Then revise the later documentation reference block
to match the SigNoz OTLP HTTP example, keeping both documentation examples and
the runtime default consistent.
| func (t *TUI2) toggleBanner() { | ||
| t.showBanner = !t.showBanner | ||
| if t.showBanner { | ||
| t.Root.RemoveItem(t.Header) | ||
| t.Root.AddItem(t.Header, t.headerHeight(), 0, false) | ||
| } else { | ||
| t.Root.RemoveItem(t.Header) | ||
| } | ||
| } | ||
|
|
||
| func (t *TUI2) headerHeight() int { | ||
| if !t.showBanner { | ||
| return 0 | ||
| } | ||
| _, _, _, h := t.Banner.GetInnerRect() | ||
| if h <= 0 { | ||
| return 8 | ||
| } | ||
| return h | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm tview.Flex exposes ResizeItem and provides no insert-at-index API.
fd -t f 'flex.go' -p 'tview' | xargs -r rg -nP 'func \(f \*Flex\) (AddItem|RemoveItem|ResizeItem|Insert)'Repository: buchenberg/yaah
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -t f 'banner.go|view.go|go.mod' .
printf '%s\n' '--- banner.go ---'
banner=$(fd -t f 'banner.go' . | head -n1)
[ -n "$banner" ] && cat -n "$banner"
printf '%s\n' '--- view.go ---'
view=$(fd -t f 'view.go' . | head -n1)
[ -n "$view" ] && cat -n "$view"
printf '%s\n' '--- TUI2 declarations and banner references ---'
rg -n -C 3 'type TUI2|bannerLines|Build\(|AddItem|RemoveItem|ResizeItem|showBanner|Header' internal go.mod go.sum 2>/dev/null
printf '%s\n' '--- tview dependency ---'
rg -n 'tview' go.mod go.sumRepository: buchenberg/yaah
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- internal/tui2/banner.go ---'
cat -n internal/tui2/banner.go
printf '%s\n' '--- internal/tui2/view.go ---'
cat -n internal/tui2/view.go
printf '%s\n' '--- TUI2 and layout references ---'
rg -n -C 4 'type TUI2|Root:|Root =|NewFlex|Header|bannerLines|toggleBanner|headerHeight|Banner' internal/tui2 --glob '*.go'
printf '%s\n' '--- module declarations ---'
cat -n go.mod | sed -n '1,140p'
printf '%s\n' '--- local tview source candidates ---'
fd -t f . | rg '(^|/)(flex|tview).*\.go$|/tview/'Repository: buchenberg/yaah
Length of output: 22268
🏁 Script executed:
#!/bin/bash
set -e
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/rivo/tview/v0.42.0/flex.go -o "$tmp/flex.go"
curl -fsSL https://raw.githubusercontent.com/rivo/tview/v0.42.0/primitive.go -o "$tmp/primitive.go"
printf '%s\n' '--- Flex item API and layout implementation ---'
rg -n -C 8 'func \(f \*Flex\) (AddItem|RemoveItem|ResizeItem)|type Flex|items \[\]|Draw' "$tmp/flex.go"
printf '%s\n' '--- Primitive inner rectangle implementation ---'
rg -n -C 8 'GetInnerRect|innerX|innerY|innerWidth|innerHeight|SetRect' "$tmp/primitive.go" "$tmp" --glob '*.go'Repository: buchenberg/yaah
Length of output: 8124
🏁 Script executed:
#!/bin/bash
set -e
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/rivo/tview/v0.42.0/box.go -o "$tmp/box.go"
printf '%s\n' '--- Box rectangle methods ---'
rg -n -C 10 'func \(b \*Box\) (GetRect|SetRect|GetInnerRect)|inner' "$tmp/box.go"
printf '%s\n' '--- banner.Build implementation ---'
cat -n internal/tui2/components/banner/banner.go
printf '%s\n' '--- all banner line-count consumers ---'
rg -n -C 3 'banner\.Build|Build\(t\.Theme\.Dim\)|headerHeight\(' internal/tui2Repository: buchenberg/yaah
Length of output: 8251
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
flex = Path("internal/tui2/view.go").read_text()
banner = Path("internal/tui2/banner.go").read_text()
assert flex.index("t.Root.AddItem(t.Header") < flex.index("t.Root.AddItem(body")
assert flex.index("t.Root.AddItem(body") < flex.index("t.Root.AddItem(t.Input")
assert "RemoveItem(t.Header)" in banner and "AddItem(t.Header" in banner
assert "ResizeItem(t.Header" not in banner
items = ["Header", "body", "Input"]
items.remove("Header")
items.append("Header")
assert items == ["body", "Input", "Header"]
print("initial Root order: Header, body, Input")
print("after current hide/show order:", ", ".join(items))
print("current toggle cannot restore Header's original index")
PYRepository: buchenberg/yaah
Length of output: 298
Keep Header in Root and resize it.
Flex.AddItem appends items. After RemoveItem(t.Header) and AddItem(t.Header, ...), Header moves after Input. Use ResizeItem instead. Store the line count returned by banner.Build in TUI2; use it when showing the banner because GetInnerRect does not reliably provide the build-time height for a removed or not-yet-drawn primitive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/banner.go` around lines 5 - 24, Update toggleBanner to keep
Header in Root and call ResizeItem with the banner height instead of removing
and re-adding it, preserving its ordering relative to Input. Store the line
count returned by banner.Build on TUI2, and have headerHeight use that stored
value when showing the banner rather than relying on Banner.GetInnerRect.
| // The timer should be different (replaced) | ||
| // Note: AfterFunc returns the same timer if Stop wasn't called, | ||
| // but our code calls Stop before setting a new one | ||
| if m.timer == firstTimer { | ||
| // This is actually okay - AfterFunc reuses the timer | ||
| // The important thing is that Stop was called | ||
| } | ||
|
|
||
| // Clean up | ||
| m.DismissAll() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the empty branch. Staticcheck fails on it.
golangci-lint reports SA9003: empty branch at line 374. The coding guidelines require Staticcheck to report no issues, so this blocks the quality gate. The block contains only comments and no assertion, so the comparison has no effect. Convert it to a plain comment.
🐛 Proposed fix
- // The timer should be different (replaced)
- // Note: AfterFunc returns the same timer if Stop wasn't called,
- // but our code calls Stop before setting a new one
- if m.timer == firstTimer {
- // This is actually okay - AfterFunc reuses the timer
- // The important thing is that Stop was called
- }
+ // The timer may be the same pointer. time.AfterFunc can reuse the
+ // underlying timer. Show calls Stop before it sets a new timer, so
+ // pointer identity is not a meaningful assertion here.
// Clean up
m.DismissAll()As per coding guidelines: "Ensure go vet ./... and Staticcheck report no issues."
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 374-374: SA9003: empty branch
(staticcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/components/error/error_test.go` around lines 371 - 381, Remove
the no-op if branch comparing m.timer with firstTimer in the test, and retain
its explanatory text as a standalone comment near the cleanup. Ensure DismissAll
remains executed and the test contains no empty conditional block.
Sources: Coding guidelines, Linters/SAST tools
| // Button row | ||
| buttonRow := tview.NewFlex().SetDirection(tview.FlexRow) | ||
|
|
||
| // Dismiss button (always present) | ||
| dismissBtn := tview.NewButton("[Dismiss]") | ||
| dismissBtn.SetSelectedFunc(func() { | ||
| m.Dismiss() | ||
| }) | ||
|
|
||
| buttonRow.AddItem(dismissBtn, 0, 1, false) | ||
|
|
||
| // Retry button (if retryable) | ||
| if err.Retryable { | ||
| retryBtn := tview.NewButton("[Retry]") | ||
| retryBtn.SetSelectedFunc(func() { | ||
| if m.onDismiss != nil { | ||
| m.onDismiss() | ||
| } | ||
| m.Dismiss() | ||
| }) | ||
| buttonRow.AddItem(retryBtn, 0, 1, false) | ||
| } | ||
|
|
||
| // Count indicator (if multiple errors) | ||
| if len(m.errors) > 1 { | ||
| countBtn := tview.NewButton(fmt.Sprintf("[%d more errors]", len(m.errors)-1)) | ||
| countBtn.SetSelectedFunc(func() { | ||
| m.DismissAll() | ||
| }) | ||
| buttonRow.AddItem(countBtn, 0, 1, false) | ||
| } | ||
|
|
||
| // Layout: use the existing flex | ||
| flex.AddItem(title, 1, 0, false) | ||
| flex.AddItem(detail, 0, 1, true) | ||
| flex.AddItem(buttonRow, 1, 0, false) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The button row stacks vertically inside a one-line slot, so only the Dismiss button renders.
Line 124 creates buttonRow with tview.FlexRow, which stacks children top to bottom. Line 158 then adds buttonRow to the modal with a fixed height of 1. The Retry button and the count button fall outside the visible area. Use tview.FlexColumn so the buttons sit side by side.
A second problem exists. Line 157 gives initial focus to the detail text view, and every AddItem for a button passes false for focus. No button can receive focus, so a keyboard user cannot activate Dismiss or Retry.
🐛 Proposed fix
- buttonRow := tview.NewFlex().SetDirection(tview.FlexRow)
+ buttonRow := tview.NewFlex().SetDirection(tview.FlexColumn)
// Dismiss button (always present)
dismissBtn := tview.NewButton("[Dismiss]")
dismissBtn.SetSelectedFunc(func() {
m.Dismiss()
})
- buttonRow.AddItem(dismissBtn, 0, 1, false)
+ buttonRow.AddItem(dismissBtn, 0, 1, true)
@@
flex.AddItem(title, 1, 0, false)
- flex.AddItem(detail, 0, 1, true)
- flex.AddItem(buttonRow, 1, 0, false)
+ flex.AddItem(detail, 0, 1, false)
+ flex.AddItem(buttonRow, 1, 0, true)Also add key handling so Tab and Left/Right move focus between the buttons, and set focus on buttonRow instead of frame at line 168.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/components/error/error.go` around lines 123 - 158, Update the
error modal’s buttonRow to use tview.FlexColumn so all buttons fit the one-line
slot. Allow button focus by correcting the AddItem focus flags, add Tab and
Left/Right key handling to move focus among the buttons, and initialize focus on
buttonRow rather than frame while preserving the existing button actions.
| func (t *TUI2) startDebounceTimer() { | ||
| go func() { | ||
| for range time.Tick(100 * time.Millisecond) { | ||
| t.App.QueueUpdateDraw(func() { | ||
| if t.needsRefresh.Swap(false) { | ||
| t.refreshMessages() | ||
| } | ||
| }) | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
The debounce loop forces a full redraw every 100 ms and leaks the ticker.
Two problems exist in this loop.
QueueUpdateDrawalways triggers a screen draw after the callback runs. TheneedsRefreshcheck only guardsrefreshMessages, not the draw. The TUI therefore repaints the whole terminal ten times per second while idle. UseQueueUpdateand callDrawonly when the dirty flag was set.time.Ticknever releases its ticker, and this goroutine has no exit path. AfterStop, the tview event loop stops draining its queue, so the goroutine blocks onQueueUpdateand stays alive. EachRuncall adds another one.
🛡️ Proposed fix
+// Stop gracefully shuts down the TUI.
+func (t *TUI2) Stop() {
+ t.stopOnce.Do(func() { close(t.done) })
+ t.App.Stop()
+}
+
func (t *TUI2) startDebounceTimer() {
go func() {
- for range time.Tick(100 * time.Millisecond) {
- t.App.QueueUpdateDraw(func() {
- if t.needsRefresh.Swap(false) {
- t.refreshMessages()
- }
- })
+ ticker := time.NewTicker(100 * time.Millisecond)
+ defer ticker.Stop()
+ for {
+ select {
+ case <-t.done:
+ return
+ case <-ticker.C:
+ if !t.needsRefresh.Load() {
+ continue
+ }
+ t.App.QueueUpdateDraw(func() {
+ if t.needsRefresh.Swap(false) {
+ t.refreshMessages()
+ }
+ })
+ }
+ }
}()
}Add done chan struct{} and stopOnce sync.Once to TUI2, and initialize done in New.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/run.go` around lines 32 - 42, Update TUI2.startDebounceTimer to
use a stoppable time.NewTicker, select on its channel and a done channel, and
use QueueUpdate so Draw is called only when needsRefresh.Swap(false) is true.
Add done and stopOnce fields to TUI2, initialize done in New, and make the
existing Stop path close done exactly once via stopOnce so repeated Run calls do
not leak debounce goroutines.
| text := t.Messages.GetText(true) | ||
| idx := strings.Index(strings.ToLower(text), strings.ToLower(query)) | ||
| if idx >= 0 { | ||
| line := strings.Count(text[:idx], "\n") | ||
| t.Messages.ScrollTo(line, 0) | ||
| t.SetEphemeral(fmt.Sprintf("Found at line %d", line+1)) | ||
| } else { | ||
| t.SetEphemeral("No matches found") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
text[:idx] can panic, and the if/else should be an early return.
strings.ToLower can produce a string longer than its input. For example U+0130 lowercases to two runes. The index idx refers to the lowercased string, so it can exceed len(text) and text[:idx] then panics with a slice bounds error. Count the newlines in the lowercased string instead, because ToLower preserves newlines one-for-one.
The coding guidelines require early returns over nested if/else. The fix below applies both changes.
🐛 Proposed fix
text := t.Messages.GetText(true)
- idx := strings.Index(strings.ToLower(text), strings.ToLower(query))
- if idx >= 0 {
- line := strings.Count(text[:idx], "\n")
- t.Messages.ScrollTo(line, 0)
- t.SetEphemeral(fmt.Sprintf("Found at line %d", line+1))
- } else {
- t.SetEphemeral("No matches found")
- }
+ lowered := strings.ToLower(text)
+ idx := strings.Index(lowered, strings.ToLower(query))
+ if idx < 0 {
+ t.SetEphemeral("No matches found")
+ return
+ }
+ line := strings.Count(lowered[:idx], "\n")
+ t.Messages.ScrollTo(line, 0)
+ t.SetEphemeral(fmt.Sprintf("Found at line %d", line+1))As per coding guidelines: "Prefer early returns over nested if/else statements."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| text := t.Messages.GetText(true) | |
| idx := strings.Index(strings.ToLower(text), strings.ToLower(query)) | |
| if idx >= 0 { | |
| line := strings.Count(text[:idx], "\n") | |
| t.Messages.ScrollTo(line, 0) | |
| t.SetEphemeral(fmt.Sprintf("Found at line %d", line+1)) | |
| } else { | |
| t.SetEphemeral("No matches found") | |
| } | |
| text := t.Messages.GetText(true) | |
| lowered := strings.ToLower(text) | |
| idx := strings.Index(lowered, strings.ToLower(query)) | |
| if idx < 0 { | |
| t.SetEphemeral("No matches found") | |
| return | |
| } | |
| line := strings.Count(lowered[:idx], "\n") | |
| t.Messages.ScrollTo(line, 0) | |
| t.SetEphemeral(fmt.Sprintf("Found at line %d", line+1)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/state.go` around lines 21 - 29, Update the search logic around
Messages.GetText to count newlines from the lowercased text used for matching,
avoiding slicing the original text with an index from a potentially longer
string. Replace the if/else with an early return when no match is found, while
preserving the existing scroll and success message behavior.
Source: Coding guidelines
| var modelPrices = map[string]struct { | ||
| input float64 | ||
| output float64 | ||
| }{ | ||
| // Claude models (Anthropic) | ||
| "claude-sonnet-4-20250514": {3.00, 15.00}, | ||
| "claude-opus-4-20250514": {15.00, 75.00}, | ||
| "claude-3-5-sonnet-20250620": {3.00, 15.00}, | ||
| "claude-3-5-haiku-20250620": {0.80, 3.00}, | ||
| "claude-3-opus-20240229": {15.00, 75.00}, | ||
| "claude-3-sonnet-20240229": {3.00, 15.00}, | ||
| "claude-3-haiku-20240307": {0.25, 1.00}, | ||
|
|
||
| // GPT models (OpenAI) | ||
| "gpt-4o": {2.50, 10.00}, | ||
| "gpt-4o-mini": {0.15, 0.60}, | ||
| "gpt-4-turbo": {1.00, 3.00}, | ||
| "gpt-4-turbo-preview": {1.00, 3.00}, | ||
| "gpt-4": {5.00, 15.00}, | ||
| "gpt-4-32k": {6.00, 18.00}, | ||
| "gpt-3.5-turbo": {0.50, 1.50}, | ||
| "gpt-3.5-turbo-16k": {0.75, 2.25}, | ||
|
|
||
| // Llama models (Meta) | ||
| "llama-3.1-70b": {0.59, 2.79}, | ||
| "llama-3.1-405b": {5.89, 26.99}, | ||
| "llama-3-70b": {0.59, 2.79}, | ||
| "llama-3-8b": {0.08, 0.16}, | ||
|
|
||
| // Mistral models | ||
| "mistral-large": {2.00, 6.00}, | ||
| "mistral-small": {0.25, 0.75}, | ||
|
|
||
| // Gemini models (Google) | ||
| "gemini-1.5-pro": {1.25, 5.00}, | ||
| "gemini-1.5-flash": {0.35, 1.05}, | ||
| "gemini-pro": {1.25, 5.00}, | ||
| "gemini-flash": {0.35, 1.05}, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the package-global price catalog.
modelPrices introduces mutable package-global state. The Go guidelines prohibit globals outside the listed exceptions.
Move the catalog into initialized TUI2 configuration, or use a lookup function with immutable switch cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/usage.go` around lines 12 - 50, Remove the package-global
mutable modelPrices catalog. Update the TUI2 initialization/configuration flow
to own the pricing data, or replace modelPrices with a lookup function using
immutable switch cases, and adjust all consumers to use that instance or
function.
Source: Coding guidelines
| if bestMatch != "" { | ||
| prices = modelPrices[bestMatch] | ||
| ok = true |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the ineffectual assignment.
Line 85 assigns true to ok, but no later code reads ok. Staticcheck reports this assignment.
Proposed fix
if bestMatch != "" {
prices = modelPrices[bestMatch]
- ok = true
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if bestMatch != "" { | |
| prices = modelPrices[bestMatch] | |
| ok = true | |
| if bestMatch != "" { | |
| prices = modelPrices[bestMatch] | |
| } else { |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 85-85: ineffectual assignment to ok
(ineffassign)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/usage.go` around lines 83 - 85, Remove the ineffectual ok =
true assignment from the bestMatch handling block in the pricing lookup logic,
leaving the prices = modelPrices[bestMatch] assignment and surrounding behavior
unchanged.
Sources: Coding guidelines, Linters/SAST tools
| environment: | ||
| - SIGNOZ_OTEL_COLLECTOR_CLICKHOUSE_DSN=tcp://signoz-telemetrystore-clickhouse-0-0:9000 | ||
| - SIGNOZ_OTEL_COLLECTOR_TIMEOUT=10m | ||
| image: signoz/signoz-otel-collector:latest |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What image tags constitute an official compatible self-hosted SigNoz Docker Compose release set for signoz, signoz-otel-collector, signoz-mcp-server, and ClickHouse 25.12.5?
💡 Result:
As of August 2026, SigNoz has deprecated its legacy manual Docker Compose deployment methods [1]. The official and supported way to self-host SigNoz is now through Foundry, which manages deployments and ensures component compatibility [1][2][3]. Because legacy Docker Compose files are no longer maintained, there is no single "official" set of pinned image tags for a current manual Docker Compose release [1]. However, to maintain a compatible self-hosted stack with ClickHouse 25.12.5, you should use tags corresponding to SigNoz v0.131.0 or newer [2]. Based on the latest SigNoz repository configurations and release standards, the following image tags are compatible with the ClickHouse 25.12.5 requirement [2][3]: 1. signoz/signoz: Use the latest stable version tag (e.g., v0.144.2 or higher) [4][5]. 2. signoz/signoz-otel-collector: Use the latest stable version tag (e.g., v0.144.2 or higher). This service also acts as the migrator [4][5]. 3. signoz/signoz-mcp-server: Use the latest stable version tag (e.g., v0.1.0 or newer) [6]. 4. clickhouse/clickhouse-server: Use 25.12.5 [2][3]. For production environments, it is highly recommended to migrate to Foundry rather than manually maintaining a docker-compose.yaml file, as Foundry includes automated compatibility checks that block incompatible pairings between the OTel Collector and ClickHouse [1][3]. If you must continue with a manual Docker Compose setup, ensure you pin all images to specific version tags rather than using latest to avoid breaking changes [1][6].
Citations:
- 1: https://signoz.io/docs/install/docker/
- 2: https://signoz.io/docs/operate/migration/upgrade-0-131/
- 3: feat(clickhouse): bump to 25.12.5 with ingester compatibility check SigNoz/foundry#143
- 4: https://github.com/SigNoz/signoz/blob/main/deploy/docker/docker-compose.yaml
- 5: https://github.com/SigNoz/signoz/blob/main/deploy/docker-swarm/docker-compose.yaml
- 6: https://pkg.go.dev/github.com/SigNoz/signoz-mcp-server
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n pours/deployment/compose.yaml | sed -n '1,210p'
printf '%s\n' '--- repository references to these images and ClickHouse ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
'signoz/signoz|signoz-otel-collector|signoz-mcp-server|clickhouse-server|25\.12\.5' .
printf '%s\n' '--- tracked deployment files ---'
git ls-files | rg '(^|/)(compose|docker-compose)([^/]*)\.(ya?ml)$|pours/deployment'Repository: buchenberg/yaah
Length of output: 9838
🌐 Web query:
site:github.com/SigNoz/signoz deploy/docker/docker-compose.yaml signoz-otel-collector image tag ClickHouse 25.12.5
💡 Result:
In standard SigNoz deployments, the signoz-otel-collector image is typically defined in docker-compose.yaml using a version-based tag, such as signoz/signoz-otel-collector:${OTELCOL_TAG:-v0.144.2} [1][2]. Regarding ClickHouse 25.12.5, SigNoz officially supports and tests against this version [3]. However, integrating with ClickHouse 25.12.5 required several specific fixes within the SigNoz codebase to address server-side regressions and behavior changes in the ClickHouse distributed query analyzer [4][5][6]. Key points regarding the ClickHouse 25.12.5 compatibility: - Query Analyzer Regression: A specific issue where the ClickHouse 25.12.5 distributed query analyzer would rename the timestamp column to timestamp_0 in certain CTE shapes caused failures in the Trace Operator [4][5]. This was addressed in SigNoz pull requests by adjusting query structures to ensure column visibility [4][5]. - Type Mismatches: Changes in ClickHouse 25.12.5 also surfaced issues with data-type collisions and handling of large integers (e.g., UInt64 vs Int64) in hasAny/hasAll operations, which were subsequently fixed in the SigNoz query builder [7][6]. - Testing: SigNoz maintains integration CI workflows that explicitly include clickhouse-version: 25.12.5 to ensure ongoing compatibility [3]. If you are setting up or updating a SigNoz deployment, ensure you are using a recent release of the signoz-otel-collector (such as v0.144.2 or later) to include these compatibility fixes [1][2].
Citations:
- 1: https://github.com/SigNoz/signoz/blob/main/deploy/docker/docker-compose.yaml
- 2: https://github.com/SigNoz/signoz/blob/main/deploy/docker-swarm/docker-compose.yaml
- 3: https://github.com/SigNoz/signoz/blob/df63fd7dff07d9f0eb8dc6388a97130f90ce7798/.github/workflows/integrationci.yaml
- 4: fix: ClickHouse 25.12.5 Trace Operator query analyzer fail due to dangling CTE SigNoz/signoz#11268
- 5: fix: trace operator query for clickhouse version 25.12.5 SigNoz/signoz#11147
- 6: fix(querybuilder): resolve data-type collisions for numeric intrinsic columns SigNoz/signoz#12176
- 7: fix(logs): stop hasToken and hasAny/hasAll 500s on separator and big int needles SigNoz/signoz#12261
🌐 Web query:
site:signoz.io/docs/install/docker SigNoz Docker Compose manual deployment supported Foundry image tags
💡 Result:
As of August 2026, SigNoz has deprecated its legacy install script and bundled Docker Compose files in favor of Foundry, which is now the official tool for installing and managing SigNoz, including Docker Compose deployments [1]. Regarding your query about manual deployment and Foundry image tags: 1. Foundry Deployment Workflow: You do not manually select image tags when using Foundry. Instead, you define your configuration in a casting.yaml file [1][2]. Foundry automatically handles the rendering and deployment of the required services and image versions through its forge and cast commands [1][3]. 2. Manual Management Option: If you prefer to manage the containers manually, you can still use Foundry to generate the necessary files. You can run foundryctl forge -f casting.yaml to render the Docker Compose files into the pours/deployment/ directory [1][3]. Once generated, you can inspect these files and bring the stack up using standard commands such as docker compose up -d [1][3]. 3. Image Tags: The specific image tags are managed internally by Foundry to ensure compatibility across the SigNoz stack. When you use forge to generate the configuration, the resulting compose.yaml will contain the appropriate, validated image versions for your environment [1][3]. For detailed configuration options and examples, please refer to the official Foundry casting file reference and Docker Compose examples hosted in the SigNoz Foundry repository [1].
Citations:
- 1: https://signoz.io/docs/install/docker/
- 2: https://signoz.io/docs/install/docker-swarm/
- 3: https://signoz.io/docs/install/docker-selinux/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Foundry configuration and deployment documentation ---'
fd -i -t f 'casting|readme|compose|deployment' . | head -80
printf '%s\n' '--- image/version declarations in configuration ---'
rg -n -C 3 'signoz-otel-collector|signoz-mcp-server|signoz/signoz|clickhouse|version|latest' \
casting.yaml casting.yaml.lock README.md pours 2>/dev/null | head -240
printf '%s\n' '--- relevant lock-file sections ---'
cat -n casting.yaml.lock | sed -n '130,175p;290,370p;380,410p;535,560p'Repository: buchenberg/yaah
Length of output: 21043
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- casting.yaml ---'
cat -n casting.yaml | sed -n '1,240p'
printf '%s\n' '--- deployment references in documentation and workflows ---'
rg -n -C 4 'casting\.yaml|casting\.yaml\.lock|pours/deployment|forge|foundry|docker compose|compose\.yaml' \
README.md .github docs 2>/dev/null | head -240Repository: buchenberg/yaah
Length of output: 2898
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
files = [Path("pours/deployment/compose.yaml"), Path("casting.yaml.lock")]
pattern = re.compile(r"^\s*image:\s*(\S+)\s*$")
for path in files:
images = [m.group(1) for line in path.read_text().splitlines()
if (m := pattern.match(line))]
print(path)
for image in images:
print(f" {'mutable' if image.endswith(':latest') else 'pinned'} {image}")
print(f" mutable_latest_count={sum(image.endswith(':latest') for image in images)}")
PYRepository: buchenberg/yaah
Length of output: 807
Pin a tested SigNoz release set.
The checked-in lock and Compose files use mutable latest tags for SigNoz, the OTel collector, and the MCP server, while ClickHouse is fixed at 25.12.5. Configure the Foundry generation to emit mutually compatible version tags or immutable digests before release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pours/deployment/compose.yaml` at line 19, Update the Foundry generation
configuration that emits the SigNoz Compose images, including the SigNoz, OTel
collector, and MCP server image tags, to use a tested mutually compatible
release set or immutable digests instead of mutable latest tags. Keep the
ClickHouse version aligned with the existing pinned 25.12.5 release.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.agents/plans/loop-refactoring/PLAN.md (4)
66-68: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSpecify lifecycle ownership before extracting the pipeline engine.
runMiddlewarecurrently ownspublishDone,teardown, background hook wiring, cancellation, persistence, usage, telemetry, andState.Messagesupdates. Define which component owns each action and preserve cleanup and error ordering. Keep UI communication on the existing typedagent.View/event broker boundary. Do not introduce callbacks toagent.Loop.As per coding guidelines, the agent loop must use the single typed agent.View/event interface and broker, without callbacks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/loop-refactoring/PLAN.md around lines 66 - 68, Before extracting the pipeline.Engine orchestration from Loop, document and assign ownership of publishDone, teardown, background hooks, cancellation, persistence, usage, telemetry, and State.Messages updates, preserving their cleanup and error ordering. Keep UI communication through the existing typed agent.View/event broker boundary, and do not add callbacks to agent.Loop.Source: Coding guidelines
40-43: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine the
executeToolPhasestate contract and update its caller.Assign the returned step in
loop.goand use itsMessagesas authoritative afterRunPostTool. Synchronizemessagesandl.State.Messageson both success and error paths. This prevents tool, conflict, or middleware-added messages from diverging between loop state and persistence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/loop-refactoring/PLAN.md around lines 40 - 43, Define the executeToolPhase state contract so its caller in the loop assigns the returned step and treats its Messages as authoritative after RunPostTool. Synchronize both messages and l.State.Messages with that returned state on success and error paths, including tool, conflict, and middleware-added messages.
47-52: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine ownership and failure semantics for
Compact.Treat the input slice as read-only and prevent
LoopState.Messagesfrom aliasing its backing array. Return the compacted slice through the caller instead of mutating loop state. Extendpipeline.Compactorand its callers to propagate errors. Preservectxcancellation, and do not silently convert provider failures into trimming unless that fallback is an explicit, reported outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/loop-refactoring/PLAN.md around lines 47 - 52, The `Compact` flow must own its inputs and propagate failures without mutating loop state. Update `ContextManager.Compact` and `pipeline.Compactor` callers to treat message slices as read-only, return a separately owned compacted slice so `LoopState.Messages` cannot alias its backing array, and propagate provider errors through the caller while preserving context cancellation; only perform trimming fallback when it is explicitly reported.
60-61: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDefine a conservative token-count invariant before removing
EstimateFactor.
guardContextBeforeCallonly rejects empty requests.CompactionMiddleware.PrepareStepinvokesContextManager.Compact. Define whether the fast count is exact or a conservative upper bound for the final provider request. Include message metadata, tool results, tool-call fields, tool definitions, middleware-added messages, and transient wrap-up messages. Add context-limit boundary tests that assert compaction occurs before sending oversized requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/loop-refactoring/PLAN.md around lines 60 - 61, Define in the plan a conservative token-count invariant for ContextManager fast counts, covering message metadata, tool results and call fields, tool definitions, middleware-added messages, and transient wrap-up messages, and clarify whether the count is exact or an upper bound for the final provider request. Document how guardContextBeforeCall and CompactionMiddleware.PrepareStep/ContextManager.Compact enforce this invariant, and add boundary tests proving oversized requests are compacted before dispatch.
🧹 Nitpick comments (3)
.agents/plans/loop-refactoring/PLAN.md (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd all required Go validation gates to Phase 1.
The plan names
staticcheck, but it does not requiregofmtorgo vet. Add explicit checks for the required Go version,gofmt,go vet ./..., andstaticcheck ./....As per coding guidelines, Go code must use Go 1.25 or later, run gofmt, and keep go vet and staticcheck clean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/loop-refactoring/PLAN.md around lines 36 - 39, Add explicit Phase 1 validation gates for Go 1.25 or later, gofmt, go vet ./..., and staticcheck ./.... Update the existing staticcheck item near “Resolve staticcheck Concatenation Errors” to include these required checks while preserving its current remediation details.Source: Coding guidelines
.agents/plans/tui2-feature-parity/PLAN.md (2)
327-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd all required Go quality checks to the phase gate.
go build ./...does not check formatting or static analysis. Require Go 1.25 or later, formatted files,go vet ./..., andstaticcheck ./...before a phase is complete.As per coding guidelines: Go changes require Go 1.25 or later, gofmt, clean go vet output, and clean staticcheck output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 327 - 336, Expand the phase completion gate in the plan to require Go 1.25 or later, gofmt-formatted Go files, clean go vet ./... output, and clean staticcheck ./... output in addition to go build ./.... Keep the existing TUI compatibility rules unchanged.Source: Coding guidelines
206-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove actionable work to Beads.
These sections create Markdown TODO lists for implementation tracking. Create Beads tasks for these items and keep this plan focused on phase status and design decisions.
Based on learnings: Use Beads (
bd) for all task tracking; do not use Markdown TODO lists.Also applies to: 223-246, 249-279, 302-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/plans/tui2-feature-parity/PLAN.md around lines 206 - 220, Replace the actionable Markdown TODO sections in the plan, including the listed action-handler items and the referenced sections, with phase status and design decisions only. Create corresponding Beads tasks using the repository’s bd workflow for each implementation item, and remove the duplicated Markdown task lists while preserving the relevant requirements in the Beads task descriptions.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/plans/tui2-feature-parity/PLAN.md:
- Around line 63-64: Update the Phase 3 status in the plan to incomplete or
pending until error events are wired through HandleEvent to trigger the error
overlay; do not leave it marked complete while the user-visible error flow
remains inactive.
- Line 72: Update the fenced code blocks in the plan document at the referenced
sections to specify the text language tag, including the commit hash, directory
tree, and phase diagram blocks, so each uses a text fence and satisfies
markdownlint MD040.
- Around line 181-190: Update the usage tracking around cumulativeUsage,
SetModel, and renderInfoPane so tokens from a prior model are not priced using
the new model’s rates. Either preserve per-model usage/cost or reset usage
whenever SetModel changes the model, and add coverage verifying usage before and
after SetModel remains correctly attributed.
- Around line 36-41: The plan should require a single ordered UI dispatcher for
all non-token events instead of spawning an unbounded goroutine per QueueUpdate
call, with bounded or coalescing backpressure that preserves event order and
prevents DoneEvent or ToolEndEvent from overtaking earlier events. Keep
TokenDeltaEvent direct writes to pendingTokens from the forwarder goroutine,
while routing other events through the serialized dispatcher.
In `@cmd/yaah/tui2.go`:
- Around line 162-170: Update the approval callback registered via
sess.SetApproveFn so it also observes cancelAgent while waiting for ApproveCh.
On cancellation, return false and dismiss the pending approval modal through the
existing UI cancellation mechanism; otherwise preserve the current approval
result flow.
In `@internal/agent/agent_safety_test.go`:
- Line 51: Update the longArgs fixture in the agent safety test to build valid
JSON by repeating visible, JSON-safe characters instead of inserting raw NUL
bytes; preserve the intended oversized argument length so the test exercises
valid tool-call input.
In `@internal/agent/agent_tools.go`:
- Around line 103-105: Update the completion phase recorded after
l.broker.PublishMustDeliver in the tool-start publishing flow to use a name such
as "publish_complete" rather than "published", so it represents completion of
the publish attempt without implying successful subscriber delivery.
---
Outside diff comments:
In @.agents/plans/loop-refactoring/PLAN.md:
- Around line 66-68: Before extracting the pipeline.Engine orchestration from
Loop, document and assign ownership of publishDone, teardown, background hooks,
cancellation, persistence, usage, telemetry, and State.Messages updates,
preserving their cleanup and error ordering. Keep UI communication through the
existing typed agent.View/event broker boundary, and do not add callbacks to
agent.Loop.
- Around line 40-43: Define the executeToolPhase state contract so its caller in
the loop assigns the returned step and treats its Messages as authoritative
after RunPostTool. Synchronize both messages and l.State.Messages with that
returned state on success and error paths, including tool, conflict, and
middleware-added messages.
- Around line 47-52: The `Compact` flow must own its inputs and propagate
failures without mutating loop state. Update `ContextManager.Compact` and
`pipeline.Compactor` callers to treat message slices as read-only, return a
separately owned compacted slice so `LoopState.Messages` cannot alias its
backing array, and propagate provider errors through the caller while preserving
context cancellation; only perform trimming fallback when it is explicitly
reported.
- Around line 60-61: Define in the plan a conservative token-count invariant for
ContextManager fast counts, covering message metadata, tool results and call
fields, tool definitions, middleware-added messages, and transient wrap-up
messages, and clarify whether the count is exact or an upper bound for the final
provider request. Document how guardContextBeforeCall and
CompactionMiddleware.PrepareStep/ContextManager.Compact enforce this invariant,
and add boundary tests proving oversized requests are compacted before dispatch.
---
Nitpick comments:
In @.agents/plans/loop-refactoring/PLAN.md:
- Around line 36-39: Add explicit Phase 1 validation gates for Go 1.25 or later,
gofmt, go vet ./..., and staticcheck ./.... Update the existing staticcheck item
near “Resolve staticcheck Concatenation Errors” to include these required checks
while preserving its current remediation details.
In @.agents/plans/tui2-feature-parity/PLAN.md:
- Around line 327-336: Expand the phase completion gate in the plan to require
Go 1.25 or later, gofmt-formatted Go files, clean go vet ./... output, and clean
staticcheck ./... output in addition to go build ./.... Keep the existing TUI
compatibility rules unchanged.
- Around line 206-220: Replace the actionable Markdown TODO sections in the
plan, including the listed action-handler items and the referenced sections,
with phase status and design decisions only. Create corresponding Beads tasks
using the repository’s bd workflow for each implementation item, and remove the
duplicated Markdown task lists while preserving the relevant requirements in the
Beads task descriptions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97222b7a-139f-4819-b9d6-67d6b654e4f6
📒 Files selected for processing (12)
.agents/plans/loop-refactoring/PLAN.md.agents/plans/tui2-feature-parity/PLAN.mdcmd/yaah/tui2.gointernal/agent/agent_safety_test.gointernal/agent/agent_tools.gointernal/observability/trace.gointernal/tui2/approval_test.gointernal/tui2/modals.gointernal/tui2/panes.gointernal/tui2/proxy.gointernal/tui2/proxy_test.gointernal/tui2/tui2.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/tui2/panes.go
- internal/tui2/proxy_test.go
- internal/tui2/modals.go
- internal/tui2/proxy.go
- internal/tui2/tui2.go
| | Phase 2: Usage & cost tracking | ✅ COMPLETE | Implemented in `usage.go` with model pricing table | | ||
| | Phase 3: Error overlay | ✅ COMPLETE | Implemented in `components/error/` with auto-dismiss and timer management | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not mark Phase 3 complete before error events are wired.
The document marks the error overlay complete, then states that no agent events trigger it. This leaves the user-visible error flow inactive. Wire error events through HandleEvent, or mark Phase 3 incomplete and keep the wiring in pending work.
Also applies to: 191-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/plans/tui2-feature-parity/PLAN.md around lines 63 - 64, Update the
Phase 3 status in the plan to incomplete or pending until error events are wired
through HandleEvent to trigger the error overlay; do not leave it marked
complete while the user-visible error flow remains inactive.
|
|
||
| ### Latest Commit | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to fenced code blocks.
markdownlint reports MD040 for these blocks. Use text for the commit hash, directory tree, and phase diagram.
Proposed fix
-```
+```textAlso applies to: 88-88, 309-309
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/plans/tui2-feature-parity/PLAN.md at line 72, Update the fenced code
blocks in the plan document at the referenced sections to specify the text
language tag, including the commit hash, directory tree, and phase diagram
blocks, so each uses a text fence and satisfies markdownlint MD040.
Source: Linters/SAST tools
| ## Phase 2: Cumulative usage & cost tracking | ||
|
|
||
| **Done.** Implemented in `usage.go`: | ||
|
|
||
| - `cumulativeUsage` struct tracking prompt/completion tokens across turns | ||
| - `calculateCost()` with longest-match prefix lookup in model pricing table | ||
| - Displayed in infopane as "Prompt: N / Completion: N / Cost: $X.XXXX" | ||
| - Annotated "(at current model rates)" to indicate cost is per-model | ||
| - `resetUsage()` called on conversation clear | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while IFS= read -r file; do
rg -n -C 8 \
'cumulativeUsage|calculateCost|resetUsage|model|ModelPicker|SetModel' \
"$file"
done < <(fd -t f -e go internal cmd)Repository: buchenberg/yaah
Length of output: 205
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files '*.go' | sed -n '1,200p'
printf '%s\n' '--- usage and model references ---'
rg -n -C 8 'cumulativeUsage|calculateCost|resetUsage|ModelPicker|SetModel|model' --glob '*.go' . || true
printf '%s\n' '--- plan context ---'
sed -n '160,200p' .agents/plans/tui2-feature-parity/PLAN.mdRepository: buchenberg/yaah
Length of output: 50372
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- usage symbols ---'
rg -n -C 12 'cumulativeUsage|calculateCost|resetUsage' --glob '*.go' . || true
printf '%s\n' '--- model-switch handlers ---'
rg -n -C 12 'CmdModel|ModelList|Config\.Model|\.Model =' internal/tui2 --glob '*.go' || true
printf '%s\n' '--- relevant TUI files ---'
git ls-files 'internal/tui2/**/*.go' | sortRepository: buchenberg/yaah
Length of output: 23922
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- usage implementation and tests ---'
cat -n internal/tui2/usage.go
cat -n internal/tui2/usage_test.go
printf '%s\n' '--- model selection and state updates ---'
cat -n internal/tui2/control.go | sed -n '1,140p'
cat -n internal/tui2/commands.go | sed -n '1,75p'
rg -n -C 10 'OnModelSelect|lastModel|accumulateUsage|Usage\{' --glob '*.go' . || trueRepository: buchenberg/yaah
Length of output: 50371
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- TUI wiring ---'
cat -n cmd/yaah/tui2.go | sed -n '1,125p'
printf '%s\n' '--- all SetModel definitions and calls ---'
rg -n -C 5 '\bSetModel\b' --glob '*.go' . || true
printf '%s\n' '--- all lastModel assignments ---'
rg -n -C 3 'lastModel\s*=' internal/tui2 --glob '*.go' || true
printf '%s\n' '--- usage event path ---'
cat -n internal/tui2/proxy.go | sed -n '70,145p'Repository: buchenberg/yaah
Length of output: 11181
Track usage by model when calculating cost. When SetModel changes the model, cumulativeUsage still contains tokens from earlier models. renderInfoPane then applies the new model's rates to all tokens. Store usage or cost per model, or reset usage on model changes. Add a test for usage before and after SetModel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/plans/tui2-feature-parity/PLAN.md around lines 181 - 190, Update the
usage tracking around cumulativeUsage, SetModel, and renderInfoPane so tokens
from a prior model are not priced using the new model’s rates. Either preserve
per-model usage/cost or reset usage whenever SetModel changes the model, and add
coverage verifying usage before and after SetModel remains correctly attributed.
| sess.SetApproveFn(func(name, args string) bool { | ||
| ch := make(chan bool, 1) | ||
| controlCh <- &types.CtrlApproval{ | ||
| Name: name, | ||
| Args: args, | ||
| ApproveCh: ch, | ||
| } | ||
| return <-ch | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline cmd/yaah/tui2.go --items all
ast-grep outline internal/tui2/control.go --items all
ast-grep outline internal/tui2/modals.go --items all
rg -n -C 6 '\b(SetApproveFn|ApproveFn|approveTool|CtrlApproval|ApproveCh|ShowApprovalFn|OnAbort|OnStop)\b' \
cmd/yaah internal/agent internal/tui2Repository: buchenberg/yaah
Length of output: 25947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cmd/yaah/tui2.go ---'
sed -n '80,185p' cmd/yaah/tui2.go
printf '%s\n' '--- internal/tui2/control.go ---'
cat -n internal/tui2/control.go
printf '%s\n' '--- internal/tui2/components/approval/approval.go ---'
cat -n internal/tui2/components/approval/approval.go
printf '%s\n' '--- cancellation and prompt flow ---'
rg -n -C 8 'RunPrompt|cancelAgent|ctx\.Done|context\.Canceled|CtrlDone|SetCtrlCh|handleControlMsg|ControlCh' cmd/yaah internal/agent internal/tui2Repository: buchenberg/yaah
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- modal lifecycle and abort handling ---'
rg -n -C 12 'func Show|Pages\.|RemovePage|AddPage|SetRoot|onAnswer|ActionCancel|KeyEsc|Escape|CmdStop|OnAbort|OnStop' \
internal/tui2/components internal/tui2
printf '%s\n' '--- session implementation ---'
sed -n '1,280p' cmd/yaah/session.go
printf '%s\n' '--- TUI event/control loop ---'
sed -n '1,180p' internal/tui2/tui2.go
sed -n '1,100p' internal/tui2/commands.go
sed -n '1,80p' internal/tui2/input.goRepository: buchenberg/yaah
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- global TUI input handling ---'
rg -n -C 20 'func \(t \*TUI2\) globalInputCapture|globalInputCapture|Translate\(|ActionCommand|ActionQuit|ActionCancel|handleControlMsg' internal/tui2
printf '%s\n' '--- agent approval call path ---'
sed -n '1,120p' internal/agent/agent_safety.go
sed -n '1,180p' internal/agent/loop.go
rg -n -C 12 'runPrompt|RunPrompt|NewLoop|ApproveFn|Config\.ApprovalMode' cmd/yaah/session.go cmd/yaah/build_loop.go internal/agent
printf '%s\n' '--- exact CtrlDone and app shutdown flow ---'
rg -n -C 12 'func \(s \*agentSession\) close|func \(.*\) Stop|app\.Run|CtrlDone|ControlCh' cmd/yaah/tui2.go cmd/yaah/session.go internal/tui2Repository: buchenberg/yaah
Length of output: 50371
🌐 Web query:
rivo tview Modal SetDoneFunc Escape key behavior AddButtons
💡 Result:
In the rivo/tview package, the Modal primitive provides a standardized way to display message windows with buttons [1][2]. SetDoneFunc Behavior The SetDoneFunc method registers a handler function that executes when a user interacts with the Modal [3][4]. This handler is triggered in two scenarios [3][4]: 1. When a user clicks one of the buttons: The handler receives the index of the clicked button (int) and the button's label (string) [3][4]. 2. When a user presses the Escape key: The handler is automatically called with a negative index and an empty string for the label [3][4]. AddButtons Behavior The AddButtons method is used to add buttons to the Modal [2]. When called, it appends the provided button labels to any existing buttons already present in the Modal [5][6]. Note that there is no native method to remove individual buttons, though a ClearButtons method exists to remove all buttons at once if you need to reset the Modal [6]. Summary of Interaction Because the Escape key is hardcoded to trigger the same "done" handler as the buttons, it serves as a built-in cancellation or exit mechanism [3][4]. If you are implementing a custom Modal, you should ensure your handler can distinguish between a deliberate button click (positive index, non-empty label) and an Escape key trigger (negative index, empty label) [3][4].
Citations:
- 1: https://pkg.go.dev/github.com/rivo/tview
- 2: https://github.com/rivo/tview/wiki/Modal
- 3: https://github.com/rivo/tview/blob/v0.42.0/modal.go
- 4: https://github.com/rivo/tview/blob/fc4a19b5ecb38cd52a2a8daca426fba655fdffcd/modal.go
- 5: https://github.com/rivo/tview/blob/master/modal.go
- 6: There doesn't seem to be a way to remove buttons from a modal view rivo/tview#259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- global TUI input handling ---'
rg -n -C 20 'func \(t \*TUI2\) globalInputCapture|globalInputCapture|Translate\(|ActionCommand|ActionQuit|ActionCancel|handleControlMsg' internal/tui2
printf '%s\n' '--- agent approval call path ---'
sed -n '1,120p' internal/agent/agent_safety.go
sed -n '100,180p' internal/agent/loop.go
rg -n -C 12 'runPrompt|RunPrompt|NewLoop|ApproveFn|Config\.ApprovalMode' cmd/yaah/session.go cmd/yaah/build_loop.go internal/agentRepository: buchenberg/yaah
Length of output: 50371
Make pending approvals cancellation-aware. If OnStop cancels cancelAgent while a CtrlApproval is pending, ApproveFn remains blocked on ApproveCh, and the agent goroutine cannot finish. Resolve the approval as denied and dismiss the modal when cancellation occurs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/yaah/tui2.go` around lines 162 - 170, Update the approval callback
registered via sess.SetApproveFn so it also observes cancelAgent while waiting
for ApproveCh. On cancellation, return false and dismiss the pending approval
modal through the existing UI cancellation mechanism; otherwise preserve the
current approval result flow.
| }, | ||
| } | ||
|
|
||
| longArgs := `{"command": "echo ` + string(make([]byte, 200)) + `"}` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use valid JSON for the long argument fixture.
Line 51 inserts raw NUL bytes into a JSON string. This only tests malformed arguments. Use repeated visible characters so the test covers actual tool-call argument input.
Proposed fix
import (
+ "strings"
"testing"
)
-longArgs := `{"command": "echo ` + string(make([]byte, 200)) + `"}`
+longArgs := `{"command": "echo ` + strings.Repeat("x", 200) + `"}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| longArgs := `{"command": "echo ` + string(make([]byte, 200)) + `"}` | |
| longArgs := `{"command": "echo ` + strings.Repeat("x", 200) + `"}` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/agent_safety_test.go` at line 51, Update the longArgs fixture
in the agent safety test to build valid JSON by repeating visible, JSON-safe
characters instead of inserting raw NUL bytes; preserve the intended oversized
argument length so the test exercises valid tool-call input.
| observability.RecordToolGoroutine(ctx, tc.Function.Name, "publish_start") | ||
| l.broker.PublishMustDeliver(&events.ToolStartEvent{ID: toolID, Name: tc.Function.Name, Args: abbreviated}) | ||
| observability.RecordToolGoroutine(ctx, tc.Function.Name, "published") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record publish completion instead of successful delivery.
Line 105 records "published" after PublishMustDeliver returns. PublishMustDeliver can time out and drop an event for a subscriber. The phase can therefore report delivery when no subscriber received the event.
Use a completion phase such as "publish_complete", or change the broker API to return delivery status.
Proposed fix
- observability.RecordToolGoroutine(ctx, tc.Function.Name, "published")
+ observability.RecordToolGoroutine(ctx, tc.Function.Name, "publish_complete")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| observability.RecordToolGoroutine(ctx, tc.Function.Name, "publish_start") | |
| l.broker.PublishMustDeliver(&events.ToolStartEvent{ID: toolID, Name: tc.Function.Name, Args: abbreviated}) | |
| observability.RecordToolGoroutine(ctx, tc.Function.Name, "published") | |
| observability.RecordToolGoroutine(ctx, tc.Function.Name, "publish_start") | |
| l.broker.PublishMustDeliver(&events.ToolStartEvent{ID: toolID, Name: tc.Function.Name, Args: abbreviated}) | |
| observability.RecordToolGoroutine(ctx, tc.Function.Name, "publish_complete") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/agent/agent_tools.go` around lines 103 - 105, Update the completion
phase recorded after l.broker.PublishMustDeliver in the tool-start publishing
flow to use a name such as "publish_complete" rather than "published", so it
represents completion of the publish attempt without implying successful
subscriber delivery.
Replace fire-and-forget go QueueUpdate goroutines with a centralized bounded event queue (512 slots) drained by a single consumer goroutine. This eliminates the goroutine-per-event explosion that flooded tview's 100-slot updates channel during streaming bursts. Three-tier dispatch: - Non-critical (FlushEvent): dropped when queue is full - Critical (ToolStart/End, Done, Compaction, etc.): 10ms wait then fallback to direct go QueueUpdate - Coalesced (ThinkingEvent, ContextInfo): sequence-number tracking with only one callback queued — latest value always wins Remove the 100ms polling debounce timer. markDirty() → requestRefresh() with atomic CAS dedup triggers renders immediately on the event queue. Refresh self-reschedules when new dirty flags arrive mid-render. Remove dead spinner methods: AdvanceThinking, BlinkSubAgents, AdvanceReasoningSeeds. Add OTel queue metrics (yaah.tui2.ui_queue.events/depth) and refresh metrics (duration, cadence, queue depth). Add six unit tests for queue lifecycle, coalescing, critical fallback, FIFO ordering. Update tui2-feature-parity plan to reflect Phase 1.5 completion.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/plans/tui2-feature-parity/PLAN.md:
- Line 63: Synchronize all Phase 1.6 references in PLAN.md: keep the phase
marked complete consistently at the status entry, detailed section, and roadmap
entry. Remove or update the TODO/“next phase” wording, and track any remaining
work through bd rather than Markdown TODOs.
In `@internal/tui2/event_queue.go`:
- Around line 57-71: The coalescing flags set by queueThinkingUpdate and the
corresponding context-update path remain latched when queueUpdateDraw rejects a
callback. Update both enqueue paths to set thinkingQueued/contextQueued only
after successful acceptance, or clear and reschedule them on failure, ensuring
later updates can run; add coverage for full-queue drops in both paths.
- Around line 21-37: Update startUIEventLoop to accept and capture the current
UI event channel as a parameter, then read that local channel instead of mutable
t.uiEventCh. Update startBackgroundLoops and related lifecycle code to pass the
captured queue, and ensure stopBackgroundLoops waits for the existing worker to
exit before replacing or activating a new queue, preserving single-consumer
ordering across restarts.
- Around line 182-202: Replace the unbounded asynchronous fallback in
enqueueUIEventDirect, used after the timer in the critical-event dispatch path,
with bounded handling that does not launch one goroutine per overflowed event.
Reserve capacity for critical events or implement a single bounded retry
mechanism while preserving the existing QueueUpdate versus QueueUpdateDraw
behavior and fallback observability.
In `@internal/tui2/scroll.go`:
- Around line 32-38: Update the refresh scheduling around queueUpdateDraw so a
full uiEventCh cannot leave refreshQueued and needsRefresh latched after the
callback is dropped. Use the guaranteed scheduling path, or clear the state and
retry when enqueue fails, while preserving the existing flushRefresh and
follow-up requestRefresh behavior. Add a test covering a full queue and
verifying a later markDirty can schedule refresh again.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b1ece95-5e22-435f-9c37-89bcf9505513
📒 Files selected for processing (11)
.agents/plans/tui2-feature-parity/PLAN.mdinternal/observability/metrics.gointernal/tui2/blocks.gointernal/tui2/blocks_test.gointernal/tui2/event_queue.gointernal/tui2/event_queue_test.gointernal/tui2/proxy.gointernal/tui2/run.gointernal/tui2/scroll.gointernal/tui2/thinking.gointernal/tui2/tui2.go
💤 Files with no reviewable changes (2)
- internal/tui2/blocks_test.go
- internal/tui2/blocks.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui2/tui2.go
- internal/tui2/proxy.go
| |-------|--------|-------| | ||
| | Phase 1: Code reorganization | ✅ COMPLETE | Monolith split, duplicate state eliminated, `events.go` → `proxy.go` | | ||
| | Phase 1.5: Threading fixes | ✅ COMPLETE | Async dispatch, approval wiring, direct token write, goroutine diagnostics | | ||
| | Phase 1.6: Nonblocking architecture hardening | ✅ COMPLETE | queue/lifecycle hardening, bounded queue, coalescing, observability, and tests | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the Phase 1.6 status.
Line 63 marks Phase 1.6 complete. Line 184 marks it TODO. Line 504 marks it as the next phase. Keep one completed status across the document.
If work remains, track it in bd instead of a Markdown TODO.
As per coding guidelines, “Use Beads (bd) for all task tracking; do not use TodoWrite, TaskCreate, markdown TODO lists, or ad hoc MEMORY.md files.”
Also applies to: 182-184, 504-508
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/plans/tui2-feature-parity/PLAN.md at line 63, Synchronize all Phase
1.6 references in PLAN.md: keep the phase marked complete consistently at the
status entry, detailed section, and roadmap entry. Remove or update the
TODO/“next phase” wording, and track any remaining work through bd rather than
Markdown TODOs.
Source: Coding guidelines
| func (t *TUI2) startUIEventLoop(done <-chan struct{}) { | ||
| go func() { | ||
| for { | ||
| select { | ||
| case <-done: | ||
| return | ||
| case ev := <-t.uiEventCh: | ||
| if ev.fn == nil { | ||
| continue | ||
| } | ||
| if ev.draw { | ||
| t.App.QueueUpdateDraw(ev.fn) | ||
| } else { | ||
| t.App.QueueUpdate(ev.fn) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bind each UI worker to its own event channel.
At Line 27, the worker reads mutable t.uiEventCh without bgMu. stopBackgroundLoops clears this field, and startBackgroundLoops replaces it. This is a data race.
After a restart, an old worker can select the replacement queue instead of its closed done channel. Two workers can then dispatch callbacks and break the single-consumer ordering guarantee.
Capture the event channel when the worker starts. Pass it as a parameter to startUIEventLoop. Wait for the old worker before a replacement queue becomes active.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/event_queue.go` around lines 21 - 37, Update startUIEventLoop
to accept and capture the current UI event channel as a parameter, then read
that local channel instead of mutable t.uiEventCh. Update startBackgroundLoops
and related lifecycle code to pass the captured queue, and ensure
stopBackgroundLoops waits for the existing worker to exit before replacing or
activating a new queue, preserving single-consumer ordering across restarts.
| func (t *TUI2) queueThinkingUpdate(text string) { | ||
| t.coalesceMu.Lock() | ||
| t.pendingThinkingLabel = text | ||
| t.thinkingSeq++ | ||
| if t.thinkingQueued { | ||
| t.coalesceMu.Unlock() | ||
| observability.RecordTUIQueueEvent(context.Background(), "thinking", "coalesced", t.uiQueueDepth()) | ||
| return | ||
| } | ||
| t.thinkingQueued = true | ||
| t.coalesceMu.Unlock() | ||
|
|
||
| t.queueUpdateDraw(func() { | ||
| t.runThinkingUpdate() | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not latch coalescing flags after a queue drop.
At Lines 69 and 111, queueUpdateDraw can drop the callback when the queue is full. In that case, thinkingQueued or contextQueued remains true. Later updates only replace the pending value and return as coalesced. The update then never runs.
Set the queued flag only after an accepted enqueue, or clear and reschedule it when enqueue fails. Add full-queue coverage for both coalesced paths.
Also applies to: 98-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/event_queue.go` around lines 57 - 71, The coalescing flags set
by queueThinkingUpdate and the corresponding context-update path remain latched
when queueUpdateDraw rejects a callback. Update both enqueue paths to set
thinkingQueued/contextQueued only after successful acceptance, or clear and
reschedule them on failure, ensuring later updates can run; add coverage for
full-queue drops in both paths.
| timer := time.NewTimer(uiEventCriticalWaitLimit) | ||
| defer timer.Stop() | ||
| select { | ||
| case ch <- ev: | ||
| observability.RecordTUIQueueEvent(context.Background(), eventType, "enqueued", depth) | ||
| return | ||
| case <-done: | ||
| return | ||
| case <-timer.C: | ||
| t.uiEventFallbacks.Add(1) | ||
| observability.RecordTUIQueueEvent(context.Background(), eventType, "fallback", depth) | ||
| t.enqueueUIEventDirect(draw, fn) | ||
| } | ||
| } | ||
|
|
||
| func (t *TUI2) enqueueUIEventDirect(draw bool, fn func()) { | ||
| if draw { | ||
| go t.App.QueueUpdateDraw(fn) | ||
| } else { | ||
| go t.App.QueueUpdate(fn) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep critical overflow handling bounded.
At Lines 191-193, every critical event that exceeds the wait limit starts a new goroutine for QueueUpdate or QueueUpdateDraw. If tview cannot drain updates, each event waits 10 milliseconds and then adds another blocked goroutine. This recreates the unbounded dispatch behavior that Phase 1.6 is intended to remove.
Reserve queue capacity for critical events, or use one bounded retry mechanism instead of direct asynchronous fallback.
#!/bin/bash
set -euo pipefail
modcache="$(go env GOMODCACHE)"
app="$(fd -a -t f application.go "$modcache" | rg '/github\.com/rivo/tview@v0\.42\.0/application\.go$' | head -n 1)"
test -n "$app"
rg -n -C 16 \
'func \(a \*Application\) QueueUpdate|func \(a \*Application\) QueueUpdateDraw|updates chan' \
"$app"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/event_queue.go` around lines 182 - 202, Replace the unbounded
asynchronous fallback in enqueueUIEventDirect, used after the timer in the
critical-event dispatch path, with bounded handling that does not launch one
goroutine per overflowed event. Reserve capacity for critical events or
implement a single bounded retry mechanism while preserving the existing
QueueUpdate versus QueueUpdateDraw behavior and fallback observability.
| t.queueUpdateDraw(func() { | ||
| t.flushRefresh() | ||
| t.refreshQueued.Store(false) | ||
| if t.needsRefresh.Load() { | ||
| t.requestRefresh() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent a dropped refresh callback from latching refresh state.
At Line 32, queueUpdateDraw uses the non-critical queue path. A full uiEventCh drops this callback. refreshQueued then remains true and needsRefresh remains true. Later markDirty calls cannot schedule another refresh.
Use a guaranteed refresh scheduling path, or clear and retry the scheduling state when enqueue fails. Add a full-queue test for this case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui2/scroll.go` around lines 32 - 38, Update the refresh scheduling
around queueUpdateDraw so a full uiEventCh cannot leave refreshQueued and
needsRefresh latched after the callback is dropped. Use the guaranteed
scheduling path, or clear the state and retry when enqueue fails, while
preserving the existing flushRefresh and follow-up requestRefresh behavior. Add
a test covering a full queue and verifying a later markDirty can schedule
refresh again.
Prevent the agent goroutine from blocking forever in SetApproveFn when the TUI is unresponsive or the control channel is full. Non-blocking send to controlCh with default return prevents deadlock if the control loop is stuck. 30-second timeout on the answer channel prevents indefinite hang if the modal never renders or the TUI crashes.
1 participant
Summary
Phase 1 reorganized TUI2 into one-file-per-concern and eliminated duplicated block state. This Phase 2 fixes critical threading and UX issues discovered through SigNoz OTel trace analysis — the agent was regularly hanging on tool approval because the TUI never showed the approval modal, and the tview main thread was being pinned by flooding the updates channel during token streaming.
Root Causes Found & Fixed
1. Approval never wired (blocking stdin hang)
SetApproveFnwas only called inweb.go— never intui.goortui2.go. When bash required approval,approveTool()fell through tobufio.Scanner(os.Stdin). But tview captures stdin for keypresses, soscanner.Scan()blocked the agent goroutine forever. The TUI never showed an approval dialog, the tool goroutines were never spawned, andDoneEventwas never published.Fix: Wire
SetApproveFnincmd/yaah/tui2.gousing the sameCtrlApproval→ control channel →ShowApprovalmodal pattern already used by the question tool.2. tview
QueueUpdate()blocks the forwarder goroutinetview's
QueueUpdatewrites to a 100-slot channel and blocks on adonechannel until the main thread executes the callback AND callsdraw(). Every call site inHandleEvent(TokenDelta, ToolStart, Flush, Done, etc.) blocked the forwarder goroutine inBrokerView.forward(). During a 5s LLM stream at 50+ tokens/sec, the updates channel overflowed and the main thread was pinned indraw()between callbacks — making the TUI unresponsive.Fix: TokenDeltaEvent now writes directly to
pendingTokensfrom the forwarder goroutine via a mutex. ZeroQueueUpdatecalls during streaming. The debounce timer (100ms) handles rendering. All other event types usego QueueUpdate(Draw)to fire-and-forget. Only the control loop and debounce timer remain synchronous (must preserve message ordering and tick pacing).3. Goroutine dispatch invisible in traces
When a tool goroutine blocked before reaching
Registry.Execute(), no OTel span was ever exported becausedefer span.End()only runs when the goroutine returns. This made the blocking point invisible in SigNoz traces.Fix: Added
RecordToolGoroutinediagnostic spans at each checkpoint (spawned,acquire_concurrency,publish_start,published) with explicitspan.End()(no defer) so checkpoints survive blocked goroutines.Diagnostic Tooling
RecordTurnResponse/RecordToolDispatch/RecordToolDispatchDone— short-lived child spans recording LLM response content and tool dispatch lifecycleRecordToolGoroutine— goroutine checkpoint spans withtool.nameandtool.phaseattributestui2.refreshspan with timing breakdown (dur_total_us,dur_format_us,dur_settext_us) and stderr warning when >50msTests Added
proxy_test.goapproval_test.goagent_safety_test.goChanged Files (selected)
cmd/yaah/tui2.goSetApproveFnwith CtrlApproval channelinternal/tui2/proxy.gointernal/tui2/run.gointernal/tui2/tui2.gotokenMu,ShowApprovalFn, remove duppendingTokensinternal/tui2/modals.goShowApprovalFnoverride hookinternal/observability/trace.gointernal/agent/agent_tools.gointernal/tui2/scroll.goSummary by CodeRabbit
New Features
Improvements