fix(backends): rewrite quoted virtual paths containing whitespace (#269)

* fix(backends): rewrite quoted virtual paths containing whitespace The `convert_virtual_paths_in_command` regex `(?<=\s)/[^\s;|&<>'"`]*` stopped at the first whitespace or quote, so: - `python "/skills/my skill/main.py"` was left completely unchanged (the `(?<=\s)` lookbehind failed after the opening `"`), and the shell then broke the inner unquoted path at the embedded space. - `python /skills/my skill/main.py` was truncated to `python ./skills/my skill/main.py` (only `/skills/my` rewritten). Replace the regex with `shlex.shlex(command, posix=True, punctuation_chars=";|&<>")` so quoted regions stay whole, then splice the rewrite back into the original command — extending the splice span to include any matching quote chars around the path so the fresh `shlex.quote` of the replacement isn't double-wrapped. `_resolve_virtual_mount_path` now returns the unquoted path; the caller owns shell-quoting, which avoids the previous `shlex.quote` inside original `"…"` leaving literal `'` chars in the argument value. Unquoted paths with embedded whitespace remain a known limitation (shlex has no way to know the user meant one path) — the workaround of avoiding spaces in skill directory names still applies, as flagged in the original issue. Closes #237 * fix(backends): backslash-escaped paths, multi-path per token, subshell paths - Fix backslash-escape handling: use unescape before rewriting - Fix re.search→re.finditer: all /-paths in a token are rewritten - Keep ( ) and backticks inside word tokens so paths spanning \ or wrapped in backticks are matched correctly - Add _try_rewrite helper with URL detection and unescape logic - Add 10 contract tests pinning the din0s review cases * fix(backends): restore () and backtick as shell operators for validate_command - Restore ( ) and backtick to the operator set in _shell_token_spans. Removing them caused a security regression: commands like (sudo ls) would not detect sudo as a blocked command because (sudo became one word token. With operators restored, validate_command correctly catches blocked commands inside subshells and command substitutions. - Fix _value_span_to_raw_span: the 'quoted' flag from the tokenizer means the token *contains* a quoted segment (not necessarily starts with a quote). Replace raw[0] assumption with a forward scan for the first quote char, consuming unquoted prefix chars 1:1. - Update test_system_path_with_shell_expansion: paths are now partially rewritten because () are operators. Test updated to reflect this known limitation (security > path rewriting). * fix(test): cross-platform compatibility for pre-existing Windows failures - python3 -> python in execute() calls (python is on PATH in any activated venv) - sleep 10 -> _sleep_cmd(10) cross-platform helper - str().endswith() -> Path().parts assertions (backslash-safe on Windows) - shlex.quote exact-match assertions -> 'in' assertions (Windows quotes paths differently) - mkdir -p E2E test -> preprocessor boundary test - Skip 3 E2E tests on Windows: shlex.quote produces POSIX quoting incompatible with cmd.exe 141 passed, 3 skipped on Windows. * fix: update docstring + strengthen shell-expansion test assertion - Fix _value_span_to_raw_span docstring: no longer assumes raw[0] is the opening quote, scans forward for first quote char - Strengthen test_system_path_with_shell_expansion: verify ./workspace/notes is rewritten, not just notes in result * style: ruff format backends.py + test_backends.py * refactor(backends): simplify quoted virtual path rewriting Replace 500+ line shlex tokenizer with 12-line pre-process step. Match quoted args via regex, unescape, rewrite via _rewrite_quoted_path, substitute with shlex.quote. 133 passed, 3 skipped. * fix: guard bare absolute paths from double-rewrite by post-process regex On POSIX, shlex.quote returns bare paths (e.g. /tmp/memories/note.md). The pre-process substitutes these into the command, then the post-process regex re-matches and incorrectly rewrites them. Fix: _guard_bare_absolute wraps bare /-paths in single quotes so the post-process regex''s character class stops at the quote char. * style: ruff format * fix(backends): narrow pre-process to exclude system-prefixed paths Only rewrite quoted paths that are NOT known system prefixes. * fix: narrow quoted-path pre-process to virtual mounts only Only rewrite quoted /... paths that resolve to actual virtual mounts (/skills/..., /memories/...) or workspace-prefixed system paths. Remove catch-all that incorrectly rewrote bare paths like echo /hi. Addresses din0s review feedback on #269. * docs: update docstring for narrower quoted-path rewrite scope
2026-06-14 00:56:30 +08:00
parent d82f0ed2a4
commit 8f9dfa159e
2 changed files with 181 additions and 33 deletions
@@ -627,36 +627,79 @@ def _resolve_virtual_mount_path(token: str) -> str | None:
    return None


+def _guard_bare_absolute(result: str | None) -> str | None:
+    """If *result* is a bare absolute path (no surrounding quotes),
+    single-quote it so the post-process regex won't re-rewrite it."""
+    if result and result.startswith("/") and result == result.strip("'\""):
+        return "'" + result + "'"
+    return result
+
+
+def _rewrite_quoted_path(
+    path: str,
+    workspace_name: str | None,
+) -> str | None:
+    """Return the shell-quoted replacement for *path* (the decoded
+    content of a quoted ``"..."`` or ``'...'`` argument),
+    or ``None`` if no rewrite applies.
+    """
+    if not path or "://" in path[max(0, len(path) - 10) :]:
+        return None
+    if not path.startswith("/"):
+        return None
+
+    resolved = _resolve_virtual_mount_path(path)
+    if resolved is not None:
+        return _guard_bare_absolute(resolved)  # already shlex.quoted
+
+    # Fix hallucinated system absolute paths that reference the workspace.
+    if workspace_name:
+        for prefix in _SYSTEM_PATH_PREFIXES:
+            if path.startswith(prefix):
+                marker = f"/{workspace_name}/"
+                idx = path.rfind(marker)
+                if idx != -1:
+                    relative = path[idx + len(marker) :]
+                    return _guard_bare_absolute(
+                        shlex.quote("./" + relative if relative else ".")
+                    )
+                if path.endswith(f"/{workspace_name}"):
+                    return _guard_bare_absolute(shlex.quote("."))
+                break
+
+    return None
+
+
 def convert_virtual_paths_in_command(
    command: str,
    workspace_name: str | None = None,
 ) -> str:
-    """
-    Convert virtual paths (starting with /) in commands to relative paths.
+    """Convert virtual paths (starting with ``/``) in commands to relative paths.

    Also auto-corrects hallucinated system absolute paths that reference the
    workspace directory (e.g. ``/Users/.../myproject/file.py`` → ``./file.py``).

-    Tier-aware mounts (``/skills/...``, ``/memories/...``) are expanded to
-    absolute paths via ``_resolve_virtual_mount_path``. Callers that pass
-    the result through ``validate_command`` MUST whitelist the tier roots
-    via ``allow_prefixes`` to avoid false-positive system-path blocks.
-
-    Args:
-        command: Original command.
-        workspace_name: Basename of the workspace directory (e.g. ``"workspace"``,
-            ``"my-project"``).  When provided, system paths containing
-            ``/<workspace_name>/`` are auto-corrected.
-
-    Examples:
-        >>> convert_virtual_paths_in_command("python /main.py")
-        'python ./main.py'
-        >>> convert_virtual_paths_in_command("ls /")
-        'ls .'
-        >>> convert_virtual_paths_in_command(
-        ...     "mkdir -p /Users/u/proj/dir", workspace_name="proj")
-        'mkdir -p ./dir'
+    Pre-process: quoted arguments whose content resolves to a virtual
+    mount (``/skills/...``, ``/memories/...``) or a workspace-prefixed
+    system path are rewritten as a single shell token — this fixes #237
+    where ``python "/skills/my skill/main.py"`` was truncated at the
+    embedded space.  Bare quoted ``/...`` paths (e.g. ``echo "/hi"``)
+    are left untouched since their semantics are ambiguous.
+    After pre-processing, the original regex handles unquoted
+    paths and workspace-name correction as before.
    """
+    # Pre-process: rewrite quoted paths whose decoded content starts with /
+    command = re.sub(
+        r'(["\'])((?:\\.|(?!\1).)*?)\1',
+        lambda m: (
+            _rewrite_quoted_path(
+                re.sub(r"\\(.)", r"\1", m.group(2)),
+                workspace_name,
+            )
+            or m.group(0)
+        ),
+        command,
+    )

    def replace_virtual_path(match: re.Match[str]) -> str:
        path = match.group(0)
@@ -670,16 +713,10 @@ def convert_virtual_paths_in_command(
            return resolved

        # Fix hallucinated system absolute paths that reference the workspace.
-        # E.g. /Users/user/.../myproject/file.py → ./file.py
-        # This mirrors _resolve_path() logic but for shell command strings.
        if workspace_name:
            for prefix in _SYSTEM_PATH_PREFIXES:
                if path.startswith(prefix):
                    marker = f"/{workspace_name}/"
-                    # rfind, not find: the workspace's parent path may itself
-                    # contain "/<workspace_name>/" (e.g. dev tree under
-                    # ~/workspace/.../workspace). Last occurrence is the
-                    # boundary closest to the file.
                    idx = path.rfind(marker)
                    if idx != -1:
                        relative = path[idx + len(marker) :]
@@ -691,8 +728,7 @@ def convert_virtual_paths_in_command(
        # Convert virtual path
        if path == "/":
            return "."
-        else:
-            return "." + path
+        return "." + path

    # Match pattern: paths starting with / (but not URLs)
    pattern = r'(?<=\s)/[^\s;|&<>\'"`]*|^/[^\s;|&<>\'"`]*'
@@ -213,6 +213,118 @@ class TestConvertVirtualPaths:
        result = convert_virtual_paths_in_command(command)
        assert result == "ssh host 'ls ./home/username/project'"

+    def test_bare_quoted_path_left_alone(self):
+        """A quoted bare ``/...`` path that is not a virtual mount
+        (``/skills/...``, ``/memories/...``) or workspace path must
+        NOT be rewritten — we cannot textually distinguish a path
+        argument from a literal string without command semantics.
+        """
+        result = convert_virtual_paths_in_command('python "/main file.py"')
+        assert result == 'python "/main file.py"'
+
+    def test_quoted_skills_path_with_whitespace_in_skill_name_resolved(
+        self, monkeypatch, tmp_path
+    ):
+        """A quoted ``/skills/<name with space>/...`` path must be
+        resolved as a single token, not truncated at the space (was:
+        the regex stopped at the first whitespace, so the resolver
+        received ``/skills/<word>`` and the suffix landed as a separate
+        argument).
+        """
+        # Tier setup identical to TestVirtualMountResolution._setup_tiers
+        user_dir = tmp_path / "ws_skills"
+        global_dir = tmp_path / "global_skills"
+        builtin_dir = tmp_path / "builtin_skills"
+        memories_dir = tmp_path / "memories"
+        for d in (user_dir, global_dir, builtin_dir, memories_dir):
+            d.mkdir()
+        monkeypatch.setattr(paths, "USER_SKILLS_DIR", user_dir)
+        monkeypatch.setattr(paths, "GLOBAL_SKILLS_DIR", global_dir)
+        monkeypatch.setattr(paths, "MEMORIES_DIR", memories_dir)
+        monkeypatch.setattr(backends, "_BUILTIN_SKILLS_DIR", builtin_dir)
+        (builtin_dir / "find skills").mkdir()
+        (builtin_dir / "find skills" / "tool.py").write_text("print('ok')")
+
+        result = convert_virtual_paths_in_command(
+            'python "/skills/find skills/tool.py"'
+        )
+
+        tokens = shlex.split(result)
+        assert tokens[0] == "python"
+        assert tokens[1] == str(builtin_dir / "find skills" / "tool.py")
+
+    def test_quoted_system_path_with_workspace_and_whitespace_corrected(self):
+        """A quoted system path that references the workspace dir name
+        (which itself contains a space) must be auto-corrected to the
+        workspace-relative form, not left as the original quoted string.
+        """
+        result = convert_virtual_paths_in_command(
+            'python "/Users/user/my project/src/main.py"',
+            workspace_name="my project",
+        )
+        tokens = shlex.split(result)
+        assert tokens == ["python", "./src/main.py"]
+
+    def test_quoted_path_with_whitespace_round_trip_safe(self):
+        """A quoted ``/skills/...`` path with whitespace must round-trip
+        through ``shlex.split`` as a single token.
+        """
+        result = convert_virtual_paths_in_command(
+            'python "/skills/find skills/tool.py"'
+        )
+        tokens = shlex.split(result)
+        assert tokens[0] == "python"
+        assert len(tokens) == 2
+        assert "find skills" in tokens[1]
+
+    def test_quoted_system_path_left_alone(self):
+        """A quoted path starting with a system prefix (e.g. ``/bin/echo``)
+        must NOT be rewritten — the pre-process excludes known system
+        prefixes so ``validate_command`` can still inspect them."""
+        result = convert_virtual_paths_in_command('python "/bin/echo"')
+        assert result == 'python "/bin/echo"'
+
+    def test_bash_c_with_quoted_system_path_left_alone(self):
+        """``bash -c "/bin/echo hi"`` must NOT be rewritten — the
+        ``/bin/echo`` inside the quoted argument is a shell command body,
+        not a virtual path argument to be rewritten."""
+        result = convert_virtual_paths_in_command('bash -c "/bin/echo hi"')
+        assert result == 'bash -c "/bin/echo hi"'
+
+    def test_unresolvable_quoted_skills_path_uses_workspace_relative_form(
+        self, monkeypatch, tmp_path
+    ):
+        """A quoted ``/skills/...`` path that no tier contains falls
+        through to the workspace-relative ``./skills/<rel>`` form. The
+        splice re-quotes the result; if the new path has no
+        whitespace, ``shlex.quote`` is a no-op and the surrounding
+        quote chars are dropped cleanly.
+        """
+        # Tier setup so the resolver is in a known empty state.
+        for d in (
+            tmp_path / "ws_skills",
+            tmp_path / "global_skills",
+            tmp_path / "builtin_skills",
+            tmp_path / "memories",
+        ):
+            d.mkdir()
+        monkeypatch.setattr(paths, "USER_SKILLS_DIR", tmp_path / "ws_skills")
+        monkeypatch.setattr(paths, "GLOBAL_SKILLS_DIR", tmp_path / "global_skills")
+        monkeypatch.setattr(paths, "MEMORIES_DIR", tmp_path / "memories")
+        monkeypatch.setattr(
+            backends, "_BUILTIN_SKILLS_DIR", tmp_path / "builtin_skills"
+        )
+        result = convert_virtual_paths_in_command(
+            'python "/skills/never-installed/foo.py"'
+        )
+        assert result == "python ./skills/never-installed/foo.py"
+
+    def test_echo_bare_quoted_path_left_alone(self):
+        """``echo "/hi"`` must NOT be rewritten — a bare ``/hi`` is not a
+        virtual mount, so the pre-process must leave it alone."""
+        result = convert_virtual_paths_in_command('echo "/hi"')
+        assert result == 'echo "/hi"'
+

 # === tier-aware virtual mounts (/skills/, /memories/) ===

@@ -675,7 +787,7 @@ class TestResolvePath:
        backend = CustomSandboxBackend(root_dir=tmp_workspace, virtual_mode=True)
        # /workspace/main.py should resolve to root/main.py
        resolved = backend._resolve_path("/workspace/main.py")
-        assert str(resolved).endswith("main.py")
+        assert Path(resolved).parts[-1] == "main.py"
        assert "workspace/workspace" not in str(resolved)

    def test_workspace_root(self, tmp_workspace):
@@ -687,13 +799,13 @@ class TestResolvePath:
    def test_system_path_with_workspace_marker(self, tmp_workspace):
        backend = CustomSandboxBackend(root_dir=tmp_workspace, virtual_mode=True)
        resolved = backend._resolve_path("/Users/someone/project/workspace/main.py")
-        assert str(resolved).endswith("main.py")
+        assert Path(resolved).parts[-1] == "main.py"

    def test_system_path_without_workspace(self, tmp_workspace):
        backend = CustomSandboxBackend(root_dir=tmp_workspace, virtual_mode=True)
        resolved = backend._resolve_path("/Users/someone/file.py")
        # Falls back to basename
-        assert str(resolved).endswith("file.py")
+        assert Path(resolved).parts[-1] == "file.py"

    def test_custom_workspace_name_prefix_stripped(self, tmp_path):
        """_resolve_path uses the actual dir name, not hardcoded 'workspace'."""
@@ -701,7 +813,7 @@ class TestResolvePath:
        ws.mkdir()
        backend = CustomSandboxBackend(root_dir=str(ws), virtual_mode=True)
        resolved = backend._resolve_path("/my-project/main.py")
-        assert str(resolved).endswith("main.py")
+        assert Path(resolved).parts[-1] == "main.py"
        assert "my-project/my-project" not in str(resolved)

    def test_custom_workspace_name_system_path(self, tmp_path):