feat(core): implement MCP elicitation (form + url) capability#28089
feat(core): implement MCP elicitation (form + url) capability#28089KittiphonKamnuan wants to merge 6 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements the Model Context Protocol (MCP) elicitation specification (2025-11-25) within the core client. By enabling form and URL-based elicitation, the client can now support complex human-in-the-loop interactions and third-party authentication flows. The changes include protocol capability advertisement, a new request handler for elicitation, and a retry path for tool calls that require external user input, ensuring that servers can gracefully request information without blocking execution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/L
|
|
/gemini review |
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
KittiphonKamnuan
had a problem deploying
to
eval-gate
GitHub Actions
Error
Advertise `elicitation: { form: {}, url: {} }` at initialize per the MCP
2025-11-25 spec and implement both modes in the MCP client:
- Register an `elicitation/create` request handler that routes form-mode and
url-mode requests to the UI via the existing MessageBus. New message types
`MCP_ELICITATION_REQUEST` / `MCP_ELICITATION_RESPONSE` carry the payload.
- In `McpCallableTool.callTool`, catch `UrlElicitationRequiredError`
(code -32042), drive the URL elicitation flow over the MessageBus, then
resume/retry the tool call. The retry loop is bounded by
`MAX_URL_ELICITATION_ROUNDS` to avoid unbounded re-elicitation.
- Thread the MessageBus from `McpClientManager` through `McpClient` into
`connectToMcpServer` so the elicitation handler can publish requests.
This addresses google-gemini#28074 (consolidating the previously-deferred google-gemini#15613 and
google-gemini#22249). Form rendering and URL opening on the CLI side will follow in a
companion PR; this PR lands the core protocol surface and the retry path.
KittiphonKamnuan
force-pushed
the
feature/mcp-elicitation-capability
branch
from
7a3fe4f to
f473f8d
Compare
KittiphonKamnuan
had a problem deploying
to
eval-gate
GitHub Actions
Error
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for MCP elicitation requests (both form and URL modes) and adds a retry mechanism for tool calls requiring URL elicitation. The feedback focuses on security and type safety: first, validating and sanitizing URLs received from the MCP server before forwarding them to the message bus to prevent potential security vulnerabilities; second, refactoring the newly introduced McpElicitationRequest and McpElicitationResponse interfaces into discriminated unions to enforce stricter type safety.
| mcpClient.setRequestHandler(ElicitRequestSchema, async (request) => { | ||
| if (!messageBus) { | ||
| // Without a message bus we cannot route the request to the UI, so | ||
| // decline rather than silently hanging the server. | ||
| return { action: 'decline' } as ElicitResult; | ||
| } | ||
|
|
||
| const params = request.params; | ||
| const isUrlMode = params.mode === 'url'; | ||
|
|
||
| const elicitationRequest: Omit<McpElicitationRequest, 'correlationId'> = | ||
| isUrlMode | ||
| ? { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: mcpServerName, | ||
| mode: 'url', | ||
| message: params.message, | ||
| elicitationId: (params as ElicitRequestURLParams).elicitationId, | ||
| url: (params as ElicitRequestURLParams).url, | ||
| } | ||
| : { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: mcpServerName, | ||
| mode: 'form', | ||
| message: params.message, | ||
| requestedSchema: (params as ElicitRequestFormParams) | ||
| .requestedSchema, | ||
| }; | ||
|
|
||
| const response = await messageBus.request< | ||
| McpElicitationRequest, | ||
| McpElicitationResponse | ||
| >( | ||
| elicitationRequest, | ||
| MessageBusType.MCP_ELICITATION_RESPONSE, | ||
| mcpServerConfig.timeout ?? MCP_DEFAULT_TIMEOUT_MSEC, | ||
| ); | ||
|
|
||
| const result: ElicitResult = { action: response.action }; | ||
| if (response.action === 'accept' && response.content) { | ||
| result.content = response.content; | ||
| } | ||
| return result; | ||
| }); |
There was a problem hiding this comment.
The MCP elicitation request handler receives a URL from the MCP server and forwards it directly to the message bus without any validation or sanitization. A malicious or compromised MCP server could send a dangerous URL (e.g., using javascript:, file:, or other dangerous protocols), which could lead to arbitrary code execution, local file disclosure, or phishing when opened by the UI/CLI. Please validate that the URL uses a safe protocol (e.g., http: or https:) before forwarding it to the message bus.
mcpClient.setRequestHandler(ElicitRequestSchema, async (request) => {
if (!messageBus) {
// Without a message bus we cannot route the request to the UI, so
// decline rather than silently hanging the server.
return { action: 'decline' } as ElicitResult;
}
const params = request.params;
const isUrlMode = params.mode === 'url';
if (isUrlMode) {
const urlStr = (params as ElicitRequestURLParams).url;
try {
const parsedUrl = new URL(urlStr);
if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') {
return { action: 'decline' } as ElicitResult;
}
} catch {
return { action: 'decline' } as ElicitResult;
}
}
const elicitationRequest: Omit<McpElicitationRequest, 'correlationId'> =
isUrlMode
? {
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: mcpServerName,
mode: 'url',
message: params.message,
elicitationId: (params as ElicitRequestURLParams).elicitationId,
url: (params as ElicitRequestURLParams).url,
}
: {
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: mcpServerName,
mode: 'form',
message: params.message,
requestedSchema: (params as ElicitRequestFormParams)
.requestedSchema,
};
const response = await messageBus.request<
McpElicitationRequest,
McpElicitationResponse
>(
elicitationRequest,
MessageBusType.MCP_ELICITATION_RESPONSE,
mcpServerConfig.timeout ?? MCP_DEFAULT_TIMEOUT_MSEC,
);
const result: ElicitResult = { action: response.action };
if (response.action === 'accept' && response.content) {
result.content = response.content;
}
return result;
});| private async runUrlElicitations( | ||
| elicitations: ElicitRequestURLParams[], | ||
| ): Promise<void> { | ||
| if (!this.messageBus) return; | ||
| for (const elicitation of elicitations) { | ||
| await this.messageBus.request< | ||
| McpElicitationRequest, | ||
| McpElicitationResponse | ||
| >( | ||
| { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: this.serverName, | ||
| mode: 'url', | ||
| message: elicitation.message, | ||
| elicitationId: elicitation.elicitationId, | ||
| url: elicitation.url, | ||
| }, | ||
| MessageBusType.MCP_ELICITATION_RESPONSE, | ||
| this.timeout, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The runUrlElicitations method receives a list of elicitations containing URLs from the MCP server and forwards them directly to the message bus without any validation or sanitization. A malicious or compromised MCP server could send a dangerous URL (e.g., using javascript:, file:, or other dangerous protocols), which could lead to arbitrary code execution, local file disclosure, or phishing when opened by the UI/CLI. Please validate that the URL uses a safe protocol (e.g., http: or https:) before forwarding it to the message bus.
private async runUrlElicitations(
elicitations: ElicitRequestURLParams[],
): Promise<void> {
if (!this.messageBus) return;
for (const elicitation of elicitations) {
try {
const parsedUrl = new URL(elicitation.url);
if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') {
continue;
}
} catch {
continue;
}
await this.messageBus.request<
McpElicitationRequest,
McpElicitationResponse
>(
{
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: this.serverName,
mode: 'url',
message: elicitation.message,
elicitationId: elicitation.elicitationId,
url: elicitation.url,
},
MessageBusType.MCP_ELICITATION_RESPONSE,
this.timeout,
);
}
}| export interface McpElicitationRequest { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST; | ||
| correlationId: string; | ||
| serverName: string; | ||
| mode: 'form' | 'url'; | ||
| message: string; | ||
| /** Present when `mode === 'form'`. JSON Schema describing requested fields. */ | ||
| requestedSchema?: unknown; | ||
| /** Present when `mode === 'url'`. Opaque server-issued id used to correlate completion. */ | ||
| elicitationId?: string; | ||
| /** Present when `mode === 'url'`. URL the user should open. */ | ||
| url?: string; | ||
| } |
There was a problem hiding this comment.
To improve type safety and align with the discriminated union pattern used elsewhere, McpElicitationRequest should be defined as a type using a discriminated union on the mode property. This will ensure that mode-specific properties (requestedSchema for form, and elicitationId/url for url) are correctly required and typed, preventing potential runtime errors.
export type McpElicitationRequest = {
type: MessageBusType.MCP_ELICITATION_REQUEST;
correlationId: string;
serverName: string;
message: string;
} & (
| {
mode: 'form';
/** JSON Schema describing requested fields. */
requestedSchema: unknown;
}
| {
mode: 'url';
/** Opaque server-issued id used to correlate completion. */
elicitationId: string;
/** URL the user should open. */
url: string;
}
);| export interface McpElicitationResponse { | ||
| type: MessageBusType.MCP_ELICITATION_RESPONSE; | ||
| correlationId: string; | ||
| action: 'accept' | 'decline' | 'cancel'; | ||
| /** Form values when `action === 'accept'` for form-mode requests. */ | ||
| content?: Record<string, string | number | boolean | string[]>; | ||
| } |
There was a problem hiding this comment.
For better type safety, McpElicitationResponse should be a discriminated union based on the action property. The content property is only relevant when action is 'accept'. This change will make the type definition stricter and prevent accessing content on responses where it's not applicable.
export type McpElicitationResponse = {
type: MessageBusType.MCP_ELICITATION_RESPONSE;
correlationId: string;
} & (
| {
action: 'accept';
/** Form values when `action === 'accept'` for form-mode requests. */
content?: Record<string, string | number | boolean | string[]>;
}
| {
action: 'decline' | 'cancel';
}
);
There was a problem hiding this comment.
Code Review
This pull request implements support for Model Context Protocol (MCP) elicitation requests in both form and URL modes, allowing MCP servers to elicit information from the user via the message bus. The changes include defining new message types, registering elicitation request handlers, and implementing retry logic for tool calls requiring URL elicitation. Feedback from the review highlights critical security vulnerabilities where server-provided URLs are forwarded to the UI without validation, potentially leading to Remote Code Execution (RCE) or phishing. To address this, the reviewer suggests strictly validating URL protocols (restricting to HTTP/HTTPS), properly handling user cancellation or decline responses during retries, and using an AbortSignal instead of a timeout for asynchronous operations.
| for (const elicitation of elicitations) { | ||
| await this.messageBus.request< | ||
| McpElicitationRequest, | ||
| McpElicitationResponse | ||
| >( | ||
| { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: this.serverName, | ||
| mode: 'url', | ||
| message: elicitation.message, | ||
| elicitationId: elicitation.elicitationId, | ||
| url: elicitation.url, | ||
| }, | ||
| MessageBusType.MCP_ELICITATION_RESPONSE, | ||
| this.timeout, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The elicitation.url is forwarded directly to the UI via the message bus without any validation or sanitization. This poses a significant security risk, as a malicious or compromised MCP server could supply a dangerous URL (e.g., using file:, javascript:, data:, or custom OS protocol handlers). Such URLs, when opened by the UI, could lead to Remote Code Execution (RCE), Local File Disclosure, or phishing. The provided code suggestion addresses this by validating that the URL strictly uses the 'http:' or 'https:' protocol before forwarding it.
Additionally, the current logic in runUrlElicitations does not properly handle user 'decline' or 'cancel' responses to URL elicitation requests. This oversight causes invokeWithUrlElicitationRetry to proceed with retrying the tool call, leading to repeated prompts for the user even after they have explicitly chosen to decline or cancel. It is recommended that runUrlElicitations be updated to return a boolean indicating whether all elicitations were successfully accepted, and invokeWithUrlElicitationRetry should abort and throw an error immediately if any elicitation is declined or cancelled.
Furthermore, when waiting for user input via the MessageBus, rely on the provided AbortSignal for cancellation instead of using a separate timeout (this.timeout) to maintain consistency with existing patterns.
for (const elicitation of elicitations) {
try {
const parsed = new URL(elicitation.url);
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
continue;
}
} catch {
continue;
}
await this.messageBus.request<
McpElicitationRequest,
McpElicitationResponse
>(
{
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: this.serverName,
mode: 'url',
message: elicitation.message,
elicitationId: elicitation.elicitationId,
url: elicitation.url,
},
MessageBusType.MCP_ELICITATION_RESPONSE,
signal,
);
}References
- Asynchronous operations waiting for user input via the MessageBus should rely on the provided AbortSignal for cancellation, rather than implementing a separate timeout, to maintain consistency with existing patterns.
| const params = request.params; | ||
| const isUrlMode = params.mode === 'url'; | ||
|
|
There was a problem hiding this comment.
The MCP client receives elicitation requests from the MCP server. If the elicitation mode is 'url', the server-provided URL (params.url) is forwarded directly to the UI via the message bus without any validation or sanitization. A malicious or compromised MCP server could supply a dangerous URL (e.g., using file:, javascript:, data:, or custom OS protocol handlers) which, when opened by the UI, could lead to Remote Code Execution (RCE), Local File Disclosure, or phishing. Please validate that the URL strictly uses the 'http:' or 'https:' protocol before forwarding it.
const params = request.params;
const isUrlMode = params.mode === 'url';
if (isUrlMode) {
const rawUrl = (params as ElicitRequestURLParams).url;
try {
const parsed = new URL(rawUrl);
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
return { action: 'decline' } as ElicitResult;
}
} catch {
return { action: 'decline' } as ElicitResult;
}
}Apply review feedback from gemini-code-assist on google-gemini#28089: - **Security**: An untrusted MCP server could return a `javascript:`, `file:`, or `data:` URL via `elicitation/create` (url-mode) or via `UrlElicitationRequiredError` (-32042). The handler now rejects any URL whose protocol is not `http(s):` before forwarding it to the MessageBus — `elicitation/create` responds with `{ action: 'decline' }`, the retry path silently skips the unsafe entry. - **Type safety**: `McpElicitationRequest` and `McpElicitationResponse` are now discriminated unions on `mode` / `action`, so mode-specific fields (`requestedSchema`, `elicitationId`, `url`) and `accept`-only `content` are correctly required and narrowed. - Add table-driven tests covering `javascript:`, `file:`, `data:`, and malformed-URL inputs.
|
Thanks for the review @gemini-code-assist! Addressed all four findings in 99010bc: Security (2/2)
Type safety (2/2)
All 136 unit tests still pass ( |
KittiphonKamnuan
had a problem deploying
to
eval-gate
GitHub Actions
Error
|
The updates to |
Follow-up to gemini-code-assist review on google-gemini#28089. Previously `runUrlElicitations` discarded the user's response: if the user declined or cancelled, the retry loop would re-fire the tool call, which would raise the same -32042 error and prompt the user again until `MAX_URL_ELICITATION_ROUNDS` was exhausted. `runUrlElicitations` now returns a boolean — `true` only if every elicitation was accepted. `invokeWithUrlElicitationRetry` aborts immediately with a clear error message when the user declined, rather than re-prompting them on every round. Note: the reviewer also suggested replacing the timeout argument to `messageBus.request` with an `AbortSignal`. The current `MessageBus.request` signature only accepts `timeoutMs`, so that refactor is left to a follow-up that touches `message-bus.ts`.
|
Second pass on the review feedback (commit 3c06ce4): Decline/cancel handling —
URL validation (suggested again on the elicitation handler at L1996) was already added in the previous commit (99010bc) — see |
KittiphonKamnuan
had a problem deploying
to
eval-gate
GitHub Actions
Error
|
/gemini review |
gemini-cli
Bot
added
priority/p2
There was a problem hiding this comment.
Code Review
This pull request introduces support for Model Context Protocol (MCP) elicitation requests (form and URL modes), allowing MCP servers to elicit information from the user via the message bus. It includes safety checks to reject unsafe protocols, retry logic for URL elicitation, and comprehensive unit tests. The review feedback focuses on improving robustness by adding defensive checks for malformed payloads, wrapping message bus requests in try-catch blocks to handle errors gracefully, and using an AbortSignal for cancellation instead of a timeout to maintain consistency with existing patterns.
| private async runUrlElicitations( | ||
| elicitations: ElicitRequestURLParams[], | ||
| ): Promise<boolean> { | ||
| if (!this.messageBus) return false; | ||
| for (const elicitation of elicitations) { | ||
| if (!isSafeElicitationUrl(elicitation.url)) { | ||
| // Untrusted MCP server sent an unsafe URL; treat as declined so | ||
| // we don't surface `javascript:`/`file:`/etc. to the UI and don't | ||
| // retry in a loop. | ||
| return false; | ||
| } | ||
| const elicitationRequest: McpElicitationRequestWithoutCorrelationId = { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: this.serverName, | ||
| mode: 'url', | ||
| message: elicitation.message, | ||
| elicitationId: elicitation.elicitationId, | ||
| url: elicitation.url, | ||
| }; | ||
| const response = await this.messageBus.request< | ||
| McpElicitationRequest, | ||
| McpElicitationResponse | ||
| >( | ||
| elicitationRequest as Omit<McpElicitationRequest, 'correlationId'>, | ||
| MessageBusType.MCP_ELICITATION_RESPONSE, | ||
| this.timeout, | ||
| ); | ||
| if (response.action !== 'accept') { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Add defensive checks to ensure elicitations is defined before iterating over it. Additionally, wrap the elicitation loop in a try-catch block to handle any message bus errors gracefully, logging the detailed error for debugging and returning false to signal a clean cancellation. To maintain consistency with existing patterns, rely on the provided AbortSignal for cancellation rather than implementing a separate timeout.
private async runUrlElicitations(
elicitations: ElicitRequestURLParams[],
signal?: AbortSignal,
): Promise<boolean> {
if (!this.messageBus || !elicitations) return false;
try {
for (const elicitation of elicitations) {
if (!isSafeElicitationUrl(elicitation.url)) {
// Untrusted MCP server sent an unsafe URL; treat as declined so
// we don't surface `javascript:`/`file:`/etc. to the UI and don't
// retry in a loop.
return false;
}
const elicitationRequest: McpElicitationRequestWithoutCorrelationId = {
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: this.serverName,
mode: 'url',
message: elicitation.message,
elicitationId: elicitation.elicitationId,
url: elicitation.url,
};
const response = await this.messageBus.request<
McpElicitationRequest,
McpElicitationResponse
>(
elicitationRequest as Omit<McpElicitationRequest, 'correlationId'>,
MessageBusType.MCP_ELICITATION_RESPONSE,
undefined,
signal,
);
if (response.action !== 'accept') {
return false;
}
}
return true;
} catch (error) {
console.error('Failed to run URL elicitations:', error);
return false;
}
}References
- Asynchronous operations waiting for user input via the MessageBus should rely on the provided AbortSignal for cancellation, rather than implementing a separate timeout, to maintain consistency with existing patterns.
- When catching exceptions, log the detailed error for debugging instead of providing only a generic error message.
| mcpClient.setRequestHandler(ElicitRequestSchema, async (request) => { | ||
| if (!messageBus) { | ||
| // Without a message bus we cannot route the request to the UI, so | ||
| // decline rather than silently hanging the server. | ||
| return { action: 'decline' } as ElicitResult; | ||
| } | ||
|
|
||
| const params = request.params; | ||
| const isUrlMode = params.mode === 'url'; | ||
|
|
||
| if (isUrlMode) { | ||
| const urlStr = (params as ElicitRequestURLParams).url; | ||
| if (!isSafeElicitationUrl(urlStr)) { | ||
| // Untrusted MCP server tried to coerce the UI into opening a | ||
| // dangerous URL (e.g. `javascript:`/`file:`/`data:`). Decline | ||
| // rather than forward it. | ||
| return { action: 'decline' } as ElicitResult; | ||
| } | ||
| } |
There was a problem hiding this comment.
Defensively guard against missing or malformed request or request.params payloads from the MCP server. Additionally, validate that the elicitation mode is either 'url' or 'form' to prevent runtime TypeError exceptions or unexpected behavior when handling unsupported modes.
mcpClient.setRequestHandler(ElicitRequestSchema, async (request) => {
if (!messageBus) {
// Without a message bus we cannot route the request to the UI, so
// decline rather than silently hanging the server.
return { action: 'decline' } as ElicitResult;
}
const params = request?.params;
if (!params) {
return { action: 'decline' } as ElicitResult;
}
const isUrlMode = params.mode === 'url';
const isFormMode = params.mode === 'form';
if (!isUrlMode && !isFormMode) {
return { action: 'decline' } as ElicitResult;
}
if (isUrlMode) {
const urlStr = (params as ElicitRequestURLParams).url;
if (!isSafeElicitationUrl(urlStr)) {
// Untrusted MCP server tried to coerce the UI into opening a
// dangerous URL (e.g. `javascript:`/`file:`/`data:`). Decline
// rather than forward it.
return { action: 'decline' } as ElicitResult;
}
}| const response = await messageBus.request< | ||
| McpElicitationRequest, | ||
| McpElicitationResponse | ||
| >( | ||
| elicitationRequest as Omit<McpElicitationRequest, 'correlationId'>, | ||
| MessageBusType.MCP_ELICITATION_RESPONSE, | ||
| mcpServerConfig.timeout ?? MCP_DEFAULT_TIMEOUT_MSEC, | ||
| ); | ||
|
|
||
| const result: ElicitResult = { action: response.action }; | ||
| if (response.action === 'accept' && response.content) { | ||
| result.content = response.content; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Wrap the messageBus.request call in a try-catch block. If the elicitation request fails on the message bus, log the detailed error for debugging and return an ElicitResult with action: "decline" or action: "cancel" to allow the server to handle the failure gracefully. Additionally, rely on the provided AbortSignal for cancellation instead of a separate timeout to maintain consistency with existing patterns.
try {
const response = await messageBus.request<
McpElicitationRequest,
McpElicitationResponse
>(
elicitationRequest as Omit<McpElicitationRequest, 'correlationId'>,
MessageBusType.MCP_ELICITATION_RESPONSE,
undefined,
signal,
);
const result: ElicitResult = { action: response.action };
if (response.action === 'accept' && response.content) {
result.content = response.content;
}
return result;
} catch (error) {
console.error('Elicitation request failed:', error);
return { action: 'decline' } as ElicitResult;
}References
- Asynchronous operations waiting for user input via the MessageBus should rely on the provided AbortSignal for cancellation, rather than implementing a separate timeout, to maintain consistency with existing patterns.
- When catching exceptions, log the detailed error for debugging instead of providing only a generic error message.
Follow-up to gemini-code-assist review on google-gemini#28089. In the `elicitation/create` handler: - Guard against missing or non-object `request.params`. - Reject unknown `mode` values up front (treat absence as form, per spec). - Wrap `messageBus.request` in try/catch — log via `debugLogger.error` (the project's logger, not `console.error`) and decline on failure so the server gets a clean response instead of hanging. In `runUrlElicitations`: - Guard against undefined / empty `elicitations`. - Wrap the loop in try/catch with the same logging pattern; treat any bus failure as a declined elicitation so the retry loop aborts. Add tests for malformed payloads, unknown modes, and bus failures.
|
Third pass on the review (commit 51dd395): Defensive guards — Error handling — Both the handler and AbortSignal — Still deferred. 84/84 tests passing (3 new), lint and tsc clean. |
KittiphonKamnuan
had a problem deploying
to
eval-gate
GitHub Actions
Error
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements support for MCP elicitation requests (elicitation/create) in both form and url modes, introducing new message types to the confirmation bus, registering request handlers, and adding retry logic with user prompts in McpCallableTool. The review feedback recommends defensively validating that url and elicitationId are valid strings in both the request handler and retry logic, as well as trimming the optional message string with a fallback to avoid whitespace-only or undefined values.
| if (isUrlMode) { | ||
| const urlStr = (params as ElicitRequestURLParams).url; | ||
| if (!isSafeElicitationUrl(urlStr)) { | ||
| // Untrusted MCP server tried to coerce the UI into opening a | ||
| // dangerous URL (e.g. `javascript:`/`file:`/`data:`). Decline | ||
| // rather than forward it. | ||
| return { action: 'decline' } as ElicitResult; | ||
| } | ||
| } | ||
|
|
||
| const elicitationRequest: McpElicitationRequestWithoutCorrelationId = | ||
| isUrlMode | ||
| ? { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: mcpServerName, | ||
| mode: 'url', | ||
| message: params.message, | ||
| elicitationId: (params as ElicitRequestURLParams).elicitationId, | ||
| url: (params as ElicitRequestURLParams).url, | ||
| } | ||
| : { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: mcpServerName, | ||
| mode: 'form', | ||
| message: params.message, | ||
| requestedSchema: (params as ElicitRequestFormParams) | ||
| .requestedSchema, | ||
| }; |
There was a problem hiding this comment.
Defensively validate that url and elicitationId are valid strings when in url mode, and provide a fallback for message since it is optional in the MCP specification. When using an optional string with a fallback value, trim the optional string and use the fallback if the result is empty to avoid uninformative messages from whitespace-only strings.
if (isUrlMode) {
const urlStr = (params as ElicitRequestURLParams).url;
const elicitationId = (params as ElicitRequestURLParams).elicitationId;
if (
typeof urlStr !== 'string' ||
typeof elicitationId !== 'string' ||
!isSafeElicitationUrl(urlStr)
) {
// Untrusted MCP server tried to coerce the UI into opening a
// dangerous URL or sent malformed parameters. Decline rather than forward it.
return { action: 'decline' } as ElicitResult;
}
}
const elicitationRequest: McpElicitationRequestWithoutCorrelationId =
isUrlMode
? {
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: mcpServerName,
mode: 'url',
message: (params as ElicitRequestURLParams).message?.trim() || '',
elicitationId: (params as ElicitRequestURLParams).elicitationId,
url: (params as ElicitRequestURLParams).url,
}
: {
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: mcpServerName,
mode: 'form',
message: (params as ElicitRequestFormParams).message?.trim() || '',
requestedSchema: (params as ElicitRequestFormParams)
.requestedSchema,
};References
- When using an optional string with a fallback value, trim the optional string and use the fallback if the result is empty to avoid uninformative messages from whitespace-only strings.
| for (const elicitation of elicitations) { | ||
| if (!isSafeElicitationUrl(elicitation.url)) { | ||
| // Untrusted MCP server sent an unsafe URL; treat as declined so | ||
| // we don't surface `javascript:`/`file:`/etc. to the UI and don't | ||
| // retry in a loop. | ||
| return false; | ||
| } | ||
| const elicitationRequest: McpElicitationRequestWithoutCorrelationId = { | ||
| type: MessageBusType.MCP_ELICITATION_REQUEST, | ||
| serverName: this.serverName, | ||
| mode: 'url', | ||
| message: elicitation.message, | ||
| elicitationId: elicitation.elicitationId, | ||
| url: elicitation.url, | ||
| }; |
There was a problem hiding this comment.
Ensure that url and elicitationId are validated as strings before processing them, and provide a fallback for message to prevent passing undefined to the McpElicitationRequest which requires a string. When using an optional string with a fallback value, trim the optional string and use the fallback if the result is empty to avoid uninformative messages from whitespace-only strings.
for (const elicitation of elicitations) {
if (
typeof elicitation.url !== 'string' ||
typeof elicitation.elicitationId !== 'string' ||
!isSafeElicitationUrl(elicitation.url)
) {
// Untrusted MCP server sent an unsafe URL or malformed parameters; treat as declined.
return false;
}
const elicitationRequest: McpElicitationRequestWithoutCorrelationId = {
type: MessageBusType.MCP_ELICITATION_REQUEST,
serverName: this.serverName,
mode: 'url',
message: elicitation.message?.trim() || '',
elicitationId: elicitation.elicitationId,
url: elicitation.url,
};References
- When using an optional string with a fallback value, trim the optional string and use the fallback if the result is empty to avoid uninformative messages from whitespace-only strings.
Follow-up to gemini-code-assist review on google-gemini#28089. Add string type guards on the url-mode payload — `url` and `elicitationId` — in both the `elicitation/create` handler and `runUrlElicitations`, so a malformed server response (non-string field) is declined before reaching the UI. Skipped the suggestion to `?.trim() || ''` the `message` field: the MCP Zod schemas require `message: string`, so an undefined or whitespace-only value would already be rejected at the SDK boundary. Add table-driven tests covering non-string `url`, missing `elicitationId`, and non-string `elicitationId`.
|
Fourth pass on the review (commit c83372e): String type guards on url-mode payload — Added Skipped: Added table-driven tests for non-string 87/87 tests passing (3 new). Lint and tsc clean. |
KittiphonKamnuan
had a problem deploying
to
eval-gate
GitHub Actions
Error
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
KittiphonKamnuan
requested a deployment
to
eval-gate
GitHub Actions
Waiting
|
This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
1 participant
Summary
Implements MCP elicitation (
form+urlmodes) in the core MCP client, addressing #28074 (which consolidates the previously-deferred #15613 and #22249).Per the 2025-11-25 spec, the client now:
elicitation: { form: {}, url: {} }atinitialize.elicitation/createfor both form-mode and url-mode requests by routing them through the existingMessageBus(newMCP_ELICITATION_REQUEST/MCP_ELICITATION_RESPONSEmessage types).-32042retry path inMcpCallableTool.callTool: catchesUrlElicitationRequiredError, drives the URL elicitation flow over the bus, and resumes the tool call. The retry loop is bounded byMAX_URL_ELICITATION_ROUNDSto prevent unbounded re-elicitation.MessageBusfromMcpClientManager→McpClient→connectToMcpServerso the handler can publish requests.Why
Quoting #28074: elicitation — especially url-mode — is the spec-native primitive for require-confirmation / human-in-the-loop flows and third-party OAuth/3LO. Without it, servers integrating Slack/Notion/Salesforce on the user's behalf are blocked, and form-mode servers receive
Method not found. VS Code Copilot's MCP client ships url-mode end-to-end as a production reference.What's NOT in this PR
The CLI-side UI (form rendering, URL opener) lives in
packages/cliand will land in a follow-up PR. This PR is the protocol surface + retry path in core, which is what the issue scopes aseffort/mediumagainstmcp-client.ts. Until the CLI subscriber lands, the handler degrades gracefully (returns{ action: 'decline' }when noMessageBusis wired).Files changed
packages/core/src/confirmation-bus/types.ts— new message typespackages/core/src/tools/mcp-client.ts— capability, handler, retry pathpackages/core/src/tools/mcp-client-manager.ts— threadsMessageBuspackages/core/src/tools/mcp-client.test.ts— unit testsTest plan
vitest run packages/core/src/tools/mcp-client.test.ts— 77/77 passing (incl. 2 new)vitest run packages/core/src/tools/mcp-client-manager.test.ts packages/core/src/confirmation-bus/message-bus.test.ts— 55/55 passingeslint— clean on changed filestsc --noEmit— no new errorsReferences
roots) — implement elicitation (form + url) #28074 (after follow-up CLI PR lands)