From 352df167431c4b2144df23c975fc82e4092dc7c2 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 09:37:50 +0800 Subject: [PATCH 1/5] 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 2/5] 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 3/5] =?UTF-8?q?chore(ci):=20empty=20commit=20=E2=80=94=20r?= =?UTF-8?q?e-trigger=20CI=20and=20the=20CodeRabbit=20current-head=20review?= =?UTF-8?q?=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 4/5] 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 5/5] 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