-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve error handling for partial ask messages (fixes #7312) #7313
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
- Replace confusing "Current ask promise was ignored" errors with more descriptive messages - These errors occur during normal operation when handling partial messages - The errors are caught by callers but the messages were confusing AI models - Fixes issue #7312 where gpt-oss-120b model was confused by these error messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this code, then reviewed it. Found nothing wrong. Ship it.
| throw new Error("Current ask promise was ignored (#1)") | ||
| // This is expected behavior when updating a partial message | ||
| // Silently return to avoid confusing error messages | ||
| return Promise.reject(new Error("Partial message update - expected behavior")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this pattern of using return Promise.reject() instead of throw new Error() consistent with the rest of the codebase? Both are functionally equivalent in async contexts, but consistency matters.
| this.updateClineMessage(lastMessage) | ||
| throw new Error("Current ask promise was ignored (#1)") | ||
| // This is expected behavior when updating a partial message | ||
| // Silently return to avoid confusing error messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Silently return" but we're actually rejecting with an error. Would "Return rejected promise with descriptive message" be more accurate?
| throw new Error("Current ask promise was ignored (#1)") | ||
| // This is expected behavior when updating a partial message | ||
| // Silently return to avoid confusing error messages | ||
| return Promise.reject(new Error("Partial message update - expected behavior")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a structured prefix like "[EXPECTED]" instead of the suffix " - expected behavior" for easier parsing by monitoring tools:
| return Promise.reject(new Error("Partial message update - expected behavior")) | |
| return Promise.reject(new Error("[EXPECTED] Partial message update")) |
| throw new Error("Current ask promise was ignored") | ||
| // This is expected behavior when multiple asks are sent in sequence | ||
| // Silently reject to avoid confusing error messages | ||
| return Promise.reject(new Error("Ask promise superseded - expected behavior")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future consideration: While marking these as "expected behavior" helps AI models, we're still using error throwing for control flow. Worth exploring alternative patterns like special return values or a different control mechanism in a future refactor?
Summary
This PR fixes issue #7312 where the error message "Current ask promise was ignored" was confusing AI models, particularly gpt-oss-120b.
Problem
When handling partial messages during streaming, the Task class was throwing errors with the message "Current ask promise was ignored". While these errors were being caught by callers (using
.catch(() => {})), the error messages were still visible to AI models and causing confusion, leading to unnecessary retries.Solution
Replace the confusing error messages with more descriptive ones that clearly indicate this is expected behavior:
These errors are still thrown (as Promise rejections) to maintain the existing control flow, but now have clearer messages that won't confuse AI models if they somehow see them.
Changes
src/core/task/Task.tsat three locations where "Current ask promise was ignored" errors were thrownTesting
npx vitest run core/task/__tests__/Task.spec.ts)npx vitest run core/tools/__tests__)Related Issues
Fixes #7312
Notes
This issue was previously fixed for other models (see CHANGELOG entry mentioning issue #2551), but resurfaced for gpt-oss-120b and potentially other OpenAI-compatible models.
Important
Improves error handling in
Task.tsby replacing vague error messages with descriptive ones to clarify expected behaviors for partial message handling.ask()inTask.tsto clarify expected behaviors for partial message handling.Task.tsexplaining expected behaviors during normal operation.This description was created by
for fa08d15. You can customize this summary. It will automatically update as commits are pushed.