Skip to content

OSS release prep#2568

Merged
SamMorrowDrums merged 3 commits into
mainfrom
sammorrowdrums/readonly-hint-validator-shared
May 29, 2026
Merged

OSS release prep#2568
SamMorrowDrums merged 3 commits into
mainfrom
sammorrowdrums/readonly-hint-validator-shared

Conversation

@SamMorrowDrums
Copy link
Copy Markdown
Collaborator

What

Release prep PR — currently contains the cherry-picked work from #2486 plus a small refactor to make the new ReadOnlyHint guardrail reusable from github/github-mcp-server-remote (and any other downstream consumer of this repo as a library). More small prep items can stack here before the next release.

Contents

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

Cherry-picked from #2486 (author: @jluocsa, authorship preserved). AST scan that fails the build if any mcp.Tool{...} literal in pkg/github omits an explicit Annotations.ReadOnlyHint. Closes the runtime gap noted in TestAllToolsHaveRequiredMetadata (Go can't distinguish unset bool from false).

Refs #2483 (item 3).

2. test(github): address reviewer feedback on ReadOnlyHint check

Cherry-picked from #2486 (author: @jluocsa, authorship preserved).

3. refactor(toolvalidation): extract ReadOnlyHint scanner into reusable package

New change. Moves the AST scan out of pkg/github's test file and into a new exported package, pkg/toolvalidation, so the remote server (which imports this repo) can apply the same guardrail with a one-line test:

func TestAllToolRegistrationsExplicitlySetReadOnlyHint(t *testing.T) {
    dir, _ := os.Getwd()
    violations, err := toolvalidation.ScanReadOnlyHint(dir)
    require.NoError(t, err)
    if len(violations) > 0 {
        t.Fatal(toolvalidation.FormatReadOnlyHintViolations(violations))
    }
}

This follows the repo's stated convention that functions usable as a library from github-mcp-server-remote should be exported.

Refactor details

  • New pkg/toolvalidation/readonlyhint.go with ScanReadOnlyHint, FormatReadOnlyHintViolations, and the ReadOnlyHintViolation type — stdlib only.
  • New pkg/toolvalidation/readonlyhint_test.go with dedicated unit tests using in-memory fixtures: compliant literal, missing hint, missing annotations, non-literal annotations, aliased import (import sdk "..."), positional fields, and a file with no MCP import. This gives the validator real coverage it didn't have when it lived inline.
  • pkg/github/tools_static_validation_test.go shrunk from 271 → 36 lines, behavior preserved.

Verification

  • script/lint → 0 issues
  • script/test → all packages pass, including the new pkg/toolvalidation and the thin pkg/github wrapper
  • No production-code, schema, or toolsnap changes; no docs regeneration needed.

Follow-up

I'll open a parallel PR against github/github-mcp-server-remote once this lands (or against a prerelease tag) that bumps the go.mod to this commit and adds the thin one-line test there.

Co-authored-by: John CSA 103165870+jluocsa@users.noreply.github.com

jluocsa and others added 3 commits May 29, 2026 10:52
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 #2483
- 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.
…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>
@SamMorrowDrums SamMorrowDrums marked this pull request as ready for review May 29, 2026 09:10
@SamMorrowDrums SamMorrowDrums requested a review from a team as a code owner May 29, 2026 09:10
Copilot AI review requested due to automatic review settings May 29, 2026 09:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Release-prep PR that cherry-picks the AST-based ReadOnlyHint guardrail from #2486 and refactors it into a new exported package, pkg/toolvalidation, so the remote server can reuse the same check.

Changes:

  • New pkg/toolvalidation package with ScanReadOnlyHint, FormatReadOnlyHintViolations, and ReadOnlyHintViolation, using only stdlib parsing.
  • Unit tests covering compliant/missing/positional/aliased/no-import fixtures plus formatter and missing-directory error.
  • pkg/github/tools_static_validation_test.go shrunk to a thin wrapper that scans the package and fails with formatted violations.
Show a summary per file
File Description
pkg/toolvalidation/readonlyhint.go New exported AST scanner that flags mcp.Tool literals missing explicit Annotations.ReadOnlyHint.
pkg/toolvalidation/readonlyhint_test.go Fixture-driven unit tests for scanner + formatter behavior.
pkg/github/tools_static_validation_test.go Thin wrapper applying the shared scanner to pkg/github.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 0

@SamMorrowDrums SamMorrowDrums merged commit f5f9c72 into main May 29, 2026
20 checks passed
@SamMorrowDrums SamMorrowDrums deleted the sammorrowdrums/readonly-hint-validator-shared branch May 29, 2026 09:16
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.

3 participants