-
Notifications
You must be signed in to change notification settings - Fork 3k
fix: harden auto-approval timeout cancellation with backend safeguards #11446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
0xMink
wants to merge
1
commit into
RooCodeInc:main
Choose a base branch
from
0xMink:fix/auto-approval-timeout-hardening
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
249 changes: 249 additions & 0 deletions
249
src/core/task/__tests__/auto-approval-timeout-cancellation.spec.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,249 @@ | ||
| import { Task } from "../Task" | ||
|
|
||
| // Mock checkAutoApproval to return a "timeout" decision, simulating a followup | ||
| // with auto-approve enabled and a configured countdown. | ||
| vi.mock("../../auto-approval", () => ({ | ||
| checkAutoApproval: vi.fn(), | ||
| AutoApprovalHandler: vi.fn(), | ||
| })) | ||
|
|
||
| import { checkAutoApproval } from "../../auto-approval" | ||
|
|
||
| const mockedCheckAutoApproval = vi.mocked(checkAutoApproval) | ||
|
|
||
| /** | ||
| * Regression test for the auto-approval countdown desynchronization bug. | ||
| * | ||
| * Scenario: | ||
| * 1. A followup ask is presented with auto-approve enabled and a 60s timeout. | ||
| * 2. The UI countdown starts and the backend setTimeout is scheduled. | ||
| * 3. User disables auto-approve while the countdown is running. | ||
| * 4. The UI countdown stops (FollowUpSuggest clears its interval). | ||
| * 5. ~40s later, the backend setTimeout fires and auto-selects an option. | ||
| * | ||
| * Root cause (two-timer split): | ||
| * - UI countdown: setInterval in FollowUpSuggest.tsx (visual only). | ||
| * - Auto-select: setTimeout in Task.ts (commits the choice). | ||
| * - Disabling auto-approve stopped the UI timer but did NOT cancel the | ||
| * backend setTimeout, and the callback did NOT re-check autoApprovalEnabled. | ||
| * | ||
| * Fix (three parts): | ||
| * A. webviewMessageHandler cancels timeout when autoApprovalEnabled toggled off. | ||
| * B. Task.ts timeout callback re-checks autoApprovalEnabled (defensive gate). | ||
| * C. ChatView wires onFollowUpUnmount to ChatRow so FollowUpSuggest cleanup | ||
| * actually sends the cancelAutoApproval message. | ||
| */ | ||
| describe("Auto-approval timeout cancellation", () => { | ||
| // Provider mock that returns mutable state (auto-approval can be toggled) | ||
| let mockAutoApprovalEnabled: boolean | ||
| const mockProvider = { | ||
| getState: vi.fn(async () => ({ | ||
| autoApprovalEnabled: mockAutoApprovalEnabled, | ||
| })), | ||
| postStateToWebview: vi.fn(async () => {}), | ||
| } | ||
|
|
||
| async function buildTask(): Promise<Task> { | ||
| const task = Object.create(Task.prototype) as Task | ||
| ;(task as any).abort = false | ||
| ;(task as any).clineMessages = [] | ||
| ;(task as any).askResponse = undefined | ||
| ;(task as any).askResponseText = undefined | ||
| ;(task as any).askResponseImages = undefined | ||
| ;(task as any).lastMessageTs = undefined | ||
| ;(task as any).autoApprovalTimeoutRef = undefined | ||
| ;(task as any).providerRef = { deref: () => mockProvider } | ||
|
|
||
| const { MessageQueueService } = await import("../../message-queue/MessageQueueService") | ||
| ;(task as any).messageQueueService = new MessageQueueService() | ||
|
|
||
| ;(task as any).addToClineMessages = vi.fn(async () => {}) | ||
| ;(task as any).saveClineMessages = vi.fn(async () => {}) | ||
| ;(task as any).updateClineMessage = vi.fn(async () => {}) | ||
| ;(task as any).checkpointSave = vi.fn(async () => {}) | ||
| ;(task as any).emit = vi.fn() | ||
| ;(task as any).findMessageByTimestamp = vi.fn(() => undefined) | ||
| return task | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| vi.useFakeTimers() | ||
| mockAutoApprovalEnabled = true | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers() | ||
| }) | ||
|
|
||
| it("should NOT auto-select when cancelAutoApprovalTimeout() is called before the timeout fires", async () => { | ||
| const task = await buildTask() | ||
|
|
||
| const autoSelectFn = vi.fn(() => ({ | ||
| askResponse: "messageResponse" as const, | ||
| text: "auto-selected answer", | ||
| })) | ||
|
|
||
| mockedCheckAutoApproval.mockResolvedValue({ | ||
| decision: "timeout", | ||
| timeout: 60_000, | ||
| fn: autoSelectFn, | ||
| }) | ||
|
|
||
| // Start the ask -- it will block on pWaitFor internally | ||
| const askPromise = task.ask("followup", '{"suggest":[{"answer":"yes"}]}', false) | ||
|
|
||
| // Let the initial async setup of ask() complete (checkAutoApproval, setTimeout scheduling) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // 5 seconds pass -- user is watching countdown | ||
| await vi.advanceTimersByTimeAsync(5_000) | ||
| expect(autoSelectFn).not.toHaveBeenCalled() | ||
|
|
||
| // User disables auto-approve --> extension host calls cancelAutoApprovalTimeout() | ||
| task.cancelAutoApprovalTimeout() | ||
|
|
||
| // Advance well past the original 60s deadline | ||
| await vi.advanceTimersByTimeAsync(120_000) | ||
|
|
||
| // The auto-select function must NOT have been called | ||
| expect(autoSelectFn).not.toHaveBeenCalled() | ||
|
|
||
| // Resolve the ask by simulating a manual user response. | ||
| // handleWebviewAskResponse sets askResponse without changing lastMessageTs, | ||
| // so pWaitFor resolves and the result is NOT "superseded". | ||
| task.handleWebviewAskResponse("messageResponse", "manual response") | ||
|
|
||
| // Advance to let pWaitFor poll (100ms interval) detect the response | ||
| await vi.advanceTimersByTimeAsync(200) | ||
|
|
||
| const result = await askPromise | ||
| expect(result.response).toBe("messageResponse") | ||
| expect(result.text).toBe("manual response") | ||
| }) | ||
|
|
||
| it("should NOT auto-select when auto-approve is disabled after timeout fires (defensive gate)", async () => { | ||
| const task = await buildTask() | ||
|
|
||
| const autoSelectFn = vi.fn(() => ({ | ||
| askResponse: "messageResponse" as const, | ||
| text: "auto-selected answer", | ||
| })) | ||
|
|
||
| mockedCheckAutoApproval.mockResolvedValue({ | ||
| decision: "timeout", | ||
| timeout: 10_000, // shorter timeout for this test | ||
| fn: autoSelectFn, | ||
| }) | ||
|
|
||
| const askPromise = task.ask("followup", '{"suggest":[{"answer":"yes"}]}', false) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // User disables auto-approve (state change) but does NOT call | ||
| // cancelAutoApprovalTimeout -- simulating the pre-fix scenario where | ||
| // the cancellation message was never sent. | ||
| mockAutoApprovalEnabled = false | ||
|
|
||
| // Advance past the timeout -- the callback fires but the defensive gate | ||
| // calls getState() and sees autoApprovalEnabled=false. | ||
| await vi.advanceTimersByTimeAsync(10_000) | ||
|
|
||
| // Flush microtasks to let the async gate (getState) resolve | ||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // The auto-select function must NOT have been called | ||
| expect(autoSelectFn).not.toHaveBeenCalled() | ||
|
|
||
| // Provider.getState should have been called by the defensive gate | ||
| expect(mockProvider.getState).toHaveBeenCalled() | ||
|
|
||
| // Resolve the ask manually | ||
| task.handleWebviewAskResponse("messageResponse", "manual response") | ||
| await vi.advanceTimersByTimeAsync(200) | ||
|
|
||
| const result = await askPromise | ||
| expect(result.response).toBe("messageResponse") | ||
| expect(result.text).toBe("manual response") | ||
| }) | ||
|
|
||
| it("should NOT auto-select when the ask has been superseded by a newer message", async () => { | ||
| const task = await buildTask() | ||
|
|
||
| const autoSelectFn = vi.fn(() => ({ | ||
| askResponse: "messageResponse" as const, | ||
| text: "auto-selected answer", | ||
| })) | ||
|
|
||
| mockedCheckAutoApproval.mockResolvedValue({ | ||
| decision: "timeout", | ||
| timeout: 10_000, | ||
| fn: autoSelectFn, | ||
| }) | ||
|
|
||
| const askPromise = task.ask("followup", '{"suggest":[{"answer":"yes"}]}', false) | ||
|
|
||
| // Attach rejection handler BEFORE advancing timers to prevent | ||
| // "unhandled rejection" warning when pWaitFor resolves during advancement. | ||
| const rejectionPromise = askPromise.catch((err) => err) | ||
|
|
||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // Simulate a new message arriving, which updates lastMessageTs. | ||
| // This makes the scheduled ask "stale". Use lastMessageTs + 1 to | ||
| // deterministically differ from the captured scheduledAskTs. | ||
| ;(task as any).lastMessageTs = (task as any).lastMessageTs + 1 | ||
|
|
||
| // Auto-approve is still enabled | ||
| mockAutoApprovalEnabled = true | ||
|
|
||
| // Advance past the timeout | ||
| await vi.advanceTimersByTimeAsync(10_000) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // The auto-select function must NOT have been called (stale ask) | ||
| expect(autoSelectFn).not.toHaveBeenCalled() | ||
|
|
||
| // The pWaitFor resolved because lastMessageTs !== askTs, causing | ||
| // ask() to throw AskIgnoredError("superseded"). | ||
| const error = await rejectionPromise | ||
| expect(error).toBeDefined() | ||
| expect(error.message).toMatch(/superseded/) | ||
| }) | ||
|
|
||
| it("should auto-select when conditions are still valid at timeout", async () => { | ||
| const task = await buildTask() | ||
|
|
||
| const autoSelectFn = vi.fn(() => ({ | ||
| askResponse: "messageResponse" as const, | ||
| text: "auto-selected answer", | ||
| })) | ||
|
|
||
| mockedCheckAutoApproval.mockResolvedValue({ | ||
| decision: "timeout", | ||
| timeout: 5_000, | ||
| fn: autoSelectFn, | ||
| }) | ||
|
|
||
| const askPromise = task.ask("followup", '{"suggest":[{"answer":"yes"}]}', false) | ||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // Auto-approve still enabled, no superseding message | ||
| mockAutoApprovalEnabled = true | ||
|
|
||
| // Advance past the timeout -- callback fires, gate passes, auto-selects | ||
| await vi.advanceTimersByTimeAsync(5_000) | ||
|
|
||
| // Flush microtasks for the async gate | ||
| await vi.advanceTimersByTimeAsync(0) | ||
|
|
||
| // The auto-select function SHOULD have been called | ||
| expect(autoSelectFn).toHaveBeenCalledOnce() | ||
|
|
||
| // Let pWaitFor poll detect the response | ||
| await vi.advanceTimersByTimeAsync(200) | ||
|
|
||
| const result = await askPromise | ||
| expect(result.response).toBe("messageResponse") | ||
| expect(result.text).toBe("auto-selected answer") | ||
| }) | ||
| }) |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bare
catchsilently swallows all errors with no logging. IfgetState()rejects orhandleWebviewAskResponsethrows, there will be zero diagnostic trace. The fail-safe behavior (don't auto-commit) is correct, but other catch blocks in this file (e.g., line 1697) log viaconsole.errorbefore continuing. Adding a log line here would preserve debuggability without changing the safety semantics.Fix it with Roo Code or mention @roomote and request a fix.