From 56b97430eb1f57b9077e35d623c52bcd4e56967e Mon Sep 17 00:00:00 2001 From: Bai Li Date: Thu, 20 Aug 2026 18:05:40 -0700 Subject: [PATCH 1/3] fix(docker): expand ~ and $VAR in extra_mounts destinations The source side of an extra_mounts spec is normalized with expandvars(expanduser(...)) so authors can write portable specs; the destination was only checked for a leading "/" and never expanded. That asymmetry makes one common mount impossible to write portably. env_passthrough forwards HOME with the HOST value on purpose, so any container-side path that must line up with $HOME -- $HOME/.uipath for the uip CLI's saved login state, for instance -- has a different literal value on every host. The only way to express it was to hardcode one host's home directory, which then mounts to the wrong place everywhere else. A login state the CLI cannot see fails tasks as a capability problem rather than a config one, so the misconfiguration is close to invisible: it cost 26% of the rows in an ad-hoc Maestro run before it was spotted. Expand the destination the same way, before the absolute-path check, so `~/.uipath:$HOME/.uipath:rw` resolves. Two details worth keeping: - expandvars leaves an unset variable verbatim, so a typo'd name still fails the absolute-path check. The message now shows the raw and the expanded form, otherwise it reads as a puzzle. - the framework-owned-mount check runs on the expanded destination, since a variable could itself expand to /work or / and the raw form would sail past the gate. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/isolation/docker_runner.py | 36 ++++++++++++++++++----- tests/test_docker_runner_mounts.py | 28 ++++++++++++++++++ 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/coder_eval/isolation/docker_runner.py b/src/coder_eval/isolation/docker_runner.py index 2331e4ec..0b97aeda 100644 --- a/src/coder_eval/isolation/docker_runner.py +++ b/src/coder_eval/isolation/docker_runner.py @@ -290,9 +290,19 @@ def _validate_extra_mount(spec: str) -> str: Defends against typos that would silently expose the host fs to the container, and against mount specs that shadow framework-owned mounts. - Normalizes the source side by expanding ``~`` and ``$VAR`` so authors - can write portable specs. Returns the (possibly rewritten) spec to - feed back into argv. + Normalizes BOTH sides by expanding ``~`` and ``$VAR`` so authors can + write portable specs. Returns the (possibly rewritten) spec to feed + back into argv. + + The destination is expanded for the same reason the source is, and it + matters more than it looks: ``env_passthrough`` forwards ``HOME`` with + the HOST value on purpose, so a container-side path that must line up + with ``$HOME`` (``$HOME/.uipath`` for the uip CLI's login state, say) + has a different literal value on every host. Without expansion the only + way to write that mount is to hardcode one host's home directory, which + then silently mounts to the wrong place everywhere else -- and a login + state the CLI cannot see reads as a capability failure, not a config + error. Notes: - Mode is REQUIRED. Forgetting ``:ro`` is the single most common way @@ -312,26 +322,36 @@ def _validate_extra_mount(spec: str) -> str: parts = body.split(":") if len(parts) < 2 or len(parts) > 3: raise ValueError(f"Invalid extra_mounts entry {spec!r}: expected `src:dst[:ro|rw]`.") - src, dst = head + parts[0], parts[1] + src, raw_dst = head + parts[0], parts[1] # Default to read-only when mode is omitted. Mounting host paths RW # by default is the wrong sandbox stance: the few RW use-cases are # better stated explicitly than implied by silence. mode = parts[2] if len(parts) == 3 else "ro" if not src: raise ValueError(f"Invalid extra_mounts entry {spec!r}: empty source path.") - if not dst: + if not raw_dst: raise ValueError(f"Invalid extra_mounts entry {spec!r}: empty destination path.") + # Expand ~ and $VAR on BOTH sides so authors can write portable specs. + # Destination expansion happens BEFORE the absolute-path check, since the + # whole point is to let `$HOME/...` resolve to an absolute path. + expanded_src = os.path.expandvars(os.path.expanduser(src)) + dst = os.path.expandvars(os.path.expanduser(raw_dst)) if not dst.startswith("/"): - raise ValueError(f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path.") + # An unset variable is left verbatim by expandvars, so a typo'd name + # lands here. Show both forms or the message is a puzzle. + detail = f"{raw_dst!r}" if dst == raw_dst else f"{raw_dst!r} (expanded to {dst!r})" + raise ValueError( + f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path, got {detail}." + ) if mode not in ("ro", "rw"): raise ValueError(f"Invalid extra_mounts entry {spec!r}: mode must be 'ro' or 'rw'.") - # Expand ~ and $VAR in the source so authors can write portable specs. - expanded_src = os.path.expandvars(os.path.expanduser(src)) if not Path(expanded_src).exists(): raise ValueError(f"Invalid extra_mounts entry {spec!r}: source path does not exist on host.") # Reject destinations that shadow framework-owned mounts inside the # container. ``/work`` substrings are caught too -- /work/foo would # land underneath our staging dir and shadow the input/output tree. + # Checked on the EXPANDED destination: `$HOME` could itself expand to a + # reserved path, and the raw form would sail past this gate. dst_norm = dst.rstrip("/") or "/" if dst_norm in _RESERVED_MOUNT_DESTS or dst_norm.startswith(CONTAINER_WORK_DIR + "/"): raise ValueError( diff --git a/tests/test_docker_runner_mounts.py b/tests/test_docker_runner_mounts.py index 0459b8e0..d5f857eb 100644 --- a/tests/test_docker_runner_mounts.py +++ b/tests/test_docker_runner_mounts.py @@ -108,6 +108,34 @@ def test_var_expansion_in_source(self, real_dir, monkeypatch): result = _validate_extra_mount("$MYDIR:/mnt/x") assert result.startswith(real_dir + ":") + def test_var_expansion_in_destination(self, real_dir, monkeypatch): + """``$VAR`` in the destination expands too. + + ``env_passthrough`` forwards ``HOME`` with the host value, so a mount + that has to line up with the container's ``$HOME`` would otherwise have + to hardcode one host's home directory. + """ + monkeypatch.setenv("HOME", "/home/someuser") + result = _validate_extra_mount(f"{real_dir}:$HOME/.uipath:rw") + assert result == f"{real_dir}:/home/someuser/.uipath:rw" + + def test_home_expansion_in_destination(self, real_dir, monkeypatch): + """``~`` in the destination expands the same way the source's does.""" + monkeypatch.setenv("HOME", "/home/someuser") + result = _validate_extra_mount(f"{real_dir}:~/.uipath:ro") + assert result == f"{real_dir}:/home/someuser/.uipath:ro" + + def test_unset_var_destination_rejected_with_both_forms(self, real_dir): + """An unset var is left verbatim, so it must fail loudly, not mount blind.""" + with pytest.raises(ValueError, match="destination must be an absolute path"): + _validate_extra_mount(f"{real_dir}:$NO_SUCH_VAR_HERE/x:ro") + + def test_destination_var_expanding_to_reserved_is_rejected(self, real_dir, monkeypatch): + """The shadow check runs on the EXPANDED destination, not the raw one.""" + monkeypatch.setenv("SNEAKY", "/work") + with pytest.raises(ValueError, match="shadows a framework-owned mount"): + _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") + def test_malformed_no_colon(self): with pytest.raises(ValueError, match="expected `src:dst"): _validate_extra_mount("just-one-token") From f1e98e592e4db48d08342e4cffd6b8aa5bab408a Mon Sep 17 00:00:00 2001 From: Bai Li Date: Fri, 21 Aug 2026 17:58:37 -0700 Subject: [PATCH 2/3] docs(docker): trim the extra_mounts destination comments to scope The added docstring and test docstrings justified destination expansion with `$HOME`/`.uipath`, which is not a case this serves: skills experiments mount login state at a literal destination. Restate it against the one real consumer, a container path that has to match a host-valued var. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/isolation/docker_runner.py | 23 ++++++----------------- tests/test_docker_runner_mounts.py | 13 ++++--------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/src/coder_eval/isolation/docker_runner.py b/src/coder_eval/isolation/docker_runner.py index 0b97aeda..a7ae6d12 100644 --- a/src/coder_eval/isolation/docker_runner.py +++ b/src/coder_eval/isolation/docker_runner.py @@ -294,17 +294,10 @@ def _validate_extra_mount(spec: str) -> str: write portable specs. Returns the (possibly rewritten) spec to feed back into argv. - The destination is expanded for the same reason the source is, and it - matters more than it looks: ``env_passthrough`` forwards ``HOME`` with - the HOST value on purpose, so a container-side path that must line up - with ``$HOME`` (``$HOME/.uipath`` for the uip CLI's login state, say) - has a different literal value on every host. Without expansion the only - way to write that mount is to hardcode one host's home directory, which - then silently mounts to the wrong place everywhere else -- and a login - state the CLI cannot see reads as a capability failure, not a config - error. - Notes: + - Destinations are expanded too. A container path that has to match a + host-valued var (``$SKILLS_REPO_PATH``) would otherwise have to be + hardcoded per machine. - Mode is REQUIRED. Forgetting ``:ro`` is the single most common way to accidentally hand the container RW access to a host directory, so we make the author write it explicitly. @@ -331,14 +324,11 @@ def _validate_extra_mount(spec: str) -> str: raise ValueError(f"Invalid extra_mounts entry {spec!r}: empty source path.") if not raw_dst: raise ValueError(f"Invalid extra_mounts entry {spec!r}: empty destination path.") - # Expand ~ and $VAR on BOTH sides so authors can write portable specs. - # Destination expansion happens BEFORE the absolute-path check, since the - # whole point is to let `$HOME/...` resolve to an absolute path. + # Expanded before the absolute-path check: that is the point. expanded_src = os.path.expandvars(os.path.expanduser(src)) dst = os.path.expandvars(os.path.expanduser(raw_dst)) if not dst.startswith("/"): - # An unset variable is left verbatim by expandvars, so a typo'd name - # lands here. Show both forms or the message is a puzzle. + # expandvars leaves an unset var verbatim, so typos land here. detail = f"{raw_dst!r}" if dst == raw_dst else f"{raw_dst!r} (expanded to {dst!r})" raise ValueError( f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path, got {detail}." @@ -350,8 +340,7 @@ def _validate_extra_mount(spec: str) -> str: # Reject destinations that shadow framework-owned mounts inside the # container. ``/work`` substrings are caught too -- /work/foo would # land underneath our staging dir and shadow the input/output tree. - # Checked on the EXPANDED destination: `$HOME` could itself expand to a - # reserved path, and the raw form would sail past this gate. + # Expanded form: a var could itself expand to a reserved path. dst_norm = dst.rstrip("/") or "/" if dst_norm in _RESERVED_MOUNT_DESTS or dst_norm.startswith(CONTAINER_WORK_DIR + "/"): raise ValueError( diff --git a/tests/test_docker_runner_mounts.py b/tests/test_docker_runner_mounts.py index d5f857eb..bceda97f 100644 --- a/tests/test_docker_runner_mounts.py +++ b/tests/test_docker_runner_mounts.py @@ -109,29 +109,24 @@ def test_var_expansion_in_source(self, real_dir, monkeypatch): assert result.startswith(real_dir + ":") def test_var_expansion_in_destination(self, real_dir, monkeypatch): - """``$VAR`` in the destination expands too. - - ``env_passthrough`` forwards ``HOME`` with the host value, so a mount - that has to line up with the container's ``$HOME`` would otherwise have - to hardcode one host's home directory. - """ + """``$VAR`` in the destination expands too.""" monkeypatch.setenv("HOME", "/home/someuser") result = _validate_extra_mount(f"{real_dir}:$HOME/.uipath:rw") assert result == f"{real_dir}:/home/someuser/.uipath:rw" def test_home_expansion_in_destination(self, real_dir, monkeypatch): - """``~`` in the destination expands the same way the source's does.""" + """``~`` in the destination expands the same way the source does.""" monkeypatch.setenv("HOME", "/home/someuser") result = _validate_extra_mount(f"{real_dir}:~/.uipath:ro") assert result == f"{real_dir}:/home/someuser/.uipath:ro" def test_unset_var_destination_rejected_with_both_forms(self, real_dir): - """An unset var is left verbatim, so it must fail loudly, not mount blind.""" + """An unset var is left verbatim, so it must fail loudly.""" with pytest.raises(ValueError, match="destination must be an absolute path"): _validate_extra_mount(f"{real_dir}:$NO_SUCH_VAR_HERE/x:ro") def test_destination_var_expanding_to_reserved_is_rejected(self, real_dir, monkeypatch): - """The shadow check runs on the EXPANDED destination, not the raw one.""" + """The shadow check runs on the expanded destination.""" monkeypatch.setenv("SNEAKY", "/work") with pytest.raises(ValueError, match="shadows a framework-owned mount"): _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") From ef7fe4fdbc6fc9f525b5af565aefb06777bb6a9b Mon Sep 17 00:00:00 2001 From: Bai Li Date: Fri, 21 Aug 2026 18:07:11 -0700 Subject: [PATCH 3/3] fix(docker): reject expansions that inject a ':' into a mount path Two problems. `ruff format` wanted the destination error on one line, which failed the Quality Gate and the Windows Smoke Test. More importantly, a variable whose value carries a ':' added fields to the spec rebuilt at the bottom of the validator. `SNEAKY=/mnt/x:rw` in `$real:$SNEAKY:ro` produced `/real:/mnt/x:rw:ro`, moving the destination and widening a declared read-only mount. Guard both sides after expansion, excluding the Windows drive prefix whose colon is legitimate and already split off. Two tests cover it. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/isolation/docker_runner.py | 9 ++++++--- tests/test_docker_runner_mounts.py | 12 ++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/coder_eval/isolation/docker_runner.py b/src/coder_eval/isolation/docker_runner.py index a7ae6d12..31683d75 100644 --- a/src/coder_eval/isolation/docker_runner.py +++ b/src/coder_eval/isolation/docker_runner.py @@ -327,12 +327,15 @@ def _validate_extra_mount(spec: str) -> str: # Expanded before the absolute-path check: that is the point. expanded_src = os.path.expandvars(os.path.expanduser(src)) dst = os.path.expandvars(os.path.expanduser(raw_dst)) + # A variable whose value carries a ':' would add fields to the spec rebuilt + # at the bottom, silently moving the destination or widening the mode. + # The drive prefix is excluded: its colon is legitimate and already split off. + if ":" in dst or ":" in expanded_src[len(head) :]: + raise ValueError(f"Invalid extra_mounts entry {spec!r}: expansion introduced a ':' into a path.") if not dst.startswith("/"): # expandvars leaves an unset var verbatim, so typos land here. detail = f"{raw_dst!r}" if dst == raw_dst else f"{raw_dst!r} (expanded to {dst!r})" - raise ValueError( - f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path, got {detail}." - ) + raise ValueError(f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path, got {detail}.") if mode not in ("ro", "rw"): raise ValueError(f"Invalid extra_mounts entry {spec!r}: mode must be 'ro' or 'rw'.") if not Path(expanded_src).exists(): diff --git a/tests/test_docker_runner_mounts.py b/tests/test_docker_runner_mounts.py index bceda97f..f6d65e72 100644 --- a/tests/test_docker_runner_mounts.py +++ b/tests/test_docker_runner_mounts.py @@ -131,6 +131,18 @@ def test_destination_var_expanding_to_reserved_is_rejected(self, real_dir, monke with pytest.raises(ValueError, match="shadows a framework-owned mount"): _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") + def test_destination_var_carrying_a_colon_is_rejected(self, real_dir, monkeypatch): + """A ':' in an expanded value would add fields to the rebuilt spec.""" + monkeypatch.setenv("SNEAKY", "/mnt/x:rw") + with pytest.raises(ValueError, match="introduced a ':'"): + _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") + + def test_source_var_carrying_a_colon_is_rejected(self, monkeypatch): + """Same guard on the source side, which is rebuilt the same way.""" + monkeypatch.setenv("SNEAKY", "/mnt/x:rw") + with pytest.raises(ValueError, match="introduced a ':'"): + _validate_extra_mount("$SNEAKY:/mnt/y:ro") + def test_malformed_no_colon(self): with pytest.raises(ValueError, match="expected `src:dst"): _validate_extra_mount("just-one-token")