From a77af15dac4a406a2271039c4260d34df654cfcd Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 14:16:31 +0000 Subject: [PATCH 01/29] 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 0cbee54352d103d672324c6601bdb1e43fd1107f Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 15:12:25 +0000 Subject: [PATCH 02/29] 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 9956b74fb7..6d7ed62ee3 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 @@ -415,6 +423,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 a5ee06eaa0a309103d9c5164d723fb7b9228cf26 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 18:27:23 +0000 Subject: [PATCH 03/29] 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 1f03e39e85..b6ae97da20 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, @@ -92,7 +87,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 api.resumeTask(taskId) await waitFor(() => taskMessages.some(({ type, ask }) => type === "ask" && ask === "resume_completed_task")) 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 a99ad2ca22..d92d95a5af 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1535,6 +1535,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 543084571d..3aec31a614 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -406,9 +406,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 @@ -515,6 +518,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") @@ -984,7 +988,7 @@ export class Task extends EventEmitter implements TaskLike { return ensureMessageIdentifiers(messages) } - private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string) { + private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string): Promise { const resolvesPendingAction = this.pendingAction && message.role === "user" && @@ -1016,6 +1020,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. @@ -3049,6 +3086,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") @@ -3821,7 +3859,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 6d7ed62ee3..ac773bc076 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -424,9 +424,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) @@ -440,39 +438,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) @@ -498,16 +466,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 2e7d094bcf0645dc1eefcbe4e23b4b8c58fd7a98 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 18:32:59 +0000 Subject: [PATCH 04/29] 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 b6ae97da20..2c35d7a4ac 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -87,7 +87,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 api.resumeTask(taskId) await waitFor(() => taskMessages.some(({ type, ask }) => type === "ask" && ask === "resume_completed_task")) From b70f20c5e814aa6fd6b8917dfde469393afc300c Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 03:03:16 +0000 Subject: [PATCH 05/29] 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 2c35d7a4ac..29f94de35d 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -86,10 +86,15 @@ 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 api.resumeTask(taskId) @@ -110,7 +115,7 @@ async function runVerify(api: RooCodeAPI): Promise { 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 3aec31a614..df538e64c7 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -988,6 +988,7 @@ export class Task extends EventEmitter implements TaskLike { return ensureMessageIdentifiers(messages) } + /** 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 && @@ -1026,6 +1027,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 ac773bc076..a15f62f3f2 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -503,6 +503,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 8057e7642dd9ef9a33fa51a0b38ae5fa1ac0c0b4 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 21:32:31 +0000 Subject: [PATCH 06/29] 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 d92d95a5af..eed8127b82 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1535,7 +1535,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 df538e64c7..29e5c47eac 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -195,6 +195,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 @@ -409,9 +411,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 @@ -1023,34 +1027,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 })() } @@ -2547,6 +2571,7 @@ export class Task extends EventEmitter implements TaskLike { } this.abort = true + this.cancelAssistantMessagePersistence() this.abortPromise ??= this.abortTaskOnce() return this.abortPromise } @@ -2610,6 +2635,7 @@ export class Task extends EventEmitter implements TaskLike { private async disposeOnce(): Promise { 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 a15f62f3f2..66ed803a33 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 @@ -584,6 +585,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 22884ed03eb70b6df98125c999264672e6798e71 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 21:47:30 +0000 Subject: [PATCH 07/29] 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 29e5c47eac..dfe07c5176 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -196,6 +196,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 @@ -413,8 +418,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 /** @@ -1037,16 +1041,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() } /** @@ -1056,21 +1070,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 @@ -1192,22 +1203,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 66ed803a33..1efa0680cc 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -620,8 +620,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 2941ec55a9ca1300cc5ef1adcc75eafbd02a6522 Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 31 Aug 2026 22:16:20 +0000 Subject: [PATCH 08/29] 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 dfe07c5176..e59291c9b2 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1206,6 +1206,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 1efa0680cc..7bf06983e9 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -586,6 +586,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 b1c5711ae263be25fe1115a5ae6fc5307f71c743 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/29] 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 e59291c9b2..b40f3c619a 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -420,6 +420,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 @@ -937,6 +938,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 @@ -962,6 +967,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) { @@ -996,7 +1005,10 @@ export class Task extends EventEmitter implements TaskLike { return ensureMessageIdentifiers(messages) } - /** 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 && @@ -1059,6 +1071,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() } @@ -1096,6 +1112,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[], persist = true) { this.hydrateApiConversationHistory(newHistory) if (persist) { @@ -1182,6 +1199,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(merge = true): Promise { try { await saveApiMessages({ @@ -1213,15 +1231,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) @@ -1231,6 +1256,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}`, ) @@ -1248,10 +1277,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) { message.messageId ??= crypto.randomUUID() this.clineMessages.push(message) @@ -1286,6 +1320,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[], persist = true) { this.hydrateClineMessages(newMessages) if (persist) { @@ -1311,6 +1349,10 @@ export class Task extends EventEmitter implements TaskLike { this.apiConversationHistory = ensureMessageIdentifiers(messages) } + /** + * 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 }) @@ -1330,6 +1372,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(merge = true): 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 7bf06983e9..80749bbd0f 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -646,6 +646,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() @@ -707,6 +711,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 79dc9e6374cadc78b04932a3fe3acf94135dfdd1 Mon Sep 17 00:00:00 2001 From: Roomote Date: Tue, 1 Sep 2026 00:43:02 +0000 Subject: [PATCH 10/29] fix(task): keep persistence retry timers generation-local --- src/core/task/Task.ts | 18 ------------------ .../task/__tests__/Task.persistence.spec.ts | 7 +++---- src/core/tools/AttemptCompletionTool.ts | 4 ---- 3 files changed, 3 insertions(+), 26 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b40f3c619a..5bec3c8ac8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -420,7 +420,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 @@ -967,10 +966,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) { @@ -1071,10 +1066,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() } @@ -1240,13 +1231,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) @@ -1277,15 +1264,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) { message.messageId ??= crypto.randomUUID() this.clineMessages.push(message) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 80749bbd0f..af7ea757d2 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -712,7 +712,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) @@ -730,14 +730,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 993fcec94abfc5c212223670e4baf96fce0dac5f Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 03:22:23 +0000 Subject: [PATCH 11/29] test(formal): model completion persistence lifecycle --- .github/alloy/CompletionPersistence.als | 141 ------------ .github/alloy/README.md | 57 ----- AGENTS.md | 2 +- docs/architecture/task-lifecycle-model.md | 63 ++++-- package.json | 2 +- scripts/check-completion-persistence.ts | 248 ++++++++++++++++++++++ 6 files changed, 293 insertions(+), 220 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/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 79ecca0a6c..4a8e53e69b 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -3,25 +3,26 @@ Zoo Code checks task lifecycle protocols through one compositional verification suite. Run the complete suite locally with: ```sh -pnpm lifecycle:model-check +pnpm lifecycle:model ``` -The command runs four independent bounded submodels in sequence: +The command runs five independent bounded submodels in sequence: 1. the persisted task delegation lifecycle; 2. shared-store concurrency across task-history hosts; -3. the task cleanup protocol; and -4. request-stream parser scoping. +3. the task cleanup protocol; +4. request-stream parser scoping; and +5. completion persistence. This umbrella command is the single model-check entry point in the `compile` CI job after type checking. Command-level composition does not merge the submodels' state spaces: each checker retains its own bounds, transitions, invariant ownership, reachability requirements, and counterexample format. In particular, parser state is not part of the persisted lifecycle graph. The focused parser checker remains directly runnable with `pnpm parser-scope:model-check` for debugging. -An individual checker fails if it finds an invariant violation, a modeled action becomes unreachable, or exploration exceeds its declared state budget. A lifecycle 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. +An individual checker 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 lifecycle 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. Executable cross-model composition should be added only when a correctness claim genuinely spans two or more submodels and there is an explicit, production-grounded boundary mapping between their events or state. That composition must state a bounded joint exploration strategy and own cross-model invariants that cannot be proved within either child model alone. Shared command orchestration or conceptual adjacency is not sufficient reason to multiply independent state spaces. ## 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. @@ -50,7 +51,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; @@ -78,9 +79,24 @@ The known-unsafe witnesses currently compare exact shortest action sequences. Th The umbrella command also runs a separate bounded child model for in-memory abort, disposal, and provider-shutdown ordering. It models cleanup settlement and rejection as environment transitions and makes no filesystem, editor Promise, fairness, or timing-liveness claim. See [Task cleanup protocol model check](./task-cleanup-protocol-model.md). +## 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. @@ -90,22 +106,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. #1279 is resolved: `reopenParentFromDelegation` reads both UI and API history and saves them under the per-file advisory lock before the lifecycle transition, so the histories are durable before `TaskDelegationCompleted` fires. `restart-persistence.test.ts` is the controlled barrier test; 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. The [native tool-call parser request-scope model](./native-tool-call-parser-scoping-model.md), whose source of truth is `scripts/check-native-tool-call-parser-scoping.ts`, exhaustively replays bounded production-parser interleavings without adding fields to this lifecycle 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. The [native tool-call parser request-scope model](./native-tool-call-parser-scoping-model.md), whose source of truth is `scripts/check-native-tool-call-parser-scoping.ts`, exhaustively replays bounded production-parser interleavings without adding fields to this lifecycle 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. @@ -119,7 +142,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. Parser request scoping is one such independent bounded submodel within the umbrella suite. Extend `scripts/check-native-tool-call-parser-scoping.ts` and its focused architecture document instead of adding parser state or transitions to `taskLifecycle.ts` or the persisted lifecycle state graph. diff --git a/package.json b/package.json index 5671d40a66..94f2d52e27 100644 --- a/package.json +++ b/package.json @@ -13,7 +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-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check", + "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts", "cleanup-protocol:model-check": "tsx scripts/check-task-cleanup-protocol.ts", "parser-scope:model-check": "node scripts/run-native-tool-call-parser-scoping.mjs", "test:coverage": "turbo test:coverage --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 1d6f49fbfd3a7e0470fee7ecc959d85068fd186b Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 03:28:06 +0000 Subject: [PATCH 12/29] 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 4a8e53e69b..4e227f5ae2 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -25,7 +25,7 @@ Executable cross-model composition should be added only when a correctness claim 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. @@ -87,12 +87,13 @@ The umbrella command also runs a separate bounded child model for in-memory abor - 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 @@ -111,9 +112,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 cd3570028f7a03c7d76a0560b5730a3bcd58ecd6 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 13:46:38 +0000 Subject: [PATCH 13/29] test(task): satisfy changed-code mutation gate --- scripts/stryker-diff.mjs | 14 +- scripts/stryker-diff.test.mjs | 42 ++++++ 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, 337 insertions(+), 53 deletions(-) diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index 32211ebbd6..d8a5ba20f5 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) @@ -384,7 +392,7 @@ function runStryker(repoRoot, packageEntry, reportRoot, dryRunOnly) { STRYKER_REPORT_DIR: reportDirectory, STRYKER_TEMP_DIR: resolveStrykerTempDir(repoRoot, runRoot), 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 55f8debea9..f00c55f985 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -24,6 +24,7 @@ import { parseVitestTestFiles, resolveStrykerTempDir, resolveVitestBinary, + shouldUseVitestRelated, packageForPath, runManifest, selectFromGit, @@ -200,6 +201,47 @@ describe("parseVitestTestFiles", () => { }) }) +describe("preferDirectTestFiles", () => { + it("uses matching focused specs and falls back to all related tests", () => { + const related = [ + "webview-ui/src/__tests__/App.spec.tsx", + "webview-ui/src/utils/__tests__/path-mentions.test.ts", + "webview-ui/src/components/chat/__tests__/ChatView.spec.tsx", + ] + assert.deepEqual(preferDirectTestFiles(related, ["webview-ui/src/utils/path-mentions.ts"]), [ + "webview-ui/src/utils/__tests__/path-mentions.test.ts", + ]) + 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", () => { it("keeps Stryker's temp directory relative to each run root", () => { assert.equal(resolveStrykerTempDir("/repo", "/repo"), ".stryker-tmp") diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 5bec3c8ac8..c4b84ffb42 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -195,7 +195,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 @@ -1038,7 +1039,7 @@ export class Task extends EventEmitter implements TaskLike { } if (message.role === "assistant") { this.assistantMessageSavedToHistory = saved - this.resolveAssistantMessagePersistence(saved ? "saved" : "failed") + this.resolveAssistantMessagePersistence(saved) } } @@ -1055,7 +1056,6 @@ export class Task extends EventEmitter implements TaskLike { resolveCancellation = resolve }), resolve: () => { - if (cancellation.cancelled) return cancellation.cancelled = true resolveCancellation() }, @@ -1066,7 +1066,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() } @@ -1081,14 +1080,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 @@ -1212,7 +1211,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. */ @@ -1222,44 +1221,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 af7ea757d2..baa349bfae 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -425,6 +425,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) @@ -575,7 +615,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", }), @@ -646,6 +686,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], @@ -687,9 +728,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 { @@ -731,6 +775,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() @@ -742,6 +787,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 b3554292b1..264ea6ea80 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -41,6 +41,7 @@ type TaskTestAccess = { saveClineMessages: () => Promise safeEnsureModelFetched: () => Promise addToApiConversationHistory: (message: unknown, reasoning?: string) => Promise + resetAssistantMessagePersistence: () => void } type TaskAskResult = Awaited> @@ -2289,6 +2290,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").mockResolvedValue(undefined) @@ -2302,6 +2304,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 () => { @@ -3455,6 +3458,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") }) @@ -3486,6 +3490,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 b5afb17c3b99793de40ac026ec022f3eeaba9619 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 13:56:02 +0000 Subject: [PATCH 14/29] 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 c4b84ffb42..ca5e104fe1 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1078,11 +1078,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) @@ -1222,14 +1219,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 08337238ba314416c31870a0b96ca4e1789c35e8 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:04:23 +0000 Subject: [PATCH 15/29] 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 baa349bfae..304ca59200 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 @@ -465,6 +467,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 eee53b83b0f692e4465695d683c8c1c56a683324 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:21:32 +0000 Subject: [PATCH 16/29] 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 ca5e104fe1..9385a45049 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1074,9 +1074,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 @@ -1092,7 +1092,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 304ca59200..2d846498ae 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -451,20 +451,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 bf409c83946ea72cce6c4f017b68f1d22458149a Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:30:50 +0000 Subject: [PATCH 17/29] 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 9385a45049..fdc5679f8a 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -195,8 +195,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 @@ -1079,12 +1078,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 @@ -1208,7 +1206,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. */ @@ -1231,14 +1229,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 2d846498ae..5b44e99d42 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -465,6 +465,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 096595e8678a24494f055630e5b9bfa6c2c00590 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 14:39:40 +0000 Subject: [PATCH 18/29] 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 fdc5679f8a..1e643e4fe8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -419,7 +419,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 @@ -1078,19 +1078,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 5b44e99d42..adc1bdc7f4 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -821,6 +821,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 ec75de88f7752cfea2838b900d6abdce1dc6dc0a Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:13:03 +0000 Subject: [PATCH 19/29] test(formal): compose persistence into model check --- AGENTS.md | 2 +- docs/architecture/task-lifecycle-model.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 4e227f5ae2..eb25c11d63 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -3,7 +3,7 @@ Zoo Code checks task lifecycle protocols through one compositional verification suite. Run the complete suite locally with: ```sh -pnpm lifecycle:model +pnpm model-check ``` The command runs five independent bounded submodels in sequence: @@ -51,7 +51,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; From 9b6662121861c2e1b34abdc3f60af4ac7c73f2a9 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:29:59 +0000 Subject: [PATCH 20/29] 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 7f21a049e7..87a899344c 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -4298,6 +4298,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 b487d8e64c62153068352675cb4b8593d7583123 Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:34:05 +0000 Subject: [PATCH 21/29] 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 ea61cf117c409176c0e2b3881907a4a0c08f525a Mon Sep 17 00:00:00 2001 From: Roomote Date: Thu, 3 Sep 2026 22:43:25 +0000 Subject: [PATCH 22/29] 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 () => { From 09f025599b3e6b9c516ef1288b0a95a188ace078 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 02:33:53 +0000 Subject: [PATCH 23/29] fix(ci): preserve lifecycle model command --- AGENTS.md | 2 +- docs/architecture/task-lifecycle-model.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 68170dd603..3b5be80ede 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 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-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 eb25c11d63..d39c0fd9e2 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -3,7 +3,7 @@ Zoo Code checks task lifecycle protocols through one compositional verification suite. Run the complete suite locally with: ```sh -pnpm model-check +pnpm lifecycle:model-check ``` The command runs five independent bounded submodels in sequence: @@ -51,7 +51,7 @@ Production completion also accepts a recovery-compatible `active` parent that st ## Shared-store concurrency model -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: +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: - 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; From d5a18ac9b426009769070c981b6c2cd816ebde83 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 5 Sep 2026 00:32:09 +0000 Subject: [PATCH 24/29] fix(task): retry assistant persistence before tool results --- scripts/stryker-diff.mjs | 16 ++--- scripts/stryker-diff.test.mjs | 6 ++ src/core/task/Task.ts | 21 ++++--- .../task/__tests__/Task.persistence.spec.ts | 61 +++++++++++++++++++ src/core/task/__tests__/Task.spec.ts | 2 +- .../flushPendingToolResultsToHistory.spec.ts | 25 ++++---- ...i-task-conversation-history-length.spec.ts | 1 + 7 files changed, 104 insertions(+), 28 deletions(-) diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index d8a5ba20f5..a4c5ddcb80 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -296,16 +296,18 @@ export function preferDirectTestFiles(testFiles, sourceFiles) { const sourceNames = sourceFiles.map((sourceFile) => path.posix.basename(sourceFile, path.posix.extname(sourceFile)).toLowerCase(), ) - const direct = testFiles.filter((testFile) => { + const isDirectMatch = (testFile, sourceName) => { const testName = path.posix.basename(testFile) const normalizedTestName = testName.toLowerCase() - return sourceNames.some( - (sourceName) => - (normalizedTestName.startsWith(`${sourceName}.`) || normalizedTestName.startsWith(`${sourceName}-`)) && - /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), + return ( + (normalizedTestName.startsWith(`${sourceName}.`) || normalizedTestName.startsWith(`${sourceName}-`)) && + /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName) ) - }) - return direct.length > 0 ? direct : testFiles + } + if (sourceNames.some((sourceName) => !testFiles.some((testFile) => isDirectMatch(testFile, sourceName)))) { + return testFiles + } + return testFiles.filter((testFile) => sourceNames.some((sourceName) => isDirectMatch(testFile, sourceName))) } export function shouldUseVitestRelated(packageEntry) { diff --git a/scripts/stryker-diff.test.mjs b/scripts/stryker-diff.test.mjs index f00c55f985..a52a27533e 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -231,6 +231,12 @@ describe("preferDirectTestFiles", () => { related.slice(0, 3), ) }) + + it("keeps all related tests when any changed source lacks a direct test", () => { + const related = ["src/__tests__/indirect-a.spec.ts", "src/__tests__/B.spec.ts"] + + assert.deepEqual(preferDirectTestFiles(related, ["src/A.ts", "src/B.ts"]), related) + }) }) describe("shouldUseVitestRelated", () => { diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 1e643e4fe8..fae796db6b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1126,6 +1126,9 @@ export class Task extends EventEmitter implements TaskLike { if (this.userMessageContent.length === 0) { return true } + if (this.abort) { + return false + } // CRITICAL: Wait for the assistant message to be saved to API history first. // Without this, tool_result blocks would appear BEFORE tool_use blocks in the @@ -1139,17 +1142,19 @@ export class Task extends EventEmitter implements TaskLike { // // The assistantMessageSavedToHistory flag is: // - Reset to false at the start of each API request - // - Set to true after the assistant message is saved in recursivelyMakeClineRequests + // - Set to true after the initial write or a bounded persistence retry succeeds if (!this.assistantMessageSavedToHistory) { - await pWaitFor(() => this.assistantMessageSavedToHistory || this.abort, { - interval: 50, - timeout: 30_000, // 30 second timeout as safety net - }).catch(() => { - // If timeout or abort, log and proceed anyway to avoid hanging + try { + if (!(await this.waitForCurrentAssistantMessagePersistence())) { + return false + } + } catch (error) { console.warn( - `[Task#${this.taskId}] flushPendingToolResultsToHistory: timed out waiting for assistant message to be saved`, + `[Task#${this.taskId}] flushPendingToolResultsToHistory: failed to persist assistant message`, + error, ) - }) + return false + } } // If task was aborted while waiting, don't flush diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index adc1bdc7f4..343af6f62a 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -862,6 +862,67 @@ describe("Task persistence", () => { vi.useRealTimers() } }) + + it("retries failed assistant persistence before flushing dependent tool results", async () => { + vi.useFakeTimers() + mockSaveApiMessages + .mockRejectedValueOnce(new Error("initial assistant 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: "tool_use", id: "tool-1", name: "read_file", input: {} }], + }) + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] + + const flushing = task.flushPendingToolResultsToHistory() + await vi.runAllTimersAsync() + + await expect(flushing).resolves.toBe(true) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(3) + expect(task.assistantMessageSavedToHistory).toBe(true) + expect(task.userMessageContent).toEqual([]) + } finally { + vi.useRealTimers() + } + }) + + it("does not flush dependent tool results when assistant persistence retries are exhausted", async () => { + vi.useFakeTimers() + mockSaveApiMessages.mockRejectedValue(new Error("assistant write failed")) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + try { + await getTaskPersistenceAccess(task).addToApiConversationHistory({ + role: "assistant", + content: [{ type: "tool_use", id: "tool-1", name: "read_file", input: {} }], + }) + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] + + const flushing = task.flushPendingToolResultsToHistory() + await vi.runAllTimersAsync() + + await expect(flushing).resolves.toBe(false) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(4) + expect(task.assistantMessageSavedToHistory).toBe(false) + expect(task.userMessageContent).toHaveLength(1) + } finally { + mockSaveApiMessages.mockResolvedValue(undefined) + vi.useRealTimers() + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 264ea6ea80..7418920cb1 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -3490,7 +3490,7 @@ describe("Cline", () => { expect(result).toBe(true) expect(safeSpy).toHaveBeenCalled() - expect(resetPersistenceSpy).toHaveBeenCalled() + expect(resetPersistenceSpy).toHaveBeenCalledTimes(1) expect(ensureModelFetched).toHaveBeenCalled() expect(task.cachedStreamingModel?.id).toBe(mockApiConfig.apiModelId) }) diff --git a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts index 80c5163c8e..4da57abd35 100644 --- a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts +++ b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts @@ -251,6 +251,7 @@ describe("flushPendingToolResultsToHistory", () => { task: "test task", startTask: false, }) + task.assistantMessageSavedToHistory = true // Set up pending tool result in userMessageContent task.userMessageContent = [ @@ -281,6 +282,7 @@ describe("flushPendingToolResultsToHistory", () => { task: "test task", startTask: false, }) + task.assistantMessageSavedToHistory = true // Set up pending tool result task.userMessageContent = [ @@ -304,6 +306,7 @@ describe("flushPendingToolResultsToHistory", () => { task: "test task", startTask: false, }) + task.assistantMessageSavedToHistory = true // Set up multiple pending tool results task.userMessageContent = [ @@ -336,6 +339,7 @@ describe("flushPendingToolResultsToHistory", () => { task: "test task", startTask: false, }) + task.assistantMessageSavedToHistory = true const beforeTs = Date.now() @@ -356,7 +360,7 @@ describe("flushPendingToolResultsToHistory", () => { expect((task.apiConversationHistory[0] as any).ts).toBeLessThanOrEqual(afterTs) }) - it("should skip waiting for assistantMessageSavedToHistory when flag is already true", async () => { + it("should skip the persistence barrier when the assistant message is already saved", async () => { const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, @@ -376,20 +380,18 @@ describe("flushPendingToolResultsToHistory", () => { }, ] - // Clear mock call history - mockPWaitFor.mockClear() + const waitForPersistence = vi.spyOn(task, "waitForCurrentAssistantMessagePersistence") await task.flushPendingToolResultsToHistory() - // Should not have called pWaitFor since flag was already true - expect(mockPWaitFor).not.toHaveBeenCalled() + expect(waitForPersistence).not.toHaveBeenCalled() // Should still save the message expect(task.apiConversationHistory.length).toBe(1) expect((task.apiConversationHistory[0].content as any[])[0].tool_use_id).toBe("tool-skip-wait") }) - it("should wait for assistantMessageSavedToHistory when flag is false", async () => { + it("should await the persistence barrier when the assistant message is not saved", async () => { const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, @@ -409,13 +411,11 @@ describe("flushPendingToolResultsToHistory", () => { }, ] - // Clear mock call history - mockPWaitFor.mockClear() + const waitForPersistence = vi.spyOn(task, "waitForCurrentAssistantMessagePersistence").mockResolvedValue(true) await task.flushPendingToolResultsToHistory() - // Should have called pWaitFor since flag was false - expect(mockPWaitFor).toHaveBeenCalled() + expect(waitForPersistence).toHaveBeenCalledTimes(1) // Should still save the message (mock resolves immediately) expect(task.apiConversationHistory.length).toBe(1) @@ -441,13 +441,14 @@ describe("flushPendingToolResultsToHistory", () => { }, ] - // Set abort flag - this will cause the condition in pWaitFor to return true - // AND will cause early return after the wait + const waitForPersistence = vi.spyOn(task, "waitForCurrentAssistantMessagePersistence") + task.abort = true await task.flushPendingToolResultsToHistory() // Should not have saved anything since task was aborted expect(task.apiConversationHistory.length).toBe(0) + expect(waitForPersistence).not.toHaveBeenCalled() }) }) 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 6568d77143..9ca14ac8d8 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -21,6 +21,7 @@ describe("API#getTaskApiConversationHistoryLength", () => { let providerListeners: Map unknown> beforeEach(() => { + // API logging only needs appendLine in this suite; a full OutputChannel fake would obscure the tested contract. mockOutputChannel = { appendLine: vi.fn(), } as unknown as vscode.OutputChannel From 69719ae079bc10046cd651574f51f06dd227ee91 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 5 Sep 2026 00:46:40 +0000 Subject: [PATCH 25/29] test(task): cover cancelled tool-result flush --- .../task/__tests__/Task.persistence.spec.ts | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 343af6f62a..1493445843 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -894,9 +894,40 @@ describe("Task persistence", () => { } }) + it("does not wait or flush dependent tool results after abort", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + task.abort = true + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] + const waitForPersistence = vi.spyOn(task, "waitForCurrentAssistantMessagePersistence") + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(false) + expect(waitForPersistence).not.toHaveBeenCalled() + expect(mockSaveApiMessages).not.toHaveBeenCalled() + }) + + it("does not flush dependent tool results when the persistence barrier is cancelled", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + task.userMessageContent = [{ type: "tool_result", tool_use_id: "tool-1", content: "done" }] + vi.spyOn(task, "waitForCurrentAssistantMessagePersistence").mockResolvedValue(false) + + await expect(task.flushPendingToolResultsToHistory()).resolves.toBe(false) + expect(mockSaveApiMessages).not.toHaveBeenCalled() + }) + it("does not flush dependent tool results when assistant persistence retries are exhausted", async () => { vi.useFakeTimers() mockSaveApiMessages.mockRejectedValue(new Error("assistant write failed")) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => undefined) const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, @@ -918,7 +949,12 @@ describe("Task persistence", () => { expect(mockSaveApiMessages).toHaveBeenCalledTimes(4) expect(task.assistantMessageSavedToHistory).toBe(false) expect(task.userMessageContent).toHaveLength(1) + expect(consoleWarn).toHaveBeenCalledWith( + expect.stringContaining("failed to persist assistant message"), + expect.any(Error), + ) } finally { + consoleWarn.mockRestore() mockSaveApiMessages.mockResolvedValue(undefined) vi.useRealTimers() } From 66b7b0aa3d4affe14d71276c11b7f4b7fddb8741 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 5 Sep 2026 01:41:16 +0000 Subject: [PATCH 26/29] chore(task): align persistence checks with cleanup rebase --- src/core/task/__tests__/Task.persistence.spec.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 1493445843..5d1ece6994 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -461,7 +461,7 @@ describe("Task persistence", () => { await getTaskPersistenceAccess(task).addToApiConversationHistory({ role: "assistant", content: "done" }) await expect(task.waitForCurrentAssistantMessagePersistence()).resolves.toBe(true) - task.dispose() + void task.dispose() await expect(task.waitForCurrentAssistantMessagePersistence()).resolves.toBe(false) }) @@ -747,7 +747,7 @@ describe("Task persistence", () => { }) const waiting = task.waitForCurrentAssistantMessagePersistence() - task.dispose() + void task.dispose() await expect(waiting).resolves.toBe(false) }) @@ -770,7 +770,7 @@ describe("Task persistence", () => { const waiting = task.waitForCurrentAssistantMessagePersistence() await vi.advanceTimersByTimeAsync(50) - task.dispose() + void task.dispose() await expect(waiting).resolves.toBe(false) await Promise.resolve() @@ -794,7 +794,7 @@ describe("Task persistence", () => { getTaskPersistenceAccess(task).resetAssistantMessagePersistence() await expect(waiting).resolves.toBe(false) - task.dispose() + void task.dispose() }) it("does not retry when cancelled after the delay resolves but before persistence starts", async () => { @@ -818,7 +818,7 @@ describe("Task persistence", () => { // Resolve the delay without flushing its promise continuation, then cancel at the save boundary. await Promise.resolve() vi.advanceTimersByTime(100) - task.dispose() + void task.dispose() await expect(waiting).resolves.toBe(false) expect(task.assistantMessageSavedToHistory).toBe(false) @@ -852,7 +852,7 @@ describe("Task persistence", () => { await vi.advanceTimersByTimeAsync(100) expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) - task.dispose() + void task.dispose() retrySave.resolve(undefined) await expect(waiting).resolves.toBe(false) From 519813f7ff4b2c293e493fb3c2a2eb4d42b1b840 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 7 Sep 2026 03:31:08 +0000 Subject: [PATCH 27/29] fix(e2e): replace undefined conversationLength with sequence check --- .../vscode-e2e/src/suite/restart-persistence.test.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 29f94de35d..4778b05857 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -106,9 +106,15 @@ async function runVerify(api: RooCodeAPI): Promise { reopenedHistoryItem.task.includes("RESTART_PERSISTENCE_SMOKE"), "Reopened task should retain its persisted history title", ) - assert.ok( - (await api.getTaskApiConversationHistoryLength(taskId)) >= conversationLength, - "Reopened task should retain its persisted API conversation history", + const reopenedCompletion = await api.hasTaskApiConversationHistorySequence(taskId, { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: MARKER, + }) + assert.strictEqual( + reopenedCompletion, + true, + "Reopened-host history should restore the marked user turn followed by its assistant completion", ) await writePhaseResult(getResultsDir(), { From a28f3c5c0e9bff3380e2b0a6f56cf75e4aa71dcf Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 7 Sep 2026 14:47:23 +0000 Subject: [PATCH 28/29] fix(test): address CodeRabbit review findings and restore mutation CI --- scripts/stryker-diff.mjs | 2 +- scripts/stryker-diff.test.mjs | 2 ++ .../task/__tests__/Task.persistence.spec.ts | 16 +++++++--- .../flushPendingToolResultsToHistory.spec.ts | 21 +++++++++--- src/extension/api.ts | 32 +++++++++++-------- 5 files changed, 50 insertions(+), 23 deletions(-) diff --git a/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index a4c5ddcb80..2d6adddfa2 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -301,7 +301,7 @@ export function preferDirectTestFiles(testFiles, sourceFiles) { const normalizedTestName = testName.toLowerCase() return ( (normalizedTestName.startsWith(`${sourceName}.`) || normalizedTestName.startsWith(`${sourceName}-`)) && - /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName) + /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(normalizedTestName) ) } if (sourceNames.some((sourceName) => !testFiles.some((testFile) => isDirectMatch(testFile, sourceName)))) { diff --git a/scripts/stryker-diff.test.mjs b/scripts/stryker-diff.test.mjs index a52a27533e..931840606f 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -22,6 +22,7 @@ import { parseChangedLines, parseNameStatus, parseVitestTestFiles, + preferDirectTestFiles, resolveStrykerTempDir, resolveVitestBinary, shouldUseVitestRelated, @@ -244,6 +245,7 @@ describe("shouldUseVitestRelated", () => { assert.equal(shouldUseVitestRelated({ testFiles: ["focused.spec.ts"] }), false) assert.equal(shouldUseVitestRelated({ testFiles: [], vitestRelated: true }), true) assert.equal(shouldUseVitestRelated({ vitestRelated: false }), false) + assert.equal(shouldUseVitestRelated({ testFiles: [] }), true) }) }) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 5d1ece6994..8d3314a9a6 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -669,9 +669,10 @@ describe("Task persistence", () => { it("emits TaskCompleted after a failed assistant save succeeds on retry", async () => { vi.useFakeTimers() + const retryDeferred = createDeferred() mockSaveApiMessages .mockRejectedValueOnce(new Error("initial write failed")) - .mockResolvedValueOnce(undefined) + .mockReturnValueOnce(retryDeferred.promise) const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, @@ -721,19 +722,24 @@ describe("Task persistence", () => { ) expect(completionListener).not.toHaveBeenCalled() - await vi.runAllTimersAsync() + // Advance past the 100 ms retry delay so the retry save starts. + await vi.advanceTimersByTimeAsync(150) + await vi.waitFor(() => expect(mockSaveApiMessages).toHaveBeenCalledTimes(2)) + // Completion must not fire while the retry save is still in-flight. + expect(completionListener).not.toHaveBeenCalled() + + // Settle the retry save; completion should follow. + retryDeferred.resolve(undefined) await handlingCompletion - 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], ) } finally { - mockSaveApiMessages.mockResolvedValue(undefined) + retryDeferred.resolve(undefined) vi.useRealTimers() } }) diff --git a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts index 4da57abd35..f32a6e2507 100644 --- a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts +++ b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts @@ -411,13 +411,26 @@ describe("flushPendingToolResultsToHistory", () => { }, ] - const waitForPersistence = vi.spyOn(task, "waitForCurrentAssistantMessagePersistence").mockResolvedValue(true) + let resolveWait!: (result: boolean) => void + const waitDeferred = new Promise((res) => { + resolveWait = res + }) + const waitForPersistence = vi + .spyOn(task, "waitForCurrentAssistantMessagePersistence") + .mockReturnValue(waitDeferred) - await task.flushPendingToolResultsToHistory() + const flushPromise = task.flushPendingToolResultsToHistory() - expect(waitForPersistence).toHaveBeenCalledTimes(1) + // Yield so the async function reaches the persistence await and suspends. + await Promise.resolve() + // History save must not start before persistence resolves. + expect(task.apiConversationHistory.length).toBe(0) + + resolveWait(true) + await flushPromise - // Should still save the message (mock resolves immediately) + expect(waitForPersistence).toHaveBeenCalledTimes(1) + // Should still save the message once persistence settled. expect(task.apiConversationHistory.length).toBe(1) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index cad44f08ba..b0677d2fc4 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -273,19 +273,25 @@ export class API extends EventEmitter implements RooCodeAPI { ) 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), - ), - ) + // Search all assistant turns that belong to the same generation: between + // this user turn and the next user turn (or end of history). A match in a + // later generation would be a false positive. + const nextUserIndex = apiConversationHistory.findIndex((m, i) => i > userTurnIndex && m.role === "user") + const generationSlice = apiConversationHistory.slice( + userTurnIndex + 1, + nextUserIndex < 0 ? undefined : nextUserIndex, + ) + return generationSlice.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 4723d3fbce639d895b0a9c60b6e5bcf36b67b0ba Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 7 Sep 2026 15:53:28 +0000 Subject: [PATCH 29/29] fix(api): skip tool_result messages at generation boundary --- ...i-task-conversation-history-length.spec.ts | 72 +++++++++++++++++++ src/extension/api.ts | 14 +++- 2 files changed, 83 insertions(+), 3 deletions(-) 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 9ca14ac8d8..b60c5eb21a 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -235,6 +235,78 @@ describe("API#getTaskApiConversationHistoryLength", () => { await expect(api.hasTaskApiConversationHistorySequence("task-1", expectedSequence)).resolves.toBe(true) }) + it("accepts attempt_completion after a persisted tool_result user message in the same generation", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [{ type: "tool_use", id: "read-1", name: "read_file", input: { path: "f.ts" } }], + }, + // tool_result is role:"user" — must not terminate the generation search + { role: "user", content: [{ type: "tool_result", tool_use_id: "read-1", content: "file contents" }] }, + { + 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 attempt_completion that follows a later genuine user text turn", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [{ type: "tool_use", id: "read-1", name: "read_file", input: { path: "f.ts" } }], + }, + // A genuine user text turn ends the generation — completion below is in the next generation + { role: "user", content: [{ type: "text", text: "follow-up question" }] }, + { + 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("treats a user turn with mixed text and tool_result content as a genuine boundary", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [{ type: "tool_use", id: "read-1", name: "read_file", input: { path: "f.ts" } }], + }, + // Mixed content: has both a tool_result and a text block — counts as a genuine user turn + { + role: "user", + content: [ + { type: "tool_result", tool_use_id: "read-1", content: "file contents" }, + { type: "text", text: "what do you think?" }, + ], + }, + { + 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("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 b0677d2fc4..316e7a6c9d 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -274,9 +274,17 @@ export class API extends EventEmitter implements RooCodeAPI { if (userTurnIndex < 0) return false // Search all assistant turns that belong to the same generation: between - // this user turn and the next user turn (or end of history). A match in a - // later generation would be a false positive. - const nextUserIndex = apiConversationHistory.findIndex((m, i) => i > userTurnIndex && m.role === "user") + // this user turn and the next genuine user text turn (or end of history). + // tool_result messages also use role:"user", so we must skip them — + // stopping at a tool_result would cut the search short before a later + // attempt_completion in the same generation. + const nextUserIndex = apiConversationHistory.findIndex( + (m, i) => + i > userTurnIndex && + m.role === "user" && + Array.isArray(m.content) && + m.content.some((block) => block.type === "text"), + ) const generationSlice = apiConversationHistory.slice( userTurnIndex + 1, nextUserIndex < 0 ? undefined : nextUserIndex,