From 9008cfdeda224b47d3d4154016f05df1eea282ba Mon Sep 17 00:00:00 2001 From: Douglas Baggett Date: Sat, 5 Sep 2026 09:49:31 -0400 Subject: [PATCH] fix(advisory): rank a withdrawn finding below every finding nobody withdrew The 2026-09-05 digest on #2364 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: [regression-risk] heartbeatBearerOK per-hive binding removed without covering test > REVISED: ... not removed ... The change is refactoring, not a security > regression. Downgrading priority. "Downgrading priority" never reached the bead. The agent edited the notes; beadPriorityToSeverity reads b.Priority; so the finding went on 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". #5945 had already taught applyTopN to demote on the three nobody-re-checked-this signals. A retraction is a different kind of 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. THE ONE DEMOTION THAT CROSSES SEVERITY BANDS. #5945's signals stay inside their band on purpose -- unverified means nobody re-checked, so an unverified critical may still be the worst thing in the report. A retraction inverts that: the filed severity is the one claim its author no longer makes. It still backfills an unclaimed slot rather than being dropped, because the bead is open and only a maintainer closes it, and it arrives carrying a caption saying the bead wants closing rather than fixing. TWO SIGNALS ARE REQUIRED, and the asymmetry is the safety argument. "REVISED:" alone is not a withdrawal -- an agent raising a finding to critical writes the same lead-in, and demoting THAT below every other finding would bury the most urgent item in the report under a caption telling the reader to disregard it. So a finding is retracted only when its detail opens with a revision marker AND says somewhere that it no longer stands. Missing a real retraction costs nothing beyond today's behaviour; inventing one costs a live critical its slot. Mutation testing found a collision worth recording: "revision" was in the marker vocabulary at first, and it is also this package's PROVENANCE idiom -- provenanceRefPattern matches "revision ", and provenance_test.go builds a fixture exactly that way. Under a one-signal rule that fixture's #5130 stale-provenance caption became a withdrawal caption. "revision" is now out of the marker list, "revised" is not, and both the collision and the reason are pinned by a test. Closes #2364 is deliberately NOT claimed: #2364 is a standing living document ("Do not close this issue"), and a maintainer has already removed `help wanted` from it for that reason. This fixes the defect its current digest exhibits. Refs #2364 Signed-off-by: Douglas Baggett --- changelog.d/fixed-2364-retracted-findings.md | 1 + src/pkg/advisory/advisory.go | 38 ++- src/pkg/advisory/retraction.go | 116 ++++++++ src/pkg/advisory/retraction_test.go | 265 +++++++++++++++++++ 4 files changed, 419 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixed-2364-retracted-findings.md create mode 100644 src/pkg/advisory/retraction.go create mode 100644 src/pkg/advisory/retraction_test.go 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") + } +}