From 04644c223203fe4029d72a23b33e8b1f8a624fac Mon Sep 17 00:00:00 2001 From: Tai Lai Date: Fri, 3 Jan 2025 16:57:27 -0800 Subject: [PATCH 1/5] fix(amazonq): codefix does not do error handling --- ...-38581ab6-e9f3-4b45-aef9-5f93e4a97a26.json | 4 + packages/core/resources/css/securityIssue.css | 7 + .../codewhisperer/commands/basicCommands.ts | 25 +- .../codewhisperer/service/codeFixHandler.ts | 15 +- .../securityIssue/securityIssueWebview.ts | 24 +- .../views/securityIssue/vue/root.vue | 20 +- .../commands/basicCommands.test.ts | 306 +++++++++--------- 7 files changed, 206 insertions(+), 195 deletions(-) create mode 100644 packages/amazonq/.changes/next-release/Bug Fix-38581ab6-e9f3-4b45-aef9-5f93e4a97a26.json diff --git a/packages/amazonq/.changes/next-release/Bug Fix-38581ab6-e9f3-4b45-aef9-5f93e4a97a26.json b/packages/amazonq/.changes/next-release/Bug Fix-38581ab6-e9f3-4b45-aef9-5f93e4a97a26.json new file mode 100644 index 00000000000..6117766958c --- /dev/null +++ b/packages/amazonq/.changes/next-release/Bug Fix-38581ab6-e9f3-4b45-aef9-5f93e4a97a26.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "/review: Improved error handling for code fix operations" +} diff --git a/packages/core/resources/css/securityIssue.css b/packages/core/resources/css/securityIssue.css index d766ccd8fe2..5cd64211ae8 100644 --- a/packages/core/resources/css/securityIssue.css +++ b/packages/core/resources/css/securityIssue.css @@ -524,6 +524,13 @@ pre.center { pre.error { color: var(--vscode-diffEditorOverview-removedForeground); + background-color: var(--vscode-diffEditor-removedTextBackground); + white-space: initial; +} + +a.cursor { + cursor: pointer; + text-decoration: none; } .dot-typing { diff --git a/packages/core/src/codewhisperer/commands/basicCommands.ts b/packages/core/src/codewhisperer/commands/basicCommands.ts index 999c2c53ac5..73c81033b28 100644 --- a/packages/core/src/codewhisperer/commands/basicCommands.ts +++ b/packages/core/src/codewhisperer/commands/basicCommands.ts @@ -50,7 +50,7 @@ import { once } from '../../shared/utilities/functionUtils' import { focusAmazonQPanel } from '../../codewhispererChat/commands/registerCommands' import { removeDiagnostic } from '../service/diagnosticsProvider' import { SsoAccessTokenProvider } from '../../auth/sso/ssoAccessTokenProvider' -import { ToolkitError, getTelemetryReason, getTelemetryReasonDesc } from '../../shared/errors' +import { ToolkitError, getErrorMsg, getTelemetryReason, getTelemetryReasonDesc } from '../../shared/errors' import { isRemoteWorkspace } from '../../shared/vscode/env' import { isBuilderIdConnection } from '../../auth/connection' import globals from '../../shared/extensionGlobals' @@ -67,6 +67,7 @@ import { startCodeFixGeneration } from './startCodeFixGeneration' import { DefaultAmazonQAppInitContext } from '../../amazonq/apps/initContext' import path from 'path' import { parsePatch } from 'diff' +import type { AWSError } from 'aws-sdk' const MessageTimeOut = 5_000 @@ -677,7 +678,8 @@ export const generateFix = Commands.declare( }) await updateSecurityIssueWebview({ isGenerateFixLoading: true, - isGenerateFixError: false, + // eslint-disable-next-line unicorn/no-null + generateFixError: null, context: context.extensionContext, filePath: targetFilePath, shouldRefreshView: false, @@ -737,22 +739,23 @@ export const generateFix = Commands.declare( await updateSecurityIssueWebview({ issue: targetIssue, isGenerateFixLoading: false, - isGenerateFixError: true, + generateFixError: getErrorMsg(err as AWSError, true), filePath: targetFilePath, context: context.extensionContext, - shouldRefreshView: true, + shouldRefreshView: false, }) SecurityIssueProvider.instance.updateIssue(targetIssue, targetFilePath) SecurityIssueTreeViewProvider.instance.refresh() throw err + } finally { + telemetry.record({ + component: targetSource, + detectorId: targetIssue.detectorId, + findingId: targetIssue.findingId, + ruleId: targetIssue.ruleId, + variant: refresh ? 'refresh' : undefined, + }) } - telemetry.record({ - component: targetSource, - detectorId: targetIssue.detectorId, - findingId: targetIssue.findingId, - ruleId: targetIssue.ruleId, - variant: refresh ? 'refresh' : undefined, - }) }) } ) diff --git a/packages/core/src/codewhisperer/service/codeFixHandler.ts b/packages/core/src/codewhisperer/service/codeFixHandler.ts index 0358d8d3ed9..e6aa5738db4 100644 --- a/packages/core/src/codewhisperer/service/codeFixHandler.ts +++ b/packages/core/src/codewhisperer/service/codeFixHandler.ts @@ -8,12 +8,7 @@ import * as CodeWhispererConstants from '../models/constants' import { codeFixState } from '../models/model' import { getLogger, sleep } from '../../shared' import { ArtifactMap, CreateUploadUrlRequest, DefaultCodeWhispererClient } from '../client/codewhisperer' -import { - CodeFixJobStoppedError, - CodeFixJobTimedOutError, - CreateCodeFixError, - CreateUploadUrlError, -} from '../models/errors' +import { CodeFixJobStoppedError, CodeFixJobTimedOutError } from '../models/errors' import { uploadArtifactToS3 } from './securityScanHandler' export async function getPresignedUrlAndUpload( @@ -28,8 +23,8 @@ export async function getPresignedUrlAndUpload( } getLogger().verbose(`Prepare for uploading src context...`) const srcResp = await client.createUploadUrl(srcReq).catch((err) => { - getLogger().error(`Failed getting presigned url for uploading src context. Request id: ${err.requestId}`) - throw new CreateUploadUrlError(err) + getLogger().error('Failed getting presigned url for uploading src context. %O', err) + throw err }) getLogger().verbose(`CreateUploadUrlRequest requestId: ${srcResp.$response.requestId}`) getLogger().verbose(`Complete Getting presigned Url for uploading src context.`) @@ -60,8 +55,8 @@ export async function createCodeFixJob( } const resp = await client.startCodeFixJob(req).catch((err) => { - getLogger().error(`Failed creating code fix job. Request id: ${err.requestId}`) - throw new CreateCodeFixError() + getLogger().error('Failed creating code fix job. %O', err) + throw err }) getLogger().info(`AmazonQ generate fix Request id: ${resp.$response.requestId}`) return resp diff --git a/packages/core/src/codewhisperer/views/securityIssue/securityIssueWebview.ts b/packages/core/src/codewhisperer/views/securityIssue/securityIssueWebview.ts index 7c1c655a937..d511bd9a5f6 100644 --- a/packages/core/src/codewhisperer/views/securityIssue/securityIssueWebview.ts +++ b/packages/core/src/codewhisperer/views/securityIssue/securityIssueWebview.ts @@ -27,12 +27,12 @@ export class SecurityIssueWebview extends VueWebview { public readonly onChangeIssue = new vscode.EventEmitter() public readonly onChangeFilePath = new vscode.EventEmitter() public readonly onChangeGenerateFixLoading = new vscode.EventEmitter() - public readonly onChangeGenerateFixError = new vscode.EventEmitter() + public readonly onChangeGenerateFixError = new vscode.EventEmitter() private issue: CodeScanIssue | undefined private filePath: string | undefined private isGenerateFixLoading: boolean = false - private isGenerateFixError: boolean = false + private generateFixError: string | null | undefined = undefined public constructor() { super(SecurityIssueWebview.sourcePath) @@ -99,13 +99,13 @@ export class SecurityIssueWebview extends VueWebview { this.onChangeGenerateFixLoading.fire(isGenerateFixLoading) } - public getIsGenerateFixError() { - return this.isGenerateFixError + public getGenerateFixError() { + return this.generateFixError } - public setIsGenerateFixError(isGenerateFixError: boolean) { - this.isGenerateFixError = isGenerateFixError - this.onChangeGenerateFixError.fire(isGenerateFixError) + public setGenerateFixError(generateFixError: string | null | undefined) { + this.generateFixError = generateFixError + this.onChangeGenerateFixError.fire(generateFixError) } public generateFix() { @@ -201,7 +201,7 @@ export async function showSecurityIssueWebview(ctx: vscode.ExtensionContext, iss activePanel.server.setIssue(issue) activePanel.server.setFilePath(filePath) activePanel.server.setIsGenerateFixLoading(false) - activePanel.server.setIsGenerateFixError(false) + activePanel.server.setGenerateFixError(undefined) const webviewPanel = await activePanel.show({ title: amazonqCodeIssueDetailsTabTitle, @@ -247,7 +247,7 @@ type WebviewParams = { issue?: CodeScanIssue filePath?: string isGenerateFixLoading?: boolean - isGenerateFixError?: boolean + generateFixError?: string | null shouldRefreshView: boolean context: vscode.ExtensionContext } @@ -255,7 +255,7 @@ export async function updateSecurityIssueWebview({ issue, filePath, isGenerateFixLoading, - isGenerateFixError, + generateFixError, shouldRefreshView, context, }: WebviewParams): Promise { @@ -271,8 +271,8 @@ export async function updateSecurityIssueWebview({ if (isGenerateFixLoading !== undefined) { activePanel.server.setIsGenerateFixLoading(isGenerateFixLoading) } - if (isGenerateFixError !== undefined) { - activePanel.server.setIsGenerateFixError(isGenerateFixError) + if (generateFixError !== undefined) { + activePanel.server.setGenerateFixError(generateFixError) } if (shouldRefreshView && filePath && issue) { await showSecurityIssueWebview(context, issue, filePath) diff --git a/packages/core/src/codewhisperer/views/securityIssue/vue/root.vue b/packages/core/src/codewhisperer/views/securityIssue/vue/root.vue index a086aea3089..c28d12a021d 100644 --- a/packages/core/src/codewhisperer/views/securityIssue/vue/root.vue +++ b/packages/core/src/codewhisperer/views/securityIssue/vue/root.vue @@ -48,16 +48,14 @@

Suggested code fix preview

- 
-                Something went wrong. Retry
-
+ 
{{ generateFixError }}
@@ -195,7 +193,7 @@ export default defineComponent({ endLine: 0, relativePath: '', isGenerateFixLoading: false, - isGenerateFixError: false, + generateFixError: undefined as string | null | undefined, languageId: 'plaintext', fixedCode: '', referenceText: '', @@ -218,8 +216,8 @@ export default defineComponent({ const relativePath = await client.getRelativePath() this.updateRelativePath(relativePath) const isGenerateFixLoading = await client.getIsGenerateFixLoading() - const isGenerateFixError = await client.getIsGenerateFixError() - this.updateGenerateFixState(isGenerateFixLoading, isGenerateFixError) + const generateFixError = await client.getGenerateFixError() + this.updateGenerateFixState(isGenerateFixLoading, generateFixError) const languageId = await client.getLanguageId() if (languageId) { this.updateLanguageId(languageId) @@ -249,16 +247,16 @@ export default defineComponent({ this.isGenerateFixLoading = isGenerateFixLoading this.scrollTo('codeFixSection') }) - client.onChangeGenerateFixError((isGenerateFixError) => { - this.isGenerateFixError = isGenerateFixError + client.onChangeGenerateFixError((generateFixError) => { + this.generateFixError = generateFixError }) }, updateRelativePath(relativePath: string) { this.relativePath = relativePath }, - updateGenerateFixState(isGenerateFixLoading: boolean, isGenerateFixError: boolean) { + updateGenerateFixState(isGenerateFixLoading: boolean, generateFixError: string | null | undefined) { this.isGenerateFixLoading = isGenerateFixLoading - this.isGenerateFixError = isGenerateFixError + this.generateFixError = generateFixError }, updateLanguageId(languageId: string) { this.languageId = languageId diff --git a/packages/core/src/test/codewhisperer/commands/basicCommands.test.ts b/packages/core/src/test/codewhisperer/commands/basicCommands.test.ts index 997b24b78f4..bc2dd2c4001 100644 --- a/packages/core/src/test/codewhisperer/commands/basicCommands.test.ts +++ b/packages/core/src/test/codewhisperer/commands/basicCommands.test.ts @@ -30,7 +30,7 @@ import { testCommand } from '../../shared/vscode/testUtils' import { Command, placeholder } from '../../../shared/vscode/commands2' import { SecurityPanelViewProvider } from '../../../codewhisperer/views/securityPanelViewProvider' import { DefaultCodeWhispererClient } from '../../../codewhisperer/client/codewhisperer' -import { stub } from '../../utilities/stubber' +import { Stub, stub } from '../../utilities/stubber' import { AuthUtil } from '../../../codewhisperer/util/authUtil' import { getTestWindow } from '../../shared/vscode/window' import { ExtContext } from '../../../shared/extensions' @@ -67,6 +67,7 @@ import { SecurityIssueProvider } from '../../../codewhisperer/service/securityIs import { CodeWhispererSettings } from '../../../codewhisperer/util/codewhispererSettings' import { confirm } from '../../../shared' import * as commentUtils from '../../../shared/utilities/commentUtils' +import * as startCodeFixGeneration from '../../../codewhisperer/commands/startCodeFixGeneration' describe('CodeWhisperer-basicCommands', function () { let targetCommand: Command & vscode.Disposable @@ -749,156 +750,159 @@ def execute_input_compliant(): }) }) - // describe('generateFix', function () { - // let sandbox: sinon.SinonSandbox - // let mockClient: Stub - // let filePath: string - // let codeScanIssue: CodeScanIssue - // let issueItem: IssueItem - // let updateSecurityIssueWebviewMock: sinon.SinonStub - // let updateIssueMock: sinon.SinonStub - // let refreshTreeViewMock: sinon.SinonStub - // let mockDocument: vscode.TextDocument - - // beforeEach(function () { - // sandbox = sinon.createSandbox() - // mockClient = stub(DefaultCodeWhispererClient) - // mockClient.generateCodeFix.resolves({ - // // TODO: Clean this up - // $response: {} as PromiseResult['$response'], - // suggestedRemediationDiff: 'diff', - // suggestedRemediationDescription: 'description', - // references: [], - // }) - // filePath = 'dummy/file.py' - // codeScanIssue = createCodeScanIssue({ - // findingId: randomUUID(), - // ruleId: 'dummy-rule-id', - // }) - // issueItem = new IssueItem(filePath, codeScanIssue) - // updateSecurityIssueWebviewMock = sinon.stub() - // updateIssueMock = sinon.stub() - // refreshTreeViewMock = sinon.stub() - // mockDocument = createMockDocument('dummy input') - // }) - - // afterEach(function () { - // sandbox.restore() - // }) - - // it('should call generateFix command successfully', async function () { - // sinon.stub(securityIssueWebview, 'updateSecurityIssueWebview').value(updateSecurityIssueWebviewMock) - // sinon.stub(SecurityIssueProvider.instance, 'updateIssue').value(updateIssueMock) - // sinon.stub(SecurityIssueTreeViewProvider.instance, 'refresh').value(refreshTreeViewMock) - // sinon.stub(vscode.workspace, 'openTextDocument').resolves(mockDocument) - // targetCommand = testCommand(generateFix, mockClient) - // await targetCommand.execute(codeScanIssue, filePath, 'webview') - // assert.ok(updateSecurityIssueWebviewMock.calledWith({ isGenerateFixLoading: true })) - // assert.ok( - // mockClient.generateCodeFix.calledWith({ - // sourceCode: 'dummy input', - // ruleId: codeScanIssue.ruleId, - // startLine: codeScanIssue.startLine, - // endLine: codeScanIssue.endLine, - // findingDescription: codeScanIssue.description.text, - // }) - // ) - - // const expectedUpdatedIssue = { - // ...codeScanIssue, - // suggestedFixes: [{ code: 'diff', description: 'description', references: [] }], - // } - // assert.ok( - // updateSecurityIssueWebviewMock.calledWith({ issue: expectedUpdatedIssue, isGenerateFixLoading: false }) - // ) - // assert.ok(updateIssueMock.calledWith(expectedUpdatedIssue, filePath)) - // assert.ok(refreshTreeViewMock.calledOnce) - - // assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { - // detectorId: codeScanIssue.detectorId, - // findingId: codeScanIssue.findingId, - // ruleId: codeScanIssue.ruleId, - // component: 'webview', - // result: 'Succeeded', - // }) - // }) - - // it('should call generateFix from tree view item', async function () { - // sinon.stub(securityIssueWebview, 'updateSecurityIssueWebview').value(updateSecurityIssueWebviewMock) - // sinon.stub(SecurityIssueProvider.instance, 'updateIssue').value(updateIssueMock) - // sinon.stub(SecurityIssueTreeViewProvider.instance, 'refresh').value(refreshTreeViewMock) - // sinon.stub(vscode.workspace, 'openTextDocument').resolves(mockDocument) - // const filePath = 'dummy/file.py' - // targetCommand = testCommand(generateFix, mockClient) - // await targetCommand.execute(issueItem, filePath, 'tree') - // assert.ok(updateSecurityIssueWebviewMock.calledWith({ isGenerateFixLoading: true })) - // assert.ok( - // mockClient.generateCodeFix.calledWith({ - // sourceCode: 'dummy input', - // ruleId: codeScanIssue.ruleId, - // startLine: codeScanIssue.startLine, - // endLine: codeScanIssue.endLine, - // findingDescription: codeScanIssue.description.text, - // }) - // ) - - // const expectedUpdatedIssue = { - // ...codeScanIssue, - // suggestedFixes: [{ code: 'diff', description: 'description', references: [] }], - // } - // assert.ok( - // updateSecurityIssueWebviewMock.calledWith({ issue: expectedUpdatedIssue, isGenerateFixLoading: false }) - // ) - // assert.ok(updateIssueMock.calledWith(expectedUpdatedIssue, filePath)) - // assert.ok(refreshTreeViewMock.calledOnce) - - // assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { - // detectorId: codeScanIssue.detectorId, - // findingId: codeScanIssue.findingId, - // ruleId: codeScanIssue.ruleId, - // component: 'tree', - // result: 'Succeeded', - // }) - // }) - - // it('should call generateFix with refresh=true to indicate fix regenerated', async function () { - // sinon.stub(securityIssueWebview, 'updateSecurityIssueWebview').value(updateSecurityIssueWebviewMock) - // sinon.stub(SecurityIssueProvider.instance, 'updateIssue').value(updateIssueMock) - // sinon.stub(SecurityIssueTreeViewProvider.instance, 'refresh').value(refreshTreeViewMock) - // sinon.stub(vscode.workspace, 'openTextDocument').resolves(mockDocument) - // targetCommand = testCommand(generateFix, mockClient) - // await targetCommand.execute(codeScanIssue, filePath, 'webview', true) - // assert.ok(updateSecurityIssueWebviewMock.calledWith({ isGenerateFixLoading: true })) - // assert.ok( - // mockClient.generateCodeFix.calledWith({ - // sourceCode: 'dummy input', - // ruleId: codeScanIssue.ruleId, - // startLine: codeScanIssue.startLine, - // endLine: codeScanIssue.endLine, - // findingDescription: codeScanIssue.description.text, - // }) - // ) - - // const expectedUpdatedIssue = { - // ...codeScanIssue, - // suggestedFixes: [{ code: 'diff', description: 'description', references: [] }], - // } - // assert.ok( - // updateSecurityIssueWebviewMock.calledWith({ issue: expectedUpdatedIssue, isGenerateFixLoading: false }) - // ) - // assert.ok(updateIssueMock.calledWith(expectedUpdatedIssue, filePath)) - // assert.ok(refreshTreeViewMock.calledOnce) - - // assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { - // detectorId: codeScanIssue.detectorId, - // findingId: codeScanIssue.findingId, - // ruleId: codeScanIssue.ruleId, - // component: 'webview', - // variant: 'refresh', - // result: 'Succeeded', - // }) - // }) - // }) + describe('generateFix', function () { + let sandbox: sinon.SinonSandbox + let mockClient: Stub + let startCodeFixGenerationStub: sinon.SinonStub + let filePath: string + let codeScanIssue: CodeScanIssue + let issueItem: IssueItem + let updateSecurityIssueWebviewMock: sinon.SinonStub + let updateIssueMock: sinon.SinonStub + let refreshTreeViewMock: sinon.SinonStub + let mockExtContext: ExtContext + + beforeEach(async function () { + sandbox = sinon.createSandbox() + mockClient = stub(DefaultCodeWhispererClient) + startCodeFixGenerationStub = sinon.stub(startCodeFixGeneration, 'startCodeFixGeneration') + filePath = 'dummy/file.py' + codeScanIssue = createCodeScanIssue({ + findingId: randomUUID(), + ruleId: 'dummy-rule-id', + }) + issueItem = new IssueItem(filePath, codeScanIssue) + updateSecurityIssueWebviewMock = sinon.stub(securityIssueWebview, 'updateSecurityIssueWebview') + updateIssueMock = sinon.stub(SecurityIssueProvider.instance, 'updateIssue') + refreshTreeViewMock = sinon.stub(SecurityIssueTreeViewProvider.instance, 'refresh') + mockExtContext = await FakeExtensionContext.getFakeExtContext() + }) + + afterEach(function () { + sandbox.restore() + }) + + it('should call generateFix command successfully', async function () { + startCodeFixGenerationStub.resolves({ + suggestedFix: { + codeDiff: 'codeDiff', + description: 'description', + references: [], + }, + jobId: 'jobId', + }) + + targetCommand = testCommand(generateFix, mockClient, mockExtContext) + await targetCommand.execute(codeScanIssue, filePath, 'webview') + + assert.ok(updateSecurityIssueWebviewMock.calledWith(sinon.match({ isGenerateFixLoading: true }))) + assert.ok( + startCodeFixGenerationStub.calledWith(mockClient, codeScanIssue, filePath, codeScanIssue.findingId) + ) + + const expectedUpdatedIssue = { + ...codeScanIssue, + fixJobId: 'jobId', + suggestedFixes: [{ code: 'codeDiff', description: 'description', references: [] }], + } + assert.ok( + updateSecurityIssueWebviewMock.calledWith( + sinon.match({ + issue: expectedUpdatedIssue, + isGenerateFixLoading: false, + filePath: filePath, + shouldRefreshView: true, + }) + ) + ) + assert.ok(updateIssueMock.calledWith(expectedUpdatedIssue, filePath)) + assert.ok(refreshTreeViewMock.calledOnce) + + assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { + detectorId: codeScanIssue.detectorId, + findingId: codeScanIssue.findingId, + ruleId: codeScanIssue.ruleId, + component: 'webview', + result: 'Succeeded', + }) + }) + + it('should call generateFix from tree view item', async function () { + startCodeFixGenerationStub.resolves({ + suggestedFix: { + codeDiff: 'codeDiff', + description: 'description', + references: [], + }, + jobId: 'jobId', + }) + + targetCommand = testCommand(generateFix, mockClient, mockExtContext) + await targetCommand.execute(issueItem, filePath, 'tree') + + assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { + detectorId: codeScanIssue.detectorId, + findingId: codeScanIssue.findingId, + ruleId: codeScanIssue.ruleId, + component: 'tree', + result: 'Succeeded', + }) + }) + + it('should call generateFix with refresh=true to indicate fix regenerated', async function () { + startCodeFixGenerationStub.resolves({ + suggestedFix: { + codeDiff: 'codeDiff', + description: 'description', + references: [], + }, + jobId: 'jobId', + }) + + targetCommand = testCommand(generateFix, mockClient, mockExtContext) + await targetCommand.execute(codeScanIssue, filePath, 'webview', true) + + assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { + detectorId: codeScanIssue.detectorId, + findingId: codeScanIssue.findingId, + ruleId: codeScanIssue.ruleId, + component: 'webview', + result: 'Succeeded', + variant: 'refresh', + }) + }) + + it('should handle generateFix error', async function () { + startCodeFixGenerationStub.throws(new Error('Unexpected error')) + + targetCommand = testCommand(generateFix, mockClient, mockExtContext) + await targetCommand.execute(codeScanIssue, filePath, 'webview') + + assert.ok(updateSecurityIssueWebviewMock.calledWith(sinon.match({ isGenerateFixLoading: true }))) + assert.ok( + updateSecurityIssueWebviewMock.calledWith( + sinon.match({ + issue: codeScanIssue, + isGenerateFixLoading: false, + generateFixError: 'Unexpected error', + shouldRefreshView: false, + }) + ) + ) + assert.ok(updateIssueMock.calledWith(codeScanIssue, filePath)) + assert.ok(refreshTreeViewMock.calledOnce) + + assertTelemetry('codewhisperer_codeScanIssueGenerateFix', { + detectorId: codeScanIssue.detectorId, + findingId: codeScanIssue.findingId, + ruleId: codeScanIssue.ruleId, + component: 'webview', + result: 'Failed', + reason: 'Error', + reasonDesc: 'Unexpected error', + }) + }) + }) describe('rejectFix', function () { let mockExtensionContext: vscode.ExtensionContext From c74177b87114ad735aa7a415277feac91cd123bc Mon Sep 17 00:00:00 2001 From: Tai Lai Date: Tue, 7 Jan 2025 12:00:30 -0800 Subject: [PATCH 2/5] add user friendly message --- packages/core/package.nls.json | 1 + .../core/src/codewhisperer/models/errors.ts | 10 ++++++++++ .../src/codewhisperer/service/codeFixHandler.ts | 17 +++++++++++++---- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/core/package.nls.json b/packages/core/package.nls.json index cab4f52dcee..f2a64d47838 100644 --- a/packages/core/package.nls.json +++ b/packages/core/package.nls.json @@ -309,6 +309,7 @@ "AWS.amazonq.scans.projectScanInProgress": "Workspace review is in progress...", "AWS.amazonq.scans.fileScanInProgress": "File review is in progress...", "AWS.amazonq.scans.noGitRepo": "Your workspace is not in a git repository. I'll review your project files for security issues, and your in-flight changes for code quality issues.", + "AWS.amazonq.codefix.error.monthlyLimitReached": "Maximum code fix count reached for this month.", "AWS.amazonq.featureDev.error.conversationIdNotFoundError": "Conversation id must exist before starting code generation", "AWS.amazonq.featureDev.error.contentLengthError": "The folder you selected is too large for me to use as context. Please choose a smaller folder to work on. For more information on quotas, see the Amazon Q Developer documentation.", "AWS.amazonq.featureDev.error.illegalStateTransition": "Illegal transition between states, restart the conversation", diff --git a/packages/core/src/codewhisperer/models/errors.ts b/packages/core/src/codewhisperer/models/errors.ts index 3fe22f22af0..9466fede54d 100644 --- a/packages/core/src/codewhisperer/models/errors.ts +++ b/packages/core/src/codewhisperer/models/errors.ts @@ -172,3 +172,13 @@ export class CodeFixJobStoppedError extends CodeFixError { super('Code fix generation stopped by user.', 'CodeFixCancelled', defaultCodeFixErrorMessage) } } + +export class MonthlyCodeFixLimitError extends CodeFixError { + constructor() { + super( + i18n('AWS.amazonq.codefix.error.monthlyLimitReached'), + MonthlyCodeFixLimitError.name, + defaultCodeFixErrorMessage + ) + } +} diff --git a/packages/core/src/codewhisperer/service/codeFixHandler.ts b/packages/core/src/codewhisperer/service/codeFixHandler.ts index e6aa5738db4..5c4f86dbf9b 100644 --- a/packages/core/src/codewhisperer/service/codeFixHandler.ts +++ b/packages/core/src/codewhisperer/service/codeFixHandler.ts @@ -6,9 +6,15 @@ import { CodeWhispererUserClient } from '../indexNode' import * as CodeWhispererConstants from '../models/constants' import { codeFixState } from '../models/model' -import { getLogger, sleep } from '../../shared' +import { getLogger, isAwsError, sleep } from '../../shared' import { ArtifactMap, CreateUploadUrlRequest, DefaultCodeWhispererClient } from '../client/codewhisperer' -import { CodeFixJobStoppedError, CodeFixJobTimedOutError } from '../models/errors' +import { + CodeFixJobStoppedError, + CodeFixJobTimedOutError, + CreateCodeFixError, + CreateUploadUrlError, + MonthlyCodeFixLimitError, +} from '../models/errors' import { uploadArtifactToS3 } from './securityScanHandler' export async function getPresignedUrlAndUpload( @@ -24,7 +30,7 @@ export async function getPresignedUrlAndUpload( getLogger().verbose(`Prepare for uploading src context...`) const srcResp = await client.createUploadUrl(srcReq).catch((err) => { getLogger().error('Failed getting presigned url for uploading src context. %O', err) - throw err + throw new CreateUploadUrlError(err.message) }) getLogger().verbose(`CreateUploadUrlRequest requestId: ${srcResp.$response.requestId}`) getLogger().verbose(`Complete Getting presigned Url for uploading src context.`) @@ -56,7 +62,10 @@ export async function createCodeFixJob( const resp = await client.startCodeFixJob(req).catch((err) => { getLogger().error('Failed creating code fix job. %O', err) - throw err + if (isAwsError(err) && err.code === 'ThrottlingException' && err.message.includes('reached for this month')) { + throw new MonthlyCodeFixLimitError() + } + throw new CreateCodeFixError() }) getLogger().info(`AmazonQ generate fix Request id: ${resp.$response.requestId}`) return resp From 687ae39d86e57eace0db1442bb08571711019412 Mon Sep 17 00:00:00 2001 From: Tai Lai Date: Mon, 13 Jan 2025 11:09:45 -0800 Subject: [PATCH 3/5] address comments --- packages/core/src/codewhisperer/commands/basicCommands.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/src/codewhisperer/commands/basicCommands.ts b/packages/core/src/codewhisperer/commands/basicCommands.ts index 73c81033b28..d519049f7d3 100644 --- a/packages/core/src/codewhisperer/commands/basicCommands.ts +++ b/packages/core/src/codewhisperer/commands/basicCommands.ts @@ -50,7 +50,7 @@ import { once } from '../../shared/utilities/functionUtils' import { focusAmazonQPanel } from '../../codewhispererChat/commands/registerCommands' import { removeDiagnostic } from '../service/diagnosticsProvider' import { SsoAccessTokenProvider } from '../../auth/sso/ssoAccessTokenProvider' -import { ToolkitError, getErrorMsg, getTelemetryReason, getTelemetryReasonDesc } from '../../shared/errors' +import { ToolkitError, getErrorMsg, getTelemetryReason, getTelemetryReasonDesc, isAwsError } from '../../shared/errors' import { isRemoteWorkspace } from '../../shared/vscode/env' import { isBuilderIdConnection } from '../../auth/connection' import globals from '../../shared/extensionGlobals' @@ -67,7 +67,6 @@ import { startCodeFixGeneration } from './startCodeFixGeneration' import { DefaultAmazonQAppInitContext } from '../../amazonq/apps/initContext' import path from 'path' import { parsePatch } from 'diff' -import type { AWSError } from 'aws-sdk' const MessageTimeOut = 5_000 @@ -736,10 +735,11 @@ export const generateFix = Commands.declare( SecurityIssueProvider.instance.updateIssue(updatedIssue, targetFilePath) SecurityIssueTreeViewProvider.instance.refresh() } catch (err) { + const error = isAwsError(err) ? err : new TypeError('Unexpected error') await updateSecurityIssueWebview({ issue: targetIssue, isGenerateFixLoading: false, - generateFixError: getErrorMsg(err as AWSError, true), + generateFixError: getErrorMsg(error, true), filePath: targetFilePath, context: context.extensionContext, shouldRefreshView: false, From 1d8491c1f95ce4ba0c3112081d53180d6070ddcc Mon Sep 17 00:00:00 2001 From: Tai Lai Date: Mon, 13 Jan 2025 16:35:17 -0800 Subject: [PATCH 4/5] fix test --- packages/core/src/codewhisperer/commands/basicCommands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/codewhisperer/commands/basicCommands.ts b/packages/core/src/codewhisperer/commands/basicCommands.ts index d519049f7d3..d8f05fed7bf 100644 --- a/packages/core/src/codewhisperer/commands/basicCommands.ts +++ b/packages/core/src/codewhisperer/commands/basicCommands.ts @@ -735,7 +735,7 @@ export const generateFix = Commands.declare( SecurityIssueProvider.instance.updateIssue(updatedIssue, targetFilePath) SecurityIssueTreeViewProvider.instance.refresh() } catch (err) { - const error = isAwsError(err) ? err : new TypeError('Unexpected error') + const error = err instanceof Error ? err : new TypeError('Unexpected error') await updateSecurityIssueWebview({ issue: targetIssue, isGenerateFixLoading: false, From 4d0af537c5c801710ce7fd3066980749123fb130 Mon Sep 17 00:00:00 2001 From: Tai Lai Date: Mon, 13 Jan 2025 16:39:45 -0800 Subject: [PATCH 5/5] lint --- packages/core/src/codewhisperer/commands/basicCommands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/codewhisperer/commands/basicCommands.ts b/packages/core/src/codewhisperer/commands/basicCommands.ts index d8f05fed7bf..16c1870ef6a 100644 --- a/packages/core/src/codewhisperer/commands/basicCommands.ts +++ b/packages/core/src/codewhisperer/commands/basicCommands.ts @@ -50,7 +50,7 @@ import { once } from '../../shared/utilities/functionUtils' import { focusAmazonQPanel } from '../../codewhispererChat/commands/registerCommands' import { removeDiagnostic } from '../service/diagnosticsProvider' import { SsoAccessTokenProvider } from '../../auth/sso/ssoAccessTokenProvider' -import { ToolkitError, getErrorMsg, getTelemetryReason, getTelemetryReasonDesc, isAwsError } from '../../shared/errors' +import { ToolkitError, getErrorMsg, getTelemetryReason, getTelemetryReasonDesc } from '../../shared/errors' import { isRemoteWorkspace } from '../../shared/vscode/env' import { isBuilderIdConnection } from '../../auth/connection' import globals from '../../shared/extensionGlobals'