-
Notifications
You must be signed in to change notification settings - Fork 519
fix: MCP error because types are not enforced. #2422
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
- Coverage 67.80% 67.68% -0.13%
==========================================
Files 172 172
Lines 13318 13336 +18
==========================================
- Hits 9030 9026 -4
- Misses 3574 3594 +20
- Partials 714 716 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scripts/run_lints.sh
Outdated
| set -ex | ||
|
|
||
| go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.6.1 run ./... | ||
| GOTOOLCHAIN=go1.25.5 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.6.1 run ./... $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this intended? this will probably override the environment variable specified in a command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended, though good point about it overriding, fixed now. Right now on our machines run_lint script won't work because it uses the system's go version rather than the projects go version.
| vulnID = "GO-2023-1558" | ||
| }) | ||
|
|
||
| if vulnID == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is redundant now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, removed.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a type safety issue in the MCP command by replacing sync.Map with a standard map protected by an RWMutex. This is a great improvement for correctness and maintainability. The addition of comprehensive integration tests is also a valuable contribution that significantly improves confidence in the MCP functionality. The changes to the build scripts are also sensible. I have one suggestion to improve the concurrency handling in the vulnerability cache to prevent a potential race condition during cache misses.
Fixes #2421
Also: