-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -686,14 +686,18 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||||
| // saves, and only post parts of partial message instead of | ||||||
| // whole array in new listener. | ||||||
| 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 | ||||||
| return Promise.reject(new Error("Partial message update - expected behavior")) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this pattern of using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Suggested change
|
||||||
| } else { | ||||||
| // This is a new partial message, so add it with partial | ||||||
| // state. | ||||||
| askTs = Date.now() | ||||||
| this.lastMessageTs = askTs | ||||||
| await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, partial, isProtected }) | ||||||
| throw new Error("Current ask promise was ignored (#2)") | ||||||
| // This is expected behavior when creating a new partial message | ||||||
| // Silently return to avoid confusing error messages | ||||||
| return Promise.reject(new Error("New partial message - expected behavior")) | ||||||
| } | ||||||
| } else { | ||||||
| if (isUpdatingPreviousPartial) { | ||||||
|
|
@@ -797,7 +801,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||||
| // Could happen if we send multiple asks in a row i.e. with | ||||||
| // command_output. It's important that when we know an ask could | ||||||
| // fail, it is handled gracefully. | ||||||
| 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")) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||
| } | ||||||
|
|
||||||
| const result = { response: this.askResponse!, text: this.askResponseText, images: this.askResponseImages } | ||||||
|
|
||||||
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?