From 352df167431c4b2144df23c975fc82e4092dc7c2 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 09:37:50 +0800 Subject: [PATCH 01/12] fix(task): guard saveClineMessages against abandoned tasks (fixes #1021) Fire-and-forget saveClineMessages() calls could execute updateTaskHistory() after abandonSubtask's atomicUpdatePair() had already cleared parentTaskId/rootTaskId, silently reattaching the severed parent-child link. Check this.abandoned before updateTaskHistory() to catch both the explicit abort save and any in-flight fire-and-forget saves. Per-task message persistence is unaffected: saveTaskMessages still runs, only the (stale) history-item update is skipped. This is the minimal upstream-main form of the fix developed on the local-usage-stats branch (commit 1d1eb915e); that commit's surrounding usage-stats changes are not part of main and are excluded. Regression test in Task.spec.ts: an abandoned task's saveClineMessages() persists messages but never calls updateTaskHistory(). --- src/core/task/Task.ts | 9 ++++ src/core/task/__tests__/Task.spec.ts | 74 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 349d9c51d3..1a3ede5294 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1130,6 +1130,15 @@ export class Task extends EventEmitter implements TaskLike { // - Final state is emitted when updates stop (trailing: true) this.debouncedEmitTokenUsage(tokenUsage, this.toolUsage) + // Guard: don't update the history item for abandoned tasks. Fire-and-forget + // saveClineMessages() calls can reach updateTaskHistory() after + // abandonSubtask's atomicUpdatePair() has already cleared + // parentTaskId/rootTaskId; writing this live Task's stale values would + // silently reattach the severed parent-child link. + if (this.abandoned) { + return false + } + const provider = this.providerRef.deref() const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 37e228f887..eba160dd7a 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -13,6 +13,7 @@ import { type GlobalState, type ProviderSettings, type ModelInfo, + type HistoryItem, type TaskLike, } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" @@ -27,6 +28,9 @@ import { ContextProxy } from "../../config/ContextProxy" import { processUserContentMentions } from "../../mentions/processUserContentMentions" import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace" import type { ApiMessage } from "../../task-persistence" +import * as taskMetadataModule from "../../task-persistence/taskMetadata" +import * as taskMessagesModule from "../../task-persistence/taskMessages" +import { getApiMetrics } from "../../../shared/getApiMetrics" type TaskTestAccess = { getSystemPrompt: () => Promise @@ -4297,3 +4301,73 @@ describe("pushToolResultToUserContent", () => { expect(task.userMessageContent[2]).toEqual(toolResult) }) }) + +describe("saveClineMessages abandoned guard (#1021)", () => { + beforeEach(() => { + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + }) + + it("persists messages but does not update task history when the task was abandoned", async () => { + // The history item carries the stale link: a fire-and-forget save that + // reaches updateTaskHistory() after abandonSubtask's atomicUpdatePair() + // cleared parentTaskId/rootTaskId would silently reattach the severed + // parent-child link. + const staleHistoryItem: HistoryItem = { + id: "orphan-subtask", + number: 7, + ts: Date.now(), + task: "orphan subtask", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + rootTaskId: "stale-root", + parentTaskId: "stale-parent", + } + const saveSpy = vi.spyOn(taskMessagesModule, "saveTaskMessages").mockResolvedValue(undefined) + const metaSpy = vi + .spyOn(taskMetadataModule, "taskMetadata") + .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) + + try { + const mockProvider = { + context: { + globalStorageUri: { fsPath: "/test/storage" }, + globalState: { + get: vi.fn().mockImplementation(() => undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + }, + getState: vi.fn().mockResolvedValue({ + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, + mcpEnabled: false, + }), + getMcpHub: vi.fn().mockReturnValue(undefined), + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + updateTaskHistory: vi.fn().mockResolvedValue(undefined), + } as unknown as MockedClineProvider + + const task = new Task({ + provider: mockProvider, + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, + task: "orphan subtask", + startTask: false, + }) + + // abandonSubtask severs the link, then aborts the subtask with + // isAbandoned=true; an in-flight fire-and-forget save lands here. + task.abandoned = true + + const saved = await getTaskTestAccess(task).saveClineMessages() + + expect(saved).toBe(false) + expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted + expect(mockProvider.updateTaskHistory).not.toHaveBeenCalled() // history link is not reattached + } finally { + saveSpy.mockRestore() + metaSpy.mockRestore() + } + }) +}) From ce34d4804c008daf4cc42841f6b2d87a18a0a6a4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 09:50:44 +0800 Subject: [PATCH 02/12] test(task): document double assertion in abandoned-guard test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per CodeRabbit review: document why the provider test double uses the as unknown as MockedClineProvider double assertion (Task receives a full ClineProvider at runtime; this focused unit test only exercises a few methods) — same pattern and rationale as the existing Subtask Rate Limiting block. --- src/core/task/__tests__/Task.spec.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index eba160dd7a..b9bb5f2ece 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -4347,6 +4347,9 @@ describe("saveClineMessages abandoned guard (#1021)", () => { getMcpHub: vi.fn().mockReturnValue(undefined), postMessageToWebview: vi.fn().mockResolvedValue(undefined), updateTaskHistory: vi.fn().mockResolvedValue(undefined), + // Task receives a full ClineProvider at runtime; this focused unit test only + // exercises these methods, so the partial double is cast (same pattern as the + // "Subtask Rate Limiting" block above). } as unknown as MockedClineProvider const task = new Task({ From b67e573125ec875229db4ae08d68b4eb8f9a7f6c Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 30 Aug 2026 12:33:03 +0800 Subject: [PATCH 03/12] =?UTF-8?q?chore(ci):=20empty=20commit=20=E2=80=94?= =?UTF-8?q?=20re-trigger=20CI=20and=20the=20CodeRabbit=20current-head=20re?= =?UTF-8?q?view=20gate=20(no=20code=20change)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From 8cce67aea9c9bbcc18d3b7e2334b42cca78148c5 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 3 Sep 2026 01:34:39 +0800 Subject: [PATCH 04/12] test(task): reach the history-update path in the abandoned-guard tests Per CodeRabbit review on PR #1382, the focused task-history tests used a provider double without taskHistoryStore, so provider?.taskHistoryStore.get() threw before the guard was evaluated and the catch returned false - the assertions passed without ever exercising updateTaskHistory. Stub taskHistoryStore.get() on the provider double (shared makeMockProvider helper) so execution reaches updateTaskHistory; add a non-abandoned control asserting updateTaskHistory() runs and the item is written as-is (covers the guard false branch); add an in-flight save test where saveTaskMessages() is deferred, the task is abandoned mid-save, and the guard must still skip the history update when the save resolves. All PR-changed lines in Task.ts (guard at 1138-1140) are now 100% covered on lines and branches: true branch hit twice (abandoned + in-flight), false branch once (control). --- src/core/task/__tests__/Task.spec.ts | 157 ++++++++++++++++++++------- 1 file changed, 115 insertions(+), 42 deletions(-) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index b9bb5f2ece..9c2089722c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -4309,55 +4309,68 @@ describe("saveClineMessages abandoned guard (#1021)", () => { } }) - it("persists messages but does not update task history when the task was abandoned", async () => { - // The history item carries the stale link: a fire-and-forget save that - // reaches updateTaskHistory() after abandonSubtask's atomicUpdatePair() - // cleared parentTaskId/rootTaskId would silently reattach the severed - // parent-child link. - const staleHistoryItem: HistoryItem = { - id: "orphan-subtask", - number: 7, - ts: Date.now(), + // The history item carries the stale link: a fire-and-forget save that + // reaches updateTaskHistory() after abandonSubtask's atomicUpdatePair() + // cleared parentTaskId/rootTaskId would silently reattach the severed + // parent-child link. + const staleHistoryItem: HistoryItem = { + id: "orphan-subtask", + number: 7, + ts: Date.now(), + task: "orphan subtask", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + rootTaskId: "stale-root", + parentTaskId: "stale-parent", + } + + // Task receives a full ClineProvider at runtime; these focused unit tests only + // exercise these methods, so the partial double is cast (same pattern as the + // "Subtask Rate Limiting" block above). taskHistoryStore must be stubbed: + // without it provider?.taskHistoryStore.get() throws before the guard is + // evaluated and the catch would mask whether execution reached + // updateTaskHistory(). + function makeMockProvider() { + const mockProvider = { + context: { + globalStorageUri: { fsPath: "/test/storage" }, + globalState: { + get: vi.fn().mockImplementation(() => undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + }, + getState: vi.fn().mockResolvedValue({ + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, + mcpEnabled: false, + }), + getMcpHub: vi.fn().mockReturnValue(undefined), + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + updateTaskHistory: vi.fn().mockResolvedValue(undefined), + taskHistoryStore: { get: vi.fn(() => undefined) }, + } as unknown as MockedClineProvider + return mockProvider + } + + function createTask(provider: MockedClineProvider) { + return new Task({ + provider, + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, task: "orphan subtask", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - rootTaskId: "stale-root", - parentTaskId: "stale-parent", - } + startTask: false, + }) + } + + it("persists messages but does not update task history when the task was abandoned", async () => { const saveSpy = vi.spyOn(taskMessagesModule, "saveTaskMessages").mockResolvedValue(undefined) const metaSpy = vi .spyOn(taskMetadataModule, "taskMetadata") .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) try { - const mockProvider = { - context: { - globalStorageUri: { fsPath: "/test/storage" }, - globalState: { - get: vi.fn().mockImplementation(() => undefined), - update: vi.fn().mockResolvedValue(undefined), - keys: vi.fn().mockReturnValue([]), - }, - }, - getState: vi.fn().mockResolvedValue({ - apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, - mcpEnabled: false, - }), - getMcpHub: vi.fn().mockReturnValue(undefined), - postMessageToWebview: vi.fn().mockResolvedValue(undefined), - updateTaskHistory: vi.fn().mockResolvedValue(undefined), - // Task receives a full ClineProvider at runtime; this focused unit test only - // exercises these methods, so the partial double is cast (same pattern as the - // "Subtask Rate Limiting" block above). - } as unknown as MockedClineProvider - - const task = new Task({ - provider: mockProvider, - apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, - task: "orphan subtask", - startTask: false, - }) + const mockProvider = makeMockProvider() + const task = createTask(mockProvider) // abandonSubtask severs the link, then aborts the subtask with // isAbandoned=true; an in-flight fire-and-forget save lands here. @@ -4373,4 +4386,64 @@ describe("saveClineMessages abandoned guard (#1021)", () => { metaSpy.mockRestore() } }) + + it("updates task history for a non-abandoned task (guard does not block the normal path)", async () => { + const saveSpy = vi.spyOn(taskMessagesModule, "saveTaskMessages").mockResolvedValue(undefined) + const metaSpy = vi + .spyOn(taskMetadataModule, "taskMetadata") + .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) + + try { + const mockProvider = makeMockProvider() + const task = createTask(mockProvider) + + const saved = await getTaskTestAccess(task).saveClineMessages() + + expect(saved).toBe(true) + expect(saveSpy).toHaveBeenCalledTimes(1) + // Control case: the guard must not block the normal path, so execution + // genuinely reached updateTaskHistory(). No pre-existing store entry, + // so the item is written as-is. + expect(mockProvider.updateTaskHistory).toHaveBeenCalledTimes(1) + expect(mockProvider.updateTaskHistory).toHaveBeenCalledWith(staleHistoryItem) + } finally { + saveSpy.mockRestore() + metaSpy.mockRestore() + } + }) + + it("skips the history update when the task is abandoned while the save is in flight", async () => { + // Fire-and-forget race: the save starts while the task is still active, + // abandonSubtask severs the link mid-save, and the guard must catch it + // when the awaited saveTaskMessages() finally resolves. + let resolveSave!: (value?: void) => void + const saveSpy = vi + .spyOn(taskMessagesModule, "saveTaskMessages") + .mockImplementation(() => new Promise((resolve) => (resolveSave = resolve))) + const metaSpy = vi + .spyOn(taskMetadataModule, "taskMetadata") + .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) + + try { + const mockProvider = makeMockProvider() + const task = createTask(mockProvider) + + const savePromise = getTaskTestAccess(task).saveClineMessages() + + // The save is in flight (awaiting saveTaskMessages) when the task is + // abandoned; only after it resumes does the save hit the guard. + task.abandoned = true + resolveSave() + + const saved = await savePromise + + expect(saved).toBe(false) + expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted + expect(mockProvider.updateTaskHistory).not.toHaveBeenCalled() // history link is not reattached + } finally { + resolveSave?.() // settle the deferred save if an assertion failed above + saveSpy.mockRestore() + metaSpy.mockRestore() + } + }) }) From ea654697e0d19b42f1daae54036144426e715f4b Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 3 Sep 2026 04:35:02 +0800 Subject: [PATCH 05/12] chore(ci): sync .coderabbit.yaml from upstream main Branch base 78c712ac4 predates #1433 (efc30cfa0, 2026-08-29), which added the CodeRabbit config to main. Without it, CodeRabbit reviews this head with defaults (request-changes workflow disabled) and can only submit COMMENTED reviews; it never submits the APPROVED review that the PR review gate requires. Sync the config (incl. #1490) so reviews on this head use the org adversarial review profile and the formal review workflow, letting the gate advance. --- .coderabbit.yaml | 157 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 157 insertions(+) create mode 100644 .coderabbit.yaml diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000000..646d3ba29a --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,157 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +language: en-US +tone_instructions: >- + Be direct and evidence-first. Report only concrete, actionable findings grounded in changed code. + Prioritize correctness, security, data loss, lifecycle, and regressions; avoid speculative style + comments and unrelated refactors. + +knowledge_base: + web_search: + enabled: true + +reviews: + profile: assertive + request_changes_workflow: true + high_level_summary: true + high_level_summary_in_walkthrough: true + review_status: true + review_details: true + collapse_walkthrough: true + changed_files_summary: true + poem: false + + auto_review: + enabled: false + drafts: false + auto_incremental_review: true + labels: + - "coderabbit-review-active" + + path_filters: + - "!**/node_modules/**" + - "!**/dist/**" + - "!**/out/**" + - "!**/coverage/**" + - "!**/.turbo/**" + - "!apps/vscode-e2e/.vscode-test/**" + - "!bin/*.vsix" + - "!webview-ui/**/__screenshots__/**" + + path_instructions: + - path: "**/*" + instructions: >- + Act as an adversarial second-opinion reviewer. Verify PR claims against implementation and + contracts. Trace changed inputs through normal, boundary, error, cancellation, retry, and + default paths and their consumers. Seek plausible counterexamples and regressions from removed + safeguards. Identify assumptions in changed code that depend on facts outside the diff. First + verify repository conventions, tests, and related implementations. When a potential finding + depends on external behavior, use web search and prefer official documentation, specifications, + or upstream repositories. Report only concrete, actionable conflicts or failure modes, citing + the relevant repository location or external source. Prioritize correctness, security, data loss, + lifecycle, and test gaps. Do not report generic best practices, unsupported concerns, speculative + style comments, or unrelated refactors. Search for existing helpers before suggesting abstractions. + + - path: "**/*.{ts,tsx,js,jsx,mts,mjs,cts,cjs}" + instructions: >- + Check strict typing and exhaustive behavior across normal, boundary, error, + cancellation, retry, and compatibility paths. Verify promises and errors are handled, + existing helpers are reused, and new code introduces no `any`, unjustified double + assertions, floating promises, duplicated helpers, or increased lint suppressions. + + - path: "{**/*.{test,spec}.{ts,tsx,js,jsx},**/__tests__/**}" + instructions: >- + Require regression coverage at the lowest valid harness with behavior-focused + assertions, including relevant negative, error, false/unset, and boundary cases. + Check cleanup and deterministic async behavior and prefer shared typed test helpers. + Visible webview changes require a durable Playwright component snapshot; behavior-only + changes do not. + Reject weak assertions on values that could take multiple forms: .toBeDefined() or + .toHaveBeenCalled() alone are not sufficient when the actual type, value, or object + identity is verifiable. For listener registration and removal, assert the same function + reference was added and removed (not expect.any(Function)). + Flag tests that assert in-flight behavior only after the call completes — these cannot + prove the behavior fires during execution. Check that describe block names match the + actual subjects of the tests they contain. + + - path: "apps/vscode-e2e/**" + instructions: >- + Reserve end-to-end coverage for behavior that requires the real VS Code host, workspace + APIs, extension activation, webview messaging, file watchers, or a full workflow. Keep + detailed protocol, parsing, storage, retry, and edge cases at lower test layers. + + - path: "{packages/types/src/**,webview-ui/src/components/settings/**,src/core/config/**,src/core/webview/**}" + instructions: >- + For persisted settings, verify the complete schema/storage/runtime/webview round trip, + shared default semantics, and focused true plus false/unset tests. SettingsView controls + must read and update local `cachedState`, include the value in the explicit save payload, + and receive the persisted value back from extension state. + + - path: "src/**" + instructions: >- + Verify extension/webview contracts, cancellation and error propagation, VS Code + lifecycle correctness, and behavior under retries and partial failure. Check listeners, + resources, and providers are disposed without stale state or duplicate work. + + - path: "webview-ui/**" + instructions: >- + Check React state and effect dependencies, cleanup, accessibility, i18n, and light/dark + theme behavior. New markup should use Tailwind; add VS Code CSS variables to + `src/index.css` before Tailwind use. Use Vitest for behavior and Playwright component + snapshots only for durable visible changes. + + - path: "{src/api/**,src/core/prompts/**,src/core/tools/**,src/services/mcp/**,src/services/destructive-command-guard/**}" + instructions: >- + Treat model, provider, MCP, path, command, and tool data as untrusted. Check approval and + allowlist bypasses, injection and traversal risks, secrets/PII exposure in logs, abort and + stream behavior, retries, provider compatibility, and enforcement at execution time—not + only at presentation or planning time. + + - path: "{src/**/{state,history,task,tasks,service,services,cache,caches,worktree,worktrees}/**,packages/**/{state,history,task,tasks,service,services,cache,caches,worktree,worktrees}/**,webview-ui/src/**/{state,history,task,tasks,service,services,cache,caches,worktree,worktrees}/**}" + instructions: >- + Check persistence and lifecycle invariants: awaited atomic writes, rollback or explicit + partial-failure behavior, cross-window state consistency, stale listeners/watchers, + cancellation, idempotency, and safe restart/resume without lost or duplicated state. + + - path: ".github/**" + instructions: >- + Require full commit SHA pins, least-privilege permissions, safe expression and shell + interpolation, and trusted metadata handling. Privileged workflows must never check out, + execute, install from, or otherwise trust a fork PR head. + + - path: "{AGENTS.md,**/AGENTS.md,CONTRIBUTING.md,.changeset/**,CHANGELOG.md,src/CHANGELOG.md}" + instructions: >- + Enforce repository policy: routine PRs must not add changesets or edit changelogs except + during release preparation. Verify documentation describes real behavior and contracts, + and deprioritize prose-only nits that do not affect correctness or usability. + + pre_merge_checks: + custom_checks: + - name: Regression evidence + mode: warning + instructions: >- + Fail only when a concrete changed behavior lacks focused coverage at the lowest valid + test layer, tests merely mirror implementation, an affected error/negative/unset branch + is omitted, or a durable visible UI change lacks its required Playwright component + snapshot. Do not demand tests for unchanged behavior, mechanical configuration, or every + branch without a plausible regression scenario. Cite the changed behavior and missing + evidence. + - name: Trust and persistence invariants + mode: error + instructions: >- + Fail only for a concrete changed path that leaks secrets or PII, trusts or executes + unvalidated input, bypasses approval or allowlist controls, can lose persisted state due + to a missing await, non-atomic write, or omitted default propagation, or leaks lifecycle + resources. Cite the path and a plausible triggering scenario; pass when no such changed + path exists. + + tools: + eslint: + enabled: true + actionlint: + enabled: true + shellcheck: + enabled: true + gitleaks: + enabled: true + semgrep: + enabled: true From a3db831f70cbd523873cee16129c6b7fe19e5217 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 28 Sep 2026 03:27:57 +0800 Subject: [PATCH 06/12] test: assert the persisted message payload in abandoned-save tests --- src/core/task/__tests__/Task.spec.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index ab3bc67bc6..9ccc9039ea 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -6572,6 +6572,11 @@ describe("saveClineMessages abandoned guard (#1021)", () => { const mockProvider = makeMockProvider() const task = createTask(mockProvider) + // Seed a message so the payload assertion is meaningful: clineMessages + // starts empty, and an empty-payload save would pass a call-count-only + // assertion even if the save dropped the messages field. + task.clineMessages = [{ ts: 1, type: "say", say: "text", text: "seeded message" }] + // abandonSubtask severs the link, then aborts the subtask with // isAbandoned=true; an in-flight fire-and-forget save lands here. task.abandoned = true @@ -6580,6 +6585,12 @@ describe("saveClineMessages abandoned guard (#1021)", () => { expect(saved).toBe(false) expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ + taskId: task.taskId, + messages: expect.arrayContaining([expect.objectContaining({ text: "seeded message" })]), + }), + ) expect(mockProvider.updateTaskHistory).not.toHaveBeenCalled() // history link is not reattached } finally { saveSpy.mockRestore() @@ -6630,6 +6641,11 @@ describe("saveClineMessages abandoned guard (#1021)", () => { const mockProvider = makeMockProvider() const task = createTask(mockProvider) + // Seed a message so the payload assertion is meaningful: clineMessages + // starts empty, and an empty-payload save would pass a call-count-only + // assertion even if the save dropped the messages field. + task.clineMessages = [{ ts: 1, type: "say", say: "text", text: "seeded message" }] + const savePromise = getTaskTestAccess(task).saveClineMessages() // The save is in flight (awaiting saveTaskMessages) when the task is @@ -6641,6 +6657,11 @@ describe("saveClineMessages abandoned guard (#1021)", () => { expect(saved).toBe(false) expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ + messages: expect.arrayContaining([expect.objectContaining({ text: "seeded message" })]), + }), + ) expect(mockProvider.updateTaskHistory).not.toHaveBeenCalled() // history link is not reattached } finally { resolveSave?.([]) // settle the deferred save if an assertion failed above From a1c71012b1e889520291e72228d36716fd67ffd4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Mon, 28 Sep 2026 03:30:00 +0800 Subject: [PATCH 07/12] test: build the abandoned-save provider double with a single structural cast --- src/core/task/__tests__/Task.spec.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 9ccc9039ea..604f288983 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -6526,13 +6526,17 @@ describe("saveClineMessages abandoned guard (#1021)", () => { } // Task receives a full ClineProvider at runtime; these focused unit tests only - // exercise these methods, so the partial double is cast (same pattern as the - // "Subtask Rate Limiting" block above). taskHistoryStore must be stubbed: - // without it provider?.taskHistoryStore.get() throws before the guard is - // evaluated and the catch would mask whether execution reached + // exercise these methods, so the partial double is cast. taskHistoryStore must + // be stubbed: without it provider?.taskHistoryStore.get() throws before the + // guard is evaluated and the catch would mask whether execution reached // updateTaskHistory(). + // + // The double does not structurally overlap MockedClineProvider, so a direct + // literal `as` fails (TS2352). Object.assign over an empty-object single cast + // keeps one assertion: the target carries the declared type and the source's + // property types stay inferred. function makeMockProvider() { - const mockProvider = { + const mockProvider = Object.assign({} as MockedClineProvider, { context: { globalStorageUri: { fsPath: "/test/storage" }, globalState: { @@ -6549,7 +6553,7 @@ describe("saveClineMessages abandoned guard (#1021)", () => { postMessageToWebview: vi.fn().mockResolvedValue(undefined), updateTaskHistory: vi.fn().mockResolvedValue(undefined), taskHistoryStore: { get: vi.fn(() => undefined) }, - } as unknown as MockedClineProvider + }) return mockProvider } From 10300ce76740a9d5dc2dfa8ad1683d725f28d979 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 13 Oct 2026 12:08:50 +0800 Subject: [PATCH 08/12] Return success from the abandoned-save guard: the write did succeed saveClineMessages documents its boolean as reflecting the message write only. The abandoned guard sits after a successful saveTaskMessages(), so returning false there misreported a durable write as a failure: callers gating on it would retry an already-persisted save (persistQueuedFeedbackAndAcknowledge burns its retry ladder and releases a queued message that is on disk) or skip a durable-state-gated update whose on-disk record is in fact current (finalizePartialToolAsk leaves the webview spinner on despite disk and memory agreeing). Return true and skip only the history write, which is the whole point of the guard. The two abandoned-save tests now assert the write-success contract. Negative control: reverting the guard to false fails exactly those two tests. Proofs: Task.spec 186 passed; tsc --noEmit clean; ESLint --max-warnings=0 clean; prettier clean on the touched files. --- src/core/task/Task.ts | 7 ++++++- src/core/task/__tests__/Task.spec.ts | 7 +++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 2bbc02d1ea..ebf140c154 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1803,7 +1803,12 @@ export class Task extends EventEmitter implements TaskLike { // parentTaskId/rootTaskId; writing this live Task's stale values would // silently reattach the severed parent-child link. if (this.abandoned) { - return false + // The message write succeeded, and the boolean reflects the message write + // only (see method docs): returning false here would make callers retry an + // already-persisted save (persistQueuedFeedbackAndAcknowledge) or skip a + // durable-state-gated update whose on-disk record is in fact current + // (finalizePartialToolAsk). Only the history write is intentionally skipped. + return true } const provider = this.providerRef.deref() diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index e6c1c798a0..aab1bf6d4c 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -8024,7 +8024,9 @@ describe("saveClineMessages abandoned guard (#1021)", () => { const saved = await getTaskTestAccess(task).saveClineMessages() - expect(saved).toBe(false) + // The boolean reflects the message write only: the write succeeded, so the + // save reports success even though the history update is skipped. + expect(saved).toBe(true) expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted expect(saveSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -8096,7 +8098,8 @@ describe("saveClineMessages abandoned guard (#1021)", () => { const saved = await savePromise - expect(saved).toBe(false) + // Write-success contract: the guard skips the history update, not the save. + expect(saved).toBe(true) expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted expect(saveSpy).toHaveBeenCalledWith( expect.objectContaining({ From 4671c9ac976ee15b14696e70d9a8408b6ac97532 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Wed, 14 Oct 2026 17:10:43 +0800 Subject: [PATCH 09/12] chore(task): drop Stryker suppression comments per maintainer directive The mutation-diff gate treats surviving mutants as advisory; the equivalence rationale previously encoded in the seven "// Stryker disable next-line" directives in this file is recorded in the PR description instead. --- src/core/task/Task.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index ebf140c154..d9923b4ecf 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -4187,7 +4187,6 @@ export class Task extends EventEmitter implements TaskLike { // under the canonical name slip past a name comparison. The first call wins. const idAlreadyUsed = this.assistantMessageContent.some( (entry) => - // Stryker disable next-line ConditionalExpression: only tool_use and mcp_tool_use entries ever carry an id — the text block pushed in the text case has no id field and the start events reaching this guard always carry the string id locked in by processRawChunk — so the type check cannot change the outcome of entry.id === event.id for any entry this pipeline produces; the false replacement is additionally killed by the rejection tests. (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4240,7 +4239,6 @@ export class Task extends EventEmitter implements TaskLike { // keep the deprecated single-entry-per-ID lookup. let streamingKey: string | undefined - // Stryker disable next-line ConditionalExpression: event.name is always a string on this path — the only delta events reaching this handler are emitted by processRawChunk with the tracked start name (never undefined), so the legacy nameless branch below is unreachable defensive code and the conditional always takes the named side. if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4257,7 +4255,6 @@ export class Task extends EventEmitter implements TaskLike { } const partialToolUse = - // Stryker disable next-line ConditionalExpression: streamingKey can never be undefined here — makeStreamingKey always returns a string and the legacy lookup branch above is unreachable (see the directive on the event.name check) — so the null side is unreachable defensive code. streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4266,7 +4263,6 @@ export class Task extends EventEmitter implements TaskLike { nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression, EqualityOperator: streamingKey is always a defined string on this path (makeStreamingKey never returns undefined; the legacy branch is unreachable per the directives above), and processStreamingChunk returns a partial ToolUse for a known key, so both defensive conditions reduce to the partialToolUse check and cannot change the observed behavior. if (partialToolUse && streamingKey !== undefined) { // Reuse the compound key built above: it is exactly the key the start event // registered under, so the delta lookup needs no second encoding. @@ -4657,7 +4653,6 @@ export class Task extends EventEmitter implements TaskLike { if (event.type === "tool_call_end") { // Create compound key for deduplication (same pattern as streaming handler). // End events carry the tool name; fall back to the id for events that don't. - // Stryker disable next-line LogicalOperator: event.name is always a string on this path — finalizeRawChunks emits end events with the tracked start name (a string, set before the tracker can start), so the ?? event.id fallback guards a nameless end event this pipeline cannot produce. let eventName = event.name ?? event.id let dedupKey = NativeToolCallParser.makeStreamingKey(event.id, eventName) @@ -4674,13 +4669,11 @@ export class Task extends EventEmitter implements TaskLike { // name (provider quirk), the compound-key lookup above misses and both the // parser entry and the dedup tracking would stay stale. Resolve the tracked // entry by id and target the real compound key for finalization and cleanup. - // Stryker disable next-line ConditionalExpression, LogicalOperator, EqualityOperator: the both-miss case is unreachable via the raw-chunk pipeline — end events carry the locked start name so the compound key resolves the registered entry, and a same-ID rejected-start end is always processed after the accepted start's end has already removed that entry; this defensive path protects against a cross-name end event the pipeline cannot produce. if (finalToolUse === null && toolUseIndex === undefined) { const resolved = NativeToolCallParser.getStreamingToolCallById( event.id, nativeToolCallParserScope, ) - // Stryker disable next-line ConditionalExpression: unreachable defensive region — getStreamingToolCallById can only resolve when the both-miss case above is reachable, which the raw-chunk pipeline cannot produce (see the directive there). if (resolved) { eventName = resolved.name dedupKey = NativeToolCallParser.makeStreamingKey(resolved.id, resolved.name) From d5e5a54e003ba89baf9f75f355bad6f8df2a34e4 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 22 Oct 2026 15:32:55 +0800 Subject: [PATCH 10/12] style: apply repo Prettier to the merge resolution --- src/core/task/Task.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 5b27e6fe1f..734a0dfb2e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -4280,7 +4280,6 @@ export class Task extends EventEmitter implements TaskLike { // under the canonical name slip past a name comparison. The first call wins. const idAlreadyUsed = this.assistantMessageContent.some( (entry) => - (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4333,7 +4332,6 @@ export class Task extends EventEmitter implements TaskLike { // keep the deprecated single-entry-per-ID lookup. let streamingKey: string | undefined - if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4350,7 +4348,6 @@ export class Task extends EventEmitter implements TaskLike { } const partialToolUse = - streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4359,7 +4356,6 @@ export class Task extends EventEmitter implements TaskLike { nativeToolCallParserScope, ) - if (partialToolUse && streamingKey !== undefined) { // Reuse the compound key built above: it is exactly the key the start event // registered under, so the delta lookup needs no second encoding. From 34dce6e8b76d50f1402e0d9dd3b8a51c404b0832 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 22 Oct 2026 16:59:38 +0800 Subject: [PATCH 11/12] chore: keep the out-of-scope Stryker directive removals out of this fix CodeRabbit Out of Scope Changes check: issue #1021 covers the abandoned-save race only. Restore the seven directives that the carried-over cleanup commit had removed; the directive removal is handled with evidence by #2054. --- src/core/task/Task.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 734a0dfb2e..005e32e65e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -4280,6 +4280,7 @@ export class Task extends EventEmitter implements TaskLike { // under the canonical name slip past a name comparison. The first call wins. const idAlreadyUsed = this.assistantMessageContent.some( (entry) => + // Stryker disable next-line ConditionalExpression: equivalent mutant, only tool_use and mcp_tool_use entries carry an id, so the type check cannot change entry.id === event.id (entry.type === "tool_use" || entry.type === "mcp_tool_use") && (entry as { id?: string }).id === event.id, ) @@ -4332,6 +4333,7 @@ export class Task extends EventEmitter implements TaskLike { // keep the deprecated single-entry-per-ID lookup. let streamingKey: string | undefined + // Stryker disable next-line ConditionalExpression: equivalent mutant, event.name is always a string here, so the legacy nameless branch is unreachable if (event.name !== undefined) { streamingKey = NativeToolCallParser.makeStreamingKey(event.id, event.name) } else { @@ -4348,6 +4350,7 @@ export class Task extends EventEmitter implements TaskLike { } const partialToolUse = + // Stryker disable next-line ConditionalExpression: equivalent mutant, streamingKey is always a string here (makeStreamingKey), so the null side is unreachable streamingKey === undefined ? null : NativeToolCallParser.processStreamingChunk( @@ -4356,6 +4359,7 @@ export class Task extends EventEmitter implements TaskLike { nativeToolCallParserScope, ) + // Stryker disable next-line ConditionalExpression, EqualityOperator: equivalent mutants, streamingKey is always defined here, so both conditions reduce to the partialToolUse check if (partialToolUse && streamingKey !== undefined) { // Reuse the compound key built above: it is exactly the key the start event // registered under, so the delta lookup needs no second encoding. @@ -4752,7 +4756,7 @@ export class Task extends EventEmitter implements TaskLike { if (event.type === "tool_call_end") { // Create compound key for deduplication (same pattern as streaming handler). // End events carry the tool name; fall back to the id for events that don't. - + // Stryker disable next-line LogicalOperator: equivalent mutant, finalizeRawChunks emits end events with a string name, so the ?? event.id fallback is unreachable let eventName = event.name ?? event.id let dedupKey = NativeToolCallParser.makeStreamingKey(event.id, eventName) @@ -4769,12 +4773,13 @@ export class Task extends EventEmitter implements TaskLike { // name (provider quirk), the compound-key lookup above misses and both the // parser entry and the dedup tracking would stay stale. Resolve the tracked // entry by id and target the real compound key for finalization and cleanup. - + // Stryker disable next-line ConditionalExpression, LogicalOperator, EqualityOperator: equivalent mutants, the both-miss case is unreachable via the raw-chunk pipeline if (finalToolUse === null && toolUseIndex === undefined) { const resolved = NativeToolCallParser.getStreamingToolCallById( event.id, nativeToolCallParserScope, ) + // Stryker disable next-line ConditionalExpression: unreachable defensive region — getStreamingToolCallById can only resolve when the both-miss case above is reachable, which the raw-chunk pipeline cannot produce (see the directive there). if (resolved) { eventName = resolved.name dedupKey = NativeToolCallParser.makeStreamingKey(resolved.id, resolved.name) From 737fdd21c57c85cc2b390b029abb2b8ed60d9d12 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:50:39 +0800 Subject: [PATCH 12/12] test(task): drive the abandoned-save tests through requestAbort The two abandoned-save tests set task.abandoned directly. Use the public abandonment transition (requestAbort(true)) so the tests exercise the same path the product takes, while the active-task control case stays untouched. --- src/core/task/__tests__/Task.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 3fc9820f40..942827a3bd 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -8337,7 +8337,7 @@ describe("saveClineMessages abandoned guard (#1021)", () => { // abandonSubtask severs the link, then aborts the subtask with // isAbandoned=true; an in-flight fire-and-forget save lands here. - task.abandoned = true + task.requestAbort(true) const saved = await getTaskTestAccess(task).saveClineMessages() @@ -8410,7 +8410,7 @@ describe("saveClineMessages abandoned guard (#1021)", () => { // The save is in flight (awaiting saveTaskMessages) when the task is // abandoned; only after it resumes does the save hit the guard. - task.abandoned = true + task.requestAbort(true) resolveSave([]) const saved = await savePromise