diff --git a/apps/vscode-e2e/src/suite/restart-persistence.test.ts b/apps/vscode-e2e/src/suite/restart-persistence.test.ts index 1f03e39e85..4778b05857 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, @@ -91,8 +86,16 @@ 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 api.resumeTask(taskId) await waitFor(() => taskMessages.some(({ type, ask }) => type === "ask" && ask === "resume_completed_task")) @@ -103,16 +106,22 @@ 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(), { 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 79ecca0a6c..d39c0fd9e2 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -6,25 +6,26 @@ Zoo Code checks task lifecycle protocols through one compositional verification pnpm lifecycle:model-check ``` -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. +- 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. @@ -78,9 +79,25 @@ 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; +- 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. @@ -90,22 +107,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. #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 +144,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/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/scripts/stryker-diff.mjs b/scripts/stryker-diff.mjs index 32211ebbd6..2d6adddfa2 100644 --- a/scripts/stryker-diff.mjs +++ b/scripts/stryker-diff.mjs @@ -293,15 +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 direct = testFiles.filter((testFile) => { + const sourceNames = sourceFiles.map((sourceFile) => + path.posix.basename(sourceFile, path.posix.extname(sourceFile)).toLowerCase(), + ) + const isDirectMatch = (testFile, sourceName) => { const testName = path.posix.basename(testFile) - return sourceNames.some( - (sourceName) => - testName.startsWith(`${sourceName}.`) && /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(testName), + const normalizedTestName = testName.toLowerCase() + return ( + (normalizedTestName.startsWith(`${sourceName}.`) || normalizedTestName.startsWith(`${sourceName}-`)) && + /\.(?:test|spec)(?:\.[^.]+)?\.[cm]?[jt]sx?$/.test(normalizedTestName) ) - }) - 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) { + return (packageEntry.testFiles?.length ?? 0) === 0 && packageEntry.vitestRelated !== false } export function resolveVitestBinary(repoRoot, packageEntry) { @@ -384,7 +394,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..931840606f 100644 --- a/scripts/stryker-diff.test.mjs +++ b/scripts/stryker-diff.test.mjs @@ -22,8 +22,10 @@ import { parseChangedLines, parseNameStatus, parseVitestTestFiles, + preferDirectTestFiles, resolveStrykerTempDir, resolveVitestBinary, + shouldUseVitestRelated, packageForPath, runManifest, selectFromGit, @@ -200,6 +202,54 @@ 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), + ) + }) + + 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", () => { + 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) + assert.equal(shouldUseVitestRelated({ testFiles: [] }), true) + }) +}) + + 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/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index a99ad2ca22..eed8127b82 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(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..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) @@ -204,6 +207,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 +256,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 = { @@ -283,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 diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 543084571d..fae796db6b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -195,6 +195,13 @@ export interface TaskOptions extends CreateTaskOptions { diffFuzzyThreshold?: number } +type AssistantMessagePersistenceResult = boolean +type AssistantMessagePersistenceCancellation = { + cancelled: boolean + promise: Promise + resolve: () => void +} + export class Task extends EventEmitter implements TaskLike { readonly taskId: string readonly rootTaskId?: string @@ -406,9 +413,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 @@ -515,6 +526,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") @@ -925,6 +937,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 @@ -984,7 +1000,11 @@ export class Task extends EventEmitter implements TaskLike { return ensureMessageIdentifiers(messages) } - 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" && @@ -1016,12 +1036,69 @@ export class Task extends EventEmitter implements TaskLike { ) } } + if (message.role === "assistant") { + this.assistantMessageSavedToHistory = saved + this.resolveAssistantMessagePersistence(saved) + } + } + + /** 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: () => { + 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.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 { + const currentCancellation = this.assistantMessagePersistenceCancellation! + if (!this.completionPersistenceReadyPromise) { + const currentPersistence = this.assistantMessagePersistencePromise + this.completionPersistenceReadyPromise = (async () => { + const result = await Promise.race([currentPersistence, currentCancellation.promise]) + if (result) return + + const retrySaved = await this.retrySaveApiConversationHistoryWithCancellation(currentCancellation) + if (!retrySaved) { + if (!currentCancellation.cancelled) { + throw new Error("Failed to persist API conversation history before task completion") + } + return + } + this.assistantMessageSavedToHistory = true + })() + } + + return this.completionPersistenceReadyPromise.then(() => !currentCancellation.cancelled) } // NOTE: We intentionally do NOT mutate stored messages to merge consecutive user turns. // 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) { @@ -1049,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 @@ -1062,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 @@ -1108,6 +1190,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({ @@ -1129,15 +1212,37 @@ export class Task extends EventEmitter implements TaskLike { * Used by delegation flow when flushPendingToolResultsToHistory reports failure. */ public async retrySaveApiConversationHistory(): Promise { + return this.retrySaveApiConversationHistoryWithCancellation() + } + + /** 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])) + if (cancellation) { + await new Promise((resolve) => { + const timer = setTimeout(resolve, delays[attempt]) + void cancellation.promise.then(() => { + clearTimeout(timer) + resolve() + }) + }) + } else { + await new Promise((resolve) => setTimeout(resolve, delays[attempt])) + } + + // Check cancellation before each save attempt + 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 false if (success) { return true @@ -1187,6 +1292,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) { @@ -1212,6 +1321,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 }) @@ -1231,6 +1344,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({ @@ -2508,6 +2622,7 @@ export class Task extends EventEmitter implements TaskLike { } this.abort = true + this.cancelAssistantMessagePersistence() this.abortPromise ??= this.abortTaskOnce() return this.abortPromise } @@ -2571,6 +2686,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 @@ -3049,6 +3165,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 +3938,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 9956b74fb7..8d3314a9a6 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,14 @@ 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 + resolveAssistantMessagePersistence: (result: boolean) => void + assistantMessagePersistenceCancellation?: { resolve: () => void } resumeTaskFromHistory: () => Promise resumePendingTaskAction: (action: PendingTaskAction) => Promise saveClineMessages: () => Promise @@ -415,6 +426,545 @@ describe("Task persistence", () => { // But the content should be the same 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("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" }) + + await expect(task.waitForCurrentAssistantMessagePersistence()).resolves.toBe(true) + void task.dispose() + 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, + 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) + + 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( + "persisting task completion", + 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() + const retryDeferred = createDeferred() + mockSaveApiMessages + .mockRejectedValueOnce(new Error("initial write failed")) + .mockReturnValueOnce(retryDeferred.promise) + 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() + + // 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(callbacks.handleError).not.toHaveBeenCalled() + expect(completionListener).toHaveBeenCalledTimes(1) + expect(task.assistantMessageSavedToHistory).toBe(true) + expect(vi.mocked(mockSaveApiMessages).mock.invocationCallOrder[1]).toBeLessThan( + vi.mocked(completionListener).mock.invocationCallOrder[0], + ) + } finally { + retryDeferred.resolve(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() + void 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() + + await vi.advanceTimersByTimeAsync(50) + void task.dispose() + + await expect(waiting).resolves.toBe(false) + await Promise.resolve() + expect(vi.getTimerCount()).toBe(0) + 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) + void 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. + await Promise.resolve() + vi.advanceTimersByTime(100) + void task.dispose() + + await expect(waiting).resolves.toBe(false) + expect(task.assistantMessageSavedToHistory).toBe(false) + await vi.runAllTimersAsync() + + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + } finally { + 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) + void task.dispose() + retrySave.resolve(undefined) + + await expect(waiting).resolves.toBe(false) + expect(task.assistantMessageSavedToHistory).toBe(false) + } finally { + retrySave.resolve(undefined) + 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 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, + 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) + expect(consoleWarn).toHaveBeenCalledWith( + expect.stringContaining("failed to persist assistant message"), + expect.any(Error), + ) + } finally { + consoleWarn.mockRestore() + 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 b3554292b1..7418920cb1 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).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..f32a6e2507 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,15 +411,26 @@ describe("flushPendingToolResultsToHistory", () => { }, ] - // Clear mock call history - mockPWaitFor.mockClear() + 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() + + // 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) - // Should have called pWaitFor since flag was false - expect(mockPWaitFor).toHaveBeenCalled() + 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) }) @@ -441,13 +454,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/core/tools/AttemptCompletionTool.ts b/src/core/tools/AttemptCompletionTool.ts index a71520b5cc..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 @@ -142,6 +147,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 +164,12 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { pushToolResult, ) if (delegation === "delegated") { - this.emitPublicTaskCompleted(task) + task.emitFinalTokenUsageUpdate() + provider.emitDelegatedTaskCompleted( + task.taskId, + task.getTokenUsage(), + task.toolUsage, + ) } if (delegation !== "continue") return } else { @@ -207,7 +225,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) { - this.emitPublicTaskCompleted(task) + try { + await this.emitPublicTaskCompleted(task) + } catch (error) { + await handleError("persisting task completion", error as Error) + } } return } @@ -234,6 +256,7 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { * Returns: * - "delegated" when completion was approved and parent resumed * - "denied" when user denied finishing the subtask + * - undefined when the persistence generation ended during approval * - "continue" when caller should fall through to normal completion ask flow */ private async delegateToParent( @@ -243,13 +266,16 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> { pendingActionId: string | undefined, askFinishSubTaskApproval: () => Promise, pushToolResult: (result: string) => void, - ): Promise<"delegated" | "denied" | "continue"> { + ): Promise<"delegated" | "denied" | "continue" | undefined> { const didApprove = await askFinishSubTaskApproval() if (!didApprove) { pushToolResult(formatResponse.toolDenied()) return "denied" } + if (!(await task.waitForCurrentAssistantMessagePersistence())) { + return + } const didReopen = await provider.reopenParentFromDelegation({ parentTaskId: task.parentTaskId!, @@ -290,10 +316,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..4bf3a89f34 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", @@ -501,12 +506,14 @@ describe("attemptCompletionTool", () => { setPendingTaskAction: vi.fn().mockResolvedValue(undefined), clearPendingTaskAction: vi.fn().mockResolvedValue(true), reopenParentFromDelegation: vi.fn().mockResolvedValue(true), + emitDelegatedTaskCompleted: vi.fn(), } Object.assign(mockTask, { taskId: "child-1", parentTaskId: "parent-1", providerRef: { deref: () => mockProvider }, + waitForCurrentAssistantMessagePersistence: vi.fn(() => persistenceReady), }) mockAskFinishSubTaskApproval.mockResolvedValue(true) @@ -519,7 +526,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", { @@ -537,6 +549,165 @@ 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, + expect.anything(), + expect.anything(), + expect.anything(), + ) + }) + + 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("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 () => { @@ -670,6 +841,7 @@ describe("attemptCompletionTool", () => { throw new Error(`unexpected task id ${id}`) }), reopenParentFromDelegation: vi.fn().mockResolvedValue(true), + emitDelegatedTaskCompleted: vi.fn(), } Object.assign(mockTask, { @@ -697,6 +869,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 () => { @@ -772,6 +945,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 +957,63 @@ 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 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", @@ -970,6 +1204,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/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 4cfd9bbe4b..b60c5eb21a 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" @@ -8,22 +9,33 @@ 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 let mockGetTaskWithId: ReturnType + 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 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) @@ -42,4 +54,282 @@ 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() + 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 () => { + 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.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("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: [ + { + 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..316e7a6c9d 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,56 @@ 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 { + let apiConversationHistory: Awaited>["apiConversationHistory"] + try { + 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 + + // Search all assistant turns that belong to the same generation: between + // 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, + ) + 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() { return this.sidebarProvider.getCurrentTaskStack() } @@ -338,6 +389,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 @@ -346,16 +408,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) })