Repository navigation
Conversation
Co-Authored-By: Quentin Rousseau <quentin@rootly.com>
Co-Authored-By: Quentin Rousseau <quentin@rootly.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
[Medium impact] The PR adds an opt-in AI chat workflow to the CLI. The PR appears safe to merge, with non-blocking fixes for stale approvals, ignored color settings, and unnecessary rendering work. Findings
Fix with agent prompt### Issue 1
internal/agent/print.go:112-117
Print mode sends a resume request even if the checkpoint expired while the user answered the approval prompt. The TUI also skips the expiry check for `n` and Esc. This submits stale decisions instead of showing that the approval expired.
Check `expired()` immediately before sending either decision, and show the existing expiration message instead.
### Issue 2
internal/cmd/agent/agent.go:47-51
`rootly agent --no-color` still produces colored interactive output. Print mode receives the flag through `PrintOptions`, but the TUI receives no equivalent setting and still creates colored messages and styled Markdown.
Pass the setting into `NewTUIModel` and disable color in both its messages and Markdown output.
### Issue 3
internal/agent/tui.go:451-455
Every streaming redraw creates a new Glamour renderer and renders the entire accumulated answer, even when only the spinner or elapsed time changed. Long answers repeat this work throughout the turn and use more CPU than needed.
Reuse the renderer until the width changes, and cache the rendered answer until its text or width changes.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds hidden
Reviews (1) · Last reviewed commit: "fix(agent): handle interrupted and resum..." · Reviewed by Greptile |
| params = &rootly.StreamAiChatParams{ | ||
| SessionID: sessionID, | ||
| CheckpointID: &checkpointID, | ||
| Decision: &decision, | ||
| } | ||
| continue |
There was a problem hiding this comment.
Expired approvals still get sent
Print mode sends a resume request even if the checkpoint expired while the user answered the approval prompt. The TUI also skips the expiry check for n and Esc. This submits stale decisions instead of showing that the approval expired.
Check expired() immediately before sending either decision, and show the existing expiration message instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/print.go
Line: 112-117
Comment:
**Expired approvals still get sent**
Print mode sends a resume request even if the checkpoint expired while the user answered the approval prompt. The TUI also skips the expiry check for `n` and Esc. This submits stale decisions instead of showing that the approval expired.
Check `expired()` immediately before sending either decision, and show the existing expiration message instead.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| model := aiagent.NewTUIModel(cmd.Context(), client) | ||
| program := tea.NewProgram(model, | ||
| tea.WithInput(stdin), | ||
| tea.WithOutput(cmd.OutOrStdout()), | ||
| tea.WithContext(cmd.Context()), |
There was a problem hiding this comment.
Interactive mode ignores color settings
rootly agent --no-color still produces colored interactive output. Print mode receives the flag through PrintOptions, but the TUI receives no equivalent setting and still creates colored messages and styled Markdown.
Pass the setting into NewTUIModel and disable color in both its messages and Markdown output.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/agent/agent.go
Line: 47-51
Comment:
**Interactive mode ignores color settings**
`rootly agent --no-color` still produces colored interactive output. Print mode receives the flag through `PrintOptions`, but the TUI receives no equivalent setting and still creates colored messages and styled Markdown.
Pass the setting into `NewTUIModel` and disable color in both its messages and Markdown output.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| renderer, err := glamour.NewTermRenderer(glamour.WithEnvironmentConfig(), glamour.WithWordWrap(width)) | ||
| if err != nil { | ||
| return markdown | ||
| } | ||
| rendered, err := renderer.Render(markdown) |
There was a problem hiding this comment.
Spinner redraws repeat Markdown work
Every streaming redraw creates a new Glamour renderer and renders the entire accumulated answer, even when only the spinner or elapsed time changed. Long answers repeat this work throughout the turn and use more CPU than needed.
Reuse the renderer until the width changes, and cache the rendered answer until its text or width changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/tui.go
Line: 451-455
Comment:
**Spinner redraws repeat Markdown work**
Every streaming redraw creates a new Glamour renderer and renders the entire accumulated answer, even when only the spinner or elapsed time changed. Long answers repeat this work throughout the turn and use more CPU than needed.
Reuse the renderer until the width changes, and cache the rendered answer until its text or width changes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Co-Authored-By: Quentin Rousseau <quentin@rootly.com>
Co-Authored-By: Quentin Rousseau <quentin@rootly.com>
|
Superseded by #74, which now targets master and includes this branch (phases 1 and 2 in one PR, per Quentin). Not merged. |
Why
Phase 1 of the CLI AI agent: chat with Rootly AI from the terminal, using the
/v1/ai/chat/streamSSE API through rootly-go'sStreamAiChatEvents.What
rootly agentcommand (aliaschat). Barerootlyand all existing subcommands are unchanged.Cancelled.) or on error./new,/help,/exit,/quit.--no-colorgives raw markdown and an ASCII colour profile (no colour codes at all).confirmation_requiredshows the question and JSON preview.yresumes withdecision=confirm;n/Esc resumes withcancel(samesession_id+checkpoint_id). Expiry is re-checked after the answer, so an expired checkpoint is never resumed in either mode. The resumed text starts on a new paragraph.rootly agent -p "q", or a non-TTY stdin (cat log | rootly agent -p "explain"appends stdin after the prompt).y/Nprompt on a TTY; non-interactive input exits non-zero.internal/api:Client.StreamAiChatEventsreuses the existing auth/transport (API key or OAuth). The--debugtransport no longer readstext/event-streambodies, which previously blocked on the live stream.ai.chat:write(re-login hint) and 429.master(v0.14.1-0.20261029113308-2c148c1fd796), which has the stream API plus the session/current-user endpoints for phase 2. No existing call sites needed changes.Known server behaviour
Ctrl+C only stops the CLI. The server keeps generating and saves the reply (
SseStreamerswallows the disconnect), so a cancelled turn's reply can land in the session after the next turn's. The UI is unaffected.Risk
Low. The command is hidden, so existing commands are untouched apart from the debug-transport change, which only affects SSE responses.
Rollback
Revert the PR.
Link to Devin session: https://rootly.devinenterprise.com/sessions/28ad93d126254ea18116565e125bb089
Open in Devin Desktop: https://rootly.devinenterprise.com/desktop/session/28ad93d126254ea18116565e125bb089?variant=devin
Requested by: @kwent