fix(sandbox): verify the sandbox-user kill before scoring

[tulerfeng]

Summary

Fixes #1078.

The second pass over the sandbox user's processes ended on sleep 1. A shell takes its exit status from the last command it ran, so that pass reported success no matter what came before it — including a pkill that failed outright.

This is worth separating from the report, because it changes the fix. The return value did go unchecked, but checking it alone would not have helped: the value was never anything other than 0. The check has to move into the command. So this PR makes the command end on the observation it is supposed to be making, and then checks the result with the existing _checked_exec.

Why a survivor matters

_freeze_workspace chowns the workspace to root and describes itself as "belt-and-suspenders against any zombie sandbox-user process that survived the pkill above". That backstop does not hold. POSIX checks permissions at open(), so a descriptor a process already holds keeps working through an ownership change.

I ran a sandbox-user process writing to a file in its workspace, then chowned the workspace to root and removed group and other write permission while it ran:

file owner after chown	root
lines before chown	3
lines 3s later	10

So a process that outlives the kill really can write into a workspace while it is being scored, which is what this step exists to prevent.

Testing

Ten tests, in two groups. The mocked ones drive _kill_sandbox_user_procs; the rest run the command string through a real /bin/sh, because the old form produced its successful status itself and no mock of env.exec can show that.

tests/test_sandbox_hardening.py	previous version	PR version
result	3 failed, 102 passed, 5 skipped	110 passed

The 5 skips are the tests that specify the new command; on a tree without it they report "not applicable" rather than failing, so the 3 failures are the defect and nothing else. One of the new tests asserts that the old command returns 0 while a process survives — it passes on both trees, and keeps the regression stated rather than described.

Full suite: 5893 passed, with 11 pre-existing failures in tests/test_cli_live_progress.py that are identical on an unmodified tree (11 failed, 54 passed there too). ruff check, ruff format --check and ty check are clean.

End-to-end verification

Real containers, driving the real _kill_sandbox_user_procs through an env whose exec is a real docker exec. The agent process, the shell, pgrep and the exit codes are the container's own. In the first case pkill is replaced with a no-op so that a real process really does survive a real kill attempt.

scenario (ubuntu:24.04)	previous version	PR version
agent process the kill cannot remove (pid 29 alive throughout)	returned — verifier would start	raised, naming the pid
normal: live agent process, kill works	returned	returned — not broken

The second row is the guard against over-correction, and it is the one that shaped the zombie handling below.

I also ran a full bench eval run on hello-world-task with --agent oracle --sandbox docker and the default sandbox user: 1/1 passed, mean reward 1.00, 0 errors. That is a regression check rather than evidence of the defect — its value is that the new command really does execute on the scoring path, which I confirmed with a temporary probe rather than assuming.

Two notes on the implementation

Zombies are excluded from the count. pgrep lists a defunct process like a live one, but a zombie holds no descriptors and cannot write, so it is not what this step guards against. Excluding by state also keeps the verdict from resting on whether PID 1 reaps orphans — the compose images run a shell there, which does reap, but that is a property of the image. The state comes from /proc/<pid>/status rather than ps -o stat=, which busybox does not support.

The first pkill stays unchecked, on purpose. It exits non-zero when there was simply nothing to kill, so checking it would fail every rollout whose agent had already exited on its own. Only the second pass carries a verdict. The function also still makes exactly two exec calls, so the acceptance test in the report runs unchanged.

I checked the rest of the repository for the same shape and found no other command that ends on a sleep and discards the status of what came before it.

---

[devin-ai-integration]

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

[devin-ai-integration]

🟡 Exited processes falsely block scoring

When a process exits during live, its missing status is treated as proof it survived. The stale PID aborts an otherwise valid score.

Prompt for agents

In src/benchflow/sandbox/lockdown.py, make the live shell helper distinguish a process whose /proc/<pid>/status disappeared after pgrep from a readable status that identifies a live process. A PID exiting between enumeration and inspection must be skipped, while genuine inspection failures must not silently establish quiescence. Add a real-shell regression test that simulates pgrep returning a PID whose status vanishes before inspection.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[tulerfeng]

Good catch, and it was real — I reproduced it before changing anything. A stub pgrep printing a pid with no /proc entry made the command exit 1 and name that pid, which on the scoring path would abort a valid run. The second scan is where it matters most, since it runs right after the kill while processes are dropping.

Fixed in 744231b. A pid now counts only on evidence it is still there: the state is read once, an empty read means the entry is gone, and the two non-survivor cases are kept apart in the comment — gone, and defunct.

Verified in a container both ways: a pid with no /proc entry exits 0, while a real unkillable agent process still exits 1 and reports its pid. There is a regression test for it, and deleting the new guard turns that test red on its own.