From cd921bad24dc6f76e3b772c1a4a58be9035ddc4e Mon Sep 17 00:00:00 2001 From: woksin Date: Sun, 25 Oct 2026 01:50:32 +0200 Subject: [PATCH 1/6] Skip packages whose version inventory returns 403 in PR cleanup A 403 on one package's version listing no longer fails the whole job: cleanup warns, skips that package without deleting anything from it, and cleans up the rest. Manual apply still fails when a package was skipped, and a 403 outside inventory still fails. --- .../tests/cleanup-pr-artifacts.test.py | 39 ++++++++++++++++++- .github/workflows/cleanup-pr-artifacts.yml | 32 +++++++++++++-- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/.github/scripts/tests/cleanup-pr-artifacts.test.py b/.github/scripts/tests/cleanup-pr-artifacts.test.py index b5196b8..3bd5a26 100644 --- a/.github/scripts/tests/cleanup-pr-artifacts.test.py +++ b/.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): diff --git a/.github/workflows/cleanup-pr-artifacts.yml b/.github/workflows/cleanup-pr-artifacts.yml index 68bc5ea..c45e961 100644 --- a/.github/workflows/cleanup-pr-artifacts.yml +++ b/.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), From b2478d996e47031358a679f213ca4d4b9cce0a0c Mon Sep 17 00:00:00 2001 From: woksin Date: Sun, 25 Oct 2026 01:50:32 +0200 Subject: [PATCH 2/6] Add java-version input to update-packages --- .github/workflows/update-packages.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/update-packages.yml b/.github/workflows/update-packages.yml index 12680e6..5b6a6d1 100644 --- a/.github/workflows/update-packages.yml +++ b/.github/workflows/update-packages.yml @@ -33,7 +33,6 @@ name: Update Packages env: DOTNET_VERSION: "10.0.x" - JAVA_VERSION: "21" ELIXIR_VERSION: "1.18" OTP_VERSION: "27" @@ -74,6 +73,13 @@ on: type: string required: false default: '24' + java-version: + description: >- + JDK version used for Gradle updates. Defaults to 21; callers whose + Gradle toolchain needs another JDK (for example 17) pass it here. + type: string + required: false + default: '21' post-update-command: description: >- Optional trusted Bash command in the caller repository root, after all @@ -302,7 +308,7 @@ jobs: uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1 with: distribution: temurin - java-version: ${{ env.JAVA_VERSION }} + java-version: ${{ inputs.java-version }} - name: Setup Elixir if: steps.detect.outputs.update_mix == 'true' From d99e84b0b2410cc480d8c3415cde3b284641e3b6 Mon Sep 17 00:00:00 2001 From: woksin Date: Sun, 25 Oct 2026 01:50:32 +0200 Subject: [PATCH 3/6] Document cleanup 403 skips and the java-version input --- README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 041ef58..97f0b8a 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ version. Titles such as `Bump Arc from 19.0 to 19.1 and Chronicle to 19.2` and | `branch-prefixes` | `align-versions/` | Space-separated head branch prefixes required for replacements, closure, and branch deletion. An empty list permits none of these. | | `authors` | `github-actions[bot]` | Space-separated trusted bot/PAT logins whose superseded PRs may be closed. | | `dry-run` | `false` | Print planned actions without any writes. | +| `java-version` | `21` | JDK version for Gradle updates. A caller whose Gradle toolchain needs another JDK (for example `jvmToolchain(17)`) passes it here. | | `runs-on` | `ubuntu-latest` | Runner label; use a self-hosted runner for private repositories. | | `workflows-ref` | `main` | Trusted Cratis/Workflows ref providing the program, not a PR head ref. | @@ -501,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. From 4f53b247660a716e68df61edd3f25e009d8e089f Mon Sep 17 00:00:00 2001 From: woksin Date: Sun, 25 Oct 2026 01:56:35 +0200 Subject: [PATCH 4/6] Move java-version row to the update-packages input table --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 97f0b8a..85901d1 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,6 @@ version. Titles such as `Bump Arc from 19.0 to 19.1 and Chronicle to 19.2` and | `branch-prefixes` | `align-versions/` | Space-separated head branch prefixes required for replacements, closure, and branch deletion. An empty list permits none of these. | | `authors` | `github-actions[bot]` | Space-separated trusted bot/PAT logins whose superseded PRs may be closed. | | `dry-run` | `false` | Print planned actions without any writes. | -| `java-version` | `21` | JDK version for Gradle updates. A caller whose Gradle toolchain needs another JDK (for example `jvmToolchain(17)`) passes it here. | | `runs-on` | `ubuntu-latest` | Runner label; use a self-hosted runner for private repositories. | | `workflows-ref` | `main` | Trusted Cratis/Workflows ref providing the program, not a PR head ref. | @@ -93,6 +92,7 @@ dedupe. | `publication-mode` | `direct` | `direct` retains legacy publication; `pull-request` uses the validated-candidate rules below. Other values fail. | | `pull-request-label` | Empty (no label) | One existing, caller-chosen label, only in `pull-request` mode. Must match `[a-zA-Z0-9][a-zA-Z0-9_.:/-]*`. No comma lists, whitespace, or implicit release intent. | | `node-version` | `24` | Node.js version for NPM updates. Satisfies the Cratis JavaScript repositories' engine floors; a caller may override it, but the minimum is Node 22 (the peer sync script needs `fs.globSync` with array `exclude`, available from 22.14/23.7). | +| `java-version` | `21` | JDK version for Gradle updates. A caller whose Gradle toolchain needs another JDK (for example `jvmToolchain(17)`) passes it here. | | `runs-on` | `ubuntu-latest` | Existing runner-label input; private callers should retain their approved self-hosted label. | | `npm-package-exclusions` | `typescript` | Existing comma-delimited exclusions, supplemented by the caller's `.github/update-packages-npm-exclusions`. | | `nuget-package-exclusions` | `StyleCop.Analyzers` (its 1.3.x betas fail the build of repositories on 1.2.0-beta.556; a caller that sets this input replaces the default and lists the package itself to keep it held) | Comma-delimited exact NuGet package IDs, matched case-insensitively after trimming whitespace. Applies to top-level project references, including centrally managed versions in `Directory.Packages.props`; transitive-only IDs warn as not directly referenced (updating a parent can still change transitive versions). Prerelease references retain the updater's prerelease eligibility; stable references do not gain prerelease-only candidates. Invalid/incomplete listings and divergent per-framework versions for eligible packages fail before updating. As with the default path, SDK exit 3 (no changes written) skips NuGet updates for that run. | From 3dabb6db7b7fe86e3f4f502e7ef3462d70f8a8ba Mon Sep 17 00:00:00 2001 From: woksin Date: Sun, 25 Oct 2026 01:58:47 +0200 Subject: [PATCH 5/6] Split the PR cleanup change into its own pull request The package-update rollout must not ship with a change that triggers Bootstrap Common Workflows. --- .../tests/cleanup-pr-artifacts.test.py | 39 +------------------ .github/workflows/cleanup-pr-artifacts.yml | 32 ++------------- 2 files changed, 5 insertions(+), 66 deletions(-) diff --git a/.github/scripts/tests/cleanup-pr-artifacts.test.py b/.github/scripts/tests/cleanup-pr-artifacts.test.py index 3bd5a26..b5196b8 100644 --- a/.github/scripts/tests/cleanup-pr-artifacts.test.py +++ b/.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, 404, 429, 500] + statuses = [404, 429, 500] if failing.startswith("package_type=") else [401, 403, 404, 429, 500] for status in statuses: with self.subTest(failing=failing, status=status): api = FakeAPI() @@ -346,42 +346,7 @@ 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=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 + api.hook = lambda m, e: Response(status=403) if "page=2" in e else None self.assert_failed_without_deletes(self.run_cleanup(api)) def test_all_target_preflight_before_first_delete(self): diff --git a/.github/workflows/cleanup-pr-artifacts.yml b/.github/workflows/cleanup-pr-artifacts.yml index c45e961..68bc5ea 100644 --- a/.github/workflows/cleanup-pr-artifacts.yml +++ b/.github/workflows/cleanup-pr-artifacts.yml @@ -81,9 +81,6 @@ jobs: class InventoryNotAuthorized(CleanupError): pass - class PackageNotAuthorized(CleanupError): - pass - def require(condition, message): if not condition: raise CleanupError(message) @@ -128,12 +125,6 @@ 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 @@ -236,7 +227,7 @@ jobs: "version_name": version_name, "tags": tags, "delete_endpoint": endpoint, "restore_endpoint": endpoint + "/restore"} - def inventory(api, repository, pr, skipped): + def inventory(api, repository, pr): targets = [] for kind in ("container", "nuget"): print(f"Inventorying {kind} packages") @@ -249,14 +240,7 @@ jobs: names.add(name) if association(package) != repository: continue - 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: + for version in api.listing(package_endpoint(kind, name) + "/versions?state=active"): target = target_for(version, kind, name, pr) if target: targets.append(target) @@ -309,9 +293,8 @@ jobs: api = API(token) if manual and not dry_run: verify_closed(api, repository, pr) - skipped = [] try: - plan = inventory(api, repository, pr, skipped) + plan = inventory(api, repository, pr) except InventoryNotAuthorized: message = ("Could not run: package inventory not authorized. No versions deleted. " "Create a classic PAT (fine-grained PATs are not supported) with " @@ -327,15 +310,6 @@ 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), From 014300f973b3a5051d4da335c6409f9a9e700b10 Mon Sep 17 00:00:00 2001 From: woksin Date: Sun, 25 Oct 2026 01:58:56 +0200 Subject: [PATCH 6/6] Keep the cleanup README change with the cleanup pull request --- README.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/README.md b/README.md index 85901d1..d3ad791 100644 --- a/README.md +++ b/README.md @@ -502,10 +502,7 @@ 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 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 +HTTP 401/403 warning described above. 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.