From a544054098b8da711241f625b7af645bae629689 Mon Sep 17 00:00:00 2001 From: Transaction Science Date: Thu, 8 Oct 2026 11:40:23 -0400 Subject: [PATCH] ci: a rebase ran every path-filtered workflow, because `paths:` matches the push delta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Forgejo matches a workflow's `paths:` filter against the **push delta**, not the pull request's diff. A rebase moves a branch across every commit main has gained, so its delta touches nearly every path and every path-filtered workflow fires. Measured on this repo, as a clean natural experiment — five PRs, all changing only `jcp/` or `mesh/`: | PRs | how pushed | workflows triggered | |---|---|---| | #189, #190 | fresh branch, one push | **2** | | #165, #177, #183 | force-push after rebase | **15** | Fifteen includes JPEG-AI whole decode, `wai-pack` on three platforms, and cross-architecture byte-equality on three — none of which a change to `jcp/jcp-rs/crates/jcp-grant/src/checkpoint.rs` can affect. Rebasing three PRs put the queue 65 runs deep behind one macOS runner, which delays everyone's real jobs, and there is no way to rebase without it: a merge of main into the branch has the same delta. So the `paths:` filters still gate the *trigger*, and a new `relevant` job gates the *work*, from the pull request's own diff. It reads the path list out of the workflow file it guards, so there is one list rather than a third copy to drift from the two `on:` blocks. On a push to main there is no pull-request context, the shas are empty, and the gate answers `true` — main runs everything, as before. **It fails open, by construction.** A gate that decides whether tests run is load-bearing in the dangerous direction: a wrong `false` silently skips a whole workflow, which is the "passes but tested nothing" failure. Every way the script can fail to decide — a missing workflow file, a filter it cannot parse, an empty or unresolvable sha, too few arguments, an empty diff — prints `true`. The self-test pins all six of those, both directions of the real decision, and that `**` spans a path separator while `*` does not. Writing that self-test caught one of its own cases: the single-star case first created a top-level `.md` alongside the nested one, so the glob matched legitimately and the case proved nothing. It now changes only the nested file, and asserts separately that the glob does match at its own level — so it cannot pass by never matching anything. Applied to `wai-pack.yml` alone, not all eighteen. What a *skipped* job reports as a commit status on this forge is the open question, and a required context that never reports would make PRs unmergeable. That failure is at least loud rather than silent, but it is worth learning on the most expensive workflow — six job instances across three platforms — before changing the rest. Validated against real branches first: every WAI workflow answers `false` for #165's jcp-only diff and `true` for a WAI branch, and `standards-build.yml` correctly answers `true` for #165 because it builds every workspace. The hygiene workflow runs the self-test, so the gate cannot regress silently, and its own `paths:` filter now lists the scripts it checks. Co-Authored-By: Claude Opus 5 --- .github/workflows/wai-pack.yml | 39 ++++++++++ .github/workflows/workflow-hygiene.yml | 21 ++++- scripts/pr-touches-paths-selftest.sh | 70 +++++++++++++++++ scripts/pr-touches-paths.py | 104 +++++++++++++++++++++++++ 4 files changed, 232 insertions(+), 2 deletions(-) create mode 100755 scripts/pr-touches-paths-selftest.sh create mode 100755 scripts/pr-touches-paths.py diff --git a/.github/workflows/wai-pack.yml b/.github/workflows/wai-pack.yml index 2fc67964a..6fcd6c678 100644 --- a/.github/workflows/wai-pack.yml +++ b/.github/workflows/wai-pack.yml @@ -47,8 +47,41 @@ on: - '.github/workflows/wai-pack.yml' jobs: + # Forgejo matches the `paths:` filters above against the **push delta**, not + # the pull request's diff. A rebase moves a branch across every commit main + # has gained, so its delta touches nearly every path and this workflow fires + # on changes that cannot affect it. Measured on this repo: a jcp-only change + # pushed to a fresh branch ran 2 workflows; the same change force-pushed after + # a rebase ran 15, this one among them, putting three platforms of wai-pack and + # the JPEG-AI whole decode in a queue they had no business being in. + # + # So the filters above still gate the *trigger*, and this job gates the + # *work*, from the pull request's own diff. It reads the path list out of this + # file, so there is one list and not a third copy to drift. On a push to main + # there is no pull-request context, the shas are empty, and the gate answers + # `true` — a push to main runs everything, as before. + relevant: + name: wai-pack · does this change touch it + runs-on: linux + outputs: + run: ${{ steps.gate.outputs.run }} + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - id: gate + run: | + set -euo pipefail + r=$(python3 scripts/pr-touches-paths.py .github/workflows/wai-pack.yml \ + "${{ github.event.pull_request.base.sha }}" \ + "${{ github.event.pull_request.head.sha }}") + echo "this change touches wai-pack: $r" + echo "run=$r" >> "$GITHUB_OUTPUT" + legs: name: wai-pack · ${{ matrix.os }} + needs: relevant + if: needs.relevant.outputs.run == 'true' runs-on: ${{ matrix.os }} strategy: fail-fast: false @@ -112,6 +145,8 @@ jobs: independent: name: wai-pack · independent verifier (Python) + needs: relevant + if: needs.relevant.outputs.run == 'true' runs-on: linux steps: - uses: actions/checkout@v5 @@ -167,6 +202,8 @@ jobs: media: name: wai-pack · the floor through a media prober + needs: relevant + if: needs.relevant.outputs.run == 'true' runs-on: linux steps: - uses: actions/checkout@v5 @@ -198,6 +235,8 @@ jobs: licences: name: wai-pack · dependency licences + needs: relevant + if: needs.relevant.outputs.run == 'true' runs-on: linux steps: - uses: actions/checkout@v5 diff --git a/.github/workflows/workflow-hygiene.yml b/.github/workflows/workflow-hygiene.yml index 8047972f8..8875064ff 100644 --- a/.github/workflows/workflow-hygiene.yml +++ b/.github/workflows/workflow-hygiene.yml @@ -2,8 +2,8 @@ name: workflow hygiene # Properties of the workflows themselves, checked rather than remembered. # -# Today that is one property: every workflow declares a concurrency group that -# varies by ref. None of them did until 2026-10-05, and the consequence was not +Two properties today. The first: every workflow declares a concurrency group +# that varies by ref. None of them did until 2026-10-05, and the consequence was not # a slow build — it was that CI stopped being usable for anyone. A session # pushing four versions of a branch queued four complete sets of runs, all of # which had to drain; one PR's 21 contexts settled none in 70 minutes behind a @@ -12,6 +12,15 @@ name: workflow hygiene # whole repo, invisibly: the workflow still passes, it just queues forever # under load. # +# **The path gate.** `scripts/pr-touches-paths.py` decides whether a +# path-filtered workflow's jobs do any work, from the pull request's own diff — +# because Forgejo matches a `paths:` filter against the push delta, so a rebase +# fires every path-filtered workflow in the repo (measured: a jcp-only change +# ran 2 workflows pushed fresh and 15 force-pushed after a rebase). A gate that +# decides whether tests run is load-bearing in the dangerous direction: a wrong +# `false` silently skips a workflow. Its self-test pins both directions and that +# every way of failing fails *open*. +# # This job runs on `linux`, which is jetson-hub — deliberately not the dev Mac, # which already serves 13 of this repo's 21 contexts through two runners. It # needs no toolchain and finishes in seconds. @@ -23,6 +32,9 @@ on: paths: - '.github/workflows/**' - 'scripts/check-workflow-concurrency.sh' + - 'scripts/check-workflow-concurrency-selftest.sh' + - 'scripts/pr-touches-paths.py' + - 'scripts/pr-touches-paths-selftest.sh' concurrency: group: ${{ github.workflow }}-${{ github.ref }} @@ -45,3 +57,8 @@ jobs: - name: Every workflow is grouped per ref run: ./scripts/check-workflow-concurrency.sh + + # The path gate fails open in every failure mode, and still answers + # `false` for a diff that cannot affect the workflow. + - name: The path gate fails open (self-test) + run: ./scripts/pr-touches-paths-selftest.sh diff --git a/scripts/pr-touches-paths-selftest.sh b/scripts/pr-touches-paths-selftest.sh new file mode 100755 index 000000000..9df21e099 --- /dev/null +++ b/scripts/pr-touches-paths-selftest.sh @@ -0,0 +1,70 @@ +#!/usr/bin/env bash +# Self-test for pr-touches-paths.py, run by the workflow-hygiene job. +# +# A gate that decides whether tests run is itself load-bearing: a wrong `false` +# silently skips a whole workflow. These cases pin both directions and, most +# importantly, that every way of failing fails OPEN. +set -euo pipefail +cd "$(dirname "$0")/.." +G=scripts/pr-touches-paths.py +tmp=$(mktemp -d); trap 'rm -rf "$tmp"' EXIT +fail=0 +check() { # name expected actual + if [ "$2" = "$3" ]; then printf ' ok %s\n' "$1" + else printf ' FAIL %s: expected %s, got %s\n' "$1" "$2" "$3"; fail=1; fi +} + +# A throwaway repo, so the diffs are real git diffs and not a mock. +repo="$tmp/r"; mkdir -p "$repo/wai/wai-pack" "$repo/jcp" +git -C "$repo" init -q +git -C "$repo" config user.email t@e; git -C "$repo" config user.name t +echo x > "$repo/jcp/SPEC.md"; echo y > "$repo/wai/wai-pack/lib.rs" +git -C "$repo" add -A; git -C "$repo" commit -qm base +BASE=$(git -C "$repo" rev-parse HEAD) +echo changed > "$repo/jcp/SPEC.md" +git -C "$repo" add -A; git -C "$repo" commit -qm jcp-only +JCP=$(git -C "$repo" rev-parse HEAD) +echo changed > "$repo/wai/wai-pack/lib.rs" +git -C "$repo" add -A; git -C "$repo" commit -qm wai-too +WAI=$(git -C "$repo" rev-parse HEAD) + +wf="$tmp/w.yml" +cat > "$wf" <<'Y' +on: + pull_request: + paths: + - 'wai/wai-pack/**' +Y + +run() { ( cd "$repo" && python3 "$OLDPWD/$G" "$1" "$2" "$3" ); } +check "a jcp-only diff does not touch a wai filter" false "$(run "$wf" "$BASE" "$JCP")" +check "a diff that touches the filter runs" true "$(run "$wf" "$BASE" "$WAI")" + +# Every failure mode must fail OPEN. +check "a missing workflow file" true "$(run "$tmp/nope.yml" "$BASE" "$JCP")" +check "a workflow with no filter" true "$(printf 'on:\n pull_request:\n' > "$tmp/nofilter.yml"; run "$tmp/nofilter.yml" "$BASE" "$JCP")" +check "an empty base sha" true "$(run "$wf" "" "$JCP")" +check "a sha git cannot resolve" true "$(run "$wf" "$BASE" deadbeefdeadbeefdeadbeefdeadbeefdeadbeef)" +check "too few arguments" true "$( cd "$repo" && python3 "$OLDPWD/$G" "$wf" )" +check "an empty diff" true "$(run "$wf" "$JCP" "$JCP")" + +# `**` spans separators; `*` does not. +cat > "$tmp/star.yml" <<'Y' +on: + pull_request: + paths: + - 'jcp/*.md' +Y +# Only the NESTED file changes, so `jcp/*.md` must not match it. (A top-level +# .md in the same commit would match legitimately and prove nothing — which is +# how this case failed the first time it was written.) +mkdir -p "$repo/jcp/sub"; echo z > "$repo/jcp/sub/x.md" +git -C "$repo" add -A >/dev/null; git -C "$repo" commit -qm sub >/dev/null +SUB=$(git -C "$repo" rev-parse HEAD) +check "a single star does not span a separator" false "$(run "$tmp/star.yml" "$WAI" "$SUB")" +# And it does match a file at that level, so the glob is not simply never matching. +echo z > "$repo/jcp/top.md" +git -C "$repo" add -A >/dev/null; git -C "$repo" commit -qm top >/dev/null +check "a single star matches at its own level" true "$(run "$tmp/star.yml" "$SUB" "$(git -C "$repo" rev-parse HEAD)")" + +[ "$fail" = 0 ] && echo "pr-touches-paths self-test: all cases pass" || { echo "pr-touches-paths self-test FAILED"; exit 1; } diff --git a/scripts/pr-touches-paths.py b/scripts/pr-touches-paths.py new file mode 100755 index 000000000..767d03bb5 --- /dev/null +++ b/scripts/pr-touches-paths.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +"""Does this pull request's own diff touch any path its workflow filters on? + +Forgejo matches a workflow's `paths:` filter against the **push delta**, not the +pull request's diff. A rebase moves a branch across every commit main has +gained, so its push delta touches nearly every path and every path-filtered +workflow fires. Measured on this repo: a jcp-only change pushed to a fresh +branch ran 2 workflows; the same change force-pushed after a rebase ran 15, +including JPEG-AI whole decode and wai-pack on three platforms. That is what +fills the queue, and it delays everyone's real jobs behind irrelevant ones. + +This reads the path list out of the workflow file itself — one source of truth, +rather than a third copy of the list to drift from the two `on:` blocks — and +compares it against `git diff --name-only BASE...HEAD`. + + pr-touches-paths.py + +Prints `true` or `false`. **Fails open**: anything it cannot determine prints +`true`, because a wrong `false` silently skips a test and a wrong `true` only +costs runner time. A gate that guesses must guess toward running. +""" +import re +import subprocess +import sys +from pathlib import Path + + +def paths_of(workflow: Path) -> list[str]: + """The union of every `paths:` list under `on:` in this workflow. + + Parsed rather than imported from a library: the runner image has no PyYAML + guaranteed, and the shape here is a flat `paths:` followed by `- 'glob'` + lines. A list this parser fails to read yields none, which fails open. + """ + globs: list[str] = [] + in_paths = False + for line in workflow.read_text().splitlines(): + if re.match(r"\s*paths(-ignore)?:\s*$", line): + in_paths = True + continue + if in_paths: + m = re.match(r"\s*-\s*'([^']+)'\s*$", line) or re.match(r'\s*-\s*"([^"]+)"\s*$', line) + if m: + globs.append(m.group(1)) + continue + if line.strip().startswith("- "): + globs.append(line.strip()[2:].strip()) + continue + in_paths = False + return globs + + +def matches(path: str, glob: str) -> bool: + """`glob` in the Actions sense: `**` spans separators, `*` does not.""" + rx = [] + i = 0 + while i < len(glob): + if glob.startswith("**", i): + rx.append(".*") + i += 2 + elif glob[i] == "*": + rx.append("[^/]*") + i += 1 + elif glob[i] == "?": + rx.append("[^/]") + i += 1 + else: + rx.append(re.escape(glob[i])) + i += 1 + return re.fullmatch("".join(rx), path) is not None + + +def main() -> None: + if len(sys.argv) != 4: + print("true") # cannot tell: run it + return + workflow, base, head = Path(sys.argv[1]), sys.argv[2], sys.argv[3] + if not workflow.is_file() or not base or not head: + print("true") + return + globs = paths_of(workflow) + if not globs: + # No filter, or a shape this parser does not read: the workflow wanted + # to run on everything, or we cannot say it did not. + print("true") + return + try: + out = subprocess.run( + ["git", "diff", "--name-only", f"{base}...{head}"], + capture_output=True, text=True, check=True, + ).stdout + except (subprocess.CalledProcessError, OSError): + print("true") + return + changed = [l for l in out.splitlines() if l.strip()] + if not changed: + print("true") + return + hit = any(matches(p, g) for p in changed for g in globs) + print("true" if hit else "false") + + +if __name__ == "__main__": + main() -- 2.54.0 (Apple Git-157)