feat(backends): use _platform_quote for Windows cmd.exe compatibility (#280)

* feat(backends): use _platform_quote for Windows cmd.exe compatibility Resolves the 3 skipped E2E tests in test_backends.py that exercised the /skills/... mount path. The path-rewriter was wrapping resolved absolute paths via shlex.quote (POSIX single-quote style); cmd.exe doesn't strip single quotes, so the literal ' characters ended up in the subprocess argv and the python script failed to find its file. Replace the 3 shlex.quote call sites in _resolve_virtual_mount_path with _platform_quote, a thin platform dispatcher: - POSIX: shlex.quote (unchanged) - Windows: _cmd_quote uses cmd.exe-compatible double-quote wrapping and properly escapes embedded " and percent signs Adds: - backends.py: _is_windows, _cmd_quote, _platform_quote (~40 lines) - test_backends.py: 6 TestPlatformQuote unit tests + _split_cmd cross-platform tokenizer helper to replace shlex.split in the 8 sites that tokenize convert_virtual_paths_in_command results (POSIX shlex strips backslashes from bare Windows paths, which broke the 5 TestVirtualMountResolution assertions on Windows) Removes: - 3 @pytest.mark.skipif(sys.platform == "win32") markers on the E2E tests for /skills/... mount resolution Refs #274. * fix: escape % as %% in _cmd_quote instead of relying on double-quoting cmd.exe expands %VAR% before processing quotes, so double-quoting cannot neutralize percent signs. Escape bare % as %% (the cmd.exe idiom for a literal percent) before any other quoting logic. Also updates _cmd_quote docstring and _resolve_virtual_mount_path docstring to reflect the actual quoting strategy. * style: fix ruff format (single → double quotes) * fix: treat % as regular char in _cmd_quote, document limitation %% escaping only collapses in .bat/.cmd files, not via cmd /c. Since virtual-mount paths should never contain % in practice, simpler to leave % alone and document the caveat.
2026-06-14 01:08:18 +08:00
parent 76972449c7
commit 477481f9d9
2 changed files with 160 additions and 36 deletions
@@ -3,6 +3,7 @@
 import os
 import re
 import shlex
+import sys
 import uuid
 from pathlib import Path

@@ -594,20 +595,65 @@ def _skills_tier_paths() -> tuple[Path, Path | None, Path]:
    return (paths.USER_SKILLS_DIR, paths.GLOBAL_SKILLS_DIR, _BUILTIN_SKILLS_DIR)


+def _is_windows() -> bool:
+    return sys.platform == "win32"
+
+
+def _cmd_quote(s: str) -> str:
+    """Quote *s* for cmd.exe using double-quote wrapping.
+
+    cmd.exe strips outer double quotes; content between them is taken
+    literally. Backslashes are not escape chars inside double quotes, so
+    Windows paths pass through unchanged. Embedded ``"`` is escaped as
+    ``\"``; bare paths with no shell-special chars need no quoting at all.
+
+    .. note::
+
+       ``%VAR%`` expansion is **not** neutralised here.  Variable expansion
+       happens before quote processing in cmd.exe, and ``%%`` collapsing
+       only occurs inside ``.bat``/``.cmd`` files — not via ``cmd /c``.
+       This is acceptable because virtual-mount paths (skills, memories)
+       should never contain percent signs in practice.
+
+    Mirrors the role of :func:`shlex.quote` for the Windows shell so the
+    sandbox command can pass a single token through :func:`subprocess.run`
+    with ``shell=True`` (which on Windows invokes cmd.exe, not /bin/sh).
+    """
+    if not s:
+        return '""'
+    if not any(c in s for c in ' \t\n"&|<>^()'):
+        return s
+    return '"' + s.replace('"', '\\"') + '"'
+
+
+def _platform_quote(s: str) -> str:
+    """Quote *s* for the host's default shell.
+
+    On POSIX, delegates to :func:`shlex.quote` (single-quote wrapping).
+    On Windows, uses double-quote wrapping compatible with cmd.exe —
+    see :func:`_cmd_quote`. The platform check is read at call time, so
+    tests can swap it via ``monkeypatch.setattr(backends, "_is_windows", ...)``
+    without mutating :mod:`sys` module state.
+    """
+    if _is_windows():
+        return _cmd_quote(s)
+    return shlex.quote(s)
+
+
 def _resolve_virtual_mount_path(token: str) -> str | None:
    """Resolve a virtual mount token to a shell-safe token, or ``None`` when
    *token* is not a registered virtual mount.

    For ``/skills/...``: walks ``_skills_tier_paths()`` priority (USER →
-    GLOBAL → BUILTIN), returning ``shlex.quote`` of the first tier where the
-    path exists. On miss, returns a workspace-relative ``./skills/<rel>``
-    form — agent typed a virtual path, so the shell error should reference a
-    location they recognise (`USER_SKILLS_DIR` defaults to
+    GLOBAL → BUILTIN), returning :func:`_platform_quote` of the first tier
+    where the path exists. On miss, returns a workspace-relative
+    ``./skills/<rel>`` form — agent typed a virtual path, so the shell error
+    should reference a location they recognise (`USER_SKILLS_DIR` defaults to
    ``WORKSPACE_ROOT / "skills"``, which is also where ``MergedSkillsBackend``
    would write a new skill).

    For ``/memories/...``: single tier (``paths.MEMORIES_DIR``), always
-    absolute and ``shlex.quote``-wrapped. Memories live outside the
+    absolute and :func:`_platform_quote`-wrapped. Memories live outside the
    workspace, so a relative form would point at an unrelated location.
    """
    rel = _subpath_under_mount(token, "/skills")
@@ -617,12 +663,12 @@ def _resolve_virtual_mount_path(token: str) -> str | None:
                continue
            candidate = Path(tier) / rel
            if candidate.exists():
-                return shlex.quote(str(candidate))
-        return shlex.quote("./skills/" + rel if rel else "./skills")
+                return _platform_quote(str(candidate))
+        return _platform_quote("./skills/" + rel if rel else "./skills")

    rel = _subpath_under_mount(token, "/memories")
    if rel is not None:
-        return shlex.quote(str(Path(paths.MEMORIES_DIR) / rel))
+        return _platform_quote(str(Path(paths.MEMORIES_DIR) / rel))

    return None

@@ -24,6 +24,47 @@ def _sleep_cmd(seconds: int) -> str:
    return f"sleep {seconds}"


+def _split_cmd(s: str) -> list[str]:
+    """Cross-platform tokenizer for shell command assertions.
+
+    POSIX (``shlex.split`` default ``posix=True``) handles single/double
+    quotes and backslash escapes produced by :func:`shlex.quote`. But
+    ``posix=True`` also treats ``\\`` as an escape char on input, which
+    would strip the backslashes from a bare Windows path like
+    ``C:\\Users\\foo`` — turning it into ``C:Usersfoo`` and breaking the
+    token comparison.
+
+    On Windows, the resolved paths from :func:`backends._platform_quote`
+    are bare (no shell-special chars) or double-quoted (when the path
+    has spaces). ``shlex.split(s, posix=False)`` is a simple whitespace
+    splitter that preserves backslashes verbatim; we then strip a
+    single layer of matching outer ``"``/``'`` and unescape ``\\"``
+    to mimic what cmd.exe does at parse time.
+
+    Examples (on Windows):
+
+    >>> _split_cmd('python C:\\\\Users\\\\foo\\\\bar.py')
+    ['python', 'C:\\\\Users\\\\foo\\\\bar.py']
+    >>> _split_cmd('python "C:\\\\Users\\\\John Smith\\\\bar.py"')
+    ['python', 'C:\\\\Users\\\\John Smith\\\\bar.py']
+    >>> _split_cmd('python "C:\\\\path\\\\a\\\\"b"')
+    ['python', 'C:\\\\path\\\\a"b']
+    """
+    if sys.platform == "win32":
+        tokens = shlex.split(s, posix=False)
+        # posix=False doesn't process quotes; mimic cmd.exe: strip a
+        # single layer of matching outer quotes per token, then
+        # unescape embedded \" → ".
+        result = []
+        for tok in tokens:
+            if len(tok) >= 2 and tok[0] == tok[-1] and tok[0] in "\"'":
+                tok = tok[1:-1]
+            tok = tok.replace('\\"', '"')
+            result.append(tok)
+        return result
+    return shlex.split(s)
+
+
 # === validate_command ===


@@ -361,10 +402,11 @@ class TestVirtualMountResolution:
        (global_dir / "hello").mkdir()
        (global_dir / "hello" / "main.py").write_text("print('global')")
        result = convert_virtual_paths_in_command("python /skills/hello/main.py")
-        # ``shlex.split`` round-trip is quote-style agnostic — the prior
-        # direct string compare broke on Windows where ``shlex.quote``
-        # adds single quotes around backslash paths.
-        assert shlex.split(result) == ["python", str(user_dir / "hello" / "main.py")]
+        # ``_split_cmd`` round-trip is cross-platform: on POSIX it parses
+        # shlex.quote-style output; on Windows it preserves the backslashes
+        # in bare paths (POSIX shlex would treat ``\`` as an escape char
+        # and strip them). See the helper docstring for details.
+        assert _split_cmd(result) == ["python", str(user_dir / "hello" / "main.py")]

    def test_skills_path_resolves_to_global_tier_when_workspace_missing(
        self, monkeypatch, tmp_path
@@ -373,7 +415,7 @@ class TestVirtualMountResolution:
        (global_dir / "hello").mkdir()
        (global_dir / "hello" / "main.py").write_text("print('global')")
        result = convert_virtual_paths_in_command("python /skills/hello/main.py")
-        assert shlex.split(result) == ["python", str(global_dir / "hello" / "main.py")]
+        assert _split_cmd(result) == ["python", str(global_dir / "hello" / "main.py")]

    def test_skills_path_resolves_to_builtin_tier_when_higher_missing(
        self, monkeypatch, tmp_path
@@ -382,7 +424,7 @@ class TestVirtualMountResolution:
        (builtin_dir / "find-skills").mkdir()
        (builtin_dir / "find-skills" / "tool.py").write_text("print('builtin')")
        result = convert_virtual_paths_in_command("python /skills/find-skills/tool.py")
-        assert shlex.split(result) == [
+        assert _split_cmd(result) == [
            "python",
            str(builtin_dir / "find-skills" / "tool.py"),
        ]
@@ -406,18 +448,18 @@ class TestVirtualMountResolution:
    ):
        _, _, _, memories_dir = self._setup_tiers(monkeypatch, tmp_path)
        result = convert_virtual_paths_in_command("cat /memories/note.md")
-        assert shlex.split(result) == ["cat", str(memories_dir / "note.md")]
+        assert _split_cmd(result) == ["cat", str(memories_dir / "note.md")]

    def test_skills_bare_root_resolves_to_user_skills_dir(self, monkeypatch, tmp_path):
        """Bare /skills and /skills/ (no subpath) resolve to USER_SKILLS_DIR;
        mirrors the existing `/` → `.` rule but for the mount root.
        """
        user_dir, _, _, _ = self._setup_tiers(monkeypatch, tmp_path)
-        assert shlex.split(convert_virtual_paths_in_command("ls /skills")) == [
+        assert _split_cmd(convert_virtual_paths_in_command("ls /skills")) == [
            "ls",
            str(user_dir),
        ]
-        assert shlex.split(convert_virtual_paths_in_command("ls /skills/")) == [
+        assert _split_cmd(convert_virtual_paths_in_command("ls /skills/")) == [
            "ls",
            str(user_dir),
        ]
@@ -573,7 +615,7 @@ class TestVirtualMountResolution:

        result = convert_virtual_paths_in_command("python /skills/hello/main.py")

-        tokens = shlex.split(result)
+        tokens = _split_cmd(result)
        assert tokens[0] == "python"
        assert tokens[1] == str(user_dir / "hello" / "main.py")

@@ -590,7 +632,7 @@ class TestVirtualMountResolution:

        result = convert_virtual_paths_in_command("cat /memories/note.md")

-        tokens = shlex.split(result)
+        tokens = _split_cmd(result)
        assert tokens[0] == "cat"
        assert tokens[1] == str(spacey / "note.md")

@@ -658,15 +700,6 @@ class TestVirtualMountResolution:
        assert result[1] == paths.GLOBAL_SKILLS_DIR
        assert result[2] == backends._BUILTIN_SKILLS_DIR

-    @pytest.mark.skipif(
-        sys.platform == "win32",
-        reason=(
-            "convert_virtual_paths_in_command wraps resolved paths in single "
-            "quotes via shlex.quote, which cmd.exe does not strip — the "
-            "literal ' chars end up in the subprocess argv. Tracked as a "
-            "follow-up to #207 (Windows-aware quoting in the convert fn)."
-        ),
-    )
    def test_execute_e2e_workspace_tier_skill(self, monkeypatch, tmp_path):
        """End-to-end: a skill in the workspace tier (USER_SKILLS_DIR) must
        execute successfully. Regression guard: USER_SKILLS_DIR must be in
@@ -702,10 +735,6 @@ class TestVirtualMountResolution:
        assert resp.exit_code == 0, resp.output
        assert "workspace-tier-fix-works" in resp.output

-    @pytest.mark.skipif(
-        sys.platform == "win32",
-        reason="see test_execute_e2e_workspace_tier_skill",
-    )
    def test_execute_e2e_workspace_tier_shadows_global(self, monkeypatch, tmp_path):
        """End-to-end: when the same skill exists in BOTH workspace and global
        tiers, the workspace version must shadow the global one when invoked
@@ -743,10 +772,6 @@ class TestVirtualMountResolution:
        assert "WORKSPACE_TIER_WINS" in resp.output
        assert "GLOBAL_TIER_LOST" not in resp.output

-    @pytest.mark.skipif(
-        sys.platform == "win32",
-        reason="see test_execute_e2e_workspace_tier_skill",
-    )
    def test_execute_e2e_global_tier_skill(self, monkeypatch, tmp_path):
        """End-to-end: a skill that exists ONLY in the global tier (workspace
        does not have a copy) must execute successfully via
@@ -1571,3 +1596,56 @@ class TestExecuteTimeoutRecovery:
        resp = backend.execute('python -c "raise SystemExit(1)"')
        assert resp.exit_code == 1
        assert "Recovery" not in resp.output
+
+
+class TestPlatformQuote:
+    """Unit tests for :func:`backends._platform_quote` / :func:`backends._cmd_quote`.
+
+    The platform check is read at call time via :func:`backends._is_windows`,
+    so we monkeypatch that function (not the ``sys`` module) to exercise the
+    Windows branch on a POSIX runner without mutating global state.
+    """
+
+    def test_posix_no_special_chars_returns_bare(self, monkeypatch):
+        monkeypatch.setattr(backends, "_is_windows", lambda: False)
+        # Forward slashes and alphanumerics are safe in POSIX shells.
+        assert backends._platform_quote("/Users/foo/file.py") == "/Users/foo/file.py"
+
+    def test_posix_path_with_space_is_single_quoted(self, monkeypatch):
+        monkeypatch.setattr(backends, "_is_windows", lambda: False)
+        # shlex.quote wraps the whole token in single quotes.
+        assert (
+            backends._platform_quote("/Users/foo/file bar.py")
+            == "'/Users/foo/file bar.py'"
+        )
+
+    def test_windows_no_special_chars_returns_bare(self, monkeypatch):
+        monkeypatch.setattr(backends, "_is_windows", lambda: True)
+        # Backslashes are NOT escape chars inside cmd.exe double quotes, and
+        # outside quotes they only appear in paths — so a bare path is fine.
+        assert (
+            backends._platform_quote(r"C:\Users\foo\file.py") == r"C:\Users\foo\file.py"
+        )
+
+    def test_windows_path_with_space_is_double_quoted(self, monkeypatch):
+        monkeypatch.setattr(backends, "_is_windows", lambda: True)
+        # cmd.exe strips outer double quotes; the space is preserved literally.
+        assert (
+            backends._platform_quote(r"C:\Users\John Smith\file.py")
+            == r'"C:\Users\John Smith\file.py"'
+        )
+
+    def test_windows_embedded_double_quote_is_escaped(self, monkeypatch):
+        monkeypatch.setattr(backends, "_is_windows", lambda: True)
+        # Embedded " is escaped as \" so cmd.exe keeps the literal quote inside
+        # the token rather than terminating the quoted region.
+        assert backends._platform_quote(r'C:\path\a"b') == r'"C:\path\a\"b"'
+
+    def test_windows_percent_sign_treated_as_regular_char(self, monkeypatch):
+        monkeypatch.setattr(backends, "_is_windows", lambda: True)
+        # %VAR% expansion is not neutralised — %% escaping only works in
+        # .bat/.cmd files, not via cmd /c.  We treat % as a regular char.
+        assert (
+            backends._platform_quote(r"C:\path\%TEMP%\file.py")
+            == r"C:\path\%TEMP%\file.py"
+        )