feat: Implement programmatic SDK-mode MCP access via DirectClient and platform-aware formatting #223
Conversation
frontegg-david
changed the title
📝 WalkthroughWalkthroughAdds a DirectClient implementation and connect utilities with LLM-platform-aware tool/result formatting, scoped connect helpers (OpenAI/Claude/LangChain/Vercel), new skills MCP handlers and types, an auth-introspection demo tool, extensive unit and e2e tests, and comprehensive docs/READMEs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Direct as DirectClientImpl
participant Transport as In-Memory Transport
participant MCP as MCP Server
App->>Direct: connect(config, options)
Direct->>Direct: getScope(config)
Direct->>Transport: createTransport(sessionId, authToken)
Transport->>MCP: open connection / init session
MCP->>MCP: handshake -> version & capabilities
MCP-->>Transport: ServerCapabilities
Transport-->>Direct: MCP client ready
Direct-->>App: DirectClient instance
App->>Direct: callTool(name, args)
Direct->>MCP: callTool request (session context)
MCP->>MCP: execute tool
MCP-->>Direct: CallToolResult
Direct->>Direct: formatResultForPlatform(result, platform)
Direct-->>App: formatted result
sequenceDiagram
participant MCPExec as MCP Tool Execution
participant Formatter as Platform Formatter
participant LLM as LLM Client
MCPExec->>Formatter: formatResultForPlatform(result, 'openai')
alt OpenAI
Formatter->>Formatter: extract text / parse JSON / concat
Formatter-->>LLM: string or parsed JSON
else Claude
Formatter->>Formatter: preserve content array and media
Formatter-->>LLM: content array
else VercelAI
Formatter->>Formatter: parse JSON from text, structure text+images
Formatter-->>LLM: structured object
else Raw
Formatter-->>LLM: passthrough full CallToolResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docs/draft/docs/deployment/direct-client.mdx`:
- Around line 260-273: The example declares `const result` twice causing a
TypeScript syntax error; update the second usage of `result` in the second
client.complete call (the one with ref: { type: 'ref/resource' }) to a different
variable name (e.g., `resultResource`) or split the examples into separate code
blocks so each has its own scope, ensuring references to `client.complete` and
any subsequent uses refer to the renamed variable.
In `@libs/sdk/src/direct/__tests__/connect.spec.ts`:
- Around line 201-216: The test is using the wrong capabilities shape; update
the call to connect in the spec so it passes a capabilities object matching
ClientCapabilities (use roots not tools). In the test around the connect
invocation (the it block that imports connect and Client), change the passed
capabilities from { tools: { listChanged: true } } to { roots: { listChanged:
true } } so the expect(Client).toHaveBeenCalledWith(...,
expect.objectContaining({ capabilities: { roots: { listChanged: true } } }))
matches the defined interface.
In `@libs/sdk/src/direct/connect.ts`:
- Around line 33-49: The cached scopePromise must not remain rejected: update
the block that sets scopePromise (using scopeCache, cacheKey, and the async
initializer that calls FrontMcpInstance.createForGraph and returns Scope) to
remove the cache entry if the async initializer throws, and rethrow an
MCP-specific error (use the project MCP error class and set the appropriate
error code) instead of a generic Error; specifically catch errors from
createForGraph/getScopes, call scopeCache.delete(cacheKey) in the catch, wrap or
convert the original error into the MCP error type with the right code, and then
throw that MCP error so subsequent connect() calls can retry.
In `@libs/sdk/src/direct/direct-client.ts`:
- Around line 135-140: Replace the generic throws for missing
serverInfo/serverCapabilities with the SDK's MCP error class (e.g., MCPError)
and include the appropriate MCP error code/enum (e.g.,
MCPErrorCode.HandshakeFailed or the project's equivalent) and context.
Specifically, in the handshake handling block where serverInfo and
serverCapabilities are checked (the code referencing serverInfo and
serverCapabilities in direct-client.ts), throw the MCP-specific error class with
a descriptive message and the correct error code rather than new Error(...).
Ensure you import the MCP error class/enum and propagate the same error shape
used across the SDK.
- Around line 212-225: The methods listTools and callTool currently return
unknown; update their signatures to return platform-specific types (e.g.,
FormattedTools for listTools and FormattedToolResult for callTool) and import
those types from client.types.ts, then ensure
formatToolsForPlatform(result.tools, this.platform) and
formatResultForPlatform(result, this.platform) are typed to produce those return
types (adjust their typings if needed) so the SDK returns explicit typed outputs
instead of unknown.
- Around line 162-185: The setupNotificationHandlers block incorrectly checks
mcpClient.onnotification (which doesn't exist) so handlers never install;
replace this pattern by calling the MCP SDK's setNotificationHandler on
mcpClient for the relevant notification schemas: register a handler for the
resources update notification that extracts uri and invokes
this.resourceUpdateHandlers.forEach(h => h(uri)), and register a handler for the
elicitation/request notification that casts params to ElicitationRequest and
calls this.handleElicitationRequest(params); ensure you use
mcpClient.setNotificationHandler(schema, handler) with the correct schema
identifiers and remove the unreachable onnotification code path.
In `@libs/sdk/src/transport/mcp-handlers/skills-list-request.handler.ts`:
- Around line 59-62: Replace the generic throw new Error in the
skills-list-request handler with the project-specific MCP error class (e.g.
MCPError): import the MCP error class used across other handlers, then throw a
new MCPError when scope.skills is missing (use the same message "Skills
capability not available"), include an MCP-specific error code (e.g.
'MissingCapability' or the project's standard code) and any relevant metadata
(handler name or scope info) so the error is consistent with other handlers like
SkillsListRequestHandler and can be recognized by MCP error handling.
In `@libs/sdk/src/transport/mcp-handlers/skills-load-request.handler.ts`:
- Around line 92-94: Replace the generic throw in the skills-load handler with
the MCP-specific error class: where the code checks "if (!skillRegistry) { throw
new Error('Skills capability not available'); }", import and throw the MCP error
(e.g., McpError) with the appropriate MCP error code/enum (e.g.,
McpErrorCode.CAPABILITY_NOT_AVAILABLE or similar) and a clear message; update
the imports at the top of skills-load-request.handler.ts to include the MCP
error class and code/enum and use them in place of the generic Error so MCP
handlers can handle the failure consistently.
In `@libs/sdk/src/transport/mcp-handlers/skills-search-request.handler.ts`:
- Around line 59-62: Replace the generic throw new Error in
skills-search-request.handler.ts when scope.skills is missing with the project’s
MCP error class (e.g., McpError/MCPError) used by other handlers: import the MCP
error class at the top of the file, and throw a new instance (for example new
McpError('CapabilityNotAvailable', 'Skills capability not available')) instead
of throw new Error('Skills capability not available') so the handler uses the
standardized MCP error type and code.
🧹 Nitpick comments (9)
libs/sdk/src/transport/mcp-handlers/skills-load-request.handler.ts (1)
129-137: Move tool lookup map construction outside the skill loop.The
toolsByNamemap is rebuilt for every skill inskillIds, buttoolRegistry.getTools()returns the same result each time. Moving this outside the loop avoids redundant work.Proposed fix
+ // Build a lookup map from tool registry if available (once, outside loop) + const toolsByName = new Map<string, { getInputJsonSchema: () => unknown }>(); + if (toolRegistry && format !== 'instructions-only') { + const allTools = toolRegistry.getTools(false); + for (const tool of allTools) { + toolsByName.set(tool.name, tool); + } + } + for (const skillId of skillIds) { // ... existing loadResult logic ... - // Get tool schemas if full format is requested - // Build a lookup map from tool registry if available - const toolsByName = new Map<string, { getInputJsonSchema: () => unknown }>(); - if (toolRegistry && format !== 'instructions-only') { - const allTools = toolRegistry.getTools(false); - for (const tool of allTools) { - toolsByName.set(tool.name, tool); - } - } const toolsWithSchemas = skill.tools.map((t) => {libs/sdk/src/transport/mcp-handlers/__tests__/skills-handlers.spec.ts (1)
161-192: Consider adding test forformat: 'instructions-only'option.The
skillsLoadRequestHandlersupports aformatparameter that affects whether tool schemas are included. Adding a test case to verify the behavior difference between'full'and'instructions-only'formats would improve coverage.Suggested additional test case
it('should not include tool schemas when format is instructions-only', async () => { mockSkillRegistry.loadSkill.mockResolvedValueOnce({ skill: { id: 'skill-1', name: 'Test Skill', description: 'A test skill', instructions: 'Do the thing', tools: [{ name: 'tool1', purpose: 'For doing' }], }, availableTools: ['tool1'], missingTools: [], isComplete: true, warning: undefined, }); const handler = skillsLoadRequestHandler(createHandlerOptions()); const request = { method: 'skills/load' as const, params: { skillIds: ['skill-1'], format: 'instructions-only' as const }, }; const ctx = createContext(); const result = await handler.handler(request, ctx as any); expect(result.skills[0].tools[0].inputSchema).toBeUndefined(); });libs/sdk/src/direct/client.types.ts (1)
38-47: Consider adding validation bounds documentation forlimit.The comment states "Maximum number of results (1-50, default: 10)" but this is purely documentation. Consider whether runtime validation of these bounds is handled elsewhere in the implementation.
apps/e2e/demo-e2e-direct/src/apps/notes/tools/get-auth-info.tool.ts (1)
43-55: Consider adding type safety for authInfo.user access.The
as anycast works for E2E testing but could mask type mismatches. If theauthInfostructure from DirectClient is stable, consider defining a type interface or using a type guard.💡 Suggested improvement
+interface DirectAuthInfo { + token?: string; + user?: { + sub?: string; + name?: string; + email?: string; + [key: string]: unknown; + }; +} + const authInfo = ctx.authInfo; -// User info is stored directly in authInfo (from DirectClient), not in extra -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -const user = (authInfo as any)?.user as Record<string, unknown> | undefined; +const user = (authInfo as DirectAuthInfo | undefined)?.user;libs/sdk/src/direct/__tests__/direct-client.spec.ts (1)
581-641: Consider adding a test for elicitation handler invocation.The current tests verify that
onElicitationreturns an unsubscribe function and thatsubmitElicitationResultcalls the MCP client correctly. However, there's no test verifying that a registered handler is actually invoked when an elicitation notification arrives.libs/sdk/src/direct/llm-platform.ts (2)
204-229: Return type doesn't reflect JSON parsing behavior.The function signature says
stringbut line 225 returnsJSON.parse(combined)which could be any type. This works because the return type is later handled asunknowninformatResultForPlatform, but the local type annotation is misleading.💡 Suggested fix
-function extractTextContent(result: CallToolResult): string { +function extractTextContent(result: CallToolResult): unknown {
112-138: Add sanitization for array item schemas to ensure complete OpenAI strict mode compliance.OpenAI's strict mode requires
additionalProperties: falseon every object schema node, including objects nested in arrayitems. The function currently handles nested objects inpropertiesbut skips theitemsfield, which means array parameters with object items would fail OpenAI validation.Suggested enhancement
function sanitizeSchemaForOpenAI(schema: Record<string, unknown>): Record<string, unknown> { const result = { ...schema }; if (result['type'] === 'object' && !('additionalProperties' in result)) { result['additionalProperties'] = false; } + // Handle array items + if (result['items'] && typeof result['items'] === 'object') { + result['items'] = sanitizeSchemaForOpenAI(result['items'] as Record<string, unknown>); + } + // Recursively handle nested objects in properties if (result['properties'] && typeof result['properties'] === 'object') { // ... existing code } return result; }libs/sdk/src/direct/connect.ts (1)
19-21: MakeclearScopeCache()actually reset the cache for tests.Right now it’s a no-op; tests that call it won’t actually clear cached scopes. Reassigning the WeakMap makes the helper match its name and intent.
♻️ Suggested change
-const scopeCache = new WeakMap<object, Promise<Scope>>(); +let scopeCache = new WeakMap<object, Promise<Scope>>(); @@ export function clearScopeCache(): void { - // WeakMap doesn't have a clear() method, but since it's weak-referenced, - // entries will be GC'd when config objects are no longer referenced. - // For testing, callers should ensure they don't reuse config objects. + scopeCache = new WeakMap<object, Promise<Scope>>(); }Also applies to: 269-273
libs/sdk/src/direct/direct-client.ts (1)
286-289: Ensure server cleanup even ifmcpClient.close()throws.A
try/finallyensures the in-memory server is always closed.♻️ Suggested change
async close(): Promise<void> { - await this.mcpClient.close(); - await this.closeServer?.(); + try { + await this.mcpClient.close(); + } finally { + await this.closeServer?.(); + } }
…ove response validation
frontegg-david
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/direct/__tests__/direct-client.spec.ts`:
- Around line 250-260: The test uses an incorrect capabilities shape; update the
object passed to DirectClientImpl.create and the MockClient expectation to match
MCP SDK ClientCapabilities (e.g., move listChanged under roots). Replace the
capabilities arg currently `{ capabilities: { tools: { listChanged: true } } }`
with a valid structure such as `{ capabilities: { roots: { listChanged: true } }
}` and assert MockClient was called with that same structure (references:
DirectClientImpl.create and MockClient).
🧹 Nitpick comments (8)
libs/sdk/src/direct/llm-platform.ts (1)
136-167: Consider handling additional JSON Schema composition keywords.The
sanitizeSchemaForOpenAIfunction recursively processespropertiesanditems, but JSON Schemas may also contain nested schemas underallOf,oneOf,anyOf, or$defs. If tools use these patterns, theadditionalProperties: falseconstraint won't be applied to nested object schemas within them.This is a minor gap—most simple tool schemas won't use these keywords, but it's worth noting for future robustness.
♻️ Optional enhancement to handle composition keywords
function sanitizeSchemaForOpenAI(schema: Record<string, unknown>): Record<string, unknown> { const result = { ...schema }; if (result['type'] === 'object' && !('additionalProperties' in result)) { result['additionalProperties'] = false; } if (result['properties'] && typeof result['properties'] === 'object') { const properties = result['properties'] as Record<string, Record<string, unknown>>; const sanitizedProperties: Record<string, Record<string, unknown>> = {}; for (const [key, value] of Object.entries(properties)) { if (value && typeof value === 'object') { sanitizedProperties[key] = sanitizeSchemaForOpenAI(value); } else { sanitizedProperties[key] = value; } } result['properties'] = sanitizedProperties; } if (result['items'] && typeof result['items'] === 'object') { result['items'] = sanitizeSchemaForOpenAI(result['items'] as Record<string, unknown>); } + // Handle composition keywords + for (const keyword of ['allOf', 'oneOf', 'anyOf'] as const) { + if (Array.isArray(result[keyword])) { + result[keyword] = (result[keyword] as Record<string, unknown>[]).map((s) => + s && typeof s === 'object' ? sanitizeSchemaForOpenAI(s) : s + ); + } + } return result; }libs/sdk/src/direct/direct-client.ts (1)
304-341: Consider documenting the server-side validation pattern.The
{} as anycasts for skills operations bypass client-side schema validation. While the inline comment notes "Schema validation happens server-side," consider adding a brief JSDoc note on these methods to clarify this design choice for future maintainers.📝 Optional: Add JSDoc clarification
+ /** + * Search for skills matching a query. + * + * Note: Schema validation is performed server-side; the client sends raw params. + */ async searchSkills(query: string, options?: SearchSkillsOptions): Promise<SearchSkillsResult> {libs/sdk/src/transport/mcp-handlers/skills-load-request.handler.ts (2)
77-100: Consider extracting inline type to improve readability.The inline type definition for
toolInfois verbose. Consider defining a separate interface or type alias at the module level for clarity and reusability.♻️ Suggested refactor
+interface ToolInfoEntry { + name: string; + purpose?: string; + available: boolean; + inputSchema?: unknown; + outputSchema?: unknown; +} + // Build tool info with schemas const toolsWithSchemas = skill.tools.map((t) => { const available = availableTools.includes(t.name); - const toolInfo: { - name: string; - purpose?: string; - available: boolean; - inputSchema?: unknown; - outputSchema?: unknown; - } = { + const toolInfo: ToolInfoEntry = { name: t.name, purpose: t.purpose, available, };
118-119: Session activation placeholder noted.The comment indicates session activation is not yet implemented. Consider tracking this as a follow-up task if it's expected functionality.
Would you like me to open an issue to track the implementation of session activation in DirectClient?
libs/sdk/src/direct/__tests__/connect.spec.ts (1)
380-422: Consider adding error case tests for platform formatting.The platform formatting tests verify the happy path but don't test behavior when
listToolsreturns an empty array (already covered) or when tool formatting fails. Consider adding tests for edge cases like malformed tool schemas.libs/sdk/src/direct/__tests__/direct-client.spec.ts (2)
684-686: Avoid hardcoded delays in tests.Using
setTimeout(resolve, 10)for async handler completion can cause flaky tests. Consider usingjest.useFakeTimers()withjest.advanceTimersByTime(), or await a specific condition/promise instead.♻️ Suggested approach
- // Wait for async handler to complete - await new Promise((resolve) => setTimeout(resolve, 10)); + // Wait for async handler to complete using fake timers + await jest.runAllTimersAsync();Or alternatively, refactor to await the actual async operation completing by capturing and awaiting the promise from the notification handler.
604-612: Improve unsubscribe verification.The test comment says "tested indirectly" but doesn't actually verify the handler is cleared. Consider triggering a notification after unsubscribe and verifying the handler is not called.
libs/sdk/src/transport/mcp-handlers/skills-mcp.types.ts (1)
43-45: Index signature pattern for extensibility.The
& { [key: string]: unknown }pattern allows additional fields in responses, which is useful for forward compatibility. However, this weakens type safety for consumers. Ensure this is intentional and document the extensibility purpose.
…tification management
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@libs/sdk/src/direct/direct-client.ts`:
- Around line 96-157: The create method currently opens the in-memory server via
createInMemoryServer and then calls mcpClient.connect and handshake methods
without ensuring close() runs on error; update DirectClient.create to wrap the
connect + handshake (the await mcpClient.connect(...) and subsequent
getServerVersion/getServerCapabilities checks) in a try/catch that calls the
close() function if any error or missing server info occurs, rethrowing the
original error (or the PublicMcpError) after cleanup so the in-memory server is
not leaked.
In `@libs/sdk/src/direct/llm-platform.ts`:
- Around line 132-182: sanitizeSchemaForOpenAI currently only adds
additionalProperties: false when the object schema lacks that key, but it must
enforce additionalProperties: false for every object schema (including when
additionalProperties is explicitly true). Update sanitizeSchemaForOpenAI so that
whenever result['type'] === 'object' you set result['additionalProperties'] =
false unconditionally (instead of checking with in), and ensure this enforcement
occurs for nested object schemas processed in properties, items, and composition
branches by letting recursive calls return schemas with additionalProperties
forced to false; reference the sanitizeSchemaForOpenAI function and the
result['type'] / result['additionalProperties'] handling to locate the change.
- Around line 108-127: FormattedToolResult uses `unknown`; define a local
JSON-serializable type (e.g., JsonValue / JsonObject / JsonArray) and replace
`unknown` in the `FormattedToolResult` union with that `JsonValue` so the SDK
API is explicit about allowed return shapes. Add the `JsonValue` type
declaration near the `FormattedToolResult` type and update the union variant to
use `JsonValue` (keep existing variants like `string`, `CallToolResult`, and the
image object intact).
🧹 Nitpick comments (4)
libs/sdk/src/transport/mcp-handlers/skills-load-request.handler.ts (2)
83-86: Consider aligning formatter selection with theformatparameter.The formatter selection uses
toolRegistryexistence, butformatSkillForLLMWithSchemasandformatSkillForLLMmay produce different output regardless of whether schemas are actually needed. For maintainability, consider making the selection logic more explicit about when schema-enriched formatting is desired.That said, the current implementation is functionally correct since
formatSkillForLLMWithSchemasreceivestoolRegistrydirectly and can handle any format internally.
147-148: Remove redundant type assertion.
SkillsLoadResultSchema.parse()returns the Zod-inferred type, making theas SkillsLoadResultcast unnecessary. SinceSkillsLoadResultis defined asz.infer<typeof SkillsLoadResultSchema>, Zod's parse output should already match.Suggested simplification
// Validate result against schema - return SkillsLoadResultSchema.parse(result) as SkillsLoadResult; + return SkillsLoadResultSchema.parse(result);libs/sdk/src/direct/direct-client.ts (2)
54-85: Prefer a typed MCP client instead ofany.
anyhides SDK API drift and violates the project TS guidelines. Consider a type-only import for the MCP Client and use that type for the field and constructor. As per coding guidelines, ...♻️ Suggested typing update
+import type { Client as McpClient } from '@modelcontextprotocol/sdk/client/index.js'; export class DirectClientImpl implements DirectClient { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - private readonly mcpClient: any; + private readonly mcpClient: McpClient; ... private constructor( - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - mcpClient: any, + mcpClient: McpClient, sessionId: string, clientInfo: ClientInfo, serverInfo: Implementation, capabilities: ServerCapabilities, ) {
326-395: Replace{} as anyschema placeholders with a typed schema.The
client.requestschema parameter can use a safe placeholder likez.any()to avoidanywhile still delegating validation to the server. As per coding guidelines, ...♻️ Example pattern
+import { z } from 'zod'; +const AnySchema = z.any(); return this.mcpClient.request( { method: 'skills/search', params: { query, ...options, }, }, - {} as any, + AnySchema, );Apply the same pattern to
skills/load,skills/list, andelicitation/result.Also applies to: 408-419
…ally for improved performance
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/direct/direct-client.ts`:
- Around line 109-114: The authInfo['user'] object currently spreads
options?.session?.user after setting sub, which means a missing user.sub can
override the default; update the construction so the spread does not overwrite
the fallback—either spread options.session.user first and then set sub:
options.session.user?.sub ?? 'direct', or keep the spread last but explicitly
set sub to options.session.user?.sub ?? 'direct' (e.g., set sub after the
spread). Target the code that builds authInfo['user'] (the
options?.session?.user handling) and ensure sub uses the nullish-coalescing
fallback after the spread so 'direct' is preserved when user.sub is undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/direct/__tests__/direct-client.spec.ts`:
- Around line 27-35: The test mock for createInMemoryServer uses the
extensionless specifier which doesn't match the production dynamic import;
update the jest.mock specifier to '../../transport/in-memory-server.js' so it
aligns with the import used by DirectClientImpl and ensure all other test
imports/mocks of '../../transport/in-memory-server' (at the noted occurrences)
are changed to include the '.js' suffix; keep the mocked export names
(createInMemoryServer, clientTransport, setAuthInfo, close) unchanged so the
mock continues to satisfy DirectClientImpl's expectations.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.