Skip to content

chore: conditionally disable modal state tools #256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

chore: conditionally disable modal state tools #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ export function createServerWithTools(options: Options): Server {
});

server.setRequestHandler(ListToolsRequestSchema, async () => {
const modalStates = context.modalStates().map(state => state.type);
const activeTools = tools.filter(tool => !tool.clearsModalState || modalStates.includes(tool.clearsModalState));
Copy link
Member

@pavelfeldman pavelfeldman Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a firm believer that the dynamic tool list is a good idea. I think it will throw LLM off. Here is my thinking:

  • If this mechanism worked, we would only list the tools that clear currently engaged states. The client would then re-build the tool prologue for the LLM and re-compile the chat. Now LLM will be presented with a chat history where it calls non-existing tools
  • An alternative, more heavyweight approach would be inlining the messages about the tools being turned off and on dynamically

Maybe there are other ways to implement this, but none of the ways I come up with improve the quality of the prompt, only adds unnecessary noise. I think dynamic tooling is not a great idea when it comes to keeping LLM sharp.

Copy link
Member Author

@Skn0tt Skn0tt Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced. If dynamic tools threw off the LLM, then MCP wouldn't have been designed with explicit support for dynamic tool lists, or at least it'd allow the client to announce compatibility. My guess is they explicitly added it for this usecase, also as a measure to limit the number of tools.

I think we can do two things to figure this out:

I'll do both of that and report back.

If this mechanism worked, we would only list the tools that clear currently engaged states.

Good idea!

An alternative, more heavyweight approach would be inlining the messages about the tools being turned off and on dynamically

As far as I know, LLMs are already trained specifically with tool usage in mind, and i'd imagine that turning tools on or off is part of that training. So the tool list feels like the natural place to communicate this, not the message.

return {
tools: tools.map(tool => ({
tools: activeTools.map(tool => ({
name: tool.schema.name,
description: tool.schema.description,
inputSchema: zodToJsonSchema(tool.schema.inputSchema)
Expand All @@ -64,9 +66,9 @@ export function createServerWithTools(options: Options): Server {
};
}

const modalStates = context.modalStates().map(state => state.type);
if ((tool.clearsModalState && !modalStates.includes(tool.clearsModalState)) ||
(!tool.clearsModalState && modalStates.length)) {
const modalStates = new Set(context.modalStates().map(state => state.type));
if ((tool.clearsModalState && !modalStates.has(tool.clearsModalState)) ||
(!tool.clearsModalState && modalStates.size)) {
const text = [
`Tool "${request.params.name}" does not handle the modal state.`,
...context.modalStatesMarkdown(),
Expand All @@ -78,7 +80,14 @@ export function createServerWithTools(options: Options): Server {
}

try {
return await context.run(tool, request.params.arguments);
const response = await context.run(tool, request.params.arguments);

const newModalStates = context.modalStates().map(state => state.type);
const modalStateChanged = newModalStates.length !== modalStates.size || newModalStates.some(state => !modalStates.has(state));
if (modalStateChanged)
await server.sendToolListChanged();

return response;
} catch (error) {
return {
content: [{ type: 'text', text: String(error) }],
Expand Down
4 changes: 0 additions & 4 deletions tests/capabilities.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ test('test snapshot tool list', async ({ client }) => {
'browser_click',
'browser_console_messages',
'browser_drag',
'browser_file_upload',
'browser_handle_dialog',
'browser_hover',
'browser_select_option',
'browser_type',
Expand Down Expand Up @@ -51,8 +49,6 @@ test('test vision tool list', async ({ visionClient }) => {
expect(new Set(visionTools.map(t => t.name))).toEqual(new Set([
'browser_close',
'browser_console_messages',
'browser_file_upload',
'browser_handle_dialog',
'browser_install',
'browser_navigate_back',
'browser_navigate_forward',
Expand Down
6 changes: 6 additions & 0 deletions tests/files.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,14 @@
* limitations under the License.
*/

import { Notification } from '@modelcontextprotocol/sdk/types.js';
import { test, expect } from './fixtures';
import fs from 'fs/promises';

test('browser_file_upload', async ({ client }) => {
const notifications: Notification[] = [];
client.fallbackNotificationHandler = async notification => { notifications.push(notification); };

expect(await client.callTool({
name: 'browser_navigate',
arguments: {
Expand All @@ -39,6 +43,8 @@ test('browser_file_upload', async ({ client }) => {
})).toContainTextContent(`### Modal state
- [File chooser]: can be handled by the "browser_file_upload" tool`);

expect(notifications).toContainEqual(expect.objectContaining({ method: 'notifications/tools/list_changed' }));

const filePath = test.info().outputPath('test.txt');
await fs.writeFile(filePath, 'Hello, world!');

Expand Down