diff --git a/.github/alloy/CompletionPersistence.als b/.github/alloy/CompletionPersistence.als new file mode 100644 index 0000000000..579f43448f --- /dev/null +++ b/.github/alloy/CompletionPersistence.als @@ -0,0 +1,141 @@ +module CompletionPersistence + +abstract sig CompletionPolicy {} +one sig CurrentPolicy, DurableFirstPolicy extends CompletionPolicy {} + +one sig Config { + policy: one CompletionPolicy +} + +one sig Marker {} + +one sig Lifecycle { + var historyWriteStarted: lone Marker, + var historyDurable: lone Marker, + var completionAccepted: lone Marker, + var completionEmitted: lone Marker, + var hostStopped: lone Marker +} + +pred init { + no Lifecycle.historyWriteStarted + no Lifecycle.historyDurable + no Lifecycle.completionAccepted + no Lifecycle.completionEmitted + no Lifecycle.hostStopped +} + +pred startHistoryWrite { + no Lifecycle.historyWriteStarted + no Lifecycle.hostStopped + Lifecycle.historyWriteStarted' = Marker + Lifecycle.historyDurable' = Lifecycle.historyDurable + Lifecycle.completionAccepted' = Lifecycle.completionAccepted + Lifecycle.completionEmitted' = Lifecycle.completionEmitted + Lifecycle.hostStopped' = Lifecycle.hostStopped +} + +pred finishHistoryWrite { + some Lifecycle.historyWriteStarted + no Lifecycle.historyDurable + no Lifecycle.hostStopped + Lifecycle.historyWriteStarted' = Lifecycle.historyWriteStarted + Lifecycle.historyDurable' = Marker + Lifecycle.completionAccepted' = Lifecycle.completionAccepted + Lifecycle.completionEmitted' = Lifecycle.completionEmitted + Lifecycle.hostStopped' = Lifecycle.hostStopped +} + +pred acceptCompletion { + no Lifecycle.completionAccepted + no Lifecycle.hostStopped + Lifecycle.historyWriteStarted' = Lifecycle.historyWriteStarted + Lifecycle.historyDurable' = Lifecycle.historyDurable + Lifecycle.completionAccepted' = Marker + Lifecycle.completionEmitted' = Lifecycle.completionEmitted + Lifecycle.hostStopped' = Lifecycle.hostStopped +} + +pred emitCompletion { + some Lifecycle.historyWriteStarted + some Lifecycle.completionAccepted + no Lifecycle.completionEmitted + no Lifecycle.hostStopped + Config.policy = DurableFirstPolicy implies some Lifecycle.historyDurable + Lifecycle.historyWriteStarted' = Lifecycle.historyWriteStarted + Lifecycle.historyDurable' = Lifecycle.historyDurable + Lifecycle.completionAccepted' = Lifecycle.completionAccepted + Lifecycle.completionEmitted' = Marker + Lifecycle.hostStopped' = Lifecycle.hostStopped +} + +pred stopHost { + some Lifecycle.completionEmitted + no Lifecycle.hostStopped + Lifecycle.historyWriteStarted' = Lifecycle.historyWriteStarted + Lifecycle.historyDurable' = Lifecycle.historyDurable + Lifecycle.completionAccepted' = Lifecycle.completionAccepted + Lifecycle.completionEmitted' = Lifecycle.completionEmitted + Lifecycle.hostStopped' = Marker +} + +pred stutter { + Lifecycle.historyWriteStarted' = Lifecycle.historyWriteStarted + Lifecycle.historyDurable' = Lifecycle.historyDurable + Lifecycle.completionAccepted' = Lifecycle.completionAccepted + Lifecycle.completionEmitted' = Lifecycle.completionEmitted + Lifecycle.hostStopped' = Lifecycle.hostStopped +} + +fact traces { + init + always ( + startHistoryWrite or + finishHistoryWrite or + acceptCompletion or + emitCompletion or + stopHost or + stutter + ) +} + +pred DurableFirstHappyPath { + Config.policy = DurableFirstPolicy + eventually ( + some Lifecycle.hostStopped and + some Lifecycle.completionEmitted and + some Lifecycle.historyDurable + ) +} + +assert CurrentCompletionIsDurable { + Config.policy = CurrentPolicy implies + always (some Lifecycle.completionEmitted implies some Lifecycle.historyDurable) +} + +assert CurrentShutdownPreservesHistory { + Config.policy = CurrentPolicy implies + always ( + some Lifecycle.hostStopped and some Lifecycle.completionEmitted implies + some Lifecycle.historyDurable + ) +} + +assert DurableFirstCompletionIsDurable { + Config.policy = DurableFirstPolicy implies + always (some Lifecycle.completionEmitted implies some Lifecycle.historyDurable) +} + +assert DurableFirstShutdownPreservesHistory { + Config.policy = DurableFirstPolicy implies + always ( + some Lifecycle.hostStopped and some Lifecycle.completionEmitted implies + some Lifecycle.historyDurable + ) +} + +check CurrentCompletionIsDurable for 6 but 6 steps +check CurrentShutdownPreservesHistory for 6 but 6 steps +run DurableFirstHappyPath for 6 but 6 steps +check DurableFirstCompletionIsDurable for 6 but 8 steps +check DurableFirstShutdownPreservesHistory for 6 but 8 steps diff --git a/.github/alloy/README.md b/.github/alloy/README.md new file mode 100644 index 0000000000..cdf184bfb5 --- /dev/null +++ b/.github/alloy/README.md @@ -0,0 +1,57 @@ +# Completion persistence model + +`CompletionPersistence.als` models the narrow lifecycle behind the restart-persistence E2E failure: + +- the streamed assistant history write starts; +- completion is accepted and `TaskCompleted` is emitted; +- the history write becomes durable; +- the extension host stops after observing completion. + +The model compares two event contracts: + +- `CurrentPolicy` permits `TaskCompleted` once completion is accepted and a history write has started; +- `DurableFirstPolicy` additionally requires the history write to be durable before completion is emitted. + +The current-policy assertions search for a hypothesized, contract-permitted bad shape: the host sees completion and stops while API history is still not durable. Here, durable means that the required history version is visible to a fresh extension host; the model does not claim power-loss durability or filesystem `fsync` semantics. The durability-gated assertions check that completion and shutdown cannot expose that state. + +The model is intentionally small. It establishes the missing ordering invariant but does not prove that the CI failure followed this exact trace or that every concrete runtime path maps to the abstract current-policy transition. Unrestricted stuttering also means this is a bounded safety model: it does not guarantee write completion, retries, or eventual task completion when persistence keeps failing. + +## Deterministic production regression + +`src/core/task/__tests__/Task.persistence.spec.ts` blocks the real `saveApiMessages` boundary on a deferred promise and accepts completion on the same `Task`. It confirms that `TaskCompleted` remains pending while the write is unresolved, then emits after the write succeeds. A second case exhausts the bounded persistence retries and confirms that the failure is reported without emitting `TaskCompleted`. + +The test maps to the model as follows: + +- the captured `saveApiMessages` call for the assistant `attempt_completion` turn is `startHistoryWrite`; +- the unresolved deferred save is `not historyDurable`; +- accepting the matching completion call is `acceptCompletion`; +- resolving the deferred is `finishHistoryWrite`; +- observing `TaskCompleted` afterward is `emitCompletion`. + +An indefinitely delayed write keeps completion pending rather than weakening the public event contract. A failed initial write is retried with the existing bounded retry policy; if all retries fail, the completion handler reports the persistence error and does not emit `TaskCompleted`. + +## Code mapping + +- `startHistoryWrite` and `finishHistoryWrite` represent `Task.saveApiConversationHistory()` entering and completing its durable file write. +- `acceptCompletion` and `emitCompletion` represent completion approval followed by `AttemptCompletionTool.emitPublicTaskCompleted()`. +- `stopHost` represents the restart E2E (or a real extension shutdown) acting on the public completion event. +- `DurableFirstPolicy` represents the production contract: the public completion boundary is not crossed until the required API history write succeeds. + +## Run Alloy 6 + +Download the pinned Alloy release, verify it, and execute all commands: + +```bash +cd .github/alloy +curl -fsSL https://github.com/AlloyTools/org.alloytools.alloy/releases/download/v6.2.0/org.alloytools.alloy.dist.jar -o alloy.jar +printf '%s %s\n' '6b8c1cb5bc93bedfc7c61435c4e1ab6e688a242dc702a394628d9a9801edb78d' alloy.jar | sha256sum --check +java -jar alloy.jar exec -c '*' -t text -o - CompletionPersistence.als +``` + +Expected results: + +- both `Current...` checks produce counterexamples where completion precedes durable history, including a trace that stops the host in that state; +- `DurableFirstHappyPath` is satisfiable, so the stronger guard does not prevent completion; +- both `DurableFirst...` assertions have no counterexample within the configured bounds. + +The JAR is a local analysis tool and must not be committed. diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 29e7fa3ddd..dfd55af923 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -43,11 +43,6 @@ async function runCreate(api: RooCodeAPI): Promise { }) await waitUntilCompleted({ api, taskId }) assert.strictEqual(sawMarker, true, `Completion should include ${MARKER}`) - const historyItem = await api.getTaskHistoryItem(taskId) - assert.ok(historyItem, "Completed task should have a history item") - assert.ok(historyItem.task.includes("RESTART_PERSISTENCE_SMOKE"), "History title should include the marker") - const conversationLength = await api.getTaskApiConversationHistoryLength(taskId) - assert.ok(conversationLength > 0, "Completed task should persist API conversation history") const result: PhaseResult = { version: PHASE_RESULT_VERSION, @@ -84,14 +79,22 @@ async function runVerify(api: RooCodeAPI): Promise { const historyItem = await api.getTaskHistoryItem(taskId) assert.ok(historyItem, "Task history item should be available after restart") assert.ok(historyItem.task.includes("RESTART_PERSISTENCE_SMOKE"), "History title should persist after restart") - const conversationLength = await api.getTaskApiConversationHistoryLength(taskId) - assert.ok(conversationLength > 0, "API conversation history should be available after restart") + const restoredCompletion = await api.hasTaskApiConversationHistorySequence(taskId, { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: MARKER, + }) + assert.strictEqual( + restoredCompletion, + true, + "Fresh-host history should restore the marked user turn followed by its assistant completion", + ) await writePhaseResult(getResultsDir(), { version: PHASE_RESULT_VERSION, phase: "verify", status: "passed", - values: { taskId, conversationLength: String(conversationLength) }, + values: { taskId }, }) await quitGracefully() } catch (error) { diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index 961b068778..de23f67491 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -10,6 +10,12 @@ import type { WebviewThemeFixture } from "./vscode-extension-host.js" export type RooCodeAPIEvents = RooCodeEvents +export interface TaskApiConversationHistorySequence { + userText: string + assistantToolName: string + assistantToolInputText: string +} + export interface RooCodeAPI extends EventEmitter { /** * Starts a new task with an optional initial message and images. @@ -52,6 +58,16 @@ export interface RooCodeAPI extends EventEmitter { * @returns The number of persisted API conversation history entries, or 0 if unavailable. */ getTaskApiConversationHistoryLength(taskId: string): Promise + /** + * Checks for an ordered user turn and assistant tool call in persisted API history. + * @param taskId The ID of the task. + * @param sequence The expected user text and assistant tool-call markers. + * @returns True when the expected turns exist in order, or false if unavailable. + */ + hasTaskApiConversationHistorySequence( + taskId: string, + sequence: TaskApiConversationHistorySequence, + ): Promise /** * Returns the current task stack. * @returns An array of task IDs. diff --git a/packages/types/src/events.ts b/packages/types/src/events.ts index fc6c3c25d4..20f7f7e71e 100644 --- a/packages/types/src/events.ts +++ b/packages/types/src/events.ts @@ -14,6 +14,7 @@ export enum RooCodeEventName { // Task Lifecycle TaskStarted = "taskStarted", + /** Emitted after the accepted completion turn is persisted and visible to a fresh extension host. */ TaskCompleted = "taskCompleted", TaskAborted = "taskAborted", TaskFocused = "taskFocused", diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index e830798b16..c1c09b6af4 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1379,6 +1379,7 @@ describe("History resume delegation - parent metadata transitions", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), } as unknown as import("../core/task/Task").Task const block = { diff --git a/src/__tests__/nested-delegation-resume.spec.ts b/src/__tests__/nested-delegation-resume.spec.ts index 8464f81b12..a07411f782 100644 --- a/src/__tests__/nested-delegation-resume.spec.ts +++ b/src/__tests__/nested-delegation-resume.spec.ts @@ -204,6 +204,7 @@ describe("Nested delegation resume (A → B → C)", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), } as unknown as Task const blockC = { @@ -252,6 +253,7 @@ describe("Nested delegation resume (A → B → C)", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), } as unknown as Task const blockB = { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 37281a9010..9c79228934 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -402,9 +402,12 @@ export class Task extends EventEmitter implements TaskLike { * appear BEFORE the assistant message with tool_uses, causing API errors. * * Reset to `false` at the start of each API request. - * Set to `true` after the assistant message is saved in `recursivelyMakeClineRequests`. + * Set to `true` only after the assistant message is durably saved. */ assistantMessageSavedToHistory = false + private assistantMessagePersistencePromise!: Promise + private resolveAssistantMessagePersistence!: (saved: boolean) => void + private completionPersistenceReadyPromise?: Promise /** * Fire-and-forget wrapper around `presentAssistantMessage` that swallows the @@ -511,6 +514,7 @@ export class Task extends EventEmitter implements TaskLike { diffFuzzyThreshold, }: TaskOptions) { super() + this.resetAssistantMessagePersistence() if (startTask && !task && !images && !historyItem) { throw new Error("Either historyItem or task/images must be provided") @@ -979,7 +983,8 @@ export class Task extends EventEmitter implements TaskLike { return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } - private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string) { + /** Appends an API turn and records whether an assistant turn reached persistent storage. */ + private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string): Promise { const resolvesPendingAction = this.pendingAction && message.role === "user" && @@ -1011,6 +1016,40 @@ export class Task extends EventEmitter implements TaskLike { ) } } + if (message.role === "assistant") { + this.assistantMessageSavedToHistory = saved + this.resolveAssistantMessagePersistence(saved) + } + } + + /** Creates the persistence boundary for the next streamed assistant turn. */ + private resetAssistantMessagePersistence(): void { + this.assistantMessagePersistencePromise = new Promise((resolve) => { + this.resolveAssistantMessagePersistence = resolve + }) + this.completionPersistenceReadyPromise = undefined + } + + /** + * Waits until the current assistant turn is visible to a fresh extension host. + * A public completion event must not be emitted before this boundary succeeds. + */ + public waitForCurrentAssistantMessagePersistence(): Promise { + if (!this.completionPersistenceReadyPromise) { + const currentPersistence = this.assistantMessagePersistencePromise + this.completionPersistenceReadyPromise = (async () => { + const saved = await currentPersistence + if (saved) return + + const retrySucceeded = await this.retrySaveApiConversationHistory() + if (!retrySucceeded) { + throw new Error("Failed to persist API conversation history before task completion") + } + this.assistantMessageSavedToHistory = true + })() + } + + return this.completionPersistenceReadyPromise } // NOTE: We intentionally do NOT mutate stored messages to merge consecutive user turns. @@ -2991,6 +3030,7 @@ export class Task extends EventEmitter implements TaskLike { this.didRejectTool = false this.didAlreadyUseTool = false this.assistantMessageSavedToHistory = false + this.resetAssistantMessagePersistence() // Reset tool failure flag for each new assistant turn - this ensures that tool failures // only prevent attempt_completion within the same assistant message, not across turns // (e.g., if a tool fails, then user sends a message saying "just complete anyway") @@ -3800,7 +3840,6 @@ export class Task extends EventEmitter implements TaskLike { { role: "assistant", content: assistantContent }, reasoningMessage || undefined, ) - this.assistantMessageSavedToHistory = true this.messageCounts.assistant++ } diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 671bd7d4b7..8b09fbc5d3 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -4,7 +4,13 @@ import * as os from "os" import * as path from "path" import * as vscode from "vscode" -import type { ClineMessage, GlobalState, PendingTaskAction, ProviderSettings } from "@roo-code/types" +import { + RooCodeEventName, + type ClineMessage, + type GlobalState, + type PendingTaskAction, + type ProviderSettings, +} from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import type { Anthropic } from "@anthropic-ai/sdk" @@ -12,9 +18,11 @@ import { Task } from "../Task" import { ClineProvider } from "../../webview/ClineProvider" import { ContextProxy } from "../../config/ContextProxy" import { providerIdentifiers } from "@roo-code/types/provider-identifiers" +import { attemptCompletionTool, type AttemptCompletionCallbacks } from "../../tools/AttemptCompletionTool" +import type { AttemptCompletionToolUse } from "../../../shared/tools" type TaskPersistenceAccess = { - addToApiConversationHistory: (message: { role: "user"; content: unknown[] }) => Promise + addToApiConversationHistory: (message: Anthropic.MessageParam) => Promise resumeTaskFromHistory: () => Promise resumePendingTaskAction: (action: PendingTaskAction) => Promise saveClineMessages: () => Promise @@ -385,6 +393,167 @@ describe("Task persistence", () => { // But the content should be the same expect(callArgs.messages).toEqual(task.apiConversationHistory) }) + + it("emits TaskCompleted only after API history persistence succeeds", async () => { + const saveDeferred = createDeferred() + mockSaveApiMessages.mockReturnValueOnce(saveDeferred.promise) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const privateTask = getTaskPersistenceAccess(task) + const completionCallId = "completion-call" + let saveSettled = false + let completionEmitted = false + let saving: Promise | undefined + + try { + vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) + vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) + task.on(RooCodeEventName.TaskCompleted, () => { + completionEmitted = true + }) + + const block: AttemptCompletionToolUse = { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + params: { result: "done" }, + nativeArgs: { result: "done" }, + partial: false, + } + const callbacks: AttemptCompletionCallbacks = { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: vi.fn(), + askFinishSubTaskApproval: vi.fn(), + toolDescription: vi.fn(), + toolCallId: completionCallId, + } + + const handlingCompletion = attemptCompletionTool.handle(task, block, callbacks) + await vi.waitFor(() => expect(task.ask).toHaveBeenCalled()) + + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(completionEmitted).toBe(false) + expect(mockSaveApiMessages).not.toHaveBeenCalled() + + saving = privateTask.addToApiConversationHistory({ + role: "assistant", + content: [ + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + input: { result: "done" }, + }, + ], + }) + void saving.finally(() => { + saveSettled = true + }) + await vi.waitFor(() => expect(mockSaveApiMessages).toHaveBeenCalledTimes(1)) + const saveRequest = mockSaveApiMessages.mock.calls[0][0] + expect(saveRequest.taskId).toBe(task.taskId) + expect(saveRequest.messages).toEqual([ + expect.objectContaining({ + role: "assistant", + content: expect.arrayContaining([ + expect.objectContaining({ + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + }), + ]), + }), + ]) + expect(saveSettled).toBe(false) + expect(completionEmitted).toBe(false) + + saveDeferred.resolve(undefined) + await Promise.all([saving, handlingCompletion]) + + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(completionEmitted).toBe(true) + } finally { + saveDeferred.resolve(undefined) + await saving + } + }) + + it("does not emit TaskCompleted when API history persistence exhausts its retries", async () => { + vi.useFakeTimers() + mockSaveApiMessages.mockRejectedValue(new Error("write failed")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const completionCallId = "failed-completion-call" + const privateTask = getTaskPersistenceAccess(task) + const callbacks: AttemptCompletionCallbacks = { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: vi.fn(), + askFinishSubTaskApproval: vi.fn(), + toolDescription: vi.fn(), + toolCallId: completionCallId, + } + vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) + vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) + const completionListener = vi.fn() + task.on(RooCodeEventName.TaskCompleted, completionListener) + + try { + await privateTask.addToApiConversationHistory({ + role: "assistant", + content: [ + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + input: { result: "done" }, + }, + ], + }) + + const handlingCompletion = attemptCompletionTool.handle( + task, + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + params: { result: "done" }, + nativeArgs: { result: "done" }, + partial: false, + }, + callbacks, + ) + await vi.runAllTimersAsync() + await handlingCompletion + + expect(mockSaveApiMessages).toHaveBeenCalledTimes(4) + expect(completionListener).not.toHaveBeenCalled() + expect(callbacks.handleError).toHaveBeenCalledWith( + "inspecting site", + expect.objectContaining({ + message: "Failed to persist API conversation history before task completion", + }), + ) + } finally { + mockSaveApiMessages.mockResolvedValue(undefined) + vi.useRealTimers() + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index a71520b5cc..d6f9a320ca 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -142,6 +142,13 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { task.flushTelemetryInstallment("attempt_completion") hasFlushedTelemetry = true + try { + await task.waitForCurrentAssistantMessagePersistence() + } catch (error) { + await handleError("persisting task completion", error as Error) + return + } + const delegation = await this.delegateToParent( task, result, @@ -151,7 +158,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { pushToolResult, ) if (delegation === "delegated") { - this.emitPublicTaskCompleted(task) + await this.emitPublicTaskCompleted(task) } if (delegation !== "continue") return } else { @@ -207,7 +214,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { // subtask that already completed (and already emitted TaskCompleted) the first // time through -- re-acknowledging it from history must not emit it again. if (!isStaleHistoryReplay) { - this.emitPublicTaskCompleted(task) + await this.emitPublicTaskCompleted(task) } return } @@ -290,10 +297,12 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { /** * Emits the public RooCodeEventName.TaskCompleted API event. Only called once the * task is genuinely finished (user accepted, or a subtask was successfully delegated - * back to its parent) -- unlike the PostHog telemetry flush, which reports on every - * model-initiated attempt_completion call regardless of outcome. + * back to its parent) and the matching assistant turn is restart-visible -- unlike the + * PostHog telemetry flush, which reports on every model-initiated attempt_completion call. */ - private emitPublicTaskCompleted(task: Task): void { + private async emitPublicTaskCompleted(task: Task): Promise { + await task.waitForCurrentAssistantMessagePersistence() + // Force final token usage update before emitting TaskCompleted. // This ensures the latest stats are captured regardless of throttle timer. task.emitFinalTokenUsageUpdate() diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 5e57ca726f..232e8092d3 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -76,6 +76,7 @@ describe("attemptCompletionTool", () => { flushTelemetryInstallment: vi.fn(), setPendingTaskAction: vi.fn(), persistQueuedFeedbackAndAcknowledge: vi.fn().mockResolvedValue(true), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), } }) @@ -478,6 +479,10 @@ describe("attemptCompletionTool", () => { describe("completion lifecycle", () => { it("delegates an active subtask completion when the active parent awaits that child", async () => { + let markPersistenceReady!: () => void + const persistenceReady = new Promise((resolve) => { + markPersistenceReady = resolve + }) const block: AttemptCompletionToolUse = { type: "tool_use", name: "attempt_completion", @@ -507,6 +512,7 @@ describe("attemptCompletionTool", () => { taskId: "child-1", parentTaskId: "parent-1", providerRef: { deref: () => mockProvider }, + waitForCurrentAssistantMessagePersistence: vi.fn(() => persistenceReady), }) mockAskFinishSubTaskApproval.mockResolvedValue(true) @@ -519,7 +525,12 @@ describe("attemptCompletionTool", () => { toolCallId: "call-attempt-completion", } - await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + const handlingCompletion = attemptCompletionTool.handle(mockTask as Task, block, callbacks) + await vi.waitFor(() => expect(mockTask.waitForCurrentAssistantMessagePersistence).toHaveBeenCalled()) + expect(mockProvider.reopenParentFromDelegation).not.toHaveBeenCalled() + + markPersistenceReady() + await handlingCompletion expect(mockAskFinishSubTaskApproval).toHaveBeenCalled() expect(mockProvider.setPendingTaskAction).toHaveBeenCalledWith("child-1", { @@ -539,6 +550,56 @@ describe("attemptCompletionTool", () => { expect(mockPushToolResult).toHaveBeenCalledWith("") }) + it("does not delegate or emit completion when child history persistence fails", async () => { + const persistenceError = new Error("history unavailable") + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "9" }, + nativeArgs: { result: "9" }, + partial: false, + } + const mockProvider = { + log: vi.fn(), + getTaskWithId: vi.fn().mockImplementation((id: string) => + Promise.resolve({ + historyItem: + id === "child-1" + ? { id, status: "active" } + : { id, status: "active", awaitingChildId: "child-1" }, + }), + ), + setPendingTaskAction: vi.fn().mockResolvedValue(undefined), + reopenParentFromDelegation: vi.fn().mockResolvedValue(true), + } + + Object.assign(mockTask, { + taskId: "child-1", + parentTaskId: "parent-1", + providerRef: { deref: () => mockProvider }, + waitForCurrentAssistantMessagePersistence: vi.fn().mockRejectedValue(persistenceError), + }) + + await attemptCompletionTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + toolCallId: "call-attempt-completion", + }) + + expect(mockHandleError).toHaveBeenCalledWith("persisting task completion", persistenceError) + expect(mockAskFinishSubTaskApproval).not.toHaveBeenCalled() + expect(mockProvider.reopenParentFromDelegation).not.toHaveBeenCalled() + expect(mockTask.emit).not.toHaveBeenCalledWith( + RooCodeEventName.TaskCompleted, + expect.anything(), + expect.anything(), + expect.anything(), + ) + }) + it("falls through to standalone completion when parent delegation becomes stale after approval", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", @@ -772,6 +833,10 @@ describe("attemptCompletionTool", () => { expect(mockHandleError).not.toHaveBeenCalled() expect(mockTask.flushTelemetryInstallment).toHaveBeenCalledTimes(1) expect(mockTask.flushTelemetryInstallment).toHaveBeenCalledWith("attempt_completion") + expect(mockTask.waitForCurrentAssistantMessagePersistence).toHaveBeenCalledTimes(1) + expect( + vi.mocked(mockTask.waitForCurrentAssistantMessagePersistence!).mock.invocationCallOrder[0], + ).toBeLessThan(vi.mocked(mockTask.emit!).mock.invocationCallOrder[0]) expect(mockTask.emit).toHaveBeenCalledWith( RooCodeEventName.TaskCompleted, "task_1", @@ -970,6 +1035,7 @@ describe("attemptCompletionTool telemetry invariants", () => { messageCounts: { user: 0, assistant: 0 }, taskId: "task_1", flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), ...overrides, } } diff --git a/src/extension/__tests__/api-task-conversation-history-length.spec.ts b/src/extension/__tests__/api-task-conversation-history-length.spec.ts index 4cfd9bbe4b..7018f59880 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -42,4 +42,65 @@ describe("API#getTaskApiConversationHistoryLength", () => { await expect(api.getTaskApiConversationHistoryLength("missing-task")).resolves.toBe(0) }) + + it("finds the expected persisted user and assistant turns in order", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [ + { type: "text", text: "Finished" }, + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "done" } }, + ], + }, + ], + }) + + await expect( + api.hasTaskApiConversationHistorySequence("task-1", { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + }), + ).resolves.toBe(true) + }) + + it("returns false when the expected persisted turns are unavailable", async () => { + mockGetTaskWithId.mockRejectedValue(new Error("Task not found")) + + await expect( + api.hasTaskApiConversationHistorySequence("missing-task", { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + }), + ).resolves.toBe(false) + }) + + it("rejects an assistant completion that does not follow the expected user turn", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { + role: "assistant", + content: [{ type: "tool_use", id: "early", name: "attempt_completion", input: { result: "done" } }], + }, + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [ + { type: "tool_use", id: "other", name: "attempt_completion", input: { result: "other" } }, + ], + }, + ], + }) + + await expect( + api.hasTaskApiConversationHistorySequence("task-1", { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + }), + ).resolves.toBe(false) + }) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index 74ea2e7680..7a173cea1c 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -14,6 +14,7 @@ import { type ProviderSettingsEntry, type TaskEvent, type CreateTaskOptions, + type TaskApiConversationHistorySequence, type WebviewThemeFixture, RooCodeEventName, TaskCommandName, @@ -251,6 +252,39 @@ export class API extends EventEmitter implements RooCodeAPI { } } + /** Checks persisted turn ordering without exposing conversation contents to tests. */ + public async hasTaskApiConversationHistorySequence( + taskId: string, + sequence: TaskApiConversationHistorySequence, + ): Promise { + try { + const { apiConversationHistory } = await this.sidebarProvider.getTaskWithId(taskId) + const userTurnIndex = apiConversationHistory.findIndex( + (message) => + message.role === "user" && + Array.isArray(message.content) && + message.content.some((block) => block.type === "text" && block.text.includes(sequence.userText)), + ) + if (userTurnIndex < 0) return false + + return apiConversationHistory + .slice(userTurnIndex + 1) + .some( + (message) => + message.role === "assistant" && + Array.isArray(message.content) && + message.content.some( + (block) => + block.type === "tool_use" && + block.name === sequence.assistantToolName && + JSON.stringify(block.input).includes(sequence.assistantToolInputText), + ), + ) + } catch { + return false + } + } + public getCurrentTaskStack() { return this.sidebarProvider.getCurrentTaskStack() }