Skip to content

feat: sandbox_agent swe resources server#2048

Open
cmunley1 wants to merge 10 commits into
mainfrom
cmunley1/sandbox-agent-anyswe
Open

feat: sandbox_agent swe resources server#2048
cmunley1 wants to merge 10 commits into
mainfrom
cmunley1/sandbox-agent-anyswe

Conversation

@cmunley1

@cmunley1 cmunley1 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

builds on #2015 with new swe verifier

cmunley1 added 9 commits July 14, 2026 02:00
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
Signed-off-by: Christian Munley <cmunley@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 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.

Signed-off-by: Christian Munley <cmunley@nvidia.com>

@ffrujeri ffrujeri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm keeping this pass at the design level — three design questions below, anchored in three inline comments. More granular code-level notes (grading fidelity vs the official harness, error handling, tests) can follow as a separate pass if useful.

Background

Where this PR sits

Dataset rows (environments/swe/) drive sandbox_agent, which provisions a per-task sandbox, runs the opencode harness inside it against the host model server, and captures the repo's git diff as model_patch; anyswe then re-grades that patch in a fresh sandbox with the official SWE-bench eval scripts.

flowchart LR
    DS[Dataset rows<br/>environments/swe/] --> DRV["Rollout driver<br/>(ng_collect_rollouts / eval loop,<br/>POSTs /run per task row)"]
    DRV -->|"/run"| SA["sandbox_agent run()"]
    SA -->|provision| BOX1[Rollout sandbox<br/>opencode harness]
    BOX1 -->|chat| MS[Model Server]
    BOX1 -->|git diff → model_patch| RS[anyswe verifier]
    RS -->|fresh box, official<br/>swebench eval| BOX2[Grading sandbox]
    BOX2 -->|"eval log"| RS
    RS -->|"reward 1/0"| DRV
    DRV --> RL[(rollouts.jsonl / training)]
Loading

Design discussion

These aren't review nits — they're design choices we're actively weighing for how sandboxed environment execution should work in Gym generally. This PR is the first consumer of that machinery, so it's the right moment to align on direction; none of them needs to block the PR itself.

Design question #1: where should the config-vs-data boundary sit for the sandbox knobs?

What's implemented today: the provider, credentials, and box size are fixed at server startup via Hydra config, while the image, workdir, and grading spec arrive per task in dataset rows. For the provider + credentials half, that placement looks exactly right — credentials stay in env vars, one provider client is built at model_post_init and reused across tasks, runs stay reproducible, and data can never swap the provider — none of that is being questioned. The open question is the middle of the spectrum: box sizing and related spec knobs, where the code currently gestures at per-task configurability without delivering it, and the boundary itself (including its trust implications) is undocumented.

For clarity, the environment involves two separate sandboxes with independently configured providers — the config/data boundary is drawn differently on each side, and that asymmetry is part of what's flagged below:

Rollout ("solve") sandbox Grading sandbox
Provisioned by agent server (sandbox_agent, per /v1/responses) resources server (anyswe, fresh box per verify())
Provider + credentials startup config (SandboxAgentConfig.sandbox_provider) startup config (AnySweConfig.sandbox_provider) — a separate config; may even be a different backend
Box size (cpu/mem/disk) startup sandbox_spec only — per-server, not per-task nowhere: no sandbox_spec on AnySweConfig, and the per-task hatch in build_spec is unreachable (below)
Image per-task (row's docker_image) per-task (same row metadata, rebuilt pristine)

The two-box split itself is a good hermeticity property — the model never touches the box it's graded in; anyswe rebuilds from the pristine instance image and applies only the extracted patch. The caveat worth documenting: the two providers can diverge across deployments (different exec shells, different default timeouts), which is a reward-fidelity consideration.

So the concrete questions, each documentation-or-small-code:

  • The grading sandbox currently cannot be sized at all: AnySweConfig exposes no sandbox_spec, and build_spec's per-task resources/provider_options keys are unreachable because verify() forwards only instance_dict into task.metadata (app.py:724). Either forwarding those keys from verifier_metadata or removing them would make the real capability match the code.
  • Symmetrically, the solve sandbox takes cpu/mem/disk only from startup sandbox_spec, so a heavy instance can't request a bigger box on either side. If that's intentional (throughput predictability), a README sentence would prevent surprise.
  • The trust model is implicit: any /v1/responses caller can name an arbitrary docker_image and, via sandbox_eval, arbitrary in-box shell. Fine for operator-owned deployments — a README note ("not for untrusted callers") would make the boundary explicit.

Design question #2: could responses() stay out of the provisioning business?

Right now responses() carries the placement machinery — provider client, gym source tarball, setup_commands bootstrap, network probe, patch extraction, and the optional in-box grading — on top of its actual job (request → trajectory).

Concretely, in responses() (app.py:341-386):

image = meta.get("docker_image") or self.config.sandbox_image        # L347  placement input from the row
handle = await self._provision_box(image, files, model_url)          # L364  provisions the sandbox
r = await self._provider.exec(handle, runner_cmd, timeout_s=...)     # L366  runs the harness in-box
resp = NeMoGymResponse.model_validate(
    await self._download_json(handle, "/work/response.json"))        # L370  artifact retrieval
r = await self._provider.exec(handle,
    f"git -C {wd} add -A . && git -C {wd} diff --cached", ...)     # L374  patch extraction
reward = await self._grade_in_box(handle, grade_spec)                # L382  in-box grading

and _provision_box (app.py:250-293) is a full provider lifecycle: provider.create(spec) (L252), file uploads (L255-259), gym-source tarball fetch + extraction (L260-275), setup_commands execution (L276-279), and a TCP reachability probe of the model endpoint (L281-289) — none of which is "produce a trajectory".

Since the repo's own decomposition assigns per-task state (the execution context) to resources servers, an alternative shape that may age better as more sandboxed environments arrive is a dedicated sandbox-broker resources server — a third server that provisions and leases boxes to whichever component needs one (the agent's rollout box, the verifier's grading box):

  1. A dedicated sandbox-broker resources server owns provisioning. It holds the one provider client + credentials and exposes lease-style endpoints (create / exec / upload / download / release, with TTLs and quotas); the agent server and the task/verifier server are both just clients of it. Because Gym components are composable, this can be built today as an ordinary resources server — no core-framework change.
  2. responses() becomes thin: it leases a box for the row's docker_image from the broker and launches the harness through the handle — no create_provider, no tarball, no per-request bootstrap. Its contract returns to "request → trajectory".
  3. verify() leases its own fresh grading box from the same broker and extracts the git diff itself rather than reading agent-attached model_patch — capture policy (e.g. whether untracked scratch files the agent leaves behind should enter the patch) becomes verifier-owned, the sandbox_eval in-agent grading path can disappear (reward computation living in the agent cuts against the agent/verifier split), and the task server itself stays stateless. One provider policy, one concurrency bound, one cleanup path for both box types — instead of the two independently configured providers in the table above.
flowchart LR
    DRV["Rollout driver<br/>(ng_collect_rollouts, POSTs /run)"] -->|"/run per task row"| RUN["run()<br/>(host orchestration)"]
    RUN -->|"1. seed_session"| RS["Task resources server<br/>(anyswe — verify logic only,<br/>stateless)"]
    RUN -->|"2. /v1/responses"| AG["Agent responses()<br/>(thin: request → trajectory)"]
    AG -->|"lease box (row's docker_image)"| SB["Sandbox-broker resources server<br/>(owns provider + creds,<br/>leases, quotas, cleanup)"]
    SB -->|"provision"| BOX1["Rollout sandbox"]
    AG -->|"launch harness via handle"| BOX1
    BOX1 -->|"model calls"| MS["Model Server (host)"]
    RUN -->|"3. verify"| RS
    RS -->|"lease fresh grading box"| SB
    SB -->|"provision"| BOX2["Grading sandbox"]
    RS -->|"extract git diff, grade,<br/>release lease"| BOX2
Loading

Supporting changes that make (2) honest: prebuilt harness images (a thin gym-harness layer over the instance images, or a versioned wheel fetched via gym_source) instead of per-request tarball + pip + installer downloads; and documenting the runner's files-in/files-out contract (request.jsonresponse.json) as a versioned interface, so "the nested agent's responses() must be self-contained" is a stated requirement rather than a discovered one.

To be fair to the current shape: the provider API is exec/upload/download-only (no inbound networking into sandboxes), which rules out the cleanest alternative — running the nested agent as a real server in the box and reverse-proxying to it — and brokered handles need lease semantics (ownership, TTLs, cleanup on crash) that don't exist yet. So a possible phasing:

  • PR-sized now: extract the provisioning/bootstrap/extraction into a SandboxRuntime module the agent calls (pure layering, no behavior change, independently testable), and consider moving sandbox_eval grading to the verifier.
  • Next: prebuilt harness images as the recommended path, with the auto-tar demoted to a dev fallback.
  • Later: the sandbox-broker resources server itself — provider credentials, leases, quotas, and observability in one place, consumed by both agents and verifiers — making the "execution environment" something any component can request without owning a provider.

Where this could end up (follow-up-sized, not this PR): port exposure in the provider API. The exec-only constraint is itself incremental to lift: if SandboxProvider grew one optional primitive — expose/tunnel a sandbox port to a host-reachable URL — the nested harness could run as its normal FastAPI server inside the box, and SandboxAgent.responses() would collapse to a pure reverse proxy. That removes the files-in/files-out runner contract entirely (the harness gets a real Request, a real ServerClient, streaming, health checks — no importlib bootstrap in a bare interpreter), and providers without port exposure would simply keep today's exec-based runner as the fallback.

flowchart LR
    subgraph BOX["Task sandbox"]
        NA["Nested agent server<br/>(unchanged harness running<br/>as its own FastAPI app)"]
    end
    DRV["Rollout driver<br/>(ng_collect_rollouts)"] -->|"/run → /v1/responses"| AG
    AG["SandboxAgent responses()<br/>(pure reverse proxy)"] -->|"proxied HTTP via<br/>provider port tunnel"| NA
    NA -->|"model calls"| MS["Model Server (host)"]
    PRV["SandboxProvider<br/>+ expose_port() — new capability"] -.->|"provision + tunnel"| BOX
Loading

Would this direction fit your intent, or is agent-owned provisioning deliberate (e.g. keeping the verifier stateless and the two servers' providers uncoupled)? Entirely reasonable as follow-up work rather than in this PR — flagging it now because the layering will be much cheaper to change before more environments build on sandbox_agent. If the port-exposure capability sounds right, I can open a tracking issue for it so it doesn't get lost.

Design question #3: should runtime placement live in an agent server at all?

Drawing the two modes as shipped makes the shape easier to discuss. agent_only_runner:

flowchart TB
    subgraph HOST["Host Gym deployment"]
        DRV["Rollout driver<br/>(ng_collect_rollouts)"] -->|"/run"| SA["sandbox_agent server"]
        SA -->|"seed_session / verify"| RS["anyswe resources server"]
        MS["Model server"]
    end
    subgraph BOX["Task sandbox (row's docker_image)"]
        RUNNER["agent_runner.py<br/>importlib-loads the harness class,<br/>calls its responses() in-process"]
    end
    SA -->|"provision; upload runner,<br/>request.json, gym tarball"| BOX
    RUNNER -->|"model calls"| MS
    SA -->|"download response.json;<br/>git diff → model_patch"| BOX
Loading

and gym_runner, where the recursion is the interesting part — the host driver calls an agent that boots a second rollout driver inside the box:

flowchart TB
    subgraph HOST["Host Gym deployment"]
        DRV["Rollout driver<br/>(ng_collect_rollouts)"] -->|"/run"| SA["sandbox_agent server<br/>(mode: gym_runner)"]
        MS["Model server"]
    end
    subgraph BOX["Task sandbox"]
        NDRV["nested ng_collect_rollouts<br/>(a second rollout driver)"] -->|"/run"| NGYM["ng_run: full nested Gym<br/>(agent + resources servers)"]
    end
    SA -->|"provision; start nested Gym;<br/>poll done-file every 20s"| BOX
    NGYM -->|"model calls"| MS
    SA -->|"download rollouts.jsonl<br/>(already verified in-box)"| BOX
Loading

The mode switch is really placement policy — "where does this rollout execute, and at what granularity (just the harness, or the whole environment)?" — encoded as agent config. Two consequences of hosting that policy here: the agent re-implements driver concerns (concurrency bound, rollout_timeout, done-file polling, log tailing, artifact download — all things the rollout driver already owns), and in gym_runner mode a workload component re-instantiates its own orchestrator one level down, so run() has two different contracts depending on a config flag (external verify vs in-box verify).

An alternative worth considering: make runtime placement an upstream responsibility — the environment's config declares its runtime, and the rollout driver (or a small launcher layer beneath it) enforces it:

flowchart TB
    CFG["Environment config<br/>runtime: host | sandbox{provider,<br/>image: row metadata, granularity}"] --> DRV
    DRV["Rollout driver / launcher<br/>(owns placement, timeouts,<br/>retries, artifact collection)"]
    DRV -->|"runtime: host (default)"| HA["Agent server on host<br/>(unchanged)"]
    DRV -->|"runtime: sandbox"| BOX
    subgraph BOX["Task sandbox"]
        SAME["The same agent server / harness,<br/>launched in-box — code unchanged,<br/>unaware of placement"]
    end
    HA -->|"seed_session / verify"| RS["Resources server"]
    SAME -->|"model calls"| MS["Model server"]
    SAME -->|"artifacts"| DRV
Loading

Under that shape the two modes dissolve into a single granularity knob on the placement policy ("place the agent" vs "place the whole environment"), the polling/tarball/done-file machinery is implemented once at the layer that already owns orchestration, and harness code runs host-side or in-box without knowing the difference. Today's sandbox_agent would become the launcher's implementation detail rather than a harness impersonating one.

To be fair, this is the biggest-lift option of the three questions — the current driver is deliberately thin (it just POSTs /run), and using a custom agent server was the extension point the framework actually offers, which is why this PR's shape is a reasonable v1. Do you read the two modes the same way — as placement policy that ideally migrates upstream once a launcher/runtime abstraction exists — or is agent-hosted placement a deliberate long-term choice? If the former, this and the port-exposure capability could share one "sandbox runtime" tracking issue.

Minor: the branch is currently behind main; a rebase would freshen CI. The only existing comment thread is the copy-pr-bot notice, so nothing else pending.

finally:
await self._close_box(handle)

async def responses(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responses_api_agents/sandbox_agent/app.py:341

Question on layering rather than a defect: was it a deliberate choice for responses() to own sandbox provisioning, gym-source shipping, and artifact extraction — rather than, say, leasing the box from a dedicated sandbox-broker resources server (per-task state being resources servers' charter) and keeping this endpoint request→trajectory? A concrete alternative sketch with a phased plan is in the review summary under "Design question #2" — curious whether broker-provided execution environments would fit your intent, or whether agent-owned provisioning is deliberate (stateless verifier, providers uncoupled across servers).

**({"slurm_job_id": str(task.metadata["slurm_job_id"])} if task.metadata.get("slurm_job_id") else {}),
},
resources=SandboxResources.from_mapping(task.metadata.get("resources", {})),
provider_options=dict(task.metadata.get("provider_options", {})),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resources_servers/anyswe/app.py:430

These per-task resources/provider_options keys are currently unreachable: verify() forwards only {"instance_dict": ...} into task.metadata (app.py:724), and AnySweConfig exposes no sandbox_spec either — so the grading sandbox can't be sized per-task or per-server. Was the intent to forward these from verifier_metadata? Either wiring them through or removing the unreachable keys would make the capability match the code. (A fuller sketch of the startup-config vs per-task-metadata boundary, which otherwise looks like the right design, is in the review summary.)

- agent_only_runner: imports the configured agent's `responses()` inside the sandbox via
a small runner. The harness code executes unchanged, only its model base URL is patched
to a sandbox-reachable address. No Gym servers in the sandbox.
- gym_runner: starts full NeMo Gym inside the sandbox (`nested_config_paths`) and runs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responses_api_agents/sandbox_agent/README.md:10

Design question on the two modes (also sketched with diagrams in the review summary, "Design question #3"): both modes are really placement policy — where a rollout executes and at what granularity — encoded as agent config. In gym_runner mode the host rollout driver ends up calling an agent that boots a second ng_collect_rollouts inside the box, and the agent re-implements driver concerns (timeout budget, done-file polling, artifact download, concurrency). Would you see runtime placement as ultimately an upstream responsibility — environment config declares runtime: host | sandbox, the rollout driver/launcher enforces it, and the two modes collapse into a granularity knob — with this server as the v1 stepping stone toward that? Asking to understand the intended end-state rather than to block this PR.

@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants