From f7791cfa2efeb22e3f533dd30feee49a7a17a8ad Mon Sep 17 00:00:00 2001 From: Sujeito Operator Date: Fri, 28 Aug 2026 06:27:52 +0200 Subject: [PATCH] fix(ci): follow bounded apt-get into composite actions, and cap it at the caller Rebased onto main. The desktop Swift lanes that held this approved PR were red on main itself; #12277 fixed both causes, so this rebase is what turns them green. The checks-manifest edit is re-applied to main's text rather than overwriting it, so the legacy-memory-surface-ratchet entry main added since the merge base survives. One prose fix in the reason field: #12194 is still open, so it proposes the composite action rather than having moved it. --- .github/checks-manifest.yaml | 4 +- .../check_workflow_apt_network_bounds.py | 140 +++++++++++++++++- .../test_check_workflow_apt_network_bounds.py | 115 ++++++++++++++ 3 files changed, 253 insertions(+), 6 deletions(-) mode change 100755 => 100644 .github/scripts/check_workflow_apt_network_bounds.py diff --git a/.github/checks-manifest.yaml b/.github/checks-manifest.yaml index e74924ebb52..4124343d571 100644 --- a/.github/checks-manifest.yaml +++ b/.github/checks-manifest.yaml @@ -36,9 +36,9 @@ checks: reason: "#9440: local CI prediction must select only bounded Flutter generator, desktop-flow, and Windows KG-worker checks for the final PR diff" - id: workflow-apt-network-bounds command: ["bash", "scripts/run-workflow-apt-network-bounds.sh"] - triggers: [".github/workflows/**", ".github/scripts/check_workflow_apt_network_bounds.py", ".github/scripts/test_check_workflow_apt_network_bounds.py", "scripts/run-workflow-apt-network-bounds.sh", ".github/checks-manifest.yaml"] + triggers: [".github/workflows/**", ".github/actions/**", ".github/scripts/check_workflow_apt_network_bounds.py", ".github/scripts/test_check_workflow_apt_network_bounds.py", "scripts/run-workflow-apt-network-bounds.sh", ".github/checks-manifest.yaml"] lanes: ["local", "ci"] - reason: "apt has no built-in timeout; a stalled Azure mirror burns the job ceiling and reports an opaque cancellation. #11872 bounded two workflows by hand and still left an install line unbounded under a comment claiming otherwise" + reason: "apt has no built-in timeout; a stalled Azure mirror burns the job ceiling and reports an opaque cancellation. #11872 bounded two workflows by hand and still left an install line unbounded under a comment claiming otherwise. #12194 proposes moving a bounded install into a composite action, so the guard follows apt into .github/actions/** and enforces the step ceiling at each caller, where timeout-minutes is a valid key" - id: pre-push-hatch-disclosure command: ["python3", ".github/scripts/check_pre_push_hatch_disclosure.py"] triggers: ["scripts/pre-push", ".github/scripts/check_pre_push_hatch_disclosure.py", ".github/checks-manifest.yaml"] diff --git a/.github/scripts/check_workflow_apt_network_bounds.py b/.github/scripts/check_workflow_apt_network_bounds.py old mode 100755 new mode 100644 index 680a65b2037..0ea106ccc8c --- a/.github/scripts/check_workflow_apt_network_bounds.py +++ b/.github/scripts/check_workflow_apt_network_bounds.py @@ -18,6 +18,24 @@ mirror fails over quickly instead of hanging. * `timeout-minutes` on the enclosing step, as a backstop for anything the acquire options do not cover (dpkg, a debconf prompt, a wedged post-install). + +COMPOSITE ACTIONS ARE IN SCOPE TOO, AND THE SECOND REQUIREMENT MOVES WHEN THEY ARE. +A bounded apt call may now legitimately live in `.github/actions/*/action.yml` rather +than in a workflow - #12194 moves the hermetic-gauntlet redis install into +`.github/actions/install-redis-server` so three jobs share one copy. Scanning only +`.github/workflows/*.yml` would leave that home unguarded, so "a new one cannot be +added unbounded" would quietly stop being true for the only place the bounded calls +still live. + +Pointing the old checker at an `action.yml` would NOT have caught it either: a +composite action keeps its steps under `runs.steps`, not `jobs..steps`, so +`check_workflow` finds no steps and returns clean. The blind spot fails open. + +`timeout-minutes` is not a valid key on a composite-action step - GitHub rejects the +workflow that uses one - so the ceiling cannot be enforced where the apt line now is. +It is enforced at every call site instead (`check_workflow_callers`). That split is +the whole point: without it, moving an install into an action would launder away the +backstop while the guard still reported success. """ from __future__ import annotations @@ -92,23 +110,137 @@ def check_workflow(path: Path) -> list[str]: return problems +def _composite_steps(document: dict): + """Yield the steps of a composite action. Other action kinds have no `run:` steps. + + A composite action's steps live under `runs.steps`, not `jobs..steps`, so + `_iter_steps` yields nothing for one. That is why pointing the workflow checker at an + `action.yml` returns clean rather than complaining: the shape simply does not match. + """ + runs = document.get("runs") + if not isinstance(runs, dict) or runs.get("using") != "composite": + return + for index, step in enumerate(runs.get("steps") or []): + if isinstance(step, dict): + yield index, step + + +def check_action(path: Path) -> list[str]: + """Composite actions: require the acquire bounds, but NOT `timeout-minutes`. + + `timeout-minutes` is not a valid key on a composite-action step - GitHub rejects the + workflow that uses it. The backstop can therefore only live on the CALLER, which is + what `check_workflow_callers` enforces. Requiring it here would be unsatisfiable, and + a guard nobody can satisfy gets deleted rather than obeyed. + """ + try: + document = yaml.safe_load(path.read_text(encoding="utf-8")) + except yaml.YAMLError as error: # pragma: no cover - malformed YAML is another check's job + return [f"{path}: could not parse ({error.__class__.__name__})"] + if not isinstance(document, dict): + return [] + + problems: list[str] = [] + for index, step in _composite_steps(document): + script = step.get("run") + if not isinstance(script, str): + continue + label = step.get("name") or f"step #{index}" + for line in _network_apt_lines(script): + missing = [option for option in REQUIRED_OPTIONS if option not in line] + if missing: + problems.append( + f"{path}: {label!r}: apt-get network call is unbounded " + f"(missing {', '.join(missing)}): {line}" + ) + return problems + + +def _action_reference(repo_root: Path, action_path: Path) -> str: + """The `uses:` string a workflow writes to call this local action.""" + return "./" + action_path.parent.relative_to(repo_root).as_posix() + + +def check_workflow_callers(path: Path, apt_action_refs: set[str]) -> list[str]: + """Every step that `uses:` an apt-running local action must cap itself. + + This is the other half of the bound. The acquire options are enforced inside the + action; the ceiling cannot be, so it is enforced at each call site. Without this the + guard would report success on a tree where an unbounded-ceiling apt install had simply + been moved one file down. + """ + if not apt_action_refs: + return [] + try: + document = yaml.safe_load(path.read_text(encoding="utf-8")) + except yaml.YAMLError as error: # pragma: no cover - malformed YAML is another check's job + return [f"{path}: could not parse ({error.__class__.__name__})"] + if not isinstance(document, dict): + return [] + + problems: list[str] = [] + for job_name, index, step in _iter_steps(document): + uses = step.get("uses") + if not isinstance(uses, str) or uses.rstrip("/") not in apt_action_refs: + continue + if "timeout-minutes" not in step: + label = step.get("name") or f"step #{index}" + problems.append( + f"{path}: job {job_name!r}, {label!r}: uses {uses}, which runs apt-get over " + f"the network, but declares no timeout-minutes backstop" + ) + return problems + + def main(argv: list[str]) -> int: - root = Path(argv[1]) if len(argv) > 1 else Path(".github/workflows") - paths = sorted(p for p in root.glob("*.yml")) + sorted(p for p in root.glob("*.yaml")) + # The argument is the REPOSITORY ROOT, because the check now spans two directories. + # The historical form - a path to the workflows directory - is still accepted so an + # existing caller does not silently start scanning nothing. + given = Path(argv[1]) if len(argv) > 1 else Path(".") + if given.name in ("workflows", "actions") and given.parent.name == ".github": + repo_root = given.parent.parent + else: + repo_root = given + + workflow_root = repo_root / ".github" / "workflows" + action_root = repo_root / ".github" / "actions" + + paths = (sorted(p for p in workflow_root.glob("*.yml")) + + sorted(p for p in workflow_root.glob("*.yaml"))) + action_paths = (sorted(action_root.glob("*/action.yml")) + + sorted(action_root.glob("*/action.yaml"))) + problems: list[str] = [] + apt_action_refs: set[str] = set() + for path in action_paths: + problems.extend(check_action(path)) + document = yaml.safe_load(path.read_text(encoding="utf-8")) + if isinstance(document, dict) and any( + _network_apt_lines(step.get("run")) + for _, step in _composite_steps(document) + if isinstance(step.get("run"), str) + ): + apt_action_refs.add(_action_reference(repo_root, path)) + for path in paths: problems.extend(check_workflow(path)) + problems.extend(check_workflow_callers(path, apt_action_refs)) if problems: print("Unbounded apt-get network calls in workflows:\n", file=sys.stderr) for problem in problems: print(f" - {problem}", file=sys.stderr) print( "\nAdd -o Acquire::Retries=3 -o Acquire::http::Timeout=N -o Acquire::https::Timeout=N " - "to the apt-get line, and timeout-minutes to the step.", + "to the apt-get line, and timeout-minutes to the step. In a composite action the " + "ceiling belongs on each caller instead - timeout-minutes is not a valid key on a " + "composite-action step.", file=sys.stderr, ) return 1 - print(f"apt-get network bounds OK ({len(paths)} workflow files)") + print( + f"apt-get network bounds OK ({len(paths)} workflow files, " + f"{len(action_paths)} composite actions)" + ) return 0 diff --git a/.github/scripts/test_check_workflow_apt_network_bounds.py b/.github/scripts/test_check_workflow_apt_network_bounds.py index db64776557c..9726384f4c5 100644 --- a/.github/scripts/test_check_workflow_apt_network_bounds.py +++ b/.github/scripts/test_check_workflow_apt_network_bounds.py @@ -73,5 +73,120 @@ def test_repository_workflows_are_bounded(self) -> None: self.assertEqual(problems, [], "\n".join(problems)) +class CompositeActionBoundsTests(unittest.TestCase): + """A bounded apt call may live in a composite action; the guard must follow it there.""" + + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.tmp_path = Path(self._tmp.name) + + def _action(self, run: str, *, name: str = "install-redis-server") -> Path: + directory = self.tmp_path / ".github" / "actions" / name + directory.mkdir(parents=True, exist_ok=True) + step = " steps:\n - name: install\n shell: bash\n run: |\n" + for line in run.splitlines(): + step += f" {line}\n" + path = directory / "action.yml" + path.write_text(f"name: {name}\nruns:\n using: composite\n{step}") + return path + + def _caller(self, uses: str, *, timeout: bool = True, name: str = "wf.yml") -> Path: + directory = self.tmp_path / ".github" / "workflows" + directory.mkdir(parents=True, exist_ok=True) + step = f" - name: deps\n uses: {uses}\n" + if timeout: + step += " timeout-minutes: 5\n" + path = directory / name + path.write_text( + f"name: t\non: push\njobs:\n j:\n runs-on: ubuntu-latest\n steps:\n{step}" + ) + return path + + def test_the_old_checker_fails_open_on_an_action(self) -> None: + """Why a separate entry point exists: the workflow shape does not match an action. + + `check_workflow` looks under `jobs..steps`; a composite action keeps its steps + under `runs.steps`. Pointed at an action file it reports clean, so widening only + the glob would have produced a guard that scanned the file and still saw nothing. + """ + path = self._action("sudo apt-get install -y redis-server") + self.assertEqual(guard.check_workflow(path), []) + self.assertTrue(guard.check_action(path)) + + def test_unbounded_apt_in_an_action_is_rejected(self) -> None: + path = self._action("sudo apt-get install -y redis-server") + problems = guard.check_action(path) + self.assertEqual(len(problems), 1, problems) + self.assertIn("Acquire::Retries", problems[0]) + + def test_bounded_apt_in_an_action_passes_without_step_timeout(self) -> None: + """`timeout-minutes` is not a valid key on a composite step, so it is not required. + + Demanding it here would make the guard unsatisfiable for the one shape it now has + to cover. + """ + path = self._action(f"{BOUNDED} update\n{BOUNDED} install --yes redis-server") + self.assertEqual(guard.check_action(path), []) + + def test_non_composite_actions_are_left_alone(self) -> None: + directory = self.tmp_path / ".github" / "actions" / "js-action" + directory.mkdir(parents=True) + path = directory / "action.yml" + path.write_text("name: js\nruns:\n using: node20\n main: index.js\n") + self.assertEqual(guard.check_action(path), []) + + def test_caller_of_an_apt_action_must_declare_a_ceiling(self) -> None: + """The backstop moves to the call site; nothing else can carry it.""" + self._action(f"{BOUNDED} install --yes redis-server") + caller = self._caller("./.github/actions/install-redis-server", timeout=False) + problems = guard.check_workflow_callers( + caller, {"./.github/actions/install-redis-server"} + ) + self.assertEqual(len(problems), 1, problems) + self.assertIn("timeout-minutes", problems[0]) + + def test_caller_with_a_ceiling_passes(self) -> None: + self._action(f"{BOUNDED} install --yes redis-server") + caller = self._caller("./.github/actions/install-redis-server", timeout=True) + self.assertEqual( + guard.check_workflow_callers(caller, {"./.github/actions/install-redis-server"}), [] + ) + + def test_callers_of_other_actions_are_not_burdened(self) -> None: + caller = self._caller("./.github/actions/detect-changes", timeout=False) + self.assertEqual( + guard.check_workflow_callers(caller, {"./.github/actions/install-redis-server"}), [] + ) + + def test_main_walks_both_trees_and_catches_the_laundered_ceiling(self) -> None: + """End to end: bounded options inside the action, no ceiling on the caller.""" + self._action(f"{BOUNDED} update\n{BOUNDED} install --yes redis-server") + self._caller("./.github/actions/install-redis-server", timeout=False) + self.assertEqual(guard.main(["prog", str(self.tmp_path)]), 1) + + def test_main_passes_when_both_halves_are_present(self) -> None: + self._action(f"{BOUNDED} update\n{BOUNDED} install --yes redis-server") + self._caller("./.github/actions/install-redis-server", timeout=True) + self.assertEqual(guard.main(["prog", str(self.tmp_path)]), 0) + + def test_main_still_accepts_the_historical_workflows_argument(self) -> None: + """The old call form pointed at `.github/workflows`; it must not scan nothing.""" + self._caller("./.github/actions/detect-changes", timeout=True) + workflows = self.tmp_path / ".github" / "workflows" + self.assertEqual(guard.main(["prog", str(workflows)]), 0) + + def test_repository_actions_are_bounded(self) -> None: + root = Path(__file__).resolve().parents[1] / "actions" + problems: list[str] = [] + for path in sorted(root.glob("*/action.yml")): + problems.extend(guard.check_action(path)) + self.assertEqual(problems, [], "\n".join(problems)) + + def test_repository_tree_passes_end_to_end(self) -> None: + repo_root = Path(__file__).resolve().parents[2] + self.assertEqual(guard.main(["prog", str(repo_root)]), 0) + + if __name__ == "__main__": unittest.main()