fix(ci): follow bounded apt-get into composite actions, and cap it at the caller - #12270
fix(ci): follow bounded apt-get into composite actions, and cap it at the caller#12270sujeito-operator wants to merge 2 commits into
Conversation
… the caller `check_workflow_apt_network_bounds.py` was written after BasedHardware#11872 and asserts the right property -- an apt-get subcommand that touches the network must carry the `Acquire::*` bounds and sit in a step with a ceiling -- but only over the literal location that incident occupied: `.github/workflows/*.yml`, shaped as `jobs.<id>.steps`. BasedHardware#12194 moves the hermetic-gauntlet redis install into `.github/actions/install-redis-server/action.yml` so three jobs share one copy. After it lands, the repo's only bounded apt calls live where the guard does not look, and its stated promise -- "so a new one cannot be added unbounded" -- quietly stops being true. Widening the glob alone would not have closed it. A composite action keeps its steps under `runs.steps`, so `check_workflow` finds no steps and returns clean: the blind spot fails open rather than erroring. `test_the_old_checker_fails_open_on_an_action` pins that, because it is the part that would have made a scope fix look done. `timeout-minutes` is not a valid key on a composite-action step, so the ceiling cannot be enforced where the apt line now is. `check_action` therefore requires the acquire options only, and `check_workflow_callers` requires the ceiling on every step that `uses:` an action which runs network apt. Without that second half, moving an install into an action would launder the backstop away while the guard still reported success. Also adds `.github/actions/**` to the check's manifest triggers: the guard could otherwise be correct and simply never run on an actions-only change. Verification -- `python3 .github/scripts/test_check_workflow_apt_network_bounds.py`: 18 tests, OK (was 6). End to end over this tree, `main()` reports "apt-get network bounds OK (70 workflow files, 10 composite actions)" and exits 0. The two cases that matter both fail before this change and pass after: an unbounded apt-get inside an `action.yml` is now rejected, and a caller of an apt-running action with no `timeout-minutes` is now rejected. No existing workflow or action changes behavior -- none of the 10 composite actions in the tree runs apt-get today. Failure-Class: FC-guard-encodes-incident-literals
|
Verified this end to end on the head commit, outside the contributor's environment: the widened guard exits 0 over the tree (70 workflow files, 10 composite actions), the self-test passes 18/18, and hand-crafted negative cases behave as designed - an unbounded The split is the right shape, and the reasoning holds up against the code:
Two residual limits surfaced while probing; neither blocks this PR and neither is a regression (both were invisible before, since actions were entirely unguarded):
On the red checks: the three Desktop Swift lanes fail with errors that reproduce identically on Since this encodes guard policy (where the ceiling lives) on a workflow-sensitive surface, leaving the final sign-off to a human maintainer - the approach matches what was asked for in #12194's review, so this should be a quick yes. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
kodjima33
left a comment
There was a problem hiding this comment.
Closes a real gap in the apt-network-bounds guard (composite actions were unscanned) and doesn't touch .github/workflows/ itself. Holding merge: 3 Desktop Swift CI checks are red (labeled pre-existing, but hard floor still applies).
Follow-up to #12194, raised by @Git-on-my-level in review there:
Opened as its own PR rather than a commit on #12194 so that branch stays at the head that already has maintainer approval.
The gap
The guard was written after #11872 and asserts the right property — an
apt-getsubcommand that touches the network must carry theAcquire::*bounds and sit in a step with a ceiling. But it only asserts it over the literal location that incident occupied:.github/workflows/*.yml, shaped asjobs.<id>.steps.#12194 moves the hermetic-gauntlet redis install into
.github/actions/install-redis-server/action.ymlso three jobs share one copy. Once that lands, the repo's only bounded apt calls live where the guard does not look.Widening the glob alone would not have closed it. A composite action keeps its steps under
runs.steps, notjobs.<id>.steps, socheck_workflowfinds no steps and returns[]. Pointed straight at anaction.ymlcontaining a rawsudo apt-get install -y redis-server, the old code reports clean — it fails open rather than erroring.test_the_old_checker_fails_open_on_an_actionpins exactly that, because it is the part that would have made a scope-only fix look finished.Why the ceiling moves to the caller
timeout-minutesis not a valid key on a composite-action step — GitHub rejects the workflow that uses one. That is the trap documented at each call site in #12194, and it means the backstop cannot be enforced where the apt line now is.So the requirement splits:
check_actionrequires theAcquire::*options on every network apt line inruns.steps, and does not requiretimeout-minutes— demanding it would be unsatisfiable, and a guard nobody can satisfy gets deleted rather than obeyed.check_workflow_callersrequirestimeout-minuteson every workflow step whoseuses:resolves to a local action that runs network apt.Without that second half, moving an install into an action would launder the backstop away while the guard still reported success.
.github/actions/**is also added to the check's manifest triggers — the guard could otherwise be correct and simply never run on an actions-only change.Verification
python3 .github/scripts/test_check_workflow_apt_network_bounds.py→ 18 tests, OK (was 6).End to end over this tree:
The two cases that matter both fail before this change and pass after:
apt-getinside anaction.ymlis now rejected (test_unbounded_apt_in_an_action_is_rejected);timeout-minutesis now rejected (test_caller_of_an_apt_action_must_declare_a_ceiling, and end to end intest_main_walks_both_trees_and_catches_the_laundered_ceiling).The historical
main()argument form (a path to.github/workflows) still resolves rather than silently scanning nothing —test_main_still_accepts_the_historical_workflows_argument.One gap in my own verification, stated rather than implied: I could not run the manifest entry point
bash scripts/run-workflow-apt-network-bounds.shto completion. It bootstraps the full backend venv, andwebrtcvad==2.0.10fails to build a wheel in my environment — unrelated to this diff, and it fails the same way on the unmodified tree. I ran the self-test file directly with a PyYAML-equipped interpreter instead, which is what that wrapper execs. Worth a maintainer running the wrapper once on CI's image.No existing workflow or action changes behavior. None of the 10 composite actions in the tree runs
apt-gettoday, so this is coverage ahead of #12194 rather than a live break;test_repository_actions_are_boundedandtest_repository_tree_passes_end_to_endassert the tree is clean under the widened scope.Failure-Class: FC-guard-encodes-incident-literals
Disclosure: this patch was written and tested end to end by an autonomous AI agent; a human principal is accountable for it. What this account is. Ask me anything about how it was produced and I will answer.