Skip to content

test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal#2486

Open
jluocsa wants to merge 1 commit into
github:mainfrom
jluocsa:feat/lint-readonly-hint-annotation-2483
Open

test(github): enforce explicit ReadOnlyHint on every mcp.Tool literal#2486
jluocsa wants to merge 1 commit into
github:mainfrom
jluocsa:feat/lint-readonly-hint-annotation-2483

Conversation

@jluocsa
Copy link
Copy Markdown

@jluocsa jluocsa commented May 17, 2026

What

Adds a source-level (AST) validation test that statically walks every non-test .go file in pkg/github, finds every mcp.Tool{...} composite literal, and fails if any of them omits an explicit Annotations.ReadOnlyHint: field.

Refs #2483 (item 3 in the description: "Add a CI check that fails if any tool registered via mcp.NewTool lacks an explicit ReadOnlyHint").

Why

The existing TestAllToolsHaveRequiredMetadata already enforces non-nil Annotations and explicitly documents its limitation:

// We can't distinguish between "not set" and "set to false" for a bool,
// but having Annotations non-nil confirms the developer thought about it.

That gap is exactly what #2483 wants closed. The Go runtime cannot tell an unset bool from one explicitly set to false, but the AST can. This test surfaces the omission at build time so a future read-intent tool cannot silently default to ReadOnlyHint: false — which has triggered downstream agents to prompt for human approval on safe read operations.

How

  • New test TestAllToolRegistrationsExplicitlySetReadOnlyHint in pkg/github/tools_static_validation_test.go.
  • Uses go/parser + go/ast (stdlib only — no new dependencies).
  • For every *ast.CompositeLit of type mcp.Tool:
    • Requires Annotations: is present.
    • Requires the value is an &mcp.ToolAnnotations{...} literal that can be statically inspected (anything else is flagged so a human can confirm).
    • Requires ReadOnlyHint: is among the keyed fields.
  • Failure message lists each offender as <file>:<line> tool=<name>: <reason>.

Verification

  • go test ./pkg/github -run TestAllToolRegistrationsExplicitlySetReadOnlyHint -v → PASS (97/97 current tool literals are compliant).
  • Fault-injected by removing ReadOnlyHint: true from issue_read:
    Found tool registrations that do not explicitly set ReadOnlyHint:
      - issues.go:255 tool=issue_read: ToolAnnotations literal does not explicitly set ReadOnlyHint
    
  • go test ./... → all green.
  • go vet ./pkg/github/... → clean.
  • No production code, schema, or toolsnap changes — no docs regeneration needed.

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 main because 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., explicit Title: annotation, OWASP-flavored side-effect classification) as follow-ups if maintainers want them.

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
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.

2 participants