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

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")