Skip to content

feat: gym_tool dual registration — every tool served over both HTTP and MCP from one declaration - #2002

Closed
adil-a wants to merge 29 commits into
mainfrom
mcp-dual-registration
Closed

feat: gym_tool dual registration — every tool served over both HTTP and MCP from one declaration#2002
adil-a wants to merge 29 commits into
mainfrom
mcp-dual-registration

Conversation

@adil-a

@adil-a adil-a commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Closes #1749.

What this PR does

One gym_tool declaration now serves a tool through both doors: a plain HTTP POST /<name>
route (what simple_agent calls) and an MCP tool on a /mcp endpoint (what MCP-native agents
like Claude Code call). The separate MCPResourcesServer class is gone — MCP is a lazy capability
of SimpleResourcesServer, activated only when a server actually declares a tool, so the 78
tool-less servers are untouched. All 16 servers that execute tools over HTTP are migrated; both
transports resolve the same per-rollout session, and verifiers score rollouts identically
regardless of which door the agent used. Of the +4.7k inserted lines, +3.5k are tests;
production code is ~+1.1k, ~600 of it in one file.

How to review (suggested order)

Commits are review-ordered (base → agent fix → example migration → docs → one server per commit →
perf → simplification → verifier normalization → test dedup), and every intentional behavior
delta is named in its server's commit message
— nothing changes silently.

  1. nemo_gym/base_resources_server.py — the whole mechanism. Path through it: the gym_tool
    docstring (the user manual: decorator form for hand-written tools, runtime call form for
    registry/discovered tools, three input_schema modes) → setup_webserver (the lazy gate that
    keeps tool-less servers untouched) → _setup_mcp (builds both doors; re-registers the MCP
    SDK's low-level list/call handlers) → _register_http_gym_tool (the HTTP twin).
  2. resources_servers/example_single_tool_call/app.py — the PR in miniature. Diff it against
    main: the hand-written route registration and request model collapse into one @gym_tool.
  3. One server per migration pattern (the rest repeat): math_advanced_calculations (typed
    methods), workplace_assistant (runtime-registered registry tools), newton_bench
    (model-described tools — also fixes a real pre-existing bug where a failed directory scan
    silently dropped tool registration).
  4. tests/unit_tests/test_gym_tool_dual_registration.py — the contract as executable spec
    (handshake, raw-argument fidelity, session parity, seed augmentation, transport parity).
  5. Skim: claude_code_agent (logging fix), the fern tutorial rewrite, nemo_gym/mcp_test_utils.py
    (shared test scaffolding).

Contract changes vs main, and why

Change Why
MCPResourcesServer deleted (breaking) One entry point was the goal; an alias would silently make isinstance checks true for every server. Failure mode is a loud ImportError with a 2-line fix.
seed_session responses on tool-bearing servers gain an additive mcp key (server name, /mcp path, signed session token) MCP agents need per-rollout connection info; injected automatically with setdefault semantics — a server returning its own mcp value is never overwritten. Tool-less servers unchanged.
POST to an unknown tool path: 404 body now lists available tools (servers with historical 200-error-string contracts keep them via a handle_unknown_tool override) Agents feed response bodies back to the model as tool output; a body naming valid tools lets the model self-correct.
Non-POST to an unknown tool path: 404 → 405 Side effect of the POST-only catch-all; 405 is the semantically correct status and no agent sends non-POST to tool routes.
Non-empty malformed JSON body to a lax tool: 422, tool not dispatched (empty body still tolerated as no-args) Differential testing caught the original migration dispatching tools on garbage input (stepping an env, mutating reward state). Restores main's behavior.
Verifiers normalize trajectory tool names via a new normalize_tool_name helper MCP agents record calls as mcp__<server>__<tool>; HTTP agents record bare names. Without normalization an identical, correct Claude-over-MCP rollout scored 0.0 (reproduced, then fixed and regression-tested). Bare names pass through untouched, so HTTP scoring is byte-identical.
A typed tool whose only parameter is a Pydantic model is rejected at startup That shape is ambiguous (callers could nest under the parameter name or send the model's fields flat); the error names the fix: @gym_tool(input_schema=<Model>). The check runs in the collection chokepoint so no registration path can bypass it.
mcp dependency pin tightened <2<1.29 The base re-registers the SDK's low-level tools/list/tools/call handlers via private attrs (mcp._mcp_server) — required because FastMCP's own call path silently drops undeclared argument names (verified against SDK source), which would corrupt registry tools' calls. Private-attr coupling means the ceiling should move deliberately, gated on the handshake test suite.

Deliberate decisions (please don't re-litigate without new evidence)

  • allowed_tools (per-session tool visibility) gates MCP only; HTTP is unrestricted status quo.
    Enforcing it on HTTP would change recorded behavior of a published benchmark
    (indirect_prompt_injection). Over MCP — a brand-new surface — each session lists and can call
    exactly its task's tools, carried as a claim inside the signed stateless token (works across
    workers, nothing to evict).
  • Error shapes differ per transport by design (MCP: isError tool result on HTTP 200; HTTP:
    status codes) — documented in the gym_tool docstring and the tutorial.
  • Wire-byte preservation was the migration acceptance bar: all 14 dispatcher/route-migrated
    servers' HTTP request/response bytes, including error paths, are replay-tested against recorded
    bytes (indirect_prompt_injection is covered by contract tests preserving its soft-error
    strings; example_mcp_weather had no pre-existing HTTP tool surface).
  • indirect_prompt_injection's MCP surface advertises permissive object schemas (its per-task
    schemas live in dataset rows, which remain the model-facing truth; one MCP tool name can carry
    only one schema). MCP there is strictly tighter than HTTP, never looser — owner sign-off
    requested above.

Runnable example — workspace env driven by Claude Code over MCP

workplace_assistant's 27 tools are MCP-callable with configuration only — no further server
changes needed. Compose config
(save as workplace_claude.yaml):

workplace_assistant:
  resources_servers:
    workplace_assistant:
      entrypoint: app.py
      domain: agent

workplace_claude:
  responses_api_agents:
    claude_code_agent:
      entrypoint: app.py
      resources_server:
        type: resources_servers
        name: workplace_assistant
      model: claude-sonnet-4-6
      anthropic_api_key: ${anthropic_api_key}   # from repo-root env.yaml
# env.yaml:  anthropic_api_key: sk-ant-...
gym env start --config workplace_claude.yaml

gym eval run --no-serve \
  --agent workplace_claude \
  --input resources_servers/workplace_assistant/data/example.jsonl \
  --output results/workplace_claude_rollouts.jsonl --limit 1

A correct rollout shows Claude Code calling mcp__workplace_assistant__email_search_emails then
mcp__workplace_assistant__email_reply_email, and reward: 1.0. (Any Anthropic-format endpoint
works via the agent's anthropic_base_url config field.)

30-second smoke test without an LLM (same server, both doors, one session):

import requests
s = requests.Session()
meta = s.post("http://127.0.0.1:<port>/seed_session", json={}).json()["mcp"]
token = meta["headers"]["X-NeMo-Gym-Session-Token"]
# HTTP door
print(s.post("http://127.0.0.1:<port>/company_directory_find_email_address", json={"name": "carlos"}).json())
# MCP door (same session, via the token)
print(s.post(f"http://127.0.0.1:<port>{meta['url_path']}",
    headers={"Accept": "application/json, text/event-stream", "X-NeMo-Gym-Session-Token": token},
    json={"jsonrpc": "2.0", "id": 1, "method": "tools/call",
          "params": {"name": "company_directory_find_email_address", "arguments": {"name": "carlos"}}}).json())

How this was verified

All 14 route-migrated servers were differentially byte-replayed against main (happy + error
paths mined from datasets) — the only intentional deltas are exactly the contract-table rows above.
Live end-to-end with real models: reward 1.0 over both transports (workplace_assistant and
example_mcp_weather, via gym env start + gym eval run). Tool-less servers proven
byte-identical, with a subprocess test asserting mcp is never imported for them. Transport parity
(unfiltered MCP tools/list == HTTP tool routes) is a tested invariant per server. 194 new tests;
base-file coverage 99%.

Migrating your own env: full patterns in
fern/versions/latest/pages/environment-tutorials/mcp-resources-server.mdx (added in this PR).

🤖 Generated with Claude Code

codex and others added 27 commits July 9, 2026 05:43
…l dual registration

One entry point, both transports: gym_tool (decorator or runtime call with owner=self)
now registers every declared tool as an HTTP POST /<name> route AND an MCP tool. The
MCP endpoint mounts lazily — only when a server declares a tool or overrides
register_mcp_tools — so tool-less servers are unchanged and never import mcp.

- gym_tool grows kwargs (name/description/input_schema/validate/owner) with three
  schema modes: None (typed params), dict (verbatim schema + raw-argument passthrough
  via a low-level call_tool handler, bypassing FastMCP's silent argument dropping),
  and a Pydantic model class (fields as schema, validated instance passed through).
- Low-level tools/list handler advertises dict schemas verbatim, hides session_id,
  and filters by the signed token's new allowed_tools claim; the call handler gates
  calls on the same claim (hiding alone does not block).
- seed_session responses are auto-augmented with the MCP metadata block (setdefault
  semantics, JSONResponse to bypass response_model stripping) on MCP-enabled servers.
- Unknown-tool catch-all with an overridable handle_unknown_tool hook (default: 404
  listing available tools; override preserves historical soft-error bytes).
- MCPResourcesServer fully deleted (breaking): subclass SimpleResourcesServer instead.
  example_mcp_weather migrated (2 lines); unit tests keep every existing assertion.
- New dual-registration suite: full JSON-RPC handshake over TestClient, raw-argument
  fidelity, nested-model cross-transport parity, Annotated/BeforeValidator fidelity,
  allowed_tools, catch-all, seed matrix, transport parity (E9), 98% file coverage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…r malformed

Previously _write_rollout_mcp_config silently returned None, so a misconfigured MCP
setup produced zero-reward rollouts with no signal. Absent metadata is a debug log
(normal for non-MCP servers, and MCP-enabled servers now auto-augment their seed
responses); present-but-malformed metadata is a warning.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
… both transports

The setup_webserver override and GetWeatherRequest body model are replaced by a single
@gym_tool decorator with flat typed params. The HTTP wire contract is unchanged (same
POST /get_weather path, same flat {"city": ...} body, same response bytes, 422 on
missing fields) and the tool is now also served over MCP. Tests assert the HTTP replay
bytes, the MCP list/call round-trip, and transport parity.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…gym_tool design

No separate MCP base class: gym_tool declarations are served over both transports with
a lazily mounted /mcp. Documents the runtime registration form (input_schema modes),
automatic seed_session metadata injection + allowed_tools, handle_unknown_tool, the
per-transport error/session semantics, the dual-transport dataset honesty note, and
the num_workers=1 constraint for stateful servers. Frozen v0.4.0 snapshot untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…registration

Each _function_map entry becomes a typed @gym_tool method (Optional-number params
reproducing the old None-filter acceptance); the /{path} dispatcher and override are
gone. All on-signature wire behavior verified byte-identical against recorded
pre-migration bytes (success envelopes, missing-arg 500s, math-error 500s, 422s).

Named behavior deltas (error paths only):
- off-signature junk params are now ignored instead of 500 unexpected-kwarg
- unknown path + invalid-typed body: 404 unconditionally (was 422-before-404)
- /seed_session now carries the injected mcp metadata; /mcp serves the same 11 tools

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
The five static routes become @gym_tool methods (request.session reads swapped for the
injected session_id; Annotated/BeforeValidator coercers preserved on both transports);
handle_unknown_tool reproduces the historical 200 catch-all stub byte-for-byte; a slim
setup_webserver override remains only for the ticker preload. 57 tests green including
8 HTTP replay, 4 MCP round-trip, and transport parity.

Named deltas: /seed_session carries injected mcp metadata; new /mcp endpoint; catch-all
now installs at lifespan startup (bytes identical under a running app).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…tool closures

model_post_init registers one closure per get_tools() schema (hand-authored dicts
advertised verbatim over MCP; raw-argument passthrough on both transports). Closures
keep the seeded-session 400, the None-filter, and the 200 error-string conversion
verbatim; handle_unknown_tool reproduces the historical unknown-tool bytes. 35 tests
green incl. 25 byte-equal HTTP replays, MCP round-trip (27 tools), transport parity.

Named deltas (malformed-input paths only): unparseable JSON now treated as {} (was
422); non-object JSON 422 body text differs; sync pandas tools now run in a threadpool
instead of serializing on the event loop.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…tool closures

ToolManager init + discovery (and the sidecar it depends on) move to model_post_init;
each discovered tool registers a closure with its nemo_skills JSON schema (advertised
verbatim, raw-argument passthrough). Closures keep timing bookkeeping, the request_id
session wiring, and the str/json.dumps conversion INSIDE so text/plain bytes stay
identical; handle_unknown_tool preserves the 200 JSON-error body.

Named deltas: missing nemo_skills now raises at construction (module import is safe
without it); unparseable JSON to a known tool runs with empty args (was 200 parse-error
text); non-object bodies 422; payload session_id keys stripped; reserved/duplicate
discovered tool names now fail loudly. Full-dep runtime verification deferred to
gym env test --resources-server ns_tools (nemo_skills not in the root venv).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
MCP-env tool discovery moves to model_post_init; each tool registers with its ORIGINAL
JSON schema (advertised verbatim over MCP — fidelity win vs the lossy type round-trip)
and validate=True keeps the shallow 422 gate on HTTP. Non-MCP envs register one 'step'
tool LAX, preserving the 200 {error, result} soft-error contract byte-for-byte
(decision: no 422 upgrade on a shipped benchmark). 43 tests green.

Named deltas: /seed_session carries injected mcp metadata; unknown-path 404 body now
lists available tools; 422 bodies differ in shape; empty /step body tolerated as {};
payload session_id keys stripped; discovery failures surface at construction.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…rom the mapping)

run_experiment_* tools register from MODULE_REQUEST_CLASSES_MAPPING with each module's
request class as input_schema — HTTP validation stays byte-identical — replacing the
dir scan whose blanket try/except silently dropped later modules on an unmapped
upstream dir (verified bug, now fixed). execute_python/end_session become @gym_tool
methods; the TTL-cleanup lifespan override stays. 76 tests green incl. 11 byte-equal
HTTP replays, MCP round-trip, transport parity.

Named deltas: MCP-advertised schemas are raw model_json_schema (dataset rows keep the
strict-mode massaged form and remain the model-facing truth); unknown-path 404 body
lists available tools; /seed_session carries injected mcp metadata.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…task MCP scoping

Each TOOL_HANDLERS entry registers a runtime gym_tool closure (permissive object
schema — per-task schemas live in the dataset rows, which remain the model-facing
truth for HTTP agents; raw-argument passthrough keeps aliased send_message calls
verbatim). Closures keep the seeded-session 400, None-filter, str/json.dumps
conversion, and 200 error-string contract byte-for-byte; handle_unknown_tool
reproduces the historical 400-unseeded / 200 'Unknown tool' behavior.

Per-task visibility over MCP: seed_session extracts the row's tools[] (agents POST the
full run body) and mints the signed allowed_tools claim — tools/list shows and
tools/call permits exactly that task's tools for the session. HTTP routes stay
unrestricted (status quo; MCP is strictly tighter than HTTP, never looser).

The MCP surface is NEW and additive; flagging for benchmark-owner review since the
MCP-advertised schemas are permissive objects rather than the per-domain row schemas.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…ration

Both tools flatten to typed params (query/url), keeping HTTP bodies byte-identical;
now also served over MCP. Replay, MCP round-trip, and parity tests added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
BaseModel-schema mode (existing request classes stay the validators); request.session
reads swap to injected session_id. Mirrors the landed newton_bench migration. Named
delta: body-less /end_session now 422 (no repo caller sends one).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…d harness-endpoint

Evidence-based non-migration: /compare is invoked by agent harness code, never by the
model (absent from every dataset row's tools[]) — it stays a plain HTTP route, like
seed_session/verify. Tests pin the current wire contract.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…ym_tool

BaseModel-schema mode; session-keyed metrics move from request.session reads to the
injected session_id; the aiohttp Tavily client is untouched and tools stay async.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
Wire bytes verified against a hand-written reference route (nominal, 422, null,
non-JSON cases).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
Same treatment as its tavily_search sibling; tools gained docstrings (from dataset
descriptions) so MCP tools/list descriptions are non-null.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…ple to gym_tool

increment_counter/get_counter_value become typed @gym_tool methods with injected
session_id — the showcase of the stateful pattern. Named delta: body-less
/get_counter_value now 422 (agents always POST a JSON object).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
Devil's-advocate review (verified against the installed SDK) confirmed the MCP low-level
handlers rebuilt per request what the HTTP twin already precomputes: create_model +
model_validate on every validate=True dict-tool call (~0.19 ms), model_json_schema on
every tools/list per BaseModel-schema tool (~0.22 ms), plus per-call inspect.signature
and URLSafeSerializer construction. Precompute all of it once in _setup_mcp via a
_RawToolEntry record (validator, advertised schema, mcp.types.Tool, body-param name,
sync/async + session flags) and cache the token serializer. tools/list now reuses
precomputed Tool objects and only runs the per-session allowed_tools filter; the raw
call path does zero rebuilds (288x faster validate path, measured).

Also tighten the mcp pin >=1.27,<2 -> <1.29 and comment the private-SDK-attr use sites
(mcp._mcp_server / _tool_manager): those aren't covered by the SDK's public API
stability, so the ceiling should move deliberately after the dual-registration suite
passes on a new minor. No behavior change; 50 core + openenv/newton/workplace/IPI
suites green, base file coverage 98%.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…tching on {}

Differential regression testing (old vs new code, byte-diffed) found the dict-schema
HTTP handler coerced a non-empty *unparseable* JSON body to {} and dispatched the tool
anyway — for openenv this stepped the environment (a real state side effect) on garbage
input, and every lax dict-schema tool returned a 200 soft error where the pre-migration
typed-body route returned 422. Distinguish a genuinely empty body (tolerated as {}, the
named delta) from a non-empty malformed body (now 422, matching pre-migration). Adds a
regression guard asserting the tool is not dispatched. Happy paths already byte-identical
across all 14 migrated servers; this restores parity on the malformed-input edge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…-807 net lines)

A simplification review (6 hunters + devil's-advocate convergence loop, 2 rounds,
22 endorsed / 1 dropped / 0 contested) found the branch could sit much closer to main
with zero behavior change. Applied per-recommendation:

Base (nemo_gym/base_resources_server.py): drop write-only _mcp_mounted; _RawToolEntry
becomes a slotted dataclass without its never-read spec field; the HTTP twin reuses the
precomputed body_param/validator from _raw_dispatch (deletes a duplicated unreachable
raise); inert ConfigDict removed; dead guard on the dynamic-tools loop collapsed; the
str->text/plain conversion and route registration now exist once in the shared invoke
closure; passthrough hoisted; vestigial resolve_session_id parameter removed (the raw
call path is MCP-only).

Servers pulled back toward main: openenv (dead _mcp_tools cache deleted; two
pure-relocation methods moved back — their diff hunks collapse to zero),
workplace_assistant (handle_unknown_tool reuses the closure factory),
math_advanced_calculations (main's named-import block restored),
example_session_state_mgmt (incidental rewrites reverted; BaseVerifyResponse stays
imported rather than shadowed), math_with_code (end_session flattened to a bare
@gym_tool; empty EndSessionRequest model deleted).

Tests: new nemo_gym/mcp_test_utils.py holds the MCP wire-test scaffolding once
(handshake/list/call/seed/parity) — 15+ per-file copies deleted; function-local
TestClient imports hoisted; one exactly-duplicate test removed.

Deliberately NOT applied: the claude_code_agent logging revert (keeps the
silent-zero-reward diagnostic) and the ns_tools import-guard revert (the apply agent
proved the guard is load-bearing for config-only tests and self-reverted — the one DA
endorsement that failed in practice).

All 17 server suites + 833 core tests green; base file coverage 99%.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…CP rollouts

MCP-native agents record tool calls namespaced (mcp__<server>__<tool>); HTTP agents
record bare names. Trajectory-scoring verifiers compared raw names, so an identical,
correct Claude-over-MCP workspace rollout scored 0.0 (proven live). Adds
normalize_tool_name to the base (module function + server method bound to the server's
own name) and applies it at every migrated verifier that reads trajectory names:
workplace_assistant (is_correct extraction), finance_sec_search (submit_final_result),
example_multi_step, circle_click, indirect_prompt_injection (extract_function_calls).
The saved Claude MCP rollout now verifies at reward 1.0 through the real endpoint;
regression test asserts namespaced == bare scoring. HTTP scoring is byte-unchanged
(bare names pass through untouched). All touched suites + 835 core tests green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
A test-redundancy audit (194 branch-added tests classified, deletion candidates
coverage-diffed with/without and defended through a 2-round devil's-advocate loop)
endorsed exactly 8 deletions — per-server copies of base-class behavior already pinned
by the central dual-registration suite or by a same-file sibling (seed_token() already
fails if the seed metadata vanishes; the no-token clean-error path keeps its tavily +
browsecomp representatives per the DA's explicit condition). Coverage is identical
with and without each deleted test, measured. 2 candidates were rescued by the DA,
1 left contested and kept (legacy bare-token compatibility). Suites re-run green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…ove lazy mcp import

Reviewer-feedback round on the diff itself:
- The gym_tool contract docstring is rewritten in plain language: when to declare
  session_id (stateful vs stateless), a concrete example of why a wrapping Pydantic
  parameter would give one tool two conflicting argument shapes, what each
  input_schema mode is for and why the model-class mode exists (preserving a migrated
  route's exact validation), and the reason error shapes differ per transport (JSON-RPC
  separates transport success from tool failure; plain HTTP only has status codes).
- Capitalized-for-emphasis words in comments/docstrings are rewritten across the diff
  (25 fixes, 15 files); acronyms and literal values untouched.
- New subprocess test proves the lazy gate: building a tool-less server never imports
  the mcp package (asserted on sys.modules in a fresh interpreter).

836 core tests green; no behavior changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
A typed (input_schema=None) tool whose only parameter is a Pydantic model is ambiguous:
the schema derives from the parameter list, so callers would have to nest every argument
under that parameter's name, while the author almost always meant the model's fields as
the flat schema (input_schema=<Model>). Instead of guessing or warning, registration now
raises with the fix spelled out — matching how reserved names, duplicates, and arity
violations already fail loudly at setup. The docstring states the rule; a test pins the
error, and the nested-model parity test keeps its coverage via a two-parameter tool.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
…ameter rejection

A cold-eyes verifier live-reproduced a gap: the ambiguity check lived only in
_register_gym_tool, which a subclass overriding register_mcp_tools without super()
never reaches — the HTTP twin then silently registered the ambiguous tool with a
nested body and no MCP counterpart (invisible to the parity warning, which only flags
MCP-only tools). The check is extracted into _check_no_single_model_param and now also
runs in _collect_gym_tools — the one chokepoint every registration path crosses — so
the bypass raises at startup. Regression test reproduces the exact bypass. Repo sweep
confirmed no real tool uses the ambiguous shape and no docs teach it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex <codex@openai.com>
@adil-a
adil-a requested a review from a team as a code owner July 12, 2026 23:39
@copy-pr-bot

copy-pr-bot Bot commented Jul 12, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@adil-a

adil-a commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 161a44d

@github-actions

Copy link
Copy Markdown
Contributor

🌿 Preview your docs: https://nvidia-preview-mcp-dual-registration.docs.buildwithfern.com/nemo/gym

Here are the markdown pages you've updated:

CI runs each server's pytest from inside the server directory, so the
repo-root-relative path to data/example.jsonl raised FileNotFoundError
(the only failure in the suite). Resolve via __file__ so the test passes
from any working directory.

Signed-off-by: Codex <adasif@nvidia.com>
Signed-off-by: Codex <codex@openai.com>
@adil-a

adil-a commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 3b4bb9d

…ansport conflict

The gym_tool docstring claimed a lone Pydantic-model parameter would produce
conflicting argument shapes between HTTP (flat) and MCP (nested). Probing the
actual dual registration with the check disabled shows both transports nest
(_synth_body_model mirrors the signature), so the shapes never diverge. The
real reason for the startup rejection is author intent: writers of that shape
usually mean the model's fields flat and would silently ship nested on both
doors. Reword the docstring, the startup error, and the test docstring; the
tested error substring (input_schema=<Model>) is unchanged.

Signed-off-by: Codex <adasif@nvidia.com>
Signed-off-by: Codex <codex@openai.com>
@adil-a

adil-a commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 1e40fd6

@ffrujeri ffrujeri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.The review-ordered commits and the contract table in the description made it easier to follow, and serving each tool over both HTTP and MCP from one @gym_tool declaration is a reasonable unification. The core mechanism holds up under scrutiny: session threading across both transports, the raw-argument dispatch that deliberately bypasses FastMCP's argument-dropping, the per-transport error contracts, and the 99% base-file coverage all check out.

Background (for reviewers new to this surface)

Where this sits

Every resources server inherits SimpleResourcesServer, so this changes a shared contract: the seed_session response (an additive mcp key carrying a signed per-rollout session token), tool dispatch, per-transport error shapes, and verifier tool-name handling. Both transports resolve the same per-rollout session id; verifiers normalize_tool_name so MCP-namespaced (mcp__<server>__<tool>) and bare HTTP names score identically.

flowchart LR
    Model[Model Server] -->|tool-call decisions| SA[simple_agent]
    Model -->|tool-call decisions| CC[claude_code_agent]
    SA -->|"POST /&lt;name&gt; (args)"| HTTP
    CC -->|"POST /mcp · tools/call"| MCP
    subgraph RS[SimpleResourcesServer]
      GT["@gym_tool"] -.->|registers at startup| HTTP[HTTP twin]
      GT -.->|registers at startup| MCP[MCP twin + raw dispatch]
      HTTP -->|writes state under session id| ST[(per-rollout session)]
      MCP -->|writes state under session id| ST
      ST -->|read at scoring| VER["verify()<br/>normalize_tool_name"]
      SEED[seed_session]
    end
    SEED -->|"mcp key: /mcp path + signed token"| CC
    VER -->|reward| RL[(reward profiling / training)]
Loading

Solid arrows: runtime data flow (label = what moves). Dashed arrows: startup-time registration.

One design alternative worth a note in the docs

Was route auto-exposure considered as an alternative to explicit declarations — e.g. fastapi-mcp-style introspection of existing FastAPI routes, or a hook on route registration that publishes every custom route as an MCP tool, so authors don't wrap anything? Reading the migrated set, that approach covers only the easy half: typed-route servers introspect cleanly, but the dispatcher/registry servers (workplace_assistant's 27 registry tools, ns_tools' POST /{tool_name}, math_advanced_calculations' /{path}, indirect_prompt_injection's per-task schemas) have no per-tool routes to introspect — their schemas live in registries and dataset rows, so some per-tool declaration carrying {name, description, input_schema} is unavoidable there, which is what gym_tool's runtime-call form is.

Any auto-exposure design does need one exclusion mechanism: verify/seed_session/aggregate_metrics are structurally indistinguishable from tools (POST routes with a single Pydantic body model), so introspection alone can't tell them apart. A static denylist of the reserved routes handles that cleanly — it's the same set this PR already maintains as RESERVED_MCP_TOOL_NAMES, and fastapi-mcp ships the equivalent as exclude_operations. So exclusion is solvable; the structural limitation is really the dispatcher/registry half above. (fastapi-mcp 0.2.0 also rides SSE, which Streamable HTTP is replacing.)

Two small asks come out of this: it would be worth capturing the "why not auto-expose routes?" rationale briefly in the tutorial so the question has a durable answer; and if zero-wrapping ever matters for external authors, an opt-in helper that introspects a subclass's typed routes and registers them through gym_tool internally would be clean follow-up sugar on top of this mechanism (a sketch, not a request for this PR). For what it's worth, the byte-replay tests establish the wire payloads didn't change — the migration cost is authoring shape only.

A follow-up suggestion: env lifecycle vs tools (Gymnasium lens)

The migration surfaces a contract question worth a follow-up issue: for environment-shaped servers, which operations are tools (model-facing affordances) and which are reserved lifecycle (harness-facing, like seed_session/verify)? The classic Gym/Gymnasium contract gives a principled split — reset()seed_session, step(action) ↔ a tool, close() ↔ reserved lifecycle — and openenv in this PR already ships step as a gym_tool. But who triggers teardown differs per server today:

  • math_with_code.end_session / newton_bench.end_session are @gym_tools — cleanup runs only if the model decides to call it, and the teardown button is advertised in tools/list on both transports;
  • openenv.verify() hand-codes cleanup inline — which quietly makes verify destructive: its reward falls back to 0.0 when the session is gone, so a second verify call on the same rollout scores 0.0 instead of the true reward (scoring and episode-end are fused);
  • unmigrated env wrappers like aviary use a separate /close route the harness must know to call — the right shape, but per-server convention rather than contract.

Suggestion for the follow-up: promote close to a base-provided reserved endpoint, mirroring how this PR treats seed_session. Servers never write the route — they implement a close_session(session_id) hook (idempotent, never in tools/list), and the base routes POST /close into it, resolving the session id from the same signed cookie verify already uses. The agent harness calls it once in its common rollout path (a finally after verify); an idle-expiry sweep in the base backstops crashed or non-compliant harnesses by calling the same hook:

sequenceDiagram
    participant AG as Agent harness
    participant RS as Resources server (base)
    participant SUB as Subclass

    AG->>RS: POST /seed_session          (reset)
    AG->>RS: tool calls                   (step — HTTP or MCP)
    AG->>RS: POST /verify                 (score — pure, repeatable)
    RS->>SUB: verify(body) → reward
    AG->>RS: POST /close (session cookie) — finally-block after verify
    RS->>SUB: close_session(session_id)   — base-routed hook
    Note over RS,SUB: idle-expiry sweep also calls close_session()<br/>— backstop for crashed rollouts / non-compliant harnesses
Loading

This keeps verify pure (repeatable scoring, no teardown latency or failures inside the scoring response), matches the Gymnasium contract exactly, and still guarantees cleanup via the sweep. A nice detail: no migrated server declares a tool named close, so close can join RESERVED_MCP_TOOL_NAMES cleanly — unlike retrofitting end_session, which would break the two servers above at startup. Rounding out the contract: step-style action tools should pass the env's action schema via input_schema= rather than a weak step(**anything) signature, and openenv's accumulate-then-verify() reward adapter becomes the documented pattern (minus the inline close). This also sharpens the auto-exposure question above: an env's step is legitimately a tool while its close never should be — the line becomes contractual instead of a per-server accident. None of this needs to happen in this PR.

The inline notes are small: two doc-accuracy points (the allowed_tools wording and the /mcp trust-boundary note) and a defensive fix for _model_from_json_schema. Nothing blocking from my side. Nice work.

@mcp._mcp_server.call_tool(validate_input=False)
async def _gym_call_tool(name: str, arguments: Optional[dict]) -> Any:
_, allowed = self._mcp_session_claims(required=False)
if allowed is not None and name not in allowed:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nemo_gym/base_resources_server.py:544

The MCP-only scope of allowed_tools is clearly deliberate (per the PR description) and this isn't re-litigating it — it's about the token-validation path within MCP. When X-NeMo-Gym-Session-Token is missing or fails signature verification, _mcp_session_claims(required=False) returns (None, None), so this call-gate and the tools/list filter are skipped: a tokenless client sees all tools and can call any stateless one (stateful tools still raise via require_mcp_session_id()). Not model-reachable — the harness sets the header — and with HTTP unrestricted by design there's no marginal exposure, so this is a docs-accuracy point: the class docstring, build_mcp_session_metadata, and the tutorial describe allowed_tools as restricting "which tools an MCP session can list and call," which reads as enforcement. Consider describing it as cooperative per-session scoping — and since test_garbage_token_lists_everything suggests the fail-open is intended, worth stating that explicitly in the docstring.

> Need full manual control of the MCP surface? Override `register_mcp_tools(self, mcp)` — call `super().register_mcp_tools(mcp)` first to keep the auto-registered ones. Note that hand-written `@mcp.tool()` registrations are **MCP-only** (no HTTP twin; the base logs a warning), so prefer `gym_tool` declarations.

<Note>
The Gym MCP mount disables the MCP SDK's default DNS-rebinding protection (`TransportSecuritySettings(enable_dns_rebinding_protection=False)`). That protection only accepts loopback `Host` headers and returns HTTP `421` otherwise — which would break multi-node / `use_absolute_ip=True` deployments where the agent reaches the server by a routable host. The endpoint is instead protected by the per-rollout session token. You don't need to set this yourself; the base class handles it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fern/versions/latest/pages/environment-tutorials/mcp-resources-server.mdx:165

Small doc-accuracy point: describing /mcp as "protected by the per-rollout session token" conflates session binding with authentication. The token's signing key is deterministic (ClassName___config.name) and DNS-rebinding protection is disabled here, so the real boundary is network isolation — the token binds a request to a rollout session, it isn't a secret. This is the same trust model as the pre-existing signed session cookie (so not a new concern), but a one-line note that /mcp must not face untrusted networks would set expectations correctly.

fields: dict[str, Any] = {}
required = set(schema.get("required", []))
for prop_name, prop in schema.get("properties", {}).items():
python_type = type_map.get(prop.get("type", "string"), str)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nemo_gym/base_resources_server.py:741

Two edge cases here for validate=True dict-schema tools: a property using the standard nullable form "type": ["string", "null"] makes dict.get([...]) raise TypeError: unhashable type: 'list' at startup (the server fails to boot), and an integer/number enum or $ref with no explicit "type" is coerced to str, so a valid int argument is wrongly 422'd over HTTP. Only openenv uses validate=True today (and its own _json_type_to_python shares the crash), so impact is low, but guarding list/non-str type values would harden the helper for other dict-schema authors:

Suggested change
python_type = type_map.get(prop.get("type", "string"), str)
prop_type = prop.get("type", "string")
if isinstance(prop_type, list): # nullable form ["string", "null"] — first non-null wins
prop_type = next((t for t in prop_type if t != "null"), "string")
python_type = type_map.get(prop_type, str)

adil-a pushed a commit that referenced this pull request Jul 16, 2026
Replaces the always-replay path with the middle-ground design the
middle-ground workflow validated: run each frozen origin/main handler
directly ONCE (fabricated Request whose .session is served by a public
Request subclass), no second app pass. A startup detector (hybrid_dispatch:
server-level middleware audit + route-level signature bind) falls a route
or the whole server back to the in-process replay bridge wherever it cannot
prove direct == a real HTTP request (custom middleware, unsupported handler
shapes).

All 32 in-tree tools (finance 5 + workplace 27) dispatch DIRECT, zero
replay; adding a custom middleware flips that server to replay
automatically (verified). 37/37 acceptance checks pass against pristine
origin/main handler code. Uses only public FastAPI/Starlette/pydantic
surface for dispatch (no solve_dependencies, body_field, or dependant).

If adopted, this synthesizes the equivalent of #2002's @gym_tool
registrations from the existing routes, so the ~861-line migration across
16 tool files could revert.

Signed-off-by: Codex <adasif@nvidia.com>
Signed-off-by: Codex <codex@openai.com>
@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Jul 17, 2026
@adil-a adil-a closed this Jul 21, 2026
bxyu-nvidia pushed a commit that referenced this pull request Jul 22, 2026
> **Draft / RFC.** Consolidates the exploration in #2002 and #2053 into
one tracked module + a small framework hook, **removes #1682's decorator
API** (`@gym_tool` / `MCPResourcesServer`) so there is exactly one
user-facing MCP mechanism, then hardened by a full review pass (details
below). Branched off `main`.

## What this is

Serve a resources server's **existing** FastAPI tool routes over MCP
with **zero handler changes** and **one opt-in config field** — no code
change at all for a typed-route server:

```yaml
resources_servers:
  my_server:
    entrypoint: app.py
    expose_tools_over_mcp: true     # the only addition — set per instance in the config
```

`run_webserver` installs the `/mcp` endpoint automatically after the app
is built (and imports the MCP SDK only for servers that opt in).
Handlers keep their `request: Request` param and `request.session[...]`
reads exactly as written. The only in-tree code a server ever needs is
for shapes the config can't express, and it is one override:
`mcp_tools(harvested, catchall)` — return the harvested typed routes
(the default), filter one out to exclude a harness-only route (e.g.
`/end_session`), or append catch-all-backed tools (`harvested +
[catchall.tool(name, input_schema, description)]`) for dispatcher
servers. A dropped route is never advertised, never callable over MCP,
never shape-checked, and unchanged over HTTP.

This PR also **removes the decorator-based MCP API** that #1682 added
(`@gym_tool`, `MCPResourcesServer`, `MCPSessionError`, the token
contextvar and header middleware): tools no longer need decorators or
MCP-specific signatures, so the old mechanism is superseded rather than
parallel. The wire contract survives (`MCPServerMetadata`, the
`X-NeMo-Gym-Session-Token` header, the token salt, the reserved-name
set) — `claude_code_agent` on `main` consumes it unchanged.
`example_mcp_weather` is rewritten onto auto-exposure: a plain typed
`POST /get_weather` route plus the yaml flag, with tests driving the MCP
door (and the wrapped `/seed_session` + `/verify`) through the real
engine mount, and the plain-HTTP tool route on the stock app.

## How to review this (~30 min reading order)

1. `nemo_gym/mcp_auto_exposure.py` module docstring — the contract in
one screen.
2. `bind_route` — the detector. The accept/refuse table below is what it
enforces.
3. `call_direct` — dispatch parity with the plain HTTP route (threadpool
for sync handlers, body bytes, session cookie, `response_model`
filtering, error-text parity).
4. `harvest_tools` → `install_auto_exposure` — tool map, startup guards,
session token, `/seed_session` wrap, `/mcp` mount.
5. `base_resources_server.py` diff — the `expose_tools_over_mcp` config
field, the two overridables (`mcp_tools(harvested, catchall)` — what to
expose; `mcp_allowed_tools_for_session(seed_body)` — per-rollout
narrowing), and the removal of the #1682 decorator API (the file drops
from 315 lines on `main` to 150; it had grown to 356 on this branch
before the removal commit).
6. `server_utils.py` — the 6-line activation point (plus a 4-line
session-middleware idempotency guard).
7. `tests/unit_tests/test_mcp_auto_exposure.py` — 45 tests double as the
spec; each guard and parity shape has a named test.

## What the detector accepts vs refuses

**Accepts (dispatches identically to the plain HTTP route):** async and
sync handlers (sync runs via `run_in_threadpool`, exactly like FastAPI);
`body: Model`; `body: dict` / `dict[str, Any]`; raw-body catch-alls
reading `await request.json()`; handlers taking both a body model *and*
a `request` they read raw; `str` path params; `response_model=`
filtering (decorator kwarg or return annotation).

**Refuses loudly at startup (`ValueError` naming the route and
reason):** non-Gym middleware; `Depends`/`Security`; `*args/**kwargs`;
multiple body models; union/optional body params; defaulted query
params; unresolvable annotations; tool names outside `^[A-Za-z0-9_-]+$`
(e.g. nested routes); tool names (from `mcp_tools()`) colliding with
`verify`/`seed_session`/`aggregate_metrics`/`mcp`; duplicate tool names;
multiple parameterized catch-all routes (auto-exposure cannot tell which
backs the tools); a server that already serves `/mcp` (a hand-rolled MCP
mount would be shadowed); a missing `/seed_session` or `/verify`. These
rules apply to route-handler signatures only — inner tool functions may
use `**kwargs` freely, and no in-tree server trips any refusal.

No silent fallback exists: a shape either dispatches provably like HTTP
or the server does not start. One soft case: a dispatcher whose
`mcp_tools()` override ignores its catch-all logs a warning (the
catch-all-backed tools stay HTTP-only).

## Session & token

`/seed_session` gains an additive `"mcp"` key (`MCPServerMetadata` — the
same shape `claude_code_agent` on `main` already consumes): the `/mcp`
URL plus a signed per-rollout token in `X-NeMo-Gym-Session-Token`. The
token payload is always `{"sid", "tools"}` — the session id plus this
rollout's allow-list from `mcp_allowed_tools_for_session(seed_body)`
(`null` = unrestricted) — signed with `URLSafeSerializer` (untimed;
expiry was cut as consumer-less scope). The header/salt/metadata
constants live in `base_resources_server` — the wire contract retained
from #1682's scheme after its decorator API was removed. The token is
verified on every call (no cache). `tools/list` works tokenless and
advertises the full exposed set; a token carrying an allow-list narrows
both `tools/list` and `tools/call` for that session. Known limitation,
deliberately out of scope: the signing secret derivation
(`class___config-name`) is pre-existing `main` behavior (the
session-middleware secret) — hardening it is a separate change.

## Verify-time tool-name normalization (install-time, gated)

MCP-native agents record trajectory tool calls namespaced
(`mcp__<server>__<tool>`); verifiers know bare names — found live when
MCP rollouts scored 0.0 on perfect trajectories. For **flag-on servers
only**, installation wraps the `/verify` route's current endpoint
(whatever handler it holds — servers that re-register `/verify` are
covered by construction): names are normalized for scoring on a deep
copy, then the model's original names are restored in the echoed
response (matched by `call_id`). Flag-off servers — every existing
benchmark — keep verify byte-for-byte, so no baselines move; persisted
rollout artifacts always keep the names the model actually emitted.

## Diff shape (+2,136 / −631, 11 files)

| Area | Change |
|---|---|
| `nemo_gym/mcp_auto_exposure.py` | **new, one file, 841 lines** —
detector, direct dispatcher, harvest + guards, token, `/seed_session` +
`/verify` wraps, `/mcp` mount |
| `nemo_gym/base_resources_server.py` | +48/−207: adds the
`expose_tools_over_mcp` config field, the `normalize_tool_name` helper,
and the `mcp_tools()` / `mcp_allowed_tools_for_session()` overridables;
**removes** `@gym_tool`, `MCPResourcesServer`, `MCPSessionError`, and
the header middleware (their unit tests go too:
`test_base_resources_server.py` −277) |
| `nemo_gym/server_utils.py` | +10: `run_webserver` activates
auto-exposure for opted-in servers (lazy MCP import);
`setup_session_middleware` is now idempotent |
| `tests/unit_tests/test_mcp_auto_exposure.py` | **new — 45
self-contained tests** (TestClient, synthetic servers, ~3 s); +37 in
`test_server_utils.py`; docs section in
`fern/.../mcp-resources-server.mdx` |
| `resources_servers/` | `example_mcp_weather` rewritten onto
auto-exposure (plain typed route + yaml flag; 6 tests: the MCP door —
plus the wrapped `/seed_session` and `/verify` — through the real engine
mount, and the plain-HTTP tool route on the stock app). **Every other
server: byte-identical to `main`.** Enablement stays per-server
follow-up: pure config for typed-route servers; a small `mcp_tools()`
override for catch-all dispatchers. |

MCP engine: the official SDK's **public low-level** `Server` — no
private-attr access.

## Review provenance

This branch already went through a 7-reviewer + devil's-advocate review
pass (59 confirmed findings) followed by a fix pass with adversarial
re-verification: all 55 code findings are fixed here with tests
(silently-wrong detector shapes, refuse-loudly gaps, an unbounded token
cache — the token is now verified on every call with no cache, and
expiry was later cut as consumer-less scope — event-loop blocking for
sync handlers, provenance-preserving verify normalization), and a
docstring-accuracy audit (77 reviewed, 3 corrected). The remaining
follow-ups are listed under "For discussion".

## Verified

- **Reproducible from this PR:** `pytest
tests/unit_tests/test_mcp_auto_exposure.py` → **45/45** (~3 s; covers
every refusal, parity shape, token path, exclusion semantics, and the
normalization gating/provenance — including a re-registered-`/verify`
regression test).
- **Live e2e at this HEAD (`3891dd65`):** `workplace_assistant` MCP door
(`claude_code_agent`) — **5/5 `reward: 1.0`**, all 8 distinct tools
namespaced; HTTP door (`simple_agent`) — **5/5** on the second run, 4/5
on the first (the failing rollout omitted the `status: "Lead"` search
filter — a trajectory-verified policy flake, not an engine failure; the
HTTP door runs the stock route code). `example_mcp_weather`
(`claude_code_agent`) — **5/5 `reward: 1.0`** at the removal commit
`4d6d569c`; the one commit since is docs plus a comment reword. Earlier
demo (below): both doors 5/5 tasks `reward: 1.0`, ~30 s wall for 5
concurrent rollouts, MCP-door trajectories persist the namespaced names
the model emitted. n=5 single runs of a stochastic policy — a smoke
demonstration that both transports work end to end, not a statistical
parity claim.
- **Development-time only (local, gitignored — not reproducible from
this PR):** a 44-check live acceptance harness exercising six
representative in-tree servers (finance, workplace, aviary,
newton_bench, openenv, ns_tools) against pristine `origin/main`
handlers, covering the factory-`__signature__`, `dict`-body, and
raw-body/`PlainTextResponse` shapes; 44/44 as of 2026-07-17 against a
pre-refactor revision of the engine (development-time evidence; the
harness predates the `mcp_tools()` API). Other in-tree servers were
covered by a static route sweep, not this live harness.

## Runnable e2e — both doors through `gym eval run`

The full 5-task `workplace_assistant` example dataset, driven end to end
by Gym. Because `resources_servers/` ships unchanged, the demo uses a
small user-side entrypoint (`app_mcp.py`, shown in the docs added by
this PR) that subclasses the stock server and overrides `mcp_tools()` to
append the 27 catch-all-backed tools; the flag itself is plain yaml:

Compose config (`workplace_2059.yaml`) — MCP door via
`claude_code_agent` (already on `main`; it reads the `mcp` key
`/seed_session` returns and wires the Claude Code binary itself), HTTP
door via `simple_agent`:

```yaml
workplace_assistant:
  resources_servers:
    workplace_assistant:
      entrypoint: app_mcp.py   # thin subclass overriding mcp_tools(); see the docs page
      domain: agent
      expose_tools_over_mcp: true

workplace_claude:
  responses_api_agents:
    claude_code_agent:
      entrypoint: app.py
      resources_server: {type: resources_servers, name: workplace_assistant}
      model: ${anthropic_model_name}
      anthropic_api_key: ${anthropic_api_key}      # env.yaml; any Anthropic-format endpoint
      anthropic_base_url: ${anthropic_base_url}    # optional gateway; CLI appends /v1/messages

policy_model:
  responses_api_models:
    inference_provider:
      entrypoint: app.py
      base_url: ${policy_base_url}                 # any OpenAI-format endpoint
      api_key: ${policy_api_key}
      model: ${policy_model_name}

workplace_simple:
  responses_api_agents:
    simple_agent:
      entrypoint: app.py
      max_steps: 12
      resources_server: {type: resources_servers, name: workplace_assistant}
      model_server: {type: responses_api_models, name: policy_model}
```

```bash
gym env start --config workplace_2059.yaml

# (a) MCP door — Claude Code discovers and calls the tools over /mcp with the per-rollout token
gym eval run --no-serve --agent workplace_claude \
  --input resources_servers/workplace_assistant/data/example.jsonl \
  --output results/workplace_claude_rollouts.jsonl

# (b) HTTP door — the same tools as plain POST /<tool_name>
gym eval run --no-serve --agent workplace_simple \
  --input resources_servers/workplace_assistant/data/example.jsonl \
  --output results/workplace_simple_rollouts.jsonl
```

Tip: if your provider's OpenAI *Responses-API* emulation is incomplete
(a common gateway gap on multi-turn tool exchanges), use
`inference_provider` (chat-completions + Gym's converter) rather than
`openai_model`.

## Enablement (deliberately none in this PR)

This PR ships the engine plus the one in-tree example
(`example_mcp_weather`) — every other server under `resources_servers/`
is byte-identical to `main` (we cannot properly test each env here, per
review). Enabling a server is per-server follow-up work: set
`expose_tools_over_mcp: true` in its instance config (typed-route
servers need no code at all), plus a small `mcp_tools()` override for
catch-all dispatcher servers — the docs added in this PR walk through
workplace_assistant end to end. During development, a fleet exercise ran
`claude_code_agent` over `/mcp` against 12 servers enabled this way
(workplace_assistant 27 tools, math_advanced_calculations,
finance_sec_search, indirect_prompt_injection with per-task narrowing,
the examples, and others), all dispatching correctly; those results are
development-time evidence, not shipped code.

**Deliberately not exposed even as follow-up (each needs design work
first):** aviary (its `/step`/`/close` bodies carry an `env_id` that can
address other rollouts' environments — needs session-derived lookup),
genrm_compare (its only route is a harness-side batch API), and the
gymnasium family (reward accrues from `/reset`/`/step`, which a model
could drive to reroll its own episode). The 74 verify-only benchmarks
have no tool routes to expose.

## For discussion
- Flag is **opt-in (default off)** — auto-exposing every route isn't
always wanted (aviary's `/step` carries `env_id`; exposing it lets a
model address other rollouts' envs).
- Follow-ups deliberately not in this PR: session-binding aviary's
`env_id` so it can be exposed safely; an MCP-safe reward design for the
gymnasium family; and hardening the pre-existing token-signing secret
derivation shared with #1682.
- **#1682's decorator API is removed in this PR** rather than left as a
parallel mechanism: auto-exposure reads the routes authors already
wrote, so `@gym_tool` had no remaining role, and keeping both would have
forced every author to choose. `example_mcp_weather` (the only in-tree
consumer) is migrated here; #2002's dual-registration approach is
superseded.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Signed-off-by: Codex <adasif@nvidia.com>
Signed-off-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
OlegSudakov pushed a commit to OlegSudakov/Gym that referenced this pull request Aug 7, 2026
> **Draft / RFC.** Consolidates the exploration in NVIDIA-NeMo#2002 and NVIDIA-NeMo#2053 into
one tracked module + a small framework hook, **removes NVIDIA-NeMo#1682's decorator
API** (`@gym_tool` / `MCPResourcesServer`) so there is exactly one
user-facing MCP mechanism, then hardened by a full review pass (details
below). Branched off `main`.

## What this is

Serve a resources server's **existing** FastAPI tool routes over MCP
with **zero handler changes** and **one opt-in config field** — no code
change at all for a typed-route server:

```yaml
resources_servers:
  my_server:
    entrypoint: app.py
    expose_tools_over_mcp: true     # the only addition — set per instance in the config
```

`run_webserver` installs the `/mcp` endpoint automatically after the app
is built (and imports the MCP SDK only for servers that opt in).
Handlers keep their `request: Request` param and `request.session[...]`
reads exactly as written. The only in-tree code a server ever needs is
for shapes the config can't express, and it is one override:
`mcp_tools(harvested, catchall)` — return the harvested typed routes
(the default), filter one out to exclude a harness-only route (e.g.
`/end_session`), or append catch-all-backed tools (`harvested +
[catchall.tool(name, input_schema, description)]`) for dispatcher
servers. A dropped route is never advertised, never callable over MCP,
never shape-checked, and unchanged over HTTP.

This PR also **removes the decorator-based MCP API** that NVIDIA-NeMo#1682 added
(`@gym_tool`, `MCPResourcesServer`, `MCPSessionError`, the token
contextvar and header middleware): tools no longer need decorators or
MCP-specific signatures, so the old mechanism is superseded rather than
parallel. The wire contract survives (`MCPServerMetadata`, the
`X-NeMo-Gym-Session-Token` header, the token salt, the reserved-name
set) — `claude_code_agent` on `main` consumes it unchanged.
`example_mcp_weather` is rewritten onto auto-exposure: a plain typed
`POST /get_weather` route plus the yaml flag, with tests driving the MCP
door (and the wrapped `/seed_session` + `/verify`) through the real
engine mount, and the plain-HTTP tool route on the stock app.

## How to review this (~30 min reading order)

1. `nemo_gym/mcp_auto_exposure.py` module docstring — the contract in
one screen.
2. `bind_route` — the detector. The accept/refuse table below is what it
enforces.
3. `call_direct` — dispatch parity with the plain HTTP route (threadpool
for sync handlers, body bytes, session cookie, `response_model`
filtering, error-text parity).
4. `harvest_tools` → `install_auto_exposure` — tool map, startup guards,
session token, `/seed_session` wrap, `/mcp` mount.
5. `base_resources_server.py` diff — the `expose_tools_over_mcp` config
field, the two overridables (`mcp_tools(harvested, catchall)` — what to
expose; `mcp_allowed_tools_for_session(seed_body)` — per-rollout
narrowing), and the removal of the NVIDIA-NeMo#1682 decorator API (the file drops
from 315 lines on `main` to 150; it had grown to 356 on this branch
before the removal commit).
6. `server_utils.py` — the 6-line activation point (plus a 4-line
session-middleware idempotency guard).
7. `tests/unit_tests/test_mcp_auto_exposure.py` — 45 tests double as the
spec; each guard and parity shape has a named test.

## What the detector accepts vs refuses

**Accepts (dispatches identically to the plain HTTP route):** async and
sync handlers (sync runs via `run_in_threadpool`, exactly like FastAPI);
`body: Model`; `body: dict` / `dict[str, Any]`; raw-body catch-alls
reading `await request.json()`; handlers taking both a body model *and*
a `request` they read raw; `str` path params; `response_model=`
filtering (decorator kwarg or return annotation).

**Refuses loudly at startup (`ValueError` naming the route and
reason):** non-Gym middleware; `Depends`/`Security`; `*args/**kwargs`;
multiple body models; union/optional body params; defaulted query
params; unresolvable annotations; tool names outside `^[A-Za-z0-9_-]+$`
(e.g. nested routes); tool names (from `mcp_tools()`) colliding with
`verify`/`seed_session`/`aggregate_metrics`/`mcp`; duplicate tool names;
multiple parameterized catch-all routes (auto-exposure cannot tell which
backs the tools); a server that already serves `/mcp` (a hand-rolled MCP
mount would be shadowed); a missing `/seed_session` or `/verify`. These
rules apply to route-handler signatures only — inner tool functions may
use `**kwargs` freely, and no in-tree server trips any refusal.

No silent fallback exists: a shape either dispatches provably like HTTP
or the server does not start. One soft case: a dispatcher whose
`mcp_tools()` override ignores its catch-all logs a warning (the
catch-all-backed tools stay HTTP-only).

## Session & token

`/seed_session` gains an additive `"mcp"` key (`MCPServerMetadata` — the
same shape `claude_code_agent` on `main` already consumes): the `/mcp`
URL plus a signed per-rollout token in `X-NeMo-Gym-Session-Token`. The
token payload is always `{"sid", "tools"}` — the session id plus this
rollout's allow-list from `mcp_allowed_tools_for_session(seed_body)`
(`null` = unrestricted) — signed with `URLSafeSerializer` (untimed;
expiry was cut as consumer-less scope). The header/salt/metadata
constants live in `base_resources_server` — the wire contract retained
from NVIDIA-NeMo#1682's scheme after its decorator API was removed. The token is
verified on every call (no cache). `tools/list` works tokenless and
advertises the full exposed set; a token carrying an allow-list narrows
both `tools/list` and `tools/call` for that session. Known limitation,
deliberately out of scope: the signing secret derivation
(`class___config-name`) is pre-existing `main` behavior (the
session-middleware secret) — hardening it is a separate change.

## Verify-time tool-name normalization (install-time, gated)

MCP-native agents record trajectory tool calls namespaced
(`mcp__<server>__<tool>`); verifiers know bare names — found live when
MCP rollouts scored 0.0 on perfect trajectories. For **flag-on servers
only**, installation wraps the `/verify` route's current endpoint
(whatever handler it holds — servers that re-register `/verify` are
covered by construction): names are normalized for scoring on a deep
copy, then the model's original names are restored in the echoed
response (matched by `call_id`). Flag-off servers — every existing
benchmark — keep verify byte-for-byte, so no baselines move; persisted
rollout artifacts always keep the names the model actually emitted.

## Diff shape (+2,136 / −631, 11 files)

| Area | Change |
|---|---|
| `nemo_gym/mcp_auto_exposure.py` | **new, one file, 841 lines** —
detector, direct dispatcher, harvest + guards, token, `/seed_session` +
`/verify` wraps, `/mcp` mount |
| `nemo_gym/base_resources_server.py` | +48/−207: adds the
`expose_tools_over_mcp` config field, the `normalize_tool_name` helper,
and the `mcp_tools()` / `mcp_allowed_tools_for_session()` overridables;
**removes** `@gym_tool`, `MCPResourcesServer`, `MCPSessionError`, and
the header middleware (their unit tests go too:
`test_base_resources_server.py` −277) |
| `nemo_gym/server_utils.py` | +10: `run_webserver` activates
auto-exposure for opted-in servers (lazy MCP import);
`setup_session_middleware` is now idempotent |
| `tests/unit_tests/test_mcp_auto_exposure.py` | **new — 45
self-contained tests** (TestClient, synthetic servers, ~3 s); +37 in
`test_server_utils.py`; docs section in
`fern/.../mcp-resources-server.mdx` |
| `resources_servers/` | `example_mcp_weather` rewritten onto
auto-exposure (plain typed route + yaml flag; 6 tests: the MCP door —
plus the wrapped `/seed_session` and `/verify` — through the real engine
mount, and the plain-HTTP tool route on the stock app). **Every other
server: byte-identical to `main`.** Enablement stays per-server
follow-up: pure config for typed-route servers; a small `mcp_tools()`
override for catch-all dispatchers. |

MCP engine: the official SDK's **public low-level** `Server` — no
private-attr access.

## Review provenance

This branch already went through a 7-reviewer + devil's-advocate review
pass (59 confirmed findings) followed by a fix pass with adversarial
re-verification: all 55 code findings are fixed here with tests
(silently-wrong detector shapes, refuse-loudly gaps, an unbounded token
cache — the token is now verified on every call with no cache, and
expiry was later cut as consumer-less scope — event-loop blocking for
sync handlers, provenance-preserving verify normalization), and a
docstring-accuracy audit (77 reviewed, 3 corrected). The remaining
follow-ups are listed under "For discussion".

## Verified

- **Reproducible from this PR:** `pytest
tests/unit_tests/test_mcp_auto_exposure.py` → **45/45** (~3 s; covers
every refusal, parity shape, token path, exclusion semantics, and the
normalization gating/provenance — including a re-registered-`/verify`
regression test).
- **Live e2e at this HEAD (`3891dd65`):** `workplace_assistant` MCP door
(`claude_code_agent`) — **5/5 `reward: 1.0`**, all 8 distinct tools
namespaced; HTTP door (`simple_agent`) — **5/5** on the second run, 4/5
on the first (the failing rollout omitted the `status: "Lead"` search
filter — a trajectory-verified policy flake, not an engine failure; the
HTTP door runs the stock route code). `example_mcp_weather`
(`claude_code_agent`) — **5/5 `reward: 1.0`** at the removal commit
`4d6d569c`; the one commit since is docs plus a comment reword. Earlier
demo (below): both doors 5/5 tasks `reward: 1.0`, ~30 s wall for 5
concurrent rollouts, MCP-door trajectories persist the namespaced names
the model emitted. n=5 single runs of a stochastic policy — a smoke
demonstration that both transports work end to end, not a statistical
parity claim.
- **Development-time only (local, gitignored — not reproducible from
this PR):** a 44-check live acceptance harness exercising six
representative in-tree servers (finance, workplace, aviary,
newton_bench, openenv, ns_tools) against pristine `origin/main`
handlers, covering the factory-`__signature__`, `dict`-body, and
raw-body/`PlainTextResponse` shapes; 44/44 as of 2026-07-17 against a
pre-refactor revision of the engine (development-time evidence; the
harness predates the `mcp_tools()` API). Other in-tree servers were
covered by a static route sweep, not this live harness.

## Runnable e2e — both doors through `gym eval run`

The full 5-task `workplace_assistant` example dataset, driven end to end
by Gym. Because `resources_servers/` ships unchanged, the demo uses a
small user-side entrypoint (`app_mcp.py`, shown in the docs added by
this PR) that subclasses the stock server and overrides `mcp_tools()` to
append the 27 catch-all-backed tools; the flag itself is plain yaml:

Compose config (`workplace_2059.yaml`) — MCP door via
`claude_code_agent` (already on `main`; it reads the `mcp` key
`/seed_session` returns and wires the Claude Code binary itself), HTTP
door via `simple_agent`:

```yaml
workplace_assistant:
  resources_servers:
    workplace_assistant:
      entrypoint: app_mcp.py   # thin subclass overriding mcp_tools(); see the docs page
      domain: agent
      expose_tools_over_mcp: true

workplace_claude:
  responses_api_agents:
    claude_code_agent:
      entrypoint: app.py
      resources_server: {type: resources_servers, name: workplace_assistant}
      model: ${anthropic_model_name}
      anthropic_api_key: ${anthropic_api_key}      # env.yaml; any Anthropic-format endpoint
      anthropic_base_url: ${anthropic_base_url}    # optional gateway; CLI appends /v1/messages

policy_model:
  responses_api_models:
    inference_provider:
      entrypoint: app.py
      base_url: ${policy_base_url}                 # any OpenAI-format endpoint
      api_key: ${policy_api_key}
      model: ${policy_model_name}

workplace_simple:
  responses_api_agents:
    simple_agent:
      entrypoint: app.py
      max_steps: 12
      resources_server: {type: resources_servers, name: workplace_assistant}
      model_server: {type: responses_api_models, name: policy_model}
```

```bash
gym env start --config workplace_2059.yaml

# (a) MCP door — Claude Code discovers and calls the tools over /mcp with the per-rollout token
gym eval run --no-serve --agent workplace_claude \
  --input resources_servers/workplace_assistant/data/example.jsonl \
  --output results/workplace_claude_rollouts.jsonl

# (b) HTTP door — the same tools as plain POST /<tool_name>
gym eval run --no-serve --agent workplace_simple \
  --input resources_servers/workplace_assistant/data/example.jsonl \
  --output results/workplace_simple_rollouts.jsonl
```

Tip: if your provider's OpenAI *Responses-API* emulation is incomplete
(a common gateway gap on multi-turn tool exchanges), use
`inference_provider` (chat-completions + Gym's converter) rather than
`openai_model`.

## Enablement (deliberately none in this PR)

This PR ships the engine plus the one in-tree example
(`example_mcp_weather`) — every other server under `resources_servers/`
is byte-identical to `main` (we cannot properly test each env here, per
review). Enabling a server is per-server follow-up work: set
`expose_tools_over_mcp: true` in its instance config (typed-route
servers need no code at all), plus a small `mcp_tools()` override for
catch-all dispatcher servers — the docs added in this PR walk through
workplace_assistant end to end. During development, a fleet exercise ran
`claude_code_agent` over `/mcp` against 12 servers enabled this way
(workplace_assistant 27 tools, math_advanced_calculations,
finance_sec_search, indirect_prompt_injection with per-task narrowing,
the examples, and others), all dispatching correctly; those results are
development-time evidence, not shipped code.

**Deliberately not exposed even as follow-up (each needs design work
first):** aviary (its `/step`/`/close` bodies carry an `env_id` that can
address other rollouts' environments — needs session-derived lookup),
genrm_compare (its only route is a harness-side batch API), and the
gymnasium family (reward accrues from `/reset`/`/step`, which a model
could drive to reroll its own episode). The 74 verify-only benchmarks
have no tool routes to expose.

## For discussion
- Flag is **opt-in (default off)** — auto-exposing every route isn't
always wanted (aviary's `/step` carries `env_id`; exposing it lets a
model address other rollouts' envs).
- Follow-ups deliberately not in this PR: session-binding aviary's
`env_id` so it can be exposed safely; an MCP-safe reward design for the
gymnasium family; and hardening the pre-existing token-signing secret
derivation shared with NVIDIA-NeMo#1682.
- **NVIDIA-NeMo#1682's decorator API is removed in this PR** rather than left as a
parallel mechanism: auto-exposure reads the routes authors already
wrote, so `@gym_tool` had no remaining role, and keeping both would have
forced every author to choose. `example_mcp_weather` (the only in-tree
consumer) is migrated here; NVIDIA-NeMo#2002's dual-registration approach is
superseded.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Signed-off-by: Codex <adasif@nvidia.com>
Signed-off-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: make existing resources servers MCP-compatible

3 participants