diff --git a/.github/workflows/code-qa.yml b/.github/workflows/code-qa.yml index 7c71478266..b4e5881f6b 100644 --- a/.github/workflows/code-qa.yml +++ b/.github/workflows/code-qa.yml @@ -90,8 +90,8 @@ jobs: run: pnpm lint - name: Check types run: pnpm check-types - - name: Model-check concurrent task lifecycle - run: pnpm lifecycle:model-check + - name: Model-check task lifecycle + run: pnpm lifecycle:model build-vsix: name: Build test VSIX diff --git a/AGENTS.md b/AGENTS.md index 3b5be80ede..e14148e1be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,7 @@ Prefer the narrowest test layer that proves the behavior. This follows standard ## Task Lifecycle Changes - Read `docs/architecture/task-lifecycle-model.md` before changing task status, delegation, interruption, completion, abandonment, persistence ownership, or scheduler fan-out behavior. -- Keep lifecycle mutations in the shared reducers under `src/core/task-persistence/taskLifecycle.ts`. Update model actions, invariants, or named semantic landmarks for every new transition or concurrency bug class representable in the lifecycle model, then run `pnpm lifecycle:model-check`. +- Keep lifecycle mutations in the shared reducers under `src/core/task-persistence/taskLifecycle.ts`. Update model actions, invariants, or named semantic landmarks for every new transition or concurrency bug class representable in the lifecycle model, then run `pnpm lifecycle:model`. - Add extension-host E2E coverage only for a boundary the reducer model cannot prove, such as restart visibility, real persistence/rehydration, delayed provider streams, scheduler permits, or webview task scoping. Do not duplicate reducer interleavings in E2E. - Run the focused lifecycle tests and `pnpm test` before completing a Zoo Code lifecycle change. diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 29e7fa3ddd..dfd55af923 100644 --- a/apps/vscode-e2e/src/suite/restart-persistence.test.ts +++ b/apps/vscode-e2e/src/suite/restart-persistence.test.ts @@ -43,11 +43,6 @@ async function runCreate(api: RooCodeAPI): Promise { }) await waitUntilCompleted({ api, taskId }) assert.strictEqual(sawMarker, true, `Completion should include ${MARKER}`) - const historyItem = await api.getTaskHistoryItem(taskId) - assert.ok(historyItem, "Completed task should have a history item") - assert.ok(historyItem.task.includes("RESTART_PERSISTENCE_SMOKE"), "History title should include the marker") - const conversationLength = await api.getTaskApiConversationHistoryLength(taskId) - assert.ok(conversationLength > 0, "Completed task should persist API conversation history") const result: PhaseResult = { version: PHASE_RESULT_VERSION, @@ -84,14 +79,22 @@ async function runVerify(api: RooCodeAPI): Promise { const historyItem = await api.getTaskHistoryItem(taskId) assert.ok(historyItem, "Task history item should be available after restart") assert.ok(historyItem.task.includes("RESTART_PERSISTENCE_SMOKE"), "History title should persist after restart") - const conversationLength = await api.getTaskApiConversationHistoryLength(taskId) - assert.ok(conversationLength > 0, "API conversation history should be available after restart") + const restoredCompletion = await api.hasTaskApiConversationHistorySequence(taskId, { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: MARKER, + }) + assert.strictEqual( + restoredCompletion, + true, + "Fresh-host history should restore the marked user turn followed by its assistant completion", + ) await writePhaseResult(getResultsDir(), { version: PHASE_RESULT_VERSION, phase: "verify", status: "passed", - values: { taskId, conversationLength: String(conversationLength) }, + values: { taskId }, }) await quitGracefully() } catch (error) { diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 588ffd5204..36339096da 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -3,17 +3,17 @@ Zoo Code checks its persisted task delegation lifecycle with a bounded, exhaustive state explorer. Run it locally with: ```sh -pnpm lifecycle:model-check +pnpm lifecycle:model ``` -The check runs in the `compile` CI job after type checking. It fails if it finds an invariant violation, a modeled action becomes unreachable, or exploration exceeds its declared state budget. A violation includes the shortest breadth-first event trace, every intermediate state, and the active bounds so the sequence can be replayed as a focused regression test. +The check runs in the `compile` CI job after type checking. It fails if it finds an invariant violation, a modeled action becomes unreachable, a named semantic landmark disappears, or exploration exceeds its declared state budget. A violation includes the shortest breadth-first event trace, every intermediate state, and the active bounds so the sequence can be replayed as a focused regression test. The command generates no checked-in artifacts; its reports and counterexamples are written to standard output. `pnpm lifecycle:model-check` remains as a compatibility alias. ## Why an executable TypeScript model -The initial model uses a small explicit-state explorer rather than adding Quint, TLA+/TLC, or Alloy. This is deliberate: +The models use small explicit-state explorers rather than adding Quint, TLA+/TLC, or Alloy. This is deliberate: - Zoo's current risks are finite safety properties over a small persisted state machine, not yet temporal liveness or fairness properties. -- The explorer calls the production transition functions in `src/core/task-persistence/taskLifecycle.ts`. `ClineProvider` uses those same functions inside serialized and atomic store operations, reducing specification drift. +- 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. @@ -39,7 +39,7 @@ Production completion also accepts a recovery-compatible `active` parent that st ## Shared-store concurrency model -The same `pnpm lifecycle:model-check` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: +The same `pnpm lifecycle:model` command also runs a second bounded explorer over two `TaskHistoryStore` hosts. It imports the production `computeHistoryDelta` and `mergeHistoryDelta` functions, so its semantics match the store rather than assuming coherent caches or transactional pair writes: - each host has an independent cache and host-local mutex; - store read/update operations hold the host mutex, while live-task snapshots used by completion and message saves may outlive it; @@ -63,9 +63,25 @@ The known-unsafe witnesses currently compare exact shortest action sequences. Th `TaskHistoryStore.realConcurrency.spec.ts` complements the abstract interleavings with one synchronized integration smoke check through the real `proper-lockfile` and filesystem rename path; broader VS Code E2E remains reserved for restart and extension-host behavior. +## Completion persistence model + +`scripts/check-completion-persistence.ts` models the completion-readiness protocol that protects the public `TaskCompleted` event. It starts from both standalone and delegated tasks and exhaustively interleaves: + +- starting, finishing, or failing the assistant-history write; +- accepting completion before, during, or after persistence; +- scheduling a bounded retry, completing its delay, and starting the retry write; +- exhausting retries; +- cancellation or disposal at every reachable non-completed state; +- 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. + +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 -The checker currently enforces: +The task delegation checker currently enforces: 1. A delegated parent has exactly one `awaitingChildId`, and `delegatedToId` matches it. 2. The awaited child exists, links back to the parent, is not completed, and remains in `childIds`. A delegated child may itself await a nested child. @@ -75,22 +91,30 @@ 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 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. 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 The following map separates issue observations from the architectural interpretation encoded here. Open issues can change after this document is written; follow each link for current status. -| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | -| ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | -| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | -| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it, while an Alloy abstraction permits the ordering. | A completion/readiness contract must define whether completion implies restart visibility. This is a liveness/durability boundary, not only a `HistoryItem` safety transition. | Not claimed by this checker. Add a controlled persistence barrier test after the contract decision; move to temporal model checking if eventual readiness and failure handling become protocol guarantees. | -| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | -| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | -| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | -| [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. It warrants a parser-scope model or deterministic interleaving test, not an unrelated field in the delegation model. | -| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | +| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | +| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | +| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it. | Completion implies restart-visible assistant history. Delayed or failed writes keep completion pending, and cancellation settles readiness without starting stale retries or emitting completion. | The completion persistence explorer checks the bounded event-ordering and cancellation contract for standalone and delegated tasks. Focused `Task` and `AttemptCompletionTool` tests cover the production adapter; `restart-persistence.test.ts` verifies visibility through a fresh extension host. | +| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | +| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | +| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | +| [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. It warrants a parser-scope model or deterministic interleaving test, not an unrelated field in the delegation model. | +| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | The issue-derived cases intentionally map to bug classes rather than issue-specific flags. In particular, stale event ownership, monotonic terminal/detached state, explicit scope, and single-writer boundaries generalize to future concurrent task work. @@ -104,7 +128,7 @@ When production lifecycle behavior changes: 4. Increase depth or task slots only when the new scenario requires it. Keep the state budget explicit and ensure CI completes quickly. 5. Convert any discovered counterexample into a focused production regression test as well as retaining the architectural invariant. -Do not weaken bounds or remove an invariant merely to make CI pass. If state growth becomes difficult to control, split independent protocols or move the model to TLC/Quint with an implementation trace adapter rather than silently sampling the state space. +Completion-readiness changes belong in `scripts/check-completion-persistence.ts`; shared-store interleavings belong in `scripts/check-task-store-concurrency.ts`. Do not weaken bounds or remove an invariant merely to make CI pass. If state growth becomes difficult to control, split independent protocols or move the model to TLC/Quint with an implementation trace adapter rather than silently sampling the state space. ## Test layering diff --git a/package.json b/package.json index 8431467918..9763a00b1b 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,8 @@ "check-types": "turbo check-types --log-order grouped --output-logs new-only", "test": "turbo test --log-order grouped --output-logs new-only", "test:mutation-ci": "node --test scripts/stryker-diff.test.mjs", - "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts", + "lifecycle:model": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-completion-persistence.ts", + "lifecycle:model-check": "pnpm lifecycle:model", "test:coverage": "turbo test:coverage --log-order grouped --output-logs new-only", "format": "turbo format --log-order grouped --output-logs new-only", "build": "turbo build --log-order grouped --output-logs new-only", diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index 961b068778..de23f67491 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -10,6 +10,12 @@ import type { WebviewThemeFixture } from "./vscode-extension-host.js" export type RooCodeAPIEvents = RooCodeEvents +export interface TaskApiConversationHistorySequence { + userText: string + assistantToolName: string + assistantToolInputText: string +} + export interface RooCodeAPI extends EventEmitter { /** * Starts a new task with an optional initial message and images. @@ -52,6 +58,16 @@ export interface RooCodeAPI extends EventEmitter { * @returns The number of persisted API conversation history entries, or 0 if unavailable. */ getTaskApiConversationHistoryLength(taskId: string): Promise + /** + * Checks for an ordered user turn and assistant tool call in persisted API history. + * @param taskId The ID of the task. + * @param sequence The expected user text and assistant tool-call markers. + * @returns True when the expected turns exist in order, or false if unavailable. + */ + hasTaskApiConversationHistorySequence( + taskId: string, + sequence: TaskApiConversationHistorySequence, + ): Promise /** * Returns the current task stack. * @returns An array of task IDs. diff --git a/packages/types/src/events.ts b/packages/types/src/events.ts index fc6c3c25d4..20f7f7e71e 100644 --- a/packages/types/src/events.ts +++ b/packages/types/src/events.ts @@ -14,6 +14,7 @@ export enum RooCodeEventName { // Task Lifecycle TaskStarted = "taskStarted", + /** Emitted after the accepted completion turn is persisted and visible to a fresh extension host. */ TaskCompleted = "taskCompleted", TaskAborted = "taskAborted", TaskFocused = "taskFocused", diff --git a/scripts/check-completion-persistence.ts b/scripts/check-completion-persistence.ts new file mode 100644 index 0000000000..10c9d96769 --- /dev/null +++ b/scripts/check-completion-persistence.ts @@ -0,0 +1,288 @@ +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 + history: HistoryPhase + retry: RetryPhase + writeStarts: WriteStarts + completionAccepted: boolean + completionEmitted: boolean + cancelled: boolean + waitSettled: boolean + cancelledAtRetryBoundary: boolean + delegation: DelegationPhase +} + +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", + "reopen-parent", + "fail-parent-reopen", + "emit-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, + "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.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 { + return { + kind, + history: "idle", + retry: "idle", + writeStarts: 0, + completionAccepted: false, + completionEmitted: false, + cancelled: false, + waitSettled: false, + cancelledAtRetryBoundary: false, + delegation: kind === "delegated" ? "awaiting-reopen" : "not-applicable", + } +} + +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.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 + ) { + result.push({ + name: "emit-completion", + next: { ...state, completionEmitted: true, waitSettled: true }, + }) + } + + return result +} + +function invariantViolations(state: ModelState): string[] { + return Object.entries(stateInvariants).flatMap(([name, check]) => { + const violation = check(state) + return violation ? [`${name}: ${violation}`] : [] + }) +} + +function transitionViolations(previous: ModelState, transition: Transition): string[] { + return Object.entries(transitionInvariants).flatMap(([name, check]) => { + const violation = check(previous, transition) + return violation ? [`${name}: ${violation}`] : [] + }) +} + +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() +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, ${invariantCount} invariants, ${Object.keys(semanticLandmarks).length}/${Object.keys(semanticLandmarks).length} landmarks reached, depth <= ${MAX_DEPTH}, writes <= 2`, +) diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index d3a24a3140..97bb6f8594 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -1380,6 +1380,7 @@ describe("History resume delegation - parent metadata transitions", () => { consecutiveMistakeCount: 0, emitFinalTokenUsageUpdate: vi.fn(), flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(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 8464f81b12..dd015e93cf 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(true), } 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(true), } as unknown as Task const blockB = { diff --git a/src/api/index.ts b/src/api/index.ts index f45412e32c..98c3c5dc7b 100644 --- a/src/api/index.ts +++ b/src/api/index.ts @@ -236,7 +236,7 @@ export function buildApiHandler(configuration: ProviderSettings): ApiHandler { case providerIdentifiers.poe: return new PoeHandler(options) case providerIdentifiers.geminiCli: - // Intentionally falls through to the Anthropic handler pending a dedicated Gemini CLI handler implementation. + // Intentionally falls through to the Anthropic handler pending a dedicated Gemini CLI handler implementation. default: return new AnthropicHandler(options) } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 37281a9010..5d2d47ca0c 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -194,6 +194,13 @@ export interface TaskOptions extends CreateTaskOptions { diffFuzzyThreshold?: number } +type AssistantMessagePersistenceResult = "saved" | "failed" | "cancelled" +type AssistantMessagePersistenceCancellation = { + cancelled: boolean + promise: Promise + resolve: () => void +} + export class Task extends EventEmitter implements TaskLike { readonly taskId: string readonly rootTaskId?: string @@ -402,9 +409,13 @@ 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!: (result: AssistantMessagePersistenceResult) => void + private assistantMessagePersistenceCancellation?: AssistantMessagePersistenceCancellation + private completionPersistenceReadyPromise?: Promise /** * Fire-and-forget wrapper around `presentAssistantMessage` that swallows the @@ -511,6 +522,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") @@ -921,6 +933,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 @@ -979,7 +995,11 @@ export class Task extends EventEmitter implements TaskLike { return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } - private async addToApiConversationHistory(message: Anthropic.MessageParam, reasoning?: string) { + /** + * Appends an API turn and records whether an assistant turn reached persistent storage. + * 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 && message.role === "user" && @@ -1011,6 +1031,67 @@ export class Task extends EventEmitter implements TaskLike { ) } } + if (message.role === "assistant") { + this.assistantMessageSavedToHistory = saved + this.resolveAssistantMessagePersistence(saved ? "saved" : "failed") + } + } + + /** Cancels the current persistence generation before creating the next assistant-turn boundary. */ + private resetAssistantMessagePersistence(): void { + this.cancelAssistantMessagePersistence() + this.assistantMessagePersistencePromise = new Promise((resolve) => { + this.resolveAssistantMessagePersistence = 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.assistantMessagePersistenceCancellation?.resolve() + } + + /** + * 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 + const currentCancellation = this.assistantMessagePersistenceCancellation! + this.completionPersistenceReadyPromise = (async () => { + const result = await Promise.race([ + currentPersistence, + currentCancellation.promise.then(() => "cancelled" as const), + ]) + if (result === "cancelled") return false + if (result === "saved") return true + + 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 + return true + })() + } + + return this.completionPersistenceReadyPromise } // NOTE: We intentionally do NOT mutate stored messages to merge consecutive user turns. @@ -1121,22 +1202,54 @@ export class Task extends EventEmitter implements TaskLike { * Used by delegation flow when flushPendingToolResultsToHistory reports failure. */ public async retrySaveApiConversationHistory(): Promise { + return (await this.retrySaveApiConversationHistoryWithCancellation()) === "saved" + } + + /** Retries API-history persistence while allowing the active assistant generation to cancel backoff. */ + 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])) + // 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]) + void cancellation.promise.then(() => { + clearTimeout(timer) + finish(false) + }) + }) + if (!delayCompleted) return "cancelled" + } 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}`, ) const success = await this.saveApiConversationHistory() + if (cancellation?.cancelled) return "cancelled" if (success) { - return true + return "saved" } } - return false + return "failed" } // Cline Messages @@ -1178,6 +1291,10 @@ export class Task extends EventEmitter implements TaskLike { } } + /** + * Replaces the entire Cline message history, restores todo state, and persists. + * Also resets cloud sync tracking to avoid re-syncing previously synced messages. + */ public async overwriteClineMessages(newMessages: ClineMessage[]) { this.clineMessages = newMessages restoreTodoListForTask(this) @@ -2473,6 +2590,7 @@ export class Task extends EventEmitter implements TaskLike { } this.abort = true + this.cancelAssistantMessagePersistence() // Reset consecutive error counters on abort (manual intervention) this.consecutiveNoToolUseCount = 0 @@ -2516,6 +2634,7 @@ export class Task extends EventEmitter implements TaskLike { public dispose(): void { console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) + this.cancelAssistantMessagePersistence() // Stop the idle telemetry check and report any unflushed activity as a // shutdown installment, so a task torn down mid-work (panel closed, task @@ -2991,6 +3110,7 @@ export class Task extends EventEmitter implements TaskLike { this.didRejectTool = false this.didAlreadyUseTool = false this.assistantMessageSavedToHistory = false + this.resetAssistantMessagePersistence() // Reset tool failure flag for each new assistant turn - this ensures that tool failures // only prevent attempt_completion within the same assistant message, not across turns // (e.g., if a tool fails, then user sends a message saying "just complete anyway") @@ -3800,7 +3920,6 @@ export class Task extends EventEmitter implements TaskLike { { role: "assistant", content: assistantContent }, reasoningMessage || undefined, ) - this.assistantMessageSavedToHistory = true this.messageCounts.assistant++ } diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 671bd7d4b7..c06b2c6ead 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,12 @@ 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 + resetAssistantMessagePersistence: () => void resumeTaskFromHistory: () => Promise resumePendingTaskAction: (action: PendingTaskAction) => Promise saveClineMessages: () => Promise @@ -385,6 +394,324 @@ describe("Task persistence", () => { // But the content should be the same expect(callArgs.messages).toEqual(task.apiConversationHistory) }) + + it("emits TaskCompleted only after API history persistence succeeds", async () => { + const saveDeferred = createDeferred() + mockSaveApiMessages.mockReturnValueOnce(saveDeferred.promise) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const privateTask = getTaskPersistenceAccess(task) + const completionCallId = "completion-call" + let saveSettled = false + let completionEmitted = false + let saving: Promise | undefined + + try { + vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) + vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) + task.on(RooCodeEventName.TaskCompleted, () => { + completionEmitted = true + }) + + const block: AttemptCompletionToolUse = { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + params: { result: "done" }, + nativeArgs: { result: "done" }, + partial: false, + } + const callbacks: AttemptCompletionCallbacks = { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: vi.fn(), + askFinishSubTaskApproval: vi.fn(), + toolDescription: vi.fn(), + toolCallId: completionCallId, + } + + const handlingCompletion = attemptCompletionTool.handle(task, block, callbacks) + await vi.waitFor(() => expect(task.ask).toHaveBeenCalled()) + + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(completionEmitted).toBe(false) + expect(mockSaveApiMessages).not.toHaveBeenCalled() + + saving = privateTask.addToApiConversationHistory({ + role: "assistant", + content: [ + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + input: { result: "done" }, + }, + ], + }) + void saving.finally(() => { + saveSettled = true + }) + await vi.waitFor(() => expect(mockSaveApiMessages).toHaveBeenCalledTimes(1)) + const saveRequest = mockSaveApiMessages.mock.calls[0][0] + expect(saveRequest.taskId).toBe(task.taskId) + expect(saveRequest.messages).toEqual([ + expect.objectContaining({ + role: "assistant", + content: expect.arrayContaining([ + expect.objectContaining({ + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + }), + ]), + }), + ]) + expect(saveSettled).toBe(false) + expect(completionEmitted).toBe(false) + + saveDeferred.resolve(undefined) + await Promise.all([saving, handlingCompletion]) + + expect(callbacks.handleError).not.toHaveBeenCalled() + expect(completionEmitted).toBe(true) + } finally { + saveDeferred.resolve(undefined) + await saving + } + }) + + it("does not emit TaskCompleted when API history persistence exhausts its retries", async () => { + vi.useFakeTimers() + mockSaveApiMessages.mockRejectedValue(new Error("write failed")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const completionCallId = "failed-completion-call" + const privateTask = getTaskPersistenceAccess(task) + const callbacks: AttemptCompletionCallbacks = { + askApproval: vi.fn(), + handleError: vi.fn(), + pushToolResult: vi.fn(), + askFinishSubTaskApproval: vi.fn(), + toolDescription: vi.fn(), + toolCallId: completionCallId, + } + vi.spyOn(task, "say").mockResolvedValue(undefined) + vi.spyOn(task, "ask").mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }) + vi.spyOn(task, "emitFinalTokenUsageUpdate").mockImplementation(() => undefined) + vi.spyOn(task, "flushTelemetryInstallment").mockImplementation(() => undefined) + const completionListener = vi.fn() + task.on(RooCodeEventName.TaskCompleted, completionListener) + + try { + await privateTask.addToApiConversationHistory({ + role: "assistant", + content: [ + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + input: { result: "done" }, + }, + ], + }) + + const handlingCompletion = attemptCompletionTool.handle( + task, + { + type: "tool_use", + id: completionCallId, + name: "attempt_completion", + params: { result: "done" }, + nativeArgs: { result: "done" }, + partial: false, + }, + callbacks, + ) + await vi.runAllTimersAsync() + await handlingCompletion + + expect(mockSaveApiMessages).toHaveBeenCalledTimes(4) + expect(completionListener).not.toHaveBeenCalled() + expect(callbacks.handleError).toHaveBeenCalledWith( + "inspecting site", + expect.objectContaining({ + message: "Failed to persist API conversation history before task completion", + }), + ) + } finally { + mockSaveApiMessages.mockResolvedValue(undefined) + vi.useRealTimers() + } + }) + + 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) + // 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() + } + }) + + 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) + await vi.runAllTimersAsync() + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + } finally { + 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() + }) + + 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) + + 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() + + // 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() + + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + } finally { + vi.useRealTimers() + } + }) }) // ── saveClineMessages ──────────────────────────────────────────────── diff --git a/src/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index a71520b5cc..f9252d054f 100644 --- a/src/core/tools/AttemptCompletionTool.ts +++ b/src/core/tools/AttemptCompletionTool.ts @@ -142,6 +142,14 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { task.flushTelemetryInstallment("attempt_completion") hasFlushedTelemetry = true + try { + const persistenceReady = await task.waitForCurrentAssistantMessagePersistence() + if (!persistenceReady) return + } catch (error) { + await handleError("persisting task completion", error as Error) + return + } + const delegation = await this.delegateToParent( task, result, @@ -151,7 +159,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 +215,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 +298,13 @@ 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 { + 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. task.emitFinalTokenUsageUpdate() diff --git a/src/core/tools/__tests__/attemptCompletionTool.spec.ts b/src/core/tools/__tests__/attemptCompletionTool.spec.ts index 5e57ca726f..fb73c08390 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(true), } }) @@ -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(true) + }) const block: AttemptCompletionToolUse = { type: "tool_use", name: "attempt_completion", @@ -507,6 +512,7 @@ describe("attemptCompletionTool", () => { taskId: "child-1", parentTaskId: "parent-1", providerRef: { deref: () => mockProvider }, + waitForCurrentAssistantMessagePersistence: vi.fn(() => persistenceReady), }) mockAskFinishSubTaskApproval.mockResolvedValue(true) @@ -519,7 +525,12 @@ describe("attemptCompletionTool", () => { toolCallId: "call-attempt-completion", } - await attemptCompletionTool.handle(mockTask as Task, block, callbacks) + const handlingCompletion = attemptCompletionTool.handle(mockTask as Task, block, callbacks) + await vi.waitFor(() => expect(mockTask.waitForCurrentAssistantMessagePersistence).toHaveBeenCalled()) + expect(mockProvider.reopenParentFromDelegation).not.toHaveBeenCalled() + + markPersistenceReady() + await handlingCompletion expect(mockAskFinishSubTaskApproval).toHaveBeenCalled() expect(mockProvider.setPendingTaskAction).toHaveBeenCalledWith("child-1", { @@ -539,6 +550,105 @@ 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("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", @@ -772,6 +882,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", @@ -780,6 +894,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", @@ -970,6 +1112,7 @@ describe("attemptCompletionTool telemetry invariants", () => { messageCounts: { user: 0, assistant: 0 }, taskId: "task_1", flushTelemetryInstallment: vi.fn(), + waitForCurrentAssistantMessagePersistence: vi.fn().mockResolvedValue(true), ...overrides, } } diff --git a/src/extension/__tests__/api-task-conversation-history-length.spec.ts b/src/extension/__tests__/api-task-conversation-history-length.spec.ts index 4cfd9bbe4b..7018f59880 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -42,4 +42,65 @@ describe("API#getTaskApiConversationHistoryLength", () => { await expect(api.getTaskApiConversationHistoryLength("missing-task")).resolves.toBe(0) }) + + it("finds the expected persisted user and assistant turns in order", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [ + { type: "text", text: "Finished" }, + { type: "tool_use", id: "completion", name: "attempt_completion", input: { result: "done" } }, + ], + }, + ], + }) + + await expect( + api.hasTaskApiConversationHistorySequence("task-1", { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + }), + ).resolves.toBe(true) + }) + + it("returns false when the expected persisted turns are unavailable", async () => { + mockGetTaskWithId.mockRejectedValue(new Error("Task not found")) + + await expect( + api.hasTaskApiConversationHistorySequence("missing-task", { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + }), + ).resolves.toBe(false) + }) + + it("rejects an assistant completion that does not follow the expected user turn", async () => { + mockGetTaskWithId.mockResolvedValue({ + apiConversationHistory: [ + { + role: "assistant", + content: [{ type: "tool_use", id: "early", name: "attempt_completion", input: { result: "done" } }], + }, + { role: "user", content: [{ type: "text", text: "RESTART_PERSISTENCE_SMOKE" }] }, + { + role: "assistant", + content: [ + { type: "tool_use", id: "other", name: "attempt_completion", input: { result: "other" } }, + ], + }, + ], + }) + + await expect( + api.hasTaskApiConversationHistorySequence("task-1", { + userText: "RESTART_PERSISTENCE_SMOKE", + assistantToolName: "attempt_completion", + assistantToolInputText: "done", + }), + ).resolves.toBe(false) + }) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index 74ea2e7680..7a173cea1c 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -14,6 +14,7 @@ import { type ProviderSettingsEntry, type TaskEvent, type CreateTaskOptions, + type TaskApiConversationHistorySequence, type WebviewThemeFixture, RooCodeEventName, TaskCommandName, @@ -251,6 +252,39 @@ export class API extends EventEmitter implements RooCodeAPI { } } + /** Checks persisted turn ordering without exposing conversation contents to tests. */ + public async hasTaskApiConversationHistorySequence( + taskId: string, + sequence: TaskApiConversationHistorySequence, + ): Promise { + try { + const { apiConversationHistory } = await this.sidebarProvider.getTaskWithId(taskId) + const userTurnIndex = apiConversationHistory.findIndex( + (message) => + message.role === "user" && + Array.isArray(message.content) && + message.content.some((block) => block.type === "text" && block.text.includes(sequence.userText)), + ) + if (userTurnIndex < 0) return false + + return apiConversationHistory + .slice(userTurnIndex + 1) + .some( + (message) => + message.role === "assistant" && + Array.isArray(message.content) && + message.content.some( + (block) => + block.type === "tool_use" && + block.name === sequence.assistantToolName && + JSON.stringify(block.input).includes(sequence.assistantToolInputText), + ), + ) + } catch { + return false + } + } + public getCurrentTaskStack() { return this.sidebarProvider.getCurrentTaskStack() }