- Notifications
You must be signed in to change notification settings - Fork3.1k
Check if the tool is _NOT_ read only before skipping it in read-only mode#1514
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
Conversation
9b34211 intomainUh oh!
There was an error while loading.Please reload this page.
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 fixes a critical logic error in theRegisterSpecificTools function that was incorrectly ported during the Go SDK migration. The bug inverted the condition for skipping write tools in read-only mode, causing the function to skip read-only tools instead.
Key Change:
- Corrects the condition from
if tool.Tool.Annotations.ReadOnlyHint && readOnlytoif !tool.Tool.Annotations.ReadOnlyHint && readOnlyto properly skip write tools (those without the ReadOnlyHint) when in read-only mode
Comments suppressed due to low confidence (1)
pkg/toolsets/toolsets.go:332
- The corrected read-only mode filtering logic lacks test coverage. Consider adding a test case in
pkg/toolsets/toolsets_test.gothat verifiesRegisterSpecificToolscorrectly skips write tools (whereReadOnlyHintis false) whenreadOnly=trueis passed, and that it registers read-only tools (whereReadOnlyHintis true).
if !tool.Tool.Annotations.ReadOnlyHint && readOnly {// Skip write tools in read-only modeskippedTools = append(skippedTools, toolName)continue}You can also share your feedback on Copilot code review for a chance to win a $100 gift card.Take the survey.
Previously these were pointers to a Bool, now they are concrete types and this was incorrectly ported in the Go SDK move.
Closes: