Skip to content

Enforce explicit ReadOnlyHint on all MCP tool definitions via AST validation#2487

Draft
Copilot wants to merge 1 commit into
mainfrom
copilot/fix-missing-annotations-check
Draft

Enforce explicit ReadOnlyHint on all MCP tool definitions via AST validation#2487
Copilot wants to merge 1 commit into
mainfrom
copilot/fix-missing-annotations-check

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026

Summary

Adds a compile-time-style guard in tests to ensure every mcp.Tool in pkg/github explicitly sets Annotations.ReadOnlyHint. This prevents silent regressions where missing annotations degrade runtime permission behavior.

Why

ReadOnlyHint presence must be explicit and continuously enforced, including for future refactors and feature-flagged tool variants.

What changed

  • AST-based annotation enforcement
    • Added TestToolDefinitionsRequireExplicitReadOnlyHint in pkg/github/tools_validation_test.go.
    • The test walks all non-test Go files in pkg/github, inspects mcp.Tool{...} literals, and fails if Annotations.ReadOnlyHint is missing.
  • Failure diagnostics
    • Test emits file:line + tool=<name> + missing-field reason to make fixes immediate.
  • Example enforced pattern
    Annotations: &mcp.ToolAnnotations{
        Title:        "...",
        ReadOnlyHint: true, // or false, but must be explicit
    }

MCP impact

  • No tool or API changes
    Validation-only change; no runtime schema/behavior/tool registration changes.
  • Tool schema or behavior changed
  • New tool added

Prompts tested (tool changes only)

  • N/A (no tool behavior change)

Security / limits

  • No security or limits impact
    Test-only guardrail; no auth/data-path changes.
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
    • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR

Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.

Lint & tests

  • Linted locally with ./script/lint
  • Tested locally with ./script/test

Docs

  • Not needed
  • Updated (README / docs / examples)

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