- Notifications
You must be signed in to change notification settings - Fork1.4k
Add ListGlobalSecurityAdvisories and GetGlobalSecurityAdvisory#693
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds support for listing and retrieving global security advisories from the GitHub Advisory Database by introducing two new tools, wiring them into the toolset group, and providing corresponding tests and snapshots.
- Registers a new
global_security_advisoriestoolset inDefaultToolsetGroup - Implements
ListGlobalSecurityAdvisoriesandGetGlobalSecurityAdvisoryhandlers with option parsing - Adds unit tests and tool definition snapshots for both tools
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/tools.go | Registers the newglobal_security_advisories toolset |
| pkg/github/global_security_advisories.go | Implements handlers for listing and getting global security advisories |
| pkg/github/global_security_advisories_test.go | Adds unit tests covering the new advisory tools |
| pkg/github/toolsnaps/list_global_security_advisories.snap | Snapshot for the list tool definition |
| pkg/github/toolsnaps/get_global_security_advisory.snap | Snapshot for the get tool definition |
Comments suppressed due to low confidence (1)
pkg/github/global_security_advisories_test.go:39
- There are no test cases covering error handling when the ListGlobalSecurityAdvisories API returns a non-200 status or the client errors. Add tests to simulate failures and verify the handler returns an error result.
tests := []struct {| return nil, fmt.Errorf("failed to get GitHub client: %w", err) | ||
| } | ||
| advisory, resp, err := client.SecurityAdvisories.GetGlobalSecurityAdvisories(ctx, ghsaID) |
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.
The call to client.SecurityAdvisories.GetGlobalSecurityAdvisories uses an incorrect method name; the correct method is GetGlobalSecurityAdvisory (singular). Update this to call the intended API.
| advisory,resp,err:=client.SecurityAdvisories.GetGlobalSecurityAdvisories(ctx,ghsaID) | |
| advisory,resp,err:=client.SecurityAdvisories.GetGlobalSecurityAdvisory(ctx,ghsaID) |
Copilot uses AI. Check for mistakes.
| // Note: EPSS parameters may not be supported in current Go SDK version | ||
| // Check if these fields exist before using them | ||
| // For now, we accept the parameters but don't use them in the API call | ||
| if _, err := OptionalParam[string](request, "epss_percentage"); err != nil { |
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.
The epss_percentage and epss_percentile parameters are parsed but never applied to the API call, which can confuse users. Consider implementing support for these options or removing them from the tool signature until they're supported.
Copilot uses AI. Check for mistakes.
Closes:#684