Repository navigation
fix(agents): hide the sandbox supervisor and runners from pkill python - #4805
Conversation
Models clean up their own work with `pkill -f python3`, `pkill python3`, `killall python3` and similar. In the task sandbox those matched Gym's supervisor (`python3 -I .../process_supervisor.py`) and the harness runner under it, so the supervisor stopped the agent CLI and the episode ended as "Agent response status: failed" with reward 0. On long Terminal-Bench-Science runs this ended Codex trials partway through, while under a harness whose agent CLI is node or a native binary the same commands are harmless. The launch shell now links `<session>/bin/gym-supervisor` to the binary behind the adapter's interpreter (`/proc/self/exe`, past venv links and shims) and execs the supervisor through it, from `process_supervisor` without a `.py` suffix. A runner that opts in with `SandboxCommand.hide_runner` runs the same way as `bin/gym-runner`. Codex and Pi opt in and upload their runner as `sandbox_runner`. Neither process name nor command line mentions python, so none of `pkill -f python3|python|"python3 -"|"\.py"`, `pkill python3|python` or `killall python3` matches. Hermes keeps its venv interpreter for its runner (the runner is the agent); its supervisor is hidden. OpenCode and OpenClaw get the hidden supervisor too; their test fakes now match the new file name. If the link cannot be made or cannot load the supervisor, both processes keep the given interpreter. Gym's stop path signals the recorded PID and is unchanged. Tests: the shared supervisor tests add a pgrep-based identity test that runs the patterns as pkill/killall would match them, a link fallback case and a check that a venv runner is left alone. Codex, Pi, Hermes, OpenCode and OpenClaw tests follow the new file names. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
The OpenCode and OpenClaw adapters launch the same shared supervisor and a standard-library runner (`python3 -I <session>/sandbox_runner.py`) above their CLI, so a task agent's `pkill -f python3` or `killall python3` stops the runner and ends the episode just as it did for Codex and Pi. The supervisor is already hidden; opt the runners in too: upload them as `sandbox_runner` and set `SandboxCommand.hide_runner`, so they run as `bin/gym-runner`. OpenCode's post-cleanup transcript snapshot uses the renamed file. Tests: each adapter's exec-transport suite gains a test that launches the real supervisor and runner and checks that neither process name nor command line mentions python; the session fakes expect the new runner name. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. Losing a Codex trial to a task-side pkill python3 is exactly the kind of harness divergence we want gone, and renaming the processes is a proportionate fix. We checked that the four opted-in runners and the supervisor are standard-library only, that the launch falls back cleanly when the link can't be made (including under dash), and that Gym's stop path still signals the recorded PID. The inline comments are small: a stray bin/ that a close race can leave behind, a docstring that claims a bit more than holds for Hermes/Stirrup, and a test that lost macOS coverage.
A few things for the PR description:
- Could you link an issue, or say why none is needed?
- This changes the launch path for every sandboxed agent, and the live OpenSandbox check ran on an earlier base. Could you add one smoke rollout (Codex is enough) on this head?
- "Benchmark impact: none expected": since this removes spurious reward-0 interruptions (~1% of Codex trials on Terminal-Bench-Science), results can move slightly. Worth saying so.
- Stirrup shares
SandboxSession, so its supervisor now also runs throughbin/gym-supervisoron the base interpreter. That's fine, but worth listing next to Hermes. - Follow-up, not for this PR:
hermes_sandboxed_agenthas its own launch path (app.py:227) that still uploads and runsprocess_supervisor.py, so it isn't protected yet.
| f"trap '' TERM; ln -s launch {claim_path} 2>/dev/null || exit 0; " | ||
| f"py={quote(python)}; " | ||
| f"real=$({quote(python)} -I -c {quote(_RESOLVE_INTERPRETER)} 2>/dev/null) && " | ||
| f'mkdir -p {quote(f"{session_dir}/bin")} && ln -sfn "$real" {link} && ' |
There was a problem hiding this comment.
nemo_gym/agent_utils/supervisor_client.py:144
If close retires the session directory while a delayed launch is still resolving the interpreter, mkdir -p recreates <session>/bin/gym-supervisor in the borrowed sandbox after close has confirmed. No launch follows (the supervisor file is gone), but on main this window wrote nothing. Could this create only bin, so it fails once the session directory is gone?
| f'mkdir -p {quote(f"{session_dir}/bin")} && ln -sfn "$real" {link} && ' | |
| f'{{ [ -d {quote(f"{session_dir}/bin")} ] || mkdir {quote(f"{session_dir}/bin")}; }} && ln -sfn "$real" {link} && ' |
There was a problem hiding this comment.
Good catch, applied in dfafcf3. It now creates only bin, so a late launch fails once close has removed the session directory. I added a test that holds a launch while it resolves the interpreter, then confirms the stop, removes the session and lets the launch go on. It fails on the old mkdir -p and passes now.
| interpreter links with neutral names, from files without a ``.py`` suffix. Neither | ||
| their process names nor their command lines mention python. Gym's own stop path |
There was a problem hiding this comment.
nemo_gym/agent_utils/supervisor_client.py:27
The supervisor's command line includes the harness argv after -- (L152). Hermes and Stirrup keep a venv-python sandbox_runner.py there, so pkill -f python and pkill -f '\.py' still match their supervisor. Not a regression, but could the docstring scope the claim to runners that opt in?
| interpreter links with neutral names, from files without a ``.py`` suffix. Neither | |
| their process names nor their command lines mention python. Gym's own stop path | |
| interpreter links with neutral names, from files without a ``.py`` suffix. Their process | |
| names never mention python, and neither do their command lines when the runner opts in; a | |
| harness argv that keeps its own python (Hermes, Stirrup) still appears in the supervisor's | |
| command line. Gym's own stop path |
There was a problem hiding this comment.
Applied your wording in eb00a07. Claude Code and Kilo, which landed on main since, are in the same position as Hermes and Stirrup: their supervisor is renamed but their runner argv keeps python. The description now lists all four.
|
|
||
|
|
||
| async def test_bootstrap_quotes_private_paths_and_uses_the_same_interpreter(tmp_path: Path) -> None: | ||
| @pytest.mark.skipif(sys.platform != "linux", reason="interpreter links resolve /proc/self/exe") |
There was a problem hiding this comment.
tests/unit_tests/test_sandbox_supervisor_bootstrap.py:159
This quoting/injection test used to run on every platform; the skipif now drops all four params off Linux, though only linkable=True depends on /proc. Dropping the skipif and setting linkable = linkable and sys.platform == "linux" (the launch falls back elsewhere) keeps it running on macOS; all four params pass there with that change.
There was a problem hiding this comment.
Done in 926a601: the skipif is gone and the test sets linkable = linkable and sys.platform == "linux".
|
|
||
| argv: list[str] | ||
| python: str | ||
| hide_runner: bool = False |
There was a problem hiding this comment.
nemo_gym/agent_utils/sandbox_session.py:55
A naming suggestion for hide_runner. Setting it carries two preconditions that the name doesn't show:
- The runner must be standard-library only. The link points at the base interpreter, outside any venv, so venv packages stop importing. That's why Codex/Pi/OpenCode/OpenClaw can opt in and Hermes/Stirrup can't. If someone turned it on for Hermes, every unit test would still pass and the failure would only show up in a real sandbox.
- argv[0] must equal
python. Otherwise the flag silently does nothing (supervisor_client.py:136).
"Hide" also reads like isolation, but the process stays visible in ps; it just runs under a neutral name. Could we name it after the contract the caller has to guarantee, e.g. stdlib_runner (or neutral_runner_name if you'd rather name the effect)? A name like that makes the opt-in a deliberate claim about the runner.
There was a problem hiding this comment.
Following up on the naming point: there may be a way to drop the stdlib-only precondition entirely, and with it the opt-in flag. Not blocking for this PR; it would make a good follow-up.
1. Put the runner link inside the runner's venv bin/. CPython decides it's in a venv by looking for pyvenv.cfg next to the unresolved executable path or one level up. That's how <venv>/bin/python, itself a symlink to the base binary, works. So <venv>/bin/gym-runner -> <base python> keeps the venv, while <session>/bin/gym-runner does not. A quick local check (macOS, CPython 3.9; worth confirming on Linux):
| launched as | sys.prefix |
in venv |
|---|---|---|
venv/bin/python |
venv | yes |
venv/bin/gym-runner -> base binary |
venv | yes |
session/bin/gym-runner -> base binary (this PR) |
base install | no |
With the link next to the runner's own interpreter, Hermes and Stirrup could get a neutral runner name too. Every adapter could then do it by default, with no hide_runner flag.
2. Pass the harness argv to the supervisor through a file. The supervisor's command line currently ends with -- <harness argv> (supervisor_client.py:152), which is why a Hermes/Stirrup supervisor still matches pkill -f python. If the argv were written to something like <session>/harness.json and the supervisor took only that path, its command line would never mention python, whatever the runner is.
Together these would cover all six adapters against name- and command-line-based kills. (Nothing name-based helps against kill -9 -1; that would need a PID namespace or a separate user, which is a much bigger change.)
There was a problem hiding this comment.
Renamed to stdlib_runner in 04ebebc, so turning it on is a claim about the runner. It also fails loudly now: SandboxCommand and supervised_launch_command raise ValueError when it's set for an argv that doesn't start with the adapter's python, instead of silently doing nothing. There are tests for both.
There was a problem hiding this comment.
Agreed, and thanks for checking the pyvenv.cfg lookup. I'll open an issue for both once this merges and follow up with a PR:
- link the runner inside its own venv's
bin/(I'll confirm the lookup on Linux first); - pass the harness argv to the supervisor through a file, so its command line never mentions python.
That covers Hermes, Stirrup, Claude Code and Kilo too and lets us drop stdlib_runner. It can also move hermes_sandboxed_agent onto the shared launch path.
…ervisor-pkill Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com> # Conflicts: # responses_api_agents/pi_agent/tests/test_sandbox.py
…aunch A launch delayed before it links the interpreter used mkdir -p for the session's bin directory, so it recreated the session directory after close had removed it. Create only bin, so the link fails once the session is gone. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
A harness argv that keeps its own python, such as Hermes's or Stirrup's, still appears in the supervisor's command line, so pkill -f python can match the supervisor there. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Only the interpreter links depend on /proc. Elsewhere the launch falls back to the given interpreter, so expect that instead of skipping the test. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…rgv it can't hide The opt-in is a claim about the runner: it must need nothing beyond the standard library, because the link runs the base interpreter outside any virtual environment. Name it after that contract. Setting it for an argv that doesn't start with the adapter's python used to do nothing silently; SandboxCommand and supervised_launch_command now raise ValueError. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Kilo landed on main after this branch renamed the uploaded supervisor to process_supervisor; its fake sandboxes still looked for process_supervisor.py. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
…harness guide Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
|
Thanks for the careful review. All four inline points are addressed (dfafcf3, eb00a07, 926a601, 04ebebc), and
The description now has:
|
…ervisor-pkill Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com> # Conflicts: # responses_api_agents/opencode_agent/app.py # responses_api_agents/opencode_agent/sandbox.py
NVIDIA-NeMo#4747's multi-activation tests expected process_supervisor.py, while this branch uploads the supervisor without the suffix. Both now use SUPERVISOR_FILE, so the OpenCode upload-delay test injects its delay again. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
|
/ok to test 7fdf8a0 |
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
|
/ok to test 1d1ec23 |
NOOA's sandbox runner landed on main after this branch renamed the uploaded supervisor to process_supervisor, so its tests still looked for process_supervisor.py and never routed the launch. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
|
/ok to test 5a53bf2 |
|
what do you think about using a separate user or namespace than process name? |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40417870764 |
Resolve the conflicts with NVIDIA-NeMo#4805, which hid the Python supervisor and opted-in Python runners from `pkill python` by running them through interpreter links. The supervisor here is bash, so its link is dropped. Its runner link survives as SandboxCommand.stdlib_runner on the bash launch: Codex, the one harness that keeps a standard-library Python runner, runs it as bin/gym-runner from `sandbox_runner`. OpenCode, OpenClaw, Pi and Kilo keep the bash runner, whose name never mentions python. The pkill and delayed-launch tests follow the bash supervisor; the READMEs and the harness guide describe the result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
Follow-up for the venv runner link and argv-by-file suggestion from review is filed as #5143. |
Main now runs the shared supervisor as gym-supervisor from a file without a .py suffix (#4805), and the Copilot test fake still matched process_supervisor.py, so 17 tests failed after merging main. Opt the Copilot runner in the same way as Pi, Codex, OpenCode and OpenClaw: upload it as sandbox_runner and set stdlib_runner, so a task agent's pkill -f python3, pkill python3 or killall python3 cannot stop the runner and end the episode. The post-run compaction probe uses the same file. The session fake now asserts the runner launches as bin/gym-runner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
What does this PR do?
Agents clean up after themselves with
pkill -f python3orkillall python3. In the task sandbox that also matches Gym's supervisor (python3 -I .../process_supervisor.py) and the harness runner under it, so the agent CLI is stopped and the episode ends with reward 0. On 8-hour Terminal-Bench-Science runs this ended 2 of 210 Codex trials partway through.The supervisor now runs as
<session>/bin/gym-supervisor, a link to the adapter's interpreter, from a file without a.pysuffix. A runner that needs only the standard library can setSandboxCommand(stdlib_runner=True)to run asbin/gym-runnerthe same way. With both, neither process name nor command line mentions python, so the usualpkill/killallpatterns don't match. If the link can't be made, both fall back to the given interpreter. Gym's own stop path is unchanged.pkill -f pythoncan match it there.hermes_sandboxed_agenthas its own launch path and still runsprocess_supervisor.pydirectly.bin/and pass the harness argv by file. That covers every adapter and dropsstdlib_runner.No issue: found in Terminal-Bench-Science runs; the repro is above.
Validation
pkillpatterns don't match them. They also check that a launch delayed past close doesn't recreate the session directory, and thatstdlib_runnerrejects an argv it can't rename.pkill -f python3,pkill python3andpkill -f pythoneach killed a python decoy, while the supervisor, runner and Codex survived with the same PIDs (that image has nokillall).Rollouts
N/A (process launch plumbing).
Compatibility and benchmark impact
Adapters that don't opt in keep their runner command line. Results can move slightly: this removes spurious reward-0 interruptions (~1% of Codex trials on Terminal-Bench-Science).