fix(docker): expand ~ and $VAR in extra_mounts destinations (#128)

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: bai-uipath

src/coder_eval/isolation/docker_runner.py

@@ -290,11 +290,14 @@ 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.
 
     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.
@@ -312,26 +315,35 @@ 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.")
+    # 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("/"):
-        raise ValueError(f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path.")
+        # 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}.")
     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.
+    # 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(

tests/test_docker_runner_mounts.py

@@ -108,6 +108,41 @@ 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."""
+        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 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."""
+        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."""
+        monkeypatch.setenv("SNEAKY", "/work")
+        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")