docs(mcp): add tutorial + tighten lifecycle guide - #602
Conversation
Adds a hands-on MCP tutorial (`docs/source/tutorials/mcp-environment.md`) covering the two sides of the OpenEnv MCP contract: consuming an MCP environment from a simulation loop (ListToolsAction / CallToolAction via step(), error handling, step vs call_tool trade-off) and building one by subclassing MCPEnvironment + FastMCP tool registration. The walkthrough uses `envs/echo_env/server/echo_environment.py` as the worked example and points readers at `examples/echo_mcp_demo.py` for an end-to-end run. The tutorial is built to complement, not duplicate, the existing `docs/source/mcp-environment-lifecycle.md` FAQ-style guide: the tutorial teaches how to do it, the lifecycle guide explains when/why. Every code sample was executed against real source before shipping. That process surfaced three inaccuracies which are fixed here: - `env.step(CallToolAction(...)).result` is a `CallToolResult` wrapper, not the raw return value. Tutorial uses `obs.result.data` and explains the wrapper (`.data`, `.content`, `.structured_content`). - `obs.error.error_type` is a `ToolErrorType` enum, not a string. - `MCPEnvironment._step_impl` is abstract; a subclass cannot be instantiated without it. The tutorial's echo example includes it and the prose now says "`_step_impl` is required, `step` is not". Also tightens two gaps in the lifecycle guide surfaced while writing the tutorial and verified against `MCPToolClient.call_tool`: - `call_tool()` is async and must be awaited (not previously stated). - `call_tool()` returns the unwrapped tool value; `step(CallToolAction( ...)).result` returns a `CallToolResult` wrapper. The "Which Pattern Should You Use?" section now calls out both shapes explicitly. Adds the tutorial to `docs/source/tutorials/index.md` toctree. Verified: all four tutorial snippets execute end-to-end against EchoEnvironment; sphinx-build passes with zero warnings on the new or edited pages; internal links resolve (`#which-pattern-should-you-use` anchor in lifecycle guide confirmed in rendered HTML). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second pass on the tutorial, driven by the "is this actually useful for an ML engineer starting with OpenEnv?" lens: - Opening "Why MCP?" now leads with the question every reader will ask — "if my tools are just local Python functions, do I even need MCP?" — and answers it honestly (process boundary, env reuse, external agents, schema discovery). The dual-API-boundary explanation becomes a sub-section of "Why", not the top of it. - "Using MCP Tools in a Training Loop" is restructured so the framework-agnostic rollout loop is the prominent path. TRL's environment_factory is a sub-section clearly labelled as a TRL API (not an MCP API) — with an explicit note that environment_factory and MCP are orthogonal and compose, but neither implies the other (Wordle uses environment_factory without MCP; MCP envs work fine without environment_factory). - New "Using MCP Tools for Evaluation" section, parallel to the "Using Rubrics for Evaluation" section in the Rubrics tutorial: plain-Python offline loop over a static dataset, linking to the Reward Design guide for scoring, honest about the in-flight state of src/openenv/core/evals/. - New note on MCP adoption in OpenEnv: RFC 003 is aspirational and still In Review; today only echo_env + finqa_env inherit from the canonical openenv.core.env_server.mcp_environment.MCPEnvironment (calendar_env uses a local wrapper with the same shape). The other ~27 envs (textarena / openspiel / chess / browsergym / ...) still use custom action types without MCP plumbing. Readers should check an env's inheritance before assuming the patterns here apply. Also fixes four issues caught in a pre-push review pass: - Broken internal link to rubrics.md (file lives in an unmerged PR, not on main) - replaced with the Reward Design guide, which does exist on main. - Framework-agnostic rollout snippet referenced env.last_observation, which is not an attribute on the Environment base class (see src/openenv/core/env_server/interfaces.py). Rewrote the snippet to use the obs variable returned from env.reset() / previous env.step(), which is the actual idiom. - Tightened the adoption note to not overstate calendar_env's MCP inheritance (it uses a local wrapper, not the canonical base). - Softened "Swap TextArenaAction for CallToolAction" in Next Steps to call out that the swap is structural (single-field vs tool_name + arguments), not cosmetic. Every code sample was re-executed against the real EchoEnvironment before commit. Build is clean: sphinx-build produces zero warnings attributable to this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
meta-cla
Bot
added
the
CLA Signed
Greptile SummaryThis docs-only PR adds a hands-on MCP tutorial (
Confidence Score: 4/5Safe to merge after fixing the tool-error propagation inaccuracy on line 125; the lifecycle guide changes and tutorial structure are otherwise accurate. One P1 finding: the tutorial incorrectly tells readers that tool execution exceptions come back in obs.result, when the source places them in obs.error as EXECUTION_ERROR. A reader following this guidance will silently miss tool failures in their training/eval loops. All other findings are P2. docs/source/tutorials/mcp-environment.md — specifically line 125 and the call_tool() description on line 143. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[env.step CallToolAction] --> B{Tool exists?}
B -- No --> C[obs.error = TOOL_NOT_FOUND\nobs.result = None]
B -- Yes --> D{Arguments valid?}
D -- No --> E[obs.error = INVALID_ARGS\nobs.result = None]
D -- Yes --> F{Tool executes}
F -- raises Exception --> G[obs.error = EXECUTION_ERROR\nobs.result = None]
F -- success --> H[obs.result = CallToolResult\n.data .content .structured_content\nobs.error = None]
subgraph call_tool path
I[await env.call_tool] --> J[calls step internally]
J --> K{obs.error set?}
K -- Yes --> L[raises RuntimeError]
K -- No --> M[returns obs.result.data]
end
style C fill:#f88,color:#000
style E fill:#f88,color:#000
style G fill:#f88,color:#000
style H fill:#8f8,color:#000
style L fill:#f88,color:#000
style M fill:#8f8,color:#000
Prompt To Fix All With AIThis is a comment left during a code review.
Path: docs/source/tutorials/mcp-environment.md
Line: 125
Comment:
**Tool execution exceptions go to `obs.error`, not `obs.result`**
The tutorial states: *"Tool-specific failures — a business-logic error that the tool itself raised — come back inside `result`."* This is factually incorrect and will cause readers to write code that silently misses tool execution failures.
In `src/openenv/core/env_server/mcp_environment.py` (lines 543-551), when a tool function raises an exception it is caught and placed in `obs.error` with `error_type=ToolErrorType.EXECUTION_ERROR` and `result=None` — the same `obs.error` field used for transport errors and unknown-tool errors:
```python
except Exception as e:
return CallToolObservation(
tool_name=tool_name,
result=None,
error=ToolError(
error_type=ToolErrorType.EXECUTION_ERROR,
message=str(e),
),
)
```
The `ToolErrorType` docstring confirms: `EXECUTION_ERROR = "execution_error" # Tool ran but failed`. A reader who relies on the tutorial's claim will write code that never checks `obs.error` for `EXECUTION_ERROR`, silently treating failed tool calls as successful ones.
All five `ToolErrorType` variants flow through `obs.error`; `obs.result` carries the `CallToolResult` wrapper only on the success path.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: docs/source/tutorials/mcp-environment.md
Line: 143
Comment:
**`call_tool()` raises `RuntimeError` on any tool error — not a drop-in swap for `step()`**
The tutorial describes `call_tool()` as "functionally equivalent in simulation mode" without noting that its error handling diverges significantly. In `src/openenv/core/mcp_client.py` (lines 430-434), when `obs.error` is set, `call_tool()` raises `RuntimeError` rather than returning the observation:
```python
if isinstance(obs, CallToolObservation) and obs.error is not None:
raise RuntimeError(
f"Tool '{name}' failed: {obs.error.message} "
f"(type: {obs.error.error_type.value})"
)
```
Code ported from `step()` that inspects `obs.error` for graceful error handling needs a `try/except RuntimeError` when switched to `call_tool()`. Consider adding a brief note that `call_tool()` converts all tool errors into exceptions.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs(mcp): reframe training section + ad..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| print(obs.error) # None | ||
| ``` | ||
|
|
||
| `obs.result` is a `CallToolResult` wrapper exposing the return value in a few shapes: `.data` is the raw Python value the tool returned, `.structured_content` is its JSON-encoded form, and `.content` is the MCP protocol's list of typed content blocks (useful when a tool returns rich multi-part output). `obs.error` is set only when the **framework** could not deliver the call (transport failure, unknown tool name, malformed arguments). Tool-specific failures — a business-logic error that the tool itself raised — come back inside `result`, so callers can handle them like any domain-specific response. |
There was a problem hiding this comment.
obs.error, not obs.result
The tutorial states: "Tool-specific failures — a business-logic error that the tool itself raised — come back inside result." This is factually incorrect and will cause readers to write code that silently misses tool execution failures.
In src/openenv/core/env_server/mcp_environment.py (lines 543-551), when a tool function raises an exception it is caught and placed in obs.error with error_type=ToolErrorType.EXECUTION_ERROR and result=None — the same obs.error field used for transport errors and unknown-tool errors:
except Exception as e:
return CallToolObservation(
tool_name=tool_name,
result=None,
error=ToolError(
error_type=ToolErrorType.EXECUTION_ERROR,
message=str(e),
),
)The ToolErrorType docstring confirms: EXECUTION_ERROR = "execution_error" # Tool ran but failed. A reader who relies on the tutorial's claim will write code that never checks obs.error for EXECUTION_ERROR, silently treating failed tool calls as successful ones.
All five ToolErrorType variants flow through obs.error; obs.result carries the CallToolResult wrapper only on the success path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/source/tutorials/mcp-environment.md
Line: 125
Comment:
**Tool execution exceptions go to `obs.error`, not `obs.result`**
The tutorial states: *"Tool-specific failures — a business-logic error that the tool itself raised — come back inside `result`."* This is factually incorrect and will cause readers to write code that silently misses tool execution failures.
In `src/openenv/core/env_server/mcp_environment.py` (lines 543-551), when a tool function raises an exception it is caught and placed in `obs.error` with `error_type=ToolErrorType.EXECUTION_ERROR` and `result=None` — the same `obs.error` field used for transport errors and unknown-tool errors:
```python
except Exception as e:
return CallToolObservation(
tool_name=tool_name,
result=None,
error=ToolError(
error_type=ToolErrorType.EXECUTION_ERROR,
message=str(e),
),
)
```
The `ToolErrorType` docstring confirms: `EXECUTION_ERROR = "execution_error" # Tool ran but failed`. A reader who relies on the tutorial's claim will write code that never checks `obs.error` for `EXECUTION_ERROR`, silently treating failed tool calls as successful ones.
All five `ToolErrorType` variants flow through `obs.error`; `obs.result` carries the `CallToolResult` wrapper only on the success path.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| ### `step(CallToolAction(...))` vs `call_tool()` | ||
|
|
||
| Environment clients that inherit from `MCPToolClient` (such as `EchoEnv` and `FinQAEnv`) expose a shorter **async** `await env.call_tool("name", arg=value)` helper. Functionally equivalent in simulation mode — it still goes through the step loop and still updates rewards, step counts, and trajectory state — but returns the tool's raw return value directly instead of a `CallToolObservation`. Use `step(CallToolAction(...))` when you need the whole observation (reward, done, metadata); reach for `call_tool()` in async scripts where the result is all you care about. The [lifecycle guide](../mcp-environment-lifecycle.md#which-pattern-should-you-use) covers the exact trade-offs. |
There was a problem hiding this comment.
call_tool() raises RuntimeError on any tool error — not a drop-in swap for step()
The tutorial describes call_tool() as "functionally equivalent in simulation mode" without noting that its error handling diverges significantly. In src/openenv/core/mcp_client.py (lines 430-434), when obs.error is set, call_tool() raises RuntimeError rather than returning the observation:
if isinstance(obs, CallToolObservation) and obs.error is not None:
raise RuntimeError(
f"Tool '{name}' failed: {obs.error.message} "
f"(type: {obs.error.error_type.value})"
)Code ported from step() that inspects obs.error for graceful error handling needs a try/except RuntimeError when switched to call_tool(). Consider adding a brief note that call_tool() converts all tool errors into exceptions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/source/tutorials/mcp-environment.md
Line: 143
Comment:
**`call_tool()` raises `RuntimeError` on any tool error — not a drop-in swap for `step()`**
The tutorial describes `call_tool()` as "functionally equivalent in simulation mode" without noting that its error handling diverges significantly. In `src/openenv/core/mcp_client.py` (lines 430-434), when `obs.error` is set, `call_tool()` raises `RuntimeError` rather than returning the observation:
```python
if isinstance(obs, CallToolObservation) and obs.error is not None:
raise RuntimeError(
f"Tool '{name}' failed: {obs.error.message} "
f"(type: {obs.error.error_type.value})"
)
```
Code ported from `step()` that inspects `obs.error` for graceful error handling needs a `try/except RuntimeError` when switched to `call_tool()`. Consider adding a brief note that `call_tool()` converts all tool errors into exceptions.
How can I resolve this? If you propose a fix, please make it concise.
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review
Automated Checks
- Lint: FAIL — pre-existing failures in
envs/carla_env/(not from this PR). - Debug code: FOUND — pre-existing
print()statements insrc/openenv/core/containers/(not from this PR).
Tier 1: Fixes Required
- Wrong docs path — files will not be built or served. The PR writes to
docs/source/tutorials/mcp-environment.mdanddocs/source/mcp-environment-lifecycle.md, but the live doc tree is rooted atdocs/(notdocs/source/). The active MkDocs config atdocs/mkdocs.ymlusesdocs_dir: .. These files will silently not be built on the deployed site. -
docs/mkdocs.ymlnav not updated. The new tutorial is added to a Sphinx-styletoctreeindocs/source/tutorials/index.md, but the live site uses MkDocs Material with anav:block indocs/mkdocs.yml. Add the tutorial there underTutorials:or it will be unreachable. - Sphinx
{note}directives incompatible with MkDocs Material. The tutorial uses MyST/Sphinx{note}fenced-block syntax, but the site usespymdownx.blocks.admonition(requires/// noteor!!! note). These blocks will render as raw fenced code. - Broken link:
../guides/rewards.md. Noguides/rewards.mdexists in the repo. The replacement link is broken too. -
obs.result.datais not a stable public API. The tutorial presentsobs.result.data(FastMCP's internalCallToolResult.data) as the canonical read path. ButCallToolObservation.resultis typedAnyinmcp_types.py, and the.dataunwrapping is an internal detail inmcp_client.py. Readers usingenv.step()directly (notcall_tool()) may get a raw value, a dict, or aCallToolResultdepending on FastMCP version. Prefercall_tool()in examples, or document thatobs.resultshape is implementation-dependent.
Tier 2: Alignment Discussion
None identified. The tutorial correctly describes the dual API boundary, accurately notes that reset/step/state are reserved tool names, and appropriately frames MCP adoption as partial. RFC 003 is correctly identified as In Review.
Summary
- 5 mechanical issues to fix (wrong doc path, missing nav entry, wrong directive syntax, broken link, fragile API claim).
- 0 alignment points for human review.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Review: docs(mcp): add tutorial + tighten lifecycle guide
Automated Checks
- Lint: PASS (no Python files changed; pre-existing carla_env lint failures are unrelated)
- Debug code: CLEAN
Tier 1: One Fix Required
docs/source/tutorials/mcp-environment.md — error-handling snippet hardcodes a version-dependent string
The error-handling example writes:
print(obs.error.message) # "Unknown tool: 'does_not_exist'"The TOOL_NOT_FOUND classification in src/openenv/core/env_server/mcp_environment.py (lines 553–574) is derived by substring-matching ("not found" or "unknown tool") on whatever exception FastMCP raises. The actual message string is FastMCP-version-dependent and is not under OpenEnv's control. Presenting it as a literal expected output will break if FastMCP changes its wording. Recommend softening to a descriptive comment:
print(obs.error.message) # human-readable message from FastMCP, e.g. "Unknown tool: 'does_not_exist'"Tier 2: Alignment Discussion
None identified. The PR correctly:
- Routes all agent actions through
step(CallToolAction(...)), never exposingreset/stateas MCP tools - Calls out the
RESERVED_TOOL_NAMESguard in the "Building" section - Describes
call_tool()as going through the step loop (preserving the orchestration boundary), not as a bypass - Accurately documents the
asyncnature ofMCPToolClient.call_tooland its unwrapped return value (verified againstsrc/openenv/core/mcp_client.pylines 294–340) - Accurately states RFC 003 adoption (only
echo_envandfinqa_envon the canonical base;calendar_envuses a local wrapper)
Summary
- 1 mechanical issue to fix (over-precise hardcoded error message string in a comment)
- 0 alignment points for human review
Otherwise the tutorial is technically accurate, well-structured, and a valuable addition. The lifecycle guide fixes (async call_tool, unwrapped return value shape) are correct per source.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS (failures are in
carla_env, pre-existing, unrelated to this PR) - Debug code: CLEAN (no debug artifacts in new files)
Tier 1: Fixes Required
-
Wrong docs root — files will be unreachable. The PR places all new files under
docs/source/tutorials/anddocs/source/mcp-environment-lifecycle.md, but the live MkDocs build hasdocs_dir: .pointing at/docs/(not/docs/source/). Thedocs/source/layout exists only in unmerged worktrees that are migrating to Sphinx. Onmain, the correct targets aredocs/tutorials/mcp-environment.md,docs/tutorials/index.md, anddocs/mcp-environment-lifecycle.md. None of the patched/created files exist onmain, so the Sphinx build claim in the PR description is against a different tree. -
Broken cross-reference:
../guides/rewards.md.docs/source/guides/rewards.mddoes not exist onmain(nor in any merged branch). The link in the eval section will 404 at build time. Replace with a link to a page that actually exists, or remove it. -
docs/source/tutorials/index.mdpatch target missing. The diff hunks againstdocs/source/tutorials/index.md, which does not exist onmain. The PR cannot apply cleanly.
Tier 2: Alignment Discussion
None identified. The tutorial correctly describes the dual API boundary (WebSocket for orchestration, MCP for agents), accurately states that reset()/step()/state() are reserved tool names, and does not expose simulation controls to agents. RFC 003 adoption caveat is honest and clearly scoped.
Summary
- 3 mechanical issues to fix (all stem from targeting the wrong docs tree)
- 0 alignment points for human review
Automated review by Claude Code | Learn more
Fixes three review issues on huggingface#602: - Tutorial claimed tool exceptions come back in `obs.result`, but per mcp_environment.py:543-551 they land in `obs.error` as EXECUTION_ERROR with `result=None`. A reader following the old text would silently miss tool failures. Rewrite the paragraph to state that `obs.error` carries every failure mode and that callers must branch on `obs.error is None` before touching `obs.result.data`. - Apply the same guard in the eval-loop snippet. - Document that `call_tool()` raises `RuntimeError` on any `obs.error` (mcp_client.py:430-435) — not a drop-in swap for `step()` when you need to branch on `error_type`. - Soften the FastMCP-dependent `TOOL_NOT_FOUND` message to `e.g. ...` so it does not break if FastMCP changes its wording. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Thorough and well-written tutorial with strong alignment to OpenEnv invariants. Two issues require changes before merge; several minor items are noted for the author's awareness.
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md:243— Broken link to../guides/rewards.md. The PR description states it replaced a broken link torubrics.md(on an unmerged PR) with../guides/rewards.md, butdocs/source/guides/rewards.mddoes not exist anywhere onmainor in any worktree. This will produce a Sphinx build warning (or broken rendered link) and should be replaced with a link to an existing page (e.g.,../core.mdor the quickstart, depending on what you want to cross-reference). -
docs/source/mcp-environment-lifecycle.md— base file not onmain. The PR diff modifies this file, butdocs/source/mcp-environment-lifecycle.mddoes not exist in the currentmainbranch. The MkDocs docs tree lives atdocs/(nosource/subdirectory); the Sphinxdocs/source/tree exists only in open worktrees. Either: (a) the PR should be stacked on the worktree/PR that introducesdocs/source/andmcp-environment-lifecycle.md, or (b) the file path should be corrected to match where these docs actually land onmain. Merging as-is drops changes that have no target file to patch.
Tier 2: Alignment Discussion
None identified. The tutorial correctly and explicitly explains the dual API boundary (WebSocket for orchestration, MCP for agents), and the note block on p.1 prominently calls out that RFC 003 is still "In Review" and that only a handful of envs are MCP-backed today. The reserved-tool-name invariant is documented accurately. The tutorial does not suggest exposing reset()/step()/state() to agents; those names appear only in the "reserved names" bullet that says they raise at construction time.
Factual accuracy notes (not blocking)
These are minor imprecisions that are defensible but worth the author knowing:
-
obs.result.dataguard.CallToolObservation.resultis typedAnyinsrc/openenv/core/env_server/mcp_types.py:284. When the call goes through the FastMCP path,resultis aCallToolResultand.dataexists. However,examples/echo_mcp_demo.py:80usesobs.result.data if hasattr(obs.result, "data") else obs.resultas the defensive idiom. The tutorial's unguardedobs.result.datais accurate for the FastMCP path the tutorial describes, but readers writing library code should be aware the type isAny. -
calendar_envinheritance. The tutorial sayscalendar_env"uses a local wrapper with the same shape." Inenvs/calendar_env/server/calendar_environment.py,CalendarEnvironmentinherits from a localMCPEnvironmentdefined inopenenv_wrapper/mcp_env_environment.py, which in turn inherits fromopenenv.core.env_server.interfaces.Environment— not from the canonicalopenenv.core.env_server.mcp_environment.MCPEnvironment. This means the "check whether it inherits from anMCPEnvironmentbase" advice in the tutorial's note block would actually pass forCalendarEnvironment(because its local base class is also namedMCPEnvironment), which could mislead readers usingisinstancechecks. -
MCPToolClientmode constraint.src/openenv/core/mcp_client.py:106-110raisesValueErrorifmode != 'production'. The lifecycle doc correctly characterisescall_tool()as going throughstep()(the default path whenuse_production_mode = False), which is accurate, but the broader picture — thatMCPToolClientforces production mode at the constructor level — is not surfaced in either the tutorial or the lifecycle doc. This could confuse readers who try to useMCPToolClientin a simulation training loop and get aValueError.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: FAIL - pre-existing failures in
envs/carla_env/(unrelated to this PR; no Python touched here) - Debug code: CLEAN - no debug code introduced by this PR
Open RFCs Context
- RFC 003 (MCP Support): In Review — directly relevant. The PR correctly notes RFC 003 is still In Review and that only ~3 envs use
MCPEnvironmenttoday. The tutorial accurately reflects the delta between the RFC vision and current adoption.
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md:258— TheEchoEnvironmentcode block includesSUPPORTS_CONCURRENT_SESSIONS = True, but this class attribute does not appear in the realenvs/echo_env/server/echo_environment.py. The tutorial explicitly says the snippet is "trimmed from" that file. Including an attribute that does not exist in the reference source is misleading; either remove it or annotate it clearly as an addition the tutorial author recommends for production use. -
docs/source/tutorials/mcp-environment.md:295—_step_impldocstring in the snippet says"Unsupported action type"but the real echo env says"Unknown action type". Minor factual inconsistency for a snippet described as trimmed from source. -
docs/source/tutorials/mcp-environment.md:215— The tutorial statescall_tool()"still goes through the step loop and still updates rewards, step counts, and trajectory state". InspectingMCPToolClient.call_toolinsrc/openenv/core/mcp_client.pyconfirms it callsself.step(action)so rewards flow correctly. However,MCPClientBase.__init__setsmode="production"by default. In production mode the client bypassesstep()and hits/mcpdirectly — meaning rewards and step counts are NOT updated. The claim is only true in simulation mode. This is an accuracy issue for users who follow the tutorial with the defaultMCPToolClient.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Tutorial describes /mcp endpoint as agent-accessible in production without adequately flagging the invariant boundary.
- Principle at stake: INVARIANTS.md §Security Invariants — "MCP tools must not expose simulation control to agents"; PRINCIPLES.md — "Agents cannot reset".
- The concern: The "Next Steps" section (line 332) says "Point an MCP-compatible client at [the
/mcpendpoint] for production inference without going through the step loop." While technically true and already described in the RFC, the tutorial gives no warning that a raw MCP client pointed at/mcpbypasses reward computation and the training invariant boundary entirely. Readers building production deployments might expose this endpoint without understanding the implications. A brief note analogous to the existing{note}block on RFC 003 adoption would bring this into alignment. - Suggested reviewer: @Darktex
Summary
- 3 mechanical/accuracy issues to fix
- 1 alignment point for human review
Overall the tutorial is high-quality, well-researched, and fills a real documentation gap. The lifecycle guide fixes for call_tool() async semantics and the CallToolResult wrapper shape are accurate and needed. The main asks are: fix the SUPPORTS_CONCURRENT_SESSIONS fabrication in the example snippet, clarify that call_tool() reward-tracking claim is mode-dependent, and add a brief dual-boundary warning near the production /mcp endpoint mention.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: N/A (docs-only)
- Debug code: CLEAN
Tier 1: Fixes Required
-
Wrong file paths — all three changed files land under
docs/source/which does not exist. The project uses mkdocs withdocs_dir: .(root isdocs/). The existing tutorials are atdocs/tutorials/openenv-tutorial.mdanddocs/tutorials/wordle-grpo.md. The PR's files should be at:docs/tutorials/mcp-environment.md(new tutorial)docs/tutorials/index.md(tutorials index)docs/mcp-environment-lifecycle.md(lifecycle guide)
As written, none of these files will be served by
mkdocs serveor the built site. The lifecycle guide fixes (asynccall_tool,CallToolResultshape) are real and valuable but are currently unreachable. -
New tutorial not added to
docs/mkdocs.ymlnav. Even after correcting the path, the file must be added to thenav:block indocs/mkdocs.ymlunderTutorials:, matching the pattern of the existing two entries. -
Verification claim is false. The PR description states
uv run sphinx-build -b html docs/source docs/_build/htmlwas used. OpenEnv uses mkdocs, not Sphinx. There is nodocs/source/directory and noconf.pyin the repo. The correct verification command (fromCLAUDE.md) ismkdocs serve --config-file docs/mkdocs.yml. This needs to be run and confirmed before merging.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: call_tool() presented as usable from training/sim loop context
- Principle at stake: Dual API boundary — WebSocket/step() for infrastructure, MCP for agents (INVARIANTS.md §Architectural Invariants #1)
- The concern: The tutorial's "step() vs call_tool()" section implies
await env.call_tool(...)is a drop-in alternative during training. In the actual codebase,MCPToolClient.call_tool()lives in the client class and its constructor raisesValueErrorif mode is not"production". TheMCPEnvironmentserver-side class has nocall_tool()method at all. A reader following the training-loop section might try to usecall_tool()in a sim-mode rollout loop and get a confusing error. The section should clearly state thatcall_tool()is a production-mode client convenience and is not available from within a server-side training loop. - Suggested reviewer: @Darktex
Summary
- 3 mechanical issues to fix (wrong paths, missing nav entry, false verification claim)
- 1 alignment point for human review (sim/production boundary blur in call_tool docs)
The content quality and factual accuracy of the tutorial are strong — the dual-API framing, the _step_impl requirement note, the honest RFC adoption caveat, and the CallToolResult wrapper explanation are all correct and valuable. The issues are structural (wrong directory) and navigational (missing nav entry), not substantive.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS (pre-existing failures in
carla_envare unrelated to this PR) - Debug code: CLEAN (no debug artifacts in changed files)
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md:243— Broken internal link../guides/rewards.md. No file atdocs/source/guides/rewards.mdexists in any branch, worktree, or stash in the local repository. The PR description says the earlierrubrics.mdlink was replaced with this path, but the target also does not exist. Verify the correct path (possibly../quickstart.mdor another existing guide), or createdocs/source/guides/rewards.mdbefore merging this tutorial. -
docs/source/tutorials/mcp-environment.md:193— The tutorial statesobs.result.dataas the unconditional way to read the raw tool return value. The canonical demo atexamples/echo_mcp_demo.py:80usesobs.result.data if hasattr(obs.result, 'data') else obs.result— ahasattrguard. WhenCallToolObservation.resultis deserialized from JSON (e.g. round-tripped over the WebSocket transport rather than constructed locally), it may arrive as a plaindictor scalar rather than aCallToolResultdataclass, making.dataabsent. The tutorial should add the same defensive pattern or call out this distinction explicitly.
Tier 2: Alignment Discussion
None identified. The tutorial's framing of the dual API boundary (WebSocket/step() for orchestration infrastructure; MCP /mcp endpoint for agents) is accurate and well-aligned with INVARIANTS.md §Architectural Invariants 1 and PRINCIPLES.md §Key Decisions Made. The explicit note that reset, step, state, and close are reserved tool names, and the transparent disclosure of RFC 003's adoption status (~10% of envs), are both positive alignment signals.
Summary
- 2 mechanical issues to fix before merge
- 0 alignment points for human review
Additional notes for the author:
The technical accuracy of the API claims is high — CallToolResult.data/.content/.structured_content, ToolErrorType enum values, _step_impl being @abstractmethod, and call_tool() being async were all verified against source. The rollout loop examples, import paths, and SUPPORTS_CONCURRENT_SESSIONS class attribute are all correct. The honest adoption note (only echo_env and finqa_env use canonical MCPEnvironment) is appreciated and should remain.
The docs/source/ path convention is consistent with the in-progress Sphinx migration visible in worktrees issue-384-eval-support and issue-385-agentic-harnesses. The lifecycle guide diff (docs/source/mcp-environment-lifecycle.md) targets a file that is expected to land as part of that migration; confirm that the base branch for this PR includes that file before merging to main.
Automated review by Claude Code | Learn more
- Change `Unsupported action type` to `Unknown action type` so the trimmed snippet matches `envs/echo_env/server/echo_environment.py:160`. - Add a note that `call_tool()` is production-only — `MCPToolClient` raises `ValueError` outside production mode — to steer simulation callers to `step(CallToolAction(...))`.
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS (for this PR's files) — The lint failures seen (
docs/source/conf.py,tests/envs/test_grid_world.py,tests/envs/test_julia_env.py) are pre-existing issues unrelated to this PR, which only touches.mdfiles not covered by ruff/usort. - Debug code: CLEAN — No new debug code introduced.
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md(call_tool production-only note) — The{note}block reads: "call_tool()is production-only:MCPToolClient.__init__raisesValueErrorifmode != "production"." This is factually wrong in a way that will actively mislead readers.MCPToolClient.call_tool()is NOT production-only. TheValueErrorinMCPClientBase.__init__(line 108 ofsrc/openenv/core/mcp_client.py) fires when themodekwarg passed to the constructor is not"production", but the methodcall_tool()itself branches onself.use_production_mode(default:False), so in the default configuration it goes throughstep(CallToolAction(...))— the simulation path. The note should say: "call_tool()is a client-side method onMCPToolClientthat requires a running server. In the default configuration (use_production_mode=False) it goes throughstep()internally. Usestep(CallToolAction(...))when working directly with an in-process environment server instance."
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Tutorial conflates client-side and server-side APIs in the "Under the Hood" section
- Principle at stake: Client-server separation (INVARIANTS.md, Architectural Invariant 2)
- The concern: The "Discovering tools" and "Calling a tool" snippets import directly from
echo_env.server.echo_environmentand callenv.step()on the server-sideEchoEnvironmentclass in-process. Then the "step vs call_tool" subsection pivots to describingcall_tool()as a method on "environment clients that inherit fromMCPToolClient" — a completely different class hierarchy on the client side. A reader following the code chronologically will reasonably infer they can callawait env.call_tool("echo_message", ...)on theEchoEnvironmentinstance they just created — which would fail, becauseEchoEnvironmentdoes not inherit fromMCPToolClient. The tutorial needs a clear demarcation: "the snippets above use the server-side class directly;call_tool()lives on the client-sideMCPToolClient, which connects to a running server." - Suggested reviewer: @Darktex
ALIGNMENT FLAG: call_tool() behavior diverges with use_production_mode — undocumented in both files
- Principle at stake: "Minimize lifecycle deltas" — training and production must use identical interfaces (PRINCIPLES.md, Core Principle 1). Silently diverging behavior based on a boolean flag contradicts this unless clearly documented.
- The concern: Both the new tutorial and the lifecycle guide describe
call_tool()as routing through the step loop (preserving episode context / rewards / step counting / trajectory semantics). This is only true whenuse_production_mode=False(the default). Whenuse_production_mode=Trueit bypasses the step loop. Neither document makes this conditional explicit. Readers who enable production mode will hit a subtle correctness gap. - Suggested reviewer: @Darktex
Positive Notes
The bulk of the tutorial is well-constructed and accurate:
obs.result.datafor the raw return value is correct and verified againstsrc/openenv/core/env_server/mcp_environment.py(CallToolResultfromfastmcp.client.clienthas.data).ToolErrorTypeenum values (TOOL_NOT_FOUND,INVALID_ARGS,EXECUTION_ERROR,TRANSPORT_ERROR,TIMEOUT) match the source atsrc/openenv/core/env_server/mcp_types.pylines 202–209.MCPEnvironment._step_implis correctly identified as@abstractmethod.- Reserved tool names (
reset,step,state,close) matchRESERVED_TOOL_NAMESinmcp_types.pyline 321. - The honest adoption caveat (3 MCP-backed envs out of ~30) is accurate and appreciated.
- The lifecycle guide edits (marking
call_tool()as async, clarifyingobs.resultvs raw return value) are correct against the source. - The
echo_mcp_demo.pyreference path (examples/echo_mcp_demo.py) is confirmed to exist. - The RFC 003 status caveat (In Review, aspirational) is consistent with the principles document.
Summary
- 1 mechanical issue to fix (factually wrong note about
call_tool()being production-only) - 2 alignment points for human review (client-server boundary clarity in tutorial flow; production-mode behavioral divergence undocumented in both docs)
The lifecycle guide edits are clean and improve accuracy. The new tutorial is generally high quality and fills a real gap. The one Tier 1 fix is important because the wrong note could cause debugging confusion for readers trying to understand when call_tool() is available, but it is a small targeted correction, not a structural rework.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: N/A — docs-only PR, no Python files changed
- Debug code: CLEAN — no Python touched
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md(Eval section, line ~185) —../guides/rewards.mdis a broken link. The filedocs/source/guides/rewards.mddoes not exist anywhere in the repo. The PR body says an earlier draft linked torubrics.md(in an unmerged PR) and that was fixed, but the replacement link (../guides/rewards.md) is equally broken. Either point to an existing page (e.g.../environments/echo.md) or remove the link entirely. -
docs/source/tutorials/mcp-environment.md(call_tool()note, line ~167) — The note reads: "MCPToolClient.__init__raisesValueErrorifmode != "production"". This is accurate forMCPClientBase.__init__(src/openenv/core/mcp_client.py:107-111), but the tutorial framescall_tool()as "production-only" without explaining thatMCPToolClient(the client class, connecting to a running server) is distinct fromMCPEnvironment(the server class). Readers building an env withMCPEnvironmentwill be confused about why a mode restriction applies to them. The note should clarify: "this restriction applies to the client (MCPToolClient), not the server-sideMCPEnvironmentsubclass being built in the section above."
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Tutorial presents /mcp endpoint as usable "without going through the step loop" for production inference
- Principle at stake: Dual API boundary — WebSocket for infrastructure (Gym-like API), MCP for agents; and Agents cannot reset (INVARIANTS.md §Security Invariants 1, §Architectural Invariants 1)
- The concern: The "Next Steps" section's last bullet says: "Serving tools to an external agent — the
/mcpJSON-RPC endpoint is available alongside/wson any MCP environment server. Point an MCP-compatible client at it for production inference without going through the step loop." This is factually accurate per RFC 003 Scenario 3 (Use Case 3: Call Tool Directly), but it surfaces a boundary question the project has not yet fully resolved: an external agent calling/mcpdirectly bypasses the step loop and therefore bypasses reward computation, episode tracking, anddonesignalling. RFC 003 marks this as an "Alternative Flow" and the note in the RFC architecture diagram labels it with a dashed line (bypass step). The tutorial presents it as a straightforward production pattern without flagging that rewards and trajectory state are not updated on this path. This could mislead env builders into thinking/mcpdirect-call is equivalent tostep(CallToolAction(...))in all contexts. The tutorial should add a note that direct/mcpaccess skips reward computation and episode tracking — acceptable for pure inference, but not for training or eval loops that depend onobs.reward. - Suggested reviewer: @Darktex
Summary
- 2 mechanical issues to fix (broken link, confusing note on production-mode restriction)
- 1 alignment point for human review (direct
/mcpbypass framing in Next Steps)
Overall: The tutorial is well-structured, accurately cross-references source code, and the lifecycle guide fixes are correct (call_tool() is indeed async and does return the unwrapped value). The dual API boundary explanation in the "dual API boundary" callout box is correct and aligns with INVARIANTS.md. The RFC 003 adoption caveat note is a strong addition. The two Tier 1 issues are mechanical and straightforward to resolve.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS — docs-only change, no Python source modified
- Debug code: CLEAN — no debug artifacts introduced
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md:193—obs.result.datais not always safe to call. The tutorial assertsobs.result.datais the canonical way to get the raw tool return, and statesobs.resultis always aCallToolResultwrapper. However,CallToolObservation.resultis typedAnyinsrc/openenv/core/env_server/mcp_types.py. The actual implementation (mcp_environment.py:558) stores a raw FastMCPCallToolResulton the happy path, but the internalecho_mcp_demo.pydemonstrates the defensive pattern that the tutorial itself should use:result_value = obs.result.data if hasattr(obs.result, 'data') else obs.result. The tutorial's unconditionalobs.result.dataon line 193 (print(obs.result.data)) and lines 110/150 would raiseAttributeErrorifresultis not aCallToolResultobject (e.g. after a JSON round-trip that deserialises it as a plain dict, or from a subclass that stores a raw value). At minimum the prose should note this is not guaranteed, and the code should mirror the defensive pattern in the demo. -
docs/source/tutorials/mcp-environment.md:263—SUPPORTS_CONCURRENT_SESSIONS = Truein the tutorial'sEchoEnvironmentsubclass does not match the actual source. The tutorial presents a "trimmed" version ofenvs/echo_env/server/echo_environment.pyand includesSUPPORTS_CONCURRENT_SESSIONS = Trueas a class-level field. The actualEchoEnvironmentonmaindoes not declare this field (confirmed by inspection). A reader building their own environment from this sample would add a flag the base class documents as optional and defaulting toFalse, without understanding when and why to set it. Either remove the field from the sample (to match the actual echo env) or add a short explanatory comment. -
docs/source/tutorials/mcp-environment.md:218—call_tool()production-only note attributes the ValueError toMCPToolClient.__init__, but it is raised inMCPClientBase.__init__. The note reads: "MCPToolClient.__init__raisesValueErrorifmode != "production"". The check lives inMCPClientBase.__init__(line 107-110 ofsrc/openenv/core/mcp_client.py). The error message text happens to say "MCPToolClient" (a pre-existing copy-paste in the error string), but the correct framing is "MCPClientBase(and therefore all MCP clients) only supports production mode". This is a minor terminology point but matters for readers tracing the code.
Tier 2: Alignment Discussion
No alignment flags. The tutorial correctly:
- Identifies
step()/reset()/state()as the infrastructure (orchestrator) boundary, never exposed to the agent via MCP tools - Frames
env.reset()in the eval loop as orchestration code, not agent code - Preserves the dual-API boundary: agent uses MCP tools, trainer uses Gym-style control plane
- Notes RFC 003 is still In Review and current adoption is partial (~3 of 30 envs)
- Does not expose or suggest exposing
reset,step,state, orcloseas MCP tool names (the reserved-names bullet is correct) - Does not introduce any client importing from a
server/directory
Summary
- 3 mechanical/factual issues to fix (unsafe
obs.result.dataaccess pattern,SUPPORTS_CONCURRENT_SESSIONSdrift from actual source, minor attribution error in production-mode note) - 0 alignment points for human review
The overall structure of the tutorial is solid and the dual-API boundary framing is accurate. The three issues above are correctness gaps that a reader following the tutorial verbatim would hit.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS - docs-only change, no Python touched
- Debug code: CLEAN - no issues in this PR's files
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md(multiple locations) - Broken cross-reference:../mcp-environment-lifecycle.mddoes not exist onmainand is not introduced by this PR. The lifecycle guide is referenced in 3 places (the "dual API boundary" note, thecall_tool()vsstep()section, and the Next Steps list) and is also the file being patched in this very diff — but it is absent from the repo tree. This PR cannot ship until the lifecycle guide either lands first (prerequisite PR) or is bundled here. -
docs/source/tutorials/mcp-environment.md:247- Broken cross-reference:../guides/rewards.mddoes not exist. Thedocs/source/guides/directory does not exist onmain. This link will produce a Sphinx build warning/error. Replace with a link that resolves, or create the file.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: "Serving tools to an external agent" section implies agents can bypass step()
- Principle at stake: Dual API boundary (INVARIANTS.md §Architectural Invariant 1); "Agents cannot reset" (PRINCIPLES.md)
- The concern: The final Next Steps bullet reads "Point an MCP-compatible client at it for production inference without going through the step loop." In a production inference context this is the documented and correct behaviour. However, the tutorial gives no guidance that the
/mcpendpoint must never exposereset,step, orstateas tool names. The "Reserved names" bullet above covers construction-time registration guards, but the narrative around external MCP clients does not reinforce that those clients still cannot trigger simulation control. A one-sentence clarification ("the/mcpendpoint exposes only registered tools —reset,step, andstateare not callable through it") would close the gap. - Suggested reviewer: @Darktex
Summary
- 2 mechanical issues to fix (broken cross-references to files that do not exist on
main) - 1 alignment point for human review (minor wording gap around the external-client path and simulation-control boundary)
The core technical content — API shapes, _step_impl requirement, ToolErrorType enum, async call_tool() semantics, and the dual-boundary framing — is accurate and well-written. The blocking issue is the two dangling doc links.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: FAIL (exit 2, no output - pre-existing, no Python changed)
- Debug code: CLEAN (prints in mcp_client.py are docstring examples, pre-existing)
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.mdline 146-150 — API surface mismatch in TRL snippet. Theself.envinside anenvironment_factorywrapper is anMCPToolClient(a remote client).MCPToolClient.step()returnsStepResult[Observation], notCallToolObservation. Thereforeobs.result.dataandobs.rewardat lines 149-150 are wrong for a client context —obsis aStepResultwhose fields areobs.observation,obs.reward, andobs.done. The correct access isobs.observation.result.dataandobs.reward. The server-side snippets earlier in the tutorial (discovery, error handling, building an env) correctly useCallToolObservationdirectly because they instantiateEchoEnvironment()locally — the TRL snippet is the only one using a remote client shape but getting it wrong.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Tutorial conflates server-side MCPEnvironment.step() with client-side MCPToolClient.step() return shapes without explicitly naming the distinction.
- Principle at stake: "Minimize lifecycle deltas" (PRINCIPLES.md) / Dual API boundary (INVARIANTS.md §Architectural 1)
- The concern: Five of the six code samples instantiate
EchoEnvironment()directly (server object,step()returnsCallToolObservation). The TRL snippet silently switches to the client shape (MCPToolClient.step()returnsStepResult) without flagging the change. A reader who wires up the TRL pattern against a remote env will get aStepResultback and hitAttributeError: 'StepResult' object has no attribute 'result'. The tutorial is thorough aboutstep()vscall_tool()but does not document the server-vs-clientstep()return type difference anywhere. - Suggested reviewer: @Darktex
Summary
- 1 mechanical issue to fix (
obs.result.data/obs.rewardin TRL snippet assumes wrong return type) - 1 alignment point for human review (server vs client step() return shape not documented)
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS (for changed files) - all three changed files are Markdown; no Python touched. Pre-existing lint issues in
src/openenv/core/containers/test_local_docker_provider.pyare unrelated to this PR. - Debug code: CLEAN - the
printstatements and TODOs flagged bycheck-debug.share pre-existing and not in any file modified here.
Tier 1: Fixes Required
-
docs/source/tutorials/mcp-environment.md(note block, line ~218 in the new file) - The note reads: "call_tool()is production-only:MCPToolClient.__init__raisesValueErrorifmode != 'production'." This is slightly inaccurate on two counts: (a) it isMCPClientBase.__init__(the base class) that does the mode validation, notMCPToolClient.__init__; (b) the constraint is on the client class as a whole being limited to production mode, not specifically on thecall_tool()method. A reader following the note literally might waste time looking in the wrong class. Suggested fix: rephrase to "TheMCPToolClient(and its baseMCPClientBase) only supportsmode='production'- construction raisesValueErrorfor any other mode." -
docs/source/mcp-environment-lifecycle.md- This file is referenced in the diff as an existing file being modified, but it does not appear to exist onmain. If it was introduced by a predecessor PR that has not yet merged, this PR will produce broken cross-links in the rendered docs. Please confirm the dependency and either stack this PR on top of the lifecycle-guide PR, or add a note in the PR description identifying which prior PR must land first.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Tutorial note on call_tool() implies it routes around step()
- Principle at stake: "Rewards inside environment" (RFC 002) + "Dual API boundary" (RFC 001, INVARIANTS.md)
- The concern: The tutorial's note block says
call_tool()is "production-only" and implies it is unsuitable for simulation. The source code bears this out (MCPClientBase.__init__hard-rejects non-production mode). However, the tutorial does not call out the deeper reason this restriction exists: allowingcall_tool()in simulation mode would let callers bypass reward computation, step counting, and trajectory recording - a violation of the dual-API boundary principle. The note would benefit from one sentence explaining the "why", so readers understand this is a design constraint and not an arbitrary limitation. Without it, a reader might file a feature request or reach for a workaround. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: Tutorial exposes direct environment instantiation in simulation code snippets
- Principle at stake: "Container isolation" (INVARIANTS.md) + "Minimize lifecycle deltas" (PRINCIPLES.md)
- The concern: Every training-loop and eval code sample instantiates
EchoEnvironment()directly (e.g.env = EchoEnvironment()without a container provider). This is fine for a local dev tutorial, but readers who adopt the pattern verbatim for actual training will run the environment in-process without Docker isolation. The tutorial does not note that in production training the environment should be launched in a container and accessed viaEnvClient. Other tutorials (Wordle GRPO) frame this correctly by pointing to theenvironment_factorywrapping path. A brief callout - even one sentence - would prevent the pattern from propagating into training scripts that skip isolation. - Suggested reviewer: @Darktex
Summary
- 2 mechanical issues to fix (base-class attribution in note; potential missing dependency on lifecycle-guide file)
- 2 alignment points for human review (missing "why" behind production-only restriction; direct env instantiation in training samples without container callout)
Overall the tutorial is high quality: API claims verified against the source, the RFC 003 adoption disclaimer is honest, and the dual-boundary framing is correct. The two Tier 1 items above should be straightforward to address before merge.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Summary
Docs-only PR adding a 264-line MCP tutorial and tightening two gaps in the lifecycle guide. The API cross-checks are thorough and mostly accurate. Two minor issues to address before merge: a misleading production-mode note and a type annotation imprecision. No invariant violations found.
Tier 1 — Bugs / Quality
-
docs/source/tutorials/mcp-environment.md(thecall_tool()production-only note block near the end of the "Under the Hood" section): The note states "call_tool()is production-only:MCPToolClient.__init__raisesValueErrorifmode != 'production'." This is technically inaccurate in a way that will confuse readers. The mode restriction lives onMCPClientBase.__init__, and it meansMCPToolClientunconditionally operates in production mode — there is no simulation mode variant. The simulation rollout loops earlier in the tutorial useEchoEnvironment()directly (server-side class), which has no such restriction;MCPToolClientis a remote client and is simply the wrong class for direct in-process simulation. The note should be rewritten to say: "The convenience methods onMCPToolClienttarget a running environment server. For direct in-process use (as in the training-loop snippets above), callenv.step(CallToolAction(...))on the environment class itself." -
docs/source/tutorials/mcp-environment.mdanddocs/source/mcp-environment-lifecycle.md(theobs.result/CallToolResultwrapper references): Both documents describeobs.resultas aCallToolResultwrapper with.data,.content, and.structured_content. Inmcp_types.py,CallToolObservation.resultis typedAny— there is noCallToolResultclass exported fromopenenv.core.env_server.mcp_types. The wrapper is afastmcpruntime object surfaced at runtime, not an OpenEnv-defined type. The documents should clarify that.datais accessible on the runtime result object but thatCallToolResultis not importable fromopenenv; or, if a typed wrapper does exist somewhere (e.g. insidefastmcp), name its import path explicitly so readers can verify it.
Tier 2 — Alignment
ALIGNMENT FLAG: Lifecycle guide footnote on WebSocket direct access
- Principle at stake: Dual API boundary (INVARIANTS.md)
- The concern: The lifecycle guide (post-patch) says "The WebSocket simulation interface remains infrastructure-only and must not be given directly to agents." This is correct and good. However, the RFC 003 architecture diagram (referenced in the tutorial's "Design rationale" Next Steps link) labels "Use Case 3: Direct MCP (Alternative Flow)" as going through the Infrastructure Control Plane box and reaching ParentMCP via
POST /mcp (bypass step). The tutorial's own prose correctly states agents use/mcp, but a careful reader following the RFC diagram might infer that going directly to/mcpis an infrastructure/orchestration action, not an agent action. This is an ambiguity in RFC 003 itself, not introduced by this PR — but since this PR links readers to the RFC as the canonical "design rationale" source, it slightly amplifies the confusion. - Suggested reviewer: @Darktex
Verdict
Approve after fixing the two Tier 1 factual inaccuracies (production-mode note and CallToolResult type path); the alignment flag is minor and can be addressed in a follow-up RFC 003 clarification.
Automated review by Claude Code | Learn more
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Conflicts: # docs/source/tutorials/index.md
# Conflicts: # docs/source/tutorials/index.md
Dismissed stale automated review after maintainer-requested fixes and passing required checks.
Dismissed stale automated review after maintainer-requested fixes and passing required checks.
burtenshaw
left a comment
burtenshaw
left a comment
There was a problem hiding this comment.
Approved per maintainer merge request after fixes and required checks passed.
3 participants
Summary
Adds a hands-on tutorial for OpenEnv's MCP surface, framed around the question a new ML engineer actually asks — "given I already have a training framework, why do I need MCP?" — and tightens two gaps in the existing lifecycle guide that surfaced while writing it.
docs/source/tutorials/mcp-environment.md(new)Walks through the four paths a reader will actually take with MCP in OpenEnv:
for turn in range(max_turns)callingenv.step(CallToolAction(...))directly), then TRL'senvironment_factoryas one concrete shape, clearly labelled as a TRL API and not an MCP API. Explicit note thatenvironment_factoryand MCP are orthogonal: Wordle usesenvironment_factorywithout MCP; MCP envs work fine withoutenvironment_factory.ListToolsAction,CallToolAction,CallToolResult(.data/.content/.structured_content),ToolErrorTypeenum, and thestep()vsawait env.call_tool()trade-off.src/openenv/core/evals/.MCPEnvironment, register tools withFastMCP's@mcp.tooldecorator (docstring → JSON schema), the required_step_implmethod for non-MCP actions, and how rewards /doneflow back throughstep().Uses
envs/echo_env/server/echo_environment.pyas the worked example and points readers atexamples/echo_mcp_demo.pyto run the discovery / call / error flows end-to-end.Includes a
{note}on current adoption: RFC 003 is aspirational and still In Review; today onlyecho_envandfinqa_envinherit from the canonicalopenenv.core.env_server.mcp_environment.MCPEnvironment(calendar_envuses a local wrapper with the same shape). The other ~27 envs still use custom action types — readers should check an env's inheritance before assuming the patterns here apply to it.Built to complement, not duplicate, the existing
docs/source/mcp-environment-lifecycle.md: the tutorial teaches how to do it, the lifecycle guide explains when/why. The tutorial links into the lifecycle guide (#which-pattern-should-you-use) rather than restating its contents.docs/source/mcp-environment-lifecycle.md(two fixes)Both gaps verified against
src/openenv/core/mcp_client.py::MCPToolClient.call_tool:call_tool()is async and must be awaited — this was not previously stated, and a sync caller tryingenv.call_tool(...)withoutawaitgets a coroutine object back.call_tool()returns the unwrapped tool value, whilestep(CallToolAction(...)).resultreturns aCallToolResultwrapper — the "Which Pattern Should You Use?" section now calls out both shapes explicitly (.data,.content,.structured_content).docs/source/tutorials/index.mdAdds the new tutorial to the "Available Tutorials" list and the toctree.
API inaccuracies caught by running the code
Every tutorial code sample was executed against the real source before shipping. That surfaced three API inaccuracies in earlier drafts, which are fixed in this PR:
env.step(CallToolAction(...)).resultis aCallToolResultwrapper — reading the raw tool return value requiresobs.result.data. Tutorial usesobs.result.dataand explains the wrapper's fields.obs.error.error_typeis aToolErrorTypeenum (TOOL_NOT_FOUND,INVALID_ARGS,EXECUTION_ERROR,TRANSPORT_ERROR,TIMEOUT), not a string.MCPEnvironment._step_implis@abstractmethod— a subclass that only overridesstep/resetcannot be instantiated. The tutorial's echo example now includes_step_impland the accompanying bullet reads "_step_implis required,stepis not".A second review pass also caught and fixed:
rubrics.md(lives in an unmerged PR, not on main) — replaced with a link to the existing Reward Design guide.env.last_observation, which is not an attribute on theEnvironmentbase class — rewrote to use theobsvariable returned fromenv.reset()/ previousenv.step().Type of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violated/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issues(Docs-only change, no Python runtime code edited. Verified via
uv run sphinx-build -b html docs/source docs/_build/html— build succeeds with zero new warnings attributable to this PR, zero warnings on eitherdocs/source/tutorials/mcp-environment.mdordocs/source/mcp-environment-lifecycle.md.)RFC Status
This PR only documents behaviour already on
mainper RFC 003 (and honestly acknowledges the gap between the RFC's "MCP for all agent actions" vision and the current ~10% adoption); no new behaviour is proposed.Test Plan
EchoEnvironmentsubclass) executed end-to-end against the realEchoEnvironment. Outputs match what the tutorial documents:obs.result.data == "Hello from MCP!",obs.error.error_type == ToolErrorType.TOOL_NOT_FOUND, etc.uv run sphinx-build -b html docs/source docs/_build/htmlsucceeds; renderedtutorials/mcp-environment.htmlshows all code blocks highlighted and the section structure expected (Why MCP → Training Loop [framework-agnostic + TRL] → Under the Hood → Eval → Building → Running the Demo → Next Steps).ListToolsAction/CallToolAction/ListToolsObservation/CallToolObservation→src/openenv/core/env_server/mcp_types.pyToolErrorTypeenum values →mcp_types.py:202-209CallToolResultwrapper with.data/.content/.structured_content→ verified at runtime viarepr(obs.result)src/openenv/core/env_server/mcp_environment.py:305-310, 333-338MCPEnvironment._step_implis@abstractmethod→mcp_environment.py:611MCPToolClient.call_toolisasync defreturningAny(unwrapped) →src/openenv/core/mcp_client.pyMCPEnvironmentinheritance acrossenvs/.../mcp-environment-lifecycle.md#which-pattern-should-you-useresolves — anchorid="which-pattern-should-you-use"confirmed in the rendered HTML.main— earlier drafts linked torubrics.md(lives in an unmerged PR) in two places; both replaced before push.@Darktex