fix/cancel-preserve-progress#182
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates assistant partial-prompt persistence to track pending tool calls and synthesize failure messages, and changes terminal prompt cancellation and response handling so canceled prompts keep their progress state. ChangesAssistant Runtime Persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Terminal Prompt Cancellation Flow
Estimated code review effort: 4 (Complex) | ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 82.89% 82.93% +0.04%
==========================================
Files 287 287
Lines 23127 23166 +39
==========================================
+ Hits 19171 19213 +42
+ Misses 2799 2796 -3
Partials 1157 1157
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/terminal/prompt_response.go (1)
11-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the prompt ID/canceled guard before applying a response.
Line 11 discards
promptID, so a canceled prompt that still returns a non-nil response can be committed as a normal assistant response. Preserve progress, but do not accept responses for missing, mismatched, or canceled active prompts.Proposed fix
-func (app *App) applyPromptResponse(ctx context.Context, response *assistant.PromptResponse, _ uint64) { +func (app *App) applyPromptResponse(ctx context.Context, response *assistant.PromptResponse, promptID uint64) { streamingBlocks := append([]chatMessage(nil), app.transcript.Streaming.Blocks...) + if app.activePrompt == nil || app.activePrompt.ID != promptID { + return + } + if app.activePrompt.Canceled { + app.working = false + app.streamingText = "" + app.streamingThinkingText = "" + app.resetStreamingBlocks() + app.streamedToolEvents = 0 + app.activePrompt = nil + app.applyFailedPromptStreamedBlocks(streamingBlocks) + app.setStatus("response canceled; progress saved") + app.processQueuedPrompt(ctx) + + return + } + app.working = false app.streamingText = "" app.streamingThinkingText = ""🤖 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/terminal/prompt_response.go` around lines 11 - 31, In applyPromptResponse, keep the existing promptID/canceled validation before committing any non-nil response so stale or canceled prompts are not applied as assistant messages. Preserve the progress/state updates already needed, but only call app.applyPromptResponseSideEffects, app.applyTokenUsage, and app.addMessage after confirming the response still matches the current active prompt and has not been canceled; otherwise discard it and continue with queued prompts.
🧹 Nitpick comments (1)
internal/assistant/runtime_test.go (1)
676-683: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider table-driven consolidation for the four new completer variants and their tests.
partialFailureCompleter,canceledPartialFailureCompleter,pendingToolFailureCompleter,pendingCanceledToolFailureCompleter, andcompletedToolFailureCompleterall follow the identicalComplete()shape, differing only by whichemit*helpers are called and which error is returned. Same for the four test functions at lines 315-387, which share the "run Prompt, fetch messages, assert" structure. This is a strong candidate for a single table-driven test with a struct-per-case (event sequence, error, expected message assertions), reducing duplication across five near-identical types and four near-identical tests.As per coding guidelines, "Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs in Go."
Also applies to: 735-786
🤖 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/assistant/runtime_test.go` around lines 676 - 683, Consolidate the duplicated completer variants and their tests into table-driven forms: the `partialFailureCompleter`, `canceledPartialFailureCompleter`, `pendingToolFailureCompleter`, `pendingCanceledToolFailureCompleter`, and `completedToolFailureCompleter` types all implement the same `Complete()` pattern with only helper calls and returned errors changing, so replace them with a shared case-driven implementation keyed by the existing `Complete` behavior. Likewise, merge the four near-identical test functions into one table-driven test that exercises `Prompt`, collects messages, and asserts per-case expectations using a struct describing the event sequence, error, and expected message checks.Source: Coding guidelines
🤖 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 `@internal/assistant/runtime_persist.go`:
- Around line 276-291: The trackPendingTool method on partialPromptProgress
currently appends a pending tool even when ToolCallEvent is nil and fallbackName
is empty, which later creates an unidentifiable synthetic result. Update
trackPendingTool to skip adding anything when both call.Name and fallbackName
are empty, while preserving the existing fallback behavior for named tools and
keeping pendingTools consistent.
In `@internal/assistant/runtime_test.go`:
- Around line 371-387: The test name suggests a canceled prompt scenario, but
the current setup in
TestRuntime_PromptDoesNotDuplicateCanceledResultForCompletedTool uses
completedToolFailureCompleter, which returns a generic error instead of
context.Canceled. Update this test to use the cancellation path (for example, a
completer that returns context.Canceled, like
pendingCanceledToolFailureCompleter) so the deduplication behavior is verified
under the actual canceled-and-completed tool case. Keep the existing assertions
against runtime.Prompt and repository.Messages, and ensure the checked
ToolResult content still excludes the canceled text.
In `@internal/terminal/async_events.go`:
- Line 230: The async event filter is incorrectly gating compaction events on
activePrompt, which causes asyncEventCompactStart/Done/Error to be dropped when
no prompt is active. Update isPromptAsyncEvent in async_events.go so
compaction-related payloads are handled using activeCompaction (or bypass the
activePrompt check for those event types), while keeping prompt events filtered
by PromptID. Make the logic branch on the event type before applying the
activePrompt guard so compaction completion and error events always reach the UI
state machine.
- Around line 489-505: The prompt error path in applyPromptError clears working
state but never drains queued follow-ups, so a queued prompt can remain stuck
after cancellation/error handling. Update applyPromptError to accept ctx like
the surrounding prompt flow, and after the status/message update call the
queue-draining path used by sendPrompt/processQueuedPrompt so any pending prompt
in queuedMessages is resumed once the terminal prompt finishes.
---
Outside diff comments:
In `@internal/terminal/prompt_response.go`:
- Around line 11-31: In applyPromptResponse, keep the existing promptID/canceled
validation before committing any non-nil response so stale or canceled prompts
are not applied as assistant messages. Preserve the progress/state updates
already needed, but only call app.applyPromptResponseSideEffects,
app.applyTokenUsage, and app.addMessage after confirming the response still
matches the current active prompt and has not been canceled; otherwise discard
it and continue with queued prompts.
---
Nitpick comments:
In `@internal/assistant/runtime_test.go`:
- Around line 676-683: Consolidate the duplicated completer variants and their
tests into table-driven forms: the `partialFailureCompleter`,
`canceledPartialFailureCompleter`, `pendingToolFailureCompleter`,
`pendingCanceledToolFailureCompleter`, and `completedToolFailureCompleter` types
all implement the same `Complete()` pattern with only helper calls and returned
errors changing, so replace them with a shared case-driven implementation keyed
by the existing `Complete` behavior. Likewise, merge the four near-identical
test functions into one table-driven test that exercises `Prompt`, collects
messages, and asserts per-case expectations using a struct describing the event
sequence, error, and expected message checks.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94c35756-ffe1-445c-b077-5d9b53fb5048
📒 Files selected for processing (15)
internal/assistant/runtime.gointernal/assistant/runtime_persist.gointernal/assistant/runtime_persist_internal_test.gointernal/assistant/runtime_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/async_events_internal_test.gointernal/terminal/interrupt_internal_test.gointernal/terminal/prompt_cancel.gointernal/terminal/prompt_cancel_internal_test.gointernal/terminal/prompt_response.gointernal/terminal/prompt_response_internal_test.gointernal/terminal/prompt_send.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/render_internal_test.go
💤 Files with no reviewable changes (1)
- internal/terminal/prompt_response_internal_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/terminal/prompt_response_internal_test.go (1)
146-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Solid coverage of the canceled-preserve-progress path.
Consider also adding a case for
Canceled=truewith anilresponse (a plausible caller path, sinceapplyPromptResponseis invoked withresponse == nilelsewhere in this same test file) to lock in thatfinishPrompt/applyFailedPromptStreamedBlocksstill run safely without touchingresponse.🤖 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/terminal/prompt_response_internal_test.go` around lines 146 - 166, The canceled-progress test in applyPromptResponse only covers a non-nil response path, so add a companion case where activePrompt.Canceled is true and the response passed to applyPromptResponse is nil. Reuse the existing terminal prompt test setup around TestApplyPromptResponsePreservesCanceledProgress and assert that finishPrompt and applyFailedPromptStreamedBlocks still complete safely, with no attempt to read response content and the same preserved-progress state transitions.internal/terminal/prompt_response.go (1)
17-24: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCanceled branch silently drops usage/session data from a completed response.
When
app.activePrompt.Canceledis true butresponseis non-nil (the request actually finished on the backend), this branch never callsapp.applyTokenUsage(&response.Usage)or updatesapp.sessionID. If the model call completed and consumed tokens despite the UI cancellation, that usage is lost from the running total. Given the PR's goal is to "preserve progress," consider whether usage/session bookkeeping should also be preserved for late-but-real responses (guarding forresponse != nilsince this function is also invoked with a nil response).Possible adjustment
if app.activePrompt.Canceled { app.finishPrompt() app.applyFailedPromptStreamedBlocks(streamingBlocks) + if response != nil { + app.sessionID = response.SessionID + app.applyTokenUsage(&response.Usage) + } app.setStatus("response canceled; progress saved") app.processQueuedPrompt(ctx) return }🤖 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/terminal/prompt_response.go` around lines 17 - 24, The canceled path in prompt_response handling is dropping late response bookkeeping even when the backend returned a real response. Update the app.activePrompt.Canceled branch in the prompt response flow to preserve usage and session state by calling app.applyTokenUsage(&response.Usage) and updating app.sessionID when response is non-nil, while still guarding against nil responses before accessing response fields. Keep the existing finishPrompt, applyFailedPromptStreamedBlocks, setStatus, and processQueuedPrompt behavior intact.
🤖 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.
Nitpick comments:
In `@internal/terminal/prompt_response_internal_test.go`:
- Around line 146-166: The canceled-progress test in applyPromptResponse only
covers a non-nil response path, so add a companion case where
activePrompt.Canceled is true and the response passed to applyPromptResponse is
nil. Reuse the existing terminal prompt test setup around
TestApplyPromptResponsePreservesCanceledProgress and assert that finishPrompt
and applyFailedPromptStreamedBlocks still complete safely, with no attempt to
read response content and the same preserved-progress state transitions.
In `@internal/terminal/prompt_response.go`:
- Around line 17-24: The canceled path in prompt_response handling is dropping
late response bookkeeping even when the backend returned a real response. Update
the app.activePrompt.Canceled branch in the prompt response flow to preserve
usage and session state by calling app.applyTokenUsage(&response.Usage) and
updating app.sessionID when response is non-nil, while still guarding against
nil responses before accessing response fields. Keep the existing finishPrompt,
applyFailedPromptStreamedBlocks, setStatus, and processQueuedPrompt behavior
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30ec0ab2-337c-44f1-a78f-f427066361ba
📒 Files selected for processing (7)
internal/assistant/runtime_persist.gointernal/assistant/runtime_test.gointernal/terminal/async_events.gointernal/terminal/async_events_internal_test.gointernal/terminal/prompt_response.gointernal/terminal/prompt_response_internal_test.gointernal/terminal/render_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/terminal/render_internal_test.go
- internal/terminal/async_events.go
- internal/assistant/runtime_test.go
- internal/assistant/runtime_persist.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.