From f1dbec16531cd3d5f0b9124accedb8cb8e49dea3 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Sun, 24 May 2026 04:15:30 -0500 Subject: fix(system-commands): make Emacs restart and destructive confirms defensive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restart-Emacs scheduled an unconditional kill-emacs one second after firing the systemctl restart. If the service was missing or the restart failed, the session still got killed with nothing to replace it. Restart now guards on (daemonp) and a present emacs.service before doing anything, and drops the separate kill-emacs entirely — systemctl restart cycles the daemon itself, so a failed restart leaves the current Emacs alive. Added cj/system-cmd--emacs-service-available-p (systemctl --user cat) for the guard. Shutdown and reboot now use a strong yes-or-no-p confirm instead of the quick (Y/n) read-char, where RET or space counted as yes — a stray Enter at the prompt could power off the machine. Logout and suspend keep the quick confirm since they are recoverable. The confirm tier rides on a property set by cj/defsystem-command. Tests cover service detection, both restart guards, and the strong-confirm accept/decline paths with the system primitives stubbed. --- modules/system-commands.el | 66 ++++++++---- tests/test-system-commands-resolve-and-run.el | 143 +++++++++++++++++++------- 2 files changed, 150 insertions(+), 59 deletions(-) diff --git a/modules/system-commands.el b/modules/system-commands.el index bd22468c..afb59747 100644 --- a/modules/system-commands.el +++ b/modules/system-commands.el @@ -57,12 +57,20 @@ If CMD is deemed dangerous, ask for confirmation." (sym (nth 0 resolved)) (cmdstr (nth 1 resolved)) (label (nth 2 resolved))) - (when (and sym (get sym 'cj/system-confirm) - (memq (read-char-choice - (format "Run %s now (%s)? (Y/n) " label cmdstr) - '(?y ?Y ?n ?N ?\r ?\n ?\s)) - '(?n ?N))) - (user-error "Aborted")) + (let ((confirm (and sym (get sym 'cj/system-confirm)))) + (cond + ;; Strong confirm for irreversible actions (shutdown, reboot): + ;; require an explicit "yes", so a stray RET/space can't trigger them. + ((eq confirm 'strong) + (unless (yes-or-no-p (format "Really run %s (%s)? " label cmdstr)) + (user-error "Aborted"))) + ;; Quick (Y/n) confirm for recoverable actions (logout, suspend). + (confirm + (when (memq (read-char-choice + (format "Run %s now (%s)? (Y/n) " label cmdstr) + '(?y ?Y ?n ?N ?\r ?\n ?\s)) + '(?n ?N)) + (user-error "Aborted"))))) (let ((proc (start-process-shell-command "cj/system-cmd" nil (format "nohup %s >/dev/null 2>&1 &" cmdstr)))) (set-process-query-on-exit-flag proc nil) @@ -71,11 +79,13 @@ If CMD is deemed dangerous, ask for confirmation." (defmacro cj/defsystem-command (name var cmdstr &optional confirm) "Define VAR with CMDSTR and interactive command NAME to run it. -If CONFIRM is non-nil, mark VAR to always require confirmation." +CONFIRM controls the confirmation prompt: t for a quick (Y/n) prompt, +the symbol `strong' for an explicit yes-or-no-p (used for irreversible +actions like shutdown and reboot), nil for no confirmation." (declare (indent defun)) `(progn (defvar ,var ,cmdstr) - ,(when confirm `(put ',var 'cj/system-confirm t)) + ,(when confirm `(put ',var 'cj/system-confirm ',confirm)) (defun ,name () ,(format "Run %s via `cj/system-cmd'." var) (interactive) @@ -85,8 +95,8 @@ If CONFIRM is non-nil, mark VAR to always require confirmation." (cj/defsystem-command cj/system-cmd-logout logout-cmd "loginctl terminate-user $(whoami)" t) (cj/defsystem-command cj/system-cmd-lock lockscreen-cmd "slock") (cj/defsystem-command cj/system-cmd-suspend suspend-cmd "systemctl suspend" t) -(cj/defsystem-command cj/system-cmd-shutdown shutdown-cmd "systemctl poweroff" t) -(cj/defsystem-command cj/system-cmd-reboot reboot-cmd "systemctl reboot" t) +(cj/defsystem-command cj/system-cmd-shutdown shutdown-cmd "systemctl poweroff" strong) +(cj/defsystem-command cj/system-cmd-reboot reboot-cmd "systemctl reboot" strong) (defun cj/system-cmd-exit-emacs () "Exit Emacs server and all clients." @@ -98,23 +108,41 @@ If CONFIRM is non-nil, mark VAR to always require confirmation." (user-error "Aborted")) (kill-emacs)) +(defun cj/system-cmd--emacs-service-available-p () + "Return non-nil if a systemd --user emacs.service unit is present. +Used to decide whether `cj/system-cmd-restart-emacs' can restart via the +service before relying on it to cycle the daemon. `systemctl --user cat' +exits 0 when the unit exists, nonzero otherwise." + (and (executable-find "systemctl") + (eq 0 (call-process "systemctl" nil nil nil + "--user" "cat" "emacs.service")))) + (defun cj/system-cmd-restart-emacs () - "Restart Emacs server after saving buffers." + "Restart the Emacs daemon via its systemd --user service, then reconnect. +Aborts without terminating anything when not running as a daemon or when +no emacs.service is present, so a missing or failed service can't leave +you with no Emacs running. The service owns the daemon lifecycle, so +there is no separate `kill-emacs': a failed restart leaves the current +daemon alive rather than killing the session blindly." (interactive) + (unless (daemonp) + (user-error "Not running as an Emacs daemon; restart Emacs manually")) + (unless (cj/system-cmd--emacs-service-available-p) + (user-error "No systemd --user emacs.service found; restart it manually")) (when (memq (read-char-choice "Restart Emacs? (Y/n) " '(?y ?Y ?n ?N ?\r ?\n ?\s)) '(?n ?N)) (user-error "Aborted")) (save-some-buffers) - ;; Start the restart process before killing Emacs - (run-at-time 0.5 nil - (lambda () - (call-process-shell-command - "systemctl --user restart emacs.service && emacsclient -c" - nil 0))) - (run-at-time 1 nil #'kill-emacs) - (message "Restarting Emacs...")) + ;; Hand the whole restart+reconnect to a detached shell. `systemctl + ;; restart' tears down this daemon itself; the detached `emacsclient -c' + ;; reconnects to the fresh one. + (call-process-shell-command + (concat "nohup sh -c 'systemctl --user restart emacs.service " + "&& sleep 1 && emacsclient -c' >/dev/null 2>&1 &") + nil 0) + (message "Restarting Emacs via emacs.service...")) (defun cj/system-command-menu () "Present system commands via \='completing-read\='." diff --git a/tests/test-system-commands-resolve-and-run.el b/tests/test-system-commands-resolve-and-run.el index f15559d7..2c9d98d0 100644 --- a/tests/test-system-commands-resolve-and-run.el +++ b/tests/test-system-commands-resolve-and-run.el @@ -5,9 +5,10 @@ ;; This file covers the runtime helpers and commands: ;; ;; cj/system-cmd--resolve -;; cj/system-cmd +;; cj/system-cmd (quick + strong confirm) +;; cj/system-cmd--emacs-service-available-p ;; cj/system-cmd-exit-emacs -;; cj/system-cmd-restart-emacs +;; cj/system-cmd-restart-emacs (daemon + service guards) ;; cj/system-command-menu ;; ;; Process and prompt primitives are stubbed. @@ -73,7 +74,7 @@ (should (string-match-p "&$" cmd-line)))) (ert-deftest test-system-cmd-confirm-decline-aborts () - "Boundary: a confirm-tagged var with N response signals user-error." + "Boundary: a quick-confirm var with N response signals user-error." (defvar test-sc-confirm-cmd "test-confirm-cmd") (put 'test-sc-confirm-cmd 'cj/system-confirm t) (unwind-protect @@ -83,6 +84,57 @@ (should-error (cj/system-cmd 'test-sc-confirm-cmd) :type 'user-error)) (put 'test-sc-confirm-cmd 'cj/system-confirm nil))) +(ert-deftest test-system-cmd-strong-confirm-decline-aborts () + "Boundary: a strong-confirm var uses yes-or-no-p; declining aborts and +does not run the command." + (defvar test-sc-strong-cmd "test-strong-cmd") + (put 'test-sc-strong-cmd 'cj/system-confirm 'strong) + (unwind-protect + (cl-letf (((symbol-function 'yes-or-no-p) (lambda (&rest _) nil)) + ((symbol-function 'read-char-choice) + (lambda (&rest _) (error "strong confirm must not use read-char-choice"))) + ((symbol-function 'start-process-shell-command) + (lambda (&rest _) (error "shouldn't run")))) + (should-error (cj/system-cmd 'test-sc-strong-cmd) :type 'user-error)) + (put 'test-sc-strong-cmd 'cj/system-confirm nil))) + +(ert-deftest test-system-cmd-strong-confirm-accept-runs () + "Normal: a strong-confirm var runs the command when yes-or-no-p returns t." + (defvar test-sc-strong-cmd-2 "echo strong") + (put 'test-sc-strong-cmd-2 'cj/system-confirm 'strong) + (let (cmd-line) + (unwind-protect + (cl-letf (((symbol-function 'yes-or-no-p) (lambda (&rest _) t)) + ((symbol-function 'start-process-shell-command) + (lambda (_name _buf c) (setq cmd-line c) 'fake-proc)) + ((symbol-function 'set-process-query-on-exit-flag) #'ignore) + ((symbol-function 'set-process-sentinel) #'ignore) + ((symbol-function 'message) #'ignore)) + (cj/system-cmd 'test-sc-strong-cmd-2)) + (put 'test-sc-strong-cmd-2 'cj/system-confirm nil)) + (should (string-match-p "echo strong" cmd-line)))) + +;;; cj/system-cmd--emacs-service-available-p + +(ert-deftest test-system-cmd-service-available-true-on-zero-exit () + "Normal: service is available when systemctl exists and `cat' exits 0." + (cl-letf (((symbol-function 'executable-find) (lambda (_p) "/usr/bin/systemctl")) + ((symbol-function 'call-process) (lambda (&rest _) 0))) + (should (cj/system-cmd--emacs-service-available-p)))) + +(ert-deftest test-system-cmd-service-available-false-on-nonzero-exit () + "Boundary: a nonzero exit (no such unit) means not available." + (cl-letf (((symbol-function 'executable-find) (lambda (_p) "/usr/bin/systemctl")) + ((symbol-function 'call-process) (lambda (&rest _) 1))) + (should-not (cj/system-cmd--emacs-service-available-p)))) + +(ert-deftest test-system-cmd-service-available-false-when-systemctl-absent () + "Error: with no systemctl on PATH the service can't be available." + (cl-letf (((symbol-function 'executable-find) (lambda (_p) nil)) + ((symbol-function 'call-process) + (lambda (&rest _) (error "must not shell out without systemctl")))) + (should-not (cj/system-cmd--emacs-service-available-p)))) + ;;; cj/system-cmd-exit-emacs (ert-deftest test-system-cmd-exit-emacs-decline-aborts () @@ -105,49 +157,60 @@ ;;; cj/system-cmd-restart-emacs -(ert-deftest test-system-cmd-restart-emacs-decline-aborts () - "Boundary: declining the prompt signals user-error before scheduling." - (let ((scheduled nil)) - (cl-letf (((symbol-function 'read-char-choice) (lambda (&rest _) ?n)) - ((symbol-function 'save-some-buffers) #'ignore) - ((symbol-function 'run-at-time) - (lambda (&rest _) (setq scheduled t)))) +(ert-deftest test-system-cmd-restart-emacs-not-daemon-aborts () + "Error: a non-daemon Emacs refuses to restart-via-service and never prompts." + (let ((prompted nil) (ran nil)) + (cl-letf (((symbol-function 'daemonp) (lambda () nil)) + ((symbol-function 'read-char-choice) + (lambda (&rest _) (setq prompted t) ?y)) + ((symbol-function 'call-process-shell-command) + (lambda (&rest _) (setq ran t)))) + (should-error (cj/system-cmd-restart-emacs) :type 'user-error)) + (should-not prompted) + (should-not ran))) + +(ert-deftest test-system-cmd-restart-emacs-no-service-aborts () + "Error: when no emacs.service exists, restart aborts without running anything." + (let ((ran nil)) + (cl-letf (((symbol-function 'daemonp) (lambda () t)) + ((symbol-function 'cj/system-cmd--emacs-service-available-p) + (lambda () nil)) + ((symbol-function 'read-char-choice) (lambda (&rest _) ?y)) + ((symbol-function 'call-process-shell-command) + (lambda (&rest _) (setq ran t)))) (should-error (cj/system-cmd-restart-emacs) :type 'user-error)) - (should-not scheduled))) + (should-not ran))) -(ert-deftest test-system-cmd-restart-emacs-accept-schedules-restart () - "Normal: accepting the prompt schedules the restart + kill timers." - (let ((schedules 0)) - (cl-letf (((symbol-function 'read-char-choice) (lambda (&rest _) ?y)) +(ert-deftest test-system-cmd-restart-emacs-decline-aborts () + "Boundary: declining the prompt aborts before running the restart." + (let ((ran nil)) + (cl-letf (((symbol-function 'daemonp) (lambda () t)) + ((symbol-function 'cj/system-cmd--emacs-service-available-p) + (lambda () t)) + ((symbol-function 'read-char-choice) (lambda (&rest _) ?n)) ((symbol-function 'save-some-buffers) #'ignore) - ((symbol-function 'run-at-time) - (lambda (&rest _) (cl-incf schedules))) - ((symbol-function 'message) #'ignore)) - (cj/system-cmd-restart-emacs)) - (should (= schedules 2)))) - -(ert-deftest test-system-cmd-restart-emacs-restart-lambda-shells-out () - "Normal: the lambda scheduled by the first `run-at-time' call invokes -`call-process-shell-command' with the systemctl restart line. - -Captures the lambda passed to the first scheduling call and invokes it -directly with all relevant side effects stubbed, so the inner body -registers under coverage even though the real timer never fires." - (let (captured-lambda call-args) - (cl-letf (((symbol-function 'read-char-choice) (lambda (&rest _) ?y)) + ((symbol-function 'call-process-shell-command) + (lambda (&rest _) (setq ran t)))) + (should-error (cj/system-cmd-restart-emacs) :type 'user-error)) + (should-not ran))) + +(ert-deftest test-system-cmd-restart-emacs-accept-runs-service-restart () + "Normal: accepting runs the systemctl restart line and never calls +kill-emacs directly (the service owns the daemon lifecycle)." + (let (cmd-line (killed nil)) + (cl-letf (((symbol-function 'daemonp) (lambda () t)) + ((symbol-function 'cj/system-cmd--emacs-service-available-p) + (lambda () t)) + ((symbol-function 'read-char-choice) (lambda (&rest _) ?y)) ((symbol-function 'save-some-buffers) #'ignore) - ((symbol-function 'run-at-time) - (lambda (_when _repeat fn) - (unless captured-lambda (setq captured-lambda fn)))) ((symbol-function 'message) #'ignore) + ((symbol-function 'kill-emacs) + (lambda (&rest _) (setq killed t))) ((symbol-function 'call-process-shell-command) - (lambda (&rest args) (setq call-args args)))) - (cj/system-cmd-restart-emacs) - (should (functionp captured-lambda)) - (funcall captured-lambda) - (should call-args) - (should (string-match-p "systemctl --user restart emacs.service" - (car call-args)))))) + (lambda (c &rest _) (setq cmd-line c)))) + (cj/system-cmd-restart-emacs)) + (should (string-match-p "systemctl --user restart emacs.service" cmd-line)) + (should-not killed))) ;;; cj/system-command-menu -- cgit v1.2.3