From 6d69d3057afb9427c78449906433c65e87605377 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Thu, 6 Aug 2026 13:19:57 +0100 Subject: [PATCH] fix: harden the install / update / uninstall scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things found while re-reading the family of scripts around the logout fix. Both installers piped curl straight into tar, so a truncated download half-extracted — and the installer explicitly supports reinstalling over an existing install, which turned an interrupted download into a tree mixing old and new files with no error saying so. They now download to a temp file and verify the archive in a staging dir before writing anything to the install directory: the ordering update.sh has had since it was written, for the same reason. update.sh never removed files deleted upstream. Extracting over the install dir only adds and overwrites, so a renamed page under docs/ kept being mounted read-only into every container for the assistant to read, and a removed command kept being discovered. It now prunes, from the directories the tarball owns end to end (web, commands, skills, docs), whatever the already-verified staging copy does not have. Pruning after the extraction rather than replacing the directory keeps every intermediate state a complete install. agents/ is deliberately excluded: dropping in an agent is a documented extension point, so that directory is not ours alone and pruning it would delete somebody's work. uninstall.sh fed `docker ps -aq --filter 'name=skald-'` to `docker rm -f`. Docker's name filter is a regex matched anywhere in the name, not a prefix, so any unrelated container merely containing "skald-" was force-removed. Anchored to ^skald-. uninstall.sh also matched uname's raw Linux/Darwin while its three siblings normalize to lowercase. It was correct on its own, but being the odd one out of four copy-paste relatives is precisely how update.sh acquired its no-op case arms, so it now normalizes like the others. Finally, the uninstaller reports that lingering is still enabled and how to turn it off, rather than disabling it: it is a persistent per-user setting other user services may rely on by now, so taking it back silently would stop those too. --- SKALD.md | 28 +++++++++++++++++++++++++++- install-nightly.sh | 24 ++++++++++++++++++++++-- install.sh | 24 ++++++++++++++++++++++-- uninstall.sh | 40 +++++++++++++++++++++++++++++++++++----- update.sh | 25 +++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 10 deletions(-) diff --git a/SKALD.md b/SKALD.md index de7535f..f206861 100644 --- a/SKALD.md +++ b/SKALD.md @@ -112,7 +112,33 @@ systemd service → ExecStart=run.sh **Problem**: `stop_service` and `start_service` matched `case "$OS" in Linux) … Darwin)`, but `$OS` had already been normalized to `linux`/`darwin` at the top of the script. Every branch fell through: both functions were no-ops. So the updater extracted the tarball **over the running binary** (`ETXTBSY` on Linux, aborting the update mid-way) and, when extraction did succeed, left the old build running in memory with the safety-net trap firing a restart that was itself a no-op. The careful stop → wait-for-exit → extract ordering the file documents at the top had not been executing at all. -**Fix**: matched the normalized lowercase values, with a comment at the seam saying why the capitalization is load-bearing. +**Fix**: matched the normalized lowercase values, with a comment at the seam saying why the capitalization is load-bearing. `uninstall.sh` was correct on its own (it matched raw `uname -s`), but it was the odd one out of four sibling scripts — which is how a `case` gets copied into the wrong one — so it now normalizes like the others. + +## Bug fix: the installers piped curl straight into tar ✅ + +**Problem**: `curl -fsSL "$TARBALL_URL" | tar xz -C "$INSTALL_DIR"`. A truncated download half-extracts, and the installer explicitly supports reinstalling over an existing install — so an interrupted download left a tree mixing old and new files, with no error saying so. `update.sh` had guarded against exactly this since it was written; the installers had not. + +**Fix**: download to a temp file, verify it extracts and carries `bin/skald` in a staging dir, and only then write to the install directory. Same ordering, same reasoning as `update.sh`. + +## Improvement: update.sh now drops files deleted upstream ✅ + +**Problem**: extracting over the install directory only ever adds and overwrites. Anything removed upstream survived every future update — a renamed page under `docs/` kept being mounted read-only into every container for the assistant to read, a deleted command kept being discovered. + +**Fix**: after extracting, prune from the directories the tarball owns end to end (`web/`, `commands/`, `skills/`, `docs/`) whatever the already-verified staging copy does not have, then remove the directories left empty. Pruning _after_ the extraction rather than replacing the directory keeps every intermediate state a complete install, and the only files removed are ones the new build has verifiably dropped. + +`agents/` is deliberately excluded: adding an agent is a documented extension point (`agents//meta.json` + `AGENT.md`), so the directory is not ours alone and pruning it would delete somebody's work — at the price of an upstream-deleted agent lingering. `bin/` is excluded too: two files, both overwritten every time. + +## Bug fix: uninstall.sh could remove containers that are not ours ✅ + +**Problem**: `docker ps -aq --filter 'name=skald-'` feeding `docker rm -f`. Docker's name filter is a regex matched _anywhere_ in the name, not a prefix, so any unrelated container whose name merely contains `skald-` was force-removed. + +**Fix**: anchored to `name=^skald-`. Ours are always `skald-{userid}`. + +**Also**: the uninstaller now reports that systemd lingering is still enabled and how to turn it off, rather than disabling it. It is a persistent per-user setting that other `systemctl --user` services may be relying on by now, so taking it back silently would stop those too — the note leaves the choice to the human. + +## Not done: update.sh does not refresh the systemd unit + +The unit is generated in one place (the installers) and `update.sh` deliberately does not rewrite it — clobbering a hand-edited unit as a side effect of an update is the kind of surprise worth avoiding, and duplicating the template into a second script is how the two drift. Consequence: unit changes (such as `Restart=always`) reach an existing box only by re-running the installer, which is idempotent — `skald-setup` is a no-op once an admin exists. ## Bug fix: skald-setup non interattivo con curl | bash ✅ diff --git a/install-nightly.sh b/install-nightly.sh index 52733e2..cd7249b 100755 --- a/install-nightly.sh +++ b/install-nightly.sh @@ -338,12 +338,32 @@ if [ -x "$INSTALL_DIR/bin/skald" ]; then fi # ── Download & extract ──────────────────────────────────────────────────────── +# Download to a temp file and verify the archive BEFORE touching the install dir +# — the same ordering update.sh uses, and for the same reason. Piping curl +# straight into tar half-extracts a truncated download, which on the +# reinstall-over-an-existing-install path above leaves a tree mixing old and new +# files: worse than either version, and with no error to say so. info "↓ Downloading Skald Circle (${DISPLAY_VERSION}) …" + +TMP_TARBALL="$(mktemp -t skald-install.XXXXXX.tar.gz)" +STAGING="$(mktemp -d -t skald-install-staging.XXXXXX)" +trap 'rm -f "$TMP_TARBALL" 2>/dev/null || true; rm -rf "$STAGING" 2>/dev/null || true' EXIT + +curl -fsSL -o "$TMP_TARBALL" "$TARBALL_URL" + +info "🔎 Verifying archive …" +tar xzf "$TMP_TARBALL" -C "$STAGING" --strip-components=1 +if [ ! -x "$STAGING/bin/skald" ]; then + err "Downloaded archive is invalid — skald binary not found." + err "Nothing was written to ${INSTALL_DIR}." + exit 1 +fi + mkdir -p "$INSTALL_DIR" -curl -fsSL "$TARBALL_URL" | tar xz -C "$INSTALL_DIR" --strip-components=1 +tar xzf "$TMP_TARBALL" -C "$INSTALL_DIR" --strip-components=1 if [ ! -x "$INSTALL_DIR/bin/skald" ]; then - err "Download or extraction failed — skald binary not found." + err "Extraction failed — skald binary not found." exit 1 fi diff --git a/install.sh b/install.sh index c7f0de8..0e0196e 100755 --- a/install.sh +++ b/install.sh @@ -343,12 +343,32 @@ if [ -x "$INSTALL_DIR/bin/skald" ]; then fi # ── Download & extract ──────────────────────────────────────────────────────── +# Download to a temp file and verify the archive BEFORE touching the install dir +# — the same ordering update.sh uses, and for the same reason. Piping curl +# straight into tar half-extracts a truncated download, which on the +# reinstall-over-an-existing-install path above leaves a tree mixing old and new +# files: worse than either version, and with no error to say so. info "↓ Downloading Skald Circle ${VERSION} …" + +TMP_TARBALL="$(mktemp -t skald-install.XXXXXX.tar.gz)" +STAGING="$(mktemp -d -t skald-install-staging.XXXXXX)" +trap 'rm -f "$TMP_TARBALL" 2>/dev/null || true; rm -rf "$STAGING" 2>/dev/null || true' EXIT + +curl -fsSL -o "$TMP_TARBALL" "$TARBALL_URL" + +info "🔎 Verifying archive …" +tar xzf "$TMP_TARBALL" -C "$STAGING" --strip-components=1 +if [ ! -x "$STAGING/bin/skald" ]; then + err "Downloaded archive is invalid — skald binary not found." + err "Nothing was written to ${INSTALL_DIR}." + exit 1 +fi + mkdir -p "$INSTALL_DIR" -curl -fsSL "$TARBALL_URL" | tar xz -C "$INSTALL_DIR" --strip-components=1 +tar xzf "$TMP_TARBALL" -C "$INSTALL_DIR" --strip-components=1 if [ ! -x "$INSTALL_DIR/bin/skald" ]; then - err "Download or extraction failed — skald binary not found." + err "Extraction failed — skald binary not found." exit 1 fi diff --git a/uninstall.sh b/uninstall.sh index 34e134a..ee27132 100644 --- a/uninstall.sh +++ b/uninstall.sh @@ -37,7 +37,16 @@ else fi # ── Detect OS ───────────────────────────────────────────────────────────────── -OS="$(uname -s)" +# Normalized to lowercase like every sibling script (install.sh, install-nightly.sh, +# update.sh). This file used to match uname's raw `Linux`/`Darwin`, which was +# correct on its own but made the four scripts disagree — and a `case` copied +# between two of them is exactly how update.sh ended up with stop_service and +# start_service as silent no-ops. +case "$(uname -s)" in + Linux) OS="linux" ;; + Darwin) OS="darwin" ;; + *) OS="$(uname -s)" ;; +esac echo "" printf "\033[1m🗑️ Skald Circle — Uninstaller\033[0m\n" @@ -69,7 +78,7 @@ fi # ── Stop & remove daemon ────────────────────────────────────────────────────── case "$OS" in - Linux) + linux) SERVICE_NAME="skald-circle.service" SERVICE_PATH="$HOME/.config/systemd/user/$SERVICE_NAME" @@ -90,7 +99,7 @@ case "$OS" in fi ;; - Darwin) + darwin) PLIST="$HOME/Library/LaunchAgents/com.skald.circle.plist" if [ -f "$PLIST" ]; then @@ -114,7 +123,10 @@ esac # mid-cleanup; removing them here also frees the (sometimes root-owned) mount # files that would otherwise force the sudo fallback on the rm below. if command -v docker >/dev/null 2>&1 && docker info >/dev/null 2>&1; then - CONTAINERS="$(docker ps -aq --filter 'name=skald-' 2>/dev/null || true)" + # Anchored: Docker's name filter is a regex matched anywhere in the name, so + # an unanchored `skald-` also selects somebody else's `my-skald-proxy` — and + # the next line is `docker rm -f`. Ours are always `skald-{userid}`. + CONTAINERS="$(docker ps -aq --filter 'name=^skald-' 2>/dev/null || true)" if [ -n "$CONTAINERS" ]; then info "🐳 Removing Skald Docker containers …" # shellcheck disable=SC2086 # word-splitting is intentional (multiple IDs) @@ -128,7 +140,7 @@ if command -v docker >/dev/null 2>&1 && docker info >/dev/null 2>&1; then fi elif command -v docker >/dev/null 2>&1; then warn "Docker daemon not reachable — skipping container cleanup." - warn "Remove leftovers later with: docker rm -f \$(docker ps -aq --filter name=skald-)" + warn "Remove leftovers later with: docker rm -f \$(docker ps -aq --filter name=^skald-)" fi # ── Remove installation directory ───────────────────────────────────────────── @@ -151,6 +163,24 @@ fi echo "" info "✅ Skald Circle has been uninstalled." + +# The installer enables systemd lingering for this user, which is a persistent +# per-user setting and not ours to take back: any other `systemctl --user` +# service on this box may be relying on it by now, and silently disabling it +# would stop those too. So we say it and leave the choice to the human. +if [ "$OS" = "linux" ] && command -v loginctl >/dev/null 2>&1; then + case "$(loginctl show-user "${USER:-$(id -un)}" --property=Linger 2>/dev/null || true)" in + *=yes) + echo "" + echo " Note: systemd lingering is still enabled for ${USER:-$(id -un)}." + echo " It was enabled at install so the server survived logout. If no other" + echo " user service needs it, turn it off with:" + echo " sudo loginctl disable-linger ${USER:-$(id -un)}" + ;; + esac +fi + +echo "" echo " If you want to reinstall:" echo " curl -fsSL https://builds.skaldagent.net/install.sh | bash" echo " curl -fsSL https://builds.skaldagent.net/install-nightly.sh | bash" diff --git a/update.sh b/update.sh index 53a7858..1e45aed 100755 --- a/update.sh +++ b/update.sh @@ -296,6 +296,31 @@ main() { exit 1 fi + # ── Drop files the new build no longer ships ─────────────────────────────── + # Extracting over the install dir only ever adds and overwrites, so anything + # deleted upstream survives forever: a renamed doc page keeps being mounted + # read-only into every container for the assistant to read, a removed command + # keeps being discovered. For the directories the tarball owns end to end we + # therefore prune whatever the (already verified) staging copy does not have. + # + # Pruned AFTER extracting rather than by replacing the directory, so every + # intermediate state is a complete install and the only files removed are + # ones the new build has verifiably dropped. + # + # agents/ is deliberately not in this list: adding an agent is a documented + # extension point (agents//meta.json + AGENT.md), so the directory is not + # ours alone and pruning it would delete somebody's work — at the price of an + # upstream-deleted agent lingering. bin/ is out too: two files, both + # overwritten every time, nothing to reclaim. + for owned in web commands skills docs; do + [ -d "$STAGING/$owned" ] && [ -d "${INSTALL_DIR}/$owned" ] || continue + ( cd "${INSTALL_DIR}/$owned" && find . -type f ) | while IFS= read -r rel; do + rel="${rel#./}" + [ -e "${STAGING}/${owned}/${rel}" ] || rm -f "${INSTALL_DIR}/${owned}/${rel}" + done + find "${INSTALL_DIR}/$owned" -mindepth 1 -type d -empty -delete 2>/dev/null || true + done + # Update version file for release channel if [ "$CHANNEL" = "release" ]; then echo "$VERSION" > "$INSTALL_DIR/.release-version"