chore: enforce no-floating-promises in core/task/#253
Conversation
📝 WalkthroughWalkthrough
ChangesUnhandled-rejection guards in Task.ts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Expands the @typescript-eslint/no-floating-promises “ratchet” to cover src/core/task/**, and updates Task implementation + tests to satisfy the rule without changing intended async behavior.
Changes:
- Extend
eslint.config.mjsto enforceno-floating-promisesincore/task/**/*.ts. - Mark intended fire-and-forget async calls in
Task.tswithvoid. - Await
submitUserMessage(...)in async tests to ensure actions under test complete before assertions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/eslint.config.mjs |
Adds core/task/**/*.ts to the scoped no-floating-promises enforcement block. |
src/core/task/Task.ts |
Replaces floating promises with explicit void for intentional background operations / UI updates. |
src/core/task/__tests__/Task.spec.ts |
Awaits task.submitUserMessage(...) calls in async tests to satisfy lint and improve test determinism. |
Comments suppressed due to low confidence (3)
src/core/task/Task.ts:575
startTask()/resumeTaskFromHistory()can throw/reject on unexpected failures (they rethrow in their catch blocks). Kicking them off withvoidin the constructor means those failures become unhandled promise rejections. Consider adding an explicit.catch(...)handler (while keeping the background execution) to log or route errors throughprovider.log.
if (startTask) {
this._started = true
if (task || images) {
void this.startTask(task, images)
} else if (historyItem) {
void this.resumeTaskFromHistory()
} else {
src/core/task/Task.ts:1788
startTask()is async and may reject; starting it withvoidhere risks an unhandled rejection if initialization fails. Consider handling the background promise with.catch(...)(and optionally logging via the provider) so unexpected failures don’t surface as process-level unhandled rejections.
const { task, images } = this.metadata
if (task || images) {
void this.startTask(task ?? undefined, images ?? undefined)
}
src/core/task/Task.ts:2772
presentAssistantMessage()is async and can reject (e.g., it throws whenabortis set). Calling it fire-and-forget viavoidmeans a rejection would be unhandled. Consider adding a.catch(...)handler (while keeping it non-blocking) to avoid process-level unhandled rejections during cancellation/aborts.
// Add to content and present
this.assistantMessageContent.push(partialToolUse)
this.userMessageContentReady = false
void presentAssistantMessage(this)
} else if (event.type === "tool_call_delta") {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
edelauna
left a comment
There was a problem hiding this comment.
🔥 - thanks for this much needed cleanup. I think this is also highlighting some uncovered lines of code in Task.ts do you think you could add some additional specs to make codecov happy?
Also lets address the copilot findings before merge.
Per Copilot review on #253: the void-prefixed async calls flagged by the reviewer (postStateToWebviewWithoutTaskHistory, startTask and resumeTaskFromHistory in the constructor, startTask in start(), and the nine presentAssistantMessage(this) sites in the streaming loop) can become unhandled promise rejections on failure and crash the extension host. Each flagged site now has a .catch handler that logs the rejection without re-throwing, while keeping the void prefix to satisfy no-floating-promises. presentAssistantMessage was wrapped in a small private helper, presentAssistantMessageSafe, that distinguishes the expected throw-on-abort path (silently swallowed) from real failures (logged). All nine streaming presenter call sites delegate through the helper so the rejection-handling logic lives in one place. Adds six specs under "unhandled-rejection guards on void async calls" that pin the new behavior: every catch handler is asserted to log on rejection, and the helper's abort vs non-abort branches are both exercised. Single-line fire-and-forget UI updates and the streaming presenter call sites carry /* v8 ignore next */ markers with a short rationale, since the rejection-handling logic they delegate to is covered separately by the helper specs.
|
Addressed in fd5d3c9: wired |
The five bare `void` prefixes in registerCommands.ts satisfy no-floating-promises but rely on ClineProvider.postMessageToWebview having its own try/catch — an implicit contract that a future change could break without notice. Matching #253's pattern, each void site now also installs a .catch arm that logs to outputChannel. Added a parameterized test asserting the .catch arm runs and logs when postMessageToWebview rejects, so a future regression in the implicit-swallow contract is caught at the test boundary.
|
Follow-up in The state-based check read Two new tests pin the discriminator — one for the swallow window (non-abort error +
|
The five bare `void` prefixes in registerCommands.ts satisfy no-floating-promises but rely on ClineProvider.postMessageToWebview having its own try/catch — an implicit contract that a future change could break without notice. Matching #253's pattern, each void site now also installs a .catch arm that logs to outputChannel. Added a parameterized test asserting the .catch arm runs and logs when postMessageToWebview rejects, so a future regression in the implicit-swallow contract is caught at the test boundary.
7773498 to
0847e54
Compare
Per Copilot review on #253: the void-prefixed async calls flagged by the reviewer (postStateToWebviewWithoutTaskHistory, startTask and resumeTaskFromHistory in the constructor, startTask in start(), and the nine presentAssistantMessage(this) sites in the streaming loop) can become unhandled promise rejections on failure and crash the extension host. Each flagged site now has a .catch handler that logs the rejection without re-throwing, while keeping the void prefix to satisfy no-floating-promises. presentAssistantMessage was wrapped in a small private helper, presentAssistantMessageSafe, that distinguishes the expected throw-on-abort path (silently swallowed) from real failures (logged). All nine streaming presenter call sites delegate through the helper so the rejection-handling logic lives in one place. Adds six specs under "unhandled-rejection guards on void async calls" that pin the new behavior: every catch handler is asserted to log on rejection, and the helper's abort vs non-abort branches are both exercised. Single-line fire-and-forget UI updates and the streaming presenter call sites carry /* v8 ignore next */ markers with a short rationale, since the rejection-handling logic they delegate to is covered separately by the helper specs.
cc0ea53 to
248005c
Compare
* chore: enforce no-floating-promises in activate/ First slice of a ratchet that re-enables @typescript-eslint/no-floating-promises directory by directory. The rule and type-aware linting are scoped to activate/** via a files-block in eslint.config.mjs, so pnpm lint (eslint --max-warnings=0) stays green; later PRs widen the scope. registerCommands.ts had 7 un-awaited postMessageToWebview calls: the 5 in synchronous command handlers are marked void (intentional fire-and-forget); the 2 in async handlers are awaited — one sits inside a try/catch, so awaiting routes a rejected post into the existing error handling. * test: cover changed lines in registerCommands.ts for codecov Per edelauna's review on Zoo-Code-Org#251: add specs for the void/await fixes in registerCommands.ts so codecov/patch passes. Ten new tests covering the six handlers touched (settingsButtonClicked, historyButtonClicked, marketplaceButtonClicked, focusInput, acceptInput, toggleAutoApprove) hitting all seven previously-uncovered lines. * fix(activate): wire .catch on void-prefixed postMessageToWebview sites The five bare `void` prefixes in registerCommands.ts satisfy no-floating-promises but rely on ClineProvider.postMessageToWebview having its own try/catch — an implicit contract that a future change could break without notice. Matching Zoo-Code-Org#253's pattern, each void site now also installs a .catch arm that logs to outputChannel. Added a parameterized test asserting the .catch arm runs and logs when postMessageToWebview rejects, so a future regression in the implicit-swallow contract is caught at the test boundary. * test(registerCommands): pin await semantics with deferred-promise pattern CodeRabbit nit on the test additions: the two await-asserting tests for focusInput and toggleAutoApprove only verified the call payload, so they would still pass if `await` were replaced with `void` in the handler. Switched both to a deferred-promise pattern that observes the handler's pending state before the underlying postMessageToWebview resolves — now a regression that drops the `await` would be caught at the test boundary. * fix(registerCommands): defensive error handling on toggleAutoApprove + log context Addresses two CodeRabbit review items: - toggleAutoApprove now wraps the await in try/catch logging to outputChannel, matching the defensive posture used on the other handlers (settingsButtonClicked, historyButtonClicked, etc.). An unhandled rejection from postMessageToWebview would otherwise bubble to VS Code's command dispatcher rather than being logged consistently with the rest of the file. - All postMessageToWebview failure log messages now carry a [<command-name>] prefix so multi-failure logs are unambiguous. * docs(test): match comment to setImmediate microtask flush CodeRabbit nit on the test additions: the explanatory comment said the test awaits Promise.resolve() to flush the .catch microtask but the code uses setImmediate. Updated the comment to match the actual implementation. --------- Co-authored-by: 0xMink <260166390+0xMink@users.noreply.github.com>
Per Copilot review on #253: the void-prefixed async calls flagged by the reviewer (postStateToWebviewWithoutTaskHistory, startTask and resumeTaskFromHistory in the constructor, startTask in start(), and the nine presentAssistantMessage(this) sites in the streaming loop) can become unhandled promise rejections on failure and crash the extension host. Each flagged site now has a .catch handler that logs the rejection without re-throwing, while keeping the void prefix to satisfy no-floating-promises. presentAssistantMessage was wrapped in a small private helper, presentAssistantMessageSafe, that distinguishes the expected throw-on-abort path (silently swallowed) from real failures (logged). All nine streaming presenter call sites delegate through the helper so the rejection-handling logic lives in one place. Adds six specs under "unhandled-rejection guards on void async calls" that pin the new behavior: every catch handler is asserted to log on rejection, and the helper's abort vs non-abort branches are both exercised. Single-line fire-and-forget UI updates and the streaming presenter call sites carry /* v8 ignore next */ markers with a short rationale, since the rejection-handling logic they delegate to is covered separately by the helper specs.
The helper's catch handler was distinguishing abort from real
failures via `this.abort` state at catch time, which has a real
TOCTOU window: a non-abort throw followed by an abort flip
between throw and catch microtask would silently swallow the
real error.
Switched to matching `error.message.endsWith("aborted")` —
the literal contract presentAssistantMessage itself throws on
abort. Same suppression for the abort case, no swallow window
for real errors.
Added a test that pins the message-based discriminator: a
non-abort error with `this.abort = true` now correctly logs
(would have been swallowed under the state-based check). Also
added a regression test for the abort-message-match path so a
future refactor can't drift back to the state check.
The four fire-and-forget updateClineMessage calls in say()/ask() partial-message branches were void-prefixed without .catch arms. The callee's webview post is internally guarded, but its synchronous RooCodeEventName.Message emit can throw via a consumer-attached listener — so the void sites can produce unhandled rejections. Replace each `void this.updateClineMessage(...)` with an explicit .catch arm that logs the error, matching the other rejection-handling sites in this file. Two new tests pin the say() and ask() catch arms. Also pin the void on getCheckpointService with a rationale comment: its top-level try/catch returns undefined on failure, so .catch is not needed there.
248005c to
e57c772
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.spec.ts`:
- Around line 2363-2371: The test setup in Task.spec.ts restores the
console.error spy but leaves the presentAssistantMessage spy active across
cases, which can leak state into later tests. Update the relevant describe block
around the Task.prototype spies to restore presentAssistantMessage as well,
either by calling mockRestore on that spy in afterEach or by replacing the
targeted cleanup with vi.restoreAllMocks() so all spies are reset between tests.
In `@src/core/task/Task.ts`:
- Around line 2753-2759: The abort handling in nextChunkWithAbort() / the
Promise.race chunk path leaves an abort listener attached when the chunk promise
wins, which can accumulate across long streams. Refactor the
signal.addEventListener("abort", ...) usage to keep a named onAbort handler,
wrap the race in a finally, and call signal.removeEventListener("abort",
onAbort) when the request completes normally. Apply the same cleanup to the
matching abort-listener block in the other referenced chunk path so both Task.ts
sites follow the same pattern.
- Around line 367-379: `presentAssistantMessageSafe()` only guards the outer
`presentAssistantMessage(this)` call, so rejections from the recursive presenter
path can still escape as unhandled. Update `presentAssistantMessage` in
`presentAssistantMessage.ts` so the internal `presentAssistantMessage(cline)`
call used for pending/auto-advanced updates is also awaited, returned, or routed
through the same guarded wrapper, matching the protection already applied in
`Task.ts` at `presentAssistantMessageSafe`.
🪄 Autofix (Beta)
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: d4a5cd29-c2ad-4539-80ba-197e67fc8076
📒 Files selected for processing (3)
src/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/eslint.config.mjs
| signal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| reject(new Error("Request cancelled by user")) | ||
| }, | ||
| { once: true }, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove abort listeners when the raced chunk wins.
{ once: true } only cleans up after abort fires. For normal streaming, each nextChunkWithAbort() call leaves its abort listener attached until request teardown, so long streams can accumulate listeners and trigger memory/listener warnings.
Suggested cleanup pattern
- signal.addEventListener(
- "abort",
- () => {
- reject(new Error("Request cancelled by user"))
- },
- { once: true },
- )
+ const onAbort = () => reject(new Error("Request cancelled by user"))
+ signal.addEventListener("abort", onAbort, { once: true })Then wrap the Promise.race(...) with finally and call signal.removeEventListener("abort", onAbort).
Also applies to: 4288-4294
🤖 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/Task.ts` around lines 2753 - 2759, The abort handling in
nextChunkWithAbort() / the Promise.race chunk path leaves an abort listener
attached when the chunk promise wins, which can accumulate across long streams.
Refactor the signal.addEventListener("abort", ...) usage to keep a named onAbort
handler, wrap the race in a finally, and call
signal.removeEventListener("abort", onAbort) when the request completes
normally. Apply the same cleanup to the matching abort-listener block in the
other referenced chunk path so both Task.ts sites follow the same pattern.
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.spec.ts`:
- Around line 2677-2679: The test in Task.spec.ts is swallowing every rejection
from task.ask, which can hide unexpected failures after updateClineMessage runs.
Update the assertion around the ask("tool", "updated partial", true) call to
explicitly expect the AskIgnoredError path from the task.ask logic, and
reference the existing partial-update flow so only the intended ignore-partial
rejection is accepted.
🪄 Autofix (Beta)
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: cbdd93e7-a0d0-465f-a0c6-94358d768448
📒 Files selected for processing (3)
src/core/assistant-message/presentAssistantMessage.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/task/Task.ts
| // Sending a new partial of the same type triggers updateClineMessage | ||
| // then throws AskIgnoredError — catch it so the test doesn't fail. | ||
| await task.ask("tool", "updated partial", true).catch(() => {}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the expected ignore-partial rejection instead of swallowing all errors.
This test is meant to pin the AskIgnoredError path, but .catch(() => {}) would also hide an unexpected rejection after updateClineMessage is called.
Proposed test tightening
- await task.ask("tool", "updated partial", true).catch(() => {})
+ await expect(task.ask("tool", "updated partial", true)).rejects.toThrow("updating existing partial")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Sending a new partial of the same type triggers updateClineMessage | |
| // then throws AskIgnoredError — catch it so the test doesn't fail. | |
| await task.ask("tool", "updated partial", true).catch(() => {}) | |
| // Sending a new partial of the same type triggers updateClineMessage | |
| // then throws AskIgnoredError — catch it so the test doesn't fail. | |
| await expect(task.ask("tool", "updated partial", true)).rejects.toThrow("updating existing partial") |
🤖 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.spec.ts` around lines 2677 - 2679, The test in
Task.spec.ts is swallowing every rejection from task.ask, which can hide
unexpected failures after updateClineMessage runs. Update the assertion around
the ask("tool", "updated partial", true) call to explicitly expect the
AskIgnoredError path from the task.ask logic, and reference the existing
partial-update flow so only the intended ignore-partial rejection is accepted.
Summary
Next slice of the
no-floating-promisesratchet (follows #251). Widens the rule's scope ineslint.config.mjsto includecore/task/**.core/task/had 25 un-awaited promises.Task.ts— 19, all markedvoidEvery one is genuine fire-and-forget, so
void(which preserves current behavior) is the correct fix:presentAssistantMessage(this)— its doc comment describes a self-locking streaming presenter designed to be called concurrently/fire-and-forget; awaiting it would be wrong.startTask/resumeTaskFromHistoryin the constructor and the synchronousstart(); awaiting isn't possible there and the task is meant to run in the background.updateClineMessage— UI message posts in the hot streaming path.awaitwould change streaming behavior;voidpreserves it, and the file already usesvoid this.updateClineMessage(...)at line 1396.setTimeoutcallback.getCheckpointService(this)— the existing comment states it kicks the work off "in the background".Task.spec.ts— 6, awaitedUn-awaited
task.submitUserMessage(...)calls inasynctests — tests should await the action under test; these are nowawaited.Test plan
eslint core/task/clean.eslint . --ext=ts --max-warnings=0(the repo lint command) passes.tsc --noEmitclean.core/task/test suite passes (17 files, 175 tests).Summary by CodeRabbit