Skip to content

Create sampling response form#246

Merged
cliffhall merged 7 commits intomodelcontextprotocol:mainfrom
nathanArseneau:sampling-form
Apr 24, 2025
Merged

Create sampling response form#246
cliffhall merged 7 commits intomodelcontextprotocol:mainfrom
nathanArseneau:sampling-form

Conversation

@nathanArseneau
Copy link
Contributor

This address #199

Motivation and Context

The sampling response was hard coded here, The goal was to remove the stubs.

How Has This Been Tested?

I created a small MCP server to send / receive sampling request
image
image

image

Breaking Changes

Nope

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@cliffhall cliffhall added enhancement New feature request waiting on sdk Waiting for an SDK feature labels Apr 2, 2025
@cliffhall
Copy link
Member

cliffhall commented Apr 2, 2025

I think a JSON Editor should be an option here and not only the form.

Two reasons:

  1. A change coming down the pike is that the content field can be an array and not just a single object. Supporting this will be complicated in the form. We'll need a mechanism for adding more content blocks, turning an object into the first element of an array and allowing editing of all of them.
  2. We already have a similar problem with the Run Tool form, and this will be another form that seemed simple but is going to need a lot of work to be more valuable than the JSON editor.

Tool run form can't create new objects (in this case states)

Screenshot 2025-04-02 at 12 07 54 PM

Tool run form fortunately offers JSON editor option

Screenshot 2025-04-02 at 12 07 38 PM

Tool run form doesn't know how to handle objects

Screenshot 2025-04-02 at 12 07 30 PM

@cliffhall cliffhall added waiting on submitter Waiting for the submitter to provide more info and removed waiting on sdk Waiting for an SDK feature labels Apr 3, 2025
@nathanArseneau nathanArseneau force-pushed the sampling-form branch 2 times, most recently from d933078 to 7cdaad0 Compare April 5, 2025 15:26
@nathanArseneau
Copy link
Contributor Author

image image

@cliffhall
Copy link
Member

Hi @nathanArseneau. In that screenshot, I like the extra content container around its elements. But I notice the mime type field is visible when the type is text. It should only be shown if type is not text.

@cliffhall
Copy link
Member

cliffhall commented Apr 5, 2025

Hey @nathanArseneau. Testing this locally, the Approve and Reject buttons seem to work as before, but I'm seeing something weird when trying to switch from JSON to the Form.

The crash comes when a hook in the App.tsx gets run again, trying to fetch config. No idea what has caused that hook to fail or even be run again, since it's a no deps hook that should only run once when the App component is mounted.

The switch to form button is inside a component you're just using, but something must be different about the component where it is embedded. Can you have a look at how it is used in the ToolTab? It is not crashing there.

This may not be about your implementation, but it's kind of a show stopper for this PR. If this just flushed out another bug, we can fix it elsewhere and you can merge it into this. Or if you pin it down and it's easy enough it could just go in as an incidental fix on this PR.

Switch to Form crashes in Sampling tab

form-crash.mov

Switch to Form doesn't crash in Tools tab

form-not-crash.mov

@nathanArseneau
Copy link
Contributor Author

nathanArseneau commented Apr 5, 2025

Sure, I will be happy to investigate, and yes, it is better we solve this issue before this PR

However, when I am testing from my end, I cannot reproduce.

The closest I came to that issue was when my server went down, but that is expected behavior, and there was no error in the console.

@nathanArseneau
Copy link
Contributor Author

My apologies; that was my mistake. The sampling is wrapped in a form for semantics, and the buttons were not typed.

I did not get your error with the config, but I managed to reproduce a similar behavior.

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Runs locally and does the right stuff now. However eslint is reporting some errors on the new SamplingRequest.tsx file.

cd client
eslint


Projects/mcp-inspector/client/src/components/SamplingRequest.tsx
   69:43  error    Unexpected any. Specify a different type                                                                                                @typescript-eslint/no-explicit-any
  111:6   warning  React Hook useMemo has a missing dependency: 'messageResult'. Either include it or remove the dependency array                          react-hooks/exhaustive-deps
  111:7   warning  React Hook useMemo has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked  react-hooks/exhaustive-deps
  111:25  error    Unexpected any. Specify a different type                                                                                                @typescript-eslint/no-explicit-any

✖ 4 problems (2 errors, 2 warnings)

@nathanArseneau nathanArseneau force-pushed the sampling-form branch 2 times, most recently from 4635597 to 4887bc2 Compare April 16, 2025 02:03
@nathanArseneau
Copy link
Contributor Author

Fixed and added in CI to avoid having to run locally to validate.
image

build will fail with any
image

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Looks good and runs. Still have a specific model name in the component though.

@cliffhall
Copy link
Member

@nathanArseneau A wrinkle here is that a recent change to the DynamicJsonForm has made it so that we do not attempt to display complex forms. If the schema to be edited is anything but a list of basic fields, the JSON form is the only affordance. So this only displays the JSON form now. See that PR for details on that decision.

Also, I notice that when I approve my edits, the response sent back to the server is not complete.

The response format:

Screenshot 2025-04-23 at 4 45 27 PM

Response actually sent from the client

Screenshot 2025-04-23 at 4 43 53 PM

@nathanArseneau
Copy link
Contributor Author

nathanArseneau commented Apr 24, 2025

@cliffhall While fixing the merge conflicts, I saw that change and personally was not a fan of it.

The fix was to remove the [Object: object], but in my opinion, the component should have been extended to support more complex schemas, instead of removing the form entirely when the schema is 'complex' or not flat. Another issue should be opened to fix this behavior.

So I assume your test scenario is:
You have a tool that creates a sampling request and waits for a response. The response from your tool call is the response for the sampling.

image

However, sampling response type and tool response type are different, which is why you do not see the expected outcome. If I'm wrong in my assumption, I'd be happy to implement a fix if you provide your test scenario.

Here is the code for my tool

server.tool(
    'sampling',
    'This is a tool to test sampling',
    { str: z.string() },
    async ({ str }) => {
        const samplingResponse = await server.server.createMessage({
            messages: [
                {
                    role: 'user',
                    content: {
                        type: 'text',
                        text: `sampling request for ${str}?`,
                    },
                },
            ],
            systemPrompt: 'You are a helpful file system assistant.',
            includeContext: 'thisServer',
            maxTokens: 100,
        })

        return {
            content: [
                {
                    type: 'text',
                    text: JSON.stringify(samplingResponse, null, 2),
                },
            ],
        }
    }
)

@cliffhall
Copy link
Member

cliffhall commented Apr 24, 2025

@cliffhall While fixing the merge conflicts, I saw that change and personally was not a fan of it.
in my opinion, the component should have been extended to support more complex schemas, instead of removing the form entirely when the schema is 'complex' or not flat. Another issue should be opened to fix this behavior.

I get it. The problem is schemas of arbitrary depth are going to result in a very complex form. Our existing code for creating the forms was not optimal, and ad-hoc extending that to handle anything seemed unwise. The input fields would be nice, but not in the form of a maintenance burden. Definitely another issue/PR that tackles it in isolation is the way to go.

@cliffhall
Copy link
Member

However, sampling response type and tool response type are different, which is why you do not see the expected outcome.

Yep, you're right. That's the server echoing back the text from the sampling response. Not an issue.

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

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

Although we need to address dynamic form building in another request, this PR gives us the ability to edit the previously stubbed LLM response that goes back to the server, which will be useful for testing the functionality of server tools that request sampling.

edit sampling response

@cliffhall cliffhall merged commit 2e4a522 into modelcontextprotocol:main Apr 24, 2025
2 checks passed
@cliffhall
Copy link
Member

This fixed #199

IgnacioC44 referenced this pull request in MCPJam/inspector Jun 21, 2025
@MAX96811
Copy link

WOW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature request waiting on submitter Waiting for the submitter to provide more info

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants