Add minesweeper env and example#236
Conversation
meta-cla
Bot
added
the
CLA Signed
|
Hi @acharyaanusha! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code (alignment-reviewer agent), not a human review. The account posting this is shared with the human maintainer.
Automated review by Claude Code | Learn more about OpenEnv's agentic workflow
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code (alignment-reviewer agent), not a human review. The account posting this is shared with the human maintainer.
Alignment Review Report for PR #236
Executive Summary
This PR adds a new Minesweeper game environment with comprehensive implementation. The code is well-structured and follows OpenEnv patterns, but there are critical issues that must be addressed before merging.
Severity Breakdown:
- 🔴 Critical (Tier 1): 5 issues requiring immediate fixes
- 🟡 Alignment Concerns (Tier 2): 3 items flagged for human review
Tier 1: Fixes Required (Immediate Action)
🔴 1. Client Uses HTTPEnvClient Instead of EnvClient (WebSocket)
Location: src/envs/minesweeper_env/client.py:22
class MinesweeperEnv(HTTPEnvClient[MinesweeperAction, MinesweeperObservation]):Issue: The environment uses the deprecated HTTPEnvClient base class instead of the modern EnvClient (WebSocket-based) that all other environments use.
Evidence from reference implementation (envs/echo_env/client.py:31):
class EchoEnv(EnvClient[EchoAction, EchoObservation, State]):Why this matters:
- According to INVARIANTS.md line 73-74: "We are in the process of deprecating HTTP in favor of WebSocket-only"
- All existing environments (echo, chat, etc.) use
EnvClientwith WebSocket - This creates inconsistency and technical debt
Required fix: Change to inherit from EnvClient and implement WebSocket communication like echo_env does.
🔴 2. Incorrect Import Paths (openenv_core vs openenv.core)
Locations: Multiple files
src/envs/minesweeper_env/client.py:509-511src/envs/minesweeper_env/models.py:620src/envs/minesweeper_env/server/app.py:905src/envs/minesweeper_env/server/minesweeper_environment.py:1049-1050
Issue: Uses openenv_core imports instead of openenv.core pattern used by all other environments.
Examples:
# ❌ Wrong (minesweeper_env)
from openenv_core.client_types import StepResult
from openenv_core.env_server.types import Action, Observation
# ✅ Correct (echo_env reference)
from openenv.core.client_types import StepResult
from openenv.core.env_server.types import Action, ObservationEvidence: All existing environments (echo, chat, etc.) use the openenv.core import pattern with fallback handling for both in-repo and standalone modes.
Required fix: Update all imports to match the pattern in envs/echo_env/ with proper try/except for in-repo vs standalone imports.
🔴 3. Missing Copy-Paste Documentation Updates
Location: Multiple files
Issues found:
-
client.py:527-532- Docstring still mentions "echoed_message" from echo_env template:>>> print(result.observation.echoed_message) # ❌ Wrong - should be board/game_status >>> result = client.step(MinesweeperAction(message="Hello!")) # ❌ Wrong action signature
-
models.py:614- Module docstring says "echoes back messages":""" Data models for the Minesweeper Env Environment. The minesweeper_env environment is a simple test environment that echoes back messages. # ❌ Wrong! """
-
server/minesweeper_environment.py:1033-1036- Docstring says "echoes back messages":""" Minesweeper Environment Implementation. A simple test environment that echoes back messages sent to it. # ❌ Wrong! Perfect for testing HTTP server infrastructure. """
Required fix: Update all docstrings to accurately describe Minesweeper functionality, not echo functionality.
🔴 4. Docker Workflow Merge Conflict
Location: .github/workflows/docker-build.yml:82-84
Issue: The PR introduces a duplicate/malformed entry in the build matrix:
- name: my-env # Add your environment here
dockerfile: src/envs/connect4_env/server/Dockerfile # ❌ Added by this PR
- name: minesweeper-env # ✅ Also added by this PR
dockerfile: src/envs/minesweeper_env/server/Dockerfile
dockerfile: envs/connect4_env/server/Dockerfile # ❌ Duplicate key from mergeWhy this happened: The PR incorrectly modified the template placeholder line AND added a new line, creating a malformed YAML structure.
Required fix:
- Remove the
my-envmodifications - Keep only the clean
minesweeper-enventry - Ensure the dockerfile path is correct
🔴 5. Missing Newline at End of Files
Locations:
src/envs/minesweeper_env/openenv.yaml:743- "\ No newline at end of file"src/envs/minesweeper_env/pyproject.toml:782- "\ No newline at end of file"src/envs/minesweeper_env/server/Dockerfile:856- "\ No newline at end of file"src/envs/minesweeper_env/server/minesweeper_environment.py:1348- "\ No newline at end of file"
Required fix: Add newline character at end of each file (POSIX standard).
Tier 2: Alignment Discussion
🟡 1. Models Use dataclass Instead of Pydantic
Location: src/envs/minesweeper_env/models.py:617-659
The pattern:
@dataclass(kw_only=True)
class MinesweeperAction(Action):
row: int
col: int
action_type: str # 'reveal' or 'flag'Reference implementation (echo_env):
class EchoAction(Action):
message: str = Field(..., min_length=1, description="Message to echo back")Principle at stake: "Type safety with generics and Pydantic across the wire" (PRINCIPLES.md line 16)
The concern:
- Echo environment uses Pydantic models directly (inheriting from
Action/Observationwhich are Pydantic BaseModels) - Minesweeper uses
@dataclassdecorator on top of these Pydantic base classes - This may cause issues with:
- Validation (no Field constraints like
min_length, value validation) - Serialization behavior
- API consistency
- Validation (no Field constraints like
Questions for human reviewer:
- Is mixing dataclass with Pydantic BaseModel inheritance intentional?
- Should
action_typehave validation (e.g.,Literal["reveal", "flag"])? - Does this pattern work correctly with Pydantic serialization?
Suggested reviewer: @Darktex
🟡 2. HTTP-Only Environment in WebSocket Transition Period
Principle at stake: "WebSocket for all environment communication" (INVARIANTS.md line 69-74)
The concern:
- INVARIANTS.md states: "We are in the process of deprecating HTTP (see PR #252) in favor of WebSocket-only, but we are still transitioning and both protocols are currently available."
- This new environment is built entirely on HTTP infrastructure
- Adds new technical debt during a known deprecation period
- Will require refactoring work when HTTP is fully deprecated
Questions for human reviewer:
- Should we accept new HTTP-only environments during the WebSocket transition?
- If yes, should the PR include a TODO or tracking issue for WebSocket migration?
- Is there a timeline for completing the HTTP deprecation that should inform this decision?
Suggested reviewer: @Darktex
🟡 3. Environment Location Inconsistency
The pattern:
- PR places environment in:
src/envs/minesweeper_env/ - Existing environments are in:
envs/echo_env/,envs/chat_env/,envs/coding_env/, etc.
Principle at stake: "One canonical way to build environments" (PRINCIPLES.md line 21)
The concern:
- All reference environments are in
envs/directory - This new environment is in
src/envs/directory - Creates inconsistency in repository structure
- Dockerfile path uses
src/envs/minesweeper_env/which differs from existing patterns
Questions for human reviewer:
- Is there a new convention to move environments to
src/envs/? - Should existing environments migrate to this location?
- Or should this PR move minesweeper to
envs/to match existing convention?
Suggested reviewer: @Darktex
Automated Checks
✅ Debug code check: CLEAN - No print statements, breakpoints, or TODO comments found in minesweeper files
uv command not available in environment
Positive Aspects
The PR does many things well:
- ✅ Comprehensive testing: 13 test cases covering edge cases, state management, serialization
- ✅ Good documentation: Detailed README with examples, API specs, usage instructions
- ✅ Clean game logic: Well-structured recursive reveal, proper state management
- ✅ Reward design: Sensible reward structure aligned with RL objectives
- ✅ Example agent: Demonstrates full usage with a simple but functional strategy
- ✅ Docker support: Multi-stage build with health checks
- ✅ No agent-accessible reset: Correctly does not expose reset/step to agents via MCP (maintains invariant)
Summary
Tier 1 Issues: 5 mechanical issues requiring fixes
- Client architecture (HTTP vs WebSocket)
- Import paths (openenv_core vs openenv.core)
- Copy-paste documentation errors
- Docker workflow YAML structure
- Missing newlines
Tier 2 Issues: 3 alignment questions requiring human decision
- dataclass vs pure Pydantic pattern
- Adding HTTP-only env during WebSocket transition
- Environment location convention (
src/envs/vsenvs/)
Recommendation: Request changes for Tier 1 fixes before merge. Tier 2 items need maintainer input on project direction.
Automated review by Claude Code | Learn more about OpenEnv's agentic workflow
Darktex
dismissed
their stale review
Dismissing automated approval due to bug in review bot. The original review either had blank content or approved despite finding blocking issues. Please disregard this approval.
|
@Darktex fixed all the claude code comments. Should be ready for another review |
Greptile SummaryThis PR adds a complete Minesweeper environment implementation following OpenEnv patterns, including core game logic, client/server architecture, Docker support, comprehensive tests, and example agent code. Key Changes:
Issues Found:
Strengths:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant Client as MinesweeperEnv<br/>(Client)
participant Server as FastAPI Server
participant Env as MinesweeperEnvironment
Note over Agent,Env: Connection Establishment
Agent->>Client: MinesweeperEnv(base_url)
Client->>Server: WebSocket connect
Server->>Env: Create environment instance
Server-->>Client: Connection established
Note over Agent,Env: Reset Episode
Agent->>Client: reset()
Client->>Server: WS: reset message
Server->>Env: reset()
Env->>Env: Place mines randomly
Env->>Env: Compute mine counts
Env-->>Server: MinesweeperObservation<br/>(board=-1, status=ONGOING)
Server-->>Client: WS: observation response
Client-->>Agent: StepResult(observation)
Note over Agent,Env: Game Loop - Reveal Action
Agent->>Client: step(MinesweeperAction<br/>row=2, col=2, reveal)
Client->>Server: WS: step message with action
Server->>Env: step(action)
Env->>Env: Validate position
Env->>Env: _reveal_cell(2,2)
alt Cell is mine
Env->>Env: Set status=LOST
Env->>Env: Add to revealed_cells
Env-->>Server: Observation(reward=-10.0, done=true)
else Cell is safe
Env->>Env: _reveal_recursive(2,2)
Env->>Env: Add to revealed_cells
alt mine_count == 0
Env->>Env: Recursively reveal neighbors
end
Env->>Env: _check_win_condition()
Env-->>Server: Observation(reward=1.0, done=false)
end
Server-->>Client: WS: observation response
Client-->>Agent: StepResult(observation, reward, done)
Note over Agent,Env: Game Loop - Flag Action
Agent->>Client: step(MinesweeperAction<br/>row=0, col=0, flag)
Client->>Server: WS: step message with action
Server->>Env: step(action)
Env->>Env: _toggle_flag(0,0)
alt Cell already flagged
Env->>Env: Remove from flags
Env-->>Server: Observation(reward=0.0)
else Cell not flagged
Env->>Env: Add to flags
alt Cell is mine
Env-->>Server: Observation(reward=0.5)
else Cell not mine
Env-->>Server: Observation(reward=0.0)
end
end
Server-->>Client: WS: observation response
Client-->>Agent: StepResult(observation, reward, done)
Note over Agent,Env: Session Cleanup
Agent->>Client: close()
Client->>Server: WS: close message
Server->>Env: Cleanup resources
Server-->>Client: Connection closed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _reveal_cell(self, row: int, col: int) -> float: | ||
| """Reveal the cell at (row, col). Returns the reward for the action.""" | ||
| if (row, col) in self._revealed_cells or (row, col) in self._flags_placed: | ||
| return -0.05 # Penalty for revealing already revealed or flagged cell | ||
|
|
||
| self._revealed_cells.add((row, col)) | ||
|
|
||
| if (row, col) in self._mine_positions: | ||
| self._game_status = GameStatus.LOST | ||
| self._revealed_cells.add((row, col)) | ||
| return -10.0 # Penalty for hitting a mine | ||
|
|
||
| # Reveal the cell and potentially adjacent cells if count is 0 | ||
| self._reveal_recursive(row, col) | ||
|
|
||
| return 1.0 # Small reward for safe reveal |
There was a problem hiding this comment.
logic: cell added to _revealed_cells on line 185, then _reveal_recursive adds it again on line 207, causing redundant addition
| def _reveal_cell(self, row: int, col: int) -> float: | |
| """Reveal the cell at (row, col). Returns the reward for the action.""" | |
| if (row, col) in self._revealed_cells or (row, col) in self._flags_placed: | |
| return -0.05 # Penalty for revealing already revealed or flagged cell | |
| self._revealed_cells.add((row, col)) | |
| if (row, col) in self._mine_positions: | |
| self._game_status = GameStatus.LOST | |
| self._revealed_cells.add((row, col)) | |
| return -10.0 # Penalty for hitting a mine | |
| # Reveal the cell and potentially adjacent cells if count is 0 | |
| self._reveal_recursive(row, col) | |
| return 1.0 # Small reward for safe reveal | |
| def _reveal_cell(self, row: int, col: int) -> float: | |
| """Reveal the cell at (row, col). Returns the reward for the action.""" | |
| if (row, col) in self._revealed_cells or (row, col) in self._flags_placed: | |
| return -0.05 # Penalty for revealing already revealed or flagged cell | |
| if (row, col) in self._mine_positions: | |
| self._game_status = GameStatus.LOST | |
| self._revealed_cells.add((row, col)) | |
| return -10.0 # Penalty for hitting a mine | |
| # Reveal the cell and potentially adjacent cells if count is 0 | |
| self._reveal_recursive(row, col) | |
| return 1.0 # Small reward for safe reveal |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/envs/minesweeper_env/server/minesweeper_environment.py
Line: 180:195
Comment:
**logic:** cell added to `_revealed_cells` on line 185, then `_reveal_recursive` adds it again on line 207, causing redundant addition
```suggestion
def _reveal_cell(self, row: int, col: int) -> float:
"""Reveal the cell at (row, col). Returns the reward for the action."""
if (row, col) in self._revealed_cells or (row, col) in self._flags_placed:
return -0.05 # Penalty for revealing already revealed or flagged cell
if (row, col) in self._mine_positions:
self._game_status = GameStatus.LOST
self._revealed_cells.add((row, col))
return -10.0 # Penalty for hitting a mine
# Reveal the cell and potentially adjacent cells if count is 0
self._reveal_recursive(row, col)
return 1.0 # Small reward for safe reveal
```
How can I resolve this? If you propose a fix, please make it concise.| def test_setup_server(self): | ||
| """Set up the Minesweeper server for testing.""" | ||
| self.server_process = subprocess.Popen( | ||
| ["python", "-m", "envs.minesweeper_env.server.app"], | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) | ||
| # Give it a few seconds to start | ||
| time.sleep(3) | ||
|
|
||
| def check_server_running(self): | ||
| """Check if the server is running and healthy.""" | ||
| try: | ||
| response = requests.get("http://127.0.0.1:8000/health") | ||
| self.assertEqual(response.status_code, 200) | ||
| except requests.ConnectionError: | ||
| self.fail("Server did not start or is unreachable") | ||
|
|
There was a problem hiding this comment.
logic: each test starts a new server but doesn't clean up previous ones, causing port conflicts. Tests should share one server instance across all tests using setUpClass and tearDownClass
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/envs/test_minesweeper_env.py
Line: 40:58
Comment:
**logic:** each test starts a new server but doesn't clean up previous ones, causing port conflicts. Tests should share one server instance across all tests using `setUpClass` and `tearDownClass`
How can I resolve this? If you propose a fix, please make it concise.| HEALTHCHECK --interval=30s --timeout=5s --start-period=10s CMD curl -f http://localhost:8000/health || exit 1 | ||
|
|
||
| # Run the FastAPI server | ||
| CMD ["/app/.venv/bin/uvicorn", "minesweeper.server.app:app", "--host", "0.0.0.0", "--port", "8000"] |
There was a problem hiding this comment.
syntax: module path minesweeper.server.app:app is incorrect. Based on the COPY and PYTHONPATH setup, it should be server.app:app
| CMD ["/app/.venv/bin/uvicorn", "minesweeper.server.app:app", "--host", "0.0.0.0", "--port", "8000"] | |
| CMD ["/app/.venv/bin/uvicorn", "server.app:app", "--host", "0.0.0.0", "--port", "8000"] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/envs/minesweeper_env/server/Dockerfile
Line: 67:67
Comment:
**syntax:** module path `minesweeper.server.app:app` is incorrect. Based on the COPY and PYTHONPATH setup, it should be `server.app:app`
```suggestion
CMD ["/app/.venv/bin/uvicorn", "server.app:app", "--host", "0.0.0.0", "--port", "8000"]
```
How can I resolve this? If you propose a fix, please make it concise.
acharyaanusha
force-pushed
the
feature/minesweeper_env
branch
from
687084c to
12d90b8
Compare
Implement a Minesweeper game environment for reinforcement learning agents. The environment features a grid-based game where agents must reveal all non-mine cells without triggering mines. Key features: - 5x5 grid with configurable mine placement - Two action types: reveal cells and place/remove flags - Number indicators showing adjacent mine counts - Recursive cell reveal for cells with zero adjacent mines - Reward system based on game progress and outcomes - Game status tracking (ONGOING, WON, LOST) Components: - Environment server with FastAPI integration - Game logic in MinesweeperEnvironment class - HTTP client for remote environment access - Pydantic models for actions and observations - Package configuration with dependencies
Add containerization and documentation for the Minesweeper environment to enable easy deployment and usage. Docker support: - Multi-stage Dockerfile based on openenv-base image - Build script for convenient local image creation - Health check endpoint configuration - Optimized layer caching for faster builds Documentation: - Comprehensive README with usage examples - Quick start guide with code samples - Environment details (actions, observations, rewards) - Docker build and deployment instructions - Configuration options and default settings Example: - Interactive agent demonstration script - Shows environment lifecycle (reset, step, close) - Demonstrates both reveal and flag actions - Example output with game state visualization
Add comprehensive test suite and GitHub Actions workflow integration for automated building and deployment of the Minesweeper environment. Test suite (13 test cases): - Server setup and health checks - Initial state validation (board size, unrevealed cells) - Action tests (reveal, flag, toggle flag) - Edge cases (invalid positions, already revealed cells) - Game state management and status tracking - Board cell value validation - Reset functionality and state cleanup - Multi-step action sequences - Proper handling of HTTP serialization for enums CI/CD integration: - Add minesweeper-env to GitHub Actions build matrix - Automated Docker image building and publishing - Multi-platform support (linux/amd64, linux/arm64) - Image pushed to GitHub Container Registry
- Changed client from HTTPEnvClient to EnvClient (WebSocket) - Updated all imports from openenv_core to openenv.core pattern - Converted models from @DataClass to pure Pydantic with Field validation - Fixed copy-paste documentation from echo_env template - Simplified server/app.py to match new factory pattern - Added missing newlines at end of files (POSIX compliance) - Fixed Docker workflow merge conflict (removed duplicate entries) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…d clarity and functionality
acharyaanusha
force-pushed
the
feature/minesweeper_env
branch
from
12d90b8 to
98bf1d4
Compare
|
@Darktex updated based on greptile comments |
|
Looks good @acharyaanusha. Have you also deployed it to the HF hub, and updated the environments page |
@burtenshaw No I haven't but I could do it and let you know |
…acity errors Tests were creating new MinesweeperEnv clients per test without closing them, exhausting the server's single-session capacity. Added setUp/tearDown for proper client lifecycle and use sys.executable for the server subprocess. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use unittest.IsolatedAsyncioTestCase with async with context managers to work directly with the async MinesweeperEnv client, avoiding the SyncEnvClient wrapper. Each test properly connects and disconnects within a single event loop, preventing session capacity errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le immediately Initialize _mine_counts with proper dimensions and call self.reset() in __init__ to ensure the board is fully set up before any step call. Prevents IndexError in _reveal_recursive when the web interface calls step on a freshly constructed environment instance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@burtenshaw : pushed it on HF as well https://huggingface.co/spaces/anushaacharya/minesweeper |
|
@Darktex : ready for final review! |
|
@burtenshaw ready for another review |
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: SKIP (files are under
src/envs/notenvs/; lint.sh scopes tosrc/for usort andenvs/for ruff format, so these new files would land in neither scope correctly until the path issue is resolved) - Debug code:
examples/minesweeper_agent.pyusesprint()throughout andbuild_docker.shhas decorative emojiecholines — acceptable in scripts/examples, but worth noting both violate the project's no-emoji convention.
Tier 1: Fixes Required
BLOCKER
-
src/envs/minesweeper_env/server/minesweeper_environment.py:_check_win_condition— Win condition can overwrite LOST with WON. When a mine is revealed,_reveal_cellsets_game_status = GameStatus.LOSTand adds the mine cell to_revealed_cells. Immediately after,step()unconditionally calls_check_win_condition(). That method checkslen(self._revealed_cells) == total_cells - self.num_mines— but_revealed_cellsnow includes the mine cell. On a near-complete board (e.g., 20 safe cells with 19 already revealed), hitting the last mine yieldsrevealed_count = 20 == 20, triggeringGameStatus.WONand overwritingLOST. Fix: guard_check_win_conditionto skip if the game is already over, or exclude mine cells from the revealed count. -
.github/workflows/docker-build.yml:88-89— Missingcontextkey for minesweeper-env; the Dockerfile path is wrong and references a non-existent directory. The entry is:- name: minesweeper-env dockerfile: src/envs/minesweeper_env/server/Dockerfile
No
contextfield, and the Dockerfile expectssrc/envs/minesweeper_envto beCOPY-able from the root. This is moot until the path issue below is resolved.
MAJOR
-
src/envs/minesweeper_env/server/minesweeper_environment.py:_reveal_cell— Penalty returned for flagged cell is ambiguous. Returns-0.05if the target cell is in_flags_placedand silently eats the action without toggling the flag — the agent gets no feedback that the cell is flagged. Per PATTERNS.md, return error info in observations. -
src/envs/minesweeper_env/models.py/client.py/server/app.py—try/except ImportErrorblocks are identically duplicated in both branches. Example inclient.py:try: from openenv.core.client_types import StepResult except ImportError: from openenv.core.client_types import StepResult # identical line
The
exceptbranch imports from the exact same package; onlyfrom .models import ...vsfrom models import ...differs. This will silently mask realImportErrors from misspelled or missing modules. -
src/envs/minesweeper_env/models.py:MinesweeperObservation—board: List[List[Any]]is not JSON-safe as specified. Board contains mixedintandstrsentinels (-1,0-8,'F','*'). The schema is untyped (Any), making auto-generated OpenAPI docs and JSON Schema validation meaningless. The test itself hasassert isinstance(observation.game_status, (GameStatus, int))— a signal that round-tripping through HTTP loses enum type information. Use a discriminated union or typed sentinel model. -
src/envs/minesweeper_env/server/Dockerfile— Build runs as root. NoUSERdirective. Follow the pattern inenvs/echo_env/server/Dockerfile.
MINOR
-
tests/envs/test_minesweeper_env.py:8-12—sys.path.insertandos.environ["PYTHONPATH"]manipulation at module level duplicates what the test runner already provides (PYTHONPATH=src:envs). Remove. -
tests/envs/test_minesweeper_env.py:test_reveal_already_revealed— Fragile branching. If(2,2)was a mine and the game ended, the second step returnsreward=0.0("game already over"), not a negative penalty, causing a spurious assertion failure. -
src/envs/minesweeper_env/server/minesweeper_environment.py:__init__— Callsself.reset()in__init__. Anti-pattern: a subclass override ofreset()referencing subclass attributes will fail. Require explicitreset()calls (Gym convention).
Tier 2: Alignment Discussion
ALIGNMENT FLAG — Wrong environment root directory
- Principle at stake: PATTERNS.md canonical layout — every environment lives under
envs/<name>/; all existing environments follow this. - Concern: This PR places the env at
src/envs/minesweeper_env/. Thesrc/subtree is foropenenv-corelibrary code (src/openenv/). Placing an environment there conflates library code with environment definitions, breaks the docker build matrix (which usesenvs/paths for context), and is inconsistent with every other env in the repo. All 14 new files need to move toenvs/minesweeper_env/.
ALIGNMENT FLAG — Missing typed generics on Environment
- Principle at stake: INVARIANTS.md §API Invariants — "All environments must use
Environment[ActT, ObsT, StateT]generics". - Concern:
MinesweeperEnvironment(Environment)is declared without type parameters; should beEnvironment[MinesweeperAction, MinesweeperObservation, MinesweeperState]. Thestateproperty returns the baseStaterather thanMinesweeperState.
ALIGNMENT FLAG — Simulation-control surface exposed
- Principle at stake: INVARIANTS.md §Security Invariants — "Agents cannot access reset/simulation controls"; PRINCIPLES.md — "Rewards inside environment / Domain knowledge encapsulated".
- Concern:
get_full_state()exposes the complete mine layout (mine_locations,revealed_cells,flags).get_legal_actions()exposes the full legal action space. Neither is routed throughapp.pytoday, but they are public methods on an object accessible through the server. If a future MCP layer auto-exposes public methods, an agent could query mine positions. Make these private (_get_full_state) or remove.
Summary
3 blockers, 6 major/minor mechanical issues, 3 alignment flags.
Top priority before this can land:
- Move all 14 files from
src/envs/minesweeper_env/toenvs/minesweeper_env/to match the project convention; fixdocker-build.ymlcontext accordingly. - Fix the win-condition bug (LOST overwritten by WON when the last mine is hit on a near-complete board).
- Add a non-root
USERto the Dockerfile. - Remove the no-op
try/except ImportErrorshims. - Type the board observation (no
List[List[Any]]).
Thanks for the contribution — the test coverage is solid and the README is thorough. Once the directory placement and the win-condition bug are sorted, the rest is incremental polish.
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 — Tier 1/Tier 2
Reviewed against .claude/docs/PRINCIPLES.md, .claude/docs/INVARIANTS.md, and the reference env at envs/echo_env/.
Automated Checks
- Lint (eyeball): imports are sorted, formatting is consistent, no obvious style violations.
- Debug code: many
print(...)calls inexamples/minesweeper_agent.py— these are intentional demo prints in an example script (outsidesrc/), so they fall outside thecheck-debug.shscope. Nobreakpoint(),pdb, orTODO/FIXMEin production code.
Tier 1: Fixes Required
-
.github/workflows/docker-build.yml— the newminesweeper-enventry is missing itscontext:key. Thecontext: envs/connect4_envline shown in the diff belongs toconnect4_env. As-written, the Docker build for minesweeper will either reuse the previous context or fail entirely. Add an explicitcontext:pointing at the env directory (matching wherever the Dockerfile'sCOPYpaths assume). -
src/envs/minesweeper_env/server/app.py:17-22— thetry/exceptimport is a no-op: both branches dofrom openenv.core.env_server import create_app. Remove the dead fallback. -
src/envs/minesweeper_env/models.py:635-640— same no-optry/except(both branches import fromopenenv.core.env_server.types). Remove. -
src/envs/minesweeper_env/server/minesweeper_environment.py:1023-1030— same no-optry/exceptforEnvironment/State. Remove. -
src/envs/minesweeper_env/client.py:509-520— thetry/exceptclaims to support a "standalone" install but both branches import from the absoluteopenenv.core.*path. If the intent is a fallback for users without theopenenvpackage installed, theexceptbranch needsfrom models import MinesweeperAction, MinesweeperObservation(or similar local-relative form). As written, the fallback can never succeed. -
tests/envs/test_minesweeper_env.py:1351—os.environ["PYTHONPATH"] = SRC_PATHmutates the process environment at import time, which both pollutes the rest of the test run and won't affect already-loaded modules. Thesys.path.insert(...)on the preceding line is sufficient; drop theos.environmutation. -
src/envs/minesweeper_env/server/minesweeper_environment.py:1077—self.reset()in__init__couples construction with a stateful reset. This works whencreate_apptreats the class as a per-session factory (which the comment atapp.py:928suggests it does), but it diverges from the Gymnasium convention thatreset()is called explicitly by the orchestrator. Consider removing the implicit reset and relying on the harness to callreset().
Tier 2: Alignment Discussion
ALIGNMENT FLAG: Wrong location — env lives under src/envs/ instead of envs/
- Principle at stake: "One canonical way to build environments" (PRINCIPLES.md)
- The concern: Every existing environment (
echo_env,connect4_env,chess_env,git_env, …) lives at the repo root underenvs/. This PR places the new env atsrc/envs/minesweeper_env/. The README import examples in the PR even usefrom envs.minesweeper_env import ..., suggesting the intended path isenvs/. The CI workflow diff confirms the mismatch (Dockerfile context inconsistency, above). - Suggested reviewer: @Darktex
ALIGNMENT FLAG: reward is part of the observation payload
- Principle at stake: Gymnasium-compatible step contract; rewards are a separate channel, not an observation feature.
- The concern:
MinesweeperObservationcarries arewardfield (andclient._parse_resultreadsrewardfrom bothpayload["reward"]andobs_data["reward"]). This conflates the observation space with the reward signal and gives the agent a feature it should not be conditioning on. Reward belongs alongsidedonein the step return, not inside the observation. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: MinesweeperState exposes hidden ground truth on a publicly-importable model
- Principle at stake: Agent isolation (INVARIANTS.md §"Agents cannot reset / cannot peek")
- The concern:
MinesweeperEnvironment.get_full_state()returns the fullMinesweeperState, which includesmine_locations.MinesweeperState.to_observation()is also defined on the model class. There is no HTTP/MCP route exposing this today, but having it on the publicly-importable class is a latent risk — a future contributor wiring up "introspection" endpoints could trivially leak the solution. Recommend moving the full-state object behind a server-private boundary and only exporting the redactedto_observation()shape. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: No MCP tool layer
- Principle at stake: "MCP as universal standard" (RFC 003), dual-API boundary (INVARIANTS.md)
- The concern: The env only exposes the HTTP/WebSocket server (
app.py); there are no MCP tool definitions like other reference envs include. Per RFC 003, agent-environment interaction is supposed to flow through MCP tools while the WebSocket layer is for training orchestration. This env skips the agent-facing half of the API boundary. - Suggested reviewer: @Darktex
Summary
- 7 mechanical issues to fix (the missing CI
context:key is the most urgent — it will break Docker builds). - 4 alignment points that need human discussion before this is the canonical "how to add an env" example.
Verdict: request_changes
Automated review by Claude Code | Learn more
- docker-build.yml: set explicit context: . for minesweeper-env (Dockerfile COPY paths assume repo root) - Remove duplicate try/except ImportError blocks where both branches imported the same path (no-op fallback) in app.py, models.py, minesweeper_environment.py - client.py: only the models import differs between in-repo/standalone, so flatten core imports and keep relative/absolute fallback only for models - Drop os.environ["PYTHONPATH"] mutation and redundant sys.path.insert in tests; PYTHONPATH=src:envs is provided by the test runner - Remove implicit self.reset() in MinesweeperEnvironment.__init__; rely on explicit reset() per Gymnasium convention
Tier 2 alignment fixes from PR review: - Move src/envs/minesweeper_env/ -> envs/minesweeper_env/ so the env lives alongside every other environment in the repo (echo_env, connect4_env, etc.) - Rewrite Dockerfile to match the connect4/echo pattern: build context is the env directory and uses 'uv sync' rather than copying repo-root core sources - Update docker-build.yml to set the new path + 'context: envs/minesweeper_env' - Tidy pyproject.toml to match the connect4_env layout (package name minesweeper_env, script entry, package-dir mapping) - README: update docker build invocation to the new path - Make MinesweeperEnvironment.get_full_state private (_get_full_state) since it exposes mine locations and must never be reachable by an agent
- Guard _check_win_condition against overwriting LOST with WON when a mine reveal pushes total revealed_count to the win threshold; count only non-mine cells. - Type MinesweeperEnvironment generics (Environment[Action, Obs, State]); keep public state as base State so /state cannot leak mine_positions. - Surface action-rejection reasons (already revealed / flagged / can't flag a revealed cell) in observation.metadata["error"]. - De-flake test_reveal_already_revealed (handle the mine-on-first-reveal branch by re-trying candidates instead of asserting on game-over reward). - Add direct unit tests for the win-condition guard and metadata errors.
|
@Darktex hey addressed all the comments. ready for re-review |
6 participants
Summary
This PR adds a new Minesweeper game environment to OpenEnv, providing a classic grid-based puzzle game for reinforcement learning agents. The environment challenges agents to reveal all non-mine cells without triggering any mines, using strategies based on number clues and flag placement.
Motivation
Minesweeper is a well-known logical puzzle that requires:
This environment is valuable for:
Changes
Core Implementation
reveal: Uncover a cell to see its valueflag: Place/remove flag on suspected mine locationsFeatures
Docker Support
openenv-baseimagebuild_docker.sh) for easy local developmentTesting
Documentation
CI/CD Integration
Files Changed
New Files (14 files, 1,552 insertions)
Environment Core:
src/envs/minesweeper_env/__init__.py- Module exports and public APIsrc/envs/minesweeper_env/client.py- HTTP client for remote environment accesssrc/envs/minesweeper_env/models.py- Pydantic models for actions, observations, and statesrc/envs/minesweeper_env/openenv.yaml- Environment metadata and configurationsrc/envs/minesweeper_env/pyproject.toml- Package dependencies and build configurationServer Implementation:
src/envs/minesweeper_env/server/__init__.py- Server module exportssrc/envs/minesweeper_env/server/app.py- FastAPI application and HTTP endpointssrc/envs/minesweeper_env/server/minesweeper_environment.py- Core game logic and environment implementationDocker & Build:
src/envs/minesweeper_env/server/Dockerfile- Multi-stage container image definitionsrc/envs/minesweeper_env/server/build_docker.sh- Build script for local developmentDocumentation & Examples:
src/envs/minesweeper_env/README.md- Comprehensive environment documentationexamples/minesweeper_agent.py- Example agent demonstrating environment usageTesting:
tests/envs/test_minesweeper_env.py- Test suite with 13 test casesModified Files (1 file)
CI/CD:
.github/workflows/docker-build.yml- Addedminesweeper-envto build matrix for automated Docker image buildingUsage Example
Testing
Run the test suite:
python tests/envs/test_minesweeper_env.pyBuild and run the Docker image: