fix: handle errors due to open dialogs during tool calls (#1953)
This change is part of the fix for #1069 - Errors during tool calls are now consumed and handled as part of McpResponse format
This commit is contained in:
@@ -203,6 +203,7 @@ export class McpResponse implements Response {
|
||||
#args: ParsedArguments;
|
||||
#page?: McpPage;
|
||||
#redactNetworkHeaders = true;
|
||||
#error?: Error;
|
||||
|
||||
constructor(args: ParsedArguments) {
|
||||
this.#args = args;
|
||||
@@ -307,6 +308,10 @@ export class McpResponse implements Response {
|
||||
};
|
||||
}
|
||||
|
||||
setError(error: Error): void {
|
||||
this.#error = error;
|
||||
}
|
||||
|
||||
attachNetworkRequest(
|
||||
reqId: number,
|
||||
options?: {requestFilePath?: string; responseFilePath?: string},
|
||||
@@ -375,6 +380,10 @@ export class McpResponse implements Response {
|
||||
return this.#consoleDataOptions?.types;
|
||||
}
|
||||
|
||||
get error(): Error | undefined {
|
||||
return this.#error;
|
||||
}
|
||||
|
||||
appendResponseLine(value: string): void {
|
||||
this.#textResponseLines.push(value);
|
||||
}
|
||||
@@ -662,6 +671,7 @@ export class McpResponse implements Response {
|
||||
lighthouseResult: this.#attachedLighthouseResult,
|
||||
inPageTools,
|
||||
webmcpTools,
|
||||
errorMessage: this.#error?.message,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -680,6 +690,7 @@ export class McpResponse implements Response {
|
||||
lighthouseResult?: LighthouseData;
|
||||
inPageTools?: ToolGroup<ToolDefinition>;
|
||||
webmcpTools?: WebMCPTool[];
|
||||
errorMessage?: string;
|
||||
},
|
||||
): {content: Array<TextContent | ImageContent>; structuredContent: object} {
|
||||
const structuredContent: {
|
||||
@@ -718,6 +729,7 @@ export class McpResponse implements Response {
|
||||
heapSnapshotNodes?: readonly object[];
|
||||
extensionServiceWorkers?: object[];
|
||||
extensionPages?: object[];
|
||||
errorMessage?: string;
|
||||
} = {};
|
||||
|
||||
const response = [];
|
||||
@@ -1080,6 +1092,11 @@ Call ${handleDialog.name} to handle it before continuing.`);
|
||||
}
|
||||
}
|
||||
|
||||
if (data.errorMessage) {
|
||||
response.push(`Error: ${data.errorMessage}`);
|
||||
structuredContent.errorMessage = data.errorMessage;
|
||||
}
|
||||
|
||||
const text: TextContent = {
|
||||
type: 'text',
|
||||
text: response.join('\n'),
|
||||
|
||||
@@ -128,8 +128,10 @@ export class WaitForHelper {
|
||||
action: () => Promise<unknown>,
|
||||
options?: {timeout?: number; handleDialog?: 'accept' | 'dismiss' | string},
|
||||
): Promise<void> {
|
||||
let dialogOpened = false;
|
||||
if (options?.handleDialog) {
|
||||
const dialogHandler = (dialog: Pick<Dialog, 'accept' | 'dismiss'>) => {
|
||||
dialogOpened = true;
|
||||
if (options.handleDialog === 'dismiss') {
|
||||
void dialog.dismiss();
|
||||
} else if (options.handleDialog === 'accept') {
|
||||
@@ -167,6 +169,10 @@ export class WaitForHelper {
|
||||
try {
|
||||
await navigationFinished;
|
||||
|
||||
if (dialogOpened) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Wait for stable dom after navigation so we execute in
|
||||
// the correct context
|
||||
await this.waitForStableDom();
|
||||
|
||||
+32
-25
@@ -232,31 +232,35 @@ export async function createMcpServer(
|
||||
: new McpResponse(serverArgs);
|
||||
|
||||
response.setRedactNetworkHeaders(serverArgs.redactNetworkHeaders);
|
||||
if ('pageScoped' in tool && tool.pageScoped) {
|
||||
const page =
|
||||
serverArgs.experimentalPageIdRouting &&
|
||||
params.pageId &&
|
||||
!serverArgs.slim
|
||||
? context.getPageById(params.pageId)
|
||||
: context.getSelectedMcpPage();
|
||||
response.setPage(page);
|
||||
await tool.handler(
|
||||
{
|
||||
params,
|
||||
page,
|
||||
},
|
||||
response,
|
||||
context,
|
||||
);
|
||||
} else {
|
||||
await tool.handler(
|
||||
// @ts-expect-error types do not match.
|
||||
{
|
||||
params,
|
||||
},
|
||||
response,
|
||||
context,
|
||||
);
|
||||
try {
|
||||
if ('pageScoped' in tool && tool.pageScoped) {
|
||||
const page =
|
||||
serverArgs.experimentalPageIdRouting &&
|
||||
params.pageId &&
|
||||
!serverArgs.slim
|
||||
? context.getPageById(params.pageId)
|
||||
: context.getSelectedMcpPage();
|
||||
response.setPage(page);
|
||||
await tool.handler(
|
||||
{
|
||||
params,
|
||||
page,
|
||||
},
|
||||
response,
|
||||
context,
|
||||
);
|
||||
} else {
|
||||
await tool.handler(
|
||||
// @ts-expect-error types do not match.
|
||||
{
|
||||
params,
|
||||
},
|
||||
response,
|
||||
context,
|
||||
);
|
||||
}
|
||||
} catch (err) {
|
||||
response.setError(err);
|
||||
}
|
||||
const {content, structuredContent} = await response.handle(
|
||||
tool.name,
|
||||
@@ -267,6 +271,9 @@ export async function createMcpServer(
|
||||
} = {
|
||||
content,
|
||||
};
|
||||
if (response.error) {
|
||||
result.isError = true;
|
||||
}
|
||||
success = true;
|
||||
if (serverArgs.experimentalStructuredContent) {
|
||||
result.structuredContent = structuredContent as Record<
|
||||
|
||||
@@ -5,3 +5,7 @@ exports[`e2e > calls a tool 1`] = `
|
||||
exports[`e2e > calls a tool multiple times 1`] = `
|
||||
[{"type":"text","text":"## Pages\\n1: about:blank [selected]"}]
|
||||
`;
|
||||
|
||||
exports[`e2e > returns blocked message when dialog is opened during tool execution 1`] = `
|
||||
{"content":[{"type":"text","text":"# Open dialog\\nalert: test dialog.\\nCall handle_dialog to handle it before continuing.\\nError: Failed to interact with the element with uid 1_1. The element did not become interactive within the configured timeout."}],"isError":true}
|
||||
`;
|
||||
|
||||
@@ -245,6 +245,37 @@ describe('e2e', () => {
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
it('returns blocked message when dialog is opened during tool execution', async t => {
|
||||
await withClient(async client => {
|
||||
// Navigate to a page with a button that triggers a dialog on click
|
||||
await client.callTool({
|
||||
name: 'new_page',
|
||||
arguments: {
|
||||
url: `data:text/html,<button id="test" onclick="alert('test dialog')">Click me</button>`,
|
||||
},
|
||||
});
|
||||
|
||||
const snapshotResult = await client.callTool({
|
||||
name: 'take_snapshot',
|
||||
arguments: {},
|
||||
});
|
||||
|
||||
const snapshotText = (snapshotResult.content as TextContent[])[0].text;
|
||||
const match = snapshotText.match(/uid=(\d+_\d+)\s+button "Click me"/);
|
||||
const uid = match ? match[1] : '1_1';
|
||||
|
||||
// Trigger the dialog
|
||||
const result = await client.callTool({
|
||||
name: 'click',
|
||||
arguments: {
|
||||
uid,
|
||||
},
|
||||
});
|
||||
|
||||
t.assert.snapshot?.(JSON.stringify(result));
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
async function getToolsWithFilteredCategories(
|
||||
|
||||
Reference in New Issue
Block a user