test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal#2486
Open
jluocsa wants to merge 1 commit into
Open
test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal#2486jluocsa wants to merge 1 commit into
jluocsa wants to merge 1 commit into
Conversation
Adds a source-level (AST) validation test that walks every non-test Go file in pkg/github and fails if any mcp.Tool composite literal omits Annotations.ReadOnlyHint. The existing TestAllToolsHaveRequiredMetadata can only assert that Annotations is non-nil at runtime: Go cannot distinguish an unset bool field from one explicitly set to false. The new test closes that gap so future read-intent tools cannot silently default to ReadOnlyHint=false, which has caused downstream agents to prompt for human approval on safe read operations. All 97 current mcp.Tool registrations pass. Fault-injected by removing ReadOnlyHint from issue_read and confirmed the test reports the exact file, line, tool name, and reason. Refs github#2483
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a source-level (AST) validation test that statically walks every non-test
.gofile inpkg/github, finds everymcp.Tool{...}composite literal, and fails if any of them omits an explicitAnnotations.ReadOnlyHint:field.Refs #2483 (item 3 in the description: "Add a CI check that fails if any tool registered via
mcp.NewToollacks an explicitReadOnlyHint").Why
The existing
TestAllToolsHaveRequiredMetadataalready enforces non-nilAnnotationsand explicitly documents its limitation:That gap is exactly what #2483 wants closed. The Go runtime cannot tell an unset
boolfrom one explicitly set tofalse, but the AST can. This test surfaces the omission at build time so a future read-intent tool cannot silently default toReadOnlyHint: false— which has triggered downstream agents to prompt for human approval on safe read operations.How
TestAllToolRegistrationsExplicitlySetReadOnlyHintinpkg/github/tools_static_validation_test.go.go/parser+go/ast(stdlib only — no new dependencies).*ast.CompositeLitof typemcp.Tool:Annotations:is present.&mcp.ToolAnnotations{...}literal that can be statically inspected (anything else is flagged so a human can confirm).ReadOnlyHint:is among the keyed fields.<file>:<line> tool=<name>: <reason>.Verification
go test ./pkg/github -run TestAllToolRegistrationsExplicitlySetReadOnlyHint -v→ PASS (97/97 current tool literals are compliant).ReadOnlyHint: truefromissue_read:go test ./...→ all green.go vet ./pkg/github/...→ clean.Scope notes
This PR is intentionally minimal: it covers item 3 of #2483 (the CI guardrail). The audit portion of the issue (items 1–2) is currently a no-op against
mainbecause all 97 registrations already set the hint, so the value of this change is purely forward-protection. Happy to fold in additional checks (e.g., explicitTitle:annotation, OWASP-flavored side-effect classification) as follow-ups if maintainers want them.