fix(updater): harden failure paths, lock apt privilege behind a root-owned wrapper, and self-heal broken components

BlocksScreen/lib/moonrakerComm.py

@@ -22,7 +22,7 @@ class OneShotTokenError(Exception):
     """Raised when unable to get oneshot token to connect to a websocket"""
 
     def __init__(self, message="Unable to get oneshot token", errors=None) -> None:
-        super(OneShotTokenError).__init__(message, errors)
+        super().__init__(message)
         self.errors = errors
         self.message = message
 

BlocksScreen/lib/panels/widgets/updatePage.py

@@ -137,9 +137,7 @@ def resizeEvent(self, a0: QtGui.QResizeEvent | None) -> None:
         return super().resizeEvent(a0)
 
     def _needs_update(self, status: ComponentStatus) -> bool:
-        # Mirrors the daemon's dirty-set in _run_update_all: an errored git repo
-        # (e.g. a corrupt repo) is included so the one "Update" button shows and
-        # the update flow self-heals it. apt errors are not repairable this way.
+        # Mirrors daemon dirty-set: errored git repos self-heal; apt errors don't.
         return bool(
             status.commits_behind
             or status.packages_upgradable > 0
@@ -315,9 +313,10 @@ def handle_status_ready(self, json_str: str) -> None:
         except (json.JSONDecodeError, TypeError) as exc:
             _log.error("handle_status_ready: bad payload '%s'", exc)
             _log.debug(json_str)
+            # Keep the last good list but tell the user it may be stale.
+            self._show_toast("Status update failed - tap refresh to retry")
             return
-        # Build per-component so one malformed entry (e.g. a newer daemon adding
-        # a field this UI build doesn't know) can't blank the whole list.
+        # Build per-component so one malformed entry can't blank the whole list.
         self._statuses = {}
         for name, fields in data.items():
             try:
@@ -381,6 +380,9 @@ def on_update_all_clicked(self) -> None:
         self._show_update_confirm()
 
     def _show_update_confirm(self) -> None:
+        # Dialogs parented to the page outlive close(); drop the previous one.
+        if self._update_confirm_popup is not None:
+            self._update_confirm_popup.deleteLater()
         popup = BasePopup(self, floating=True)
         popup.set_message(
             "The printer will restart.\n"

BlocksScreen/lib/updater_worker.py

@@ -22,10 +22,7 @@
 
 _RECONNECT_DELAYS = (5.0, 15.0, 30.0, 60.0)
 
-# Max seconds of *silence* from a busy daemon before declaring it gone.
-# Progress signals refresh the deadline, so long multi-component updates
-# (big apt upgrades, several klipper restarts) never trip it as long as
-# the daemon keeps reporting steps.
+# Max silence from a busy daemon; progress signals refresh the deadline.
 _BUSY_IDLE_LIMIT = 360.0
 
 
@@ -147,16 +144,14 @@ async def _async_initialize(self) -> None:
             self._listener_tasks.append(task)
             task.add_done_callback(self._on_listener_done)
 
-        # Let each listener enter its async-for and register its D-Bus signal
-        # subscription before we poll current state (one sleep(0) per task).
+        # One sleep(0) per task lets each listener register its subscription.
         for _ in listeners:
             await asyncio.sleep(0)
 
         try:
             busy = await self._proxy.get_busy()
         except sdbus.SdBusBaseError as exc:
-            # Proxy creation is lazy; this first real call is what proves the
-            # daemon is actually reachable. Treat failure as daemon-down.
+            # Proxy is lazy; this first call proves the daemon is reachable.
             _log.warning("get_busy failed on (re)connect: %s - scheduling retry", exc)
             self.daemon_unavailable.emit()
             self._schedule_reconnect()

scripts/bs-apt-helper.sh

@@ -0,0 +1,53 @@
+#!/bin/bash
+# bs-apt-helper: the only apt entry sudoers grants 'blocks'; fixed argvs stop option injection.
+set -euo pipefail
+
+APT_GET=/usr/bin/apt-get
+DPKG=/usr/bin/dpkg
+LOCK=(-o DPkg::Lock::Timeout=60)
+CONF=(-o Dpkg::Options::=--force-confdef -o Dpkg::Options::=--force-confold)
+export DEBIAN_FRONTEND=noninteractive NEEDRESTART_MODE=a
+
+usage() {
+    echo "usage: bs-apt-helper {update|upgrade <pkg...>|autoremove|fix-broken|dselect-upgrade|set-selections}" >&2
+    exit 2
+}
+
+verb="${1:-}"
+[ $# -ge 1 ] && shift
+case "$verb" in
+    update)
+        [ $# -eq 0 ] || usage
+        exec "$APT_GET" "${LOCK[@]}" update
+        ;;
+    upgrade)
+        [ $# -ge 1 ] || usage
+        for p in "$@"; do
+            # Debian name shape; first char alnum so '-options' can't pass.
+            [[ "$p" =~ ^[a-zA-Z0-9][a-zA-Z0-9+.-]*$ ]] || {
+                echo "bs-apt-helper: invalid package name: $p" >&2
+                exit 2
+            }
+        done
+        exec "$APT_GET" "${LOCK[@]}" "${CONF[@]}" install --only-upgrade -y "$@"
+        ;;
+    autoremove)
+        [ $# -eq 0 ] || usage
+        exec "$APT_GET" "${LOCK[@]}" autoremove -y
+        ;;
+    fix-broken)
+        [ $# -eq 0 ] || usage
+        exec "$APT_GET" "${LOCK[@]}" "${CONF[@]}" -f install -y
+        ;;
+    dselect-upgrade)
+        [ $# -eq 0 ] || usage
+        exec "$APT_GET" "${LOCK[@]}" "${CONF[@]}" dselect-upgrade -y
+        ;;
+    set-selections)
+        [ $# -eq 0 ] || usage
+        exec "$DPKG" --set-selections
+        ;;
+    *)
+        usage
+        ;;
+esac

scripts/install-updater.sh

@@ -15,8 +15,7 @@ flock -x -w 30 200 || { echo_error "Another install-updater.sh is already runnin
 
 SCRIPT_PATH=$(dirname -- "$(readlink -f -- "$0")")
 BS_PATH=$(dirname "$SCRIPT_PATH")
-# Service always runs as 'blocks'; derive home from that, not the caller's identity.
-# Callers vary (sudo, post-merge hook, root shell) so SUDO_USER/$USER are unreliable.
+# Always derive home from 'blocks': callers vary, SUDO_USER/$USER unreliable.
 _BSENV_USER="blocks"
 _BSENV_HOME=$(getent passwd "$_BSENV_USER" | cut -d: -f6)
 BSENV="${BLOCKSSCREEN_VENV:-${_BSENV_HOME}/.BlocksScreen-env}"
@@ -30,6 +29,12 @@ echo_info "Installing D-Bus activation file ..."
 sudo cp "$SCRIPT_PATH/com.blockscreen.Updater.service" /usr/share/dbus-1/system-services/
 echo_ok "D-Bus activation file installed"
 
+echo_info "Installing bs-apt-helper (root-owned apt wrapper) ..."
+# Atomic + before the daemon (re)start: new updater code has no apt fallback.
+sudo install -o root -g root -m 0755 "$SCRIPT_PATH/bs-apt-helper.sh" /usr/local/sbin/.bs-apt-helper.new
+sudo mv -Tf /usr/local/sbin/.bs-apt-helper.new /usr/local/sbin/bs-apt-helper
+echo_ok "bs-apt-helper installed"
+
 echo_info "Installing BlocksScreen-updater service ..."
 SERVICE=$(cat "$SCRIPT_PATH/BlocksScreen-updater.service")
 SERVICE=${SERVICE//BS_DIR/$BS_PATH}
@@ -51,18 +56,13 @@ echo_ok "Spoolman service unit installed"
 echo_info "Installing sudoers rules for updater ..."
 SUDOERS_FILE="/etc/sudoers.d/blockscreen-updater"
 SUDOERS_TMP=$(mktemp)
-printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get update\n' >>"$SUDOERS_TMP"
-printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get install --only-upgrade -y *\n' >>"$SUDOERS_TMP"
-printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get install -y --quiet *\n' >>"$SUDOERS_TMP"
-printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/apt-get autoremove -y\n' >>"$SUDOERS_TMP"
+# The wrapper is the only apt/dpkg grant; kept in sync by tests/scripts/test_sudoers_rules.py.
+printf 'blocks ALL=(ALL) NOPASSWD: /usr/local/sbin/bs-apt-helper *\n' >>"$SUDOERS_TMP"
 printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/systemctl reboot\n' >>"$SUDOERS_TMP"
 printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/chvt 7\n' >>"$SUDOERS_TMP"
 printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/chvt 8\n' >>"$SUDOERS_TMP"
 printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/bash %s/scripts/install-updater.sh\n' "$BS_PATH" >>"$SUDOERS_TMP"
-# Derive allowed restart targets from components.yaml instead of wildcard.
-# Per service we allow: restart (normal), reset-failed (start-limit recovery,
-# replaces the old systemctl-kill fallback), and --no-block restart (used for the
-# self UI service so a slow restart never blocks/aborts the update batch).
+# Per components.yaml service: restart, reset-failed (start-limit), --no-block restart.
 _emit_svc_rules() {
     local _s="$1"
     printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/systemctl restart %s\n' "$_s" >>"$SUDOERS_TMP"
@@ -79,11 +79,9 @@ else
         _emit_svc_rules "$_svc"
     done
 fi
-# The daemon restarts itself (--no-block) to adopt new updater code after a
-# successful self-update; this service is never in components.yaml.
+# Daemon self-restart target; never in components.yaml.
 _emit_svc_rules BlocksScreen-updater.service
-# Spoolman is provisioned on demand: the daemon enables its unit after a clean
-# first start (restart rules are auto-derived above from components.yaml).
+# Spoolman is provisioned on demand; enable rule needed for its first clean start.
 printf 'blocks ALL=(ALL) NOPASSWD: /usr/bin/systemctl enable Spoolman.service\n' >>"$SUDOERS_TMP"
 if sudo visudo -cf "$SUDOERS_TMP" >/dev/null 2>&1; then
     sudo install -m 0440 "$SUDOERS_TMP" "$SUDOERS_FILE"
@@ -110,13 +108,10 @@ echo_info "Converting BlocksScreen.service copy to symlink (enables sudo-free ho
 _BS_SVC_SRC="$BS_PATH/scripts/BlocksScreen.service"
 _BS_SVC_DEST="/etc/systemd/system/BlocksScreen.service"
 if [[ ! -f "$_BS_SVC_SRC" ]]; then
-    # Never remove the running unit when there is no source to replace it with:
-    # a missing BlocksScreen.service on a no-SSH box is unrecoverable.
+    # Never remove the running unit without a replacement; the device has no SSH recovery.
     echo_info "WARN: $_BS_SVC_SRC missing, leaving existing BlocksScreen.service intact"
 elif [[ "$(readlink -f "$_BS_SVC_DEST" 2>/dev/null)" != "$(readlink -f "$_BS_SVC_SRC")" ]]; then
-    # Atomic replace: create the symlink under a temp name and rename it over the
-    # destination, so the unit is never absent. The old rm-then-link left a window
-    # where a link failure (e.g. missing source) deleted the service outright.
+    # Atomic replace via temp symlink + rename: the unit is never absent.
     sudo ln -sfn "$_BS_SVC_SRC" "${_BS_SVC_DEST}.new"
     sudo mv -Tf "${_BS_SVC_DEST}.new" "$_BS_SVC_DEST"
     sudo systemctl unmask BlocksScreen.service 2>/dev/null || true
@@ -132,11 +127,7 @@ sudo chown -R blocks:blocksscreen "$_BSENV_HOME/.cache/blockscreen"
 sudo chmod 775 "$_BSENV_HOME/.cache/blockscreen"
 echo_ok "Apt cache directory ready"
 
-# Re-arm the crash-loop boot counter on a deliberate deploy. A stale count left
-# over from a prior unstable period would otherwise let BlocksScreen-start.sh
-# roll this fresh install straight back to last_good on the first boot. Reset
-# boot_attempts only; last_good_commit stays as the rollback target so crash-loop
-# protection of the new code still works.
+# Deploy re-arms boot_attempts only; last_good stays as the rollback target.
 echo_info "Re-arming crash-loop boot counter ..."
 printf '0\n' | sudo -u "$_BSENV_USER" tee "$_BSENV_HOME/.cache/blockscreen/boot_attempts" >/dev/null 2>&1 || true
 echo_ok "Boot counter re-armed (boot_attempts=0)"
@@ -151,9 +142,18 @@ sudo systemd-tmpfiles --create --prefix /var/log/journal 2>/dev/null || true
 sudo systemctl restart systemd-journald 2>/dev/null || true
 echo_ok "Persistent journald enabled"
 
-echo_info "Allowing blocks to operate on repos owned by primary user ..."
-sudo git config --system --add safe.directory '*'
-echo_ok "Git safe.directory configured"
+echo_info "Scoping git safe.directory to component repos ..."
+# Scope root git trust to component paths instead of '*'.
+sudo git config --system --unset-all safe.directory 2>/dev/null || true
+_SAFE_YAML="$BS_PATH/updater/components.yaml"
+if [[ -f "$_SAFE_YAML" ]]; then
+    grep '^\s*path:' "$_SAFE_YAML" | awk '{print $2}' | sort -u | while read -r _p; do
+        sudo git config --system --add safe.directory "${_p/#\~/$_BSENV_HOME}"
+    done
+fi
+git config --system --get-all safe.directory 2>/dev/null | grep -qxF "$BS_PATH" || \
+    sudo git config --system --add safe.directory "$BS_PATH"
+echo_ok "Git safe.directory scoped"
 
 echo_info "Setting git repo permissions for updater ..."
 COMPONENTS_YAML="$BS_PATH/updater/components.yaml"
@@ -206,14 +206,12 @@ git -C "$BS_PATH" config core.hooksPath scripts
 echo_ok "post-merge hook installed"
 
 echo_info "Installing Python requirements ..."
-# Keep pip itself current on each update (best-effort: a failed self-upgrade must
-# not block the requirements install below).
+# Best-effort pip self-update; must not block the requirements install below.
 "$BSENV/bin/pip" install --quiet --upgrade pip 2>/dev/null || true
 apt-get install -y --quiet libsystemd-dev python3-dev 2>/dev/null || true
 # xsetroot is used as belt-and-suspenders cursor hiding alongside the Xorg -nocursor server flag.
 sudo apt-get install -y --quiet x11-xserver-utils 2>/dev/null || true
-# sdbus is pinned in requirements.txt (0.12.0); --no-binary forces a clean source build
-# (needs libsystemd-dev). --upgrade-strategy=only-if-needed skips satisfied packages.
+# sdbus pinned; --no-binary needs libsystemd-dev; only-if-needed skips satisfied.
 "$BSENV/bin/pip" install --quiet --only-binary :all: --no-binary sdbus,sdbus-networkmanager \
     --upgrade-strategy=only-if-needed \
     -r "$BS_PATH/scripts/requirements.txt" || true

scripts/post-merge

@@ -14,8 +14,7 @@ bs_migrate_moonraker_conf "$_BSENV_HOME/printer_data/config/moonraker.conf" post
 
 changed=$(git -C "$BS_PATH" diff ORIG_HEAD HEAD --name-only 2>/dev/null || true)
 
-# ── Pre-flight: run all heavy prep before any restart ────────────────────────
-# Everything here is idempotent - safe to re-run on every merge.
+# Pre-flight: heavy prep before any restart; everything here is idempotent.
 
 # 1. Pre-install pip requirements so the next service start is instant.
 REQS_HASH=$(md5sum "$BS_PATH/scripts/requirements.txt" | cut -d' ' -f1)
@@ -32,10 +31,7 @@ if [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; then
         rm -f "$ATTEMPT" 2>/dev/null || true
         echo "[post-merge] deps installed"
     else
-        # An atomic -r failure (e.g. a dep without an aarch64 wheel) must not
-        # re-thrash on every pull. Retry a few times - covering transient or
-        # offline failures - then accept the partial venv so the sentinel
-        # advances and the boot path stops retrying a known-failing set.
+        # Retry atomic -r failures 3x, then accept the partial venv to stop thrash.
         _prev=$(cut -d' ' -f1 "$ATTEMPT" 2>/dev/null || true)
         _cnt=$(cut -d' ' -f2 "$ATTEMPT" 2>/dev/null || true)
         case "$_cnt" in '' | *[!0-9]*) _cnt=0 ;; esac
@@ -60,13 +56,8 @@ fi
 # 4. Precompute splash cache so the first Qt restart shows the right logo.
 "$BSENV/bin/python3.11" "$SCRIPT_PATH/bs-splash.py" --precompute 2>/dev/null || true
 
-# ─────────────────────────────────────────────────────────────────────────────
 
-# ── Service restart based on what changed ────────────────────────────────────
-# When run by the updater daemon's own `git pull` (mid-batch, inside its cgroup),
-# restarting the daemon here would SIGTERM the cgroup and abort+revert the
-# update. In that case record the reason to the sentinel and let the daemon act
-# once, after the batch completes. Manual pulls (no env) restart as before.
+# Under the daemon's own pull a restart here aborts the batch: defer via sentinel.
 _record_or() {
     local _reason="$1"; shift
     if [ -n "${BS_UPDATER_SELF_UPDATE:-}" ] && [ -n "${BS_UPDATER_RESTART_SENTINEL:-}" ]; then
@@ -78,7 +69,7 @@ _record_or() {
     fi
 }
 
-if echo "$changed" | grep -qE '^scripts/BlocksScreen-updater\.service$|^scripts/BlocksScreen-bootstrap\.service$|^scripts/com\.blockscreen\.Updater\.conf$|^scripts/install-updater\.sh$'; then
+if echo "$changed" | grep -qE '^scripts/BlocksScreen-updater\.service$|^scripts/BlocksScreen-bootstrap\.service$|^scripts/com\.blockscreen\.Updater\.conf$|^scripts/install-updater\.sh$|^scripts/bs-apt-helper\.sh$'; then
     echo "[post-merge] Updater install files changed - reinstalling updater ..."
     _record_or install sudo bash "$SCRIPT_PATH/install-updater.sh"
 elif echo "$changed" | grep -qE '^scripts/bs-splash-holder\.py$'; then

tests/amu/test_config_toggler_unit.py

@@ -1,6 +1,5 @@
 """Unit tests for BlocksScreen.devices.amu.config_toggler."""
 
-import pytest
 from BlocksScreen.devices.amu.config_toggler import ConfigToggler
 from tests.amu.conftest import COMMENTED_CFG, UNCOMMENTED_CFG
 

tests/amu/test_manager_unit.py

@@ -157,7 +157,7 @@ def test_from_status_gate_speed_override_defaults_to_empty(self, manager) -> Non
         assert manager.get_state().gate_speed_override == ()
 
     def test_from_status_parses_endless_spool_groups(self, manager) -> None:
-        data = {**_FULL_STATUS, "endless_spool_groups": [0, 1, 3,0]}
+        data = {**_FULL_STATUS, "endless_spool_groups": [0, 1, 3, 0]}
         manager.update_mmu_state(data)
         assert manager.get_state().endless_spool_groups == (0, 1, 3, 0)
 

tests/amu/test_models_unit.py

@@ -140,7 +140,7 @@ def _full_status_extended(self, num_gates: int = 2) -> dict:
     def test_from_status_builds_correctly(self) -> None:
         state: MMUState = MMUState.from_status(self._full_status())
         assert state.num_gates == 2
-        assert state.enabled == True
+        assert state.enabled
         assert len(state.gates) == 2
         assert state.gates[0].material == "PLA"
         assert state.gates[1].status == GateStatus.EMPTY

tests/amu/test_moonrest_unit.py

@@ -1,18 +1,24 @@
 """Unit tests for BlocksScreen.lib.moonrest"""
+
 from unittest.mock import patch
 import pytest
 from BlocksScreen.lib.moonrest import MoonRest
 
+
 @pytest.fixture
 def rest():
     """MoonRest instance poiting at localhost"""
     return MoonRest(host="localhost", port=7125)
 
+
 class TestGetSpool:
     def test_return_spool_dict_on_sucess(self, rest) -> None:
-        spool_data = {"id": 42, "filament": {"name": "PLA", "color_hex": "ff0000"},
-                      "used_weight": 50.0, "remaining_weight": 200.0
-                      }
+        spool_data = {
+            "id": 42,
+            "filament": {"name": "PLA", "color_hex": "ff0000"},
+            "used_weight": 50.0,
+            "remaining_weight": 200.0,
+        }
         with patch.object(rest, "get_request", return_value={"result": spool_data}):
             assert rest.get_spool(42) == spool_data
 
@@ -29,6 +35,7 @@ def test_calls_correct_endpoint(self, rest) -> None:
             rest.get_spool(7)
         mock_get.assert_called_once_with("server/spoolman/spool/7")
 
+
 class TestSetSpoolUsedWeight:
     def test_returns_true_on_sucess(self, rest) -> None:
         with patch.object(rest, "post_request", return_value={"result": "ok"}):
@@ -37,15 +44,10 @@ def test_returns_true_on_sucess(self, rest) -> None:
     def test_returns_false_on_error(self, rest) -> None:
         with patch.object(rest, "post_request", return_value={"result": "ok"}):
             assert rest.set_spool_used_weight(42, 75.5)
-    
+
     def test_calls_correct_endpoint(self, rest) -> None:
         with patch.object(rest, "post_request", return_value=None) as mock_post:
             rest.set_spool_used_weight(5, 100.0)
         mock_post.assert_called_once_with(
-            "server/spoolman/spool/5", json={
-                "used_weight": 100.0
-            }
+            "server/spoolman/spool/5", json={"used_weight": 100.0}
         )
-
-
-

tests/lib/test_moonraker_comm_unit.py

@@ -0,0 +1,19 @@
+"""Unit tests for moonrakerComm exception types."""
+
+from BlocksScreen.lib.moonrakerComm import OneShotTokenError
+
+
+class TestOneShotTokenError:
+    def test_str_carries_message(self):
+        # A super(OneShotTokenError).__init__ regression left str(exc) empty.
+        exc = OneShotTokenError("token fetch failed")
+        assert str(exc) == "token fetch failed"
+        assert exc.message == "token fetch failed"
+
+    def test_default_message(self):
+        exc = OneShotTokenError()
+        assert str(exc) == "Unable to get oneshot token"
+
+    def test_errors_attribute_kept(self):
+        exc = OneShotTokenError("boom", errors={"code": 401})
+        assert exc.errors == {"code": 401}