fix(task): guard saveClineMessages against abandoned tasks (fixes #1021)

.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

src/core/task/Task.ts

@@ -1130,6 +1130,15 @@ export class Task extends EventEmitter<TaskEvents> 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)

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<string>
@@ -4297,3 +4301,149 @@ describe("pushToolResultToUserContent", () => {
 		expect(task.userMessageContent[2]).toEqual(toolResult)
 	})
 })
+
+describe("saveClineMessages abandoned guard (#1021)", () => {
+	beforeEach(() => {
+		if (!TelemetryService.hasInstance()) {
+			TelemetryService.createInstance([])
+		}
+	})
+
+	// 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",
+			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 = 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.
+			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()
+		}
+	})
+
+	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<void>((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()
+		}
+	})
+})