From 0798569becfab3cb16d65518cdd28ede489995df 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/3] 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 5e1c7e6008d189a483fc2ede0f32cfc020129481 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/3] 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 } From 18434bd756645c3c5eb0c559fff05c2601a6fa83 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Fri, 29 May 2026 10:55:46 +0200 Subject: [PATCH 3/3] refactor(toolvalidation): extract ReadOnlyHint scanner into reusable package Move the AST-based ReadOnlyHint scan introduced in #2486 out of pkg/github's test file and into a new exported package, pkg/toolvalidation, so downstream consumers (notably github/github-mcp-server-remote, which uses this repo as a library) can apply the same guardrail to their own tool registrations with a one-line test: violations, err := toolvalidation.ScanReadOnlyHint(pkgDir) Changes: - New pkg/toolvalidation/readonlyhint.go with ScanReadOnlyHint, FormatReadOnlyHintViolations, and the ReadOnlyHintViolation type. - Dedicated unit tests for the scanner using in-memory fixtures (compliant, missing-hint, missing-annotations, non-literal, aliased import, positional fields, file without mcp import). - pkg/github/tools_static_validation_test.go shrunk to a thin wrapper that calls ScanReadOnlyHint against its own package directory; the existing behavior for pkg/github is preserved. No production-code, schema, or toolsnap changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/tools_static_validation_test.go | 261 +-------------------- pkg/toolvalidation/readonlyhint.go | 256 ++++++++++++++++++++ pkg/toolvalidation/readonlyhint_test.go | 176 ++++++++++++++ 3 files changed, 445 insertions(+), 248 deletions(-) create mode 100644 pkg/toolvalidation/readonlyhint.go create mode 100644 pkg/toolvalidation/readonlyhint_test.go diff --git a/pkg/github/tools_static_validation_test.go b/pkg/github/tools_static_validation_test.go index 5028df2882..34cd309d6a 100644 --- a/pkg/github/tools_static_validation_test.go +++ b/pkg/github/tools_static_validation_test.go @@ -1,271 +1,36 @@ package github import ( - "go/ast" - "go/parser" - "go/token" "os" - "path/filepath" - "strconv" - "strings" "testing" + "github.com/github/github-mcp-server/pkg/toolvalidation" "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. // +// The AST scan itself lives in pkg/toolvalidation so downstream packages +// (e.g. github/github-mcp-server-remote) can apply the same guardrail to +// their own tool registrations without duplicating the parser logic. +// // 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). +// 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 - - 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 !isQualifiedType(cl.Type, aliases, "Tool") { - return true - } - - toolName := extractToolName(cl) - if toolName == "" { - toolName = "" - } - pos := fset.Position(cl.Pos()) - rel, _ := filepath.Rel(pkgDir, filename) - if rel == "" { - 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{ - file: rel, - line: pos.Line, - toolName: toolName, - reason: "mcp.Tool literal is missing an Annotations field", - }) - return true - } - - 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 - // 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 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, - line: pos.Line, - toolName: toolName, - reason: "ToolAnnotations literal does not explicitly set ReadOnlyHint", - }) - } - return true - }) - } - } - - // 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. - + violations, err := toolvalidation.ScanReadOnlyHint(pkgDir) + require.NoError(t, err) 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()) - } -} - -// 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 aliases -} - -// 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 - } - ident, ok := sel.X.(*ast.Ident) - if !ok { - return false - } - 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 -// 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, -// 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 - } - cl, ok := expr.(*ast.CompositeLit) - if !ok { - return nil - } - if !isQualifiedType(cl.Type, aliases, "ToolAnnotations") { - 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. -// 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 { - return "" - } - bl, ok := v.(*ast.BasicLit) - if !ok || bl.Kind != token.STRING { - return "" - } - if unq, err := strconv.Unquote(bl.Value); err == nil { - return unq + t.Fatal(toolvalidation.FormatReadOnlyHintViolations(violations)) } - return bl.Value } diff --git a/pkg/toolvalidation/readonlyhint.go b/pkg/toolvalidation/readonlyhint.go new file mode 100644 index 0000000000..bcde92a5ec --- /dev/null +++ b/pkg/toolvalidation/readonlyhint.go @@ -0,0 +1,256 @@ +// Package toolvalidation provides source-level (AST) validators for MCP tool +// registrations. It is intended to be consumed from _test.go files in any +// package that registers mcp.Tool literals (including downstream repositories +// such as github-mcp-server-remote) so the same guardrails apply everywhere +// without duplicating the parsing logic. +package toolvalidation + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strconv" + "strings" +) + +// MCPImportPath is the canonical module path of the MCP go-sdk. Source files +// that import this path under any alias (including the default `mcp`) are +// candidates for tool-literal validation. +const MCPImportPath = "github.com/modelcontextprotocol/go-sdk/mcp" + +// ReadOnlyHintViolation describes a single mcp.Tool composite literal that +// failed the ReadOnlyHint check. +type ReadOnlyHintViolation struct { + // File is the path to the offending source file, made relative to the + // scan directory when possible. + File string + // Line is the 1-indexed line number of the offending literal. + Line int + // ToolName is the value of the Name field on the mcp.Tool literal, or + // "" when it cannot be statically extracted. + ToolName string + // Reason is a human-readable explanation of why the literal failed. + Reason string +} + +// String renders a violation in the format used by FormatReadOnlyHintViolations: +// ": tool=: ". +func (v ReadOnlyHintViolation) String() string { + return fmt.Sprintf("%s:%d tool=%s: %s", v.File, v.Line, v.ToolName, v.Reason) +} + +// ScanReadOnlyHint parses every non-test .go file in dir (a single package +// directory) and returns a violation for each mcp.Tool composite literal that +// does not explicitly set Annotations.ReadOnlyHint. +// +// The Go runtime cannot distinguish an unset bool field from one explicitly +// set to false, so this AST-level check exists to prevent future tool +// registrations from silently defaulting ReadOnlyHint to false — which has +// triggered downstream agents to prompt for human approval on safe read +// operations. +// +// Callers typically invoke this from a _test.go file: +// +// dir, _ := os.Getwd() +// violations, err := toolvalidation.ScanReadOnlyHint(dir) +func ScanReadOnlyHint(dir string) ([]ReadOnlyHintViolation, error) { + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, dir, 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) + if err != nil { + return nil, fmt.Errorf("parse package directory %q: %w", dir, err) + } + + var violations []ReadOnlyHintViolation + for _, pkg := range pkgs { + for filename, file := range pkg.Files { + aliases := mcpAliasesFor(file) + if len(aliases) == 0 { + continue + } + rel, relErr := filepath.Rel(dir, filename) + if relErr != nil || rel == "" { + rel = filepath.Base(filename) + } + ast.Inspect(file, func(n ast.Node) bool { + cl, ok := n.(*ast.CompositeLit) + if !ok { + return true + } + if !isQualifiedType(cl.Type, aliases, "Tool") { + return true + } + violations = append(violations, checkToolLiteral(cl, aliases, rel, fset.Position(cl.Pos()).Line)...) + return true + }) + } + } + return violations, nil +} + +// FormatReadOnlyHintViolations renders a single multi-line error message +// suitable for passing to t.Fatal. Returns "" when violations is empty. +func FormatReadOnlyHintViolations(violations []ReadOnlyHintViolation) string { + if len(violations) == 0 { + return "" + } + 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.String()) + msg.WriteByte('\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/toolvalidation.ScanReadOnlyHint.") + return msg.String() +} + +func checkToolLiteral(cl *ast.CompositeLit, aliases map[string]struct{}, file string, line int) []ReadOnlyHintViolation { + toolName := extractToolName(cl) + if toolName == "" { + toolName = "" + } + mk := func(reason string) ReadOnlyHintViolation { + return ReadOnlyHintViolation{File: file, Line: line, ToolName: toolName, Reason: reason} + } + + if hasUnkeyedFields(cl) { + return []ReadOnlyHintViolation{mk("mcp.Tool literal uses positional (unkeyed) fields; this check requires keyed fields so Annotations.ReadOnlyHint can be verified")} + } + + annotations := findFieldValue(cl, "Annotations") + if annotations == nil { + return []ReadOnlyHintViolation{mk("mcp.Tool literal is missing an Annotations field")} + } + + annoLit := unwrapAnnotationsLiteral(annotations, aliases) + if annoLit == nil { + return []ReadOnlyHintViolation{mk("Annotations is not an &mcp.ToolAnnotations{...} literal; ReadOnlyHint cannot be statically verified")} + } + + if hasUnkeyedFields(annoLit) { + return []ReadOnlyHintViolation{mk("mcp.ToolAnnotations literal uses positional (unkeyed) fields; use keyed fields so ReadOnlyHint can be verified")} + } + + if findFieldValue(annoLit, "ReadOnlyHint") == nil { + return []ReadOnlyHintViolation{mk("ToolAnnotations literal does not explicitly set ReadOnlyHint")} + } + return nil +} + +// 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 aliases +} + +// 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 + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + 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 +// 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, +// 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 + } + cl, ok := expr.(*ast.CompositeLit) + if !ok { + return nil + } + if !isQualifiedType(cl.Type, aliases, "ToolAnnotations") { + 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. +// 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 { + return "" + } + bl, ok := v.(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return "" + } + if unq, err := strconv.Unquote(bl.Value); err == nil { + return unq + } + return bl.Value +} diff --git a/pkg/toolvalidation/readonlyhint_test.go b/pkg/toolvalidation/readonlyhint_test.go new file mode 100644 index 0000000000..7ef3c4829b --- /dev/null +++ b/pkg/toolvalidation/readonlyhint_test.go @@ -0,0 +1,176 @@ +package toolvalidation_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/github/github-mcp-server/pkg/toolvalidation" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// writePackage writes a single Go source file into a fresh temp directory and +// returns that directory, suitable for passing to ScanReadOnlyHint. +func writePackage(t *testing.T, filename, source string) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, filename), []byte(source), 0o600)) + return dir +} + +func TestScanReadOnlyHint(t *testing.T) { + t.Parallel() + + const compliant = `package fixture + +import "github.com/modelcontextprotocol/go-sdk/mcp" + +var Tool = mcp.Tool{ + Name: "compliant_tool", + Annotations: &mcp.ToolAnnotations{ + ReadOnlyHint: true, + }, +} +` + + const missingHint = `package fixture + +import "github.com/modelcontextprotocol/go-sdk/mcp" + +var Tool = mcp.Tool{ + Name: "missing_hint", + Annotations: &mcp.ToolAnnotations{ + Title: "no hint", + }, +} +` + + const missingAnnotations = `package fixture + +import "github.com/modelcontextprotocol/go-sdk/mcp" + +var Tool = mcp.Tool{ + Name: "missing_annotations", +} +` + + const nonLiteralAnnotations = `package fixture + +import "github.com/modelcontextprotocol/go-sdk/mcp" + +func annotations() *mcp.ToolAnnotations { return &mcp.ToolAnnotations{ReadOnlyHint: true} } + +var Tool = mcp.Tool{ + Name: "non_literal", + Annotations: annotations(), +} +` + + const unkeyedTool = `package fixture + +import "github.com/modelcontextprotocol/go-sdk/mcp" + +var Tool = mcp.Tool{"unkeyed", "desc", nil, nil, nil, nil} +` + + const aliasedImport = `package fixture + +import sdk "github.com/modelcontextprotocol/go-sdk/mcp" + +var Tool = sdk.Tool{ + Name: "aliased", + Annotations: &sdk.ToolAnnotations{ + ReadOnlyHint: false, + }, +} +` + + const noMCPImport = `package fixture + +import "fmt" + +var _ = fmt.Sprintln("nothing to scan here") +` + + cases := []struct { + name string + source string + expectCount int + expectReason string + expectToolName string + }{ + {name: "compliant literal passes", source: compliant, expectCount: 0}, + {name: "aliased import is detected", source: aliasedImport, expectCount: 0}, + {name: "file without mcp import is skipped", source: noMCPImport, expectCount: 0}, + { + name: "missing ReadOnlyHint is flagged", + source: missingHint, + expectCount: 1, + expectReason: "does not explicitly set ReadOnlyHint", + expectToolName: "missing_hint", + }, + { + name: "missing Annotations is flagged", + source: missingAnnotations, + expectCount: 1, + expectReason: "missing an Annotations field", + expectToolName: "missing_annotations", + }, + { + name: "non-literal Annotations is flagged", + source: nonLiteralAnnotations, + expectCount: 1, + expectReason: "not an &mcp.ToolAnnotations{...} literal", + expectToolName: "non_literal", + }, + { + name: "positional Tool fields are flagged", + source: unkeyedTool, + expectCount: 1, + expectReason: "positional (unkeyed) fields", + expectToolName: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := writePackage(t, "fixture.go", tc.source) + violations, err := toolvalidation.ScanReadOnlyHint(dir) + require.NoError(t, err) + require.Len(t, violations, tc.expectCount) + if tc.expectCount == 0 { + return + } + v := violations[0] + assert.Equal(t, "fixture.go", v.File) + assert.Greater(t, v.Line, 0) + assert.Equal(t, tc.expectToolName, v.ToolName) + assert.Contains(t, v.Reason, tc.expectReason) + }) + } +} + +func TestFormatReadOnlyHintViolations(t *testing.T) { + t.Parallel() + + assert.Empty(t, toolvalidation.FormatReadOnlyHintViolations(nil)) + + msg := toolvalidation.FormatReadOnlyHintViolations([]toolvalidation.ReadOnlyHintViolation{{ + File: "issues.go", + Line: 42, + ToolName: "issue_read", + Reason: "ToolAnnotations literal does not explicitly set ReadOnlyHint", + }}) + assert.True(t, strings.HasPrefix(msg, "Found tool registrations that do not explicitly set ReadOnlyHint:")) + assert.Contains(t, msg, "issues.go:42 tool=issue_read") + assert.Contains(t, msg, "true for read-only tools, false for tools with side effects") +} + +func TestScanReadOnlyHint_ReturnsErrorForMissingDirectory(t *testing.T) { + t.Parallel() + _, err := toolvalidation.ScanReadOnlyHint(filepath.Join(t.TempDir(), "does-not-exist")) + require.Error(t, err) +}