fix(install): confirm pipx ownership and quote the removal path

Review feedback on the conflict guidance, both valid.

The pipx hint keyed off `.local/bin` appearing in the path, which is
where a uv-managed or hand-placed strix lands too. Telling that user to
`pipx uninstall strix-agent` either does nothing or removes a different
package, and leaves the real PATH conflict in place. Ask pipx instead:
compare the executable's directory against `PIPX_BIN_DIR` and confirm
`strix-agent` is in `pipx list`.

The `rm` suggestion printed the path bare, so copying it would split on
spaces or expand a glob. Print it through `printf '%q'`.

The decoy tests asserted that pipx was never invoked at all, which the
ownership check now legitimately does. They assert no `uninstall`
instead, which is the contract that matters. `describe_removal` only
runs when another executable wins PATH resolution, which a successful
install prevents, so the advice itself is now tested by lifting the
functions out of the script and calling them directly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

scripts/install.sh

@@ -99,13 +99,30 @@ print_message() {
     echo -e "${color}${message}${NC}"
 }
 
+pipx_owns() {
+    # Ask pipx instead of guessing from the path. A uv-managed or hand-placed
+    # strix can sit in ~/.local/bin too, and telling the user to
+    # "pipx uninstall strix-agent" would then either do nothing or uninstall a
+    # different package than the one winning PATH resolution.
+    local path=$1
+    local bin_dir
+
+    command -v pipx >/dev/null 2>&1 || return 1
+    bin_dir=$(pipx environment --value PIPX_BIN_DIR 2>/dev/null) || return 1
+    [[ -n "$bin_dir" ]] || return 1
+    [[ "$(dirname "$path")" == "${bin_dir%/}" ]] || return 1
+    pipx list --short 2>/dev/null | grep -q "^strix-agent "
+}
+
 describe_removal() {
     local path=$1
 
-    if [[ "$path" == *".local/bin"* ]] && command -v pipx >/dev/null 2>&1; then
-        echo -e "${MUTED}  It looks like a pipx installation. To remove it: ${NC}pipx uninstall strix-agent"
+    if pipx_owns "$path"; then
+        echo -e "${MUTED}  pipx installed it. To remove it: ${NC}pipx uninstall strix-agent"
     else
-        echo -e "${MUTED}  To remove it: ${NC}rm $path"
+        # Quote the path: a copied "rm" of a path with spaces or globbing
+        # characters would otherwise split or expand.
+        echo -e "${MUTED}  To remove it: ${NC}rm $(printf '%q' "$path")"
     fi
 }
 

tests/test_install_script.py

@@ -5,10 +5,13 @@ import subprocess
 import sys
 import tarfile
 from pathlib import Path
+from typing import NamedTuple
 
 import pytest
 
 
+DECOY_CONTENT = "#!/bin/sh\nprintf 'other-strix 1.2.3\\n'\n"
+
 RELEASE_VERSION = "9.9.9"
 RELEASE_TARGET = "linux-arm64"
 
@@ -154,22 +157,49 @@ def test_installer_rejects_unsupported_architecture(tmp_path: Path) -> None:
     assert not (home_path / ".strix").exists()
 
 
-def _install_with_decoy(tmp_path: Path, decoy_directory_name: str) -> tuple[Path, Path, Path]:
-    """Run the installer with an unrelated `strix` ahead of it on `PATH`."""
+class _DecoyRun(NamedTuple):
+    decoy_path: Path
+    pipx_log_path: Path
+    home_path: Path
+    stdout: str
+
+
+def _install_with_decoy(
+    tmp_path: Path,
+    decoy_directory_name: str,
+    *,
+    pipx_owns_it: bool = False,
+) -> _DecoyRun:
+    """Run the installer with an unrelated `strix` ahead of it on `PATH`.
+
+    The mock `pipx` answers the two ownership questions the installer asks and
+    records every invocation, so a test can tell an ownership *query* apart
+    from an `uninstall`.
+    """
     repository_root = Path(__file__).resolve().parents[1]
     archive_path = _create_release_archive(tmp_path)
     mock_bin = _create_mock_commands(tmp_path, machine="aarch64")
 
-    pipx_log_path = tmp_path / "pipx.log"
-    _write_executable(
-        mock_bin / "pipx",
-        f'#!/bin/sh\nprintf \'%s\n\' "$*" >> "{pipx_log_path}"\n',
-    )
-
     decoy_directory = tmp_path / decoy_directory_name
     decoy_directory.mkdir(parents=True)
     decoy_path = decoy_directory / "strix"
-    _write_executable(decoy_path, "#!/bin/sh\nprintf 'other-strix 1.2.3\n'\n")
+    _write_executable(decoy_path, DECOY_CONTENT)
+
+    # When pipx "owns" the decoy it reports the decoy's own directory as its
+    # bin dir and lists the package; otherwise it points somewhere else.
+    pipx_bin_dir = decoy_directory if pipx_owns_it else tmp_path / "elsewhere"
+    pipx_listing = "strix-agent 1.2.3" if pipx_owns_it else ""
+    pipx_log_path = tmp_path / "pipx.log"
+    _write_executable(
+        mock_bin / "pipx",
+        f"""#!/bin/sh
+printf '%s\n' "$*" >> "{pipx_log_path}"
+case "$1 $2" in
+  "environment --value") printf '%s\n' "{pipx_bin_dir}" ;;
+  "list --short") printf '%s' "{pipx_listing}" ;;
+esac
+""",
+    )
 
     environment, home_path, _ = _create_installer_environment(tmp_path, archive_path, mock_bin)
     environment["PATH"] = f"{decoy_directory}:{environment['PATH']}"
@@ -177,21 +207,138 @@ def _install_with_decoy(tmp_path: Path, decoy_directory_name: str) -> tuple[Path
     result = _run_installer(repository_root, environment)
     assert result.returncode == 0, result.stderr
 
-    return decoy_path, pipx_log_path, home_path
+    return _DecoyRun(decoy_path, pipx_log_path, home_path, result.stdout)
+
+
+def _pipx_invocations(pipx_log_path: Path) -> list[str]:
+    if not pipx_log_path.exists():
+        return []
+    return pipx_log_path.read_text(encoding="utf-8").splitlines()
 
 
 def test_installer_leaves_unrelated_strix_executables_alone(tmp_path: Path) -> None:
-    decoy_path, pipx_log_path, home_path = _install_with_decoy(tmp_path, "other-bin")
+    run = _install_with_decoy(tmp_path, "other-bin")
 
-    assert decoy_path.exists()
-    assert decoy_path.read_text(encoding="utf-8") == "#!/bin/sh\nprintf 'other-strix 1.2.3\n'\n"
-    assert not pipx_log_path.exists()
-    assert (home_path / ".strix/bin/strix").exists()
+    assert run.decoy_path.exists()
+    assert run.decoy_path.read_text(encoding="utf-8") == DECOY_CONTENT
+    assert (run.home_path / ".strix/bin/strix").exists()
 
 
 def test_installer_does_not_uninstall_a_pipx_managed_strix(tmp_path: Path) -> None:
-    decoy_path, pipx_log_path, home_path = _install_with_decoy(tmp_path, ".local/bin")
+    run = _install_with_decoy(tmp_path, ".local/bin", pipx_owns_it=True)
 
-    assert decoy_path.exists()
-    assert not pipx_log_path.exists()
-    assert (home_path / ".strix/bin/strix").exists()
+    assert run.decoy_path.exists()
+    assert (run.home_path / ".strix/bin/strix").exists()
+    # Querying pipx about ownership is fine; changing anything is not.
+    assert not any(line.startswith("uninstall") for line in _pipx_invocations(run.pipx_log_path))
+
+
+def _extract_shell_functions(repository_root: Path, names: tuple[str, ...]) -> str:
+    """Pull named function definitions out of the installer.
+
+    `describe_removal` only runs when another executable wins PATH resolution,
+    which a successful install prevents, so driving it through a full install
+    would assert nothing. Lift the functions out and call them directly.
+    """
+    source = (repository_root / "scripts/install.sh").read_text(encoding="utf-8")
+    blocks = []
+    for name in names:
+        opening = f"{name}() {{\n"
+        begin = source.index(opening)
+        end = source.index("\n}\n", begin) + len("\n}\n")
+        blocks.append(source[begin:end])
+    return "".join(blocks)
+
+
+def _describe_removal(
+    tmp_path: Path,
+    path: str,
+    *,
+    pipx_bin_dir: str | None = None,
+    pipx_listing: str = "",
+) -> str:
+    """Run the installer's `describe_removal` against a mock `pipx`."""
+    repository_root = Path(__file__).resolve().parents[1]
+    mock_bin = tmp_path / "fn-bin"
+    mock_bin.mkdir()
+    if pipx_bin_dir is not None:
+        _write_executable(
+            mock_bin / "pipx",
+            f"""#!/bin/sh
+case "$1 $2" in
+  "environment --value") printf '%s\\n' "{pipx_bin_dir}" ;;
+  "list --short") printf '%s' "{pipx_listing}" ;;
+esac
+""",
+        )
+
+    script = tmp_path / "describe.sh"
+    script.write_text(
+        "set -euo pipefail\nMUTED=''\nNC=''\n"
+        + _extract_shell_functions(repository_root, ("pipx_owns", "describe_removal"))
+        + f'\ndescribe_removal "{path}"\n',
+        encoding="utf-8",
+    )
+
+    result = subprocess.run(  # noqa: S603
+        ["/bin/bash", str(script)],
+        env={"PATH": f"{mock_bin}:/usr/bin:/bin"},
+        capture_output=True,
+        text=True,
+        check=True,
+    )
+    return result.stdout
+
+
+def test_removal_advice_names_pipx_only_when_pipx_owns_the_executable(tmp_path: Path) -> None:
+    pipx_bin = tmp_path / "pipx-bin"
+    owned = _describe_removal(
+        tmp_path,
+        str(pipx_bin / "strix"),
+        pipx_bin_dir=str(pipx_bin),
+        pipx_listing="strix-agent 1.2.3",
+    )
+    assert "pipx uninstall strix-agent" in owned
+
+
+def test_removal_advice_does_not_claim_pipx_owns_an_unrelated_local_bin_strix(
+    tmp_path: Path,
+) -> None:
+    """A uv-managed or hand-placed strix in ~/.local/bin is not a pipx install.
+
+    The old heuristic keyed off `.local/bin` appearing in the path, so it would
+    tell the user to `pipx uninstall strix-agent` for a file pipx never
+    installed. That either does nothing or removes a different package, and
+    leaves the real PATH conflict in place.
+    """
+    local_bin = tmp_path / ".local" / "bin"
+    advice = _describe_removal(
+        tmp_path,
+        str(local_bin / "strix"),
+        pipx_bin_dir=str(tmp_path / "elsewhere"),
+    )
+    assert "pipx uninstall" not in advice
+    assert "rm " in advice
+
+
+def test_removal_advice_reports_rm_when_pipx_is_absent(tmp_path: Path) -> None:
+    advice = _describe_removal(tmp_path, str(tmp_path / ".local" / "bin" / "strix"))
+    assert "pipx" not in advice
+    assert "rm " in advice
+
+
+def test_removal_advice_quotes_paths_with_spaces_and_globs(tmp_path: Path) -> None:
+    """A copied `rm` must survive spaces and globbing characters in the path."""
+    awkward = tmp_path / "od d bin" / "strix*"
+    advice = _describe_removal(tmp_path, str(awkward))
+
+    assert f"rm {awkward}" not in advice, "the bare path was printed unquoted"
+    command = advice.split("rm ", 1)[1].strip()
+    # Re-expanding the printed argument must yield exactly the original path.
+    expanded = subprocess.run(  # noqa: S603
+        ["/bin/bash", "-c", f"printf '%s' {command}"],
+        capture_output=True,
+        text=True,
+        check=True,
+    )
+    assert expanded.stdout == str(awkward)