fix: implement proper error handling. #1115
Conversation
Hweinstock
changed the title
server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts
Show resolved
Hide resolved
server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts
Show resolved
Hide resolved
server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts
Show resolved
Hide resolved
server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts
Outdated
Show resolved
Hide resolved
Hweinstock
force-pushed
the
userActionError
branch
from
1351cae to
3fe503a
Compare
| ) | ||
| const backendError = err.cause | ||
| // Send the backend error message directly to the client to be displayed in chat. | ||
| return { |
There was a problem hiding this comment.
Another, more LSP, way to handle this would be to return ResponseError in result of LSP request. Then extension would need to translate that to error on the UI by calling "errorMessage" command with params through postMessage.
There was a problem hiding this comment.
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 ResponseError?
Looks like this can be done by passing in a ChatResult as data in the error potentially?
There was a problem hiding this comment.
ResponseError has generic data param, so that might be used. Custom formatting could also be on chat-client side (here), depending on what exactly you mean.
| return response | ||
| } catch (e) { | ||
| this.#features.logging.error(`Error in call: ${JSON.stringify(e)}`) | ||
| throw new ModelServiceException(e as Error) |
There was a problem hiding this comment.
Technically, we also throw throw new Error('amazonQServiceManager is not initialized') from within generateAssistantResponse (aka makeRequest) function. Which is something that should not be show to user.
Additionally, is exception thrown from Q API user-friendly enough to show it to user?
There was a problem hiding this comment.
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.
## Problem - Some follow up work from #1115. ## Solution - Follow the LSP paradigm to return response error to the client, and handle it there. - This is in combination with client side changes [here](aws/aws-toolkit-vscode#7161). - These together produce the following experience on model api failures.
| 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' |
There was a problem hiding this comment.
Are there any plans to add something like innerCode which holds the code (sdk v2) or name (sdk v3) from the original error?
There was a problem hiding this comment.
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 code field , but only on the language server side, and is not sent back to the client.
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 ChatResult from the data field of the ResponseError. My thinking is that this will reduce the amount of custom error handling each IDE will have to implement.
There was a problem hiding this comment.
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 )
Problem
Error handling for backend requests is non-existent and the existing error handling elsewhere relies on flaky checks.
Testing these changes with @jpinkney-aws, we identified 3 problems fixed by these changes.
Solution
Testing
errorHandlingFix.mov
Notes
there is a hack implemented to ensure the thinking bubble is killed. This highlights potential bugs in the chat-client and is explained with a comment in the code.
Future Work
ResponseErrorand handling it on client side).License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.