feat: add tool discovery mode with automatic client capability fallback

[cameroncooke]

Summary

This PR introduces a new Tool Discovery Mode for XcodeBuildMCP that enables AI-powered workflow selection based on natural language task descriptions, plus a comprehensive refactor addressing code quality and testing violations.

Background/Details

Major Refactor (WIP Commit)

The first commit contains a comprehensive refactor that addresses code quality and testing violations:

• Handler Testing Violations Fixed: Replaced 7 handler testing violations across 3 test files to follow architectural principle of never testing handlers directly
• Dependency Injection: All tests now use createMockExecutor() and dependency injection patterns instead of direct handler calls
• Code Quality: Added comprehensive documentation, improved type safety, and enhanced test coverage
• Architecture Compliance: Maintains 100% ESLint compliance and proper separation of concerns

Tool Discovery Mode (Main Feature)

The second commit introduces the core feature:

• Smart Dynamic Mode: When XCODEBUILDMCP_DYNAMIC_TOOLS=true, server starts with only discover_tools and discover_projs available
• AI-Powered Selection: Uses client's LLM via MCP sampling to analyze task descriptions and select relevant workflow groups
• Bulk Registration: Uses new MCP SDK bulk registration methods for efficient tool loading of selected workflows
• Workflow-Based Organization: Tools are organized into 15 workflow groups for intelligent selection

Solution

Technical Implementation

1. Dynamic Mode Option: Set XCODEBUILDMCP_DYNAMIC_TOOLS=true to enable discovery mode
2. Static Mode Default: Without the environment variable, all 83+ tools are registered normally
3. AI Workflow Selection: discover_tools analyzes natural language task descriptions to select relevant workflows
4. Efficient Registration: Selected workflows are registered in bulk for better performance

Architecture Changes

• Modified src/index.ts to support both static and dynamic modes
• Enhanced src/core/dynamic-tools.ts with bulk registration support
• Added workflow selection logic using MCP sampling
• Updated dependency to use forked TypeScript SDK with bulk registration methods

Testing

Reproduction Steps

1. Dynamic Mode: Set XCODEBUILDMCP_DYNAMIC_TOOLS=true and start with sampling-capable client
2. Static Mode: Start without environment variable → All tools available immediately
3. Workflow Selection: Use discover_tools with task description → AI selects relevant workflows

Validation Method

• Dynamic Mode: Confirmed discover_tools successfully analyzes tasks and enables relevant workflows
• Static Mode: Verified normal operation with all tools available from startup
• Bulk Registration: Tested efficient registration of workflow groups
• Test Coverage: Maintained comprehensive test suite with all handler violations fixed

Edge Cases

• Empty task descriptions handled gracefully
• Invalid workflow selections fail safely
• MCP sampling errors handled appropriately
• Both modes work with appropriate client capabilities

Notes

Client Compatibility

• Dynamic Mode: Requires MCP clients with sampling capability (VS Code with Claude)
• Static Mode: Works with all MCP clients (default behavior)
• Configuration Required: Must set XCODEBUILDMCP_DYNAMIC_TOOLS=true to enable discovery mode

Performance Benefits

• Context Window: Dynamic mode only loads relevant tools, reducing LLM context usage
• Startup Speed: Bulk registration significantly faster than individual tool registration
• Memory Efficiency: Only selected workflows loaded in dynamic mode

Dependencies

• Requires forked TypeScript SDK: github:cameroncooke/typescript-sdk#main
• Maintains backward compatibility with existing configurations
• No breaking changes to existing tool interfaces

This implementation provides intelligent tool discovery for clients that support it, while maintaining full backward compatibility through explicit configuration.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Important

Review skipped

More than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review.

58 files out of 203 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/handler-testing-violations

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai generate unit tests to generate unit tests for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[claude]

Claude finished @cameroncooke's task —— View job

---

Code Review: Tool Discovery Mode Implementation

This is an excellent PR that successfully introduces AI-powered tool discovery while addressing significant technical debt. The implementation demonstrates strong architectural discipline and comprehensive testing improvements.

✅ Major Strengths

1. Exemplary Testing Refactor

• Perfect dependency injection implementation: All 7 handler testing violations properly fixed
• Consistent patterns: Every test now uses createMockExecutor() instead of direct handler calls
• Comprehensive coverage: Three-dimensional testing (input validation, command generation, output processing)
• Literal validation: Tests verify exact expected outputs, catching regressions effectively

2. Clean Architecture

• Proper separation of concerns: Logic functions cleanly separated from handlers
• Smart fallback mechanism: Dynamic mode with automatic static fallback for non-sampling clients
• Global server management: Well-handled via type-safe global state
• Bulk registration: Efficient tool loading using forked SDK with proper error handling

3. Robust Error Handling

• Client capability detection: Graceful handling of sampling vs non-sampling clients
• JSON parsing resilience: Handles multiple content formats from LLM responses
• Workflow validation: Filters invalid workflows from LLM responses
• Comprehensive error messages: Clear user feedback for all failure modes

4. Performance Considerations

• Code splitting: Dynamic imports with proper workflow loaders
• Bulk registration: Uses registerTools() instead of individual registration
• Context window optimization: Dynamic mode only loads relevant tools
• Efficient fallback: Quick detection and bulk static registration

⚠️ Areas for Improvement

1. Security Considerations

Medium Priority

• Global state access: globalThis.mcpServer creates potential injection points
• LLM prompt injection: User task descriptions are inserted directly into prompts without sanitization
• File system operations: list_devices.ts uses temp files with timestamp-based naming

Recommendations:

// Consider sanitizing user input
function sanitizeTaskDescription(input: string): string {
  return input.replace(/[<>{}]/g, '').trim().slice(0, 500);
}

// Use more secure temp file naming
const tempJsonPath = join(tempDir, `devicectl-${crypto.randomUUID()}.json`);

2. Type Safety

Low Priority

• SDK interface casting: Multiple as unknown casts in bulk registration
• globalThis typing: Could benefit from more specific typing

// src/core/dynamic-tools.ts:158
inputSchema: tool.config.inputSchema as unknown, // Cast needed for SDK interface

3. Code Quality

Minor Issues

• Commented code: Some TODO/FIXME comments could be addressed
• Magic numbers: maxTokens: 200 hardcoded in discover_tools.ts
• Error handling consistency: Some error paths return different response formats

🚀 Notable Innovations

1. Smart Client Detection

The automatic fallback mechanism is particularly well-designed:

// Dynamic mode by default with automatic fallback
const isDynamicModeEnabled = process.env.XCODEBUILDMCP_DYNAMIC_TOOLS === 'true';

2. LLM Integration Pattern

Excellent use of MCP sampling for workflow selection:

• Clear selection guidelines in prompts
• Robust JSON parsing with multiple format support
• Workflow validation and filtering

3. Dependency Management

The forked SDK dependency is properly managed:

"@modelcontextprotocol/sdk": "github:cameroncooke/typescript-sdk#main"

📊 Code Quality Metrics

• Test Coverage: Comprehensive with proper dependency injection
• ESLint Compliance: 100% compliant with strict TypeScript rules
• Architecture Compliance: Perfect adherence to established patterns
• Documentation: Well-documented with clear inline comments

🔧 Specific Technical Comments

src/index.ts:64-67

// Make server available globally for dynamic tools
(globalThis as { mcpServer?: McpServer }).mcpServer = server;

✅ Good: Type-safe global state management
⚠️ Consider: Could this be passed as a parameter instead of global state?

src/core/dynamic-tools.ts:141-175

The bulk registration with fallback is excellent:

// Use bulk registration with proper types - no runtime checking needed
try {
  const registeredTools = registerAndTrackTools(server, toolRegistrations);
} catch (error) {
  // Fallback to simplified tool registration one at a time
}

✅ Excellent: Robust error handling with graceful degradation

src/mcp/tools/discovery/discover_tools.ts:100-105

const samplingResult = await server.server.createMessage({
  messages: [{ role: 'user', content: { type: 'text', text: userPrompt } }],
  maxTokens: 200,
});

⚠️ Minor: Consider making maxTokens configurable

🎯 Final Recommendation

APPROVE - This is a high-quality implementation that:

• ✅ Successfully introduces the requested feature
• ✅ Fixes significant technical debt (handler testing violations)
• ✅ Maintains architectural consistency
• ✅ Provides comprehensive test coverage
• ✅ Includes robust error handling

The security considerations are minor and can be addressed in follow-up PRs. The core implementation is solid and ready for production.

📋 Post-Merge Suggestions

1. Security hardening: Add user input sanitization for LLM prompts
2. Configuration: Make LLM parameters (maxTokens, etc.) configurable
3. Type safety: Remove remaining as unknown casts when SDK types improve
4. Performance monitoring: Add metrics for dynamic vs static mode usage

---

[claude]

Claude finished @cameroncooke's task —— View job

---

Code Review: Tool Discovery Mode Implementation

I've conducted a comprehensive review of this PR that introduces AI-powered tool discovery and addresses testing violations. Here's my detailed analysis:

✅ Major Strengths

1. Exemplary Testing Refactor

• Perfect dependency injection implementation: All 7 handler testing violations properly fixed across the codebase
• Consistent patterns: Every test now uses createMockExecutor() and createMockFileSystemExecutor() instead of direct handler calls
• Comprehensive coverage: Three-dimensional testing (input validation, command generation, output processing) consistently applied
• Literal validation: Tests verify exact expected outputs, catching regressions effectively

Example from src/mcp/tools/device-shared/__tests__/list_devices.test.ts:103:

await list_devicesLogic({}, trackingExecutor, mockPathDeps, mockFsDeps);
expect(commandCalls[0].command).toEqual([
  'xcrun', 'devicectl', 'list', 'devices', '--json-output', '/tmp/devicectl-123.json'
]);

2. Clean Architecture Implementation

• Smart fallback mechanism: Dynamic mode with automatic static fallback for non-sampling clients
• Global server management: Well-handled via type-safe global state in src/index.ts:64
• Bulk registration: Efficient tool loading using forked SDK with proper error handling in src/core/dynamic-tools.ts:141-175
• Proper separation of concerns: Logic functions cleanly separated from MCP handlers

3. Robust Error Handling

• Client capability detection: Graceful handling at src/mcp/tools/discovery/discover_tools.ts:61
• JSON parsing resilience: Handles multiple content formats from LLM responses (lines 118-130)
• Workflow validation: Filters invalid workflows from LLM responses (lines 147-157)
• Comprehensive fallback: Individual tool registration fallback when bulk registration fails (lines 175-204)

4. Performance Optimizations

• Code splitting: Dynamic imports with proper workflow loaders
• Bulk registration: Uses registerAndTrackTools() instead of individual registration
• Context window optimization: Dynamic mode only loads relevant tools
• Efficient static fallback: Quick detection and bulk static registration

⚠️ Areas for Improvement

1. Security Considerations (Medium Priority)

Issue identified in Cursor review: Missing null check in src/mcp/tools/discovery/discover_tools.ts:61

if (!server.server.getClientCapabilities()?.sampling) {

Risk: Potential runtime error if server.server is null/undefined.

Additional security concerns:

• LLM prompt injection: User task descriptions inserted directly into prompts without sanitization (line 79)
• Global state access: globalThis.mcpServer creates potential injection points (src/index.ts:64)
• Temp file naming: Uses timestamp-based naming in list_devices.ts:39 which could be predictable

Recommendations:

// Add null safety
if (!server?.server?.getClientCapabilities()?.sampling) {

// Sanitize user input
function sanitizeTaskDescription(input: string): string {
  return input.replace(/[<>{}]/g, '').trim().slice(0, 500);
}

2. Race Condition Issue (High Priority)

Bug identified in Cursor review: Race condition in src/utils/command.ts:155 for detached processes:

setTimeout(() => {
  if (childProcess.pid) {
    resolve({ success: true, output: '', process: childProcess });
  }
}, 100);

Risk: Promise can be resolved multiple times by both timeout and event handlers.

3. Type Safety Improvements (Low Priority)

• SDK interface casting: Multiple as unknown casts in bulk registration (line 158)
• globalThis typing: Could benefit from more specific typing
• Magic numbers: maxTokens: 200 hardcoded in discover_tools.ts:104

🔧 Technical Implementation Review

Dynamic Mode Architecture

The implementation in src/index.ts:66-78 is well-designed:

const isDynamicModeEnabled = process.env.XCODEBUILDMCP_DYNAMIC_TOOLS === 'true';
if (isDynamicModeEnabled) {
  await registerDiscoveryTools(server);
} else {
  await registerAllToolsStatic(server);
}

Bulk Registration Pattern

Excellent use of fallback strategy in src/core/dynamic-tools.ts:142-204:

try {
  const registeredTools = registerAndTrackTools(server, toolRegistrations);
} catch (error) {
  // Fallback to individual registration
}

LLM Integration

Smart prompt engineering in src/mcp/tools/discovery/discover_tools.ts:77-98 with clear workflow selection guidelines.

🚀 Notable Innovations

1. Dependency Management: Uses forked SDK github:cameroncooke/typescript-sdk#main for bulk registration
2. Testing Philosophy: Complete elimination of Vitest mocking in favor of dependency injection
3. Client Compatibility: Automatic fallback for non-sampling clients
4. Workflow Organization: 15 well-defined workflow groups for intelligent selection

📊 Code Quality Metrics

• Test Coverage: ✅ Comprehensive with proper dependency injection patterns
• ESLint Compliance: ✅ 100% compliant with strict TypeScript rules
• Architecture Compliance: ✅ Perfect adherence to established patterns
• Security: ⚠️ Minor issues identified that should be addressed

🎯 Final Recommendation

APPROVE WITH CONDITIONS - This is a high-quality implementation that:

• ✅ Successfully introduces AI-powered tool discovery
• ✅ Fixes significant technical debt (handler testing violations)
• ✅ Maintains architectural consistency
• ✅ Provides comprehensive test coverage
• ✅ Includes robust error handling

Pre-merge requirements:

1. Fix security null check in discover_tools.ts:61
2. Address race condition in command.ts:155
3. Add input sanitization for LLM prompts

📋 Post-Merge Suggestions

1. Security hardening: Complete input sanitization implementation
2. Configuration: Make LLM parameters configurable
3. Type safety: Remove remaining as unknown casts when SDK types improve
4. Monitoring: Add metrics for dynamic vs static mode usage

The core implementation is solid and the testing improvements are exemplary. The identified issues are fixable and don't compromise the fundamental architecture.

---

[claude]

Claude finished @cameroncooke's task —— View job

---

Code Review: Tool Discovery Mode Implementation

I've conducted a comprehensive review of this PR that introduces AI-powered tool discovery and addresses testing violations. Here's my detailed analysis:

✅ Major Strengths

1. Exemplary Testing Refactor

• Perfect dependency injection implementation: All 7 handler testing violations properly fixed across the codebase
• Consistent patterns: Every test now uses createMockExecutor() and createMockFileSystemExecutor() instead of direct handler calls
• Comprehensive coverage: Three-dimensional testing (input validation, command generation, output processing) consistently applied
• Literal validation: Tests verify exact expected outputs, catching regressions effectively

Example from src/mcp/tools/device-shared/__tests__/list_devices.test.ts:103:

await list_devicesLogic({}, trackingExecutor, mockPathDeps, mockFsDeps);
expect(commandCalls[0].command).toEqual([
  'xcrun', 'devicectl', 'list', 'devices', '--json-output', '/tmp/devicectl-123.json'
]);

2. Clean Architecture Implementation

• Smart fallback mechanism: Dynamic mode with automatic static fallback for non-sampling clients
• Global server management: Well-handled via type-safe global state in src/index.ts:64
• Bulk registration: Efficient tool loading using forked SDK with proper error handling in src/core/dynamic-tools.ts:141-175
• Proper separation of concerns: Logic functions cleanly separated from MCP handlers

3. Robust Error Handling

• Client capability detection: Graceful handling at src/mcp/tools/discovery/discover_tools.ts:61
• JSON parsing resilience: Handles multiple content formats from LLM responses (lines 118-130)
• Workflow validation: Filters invalid workflows from LLM responses (lines 147-157)
• Comprehensive fallback: Individual tool registration fallback when bulk registration fails (lines 175-204)

4. Performance Optimizations

• Code splitting: Dynamic imports with proper workflow loaders
• Bulk registration: Uses registerAndTrackTools() instead of individual registration
• Context window optimization: Dynamic mode only loads relevant tools
• Efficient static fallback: Quick detection and bulk static registration

⚠️ Areas for Improvement

1. Security Considerations (Medium Priority)

Issue identified in Cursor review: Missing null check in src/mcp/tools/discovery/discover_tools.ts:61

if (!server.server.getClientCapabilities()?.sampling) {

Risk: Potential runtime error if server.server is null/undefined.

Additional security concerns:

• LLM prompt injection: User task descriptions inserted directly into prompts without sanitization (line 79)
• Global state access: globalThis.mcpServer creates potential injection points (src/index.ts:64)
• Temp file naming: Uses timestamp-based naming in list_devices.ts:39 which could be predictable

Recommendations:

// Add null safety
if (!server?.server?.getClientCapabilities()?.sampling) {

// Sanitize user input
function sanitizeTaskDescription(input: string): string {
  return input.replace(/[<>{}]/g, '').trim().slice(0, 500);
}

2. Race Condition Issue (High Priority)

Bug identified in Cursor review: Race condition in src/utils/command.ts:155 for detached processes:

setTimeout(() => {
  if (childProcess.pid) {
    resolve({ success: true, output: '', process: childProcess });
  }
}, 100);

Risk: Promise can be resolved multiple times by both timeout and event handlers.

3. Type Safety Improvements (Low Priority)

• SDK interface casting: Multiple as unknown casts in bulk registration (line 158)
• globalThis typing: Could benefit from more specific typing
• Magic numbers: maxTokens: 200 hardcoded in discover_tools.ts:104

🔧 Technical Implementation Review

Dynamic Mode Architecture

The implementation in src/index.ts:66-78 is well-designed:

const isDynamicModeEnabled = process.env.XCODEBUILDMCP_DYNAMIC_TOOLS === 'true';
if (isDynamicModeEnabled) {
  await registerDiscoveryTools(server);
} else {
  await registerAllToolsStatic(server);
}

Bulk Registration Pattern

Excellent use of fallback strategy in src/core/dynamic-tools.ts:142-204:

try {
  const registeredTools = registerAndTrackTools(server, toolRegistrations);
} catch (error) {
  // Fallback to individual registration
}

LLM Integration

Smart prompt engineering in src/mcp/tools/discovery/discover_tools.ts:77-98 with clear workflow selection guidelines.

🚀 Notable Innovations

1. Dependency Management: Uses forked SDK github:cameroncooke/typescript-sdk#main for bulk registration
2. Testing Philosophy: Complete elimination of Vitest mocking in favor of dependency injection
3. Client Compatibility: Automatic fallback for non-sampling clients
4. Workflow Organization: 15 well-defined workflow groups for intelligent selection

📊 Code Quality Metrics

• Test Coverage: ✅ Comprehensive with proper dependency injection patterns
• ESLint Compliance: ✅ 100% compliant with strict TypeScript rules
• Architecture Compliance: ✅ Perfect adherence to established patterns
• Security: ⚠️ Minor issues identified that should be addressed

🎯 Final Recommendation

APPROVE WITH CONDITIONS - This is a high-quality implementation that:

• ✅ Successfully introduces AI-powered tool discovery
• ✅ Fixes significant technical debt (handler testing violations)
• ✅ Maintains architectural consistency
• ✅ Provides comprehensive test coverage
• ✅ Includes robust error handling

Pre-merge requirements:

1. Fix security null check in discover_tools.ts:61
2. Address race condition in command.ts:155
3. Add input sanitization for LLM prompts

📋 Post-Merge Suggestions

1. Security hardening: Complete input sanitization implementation
2. Configuration: Make LLM parameters configurable
3. Type safety: Remove remaining as unknown casts when SDK types improve
4. Monitoring: Add metrics for dynamic vs static mode usage

The core implementation is solid and the testing improvements are exemplary. The identified issues are fixable and don't compromise the fundamental architecture.

---

[claude]

Claude finished @cameroncooke's task —— View job

---

Code Review: Tool Discovery Mode Implementation

I've conducted a comprehensive review of this PR that introduces AI-powered tool discovery and addresses testing violations. Here's my detailed analysis:

Review Tasks:

• Analyze the tool discovery implementation
• Review testing improvements and dependency injection changes
• Check for security vulnerabilities and bugs
• Evaluate performance considerations
• Assess test coverage improvements
• Provide final recommendations

---

✅ Major Strengths

1. Exemplary Testing Refactor

• Perfect dependency injection implementation: All 7 handler testing violations properly fixed across the codebase
• Consistent patterns: Every test now uses createMockExecutor() and createMockFileSystemExecutor() instead of direct handler calls
• Comprehensive coverage: Three-dimensional testing (input validation, command generation, output processing) consistently applied
• Literal validation: Tests verify exact expected outputs, catching regressions effectively

Example from src/mcp/tools/device-shared/__tests__/list_devices.test.ts:103:

await list_devicesLogic({}, trackingExecutor, mockPathDeps, mockFsDeps);
expect(commandCalls[0].command).toEqual([
  'xcrun', 'devicectl', 'list', 'devices', '--json-output', '/tmp/devicectl-123.json'
]);

2. Clean Architecture Implementation

• Smart fallback mechanism: Dynamic mode with automatic static fallback for non-sampling clients
• Global server management: Well-handled via type-safe global state in src/index.ts:64
• Bulk registration: Efficient tool loading using forked SDK with proper error handling in src/core/dynamic-tools.ts:141-175
• Proper separation of concerns: Logic functions cleanly separated from MCP handlers

3. Robust Error Handling

• Client capability detection: Graceful handling at src/mcp/tools/discovery/discover_tools.ts:61
• JSON parsing resilience: Handles multiple content formats from LLM responses (lines 118-130)
• Workflow validation: Filters invalid workflows from LLM responses (lines 147-157)
• Comprehensive fallback: Individual tool registration fallback when bulk registration fails (lines 175-204)

4. Performance Optimizations

• Code splitting: Dynamic imports with proper workflow loaders
• Bulk registration: Uses registerAndTrackTools() instead of individual registration
• Context window optimization: Dynamic mode only loads relevant tools
• Efficient static fallback: Quick detection and bulk static registration

⚠️ Areas for Improvement

1. Security Considerations (Medium Priority)

Issue identified in Cursor review: Missing null check in src/mcp/tools/discovery/discover_tools.ts:61

if (!server.server.getClientCapabilities()?.sampling) {

Risk: Potential runtime error if server.server is null/undefined.

Additional security concerns:

• LLM prompt injection: User task descriptions inserted directly into prompts without sanitization (line 79)
• Global state access: globalThis.mcpServer creates potential injection points (src/index.ts:64)
• Temp file naming: Uses timestamp-based naming in list_devices.ts:39 which could be predictable

Recommendations:

// Add null safety
if (!server?.server?.getClientCapabilities()?.sampling) {

// Sanitize user input
function sanitizeTaskDescription(input: string): string {
  return input.replace(/[<>{}]/g, '').trim().slice(0, 500);
}

2. Race Condition Issue (High Priority)

Bug identified in Cursor review: Race condition in src/utils/command.ts:155 for detached processes:

setTimeout(() => {
  if (childProcess.pid) {
    resolve({ success: true, output: '', process: childProcess });
  }
}, 100);

Risk: Promise can be resolved multiple times by both timeout and event handlers.

3. Code Quality Issues (Low Priority)

• Duplicate validation: UI testing tools (long_press.ts:55-62, key_sequence.ts, touch.ts) contain redundant parameter validation after migrating to createTypedTool with Zod schemas
• Type casting: Multiple as unknown casts in bulk registration (line 158)
• Magic numbers: maxTokens: 200 hardcoded in discover_tools.ts:104

🔧 Technical Implementation Review

Dynamic Mode Architecture

The implementation in src/index.ts:66-78 is well-designed:

const isDynamicModeEnabled = process.env.XCODEBUILDMCP_DYNAMIC_TOOLS === 'true';
if (isDynamicModeEnabled) {
  await registerDiscoveryTools(server);
} else {
  await registerAllToolsStatic(server);
}

Bulk Registration Pattern

Excellent use of fallback strategy in src/core/dynamic-tools.ts:142-204:

try {
  const registeredTools = registerAndTrackTools(server, toolRegistrations);
} catch (error) {
  // Fallback to individual registration
}

LLM Integration

Smart prompt engineering in src/mcp/tools/discovery/discover_tools.ts:77-98 with clear workflow selection guidelines.

🚀 Notable Innovations

1. Dependency Management: Uses forked SDK github:cameroncooke/typescript-sdk#main for bulk registration
2. Testing Philosophy: Complete elimination of Vitest mocking in favor of dependency injection
3. Client Compatibility: Automatic fallback for non-sampling clients
4. Workflow Organization: 15 well-defined workflow groups for intelligent selection

📊 Code Quality Metrics

• Test Coverage: ✅ Comprehensive with proper dependency injection patterns
• ESLint Compliance: ✅ 100% compliant with strict TypeScript rules
• Architecture Compliance: ✅ Perfect adherence to established patterns
• Security: ⚠️ Minor issues identified that should be addressed

🎯 Final Recommendation

APPROVE WITH CONDITIONS - This is a high-quality implementation that:

• ✅ Successfully introduces AI-powered tool discovery
• ✅ Fixes significant technical debt (handler testing violations)
• ✅ Maintains architectural consistency
• ✅ Provides comprehensive test coverage
• ✅ Includes robust error handling

Pre-merge requirements:

1. Fix security null check in discover_tools.ts:61
2. Address race condition in command.ts:155
3. Remove duplicate validation in UI testing tools
4. Add input sanitization for LLM prompts

📋 Post-Merge Suggestions

1. Security hardening: Complete input sanitization implementation
2. Configuration: Make LLM parameters configurable
3. Type safety: Remove remaining as unknown casts when SDK types improve
4. Monitoring: Add metrics for dynamic vs static mode usage

The core implementation is solid and the testing improvements are exemplary. The identified issues are fixable and don't compromise the fundamental architecture.

---

[claude]

Claude finished @cameroncooke's task —— View job

---

Code Review: Tool Discovery Mode Implementation

I've conducted a comprehensive review of this PR that introduces AI-powered tool discovery and addresses testing violations. Here's my detailed analysis:

Review Tasks:

• Analyze the tool discovery implementation
• Review testing improvements and dependency injection changes
• Check for security vulnerabilities and bugs
• Evaluate performance considerations
• Assess test coverage improvements
• Provide final recommendations

---

✅ Major Strengths

1. Exemplary Testing Refactor

• Perfect dependency injection implementation: All 7 handler testing violations properly fixed across the codebase
• Consistent patterns: Every test now uses createMockExecutor() and createMockFileSystemExecutor() instead of direct handler calls
• Comprehensive coverage: Three-dimensional testing (input validation, command generation, output processing) consistently applied
• Literal validation: Tests verify exact expected outputs, catching regressions effectively

Example from src/mcp/tools/device-shared/__tests__/list_devices.test.ts:103:

await list_devicesLogic({}, trackingExecutor, mockPathDeps, mockFsDeps);
expect(commandCalls[0].command).toEqual([
  'xcrun', 'devicectl', 'list', 'devices', '--json-output', '/tmp/devicectl-123.json'
]);

2. Clean Architecture Implementation

• Smart fallback mechanism: Dynamic mode with automatic static fallback for non-sampling clients
• Global server management: Well-handled via type-safe global state in src/index.ts:64
• Bulk registration: Efficient tool loading using forked SDK with proper error handling in src/core/dynamic-tools.ts:141-175
• Proper separation of concerns: Logic functions cleanly separated from MCP handlers

3. Robust Error Handling

• Client capability detection: Graceful handling at src/mcp/tools/discovery/discover_tools.ts:61
• JSON parsing resilience: Handles multiple content formats from LLM responses (lines 118-130)
• Workflow validation: Filters invalid workflows from LLM responses (lines 147-157)
• Comprehensive fallback: Individual tool registration fallback when bulk registration fails (lines 175-204)

4. Performance Optimizations

• Code splitting: Dynamic imports with proper workflow loaders
• Bulk registration: Uses registerAndTrackTools() instead of individual registration
• Context window optimization: Dynamic mode only loads relevant tools
• Efficient static fallback: Quick detection and bulk static registration

⚠️ Areas for Improvement

1. Security Considerations (Medium Priority)

Issue identified in Cursor review: Missing null check in src/mcp/tools/discovery/discover_tools.ts:61

if (!server.server.getClientCapabilities()?.sampling) {

Risk: Potential runtime error if server.server is null/undefined.

Additional security concerns:

• LLM prompt injection: User task descriptions inserted directly into prompts without sanitization (line 79)
• Global state access: globalThis.mcpServer creates potential injection points (src/index.ts:64)
• Temp file naming: Uses timestamp-based naming in list_devices.ts:39 which could be predictable

Recommendations:

// Add null safety
if (!server?.server?.getClientCapabilities()?.sampling) {

// Sanitize user input
function sanitizeTaskDescription(input: string): string {
  return input.replace(/[<>{}]/g, '').trim().slice(0, 500);
}

2. Race Condition Issue (High Priority)

Bug identified in Cursor review: Race condition in src/utils/command.ts:155 for detached processes:

setTimeout(() => {
  if (childProcess.pid) {
    resolve({ success: true, output: '', process: childProcess });
  }
}, 100);

Risk: Promise can be resolved multiple times by both timeout and event handlers.

3. Code Quality Issues (Low Priority)

• Duplicate validation: UI testing tools (long_press.ts:55-62, key_sequence.ts, touch.ts) contain redundant parameter validation after migrating to createTypedTool with Zod schemas
• Type casting: Multiple as unknown casts in bulk registration (line 158)
• Magic numbers: maxTokens: 200 hardcoded in discover_tools.ts:104

🔧 Technical Implementation Review

Dynamic Mode Architecture

The implementation in src/index.ts:66-78 is well-designed:

const isDynamicModeEnabled = process.env.XCODEBUILDMCP_DYNAMIC_TOOLS === 'true';
if (isDynamicModeEnabled) {
  await registerDiscoveryTools(server);
} else {
  await registerAllToolsStatic(server);
}

Bulk Registration Pattern

Excellent use of fallback strategy in src/core/dynamic-tools.ts:142-204:

try {
  const registeredTools = registerAndTrackTools(server, toolRegistrations);
} catch (error) {
  // Fallback to individual registration
}

LLM Integration

Smart prompt engineering in src/mcp/tools/discovery/discover_tools.ts:77-98 with clear workflow selection guidelines.

🚀 Notable Innovations

1. Dependency Management: Uses forked SDK github:cameroncooke/typescript-sdk#main for bulk registration
2. Testing Philosophy: Complete elimination of Vitest mocking in favor of dependency injection
3. Client Compatibility: Automatic fallback for non-sampling clients
4. Workflow Organization: 15 well-defined workflow groups for intelligent selection

📊 Code Quality Metrics

• Test Coverage: ✅ Comprehensive with proper dependency injection patterns
• ESLint Compliance: ✅ 100% compliant with strict TypeScript rules
• Architecture Compliance: ✅ Perfect adherence to established patterns
• Security: ⚠️ Minor issues identified that should be addressed

🎯 Final Recommendation

APPROVE WITH CONDITIONS - This is a high-quality implementation that:

• ✅ Successfully introduces AI-powered tool discovery
• ✅ Fixes significant technical debt (handler testing violations)
• ✅ Maintains architectural consistency
• ✅ Provides comprehensive test coverage
• ✅ Includes robust error handling

Pre-merge requirements:

1. Fix security null check in discover_tools.ts:61
2. Address race condition in command.ts:155
3. Remove duplicate validation in UI testing tools
4. Add input sanitization for LLM prompts

📋 Post-Merge Suggestions

1. Security hardening: Complete input sanitization implementation
2. Configuration: Make LLM parameters configurable
3. Type safety: Remove remaining as unknown casts when SDK types improve
4. Monitoring: Add metrics for dynamic vs static mode usage

The core implementation is solid and the testing improvements are exemplary. The identified issues are fixable and don't compromise the fundamental architecture.

---

[cameroncooke]

cursor review

[claude]

Claude finished @cameroncooke's task —— View job

---

Code Review: Tool Discovery Mode Implementation

I've conducted a comprehensive review of this PR that introduces AI-powered tool discovery and addresses testing violations. Here's my detailed analysis:

Review Tasks:

• Analyze the tool discovery implementation
• Review testing improvements and dependency injection changes
• Check for security vulnerabilities and bugs
• Evaluate performance considerations
• Assess test coverage improvements
• Provide final recommendations

---

✅ Major Strengths

1. Exemplary Testing Refactor

• Perfect dependency injection implementation: All 7 handler testing violations properly fixed across the codebase
• Consistent patterns: Every test now uses createMockExecutor() and createMockFileSystemExecutor() instead of direct handler calls
• Comprehensive coverage: Three-dimensional testing (input validation, command generation, output processing) consistently applied
• Literal validation: Tests verify exact expected outputs, catching regressions effectively

Example from src/mcp/tools/device-shared/__tests__/list_devices.test.ts:103:

await list_devicesLogic({}, trackingExecutor, mockPathDeps, mockFsDeps);
expect(commandCalls[0].command).toEqual([
  'xcrun', 'devicectl', 'list', 'devices', '--json-output', '/tmp/devicectl-123.json'
]);

2. Clean Architecture Implementation

• Smart fallback mechanism: Dynamic mode with automatic static fallback for non-sampling clients
• Global server management: Well-handled via type-safe global state in src/index.ts:64
• Bulk registration: Efficient tool loading using forked SDK with proper error handling in src/core/dynamic-tools.ts:141-175
• Proper separation of concerns: Logic functions cleanly separated from MCP handlers

3. Robust Error Handling

• Client capability detection: Graceful handling at src/mcp/tools/discovery/discover_tools.ts:61
• JSON parsing resilience: Handles multiple content formats from LLM responses (lines 118-130)
• Workflow validation: Filters invalid workflows from LLM responses (lines 147-157)
• Comprehensive fallback: Individual tool registration fallback when bulk registration fails (lines 175-204)

4. Performance Optimizations

• Code splitting: Dynamic imports with proper workflow loaders
• Bulk registration: Uses registerAndTrackTools() instead of individual registration
• Context window optimization: Dynamic mode only loads relevant tools
• Efficient static fallback: Quick detection and bulk static registration

⚠️ Areas for Improvement

1. Security Considerations (Medium Priority)

Issue identified in Cursor review: Missing null check in src/mcp/tools/discovery/discover_tools.ts:61

if (!server.server.getClientCapabilities()?.sampling) {

Risk: Potential runtime error if server.server is null/undefined.

Additional security concerns:

• LLM prompt injection: User task descriptions inserted directly into prompts without sanitization (line 79)
• Global state access: globalThis.mcpServer creates potential injection points (src/index.ts:64)
• Temp file naming: Uses timestamp-based naming in list_devices.ts:39 which could be predictable

Recommendations:

// Add null safety
if (!server?.server?.getClientCapabilities()?.sampling) {

// Sanitize user input
function sanitizeTaskDescription(input: string): string {
  return input.replace(/[<>{}]/g, '').trim().slice(0, 500);
}

2. Race Condition Issue (High Priority)

Bug identified in Cursor review: Race condition in src/utils/command.ts:155 for detached processes:

setTimeout(() => {
  if (childProcess.pid) {
    resolve({ success: true, output: '', process: childProcess });
  }
}, 100);

Risk: Promise can be resolved multiple times by both timeout and event handlers.

3. Code Quality Issues (Low Priority)

• Duplicate validation: UI testing tools (long_press.ts:55-62, key_sequence.ts, touch.ts) contain redundant parameter validation after migrating to createTypedTool with Zod schemas
• Type casting: Multiple as unknown casts in bulk registration (line 158)
• Magic numbers: maxTokens: 200 hardcoded in discover_tools.ts:104

🔧 Technical Implementation Review

Dynamic Mode Architecture

The implementation in src/index.ts:66-78 is well-designed:

const isDynamicModeEnabled = process.env.XCODEBUILDMCP_DYNAMIC_TOOLS === 'true';
if (isDynamicModeEnabled) {
  await registerDiscoveryTools(server);
} else {
  await registerAllToolsStatic(server);
}

Bulk Registration Pattern

Excellent use of fallback strategy in src/core/dynamic-tools.ts:142-204:

try {
  const registeredTools = registerAndTrackTools(server, toolRegistrations);
} catch (error) {
  // Fallback to individual registration
}

LLM Integration

Smart prompt engineering in src/mcp/tools/discovery/discover_tools.ts:77-98 with clear workflow selection guidelines.

🚀 Notable Innovations

1. Dependency Management: Uses forked SDK github:cameroncooke/typescript-sdk#main for bulk registration
2. Testing Philosophy: Complete elimination of Vitest mocking in favor of dependency injection
3. Client Compatibility: Automatic fallback for non-sampling clients
4. Workflow Organization: 15 well-defined workflow groups for intelligent selection

📊 Code Quality Metrics

• Test Coverage: ✅ Comprehensive with proper dependency injection patterns
• ESLint Compliance: ✅ 100% compliant with strict TypeScript rules
• Architecture Compliance: ✅ Perfect adherence to established patterns
• Security: ⚠️ Minor issues identified that should be addressed

🎯 Final Recommendation

APPROVE WITH CONDITIONS - This is a high-quality implementation that:

• ✅ Successfully introduces AI-powered tool discovery
• ✅ Fixes significant technical debt (handler testing violations)
• ✅ Maintains architectural consistency
• ✅ Provides comprehensive test coverage
• ✅ Includes robust error handling

Pre-merge requirements:

1. Fix security null check in discover_tools.ts:61
2. Address race condition in command.ts:155
3. Remove duplicate validation in UI testing tools
4. Add input sanitization for LLM prompts

📋 Post-Merge Suggestions

1. Security hardening: Complete input sanitization implementation
2. Configuration: Make LLM parameters configurable
3. Type safety: Remove remaining as unknown casts when SDK types improve
4. Monitoring: Add metrics for dynamic vs static mode usage

The core implementation is solid and the testing improvements are exemplary. The identified issues are fixable and don't compromise the fundamental architecture.

---