Fix MCP session lifecycle and replay safety - #2134
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review Unable to check for correctness in 552f444. This PR introduces substantial new MCP session lifecycle management code (~300 lines) with complex async cancellation handling, new public error types, and changes to retry behavior that gate when operations can be replayed. The scope and complexity warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 18109b8. Configure here.
mikasenghaas
left a comment
mikasenghaas
left a comment
There was a problem hiding this comment.
do we have some data/ experiments to backup the robustness improvs around mcp?
| MCP_CALL_ATTEMPTS = 6 | ||
| MCP_TIMEOUT = httpx.Timeout(600.0, connect=5.0) # the OpenAI SDK client defaults | ||
|
|
||
| # <vf:mcp-client> |
| parser.add_argument("--prompt", default="") | ||
| parser.add_argument("--initial-messages-file", default="") | ||
| parser.add_argument("--mcp-config", default="") | ||
| parser.add_argument("--bash", action="store_true") |
There was a problem hiding this comment.
ohh i see, i can get behind it. but maybe we should mark shared utils in the filetree a bit clearer. myb:
v1.harnesses.minimal.mcp-> mcp client stuffv1.harnesses.minimal.program-> shared program (imported by bsah + null)
| transient = ( | ||
| isinstance( | ||
| leaf, | ||
| ( | ||
| asyncio.CancelledError, | ||
| BrokenResourceError, | ||
| ClosedResourceError, | ||
| EndOfStream, | ||
| httpx.TransportError, | ||
| OSError, | ||
| TimeoutError, | ||
| ), | ||
| ) | ||
| or ( | ||
| isinstance(leaf, McpError) | ||
| and leaf.error.code == CONNECTION_CLOSED | ||
| and leaf.error.message == "Connection closed" | ||
| ) | ||
| or status in (408, 429) | ||
| or (status is not None and status >= 500) | ||
| ) |
2 participants
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
Changes MCP failure semantics and default retry behavior (delivery-unknown failures no longer replay unless opted in), and consolidates null/bash agent programs—important for harness rollouts but not auth or payment paths.
Overview
Centralizes MCP handling for null and bash harnesses into a shared embedded
mcp_client.py, replacing duplicated inline logic and deleting the null-only program.The new client runs the MCP transport in a dedicated task (so SDK cancellation scopes stay isolated), classifies transport loss as transient vs not and replay-safe vs not, and emits
VF_MCP_ERROR=JSON diagnostics.Harness.runparses those lines and raisesMCPTransportErrororMCPDeliveryUnknownErrorinstead of a genericHarnessError. In-process retries only fire when failures are markedsession_retry_safe;call_toolusesreplay_safe=Falseso a lost response is not automatically replayed.Null now runs the same bash
program.pywithout--bash(MCP-only chat, 60s enumeration timeout); bash passes--bashfor local tools and unbounded MCP setup.trace_should_retry/episode_should_retryblock whole-rollout replay onMCPDeliveryUnknownErrorunlessRetryConfigexplicitly includes that type.Reviewed by Cursor Bugbot for commit 552f444. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix MCP session lifecycle and add replay-safe retry policy for MCP transport failures
MCPTransportFailureexception with JSON metadata (delivery status, replay safety, operation phase) on failure.MCPTransportErrorandMCPDeliveryUnknownErrorin errors.py; the harness parsesVF_MCP_ERROR=output from the program and raises the appropriate error.with_retrythat retries transient transport failures only when the operation is replay-safe (e.g.list_toolsbut notcall_tool).# <vf:mcp-client>marker; null harness drops its own program.py.trace_should_retryin retries.py to veto episode retries whenMCPDeliveryUnknownErroris present unless the retry policy explicitly opts in.Macroscope summarized 552f444.