Repository navigation
fix: preserve structured MCP tool identity for verification - #2673
cwing-nvidia wants to merge 10 commits into
Conversation
Carry adapter-observed MCP provenance separately from model-facing aliases so verification remains correct across harnesses and external servers. Signed-off-by: Chris Wing <cwing@nvidia.com>
This comment has been minimized.
This comment has been minimized.
Real rollout and reverification evidenceWhat was testedI used one deterministic Workplace Assistant email-reply task with The simple HTTP path establishes the expected verifier result without MCP. Codex and Claude Code exercise the new structured-provenance path through Gym's rollout-specific MCP server. Results
Claude Code still emits a flattened Stored artifact checksSelected fields from the Codex rollout: {
"response": {
"output": [
{
"type": "function_call",
"call_id": "item_3",
"name": "email_reply_email"
}
]
},
"mcp_tool_call_provenance": {
"item_3": {
"server_name": "workplace_assistant",
"tool_name": "email_reply_email"
}
},
"reward": 1.0
}Selected fields from the Claude Code rollout: {
"response": {
"output": [
{
"type": "function_call",
"call_id": "chatcmpl-tool-949109ad5d825174",
"name": "mcp__workplace_assistant__email_reply_email"
}
]
},
"mcp_tool_call_provenance": {
"chatcmpl-tool-949109ad5d825174": {
"server_name": "workplace_assistant",
"tool_name": "email_reply_email"
}
},
"reward": 1.0
}Controlled reverificationTo isolate the sidecar from every supported naming convention, I copied the stored rollouts and changed only
Scope and observations
Reproduction filesSave the following four files at the repository root.
|
Advertise the authoritative Gym MCP tool list so new Claude trajectories use structured provenance while historical artifacts retain the legacy fallback. Signed-off-by: Chris Wing <cwing@nvidia.com>
Keep the structured provenance PR limited to harnesses that can run Gym MCP end to end, and move OpenClaw integration behind issue #2670. Signed-off-by: Chris Wing <cwing@nvidia.com>
Document the rollout sidecar, legacy Claude fallback, and reverification behavior so the MCP tutorial matches the new verification contract. Signed-off-by: Chris Wing <cwing@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-fix-structured-mcp-tool-provenance.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
Remove the post-release MCP provenance entry so the published release notes continue to describe only shipped v0.5.0 behavior. Signed-off-by: Chris Wing <cwing@nvidia.com>
|
/claude review |
|
SHIP — structured MCP provenance is a net correctness improvement over string-parsing, and the scoring path is carefully guarded and well-tested. Traced the paths that can silently corrupt reward:
NOTE (author's call): The Claude path reconstructs provenance by exact-matching emitted names against No blockers. |
cmunley1
left a comment
There was a problem hiding this comment.
shared some comments offline
Avoid converting incomplete metadata into authoritative empty sidecars and strip caller-provided provenance before verification. Signed-off-by: Chris Wing <cwing@nvidia.com>
Preserve emitted response artifacts and sidecars for reverification while falling back safely for duplicate call IDs. Signed-off-by: Chris Wing <cwing@nvidia.com>
Reject forged tool identities unless they match the session's MCP dispatch ledger, then sign accepted claims for safe reverification. Signed-off-by: Chris Wing <cwing@nvidia.com>
|
Could you clarify whether Could this metadata instead remain in a Gym-owned verification envelope or another layer outside the common verifier models, while still being persisted and forwarded by My understanding is that there are two opposite transformations:
The flattened tool name enters NeMoGymResponse, because Claude Code exposes MCP tools to its model using: The place where NeMo-Gym normalizes it back is in the server.normalize_tool_name() eventually runs: |
| rollout_id: Optional[str] = None, | ||
| observation_collector: Optional[Callable[[Path, dict[str, Any]], None]] = None, | ||
| ) -> NeMoGymResponse: | ||
| ) -> AgentExecutionResult: |
There was a problem hiding this comment.
Thinking out loud, since this is somehow a contract change.
Could _create_response() remain responsible only for producing the standard NeMoGymResponse, with run() deriving and forwarding mcp_tool_call_provenance afterward?
That seems closer to the separation in simple_agent: responses() produces the Responses API object, while run() owns rollout orchestration (/seed_session, verification, and Gym-specific metadata). I understand why Claude's run() cannot simply self-call the public responses() endpoint—the temporary rollout-specific MCP config is an internal parameter—but the provenance join can still happen in run() after _create_response() returns. That would avoid changing this execution helper's return contract solely for verification metadata and keep responses() from immediately discarding part of AgentExecutionResult.
There was a problem hiding this comment.
The core problem as I understand it is that an MCP tool call currently has two representations: the harness/model-facing name (for example, Claude Code's mcp__example_mcp_weather__get_weather) and the canonical MCP identity (server_name=example_mcp_weather, tool_name=get_weather). The emitted name is useful for faithful replay, observability, and training, while the canonical identity is what the environment understands.
A Resources Server's verify() should remain generic and agent-agnostic: it should consume its own canonical tool vocabulary without needing to know Claude, Codex, Hermes, or OpenClaw naming and sanitization conventions. The agent adapter is the component that understands how its harness projected (server, tool) into the model-facing namespace, so that mapping needs to be established before the environment verifier runs.
The PR's structured-provenance direction proposes one way to addresses that boundary. I think it is worth understanding design-wise where is this issue coming from and the data contracts involved. @ananthsub, @marta-sd, @adil-a
A Resources Server’s verify() should understand its own domain vocabulary:
get_weather
email_reply_email
append
It should not need to understand every agent harness’s presentation rules:
Claude: mcp__example_mcp_weather__get_weather
Hermes: mcp_example_mcp_weather_get_weather
OpenClaw: example_mcp_weather__get_weather
Codex: get_weather
Otherwise every verifier becomes coupled to:
- Which agent produced the rollout
- That agent’s version
- Its sanitization and truncation rules
- Future naming changes
- External MCP server configurations
The desired boundary by this PR is something like
Agent adapter
understands harness-specific names
↓
Gym verification boundary
maps them to canonical MCP identity
↓
verify()
sees only environment-level tool names
Notice that it is an architectural choice here to
Keep the standard Responses function_call shape and its emitted name unchanged.
The rationale is that NeMoGymResponse represents what the model actually produced, which matters for:
- Exact replay
- Training-token fidelity
- Debugging and observability
- Distinguishing calls as presented by different harnesses
I think the friction here is that fundamentally Gym cannot preserve the raw alias and later recover an ambiguous MCP identity without storing extra information somewhere. The real choice is:
- Canonicalize NeMoGymResponse and store raw evidence elsewhere, or
- Keep NeMoGymResponse raw and introduce a first-class canonical trajectory.
Alternatively, maybe something worth considering @cwing-nvidia is if the verify method really needs to depend on the tool_call name (and replay of the trajectory) or if we could enforce it to depend on state of the environment.
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class AgentExecutionResult: |
There was a problem hiding this comment.
agentexecutionresult feels overly general for mcp only path
cmunley1
left a comment
There was a problem hiding this comment.
- The in-memory execution_ledger has no eviction. Sessions that call MCP tools but never reach /verify leave entries forever.
- Provenance validation compares claimed and executed calls as ordered lists. Parallel MCP calls may arrive in a different order than the trajectory, causing valid provenance to be rejected. It needs an order-independent comparison or an explicit stable call identifier.
## What does this PR do? Adds a bounded ATIF v1.7 input path to `gym eval reverify`, allowing completed Relay trajectories to be scored by Gym's existing stateless verifier flow without rerunning the agent or policy model. Together, NVIDIA-NeMo#2737 and NVIDIA-NeMo#2767 establish a file-based interoperability path between NeMo Relay and NeMo Gym. Relay-exported ATIF trajectories can be evaluated in Gym, while Gym-native trajectories can be exported to the same ATIF format for downstream consumers. This does not couple the Relay and Gym runtimes or add Relay as a Gym dependency. Relates to NVIDIA-NeMo#2468. ## Before `gym eval reverify` only accepted native Gym rollout JSONL. ATIF contains the external agent trajectory, but not the Gym task, expected answer, verifier configuration, or resources-server routing needed to calculate a reward. ## After ```bash gym eval reverify \ --input-format atif \ --config my_resources_server.yaml \ --inputs materialized-inputs.jsonl \ --atif-manifest relay-trajectories.jsonl \ --output rewards.jsonl ``` The inputs have separate responsibilities: - Materialized Gym inputs provide the task, verifier metadata, request context, and routing. - ATIF provides the completed external-agent output. - The manifest explicitly joins each trajectory to `(_ng_task_index, _ng_rollout_index)` and may pin its SHA-256. Gym projects the supported ATIF output into its existing Responses-shaped verifier payload, calls the normal stateless `/verify` path, and computes aggregate metrics. ## Integration contract Gym consumes the canonical ATIF fields needed to build that bounded scoring payload. It does not reconstruct provider-native payloads stored in optional `extra` metadata; the ATIF producer owns that conversion. `projection_status=complete` means Gym constructed the supported verifier payload without guessing or dropping a field the adapter maps. It does not claim full-document ATIF fidelity, certify the producer's provider-to-ATIF conversion, or interpret arbitrary producer metadata. Persisted source and trajectory hashes retain that provenance. Supported in this initial path: - exact ATIF v1.7 schema and text-only trajectories; - canonical agent text and `reasoning_content`; - generic function calls with exactly one correlated result per call; - standard token usage and supported Relay usage details; - explicit one-to-one trajectory/task joins with optional source-hash verification; and - persisted trajectory ID, session ID, schema version, source hash, and projection status. Standard usage is strict. If any model step reports supported usage, every model-backed step must provide `prompt_tokens` and `completion_tokens`, and `final_metrics` must provide matching totals. Optional cached, reasoning, and reported-total details are projected only when their aggregate can be proven. Partially reported optional details remain unknown rather than being counted as zero. Equivalent Relay reasoning-token aliases are normalized, conflicting aliases are rejected, and cost metadata remains provenance rather than verifier input. The path fails closed on shapes that cannot be mapped without changing what the verifier scores, including: - multimodal, continued, copied-context, or subagent trajectories; - later user or system input after agent output; - aggregated LLM calls or ambiguous message/tool ordering; - incomplete, duplicated, or uncorrelated tool calls and results; and - training token IDs or log probabilities. Optional producer-specific status or provider payloads in `extra` do not change the canonical projection. A trajectory may end after a tool call when every call has a correlated observation result. Tool-bearing ATIF is rejected when routed to a resources server with `expose_tools_over_mcp: true`. ATIF proves generic call/result correlation, but does not carry Gym's canonical MCP `(server_name, tool_name)` identity. ATIF mode requires the verifiers selected by the batch to be stateless and does not support `--force`, `--resume`, `--judge-failed-only`, or `--append`. Native Gym reverification remains the default. This adds no Relay, NeMo Agent Toolkit, or Harbor runtime dependency. ## Suggested review order Most of the added lines are qualification tests and fixtures. The maintainer-facing behavior is concentrated in these areas: | Review area | Files | What to verify | | --- | --- | --- | | Strict ATIF boundary | `nemo_gym/atif_v1_7.py`, `nemo_gym/atif_json.py` | Exact v1.7/schema and lossless JSON rules | | Projection | `nemo_gym/atif_reverification.py` | Supported trajectory mapping, usage semantics, identity, and fail-closed boundaries | | Gym integration | `nemo_gym/rollout_reverification.py` | Selected-route preflight and reuse of the existing stateless verifier path | | Wiring and evidence | CLI, docs, fixtures, and tests | User-facing contract and regression coverage | Suggested pass: review the boundary first, then projection, then the small Gym integration. The remaining files document and prove that contract. ## Validation - Focused ATIF projection, rollout-reverification, and CLI suites: **491 passed**. - Changed-file pre-commit and `git diff --check`: passed. - Fixture tests prove reward equivalence between native and projected responses and show that the same ATIF trajectory can be rescored with a second stateless verifier. - The live Codex CLI → Relay → ATIF → Gym check executed a real tool call, preserved `function_call → function_call_output → message`, and returned reward `1.0` with aggregate accuracy `100%`. The broader local unit run reported the same five failures in both ATIF worktrees, all outside the affected paths: sandbox port binding, existing `aime24` metadata, macOS `/proc` behavior, system Python missing `aiohttp`, and sandbox semaphore permissions. The affected suites pass. ## Related work - NVIDIA-NeMo#2468 tracks ATIF input for Gym reverification. - NVIDIA/NeMo-Relay#887 fixes Responses tool-call correlation. - NVIDIA-NeMo#2673 covers Gym-native structured MCP tool identity and is complementary to this PR. ## Checklist - [x] I have read the contributing guidelines. - [x] The change is focused and adds no unrelated runtime dependency. - [x] Tests and documentation were added. - [x] Changed-file pre-commit checks pass. - [x] Commits include DCO sign-off. --------- Signed-off-by: Alex Fournier <afournier@nvidia.com>
Summary
This PR makes MCP-backed rollouts score consistently across agent harnesses while preventing an untrusted verification request from forging a canonical MCP tool identity.
Details
mcp_tool_call_provenancesidecar keyed by Responses APIcall_id, while preserving each harness's emitted tool name in the stored trajectorymcp__<server>__<tool>fallback for historical trajectoriesNone) from authoritative empty provenance ({}), reject malformed seeded tool metadata, and strip caller-provided sidecars before agent adapters build verification requestsnemo_gym/mcp.py/seed_sessionCloses #2668
Test plan
pytest tests/unit_tests/test_mcp.py tests/unit_tests/test_mcp_auto_exposure.py responses_api_agents/claude_code_agent/tests/test_app.py responses_api_agents/codex_agent/tests/test_app.py -q— 172 passedpre-commit run --files ...Scope
Codex and Claude Code are validated end to end here. OpenClaw and Hermes consume the shared contract in stacked PRs #2733 and #2778; their native handoff and event parsing remain adapter-specific.