diff --git a/CHANGELOG.md b/CHANGELOG.md index b75c9340..cdd58219 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,19 @@ PyPI version — not the changelog header. ### Fixed +- **`aipass install` no longer silently repoints your global `drone`/`aipass` + symlinks (issue #660).** `setup.sh` force-overwrote the global CLI symlinks with + `ln -sf` on every run, no check and no opt-out — so `aipass install + --path /tmp/scratch` "to try it" silently hijacked your real global commands to + the scratch tree, which broke them once `/tmp` cleared, disconnected from the + cause. A new `safe_symlink` guard refuses to repoint a symlink that points at a + *different* install: it prints a loud from→to warning and leaves the existing + link untouched unless you pass `--force-symlink`; `--no-symlink` opts out of + symlinking entirely. Both flags thread through `aipass install`. Fresh installs + and same-location reinstalls behave exactly as before. Adds a `safe_symlink` + regression test (`tests/setup_symlink_guard_test.sh`) and 3 flag-forwarding + tests; the touched install output was migrated to `@cli` helpers (#661). + - **`drone @flow close` no longer reports a false "timed out after 30s" on a successful close (issue #662).** A single-plan close committed early (plan marked closed, file archived) and then ran memory vectorization diff --git a/setup.sh b/setup.sh index a91445c4..e6a05c46 100755 --- a/setup.sh +++ b/setup.sh @@ -5,10 +5,12 @@ # On interactive terminals it then chains into `aipass init run` to scaffold a first # project (DPLAN-0234: one command does setup + init). # -# Usage: ./setup.sh [--no-init] [--with-init] [--project ] +# Usage: ./setup.sh [--no-init] [--with-init] [--project ] [--no-symlink] [--force-symlink] # --no-init skip the first-project init chain # --with-init force the init chain even headless (init runs --non-interactive) # --project first-project directory (default: ~/aipass-project) +# --no-symlink do not create/modify global drone/aipass CLI symlinks +# --force-symlink repoint a global symlink even if it points at a different install (#660) # set -euo pipefail @@ -39,6 +41,8 @@ esac # default (auto) chains into init on interactive terminals only — CI/headless skip. RUN_INIT="auto" INIT_PROJECT="" +SKIP_SYMLINK="no" +FORCE_SYMLINK="no" PREV_ARG="" for arg in "$@"; do if [ "$PREV_ARG" = "--project" ]; then @@ -47,10 +51,12 @@ for arg in "$@"; do continue fi case "$arg" in - --no-init) RUN_INIT="no" ;; - --with-init) RUN_INIT="yes" ;; - --project=*) INIT_PROJECT="${arg#--project=}" ;; - --project) PREV_ARG="--project" ;; + --no-init) RUN_INIT="no" ;; + --with-init) RUN_INIT="yes" ;; + --no-symlink) SKIP_SYMLINK="yes" ;; + --force-symlink) FORCE_SYMLINK="yes" ;; + --project=*) INIT_PROJECT="${arg#--project=}" ;; + --project) PREV_ARG="--project" ;; *) echo "WARN: unknown argument '$arg' (ignored)" ;; esac done @@ -964,8 +970,42 @@ else fi # --- Create global symlinks for CLI tools (Linux/macOS only) --- +# #660: never SILENTLY hijack a global 'drone'/'aipass' that points at a +# DIFFERENT install. safe_symlink skips a different-target link (loud warning) +# unless --force-symlink; --no-symlink opts out of symlinking entirely. +SYMLINK_SKIPPED=0 +safe_symlink() { + # safe_symlink [sudo] -> 0 linked · 1 skipped(diff target) · 2 ln failed + local src="$1" dest="$2" use_sudo="${3:-}" existing="" + if [ -L "$dest" ]; then + existing="$(readlink "$dest" 2>/dev/null)" + elif [ -e "$dest" ]; then + existing="$dest (real file, not a symlink)" + fi + if [ -n "$existing" ] && [ "$existing" != "$src" ]; then + if [ "$FORCE_SYMLINK" != "yes" ]; then + echo " SKIP $dest — already points at a different install:" + echo " $existing" + echo " Not repointing (would hijack your existing '$(basename "$dest")'); PATH keeps the above." + echo " Re-run 'aipass install' with --force-symlink to repoint here, or --no-symlink to skip quietly." + SYMLINK_SKIPPED=$((SYMLINK_SKIPPED + 1)) + return 1 + fi + echo " WARNING: repointing $dest" + echo " from $existing" + echo " to $src (--force-symlink)" + fi + if [ -n "$use_sudo" ]; then + $use_sudo ln -sf "$src" "$dest" 2>/dev/null && return 0 || return 2 + fi + ln -sf "$src" "$dest" 2>/dev/null && return 0 || return 2 +} + echo "" -if [ "$IS_WINDOWS" -eq 1 ]; then +if [ "$SKIP_SYMLINK" = "yes" ]; then + echo "Skipping global CLI symlinks (--no-symlink)." + echo " 'drone'/'aipass' resolve from $SCRIPT_DIR/.venv/bin — add it to PATH to use them." +elif [ "$IS_WINDOWS" -eq 1 ]; then echo "Windows: drone available via PATH (set above)" elif [ "$IS_MACOS" -eq 1 ]; then # Mac: symlink into ~/.local/bin (user-writable, no sudo needed). @@ -977,9 +1017,11 @@ elif [ "$IS_MACOS" -eq 1 ]; then for cmd in drone aipass; do if [ -f "$VENV_BIN/$cmd" ]; then - if ln -sf "$VENV_BIN/$cmd" "$LOCAL_BIN/$cmd"; then + safe_symlink "$VENV_BIN/$cmd" "$LOCAL_BIN/$cmd" + rc=$? + if [ "$rc" -eq 0 ]; then echo " $LOCAL_BIN/$cmd -> $VENV_BIN/$cmd" - else + elif [ "$rc" -eq 2 ]; then echo " WARN: Could not create symlink for $cmd" echo " Manual fix: ln -sf $VENV_BIN/$cmd $LOCAL_BIN/$cmd" fi @@ -992,14 +1034,20 @@ else for cmd in drone aipass; do if [ -f "$VENV_BIN/$cmd" ]; then - if sudo ln -sf "$VENV_BIN/$cmd" "/usr/local/bin/$cmd" 2>/dev/null; then + safe_symlink "$VENV_BIN/$cmd" "/usr/local/bin/$cmd" "sudo" + rc=$? + if [ "$rc" -eq 0 ]; then echo " /usr/local/bin/$cmd -> $VENV_BIN/$cmd" LINUX_SYMLINK_DIR="/usr/local/bin" + elif [ "$rc" -eq 1 ]; then + : # skipped a different install — safe_symlink explained; do NOT fall back else - # Fallback: user-local bin (no sudo needed) + # sudo/ln failed (e.g. no sudo) — fall back to user-local bin LOCAL_BIN="$HOME/.local/bin" mkdir -p "$LOCAL_BIN" - if ln -sf "$VENV_BIN/$cmd" "$LOCAL_BIN/$cmd"; then + safe_symlink "$VENV_BIN/$cmd" "$LOCAL_BIN/$cmd" + rc=$? + if [ "$rc" -eq 0 ]; then echo " /usr/local/bin failed (no sudo) — using $LOCAL_BIN/$cmd instead" LINUX_SYMLINK_DIR="$LOCAL_BIN" # Ensure ~/.local/bin is on PATH @@ -1009,7 +1057,7 @@ else echo " ~/.local/bin added to PATH in $PROFILE" fi export PATH="$HOME/.local/bin:$PATH" - else + elif [ "$rc" -eq 2 ]; then echo " WARN: Could not create symlink for $cmd" echo " Manual fix: ln -sf $VENV_BIN/$cmd $LOCAL_BIN/$cmd" fi @@ -1018,6 +1066,12 @@ else done fi +if [ "$SYMLINK_SKIPPED" -gt 0 ]; then + echo "" + echo " NOTE: $SYMLINK_SKIPPED global symlink(s) left untouched (pointed at a different install)." + echo " Your existing 'drone'/'aipass' still work. Use --force-symlink to repoint them here." +fi + # --- Result --- echo "" if [ "$FAIL" -eq 0 ]; then diff --git a/src/aipass/aipass/apps/modules/install.py b/src/aipass/aipass/apps/modules/install.py index 6e225738..0d8a471e 100644 --- a/src/aipass/aipass/apps/modules/install.py +++ b/src/aipass/aipass/apps/modules/install.py @@ -43,7 +43,7 @@ import sys from pathlib import Path from typing import Dict -from aipass.cli.apps.modules import console, warning +from aipass.cli.apps.modules import console, success, warning from aipass.prax import logger from aipass.aipass.apps.handlers.json import json_handler @@ -120,20 +120,26 @@ def _clone_repo(home: Path, dry_run: bool) -> bool: return False -def _run_setup(home: Path, dry_run: bool) -> bool: +def _run_setup(home: Path, dry_run: bool, no_symlink: bool = False, force_symlink: bool = False) -> bool: """Run the repo's setup.sh (venv + editable install + hook wiring + binaries).""" setup = home / "setup.sh" + # --no-init: install owns the init handoff (_handoff_to_init) — without it, + # setup.sh's own init chain (DPLAN-0234) would scaffold the project twice. + # --no-symlink / --force-symlink (#660) pass through to setup.sh's CLI-symlink guard. + setup_args = ["bash", str(setup), "--no-init"] + if no_symlink: + setup_args.append("--no-symlink") + if force_symlink: + setup_args.append("--force-symlink") if dry_run: - console.print(f"[yellow]\\[dry-run][/yellow] would run: bash {setup}") + console.print(f"[yellow]\\[dry-run][/yellow] would run: {' '.join(setup_args)}") return True if not setup.is_file(): warning(f"setup.sh not found at {setup} — cannot build the environment.") return False console.print("[cyan]Building environment[/cyan] [dim](venv, dependencies, hook wiring)…[/dim]") try: - # --no-init: install owns the init handoff (_handoff_to_init) — without it, - # setup.sh's own init chain (DPLAN-0234) would scaffold the project twice. - proc = subprocess.run(["bash", str(setup), "--no-init"], cwd=str(home), timeout=_SETUP_TIMEOUT) + proc = subprocess.run(setup_args, cwd=str(home), timeout=_SETUP_TIMEOUT) if proc.returncode == 0: return True logger.warning("[install] setup.sh exited %s", proc.returncode) @@ -162,11 +168,11 @@ def _verify_binaries(home: Path) -> Dict[str, str | None]: ) aipass = _resolve_aipass_bin(home) if drone: - console.print(f"[green]✓[/green] drone: {drone}") + success(f"drone: {drone}") else: warning("drone not found after setup — check the setup output above.") if aipass: - console.print(f"[green]✓[/green] aipass: {aipass}") + success(f"aipass: {aipass}") else: warning("aipass not found after setup — check the setup output above.") return {"drone": drone, "aipass": aipass} @@ -201,7 +207,7 @@ def _handoff_to_init( headless, init is launched headless too so the whole chain stays non-blocking. """ console.print() - console.print(f"[bold green]✓ AIPass is installed at {home}[/bold green]") + success(f"AIPass is installed at {home}") console.print() console.print(" [cyan]drone systems[/cyan] [dim]# list every agent[/dim]") console.print(" [cyan]aipass doctor[/cyan] [dim]# check system health[/dim]") @@ -258,6 +264,8 @@ def run_install( with_init: bool = False, no_init: bool = False, project: str | None = None, + no_symlink: bool = False, + force_symlink: bool = False, ) -> int: """Run the 4-step one-command install. Returns 0 on success, 1 on failure.""" console.print() @@ -278,20 +286,20 @@ def run_install( console.print(f" Home: [cyan]{home}[/cyan]") if _looks_like_aipass_tree(home): - console.print(f"[green]✓[/green] AIPass already present at {home} — skipping download") + success(f"AIPass already present at {home} — skipping download") elif not _clone_repo(home, dry_run): warning("Could not fetch AIPass — aborting install.") return 1 else: - console.print(f"[green]✓[/green] AIPass downloaded to {home}") + success(f"AIPass downloaded to {home}") # Step 2 — build the environment via setup.sh console.print() console.print(render_step_header(2, TOTAL_STEPS, "Building environment")) - if not _run_setup(home, dry_run): + if not _run_setup(home, dry_run, no_symlink=no_symlink, force_symlink=force_symlink): warning("Environment build failed — aborting install.") return 1 - console.print("[green]✓[/green] Environment ready") + success("Environment ready") # Step 3 — verify the binaries landed console.print() @@ -323,6 +331,8 @@ def print_help() -> None: console.print(" [green]aipass install --here[/green] [dim]# install into current dir[/dim]") console.print(" [green]aipass install --no-init[/green] [dim]# install only, skip init[/dim]") console.print(" [green]aipass install --with-init[/green] [dim]# force init even when headless[/dim]") + console.print(" [green]aipass install --no-symlink[/green] [dim]# skip global CLI symlinks[/dim]") + console.print(" [green]aipass install --force-symlink[/green] [dim]# repoint from another install[/dim]") console.print(" [green]aipass install --project DIR[/green] [dim]# where the first project scaffolds[/dim]") console.print(" [green]aipass install --dry-run[/green] [dim]# walk steps, no side effects[/dim]") console.print() @@ -368,6 +378,8 @@ def handle_command(command: str, args: list[str]) -> bool: here = "--here" in run_args with_init = "--with-init" in run_args no_init = "--no-init" in run_args + no_symlink = "--no-symlink" in run_args + force_symlink = "--force-symlink" in run_args path = _flag_value("--path") project = _flag_value("--project") @@ -379,6 +391,8 @@ def handle_command(command: str, args: list[str]) -> bool: with_init=with_init, no_init=no_init, project=project, + no_symlink=no_symlink, + force_symlink=force_symlink, ) json_handler.log_operation( "install_run", diff --git a/src/aipass/aipass/tests/test_install.py b/src/aipass/aipass/tests/test_install.py index 77da87e9..bb1772b9 100644 --- a/src/aipass/aipass/tests/test_install.py +++ b/src/aipass/aipass/tests/test_install.py @@ -146,6 +146,32 @@ class TestRunSetup: assert _run_setup(tmp_path, dry_run=False) is True run.assert_called_once() + def test_no_symlink_flag_forwarded(self, tmp_path: Path) -> None: + """--no-symlink passes through to setup.sh (#660).""" + (tmp_path / "setup.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + with patch(f"{_MOD}.subprocess.run", return_value=MagicMock(returncode=0)) as run: + assert _run_setup(tmp_path, dry_run=False, no_symlink=True) is True + argv = run.call_args[0][0] + assert "--no-symlink" in argv + assert "--force-symlink" not in argv + + def test_force_symlink_flag_forwarded(self, tmp_path: Path) -> None: + """--force-symlink passes through to setup.sh (#660).""" + (tmp_path / "setup.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + with patch(f"{_MOD}.subprocess.run", return_value=MagicMock(returncode=0)) as run: + assert _run_setup(tmp_path, dry_run=False, force_symlink=True) is True + argv = run.call_args[0][0] + assert "--force-symlink" in argv + + def test_symlink_flags_absent_by_default(self, tmp_path: Path) -> None: + """No symlink flags forwarded unless requested (#660).""" + (tmp_path / "setup.sh").write_text("#!/usr/bin/env bash\n", encoding="utf-8") + with patch(f"{_MOD}.subprocess.run", return_value=MagicMock(returncode=0)) as run: + assert _run_setup(tmp_path, dry_run=False) is True + argv = run.call_args[0][0] + assert "--no-symlink" not in argv + assert "--force-symlink" not in argv + class TestRunInstall: """The four-step orchestrator.""" diff --git a/tests/setup_symlink_guard_test.sh b/tests/setup_symlink_guard_test.sh new file mode 100755 index 00000000..cd8f1343 --- /dev/null +++ b/tests/setup_symlink_guard_test.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +# +# Regression test for setup.sh safe_symlink guard (GitHub #660). +# +# #660: `aipass install` (via setup.sh) must NEVER silently repoint a global +# `drone`/`aipass` symlink that points at a DIFFERENT install. This test sources +# the real safe_symlink function out of setup.sh and asserts its behaviour across +# the meaningful cases. Exits 0 on all-pass, non-zero on any regression. +# +# Run: bash tests/setup_symlink_guard_test.sh +set -u + +REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +SETUP="$REPO_ROOT/setup.sh" +TMP="$(mktemp -d)" +FAILURES=0 + +cleanup() { rm -f "$TMP"/binA/* "$TMP"/binB/* "$TMP"/dest/* 2>/dev/null; rmdir "$TMP"/binA "$TMP"/binB "$TMP"/dest "$TMP" 2>/dev/null; } +trap cleanup EXIT + +# Pull the real safe_symlink out of setup.sh (single source of truth — no copy). +FN="$TMP/fn.sh" +sed -n '/^safe_symlink() {/,/^}/p' "$SETUP" > "$FN" +if ! grep -q "safe_symlink()" "$FN"; then + echo "FAIL: could not extract safe_symlink from $SETUP" + exit 1 +fi +# shellcheck disable=SC1090 +source "$FN" + +mkdir -p "$TMP/binA" "$TMP/binB" "$TMP/dest" +echo A > "$TMP/binA/aipass" +echo B > "$TMP/binB/aipass" +echo A > "$TMP/binA/drone" + +assert() { # assert