Repository navigation
feat(codex): use MCP tool grants in sandbox sessions - #4590
Conversation
098f053 to
ef74ead
Compare
|
/ok to test ef74ead |
|
🌿 Preview your docs: https://nvidia-preview-pr-4590-452cfc2d2b9d.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — 1 BLOCKER and 1 NOTE.
MCP grant wiring and required-server failure handling are covered by focused tests, but truncated multiword header values can still expose secrets in startup diagnostics. The newly enabled session path also records MCP event categories instead of tool names in close observations.
Completed a source-only review of all seven changed files and their directly affected grant, configuration, observation, and session-close contracts. Tests were inspected, not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
|
/review reset=true |
|
/ok to test 55ba5a6 |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — 2 BLOCKERs.
The prior truncated-multiword counterexample and MCP observation-name mismatch are fixed in the current source. Two distinct redaction failures remain: headers sharing a prefix can interfere with one another, and JSON-escaped header echoes bypass matching. Both can leave secret fields in activation errors and persisted startup-failure gaps.
Completed a source-only review of all seven changed files and the directly affected grant, configuration, observation, and session-close contracts. Tests were inspected, not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
|
/review reset=true |
|
/ok to test 0f7aa16 |
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
…tion gaps Signed-off-by: Abhishree <athittenaman@nvidia.com>
Signed-off-by: Abhishree <athittenaman@nvidia.com>
…ervations Signed-off-by: Abhishree <athittenaman@nvidia.com>
…ped echoes Signed-off-by: Abhishree <athittenaman@nvidia.com>
bea2c58 to
452cfc2
Compare
|
/ok to test 452cfc2 |
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the rebase. Re-reviewed at 452cfc2.
- The rebase onto #4740 and #4854 carries every change in this PR over to main's new activation code exactly once:
- grants written to
config.toml; - the seed refusals, with main's "Codex in the task sandbox" wording;
- required servers stopping Codex before the first model call;
- the redacted stderr tail in the 502 and in the
agent_startup_failedgap; - the
mcp_server_skippedgaps; - observations kept when close interrupts the stderr read.
- grants written to
calls_model_from_sandboxand main's fern wording survive. Comparing the PR's tests before and after the rebase, no assertion was weakened; the only change is the switch to main's identicalpinned_cli()helper.pytest responses_api_agents/codex_agent/testsat this head: 318 passed, 51 skipped (Linux-only andCODEX_TEST_BINcases), and 1 failure from the OpenSandbox SDK missing locally, unrelated.- Optional: the startup-failure tests run only on the CLI-events fallback path, because the fake sandbox never publishes
native_session.jsonl. We checked that the native-transcript path also gets the stderr tail and exactly one gap. A parametrized case with a minimal native transcript would keep that from regressing.
No further comments from us.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39949344842 |
What does this PR do?
Codex sandbox sessions now accept MCP tool grants. Before this PR, a session with any required tool grant was rejected with a 422, so a benchmark whose tools live on its resources server could not run with Codex.
mcp_serversentry in the session'sconfig.toml: the grant URL, the session token ashttp_headers, andrequired = truefor a required grant. The non-session path already writes this entry from/seed_sessionmetadata; both paths now share one helper.[redacted]. Each value is matched as written and JSON-escaped. A value that Codex cut off inside a long error body is replaced through its last echoed character, so no part of a multiword value stays visible. All values are matched against the original text and replaced in one pass.agent_startup_failedobservation gap. Environment Servers keep the observations, not the 502 body, in their failure records. If the session is closed during the stderr read, the observations are still returned, without this gap.session_close_timeout_seconds.direct_httpgrants are still rejected with a 422.-and_. A required grant with another name is rejected at seed time. An optional one is left out and recorded as anmcp_server_skippedobservation gap.mcp_tool_timeout_seconds. It is written astool_timeout_secand must not exceedtimeout. Unset keeps the pinned CLI's default of 300 seconds. The README explains why it should exceed the server's slowest call: Codex runs no other tool while an MCP call is in flight, and a call that times out keeps running on the server.mcp_tool_call.Needed to run Codex on GDPVal (#4178, merged), whose resources server serves
finish,abandon_task_finish,web_searchandfetch_web_page. No issue.Related:
pkill -f python3runner kill seen in the rollouts below.codex_agent/app.py:_build_configcall and the error path. It rebuilds the observation bundle after that point, so whichever lands second must add theagent_startup_failedgap to the final bundle.AgentSeedRejectedError.Validation
mainat c62f9cc, which adds Codex failure classification (feat(agent): report native causes for shared grading policy #4834) and builds observations from the native transcript when there is one (fix(codex): retain session evidence and report model errors #4740). A startup failure has noturn.failed, so it still ends in the 502 with the stderr tail, and theagent_startup_failedgap is added to the observations on both paths. It keeps main's "agent session" wording (refactor: describe session support without "native" #4854), its 422 message for unusable grants, andcalls_model_from_sandbox.pytest responses_api_agents/codex_agent/tests tests/unit_tests/test_agent_outcome.pyat the final head, withCODEX_TEST_BINset to the Codex 0.144.4 binary, in a venv built withuv sync --extra dev --extra sandboxfromuv.lock: 409 passed, 1 skipped (the skipped test needs Python 3.8).config.toml(URL, token header,required, timeout, a hyphenated name), and an episode grant replacing a configured grant of the same name;tool_timeout_sec;mcp_server_skippedgap;timeout; equal totimeoutis accepted);agent_startup_failedgap, while a failure after a turn started does not;finish), notmcp_tool_call;CODEX_TEST_BINis set):turn.startedevent, and "required MCP servers failed to initialize" on stderr.mcp__<server>. It addslist_mcp_resources,list_mcp_resource_templatesandread_mcp_resourceonce any MCP server is configured.tool_timeout_secends a slow call with an error; without it, a call times out after 300 seconds. After a timeout, the next call to the same server waits until the first returns.expose_tools_over_mcp: true, Codex sees the four task tools, itsfinishcall is recorded for the session, and/verifypersists the submitted file.pre-commit run --all-files: passed, with no files changed.Rollouts
GDPVal with #4178 (before it merged) plus this PR, the HOME change from #3902 and #4591:
mcp_tool_timeout_seconds(unset in these runs), skip optional grants whose names Codex cannot use, and otherwise changed only the error text, observation gaps, docs and tests. The entry written for GDPVal'sgdpval_resources_servergrant is unchanged.Results:
finishonce over MCP, and the files were saved undertask_<id>/repeat_0/. 81 model calls, all HTTP 200.finishonce over MCP, and the submitted workbooks were persisted withfinish_params.json. 56 model calls, all HTTP 200.finish(one of them twice).finish, and one hit the 3,600-second Codex timeout.pkill -f python3, which killed the Codex runner (fix(agents): hide the sandbox supervisor and runners from pkill python #4805).Compatibility and benchmark impact
mcp_serversentry is written.config.toml. No config in the repo creates them today.mcp_tool_timeout_secondsis set.Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).