feat: Add MCP wildcard permission helpers (P2)

[arcaputo3]

Summary

• Add McpToolName.wildcard(serverName) for server-specific wildcards (e.g., mcp__weather-api__*)
• Add McpToolName.wildcardAll for global MCP wildcard (mcp__*)
• Add McpToolNames.wildcard instance method for convenient per-server wildcards

These helpers make it easier to auto-approve MCP tools via allowedTools:

// Allow all tools from a specific server
AgentOptions.default.withAllowedTools(McpToolName.wildcard("weather-api"))

// Or using the instance method
AgentOptions.default.withAllowedTools(WeatherTools.wildcard)

// Allow all MCP tools (use with caution)
AgentOptions.default.withAllowedTools(McpToolName.wildcardAll)

Test plan

• Added 3 new tests for wildcard functionality
• All 156 tests pass

Closes TJC-564

🤖 Generated with Claude Code

[claude]

Pull Request Review

Summary

This PR adds helpful wildcard permission patterns for MCP tools, making it easier to auto-approve tools from specific MCP servers or all MCP tools at once. The implementation is clean, well-documented, and includes comprehensive test coverage.

---

✅ Strengths

1. Code Quality & Design

• Clean, focused implementation with clear separation of concerns
• Follows existing code patterns and conventions consistently
• Type-safe API that leverages the existing ToolName infrastructure
• Excellent documentation with inline examples in Scaladoc

2. API Design

• Three levels of wildcard granularity make sense:
  • McpToolName.wildcard(serverName) - server-specific (e.g., mcp__weather-api__*)
  • McpToolNames.wildcard - instance method for convenience
  • McpToolName.wildcardAll - global MCP wildcard (with appropriate warning)
• Good use of the existing ToolName.Custom wrapper
• The instance method on McpToolNames provides nice ergonomics

3. Test Coverage

• All three wildcard methods have dedicated test cases (lines 117-133 in McpToolNameSpec.scala:117-133)
• Tests verify the correct pattern generation
• Integrates well with existing test structure

4. Documentation

• Clear Scaladoc with usage examples for each method
• Appropriate security warning on wildcardAll (line 49 in McpToolName.scala:49)
• PR description provides good context and examples

---

⚠️ Potential Issues & Concerns

1. CRITICAL: Wildcard Matching Implementation Missing

The most significant concern is that I cannot find the actual wildcard matching logic in the codebase. This PR creates wildcard patterns like mcp__weather-api__* and mcp__*, but I don't see where these patterns are actually matched against tool names.

Looking at CanUseTool.allowOnly (CanUseTool.scala:69-71), it uses:

if allowedTools.contains(toolName) then ZIO.succeed(PermissionResult.allow)

This is an exact match check, not a wildcard match. For the wildcards to work, you'd need pattern matching logic like:

allowedTools.exists(pattern => matchesWildcard(toolName.raw, pattern.raw))

Questions:

• Is wildcard matching implemented in the TypeScript SDK layer and this PR just creates the patterns?
• If so, this should be documented in the code
• If not, these wildcards won't actually work and matching logic needs to be added

2. Security Consideration

McpToolName.wildcardAll returns "mcp__*" which might match more than intended if the wildcard matcher isn't precise. Consider:

• Does "mcp__*" match "mcp__a__b__c__d" (more underscores than expected)?
• Should it be "mcp__*__*" to enforce the server/tool structure?

3. Minor: Inconsistent Documentation

In line 54 of McpToolName.scala:54, the comment says:

// Allows: all mcp__*__* tools

But the actual pattern returned is "mcp__*", not "mcp__*__*". The comment should match the implementation, or the implementation should match the comment.

---

🔍 Suggestions for Improvement

1. Add Integration Test
Add a test that verifies wildcards actually work end-to-end with AgentOptions.withAllowedTools:

test("wildcard patterns allow matching MCP tools"):
  val options = AgentOptions.default
    .withAllowedTools(McpToolName.wildcard("weather-api"))
  
  // Verify that mcp__weather-api__get_weather would be allowed
  // (This test would reveal if wildcard matching isn't implemented)

2. Clarify Wildcard Semantics
Add documentation explaining:

• How wildcards are matched (glob-style? regex?)
• Whether they're case-sensitive
• Where the matching logic lives (SDK vs Scala)

3. Consider Adding a Validation Helper

extension (pattern: ToolName)
  def matches(toolName: ToolName): Boolean =
    // Implement wildcard matching logic here
    // or delegate to SDK if it handles it

4. Type Safety Enhancement (Optional)
Consider creating a WildcardPattern type to distinguish wildcards from regular tool names:

opaque type WildcardPattern <: ToolName = ToolName
object WildcardPattern:
  def apply(pattern: String): WildcardPattern = ToolName.Custom(pattern)

---

📝 Minor Code Style Notes

1. Line length: All code follows good line length conventions
2. Naming: Method names are clear and self-documenting
3. Scala 3 syntax: Properly uses significant indentation and modern Scala 3 features

---

🧪 Testing

According to the PR description, all 156 tests pass. However:

• Consider running tests in CI to verify
• The wildcard matching functionality needs integration testing to ensure it works as expected

---

🔒 Security Assessment

Severity: Medium

The wildcardAll pattern is appropriately marked with a warning, but consider:

1. Making it even more explicit (e.g., rename to wildcardAllDangerous)
2. Adding a @deprecated or lint warning for production use
3. Documenting the security implications more thoroughly

---

✅ Final Recommendation

Conditional Approval - pending clarification on the critical issue:

This PR is well-implemented and follows best practices, BUT the core functionality depends on wildcard matching logic that I cannot locate in the codebase. Before merging:

1. MUST: Clarify where wildcard matching is implemented (SDK layer or missing?)
2. MUST: Add integration tests showing wildcards actually work
3. SHOULD: Fix the documentation inconsistency in line 54
4. SHOULD: Consider the security implications of the mcp__* pattern

If wildcard matching is already implemented in the SDK layer and these are just pattern generators, please add a comment explaining this. Otherwise, matching logic needs to be added to the Scala layer.

---

📊 Review Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Clean, idiomatic Scala 3
API Design	⭐⭐⭐⭐	Good ergonomics, minor improvements possible
Documentation	⭐⭐⭐⭐	Excellent inline docs, examples provided
Test Coverage	⭐⭐⭐	Unit tests good, needs integration tests
Security	⚠️	Appropriate warnings, but needs review
Completeness	⚠️	Wildcard matching logic unclear

Overall: Good work! Address the wildcard matching question and this will be ready to merge.

🤖 Generated with Claude Code (Sonnet 4.5)

[claude]

Pull Request Review

Summary

This PR adds wildcard helper methods for MCP tool permissions, making it easier to auto-approve all tools from a specific server or all MCP tools globally. The implementation is clean, well-documented, and includes comprehensive test coverage.

---

✅ Strengths

Code Quality

• Excellent documentation: All new methods have clear ScalaDoc with usage examples
• Type safety: Uses ToolName.Custom wrapper, maintaining type safety throughout
• Consistent API: The three wildcard methods (wildcard(serverName), wildcardAll, and McpToolNames.wildcard) follow a consistent naming pattern
• Well-structured: Code is logically organized and easy to understand

Test Coverage

• Comprehensive tests: 3 new wildcard tests + 3 integration tests with AgentOptions
• Good test organization: Tests are grouped by functionality with clear section headers
• Integration testing: Tests verify the wildcard patterns work correctly with AgentOptions.withAllowedTools

Design

• Convenience without complexity: The instance method McpToolNames.wildcard provides a nice shorthand that avoids repeating the server name
• Clear security warning: wildcardAll includes appropriate "use with caution" documentation

---

💡 Suggestions

1. Pattern Format Documentation

The implementation relies on glob-style pattern matching in the TypeScript SDK layer. Consider adding a comment about what patterns are supported:

/** Create a wildcard pattern to allow all tools from a server.
  *
  * Wildcard matching uses glob-style patterns (* matches any characters).
  * The pattern is evaluated by the Claude Code TypeScript SDK.
  * ...

Impact: Low - Documentation enhancement
Priority: Optional

2. Validation Considerations

The wildcard(serverName) method does not validate the server name format. While this provides flexibility, consider whether you want to add validation:

def wildcard(serverName: String): ToolName =
  require(serverName.nonEmpty && !serverName.contains("*"), "Invalid server name")
  ToolName.Custom(s"mcp__${serverName}__*")

However, given that this would be caught at runtime anyway when no tools match, the current approach is acceptable.

Impact: Low - Would catch errors earlier but adds complexity
Priority: Optional (probably not worth it)

3. Test Coverage - Edge Cases

Consider adding tests for edge cases:

• Empty server name: McpToolName.wildcard("")
• Server name with special characters: McpToolName.wildcard("my-server-123")
• Server name with double underscores: McpToolName.wildcard("my__server")

Impact: Low - Current implementation handles these fine, but explicit tests would document expected behavior
Priority: Nice to have

4. Example in PR Description

The PR description shows excellent usage examples. Consider adding these to the main ScalaDoc for McpToolNames:

abstract class McpToolNames(val serverName: String):
  /** Get a wildcard pattern to allow all tools from this server.
    *
    * Example:
    * {{{
    * object WeatherTools extends McpToolNames("weather-api"):
    *   val getWeather = tool("get_weather")
    *
    * // Allow all weather tools without permission prompts
    * AgentOptions.default.withAllowedTools(WeatherTools.wildcard)
    * // or
    * AgentOptions.default.withAllowedTools(McpToolName.wildcard("weather-api"))
    * }}}

Impact: Low - Documentation enhancement
Priority: Nice to have

---

🔍 Specific Code Review

McpToolName.scala (lines 30-61)

✅ Correct implementation: The wildcard patterns follow the expected mcp__serverName__* and mcp__* formats

✅ Good separation: wildcard vs wildcardAll makes the distinction between server-specific and global wildcards clear

McpToolNameSpec.scala (lines 117-217)

✅ Thorough testing: Tests cover both the static methods and instance method, plus integration

✅ Pattern matching assertions: Tests correctly verify the ToolName.Custom wrapper and raw string value

⚠️ Minor observation: The integration tests at lines 186-217 import AgentOptions inside each test. This is fine but could be moved to the top for consistency with other imports.

Example:

// At top of file after other imports
import com.tjclp.scalagent.config.AgentOptions

// Then in tests, just use directly
test("wildcard patterns work with AgentOptions.withAllowedTools"):
  val opts = AgentOptions.default.withAllowedTools(McpToolName.wildcard("weather-api"))
  // ...

Impact: Trivial - Style preference
Priority: Optional

---

🛡️ Security Considerations

✅ Appropriate warning: The wildcardAll method includes a "use with caution" warning
✅ Safe by design: Wildcards are strings that get matched by the SDK layer - no code injection risk
✅ Clear semantics: The naming makes it obvious what each wildcard does

Recommendation

Consider adding a note in the documentation about when to use wildcards vs explicit tool lists:

/** Create a wildcard pattern to allow all tools from a server.
  *
  * Use this when you trust all tools from a server and want to avoid
  * permission prompts. For production use, consider explicitly listing
  * allowed tools for better security and clarity.
  * ...

---

🚀 Performance Considerations

✅ No performance impact: Wildcard methods are simple string construction
✅ Efficient: No runtime overhead beyond pattern matching in the SDK

---

📋 Test Plan Verification

✅ PR claims "All 156 tests pass" - this is verifiable via CI
✅ 3 new wildcard tests added as promised
✅ Integration tests verify end-to-end functionality

---

Final Verdict

Approval Status: ✅ APPROVED

This is a high-quality PR that:

• Adds useful convenience methods with clear semantics
• Includes comprehensive tests and documentation
• Maintains type safety and consistency with existing code
• Has no security or performance concerns

The suggestions above are minor enhancements that could be addressed in a follow-up PR if desired, but they do not block merging this PR.

Recommendation

Merge when ready - This PR is ready to merge as-is. The suggested improvements are optional nice-to-haves.

---

🤖 Review completed

Great work on the implementation and testing! 🎉