#660 install: aipass install no longer silently repoints global drone/aipass symlinks. setup.sh safe_symlink guard refuses to hijack a symlink pointing at a DIFFERENT install (loud from->to warning, left untouched) unless --force-symlink; --no-symlink opts out entirely. Both flags thread through install.py -> _run_setup. Fresh/same-location installs unchanged. +tests/setup_symlink_guard_test.sh (10 asserts) +3 flag-forwarding tests; touched install output migrated to cli success() (#661). Verified: 635 aipass tests, seedgo 31/31, live dry-run forwarding + guard test all-pass.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 <dir>]
|
||||
# Usage: ./setup.sh [--no-init] [--with-init] [--project <dir>] [--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 <dir> 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 <src> <dest> [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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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."""
|
||||
|
||||
Executable
+81
@@ -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 <label> <expected> <actual>
|
||||
if [ "$2" = "$3" ]; then
|
||||
echo " PASS: $1"
|
||||
else
|
||||
echo " FAIL: $1 (expected '$2', got '$3')"
|
||||
FAILURES=$((FAILURES + 1))
|
||||
fi
|
||||
}
|
||||
|
||||
FORCE_SYMLINK="no"; SYMLINK_SKIPPED=0
|
||||
|
||||
# Case 1: dest missing -> link, rc 0
|
||||
safe_symlink "$TMP/binA/aipass" "$TMP/dest/aipass" >/dev/null; rc=$?
|
||||
assert "fresh link returns 0" "0" "$rc"
|
||||
assert "fresh link points at src" "$TMP/binA/aipass" "$(readlink "$TMP/dest/aipass")"
|
||||
|
||||
# Case 2: dest already points at same src (re-install same location) -> rc 0, no warn
|
||||
safe_symlink "$TMP/binA/aipass" "$TMP/dest/aipass" >/dev/null; rc=$?
|
||||
assert "same-target relink returns 0" "0" "$rc"
|
||||
|
||||
# Case 3: dest points at a DIFFERENT install, no force -> rc 1, target UNCHANGED
|
||||
safe_symlink "$TMP/binB/aipass" "$TMP/dest/aipass" >/dev/null; rc=$?
|
||||
assert "different-target without force returns 1 (skip)" "1" "$rc"
|
||||
assert "different-target NOT repointed (no silent hijack)" "$TMP/binA/aipass" "$(readlink "$TMP/dest/aipass")"
|
||||
assert "skip counter incremented" "1" "$SYMLINK_SKIPPED"
|
||||
|
||||
# Case 4: same, but --force-symlink -> rc 0, repointed
|
||||
FORCE_SYMLINK="yes"
|
||||
safe_symlink "$TMP/binB/aipass" "$TMP/dest/aipass" >/dev/null; rc=$?
|
||||
assert "different-target WITH force returns 0" "0" "$rc"
|
||||
assert "force repoints to new src" "$TMP/binB/aipass" "$(readlink "$TMP/dest/aipass")"
|
||||
|
||||
# Case 5: dest is a REAL file (not a symlink), no force -> rc 1, file intact
|
||||
FORCE_SYMLINK="no"
|
||||
echo realfile > "$TMP/dest/drone"
|
||||
safe_symlink "$TMP/binA/drone" "$TMP/dest/drone" >/dev/null; rc=$?
|
||||
assert "real-file dest without force returns 1 (skip)" "1" "$rc"
|
||||
assert "real-file dest left intact" "realfile" "$(cat "$TMP/dest/drone" 2>/dev/null)"
|
||||
|
||||
echo ""
|
||||
if [ "$FAILURES" -eq 0 ]; then
|
||||
echo "setup_symlink_guard_test: ALL PASS"
|
||||
exit 0
|
||||
fi
|
||||
echo "setup_symlink_guard_test: $FAILURES FAILURE(S)"
|
||||
exit 1
|
||||
Reference in New Issue
Block a user