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

ctlai95 · 2025-02-07T22:12:28Z

Problem

Auto-reviews often produce less code issues than manual reviews, but the current behavior is to remove all the issues in the file when processing the new ones. This means that issues discovered by manual reviews but not auto-reviews will silently disappear if auto-reviews is enabled.

Solution

Auto-reviews should not clear the previous issues, but instead merge in the new results to the existing group.
Fixed a related issue with the ignoreIssue command being flaky

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.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

ashishrp-aws · 2025-02-11T05:14:46Z

packages/core/src/codewhisperer/activation.ts

        ...group,
        issues: group.issues.map((issue) => ({ ...issue, visible: visibleCondition(issue, group.filePath) })),
    }))
-    securityScanRender.securityDiagnosticCollection?.clear()


why is this being removed?

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.

ashishrp-aws · 2025-02-11T05:16:26Z

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

    for (const securityRecommendation of securityRecommendationList) {
        updateSecurityDiagnosticCollection(securityRecommendation)
-        updateSecurityIssuesForProviders(securityRecommendation)
+        updateSecurityIssuesForProviders(securityRecommendation, scope === CodeAnalysisScope.FILE_AUTO)


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

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

ashishrp-aws · 2025-02-11T05:18:36Z

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

+                (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 &&


isn't this a bit manual check?

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.

## Problem Auto-reviews often produce less code issues than manual reviews, but the current behavior is to remove all the issues in the file when processing the new ones. This means that issues discovered by manual reviews but not auto-reviews will silently disappear if auto-reviews is enabled. ## Solution - Auto-reviews should not clear the previous issues, but instead merge in the new results to the existing group. - Fixed a related issue with the `ignoreIssue` command being flaky --- - 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.

* fix(appbuilder): pass in the negative version of --use-container when using build quickpick (aws#6603) ## Problem When users use appbuilder to build their lambda functions, they choose between using their samconfig file or manually selecting the build parameters/flags. The problem is that when the user selects build flags and intentionally doesn't select the ```--use-container``` flag, the command will still be run with --use-container if the samconfig file has ```use_container``` is set to true. ## Solution Whenever the user manually selects the build flags and doesn't select ```--use-container```, we add the negative version of ```--use-container```, which is ```--no-use-container```. This serves as an override if the samconfig file has ```--use-container``` set to true. --- - 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. * ci(jscpd): merge target branch in jscpd to avoid false negatives. (aws#6572) ## Problem - Follow up to aws#6564 (review). ## Solution - It appears that there is an undocumented "feature" that GHA don't run when there is a merge conflict. See [here](https://github.com/orgs/community/discussions/11265) - This means we don't have to handle the failure case where a merge fails. - Add fake config identity to mitigate this error: <img width="913" alt="image" src="https://github.com/user-attachments/assets/cd426ec7-e1ca-4d13-a3b1-3985b5593c07" /> ## Notes Going to let this sit and make sure it works as changes are merged into master. --- - 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: Justin M. Keyes <[email protected]> * ci: fix and enable post-release notification (aws#6613) - Enable for prod runs - Fix script slightly because the way codebuild runs bash and the way my local runs bash seems to not be the same. - Tested on dev release pipeline --- - 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. * refactor: notify.txt typos (aws#6616) --- - 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. * config(amazonq): update polling config for codefix (aws#6617) ## Problem Increase in codefix timeouts ## Solution Increase default timeout and lower the polling frequency --- - 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. * fix(amazonq): auto-review removes existing issues (aws#6535) ## Problem Auto-reviews often produce less code issues than manual reviews, but the current behavior is to remove all the issues in the file when processing the new ones. This means that issues discovered by manual reviews but not auto-reviews will silently disappear if auto-reviews is enabled. ## Solution - Auto-reviews should not clear the previous issues, but instead merge in the new results to the existing group. - Fixed a related issue with the `ignoreIssue` command being flaky --- - 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. * feat(amazonq): /doc: add support for infrastructure diagrams (aws#6561) Problem: - Amazon Q does not have support for infrastructure diagrams Solution: - Add support for them ![infra_diagram](https://github.com/user-attachments/assets/79693ab0-d95d-415e-8daf-04d59fed8573) --- - 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: Viktor Shesternyak <[email protected]> * refactor(cleanup): remove dead code (aws#6619) This does nothing anymore --- - 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. Signed-off-by: nkomonen-amazon <[email protected]> * telemetry(auth): Update metrics to better debug Auth dropoff (aws#6625) ## Problem We noticed that there was an auth dropoff between `session_start` with a brand new clientId, versus when the `auth_userState` metric indicated `isFirstUse` meaning the user is net new. We went from 12.7k for `session_start` to 9.8k for `auth_userState`, these should have been basically the same. ## Solution Add in certain metrics to help debug where the discrepancy is coming from: - When we determine the user is a first time user, we will also check if the clientId is newly generated. If this is not the case we know there is a discrepancy here - The relevant metric will be `function_call` with a `functionName: isFirstUse`, `result: Failed`, and a `reason: ClientIdAlreadyExisted` - When we determine the user is a first time user, if we detected that they had previous auth connections, this will indicate a likely cause for the discrepancy - The relevant metric will be `function_call` with `reason: UnexpectedConnections` - We will emit metrics when the Auth Login page loads since that also had a discrepancy and the telemetry did not exist - The relevant metric is `webview_load` and it will indicate when the Auth Login/Reauth page has actually loaded - Previously we were observing the telemetry for the command `aws.amazonq.focusChat`, but all this did was emit when called and didn't confirm the UI actually loaded. - We will also add the `isFirstUse` metric source value in to some other existing metrics --- - 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. --------- Signed-off-by: nkomonen-amazon <[email protected]> --------- Signed-off-by: nkomonen-amazon <[email protected]> Co-authored-by: Frederic Mbea <[email protected]> Co-authored-by: Hweinstock <[email protected]> Co-authored-by: Justin M. Keyes <[email protected]> Co-authored-by: Maxim Hayes <[email protected]> Co-authored-by: Tai Lai <[email protected]> Co-authored-by: Viktor Shesternyak <[email protected]> Co-authored-by: Viktor Shesternyak <[email protected]> Co-authored-by: Nikolas Komonen <[email protected]>

fix(amazonq): auto-review removes existing issues

3c3c06e

ctlai95 marked this pull request as ready for review February 7, 2025 22:30

ctlai95 requested review from a team as code owners February 7, 2025 22:30

hayemaxi approved these changes Feb 10, 2025

View reviewed changes

ashishrp-aws reviewed Feb 11, 2025

View reviewed changes

ctlai95 enabled auto-merge (squash) February 19, 2025 01:21

ashishrp-aws approved these changes Feb 19, 2025

View reviewed changes

ctlai95 merged commit b9af56c into aws:master Feb 19, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ctlai95 commented Feb 7, 2025

Uh oh!

ashishrp-aws Feb 11, 2025

Uh oh!

ctlai95 Feb 19, 2025

Uh oh!

ashishrp-aws Feb 11, 2025

Uh oh!

ctlai95 Feb 19, 2025

Uh oh!

ashishrp-aws Feb 11, 2025

Uh oh!

ctlai95 Feb 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

Uh oh!

Conversation

ctlai95 commented Feb 7, 2025

Problem

Solution

Uh oh!

ashishrp-aws Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

ctlai95 Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

ashishrp-aws Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

ctlai95 Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

ashishrp-aws Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

ctlai95 Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants