Skip to content

Conversation

@omgitsads
Copy link
Member

Previously these were pointers to a Bool, now they are concrete types and this was incorrectly ported in the Go SDK move.

Closes:

@omgitsads omgitsads requested a review from a team as a code owner December 1, 2025 15:25
Copilot AI review requested due to automatic review settings December 1, 2025 15:25
@omgitsads omgitsads merged commit 9b34211 into main Dec 1, 2025
20 checks passed
@omgitsads omgitsads deleted the fix-broken-read-only-hint branch December 1, 2025 15:27
Copilot finished reviewing on behalf of omgitsads December 1, 2025 15:27
Copy link
Contributor

Copilot AI left a 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 the RegisterSpecificTools 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 && readOnly to if !tool.Tool.Annotations.ReadOnlyHint && readOnly to 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.go that verifies RegisterSpecificTools correctly skips write tools (where ReadOnlyHint is false) when readOnly=true is passed, and that it registers read-only tools (where ReadOnlyHint is true).
		if !tool.Tool.Annotations.ReadOnlyHint && readOnly {
			// Skip write tools in read-only mode
			skippedTools = 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.

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