🎯 refactor: MCP Registry To Handle serverInstructions As String "true" (#9703)

- Updated MCPServersRegistry to correctly process serverInstructions when provided as a string "true", allowing it to fetch instructions from the server.
- Added a new utility function `isEnabled` to determine if serverInstructions should trigger a fetch.
- Introduced comprehensive tests to validate the behavior for different serverInstructions configurations, ensuring both string "true" and boolean true fetch from the server while custom strings remain unchanged.

🎯 This enhancement improves the flexibility and correctness of server instruction handling in the MCPServersRegistry.
This commit is contained in:
Danny Avila 2025-09-18 21:03:21 -04:00 committed by GitHub
parent 26a58fcabc
commit 98af4564e8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 76 additions and 2 deletions

View file

@ -8,7 +8,7 @@ import type * as t from '~/mcp/types';
import { ConnectionsRepository } from '~/mcp/ConnectionsRepository';
import { detectOAuthRequirement } from '~/mcp/oauth';
import { sanitizeUrlForLogging } from '~/mcp/utils';
import { processMCPEnv } from '~/utils';
import { processMCPEnv, isEnabled } from '~/utils';
import { CONSTANTS } from '~/mcp/enum';
/**
@ -158,8 +158,13 @@ export class MCPServersRegistry {
private async fetchServerInstructions(serverName: string): Promise<void> {
const config = this.parsedConfigs[serverName];
if (!config.serverInstructions) return;
if (typeof config.serverInstructions === 'string') return;
// If it's a string that's not "true", it's a custom instruction
if (typeof config.serverInstructions === 'string' && !isEnabled(config.serverInstructions)) {
return;
}
// Fetch from server if true (boolean) or "true" (string)
const conn = await this.connections.get(serverName);
config.serverInstructions = conn.client.getInstructions();
if (!config.serverInstructions) {

View file

@ -288,5 +288,74 @@ describe('MCPServersRegistry - Initialize Function', () => {
// Compare the actual parsedConfigs against the expected fixture
expect(registry.parsedConfigs).toEqual(expectedParsedConfigs);
});
it('should handle serverInstructions as string "true" correctly and fetch from server', async () => {
// Create test config with serverInstructions as string "true"
const testConfig: t.MCPServers = {
test_server_string_true: {
type: 'stdio',
args: [],
command: 'test-command',
serverInstructions: 'true', // Simulating string "true" from YAML parsing
},
test_server_custom_string: {
type: 'stdio',
args: [],
command: 'test-command',
serverInstructions: 'Custom instructions here',
},
test_server_bool_true: {
type: 'stdio',
args: [],
command: 'test-command',
serverInstructions: true,
},
};
const registry = new MCPServersRegistry(testConfig);
// Setup mock connection for servers that should fetch
const mockClient = {
listTools: jest.fn().mockResolvedValue({ tools: [] }),
getInstructions: jest.fn().mockReturnValue('Fetched instructions from server'),
getServerCapabilities: jest.fn().mockReturnValue({ tools: {} }),
};
const mockConnection = {
client: mockClient,
} as unknown as jest.Mocked<MCPConnection>;
mockConnectionsRepo.get.mockResolvedValue(mockConnection);
mockConnectionsRepo.getLoaded.mockResolvedValue(
new Map([
['test_server_string_true', mockConnection],
['test_server_bool_true', mockConnection],
]),
);
mockDetectOAuthRequirement.mockResolvedValue({
requiresOAuth: false,
method: 'no-metadata-found',
metadata: null,
});
await registry.initialize();
// Verify that string "true" was treated as fetch-from-server
expect(registry.parsedConfigs['test_server_string_true'].serverInstructions).toBe(
'Fetched instructions from server',
);
// Verify that custom string was kept as-is
expect(registry.parsedConfigs['test_server_custom_string'].serverInstructions).toBe(
'Custom instructions here',
);
// Verify that boolean true also fetched from server
expect(registry.parsedConfigs['test_server_bool_true'].serverInstructions).toBe(
'Fetched instructions from server',
);
// Verify getInstructions was called for both "true" cases
expect(mockClient.getInstructions).toHaveBeenCalledTimes(2);
});
});
});