refactor: improve execution tool guidance for script execution#431
Conversation
|
For team members: test commit |
|
Note: Returning |
|
I like this idea. |
|
Let me fix the tests! 😁 |
|
Great idea! This should guide the model into the next step of the loop😁 Just curious, which model were you using to validate the new guidance? |
Jazzcort
force-pushed
the
improve-toolcall-result-instruction
branch
from
eb0c6cc to
2fbc786
Compare
|
For team members: test commit |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@panyamkeerthana I use |
|
I think I'll wait for some feedback on this. If it ends up quite positive and we want to merge this into main, I'll work on the test coverage then. 😁 |
owtaylor
left a comment
owtaylor
left a comment
There was a problem hiding this comment.
Looks like a nice cleanup and improvement! I think we can make it a bit cleaner by using get_context() rather than feeding around client_params.
I was worried when I started looking at it it would be a big conflict with #457, but I think it won't be bad whichever lands first.
| MCP_UI_EXTENSION = "io.modelcontextprotocol/ui" | ||
|
|
||
|
|
||
| def use_mcp_app_for_client(client_params: InitializeRequestParams): |
There was a problem hiding this comment.
Passing around client_params gets really tedious. We can use fastmcp's get_context() which gets the current context (async safely), but can't avoid client_params entirely because get_context() won't work in on_initialize, as far as I recall.
| def use_mcp_app_for_client(client_params: InitializeRequestParams): | |
| def use_mcp_app_for_client(client_params: InitializeRequestParams | None = None): | |
| if client_params is None: | |
| client_params = get_context().session.client_params |
| def _pick_execution_tool(readonly: bool, needs_confirmation: bool, use_mcp_app: bool): | ||
| if not needs_confirmation and readonly: | ||
| return "run_script" | ||
|
|
||
| if use_mcp_app: | ||
| return "run_script_interactive" | ||
| else: | ||
| return "run_script_with_confirmation" |
There was a problem hiding this comment.
Don't see why readonly is needed here - that should already be reflected in needs_confirmation.
| def _pick_execution_tool(readonly: bool, needs_confirmation: bool, use_mcp_app: bool): | |
| if not needs_confirmation and readonly: | |
| return "run_script" | |
| if use_mcp_app: | |
| return "run_script_interactive" | |
| else: | |
| return "run_script_with_confirmation" | |
| def _pick_execution_tool(needs_confirmation: bool): | |
| if not needs_confirmation: | |
| return "run_script" | |
| elif use_mcp_app_for_client(): | |
| return "run_script_interactive" | |
| else: | |
| return "run_script_with_confirmation" |
| # Verify that this script doesn't require confirmation | ||
| if script_details.needs_confirmation: | ||
| raise ToolError("This script requires confirmation. Use run_script_with_confirmation instead of run_script.") | ||
| raise ToolError(f"This script requires confirmation. Use {confirmation_tool_name} instead of run_script.") |
There was a problem hiding this comment.
| raise ToolError(f"This script requires confirmation. Use {confirmation_tool_name} instead of run_script.") | |
| raise ToolError(f"This script requires confirmation. Use {_pick_execution_tool(True)} instead of run_script.") |
This commit refactors MCP app detection and enhances the tool feedback loop to guide clients toward the correct execution tools. Key changes: * Extracted `_use_mcp_app_for_client` from `server.py` into a public `use_mcp_app_for_client` utility in `mcp_app.py` for cross-module reuse. * Added a `_pick_execution_tool` helper in `run_script.py` to dynamically determine the right tool (`run_script`, `run_script_interactive`, or `run_script_with_confirmation`) based on script flag (needs_confirmation) and client capabilities. * Updated `validate_script` to explicitly instruct the client on which execution tool to use next in its text response. * Improved the error message in `run_script` to dynamically suggest the appropriate confirmation tool if the script requires confirmation but was called incorrectly.
Jazzcort
force-pushed
the
improve-toolcall-result-instruction
branch
from
2fbc786 to
3a455cb
Compare
|
For team members: test commit |
|
Not sure if I should move |
I'm fine with it that way. |
|
Thanks for reviewing! 😁 |
4 participants
This commit refactors MCP app detection and enhances the tool feedback loop to guide clients toward the correct execution tools.
Key changes:
_use_mcp_app_for_clientfromserver.pyinto a publicuse_mcp_app_for_clientutility inmcp_app.pyfor cross-module reuse._pick_execution_toolhelper inrun_script.pyto dynamically determine the right tool (run_script,run_script_interactive, orrun_script_with_confirmation) based on script flags (readonly, needs_confirmation) and client capabilities.validate_scriptto explicitly instruct the client on which execution tool to use next in its text response.run_scriptto dynamically suggest the appropriate confirmation tool if the script requires confirmation but was called incorrectly.