Fix MCP session lifecycle and replay safety - #2134
Conversation
ApprovabilityVerdict: Unable to determine Macroscope's correctness review was unable to post its findings for this PR. Approvability cannot proceed without a successful correctness review. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67e0acc36c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fffc408 to
18109b8
Compare
mikasenghaas
left a comment
There was a problem hiding this comment.
do we have some data/ experiments to backup the robustness improvs around mcp?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9789a1d4b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc39d94a2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fc39d94 to
d9c602c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d9c602c. Configure here.
| class MCPTransportError(HarnessError): | ||
| """A harness-owned MCP session lost its transport; the message is structured JSON.""" | ||
|
|
||
|
|
||
| class MCPDeliveryUnknownError(MCPTransportError): | ||
| """An MCP transport failed after a non-replayable operation may have been accepted.""" |
There was a problem hiding this comment.
lets not try to reinvent finegrained error cls and lets just have the error that is raised in the harness bubble up. we just wrap with harness error to have a rough idea of the "location" of the error
| if ( | ||
| isinstance(payload, dict) | ||
| and payload.get("delivery") | ||
| in {"operation_not_started", "response_unknown"} | ||
| and isinstance(payload.get("replay_safe"), bool) | ||
| ): | ||
| error_cls = ( | ||
| MCPDeliveryUnknownError | ||
| if payload["delivery"] == "response_unknown" | ||
| and not payload["replay_safe"] | ||
| else MCPTransportError | ||
| ) | ||
| mcp_error = error_cls(diagnostic) | ||
| if isinstance(mcp_error, MCPDeliveryUnknownError): | ||
| raise mcp_error |
There was a problem hiding this comment.
then we can also remove those shenannigans
| m.name | ||
| for m in pkgutil.iter_modules(harnesses.__path__) | ||
| if m.ispkg | ||
| and importlib.util.find_spec(f"{harnesses.__name__}.{m.name}.harness") |
There was a problem hiding this comment.
bit ugly to break with our one folder = one harness convention but ig this is the lesser evil than duplicated code?
| its trace is retryable (all captures count, not just the most recent).""" | ||
| its trace is retryable (all captures count, not just the most recent). | ||
| A possibly-delivered MCP mutation vetoes replay unless explicitly included.""" | ||
| replay_unknown = ( |
There was a problem hiding this comment.
hmmm im not sure im loving this. in between who and who is this error raised? if its between harness + tool server we should fix in the harness and not patch here. retrying is really last resort in my mind
b20f3fb to
53b52ed
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |

Overview
Centralize null and bash harness MCP handling into one lifecycle-aware client and make transport failures explicit, replay-safe, and diagnosable.
Details
Note
Medium Risk
MCP delivery-unknown handling and restricted-runtime reuse change failure and isolation behavior on eval retries and multi-agent Docker; NeMo Gym adds a large optional dependency tree (Ray, wandb) for Python 3.12+.
Overview
Adds NeMo Gym v1 support: a
NeMoGymTasksetthat seeds/verifies against Gym resource servers while verifiers harnesses run the agent loop, optionalNeMoGymEnvto start packaged servers, averifiers[nemo-gym]extra, docs, and thenemo-gym-weather-v1example.MCP harness programs are merged into one shared
minimal/program.py(bash gated by--bash). Transport runs in a dedicated task; failures emitVF_MCP_ERROR=JSON that the parent maps toMCPTransportErrororMCPDeliveryUnknownError. Retries skip replay when delivery is unknown unlessRetryConfigexplicitly allowsMCPDeliveryUnknownError.Restricted runtimes can be reused across rollouts:
runtime.rollout()replacesprepare_setup, kills stray processes, resets caches, and toggles network policy via_apply_network_policy. Rollouts enter that context; agentic-judge shared Docker uses a blocklist in e2e.Also exports
mcp_sessionon the launch path, tightensbuiltin_harness_idsto packages with aharnessmodule, and propagatesnetwork_allow/network_blockinto judge tasks.Reviewed by Cursor Bugbot for commit 53b52ed. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix MCP session lifecycle and replay safety in harness program
VF_MCP_ERROR=markers for the parent harness to parse.MCPTransportErrorandMCPDeliveryUnknownError— so callers can distinguish clean transport failures from cases where a non-replayable mutation may have already been accepted by the server.MCPDeliveryUnknownErroris present, unless explicitly opted into via retry config.execution_preparedtracking and arollout()context manager to the runtime base class, enabling Docker and Prime runtimes to toggle between unrestricted trusted-setup policy and restricted execution network policy per rollout.Macroscope summarized 53b52ed.