feat: Add support for elicitation #218
Conversation
… agent and tool execution
…or interactive user input
…or interactive user input
📝 WalkthroughWalkthroughAdds end-to-end elicitation: types, errors, helpers, flows, transport adapters, distributed encrypted stores, tool/agent elicit APIs and fallback tooling, testing client hooks, demo app + e2e tests, docs, and build/test config for elicitation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Transport
participant Store
participant ToolContext
Client->>Server: initialize (capabilities incl. elicitation?)
Server->>Store: ensure ElicitationStore created (if enabled)
Client->>Server: call tool
Server->>ToolContext: execute tool
ToolContext->>ToolContext: elicit(message, schema, options)
alt Client supports elicitation
ToolContext->>Transport: sendElicitRequest(relatedRequestId, message, schema, options)
Transport->>Client: elicitation/create (form/url)
Client->>User: render UI
User->>Client: submit result
Client->>Transport: elicitation/result POST
Transport->>Store: publishResult(elicitId, sessionId, result)
Store->>Transport: notify subscriber -> resolves pending promise
Transport->>ToolContext: return ElicitResult
ToolContext->>Server: continue tool execution -> final response
else Client does NOT support elicitation
ToolContext->>ToolContext: throw ElicitationFallbackRequired(elicitId,...)
Server->>Store: setPendingFallback(pendingRecord)
Server->>Client: return response with _meta.elicitationPending
Client->>Server: call SendElicitationResult tool (elicitId/action/content)
Server->>Store: getResolvedResult(elicitId)
Server->>ToolContext: setPreResolvedElicitResult(result) and re-invoke original tool
ToolContext->>Server: continue tool execution -> final response
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (2)
50-107: Info-level response/body logging risks leaking sensitive data.The initialize path logs request metadata and a response body preview at info level. That can leak user content/PII and create noisy logs in production. Consider demoting to verbose/debug and logging length-only or redacting payloads.
🔒 Suggested tightening for log hygiene
- adapter.logger.info('[StreamableHttpAdapter] initialize response', { + adapter.logger.verbose('[StreamableHttpAdapter] initialize response', { statusCode: res.statusCode, headers: res.getHeaders?.() ?? {}, - bodyPreview: responseBody.slice(0, 1000), + bodyLength: responseBody.length, });
129-239: Validate URL-mode options and handle subscription failures.Same concerns as the SSE adapter: URL mode can proceed without
elicitationId, and a failedsubscribeResultleaves the promise pending until timeout. Consider failing fast and catching subscription errors. As per coding guidelines, use a specific MCP error type for invalid input.🐛 Suggested fix (validation + subscription error handling)
+import { InvalidInputError } from '../../errors/mcp.error'; import { ElicitationTimeoutError } from '../../errors'; async sendElicitRequest<S extends ZodType>( relatedRequestId: RequestId, message: string, requestedSchema: S, options?: ElicitOptions, ): Promise<ElicitResult<S extends ZodType<infer O> ? O : unknown>> { const { mode = 'form', ttl = DEFAULT_ELICIT_TTL, elicitationId } = options ?? {}; + + if (mode === 'url' && !elicitationId) { + throw new InvalidInputError('elicitationId is required when mode is "url"'); + } this.elicitStore .subscribeResult<S extends ZodType<infer O> ? O : unknown>(elicitId, (result) => { clearTimeout(timeoutHandle); this.pendingElicit = undefined; unsubscribe?.(); resolve(result); }) .then((unsub) => { unsubscribe = unsub; - }); + }) + .catch((err) => { + clearTimeout(timeoutHandle); + this.pendingElicit = undefined; + unsubscribe?.(); + reject(err); + });
🤖 Fix all issues with AI agents
In `@docs/draft/docs/servers/elicitation.mdx`:
- Around line 392-409: The docs currently say elicitationId is "Auto-generated"
but the ElicitOptions/ elicit() behavior requires callers to provide
elicitationId when mode is 'url' for external callback correlation; update the
text to state that elicitationId is auto-generated for 'form' mode but required
(caller-provided) when using elicit({ mode: 'url', ... }) and adjust the
table/description for elicitationId and the ElicitOptions note to make this
distinction explicit.
In `@libs/sdk/src/elicitation/elicitation.store.ts`:
- Around line 76-80: The documentation example for publishResult is missing the
required sessionId parameter; update the example to call publishResult with
(elicitId, sessionId, result) using the sessionId obtained from the pending
object (e.g., pending.sessionId) after calling getPending; ensure the example
references publishResult and getPending and passes pending.elicitId and
pending.sessionId when invoking publishResult so it matches the function
signature.
In `@libs/sdk/src/elicitation/elicitation.types.ts`:
- Around line 26-40: Update the JSDoc on the ElicitResult<T> interface to use
the correct status terminology: replace occurrences of "submit" with "accept" in
both the generic T description and the content comment so it matches the
ElicitStatus values ('accept' | 'cancel' | 'decline'); ensure the content
comment reads that content is only present when status is 'accept' and, if
needed, mention the valid statuses via ElicitStatus to avoid future confusion.
In `@libs/sdk/src/elicitation/redis-elicitation.store.ts`:
- Around line 89-95: The setPending method computes ttlSeconds which can be 0 or
negative and causes redis.set(key, JSON.stringify(record), 'EX', ttlSeconds) to
error; in setPending (in libs/sdk/src/elicitation/redis-elicitation.store.ts)
check ttlMs/ttlSeconds after computing Math.max(0, record.expiresAt -
Date.now()) and if ttlSeconds <= 0 either delete the existing key via
this.redis.del(key) or simply return early without calling this.redis.set;
ensure you reference PENDING_KEY_PREFIX, PendingElicitRecord, expiresAt and
redis.set when making the change so expired records are not passed to SET EX 0.
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 401-407: Replace the generic throw in the isEdge branch with a
typed MCP error: use the PublicMcpError subclass (e.g.,
ElicitationNotSupportedError) instead of new Error so MCP error codes and
handlers work; update the throw in the isEdge block inside scope.instance.ts
(the branch that currently throws when isEdge is true) to throw new
ElicitationNotSupportedError(...) with the same helpful message and ensure the
error class carries the appropriate MCP error code (or add the subclass
extending PublicMcpError if it doesn't exist).
In `@libs/sdk/src/transport/adapters/__tests__/elicit-handler.test.ts`:
- Around line 236-267: The test "should reject with ElicitationTimeoutError
after TTL expires" currently asserts with toThrow and toMatchObject but is
missing an explicit instanceof check; update the assertions for the promise
(variable promise) to include an instanceof assertion against the
ElicitationTimeoutError class (i.e., await
expect(promise).rejects.toBeInstanceOf(ElicitationTimeoutError)) in addition to
the existing toThrow/toMatchObject checks so the error hierarchy is verified
consistently.
In `@libs/sdk/src/transport/adapters/transport.local.adapter.ts`:
- Around line 253-258: The log in handleIfElicitResult currently outputs
resultPreview which may contain user PII; remove resultPreview from the
logger.info call (in transport.local.adapter.ts) and instead log only
non-sensitive metadata such as hasResult (this is already present) and a safe
metric like resultSize (e.g., length/type) or a redacted placeholder for
req.body?.result to preserve usefulness without exposing payloads; update the
logger.info invocation that references this.pendingElicit and req.body?.result
accordingly.
In `@libs/sdk/src/transport/adapters/transport.sse.adapter.ts`:
- Around line 93-176: sendElicitRequest currently allows a URL-mode request
without an elicitationId and doesn't handle a rejected
elicitStore.subscribeResult promise; validate at the top of sendElicitRequest
(before calling this.elicitStore.setPending and before sending rpcRequest) that
if mode === 'url' then elicitationId is present, and if not throw a specific MCP
error (e.g., InvalidInputError) referencing elicitationId; additionally wrap the
this.elicitStore.subscribeResult(...) call with a .catch handler so subscription
failures clear the timeoutHandle, unset this.pendingElicit, call unsubscribe if
available, delete the pending entry from elicitStore (sessionId), and reject the
Promise with a descriptive error (prefer InvalidInputError or a
SubscriptionError) so the caller fails fast instead of waiting for the ttl; keep
cleanup logic consistent with existing ElicitationTimeoutError and
pendingElicit.resolve/reject handlers.
In `@libs/sdk/src/transport/flows/handle.streamable-http.flow.ts`:
- Around line 204-235: The onElicitResult flow currently only calls
transportService.getTransporter('streamable-http', token, session.id) and treats
missing transports as expired; update onElicitResult to mirror the
session/transport recreation logic used in onMessage by attempting to restore
the session and recreate the transporter from persisted storage (e.g., Redis)
before marking it expired — use the same sequence of calls used for session
restoration (the transportService and any session restore helpers used in
onMessage) to rehydrate the transport, then call
transport.handleRequest(request, response) and fall back to
this.respond(httpRespond.sessionExpired('session not found')) only if recreation
fails; ensure you reference transportService.getTransporter,
transport.handleRequest, this.rawInput.request/response,
this.state.required.session, this.respond and this.handled when applying the
change.
🧹 Nitpick comments (10)
libs/sdk/src/elicitation/__tests__/elicitation.types.test.ts (1)
206-268: Consider usingtry/finallyorjest.useFakeTimers()for timer cleanup.The
setTimeouthandles are cleaned up manually withclearTimeout, but if a test assertion fails before reaching the cleanup, the timer could leak. This is a minor issue since the timers are short-lived.♻️ Optional: Use fake timers for deterministic cleanup
describe('PendingElicit', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + it('should create a pending elicit with all required fields', () => { const resolveFn = jest.fn(); const rejectFn = jest.fn(); const timeoutHandle = setTimeout(() => {}, 1000); const pending: PendingElicit<{ confirmed: boolean }> = { elicitId: 'elicit-abc', timeoutHandle, resolve: resolveFn, reject: rejectFn, }; expect(pending.elicitId).toBe('elicit-abc'); expect(pending.timeoutHandle).toBeDefined(); expect(pending.resolve).toBe(resolveFn); expect(pending.reject).toBe(rejectFn); - - clearTimeout(timeoutHandle); });This approach eliminates the need for manual cleanup and makes tests deterministic.
apps/e2e/demo-e2e-elicitation/tsconfig.app.json (1)
1-14: LGTM!The TypeScript configuration is appropriate for the e2e demo app with proper settings for NodeNext module resolution and decorator support.
Minor note: The
"jsx": "react-jsx"setting may be unnecessary if this e2e app doesn't use JSX/React components. Consider removing it if TSX files aren't actually needed, or keep it if planning to add React-based UI components later.libs/sdk/src/scope/scope.instance.ts (1)
375-417: Avoid the non‑null assertion on_elicitationStore.Prefer a local
storevariable and return it directly to keep the type flow safe without!. As per coding guidelines, avoid non‑null assertions.♻️ Suggested refactor
get elicitationStore(): ElicitationStore { - if (!this._elicitationStore) { - const redis = this.metadata.redis; - const isEdge = this.isEdgeRuntime(); + if (this._elicitationStore) { + return this._elicitationStore; + } + + const redis = this.metadata.redis; + const isEdge = this.isEdgeRuntime(); - // Check if Redis is configured (has host property for Redis provider) - // Note: Vercel KV doesn't support pub/sub, so we only support Redis provider - const hasRedisConfig = redis && 'host' in redis && typeof redis.host === 'string'; + // Check if Redis is configured (has host property for Redis provider) + // Note: Vercel KV doesn't support pub/sub, so we only support Redis provider + const hasRedisConfig = redis && 'host' in redis && typeof redis.host === 'string'; + let store: ElicitationStore; - if (hasRedisConfig) { - // Use Redis store for distributed deployments - // Lazy require to avoid bundling ioredis when not used - const { RedisElicitationStore } = require('../elicitation/redis-elicitation.store'); - const Redis = require('ioredis').default; + if (hasRedisConfig) { + // Use Redis store for distributed deployments + // Lazy require to avoid bundling ioredis when not used + const { RedisElicitationStore } = require('../elicitation/redis-elicitation.store'); + const Redis = require('ioredis').default; - const redisConfig = redis as { host: string; port?: number; password?: string; db?: number; tls?: boolean }; - const redisClient = new Redis({ - host: redisConfig.host, - port: redisConfig.port ?? 6379, - password: redisConfig.password, - db: redisConfig.db ?? 0, - ...(redisConfig.tls && { tls: {} }), - }); + const redisConfig = redis as { host: string; port?: number; password?: string; db?: number; tls?: boolean }; + const redisClient = new Redis({ + host: redisConfig.host, + port: redisConfig.port ?? 6379, + password: redisConfig.password, + db: redisConfig.db ?? 0, + ...(redisConfig.tls && { tls: {} }), + }); - this._elicitationStore = new RedisElicitationStore(redisClient, this.logger); - this.logger.info('Elicitation: using Redis store (distributed mode)'); - } else if (isEdge) { - // Edge runtime requires Redis - throw helpful error - throw new Error( - 'Elicitation requires Redis configuration when running on Edge runtime. ' + - 'Edge functions are stateless and cannot use in-memory elicitation. ' + - 'Configure redis in `@FrontMcp`({ redis: { provider: "redis", host: "...", port: ... } })', - ); - } else { - // Fall back to in-memory store for single-node/dev - this._elicitationStore = new InMemoryElicitationStore(); - this.logger.warn( - 'Elicitation: using in-memory store (single-node mode). ' + 'Configure Redis for distributed deployments.', - ); - } - } - return this._elicitationStore!; + store = new RedisElicitationStore(redisClient, this.logger); + this.logger.info('Elicitation: using Redis store (distributed mode)'); + } else if (isEdge) { + // Edge runtime requires Redis - throw helpful error + throw new Error( + 'Elicitation requires Redis configuration when running on Edge runtime. ' + + 'Edge functions are stateless and cannot use in-memory elicitation. ' + + 'Configure redis in `@FrontMcp`({ redis: { provider: "redis", host: "...", port: ... } })', + ); + } else { + // Fall back to in-memory store for single-node/dev + store = new InMemoryElicitationStore(); + this.logger.warn( + 'Elicitation: using in-memory store (single-node mode). ' + 'Configure Redis for distributed deployments.', + ); + } + + this._elicitationStore = store; + return store; }libs/sdk/src/elicitation/memory-elicitation.store.ts (1)
110-123: Avoid non-null assertions when retrieving listeners.You can initialize the set via a guard to keep strict null safety.
♻️ Suggested refactor
- if (!this.listeners.has(elicitId)) { - this.listeners.set(elicitId, new Set()); - } - - const listeners = this.listeners.get(elicitId)!; + let listeners = this.listeners.get(elicitId); + if (!listeners) { + listeners = new Set(); + this.listeners.set(elicitId, listeners); + }As per coding guidelines, avoid non-null assertions in libs/sdk TypeScript.
libs/sdk/src/elicitation/elicitation.store.ts (1)
83-135: Public API addition: ensure docs/draft updates.This introduces a new public store contract in
libs/sdk; please confirm the matching docs/draft updates land alongside the API surface. As per coding guidelines, ensure public API changes are reflected in docs/draft.libs/sdk/src/elicitation/redis-elicitation.store.ts (1)
150-187: Avoid non-null assertions insubscribeResult.Use explicit guards/local variables so the type system can track readiness without
!.♻️ Suggested refactor
- if (!this.subscriberReady && this.subscriber) { + const subscriber = this.subscriber; + if (!this.subscriberReady && subscriber) { await new Promise<void>((resolve, reject) => { const timeout = setTimeout(() => { reject(new Error('Subscriber connection timeout')); }, 5000); - this.subscriber!.once('ready', () => { + subscriber.once('ready', () => { clearTimeout(timeout); resolve(); }); @@ - await this.subscriber?.subscribe(channel); + await subscriber.subscribe(channel); @@ - this.subscriptions.get(channel)!.add(callback as ElicitResultCallback); + this.subscriptions.get(channel)!.add(callback as ElicitResultCallback);As per coding guidelines, avoid non-null assertions in libs/sdk TypeScript.
libs/sdk/src/common/interfaces/agent.interface.ts (1)
554-558: ConstrainZodTypeto avoid implicitany.The bare
ZodTypeconstraint defaults toanyin Zod. PreferZodType<unknown>to keep strict typing.♻️ Suggested refactor
- protected async elicit<S extends ZodType>( + protected async elicit<S extends ZodType<unknown>>( message: string, requestedSchema: S, options?: ElicitOptions, ): Promise<ElicitResult<S extends ZodType<infer O> ? O : unknown>> {As per coding guidelines, avoid implicit
anyin libs/sdk TypeScript.libs/sdk/src/transport/adapters/transport.local.adapter.ts (1)
69-74: ConstrainZodTypeinsendElicitRequestto keep strict typing.Using bare
ZodTypeintroduces implicitanydefaults. ConsiderZodType<unknown>.♻️ Suggested refactor
- abstract sendElicitRequest<S extends ZodType>( + abstract sendElicitRequest<S extends ZodType<unknown>>( relatedRequestId: RequestId, message: string, requestedSchema: S, options?: ElicitOptions, ): Promise<ElicitResult<S extends ZodType<infer O> ? O : unknown>>;As per coding guidelines, avoid implicit
anyin libs/sdk TypeScript.libs/sdk/src/context/frontmcp-context.ts (2)
103-113: Preferinterfacefor object shape typing.Since this is a shape definition, consider using an
interfaceinstead of atypealias for consistency. As per coding guidelines, preferinterfacefor object shapes.♻️ Suggested refactor
-type TransportRef = { +interface TransportRef { sendElicitRequest: <S extends ZodType>( relatedRequestId: number | string, message: string, requestedSchema: S, options?: ElicitOptions, ) => Promise<ElicitResult<S extends ZodType<infer O> ? O : unknown>>; readonly type: string; /** JSON-RPC request ID for this request - used for elicitation routing */ readonly jsonRpcRequestId?: number | string; -}; +}
308-314: Consider derivingsupportsElicitfrom transport capabilities.
supportsElicitis hardcoded totrue. If transports can run without elicitation, consider sourcing this from a capability getter instead of a constant so callers can rely on accurate introspection. As per coding guidelines, use dynamic capability exposure rather than hardcoding.
…ts with new sendElicitationResult tool
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@libs/sdk/src/elicitation/memory-elicitation.store.ts`:
- Around line 248-254: The in-memory setResolvedResult currently stores entries
indefinitely; update ElicitationMemoryStore.setResolvedResult to mirror
RedisElicitationStore by adding a 300-second TTL: store a timer in
resolvedTimers keyed by elicitId that clears the entry from resolvedResults and
removes the timer after 300_000 ms, and ensure any existing timer for that
elicitId is cleared before setting a new one; also update destroy() to iterate
and clear all resolvedTimers so no timers remain after shutdown.
In `@libs/sdk/src/elicitation/redis-elicitation.store.ts`:
- Around line 177-194: The current wait logic in redis-elicitation.store.ts can
hang because this.subscriberReady may flip true between the initial if-check and
the Promise resolution; update the block that uses subscriber.once('ready') to
(1) attach the ready listener, (2) immediately check the subscriber's actual
connection state or this.subscriberReady again and resolve synchronously if
already ready, and (3) ensure you remove the listener and clear the timeout on
both resolve and reject to avoid double callbacks; reference the existing
this.subscriberReady flag and the subscriber.once('ready') listener in your fix
and prefer checking subscriber.connected/readyState (or re-checking
this.subscriberReady) right after registering the listener to eliminate the
race.
In `@libs/sdk/src/transport/adapters/transport.sse.adapter.ts`:
- Around line 173-189: The pendingElicit can be resolved/rejected twice by both
the subscribeResult callback and the local pendingElicit.resolve/reject
handlers; to fix, capture the current pendingElicit object into a local const
when setting it and add a single-settlement guard (e.g., a local boolean settled
or check that this.pendingElicit === capturedElicit) before calling
clearTimeout, unsubscribe, and resolve/reject so those cleanup/resolve paths run
only once; update the subscribeResult callback and the resolve/reject closures
on the pendingElicit object to use the same guard and to null out
this.pendingElicit before performing resolution to prevent double-resolution
(referencing pendingElicit, subscribeResult, resolve, reject, unsubscribe,
timeoutHandle).
♻️ Duplicate comments (1)
libs/sdk/src/elicitation/elicitation.store.ts (1)
76-81: Doc example now correctly includessessionId.The example at line 79 now properly passes
pending.sessionIdtopublishResult, matching the method signature.
🧹 Nitpick comments (13)
libs/sdk/src/elicitation/__tests__/elicitation.types.test.ts (1)
206-270: Add test coverage forPendingElicitFallbackandResolvedElicitResultinterfaces.The test file covers most types but is missing tests for
PendingElicitFallbackandResolvedElicitResultinterfaces exported from the types module. Per coding guidelines, 95%+ test coverage is required.✅ Suggested additional tests
describe('PendingElicitFallback', () => { it('should create a fallback record with all required fields', () => { const fallback: PendingElicitFallback = { elicitId: 'elicit-123', sessionId: 'session-456', toolName: 'myTool', toolInput: { key: 'value' }, elicitMessage: 'Please confirm', elicitSchema: { type: 'object', properties: {} }, createdAt: Date.now(), expiresAt: Date.now() + 300000, }; expect(fallback.elicitId).toBe('elicit-123'); expect(fallback.sessionId).toBe('session-456'); expect(fallback.toolName).toBe('myTool'); expect(fallback.expiresAt).toBeGreaterThan(fallback.createdAt); }); }); describe('ResolvedElicitResult', () => { it('should create a resolved result with all required fields', () => { const resolved: ResolvedElicitResult = { elicitId: 'elicit-123', result: { status: 'accept', content: { confirmed: true } }, resolvedAt: Date.now(), }; expect(resolved.elicitId).toBe('elicit-123'); expect(resolved.result.status).toBe('accept'); expect(resolved.resolvedAt).toBeDefined(); }); });libs/sdk/src/tool/flows/tools-list.flow.ts (1)
261-292: Consider simplifying tool filtering logic.The filtering at lines 273-276 may be redundant. If
sendElicitationResultis registered withhideFromDiscovery: true, andincludeHidden = !clientSupportsElicitation(line 266), then:
- When client supports elicitation:
includeHidden = false→ hidden tools excluded bygetTools(false)- When client doesn't support:
includeHidden = true→ hidden tools includedThe explicit
isSendElicitationResultToolcheck adds defensive safety but may be unnecessary if the hidden flag is reliably set.💡 Optional simplification
If the tool is guaranteed to have
hideFromDiscovery: true, you could remove the explicit check:for (const tool of scopeTools) { - const toolName = tool.metadata?.name ?? ''; - - // Skip sendElicitationResult for clients that support standard elicitation - if (clientSupportsElicitation && isSendElicitationResultTool(toolName)) { - continue; - } - // Deduplicate tools by owner + name combinationHowever, keeping it provides an extra safety net if the hidden flag is ever misconfigured.
libs/sdk/src/scope/scope.instance.ts (1)
398-414: Consider managing Redis client lifecycle.The Redis client is created lazily but there's no cleanup mechanism when the scope is destroyed. This could lead to connection leaks in scenarios with dynamic scope creation/destruction.
💡 Suggested improvement
Consider storing the Redis client reference and adding a cleanup method:
/** Lazy-initialized elicitation store for distributed elicitation support */ private _elicitationStore?: ElicitationStore; +private _elicitationRedisClient?: import('ioredis').Redis; // In the getter, after creating redisClient: +this._elicitationRedisClient = redisClient; store = new RedisElicitationStore(redisClient, this.logger); +// Add cleanup method +async destroy(): Promise<void> { + await this._elicitationRedisClient?.quit(); +}apps/e2e/demo-e2e-elicitation/e2e/elicitation.e2e.test.ts (1)
390-411: Good error handling coverage for invalid elicitation ID.This test correctly verifies that the
sendElicitationResulttool returns an appropriate error when given a non-existentelicitId.Consider adding a test for the timeout scenario (elicitation expires before user responds), though this may require careful TTL configuration to avoid slow tests.
libs/sdk/src/elicitation/send-elicitation-result.tool.ts (2)
46-51: Return type should be strictly typed per SDK conventions.Based on learnings, MCP entry methods should return strictly typed protocol responses instead of
Promise<unknown>. Consider defining a specific result type or usingPromise<CallToolResult>.- async execute(input: { - elicitId: string; - action: 'accept' | 'cancel' | 'decline'; - content?: unknown; - }): Promise<unknown> { + async execute(input: { + elicitId: string; + action: 'accept' | 'cancel' | 'decline'; + content?: unknown; + }): Promise<CallToolResult> {
63-72: Consider extracting error response construction to a helper.The error response pattern
{ content: [{ type: 'text', text: ... }], isError: true }is repeated three times. A helper function would reduce duplication.♻️ Optional helper extraction
function errorResponse(text: string) { return { content: [{ type: 'text', text }], isError: true, }; }libs/sdk/src/common/interfaces/tool.interface.ts (1)
258-261: Justify or remove theas anycast.Per coding guidelines,
anytypes require strong justification. The castrequestedSchema as anyappears to handle the case whererequestedSchemamight be a plain object shape instead of aZodType. Consider using a more specific type or adding a comment explaining why this is necessary.- const zodSchema = requestedSchema instanceof z.ZodType ? requestedSchema : z.object(requestedSchema as any); + // Handle both ZodType instances and raw object shapes for schema definition + const zodSchema = requestedSchema instanceof z.ZodType + ? requestedSchema + : z.object(requestedSchema as z.ZodRawShape);libs/sdk/src/transport/adapters/transport.local.adapter.ts (1)
228-243: Minor: Non-atomic store operations in cancelPendingElicit.The
getPendingfollowed bypublishResultisn't atomic, which could cause a race condition in distributed mode. However, the impact is low since the localpendingElicitis resolved first, ensuring correct user-facing behavior. The worst case is publishing a cancel for an already-processed elicitation.libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (2)
202-251: Missingawaitonunsubscribecalls may cause cleanup timing issues.The
unsubscribe?.()calls in the resolve/reject closures (lines 219, 241, 247) are not awaited, while the timeout handler (line 209) does await. This inconsistency could lead to subscription cleanup racing with subsequent operations.♻️ Suggested fix for consistent cleanup
.subscribeResult<S extends ZodType<infer O> ? O : unknown>(elicitId, (result) => { clearTimeout(timeoutHandle); this.pendingElicit = undefined; - unsubscribe?.(); + void unsubscribe?.(); // Fire-and-forget is intentional resolve(result); })If fire-and-forget is intentional, add
voidto make it explicit. Otherwise, consider restructuring to ensure cleanup completes before resolution.
207-212: Uncaught errors in timeout cleanup could be silently ignored.The async operations in the timeout handler (
unsubscribe,deletePending) aren't wrapped in try-catch. If they throw, the error won't be caught (though therejectwill still be called). Consider wrapping cleanup in try-catch for robustness.const timeoutHandle = setTimeout(async () => { this.pendingElicit = undefined; - await unsubscribe?.(); - await this.elicitStore.deletePending(sessionId); + try { + await unsubscribe?.(); + await this.elicitStore.deletePending(sessionId); + } catch (cleanupError) { + this.logger.warn('Cleanup error during elicit timeout', { error: cleanupError }); + } reject(new ElicitationTimeoutError(elicitId, ttl)); }, ttl);libs/sdk/src/common/interfaces/agent.interface.ts (1)
594-597: Unnecessaryanycast and redundant type check.The
requestedSchemaparameter is already typed asS extends ZodType, so theinstanceof z.ZodTypecheck is always true. Theas anycast and thez.object()fallback path are unreachable and add confusion.♻️ Suggested simplification
- // Convert Zod schema to JSON Schema for the fallback response - // Wrap in z.object if it's a raw shape (plain object), otherwise use as-is - const zodSchema = requestedSchema instanceof z.ZodType ? requestedSchema : z.object(requestedSchema as any); - const jsonSchema = toJSONSchema(zodSchema) as Record<string, unknown>; + // Convert Zod schema to JSON Schema for the fallback response + const jsonSchema = toJSONSchema(requestedSchema) as Record<string, unknown>;libs/sdk/src/elicitation/redis-elicitation.store.ts (1)
392-393: Hardcoded TTL for resolved results may be inconsistent with elicitation TTL.The resolved result uses a fixed 300-second (5-minute) TTL, but the elicitation itself may have a different TTL configured via
options.ttl. Consider accepting the TTL as a parameter or deriving it from the fallback record's remaining TTL for consistency.♻️ Optional: Accept TTL parameter
- async setResolvedResult(elicitId: string, result: ElicitResult<unknown>): Promise<void> { + async setResolvedResult(elicitId: string, result: ElicitResult<unknown>, ttlSeconds = 300): Promise<void> { const key = RESOLVED_KEY_PREFIX + elicitId; const resolved: ResolvedElicitResult = { elicitId, result, resolvedAt: Date.now(), }; - // Store with 5-minute TTL (300 seconds) - await this.redis.set(key, JSON.stringify(resolved), 'EX', 300); + await this.redis.set(key, JSON.stringify(resolved), 'EX', ttlSeconds);Note: This would require updating the
ElicitationStoreinterface as well.libs/sdk/src/context/frontmcp-context.ts (1)
314-319: DefaultingjsonRpcRequestIdto0may conflict with valid request IDs.JSON-RPC allows
0as a valid request ID. If the transport genuinely has no request ID, consider usingundefinedor a sentinel value that the downstreamsendElicitRequestcan handle explicitly, rather than masking missing IDs with a potentially valid value.♻️ Consider throwing or handling undefined explicitly
elicit: (message, schema, options) => { - // Use the JSON-RPC request ID from the transport for proper stream routing - // The MCP SDK uses this to route elicitation requests through the correct SSE stream - const relatedRequestId = transportRef.jsonRpcRequestId ?? 0; + const relatedRequestId = transportRef.jsonRpcRequestId; + if (relatedRequestId === undefined) { + throw new Error('Cannot elicit: transport has no associated request ID'); + } return transportRef.sendElicitRequest(relatedRequestId, message, schema, options); },Alternatively, if
0is intentionally a fallback for certain transports, add a comment clarifying this is expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/elicitation/README.md`:
- Around line 92-97: Add a language identifier (e.g., "text") to the fenced code
blocks that contain the ASCII diagrams in libs/sdk/src/elicitation/README.md
(the blocks illustrating elicit(), Transport, Client and the second diagram with
throws Fallback Error, instructions, LLM and sendElicitationResult) so
markdownlint MD040 is satisfied; edit each triple-backtick fence around those
diagrams to include the language token (```text) while leaving the ASCII content
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@libs/sdk/src/elicitation/elicitation-store.factory.ts`:
- Around line 93-101: Replace the generic Error thrown in the redis provider
check with the MCP-specific error class (ElicitationNotSupportedError) so MCP
error codes and structured handling are preserved: in the if block that checks
'provider' in redis and redis.provider === 'vercel-kv' (inside
elicitation-store.factory.ts) throw new ElicitationNotSupportedError(...)
instead of new Error(...), keep the explanatory message (optionally include the
actual provider value), and add the necessary import for
ElicitationNotSupportedError from the MCP errors module.
In `@libs/sdk/src/elicitation/send-elicitation-result.tool.ts`:
- Around line 21-25: The inputSchema currently allows action: 'accept' without
content, which can produce invalid ElicitResult; update inputSchema (the object
with fields elicitId, action, content) to enforce that content is required when
action === 'accept'—either by converting to a discriminated union
(z.discriminatedUnion('action', [...]) with an {action: 'accept', content:
z.unknown()} branch and other branches for 'cancel'/'decline') or by adding a
refine on the existing schema that checks !(action === 'accept' && (content ===
undefined || content === null)) and returns a clear error message; apply the
same fix to the other matching schema instance in this file to ensure downstream
code receives a valid ElicitResult.
- Around line 57-62: The code casts this.scope to Scope and immediately
dereferences scope.elicitationStore, which can still be undefined; add a runtime
guard that checks this.scope (and/or the casted const scope) is truthy and has
an elicitationStore before calling
scope.elicitationStore.getPendingFallback(elicitId), and if missing return or
throw a clear error (e.g., log/return an error result) so functions like
sendElicitationResultTool (or the surrounding method) do not crash when scope is
absent.
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 428-439: The isEdgeRuntime() function accesses process.env
directly which can throw in Edge runtimes; update the final environment-variable
check inside isEdgeRuntime to first verify typeof process !== 'undefined' and
typeof process.env !== 'undefined' (or process && process.env) before reading
VEREL_ENV and EDGE_RUNTIME, so the function returns true only when those vars
exist and are defined; keep the other globalThis checks as-is and ensure no
direct process.env access occurs without this guard.
In `@libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts`:
- Around line 165-200: When transport.send in sendElicitRequest fails after a
successful this.elicitStore.setPending, remove the pending record to avoid stale
state: inside the catch block for transport.send (in sendElicitRequest), call
the elicit-store cleanup method (e.g., this.elicitStore.deletePending or
removePending) with the identifying keys used when setting the pending entry
(elicitId and sessionId), log the cleanup result or any cleanup error, and then
rethrow the original error; reference sendElicitRequest,
this.elicitStore.setPending, transport.send, elicitId and sessionId to locate
and implement the fix.
🧹 Nitpick comments (3)
libs/sdk/src/common/interfaces/tool.interface.ts (1)
258-262: Potential runtime trap withinstanceof z.ZodType.
In Zod v4,z.ZodTypemay not be a runtime constructor. Safer to use the importedZodTypevalue (or drop the guard entirely since the signature already requires a Zod type).🐛 Suggested adjustment
- const zodSchema = - requestedSchema instanceof z.ZodType ? requestedSchema : z.object(requestedSchema as z.ZodRawShape); + const zodSchema = + requestedSchema instanceof ZodType ? requestedSchema : z.object(requestedSchema as z.ZodRawShape);libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
50-103: Avoidanycasts in transport state loggingThe debug block relies on
anyto access_webStandardTransport, which weakens strict typing and violates the “no any without strong justification” guideline. Consider using a narrow internal type/guard so you can drop the eslint suppressions. As per coding guidelines, avoidany.♻️ Suggested refactor (typed guard)
+ type TransportWithWeb = { + _webStandardTransport?: { + _initialized?: boolean; + _started?: boolean; + sessionIdGenerator?: unknown; + }; + }; + const webTransport = (this.transport as TransportWithWeb)._webStandardTransport; this.logger.info('[StreamableHttpAdapter] transport state before handleRequest', { isRecreatable: this.transport instanceof RecreateableStreamableHTTPServerTransport, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - hasWebTransport: !!(this.transport as any)._webStandardTransport, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - webTransportInitialized: (this.transport as any)._webStandardTransport?._initialized, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - webTransportStarted: (this.transport as any)._webStandardTransport?._started, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - hasSessionIdGenerator: (this.transport as any)._webStandardTransport?.sessionIdGenerator !== undefined, + hasWebTransport: !!webTransport, + webTransportInitialized: webTransport?._initialized, + webTransportStarted: webTransport?._started, + hasSessionIdGenerator: webTransport?.sessionIdGenerator !== undefined, });libs/sdk/src/context/frontmcp-context.ts (1)
311-322: Don’t hardcodesupportsElicit
supportsElicitis alwaystruewhen a transport exists. Per SDK guidelines, this should be derived from transport capabilities (e.g.,getCapabilities()), so callers don’t assume elicitation support when it’s not actually available. As per coding guidelines, avoid hardcoding capabilities.
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/elicitation/elicitation-store.factory.ts`:
- Around line 151-164: The Redis client is currently created with a hardcoded
tls: {} which doesn't pass through user TLS settings; update the instantiation
in elicitation-store.factory.ts so redisClient uses the actual options.tls
(e.g., ...(options.tls && { tls: options.tls })) or, alternatively, add
validation logic before creating RedisElicitationStore to ensure options.tls
contains required fields (cert/ca/servername/SNI as appropriate) and throw a
clear error if missing; keep keyPrefix handling unchanged and reference
redisClient and RedisElicitationStore when applying the fix.
🧹 Nitpick comments (2)
libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
253-262: Potential race:unsubscribemay be undefined when error handler runs.If
subscribeResult()rejects before its.then()callback assignsunsubscribe, theawait unsubscribe?.()is safe due to optional chaining. However, ifsubscribeResultresolves successfully but the callback in.then()throws, the error handler may still seeunsubscribeas undefined.This is low-risk because the optional chaining handles it gracefully, but consider assigning
unsubscribeatomically or restructuring with async/await for clarity.♻️ Optional: Use async IIFE for cleaner subscription handling
- // Subscribe to results via the store (for distributed mode) - this.elicitStore - .subscribeResult<S extends ZodType<infer O> ? O : unknown>(elicitId, (result) => { - safeResolve(result); - }) - .then((unsub) => { - unsubscribe = unsub; - }) - .catch(async (err) => { - // Fail fast on subscription error instead of waiting for timeout - if (settled) return; - settled = true; - clearTimeout(timeoutHandle); - this.pendingElicit = undefined; - await unsubscribe?.(); - await this.elicitStore.deletePending(sessionId); - reject(err); - }); + // Subscribe to results via the store (for distributed mode) + (async () => { + try { + unsubscribe = await this.elicitStore.subscribeResult<S extends ZodType<infer O> ? O : unknown>( + elicitId, + (result) => safeResolve(result), + ); + } catch (err) { + // Fail fast on subscription error instead of waiting for timeout + safeReject(err); + await this.elicitStore.deletePending(sessionId); + } + })();libs/sdk/src/scope/scope.instance.ts (1)
406-422: Consider logging atinfolevel for successful registration.Currently logs at
verboselevel on success butwarnon failure. Since this is a system tool enabling fallback elicitation, consider logging success atinfolevel for better visibility in production.♻️ Optional: Log success at info level
this.scopeTools.registerToolInstance(systemToolEntry); - this.logger.verbose('Registered sendElicitationResult system tool'); + this.logger.info('Registered sendElicitationResult system tool for fallback elicitation'); } catch (error) {
…tion and improve error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 383-397: Replace the generic Error thrown in the elicitationStore
getter with a typed MCP error class (e.g., throw new MCPError(...) or a
dedicated ElicitationStoreNotInitializedError) so MCP error codes and handlers
are used; update the getter that checks this._elicitationStore (in the
elicitationStore accessor) to throw the chosen MCP error with a clear
code/message advising to await scope.ready, and add any required import for the
MCP error class at the top of the file.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/demo/src/main.ts (1)
2-2: Unused import:DashboardAppis imported but no longer used.The import remains on line 2, but
DashboardAppwas removed from theappsarray on line 13. Consider removing this unused import.Suggested fix
import { FrontMcp, LogLevel } from '@frontmcp/sdk'; -import { DashboardApp } from '@frontmcp/plugins';
🧹 Nitpick comments (2)
plugins/plugin-cache/src/providers/cache-redis.provider.ts (1)
28-30: TODO noted: migration to storage utils.The TODO comment is acknowledged. The current implementation uses direct
console.log/console.errorcalls for connection events, which the storage utils migration would likely abstract.Would you like me to help implement the migration to storage utils, or should I open an issue to track this task?
apps/demo/src/main.ts (1)
21-24: Consider adding environment variable fallbacks for Redis config.Other configuration values in this file (e.g.,
IDP_PROVIDER_URL) use environment variable fallbacks. For consistency and flexibility in different environments, consider the same pattern here.Suggested improvement
redis: { - port: 6379, - host: 'localhost', + port: parseInt(process.env['REDIS_PORT'] || '6379', 10), + host: process.env['REDIS_HOST'] || 'localhost', },
…ng in elicitation store access
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/sdk/src/auth/session/__tests__/session-id.utils.test.ts`:
- Around line 167-172: The test currently uses a no-op arrow function instead of
asserting the result; change it to capture the return value of
createSessionId('streamable-http', TEST_TOKEN) (e.g., const result =
createSessionId(...)) and then assert directly with
expect(result.payload.platformType).toBeUndefined(); keep the existing
expect(mockDetectPlatformFromUserAgent).not.toHaveBeenCalled() assertion
unchanged so the test verifies no platform detection occurred when there's no
user-agent.
In
`@libs/sdk/src/transport/mcp-handlers/__tests__/initialize-request.handler.test.ts`:
- Around line 439-447: The test currently only asserts that an error is thrown;
update it to assert the specific error type for invalid protocol versions by
invoking the handler returned by initializeRequestHandler and then expecting the
promise rejection to be an instance of the protocol-version error class (e.g.,
InvalidProtocolVersionError or the project-specific error type). Replace the
generic await expect(handler.handler(request, ctx)).rejects.toThrow() with an
assertion that checks the rejected error is instanceof the correct error class
(using rejects.toEqual/expect.assertions or a .catch to inspect the error) so
the test verifies the error hierarchy for invalid protocolVersion handling.
🧹 Nitpick comments (1)
libs/sdk/src/auth/session/__tests__/session-id.utils.test.ts (1)
14-35: Avoidanyin mocks/config to keep strict TS guarantees.The explicit
anyin mock signatures andconfig as anybypass strict typing. Consider deriving the config type from the real function signature and usingunknownfor encrypted inputs. As per coding guidelines, avoidany.♻️ Proposed refactor
-import { SessionIdPayload, TransportProtocolType } from '../../../common'; +import { SessionIdPayload, TransportProtocolType } from '../../../common'; +import type * as NotificationService from '../../../notification/notification.service'; @@ - decryptValue: (encrypted: any, key: Uint8Array) => mockDecryptValue(encrypted, key), + decryptValue: (encrypted: unknown, key: Uint8Array) => mockDecryptValue(encrypted, key), @@ -const mockDetectPlatformFromUserAgent = jest.fn(); -jest.mock('../../../notification/notification.service', () => ({ - detectPlatformFromUserAgent: (ua: string, config?: any) => mockDetectPlatformFromUserAgent(ua, config), -})); +type PlatformDetectionConfig = Parameters<typeof NotificationService.detectPlatformFromUserAgent>[1]; +const mockDetectPlatformFromUserAgent = jest.fn(); +jest.mock('../../../notification/notification.service', () => ({ + detectPlatformFromUserAgent: (ua: string, config?: PlatformDetectionConfig) => + mockDetectPlatformFromUserAgent(ua, config), +})); @@ - const config = { userAgentPatterns: [{ pattern: 'custom', platform: 'cursor' }] }; + const config: PlatformDetectionConfig = { userAgentPatterns: [{ pattern: 'custom', platform: 'cursor' }] }; @@ - platformDetectionConfig: config as any, + platformDetectionConfig: config,Also applies to: 145-152
libs/sdk/src/transport/mcp-handlers/__tests__/initialize-request.handler.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@libs/sdk/src/auth/session/__tests__/session-id.utils.test.ts`:
- Around line 267-282: The test captures result from updateSessionPayload for a
public session but never asserts it; update the test to assert that result is
true for the valid publicPayload path (after mocking mockDecryptValue to return
publicPayload) so the behavior of updateSessionPayload('public.session.id', {
platformType: 'claude' }) is validated; reference the existing symbols
updateSessionPayload, mockDecryptValue, publicPayload and result when adding the
expect assertion.
- Around line 252-265: The test currently only asserts that mockDecryptValue was
called; add an assertion that updateSessionPayload('iv-part.tag-part.data-part',
{ clientName: 'Decrypted Client' }) returns true (since mockDecryptValue returns
a valid payload), by capturing the result variable and asserting it is true to
confirm the successful decrypt-and-update path in updateSessionPayload;
reference the updateSessionPayload call and mockDecryptValue mock in the test to
implement this assertion.
- Around line 501-512: The test "should complete encryption in development mode
without secret" sets up a console.warn spy but never asserts it; either assert
the expected warning or remove the spy. To fix, in the test for
createSessionId('streamable-http', TEST_TOKEN) either add an assertion like
expect(warnSpy).toHaveBeenCalled() (or toHaveBeenCalledWith(...) if a specific
message is expected) before warnSpy.mockRestore(), or simply remove the
jest.spyOn(console, 'warn') mocking and mockRestore call if no warning assertion
is desired.
In
`@libs/sdk/src/transport/mcp-handlers/__tests__/initialize-request.handler.test.ts`:
- Around line 213-230: The test in initialize-request.handler.test.ts
incorrectly uses a non-empty elicitation capability; update the test to use an
empty object for elicitation to match the MCP spec: change the
elicitationCapability assigned in the 'should call supportsElicitation with
elicitation capability' test to {} so that when
initializeRequestHandler(handlerOptions) runs and handler.handler(request, ctx)
is invoked, mockSupportsElicitation is asserted against an objectContaining({
elicitation: {} }) as expected by the protocol.
🧹 Nitpick comments (3)
libs/sdk/src/auth/session/__tests__/session-id.utils.test.ts (2)
19-19: Avoid untypedanyin mock signatures.Per coding guidelines, prefer
unknownor specific types overany. For the mock parameters:
- Line 19:
encrypted: any→ useunknownor the actual encrypted value type- Line 34:
config?: any→ use the actualPlatformDetectionConfigtype orunknown♻️ Suggested fix
- decryptValue: (encrypted: any, key: Uint8Array) => mockDecryptValue(encrypted, key), + decryptValue: (encrypted: unknown, key: Uint8Array) => mockDecryptValue(encrypted, key),- detectPlatformFromUserAgent: (ua: string, config?: any) => mockDetectPlatformFromUserAgent(ua, config), + detectPlatformFromUserAgent: (ua: string, config?: unknown) => mockDetectPlatformFromUserAgent(ua, config),Also applies to: 34-34
148-155: Avoidas anytype assertion.The
as anycast on line 151 bypasses type checking. Import and use the actualPlatformDetectionConfigtype to maintain type safety.♻️ Suggested fix
+import { PlatformDetectionConfig } from '../../../notification/notification.service'; + // In the test: - const config = { userAgentPatterns: [{ pattern: 'custom', platform: 'cursor' }] }; + const config: Partial<PlatformDetectionConfig> = { + userAgentPatterns: [{ pattern: 'custom', platform: 'cursor' as const }] + }; createSessionId('streamable-http', TEST_TOKEN, { userAgent: 'Custom-Client/1.0', - platformDetectionConfig: config as any, + platformDetectionConfig: config as PlatformDetectionConfig, });libs/sdk/src/transport/mcp-handlers/__tests__/initialize-request.handler.test.ts (1)
12-25: Consider using typed mock signatures instead of spreadingany.The mock setup uses
...args: any[]which bypasses TypeScript's type checking. While this is common in test files, you could improve type safety by using the actual function signatures.// More type-safe approach (optional improvement) const mockUpdateSessionPayload = jest.fn<boolean, [string, Partial<SessionIdPayload>]>();This is a minor concern since it's test code, but worth considering for consistency with strict TypeScript mode.
libs/sdk/src/transport/mcp-handlers/__tests__/initialize-request.handler.test.ts
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
80-91: Remove rawconsole.logdebugging from request handling.These logs bypass the structured logger and can leak session context in production. Prefer scoped logger calls or remove.
🧹 Proposed cleanup
- console.log('[DEBUG] parseInput: starting'); const authorization = request[ServerRequestTokens.auth] as Authorization; - console.log( - '[DEBUG] parseInput: authorization =', - authorization - ? { - hasToken: !!authorization.token, - hasSession: !!authorization.session, - sessionId: authorization.session?.id?.slice(0, 30), - } - : 'undefined', - ); const { token } = authorization;
🤖 Fix all issues with AI agents
In `@libs/sdk/src/common/metadata/front-mcp.metadata.ts`:
- Around line 92-98: The JSDoc for the `elicitation?: ElicitationOptionsInput`
field is incorrect: it states `@default { enabled: true }` but
`elicitationOptionsSchema` actually defaults `enabled` to false and the field is
optional; either update the comment to `@default { enabled: false }` to match
the schema, or if the intended behavior is true, set the default in
`elicitationOptionsSchema` to `enabled: true` instead—make the change in the doc
or schema for `elicitation`/`ElicitationOptionsInput`/`elicitationOptionsSchema`
so they are consistent.
In `@libs/sdk/src/context/frontmcp-context.ts`:
- Around line 315-322: The code currently falls back to 0 when
transportRef.jsonRpcRequestId is undefined which can cause routing collisions;
change the behavior in FrontMcpContext's elicit path to fail fast instead of
using 0: detect when transportRef.jsonRpcRequestId is undefined and throw or
return a rejected MCP-specific error (e.g., new Error or your project’s MCP
error class) with a clear message like "Missing jsonRpcRequestId for
sendElicitRequest", and/or enforce generation of a unique ID upstream if
appropriate; update the call site that invokes transportRef.sendElicitRequest to
only proceed when jsonRpcRequestId is present (use transportRef.jsonRpcRequestId
and sendElicitRequest together) so no fallback numeric ID is used.
In `@libs/sdk/src/elicitation/helpers/elicit.helper.ts`:
- Around line 83-87: The comment above the elicitationEnabled check contradicts
the ElicitHelperDeps interface which documents `@default` true; update one of them
for consistency: either change the comment to state "elicitationEnabled defaults
to true if not specified" to match the ElicitHelperDeps interface, or modify the
ElicitHelperDeps JSDoc `@default` to false to match the runtime check that throws
ElicitationDisabledError when elicitationEnabled !== true; locate the
ElicitHelperDeps interface and the elicitationEnabled check (and the
ElicitationDisabledError reference) and make the chosen source of truth
consistent.
In `@libs/sdk/src/elicitation/helpers/extend-output-schema.ts`:
- Around line 36-42: The current branch in extend-output-schema.ts returns a
oneOf combining a generic object and ELICITATION_FALLBACK_JSON_SCHEMA which
overlaps (both match objects); change the returned discriminator to anyOf
instead of oneOf (or alternatively make the generic object explicitly exclude
the fallback using a not with ELICITATION_FALLBACK_JSON_SCHEMA) so the fallback
remains usable — update the return in the no-original-schema branch that
references ELICITATION_FALLBACK_JSON_SCHEMA (and any tests or callers expecting
oneOf) accordingly.
In `@libs/sdk/src/elicitation/store/storage-elicitation.store.ts`:
- Around line 126-198: The subscribeResult method can race and create duplicate
pub/sub subscriptions; add an in-flight subscription guard (e.g., a
pendingSubscriptions Map keyed by elicitId) and use it alongside
activeSubscriptions so only one storage.subscribe is invoked per elicitId: when
about to subscribe, check pendingSubscriptions and activeSubscriptions, if none
create and store a Promise for the subscribe call, await that Promise, then
store the resolved unsubscribe in activeSubscriptions and remove the pending
entry; ensure errors remove the pending entry and log appropriately; keep the
existing localCallbacks behavior and ensure the returned unsubscribe clears both
activeSubscriptions and pendingSubscriptions as needed.
In `@scripts/fix-unused-imports.mjs`:
- Around line 22-54: The git command invocation in getChangedFiles uses string
interpolation with execSync which allows injection and breaks on paths with
spaces; change it to use execFileSync (or execFile) passing the git binary and
arguments as an array (e.g., ['diff', baseBranch + '...HEAD', '--name-only',
'--diff-filter=ACM']) and validate the user-supplied base branch from
getBaseBranch (ensure it matches /^[A-Za-z0-9._\/-]+$/ or similar safe pattern
and reject/exit on invalid input) before passing it to execFileSync; also apply
the same argv-array approach to any other execSync calls (e.g., the rev-parse
checks in getBaseBranch) so all git calls avoid shell interpolation and handle
filenames with spaces/special chars safely.
- Around line 99-107: The current try/catch around the execSync call swallows
all ESLint failures; change the catch to log the error and rethrow it so real
failures surface (while keeping the existing finally cleanup). Specifically, in
the block surrounding execSync(`npx eslint ...`, { stdio: 'inherit' }) that
writes tempConfigPath and calls execSync, replace the silent catch that prints
"Done processing files." with logic that logs the caught error (include
error.message/status) and then throw error; keep the finally block intact so
tempConfigPath cleanup runs regardless.
♻️ Duplicate comments (1)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
214-223: Recreate transports for elicit results in distributed mode.
onElicitResultonly checks in-memory transports; in multi-node setups an elicitation result can land on a node without the in-memory transport and will be treated as expired even if a stored session exists. Mirror theonMessagerecreation flow before returning sessionExpired.🔧 Suggested fix (mirror session recreation)
- const transport = await transportService.getTransporter('streamable-http', token, session.id); + let transport = await transportService.getTransporter('streamable-http', token, session.id); + + if (!transport) { + try { + const storedSession = await transportService.getStoredSession('streamable-http', token, session.id); + if (storedSession) { + logger.info('onElicitResult: recreating transport from stored session', { + sessionId: session.id?.slice(0, 20), + }); + transport = await transportService.recreateTransporter( + 'streamable-http', + token, + session.id, + storedSession, + response, + ); + } + } catch (error) { + logger.warn('onElicitResult: failed to recreate transport from stored session', { + sessionId: session.id?.slice(0, 20), + error: error instanceof Error ? error.message : String(error), + }); + } + }
🧹 Nitpick comments (10)
scripts/fix-unused-imports.mjs (1)
68-70: Use a unique temp config name to avoid clobbering.A fixed filename can overwrite an existing file or collide in parallel runs. Use a unique filename (PID/timestamp) instead.
♻️ Suggested tweak
- const tempConfigPath = join(process.cwd(), 'eslint.fix-imports.config.mjs'); + const tempConfigPath = join( + process.cwd(), + `eslint.fix-imports.${process.pid}.config.mjs` + );libs/sdk/src/scope/scope.instance.ts (1)
300-307: Consider registering elicitation flows only when enabled.
Right now the flows are always registered; if they don’t internally guard onelicitation.enabled, they could become callable even when the feature is disabled.♻️ Suggested gating
- await this.scopeFlows.registryFlows([ - SetLevelFlow, - CompleteFlow, - CallAgentFlow, - ElicitationRequestFlow, - ElicitationResultFlow, - ]); + const flows: FlowType[] = [SetLevelFlow, CompleteFlow, CallAgentFlow]; + if (elicitationEnabled) { + flows.push(ElicitationRequestFlow, ElicitationResultFlow); + } + await this.scopeFlows.registryFlows(flows);libs/sdk/src/tool/tool.instance.ts (1)
28-28: Avoidanyin raw output schema typing.
Tighten the return type tounknown(or a JSON schema type) to align with strict TS guidance. As per coding guidelines, avoidanyin SDK code.♻️ Suggested typing update
- override getRawOutputSchema(): any | undefined { + override getRawOutputSchema(): unknown { const baseSchema = this.rawOutputSchema;Also applies to: 111-129
libs/sdk/src/elicitation/flows/elicitation-result.flow.ts (1)
156-162: Avoid non-null assertion onsessionId.The
sessionId!assertions at lines 157 and 161 could be replaced with a guard or by accessing it fromthis.state.requiredwhere it's already validated. As per coding guidelines, prefer proper error handling over non-null assertions.♻️ Suggested fix
`@Stage`('publishResult') async publishResult() { this.logger.verbose('publishResult:start'); - const { sessionId, pendingRecord, elicitResult } = this.state; + const { sessionId, pendingRecord, elicitResult } = this.state.required; const scope = this.scope as Scope; if (!pendingRecord || !elicitResult) { this.state.set('handled', false); this.logger.verbose('publishResult:skip (no pending or no result)'); return; } try { - await scope.elicitationStore.publishResult(pendingRecord.elicitId, sessionId!, elicitResult); + await scope.elicitationStore.publishResult(pendingRecord.elicitId, sessionId, elicitResult); this.state.set('handled', true); this.logger.verbose('publishResult:done', { elicitId: pendingRecord.elicitId, - sessionId: sessionId!.slice(0, 20), + sessionId: sessionId.slice(0, 20), });libs/sdk/src/elicitation/flows/elicitation-request.flow.ts (2)
145-158: Consider usingrandomUUIDfor elicit ID generation.The current ID generation (
elicit-${Date.now()}-${Math.random().toString(36).slice(2)}) works but@frontmcp/utilsprovidesrandomUUIDwhich is already used elsewhere in the codebase (e.g.,call-tool.flow.tsline 578). This would provide stronger uniqueness guarantees in distributed scenarios.♻️ Suggested improvement
+import { randomUUID } from '@frontmcp/utils'; + `@Stage`('generateElicitId') async generateElicitId() { this.logger.verbose('generateElicitId:start'); const { elicitationId, ttl } = this.state; // Generate elicit ID if not provided - const elicitId = elicitationId ?? `elicit-${Date.now()}-${Math.random().toString(36).slice(2)}`; + const elicitId = elicitationId ?? `elicit-${randomUUID()}`; const expiresAt = Date.now() + (ttl ?? DEFAULT_ELICIT_TTL);
214-221: Avoid non-null assertions in finalize.The
requestParams!andpendingRecord!assertions assume these are set by earlier stages. Consider usingthis.state.requiredor adding guards to satisfy TypeScript without assertions.♻️ Suggested fix
`@Stage`('finalize') async finalize() { this.logger.verbose('finalize:start'); - const { elicitId, sessionId, expiresAt, mode, requestParams, pendingRecord } = this.state.required; + const { elicitId, sessionId, expiresAt, mode } = this.state.required; + const { requestParams, pendingRecord } = this.state; + + if (!requestParams || !pendingRecord) { + throw new Error('finalize: missing requestParams or pendingRecord from prior stages'); + } // Set the output using respond() so runFlowForOutput can return it this.respond({ elicitId, sessionId, expiresAt, mode, - requestParams: requestParams!, - pendingRecord: pendingRecord!, + requestParams, + pendingRecord, });libs/sdk/src/elicitation/helpers/elicit.helper.ts (2)
137-139: ID generation could have collisions under high concurrency.Using
Date.now()combined withMath.random().toString(36).slice(2)provides limited entropy. Under high-concurrency scenarios, collisions are possible.Consider using a cryptographically stronger approach. Per coding guidelines, use
@frontmcp/utilsfor cryptographic operations:♻️ Suggested improvement
+import { randomUUID } from '@frontmcp/utils'; + export function generateElicitationId(): string { - return `elicit-${Date.now()}-${Math.random().toString(36).slice(2)}`; + return `elicit-${randomUUID()}`; }
114-117: Redundant ZodType check for constrained generic.Since
S extends ZodTypeis enforced by the type parameter,requestedSchemais always aZodTypeinstance. Theinstanceofcheck and fallback toz.object()appears defensive but may mask type errors at compile time.💡 Suggested simplification
- // Convert Zod schema to JSON Schema for the fallback response - // Wrap in z.object if it's a raw shape (plain object), otherwise use as-is - const zodSchema = - requestedSchema instanceof z.ZodType ? requestedSchema : z.object(requestedSchema as z.ZodRawShape); - const jsonSchema = toJSONSchema(zodSchema) as Record<string, unknown>; + // Convert Zod schema to JSON Schema for the fallback response + const jsonSchema = toJSONSchema(requestedSchema) as Record<string, unknown>;#!/bin/bash # Verify if performElicit is ever called with non-ZodType schemas rg -n "performElicit\(" --type=ts -A5 | head -50libs/sdk/src/elicitation/store/elicitation-store.factory.ts (1)
182-209: Legacy Redis format handling uses type assertions without validation.The conversion of legacy Redis options uses multiple type assertions (
as { host: string }) without validating the actual shape. If the format doesn't match expectations, runtime errors could occur.♻️ Safer type handling
if (isNewRedisFormat || isLegacyRedisFormat) { - const redisHost = (redis as { host: string }).host; - const redisPort = (redis as { port?: number }).port ?? 6379; - const redisPassword = (redis as { password?: string }).password; - const redisDb = (redis as { db?: number }).db ?? 0; - const redisTls = (redis as { tls?: boolean }).tls ?? false; + const redisConfig = redis as Record<string, unknown>; + const redisHost = redisConfig.host as string; + const redisPort = typeof redisConfig.port === 'number' ? redisConfig.port : 6379; + const redisPassword = typeof redisConfig.password === 'string' ? redisConfig.password : undefined; + const redisDb = typeof redisConfig.db === 'number' ? redisConfig.db : 0; + const redisTls = typeof redisConfig.tls === 'boolean' ? redisConfig.tls : false;libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (1)
197-259: Significant code duplication with SSE adapter's promise handling.The promise creation, settlement guard, timeout handling, and subscription logic (lines 199-259) is nearly identical to
transport.sse.adapter.ts(lines 147-207). Per coding guidelines, consider extracting to a shared base class or helper.The coding guidelines mention "Create shared base classes for common functionality like ExecutionContextBase for ToolContext and ResourceContext." The same principle applies here.
💡 Suggested approach
Consider extracting the promise-with-timeout-and-subscription logic into
LocalTransportAdapter:// In LocalTransportAdapter protected createElicitPromise<S extends ZodType>( elicitId: string, sessionId: string, ttl: number, ): Promise<ElicitResult<S extends ZodType<infer O> ? O : unknown>> { // Shared settlement guard, timeout, subscription, cleanup logic }Both adapters would then call this shared method after their adapter-specific setup.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@libs/sdk/src/elicitation/store/encrypted-elicitation.store.ts`:
- Around line 86-112: In setPending (EncryptedElicitationStore) you are
currently spreading the original PendingElicitRecord into the object passed to
this.store.setPending which stores unencrypted fields alongside __encrypted;
instead pass only the minimal metadata and the encrypted blob (e.g., sessionId,
elicitId or whatever store needs) plus __encrypted produced by
encryptElicitationData, removing all original plaintext fields from the payload,
and adjust the cast/type (PendingElicitRecord) expected by store.setPending
accordingly so only the encrypted blob and approved metadata are persisted.
- Around line 382-419: The method decryptPendingRecord currently treats records
without an __encrypted blob as an error and returns null, but the module header
promises a plaintext fallback for migration; update decryptPendingRecord
(function name) so that when encryptedBlob is missing or
isEncryptedBlob(encryptedBlob) is false it returns the original stored
PendingElicitRecord (not null), and adjust the logger message to reflect a
plaintext fallback (use sessionId and stored.elicitId where available) so
callers receive the plaintext record for migration to encrypted storage.
In `@libs/sdk/src/elicitation/store/storage-elicitation.store.ts`:
- Around line 263-299: In publishResult, avoid double-delivery by using the same
pattern as publishFallbackResult: if local callbacks exist for elicitId
(this.localCallbacks.get(elicitId)), invoke them and skip publishing to pub/sub;
otherwise, if this.storage.supportsPubSub() publish the JSON result to
RESULT_CHANNEL_PREFIX + elicitId; always call this.deletePending(sessionId) and
log via this.logger afterwards; update the conditional flow in publishResult to
choose either local callback invocation or storage.publish, not both.
In `@libs/sdk/src/tool/flows/call-tool.flow.ts`:
- Around line 649-667: When subscribe to the fallback result fails inside
handleWaitingFallback, ensure the pending fallback record is removed before
rejecting: call and await the cleanup routine (or explicitly invoke the
pending-delete helper for elicitId) in the catch block so the pending record is
deleted, log the elicitId and any cleanup errors via this.logger.error, then
reject with the ToolExecutionError (preserving the original error message);
reference handleWaitingFallback, cleanup(), elicitId, resolved and
ToolExecutionError when making the change.
♻️ Duplicate comments (2)
scripts/fix-unused-imports.mjs (2)
17-17: Avoid shell interpolation for ESLint invocation.Line 145–146 still uses
execSyncwith a string; quoting is fragile and can break on filenames with quotes or metacharacters. PreferexecFileSyncwith an арг array and drop the shell.🔧 Safer invocation
-import { execFileSync, execSync } from 'child_process'; +import { execFileSync } from 'child_process';- const quotedFiles = files.map((f) => `"${f}"`).join(' '); - execSync(`npx eslint --config "${tempConfigPath}" --fix ${quotedFiles}`, { stdio: 'inherit' }); + execFileSync( + 'npx', + ['eslint', '--config', tempConfigPath, '--fix', ...files], + { stdio: 'inherit' } + );Also applies to: 144-146
148-155: Don’t treat ESLint exit code 1 as success.Line 149–155 swallows failures; ESLint returns
1when lint errors remain after--fix. Surface the failure after cleanup.✅ Preserve failures
- } catch (error) { - // ESLint exits with code 1 if it finds (and fixes) issues, which is expected - // Only log and rethrow for other errors (exit code > 1 indicates actual failure) - if (error.status !== undefined && error.status > 1) { - console.error('ESLint encountered an error:', error.message || 'Unknown error'); - throw error; - } - console.log('Done processing files.'); - } finally { + } catch (error) { + console.error('ESLint failed. See output above.'); + throw error; + } finally {
🧹 Nitpick comments (12)
libs/sdk/src/elicitation/store/__tests__/elicitation-encryption.spec.ts (1)
27-37: Consider usingafterEachfor environment cleanup.Using
afterAllfor restoringprocess.envmeans if a test fails mid-suite, subsequent tests in other describe blocks could see stale environment state. Moving restoration toafterEachprovides better isolation.♻️ Suggested improvement
beforeEach(() => { // Reset environment process.env = { ...originalEnv }; delete process.env['MCP_ELICITATION_SECRET']; delete process.env['MCP_SESSION_SECRET']; delete process.env['MCP_SERVER_SECRET']; }); - afterAll(() => { + afterEach(() => { process.env = originalEnv; });libs/sdk/src/elicitation/helpers/__tests__/validate-elicitation-content.test.ts (2)
138-141: Avoid non-null assertions; use proper type guards.Per coding guidelines, avoid
!non-null assertions. Use type narrowing or assertions that fail the test clearly.♻️ Suggested fix
expect(result.success).toBe(false); expect(result.errors).toBeDefined(); - expect(result.errors!.length).toBeGreaterThan(0); + expect(result.errors?.length).toBeGreaterThan(0);Or use explicit assertion:
expect(result.errors).toBeDefined(); if (!result.errors) throw new Error('Expected errors to be defined'); expect(result.errors.length).toBeGreaterThan(0);This pattern appears in multiple places (lines 140, 239-242, 269-270).
274-330: Add test for malformed JSON Schema to cover error handling path.The implementation's catch block (lines 74-85 in
validate-elicitation-content.ts) handles schema conversion errors. Consider adding a test to exercise this path and ensure 95%+ branch coverage.🧪 Suggested test case
it('should handle malformed JSON Schema gracefully', () => { const content = { name: 'John' }; const invalidSchema = { type: 'invalid-type-that-does-not-exist', properties: 'not-an-object', // Invalid structure }; const result = validateElicitationContent(content, invalidSchema); expect(result.success).toBe(false); expect(result.errors).toBeDefined(); expect(result.errors?.[0]?.message).toContain('Schema validation error'); });libs/sdk/src/elicitation/send-elicitation-result.tool.ts (1)
61-62: Consider improving type safety for scope access.The double cast
as unknown as Scopebypasses TypeScript's type checking. Since the scope guard on lines 65-76 handles the null case, consider defining a proper type guard or using a more explicit pattern.♻️ Alternative approach
- // Cast scope to Scope type to access elicitationStore - const scope = this.scope as unknown as Scope; + // Access scope with explicit undefined handling + const scope = this.scope as Scope | undefined;The existing guard at line 65 already handles the undefined case with optional chaining (
scope?.elicitationStore), so this change makes the type more accurate.libs/sdk/src/elicitation/flows/elicitation-result.flow.ts (1)
28-45: Consider usingz.unknown()instead ofz.any()for schema definitions.Per coding guidelines,
unknownis preferred overanyfor generic type parameter defaults. While the type assertions provide runtime type safety, usingz.unknown()would be more consistent:const outputSchema = z.object({ /** Whether the result was handled */ handled: z.boolean(), /** The elicit ID (if pending was found) */ elicitId: z.string().optional(), /** The processed result */ - result: z.any().optional() as z.ZodType<ElicitResult | undefined>, + result: z.unknown().optional() as z.ZodType<ElicitResult | undefined>, }); const stateSchema = z.object({ sessionId: z.string(), action: z.enum(['accept', 'cancel', 'decline']), content: z.unknown().optional(), - pendingRecord: z.any().optional() as z.ZodType<PendingElicitRecord | undefined>, - elicitResult: z.any().optional() as z.ZodType<ElicitResult | undefined>, + pendingRecord: z.unknown().optional() as z.ZodType<PendingElicitRecord | undefined>, + elicitResult: z.unknown().optional() as z.ZodType<ElicitResult | undefined>, handled: z.boolean().default(false), output: outputSchema.optional(), });libs/sdk/src/elicitation/flows/elicitation-request.flow.ts (1)
38-66: Consider usingz.unknown()instead ofz.any()for schema definitions.Similar to the result flow, using
z.unknown()would be more consistent with coding guidelines:const outputSchema = z.object({ // ... - pendingRecord: z.any() as z.ZodType<PendingElicitRecord>, + pendingRecord: z.unknown() as z.ZodType<PendingElicitRecord>, }); const stateSchema = z.object({ // ... - pendingRecord: z.any().optional() as z.ZodType<PendingElicitRecord | undefined>, + pendingRecord: z.unknown().optional() as z.ZodType<PendingElicitRecord | undefined>, // ... });libs/sdk/src/elicitation/store/__tests__/storage-elicitation.store.spec.ts (1)
85-136: Consider extracting the timing delay into a constant.The 50ms delay for pub/sub propagation appears multiple times. Extracting it would improve maintainability and make it easier to adjust if tests become flaky:
const PUBSUB_PROPAGATION_DELAY = 50; // ... await new Promise((resolve) => setTimeout(resolve, PUBSUB_PROPAGATION_DELAY));This is a minor suggestion for test maintainability.
libs/sdk/src/elicitation/store/__tests__/encrypted-elicitation.store.spec.ts (1)
185-187: Avoid fixed sleeps in pub/sub tests.
The 50ms waits can be flaky under CI load; prefer awaiting a promise resolved by the subscription callback.♻️ Example refactor for the publishResult test (apply similarly to fallback tests)
- let receivedResult: ElicitResult<unknown> | null = null; - - // Subscribe first - const unsubscribe = await encryptedStore.subscribeResult( - elicitId, - (result) => { - receivedResult = result; - }, - testSessionId1, - ); + let resolveResult!: (result: ElicitResult<unknown>) => void; + const received = new Promise<ElicitResult<unknown>>((resolve) => { + resolveResult = resolve; + }); + + // Subscribe first + const unsubscribe = await encryptedStore.subscribeResult(elicitId, resolveResult, testSessionId1); // Publish result await encryptedStore.publishResult(elicitId, testSessionId1, expectedResult); - // Wait a bit for pub/sub - await new Promise((resolve) => setTimeout(resolve, 50)); - - // Check received - expect(receivedResult).toEqual(expectedResult); + // Check received (no fixed sleep) + await expect(received).resolves.toEqual(expectedResult); await unsubscribe();Also applies to: 318-319, 346-347, 381-382
libs/sdk/src/tool/flows/call-tool.flow.ts (1)
379-397: Guard transport wiring when requestId is missing.
IfjsonRpcRequestIdis undefined,FrontMcpContext.transport.elicit()will fail later; consider skipping wiring or logging in that case.♻️ Suggested guard
- const frontmcpContext = context.tryGetContext(); - if (frontmcpContext && authInfo?.transport?.sendElicitRequest) { - const transport = authInfo.transport; - // Pass the JSON-RPC request ID for proper elicitation routing - // The MCP SDK uses this to route messages through the correct SSE stream - const jsonRpcRequestId = this.state.jsonRpcRequestId; + const frontmcpContext = context.tryGetContext(); + const jsonRpcRequestId = this.state.jsonRpcRequestId; + if (frontmcpContext && authInfo?.transport?.sendElicitRequest && jsonRpcRequestId !== undefined) { + const transport = authInfo.transport; + // Pass the JSON-RPC request ID for proper elicitation routing + // The MCP SDK uses this to route messages through the correct SSE stream frontmcpContext.setTransport({ sendElicitRequest: transport.sendElicitRequest.bind(transport), type: transport.type, jsonRpcRequestId, }); }libs/sdk/src/transport/adapters/transport.streamable-http.adapter.ts (2)
67-77: Consider extracting transport state inspection to a debug utility.Multiple
eslint-disablecomments for accessing private_webStandardTransportproperties indicate this is debugging code. While acceptable for troubleshooting, consider:
- Adding a
getDebugState()method toRecreateableStreamableHTTPServerTransportfor clean access- Removing this logging once the transport is stable
203-207: Silent error swallowing in cleanup's unsubscribe call.Using
void unsubscribe?.()discards any rejection from the async unsubscribe. If unsubscription fails, there's no visibility into the issue.🔧 Consider logging unsubscribe errors
const cleanup = () => { clearTimeout(timeoutHandle); this.pendingElicit = undefined; - void unsubscribe?.(); + unsubscribe?.().catch((err) => { + this.logger.warn('[StreamableHttpAdapter] Failed to unsubscribe during cleanup', err); + }); };libs/sdk/src/context/frontmcp-context.ts (1)
23-24: Consider usingSymbol()instead ofSymbol.for()for internal key.
Symbol.for('frontmcp:pre-resolved-elicit')creates a globally-shared symbol, accessible by any code callingSymbol.for()with the same key. Since this is marked@internaland only used within this class, a localSymbol()would provide stronger encapsulation.🔒 Use local Symbol for encapsulation
-/** Symbol key for storing pre-resolved elicit result in context store */ -const PRE_RESOLVED_ELICIT_KEY = Symbol.for('frontmcp:pre-resolved-elicit'); +/** Symbol key for storing pre-resolved elicit result in context store */ +const PRE_RESOLVED_ELICIT_KEY = Symbol('frontmcp:pre-resolved-elicit');
| async setPending(record: PendingElicitRecord): Promise<void> { | ||
| const { sessionId } = record; | ||
|
|
||
| try { | ||
| // Encrypt the record using its sessionId | ||
| const encrypted = await encryptElicitationData(record, sessionId, this.secret); | ||
|
|
||
| // Store as encrypted blob (the underlying store serializes as JSON) | ||
| await this.store.setPending({ | ||
| ...record, | ||
| // Replace the record fields with encrypted blob marker | ||
| // The actual data is in the encrypted blob | ||
| __encrypted: encrypted, | ||
| } as unknown as PendingElicitRecord); | ||
|
|
||
| this.logger?.debug('[EncryptedElicitationStore] Stored encrypted pending record', { | ||
| sessionId: sessionId.slice(0, 8) + '...', | ||
| elicitId: record.elicitId, | ||
| }); | ||
| } catch (error) { | ||
| this.logger?.error('[EncryptedElicitationStore] Failed to encrypt pending record', { | ||
| sessionId: sessionId.slice(0, 8) + '...', | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Encrypted blob stored alongside unencrypted record fields.
The current pattern spreads the original record and adds __encrypted:
await this.store.setPending({
...record, // Original unencrypted fields
__encrypted: encrypted,
} as unknown as PendingElicitRecord);This means both encrypted and unencrypted data are stored. The underlying store receives the full record with plain fields plus the encrypted blob.
🔒 Store only the encrypted blob
// Store as encrypted blob (the underlying store serializes as JSON)
await this.store.setPending({
- ...record,
- // Replace the record fields with encrypted blob marker
- // The actual data is in the encrypted blob
+ // Store minimal fields needed for key lookup, plus encrypted blob
+ sessionId: record.sessionId,
+ elicitId: record.elicitId,
+ expiresAt: record.expiresAt,
__encrypted: encrypted,
} as unknown as PendingElicitRecord);🤖 Prompt for AI Agents
In `@libs/sdk/src/elicitation/store/encrypted-elicitation.store.ts` around lines
86 - 112, In setPending (EncryptedElicitationStore) you are currently spreading
the original PendingElicitRecord into the object passed to this.store.setPending
which stores unencrypted fields alongside __encrypted; instead pass only the
minimal metadata and the encrypted blob (e.g., sessionId, elicitId or whatever
store needs) plus __encrypted produced by encryptElicitationData, removing all
original plaintext fields from the payload, and adjust the cast/type
(PendingElicitRecord) expected by store.setPending accordingly so only the
encrypted blob and approved metadata are persisted.
| /** | ||
| * Decrypt a pending record. | ||
| */ | ||
| private async decryptPendingRecord( | ||
| stored: PendingElicitRecord, | ||
| sessionId: string, | ||
| ): Promise<PendingElicitRecord | null> { | ||
| // Check if the record has an encrypted blob | ||
| const encryptedBlob = (stored as unknown as { __encrypted?: ElicitationEncryptedBlob }).__encrypted; | ||
|
|
||
| if (!encryptedBlob || !isEncryptedBlob(encryptedBlob)) { | ||
| this.logger?.warn('[EncryptedElicitationStore] Pending record is not encrypted', { | ||
| sessionId: sessionId.slice(0, 8) + '...', | ||
| }); | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const decrypted = await decryptElicitationData<PendingElicitRecord>(encryptedBlob, sessionId, this.secret); | ||
| if (decrypted) { | ||
| this.logger?.debug('[EncryptedElicitationStore] Decrypted pending record', { | ||
| sessionId: sessionId.slice(0, 8) + '...', | ||
| elicitId: decrypted.elicitId, | ||
| }); | ||
| return decrypted; | ||
| } | ||
| this.logger?.warn('[EncryptedElicitationStore] Failed to decrypt pending record', { | ||
| sessionId: sessionId.slice(0, 8) + '...', | ||
| }); | ||
| return null; | ||
| } catch (error) { | ||
| this.logger?.warn('[EncryptedElicitationStore] Decryption error for pending record', { | ||
| sessionId: sessionId.slice(0, 8) + '...', | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Documentation claims plaintext fallback, but implementation returns null.
The file header (lines 13-16) states:
"Reads attempt decryption first, falls back to plaintext for migration"
However, decryptPendingRecord returns null when the record isn't encrypted (lines 392-397), not the plaintext record. This breaks migration from unencrypted to encrypted stores.
🔧 Return plaintext for migration support
private async decryptPendingRecord(
stored: PendingElicitRecord,
sessionId: string,
): Promise<PendingElicitRecord | null> {
// Check if the record has an encrypted blob
const encryptedBlob = (stored as unknown as { __encrypted?: ElicitationEncryptedBlob }).__encrypted;
if (!encryptedBlob || !isEncryptedBlob(encryptedBlob)) {
- this.logger?.warn('[EncryptedElicitationStore] Pending record is not encrypted', {
+ // Migration fallback: return plaintext record for unencrypted data
+ this.logger?.debug('[EncryptedElicitationStore] Returning plaintext pending record (migration)', {
sessionId: sessionId.slice(0, 8) + '...',
});
- return null;
+ return stored;
}📝 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.
| /** | |
| * Decrypt a pending record. | |
| */ | |
| private async decryptPendingRecord( | |
| stored: PendingElicitRecord, | |
| sessionId: string, | |
| ): Promise<PendingElicitRecord | null> { | |
| // Check if the record has an encrypted blob | |
| const encryptedBlob = (stored as unknown as { __encrypted?: ElicitationEncryptedBlob }).__encrypted; | |
| if (!encryptedBlob || !isEncryptedBlob(encryptedBlob)) { | |
| this.logger?.warn('[EncryptedElicitationStore] Pending record is not encrypted', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| }); | |
| return null; | |
| } | |
| try { | |
| const decrypted = await decryptElicitationData<PendingElicitRecord>(encryptedBlob, sessionId, this.secret); | |
| if (decrypted) { | |
| this.logger?.debug('[EncryptedElicitationStore] Decrypted pending record', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| elicitId: decrypted.elicitId, | |
| }); | |
| return decrypted; | |
| } | |
| this.logger?.warn('[EncryptedElicitationStore] Failed to decrypt pending record', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| }); | |
| return null; | |
| } catch (error) { | |
| this.logger?.warn('[EncryptedElicitationStore] Decryption error for pending record', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| return null; | |
| } | |
| } | |
| /** | |
| * Decrypt a pending record. | |
| */ | |
| private async decryptPendingRecord( | |
| stored: PendingElicitRecord, | |
| sessionId: string, | |
| ): Promise<PendingElicitRecord | null> { | |
| // Check if the record has an encrypted blob | |
| const encryptedBlob = (stored as unknown as { __encrypted?: ElicitationEncryptedBlob }).__encrypted; | |
| if (!encryptedBlob || !isEncryptedBlob(encryptedBlob)) { | |
| // Migration fallback: return plaintext record for unencrypted data | |
| this.logger?.debug('[EncryptedElicitationStore] Returning plaintext pending record (migration)', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| }); | |
| return stored; | |
| } | |
| try { | |
| const decrypted = await decryptElicitationData<PendingElicitRecord>(encryptedBlob, sessionId, this.secret); | |
| if (decrypted) { | |
| this.logger?.debug('[EncryptedElicitationStore] Decrypted pending record', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| elicitId: decrypted.elicitId, | |
| }); | |
| return decrypted; | |
| } | |
| this.logger?.warn('[EncryptedElicitationStore] Failed to decrypt pending record', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| }); | |
| return null; | |
| } catch (error) { | |
| this.logger?.warn('[EncryptedElicitationStore] Decryption error for pending record', { | |
| sessionId: sessionId.slice(0, 8) + '...', | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@libs/sdk/src/elicitation/store/encrypted-elicitation.store.ts` around lines
382 - 419, The method decryptPendingRecord currently treats records without an
__encrypted blob as an error and returns null, but the module header promises a
plaintext fallback for migration; update decryptPendingRecord (function name) so
that when encryptedBlob is missing or isEncryptedBlob(encryptedBlob) is false it
returns the original stored PendingElicitRecord (not null), and adjust the
logger message to reflect a plaintext fallback (use sessionId and
stored.elicitId where available) so callers receive the plaintext record for
migration to encrypted storage.
| /** | ||
| * Publish an elicitation result. | ||
| */ | ||
| async publishResult<T = unknown>(elicitId: string, sessionId: string, result: ElicitResult<T>): Promise<void> { | ||
| // First, invoke local callbacks directly for same-node responses | ||
| const callbacks = this.localCallbacks.get(elicitId); | ||
| if (callbacks) { | ||
| for (const cb of callbacks) { | ||
| try { | ||
| cb(result); | ||
| } catch (err) { | ||
| this.logger?.error('[StorageElicitationStore] Callback error during publish', { | ||
| elicitId, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Publish to channel for cross-node routing | ||
| if (this.storage.supportsPubSub()) { | ||
| const channel = RESULT_CHANNEL_PREFIX + elicitId; | ||
| try { | ||
| await this.storage.publish(channel, JSON.stringify(result)); | ||
| } catch (err) { | ||
| this.logger?.error('[StorageElicitationStore] Failed to publish result', { | ||
| elicitId, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Clean up the pending record | ||
| await this.deletePending(sessionId); | ||
|
|
||
| this.logger?.debug('[StorageElicitationStore] Published result', { elicitId, sessionId, status: result.status }); | ||
| } |
There was a problem hiding this comment.
Potential double-delivery for same-node subscribers in publishResult.
Local callbacks are invoked directly (lines 268-280), then the result is also published to the pub/sub channel (lines 282-293). If the subscriber is on the same node AND subscribed via pub/sub, they'll receive the result twice.
Compare with publishFallbackResult (lines 503-546) which correctly uses pub/sub OR local callbacks, not both.
🔧 Suggested fix to match publishFallbackResult pattern
async publishResult<T = unknown>(elicitId: string, sessionId: string, result: ElicitResult<T>): Promise<void> {
- // First, invoke local callbacks directly for same-node responses
- const callbacks = this.localCallbacks.get(elicitId);
- if (callbacks) {
- for (const cb of callbacks) {
- try {
- cb(result);
- } catch (err) {
- this.logger?.error('[StorageElicitationStore] Callback error during publish', {
- elicitId,
- error: err instanceof Error ? err.message : String(err),
- });
- }
- }
- }
-
- // Publish to channel for cross-node routing
if (this.storage.supportsPubSub()) {
+ // Publish to channel - pub/sub handles both local and remote delivery
const channel = RESULT_CHANNEL_PREFIX + elicitId;
try {
await this.storage.publish(channel, JSON.stringify(result));
} catch (err) {
this.logger?.error('[StorageElicitationStore] Failed to publish result', {
elicitId,
error: err instanceof Error ? err.message : String(err),
});
}
+ } else {
+ // No pub/sub - invoke local callbacks directly
+ const callbacks = this.localCallbacks.get(elicitId);
+ if (callbacks) {
+ for (const cb of callbacks) {
+ try {
+ cb(result);
+ } catch (err) {
+ this.logger?.error('[StorageElicitationStore] Callback error during publish', {
+ elicitId,
+ error: err instanceof Error ? err.message : String(err),
+ });
+ }
+ }
+ }
}
// Clean up the pending record
await this.deletePending(sessionId);🤖 Prompt for AI Agents
In `@libs/sdk/src/elicitation/store/storage-elicitation.store.ts` around lines 263
- 299, In publishResult, avoid double-delivery by using the same pattern as
publishFallbackResult: if local callbacks exist for elicitId
(this.localCallbacks.get(elicitId)), invoke them and skip publishing to pub/sub;
otherwise, if this.storage.supportsPubSub() publish the JSON result to
RESULT_CHANNEL_PREFIX + elicitId; always call this.deletePending(sessionId) and
log via this.logger afterwards; update the conditional flow in publishResult to
choose either local callback invocation or storage.publish, not both.
| .catch((err) => { | ||
| if (resolved) return; | ||
| resolved = true; | ||
|
|
||
| this.logger.error('handleWaitingFallback: failed to subscribe', { | ||
| elicitId, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
|
|
||
| cleanup().then(() => { | ||
| reject( | ||
| new ToolExecutionError( | ||
| error.toolName, | ||
| new Error( | ||
| `Failed to subscribe to fallback result: ${err instanceof Error ? err.message : String(err)}`, | ||
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Clean up pending fallback on subscription failure.
If subscribing to the fallback result fails, the pending record can remain orphaned; consider deleting it to avoid stale entries.
🐛 Suggested cleanup on subscribe failure
.catch((err) => {
if (resolved) return;
resolved = true;
this.logger.error('handleWaitingFallback: failed to subscribe', {
elicitId,
error: err instanceof Error ? err.message : String(err),
});
- cleanup().then(() => {
- reject(
- new ToolExecutionError(
- error.toolName,
- new Error(
- `Failed to subscribe to fallback result: ${err instanceof Error ? err.message : String(err)}`,
- ),
- ),
- );
- });
+ cleanup()
+ .then(async () => {
+ try {
+ await scope.elicitationStore.deletePendingFallback(elicitId);
+ } catch (cleanupErr) {
+ this.logger.warn('handleWaitingFallback: failed to cleanup pending fallback', {
+ elicitId,
+ error: cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr),
+ });
+ }
+ reject(
+ new ToolExecutionError(
+ error.toolName,
+ new Error(
+ `Failed to subscribe to fallback result: ${err instanceof Error ? err.message : String(err)}`,
+ ),
+ ),
+ );
+ });
});🤖 Prompt for AI Agents
In `@libs/sdk/src/tool/flows/call-tool.flow.ts` around lines 649 - 667, When
subscribe to the fallback result fails inside handleWaitingFallback, ensure the
pending fallback record is removed before rejecting: call and await the cleanup
routine (or explicitly invoke the pending-delete helper for elicitId) in the
catch block so the pending record is deleted, log the elicitId and any cleanup
errors via this.logger.error, then reject with the ToolExecutionError
(preserving the original error message); reference handleWaitingFallback,
cleanup(), elicitId, resolved and ToolExecutionError when making the change.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.