From f99bce7ee4ca74f0f2fb98c0208da86632e06214 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 9 May 2026 13:12:52 +0100 Subject: [PATCH] Author Proxmox VM/CT lifecycle preflight context for approval review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pulse's primary monitored platform is Proxmox, but the per-command-class preflight catalog only covered systemd services, Docker containers, and Kubernetes deployments. Operators approving a Patrol-proposed qm restart/qm stop/qm shutdown saw the generic preflight without the operational nuance that distinguishes those verbs (qm stop is a hard halt, qm shutdown is graceful with a 60s ACPI timeout, etc.). Adds eight new classes covering qm and pct lifecycle (reboot/restart, stop, start, shutdown). Each gets hand-authored safety and verification copy that names the actual semantics — pct stop is destructive vs pct shutdown's lxc-attach handoff — so the operator sees concrete context at approval time. Broker-level VerificationCommandForCommand intentionally does not derive qm status / pct status from these classes — pulse_control's verifyGuestAction already runs those checks at the tool layer, so adding a parallel broker dispatch would double-run. The preflight copy still names what the tool-layer verification will read. --- .../v6/internal/subsystems/ai-runtime.md | 18 ++- internal/ai/tools/approval_preflight_test.go | 37 ++++++ internal/ai/tools/tools_control.go | 106 +++++++++++++++++- 3 files changed, 150 insertions(+), 11 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 875232f1b..5495014ae 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -321,15 +321,23 @@ runtime cost control, and shared AI transport surfaces. `approvalCommandClassPreflightAdditions` in `internal/ai/tools/tools_control.go` bucket common Pulse remediation actions (service-restart, service-stop, service-start, service-reload, - container-restart, container-stop, k8s-rollout-restart) and return - hand-authored operational copy: what the command actually touches, - how Pulse will read back success. The additions append onto the - default safety/verification arrays rather than replacing them, so - the broker's structural posture (org scope, hash match, single-use + container-restart, container-stop, k8s-rollout-restart, plus the + Proxmox VM lifecycle classes proxmox-vm-reboot, proxmox-vm-stop, + proxmox-vm-start, proxmox-vm-shutdown and the matching pct-driven + proxmox-ct-* container lifecycle classes) and return hand-authored + operational copy: what the command actually touches, how Pulse will + read back success. The additions append onto the default + safety/verification arrays rather than replacing them, so the + broker's structural posture (org scope, hash match, single-use approval) remains visible alongside the class-specific copy. Unknown command classes must return empty additions rather than fabricated padding — operators see only the default content, not invented assertions about what an unrecognized command will do. + The Proxmox classes intentionally do not derive a broker-level + `VerificationCommandForCommand` check because pulse_control's + `verifyGuestAction` already runs `qm status` / `pct status` at the + tool layer; adding a parallel broker dispatch would double-run the + same read-after-write check. Drift refusal must also persist a Failed audit record with the Request, Plan, and Approvals snapshots intact and a Result whose ErrorMessage is prefixed `plan_drift:` so the audit trail shows diff --git a/internal/ai/tools/approval_preflight_test.go b/internal/ai/tools/approval_preflight_test.go index dc5e71ac1..29274f54e 100644 --- a/internal/ai/tools/approval_preflight_test.go +++ b/internal/ai/tools/approval_preflight_test.go @@ -24,6 +24,23 @@ func TestClassifyApprovalCommand_BucketsKnownCommandShapes(t *testing.T) { {"podman restart", "agent", "podman restart pihole", "container-restart"}, {"docker stop", "docker", "docker stop oldcontainer", "container-stop"}, {"kubectl rollout restart", "kubernetes", "kubectl rollout restart deployment/api", "k8s-rollout-restart"}, + // Proxmox VM lifecycle classes — Pulse's primary monitored platform. + // Each verb has distinct semantics so they map to distinct classes. + {"qm reboot", "agent", "qm reboot 101", "proxmox-vm-reboot"}, + {"qm restart alias", "agent", "qm restart 101", "proxmox-vm-reboot"}, + {"qm stop hard halt", "agent", "qm stop 101", "proxmox-vm-stop"}, + {"qm start", "agent", "qm start 101", "proxmox-vm-start"}, + {"qm shutdown graceful", "agent", "qm shutdown 101", "proxmox-vm-shutdown"}, + // Proxmox LXC container lifecycle classes. + {"pct reboot", "agent", "pct reboot 200", "proxmox-ct-reboot"}, + {"pct restart alias", "agent", "pct restart 200", "proxmox-ct-reboot"}, + {"pct stop hard halt", "agent", "pct stop 200", "proxmox-ct-stop"}, + {"pct start", "agent", "pct start 200", "proxmox-ct-start"}, + {"pct shutdown graceful", "agent", "pct shutdown 200", "proxmox-ct-shutdown"}, + // Negative cases: tool prefix without a recognized verb must NOT + // classify (defends against `qm migrate`, `pct destroy`, etc.). + {"qm migrate not a lifecycle verb", "agent", "qm migrate 101 pve-2", ""}, + {"pct destroy not a lifecycle verb", "agent", "pct destroy 200", ""}, {"unknown free-form command", "agent", "echo hello world", ""}, {"empty command", "agent", "", ""}, } @@ -48,6 +65,15 @@ func TestApprovalCommandClassPreflightAdditions_AuthorsConcreteContextForKnownCl {"service-stop warns dependent services", "systemctl stop postgres", "dependent services", "inactive"}, {"container-restart names docker inspect", "docker restart homepage", "briefly unavailable", "docker inspect"}, {"k8s rollout names rollout status", "kubectl rollout restart deployment/api", "PodDisruptionBudget", "rollout status"}, + // Proxmox classes must concretely warn about the destructive vs + // graceful split (qm stop = hard halt, qm shutdown = graceful) and + // name the canonical `qm status` / `pct status` read-after-write + // check the operator should see post-dispatch. + {"qm reboot names qm status", "qm reboot 101", "ACPI shutdown", "qm status"}, + {"qm stop warns it is hard not graceful", "qm stop 101", "hard stop", "stopped"}, + {"qm shutdown names timeout fallback", "qm shutdown 101", "ACPI shutdown", "qm status"}, + {"pct stop warns it is hard not graceful", "pct stop 200", "hard stop", "stopped"}, + {"pct shutdown names lxc-attach", "pct shutdown 200", "lxc-attach", "pct status"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -92,6 +118,17 @@ func TestVerificationCommandForCommand_DerivesPerClassReadAfterWriteCheck(t *tes // the tool layer, so adding a broker-level dispatch would double-run. {"docker restart deferred to tool layer", "docker", "docker restart homepage", "", false}, {"podman restart deferred to tool layer", "agent", "podman restart pihole", "", false}, + // Proxmox classes are intentionally excluded from broker-level + // verification — pulse_control's verifyGuestAction already runs + // `qm status` and `pct status` at the tool layer, so adding a + // parallel broker-level dispatch would double-run the same check. + // The preflight copy still names the verification narrative; only + // the broker-side derivation is suppressed. + {"qm reboot deferred to pulse_control", "agent", "qm reboot 101", "", false}, + {"qm stop deferred to pulse_control", "agent", "qm stop 101", "", false}, + {"qm shutdown deferred to pulse_control", "agent", "qm shutdown 101", "", false}, + {"pct stop deferred to pulse_control", "agent", "pct stop 200", "", false}, + {"pct shutdown deferred to pulse_control", "agent", "pct shutdown 200", "", false}, {"unknown command", "agent", "echo hello", "", false}, {"systemctl with single-quote in unit", "agent", `systemctl restart nasty'name`, "systemctl is-active 'nasty'\\''name'", true}, } diff --git a/internal/ai/tools/tools_control.go b/internal/ai/tools/tools_control.go index fd52f96d5..8a4c6bc02 100644 --- a/internal/ai/tools/tools_control.go +++ b/internal/ai/tools/tools_control.go @@ -1525,12 +1525,19 @@ func approvalPreflight(req *approval.ApprovalRequest) *approval.ActionPreflight // classes without a derivable check; the broker must skip verification // rather than fabricate one. // -// Container classes (container-restart, container-stop) are intentionally -// excluded here: pulse_docker already runs its own per-container -// `docker inspect` verification at the tool layer, and adding a parallel -// broker-level dispatch would double-run the same check. If the tool layer -// stops doing its own verification, this function should grow a docker -// branch using `extractContainerName`. +// Tool-layer-verified classes are intentionally excluded here: +// - Container classes (container-restart, container-stop) are verified +// by pulse_docker via per-container `docker inspect` at the tool +// layer. +// - Proxmox VM/CT classes (proxmox-vm-*, proxmox-ct-*) are verified by +// pulse_control via `verifyGuestAction` (which dispatches `qm status` +// or `pct status` already) at the tool layer. +// +// Adding a parallel broker-level dispatch for these would double-run the +// same check. The preflight copy authored by +// approvalCommandClassPreflightAdditions still names what the +// tool-layer verification will read so the operator-facing narrative +// stays accurate. func VerificationCommandForCommand(targetType, command string) (string, bool) { class := classifyApprovalCommand(targetType, command) switch class { @@ -1627,6 +1634,29 @@ func classifyApprovalCommand(targetType, command string) string { return "container-stop" case strings.HasPrefix(cmd, "kubectl rollout restart"): return "k8s-rollout-restart" + // Proxmox VM lifecycle (qm) — Pulse's primary monitored platform. Each + // verb has distinct operational semantics (graceful shutdown vs hard + // stop vs reboot) so they map to distinct classes rather than being + // folded into the generic service-* buckets. + case strings.HasPrefix(cmd, "qm reboot ") || strings.HasPrefix(cmd, "qm restart "): + return "proxmox-vm-reboot" + case strings.HasPrefix(cmd, "qm stop "): + return "proxmox-vm-stop" + case strings.HasPrefix(cmd, "qm start "): + return "proxmox-vm-start" + case strings.HasPrefix(cmd, "qm shutdown "): + return "proxmox-vm-shutdown" + // Proxmox LXC container lifecycle (pct). Same per-verb split as qm + // because pct shutdown initiates an in-guest shutdown via lxc-attach + // while pct stop is an immediate halt. + case strings.HasPrefix(cmd, "pct reboot ") || strings.HasPrefix(cmd, "pct restart "): + return "proxmox-ct-reboot" + case strings.HasPrefix(cmd, "pct stop "): + return "proxmox-ct-stop" + case strings.HasPrefix(cmd, "pct start "): + return "proxmox-ct-start" + case strings.HasPrefix(cmd, "pct shutdown "): + return "proxmox-ct-shutdown" } return "" } @@ -1695,6 +1725,70 @@ func approvalCommandClassPreflightAdditions(targetType, command string) (safetyC "Watch `kubectl rollout status` until the deployment converges.", "Verify pod readiness with `kubectl get pods -l ` after rollout.", } + case "proxmox-vm-reboot": + return []string{ + "`qm reboot` performs an ACPI shutdown followed by a start; in-guest workloads see a clean OS reboot.", + "VM RAM state is not preserved (no live migration); guests with non-persistent state will lose it.", + }, []string{ + "Read back `qm status ` and confirm `status: running`.", + "Verify the guest's monitoring agent reconnects (Pulse will detect and update the resource state).", + } + case "proxmox-vm-stop": + return []string{ + "`qm stop` is an immediate hard stop, NOT a graceful shutdown — guest filesystems may be left dirty.", + "Use `qm shutdown` instead if the workload needs to flush state cleanly; only approve `qm stop` when the guest is unresponsive.", + }, []string{ + "Read back `qm status ` and confirm `status: stopped`.", + "Verify no auto-start policy will immediately restart the VM.", + } + case "proxmox-vm-start": + return []string{ + "VM will boot from disk with the configuration currently on the host; uncommitted config drift will take effect.", + "Boot order, attached disks, and network bridges all apply as currently defined — verify before approving.", + }, []string{ + "Read back `qm status ` and confirm `status: running`.", + "Tail `journalctl -u qmeventd` for the start lifecycle event.", + } + case "proxmox-vm-shutdown": + return []string{ + "`qm shutdown` issues an ACPI shutdown to the guest and waits for the VM to power off cleanly.", + "Default timeout is 60s; an unresponsive guest will force a stop after the timeout, leaving filesystems in the same state as `qm stop`.", + }, []string{ + "Read back `qm status ` and confirm `status: stopped`.", + "Verify the shutdown was clean (exit code 0) rather than a forced stop after timeout.", + } + case "proxmox-ct-reboot": + return []string{ + "`pct reboot` performs an in-guest reboot; container processes restart while the LXC instance stays managed by Proxmox.", + "Mounted bind-mounts and shared volumes persist through the reboot; only in-memory state is lost.", + }, []string{ + "Read back `pct status ` and confirm `status: running`.", + "Verify the container's foreground service has come back up before treating the reboot as complete.", + } + case "proxmox-ct-stop": + return []string{ + "`pct stop` is an immediate hard stop — in-guest processes do not get a chance to flush state cleanly.", + "Use `pct shutdown` instead for graceful in-guest shutdown; reserve `pct stop` for unresponsive containers.", + }, []string{ + "Read back `pct status ` and confirm `status: stopped`.", + "Verify no on-boot auto-start policy will immediately restart the container.", + } + case "proxmox-ct-start": + return []string{ + "Container will start with the configuration currently on the host; uncommitted config drift takes effect.", + "Mounted volumes, network bridges, and resource limits all apply as currently defined.", + }, []string{ + "Read back `pct status ` and confirm `status: running`.", + "Verify the container's foreground service is reachable from the host network.", + } + case "proxmox-ct-shutdown": + return []string{ + "`pct shutdown` issues a graceful in-guest shutdown via lxc-attach and waits for the container to halt.", + "Default timeout falls back to a hard stop on unresponsive containers, leaving processes in the same state as `pct stop`.", + }, []string{ + "Read back `pct status ` and confirm `status: stopped`.", + "Verify the shutdown was clean rather than a forced stop after timeout.", + } } return nil, nil }