Skip to content
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

Add support to generate auto fixes using LLM (AI) #1177

Merged
merged 37 commits into from
Aug 12, 2024

Conversation

tran-the-lam
Copy link
Contributor

@tran-the-lam tran-the-lam commented Jul 31, 2024

This feature adds support to generate auto fixes for Go scanning findings using LLM (AI). In a first instance, it relies on Gemini API to get a suggestion for a solution. This can be later extended, to integrate also other AI providers.

@tran-the-lam
Copy link
Contributor Author

Example Output: Screenshot 2024-07-31 at 22 18 26

@tran-the-lam
Copy link
Contributor Author

@ccojocar Please review this pr

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Thanks @tran-the-lam for this contribution. Please could you address the review comments.

Is is also possible to add a unit tests? Is there an option to use a fake client for gemeni api inside of the unit test?

cmd/gosec/main.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
cmd/gosec/main.go Outdated Show resolved Hide resolved
issue/issue.go Outdated Show resolved Hide resolved
proposesolution/gemini.go Outdated Show resolved Hide resolved
proposesolution/gemini.go Outdated Show resolved Hide resolved
proposesolution/gemini.go Outdated Show resolved Hide resolved
proposesolution/gemini.go Outdated Show resolved Hide resolved
report/text/template.txt Outdated Show resolved Hide resolved
@ccojocar
Copy link
Member

ccojocar commented Aug 1, 2024

Also after you address the review suggestions, please fix the lint warnings. Thanks

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the review comments. I still left a few more comments which should be addressed before we merge this.

The unit test should not call the real gemeni api, this needs to be mocked. I would convert the GenerateSolution function into a method and use a struct to store the gemeni client. In these you can inject a mocked client instead of the real client.

Also please add a section in the README how to use this feature. Have you done any performance tests? Please mention in the doc if you have any performance data, e.g for instance execution time per number of issues (autofixes to generate).

issue/issue.go Outdated Show resolved Hide resolved
proposesolution/ai.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Outdated Show resolved Hide resolved
proposesolution/ai.go Outdated Show resolved Hide resolved
proposesolution/ai.go Outdated Show resolved Hide resolved
proposesolution/ai.go Outdated Show resolved Hide resolved
proposesolution/ai.go Outdated Show resolved Hide resolved
proposesolution/ai_test.go Outdated Show resolved Hide resolved
Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the review comments. There are two major things to address in order for me to accept your contribution:

  1. Fix the unit test to use a mock server, see the instructions in my comments.
  2. Document how to use this feature in README in a dedicated section

Without these two issue addressed, I won't be able to accept this contribution. Thank you for understanding.

autofix/ai.go Outdated Show resolved Hide resolved
autofix/ai_test.go Outdated Show resolved Hide resolved
@ccojocar
Copy link
Member

ccojocar commented Aug 3, 2024

Also make sure that all checks are passing. The CI is still failing.

@tran-the-lam tran-the-lam requested a review from ccojocar August 3, 2024 13:26
Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the review comments. It looks better now. Just a few small things to address that we can merge it.

I think still the unit tests are failing.

README.md Outdated Show resolved Hide resolved
autofix/ai.go Outdated Show resolved Hide resolved
autofix/ai.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

A few suggestions

I'm also reporting issues that were present in code already there. But I'm noticing them as you worked around them.

Feel free to ignore

autofix/ai.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Show resolved Hide resolved
issue/issue.go Show resolved Hide resolved
issue/issue.go Show resolved Hide resolved
issue/issue.go Show resolved Hide resolved
@ccojocar
Copy link
Member

ccojocar commented Aug 6, 2024

There are some issue to fix in the build/tests https://github.com/securego/gosec/actions/runs/10259787997/job/28392108382?pr=1177. Otherwise look good! Thanks

@ccojocar ccojocar changed the title Feat/gpt suggest fix error Add support to generate auto fixes using LLM (AI) Aug 6, 2024
@tran-the-lam tran-the-lam requested a review from ccojocar August 6, 2024 09:58
autofix/ai.go Outdated Show resolved Hide resolved
autofix/ai.go Outdated Show resolved Hide resolved
autofix/ai.go Outdated Show resolved Hide resolved
cmd/gosec/main.go Show resolved Hide resolved
autofix/ai.go Show resolved Hide resolved
autofix/ai.go Show resolved Hide resolved
@ccojocar
Copy link
Member

ccojocar commented Aug 7, 2024

Unit tests are failing and it seems related to this change, please have a look at https://github.com/securego/gosec/actions/runs/10264724512/job/28456313532?pr=1177#step:5:98

@tran-the-lam tran-the-lam requested a review from ccojocar August 7, 2024 14:14
@tran-the-lam
Copy link
Contributor Author

tran-the-lam commented Aug 9, 2024

@ccojocar Please review and merge.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 47.76119% with 35 lines in your changes missing coverage. Please review.

Project coverage is 69.13%. Comparing base (f33fd4b) to head (f97e82f).
Report is 48 commits behind head on master.

Files with missing lines Patch % Lines
autofix/ai.go 41.50% 30 Missing and 1 partial ⚠️
cmd/gosec/main.go 0.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
- Coverage   69.52%   69.13%   -0.39%     
==========================================
  Files          71       72       +1     
  Lines        3865     3930      +65     
==========================================
+ Hits         2687     2717      +30     
- Misses       1061     1095      +34     
- Partials      117      118       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tran-the-lam and others added 21 commits August 12, 2024 08:59
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Change-Id: Ic41d859830b3753afe6ba6db9cc7868678b8c799
@ccojocar ccojocar force-pushed the feat/gpt_suggest_fix_error branch from f3e8bee to 4722af5 Compare August 12, 2024 09:03
Change-Id: I20a7e9a2e95ce9af868c387f5ad3b69f1209265f
Change-Id: Ib122328df33f7a883de47c43cb3cad95af21e327
Signed-off-by: Cosmin Cojocar <ccojocar@google.com>
Change-Id: I3e5ad9bc42a70a9ce2d46304474a0d879b48dffc
Signed-off-by: Cosmin Cojocar <ccojocar@google.com>
Change-Id: I8e006073fc3c9b7587e255e91017d15b9e2c9189
Signed-off-by: Cosmin Cojocar <ccojocar@google.com>
Change-Id: Ie5cbd52bf7efd372c07fa6efac74352e3c7960e8
Signed-off-by: Cosmin Cojocar <ccojocar@google.com>
@ccojocar ccojocar merged commit 56f943b into securego:master Aug 12, 2024
6 checks passed
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.

4 participants