Skip packages whose inventory returns 403 in PR cleanup

.github/scripts/tests/cleanup-pr-artifacts.test.py

@@ -308,7 +308,7 @@ def test_package_inventory_later_page_not_authorized_discards_partial_inventory(
 
     def test_listing_failures_in_each_family_and_level(self):
         for failing in ["package_type=container", "package_type=nuget", "container/image/versions?", "nuget/Cratis.Test/versions?"]:
-            statuses = [404, 429, 500] if failing.startswith("package_type=") else [401, 403, 404, 429, 500]
+            statuses = [404, 429, 500] if failing.startswith("package_type=") else [401, 404, 429, 500]
             for status in statuses:
                 with self.subTest(failing=failing, status=status):
                     api = FakeAPI()
@@ -346,7 +346,42 @@ def test_bad_pagination_and_repeated_ids(self):
     def test_page_two_failure_prevents_deletion(self):
         api = FakeAPI()
         api.versions[("nuget", "Cratis.Test")] = [version(i) for i in range(1, 102)]
-        api.hook = lambda m, e: Response(status=403) if "page=2" in e else None
+        api.hook = lambda m, e: Response(status=500) if "page=2" in e else None
+        self.assert_failed_without_deletes(self.run_cleanup(api))
+
+    def unauthorized_package_api(self, failing):
+        api = FakeAPI()
+        api.packages["container"] = [package("container", "image")]
+        api.versions[("container", "image")] = [version(7, tags=["pr42"])] + [
+            version(i, tags=["other"]) for i in range(1000, 1100)]
+        api.hook = lambda m, e: Response(status=403, raw=TOKEN.encode()) if failing in e else None
+        return api
+
+    def test_package_version_inventory_403_skips_only_that_package(self):
+        for failing in ["container/image/versions?", "container/image/versions?state=active&per_page=100&page=2"]:
+            for mode in ["reusable", "dry-run"]:
+                with self.subTest(failing=failing, mode=mode):
+                    api = self.unauthorized_package_api(failing)
+                    status, plan, output, api = self.run_cleanup(
+                        api, manual=mode == "dry-run",
+                        expected_summary="Skipped 1 package(s) whose version inventory was not authorized")
+                    self.assertEqual(status, 0, output)
+                    self.assertIn("::warning::Skipped container package image", output)
+                    self.assertNotIn("::error::", output)
+                    self.assertEqual([t["package_name"] for t in plan["targets"]], ["Cratis.Test"])
+                    self.assertTrue(all("container" not in e for e in api.deletes))
+                    self.assertEqual(len(api.deletes), 0 if mode == "dry-run" else 1)
+
+    def test_manual_apply_with_skipped_package_fails_without_deletion(self):
+        approved = self.approved()
+        api = self.unauthorized_package_api("container/image/versions?")
+        result = self.run_cleanup(api, manual=True, **approved)
+        self.assert_failed_without_deletes(result)
+        self.assertIn("::error::Skipped 1 package(s)", result[2])
+
+    def test_package_403_outside_inventory_still_fails(self):
+        api = FakeAPI()
+        api.hook = lambda m, e: Response(status=403) if m == "GET" and e.endswith("nuget/Cratis.Test") else None
         self.assert_failed_without_deletes(self.run_cleanup(api))
 
     def test_all_target_preflight_before_first_delete(self):

.github/workflows/cleanup-pr-artifacts.yml

@@ -81,6 +81,9 @@ jobs:
           class InventoryNotAuthorized(CleanupError):
               pass
 
+          class PackageNotAuthorized(CleanupError):
+              pass
+
           def require(condition, message):
               if not condition:
                   raise CleanupError(message)
@@ -125,6 +128,12 @@ jobs:
                       if (method == "GET" and endpoint.split("?", 1)[0] == "orgs/Cratis/packages"
                               and status in (401, 403)):
                           raise InventoryNotAuthorized("Package inventory not authorized")
+                      # A 403 on one package or its versions means the token cannot see that
+                      # package; inventory skips it. Outside inventory it still fails.
+                      if (method == "GET" and status == 403
+                              and re.fullmatch(r"orgs/Cratis/packages/[^/?]+/[^/?]+(/versions)?",
+                                               endpoint.split("?", 1)[0])):
+                          raise PackageNotAuthorized(f"API {method} failed (HTTP {status})")
                       require(status in allowed, f"API {method} failed (HTTP {status})")
                       # Never log API bodies, headers, URLs from errors, or exception text.
                       data = parse_json(response.read()) if status == 200 else None
@@ -227,7 +236,7 @@ jobs:
                       "version_name": version_name, "tags": tags,
                       "delete_endpoint": endpoint, "restore_endpoint": endpoint + "/restore"}
 
-          def inventory(api, repository, pr):
+          def inventory(api, repository, pr, skipped):
               targets = []
               for kind in ("container", "nuget"):
                   print(f"Inventorying {kind} packages")
@@ -240,7 +249,14 @@ jobs:
                       names.add(name)
                       if association(package) != repository:
                           continue
-                      for version in api.listing(package_endpoint(kind, name) + "/versions?state=active"):
+                      try:
+                          versions = api.listing(package_endpoint(kind, name) + "/versions?state=active")
+                      except PackageNotAuthorized:
+                          skipped.append(f"{kind}/{name}")
+                          print(f"::warning::Skipped {kind} package {name}: version inventory not "
+                                "authorized (HTTP 403). None of its versions were deleted.")
+                          continue
+                      for version in versions:
                           target = target_for(version, kind, name, pr)
                           if target:
                               targets.append(target)
@@ -293,8 +309,9 @@ jobs:
               api = API(token)
               if manual and not dry_run:
                   verify_closed(api, repository, pr)
+              skipped = []
               try:
-                  plan = inventory(api, repository, pr)
+                  plan = inventory(api, repository, pr, skipped)
               except InventoryNotAuthorized:
                   message = ("Could not run: package inventory not authorized. No versions deleted. "
                              "Create a classic PAT (fine-grained PATs are not supported) with "
@@ -310,6 +327,15 @@ jobs:
                       raise CleanupError(message)
                   print(f"::warning::{message}")
                   return
+              if skipped:
+                  message = (f"Skipped {len(skipped)} package(s) whose version inventory was not "
+                             f"authorized (HTTP 403): {', '.join(skipped)}. Their versions were not "
+                             "deleted; give PACKAGE_CLEANUP_TOKEN admin access to them to clean them up.")
+                  if os.environ.get("GITHUB_STEP_SUMMARY"):
+                      with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as summary:
+                          summary.write(f"### PR package cleanup\n\n{message}\n")
+                  if expected is not None:
+                      raise CleanupError(message)
               if expected is not None:
                   # Comparing canonical JSON also distinguishes booleans from numeric IDs.
                   require(json.dumps(expected, sort_keys=True) == json.dumps(plan, sort_keys=True),

README.md

@@ -502,7 +502,10 @@ caller. Callers with an explicit secrets map must add
 that pass only `PAT_WORKFLOWS` are unaffected.
 
 Permission failures remain blocking except for the organization inventory
-HTTP 401/403 warning described above. HTTP 401 also covers an expired or revoked
+HTTP 401/403 warning described above and an HTTP 403 on one package's version
+inventory: cleanup warns, skips that package without deleting any of its
+versions, and cleans up the others. Manual apply fails instead when a package was
+skipped. HTTP 401 also covers an expired or revoked
 token: it now produces a warning in reusable cleanup calls and dry runs, but
 manual apply and other workflows using that secret still fail.