feat(evaluator-sdk): sandboxed containerized Fabric runner (AALGO-321)#604
Open
SandyChapman wants to merge 1 commit into
Open
feat(evaluator-sdk): sandboxed containerized Fabric runner (AALGO-321)#604SandyChapman wants to merge 1 commit into
SandyChapman wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughAdds sandbox abstractions and a Docker provider, shared Fabric runtime helpers, Fabric image provisioning, a containerized Fabric runtime with an end-to-end example, and relay dependency/license metadata updates. ChangesFabric Container Runtime
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py (1)
120-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
import tempfileto module top-level.Local import inside
_write_fileis unconventional; hoist to the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py` around lines 120 - 127, The `_write_file` helper currently does a local `import tempfile`, which should be hoisted to the module top level for consistency. Move the import alongside the other top-level imports in this sandbox API module and remove the inline import from `_write_file`, keeping the rest of the function behavior unchanged.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py (2)
92-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo handling for a missing
dockerbinary.
_runonly catches timeouts; ifdocker_binisn't found,asyncio.create_subprocess_execraises a rawFileNotFoundErrorthat propagates uncaught through every operation (create,exec,status,close) with no actionable message.🛡️ Proposed fix
- proc = await asyncio.create_subprocess_exec( - *argv, - stdin=asyncio.subprocess.PIPE if stdin is not None else None, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - start_new_session=True, - ) + try: + proc = await asyncio.create_subprocess_exec( + *argv, + stdin=asyncio.subprocess.PIPE if stdin is not None else None, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + start_new_session=True, + ) + except FileNotFoundError as exc: + raise RuntimeError(f"docker binary not found: {argv[0]!r}") from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py` around lines 92 - 121, The _run chokepoint in DockerProvider only handles timeouts, so a missing docker binary still bubbles up as a raw FileNotFoundError through create, exec, status, and close. Update _run to catch the create_subprocess_exec failure for the docker executable and re-raise a clearer, actionable error that names the missing binary and the argv/context, while preserving the existing timeout handling and return semantics for successful runs.
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
_KEEP_ALIVE_COMMANDconstant duplicatescreate()'s hardcoded argv.
create()hardcodes["sh", "-c", "exec sleep infinity"]directly instead of using this constant, so they can silently drift.♻️ Consolidate
-argv += [spec.image, "sh", "-c", "exec sleep infinity"] +argv += [spec.image, *shlex.split(_KEEP_ALIVE_COMMAND)]Also applies to: 137-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py` at line 45, The keep-alive command is duplicated between the _KEEP_ALIVE_COMMAND constant and Docker provider create(), which can drift over time. Update the create() path in the Docker provider to use _KEEP_ALIVE_COMMAND instead of hardcoding the argv, and keep all keep-alive command construction centralized so the constant is the single source of truth.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md (1)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMixes explanation and reference content on one page.
Rationale ("Why an owned seam") and reference material (contract/file listing, test listing) sit on the same page. Per coding guidelines, each doc page should fit one Diataxis quadrant with cross-links used instead of mixing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md` around lines 1 - 45, The Sandbox README currently mixes explanatory rationale with reference material, so split it to fit a single Diataxis quadrant. Refactor the content in the Sandbox seam README so the overview and “Why an owned seam” narrative stay separate from the contract, isolation note, roadmap, and tests; move the reference-style file listings and test listings to a dedicated reference page or appendix, and add cross-links from this page to the new target. Keep the main README focused on the conceptual explanation of the sandbox seam and use the existing section titles like “The contract” and “Tests” to relocate or trim the reference details.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.py`:
- Around line 82-96: Add bounded timeouts to the Docker subprocess invocations
so they cannot hang indefinitely. In image_exists, wrap the subprocess.run call
with a short timeout and convert timeout expiry into FabricImageError with a
clear daemon-unreachable message; also apply a timeout to the docker build path
in the same fabric image runtime code so both the quick inspect check and build
step fail fast instead of blocking forever.
- Around line 72-158: The cached Fabric image tag only depends on the Dockerfile
and extras, so changes in the staged NeMo-Fabric source can be missed and a
stale image reused. Update fabric_image_tag() so its digest also incorporates
the relevant source contents from the staged fabric_repo (or the same build
inputs used by _stage_source()), and make ensure_fabric_image() compute the tag
after resolving the repo so local source edits produce a new tag and trigger a
rebuild without requiring force_build.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfile`:
- Around line 29-42: The runtime stage in sandbox.Dockerfile is still executing
as root because it never switches away from the default user. Update the runtime
setup after the existing COPY/RUN steps to create and use a dedicated non-root
user, and add a USER instruction so the final image runs sandboxed instead of as
root. Keep the change in the runtime stage near the existing ENV, WORKDIR, and
fabric version check so it is easy to locate alongside the image bootstrap
logic.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py`:
- Around line 120-127: The `_write_file` helper currently does a local `import
tempfile`, which should be hoisted to the module top level for consistency. Move
the import alongside the other top-level imports in this sandbox API module and
remove the inline import from `_write_file`, keeping the rest of the function
behavior unchanged.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py`:
- Around line 92-121: The _run chokepoint in DockerProvider only handles
timeouts, so a missing docker binary still bubbles up as a raw FileNotFoundError
through create, exec, status, and close. Update _run to catch the
create_subprocess_exec failure for the docker executable and re-raise a clearer,
actionable error that names the missing binary and the argv/context, while
preserving the existing timeout handling and return semantics for successful
runs.
- Line 45: The keep-alive command is duplicated between the _KEEP_ALIVE_COMMAND
constant and Docker provider create(), which can drift over time. Update the
create() path in the Docker provider to use _KEEP_ALIVE_COMMAND instead of
hardcoding the argv, and keep all keep-alive command construction centralized so
the constant is the single source of truth.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md`:
- Around line 1-45: The Sandbox README currently mixes explanatory rationale
with reference material, so split it to fit a single Diataxis quadrant. Refactor
the content in the Sandbox seam README so the overview and “Why an owned seam”
narrative stay separate from the contract, isolation note, roadmap, and tests;
move the reference-style file listings and test listings to a dedicated
reference page or appendix, and add cross-links from this page to the new
target. Keep the main README focused on the conceptual explanation of the
sandbox seam and use the existing section titles like “The contract” and “Tests”
to relocate or trim the reference details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4923bf8f-c0ef-495e-8637-c7626bafd2fe
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.py
Comment on lines
+72
to
+158
| def fabric_image_tag(*, repo: str = DEFAULT_FABRIC_IMAGE_REPO) -> str: | ||
| """Content-addressed tag for the harness-agnostic Fabric image: ``<repo>:<digest(recipe + extras)>``. | ||
|
|
||
| Not keyed by harness: one Fabric install + the bundled adapters runs any built-in harness, so the | ||
| image is the same regardless of which harness a task's config selects. | ||
| """ | ||
| recipe = _DOCKERFILE.read_bytes() + _extras_arg().encode("utf-8") | ||
| return f"{repo}:{hashlib.sha256(recipe).hexdigest()[:12]}" | ||
|
|
||
|
|
||
| def image_exists(tag: str, *, docker_bin: str = "docker") -> bool: | ||
| """Whether an image tag is present in the local Docker image store. | ||
|
|
||
| Raises :class:`FabricImageError` when the Docker daemon is unreachable, so a stopped/misconfigured | ||
| daemon surfaces as a clear error instead of masquerading as "image absent" and triggering a build | ||
| that then also fails confusingly. | ||
| """ | ||
| result = subprocess.run([docker_bin, "image", "inspect", tag], capture_output=True, check=False) | ||
| if result.returncode == 0: | ||
| return True | ||
| stderr = result.stderr.decode("utf-8", errors="replace") | ||
| if "cannot connect to the docker daemon" in stderr.lower(): | ||
| raise FabricImageError(f"cannot reach the Docker daemon (is it running?): {stderr.strip()}") | ||
| logger.debug("Fabric image %s not present in local store", tag) | ||
| return False | ||
|
|
||
|
|
||
| def _resolve_fabric_repo(fabric_repo: str | Path | None) -> Path: | ||
| default = Path(os.environ.get(FABRIC_REPO_ENV, _DEFAULT_FABRIC_REPO)) | ||
| repo = (Path(fabric_repo) if fabric_repo is not None else default).expanduser() | ||
| if not (repo / "pyproject.toml").is_file(): | ||
| raise FabricImageError( | ||
| f"NeMo-Fabric source not found at {repo}. The Fabric image is built from source " | ||
| f"(no public wheel); set {FABRIC_REPO_ENV} or pass fabric_repo to point at a checkout." | ||
| ) | ||
| return repo | ||
|
|
||
|
|
||
| def _stage_source(repo: Path, dest: Path) -> None: | ||
| """Copy only the maturin build inputs from ``repo`` into ``dest`` (not the whole checkout).""" | ||
| dest.mkdir(parents=True, exist_ok=True) | ||
| for name in _BUILD_SOURCE_PATHS: | ||
| src = repo / name | ||
| if src.is_dir(): | ||
| shutil.copytree(src, dest / name, ignore=shutil.ignore_patterns("target", "__pycache__", "*.whl")) | ||
| elif src.is_file(): | ||
| shutil.copy2(src, dest / name) | ||
| else: | ||
| raise FabricImageError(f"expected Fabric build input {name!r} not found under {repo}") | ||
|
|
||
|
|
||
| def ensure_fabric_image( | ||
| *, | ||
| fabric_repo: str | Path | None = None, | ||
| docker_bin: str = "docker", | ||
| force_build: bool = False, | ||
| ) -> str: | ||
| """Return a usable Fabric image tag, building it only if not already present. | ||
|
|
||
| One harness-agnostic image serves every built-in harness. Idempotent and content-addressed: an | ||
| unchanged recipe reuses the cached image; a changed recipe yields a new tag. Builds from a staged | ||
| copy of the local NeMo-Fabric source (build inputs only). | ||
| """ | ||
| tag = fabric_image_tag() | ||
| if not force_build and image_exists(tag, docker_bin=docker_bin): | ||
| logger.debug("Fabric image %s already present; skipping build.", tag) | ||
| return tag | ||
|
|
||
| repo = _resolve_fabric_repo(fabric_repo) | ||
| logger.info( | ||
| "Building Fabric image (first build compiles nemo-fabric; this can take minutes)...", | ||
| extra=dict(tag=tag, repo=repo), | ||
| ) | ||
| with tempfile.TemporaryDirectory(prefix="nemo-fabric-image-") as ctx_dir: | ||
| ctx = Path(ctx_dir) | ||
| _stage_source(repo, ctx / "nemo-fabric") | ||
| shutil.copy2(_DOCKERFILE, ctx / "Dockerfile") | ||
| try: | ||
| subprocess.run( | ||
| [docker_bin, "build", "--build-arg", f"EXTRAS={_extras_arg()}", "-t", tag, str(ctx)], | ||
| check=True, | ||
| env={**os.environ, "DOCKER_BUILDKIT": "1"}, | ||
| ) | ||
| except subprocess.CalledProcessError as exc: | ||
| raise FabricImageError(f"docker build failed for {tag}: {exc}") from exc | ||
| logger.info("Built Fabric image %s.", tag, extra=dict(tag=tag)) | ||
| return tag |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Tag doesn't bust cache when Fabric source changes.
fabric_image_tag() hashes only the Dockerfile bytes + extras (Line 78-79). ensure_fabric_image reuses any image matching that tag (Line 136-138) without hashing the staged fabric_repo contents (Line 147). A developer who edits their local NeMo-Fabric checkout without touching sandbox.Dockerfile will silently get a stale cached image on the next run — no rebuild happens unless force_build=True is passed explicitly.
♻️ Include the staged source in the tag digest
-def fabric_image_tag(*, repo: str = DEFAULT_FABRIC_IMAGE_REPO) -> str:
+def fabric_image_tag(*, repo: str = DEFAULT_FABRIC_IMAGE_REPO, fabric_repo: Path | None = None) -> str:
"""Content-addressed tag for the harness-agnostic Fabric image: ``<repo>:<digest(recipe + extras)>``."""
recipe = _DOCKERFILE.read_bytes() + _extras_arg().encode("utf-8")
+ if fabric_repo is not None:
+ recipe += _hash_source_tree(fabric_repo).encode("utf-8")
return f"{repo}:{hashlib.sha256(recipe).hexdigest()[:12]}"🧰 Tools
🪛 ast-grep (0.44.1)
[error] 88-88: Command coming from incoming request
Context: subprocess.run([docker_bin, "image", "inspect", tag], capture_output=True, check=False)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 149-153: Command coming from incoming request
Context: subprocess.run(
[docker_bin, "build", "--build-arg", f"EXTRAS={_extras_arg()}", "-t", tag, str(ctx)],
check=True,
env={**os.environ, "DOCKER_BUILDKIT": "1"},
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.py`
around lines 72 - 158, The cached Fabric image tag only depends on the
Dockerfile and extras, so changes in the staged NeMo-Fabric source can be missed
and a stale image reused. Update fabric_image_tag() so its digest also
incorporates the relevant source contents from the staged fabric_repo (or the
same build inputs used by _stage_source()), and make ensure_fabric_image()
compute the tag after resolving the repo so local source edits produce a new tag
and trigger a rebuild without requiring force_build.
Comment on lines
+29
to
+42
| FROM python:${PYTHON_VERSION}-slim-bookworm AS runtime | ||
| COPY --from=builder /opt/venv /opt/venv | ||
| COPY --from=builder /src/target/release/fabric /usr/local/bin/fabric | ||
| # The CLI binary resolves built-in adapters from its compile-time repository path | ||
| # (CARGO_MANIFEST_DIR/../../python/src/nemo_fabric/adapters); ship just that dir to the baked path so a | ||
| # wheel-only image can resolve them. Depends on NeMo-Fabric's installed-adapter-discovery layout | ||
| # (see image.py); swap to installing the top-level adapters/* packages once that lands on main. | ||
| COPY --from=builder /src/python/src/nemo_fabric/adapters /src/python/src/nemo_fabric/adapters | ||
| # The CLI's baked path is the literal `<fabric-core crate>/../../python/src/nemo_fabric/adapters`; the | ||
| # kernel needs `/src/crates/fabric-core` to exist to walk the `..`, even though nothing lives there. | ||
| RUN mkdir -p /src/crates/fabric-core | ||
| ENV PATH=/opt/venv/bin:$PATH | ||
| RUN python -c "from nemo_fabric import FabricClient" && fabric version | ||
| WORKDIR /out |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Final image runs as root.
No USER instruction in the runtime stage. This sandbox execs agent-generated code (fabric run), so running as root widens the blast radius of a container escape.
🔒️ Add a non-root user
ENV PATH=/opt/venv/bin:$PATH
RUN python -c "from nemo_fabric import FabricClient" && fabric version
-WORKDIR /out
+RUN useradd --create-home --uid 1000 sandbox
+WORKDIR /out
+RUN chown sandbox:sandbox /out
+USER sandbox📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM python:${PYTHON_VERSION}-slim-bookworm AS runtime | |
| COPY --from=builder /opt/venv /opt/venv | |
| COPY --from=builder /src/target/release/fabric /usr/local/bin/fabric | |
| # The CLI binary resolves built-in adapters from its compile-time repository path | |
| # (CARGO_MANIFEST_DIR/../../python/src/nemo_fabric/adapters); ship just that dir to the baked path so a | |
| # wheel-only image can resolve them. Depends on NeMo-Fabric's installed-adapter-discovery layout | |
| # (see image.py); swap to installing the top-level adapters/* packages once that lands on main. | |
| COPY --from=builder /src/python/src/nemo_fabric/adapters /src/python/src/nemo_fabric/adapters | |
| # The CLI's baked path is the literal `<fabric-core crate>/../../python/src/nemo_fabric/adapters`; the | |
| # kernel needs `/src/crates/fabric-core` to exist to walk the `..`, even though nothing lives there. | |
| RUN mkdir -p /src/crates/fabric-core | |
| ENV PATH=/opt/venv/bin:$PATH | |
| RUN python -c "from nemo_fabric import FabricClient" && fabric version | |
| WORKDIR /out | |
| FROM python:${PYTHON_VERSION}-slim-bookworm AS runtime | |
| COPY --from=builder /opt/venv /opt/venv | |
| COPY --from=builder /src/target/release/fabric /usr/local/bin/fabric | |
| # The CLI binary resolves built-in adapters from its compile-time repository path | |
| # (CARGO_MANIFEST_DIR/../../python/src/nemo_fabric/adapters); ship just that dir to the baked path so a | |
| # wheel-only image can resolve them. Depends on NeMo-Fabric's installed-adapter-discovery layout | |
| # (see image.py); swap to installing the top-level adapters/* packages once that lands on main. | |
| COPY --from=builder /src/python/src/nemo_fabric/adapters /src/python/src/nemo_fabric/adapters | |
| # The CLI's baked path is the literal `<fabric-core crate>/../../python/src/nemo_fabric/adapters`; the | |
| # kernel needs `/src/crates/fabric-core` to exist to walk the `..`, even though nothing lives there. | |
| RUN mkdir -p /src/crates/fabric-core | |
| ENV PATH=/opt/venv/bin:$PATH | |
| RUN python -c "from nemo_fabric import FabricClient" && fabric version | |
| RUN useradd --create-home --uid 1000 sandbox | |
| WORKDIR /out | |
| RUN chown sandbox:sandbox /out | |
| USER sandbox |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfile`
around lines 29 - 42, The runtime stage in sandbox.Dockerfile is still executing
as root because it never switches away from the default user. Update the runtime
setup after the existing COPY/RUN steps to create and use a dedicated non-root
user, and add a USER instruction so the final image runs sandboxed instead of as
root. Keep the change in the runtime stage near the existing ENV, WORKDIR, and
fabric version check so it is easy to locate alongside the image bootstrap
logic.
Source: Linters/SAST tools
Contributor
|
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
2 times, most recently
from
c7902c0 to
5382c71
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTYPE_CHECKING-only import of
FabricConfig/FabricProfileConfig.As per coding guidelines, "In Python code, prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible." These are imported only underTYPE_CHECKING, relying onfrom __future__ import annotationsfor deferred evaluation. Note_common.pyimportsnemo_relay.observabilitynormally since it's a hard dependency —nemo_fabrichere is optional, which is the likely reason for this pattern, but it still deviates from the stated guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py` around lines 61 - 64, The `container_runtime.py` module is using `FabricConfig` and `FabricProfileConfig` only inside `TYPE_CHECKING`, which conflicts with the typing guideline. Update the `FabricAgentRuntime`-related imports so these types are imported at runtime when available, and keep the module importable by handling the optional `nemo_fabric` dependency safely instead of relying on `TYPE_CHECKING`-only imports. Use the concrete type names directly in the runtime-facing annotations/config handling paths.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 61-64: The `container_runtime.py` module is using `FabricConfig`
and `FabricProfileConfig` only inside `TYPE_CHECKING`, which conflicts with the
typing guideline. Update the `FabricAgentRuntime`-related imports so these types
are imported at runtime when available, and keep the module importable by
handling the optional `nemo_fabric` dependency safely instead of relying on
`TYPE_CHECKING`-only imports. Use the concrete type names directly in the
runtime-facing annotations/config handling paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 923c56c2-87d8-44ed-90ee-4404ed535c9d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.pythird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txttools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
✅ Files skipped from review due to trivial changes (3)
- tools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
- third_party/licenses.jsonl
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/nemo_evaluator_sdk/pyproject.toml
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.py
SandyChapman
added a commit
that referenced
this pull request
Jul 8, 2026
The agent-eval runtimes framed the harness prompt with `task.intent` — the eval-side description of the *desired behavior* (what the grader checks for). Exposing it to the agent under evaluation is a reward-hacking hole: the agent can read the grader's intent and target it directly. This mirrors the held-out-`reference` concern (AGENT-EVAL reference field work). Unify the framing so every runtime builds the prompt from `task.inputs` only: - New `runtimes/fabric/_common.py` with a shared, intent-free `fabric_input`; the host `FabricAgentRuntime` now calls it (drops the `Task id/Intent/Inputs` framing). Designed as the shared home so the AALGO-321 container runtime reuses it instead of carrying its own copy. - `default_codex_prompt` no longer emits `Intent:`; it lifts the instruction from `inputs["instruction"|"prompt"]` and keeps its workspace-edit trailer. - `docker_sandbox._task_prompt` no longer falls back to `task.intent`; a task with no instruction in inputs yields an empty prompt rather than leaking it. Tests updated to lock in the intent-free contract across all three runtimes. Relates to AALGO-321 fabric-runtime dedup (PR #604). Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTYPE_CHECKING-only import of
FabricConfig/FabricProfileConfig.Guideline forbids gating type imports behind
TYPE_CHECKING. Understand the intent (nemo_fabric is an optional native dep), but this is the exact pattern the guideline disallows — flagging for visibility even though "import normally" here would reintroduce a hard dependency the module deliberately avoids.As per coding guidelines, "do not import those types only under
TYPE_CHECKING; import them normally when possible."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py` around lines 61 - 64, The `TYPE_CHECKING`-only imports for `FabricConfig` and `FabricProfileConfig` in `container_runtime` violate the import guideline, but the module also needs to stay importable without the optional `nemo_fabric` dependency. Update the typing strategy around `FabricAgentRuntime` and any `to_mapping()`-based config usage so these symbols are available without relying on a `TYPE_CHECKING`-gated import, while still preserving optional-dependency behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 127-153: The cleanup in ContainerRuntime.run_tasks is not
guaranteed because ensure_fabric_image and resolve_secrets run before the
try/finally that closes the provider. Move the image provisioning and secret
resolution into the guarded block in run_tasks so that self._provider.aclose()
still runs if setup fails, keeping the semaphore/gather flow unchanged and
preserving the existing run_one logic.
In
`@packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.py`:
- Around line 26-29: The _docker_ready helper can hang indefinitely on an
unresponsive Docker daemon, blocking the test run instead of skipping cleanly.
Update the subprocess.run call inside _docker_ready to use a bounded timeout and
treat timeout failures the same as an unavailable daemon by returning False.
Keep the change localized to _docker_ready so the live sandbox test setup
continues to use this readiness gate safely.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py`:
- Around line 61-64: The `TYPE_CHECKING`-only imports for `FabricConfig` and
`FabricProfileConfig` in `container_runtime` violate the import guideline, but
the module also needs to stay importable without the optional `nemo_fabric`
dependency. Update the typing strategy around `FabricAgentRuntime` and any
`to_mapping()`-based config usage so these symbols are available without relying
on a `TYPE_CHECKING`-gated import, while still preserving optional-dependency
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 849b4043-6962-4af0-b097-d2d035af98a0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/pyproject.tomlpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/_common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/image.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/sandbox.Dockerfilepackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider_live.pythird_party/licenses.jsonlthird_party/osv-licenses.jsonthird_party/requirements-main.txttools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/nemo_evaluator_sdk/pyproject.toml
- tools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/license/overrides.yaml
- third_party/licenses.jsonl
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_api.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/base.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_image.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/providers/docker.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_sandbox_docker_provider.py
- third_party/requirements-main.txt
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/sandbox/api.py
SandyChapman
added a commit
that referenced
this pull request
Jul 8, 2026
The agent-eval runtimes framed the harness prompt with `task.intent` — the eval-side description of the *desired behavior* (what the grader checks for). Exposing it to the agent under evaluation is a reward-hacking hole: the agent can read the grader's intent and target it directly. This mirrors the held-out-`reference` concern (AGENT-EVAL reference field work). Unify the framing so every runtime builds the prompt from `task.inputs` only: - New `runtimes/fabric/_common.py` with a shared, intent-free `fabric_input`; the host `FabricAgentRuntime` now calls it (drops the `Task id/Intent/Inputs` framing). Designed as the shared home so the AALGO-321 container runtime reuses it instead of carrying its own copy. - `default_codex_prompt` no longer emits `Intent:`; it lifts the instruction from `inputs["instruction"|"prompt"]` and keeps its workspace-edit trailer. - `docker_sandbox._task_prompt` no longer falls back to `task.intent`; a task with no instruction in inputs yields an empty prompt rather than leaking it. Tests updated to lock in the intent-free contract across all three runtimes. Relates to AALGO-321 fabric-runtime dedup (PR #604). Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Add FabricContainerRuntime: runs NeMo Fabric + a coding agent inside a per-task Docker sandbox and maps the result into the existing CandidateEvidence contract, so the workspace-file, run_verifier, and trajectory metrics score container trials with no metric changes. - Provider-neutral sandbox seam (SandboxSpec/SandboxProvider/AsyncSandbox) with a Docker provider that shells out to the docker CLI. Context/artifacts cross the boundary by file transfer (docker cp), so a Kubernetes provider can slot in behind the same seam. AsyncSandbox tears down only its own sandbox; the shared provider's lifetime is the batch owner's (run_tasks disposes it once). - Opaque build-if-missing Fabric image (content-addressed tag) — no Dockerfile for callers to write. - Declare nemo-relay as a hard dependency and build the trajectory profile from its typed config, so Relay owns its schema instead of a hand-maintained dict. - Share the host/container helpers (failed-trial builder, output extraction, evidence subdir naming, trajectory telemetry) in fabric/_common.py so the two runtimes cannot silently drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
schapman/aalgo-321-containerized-fabric-runner
branch
from
5382c71 to
0945799
Compare
Contributor
Author
|
Thanks @coderabbitai — addressed in the latest push (also rebased onto Fixed
Deferred to a hardening follow-up (tracked; kept out of this spike to preserve the verified live e2e)
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
FabricContainerRuntime(AALGO-321): runs NeMo Fabric + a coding agent inside a per-task Docker sandbox and maps the result into the existingCandidateEvidencecontract, so the workspace-file,run_verifier, and trajectory metrics score container trials with no metric changes.Highlights
SandboxSpec/SandboxProvider/AsyncSandbox) with a Docker provider that shells out to thedockerCLI. Context/artifacts cross the boundary by file transfer (docker cp), so a Kubernetes/agent-sandbox provider can slot in behind the same seam later.AsyncSandboxtears down only its own sandbox; the shared provider's lifetime is the batch owner's (run_tasksdisposes it once).ensure_fabric_imageseam.nemo-relayis now a hard dependency: the trajectory profile is built from Relay's own typed config (via the sharedfabric/_common.py), so Relay owns its schema instead of a hand-maintained dict that silently drifts. Verifiedcp311-abi3wheels exist for every platform the workspace locks.fabric/_common.pyso the two runtimes cannot drift apart.Scope
Exercises the runtime directly (hermes-sdk harness, validated live end-to-end); the plugin
_resolve_targetwiring is a follow-up. Seeexamples/fabric_container/for a runnable end-to-end.Verification
Full
agent_evalsuite green (143 passed, 1 skipped — the gated live Docker test); ruff + pyright clean.Summary by CodeRabbit