Skip to content

Conversation

@tommaso-moro
Copy link
Contributor

@tommaso-moro tommaso-moro commented Jan 9, 2026

Summary

This PR ensures that invalid tool names error out, like in the remote server and as per product requirements.

The PR Also moves the cleanTools() logic inside the Builder , which allows us to

  1. clean up the code and be consistent with the toolsets logic for a cleaner caller code
// Before
WithTools(github.CleanTools(cfg.EnabledTools))

// After
WithTools(cfg.EnabledTools)
  1. clean up the remote server by removing the custom tools cleaning + validation logic there and relying on the inventory to compute unrecognized tools instead (see remote server PR: https://github.com/github/github-mcp-server-remote/pull/587 )

The Builder now fully owns the responsibility of processing tool inputs, including cleaning and validation. Callers don't need to know about these implementation details.

Why

  • Product reason: invalid tool names should error out, this is needed mainly for that
  • Engineering reason: this improves our logic for handling invalid tool names and makes it more consistent both between tools and toolsets and also across remote and local MCP

What changed

  • added logic to throw errors if invalid tool names are provided
  • moved cleanTools logic to inside the Builder
  • adds tests

MCP impact

  • No tool or API changes
  • Tool schema or behavior changed
  • New tool added

Prompts tested (tool changes only)

Security / limits

  • No security or limits impact
  • 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)

@tommaso-moro tommaso-moro changed the title Tommy/bug fix invalid tool should error out Bug fix: invalid tool should error out Jan 9, 2026
@tommaso-moro tommaso-moro marked this pull request as ready for review January 9, 2026 14:11
@tommaso-moro tommaso-moro requested a review from a team as a code owner January 9, 2026 14:11
Copilot AI review requested due to automatic review settings January 9, 2026 14:11
Copy link
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

This PR adds validation to error out when invalid tool names are provided, making the local server behavior consistent with the remote server. It also refactors the tool cleaning logic by moving it from pkg/github/tools.go into the inventory builder, simplifying the API and centralizing validation.

Key changes:

  • Invalid tool names now cause server startup errors instead of being silently ignored
  • Tool name cleaning (trimming, deduplication) is now handled internally by the builder
  • Unrecognized tools are tracked and reported, similar to how unrecognized toolsets are handled

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
pkg/inventory/registry.go Adds unrecognizedTools field and UnrecognizedTools() getter method to expose validation results
pkg/inventory/builder.go Moves cleanTools function inside the builder and adds logic to track unrecognized tool names during build
pkg/inventory/registry_test.go Adds comprehensive test suite for UnrecognizedTools() functionality covering valid tools, invalid tools, and deprecated aliases
internal/ghmcp/server.go Updates to use simplified API (WithTools(cfg.EnabledTools) instead of WithTools(github.CleanTools(...))) and adds error handling for unrecognized tools

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