Skip to content

Commit d40d74a

Browse files
BlakeLazarineblakelaz-amazonsinghAws
authored
feat(amazonq): enable displayFindings tool (aws#7799)
## Problem Need to enable and support findings coming in from the displayFindings tool Also - Agentic scans are supposed to use the CodeAnalysisScope of AGENTIC, which needed to be added ## Solution Set feature flag for displayFindings to be true Listen for displayFindings messageId from FLARE similarly to how is being done for CodeReview tool. Treat displayFindings findings and CodeReview findings separately, so they do not overwrite one another. <img width="435" height="1363" alt="image" src="https://github.com/user-attachments/assets/17a2e571-65c9-45d5-b89d-bccafda76fe2" /> --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Blake Lazarine <[email protected]> Co-authored-by: Nitish <[email protected]>
1 parent d3e1e4f commit d40d74a

File tree

7 files changed

+152
-14
lines changed

7 files changed

+152
-14
lines changed

‎packages/amazonq/src/lsp/chat/messages.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -732,7 +732,11 @@ async function handlePartialResult<T extends ChatResult>(
732732
// This is to filter out the message containing findings from CodeReview tool to update CodeIssues panel
733733
decryptedMessage.additionalMessages = decryptedMessage.additionalMessages?.filter(
734734
(message) =>
735-
!(message.messageId !== undefined && message.messageId.endsWith(CodeWhispererConstants.findingsSuffix))
735+
!(
736+
message.messageId !== undefined &&
737+
(message.messageId.endsWith(CodeWhispererConstants.codeReviewFindingsSuffix) ||
738+
message.messageId.endsWith(CodeWhispererConstants.displayFindingsSuffix))
739+
)
736740
)
737741

738742
if (decryptedMessage.body !== undefined) {
@@ -784,7 +788,11 @@ async function handleSecurityFindings(
784788
}
785789
for (let i = decryptedMessage.additionalMessages.length - 1; i >= 0; i--) {
786790
const message = decryptedMessage.additionalMessages[i]
787-
if (message.messageId !== undefined && message.messageId.endsWith(CodeWhispererConstants.findingsSuffix)) {
791+
if (
792+
message.messageId !== undefined &&
793+
(message.messageId.endsWith(CodeWhispererConstants.codeReviewFindingsSuffix) ||
794+
message.messageId.endsWith(CodeWhispererConstants.displayFindingsSuffix))
795+
) {
788796
if (message.body !== undefined) {
789797
try {
790798
const aggregatedCodeScanIssues: AggregatedCodeScanIssue[] = JSON.parse(message.body)
@@ -803,7 +811,12 @@ async function handleSecurityFindings(
803811
issue.visible = !isIssueTitleIgnored && !isSingleIssueIgnored
804812
}
805813
}
806-
initSecurityScanRender(aggregatedCodeScanIssues, undefined, CodeAnalysisScope.PROJECT)
814+
initSecurityScanRender(
815+
aggregatedCodeScanIssues,
816+
undefined,
817+
CodeAnalysisScope.AGENTIC,
818+
message.messageId.endsWith(CodeWhispererConstants.codeReviewFindingsSuffix)
819+
)
807820
SecurityIssueTreeViewProvider.focus()
808821
} catch (e) {
809822
languageClient.info('Failed to parse findings')

‎packages/amazonq/src/lsp/client.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,8 @@ export async function startLanguageServer(
183183
modelSelection: true,
184184
workspaceFilePath: vscode.workspace.workspaceFile?.fsPath,
185185
codeReviewInChat: codeReviewInChat,
186+
// feature flag for displaying findings found not through CodeReview in the Code Issues Panel
187+
displayFindings: true,
186188
},
187189
window: {
188190
notifications: true,

‎packages/core/src/codewhisperer/commands/startSecurityScan.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,9 @@ export async function startSecurityScan(
108108
zipUtil: ZipUtil = new ZipUtil(),
109109
scanUuid?: string
110110
) {
111+
if (scope === CodeAnalysisScope.AGENTIC) {
112+
throw new CreateCodeScanFailedError('Cannot use Agentic scope')
113+
}
111114
const profile = AuthUtil.instance.regionProfileManager.activeRegionProfile
112115
const logger = getLoggerForScope(scope)
113116
/**

‎packages/core/src/codewhisperer/models/constants.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -841,6 +841,7 @@ export enum CodeAnalysisScope {
841841
FILE_AUTO = 'FILE_AUTO',
842842
FILE_ON_DEMAND = 'FILE_ON_DEMAND',
843843
PROJECT = 'PROJECT',
844+
AGENTIC = 'AGENTIC',
844845
}
845846

846847
export enum TestGenerationJobStatus {
@@ -907,4 +908,7 @@ export const predictionTrackerDefaultConfig = {
907908
maxSupplementalContext: 15,
908909
}
909910

910-
export const findingsSuffix = '_codeReviewFindings'
911+
export const codeReviewFindingsSuffix = '_codeReviewFindings'
912+
export const displayFindingsSuffix = '_displayFindings'
913+
914+
export const displayFindingsDetectorName = 'DisplayFindings'

‎packages/core/src/codewhisperer/service/diagnosticsProvider.ts‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,13 @@ export const securityScanRender: SecurityScanRender = {
2626
export function initSecurityScanRender(
2727
securityRecommendationList: AggregatedCodeScanIssue[],
2828
editor: vscode.TextEditor | undefined,
29-
scope: CodeAnalysisScope
29+
scope: CodeAnalysisScope,
30+
fromQCA: boolean = true
3031
) {
32+
// fromQCA parameter is used to determine if the findings are coming from QCA or from displayFindings tool.
33+
// if the incoming findings are from QCA review, then keep only existing findings from displayFindings
34+
// if the incoming findings are not from QCA review, then keep only the existing QCA findings
35+
securityScanRender.securityDiagnosticCollection = createSecurityDiagnosticCollection()
3136
securityScanRender.initialized = false
3237
if (scope === CodeAnalysisScope.FILE_ON_DEMAND && editor) {
3338
securityScanRender.securityDiagnosticCollection?.delete(editor.document.uri)
@@ -36,22 +41,20 @@ export function initSecurityScanRender(
3641
}
3742
for (const securityRecommendation of securityRecommendationList) {
3843
updateSecurityDiagnosticCollection(securityRecommendation)
39-
updateSecurityIssuesForProviders(securityRecommendation, scope === CodeAnalysisScope.FILE_AUTO)
44+
updateSecurityIssuesForProviders(securityRecommendation, scope === CodeAnalysisScope.FILE_AUTO, fromQCA)
4045
}
4146
securityScanRender.initialized = true
4247
}
4348

44-
function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue, isAutoScope?: boolean) {
49+
function updateSecurityIssuesForProviders(
50+
securityRecommendation: AggregatedCodeScanIssue,
51+
isAutoScope?: boolean,
52+
fromQCA: boolean = true
53+
) {
4554
if (isAutoScope) {
4655
SecurityIssueProvider.instance.mergeIssues(securityRecommendation)
4756
} else {
48-
const updatedSecurityRecommendationList = [
49-
...SecurityIssueProvider.instance.issues.filter(
50-
(group) => group.filePath !== securityRecommendation.filePath
51-
),
52-
securityRecommendation,
53-
]
54-
SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList
57+
SecurityIssueProvider.instance.mergeIssuesDisplayFindings(securityRecommendation, fromQCA)
5558
}
5659
SecurityIssueTreeViewProvider.instance.refresh()
5760
}

‎packages/core/src/codewhisperer/service/securityIssueProvider.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import * as vscode from 'vscode'
77
import { AggregatedCodeScanIssue, CodeScanIssue, SuggestedFix } from '../models/model'
88
import { randomUUID } from '../../shared/crypto'
9+
import { displayFindingsDetectorName } from '../models/constants'
910

1011
export class SecurityIssueProvider {
1112
static #instance: SecurityIssueProvider
@@ -161,6 +162,30 @@ export class SecurityIssueProvider {
161162
)
162163
}
163164

165+
public mergeIssuesDisplayFindings(newIssues: AggregatedCodeScanIssue, fromQCA: boolean) {
166+
const existingGroup = this._issues.find((group) => group.filePath === newIssues.filePath)
167+
if (!existingGroup) {
168+
this._issues.push(newIssues)
169+
return
170+
}
171+
172+
this._issues = this._issues.map((group) =>
173+
group.filePath !== newIssues.filePath
174+
? group
175+
: {
176+
...group,
177+
issues: [
178+
...group.issues.filter(
179+
// if the incoming findings are from QCA review, then keep only existing findings from displayFindings
180+
// if the incoming findings are not from QCA review, then keep only the existing QCA findings
181+
(issue) => fromQCA === (issue.detectorName === displayFindingsDetectorName)
182+
),
183+
...newIssues.issues,
184+
],
185+
}
186+
)
187+
}
188+
164189
private isExistingIssue(issue: CodeScanIssue, filePath: string) {
165190
return this._issues
166191
.find((group) => group.filePath === filePath)

‎packages/core/src/test/codewhisperer/mergeIssuesDisplayFindings.test.ts‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/*!
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
import assert from 'assert'
7+
import { SecurityIssueProvider } from '../../codewhisperer/service/securityIssueProvider'
8+
import { createCodeScanIssue } from './testUtil'
9+
import { displayFindingsDetectorName } from '../../codewhisperer/models/constants'
10+
import { AggregatedCodeScanIssue } from '../../codewhisperer/models/model'
11+
12+
describe('mergeIssuesDisplayFindings', () => {
13+
let provider: SecurityIssueProvider
14+
const testFilePath = '/test/file.py'
15+
16+
beforeEach(() => {
17+
provider = Object.create(SecurityIssueProvider.prototype)
18+
provider.issues = []
19+
})
20+
21+
it('should add new issues when no existing group', () => {
22+
const newIssues: AggregatedCodeScanIssue = {
23+
filePath: testFilePath,
24+
issues: [createCodeScanIssue({ findingId: 'new-1' })],
25+
}
26+
27+
provider.mergeIssuesDisplayFindings(newIssues, true)
28+
29+
assert.strictEqual(provider.issues.length, 1)
30+
assert.strictEqual(provider.issues[0].filePath, testFilePath)
31+
assert.strictEqual(provider.issues[0].issues.length, 1)
32+
assert.strictEqual(provider.issues[0].issues[0].findingId, 'new-1')
33+
})
34+
35+
it('should keep displayFindings when fromQCA is true', () => {
36+
provider.issues = [
37+
{
38+
filePath: testFilePath,
39+
issues: [
40+
createCodeScanIssue({ findingId: 'qca-1', detectorName: 'QCA-detector' }),
41+
createCodeScanIssue({ findingId: 'display-1', detectorName: displayFindingsDetectorName }),
42+
],
43+
},
44+
]
45+
46+
const newIssues: AggregatedCodeScanIssue = {
47+
filePath: testFilePath,
48+
issues: [createCodeScanIssue({ findingId: 'new-qca-1', detectorName: 'QCA-detector' })],
49+
}
50+
51+
provider.mergeIssuesDisplayFindings(newIssues, true)
52+
53+
assert.strictEqual(provider.issues.length, 1)
54+
assert.strictEqual(provider.issues[0].issues.length, 2)
55+
56+
const findingIds = provider.issues[0].issues.map((issue) => issue.findingId)
57+
assert.ok(findingIds.includes('display-1'))
58+
assert.ok(findingIds.includes('new-qca-1'))
59+
assert.ok(!findingIds.includes('qca-1'))
60+
})
61+
62+
it('should keep QCA findings when fromQCA is false', () => {
63+
provider.issues = [
64+
{
65+
filePath: testFilePath,
66+
issues: [
67+
createCodeScanIssue({ findingId: 'qca-1', detectorName: 'QCA-detector' }),
68+
createCodeScanIssue({ findingId: 'display-1', detectorName: displayFindingsDetectorName }),
69+
],
70+
},
71+
]
72+
73+
const newIssues: AggregatedCodeScanIssue = {
74+
filePath: testFilePath,
75+
issues: [createCodeScanIssue({ findingId: 'new-display-1', detectorName: displayFindingsDetectorName })],
76+
}
77+
78+
provider.mergeIssuesDisplayFindings(newIssues, false)
79+
80+
assert.strictEqual(provider.issues.length, 1)
81+
assert.strictEqual(provider.issues[0].issues.length, 2)
82+
83+
const findingIds = provider.issues[0].issues.map((issue) => issue.findingId)
84+
assert.ok(findingIds.includes('qca-1'))
85+
assert.ok(findingIds.includes('new-display-1'))
86+
assert.ok(!findingIds.includes('display-1'))
87+
})
88+
})

0 commit comments

Comments
 (0)