From 0be15755651a91bfac420f430b1e0a88553ce8c5 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 14:16:31 +0000 Subject: [PATCH 01/22] test(formal): model completion persistence ordering --- .github/alloy/CompletionPersistence.als | 141 ++++++++++++++++++++++++ .github/alloy/README.md | 43 ++++++++ 2 files changed, 184 insertions(+) create mode 100644 .github/alloy/CompletionPersistence.als create mode 100644 .github/alloy/README.md 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..71219eab4e --- /dev/null +++ b/.github/alloy/README.md @@ -0,0 +1,43 @@ +# 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. + +## 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 an implementation contract where 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. From a53635b0d2be8e76b3ac2b953dab993a620ba8e3 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 15:12:25 +0000 Subject: [PATCH 02/22] test(task): reproduce completion persistence race --- .github/alloy/README.md | 13 +++ .../task/__tests__/Task.persistence.spec.ts | 97 ++++++++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/.github/alloy/README.md b/.github/alloy/README.md index 71219eab4e..6c505108b7 100644 --- a/.github/alloy/README.md +++ b/.github/alloy/README.md @@ -16,6 +16,19 @@ The current-policy assertions search for a hypothesized, contract-permitted bad 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 characterization + +`src/core/task/__tests__/Task.persistence.spec.ts` blocks the real `saveApiMessages` boundary on a deferred promise, accepts completion on the same `Task`, and confirms that `TaskCompleted` is emitted while the write remains unresolved. This establishes the production contract gap represented by `CurrentPolicy` without relying on the intermittent extension-host timing. + +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 and observing `TaskCompleted` are `acceptCompletion` and `emitCompletion`; +- resolving and awaiting the deferred in `finally` is `finishHistoryWrite`. + +The characterization does not prove that the failed CI run followed the same concrete stream interleaving. It intentionally records the current unsafe behavior; a production fix should invert the ordering assertion so completion stays pending until persistence succeeds. + ## Code mapping - `startHistoryWrite` and `finishHistoryWrite` represent `Task.saveApiConversationHistory()` entering and completing its durable file write. diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 671bd7d4b7..8680fa3cb4 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,91 @@ describe("Task persistence", () => { // But the content should be the same expect(callArgs.messages).toEqual(task.apiConversationHistory) }) + + it("reproduces TaskCompleted emission while API history persistence is blocked", async () => { + // Characterizes the unsafe contract tracked by #1453. A production fix should + // invert this ordering so completion remains pending until the write resolves. + 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 + + const saving = privateTask.addToApiConversationHistory({ + role: "assistant", + content: [ + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + input: { result: "done" }, + }, + ], + }) + void saving.finally(() => { + saveSettled = true + }) + + try { + 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", + }), + ]), + }), + ]) + + 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, + } + + await attemptCompletionTool.handle(task, block, callbacks) + + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(completionEmitted).toBe(true) + expect(saveSettled).toBe(false) + } finally { + saveDeferred.resolve(undefined) + await saving + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── From 1116ea1c585e6634b9ddbfe0d099546653708325 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 18:27:23 +0000 Subject: [PATCH 03/22] fix(task): persist history before completion --- .github/alloy/README.md | 13 +- .../src/suite/restart-persistence.test.ts | 7 +- packages/types/src/events.ts | 1 + .../history-resume-delegation.spec.ts | 1 + .../nested-delegation-resume.spec.ts | 2 + src/core/task/Task.ts | 43 ++++- .../task/__tests__/Task.persistence.spec.ts | 147 +++++++++++++----- src/core/tools/AttemptCompletionTool.ts | 19 ++- .../__tests__/attemptCompletionTool.spec.ts | 18 ++- 9 files changed, 194 insertions(+), 57 deletions(-) diff --git a/.github/alloy/README.md b/.github/alloy/README.md index 6c505108b7..cdf184bfb5 100644 --- a/.github/alloy/README.md +++ b/.github/alloy/README.md @@ -16,25 +16,26 @@ The current-policy assertions search for a hypothesized, contract-permitted bad 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 characterization +## Deterministic production regression -`src/core/task/__tests__/Task.persistence.spec.ts` blocks the real `saveApiMessages` boundary on a deferred promise, accepts completion on the same `Task`, and confirms that `TaskCompleted` is emitted while the write remains unresolved. This establishes the production contract gap represented by `CurrentPolicy` without relying on the intermittent extension-host timing. +`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 and observing `TaskCompleted` are `acceptCompletion` and `emitCompletion`; -- resolving and awaiting the deferred in `finally` is `finishHistoryWrite`. +- accepting the matching completion call is `acceptCompletion`; +- resolving the deferred is `finishHistoryWrite`; +- observing `TaskCompleted` afterward is `emitCompletion`. -The characterization does not prove that the failed CI run followed the same concrete stream interleaving. It intentionally records the current unsafe behavior; a production fix should invert the ordering assertion so completion stays pending until persistence succeeds. +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 an implementation contract where the public completion boundary is not crossed until the required API history write succeeds. +- `DurableFirstPolicy` represents the production contract: the public completion boundary is not crossed until the required API history write succeeds. ## Run Alloy 6 diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 29e7fa3ddd..f299f9023a 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, @@ -85,7 +80,7 @@ async function runVerify(api: RooCodeAPI): Promise { 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") + assert.ok(conversationLength > 0, "Completion should make API conversation history available to a fresh host") await writePhaseResult(getResultsDir(), { version: PHASE_RESULT_VERSION, 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 d3a24a3140..e0a2d1a049 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1380,6 +1380,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..e4fd5fc754 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,7 @@ export class Task extends EventEmitter implements TaskLike { return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } - private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string) { + private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string): Promise { const resolvesPendingAction = this.pendingAction && message.role === "user" && @@ -1011,6 +1015,39 @@ export class Task extends EventEmitter implements TaskLike { ) } } + if (message.role === "assistant") { + this.assistantMessageSavedToHistory = saved + this.resolveAssistantMessagePersistence(saved) + } + } + + 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 +3028,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 +3838,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 8680fa3cb4..2a39322a7f 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -394,9 +394,7 @@ describe("Task persistence", () => { expect(callArgs.messages).toEqual(task.apiConversationHistory) }) - it("reproduces TaskCompleted emission while API history persistence is blocked", async () => { - // Characterizes the unsafe contract tracked by #1453. A production fix should - // invert this ordering so completion remains pending until the write resolves. + it("emits TaskCompleted only after API history persistence succeeds", async () => { const saveDeferred = createDeferred() mockSaveApiMessages.mockReturnValueOnce(saveDeferred.promise) @@ -410,39 +408,9 @@ describe("Task persistence", () => { const completionCallId = "completion-call" let saveSettled = false let completionEmitted = false - - const saving = privateTask.addToApiConversationHistory({ - role: "assistant", - content: [ - { - type: "tool_use", - id: completionCallId, - name: "attempt_completion", - input: { result: "done" }, - }, - ], - }) - void saving.finally(() => { - saveSettled = true - }) + let saving: Promise | undefined try { - 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", - }), - ]), - }), - ]) - vi.spyOn(task, "say").mockResolvedValue(undefined) vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) @@ -468,16 +436,123 @@ describe("Task persistence", () => { toolCallId: completionCallId, } - await attemptCompletionTool.handle(task, block, callbacks) + const handlingCompletion = attemptCompletionTool.handle(task, block, callbacks) + await vi.waitFor(() => expect(task.ask).toHaveBeenCalled()) expect(callbacks.handleError).not.toHaveBeenCalled() - expect(completionEmitted).toBe(true) + 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) + + 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..f1f212cac8 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", { @@ -772,6 +783,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 +985,7 @@ describe("attemptCompletionTool telemetry invariants", () => { messageCounts: { user: 0, assistant: 0 }, taskId: "task_1", flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), ...overrides, } } From ea0f8440895e82c3c5e9ac4ac6c0e63d2f7d3402 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 18:32:59 +0000 Subject: [PATCH 04/22] test(e2e): require restored completion turn --- apps/vscode-e2e/src/suite/restart-persistence.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index f299f9023a..90c97b0f57 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -80,7 +80,10 @@ async function runVerify(api: RooCodeAPI): Promise { 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, "Completion should make API conversation history available to a fresh host") + assert.ok( + conversationLength >= 2, + "Completion should make the user and assistant API conversation turns available to a fresh host", + ) await writePhaseResult(getResultsDir(), { version: PHASE_RESULT_VERSION, From 9f1a54878b5f02ba8fb5369849393a83ec8a80a9 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 03:03:16 +0000 Subject: [PATCH 05/22] test(api): strengthen completion persistence checks --- .../src/suite/restart-persistence.test.ts | 15 +++-- packages/types/src/api.ts | 16 +++++ src/core/task/Task.ts | 2 + .../task/__tests__/Task.persistence.spec.ts | 1 + .../__tests__/attemptCompletionTool.spec.ts | 50 +++++++++++++++ ...i-task-conversation-history-length.spec.ts | 61 +++++++++++++++++++ src/extension/api.ts | 34 +++++++++++ 7 files changed, 174 insertions(+), 5 deletions(-) diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 90c97b0f57..dfd55af923 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -79,17 +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 >= 2, - "Completion should make the user and assistant API conversation turns available to a fresh host", + 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/src/core/task/Task.ts b/src/core/task/Task.ts index e4fd5fc754..9c79228934 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -983,6 +983,7 @@ export class Task extends EventEmitter implements TaskLike { return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } + /** 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 && @@ -1021,6 +1022,7 @@ export class Task extends EventEmitter implements TaskLike { } } + /** Creates the persistence boundary for the next streamed assistant turn. */ private resetAssistantMessagePersistence(): void { this.assistantMessagePersistencePromise = new Promise((resolve) => { this.resolveAssistantMessagePersistence = resolve diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 2a39322a7f..8b09fbc5d3 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -473,6 +473,7 @@ describe("Task persistence", () => { }), ]) expect(saveSettled).toBe(false) + expect(completionEmitted).toBe(false) saveDeferred.resolve(undefined) await Promise.all([saving, handlingCompletion]) diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index f1f212cac8..232e8092d3 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -550,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", 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() } From 7886d637735de6ce8a257127401ccc18f07e0516 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 21:32:31 +0000 Subject: [PATCH 06/22] fix(task): cancel pending persistence waits --- .../history-resume-delegation.spec.ts | 2 +- .../nested-delegation-resume.spec.ts | 4 +- src/core/task/Task.ts | 50 +++++++++++++---- .../task/__tests__/Task.persistence.spec.ts | 56 +++++++++++++++++++ src/core/tools/AttemptCompletionTool.ts | 6 +- .../__tests__/attemptCompletionTool.spec.ts | 36 ++++++++++-- 6 files changed, 133 insertions(+), 21 deletions(-) diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index e0a2d1a049..97bb6f8594 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1380,7 +1380,7 @@ describe("History resume delegation - parent metadata transitions", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), - waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(true), } 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 a07411f782..dd015e93cf 100644 --- a/src/__tests__/nested-delegation-resume.spec.ts +++ b/src/__tests__/nested-delegation-resume.spec.ts @@ -204,7 +204,7 @@ describe("Nested delegation resume (A → B → C)", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), - waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(true), } as unknown as Task const blockC = { @@ -253,7 +253,7 @@ describe("Nested delegation resume (A → B → C)", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), - waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(true), } as unknown as Task const blockB = { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 9c79228934..e4d8cf037e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -194,6 +194,8 @@ export interface TaskOptions extends CreateTaskOptions { diffFuzzyThreshold?: number } +type AssistantMessagePersistenceResult = "saved" | "failed" | "cancelled" + export class Task extends EventEmitter implements TaskLike { readonly taskId: string readonly rootTaskId?: string @@ -405,9 +407,11 @@ export class Task extends EventEmitter implements TaskLike { * Set to `true` only after the assistant message is durably saved. */ assistantMessageSavedToHistory = false - private assistantMessagePersistencePromise!: Promise - private resolveAssistantMessagePersistence!: (saved: boolean) => void - private completionPersistenceReadyPromise?: Promise + private assistantMessagePersistencePromise!: Promise + private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void + private assistantMessagePersistenceCancellationPromise!: Promise + private resolveAssistantMessagePersistenceCancellation!: () => void + private completionPersistenceReadyPromise?: Promise /** * Fire-and-forget wrapper around `presentAssistantMessage` that swallows the @@ -1018,34 +1022,54 @@ export class Task extends EventEmitter implements TaskLike { } if (message.role === "assistant") { this.assistantMessageSavedToHistory = saved - this.resolveAssistantMessagePersistence(saved) + this.resolveAssistantMessagePersistence(saved ? "saved" : "failed") } } - /** Creates the persistence boundary for the next streamed assistant turn. */ + /** Cancels the current persistence generation before creating the next assistant-turn boundary. */ private resetAssistantMessagePersistence(): void { - this.assistantMessagePersistencePromise = new Promise((resolve) => { + this.cancelAssistantMessagePersistence() + this.assistantMessagePersistencePromise = new Promise((resolve) => { this.resolveAssistantMessagePersistence = resolve }) + this.assistantMessagePersistenceCancellationPromise = new Promise((resolve) => { + this.resolveAssistantMessagePersistenceCancellation = resolve + }) this.completionPersistenceReadyPromise = undefined } + /** Settles persistence waiters when the task or current stream generation ends. */ + private cancelAssistantMessagePersistence(): void { + this.resolveAssistantMessagePersistence?.("cancelled") + this.resolveAssistantMessagePersistenceCancellation?.() + } + /** * 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 { + public waitForCurrentAssistantMessagePersistence(): Promise { if (!this.completionPersistenceReadyPromise) { const currentPersistence = this.assistantMessagePersistencePromise + const currentCancellation = this.assistantMessagePersistenceCancellationPromise this.completionPersistenceReadyPromise = (async () => { - const saved = await currentPersistence - if (saved) return - - const retrySucceeded = await this.retrySaveApiConversationHistory() - if (!retrySucceeded) { + const result = await Promise.race([ + currentPersistence, + currentCancellation.then(() => "cancelled" as const), + ]) + if (result === "cancelled") return false + if (result === "saved") return true + + const retryResult = await Promise.race([ + this.retrySaveApiConversationHistory().then((saved) => ({ status: "complete" as const, saved })), + currentCancellation.then(() => ({ status: "cancelled" as const })), + ]) + if (retryResult.status === "cancelled") return false + if (!retryResult.saved) { throw new Error("Failed to persist API conversation history before task completion") } this.assistantMessageSavedToHistory = true + return true })() } @@ -2512,6 +2536,7 @@ export class Task extends EventEmitter implements TaskLike { } this.abort = true + this.cancelAssistantMessagePersistence() // Reset consecutive error counters on abort (manual intervention) this.consecutiveNoToolUseCount = 0 @@ -2555,6 +2580,7 @@ export class Task extends EventEmitter implements TaskLike { public dispose(): void { console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) + this.cancelAssistantMessagePersistence() // Stop the idle telemetry check and report any unflushed activity as a // shutdown installment, so a task torn down mid-work (panel closed, task diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 8b09fbc5d3..7ab6a64502 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -23,6 +23,7 @@ import type { AttemptCompletionToolUse } from "../../../shared/tools" type TaskPersistenceAccess = { addToApiConversationHistory: (message: Anthropic.MessageParam) => Promise + resetAssistantMessagePersistence: () => void resumeTaskFromHistory: () => Promise resumePendingTaskAction: (action: PendingTaskAction) => Promise saveClineMessages: () => Promise @@ -554,6 +555,61 @@ describe("Task persistence", () => { vi.useRealTimers() } }) + + it("settles a pending assistant persistence wait when the task is disposed", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const waiting = task.waitForCurrentAssistantMessagePersistence() + task.dispose() + + await expect(waiting).resolves.toBe(false) + }) + + it("cancels a persistence wait while failed history is awaiting retry", async () => { + vi.useFakeTimers() + mockSaveApiMessages.mockRejectedValueOnce(new Error("write failed")) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + await getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "assistant", + content: [{ type: "text", text: "completion" }], + }) + const waiting = task.waitForCurrentAssistantMessagePersistence() + + task.dispose() + + await expect(waiting).resolves.toBe(false) + } finally { + vi.clearAllTimers() + vi.useRealTimers() + } + }) + + it("settles the previous persistence generation when a new request resets the barrier", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const waiting = task.waitForCurrentAssistantMessagePersistence() + + getTaskPersistenceAccess(task).resetAssistantMessagePersistence() + + await expect(waiting).resolves.toBe(false) + task.dispose() + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index d6f9a320ca..f9252d054f 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -143,7 +143,8 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { hasFlushedTelemetry = true try { - await task.waitForCurrentAssistantMessagePersistence() + const persistenceReady = await task.waitForCurrentAssistantMessagePersistence() + if (!persistenceReady) return } catch (error) { await handleError("persisting task completion", error as Error) return @@ -301,7 +302,8 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { * PostHog telemetry flush, which reports on every model-initiated attempt_completion call. */ private async emitPublicTaskCompleted(task: Task): Promise { - await task.waitForCurrentAssistantMessagePersistence() + const persistenceReady = await task.waitForCurrentAssistantMessagePersistence() + if (!persistenceReady) return // Force final token usage update before emitting TaskCompleted. // This ensures the latest stats are captured regardless of throttle timer. diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 232e8092d3..4dc00a9321 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -76,7 +76,7 @@ describe("attemptCompletionTool", () => { flushTelemetryInstallment: vi.fn(), setPendingTaskAction: vi.fn(), persistQueuedFeedbackAndAcknowledge: vi.fn().mockResolvedValue(true), - waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(true), } }) @@ -480,8 +480,8 @@ 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 persistenceReady = new Promise((resolve) => { + markPersistenceReady = () => resolve(true) }) const block: AttemptCompletionToolUse = { type: "tool_use", @@ -845,6 +845,34 @@ describe("attemptCompletionTool", () => { ) }) + it("does not emit TaskCompleted when persistence is cancelled", async () => { + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "2" }, + nativeArgs: { result: "2" }, + partial: false, + } + mockTask.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + mockTask.waitForCurrentAssistantMessagePersistence = vi.fn().mockResolvedValue(false) + + await attemptCompletionTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + }) + + expect(mockHandleError).not.toHaveBeenCalled() + expect(mockTask.emit).not.toHaveBeenCalledWith( + RooCodeEventName.TaskCompleted, + expect.anything(), + expect.anything(), + expect.anything(), + ) + }) + it("reports telemetry but does not emit the public TaskCompleted event when user provides follow-up feedback", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", @@ -1035,7 +1063,7 @@ describe("attemptCompletionTool telemetry invariants", () => { messageCounts: { user: 0, assistant: 0 }, taskId: "task_1", flushTelemetryInstallment: vi.fn(), - waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(undefined), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(true), ...overrides, } } From 8161f933c4eb8e24dcc28800ba233623cb613e0c Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 21:47:30 +0000 Subject: [PATCH 07/22] fix(task): stop persistence retries on cancel --- src/core/task/Task.ts | 69 ++++++++++++++----- .../task/__tests__/Task.persistence.spec.ts | 3 +- .../__tests__/attemptCompletionTool.spec.ts | 49 +++++++++++++ 3 files changed, 103 insertions(+), 18 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index e4d8cf037e..2532885951 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -195,6 +195,11 @@ export interface TaskOptions extends CreateTaskOptions { } type AssistantMessagePersistenceResult = "saved" | "failed" | "cancelled" +type AssistantMessagePersistenceCancellation = { + cancelled: boolean + promise: Promise + resolve: () => void +} export class Task extends EventEmitter implements TaskLike { readonly taskId: string @@ -409,8 +414,7 @@ export class Task extends EventEmitter implements TaskLike { assistantMessageSavedToHistory = false private assistantMessagePersistencePromise!: Promise private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void - private assistantMessagePersistenceCancellationPromise!: Promise - private resolveAssistantMessagePersistenceCancellation!: () => void + private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation private completionPersistenceReadyPromise?: Promise /** @@ -1032,16 +1036,26 @@ export class Task extends EventEmitter implements TaskLike { this.assistantMessagePersistencePromise = new Promise((resolve) => { this.resolveAssistantMessagePersistence = resolve }) - this.assistantMessagePersistenceCancellationPromise = new Promise((resolve) => { - this.resolveAssistantMessagePersistenceCancellation = resolve - }) + let resolveCancellation!: () => void + const cancellation: AssistantMessagePersistenceCancellation = { + cancelled: false, + promise: new Promise((resolve) => { + resolveCancellation = resolve + }), + resolve: () => { + if (cancellation.cancelled) return + cancellation.cancelled = true + resolveCancellation() + }, + } + this.assistantMessagePersistenceCancellation = cancellation this.completionPersistenceReadyPromise = undefined } /** Settles persistence waiters when the task or current stream generation ends. */ private cancelAssistantMessagePersistence(): void { this.resolveAssistantMessagePersistence?.("cancelled") - this.resolveAssistantMessagePersistenceCancellation?.() + this.assistantMessagePersistenceCancellation?.resolve() } /** @@ -1051,21 +1065,18 @@ export class Task extends EventEmitter implements TaskLike { public waitForCurrentAssistantMessagePersistence(): Promise { if (!this.completionPersistenceReadyPromise) { const currentPersistence = this.assistantMessagePersistencePromise - const currentCancellation = this.assistantMessagePersistenceCancellationPromise + const currentCancellation = this.assistantMessagePersistenceCancellation! this.completionPersistenceReadyPromise = (async () => { const result = await Promise.race([ currentPersistence, - currentCancellation.then(() => "cancelled" as const), + currentCancellation.promise.then(() => "cancelled" as const), ]) if (result === "cancelled") return false if (result === "saved") return true - const retryResult = await Promise.race([ - this.retrySaveApiConversationHistory().then((saved) => ({ status: "complete" as const, saved })), - currentCancellation.then(() => ({ status: "cancelled" as const })), - ]) - if (retryResult.status === "cancelled") return false - if (!retryResult.saved) { + const retryResult = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) + if (retryResult === "cancelled") return false + if (retryResult === "failed") { throw new Error("Failed to persist API conversation history before task completion") } this.assistantMessageSavedToHistory = true @@ -1184,22 +1195,46 @@ export class Task extends EventEmitter implements TaskLike { * Used by delegation flow when flushPendingToolResultsToHistory reports failure. */ public async retrySaveApiConversationHistory(): Promise { + return (await this.retrySaveApiConversationHistoryWithCancellation()) === "saved" + } + + private async retrySaveApiConversationHistoryWithCancellation( + cancellation?: AssistantMessagePersistenceCancellation, + ): Promise { const delays = [100, 500, 1500] for (let attempt = 0; attempt < delays.length; attempt++) { - await new Promise((resolve) => setTimeout(resolve, delays[attempt])) + if (cancellation) { + const delayCompleted = await new Promise((resolve) => { + let settled = false + const finish = (completed: boolean) => { + if (settled) return + settled = true + resolve(completed) + } + const timer = setTimeout(() => finish(true), delays[attempt]) + void cancellation.promise.then(() => { + clearTimeout(timer) + finish(false) + }) + }) + if (!delayCompleted) return "cancelled" + } else { + await new Promise((resolve) => setTimeout(resolve, delays[attempt])) + } console.warn( `[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`, ) const success = await this.saveApiConversationHistory() + if (cancellation?.cancelled) return "cancelled" if (success) { - return true + return "saved" } } - return false + return "failed" } // Cline Messages diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 7ab6a64502..862acaac30 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -590,8 +590,9 @@ describe("Task persistence", () => { task.dispose() await expect(waiting).resolves.toBe(false) + await vi.runAllTimersAsync() + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) } finally { - vi.clearAllTimers() vi.useRealTimers() } }) diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 4dc00a9321..fb73c08390 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -600,6 +600,55 @@ describe("attemptCompletionTool", () => { ) }) + it("does not delegate or report an error when child history persistence is cancelled", async () => { + 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().mockResolvedValue(false), + }) + + await attemptCompletionTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + toolCallId: "call-attempt-completion", + }) + + expect(mockHandleError).not.toHaveBeenCalled() + 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", From 6d0734fad633bf8e1c07b4677c98b5dd4c581a01 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 22:16:20 +0000 Subject: [PATCH 08/22] test(task): cover completion retry recovery --- src/core/task/Task.ts | 1 + .../task/__tests__/Task.persistence.spec.ts | 66 +++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 2532885951..24b86a449b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1198,6 +1198,7 @@ export class Task extends EventEmitter implements TaskLike { return (await this.retrySaveApiConversationHistoryWithCancellation()) === "saved" } + /** Retries API-history persistence while allowing the active assistant generation to cancel backoff. */ private async retrySaveApiConversationHistoryWithCancellation( cancellation?: AssistantMessagePersistenceCancellation, ): Promise { diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 862acaac30..368e055e2c 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -556,6 +556,72 @@ describe("Task persistence", () => { } }) + it("emits TaskCompleted after a failed assistant save succeeds on retry", async () => { + vi.useFakeTimers() + mockSaveApiMessages + .mockRejectedValueOnce(new Error("initial write failed")) + .mockResolvedValueOnce(undefined) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const completionCallId = "retried-completion-call" + 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 getTaskPersistenceAccess(task).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, + ) + expect(completionListener).not.toHaveBeenCalled() + + await vi.runAllTimersAsync() + await handlingCompletion + + expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(completionListener).toHaveBeenCalledTimes(1) + } finally { + mockSaveApiMessages.mockResolvedValue(undefined) + vi.useRealTimers() + } + }) + it("settles a pending assistant persistence wait when the task is disposed", async () => { const task = new Task({ provider: mockProvider, From e668eb84a766143882dd54dc114d0c8c7f912dc0 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 23:59:45 +0000 Subject: [PATCH 09/22] fix: apply CodeRabbit auto-fixes Fixed 3 file(s) based on 3 failed pre-merge checks. Co-authored-by: CodeRabbit --- src/core/task/Task.ts | 45 ++++++++++++++++++- .../task/__tests__/Task.persistence.spec.ts | 36 +++++++++++++++ src/core/tools/AttemptCompletionTool.ts | 4 ++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 24b86a449b..f2aa13d3b6 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -416,6 +416,7 @@ export class Task extends EventEmitter implements TaskLike { private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation private completionPersistenceReadyPromise?: Promise + private assistantMessageRetryTimeoutHandle?: NodeJS.Timeout /** * Fire-and-forget wrapper around `presentAssistantMessage` that swallows the @@ -933,6 +934,10 @@ export class Task extends EventEmitter implements TaskLike { return false } + /** + * Clears the pending action metadata after its durable result is saved. + * Reconciles in-memory state with the task history store to avoid clearing a newer action. + */ private async clearPendingActionAfterDurableResult(actionId: string): Promise { if (this.pendingAction?.actionId !== actionId) { return @@ -958,6 +963,10 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Processes a queued ask response and determines if a durable acknowledgment is needed. + * Returns the message ID if persistence is required, otherwise removes the message and returns undefined. + */ private handleQueuedAskResponse(message: QueuedMessage, resolution: QueuedAskResolution): string | undefined { this.handleWebviewAskResponse(resolution.response, message.text, message.images) if (resolution.requiresDurableAck) { @@ -991,7 +1000,10 @@ export class Task extends EventEmitter implements TaskLike { return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } - /** Appends an API turn and records whether an assistant turn reached persistent storage. */ + /** + * Appends an API turn and records whether an assistant turn reached persistent storage. + * If the message resolves a pending action, retries the save on initial failure before clearing the action. + */ private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string): Promise { const resolvesPendingAction = this.pendingAction && @@ -1054,6 +1066,10 @@ export class Task extends EventEmitter implements TaskLike { /** Settles persistence waiters when the task or current stream generation ends. */ private cancelAssistantMessagePersistence(): void { + if (this.assistantMessageRetryTimeoutHandle !== undefined) { + clearTimeout(this.assistantMessageRetryTimeoutHandle) + this.assistantMessageRetryTimeoutHandle = undefined + } this.resolveAssistantMessagePersistence?.("cancelled") this.assistantMessagePersistenceCancellation?.resolve() } @@ -1091,6 +1107,7 @@ export class Task extends EventEmitter implements TaskLike { // For API requests, consecutive same-role messages are merged via mergeConsecutiveApiMessages() // so rewind/edit behavior can still reference original message boundaries. + /** Replaces the entire API conversation history and persists the new state. */ async overwriteApiConversationHistory(newHistory: ApiMessage[]) { this.apiConversationHistory = newHistory await this.saveApiConversationHistory() @@ -1175,6 +1192,7 @@ export class Task extends EventEmitter implements TaskLike { return saved } + /** Persists the current API conversation history to disk, returning false on I/O errors. */ private async saveApiConversationHistory(): Promise { try { await saveApiMessages({ @@ -1205,15 +1223,22 @@ export class Task extends EventEmitter implements TaskLike { const delays = [100, 500, 1500] for (let attempt = 0; attempt < delays.length; attempt++) { + // Check cancellation before each retry delay + if (cancellation?.cancelled) return "cancelled" + if (cancellation) { const delayCompleted = await new Promise((resolve) => { let settled = false const finish = (completed: boolean) => { if (settled) return settled = true + if (this.assistantMessageRetryTimeoutHandle !== undefined) { + this.assistantMessageRetryTimeoutHandle = undefined + } resolve(completed) } const timer = setTimeout(() => finish(true), delays[attempt]) + this.assistantMessageRetryTimeoutHandle = timer void cancellation.promise.then(() => { clearTimeout(timer) finish(false) @@ -1223,6 +1248,10 @@ export class Task extends EventEmitter implements TaskLike { } else { await new Promise((resolve) => setTimeout(resolve, delays[attempt])) } + + // Check cancellation before each save attempt + if (cancellation?.cancelled) return "cancelled" + console.warn( `[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`, ) @@ -1240,10 +1269,15 @@ export class Task extends EventEmitter implements TaskLike { // Cline Messages + /** Reads the persisted Cline messages from disk for this task. */ private async getSavedClineMessages(): Promise { return readTaskMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } + /** + * Appends a new Cline message, posts it to the webview, emits an event, and persists. + * Partial messages and unanswered asks are flushed immediately to the webview. + */ private async addToClineMessages(message: ClineMessage) { this.clineMessages.push(message) const provider = this.providerRef.deref() @@ -1277,6 +1311,10 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Replaces the entire Cline message history, restores todo state, and persists. + * Also resets cloud sync tracking to avoid re-syncing previously synced messages. + */ public async overwriteClineMessages(newMessages: ClineMessage[]) { this.clineMessages = newMessages restoreTodoListForTask(this) @@ -1292,6 +1330,10 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Updates a Cline message in the webview and emits an event. + * Non-partial messages are synced to cloud telemetry if not already synced. + */ private async updateClineMessage(message: ClineMessage) { const provider = this.providerRef.deref() await provider?.postMessageToWebview({ type: "messageUpdated", clineMessage: message }) @@ -1311,6 +1353,7 @@ export class Task extends EventEmitter implements TaskLike { } } + /** Persists Cline messages and updates task metadata in the history store. Returns false on failure. */ private async saveClineMessages(): Promise { try { await saveTaskMessages({ diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 368e055e2c..12f7ef212b 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -616,6 +616,10 @@ describe("Task persistence", () => { expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) expect(callbacks.handleError).not.toHaveBeenCalled() expect(completionListener).toHaveBeenCalledTimes(1) + // Assert ordering: retry save completes before TaskCompleted is emitted + expect(vi.mocked(mockSaveApiMessages).mock.invocationCallOrder[1]).toBeLessThan( + vi.mocked(completionListener).mock.invocationCallOrder[0], + ) } finally { mockSaveApiMessages.mockResolvedValue(undefined) vi.useRealTimers() @@ -677,6 +681,38 @@ describe("Task persistence", () => { await expect(waiting).resolves.toBe(false) task.dispose() }) + + it("stops retry attempts immediately when cancelled during delay", async () => { + vi.useFakeTimers() + mockSaveApiMessages.mockRejectedValueOnce(new Error("initial write failed")).mockResolvedValue(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + await getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "assistant", + content: [{ type: "text", text: "message" }], + }) + const waiting = task.waitForCurrentAssistantMessagePersistence() + + // Cancel during the first retry delay (100ms) + await vi.advanceTimersByTimeAsync(50) + task.dispose() + + await expect(waiting).resolves.toBe(false) + await vi.runAllTimersAsync() + + // Should only have attempted the initial save, no retry saves + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + } finally { + vi.useRealTimers() + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index f9252d054f..6c3d9bd3e3 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -277,6 +277,10 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { return "delegated" } + /** + * Handles streaming partial blocks for attempt_completion, updating the completion result + * or command ask as the model streams its response. + */ override async handlePartial(task: Task, block: ToolUse<"attempt_completion">): Promise { const result: string | undefined = block.params.result const command: string | undefined = block.params.command From 1bb5edcaf48afa0b41967257b89f39cbd2b48040 Mon Sep 17 00:00:00 2001 From: Roomote Date: Tue, 1 Sep 2026 00:43:02 +0000 Subject: [PATCH 10/22] fix(task): keep persistence retry timers generation-local --- src/core/task/Task.ts | 25 ------------------- .../task/__tests__/Task.persistence.spec.ts | 7 +++--- src/core/tools/AttemptCompletionTool.ts | 4 --- 3 files changed, 3 insertions(+), 33 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index f2aa13d3b6..5d2d47ca0c 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -416,7 +416,6 @@ export class Task extends EventEmitter implements TaskLike { private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation private completionPersistenceReadyPromise?: Promise - private assistantMessageRetryTimeoutHandle?: NodeJS.Timeout /** * Fire-and-forget wrapper around `presentAssistantMessage` that swallows the @@ -963,10 +962,6 @@ export class Task extends EventEmitter implements TaskLike { } } - /** - * Processes a queued ask response and determines if a durable acknowledgment is needed. - * Returns the message ID if persistence is required, otherwise removes the message and returns undefined. - */ private handleQueuedAskResponse(message: QueuedMessage, resolution: QueuedAskResolution): string | undefined { this.handleWebviewAskResponse(resolution.response, message.text, message.images) if (resolution.requiresDurableAck) { @@ -1066,10 +1061,6 @@ export class Task extends EventEmitter implements TaskLike { /** Settles persistence waiters when the task or current stream generation ends. */ private cancelAssistantMessagePersistence(): void { - if (this.assistantMessageRetryTimeoutHandle !== undefined) { - clearTimeout(this.assistantMessageRetryTimeoutHandle) - this.assistantMessageRetryTimeoutHandle = undefined - } this.resolveAssistantMessagePersistence?.("cancelled") this.assistantMessagePersistenceCancellation?.resolve() } @@ -1107,7 +1098,6 @@ export class Task extends EventEmitter implements TaskLike { // For API requests, consecutive same-role messages are merged via mergeConsecutiveApiMessages() // so rewind/edit behavior can still reference original message boundaries. - /** Replaces the entire API conversation history and persists the new state. */ async overwriteApiConversationHistory(newHistory: ApiMessage[]) { this.apiConversationHistory = newHistory await this.saveApiConversationHistory() @@ -1192,7 +1182,6 @@ export class Task extends EventEmitter implements TaskLike { return saved } - /** Persists the current API conversation history to disk, returning false on I/O errors. */ private async saveApiConversationHistory(): Promise { try { await saveApiMessages({ @@ -1232,13 +1221,9 @@ export class Task extends EventEmitter implements TaskLike { const finish = (completed: boolean) => { if (settled) return settled = true - if (this.assistantMessageRetryTimeoutHandle !== undefined) { - this.assistantMessageRetryTimeoutHandle = undefined - } resolve(completed) } const timer = setTimeout(() => finish(true), delays[attempt]) - this.assistantMessageRetryTimeoutHandle = timer void cancellation.promise.then(() => { clearTimeout(timer) finish(false) @@ -1269,15 +1254,10 @@ export class Task extends EventEmitter implements TaskLike { // Cline Messages - /** Reads the persisted Cline messages from disk for this task. */ private async getSavedClineMessages(): Promise { return readTaskMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } - /** - * Appends a new Cline message, posts it to the webview, emits an event, and persists. - * Partial messages and unanswered asks are flushed immediately to the webview. - */ private async addToClineMessages(message: ClineMessage) { this.clineMessages.push(message) const provider = this.providerRef.deref() @@ -1330,10 +1310,6 @@ export class Task extends EventEmitter implements TaskLike { } } - /** - * Updates a Cline message in the webview and emits an event. - * Non-partial messages are synced to cloud telemetry if not already synced. - */ private async updateClineMessage(message: ClineMessage) { const provider = this.providerRef.deref() await provider?.postMessageToWebview({ type: "messageUpdated", clineMessage: message }) @@ -1353,7 +1329,6 @@ export class Task extends EventEmitter implements TaskLike { } } - /** Persists Cline messages and updates task metadata in the history store. Returns false on failure. */ private async saveClineMessages(): Promise { try { await saveTaskMessages({ diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 12f7ef212b..c06b2c6ead 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -682,7 +682,7 @@ describe("Task persistence", () => { task.dispose() }) - it("stops retry attempts immediately when cancelled during delay", async () => { + it("does not retry when cancelled after the delay resolves but before persistence starts", async () => { vi.useFakeTimers() mockSaveApiMessages.mockRejectedValueOnce(new Error("initial write failed")).mockResolvedValue(undefined) @@ -700,14 +700,13 @@ describe("Task persistence", () => { }) const waiting = task.waitForCurrentAssistantMessagePersistence() - // Cancel during the first retry delay (100ms) - await vi.advanceTimersByTimeAsync(50) + // Resolve the delay without flushing its promise continuation, then cancel at the save boundary. + vi.advanceTimersByTime(100) task.dispose() await expect(waiting).resolves.toBe(false) await vi.runAllTimersAsync() - // Should only have attempted the initial save, no retry saves expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) } finally { vi.useRealTimers() diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index 6c3d9bd3e3..f9252d054f 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -277,10 +277,6 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { return "delegated" } - /** - * Handles streaming partial blocks for attempt_completion, updating the completion result - * or command ask as the model streams its response. - */ override async handlePartial(task: Task, block: ToolUse<"attempt_completion">): Promise { const result: string | undefined = block.params.result const command: string | undefined = block.params.command From 5f9673f83ad7140a525f53a2f8b6c4c27472104b Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 03:22:23 +0000 Subject: [PATCH 11/22] test(formal): model completion persistence lifecycle --- .github/alloy/CompletionPersistence.als | 141 ------------ .github/alloy/README.md | 57 ----- .github/workflows/code-qa.yml | 4 +- AGENTS.md | 2 +- docs/architecture/task-lifecycle-model.md | 56 +++-- package.json | 3 +- scripts/check-completion-persistence.ts | 248 ++++++++++++++++++++++ 7 files changed, 292 insertions(+), 219 deletions(-) delete mode 100644 .github/alloy/CompletionPersistence.als delete mode 100644 .github/alloy/README.md create mode 100644 scripts/check-completion-persistence.ts diff --git a/.github/alloy/CompletionPersistence.als b/.github/alloy/CompletionPersistence.als deleted file mode 100644 index 579f43448f..0000000000 --- a/.github/alloy/CompletionPersistence.als +++ /dev/null @@ -1,141 +0,0 @@ -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 deleted file mode 100644 index cdf184bfb5..0000000000 --- a/.github/alloy/README.md +++ /dev/null @@ -1,57 +0,0 @@ -# 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/.github/workflows/code-qa.yml b/.github/workflows/code-qa.yml index 7c71478266..b4e5881f6b 100644 --- a/.github/workflows/code-qa.yml +++ b/.github/workflows/code-qa.yml @@ -90,8 +90,8 @@ jobs: run: pnpm lint - name: Check types run: pnpm check-types - - name: Model-check concurrent task lifecycle - run: pnpm lifecycle:model-check + - name: Model-check task lifecycle + run: pnpm lifecycle:model build-vsix: name: Build test VSIX diff --git a/AGENTS.md b/AGENTS.md index 3b5be80ede..e14148e1be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,7 @@ Prefer the narrowest test layer that proves the behavior. This follows standard ## Task Lifecycle Changes - Read `docs/architecture/task-lifecycle-model.md` before changing task status, delegation, interruption, completion, abandonment, persistence ownership, or scheduler fan-out behavior. -- Keep lifecycle mutations in the shared reducers under `src/core/task-persistence/taskLifecycle.ts`. Update model actions, invariants, or named semantic landmarks for every new transition or concurrency bug class representable in the lifecycle model, then run `pnpm lifecycle:model-check`. +- Keep lifecycle mutations in the shared reducers under `src/core/task-persistence/taskLifecycle.ts`. Update model actions, invariants, or named semantic landmarks for every new transition or concurrency bug class representable in the lifecycle model, then run `pnpm lifecycle:model`. - Add extension-host E2E coverage only for a boundary the reducer model cannot prove, such as restart visibility, real persistence/rehydration, delayed provider streams, scheduler permits, or webview task scoping. Do not duplicate reducer interleavings in E2E. - Run the focused lifecycle tests and `pnpm test` before completing a Zoo Code lifecycle change. diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 588ffd5204..cc669ffed2 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -3,14 +3,14 @@ Zoo Code checks its persisted task delegation lifecycle with a bounded, exhaustive state explorer. Run it locally with: ```sh -pnpm lifecycle:model-check +pnpm lifecycle:model ``` -The check runs in the `compile` CI job after type checking. It fails if it finds an invariant violation, a modeled action becomes unreachable, or exploration exceeds its declared state budget. A violation includes the shortest breadth-first event trace, every intermediate state, and the active bounds so the sequence can be replayed as a focused regression test. +The check runs in the `compile` CI job after type checking. It fails if it finds an invariant violation, a modeled action becomes unreachable, a named semantic landmark disappears, or exploration exceeds its declared state budget. A violation includes the shortest breadth-first event trace, every intermediate state, and the active bounds so the sequence can be replayed as a focused regression test. The command generates no checked-in artifacts; its reports and counterexamples are written to standard output. `pnpm lifecycle:model-check` remains as a compatibility alias. ## Why an executable TypeScript model -The initial model uses a small explicit-state explorer rather than adding Quint, TLA+/TLC, or Alloy. This is deliberate: +The models use small explicit-state explorers rather than adding Quint, TLA+/TLC, or Alloy. This is deliberate: - Zoo's current risks are finite safety properties over a small persisted state machine, not yet temporal liveness or fairness properties. - The explorer calls the production transition functions in `src/core/task-persistence/taskLifecycle.ts`. `ClineProvider` uses those same functions inside serialized and atomic store operations, reducing specification drift. @@ -39,7 +39,7 @@ Production completion also accepts a recovery-compatible `active` parent that st ## Shared-store concurrency model -The same `pnpm lifecycle:model-check` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: +The same `pnpm lifecycle:model` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: - each host has an independent cache and host-local mutex; - store read/update operations hold the host mutex, while live-task snapshots used by completion and message saves may outlive it; @@ -63,9 +63,24 @@ The known-unsafe witnesses currently compare exact shortest action sequences. Th `TaskHistoryStore.realConcurrency.spec.ts` complements the abstract interleavings with one synchronized integration smoke check through the real `proper-lockfile` and filesystem rename path; broader VS Code E2E remains reserved for restart and extension-host behavior. +## Completion persistence model + +`scripts/check-completion-persistence.ts` models the completion-readiness protocol that protects the public `TaskCompleted` event. It starts from both standalone and delegated tasks and exhaustively interleaves: + +- starting, finishing, or failing the assistant-history write; +- accepting completion before, during, or after persistence; +- scheduling a bounded retry, completing its delay, and starting the retry write; +- exhausting retries; +- cancellation or disposal at every reachable non-completed state; and +- emitting completion. + +The model abstracts restart visibility as the `durable` history phase. It allows an already-started write to finish after cancellation because the filesystem operation itself is not cancellable, but it forbids starting a retry write or emitting completion after cancellation. The retry bound is two write starts (the initial attempt plus one retry), which is sufficient to cover the ordering and cancellation state classes without mirroring the production retry count. + +Six semantic landmarks keep the intended positive and negative paths reachable: delayed completion remains pending, failed completion remains pending, exhausted retries settle without completion, cancellation can win after retry delay but before persistence, and both standalone and delegated tasks can complete after durable history. The checker explores all reachable states through depth 10 and fails rather than reporting a truncated pass if an unseen successor remains. + ## Invariants -The checker currently enforces: +The task delegation checker currently enforces: 1. A delegated parent has exactly one `awaitingChildId`, and `delegatedToId` matches it. 2. The awaited child exists, links back to the parent, is not completed, and remains in `childIds`. A delegated child may itself await a nested child. @@ -75,22 +90,29 @@ The checker currently enforces: 6. Completed task records cannot be changed by later lifecycle events. 7. Active-child re-delegation, stale completion after ownership moves to another child, duplicate/late completion, and abandonment of a live child are rejected by the shared production guards. -These are safety claims within the documented bounds. The check does not claim liveness, fairness, crash consistency, filesystem-lock correctness, API history correctness, or exhaustive coverage of arbitrary task counts. It also does not distinguish a delayed pre-interruption completion from a legitimate post-resume completion for the same child ID; that requires a persisted attempt/generation token before it can become a sound invariant. +The completion persistence checker additionally enforces: + +1. `TaskCompleted` requires accepted completion and restart-visible assistant history. +2. Delayed, failed, and retry-exhausted persistence cannot emit completion. +3. Cancellation or disposal settles the modeled readiness wait, clears pending retry state, starts no later retry write, and emits no completion. +4. Delegated completion crosses the same durability boundary as standalone completion. + +These are safety claims within the documented bounds. The checks do not claim liveness, fairness, power-loss durability, filesystem-lock correctness, or exhaustive coverage of arbitrary task counts or retry counts. The completion explorer specifies the event contract rather than importing `Task` or `AttemptCompletionTool`; focused unit tests and the restart E2E verify that concrete production paths implement the modeled guards. The lifecycle checker also does not distinguish a delayed pre-interruption completion from a legitimate post-resume completion for the same child ID; that requires a persisted attempt/generation token before it can become a sound invariant. ## Open-issue traceability The following map separates issue observations from the architectural interpretation encoded here. Open issues can change after this document is written; follow each link for current status. -| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | -| ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | -| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | -| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it, while an Alloy abstraction permits the ordering. | A completion/readiness contract must define whether completion implies restart visibility. This is a liveness/durability boundary, not only a `HistoryItem` safety transition. | Not claimed by this checker. Add a controlled persistence barrier test after the contract decision; move to temporal model checking if eventual readiness and failure handling become protocol guarantees. | -| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | -| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | -| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | -| [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. It warrants a parser-scope model or deterministic interleaving test, not an unrelated field in the delegation model. | -| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | +| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | +| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | +| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it. | Completion implies restart-visible assistant history. Delayed or failed writes keep completion pending, and cancellation settles readiness without starting stale retries or emitting completion. | The completion persistence explorer checks the bounded event-ordering and cancellation contract for standalone and delegated tasks. Focused `Task` and `AttemptCompletionTool` tests cover the production adapter; `restart-persistence.test.ts` verifies visibility through a fresh extension host. | +| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | +| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | +| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | +| [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. It warrants a parser-scope model or deterministic interleaving test, not an unrelated field in the delegation model. | +| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | The issue-derived cases intentionally map to bug classes rather than issue-specific flags. In particular, stale event ownership, monotonic terminal/detached state, explicit scope, and single-writer boundaries generalize to future concurrent task work. @@ -104,7 +126,7 @@ When production lifecycle behavior changes: 4. Increase depth or task slots only when the new scenario requires it. Keep the state budget explicit and ensure CI completes quickly. 5. Convert any discovered counterexample into a focused production regression test as well as retaining the architectural invariant. -Do not weaken bounds or remove an invariant merely to make CI pass. If state growth becomes difficult to control, split independent protocols or move the model to TLC/Quint with an implementation trace adapter rather than silently sampling the state space. +Completion-readiness changes belong in `scripts/check-completion-persistence.ts`; shared-store interleavings belong in `scripts/check-task-store-concurrency.ts`. Do not weaken bounds or remove an invariant merely to make CI pass. If state growth becomes difficult to control, split independent protocols or move the model to TLC/Quint with an implementation trace adapter rather than silently sampling the state space. ## Test layering diff --git a/package.json b/package.json index 8431467918..9763a00b1b 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,8 @@ "check-types": "turbo check-types --log-order grouped --output-logs new-only", "test": "turbo test --log-order grouped --output-logs new-only", "test:mutation-ci": "node --test scripts/stryker-diff.test.mjs", - "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts", + "lifecycle:model": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-completion-persistence.ts", + "lifecycle:model-check": "pnpm lifecycle:model", "test:coverage": "turbo test:coverage --log-order grouped --output-logs new-only", "format": "turbo format --log-order grouped --output-logs new-only", "build": "turbo build --log-order grouped --output-logs new-only", diff --git a/scripts/check-completion-persistence.ts b/scripts/check-completion-persistence.ts new file mode 100644 index 0000000000..1a49f44fb8 --- /dev/null +++ b/scripts/check-completion-persistence.ts @@ -0,0 +1,248 @@ +type TaskKind = "standalone" | "delegated" +type HistoryPhase = "idle" | "writing" | "failed" | "durable" | "exhausted" +type RetryPhase = "idle" | "waiting" | "ready" +type WriteStarts = 0 | 1 | 2 + +interface ModelState { + kind: TaskKind + history: HistoryPhase + retry: RetryPhase + writeStarts: WriteStarts + completionAccepted: boolean + completionEmitted: boolean + cancelled: boolean + waitSettled: boolean + cancelledAtRetryBoundary: boolean +} + +interface Transition { + name: string + next: ModelState +} + +interface TraceStep { + action: string + state: ModelState +} + +const MAX_DEPTH = 10 +const MAX_STATES = 1_000 +const taskKinds = ["standalone", "delegated"] as const +const expectedActions = [ + "start-initial-write", + "accept-completion", + "finish-write", + "fail-write", + "schedule-retry", + "finish-retry-delay", + "start-retry-write", + "exhaust-retries", + "cancel", + "emit-completion", +] as const +const invariantNames = [ + "completion requires restart-visible history", + "delayed and failed persistence keep completion pending", + "cancellation settles waits without later writes or completion", +] as const +const semanticLandmarks = { + "delayed-completion-pending": (state: ModelState) => + state.completionAccepted && state.history === "writing" && !state.completionEmitted, + "failed-completion-pending": (state: ModelState) => + state.completionAccepted && state.history === "failed" && !state.completionEmitted, + "exhausted-completion-pending": (state: ModelState) => + state.completionAccepted && state.history === "exhausted" && state.waitSettled && !state.completionEmitted, + "cancelled-retry-boundary": (state: ModelState) => + state.cancelledAtRetryBoundary && state.waitSettled && state.retry === "idle" && !state.completionEmitted, + "standalone-durable-completion": (state: ModelState) => + state.kind === "standalone" && state.history === "durable" && state.completionEmitted, + "delegated-durable-completion": (state: ModelState) => + state.kind === "delegated" && state.history === "durable" && state.completionEmitted, +} satisfies Record boolean> + +function initialState(kind: TaskKind): ModelState { + return { + kind, + history: "idle", + retry: "idle", + writeStarts: 0, + completionAccepted: false, + completionEmitted: false, + cancelled: false, + waitSettled: false, + cancelledAtRetryBoundary: false, + } +} + +function transitions(state: ModelState): Transition[] { + const result: Transition[] = [] + + if (state.history === "idle" && !state.cancelled) { + result.push({ + name: "start-initial-write", + next: { ...state, history: "writing", writeStarts: 1 }, + }) + } + if (!state.completionAccepted && !state.cancelled) { + result.push({ name: "accept-completion", next: { ...state, completionAccepted: true } }) + } + if (state.history === "writing") { + result.push({ + name: "finish-write", + next: { ...state, history: "durable", waitSettled: true }, + }) + result.push({ name: "fail-write", next: { ...state, history: "failed" } }) + } + if (state.history === "failed" && state.retry === "idle" && !state.cancelled) { + if (state.writeStarts < 2) { + result.push({ name: "schedule-retry", next: { ...state, retry: "waiting" } }) + } else { + result.push({ + name: "exhaust-retries", + next: { ...state, history: "exhausted", waitSettled: true }, + }) + } + } + if (state.retry === "waiting" && !state.cancelled) { + result.push({ name: "finish-retry-delay", next: { ...state, retry: "ready" } }) + } + if (state.retry === "ready" && !state.cancelled && state.writeStarts < 2) { + result.push({ + name: "start-retry-write", + next: { + ...state, + history: "writing", + retry: "idle", + writeStarts: (state.writeStarts + 1) as WriteStarts, + }, + }) + } + if (!state.cancelled && !state.completionEmitted) { + result.push({ + name: "cancel", + next: { + ...state, + retry: "idle", + cancelled: true, + waitSettled: true, + cancelledAtRetryBoundary: state.retry === "ready", + }, + }) + } + if ( + state.completionAccepted && + state.history === "durable" && + state.waitSettled && + !state.completionEmitted && + !state.cancelled + ) { + result.push({ + name: "emit-completion", + next: { ...state, completionEmitted: true, waitSettled: true }, + }) + } + + return result +} + +function invariantViolations(state: ModelState): string[] { + const violations: string[] = [] + if (state.completionEmitted && state.history !== "durable") { + violations.push("completion emitted before assistant history became restart-visible") + } + if ( + state.completionAccepted && + (state.history === "writing" || state.history === "failed" || state.history === "exhausted") && + state.completionEmitted + ) { + violations.push("delayed or failed persistence allowed completion") + } + if (state.cancelled && (!state.waitSettled || state.retry !== "idle" || state.completionEmitted)) { + violations.push("cancellation did not settle the wait and suppress retry/completion") + } + return violations +} + +function transitionViolations(previous: ModelState, transition: Transition): string[] { + const violations: string[] = [] + if (previous.cancelled && transition.next.writeStarts > previous.writeStarts) { + violations.push(`cancelled task started a stale history write after ${transition.name}`) + } + if (previous.cancelled && !previous.completionEmitted && transition.next.completionEmitted) { + violations.push(`cancelled task emitted completion after ${transition.name}`) + } + return violations +} + +function canonical(state: ModelState): string { + return JSON.stringify(state) +} + +function formatCounterexample(message: string, trace: TraceStep[]): string { + return [ + `Completion persistence invariant failed: ${message}`, + `Bounds: depth=${MAX_DEPTH}, states=${MAX_STATES}, writes<=2`, + ...trace.map((step, index) => `${index}. ${step.action}\n${JSON.stringify(step.state, null, 2)}`), + ].join("\n") +} + +function runModelCheck(): number { + const queue: Array<{ state: ModelState; trace: TraceStep[] }> = taskKinds.map((kind) => { + const state = initialState(kind) + return { state, trace: [{ action: `initial(${kind})`, state }] } + }) + const visited = new Set(queue.map(({ state }) => canonical(state))) + const reachedActions = new Set() + const reachedLandmarks = new Set() + const frontier: ModelState[] = [] + + for (let index = 0; index < queue.length; index++) { + const node = queue[index]! + for (const [name, predicate] of Object.entries(semanticLandmarks)) { + if (predicate(node.state)) reachedLandmarks.add(name) + } + const violations = invariantViolations(node.state) + if (violations.length) throw new Error(formatCounterexample(violations.join("; "), node.trace)) + if (node.trace.length - 1 === MAX_DEPTH) { + frontier.push(node.state) + continue + } + + for (const transition of transitions(node.state)) { + reachedActions.add(transition.name) + const trace = [...node.trace, { action: transition.name, state: transition.next }] + const violations = transitionViolations(node.state, transition) + if (violations.length) throw new Error(formatCounterexample(violations.join("; "), trace)) + const key = canonical(transition.next) + if (visited.has(key)) continue + visited.add(key) + queue.push({ state: transition.next, trace }) + if (visited.size > MAX_STATES) { + throw new Error(`Completion persistence exploration exceeded its ${MAX_STATES}-state budget`) + } + } + } + + const unreachableActions = expectedActions.filter((action) => !reachedActions.has(action)) + if (unreachableActions.length) { + throw new Error(`Completion persistence model has unreachable actions: ${unreachableActions.join(", ")}`) + } + const missingLandmarks = Object.keys(semanticLandmarks).filter((name) => !reachedLandmarks.has(name)) + if (missingLandmarks.length) { + throw new Error(`Completion persistence model has unreachable landmarks: ${missingLandmarks.join(", ")}`) + } + const unexploredSuccessor = frontier + .flatMap((state) => transitions(state)) + .find((transition) => !visited.has(canonical(transition.next))) + if (unexploredSuccessor) { + throw new Error( + `Completion persistence exploration reached depth ${MAX_DEPTH} with an unseen successor (${unexploredSuccessor.name})`, + ) + } + return visited.size +} + +const checkedStates = runModelCheck() +console.log( + `Completion persistence model check passed: ${checkedStates} states, ${expectedActions.length}/${expectedActions.length} actions reachable, ${invariantNames.length} invariants, ${Object.keys(semanticLandmarks).length}/${Object.keys(semanticLandmarks).length} landmarks reached, depth <= ${MAX_DEPTH}, writes <= 2`, +) From 777aacfda68e8f6e263710f6a3ea69b0be3a2bc6 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 03:28:06 +0000 Subject: [PATCH 12/22] test(formal): model delegated completion ordering --- docs/architecture/task-lifecycle-model.md | 12 +-- scripts/check-completion-persistence.ts | 100 +++++++++++++++------- 2 files changed, 77 insertions(+), 35 deletions(-) diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index cc669ffed2..36339096da 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -13,7 +13,7 @@ The check runs in the `compile` CI job after type checking. It fails if it finds The models use small explicit-state explorers rather than adding Quint, TLA+/TLC, or Alloy. This is deliberate: - Zoo's current risks are finite safety properties over a small persisted state machine, not yet temporal liveness or fairness properties. -- The explorer calls the production transition functions in `src/core/task-persistence/taskLifecycle.ts`. `ClineProvider` uses those same functions inside serialized and atomic store operations, reducing specification drift. +- The delegation and shared-store explorers call production transition functions from `src/core/task-persistence`. `ClineProvider` uses those same functions inside serialized and atomic store operations, reducing specification drift for those protocols. - Breadth-first exploration gives a deterministic, shortest-by-event counterexample with no Java or separate specification toolchain. - Bounds and budget exhaustion are explicit. CI never reports a truncated exploration as a pass. @@ -71,12 +71,13 @@ The known-unsafe witnesses currently compare exact shortest action sequences. Th - accepting completion before, during, or after persistence; - scheduling a bounded retry, completing its delay, and starting the retry write; - exhausting retries; -- cancellation or disposal at every reachable non-completed state; and +- cancellation or disposal at every reachable non-completed state; +- delegated parent reopen success or failure after durable child history; and - emitting completion. The model abstracts restart visibility as the `durable` history phase. It allows an already-started write to finish after cancellation because the filesystem operation itself is not cancellable, but it forbids starting a retry write or emitting completion after cancellation. The retry bound is two write starts (the initial attempt plus one retry), which is sufficient to cover the ordering and cancellation state classes without mirroring the production retry count. -Six semantic landmarks keep the intended positive and negative paths reachable: delayed completion remains pending, failed completion remains pending, exhausted retries settle without completion, cancellation can win after retry delay but before persistence, and both standalone and delegated tasks can complete after durable history. The checker explores all reachable states through depth 10 and fails rather than reporting a truncated pass if an unseen successor remains. +Seven semantic landmarks keep the intended positive and negative paths reachable: delayed completion remains pending, failed completion remains pending, exhausted retries settle without completion, cancellation can win after retry delay but before persistence, delegated reopen failure emits no delegated completion, and both standalone and delegated tasks can complete after durable history. The checker explores all reachable states through depth 10 and fails rather than reporting a truncated pass if an unseen successor remains. ## Invariants @@ -95,9 +96,10 @@ The completion persistence checker additionally enforces: 1. `TaskCompleted` requires accepted completion and restart-visible assistant history. 2. Delayed, failed, and retry-exhausted persistence cannot emit completion. 3. Cancellation or disposal settles the modeled readiness wait, clears pending retry state, starts no later retry write, and emits no completion. -4. Delegated completion crosses the same durability boundary as standalone completion. +4. Delegated completion crosses the same durability boundary as standalone completion and requires successful parent reopen. +5. A failed delegated parent reopen cannot emit the delegated completion event. -These are safety claims within the documented bounds. The checks do not claim liveness, fairness, power-loss durability, filesystem-lock correctness, or exhaustive coverage of arbitrary task counts or retry counts. The completion explorer specifies the event contract rather than importing `Task` or `AttemptCompletionTool`; focused unit tests and the restart E2E verify that concrete production paths implement the modeled guards. The lifecycle checker also does not distinguish a delayed pre-interruption completion from a legitimate post-resume completion for the same child ID; that requires a persisted attempt/generation token before it can become a sound invariant. +These are safety claims within the documented bounds. The checks do not claim liveness, fairness, power-loss durability, filesystem-lock correctness, or exhaustive coverage of arbitrary task counts or retry counts. The completion explorer specifies the event contract rather than importing `Task` or `AttemptCompletionTool`; focused unit tests and the restart E2E verify that concrete production paths implement the modeled guards. Delegated reopen is abstracted as one success-or-failure event after durable child history; fallback from a failed reopen into the normal standalone completion flow remains production-test coverage rather than part of this model. The lifecycle checker also does not distinguish a delayed pre-interruption completion from a legitimate post-resume completion for the same child ID; that requires a persisted attempt/generation token before it can become a sound invariant. ## Open-issue traceability diff --git a/scripts/check-completion-persistence.ts b/scripts/check-completion-persistence.ts index 1a49f44fb8..10c9d96769 100644 --- a/scripts/check-completion-persistence.ts +++ b/scripts/check-completion-persistence.ts @@ -2,6 +2,7 @@ type TaskKind = "standalone" | "delegated" type HistoryPhase = "idle" | "writing" | "failed" | "durable" | "exhausted" type RetryPhase = "idle" | "waiting" | "ready" type WriteStarts = 0 | 1 | 2 +type DelegationPhase = "not-applicable" | "awaiting-reopen" | "reopened" | "reopen-failed" interface ModelState { kind: TaskKind @@ -13,6 +14,7 @@ interface ModelState { cancelled: boolean waitSettled: boolean cancelledAtRetryBoundary: boolean + delegation: DelegationPhase } interface Transition { @@ -38,13 +40,47 @@ const expectedActions = [ "start-retry-write", "exhaust-retries", "cancel", + "reopen-parent", + "fail-parent-reopen", "emit-completion", ] as const -const invariantNames = [ - "completion requires restart-visible history", - "delayed and failed persistence keep completion pending", - "cancellation settles waits without later writes or completion", -] as const +const stateInvariants = { + "completion requires accepted restart-visible history": (state: ModelState) => + state.completionEmitted && (!state.completionAccepted || state.history !== "durable" || !state.waitSettled) + ? "completion emitted before accepted assistant history became restart-visible" + : undefined, + "delayed and failed persistence keep completion pending": (state: ModelState) => { + if (state.cancelled || !state.completionAccepted) return undefined + if ((state.history === "writing" || state.history === "failed") && state.waitSettled) { + return "completion wait settled while persistence could still retry" + } + if (state.history === "exhausted" && (!state.waitSettled || state.completionEmitted)) { + return "exhausted persistence did not settle without completion" + } + return state.history !== "durable" && state.completionEmitted + ? "delayed or failed persistence allowed completion" + : undefined + }, + "cancellation settles waits and suppresses retry/completion": (state: ModelState) => + state.cancelled && (!state.waitSettled || state.retry !== "idle" || state.completionEmitted) + ? "cancellation did not settle the wait and suppress retry/completion" + : undefined, + "delegated completion requires successful parent reopen": (state: ModelState) => + state.kind === "delegated" && state.completionEmitted && state.delegation !== "reopened" + ? "delegated completion emitted before the parent reopened" + : undefined, +} satisfies Record string | undefined> +const transitionInvariants = { + "cancellation starts no later write or completion": (previous: ModelState, transition: Transition) => { + if (previous.cancelled && transition.next.writeStarts > previous.writeStarts) { + return `cancelled task started a stale history write after ${transition.name}` + } + if (previous.cancelled && !previous.completionEmitted && transition.next.completionEmitted) { + return `cancelled task emitted completion after ${transition.name}` + } + return undefined + }, +} satisfies Record string | undefined> const semanticLandmarks = { "delayed-completion-pending": (state: ModelState) => state.completionAccepted && state.history === "writing" && !state.completionEmitted, @@ -57,7 +93,12 @@ const semanticLandmarks = { "standalone-durable-completion": (state: ModelState) => state.kind === "standalone" && state.history === "durable" && state.completionEmitted, "delegated-durable-completion": (state: ModelState) => - state.kind === "delegated" && state.history === "durable" && state.completionEmitted, + state.kind === "delegated" && + state.history === "durable" && + state.delegation === "reopened" && + state.completionEmitted, + "delegated-reopen-failure-pending": (state: ModelState) => + state.kind === "delegated" && state.delegation === "reopen-failed" && !state.completionEmitted, } satisfies Record boolean> function initialState(kind: TaskKind): ModelState { @@ -71,6 +112,7 @@ function initialState(kind: TaskKind): ModelState { cancelled: false, waitSettled: false, cancelledAtRetryBoundary: false, + delegation: kind === "delegated" ? "awaiting-reopen" : "not-applicable", } } @@ -130,9 +172,21 @@ function transitions(state: ModelState): Transition[] { }) } if ( + state.kind === "delegated" && + state.delegation === "awaiting-reopen" && state.completionAccepted && state.history === "durable" && state.waitSettled && + !state.cancelled + ) { + result.push({ name: "reopen-parent", next: { ...state, delegation: "reopened" } }) + result.push({ name: "fail-parent-reopen", next: { ...state, delegation: "reopen-failed" } }) + } + if ( + state.completionAccepted && + state.history === "durable" && + state.waitSettled && + (state.kind === "standalone" || state.delegation === "reopened") && !state.completionEmitted && !state.cancelled ) { @@ -146,32 +200,17 @@ function transitions(state: ModelState): Transition[] { } function invariantViolations(state: ModelState): string[] { - const violations: string[] = [] - if (state.completionEmitted && state.history !== "durable") { - violations.push("completion emitted before assistant history became restart-visible") - } - if ( - state.completionAccepted && - (state.history === "writing" || state.history === "failed" || state.history === "exhausted") && - state.completionEmitted - ) { - violations.push("delayed or failed persistence allowed completion") - } - if (state.cancelled && (!state.waitSettled || state.retry !== "idle" || state.completionEmitted)) { - violations.push("cancellation did not settle the wait and suppress retry/completion") - } - return violations + return Object.entries(stateInvariants).flatMap(([name, check]) => { + const violation = check(state) + return violation ? [`${name}: ${violation}`] : [] + }) } function transitionViolations(previous: ModelState, transition: Transition): string[] { - const violations: string[] = [] - if (previous.cancelled && transition.next.writeStarts > previous.writeStarts) { - violations.push(`cancelled task started a stale history write after ${transition.name}`) - } - if (previous.cancelled && !previous.completionEmitted && transition.next.completionEmitted) { - violations.push(`cancelled task emitted completion after ${transition.name}`) - } - return violations + return Object.entries(transitionInvariants).flatMap(([name, check]) => { + const violation = check(previous, transition) + return violation ? [`${name}: ${violation}`] : [] + }) } function canonical(state: ModelState): string { @@ -243,6 +282,7 @@ function runModelCheck(): number { } const checkedStates = runModelCheck() +const invariantCount = Object.keys(stateInvariants).length + Object.keys(transitionInvariants).length console.log( - `Completion persistence model check passed: ${checkedStates} states, ${expectedActions.length}/${expectedActions.length} actions reachable, ${invariantNames.length} invariants, ${Object.keys(semanticLandmarks).length}/${Object.keys(semanticLandmarks).length} landmarks reached, depth <= ${MAX_DEPTH}, writes <= 2`, + `Completion persistence model check passed: ${checkedStates} states, ${expectedActions.length}/${expectedActions.length} actions reachable, ${invariantCount} invariants, ${Object.keys(semanticLandmarks).length}/${Object.keys(semanticLandmarks).length} landmarks reached, depth <= ${MAX_DEPTH}, writes <= 2`, ) From 1bf89473b1c64edb5ce88a957ff0638df4c37cea Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 13:46:38 +0000 Subject: [PATCH 13/22] test(task): satisfy changed-code mutation gate --- scripts/stryker-diff.mjs | 14 +- scripts/stryker-diff.test.mjs | 27 ++++ src/core/task/Task.ts | 42 +++--- .../task/__tests__/Task.persistence.spec.ts | 80 ++++++++++- src/core/task/__tests__/Task.spec.ts | 5 + src/core/tools/AttemptCompletionTool.ts | 6 +- .../__tests__/attemptCompletionTool.spec.ts | 29 ++++ ...i-task-conversation-history-length.spec.ts | 125 ++++++++++++++++++ src/extension/api.ts | 47 ++++--- 9 files changed, 322 insertions(+), 53 deletions(-) diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index c0e8a6cd1a..75d9096de2 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -293,17 +293,25 @@ export function parseVitestTestFiles(report, runRoot) { } export function preferDirectTestFiles(testFiles, sourceFiles) { - const sourceNames = sourceFiles.map((sourceFile) => path.posix.basename(sourceFile, path.posix.extname(sourceFile))) + const sourceNames = sourceFiles.map((sourceFile) => + path.posix.basename(sourceFile, path.posix.extname(sourceFile)).toLowerCase(), + ) const direct = testFiles.filter((testFile) => { const testName = path.posix.basename(testFile) + const normalizedTestName = testName.toLowerCase() return sourceNames.some( (sourceName) => - testName.startsWith(`${sourceName}.`) && /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), + (normalizedTestName.startsWith(`${sourceName}.`) || normalizedTestName.startsWith(`${sourceName}-`)) && + /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), ) }) return direct.length > 0 ? direct : testFiles } +export function shouldUseVitestRelated(packageEntry) { + return (packageEntry.testFiles?.length ?? 0) === 0 && packageEntry.vitestRelated !== false +} + export function resolveVitestBinary(repoRoot, packageEntry) { const packageRoot = path.join(repoRoot, packageEntry.root) const runRoot = path.join(repoRoot, packageEntry.runRoot ?? packageEntry.root) @@ -379,7 +387,7 @@ function runStryker(repoRoot, packageEntry, reportRoot, dryRunOnly) { .replaceAll("\\", "/"), STRYKER_REPORT_DIR: reportDirectory, STRYKER_IN_PLACE: "false", - STRYKER_VITEST_RELATED: packageEntry.vitestRelated === false ? "false" : "true", + STRYKER_VITEST_RELATED: shouldUseVitestRelated(packageEntry) ? "true" : "false", STRYKER_TEST_FILES: JSON.stringify(packageEntry.testFiles ?? []), }, }) diff --git a/scripts/stryker-diff.test.mjs b/scripts/stryker-diff.test.mjs index 0f39dc507f..72421f26b3 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -24,6 +24,7 @@ import { parseVitestTestFiles, preferDirectTestFiles, resolveVitestBinary, + shouldUseVitestRelated, packageForPath, runManifest, selectFromGit, @@ -208,6 +209,32 @@ describe("preferDirectTestFiles", () => { ]) assert.deepEqual(preferDirectTestFiles(related, ["webview-ui/src/utils/unmatched.ts"]), related) }) + + it("matches direct tests case-insensitively with dot and hyphen suffixes", () => { + const related = [ + "core/task/__tests__/Task.persistence.spec.ts", + "core/tools/__tests__/attemptCompletionTool.spec.ts", + "extension/__tests__/api-task-conversation-history-length.spec.ts", + "core/task/__tests__/unrelated.spec.ts", + ] + + assert.deepEqual( + preferDirectTestFiles(related, [ + "core/task/Task.ts", + "core/tools/AttemptCompletionTool.ts", + "extension/api.ts", + ]), + related.slice(0, 3), + ) + }) +}) + +describe("shouldUseVitestRelated", () => { + it("does not re-filter an explicit discovered test list", () => { + assert.equal(shouldUseVitestRelated({ testFiles: ["focused.spec.ts"] }), false) + assert.equal(shouldUseVitestRelated({ testFiles: [], vitestRelated: true }), true) + assert.equal(shouldUseVitestRelated({ vitestRelated: false }), false) + }) }) describe("related-test discovery", () => { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 5d2d47ca0c..a4727fa049 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -194,7 +194,8 @@ export interface TaskOptions extends CreateTaskOptions { diffFuzzyThreshold?: number } -type AssistantMessagePersistenceResult = "saved" | "failed" | "cancelled" +const ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED = Symbol() +type AssistantMessagePersistenceResult = boolean | typeof ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED type AssistantMessagePersistenceCancellation = { cancelled: boolean promise: Promise @@ -1033,7 +1034,7 @@ export class Task extends EventEmitter implements TaskLike { } if (message.role === "assistant") { this.assistantMessageSavedToHistory = saved - this.resolveAssistantMessagePersistence(saved ? "saved" : "failed") + this.resolveAssistantMessagePersistence(saved) } } @@ -1050,7 +1051,6 @@ export class Task extends EventEmitter implements TaskLike { resolveCancellation = resolve }), resolve: () => { - if (cancellation.cancelled) return cancellation.cancelled = true resolveCancellation() }, @@ -1061,7 +1061,6 @@ export class Task extends EventEmitter implements TaskLike { /** Settles persistence waiters when the task or current stream generation ends. */ private cancelAssistantMessagePersistence(): void { - this.resolveAssistantMessagePersistence?.("cancelled") this.assistantMessagePersistenceCancellation?.resolve() } @@ -1076,14 +1075,14 @@ export class Task extends EventEmitter implements TaskLike { this.completionPersistenceReadyPromise = (async () => { const result = await Promise.race([ currentPersistence, - currentCancellation.promise.then(() => "cancelled" as const), + currentCancellation.promise.then(() => ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED), ]) - if (result === "cancelled") return false - if (result === "saved") return true + if (result === ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED) return false + if (result) return true const retryResult = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) - if (retryResult === "cancelled") return false - if (retryResult === "failed") { + if (retryResult === ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED) return false + if (!retryResult) { throw new Error("Failed to persist API conversation history before task completion") } this.assistantMessageSavedToHistory = true @@ -1202,7 +1201,7 @@ export class Task extends EventEmitter implements TaskLike { * Used by delegation flow when flushPendingToolResultsToHistory reports failure. */ public async retrySaveApiConversationHistory(): Promise { - return (await this.retrySaveApiConversationHistoryWithCancellation()) === "saved" + return (await this.retrySaveApiConversationHistoryWithCancellation()) === true } /** Retries API-history persistence while allowing the active assistant generation to cancel backoff. */ @@ -1212,44 +1211,35 @@ export class Task extends EventEmitter implements TaskLike { const delays = [100, 500, 1500] for (let attempt = 0; attempt < delays.length; attempt++) { - // Check cancellation before each retry delay - if (cancellation?.cancelled) return "cancelled" - if (cancellation) { const delayCompleted = await new Promise((resolve) => { - let settled = false - const finish = (completed: boolean) => { - if (settled) return - settled = true - resolve(completed) - } - const timer = setTimeout(() => finish(true), delays[attempt]) + const timer = setTimeout(() => resolve(true), delays[attempt]) void cancellation.promise.then(() => { clearTimeout(timer) - finish(false) + resolve(false) }) }) - if (!delayCompleted) return "cancelled" + if (!delayCompleted) return ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED } else { await new Promise((resolve) => setTimeout(resolve, delays[attempt])) } // Check cancellation before each save attempt - if (cancellation?.cancelled) return "cancelled" + if (cancellation?.cancelled) return ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED console.warn( `[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`, ) const success = await this.saveApiConversationHistory() - if (cancellation?.cancelled) return "cancelled" + if (cancellation?.cancelled) return ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED if (success) { - return "saved" + return true } } - return "failed" + return false } // Cline Messages diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index c06b2c6ead..5a4b729fe4 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -395,6 +395,46 @@ describe("Task persistence", () => { expect(callArgs.messages).toEqual(task.apiConversationHistory) }) + it("settles the current assistant persistence boundary only for assistant messages", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const privateTask = getTaskPersistenceAccess(task) + const waiting = task.waitForCurrentAssistantMessagePersistence() + let settled = false + void waiting.then(() => { + settled = true + }) + + await privateTask.addToApiConversationHistory({ role: "user", content: "hello" }) + await new Promise((resolve) => setImmediate(resolve)) + expect(settled).toBe(false) + + await privateTask.addToApiConversationHistory({ role: "assistant", content: "done" }) + await expect(waiting).resolves.toBe(true) + expect(task.assistantMessageSavedToHistory).toBe(true) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) + }) + + it("shares one completion persistence result per assistant generation", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const first = task.waitForCurrentAssistantMessagePersistence() + const second = task.waitForCurrentAssistantMessagePersistence() + + expect(second).toBe(first) + task.dispose() + await expect(first).resolves.toBe(false) + }) + it("emits TaskCompleted only after API history persistence succeeds", async () => { const saveDeferred = createDeferred() mockSaveApiMessages.mockReturnValueOnce(saveDeferred.promise) @@ -545,7 +585,7 @@ describe("Task persistence", () => { expect(mockSaveApiMessages).toHaveBeenCalledTimes(4) expect(completionListener).not.toHaveBeenCalled() expect(callbacks.handleError).toHaveBeenCalledWith( - "inspecting site", + "persisting task completion", expect.objectContaining({ message: "Failed to persist API conversation history before task completion", }), @@ -616,6 +656,7 @@ describe("Task persistence", () => { expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) expect(callbacks.handleError).not.toHaveBeenCalled() expect(completionListener).toHaveBeenCalledTimes(1) + expect(task.assistantMessageSavedToHistory).toBe(true) // Assert ordering: retry save completes before TaskCompleted is emitted expect(vi.mocked(mockSaveApiMessages).mock.invocationCallOrder[1]).toBeLessThan( vi.mocked(completionListener).mock.invocationCallOrder[0], @@ -657,9 +698,12 @@ describe("Task persistence", () => { }) const waiting = task.waitForCurrentAssistantMessagePersistence() + await vi.advanceTimersByTimeAsync(50) task.dispose() await expect(waiting).resolves.toBe(false) + await Promise.resolve() + expect(vi.getTimerCount()).toBe(0) await vi.runAllTimersAsync() expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) } finally { @@ -701,6 +745,7 @@ describe("Task persistence", () => { const waiting = task.waitForCurrentAssistantMessagePersistence() // Resolve the delay without flushing its promise continuation, then cancel at the save boundary. + await Promise.resolve() vi.advanceTimersByTime(100) task.dispose() @@ -712,6 +757,39 @@ describe("Task persistence", () => { vi.useRealTimers() } }) + + it("does not mark persistence ready when cancelled during a retry write", async () => { + vi.useFakeTimers() + const retrySave = createDeferred() + mockSaveApiMessages + .mockRejectedValueOnce(new Error("initial write failed")) + .mockReturnValueOnce(retrySave.promise) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + await getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "assistant", + content: [{ type: "text", text: "message" }], + }) + const waiting = task.waitForCurrentAssistantMessagePersistence() + + await vi.advanceTimersByTimeAsync(100) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) + task.dispose() + retrySave.resolve(undefined) + + await expect(waiting).resolves.toBe(false) + expect(task.assistantMessageSavedToHistory).toBe(false) + } finally { + retrySave.resolve(undefined) + vi.useRealTimers() + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 37e228f887..4fb1587844 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -40,6 +40,7 @@ type TaskTestAccess = { saveClineMessages: () => Promise safeEnsureModelFetched: () => Promise addToApiConversationHistory: (message: unknown, reasoning?: string) => Promise + resetAssistantMessagePersistence: () => void } type TaskAskResult = Awaited> @@ -2109,6 +2110,7 @@ describe("Cline", () => { // Spy on emit method const emitSpy = vi.spyOn(task, "emit") + const persistenceWait = task.waitForCurrentAssistantMessagePersistence() // Mock the dispose method to avoid actual cleanup vi.spyOn(task, "dispose").mockImplementation(() => {}) @@ -2121,6 +2123,7 @@ describe("Cline", () => { // Verify TaskAborted event was emitted expect(emitSpy).toHaveBeenCalledWith("taskAborted") + await expect(persistenceWait).resolves.toBe(false) }) it("should be equivalent to clicking Cancel button functionality", async () => { @@ -3209,6 +3212,7 @@ describe("Cline", () => { mode: undefined, }) const safeSpy = vi.spyOn(getTaskTestAccess(task), "safeEnsureModelFetched") + const resetPersistenceSpy = vi.spyOn(getTaskTestAccess(task), "resetAssistantMessagePersistence") vi.spyOn(task, "attemptApiRequest").mockImplementation(() => { throw new Error("stop after model metadata fetch") }) @@ -3240,6 +3244,7 @@ describe("Cline", () => { expect(result).toBe(true) expect(safeSpy).toHaveBeenCalled() + expect(resetPersistenceSpy).toHaveBeenCalled() expect(ensureModelFetched).toHaveBeenCalled() expect(task.cachedStreamingModel?.id).toBe(mockApiConfig.apiModelId) }) diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index f9252d054f..eff3a4e1af 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -215,7 +215,11 @@ 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) { - await this.emitPublicTaskCompleted(task) + try { + await this.emitPublicTaskCompleted(task) + } catch (error) { + await handleError("persisting task completion", error as Error) + } } return } diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index fb73c08390..2d12396474 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -922,6 +922,35 @@ describe("attemptCompletionTool", () => { ) }) + it("reports accepted-completion persistence failures with persistence context", async () => { + const persistenceError = new Error("history write failed") + const block: AttemptCompletionToolUse = { + type: "tool_use", + name: "attempt_completion", + params: { result: "2" }, + nativeArgs: { result: "2" }, + partial: false, + } + mockTask.ask = vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + mockTask.waitForCurrentAssistantMessagePersistence = vi.fn().mockRejectedValue(persistenceError) + + await attemptCompletionTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + }) + + expect(mockHandleError).toHaveBeenCalledWith("persisting task completion", persistenceError) + expect(mockTask.emit).not.toHaveBeenCalledWith( + RooCodeEventName.TaskCompleted, + expect.anything(), + expect.anything(), + expect.anything(), + ) + }) + it("reports telemetry but does not emit the public TaskCompleted event when user provides follow-up feedback", async () => { const block: AttemptCompletionToolUse = { type: "tool_use", 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 7018f59880..d2a61e58bb 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -8,6 +8,11 @@ vi.mock("vscode") vi.mock("../../core/webview/ClineProvider") describe("API#getTaskApiConversationHistoryLength", () => { + const expectedSequence = { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + } let api: API let mockOutputChannel: vscode.OutputChannel let mockProvider: ClineProvider @@ -78,6 +83,126 @@ describe("API#getTaskApiConversationHistoryLength", () => { ).resolves.toBe(false) }) + it.each([ + ["has no matching user text", [{ role: "user", content: [{ type: "text", text: "different" }] }]], + [ + "finds the text on an assistant turn", + [{ role: "assistant", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }], + ], + ["stores non-array user content", [{ role: "user", content: "RESTART_PERSISTENCE_SMOKE" }]], + ] as const)("returns false when history %s", async (_name, apiConversationHistory) => { + mockGetTaskWithId.mockResolvedValue({ apiConversationHistory }) + + await expect(api.hasTaskApiConversationHistorySequence("task-1", expectedSequence)).resolves.toBe(false) + }) + + it.each([ + [ + "matching text belongs to an assistant", + { role: "assistant", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + ], + ["the user text does not match", { role: "user", content: [{ type: "text", text: "different" }] }], + [ + "matching text is on a non-text user block", + { role: "user", content: [{ type: "image", text: "RESTART_PERSISTENCE_SMOKE" }] }, + ], + ] as const)("does not use a false user match when %s", async (_name, invalidUserCandidate) => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + invalidUserCandidate, + { + role: "assistant", + content: [ + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "done" } }, + ], + }, + ], + }) + + await expect(api.hasTaskApiConversationHistorySequence("task-1", expectedSequence)).resolves.toBe(false) + }) + + it("accepts matching text among mixed user blocks", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { + role: "user", + content: [ + { type: "image", source: { type: "base64", media_type: "image/png", data: "data" } }, + { type: "text", text: "RESTART_PERSISTENCE_SMOKE" }, + ], + }, + { + role: "assistant", + content: [ + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "done" } }, + ], + }, + ], + }) + + await expect(api.hasTaskApiConversationHistorySequence("task-1", expectedSequence)).resolves.toBe(true) + }) + + it.each([ + [ + "matching tool data on a user turn", + { + role: "user", + content: [ + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "done" } }, + ], + }, + ], + ["non-array assistant content", { role: "assistant", content: "attempt_completion done" }], + [ + "matching fields on a non-tool block", + { role: "assistant", content: [{ type: "text", text: "done", name: "attempt_completion", input: "done" }] }, + ], + [ + "the wrong tool name", + { + role: "assistant", + content: [{ type: "tool_use", id: "completion", name: "other", input: { result: "done" } }], + }, + ], + [ + "the wrong tool input", + { + role: "assistant", + content: [ + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "other" } }, + ], + }, + ], + ] as const)("returns false for %s after the expected user turn", async (_name, assistantCandidate) => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + assistantCandidate, + ], + }) + + await expect(api.hasTaskApiConversationHistorySequence("task-1", expectedSequence)).resolves.toBe(false) + }) + + it("accepts a later matching assistant turn after unrelated history", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { role: "assistant", content: [{ type: "text", text: "working" }] }, + { + role: "assistant", + content: [ + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "done" } }, + ], + }, + ], + }) + + await expect(api.hasTaskApiConversationHistorySequence("task-1", expectedSequence)).resolves.toBe(true) + }) + it("rejects an assistant completion that does not follow the expected user turn", async () => { mockGetTaskWithId.mockResolvedValue({ apiConversationHistory: [ diff --git a/src/extension/api.ts b/src/extension/api.ts index 7a173cea1c..c0f3d49d89 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -257,32 +257,35 @@ export class API extends EventEmitter implements RooCodeAPI { taskId: string, sequence: TaskApiConversationHistorySequence, ): Promise { + let apiConversationHistory: Awaited>["apiConversationHistory"] 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), - ), - ) + const task = await this.sidebarProvider.getTaskWithId(taskId) + apiConversationHistory = task.apiConversationHistory } catch { return false } + + 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), + ), + ) } public getCurrentTaskStack() { From cf6e1956741098a8a0aa1efd3902cc931132004f Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 13:56:02 +0000 Subject: [PATCH 14/22] refactor(task): remove duplicate cancellation branches --- src/core/task/Task.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index a4727fa049..f5d329f11e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1073,11 +1073,8 @@ export class Task extends EventEmitter implements TaskLike { const currentPersistence = this.assistantMessagePersistencePromise const currentCancellation = this.assistantMessagePersistenceCancellation! this.completionPersistenceReadyPromise = (async () => { - const result = await Promise.race([ - currentPersistence, - currentCancellation.promise.then(() => ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED), - ]) - if (result === ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED) return false + const result = await Promise.race([currentPersistence, currentCancellation.promise]) + if (currentCancellation.cancelled) return false if (result) return true const retryResult = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) @@ -1212,14 +1209,13 @@ export class Task extends EventEmitter implements TaskLike { for (let attempt = 0; attempt < delays.length; attempt++) { if (cancellation) { - const delayCompleted = await new Promise((resolve) => { - const timer = setTimeout(() => resolve(true), delays[attempt]) + await new Promise((resolve) => { + const timer = setTimeout(resolve, delays[attempt]) void cancellation.promise.then(() => { clearTimeout(timer) - resolve(false) + resolve() }) }) - if (!delayCompleted) return ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED } else { await new Promise((resolve) => setTimeout(resolve, delays[attempt])) } From 61ad993d7c0f0efa66794d74eae03c93cfd39032 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:04:23 +0000 Subject: [PATCH 15/22] test(task): cover same-turn persistence cancellation --- .../task/__tests__/Task.persistence.spec.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 5a4b729fe4..3d6151b0e6 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -24,6 +24,8 @@ import type { AttemptCompletionToolUse } from "../../../shared/tools" type TaskPersistenceAccess = { addToApiConversationHistory: (message: Anthropic.MessageParam) => Promise resetAssistantMessagePersistence: () => void + resolveAssistantMessagePersistence: (result: boolean) => void + assistantMessagePersistenceCancellation?: { resolve: () => void } resumeTaskFromHistory: () => Promise resumePendingTaskAction: (action: PendingTaskAction) => Promise saveClineMessages: () => Promise @@ -435,6 +437,22 @@ describe("Task persistence", () => { await expect(first).resolves.toBe(false) }) + it("lets same-turn cancellation win over a successful persistence result", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const privateTask = getTaskPersistenceAccess(task) + const waiting = task.waitForCurrentAssistantMessagePersistence() + + privateTask.resolveAssistantMessagePersistence(true) + privateTask.assistantMessagePersistenceCancellation?.resolve() + + await expect(waiting).resolves.toBe(false) + }) + it("emits TaskCompleted only after API history persistence succeeds", async () => { const saveDeferred = createDeferred() mockSaveApiMessages.mockReturnValueOnce(saveDeferred.promise) From 48e375b591bacdf5373dc123779a10f807a2f8a4 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:21:32 +0000 Subject: [PATCH 16/22] fix(task): suppress completion after durable cancellation --- src/core/task/Task.ts | 4 +- .../task/__tests__/Task.persistence.spec.ts | 10 ++-- src/core/tools/AttemptCompletionTool.ts | 6 ++- .../__tests__/attemptCompletionTool.spec.ts | 52 +++++++++++++++++++ 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index f5d329f11e..c04a0f74ad 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1069,9 +1069,9 @@ export class Task extends EventEmitter implements TaskLike { * A public completion event must not be emitted before this boundary succeeds. */ public waitForCurrentAssistantMessagePersistence(): Promise { + const currentCancellation = this.assistantMessagePersistenceCancellation! if (!this.completionPersistenceReadyPromise) { const currentPersistence = this.assistantMessagePersistencePromise - const currentCancellation = this.assistantMessagePersistenceCancellation! this.completionPersistenceReadyPromise = (async () => { const result = await Promise.race([currentPersistence, currentCancellation.promise]) if (currentCancellation.cancelled) return false @@ -1087,7 +1087,7 @@ export class Task extends EventEmitter implements TaskLike { })() } - return this.completionPersistenceReadyPromise + return this.completionPersistenceReadyPromise.then((ready) => ready && !currentCancellation.cancelled) } // NOTE: We intentionally do NOT mutate stored messages to merge consecutive user turns. diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 3d6151b0e6..6cc39131ca 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -421,20 +421,18 @@ describe("Task persistence", () => { expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) }) - it("shares one completion persistence result per assistant generation", async () => { + it("invalidates a cached successful persistence result when the generation is disposed", async () => { const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", startTask: false, }) + await getTaskPersistenceAccess(task).addToApiConversationHistory({ role: "assistant", content: "done" }) - const first = task.waitForCurrentAssistantMessagePersistence() - const second = task.waitForCurrentAssistantMessagePersistence() - - expect(second).toBe(first) + await expect(task.waitForCurrentAssistantMessagePersistence()).resolves.toBe(true) task.dispose() - await expect(first).resolves.toBe(false) + await expect(task.waitForCurrentAssistantMessagePersistence()).resolves.toBe(false) }) it("lets same-turn cancellation win over a successful persistence result", async () => { diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index eff3a4e1af..ed51bb569b 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -246,6 +246,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { * Returns: * - "delegated" when completion was approved and parent resumed * - "denied" when user denied finishing the subtask + * - "cancelled" when the persistence generation ended during approval * - "continue" when caller should fall through to normal completion ask flow */ private async delegateToParent( @@ -255,13 +256,16 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { pendingActionId: string | undefined, askFinishSubTaskApproval: () => Promise, pushToolResult: (result: string) => void, - ): Promise<"delegated" | "denied" | "continue"> { + ): Promise<"delegated" | "denied" | "cancelled" | "continue"> { const didApprove = await askFinishSubTaskApproval() if (!didApprove) { pushToolResult(formatResponse.toolDenied()) return "denied" } + if (!(await task.waitForCurrentAssistantMessagePersistence())) { + return "cancelled" + } const didReopen = await provider.reopenParentFromDelegation({ parentTaskId: task.parentTaskId!, diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 2d12396474..c544c3cafb 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -649,6 +649,58 @@ describe("attemptCompletionTool", () => { ) }) + it("does not reopen the parent when persistence is cancelled during approval", async () => { + 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() + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false), + }) + mockAskFinishSubTaskApproval.mockResolvedValue(true) + + await attemptCompletionTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + askFinishSubTaskApproval: mockAskFinishSubTaskApproval, + toolDescription: mockToolDescription, + toolCallId: "call-attempt-completion", + }) + + expect(mockTask.waitForCurrentAssistantMessagePersistence).toHaveBeenCalledTimes(2) + 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", From 8b89effac6d4d13d6f1245be3a0cae66e061af47 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:30:50 +0000 Subject: [PATCH 17/22] refactor(task): unify persistence cancellation results --- src/core/task/Task.ts | 16 ++++++------ .../task/__tests__/Task.persistence.spec.ts | 25 +++++++++++++++++++ src/core/tools/AttemptCompletionTool.ts | 6 ++--- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index c04a0f74ad..17ccfe5a34 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -194,8 +194,7 @@ export interface TaskOptions extends CreateTaskOptions { diffFuzzyThreshold?: number } -const ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED = Symbol() -type AssistantMessagePersistenceResult = boolean | typeof ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED +type AssistantMessagePersistenceResult = boolean type AssistantMessagePersistenceCancellation = { cancelled: boolean promise: Promise @@ -1074,12 +1073,11 @@ export class Task extends EventEmitter implements TaskLike { const currentPersistence = this.assistantMessagePersistencePromise this.completionPersistenceReadyPromise = (async () => { const result = await Promise.race([currentPersistence, currentCancellation.promise]) - if (currentCancellation.cancelled) return false if (result) return true - const retryResult = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) - if (retryResult === ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED) return false - if (!retryResult) { + const retrySaved = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) + if (!retrySaved) { + if (currentCancellation.cancelled) return false throw new Error("Failed to persist API conversation history before task completion") } this.assistantMessageSavedToHistory = true @@ -1198,7 +1196,7 @@ export class Task extends EventEmitter implements TaskLike { * Used by delegation flow when flushPendingToolResultsToHistory reports failure. */ public async retrySaveApiConversationHistory(): Promise { - return (await this.retrySaveApiConversationHistoryWithCancellation()) === true + return this.retrySaveApiConversationHistoryWithCancellation() } /** Retries API-history persistence while allowing the active assistant generation to cancel backoff. */ @@ -1221,14 +1219,14 @@ export class Task extends EventEmitter implements TaskLike { } // Check cancellation before each save attempt - if (cancellation?.cancelled) return ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED + if (cancellation?.cancelled) return false console.warn( `[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`, ) const success = await this.saveApiConversationHistory() - if (cancellation?.cancelled) return ASSISTANT_MESSAGE_PERSISTENCE_CANCELLED + if (cancellation?.cancelled) return false if (success) { return true diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 6cc39131ca..992d9e47f3 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -435,6 +435,31 @@ describe("Task persistence", () => { await expect(task.waitForCurrentAssistantMessagePersistence()).resolves.toBe(false) }) + it("shares one retry operation across concurrent persistence waiters", async () => { + vi.useFakeTimers() + mockSaveApiMessages + .mockRejectedValueOnce(new Error("initial write failed")) + .mockResolvedValueOnce(undefined) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + await getTaskPersistenceAccess(task).addToApiConversationHistory({ role: "assistant", content: "done" }) + const first = task.waitForCurrentAssistantMessagePersistence() + const second = task.waitForCurrentAssistantMessagePersistence() + + await vi.runAllTimersAsync() + await expect(Promise.all([first, second])).resolves.toEqual([true, true]) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) + it("lets same-turn cancellation win over a successful persistence result", async () => { const task = new Task({ provider: mockProvider, diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index ed51bb569b..066e6aa29a 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -246,7 +246,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { * Returns: * - "delegated" when completion was approved and parent resumed * - "denied" when user denied finishing the subtask - * - "cancelled" when the persistence generation ended during approval + * - undefined when the persistence generation ended during approval * - "continue" when caller should fall through to normal completion ask flow */ private async delegateToParent( @@ -256,7 +256,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { pendingActionId: string | undefined, askFinishSubTaskApproval: () => Promise, pushToolResult: (result: string) => void, - ): Promise<"delegated" | "denied" | "cancelled" | "continue"> { + ): Promise<"delegated" | "denied" | "continue" | undefined> { const didApprove = await askFinishSubTaskApproval() if (!didApprove) { @@ -264,7 +264,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { return "denied" } if (!(await task.waitForCurrentAssistantMessagePersistence())) { - return "cancelled" + return } const didReopen = await provider.reopenParentFromDelegation({ From 61465dd0a608c31bdae2cb0156ebb28ce8be0662 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:39:40 +0000 Subject: [PATCH 18/22] refactor(task): derive readiness from generation state --- src/core/task/Task.ts | 13 +++++++------ src/core/task/__tests__/Task.persistence.spec.ts | 1 + 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 17ccfe5a34..41393a86b8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -415,7 +415,7 @@ export class Task extends EventEmitter implements TaskLike { private assistantMessagePersistencePromise!: Promise private resolveAssistantMessagePersistence!: (result: AssistantMessagePersistenceResult) => void private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation - private completionPersistenceReadyPromise?: Promise + private completionPersistenceReadyPromise?: Promise /** * Fire-and-forget wrapper around `presentAssistantMessage` that swallows the @@ -1073,19 +1073,20 @@ export class Task extends EventEmitter implements TaskLike { const currentPersistence = this.assistantMessagePersistencePromise this.completionPersistenceReadyPromise = (async () => { const result = await Promise.race([currentPersistence, currentCancellation.promise]) - if (result) return true + if (result) return const retrySaved = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) if (!retrySaved) { - if (currentCancellation.cancelled) return false - throw new Error("Failed to persist API conversation history before task completion") + if (!currentCancellation.cancelled) { + throw new Error("Failed to persist API conversation history before task completion") + } + return } this.assistantMessageSavedToHistory = true - return true })() } - return this.completionPersistenceReadyPromise.then((ready) => ready && !currentCancellation.cancelled) + return this.completionPersistenceReadyPromise.then(() => !currentCancellation.cancelled) } // NOTE: We intentionally do NOT mutate stored messages to merge consecutive user turns. diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 992d9e47f3..894715a963 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -791,6 +791,7 @@ describe("Task persistence", () => { task.dispose() await expect(waiting).resolves.toBe(false) + expect(task.assistantMessageSavedToHistory).toBe(false) await vi.runAllTimersAsync() expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) From 563b399f130e41ff4f7e72cd443b445531a9ed68 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:13:03 +0000 Subject: [PATCH 19/22] test(formal): compose persistence into model check --- .github/workflows/code-qa.yml | 2 +- AGENTS.md | 2 +- docs/architecture/task-lifecycle-model.md | 8 +++++--- package.json | 3 +-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/.github/workflows/code-qa.yml b/.github/workflows/code-qa.yml index b4e5881f6b..ae0e520d9b 100644 --- a/.github/workflows/code-qa.yml +++ b/.github/workflows/code-qa.yml @@ -91,7 +91,7 @@ jobs: - name: Check types run: pnpm check-types - name: Model-check task lifecycle - run: pnpm lifecycle:model + run: pnpm model-check build-vsix: name: Build test VSIX diff --git a/AGENTS.md b/AGENTS.md index e14148e1be..68170dd603 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,7 @@ Prefer the narrowest test layer that proves the behavior. This follows standard ## Task Lifecycle Changes - Read `docs/architecture/task-lifecycle-model.md` before changing task status, delegation, interruption, completion, abandonment, persistence ownership, or scheduler fan-out behavior. -- Keep lifecycle mutations in the shared reducers under `src/core/task-persistence/taskLifecycle.ts`. Update model actions, invariants, or named semantic landmarks for every new transition or concurrency bug class representable in the lifecycle model, then run `pnpm lifecycle:model`. +- Keep lifecycle mutations in the shared reducers under `src/core/task-persistence/taskLifecycle.ts`. Update model actions, invariants, or named semantic landmarks for every new transition or concurrency bug class representable in the lifecycle model, then run `pnpm model-check`. - Add extension-host E2E coverage only for a boundary the reducer model cannot prove, such as restart visibility, real persistence/rehydration, delayed provider streams, scheduler permits, or webview task scoping. Do not duplicate reducer interleavings in E2E. - Run the focused lifecycle tests and `pnpm test` before completing a Zoo Code lifecycle change. diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 36339096da..727cbbb4d6 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -3,10 +3,12 @@ Zoo Code checks its persisted task delegation lifecycle with a bounded, exhaustive state explorer. Run it locally with: ```sh -pnpm lifecycle:model +pnpm model-check ``` -The check runs in the `compile` CI job after type checking. It fails if it finds an invariant violation, a modeled action becomes unreachable, a named semantic landmark disappears, or exploration exceeds its declared state budget. A violation includes the shortest breadth-first event trace, every intermediate state, and the active bounds so the sequence can be replayed as a focused regression test. The command generates no checked-in artifacts; its reports and counterexamples are written to standard output. `pnpm lifecycle:model-check` remains as a compatibility alias. +The check runs in the `compile` CI job after type checking. It fails if it finds an invariant violation, a modeled action becomes unreachable, a named semantic landmark disappears, or exploration exceeds its declared state budget. A violation includes the shortest breadth-first event trace, every intermediate state, and the active bounds so the sequence can be replayed as a focused regression test. The command generates no checked-in artifacts; its reports and counterexamples are written to standard output. + +`pnpm model-check` preserves the task lifecycle and shared-store checkers with their original state spaces, actions, invariants, landmarks, and reports, then appends the completion persistence explorer to the same command and output stream. ## Why an executable TypeScript model @@ -39,7 +41,7 @@ Production completion also accepts a recovery-compatible `active` parent that st ## Shared-store concurrency model -The same `pnpm lifecycle:model` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: +The same `pnpm model-check` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: - each host has an independent cache and host-local mutex; - store read/update operations hold the host mutex, while live-task snapshots used by completion and message saves may outlive it; diff --git a/package.json b/package.json index 9763a00b1b..685a573db6 100644 --- a/package.json +++ b/package.json @@ -13,8 +13,7 @@ "check-types": "turbo check-types --log-order grouped --output-logs new-only", "test": "turbo test --log-order grouped --output-logs new-only", "test:mutation-ci": "node --test scripts/stryker-diff.test.mjs", - "lifecycle:model": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-completion-persistence.ts", - "lifecycle:model-check": "pnpm lifecycle:model", + "model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-completion-persistence.ts", "test:coverage": "turbo test:coverage --log-order grouped --output-logs new-only", "format": "turbo format --log-order grouped --output-logs new-only", "build": "turbo build --log-order grouped --output-logs new-only", From ee8fdaa29990ca037fb41eaf95aef88b04ed251f Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:29:59 +0000 Subject: [PATCH 20/22] fix(api): emit delegated completion after child disposal --- src/core/tools/AttemptCompletionTool.ts | 12 ++++++++++- .../__tests__/attemptCompletionTool.spec.ts | 10 +++++++++ src/core/webview/ClineProvider.ts | 5 +++++ .../ClineProvider.taskHistory.spec.ts | 10 +++++++++ ...i-task-conversation-history-length.spec.ts | 19 ++++++++++++++++- src/extension/api.ts | 21 ++++++++++--------- 6 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index 066e6aa29a..4fe03ce94c 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -29,6 +29,11 @@ interface DelegationProvider { getTaskWithId(id: string): Promise<{ historyItem: HistoryItem }> setPendingTaskAction(taskId: string, pendingAction: PendingTaskAction): Promise clearPendingTaskAction(taskId: string, actionId: string): Promise + emitDelegatedTaskCompleted( + taskId: string, + tokenUsage: ReturnType, + toolUsage: Task["toolUsage"], + ): void reopenParentFromDelegation(params: { parentTaskId: string childTaskId: string @@ -159,7 +164,12 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { pushToolResult, ) if (delegation === "delegated") { - await this.emitPublicTaskCompleted(task) + task.emitFinalTokenUsageUpdate() + provider.emitDelegatedTaskCompleted( + task.taskId, + task.getTokenUsage(), + task.toolUsage, + ) } if (delegation !== "continue") return } else { diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index c544c3cafb..60c7fccc9e 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -506,6 +506,7 @@ describe("attemptCompletionTool", () => { setPendingTaskAction: vi.fn().mockResolvedValue(undefined), clearPendingTaskAction: vi.fn().mockResolvedValue(true), reopenParentFromDelegation: vi.fn().mockResolvedValue(true), + emitDelegatedTaskCompleted: vi.fn(), } Object.assign(mockTask, { @@ -548,6 +549,13 @@ describe("attemptCompletionTool", () => { }) expect(mockTask.ask).not.toHaveBeenCalled() expect(mockPushToolResult).toHaveBeenCalledWith("") + expect(mockProvider.emitDelegatedTaskCompleted).toHaveBeenCalledTimes(1) + expect(mockTask.emit).not.toHaveBeenCalledWith( + RooCodeEventName.TaskCompleted, + expect.anything(), + expect.anything(), + expect.anything(), + ) }) it("does not delegate or emit completion when child history persistence fails", async () => { @@ -832,6 +840,7 @@ describe("attemptCompletionTool", () => { throw new Error(`unexpected task id ${id}`) }), reopenParentFromDelegation: vi.fn().mockResolvedValue(true), + emitDelegatedTaskCompleted: vi.fn(), } Object.assign(mockTask, { @@ -859,6 +868,7 @@ describe("attemptCompletionTool", () => { }) expect(mockTask.ask).not.toHaveBeenCalled() expect(mockPushToolResult).toHaveBeenCalledWith("") + expect(mockProvider.emitDelegatedTaskCompleted).toHaveBeenCalledTimes(1) }) it("does not resume the parent when the parent is active but awaiting a different child", async () => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0a251aba5f..5674a9f2f0 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4262,6 +4262,11 @@ export class ClineProvider }) } + /** Emits completion after delegated child disposal through the provider-owned event channel. */ + public emitDelegatedTaskCompleted(taskId: string, tokenUsage: TokenUsage, toolUsage: ToolUsage): void { + this.emit(RooCodeEventName.TaskCompleted, taskId, tokenUsage, toolUsage) + } + /** * Explicitly sever a delegated parent-child link, e.g. when the user gives up on * an "interrupted" subtask instead of resuming it. Unlike removeClineFromStack()'s diff --git a/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts b/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts index fe1eac8e20..0365283222 100644 --- a/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts @@ -846,5 +846,15 @@ describe("ClineProvider Task History Synchronization", () => { expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[onTaskCompleted] Failed to write")) }) + + it("emits delegated completion through the provider after the child is disposed", () => { + const listener = vi.fn() + provider.on(RooCodeEventName.TaskCompleted, listener) + + provider.emitDelegatedTaskCompleted("child-task", {} as never, {}) + + expect(listener).toHaveBeenCalledTimes(1) + expect(listener).toHaveBeenCalledWith("child-task", {}, {}) + }) }) }) 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 d2a61e58bb..45c9237a61 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi, beforeEach } from "vitest" import * as vscode from "vscode" +import { RooCodeEventName } from "@roo-code/types" import { API } from "../api" import { ClineProvider } from "../../core/webview/ClineProvider" @@ -17,6 +18,7 @@ describe("API#getTaskApiConversationHistoryLength", () => { let mockOutputChannel: vscode.OutputChannel let mockProvider: ClineProvider let mockGetTaskWithId: ReturnType + let providerListeners: Map unknown> beforeEach(() => { mockOutputChannel = { @@ -24,11 +26,15 @@ describe("API#getTaskApiConversationHistoryLength", () => { } as unknown as vscode.OutputChannel mockGetTaskWithId = vi.fn() + providerListeners = new Map() mockProvider = { context: {} as vscode.ExtensionContext, getTaskWithId: mockGetTaskWithId, - on: vi.fn(), + taskHistoryStore: { get: vi.fn() }, + on: vi.fn((event: string, listener: (...args: unknown[]) => unknown) => { + providerListeners.set(event, listener) + }), } as unknown as ClineProvider api = new API(mockOutputChannel, mockProvider, undefined, true) @@ -48,6 +54,17 @@ describe("API#getTaskApiConversationHistoryLength", () => { await expect(api.getTaskApiConversationHistoryLength("missing-task")).resolves.toBe(0) }) + it("forwards provider completion exactly once after a delegated child is disposed", async () => { + vi.mocked(mockProvider.taskHistoryStore.get).mockReturnValue({ parentTaskId: "parent-1" } as never) + const listener = vi.fn() + api.on(RooCodeEventName.TaskCompleted, listener) + + await providerListeners.get(RooCodeEventName.TaskCompleted)?.("child-1", {}, {}) + + expect(listener).toHaveBeenCalledTimes(1) + expect(listener).toHaveBeenCalledWith("child-1", {}, {}, { isSubtask: true }) + }) + it("finds the expected persisted user and assistant turns in order", async () => { mockGetTaskWithId.mockResolvedValue({ apiConversationHistory: [ diff --git a/src/extension/api.ts b/src/extension/api.ts index c0f3d49d89..cad44f08ba 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -375,6 +375,17 @@ export class API extends EventEmitter implements RooCodeAPI { } private registerListeners(provider: ClineProvider) { + provider.on(RooCodeEventName.TaskCompleted, async (taskId, tokenUsage, toolUsage) => { + const historyItem = provider.taskHistoryStore.get(taskId) + this.emit(RooCodeEventName.TaskCompleted, taskId, tokenUsage, toolUsage, { + isSubtask: !!historyItem?.parentTaskId, + }) + + await this.fileLog( + `[${new Date().toISOString()}] taskCompleted -> ${taskId} | ${JSON.stringify(tokenUsage, null, 2)} | ${JSON.stringify(toolUsage, null, 2)}\n`, + ) + }) + provider.on(RooCodeEventName.TaskCreated, (task) => { // Task Lifecycle @@ -383,16 +394,6 @@ export class API extends EventEmitter implements RooCodeAPI { await this.fileLog(`[${new Date().toISOString()}] taskStarted -> ${task.taskId}\n`) }) - task.on(RooCodeEventName.TaskCompleted, async (_, tokenUsage, toolUsage) => { - this.emit(RooCodeEventName.TaskCompleted, task.taskId, tokenUsage, toolUsage, { - isSubtask: !!task.parentTaskId, - }) - - await this.fileLog( - `[${new Date().toISOString()}] taskCompleted -> ${task.taskId} | ${JSON.stringify(tokenUsage, null, 2)} | ${JSON.stringify(toolUsage, null, 2)}\n`, - ) - }) - task.on(RooCodeEventName.TaskAborted, () => { this.emit(RooCodeEventName.TaskAborted, task.taskId) }) From d0e177d8efa0dbd35836e3ff5fe1eae121969f83 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:34:05 +0000 Subject: [PATCH 21/22] fix(api): emit delegated completion after child disposal --- src/__tests__/nested-delegation-resume.spec.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/__tests__/nested-delegation-resume.spec.ts b/src/__tests__/nested-delegation-resume.spec.ts index dd015e93cf..9b06ad4162 100644 --- a/src/__tests__/nested-delegation-resume.spec.ts +++ b/src/__tests__/nested-delegation-resume.spec.ts @@ -178,6 +178,9 @@ describe("Nested delegation resume (A → B → C)", () => { createTaskWithHistoryItem, updateTaskHistory, taskHistoryStore, + emitDelegatedTaskCompleted: vi.fn((taskId, tokenUsage, toolUsage) => { + ClineProvider.prototype.emitDelegatedTaskCompleted.call(provider, taskId, tokenUsage, toolUsage) + }), // Wire through provider method so attemptCompletionTool can call it reopenParentFromDelegation: vi.fn(async (params: any) => { return await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, params) @@ -285,8 +288,10 @@ describe("Nested delegation resume (A → B → C)", () => { (c: any[]) => c[0] === RooCodeEventName.TaskDelegationCompleted, ) const resumedEvents = emitSpy.mock.calls.filter((c: any[]) => c[0] === RooCodeEventName.TaskDelegationResumed) + const taskCompletedEvents = emitSpy.mock.calls.filter((call) => call[0] === RooCodeEventName.TaskCompleted) expect(completedEvents.length).toBeGreaterThanOrEqual(2) expect(resumedEvents.length).toBeGreaterThanOrEqual(2) + expect(taskCompletedEvents).toHaveLength(2) // Verify second hop used parentId = A // Find a TaskDelegationCompleted matching A <- B From 7799792207ab77ccfb376c0e76440955eb5ebf69 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:43:25 +0000 Subject: [PATCH 22/22] test(api): cover delegated completion forwarding --- .../tools/__tests__/attemptCompletionTool.spec.ts | 1 + .../api-task-conversation-history-length.spec.ts | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 60c7fccc9e..4bf3a89f34 100644 --- a/src/core/tools/__tests__/attemptCompletionTool.spec.ts +++ b/src/core/tools/__tests__/attemptCompletionTool.spec.ts @@ -549,6 +549,7 @@ describe("attemptCompletionTool", () => { }) expect(mockTask.ask).not.toHaveBeenCalled() expect(mockPushToolResult).toHaveBeenCalledWith("") + expect(mockTask.emitFinalTokenUsageUpdate).toHaveBeenCalledTimes(1) expect(mockProvider.emitDelegatedTaskCompleted).toHaveBeenCalledTimes(1) expect(mockTask.emit).not.toHaveBeenCalledWith( RooCodeEventName.TaskCompleted, 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 45c9237a61..6568d77143 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -57,12 +57,26 @@ describe("API#getTaskApiConversationHistoryLength", () => { it("forwards provider completion exactly once after a delegated child is disposed", async () => { vi.mocked(mockProvider.taskHistoryStore.get).mockReturnValue({ parentTaskId: "parent-1" } as never) const listener = vi.fn() + const fileLog = vi + .spyOn(api as unknown as { fileLog: (message: string) => Promise }, "fileLog") + .mockResolvedValue(undefined) api.on(RooCodeEventName.TaskCompleted, listener) await providerListeners.get(RooCodeEventName.TaskCompleted)?.("child-1", {}, {}) expect(listener).toHaveBeenCalledTimes(1) expect(listener).toHaveBeenCalledWith("child-1", {}, {}, { isSubtask: true }) + expect(fileLog).toHaveBeenCalledWith(expect.stringContaining("taskCompleted -> child-1")) + }) + + it("forwards provider completion for a task absent from local history", async () => { + vi.mocked(mockProvider.taskHistoryStore.get).mockReturnValue(undefined) + const listener = vi.fn() + api.on(RooCodeEventName.TaskCompleted, listener) + + await providerListeners.get(RooCodeEventName.TaskCompleted)?.("task-1", {}, {}) + + expect(listener).toHaveBeenCalledWith("task-1", {}, {}, { isSubtask: false }) }) it("finds the expected persisted user and assistant turns in order", async () => {