diff --git a/internal/investigate/findings_contract_test.go b/internal/investigate/findings_contract_test.go new file mode 100644 index 00000000..22753a35 --- /dev/null +++ b/internal/investigate/findings_contract_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 + +package investigate + +import ( + "encoding/json" + "slices" + "strings" + "testing" + + "github.com/Smana/runlore/internal/providers" +) + +// TestActionWithoutRemedy pins the other half of the shape contract: a verdict that +// tells the on-call to act must ship something to act on. +// +// Live card, 2026-08-24 02:18: header "๐Ÿ›  Action suggested", body ending on +// "โ€ฆ1 more hypothesis below", and no next-steps section at all โ€” because +// suggested_action and actions are both optional in the schema, so the payload was +// legal. A card that promises an action and has none costs more trust than a card +// that says inconclusive. +func TestActionWithoutRemedy(t *testing.T) { + cause := func(action string) []providers.Hypothesis { + return []providers.Hypothesis{{Summary: "s", SuggestedAction: action}} + } + cases := []struct { + name string + inv providers.Investigation + want bool + }{ + {"action_suggested with no remedy anywhere", + providers.Investigation{Verdict: providers.VerdictActionSuggested, RootCauses: cause("")}, true}, + {"action_required with no remedy anywhere", + providers.Investigation{Verdict: providers.VerdictActionRequired, RootCauses: cause("")}, true}, + {"a suggested_action on the root cause accounts for it", + providers.Investigation{Verdict: providers.VerdictActionSuggested, RootCauses: cause("delete the stale Job")}, false}, + {"a proposed action accounts for it", + providers.Investigation{Verdict: providers.VerdictActionRequired, RootCauses: cause(""), + Actions: []providers.Action{{Description: "restore Drive access"}}}, false}, + {"no_action is not claiming an action", + providers.Investigation{Verdict: providers.VerdictNoAction, RootCauses: cause("")}, false}, + {"inconclusive is the other predicate's business", + providers.Investigation{Verdict: providers.VerdictInconclusive, RootCauses: cause("")}, false}, + {"an omitted verdict is a parse concern, not this one", + providers.Investigation{RootCauses: cause("")}, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := c.inv.ActionWithoutRemedy(); got != c.want { + t.Fatalf("ActionWithoutRemedy = %v, want %v", got, c.want) + } + }) + } +} + +// TestUnevidencedConclusion: a conclusive verdict whose leading root cause cites no +// evidence at all. Live card, 2026-08-22 22:53 โ€” "High confidence ยท 85%" with a Why +// paragraph and not one bullet under it. The prose may still be right, but nothing +// in the card lets a human check it, and nothing lets the verify pass trace it. +func TestUnevidencedConclusion(t *testing.T) { + cases := []struct { + name string + inv providers.Investigation + want bool + }{ + {"conclusive, leading cause cites nothing", + providers.Investigation{Verdict: providers.VerdictActionSuggested, + RootCauses: []providers.Hypothesis{{Summary: "stale Job never deleted"}}}, true}, + {"evidence on the leading cause accounts for it", + providers.Investigation{Verdict: providers.VerdictActionSuggested, + RootCauses: []providers.Hypothesis{{Summary: "s", Evidence: []string{"kube_job_failed=1"}}}}, false}, + {"no_action still has to show its work", + providers.Investigation{Verdict: providers.VerdictNoAction, + RootCauses: []providers.Hypothesis{{Summary: "self-healed"}}}, true}, + {"inconclusive is allowed to have nothing", + providers.Investigation{Verdict: providers.VerdictInconclusive, + RootCauses: []providers.Hypothesis{{Summary: "s"}}}, false}, + {"no root cause at all is the inconclusive predicate's business", + providers.Investigation{Verdict: providers.VerdictActionSuggested}, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := unevidencedConclusion(c.inv); got != c.want { + t.Fatalf("unevidencedConclusion = %v, want %v", got, c.want) + } + }) + } +} + +// TestSubmitFindingsRequiresEvidence guards the model-facing half of this fix, which +// has no compiler. The predicates above test Go functions; the schema is a JSON +// string literal, so a future edit that reflows or re-wraps that line can silently +// revert `evidence` to optional while every other test in this file keeps passing. +// Same reasoning as TestDataGapsForbidsSpeculation. +func TestSubmitFindingsRequiresEvidence(t *testing.T) { + var schema struct { + Properties struct { + RootCauses struct { + Items struct { + Properties map[string]struct { + MinItems int `json:"minItems"` + Description string `json:"description"` + } `json:"properties"` + Required []string `json:"required"` + } `json:"items"` + } `json:"root_causes"` + Verdict struct { + Description string `json:"description"` + } `json:"verdict"` + } `json:"properties"` + } + if err := json.Unmarshal([]byte(submitFindingsSpec().Schema), &schema); err != nil { + t.Fatalf("submit_findings schema is not valid JSON: %v", err) + } + rc := schema.Properties.RootCauses.Items + + if !slices.Contains(rc.Required, "evidence") { + t.Errorf("a root cause must REQUIRE evidence; required = %v", rc.Required) + } + if got := rc.Properties["evidence"].MinItems; got != 1 { + t.Errorf("evidence minItems = %d, want 1 โ€” an empty array satisfies a bare `required`", got) + } + // The conditional requirement that pairs a remedy with a verdict claiming one. + // ActionWithoutRemedy only WARNS, so this sentence is the only thing that stops + // the payload being produced in the first place. + if d := rc.Properties["suggested_action"].Description; !strings.Contains(d, "action_required") { + t.Errorf("suggested_action must say when it is required:\n%s", d) + } + // The converse of the inconclusive rule: high confidence in "I could not tell" + // is still inconclusive. Live 2026-08-24, an 85%% action_suggested whose finding + // was that it could not identify the failing resource. + if d := schema.Properties.Verdict.Description; !strings.Contains(d, "could not name the failing resource") { + t.Errorf("verdict must state the converse of the recurrence rule:\n%s", d) + } +} diff --git a/internal/investigate/loop.go b/internal/investigate/loop.go index bce23181..18e92f76 100644 --- a/internal/investigate/loop.go +++ b/internal/investigate/loop.go @@ -753,9 +753,19 @@ func (li *LoopInvestigator) Investigate(ctx context.Context, req Request) error // Say it out loud when the submission contradicts itself, BEFORE verify can // rewrite it: this is the model's own payload, and the mislabel that motivated // #471 was invisible until someone read the empty card it produced. + contractWarn := func(msg string) { + li.Log.Warn("submit_findings: "+msg, + "title", inv.Title, "trigger_key", req.TriggerKey, "verdict", inv.Verdict, + "confidence", inv.Confidence, "tools_used", used) + } if unaccountedInconclusive(inv) { - li.Log.Warn("submit_findings: verdict=inconclusive with no cause, no open question and no data gap โ€” the delivered card will have no Why and no next steps", - "title", inv.Title, "trigger_key", req.TriggerKey, "confidence", inv.Confidence, "tools_used", used) + contractWarn("verdict=inconclusive with no cause, no open question and no data gap โ€” the delivered card will have no Why and no next steps") + } + if inv.ActionWithoutRemedy() { + contractWarn("verdict claims an action but no suggested_action or action was supplied โ€” the card's header promises a remedy its body does not carry") + } + if unevidencedConclusion(inv) { + contractWarn("conclusive verdict whose leading root cause cites no evidence โ€” the confidence badge is backed by nothing the reader or the verify pass can trace") } if li.Metrics != nil { // Usage-anchored when the provider reported usage; heuristic otherwise. diff --git a/internal/investigate/tools.go b/internal/investigate/tools.go index dc4e5a4e..a89ee391 100644 --- a/internal/investigate/tools.go +++ b/internal/investigate/tools.go @@ -76,11 +76,15 @@ func submitFindingsSpec() providers.ToolSpec { "confidence":{"type":"number"}, "affected_resource":{"type":"object","description":"the workload your investigation identified as the failing/affected resource","properties":{"kind":{"type":"string"},"name":{"type":"string"},"namespace":{"type":"string"}}}, "root_causes":{"type":"array","items":{"type":"object","properties":{ -"summary":{"type":"string"},"confidence":{"type":"number","description":"how strongly the evidence supports THIS root cause, 0-1. Required: an omitted confidence is delivered as 0%, which reads to the on-call as 'no confidence' and buries a sound finding under a red badge"},"change_ref":{"type":"string"}, -"evidence":{"type":"array","items":{"type":"string"}},"suggested_action":{"type":"string"},"reversible":{"type":"boolean"}}, -"required":["summary","confidence"]}}, +"summary":{"type":"string"}, +"confidence":{"type":"number","description":"how strongly the evidence below supports THIS root cause, 0-1. It measures support for the cause you STATED, not how sure you are of the narrative around it. Required: an omitted confidence is delivered as 0%, which reads to the on-call as 'no confidence' and buries a sound finding under a red badge"}, +"change_ref":{"type":"string"}, +"evidence":{"type":"array","minItems":1,"items":{"type":"string"},"description":"REQUIRED, at least one: the specific tool results that support this cause - name the tool and quote the value, error or log line it returned. This is what lets a human check the cause and what the verify pass traces. A cause asserted with no evidence is delivered as a bare paragraph under a confidence badge nothing backs"}, +"suggested_action":{"type":"string","description":"what the human should DO about this cause. Required whenever the verdict is action_suggested or action_required, unless a top-level actions entry already covers it - a card whose verdict promises an action and then offers none is worse than one that admits it is inconclusive"}, +"reversible":{"type":"boolean"}}, +"required":["summary","confidence","evidence"]}}, "unresolved":{"type":"array","items":{"type":"string"},"description":"genuine open questions only a human can answer - put tool or data limitations in data_gaps instead"}, -"verdict":{"type":"string","enum":["no_action","action_suggested","action_required","inconclusive"],"description":"actionability for the on-call: no_action (benign/self-healed/synthetic), action_suggested (a human should follow the next steps), action_required (live impact needing prompt action), inconclusive (you genuinely could not determine the cause, and data_gaps/unresolved say what blocked you). A recurrence of a fault you DID identify is NOT inconclusive - naming a known cause again is still a conclusion, so restate it with the actionability verdict it deserves"}, +"verdict":{"type":"string","enum":["no_action","action_suggested","action_required","inconclusive"],"description":"actionability for the on-call: no_action (benign/self-healed/synthetic), action_suggested (a human should follow the next steps), action_required (live impact needing prompt action), inconclusive (you genuinely could not determine the cause, and data_gaps/unresolved say what blocked you). A recurrence of a fault you DID identify is NOT inconclusive - naming a known cause again is still a conclusion, so restate it with the actionability verdict it deserves. The converse also holds: if you could not name the failing resource or the cause THIS time, the verdict is inconclusive however sure you are of everything around it - do not report a high-confidence action_suggested whose actual finding is that you could not identify what failed"}, "ruled_out":{"type":"array","items":{"type":"string"},"description":"hypotheses you considered and REJECTED, one line each naming the disproving evidence"}, "data_gaps":{"type":"array","items":{"type":"string"},"description":"signals you could not obtain (tool errors, RBAC denials, truncated output) that limited the investigation - data limitations, NOT questions for a human. State the tool you called and the ACTUAL error it returned. Do NOT speculate about a cause you did not verify: if a metrics query returned nothing, call discover_metrics before claiming a metric or its name is absent; if a logs query returned nothing, call discover_log_fields; if a tool was refused, report the refusal rather than guessing why. An empty result is not evidence that the data does not exist."}, "actions":{"type":"array","description":"proposed remediations; prefer reversible, low-blast-radius","items":{"type":"object","properties":{ @@ -210,6 +214,33 @@ func unaccountedInconclusive(inv providers.Investigation) bool { return inv.UnaccountedInconclusive() } +// unevidencedConclusion reports whether a finding reaches a conclusive verdict while +// its leading root cause cites no evidence at all. +// +// Live on 2026-08-22: "High confidence ยท 85%" over a Why paragraph with not one +// bullet beneath it. The prose may well have been right, but nothing on the card let +// a human check it and nothing let the verify pass trace it โ€” the confidence badge +// was backed by the model's say-so alone. `inconclusive` is exempt: having nothing +// to show is what that verdict is for. So is an empty root-cause list, which is +// unaccountedInconclusive's business. +func unevidencedConclusion(inv providers.Investigation) bool { + // Conclusive() rather than "!= inconclusive": an omitted or unparseable verdict + // leaves inv.Verdict empty, which renders no badge at all, so there is no claim + // to hold to account. Testing it the other way warned about a badge never drawn. + if !inv.Verdict.Conclusive() || len(inv.RootCauses) == 0 { + return false + } + // Non-blank, because the schema's minItems:1 counts an empty string. Requiring a + // present-but-empty array to silence the warning would make the cheapest possible + // non-answer the way to pass, and the card then renders a bare bullet. + for _, e := range inv.RootCauses[0].Evidence { + if strings.TrimSpace(e) != "" { + return false + } + } + return true +} + // buildInvestigation maps the parsed findings shape onto a providers.Investigation, // clamping confidences. Shared by the direct and the tolerant parse paths. func buildInvestigation(f findings) providers.Investigation { diff --git a/internal/notify/format.go b/internal/notify/format.go index 9341c7f2..29ac01d8 100644 --- a/internal/notify/format.go +++ b/internal/notify/format.go @@ -176,6 +176,37 @@ func unaccountedForReader(inv providers.Investigation) bool { return inv.UnaccountedInconclusive() && len(inv.RuledOut) == 0 } +// noRemedyNotice is what a delivered notification says when its verdict tells the +// on-call to act and it has nothing to show them. Naming the gap keeps the +// actionability signal โ€” a human should still look โ€” without promising a remedy +// that does not exist, and lets the reader tell an ABSENT remedy from a failed +// render, which is what the live 2026-08-24 card could not do. +const noRemedyNotice = "*๐Ÿ›  No next steps proposed* โ€” the verdict says a human should act, but the " + + "investigation did not supply a remedy. Work from the analysis above and any data gaps in the thread." + +// remedyMissingForReader reports whether nothing the delivered notification can +// show accounts for a verdict that tells the on-call to act. +// +// It is the reader-side counterpart to providers.Investigation.ActionWithoutRemedy, +// split for the same reason as unaccountedForReader above. +// +// The difference is load-bearing, and the shipped card fixtures prove it. Both +// `seen_before` (a human-reviewed resolution quoted from the KB entry) and +// `matched_runbook` (a matched entry with its URL) carry an action verdict and no +// suggested_action of their own โ€” and both render a remedy the reader can follow, +// several blocks ABOVE where this notice would print. Announcing that the +// investigation supplied no remedy there would point the on-call away from an +// answer that is on screen. +func remedyMissingForReader(inv providers.Investigation) bool { + if !inv.ActionWithoutRemedy() { + return false + } + if p := inv.Prior; p != nil && strings.TrimSpace(p.Resolution) != "" { + return false + } + return inv.MatchedKnowledge == nil +} + // verdictBadge maps a model verdict to its emoji + human label. Empty/unknown // verdicts return ("", "") and are rendered nowhere โ€” never invent a verdict. func verdictBadge(v providers.Verdict) (emoji, label string) { @@ -323,6 +354,14 @@ func Format(inv providers.Investigation) string { if unaccountedForReader(inv) { b.WriteString(unaccountedInconclusiveNotice + "\n") } + // The action-verdict counterpart, and it renders HERE for the reason Format's doc + // comment gives: this message feeds Matrix, the webhook text and the CLI, and its + // ordering "must never diverge" from the Slack card in what it claims. Gating the + // Slack block and not this one would have left every non-Slack channel showing the + // 2026-08-24 defect unfixed. + if remedyMissingForReader(inv) { + b.WriteString(noRemedyNotice + "\n") + } if len(inv.Unresolved) > 0 { b.WriteString("*Unresolved:*\n") for _, u := range inv.Unresolved { diff --git a/internal/notify/slack.go b/internal/notify/slack.go index 402a02c3..14fbdd65 100644 --- a/internal/notify/slack.go +++ b/internal/notify/slack.go @@ -670,7 +670,21 @@ func summaryBlocks(inv providers.Investigation) []map[string]any { // 6. Suggested next steps โ€” the resolution guide (per-root-cause suggestions + // policy actions, de-duplicated, reversibility-flagged), capped at three. - if steps := nextSteps(inv); len(steps) > 0 { + // + // Rendered even with nothing to show, saying so โ€” see + // providers.Investigation.ActionWithoutRemedy for the payload that motivated it, + // and notify.remedyMissingForReader for why the gate is not that predicate + // directly. The two branches are mutually exclusive: the predicate requires that + // no root cause and no action carried a remedy, and nextSteps builds its list + // from exactly those fields, trimmed the same way. + steps := nextSteps(inv) + if remedyMissingForReader(inv) { + blocks = append(blocks, + map[string]any{"type": "divider"}, + map[string]any{"type": "section", "text": map[string]any{"type": "mrkdwn", + "text": noRemedyNotice}}) + } + if len(steps) > 0 { var s strings.Builder s.WriteString("*๐Ÿ›  Suggested next steps* _(read-only โ€” RunLore won't apply these)_") // Own loop, not appendCappedBullets: nextSteps has already escaped these @@ -1010,6 +1024,11 @@ func nextSteps(inv providers.Investigation) []string { var steps []string seen := map[string]bool{} add := func(desc string, reversible bool) { + // Trimmed, so this agrees with providers.Investigation.ActionWithoutRemedy, + // which trims too. Gating on desc == "" instead let a whitespace-only + // suggested_action produce a step, suppressing the no-remedy notice while + // rendering an empty bullet โ€” the promised remedy, spelled as one space. + desc = strings.TrimSpace(desc) if desc == "" || seen[desc] { return } diff --git a/internal/notify/slack_test.go b/internal/notify/slack_test.go index 9b7e3198..d150182b 100644 --- a/internal/notify/slack_test.go +++ b/internal/notify/slack_test.go @@ -1752,3 +1752,89 @@ func TestMultiDeliverKBUpdateNilAndEmptyAreNoOps(t *testing.T) { t.Fatalf("empty Multi: got %v, want nil", err) } } + +// TestNoRemedyNoticeRespectsWhatTheCardShows pins the split between the contract +// predicate and the reader-side one. The contract asks "did the model supply a +// remedy"; the card must ask "does the reader have one in front of them", and the +// two differ on the shapes the shipped golden fixtures already cover. +// +// Without this split the notice printed on `seen_before` and `matched_runbook` โ€” +// telling the on-call the investigation supplied no remedy directly below a +// human-reviewed resolution, or a matched runbook with its URL. +func TestNoRemedyNoticeRespectsWhatTheCardShows(t *testing.T) { + base := func() providers.Investigation { + return providers.Investigation{ + Title: "harbor-db crash-looping again", + Verdict: providers.VerdictActionSuggested, + RootCauses: []providers.Hypothesis{{Summary: "the same migration lock", Confidence: 0.6}}, + } + } + cases := []struct { + name string + mutate func(*providers.Investigation) + wantNotice bool + wantSteps string // when set, the normal section must render carrying this text + }{ + {"nothing to act on at all", func(*providers.Investigation) {}, true, ""}, + {"a validated prior resolution is a remedy", func(inv *providers.Investigation) { + inv.Prior = &providers.PriorKnowledge{Resolution: "roll back to 1.14.2, clear the lock"} + }, false, ""}, + {"a whitespace-only prior resolution is not", func(inv *providers.Investigation) { + inv.Prior = &providers.PriorKnowledge{Resolution: " "} + }, true, ""}, + {"a matched runbook is a remedy", func(inv *providers.Investigation) { + inv.MatchedKnowledge = &providers.MatchedEntry{ + Path: "entries/x.md", Title: "Registry credentials deleted", Score: 6.4} + }, false, ""}, + {"the model's own suggested_action is a remedy", func(inv *providers.Investigation) { + inv.RootCauses[0].SuggestedAction = "clear the migration lock" + }, false, "clear the migration lock"}, + {"a whitespace-only suggested_action is not, and renders no empty bullet", func(inv *providers.Investigation) { + inv.RootCauses[0].SuggestedAction = " " + }, true, ""}, + {"no_action promises nothing", func(inv *providers.Investigation) { + inv.Verdict = providers.VerdictNoAction + }, false, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + inv := base() + c.mutate(&inv) + txt := blocksText(t, summaryBlocks(inv)) + if got := strings.Contains(txt, "No next steps proposed"); got != c.wantNotice { + t.Errorf("notice rendered = %v, want %v:\n%s", got, c.wantNotice, txt) + } + // A supplied remedy must render the normal section carrying its text. + if c.wantSteps != "" { + if !strings.Contains(txt, "Suggested next steps") || !strings.Contains(txt, c.wantSteps) { + t.Errorf("a supplied remedy must render normally:\n%s", txt) + } + } + // Whichever way it went, the card must never show a bullet with nothing in + // it. Next steps render as "\nโ€ข %s", with the reversible marker on the bullet. + if strings.Contains(txt, "โ€ข \n") || strings.Contains(txt, "โ€ข _(reversible)_") { + t.Errorf("rendered an empty next-step bullet:\n%s", txt) + } + }) + } +} + +// TestFormatCarriesTheNoRemedyNotice: Format feeds Matrix, the webhook text and the +// CLI, and its doc comment says its claims "must never diverge" from the Slack card. +// Gating only the Slack block would have left every non-Slack channel showing the +// 2026-08-24 defect unfixed. +func TestFormatCarriesTheNoRemedyNotice(t *testing.T) { + inv := providers.Investigation{ + Title: "BackupJobFailed", + Verdict: providers.VerdictActionRequired, + RootCauses: []providers.Hypothesis{{Summary: "the failing RDS resource could not be identified"}}, + } + if out := Format(inv); !strings.Contains(out, "No next steps proposed") { + t.Errorf("Format must say the verdict promises a remedy it does not carry:\n%s", out) + } + // And it must not, when the reader has something to act on. + inv.RootCauses[0].SuggestedAction = "delete the stale snapshot" + if out := Format(inv); strings.Contains(out, "No next steps proposed") { + t.Errorf("Format printed the notice over a supplied remedy:\n%s", out) + } +} diff --git a/internal/providers/providers.go b/internal/providers/providers.go index ca158527..9f64e386 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -1585,6 +1585,50 @@ func (inv Investigation) UnaccountedInconclusive() bool { len(inv.RootCauses) == 0 && len(inv.Unresolved) == 0 && len(inv.DataGaps) == 0 } +// ActionWithoutRemedy reports whether inv tells the on-call to act and then gives +// them nothing to do: a verdict of action_suggested/action_required with no +// suggested_action on any root cause and no proposed action. +// +// Both fields are optional in submit_findings, so this payload is legal, and it +// shipped live on 2026-08-24 โ€” a card headed "Action suggested" whose next-steps +// section was simply absent. The verdict badge is rendered from the verdict alone, +// so the header promises a remedy the body never carries; a card that does that +// costs more trust than one that says inconclusive. +// +// Placement, and the pre-verify/post-verify caveat, are exactly as for +// UnaccountedInconclusive above. As there, the notifier does not render straight off +// this predicate โ€” see notify.remedyMissingForReader, which additionally requires +// that the card show no OTHER remedy (a validated prior resolution, a matched +// runbook). +func (inv Investigation) ActionWithoutRemedy() bool { + if !inv.Verdict.ClaimsAction() { + return false + } + for _, rc := range inv.RootCauses { + if strings.TrimSpace(rc.SuggestedAction) != "" { + return false + } + } + for _, a := range inv.Actions { + if strings.TrimSpace(a.Description) != "" { + return false + } + } + return true +} + +// ClaimsAction reports whether a verdict tells the on-call to do something, and so +// obliges the delivered notification to show what. no_action and inconclusive make +// no such promise, and an empty/unknown verdict is a parse concern rather than a +// contract breach โ€” it renders no badge at all, so it promises nothing. +// +// Defined in terms of Conclusive for the reason ValidVerdict gives: the three +// actionability verdicts stay enumerated once, so a fifth is one switch to update +// rather than two adjacent ones. +func (v Verdict) ClaimsAction() bool { + return v.Conclusive() && v != VerdictNoAction +} + // MatchedEntry is the strongest pre-existing catalog entry an investigation's // kb_search calls matched at clear-match strength. It closes a live visibility gap: // when a full investigation's kb_search found a known runbook and used it, the