diff --git a/changelog.d/fixed-2364-retracted-findings.md b/changelog.d/fixed-2364-retracted-findings.md new file mode 100644 index 000000000..6e5cbd82a --- /dev/null +++ b/changelog.d/fixed-2364-retracted-findings.md @@ -0,0 +1 @@ +- The advisory digest no longer spends a top-N slot on a finding the reporting agent has already withdrawn ([#2364](https://github.com/hivecommons/hive/issues/2364)). The 2026-09-05 digest held one of its five CRITICAL slots with `heartbeatBearerOK per-hive binding removed without covering test`, whose own detail begins *"REVISED: ... not removed ... The change is refactoring, not a security regression. Downgrading priority."* — while 282 findings went unshown. "Downgrading priority" never reached the bead: the agent edited the notes, severity is read from the bead's priority, so the finding kept rendering at critical and winning a slot every cycle. A withdrawn finding now ranks below every finding nobody withdrew, in any severity band, and renders with a caption saying the bead wants closing rather than fixing. This is the only demotion that crosses severity bands: [#5945](https://github.com/hivecommons/hive/issues/5945)'s three signals all mean "nobody re-checked this", so an unverified critical may still be the worst thing in the report, whereas a retraction is the reporting agent re-checking and saying the finding does not stand. It is demoted rather than dropped, because the bead is still open and only a maintainer closes it. Detection requires two signals — a revision marker at the start of the detail AND a phrase saying the finding no longer stands — because "REVISED:" alone is equally how an agent raises a finding's severity, and demoting that would bury the most urgent item in the report. diff --git a/src/pkg/advisory/advisory.go b/src/pkg/advisory/advisory.go index bc1627ed8..a63011776 100644 --- a/src/pkg/advisory/advisory.go +++ b/src/pkg/advisory/advisory.go @@ -503,6 +503,16 @@ func (o DigestOptions) resolvedRenderCap() int { // resolved on the findings by the time ranking runs, so only the path check // needs verify. // +// A finding its own author has WITHDRAWN (findingRetracted) ranks below every +// finding nobody has withdrawn, in ANY band. This is the one demotion that +// crosses severity bands, and the reason is the same sentence that keeps the +// others inside theirs: unverified means nobody re-checked, so an unverified +// critical may still be the worst thing here. A retraction is the opposite -- +// the reporting agent DID re-check and said the finding does not stand -- so +// its filed severity is the one claim it no longer makes. It still backfills +// rather than being dropped, because the bead is open and only a maintainer +// closes it (hivecommons/hive#2364). +// // verify, when non-nil, reports whether a finding's file path still exists at // the analyzed snapshot. Verification is on-demand and ordered: the ranked list // is walked from the top and verify is called only until cap findings are in @@ -556,6 +566,9 @@ func applyTopN(byAgent map[string][]Finding, cap int, verify func(path string) b // confirmed low — demoting across bands would let a cosmetic nit displace a // security finding whose file was merely renamed. var kept []Finding + // Withdrawn findings are collected across ALL bands and placed last -- see + // the band-crossing paragraph in the doc comment above. + var retracted []Finding for i := 0; i < len(all) && len(kept) < cap; { rank := severityRank(all[i].Severity) j := i @@ -571,6 +584,13 @@ func applyTopN(byAgent map[string][]Finding, cap int, verify func(path string) b if len(kept) == cap { break } + if findingRetracted(f) { + // Set aside BEFORE markPathStale: a withdrawn finding is only + // reached if slots go unclaimed, and verify is a lookup the + // ranking promises not to spend on findings it never renders. + retracted = append(retracted, f) + continue + } markPathStale(&f) if f.evidenceUnverified() { unverified = append(unverified, f) @@ -583,6 +603,15 @@ func applyTopN(byAgent map[string][]Finding, cap int, verify func(path string) b } i = j } + // Nothing live is left to show: rather than render a short digest, fill the + // remaining slots with the withdrawn findings, in the order they ranked. + // They arrive carrying the renderer's withdrawal caption, which is what + // tells a maintainer the bead wants closing rather than fixing. + for k := 0; len(kept) < cap && k < len(retracted); k++ { + f := retracted[k] + markPathStale(&f) + kept = append(kept, f) + } capped := make(map[string][]Finding, len(byAgent)) for _, f := range kept { @@ -1092,7 +1121,14 @@ func FormatDigestMarkdown(d *Digest, opts DigestOptions) string { // letting it cover this one is the overclaim that got a stale // finding reported as a fabrication. prov := "" - if f.ProvenanceStale && f.ProvenanceSHA != "" { + if findingRetracted(f) { + // The reporting agent withdrew this finding in the detail + // rendered directly below, but the bead is still open at this + // severity -- only a maintainer closes it. Say which of the two + // the reader is looking at, so the entry reads as a bead to + // close rather than a problem to fix (#2364). + prov = " ⚠️ _(withdrawn by the reporting agent in the detail below — the bead is still open at this severity, so it wants closing rather than fixing)_" + } else if f.ProvenanceStale && f.ProvenanceSHA != "" { prov = fmt.Sprintf(" ⚠️ _(evidence computed at `%s`, not re-verified at the analyzed commit)_", shortSHA(f.ProvenanceSHA)) } else if f.CachedReplays > 0 && f.ProvenanceSHA == "" { // A no-provenance finding whose only "confirmations" were diff --git a/src/pkg/advisory/retraction.go b/src/pkg/advisory/retraction.go new file mode 100644 index 000000000..498c7b9c1 --- /dev/null +++ b/src/pkg/advisory/retraction.go @@ -0,0 +1,116 @@ +package advisory + +import ( + "regexp" + "strings" +) + +// A finding can stop being worth a top-N slot in two very different ways, and +// the digest only ever handled one of them. +// +// The three signals behind evidenceUnverified all say the same thing: NOBODY +// re-checked this. The path is gone (#3704), the evidence came from another +// commit (#5130), the repetition was cached text (#5236). None is a statement +// about whether the finding is true, which is exactly why applyTopN demotes on +// them WITHIN a severity band rather than across bands -- an unverified +// critical may still be the worst thing in the report. +// +// This file covers the other way: the agent that filed the finding came back, +// re-checked it, and said in the finding's own detail that it does not stand. +// That is not an absence of verification. It is verification with a negative +// result, from the only party that ever looked, and it was the one such signal +// the ranking ignored entirely. +// +// The live case, from the 2026-09-05 digest on hivecommons/hive#2364, held one +// of the five CRITICAL slots while 282 findings went unshown: +// +// [regression-risk] heartbeatBearerOK per-hive binding removed without +// covering test +// > REVISED: Per-hive identity binding was MOVED from heartbeatBearerOK to +// > verifyHeartbeatBearer in hub_keys.go, not removed. ... The change is +// > refactoring, not a security regression. Downgrading priority. +// +// "Downgrading priority" never reached the bead: beadPriorityToSeverity reads +// b.Priority, the agent edited only the notes, so the finding kept rendering at +// critical and winning a slot every cycle. The maintainer working the tracker +// wrote it up as "refuted by its own detail line and should be closed rather +// than fixed -- its bead is still open at critical, which is why it keeps +// consuming a slot". + +// TWO SIGNALS ARE REQUIRED, and the asymmetry is deliberate. +// +// "REVISED:" alone is not a withdrawal. An agent can equally write "REVISED: +// raising this to critical after finding a second call site", and demoting THAT +// below every other finding would bury the most urgent thing in the report +// under a caption telling the reader to disregard it. So a finding counts as +// retracted only when its detail opens with a revision marker AND says +// somewhere that the finding does not stand. +// +// Missing a real retraction costs nothing beyond the status quo: the finding +// keeps its filed severity, exactly as today. Inventing one costs a live +// critical its slot. The rule fails toward KEEPING findings, and both +// directions are tested. + +// retractionMarkerPattern matches an explicit revision lead-in at the very +// START of a finding's detail: "REVISED:", "**CORRECTION:**", "> RETRACTED --". +// +// Anchored on purpose. A finding whose body merely mentions the word revised +// ("the workflow was revised in #4102") is reporting something, not retracting +// it, and only the lead-in position separates the two. The leading character +// class absorbs the markdown and quoting agents decorate these with. +// +// "revision" is deliberately NOT in this list, though "revised" is. This +// package already uses "revision " as PROVENANCE vocabulary -- +// provenanceRefPattern matches exactly that -- so a detail opening +// "revision c9546a8 ..." states where the evidence came from rather than +// withdrawing it. The collision is not hypothetical: provenance_test.go builds +// a fixture that way, and it is what caught this while the rule was written. +var retractionMarkerPattern = regexp.MustCompile( + "(?i)^[[:space:]>*_#-]*(revised|correction|corrected|retracted|retraction|withdrawn)\\b[[:space:]:.,-]*") + +// retractionWithdrawalCues are the phrases that turn a revision into a +// withdrawal. One is enough. They are matched as substrings of the LOWERCASED +// detail rather than as a regex, so extending the list later needs no thought +// about escaping. +// +// Deliberately short: every entry is something an agent would only write about +// a finding it no longer stands behind. +var retractionWithdrawalCues = []string{ + "downgrad", // "Downgrading priority", "downgraded to low" + "retract", // "retracting this finding" + "withdraw", // + "false positive", + "not a regression", + "not a security regression", + "does not stand", + "no longer applies", + "no longer valid", + "was incorrect", + "is incorrect", + "not an issue", + "superseded", +} + +// findingRetracted reports whether the agent that filed this finding has since +// withdrawn it in the finding's own detail. +// +// It reads only the detail. A title is written once, when the finding is first +// filed; the notes are where an agent goes back and revises, which is why the +// live case says nothing about the retraction in its title and why the bead's +// priority -- and therefore its rendered severity -- never moved. +func findingRetracted(f Finding) bool { + detail := strings.TrimSpace(f.Detail) + if detail == "" { + return false + } + if !retractionMarkerPattern.MatchString(detail) { + return false + } + lower := strings.ToLower(detail) + for _, cue := range retractionWithdrawalCues { + if strings.Contains(lower, cue) { + return true + } + } + return false +} diff --git a/src/pkg/advisory/retraction_test.go b/src/pkg/advisory/retraction_test.go new file mode 100644 index 000000000..bff59f8e3 --- /dev/null +++ b/src/pkg/advisory/retraction_test.go @@ -0,0 +1,265 @@ +package advisory + +import ( + "strings" + "testing" + + "github.com/hivecommons/hive/pkg/beads" +) + +// Regression coverage for hivecommons/hive#2364. +// +// The 2026-09-05 09:32 EDT digest on that tracker rendered 10 findings out of +// 292, and one of its five CRITICAL slots was held by a finding the reporting +// agent had already withdrawn in the finding's own detail. The maintainer +// working the tracker wrote it up as "refuted by its own detail line and +// should be closed rather than fixed -- its bead is still open at critical, +// which is why it keeps consuming a slot". +// +// #5945 had already taught applyTopN to demote on the three +// nobody-re-checked-this signals. A retraction is a different thing: the one +// party who ever looked went back, looked again, and said it does not stand. +// That was the only such signal the ranking ignored. + +// retractedDetail is the live finding's detail, verbatim from the digest. +const retractedDetail = "REVISED: Per-hive identity binding was MOVED from " + + "heartbeatBearerOK to verifyHeartbeatBearer in hub_keys.go, not removed. " + + "heartbeatBearerOK is now a thin prefix-check, and identity verification " + + "happens via verifyHeartbeatBearer which has tests in " + + "heartbeat_identity_test.go. The change is refactoring, not a security " + + "regression. Downgrading priority." + +// TestFindingRetracted_TwoSignalRule is the whole safety argument. A revision +// marker alone must NOT retract: an agent revising a finding UPWARD writes the +// same lead-in, and demoting that would bury the most urgent thing in the +// report under a caption telling the reader to disregard it. +func TestFindingRetracted_TwoSignalRule(t *testing.T) { + cases := []struct { + name string + detail string + want bool + }{ + {"the live #2364 finding", retractedDetail, true}, + { + "a revision that RAISES severity is not a retraction", + "REVISED: found a second call site with the same gap; raising this to critical.", + false, + }, + { + "a marker with no withdrawal cue is not a retraction", + "REVISED: added the exact line numbers and the failing test name.", + false, + }, + { + "a withdrawal cue with no marker is not a retraction", + "The scheduler downgrades the request when the queue is full.", + false, + }, + { + "the word revised mid-sentence is reporting, not retracting", + "The workflow was revised in #4102 and the guard was never restored; downgrade risk.", + false, + }, + {"an empty detail retracts nothing", "", false}, + {"markdown decoration is absorbed", "**CORRECTION:** this was a false positive.", true}, + {"quote decoration is absorbed", "> RETRACTED - no longer applies after #5900.", true}, + {"leading whitespace is tolerated", " revised: withdrawing this finding.", true}, + {"case does not matter", "ReViSeD: this is incorrect, see below.", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := findingRetracted(Finding{Detail: tc.detail}); got != tc.want { + t.Errorf("findingRetracted(%q) = %v, want %v", tc.detail, got, tc.want) + } + }) + } +} + +// The retraction lives in the notes, never the title -- which is exactly why +// the bead's priority never moved and the finding kept its critical slot. +func TestFindingRetracted_ReadsTheDetailNotTheTitle(t *testing.T) { + f := Finding{Title: "REVISED: downgrading this", Detail: "the guard is still missing"} + if findingRetracted(f) { + t.Error("a retraction-shaped TITLE retracted the finding; only the detail may") + } +} + +// retractedFinding builds the live #2364 finding at a given severity. +func retractedFinding(sev, title string) Finding { + return Finding{Agent: "quality", Type: "regression-risk", Severity: sev, Title: title, Detail: retractedDetail} +} + +// TestApplyTopN_RetractedLosesItsSlotAcrossBands is the reported defect: a +// withdrawn CRITICAL was holding a top-N slot while live findings of LOWER +// severity went unshown. This is the one demotion that crosses severity bands. +func TestApplyTopN_RetractedLosesItsSlotAcrossBands(t *testing.T) { + byAgent := map[string][]Finding{ + "quality": { + retractedFinding("critical", "heartbeatBearerOK per-hive binding removed without covering test"), + {Agent: "quality", Severity: "low", Title: "a live low finding nobody withdrew"}, + }, + } + + capped, overflow := applyTopN(byAgent, 1, nil) + if overflow != 1 { + t.Fatalf("overflow = %d, want 1", overflow) + } + kept := keptTitles(capped) + if len(kept) != 1 { + t.Fatalf("kept %d findings under a cap of 1: %v", len(kept), kept) + } + if kept[0] != "a live low finding nobody withdrew" { + t.Errorf("the single slot went to %q; a withdrawn critical must not outrank a live low", kept[0]) + } +} + +// The discriminating counterpart. Without it the suite would pass on an +// implementation that demoted every critical, or every regression-risk finding. +func TestApplyTopN_UnwithdrawnCriticalStillWins(t *testing.T) { + live := retractedFinding("critical", "heartbeatBearerOK per-hive binding removed without covering test") + // Same finding, same severity, same agent -- only the withdrawal is gone. + live.Detail = "REVISED: added the exact line numbers and the failing test name." + byAgent := map[string][]Finding{ + "quality": { + live, + {Agent: "quality", Severity: "low", Title: "a live low finding nobody withdrew"}, + }, + } + + capped, _ := applyTopN(byAgent, 1, nil) + kept := keptTitles(capped) + if len(kept) != 1 || !strings.Contains(kept[0], "heartbeatBearerOK") { + t.Errorf("the slot went to %v; a critical nobody withdrew must still win it", kept) + } +} + +// A withdrawn finding is DEMOTED, never dropped: the bead is still open and +// only a maintainer closes it, so rendering 9 findings under a cap of 10 would +// hide the very entry that wants closing. +func TestApplyTopN_RetractedBackfillsRatherThanDisappearing(t *testing.T) { + byAgent := map[string][]Finding{ + "quality": { + retractedFinding("critical", "heartbeatBearerOK per-hive binding removed without covering test"), + {Agent: "quality", Severity: "low", Title: "a live low finding nobody withdrew"}, + {Agent: "quality", Severity: "low", Title: "another live low finding"}, + }, + } + + // Cap of 3 with 3 findings returns early, so use a cap that still ranks. + byAgent["quality"] = append(byAgent["quality"], Finding{Agent: "quality", Severity: "low", Title: "a third live low finding"}) + capped, _ := applyTopN(byAgent, 3, nil) + kept := keptTitles(capped) + if len(kept) != 3 { + t.Fatalf("kept %d under a cap of 3: %v", len(kept), kept) + } + var sawRetracted bool + for _, k := range kept { + if strings.Contains(k, "heartbeatBearerOK") { + sawRetracted = true + } + } + if sawRetracted { + t.Error("the withdrawn finding took a slot three live findings could fill") + } + + // Now with only two live findings, the third slot has no live claimant and + // the withdrawn one must fill it rather than leaving the digest short. + byAgent2 := map[string][]Finding{ + "quality": { + retractedFinding("critical", "heartbeatBearerOK per-hive binding removed without covering test"), + {Agent: "quality", Severity: "low", Title: "a live low finding nobody withdrew"}, + {Agent: "quality", Severity: "low", Title: "another live low finding"}, + {Agent: "quality", Severity: "low", Title: "a fourth live low finding"}, + }, + } + capped2, _ := applyTopN(byAgent2, 4, nil) + if got := len(keptTitles(capped2)); got != 4 { + t.Errorf("kept %d under a cap of 4; a withdrawn finding must backfill an unclaimed slot", got) + } +} + +// The caption is the other half. A demoted finding that still renders with no +// explanation reads as a live problem sitting oddly low in the report; the +// point is to tell a maintainer this bead wants CLOSING rather than fixing. +func TestFormatDigestMarkdown_CaptionsWithdrawnFindings(t *testing.T) { + d := &Digest{ + ByAgent: map[string][]Finding{ + "quality": {retractedFinding("critical", "heartbeatBearerOK per-hive binding removed without covering test")}, + }, + TotalCount: 1, + } + out := FormatDigestMarkdown(d, DigestOptions{}) + + if !strings.Contains(out, "withdrawn by the reporting agent") { + t.Errorf("a withdrawn finding rendered with no caption; got:\n%s", out) + } + if !strings.Contains(out, "closing rather than fixing") { + t.Errorf("the caption does not say what the maintainer should DO; got:\n%s", out) + } + // The detail carrying the withdrawal has to be right there under it, or the + // caption is an assertion the reader cannot check. + if !strings.Contains(out, "Downgrading priority") { + t.Errorf("the withdrawal text itself is not rendered; got:\n%s", out) + } +} + +// A finding nobody withdrew must not pick up the caption -- that would tell a +// maintainer to close a live critical. +func TestFormatDigestMarkdown_LiveFindingKeepsNoWithdrawalCaption(t *testing.T) { + f := retractedFinding("critical", "heartbeatBearerOK per-hive binding removed without covering test") + f.Detail = "REVISED: added the exact line numbers and the failing test name." + d := &Digest{ByAgent: map[string][]Finding{"quality": {f}}, TotalCount: 1} + + if out := FormatDigestMarkdown(d, DigestOptions{}); strings.Contains(out, "withdrawn by the reporting agent") { + t.Errorf("a live finding was captioned as withdrawn; got:\n%s", out) + } +} + +// End to end from the bead store, because the withdrawal lives in the bead's +// NOTES and the severity comes from its PRIORITY. That split is the whole +// mechanism of the bug: the agent edited the notes, beadPriorityToSeverity +// kept reading the priority, and the finding went on rendering at critical. +func TestBuildDigestFromBeads_WithdrawnFindingYieldsItsSlot(t *testing.T) { + store, err := beads.NewStore(t.TempDir()) + if err != nil { + t.Fatalf("creating store: %v", err) + } + withdrawn, err := store.Create("heartbeatBearerOK per-hive binding removed without covering test", + beads.TypeAdvisory, severityToPriority("critical"), "quality", "") + if err != nil { + t.Fatalf("creating bead: %v", err) + } + if err := store.Update(withdrawn.ID, func(b *beads.Bead) { b.Notes = retractedDetail }); err != nil { + t.Fatalf("setting notes: %v", err) + } + if _, err := store.Create("gateway health faults are dropped on the merge path", + beads.TypeAdvisory, severityToPriority("medium"), "quality", ""); err != nil { + t.Fatalf("creating bead: %v", err) + } + + d := BuildDigestFromBeads(map[string]*beads.Store{"quality": store}, "advisory", DigestOptions{MaxFindings: 1}) + + all := digestFindings(d) + if len(all) != 1 { + t.Fatalf("%d findings rendered under a cap of 1", len(all)) + } + if strings.Contains(all[0].Title, "heartbeatBearerOK") { + t.Errorf("the withdrawn critical took the only slot; a live medium was available") + } + // The bead is still OPEN -- the pipeline demotes, it does not close. + if len(d.RecentlyResolved) != 0 { + t.Errorf("a withdrawn finding was reported as resolved; only a maintainer closes the bead") + } +} + +// "revision " is this package's PROVENANCE idiom (provenanceRefPattern), +// not a withdrawal. Reading it as one would caption a #5130 stale-provenance +// finding as withdrawn and demote it across every severity band. Pinned here +// because the two vocabularies genuinely overlap and the next person adding a +// marker word will not know that. +func TestFindingRetracted_ProvenanceVocabularyIsNotAWithdrawal(t *testing.T) { + f := Finding{Detail: "revision c9546a8a24b3dded3146e3ab7a93dd99edc56fa3 - downgrading the linked issue"} + if findingRetracted(f) { + t.Error("a provenance line was read as a retraction; 'revision ' states where evidence came from") + } +}