Recommend the smallest upgrade that clears an advisory, not the highest

The rollup took the highest fix version across a component's advisories, which
is wrong whenever an advisory is patched on more than one release branch. nanoid
is fixed on both 3.3.x and 5.x, and it is transitive under a postcss range of
^3.3.11 that no 5.x release can satisfy - so the recommendation was unreachable
rather than merely large.

Per advisory the target is now the lowest fix above the version in use; per
component, the highest of those. Fixes on older branches are skipped rather than
offered as a downgrade.

scripts/scan_sbom.py

@@ -274,21 +274,43 @@ def version_key(version):
     return parts
 
 
-def upgrade_plan(findings):
+def minimal_fix(fixedin, current):
-    """[(component, version, count)] - one row per component that has a fix.
+    """The SMALLEST published fix above the version in use, or None.
 
-    The highest fix version across that component's advisories, because
+    An advisory patched on several release branches lists a fix per branch:
-    upgrading once to the top of the list clears the ones below it.
+    nanoid's is fixed in both 3.3.18 and 5.1.16. Taking the highest recommends
+    a major upgrade when a patch on the current branch would do - and where the
+    package is transitive, that major may not even be reachable, because the
+    parent's semver range forbids it. The smallest fix above the current
+    version is the one that actually clears the advisory.
+
+    Fixes at or below the current version belong to older branches and are
+    ignored; recommending one would be a downgrade.
+    """
+    above = [v for v in fixedin if version_key(v) > version_key(current)]
+    if not above:
+        return None
+    return min(above, key=version_key)
+
+
+def upgrade_plan(findings):
+    """[(component, version, count, major)] - one row per component with a fix.
+
+    Per advisory, the minimal upgrade that clears it; per component, the
+    highest of those, since one upgrade has to satisfy every advisory against
+    it.
     """
     by_component = {}
     for f in findings:
         if f['allowlisted'] or not f['fixedin']:
             continue
+        target = minimal_fix(f['fixedin'], f['version'])
+        if target is None:
+            continue
         name = f['component']
         best, count, current = by_component.get(name, (None, 0, f['version']))
-        for version in f['fixedin']:
+        if best is None or version_key(target) > version_key(best):
-            if best is None or version_key(version) > version_key(best):
+            best = target
-                best = version
         by_component[name] = (best, count + 1, current)
 
     rows = []
@@ -337,7 +359,7 @@ def report(findings, blocking):
     if ship:
         upgrades = upgrade_plan(ship)
         if upgrades:
-            print('\nWHAT TO UPGRADE - shipped components, highest fix seen')
+            print('\nWHAT TO UPGRADE - shipped components, smallest upgrade that clears them')
             for component, version, count, major in upgrades:
                 note = '  MAJOR - check for breaking changes' if major else ''
                 print('  %-30s -> %-12s clears %-3d%s'

tests/test_scan_sbom.py

@@ -142,6 +142,37 @@ class TestUpgradePlan:
         findings = [self._finding('nanoid 3.3.11', '3.3.11', ['5.1.16'])]
         assert scan.upgrade_plan(findings)[0][3] is True
 
+    def test_a_branch_patch_beats_a_major_on_the_same_advisory(self, scan):
+        # The real case this was found on: nanoid is fixed in BOTH 3.3.18 and
+        # 5.1.16, and it is transitive under a postcss range of ^3.3.11 - so
+        # 5.x is not merely a bigger job, it is unreachable. Recommending the
+        # highest fix sent somebody to a major upgrade they could not make.
+        findings = [self._finding('nanoid 3.3.11', '3.3.11',
+                                  ['3.3.18', '5.1.16'])]
+        component, version, _count, major = scan.upgrade_plan(findings)[0]
+        assert version == '3.3.18'
+        assert major is False
+
+    def test_component_target_satisfies_every_advisory_against_it(self, scan):
+        # Two advisories, minimal fixes 3.3.16 and 3.3.18: the row must be the
+        # higher, or the upgrade clears one and leaves the other.
+        findings = [
+            self._finding('nanoid 3.3.11', '3.3.11', ['3.3.16', '5.1.16']),
+            self._finding('nanoid 3.3.11', '3.3.11', ['3.3.18', '5.1.6']),
+        ]
+        assert scan.upgrade_plan(findings)[0][1] == '3.3.18'
+
+    def test_a_fix_on_an_older_branch_is_never_a_downgrade(self, scan):
+        # axios advisories list fixes on the 0.30.x line too; on 1.13.2 those
+        # are behind us and must not be recommended.
+        findings = [self._finding('axios 1.13.2', '1.13.2',
+                                  ['0.30.2', '1.15.2'])]
+        assert scan.upgrade_plan(findings)[0][1] == '1.15.2'
+
+    def test_only_older_branch_fixes_means_no_row(self, scan):
+        findings = [self._finding('axios 1.13.2', '1.13.2', ['0.30.2'])]
+        assert scan.upgrade_plan(findings) == []
+
     def test_a_patch_bump_is_not_flagged(self, scan):
         findings = [self._finding('pyjwt 2.12.1', '2.12.1', ['2.13.0'])]
         assert scan.upgrade_plan(findings)[0][3] is False