From fb293169f71103062ef8a9760d866cca24eb5053 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 9 May 2026 10:19:28 +0100 Subject: [PATCH] Make will_fix_later an operational commitment, not silent shut-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this change, all three dismissal reasons funnelled through the same "swallow re-detection at same/lower severity" path in FindingsStore.Add, so will_fix_later was functionally identical to expected_behavior — Pulse stayed quiet forever despite the dismiss_finding tool literally telling the LLM "Pulse Patrol will continue to monitor this issue." Now will_fix_later sets Finding.RemindAt (default 7 days) at dismissal time. Once RemindAt has passed, the next re-detection clears the dismissal and emits a `reminded` lifecycle event so the operator sees their lapsed commitment instead of a swallowed finding. expected_behavior keeps acknowledged-forever semantics; not_an_issue keeps Suppressed=true. The LLM tool response now surfaces the remind-at date so Patrol's conversational explanations stay aligned with the contract. --- .../v6/internal/subsystems/ai-runtime.md | 11 ++ .../subsystems/patrol-intelligence.md | 12 ++ internal/ai/findings.go | 78 +++++++- internal/ai/findings_test.go | 183 ++++++++++++++++++ internal/ai/service.go | 3 +- 5 files changed, 277 insertions(+), 10 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index 479f5dbd5..7544ab490 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -1534,3 +1534,14 @@ The open-source/free `PUT /api/ai/patrol/autonomy` adapter may persist findings-only `monitor` configuration and the governed investigation budget / timeout clamps, but it must continue to reject `approval`, `assisted`, and `full` autonomy with the canonical safe-remediation license response. +The same canonical findings store owns dismissal-reason semantics. The three +`dismissed_reason` values must remain behaviorally distinct, not copy-only +variants: `not_an_issue` flips `Suppressed=true`, `expected_behavior` +acknowledges without escalation, and `will_fix_later` is an operator +commitment that populates `Finding.RemindAt` (default +`DefaultWillFixLaterRemindAfter`, 7 days). On re-detection, the canonical +store wakes a `will_fix_later` finding once `RemindAt` has passed by +clearing the dismissal and emitting a `reminded` lifecycle event, and the +`dismiss_finding` LLM tool response must communicate the remind-at date so +Patrol's conversational explanations stay aligned with the persisted +behavior. diff --git a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md index 4633ec0e2..92dceed37 100644 --- a/docs/release-control/v6/internal/subsystems/patrol-intelligence.md +++ b/docs/release-control/v6/internal/subsystems/patrol-intelligence.md @@ -1075,3 +1075,15 @@ on shared helpers. Findings shells may link or format from feature-owned presentation helpers, but Patrol runtime severity, title cleanup, and primary settings actions must stay keyed to the canonical Patrol service identity instead of reimplementing those branches in links-only or leaf badge surfaces. +Finding dismissal reasons now carry distinct operational semantics, not just +copy variants. `not_an_issue` permanently suppresses (Suppressed=true); +`expected_behavior` acknowledges the finding forever without escalation; +`will_fix_later` is a real operational commitment — `FindingsStore.Dismiss` +populates `Finding.RemindAt` (default `DefaultWillFixLaterRemindAfter`, 7 +days), and the next re-detection after `RemindAt` clears the dismissal and +records a `reminded` lifecycle event so the operator sees their commitment +lapsed instead of the finding being silently swallowed forever. Severity +escalation still wakes any dismissed finding regardless of reason. The +`dismiss_finding` LLM tool response surfaces the remind-at date in plain +language so Patrol's own conversational explanations stay aligned with this +contract. diff --git a/internal/ai/findings.go b/internal/ai/findings.go index 852fe05ae..67c58734a 100644 --- a/internal/ai/findings.go +++ b/internal/ai/findings.go @@ -45,6 +45,17 @@ const ( FindingCategoryGeneral FindingCategory = "general" ) +// DismissReasonWillFixLater marks a finding as "I will fix this later" — an +// operational commitment with an implicit deadline. See Finding.RemindAt. +const DismissReasonWillFixLater = "will_fix_later" + +// DefaultWillFixLaterRemindAfter is how long Pulse waits before nagging the +// operator about an open will_fix_later finding. This makes the dismissal a +// real commitment rather than a silent shut-up: if the finding still trips +// after this window, it surfaces again with a "reminded" lifecycle event +// instead of being swallowed forever. +const DefaultWillFixLaterRemindAfter = 7 * 24 * time.Hour + // FindingLoopState represents the current stage of the patrol detect/remediate loop. type FindingLoopState string @@ -140,6 +151,13 @@ type Finding struct { UserNote string `json:"user_note,omitempty"` // Freeform user explanation, included in LLM context TimesRaised int `json:"times_raised"` // How many times this finding has been detected Suppressed bool `json:"suppressed"` // Permanently suppress similar findings for this resource + // RemindAt turns "will_fix_later" into a real operational commitment. + // When set, the dismissal stays quiet until this timestamp passes; after + // it passes, the next re-detection clears the dismissal and surfaces the + // finding again with a "reminded" lifecycle event so the operator sees + // "you said you'd fix this on , it's still happening." Only + // populated for reason == "will_fix_later". + RemindAt *time.Time `json:"remind_at,omitempty"` // Investigation fields - tracks autonomous AI investigation of findings InvestigationSessionID string `json:"investigation_session_id,omitempty"` // Chat session ID if being investigated @@ -183,6 +201,7 @@ type findingJSON struct { UserNote string `json:"user_note,omitempty"` TimesRaised int `json:"times_raised"` Suppressed bool `json:"suppressed"` + RemindAt *time.Time `json:"remind_at,omitempty"` InvestigationSessionID string `json:"investigation_session_id,omitempty"` InvestigationStatus string `json:"investigation_status,omitempty"` InvestigationOutcome string `json:"investigation_outcome,omitempty"` @@ -226,6 +245,7 @@ func (f Finding) MarshalJSON() ([]byte, error) { UserNote: f.UserNote, TimesRaised: f.TimesRaised, Suppressed: f.Suppressed, + RemindAt: f.RemindAt, InvestigationSessionID: f.InvestigationSessionID, InvestigationStatus: f.InvestigationStatus, InvestigationOutcome: f.InvestigationOutcome, @@ -274,6 +294,7 @@ func (f *Finding) UnmarshalJSON(data []byte) error { UserNote: payload.UserNote, TimesRaised: payload.TimesRaised, Suppressed: payload.Suppressed, + RemindAt: payload.RemindAt, InvestigationSessionID: payload.InvestigationSessionID, InvestigationStatus: payload.InvestigationStatus, InvestigationOutcome: payload.InvestigationOutcome, @@ -967,7 +988,8 @@ func (s *FindingsStore) Add(f *Finding) bool { } } - // Check if dismissed or suppressed - only update if severity has escalated + // Check if dismissed or suppressed - only update if severity has escalated, + // or if a "will_fix_later" remind-at deadline has passed. if existing.DismissedReason != "" || existing.Suppressed { severityOrder := map[FindingSeverity]int{ FindingSeverityInfo: 0, @@ -975,8 +997,17 @@ func (s *FindingsStore) Add(f *Finding) bool { FindingSeverityWarning: 2, FindingSeverityCritical: 3, } - // If new severity is same or lower, don't reactivate - if severityOrder[f.Severity] <= severityOrder[existing.Severity] { + // "will_fix_later" with a passed RemindAt is a real operator + // commitment that has expired — surface the finding again. This + // is the only case where a same-or-lower-severity re-detection + // reactivates a dismissed finding; "expected_behavior" stays + // quiet, "not_an_issue" stays suppressed. + willFixReminderDue := existing.DismissedReason == DismissReasonWillFixLater && + existing.RemindAt != nil && + time.Now().After(*existing.RemindAt) && + !existing.Suppressed + if !willFixReminderDue && severityOrder[f.Severity] <= severityOrder[existing.Severity] { + // New severity is same or lower and there's no reminder due — don't reactivate. existing.LastSeenAt = time.Now() existing.TimesRaised++ s.appendLifecycleLocked(existing, "seen_while_suppressed", "Re-detected while dismissed/suppressed with non-escalated severity", existing.LoopState, existing.LoopState, nil) @@ -984,10 +1015,23 @@ func (s *FindingsStore) Add(f *Finding) bool { s.scheduleSave() return false } - // Severity escalated - clear dismissal/suppression and reactivate + if willFixReminderDue { + prevRemindAt := existing.RemindAt + meta := map[string]string{"reason_was": existing.DismissedReason} + if prevRemindAt != nil { + meta["remind_at"] = prevRemindAt.Format(time.RFC3339) + } + s.appendLifecycleLocked(existing, "reminded", "will_fix_later remind-at deadline passed; re-surfacing finding", string(FindingLoopStateDismissed), string(FindingLoopStateDetected), meta) + } + // Severity escalated or remind-at fired - clear dismissal/suppression and reactivate. existing.DismissedReason = "" existing.Suppressed = false - existing.UserNote = "" // Clear note since situation changed + existing.RemindAt = nil + if !willFixReminderDue { + // On severity escalation we drop the operator's note (situation changed); + // on a remind-at wake we keep it because the operator's promise is still relevant context. + existing.UserNote = "" + } existing.AcknowledgedAt = nil } @@ -1236,20 +1280,35 @@ func (s *FindingsStore) Dismiss(id, reason, note string) bool { // Mark as acknowledged for all dismiss reasons now := time.Now() f.AcknowledgedAt = &now - s.appendLifecycleLocked(f, "dismissed", "", f.LoopState, string(FindingLoopStateDismissed), map[string]string{ - "reason": reason, - }) + dismissMeta := map[string]string{"reason": reason} // Only "not_an_issue" creates permanent suppression // This is for true false positives where the detection logic is wrong if reason == "not_an_issue" { f.Suppressed = true + f.RemindAt = nil } + // "will_fix_later" is an operational commitment, not a permanent silence. + // Default a 7-day remind-at so the next re-detection after the window + // surfaces the finding with a "reminded" lifecycle event instead of being + // swallowed forever like "expected_behavior". + if reason == DismissReasonWillFixLater { + remindAt := now.Add(DefaultWillFixLaterRemindAfter) + f.RemindAt = &remindAt + dismissMeta["remind_at"] = remindAt.Format(time.RFC3339) + } else if reason != "" { + f.RemindAt = nil + } + s.appendLifecycleLocked(f, "dismissed", "", f.LoopState, string(FindingLoopStateDismissed), dismissMeta) s.syncLoopStateLocked(f) - // For "expected_behavior" and "will_fix_later": + // For "expected_behavior": // - Finding stays visible (not suppressed, not snoozed) // - But is marked as dismissed/acknowledged so user knows they've reviewed it // - Severity escalation will clear DismissedReason and reactivate + // For "will_fix_later": + // - Same as expected_behavior, plus RemindAt is set; the next re-detection + // after RemindAt clears the dismissal and the finding surfaces again + // with a "reminded" lifecycle event. s.mu.Unlock() s.scheduleSave() @@ -1278,6 +1337,7 @@ func (s *FindingsStore) Undismiss(id string) bool { f.DismissedReason = "" f.Suppressed = false f.AcknowledgedAt = nil + f.RemindAt = nil s.appendLifecycleLocked(f, "undismissed", "", string(FindingLoopStateDismissed), string(FindingLoopStateDetected), nil) // Keep UserNote in case user wants to see their notes diff --git a/internal/ai/findings_test.go b/internal/ai/findings_test.go index 60fab664a..2a639e184 100644 --- a/internal/ai/findings_test.go +++ b/internal/ai/findings_test.go @@ -1426,3 +1426,186 @@ func TestFindingsStore_Dismiss_DifferentReasons(t *testing.T) { } }) } + +func TestFindingsStore_Dismiss_WillFixLater_DefaultsRemindAt(t *testing.T) { + // will_fix_later is meant to be an operational commitment, not a silent + // shut-up. Dismiss must populate RemindAt so the finding can wake itself + // after the deadline; expected_behavior must NOT populate it (that's + // the "acknowledged forever" semantic), and not_an_issue must clear it + // (that's the "permanent suppression" semantic). + t.Run("will_fix_later sets RemindAt ~7 days out", func(t *testing.T) { + store := NewFindingsStore() + f := &Finding{ + ID: "f-wfl", + ResourceID: "res-1", + Severity: FindingSeverityWarning, + Title: "Disk pressure", + } + store.Add(f) + + before := time.Now() + store.Dismiss("f-wfl", DismissReasonWillFixLater, "Plan to upgrade Q3") + after := time.Now() + + got := store.Get("f-wfl") + if got.RemindAt == nil { + t.Fatal("Expected RemindAt to be set for will_fix_later") + } + expectedMin := before.Add(DefaultWillFixLaterRemindAfter) + expectedMax := after.Add(DefaultWillFixLaterRemindAfter) + if got.RemindAt.Before(expectedMin) || got.RemindAt.After(expectedMax) { + t.Errorf("RemindAt should be ~7 days out, got %v (expected window %v..%v)", got.RemindAt, expectedMin, expectedMax) + } + }) + + t.Run("expected_behavior does not set RemindAt", func(t *testing.T) { + store := NewFindingsStore() + store.Add(&Finding{ID: "f-eb", ResourceID: "res-2", Severity: FindingSeverityWarning}) + store.Dismiss("f-eb", "expected_behavior", "Known") + if got := store.Get("f-eb"); got.RemindAt != nil { + t.Errorf("expected_behavior must not set RemindAt; got %v", got.RemindAt) + } + }) + + t.Run("not_an_issue clears RemindAt", func(t *testing.T) { + store := NewFindingsStore() + store.Add(&Finding{ID: "f-nai", ResourceID: "res-3", Severity: FindingSeverityWarning}) + // Pre-stage a stale RemindAt to confirm not_an_issue clears it. + store.Dismiss("f-nai", DismissReasonWillFixLater, "later") + if store.Get("f-nai").RemindAt == nil { + t.Fatal("setup: RemindAt should have been set by will_fix_later") + } + store.Undismiss("f-nai") + store.Dismiss("f-nai", "not_an_issue", "false positive") + if got := store.Get("f-nai"); got.RemindAt != nil { + t.Errorf("not_an_issue must clear RemindAt; got %v", got.RemindAt) + } + }) + + t.Run("Undismiss clears RemindAt", func(t *testing.T) { + store := NewFindingsStore() + store.Add(&Finding{ID: "f-un", ResourceID: "res-4", Severity: FindingSeverityWarning}) + store.Dismiss("f-un", DismissReasonWillFixLater, "later") + store.Undismiss("f-un") + if got := store.Get("f-un"); got.RemindAt != nil { + t.Errorf("Undismiss must clear RemindAt; got %v", got.RemindAt) + } + }) +} + +func TestFindingsStore_Add_WillFixLater_StaysQuietBeforeRemindAt(t *testing.T) { + // While the will_fix_later remind-at is still in the future, re-detection + // at same severity must NOT reactivate the finding — the operator's + // commitment is still "honored" and Pulse stays quiet. + store := NewFindingsStore() + store.Add(&Finding{ + ID: "f-quiet", + ResourceID: "res-1", + Severity: FindingSeverityWarning, + Title: "Disk pressure", + }) + store.Dismiss("f-quiet", DismissReasonWillFixLater, "Q3 upgrade") + + // Re-detect at same severity while RemindAt is still in the future. + store.Add(&Finding{ + ID: "f-quiet", + ResourceID: "res-1", + Severity: FindingSeverityWarning, + Title: "Disk pressure (again)", + }) + + got := store.Get("f-quiet") + if !got.IsDismissed() { + t.Error("Finding must stay dismissed before RemindAt fires") + } + if got.DismissedReason != DismissReasonWillFixLater { + t.Errorf("DismissedReason should still be will_fix_later, got %s", got.DismissedReason) + } + if got.TimesRaised < 1 { + t.Error("TimesRaised must still increment to track recurrences while quiet") + } +} + +func TestFindingsStore_Add_WillFixLater_WakesAfterRemindAt(t *testing.T) { + // Once the will_fix_later remind-at has passed, the next re-detection + // must clear the dismissal and surface the finding again with a + // "reminded" lifecycle event so the operator sees their commitment lapsed. + store := NewFindingsStore() + store.Add(&Finding{ + ID: "f-wake", + ResourceID: "res-1", + Severity: FindingSeverityWarning, + Title: "Disk pressure", + }) + store.Dismiss("f-wake", DismissReasonWillFixLater, "Q3 upgrade") + + // Backdate RemindAt so it has already passed. + store.mu.Lock() + past := time.Now().Add(-1 * time.Hour) + store.findings["f-wake"].RemindAt = &past + store.mu.Unlock() + + // Re-detect at same severity — should now wake. + store.Add(&Finding{ + ID: "f-wake", + ResourceID: "res-1", + Severity: FindingSeverityWarning, + Title: "Disk pressure (still)", + }) + + got := store.Get("f-wake") + if got.IsDismissed() { + t.Error("Finding must be un-dismissed once RemindAt has passed") + } + if got.DismissedReason != "" { + t.Errorf("DismissedReason must be cleared after wake, got %s", got.DismissedReason) + } + if got.RemindAt != nil { + t.Errorf("RemindAt must be cleared after wake, got %v", got.RemindAt) + } + // UserNote should be preserved on remind-at wake (operator's promise is still relevant context). + if got.UserNote != "Q3 upgrade" { + t.Errorf("UserNote must be preserved across remind-at wake; got %q", got.UserNote) + } + // Lifecycle must record the "reminded" event so the operator sees their commitment lapsed. + foundReminded := false + for _, ev := range got.Lifecycle { + if ev.Type == "reminded" { + foundReminded = true + break + } + } + if !foundReminded { + t.Error("Lifecycle must contain a 'reminded' event after will_fix_later wake") + } +} + +func TestFinding_RemindAt_RoundTripsThroughJSON(t *testing.T) { + // RemindAt must persist across save/load so the operator's commitment + // survives a process restart. + when := time.Now().Add(48 * time.Hour).UTC().Truncate(time.Second) + original := Finding{ + ID: "f-json", + ResourceID: "res-1", + Severity: FindingSeverityWarning, + DismissedReason: DismissReasonWillFixLater, + RemindAt: &when, + } + bytes, err := original.MarshalJSON() + if err != nil { + t.Fatalf("MarshalJSON: %v", err) + } + if !strings.Contains(string(bytes), "\"remind_at\"") { + t.Errorf("MarshalJSON output should contain remind_at field, got %s", string(bytes)) + } + var roundTripped Finding + if err := roundTripped.UnmarshalJSON(bytes); err != nil { + t.Fatalf("UnmarshalJSON: %v", err) + } + if roundTripped.RemindAt == nil { + t.Fatal("RemindAt must round-trip through JSON") + } + if !roundTripped.RemindAt.Equal(when) { + t.Errorf("RemindAt round-trip mismatch: got %v want %v", roundTripped.RemindAt, when) + } +} diff --git a/internal/ai/service.go b/internal/ai/service.go index 4d0e5ad21..70a80212d 100644 --- a/internal/ai/service.go +++ b/internal/ai/service.go @@ -3383,7 +3383,8 @@ func (s *Service) executeTool(ctx context.Context, req ExecuteRequest, tc provid // Format a helpful response based on reason var resultMsg string if reason == "will_fix_later" { - resultMsg = fmt.Sprintf("Finding dismissed as '%s'. Pulse Patrol will continue to monitor this issue.\nID: %s\nNote: %s", reason, findingID, note) + remindAt := time.Now().Add(DefaultWillFixLaterRemindAfter).Format("2006-01-02") + resultMsg = fmt.Sprintf("Finding dismissed as '%s'. Pulse Patrol will stay quiet on this until %s; if the finding is still tripping after that date, it will surface again with a 'reminded' lifecycle event.\nID: %s\nNote: %s", reason, remindAt, findingID, note) } else { resultMsg = fmt.Sprintf("Finding dismissed as '%s' and suppression rule created. Similar findings for this resource will not be raised again.\nID: %s\nNote: %s", reason, findingID, note) }