From ad61b6c19f0e1fc1418d9884b473102497cc6e59 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 7 May 2026 01:24:57 +0100 Subject: [PATCH] Require approval for backend alert investigation commands --- .../v6/internal/subsystems/agent-lifecycle.md | 5 ++ .../v6/internal/subsystems/ai-runtime.md | 7 ++ .../v6/internal/subsystems/api-contracts.md | 7 +- .../internal/subsystems/storage-recovery.md | 5 ++ internal/ai/alert_provider.go | 2 +- internal/ai/alert_provider_test.go | 6 ++ internal/ai/service.go | 72 +++++++++++++------ internal/ai/service_extended_test.go | 41 +++++++++++ internal/api/ai_handlers.go | 9 ++- internal/api/ai_handlers_test.go | 12 ++++ internal/api/contract_test.go | 14 ++++ 11 files changed, 152 insertions(+), 28 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index f752b864d..9b22e7aed 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -1094,6 +1094,11 @@ AI handlers add split scoped-trigger fields, recency labels, or trigger-state transport for Patrol, lifecycle-adjacent setup and fleet surfaces must treat those payloads as Patrol-only runtime context and must not reinterpret them as agent install readiness, enrollment health, or fleet-control state. +That same shared AI handler dependency also assumes direct alert-investigation +execution mode is AI/API-owned. Request-scoped `AutonomousMode:false` and +`RequireCommandApproval:true` on `/api/ai/investigate-alert` are Assistant +action-governance facts, not agent install readiness, command reachability, or +fleet-control capability signals. That same shared `internal/api/` dependency also now assumes SSO test and metadata-preview routes fail closed on validated outbound URL handling. Lifecycle-adjacent setup and hosted bootstrap surfaces may depend on those diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 6d96f8e49..a1ae311ce 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -1055,6 +1055,13 @@ only governed prompt/context data, but the submitted chat request must set `autonomous_mode:false`, preserve the operator's persistent Assistant control-level setting, and disclose the temporary approval-required mode in the drawer instead of showing the generic Autonomous warning. +Direct alert-investigation runtime handoffs follow the same rule even when +they bypass the chat drawer. `/api/ai/investigate-alert` must set +`ai.ExecuteRequest.AutonomousMode` to false plus +`ai.ExecuteRequest.RequireCommandApproval` to true, and +`internal/ai/alert_provider.go` must frame diagnostics as approval-bound +operator actions rather than instructing the model to execute commands because +they appear safe. Those backend AI and Patrol change summaries should derive their canonical labels and provenance fragments from `internal/unifiedresources/change_presentation.go`, so the resource-model diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index db3642152..e4b659ebf 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -327,7 +327,12 @@ the canonical monitored-system blocked payload. surfaces. Frontend finding-discussion handoffs that carry any live approval, proposed-fix, fix outcome, or remediation-plan reference must force a request-local approval-required Assistant mode instead of inheriting the - user's persistent autonomous control setting. The operator + user's persistent autonomous control setting. Direct alert-investigation API + handoffs through `internal/api/ai_handlers.go` must enforce that same + request-scoped boundary by setting `ai.ExecuteRequest.AutonomousMode` to + false and `ai.ExecuteRequest.RequireCommandApproval` to true; API proof must + keep this guarded in both `internal/api/ai_handlers_test.go` and + `internal/api/contract_test.go`. The operator decision and action-posture lines in the briefing must derive from those same structured action references after recovery so the briefing cannot contradict the handoff action payload. Related finding diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 55e45f916..481ff66f4 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -393,6 +393,11 @@ bypass the API fail-closed execution gate. 19. Preserve shipped local security-doc guidance in shared `internal/api/` config/setup helpers so storage- and recovery-adjacent transport surfaces do not reintroduce GitHub `main` security links when the running build already serves its own local security documentation route. 20. Keep shared `internal/api/` Patrol transport and alert-trigger edits feature-isolated: Patrol-specific recency fields, callback fan-out, or alert-bridge wiring changes must not leak into recovery queries, storage links, or recovery-adjacent install/setup flows unless this contract changes in the same slice. The same adjacency rule applies to AI settings transport in `internal/api/ai_handlers.go`: provider auth state, masked-secret payload fields, and provider-test model selection remain AI/runtime plus API-contract concerns and must not be absorbed into storage/recovery transport ownership just because those handlers live under the shared backend API tree. + Direct alert-investigation execution controls in `internal/api/ai_handlers.go` + follow that same split: request-scoped `AutonomousMode:false` and + `RequireCommandApproval:true` are AI action-governance constraints, not + storage/recovery restore approval, recovery freshness, or storage diagnostic + payload semantics. That same adjacent boundary also keeps the retired Patrol quickstart contract out of storage/recovery ownership: shared AI handlers no longer expose active quickstart credit, token, or hosted-model provider state, and diff --git a/internal/ai/alert_provider.go b/internal/ai/alert_provider.go index 2c14500c4..6c36e68ec 100644 --- a/internal/ai/alert_provider.go +++ b/internal/ai/alert_provider.go @@ -300,7 +300,7 @@ func GenerateAlertInvestigationPrompt(req AlertInvestigationRequest) string { prompt.WriteString("1. Identify the root cause of this alert\n") prompt.WriteString("2. Check related metrics and system state\n") prompt.WriteString("3. Suggest specific remediation steps\n") - prompt.WriteString("4. If safe, execute diagnostic commands to gather more info\n") + prompt.WriteString("4. Ask for operator approval before running any diagnostic command or change\n") return prompt.String() } diff --git a/internal/ai/alert_provider_test.go b/internal/ai/alert_provider_test.go index 3f32ce3e3..c223df306 100644 --- a/internal/ai/alert_provider_test.go +++ b/internal/ai/alert_provider_test.go @@ -229,6 +229,12 @@ func TestGenerateAlertInvestigationPrompt(t *testing.T) { !strings.Contains(out, "**Node:** pve1") { t.Fatalf("unexpected prompt: %s", out) } + if !strings.Contains(out, "Ask for operator approval before running any diagnostic command") { + t.Fatalf("expected approval-bound diagnostic guidance, got: %s", out) + } + if strings.Contains(out, "execute diagnostic commands") { + t.Fatalf("prompt must not instruct autonomous diagnostic command execution: %s", out) + } } func TestAlertInvestigationRequestJSONCanonicalOutput(t *testing.T) { diff --git a/internal/ai/service.go b/internal/ai/service.go index 7d1eced55..79b6b02fe 100644 --- a/internal/ai/service.go +++ b/internal/ai/service.go @@ -1838,6 +1838,13 @@ func (s *Service) IsAutonomous() bool { return true } +func (s *Service) isAutonomousForRequest(req ExecuteRequest) bool { + if req.AutonomousMode != nil && !*req.AutonomousMode { + return false + } + return s.IsAutonomous() +} + // ConversationMessage represents a message in conversation history type ConversationMessage struct { Role string `json:"role"` // "user" or "assistant" @@ -1846,15 +1853,17 @@ type ConversationMessage struct { // ExecuteRequest represents a request to execute an AI prompt type ExecuteRequest struct { - Prompt string `json:"prompt"` - TargetType string `json:"target_type,omitempty"` // "agent", "system-container", "vm" - TargetID string `json:"target_id,omitempty"` - Context map[string]interface{} `json:"context,omitempty"` // Current metrics, state, etc. - SystemPrompt string `json:"system_prompt,omitempty"` // Override system prompt - History []ConversationMessage `json:"history,omitempty"` // Previous conversation messages - FindingID string `json:"finding_id,omitempty"` // If fixing a patrol finding, the ID to resolve - Model string `json:"model,omitempty"` // Override model for this request (for user selection in chat) - UseCase string `json:"use_case,omitempty"` // "chat" or "patrol" - determines which default model to use + Prompt string `json:"prompt"` + TargetType string `json:"target_type,omitempty"` // "agent", "system-container", "vm" + TargetID string `json:"target_id,omitempty"` + Context map[string]interface{} `json:"context,omitempty"` // Current metrics, state, etc. + SystemPrompt string `json:"system_prompt,omitempty"` // Override system prompt + History []ConversationMessage `json:"history,omitempty"` // Previous conversation messages + FindingID string `json:"finding_id,omitempty"` // If fixing a patrol finding, the ID to resolve + Model string `json:"model,omitempty"` // Override model for this request (for user selection in chat) + UseCase string `json:"use_case,omitempty"` // "chat" or "patrol" - determines which default model to use + AutonomousMode *bool `json:"autonomous_mode,omitempty"` // Per-request execution override; false keeps scoped handoffs approval-required + RequireCommandApproval bool `json:"require_command_approval,omitempty"` // Force every run_command tool call through operator approval } // ExecuteResponse represents the AI's response @@ -2386,6 +2395,7 @@ Always execute the commands rather than telling the user how to do it.` // Check if this command needs approval needsApproval := false approvalID := "" + approvalReason := "Command requires user approval" if tc.Name == "run_command" { cmd, _ := tc.Input["command"].(string) runOnHost, _ := tc.Input["run_on_host"].(bool) @@ -2406,7 +2416,7 @@ Always execute the commands rather than telling the user how to do it.` } } - isAuto := s.IsAutonomous() + isAuto := s.isAutonomousForRequest(req) policyDecision := s.policy.Evaluate(cmd) log.Debug(). Bool("autonomous", isAuto). @@ -2446,10 +2456,13 @@ Always execute the commands rather than telling the user how to do it.` continue } - // If policy requires approval and we're not in autonomous mode, request approval. - if !isAuto && policyDecision == agentexec.PolicyRequireApproval { + if req.RequireCommandApproval || (!isAuto && policyDecision == agentexec.PolicyRequireApproval) { needsApproval = true anyNeedsApproval = true + approvalReason = "Security policy requires approval" + if req.RequireCommandApproval { + approvalReason = "This handoff requires operator approval before command execution" + } approvalID = createRunCommandApprovalRecord( approvalOrgID, cmd, @@ -2457,7 +2470,7 @@ Always execute the commands rather than telling the user how to do it.` req, runOnHost, targetHost, - "Security policy requires approval", + approvalReason, ) callback(StreamEvent{ Type: "approval_needed", @@ -2482,7 +2495,7 @@ Always execute the commands rather than telling the user how to do it.` // Note: We don't add to toolExecutions here because the approval_needed event // already tells the frontend to show the approval UI cmd, _ := tc.Input["command"].(string) - result = formatApprovalNeededToolResult(cmd, tc.ID, "Command requires user approval", approvalID) + result = formatApprovalNeededToolResult(cmd, tc.ID, approvalReason, approvalID) execution = ToolExecution{ Name: tc.Name, Input: toolInput, @@ -3106,10 +3119,14 @@ func (s *Service) executeTool(ctx context.Context, req ExecuteRequest, tc provid execution.Output = formatPolicyBlockedToolResult(command, "This command is blocked by security policy") return execution.Output, execution } - if decision == agentexec.PolicyRequireApproval && !s.IsAutonomous() { + if req.RequireCommandApproval || (decision == agentexec.PolicyRequireApproval && !s.isAutonomousForRequest(req)) { s.mu.RLock() approvalOrgID := s.orgID s.mu.RUnlock() + approvalReason := "Security policy requires approval" + if req.RequireCommandApproval { + approvalReason = "This handoff requires operator approval before command execution" + } approvalID := createRunCommandApprovalRecord( approvalOrgID, command, @@ -3117,9 +3134,9 @@ func (s *Service) executeTool(ctx context.Context, req ExecuteRequest, tc provid req, runOnHost, targetHost, - "Security policy requires approval", + approvalReason, ) - execution.Output = formatApprovalNeededToolResult(command, tc.ID, "Security policy requires approval", approvalID) + execution.Output = formatApprovalNeededToolResult(command, tc.ID, approvalReason, approvalID) execution.Success = true // Not an error, just needs approval return execution.Output, execution } @@ -3992,14 +4009,23 @@ func (s *Service) RunCommand(ctx context.Context, req RunCommandRequest) (*RunCo // buildSystemPrompt creates the system prompt based on the request context func (s *Service) buildSystemPrompt(req ExecuteRequest, destinationModel string) string { - prompt := `You are Pulse's diagnostic assistant for Proxmox, Docker, and Kubernetes homelabs. - -## Command Approval -Pulse has a built-in approval system: + commandApproval := `Pulse has a built-in approval system: - Safe commands (read-only: ls, df, cat, ps) execute immediately - Destructive commands (rm, service restart, apt install) require user approval - Blocked commands are never executed -Execute commands normally - Pulse handles the approval flow automatically. +Execute commands normally - Pulse handles the approval flow automatically.` + if req.RequireCommandApproval { + commandApproval = `This handoff is approval-bound: +- Do not run commands just because they look safe. +- Any run_command tool call will pause for explicit operator approval before execution. +- Prefer existing metrics, alert context, and known system state before requesting command approval. +- Blocked commands are never executed.` + } + + prompt := fmt.Sprintf(`You are Pulse's diagnostic assistant for Proxmox, Docker, and Kubernetes homelabs. + +## Command Approval +%s ## Command Execution - run_on_host=true: Run on PVE/Docker host (pct, qm, vzdump, docker commands) @@ -4030,7 +4056,7 @@ Example: "homelab:pve-node:201" for VMID 201 on node pve-node in instance homela ## Installing/Updating Pulse curl -sSL https://raw.githubusercontent.com/rcourtman/Pulse/main/install.sh | bash After: systemctl enable pulse && systemctl start pulse -Latest version: https://api.github.com/repos/rcourtman/Pulse/releases/latest` +Latest version: https://api.github.com/repos/rcourtman/Pulse/releases/latest`, commandApproval) // Add custom context from AI settings (user's infrastructure description) s.mu.RLock() diff --git a/internal/ai/service_extended_test.go b/internal/ai/service_extended_test.go index a9dd1ef27..2832dac29 100644 --- a/internal/ai/service_extended_test.go +++ b/internal/ai/service_extended_test.go @@ -735,6 +735,32 @@ func TestService_ExecuteTool_RunCommand(t *testing.T) { if !strings.Contains(result, "APPROVAL_REQUIRED") { t.Errorf("Expected APPROVAL_REQUIRED message, got %s", result) } + + // 4. Request-scoped approval mode must clamp an autonomous default. + autonomousMode := false + svc.cfg = &config.AIConfig{ControlLevel: config.ControlLevelAutonomous} + req.AutonomousMode = &autonomousMode + result, exec = svc.executeTool(ctx, req, tc) + if !exec.Success { + t.Errorf("Expected success (not an error to need approval), got: %s", result) + } + if !strings.Contains(result, "APPROVAL_REQUIRED") { + t.Errorf("Expected request-scoped approval mode to require approval, got %s", result) + } + + // 5. Approval-bound handoffs require approval even for policy-allowed commands. + mockPolicy.decision = agentexec.PolicyAllow + req.RequireCommandApproval = true + result, exec = svc.executeTool(ctx, req, tc) + if !exec.Success { + t.Errorf("Expected success (not an error to need approval), got: %s", result) + } + if !strings.Contains(result, "APPROVAL_REQUIRED") { + t.Errorf("Expected approval-bound handoff to require approval, got %s", result) + } + if !strings.Contains(result, "handoff requires operator approval") { + t.Errorf("Expected handoff approval reason, got %s", result) + } } func TestService_ExecuteTool_SetResourceURL(t *testing.T) { @@ -2929,6 +2955,21 @@ func TestService_FindingContextInSystemPrompt(t *testing.T) { } } +func TestService_BuildSystemPrompt_ApprovalBoundCommandHandoff(t *testing.T) { + svc := NewService(nil, nil) + prompt := svc.buildSystemPrompt(ExecuteRequest{RequireCommandApproval: true}, "") + + if !strings.Contains(prompt, "This handoff is approval-bound") { + t.Fatalf("expected approval-bound command guidance, got: %s", prompt) + } + if !strings.Contains(prompt, "Any run_command tool call will pause for explicit operator approval") { + t.Fatalf("expected all command tool calls to require approval, got: %s", prompt) + } + if strings.Contains(prompt, "Safe commands (read-only: ls, df, cat, ps) execute immediately") { + t.Fatalf("approval-bound prompt must not say safe commands execute immediately: %s", prompt) + } +} + // TestService_FindingContextCommandRouting verifies that commands route correctly // when ExecuteStream is called with a FindingID. This is an end-to-end test. func TestService_FindingContextCommandRouting(t *testing.T) { diff --git a/internal/api/ai_handlers.go b/internal/api/ai_handlers.go index 17f5b66f7..2c18120d1 100644 --- a/internal/api/ai_handlers.go +++ b/internal/api/ai_handlers.go @@ -4300,10 +4300,13 @@ func (h *AISettingsHandler) HandleInvestigateAlert(w http.ResponseWriter, r *htt } }() + autonomousMode := false resp, err := h.GetAIService(r.Context()).ExecuteStream(ctx, ai.ExecuteRequest{ - Prompt: investigationPrompt, - TargetType: targetType, - TargetID: targetID, + Prompt: investigationPrompt, + TargetType: targetType, + TargetID: targetID, + AutonomousMode: &autonomousMode, + RequireCommandApproval: true, Context: map[string]interface{}{ "alertIdentifier": alertIdentifier, "alertType": req.AlertType, diff --git a/internal/api/ai_handlers_test.go b/internal/api/ai_handlers_test.go index f33bb65d6..0cf8633b9 100644 --- a/internal/api/ai_handlers_test.go +++ b/internal/api/ai_handlers_test.go @@ -2048,6 +2048,18 @@ func TestHandleInvestigateAlert_AcceptsCanonicalAlertIdentifier(t *testing.T) { require.NotContains(t, rec.Header().Get("Content-Type"), "text/event-stream") } +func TestHandleInvestigateAlert_ForcesApprovalBoundExecuteRequest(t *testing.T) { + t.Parallel() + + source, err := os.ReadFile("ai_handlers.go") + require.NoError(t, err) + text := string(source) + + require.Contains(t, text, "autonomousMode := false") + require.Contains(t, text, "AutonomousMode: &autonomousMode") + require.Contains(t, text, "RequireCommandApproval: true") +} + // ======================================== // AISettingsHandler setter method tests // ======================================== diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index c8c6d944f..dffa2ff11 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -295,6 +295,20 @@ func TestContract_AssistantFindingContextUsesModelOnlyHandoff(t *testing.T) { } } +func TestContract_InvestigateAlertIsApprovalBound(t *testing.T) { + source, err := os.ReadFile(filepath.Clean("ai_handlers.go")) + if err != nil { + t.Fatalf("read ai_handlers.go: %v", err) + } + text := string(source) + + if !strings.Contains(text, "autonomousMode := false") || + !strings.Contains(text, "AutonomousMode: &autonomousMode") || + !strings.Contains(text, "RequireCommandApproval: true") { + t.Fatal("investigate-alert API handoff must force request-scoped approval and command approval") + } +} + func TestContract_ProxmoxSetupScriptUsesPrivilegeSeparatedTokenACLs(t *testing.T) { storagePerms := ` pveum aclmod /storage -user pulse-monitor@pve -role PVEDatastoreAdmin