Pick the right fix when an advisory patched several branches

A review of the fixed-version work in #4 turned up one behavioural bug and
six places where the code and its own documentation had drifted apart.

An OSV record routinely carries more than one `fixed` event, because a vendor
maintaining several branches patches them all: GO-2022-0623 fixed Vault in
1.5.9, 1.6.5 and 1.7.2, and 60 of the 131 records returned for that module
read the same way. The last event won, on the reading that OSV lists them
ascending so the last is newest -- true, and irrelevant, because the newest of
several maintained branches is not the upgrade target for someone on the
oldest of them. A 1.5.4 install was told to move to 1.8.5 to close
CVE-2021-43998, when 1.7.6 closes it.

Advisory.Fixed becomes a list, Advisory.Ecosystem travels with it, and the
overlay picks the lowest fix that is actually an upgrade. Picking needs a
version order, so it is scoped to what this tool can order with confidence:
Debian and Ubuntu through internal/debver, Go and npm through semver. PyPI is
deliberately out, since PEP 440 sorts 1.0rc1 before 1.0 and semver sorts it
after; so are the RPM distros, since rpmvercmp is not verrevcmp. Everything
else keeps the old last-wins target, which overshoots rather than naming a
version that may not contain the fix -- and --details now names the branches
it did not choose either way, so the overshoot is visible.

The rest, all from the same review:

- Two doc comments lost their first line when the new code was spliced in
  above them. `go doc` was rendering writePriority's as "severity spread above
  it counts." and aliases's as "It exists for the VEX overlay."
- The fix plan borrowed renderText's footer, which repeats the main report's
  section index -- naming AFFECTED and RULED OUT under a document whose only
  headings are UPGRADE and NO FIX YET. It gets its own footer: its own summary,
  and the caveats, which are about the scan and not about the view.
- "clears 86 advisories; 154 with no fix yet" mixed units in one sentence. The
  86 are advisories and the 154 are findings; both now say so.
- The fix plan silently dropped findings a vendor VEX statement had answered
  and findings that were undetermined. A remediation view that omits rows is
  the one report whose shortness reads as good news, so they are counted.
- writeRemediation's doc comment claimed the fix clause is skipped when
  nothing is fixable. An autofix commit had already changed that, correctly --
  a fully-patched image is the case a reader most wants confirmed. The comment,
  the README and the test name now say what the code does.
- fixed_version had omitempty, which erased the difference between "no patch
  has shipped" and a scan run before the field existed. The text report goes
  to the trouble of printing "no fix" rather than a blank for exactly this
  reason.
- The --help example for --format fixplan used debian:12, which is fully
  patched and renders the degenerate case.

Verified against debian:12.0: 138 of 292 fixable, 28 upgrade rows, 86
advisories cleared -- unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: cwayne18

README.md

@@ -967,8 +967,38 @@ appears only when a section holds at least one row with a published fix, so a
 fully-patched image or an ecosystem that ships no fixed versions gets no column
 of blanks. `no fix` and an empty cell are kept distinct on purpose: `no fix` is
 data (the advisory is acknowledged and no patch has shipped), while a blank
-would read as missing data. The summary's `N fixable, M with no fix yet` clause
-is dropped entirely when nothing is fixable, so it never prints `0 fixable`.
+would read as missing data — and for the same reason `fixed_version` is one of
+the few JSON fields with no `omitempty`. The summary's `N fixable, M with no fix
+yet` clause is printed even when nothing is fixable, where it reads `154 with no
+fix yet`: a fully-patched image is the case a reader most wants confirmed, and
+silence in a summary reads as a missing measurement rather than a measured zero.
+It never phrases it as `0 fixable`.
+
+One advisory often publishes more than one fix. A vendor maintaining several
+branches patches them all: `GO-2022-0623` fixed Vault in `1.5.9`, `1.6.5` **and**
+`1.7.2`, and 22 of the 110 records for that module read the same way. Those are
+alternatives, not a progression, so the target depends on the branch you are on
+— for a `1.5.4` install the answer is `1.5.9`, and naming `1.7.2` would prescribe
+two major versions of unrelated change to close one advisory. vexscan keeps every
+published fix, picks the lowest one that is actually an upgrade, and shows the
+rest under `--details`:
+
+```
+  fixed in: 1.5.9 (also fixed in 1.6.5, 1.7.2)
+```
+
+Picking needs a version order, and where the tool has none it keeps the newest
+fix — the behaviour it had before it kept the list — and still discloses the
+alternatives, so an overshoot is visible rather than silent. The ordered
+ecosystems are Debian and Ubuntu (dpkg's own algorithm, `internal/debver`) and Go
+and npm (semver, which both databases publish by definition). PyPI is
+deliberately absent: PEP 440 sorts `1.0rc1` before `1.0` and semver sorts it
+after, so ordering Python fixes with semver would silently invert the pair. So
+are the RPM distros, because `rpmvercmp` is not dpkg's `verrevcmp` however
+similar they look. The asymmetry is the reason for the caution — too high a
+target is a bigger upgrade than necessary, while too low is a version that does
+not contain the fix, reported as the version that does. Distro records are
+single-branch, so on Debian and Ubuntu this almost never comes up.
 
 `--format fixplan` reorganizes the same affected findings by the action that
 clears them. Instead of one row per advisory, it is one row per **upgrade** —
@@ -980,7 +1010,7 @@ $ vexscan --image debian:bookworm-20230919 --all --ecosystem os --format fixplan
 vexscan report (image) for debian:bookworm-20230919
 
   138 of 292 affected findings have a fix.
-  upgrading 28 packages clears 86 advisories; 154 with no fix yet.
+  upgrading 28 packages clears 86 advisories; 154 findings have no fix yet.
 
 UPGRADE (28) - apply these to clear the fixable findings
 PACKAGE       CURRENT           FIXED IN            CLEARS  SEVERITY
@@ -1007,7 +1037,9 @@ revision), and findings stay split by their published fixed version rather than
 risk naming the wrong target as newest. It is a view, not a filter: every
 affected finding with no fix is still listed under `NO FIX YET`, because a
 remediation plan that quietly dropped the un-fixable rows would read as
-complete when it is not.
+complete when it is not. The rows it genuinely has nothing to plan for — the
+ones a vendor VEX statement already answered, and the undetermined ones — are
+counted in the summary rather than left out of the arithmetic.
 
 The rows are sorted worst-first — known-exploited, then severity, then the
 upgrades that clear the most — so the first line is the one to do first.
@@ -1358,6 +1390,11 @@ The JSON is `schema_version: 2`:
   "schema_version": 2,
   "target": "...", "mode": "image",          // or "rootfs", or "repo"
   "findings": [ /* flat, sorted — jq '.findings[]' still works */ ],
+  // every finding carries "fixed_version", always present: "" is the "no patch
+  // has shipped" answer, so omitting it would hide the thing worth acting on
+  // "fixed_versions" joins it only when the advisory patched several branches,
+  // listing all of them so a consumer can pick differently than the report did
+
   "ecosystems": [ { "id": "os", "components": 65, "error": "" } ],
   "unreadable": { "count": 3, "paths": ["/opt/vendor"] },  // omitted when nothing was skipped
   "vex_hubs": [ { "url": "...", "author": "...", "products": 1082, "matched": 3 } ],  // only with --vexhub

fixplan.go

@@ -38,15 +38,16 @@ func renderFixPlan(res *analyze.Result) string {
 	// by whether a fix exists so the actionable rows lead and the rest are
 	// still accounted for.
 	var fixable, noFix []analyze.Finding
-	undetermined := 0
+	var s fixSummary
 	for _, f := range res.Findings {
 		if !f.Affected() {
 			if f.Status != analyze.StatusNotPresent && f.Status != analyze.StatusNotInPath {
-				undetermined++
+				s.undetermined++
 			}
 			continue
 		}
 		if alreadyVexed(f) {
+			s.vexed++
 			continue
 		}
 		if f.FixedVersion != "" {
@@ -56,33 +57,13 @@ func renderFixPlan(res *analyze.Result) string {
 		}
 	}
 
-	total := len(fixable) + len(noFix)
-	if total == 0 {
-		b.WriteString("No affected findings to fix.\n")
-		if undetermined > 0 {
-			fmt.Fprintf(&b, "%d undetermined finding(s) are not shown here; run the default report to review them.\n", undetermined)
-		}
-		return b.String()
-	}
-
 	plan := groupUpgrades(fixable, dpkgPlugins(res))
-	cleared := uniqueAdvisories(fixable)
+	s.fixable, s.noFix = len(fixable), len(noFix)
+	s.upgrades, s.cleared = len(plan), uniqueAdvisories(fixable)
 
-	fmt.Fprintf(&b, "  %d of %d affected findings have a fix.\n", len(fixable), total)
-	switch {
-	case len(plan) > 0:
-		line := fmt.Sprintf("  upgrading %d %s clears %d %s",
-			len(plan), plural(len(plan), "package", "packages"),
-			cleared, plural(cleared, "advisory", "advisories"))
-		if len(noFix) > 0 {
-			line += fmt.Sprintf("; %d with no fix yet", len(noFix))
-		}
-		b.WriteString(line + ".\n")
-	case len(noFix) > 0:
-		fmt.Fprintf(&b, "  no published fixes yet for any of the %d affected findings.\n", len(noFix))
-	}
-	if undetermined > 0 {
-		fmt.Fprintf(&b, "  (%d undetermined finding(s) not shown; run the default report to review them.)\n", undetermined)
+	writeFixSummary(&b, s)
+	if s.fixable+s.noFix == 0 {
+		return b.String()
 	}
 	b.WriteString("\n")
 
@@ -93,12 +74,68 @@ func renderFixPlan(res *analyze.Result) string {
 		writeNoFix(&b, noFix)
 	}
 
+	// The footer is the fix plan's own, not renderText's. writeFooter repeats
+	// the main report's summary and its section index, which here would name
+	// AFFECTED and RULED OUT under a document whose only headings are UPGRADE
+	// and NO FIX YET -- an index of sections the reader cannot find. The
+	// caveats still repeat verbatim, because that promise is about the scan
+	// and not about the view.
 	if strings.Count(b.String(), "\n") > footerThreshold {
-		writeFooter(&b, res)
+		writeCaveats(&b, res)
+		writeFixSummary(&b, s)
 	}
 	return b.String()
 }
 
+// fixSummary is the fix plan's count of itself, computed once and printed at
+// both ends of the report so the header and the footer cannot disagree.
+type fixSummary struct {
+	fixable      int // affected findings with a published fix
+	noFix        int // affected findings without one
+	upgrades     int // rows in the UPGRADE table
+	cleared      int // distinct advisories those upgrades clear
+	vexed        int // affected, but already answered by a vendor
+	undetermined int // neither affected nor ruled out
+}
+
+// writeFixSummary prints what the plan covers and, as importantly, what it does
+// not.
+//
+// Every count names its unit. An earlier cut read "clears 86 advisories; 154
+// with no fix yet", where the first number is advisories and the second is
+// findings -- two units in one sentence, presented as if they were comparable.
+// The vexed and undetermined lines are here for the same reason the NO FIX YET
+// table is: a remediation view that silently omits rows is the one kind of
+// report whose shortness reads as good news.
+func writeFixSummary(b *strings.Builder, s fixSummary) {
+	total := s.fixable + s.noFix
+	switch {
+	case total == 0:
+		b.WriteString("  no affected findings to fix.\n")
+	default:
+		fmt.Fprintf(b, "  %d of %d affected findings have a fix.\n", s.fixable, total)
+	}
+	switch {
+	case s.upgrades > 0:
+		line := fmt.Sprintf("  upgrading %d %s clears %d %s",
+			s.upgrades, plural(s.upgrades, "package", "packages"),
+			s.cleared, plural(s.cleared, "advisory", "advisories"))
+		if s.noFix > 0 {
+			line += fmt.Sprintf("; %d %s no fix yet",
+				s.noFix, plural(s.noFix, "finding has", "findings have"))
+		}
+		b.WriteString(line + ".\n")
+	case s.noFix > 0:
+		fmt.Fprintf(b, "  no published fixes yet for any of the %d affected findings.\n", s.noFix)
+	}
+	if s.vexed > 0 {
+		fmt.Fprintf(b, "  (%d already answered by a vendor VEX statement, so not planned for.)\n", s.vexed)
+	}
+	if s.undetermined > 0 {
+		fmt.Fprintf(b, "  (%d undetermined finding(s) not shown; run the default report to review them.)\n", s.undetermined)
+	}
+}
+
 // upgrade is one action: move a package from its installed version to the
 // version that clears the vulnerabilities found on it. Several advisories
 // usually collapse into one upgrade, which is the whole point of the view.

fixplan_test.go

@@ -1,11 +1,13 @@
 package main
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/cwayne18/vexscan/internal/analyze"
 	"github.com/cwayne18/vexscan/internal/ecosystem"
+	"github.com/cwayne18/vexscan/internal/target"
 )
 
 // fixplan renders a fix plan from a set of findings, telling the renderer which
@@ -82,7 +84,9 @@ func TestFixPlanSeparatesNoFix(t *testing.T) {
 		t.Errorf("NO FIX YET should hold zlib1g only:\n%s", out)
 	}
 	lineWith(t, out, "1 of 2 affected findings have a fix.")
-	lineWith(t, out, "upgrading 1 package clears 1 advisory; 1 with no fix yet.")
+	// Every count names its unit: the 1 that is cleared is an advisory and the
+	// 1 that is not is a finding, and the sentence has to say so.
+	lineWith(t, out, "upgrading 1 package clears 1 advisory; 1 finding has no fix yet.")
 }
 
 // TestFixPlanDoesNotCollapseUnorderableEcosystem is the safety rule: where the
@@ -120,6 +124,73 @@ func TestFixPlanNothingFixable(t *testing.T) {
 	}
 }
 
+// The fix plan's footer has to be its own. writeFooter repeats the main
+// report's summary and its section index, which named AFFECTED and RULED OUT
+// under a document whose only headings are UPGRADE and NO FIX YET -- an index
+// of sections the reader cannot find anywhere above it.
+func TestTheFixPlanFooterDoesNotIndexSectionsItDoesNotHave(t *testing.T) {
+	var findings []analyze.Finding
+	for i := 0; i < 40; i++ {
+		findings = append(findings, linked(
+			fmt.Sprintf("CVE-2023-%04d", i), fmt.Sprintf("pkg%02d", i),
+			"1.0-1", "1.0-2", "HIGH"))
+	}
+	out := fixplan(t, map[string][]string{"os": {"Debian:12"}}, findings...)
+	if n := strings.Count(out, "\n"); n <= footerThreshold {
+		t.Fatalf("this test needs a report past the footer threshold, got %d lines", n)
+	}
+	if strings.Contains(out, "section(s)") {
+		t.Errorf("the fix plan indexed sections it does not contain:\n%s", out)
+	}
+	// What it repeats instead is its own count of itself, at both ends.
+	if n := strings.Count(out, "affected findings have a fix."); n != 2 {
+		t.Errorf("want the fix summary at both ends, got %d:\n%s", n, out)
+	}
+}
+
+// The section index does not belong in a fix plan, but a caveat does. "Part of
+// the target could not be read" is a fact about the scan and not about the
+// view, and a long report hiding its own header is the entire reason the footer
+// exists -- so dropping writeFooter must not drop that with it.
+func TestTheFixPlanFooterRepeatsTheCaveats(t *testing.T) {
+	var findings []analyze.Finding
+	for i := 0; i < 40; i++ {
+		findings = append(findings, linked(
+			fmt.Sprintf("CVE-2023-%04d", i), fmt.Sprintf("pkg%02d", i),
+			"1.0-1", "1.0-2", "HIGH"))
+	}
+	out := renderFixPlan(&analyze.Result{
+		SchemaVersion: analyze.SchemaVersion,
+		Target:        "debian:12",
+		Mode:          "image",
+		Ecosystems:    []ecosystem.EcosystemResult{{ID: "os", Ecosystems: []string{"Debian:12"}, Components: 1}},
+		Findings:      findings,
+		Unreadable:    &target.Unreadable{Count: 3, Paths: []string{"/var/lib/private"}},
+	})
+	if n := strings.Count(out, "INCOMPLETE: 3 path(s) could not be read"); n != 2 {
+		t.Errorf("want the INCOMPLETE banner at both ends, got %d:\n%s", n, out)
+	}
+}
+
+// A remediation view that silently omits rows is the one kind of report whose
+// shortness reads as good news. Everything the plan declines to plan for gets
+// counted somewhere.
+func TestTheFixPlanAccountsForWhatItDoesNotPlan(t *testing.T) {
+	vexed := linked("CVE-2023-0001", "libc6", "2.36-9", "2.36-9+deb12u1", "HIGH")
+	vexed.VEX = &ecosystem.VEXStatement{Status: "not_affected", Author: "SUSE"}
+	undetermined := linked("CVE-2023-0002", "zlib1g", "1.2.13", "", "UNKNOWN")
+	undetermined.Status = analyze.StatusUndetermined
+
+	out := fixplan(t, map[string][]string{"os": {"Debian:12"}},
+		linked("CVE-2023-0003", "perl-base", "5.36.0-7", "5.36.0-7+deb12u3", "HIGH"),
+		vexed, undetermined)
+
+	lineWith(t, out, "1 already answered by a vendor VEX statement")
+	lineWith(t, out, "1 undetermined finding(s) not shown")
+	// And neither is counted as work: the plan is the one remaining row.
+	lineWith(t, out, "1 of 1 affected findings have a fix.")
+}
+
 func containsRow(lines []string, want string) bool {
 	for _, l := range lines {
 		if strings.Contains(l, want) {

internal/analyze/analyze.go

@@ -1018,7 +1018,7 @@ func (r *advisoryResolver) severities() map[string]severity {
 }
 
 // fixedVersions flattens the fixed versions the resolver fetched into a lookup
-// from advisory id to a per-package map of the version its patch lands in.
+// from advisory id to a per-package set of the versions its patch landed in.
 //
 // It is a nested map because a fixed version is a claim about a package, not
 // about an advisory: one CVE fixed in gcc-12 says nothing about the version
@@ -1027,10 +1027,14 @@ func (r *advisoryResolver) severities() map[string]severity {
 // row sits; a fix cannot, so the package name is kept as a second key and the
 // overlay joins on it.
 //
+// The OSV ecosystem is carried down with the versions rather than left behind,
+// because it is the only thing that says which comparator can order them and
+// the overlay works from Findings, which do not record it. See fixCandidates.
+//
 // Like severities() this costs no network traffic: every affected range was in
 // the records already fetched to decide the findings existed.
-func (r *advisoryResolver) fixedVersions() map[string]map[string]string {
-	out := map[string]map[string]string{}
+func (r *advisoryResolver) fixedVersions() map[string]map[string]fixCandidates {
+	out := map[string]map[string]fixCandidates{}
 	for _, set := range r.cache {
 		for _, adv := range set {
 			if adv == nil || len(adv.Fixed) == 0 {
@@ -1042,18 +1046,21 @@ func (r *advisoryResolver) fixedVersions() map[string]map[string]string {
 				}
 				byPkg, ok := out[key]
 				if !ok {
-					byPkg = map[string]string{}
+					byPkg = map[string]fixCandidates{}
 					out[key] = byPkg
 				}
 				for pkg, fixed := range adv.Fixed {
-					byPkg[pkg] = fixed
+					byPkg[pkg] = fixCandidates{ecosystem: adv.Ecosystem, versions: fixed}
 				}
 			}
 		}
 	}
 	return out
 }
 
+// aliases maps every id an advisory is known by onto that advisory's whole set
+// of ids.
+//
 // It exists for the VEX overlay. A finding names its advisory by whichever id
 // its plugin was working from -- the Go plugin reports GO-2025-3547 -- while a
 // hub files under whichever its own scanner used, which for every hub seen so
@@ -1201,40 +1208,6 @@ func severityOverlay(findings []Finding, sev map[string]severity) {
 	}
 }
 
-// fixedOverlay labels each finding with the version its advisory's patch lands
-// in, in place. It runs beside severityOverlay for the same reason: a plugin
-// answers a presence question and never sees the affected ranges the fix lives
-// in, so the join has to happen where the advisories are.
-//
-// The join is on advisory id and then on package name. The package key is the
-// finding's Package -- the source name OSV files its affected entry against,
-// which is why two binaries built from one source (libgcc-s1, libstdc++6 from
-// gcc-12) both resolve to the same fixed source version. A finding whose
-// advisory published no fix for its package keeps an empty FixedVersion, which
-// the renderer shows as "no fix" rather than a blank.
-func fixedOverlay(findings []Finding, fixed map[string]map[string]string) {
-	for i := range findings {
-		f := &findings[i]
-		for _, key := range []string{f.CVE, f.ID, f.GoID} {
-			if key == "" {
-				continue
-			}
-			byPkg, ok := fixed[key]
-			if !ok {
-				continue
-			}
-			if v, ok := byPkg[f.Package]; ok {
-				f.FixedVersion = v
-				break
-			}
-			if v, ok := byPkg[f.Component()]; ok {
-				f.FixedVersion = v
-				break
-			}
-		}
-	}
-}
-
 // queryNames is the component's OSV names, primary first, deduplicated.
 func queryNames(c ecosystem.Component) []string {
 	var out []string