refactor: move auth to within clients and create common client init #177
Conversation
🎯 Coverage Target Met!📈 Coverage Metrics
📊 Test Statistics
🔍 Files Needing Coverage
📝 Report generated on Node.js v24.10.0 • View workflow • Coverage by Vitest + v8 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MCP client architecture by moving authentication handling into individual client classes and introducing a centralized client registry system. The changes eliminate hardcoded client initialization in the main entry point and make the system more extensible for future client additions.
- Implements a client registry pattern for dynamic client discovery and initialization
- Adds standardized authentication configuration methods to all client classes
- Centralizes client setup logic into a factory function with consistent error handling
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Replaces manual client initialization with registry-based setup |
| src/common/types.ts | Adds authentication configuration interfaces |
| src/common/client-registry.ts | Implements central client registry system |
| src/common/client-factory.ts | Creates factory for environment-based client initialization |
| src/common/register-clients.ts | Registers all available clients with the registry |
| src/zephyr/client.ts | Adds authentication config and fromEnv method |
| src/reflect/client.ts | Adds authentication config and fromEnv method |
| src/qmetry/client.ts | Adds authentication config and fromEnv method |
| src/pactflow/client.ts | Adds authentication config and fromEnv method |
| src/bugsnag/client.ts | Adds authentication config and fromEnv method |
| src/api-hub/client.ts | Adds authentication config and fromEnv method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!token) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
This check is redundant since the same validation is performed in the loop above (lines 73-80). The loop already ensures all required auth values are present, making this additional check unnecessary.
| if (!token) { | |
| return null; | |
| } |
| "initialize" in client && | ||
| typeof client.initialize === "function" | ||
| ) { | ||
| await (client as any).initialize(); |
There was a problem hiding this comment.
Using 'any' type assertion bypasses TypeScript's type safety. Consider casting to a more specific interface that defines the initialize method, or create a proper interface for clients that support async initialization.
| if ("getAuthConfig" in entry.clientClass) { | ||
| const config = (entry.clientClass as any).getAuthConfig(); |
There was a problem hiding this comment.
Using 'any' type assertion bypasses TypeScript's type safety. Consider extending the ClientClass interface to include an optional getAuthConfig method, or create a separate interface for clients that support auth configuration.
| // Perform async initialization if needed | ||
| if ( | ||
| entry.asyncInit && | ||
| "initialize" in client && |
There was a problem hiding this comment.
Shall we add initialize to the interface as an optional method and just call it asynchronously (if non-null) anyway?
Similarly, passing a reference to the server in each client constructor seems reasonable, without the complication of making it only on some clients.
| for (const entry of clientRegistry.getAll()) { | ||
| try { | ||
| // Create client from environment variables | ||
| const client = entry.needsMcpServer |
There was a problem hiding this comment.
Would it better to push the env-reading outside the client so that it can be agnostic to where the config values come from – then the entry point main or a webserver init can loop through the expected config (from getAuthConfig) and get it from env or headers in a map to init clients with.
Perhaps the new ClientRegistry is where that should live and it can be subclassed to fetch from the appropriate store (e.g. StdioCientRegistry).
What if we zod-typed the config requirements and the client expects to have an object of that type passed into its constructor?
|
Needs a bit more thinking about how auth would work for http/sse |
|
closing in favour of the #196 pr which adds hosted server support and builds on top of this. Most the comments have been addressed via a refactor |
2 participants
Goal
Moves auth requirements to within clients and make client initialization into a separate module.
Design
Moving auth to within the client makes it easier to identify what configuration is required for each client. Also this is a precursor to adding support for http/sse.
Changeset
Testing