From d1e816ade2d429f3fd687c1c208063b3a7b72355 Mon Sep 17 00:00:00 2001 From: Smarter Harder <33955773+NWarila@users.noreply.github.com> Date: Mon, 20 Jul 2026 14:41:35 -0400 Subject: [PATCH] Guard repository destroys instead of blocking them outright A Terraform 'delete' of github_repository is not necessarily a deletion. archive_on_destroy defaults to TRUE, and the provider's delete path sets archived=true and calls Edit rather than Delete - confirmed in the provider source. So removing a definition ARCHIVES the repository, and the archive-then- delete-in-UI retirement path already existed. prevent_destroy is therefore NOT added: it would block the archive too, replacing a working retirement path with a hard plan error. It also cannot help decide the interesting cases, being a lifecycle meta-argument that must be a literal and cannot read the before-state. The plan shows 'delete' for both an archive and a real deletion, so the plan alone cannot tell them apart. The guard reads the before-state and splits them: REFUSED archive_on_destroy = false the only irreversible path. Nothing sets it today. REFUSED repository not already archived archiving as a SIDE EFFECT of a definition going missing - a bug signature, not a retirement. ALLOWED already-archived repo leaving provider no-ops, state forgets it. The last step of a deliberate retirement; blocking it would make retirement impossible. REPORTED non-repository destroys rulesets and environments churn legitimately, but a destroy should never scroll past unread. The premature-archive rule is the one that matters. On the-hero-wars-guys a dotfile-blind glob dropped a definitions file and the plan proposed to archive .github - archived=false, public, the org profile repo. Archiving it would have made it read-only and stopped its Actions org-wide. tools/test_destroy_guard.py extracts the guard's python from the workflow and runs it, so the tested code is the shipped code rather than a copy that can drift. 12 tests including the real incident plan shape, plus two edge cases that decide ambiguity in opposite directions: an absent archive_on_destroy falls to the provider default (allow), a null before-state is unknown (refuse). Gate: 12/12 destroy-guard, assemble-guard passing, 93/93 terraform, fmt clean. --- .../workflows/reusable-terraform-deploy.yaml | 86 ++++++++ .gitignore | 1 + Makefile | 4 + terraform/41-resources-github.tf | 12 ++ tools/test_destroy_guard.py | 189 ++++++++++++++++++ 5 files changed, 292 insertions(+) create mode 100644 tools/test_destroy_guard.py diff --git a/.github/workflows/reusable-terraform-deploy.yaml b/.github/workflows/reusable-terraform-deploy.yaml index 1b1b669..d9477f2 100644 --- a/.github/workflows/reusable-terraform-deploy.yaml +++ b/.github/workflows/reusable-terraform-deploy.yaml @@ -518,6 +518,92 @@ jobs: # (sensitive TF_VARs already render as "(sensitive value)".) terraform show -json tfplan > plan.json + - name: Guard repository destroys + working-directory: framework/terraform + run: | + set -euo pipefail + # A Terraform "delete" of github_repository is NOT necessarily a + # deletion. `archive_on_destroy` defaults to true, and the provider's + # delete path then archives the repository instead - so removing a + # definition retires it, which is the intended behaviour. The plan + # shows `delete` either way, so the plan alone cannot tell you which + # you are about to do. This step can, because it reads the before-state. + # + # Refused: + # archive_on_destroy = false -> a genuine, irreversible deletion. + # repository not already archived -> archiving as a SIDE EFFECT of a + # definition going missing. That is the signature of a bug, not a + # retirement: a dotfile-blind glob dropped a definitions file, the + # repository stopped being declared, and the plan proposed to + # archive a live public org-profile repository. + # + # Allowed: + # already-archived repository leaving the config -> the provider + # no-ops and state simply forgets it. This is the last step of a + # deliberate retirement. + python3 - <<'PY' + import json, sys + + with open("plan.json", encoding="utf-8") as fh: + plan = json.load(fh) + + deletions, premature, retiring, other = [], [], [], [] + for rc in plan.get("resource_changes", []): + if "delete" not in rc.get("change", {}).get("actions", []): + continue + addr = rc.get("address", "") + if rc.get("type") != "github_repository": + other.append(addr) + continue + before = rc.get("change", {}).get("before") or {} + if not before.get("archive_on_destroy", True): + deletions.append(addr) + elif not before.get("archived", False): + premature.append(addr) + else: + retiring.append(addr) + + if other: + # Dependent resources legitimately churn, but a destroy should + # never scroll past unread. + print(f"note: {len(other)} non-repository resource(s) will be destroyed:") + for a in sorted(other)[:20]: + print(f" {a}") + print() + + for addr in sorted(retiring): + print(f"ok: {addr} is already archived; leaving management is a no-op on GitHub.") + + if deletions: + print("::error::REFUSING TO DELETE REPOSITORIES.") + for addr in sorted(deletions): + print(f"::error:: {addr} (archive_on_destroy = false)") + print() + print("Terraform does not delete repositories here. Remove") + print("`archive_on_destroy: false` from the repository's YAML; retirement is") + print("`archived: true`, then a deliberate delete in the GitHub UI.") + + if premature: + print("::error::REFUSING TO ARCHIVE REPOSITORIES THAT WERE NOT ARCHIVED FIRST.") + for addr in sorted(premature): + print(f"::error:: {addr}") + print() + print("These would be archived only because their definition is no longer") + print("being read. If that was not intended, check the assembly step's file") + print("counts - a definition silently going missing is the known cause of") + print("this plan shape, and archiving a live repository breaks its CI.") + print() + print("To retire a repository deliberately: set `archived: true` and apply,") + print("THEN remove its YAML. This step allows that second removal.") + + if deletions or premature: + sys.exit(1) + + if not retiring: + print("No repository destroys in this plan.") + PY + + # Publishes on EVERY run, not just plan_only. The plan render is suppressed # from the (public) Actions log, so without this a workflow_dispatch plan # (apply=false) — the pre-apply review path — produced NO reviewable output diff --git a/.gitignore b/.gitignore index a98b841..cde64bb 100644 --- a/.gitignore +++ b/.gitignore @@ -194,6 +194,7 @@ !/tools/ !/tools/check_docs_layout.py !/tools/test_assemble_guard.sh +!/tools/test_destroy_guard.py # Contract files (seeded by seed_consumer.py) !/terraform/locals.tf diff --git a/Makefile b/Makefile index d644e18..2b04338 100644 --- a/Makefile +++ b/Makefile @@ -39,6 +39,9 @@ opa-test: echo "no OPA policies to test"; \ fi +destroy-guard-test: + $(PYTHON) tools/test_destroy_guard.py + assemble-guard-test: bash tools/test_assemble_guard.sh @@ -52,3 +55,4 @@ ci: $(MAKE) docs-check $(MAKE) opa-test $(MAKE) assemble-guard-test + $(MAKE) destroy-guard-test diff --git a/terraform/41-resources-github.tf b/terraform/41-resources-github.tf index 22cd272..ecb0569 100644 --- a/terraform/41-resources-github.tf +++ b/terraform/41-resources-github.tf @@ -205,6 +205,18 @@ resource "github_repository" "repo" { # check repo-scoped invariants so their errors point at the specific # github_repository.repo[] address. lifecycle { + # NOTE: prevent_destroy is deliberately NOT set here. `archive_on_destroy` + # defaults to true, and the provider's delete path archives the repository + # rather than deleting it — so removing a definition retires the repository + # instead of destroying it, which is the intended behaviour. prevent_destroy + # would block that archive too, replacing a working retirement path with a + # hard plan error. + # + # Genuine deletion, and archiving a repository that was not deliberately + # archived first, are both refused by the destroy guard in + # .github/workflows/reusable-terraform-deploy.yaml. That guard can read the + # plan's before-state, which a lifecycle meta-argument cannot. + # `auto_init` and `license_template` are CREATE-time only; ignoring # them prevents spurious diff after the initial create. `allow_forking` # defaults to true for public, false for internal and organization-owned diff --git a/tools/test_destroy_guard.py b/tools/test_destroy_guard.py new file mode 100644 index 0000000..1157a40 --- /dev/null +++ b/tools/test_destroy_guard.py @@ -0,0 +1,189 @@ +#!/usr/bin/env python3 +"""Tests for the repository destroy guard. + +A Terraform "delete" of `github_repository` is not necessarily a deletion. +`archive_on_destroy` defaults to true, so the provider's delete path archives the +repository instead — verified in the provider source, where the delete function +sets `archived = true` and calls Edit rather than Delete. The plan shows `delete` +either way, which is precisely why a guard reading the before-state is needed: +the plan alone cannot distinguish retirement from deletion. + +The premature-archive fixture is the real plan shape from the-hero-wars-guys, +where a dotfile-blind glob dropped a definitions file and the plan proposed to +archive a live public org-profile repository. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +WORKFLOW = ROOT / ".github" / "workflows" / "reusable-terraform-deploy.yaml" + + +def extract_guard() -> str: + """Pull the guard's python out of the workflow so the shipped code is tested. + + Copying the logic into the test would let the two drift apart, and a guard + that is not the one actually running is not evidence of anything. + """ + text = WORKFLOW.read_text(encoding="utf-8") + marker = "Guard repository destroys" + assert marker in text, "guard step is missing from the workflow" + body = text.split(marker, 1)[1] + start = body.index("python3 - <<'PY'") + len("python3 - <<'PY'") + end = body.index("\n PY", start) + return "\n".join(line[10:] for line in body[start:end].splitlines()) + + +GUARD = extract_guard() + + +def run_guard(resource_changes: list[dict]) -> tuple[int, str]: + with tempfile.TemporaryDirectory() as tmp: + (Path(tmp) / "plan.json").write_text(json.dumps({"resource_changes": resource_changes})) + (Path(tmp) / "guard.py").write_text(GUARD) + proc = subprocess.run( + [sys.executable, "guard.py"], + cwd=tmp, + env={"PATH": "/usr/bin:/bin"}, + capture_output=True, + text=True, + ) + return proc.returncode, proc.stdout + proc.stderr + + +def repo_delete(name: str, *, archived: bool, archive_on_destroy: bool = True) -> dict: + return { + "type": "github_repository", + "index": name, + "address": f'github_repository.repo["{name}"]', + "change": { + "actions": ["delete"], + "before": {"archived": archived, "archive_on_destroy": archive_on_destroy}, + }, + } + + +def dependent_delete(addr: str, rtype: str) -> dict: + return {"type": rtype, "address": addr, "change": {"actions": ["delete"], "before": {}}} + + +class GenuineDeletionTests(unittest.TestCase): + def test_archive_on_destroy_false_is_refused(self): + # The only path to an irreversible deletion. + code, out = run_guard([repo_delete("doomed", archived=True, archive_on_destroy=False)]) + self.assertEqual(code, 1) + self.assertIn("REFUSING TO DELETE REPOSITORIES", out) + self.assertIn("archive_on_destroy = false", out) + + def test_refused_even_when_repository_is_already_archived(self): + code, _ = run_guard([repo_delete("doomed", archived=True, archive_on_destroy=False)]) + self.assertEqual(code, 1) + + +class PrematureArchiveTests(unittest.TestCase): + def test_the_real_incident_plan_is_blocked(self): + # the-hero-wars-guys: .github was archived=false and live. + changes = [ + repo_delete(".github", archived=False), + dependent_delete('github_branch_default.default[".github"]', "github_branch_default"), + dependent_delete('github_repository_file.codeowners[".github"]', "github_repository_file"), + ] + [ + dependent_delete(f'github_repository_ruleset.branch[".github-rules-{i}"]', + "github_repository_ruleset") + for i in range(3) + ] + code, out = run_guard(changes) + self.assertEqual(code, 1) + self.assertIn("NOT ARCHIVED FIRST", out) + self.assertIn(".github", out) + + def test_error_points_at_the_likely_cause(self): + code, out = run_guard([repo_delete("live-repo", archived=False)]) + self.assertEqual(code, 1) + self.assertIn("no longer", out) + self.assertIn("assembly step", out) + + def test_there_is_no_override_environment_variable(self): + import os + + for candidate in ("ALLOW_DESTROY", "FORCE", "ALLOW_ARCHIVE"): + os.environ[candidate] = "1" + try: + code, _ = run_guard([repo_delete("live-repo", archived=False)]) + finally: + for candidate in ("ALLOW_DESTROY", "FORCE", "ALLOW_ARCHIVE"): + os.environ.pop(candidate, None) + self.assertEqual(code, 1) + + +class DeliberateRetirementTests(unittest.TestCase): + def test_already_archived_repository_may_leave_management(self): + # Second step of a deliberate retirement: the provider no-ops and state + # forgets it. Blocking this would make retirement impossible. + code, out = run_guard([repo_delete("retired", archived=True)]) + self.assertEqual(code, 0) + self.assertIn("already archived", out) + + def test_mixed_plan_blocks_only_the_premature_one(self): + code, out = run_guard( + [repo_delete("retired", archived=True), repo_delete("live-repo", archived=False)] + ) + self.assertEqual(code, 1) + self.assertIn("live-repo", out) + + +class OtherResourceTests(unittest.TestCase): + def test_dependent_resource_destroys_are_reported_not_blocked(self): + # Rulesets and environments churn on ordinary edits. + code, out = run_guard( + [dependent_delete('github_repository_ruleset.branch["x-rules-0"]', + "github_repository_ruleset")] + ) + self.assertEqual(code, 0) + self.assertIn("non-repository resource(s) will be destroyed", out) + + def test_clean_plan_passes(self): + code, out = run_guard([]) + self.assertEqual(code, 0) + self.assertIn("No repository destroys", out) + + +class EdgeCaseTests(unittest.TestCase): + def test_replace_counts_as_a_destroy(self): + change = repo_delete("recreated", archived=False) + change["change"]["actions"] = ["delete", "create"] + code, _ = run_guard([change]) + self.assertEqual(code, 1) + + def test_missing_archive_on_destroy_is_treated_as_the_provider_default(self): + # Absent from before-state means the provider default (true) applies; + # assuming deletion here would block ordinary retirements. + change = { + "type": "github_repository", + "index": "r", + "address": 'github_repository.repo["r"]', + "change": {"actions": ["delete"], "before": {"archived": True}}, + } + code, _ = run_guard([change]) + self.assertEqual(code, 0) + + def test_null_before_state_is_not_treated_as_permission(self): + change = { + "type": "github_repository", + "index": "r", + "address": 'github_repository.repo["r"]', + "change": {"actions": ["delete"], "before": None}, + } + code, _ = run_guard([change]) + self.assertEqual(code, 1) # unknown archived state -> premature + + +if __name__ == "__main__": + unittest.main(verbosity=2)