Skip to content

fix(sandbox): implement canonical task environment resolution - #2772

Open
Abdullah-Builds wants to merge 3 commits into
NVIDIA-NeMo:mainfrom
Abdullah-Builds:fix/2769-task-environment-contract
Open

Abdullah-Builds wants to merge 3 commits into
NVIDIA-NeMo:mainfrom
Abdullah-Builds:fix/2769-task-environment-contract

Conversation

@Abdullah-Builds

Copy link
Copy Markdown

What does this PR do?

Implements the core task_environment contract described in #2769.

This change adds a canonical task-environment resolver that converts dataset-provided task environment configuration into the existing provider-neutral SandboxSpec.

The resolver supports:

  • Immutable container images using image@sha256:<digest>.
  • Trusted manifest-backed task resolution using task_id.
  • Validation that direct image references agree with the trusted manifest.
  • Restricting dataset-controlled sandbox fields to the supported task-environment contract.
  • Preserving operator-controlled sandbox configuration.
  • Recording the resolved task identity and image in SandboxSpec.metadata.
  • Backward compatibility for tasks that do not provide task_environment.

Unit tests are included for task-environment validation, manifest resolution, image consistency, security boundaries, and operator configuration preservation.

Closes #2769.

Checklist

  • I have read the [contributing guidelines](https://docs.nvidia.com/nemo/gym/latest/contribute/development-setup).
  • The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs.
  • Tests added or updated and pass locally, or N/A for docs-only / non-code changes (so CI unit/server checks pass when applicable).
  • Pre-commit checks pass locally (pre-commit run --all-files) (so CI lint/format/copyright pass).
  • All commits have DCO sign-off (git commit -s) (so the DCO check passes).

@copy-pr-bot

copy-pr-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@nemo-automation-bot nemo-automation-bot Bot added the community-request Issue reported or requested by someone from the community label Aug 25, 2026
@Abdullah-Builds

Abdullah-Builds commented Aug 25, 2026 •

Copy link
Copy Markdown
Author

@claude @suhara waiting for approval

@cmunley1

Copy link
Copy Markdown
Contributor

/claude review

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

SHIP WITH CARE — the resolver logic is correct, but the security-critical rejection paths that are this module's whole purpose are largely untested.

task_environment.py is a trust boundary: it stops an untrusted dataset row from pinning a mutable image tag or escaping into operator-owned SandboxSpec fields. I traced every branch — sandbox_spec() round-trips all 11 SandboxSpec fields with no silent drop, validator ordering is correct, and both the direct-image and manifest paths enforce the immutable-digest guard (lines 60, 158). No correctness bug found.

RISK — untested security-rejection branches. WHAT: resolve_task_environment / _manifest_entry guard the trust boundary through raise-paths that the 6 tests never exercise:

  • task_id set but manifest is None → raise (L153–156)
  • manifest image not an immutable digest → raise (L158–161) — the core anti-tampering guarantee on the manifest path
  • unknown task_id (L106–107)
  • task_environment not a Mapping (L145–146)
  • manifest entry with unsupported keys / missing image / non-mapping (L111–123)
  • empty/whitespace field rejection (L53–54)

BLAST RADIUS: this is exactly the verify()-class logic the repo treats as a silent-risk if untested — a future refactor that weakens the digest check or the manifest is None guard would let a dataset inject a mutable/arbitrary image into a training or eval sandbox, and green tests would still pass. It also almost certainly sits below the repo's 96% coverage bar. FIX: add negative tests for each raise above (assert ValueError with the matching message), mirroring the two rejection tests already present (test_dataset_cannot_set_operator_fields, test_mutable_image_is_rejected).

NOTE — the module has no callers yet in this PR; it's standalone infrastructure. Nothing regresses at runtime, but nothing exercises it end-to-end either, so the tests are the only guarantee until it's wired in.

@github-actions github-actions Bot added the sla:triage-overdue Review assignment is over the one-business-day SLA label Aug 27, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Aug 28, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request Issue reported or requested by someone from the community sla:triage-overdue Review assignment is over the one-business-day SLA waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add and document a standard per-task execution environment contract

3 participants