diff --git a/EvoScientist/backends.py b/EvoScientist/backends.py index b17acde..2ef04bd 100644 --- a/EvoScientist/backends.py +++ b/EvoScientist/backends.py @@ -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 - ``//`` 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 "//" (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;|&<>\'"`]*' diff --git a/tests/test_backends.py b/tests/test_backends.py index e49f9ed..9721332 100644 --- a/tests/test_backends.py +++ b/tests/test_backends.py @@ -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//...`` 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/`` 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/`` 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):