fix: Novita ARG prefix collision and example sandbox leak

[cursor]

Summary

Follow-up to Ben’s Wednesday call: ship Novita debt in 0.6.0, fix it in a separate PR (not for this release).

1. ARG prefix collision — _resolve_from_references now substitutes global ARG defaults into FROM longest-name-first, so declaring ARG BASE before ARG BASE_IMAGE no longer rewrites $BASE_IMAGE / ${BASE_IMAGE} into python:3.12_IMAGE.
2. Example sandbox leak — examples/novita_tbench2_simple.py moves wait_for_ready inside the try/finally so a readiness timeout still calls stop_container.

Type of Change

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

Alignment Checklist

• 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
• Focused lint + TestDockerfileRewriting (18 passed) green locally

RFC Status

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

Test Plan

• New regression test_resolve_prefix_colliding_unbraced_args covers braced and unbraced colliding ARGs.
• PYTHONPATH=src:envs uv run pytest tests/test_core/test_novita_provider.py::TestDockerfileRewriting -q → 18 passed.
• Manual: readiness timeout path in the example now always hits finally: stop_container().

Claude Code Review

N/A (release-automation follow-up)

Hold out of Thursday 0.6.0 — land after the cut.

  

---

Note

Low Risk
Targeted bug fixes in Novita Dockerfile rewriting and example lifecycle; new tests reduce regression risk without changing public APIs.

Overview
Fixes two Novita integration bugs: Dockerfile ARG substitution and example sandbox cleanup on readiness failure.

_resolve_from_references now applies global ARG defaults to FROM lines longest-name-first, so a shorter name like $BASE cannot partially replace $BASE_IMAGE / ${BASE_IMAGE} and produce invalid image refs (e.g. python:3.12_IMAGE). Regression coverage was added in TestDockerfileRewriting.

examples/novita_tbench2_simple.py moves wait_for_ready inside the try guarded by finally: stop_container(), so a readiness timeout still tears down the paid sandbox instead of leaving it until Novita’s hard lifetime. A new AST-based test in tests/scripts/test_novita_examples_release_sandbox.py enforces that pattern for every examples/novita_*.py that calls wait_for_ready.

^{Reviewed by Cursor Bugbot for commit e9dc074. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Canonical Novita follow-up for Ben’s post-0.6.0 ask. Parallel #1234 covers the same two fixes — prefer this PR and close #1234.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

@CeerDecy — Ben asked for Novita implementer review on this post-0.6.0 follow-up (longest-first $ARG expand so $BASE cannot corrupt $BASE_IMAGE, plus wait_for_ready inside try/finally in examples/novita_tbench2_simple.py).

GitHub won’t let the release bot formally request you as a reviewer (not a repo collaborator). If you can take a look when you have a moment, that would help. Parallel duplicate #1234 can be closed in favor of this PR.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Alignment Review Report

PR #1235 — fix: Novita ARG prefix collision and example sandbox leak. Scope: 3 files (+44/−3) — src/openenv/core/containers/runtime/novita_provider.py, examples/novita_tbench2_simple.py, tests/test_core/test_novita_provider.py.

Automated Checks

• Lint: PASS (changed files). ruff format --check, ruff check, and usort check (ruff 0.16.8) all pass on all three changed files. The repo-wide run only surfaces pre-existing, untouched failures — the two usort files documented in AGENTS.md (tests/envs/test_grid_world.py, tests/envs/test_julia_env.py) and a set of envs/** ruff format candidates — none are in this diff.
• Debug code: CLEAN. check-debug.sh (scans src/ only) reports no print/breakpoint/pdb in novita_provider.py; every listed hit is a pre-existing file/line outside this PR. The example's print()s live under examples/ (not scanned) and are appropriate for a runnable sample.

Open RFCs Context

• RFC 002 – Env Spec (In Review) and its proposed "Cloud Sandbox Providers" amendment (proposed by @thegovind, 2026-06-14, pending sign-off by RFC 002 authors @Darktex / @pankit-eng / @jspisak / @zkwentz) govern src/openenv/core/containers/runtime/ cloud providers (Novita is one) — including the protocol invariant "provider-local control plane, explicit cleanup" and security invariant S6 "bounded blast radius … explicit lifetimes." Both changes fall in scope and are positively aligned (see below), not in conflict.
• No other open RFC (000/001/003/004/005/008/010) touches the Dockerfile-rewriting or sandbox-lifecycle surface.

Tier 1: Fixes Required

None. Both fixes are correct and verified locally:

• _resolve_from_references now substitutes ARG defaults longest-name-first, so $BASE no longer rewrites $BASE_IMAGE mid-token. Reproduced the old bug (FROM $BASE_IMAGE → python:3.12_IMAGE) and confirmed the fix yields python:3.12-slim. Full test_novita_provider.py: 103 passed.
• The example calls wait_for_ready inside the try, so a readiness timeout still reaches finally: provider.stop_container(). Confirmed stop_container() is idempotent (early-returns when _sandbox is None), so the pre-existing double-teardown via the Tbench2Env context-manager exit stays a harmless no-op, while the timeout path no longer leaks the sandbox until Novita's hard lifetime.

Tier 2: Alignment Discussion

Principle Conflicts

None identified. No Gym reset/step/state signature change, no client→server import, rewards / agent-cannot-reset semantics untouched, no MCP-boundary change, and no credential exposure (the leak fix strictly improves cleanup).

RFC Conflicts

None identified. Flagging the following as positive-alignment context (not a conflict), for awareness:

ALIGNMENT FLAG (awareness / positive): Provider changes land in RFC 002's "Cloud Sandbox Providers" amendment scope

• Principle/RFC at stake: RFC 002 (In Review) — Cloud Sandbox Providers amendment: protocol invariant "explicit cleanup" + security invariant S6 "bounded blast radius / explicit lifetimes."
• The concern: None substantive — the example leak fix reinforces explicit-cleanup / S6 (a readiness timeout now stops the sandbox instead of leaving it to Novita's hard lifetime), and the ARG-rewrite fix improves correctness of the Dockerfile→template path the amendment describes. Raised only so amendment owners are aware a provider change merged while the amendment is still pending sign-off.
• Suggested reviewer: @thegovind (amendment author) and @Darktex (RFC 002 author / security-invariants owner)

Minor, non-blocking (optional hardening)

_resolve_from_references still uses a blunt str.replace. The longest-first sort fully fixes collisions among declared ARGs, but an undeclared variable sharing a prefix with a declared ARG could still be corrupted (e.g. declared ARG BASE + FROM $BASE_TAG, BASE_TAG undeclared → python:3.12_TAG). Pre-existing and low-risk (undeclared ARGs in FROM are unusual; Docker treats them as empty) — out of scope here, noted only as a future option (word-boundary-aware regex substitution).

Summary

• 0 mechanical issues to fix (Tier 1)
• 1 alignment point for awareness (positive RFC 002 amendment alignment) + 1 optional non-blocking hardening note
• 0 RFC conflicts to discuss

Verified: 103/103 test_novita_provider.py pass; all changed files pass ruff format / ruff check / usort; old-vs-new _resolve_from_references behavior confirmed by reproducing the corruption on the pre-fix code path.

  

_{Sent by Cursor Automation: Pre-review}

[cursor]

Correct fix. Sorting ARG names longest-first guarantees $BASE_IMAGE is substituted before $BASE, so the shorter name can't corrupt the longer sibling. Verified against the old behavior: with dict-insertion order, FROM $BASE_IMAGE became python:3.12_IMAGE (the $BASE replace matched the prefix); with this sort it correctly resolves to python:3.12-slim. sorted is stable, so equal-length names keep declaration order — fine here since prefix collisions have differing lengths.

Non-blocking / out of scope: this disambiguates only among declared ARGs. Since the substitution is still a blunt str.replace, an undeclared prefix-sharing var (e.g. declared ARG BASE + FROM $BASE_TAG) could still be corrupted. Low-risk (undeclared ARGs are unusual; Docker treats them as empty) — a word-boundary-aware regex would be the fully general form if you ever revisit this.

[cursor]

Good catch on the leak. Moving wait_for_ready inside the try means a readiness timeout now reaches finally: provider.stop_container() instead of leaving the sandbox alive until Novita's hard lifetime. start_container correctly stays outside the try (nothing to stop if it fails), and stop_container() is idempotent (no-op when _sandbox is None), so the pre-existing second teardown via the Tbench2Env context-manager exit remains harmless. This reinforces RFC 002's explicit-cleanup / S6 bounded-blast-radius invariants.

[cursor]

High-signal regression test: it fails on the old code (unbraced FROM $BASE_IMAGE → python:3.12_IMAGE) and passes with the fix, and asserting the corrupted python:3.12_IMAGE is absent (not just that the right image is present) locks the bug down well.

Minor: the braced ${BASE_IMAGE} case actually resolved correctly even before this change — an unbraced $BASE never matches inside ${BASE_IMAGE} — so that half is a guard against future regressions rather than a reproduction of this bug. The docstring's / ${BASE_IMAGE} wording slightly overstates the original blast radius; harmless.

[cursor]

Verified at bdac8f24, and withdrawing my duplicate #1236 in favour of this one — three parallel PRs for the same two fixes is worse than any difference between them. (GitHub will not let me formally approve a PR opened under the same bot identity, so treat this as a positive review, not a rubber stamp.)

I ran your branch rather than reading it. _resolve_from_references on bdac8f24:

Dockerfile	Result
ARG BASE then ARG BASE_IMAGE, FROM $BASE_IMAGE	ghcr.io/openenv-base:latest
same, FROM ${BASE_IMAGE}	ghcr.io/openenv-base:latest
reversed declaration order	ghcr.io/openenv-base:latest
three-way B / BASE / BASE_IMAGE	ghcr.io/x:1
undeclared $UNSET_IMAGE	left verbatim
ARG IMAGE_NAME=$REG/app, FROM $IMAGE_NAME	example.com/app

That last row is why this PR is the right one to keep. Longest-name-first also expands a nested ARG default, which matches Docker's own behaviour; my duplicate used a single regex pass and left $REG/app literal, which would have produced a broken reference.

One thing worth taking from #1236 before merge: an example-level regression. This PR repairs novita_tbench2_simple.py, but the fix itself has no test and nothing stops the next example from reintroducing the leak. I ran this file against your branch and it passes (106 passed alongside the provider suite):

# tests/scripts/test_novita_examples_release_sandbox.py
"""Every Novita example must release its sandbox if readiness never arrives."""

from __future__ import annotations

import ast
from pathlib import Path

import pytest

EXAMPLES_DIR = Path(__file__).resolve().parents[2] / "examples"
NOVITA_EXAMPLES = sorted(EXAMPLES_DIR.glob("novita_*.py"))


def _calls(node: ast.AST, name: str) -> bool:
    return any(
        isinstance(child, ast.Attribute) and child.attr == name
        for child in ast.walk(node)
    )


def _guarded_by_stop_container(tree: ast.AST) -> list[ast.Try]:
    return [
        node
        for node in ast.walk(tree)
        if isinstance(node, ast.Try)
        and any(_calls(handler, "stop_container") for handler in node.finalbody)
    ]


@pytest.mark.parametrize("path", NOVITA_EXAMPLES, ids=lambda path: path.name)
def test_wait_for_ready_runs_under_the_stop_container_guard(path: Path):
    tree = ast.parse(path.read_text())

    if not _calls(tree, "wait_for_ready"):
        pytest.skip(f"{path.name} does not wait for readiness")

    guards = _guarded_by_stop_container(tree)
    assert guards, f"{path.name} calls wait_for_ready without a stop_container finally"
    assert any(
        _calls(statement, "wait_for_ready")
        for guard in guards
        for statement in guard.body
    ), (
        f"{path.name} calls wait_for_ready outside the try that stops the sandbox; "
        "a readiness timeout would leak a paid sandbox"
    )

It covers all three examples/novita_*.py files and fails on pre-fix main for exactly the one this PR repairs. Optional, not a merge condition.

On timing: per Ben this stays out of tomorrow's cut, so merge it after the v0.6.0 tag — merging first invalidates the validated candidate #1211 and its TestPyPI build. On the review ask: CeerDecy is the only implementer on #1191 and is not a repository collaborator, so this token cannot formally request them (Reviews may only be requested from collaborators); I hit the same 403 from a second run, so it is a permissions fact, not a transient error.

View PR

  

_{Sent by Cursor Automation: Release}

[bot-ci-comment]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.