Merge f21c5cf5ad into ae38fe70cd

scripts/install.sh

@@ -99,33 +99,52 @@ 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 pipx_owns "$path"; then
+        echo -e "${MUTED}  pipx installed it. To remove it: ${NC}pipx uninstall strix-agent"
+    else
+        # 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
+}
+
 check_existing_installation() {
+    UNMANAGED_INSTALLATIONS=()
+
     local found_paths=()
     while IFS= read -r -d '' path; do
         found_paths+=("$path")
     done < <(which -a strix 2>/dev/null | tr '\n' '\0' || true)
 
-    if [ ${#found_paths[@]} -gt 0 ]; then
-        for path in "${found_paths[@]}"; do
-            if [[ ! -e "$path" ]] || [[ "$path" == "$INSTALL_DIR/strix"* ]]; then
-                continue
-            fi
+    for path in "${found_paths[@]:-}"; do
+        if [[ -z "$path" ]] || [[ ! -e "$path" ]] || [[ "$path" == "$INSTALL_DIR/strix"* ]]; then
+            continue
+        fi
 
-            if [[ -n "$path" ]]; then
-                echo -e "${MUTED}Found existing strix at: ${NC}$path"
+        UNMANAGED_INSTALLATIONS+=("$path")
+        echo -e "${MUTED}Found another strix at: ${NC}$path"
+    done
 
-                if [[ "$path" == *".local/bin"* ]]; then
-                    echo -e "${MUTED}Removing old pipx installation...${NC}"
-                    if command -v pipx >/dev/null 2>&1; then
-                        pipx uninstall strix-agent 2>/dev/null || true
-                    fi
-                    rm -f "$path" 2>/dev/null || true
-                elif [[ -L "$path" || -f "$path" ]]; then
-                    echo -e "${MUTED}Removing old installation...${NC}"
-                    rm -f "$path" 2>/dev/null || true
-                fi
-            fi
-        done
+    if [ ${#UNMANAGED_INSTALLATIONS[@]} -gt 0 ]; then
+        echo -e "${MUTED}This installer only manages ${NC}$INSTALL_DIR${MUTED}, so it is left alone.${NC}"
     fi
 }
 
@@ -296,15 +315,10 @@ verify_installation() {
 
     if [[ "$which_strix" != "$INSTALL_DIR/strix" && "$which_strix" != "$INSTALL_DIR/strix.exe" ]]; then
         if [[ -n "$which_strix" ]]; then
-            echo -e "${YELLOW}⚠ Found conflicting strix at: ${NC}$which_strix"
-            echo -e "${MUTED}Attempting to remove...${NC}"
-
-            if rm -f "$which_strix" 2>/dev/null; then
-                echo -e "${GREEN}✓ Removed conflicting installation${NC}"
-            else
-                echo -e "${YELLOW}Could not remove automatically.${NC}"
-                echo -e "${MUTED}Please remove manually: ${NC}rm $which_strix"
-            fi
+            echo -e "${YELLOW}⚠ Another strix wins PATH resolution: ${NC}$which_strix"
+            echo -e "${MUTED}The version just installed lives in ${NC}$INSTALL_DIR${MUTED} and will not run until that changes.${NC}"
+            echo -e "${MUTED}Put ${NC}$INSTALL_DIR${MUTED} earlier on your PATH, or remove the other executable yourself.${NC}"
+            describe_removal "$which_strix"
         fi
     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"
 
@@ -152,3 +155,190 @@ def test_installer_rejects_unsupported_architecture(tmp_path: Path) -> None:
     assert "Unsupported OS/Arch: linux/riscv64" in result.stdout
     assert not curl_log_path.exists()
     assert not (home_path / ".strix").exists()
+
+
+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")
+
+    decoy_directory = tmp_path / decoy_directory_name
+    decoy_directory.mkdir(parents=True)
+    decoy_path = decoy_directory / "strix"
+    _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']}"
+
+    result = _run_installer(repository_root, environment)
+    assert result.returncode == 0, result.stderr
+
+    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:
+    run = _install_with_decoy(tmp_path, "other-bin")
+
+    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:
+    run = _install_with_decoy(tmp_path, ".local/bin", pipx_owns_it=True)
+
+    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)