Skip to content

Add "View Autofixes" feature for variant analysis results #4065

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.

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 49 commits into from
Jul 8, 2025

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Jun 29, 2025

Description

Adds a "View Autofixes" feature for variant analysis results. This is a canary-only feature.
Generates autofixes for selected variant analysis results and opens the resulting autofix outputs in a markdown file.
Activated via the "View Autofixes" button in the variant analysis webview or via right-clicking the variant analysis query history item.

AutofixInMrvaScreenshot

At a high-level, the implementation:
  • Checks for a local checkout of autofix
  • Sets up to run autofix by:
    • Overriding the relevant query help
    • Getting the relevant SARIF(s)
    • Downloading source root(s)
  • Runs autofix locally
  • Outputs autofix results in a markdown file

Commit-by-commit review recommended. Let me know if you want me to split anything onto a separate PR.

Consideration

  • See in-line comments
  • Change from my original implementation: Instead of downloading databases and then extracting the source root from the database, I directly download the source root since it seemed a bit faster. Let me know if you think the database approach is better.
  • I will make a follow-up PR to convert this to support Go autofix instead of cocofix, and I'll update the queryHelpOverrideDirectory to the new go/pkg/autofix/prompt/qhelps directory in that PR.

Testing

  • I've manually tested this feature but have not added unit/view/integration tests since it seems another canary feature did not add tests. Let me know if I should add tests after all.
  • Note: There is a known issue with the E2E CI test failing. It is not caused by changes in this PR.

@jcogs33 jcogs33 force-pushed the jcogs33/view-autofixes branch 3 times, most recently from 8095989 to c5f0e5e Compare June 30, 2025 21:29
@jcogs33 jcogs33 force-pushed the jcogs33/view-autofixes branch from c5f0e5e to 3ff5d64 Compare July 2, 2025 00:44
@jcogs33
Copy link
Contributor Author

jcogs33 commented Jul 4, 2025

There's one more comment from @koesie10 that should be addressed

@aeisenberg Which comment are you referring to? There were four things I still was planning to address: three comments related to editing parts of downloadPublicCommitSource, and one comment about getting the autofix CLI logs into the extension logs.

@aeisenberg
Copy link
Contributor

Oh...there might be more things that I missed. I'm out tomorrow. I think it's also a holiday in the US, so I can take another look on Monday.

@jcogs33 jcogs33 requested review from aeisenberg and koesie10 July 6, 2025 23:02
@jcogs33
Copy link
Contributor Author

jcogs33 commented Jul 6, 2025

@aeisenberg @koesie10 I've addressed all review comments. Let me know if you want any other changes, especially regarding the environment variables.

Copy link
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. I have some optional comments, but I don't think any of those are necessary.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Looks generally good. I see one area where I think you can simplify things.

@jcogs33
Copy link
Contributor Author

jcogs33 commented Jul 7, 2025

@koesie10 @aeisenberg I've responded to all new review comments. Thanks for the thorough reviews! 🙇

@jcogs33 jcogs33 requested review from aeisenberg and koesie10 July 7, 2025 23:08
Copy link
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Great job!

I haven't tried this locally, but the code looks good. It's likely that the failing E2E job is transient. I'll kick it off again and hopefully it will work.

@jcogs33
Copy link
Contributor Author

jcogs33 commented Jul 8, 2025

@aeisenberg There's a known issue with the E2E tests failing since end of March. The issue tracking it is in the private code-scanning repo. So I think this PR can be merged despite that test failure?

@aeisenberg
Copy link
Contributor

I'm fine with that.

@aeisenberg aeisenberg merged commit 5328aa2 into github:main Jul 8, 2025
18 of 20 checks passed
@jcogs33 jcogs33 deleted the jcogs33/view-autofixes branch July 8, 2025 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants