fix: Atom C fail-closed Bubblewrap boundary - #35
Conversation
Implements the accepted 18:16 contract as the last integrity atom: - justai/sandbox.py: pure argv builder + fail-closed runner. --unshare-all --unshare-net --die-with-parent --new-session --cap-drop ALL --clearenv; fresh /proc, minimal /dev, tmpfs /tmp; ro loader roots (/usr /bin /sbin /lib /lib64 + narrow /etc/alternatives for Debian python3 symlink chain); exactly one rw bind = the task workdir; explicit env allowlist (PATH forced, LANG/LC_*/TZ pass-through only); timeout kills the whole process group. No unsandboxed escape hatch: SandboxUnavailable when bwrap is absent and the command never runs. - agent_dispatch: BOTH arbitrary-shell surfaces routed through the boundary — _run_local_command (model-produced action) and _verify_task (success criteria). Sandbox-unavailable maps to pre-execution 'error' (reason reaches the operator), never executed/executed_failed; verify fails closed. _task_workdir(): JUSTAI_TASK_WORKDIR else per-process private scratch. - tests/test_sandbox_boundary.py: 15 RED-first tests — argv contract, env allowlist (JUSTAI_LLM_KEY dropped), fail-closed (Popen never called), AST no-escape-hatch guard, timeout killpg, dispatch wiring (subprocess.run unreachable), SandboxUnavailable->error effect, + 2 LIVE effect proofs (greeting.py creatable in workdir; sibling/parent/network denied; host untouched; end-to-end dispatch greeting) — live on this host's bwrap 0.11.1. - tests/test_local_execution.py: adapted to Atom C semantics — task workdir = tmp_path (artifact assertions still prove the real host effect through the rw bind); real-execution tests marked needs_bwrap. RED: 15/15 fail pre-fix. GREEN: 15/15; full suite 536 passed + 14 subtests. AgentDispatchPipeline._run_tests (legacy, non-orchestrated path) deliberately out of slice — flagged in PR body. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPortjb8U5oteKxppGuVDQ
…result (PR #34 P2) Codex 0106 P2: bwrap dying during namespace/mount setup (before launching COMMAND) returned a normal nonzero SandboxResult, which dispatch mislabeled executed_failed; a Popen startup OSError was likewise swallowed into an executed label. Both contradict the integrity invariant: construction failure means the command did not run. - run_sandboxed now passes --json-status-fd on a private pipe; bwrap emits {"child-pid": N} only once the command process is actually forked. - No child-pid after exit -> SandboxUnavailable (with bwrap's stderr tail); covers nonzero AND zero-exit impostors, and unsupported/old bwrap fails closed (unknown option -> setup error, no child event). - Popen startup OSError -> SandboxUnavailable('failed to start bwrap: ...'). - Timeout with no child-pid -> SandboxUnavailable (construction stalled); process group still killed. Timeout after launch unchanged (timed_out). - child-pid observed -> nonzero stays a genuine executed result. Tests (RED-first, 7 failed pre-fix): argv --json-status-fd placement; mocked setup-failure -> SandboxUnavailable; launched-nonzero contrast stays executed; Popen OSError; timeout-before-launch (killpg still called); LIVE no-mock impostor test (/bin/false AND /bin/true as bwrap_path both raise); existing timeout test updated to represent launched-then-hung. Existing live greeting proofs now additionally prove the status channel end-to-end on real bwrap (success requires the child-pid event). Focused 21 passed; full suite 542 passed + 14 subtests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VPortjb8U5oteKxppGuVDQ
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a fail-closed Bubblewrap (“bwrap”) sandbox boundary for all model-produced local shell execution and for task verification, ensuring commands never run unsandboxed when the boundary can’t be constructed.
Changes:
- Add
justai/sandbox.pywith a pure argv builder, strict env allowlist, a single writable bind (task workdir), and--json-status-fd-based detection of “child never launched” setup failures. - Route
agent_dispatch._run_local_commandandagent_dispatch._verify_taskthroughrun_sandboxed, propagating fail-closed behavior to operator-visible outcomes. - Add focused unit/live-effect tests validating isolation flags, fail-closed semantics, process-group cleanup, and dispatch wiring; adapt local execution tests to the per-task workdir bind.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_sandbox_boundary.py | New contract tests for argv construction, fail-closed behavior, timeout cleanup, setup-failure classification, and dispatch wiring (plus optional live bwrap proofs). |
| tests/test_local_execution.py | Updates local-exec regression tests to run with the sandbox workdir semantics and skip live exec tests when bwrap is unavailable. |
| justai/sandbox.py | New Bubblewrap boundary implementation (env allowlist, RO binds, status-fd launch proof, timeout process-group cleanup). |
| justai/agent_dispatch.py | Routes execution + verification through the sandbox and introduces a stable task workdir mechanism. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -485,19 +507,21 @@ def _verify_task(task: Task) -> tuple[bool, str]: | |||
| return None, "no automated verification (task not confirmed done)" | |||
| if not _FALLBACK_WORKDIR: | ||
| _FALLBACK_WORKDIR.append(tempfile.mkdtemp(prefix="justai-work-")) | ||
| return _FALLBACK_WORKDIR[0] |
| proc = subprocess.Popen( | ||
| argv, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| stdin=subprocess.DEVNULL, | ||
| start_new_session=True, | ||
| text=True, | ||
| pass_fds=(write_fd,), | ||
| ) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bb48af566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| candidate = bwrap_path or shutil.which("bwrap") | ||
| if not candidate: | ||
| raise SandboxUnavailable("bwrap not found on PATH; refusing to run unsandboxed") |
There was a problem hiding this comment.
When bwrap is absent, every local action and verification now fails before execution, but neither install.sh, pyproject.toml, the runtime preflight, nor the README installs or checks this new system prerequisite. On a fresh supported installation without Bubblewrap, the installer's final pytest invocation currently reports five failures and justai run --local cannot complete any task; the installer/preflight needs to provision or explicitly reject this configuration before exposing local execution.
Useful? React with 👍 / 👎.
| if not _FALLBACK_WORKDIR: | ||
| _FALLBACK_WORKDIR.append(tempfile.mkdtemp(prefix="justai-work-")) | ||
| return _FALLBACK_WORKDIR[0] |
There was a problem hiding this comment.
When JUSTAI_TASK_WORKDIR is not manually set—which is the case for the documented CLI and orchestrator paths—this selects a new empty scratch directory instead of the project being operated on. Consequently exploration and relative-path edits run against an empty directory, and a task can even verify successfully while leaving the user's checkout unchanged; connect the run's project/workdir to this boundary or require it explicitly rather than silently falling back.
Useful? React with 👍 / 👎.
| src = os.environ if host_env is None else host_env | ||
| wd = str(Path(workdir).resolve()) | ||
| env: dict[str, str] = {"PATH": "/usr/bin:/bin", "HOME": wd, "TMPDIR": "/tmp"} |
There was a problem hiding this comment.
For the documented virtualenv installation flow, replacing PATH with /usr/bin:/bin causes sandboxed python3 and pytest commands to bypass the environment that contains JustAi's dependencies. This directly breaks planner-generated criteria such as python3 -m pytest ... whenever pytest is installed only in the activated .venv (as in a normal fresh install), even if the project workdir is configured correctly; expose a controlled project toolchain path or invoke the configured interpreter explicitly.
Useful? React with 👍 / 👎.
Independent review of PR #45: remounting the root read-only turned scratch writes by TMPDIR-unaware tools (/var/tmp, /run lock/pid files) from silently ephemeral into hard EROFS failures. That buys no correctness -- the intent there is already ephemeral -- so give them their own tmpfs. Added the ordering guard comment: every filesystem op must stay above the remount. Not needed: the reviewer's concern that a failed remount would masquerade as a command failure is already covered -- a setup failure produces no child-pid status event, so run_sandboxed raises SandboxUnavailable (#35). Focused 27/27; full suite 579 + 14 subtests.
* fix(sandbox): remount the ephemeral root read-only after the binds
A write to a path outside the workdir landed on the sandbox's own tmpfs root,
exited 0, and vanished. The executor therefore reported success for an effect
that never existed; only the later verification step (a separate sandbox with a
fresh tmpfs) collapsed it, and nothing in the receipts named the boundary.
--remount-ro / after every bind makes that write fail with EROFS instead.
Submounts keep their own flags, so the workdir bind, tmpfs /tmp, /proc and /dev
are unaffected. Measured before/after on bwrap 0.11.1: outside write rc 0 -> 1
('Read-only file system'), workdir write rc 0, /tmp write rc 0, interpreter
rc 0, host never touched.
This also makes the boundary hint added in #43 fire with correct attribution,
since the command now actually fails.
Focused 26/26; full suite 578 + 14 subtests.
* review fix: keep /var/tmp and /run writable as ephemeral scratch
Independent review of PR #45: remounting the root read-only turned scratch
writes by TMPDIR-unaware tools (/var/tmp, /run lock/pid files) from silently
ephemeral into hard EROFS failures. That buys no correctness -- the intent
there is already ephemeral -- so give them their own tmpfs. Added the ordering
guard comment: every filesystem op must stay above the remount.
Not needed: the reviewer's concern that a failed remount would masquerade as a
command failure is already covered -- a setup failure produces no child-pid
status event, so run_sandboxed raises SandboxUnavailable (#35).
Focused 27/27; full suite 579 + 14 subtests.
Supersedes #34 with the exact reviewed Atom C tree plus the final bounded prelaunch-classification closure.
--json-status-fd; setup failure isSandboxUnavailable, neverexecuted_failed.PopenstartupOSError/ValueErrorand status-pipe allocation failure are pre-execution errors.error/ command not executed.Source-only. No install, executor, productive artifact, credential, or runtime effect.