- 
                Notifications
    You must be signed in to change notification settings 
- Fork 14
refactor: bugsnag tools modularization #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
          
     Open
      
      
            sazap10
  wants to merge
  3
  commits into
  next
  
    
      
        
          
  
    
      Choose a base branch
      
     
    
      
        
      
      
        
          
          
        
        
          
            
              
              
              
  
           
        
        
          
            
              
              
           
        
       
     
  
        
          
            
          
            
          
        
       
    
      
from
bugsnag_tools_refactor_specs
  
      
      
   
  
    
  
  
  
 
  
      
    base: next
Could not load branches
            
              
  
    Branch not found: {{ refName }}
  
            
                
      Loading
              
            Could not load tags
            
            
              Nothing to show
            
              
  
            
                
      Loading
              
            Are you sure you want to change the base?
            Some commits from the old base branch may be removed from the timeline,
            and old review comments may become outdated.
          
          
  
     Open
                    Changes from all commits
      Commits
    
    
  File filter
Filter by extension
Conversations
          Failed to load comments.   
        
        
          
      Loading
        
  Jump to
        
          Jump to file
        
      
      
          Failed to load files.   
        
        
          
      Loading
        
  Diff view
Diff view
There are no files selected for viewing
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,260 @@ | ||
| # Design Document | ||
|  | ||
| ## Overview | ||
|  | ||
| This design outlines the refactoring of the Bugsnag client's monolithic tool registration system into a modular, maintainable architecture. The current implementation has all 10+ tools defined in a single 1200+ line `registerTools` method, making it difficult to maintain, test, and extend. | ||
|  | ||
| The refactored architecture will separate each tool into its own module while maintaining backward compatibility and introducing consistent patterns for tool development. | ||
|  | ||
| ## Architecture | ||
|  | ||
| ### Current Architecture Issues | ||
|  | ||
| - **Monolithic Design**: All tools are defined in a single large method | ||
| - **Tight Coupling**: Tools are tightly coupled to the BugsnagClient class | ||
| - **Testing Challenges**: Difficult to test individual tools in isolation | ||
| - **Maintenance Overhead**: Changes to one tool risk affecting others | ||
| - **Code Duplication**: Similar patterns repeated across tools without abstraction | ||
|  | ||
| ### Proposed Architecture | ||
|  | ||
| The new architecture introduces a layered approach: | ||
|  | ||
| ``` | ||
| BugsnagClient | ||
| ├── ToolRegistry (manages tool discovery and registration) | ||
| ├── SharedServices (provides common functionality to tools) | ||
| └── Tools/ | ||
| ├── ListProjectsTool | ||
| ├── GetErrorTool | ||
| ├── GetEventDetailsTool | ||
| ├── ListProjectErrorsTool | ||
| ├── ListProjectEventFiltersTool | ||
| ├── UpdateErrorTool | ||
| ├── ListBuildsTool | ||
| ├── GetBuildTool | ||
| ├── ListReleasesTool | ||
| ├── GetReleaseTool | ||
| └── ListBuildsInReleaseTool | ||
| ``` | ||
|  | ||
| ## Components and Interfaces | ||
|  | ||
| ### 1. Base Tool Interface | ||
|  | ||
| All tools will implement a common interface to ensure consistency: | ||
|  | ||
| ```typescript | ||
| interface BugsnagTool { | ||
| readonly name: string; | ||
| readonly definition: ToolDefinition; | ||
| execute(args: any, context: ToolExecutionContext): Promise<ToolResult>; | ||
| } | ||
|  | ||
| interface ToolExecutionContext { | ||
| services: SharedServices; | ||
| getInput: GetInputFunction; | ||
| } | ||
|  | ||
| interface ToolDefinition { | ||
| title: string; | ||
| summary: string; | ||
| purpose: string; | ||
| useCases: string[]; | ||
| parameters: ParameterDefinition[]; | ||
| examples: ToolExample[]; | ||
| hints: string[]; | ||
| outputFormat?: string; | ||
| } | ||
| ``` | ||
|  | ||
| ### 2. Shared Services Interface | ||
|  | ||
| Common functionality will be provided through a shared services interface: | ||
|  | ||
| ```typescript | ||
| interface SharedServices { | ||
| // Project management | ||
| getProjects(): Promise<Project[]>; | ||
| getProject(projectId: string): Promise<Project | null>; | ||
| getCurrentProject(): Promise<Project | null>; | ||
| getInputProject(projectId?: string): Promise<Project>; | ||
|  | ||
| // API clients | ||
| getCurrentUserApi(): CurrentUserAPI; | ||
| getErrorsApi(): ErrorAPI; | ||
| getProjectApi(): ProjectAPI; | ||
|  | ||
| // Caching | ||
| getCache(): NodeCache; | ||
|  | ||
| // URL generation | ||
| getDashboardUrl(project: Project): Promise<string>; | ||
| getErrorUrl(project: Project, errorId: string, queryString?: string): Promise<string>; | ||
|  | ||
| // Configuration | ||
| getProjectApiKey(): string | undefined; | ||
| hasProjectApiKey(): boolean; | ||
| } | ||
| ``` | ||
|  | ||
| ### 3. Tool Registry | ||
|  | ||
| The tool registry will handle automatic discovery and registration of tools: | ||
|  | ||
| ```typescript | ||
| class ToolRegistry { | ||
| private tools: Map<string, BugsnagTool> = new Map(); | ||
|  | ||
| registerTool(tool: BugsnagTool): void; | ||
| discoverTools(): BugsnagTool[]; | ||
| registerAllTools(register: RegisterToolsFunction, context: ToolExecutionContext): void; | ||
| } | ||
| ``` | ||
|  | ||
| ### 4. Individual Tool Modules | ||
|  | ||
| Each tool will be implemented as a separate class: | ||
|  | ||
| ```typescript | ||
| // Example: GetErrorTool | ||
| export class GetErrorTool implements BugsnagTool { | ||
| readonly name = "get_error"; | ||
| readonly definition: ToolDefinition = { | ||
| title: "Get Error", | ||
| summary: "Get full details on an error...", | ||
| // ... rest of definition | ||
| }; | ||
|  | ||
| async execute(args: any, context: ToolExecutionContext): Promise<ToolResult> { | ||
| const { services } = context; | ||
| const project = await services.getInputProject(args.projectId); | ||
| // ... tool implementation | ||
| } | ||
| } | ||
| ``` | ||
|  | ||
| ## Data Models | ||
|  | ||
| ### Tool Categories | ||
|  | ||
| Tools will be organized into logical categories: | ||
|  | ||
| 1. **Project Tools**: `ListProjectsTool` | ||
| 2. **Error Tools**: `GetErrorTool`, `ListProjectErrorsTool`, `UpdateErrorTool` | ||
| 3. **Event Tools**: `GetEventDetailsTool`, `ListProjectEventFiltersTool` | ||
| 4. **Build Tools**: `ListBuildsTool`, `GetBuildTool`, `ListBuildsInReleaseTool` | ||
| 5. **Release Tools**: `ListReleasesTool`, `GetReleaseTool` | ||
|  | ||
| ### Parameter Validation | ||
|  | ||
| Each tool will define its parameters using Zod schemas for consistent validation: | ||
|  | ||
| ```typescript | ||
| interface ParameterDefinition { | ||
| name: string; | ||
| type: z.ZodType<any>; | ||
| required: boolean; | ||
| description: string; | ||
| examples: string[]; | ||
| constraints?: string[]; | ||
| } | ||
| ``` | ||
|  | ||
| ## Error Handling | ||
|  | ||
| ### Consistent Error Patterns | ||
|  | ||
| All tools will follow consistent error handling patterns: | ||
|  | ||
| 1. **Parameter Validation**: Use Zod schemas for input validation | ||
| 2. **Business Logic Errors**: Throw descriptive errors with context | ||
| 3. **API Errors**: Wrap and enhance API errors with additional context | ||
| 4. **Error Propagation**: Allow errors to bubble up with proper error messages | ||
|  | ||
| ### Error Types | ||
|  | ||
| ```typescript | ||
| class BugsnagToolError extends Error { | ||
| constructor( | ||
| message: string, | ||
| public readonly toolName: string, | ||
| public readonly cause?: Error | ||
| ) { | ||
| super(message); | ||
| } | ||
| } | ||
| ``` | ||
|  | ||
| ## Testing Strategy | ||
|  | ||
| ### Unit Testing Approach | ||
|  | ||
| 1. **Tool Isolation**: Each tool can be tested independently | ||
| 2. **Service Mocking**: SharedServices interface can be easily mocked | ||
| 3. **Parameter Testing**: Validate parameter schemas and edge cases | ||
| 4. **Error Scenarios**: Test error handling and edge cases | ||
|  | ||
| ### Test Structure | ||
|  | ||
| ```typescript | ||
| describe('GetErrorTool', () => { | ||
| let tool: GetErrorTool; | ||
| let mockServices: jest.Mocked<SharedServices>; | ||
|  | ||
| beforeEach(() => { | ||
| mockServices = createMockServices(); | ||
| tool = new GetErrorTool(); | ||
| }); | ||
|  | ||
| it('should retrieve error details successfully', async () => { | ||
| // Test implementation | ||
| }); | ||
|  | ||
| it('should handle missing error gracefully', async () => { | ||
| // Test error handling | ||
| }); | ||
| }); | ||
| ``` | ||
|  | ||
| ### Integration Testing | ||
|  | ||
| 1. **Tool Registry**: Test tool discovery and registration | ||
| 2. **End-to-End**: Test complete tool execution flow | ||
| 3. **Backward Compatibility**: Ensure existing functionality works unchanged | ||
|  | ||
| ## Migration Strategy | ||
|  | ||
| ### Phase 1: Infrastructure Setup | ||
| - Create base interfaces and shared services | ||
| - Implement tool registry | ||
| - Set up testing framework | ||
|  | ||
| ### Phase 2: Tool Migration | ||
| - Migrate tools one by one, starting with simpler ones | ||
| - Maintain backward compatibility during migration | ||
| - Add comprehensive tests for each migrated tool | ||
|  | ||
| ### Phase 3: Cleanup and Optimization | ||
| - Remove old monolithic implementation | ||
| - Optimize shared services | ||
| - Add performance monitoring | ||
|  | ||
| ### Backward Compatibility | ||
|  | ||
| The refactoring will maintain 100% backward compatibility: | ||
| - All existing tool names and parameters remain unchanged | ||
| - Response formats stay identical | ||
| - Error messages and behavior preserved | ||
| - No breaking changes to the public API | ||
|  | ||
| ## Benefits of This Design | ||
|  | ||
| 1. **Modularity**: Each tool is self-contained and focused | ||
| 2. **Testability**: Tools can be tested in isolation with mocked dependencies | ||
| 3. **Maintainability**: Changes to one tool don't affect others | ||
| 4. **Extensibility**: New tools can be added easily following established patterns | ||
| 5. **Consistency**: All tools follow the same interface and patterns | ||
| 6. **Reusability**: Shared services eliminate code duplication | ||
| 7. **Type Safety**: Strong typing throughout the system | ||
| 8. **Documentation**: Each tool is self-documenting with clear definitions | 
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| # Requirements Document | ||
|  | ||
| ## Introduction | ||
|  | ||
| The current Bugsnag client implementation has all tools defined in a single large `registerTools` method within the `BugsnagClient` class. This monolithic approach makes the code difficult to maintain, test, and extend. The refactoring will break down the tools into modular, focused components that are easier to understand, test, and maintain while preserving all existing functionality. | ||
|  | ||
| ## Requirements | ||
|  | ||
| ### Requirement 1 | ||
|  | ||
| **User Story:** As a developer maintaining the Bugsnag MCP server, I want the tools to be organized into separate, focused modules, so that I can easily understand, modify, and test individual tool implementations. | ||
|  | ||
| #### Acceptance Criteria | ||
|  | ||
| 1. WHEN the refactoring is complete THEN each tool SHALL be defined in its own separate module | ||
| 2. WHEN a tool module is created THEN it SHALL contain only the logic specific to that tool | ||
| 3. WHEN examining a tool module THEN it SHALL be self-contained with clear dependencies | ||
| 4. WHEN adding a new tool THEN it SHALL require minimal changes to existing code | ||
|  | ||
| ### Requirement 2 | ||
|  | ||
| **User Story:** As a developer working on the codebase, I want consistent patterns across all tool implementations, so that I can quickly understand and work with any tool module. | ||
|  | ||
| #### Acceptance Criteria | ||
|  | ||
| 1. WHEN implementing tool modules THEN they SHALL follow a consistent interface pattern | ||
| 2. WHEN a tool is registered THEN it SHALL use the same registration mechanism as other tools | ||
| 3. WHEN tool parameters are defined THEN they SHALL use consistent validation patterns | ||
| 4. WHEN error handling is implemented THEN it SHALL follow the same patterns across all tools | ||
|  | ||
| ### Requirement 3 | ||
|  | ||
| **User Story:** As a developer testing the Bugsnag client, I want each tool to be independently testable, so that I can write focused unit tests and isolate issues quickly. | ||
|  | ||
| #### Acceptance Criteria | ||
|  | ||
| 1. WHEN a tool module is created THEN it SHALL be testable in isolation | ||
| 2. WHEN testing a tool THEN it SHALL not require the entire BugsnagClient to be instantiated | ||
| 3. WHEN mocking dependencies THEN it SHALL be straightforward to mock only the required dependencies | ||
| 4. WHEN running tests THEN each tool's tests SHALL be independent of other tools | ||
|  | ||
| ### Requirement 4 | ||
|  | ||
| **User Story:** As a user of the MCP server, I want all existing functionality to work exactly as before, so that the refactoring doesn't break my current workflows. | ||
|  | ||
| #### Acceptance Criteria | ||
|  | ||
| 1. WHEN the refactoring is complete THEN all existing tools SHALL function identically to before | ||
| 2. WHEN calling any tool THEN it SHALL return the same response format as the original implementation | ||
| 3. WHEN using tool parameters THEN they SHALL accept the same inputs as before | ||
| 4. WHEN errors occur THEN they SHALL be handled and reported in the same way as before | ||
|  | ||
| ### Requirement 5 | ||
|  | ||
| **User Story:** As a developer extending the Bugsnag client, I want a clear and simple way to add new tools, so that I can implement new features without modifying core client logic. | ||
|  | ||
| #### Acceptance Criteria | ||
|  | ||
| 1. WHEN adding a new tool THEN it SHALL require creating only a new tool module | ||
| 2. WHEN registering a new tool THEN it SHALL be automatically discovered and registered | ||
| 3. WHEN a tool module follows the standard pattern THEN it SHALL integrate seamlessly with the client | ||
| 4. WHEN the client initializes THEN it SHALL automatically load all available tool modules | ||
|  | ||
| ### Requirement 6 | ||
|  | ||
| **User Story:** As a developer maintaining the codebase, I want clear separation of concerns between tool logic and client infrastructure, so that I can modify tools without affecting core client functionality. | ||
|  | ||
| #### Acceptance Criteria | ||
|  | ||
| 1. WHEN implementing tool logic THEN it SHALL be separate from client initialization and configuration | ||
| 2. WHEN the client provides shared services THEN tools SHALL access them through well-defined interfaces | ||
| 3. WHEN tool implementations change THEN they SHALL not require changes to the core client class | ||
| 4. WHEN client infrastructure changes THEN it SHALL not require changes to individual tool modules | 
      
      Oops, something went wrong.
        
    
  
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we want to keep these or not