From 91737ee207efcacde8d30f076f5f8cfa5d18b11b Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sun, 16 Aug 2026 18:38:58 +0100 Subject: [PATCH] Canonicalize Patrol health finding keys --- .../v6/internal/subsystems/ai-runtime.md | 5 ++++ internal/ai/patrol_findings.go | 14 +++++++-- .../ai/patrol_findings_additional_test.go | 30 +++++++++++++++++++ internal/ai/patrol_reconcile_test.go | 16 ++++++---- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index bebc719ac..3a0284807 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -3550,6 +3550,11 @@ resolve canonical/source IDs and unique aliases before collection, reject verification lifecycle. 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 preflight-runtime-recovery contract where a successful Patrol preflight with an observed tool call resolves the synthetic Patrol runtime failure finding while failed or no-tool-call preflights leave it active, 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 + Container-health finding variants such as `container-health-failing`, + `container-unhealthy`, and `failing-health-check` must normalize to the + canonical `health-check-failed` key before finding identity is generated. + Existing-finding reconfirmation therefore converges on one durable issue + even when the model changes an unambiguous key spelling between runs. 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_findings.go b/internal/ai/patrol_findings.go index 049e70237..b75c6752a 100644 --- a/internal/ai/patrol_findings.go +++ b/internal/ai/patrol_findings.go @@ -1346,9 +1346,17 @@ func (p *PatrolService) dispatchPatrolInvestigations(result *AIAnalysisResult) { // 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", + "container-health-check-failed": "health-check-failed", + "container-health-failed": "health-check-failed", + "container-health-failing": "health-check-failed", + "container-unhealthy": "health-check-failed", + "failing-health-check": "health-check-failed", + "health-check-failing": "health-check-failed", + "healthcheck-failed": "health-check-failed", + "high-cpu": "cpu-high", + "high-memory": "memory-high", + "high-disk": "disk-high", + "unhealthy-container": "health-check-failed", } func normalizeFindingKey(key string) string { diff --git a/internal/ai/patrol_findings_additional_test.go b/internal/ai/patrol_findings_additional_test.go index 144b9b7af..2457d0038 100644 --- a/internal/ai/patrol_findings_additional_test.go +++ b/internal/ai/patrol_findings_additional_test.go @@ -1718,6 +1718,36 @@ func TestPatrolFindingAdapterDistinguishesSameRunCreationFromExistingRereport(t } } +func TestPatrolFindingAdapterCanonicalizesContainerHealthKeyVariantsAcrossRuns(t *testing.T) { + ps := NewPatrolService(nil, nil) + firstInput := tools.PatrolFindingInput{ + ResourceID: "app-container-1", ResourceName: "api", ResourceType: "app-container", + Key: "health-check-failed", Severity: "warning", Category: "reliability", + Title: "Container health check is failing", Description: "The container is unhealthy.", + Impact: "Requests may fail.", Recommendation: "Inspect the health check.", + Evidence: "Current provider health is unhealthy.", + } + + firstRun := newPatrolFindingCreatorAdapterState(ps, patrolRuntimeState{}) + findingID, isNew, err := firstRun.CreateFinding(firstInput) + if err != nil || !isNew { + t.Fatalf("first report = (%q, %t, %v), want a new finding", findingID, isNew, err) + } + + secondInput := firstInput + secondInput.Key = "container-health-failing" + secondInput.Title = "Container health is failing while running" + secondRun := newPatrolFindingCreatorAdapterState(ps, patrolRuntimeState{}) + secondRun.GetActiveFindings("", "") + reportedID, secondIsNew, err := secondRun.CreateFinding(secondInput) + if err != nil || secondIsNew || reportedID != findingID { + t.Fatalf("variant report = (%q, %t, %v), want existing %q", reportedID, secondIsNew, err, findingID) + } + if active := ps.findings.GetActive(FindingSeverityInfo); len(active) != 1 || active[0].ID != findingID || active[0].Key != "health-check-failed" { + t.Fatalf("active findings = %+v, want one canonical health-check finding", active) + } +} + func TestPatrolFindingAdapterTreatsStoppedContainerStateAsAlertOwned(t *testing.T) { ps := NewPatrolService(nil, nil) state := newPatrolRuntimeState(models.StateSnapshot{ diff --git a/internal/ai/patrol_reconcile_test.go b/internal/ai/patrol_reconcile_test.go index 92e5cdd57..dee1cebda 100644 --- a/internal/ai/patrol_reconcile_test.go +++ b/internal/ai/patrol_reconcile_test.go @@ -448,11 +448,17 @@ func TestReconcileStaleFindings_VerificationCapDefersExcessCandidates(t *testing 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", + "high-cpu": "cpu-high", + "High_Memory": "memory-high", + "high disk": "disk-high", + "cpu-high": "cpu-high", + "backup-stale": "backup-stale", + "container-health-failing": "health-check-failed", + "container-health-check-failed": "health-check-failed", + "healthcheck-failed": "health-check-failed", + "failing-health-check": "health-check-failed", + "container-unhealthy": "health-check-failed", + "unhealthy-container": "health-check-failed", // Non-aliased keys pass through normalization unchanged. "pbs-job-failed": "pbs-job-failed", }