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

[jluocsa]

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.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a static-analysis Go test to enforce that every mcp.Tool composite literal in pkg/github explicitly sets Annotations.ReadOnlyHint, preventing unintended defaulting to false.

Changes:

• Introduces an AST-based package scan over non-test Go files to find mcp.Tool{...} literals.
• Validates that Annotations is present and statically inspectable as a mcp.ToolAnnotations{...} literal.
• Fails the test with a detailed, file/line/tool-name-specific report when ReadOnlyHint is not explicitly set.

[pachecocordovamoiseseduardo-byte]

🧐

[jluocsa]

@pachecocordovamoiseseduardo-byte Thanks for the review! The feedback was addressed in 3356343:

1. Alias-aware resolution — Per-file MCP import alias is now resolved via file.Imports instead of hard-coding the mcp qualifier, so the check also covers files that import the SDK under a non-default alias.
2. Unkeyed composite literals — Detected explicitly and reported with a dedicated diagnostic, instead of producing misleading "missing field" violations.
3. Brittle assertion removed — Dropped the expected to discover at least one mcp.Tool literal check; if registrations move behind constructors/factories the walker can legitimately find nothing.
4. Robust tool-name decoding — Use strconv.Unquote for tool-name string literals (handles escapes), with a graceful fallback to the raw lexeme on parse error.

Could you take another look when you have a moment? 🙏

[SamMorrowDrums]

Commits merged separately.

[jluocsa]

To capture the rationale here for reviewers:

The read-only (ReadOnlyHint) annotations are load-bearing — they gate read-intent tool behavior — and an audit (including the investigation in #2487) confirmed that none are currently missing. So this check is a regression guard against future drift, not a fix for an existing gap. The value is preventing a tool from silently shipping without an explicit annotation down the line.

One open question for maintainers on reuse: the logic currently lives in a _test.go AST walk, so it can't be imported by the remote server. If enforcing the same invariant server-side is desirable, I'm happy to extract the core check into an exported (non-test) helper that both the unit test here and the remote server can call — keeping a single source of truth. Glad to do that in this PR or as a follow-up; let me know the preference.

[jluocsa]

To capture the rationale here for reviewers:

The read-only (ReadOnlyHint) annotations are load-bearing — they gate read-intent tool behavior — and an audit (including the investigation in #2487) confirmed that none are currently missing. So this check is a regression guard against future drift, not a fix for an existing gap. The value is preventing a tool from silently shipping without an explicit annotation down the line.

One open question for maintainers on reuse: the logic currently lives in a _test.go AST walk, so it can't be imported by the remote server. If enforcing the same invariant server-side is desirable, I'm happy to extract the core check into an exported (non-test) helper that both the unit test here and the remote server can call — keeping a single source of truth. Glad to do that in this PR or as a follow-up; let me know the preference.