diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 132852209..3058e60ac 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -7796,3 +7796,78 @@ Menu-opening controls remain buttons, and the mobile bar continues to own `--pulse-mobile-nav-height`; Assistant overlay containment and action approval boundaries are unchanged. Mobile navigation and AppLayout tests pin the route and focus behavior. + +### Advertised lifecycle actions are submitted, never narrated + +When an operator asks the Assistant to perform a lifecycle action (start, +stop, shutdown, reboot/restart) and a canonical resource the session has +resolved advertises that capability, the Assistant submits `pulse_control` +for each target and lets the shared action lifecycle decide availability +through planning, approval, execution, and verification. It may report a +limitation only from a tool result in the current turn; an assumed +prerequisite (QEMU guest agent, "discovery binding", "session state") or a +manual `qm`/`pct` instruction for an action Pulse offers is a contract +violation (GitHub issue #1782). Three structural guarantees enforce this: + +- `pulse_control` binds its `resource_id` to the canonical unified resource. + Session context is consulted first; a reference absent from the session + that resolves uniquely in the unified inventory is registered and planned, + an ambiguous name is refused with the candidate canonical ids, and a + lookup miss names the exact `pulse_query` recovery call. The plan request + carries the canonical unified id (never the session-scoped + `kind:host:uid`), and the legacy per-executor action list is not a gate: + whether the action exists is the action lifecycle's decision from the + resource's advertised capabilities (`internal/ai/tools/control_targets.go`). + A capability the resource does not advertise comes back as + `ACTION_NOT_ALLOWED` tool evidence listing the currently advertised + capabilities. +- Recoverable ordering blocks (the RESOLVING FSM state, a strict-resolution + miss) name the read-only step to take first and state that they are not a + limitation to report; the shared operating instructions say the same and + require the governed action tool for advertised capabilities. +- The agentic loop's advertised-action gate refuses, once per run, a + tool-free final answer when the operator's message requests a lifecycle + action, `pulse_control` was offered, no `pulse_control` call reached + execution, and at least one session-resolved resource currently advertises + the action. The refusal is a provider-only user-role correction naming the + exact calls per target; it fails open on the next prose answer so a model + with tool-evidenced reasons not to act is never livelocked + (`internal/ai/chat/agentic_action_gate.go`). + +Proofs: `internal/ai/tools/control_targets_test.go`, +`internal/ai/chat/agentic_action_gate_test.go` (the #1782 transcript against +a scripted provider), `internal/agentcapabilities/governance_prompt_test.go`, +and the live eval `ProxmoxBulkLifecycleActionScenario` in +`internal/ai/eval/scenarios.go`. + +### Alert-mirroring findings fold under the alert; flapping collapses to one row + +Real-time alerts own down, threshold, and age conditions, but Patrol's +deterministic watchers and the model can still emit a finding for the same +resource and condition. `internal/ai/findings_alert_mirror.go` is the single +matcher: a finding mirrors an alert when it carries the alert's identifier or +when both name the same canonical resource and the same condition class +(down, restart loop, container health, disk capacity, memory, CPU, backup age, +snapshot age, temperature). Unknown alert types and free-form findings never +match. `PatrolService.reconcileAlertMirrors` runs at the end of every real and +demo patrol cycle against the unscoped active-alert snapshot and stamps +`mirrors_alert_id` and `mirrors_alert_type` on unresolved findings through +`FindingsStore.StampAlertMirrors`, clearing the stamp once the alert resolves. +The stamp is a derived read-model annotation: it is not persisted and is not a +reason to drop the finding. Surfaces demote stamped findings under the alert +instead of listing the same problem twice. + +Flapping is owned by the existing storm throttler rather than a parallel +mechanism. `FindingsStore.recordTransitionLifecycleLocked` routes every +regressed, resolved, and auto_resolved transition through +`findingStormThrottler.observeFlapLocked`, which keeps a 24-hour sliding window +per finding (hydrated from the persisted lifecycle on first sight so a restart +does not forget). At four or more transitions the finding carries `flapping` +(count, window, first and latest transition) and the store maintains one +`flapping` lifecycle row whose metadata carries the count, updating it in place +instead of appending a row per transition; below the threshold the ordinary +rows return and the label clears. Regression counters keep counting while +collapsed. `internal/ai/findings_flapping_test.go`, +`internal/ai/findings_storm_throttler_test.go`, and +`internal/ai/findings_alert_mirror_test.go` pin the threshold, the collapse, +the hydration, and the matcher. diff --git a/internal/ai/chat/agentic.go b/internal/ai/chat/agentic.go index 2e60cb7d7..b95ce69a4 100644 --- a/internal/ai/chat/agentic.go +++ b/internal/ai/chat/agentic.go @@ -972,6 +972,12 @@ func (a *AgenticLoop) executeWithTools(ctx context.Context, sessionID string, me lookGateToolAttempted := false lookGateBlocks := 0 + // Advertised-action gate state; see maxAdvertisedActionGateBlocks. Only a + // pulse_control call that reached execution counts: a call the FSM refused + // for ordering has not been submitted yet. + controlToolExecutedThisRun := false + advertisedActionGateBlocks := 0 + // Track where each turn's messages begin in providerMessages for compaction. // We keep the last N turns' tool results in full; older ones get compacted. const compactionKeepTurns = 2 // Keep last 2 turns' tool results in full (KA preserves key facts) @@ -1778,6 +1784,41 @@ agenticLoop: } } + // === ADVERTISED-ACTION GATE: an action request ends in pulse_control, not prose === + // The field failure this pins: the operator asks to reboot N guests, + // the model resolves them, then writes a report with "next steps" + // and an invented prerequisite instead of submitting the governed + // action. When the resolved targets advertise the requested + // capability and pulse_control was offered but never submitted, + // refuse the prose ending once and steer to the exact calls. + if !textOnlySafetyBrake && + !controlToolExecutedThisRun && + advertisedActionGateBlocks < maxAdvertisedActionGateBlocks && + !isPatrolDetectionExecution(a.currentExecutionProfile()) && + !isPatrolInvestigationExecution(a.currentExecutionProfile()) && + a.executor != nil && + providerToolOffered(tools, agentcapabilities.PulseControlToolName) { + if action, ok := requestedLifecycleAction(latestUserRequest(messages)); ok { + if targets := a.executor.SessionTargetsAdvertisingAction(action); len(targets) > 0 { + advertisedActionGateBlocks++ + gatePrompt := buildAdvertisedActionGatePrompt(action, targets) + log.Warn(). + Str("session_id", sessionID). + Str("requested_action", action). + Int("advertised_targets", len(targets)). + Int("gate_blocks", advertisedActionGateBlocks). + Msg("[AgenticLoop] Refused prose-only ending for an advertised action request (advertised-action gate)") + // The premature prose stays in the transcript (it was + // already streamed); the correction is a provider-only + // user-role anchor so the next turn can submit the action. + providerMessages = appendFSMVerificationPrompt(providerMessages, gatePrompt) + currentTurnStartIndex = len(providerMessages) + turn++ + continue + } + } + } + // === FSM ENFORCEMENT GATE 2: Check if final answer is allowed === a.mu.Lock() fsm := a.sessionFSM @@ -2274,6 +2315,12 @@ agenticLoop: pendingExec = append(pendingExec, pendingToolExec{tc: tc, toolKind: toolKind}) // A real tool attempt satisfies the look-before-asking gate. lookGateToolAttempted = true + if tc.Name == agentcapabilities.PulseControlToolName { + // A submitted governed action satisfies the advertised-action + // gate whatever the plan outcome: a real boundary from this + // call is evidence the model may report. + controlToolExecutedThisRun = true + } } // --- Phase 2: Execute pending tools --- diff --git a/internal/ai/chat/agentic_action_gate.go b/internal/ai/chat/agentic_action_gate.go new file mode 100644 index 000000000..3d406f61f --- /dev/null +++ b/internal/ai/chat/agentic_action_gate.go @@ -0,0 +1,94 @@ +package chat + +import ( + "fmt" + "regexp" + "strings" + + "github.com/rcourtman/pulse-go-rewrite/internal/ai/providers" + "github.com/rcourtman/pulse-go-rewrite/internal/ai/tools" +) + +// maxAdvertisedActionGateBlocks bounds the advertised-action gate. When the +// user asked for a lifecycle action, the session has resolved resources that +// advertise it, pulse_control was offered, and the model still ends the run +// with prose instead of a governed call, the gate refuses that final answer +// once and steers the model to submit pulse_control for each target. It fails +// open after this many refusals so a model that has a genuine reason not to +// act (which it must then state from tool evidence) cannot livelock. +const maxAdvertisedActionGateBlocks = 1 + +// lifecycleRequestPatterns maps operator phrasing to the canonical lifecycle +// verb pulse_control accepts. Order matters: "restart" must resolve before the +// bare "start" pattern is considered, and the reboot/restart pair is folded +// to one verb because the action lifecycle treats them as synonyms. +var lifecycleRequestPatterns = []struct { + action string + pattern *regexp.Regexp +}{ + {action: "reboot", pattern: regexp.MustCompile(`\b(reboot|restart|power[- ]?cycle|bounce)\b`)}, + {action: "shutdown", pattern: regexp.MustCompile(`\b(shut ?down|power[- ]?off|halt)\b`)}, + {action: "stop", pattern: regexp.MustCompile(`\bstop\b`)}, + {action: "start", pattern: regexp.MustCompile(`\b(start|boot|power[- ]?on|bring up|spin up)\b`)}, +} + +// interrogativeLead matches messages that ask about an action rather than +// request one ("why did X reboot?", "is it safe to stop Y?"). Those must keep +// their investigative answer; the gate only applies to action requests. +var interrogativeLead = regexp.MustCompile(`^\s*(why|what|when|where|who|how|did|was|were|is|are|has|have|should|do|does|which|whether)\b`) + +// requestedLifecycleAction reports the canonical lifecycle verb an operator +// message asks Pulse to perform, if any. +func requestedLifecycleAction(userText string) (string, bool) { + text := strings.ToLower(strings.TrimSpace(userText)) + if text == "" || interrogativeLead.MatchString(text) { + return "", false + } + for _, candidate := range lifecycleRequestPatterns { + if candidate.pattern.MatchString(text) { + return candidate.action, true + } + } + return "", false +} + +// latestUserRequest returns the most recent operator message in the run's +// input transcript, ignoring tool-result carriers that share the user role. +func latestUserRequest(messages []Message) string { + for i := len(messages) - 1; i >= 0; i-- { + msg := messages[i] + if msg.Role != "user" || msg.ToolResult != nil { + continue + } + if content := strings.TrimSpace(msg.Content); content != "" { + return content + } + } + return "" +} + +func providerToolOffered(offered []providers.Tool, name string) bool { + for _, tool := range offered { + if strings.TrimSpace(tool.Name) == name { + return true + } + } + return false +} + +// buildAdvertisedActionGatePrompt is the user-role correction injected when +// the gate refuses a prose-only ending. It names the exact calls to make and +// forbids the invented-prerequisite failure mode seen in the field. +func buildAdvertisedActionGatePrompt(action string, targets []tools.AdvertisedActionTarget) string { + lines := make([]string, 0, len(targets)) + for _, target := range targets { + lines = append(lines, fmt.Sprintf("- pulse_control {\"type\":\"resource\",\"resource_id\":%q,\"action\":%q} (%s %s)", target.CanonicalID, target.Capability, target.Kind, target.Name)) + } + noun := "resource advertises" + if len(targets) != 1 { + noun = "resources advertise" + } + return fmt.Sprintf(`BLOCKED: the user asked you to %s, and %d resolved %s that capability right now. Pulse offers pulse_control for exactly this, so a final answer that narrates next steps, manual commands, or prerequisites is not acceptable. Submit the governed action for each target now, one call per target, using the canonical resource id: +%s +Pulse owns planning, approval, execution, and verification from there; the user approves in Pulse, not by running commands. If a tool result in this turn reported a real boundary for a target, quote that exact result for that target instead. Do not invent prerequisites such as discovery, session or context binding, or guest-agent availability.`, action, len(targets), noun, strings.Join(lines, "\n")) +} diff --git a/internal/ai/chat/agentic_action_gate_test.go b/internal/ai/chat/agentic_action_gate_test.go new file mode 100644 index 000000000..1b78f6f97 --- /dev/null +++ b/internal/ai/chat/agentic_action_gate_test.go @@ -0,0 +1,336 @@ +package chat + +// Regression transcript for GitHub issue #1782. A paying operator asked the +// Assistant (Gemini, Controlled mode) to reboot five Windows VMs matching a +// name pattern. The model resolved all five with pulse_query, then ended the +// run with a markdown report ("Next steps", an invented prerequisite) and +// never submitted pulse_control. The expected behaviour is one governed +// pulse_control plan per target, each awaiting approval in Pulse. +// +// The scripted provider below reproduces the field transcript turn by turn. +// On a build without the advertised-action gate the run ends at turn 2 with +// the report and zero pulse_control calls, which is the failing assertion. + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "sync" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/ai/providers" + "github.com/rcourtman/pulse-go-rewrite/internal/ai/tools" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" + "github.com/stretchr/testify/require" +) + +const gateTestReport = `## Summary +I found five Windows VMs matching "win": win-01, win-02, win-03, win-04, win-05. + +## Limitation +The reboot could not be scheduled because these VMs are not yet bound to a discovery session in the current context. + +## Next steps +1. Run a discovery for the VMs. +2. Ask me again and I will reboot them.` + +func gateTestProxmoxVM(name string, vmid int) unifiedresources.Resource { + capabilities := []unifiedresources.ResourceCapability{} + for _, operation := range []string{"shutdown", "reboot", "stop"} { + capabilities = append(capabilities, unifiedresources.ResourceCapability{ + Name: operation, + Type: unifiedresources.CapabilityTypeCommon, + Description: "Proxmox VM lifecycle " + operation, + MinimumApprovalLevel: unifiedresources.ApprovalAdmin, + Platform: "qemu", + InternalHandler: "proxmox.vm.lifecycle", + }) + } + return unifiedresources.Resource{ + ID: "vm-pve-" + name, + Type: unifiedresources.ResourceTypeVM, + Name: name, + Status: unifiedresources.StatusOnline, + ParentName: "pve", + Capabilities: capabilities, + Proxmox: &unifiedresources.ProxmoxData{ + SourceID: fmt.Sprintf("pve:pve:%d", vmid), + NodeName: "pve", + Instance: "pve", + VMID: vmid, + }, + } +} + +type gateTestPlanner struct { + mu sync.Mutex + requests []unifiedresources.ActionRequest +} + +func (p *gateTestPlanner) PlanTypedAction(_ context.Context, _ string, req unifiedresources.ActionRequest) (*unifiedresources.ActionPlan, error) { + p.mu.Lock() + defer p.mu.Unlock() + p.requests = append(p.requests, req) + return &unifiedresources.ActionPlan{ + ActionID: fmt.Sprintf("action-%d", len(p.requests)), + RequestID: req.RequestID, + Allowed: true, + RequiresApproval: true, + ApprovalPolicy: unifiedresources.ApprovalAdmin, + PlanHash: "hash", + }, nil +} + +func (p *gateTestPlanner) snapshot() []unifiedresources.ActionRequest { + p.mu.Lock() + defer p.mu.Unlock() + return append([]unifiedresources.ActionRequest(nil), p.requests...) +} + +func newGateTestExecutor(t *testing.T, planner tools.TypedActionPlanner, vms ...unifiedresources.Resource) *tools.PulseToolExecutor { + t.Helper() + exec := tools.NewPulseToolExecutor(tools.ExecutorConfig{ + StateProvider: &mockStateProvider{}, + AgentServer: &mockAgentServer{}, + UnifiedResourceProvider: plainTextResourceTestProvider(vms...), + TypedActionPlanner: planner, + ControlLevel: tools.ControlLevelControlled, + }) + exec.SetResolvedContext(NewResolvedContext("gate-session")) + return exec +} + +func gateTestControlCalls(vms []unifiedresources.Resource, action string) []providers.ToolCall { + calls := make([]providers.ToolCall, 0, len(vms)) + for i, vm := range vms { + calls = append(calls, providers.ToolCall{ + ID: fmt.Sprintf("c-%d", i+1), + Name: "pulse_control", + Input: map[string]interface{}{ + "type": "resource", + "resource_id": vm.ID, + "action": action, + }, + }) + } + return calls +} + +func gateTestContainsBlock(req providers.ChatRequest) bool { + for _, msg := range req.Messages { + if msg.Role == "user" && strings.Contains(msg.Content, "BLOCKED: the user asked you to reboot") { + return true + } + } + return false +} + +// TestAgenticLoop_BulkLifecycleRequestEndsInPulseControlPlans is the #1782 +// transcript: resolve five VMs, try to end with a report, and prove the run +// instead submits one governed plan per target before answering. +func TestAgenticLoop_BulkLifecycleRequestEndsInPulseControlPlans(t *testing.T) { + vms := []unifiedresources.Resource{ + gateTestProxmoxVM("win-01", 101), + gateTestProxmoxVM("win-02", 102), + gateTestProxmoxVM("win-03", 103), + gateTestProxmoxVM("win-04", 104), + gateTestProxmoxVM("win-05", 105), + } + planner := &gateTestPlanner{} + exec := newGateTestExecutor(t, planner, vms...) + + var ( + mu sync.Mutex + turn int + blockSeenAtTurn int + requestsPerTurn []providers.ChatRequest + reportStreamed bool + finalAnswerTurns int + ) + provider := &stubStreamingProvider{} + provider.chatStream = func(ctx context.Context, req providers.ChatRequest, callback providers.StreamCallback) error { + mu.Lock() + turn++ + current := turn + requestsPerTurn = append(requestsPerTurn, req) + if gateTestContainsBlock(req) && blockSeenAtTurn == 0 { + blockSeenAtTurn = current + } + mu.Unlock() + + switch current { + case 1: + // The model resolves the targets exactly as in the field. + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{ + ToolCalls: []providers.ToolCall{{ + ID: "q-1", + Name: "pulse_query", + Input: map[string]interface{}{"action": "search", "query": "win", "type": "vm"}, + }}, + }}) + case 2: + // The field failure: a report with an invented prerequisite. + mu.Lock() + reportStreamed = true + mu.Unlock() + callback(providers.StreamEvent{Type: "content", Data: providers.ContentEvent{Text: gateTestReport}}) + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{}}) + case 3: + // Steered by the gate, the model submits one plan per target. + require.True(t, gateTestContainsBlock(req), "turn 3 must carry the advertised-action correction") + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{ToolCalls: gateTestControlCalls(vms, "reboot")}}) + case 4: + // Post-write verification read demanded by the FSM. + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{ + ToolCalls: []providers.ToolCall{{ + ID: "q-2", + Name: "pulse_query", + Input: map[string]interface{}{"action": "search", "query": "win", "type": "vm"}, + }}, + }}) + default: + mu.Lock() + finalAnswerTurns++ + mu.Unlock() + callback(providers.StreamEvent{Type: "content", Data: providers.ContentEvent{Text: "Planned a reboot for all five VMs; approve them in Pulse to proceed."}}) + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{}}) + } + return nil + } + + loop := NewAgenticLoop(provider, exec, "base prompt") + loop.SetSessionFSM(NewSessionFSM()) + + messages, err := loop.ExecuteWithTools( + context.Background(), + "gate-session", + []Message{{Role: "user", Content: "Reboot all my Windows VMs whose name starts with win-. There should be five of them."}}, + nil, + func(StreamEvent) {}, + ) + require.NoError(t, err) + + planned := map[string]bool{} + for _, msg := range messages { + if msg.ToolResult != nil && msg.ToolResult.ToolUseID == "q-1" { + require.False(t, msg.ToolResult.IsError, "resolution query must succeed: %s", msg.ToolResult.Content) + require.Contains(t, msg.ToolResult.Content, "win-05", "resolution query must list every target: %s", msg.ToolResult.Content) + } + if msg.ToolResult == nil || !strings.HasPrefix(msg.ToolResult.ToolUseID, "c-") { + continue + } + require.False(t, msg.ToolResult.IsError, "pulse_control must plan, not error: %s", msg.ToolResult.Content) + var payload map[string]any + require.NoError(t, json.Unmarshal([]byte(msg.ToolResult.Content), &payload), msg.ToolResult.Content) + require.Equal(t, true, payload["planned"], payload) + require.Equal(t, true, payload["requires_approval"], "Controlled mode plans wait for approval: %v", payload) + require.Equal(t, "reboot", payload["capability"], payload) + planned[fmt.Sprint(payload["resource_id"])] = true + } + require.Len(t, planned, len(vms), "one governed plan per resolved target; got %v", planned) + + requests := planner.snapshot() + require.Len(t, requests, len(vms)) + for _, req := range requests { + require.True(t, strings.HasPrefix(req.ResourceID, "vm-pve-win-0"), "plans must carry the canonical unified id, got %q", req.ResourceID) + require.Equal(t, "reboot", req.CapabilityName) + } + + mu.Lock() + defer mu.Unlock() + require.True(t, reportStreamed, "the scripted field report must have been produced") + require.Equal(t, 3, blockSeenAtTurn, "the gate must refuse the report once and steer the very next turn") + require.Equal(t, 1, finalAnswerTurns, "after planning and verifying, the answer is accepted") + require.Len(t, requestsPerTurn, 5) + require.True(t, hasFinalAssistantText(messages)) +} + +// TestAgenticLoop_AdvertisedActionGateFailsOpenAfterOneRefusal pins the +// bounded escape hatch: a model that still answers in prose after the single +// correction is not livelocked, and the run ends with its answer. +func TestAgenticLoop_AdvertisedActionGateFailsOpenAfterOneRefusal(t *testing.T) { + vms := []unifiedresources.Resource{gateTestProxmoxVM("win-01", 101)} + planner := &gateTestPlanner{} + exec := newGateTestExecutor(t, planner, vms...) + + turn := 0 + provider := &stubStreamingProvider{} + provider.chatStream = func(ctx context.Context, req providers.ChatRequest, callback providers.StreamCallback) error { + turn++ + if turn == 1 { + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{ + ToolCalls: []providers.ToolCall{{ID: "q-1", Name: "pulse_query", Input: map[string]interface{}{"action": "search", "query": "win"}}}, + }}) + return nil + } + callback(providers.StreamEvent{Type: "content", Data: providers.ContentEvent{Text: "I will not reboot win-01: its console shows an in-progress Windows update (pulse_read evidence above)."}}) + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{}}) + return nil + } + + loop := NewAgenticLoop(provider, exec, "base prompt") + loop.SetSessionFSM(NewSessionFSM()) + messages, err := loop.ExecuteWithTools(context.Background(), "gate-failopen", []Message{{Role: "user", Content: "please reboot win-01"}}, nil, func(StreamEvent) {}) + require.NoError(t, err) + require.Equal(t, 3, turn, "query, refused prose, accepted prose") + require.Empty(t, planner.snapshot(), "the gate steers; it never submits on the model's behalf") + require.True(t, hasFinalAssistantText(messages)) +} + +// TestAgenticLoop_AdvertisedActionGateLeavesQuestionsAlone pins that an +// operator asking *about* a lifecycle event keeps a normal investigative +// answer: the gate only applies to action requests. +func TestAgenticLoop_AdvertisedActionGateLeavesQuestionsAlone(t *testing.T) { + vms := []unifiedresources.Resource{gateTestProxmoxVM("win-01", 101)} + planner := &gateTestPlanner{} + exec := newGateTestExecutor(t, planner, vms...) + + turn := 0 + provider := &stubStreamingProvider{} + provider.chatStream = func(ctx context.Context, req providers.ChatRequest, callback providers.StreamCallback) error { + turn++ + require.False(t, gateTestContainsBlock(req), "a question must never trip the advertised-action gate") + if turn == 1 { + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{ + ToolCalls: []providers.ToolCall{{ID: "q-1", Name: "pulse_query", Input: map[string]interface{}{"action": "search", "query": "win"}}}, + }}) + return nil + } + callback(providers.StreamEvent{Type: "content", Data: providers.ContentEvent{Text: "win-01 is running; nothing in the current state explains a reboot."}}) + callback(providers.StreamEvent{Type: "done", Data: providers.DoneEvent{}}) + return nil + } + + loop := NewAgenticLoop(provider, exec, "base prompt") + loop.SetSessionFSM(NewSessionFSM()) + _, err := loop.ExecuteWithTools(context.Background(), "gate-question", []Message{{Role: "user", Content: "Why did win-01 reboot last night?"}}, nil, func(StreamEvent) {}) + require.NoError(t, err) + require.Equal(t, 2, turn) + require.Empty(t, planner.snapshot()) +} + +func TestRequestedLifecycleAction(t *testing.T) { + cases := []struct { + text string + action string + ok bool + }{ + {"Reboot all my Windows VMs matching win-*", "reboot", true}, + {"can you restart the five win VMs?", "reboot", true}, + {"Please power-cycle win-01", "reboot", true}, + {"shut down win-02 gracefully", "shutdown", true}, + {"stop win-03 now", "stop", true}, + {"start win-04 again", "start", true}, + {"Why did win-01 reboot last night?", "", false}, + {"Is it safe to stop win-02?", "", false}, + {"Should I restart win-03?", "", false}, + {"how is my infrastructure doing?", "", false}, + {"", "", false}, + } + for _, tc := range cases { + action, ok := requestedLifecycleAction(tc.text) + require.Equal(t, tc.ok, ok, tc.text) + require.Equal(t, tc.action, action, tc.text) + } +} diff --git a/internal/ai/chat/fsm.go b/internal/ai/chat/fsm.go index edf37250b..d971ede7d 100644 --- a/internal/ai/chat/fsm.go +++ b/internal/ai/chat/fsm.go @@ -154,7 +154,7 @@ func (fsm *SessionFSM) CanExecuteTool(kind ToolKind, toolName string) error { State: fsm.State, ToolName: toolName, ToolKind: kind, - Reason: "POLICY_BLOCKED: state-changing tools require a validated target/resource context. Establish the target from current context or available tools, then retry only if the requested action still fits.", + Reason: "POLICY_BLOCKED: no resource has been resolved in this session yet, so this action cannot be bound to a canonical target. This is an ordering rule, not a missing prerequisite and not a limitation to report to the user: resolve the target with a read-only call first (for example pulse_query action=search query=), then retry this exact call.", Recoverable: true, } } diff --git a/internal/ai/chat/service.go b/internal/ai/chat/service.go index a28b9a117..aad96c31b 100644 --- a/internal/ai/chat/service.go +++ b/internal/ai/chat/service.go @@ -3861,6 +3861,8 @@ func (s *Service) buildSystemPromptWithToolGovernance(toolGovernance string) str - Not every VM or container supports control. Some API-backed platforms are read-only even when the resource type is "vm" or "system-container". - A Proxmox VM or LXC reboot is a hypervisor lifecycle action and does not require the QEMU guest agent. When the canonical resource advertises the requested lifecycle capability, use pulse_control and let Pulse enforce planning, approval, execution, and verification. - Never redirect the user to manual qm or pct commands when Pulse offers the requested lifecycle capability. Do not claim that a guest agent or capability is unavailable without current tool evidence; if a governed tool reports a real boundary, explain that exact result instead of inventing a prerequisite. +- A recoverable block that names the read-only step to take first (resolve the target, then retry) is an ordering rule, not a boundary: complete that step and retry the same governed call. Never present it to the user as a prerequisite, a session or discovery binding, or a limitation. +- When the user asks for an action and a canonical resource advertises that capability, submit pulse_control for each target and let planning, approval, execution, and verification decide availability. Report a limitation only from a tool result in this turn, never from an assumed prerequisite, and never hand the user manual steps for an action Pulse offers. - Write tools are allowed only when the user explicitly asks you to perform an action. - Status checks and monitoring are read-oriented; do not change state unless the user asked for a state change. - Resolve a missing target yourself before asking: use read-only query/topology tools to see which hosts, VMs, or containers exist. If exactly one plausible target matches the request (a single connected host, or one resource by that name), run read-only diagnostics against it and name the target in your answer instead of asking. diff --git a/internal/ai/chat/service_tooling_test.go b/internal/ai/chat/service_tooling_test.go index 2af0ab0e1..87aac0717 100644 --- a/internal/ai/chat/service_tooling_test.go +++ b/internal/ai/chat/service_tooling_test.go @@ -333,6 +333,17 @@ func TestBuildSystemPrompt_DoesNotClaimGenericVMControl(t *testing.T) { if !strings.Contains(prompt, "Never redirect the user to manual qm or pct commands when Pulse offers the requested lifecycle capability") { t.Fatalf("expected system prompt to keep offered Proxmox lifecycle actions on the governed Pulse path, got %q", prompt) } + for _, want := range []string{ + "is an ordering rule, not a boundary", + "Never present it to the user as a prerequisite, a session or discovery binding, or a limitation.", + "submit pulse_control for each target", + "Report a limitation only from a tool result in this turn", + "never hand the user manual steps for an action Pulse offers", + } { + if !strings.Contains(prompt, want) { + t.Fatalf("expected system prompt to carry the advertised-action contract (#1782) %q", want) + } + } if !strings.Contains(prompt, "Do not claim that a guest agent or capability is unavailable without current tool evidence") { t.Fatalf("expected system prompt to prohibit invented guest-agent limitations, got %q", prompt) } diff --git a/internal/ai/chat/types.go b/internal/ai/chat/types.go index 4dfd8f22a..2f307942c 100644 --- a/internal/ai/chat/types.go +++ b/internal/ai/chat/types.go @@ -1352,6 +1352,29 @@ func (rc *ResolvedContext) GetResolvedResourceByAlias(alias string) (tools.Resol return res, true } +// ListResolvedResources returns every live resource the session has resolved, +// ordered by canonical ID. It does not touch LRU state: enumeration is +// evidence gathering for the advertised-action gate, not user access. +func (rc *ResolvedContext) ListResolvedResources() []tools.ResolvedResourceInfo { + rc.mu.Lock() + defer rc.mu.Unlock() + + rc.evictExpired() + + ids := make([]string, 0, len(rc.ResourcesByID)) + for id, res := range rc.ResourcesByID { + if res != nil { + ids = append(ids, id) + } + } + sort.Strings(ids) + resources := make([]tools.ResolvedResourceInfo, 0, len(ids)) + for _, id := range ids { + resources = append(resources, rc.ResourcesByID[id]) + } + return resources +} + // ValidateResourceID checks if a resource ID exists in this context // and returns the resource if valid func (rc *ResolvedContext) ValidateResourceID(resourceID string) (*ResolvedResource, error) { diff --git a/internal/ai/eval/assertions.go b/internal/ai/eval/assertions.go index 3b8ea3b33..9bd83df9f 100644 --- a/internal/ai/eval/assertions.go +++ b/internal/ai/eval/assertions.go @@ -814,3 +814,29 @@ func min(a, b int) int { } return b } + +// AssertToolCallCountAtLeast checks that a tool was called at least min times +// in the step. Bulk action requests plan one governed action per target, so a +// single call for a five-target request is a failure, not a pass. +func AssertToolCallCountAtLeast(toolName string, min int) Assertion { + return func(result *StepResult) AssertionResult { + count := 0 + for _, tc := range result.ToolCalls { + if tc.Name == toolName { + count++ + } + } + if count < min { + return AssertionResult{ + Name: fmt.Sprintf("ToolCallCountAtLeast(%s, %d)", toolName, min), + Passed: false, + Message: fmt.Sprintf("%s was called %d time(s), want at least %d", toolName, count, min), + } + } + return AssertionResult{ + Name: fmt.Sprintf("ToolCallCountAtLeast(%s, %d)", toolName, min), + Passed: true, + Message: fmt.Sprintf("%s was called %d time(s)", toolName, count), + } + } +} diff --git a/internal/ai/eval/branchcov0723pm_test.go b/internal/ai/eval/branchcov0723pm_test.go index ded125807..7f0c02e53 100644 --- a/internal/ai/eval/branchcov0723pm_test.go +++ b/internal/ai/eval/branchcov0723pm_test.go @@ -60,6 +60,7 @@ func branchcov0723pmScenarioCtors() []struct { {"StrictResolutionScenario", StrictResolutionScenario}, {"StrictResolutionRecoveryScenario", StrictResolutionRecoveryScenario}, {"StrictResolutionBlockScenario", StrictResolutionBlockScenario}, + {"ProxmoxBulkLifecycleActionScenario", ProxmoxBulkLifecycleActionScenario}, {"ApprovalScenario", ApprovalScenario}, {"ApprovalComboScenario", ApprovalComboScenario}, {"ApprovalApproveScenario", ApprovalApproveScenario}, @@ -207,7 +208,7 @@ func TestBranchcov0723pm_ScenarioCatalogParity(t *testing.T) { func TestBranchcov0723pm_ScenarioCatalogInvariants(t *testing.T) { ctors := branchcov0723pmScenarioCtors() - require.Len(t, ctors, 36, "catalog table must list every scenario constructor") + require.Len(t, ctors, 37, "catalog table must list every scenario constructor") seen := make(map[string]string, len(ctors)) diff --git a/internal/ai/eval/eval_test.go b/internal/ai/eval/eval_test.go index e12239f80..df9375ce1 100644 --- a/internal/ai/eval/eval_test.go +++ b/internal/ai/eval/eval_test.go @@ -118,6 +118,7 @@ func TestAllScenarios(t *testing.T) { RoutingValidationScenario(), LogTailingScenario(), DiscoveryScenario(), + ProxmoxBulkLifecycleActionScenario(), } allPassed := true @@ -212,3 +213,23 @@ func TestAllPatrolScenarios(t *testing.T) { t.Fatal("One or more patrol scenarios failed") } } + +// TestProxmoxBulkLifecycleAction runs the issue #1782 regression: a bulk VM +// reboot request must end in governed pulse_control plans, not a report. +// Run with: go test -v ./internal/ai/eval -run TestProxmoxBulkLifecycleAction -live +// Set EVAL_LIFECYCLE_PATTERN and EVAL_LIFECYCLE_TARGET_COUNT for the estate. +func TestProxmoxBulkLifecycleAction(t *testing.T) { + if !*runLiveEval { + t.Skip("Skipping live eval test. Use -live flag to run against live Pulse API") + } + + runner := NewRunner(DefaultConfig()) + scenario := ProxmoxBulkLifecycleActionScenario() + + result := runner.RunScenario(scenario) + runner.PrintSummary(result) + + if !result.Passed { + t.Fatalf("Scenario '%s' failed", scenario.Name) + } +} diff --git a/internal/ai/eval/scenarios.go b/internal/ai/eval/scenarios.go index 582648347..04d4e4eb9 100644 --- a/internal/ai/eval/scenarios.go +++ b/internal/ai/eval/scenarios.go @@ -1650,3 +1650,55 @@ func NonInteractiveGuardrailScenario() Scenario { }, } } + +// ProxmoxBulkLifecycleActionScenario pins GitHub issue #1782: an operator asks +// the Assistant to reboot every Proxmox VM matching a name pattern. The +// Assistant must resolve the targets and then submit one governed +// pulse_control plan per target (approval stays in Pulse), never end with a +// report that invents a prerequisite (QEMU guest agent, "discovery binding", +// "session state") or redirects the operator to manual qm/pct commands. +// +// Targets: EVAL_LIFECYCLE_PATTERN (default "win") selects the VM name pattern; +// EVAL_LIFECYCLE_TARGET_COUNT (default 1) is the minimum number of plans the +// step must produce. Approvals raised in chat are denied so the eval never +// executes a reboot; canonical action plans wait for approval in Pulse and +// are left unapproved. +func ProxmoxBulkLifecycleActionScenario() Scenario { + pattern := envOrDefault("EVAL_LIFECYCLE_PATTERN", "win") + minTargets := 1 + if raw := strings.TrimSpace(os.Getenv("EVAL_LIFECYCLE_TARGET_COUNT")); raw != "" { + if _, err := fmt.Sscanf(raw, "%d", &minTargets); err != nil || minTargets < 1 { + minTargets = 1 + } + } + prompt := fmt.Sprintf("Reboot every Proxmox VM whose name contains %q. Do it through Pulse so I can approve each action; do not tell me to run commands myself.", pattern) + + return Scenario{ + Name: "Proxmox Bulk Lifecycle Action", + Description: "Bulk reboot request ends in one pulse_control plan per resolved VM, not a report with invented prerequisites (issue #1782)", + Steps: []Step{ + { + Name: "Reboot VMs by name pattern", + Prompt: prompt, + ApprovalDecision: ApprovalDeny, + ApprovalReason: "eval deny (lifecycle plans stay unapproved)", + Assertions: []Assertion{ + AssertNoError(), + AssertHasContent(), + AssertToolUsed("pulse_query"), + AssertToolUsed("pulse_control"), + AssertToolSequence([]string{"pulse_query", "pulse_control"}), + AssertToolCallCountAtLeast("pulse_control", minTargets), + AssertAnyToolInputContainsAny("pulse_control", "\"reboot\"", "\"restart\""), + AssertToolOutputContainsAny("pulse_control", "\"planned\":true", "\"planned\": true"), + AssertContentOmitsAll( + "qm reboot", "qm start", "qm stop", "pct reboot", "pct start", "pct stop", + "guest agent", "guest-agent", "qemu-guest-agent", + "discovery binding", "discovery session", "session binding", "session state", + ), + AssertNoResourceIdentityQuestion(), + }, + }, + }, + } +} diff --git a/internal/ai/tools/control_targets.go b/internal/ai/tools/control_targets.go new file mode 100644 index 000000000..7d0d0fe8a --- /dev/null +++ b/internal/ai/tools/control_targets.go @@ -0,0 +1,400 @@ +package tools + +import ( + "errors" + "fmt" + "sort" + "strings" + + "github.com/rcourtman/pulse-go-rewrite/internal/actionplanner" + "github.com/rcourtman/pulse-go-rewrite/internal/agentcapabilities" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// resolvedResourceLister is the optional session-context extension that lets +// the executor enumerate every resource the session has resolved so far. The +// chat ResolvedContext implements it; narrower test doubles may omit it. +type resolvedResourceLister interface { + ListResolvedResources() []ResolvedResourceInfo +} + +// controlTarget is the canonical binding pulse_control plans against. The +// session entry is the alias index the model saw in earlier query output; the +// canonical resource is the unified-inventory record whose ID the shared +// action lifecycle keys on and whose capability list is the only source of +// truth for "advertised". +type controlTarget struct { + session ResolvedResourceInfo + canonical *unifiedresources.Resource +} + +// canonicalID returns the identifier the action lifecycle registry keys on. +// A session-only binding (no unified provider wired) falls back to the +// session ID so narrow deployments keep working. +func (t controlTarget) canonicalID() string { + if t.canonical != nil { + if id := unifiedresources.CanonicalResourceID(t.canonical.ID); id != "" { + return id + } + } + if t.session != nil { + return unifiedresources.CanonicalResourceID(t.session.GetResourceID()) + } + return "" +} + +func (t controlTarget) displayName() string { + if t.canonical != nil { + if name := strings.TrimSpace(resourceDisplayName(*t.canonical)); name != "" { + return name + } + } + if t.session != nil { + for _, alias := range t.session.GetAliases() { + if alias = strings.TrimSpace(alias); alias != "" { + return alias + } + } + return strings.TrimSpace(t.session.GetResourceID()) + } + return "" +} + +// AdvertisedActionTarget names a session-resolved canonical resource that +// currently advertises a lifecycle capability. The agentic loop uses it to +// refuse a final answer that narrates an advertised action instead of +// submitting it through pulse_control. +type AdvertisedActionTarget struct { + CanonicalID string + Name string + Kind string + Capability string +} + +// lifecycleActionSynonym mirrors the action lifecycle's capability synonym +// table: Proxmox guests advertise "reboot" while container platforms +// advertise "restart", and operators use the words interchangeably. +func lifecycleActionSynonym(action string) (string, bool) { + switch strings.ToLower(strings.TrimSpace(action)) { + case "restart": + return "reboot", true + case "reboot": + return "restart", true + } + return "", false +} + +// advertisedCapabilityNames lists the resource's current capability names in a +// stable order for tool evidence. +func advertisedCapabilityNames(resource unifiedresources.Resource) []string { + names := canonicalCapabilityActions(resource) + sort.Strings(names) + return names +} + +// advertisedActionName reports the capability name the resource advertises +// for a requested lifecycle action, following the same synonym rule the +// action lifecycle applies when it plans. +func advertisedActionName(resource unifiedresources.Resource, action string) (string, bool) { + action = strings.ToLower(strings.TrimSpace(action)) + if action == "" { + return "", false + } + if _, found := actionplanner.FindCapability(resource.Capabilities, action); found { + return action, true + } + if synonym, ok := lifecycleActionSynonym(action); ok { + if _, found := actionplanner.FindCapability(resource.Capabilities, synonym); found { + return synonym, true + } + } + return "", false +} + +var controlCandidateResourceTypes = []unifiedresources.ResourceType{ + unifiedresources.ResourceTypeVM, + unifiedresources.ResourceTypeSystemContainer, + unifiedresources.ResourceTypeAppContainer, + unifiedresources.ResourceTypeAgent, +} + +func controlResourceTypeForKind(kind string) (unifiedresources.ResourceType, bool) { + switch strings.ToLower(strings.TrimSpace(kind)) { + case "vm": + return unifiedresources.ResourceTypeVM, true + case "system-container", "lxc": + return unifiedresources.ResourceTypeSystemContainer, true + case "app-container": + return unifiedresources.ResourceTypeAppContainer, true + case "agent", "node", "docker-host": + return unifiedresources.ResourceTypeAgent, true + } + return "", false +} + +// canonicalResourceForResolved maps a session-resolved resource back to its +// unified-inventory record. It matches on the canonical ID that query tools +// register as an alias, then on provider identity (Proxmox VMID on the same +// node, app-container provider ID on the same host, agent name). It never +// matches on host names, IP addresses, or tags, which the alias list also +// carries: those are lookup conveniences, not identity, and a write must not +// bind to a neighbour by accident. +func canonicalResourceForResolved(provider UnifiedResourceProvider, resolved ResolvedResourceInfo) (unifiedresources.Resource, bool) { + if provider == nil || resolved == nil { + return unifiedresources.Resource{}, false + } + resourceType, ok := controlResourceTypeForKind(firstNonEmptyString(resolved.GetKind(), resolved.GetResourceType())) + if !ok { + return unifiedresources.Resource{}, false + } + candidates := provider.GetByType(resourceType) + for _, alias := range resolved.GetAliases() { + alias = strings.TrimSpace(alias) + if alias == "" { + continue + } + for _, resource := range candidates { + if strings.EqualFold(strings.TrimSpace(resource.ID), alias) { + return resource, true + } + } + } + switch resourceType { + case unifiedresources.ResourceTypeVM, unifiedresources.ResourceTypeSystemContainer: + vmid := resolved.GetVMID() + node := strings.TrimSpace(resolved.GetNode()) + if vmid <= 0 { + return unifiedresources.Resource{}, false + } + for _, resource := range candidates { + if resource.Proxmox == nil || resource.Proxmox.VMID != vmid { + continue + } + if node == "" || strings.EqualFold(strings.TrimSpace(resource.Proxmox.NodeName), node) { + return resource, true + } + } + case unifiedresources.ResourceTypeAppContainer: + providerUID := strings.TrimSpace(resolved.GetProviderUID()) + host := strings.TrimSpace(resolved.GetTargetHost()) + if providerUID == "" { + return unifiedresources.Resource{}, false + } + for _, resource := range candidates { + if !strings.EqualFold(strings.TrimSpace(appContainerProviderID(resource)), providerUID) { + continue + } + if host == "" || strings.EqualFold(strings.TrimSpace(canonicalAppContainerHost(resource)), host) { + return resource, true + } + } + case unifiedresources.ResourceTypeAgent: + for _, resource := range candidates { + name := strings.TrimSpace(resourceDisplayName(resource)) + for _, alias := range resolved.GetAliases() { + if name != "" && strings.EqualFold(name, strings.TrimSpace(alias)) { + return resource, true + } + } + } + } + return unifiedresources.Resource{}, false +} + +// canonicalControlCandidates resolves a model-supplied reference against the +// unified inventory. An exact canonical-ID match wins outright; otherwise every +// control-capable resource whose display name or name equals the reference is +// returned so the caller can refuse an ambiguous write instead of guessing. +func canonicalControlCandidates(provider UnifiedResourceProvider, ref string) []unifiedresources.Resource { + ref = strings.TrimSpace(ref) + if provider == nil || ref == "" { + return nil + } + var byName []unifiedresources.Resource + for _, resourceType := range controlCandidateResourceTypes { + for _, resource := range provider.GetByType(resourceType) { + if strings.EqualFold(strings.TrimSpace(resource.ID), ref) { + return []unifiedresources.Resource{resource} + } + if strings.EqualFold(strings.TrimSpace(resourceDisplayName(resource)), ref) || + strings.EqualFold(strings.TrimSpace(resource.Name), ref) { + byName = append(byName, resource) + } + } + } + return byName +} + +// resolveControlTarget binds a pulse_control reference to a canonical +// resource. Session context is consulted first because it is what the model +// just saw, but a reference that is absent from the session and resolves +// uniquely in the unified inventory is registered and accepted: the inventory +// is Pulse's own discovery, so demanding a second in-session "discovery" step +// is an ordering accident, not a safety property. Capability, approval, and +// execution stay with the shared action lifecycle. +func (e *PulseToolExecutor) resolveControlTarget(ref, action string) (controlTarget, *CallToolResult) { + target := controlTarget{} + if e.resolvedContext != nil { + if res, ok := e.resolvedContext.GetResolvedResourceByAlias(ref); ok && res != nil { + target.session = res + } else if res, ok := e.resolvedContext.GetResolvedResourceByID(ref); ok && res != nil { + target.session = res + } + } + + if e.unifiedResourceProvider != nil { + if target.session != nil { + if resource, ok := canonicalResourceForResolved(e.unifiedResourceProvider, target.session); ok { + target.canonical = &resource + } + } + if target.canonical == nil { + candidates := canonicalControlCandidates(e.unifiedResourceProvider, ref) + switch len(candidates) { + case 0: + case 1: + resource := candidates[0] + target.canonical = &resource + if target.session == nil && e.resolvedContext != nil { + if reg, ok := CanonicalHandoffResourceRegistration(e.unifiedResourceProvider, resource.ID, "", string(unifiedresources.ContractResourceType(resource)), ""); ok { + e.registerResolvedResourceWithExplicitAccess(reg) + if res, ok := e.resolvedContext.GetResolvedResourceByID(resource.ID); ok && res != nil { + target.session = res + } else if res, ok := e.resolvedContext.GetResolvedResourceByAlias(reg.Name); ok && res != nil { + target.session = res + } + } + } + default: + ids := make([]string, 0, len(candidates)) + for _, candidate := range candidates { + ids = append(ids, fmt.Sprintf("%s (%s)", candidate.ID, unifiedresources.ContractResourceType(candidate))) + } + sort.Strings(ids) + result := NewToolResponseResult(NewToolBlockedError( + agentcapabilities.ErrCodeInvalidInput, + fmt.Sprintf("%d canonical resources are named %q; call pulse_control again with one of these canonical resource ids: %s.", len(candidates), ref, strings.Join(ids, ", ")), + map[string]interface{}{ + "resource_id": ref, + "action": action, + "candidates": ids, + "policy_boundary": "Ambiguous target reference. Retry with a canonical resource id from this list; this is a lookup detail, not a missing prerequisite.", + }, + )) + return target, &result + } + } + } + + if target.session == nil && target.canonical == nil { + if isStrictResolutionEnabled() && isWriteAction(action) { + if e.telemetryCallback != nil { + e.telemetryCallback.RecordStrictResolutionBlock("pulse_control", action) + } + strict := &ErrStrictResolution{ + ResourceID: ref, + Action: action, + Message: fmt.Sprintf("No resolved or canonical resource matches %q. Call pulse_query action=search query=%q, then retry pulse_control with a returned name or canonical resource id before performing %q.", ref, ref, action), + } + result := NewToolResponseResult(strict.ToToolResponse()) + return target, &result + } + result := NewToolResponseResult(NewToolBlockedError( + agentcapabilities.ErrCodeNotFound, + fmt.Sprintf("No canonical resource matches %q. Call pulse_query action=search query=%q to list matching resources, then call pulse_control again with a returned name or canonical resource id.", ref, ref), + map[string]interface{}{ + "resource_id": ref, + "action": action, + "policy_boundary": "Target lookup miss. Retry with a name or canonical id returned by pulse_query; this is a lookup detail, not a missing prerequisite, and the user does not need to do anything.", + }, + )) + return target, &result + } + + return target, nil +} + +// controlPlanFailureResult turns a planning error into tool evidence the model +// can report faithfully. A capability the resource does not advertise is a +// real boundary and is described with the resource's current capability list; +// anything else is passed through unchanged. +func controlPlanFailureResult(target controlTarget, action string, err error) CallToolResult { + if err == nil { + return NewErrorResult(fmt.Errorf("canonical action planning failed")) + } + if !errors.Is(err, actionplanner.ErrCapabilityNotFound) { + return NewErrorResult(err) + } + name := target.displayName() + details := map[string]interface{}{ + "resource_id": target.canonicalID(), + "requested_action": action, + "policy_boundary": "The resource does not currently advertise this capability. Report exactly this boundary; do not invent other prerequisites or redirect the user to manual commands.", + } + message := fmt.Sprintf("%q is not permitted on %s: the resource does not currently advertise that capability.", action, name) + if target.canonical != nil { + advertised := advertisedCapabilityNames(*target.canonical) + details["advertised_capabilities"] = advertised + details["status"] = string(target.canonical.Status) + if len(advertised) > 0 { + message = fmt.Sprintf("%q is not permitted on %s: it does not currently advertise that capability; its advertised capabilities right now are %s (status %s).", action, name, strings.Join(advertised, ", "), target.canonical.Status) + } else { + message = fmt.Sprintf("%q is not permitted on %s: it does not currently advertise that capability or any other lifecycle capability (status %s).", action, name, target.canonical.Status) + } + } + return NewToolResponseResult(NewToolBlockedError(agentcapabilities.ErrCodeActionNotAllowed, message, details)) +} + +// SessionTargetsAdvertisingAction lists the session-resolved resources whose +// unified-inventory record currently advertises the requested lifecycle +// action (or its lifecycle synonym). It is the evidence behind the agentic +// loop's advertised-action gate. +func (e *PulseToolExecutor) SessionTargetsAdvertisingAction(action string) []AdvertisedActionTarget { + if e == nil || e.resolvedContext == nil || e.unifiedResourceProvider == nil { + return nil + } + lister, ok := e.resolvedContext.(resolvedResourceLister) + if !ok { + return nil + } + action = strings.ToLower(strings.TrimSpace(action)) + if action == "" { + return nil + } + seen := make(map[string]struct{}) + var targets []AdvertisedActionTarget + for _, resolved := range lister.ListResolvedResources() { + if resolved == nil { + continue + } + resource, ok := canonicalResourceForResolved(e.unifiedResourceProvider, resolved) + if !ok { + continue + } + capability, ok := advertisedActionName(resource, action) + if !ok { + continue + } + id := unifiedresources.CanonicalResourceID(resource.ID) + if id == "" { + continue + } + if _, dup := seen[id]; dup { + continue + } + seen[id] = struct{}{} + targets = append(targets, AdvertisedActionTarget{ + CanonicalID: id, + Name: firstNonEmptyString(strings.TrimSpace(resourceDisplayName(resource)), id), + Kind: string(unifiedresources.ContractResourceType(resource)), + Capability: capability, + }) + } + sort.Slice(targets, func(i, j int) bool { + if targets[i].Name == targets[j].Name { + return targets[i].CanonicalID < targets[j].CanonicalID + } + return targets[i].Name < targets[j].Name + }) + return targets +} diff --git a/internal/ai/tools/control_targets_test.go b/internal/ai/tools/control_targets_test.go new file mode 100644 index 000000000..313f04a7e --- /dev/null +++ b/internal/ai/tools/control_targets_test.go @@ -0,0 +1,404 @@ +package tools + +// Regression coverage for GitHub issue #1782: an operator asks the Assistant +// to reboot Proxmox VMs, the model resolves them, and pulse_control must plan +// the advertised lifecycle action rather than manufacture a "discovery", +// "session binding", or guest-agent prerequisite. Three defects produced that +// behaviour and each is pinned here: +// +// 1. pulse_control handed the session-scoped ID (vm::) to the +// action lifecycle, whose registry keys on canonical unified IDs, so a +// Proxmox guest plan could never resolve. +// 2. pulse_control gated the action on the legacy per-executor action list, +// which never knew the canonical "reboot" capability Proxmox guests +// advertise. +// 3. A reference absent from the session context was refused with a +// "discovery is required" message even when the unified inventory +// resolved it uniquely. + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/actionplanner" + "github.com/rcourtman/pulse-go-rewrite/internal/agentcapabilities" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +func controlTestProxmoxVM(name string, vmid int, node string, running bool) unifiedresources.Resource { + status := unifiedresources.StatusOnline + operations := []string{"shutdown", "reboot", "stop"} + if !running { + status = unifiedresources.StatusOffline + operations = []string{"start"} + } + capabilities := make([]unifiedresources.ResourceCapability, 0, len(operations)) + for _, operation := range operations { + capabilities = append(capabilities, unifiedresources.ResourceCapability{ + Name: operation, + Type: unifiedresources.CapabilityTypeCommon, + Description: "Proxmox VM lifecycle " + operation, + MinimumApprovalLevel: unifiedresources.ApprovalAdmin, + Platform: "qemu", + InternalHandler: "proxmox.vm.lifecycle", + }) + } + return unifiedresources.Resource{ + ID: fmt.Sprintf("vm-%s-%s", node, name), + Type: unifiedresources.ResourceTypeVM, + Name: name, + Status: status, + ParentName: node, + Capabilities: capabilities, + Proxmox: &unifiedresources.ProxmoxData{ + SourceID: fmt.Sprintf("%s:%s:%d", node, node, vmid), + NodeName: node, + Instance: node, + VMID: vmid, + }, + } +} + +// listingResolvedContext adds the optional enumeration surface the +// advertised-action gate consults. +type listingResolvedContext struct { + *mockResolvedContext +} + +func (l *listingResolvedContext) ListResolvedResources() []ResolvedResourceInfo { + out := make([]ResolvedResourceInfo, 0, len(l.resources)) + for _, res := range l.resources { + out = append(out, res) + } + return out +} + +func newControlTestResolvedContext() *listingResolvedContext { + return &listingResolvedContext{mockResolvedContext: &mockResolvedContext{ + resources: make(map[string]ResolvedResourceInfo), + aliases: make(map[string]ResolvedResourceInfo), + lastAccessed: make(map[string]time.Time), + }} +} + +type recordedPlan struct { + requests []unifiedresources.ActionRequest +} + +func (r *recordedPlan) planner(err error) typedActionPlannerFunc { + return func(_ context.Context, _ string, req unifiedresources.ActionRequest) (*unifiedresources.ActionPlan, error) { + r.requests = append(r.requests, req) + if err != nil { + return nil, err + } + return &unifiedresources.ActionPlan{ + ActionID: "action-" + req.RequestID, + RequestID: req.RequestID, + Allowed: true, + RequiresApproval: true, + ApprovalPolicy: unifiedresources.ApprovalAdmin, + PlanHash: "hash-1", + }, nil + } +} + +func decodeControlPayload(t *testing.T, result CallToolResult) map[string]any { + t.Helper() + if len(result.Content) == 0 { + t.Fatalf("expected result content, got %+v", result) + } + var payload map[string]any + if err := json.Unmarshal([]byte(result.Content[0].Text), &payload); err != nil { + t.Fatalf("decode result payload %q: %v", result.Content[0].Text, err) + } + return payload +} + +func decodeControlToolResponse(t *testing.T, result CallToolResult) ToolResponse { + t.Helper() + if len(result.Content) == 0 { + t.Fatalf("expected result content, got %+v", result) + } + var response ToolResponse + if err := json.Unmarshal([]byte(result.Content[0].Text), &response); err != nil { + t.Fatalf("decode tool response %q: %v", result.Content[0].Text, err) + } + return response +} + +func TestExecuteControlResource_PlansAdvertisedRebootAgainstCanonicalID(t *testing.T) { + vm := controlTestProxmoxVM("win-01", 101, "pve", true) + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{vm}} + resolved := newControlTestResolvedContext() + reg, ok := canonicalGuestRegistration("vm", vm) + if !ok { + t.Fatal("expected canonical guest registration") + } + resolved.AddResolvedResource(reg) + + plans := &recordedPlan{} + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + TypedActionPlanner: plans.planner(nil), + }) + executor.SetResolvedContext(resolved) + + result, err := executor.executeControl(context.Background(), map[string]interface{}{ + "type": "resource", + "resource_id": "win-01", + "action": "reboot", + }) + if err != nil { + t.Fatalf("executeControl: unexpected error: %v", err) + } + if result.IsError { + t.Fatalf("expected planned result, got error %+v", result) + } + if len(plans.requests) != 1 { + t.Fatalf("expected one plan request, got %d", len(plans.requests)) + } + if plans.requests[0].ResourceID != vm.ID { + t.Fatalf("plan ResourceID = %q, want canonical %q (session-scoped ids never resolve in the lifecycle registry)", plans.requests[0].ResourceID, vm.ID) + } + if plans.requests[0].CapabilityName != "reboot" { + t.Fatalf("plan CapabilityName = %q, want reboot", plans.requests[0].CapabilityName) + } + payload := decodeControlPayload(t, result) + if payload["planned"] != true || payload["requires_approval"] != true { + t.Fatalf("expected planned action awaiting approval, got %+v", payload) + } + if payload["resource_id"] != vm.ID || payload["capability"] != "reboot" || payload["resource_name"] != "win-01" { + t.Fatalf("unexpected plan payload %+v", payload) + } + if message, _ := payload["message"].(string); strings.Contains(strings.ToLower(message), "discover") { + t.Fatalf("plan message must not mention discovery: %q", message) + } +} + +func TestExecuteControlResource_RestartMapsToAdvertisedRebootInPayload(t *testing.T) { + vm := controlTestProxmoxVM("win-02", 102, "pve", true) + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{vm}} + resolved := newControlTestResolvedContext() + reg, _ := canonicalGuestRegistration("vm", vm) + resolved.AddResolvedResource(reg) + + plans := &recordedPlan{} + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + TypedActionPlanner: plans.planner(nil), + }) + executor.SetResolvedContext(resolved) + + result, err := executor.executeControl(context.Background(), map[string]interface{}{ + "type": "resource", + "resource_id": vm.ID, + "action": "restart", + }) + if err != nil || result.IsError { + t.Fatalf("expected planned result, got err=%v result=%+v", err, result) + } + payload := decodeControlPayload(t, result) + if payload["requested_action"] != "restart" || payload["capability"] != "reboot" { + t.Fatalf("expected restart to surface the advertised reboot capability, got %+v", payload) + } +} + +func TestExecuteControlResource_ResolvesCanonicalTargetWithoutSessionDiscovery(t *testing.T) { + vm := controlTestProxmoxVM("win-03", 103, "pve", true) + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{vm}} + resolved := newControlTestResolvedContext() + + plans := &recordedPlan{} + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + TypedActionPlanner: plans.planner(nil), + }) + executor.SetResolvedContext(resolved) + + result, err := executor.executeControl(context.Background(), map[string]interface{}{ + "type": "resource", + "resource_id": "win-03", + "action": "reboot", + }) + if err != nil { + t.Fatalf("executeControl: unexpected error: %v", err) + } + if result.IsError { + t.Fatalf("a reference that resolves uniquely in the unified inventory must plan, got %+v", result) + } + if len(plans.requests) != 1 || plans.requests[0].ResourceID != vm.ID { + t.Fatalf("expected one plan against %q, got %+v", vm.ID, plans.requests) + } + if _, ok := resolved.GetResolvedResourceByAlias("win-03"); !ok { + t.Fatal("expected the canonical target to be registered in session context after planning") + } +} + +func TestExecuteControlResource_UnknownReferenceNamesTheRecoveryCall(t *testing.T) { + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{controlTestProxmoxVM("win-04", 104, "pve", true)}} + plans := &recordedPlan{} + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + TypedActionPlanner: plans.planner(nil), + }) + executor.SetResolvedContext(newControlTestResolvedContext()) + + result, err := executor.executeControl(context.Background(), map[string]interface{}{ + "type": "resource", + "resource_id": "no-such-guest", + "action": "reboot", + }) + if err != nil { + t.Fatalf("executeControl: unexpected error: %v", err) + } + if !result.IsError { + t.Fatalf("expected a blocked result for an unknown reference, got %+v", result) + } + if len(plans.requests) != 0 { + t.Fatalf("an unresolved reference must not reach the planner, got %+v", plans.requests) + } + response := decodeControlToolResponse(t, result) + if response.Error == nil || response.Error.Code != agentcapabilities.ErrCodeNotFound { + t.Fatalf("expected %s, got %+v", agentcapabilities.ErrCodeNotFound, response.Error) + } + if !strings.Contains(response.Error.Message, "pulse_query action=search") { + t.Fatalf("lookup miss must name the exact recovery call, got %q", response.Error.Message) + } + lower := strings.ToLower(response.Error.Message + fmt.Sprint(response.Error.Details["policy_boundary"])) + if strings.Contains(lower, "discovery is required") || strings.Contains(lower, "has not been discovered") { + t.Fatalf("lookup miss must not read as a discovery prerequisite: %q", lower) + } +} + +func TestExecuteControlResource_RefusesAmbiguousNameWithCandidates(t *testing.T) { + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{ + controlTestProxmoxVM("win-05", 105, "pve-a", true), + controlTestProxmoxVM("win-05", 205, "pve-b", true), + }} + plans := &recordedPlan{} + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + TypedActionPlanner: plans.planner(nil), + }) + executor.SetResolvedContext(newControlTestResolvedContext()) + + result, err := executor.executeControl(context.Background(), map[string]interface{}{ + "type": "resource", + "resource_id": "win-05", + "action": "reboot", + }) + if err != nil { + t.Fatalf("executeControl: unexpected error: %v", err) + } + if !result.IsError || len(plans.requests) != 0 { + t.Fatalf("an ambiguous name must not plan, got result=%+v plans=%+v", result, plans.requests) + } + response := decodeControlToolResponse(t, result) + if response.Error == nil || response.Error.Code != agentcapabilities.ErrCodeInvalidInput { + t.Fatalf("expected %s, got %+v", agentcapabilities.ErrCodeInvalidInput, response.Error) + } + candidates, _ := response.Error.Details["candidates"].([]any) + if len(candidates) != 2 { + t.Fatalf("expected both canonical candidates to be listed, got %+v", response.Error.Details) + } +} + +func TestExecuteControlResource_UnadvertisedCapabilityIsToolEvidence(t *testing.T) { + vm := controlTestProxmoxVM("win-06", 106, "pve", false) // stopped: advertises start only + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{vm}} + resolved := newControlTestResolvedContext() + reg, _ := canonicalGuestRegistration("vm", vm) + resolved.AddResolvedResource(reg) + + plans := &recordedPlan{} + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + TypedActionPlanner: plans.planner(fmt.Errorf("plan: %w", actionplanner.ErrCapabilityNotFound)), + }) + executor.SetResolvedContext(resolved) + + result, err := executor.executeControl(context.Background(), map[string]interface{}{ + "type": "resource", + "resource_id": "win-06", + "action": "reboot", + }) + if err != nil { + t.Fatalf("executeControl: unexpected error: %v", err) + } + if !result.IsError { + t.Fatalf("expected a boundary result, got %+v", result) + } + response := decodeControlToolResponse(t, result) + if response.Error == nil || response.Error.Code != agentcapabilities.ErrCodeActionNotAllowed { + t.Fatalf("expected %s, got %+v", agentcapabilities.ErrCodeActionNotAllowed, response.Error) + } + if !strings.Contains(response.Error.Message, "start") || !strings.Contains(response.Error.Message, "reboot") { + t.Fatalf("boundary must name the requested and advertised capabilities, got %q", response.Error.Message) + } + advertised, _ := response.Error.Details["advertised_capabilities"].([]any) + if len(advertised) != 1 || advertised[0] != "start" { + t.Fatalf("expected advertised_capabilities [start], got %+v", response.Error.Details) + } +} + +func TestSessionTargetsAdvertisingAction_UsesCanonicalCapabilities(t *testing.T) { + running := controlTestProxmoxVM("win-07", 107, "pve", true) + stopped := controlTestProxmoxVM("win-08", 108, "pve", false) + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{running, stopped}} + resolved := newControlTestResolvedContext() + for _, vm := range []unifiedresources.Resource{running, stopped} { + reg, ok := canonicalGuestRegistration("vm", vm) + if !ok { + t.Fatalf("expected registration for %s", vm.Name) + } + resolved.AddResolvedResource(reg) + } + executor := NewPulseToolExecutor(ExecutorConfig{ + UnifiedResourceProvider: provider, + ControlLevel: ControlLevelControlled, + }) + executor.SetResolvedContext(resolved) + + reboot := executor.SessionTargetsAdvertisingAction("reboot") + if len(reboot) != 1 || reboot[0].CanonicalID != running.ID || reboot[0].Capability != "reboot" || reboot[0].Kind != "vm" { + t.Fatalf("expected only the running VM to advertise reboot, got %+v", reboot) + } + // The legacy executor action list never carried "reboot"; the canonical + // capability list is the only source of truth. + if targets := executor.SessionTargetsAdvertisingAction("restart"); len(targets) != 1 || targets[0].Capability != "reboot" { + t.Fatalf("restart must resolve to the advertised reboot synonym, got %+v", targets) + } + start := executor.SessionTargetsAdvertisingAction("start") + if len(start) != 1 || start[0].CanonicalID != stopped.ID { + t.Fatalf("expected only the stopped VM to advertise start, got %+v", start) + } + if targets := executor.SessionTargetsAdvertisingAction("delete"); len(targets) != 0 { + t.Fatalf("expected no targets for an unadvertised action, got %+v", targets) + } +} + +func TestSessionTargetsAdvertisingAction_IgnoresContextsWithoutEnumeration(t *testing.T) { + vm := controlTestProxmoxVM("win-09", 109, "pve", true) + provider := &stubUnifiedResourceProvider{resources: []unifiedresources.Resource{vm}} + resolved := &mockResolvedContext{ + resources: make(map[string]ResolvedResourceInfo), + aliases: make(map[string]ResolvedResourceInfo), + } + reg, _ := canonicalGuestRegistration("vm", vm) + resolved.AddResolvedResource(reg) + executor := NewPulseToolExecutor(ExecutorConfig{UnifiedResourceProvider: provider, ControlLevel: ControlLevelControlled}) + executor.SetResolvedContext(resolved) + if targets := executor.SessionTargetsAdvertisingAction("reboot"); len(targets) != 0 { + t.Fatalf("a context that cannot enumerate must yield no gate evidence, got %+v", targets) + } +} diff --git a/internal/ai/tools/tools_control.go b/internal/ai/tools/tools_control.go index a7d5d341c..b0ff3d91d 100644 --- a/internal/ai/tools/tools_control.go +++ b/internal/ai/tools/tools_control.go @@ -9,6 +9,7 @@ import ( "time" "github.com/google/uuid" + "github.com/rcourtman/pulse-go-rewrite/internal/actionplanner" "github.com/rcourtman/pulse-go-rewrite/internal/agentcapabilities" "github.com/rcourtman/pulse-go-rewrite/internal/agentexec" "github.com/rcourtman/pulse-go-rewrite/internal/ai/approval" @@ -81,24 +82,31 @@ func (e *PulseToolExecutor) executeControlResource(ctx context.Context, args map return NewErrorResult(fmt.Errorf("action is required")), nil } - validation := e.validateResolvedResource(resourceRef, action, true) - if validation.IsBlocked() { - return NewToolResponseResult(validation.StrictError.ToToolResponse()), nil + // Bind the reference to a canonical resource. The legacy per-executor + // allowed-action list is deliberately not consulted here: it predates + // canonical capabilities (Proxmox guests advertise "reboot", the legacy + // list only knew "restart"), and whether the action is available is the + // action lifecycle's decision at plan time, from the resource's own + // advertised capabilities. + target, blocked := e.resolveControlTarget(resourceRef, action) + if blocked != nil { + return *blocked, nil } - if validation.Resource == nil { - if validation.ErrorMsg != "" { - return NewErrorResult(errors.New(validation.ErrorMsg)), nil + // Current capability evidence is the only permitted source of "not + // available". When the canonical record is in hand and does not advertise + // the action (or its lifecycle synonym), answer from that evidence now so + // read-only platforms are refused with the resource's real capability + // list even before the planner runs. The planner applies the same rule. + if target.canonical != nil { + if _, ok := advertisedActionName(*target.canonical, action); !ok { + return controlPlanFailureResult(target, action, actionplanner.ErrCapabilityNotFound), nil } - return NewErrorResult(fmt.Errorf("resource '%s' has not been discovered in this session. Resource discovery is required first", resourceRef)), nil - } - if validation.ErrorMsg != "" { - return NewErrorResult(errors.New(validation.ErrorMsg)), nil } if e.typedActionPlanner == nil { return NewErrorResult(fmt.Errorf("canonical action planning is unavailable")), nil } - resourceID := unifiedresources.CanonicalResourceID(validation.Resource.GetResourceID()) + resourceID := target.canonicalID() if resourceID == "" { return NewErrorResult(fmt.Errorf("resource %q has no canonical resource id", resourceRef)), nil } @@ -110,18 +118,26 @@ func (e *PulseToolExecutor) executeControlResource(ctx context.Context, args map RequestedBy: "pulse_assistant", }) if err != nil { - return NewErrorResult(err), nil + return controlPlanFailureResult(target, action, err), nil + } + capability := action + if target.canonical != nil { + if advertised, ok := advertisedActionName(*target.canonical, action); ok { + capability = advertised + } } return NewJSONResult(map[string]interface{}{ "planned": true, "action_id": plan.ActionID, "resource_id": resourceID, - "capability": action, + "resource_name": target.displayName(), + "requested_action": action, + "capability": capability, "requires_approval": plan.RequiresApproval, "approval_policy": plan.ApprovalPolicy, "plan_hash": plan.PlanHash, "expires_at": plan.ExpiresAt, - "message": "Typed action planned. Approval and execution remain on the canonical action lifecycle.", + "message": "Typed action planned. Pulse owns approval, execution, and verification from here; do not ask the user to run the action manually.", }), nil }