From 1d5294c76a1c409d0bb17386d7e2912a363dc392 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 18 Sep 2026 16:50:08 +0100 Subject: [PATCH 1/7] Place fork-reachable lanes and bound every job No Ubicloud lane carried a fork fallback. A pull request from a fork cannot obtain an Ubicloud runner, so six fork-reachable jobs would have queued forever, and between them they cover seven of the thirteen contexts the `main-required-checks` ruleset requires. `build-wheels.yml` declares only `workflow_call`, which understates its exposure: `ci.yml` calls it on every pull request. Add the fallback to `lint-test`, `typecheck-test`, `extension-tests`, `coverage`, `benchmark-ratchet`, `build-pure-wheel` and `verify-wheel-install`. A called workflow inherits its caller's `github` context, so `build-wheels.yml` takes the expression directly: on the tag-push caller `github.event.pull_request` is null and the Ubicloud arm is selected. `coverage-upload` needs none, because `coverage-main.yml` triggers only on a push to `main` and a dispatch. Move `lint-test` onto the paid lane with a 45-minute ceiling and the resource sampler every other paid Linux job carries. Six runs measured it at 198 to 292 s on four hosted vCPU, with a queue that was two to three seconds five times and 154 seconds once; the same run queued every hosted lane 77 to 164 s while its Ubicloud neighbours queued 18 to 37. Its required context is the bare job id, so the move renames nothing. It stops saving the Cargo registry: on its owned arm it now renders the `self-hosted` family `extension-tests` writes, and one writer per family is the invariant. Give every job that declares steps a ceiling, and none to a reusable-workflow caller, where GitHub rejects `timeout-minutes`. The rule is keyed on declaring steps rather than on the label, because once a label is an expression "an Ubicloud lane" is a property of the event. Put `runs-on` behind one reader in `tests/helpers/ci_placement.py`. It models a literal label, the fork fallback read by position, and a matrix key resolved through its `include` entries, and refuses everything else by name: an expression recorded as one opaque label carries no vendor prefix, so it would exempt the lane from every placement, ceiling and registry rule while still asking for a paid runner. The cache-family resolver reads lanes through it too. Rebuild the registry contract in its canonical shape, add contracts for whether a job runs, for `continue-on-error` and for job-name stability, and split the placement module so each file stays under the 400-line limit. The two Linux native-wheel legs are unchanged. GitHub spells matrix values into the check name, so moving either renames a required context, and nine steps there are guarded on `startsWith(matrix.os, 'ubuntu')`. That is the repository owner's decision. Both legs did gain a ceiling. --- .github/workflows/build-wheels.yml | 31 +- .github/workflows/ci.yml | 119 ++++-- .github/workflows/delayed-pr-comment.yml | 4 + docs/ci-cache-ownership.md | 12 +- docs/developers-guide.md | 172 ++++++-- tests/helpers/ci_cache_families.py | 19 +- tests/helpers/ci_placement.py | 324 ++++++++++++++ tests/helpers/ci_runners.py | 74 +++- tests/test_ci_actionlint_registry.py | 123 ++++++ tests/test_ci_cache_families_helper.py | 35 +- tests/test_ci_placement_reader.py | 336 +++++++++++++++ tests/test_ci_resource_sampling.py | 4 + tests/test_ci_runner_placement.py | 513 ++++++++++------------- tests/test_ci_tool_installation.py | 146 +++++++ tests/test_ci_worker_bounds.py | 102 +++++ 15 files changed, 1654 insertions(+), 360 deletions(-) create mode 100644 tests/helpers/ci_placement.py create mode 100644 tests/test_ci_actionlint_registry.py create mode 100644 tests/test_ci_placement_reader.py create mode 100644 tests/test_ci_tool_installation.py create mode 100644 tests/test_ci_worker_bounds.py diff --git a/.github/workflows/build-wheels.yml b/.github/workflows/build-wheels.yml index 2731f5738..35bc078f7 100644 --- a/.github/workflows/build-wheels.yml +++ b/.github/workflows/build-wheels.yml @@ -25,7 +25,16 @@ jobs: # `~/.cache/uv` a second writer alongside ci.yml's tool cache for a job that # would save less time than the archive costs to move. build-pure-wheel: - runs-on: ubicloud-standard-2 + # Reached from ci.yml on every pull request, forks included, and from + # release.yml on a tag. A called workflow inherits its caller's `github` + # context, so this expression reads the caller's event: on the tag caller + # `github.event.pull_request` is null and the Ubicloud arm is selected, + # and on a fork's pull request the hosted arm is, because a fork cannot + # obtain an Ubicloud runner. Keep the continuation at this indent; a + # more-indented one would put a newline inside the expression. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 20 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -37,7 +46,16 @@ jobs: artifact-name: wheels-pure build-native-wheels: + # Unchanged placement. GitHub spells matrix values into the check name, so + # both `ubuntu-latest` legs are named verbatim in the `main-required-checks` + # ruleset; moving either renames a required context, and nine steps here + # are guarded by `startsWith(matrix.os, 'ubuntu')`, which an expression + # would switch off. That is a decision for the repository owner, not a + # placement change. runs-on: ${{ matrix.os }} + # Measured at 29 to 107 s across the five legs; the Linux ones build in a + # container and the aarch64 leg emulates under QEMU. + timeout-minutes: 45 strategy: fail-fast: false matrix: @@ -156,7 +174,16 @@ jobs: # Also uncached: it installs published wheels into a throwaway virtual # environment and imports them. There is nothing expensive to keep. verify-wheel-install: - runs-on: ubicloud-standard-2 + # Reached from ci.yml on every pull request, forks included, and from + # release.yml on a tag. A called workflow inherits its caller's `github` + # context, so this expression reads the caller's event: on the tag caller + # `github.event.pull_request` is null and the Ubicloud arm is selected, + # and on a fork's pull request the hosted arm is, because a fork cannot + # obtain an Ubicloud runner. Keep the continuation at this indent; a + # more-indented one would put a newline inside the expression. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 20 needs: - build-pure-wheel diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 557083e16..d2fe6ed42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,7 +55,23 @@ env: jobs: lint-test: - runs-on: ubuntu-latest + # A pull request from a fork cannot obtain an Ubicloud runner, so the fork + # arm falls back to a GitHub-hosted label and only forks take it. Keep the + # continuation at this indent: a more-indented continuation in a folded + # scalar keeps its line break and puts a newline inside the expression. + # GitHub evaluates the broken value regardless, so a green run is not + # evidence; `test_placement_expressions_parse_to_one_line` is. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} + # Compile-bound on two toolchains plus a Windows cross-target check, + # measured at 198 to 292 s over six runs on four GitHub-hosted vCPU. + # Ubicloud vCPU are not GitHub vCPU, so the Ubicloud figure is expected to + # be higher and the arm is judged on the third run. The ceiling matches the + # interpreter-suite lanes: a regression fails visibly instead of quietly + # costing, and `ubicloud-standard-4` needs measured evidence this + # repository does not have yet. + timeout-minutes: 45 env: MBAKE_VERSION: '1.4.6' MDTABLEFIX_VERSION: '0.6.0' @@ -72,6 +88,12 @@ jobs: # token must not stay in .git/config where that code could reach it. persist-credentials: false + - name: Start resource sampler + uses: ./.github/actions/resource-sampler + with: + mode: start + vcpus: ${{ env.LINUX_RUNNER_VCPUS }} + - name: Compute cache keys uses: ./.github/actions/cache-keys with: @@ -80,10 +102,11 @@ jobs: ubuntu-release: ${{ env.UBUNTU_RELEASE }} compiler-shape: lint - # This job is GitHub-hosted, so these archives live in GitHub's cache - # service rather than Ubicloud's. Every key carries `runner.environment`, - # so the two lanes cannot restore each other's entries even though they - # share a family name, and each lane has its own single writer. + # Every key carries `runner.environment`, so the owned arm and the fork + # arm cannot restore each other's entries even though they share a family + # name. Nothing writes the fork arm's scope: every save below requires a + # push to `refs/heads/main`, which a fork can never produce, so a fork's + # pull request restores nothing and compiles cold. - name: Restore the Cargo registry id: cargo-cache uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 @@ -328,26 +351,19 @@ jobs: printf '%s\n' "${fence}" } >> "${GITHUB_STEP_SUMMARY}" - # Single writer for the GitHub-hosted lane, mirroring `typecheck-test` - # on the Ubicloud lane. It is the only Linux gate here and the only one - # that compiles, so there is nothing to race with. - - name: Save the Cargo registry - if: >- - github.event_name == 'push' && github.ref == 'refs/heads/main' && - steps.cargo-cache.outputs.cache-hit != 'true' - uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - key: ${{ env.CARGO_CACHE_KEY }} - + # This job restores the Cargo registry and no longer saves it. On the + # owned arm it now runs on the Ubicloud lane, where `runner.environment` + # renders `self-hosted` and the key it would publish is the very family + # `extension-tests` writes. Two writers of one family is the collision + # `test_each_cache_family_has_exactly_one_writer` exists to refuse, and + # the registry holds the resolved dependency graph, which is the same + # content either job would archive. - name: Save the compiler cache - # The only GitHub-hosted job that compiles, so it owns the whole - # `github-hosted` compiler-cache scope; its Cranelift lint build is - # also a shape no other job produces. Its archive lands in GitHub's - # cache rather than Ubicloud's, which is the other reason the lane is - # named in the key. + # The `lint` build shape is produced by no other job, so this remains + # the sole writer of its compiler-cache family. Only the lane in the + # rendered key changed: the fork arm cannot reach this step, because + # the guard below requires a push to `refs/heads/main` and a fork's + # pull request is neither. # # The entry is a rolling generation named after the run, so there is # no cache-hit guard to skip: the key is new every time, and a run @@ -362,10 +378,25 @@ jobs: path: ~/.cache/sccache key: ${{ env.SCCACHE_CACHE_KEY }} + - name: Report peak resource use + if: always() + uses: ./.github/actions/resource-sampler + with: + mode: report + vcpus: ${{ env.LINUX_RUNNER_VCPUS }} + typecheck-test: name: Typecheck and test (Python ${{ matrix.python-label }}) continue-on-error: ${{ matrix.experimental }} - runs-on: ubicloud-standard-2 + # A pull request from a fork cannot obtain an Ubicloud runner, so the fork + # arm falls back to a GitHub-hosted label and only forks take it. Keep the + # continuation at this indent: a more-indented continuation in a folded + # scalar keeps its line break and puts a newline inside the expression. + # GitHub evaluates the broken value regardless, so a green run is not + # evidence; `test_placement_expressions_parse_to_one_line` is. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 45 env: MDTABLEFIX_VERSION: '0.6.0' @@ -617,7 +648,15 @@ jobs: extension-tests: name: Extension-gated tests (Python/Rust boundary) - runs-on: ubicloud-standard-2 + # A pull request from a fork cannot obtain an Ubicloud runner, so the fork + # arm falls back to a GitHub-hosted label and only forks take it. Keep the + # continuation at this indent: a more-indented continuation in a folded + # scalar keeps its line break and puts a newline inside the expression. + # GitHub evaluates the broken value regardless, so a green run is not + # evidence; `test_placement_expressions_parse_to_one_line` is. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 45 steps: - name: Check out repository @@ -822,6 +861,11 @@ jobs: extension-tests-windows: name: Extension-gated tests (Windows Python/Rust boundary) runs-on: windows-2022 + # Measured at 67 to 72 s. A ceiling belongs on every job that declares + # steps, not only on the paid ones: once a label is event-dependent, + # "an Ubicloud lane" is a property of the event, so a rule keyed on the + # label would stop applying on exactly the arm that hangs. + timeout-minutes: 20 steps: - name: Check out repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -873,7 +917,15 @@ jobs: # ratchet baseline, so this lane only compares its coverage locally # against the trusted baseline without generating coverage twice on main. if: github.event_name == 'pull_request' - runs-on: ubicloud-standard-2 + # A pull request from a fork cannot obtain an Ubicloud runner, so the fork + # arm falls back to a GitHub-hosted label and only forks take it. Keep the + # continuation at this indent: a more-indented continuation in a folded + # scalar keeps its line break and puts a newline inside the expression. + # GitHub evaluates the broken value regardless, so a green run is not + # evidence; `test_placement_expressions_parse_to_one_line` is. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} # Above the 2,700 s cargo watchdog set below plus the work either side # of the coverage step, which reached 43 s on run 34071469378. Fifty # minutes is the requirement, and the estate's rule is fifteen above @@ -1128,7 +1180,10 @@ jobs: # `needs` edge never leaves benchmark-ratchet skipped on pushes to # main, where the baseline artefact must stay fresh. changes: + # Stays GitHub-hosted: a five-second paths-filter call is administrative, + # API-bound work that gains nothing from a metered build slot. runs-on: ubuntu-latest + timeout-minutes: 10 permissions: contents: read # paths-filter reads pull-request changed files through GitHub's API. @@ -1426,7 +1481,15 @@ jobs: # ratio between the baseline and the candidate, so runner speed cancels out # of the comparison and the job does not need a fixed or larger shape. It # therefore starts at the estate default like every other Linux gate. - runs-on: ubicloud-standard-2 + # A pull request from a fork cannot obtain an Ubicloud runner, so the fork + # arm falls back to a GitHub-hosted label and only forks take it. Keep the + # continuation at this indent: a more-indented continuation in a folded + # scalar keeps its line break and puts a newline inside the expression. + # GitHub evaluates the broken value regardless, so a green run is not + # evidence; `test_placement_expressions_parse_to_one_line` is. + runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} timeout-minutes: 60 needs: - lint-test diff --git a/.github/workflows/delayed-pr-comment.yml b/.github/workflows/delayed-pr-comment.yml index c73c0642a..833bcab37 100644 --- a/.github/workflows/delayed-pr-comment.yml +++ b/.github/workflows/delayed-pr-comment.yml @@ -18,6 +18,10 @@ jobs: # Deliberately stays on GitHub-hosted Ubuntu: this sleeps while holding a # runner and must not consume a metered Ubicloud build slot. runs-on: ubuntu-latest + # A ceiling far above any plausible delay, so a runaway `delay_minutes` + # cannot hold a runner for GitHub's six-hour default. A delay longer than + # this belongs in a scheduled workflow, not in a sleeping job. + timeout-minutes: 180 permissions: pull-requests: write # needed to post comments steps: diff --git a/docs/ci-cache-ownership.md b/docs/ci-cache-ownership.md index 7881219d7..b429ec013 100644 --- a/docs/ci-cache-ownership.md +++ b/docs/ci-cache-ownership.md @@ -102,7 +102,6 @@ the family without saving it. | Family | Writer | | ------------------------------------- | ------------------------------------- | | Cargo registry, Ubicloud | `ci.yml` `extension-tests` | -| Cargo registry, GitHub-hosted | `ci.yml` `lint-test` | | Tools, per interpreter | `ci.yml` `typecheck-test`, that leg | | Compiler, 3.13 unoptimized | `ci.yml` `extension-tests` | | Compiler, 3.12/3.14/3.15a unoptimized | `ci.yml` `typecheck-test`, that leg | @@ -111,6 +110,17 @@ the family without saving it. | Compiler, Cranelift lint | `ci.yml` `lint-test` | | Compiler, Loom model | `loom.yml` `loom` | +`lint-test` moved to the Ubicloud lane with the fork fallback, taking its +Cranelift lint family with it, and stopped saving the Cargo registry: on its +owned arm it renders the same family `extension-tests` writes, and one writer +per family is the invariant. The Loom family stays on the GitHub-hosted lane, +because its writer is a scheduled job that stays there too. + +The fork arm renders `runner.environment` as `github-hosted`, so a fork's pull +request reads a scope whose only writer is that scheduled Loom job and runs +cold for everything else. Every save is guarded on a push to `refs/heads/main`, +which a fork can never produce, so the fork arm restores and never publishes. + _Table 2: The single job that publishes each family under the writer trigger described above._ diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 869fc76fe..8c5a24423 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -43,24 +43,34 @@ gains nothing from a metered build slot. Table 1: GitHub Actions jobs, workflows, and runners -| Job | Workflow | Runner | -| ------------------------- | ---------------------------- | --------------------- | -| `typecheck-test` | `ci.yml` | `ubicloud-standard-2` | -| `extension-tests` | `ci.yml` | `ubicloud-standard-2` | -| `coverage` | `ci.yml` | `ubicloud-standard-2` | -| `benchmark-ratchet` | `ci.yml` | `ubicloud-standard-2` | -| `build-pure-wheel` | `build-wheels.yml` | `ubicloud-standard-2` | -| `verify-wheel-install` | `build-wheels.yml` | `ubicloud-standard-2` | -| `coverage-upload` | `coverage-main.yml` | `ubicloud-standard-2` | -| `lint-test` | `ci.yml` | `ubuntu-latest` | -| `changes` | `ci.yml` | `ubuntu-latest` | -| `workflow-harness` | `benchmark-gate-harness.yml` | `ubuntu-latest` | -| `loom-smoke` | `ci.yml` | `ubuntu-latest` | -| `loom` | `loom.yml` | `ubuntu-latest` | -| `extension-tests-windows` | `ci.yml` | `windows-2022` | -| `publish` | `release.yml` | `ubuntu-latest` | -| `delay_and_comment` | `delayed-pr-comment.yml` | `ubuntu-latest` | -| `build-native-wheels` | `build-wheels.yml` | `${{ matrix.os }}` | +| Job | Workflow | Owned arm | Fork arm | Ceiling | +| ------------------------- | ---------------------------- | --------------------- | --------------- | ------- | +| `lint-test` | `ci.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 45 | +| `typecheck-test` | `ci.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 45 | +| `extension-tests` | `ci.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 45 | +| `coverage` | `ci.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 65 | +| `benchmark-ratchet` | `ci.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 60 | +| `build-pure-wheel` | `build-wheels.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 20 | +| `verify-wheel-install` | `build-wheels.yml` | `ubicloud-standard-2` | `ubuntu-latest` | 20 | +| `coverage-upload` | `coverage-main.yml` | `ubicloud-standard-2` | none | 65 | +| `changes` | `ci.yml` | `ubuntu-latest` | none | 10 | +| `loom-smoke` | `ci.yml` | `ubuntu-latest` | none | 10 | +| `workflow-harness` | `benchmark-gate-harness.yml` | `ubuntu-latest` | none | 30 | +| `loom` | `loom.yml` | `ubuntu-latest` | none | 30 | +| `verus` | `rust-boundaries.yml` | `ubuntu-latest` | none | 20 | +| `extended` | `rust-boundaries.yml` | `ubuntu-latest` | none | 45 | +| `extension-tests-windows` | `ci.yml` | `windows-2022` | none | 20 | +| `publish` | `release.yml` | `ubuntu-latest` | none | 20 | +| `delay_and_comment` | `delayed-pr-comment.yml` | `ubuntu-latest` | none | 180 | +| `build-native-wheels` | `build-wheels.yml` | `${{ matrix.os }}` | none | 45 | +| `native` | `rust-boundaries.yml` | `${{ matrix.os }}` | none | 20 | + +The hosted rows are hosted for a reason, not by omission. `changes` and +`loom-smoke` are cheap gate jobs; `workflow-harness`, `loom` and `extended` run +only on a schedule or a dispatch; and `rust-boundaries.yml`'s verifier lanes +stay on GitHub-hosted Linux by this repository's own decision. Placing `verus` +or `loom-smoke` on the paid lane is a separate question needing its own +measurements. `ubicloud-standard-2` (2 vCPU, 8 GB, Ubuntu 24.04 amd64) is the default shape and the only self-hosted label registered in `.github/actionlint.yaml`. @@ -76,17 +86,110 @@ scenario's within-run `rust_mean / python_mean` ratio between the baseline and the candidate, so runner speed cancels out of the comparison and the job needs neither a fixed nor a larger shape. -`lint-test` stays on `ubuntu-latest`. It installs Whitaker and keeps its own -npm and Whitaker archives on GitHub's cache service, which is a different store -from Ubicloud's. It owns its Cargo registry and compiler caches on that side -exactly as the Ubicloud jobs own theirs. +`lint-test` moved onto the paid lane. It is a developer-blocking gate that +compiles real work: Whitaker, clippy under two toolchains, a Windows +cross-target check and Nixie. Six runs measured it at 198 to 292 seconds on +four GitHub-hosted vCPU, with a queue that was two to three seconds five times +and 154 seconds once; the same run queued every hosted lane 77 to 164 seconds +while its Ubicloud neighbours queued 18 to 37. The case is that tail. Ubicloud +vCPU are not GitHub vCPU, so the Ubicloud figure is expected to be higher than +the hosted range, and the arm is judged on its third warm run rather than its +first. Its required check context is the bare job id, which no runner label +spells, so the move renames nothing. The native-wheel matrix keeps its platform runners. Ubicloud has no Windows or macOS capacity, and its Linux legs build inside manylinux containers and under -QEMU emulation. +QEMU emulation. The two Linux legs are also the one place a placement change +would cost more than it looks: GitHub spells matrix values into the check name, +so `build-wheels / build-native-wheels (ubuntu-latest, …)` appears verbatim +twice in the `main-required-checks` ruleset, and nine steps in that job are +guarded on `startsWith(matrix.os, 'ubuntu')`, which an expression would switch +off. Moving either leg is a ruleset edit and a guard rewrite, not a label +change. -Every Ubicloud job declares `timeout-minutes` so a wedged runner cannot bill -for GitHub's six-hour default. +### The fork arm + +A pull request from a fork cannot obtain an Ubicloud runner, so every +fork-reachable lane selects its runner by event: + +```yaml +runs-on: >- + ${{ github.event.pull_request.head.repo.fork + && 'ubuntu-latest' || 'ubicloud-standard-2' }} +``` + +Keep the continuation at the same indent. A continuation indented one level +deeper keeps its line break inside the folded scalar and puts a newline in the +middle of the expression; GitHub evaluates the broken value anyway, so a green +run proves nothing and `test_placement_expressions_parse_to_one_line` is what +catches it. + +Reachability is a property of the triggers, not of the file. `build-wheels.yml` +declares only `workflow_call`, and its two Ubicloud jobs are as exposed as the +rest because `ci.yml` calls it on every pull request. A called workflow +inherits its caller's `github` context, so the expression above works unchanged +there: on the tag-push caller `github.event.pull_request` is null and the +Ubicloud arm is selected. `coverage-upload` carries no fallback because +`coverage-main.yml` triggers only on a push to `main` and a dispatch, neither +of which a fork can cause. + +The fork arm runs cold, and that is the accepted price. Every cache key carries +`runner.environment`, which renders `github-hosted` on the fork arm and +`self-hosted` on the owned one, so the two arms read different scopes. No +writer changes: every save step is guarded on a push to `refs/heads/main`, +which is never a fork, so the fork arm restores and never publishes. Since +`lint-test` moved, no job writes a `github-hosted` family at all. + +`lint-test` therefore no longer saves the Cargo registry. On its owned arm it +renders the same `self-hosted` family `extension-tests` writes, and one writer +per family is the invariant `test_each_cache_family_has_exactly_one_writer` +holds. It still restores that archive; the registry holds the resolved +dependency graph, which is the same content either job would have published. + +### Ceilings + +Every job that declares `steps` declares `timeout-minutes`, so a wedged runner +cannot bill for GitHub's six-hour default. The rule is keyed on declaring steps +rather than on carrying an Ubicloud label, because once a label is an +expression "an Ubicloud lane" is a property of the event, and a rule keyed on +the label would stop applying on exactly the arm that hangs. + +Jobs that declare `uses:` are exempt and must declare no ceiling: GitHub rejects +`timeout-minutes` on a reusable-workflow caller, so placing a job and bounding +it are separate ideas and the bound lives in the callee. Cuprum's four callers +all call workflows whose own jobs are bounded by the rule above. + +### The actionlint registry + +`.github/actionlint.yaml` registers exactly the labels in use that are neither +GitHub-hosted nor already registered, asserted as an equality in both +directions. "In use" is derived from every job in the estate, reading both arms +of a conditional and every matrix runner entry, minus the named frozen set of +GitHub-hosted labels in `tests/helpers/ci_placement.py`. + +The frozen set is named rather than derived from a vendor prefix on purpose. A +`ubicloud-` prefix filter agrees with the named set over today's workflows and +differs on the case that matters: it would silently exempt a second paid +provider's labels from the registry question altogether. A subset assertion +would miss a stale registration, which is the other half of the equality. + +### The placement reader + +`tests/helpers/ci_placement.py` is the one reader of `runs-on` in the test +suite. Its scope is deliberately narrow: it models the three shapes this +repository declares, a literal label, the fork fallback and a matrix key +resolved through its `include` entries, and refuses everything else by name. +Every contract that asks where a job runs, whether it runs, or what label it +resolves to goes through it, including the cache-family resolver in +`tests/helpers/ci_cache_families.py`, so there is one definition of a lane +rather than two that drift. + +Refusal is the design, not a gap. An expression recorded as one opaque label +carries no vendor prefix, so an Ubicloud classifier drops the lane and it sits +exempt from every placement, ceiling and registry rule while still asking for a +paid runner. `tests/test_ci_placement_reader.py` drives the reader with +synthetic declarations, because a rule parametrized over this repository's own +correct workflows passes whether or not it discriminates. ### Cache ownership @@ -103,11 +206,11 @@ key and its restore-key prefix as environment values. A restore and its save cannot disagree, and the rendered key is printed into the run summary so any miss can be explained from the run alone. -| Key family | Paths | Key inputs | Writer | -| ---------- | -------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------ | -| `cargo-` | `~/.cargo/registry`, `~/.cargo/git` | generation, OS, arch, runner environment, hash of `rust/Cargo.lock` and `rust/rust-toolchain.toml` | `extension-tests`, and `lint-test` on the GitHub-hosted lane | -| `tool-` | `~/.cargo/bin`, `~/.local/bin`, `~/.cache/uv`, `~/.local/share/uv`, `.uv-cache`, `.uv-tools` | the above plus Ubuntu release, Python version, nextest pin, hash of `uv.lock`, `pyproject.toml`, `Makefile`, and the sccache action | `typecheck-test` | -| `sccache-` | `~/.cache/sccache` | generation, OS, arch, runner environment, Ubuntu release, run identifier | `typecheck-test`, and `lint-test` on the GitHub-hosted lane | +| Key family | Paths | Key inputs | Writer | +| ---------- | -------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------- | +| `cargo-` | `~/.cargo/registry`, `~/.cargo/git` | generation, OS, arch, runner environment, hash of `rust/Cargo.lock` and `rust/rust-toolchain.toml` | `extension-tests` | +| `tool-` | `~/.cargo/bin`, `~/.local/bin`, `~/.cache/uv`, `~/.local/share/uv`, `.uv-cache`, `.uv-tools` | the above plus Ubuntu release, Python version, nextest pin, hash of `uv.lock`, `pyproject.toml`, `Makefile`, and the sccache action | `typecheck-test` | +| `sccache-` | `~/.cache/sccache` | generation, OS, arch, runner environment, Ubuntu release, run identifier | `typecheck-test`, `lint-test`, and the other compiling jobs | Four rules hold that table together, each with a contract test: @@ -221,9 +324,12 @@ measurement that forced the split. In outline, `extension-tests` writes the 3.13 unoptimized family, each `typecheck-test` leg that runs a suite writes its own interpreter's, `benchmark-ratchet` writes the 3.13 release family, `coverage-upload` writes the instrumented one, and `lint-test` writes the -GitHub-hosted lint family. The scheduled `loom.yml` job writes its separate -model family only on `refs/heads/main`, while manual dispatches and -`loom-smoke` restore it without saving. +Cranelift lint family, which moves to the Ubicloud lane with it. The scheduled +`loom.yml` job writes its separate model family only on `refs/heads/main`, and +stays on the GitHub-hosted lane because its writer does, while manual +dispatches and `loom-smoke` restore it without saving. Apart from that Loom +family, a fork's pull request reads a `github-hosted` scope that nothing +writes, so it compiles cold. The writer has to be a job that actually compiles, or the rolling generation freezes: it would restore the previous entry and republish it unchanged diff --git a/tests/helpers/ci_cache_families.py b/tests/helpers/ci_cache_families.py index cda8079ca..00b43ca9c 100644 --- a/tests/helpers/ci_cache_families.py +++ b/tests/helpers/ci_cache_families.py @@ -19,6 +19,7 @@ import re import typing as typ +from tests.helpers.ci_placement import placement from tests.helpers.ci_workflows import job, save_steps, step_inputs, steps if typ.TYPE_CHECKING: @@ -43,6 +44,13 @@ #: ``runner.environment`` renders from the runner label, and the two lanes read #: different cache services, so an archive can never cross between them. +#: +#: A fork-fallback lane resolves to two labels and therefore to two lanes. The +#: lane an archive is *published* to is the owned one, because every save step +#: here is guarded on a push to ``refs/heads/main`` and a fork's pull request is +#: neither a push nor on that ref. That premise is not assumed: it is asserted +#: by ``test_saves_happen_only_on_trunk_and_only_after_a_miss``, and removing +#: the guard from a save fails it. LANE_OF_LABEL: typ.Final[cabc.Mapping[str, str]] = { "ubicloud-standard-2": "self-hosted", "ubicloud-standard-4": "self-hosted", @@ -84,8 +92,15 @@ def __str__(self) -> str: def _lane(workflow_name: str, job_name: str) -> str: - """Return the cache lane a job's runner label resolves to.""" - label = job(workflow_name, job_name).get("runs-on") + """Return the cache lane a job publishes its archives to.""" + label = placement(workflow_name, job_name).owned + _require( + condition=label is not None, + message=( + f"{workflow_name}:{job_name} has no single owned runner, so the " + "lane its archives land in is undefined" + ), + ) _require( condition=label in LANE_OF_LABEL, message=f"{workflow_name}:{job_name} runs on unmapped label {label!r}", diff --git a/tests/helpers/ci_placement.py b/tests/helpers/ci_placement.py new file mode 100644 index 000000000..ea8550520 --- /dev/null +++ b/tests/helpers/ci_placement.py @@ -0,0 +1,324 @@ +"""Read where a Cuprum CI job can run, and whether it runs at all. + +A ``runs-on`` declaration stopped being a label the moment the fork fallback +landed. A pull request from a fork cannot obtain an Ubicloud runner, so every +fork-reachable lane selects its runner from an expression, and a reader that +narrows that value with ``str()`` sees one opaque label that carries no vendor +prefix. Axinite #372 recorded what that costs: the Ubicloud classifier said no, +the job was dropped before any assertion ran, and the lane sat exempt from the +placement, ceiling and registry contracts while still asking for a paid runner. + +This module therefore fails towards refusal. It models the three shapes the +repository actually declares and rejects everything else by name, so an +unreadable declaration reddens a contract instead of quietly satisfying one: + +``literal`` + A bare label, such as ``windows-2022``. +``fork`` + The canonical fork fallback, read **by position**. Nile-valley #106's + contract asserted only that the expression named one hosted and one + Ubicloud label somewhere, so swapping the arms passed while sending forks + to a runner they cannot obtain. +``matrix`` + ``${{ matrix. }}``, resolved through the ``include`` entries it + selects. Rstest-bdd #788 showed why the resolution matters: a reader that + stops at ``matrix.os`` never sees the expression the matrix value holds. + +Whether a job *runs* is modelled here too. Lille #349's placement, budget and +cache rules all read a job's declared configuration and none read whether it +executes, so ``if: false`` on a build lane satisfied every one of them while +running nothing. +""" + +from __future__ import annotations + +import re +import typing as typ + +from tests.helpers.ci_workflows import job, jobs, workflow_sources + +if typ.TYPE_CHECKING: + import collections.abc as cabc + +#: The field the fork fallback branches on. A sibling such as +#: ``head.repo.private`` is the correct-variant mutation: an expression that +#: reads it is well-formed and wrong, and a contract that merely pattern-matches +#: a conditional accepts it. +FORK_FIELD = "github.event.pull_request.head.repo.fork" + +#: GitHub-hosted labels this repository is permitted to use. Named rather than +#: derived from a vendor prefix: a ``ubicloud-`` prefix filter would silently +#: exempt a second paid provider's labels from the registry question, which is +#: the substantive defect. The two agree over today's workflows and differ on +#: exactly the case that matters. +FROZEN_HOSTED_LABELS: typ.Final = ( + "macos-15-intel", + "macos-latest", + "ubuntu-20.04", + "ubuntu-22.04", + "ubuntu-latest", + "windows-2022", + "windows-latest", +) + +_EXPRESSION = re.compile(r"^\$\{\{(?P.*)\}\}$", re.DOTALL) +#: The canonical fork fallback, anchored end to end so the arms are read by +#: position rather than by membership. `\A`/`\Z` rather than a bare `fullmatch` +#: on a pattern that could otherwise be satisfied by a prefix. +_FORK_EXPRESSION = re.compile( + r"\A\s*(?P[A-Za-z_][\w.]*)" + r"\s*&&\s*'(?P[^']+)'" + r"\s*\|\|\s*'(?P[^']+)'\s*\Z" +) +_MATRIX_EXPRESSION = re.compile(r"\A\s*matrix\.(?P[\w-]+)\s*\Z") +#: Every ``${{ ... }}`` reference in a string, for the job-name stability rule. +_REFERENCE = re.compile(r"\$\{\{(?P[^}]*)\}\}") +#: Expressions a job-level ``if:`` may not reduce to. A job that cannot run +#: satisfies every declaration-reading rule while gating nothing. +_NEVER_RUNS: typ.Final = frozenset({"false", "0", "'false'", '"false"'}) + + +def _require(*, condition: bool, message: str) -> None: + """Raise a contract failure when ``condition`` does not hold.""" + if not condition: + raise AssertionError(message) + + +class Placement(typ.NamedTuple): + """Where one job can run. + + Attributes + ---------- + kind: + ``literal``, ``fork`` or ``matrix``; the shape the declaration took. + owned: + The label selected when the fork arm is not taken, or ``None`` for a + matrix placement, which has no single owned label. Cache lanes resolve + through this, which is sound only because every save step in this + repository is guarded on a push to ``refs/heads/main``, an event a fork + can never produce. ``test_saves_happen_only_on_trunk_and_only_after_a_miss`` + holds that premise. + fork: + The label a fork's pull request takes, or ``None`` when the job + declares no fallback. + labels: + Every label the declaration can resolve to. + references: + Every expression reference the declaration reads, resolved through the + matrix values it selects. + """ + + kind: str + owned: str | None + fork: str | None + labels: frozenset[str] + references: frozenset[str] + + +def declares_steps(workflow_name: str, job_name: str) -> bool: + """Report whether a job runs its own steps rather than calling a workflow. + + A job that declares ``uses`` is a reusable-workflow caller: it declares no + runner of its own and GitHub rejects ``timeout-minutes`` on it, so placing + and bounding it are the callee's business. Keying the exemption on ``uses`` + rather than on a missing ``runs-on`` means a job with neither is still + refused (whitaker #438). + + Returns + ------- + bool + Whether the job declares its own steps and no ``uses`` key. + """ + declared = job(workflow_name, job_name) + return "uses" not in declared and isinstance(declared.get("steps"), list) + + +def all_jobs() -> list[tuple[str, str]]: + """Return every job in the estate as a ``(workflow, job)`` pair. + + Partitioning matters more than it looks: filtering to the jobs a manifest + recognizes sees nothing else, which is how an unclassified lane acquires a + runner unreviewed. + + Returns + ------- + list[tuple[str, str]] + Every job in the estate, in workflow and declaration order. + """ + return [ + (workflow_name, job_name) + for workflow_name, _ in workflow_sources() + for job_name in jobs(workflow_name) + ] + + +def _matrix_values(workflow_name: str, job_name: str, key: str) -> list[object]: + """Return the values one matrix key takes across the job's ``include``.""" + strategy = job(workflow_name, job_name).get("strategy") + _require( + condition=isinstance(strategy, dict), + message=( + f"{workflow_name}:{job_name} selects its runner from matrix.{key} " + "but declares no strategy" + ), + ) + matrix = typ.cast("dict[str, object]", strategy).get("matrix") + _require( + condition=isinstance(matrix, dict), + message=f"{workflow_name}:{job_name} strategy declares no matrix", + ) + declared = typ.cast("dict[str, object]", matrix) + # Two shapes reach here. A plain matrix lists the key's values directly + # (`os: [ubuntu-latest, windows-2022]`); an `include` matrix lists legs and + # the key sits inside each. Modelling only `include` refused + # `rust-boundaries.yml:native` outright, which fails safe but rejects a + # perfectly ordinary workflow. + direct = declared.get(key) + if isinstance(direct, list): + return list(typ.cast("list[object]", direct)) + include = declared.get("include") + _require( + condition=isinstance(include, list), + message=( + f"{workflow_name}:{job_name} declares a matrix this reader cannot " + f"expand; the key {key!r} is neither a list nor present in an " + "`include` list" + ), + ) + legs = typ.cast("list[object]", include) + values: list[object] = [] + for index, leg in enumerate(legs): + _require( + condition=isinstance(leg, dict) and key in leg, + message=( + f"{workflow_name}:{job_name} matrix leg {index} declares no " + f"{key!r}, so its runner is unreadable" + ), + ) + values.append(typ.cast("dict[str, object]", leg)[key]) + return values + + +def _literal_labels( + workflow_name: str, job_name: str, values: cabc.Iterable[object] +) -> frozenset[str]: + """Narrow matrix runner values to literal labels, refusing expressions.""" + labels: set[str] = set() + for value in values: + _require( + condition=isinstance(value, str) and _EXPRESSION.match(value) is None, + message=( + f"{workflow_name}:{job_name} resolves its runner to {value!r}; " + "a matrix value holding an expression is a placement this " + "reader cannot model, and recording it as a label would exempt " + "the lane from every placement rule" + ), + ) + labels.add(str(value)) + return frozenset(labels) + + +def placement(workflow_name: str, job_name: str) -> Placement: + """Model one job's runner selection, refusing any shape it cannot read. + + Returns + ------- + Placement + The shape read, its arms, every label it can resolve to, and every + expression reference it reads. + + Raises + ------ + AssertionError + When the declaration is missing on a step-declaring job, is a list, is + an expression in neither the fork nor the matrix shape, or parses with + an embedded line break. + """ + where = f"{workflow_name}:{job_name}" + declared = job(workflow_name, job_name).get("runs-on") + _require( + condition=not isinstance(declared, list), + message=( + f"{where} declares a list runs-on. Each entry would be recorded as " + "a label, so a runner group or an entry holding an expression would " + "read as a job placed on two runners, one named after the " + "expression (dev-env-rocky #216). Refused rather than modelled." + ), + ) + _require( + condition=isinstance(declared, str) and bool(declared), + message=f"{where} must declare a non-empty string runs-on, got {declared!r}", + ) + text = str(declared) + # The folded-scalar hazard. A continuation indented one level deeper keeps + # its line break, and GitHub evaluates the broken value regardless, so a + # green run is not evidence that the expression is one line. + _require( + condition="\n" not in text, + message=( + f"{where} runs-on parses with an embedded line break: {text!r}. " + "Keep a folded-scalar continuation at the same indent." + ), + ) + match = _EXPRESSION.match(text) + if match is None: + return Placement("literal", text, None, frozenset({text}), frozenset()) + body = match.group("body") + fork = _FORK_EXPRESSION.match(body) + if fork is not None: + return Placement( + "fork", + fork.group("owned_arm"), + fork.group("fork_arm"), + frozenset({fork.group("owned_arm"), fork.group("fork_arm")}), + frozenset({fork.group("condition")}), + ) + matrix = _MATRIX_EXPRESSION.match(body) + if matrix is not None: + key = matrix.group("key") + values = _matrix_values(workflow_name, job_name, key) + return Placement( + "matrix", + None, + None, + _literal_labels(workflow_name, job_name, values), + frozenset({f"matrix.{key}"}), + ) + message = ( + f"{where} selects its runner from an expression this reader cannot " + f"model: {text!r}. Recording it as one opaque label would carry no " + "vendor prefix, so the lane would be dropped by every Ubicloud " + "classifier while still asking for a paid runner (axinite #372)." + ) + raise AssertionError(message) + + +def references(value: object) -> frozenset[str]: + """Return every ``${{ ... }}`` reference a declaration reads.""" + if not isinstance(value, str): + return frozenset() + return frozenset(body.strip() for body in _REFERENCE.findall(value) if body.strip()) + + +def never_runs(workflow_name: str, job_name: str) -> bool: + """Report whether a job's own ``if:`` makes it unreachable. + + A constant-false guard leaves every declaration intact and runs nothing, + which is exactly the state a placement contract must refuse rather than + report as compliant. + + Returns + ------- + bool + Whether the job's own ``if:`` reduces to a constant false. + """ + condition = job(workflow_name, job_name).get("if") + if not isinstance(condition, str): + return False + text = " ".join(condition.split()) + expression = _EXPRESSION.match(text) + if expression is not None: + text = " ".join(expression.group("body").split()) + # A containment test would accept `false && matrix.target == 'x'`; the + # leading clause is matched explicitly instead. + return text.lower() in _NEVER_RUNS or text.lower().startswith("false &&") diff --git a/tests/helpers/ci_runners.py b/tests/helpers/ci_runners.py index e22059abd..5bf8aba96 100644 --- a/tests/helpers/ci_runners.py +++ b/tests/helpers/ci_runners.py @@ -13,6 +13,16 @@ import typing as typ +from tests.helpers.ci_placement import ( + FORK_FIELD, + FROZEN_HOSTED_LABELS, + Placement, + all_jobs, + declares_steps, + never_runs, + placement, + references, +) from tests.helpers.ci_workflows import ( CACHE_ACTION_PIN, CACHE_PLAIN, @@ -46,8 +56,11 @@ # fmt: off __all__ = ( "CACHE_ACTION_PIN", "CACHE_PLAIN", "CACHE_RESTORE", "CACHE_SAVE", - "ROOT", "WORKFLOW_DIR", "cache_paths", "cache_steps", "expand", "job", - "job_env", "jobs", "restore_steps", "save_steps", + "FORK_FIELD", "FROZEN_HOSTED_LABELS", "ROOT", "WORKFLOW_DIR", + "Placement", "RUNNER_NAMED_JOBS", "all_jobs", "cache_paths", + "cache_steps", "ceiling", + "declares_steps", "expand", "job", "job_env", "jobs", "never_runs", + "placement", "references", "restore_steps", "save_steps", "single_step_position_using", "single_step_using", "step_inputs", "steps", "workflow_document", "workflow_env", "workflow_sources", ) @@ -98,14 +111,49 @@ #: and does real work, which is what buys it a paid runner. UBICLOUD_JOBS: typ.Final[cabc.Mapping[str, tuple[str, ...]]] = { "build-wheels.yml": ("build-pure-wheel", "verify-wheel-install"), - "ci.yml": ("typecheck-test", "extension-tests", "coverage", "benchmark-ratchet"), + "ci.yml": ( + "lint-test", + "typecheck-test", + "extension-tests", + "coverage", + "benchmark-ratchet", + ), "coverage-main.yml": ("coverage-upload",), } +#: Ubicloud lanes a pull request from a fork can reach, which must therefore +#: declare the fallback arm. Derived intent, not derived fact: the workflows +#: are read back against it, and `test_fork_reachability_matches_the_manifest` +#: holds it against the triggers so a lane cannot quietly leave the set. +#: +#: `coverage-upload` is absent because `coverage-main.yml` triggers only on a +#: push to `main` and a dispatch, neither of which a fork can cause. The two +#: `build-wheels.yml` jobs are present because `ci.yml` calls that workflow on +#: every pull request, so its own `workflow_call` trigger understates its +#: exposure (weaver: a called workflow's triggers are its callers'). +FORK_REACHABLE_UBICLOUD_JOBS: typ.Final[cabc.Mapping[str, tuple[str, ...]]] = { + "build-wheels.yml": ("build-pure-wheel", "verify-wheel-install"), + "ci.yml": ( + "lint-test", + "typecheck-test", + "extension-tests", + "coverage", + "benchmark-ratchet", + ), +} +#: The one job permitted to fail without failing the workflow, and the matrix +#: key that says so. The 3.15a leg tracks a pre-release interpreter and is not +#: a required context; every other leg gates a merge. +EXPERIMENTAL_LEG_KEY = "experimental" +CONTINUE_ON_ERROR_JOBS: typ.Final = (("ci.yml", "typecheck-test"),) #: Jobs that stay on GitHub-hosted runners, and why. Ubicloud offers Linux #: only, and a job that sleeps, calls an API, or publishes an artefact someone #: else built gains nothing from a metered build slot. GITHUB_HOSTED_JOBS: typ.Final[cabc.Mapping[str, tuple[str, ...]]] = { - "ci.yml": ("lint-test", "changes", "loom-smoke"), + # `lint-test` moved to the Ubicloud manifest: it is a developer-blocking + # Linux gate that compiles, which is what buys a paid runner. `loom-smoke` + # and `workflow-harness` stay here, and `rust-boundaries.yml`'s verifier + # lanes stay by this repository's own decision recorded below. + "ci.yml": ("changes", "loom-smoke"), "benchmark-gate-harness.yml": ("workflow-harness",), "delayed-pr-comment.yml": ("delay_and_comment",), "loom.yml": ("loom",), @@ -113,6 +161,15 @@ # Issue379 requires verifier schedules on GitHub-hosted Linux. "rust-boundaries.yml": ("verus", "extended"), } +#: Matrix jobs whose check context spells the runner label, reviewed and +#: accepted. A name reading a matrix key is stable per event, which is the +#: property the stability rule protects, but the context still carries a label +#: and a placement change would rename it. Two of `build-native-wheels`' +#: contexts are named verbatim in the `main-required-checks` ruleset, which is +#: why moving its Linux legs is the repository owner's decision rather than a +#: label change; `native` is not a required context today. +RUNNER_NAMED_JOBS: typ.Final = (("rust-boundaries.yml", "native"),) + #: Windows-native validation needs GitHub's hosted Windows image; Ubicloud #: offers Linux capacity only. WINDOWS_HOSTED_JOBS: typ.Final[cabc.Mapping[str, tuple[str, ...]]] = { @@ -178,7 +235,11 @@ #: the GitHub-hosted lane and the Ubicloud lane render different values and #: read different cache services; a key with two writers has one on each side. CACHE_WRITERS: typ.Final[cabc.Mapping[str, tuple[tuple[str, str], ...]]] = { - "CARGO_CACHE_KEY": (("ci.yml", "extension-tests"), ("ci.yml", "lint-test")), + # `lint-test` restores this key and no longer saves it. On its owned arm + # it renders the `self-hosted` lane, which is the family `extension-tests` + # writes, and the registry holds the resolved dependency graph either job + # would have archived. + "CARGO_CACHE_KEY": (("ci.yml", "extension-tests"),), # The compiler cache is written by whichever job actually compiles, and # each compile shape is its own family. See CACHE_FAMILY_WRITERS: this # mapping only says which jobs hold a save step, not which archive each @@ -214,12 +275,11 @@ cabc.Mapping[tuple[str, str, tuple[str, ...]], tuple[str, str]] ] = { ("CARGO_CACHE_KEY", "self-hosted", ()): ("ci.yml", "extension-tests"), - ("CARGO_CACHE_KEY", "github-hosted", ()): ("ci.yml", "lint-test"), ("TOOL_CACHE_KEY", "self-hosted", ("3.12",)): ("ci.yml", "typecheck-test"), ("TOOL_CACHE_KEY", "self-hosted", ("3.13",)): ("ci.yml", "typecheck-test"), ("TOOL_CACHE_KEY", "self-hosted", ("3.14",)): ("ci.yml", "typecheck-test"), ("TOOL_CACHE_KEY", "self-hosted", ("3.15",)): ("ci.yml", "typecheck-test"), - ("SCCACHE_CACHE_KEY", "github-hosted", ("3.13", "lint")): ( + ("SCCACHE_CACHE_KEY", "self-hosted", ("3.13", "lint")): ( "ci.yml", "lint-test", ), diff --git a/tests/test_ci_actionlint_registry.py b/tests/test_ci_actionlint_registry.py new file mode 100644 index 000000000..062a449a9 --- /dev/null +++ b/tests/test_ci_actionlint_registry.py @@ -0,0 +1,123 @@ +"""Contract tests for the actionlint runner-label registry. + +actionlint knows GitHub's own labels natively but not a self-hosted provider's, +so an unregistered label is not a lint error: the job simply queues for a +runner that never arrives. The registry is therefore asserted as an equality in +both directions against the labels the workflows actually resolve to, because a +subset assertion misses a stale registration and a vendor-prefix derivation +would exempt a second paid provider's labels from the question entirely. +""" + +from __future__ import annotations + +import re + +import yaml + +from tests.helpers.ci_placement import FROZEN_HOSTED_LABELS, declares_steps, placement +from tests.helpers.ci_runners import ROOT, all_jobs, workflow_sources + +ACTIONLINT_CONFIG = ROOT / ".github" / "actionlint.yaml" +#: A `vars.*` reference in a workflow. actionlint checks such a reference +#: against `config-variables`, so the declared list and the references read +#: back from the workflows have to agree in both directions: an undeclared +#: name is a lint error, and a declared name nothing reads is a stale +#: allowance that would let a deleted variable's typo pass. +CONFIGURATION_VARIABLE = re.compile(r"vars\.([A-Za-z_][A-Za-z0-9_]*)") +ALL_CASES = all_jobs() + + +def _labels_in_use() -> frozenset[str]: + """Return every runner label the estate's jobs can resolve to. + + Derived from every job, both arms of a conditional and every matrix runner + entry, because a registry assertion is only as complete as its derivation. + Netsuke's contract searched the concatenated workflow text for each + registered label, so a label named only in a comment explaining why a lane + no longer used it stayed registered for ever. + + Returns + ------- + frozenset[str] + Every label any step-declaring job in the estate can resolve to. + """ + labels: set[str] = set() + for workflow_name, job_name in ALL_CASES: + if not declares_steps(workflow_name, job_name): + # A reusable-workflow caller declares no runner. Exempt by `uses`, + # not by a missing `runs-on`: a job with neither is still refused + # by the reader (whitaker #438). + continue + labels |= placement(workflow_name, job_name).labels + return frozenset(labels) + + +def test_actionlint_registers_exactly_the_self_hosted_labels_in_use() -> None: + """Register intentional labels so a typo fails lint instead of queueing. + + Equality in both directions. A subset assertion misses a stale + registration, and deriving "in use" by a vendor prefix would silently + exempt a second paid provider's labels from the registry question + altogether, which is the substantive defect; the named frozen set and the + prefix filter agree over today's workflows and differ on exactly that case. + actionlint knows GitHub's own labels natively, so the frozen set is + subtracted rather than registered (whitaker #438). + """ + config = yaml.safe_load(ACTIONLINT_CONFIG.read_text(encoding="utf-8")) + declared = config["self-hosted-runner"]["labels"] + used = _labels_in_use() + unknown = used - frozenset(FROZEN_HOSTED_LABELS) + assert sorted(declared) == sorted(unknown), ( + "actionlint must register every label in use that is neither a " + f"GitHub-hosted label nor already registered; declared {sorted(declared)}, " + f"derived {sorted(unknown)}" + ) + # Carried forward from main, which replaced a hardcoded + # `["CODESCENE_CLI_SHA256"]` with a derivation after that variable's last + # reader was deleted. Keeping the old literal through this rebase would + # have silently reverted that fix and re-asserted a variable nothing reads. + # + # Compared as sorted lists: the config registers names for lint to + # resolve, and actionlint does not care what order they appear in. Sorting + # still fails on a duplicate, so the comparison loses nothing. + read = { + match.group(1) + for _, source in workflow_sources() + for match in CONFIGURATION_VARIABLE.finditer(source) + } + assert sorted(config["config-variables"]) == sorted(read), ( + "list only the configuration variables the workflows read, so a typo " + f"in a vars.* reference fails lint; declared " + f"{config['config-variables']}, read {sorted(read)}" + ) + + +def test_every_hosted_label_in_use_is_named_in_the_frozen_set() -> None: + """Keep the frozen set a closed list rather than an implicit tolerance. + + Without this the registry equality above could be satisfied by widening + `FROZEN_HOSTED_LABELS` to swallow a paid provider's label, which is the + failure the named set exists to prevent. + """ + hosted = _labels_in_use() & frozenset(FROZEN_HOSTED_LABELS) + config = yaml.safe_load(ACTIONLINT_CONFIG.read_text(encoding="utf-8")) + registered = frozenset(config["self-hosted-runner"]["labels"]) + assert not (hosted & registered), ( + "a GitHub-hosted label must not also be registered as self-hosted; " + f"both claim {sorted(hosted & registered)}" + ) + assert hosted == _labels_in_use() - registered, ( + "every label in use must be either registered or named in the frozen " + f"hosted set; unaccounted: {sorted(_labels_in_use() - registered - hosted)}" + ) + + +def test_no_retired_runner_labels_remain() -> None: + """Leave no Namespace label or cache action behind after the migration.""" + for workflow_name, source in workflow_sources(): + assert "namespace-profile" not in source, ( + f"{workflow_name} still references a Namespace runner profile" + ) + assert "nscloud" not in source, ( + f"{workflow_name} still references the Namespace cache action" + ) diff --git a/tests/test_ci_cache_families_helper.py b/tests/test_ci_cache_families_helper.py index 71dad1a82..9cf608298 100644 --- a/tests/test_ci_cache_families_helper.py +++ b/tests/test_ci_cache_families_helper.py @@ -17,6 +17,7 @@ from hypothesis import strategies as st from tests.helpers import ci_cache_families as families +from tests.helpers.ci_placement import Placement if typ.TYPE_CHECKING: from tests.helpers.workflow_types import Step @@ -133,8 +134,40 @@ def test_an_unmapped_runner_label_is_a_contract_failure( "uses": families.CACHE_KEYS_ACTION, "with": {"python-version": "3.13", "compiler-shape": "debug"}, } - monkeypatch.setattr(families, "job", lambda *_: {"runs-on": "some-new-shape"}) + monkeypatch.setattr( + families, + "placement", + lambda *_: Placement( + "literal", "some-new-shape", None, frozenset(), frozenset() + ), + ) monkeypatch.setattr(families, "steps", lambda *_: [renderer]) monkeypatch.setattr(families, "save_steps", lambda *_: []) with pytest.raises(AssertionError, match="unmapped label"): families.writer_families("w.yml", "j") + + +def test_a_job_without_one_owned_runner_has_no_cache_lane( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Refuse to place an archive for a job whose runner the matrix decides. + + A matrix placement resolves to several labels and therefore to several + lanes. Choosing one would attribute an archive to a store it may never + reach, so the reader refuses rather than guesses. + """ + renderer = { + "uses": families.CACHE_KEYS_ACTION, + "with": {"python-version": "3.13", "compiler-shape": "debug"}, + } + monkeypatch.setattr( + families, + "placement", + lambda *_: Placement( + "matrix", None, None, frozenset({"ubuntu-latest"}), frozenset() + ), + ) + monkeypatch.setattr(families, "steps", lambda *_: [renderer]) + monkeypatch.setattr(families, "save_steps", lambda *_: []) + with pytest.raises(AssertionError, match="no single owned runner"): + families.writer_families("w.yml", "j") diff --git a/tests/test_ci_placement_reader.py b/tests/test_ci_placement_reader.py new file mode 100644 index 000000000..b1e32059e --- /dev/null +++ b/tests/test_ci_placement_reader.py @@ -0,0 +1,336 @@ +"""Tests for the placement reader itself, driven by synthetic declarations. + +The placement contracts in `tests/test_ci_runner_placement.py` are +parametrized over this repository's own workflows, which are correct. A rule +cannot be proved by the sources it guards: parametrized over correct files it +passes whether or not it discriminates, so the reader is driven directly here +with the shapes a workflow could acquire. + +The shapes that must be *refused* matter as much as the ones that must be +read. An unparsable `runs-on` recorded as one opaque label carries no vendor +prefix, so the Ubicloud classifier drops the lane and it sits exempt from every +placement, ceiling and registry assertion while still asking for a paid runner +(axinite #372). +""" + +from __future__ import annotations + +import pytest + +from tests.helpers import ci_placement as reader + +#: Every workflow the traversal must reach. Named rather than derived, so the +#: claim that `all_jobs` sees the whole estate is asserted rather than merely +#: true: a file the reader skipped would otherwise be invisible. +ESTATE_WORKFLOWS = frozenset({ + "benchmark-gate-harness.yml", + "build-wheels.yml", + "ci.yml", + "coverage-main.yml", + "delayed-pr-comment.yml", + "dependabot-automerge.yml", + "loom.yml", + "mutation-testing.yml", + "release.yml", + "rust-boundaries.yml", +}) +OWNED = "ubicloud-standard-2" +HOSTED = "ubuntu-latest" +FORK_EXPRESSION = f"${{{{ {reader.FORK_FIELD} && '{HOSTED}' || '{OWNED}' }}}}" + + +def _declare(monkeypatch: pytest.MonkeyPatch, declared: dict[str, object]) -> None: + """Make the reader see one synthetic job declaration.""" + monkeypatch.setattr(reader, "job", lambda *_: declared) + + +def test_a_bare_label_is_read_as_itself(monkeypatch: pytest.MonkeyPatch) -> None: + """Keep the common case simple: one literal label, no arms, no references.""" + _declare(monkeypatch, {"runs-on": "windows-2022", "steps": []}) + placed = reader.placement("w.yml", "j") + assert placed.kind == "literal", f"a bare label is a literal, got {placed.kind}" + assert placed.owned == "windows-2022", f"owned arm was {placed.owned!r}" + assert placed.fork is None, ( + f"a literal placement has no fork arm, got {placed.fork!r}" + ) + assert placed.labels == frozenset({"windows-2022"}), ( + f"a literal resolves to one label, got {sorted(placed.labels)}" + ) + assert placed.references == frozenset(), ( + f"a literal reads no expression, got {sorted(placed.references)}" + ) + + +def test_the_fork_expression_is_read_by_position( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Report which arm is which, so a caller can assert them separately.""" + _declare(monkeypatch, {"runs-on": FORK_EXPRESSION, "steps": []}) + placed = reader.placement("w.yml", "j") + assert placed.kind == "fork", f"the fallback is a fork placement, got {placed.kind}" + assert placed.fork == HOSTED, f"fork arm was {placed.fork!r}" + assert placed.owned == OWNED, f"owned arm was {placed.owned!r}" + assert placed.labels == frozenset({HOSTED, OWNED}), ( + f"both arms are labels in use, got {sorted(placed.labels)}" + ) + assert placed.references == frozenset({reader.FORK_FIELD}), ( + f"the condition must be the fork field, got {sorted(placed.references)}" + ) + + +def test_swapped_arms_are_reported_as_swapped( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Discriminate the defect a membership test cannot see. + + Nile-valley #106's contract asserted that the expression named one hosted + and one Ubicloud label somewhere. This declaration satisfies that and sends + every fork to a runner no fork can obtain. + """ + swapped = f"${{{{ {reader.FORK_FIELD} && '{OWNED}' || '{HOSTED}' }}}}" + _declare(monkeypatch, {"runs-on": swapped, "steps": []}) + placed = reader.placement("w.yml", "j") + assert placed.fork == OWNED, "the reader must report the arm as declared" + assert placed.owned == HOSTED, "the owned arm here is the hosted label" + + +def test_a_sibling_field_is_reported_as_a_different_condition( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Separate a well-formed expression from a correct one. + + `head.repo.private` produces an expression with the right shape and the + right two arms that branches on the wrong thing. Only the condition itself + distinguishes it, so the reader returns the condition rather than a + boolean. + """ + sibling = ( + "${{ github.event.pull_request.head.repo.private" + f" && '{HOSTED}' || '{OWNED}' }}}}" + ) + _declare(monkeypatch, {"runs-on": sibling, "steps": []}) + placed = reader.placement("w.yml", "j") + assert placed.kind == "fork", f"the shape is still a fork arm, got {placed.kind}" + assert placed.references != frozenset({reader.FORK_FIELD}), ( + "a sibling field must be distinguishable from the fork field" + ) + + +def test_a_matrix_placement_resolves_through_its_include( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Follow `runs-on` into the matrix values it selects, not merely to the key.""" + _declare( + monkeypatch, + { + "runs-on": "${{ matrix.os }}", + "steps": [], + "strategy": { + "matrix": {"include": [{"os": "macos-latest"}, {"os": HOSTED}]} + }, + }, + ) + placed = reader.placement("w.yml", "j") + assert placed.kind == "matrix", f"a matrix key placement, got {placed.kind}" + assert placed.owned is None, ( + f"a matrix placement has no single owned label, got {placed.owned!r}" + ) + assert placed.labels == frozenset({"macos-latest", HOSTED}), ( + f"every include entry is a label, got {sorted(placed.labels)}" + ) + assert placed.references == frozenset({"matrix.os"}), ( + f"the placement reads the matrix key, got {sorted(placed.references)}" + ) + + +def test_a_matrix_value_holding_an_expression_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Refuse the shape rstest-bdd #788's contract read straight through. + + A reader that stopped at `matrix.os` never saw that the value behind the + key was itself the fork expression, so a name reading the fork field shared + no reference with `runs-on` and the stability contract passed while the + name rendered differently per event. + """ + _declare( + monkeypatch, + { + "runs-on": "${{ matrix.os }}", + "steps": [], + "strategy": {"matrix": {"include": [{"os": FORK_EXPRESSION}]}}, + }, + ) + with pytest.raises(AssertionError, match="cannot model"): + reader.placement("w.yml", "j") + + +@pytest.mark.parametrize( + ("declared", "expected"), + [ + pytest.param( + {"runs-on": ["self-hosted", "linux"], "steps": []}, + "list runs-on", + id="list", + ), + pytest.param({"runs-on": [], "steps": []}, "list runs-on", id="empty-list"), + pytest.param({"steps": []}, "non-empty string", id="absent"), + pytest.param({"runs-on": "", "steps": []}, "non-empty string", id="blank"), + pytest.param( + {"runs-on": "${{ inputs.chosen-os }}", "steps": []}, + "cannot model", + id="opaque-expression", + ), + pytest.param( + { + "runs-on": f"${{{{ {reader.FORK_FIELD}\n && '{HOSTED}' }}}}", + "steps": [], + }, + "line break", + id="folded-scalar", + ), + ], +) +def test_unmodelled_declarations_are_refused( + monkeypatch: pytest.MonkeyPatch, declared: dict[str, object], expected: str +) -> None: + """Fail on a shape the reader cannot model rather than record it as read.""" + _declare(monkeypatch, declared) + with pytest.raises(AssertionError, match=expected): + reader.placement("w.yml", "j") + + +def test_the_refusal_of_an_opaque_expression_is_still_needed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Fail if the reader ever learns the shape the test above refuses. + + A refusal contract goes quiet the day the reader gains the ability it was + written to compensate for, and a quiet contract reads exactly like a + passing one. This makes that day loud: the refusal becomes redundant, not + invisible, and both tests are then rewritten together. + """ + _declare(monkeypatch, {"runs-on": "${{ inputs.chosen-os }}", "steps": []}) + with pytest.raises(AssertionError): + reader.placement("w.yml", "j") + + +@pytest.mark.parametrize( + "condition", + ["false", "${{ false }}", "'false'", "false && matrix.target == 'x'", "0"], +) +def test_a_constant_false_guard_is_reported( + monkeypatch: pytest.MonkeyPatch, condition: str +) -> None: + """Catch a lane that satisfies every declaration rule and runs nothing.""" + _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) + assert reader.never_runs("w.yml", "j"), ( + f"{condition!r} can never be true, so the job gates nothing" + ) + + +@pytest.mark.parametrize( + "condition", + [ + "github.event_name == 'pull_request'", + "needs.changes.outputs.bench == 'true'", + "${{ !cancelled() }}", + "matrix.python-suite", + ], +) +def test_a_real_guard_is_not_reported_as_never_running( + monkeypatch: pytest.MonkeyPatch, condition: str +) -> None: + """Prove the rule narrow: a legitimate condition must still pass. + + Sufficiency alone is not enough. A guard rule that also refused the + conditions this repository genuinely uses would be switched off, and a + contract with a false positive gates nothing (femtologging #480). + """ + _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) + assert not reader.never_runs("w.yml", "j"), ( + f"{condition!r} is a legitimate guard and must not be refused" + ) + + +@pytest.mark.parametrize( + ("declared", "expected"), + [ + pytest.param({"steps": []}, True, id="steps"), + pytest.param({"uses": "./w.yml"}, False, id="caller"), + pytest.param({"uses": "./w.yml", "steps": []}, False, id="both"), + pytest.param({}, False, id="neither"), + ], +) +def test_the_caller_exemption_is_keyed_on_uses( + monkeypatch: pytest.MonkeyPatch, declared: dict[str, object], expected: bool +) -> None: + """Exempt a reusable-workflow caller, and refuse a job with neither key. + + A blanket tolerance of a missing `runs-on` would exempt a malformed job + too; keying on `uses` leaves that job refused (whitaker #438). + """ + _declare(monkeypatch, declared) + assert reader.declares_steps("w.yml", "j") is expected, ( + f"{sorted(declared)} must read as declares_steps={expected}" + ) + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + pytest.param("plain", frozenset(), id="no-reference"), + pytest.param("${{ matrix.os }}", frozenset({"matrix.os"}), id="one"), + pytest.param( + "a ${{ matrix.a }} b ${{ matrix.b }}", + frozenset({"matrix.a", "matrix.b"}), + id="two", + ), + pytest.param(None, frozenset(), id="absent"), + pytest.param(True, frozenset(), id="non-string"), + ], +) +def test_references_are_read_from_any_declaration( + value: object, expected: frozenset[str] +) -> None: + """Read every reference, so a name and a runner can be compared.""" + assert reader.references(value) == expected, ( + f"{value!r} reads {sorted(reader.references(value))}" + ) + + +def test_a_matrix_leg_missing_the_runner_key_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Refuse an incomplete matrix rather than silently dropping a leg.""" + _declare( + monkeypatch, + { + "runs-on": "${{ matrix.os }}", + "steps": [], + "strategy": {"matrix": {"include": [{"os": HOSTED}, {"arch": "x86_64"}]}}, + }, + ) + with pytest.raises(AssertionError, match="declares no 'os'"): + reader.placement("w.yml", "j") + + +def test_all_jobs_reaches_every_workflow() -> None: + """Name the traversal, or the claim that it is complete is unasserted. + + Falcon-pachinko #149: a `workflow_call` file with no caller is one line + from gaining one, and a caller can live in another repository, so the + registry counts its labels rather than excluding the file by rule. + """ + reached = {workflow_name for workflow_name, _ in reader.all_jobs()} + assert reached == ESTATE_WORKFLOWS, ( + f"the traversal must reach every workflow; reached {sorted(reached)}" + ) + + +def test_the_frozen_hosted_set_holds_no_paid_label() -> None: + """Keep the exemption list free of the labels it exists to expose.""" + assert all( + not label.startswith(("ubicloud-", "namespace-", "buildjet-", "warpbuild-")) + for label in reader.FROZEN_HOSTED_LABELS + ), "a paid provider's label must never be frozen out of the registry question" diff --git a/tests/test_ci_resource_sampling.py b/tests/test_ci_resource_sampling.py index 93356882f..5ca17eb5c 100644 --- a/tests/test_ci_resource_sampling.py +++ b/tests/test_ci_resource_sampling.py @@ -22,6 +22,10 @@ #: every 15 s is interference rather than observation; its disk figure was #: taken once, in run 33857764655. SAMPLED_JOBS: typ.Final = ( + # `lint-test` joined this list when it moved onto the paid lane: it builds + # Whitaker, clippy under two toolchains and a Windows cross-target check on + # two vCPU, which is exactly the shape whose failure mode is a silent death. + ("ci.yml", "lint-test"), ("ci.yml", "typecheck-test"), ("ci.yml", "extension-tests"), ("ci.yml", "coverage"), diff --git a/tests/test_ci_runner_placement.py b/tests/test_ci_runner_placement.py index 673901dc6..d19cdce92 100644 --- a/tests/test_ci_runner_placement.py +++ b/tests/test_ci_runner_placement.py @@ -1,68 +1,52 @@ -"""Contract tests for where Cuprum's CI jobs run and how wide they fan out. - -Runner placement and worker counts are declarations no functional test can -reach: a job that drifts back to a GitHub-hosted label, or that asks -`pytest-xdist` for more workers than the runner has cores, still produces a -green suite while queueing for hours or thrashing two vCPUs. These tests read -the declarations back against the manifests in `tests/helpers/ci_runners.py`. +"""Contract tests for where Cuprum's CI jobs run and whether they run. + +Runner placement is a declaration no functional test can reach: a job that +drifts back to a GitHub-hosted label, asks for a paid runner a fork can never +obtain, or carries no ceiling at all still produces a green suite while +queueing for hours or billing for six. These tests read the declarations back +against the manifests in `tests/helpers/ci_runners.py`, through the one reader +in `tests/helpers/ci_placement.py`. + +Companion modules cover the neighbouring declarations: the label registry in +`test_ci_actionlint_registry.py`, worker counts in `test_ci_worker_bounds.py`, +and tool installation in `test_ci_tool_installation.py`. """ from __future__ import annotations -import re -import typing as typ - import pytest import yaml from tests.helpers.ci_runners import ( + CONTINUE_ON_ERROR_JOBS, + EXPERIMENTAL_LEG_KEY, + FORK_FIELD, + FORK_REACHABLE_UBICLOUD_JOBS, GITHUB_HOSTED_JOBS, GITHUB_LABEL, - ROOT, + RUNNER_NAMED_JOBS, UBICLOUD_JOBS, UBICLOUD_LABEL, - UBICLOUD_VCPUS, WINDOWS_HOSTED_JOBS, WINDOWS_LABEL, + all_jobs, + declares_steps, expand, job, - step_inputs, - steps, + never_runs, + placement, + references, workflow_document, - workflow_env, workflow_sources, ) -if typ.TYPE_CHECKING: - from tests.helpers.workflow_types import Step - -ACTIONLINT_CONFIG = ROOT / ".github" / "actionlint.yaml" -MAKEFILE = ROOT / "Makefile" -VCPU_CONSTANT = "LINUX_RUNNER_VCPUS" -#: Any spelling of the tool, so `nextest@` and `cargo-nextest@` both match. -NEXTEST_TOOL_NAME = "nextest" -#: Shell fragments that mean "fetch a binary" rather than "run one". -INSTALL_VERBS = ("install", "curl", "wget") -#: The upstream install host. Its name does not contain "nextest", so it needs -#: its own token, and it only ever appears in an install command. -NEXTEST_INSTALL_HOST = "get.nexte.st" -#: Steps invoking a shared action are exempt: the coverage action installs -#: nextest on purpose, and that is the only sanctioned place. -SHARED_ACTION_PREFIX = "leynos/shared-actions/" -#: A `vars.*` reference in a workflow. actionlint checks such a reference -#: against `config-variables`, so the declared list and the references read -#: back from the workflows have to agree in both directions: an undeclared -#: name is a lint error, and a declared name nothing reads is a stale -#: allowance that would let a deleted variable's typo pass. -CONFIGURATION_VARIABLE = re.compile(r"vars\.([A-Za-z_][A-Za-z0-9_]*)") -#: Make variables that carry the runner's vCPU count into the test command. -#: Only the pytest one remains here: the Rust suite moved to the coverage job, -#: which bounds itself through `CARGO_BUILD_JOBS` and `NEXTEST_TEST_THREADS`. -PARALLELISM_OVERRIDES = ("PYTEST_CARGO_BUILD_JOBS",) - UBICLOUD_CASES = expand(UBICLOUD_JOBS) GITHUB_HOSTED_CASES = expand(GITHUB_HOSTED_JOBS) WINDOWS_HOSTED_CASES = expand(WINDOWS_HOSTED_JOBS) +FORK_REACHABLE_CASES = expand(FORK_REACHABLE_UBICLOUD_JOBS) +ALL_CASES = all_jobs() +STEP_CASES = [case for case in ALL_CASES if declares_steps(*case)] +CALLER_CASES = [case for case in ALL_CASES if not declares_steps(*case)] @pytest.mark.parametrize(("workflow_name", "job_name"), UBICLOUD_CASES) @@ -70,30 +54,205 @@ def test_linux_build_and_test_jobs_use_the_ubicloud_default_shape( workflow_name: str, job_name: str ) -> None: """Keep every repository-owned Linux gate on the reviewed Ubicloud shape.""" - runner = job(workflow_name, job_name).get("runs-on") - assert runner == UBICLOUD_LABEL, ( + owned = placement(workflow_name, job_name).owned + assert owned == UBICLOUD_LABEL, ( f"{workflow_name}:{job_name} must run on {UBICLOUD_LABEL}; escalating to a " - f"larger shape needs recorded measurements, got {runner!r}" + f"larger shape needs recorded measurements, got {owned!r}" ) -@pytest.mark.parametrize(("workflow_name", "job_name"), UBICLOUD_CASES) -def test_ubicloud_jobs_declare_a_timeout(workflow_name: str, job_name: str) -> None: - """Bound a wedged paid runner rather than paying for its default six hours.""" +@pytest.mark.parametrize(("workflow_name", "job_name"), FORK_REACHABLE_CASES) +def test_fork_reachable_lanes_fall_back_by_position( + workflow_name: str, job_name: str +) -> None: + """Send a fork's pull request to a runner it can actually obtain. + + Read by position, not by membership. Nile-valley #106's contract asserted + that the expression named one hosted and one Ubicloud label somewhere, so + swapping the arms passed while sending forks to a runner no fork can get. + The condition is full-matched too: a sibling field such as + `head.repo.private` produces a well-formed expression that branches on the + wrong thing. + """ + placed = placement(workflow_name, job_name) + assert placed.kind == "fork", ( + f"{workflow_name}:{job_name} must select its runner with the fork " + f"fallback expression, got a {placed.kind} placement" + ) + assert placed.references == frozenset({FORK_FIELD}), ( + f"{workflow_name}:{job_name} must branch on {FORK_FIELD}, " + f"got {sorted(placed.references)}" + ) + assert placed.fork == GITHUB_LABEL, ( + f"{workflow_name}:{job_name} must send a fork to {GITHUB_LABEL}, " + f"got {placed.fork!r}" + ) + assert placed.owned == UBICLOUD_LABEL, ( + f"{workflow_name}:{job_name} must keep {UBICLOUD_LABEL} on the owned " + f"arm, got {placed.owned!r}" + ) + + +@pytest.mark.parametrize(("workflow_name", "job_name"), STEP_CASES) +def test_placement_expressions_parse_to_one_line( + workflow_name: str, job_name: str +) -> None: + """Refuse a folded scalar whose continuation kept its line break. + + A continuation indented one level deeper puts a newline inside the + expression. GitHub evaluates the broken value regardless, so a green run is + no evidence; only the parsed document shows it (dev-env-rocky #216). + + Reading the declaration is the assertion: the reader refuses an embedded + line break, a list, an absent value and any expression it cannot model. + """ + placement(workflow_name, job_name) + + +def test_fork_reachability_matches_the_manifest() -> None: + """Hold the fallback manifest against the triggers it rests on. + + `runs-on` can never check the premise these rules depend on: that a fork + can reach the lane at all. Without the assertion a silent trigger change + leaves the consequence looking deliberate (falcon-pachinko). YAML 1.1 reads + the bare word `on` as the boolean `True`, so the triggers are read under + that key; reading the string `"on"` would find nothing and pass vacuously. + """ + ci_triggers = workflow_document("ci.yml")[True] + assert isinstance(ci_triggers, dict), "ci.yml must declare its triggers" + assert "pull_request" in ci_triggers, ( + "ci.yml must declare the pull_request trigger; without it no fork " + "reaches these lanes and every fallback arm below is dead code" + ) + caller = job("ci.yml", "build-wheels").get("uses") + assert caller == "./.github/workflows/build-wheels.yml", ( + "ci.yml must call build-wheels.yml; that call, not build-wheels.yml's " + "own workflow_call trigger, is what exposes its jobs to forks" + ) + coverage_triggers = workflow_document("coverage-main.yml")[True] + assert isinstance(coverage_triggers, dict), ( + "coverage-main.yml must declare its triggers" + ) + assert "pull_request" not in coverage_triggers, ( + "coverage-main.yml is absent from the fork manifest because no fork " + "can trigger it; a pull_request trigger here would make that false" + ) + + +@pytest.mark.parametrize(("workflow_name", "job_name"), STEP_CASES) +def test_every_job_running_steps_declares_a_ceiling( + workflow_name: str, job_name: str +) -> None: + """Bound a wedged runner rather than paying for the six-hour default. + + Keyed on declaring steps rather than on carrying an Ubicloud label. Once a + label is an expression, "an Ubicloud lane" is a property of the event, so a + rule keyed on the label stops applying on exactly the arm that hangs. + """ timeout = job(workflow_name, job_name).get("timeout-minutes") assert isinstance(timeout, int), ( f"{workflow_name}:{job_name} must declare timeout-minutes, got {timeout!r}" ) +@pytest.mark.parametrize(("workflow_name", "job_name"), CALLER_CASES) +def test_reusable_workflow_callers_declare_no_ceiling( + workflow_name: str, job_name: str +) -> None: + """Keep placing a job and bounding it as separate ideas. + + GitHub rejects `timeout-minutes` on a job with `uses:`, so the bound lives + in the callee. Each of this repository's callers calls a workflow whose own + jobs are bounded by the rule above (weaver). + """ + declared = job(workflow_name, job_name) + assert "uses" in declared, ( + f"{workflow_name}:{job_name} declares neither steps nor uses" + ) + assert declared.get("timeout-minutes") is None, ( + f"{workflow_name}:{job_name} calls a reusable workflow, where GitHub " + "rejects timeout-minutes; bound the callee instead" + ) + + +@pytest.mark.parametrize(("workflow_name", "job_name"), ALL_CASES) +def test_no_reviewed_lane_is_switched_off(workflow_name: str, job_name: str) -> None: + """Read whether a job runs, not only how it is configured. + + Every placement, budget and cache rule in this repository reads a + declaration. `if: false` leaves all of them satisfied and runs nothing + (lille #349). A containment test would accept `false && matrix.x == 'y'`, + so the leading clause is matched explicitly. + """ + assert not never_runs(workflow_name, job_name), ( + f"{workflow_name}:{job_name} can never run, so every rule about where " + "it runs and what it costs holds vacuously" + ) + + +@pytest.mark.parametrize(("workflow_name", "job_name"), ALL_CASES) +def test_only_the_experimental_matrix_leg_may_fail_silently( + workflow_name: str, job_name: str +) -> None: + """Keep `continue-on-error` to the leg that is allowed to be broken.""" + declared = job(workflow_name, job_name).get("continue-on-error") + if declared is None: + return + assert (workflow_name, job_name) in CONTINUE_ON_ERROR_JOBS, ( + f"{workflow_name}:{job_name} tolerates its own failure but is not in " + "the manifest; a gate that cannot fail the workflow gates nothing" + ) + assert references(declared) == frozenset({f"matrix.{EXPERIMENTAL_LEG_KEY}"}), ( + f"{workflow_name}:{job_name} must tolerate failure only on the leg its " + f"matrix marks {EXPERIMENTAL_LEG_KEY!r}, got {declared!r}" + ) + + +@pytest.mark.parametrize(("workflow_name", "job_name"), STEP_CASES) +def test_job_names_do_not_follow_their_runner( + workflow_name: str, job_name: str +) -> None: + """Keep a required check's context stable across events. + + The hazard is event dependence, not matrix dependence. With the fork + fallback the runner follows the event, so a name reading that condition + renders differently on a fork's pull request and an internal one, and no + single required context exists on both. + + A name reading a matrix key is a different thing: the values behind the key + are literals, so the name varies per leg and is stable per event, which is + how matrix legs get distinct contexts at all. Those jobs are listed rather + than waved through, because a context spelling a runner label is still a + context that a placement change would rename (shared-actions #480). + + Resolved through the matrix either way: rstest-bdd #788's contract compared + the name's references with `runs-on`'s, which read `matrix.os` and stopped, + while the matrix value behind that key held the fork expression. + """ + placed = placement(workflow_name, job_name) + shared = references(job(workflow_name, job_name).get("name")) & placed.references + if placed.kind == "matrix": + assert (workflow_name, job_name) in RUNNER_NAMED_JOBS or not shared, ( + f"{workflow_name}:{job_name} spells its runner in its name but is " + "not in RUNNER_NAMED_JOBS; a placement change would rename its " + "check context" + ) + return + assert not shared, ( + f"{workflow_name}:{job_name} names itself from {sorted(shared)}, which " + "its runner also reads; the check context would follow the event" + ) + + @pytest.mark.parametrize(("workflow_name", "job_name"), GITHUB_HOSTED_CASES) def test_administrative_and_serial_jobs_stay_github_hosted( workflow_name: str, job_name: str ) -> None: """Keep sleeping, API-bound, and publish-only work off metered build slots.""" - runner = job(workflow_name, job_name).get("runs-on") - assert runner == GITHUB_LABEL, ( - f"{workflow_name}:{job_name} must stay on {GITHUB_LABEL}, got {runner!r}" + placed = placement(workflow_name, job_name) + assert placed.labels == frozenset({GITHUB_LABEL}), ( + f"{workflow_name}:{job_name} must stay on {GITHUB_LABEL}, " + f"got {sorted(placed.labels)}" ) @@ -101,19 +260,31 @@ def test_administrative_and_serial_jobs_stay_github_hosted( def test_windows_native_jobs_stay_on_github_hosted_windows( workflow_name: str, job_name: str ) -> None: - """Keep native Windows validation on the reviewed hosted runner image.""" - runner = job(workflow_name, job_name).get("runs-on") - assert runner == WINDOWS_LABEL, ( - f"{workflow_name}:{job_name} must stay on {WINDOWS_LABEL}, got {runner!r}" + """Keep native Windows validation on the reviewed hosted runner image. + + A hosted-at-all predicate is not a placement predicate: an API-bound job + moved to windows-latest satisfied a contract whose message named + ubuntu-latest (lille #349), so the label is asserted by equality. + """ + placed = placement(workflow_name, job_name) + assert placed.labels == frozenset({WINDOWS_LABEL}), ( + f"{workflow_name}:{job_name} must stay on {WINDOWS_LABEL}, " + f"got {sorted(placed.labels)}" ) def test_native_wheel_matrix_keeps_its_platform_runners() -> None: """Ubicloud has no Windows or macOS capacity, so the matrix stays hosted.""" matrix_job = job("build-wheels.yml", "build-native-wheels") - assert matrix_job.get("runs-on") == "${{ matrix.os }}", ( + placed = placement("build-wheels.yml", "build-native-wheels") + assert placed.kind == "matrix", ( "build-wheels.yml:build-native-wheels must keep its platform matrix" ) + assert UBICLOUD_LABEL not in placed.labels, ( + "both ubuntu legs are named verbatim in the main-required-checks " + "ruleset, so moving either renames a required context; that is the " + "repository owner's decision, not a placement change" + ) strategy = matrix_job.get("strategy") assert isinstance(strategy, dict), "the native wheel job must declare a strategy" matrix = strategy.get("matrix") @@ -153,236 +324,6 @@ def test_every_workflow_job_appears_in_one_placement_manifest() -> None: ) -def test_actionlint_registers_exactly_the_self_hosted_labels_in_use() -> None: - """Register intentional labels so a typo fails lint instead of queueing.""" - config = yaml.safe_load(ACTIONLINT_CONFIG.read_text(encoding="utf-8")) - declared = config["self-hosted-runner"]["labels"] - used = { - str(job(workflow_name, job_name).get("runs-on")) - for workflow_name, job_name in UBICLOUD_CASES - } - assert sorted(declared) == sorted(used), ( - "actionlint must list every self-hosted label the workflows use and no " - f"others; declared {declared}, used {sorted(used)}" - ) - # Compared as sorted lists: the config registers names for lint to - # resolve, and actionlint does not care what order they appear in. Sorting - # still fails on a duplicate, so the comparison loses nothing. - read = { - match.group(1) - for _, source in workflow_sources() - for match in CONFIGURATION_VARIABLE.finditer(source) - } - assert sorted(config["config-variables"]) == sorted(read), ( - "list only the configuration variables the workflows read, so a typo " - f"in a vars.* reference fails lint; declared " - f"{config['config-variables']}, read {sorted(read)}" - ) - - -def test_no_retired_runner_labels_remain() -> None: - """Leave no Namespace label or cache action behind after the migration.""" - for workflow_name, source in workflow_sources(): - assert "namespace-profile" not in source, ( - f"{workflow_name} still references a Namespace runner profile" - ) - assert "nscloud" not in source, ( - f"{workflow_name} still references the Namespace cache action" - ) - - -def test_the_vcpu_constant_matches_the_assigned_label() -> None: - """Tie the one parallelism constant to the shape the job is billed for.""" - declared = workflow_env("ci.yml")[VCPU_CONSTANT] - assert declared == str(UBICLOUD_VCPUS), ( - f"{VCPU_CONSTANT} must equal the vCPU count of {UBICLOUD_LABEL}, " - f"got {declared!r}" - ) - - -def test_python_tests_derive_their_worker_counts_from_that_constant() -> None: - """Size the matrix suite's Cargo work from the constant, not a literal.""" - script = next( - step["run"] - for step in steps("ci.yml", "typecheck-test") - if step.get("name") == "Run tests" - ) - assert isinstance(script, str), "ci.yml:typecheck-test must run a test script" - for variable in PARALLELISM_OVERRIDES: - assert f'{variable}="${{{VCPU_CONSTANT}}}"' in script, ( - f"ci.yml:typecheck-test must pass {variable} from {VCPU_CONSTANT}" - ) - - -def test_extension_and_benchmark_builds_are_bounded_too() -> None: - """Bound the two jobs that compile outside `make test` to the same count.""" - for job_name, step_name in ( - ("extension-tests", "Build the native extension"), - ("benchmark-ratchet", "Run throughput benchmarks and ratchet comparison"), - ): - script = next( - step["run"] - for step in steps("ci.yml", job_name) - if step.get("name") == step_name - ) - assert isinstance(script, str), f"ci.yml:{job_name} must run {step_name!r}" - assert f'CARGO_BUILD_JOBS="${{{VCPU_CONSTANT}}}"' in script, ( - f"ci.yml:{job_name} must bound Cargo build jobs by {VCPU_CONSTANT}" - ) - - -def test_python_suites_never_ask_for_unbounded_workers() -> None: - """Reject `-n auto`: the runner has two cores whatever the host reports.""" - sources = [source for _, source in workflow_sources()] - sources.append(MAKEFILE.read_text(encoding="utf-8")) - for source in sources: - assert "-n auto" not in source, "xdist worker counts must be explicit" - - -def test_the_python_suite_stays_serial() -> None: - """Keep pytest serial while its batches contend on one Cargo target.""" - makefile = MAKEFILE.read_text(encoding="utf-8") - assert re.search(r"^PYTEST_WORKERS \?= 0$", makefile, re.MULTILINE), ( - "PYTEST_WORKERS must default to 0; the batches compile and reuse the " - "same Rust artefacts, so xdist workers would contend on one build lock" - ) - for workflow_name, job_name in (("ci.yml", "coverage"),): - coverage_step = next( - step - for step in steps(workflow_name, job_name) - if str(step.get("uses", "")).startswith( - "leynos/shared-actions/.github/actions/generate-coverage@" - ) - ) - inputs = step_inputs(coverage_step, f"{workflow_name}:{job_name} inputs") - workers = inputs.get("pytest-workers") - assert isinstance(workers, str), ( - f"{workflow_name}:{job_name} coverage must declare pytest-workers" - ) - assert not workers, ( - f"{workflow_name}:{job_name} coverage must run pytest serially, " - f"got {workers!r}" - ) - - -def test_ci_does_not_build_tools_from_source() -> None: - """Keep CI tool installation on trusted, pinned prebuilt paths.""" - for workflow_name, source in workflow_sources(): - assert "cargo install" not in source, ( - f"{workflow_name} must not source-build a Cargo tool" - ) - assert "get.nexte.st/latest" not in source, ( - f"{workflow_name} must pin the nextest binary version" - ) - if "cargo binstall" in source: - assert "--disable-strategies compile" in source, ( - f"{workflow_name} must stop cargo-binstall falling back to a " - "source build" - ) - - -def test_no_workflow_installs_cargo_nextest() -> None: - """Leave the nextest install to the coverage action that now needs it. - - The matrix jobs stopped running the Rust suite, so an installer here would - be an unused download whose failure mode nothing in this repository - exercises. - - Checked structurally rather than by one literal string. A `tool:` input - naming either spelling, a different installer action, or a shell command - that fetches nextest would all pass a check for `tool: nextest@`. - """ - offenders: list[str] = [] - for workflow_name, _ in workflow_sources(): - document = workflow_document(workflow_name) - jobs_mapping = document.get("jobs") - if not isinstance(jobs_mapping, dict): - continue - for job_name in jobs_mapping: - offenders.extend( - _nextest_installers(workflow_name, str(job_name)), - ) - assert not offenders, ( - f"only the shared coverage action may install cargo-nextest; found {offenders}" - ) - - -def _declares_own_steps(workflow_name: str, job_name: str) -> bool: - """Report whether a job runs steps rather than calling a reusable workflow.""" - return isinstance(job(workflow_name, job_name).get("steps"), list) - - -def _installs_nextest_by_input(step: Step) -> bool: - """Report whether a step asks an installer action for cargo-nextest.""" - if str(step.get("uses", "")).startswith(SHARED_ACTION_PREFIX): - # The coverage action installs nextest deliberately; that is the one - # place it is meant to happen. - return False - inputs = step.get("with") - if not isinstance(inputs, dict): - return False - return any(NEXTEST_TOOL_NAME in str(value).lower() for value in inputs.values()) - - -def _installs_nextest_by_script(step: Step) -> bool: - """Report whether a step's script fetches cargo-nextest rather than runs it.""" - script = str(step.get("run", "")).lower() - named = NEXTEST_TOOL_NAME in script and any( - verb in script for verb in INSTALL_VERBS - ) - # The install host needs its own token: its name does not contain - # "nextest", and it only ever appears in an install command. - return named or NEXTEST_INSTALL_HOST in script - - -def _nextest_installers(workflow_name: str, job_name: str) -> list[str]: - """Return descriptions of steps in one job that would install nextest.""" - if not _declares_own_steps(workflow_name, job_name): - return [] - return [ - f"{workflow_name}:{job_name}:{step.get('name') or step.get('uses')}" - for step in steps(workflow_name, job_name) - if _installs_nextest_by_input(step) or _installs_nextest_by_script(step) - ] - - -def test_markdown_lint_runs_through_the_pinned_action() -> None: - """Keep the Markdown linter on a reproducible, SHA-pinned release. - - The estate's markdown-formatting-baseline rule requires CI to lint - Markdown through the upstream markdownlint-cli2 action rather than a - shell install, so the pin lives on the action reference. - """ - lint_steps = [ - step - for step in steps("ci.yml", "lint-test") - if str(step.get("uses", "")).startswith("DavidAnson/markdownlint-cli2-action@") - ] - assert len(lint_steps) == 1, ( - "ci.yml:lint-test must lint Markdown with the action once" - ) - reference = str(lint_steps[0]["uses"]).split("@", 1)[1] - assert re.fullmatch(r"[0-9a-f]{40}", reference), ( - "ci.yml:lint-test must pin the markdownlint-cli2 action to a full SHA" - ) - inputs = lint_steps[0].get("with") - assert isinstance(inputs, dict), "ci.yml:lint-test action step must carry inputs" - assert str(inputs.get("globs", "")).splitlines() == [ - "**/*.md", - "**/*.markdown", - "**/*.mdx", - ], "ci.yml:lint-test must lint every supported Markdown extension" - script = next( - step["run"] - for step in steps("ci.yml", "lint-test") - if step.get("name") == "Install CLI tools" - ) - assert isinstance(script, str), "ci.yml:Install CLI tools must run a script" - assert "markdownlint-cli2" not in script, ( - "ci.yml:Install CLI tools must not install markdownlint-cli2 from npm" - ) - - def test_ci_can_be_dispatched_for_warm_cache_measurement() -> None: """Keep a way to re-run CI on trunk without inventing a code change. diff --git a/tests/test_ci_tool_installation.py b/tests/test_ci_tool_installation.py new file mode 100644 index 000000000..f4636ba09 --- /dev/null +++ b/tests/test_ci_tool_installation.py @@ -0,0 +1,146 @@ +"""Contract tests for how Cuprum's CI installs its tools. + +Every tool a gate runs is fetched from a pinned, prebuilt, checksum-verified +path. A source build is slow, unpinned in its dependencies, and fails against a +pinned toolchain in ways no code change explains; and an installer for a tool +nothing runs is a download whose failure mode nothing exercises. +""" + +from __future__ import annotations + +import re +import typing as typ + +from tests.helpers.ci_placement import declares_steps +from tests.helpers.ci_runners import ( + steps, + workflow_document, + workflow_sources, +) + +if typ.TYPE_CHECKING: + from tests.helpers.workflow_types import Step + +#: Any spelling of the tool, so `nextest@` and `cargo-nextest@` both match. +NEXTEST_TOOL_NAME = "nextest" +#: Shell fragments that mean "fetch a binary" rather than "run one". +INSTALL_VERBS = ("install", "curl", "wget") +#: The upstream install host. Its name does not contain "nextest", so it needs +#: its own token, and it only ever appears in an install command. +NEXTEST_INSTALL_HOST = "get.nexte.st" +#: Steps invoking a shared action are exempt: the coverage action installs +#: nextest on purpose, and that is the only sanctioned place. +SHARED_ACTION_PREFIX = "leynos/shared-actions/" + + +def test_ci_does_not_build_tools_from_source() -> None: + """Keep CI tool installation on trusted, pinned prebuilt paths.""" + for workflow_name, source in workflow_sources(): + assert "cargo install" not in source, ( + f"{workflow_name} must not source-build a Cargo tool" + ) + assert "get.nexte.st/latest" not in source, ( + f"{workflow_name} must pin the nextest binary version" + ) + if "cargo binstall" in source: + assert "--disable-strategies compile" in source, ( + f"{workflow_name} must stop cargo-binstall falling back to a " + "source build" + ) + + +def test_no_workflow_installs_cargo_nextest() -> None: + """Leave the nextest install to the coverage action that now needs it. + + The matrix jobs stopped running the Rust suite, so an installer here would + be an unused download whose failure mode nothing in this repository + exercises. + + Checked structurally rather than by one literal string. A `tool:` input + naming either spelling, a different installer action, or a shell command + that fetches nextest would all pass a check for `tool: nextest@`. + """ + offenders: list[str] = [] + for workflow_name, _ in workflow_sources(): + document = workflow_document(workflow_name) + jobs_mapping = document.get("jobs") + if not isinstance(jobs_mapping, dict): + continue + for job_name in jobs_mapping: + offenders.extend( + _nextest_installers(workflow_name, str(job_name)), + ) + assert not offenders, ( + f"only the shared coverage action may install cargo-nextest; found {offenders}" + ) + + +def _installs_nextest_by_input(step: Step) -> bool: + """Report whether a step asks an installer action for cargo-nextest.""" + if str(step.get("uses", "")).startswith(SHARED_ACTION_PREFIX): + # The coverage action installs nextest deliberately; that is the one + # place it is meant to happen. + return False + inputs = step.get("with") + if not isinstance(inputs, dict): + return False + return any(NEXTEST_TOOL_NAME in str(value).lower() for value in inputs.values()) + + +def _installs_nextest_by_script(step: Step) -> bool: + """Report whether a step's script fetches cargo-nextest rather than runs it.""" + script = str(step.get("run", "")).lower() + named = NEXTEST_TOOL_NAME in script and any( + verb in script for verb in INSTALL_VERBS + ) + # The install host needs its own token: its name does not contain + # "nextest", and it only ever appears in an install command. + return named or NEXTEST_INSTALL_HOST in script + + +def _nextest_installers(workflow_name: str, job_name: str) -> list[str]: + """Return descriptions of steps in one job that would install nextest.""" + if not declares_steps(workflow_name, job_name): + return [] + return [ + f"{workflow_name}:{job_name}:{step.get('name') or step.get('uses')}" + for step in steps(workflow_name, job_name) + if _installs_nextest_by_input(step) or _installs_nextest_by_script(step) + ] + + +def test_markdown_lint_runs_through_the_pinned_action() -> None: + """Keep the Markdown linter on a reproducible, SHA-pinned release. + + The estate's markdown-formatting-baseline rule requires CI to lint + Markdown through the upstream markdownlint-cli2 action rather than a + shell install, so the pin lives on the action reference. + """ + lint_steps = [ + step + for step in steps("ci.yml", "lint-test") + if str(step.get("uses", "")).startswith("DavidAnson/markdownlint-cli2-action@") + ] + assert len(lint_steps) == 1, ( + "ci.yml:lint-test must lint Markdown with the action once" + ) + reference = str(lint_steps[0]["uses"]).split("@", 1)[1] + assert re.fullmatch(r"[0-9a-f]{40}", reference), ( + "ci.yml:lint-test must pin the markdownlint-cli2 action to a full SHA" + ) + inputs = lint_steps[0].get("with") + assert isinstance(inputs, dict), "ci.yml:lint-test action step must carry inputs" + assert str(inputs.get("globs", "")).splitlines() == [ + "**/*.md", + "**/*.markdown", + "**/*.mdx", + ], "ci.yml:lint-test must lint every supported Markdown extension" + script = next( + step["run"] + for step in steps("ci.yml", "lint-test") + if step.get("name") == "Install CLI tools" + ) + assert isinstance(script, str), "ci.yml:Install CLI tools must run a script" + assert "markdownlint-cli2" not in script, ( + "ci.yml:Install CLI tools must not install markdownlint-cli2 from npm" + ) diff --git a/tests/test_ci_worker_bounds.py b/tests/test_ci_worker_bounds.py new file mode 100644 index 000000000..1517ad863 --- /dev/null +++ b/tests/test_ci_worker_bounds.py @@ -0,0 +1,102 @@ +"""Contract tests for how wide Cuprum's CI jobs fan out. + +A job that asks `pytest-xdist` or Cargo for more workers than the runner has +cores still produces a green suite while thrashing two vCPU, so the worker +counts are derived from one constant that names the shape the job is billed +for. Nothing in a passing run says whether that constant and the label agree. +""" + +from __future__ import annotations + +import re + +from tests.helpers.ci_runners import ( + ROOT, + UBICLOUD_LABEL, + UBICLOUD_VCPUS, + step_inputs, + steps, + workflow_env, + workflow_sources, +) + +MAKEFILE = ROOT / "Makefile" +VCPU_CONSTANT = "LINUX_RUNNER_VCPUS" +#: Make variables that carry the runner's vCPU count into the test command. +#: Only the pytest one remains here: the Rust suite moved to the coverage job, +#: which bounds itself through `CARGO_BUILD_JOBS` and `NEXTEST_TEST_THREADS`. +PARALLELISM_OVERRIDES = ("PYTEST_CARGO_BUILD_JOBS",) + + +def test_the_vcpu_constant_matches_the_assigned_label() -> None: + """Tie the one parallelism constant to the shape the job is billed for.""" + declared = workflow_env("ci.yml")[VCPU_CONSTANT] + assert declared == str(UBICLOUD_VCPUS), ( + f"{VCPU_CONSTANT} must equal the vCPU count of {UBICLOUD_LABEL}, " + f"got {declared!r}" + ) + + +def test_python_tests_derive_their_worker_counts_from_that_constant() -> None: + """Size the matrix suite's Cargo work from the constant, not a literal.""" + script = next( + step["run"] + for step in steps("ci.yml", "typecheck-test") + if step.get("name") == "Run tests" + ) + assert isinstance(script, str), "ci.yml:typecheck-test must run a test script" + for variable in PARALLELISM_OVERRIDES: + assert f'{variable}="${{{VCPU_CONSTANT}}}"' in script, ( + f"ci.yml:typecheck-test must pass {variable} from {VCPU_CONSTANT}" + ) + + +def test_extension_and_benchmark_builds_are_bounded_too() -> None: + """Bound the two jobs that compile outside `make test` to the same count.""" + for job_name, step_name in ( + ("extension-tests", "Build the native extension"), + ("benchmark-ratchet", "Run throughput benchmarks and ratchet comparison"), + ): + script = next( + step["run"] + for step in steps("ci.yml", job_name) + if step.get("name") == step_name + ) + assert isinstance(script, str), f"ci.yml:{job_name} must run {step_name!r}" + assert f'CARGO_BUILD_JOBS="${{{VCPU_CONSTANT}}}"' in script, ( + f"ci.yml:{job_name} must bound Cargo build jobs by {VCPU_CONSTANT}" + ) + + +def test_python_suites_never_ask_for_unbounded_workers() -> None: + """Reject `-n auto`: the runner has two cores whatever the host reports.""" + sources = [source for _, source in workflow_sources()] + sources.append(MAKEFILE.read_text(encoding="utf-8")) + for source in sources: + assert "-n auto" not in source, "xdist worker counts must be explicit" + + +def test_the_python_suite_stays_serial() -> None: + """Keep pytest serial while its batches contend on one Cargo target.""" + makefile = MAKEFILE.read_text(encoding="utf-8") + assert re.search(r"^PYTEST_WORKERS \?= 0$", makefile, re.MULTILINE), ( + "PYTEST_WORKERS must default to 0; the batches compile and reuse the " + "same Rust artefacts, so xdist workers would contend on one build lock" + ) + for workflow_name, job_name in (("ci.yml", "coverage"),): + coverage_step = next( + step + for step in steps(workflow_name, job_name) + if str(step.get("uses", "")).startswith( + "leynos/shared-actions/.github/actions/generate-coverage@" + ) + ) + inputs = step_inputs(coverage_step, f"{workflow_name}:{job_name} inputs") + workers = inputs.get("pytest-workers") + assert isinstance(workers, str), ( + f"{workflow_name}:{job_name} coverage must declare pytest-workers" + ) + assert not workers, ( + f"{workflow_name}:{job_name} coverage must run pytest serially, " + f"got {workers!r}" + ) From 4fbdabda1b34a5b1ffca0ad08cefe7924d9374c8 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 18 Sep 2026 17:13:07 +0100 Subject: [PATCH 2/7] Refuse an interpolated runner label Self-audit of the reader found two holes on the literal path. `ubuntu-${{ matrix.release }}` was recorded as a literal label. That is the defect this reader exists to prevent wearing a different shape: the label it resolves to at run time is invisible to every placement and registry assertion while the job still asks for a runner. A value that interpolates without being wholly an expression is now refused. A folded scalar can also leave incidental whitespace around the expression, and the anchored pattern sent such a declaration down the literal path, so a correctly written fork lane quietly stopped being read as one. That is a false negative, which is the harder kind to notice, and the pattern now tolerates surrounding whitespace. Both proved by mutation: removing the interpolation guard fails the refusal test, and re-anchoring the pattern fails the whitespace test. --- tests/helpers/ci_placement.py | 22 ++++++++++++++++++++- tests/test_ci_placement_reader.py | 32 +++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/helpers/ci_placement.py b/tests/helpers/ci_placement.py index ea8550520..fbeade1f2 100644 --- a/tests/helpers/ci_placement.py +++ b/tests/helpers/ci_placement.py @@ -61,7 +61,14 @@ "windows-latest", ) -_EXPRESSION = re.compile(r"^\$\{\{(?P.*)\}\}$", re.DOTALL) +# Surrounding whitespace is incidental: a folded scalar can leave a trailing +# space, and a lane written that way is correct. Without the `\s*` the whole +# declaration falls through to the literal branch, which is how a correctly +# written fork lane would quietly stop being read as one. +_EXPRESSION = re.compile(r"^\s*\$\{\{(?P.*)\}\}\s*$", re.DOTALL) +#: An expression *fragment*, used to refuse a value that interpolates without +#: being wholly an expression. +_INTERPOLATION = re.compile(r"\$\{\{") #: The canonical fork fallback, anchored end to end so the arms are read by #: position rather than by membership. `\A`/`\Z` rather than a bare `fullmatch` #: on a pattern that could otherwise be satisfied by a prefix. @@ -262,6 +269,19 @@ def placement(workflow_name: str, job_name: str) -> Placement: ) match = _EXPRESSION.match(text) if match is None: + # A value that interpolates without being wholly an expression, such as + # `ubuntu-${{ matrix.release }}`, is not a literal label. Recording it + # as one is the same defect this reader exists to prevent, wearing a + # different shape: the label it resolves to at run time is invisible to + # every placement and registry assertion. + _require( + condition=_INTERPOLATION.search(text) is None, + message=( + f"{where} interpolates its runner label: {text!r}. The labels " + "this can resolve to are unreadable, so it is refused rather " + "than recorded as one literal." + ), + ) return Placement("literal", text, None, frozenset({text}), frozenset()) body = match.group("body") fork = _FORK_EXPRESSION.match(body) diff --git a/tests/test_ci_placement_reader.py b/tests/test_ci_placement_reader.py index b1e32059e..6389587aa 100644 --- a/tests/test_ci_placement_reader.py +++ b/tests/test_ci_placement_reader.py @@ -200,6 +200,38 @@ def test_unmodelled_declarations_are_refused( reader.placement("w.yml", "j") +def test_an_interpolated_label_is_refused(monkeypatch: pytest.MonkeyPatch) -> None: + """Refuse a value that interpolates without being wholly an expression. + + `ubuntu-${{ matrix.release }}` is not a literal label, and recording it as + one is the same defect as recording an opaque expression as one: the label + it resolves to at run time is invisible to every placement and registry + assertion, while the job still asks for a runner. + """ + _declare(monkeypatch, {"runs-on": "ubuntu-${{ matrix.release }}", "steps": []}) + with pytest.raises(AssertionError, match="interpolates its runner label"): + reader.placement("w.yml", "j") + + +def test_incidental_whitespace_around_an_expression_is_read( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Keep a correctly written lane readable when a scalar leaves a space. + + A folded scalar can leave a trailing space. Without tolerating it the whole + declaration falls through to the literal branch, and a correct fork lane + quietly stops being read as one, which is a false negative rather than a + false positive and so is the harder kind to notice. + """ + _declare(monkeypatch, {"runs-on": f" {FORK_EXPRESSION} ", "steps": []}) + placed = reader.placement("w.yml", "j") + assert placed.kind == "fork", ( + f"incidental whitespace must not change the shape, got {placed.kind}" + ) + assert placed.owned == OWNED, f"owned arm was {placed.owned!r}" + assert placed.fork == HOSTED, f"fork arm was {placed.fork!r}" + + def test_the_refusal_of_an_opaque_expression_is_still_needed( monkeypatch: pytest.MonkeyPatch, ) -> None: From 4d6ce1b5015d7399f186eee1d55973f1e6cc288a Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 18 Sep 2026 18:12:46 +0100 Subject: [PATCH 3/7] Answer the placement review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven review threads and the pre-merge table's testing error. **Reader correctness.** `references()` read whole expression bodies with a pattern that stops at the first `}`, which sits inside `format('{0}', …)`, so a composite name reported no reference at all and the job-name stability rule compared nothing. It now reads dotted context properties across every `${{ }}` span, with quoted literals stripped first so a label such as `'ubuntu-22.04'` is not mistaken for a path. The falsy set encoded the quoted `'false'` as false. GitHub treats a non-empty string as truthy, so that is a job that runs; the empty string, `null` and numeric zero were missing instead. The leading-clause check now covers every falsy literal rather than only `false`. **A correction to this branch's own last commit.** That commit broadened the expression pattern to tolerate surrounding whitespace, which made a padded fork expression read as a correct fork placement. GitHub interpolates into the surrounding string and keeps what is around it, so `" ${{ … }}"` resolves to a label no runner matches: the broadening was a false positive in the dangerous direction. The pattern is anchored again and padded values fall to the interpolation refusal. **Contract narrowing.** Timeouts require a positive integer rather than an `isinstance` check that YAML's `true` satisfies. The nextest exemption names the coverage action exactly instead of the whole shared-actions namespace. The registry gains a second, independently maintained list of the hosted labels in use, so a misspelling added to both a matrix leg and the frozen set fails rather than leaving a job queued forever. **The table's testing error.** Adds a behavioural feature that evaluates the composed workflows against an event, follows the call from `ci.yml` into `build-wheels.yml`, and asserts the runner each event selects. `act` cannot serve here: it skips a job whose label it cannot map and exits zero, so the Ubicloud lanes would go unexercised while the run reported success. **The table's property warning.** Adds Hypothesis properties over the reader's input space. They immediately found a defect no example had: a condition carrying a hyphen, such as `matrix.python-version`, was refused as unmodellable. Also adds the Oxford commas, and splits the reader tests so every module stays under the 400-line limit. --- docs/developers-guide.md | 23 +- tests/behaviour/test_ci_runner_selection.py | 244 +++++++++++++++++ tests/features/ci_runner_selection.feature | 43 +++ tests/helpers/ci_events.py | 113 ++++++++ tests/helpers/ci_placement.py | 68 ++++- tests/test_ci_actionlint_registry.py | 18 ++ tests/test_ci_placement_guards.py | 202 ++++++++++++++ tests/test_ci_placement_properties.py | 287 ++++++++++++++++++++ tests/test_ci_placement_reader.py | 177 +++--------- tests/test_ci_runner_placement.py | 13 +- tests/test_ci_tool_installation.py | 12 +- 11 files changed, 1038 insertions(+), 162 deletions(-) create mode 100644 tests/behaviour/test_ci_runner_selection.py create mode 100644 tests/features/ci_runner_selection.feature create mode 100644 tests/helpers/ci_events.py create mode 100644 tests/test_ci_placement_guards.py create mode 100644 tests/test_ci_placement_properties.py diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 8c5a24423..4b5c609b3 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -177,7 +177,7 @@ would miss a stale registration, which is the other half of the equality. `tests/helpers/ci_placement.py` is the one reader of `runs-on` in the test suite. Its scope is deliberately narrow: it models the three shapes this -repository declares, a literal label, the fork fallback and a matrix key +repository declares, a literal label, the fork fallback, and a matrix key resolved through its `include` entries, and refuses everything else by name. Every contract that asks where a job runs, whether it runs, or what label it resolves to goes through it, including the cache-family resolver in @@ -186,11 +186,28 @@ rather than two that drift. Refusal is the design, not a gap. An expression recorded as one opaque label carries no vendor prefix, so an Ubicloud classifier drops the lane and it sits -exempt from every placement, ceiling and registry rule while still asking for a -paid runner. `tests/test_ci_placement_reader.py` drives the reader with +exempt from every placement, ceiling, and registry rule while still asking for +a paid runner. `tests/test_ci_placement_reader.py` drives the reader with synthetic declarations, because a rule parametrized over this repository's own correct workflows passes whether or not it discriminates. +Three layers sit above it, each answering a question the layer below cannot: + +- `tests/test_ci_placement_guards.py` covers whether a job runs and which + context properties a declaration reads. +- `tests/test_ci_placement_properties.py` generates the input space rather than + enumerating it. Every reader defect found in review was an input nobody had + written down, and the properties have since found one more of their own: a + condition carrying a hyphen, such as `matrix.python-version`, was refused as + unmodellable. +- `tests/behaviour/test_ci_runner_selection.py` evaluates the composed + workflows against an event, following the call from `ci.yml` into + `build-wheels.yml`, and asserts the runner each event actually selects. That + is where the property that matters lives: a fork's pull request must never + select a runner a fork cannot obtain. `act` cannot stand in for it, because + it skips a job whose label it cannot map and exits zero, so the Ubicloud + lanes would go unexercised while the run reported success. + ### Cache ownership Caching goes through `actions/cache` pinned to diff --git a/tests/behaviour/test_ci_runner_selection.py b/tests/behaviour/test_ci_runner_selection.py new file mode 100644 index 000000000..48b628033 --- /dev/null +++ b/tests/behaviour/test_ci_runner_selection.py @@ -0,0 +1,244 @@ +"""Behavioural tests for the runner each CI event selects. + +The contract tests in `tests/test_ci_runner_placement.py` read one job's +declaration at a time. These scenarios evaluate the composed workflows against +an event: they resolve every expression, follow the call from `ci.yml` into +`build-wheels.yml`, and assert the runner the event actually selects. + +That composition is where the interesting failures live. A lane can declare two +correct labels and still send a fork to a runner no fork can obtain, and a +called workflow's placement is invisible to any rule that reads its own +`workflow_call` trigger. `act` cannot stand in here: it skips a job whose label +it cannot map and exits zero, so the Ubicloud lanes would go unexercised while +the run reported success. +""" + +from __future__ import annotations + +import typing as typ + +import pytest +from pytest_bdd import given, parsers, scenario, then, when + +from tests.helpers.ci_events import ( + FORK_PULL_REQUEST, + OWNED_PULL_REQUEST, + PUSH_TO_MAIN, + TAG_PUSH, + schedule, +) +from tests.helpers.ci_runners import ( + FORK_REACHABLE_UBICLOUD_JOBS, + GITHUB_LABEL, + UBICLOUD_JOBS, + UBICLOUD_LABEL, + expand, +) + +if typ.TYPE_CHECKING: + from tests.helpers.ci_events import Scheduled + +FEATURE = "../features/ci_runner_selection.feature" +#: The wheel jobs that only a call can reach. `build-wheels.yml` declares +#: `workflow_call` alone, so its own trigger says nothing about who runs it. +CALLED_WHEEL_JOBS: typ.Final = ("build-pure-wheel", "verify-wheel-install") +PLATFORM_LABELS: typ.Final = frozenset({ + "macos-15-intel", + "macos-latest", + "ubuntu-latest", + "windows-2022", +}) +EVENTS: typ.Final = { + "fork": FORK_PULL_REQUEST, + "owned": OWNED_PULL_REQUEST, + "push": PUSH_TO_MAIN, +} + + +@pytest.fixture +def selection() -> dict[str, object]: + """Carry the workflow under test and the schedule between steps.""" + return {} + + +@given("the continuous integration workflow") +def _ci_workflow(selection: dict[str, object]) -> None: + """Select the workflow every pull request and push triggers.""" + selection["workflow"] = "ci.yml" + + +@given("the release workflow") +def _release_workflow(selection: dict[str, object]) -> None: + """Select the workflow a tag push triggers.""" + selection["workflow"] = "release.yml" + + +@when("a pull request is opened from a branch of this repository") +def _owned_pull_request(selection: dict[str, object]) -> None: + """Resolve the schedule for a pull request from this repository.""" + selection["scheduled"] = schedule( + typ.cast("str", selection["workflow"]), OWNED_PULL_REQUEST + ) + + +@when("a pull request is opened from a fork") +def _fork_pull_request(selection: dict[str, object]) -> None: + """Resolve the schedule for a pull request from a fork.""" + selection["scheduled"] = schedule( + typ.cast("str", selection["workflow"]), FORK_PULL_REQUEST + ) + + +@when("a commit is pushed to the default branch") +def _push_to_main(selection: dict[str, object]) -> None: + """Resolve the schedule for a push to the default branch.""" + selection["scheduled"] = schedule( + typ.cast("str", selection["workflow"]), PUSH_TO_MAIN + ) + + +@when("a tag is pushed") +def _tag_push(selection: dict[str, object]) -> None: + """Resolve the schedule for a tag push.""" + selection["scheduled"] = schedule(typ.cast("str", selection["workflow"]), TAG_PUSH) + + +@when(parsers.parse("the {event} event is evaluated")) +def _named_event(selection: dict[str, object], event: str) -> None: + """Resolve the schedule for the event the outline names.""" + selection["scheduled"] = schedule( + typ.cast("str", selection["workflow"]), EVENTS[event] + ) + + +def _scheduled(selection: dict[str, object]) -> list[Scheduled]: + """Return the resolved schedule, refusing an empty one. + + An empty schedule would satisfy every assertion below by vacuity, which is + indistinguishable from a passing run until something is mutated. + + Returns + ------- + list[Scheduled] + Every job the event scheduled, guaranteed non-empty. + """ + resolved = typ.cast("list[Scheduled]", selection["scheduled"]) + assert resolved, "the event scheduled no jobs, so every assertion is vacuous" + return resolved + + +@then("every reviewed lane selects the Ubicloud runner") +def _reviewed_lanes_are_paid(selection: dict[str, object]) -> None: + """Assert each reviewed lane took the Ubicloud arm.""" + resolved = { + (item.workflow, item.job): item.labels for item in _scheduled(selection) + } + for workflow_name, job_name in expand(UBICLOUD_JOBS): + if (workflow_name, job_name) not in resolved: + continue + assert resolved[workflow_name, job_name] == (UBICLOUD_LABEL,), ( + f"{workflow_name}:{job_name} must select {UBICLOUD_LABEL} on this " + f"event, got {resolved[workflow_name, job_name]}" + ) + + +@then("the wheel jobs reached through the called workflow select it too") +def _called_wheel_jobs_are_paid(selection: dict[str, object]) -> None: + """Assert the called workflow's wheel jobs were reached and placed.""" + resolved = {item.job: item.labels for item in _scheduled(selection)} + for job_name in CALLED_WHEEL_JOBS: + assert job_name in resolved, ( + f"{job_name} must be reached through the call into " + "build-wheels.yml; a caller that stopped calling it would leave " + "every rule about its placement true and unexercised" + ) + assert resolved[job_name] == (UBICLOUD_LABEL,), ( + f"{job_name} must select {UBICLOUD_LABEL}, got {resolved[job_name]}" + ) + + +@then("no lane selects a paid runner") +def _no_paid_runner_for_a_fork(selection: dict[str, object]) -> None: + """Assert no lane asked for a runner a fork cannot obtain.""" + offenders = [ + f"{item.workflow}:{item.job}={label}" + for item in _scheduled(selection) + for label in item.labels + if label.startswith("ubicloud-") + ] + assert not offenders, ( + "a fork's pull request cannot obtain an Ubicloud runner, so a lane " + f"selecting one would queue until it timed out: {offenders}" + ) + + +@then("every fork-reachable lane selects the hosted fallback") +def _fork_lanes_fall_back(selection: dict[str, object]) -> None: + """Assert every fork-reachable lane took the hosted arm.""" + resolved = { + (item.workflow, item.job): item.labels for item in _scheduled(selection) + } + for workflow_name, job_name in expand(FORK_REACHABLE_UBICLOUD_JOBS): + assert (workflow_name, job_name) in resolved, ( + f"{workflow_name}:{job_name} is declared fork-reachable but this " + "event did not schedule it" + ) + assert resolved[workflow_name, job_name] == (GITHUB_LABEL,), ( + f"{workflow_name}:{job_name} must fall back to {GITHUB_LABEL} for " + f"a fork, got {resolved[workflow_name, job_name]}" + ) + + +@then("the native wheel matrix keeps its platform runners") +def _native_matrix_is_hosted(selection: dict[str, object]) -> None: + """Assert the native matrix kept exactly its platform runners.""" + resolved = {item.job: item.labels for item in _scheduled(selection)} + assert "build-native-wheels" in resolved, ( + "the native wheel matrix must be reached through the call" + ) + assert frozenset(resolved["build-native-wheels"]) == PLATFORM_LABELS, ( + "the native matrix must keep exactly its platform runners, got " + f"{sorted(resolved['build-native-wheels'])}" + ) + + +@then("every selected label is a concrete runner name") +def _labels_are_schedulable(selection: dict[str, object]) -> None: + """Assert every selected label is one GitHub could schedule.""" + for item in _scheduled(selection): + for label in item.labels: + assert "${{" not in label, ( + f"{item.workflow}:{item.job} selected an unevaluated " + f"expression: {label!r}" + ) + assert label, f"{item.workflow}:{item.job} selected an empty label" + assert label == label.strip(), ( + f"{item.workflow}:{item.job} selected {label!r}; GitHub keeps " + "whatever surrounds an interpolation, and a padded label " + "matches no runner" + ) + + +@scenario(FEATURE, "an owned pull request keeps every reviewed lane on the paid runner") +def test_an_owned_pull_request_keeps_the_paid_lane() -> None: + """An internal pull request uses the runners the repository pays for.""" + + +@scenario(FEATURE, "a fork's pull request selects only runners a fork can obtain") +def test_a_fork_pull_request_is_schedulable() -> None: + """A fork's pull request never waits on a runner it cannot be given.""" + + +@scenario(FEATURE, "a push to the default branch takes the owned arm") +def test_a_push_takes_the_owned_arm() -> None: + """With no pull-request payload the fork field is absent, so falsy.""" + + +@scenario(FEATURE, "a tag push reaches the wheel jobs on the paid runner") +def test_a_tag_push_reaches_the_wheel_jobs() -> None: + """The release path resolves through the same called workflow.""" + + +@scenario(FEATURE, "every selected runner is a schedulable label") +def test_every_selected_runner_is_schedulable() -> None: + """No event may select an unevaluated or padded label.""" diff --git a/tests/features/ci_runner_selection.feature b/tests/features/ci_runner_selection.feature new file mode 100644 index 000000000..aede6b827 --- /dev/null +++ b/tests/features/ci_runner_selection.feature @@ -0,0 +1,43 @@ +Feature: CI runner selection by event + The placement contracts read declarations one job at a time. These scenarios + exercise the composed workflows: they evaluate each event against the real + YAML, follow the call from ci.yml into build-wheels.yml, and assert the + runner each event actually selects. + + The property that matters is not that a lane names two labels. It is that a + fork's pull request never selects a runner a fork cannot obtain, and that an + owned event never gives away the paid lane. + + Scenario: an owned pull request keeps every reviewed lane on the paid runner + Given the continuous integration workflow + When a pull request is opened from a branch of this repository + Then every reviewed lane selects the Ubicloud runner + And the wheel jobs reached through the called workflow select it too + + Scenario: a fork's pull request selects only runners a fork can obtain + Given the continuous integration workflow + When a pull request is opened from a fork + Then no lane selects a paid runner + And every fork-reachable lane selects the hosted fallback + + Scenario: a push to the default branch takes the owned arm + Given the continuous integration workflow + When a commit is pushed to the default branch + Then every reviewed lane selects the Ubicloud runner + + Scenario: a tag push reaches the wheel jobs on the paid runner + Given the release workflow + When a tag is pushed + Then the wheel jobs reached through the called workflow select it too + And the native wheel matrix keeps its platform runners + + Scenario Outline: every selected runner is a schedulable label + Given the continuous integration workflow + When the event is evaluated + Then every selected label is a concrete runner name + + Examples: + | event | + | fork | + | owned | + | push | diff --git a/tests/helpers/ci_events.py b/tests/helpers/ci_events.py new file mode 100644 index 000000000..ca7bfd113 --- /dev/null +++ b/tests/helpers/ci_events.py @@ -0,0 +1,113 @@ +"""Resolve the runner each workflow event actually selects. + +The placement contracts read declarations. This module evaluates them: given an +event, it resolves every job's `runs-on` to the concrete label GitHub would +schedule on, following a reusable-workflow call into its callee, so the +`ci.yml` to `build-wheels.yml` path is exercised rather than asserted a job at +a time. + +`act` is not the tool for this. It skips a job whose label it cannot map and +exits zero, so an Ubicloud lane would be silently unexercised and the run would +report success: a harness that cannot fail on the thing it exists to check. +Evaluating the expression against an event payload is the part that can be +modelled honestly without a runner, and it is the part that decides whether a +fork's pull request can be scheduled at all. +""" + +from __future__ import annotations + +import typing as typ + +from tests.helpers.ci_placement import declares_steps, placement +from tests.helpers.ci_workflows import job, jobs + +if typ.TYPE_CHECKING: + import collections.abc as cabc + +#: A pull request opened from a fork of this repository. +FORK_PULL_REQUEST: typ.Final = {"event_name": "pull_request", "fork": True} +#: A pull request opened from a branch of this repository. +OWNED_PULL_REQUEST: typ.Final = {"event_name": "pull_request", "fork": False} +#: A push to the default branch. There is no pull-request payload, so the fork +#: field is absent, which GitHub evaluates as falsy. +PUSH_TO_MAIN: typ.Final = {"event_name": "push", "fork": None} +#: A tag push, which is what reaches `release.yml`. +TAG_PUSH: typ.Final = {"event_name": "push", "fork": None} + + +class Scheduled(typ.NamedTuple): + """One job as an event would schedule it. + + Attributes + ---------- + workflow: + The workflow the job is declared in, which is the callee for a job + reached through a reusable-workflow call. + job: + The job key within that workflow. + labels: + The label or labels the event selects. A matrix job selects one per + leg; every other job selects exactly one. + """ + + workflow: str + job: str + labels: tuple[str, ...] + + +def _select( + workflow_name: str, job_name: str, event: cabc.Mapping[str, object] +) -> tuple[str, ...]: + """Return the labels one event selects for one job.""" + placed = placement(workflow_name, job_name) + if placed.kind == "matrix": + return tuple(sorted(placed.labels)) + if placed.kind == "literal": + return (typ.cast("str", placed.owned),) + # The fork fallback. GitHub evaluates the condition against the event + # payload: absent on a push, false on an owned pull request, true on a + # fork's. Only a truthy value takes the hosted arm. + takes_fork_arm = bool(event.get("fork")) + return (typ.cast("str", placed.fork if takes_fork_arm else placed.owned),) + + +def schedule(workflow_name: str, event: cabc.Mapping[str, object]) -> list[Scheduled]: + """Resolve every job one event schedules, following reusable-workflow calls. + + Parameters + ---------- + workflow_name: + The workflow the event triggers. + event: + One of the event constants in this module. + + Returns + ------- + list[Scheduled] + Every job the event would schedule, with a called workflow's jobs + reported under the callee's name rather than the caller's, because that + is where their runners are declared. + + Notes + ----- + A called workflow inherits its caller's `github` context, so the same event + is passed through the call unchanged. That inheritance is the reason the + fork expression works inside `build-wheels.yml` without an extra input, and + modelling the call is what makes that claim testable. + """ + scheduled: list[Scheduled] = [] + for job_name in jobs(workflow_name): + if declares_steps(workflow_name, job_name): + scheduled.append( + Scheduled( + workflow_name, job_name, _select(workflow_name, job_name, event) + ) + ) + continue + called = job(workflow_name, job_name).get("uses") + if not isinstance(called, str) or not called.startswith("./.github/workflows/"): + # A caller of a workflow outside this repository declares no runner + # this harness can resolve, and its callee is not ours to model. + continue + scheduled.extend(schedule(called.rsplit("/", 1)[-1], event)) + return scheduled diff --git a/tests/helpers/ci_placement.py b/tests/helpers/ci_placement.py index fbeade1f2..5e5450bf5 100644 --- a/tests/helpers/ci_placement.py +++ b/tests/helpers/ci_placement.py @@ -6,7 +6,7 @@ narrows that value with ``str()`` sees one opaque label that carries no vendor prefix. Axinite #372 recorded what that costs: the Ubicloud classifier said no, the job was dropped before any assertion ran, and the lane sat exempt from the -placement, ceiling and registry contracts while still asking for a paid runner. +placement, ceiling, and registry contracts while still asking for a paid runner. This module therefore fails towards refusal. It models the three shapes the repository actually declares and rejects everything else by name, so an @@ -61,28 +61,46 @@ "windows-latest", ) -# Surrounding whitespace is incidental: a folded scalar can leave a trailing -# space, and a lane written that way is correct. Without the `\s*` the whole -# declaration falls through to the literal branch, which is how a correctly -# written fork lane would quietly stop being read as one. -_EXPRESSION = re.compile(r"^\s*\$\{\{(?P.*)\}\}\s*$", re.DOTALL) +# Anchored with no tolerance for surrounding characters. GitHub interpolates +# into the surrounding string and keeps what is around the expression, so +# `runs-on: " ${{ ... }}"` resolves to a label with a leading space that no +# runner matches. Tolerating the padding would report such a lane as a correct +# fork placement, a false positive in the dangerous direction; anything padded +# falls to the interpolation refusal instead. +_EXPRESSION = re.compile(r"^\$\{\{(?P.*)\}\}$", re.DOTALL) #: An expression *fragment*, used to refuse a value that interpolates without -#: being wholly an expression. +#: being wholly an expression, whether padded or concatenated. _INTERPOLATION = re.compile(r"\$\{\{") #: The canonical fork fallback, anchored end to end so the arms are read by #: position rather than by membership. `\A`/`\Z` rather than a bare `fullmatch` #: on a pattern that could otherwise be satisfied by a prefix. _FORK_EXPRESSION = re.compile( - r"\A\s*(?P[A-Za-z_][\w.]*)" + # Hyphens belong in the character class: `matrix.python-version` is a + # legitimate condition, and excluding `-` refused it as unmodellable. Found + # by the generated property, not by any example anyone wrote down. + r"\A\s*(?P[A-Za-z_][\w.-]*)" r"\s*&&\s*'(?P[^']+)'" r"\s*\|\|\s*'(?P[^']+)'\s*\Z" ) _MATRIX_EXPRESSION = re.compile(r"\A\s*matrix\.(?P[\w-]+)\s*\Z") #: Every ``${{ ... }}`` reference in a string, for the job-name stability rule. -_REFERENCE = re.compile(r"\$\{\{(?P[^}]*)\}\}") +#: One `${{ ... }}` span. Non-greedy to the first `}}`, because `[^}]*` stops +#: at the `}` inside `format('{0}', matrix.os)` and reports no reference at all. +_EXPRESSION_SPAN = re.compile(r"\$\{\{(?P.*?)\}\}", re.DOTALL) +#: A quoted literal inside an expression body. Stripped before references are +#: read, so a label such as `'ubuntu-22.04'` is not mistaken for a property +#: path on the strength of its dot. +_QUOTED = re.compile(r"'[^']*'|\"[^\"]*\"") +#: A dotted context property such as `matrix.os` or +#: `github.event.pull_request.head.repo.fork`. A bare function name such as +#: `format` carries no dot and is correctly not a reference. +_PROPERTY = re.compile(r"[A-Za-z_][\w-]*(?:\.[\w-]+)+") #: Expressions a job-level ``if:`` may not reduce to. A job that cannot run #: satisfies every declaration-reading rule while gating nothing. -_NEVER_RUNS: typ.Final = frozenset({"false", "0", "'false'", '"false"'}) +#: GitHub Actions evaluates `false`, numeric zero, the empty string and `null` +#: as falsy, and **a non-empty string as truthy**, so the quoted `'false'` +#: belongs on the other side of this line rather than in it. +_NEVER_RUNS: typ.Final = frozenset({"false", "0", "0.0", "''", '""', "null"}) def _require(*, condition: bool, message: str) -> None: @@ -314,10 +332,26 @@ def placement(workflow_name: str, job_name: str) -> Placement: def references(value: object) -> frozenset[str]: - """Return every ``${{ ... }}`` reference a declaration reads.""" + """Return every context property a declaration reads. + + Property paths rather than whole expression bodies. A name built with + ``${{ format('{0}', matrix.os) }}`` reads ``matrix.os`` and has to be + comparable with a ``runs-on`` reading the same key; returning the body made + that comparison miss entirely, so an unstable name passed the stability + rule. + + Returns + ------- + frozenset[str] + Every dotted context property the value reads, across every + ``${{ ... }}`` span, with quoted literals excluded. + """ if not isinstance(value, str): return frozenset() - return frozenset(body.strip() for body in _REFERENCE.findall(value) if body.strip()) + found: set[str] = set() + for body in _EXPRESSION_SPAN.findall(value): + found.update(_PROPERTY.findall(_QUOTED.sub(" ", body))) + return frozenset(found) def never_runs(workflow_name: str, job_name: str) -> bool: @@ -339,6 +373,10 @@ def never_runs(workflow_name: str, job_name: str) -> bool: expression = _EXPRESSION.match(text) if expression is not None: text = " ".join(expression.group("body").split()) - # A containment test would accept `false && matrix.target == 'x'`; the - # leading clause is matched explicitly instead. - return text.lower() in _NEVER_RUNS or text.lower().startswith("false &&") + lowered = text.lower() + if lowered in _NEVER_RUNS: + return True + # A containment test would accept `false && matrix.target == 'x'`, so the + # leading clause is matched explicitly, and for every falsy literal rather + # than only `false`: `null && ...` and `0 && ...` are equally unreachable. + return any(lowered.startswith(f"{literal} &&") for literal in _NEVER_RUNS) diff --git a/tests/test_ci_actionlint_registry.py b/tests/test_ci_actionlint_registry.py index 062a449a9..96abea936 100644 --- a/tests/test_ci_actionlint_registry.py +++ b/tests/test_ci_actionlint_registry.py @@ -25,6 +25,17 @@ #: allowance that would let a deleted variable's typo pass. CONFIGURATION_VARIABLE = re.compile(r"vars\.([A-Za-z_][A-Za-z0-9_]*)") ALL_CASES = all_jobs() +#: The GitHub-hosted labels this repository actually runs on, maintained +#: independently of `FROZEN_HOSTED_LABELS`. The frozen set is an allowance +#: list, so a misspelling added to both it and a matrix leg would satisfy every +#: assertion that reads one against the other; this second list has to be +#: edited too, and it is an equality rather than a membership test. +HOSTED_LABELS_IN_USE = frozenset({ + "macos-15-intel", + "macos-latest", + "ubuntu-latest", + "windows-2022", +}) def _labels_in_use() -> frozenset[str]: @@ -100,6 +111,13 @@ def test_every_hosted_label_in_use_is_named_in_the_frozen_set() -> None: failure the named set exists to prevent. """ hosted = _labels_in_use() & frozenset(FROZEN_HOSTED_LABELS) + assert hosted == HOSTED_LABELS_IN_USE, ( + "the GitHub-hosted labels in use must match the independently " + f"maintained list; in use {sorted(hosted)}, expected " + f"{sorted(HOSTED_LABELS_IN_USE)}. A misspelt label added to both a " + "matrix leg and the frozen set fails here, where it would otherwise " + "leave a job queued for a runner that does not exist" + ) config = yaml.safe_load(ACTIONLINT_CONFIG.read_text(encoding="utf-8")) registered = frozenset(config["self-hosted-runner"]["labels"]) assert not (hosted & registered), ( diff --git a/tests/test_ci_placement_guards.py b/tests/test_ci_placement_guards.py new file mode 100644 index 000000000..a33adb4f9 --- /dev/null +++ b/tests/test_ci_placement_guards.py @@ -0,0 +1,202 @@ +"""Tests for the reader's guards: whether a job runs, and what it reads. + +Split from `test_ci_placement_reader.py`, which reads runner *shapes*. These +cover the two questions asked alongside the shape: whether a job's own guard +lets it run at all, and which context properties a declaration reads, which is +what the job-name stability rule compares. + +Both are driven with synthetic declarations rather than this repository's own +correct workflows, because a rule parametrized over correct sources passes +whether or not it discriminates. +""" + +from __future__ import annotations + +import pytest + +from tests.helpers import ci_placement as reader + +OWNED = "ubicloud-standard-2" +HOSTED = "ubuntu-latest" +#: Every workflow the traversal must reach. Named rather than derived, so the +#: claim that `all_jobs` sees the whole estate is asserted rather than merely +#: true: a file the reader skipped would otherwise be invisible. +ESTATE_WORKFLOWS = frozenset({ + "build-wheels.yml", + "ci.yml", + "coverage-main.yml", + "delayed-pr-comment.yml", + "dependabot-automerge.yml", + "get-codescene-sha.yml", + "mutation-testing.yml", + "release.yml", +}) + + +def _declare(monkeypatch: pytest.MonkeyPatch, declared: dict[str, object]) -> None: + """Make the reader see one synthetic job declaration.""" + monkeypatch.setattr(reader, "job", lambda *_: declared) + + +@pytest.mark.parametrize( + "condition", + [ + "false", + "${{ false }}", + "0", + "0.0", + "''", + '""', + "null", + "false && matrix.target == 'x'", + "null && matrix.target == 'x'", + "0 && matrix.target == 'x'", + ], +) +def test_a_constant_false_guard_is_reported( + monkeypatch: pytest.MonkeyPatch, condition: str +) -> None: + """Catch a lane that satisfies every declaration rule and runs nothing.""" + _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) + assert reader.never_runs("w.yml", "j"), ( + f"{condition!r} can never be true, so the job gates nothing" + ) + + +@pytest.mark.parametrize( + "condition", + [ + "github.event_name == 'pull_request'", + "needs.changes.outputs.bench == 'true'", + "${{ !cancelled() }}", + "matrix.python-suite", + # GitHub treats a non-empty string as truthy, so a quoted `false` is a + # job that runs. Refusing it would be a false positive on a lane that + # executes, which is why it sits here rather than above. + "'false'", + '"false"', + "falsey", + "false_positive_guard", + ], +) +def test_a_real_guard_is_not_reported_as_never_running( + monkeypatch: pytest.MonkeyPatch, condition: str +) -> None: + """Prove the rule narrow: a legitimate condition must still pass. + + Sufficiency alone is not enough. A guard rule that also refused the + conditions this repository genuinely uses would be switched off, and a + contract with a false positive gates nothing (femtologging #480). + """ + _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) + assert not reader.never_runs("w.yml", "j"), ( + f"{condition!r} is a legitimate guard and must not be refused" + ) + + +@pytest.mark.parametrize( + ("declared", "expected"), + [ + pytest.param({"steps": []}, True, id="steps"), + pytest.param({"uses": "./w.yml"}, False, id="caller"), + pytest.param({"uses": "./w.yml", "steps": []}, False, id="both"), + pytest.param({}, False, id="neither"), + ], +) +def test_the_caller_exemption_is_keyed_on_uses( + monkeypatch: pytest.MonkeyPatch, declared: dict[str, object], expected: bool +) -> None: + """Exempt a reusable-workflow caller, and refuse a job with neither key. + + A blanket tolerance of a missing `runs-on` would exempt a malformed job + too; keying on `uses` leaves that job refused (whitaker #438). + """ + _declare(monkeypatch, declared) + assert reader.declares_steps("w.yml", "j") is expected, ( + f"{sorted(declared)} must read as declares_steps={expected}" + ) + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + pytest.param("plain", frozenset(), id="no-reference"), + pytest.param("${{ matrix.os }}", frozenset({"matrix.os"}), id="one"), + pytest.param( + "a ${{ matrix.a }} b ${{ matrix.b }}", + frozenset({"matrix.a", "matrix.b"}), + id="two", + ), + # The regression case. A body-level reader using `[^}]*` stops at the + # `}` inside `{0}` and reports no reference at all, so a name built + # this way shared nothing with a `runs-on` reading the same key and + # the stability rule passed on an unstable name. + pytest.param( + "${{ format('{0}', matrix.os) }}", + frozenset({"matrix.os"}), + id="nested-function", + ), + pytest.param( + "${{ format('{0}-{1}', matrix.os, matrix.arch) }}", + frozenset({"matrix.os", "matrix.arch"}), + id="nested-two-args", + ), + # A quoted label carrying a dot is a literal, not a property path. + pytest.param( + "${{ matrix.os == 'ubuntu-22.04' }}", + frozenset({"matrix.os"}), + id="quoted-dotted-literal", + ), + pytest.param( + f"${{{{ {reader.FORK_FIELD} && 'a' || 'b' }}}}", + frozenset({reader.FORK_FIELD}), + id="fork-expression", + ), + pytest.param(None, frozenset(), id="absent"), + pytest.param(True, frozenset(), id="non-string"), + ], +) +def test_references_are_read_from_any_declaration( + value: object, expected: frozenset[str] +) -> None: + """Read every reference, so a name and a runner can be compared.""" + assert reader.references(value) == expected, ( + f"{value!r} reads {sorted(reader.references(value))}" + ) + + +def test_a_matrix_leg_missing_the_runner_key_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Refuse an incomplete matrix rather than silently dropping a leg.""" + _declare( + monkeypatch, + { + "runs-on": "${{ matrix.os }}", + "steps": [], + "strategy": {"matrix": {"include": [{"os": HOSTED}, {"arch": "x86_64"}]}}, + }, + ) + with pytest.raises(AssertionError, match="declares no 'os'"): + reader.placement("w.yml", "j") + + +def test_all_jobs_reaches_every_workflow() -> None: + """Name the traversal, or the claim that it is complete is unasserted. + + Falcon-pachinko #149: a `workflow_call` file with no caller is one line + from gaining one, and a caller can live in another repository, so the + registry counts its labels rather than excluding the file by rule. + """ + reached = {workflow_name for workflow_name, _ in reader.all_jobs()} + assert reached == ESTATE_WORKFLOWS, ( + f"the traversal must reach every workflow; reached {sorted(reached)}" + ) + + +def test_the_frozen_hosted_set_holds_no_paid_label() -> None: + """Keep the exemption list free of the labels it exists to expose.""" + assert all( + not label.startswith(("ubicloud-", "namespace-", "buildjet-", "warpbuild-")) + for label in reader.FROZEN_HOSTED_LABELS + ), "a paid provider's label must never be frozen out of the registry question" diff --git a/tests/test_ci_placement_properties.py b/tests/test_ci_placement_properties.py new file mode 100644 index 000000000..98eab7be2 --- /dev/null +++ b/tests/test_ci_placement_properties.py @@ -0,0 +1,287 @@ +"""Property tests for the placement reader over its whole input space. + +The reader is a parser, and the example tests beside it pin the shapes this +repository happens to declare today. Three of the defects found in review were +inputs nobody had thought to write down: a composite expression whose +references went unread, a quoted string treated as falsy, and a padded +expression read as the bare one. Generating the space is how that class is +covered rather than enumerated. + +Each property states an invariant the contracts depend on, so a reader change +that quietly narrowed one would fail here rather than in a workflow months +later. +""" + +from __future__ import annotations + +import typing as typ +from unittest import mock + +import pytest +from hypothesis import given, settings +from hypothesis import strategies as st + +from tests.helpers import ci_placement as reader + +if typ.TYPE_CHECKING: + import collections.abc as cabc + import contextlib + + from tests.helpers.workflow_types import Job + +#: Hypothesis's default deadline measures the host, not the code, and these +#: cases are trivial; a flaky contract test is worse than a slow one. +SETTINGS = settings(deadline=None, max_examples=200) + +#: Horizontal whitespace that may sit *inside* an expression body without +#: changing it. Deliberately no newline: a line break in the parsed value is +#: the folded-scalar defect, refused by its own rule and generated by its own +#: property below. Padding *outside* the braces is a third thing, also refused. +INNER_SPACE = st.sampled_from(["", " ", " ", "\t"]) +#: The three junctions inside a fork body, drawn together so the property takes +#: one argument rather than three. +INNER_SPACING = st.tuples(INNER_SPACE, INNER_SPACE, INNER_SPACE) +LABELS = st.sampled_from([ + "ubuntu-latest", + "ubuntu-22.04", + "windows-2022", + "macos-latest", + "macos-15-intel", + "ubicloud-standard-2", + "ubicloud-standard-4", +]) +PROPERTY_PATHS = st.sampled_from([ + reader.FORK_FIELD, + "github.event.pull_request.head.repo.private", + "matrix.os", + "matrix.python-version", + "needs.changes.outputs.bench", +]) +MATRIX_KEYS = st.sampled_from(["os", "runner", "platform", "image-label"]) + + +def declaring( + declared: cabc.Mapping[str, object], +) -> contextlib.AbstractContextManager[object]: + """Make the reader see one synthetic declaration, for one block. + + A context manager rather than `monkeypatch`: a function-scoped fixture is + not reset between the inputs `@given` generates, so state would leak from + one example to the next. Hypothesis fails the health check for exactly that + reason, and suppressing it would keep the leak rather than remove it. + + Returns + ------- + contextlib.AbstractContextManager[object] + A context manager that installs the declaration and restores the real + reader on exit, including when the block raises. + """ + + def _declared(workflow_name: str, job_name: str) -> Job: + """Return the synthetic declaration whatever job is asked for.""" + del workflow_name, job_name + return typ.cast("Job", declared) + + return mock.patch.object(reader, "job", _declared) + + +@SETTINGS +@given( + condition=PROPERTY_PATHS, + fork_arm=LABELS, + owned_arm=LABELS, + spacing=INNER_SPACING, +) +def test_a_fork_expression_preserves_its_arms_by_position( + condition: str, + fork_arm: str, + owned_arm: str, + spacing: tuple[str, str, str], +) -> None: + """Read the arms by position however the body is spaced. + + Position is the whole point: an expression naming one hosted and one paid + label in either order satisfies a membership test, and one of those two + orders sends every fork to a runner it cannot obtain. + """ + before, after_condition, after_or = spacing + body = ( + f"{before}{condition}{after_condition}&& '{fork_arm}' ||{after_or}'{owned_arm}'" + ) + with declaring({"runs-on": f"${{{{{body}}}}}", "steps": []}): + placed = reader.placement("w.yml", "j") + assert placed.kind == "fork", f"body {body!r} read as {placed.kind}" + assert placed.fork == fork_arm, f"fork arm was {placed.fork!r}" + assert placed.owned == owned_arm, f"owned arm was {placed.owned!r}" + assert placed.references == frozenset({condition}), ( + f"condition read as {sorted(placed.references)}" + ) + + +@SETTINGS +@given(key=MATRIX_KEYS, labels=st.lists(LABELS, min_size=1, max_size=5, unique=True)) +def test_a_matrix_placement_resolves_every_leg(key: str, labels: list[str]) -> None: + """Resolve `runs-on` through the matrix rather than stopping at the key.""" + declared = { + "runs-on": f"${{{{ matrix.{key} }}}}", + "steps": [], + "strategy": {"matrix": {"include": [{key: label} for label in labels]}}, + } + with declaring(declared): + placed = reader.placement("w.yml", "j") + assert placed.kind == "matrix", f"read as {placed.kind}" + assert placed.labels == frozenset(labels), ( + f"legs {labels} resolved to {sorted(placed.labels)}" + ) + assert placed.references == frozenset({f"matrix.{key}"}), ( + f"references were {sorted(placed.references)}" + ) + assert placed.owned is None, "a matrix placement has no single owned label" + + +@SETTINGS +@given(label=LABELS) +def test_a_literal_label_round_trips(label: str) -> None: + """Report a bare label as itself, reading no expression from it. + + `ubuntu-22.04` is the case that matters: it carries a dot, and a reference + reader that did not exclude literals would report it as a context property. + """ + with declaring({"runs-on": label, "steps": []}): + placed = reader.placement("w.yml", "j") + assert placed.kind == "literal", f"{label!r} read as {placed.kind}" + assert placed.labels == frozenset({label}), f"labels were {sorted(placed.labels)}" + assert placed.references == frozenset(), ( + f"a literal reads no property, got {sorted(placed.references)}" + ) + + +@SETTINGS +@given( + prefix=st.sampled_from(["", " ", "runner-", "ubuntu-"]), + suffix=st.sampled_from(["", " ", "-latest"]), + path=PROPERTY_PATHS, +) +def test_anything_around_an_expression_is_refused( + prefix: str, suffix: str, path: str +) -> None: + """Refuse a value that interpolates without being wholly an expression. + + GitHub substitutes into the surrounding string and keeps what is around it, + so every one of these resolves to a label no runner matches. The empty + prefix and suffix pair is excluded because that is the supported shape. + """ + if not prefix and not suffix: + return + declared = {"runs-on": f"{prefix}${{{{ {path} }}}}{suffix}", "steps": []} + with ( + declaring(declared), + pytest.raises(AssertionError, match="interpolates its runner label"), + ): + reader.placement("w.yml", "j") + + +@SETTINGS +@given( + condition=PROPERTY_PATHS, + fork_arm=LABELS, + owned_arm=LABELS, + position=st.integers(min_value=0, max_value=3), +) +def test_a_line_break_anywhere_in_the_value_is_refused( + condition: str, fork_arm: str, owned_arm: str, position: int +) -> None: + """Refuse an embedded line break wherever the fold put it. + + A folded-scalar continuation indented one level deeper keeps its line break + and puts a newline inside the expression. GitHub evaluates the broken value + regardless, so a green run is not evidence and only the parsed document + shows it. The break is generated at each junction rather than at the one + place a handwritten fixture would put it. + """ + parts = [condition, "&&", f"'{fork_arm}'", "||", f"'{owned_arm}'"] + separators = [" "] * 4 + separators[position] = "\n" + body = "".join( + part + (separators[index] if index < len(separators) else "") + for index, part in enumerate(parts) + ) + with ( + declaring({"runs-on": f"${{{{{body}}}}}", "steps": []}), + pytest.raises(AssertionError, match="embedded line break"), + ): + reader.placement("w.yml", "j") + + +@SETTINGS +@given(paths=st.lists(PROPERTY_PATHS, min_size=1, max_size=4, unique=True)) +def test_every_property_in_a_composite_expression_is_read( + paths: list[str], +) -> None: + """Read every property a composite expression names, not the body. + + A body-level reader stops at the first `}`, which is inside `{0}`, and + reports no reference at all. The job-name stability rule compares + references, so that silence let an unstable name pass. + """ + arguments = ", ".join(paths) + template = "-".join(f"{{{index}}}" for index in range(len(paths))) + value = f"a ${{{{ format('{template}', {arguments}) }}}} b" + assert reader.references(value) == frozenset(paths), ( + f"{value!r} read {sorted(reader.references(value))}" + ) + + +#: Guards built the way a workflow builds them. Generated rather than listed, +#: because the risk here is a false positive on an ordinary condition, and a +#: rule that refused those would be switched off. +GUARD_SUBJECTS = st.sampled_from([ + "github.event_name", + "github.ref", + "needs.changes.outputs.bench", + "matrix.python-suite", + "inputs.publish-baseline", +]) +GUARD_VALUES = st.sampled_from([ + "'push'", + "'pull_request'", + "'refs/heads/main'", + "'true'", + "'false'", + "true", +]) + + +#: A comparison and the two ways a workflow dresses it: negated, wrapped in +#: `${{ }}`, both, or neither. Drawn as one value so the property keeps a +#: readable signature. +GUARD_SHAPES = st.tuples( + GUARD_SUBJECTS, + st.sampled_from(["==", "!="]), + GUARD_VALUES, + st.booleans(), + st.booleans(), +) + + +@SETTINGS +@given(shape=GUARD_SHAPES) +def test_a_real_guard_is_never_reported_as_dead( + shape: tuple[str, str, str, bool, bool], +) -> None: + """Keep the dead-lane rule off conditions that can be true. + + Note `'false'` among the generated values: GitHub treats a non-empty string + as truthy, so a comparison against it is an ordinary guard on a job that + runs. Encoding the quoted form as falsy was the defect this covers. + """ + subject, operator, value, negated, wrapped = shape + text = f"{subject} {operator} {value}" + if negated: + text = f"!({text})" + if wrapped: + text = f"${{{{ {text} }}}}" + with declaring({"runs-on": "ubuntu-latest", "steps": [], "if": text}): + assert not reader.never_runs("w.yml", "j"), ( + f"{text!r} names no falsy literal, so the job can run" + ) diff --git a/tests/test_ci_placement_reader.py b/tests/test_ci_placement_reader.py index 6389587aa..bad0a1175 100644 --- a/tests/test_ci_placement_reader.py +++ b/tests/test_ci_placement_reader.py @@ -9,7 +9,7 @@ The shapes that must be *refused* matter as much as the ones that must be read. An unparsable `runs-on` recorded as one opaque label carries no vendor prefix, so the Ubicloud classifier drops the lane and it sits exempt from every -placement, ceiling and registry assertion while still asking for a paid runner +placement, ceiling, and registry assertion while still asking for a paid runner (axinite #372). """ @@ -116,6 +116,24 @@ def test_a_sibling_field_is_reported_as_a_different_condition( ) +def test_a_hyphenated_condition_is_read(monkeypatch: pytest.MonkeyPatch) -> None: + """Read a condition whose property path carries a hyphen. + + `matrix.python-version` is a legitimate condition, and a character class + without `-` refused the whole expression as unmodellable. The refusal + failed safe, so nothing went unchecked, but it would have rejected a + correct lane. Found by the generated property in + `test_ci_placement_properties.py` rather than by any example. + """ + hyphenated = f"${{{{ matrix.python-version && '{HOSTED}' || '{OWNED}' }}}}" + _declare(monkeypatch, {"runs-on": hyphenated, "steps": []}) + placed = reader.placement("w.yml", "j") + assert placed.kind == "fork", f"read as {placed.kind}" + assert placed.references == frozenset({"matrix.python-version"}), ( + f"condition read as {sorted(placed.references)}" + ) + + def test_a_matrix_placement_resolves_through_its_include( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -213,23 +231,29 @@ def test_an_interpolated_label_is_refused(monkeypatch: pytest.MonkeyPatch) -> No reader.placement("w.yml", "j") -def test_incidental_whitespace_around_an_expression_is_read( - monkeypatch: pytest.MonkeyPatch, +@pytest.mark.parametrize( + "declared", + [ + pytest.param(f" {FORK_EXPRESSION}", id="leading-space"), + pytest.param(f"{FORK_EXPRESSION} ", id="trailing-space"), + pytest.param(" ${{ inputs.chosen-os }}", id="padded-opaque"), + ], +) +def test_a_padded_expression_is_refused( + monkeypatch: pytest.MonkeyPatch, declared: str ) -> None: - """Keep a correctly written lane readable when a scalar leaves a space. - - A folded scalar can leave a trailing space. Without tolerating it the whole - declaration falls through to the literal branch, and a correct fork lane - quietly stops being read as one, which is a false negative rather than a - false positive and so is the harder kind to notice. + """Refuse a padded expression rather than reading it as the bare one. + + GitHub interpolates into the surrounding string and keeps what is around + the expression, so `" ${{ ... }}"` resolves to a label with a leading space + that matches no runner. Reading it as the unpadded expression would report + a broken lane as a correct fork placement, which is the dangerous + direction: the contract would go green on a job that can never be + scheduled. """ - _declare(monkeypatch, {"runs-on": f" {FORK_EXPRESSION} ", "steps": []}) - placed = reader.placement("w.yml", "j") - assert placed.kind == "fork", ( - f"incidental whitespace must not change the shape, got {placed.kind}" - ) - assert placed.owned == OWNED, f"owned arm was {placed.owned!r}" - assert placed.fork == HOSTED, f"fork arm was {placed.fork!r}" + _declare(monkeypatch, {"runs-on": declared, "steps": []}) + with pytest.raises(AssertionError, match="interpolates its runner label"): + reader.placement("w.yml", "j") def test_the_refusal_of_an_opaque_expression_is_still_needed( @@ -245,124 +269,3 @@ def test_the_refusal_of_an_opaque_expression_is_still_needed( _declare(monkeypatch, {"runs-on": "${{ inputs.chosen-os }}", "steps": []}) with pytest.raises(AssertionError): reader.placement("w.yml", "j") - - -@pytest.mark.parametrize( - "condition", - ["false", "${{ false }}", "'false'", "false && matrix.target == 'x'", "0"], -) -def test_a_constant_false_guard_is_reported( - monkeypatch: pytest.MonkeyPatch, condition: str -) -> None: - """Catch a lane that satisfies every declaration rule and runs nothing.""" - _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) - assert reader.never_runs("w.yml", "j"), ( - f"{condition!r} can never be true, so the job gates nothing" - ) - - -@pytest.mark.parametrize( - "condition", - [ - "github.event_name == 'pull_request'", - "needs.changes.outputs.bench == 'true'", - "${{ !cancelled() }}", - "matrix.python-suite", - ], -) -def test_a_real_guard_is_not_reported_as_never_running( - monkeypatch: pytest.MonkeyPatch, condition: str -) -> None: - """Prove the rule narrow: a legitimate condition must still pass. - - Sufficiency alone is not enough. A guard rule that also refused the - conditions this repository genuinely uses would be switched off, and a - contract with a false positive gates nothing (femtologging #480). - """ - _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) - assert not reader.never_runs("w.yml", "j"), ( - f"{condition!r} is a legitimate guard and must not be refused" - ) - - -@pytest.mark.parametrize( - ("declared", "expected"), - [ - pytest.param({"steps": []}, True, id="steps"), - pytest.param({"uses": "./w.yml"}, False, id="caller"), - pytest.param({"uses": "./w.yml", "steps": []}, False, id="both"), - pytest.param({}, False, id="neither"), - ], -) -def test_the_caller_exemption_is_keyed_on_uses( - monkeypatch: pytest.MonkeyPatch, declared: dict[str, object], expected: bool -) -> None: - """Exempt a reusable-workflow caller, and refuse a job with neither key. - - A blanket tolerance of a missing `runs-on` would exempt a malformed job - too; keying on `uses` leaves that job refused (whitaker #438). - """ - _declare(monkeypatch, declared) - assert reader.declares_steps("w.yml", "j") is expected, ( - f"{sorted(declared)} must read as declares_steps={expected}" - ) - - -@pytest.mark.parametrize( - ("value", "expected"), - [ - pytest.param("plain", frozenset(), id="no-reference"), - pytest.param("${{ matrix.os }}", frozenset({"matrix.os"}), id="one"), - pytest.param( - "a ${{ matrix.a }} b ${{ matrix.b }}", - frozenset({"matrix.a", "matrix.b"}), - id="two", - ), - pytest.param(None, frozenset(), id="absent"), - pytest.param(True, frozenset(), id="non-string"), - ], -) -def test_references_are_read_from_any_declaration( - value: object, expected: frozenset[str] -) -> None: - """Read every reference, so a name and a runner can be compared.""" - assert reader.references(value) == expected, ( - f"{value!r} reads {sorted(reader.references(value))}" - ) - - -def test_a_matrix_leg_missing_the_runner_key_is_refused( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """Refuse an incomplete matrix rather than silently dropping a leg.""" - _declare( - monkeypatch, - { - "runs-on": "${{ matrix.os }}", - "steps": [], - "strategy": {"matrix": {"include": [{"os": HOSTED}, {"arch": "x86_64"}]}}, - }, - ) - with pytest.raises(AssertionError, match="declares no 'os'"): - reader.placement("w.yml", "j") - - -def test_all_jobs_reaches_every_workflow() -> None: - """Name the traversal, or the claim that it is complete is unasserted. - - Falcon-pachinko #149: a `workflow_call` file with no caller is one line - from gaining one, and a caller can live in another repository, so the - registry counts its labels rather than excluding the file by rule. - """ - reached = {workflow_name for workflow_name, _ in reader.all_jobs()} - assert reached == ESTATE_WORKFLOWS, ( - f"the traversal must reach every workflow; reached {sorted(reached)}" - ) - - -def test_the_frozen_hosted_set_holds_no_paid_label() -> None: - """Keep the exemption list free of the labels it exists to expose.""" - assert all( - not label.startswith(("ubicloud-", "namespace-", "buildjet-", "warpbuild-")) - for label in reader.FROZEN_HOSTED_LABELS - ), "a paid provider's label must never be frozen out of the registry question" diff --git a/tests/test_ci_runner_placement.py b/tests/test_ci_runner_placement.py index d19cdce92..80325be6c 100644 --- a/tests/test_ci_runner_placement.py +++ b/tests/test_ci_runner_placement.py @@ -150,8 +150,17 @@ def test_every_job_running_steps_declares_a_ceiling( rule keyed on the label stops applying on exactly the arm that hangs. """ timeout = job(workflow_name, job_name).get("timeout-minutes") - assert isinstance(timeout, int), ( - f"{workflow_name}:{job_name} must declare timeout-minutes, got {timeout!r}" + # `isinstance(True, int)` is true, so an `isinstance` check also accepts + # `timeout-minutes: true`, and it accepts `0` and negative values besides. + # GitHub requires a positive integer. No upper bound is asserted: 360 is + # the GitHub-hosted execution limit, not a repository-wide job ceiling. + assert type(timeout) is int, ( + f"{workflow_name}:{job_name} must declare an integer timeout-minutes, " + f"got {timeout!r}; note YAML's `true` is an int to `isinstance`" + ) + assert timeout > 0, ( + f"{workflow_name}:{job_name} must declare a positive timeout-minutes, " + f"got {timeout!r}" ) diff --git a/tests/test_ci_tool_installation.py b/tests/test_ci_tool_installation.py index f4636ba09..4339f5460 100644 --- a/tests/test_ci_tool_installation.py +++ b/tests/test_ci_tool_installation.py @@ -13,6 +13,7 @@ from tests.helpers.ci_placement import declares_steps from tests.helpers.ci_runners import ( + GENERATE_COVERAGE, steps, workflow_document, workflow_sources, @@ -28,9 +29,9 @@ #: The upstream install host. Its name does not contain "nextest", so it needs #: its own token, and it only ever appears in an install command. NEXTEST_INSTALL_HOST = "get.nexte.st" -#: Steps invoking a shared action are exempt: the coverage action installs -#: nextest on purpose, and that is the only sanctioned place. -SHARED_ACTION_PREFIX = "leynos/shared-actions/" +#: The one sanctioned installer, matched exactly. A prefix over the whole +#: shared-actions namespace would exempt setup-rust, install-mdtablefix and +#: every sibling, so any of them could take a nextest input unremarked. def test_ci_does_not_build_tools_from_source() -> None: @@ -77,9 +78,10 @@ def test_no_workflow_installs_cargo_nextest() -> None: def _installs_nextest_by_input(step: Step) -> bool: """Report whether a step asks an installer action for cargo-nextest.""" - if str(step.get("uses", "")).startswith(SHARED_ACTION_PREFIX): + if str(step.get("uses", "")) == GENERATE_COVERAGE: # The coverage action installs nextest deliberately; that is the one - # place it is meant to happen. + # place it is meant to happen, and it is matched exactly rather than by + # namespace so no sibling action inherits the exemption. return False inputs = step.get("with") if not isinstance(inputs, dict): From 9c0f77ef79223295b5192ec5f4ffb5562c2ec8ec Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Tue, 22 Sep 2026 01:24:17 +0100 Subject: [PATCH 4/7] Make the ceiling and exemption rules falsifiable Mutation-proving the round's own fixes found two that were correct and unasserted: reverting the timeout check to `isinstance` and widening the nextest exemption back to the whole shared-actions namespace both survived every test. Neither defect exists in this repository's workflows, so a contract parametrized over them passes whether or not it discriminates. Extract the ceiling into a predicate and drive it directly over the values no workflow here declares: YAML's `true`, which `isinstance` accepts as an int, plus a string, a float, absence, zero and a negative. Test the nextest predicate directly against a sibling shared action. Both mutations now fail. Splitting the reader again to stay under the 400-line limit: the three rules that are about a job rather than about its runner move to `ci_job_rules`, and the contract guard becomes public rather than importing a private name across modules. --- tests/helpers/ci_job_rules.py | 129 ++++++++++++++++++++++++++ tests/helpers/ci_placement.py | 114 ++++++----------------- tests/helpers/ci_runners.py | 42 +++++++-- tests/test_ci_placement_guards.py | 56 +++++++++-- tests/test_ci_placement_properties.py | 21 +++-- tests/test_ci_runner_placement.py | 19 ++-- tests/test_ci_tool_installation.py | 30 ++++++ 7 files changed, 288 insertions(+), 123 deletions(-) create mode 100644 tests/helpers/ci_job_rules.py diff --git a/tests/helpers/ci_job_rules.py b/tests/helpers/ci_job_rules.py new file mode 100644 index 000000000..2f977e00b --- /dev/null +++ b/tests/helpers/ci_job_rules.py @@ -0,0 +1,129 @@ +"""Rules about a job that are not about which runner it selects. + +Split from `ci_placement`, which models `runs-on`. These three read the other +declarations a placement contract depends on: the ceiling that bounds a job, +whether its own guard lets it run, and which context properties a declaration +reads, which is what the job-name stability rule compares. + +Each is a predicate rather than an assertion inside a contract, so it can be +driven directly over the values this repository does not happen to declare. +That is not a stylistic preference: a rule parametrized over correct sources +passes whether or not it discriminates, and two of these survived their own +mutations until they were extracted and tested this way. +""" + +from __future__ import annotations + +import re +import typing as typ + +from tests.helpers.ci_placement import EXPRESSION, require +from tests.helpers.ci_workflows import job + +#: One `${{ ... }}` span. Non-greedy to the first `}}`, because `[^}]*` stops +#: at the `}` inside `format('{0}', matrix.os)` and reports no reference at all. +_EXPRESSION_SPAN = re.compile(r"\$\{\{(?P.*?)\}\}", re.DOTALL) +#: A quoted literal inside an expression body. Stripped before references are +#: read, so a label such as `'ubuntu-22.04'` is not mistaken for a property +#: path on the strength of its dot. +_QUOTED = re.compile(r"'[^']*'|\"[^\"]*\"") +#: A dotted context property such as `matrix.os` or +#: `github.event.pull_request.head.repo.fork`. A bare function name such as +#: `format` carries no dot and is correctly not a reference. +_PROPERTY = re.compile(r"[A-Za-z_][\w-]*(?:\.[\w-]+)+") +#: Expressions a job-level ``if:`` may not reduce to. A job that cannot run +#: satisfies every declaration-reading rule while gating nothing. +#: GitHub Actions evaluates `false`, numeric zero, the empty string and `null` +#: as falsy, and **a non-empty string as truthy**, so the quoted `'false'` +#: belongs on the other side of this line rather than in it. +_NEVER_RUNS: typ.Final = frozenset({"false", "0", "0.0", "''", '""', "null"}) + + +def ceiling(workflow_name: str, job_name: str) -> int: + """Return a job's declared ``timeout-minutes``, refusing an unusable value. + + GitHub requires a positive integer. Two spellings pass an ``isinstance`` + check and should not: YAML's ``true`` is an ``int`` to Python, and zero and + negatives are integers that bound nothing. + + Extracted from the contract so the rule can be driven directly. Asserted + against this repository's own correct workflows it would pass whether or + not it discriminated, and a mutation from ``type(...) is int`` back to + ``isinstance`` survived until this existed. + + Returns + ------- + int + The declared ceiling, guaranteed a positive, non-boolean integer. + + Notes + ----- + Fails the contract, through :func:`_require`, when the value is absent, + boolean, a non-integer, or not positive. + """ + declared = job(workflow_name, job_name).get("timeout-minutes") + where = f"{workflow_name}:{job_name}" + require( + condition=type(declared) is int, + message=( + f"{where} must declare an integer timeout-minutes, got {declared!r}; " + "note YAML's `true` is an int to `isinstance`" + ), + ) + value = typ.cast("int", declared) + require( + condition=value > 0, + message=f"{where} must declare a positive timeout-minutes, got {value!r}", + ) + return value + + +def references(value: object) -> frozenset[str]: + """Return every context property a declaration reads. + + Property paths rather than whole expression bodies. A name built with + ``${{ format('{0}', matrix.os) }}`` reads ``matrix.os`` and has to be + comparable with a ``runs-on`` reading the same key; returning the body made + that comparison miss entirely, so an unstable name passed the stability + rule. + + Returns + ------- + frozenset[str] + Every dotted context property the value reads, across every + ``${{ ... }}`` span, with quoted literals excluded. + """ + if not isinstance(value, str): + return frozenset() + found: set[str] = set() + for body in _EXPRESSION_SPAN.findall(value): + found.update(_PROPERTY.findall(_QUOTED.sub(" ", body))) + return frozenset(found) + + +def never_runs(workflow_name: str, job_name: str) -> bool: + """Report whether a job's own ``if:`` makes it unreachable. + + A constant-false guard leaves every declaration intact and runs nothing, + which is exactly the state a placement contract must refuse rather than + report as compliant. + + Returns + ------- + bool + Whether the job's own ``if:`` reduces to a constant false. + """ + condition = job(workflow_name, job_name).get("if") + if not isinstance(condition, str): + return False + text = " ".join(condition.split()) + expression = EXPRESSION.match(text) + if expression is not None: + text = " ".join(expression.group("body").split()) + lowered = text.lower() + if lowered in _NEVER_RUNS: + return True + # A containment test would accept `false && matrix.target == 'x'`, so the + # leading clause is matched explicitly, and for every falsy literal rather + # than only `false`: `null && ...` and `0 && ...` are equally unreachable. + return any(lowered.startswith(f"{literal} &&") for literal in _NEVER_RUNS) diff --git a/tests/helpers/ci_placement.py b/tests/helpers/ci_placement.py index 5e5450bf5..efb9ef6fe 100644 --- a/tests/helpers/ci_placement.py +++ b/tests/helpers/ci_placement.py @@ -67,14 +67,14 @@ # runner matches. Tolerating the padding would report such a lane as a correct # fork placement, a false positive in the dangerous direction; anything padded # falls to the interpolation refusal instead. -_EXPRESSION = re.compile(r"^\$\{\{(?P.*)\}\}$", re.DOTALL) +EXPRESSION = re.compile(r"^\$\{\{(?P.*)\}\}$", re.DOTALL) #: An expression *fragment*, used to refuse a value that interpolates without #: being wholly an expression, whether padded or concatenated. _INTERPOLATION = re.compile(r"\$\{\{") #: The canonical fork fallback, anchored end to end so the arms are read by #: position rather than by membership. `\A`/`\Z` rather than a bare `fullmatch` #: on a pattern that could otherwise be satisfied by a prefix. -_FORK_EXPRESSION = re.compile( +_FORKEXPRESSION = re.compile( # Hyphens belong in the character class: `matrix.python-version` is a # legitimate condition, and excluding `-` refused it as unmodellable. Found # by the generated property, not by any example anyone wrote down. @@ -83,28 +83,19 @@ r"\s*\|\|\s*'(?P[^']+)'\s*\Z" ) _MATRIX_EXPRESSION = re.compile(r"\A\s*matrix\.(?P[\w-]+)\s*\Z") -#: Every ``${{ ... }}`` reference in a string, for the job-name stability rule. -#: One `${{ ... }}` span. Non-greedy to the first `}}`, because `[^}]*` stops -#: at the `}` inside `format('{0}', matrix.os)` and reports no reference at all. -_EXPRESSION_SPAN = re.compile(r"\$\{\{(?P.*?)\}\}", re.DOTALL) -#: A quoted literal inside an expression body. Stripped before references are -#: read, so a label such as `'ubuntu-22.04'` is not mistaken for a property -#: path on the strength of its dot. -_QUOTED = re.compile(r"'[^']*'|\"[^\"]*\"") -#: A dotted context property such as `matrix.os` or -#: `github.event.pull_request.head.repo.fork`. A bare function name such as -#: `format` carries no dot and is correctly not a reference. -_PROPERTY = re.compile(r"[A-Za-z_][\w-]*(?:\.[\w-]+)+") -#: Expressions a job-level ``if:`` may not reduce to. A job that cannot run -#: satisfies every declaration-reading rule while gating nothing. -#: GitHub Actions evaluates `false`, numeric zero, the empty string and `null` -#: as falsy, and **a non-empty string as truthy**, so the quoted `'false'` -#: belongs on the other side of this line rather than in it. -_NEVER_RUNS: typ.Final = frozenset({"false", "0", "0.0", "''", '""', "null"}) - - -def _require(*, condition: bool, message: str) -> None: - """Raise a contract failure when ``condition`` does not hold.""" + + +def require(*, condition: bool, message: str) -> None: + """Raise a contract failure when ``condition`` does not hold. + + Public because `ci_job_rules` shares it; the alternative was a third copy + of the same three lines. + + Raises + ------ + AssertionError + When ``condition`` is false, carrying ``message``. + """ if not condition: raise AssertionError(message) @@ -180,7 +171,7 @@ def all_jobs() -> list[tuple[str, str]]: def _matrix_values(workflow_name: str, job_name: str, key: str) -> list[object]: """Return the values one matrix key takes across the job's ``include``.""" strategy = job(workflow_name, job_name).get("strategy") - _require( + require( condition=isinstance(strategy, dict), message=( f"{workflow_name}:{job_name} selects its runner from matrix.{key} " @@ -188,7 +179,7 @@ def _matrix_values(workflow_name: str, job_name: str, key: str) -> list[object]: ), ) matrix = typ.cast("dict[str, object]", strategy).get("matrix") - _require( + require( condition=isinstance(matrix, dict), message=f"{workflow_name}:{job_name} strategy declares no matrix", ) @@ -202,7 +193,7 @@ def _matrix_values(workflow_name: str, job_name: str, key: str) -> list[object]: if isinstance(direct, list): return list(typ.cast("list[object]", direct)) include = declared.get("include") - _require( + require( condition=isinstance(include, list), message=( f"{workflow_name}:{job_name} declares a matrix this reader cannot " @@ -213,7 +204,7 @@ def _matrix_values(workflow_name: str, job_name: str, key: str) -> list[object]: legs = typ.cast("list[object]", include) values: list[object] = [] for index, leg in enumerate(legs): - _require( + require( condition=isinstance(leg, dict) and key in leg, message=( f"{workflow_name}:{job_name} matrix leg {index} declares no " @@ -230,8 +221,8 @@ def _literal_labels( """Narrow matrix runner values to literal labels, refusing expressions.""" labels: set[str] = set() for value in values: - _require( - condition=isinstance(value, str) and _EXPRESSION.match(value) is None, + require( + condition=isinstance(value, str) and EXPRESSION.match(value) is None, message=( f"{workflow_name}:{job_name} resolves its runner to {value!r}; " "a matrix value holding an expression is a placement this " @@ -261,7 +252,7 @@ def placement(workflow_name: str, job_name: str) -> Placement: """ where = f"{workflow_name}:{job_name}" declared = job(workflow_name, job_name).get("runs-on") - _require( + require( condition=not isinstance(declared, list), message=( f"{where} declares a list runs-on. Each entry would be recorded as " @@ -270,7 +261,7 @@ def placement(workflow_name: str, job_name: str) -> Placement: "expression (dev-env-rocky #216). Refused rather than modelled." ), ) - _require( + require( condition=isinstance(declared, str) and bool(declared), message=f"{where} must declare a non-empty string runs-on, got {declared!r}", ) @@ -278,21 +269,21 @@ def placement(workflow_name: str, job_name: str) -> Placement: # The folded-scalar hazard. A continuation indented one level deeper keeps # its line break, and GitHub evaluates the broken value regardless, so a # green run is not evidence that the expression is one line. - _require( + require( condition="\n" not in text, message=( f"{where} runs-on parses with an embedded line break: {text!r}. " "Keep a folded-scalar continuation at the same indent." ), ) - match = _EXPRESSION.match(text) + match = EXPRESSION.match(text) if match is None: # A value that interpolates without being wholly an expression, such as # `ubuntu-${{ matrix.release }}`, is not a literal label. Recording it # as one is the same defect this reader exists to prevent, wearing a # different shape: the label it resolves to at run time is invisible to # every placement and registry assertion. - _require( + require( condition=_INTERPOLATION.search(text) is None, message=( f"{where} interpolates its runner label: {text!r}. The labels " @@ -302,7 +293,7 @@ def placement(workflow_name: str, job_name: str) -> Placement: ) return Placement("literal", text, None, frozenset({text}), frozenset()) body = match.group("body") - fork = _FORK_EXPRESSION.match(body) + fork = _FORKEXPRESSION.match(body) if fork is not None: return Placement( "fork", @@ -329,54 +320,3 @@ def placement(workflow_name: str, job_name: str) -> Placement: "classifier while still asking for a paid runner (axinite #372)." ) raise AssertionError(message) - - -def references(value: object) -> frozenset[str]: - """Return every context property a declaration reads. - - Property paths rather than whole expression bodies. A name built with - ``${{ format('{0}', matrix.os) }}`` reads ``matrix.os`` and has to be - comparable with a ``runs-on`` reading the same key; returning the body made - that comparison miss entirely, so an unstable name passed the stability - rule. - - Returns - ------- - frozenset[str] - Every dotted context property the value reads, across every - ``${{ ... }}`` span, with quoted literals excluded. - """ - if not isinstance(value, str): - return frozenset() - found: set[str] = set() - for body in _EXPRESSION_SPAN.findall(value): - found.update(_PROPERTY.findall(_QUOTED.sub(" ", body))) - return frozenset(found) - - -def never_runs(workflow_name: str, job_name: str) -> bool: - """Report whether a job's own ``if:`` makes it unreachable. - - A constant-false guard leaves every declaration intact and runs nothing, - which is exactly the state a placement contract must refuse rather than - report as compliant. - - Returns - ------- - bool - Whether the job's own ``if:`` reduces to a constant false. - """ - condition = job(workflow_name, job_name).get("if") - if not isinstance(condition, str): - return False - text = " ".join(condition.split()) - expression = _EXPRESSION.match(text) - if expression is not None: - text = " ".join(expression.group("body").split()) - lowered = text.lower() - if lowered in _NEVER_RUNS: - return True - # A containment test would accept `false && matrix.target == 'x'`, so the - # leading clause is matched explicitly, and for every falsy literal rather - # than only `false`: `null && ...` and `0 && ...` are equally unreachable. - return any(lowered.startswith(f"{literal} &&") for literal in _NEVER_RUNS) diff --git a/tests/helpers/ci_runners.py b/tests/helpers/ci_runners.py index 5bf8aba96..2402277e8 100644 --- a/tests/helpers/ci_runners.py +++ b/tests/helpers/ci_runners.py @@ -13,15 +13,14 @@ import typing as typ +from tests.helpers.ci_job_rules import ceiling, never_runs, references from tests.helpers.ci_placement import ( FORK_FIELD, FROZEN_HOSTED_LABELS, Placement, all_jobs, declares_steps, - never_runs, placement, - references, ) from tests.helpers.ci_workflows import ( CACHE_ACTION_PIN, @@ -55,14 +54,37 @@ # workflows are meant to contain, `ci_workflows` reads what they do contain. # fmt: off __all__ = ( - "CACHE_ACTION_PIN", "CACHE_PLAIN", "CACHE_RESTORE", "CACHE_SAVE", - "FORK_FIELD", "FROZEN_HOSTED_LABELS", "ROOT", "WORKFLOW_DIR", - "Placement", "RUNNER_NAMED_JOBS", "all_jobs", "cache_paths", - "cache_steps", "ceiling", - "declares_steps", "expand", "job", "job_env", "jobs", "never_runs", - "placement", "references", "restore_steps", "save_steps", - "single_step_position_using", "single_step_using", "step_inputs", - "steps", "workflow_document", "workflow_env", "workflow_sources", + "CACHE_ACTION_PIN", + "CACHE_PLAIN", + "CACHE_RESTORE", + "CACHE_SAVE", + "FORK_FIELD", + "FROZEN_HOSTED_LABELS", + "ROOT", + "RUNNER_NAMED_JOBS", + "WORKFLOW_DIR", + "Placement", + "all_jobs", + "cache_paths", + "cache_steps", + "ceiling", + "declares_steps", + "expand", + "job", + "job_env", + "jobs", + "never_runs", + "placement", + "references", + "restore_steps", + "save_steps", + "single_step_position_using", + "single_step_using", + "step_inputs", + "steps", + "workflow_document", + "workflow_env", + "workflow_sources", ) # fmt: on diff --git a/tests/test_ci_placement_guards.py b/tests/test_ci_placement_guards.py index a33adb4f9..f9901f240 100644 --- a/tests/test_ci_placement_guards.py +++ b/tests/test_ci_placement_guards.py @@ -14,6 +14,7 @@ import pytest +from tests.helpers import ci_job_rules as rules from tests.helpers import ci_placement as reader OWNED = "ubicloud-standard-2" @@ -22,20 +23,28 @@ #: claim that `all_jobs` sees the whole estate is asserted rather than merely #: true: a file the reader skipped would otherwise be invisible. ESTATE_WORKFLOWS = frozenset({ + "benchmark-gate-harness.yml", "build-wheels.yml", "ci.yml", "coverage-main.yml", "delayed-pr-comment.yml", "dependabot-automerge.yml", - "get-codescene-sha.yml", + "loom.yml", "mutation-testing.yml", "release.yml", + "rust-boundaries.yml", }) def _declare(monkeypatch: pytest.MonkeyPatch, declared: dict[str, object]) -> None: - """Make the reader see one synthetic job declaration.""" + """Make both readers see one synthetic job declaration. + + The shape reader and the job rules each import `job` into their own + namespace, so patching one would leave the other reading the repository's + real workflows and the test would silently assert nothing. + """ monkeypatch.setattr(reader, "job", lambda *_: declared) + monkeypatch.setattr(rules, "job", lambda *_: declared) @pytest.mark.parametrize( @@ -58,7 +67,7 @@ def test_a_constant_false_guard_is_reported( ) -> None: """Catch a lane that satisfies every declaration rule and runs nothing.""" _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) - assert reader.never_runs("w.yml", "j"), ( + assert rules.never_runs("w.yml", "j"), ( f"{condition!r} can never be true, so the job gates nothing" ) @@ -89,7 +98,7 @@ def test_a_real_guard_is_not_reported_as_never_running( contract with a false positive gates nothing (femtologging #480). """ _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) - assert not reader.never_runs("w.yml", "j"), ( + assert not rules.never_runs("w.yml", "j"), ( f"{condition!r} is a legitimate guard and must not be refused" ) @@ -160,8 +169,8 @@ def test_references_are_read_from_any_declaration( value: object, expected: frozenset[str] ) -> None: """Read every reference, so a name and a runner can be compared.""" - assert reader.references(value) == expected, ( - f"{value!r} reads {sorted(reader.references(value))}" + assert rules.references(value) == expected, ( + f"{value!r} reads {sorted(rules.references(value))}" ) @@ -200,3 +209,38 @@ def test_the_frozen_hosted_set_holds_no_paid_label() -> None: not label.startswith(("ubicloud-", "namespace-", "buildjet-", "warpbuild-")) for label in reader.FROZEN_HOSTED_LABELS ), "a paid provider's label must never be frozen out of the registry question" + + +@pytest.mark.parametrize( + ("declared", "expected"), + [ + pytest.param(True, "integer timeout-minutes", id="yaml-true"), + pytest.param(False, "integer timeout-minutes", id="yaml-false"), + pytest.param("45", "integer timeout-minutes", id="string"), + pytest.param(45.0, "integer timeout-minutes", id="float"), + pytest.param(None, "integer timeout-minutes", id="absent"), + pytest.param(0, "positive timeout-minutes", id="zero"), + pytest.param(-5, "positive timeout-minutes", id="negative"), + ], +) +def test_an_unusable_ceiling_is_refused( + monkeypatch: pytest.MonkeyPatch, declared: object, expected: str +) -> None: + """Refuse the ceilings that bound nothing. + + Driven directly rather than over this repository's workflows, which all + declare sensible values. Parametrized over correct sources the rule passes + whether or not it discriminates, and a mutation from `type(...) is int` + back to `isinstance` survived until this test existed: `True` is an `int` + to Python, so `timeout-minutes: true` would have satisfied the contract + while bounding the job at one minute. + """ + _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "timeout-minutes": declared}) + with pytest.raises(AssertionError, match=expected): + rules.ceiling("w.yml", "j") + + +def test_a_usable_ceiling_is_returned(monkeypatch: pytest.MonkeyPatch) -> None: + """Prove the rule narrow: an ordinary positive integer must pass.""" + _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "timeout-minutes": 45}) + assert rules.ceiling("w.yml", "j") == 45, "a positive integer is a valid ceiling" diff --git a/tests/test_ci_placement_properties.py b/tests/test_ci_placement_properties.py index 98eab7be2..473492928 100644 --- a/tests/test_ci_placement_properties.py +++ b/tests/test_ci_placement_properties.py @@ -14,6 +14,7 @@ from __future__ import annotations +import contextlib import typing as typ from unittest import mock @@ -21,11 +22,11 @@ from hypothesis import given, settings from hypothesis import strategies as st +from tests.helpers import ci_job_rules as rules from tests.helpers import ci_placement as reader if typ.TYPE_CHECKING: import collections.abc as cabc - import contextlib from tests.helpers.workflow_types import Job @@ -73,8 +74,8 @@ def declaring( Returns ------- contextlib.AbstractContextManager[object] - A context manager that installs the declaration and restores the real - reader on exit, including when the block raises. + A context manager that installs the declaration in both reader modules + and restores them on exit, including when the block raises. """ def _declared(workflow_name: str, job_name: str) -> Job: @@ -82,7 +83,13 @@ def _declared(workflow_name: str, job_name: str) -> Job: del workflow_name, job_name return typ.cast("Job", declared) - return mock.patch.object(reader, "job", _declared) + stack = contextlib.ExitStack() + # Both modules import `job` into their own namespace, so patching one would + # leave the other reading the repository's real workflows and the property + # would assert nothing. + for module in (reader, rules): + stack.enter_context(mock.patch.object(module, "job", _declared)) + return stack @SETTINGS @@ -227,8 +234,8 @@ def test_every_property_in_a_composite_expression_is_read( arguments = ", ".join(paths) template = "-".join(f"{{{index}}}" for index in range(len(paths))) value = f"a ${{{{ format('{template}', {arguments}) }}}} b" - assert reader.references(value) == frozenset(paths), ( - f"{value!r} read {sorted(reader.references(value))}" + assert rules.references(value) == frozenset(paths), ( + f"{value!r} read {sorted(rules.references(value))}" ) @@ -282,6 +289,6 @@ def test_a_real_guard_is_never_reported_as_dead( if wrapped: text = f"${{{{ {text} }}}}" with declaring({"runs-on": "ubuntu-latest", "steps": [], "if": text}): - assert not reader.never_runs("w.yml", "j"), ( + assert not rules.never_runs("w.yml", "j"), ( f"{text!r} names no falsy literal, so the job can run" ) diff --git a/tests/test_ci_runner_placement.py b/tests/test_ci_runner_placement.py index 80325be6c..ed2969995 100644 --- a/tests/test_ci_runner_placement.py +++ b/tests/test_ci_runner_placement.py @@ -30,6 +30,7 @@ WINDOWS_HOSTED_JOBS, WINDOWS_LABEL, all_jobs, + ceiling, declares_steps, expand, job, @@ -149,19 +150,11 @@ def test_every_job_running_steps_declares_a_ceiling( label is an expression, "an Ubicloud lane" is a property of the event, so a rule keyed on the label stops applying on exactly the arm that hangs. """ - timeout = job(workflow_name, job_name).get("timeout-minutes") - # `isinstance(True, int)` is true, so an `isinstance` check also accepts - # `timeout-minutes: true`, and it accepts `0` and negative values besides. - # GitHub requires a positive integer. No upper bound is asserted: 360 is - # the GitHub-hosted execution limit, not a repository-wide job ceiling. - assert type(timeout) is int, ( - f"{workflow_name}:{job_name} must declare an integer timeout-minutes, " - f"got {timeout!r}; note YAML's `true` is an int to `isinstance`" - ) - assert timeout > 0, ( - f"{workflow_name}:{job_name} must declare a positive timeout-minutes, " - f"got {timeout!r}" - ) + # The rule lives in `ceiling`, which `tests/test_ci_placement_guards.py` + # drives directly over the values this repository does not happen to + # declare. No upper bound: 360 is the GitHub-hosted execution limit, not a + # repository-wide job ceiling. + ceiling(workflow_name, job_name) @pytest.mark.parametrize(("workflow_name", "job_name"), CALLER_CASES) diff --git a/tests/test_ci_tool_installation.py b/tests/test_ci_tool_installation.py index 4339f5460..52c16f6b2 100644 --- a/tests/test_ci_tool_installation.py +++ b/tests/test_ci_tool_installation.py @@ -11,6 +11,8 @@ import re import typing as typ +import pytest + from tests.helpers.ci_placement import declares_steps from tests.helpers.ci_runners import ( GENERATE_COVERAGE, @@ -146,3 +148,31 @@ def test_markdown_lint_runs_through_the_pinned_action() -> None: assert "markdownlint-cli2" not in script, ( "ci.yml:Install CLI tools must not install markdownlint-cli2 from npm" ) + + +@pytest.mark.parametrize( + ("uses", "reported"), + [ + pytest.param(GENERATE_COVERAGE, False, id="the-sanctioned-action"), + pytest.param( + "leynos/shared-actions/.github/actions/setup-rust@" + "0" * 40, + True, + id="a-sibling-shared-action", + ), + pytest.param("actions/checkout@" + "0" * 40, True, id="a-third-party-action"), + ], +) +def test_only_the_coverage_action_may_ask_for_nextest( + uses: str, *, reported: bool +) -> None: + """Restrict the exemption to the one action that installs nextest on purpose. + + Driven directly because no workflow here gives a sibling action a nextest + input, so a contract read over the real files passes whether the exemption + is exact or spans the whole namespace. Widening it back to a prefix + survived every test until this one existed. + """ + step = typ.cast("Step", {"uses": uses, "with": {"tool": "cargo-nextest@0.9.0"}}) + assert _installs_nextest_by_input(step) is reported, ( + f"{uses} must {'be' if reported else 'not be'} reported as installing nextest" + ) From 7fe07afd45bb73e1e73b9e575e7009caf8bece6c Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Tue, 22 Sep 2026 08:08:52 +0100 Subject: [PATCH 5/7] Honour a job's own event guard in the schedule Three review threads on the behavioural harness, and one defect the mutations found behind them. `schedule()` appended every step-declaring job without reading its job-level `if:`, so the push scenario reported `ci.yml:coverage` as scheduled although its guard admits pull requests only. It now evaluates `github.event_name` and `github.ref` comparisons, joined by `&&` and `||`. Every other term is unknown and admits the job: this harness models the event, not `needs` outcomes or dispatch inputs, and a schedule that is a superset of what runs never claims a job is absent when it is present. Removing that evaluation passed every scenario, because a wrongly scheduled job still declares the right label and only a label is asserted. Two scenarios now assert the lane's absence on a push and its presence on a pull request, which is what makes the fix falsifiable. Driving the predicate directly then caught more. No workflow reachable from the scenarios uses a `!=` guard, so inverting the comparison passed everything; `tests/test_ci_event_conditions.py` covers both operators, both modelled fields, conjunction, alternation and the unmodelled operands. And an explicit parenthesis refusal could not change any outcome, because the terms such an expression splits into fail the comparison pattern anyway: it is removed rather than left looking protective. The reviewed-lane step counted no lanes when a schedule contained none, which reads exactly like a pass; it now asserts the count. The workflow filename comes from `PurePosixPath` rather than `rsplit`. --- tests/behaviour/test_ci_runner_selection.py | 59 ++++++++- tests/features/ci_runner_selection.feature | 10 ++ tests/helpers/ci_events.py | 89 +++++++++++++- tests/test_ci_event_conditions.py | 129 ++++++++++++++++++++ 4 files changed, 280 insertions(+), 7 deletions(-) create mode 100644 tests/test_ci_event_conditions.py diff --git a/tests/behaviour/test_ci_runner_selection.py b/tests/behaviour/test_ci_runner_selection.py index 48b628033..694a1dd46 100644 --- a/tests/behaviour/test_ci_runner_selection.py +++ b/tests/behaviour/test_ci_runner_selection.py @@ -48,6 +48,14 @@ "ubuntu-latest", "windows-2022", }) +#: The fewest reviewed lanes any event asserting that step schedules. Named +#: rather than derived: a derivation from the same schedule the assertion +#: reads would move with the defect it is meant to catch. +MINIMUM_REVIEWED_LANES: typ.Final = 4 +#: The lane whose own guard admits one event only. Its presence and absence +#: are what make the event evaluation falsifiable: every other assertion here +#: reads a label, and a wrongly scheduled job declares the right one. +PULL_REQUEST_ONLY_LANE: typ.Final = ("ci.yml", "coverage") EVENTS: typ.Final = { "fork": FORK_PULL_REQUEST, "owned": OWNED_PULL_REQUEST, @@ -129,17 +137,29 @@ def _scheduled(selection: dict[str, object]) -> list[Scheduled]: @then("every reviewed lane selects the Ubicloud runner") def _reviewed_lanes_are_paid(selection: dict[str, object]) -> None: - """Assert each reviewed lane took the Ubicloud arm.""" + """Assert each reviewed lane took the Ubicloud arm. + + The `continue` below is a vacuity hole on its own: a schedule containing no + reviewed lane at all would take it every time, check nothing, and read + exactly like a pass. The checked count closes it. + """ resolved = { (item.workflow, item.job): item.labels for item in _scheduled(selection) } + checked = 0 for workflow_name, job_name in expand(UBICLOUD_JOBS): if (workflow_name, job_name) not in resolved: continue + checked += 1 assert resolved[workflow_name, job_name] == (UBICLOUD_LABEL,), ( f"{workflow_name}:{job_name} must select {UBICLOUD_LABEL} on this " f"event, got {resolved[workflow_name, job_name]}" ) + assert checked >= MINIMUM_REVIEWED_LANES, ( + f"this event scheduled only {checked} reviewed lanes, fewer than the " + f"{MINIMUM_REVIEWED_LANES} every event asserting this step places; the " + "loop above would have checked nothing and still passed" + ) @then("the wheel jobs reached through the called workflow select it too") @@ -189,6 +209,33 @@ def _fork_lanes_fall_back(selection: dict[str, object]) -> None: ) +@then("the pull-request-only lane is absent") +def _pull_request_lane_absent(selection: dict[str, object]) -> None: + """Assert a push does not schedule the lane its own guard excludes. + + `ci.yml:coverage` declares `github.event_name == 'pull_request'`, so GitHub + skips it on a push. A schedule that listed it anyway would report the push + event placing a job the push event never runs, and every placement + assertion over that schedule would still pass, because the label it + declares is correct. Only its absence discriminates. + """ + scheduled = {(item.workflow, item.job) for item in _scheduled(selection)} + assert PULL_REQUEST_ONLY_LANE not in scheduled, ( + f"{PULL_REQUEST_ONLY_LANE} is guarded on the pull_request event, so a " + "push must not schedule it" + ) + + +@then("the pull-request-only lane is present") +def _pull_request_lane_present(selection: dict[str, object]) -> None: + """Prove the exclusion narrow: the event it is written for still runs it.""" + scheduled = {(item.workflow, item.job) for item in _scheduled(selection)} + assert PULL_REQUEST_ONLY_LANE in scheduled, ( + f"{PULL_REQUEST_ONLY_LANE} must run on a pull request; excluding it " + "here would make the absence assertion above hold for the wrong reason" + ) + + @then("the native wheel matrix keeps its platform runners") def _native_matrix_is_hosted(selection: dict[str, object]) -> None: """Assert the native matrix kept exactly its platform runners.""" @@ -234,6 +281,16 @@ def test_a_push_takes_the_owned_arm() -> None: """With no pull-request payload the fork field is absent, so falsy.""" +@scenario(FEATURE, "a push does not schedule the pull-request-only lane") +def test_a_push_skips_the_pull_request_only_lane() -> None: + """A job's own event guard is honoured by the schedule.""" + + +@scenario(FEATURE, "a pull request does schedule the pull-request-only lane") +def test_a_pull_request_runs_the_pull_request_only_lane() -> None: + """The same guard admits the event it is written for.""" + + @scenario(FEATURE, "a tag push reaches the wheel jobs on the paid runner") def test_a_tag_push_reaches_the_wheel_jobs() -> None: """The release path resolves through the same called workflow.""" diff --git a/tests/features/ci_runner_selection.feature b/tests/features/ci_runner_selection.feature index aede6b827..144112910 100644 --- a/tests/features/ci_runner_selection.feature +++ b/tests/features/ci_runner_selection.feature @@ -25,6 +25,16 @@ Feature: CI runner selection by event When a commit is pushed to the default branch Then every reviewed lane selects the Ubicloud runner + Scenario: a push does not schedule the pull-request-only lane + Given the continuous integration workflow + When a commit is pushed to the default branch + Then the pull-request-only lane is absent + + Scenario: a pull request does schedule the pull-request-only lane + Given the continuous integration workflow + When a pull request is opened from a branch of this repository + Then the pull-request-only lane is present + Scenario: a tag push reaches the wheel jobs on the paid runner Given the release workflow When a tag is pushed diff --git a/tests/helpers/ci_events.py b/tests/helpers/ci_events.py index ca7bfd113..2d35a809e 100644 --- a/tests/helpers/ci_events.py +++ b/tests/helpers/ci_events.py @@ -16,6 +16,8 @@ from __future__ import annotations +import pathlib as pth +import re import typing as typ from tests.helpers.ci_placement import declares_steps, placement @@ -25,14 +27,83 @@ import collections.abc as cabc #: A pull request opened from a fork of this repository. -FORK_PULL_REQUEST: typ.Final = {"event_name": "pull_request", "fork": True} +FORK_PULL_REQUEST: typ.Final = { + "event_name": "pull_request", + "fork": True, + "ref": "refs/pull/1/merge", +} #: A pull request opened from a branch of this repository. -OWNED_PULL_REQUEST: typ.Final = {"event_name": "pull_request", "fork": False} +OWNED_PULL_REQUEST: typ.Final = { + "event_name": "pull_request", + "fork": False, + "ref": "refs/pull/1/merge", +} #: A push to the default branch. There is no pull-request payload, so the fork #: field is absent, which GitHub evaluates as falsy. -PUSH_TO_MAIN: typ.Final = {"event_name": "push", "fork": None} +PUSH_TO_MAIN: typ.Final = { + "event_name": "push", + "fork": None, + "ref": "refs/heads/main", +} #: A tag push, which is what reaches `release.yml`. -TAG_PUSH: typ.Final = {"event_name": "push", "fork": None} +TAG_PUSH: typ.Final = {"event_name": "push", "fork": None, "ref": "refs/tags/v1.2.3"} + +#: A comparison this evaluator can decide: one context property this module +#: models, against a quoted literal. +_COMPARISON = re.compile( + r"\A\s*github\.(?Pevent_name|ref)\s*(?P==|!=)\s*" + r"'(?P[^']*)'\s*\Z" +) +#: Everything else in a condition is a run-time value this harness does not +#: model: `needs.*` outcomes, `inputs.*`, `github.actor`. Treating them as true +#: keeps the schedule a superset of what runs, which is the safe direction for +#: a placement assertion: it never claims a job is absent when it runs. +_UNKNOWN: typ.Final = True + + +def _decide(term: str, event: cabc.Mapping[str, object]) -> bool: + """Evaluate one `&&`-joined term against the event.""" + match = _COMPARISON.match(term) + if match is None: + return _UNKNOWN + actual = event.get(match.group("field")) + equal = actual == match.group("literal") + return equal if match.group("operator") == "==" else not equal + + +def runs_on_event(condition: object, event: cabc.Mapping[str, object]) -> bool: + """Report whether a job's own ``if:`` admits this event. + + GitHub skips `ci.yml:coverage` on a push, because it declares + `github.event_name == 'pull_request'`. A schedule that listed it anyway + would say the push event places a job the push event never runs. + + Only `github.event_name` and `github.ref` comparisons are decided. Every + other term is left to :data:`_UNKNOWN`, including any term a parenthesized + expression splits into, since none of those match the comparison pattern + either. An explicit parenthesis check stood here until a mutation showed it + could not change an outcome: the unknown-term path already covers it, and + an unreachable guard is worse than none because it reads as protection. + + This harness models the event, not `needs` outcomes or dispatch inputs, so + a schedule that is a superset of what runs never claims a job is absent + when it is present. + + Returns + ------- + bool + Whether the event admits the job. + """ + if not isinstance(condition, str): + return True + text = " ".join(condition.split()) + expression = re.fullmatch(r"\$\{\{(?P.*)\}\}", text, re.DOTALL) + if expression is not None: + text = " ".join(expression.group("body").split()) + return any( + all(_decide(term, event) for term in alternative.split("&&")) + for alternative in text.split("||") + ) class Scheduled(typ.NamedTuple): @@ -97,6 +168,9 @@ def schedule(workflow_name: str, event: cabc.Mapping[str, object]) -> list[Sched """ scheduled: list[Scheduled] = [] for job_name in jobs(workflow_name): + declared = job(workflow_name, job_name) + if not runs_on_event(declared.get("if"), event): + continue if declares_steps(workflow_name, job_name): scheduled.append( Scheduled( @@ -104,10 +178,13 @@ def schedule(workflow_name: str, event: cabc.Mapping[str, object]) -> list[Sched ) ) continue - called = job(workflow_name, job_name).get("uses") + called = declared.get("uses") if not isinstance(called, str) or not called.startswith("./.github/workflows/"): # A caller of a workflow outside this repository declares no runner # this harness can resolve, and its callee is not ours to model. continue - scheduled.extend(schedule(called.rsplit("/", 1)[-1], event)) + # `PurePosixPath` rather than `rsplit`: a workflow reference is a POSIX + # path and the separator is part of its grammar, not a character to + # split on. + scheduled.extend(schedule(pth.PurePosixPath(called).name, event)) return scheduled diff --git a/tests/test_ci_event_conditions.py b/tests/test_ci_event_conditions.py new file mode 100644 index 000000000..9b944f454 --- /dev/null +++ b/tests/test_ci_event_conditions.py @@ -0,0 +1,129 @@ +"""Tests for the event-condition evaluator behind the runner-selection scenarios. + +`tests/behaviour/test_ci_runner_selection.py` exercises the evaluator through +this repository's workflows, which between them use one `==` guard on an event +name and little else. That left the `!=` branch unexercised: inverting the +comparison passed every scenario, because no scheduled workflow depends on it. + +The predicate is the mechanism, so it is driven here directly, over both +operators, both modelled fields, the `${{ }}` wrapper, and the operands this +harness deliberately declines to model. +""" + +from __future__ import annotations + +import pytest + +from tests.helpers.ci_events import ( + OWNED_PULL_REQUEST, + PUSH_TO_MAIN, + TAG_PUSH, + runs_on_event, +) + + +@pytest.mark.parametrize( + ("condition", "event", "admitted"), + [ + pytest.param( + "github.event_name == 'pull_request'", + OWNED_PULL_REQUEST, + True, + id="equal-match", + ), + pytest.param( + "github.event_name == 'pull_request'", + PUSH_TO_MAIN, + False, + id="equal-mismatch", + ), + # The case the scenarios could not reach. `rust-boundaries.yml`'s + # `extended` job is guarded exactly this way, and inverting the + # operator passed every scenario until this table existed. + pytest.param( + "github.event_name != 'pull_request'", + PUSH_TO_MAIN, + True, + id="not-equal-match", + ), + pytest.param( + "github.event_name != 'pull_request'", + OWNED_PULL_REQUEST, + False, + id="not-equal-mismatch", + ), + pytest.param( + "github.ref == 'refs/heads/main'", PUSH_TO_MAIN, True, id="ref-match" + ), + pytest.param( + "github.ref == 'refs/heads/main'", TAG_PUSH, False, id="ref-mismatch" + ), + pytest.param( + "${{ github.event_name == 'push' }}", PUSH_TO_MAIN, True, id="wrapped" + ), + pytest.param( + "github.event_name == 'schedule' || github.event_name == 'push'", + PUSH_TO_MAIN, + True, + id="alternative-admits", + ), + pytest.param( + "github.event_name == 'schedule' || " + "github.event_name == 'workflow_dispatch'", + PUSH_TO_MAIN, + False, + id="no-alternative-admits", + ), + pytest.param( + "github.event_name == 'push' && github.ref == 'refs/heads/main'", + PUSH_TO_MAIN, + True, + id="conjunction-admits", + ), + pytest.param( + "github.event_name == 'push' && github.ref == 'refs/heads/main'", + TAG_PUSH, + False, + id="conjunction-refuses", + ), + ], +) +def test_event_conditions_are_evaluated( + condition: str, event: dict[str, object], *, admitted: bool +) -> None: + """Decide the comparisons this harness models, on both operators.""" + assert runs_on_event(condition, event) is admitted, ( + f"{condition!r} against {event['event_name']!r} must be " + f"{'admitted' if admitted else 'refused'}" + ) + + +@pytest.mark.parametrize( + "condition", + [ + pytest.param("needs.changes.outputs.bench == 'true'", id="needs-output"), + pytest.param("inputs.workflow-harness", id="dispatch-input"), + pytest.param("github.actor == 'dependabot[bot]'", id="actor"), + pytest.param( + "needs.changes.result == 'success' && (github.event_name != " + "'pull_request' || needs.changes.outputs.bench == 'true')", + id="parenthesized", + ), + pytest.param(None, id="no-condition"), + pytest.param(True, id="non-string"), + ], +) +def test_unmodelled_conditions_admit_the_job(condition: object) -> None: + """Admit a job whose guard this harness does not model. + + `needs` outcomes, dispatch inputs and the actor are run-time values. A + parenthesized expression reaches the same place by a different route: the + terms it splits into do not match the comparison pattern either, so no + special case is needed and none exists. Every one of them leaves the + schedule a superset of what actually runs, which is the safe direction + here: it can overstate what an event places, but it never claims a job is + absent when it runs, and absence is what the scenarios assert on. + """ + assert runs_on_event(condition, PUSH_TO_MAIN), ( + f"{condition!r} is not modelled, so the job must still be scheduled" + ) From fbadf7ef58d61fb4f437d1bea2588367435ac8ef Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 23 Sep 2026 03:40:04 +0100 Subject: [PATCH 6/7] Name the file when a workflow cannot be read The review's architecture row found that every placement, event and cache query reaches the filesystem through the shared loader, where `Path.read_text()` and `yaml.safe_load()` raise unattributed errors from whichever query touched the file first. Give the loader one failure boundary. `read_source` and `read_workflow` translate an `OSError`, a `YAMLError`, and a document that is not a mapping into an `AssertionError` naming the file, and `workflow_sources` reads through the same boundary, taking the directory as a parameter so the sweep can be driven over a temporary one. The new loader tests use real files: a missing file, a directory in place of a file, genuinely invalid YAML, a list document, and the sweep. Each translation was removed in turn and its test failed. The queries keep taking workflow and job names rather than an injected reader. Their only input is the repository's committed workflows, and the reader tests already substitute synthetic declarations at the module seam; the developers' guide records the reasoning. --- docs/developers-guide.md | 11 ++++ tests/helpers/ci_workflows.py | 101 +++++++++++++++++++++++++++---- tests/test_ci_workflow_loader.py | 83 +++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 11 deletions(-) create mode 100644 tests/test_ci_workflow_loader.py diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4b5c609b3..4b7a0ebf3 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -191,6 +191,17 @@ a paid runner. `tests/test_ci_placement_reader.py` drives the reader with synthetic declarations, because a rule parametrized over this repository's own correct workflows passes whether or not it discriminates. +Reading is fallible, and the failure has one boundary. Every query reaches the +filesystem through `read_source` and `read_workflow` in +`tests/helpers/ci_workflows.py`, which translate an I/O error, invalid YAML, or +a document that is not a mapping into an `AssertionError` naming the file. +`tests/test_ci_workflow_loader.py` drives both against real files in a +temporary directory. The queries take workflow and job names rather than an +injected reader: their only input is this repository's committed workflows, and +the reader tests already substitute a synthetic declaration at the module seam, +so the placement logic is exercised over plain mappings without a reader +threaded through every contract. + Three layers sit above it, each answering a question the layer below cannot: - `tests/test_ci_placement_guards.py` covers whether a job runs and which diff --git a/tests/helpers/ci_workflows.py b/tests/helpers/ci_workflows.py index 2d771ff55..c301303c1 100644 --- a/tests/helpers/ci_workflows.py +++ b/tests/helpers/ci_workflows.py @@ -4,6 +4,13 @@ ``tests/helpers/ci_runners.py``; this module only reads the workflows back. Every accessor validates the shape it narrows, so a malformed workflow fails with a named diagnostic rather than an opaque ``TypeError`` deep in a test. + +Reading and parsing are fallible too, and every query in the contract helpers +reaches the filesystem through the two readers here, ``read_workflow`` and +``read_source``. They translate an I/O or YAML failure into an +``AssertionError`` naming the file, so a contract over an unreadable workflow +fails as a contract, citing the file, rather than raising an unattributed +parser error from whichever query happened to touch it first. """ from __future__ import annotations @@ -48,22 +55,83 @@ def _mapping(value: object, message: str) -> dict[str, object]: return typ.cast("dict[str, object]", value) -def workflow_document(workflow_name: str) -> dict[object, object]: - """Parse one repository workflow, without narrowing its top-level keys.""" - path = WORKFLOW_DIR / workflow_name +def read_source(path: Path) -> str: + """Return one workflow's source text, naming the file if it cannot be read. + + Parameters + ---------- + path : Path + The workflow file to read. + + Returns + ------- + str + The file's UTF-8 text. + + Raises + ------ + AssertionError + If the file cannot be read, naming the file and the ``OSError``. + + Examples + -------- + >>> "jobs:" in read_source(WORKFLOW_DIR / "ci.yml") + True + """ + try: + return path.read_text(encoding="utf-8") + except OSError as error: + message = f"{path.name} could not be read: {error}" + raise AssertionError(message) from error + + +def read_workflow(path: Path) -> dict[object, object]: + """Parse one workflow file, without narrowing its top-level keys. + + Parameters + ---------- + path : Path + The workflow file to parse. + + Returns + ------- + dict[object, object] + The parsed document, whose trigger key YAML 1.1 reads as ``True``. + + Raises + ------ + AssertionError + If the file cannot be read, is not valid YAML, or does not parse to a + mapping, naming the file in each case. + + Examples + -------- + >>> "jobs" in read_workflow(WORKFLOW_DIR / "ci.yml") + True + """ + source = read_source(path) + try: + document = yaml.safe_load(source) + except yaml.YAMLError as error: + message = f"{path.name} is not valid YAML: {error}" + raise AssertionError(message) from error # YAML 1.1 reads the `on:` trigger key as the boolean `True`, so the # document is genuinely not string-keyed and the return type says so. # Claiming `dict[str, object]` here would be a false contract that hides # the one key a caller cannot reach by name. Callers that need a # string-keyed mapping narrow one of its values through `_mapping`. - document = yaml.safe_load(path.read_text(encoding="utf-8")) _require( condition=isinstance(document, dict), - message=f"{workflow_name} must parse to a mapping", + message=f"{path.name} must parse to a mapping", ) return typ.cast("dict[object, object]", document) +def workflow_document(workflow_name: str) -> dict[object, object]: + """Parse one repository workflow, without narrowing its top-level keys.""" + return read_workflow(WORKFLOW_DIR / workflow_name) + + def workflow_env(workflow_name: str) -> dict[str, object]: """Return the workflow-level ``env`` mapping.""" return _mapping( @@ -259,9 +327,20 @@ def expand(manifest: cabc.Mapping[str, tuple[str, ...]]) -> list[tuple[str, str] ] -def workflow_sources() -> list[tuple[str, str]]: - """Return every workflow's name and source text.""" - return [ - (path.name, path.read_text(encoding="utf-8")) - for path in sorted(WORKFLOW_DIR.glob("*.yml")) - ] +def workflow_sources(directory: Path = WORKFLOW_DIR) -> list[tuple[str, str]]: + """Return every workflow's name and source text. + + Parameters + ---------- + directory : Path + The directory to sweep. It defaults to this repository's workflows; + the loader tests pass a temporary one to show that an unreadable file + fails by name. + + Returns + ------- + list[tuple[str, str]] + Each ``*.yml`` file's name and text, sorted by path. Every read goes + through ``read_source``, so an unreadable file fails by name. + """ + return [(path.name, read_source(path)) for path in sorted(directory.glob("*.yml"))] diff --git a/tests/test_ci_workflow_loader.py b/tests/test_ci_workflow_loader.py new file mode 100644 index 000000000..fe0ff8a74 --- /dev/null +++ b/tests/test_ci_workflow_loader.py @@ -0,0 +1,83 @@ +"""Tests for the workflow loader's failure boundary. + +Every placement, event and cache query reaches the filesystem through +`read_source` and `read_workflow`. These drive both with real files under a +temporary directory, because this repository's own workflows are readable and +valid, so a contract parametrized over them cannot show what a read or parse +failure looks like. +""" + +from __future__ import annotations + +import typing as typ + +import pytest + +from tests.helpers.ci_workflows import read_source, read_workflow, workflow_sources + +if typ.TYPE_CHECKING: + from pathlib import Path + + +def test_a_missing_workflow_fails_naming_the_file(tmp_path: Path) -> None: + """A workflow that does not exist is a contract failure citing its name.""" + with pytest.raises(AssertionError, match=r"absent\.yml could not be read"): + read_workflow(tmp_path / "absent.yml") + + +def test_an_unreadable_source_fails_naming_the_file(tmp_path: Path) -> None: + """A path that cannot be read as text fails as a contract, not an OSError. + + A directory stands in for an unreadable file: reading it raises an + ``OSError`` on every platform, without depending on permission bits the + test process may be privileged enough to ignore. + """ + unreadable = tmp_path / "unreadable.yml" + unreadable.mkdir() + with pytest.raises(AssertionError, match=r"unreadable\.yml could not be read"): + read_source(unreadable) + + +def test_invalid_yaml_fails_naming_the_file(tmp_path: Path) -> None: + """A parser error is translated and attributed to the file that caused it. + + The case is genuinely unparsable. A document that parses but has the + wrong shape exercises only the mapping check, which is covered separately. + """ + broken = tmp_path / "broken.yml" + broken.write_text("jobs: [unclosed\n", encoding="utf-8") + with pytest.raises(AssertionError, match=r"broken\.yml is not valid YAML"): + read_workflow(broken) + + +def test_a_document_that_is_not_a_mapping_fails_naming_the_file( + tmp_path: Path, +) -> None: + """Valid YAML of the wrong shape is refused with the file's name.""" + listed = tmp_path / "listed.yml" + listed.write_text("- not a mapping\n", encoding="utf-8") + with pytest.raises(AssertionError, match=r"listed\.yml must parse to a mapping"): + read_workflow(listed) + + +def test_a_valid_workflow_keeps_its_boolean_trigger_key(tmp_path: Path) -> None: + """A readable workflow parses unnarrowed, with YAML 1.1's `on` as `True`.""" + valid = tmp_path / "valid.yml" + valid.write_text("on: push\njobs: {}\n", encoding="utf-8") + parsed = read_workflow(valid) + assert parsed == {True: "push", "jobs": {}}, ( + f"valid.yml should parse unnarrowed, got {parsed!r}" + ) + + +def test_the_source_sweep_reads_through_the_named_boundary(tmp_path: Path) -> None: + """The sweep over every workflow fails by name, not with a bare OSError. + + `workflow_sources` feeds the text-level contracts, so it must use the same + boundary as the parsed readers. A directory matching the glob stands in + for a workflow that cannot be read. + """ + (tmp_path / "ci.yml").write_text("on: push\n", encoding="utf-8") + (tmp_path / "stuck.yml").mkdir() + with pytest.raises(AssertionError, match=r"stuck\.yml could not be read"): + workflow_sources(tmp_path) From 456fb6fcf908d5af6904e38fcb8eb86e2a7ac988 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 23 Sep 2026 11:29:31 +0100 Subject: [PATCH 7/7] Answer the third placement review round - Read an unquoted `if: false`, `if: 0` or `if: 0.0` as a job that never runs. YAML types them before GitHub sees them, so the commonest way to switch a job off reached the string path as a bool and passed. - Merge a direct matrix list with the values its `include` legs add. An `include` leg with a new value for the key runs on that label, and the reader returned before reading it. - Refuse any interpolation in a matrix value, not only a wholly expression value: `ubuntu-${{ inputs.release }}` runs on a label nobody wrote down. - Fail the workflow sweep on a missing, non-directory, or empty directory. `Path.glob` returns nothing for a missing directory, which every "no workflow does X" contract would read as a clean estate. - Delete the reader tests' unused copy of the workflow list. - Scope the `ci.yml` and guide claims about saves to the key families, name the Loom family as the one `github-hosted` family still written, and add two serial commas. Removing each fix fails its new test cases. --- .github/workflows/ci.yml | 13 +++++--- docs/developers-guide.md | 19 ++++++----- tests/helpers/ci_job_rules.py | 5 ++- tests/helpers/ci_placement.py | 16 +++++++-- tests/helpers/ci_workflows.py | 19 ++++++++++- tests/test_ci_placement_guards.py | 11 +++++-- tests/test_ci_placement_reader.py | 55 +++++++++++++++++++++---------- tests/test_ci_workflow_loader.py | 22 +++++++++++++ 8 files changed, 123 insertions(+), 37 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2fe6ed42..f340914c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,11 +102,14 @@ jobs: ubuntu-release: ${{ env.UBUNTU_RELEASE }} compiler-shape: lint - # Every key carries `runner.environment`, so the owned arm and the fork - # arm cannot restore each other's entries even though they share a family - # name. Nothing writes the fork arm's scope: every save below requires a - # push to `refs/heads/main`, which a fork can never produce, so a fork's - # pull request restores nothing and compiles cold. + # Every `CARGO_CACHE_KEY` and `SCCACHE_CACHE_KEY` family carries + # `runner.environment`, so the owned arm and the fork arm cannot restore + # each other's entries even though they share a family name. Nothing + # writes the fork arm's scope of those two families: their saves require + # a push to `refs/heads/main`, which a fork can never produce, so a + # fork's pull request restores nothing from them and compiles cold. The + # plain `actions/cache` tool caches further down are not key families: + # they save on every event under GitHub's own branch scoping. - name: Restore the Cargo registry id: cargo-cache uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4b7a0ebf3..57a54280c 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -66,10 +66,10 @@ Table 1: GitHub Actions jobs, workflows, and runners | `native` | `rust-boundaries.yml` | `${{ matrix.os }}` | none | 20 | The hosted rows are hosted for a reason, not by omission. `changes` and -`loom-smoke` are cheap gate jobs; `workflow-harness`, `loom` and `extended` run -only on a schedule or a dispatch; and `rust-boundaries.yml`'s verifier lanes -stay on GitHub-hosted Linux by this repository's own decision. Placing `verus` -or `loom-smoke` on the paid lane is a separate question needing its own +`loom-smoke` are cheap gate jobs; `workflow-harness`, `loom`, and `extended` +run only on a schedule or a dispatch; and `rust-boundaries.yml`'s verifier +lanes stay on GitHub-hosted Linux by this repository's own decision. Placing +`verus` or `loom-smoke` on the paid lane is a separate question needing its own measurements. `ubicloud-standard-2` (2 vCPU, 8 GB, Ubuntu 24.04 amd64) is the default shape @@ -88,7 +88,7 @@ neither a fixed nor a larger shape. `lint-test` moved onto the paid lane. It is a developer-blocking gate that compiles real work: Whitaker, clippy under two toolchains, a Windows -cross-target check and Nixie. Six runs measured it at 198 to 292 seconds on +cross-target check, and Nixie. Six runs measured it at 198 to 292 seconds on four GitHub-hosted vCPU, with a queue that was two to three seconds five times and 154 seconds once; the same run queued every hosted lane 77 to 164 seconds while its Ubicloud neighbours queued 18 to 37. The case is that tail. Ubicloud @@ -136,9 +136,12 @@ of which a fork can cause. The fork arm runs cold, and that is the accepted price. Every cache key carries `runner.environment`, which renders `github-hosted` on the fork arm and `self-hosted` on the owned one, so the two arms read different scopes. No -writer changes: every save step is guarded on a push to `refs/heads/main`, -which is never a fork, so the fork arm restores and never publishes. Since -`lint-test` moved, no job writes a `github-hosted` family at all. +writer changes: every key-family save step is guarded on a push to +`refs/heads/main`, which is never a fork, so the fork arm restores and never +publishes. Since `lint-test` moved, the one `github-hosted` family still +written is the Loom model family, which `loom.yml` saves only on its scheduled +runs on `main`; `loom-smoke` restores it on a pull request, a fork's included, +and never saves it. `lint-test` therefore no longer saves the Cargo registry. On its owned arm it renders the same `self-hosted` family `extension-tests` writes, and one writer diff --git a/tests/helpers/ci_job_rules.py b/tests/helpers/ci_job_rules.py index 2f977e00b..0333b3aba 100644 --- a/tests/helpers/ci_job_rules.py +++ b/tests/helpers/ci_job_rules.py @@ -114,8 +114,11 @@ def never_runs(workflow_name: str, job_name: str) -> bool: Whether the job's own ``if:`` reduces to a constant false. """ condition = job(workflow_name, job_name).get("if") + # YAML types an unquoted `if: false` or `if: 0` before GitHub sees it, so + # the commonest way to switch a job off arrives as a bool or a number, + # not as text. Its truthiness is what GitHub evaluates. if not isinstance(condition, str): - return False + return isinstance(condition, (bool, int, float)) and not condition text = " ".join(condition.split()) expression = EXPRESSION.match(text) if expression is not None: diff --git a/tests/helpers/ci_placement.py b/tests/helpers/ci_placement.py index efb9ef6fe..a58a9fb43 100644 --- a/tests/helpers/ci_placement.py +++ b/tests/helpers/ci_placement.py @@ -191,7 +191,17 @@ def _matrix_values(workflow_name: str, job_name: str, key: str) -> list[object]: # perfectly ordinary workflow. direct = declared.get(key) if isinstance(direct, list): - return list(typ.cast("list[object]", direct)) + # `include` can still add a leg with a new value for the key, and that + # leg runs too. An entry without the key only extends existing legs. + extra = declared.get("include", []) + require( + condition=isinstance(extra, list) + and all(isinstance(leg, dict) for leg in extra), + message=f"{workflow_name}:{job_name} declares an unreadable include", + ) + return list(typ.cast("list[object]", direct)) + [ + leg[key] for leg in typ.cast("list[dict[str, object]]", extra) if key in leg + ] include = declared.get("include") require( condition=isinstance(include, list), @@ -222,7 +232,9 @@ def _literal_labels( labels: set[str] = set() for value in values: require( - condition=isinstance(value, str) and EXPRESSION.match(value) is None, + # Any interpolation, not only a wholly-expression value: + # `ubuntu-${{ inputs.release }}` runs on a label nobody wrote down. + condition=isinstance(value, str) and _INTERPOLATION.search(value) is None, message=( f"{workflow_name}:{job_name} resolves its runner to {value!r}; " "a matrix value holding an expression is a placement this " diff --git a/tests/helpers/ci_workflows.py b/tests/helpers/ci_workflows.py index c301303c1..5ebe04b95 100644 --- a/tests/helpers/ci_workflows.py +++ b/tests/helpers/ci_workflows.py @@ -342,5 +342,22 @@ def workflow_sources(directory: Path = WORKFLOW_DIR) -> list[tuple[str, str]]: list[tuple[str, str]] Each ``*.yml`` file's name and text, sorted by path. Every read goes through ``read_source``, so an unreadable file fails by name. + + Raises + ------ + AssertionError + If ``directory`` is not a readable directory or holds no workflow. + ``Path.glob`` returns nothing for a missing directory, and an empty + sweep would satisfy every "no workflow does X" contract vacuously. """ - return [(path.name, read_source(path)) for path in sorted(directory.glob("*.yml"))] + _require( + condition=directory.is_dir(), + message=f"{directory} is not a workflow directory", + ) + try: + paths = sorted(directory.glob("*.yml")) + except OSError as error: + message = f"{directory} could not be listed: {error}" + raise AssertionError(message) from error + _require(condition=bool(paths), message=f"{directory} holds no workflow") + return [(path.name, read_source(path)) for path in paths] diff --git a/tests/test_ci_placement_guards.py b/tests/test_ci_placement_guards.py index f9901f240..33c74cba8 100644 --- a/tests/test_ci_placement_guards.py +++ b/tests/test_ci_placement_guards.py @@ -60,10 +60,15 @@ def _declare(monkeypatch: pytest.MonkeyPatch, declared: dict[str, object]) -> No "false && matrix.target == 'x'", "null && matrix.target == 'x'", "0 && matrix.target == 'x'", + # YAML types these before GitHub sees them: an unquoted `if: false` + # arrives as a bool, and `if: 0` as a number. + False, + 0, + 0.0, ], ) def test_a_constant_false_guard_is_reported( - monkeypatch: pytest.MonkeyPatch, condition: str + monkeypatch: pytest.MonkeyPatch, condition: object ) -> None: """Catch a lane that satisfies every declaration rule and runs nothing.""" _declare(monkeypatch, {"runs-on": HOSTED, "steps": [], "if": condition}) @@ -86,10 +91,12 @@ def test_a_constant_false_guard_is_reported( '"false"', "falsey", "false_positive_guard", + True, + 1, ], ) def test_a_real_guard_is_not_reported_as_never_running( - monkeypatch: pytest.MonkeyPatch, condition: str + monkeypatch: pytest.MonkeyPatch, condition: object ) -> None: """Prove the rule narrow: a legitimate condition must still pass. diff --git a/tests/test_ci_placement_reader.py b/tests/test_ci_placement_reader.py index bad0a1175..b44e87028 100644 --- a/tests/test_ci_placement_reader.py +++ b/tests/test_ci_placement_reader.py @@ -19,21 +19,6 @@ from tests.helpers import ci_placement as reader -#: Every workflow the traversal must reach. Named rather than derived, so the -#: claim that `all_jobs` sees the whole estate is asserted rather than merely -#: true: a file the reader skipped would otherwise be invisible. -ESTATE_WORKFLOWS = frozenset({ - "benchmark-gate-harness.yml", - "build-wheels.yml", - "ci.yml", - "coverage-main.yml", - "delayed-pr-comment.yml", - "dependabot-automerge.yml", - "loom.yml", - "mutation-testing.yml", - "release.yml", - "rust-boundaries.yml", -}) OWNED = "ubicloud-standard-2" HOSTED = "ubuntu-latest" FORK_EXPRESSION = f"${{{{ {reader.FORK_FIELD} && '{HOSTED}' || '{OWNED}' }}}}" @@ -161,22 +146,56 @@ def test_a_matrix_placement_resolves_through_its_include( ) -def test_a_matrix_value_holding_an_expression_is_refused( +def test_a_direct_matrix_list_also_reads_its_include_legs( monkeypatch: pytest.MonkeyPatch, +) -> None: + """An `include` leg with a new value for the key runs on that label too. + + An `include` entry without the key only extends the existing legs, so it + is read past rather than refused. + """ + _declare( + monkeypatch, + { + "runs-on": "${{ matrix.os }}", + "steps": [], + "strategy": { + "matrix": { + "os": [HOSTED], + "include": [{"os": "windows-2022", "target": "x"}, {"target": "y"}], + } + }, + }, + ) + placed = reader.placement("w.yml", "j") + assert placed.labels == frozenset({HOSTED, "windows-2022"}), ( + f"the include leg's label must be read, got {sorted(placed.labels)}" + ) + + +@pytest.mark.parametrize( + "value", + [FORK_EXPRESSION, "ubuntu-${{ inputs.release }}", " ${{ matrix.x }}"], + ids=["whole-expression", "partial-interpolation", "padded-expression"], +) +def test_a_matrix_value_holding_an_expression_is_refused( + monkeypatch: pytest.MonkeyPatch, value: str ) -> None: """Refuse the shape rstest-bdd #788's contract read straight through. A reader that stopped at `matrix.os` never saw that the value behind the key was itself the fork expression, so a name reading the fork field shared no reference with `runs-on` and the stability contract passed while the - name rendered differently per event. + name rendered differently per event. A partial interpolation is the same + defect: GitHub evaluates it, so the recorded text is not the label that + runs. """ _declare( monkeypatch, { "runs-on": "${{ matrix.os }}", "steps": [], - "strategy": {"matrix": {"include": [{"os": FORK_EXPRESSION}]}}, + "strategy": {"matrix": {"include": [{"os": value}]}}, }, ) with pytest.raises(AssertionError, match="cannot model"): diff --git a/tests/test_ci_workflow_loader.py b/tests/test_ci_workflow_loader.py index fe0ff8a74..520a7097f 100644 --- a/tests/test_ci_workflow_loader.py +++ b/tests/test_ci_workflow_loader.py @@ -81,3 +81,25 @@ def test_the_source_sweep_reads_through_the_named_boundary(tmp_path: Path) -> No (tmp_path / "stuck.yml").mkdir() with pytest.raises(AssertionError, match=r"stuck\.yml could not be read"): workflow_sources(tmp_path) + + +@pytest.mark.parametrize("shape", ["missing", "file", "empty"]) +def test_the_source_sweep_refuses_a_directory_it_cannot_sweep( + tmp_path: Path, shape: str +) -> None: + """A sweep that finds nothing fails instead of reporting a clean estate. + + ``Path.glob`` on a missing directory returns an empty list, and every + "no workflow does X" contract would then pass having read nothing. A path + that is a file, and a directory holding no workflow, are the same hazard. + """ + target = tmp_path / "workflows" + match shape: + case "file": + target.write_text("", encoding="utf-8") + case "empty": + target.mkdir() + case _: + pass + with pytest.raises(AssertionError, match=r"workflow directory|holds no workflow"): + workflow_sources(target)