Skip to content

Conversation

@daniel-lxs
Copy link
Member

@daniel-lxs daniel-lxs commented Dec 8, 2025

Summary

The Task.ask() method throws "Current ask promise was ignored" as a control flow mechanism for:

  1. Partial UI updates (fire-and-forget progress messages)
  2. Superseded asks (when a newer ask invalidates an older one)

While callers already handle this with .catch(() => {}), the error was leaking through handleError() in presentAssistantMessage.ts, causing it to be reported to the LLM and confuse the conversation.

Changes

Modified handleError() callbacks in presentAssistantMessage.ts to silently ignore this expected control flow error.

Why This Approach

The "proper" fix would be to refactor Task.ask() to return undefined instead of throwing, then update all 21+ callers to handle the return value instead of using .catch(() => {}). However, this is a large refactor with risk of introducing regressions.

The handleError() suppression is a targeted, safe fix because:

  • The error is intentional and expected (all callers already catch and ignore it)
  • The error does nothing useful when caught (no state reset, no cleanup)
  • The fix prevents the symptom (error leaking to LLM) without architectural risk

Testing

  • All 4534 tests pass
  • Verified the fix prevents the error from appearing in tool results

Important

Suppress 'ask promise was ignored' error in presentAssistantMessage.ts by introducing AskIgnoredError and updating Task.ask() in Task.ts.

  • Error Handling:
    • Introduce AskIgnoredError in AskIgnoredError.ts for control flow in Task.ask().
    • Modify handleError() in presentAssistantMessage.ts to ignore AskIgnoredError.
  • Task Class:
    • Replace generic error with AskIgnoredError in Task.ask() in Task.ts for partial and superseded asks.

This description was created by Ellipsis for 057104b. You can customize this summary. It will automatically update as commits are pushed.

The Task.ask() method throws this error as control flow for partial UI
updates and superseded asks. While callers handle this with .catch(() => {}),
it was leaking through handleError() and being reported to the LLM.

This change silently ignores this expected control flow error in handleError()
since it's not a real error that needs user or LLM notification.
@daniel-lxs daniel-lxs requested review from cte, jr and mrubens as code owners December 8, 2025 16:01
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Dec 8, 2025
@roomote
Copy link
Contributor

roomote bot commented Dec 8, 2025

Rooviewer Clock   See task on Roo Cloud

Review complete. No new issues found.

The latest commit introduces a typed AskIgnoredError class and updates handleError() to use instanceof AskIgnoredError instead of string matching. This addresses the feedback from @cte and @mrubens requesting a less fragile check. The implementation is correct:

  • AskIgnoredError properly extends Error with correct prototype chain for instanceof checks
  • Both handleError() functions in presentAssistantMessage.ts use instanceof AskIgnoredError
  • Task.ask() throws AskIgnoredError with descriptive reasons ("updating existing partial", "new partial", "superseded")
Previous reviews

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Dec 8, 2025
@cte
Copy link
Collaborator

cte commented Dec 8, 2025

If we're actually using that particular exception as control flow then I'd consider typing the error so you can use a less fragile check (i.e. instanceof ErrorType).

@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Review] in Roo Code Roadmap Dec 8, 2025
@hannesrudolph hannesrudolph added PR - Needs Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Dec 8, 2025
@mrubens
Copy link
Collaborator

mrubens commented Dec 8, 2025

Agree on a less fragile check, sounds good otherwise

- Create AskIgnoredError class for the ask-ignored control flow signal
- Throw AskIgnoredError instead of generic Error in Task.ask()
- Use instanceof AskIgnoredError for type-safe detection in handleError()

This is more robust than string matching because instanceof is type-safe
and won't accidentally match unrelated errors.
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Dec 8, 2025
@dcbartlett
Copy link
Contributor

I was able to get this error to trigger without a "newer ask" being sent to the user. I'll investigate this a bit more, but I think there is a bit more to this than just hiding the error in this case.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 9, 2025
@mrubens mrubens merged commit ee48b3a into main Dec 9, 2025
16 checks passed
@mrubens mrubens deleted the fix/suppress-ask-promise-ignored-error branch December 9, 2025 01:19
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Dec 9, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants