-
Notifications
You must be signed in to change notification settings - Fork 83
fix: implement proper error handling. #1115
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
c334564
2e7abc7
f616116
047756f
69c52a1
c91d35b
f610602
b7aa874
4a39405
3fe503a
f6df4a7
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 |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| * Will be deleted or merged. | ||
| */ | ||
|
|
||
| import * as path from 'path' | ||
|
Check warning on line 6 in server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts
|
||
| import { | ||
| ChatTriggerType, | ||
| GenerateAssistantResponseCommandInput, | ||
|
|
@@ -101,6 +101,7 @@ | |
| import { FsWrite, FsWriteParams, getDiffChanges } from './tools/fsWrite' | ||
| import { ExecuteBash, ExecuteBashOutput, ExecuteBashParams } from './tools/executeBash' | ||
| import { ExplanatoryParams, InvokeOutput, ToolApprovalException } from './tools/toolShared' | ||
| import { ModelServiceException } from './errors' | ||
| import { FileSearch, FileSearchParams } from './tools/fileSearch' | ||
| import { GrepSearch, SanitizedRipgrepOutput } from './tools/grepSearch' | ||
|
|
||
|
|
@@ -337,8 +338,20 @@ | |
| chatResultStream | ||
| ) | ||
| } catch (err) { | ||
| // TODO: On ToolValidationException, we want to show custom mynah-ui components making it clear it was cancelled. | ||
| if (CancellationError.isUserCancelled(err) || err instanceof ToolApprovalException) { | ||
| // HACK: the chat-client needs to have a partial event with the associated messageId sent before it can accept the final result. | ||
| // Without this, the `thinking` indicator never goes away. | ||
| // Note: buttons being explicitly empty is required for this hack to work. | ||
| const errorMessageId = `error-message-id-${uuid()}` | ||
| await this.#sendProgressToClient( | ||
| { | ||
| type: 'answer', | ||
| body: '', | ||
| messageId: errorMessageId, | ||
| buttons: [], | ||
| }, | ||
| params.partialResultToken | ||
| ) | ||
| if (this.isUserAction(err, token)) { | ||
| /** | ||
| * when the session is aborted it generates an error. | ||
| * we need to resolve this error with an answer so the | ||
|
|
@@ -347,9 +360,11 @@ | |
| return { | ||
| type: 'answer', | ||
| body: '', | ||
| messageId: errorMessageId, | ||
| buttons: [], | ||
| } | ||
| } | ||
| return this.#handleRequestError(err, params.tabId, metric) | ||
| return this.#handleRequestError(err, errorMessageId, params.tabId, metric) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -445,10 +460,9 @@ | |
| } | ||
|
|
||
| // Phase 3: Request Execution | ||
| this.#debug(`Request Input: ${JSON.stringify(currentRequestInput)}`) | ||
|
|
||
| const response = await session.generateAssistantResponse(currentRequestInput) | ||
| this.#debug(`Response received for iteration ${iterationCount}:`, JSON.stringify(response.$metadata)) | ||
| const response = await this.fetchModelResponse(currentRequestInput, i => | ||
| session.generateAssistantResponse(i) | ||
| ) | ||
|
|
||
| // remove the temp loading message when we have response | ||
| if (loadingMessageId) { | ||
|
|
@@ -699,8 +713,8 @@ | |
| this.#features.chat.sendChatUpdate({ tabId, state: { inProgress: false } }) | ||
| loadingMessageId = undefined | ||
| } | ||
| // If we did not approve a tool to be used or the user stopped the response, bubble this up to interrupt agentic loop | ||
| if (CancellationError.isUserCancelled(err) || err instanceof ToolApprovalException) { | ||
|
|
||
| if (this.isUserAction(err, token)) { | ||
| if (err instanceof ToolApprovalException && toolUse.name === 'executeBash') { | ||
| if (buttonBlockId) { | ||
| await chatResultStream.overwriteResultBlock( | ||
|
|
@@ -714,9 +728,6 @@ | |
| throw err | ||
| } | ||
| const errMsg = err instanceof Error ? err.message : 'unknown error' | ||
| await chatResultStream.writeResultBlock({ | ||
| body: toolErrorMessage(toolUse, errMsg), | ||
| }) | ||
| this.#log(`Error running tool ${toolUse.name}:`, errMsg) | ||
| results.push({ | ||
| toolUseId: toolUse.toolUseId, | ||
|
|
@@ -729,6 +740,34 @@ | |
| return results | ||
| } | ||
|
|
||
| /** | ||
| * Determines if error is thrown as a result of a user action (Ex. rejecting tool, stop button) | ||
| * @param err | ||
| * @returns | ||
| */ | ||
| isUserAction(err: unknown, token?: CancellationToken): boolean { | ||
| return ( | ||
| CancellationError.isUserCancelled(err) || | ||
| err instanceof ToolApprovalException || | ||
| (token?.isCancellationRequested ?? false) | ||
| ) | ||
| } | ||
|
|
||
| async fetchModelResponse<RequestType, ResponseType>( | ||
| requestInput: RequestType, | ||
| makeRequest: (requestInput: RequestType) => Promise<ResponseType> | ||
| ): Promise<ResponseType> { | ||
| this.#debug(`Q Backend Request: ${JSON.stringify(requestInput)}`) | ||
| try { | ||
| const response = await makeRequest(requestInput) | ||
| this.#debug(`Q Backend Response: ${JSON.stringify(response)}`) | ||
| return response | ||
| } catch (e) { | ||
| this.#features.logging.error(`Error in call: ${JSON.stringify(e)}`) | ||
| throw new ModelServiceException(e as Error) | ||
|
Contributor
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. Technically, we also throw Additionally, is exception thrown from Q API user-friendly enough to show it to user?
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. We were asking the same question. We reached out the backend team and they said this should be okay for now, but we'll likely want something better in the long run. I was hoping to find a better solution here, but wasn't able to given the time constraints. |
||
| } | ||
| } | ||
|
|
||
| #validateToolResult(toolUse: ToolUse, result: ToolResultContentBlock) { | ||
| let maxToolResponseSize | ||
| switch (toolUse.name) { | ||
|
|
@@ -1111,6 +1150,7 @@ | |
| */ | ||
| #handleRequestError( | ||
| err: any, | ||
| errorMessageId: string, | ||
| tabId: string, | ||
| metric: Metric<CombinedConversationEvent> | ||
| ): ChatResult | ResponseError<ChatResult> { | ||
|
|
@@ -1119,12 +1159,23 @@ | |
| this.#telemetryController.emitMessageResponseError(tabId, metric.metric, err.requestId, err.message) | ||
| } | ||
|
|
||
| if (err instanceof AmazonQServicePendingSigninError) { | ||
| // return non-model errors back to the client as errors | ||
| if (!(err instanceof ModelServiceException)) { | ||
Hweinstock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| this.#log(`unknown error ${err instanceof Error ? JSON.stringify(err) : 'unknown'}`) | ||
| this.#debug(`stack ${err instanceof Error ? JSON.stringify(err.stack) : 'unknown'}`) | ||
| this.#debug(`cause ${err instanceof Error ? JSON.stringify(err.cause) : 'unknown'}`) | ||
| return new ResponseError<ChatResult>( | ||
| LSPErrorCodes.RequestFailed, | ||
| err instanceof Error ? err.message : 'Unknown request error' | ||
|
Contributor
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. Are there any plans to add something like
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. it sounds like their isn't yet a reliable way to distinguish errors produced by the backend yet (source). So rather than rely on their errors, I refactored this to wrap their error and include a My intention with this error handling is to do as much on the language server side as possible, such that the client will only have to render the
Contributor
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. That makes. But for unforeseen cases, we might want to give clients an escape-hatch? Else they will need to do their own checks, or check the message text, for errors that require client-side handling (such as #1197 ) |
||
| ) | ||
| } | ||
|
|
||
| if (err.cause instanceof AmazonQServicePendingSigninError) { | ||
| this.#log(`Q Chat SSO Connection error: ${getErrorMessage(err)}`) | ||
| return createAuthFollowUpResult('full-auth') | ||
| } | ||
|
|
||
| if (err instanceof AmazonQServicePendingProfileError) { | ||
| if (err.cause instanceof AmazonQServicePendingProfileError) { | ||
| this.#log(`Q Chat SSO Connection error: ${getErrorMessage(err)}`) | ||
| const followUpResult = createAuthFollowUpResult('use-supported-auth') | ||
| // Access first element in array | ||
|
|
@@ -1140,13 +1191,14 @@ | |
| return createAuthFollowUpResult(authFollowType) | ||
| } | ||
|
|
||
| this.#log(`Q api request error ${err instanceof Error ? JSON.stringify(err) : 'unknown'}`) | ||
| this.#debug(`Q api request error stack ${err instanceof Error ? JSON.stringify(err.stack) : 'unknown'}`) | ||
| this.#debug(`Q api request error cause ${err instanceof Error ? JSON.stringify(err.cause) : 'unknown'}`) | ||
| return new ResponseError<ChatResult>( | ||
| LSPErrorCodes.RequestFailed, | ||
| err instanceof Error ? err.message : 'Unknown request error' | ||
| ) | ||
| const backendError = err.cause | ||
| // Send the backend error message directly to the client to be displayed in chat. | ||
| return { | ||
|
Contributor
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. Another, more LSP, way to handle this would be to return
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. Yeah, that makes sense. My thinking was that if we wanted any custom formatting on this message in the future it could be done once on the LSP side rather than reimplemented by each client. Is there a way to do that using Looks like this can be done by passing in a ChatResult as
Contributor
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.
|
||
| type: 'answer', | ||
| body: backendError.message, | ||
Hweinstock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| messageId: errorMessageId, | ||
| buttons: [], | ||
| } | ||
| } | ||
|
|
||
| async onInlineChatPrompt( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export class ModelServiceException { | ||
| public constructor(public readonly cause: Error) {} | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.