diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index b4d885d88..3002a5607 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -1753,7 +1753,7 @@ deriving an older display status from `workflowStatusHistory`. ## Completion Obligations -1. Update this contract when canonical AI runtime or transport entry points move, including transport-level provider request-shape changes such as OpenAI-compatible `tool_choice` handling, runtime-failure classification splits (for example separating tool-choice request rejection, no tool-capable endpoint, and generic model-level lack of tool support into distinct causes), Patrol-specific verification surfaces such as `POST /api/ai/patrol/preflight` that exercise the full chat-completions path with a minimal tool definition rather than only listing models, Patrol-preflight cache observability where the AI Service caches the most recent preflight outcome (success, soft warning, or classified failure) and the AI settings response surfaces it as `patrol_preflight` so the UI can hydrate a "last verified" indicator without forcing operators to re-run preflight on every page load, the auto-trigger contract on `HandleUpdateAISettings` where the save handler runs `TriggerPatrolPreflightAsync` only when the change actually moved Patrol transport (model swap, provider key for that model changed, or assistant just enabled with a Patrol model) so routine settings saves do not burn provider tokens, the startup-seed contract where the AI Service handler dispatches the same async preflight on Pulse boot when assistant is enabled and a Patrol model is configured so the cache is populated for the first `/api/settings/ai` poll after a restart instead of blanking back to "never verified", the readiness-integration contract where the `tools` check in the Patrol readiness payload consults the cached preflight and surfaces the classified evidence (success, soft warning, or failure with classified summary plus "last preflight ") for the configured provider+model when available (falling back to the static `PatrolToolReadinessForModel` classifier only when the cache is empty or holds a result for a different model), the stateless-Patrol-input contract where `ExecutePatrolStream` must pass only the current run's user prompt into the agentic loop rather than reloading the persisted `patrol-main` session history (so a prior run that ended with orphan `tool_calls` cannot poison every subsequent run with malformed conversation structure), and the deterministic-resolve-gate contract where the `patrol_resolve_finding` tool adapter rejects LLM-driven resolves of event/persistent category findings (`backup`, `reliability`, `security`, `general`) when a deterministic verifier exists for the finding's key and that verifier either still detects the failure signal **or returns an inconclusive result** — preventing the LLM from optimistically resolving a finding its current investigation simply didn't re-surface, which was the source of the "Backup failed" flap (detected → auto-resolved → re-detected ten times in a day before this gate). The fail-closed-on-inconclusive policy treats verifier errors (timeouts, executor unavailability, transport faults) as "we don't know" rather than "go ahead": resolution of an event/persistent finding is effectively permanent (next detection registers as a regression and inflates counters), so the safe default is to refuse and require either a successful re-verification or operator action, the assessment-recovery contract where the overall-health "Recent Patrol errors" coverage factor in `summarizeRecentPatrolCoverage` suppresses the score penalty once three consecutive trailing successful full Patrol runs exist at the most-recent end of the recent-runs window — so the grade reflects current reality after a Patrol-affecting bug is fixed rather than dragging stale failures forward for the ~9 hours it takes scheduled runs to age them out of the trailing-10 ratio, the orphan-tool-call-repair contract where `convertToProviderMessages` injects synthetic is_error tool result messages for any `tool_call_id` in an assistant message that has no matching downstream tool result, so a chat session that ended mid-tool-call (network drop, ctx timeout, browser crash) cannot poison its next message with the structural-violation error the provider rejects — the synthetic content is marked is_error=true and explains the interruption so the model can retry the call or proceed without the data, and the patrol-session-bound contract where `ExecutePatrolStream` calls `SessionStore.TrimMessages` after persisting each run's messages to cap the patrol-main session at 200 messages (roughly two recent runs' worth) — without the bound the file grew unbounded at every scheduled run, reaching 16 MB and 3,593 messages within a month and making every `AddMessage` rewrite linearly more expensive; the canonical Patrol forensic log is the `PatrolRunRecord` history surfaced at `/api/ai/patrol/runs`, not the chat-session-shaped file +1. Update this contract when canonical AI runtime or transport entry points move, including transport-level provider request-shape changes such as OpenAI-compatible `tool_choice` handling, runtime-failure classification splits (for example separating tool-choice request rejection, no tool-capable endpoint, and generic model-level lack of tool support into distinct causes), Patrol-specific verification surfaces such as `POST /api/ai/patrol/preflight` that exercise the full chat-completions path with a minimal tool definition rather than only listing models, Patrol-preflight cache observability where the AI Service caches the most recent preflight outcome (success, soft warning, or classified failure) and the AI settings response surfaces it as `patrol_preflight` so the UI can hydrate a "last verified" indicator without forcing operators to re-run preflight on every page load, the auto-trigger contract on `HandleUpdateAISettings` where the save handler runs `TriggerPatrolPreflightAsync` only when the change actually moved Patrol transport (model swap, provider key for that model changed, or assistant just enabled with a Patrol model) so routine settings saves do not burn provider tokens, the startup-seed contract where the AI Service handler dispatches the same async preflight on Pulse boot when assistant is enabled and a Patrol model is configured so the cache is populated for the first `/api/settings/ai` poll after a restart instead of blanking back to "never verified", the readiness-integration contract where the `tools` check in the Patrol readiness payload consults the cached preflight and surfaces the classified evidence (success, soft warning, or failure with classified summary plus "last preflight ") for the configured provider+model when available (falling back to the static `PatrolToolReadinessForModel` classifier only when the cache is empty or holds a result for a different model), the stateless-Patrol-input contract where `ExecutePatrolStream` must pass only the current run's user prompt into the agentic loop rather than reloading the persisted `patrol-main` session history (so a prior run that ended with orphan `tool_calls` cannot poison every subsequent run with malformed conversation structure), and the deterministic-resolve-gate contract where the `patrol_resolve_finding` tool adapter rejects LLM-driven resolves of event/persistent category findings (`backup`, `reliability`, `security`, `general`) when a deterministic verifier exists for the finding's key and that verifier either still detects the failure signal **or returns an inconclusive result** — preventing the LLM from optimistically resolving a finding its current investigation simply didn't re-surface, which was the source of the "Backup failed" flap (detected → auto-resolved → re-detected ten times in a day before this gate). The fail-closed-on-inconclusive policy treats verifier errors (timeouts, executor unavailability, transport faults) as "we don't know" rather than "go ahead": resolution of an event/persistent finding is effectively permanent (next detection registers as a regression and inflates counters), so the safe default is to refuse and require either a successful re-verification or operator action. The gate has a symmetric counterpart, the verified stale-resolve contract: `reconcileStaleFindings` (`internal/ai/patrol_ai.go`) may auto-resolve an event/persistent finding that was seeded but neither re-reported nor resolved ONLY when the finding's key has a deterministic verifier and that verifier affirmatively confirms the failure signal is gone — absence of a re-report remains insufficient evidence for these categories, "still present" or inconclusive verification leaves the finding active (the same fail-closed default), and verifications are capped per reconcile pass (`maxVerifiedStaleResolvesPerRun`) with deferred candidates logged and retried on the next successful full patrol. Without this counterpart the lifecycle was asymmetric: a genuinely fixed backup or recovered service stayed an active finding indefinitely unless the LLM happened to call `patrol_resolve_finding`. `hasDeterministicVerifierForKey` (`internal/ai/patrol_findings.go`) is the single source of truth for which keys have verifiers, consulted by both the gate and the reconcile pass, and must stay aligned with the dispatch switch in `verifyFixDeterministically` (it previously listed two of the seven dispatch keys, silently skipping verification that existed). Finding keys normalize onto the canonical verifier vocabulary in `normalizeFindingKey` via an alias map of unambiguous directional synonyms (`high-cpu` → `cpu-high`, `high-memory` → `memory-high`, `high-disk` → `disk-high`) so deduplication and deterministic verification meet on one key, and the `patrol_report_finding` key guidance (`internal/ai/tools/tools_patrol.go`) teaches the canonical vocabulary; semantically distinct keys (`pbs-job-failed`, `node-offline`) must not be aliased onto verifier keys whose resource model they do not match, the assessment-recovery contract where the overall-health "Recent Patrol errors" coverage factor in `summarizeRecentPatrolCoverage` suppresses the score penalty once three consecutive trailing successful full Patrol runs exist at the most-recent end of the recent-runs window — so the grade reflects current reality after a Patrol-affecting bug is fixed rather than dragging stale failures forward for the ~9 hours it takes scheduled runs to age them out of the trailing-10 ratio, the orphan-tool-call-repair contract where `convertToProviderMessages` injects synthetic is_error tool result messages for any `tool_call_id` in an assistant message that has no matching downstream tool result, so a chat session that ended mid-tool-call (network drop, ctx timeout, browser crash) cannot poison its next message with the structural-violation error the provider rejects — the synthetic content is marked is_error=true and explains the interruption so the model can retry the call or proceed without the data, and the patrol-session-bound contract where `ExecutePatrolStream` calls `SessionStore.TrimMessages` after persisting each run's messages to cap the patrol-main session at 200 messages (roughly two recent runs' worth) — without the bound the file grew unbounded at every scheduled run, reaching 16 MB and 3,593 messages within a month and making every `AddMessage` rewrite linearly more expensive; the canonical Patrol forensic log is the `PatrolRunRecord` history surfaced at `/api/ai/patrol/runs`, not the chat-session-shaped file 2. Keep AI runtime and shared API proof routing aligned in `registry.json` 3. Preserve explicit coverage for chat, Patrol, remediation, and cost-control behavior when AI runtime changes. Interactive Assistant and Patrol tool selection must remain model-owned: Pulse may provide governed context, tools, approval state, resource-resolution facts, safety policy, and neutral resource-scoped action history, but it must not add prompt-keyword routers, expected-tool retries, auto-recovery tool calls, keyword-matched prior-fix suggestions, or Pulse-authored remediation/finding fallbacks that choose the next investigative or corrective action for the model. Assistant FSM gates remain safety boundaries after the model chooses a tool: diff --git a/internal/ai/patrol.go b/internal/ai/patrol.go index 3f23737c6..dc717285e 100644 --- a/internal/ai/patrol.go +++ b/internal/ai/patrol.go @@ -36,6 +36,7 @@ package ai import ( + "context" "encoding/json" "strings" "sync" @@ -453,6 +454,11 @@ type PatrolService struct { incidentStore *memory.IncidentStore // For incident timeline capture alertResolver AlertResolver // For AI-based alert resolution + // verifyFixResolvedFn is the deterministic-verification seam used by the + // stale-finding reconcile pass. Tests inject a stub; nil means use the + // real VerifyFixResolved. + verifyFixResolvedFn func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) + // Unified resource provider — reads physical disks, Ceph, etc. from canonical model unifiedResourceProvider UnifiedResourceProvider diff --git a/internal/ai/patrol_ai.go b/internal/ai/patrol_ai.go index 35c8d1087..ee4c8f56b 100644 --- a/internal/ai/patrol_ai.go +++ b/internal/ai/patrol_ai.go @@ -3547,6 +3547,35 @@ func seedFormatTimeAgo(now, t time.Time) string { // a security vulnerability does not mean the issue has been addressed, and silent // auto-resolve there produced bogus auto_resolved → re-detected → regressed // cycles that inflated the trust strip and the regression counter. +// +// maxVerifiedStaleResolvesPerRun bounds how many deterministic verifications +// one reconcile pass may run. Verifications hit live state (and for +// smart-failure/backup-failed, governed tool calls), so a pathological store +// must not turn the end-of-run reconcile into a verification storm. Deferred +// candidates are logged and retried on the next successful full patrol. +const maxVerifiedStaleResolvesPerRun = 3 + +// verifiedStaleResolveReason is the resolve reason for event/persistent +// findings cleared by an affirmative deterministic verification (as opposed +// to the absence-based "No longer detected by patrol" used for +// performance/capacity findings). +const verifiedStaleResolveReason = "Deterministic verification confirmed the issue cleared" + +// verifyStaleFindingResolved runs the deterministic verifier for a finding's +// key with the same timeout the LLM-resolve gate uses. The injectable seam +// (verifyFixResolvedFn) exists for tests; production uses VerifyFixResolved. +func (p *PatrolService) verifyStaleFindingResolved(finding *Finding) (bool, error) { + p.mu.RLock() + verify := p.verifyFixResolvedFn + p.mu.RUnlock() + if verify == nil { + verify = p.VerifyFixResolved + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + return verify(ctx, finding.ResourceID, finding.ResourceType, finding.Key, finding.ID) +} + func (p *PatrolService) reconcileStaleFindings(reportedIDs, resolvedIDs, seededFindingIDs []string, runHadErrors bool) int { if runHadErrors { return 0 @@ -3569,18 +3598,65 @@ func (p *PatrolService) reconcileStaleFindings(reportedIDs, resolvedIDs, seededF } count := 0 + verifyAttempts := 0 for _, id := range seededFindingIDs { if reported[id] || resolved[id] { continue } // Category gate: only continuous current-state categories are safe to // auto-resolve from absence. Event/persistent categories stay active - // until explicitly resolved. + // until explicitly resolved — unless a deterministic verifier can + // affirmatively confirm the underlying signal is gone. finding := p.findings.Get(id) if finding == nil { continue } if !CategorySupportsStaleAutoResolve(finding.Category) { + // Absence of a re-report is not evidence for these categories, + // but an affirmative deterministic verification is — the same + // standard the LLM-resolve gate demands before honoring a + // patrol_resolve_finding call. Without this pass, a fixed backup + // or recovered service stays an active finding forever unless + // the LLM happens to call resolve: the asymmetric half of the + // "Backup failed" flap. Resolve only on a positive "signal gone" + // verification; on "still present" or inconclusive, fail closed + // and leave the finding active. + if p.hasDeterministicVerifierForKey(finding.Key) { + if verifyAttempts >= maxVerifiedStaleResolvesPerRun { + log.Info(). + Str("finding_id", id). + Str("key", finding.Key). + Msg("AI Patrol: deferring verified stale-finding check — per-run verification cap reached, will retry next run") + continue + } + verifyAttempts++ + verified, verifyErr := p.verifyStaleFindingResolved(finding) + if verifyErr == nil && verified { + if ok := p.findings.ResolveWithReason(id, verifiedStaleResolveReason); ok { + count++ + p.mu.RLock() + resolveUnified := p.unifiedFindingResolver + p.mu.RUnlock() + if resolveUnified != nil { + resolveUnified(id) + } + log.Info(). + Str("finding_id", id). + Str("category", string(finding.Category)). + Str("key", finding.Key). + Msg("AI Patrol: Auto-resolved event/persistent finding — deterministic verification confirmed the issue cleared") + } + continue + } + log.Debug(). + Err(verifyErr). + Str("finding_id", id). + Str("category", string(finding.Category)). + Str("key", finding.Key). + Bool("still_present", verifyErr == nil && !verified). + Msg("AI Patrol: keeping event/persistent finding active — deterministic verification did not confirm resolution (fail closed)") + continue + } log.Debug(). Str("finding_id", id). Str("category", string(finding.Category)). diff --git a/internal/ai/patrol_findings.go b/internal/ai/patrol_findings.go index 2ce1a9351..dc5f6ca5e 100644 --- a/internal/ai/patrol_findings.go +++ b/internal/ai/patrol_findings.go @@ -938,6 +938,18 @@ func (a *patrolFindingCreatorAdapter) getResolvedIDs() []string { return result } +// findingKeyAliases maps directional synonyms the LLM plausibly assigns onto +// the canonical verifier vocabulary (the verifyFixDeterministically switch), +// so deduplication and deterministic verification meet on one key. Only +// unambiguous aliases belong here — node-offline is NOT guest-unreachable +// and pbs-job-failed is NOT backup-failed; mapping those would point the +// verifier at the wrong resource model. +var findingKeyAliases = map[string]string{ + "high-cpu": "cpu-high", + "high-memory": "memory-high", + "high-disk": "disk-high", +} + func normalizeFindingKey(key string) string { if key == "" { return "" @@ -954,7 +966,11 @@ func normalizeFindingKey(key string) string { b.WriteRune(r) } } - return strings.Trim(b.String(), "-") + normalized := strings.Trim(b.String(), "-") + if canonical, ok := findingKeyAliases[normalized]; ok { + return canonical + } + return normalized } // recoverStuckInvestigations detects findings stuck in "running" state for longer than @@ -1365,13 +1381,18 @@ func (p *PatrolService) verifyFixDeterministically( } // hasDeterministicVerifierForKey reports whether a deterministic -// verifier exists for the given finding key. Used by ResolveFinding to -// decide whether an LLM-driven resolve attempt on an event/persistent -// category finding can be grounded in deterministic evidence. Keep this -// list aligned with the switch in verifyFixDeterministically. +// verifier exists for the given finding key. It is the single source of +// truth consulted by both the LLM-resolve gate (ResolveFinding) and the +// verified stale-finding reconcile pass (reconcileStaleFindings), and it +// must stay aligned with the dispatch switch in verifyFixDeterministically. +// It previously listed only smart-failure and backup-failed despite the +// dispatch handling seven keys, which made the resolve gate silently skip +// verification that existed (trust-the-LLM fallback) for backup-stale and +// guest-unreachable findings. func (p *PatrolService) hasDeterministicVerifierForKey(key string) bool { - switch key { - case "smart-failure", "backup-failed": + switch normalizeFindingKey(key) { + case "backup-stale", "cpu-high", "memory-high", "disk-high", + "guest-unreachable", "smart-failure", "backup-failed": return true default: return false diff --git a/internal/ai/patrol_findings_additional_test.go b/internal/ai/patrol_findings_additional_test.go index 6c59f4530..e2ccf8215 100644 --- a/internal/ai/patrol_findings_additional_test.go +++ b/internal/ai/patrol_findings_additional_test.go @@ -1175,16 +1175,22 @@ func TestPatrolService_GetAllFindingsIncludingResolved_IncludesResolvedAndDismis func TestPatrolService_HasDeterministicVerifierForKey(t *testing.T) { ps := NewPatrolService(nil, nil) - // Keys with deterministic verifiers (mirrors the switch in - // verifyFixDeterministically). Keep this test in lockstep with that - // switch — the gate in ResolveFinding depends on the two staying - // aligned. - for _, key := range []string{"smart-failure", "backup-failed"} { + // Keys with deterministic verifiers (mirrors the FULL dispatch switch in + // verifyFixDeterministically — hasDeterministicVerifierForKey is the + // single source of truth for both the LLM-resolve gate and the verified + // stale-finding reconcile pass; it previously listed only the two + // tool-based keys and silently skipped verification that existed for + // the state-only verifiers). Alias keys normalize onto canonical keys. + for _, key := range []string{ + "backup-stale", "cpu-high", "memory-high", "disk-high", + "guest-unreachable", "smart-failure", "backup-failed", + "high-cpu", "high-memory", "high-disk", + } { if !ps.hasDeterministicVerifierForKey(key) { t.Errorf("hasDeterministicVerifierForKey(%q) = false, want true", key) } } - for _, key := range []string{"", "unknown-key", "cpu-high", "node-offline", "service-restart"} { + for _, key := range []string{"", "unknown-key", "node-offline", "service-restart", "pbs-job-failed", "restart-loop"} { if ps.hasDeterministicVerifierForKey(key) { t.Errorf("hasDeterministicVerifierForKey(%q) = true, want false", key) } diff --git a/internal/ai/patrol_integration_test.go b/internal/ai/patrol_integration_test.go index 860a3cce2..c0a695860 100644 --- a/internal/ai/patrol_integration_test.go +++ b/internal/ai/patrol_integration_test.go @@ -138,17 +138,19 @@ func TestIntegration_PatrolRunCreatesFindings(t *testing.T) { time.Sleep(20 * time.Millisecond) } - // Verify finding was created + // Verify finding was created. The LLM stub reports key "high-cpu", + // which normalizes onto the canonical verifier vocabulary ("cpu-high") + // at creation so dedup and deterministic verification share one key. allFindings := ps.findings.GetActive(FindingSeverityInfo) found := false for _, f := range allFindings { - if f.ResourceID == "vm-100" && f.Key == "high-cpu" { + if f.ResourceID == "vm-100" && f.Key == "cpu-high" { found = true break } } if !found { - t.Fatalf("expected finding for vm-100 with key 'high-cpu', got %d active findings", len(allFindings)) + t.Fatalf("expected finding for vm-100 with key 'cpu-high', got %d active findings", len(allFindings)) } // Verify investigation was triggered (check with timeout due to async goroutine) diff --git a/internal/ai/patrol_reconcile_test.go b/internal/ai/patrol_reconcile_test.go index 4fa46b009..ea91c14c4 100644 --- a/internal/ai/patrol_reconcile_test.go +++ b/internal/ai/patrol_reconcile_test.go @@ -1,6 +1,8 @@ package ai import ( + "context" + "fmt" "testing" "time" ) @@ -284,3 +286,178 @@ func TestCategorySupportsStaleAutoResolve(t *testing.T) { }) } } + +// --- Verified auto-clear of event/persistent findings --------------------- +// +// Event/persistent categories must not auto-resolve from absence, but an +// affirmative deterministic verification ("the failure signal is gone") is +// stronger evidence than absence — the same standard the LLM-resolve gate +// demands. These tests pin the asymmetric half of the "Backup failed" flap +// fix: a fixed issue clears once the verifier confirms it, and anything +// short of an affirmative confirmation fails closed. + +func newVerifierFinding(id, key string) *Finding { + return &Finding{ + ID: id, + Key: key, + Severity: FindingSeverityWarning, + Category: FindingCategoryBackup, + ResourceID: "vm-100", + ResourceName: "web", + Title: "Backup failed", + DetectedAt: time.Now().Add(-1 * time.Hour), + } +} + +func TestReconcileStaleFindings_VerifiedAutoClearResolvesFixedFinding(t *testing.T) { + f := newVerifierFinding("find-backup", "backup-failed") + p := newTestPatrolWithFindings([]*Finding{f}) + p.verifyFixResolvedFn = func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) { + if resourceID != "vm-100" || findingKey != "backup-failed" || findingID != "find-backup" { + t.Fatalf("verifier called with wrong identity: %s/%s/%s", resourceID, findingKey, findingID) + } + return true, nil // signal affirmatively gone + } + + resolved := p.reconcileStaleFindings(nil, nil, []string{f.ID}, false) + + if resolved != 1 { + t.Fatalf("expected 1 verified auto-resolve, got %d", resolved) + } + stored := p.findings.Get(f.ID) + if stored == nil || stored.ResolvedAt == nil { + t.Fatal("finding should be resolved after affirmative verification") + } + if !stored.AutoResolved { + t.Fatal("verified clear should be marked auto-resolved") + } + if stored.ResolveReason != verifiedStaleResolveReason { + t.Fatalf("unexpected resolve reason: %s", stored.ResolveReason) + } +} + +func TestReconcileStaleFindings_VerifierStillDetectsKeepsActive(t *testing.T) { + f := newVerifierFinding("find-backup", "backup-failed") + p := newTestPatrolWithFindings([]*Finding{f}) + p.verifyFixResolvedFn = func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) { + return false, nil // failure signal still present + } + + if resolved := p.reconcileStaleFindings(nil, nil, []string{f.ID}, false); resolved != 0 { + t.Fatalf("expected 0 resolves while signal still present, got %d", resolved) + } + if stored := p.findings.Get(f.ID); stored == nil || stored.ResolvedAt != nil { + t.Fatal("finding must stay active while the failure signal is still present") + } +} + +func TestReconcileStaleFindings_VerifierInconclusiveFailsClosed(t *testing.T) { + f := newVerifierFinding("find-backup", "backup-failed") + p := newTestPatrolWithFindings([]*Finding{f}) + p.verifyFixResolvedFn = func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) { + return false, context.DeadlineExceeded // inconclusive + } + + if resolved := p.reconcileStaleFindings(nil, nil, []string{f.ID}, false); resolved != 0 { + t.Fatalf("expected 0 resolves on inconclusive verification, got %d", resolved) + } + if stored := p.findings.Get(f.ID); stored == nil || stored.ResolvedAt != nil { + t.Fatal("finding must stay active when verification is inconclusive (fail closed)") + } +} + +func TestReconcileStaleFindings_NoVerifierEventFindingStaysActive(t *testing.T) { + // pbs-job-failed has no deterministic verifier and must not be mapped + // onto backup-failed; the finding stays active until LLM/operator resolve. + f := newVerifierFinding("find-pbs", "pbs-job-failed") + p := newTestPatrolWithFindings([]*Finding{f}) + p.verifyFixResolvedFn = func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) { + t.Fatal("verifier must not be called for keys without a deterministic verifier") + return false, nil + } + + if resolved := p.reconcileStaleFindings(nil, nil, []string{f.ID}, false); resolved != 0 { + t.Fatalf("expected 0 resolves for verifier-less event finding, got %d", resolved) + } + if stored := p.findings.Get(f.ID); stored == nil || stored.ResolvedAt != nil { + t.Fatal("verifier-less event finding must stay active") + } +} + +func TestReconcileStaleFindings_VerifiedAutoClearIsIdempotent(t *testing.T) { + // The no-noise invariant: repeating reconcile over unchanged state must + // not produce new resolves, lifecycle events, or resolve/re-detect cycles. + f := newVerifierFinding("find-backup", "backup-failed") + p := newTestPatrolWithFindings([]*Finding{f}) + p.verifyFixResolvedFn = func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) { + return true, nil + } + + if first := p.reconcileStaleFindings(nil, nil, []string{f.ID}, false); first != 1 { + t.Fatalf("expected first pass to resolve 1, got %d", first) + } + stored := p.findings.Get(f.ID) + resolvedAt := *stored.ResolvedAt + lifecycleLen := len(stored.Lifecycle) + + if second := p.reconcileStaleFindings(nil, nil, []string{f.ID}, false); second != 0 { + t.Fatalf("expected second pass over unchanged state to resolve 0, got %d", second) + } + stored = p.findings.Get(f.ID) + if stored.ResolvedAt == nil || !stored.ResolvedAt.Equal(resolvedAt) { + t.Fatal("second pass must not touch the resolution") + } + if len(stored.Lifecycle) != lifecycleLen { + t.Fatalf("second pass must not append lifecycle events: %d -> %d", lifecycleLen, len(stored.Lifecycle)) + } +} + +func TestReconcileStaleFindings_VerificationCapDefersExcessCandidates(t *testing.T) { + findings := []*Finding{ + newVerifierFinding("find-1", "backup-failed"), + newVerifierFinding("find-2", "backup-failed"), + newVerifierFinding("find-3", "backup-failed"), + newVerifierFinding("find-4", "backup-failed"), + } + // Distinct resources so the store keeps all four. + for i, f := range findings { + f.ResourceID = fmt.Sprintf("vm-%d", 100+i) + } + p := newTestPatrolWithFindings(findings) + calls := 0 + p.verifyFixResolvedFn = func(ctx context.Context, resourceID, resourceType, findingKey, findingID string) (bool, error) { + calls++ + return true, nil + } + + ids := []string{"find-1", "find-2", "find-3", "find-4"} + resolved := p.reconcileStaleFindings(nil, nil, ids, false) + + if calls != maxVerifiedStaleResolvesPerRun { + t.Fatalf("expected verification calls capped at %d, got %d", maxVerifiedStaleResolvesPerRun, calls) + } + if resolved != maxVerifiedStaleResolvesPerRun { + t.Fatalf("expected %d resolves this run, got %d", maxVerifiedStaleResolvesPerRun, resolved) + } + deferred := p.findings.Get("find-4") + if deferred == nil || deferred.ResolvedAt != nil { + t.Fatal("candidate beyond the cap must stay active and retry next run") + } +} + +func TestNormalizeFindingKey_CanonicalAliases(t *testing.T) { + cases := map[string]string{ + "high-cpu": "cpu-high", + "High_Memory": "memory-high", + "high disk": "disk-high", + "cpu-high": "cpu-high", + "backup-stale": "backup-stale", + // Non-aliased keys pass through normalization unchanged. + "pbs-job-failed": "pbs-job-failed", + } + for in, want := range cases { + if got := normalizeFindingKey(in); got != want { + t.Fatalf("normalizeFindingKey(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/ai/patrol_test.go b/internal/ai/patrol_test.go index 3bd7d6cef..a84569227 100644 --- a/internal/ai/patrol_test.go +++ b/internal/ai/patrol_test.go @@ -1164,9 +1164,11 @@ func TestNormalizeFindingKey(t *testing.T) { expected: "cpu100warning", }, { + // Trim applies before alias lookup, so the trimmed key still + // canonicalizes onto the verifier vocabulary (high-cpu → cpu-high). name: "leading/trailing whitespace", input: " high-cpu ", - expected: "high-cpu", + expected: "cpu-high", }, { name: "with numbers", @@ -1176,7 +1178,7 @@ func TestNormalizeFindingKey(t *testing.T) { { name: "leading/trailing dashes trimmed", input: "-high-cpu-", - expected: "high-cpu", + expected: "cpu-high", }, } diff --git a/internal/ai/tools/tools_patrol.go b/internal/ai/tools/tools_patrol.go index 74e55209c..3787846fc 100644 --- a/internal/ai/tools/tools_patrol.go +++ b/internal/ai/tools/tools_patrol.go @@ -25,7 +25,7 @@ Returns: {"ok": true, "finding_id": "...", "is_new": true/false} on success.`, Properties: map[string]PropertySchema{ "key": { Type: "string", - Description: "Stable issue key for deduplication (e.g. high-cpu, high-memory, high-disk, backup-stale, backup-never, storage-high-usage, node-offline, restart-loop, pbs-job-failed)", + Description: "Stable issue key for deduplication. Use the canonical key when one fits — cpu-high, memory-high, disk-high, backup-stale, backup-failed, smart-failure, guest-unreachable (these have deterministic verifiers that ground resolution) — otherwise a stable kebab-case key like backup-never, storage-high-usage, node-offline, restart-loop, pbs-job-failed.", }, "severity": { Type: "string",