fix: harden snapshot validation and spice handling

CHANGELOG.md

@@ -8,9 +8,19 @@ Versions follow [Semantic Versioning](https://semver.org/).
 
 ## [Unreleased]
 
-### Added
+### Fixed
 - VM IP lookup now accepts the native QEMU Guest Agent list payload from `qm agent ... network-get-interfaces`
   as well as the earlier wrapped shape, preventing parser errors on hosts that return a bare JSON array.
+- Snapshot name validation now matches Proxmox `pve-configid` rules before calling `pct` / `qm`;
+  numeric-only and single-letter names are rejected locally with a clearer message.
+- SPICE `.vv` files now prefer the actual SPICE bind address/port from Proxmox instead of always
+  using the host LAN IP, preventing mismatched connection targets on loopback-bound SPICE setups.
+- `spice_info()` no longer claims success launching `virt-viewer` from a non-graphical shell; it
+  falls back to the `.vv` file with a clear desktop-session hint.
+- VM console guidance now tells the user to follow the escape hint printed by Proxmox instead of
+  hardcoding a potentially wrong key sequence.
+
+### Added
 - `--filter STATUS` flag: filter `--list` / `--json` output to `running`, `stopped`, or `paused`
   instances; invalid values exit 1 with a clear error message.
 - `--timeout SECS` flag: set a timeout (default 60 s) for stop operations; on exit code 124

proxmox-manager.sh

@@ -192,12 +192,13 @@ validate_vmid() {
 }
 
 # validate_snapshot_name NAME — reject names Proxmox would refuse.
-# Valid: starts with alphanumeric, only [a-zA-Z0-9_-], max 40 chars.
+# Proxmox uses the pve-configid format for snapshot names:
+# start with a letter, then one or more of [a-zA-Z0-9_-], max 40 chars.
 validate_snapshot_name() {
   local sn="$1"
-  if [[ ! "$sn" =~ ^[a-zA-Z0-9][a-zA-Z0-9_-]{0,39}$ ]]; then
+  if [[ ! "$sn" =~ ^[a-zA-Z][a-zA-Z0-9_-]{1,39}$ ]]; then
     err "Invalid snapshot name '$sn'."
-    note "Name must start with a letter or digit, contain only [a-zA-Z0-9_-], and be at most 40 characters."
+    note "Name must start with a letter, contain only [a-zA-Z0-9_-], and be at most 40 characters."
     return 1
   fi
   return 0
@@ -1080,7 +1081,7 @@ open_console() {
       err "VM $id is not running. Start it first."
       return
     fi
-    note "Press Ctrl+] to exit the VM terminal."
+    note "If the terminal opens, use the escape hint shown by Proxmox to exit."
     if qm terminal "$id" 2>/dev/null; then
       :
     else
@@ -1292,18 +1293,46 @@ snapshots_menu() {
 # SPICE
 # =============================================================================
 
-spice_info() {
-  local id="$1" name="$2"
-  local host port
-  host="$(hostname -I 2>/dev/null | awk '{print $1}')"
-  port="$(qm monitor "$id" <<<"info spice" 2>/dev/null |
-    awk '/port/ {for(i=1;i<=NF;i++) if($i ~ /^[0-9]+$/){print $i; exit}}')"
-  [[ -z "$port" ]] && port="$(grep -E "(spice).*port" "/var/log/qemu-server/${id}.log" 2>/dev/null |
-    tail -1 | sed -n 's/.*port=\([0-9]\+\).*/\1/p' || true)"
-  local id_int
+_spice_has_gui_session() {
+  [[ -n "${DISPLAY:-}" ]] || [[ -n "${WAYLAND_DISPLAY:-}" && -n "${XDG_RUNTIME_DIR:-}" ]]
+}
+
+_spice_endpoint() {
+  local id="$1"
+  local monitor endpoint host='' port='' cfg_spice cfg_addr id_int
+
+  monitor="$(qm monitor "$id" <<<'info spice' 2>/dev/null || true)"
+  endpoint="$(sed -n 's/^[[:space:]]*address:[[:space:]]*\([^[:space:]]*\).*/\1/p' <<<"$monitor" | head -1)"
+  if [[ -n "$endpoint" ]]; then
+    if [[ "$endpoint" == \[*\]:* ]]; then
+      host="${endpoint%%]:*}"
+      host="${host#[}"
+      port="${endpoint##*:}"
+    elif [[ "$endpoint" == *:* ]]; then
+      host="${endpoint%:*}"
+      port="${endpoint##*:}"
+    fi
+  fi
+
+  cfg_spice="$(qm config "$id" 2>/dev/null | sed -n 's/^spice: .*port=\([0-9]\+\).*/\1/p' | head -1)"
+  cfg_addr="$(qm config "$id" 2>/dev/null | sed -n 's/^spice: .*addr=\([^,]*\).*/\1/p' | head -1)"
+  [[ -z "$port" && -n "$cfg_spice" ]] && port="$cfg_spice"
+  [[ -z "$host" && -n "$cfg_addr" ]] && host="$cfg_addr"
+
+  [[ -z "$host" ]] && host="${PROXMOX_MANAGER_SPICE_ADDR:-$(hostname -I 2>/dev/null | awk '{print $1}')}"
   id_int=$((10#$id))
   [[ -z "$port" ]] && port="$((61000 + id_int))"
 
+  printf '%s\t%s\n' "$host" "$port"
+}
+
+spice_info() {
+  local id="$1" name="$2"
+  local host port endpoint
+  endpoint="$(_spice_endpoint "$id")"
+  host="${endpoint%%$'\t'*}"
+  port="${endpoint##*$'\t'}"
+
   printf '  %bSPICE:%b spice://%s:%s\n' "${BOLD}${CYAN_BRIGHT}" "${NC}" "$host" "$port"
   umask 077
   local vv
@@ -1321,12 +1350,16 @@ title=VM ${id} (${name})
 delete-this-file=1
 fullscreen=0
 EOF
-  if have virt-viewer; then
+  if have virt-viewer && _spice_has_gui_session; then
     virt-viewer "$vv" &
     ok "Launching virt-viewer for VM ${id}..."
   else
     ok "SPICE connection file: ${vv}"
-    note "Install virt-viewer with: apt install virt-viewer"
+    if have virt-viewer; then
+      note "No graphical session detected in this shell. Open the .vv file from a desktop session."
+    else
+      note "Install virt-viewer with: apt install virt-viewer"
+    fi
   fi
 }
 

tests/mock-bin/hostname

@@ -0,0 +1,7 @@
+#!/usr/bin/env bash
+set -euo pipefail
+if [[ "${1:-}" == "-I" ]]; then
+  echo "192.168.178.222 10.0.0.1"
+else
+  echo "mock-host"
+fi

tests/mock-bin/qm

@@ -19,8 +19,29 @@ EOL
 EOL
     fi
     ;;
+  monitor)
+    cat <<'EOL'
+qm> info spice
+qm> Entering QEMU Monitor for VM 200 - type 'help' for help
+Server:
+     address: 127.0.0.1:61000 [tls]
+    migrated: false
+        auth: spice
+    compiled: 0.15.2
+  mouse-mode: client
+Channels: none
+EOL
+    ;;
   config)
     case "${1:-}" in
+      200)
+        cat <<'EOL'
+agent: enabled=1
+name: vm-one
+spice: port=61000,addr=127.0.0.1
+vga: qxl
+EOL
+        ;;
       201) echo "name: vm-fallback" ;;
       *) echo "name: vm-${1:-unknown}" ;;
     esac

tests/mock-bin/virt-viewer

@@ -0,0 +1,4 @@
+#!/usr/bin/env bash
+set -euo pipefail
+: "${HERMES_VIRT_VIEWER_MARKER:=/tmp/hermes-virt-viewer-called}"
+printf '%s\n' "$*" >> "$HERMES_VIRT_VIEWER_MARKER"

tests/run.sh

@@ -153,6 +153,8 @@ _snap_test "my-snap_2" 0 "with hyphen and underscore"
 _snap_test "$(printf 'a%.0s' {1..40})" 0 "exactly 40 chars (max valid)"
 
 # Invalid names — expect exit 1
+_snap_test "1"         1 "numeric-only name"
+_snap_test "a"         1 "single-letter name"
 _snap_test "_snap"     1 "starts with underscore"
 _snap_test "-bad"      1 "starts with hyphen"
 _snap_test "snap name" 1 "contains space"
@@ -178,6 +180,33 @@ else
   _fail "ip_info: CT did not return expected IPv4 address"
 fi
 
+# ---------------------------------------------------------------------------
+# Unit tests: spice_info()
+# ---------------------------------------------------------------------------
+rm -f /tmp/hermes-virt-viewer-called
+unset DISPLAY WAYLAND_DISPLAY XDG_RUNTIME_DIR
+_spice_exit=0
+_spice_out="$(spice_info 200 vm-one 2>&1)" || _spice_exit=$?
+_spice_vv="$(printf '%s\n' "$_spice_out" | sed -n 's/.*SPICE connection file: //p' | tail -1)"
+if [[ "$_spice_exit" == "0" ]] && printf '%s\n' "$_spice_out" | grep -q 'spice://127.0.0.1:61000'; then
+  _pass "spice_info: URI uses monitor/config host and port"
+else
+  _fail "spice_info: URI did not use expected host/port"
+fi
+
+if [[ -n "$_spice_vv" ]] && [[ -f "$_spice_vv" ]] && grep -q '^host=127.0.0.1$' "$_spice_vv"; then
+  _pass "spice_info: .vv file uses actual SPICE bind host"
+else
+  _fail "spice_info: .vv file did not use expected host"
+fi
+
+if [[ -n "$_spice_vv" ]]; then rm -f "$_spice_vv"; fi
+if [[ ! -e /tmp/hermes-virt-viewer-called ]]; then
+  _pass "spice_info: does not auto-launch virt-viewer without GUI session"
+else
+  _fail "spice_info: auto-launched virt-viewer without GUI session"
+fi
+
 # ---------------------------------------------------------------------------
 # Tests: --filter flag
 # ---------------------------------------------------------------------------