Fix coding_env API compatibility and safety reward false positives

[abhinavgautam01]

Summary

This PR fixes multiple coding_env reliability issues with a focus on API compatibility and reward correctness.
It aligns PythonCodeActEnv with expected optional reset/step parameters, removes duplicate server entrypoint execution blocks, and replaces regex-based safety checks
with AST-based detection to prevent reward false positives.
It also adds focused tests for the new behavior and safety-detection edge cases.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation
• New environment
• Refactoring

Alignment Checklist

Before submitting, verify:

• I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
• I have checked .claude/docs/INVARIANTS.md and no invariants are violated
• I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues

RFC Status

• Not required (bug fix, docs, minor refactoring)
• RFC exists: #___
• RFC needed (will create before merge)

Test Plan

Reviewers can verify with:

# Lint / syntax checks for touched files
uv run ruff check envs/coding_env/server/python_codeact_env.py \
 envs/coding_env/server/app.py \
 envs/coding_env/server/transforms.py \
 envs/coding_env/server/__init__.py \
 tests/envs/test_python_codeact_reset.py \
 tests/envs/test_coding_safety_transform.py

uv run python -m py_compile \
 envs/coding_env/server/python_codeact_env.py \
 envs/coding_env/server/app.py \
 envs/coding_env/server/transforms.py \
 envs/coding_env/server/__init__.py \
 tests/envs/test_python_codeact_reset.py \
 tests/envs/test_coding_safety_transform.py

# Focused behavior tests
PYTHONPATH=src:envs uv run pytest tests/envs/test_python_codeact_reset.py -q
PYTHONPATH=src:envs uv run pytest tests/envs/test_coding_safety_transform.py -q
PYTHONPATH=src:envs uv run pytest tests/envs/test_coding_env_integration.py -q

[greptile-apps]

Greptile Summary

This PR fixes three coding_env reliability issues: AST-based safety detection replaces regex to eliminate false positives from string literals and same-named user functions, PythonCodeActEnv.reset() gains the seed/episode_id/**kwargs signature required by the Gymnasium API contract, and the __init__.py module adopts a lazy import to decouple transforms from the optional smolagents runtime dependency.

• transforms.py: CodeSafetyTransform now uses ast.walk to detect only direct builtin calls and top-level os/subprocess imports; attribute-method calls like db.exec() are no longer penalised. CodeQualityTransform broadened its ast.parse guard to (SyntaxError, RecursionError, ValueError).
• python_codeact_env.py: reset() accepts seed, episode_id, and **kwargs for API compatibility; step() is restored to the canonical step(action) -> Observation signature from INVARIANTS.md.
• __init__.py / app.py: Module-level lazy loading via __getattr__ and an import reorder in app.py.

Confidence Score: 5/5

Safe to merge — all previously-identified bugs are resolved and the core transform/env logic is correct.

The two previous out-of-diff findings (CodeQualityTransform crash on deeply-nested code and the dead timeout_s parameter in step()) are both fixed in the current code. The remaining comments are style-level and do not affect runtime correctness.

No files require special attention for correctness; tests/envs/test_coding_safety_transform.py is worth a second look for CI portability.

Important Files Changed

Filename	Overview
envs/coding_env/server/init.py	Switched to lazy __getattr__-based import to avoid pulling in optional smolagents dependency; the resolved class is not cached on the module, so __getattr__ fires on every attribute access rather than only the first.
envs/coding_env/server/app.py	Import reordering only — openenv.core.env_server moved before local coding_env imports; no functional change.
envs/coding_env/server/python_codeact_env.py	Added seed, episode_id, and **kwargs to reset() for Gymnasium API compatibility; step() signature restored to the canonical step(action) -> Observation; both changes align with INVARIANTS.md.
envs/coding_env/server/transforms.py	Replaced regex-based safety detection with AST-based _detect_violation; broadened CodeQualityTransform exception guard to include RecursionError and ValueError; both previous P1 findings are resolved.
tests/envs/test_coding_safety_transform.py	New test file covering safety-transform false positives and recursion-error handling; missing the sys.path bootstrap used by the sibling test, so it silently breaks without PYTHONPATH=src:envs.
tests/envs/test_python_codeact_reset.py	Added two new reset tests for episode_id override and empty-string preservation; both correctly exercise the new guard in python_codeact_env.py.

Sequence Diagram

sequenceDiagram
    participant Orch as Orchestrator
    participant Env as PythonCodeActEnv
    participant Exec as PyExecutor
    participant ST as CodeSafetyTransform
    participant QT as CodeQualityTransform

    Orch->>Env: "reset(seed?, episode_id?, **kwargs)"
    Env->>Exec: PyExecutor() (fresh instance)
    Env->>ST: create_safe_coding_transform()
    Env-->>Orch: "CodeObservation(reward=0.0)"

    Orch->>Env: "step(CodeAction(code=...))"
    Env->>Exec: run(code)
    Exec-->>Env: ExecutionResult(stdout, stderr, exit_code)
    Env->>ST: __call__(obs + last_code metadata)
    Note over ST: ast.parse to _detect_violation()
    ST-->>Env: "obs(reward=-1.0 or 0.0)"
    Env->>QT: __call__(obs)
    QT-->>Env: obs(reward adjusted)
    Env-->>Orch: CodeObservation

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
tests/envs/test_coding_safety_transform.py:9-10
**Test missing sys.path bootstrap — breaks without explicit PYTHONPATH**

The sibling test `test_python_codeact_reset.py` guards against missing path setup with `sys.path.insert(...)` for the repo root plus `src/`, then uses the fully-qualified prefix `envs.coding_env.*`. This file uses the short form `coding_env.*`, which works only when the runner sets `PYTHONPATH=src:envs`. Running bare (`pytest tests/`) or in a CI job that doesn't set that variable produces an `ImportError`, while the sibling test still passes. Following the same bootstrap pattern would make the suite self-contained.

### Issue 2 of 2
envs/coding_env/server/__init__.py:18-23
**Lazy loader doesn't cache the resolved class — `__getattr__` fires on every access**

The current implementation returns `PythonCodeActEnv` on each call to `__getattr__` but never writes it back onto the module. Any code that repeatedly accesses `coding_env.server.PythonCodeActEnv` as an attribute (rather than a one-time `from … import`) will re-execute the `__getattr__` body on each access. The standard fix is to set the resolved name on the module after the first load, turning subsequent lookups into a direct `__dict__` hit.

```suggestion
def __getattr__(name: str) -> Any:
    if name == "PythonCodeActEnv":
        from .python_codeact_env import PythonCodeActEnv
        import sys

        setattr(sys.modules[__name__], name, PythonCodeActEnv)
        return PythonCodeActEnv
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
```

_{Reviews (7): Last reviewed commit: "Harden coding env quality transform" | Re-trigger Greptile}

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report — Tier 1/Tier 2

Automated Checks

• Lint: envs/coding_env/server/app.py has a usort import sort violation (pre-existing, but CI blocker)
• Debug code: CLEAN — no debug artifacts introduced

Tier 1: Fixes Required

1. step() signature incomplete vs base class

python_codeact_env.py — The base class Environment.step() declares step(self, action: ActT, timeout_s: Optional[float] = None, **kwargs: Any). The concrete implementation only has step(self, action: Action). This is a Liskov substitution violation. Since the PR reformats this method signature, this is the right moment to bring it into conformance with the base class.

2. Import sort violation in app.py

app.py — usort reports the file needs import sorting. Pre-existing issue but will block CI on merge.

3. sys.path manipulation in new test file

test_coding_safety_transform.py — Uses sys.path.insert() instead of relying on the project's standard PYTHONPATH=src:envs test setup. Should use conftest.py or the standard env var approach for consistency.

4. SyntaxError handling is a behavioral regression (minor)

transforms.py — When ast.parse() fails on syntactically invalid code containing dangerous patterns (e.g., import os\n\x00), _detect_violation() returns None and reward is set to 0.0. The old regex approach would have caught import os even in broken code. This is likely acceptable (CodeQualityTransform handles syntax errors separately), but should be documented as an intentional trade-off.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: reset() signature extended with seed, episode_id, **kwargs

• Principle at stake: API Invariant #1 from INVARIANTS.md
• Assessment: The base class Environment.reset() already declares these parameters. Adding them to the concrete class brings coding_env into compliance with the documented invariant, not violating it. This is correct.
• Concern: The episode_id override allows the training orchestrator to force a specific episode ID. Confirm that no MCP-exposed tool forwards episode_id to reset(), which would let an agent influence its own episode identity.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: AST safety check coverage

• Principle at stake: Rewards inside environment (RFC 002)
• Assessment: The switch from regex to AST is a clear improvement — eliminates false positives from string literals (print("import os")) and similarly-named functions (myopen()). The AST check correctly only inspects ast.Name nodes, so db.exec("sql") is properly ignored (contradicting Greptile's P1 claim).
• Concern: The blocked set is the same scope as before, but since this is a rewrite of the safety logic, worth asking: is the set complete? Notably absent: importlib.import_module("os") bypasses the check since it's an ast.Attribute call, not ast.Name. Similarly pathlib, socket, ctypes. May be acceptable if the environment has OS-level isolation.
• Suggested reviewer: @Darktex

Clarification on Greptile's Prior P1 Claims

Greptile's review (from May 2) flagged two P1 issues that do not apply to the actual diff:

1. "timeout_s accepted but never applied" — The actual diff does NOT add timeout_s to step(). It only reformats the existing signature. Greptile appears to have reviewed a different version of the code.
2. "Attribute-based call detection introduces new false positives" — The actual code checks only ast.Name (bare function calls like exec(...)), NOT ast.Attribute (method calls like db.exec(...)). The test test_does_not_flag_attribute_method_named_exec explicitly verifies this. Greptile's claim is incorrect.

Summary

• 2 mechanical fixes needed (step() signature conformance, import sort)
• 1 minor style fix (sys.path manipulation in tests)
• 1 behavioral note to document (SyntaxError handling trade-off)
• 2 alignment questions for human review (episode_id/MCP boundary, safety check coverage)
• Greptile's P1 flags are not applicable to the actual diff

Verdict: REQUEST_CHANGES

---

Automated review by Claude Code | Learn more

[abhinavgautam01]

@greptile review

[abhinavgautam01]

@greptile review

[abhinavgautam01]

@greptile review

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review: PR #635

Tier 1: Bugs & Issues

1. reset() signature missing **kwargs from base class

The base class Environment.reset() accepts **kwargs: Any for forward-compatibility. This PR adds seed and episode_id but omits **kwargs. If the framework ever passes an additional keyword argument to reset(), PythonCodeActEnv will raise a TypeError while other environments will not. Please add **kwargs to match the full base signature.

2. seed parameter accepted but unused — intentional?

seed is accepted in the new signature but never referenced in the method body. This is likely fine (no random state to seed), but a brief inline comment clarifying this would prevent future contributors from assuming it's a bug.

3. AST-based safety detection has known bypass vectors

The switch from regex to AST analysis is a clear improvement that eliminates false positives. However, the following bypass patterns will NOT be caught:

• getattr(__builtins__, 'eval')(code) — attribute-based access
• importlib.import_module('os') — dynamic import via importlib
• __builtins__['eval'](code) — subscript-based access

These are acceptable since the transform is a reward shaping heuristic, not a security boundary (container isolation provides actual sandboxing per INVARIANTS.md). However, the class docstring currently says it "evaluates code safety" which may mislead contributors into treating it as a security control. Consider adding a note: "This is a reward heuristic, not a security sandbox."

Tier 2: Alignment

ALIGNMENT FLAG: reset() should mirror full base signature

• Principle: INVARIANTS.md §API Invariant 1 — Gymnasium API signatures must not change without a major version bump
• Concern: Omitting **kwargs from the override breaks forward-compatibility. All Environment subclasses should mirror the full base class signature.

What looks good

• AST-based detection is a significant improvement — eliminates false positives on string literals and user-defined functions
• New test file covers both true positives and false negative cases thoroughly
• Lazy import pattern in __init__.py is clean and well-motivated
• episode_id override support aligns with the Gym-like API contract

---

Automated review by Claude Code | Learn more

[abhinavgautam01]

@greptile review

[abhinavgautam01]

@greptile review

[abhinavgautam01]

@greptile review

[abhinavgautam01]

ping @Darktex

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Thanks for the improvement — the AST-based safety detection is a real fix for false-positive reward penalties, and the reset() signature alignment is needed. There are two blocking issues to address before merge.

Automated Checks

• Lint (existing files): PASS — ruff format/check clean for envs/coding_env/server/
• Debug code: CLEAN — no debug artifacts in changed files
• Note: a pre-existing lint issue in python_executor.py:152 (unused variable e) is unrelated to this PR

---

Tier 1: Fixes Required

[BLOCKING] envs/coding_env/server/app.py — duplicate if __name__ == "__main__": guard still present

The PR description says it "removes duplicate server entrypoint blocks," but the post-patch file has two __main__ guards:

# first guard (pre-existing)
if __name__ == "__main__":
    import uvicorn
    uvicorn.run(app, host="0.0.0.0", port=8000)

def main():
    ...

# second guard — newly added by this PR
if __name__ == "__main__":
    main()

Python runs the first guard and ignores the second, so the new main()-based block is dead code — the PR actually made the duplication slightly worse. Keep a single canonical guard: remove the inline uvicorn.run block and keep only the main() function and one guard that calls it.

[BLOCKING] envs/coding_env/server/python_codeact_env.py — step() does not match the base-class signature

The abstract base (interfaces.py) declares:

@abstractmethod
def step(self, action: ActT, timeout_s: Optional[float] = None, **kwargs: Any) -> ObsT:

reset() was correctly aligned, but step() is left as def step(self, action: Action) -> Observation:. If the framework calls step(action, timeout_s=...), Python raises TypeError: step() got an unexpected keyword argument 'timeout_s'. Mirror the reset() fix:

def step(
    self,
    action: Action,
    timeout_s: Optional[float] = None,
    **kwargs: Any,
) -> Observation:
    del timeout_s, kwargs
    ...

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG: AST safety detection silently returns None on SyntaxError.

• Principle at stake: rewards inside environment (RFC 002) — reward computation must be deterministic and explainable.
• The concern: when code fails to parse, _detect_violation returns None, so CodeSafetyTransform contributes 0.0 instead of the -1.0 penalty. Because transforms in CompositeTransform add, syntactically-broken code containing import os nets 0.0 + (-0.2) = -0.2, whereas valid code with import os nets -1.0. That asymmetry could teach an agent that wrapping a violation in a syntax error is preferable. Worth a deliberate, documented decision.
• Suggested reviewer: a human maintainer.

ALIGNMENT FLAG: __init__.py lazy __getattr__ import changes module semantics.

• Principle at stake: stable module interfaces (INVARIANTS.md).
• The concern: replacing the eager top-level import with __getattr__ means coding_env.server.__dict__ won't contain PythonCodeActEnv until first access — dir(module) won't list it and introspection tools may be surprised. The motivation (avoid pulling smolagents transitively) is valid, but a try/except ImportError at the top of python_codeact_env.py is the more conventional way to make an optional dependency optional.
• Suggested reviewer: a human maintainer.

---

Additional Notes (Non-Blocking)

• test_python_codeact_reset.py: no test exercises reset(seed=42) (the newly accepted seed param) — add one to prove it doesn't raise.
• test_python_codeact_reset.py: no test calls step(action, timeout_s=30.0) — add once the signature is fixed.
• test_coding_safety_transform.py: no explicit test for import os as foo (the new AST detection does correctly block this — ast.walk sees the Import node regardless of asname — but it was a known regex gap worth pinning).
• getattr(__builtins__, 'open')(...)-style bypass is not caught; the code comment acknowledges this is a reward heuristic, not a sandbox — informational only.

Summary

• 2 mechanical issues to fix (duplicate __main__ in app.py; missing timeout_s/**kwargs on step()).
• 2 alignment points for human review (silent None on SyntaxError for the safety reward; the lazy __getattr__ import pattern).

---

Automated review by Claude Code | Learn more

[abhinavgautam01]

@greptile review

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Summary

The intent (API-compat fixes + AST-based safety detection to remove false positives) is good and the reward logic stays correctly encapsulated server-side. However, the AST rewrite introduces two genuine detection gaps that weaken the safety reward, plus a couple of correctness/robustness issues. Recommending changes before merge.

---

Automated Checks

• Lint (ruff format, changed source files): PASS.
• Import sort: PASS.
• Debug code: CLEAN.

---

Tier 1: Fixes Required

• [High] AST detection regresses open() (and other) calls made as attribute methods. transforms.py _detect_violation only flags calls where node.func is an ast.Name. This correctly fixes false positives like print("import os")/myopen() and intentionally stops flagging exec/eval — but it also silently stops detecting file opens via attribute calls: Path("f.txt").open(), io.open(...), cursor.exec(...) all have node.func as ast.Attribute and are no longer flagged. The old open\( regex caught these. Decide explicitly: also inspect ast.Attribute calls for open, or document that only bare-name open(...) is penalized.
• [High] Syntax-error evasion vector. When ast.parse raises (SyntaxError/RecursionError/ValueError), _detect_violation returns None and applies no safety penalty. The old regex worked on raw text and would still catch import os in syntactically invalid code. An agent submitting dangerous-but-malformed code now receives only CodeQualityTransform's syntax penalty (e.g. -0.2) instead of the full safety penalty (-1.0). At minimum document this trade-off explicitly; ideally fall back to the regex scan when parsing fails.
• [Bug] episode_id="" preserved as a valid episode ID. python_codeact_env.py: episode_id=episode_id if episode_id is not None else str(uuid.uuid4()) preserves an empty string (and test_reset_preserves_empty_episode_id_override asserts this). An empty-string ID is not meaningful and can cause silent downstream issues. Use a truthy check (if episode_id:) instead of is not None.
• [Bug] __init__.py lazy-import pattern is fragile. __all__ = ["PythonCodeActEnv"] is declared at module level, but the name is only bound via __getattr__. Static analysis/type checkers will flag PythonCodeActEnv as unresolvable, and the pattern is non-obvious. Prefer keeping from .python_codeact_env import PythonCodeActEnv at module level (the original), or use a TYPE_CHECKING guard if the goal is deferring an optional-dependency import.
• [Minor] PR claims duplicate __main__ blocks were removed, but the diff only reorders imports. app.py still has two if __name__ == "__main__" blocks (the first calling uvicorn.run(app, ...) is effectively dead code). Either remove it as the description states, or drop the claim.
• [Minor] test_quality_transform_handles_ast_recursion_error monkeypatches ast.parse module-wide. Patching coding_env.server.transforms.ast.parse affects both CodeQualityTransform and CodeSafetyTransform in that module. The test targets quality only; the broader scope could mask cross-test interactions. Tighten the patch scope.

---

Tier 2: Alignment Discussion

None blocking. Reward/safety computation remains inside the environment's server-side Transform pipeline ("Rewards inside environment" — RFC 002, PRINCIPLES.md). The reset/step signatures are now properly aligned with the base Environment ABC contract. __init__.py changes do not expose server code to clients, and no MCP/agent-facing interfaces change.

Verdict: request_changes — the two safety-detection gaps (attribute-call open, syntax-error bypass) meaningfully weaken the safety reward and should be closed or explicitly documented; the episode_id and __init__ issues are quick fixes.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint (ruff): PASS — no violations in PR-touched source files.
• Import sort (usort): The app.py change moves from openenv.core.env_server import create_app above the local imports — this is what usort requires. PASS.
• Debug code: CLEAN — no debug artifacts in changed files.

---

Tier 1: Findings

On the "removes duplicate server entrypoint execution blocks" claim — verified, no action needed.

I checked envs/coding_env/server/app.py at this PR's head commit (0458ee2). It contains exactly one if __name__ == "__main__": block (calling main() → uvicorn.run). There is no duplicate entrypoint block in the PR. The app.py diff itself only reorders imports, and the __init__.py change is a switch to lazy imports (__getattr__) so transforms stays importable without optional deps like smolagents. No blocking bug here.

OBSERVATION (non-blocking): AST safety detector has known edge-case gaps (envs/coding_env/server/transforms.py)

Snippets that invoke dangerous calls without a preceding import statement (e.g. subprocess.run([...]) where subprocess is injected externally, or __builtins__['eval'](...)) are not caught by the new AST detector. These edge cases were also not caught by the old regex detector, so there is no regression, and the class docstring correctly frames this as "a reward heuristic, not a security sandbox". No change required — noted for reviewer awareness.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG: reset() now accepts an episode_id override from callers

• Principle at stake: "Agents cannot reset" (INVARIANTS.md §Security/Architectural Invariants; PRINCIPLES.md).
• Concern: The signature change aligns PythonCodeActEnv.reset() with the base Environment abstract class (which already declares seed/episode_id), so it is correct for API compatibility. The orchestration/WebSocket layer calls reset(), not agents — so this is likely fine. The open question for humans: if episode_id were ever forwarded from an agent-visible request (e.g. a future MCP tool), it would let agents influence episode identity. Worth confirming the parameter is only populated by infrastructure-layer callers.

---

Summary

• 0 blocking Tier 1 bugs. The "duplicate __main__ block" concern was verified against the PR head commit and is not present — app.py has a single entrypoint block.
• 1 informational observation: AST detector has pre-existing edge-case gaps (not a regression; correctly framed as a heuristic).
• 1 Tier 2 alignment flag: episode_id parameter surface, for human review.

The signature alignment with the base class, the lazy-import __init__.py, the regex→AST migration, and the added tests are all correct and well-implemented. Verdict downgraded to comment — no blocking issues.

---

Automated review by Claude Code | Learn more

[abhinavgautam01]

@Darktex thanks for checking. Confirmed there are no blocking issues left from this review. The episode_id point is an alignment note only: reset(seed, episode_id, **kwargs) mirrors the base Environment API and is intended for infrastructure/orchestration callers, not agent-facing MCP tools. The AST detector edge cases are acknowledged as non-regressions and the transform is documented as a reward heuristic rather than a security sandbox...

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint (ruff check + ruff format): PASS on all modified source files
• Import sort (usort): the PR's app.py change corrects a pre-existing import-order violation; the fix is correct
• Debug code: CLEAN — no debug artifacts introduced

---

Tier 1: Fixes Required

• tests/envs/test_coding_safety_transform.py — New tests will hard-fail (not skip) on import in environments without smolagents (blocking). The new file imports from coding_env.server.transforms import ..., which transitively imports python_codeact_env → python_executor → smolagents. The existing test_python_codeact_reset.py:21 guards this with pytest.importorskip("smolagents", ...). Without the same guard, this test file will error at collection time in CI environments that don't have smolagents installed. Add the importorskip guard.

• envs/coding_env/server/app.py:33-36 — Duplicate __main__ block not removed despite PR claim. The PR description states it "removes duplicate server entrypoint execution blocks," but the diff only reorders imports. The first if __name__ == "__main__": block (lines 33–36) is permanently dead code because Python evaluates only the last if __name__ == "__main__": block (line 46). Delete the first block to match the stated goal.

• tests/envs/test_coding_safety_transform.py:261-262 — Import-style inconsistency (minor). Uses from coding_env.models import ... (no envs. prefix) while test_python_codeact_reset.py uses from envs.coding_env.models import .... Both work under PYTHONPATH=src:envs; worth aligning for maintainability.

---

Tier 2: Alignment Discussion

None identified. The changes are well-aligned with OpenEnv design principles:

• The AST-based CodeSafetyTransform keeps reward computation inside the environment boundary ("Rewards inside environment", RFC 002).
• The lazy-import pattern in server/__init__.py matches the established pattern in envs/finqa_env/server/__init__.py.
• The reset(seed, episode_id, **kwargs) and step(action, timeout_s, **kwargs) signatures match the abstract base class (src/openenv/core/env_server/interfaces.py:163-169) and the echo_env reference — backward-compatible (optional params with defaults + **kwargs).
• No client-server separation violations; no simulation controls exposed via MCP to agents.

Notes on the core fix

The AST-based false-positive fix in transforms.py is technically sound: it correctly avoids flagging print("import os") or myopen() by inspecting the AST rather than raw text, catches from os import path and import os as alias, and falls back to regex on parse failure (appropriate and tested). The one inherent limitation (aliased calls like my_open = open; my_open(...)) is acceptable for a reward heuristic — not a security boundary — and the updated docstring documents it.

---

Summary

• 3 mechanical issues (1 blocking: missing smolagents importorskip guard; 1 incomplete-claim dead-code removal; 1 minor import style)
• 0 alignment points for human review

The substantive fixes are correct; the blockers are test-hygiene and an unfinished cleanup the PR description claims to have done.

---

Automated review by Claude Code | Learn more

[abhinavgautam01]

@Darktex i checked these against the PR head and i don’t think code changes are needed here...

test_coding_safety_transform.py does not require smolagents: coding_env.server.__init__ is lazy and importing coding_env.server.transforms does not import python_codeact_env or python_executor. I verified this in an environment where smolagents is absent and the transform test file collects and passes.

app.py also has a single if __name__ == "__main__": guard at the current PR head.

For the import style, the new transform test intentionally uses the standard PYTHONPATH=src:envs package import and avoids the local sys.path mutation pattern that earlier review feedback asked us to remove...

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: Cannot run in review mode — import order change in app.py is suspicious (see Tier 1)
• Debug code: CLEAN — no print, breakpoint, or TODO artifacts in changed files

Open RFCs Context

No open RFC governs this change. RFC 002 (env-spec) defines reset()/step() signatures; the PR's API-compatibility additions match the base Environment interface in src/openenv/core/env_server/interfaces.py exactly (seed, episode_id, **kwargs for reset; timeout_s, **kwargs for step). No RFC is required for a bug fix of this nature, and the author correctly checked "Not required."

Tier 1: Fixes Required

• envs/coding_env/server/app.py — Duplicate if __name__ == "__main__" block. The existing file already has this defect (a bare block runs uvicorn.run(app, ...) directly, then a later block defines main() and repeats it). The PR preserves it. Only the second block (calling main()) should exist; the first bare block is dead code. Fix: remove the first bare if __name__ == "__main__" block.

• envs/coding_env/server/app.py — Import ordering likely violates usort/ruff rules. The PR moves from openenv.core.env_server import create_app above the coding_env local-env imports. usort treats these as different import groups and will reorder them. Run uv run usort format envs/coding_env/server/app.py and commit the result. (Note: lint.sh only runs usort on src/ and tests/, not envs/, so this won't be auto-caught — but ruff check still applies.)

• envs/coding_env/server/transforms.py — _parse_code is a test-seam antipattern. The function is a one-line wrapper around ast.parse whose sole purpose is to allow monkeypatch.setattr in test_quality_transform_handles_ast_recursion_error. Exposing a module-level symbol purely for test-patching leaks test concerns into production code. Prefer patching ast.parse directly in the test (unittest.mock.patch) or inject the parser as a constructor argument, and remove the wrapper.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: Attribute .open() check introduces new false positives in reward logic

• Principle at stake: "Rewards inside environment" (RFC 002, PRINCIPLES.md) — reward signals must be correct and domain-meaningful; reward false positives degrade training signal.
• The concern: envs/coding_env/server/transforms.py (diff ~line 207): if isinstance(node.func, ast.Attribute) and node.func.attr == "open" penalises any call whose method is named open — zipfile.ZipFile.open(), tarfile.open(), pathlib.Path.open(), io.open(), etc. The built-in open() is already caught by the ast.Name branch; the .attr == "open" branch adds coverage beyond what the regex caught and re-introduces false positives for a whole new class of safe calls — directly counter to the PR's stated goal of reducing false positives. There is no test asserting that e.g. zipfile.ZipFile("f.zip").open("entry") is NOT penalised.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: timeout_s silently ignored with no observability

• Principle at stake: "Minimize lifecycle deltas" (PRINCIPLES.md) — orchestration code that passes timeout_s expecting bounded execution gets no signal that the timeout was not honored.
• The concern: envs/coding_env/server/python_codeact_env.py:125 does del timeout_s, kwargs after accepting the parameter. The docstring notes PyExecutor does not expose per-call timeout control — fine as a transitional note — but there is no warnings.warn(...) or log entry. An orchestrator passing timeout_s=5.0 to guard against runaway code silently gets no protection, and the observation after 60s looks identical to one after 0.1s. This is a training-production safety concern.
• Suggested reviewer: @Darktex

Summary

• 3 mechanical issues to fix (duplicate __main__ block, import ordering, test-seam antipattern)
• 2 alignment points for human review (attribute open() false positives in reward logic; silent timeout_s drop)

The core AST-based detection for import os/subprocess, eval, exec, __import__, and direct open() calls is a genuine improvement over the regex approach. The reset/step API-compatibility signatures exactly match the base Environment interface, and the new tests cover key false-positive cases. The issues above are addressable without a redesign.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: PASS (import reorder in app.py aligns with usort conventions; no ruff violations in the diff)
• Debug code: CLEAN (no debug artifacts in the changed files)

---

Tier 1: Fixes Required

• envs/coding_env/server/app.py — Duplicate if __name__ == "__main__": block NOT removed

  The existing app.py has two if __name__ == "__main__": blocks. The diff only reorders imports but leaves both guards in place. The PR summary claims it "removes duplicate server entrypoint blocks" — this appears factually incorrect; the duplicate remains. The fix is to keep only one guard — conventionally the main() / if __name__ == "__main__": main() pattern. Please remove the redundant uvicorn.run(app, ...) guard.

• envs/coding_env/server/transforms.py — .open( missing from fallback patterns (consistency bug)

  The AST check isinstance(node.func, ast.Attribute) and node.func.attr == "open" catches Path.open(), io.open(), etc. But there is no corresponding .open( pattern in _fallback_patterns — only \bopen\s*\( (bare call). So when AST parsing fails and the code falls back to regex, Path('f').open() will not be penalized, violating the docstring's stated intent ("fall back to the previous raw-text heuristic"). Fix: add (re.compile(r"\.open\s*\("), "open") to _fallback_patterns to keep AST and fallback paths consistent.

• envs/coding_env/server/transforms.py — _parse_code is test-induced indirection

  _parse_code wraps ast.parse with no added logic; its only purpose is to be monkeypatched in test_quality_transform_handles_ast_recursion_error. Monkeypatching a thin production wrapper as a test seam is a design smell that sets a precedent. Prefer injecting a _parse_fn into CodeQualityTransform.__init__ for testability, or exercise the fallback via genuinely malformed/recursive input.

• tests/envs/test_python_codeact_reset.py — empty-episode_id behavior is silent and undocumented

  test_reset_replaces_empty_episode_id_override relies on episode_id or str(uuid.uuid4()) short-circuiting on "", so a caller passing episode_id="" silently gets a random UUID instead of an empty ID. Either document this in the reset() docstring or raise ValueError on empty string so callers get a clear error rather than silent ID generation.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG: step() signature deviates from the Gymnasium API invariant

• Principle at stake: INVARIANTS.md §API Invariants — step(action) -> Observation is the canonical signature.
• The concern: The PR adds timeout_s: Optional[float] = None and **kwargs: Any to step(). INVARIANTS.md enumerates the Gym-like API as step(action) -> Observation with no additional params. timeout_s is described as "accepted for API compatibility" but is silently discarded (del timeout_s). If the base Environment interface does not declare timeout_s, this is a unilateral extension at the concrete-class level rather than real "API compatibility." Better: update the base Environment interface if timeout_s is part of the intended contract, or drop it.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: safety reward heuristic now penalizes Path.open() and any .open() method

• Principle at stake: PRINCIPLES.md — "Rewards inside environment"; reward shaping must match task intent.
• The concern: The old regex caught bare open(...); the new AST check expands the blocked set to any .open() attribute call (e.g., Path('f.txt').open()), so any task involving pathlib file I/O receives a -1.0 penalty. The PR correctly notes this is "a reward heuristic, not a security sandbox," but expanding the blocked set is a conscious reward-design decision about the task's intended scope and should be made deliberately, not as a side effect of the regex→AST migration.
• Suggested reviewer: @Darktex

---

Summary

• 4 mechanical issues to fix (1 bug surviving from pre-existing code — the duplicate entrypoint the PR claims to remove; 1 AST/fallback inconsistency; 1 test-seam design smell; 1 undocumented empty-id behavior)
• 2 alignment points for human review (step-signature deviation from the invariant; implicit reward-scope expansion)

The move from regex to AST-based safety detection is the right direction and the test additions are welcome — the fixes above are about closing the AST/fallback gap and reconciling the PR description with what the diff actually changes.

---

Automated review by Claude Code | Learn more