diff --git a/.github/scripts/dependency_age.py b/.github/scripts/dependency_age.py index a08a7ed8b7e..d67f1fdcffa 100644 --- a/.github/scripts/dependency_age.py +++ b/.github/scripts/dependency_age.py @@ -501,6 +501,12 @@ def is_instrumentation_path(relative_path: str) -> bool: # build summary of reverted/downgraded dependencies for PR descriptions # path_filter restricts the summary to lockfiles whose relative path matches, # so each PR (core vs instrumentation) only lists the dependencies it actually changes +# +# The summary is split into three on-screen sections, ordered by how urgently a human +# must act on them: +# 1. "Cannot verify age, reverted" — age could not be checked at all, needs manual resolution +# 2. "h cooldown, reverted" — too new and no older eligible version exists, so reverted to baseline +# 3. "h cooldown, updated to the previous version" — too new, downgraded to an older eligible version def build_validation_summary( *, violations_by_file: dict[str, list[tuple[str, str, int]]], @@ -509,34 +515,60 @@ def build_validation_summary( min_age_hours: int, path_filter: Callable[[str], bool], ) -> str: - header = ["## Dependency age policy", ""] - lines = list(header) + unverified: list[str] = [] + cooldown_reverted: list[str] = [] + cooldown_updated: list[str] = [] seen: set[str] = set() + for relative_path, replacements in replacements_by_file.items(): if not path_filter(relative_path): continue baseline_coords = baseline_lockfiles.get(relative_path, set()) for old_gav, (new_gav, hours_remaining) in replacements.items(): - if old_gav not in seen: - seen.add(old_gav) - if new_gav in baseline_coords: - lines.append(f"- `{old_gav}` is {hours_remaining}h away from meeting {min_age_hours}h cooldown, reverted") - else: - new_version = new_gav.rsplit(":", 1)[1] - lines.append(f"- `{old_gav}` is {hours_remaining}h away from meeting {min_age_hours}h cooldown, updated to `{new_version}`") + if old_gav in seen: + continue + seen.add(old_gav) + if new_gav in baseline_coords: + # the only eligible version was the baseline, so this is effectively a revert + cooldown_reverted.append(f"- `{old_gav}` is {hours_remaining}h away from meeting cooldown") + else: + new_version = new_gav.rsplit(":", 1)[1] + cooldown_updated.append(f"- `{old_gav}` is {hours_remaining}h away from meeting cooldown, updated to `{new_version}`") for relative_path, entries in violations_by_file.items(): if not path_filter(relative_path): continue for gav, kind, hours_remaining in entries: - if gav not in seen: - seen.add(gav) - if kind == "unverified": - lines.append(f"- `{gav}` — cannot verify age, reverted") - else: - lines.append(f"- `{gav}` is {hours_remaining}h away from meeting {min_age_hours}h cooldown, reverted") - if len(lines) == len(header): # nothing matched the filter + if gav in seen: + continue + seen.add(gav) + if kind == "unverified": + unverified.append(f"- `{gav}`") + else: + cooldown_reverted.append(f"- `{gav}` is {hours_remaining}h away from meeting cooldown") + + blocks: list[str] = [] + if unverified: + blocks.append( + "### :warning: Cannot verify age, reverted\n\n" + "The age of these dependencies could not be verified, so the lockfiles were reverted. " + "**This needs to be resolved manually.**\n\n" + + "\n".join(sorted(unverified)) + ) + if cooldown_reverted: + blocks.append( + f"### {min_age_hours}h cooldown, reverted\n\n" + "Too new and no older eligible version exists, so the lockfiles were reverted to the baseline.\n\n" + + "\n".join(sorted(cooldown_reverted)) + ) + if cooldown_updated: + blocks.append( + f"### {min_age_hours}h cooldown, updated to the previous version\n\n" + "Too new, so an older eligible version was used instead.\n\n" + + "\n".join(sorted(cooldown_updated)) + ) + if not blocks: # nothing matched the filter return "" - return "\n".join(lines) + return "## Dependency age policy\n\n" + "\n\n".join(blocks) # replace specific coordinates in lockfiles (for version downgrades) diff --git a/.github/scripts/tests/test_dependency_age.py b/.github/scripts/tests/test_dependency_age.py index 6cd2bd2cf7f..6813497e11a 100644 --- a/.github/scripts/tests/test_dependency_age.py +++ b/.github/scripts/tests/test_dependency_age.py @@ -424,6 +424,64 @@ def test_summary_is_empty_when_filter_matches_nothing(self) -> None: ) self.assertEqual(empty, "") + def test_summary_groups_outcomes_into_sections(self) -> None: + # one of each outcome: unverified, too-new revert, replacement-as-revert, replacement-as-update + summary = dependency_age.build_validation_summary( + violations_by_file={ + "core/gradle.lockfile": [ + ("com.example:unverified-lib:1.0.0", "unverified", 0), + ("com.example:too-new-lib:2.0.0", "too_new", 5), + ], + }, + replacements_by_file={ + "core/gradle.lockfile": { + # eligible version equals the baseline -> effectively a revert + "com.example:revert-lib:3.0.0": ("com.example:revert-lib:2.9.0", 7), + # eligible version is newer than the baseline -> an update to a previous version + "com.example:update-lib:4.0.0": ("com.example:update-lib:3.9.0", 9), + }, + }, + baseline_lockfiles={ + "core/gradle.lockfile": {"com.example:revert-lib:2.9.0"}, + }, + min_age_hours=48, + path_filter=lambda p: True, + ) + + # three section headings present, in priority order + unverified_idx = summary.index("### :warning: Cannot verify age, reverted") + reverted_idx = summary.index("### 48h cooldown, reverted") + updated_idx = summary.index("### 48h cooldown, updated to the previous version") + self.assertLess(unverified_idx, reverted_idx) + self.assertLess(reverted_idx, updated_idx) + + # unverified entry lives under the manual-resolution section + self.assertIn("**This needs to be resolved manually.**", summary) + self.assertIn("- `com.example:unverified-lib:1.0.0`", summary) + + # both the too-new violation and the revert-style replacement land in the reverted section + reverted_block = summary[reverted_idx:updated_idx] + self.assertIn("com.example:too-new-lib:2.0.0", reverted_block) + self.assertIn("com.example:revert-lib:3.0.0", reverted_block) + + # the update names the older version that was used instead + updated_block = summary[updated_idx:] + self.assertIn("com.example:update-lib:4.0.0", updated_block) + self.assertIn("updated to `3.9.0`", updated_block) + + def test_summary_omits_empty_sections(self) -> None: + # only too-new violations -> only the "reverted" section should appear + summary = dependency_age.build_validation_summary( + violations_by_file={"core/gradle.lockfile": [("com.example:core-lib:2.0.0", "too_new", 5)]}, + replacements_by_file={}, + baseline_lockfiles={}, + min_age_hours=48, + path_filter=lambda p: True, + ) + self.assertIn("### 48h cooldown, reverted", summary) + self.assertNotIn("Cannot verify age", summary) + self.assertNotIn("updated to the previous version", summary) + if __name__ == "__main__": unittest.main()