feat: implement lazy loading for MCP server tools #221
Conversation
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
…ing capabilities - Added ToolRegistry class for managing tool metadata with methods for filtering, pagination, and categorization. - Introduced ToolMetadata interface to define the structure of tool information. - Created unit tests for ToolRegistry covering basic operations, filtering, pagination, server and tag operations, and edge cases. - Integrated LazyLoadingOrchestrator into InstructionAggregator and request handlers to support lazy loading of tools. - Updated AgentConfig to include lazy loading configuration options. - Enhanced server setup to initialize lazy loading orchestrator based on agent configuration. - Modified connection and server managers to handle lazy loading orchestrator.
# Conflicts: # src/transport/http/routes/streamableHttpRoutes.test.ts # src/transport/http/routes/streamableHttpRoutes.ts
…d schema fetching - Rename meta-tools: mcp_list_available_tools → tool_list, mcp_describe_tool → tool_schema, mcp_call_tool → tool_invoke - Add Zod schemas for input/output validation of all meta-tools - Implement on-demand schema fetching in tool_schema when not cached - Simplify lazy loading config by removing mode field (always metatool when enabled) - Add lazyTools test module with 22 tests covering all factory functions - Add 9 tests for getHealthStatus() and logStatistics() in lazyLoadingOrchestrator - Improve test coverage: lazyLoadingOrchestrator 79%→93%, lazyTools 77%→100% All 3055 unit tests pass.
…action in InstructionAggregator
… and template servers
- Implemented integration tests for the PresetManager and TagQueryEvaluator to ensure correct loading and evaluation of presets and tag queries. - Created a new test file `lazy-loading-preset-integration.test.ts` for organizing these tests. - Updated `CommandTestEnvironment` to create temporary directories under `./build/`. - Enhanced `McpTestClient` to support a new transport type: `streamable-http`. - Introduced `SimpleMcpClient` for testing with raw JSON-RPC 2.0 protocol, bypassing SDK validation issues. - Added a fast mock MCP server (`mock-mcp-server-fast.js`) for improved testing performance. - Created a minimal mock MCP server (`mock-mcp-server.js`) for handling basic requests in tests.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| const totalRequests = stats.hits + stats.misses; | ||
|
|
||
| if (totalRequests > 100) { | ||
| const hitRate = totalRequests > 0 ? stats.hitRate : 0; |
This commit addresses all critical bugs and quality issues identified in the
lazy loading feature, adding comprehensive test coverage and production-ready
error handling.
## Critical Bug Fixes
- Remove duplicate ListResourceTemplatesRequestSchema handler registration
- Add missing sessionId parameter to executeTool for multi-tenant filtering
- Fix server name mismatch in lazy loading (use connection.name vs Map keys)
- Fix empty Set filtering logic in setAllowedServers (handle new Set([]))
- Replace unsafe type casts with Zod runtime validation
## Error Handling Improvements
- Replace debugIf() with logger.warn() in capability aggregation safe methods
- Add template server creation failure tracking with getFailedTemplates()
- Add LLM-directed notice for custom template fallback failures
- Update schemaCache.preload() to return {loaded, failed[]} for monitoring
## Type Safety & Code Quality
- Consolidate duplicate Zod schemas into shared metaToolSchemas.ts
- Replace all unsafe 'args as Type' casts with safeParse() validation
- Fix misleading LRU comment to accurately reflect FIFO eviction
- Remove unused registryIds WeakMap field
## Test Coverage (+36 comprehensive tests)
- Add 12 tests for filterByServers functionality (toolRegistry.test.ts)
- Add 7 tests for SchemaLoader integration (metaToolProvider.test.ts)
- Add 10 tests for session-based filtering (lazyLoadingOrchestrator.test.ts)
- Add 7 tests for setAllowedServers filtering (metaToolProvider.test.ts)
## Quality Verification
- TypeScript compilation: 0 errors ✓
- ESLint: 0 warnings ✓
- Test suite: 3,100/3,100 tests passing (100%) ✓
- Build: successful ✓
Files changed: 13 (+1 new)
Lines changed: +1,150 / -200
Test coverage: All new features fully tested
Breaking changes: None (fully backward compatible)
Add idempotentHint and openWorldHint annotations to all internal MCP tools following the official MCP specification. These hints help AI clients better understand tool behavior characteristics for improved decision-making. Changes: - lazyTools.ts: Add title, readOnlyHint, openWorldHint annotations - managementTools.ts: Add idempotentHint and openWorldHint to all 6 tools - installationTools.ts: Add idempotentHint and openWorldHint to all 3 tools All 17 internal tools now have complete annotations with proper hints: - title: Human-readable tool names - readOnlyHint/destructiveHint: Operation safety characteristics - idempotentHint: Repeat execution behavior - openWorldHint: External system interaction indicator
Fix tool_invoke and tool_schema failing with template MCP servers due to server name mismatch between clean names (in registry) and hash-suffixed keys (in connections). **Problem:** - Template servers stored with hash-suffixed keys: "template-server:abc123" - ToolRegistry uses clean names: "template-server" - tool_invoke/tool_schema failed on connection lookup with clean names **Solution:** - Add resolveConnectionKey() method to MetaToolProvider - Resolves clean server names to actual connection keys - Searches connections by name property and key prefix matching - Works with both static servers and template servers **Changes:** - src/core/capabilities/metaToolProvider.ts: Add connection key resolution - src/core/capabilities/metaToolProvider-template-servers.test.ts: Unit tests - test/e2e/lazy-tools-template-servers.test.ts: E2E test suite **Test Results:** - All 6 unit tests pass - All 10 E2E tests pass - tool_invoke now works with template servers ✅ - tool_schema now works with template servers ✅ - tool_list continues to work correctly ✅
… server connections - Added ServerRegistry class to manage server adapters and provide unified access to server types. - Introduced TemplateServerAdapter to handle template-based MCP servers with dynamic connections. - Created tests for TemplateServerAdapter to ensure correct functionality. - Developed ConnectionResolver to encapsulate connection resolution logic for static and template servers. - Updated ServerManager to integrate ServerRegistry and populate it with external and template servers. - Added utility functions and types for better server management and connection handling.
|
Great, can we get this merged? |
Contributor
Author
Not yet, I'm still testing it by myself, and plan to release a beta version next week. However, if possible, you can also use this branch to test together. If you have any problems, you can provide feedback here. |
📝 WalkthroughWalkthroughIntroduces comprehensive lazy loading infrastructure enabling on-demand tool discovery and schema loading via meta-tools, schema caching with TTL and request coalescing, server filtering by presets and tags, session-scoped server filtering, and new CLI options for the serve command alongside integration with capabilities system and HTTP transport. Changes
Sequence DiagramssequenceDiagram
actor Client
participant LazyLoadingOrchestrator as LazyLoading<br/>Orchestrator
participant MetaToolProvider
participant ToolRegistry
participant SchemaCache
participant UpstreamServer as Upstream<br/>Server
Client->>LazyLoadingOrchestrator: callMetaTool("tool_list", {...})
activate LazyLoadingOrchestrator
LazyLoadingOrchestrator->>MetaToolProvider: callMetaTool("tool_list", {...})
activate MetaToolProvider
MetaToolProvider->>ToolRegistry: listTools(filters)
activate ToolRegistry
ToolRegistry-->>MetaToolProvider: {tools, totalCount, ...}
deactivate ToolRegistry
MetaToolProvider-->>LazyLoadingOrchestrator: {tools, servers, ...}
deactivate MetaToolProvider
LazyLoadingOrchestrator-->>Client: {tools, servers, hasMore, ...}
deactivate LazyLoadingOrchestrator
Note over Client,UpstreamServer: Tool Schema Loading Flow
Client->>LazyLoadingOrchestrator: callMetaTool("tool_schema", {server, toolName})
activate LazyLoadingOrchestrator
LazyLoadingOrchestrator->>SchemaCache: getOrLoad(server, toolName)
activate SchemaCache
alt Cache Hit
SchemaCache-->>LazyLoadingOrchestrator: {tool}
else Cache Miss
SchemaCache->>UpstreamServer: fetch tool schema
activate UpstreamServer
UpstreamServer-->>SchemaCache: {tool, schema}
deactivate UpstreamServer
SchemaCache->>SchemaCache: cache(server, toolName, tool)
SchemaCache-->>LazyLoadingOrchestrator: {tool}
end
deactivate SchemaCache
LazyLoadingOrchestrator-->>Client: {schema, fromCache}
deactivate LazyLoadingOrchestrator
Note over Client,UpstreamServer: Tool Invocation Flow
Client->>LazyLoadingOrchestrator: callMetaTool("tool_invoke", {server, toolName, args})
activate LazyLoadingOrchestrator
LazyLoadingOrchestrator->>MetaToolProvider: callMetaTool("tool_invoke", {...})
activate MetaToolProvider
MetaToolProvider->>UpstreamServer: callTool(toolName, args)
activate UpstreamServer
UpstreamServer-->>MetaToolProvider: {result}
deactivate UpstreamServer
MetaToolProvider-->>LazyLoadingOrchestrator: {result}
deactivate MetaToolProvider
LazyLoadingOrchestrator-->>Client: {result}
deactivate LazyLoadingOrchestrator
sequenceDiagram
actor User
participant ConnectionManager
participant ServerRegistry
participant ConnectionResolver
participant LazyLoadingOrchestrator as LazyLoading<br/>Orchestrator
participant ExternalServerAdapter
participant TemplateServerAdapter
User->>ConnectionManager: setLazyLoadingOrchestrator(orchestrator)
activate ConnectionManager
ConnectionManager->>LazyLoadingOrchestrator: register
deactivate ConnectionManager
User->>ServerRegistry: registerExternal(name, config)
activate ServerRegistry
ServerRegistry->>ExternalServerAdapter: new ExternalServerAdapter(...)
activate ExternalServerAdapter
ExternalServerAdapter-->>ServerRegistry: adapter
deactivate ExternalServerAdapter
deactivate ServerRegistry
User->>ServerRegistry: registerTemplate(name, config)
activate ServerRegistry
ServerRegistry->>TemplateServerAdapter: new TemplateServerAdapter(...)
activate TemplateServerAdapter
TemplateServerAdapter-->>ServerRegistry: adapter
deactivate TemplateServerAdapter
deactivate ServerRegistry
User->>ConnectionResolver: resolve(serverName, sessionId)
activate ConnectionResolver
ConnectionResolver->>TemplateServerAdapter: resolveConnection({sessionId})
activate TemplateServerAdapter
TemplateServerAdapter-->>ConnectionResolver: connection
deactivate TemplateServerAdapter
ConnectionResolver-->>User: OutboundConnection
deactivate ConnectionResolver
User->>LazyLoadingOrchestrator: getCapabilitiesForFilteredServers(serverNames, sessionId)
activate LazyLoadingOrchestrator
LazyLoadingOrchestrator->>ConnectionResolver: filterForSession(sessionId)
activate ConnectionResolver
ConnectionResolver-->>LazyLoadingOrchestrator: filtered connections
deactivate ConnectionResolver
LazyLoadingOrchestrator-->>User: AggregatedCapabilities
deactivate LazyLoadingOrchestrator
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/flags/flagManager.ts (1)
105-167: Include lazyTools in flag summary for parity.With the new
lazyToolscategory,getFlagSummary()still only reportsinternalTools, so summary output won’t reflect lazy-loading state. Consider adding a lazyTools entry (with a safe default).🛠️ Proposed fix (in getFlagSummary)
return { internalTools: configManager.areInternalToolsEnabled(), + lazyTools: configManager.get('lazyLoading')?.enabled ?? false, };src/core/server/connectionManager.ts (1)
234-272: Keep lazy-loading session filters aligned with the effective sessionId.
mergedContext.sessionIdcan override the transportsessionId, butinitializeSessionFilteruses the transport id. If they diverge, filters are stored under the wrong session and tool discovery won’t reflect tags/presets. Use a single effective session id (or validate mismatch) when initializing the filter.🧩 Proposed fix
- const mergedContext = { - ...(context || {}), - ...(opts.context || {}), - // Use opts.context.sessionId if provided, otherwise fall back to transport sessionId - sessionId: opts.context?.sessionId || context?.sessionId || sessionId, - }; + const effectiveSessionId = opts.context?.sessionId || context?.sessionId || sessionId; + const mergedContext = { + ...(context || {}), + ...(opts.context || {}), + sessionId: effectiveSessionId, + }; @@ - if (this.lazyLoadingOrchestrator?.isEnabled()) { - await this.initializeSessionFilter(sessionId, serverInfo); - } + if (this.lazyLoadingOrchestrator?.isEnabled()) { + await this.initializeSessionFilter(effectiveSessionId, serverInfo); + }
🤖 Fix all issues with AI agents
In `@src/core/capabilities/capabilityAggregator.ts`:
- Around line 195-206: The code advances resultIndex only when resources are
fulfilled, so if a resources query was requested but rejected the next item
(prompts) is read from the wrong index and dropped; in capabilityAggregator.ts
locate the block using serverCapabilities.resources, results, resultIndex,
resourceResult, resourcesResult and allResources and change the logic to always
increment resultIndex when resources were requested (i.e., move or add
resultIndex++ to run regardless of resourceResult.status) while still only
pushing to allResources when resourcesResult.resources exists so prompts use the
correct subsequent index.
In `@src/core/capabilities/internalCapabilitiesProvider.ts`:
- Around line 250-253: When flagManager.isToolEnabled('lazyTools') is true,
guard exposing the lazy meta-tools until the orchestrator is wired by checking
whether setLazyLoadingOrchestrator has been called or the orchestrator instance
is present; if the orchestrator is absent, skip pushing createToolListTool(),
createToolSchemaTool(), and createToolInvokeTool() (or log a warning and skip)
so clients don't discover tools that always fail with "Lazy loading not
available." Ensure the check references the orchestrator state used by
setLazyLoadingOrchestrator and gate the tools push accordingly.
In `@src/core/capabilities/lazyLoadingOrchestrator.test.ts`:
- Around line 1048-1053: The test calls
orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId)
without awaiting it, which can leave an unhandled promise and cause flakes;
update the test to await the async call (add await before
getCapabilitiesForFilteredServers) so the filter is fully set before calling
orchestrator.getSessionAllowedServers(sessionId) and the subsequent expect
assertion; reference the existing variables orchestrat or, allowedServers, and
sessionId when making the change.
In `@src/core/capabilities/lazyLoadingOrchestrator.ts`:
- Around line 238-252: loadSchemaFromServer currently calls
ConnectionResolver.findByServerName without session context, causing incorrect
template-instance resolution across sessions; modify loadSchemaFromServer to
accept a sessionId parameter and pass it to a session-aware lookup, then update
ConnectionResolver.findByServerName (or add findByServerNameAndSession) to use
the sessionId and sessionAllowedServers logic when resolving connections.
Specifically, change the signature of loadSchemaFromServer(server: string,
toolName: string) to include sessionId, thread that sessionId through callers
like getCapabilitiesForFilteredServers and callMetaTool, and alter/find the
resolver method so it filters connections by the provided sessionId before
returning result.connection.client so the correct template instance is selected.
- Around line 106-111: The event handler attached to asyncOrchestrator for
'server-capabilities-updated' is declared async so a rejection from
this.refreshCapabilities() can become an unhandledRejection; change the listener
to a non-async callback and invoke this.refreshCapabilities().catch(...) to
handle errors (e.g. log via debugIf or a logger) and keep the debugIf call
intact; update the listener registration on asyncOrchestrator to remove async
from the function and add a .catch handler on refreshCapabilities to swallow/log
errors.
- Around line 486-500: The callMetaTool implementation on
LazyLoadingOrchestrator mutates the shared metaToolProvider state via
metaToolProvider.setAllowedServers(sessionFilter) causing race conditions across
concurrent requests; instead, change MetaToolProvider.callMetaTool to accept an
allowedServers (or sessionFilter) parameter and use that per-call (remove
reliance on metaToolProvider.setAllowedServers), then update
LazyLoadingOrchestrator.callMetaTool to retrieve allowedServers from
sessionAllowedServers and pass it into metaToolProvider.callMetaTool(name, args,
allowedServers); update all other callers of MetaToolProvider.callMetaTool
accordingly and remove or deprecate setAllowedServers to ensure per-call
isolation.
In `@src/core/capabilities/lazyLoadingPerformance.test.ts`:
- Around line 20-39: The test's module-scoped array mockTools is being appended
in beforeEach and grows across tests; reset or reinitialize mockTools at the
start of each test to avoid accumulating entries. Modify the beforeEach that
currently pushes 100 items into mockTools so it either sets mockTools = [] or
calls mockTools.length = 0 before the for loop (or declare mockTools inside the
beforeEach), ensuring the loop always creates exactly 100 tools for tests
referencing mockTools.
In `@src/core/capabilities/schemaCache.ts`:
- Around line 253-268: getCachedTools currently uses cacheKey.split(':') which
breaks when tool names contain colons; update getCachedTools to locate the first
colon via cacheKey.indexOf(':') and extract server = cacheKey.substring(0, idx)
and toolName = cacheKey.substring(idx+1), handling the case where idx === -1
(skip or treat whole key as server) to avoid crashes, and add unit tests for
SchemaCache (or schemaCache) that insert entries whose tool names include
special chars/colons to assert getCachedTools returns the correct server and
full toolName.
In `@src/core/capabilities/schemas/metaToolSchemas.ts`:
- Around line 12-17: ToolListInputSchema is missing the optional cursor field
required for pagination (ListAvailableToolsArgs includes cursor?: string);
update the ToolListInputSchema object to add cursor: z.string().optional() so
the schema matches the interface and supports fetching subsequent pages, and
ensure any validation/consumers of ToolListInputSchema continue to accept the
new optional cursor property.
- Around line 24-28: The ToolInvokeInputSchema uses the older
z.object({}).loose() pattern; update it to use Zod v4 idiomatic constructor by
replacing the args definition with z.looseObject({}) so ToolInvokeInputSchema
remains compatible and idiomatic in v4—locate the ToolInvokeInputSchema
declaration and change args: z.object({}).loose() to args: z.looseObject({}).
In `@src/core/capabilities/toolRegistry.ts`:
- Around line 139-156: The listTools method's glob-to-regex conversion uses
unescaped client input (options.pattern) which can produce invalid regexes or
ReDoS; escape regex metacharacters in options.pattern before applying glob
replacements for '*' and '?' (operate on the escaped string), enforce a
reasonable max length for options.pattern to mitigate ReDoS (e.g., reject or
ignore patterns exceeding the limit), and wrap the RegExp construction/test in a
try-catch so an invalid pattern fails gracefully (e.g., skip filtering or return
no matches) instead of throwing; update the filter that creates patternRegex and
the related logic in listTools to implement these checks and error handling.
In `@src/core/client/clientManager.ts`:
- Around line 71-87: removeServer currently removes instruction entries using
the raw name while instructions are cached under a cleaned key (cleanName) in
the caching path; update the removeServer logic in clientManager.ts to compute
the same cleanName (use connectionInfo?.name || name) and call the
instructionAggregator removal method with that cleanName (e.g.,
instructionAggregator.removeInstructions(cleanName) or equivalent) instead of
using the raw name so cached instructions for template servers are properly
cleared.
In `@src/core/server/connectionResolver.ts`:
- Around line 96-108: The split on connection key using key.split(':') in
connectionResolver.ts incorrectly breaks keys that contain additional ':'
characters; change the logic in the block that derives [name, suffix] to locate
the first ':' via indexOf and use slice/substr to extract name and suffix
(falling back when no ':' exists), then proceed with the existing checks that
use name, suffix, sessionId, sessionHashes, filtered and conn; ensure you handle
missing suffix correctly so per-client and shareable template server checks
still behave as intended.
In `@src/transport/http/middlewares/tagsExtractor.ts`:
- Around line 303-328: The catch block for advanced parsing falls through when
TagQueryParser.parseSimple(filterStr) yields an empty array, causing requests
like filter=!!! or filter=,,, to be treated as "no filtering"; update the catch
block in tagsExtractor.ts (around TagQueryParser.parseSimple,
validateAndSanitizeTags, res.locals.tagFilterMode) to explicitly handle
rawTags.length === 0 by returning a 400 JSON error (use ErrorCode.InvalidParams
and a clear message like "Invalid or empty filter") and ensure you call return
after sending the response so execution does not proceed to the "no filtering"
path; keep the existing simple-or handling for non-empty valid tags and call
next() as currently done.
In `@test/e2e/lazy-loading-preset-filtering-e2e.test.ts`:
- Around line 159-203: In startHttpServer, remove the redundant
args.push('--filter', options.preset) call because the preset is already
supplied via the ONE_MCP_PRESET environment variable; keep setting
env.ONE_MCP_PRESET = options.preset and ensure only args.push('--filter',
options.tagFilter) remains for tag filtering so the CLI will load the preset
(checked via ONE_MCP_PRESET) instead of ignoring the flag.
- Around line 133-150: Move the locally-scoped testEnv created in beforeEach to
suite scope so it can be referenced in afterEach; ensure the
CommandTestEnvironment instance returned by new CommandTestEnvironment(...) is
assigned to that outer testEnv variable (instead of a const inside beforeEach),
and add an afterEach that calls await testEnv.cleanup() (and nulls it) to remove
temporary directories created by setup(); keep existing TestProcessManager,
configDir, and other setup steps unchanged.
In `@test/e2e/lazy-loading-preset-integration.test.ts`:
- Around line 27-34: Store the PresetManager instance created in each test and
call its cleanup() from the afterEach block before removing testConfigDir;
specifically, when you create a PresetManager in tests (the PresetManager
constructor/instance), save it to a local variable and in afterEach call
instance.cleanup() (or await instance.cleanup()) then call
PresetManager.resetInstance() if needed, and only after those await calls
perform rm(testConfigDir, { recursive: true, force: true }) to ensure
watchers/file handles are released; update all affected test blocks where a
PresetManager is created (and the afterEach at lines referenced) to follow this
pattern.
In `@test/e2e/lazy-tools-template-servers.test.ts`:
- Around line 123-128: Replace the flaky getRandomPort() usage by using port 0
so the OS assigns a free port: remove or stop calling the getRandomPort()
function and instead start the test server with the same pattern used in
CliTestRunner (pass the port flag as 0, e.g. "--port", "0") so the OS picks an
available port; also remove the duplicated getRandomPort() definition
(referenced in lazy-loading-preset-filtering-e2e.test.ts) and consolidate any
remaining shared port logic into a single test util if needed.
In `@test/e2e/utils/mock-mcp-server-fast.js`:
- Around line 104-116: The code currently concatenates the raw request.id into
prebuilt JSON strings (variables initializeResponse, toolsListResponse,
resourcesListResponse, promptsListResponse) which breaks when id is a string or
undefined; update the logic where responseStr is built (the branches checking
request.method and the id variable) to: compute a safeId = (id === undefined) ?
'' : `,"id":${JSON.stringify(id)}` and append safeId to the sliced response only
when id is defined (i.e., use JSON.stringify(id) to serialize strings and other
types, and omit the id entirely when undefined), applying this change for each
branch that currently does slice(0, -1) + `,"id":${id}}`.
In `@test/e2e/utils/mock-mcp-server.js`:
- Around line 77-87: The tools/call handler uses request.params without
guarding, causing a throw when params is missing; update the handler (the block
checking request.method === 'tools/call') to safely handle absent params by
using a guarded reference (e.g., const params = request.params || {}; then use
params.name and params.arguments via optional chaining or defaults) and ensure
the response builds with defaults (like name = '<unknown>' and arguments = [] or
{}), so the server returns a valid tool response instead of emitting a parse
error; keep serverName usage unchanged.
🧹 Nitpick comments (17)
src/transport/http/middlewares/tagsExtractor.ts (3)
77-90: Minor: Error message doesn't mention the newfilterparameter.The mutual exclusion error message (line 86) lists
"preset","tag-filter", and"tags"but doesn't mention the new"filter"parameter. Consider updating for completeness.Suggested improvement
error: { code: ErrorCode.InvalidParams, message: - 'Cannot use multiple filtering parameters simultaneously. Use "preset" for dynamic presets, "tag-filter" for advanced expressions, or "tags" for simple OR filtering.', + 'Cannot use multiple filtering parameters simultaneously. Use "preset" for dynamic presets, "tag-filter" for advanced expressions, "filter" for STDIO proxy compatibility, or "tags" for simple OR filtering.', },
329-337: Consider: outer catch may be unreachable.The outer
catchblock appears unreachable in practice since:
parseSimplereturns an empty array rather than throwingvalidateAndSanitizeTagsreturns a result object rather than throwing- Advanced parsing errors are caught by the inner catch
While defensive, this dead code path could be confusing. Consider either removing it or adding a comment explaining it's a safeguard for future implementation changes.
310-316: Consider adding examples to filter error response for consistency.The
tag-filterhandler (lines 264-269) provides example syntax in error responses, but thefilterhandler doesn't. Adding examples would help users understand valid formats.Suggested improvement
res.status(400).json({ error: { code: ErrorCode.InvalidParams, message: `Invalid filter: ${validation.errors.join('; ')}`, + examples: [ + 'filter=web,api', + 'filter=web+api', + 'filter=(web,api)+prod', + ], }, });src/core/capabilities/capabilityManager.ts (1)
13-24: Update JSDoc to reflect the new parameter.The doc block still references a
tagsparam and doesn’t mentionlazyLoadingOrchestrator, which can confuse readers.♻️ Proposed doc fix
- * `@param` tags Array of tags to filter clients by + * `@param` lazyLoadingOrchestrator Optional orchestrator for lazy-loading toolssrc/core/instructions/instructionAggregator.test.ts (1)
504-516: Align the “connection.name” test with the actual lookup path.This test currently only checks the in-memory instructions map, so it doesn’t exercise the connection.name vs hash-suffixed key resolution path. Consider driving it through
getFilteredInstructionswith a connections map keyed by a hash-suffixed name to validate the intended behavior.src/server.ts (1)
132-145: Consider extracting lazy-loading orchestrator setup to a helper.Async and sync setup repeat the same initialization + InternalCapabilitiesProvider wiring; a small helper would reduce drift and ease maintenance.
Also applies to: 202-215
src/core/server/templateServerManager.ts (1)
135-149: Remove async wrapper suggestion and verify intent for per-client template instruction caching.The
getInstructions()method is synchronous (no await needed), so thePromise.resolve()wrapper is unnecessary. However, there's a minor design consideration: you cache instructions undertemplateNameregardless of whether the template isshareableor per-client. If per-client templates produce different instructions across sessions, this becomes last-write-wins. SinceisShareableis already available at line 121, consider adding it as a guard if per-client templates should skip instruction caching:- if (this.instructionAggregator) { + if (this.instructionAggregator && isShareable) { try { const instructions = instance.client.getInstructions();Otherwise, if instruction caching is intentionally uniform across all template instances (since MCP
getInstructions()returns static server capabilities), the current code is fine as-is.src/core/capabilities/toolRegistry.test.ts (1)
8-26: Avoid mutating private registry state in tests.
Using(registry as any).tools.pushcouples tests to internals. Prefer public factory methods to build fixtures.♻️ Proposed refactor
const mockTools: ToolMetadata[] = [ { name: 'read_file', server: 'filesystem', description: 'Read a file', tags: ['fs', 'file'] }, { name: 'write_file', server: 'filesystem', description: 'Write a file', tags: ['fs', 'file'] }, { name: 'search', server: 'search', description: 'Search files', tags: ['search'] }, { name: 'query', server: 'database', description: 'Query database', tags: ['db', 'sql'] }, { name: 'execute', server: 'database', description: 'Execute command', tags: ['db'] }, { name: 'git_status', server: 'git', description: 'Git status', tags: ['git', 'vcs'] }, ]; + const mockToolsWithServer = mockTools.map((tool) => ({ + tool: { name: tool.name, description: tool.description, inputSchema: { type: 'object' as const } }, + server: tool.server, + tags: tool.tags, + })); let registry: ToolRegistry; beforeEach(() => { - registry = ToolRegistry.empty(); - // Manually add tools since constructor is private - for (const tool of mockTools) { - // Use reflection to access private constructor - (registry as any).tools.push(tool); - } + registry = ToolRegistry.fromToolsWithServer(mockToolsWithServer); });src/core/instructions/instructionAggregator.ts (2)
252-268: Consider edge case: colons in instructions may break description extraction.The regex
^[^.]*\.matches everything up to the first period, but instructions containing URLs (e.g.,Visit https://example.com. More info...) would extract the URL fragment. Also, the 100-character limit is arbitrary without documentation.This is a minor concern since the description is optional and used only for template display.
416-443: Hardcoded meta-tool count and TODO placeholders.The
exposedToolsCount = 3is hardcoded, which will break if meta-tools are added/removed. Consider deriving this from a constant or the orchestrator.The TODOs at lines 439 and 442 indicate incomplete implementation. Ensure these are tracked for follow-up.
Suggested improvement
+const META_TOOLS = ['tool_list', 'tool_schema', 'tool_invoke'] as const; + private generateLazyLoadingState(): LazyLoadingState | undefined { // ... if (isEnabled) { - exposedToolsCount = 3; // tool_list, tool_schema, tool_invoke + exposedToolsCount = META_TOOLS.length; } - const metaTools = isEnabled ? ['tool_list', 'tool_schema', 'tool_invoke'] : undefined; + const metaTools = isEnabled ? [...META_TOOLS] : undefined;Do you want me to open an issue to track the TODO items for
directExposeCountandcatalogimplementation?src/core/protocol/requestHandlers.ts (1)
416-419: Duplicate hardcoded meta-tool names.The
lazyToolNamesarray duplicates the meta-tool names frominstructionAggregator.ts(line 432). Consider extracting to a shared constant to ensure consistency.Suggested approach
Create a shared constant in a common location (e.g.,
constants.tsor a lazy-loading types file):// src/constants.ts or src/core/capabilities/lazyLoadingTypes.ts export const META_TOOL_NAMES = ['tool_list', 'tool_schema', 'tool_invoke'] as const;Then import and use it in both files.
test/e2e/utils/SimpleMcpClient.ts (2)
90-115: Consider making the timeout configurable.The 10-second timeout is hardcoded. For tests with slow operations or debugging, a configurable timeout would be helpful.
Suggested improvement
export interface SimpleMcpClientConfig { transport: 'stdio'; stdioConfig: { command: string; args?: string[]; env?: Record<string, string>; }; + requestTimeout?: number; // ms, default 10000 context?: { ... }; } async request(method: string, params?: Record<string, unknown>): Promise<any> { // ... return new Promise((resolve, reject) => { const timeout = setTimeout(() => { reject(new Error(`Request timeout: ${method}`)); - }, 10000); + }, this.config.requestTimeout ?? 10000);
163-167:disconnectdoesn't wait for process termination.The method kills the process but returns immediately. Tests relying on cleanup may have race conditions.
Suggested improvement
async disconnect(): Promise<void> { if (this.process) { - this.process.kill(); + return new Promise<void>((resolve) => { + this.process.once('exit', () => resolve()); + this.process.kill(); + // Fallback timeout in case process doesn't exit cleanly + setTimeout(() => resolve(), 1000); + }); } }src/core/capabilities/metaToolProvider.ts (2)
322-327: Consider usingschemaCache.set()directly instead ofpreload()for single items.The
preload()method is designed for batch loading with parallel fetching. For caching a single already-loaded schema, usingschemaCache.set(args.server, args.toolName, tool)would be more direct and efficient.Suggested simplification
- // Cache the loaded schema - await this.schemaCache.preload([{ server: args.server, toolName: args.toolName }], async (s, t) => { - if (s === args.server && t === args.toolName) { - return tool; - } - throw new Error('Unexpected preload request'); - }); + // Cache the loaded schema + this.schemaCache.set(args.server, args.toolName, tool);
439-449: Fragile error type detection based on message content.The check
error.message.includes('not found')to detect upstream not_found errors is fragile - it depends on specific wording from upstream servers which may vary or be localized.Consider either:
- Checking for specific error types/codes from the MCP SDK if available
- Treating all upstream errors uniformly as 'upstream' type
src/core/capabilities/metaToolProvider.test.ts (1)
783-789: Test assertion pattern could be clearer.The assertion
expect('error' in allowedResult && allowedResult.error).toBe(false)is a bit unusual. Iferrorexists but isundefined, this would pass. Consider using more explicit assertions.Clearer assertion
- expect('error' in allowedResult && allowedResult.error).toBe(false); + expect(allowedResult.error).toBeUndefined();src/core/capabilities/lazyLoadingOrchestrator.ts (1)
190-233: Precompile preload patterns outside the loop for safety and performance.Patterns are reconstructed as
RegExpobjects inside the hot loop (line 204–206); unescaped regex metacharacters (e.g.,.,+,^,$,{},[], etc.) can throw or match incorrectly. Precompile and validate patterns once at the start ofpreloadTools(), handle errors gracefully, and reuse the compiled regexes.♻️ Suggested refactor
+ const serverRegexes = preload.patterns + .map((pattern) => { + try { + const escaped = pattern.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + return new RegExp(`^${escaped.replace(/\*/g, '.*').replace(/\?/g, '.')}$`); + } catch (error) { + logger.warn(`Invalid preload server pattern: ${pattern}`, { + error: error instanceof Error ? error.message : String(error), + }); + return undefined; + } + }) + .filter((regex): regex is RegExp => Boolean(regex)); + for (const tool of allTools) { // Check server pattern match - const serverMatch = preload.patterns.some((pattern) => { - const regex = new RegExp(`^${pattern.replace(/\*/g, '.*')}$`); - return regex.test(tool.server); - }); + const serverMatch = serverRegexes.some((regex) => regex.test(tool.server));
Comment on lines
+195
to
206
| // Process resources (second if available) | ||
| let resultIndex = 1; | ||
| if (serverCapabilities.resources) { | ||
| const resourceResult = results[resultIndex]; | ||
| if (resourceResult && resourceResult.status === 'fulfilled') { | ||
| const resourcesResult = resourceResult.value as ListResourcesResult; | ||
| if (resourcesResult.resources) { | ||
| allResources.push(...resourcesResult.resources); | ||
| } | ||
| resultIndex++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix result indexing when resources are requested but fail.
resultIndex advances only when resources are fulfilled. If resources are requested but rejected, the prompts result shifts to the next index and is never read, so prompts can be silently dropped.
🛠️ Suggested fix
- let resultIndex = 1;
- if (serverCapabilities.resources) {
- const resourceResult = results[resultIndex];
- if (resourceResult && resourceResult.status === 'fulfilled') {
- const resourcesResult = resourceResult.value as ListResourcesResult;
- if (resourcesResult.resources) {
- allResources.push(...resourcesResult.resources);
- }
- resultIndex++;
- }
- }
+ let resultIndex = 1;
+ if (serverCapabilities.resources) {
+ const resourceResult = results[resultIndex];
+ if (resourceResult && resourceResult.status === 'fulfilled') {
+ const resourcesResult = resourceResult.value as ListResourcesResult;
+ if (resourcesResult.resources) {
+ allResources.push(...resourcesResult.resources);
+ }
+ }
+ // Always advance when resources were requested
+ resultIndex++;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Process resources (second if available) | |
| let resultIndex = 1; | |
| if (serverCapabilities.resources) { | |
| const resourceResult = results[resultIndex]; | |
| if (resourceResult && resourceResult.status === 'fulfilled') { | |
| const resourcesResult = resourceResult.value as ListResourcesResult; | |
| if (resourcesResult.resources) { | |
| allResources.push(...resourcesResult.resources); | |
| } | |
| resultIndex++; | |
| } | |
| } | |
| // Process resources (second if available) | |
| let resultIndex = 1; | |
| if (serverCapabilities.resources) { | |
| const resourceResult = results[resultIndex]; | |
| if (resourceResult && resourceResult.status === 'fulfilled') { | |
| const resourcesResult = resourceResult.value as ListResourcesResult; | |
| if (resourcesResult.resources) { | |
| allResources.push(...resourcesResult.resources); | |
| } | |
| } | |
| // Always advance when resources were requested | |
| resultIndex++; | |
| } |
🤖 Prompt for AI Agents
In `@src/core/capabilities/capabilityAggregator.ts` around lines 195 - 206, The
code advances resultIndex only when resources are fulfilled, so if a resources
query was requested but rejected the next item (prompts) is read from the wrong
index and dropped; in capabilityAggregator.ts locate the block using
serverCapabilities.resources, results, resultIndex, resourceResult,
resourcesResult and allResources and change the logic to always increment
resultIndex when resources were requested (i.e., move or add resultIndex++ to
run regardless of resourceResult.status) while still only pushing to
allResources when resourcesResult.resources exists so prompts use the correct
subsequent index.
Comment on lines
+250
to
+253
| // === Lazy Tools (controlled by lazyLoading.enabled) === | ||
| if (this.flagManager.isToolEnabled('lazyTools')) { | ||
| tools.push(createToolListTool(), createToolSchemaTool(), createToolInvokeTool()); | ||
| } |
There was a problem hiding this comment.
Guard lazy-tools exposure when the orchestrator isn’t wired.
If lazyTools is enabled but setLazyLoadingOrchestrator hasn’t run yet, clients will discover meta-tools that always fail with “Lazy loading not available.” Consider only exposing these tools when the orchestrator is present (or log a warning and skip).
🛠️ Suggested guard
- if (this.flagManager.isToolEnabled('lazyTools')) {
- tools.push(createToolListTool(), createToolSchemaTool(), createToolInvokeTool());
- }
+ if (this.flagManager.isToolEnabled('lazyTools')) {
+ if (!this.lazyLoadingOrchestrator) {
+ logger.warn('lazyTools enabled but LazyLoadingOrchestrator is not set');
+ } else {
+ tools.push(createToolListTool(), createToolSchemaTool(), createToolInvokeTool());
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // === Lazy Tools (controlled by lazyLoading.enabled) === | |
| if (this.flagManager.isToolEnabled('lazyTools')) { | |
| tools.push(createToolListTool(), createToolSchemaTool(), createToolInvokeTool()); | |
| } | |
| // === Lazy Tools (controlled by lazyLoading.enabled) === | |
| if (this.flagManager.isToolEnabled('lazyTools')) { | |
| if (!this.lazyLoadingOrchestrator) { | |
| logger.warn('lazyTools enabled but LazyLoadingOrchestrator is not set'); | |
| } else { | |
| tools.push(createToolListTool(), createToolSchemaTool(), createToolInvokeTool()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/core/capabilities/internalCapabilitiesProvider.ts` around lines 250 -
253, When flagManager.isToolEnabled('lazyTools') is true, guard exposing the
lazy meta-tools until the orchestrator is wired by checking whether
setLazyLoadingOrchestrator has been called or the orchestrator instance is
present; if the orchestrator is absent, skip pushing createToolListTool(),
createToolSchemaTool(), and createToolInvokeTool() (or log a warning and skip)
so clients don't discover tools that always fail with "Lazy loading not
available." Ensure the check references the orchestrator state used by
setLazyLoadingOrchestrator and gate the tools push accordingly.
Comment on lines
+1048
to
+1053
| // Store filter via getCapabilitiesForFilteredServers | ||
| orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId); | ||
|
|
||
| // Should retrieve the same filter | ||
| const retrieved = orchestrator.getSessionAllowedServers(sessionId); | ||
| expect(retrieved).toBeDefined(); |
There was a problem hiding this comment.
Await the async call to avoid unhandled promises.
At Line 1049, getCapabilitiesForFilteredServers is invoked without await, while other tests await it. If it rejects or sets filters after an await, this can cause flakes and unobserved failures.
✅ Suggested fix
- orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId);
+ await orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Store filter via getCapabilitiesForFilteredServers | |
| orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId); | |
| // Should retrieve the same filter | |
| const retrieved = orchestrator.getSessionAllowedServers(sessionId); | |
| expect(retrieved).toBeDefined(); | |
| // Store filter via getCapabilitiesForFilteredServers | |
| await orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId); | |
| // Should retrieve the same filter | |
| const retrieved = orchestrator.getSessionAllowedServers(sessionId); | |
| expect(retrieved).toBeDefined(); |
🤖 Prompt for AI Agents
In `@src/core/capabilities/lazyLoadingOrchestrator.test.ts` around lines 1048 -
1053, The test calls
orchestrator.getCapabilitiesForFilteredServers(allowedServers, sessionId)
without awaiting it, which can leave an unhandled promise and cause flakes;
update the test to await the async call (add await before
getCapabilitiesForFilteredServers) so the filter is fully set before calling
orchestrator.getSessionAllowedServers(sessionId) and the subsequent expect
assertion; reference the existing variables orchestrat or, allowedServers, and
sessionId when making the change.
Comment on lines
+238
to
+252
| private async loadSchemaFromServer(server: string, toolName: string): Promise<Tool> { | ||
| // Use ConnectionResolver to find the connection (handles template servers with hash-suffixed keys) | ||
| const result = this.connectionResolver.findByServerName(server); | ||
| if (!result || !result.connection.client) { | ||
| throw new Error(`Server not connected: ${server}`); | ||
| } | ||
|
|
||
| // Get the tool from server's listTools | ||
| const toolsResult = await result.connection.client.listTools(); | ||
| const tool = toolsResult.tools.find((t) => t.name === toolName); | ||
|
|
||
| if (!tool) { | ||
| throw new Error(`Tool not found: ${server}:${toolName}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ConnectionResolver class and findByServerName method
echo "=== ConnectionResolver class and findByServerName ==="
find src/core -name "*.ts" | xargs grep -l "class ConnectionResolver" | head -5Repository: 1mcp-app/agent
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Look for ConnectionResolver implementation
rg -n "class ConnectionResolver|findByServerName" src/core -A 10 -B 2Repository: 1mcp-app/agent
Length of output: 11269
🏁 Script executed:
#!/bin/bash
# Find all call sites of loadSchemaFromServer to understand context
rg -n "loadSchemaFromServer" src/core -C 3Repository: 1mcp-app/agent
Length of output: 2607
🏁 Script executed:
#!/bin/bash
# Check how template servers are managed and if there's session context
rg -n "template.*server|session" src/core/capabilities/lazyLoadingOrchestrator.ts -i -C 2Repository: 1mcp-app/agent
Length of output: 3891
Pass sessionId through to loadSchemaFromServer for session-aware template server resolution.
Multiple template instances can share a server name (e.g., "template-server:abc123" and "template-server:def456" both have connection.name "template-server"). Currently, findByServerName() lacks session context and returns whichever instance matches first during Map iteration, which could target the wrong connection across sessions.
The codebase already tracks session-specific filters in sessionAllowedServers and uses sessionId in getCapabilitiesForFilteredServers() and callMetaTool(). Thread this same sessionId parameter through loadSchemaFromServer() and update findByServerName() to accept and respect session context, or provide a findByServerNameAndSession() variant.
🤖 Prompt for AI Agents
In `@src/core/capabilities/lazyLoadingOrchestrator.ts` around lines 238 - 252,
loadSchemaFromServer currently calls ConnectionResolver.findByServerName without
session context, causing incorrect template-instance resolution across sessions;
modify loadSchemaFromServer to accept a sessionId parameter and pass it to a
session-aware lookup, then update ConnectionResolver.findByServerName (or add
findByServerNameAndSession) to use the sessionId and sessionAllowedServers logic
when resolving connections. Specifically, change the signature of
loadSchemaFromServer(server: string, toolName: string) to include sessionId,
thread that sessionId through callers like getCapabilitiesForFilteredServers and
callMetaTool, and alter/find the resolver method so it filters connections by
the provided sessionId before returning result.connection.client so the correct
template instance is selected.
Comment on lines
+159
to
+203
| async function startHttpServer(options: { | ||
| enableLazyLoading: boolean; | ||
| enableInternalTools?: boolean; | ||
| tagFilter?: string; | ||
| preset?: string; | ||
| }): Promise<void> { | ||
| const args = [ | ||
| resolve(__dirname, '../../build/index.js'), | ||
| 'serve', | ||
| '--transport', | ||
| 'http', | ||
| '--port', | ||
| String(httpPort), | ||
| '--config-dir', | ||
| configDir, | ||
| ]; | ||
|
|
||
| if (options.enableLazyLoading) { | ||
| args.push('--enable-lazy-loading'); | ||
| } | ||
|
|
||
| if (options.enableInternalTools) { | ||
| args.push('--enable-internal-tools'); | ||
| } | ||
|
|
||
| if (options.tagFilter) { | ||
| args.push('--filter', options.tagFilter); | ||
| } | ||
|
|
||
| if (options.preset) { | ||
| args.push('--filter', options.preset); | ||
| } | ||
|
|
||
| const env: Record<string, string> = { | ||
| ONE_MCP_CONFIG_DIR: configDir, | ||
| NODE_ENV: 'test', | ||
| ONE_MCP_LOG_LEVEL: 'error', | ||
| ONE_MCP_ENABLE_AUTH: 'false', | ||
| }; | ||
|
|
||
| // Set preset via environment variable for proper preset resolution | ||
| if (options.preset) { | ||
| env.ONE_MCP_PRESET = options.preset; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check CLI flag definitions and preset handling in serve command
find . -type f -name "*.ts" -path "*/src/commands/serve*" | head -20Repository: 1mcp-app/agent
Length of output: 152
🏁 Script executed:
# Search for filter and preset flag definitions
rg -n "filter|preset" src/commands/serve -g'*.ts' -i --context=3Repository: 1mcp-app/agent
Length of output: 13604
🏁 Script executed:
# Search for ONE_MCP_PRESET environment variable usage
rg -n "ONE_MCP_PRESET" -g'*.ts' --context=3Repository: 1mcp-app/agent
Length of output: 1240
🏁 Script executed:
# Look for the serve command implementation and flag parsing
fd -e ts -p 'serve' src/ | grep -E 'serve|command' | head -20Repository: 1mcp-app/agent
Length of output: 918
Remove redundant --filter argument; preset is set via ONE_MCP_PRESET environment variable.
The --filter flag is only applied when no preset is loaded via ONE_MCP_PRESET. Since the test code sets ONE_MCP_PRESET, the --filter argument with the preset name will be ignored (the CLI implementation checks ONE_MCP_PRESET first at line 419 and skips --filter if a preset was successfully loaded). Remove the redundant args.push('--filter', options.preset) to clarify the intent.
🤖 Prompt for AI Agents
In `@test/e2e/lazy-loading-preset-filtering-e2e.test.ts` around lines 159 - 203,
In startHttpServer, remove the redundant args.push('--filter', options.preset)
call because the preset is already supplied via the ONE_MCP_PRESET environment
variable; keep setting env.ONE_MCP_PRESET = options.preset and ensure only
args.push('--filter', options.tagFilter) remains for tag filtering so the CLI
will load the preset (checked via ONE_MCP_PRESET) instead of ignoring the flag.
Comment on lines
+27
to
+34
| afterEach(async () => { | ||
| // Clean up the test directory | ||
| try { | ||
| await rm(testConfigDir, { recursive: true, force: true }); | ||
| } catch (_error) { | ||
| // Ignore cleanup errors | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C3 "resetInstance|cleanup" src/domains/preset/manager/presetManager.tsRepository: 1mcp-app/agent
Length of output: 1318
🏁 Script executed:
cat -n test/e2e/lazy-loading-preset-integration.test.ts | head -140Repository: 1mcp-app/agent
Length of output: 6288
Call cleanup() in afterEach to prevent resource leaks from PresetManager watchers.
PresetManager.resetInstance() does call cleanup() internally, but only asynchronously at the start of each test to reset the previous test's state. The PresetManager instances created within each test are never cleaned up before afterEach removes the test directory, leaving watchers and file handles open. Store the instance and explicitly clean it up in afterEach to ensure proper resource release before directory removal.
🧹 Proposed fix
- let testConfigDir: string;
+ let testConfigDir: string;
+ let presetManager: PresetManager | undefined;
...
afterEach(async () => {
+ if (presetManager) {
+ await presetManager.cleanup();
+ PresetManager.resetInstance();
+ presetManager = undefined;
+ }
// Clean up the test directory
try {
await rm(testConfigDir, { recursive: true, force: true });
} catch (_error) {
// Ignore cleanup errors
}
});
...
- const presetManager = PresetManager.getInstance(testConfigDir);
- await presetManager.initialize();
+ presetManager = PresetManager.getInstance(testConfigDir);
+ await presetManager.initialize();
...
- const presetManager = PresetManager.getInstance(testConfigDir);
- await presetManager.initialize();
+ presetManager = PresetManager.getInstance(testConfigDir);
+ await presetManager.initialize();Also applies to: 50-53, 120-123
🤖 Prompt for AI Agents
In `@test/e2e/lazy-loading-preset-integration.test.ts` around lines 27 - 34, Store
the PresetManager instance created in each test and call its cleanup() from the
afterEach block before removing testConfigDir; specifically, when you create a
PresetManager in tests (the PresetManager constructor/instance), save it to a
local variable and in afterEach call instance.cleanup() (or await
instance.cleanup()) then call PresetManager.resetInstance() if needed, and only
after those await calls perform rm(testConfigDir, { recursive: true, force: true
}) to ensure watchers/file handles are released; update all affected test blocks
where a PresetManager is created (and the afterEach at lines referenced) to
follow this pattern.
Comment on lines
+123
to
+128
| /** | ||
| * Generate a random port in the range 10000-60000 | ||
| */ | ||
| function getRandomPort(): number { | ||
| return 10000 + Math.floor(Math.random() * 50000); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for existing free-port helpers or conventions in the repo
rg -n "getRandomPort|getAvailablePort|findOpenPort|reservePort|freePort" -g'*.ts' -g'*.js'Repository: 1mcp-app/agent
Length of output: 399
🏁 Script executed:
# Also check the test file to see the context and usage of getRandomPort
cat -n test/e2e/lazy-tools-template-servers.test.ts | head -150Repository: 1mcp-app/agent
Length of output: 5538
🏁 Script executed:
# Search for any port-related patterns or dependencies
rg -n "port" test/e2e/lazy-tools-template-servers.test.ts -A 2 -B 2Repository: 1mcp-app/agent
Length of output: 2180
🏁 Script executed:
# Check if there are any package dependencies related to port management
rg -n "port-finder|get-port|detect-port|available-port" package.jsonRepository: 1mcp-app/agent
Length of output: 40
🏁 Script executed:
# Search for port conflict error handling (EADDRINUSE)
rg -n "EADDRINUSE|already in use|port.*error|port.*conflict" -g'*.ts' -g'*.js'Repository: 1mcp-app/agent
Length of output: 12551
🏁 Script executed:
# Check the TestProcessManager to see how it handles server startup
fd -t f "TestProcessManager|test.*manager" --type f | head -20Repository: 1mcp-app/agent
Length of output: 94
🏁 Script executed:
# Search for any existing port detection or binding strategies
rg -n "bind.*0|port.*0|getPort|availablePort|freePort" -g'*.ts' -g'*.js'Repository: 1mcp-app/agent
Length of output: 21844
Use port 0 instead of random port selection to avoid flaky tests.
Randomly chosen ports can collide with already-in-use ports and cause flaky E2E runs. The codebase already demonstrates a better pattern in test/e2e/utils/CliTestRunner.ts (line 135) using --port 0, which lets the OS automatically assign a free port. Consider applying this same approach here instead of getRandomPort(). Also note that getRandomPort() is duplicated in test/e2e/lazy-loading-preset-filtering-e2e.test.ts and could be consolidated.
🤖 Prompt for AI Agents
In `@test/e2e/lazy-tools-template-servers.test.ts` around lines 123 - 128, Replace
the flaky getRandomPort() usage by using port 0 so the OS assigns a free port:
remove or stop calling the getRandomPort() function and instead start the test
server with the same pattern used in CliTestRunner (pass the port flag as 0,
e.g. "--port", "0") so the OS picks an available port; also remove the
duplicated getRandomPort() definition (referenced in
lazy-loading-preset-filtering-e2e.test.ts) and consolidate any remaining shared
port logic into a single test util if needed.
Comment on lines
+104
to
+116
| // Build response based on method | ||
| let responseStr; | ||
| let id = request.id; | ||
|
|
||
| if (request.method === 'initialize') { | ||
| responseStr = initializeResponse.slice(0, -1) + `,"id":${id}}`; | ||
| } else if (request.method === 'tools/list') { | ||
| responseStr = toolsListResponse.slice(0, -1) + `,"id":${id}}`; | ||
| } else if (request.method === 'resources/list') { | ||
| responseStr = resourcesListResponse.slice(0, -1) + `,"id":${id}}`; | ||
| } else if (request.method === 'prompts/list') { | ||
| responseStr = promptsListResponse.slice(0, -1) + `,"id":${id}}`; | ||
| } else if (request.method === 'tools/call') { |
There was a problem hiding this comment.
Serialize JSON-RPC id safely when stitching prebuilt responses.
If request.id is a string or undefined, ,"id":${id} yields invalid JSON. Use JSON.stringify(id) and omit the id when it’s undefined.
🐛 Proposed fix
+function attachId(base, id) {
+ if (id === undefined) return base;
+ return base.slice(0, -1) + `,"id":${JSON.stringify(id)}}`;
+}
...
- responseStr = initializeResponse.slice(0, -1) + `,"id":${id}}`;
+ responseStr = attachId(initializeResponse, id);
} else if (request.method === 'tools/list') {
- responseStr = toolsListResponse.slice(0, -1) + `,"id":${id}}`;
+ responseStr = attachId(toolsListResponse, id);
} else if (request.method === 'resources/list') {
- responseStr = resourcesListResponse.slice(0, -1) + `,"id":${id}}`;
+ responseStr = attachId(resourcesListResponse, id);
} else if (request.method === 'prompts/list') {
- responseStr = promptsListResponse.slice(0, -1) + `,"id":${id}}`;
+ responseStr = attachId(promptsListResponse, id);🤖 Prompt for AI Agents
In `@test/e2e/utils/mock-mcp-server-fast.js` around lines 104 - 116, The code
currently concatenates the raw request.id into prebuilt JSON strings (variables
initializeResponse, toolsListResponse, resourcesListResponse,
promptsListResponse) which breaks when id is a string or undefined; update the
logic where responseStr is built (the branches checking request.method and the
id variable) to: compute a safeId = (id === undefined) ? '' :
`,"id":${JSON.stringify(id)}` and append safeId to the sliced response only when
id is defined (i.e., use JSON.stringify(id) to serialize strings and other
types, and omit the id entirely when undefined), applying this change for each
branch that currently does slice(0, -1) + `,"id":${id}}`.
Comment on lines
+77
to
+87
| // Handle tools/call | ||
| if (request.method === 'tools/call') { | ||
| const response = { | ||
| jsonrpc: '2.0', | ||
| id: request.id, | ||
| result: { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `Called ${request.params.name} on ${serverName} with args: ${JSON.stringify(request.params.arguments)}`, | ||
| }, |
There was a problem hiding this comment.
Guard against missing params in tools/call requests.
If request.params is absent, the handler throws and emits a parse error instead of a tool response.
🐛 Proposed fix
- if (request.method === 'tools/call') {
+ if (request.method === 'tools/call') {
+ const params = request.params ?? {};
const response = {
jsonrpc: '2.0',
id: request.id,
result: {
content: [
{
type: 'text',
- text: `Called ${request.params.name} on ${serverName} with args: ${JSON.stringify(request.params.arguments)}`,
+ text: `Called ${params.name ?? 'unknown'} on ${serverName} with args: ${JSON.stringify(
+ params.arguments ?? {},
+ )}`,
},
],
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handle tools/call | |
| if (request.method === 'tools/call') { | |
| const response = { | |
| jsonrpc: '2.0', | |
| id: request.id, | |
| result: { | |
| content: [ | |
| { | |
| type: 'text', | |
| text: `Called ${request.params.name} on ${serverName} with args: ${JSON.stringify(request.params.arguments)}`, | |
| }, | |
| // Handle tools/call | |
| if (request.method === 'tools/call') { | |
| const params = request.params ?? {}; | |
| const response = { | |
| jsonrpc: '2.0', | |
| id: request.id, | |
| result: { | |
| content: [ | |
| { | |
| type: 'text', | |
| text: `Called ${params.name ?? 'unknown'} on ${serverName} with args: ${JSON.stringify( | |
| params.arguments ?? {}, | |
| )}`, | |
| }, |
🤖 Prompt for AI Agents
In `@test/e2e/utils/mock-mcp-server.js` around lines 77 - 87, The tools/call
handler uses request.params without guarding, causing a throw when params is
missing; update the handler (the block checking request.method === 'tools/call')
to safely handle absent params by using a guarded reference (e.g., const params
= request.params || {}; then use params.name and params.arguments via optional
chaining or defaults) and ensure the response builds with defaults (like name =
'<unknown>' and arguments = [] or {}), so the server returns a valid tool
response instead of emitting a parse error; keep serverName usage unchanged.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/transport/http/middlewares/tagsExtractor.ts (1)
78-88: Error message doesn't mention the newfilterparameter.The mutual exclusion error message still only references
preset,tag-filter, andtags, but doesn't mention the newly addedfilterparameter. Users combiningfilterwith another parameter would receive a confusing message.Proposed fix
if (paramCount > 1) { sendBadRequest( res, - 'Cannot use multiple filtering parameters simultaneously. Use "preset" for dynamic presets, "tag-filter" for advanced expressions, or "tags" for simple OR filtering.', + 'Cannot use multiple filtering parameters simultaneously. Use "preset" for dynamic presets, "tag-filter" for advanced expressions, "filter" for STDIO proxy compatibility, or "tags" for simple OR filtering.', ); return; }src/commands/serve/serve.ts (1)
300-374: Propagate--lazy-modeso the CLI option actually takes effect.
lazy-modeis defined in CLI options but never written intolazyLoadingconfig, so user-selected modes are ignored.✅ Suggested fix
- serverConfigManager.updateConfig({ + const lazyMode = (parsedArgv['lazy-mode'] || 'full') as 'metatool' | 'hybrid' | 'full'; + serverConfigManager.updateConfig({ @@ - lazyLoading: { - enabled: parsedArgv['enable-lazy-loading'], + lazyLoading: { + enabled: parsedArgv['enable-lazy-loading'] && lazyMode !== 'full', + mode: lazyMode, inlineCatalog: parsedArgv['lazy-inline-catalog'], catalogFormat: (parsedArgv['lazy-catalog-format'] || 'grouped') as 'flat' | 'grouped' | 'categorized',
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 68-70: Replace duplicate top-level "## New Contributors" headings
with unique identifiers per release by appending the release tag or version
(e.g., "## New Contributors — v1.2.3") or convert them into a release-scoped
subheading under the release section (e.g., "### New Contributors") so anchors
are unique; update each occurrence of the heading text "## New Contributors"
(including the instances noted) consistently across the changelog to follow the
chosen pattern.
In `@src/core/capabilities/lazyLoadingOrchestrator.ts`:
- Around line 568-623: In getHealthStatus, guard against
lazyConfig.cache.maxEntries being zero before computing utilizationRate: if
maxEntries is 0 (or falsy) set utilizationRate to 0 (or a safe sentinel) and
avoid dividing by zero; update the health output to reflect this safe value and
keep checks that push issues (e.g., Cache utilization high) using the guarded
utilizationRate; reference lazyConfig.cache.maxEntries, utilizationRate,
schemaCache.getStats(), and schemaCache.size() to locate the code to change.
- Around line 214-266: The preload pattern conversion only replaces '*' but
leaves '?' as a regex quantifier; update the pattern escaping in preload
handling (the code using preload.patterns inside preloadTools) to mirror the
glob-to-regex logic used by ToolRegistry.listTools: escape regex special chars,
convert '*' -> '.*' and convert '?' -> '.', then build the RegExp from
`^${escaped}$`; keep the try/catch/errorIf logic and the
serverMatch/keywordMatch checks and ensure this change is applied where tools
are gathered before calling this.schemaCache.preload and
this.loadSchemaFromServer.
In `@src/core/capabilities/metaToolProvider.ts`:
- Around line 224-244: In listAvailableTools, the code uses
registry.getServers() which ignores per-call filters; replace that with the
filtered servers returned by result (use result.servers) so the servers output
aligns with the returned tools; update the servers variable in the
ListToolsResult construction within the listAvailableTools method to reference
result.servers instead of calling registry.getServers().
In `@src/core/capabilities/toolRegistry.ts`:
- Around line 176-194: The cursor handling in ToolRegistry.decodeCursor
currently only uses decoded.offset but ignores decoded.server/ pattern/tag,
allowing clients to reuse a cursor with different filters; update the logic in
the pagination block (around ToolRegistry.decodeCursor, offset, filtered, and
ToolRegistry.encodeCursor) to validate that the decoded cursor's server,
pattern, and tag match the current options.server/options.pattern/options.tag
and if they differ either reject/ignore the cursor (reset offset to 0) or
surface an error; ensure nextCursor continues to encode the correct current
filters when calling ToolRegistry.encodeCursor so pagination remains consistent
with the active filters.
- Around line 181-183: The current limit computation treats 0 as falsy and
allows negative values, causing unexpected slice ranges; update the logic around
DEFAULT_PAGE_SIZE and limit so you use a nullish check on options.limit
(options.limit ?? DEFAULT_PAGE_SIZE) and clamp the result with Math.max to
enforce a minimum of DEFAULT_PAGE_SIZE (and still Math.min with 5000) before
calling filtered.slice(offset, offset + limit); adjust the variable named limit
that is derived from options.limit and ensure filtered.slice uses this validated
positive bounded value.
🧹 Nitpick comments (3)
src/transport/http/middlewares/tagsExtractor.ts (1)
240-247: Consider usingsendBadRequesthelper for consistency.The
filterhandler uses inlineres.status(400).json(...)for error responses while other handlers (preset,tags,tag-filter) use the importedsendBadRequest()helper. Using the helper consistently would improve maintainability and ensure uniform error response formatting.Example refactor for one occurrence
if (typeof filterStr !== 'string') { - res.status(400).json({ - error: { - code: ErrorCode.InvalidParams, - message: 'Invalid params: filter must be a string', - }, - }); + sendBadRequest(res, 'Invalid params: filter must be a string'); return; }Also applies to: 270-276, 289-296
src/commands/serve/serve.test.ts (1)
112-121: Reduce repeated lazy-loading option literals across tests.
The same block appears three times; extracting it keeps updates in one place.♻️ Suggested refactor
+const lazyLoadingOptions = { + 'enable-lazy-loading': false, + 'lazy-mode': 'full', + 'lazy-inline-catalog': false, + 'lazy-catalog-format': 'grouped', + 'lazy-cache-max-entries': 1000, + 'lazy-cache-ttl': 300000, + 'lazy-preload': undefined, + 'lazy-preload-keywords': undefined, + 'lazy-fallback-on-error': undefined, + 'lazy-fallback-timeout': undefined, +};- 'enable-lazy-loading': false, - 'lazy-mode': 'full', - 'lazy-inline-catalog': false, - 'lazy-catalog-format': 'grouped', - 'lazy-cache-max-entries': 1000, - 'lazy-cache-ttl': 300000, - 'lazy-preload': undefined, - 'lazy-preload-keywords': undefined, - 'lazy-fallback-on-error': undefined, - 'lazy-fallback-timeout': undefined, + ...lazyLoadingOptions,Apply the same replacement in the other two option objects.
Also applies to: 184-193, 249-258
src/core/capabilities/toolRegistry.ts (1)
147-165: Precompile the pattern regex once per call to avoid log spam and extra work.
Line 149–165 compiles the regex for every tool and logs on every failure. Consider compiling once, then filtering (or short-circuiting) to avoid O(n) RegExp creation and repeated error logs.♻️ Proposed refactor
if (options.pattern) { - filtered = filtered.filter((t) => { - try { - // Escape special regex characters except * and ? - const escaped = options - .pattern!.replace(/[.+^${}()|[\]\\]/g, '\\$&') // Escape special chars - .replace(/\*/g, '.*') // * becomes .* - .replace(/\?/g, '.'); // ? becomes . - const patternRegex = new RegExp(`^${escaped}$`); - return patternRegex.test(t.name); - } catch (error) { - // Invalid regex pattern - log and exclude this tool - errorIf(() => ({ - message: 'Invalid pattern regex in tool filter', - meta: { pattern: options.pattern, error }, - })); - return false; - } - }); + let patternRegex: RegExp | undefined; + try { + const escaped = options + .pattern!.replace(/[.+^${}()|[\]\\]/g, '\\$&') + .replace(/\*/g, '.*') + .replace(/\?/g, '.'); + patternRegex = new RegExp(`^${escaped}$`); + } catch (error) { + errorIf(() => ({ + message: 'Invalid pattern regex in tool filter', + meta: { pattern: options.pattern, error }, + })); + } + + filtered = patternRegex ? filtered.filter((t) => patternRegex!.test(t.name)) : []; }
Comment on lines
68
to
+70
| ## New Contributors | ||
| * @triepod-ai made their first contribution in [#197](https://github.com/1mcp-app/agent/pull/197) | ||
|
|
||
| - @triepod-ai made their first contribution in [#197](https://github.com/1mcp-app/agent/pull/197) |
There was a problem hiding this comment.
Disambiguate repeated “New Contributors” headings to avoid duplicate anchors.
Consider appending the release version or demoting these to a subheading under each release.
✏️ Suggested pattern (apply to each release section)
-## New Contributors
+## New Contributors (0.28.1)Also applies to: 88-90, 417-419, 872-874, 1023-1026
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 68 - 70, Replace duplicate top-level "## New
Contributors" headings with unique identifiers per release by appending the
release tag or version (e.g., "## New Contributors — v1.2.3") or convert them
into a release-scoped subheading under the release section (e.g., "### New
Contributors") so anchors are unique; update each occurrence of the heading text
"## New Contributors" (including the instances noted) consistently across the
changelog to follow the chosen pattern.
Comment on lines
+214
to
+266
| private async preloadTools(): Promise<void> { | ||
| const lazyConfig = this.config.get('lazyLoading'); | ||
| const preload = lazyConfig.preload; | ||
|
|
||
| if (preload.patterns.length === 0 && preload.keywords.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Find tools to preload | ||
| const toolsToPreload: Array<{ server: string; toolName: string }> = []; | ||
| const allTools = this.toolRegistry.listTools({}).tools; | ||
|
|
||
| for (const tool of allTools) { | ||
| // Check server pattern match | ||
| const serverMatch = preload.patterns.some((pattern) => { | ||
| try { | ||
| const escaped = pattern.replace(/[.+^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '.*'); | ||
| const regex = new RegExp(`^${escaped}$`); | ||
| return regex.test(tool.server); | ||
| } catch (error) { | ||
| errorIf(() => ({ | ||
| message: 'Invalid pattern in preload configuration', | ||
| meta: { pattern, error }, | ||
| })); | ||
| return false; | ||
| } | ||
| }); | ||
|
|
||
| // Check keyword match | ||
| const keywordMatch = preload.keywords.some((keyword) => tool.name.toLowerCase().includes(keyword.toLowerCase())); | ||
|
|
||
| if (serverMatch || keywordMatch) { | ||
| toolsToPreload.push({ | ||
| server: tool.server, | ||
| toolName: tool.name, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (toolsToPreload.length === 0) { | ||
| debugIf('No tools matched preload patterns'); | ||
| return; | ||
| } | ||
|
|
||
| debugIf(() => ({ message: `Preloading ${toolsToPreload.length} tools` })); | ||
|
|
||
| // Preload schemas | ||
| await this.schemaCache.preload(toolsToPreload, async (server, toolName) => { | ||
| return this.loadSchemaFromServer(server, toolName); | ||
| }); | ||
|
|
||
| logger.info(`Preloaded ${toolsToPreload.length} tool schemas`); | ||
| } |
There was a problem hiding this comment.
Preload patterns handle * but not ? (inconsistent with listTools).
Line 230–231 only converts * to .*; ? remains a regex quantifier. If you intend glob-like semantics, map ? to . like in ToolRegistry.listTools.
🛠️ Suggested fix
- const escaped = pattern.replace(/[.+^${}()|[\]\\]/g, '\\$&').replace(/\*/g, '.*');
+ const escaped = pattern
+ .replace(/[.+^${}()|[\]\\]/g, '\\$&')
+ .replace(/\*/g, '.*')
+ .replace(/\?/g, '.');🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 230-230: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escaped}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@src/core/capabilities/lazyLoadingOrchestrator.ts` around lines 214 - 266, The
preload pattern conversion only replaces '*' but leaves '?' as a regex
quantifier; update the pattern escaping in preload handling (the code using
preload.patterns inside preloadTools) to mirror the glob-to-regex logic used by
ToolRegistry.listTools: escape regex special chars, convert '*' -> '.*' and
convert '?' -> '.', then build the RegExp from `^${escaped}$`; keep the
try/catch/errorIf logic and the serverMatch/keywordMatch checks and ensure this
change is applied where tools are gathered before calling
this.schemaCache.preload and this.loadSchemaFromServer.
Comment on lines
+568
to
+623
| public getHealthStatus(): { | ||
| healthy: boolean; | ||
| enabled: boolean; | ||
| cache: { | ||
| size: number; | ||
| maxEntries: number; | ||
| utilizationRate: number; | ||
| }; | ||
| stats: { | ||
| hitRate: number; | ||
| coalescedRequests: number; | ||
| evictions: number; | ||
| }; | ||
| issues: string[]; | ||
| } { | ||
| const lazyConfig = this.config.get('lazyLoading'); | ||
| const cacheStats = this.schemaCache.getStats(); | ||
| const cacheSize = this.schemaCache.size(); | ||
| const issues: string[] = []; | ||
|
|
||
| // Check cache utilization | ||
| const utilizationRate = (cacheSize / lazyConfig.cache.maxEntries) * 100; | ||
| if (utilizationRate > 90) { | ||
| issues.push(`Cache utilization high: ${utilizationRate.toFixed(1)}%`); | ||
| } | ||
|
|
||
| // Check hit rate | ||
| const totalRequests = cacheStats.hits + cacheStats.misses; | ||
| const hitRate = totalRequests > 0 ? cacheStats.hitRate : 0; | ||
|
|
||
| // Only warn about low hit rate if we've had enough requests | ||
| if (totalRequests > 100 && hitRate < 50) { | ||
| issues.push(`Low cache hit rate: ${hitRate.toFixed(1)}%`); | ||
| } | ||
|
|
||
| // Check eviction rate | ||
| if (cacheStats.evictions > 100) { | ||
| issues.push(`High eviction count: ${cacheStats.evictions}`); | ||
| } | ||
|
|
||
| return { | ||
| healthy: issues.length === 0, | ||
| enabled: lazyConfig.enabled, | ||
| cache: { | ||
| size: cacheSize, | ||
| maxEntries: lazyConfig.cache.maxEntries, | ||
| utilizationRate, | ||
| }, | ||
| stats: { | ||
| hitRate, | ||
| coalescedRequests: cacheStats.coalesced, | ||
| evictions: cacheStats.evictions, | ||
| }, | ||
| issues, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Guard cache utilization against zero maxEntries.
If lazyConfig.cache.maxEntries is 0, utilization becomes Infinity and can produce misleading health output.
🛠️ Suggested fix
- const utilizationRate = (cacheSize / lazyConfig.cache.maxEntries) * 100;
+ const maxEntries = lazyConfig.cache.maxEntries;
+ const utilizationRate = maxEntries > 0 ? (cacheSize / maxEntries) * 100 : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public getHealthStatus(): { | |
| healthy: boolean; | |
| enabled: boolean; | |
| cache: { | |
| size: number; | |
| maxEntries: number; | |
| utilizationRate: number; | |
| }; | |
| stats: { | |
| hitRate: number; | |
| coalescedRequests: number; | |
| evictions: number; | |
| }; | |
| issues: string[]; | |
| } { | |
| const lazyConfig = this.config.get('lazyLoading'); | |
| const cacheStats = this.schemaCache.getStats(); | |
| const cacheSize = this.schemaCache.size(); | |
| const issues: string[] = []; | |
| // Check cache utilization | |
| const utilizationRate = (cacheSize / lazyConfig.cache.maxEntries) * 100; | |
| if (utilizationRate > 90) { | |
| issues.push(`Cache utilization high: ${utilizationRate.toFixed(1)}%`); | |
| } | |
| // Check hit rate | |
| const totalRequests = cacheStats.hits + cacheStats.misses; | |
| const hitRate = totalRequests > 0 ? cacheStats.hitRate : 0; | |
| // Only warn about low hit rate if we've had enough requests | |
| if (totalRequests > 100 && hitRate < 50) { | |
| issues.push(`Low cache hit rate: ${hitRate.toFixed(1)}%`); | |
| } | |
| // Check eviction rate | |
| if (cacheStats.evictions > 100) { | |
| issues.push(`High eviction count: ${cacheStats.evictions}`); | |
| } | |
| return { | |
| healthy: issues.length === 0, | |
| enabled: lazyConfig.enabled, | |
| cache: { | |
| size: cacheSize, | |
| maxEntries: lazyConfig.cache.maxEntries, | |
| utilizationRate, | |
| }, | |
| stats: { | |
| hitRate, | |
| coalescedRequests: cacheStats.coalesced, | |
| evictions: cacheStats.evictions, | |
| }, | |
| issues, | |
| }; | |
| } | |
| public getHealthStatus(): { | |
| healthy: boolean; | |
| enabled: boolean; | |
| cache: { | |
| size: number; | |
| maxEntries: number; | |
| utilizationRate: number; | |
| }; | |
| stats: { | |
| hitRate: number; | |
| coalescedRequests: number; | |
| evictions: number; | |
| }; | |
| issues: string[]; | |
| } { | |
| const lazyConfig = this.config.get('lazyLoading'); | |
| const cacheStats = this.schemaCache.getStats(); | |
| const cacheSize = this.schemaCache.size(); | |
| const issues: string[] = []; | |
| // Check cache utilization | |
| const maxEntries = lazyConfig.cache.maxEntries; | |
| const utilizationRate = maxEntries > 0 ? (cacheSize / maxEntries) * 100 : 0; | |
| if (utilizationRate > 90) { | |
| issues.push(`Cache utilization high: ${utilizationRate.toFixed(1)}%`); | |
| } | |
| // Check hit rate | |
| const totalRequests = cacheStats.hits + cacheStats.misses; | |
| const hitRate = totalRequests > 0 ? cacheStats.hitRate : 0; | |
| // Only warn about low hit rate if we've had enough requests | |
| if (totalRequests > 100 && hitRate < 50) { | |
| issues.push(`Low cache hit rate: ${hitRate.toFixed(1)}%`); | |
| } | |
| // Check eviction rate | |
| if (cacheStats.evictions > 100) { | |
| issues.push(`High eviction count: ${cacheStats.evictions}`); | |
| } | |
| return { | |
| healthy: issues.length === 0, | |
| enabled: lazyConfig.enabled, | |
| cache: { | |
| size: cacheSize, | |
| maxEntries: lazyConfig.cache.maxEntries, | |
| utilizationRate, | |
| }, | |
| stats: { | |
| hitRate, | |
| coalescedRequests: cacheStats.coalesced, | |
| evictions: cacheStats.evictions, | |
| }, | |
| issues, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/core/capabilities/lazyLoadingOrchestrator.ts` around lines 568 - 623, In
getHealthStatus, guard against lazyConfig.cache.maxEntries being zero before
computing utilizationRate: if maxEntries is 0 (or falsy) set utilizationRate to
0 (or a safe sentinel) and avoid dividing by zero; update the health output to
reflect this safe value and keep checks that push issues (e.g., Cache
utilization high) using the guarded utilizationRate; reference
lazyConfig.cache.maxEntries, utilizationRate, schemaCache.getStats(), and
schemaCache.size() to locate the code to change.
Comment on lines
+224
to
+244
| private async listAvailableTools(args: ListAvailableToolsArgs): Promise<ListToolsResult> { | ||
| try { | ||
| const registry = this.toolRegistry(); | ||
| const result = registry.listTools(args); | ||
| const servers = registry.getServers(); | ||
|
|
||
| // Format tools for response | ||
| const tools = result.tools.map((tool: ToolMetadata) => ({ | ||
| name: tool.name, | ||
| server: tool.server, | ||
| description: tool.description, | ||
| tags: tool.tags, | ||
| })); | ||
|
|
||
| // Return structured result matching outputSchema | ||
| const response: ListToolsResult = { | ||
| tools, | ||
| totalCount: result.totalCount, | ||
| servers, | ||
| hasMore: result.hasMore, | ||
| ...(result.nextCursor ? { nextCursor: result.nextCursor } : {}), |
There was a problem hiding this comment.
Use the filtered server list from listTools to keep output consistent.
registry.getServers() ignores per-call filters (server/tag/pattern). Use result.servers so servers aligns with the returned tools.
✅ Suggested fix
- const servers = registry.getServers();
+ const servers = result.servers;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private async listAvailableTools(args: ListAvailableToolsArgs): Promise<ListToolsResult> { | |
| try { | |
| const registry = this.toolRegistry(); | |
| const result = registry.listTools(args); | |
| const servers = registry.getServers(); | |
| // Format tools for response | |
| const tools = result.tools.map((tool: ToolMetadata) => ({ | |
| name: tool.name, | |
| server: tool.server, | |
| description: tool.description, | |
| tags: tool.tags, | |
| })); | |
| // Return structured result matching outputSchema | |
| const response: ListToolsResult = { | |
| tools, | |
| totalCount: result.totalCount, | |
| servers, | |
| hasMore: result.hasMore, | |
| ...(result.nextCursor ? { nextCursor: result.nextCursor } : {}), | |
| private async listAvailableTools(args: ListAvailableToolsArgs): Promise<ListToolsResult> { | |
| try { | |
| const registry = this.toolRegistry(); | |
| const result = registry.listTools(args); | |
| const servers = result.servers; | |
| // Format tools for response | |
| const tools = result.tools.map((tool: ToolMetadata) => ({ | |
| name: tool.name, | |
| server: tool.server, | |
| description: tool.description, | |
| tags: tool.tags, | |
| })); | |
| // Return structured result matching outputSchema | |
| const response: ListToolsResult = { | |
| tools, | |
| totalCount: result.totalCount, | |
| servers, | |
| hasMore: result.hasMore, | |
| ...(result.nextCursor ? { nextCursor: result.nextCursor } : {}), |
🤖 Prompt for AI Agents
In `@src/core/capabilities/metaToolProvider.ts` around lines 224 - 244, In
listAvailableTools, the code uses registry.getServers() which ignores per-call
filters; replace that with the filtered servers returned by result (use
result.servers) so the servers output aligns with the returned tools; update the
servers variable in the ListToolsResult construction within the
listAvailableTools method to reference result.servers instead of calling
registry.getServers().
Comment on lines
+176
to
+194
| if (options.cursor) { | ||
| const decoded = ToolRegistry.decodeCursor(options.cursor); | ||
| offset = decoded.offset; | ||
| } | ||
|
|
||
| const DEFAULT_PAGE_SIZE = 20; | ||
| const limit = options.limit ? Math.min(options.limit, 5000) : DEFAULT_PAGE_SIZE; | ||
| const paginated = filtered.slice(offset, offset + limit); | ||
| const hasMore = offset + limit < filtered.length; | ||
|
|
||
| // Encode next cursor | ||
| let nextCursor: string | undefined; | ||
| if (hasMore) { | ||
| nextCursor = ToolRegistry.encodeCursor({ | ||
| offset: offset + limit, | ||
| server: options.server, | ||
| pattern: options.pattern, | ||
| tag: options.tag, | ||
| }); |
There was a problem hiding this comment.
Validate that the cursor matches current filters.
The cursor encodes server/pattern/tag, but only offset is used. If a client reuses a cursor with different filters, pagination becomes inconsistent.
🧭 Suggested guard
let offset = 0;
if (options.cursor) {
const decoded = ToolRegistry.decodeCursor(options.cursor);
- offset = decoded.offset;
+ const matchesFilters =
+ decoded.server === options.server &&
+ decoded.pattern === options.pattern &&
+ decoded.tag === options.tag;
+ offset = matchesFilters ? decoded.offset : 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (options.cursor) { | |
| const decoded = ToolRegistry.decodeCursor(options.cursor); | |
| offset = decoded.offset; | |
| } | |
| const DEFAULT_PAGE_SIZE = 20; | |
| const limit = options.limit ? Math.min(options.limit, 5000) : DEFAULT_PAGE_SIZE; | |
| const paginated = filtered.slice(offset, offset + limit); | |
| const hasMore = offset + limit < filtered.length; | |
| // Encode next cursor | |
| let nextCursor: string | undefined; | |
| if (hasMore) { | |
| nextCursor = ToolRegistry.encodeCursor({ | |
| offset: offset + limit, | |
| server: options.server, | |
| pattern: options.pattern, | |
| tag: options.tag, | |
| }); | |
| if (options.cursor) { | |
| const decoded = ToolRegistry.decodeCursor(options.cursor); | |
| const matchesFilters = | |
| decoded.server === options.server && | |
| decoded.pattern === options.pattern && | |
| decoded.tag === options.tag; | |
| offset = matchesFilters ? decoded.offset : 0; | |
| } | |
| const DEFAULT_PAGE_SIZE = 20; | |
| const limit = options.limit ? Math.min(options.limit, 5000) : DEFAULT_PAGE_SIZE; | |
| const paginated = filtered.slice(offset, offset + limit); | |
| const hasMore = offset + limit < filtered.length; | |
| // Encode next cursor | |
| let nextCursor: string | undefined; | |
| if (hasMore) { | |
| nextCursor = ToolRegistry.encodeCursor({ | |
| offset: offset + limit, | |
| server: options.server, | |
| pattern: options.pattern, | |
| tag: options.tag, | |
| }); |
🤖 Prompt for AI Agents
In `@src/core/capabilities/toolRegistry.ts` around lines 176 - 194, The cursor
handling in ToolRegistry.decodeCursor currently only uses decoded.offset but
ignores decoded.server/ pattern/tag, allowing clients to reuse a cursor with
different filters; update the logic in the pagination block (around
ToolRegistry.decodeCursor, offset, filtered, and ToolRegistry.encodeCursor) to
validate that the decoded cursor's server, pattern, and tag match the current
options.server/options.pattern/options.tag and if they differ either
reject/ignore the cursor (reset offset to 0) or surface an error; ensure
nextCursor continues to encode the correct current filters when calling
ToolRegistry.encodeCursor so pagination remains consistent with the active
filters.
Comment on lines
+181
to
+183
| const DEFAULT_PAGE_SIZE = 20; | ||
| const limit = options.limit ? Math.min(options.limit, 5000) : DEFAULT_PAGE_SIZE; | ||
| const paginated = filtered.slice(offset, offset + limit); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/core/capabilities/toolRegistry.ts | sed -n '175,190p'Repository: 1mcp-app/agent
Length of output: 718
🏁 Script executed:
# Let's also check if there are any tests for this function
fd -t f '\.(test|spec)\.' | xargs rg -l "toolRegistry" | head -5Repository: 1mcp-app/agent
Length of output: 211
🏁 Script executed:
# Check the context and usage of this code more broadly
rg -A 10 -B 10 "DEFAULT_PAGE_SIZE" src/core/capabilities/toolRegistry.tsRepository: 1mcp-app/agent
Length of output: 738
🏁 Script executed:
cat -n src/core/capabilities/toolRegistry.test.ts | grep -A 30 -B 5 "limit"Repository: 1mcp-app/agent
Length of output: 6126
🏁 Script executed:
# Also check what options.limit type is and how it's validated before reaching this code
ast-grep --pattern 'interface $_Options {
$$$
limit
$$$
}'Repository: 1mcp-app/agent
Length of output: 40
🏁 Script executed:
# Look for the function containing this code to understand the full context
rg -B 30 "const DEFAULT_PAGE_SIZE = 20" src/core/capabilities/toolRegistry.ts | head -50Repository: 1mcp-app/agent
Length of output: 1025
Clamp limit to a sane minimum.
Line 182 uses falsy evaluation: 0 is treated as falsy and defaults to 20, even if explicitly provided. Negative values also pass through unchecked—for example, limit: -5 becomes Math.min(-5, 5000) = -5, producing unexpected slice behavior. The suggested fix using the nullish coalescing operator (??) and Math.max ensures valid bounds:
🛠️ Suggested fix
- const DEFAULT_PAGE_SIZE = 20;
- const limit = options.limit ? Math.min(options.limit, 5000) : DEFAULT_PAGE_SIZE;
+ const DEFAULT_PAGE_SIZE = 20;
+ const rawLimit = options.limit ?? DEFAULT_PAGE_SIZE;
+ const limit = Math.min(Math.max(rawLimit, 1), 5000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const DEFAULT_PAGE_SIZE = 20; | |
| const limit = options.limit ? Math.min(options.limit, 5000) : DEFAULT_PAGE_SIZE; | |
| const paginated = filtered.slice(offset, offset + limit); | |
| const DEFAULT_PAGE_SIZE = 20; | |
| const rawLimit = options.limit ?? DEFAULT_PAGE_SIZE; | |
| const limit = Math.min(Math.max(rawLimit, 1), 5000); | |
| const paginated = filtered.slice(offset, offset + limit); |
🤖 Prompt for AI Agents
In `@src/core/capabilities/toolRegistry.ts` around lines 181 - 183, The current
limit computation treats 0 as falsy and allows negative values, causing
unexpected slice ranges; update the logic around DEFAULT_PAGE_SIZE and limit so
you use a nullish check on options.limit (options.limit ?? DEFAULT_PAGE_SIZE)
and clamp the result with Math.max to enforce a minimum of DEFAULT_PAGE_SIZE
(and still Math.min with 5000) before calling filtered.slice(offset, offset +
limit); adjust the variable named limit that is derived from options.limit and
ensure filtered.slice uses this validated positive bounded value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
LazyLoadingOrchestrator,ToolRegistry, andSchemaCachefor on-demand tool schema fetchingtool_list,tool_schema,tool_invoke) that allow clients to discover and invoke tools without loading all schemas upfrontTest plan
pnpm test:unit- all unit tests passpnpm test:e2e- all E2E tests pass including new lazy loading tests--lazyflag on serve commandSummary by CodeRabbit
Release Notes
New Features
Performance Improvements
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.