Skip to content

fix(sandbox): keep command paths POSIX on a Windows host - #4958

Open
hyeonsang010716 wants to merge 1 commit into
openai:mainfrom
hyeonsang010716:fix/sandbox-posix-command-paths
Open

fix(sandbox): keep command paths POSIX on a Windows host#4958
hyeonsang010716 wants to merge 1 commit into
openai:mainfrom
hyeonsang010716:fix/sandbox-posix-command-paths

Conversation

@hyeonsang010716

Copy link
Copy Markdown
Contributor

Summary

Three sandbox commands built an argument with str() on a host-native Path. On a Windows host that
yields backslashes, and a Linux sandbox reads a backslash as an ordinary filename character rather
than a separator, so each command silently addressed the wrong path.

  • GitRepo.apply sent \workspace\repo/ as the cp destination. GNU cp -R -- src/. '\workspace\repo/'
    exits 0 and creates a directory with that literal name, so the clone was misplaced and the real
    destination stayed empty with no error to surface. I confirmed the exit status and the resulting
    layout against real coreutils 9.4 rather than inferring it.
  • SandboxMemoryStorage.ensure_text_file probed test -f \workspace\memories\MEMORY.md, which can
    never match. ensure_layout therefore treated an existing file as absent and overwrote it with an
    empty one. It runs on every rollout enqueue and on flush, so a user's MEMORY.md was lost
    repeatedly.
  • E2BSandboxSession.mkdir probed the parent the same way, so every mkdir without parents=True
    raised ExecNonZeroError even when the parent existed.

Each site now uses sandbox_path_str, which entries/mounts/patterns.py, sandboxes/docker.py,
runtime.py and the rest of e2b/sandbox.py already apply to sandbox command arguments.
.agents/references/sandbox-runtime-boundary.md states the rule these three sites broke.

Scope. The change is limited to values that become sandbox command arguments. I swept every
exec/exec_stream argument in src/agents/sandbox and src/agents/extensions/sandbox and these
three are the complete set; the remaining callers already normalize, and util/github.py correctly
uses str() because it runs git on the host through subprocess.

Two deliberate exclusions, happy to fold either in if you would prefer:

  • Sites that only serialize a path for error context or instrumentation (GitCopyError(dest=...),
    memory/manager.py:189, sandboxes/docker.py:520, base_sandbox_session.py:795). These produce
    backslashes in Windows error text but break no command, and including them would widen the rule
    from "command argument" to "command argument or serialized value" without covering every sibling.
  • _prepare_exec_command, where str(c) stringifies a Path at the public exec(*command: str | Path)
    boundary. Normalizing there fixes none of the three bugs above, because each call site stringifies
    before reaching it, and no first-party caller passes a bare Path. It is worth doing as
    defense-in-depth, but as an untested behavior change at a public boundary it does not belong in a
    bug fix.

Test plan

  • tests/sandbox/test_entries.py: test_git_repo_copies_to_a_posix_destination_from_a_windows_host
    drives GitRepo.apply with a PureWindowsPath destination and asserts the cp argument and that no
    argument contains a backslash. PureWindowsPath is flavour-fixed, so this case reproduces the
    host-native shape and fails on every platform, not only Windows. I verified that on Linux directly.
  • tests/sandbox/test_posix_tool_paths.py: test_memory_layout_probes_existing_files_with_posix_paths
    asserts the exact probe arguments ensure_layout issues. It lives here rather than in
    sandbox/test_memory.py because that file is in the Windows collect_ignore list, where the bug is
    the only place it reproduces.
  • tests/extensions/sandbox/test_e2b.py: test_e2b_mkdir_probes_the_parent_with_a_posix_path asserts
    the parent probe. Before the fix the recorded command is test -d '\workspace\sub'.
  • Reverting only src/ fails 9 cases: the 3 new ones plus the 6 existing cp assertions, which now
    detect the bug on Windows as well.
  • Two existing assertions in test_entries.py applied .replace("\\", "/") to the cp destination
    before comparing, which is exactly what hid this bug. They now compare the argument directly.
  • All three test files are collected on Windows, so the tests-windows job exercises them. The memory
    and E2B cases detect the regression only on that leg, since a native Path already stringifies to
    POSIX on Linux; the GitRepo case guards every leg.
  • ruff format --check, ruff check, check_optional_truthiness.py src/agents, mypy src (312 files)
    and pyright are clean.
  • Parallel suite: 8570 passed, 178 skipped. Serial suite: 77 passed, 4 skipped. The 12 sandbox failures
    are the pre-existing symlink-privilege ones (Sandbox tar extraction and 12 sandbox tests fail on Windows without symlink privilege #4852); I confirmed a pristine tree on this host
    produces the same 12.
  • make is not installed here, so the Makefile targets were run directly in the script's order:
    format, lint, typecheck, then the parallel and serial suites.

Issue number

None.

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

Three sandbox commands built an argument with `str()` on a host-native
`Path`. On a Windows host that yields backslashes, which a Linux sandbox
reads as an ordinary filename rather than a separator.

`GitRepo.apply` sent `\workspace\repo/` as the `cp` destination, so the
clone landed in a directory with that literal name and the real
destination stayed empty. `cp` exits 0, so the misplacement was silent.
`SandboxMemoryStorage.ensure_text_file` probed with `test -f` on a
backslash path that can never match, so `ensure_layout` replaced an
existing `MEMORY.md` with an empty file on every rollout enqueue and
flush. `E2BSandboxSession.mkdir` probed the parent the same way, so every
`mkdir` without `parents=True` raised `ExecNonZeroError` even when the
parent existed.

Each site now uses `sandbox_path_str`, the helper the surrounding code
already applies to sandbox command arguments.

Two assertions in `test_entries.py` normalized separators before
comparing, which masked the `cp` destination bug. They now compare the
argument directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants