Skip to content

fix(amazonq): auto-review removes existing issues #6535

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 19, 2025
Merged

fix(amazonq): auto-review removes existing issues #6535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/amazonq/.changes/next-release/Bug Fix-c8a60497-e42c-4141-bcfe-2c44188bc98b.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "/review: Auto-review should not remove issues from manual reviews"
}
1 change: 0 additions & 1 deletion packages/core/src/codewhisperer/activation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,6 @@ function toggleIssuesVisibility(visibleCondition: (issue: CodeScanIssue, filePat
...group,
issues: group.issues.map((issue) => ({ ...issue, visible: visibleCondition(issue, group.filePath) })),
}))
securityScanRender.securityDiagnosticCollection?.clear()
Copy link
Contributor

@ashishrp-aws ashishrp-aws Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this being removed?

Copy link
Contributor Author

@ctlai95 ctlai95 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug. The toggleIssuesVisibility was getting invoked on every document change, so it always removes all of the diagnostics when really we just want to check if an issue should be visible or not.

for (const issue of updatedIssues) {
updateSecurityDiagnosticCollection(issue)
}
Expand Down
45 changes: 32 additions & 13 deletions packages/core/src/codewhisperer/service/diagnosticsProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import * as vscode from 'vscode'
import { CodeScanIssue, AggregatedCodeScanIssue, CodeScansState } from '../models/model'
import { CodeScanIssue, AggregatedCodeScanIssue } from '../models/model'
import { CodeAnalysisScope, codewhispererDiagnosticSourceLabel } from '../models/constants'
import { SecurityIssueTreeViewProvider } from './securityIssueTreeViewProvider'
import { SecurityIssueProvider } from './securityIssueProvider'
Expand All @@ -30,24 +30,30 @@ export function initSecurityScanRender(
scope: CodeAnalysisScope
) {
securityScanRender.initialized = false
if ((scope === CodeAnalysisScope.FILE_AUTO || scope === CodeAnalysisScope.FILE_ON_DEMAND) && editor) {
if (scope === CodeAnalysisScope.FILE_ON_DEMAND && editor) {
securityScanRender.securityDiagnosticCollection?.delete(editor.document.uri)
} else if (scope === CodeAnalysisScope.PROJECT) {
securityScanRender.securityDiagnosticCollection?.clear()
}
for (const securityRecommendation of securityRecommendationList) {
updateSecurityDiagnosticCollection(securityRecommendation)
updateSecurityIssuesForProviders(securityRecommendation)
updateSecurityIssuesForProviders(securityRecommendation, scope === CodeAnalysisScope.FILE_AUTO)
Copy link
Contributor

@ashishrp-aws ashishrp-aws Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we are skipping rendering of issues all together for auto scans and updating the list.

Copy link
Contributor Author

@ctlai95 ctlai95 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, here auto-scan results should not remove anything existing. Only append new ones or skip existing ones

}
securityScanRender.initialized = true
}

function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue) {
const updatedSecurityRecommendationList = [
...SecurityIssueProvider.instance.issues.filter((group) => group.filePath !== securityRecommendation.filePath),
securityRecommendation,
]
SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList
function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue, isAutoScope?: boolean) {
if (isAutoScope) {
SecurityIssueProvider.instance.mergeIssues(securityRecommendation)
} else {
const updatedSecurityRecommendationList = [
...SecurityIssueProvider.instance.issues.filter(
(group) => group.filePath !== securityRecommendation.filePath
),
securityRecommendation,
]
SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList
}
SecurityIssueTreeViewProvider.instance.refresh()
}

Expand All @@ -58,8 +64,22 @@ export function updateSecurityDiagnosticCollection(securityRecommendation: Aggre
const securityDiagnostics: vscode.Diagnostic[] = vscode.languages
.getDiagnostics(uri)
.filter((diagnostic) => diagnostic.source === codewhispererDiagnosticSourceLabel)
for (const securityIssue of securityRecommendation.issues.filter((securityIssue) => securityIssue.visible)) {
securityDiagnostics.push(createSecurityDiagnostic(securityIssue))
for (const securityIssue of securityRecommendation.issues) {
const existingDiagnosticIndex = securityDiagnostics.findIndex(
(diagnostic) =>
(diagnostic.message === securityIssue.title &&
diagnostic.range.start.line === securityIssue.startLine &&
diagnostic.range.end.line === securityIssue.endLine) ||
(diagnostic.message === 'Re-scan to validate the fix: ' + securityIssue.title &&
Copy link
Contributor

@ashishrp-aws ashishrp-aws Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this a bit manual check?

Copy link
Contributor Author

@ctlai95 ctlai95 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can clean this up later. We need to check this because there are cases where the title has been modified by document changes, but ultimately it is still the same finding.

diagnostic.range.start.line === securityIssue.startLine &&
diagnostic.range.end.line === securityIssue.startLine)
)
if (existingDiagnosticIndex !== -1) {
securityDiagnostics.splice(existingDiagnosticIndex, 1)
}
if (securityIssue.visible) {
securityDiagnostics.push(createSecurityDiagnostic(securityIssue))
}
}
securityDiagnosticCollection.set(uri, securityDiagnostics)
}
Expand Down Expand Up @@ -112,8 +132,7 @@ export function disposeSecurityDiagnostic(event: vscode.TextDocumentChangeEvent)
if (
issue.severity === vscode.DiagnosticSeverity.Warning &&
intersection &&
(/\S/.test(changedText) || changedText === '') &&
!CodeScansState.instance.isScansEnabled()
(/\S/.test(changedText) || changedText === '')
) {
issue.severity = vscode.DiagnosticSeverity.Information
issue.message = 'Re-scan to validate the fix: ' + issue.message
Expand Down
36 changes: 30 additions & 6 deletions packages/core/src/codewhisperer/service/securityIssueProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import * as vscode from 'vscode'
import { AggregatedCodeScanIssue, CodeScanIssue, CodeScansState, SuggestedFix } from '../models/model'
import { AggregatedCodeScanIssue, CodeScanIssue, SuggestedFix } from '../models/model'
export class SecurityIssueProvider {
static #instance: SecurityIssueProvider
public static get instance() {
Expand Down Expand Up @@ -53,11 +53,7 @@ export class SecurityIssueProvider {
event.document.lineAt(issue.endLine - 1)?.range.end.character ?? 0
)
const intersection = changedRange.intersection(range)
return !(
intersection &&
(/\S/.test(changedText) || changedText === '') &&
!CodeScansState.instance.isScansEnabled()
)
return !(intersection && (/\S/.test(changedText) || changedText === ''))
})
.map((issue) => {
if (issue.startLine < changedRange.end.line) {
Expand Down Expand Up @@ -132,4 +128,32 @@ export class SecurityIssueProvider {
}
})
}

public mergeIssues(newIssues: AggregatedCodeScanIssue) {
const existingGroup = this._issues.find((group) => group.filePath === newIssues.filePath)
if (!existingGroup) {
this._issues.push(newIssues)
return
}

this._issues = this._issues.map((group) =>
group.filePath !== newIssues.filePath
? group
: {
...group,
issues: [
...group.issues,
...newIssues.issues.filter((issue) => !this.isExistingIssue(issue, newIssues.filePath)),
],
}
)
}

private isExistingIssue(issue: CodeScanIssue, filePath: string) {
return this._issues
.find((group) => group.filePath === filePath)
?.issues.find(
(i) => i.title === issue.title && i.startLine === issue.startLine && i.endLine === issue.endLine
)
}
}
11 changes: 7 additions & 4 deletions packages/core/src/shared/utilities/commentUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,16 @@ export function insertCommentAboveLine(document: vscode.TextDocument, line: numb
}

const edit = new vscode.WorkspaceEdit()
const position = new vscode.Position(line, 0)
const previousLine = Math.max(0, line - 1)
const previousLineLength = previousLine > 0 ? document.lineAt(previousLine).text.length : 0
const position = new vscode.Position(previousLine, previousLineLength)
const indent = ' '.repeat(Math.max(0, document.lineAt(line).firstNonWhitespaceCharacterIndex))
const commentPrefix = line === 0 ? '' : '\n'
const commentText = lineComment
? `${indent}${lineComment} ${comment}\n`
? `${commentPrefix}${indent}${lineComment} ${comment}`
: blockComment?.[0] && blockComment[1]
? `${indent}${blockComment[0]} ${comment} ${blockComment[1]}\n`
: `${indent}${defaultCommentConfig.lineComment} ${comment}\n`
? `${commentPrefix}${indent}${blockComment[0]} ${comment} ${blockComment[1]}`
: `${commentPrefix}${indent}${defaultCommentConfig.lineComment} ${comment}`
edit.insert(document.uri, position, commentText)
void vscode.workspace.applyEdit(edit)
}
6 changes: 3 additions & 3 deletions packages/core/src/test/shared/utilities/commentUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe('CommentUtils', function () {

const document = createMockDocument('foo = 1\nbar = 2', 'foo.py', 'python')
insertCommentAboveLine(document, 1, 'some-comment')
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), '# some-comment\n'))
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n# some-comment'))
})

it('should indent the comment by the same amount as the current line', function () {
Expand All @@ -86,7 +86,7 @@ describe('CommentUtils', function () {

const document = createMockDocument(' foo = 1\n bar = 2', 'foo.py', 'python')
insertCommentAboveLine(document, 1, 'some-comment')
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), ' # some-comment\n'))
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n # some-comment'))
})

it('should fallback to block comment if line comment is undefined', function () {
Expand All @@ -95,7 +95,7 @@ describe('CommentUtils', function () {

const document = createMockDocument('<aaa>\n <bbb></bbb>\n</aaa>', 'foo.xml', 'xml')
insertCommentAboveLine(document, 1, 'some-comment')
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), ' <!-- some-comment -->\n'))
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n <!-- some-comment -->'))
})
})
})
Loading