From b337a42771f082b5268326fcd5d642c0e01bbfb8 Mon Sep 17 00:00:00 2001 From: itzzdev09 Date: Sat, 12 Sep 2026 09:00:19 +0530 Subject: [PATCH 1/2] fix(install): only manage strix in the installer's own directory `check_existing_installation` walked `which -a strix` and deleted every match outside `$INSTALL_DIR`, and `verify_installation` deleted whatever executable won PATH resolution. Path discovery shows that another `strix` exists; it does not show that the installer owns it. A pipx install or a development checkout on `PATH` was removed without being asked about, including a `pipx uninstall strix-agent` triggered purely by the path containing `.local/bin`. The installer now touches only `$INSTALL_DIR`. Other executables are reported, and when one wins PATH resolution the user is told how to reorder `PATH` or remove it themselves. Fixes #1262 Co-Authored-By: Claude Opus 5 --- scripts/install.sh | 53 +++++++++++++++++------------------- tests/test_install_script.py | 43 +++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index e179b558e..9f4b0a0dc 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -99,33 +99,35 @@ print_message() { echo -e "${color}${message}${NC}" } +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" + else + echo -e "${MUTED} To remove it: ${NC}rm $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 +298,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 diff --git a/tests/test_install_script.py b/tests/test_install_script.py index 67ca48437..e099e3348 100644 --- a/tests/test_install_script.py +++ b/tests/test_install_script.py @@ -152,3 +152,46 @@ 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() + + +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`.""" + 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") + + 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 decoy_path, pipx_log_path, home_path + + +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") + + 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() + + +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") + + assert decoy_path.exists() + assert not pipx_log_path.exists() + assert (home_path / ".strix/bin/strix").exists() From f21c5cf5addcd526c3a8a6dbe4f7b41270227dc1 Mon Sep 17 00:00:00 2001 From: itzzdev09 Date: Sat, 12 Sep 2026 12:38:51 +0530 Subject: [PATCH 2/2] 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 --- scripts/install.sh | 23 ++++- tests/test_install_script.py | 185 +++++++++++++++++++++++++++++++---- 2 files changed, 186 insertions(+), 22 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index 9f4b0a0dc..1ddda78ec 100755 --- a/scripts/install.sh +++ b/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 } diff --git a/tests/test_install_script.py b/tests/test_install_script.py index e099e3348..dbcc51c80 100644 --- a/tests/test_install_script.py +++ b/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)