Repository navigation
feat(pi): use MCP tool grants in sandbox sessions - #4587
Conversation
2321554 to
6730336
Compare
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
6730336 to
e7445d0
Compare
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
|
/ok to test 4e2f867 |
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks @athitten, this looks good. Reviewed at 4e2f867.
The seed checks, the MCP setup in the sandbox, ending the run on finish (including the cut-off refusal), and the mcp__<grant>__<tool> naming for /verify all look correct. We checked the termination, message_end ordering and stdout assumptions against Pi 0.80.2's source. Locally, the pi_agent tests, test_gym_mcp.mjs and the pi_sandboxed_agent tests pass; the real-Pi tests need Linux, so they were not run here.
Non-blocking, for a follow-up if useful:
- Tests for the grant-overlap rule beyond the both-required case (required plus optional, two optional, the 62-character boundary).
finish_tool_namesmatches bare tool names on every grant; scoping it per grant would matter once a second grant can offer a tool with the same name.
Thanks @yaoyu-33 ! Will add in a separate small Pi PR |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39536523049 |
What does this PR do?
Pi's sandbox sessions can now use the Resources server's tools through MCP tool grants. Before this PR, a session refused any required tool grant with a 422. So Pi could not run a benchmark whose task tools live on the Resources server, such as GDPVal since #4178, where the agent hands in its files by calling the Resources server's
finishtool.Tool grants. With
expose_tools_over_mcp: trueon the Resources server andresources_tool_transports: [mcp]on the Environment Server, each MCP grant reaches Pi throughgym_mcp.mjs, the extension the local path already uses formcp_servers. Pi names each tool<grant name>_<tool name>, at most 64 characters. Discovery waits up to 30 seconds per request, and each tool call up to the newsandbox_mcp_timeout_seconds(default 300).Grants Pi cannot use. Seeding refuses with a 422:
direct_httpgrants, as before;-and_;_<suffix>(such aswebandweb_v2), because Pi could not tell their tools apart.An optional MCP grant with either problem is left out and recorded as an
mcp_server_skippedobservation gap.Setup failures. A required grant that Pi cannot set up stops Pi before the first model call. The error and an
mcp_setup_failedobservation gap give the cause, naming the grant and the server's scheme, host and port when a grant failed. An optional grant that fails is skipped and recorded asmcp_server_skipped. Error text leaves out response bodies, header values and the URL's path and query.Finish tools. In a GDPVal smoke run, one model called
finish573 times in a row until its context overflowed. The newfinish_tool_names(like Stirrup's setting of the same name) lists granted tools whose successful call ends the run:gym_mcp.mjsreturns Pi'sterminate: truefor them (Pi 0.69.0 or later), Pi stops when every tool call of the turn is one of them, and the response and observations count the run as completed. Setup fails if no granted server offers a listed tool.length, or when it used all of itsmax_tokens: vLLM reportstool_callsfor a reply it parsed a tool call from, even when the reply was cut off.Tool names in responses. Responses name MCP calls
mcp__<grant>__<tool>, the form a Resources server's/verifystrips, as Hermes does. Observations and model calls keep the names the model saw.Event delivery. Pi's JSON mode sends
console.logandprocess.stdout.writeto stderr, and the runner records only stdout.gym_mcp.mjswrites its events with the stdout stream's ownwrite, which still reaches stdout and keeps each line whole.Most of the
gym_mcp.mjsdiff is re-indentation: per-server discovery moved into its own function. Hide whitespace changes to review it.No issue. Needed by #4589 (Pi on GDPVal), which is still stacked on #4178's pre-merge branch and will be rebased onto
main. Related:ng_pi_outcome(context-overflow) event. This PR does not depend on it; the rollouts below include it._sandbox_response. Whichever merges second must keep the MCP setup message as the error and keep the finish-tool check.Validation
At 4e2f867, on
maind96b337 (mainhas moved since without touching these files; the branch merges cleanly):pytest responses_api_agents/pi_agent/testswith real Pi 0.80.2 onPATH: 296 passed, 2 skipped (they need Python 3.8).node --test responses_api_agents/pi_agent/tests/test_gym_mcp.mjs: 17 passed on Node 24.21.0 and on Node 22.19.0 (the sandbox's Node).pytest responses_api_agents/pi_sandboxed_agent/tests, which sharesgym_mcp.mjs: 49 passed.lengthor as vLLM does, never reaches the server, and the model calls it again; a required server that is down stops Pi before any model call, with the cause. The test skips when the pinned Pi is not installed. In CI it runs only in the non-blocking harness conformance job (Pi job, step "Validate probes, checker, and adapter").toolUsecheck in_ends_on_finish_tool) changes nothing, because a reply cut off at the output limit is reported incomplete anyway. The other nine are rare cases left without tests on purpose: name-length and overlap boundaries, malformed tool results, several finish calls in one turn, finish tools listed on another server, the exit wait with a full stdout pipe, escaping of hostile tool names, and tools without a name.gym_mcp.mjsat 4e2f867 andmain's GDPVal resources server (expose_tools_over_mcp: true, stub model), on Node 24 and 22.19.0: the model saw the four GDPVal tools,finishwas recorded for the session with its arguments, and the run ended after that call with one model request.pre-commit run --all-files: passed, with no changes.Rollouts
The rollouts used earlier revisions of this PR. Later commits changed the seed checks, error text, observation gaps and event delivery, and added the refusal of a finish call from a cut-off reply. Every session in these runs set up its tools without errors.
Smoke on #4178 at bf9f8db with this PR (then 6730336), #4588, #4589 and #4586, 3 tasks: all 3 runs ended on
finishwith no follow-up model call, and the files were saved. Details are in #4589.220 GDPVal tasks with Pi on #4178 merged with
main, run with #4589, the HOME change from #3902 and an earlier revision of this PR:finish, with no follow-up model call and no finish loops. In 4 of them, Pi's argument check rejected the firstfinishcall, and the model called it again correctly in the next turn.finishcalls came from a reply that used its whole output budget (22 of the 6,449 model calls did, none with afinishcall), so the refusal added since would not have changed these runs.Compatibility and benchmark impact
resources_tool_transports: []or[direct_http]), which covers every Pi config in the repo. Requireddirect_httpgrants are still refused.mcp_server_skippedgap. Today only an agent config'stool_accesses(none in the repo) or docs(harbor): add a SWE-Interact run recipe on the Harbor task shape #4728 would send them.mcp__<grant>__<tool>in the response; observations keep<grant>_<tool>.mcp_server_skippedandmcp_setup_failed.lengthor uses all of the request'smax_tokens. A lower limit that the model server applies itself, such as the room left in the context window, is not detected.mcp_serverspath andpi_sandboxed_agentsharegym_mcp.mjs. Their entries have norequiredfield, so they stay required. On a setup failure, stderr now carries the cause with the server's scheme, host and port, and stdout gets anng_gym_mcp_setup_failedline.finish_tool_namesneeds Pi 0.69.0 or later; the default pin is 0.80.2.Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).