From a9734167e44f81d11b31bfc8f2dd4cf2ee333a10 Mon Sep 17 00:00:00 2001 From: John CSA <103165870+jluocsa@users.noreply.github.com> Date: Sat, 16 May 2026 21:54:11 -0700 Subject: [PATCH 1/2] test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal 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/github-mcp-server#2483 --- pkg/github/tools_static_validation_test.go | 213 +++++++++++++++++++++ 1 file changed, 213 insertions(+) create mode 100644 pkg/github/tools_static_validation_test.go diff --git a/pkg/github/tools_static_validation_test.go b/pkg/github/tools_static_validation_test.go new file mode 100644 index 0000000000..881ee0d3df --- /dev/null +++ b/pkg/github/tools_static_validation_test.go @@ -0,0 +1,213 @@ +package github + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestAllToolRegistrationsExplicitlySetReadOnlyHint statically scans every +// non-test Go source file in this package and asserts that every mcp.Tool +// composite literal explicitly sets Annotations.ReadOnlyHint. +// +// This complements TestAllToolsHaveRequiredMetadata, which can only check +// that Annotations is non-nil at runtime: Go cannot distinguish an +// unset bool field from one explicitly set to false. Source-level +// validation closes that gap and prevents future tool registrations +// from silently defaulting ReadOnlyHint to false (which has caused +// downstream agents to prompt for human approval on read-intent tools). +// +// Related issue: github/github-mcp-server#2483 +func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { + pkgDir, err := os.Getwd() + require.NoError(t, err, "must be able to resolve package directory") + + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, pkgDir, func(info os.FileInfo) bool { + // Skip test files: they are allowed to construct mcp.Tool literals + // for fixtures or mocks where ReadOnlyHint is not meaningful. + return !strings.HasSuffix(info.Name(), "_test.go") + }, parser.ParseComments) + require.NoError(t, err, "parser.ParseDir on package directory") + require.NotEmpty(t, pkgs, "expected at least one package parsed") + + type violation struct { + file string + line int + toolName string + reason string + } + var violations []violation + literalsSeen := 0 + + for _, pkg := range pkgs { + for filename, file := range pkg.Files { + ast.Inspect(file, func(n ast.Node) bool { + cl, ok := n.(*ast.CompositeLit) + if !ok { + return true + } + if !isMCPToolType(cl.Type) { + return true + } + literalsSeen++ + + toolName := extractToolName(cl) + if toolName == "" { + toolName = "" + } + pos := fset.Position(cl.Pos()) + rel, _ := filepath.Rel(pkgDir, filename) + if rel == "" { + rel = filepath.Base(filename) + } + + annotations := findFieldValue(cl, "Annotations") + if annotations == nil { + violations = append(violations, violation{ + file: rel, + line: pos.Line, + toolName: toolName, + reason: "mcp.Tool literal is missing an Annotations field", + }) + return true + } + + annoLit := unwrapAnnotationsLiteral(annotations) + if annoLit == nil { + // Annotations is set to something we can't statically + // verify (e.g. a function call). Flag it so reviewers + // can confirm ReadOnlyHint is honored. + violations = append(violations, violation{ + file: rel, + line: pos.Line, + toolName: toolName, + reason: "Annotations is not an &mcp.ToolAnnotations{...} literal; ReadOnlyHint cannot be statically verified", + }) + return true + } + + if findFieldValue(annoLit, "ReadOnlyHint") == nil { + violations = append(violations, violation{ + file: rel, + line: pos.Line, + toolName: toolName, + reason: "ToolAnnotations literal does not explicitly set ReadOnlyHint", + }) + } + return true + }) + } + } + + require.NotZero(t, literalsSeen, + "expected to discover at least one mcp.Tool literal; AST walker may be broken") + + if len(violations) > 0 { + var msg strings.Builder + msg.WriteString("Found tool registrations that do not explicitly set ReadOnlyHint:\n") + for _, v := range violations { + msg.WriteString(" - ") + msg.WriteString(v.file) + msg.WriteString(":") + msg.WriteString(strconv.Itoa(v.line)) + msg.WriteString(" tool=") + msg.WriteString(v.toolName) + msg.WriteString(": ") + msg.WriteString(v.reason) + msg.WriteString("\n") + } + msg.WriteString("\nEvery mcp.Tool registration must declare Annotations.ReadOnlyHint explicitly ") + msg.WriteString("(true for read-only tools, false for tools with side effects). ") + msg.WriteString("See pkg/github/tools_static_validation_test.go.") + t.Fatal(msg.String()) + } +} + +// isMCPToolType reports whether the given AST expression refers to mcp.Tool. +func isMCPToolType(expr ast.Expr) bool { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return false + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + return ident.Name == "mcp" && sel.Sel != nil && sel.Sel.Name == "Tool" +} + +// isMCPToolAnnotationsType reports whether the given AST expression refers to mcp.ToolAnnotations. +func isMCPToolAnnotationsType(expr ast.Expr) bool { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return false + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + return ident.Name == "mcp" && sel.Sel != nil && sel.Sel.Name == "ToolAnnotations" +} + +// findFieldValue returns the value expression for the named keyed field of a +// composite literal, or nil if the field is absent. +func findFieldValue(cl *ast.CompositeLit, name string) ast.Expr { + for _, elt := range cl.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + key, ok := kv.Key.(*ast.Ident) + if !ok { + continue + } + if key.Name == name { + return kv.Value + } + } + return nil +} + +// unwrapAnnotationsLiteral attempts to extract the *ast.CompositeLit for +// &mcp.ToolAnnotations{...} or mcp.ToolAnnotations{...} from an expression. +// Returns nil if the expression is not a statically inspectable literal. +func unwrapAnnotationsLiteral(expr ast.Expr) *ast.CompositeLit { + if u, ok := expr.(*ast.UnaryExpr); ok && u.Op == token.AND { + expr = u.X + } + cl, ok := expr.(*ast.CompositeLit) + if !ok { + return nil + } + if !isMCPToolAnnotationsType(cl.Type) { + return nil + } + return cl +} + +// extractToolName returns the literal value of the Name field of an mcp.Tool +// composite literal, or empty string if the value is not a basic string literal. +func extractToolName(cl *ast.CompositeLit) string { + v := findFieldValue(cl, "Name") + if v == nil { + return "" + } + bl, ok := v.(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return "" + } + // Strip surrounding quotes; tolerate raw strings too. + s := bl.Value + if len(s) >= 2 && (s[0] == '"' || s[0] == '`') { + s = s[1 : len(s)-1] + } + return s +} From 335634382c3b57a76227e591e0adb666d331f719 Mon Sep 17 00:00:00 2001 From: John CSA <103165870+jluocsa@users.noreply.github.com> Date: Fri, 22 May 2026 20:07:22 -0700 Subject: [PATCH 2/2] test(github): address reviewer feedback on ReadOnlyHint check - Resolve each file's local alias for github.com/modelcontextprotocol/go-sdk/mcp via file.Imports rather than hard-coding the "mcp" qualifier, so the check also covers files that import the SDK under a non-default alias. - Detect positional (unkeyed) composite literals and report a dedicated diagnostic instead of producing misleading "missing field" violations. - Drop the brittle 'expected to discover at least one mcp.Tool literal' assertion: if registrations move behind constructors/factories the AST walker legitimately finds nothing. - Use strconv.Unquote to decode tool-name string literals (handles escapes in interpreted strings); fall back to the raw lexeme on parse error. --- pkg/github/tools_static_validation_test.go | 114 ++++++++++++++++----- 1 file changed, 86 insertions(+), 28 deletions(-) diff --git a/pkg/github/tools_static_validation_test.go b/pkg/github/tools_static_validation_test.go index 881ee0d3df..5028df2882 100644 --- a/pkg/github/tools_static_validation_test.go +++ b/pkg/github/tools_static_validation_test.go @@ -13,6 +13,12 @@ import ( "github.com/stretchr/testify/require" ) +// mcpImportPath is the canonical module path of the MCP go-sdk that pkg/github +// imports as `mcp` (or under an alias). Per-file alias resolution lets this +// test correctly identify mcp.Tool / mcp.ToolAnnotations literals even when a +// file imports the SDK under a non-default local name. +const mcpImportPath = "github.com/modelcontextprotocol/go-sdk/mcp" + // TestAllToolRegistrationsExplicitlySetReadOnlyHint statically scans every // non-test Go source file in this package and asserts that every mcp.Tool // composite literal explicitly sets Annotations.ReadOnlyHint. @@ -45,19 +51,22 @@ func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { reason string } var violations []violation - literalsSeen := 0 for _, pkg := range pkgs { for filename, file := range pkg.Files { + aliases := mcpAliasesFor(file) + if len(aliases) == 0 { + // File does not import the MCP go-sdk; no tool literals possible. + continue + } ast.Inspect(file, func(n ast.Node) bool { cl, ok := n.(*ast.CompositeLit) if !ok { return true } - if !isMCPToolType(cl.Type) { + if !isQualifiedType(cl.Type, aliases, "Tool") { return true } - literalsSeen++ toolName := extractToolName(cl) if toolName == "" { @@ -69,6 +78,16 @@ func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { rel = filepath.Base(filename) } + if hasUnkeyedFields(cl) { + violations = append(violations, violation{ + file: rel, + line: pos.Line, + toolName: toolName, + reason: "mcp.Tool literal uses positional (unkeyed) fields; this check requires keyed fields so Annotations.ReadOnlyHint can be verified", + }) + return true + } + annotations := findFieldValue(cl, "Annotations") if annotations == nil { violations = append(violations, violation{ @@ -80,7 +99,7 @@ func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { return true } - annoLit := unwrapAnnotationsLiteral(annotations) + annoLit := unwrapAnnotationsLiteral(annotations, aliases) if annoLit == nil { // Annotations is set to something we can't statically // verify (e.g. a function call). Flag it so reviewers @@ -94,6 +113,16 @@ func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { return true } + if hasUnkeyedFields(annoLit) { + violations = append(violations, violation{ + file: rel, + line: pos.Line, + toolName: toolName, + reason: "mcp.ToolAnnotations literal uses positional (unkeyed) fields; use keyed fields so ReadOnlyHint can be verified", + }) + return true + } + if findFieldValue(annoLit, "ReadOnlyHint") == nil { violations = append(violations, violation{ file: rel, @@ -107,8 +136,9 @@ func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { } } - require.NotZero(t, literalsSeen, - "expected to discover at least one mcp.Tool literal; AST walker may be broken") + // Intentionally do not assert that any literals were observed: if tool + // registrations move behind constructors/factories there may be nothing + // for this check to validate, and that is a legitimate state. if len(violations) > 0 { var msg strings.Builder @@ -131,21 +161,32 @@ func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) { } } -// isMCPToolType reports whether the given AST expression refers to mcp.Tool. -func isMCPToolType(expr ast.Expr) bool { - sel, ok := expr.(*ast.SelectorExpr) - if !ok { - return false - } - ident, ok := sel.X.(*ast.Ident) - if !ok { - return false +// mcpAliasesFor returns the set of local identifiers under which the given +// file imports the MCP go-sdk (mcpImportPath). The default unaliased import +// resolves to the package name "mcp". Blank (`_`) and dot (`.`) imports are +// skipped because tool literals cannot meaningfully be qualified through them. +func mcpAliasesFor(file *ast.File) map[string]struct{} { + aliases := map[string]struct{}{} + for _, imp := range file.Imports { + path, err := strconv.Unquote(imp.Path.Value) + if err != nil || path != mcpImportPath { + continue + } + if imp.Name != nil { + if imp.Name.Name == "_" || imp.Name.Name == "." { + continue + } + aliases[imp.Name.Name] = struct{}{} + continue + } + aliases["mcp"] = struct{}{} } - return ident.Name == "mcp" && sel.Sel != nil && sel.Sel.Name == "Tool" + return aliases } -// isMCPToolAnnotationsType reports whether the given AST expression refers to mcp.ToolAnnotations. -func isMCPToolAnnotationsType(expr ast.Expr) bool { +// isQualifiedType reports whether expr is a SelectorExpr of the form +// . where alias is in the provided alias set. +func isQualifiedType(expr ast.Expr, aliases map[string]struct{}, typeName string) bool { sel, ok := expr.(*ast.SelectorExpr) if !ok { return false @@ -154,7 +195,23 @@ func isMCPToolAnnotationsType(expr ast.Expr) bool { if !ok { return false } - return ident.Name == "mcp" && sel.Sel != nil && sel.Sel.Name == "ToolAnnotations" + if _, ok := aliases[ident.Name]; !ok { + return false + } + return sel.Sel != nil && sel.Sel.Name == typeName +} + +// hasUnkeyedFields reports whether the composite literal has any positional +// (non-key/value) elements. The static check cannot reliably map positional +// fields without full type information, so such literals are rejected with a +// dedicated diagnostic rather than producing false "missing field" violations. +func hasUnkeyedFields(cl *ast.CompositeLit) bool { + for _, elt := range cl.Elts { + if _, ok := elt.(*ast.KeyValueExpr); !ok { + return true + } + } + return false } // findFieldValue returns the value expression for the named keyed field of a @@ -177,9 +234,9 @@ func findFieldValue(cl *ast.CompositeLit, name string) ast.Expr { } // unwrapAnnotationsLiteral attempts to extract the *ast.CompositeLit for -// &mcp.ToolAnnotations{...} or mcp.ToolAnnotations{...} from an expression. -// Returns nil if the expression is not a statically inspectable literal. -func unwrapAnnotationsLiteral(expr ast.Expr) *ast.CompositeLit { +// &mcp.ToolAnnotations{...} or mcp.ToolAnnotations{...} from an expression, +// resolving the MCP package's local alias per file. +func unwrapAnnotationsLiteral(expr ast.Expr, aliases map[string]struct{}) *ast.CompositeLit { if u, ok := expr.(*ast.UnaryExpr); ok && u.Op == token.AND { expr = u.X } @@ -187,7 +244,7 @@ func unwrapAnnotationsLiteral(expr ast.Expr) *ast.CompositeLit { if !ok { return nil } - if !isMCPToolAnnotationsType(cl.Type) { + if !isQualifiedType(cl.Type, aliases, "ToolAnnotations") { return nil } return cl @@ -195,6 +252,9 @@ func unwrapAnnotationsLiteral(expr ast.Expr) *ast.CompositeLit { // extractToolName returns the literal value of the Name field of an mcp.Tool // composite literal, or empty string if the value is not a basic string literal. +// Interpreted ("...") and raw (`...`) string literals are handled via +// strconv.Unquote so embedded escapes are decoded correctly; the raw +// literal value is returned as a best-effort fallback if unquoting fails. func extractToolName(cl *ast.CompositeLit) string { v := findFieldValue(cl, "Name") if v == nil { @@ -204,10 +264,8 @@ func extractToolName(cl *ast.CompositeLit) string { if !ok || bl.Kind != token.STRING { return "" } - // Strip surrounding quotes; tolerate raw strings too. - s := bl.Value - if len(s) >= 2 && (s[0] == '"' || s[0] == '`') { - s = s[1 : len(s)-1] + if unq, err := strconv.Unquote(bl.Value); err == nil { + return unq } - return s + return bl.Value }