-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix: suppress 'ask promise was ignored' error in handleError #9914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Review complete. No new issues found. The latest commit introduces a typed
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
|
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. |
|
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.
|
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. |
Summary
The
Task.ask()method throws"Current ask promise was ignored"as a control flow mechanism for:While callers already handle this with
.catch(() => {}), the error was leaking throughhandleError()inpresentAssistantMessage.ts, causing it to be reported to the LLM and confuse the conversation.Changes
Modified
handleError()callbacks inpresentAssistantMessage.tsto silently ignore this expected control flow error.Why This Approach
The "proper" fix would be to refactor
Task.ask()to returnundefinedinstead 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:Testing
Important
Suppress 'ask promise was ignored' error in
presentAssistantMessage.tsby introducingAskIgnoredErrorand updatingTask.ask()inTask.ts.AskIgnoredErrorinAskIgnoredError.tsfor control flow inTask.ask().handleError()inpresentAssistantMessage.tsto ignoreAskIgnoredError.AskIgnoredErrorinTask.ask()inTask.tsfor partial and superseded asks.This description was created by
for 057104b. You can customize this summary. It will automatically update as commits are pushed.