gitea_saas/review_workspace.py

@@ -25,13 +25,17 @@ clone —
 All git commands go through the same ``Runner`` subprocess seam as the
 rest of the review gate, so hermetic tests can use local-path ``origin``
 remotes (zero network) and/or a scripted command runner to assert the
-exact commands, working directories, and ranges.
+exact commands, working directories, and ranges.  Every invocation here
+passes an **argument list** (``shell=False`` in the real Runner): no
+shell ever interprets the arguments, so a ``^{commit}`` suffix, a path
+containing spaces/``$``, or any other shell-special character survives
+verbatim on every platform (fixes the Windows cmd.exe ``^``-eating
+break — issue #48).
 """
 
 from __future__ import annotations
 
 import re
-import shlex
 import shutil
 import tempfile
 from pathlib import Path
@@ -156,11 +160,11 @@ class ReviewWorkspace:
             "head_sha": head_sha,
             "worktree_dir": str(tmpdir),
         }
-        tmpdir_q = shlex.quote(str(tmpdir))
         try:
             add = self.runner.run(
                 self.clone_dir,
-                f"git worktree add --detach {tmpdir_q} {base_sha}")
+                ["git", "worktree", "add", "--detach",
+                 str(tmpdir), base_sha])
             if add.get("exit") != 0:
                 raise ReviewWorkspaceError(
                     "cannot create the temporary review worktree: "
@@ -168,9 +172,11 @@ class ReviewWorkspace:
 
             merge = self.runner.run(
                 tmpdir,
-                f"git merge --no-ff -m '{_MERGE_MESSAGE}' {head_sha}")
+                ["git", "merge", "--no-ff", "-m",
+                 _MERGE_MESSAGE, head_sha])
             evidence["merge_command"] = (
-                f"git merge --no-ff -m '{_MERGE_MESSAGE}' {head_sha}")
+                ["git", "merge", "--no-ff", "-m",
+                 _MERGE_MESSAGE, head_sha])
             evidence["merge_exit"] = merge.get("exit")
             evidence["merge_summary"] = merge.get("summary", "")[:800]
 
@@ -178,14 +184,17 @@ class ReviewWorkspace:
                 # Distinguish a real merge conflict (MERGE_HEAD exists) from an
                 # environmental merge failure (no conflict state was entered).
                 merget = self.runner.run(
-                    tmpdir, "git rev-parse -q --verify MERGE_HEAD")
+                    tmpdir, ["git", "rev-parse", "-q",
+                             "--verify", "MERGE_HEAD"])
                 if merget.get("exit") != 0:
                     raise ReviewWorkspaceError(
                         "synthetic merge failed without entering a conflict "
                         f"state (environmental failure): {merge.get('summary', '')[:400]}")
                 conflicted = self.runner.run(
-                    tmpdir, "git diff --name-only --diff-filter=U")
-                status = self.runner.run(tmpdir, "git status --porcelain")
+                    tmpdir, ["git", "diff", "--name-only",
+                             "--diff-filter=U"])
+                status = self.runner.run(
+                    tmpdir, ["git", "status", "--porcelain"])
                 evidence["conflicted_files"] = [
                     line for line in (conflicted.get("summary") or "").splitlines()
                     if line.strip()]
@@ -194,7 +203,8 @@ class ReviewWorkspace:
                     tmpdir, base_sha, head_sha, merge_ok=False,
                     evidence=evidence, cleanup=self._make_cleanup(tmpdir))
 
-            result_head = self.runner.run(tmpdir, "git rev-parse HEAD")
+            result_head = self.runner.run(
+                tmpdir, ["git", "rev-parse", "HEAD"])
             evidence["result_head"] = (result_head.get("summary") or "").strip()
             return MergeWorkspace(
                 tmpdir, base_sha, head_sha, merge_ok=True,
@@ -215,16 +225,12 @@ class ReviewWorkspace:
     def _make_cleanup(self, tmpdir: Path) -> Callable[[], None]:
         """A cleanup closure: ``git worktree remove --force`` + rmtree."""
 
-        # Shell-quoted exactly like construction: an unquoted path with
-        # spaces/shell metacharacters would be split by the shell, git
-        # would fail to remove the worktree's admin entry, and the clone
-        # would keep a dangling worktree record (violating criterion 7).
-        tmpdir_q = shlex.quote(str(tmpdir))
-
         def cleanup() -> None:
             try:
                 self.runner.run(
-                    self.clone_dir, f"git worktree remove --force {tmpdir_q}")
+                    self.clone_dir,
+                    ["git", "worktree", "remove", "--force",
+                     str(tmpdir)])
             except Exception:  # noqa: BLE001 - cleanup is best-effort
                 pass
             shutil.rmtree(tmpdir, ignore_errors=True)
@@ -240,16 +246,19 @@ class ReviewWorkspace:
         re-verifies.  Hermetic tests use a local-path ``origin``.
         """
         res = self.runner.run(
-            self.clone_dir, f"git fetch origin {base_sha} {head_sha}")
+            self.clone_dir,
+            ["git", "fetch", "origin", base_sha, head_sha])
         if res.get("exit") != 0:
-            res = self.runner.run(self.clone_dir, "git fetch origin")
+            res = self.runner.run(
+                self.clone_dir, ["git", "fetch", "origin"])
             if res.get("exit") != 0:
                 raise ReviewWorkspaceError(
                     "cannot fetch the named base/head SHAs from the configured "
                     f"clone's origin: {res.get('summary', '')[:400]}")
         for label, sha in (("base", base_sha), ("head", head_sha)):
             chk = self.runner.run(
-                self.clone_dir, f"git rev-parse --verify {sha}^{{commit}}")
+                self.clone_dir,
+                ["git", "rev-parse", "--verify", f"{sha}^{{commit}}"])
             if chk.get("exit") != 0:
                 raise ReviewWorkspaceError(
                     f"cannot resolve the {label} SHA {sha} as a commit in the "

gitea_saas/reviewer.py

@@ -143,6 +143,16 @@ def _redact_quote(text: str, match: "re.Match[str]") -> str:
 class Runner:
     """Subprocess seam. ``command_runner`` is injectable for hermetic tests.
 
+    ``run`` accepts a **command** that is either
+
+    * an argument **list** (``["git", "rev-parse", "--verify", f"{sha}^{{commit}}"]``)
+      — executed with ``shell=False`` so no shell ever interprets the
+      arguments (``^``, ``~``, ``&``, ``|``, ``<``, ``>``, ``%``, spaces,
+      …) — the Windows-portable form used by every git invocation in this
+      package (issue #48); or
+    * a **string**, for callers that genuinely need shell semantics
+      (``python -m pytest``, admin-configured ``build_cmd``).
+
     Returns ``{"exit": int | "timed_out", "summary": tail-of-output}``.
     """
 
@@ -150,10 +160,17 @@ class Runner:
                  command_runner: Optional[Callable] = None):
         self.timeout_s = timeout_s
         self._command_runner = command_runner or (
-            lambda command, **kwargs: subprocess.run(command, shell=True, **kwargs))
+            lambda command, **kwargs:
+                subprocess.run(command, shell=isinstance(command, str),
+                               **kwargs))
 
-    def run(self, repo_dir, command: str, timeout_s: Optional[int] = None) -> dict:
+    def run(self, repo_dir, command, timeout_s: Optional[int] = None) -> dict:
+        """Run ``command`` (arg list, or string) with ``cwd=repo_dir``."""
         timeout = timeout_s or self.timeout_s
+        if isinstance(command, str) and command.lstrip().startswith("["):
+            raise TypeError(
+                "Runner.run command must be an argument LIST or a shell "
+                f"string, not a stringified list: {command!r}")
         try:
             completed = self._command_runner(
                 command, cwd=str(repo_dir), timeout=timeout,
@@ -320,8 +337,12 @@ class Reviewer:
 
             # Explicit base/result range: the whitespace gate checks only the
             # synthetic merge result vs the exact base it was built on.
+            # Argument list (shell=False), same Windows-safety rationale as
+            # review_workspace (issue #48): the SHA and "HEAD" are untrusted
+            # data and must reach git verbatim.
             hygiene = self.runner.run(
-                ws.dir, f"git diff --check {ws.base_sha} HEAD",
+                ws.dir,
+                ["git", "diff", "--check", ws.base_sha, "HEAD"],
                 timeout_s=self.config["test_timeout_s"])
 
             findings = self._collect_findings(diff_text, tests, hygiene)

tests/test_review_workspace.py

@@ -241,23 +241,33 @@ def test_git_commands_run_in_workspace_cwd(fixtures):
 
     def spy(command, **kwargs):
         calls.append((kwargs.get("cwd"), command))
-        return subprocess.run(command, shell=True, **kwargs)
+        if isinstance(command, str):
+            return subprocess.run(command, shell=True, **kwargs)
+        return subprocess.run(command, shell=False, **kwargs)
 
     runner = Runner(timeout_s=120, command_runner=spy)
     ws = ReviewWorkspace(fixtures.clone, runner)
     with ws.build("acme", 8, fixtures.head_clean, fixtures.base_clean) as mws:
         assert mws.merge_ok is True
-        merges = [c for c in calls if "git merge --no-ff" in c[1]]
+        merges = [c for c in calls
+                  if isinstance(c[1], list)
+                  and c[1][:4] == ["git", "merge", "--no-ff", "-m"]]
         assert merges, "synthetic merge command must run"
         assert Path(merges[0][0]) == mws.dir, "merge must run INSIDE the workspace"
         assert "review-ws" in str(merges[0][0])
         # fetch + worktree add run against the configured clone (read-only use)
-        adds = [c for c in calls if "worktree add --detach" in c[1]]
+        adds = [c for c in calls
+                if isinstance(c[1], list)
+                and c[1][:4] == ["git", "worktree", "add", "--detach"]]
         assert adds and Path(adds[0][0]) == fixtures.clone
-        fetches = [c for c in calls if "git fetch origin" in c[1]]
+        fetches = [c for c in calls
+                   if isinstance(c[1], list)
+                   and c[1][:3] == ["git", "fetch", "origin"]]
         assert fetches and Path(fetches[0][0]) == fixtures.clone
     # cleanup command targets the clone
-    removes = [c for c in calls if "worktree remove --force" in c[1]]
+    removes = [c for c in calls
+               if isinstance(c[1], list)
+               and c[1][:4] == ["git", "worktree", "remove", "--force"]]
     assert removes and Path(removes[0][0]) == fixtures.clone
 
 
@@ -266,7 +276,9 @@ def test_explicit_range_diff_check_from_workspace(fixtures):
 
     def spy(command, **kwargs):
         calls.append((kwargs.get("cwd"), command))
-        return subprocess.run(command, shell=True, **kwargs)
+        if isinstance(command, str):
+            return subprocess.run(command, shell=True, **kwargs)
+        return subprocess.run(command, shell=False, **kwargs)
 
     runner = Runner(timeout_s=120, command_runner=spy)
     ws = ReviewWorkspace(fixtures.clone, runner)
@@ -274,15 +286,58 @@ def test_explicit_range_diff_check_from_workspace(fixtures):
         assert mws.merge_ok is True
         # The reviewer's hygiene gate: diff --check over the EXPLICIT
         # base..result range, run from INSIDE the isolated workspace.
-        res = runner.run(mws.dir, f"git diff --check {fixtures.base_clean} HEAD")
+        res = runner.run(
+            mws.dir, ["git", "diff", "--check",
+                      fixtures.base_clean, "HEAD"])
         assert res["exit"] == 0
-    checks = [c for c in calls if "diff --check" in c[1]]
+    checks = [c for c in calls
+              if isinstance(c[1], list)
+              and c[1][:3] == ["git", "diff", "--check"]]
     assert checks, "explicit-range diff --check must run"
-    assert fixtures.base_clean in checks[0][1]   # explicit BASE
-    assert " HEAD" in checks[0][1]               # explicit result end
+    assert checks[0][1][3] == fixtures.base_clean  # explicit BASE
+    assert checks[0][1][4] == "HEAD"               # explicit result end
     assert "review-ws" in str(checks[0][0])      # inside the isolated workspace
 
 
+# ---------------------------------------------------------------------------
+# Windows portability (issue #48): git args reach git as a LIST, never a
+# shell string — cmd.exe must not be able to eat `^` (or any other
+# shell-special char) from `^{commit}` and friends.
+# ---------------------------------------------------------------------------
+
+def test_caret_peel_reaches_git_as_single_list_token():
+    recorded = []
+
+    def fake_runner(command, **kwargs):
+        recorded.append(command)
+        # All invocations are argument lists (never shell strings): every
+        # git command is recorded for the assertions below.
+        assert isinstance(command, list), (
+            f"git command must be an argument list, got {command!r}")
+        head = command[:2]
+        if head == ["git", "rev-parse"] and "--verify" in command:
+            return SimpleNamespace(
+                returncode=0, stdout="1fccd03843f7d3e7ef0bf5428aa7a40114b70263",
+                stderr="")
+        return SimpleNamespace(returncode=0, stdout="", stderr="")
+
+    runner = Runner(timeout_s=60, command_runner=fake_runner)
+    ws = ReviewWorkspace("/fake/clone", runner)
+    sha = "d3df1df0c2e458ac5da94a4edb2d254777f64a02"
+    with ws.build("acme", 48, sha, sha) as mws:
+        assert mws.merge_ok is True
+        # The peel suffix survives as ONE list element — cmd.exe never sees
+        # it, so the Windows `^`-escape class of breakage is impossible.
+        peels = [c for c in recorded
+                 if c[:2] == ["git", "rev-parse"]
+                 and "--verify" in c and c[-1].endswith("^{commit}")]
+        assert peels, "rev-parse ^{commit} verification must run"
+        assert all(len(c) == 4 and c[3] == f"{sha}^{{commit}}"
+                   for c in peels), f"peel must be a single token, got {peels}"
+        # No stringified-list commands anywhere: a shell can never see them.
+        assert all(isinstance(c, list) for c in recorded)
+
+
 # ---------------------------------------------------------------------------
 # Bounce-fix: shell-safe path quoting + filesystem-safe temp prefix
 # ---------------------------------------------------------------------------

tests/test_reviewer.py

@@ -345,10 +345,16 @@ def test_hygiene_diff_check_uses_explicit_base_head_range():
     # workspace directory — never a bare `git diff --check`.
     assert len(runner.commands) == 2  # pytest + explicit-range diff --check
     ws_dir, hygiene_cmd, _ = runner.commands[1]
-    assert ws_dir == "/fake/ws"
-    assert hygiene_cmd == "git diff --check bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb HEAD"
-    assert "git diff --check" not in hygiene_cmd.replace(
-        "git diff --check bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb HEAD", "")
+    # Compare as PATHs, not strings: on Windows str(Path('/fake/ws')) is
+    # '\\fake\\ws' (a valid spelling of the same directory) — the string
+    # comparison would fail on the platform this gate actually runs on.
+    # Fix in the test, not the source: passing a Path as cwd is correct
+    # (bounce-fix review 11154; issue #48).
+    assert Path(ws_dir) == Path("/fake/ws")
+    # The whitespace gate runs as an ARGUMENT LIST (shell=False) — no shell
+    # can mangle the SHA/HEAD tokens on any platform (issue #48).
+    assert hygiene_cmd == ["git", "diff", "--check",
+                           "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "HEAD"]
 
 
 def test_merge_conflict_bounces_without_running_tests():