From 7a634d91b071d8631d9d70d14f04174d52fe9bb0 Mon Sep 17 00:00:00 2001 From: Cameron Brooks Date: Thu, 8 Oct 2026 04:26:24 -0400 Subject: [PATCH 1/3] feat: flag a required check with a second pull request producer The DUPLICATE rule found a second producer only on push. Another active workflow that runs a job of the same context on a pull request into the default branch, on pull_request or pull_request_target, gives that head the context from two suites as well, and the audit passed it. So did one workflow that runs on both events. pull_request_target now counts as a pull request event. Its check runs carry the pull request's head sha, and GitHub lists it among the events whose checks satisfy a required status check, so a required check it alone produces is no longer ORPHANED, and one it produces unrequired is UNGATED. A path filter or a types: restriction is a finding only when every event that reaches the pull request has one. A pair of pull request producers is reported once, from the first workflow by path. A job elsewhere whose context cannot be resolved, or whose branch filter cannot be read, is refused with exit 2. Closes #20 Signed-off-by: Cameron Brooks --- AGENTS.md | 14 +- README.md | 3 +- STATE.md | 54 ++++- scripts/check_required_checks.py | 286 ++++++++++++++++++++++--- scripts/sweep_check_required_checks.py | 112 ++++++++-- 5 files changed, 408 insertions(+), 61 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 410c669..4bbd769 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -179,10 +179,16 @@ Each is the pattern in §2.2, one layer up in the infrastructure. the gate resolves each required name to whichever run it finds (kindspec/rowspec#43). The second producer can also be a `push` job in a different workflow with the same name. Restrict `push` to the default branch, - or to tags. The audit reports either as DUPLICATE: `push` with no branch - filter, a `branches-ignore` that leaves some other branch, or `branches` that - match one. It refuses, exit 2, a filter it cannot settle either way, and a - `push` job elsewhere whose name it cannot resolve. + or to tags. It can also run on the pull request itself: a job of the same + name in another workflow on `pull_request` or `pull_request_target`, or one + workflow on both. `pull_request_target` reports on the pull request's head + like `pull_request` does, and satisfies a required check, but it runs the + workflow file from the base branch, so the pull request cannot change it. The + audit reports each of these as DUPLICATE: `push` with no branch filter, a + `branches-ignore` that leaves some other branch, or `branches` that match + one; or a second pull request producer into the default branch. It refuses, + exit 2, a filter it cannot settle either way, and a job elsewhere whose name + it cannot resolve. **Verify a rule by watching it reject, not by reading its configuration.** `git push --dry-run` does not evaluate server-side rules and will report success diff --git a/README.md b/README.md index 42182da..1f90afa 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,8 @@ Org-level defaults for [kindspec](https://github.com/kindspec). - `scripts/check_required_checks.py` — fails when a job that runs on pull requests gates no merge, a required check never reports or can be skipped, or a required check also comes from a `push` run on the pull request's - branch, in the same workflow or another; its mutation + branch, in the same workflow or another, or from a second workflow that runs + on the pull request (`pull_request` or `pull_request_target`); its mutation sweep is `scripts/sweep_check_required_checks.py`. `.github/workflows/required-checks-audit.yml` runs it daily, and on demand. - `profile/README.md` — the org landing page. diff --git a/STATE.md b/STATE.md index 4c35343..63147ab 100644 --- a/STATE.md +++ b/STATE.md @@ -241,7 +241,7 @@ and calls the third the most likely home of a genuine silent-wrong merge. `scripts/check_required_checks.py`. **The required-checks audit exits 0 on the org.** On 2026-10-08, with the -DUPLICATE rule of .github#16 and .github#18: +DUPLICATE rule of .github#16, .github#18 and .github#20: $ python3 -I scripts/check_required_checks.py --public-only 1 private repositories skipped (--public-only) @@ -283,9 +283,41 @@ refused, exit 2. The issue's reproduction, a `push.yml` with `on: push` and a job `check` added to kindkit in a `--save` snapshot, gives one DUPLICATE for `check`. -**What it still does not see: two `pull_request` producers.** Two workflows -that both run a job of the same name on `pull_request` also give a required -context two suites. Nothing flags that yet. +**And a second producer on the pull request itself** (.github#20). Two active +workflows that run a job of the same context on a pull request into the default +branch, on `pull_request` or `pull_request_target`, are a DUPLICATE, reported +once per pair; so is one workflow on both events. A job there whose context it +cannot resolve is refused, exit 2. The issue's reproduction, a `second.yml` with +a job `check` added to kindkit in a `--save` snapshot, gives one DUPLICATE for +`check` on `pull_request` and one on `pull_request_target`; main gives 0 +findings for both. + +**`pull_request_target` reports on the pull request's head**, so the audit now +counts it as a pull request event: a required check it alone produces is no +longer ORPHANED, and one it produces unrequired is UNGATED. Its check runs carry +the pull request's head sha: +python/cpython run 25066296099 (fork PR 149109, the issue's case), +electron/electron run 37748122900 on PR 54550's head `a965781` (a same-repo +branch), and react/react run 37730121633 on fork PR 37788's head `7af445b`. +For the last two, `gh api repos///commits//check-runs` lists the job, +and the GraphQL `statusCheckRollup` of the PR's last commit lists it with +`event: pull_request_target`. GitHub's +"Troubleshooting required status checks" says: "For checks created by workflow +jobs to be evaluated for a pull request, the workflow run must be triggered by +one of these events", and lists `push`, `pull_request`, `pull_request_review`, +`pull_request_target`, `deployment` and `deployment_status`. It names +`workflow_dispatch` on the head branch as one whose checks "do not satisfy a +required status check in a branch ruleset". That a ruleset accepts a +`pull_request_target` check is the documentation's word; no kindspec ruleset +has been watched doing it. A `pull_request_target` job runs the base branch's +workflow file with `GITHUB_SHA` the base's last commit, so a pull request cannot +change it, and it tests the pull request's code only if it checks that out. + +**What it still does not see.** The same page also lists `pull_request_review`, +`deployment` and `deployment_status` as events whose checks count; the audit +reads none of them as a producer, for DUPLICATE or anything else. Two jobs of +one workflow with the same context (`name: check` on two job ids) are not +flagged either. `evaluate` returns no finding for each. #### Security settings, org-wide @@ -666,10 +698,11 @@ and these two had, at ten against twelve: - a CI mutation step accepting any exit 0, so `true` passed as a mutation gate that had run nothing (kindkit#24 review) - a required-checks audit reporting 0 findings for a required check produced - only by `merge_group`, `pull_request_target`, or a `pull_request` filtered to - another branch, none of which ever reports on the pull request it gates - (.github#10 review; reproduced with `--load` against that pull request's - first commit, `0970870`) + only by `merge_group` or a `pull_request` filtered to another branch, neither + of which ever reports on the pull request it gates (.github#10 review; + reproduced with `--load` against that pull request's first commit, + `0970870`). That review also put `pull_request_target` in this list, which + was wrong: its check runs attach to the pull request's head (.github#20) - the same audit reporting 0 findings while kindkit's required `check` and `kind / conformance` ran twice on a pull request head, once from a bare `push` and once from `pull_request`, because it read only the @@ -678,6 +711,11 @@ and these two had, at ten against twelve: producer was a `push` job of the same name in a different workflow, because it read only each workflow's own `push` (.github#18; reproduced with `--load` on a `--save` snapshot with a `push.yml` added to kindkit) +- the same audit, with that fixed, still reporting 0 findings when the second + producer was a job of the same name in another workflow on `pull_request` or + `pull_request_target`, because it looked for a second producer only on `push` + (.github#20; reproduced with `--load` on a `--save` snapshot with a + `second.yml` added to kindkit) - `corpus_check.py`, a green CI step in rowspec that never opened a `.mdtbl` file and printed `0 identified artifact(s), 0 duplicate id(s)` on rowspec's own tree (rowspec#68) diff --git a/scripts/check_required_checks.py b/scripts/check_required_checks.py index d7b30d2..8bfa15c 100644 --- a/scripts/check_required_checks.py +++ b/scripts/check_required_checks.py @@ -4,15 +4,15 @@ Adding a workflow to a repository does not add it to the ruleset, so a suite can run on every pull request and gate nothing (kindspec/.github#3). This fails when: - UNGATED a job that runs on pull requests to the default branch is not a - required check + UNGATED a job that runs on pull requests to the default branch, on + pull_request or pull_request_target, is not a required check ORPHANED a required check is a context no such job produces. A job that - runs only on merge_group, pull_request_target (its check runs - attach to the base commit), push, on pull requests to other + runs only on merge_group, push, on pull requests to other branches, or in a workflow that is not `active` does not count: the context never reports on the PR head - TRIGGER a required job's pull_request `types:` omit opened or synchronize, - so it does not report on every pull request head + TRIGGER a required job's pull_request (or pull_request_target) `types:` + omit opened or synchronize, so it does not report on every pull + request head CONDITIONAL a required job can be skipped, and GitHub reports a skipped job as success: it has a job-level `if:`, or something in its `needs:` chain has one or is not itself required. When the skipped job calls @@ -30,9 +30,21 @@ whose push admits such a branch: its context (' / ' for a call), by `name:` or else its id, equals the required one. Its `if:` does not matter: a skipped job still - reports its context + reports its context. Likewise when a second producer runs on the + pull request itself: the required job's workflow runs on both + pull_request and pull_request_target, or another active workflow + runs a job of the same context on either, into the default + branch. That pair is reported once, from the first workflow by path STALE-ALLOW an ALLOW entry matches no job any more +pull_request_target counts as a pull request event: its check runs carry the +pull request's head sha, and GitHub lists it among the events whose checks +satisfy a required status check (kindspec/.github#20). It runs the workflow file +from the base branch, with GITHUB_SHA the base's last commit, so a pull request +cannot change what such a job does, and the job tests the pull request's code +only if it checks that out. A pull_request_target job that is meant to gate +nothing, such as a labeller, belongs in ALLOW with that reason. + Exit 0 clean, 1 on any finding, 2 when something could not be evaluated: no repositories read; an API or authentication failure; fewer repositories listed than the org reports, or a private count this token cannot read; a repository @@ -44,8 +56,9 @@ or, for a required job, a push branch filter this script cannot settle either way (it decides by finding a branch the filter admits, or by enumerating every branch it could), in its own workflow or in another whose job has the same -context; or a job of another workflow that runs on such a push and whose -context is not known (matrix, expression or block name, or a call not read). +context; or a job of another workflow that runs on such a push, or on a pull +request into the default branch, and whose context is not known (matrix, +expression or block name, or a call not read). A run that could not look must not report a pass. A job that calls a reusable workflow (`uses: ./.github/workflows/F.yml`, or @@ -89,6 +102,10 @@ ACTIONS_APP_ID = 15368 # GitHub Actions; a context from another app is not a workflow job DEFAULT_TYPES = {"opened", "synchronize"} # without both, a PR head can go unreported +# Events whose check runs attach to a pull request's head commit and count toward its +# required checks. pull_request_target runs the base branch's workflow, but its check +# runs still carry the head sha (kindspec/.github#20). +PR_EVENTS = ("pull_request", "pull_request_target") SAMPLES = 4096 # names tried per branch filter pattern; a pattern with more is not enumerated GH = ["gh"] @@ -384,24 +401,36 @@ def _events(workflow): return on +def pr_events(on, branch): + """The events in PR_EVENTS on which the workflow runs for a pull request into branch.""" + return [e for e in PR_EVENTS if e in on and _branch_runs(on[e], branch)] + + +def pr_admits(on, branch): + """Why the workflow reports on a pull request into branch, or None; for producers().""" + events = pr_events(on, branch) + return "on " + " and ".join(events) if events else None + + def pr_trigger(workflow, branch): - """(how the workflow reaches a PR into branch, path-filtered, restricted types, why not).""" + """(events that reach a PR into branch, path-filtered, restricted types, why not).""" on = _events(workflow) - if "pull_request" in on: - config = on["pull_request"] - if _branch_runs(config, branch): - config = config if isinstance(config, dict) else {} - filtered = bool({"paths", "paths-ignore"} & set(config)) - types = config.get("types") - if types is not None: - types = set(map(str, types if isinstance(types, list) else [types])) - restricted = types is not None and not DEFAULT_TYPES <= types - return True, filtered, sorted(types) if restricted else None, None - why = f"pull_request excludes '{branch}'" - else: - others = [e for e in ("merge_group", "pull_request_target", "push") if e in on] - why = "only " + ", ".join(others) if others else None - return False, False, None, why + events = pr_events(on, branch) + if not events: + why = "; ".join(f"{e} excludes '{branch}'" for e in PR_EVENTS if e in on) + others = [e for e in ("merge_group", "push") if e in on] + why = why or ("only " + ", ".join(others) if others else None) + return [], False, None, why + # a filter or a types: restriction matters only if every event that reaches the PR + # has one; otherwise another event reports the context on every head + configs = [on[e] if isinstance(on[e], dict) else {} for e in events] + filtered = all({"paths", "paths-ignore"} & set(c) for c in configs) + types = [c.get("types") for c in configs] + types = [None if t is None else set(map(str, t if isinstance(t, list) else [t])) for t in types] + restricted = None + if not any(t is None or DEFAULT_TYPES <= t for t in types): + restricted = sorted(set().union(*types)) + return events, filtered, restricted, None def jobs_of(workflow): @@ -468,9 +497,11 @@ def expand(context, uses, data): return called, None -def push_producers(data, branch): - """The contexts each active workflow produces on push to a branch other than branch. +def producers(data, branch, admits): + """The contexts each active workflow produces where admits(on, branch) gives a reason. + With push_duplicates, that is on push to a branch other than branch; with pr_admits, + on a pull request into branch. Returns ({context: [(path, producing job, why)]}, [(path, context, why not evaluated)]), where a context of None could be any. A job's `if:` does not matter: a skipped job still reports its context. A workflow that cannot be read is refused by evaluate. @@ -486,7 +517,7 @@ def push_producers(data, branch): except Unsupported: continue try: - why, doubt = push_duplicates(on, branch), None + why, doubt = admits(on, branch), None except Unsupported as e: why, doubt = None, str(e) if not why and not doubt: @@ -552,7 +583,8 @@ def evaluate(snapshot, allow): required.setdefault(c["context"], c.get("integration_id")) stats["required"] += len(required) produced, elsewhere, blind = set(), {}, False - on_push, push_unknown = push_producers(data, branch) + on_push, push_unknown = producers(data, branch, push_duplicates) + on_pr, pr_unknown = producers(data, branch, pr_admits) for path, text in sorted(data["workflows"].items()): stats["workflows"] += 1 state = data["workflow_state"].get(path) @@ -562,15 +594,15 @@ def evaluate(snapshot, allow): continue try: workflow = parse_yaml(text) - runs, filtered, types, why = pr_trigger(workflow, branch) + events, filtered, types, why = pr_trigger(workflow, branch) jobs = {j[0]: j for j in jobs_of(workflow)} except Unsupported as e: incomplete.append(f"{repo}: {path}: not evaluated: {e}") blind = True continue if state != "active": - runs, why = False, f"workflow is {state}" - if not runs: + events, why = [], f"workflow is {state}" + if not events: for _, context, *_ in jobs.values(): if why: elsewhere.setdefault(context, why) @@ -622,7 +654,22 @@ def evaluate(snapshot, allow): ) elif unknown: incomplete.append(f"{unit}: not evaluated for a push producer: {unknown}") - # its own push is the finding above; another workflow's is this one + if len(events) > 1: + findings.append( + f"DUPLICATE {unit} is required and its workflow runs on both " + f"{' and '.join(events)}, so a pull request head can carry " + f"'{context}' from two suites and the merge takes either" + ) + # one finding per pair of workflows, from the first in path order + for other, job, why in on_pr.get(context, []): + if other > path: + findings.append( + f"DUPLICATE {unit} is required, and {other}: {job} also " + f"produces '{context}' on a pull request into '{branch}' ({why}), " + "so a pull request head can carry it from two suites and the " + "merge takes either" + ) + # its own push is reported above; another workflow's push is this one for other, job, why in on_push.get(context, []): if other != path: findings.append( @@ -641,6 +688,16 @@ def evaluate(snapshot, allow): f"{unit}: not evaluated for a push producer in another workflow: " + "; ".join(doubts) ) + doubts = [ + f"{other}: {why}" + for other, c, why in pr_unknown + if other != path and c in (None, context) + ] + if doubts: + incomplete.append( + f"{unit}: not evaluated for a pull request producer in another " + "workflow: " + "; ".join(doubts) + ) if filtered: findings.append( f"PATH-FILTER {unit} is required but its pull_request " @@ -1411,6 +1468,158 @@ def push_job(head="check:", body="", on="push"): inc1, ), ] + + # a second producer on the pull request itself: pull_request or pull_request_target (#20) + def target(trigger=" pull_request_target:\n", job="check", body=""): + return f"on:\n{trigger}jobs:\n {job}:\n runs-on: x\n{body}" + + pr_target = target() + pr_plain = target(" pull_request:\n") + + def state_of(snap, path, state): + snap["r"]["workflow_state"][path] = state + return snap + + lint = _CHECK + [{"context": "lint"}] + inactive_push = state_of(beside(push_job()), "p.yml", "disabled_inactivity") + pulled_caller = f"on: pull_request\njobs:\n kind:\n uses: {remote}\n" + pr_pair = [ + ("two pull_request workflows", beside(pr_plain), dup), + ("pull_request and pull_request_target workflows", beside(pr_target), dup), + ( + "pull_request_target first by path, pull_request second", + repo(pr_target, more={"x.yml": pr_plain}), + dup, + ), + ( + "pull_request_target job named by name:", + beside(target(job="p", body=" name: check\n")), + dup, + ), + ( + "one workflow on pull_request and pull_request_target", + repo(target(" pull_request:\n pull_request_target:\n")), + dup, + ), + ( + "three workflows give one finding per pair", + repo(pr_plain, more={"p.yml": pr_plain, "q.yml": pr_target}), + ["DUPLICATE", "DUPLICATE", "DUPLICATE"], + ), + ( + "push and pull_request in another workflow are two producers", + beside(target(" push:\n pull_request:\n")), + ["DUPLICATE", "DUPLICATE", "DUPLICATE"], + ), + ( + "called workflow under pull_request in another workflow", + repo( + caller(local), + kind, + more={".github/workflows/k.yml": conf, "p.yml": pulled_caller}, + called={remote: conf}, + ), + dup, + ), + ("unrequired pull_request_target job", repo(pr_target, []), ["UNGATED"]), + ( + "pull_request_target with an if:", + repo(pr_target + " if: false\n"), + ["CONDITIONAL"], + ), + ( + "pull_request_target types: [labeled]", + repo(target(" pull_request_target:\n types: [labeled]\n")), + ["TRIGGER"], + ), + ( + "pull_request_target path-filtered", + repo(target(" pull_request_target:\n paths: [a]\n")), + ["PATH-FILTER"], + ), + # with both events, a filter or types: matter only when both have one + ( + "pull_request path-filtered, pull_request_target not", + repo(target(" pull_request:\n paths: [a]\n pull_request_target:\n")), + dup, + ), + ( + "both events path-filtered", + repo( + target(" pull_request:\n paths: [a]\n pull_request_target:\n paths: [a]\n") + ), + ["DUPLICATE", "PATH-FILTER"], + ), + ( + "pull_request types: [labeled], pull_request_target default", + repo(target(" pull_request:\n types: [labeled]\n pull_request_target:\n")), + dup, + ), + ( + "both events types: [labeled]", + repo( + target( + " pull_request:\n types: [labeled]\n" + " pull_request_target:\n types: [labeled]\n" + ) + ), + ["DUPLICATE", "TRIGGER"], + ), + # negative controls + ( + "pull_request_target into another branch", + repo(target(" pull_request_target:\n branches: [develop]\n")), + ["ORPHANED"], + ), + ( + "other pull_request filtered to another base branch", + beside(target(" pull_request:\n branches: [develop]\n")), + [], + ), + ( + "other pull_request_target filtered to another base branch", + beside(target(" pull_request_target:\n branches-ignore: [main]\n")), + [], + ), + ( + "other pull_request job with a different name", + beside(target(" pull_request:\n", job="lint"), None, lint), + [], + ), + ( + "other pull_request_target job with a different name", + beside(target(job="lint"), None, lint), + [], + ), + ("other merge_group job of the same name", beside(target(" merge_group:\n")), []), + ( + "disabled pull_request workflow", + state_of(beside(pr_plain), "p.yml", "disabled_manually"), + [], + ), + ( + "disabled_inactivity pull_request_target workflow", + state_of(beside(pr_target), "p.yml", "disabled_inactivity"), + [], + ), + ("disabled_inactivity push workflow", inactive_push, []), + # cannot be decided: refused, never passed + ( + "other pull_request matrix job is refused", + beside(pr_plain + " strategy:\n matrix:\n v: [1]\n"), + inc2, + ), + ( + "other pull_request filter it cannot read is refused", + beside(target(" pull_request:\n branches: ['ma?n']\n")), + inc2, + ), + ( + "other pull_request filter it cannot read, different name", + beside(target(" pull_request:\n branches: ['ma?n']\n", job="lint")), + inc1, + ), + ] cases = [ ("clean", repo(_PR.format(job="check")), [], None), ("named job is clean", repo(_PR.format(job="j") + " name: check\n"), [], None), @@ -1453,9 +1662,9 @@ def push_job(head="check:", body="", on="push"): None, ), ( - "pull_request_target attaches to the base commit", + "pull_request_target reports on the PR head", repo("on: pull_request_target\njobs:\n check:\n runs-on: x\n"), - ["ORPHANED"], + [], None, ), ( @@ -1638,6 +1847,12 @@ def push_job(head="check:", body="", on="push"): ["ORPHANED"], None, ), + ( + "disabled_inactivity workflow produces nothing", + repo(_PR.format(job="check"), state="disabled_inactivity"), + ["ORPHANED"], + None, + ), ( "workflow with no state is not passed", {"r": {**repo(_PR.format(job="check"))["r"], "workflow_state": {}}}, @@ -1659,6 +1874,7 @@ def push_job(head="check:", body="", on="push"): *reusable, *((label, snap, want, None) for label, snap, want in duplicate), *((label, snap, want, None) for label, snap, want in cross), + *((label, snap, want, None) for label, snap, want in pr_pair), ] bad = 0 diff --git a/scripts/sweep_check_required_checks.py b/scripts/sweep_check_required_checks.py index f2fcfbb..7d8aaa2 100644 --- a/scripts/sweep_check_required_checks.py +++ b/scripts/sweep_check_required_checks.py @@ -35,7 +35,7 @@ ("path filter ignored", " if filtered:\n", " if False:\n"), ("paths-ignore ignored", '{"paths", "paths-ignore"}', '{"paths"}'), ("restrictive types ignored", " if types:\n", " if False:\n"), - ("types check inverted", "not DEFAULT_TYPES <= types", "DEFAULT_TYPES <= types"), + ("types check inverted", "DEFAULT_TYPES <= t for t", "not DEFAULT_TYPES <= t for t"), ( "conditional never reported", " if skip:\n", @@ -44,9 +44,9 @@ ("job-level if ignored", " if jobs[job_id][2]:\n", " if False:\n"), ("unrequired need ignored", "if any(c not in required for c in contexts):", "if False:"), ( - "any pr-ish event counts", - 'if "pull_request" in on:', - 'if {"pull_request", "pull_request_target", "merge_group"} & set(on):', + "merge_group counted as a pr event", + 'PR_EVENTS = ("pull_request", "pull_request_target")', + 'PR_EVENTS = ("pull_request", "pull_request_target", "merge_group")', ), ( "branches-ignore ignored", @@ -335,23 +335,29 @@ ), ( "own undecided push refused again", - "if other != path and c in (None, context)", - "if c in (None, context)", + "in push_unknown\n if other != path and c in (None, context)", + "in push_unknown\n if c in (None, context)", ), ( "undecided push refused for any context", - "if other != path and c in (None, context)", - "if other != path", + "in push_unknown\n if other != path and c in (None, context)", + "in push_unknown\n if other != path", ), ( "unknown-name push job refused only by name", - "if other != path and c in (None, context)", - "if other != path and c == context", + "in push_unknown\n if other != path and c in (None, context)", + "in push_unknown\n if other != path and c == context", ), ( "push doubts never refused", - " if doubts:\n", - " if False:\n", + ( + " if doubts:\n incomplete.append(\n" + ' f"{unit}: not evaluated for a push producer' + ), + ( + " if False:\n incomplete.append(\n" + ' f"{unit}: not evaluated for a push producer' + ), ), ( "inactive workflow pushes", @@ -381,9 +387,89 @@ ), ( "push producers not collected", - "on_push, push_unknown = push_producers(data, branch)", + "on_push, push_unknown = producers(data, branch, push_duplicates)", "on_push, push_unknown = {}, []", ), + # DUPLICATE: a second producer on the pull request itself (#20) + ( + "pull_request_target not a pr event", + 'PR_EVENTS = ("pull_request", "pull_request_target")', + 'PR_EVENTS = ("pull_request",)', + ), + ( + "pr producers not collected", + "on_pr, pr_unknown = producers(data, branch, pr_admits)", + "on_pr, pr_unknown = {}, []", + ), + ( + "cross-workflow pr duplicate never reported", + "for other, job, why in on_pr.get(context, []):", + "for other, job, why in []:", + ), + ( + "pr pair reported from both sides", + " if other > path:\n", + (" if other != path:\n"), + ), + ("own job counted as its own pr producer", "if other > path:", "if other >= path:"), + ("two-event workflow not flagged", "if len(events) > 1:", "if False:"), + ( + "pr branch filter ignored elsewhere", + ' events = pr_events(on, branch)\n return "on "', + ' events = [e for e in PR_EVENTS if e in on]\n return "on "', + ), + ( + "path filter on one event of two counted", + 'filtered = all({"paths", "paths-ignore"}', + 'filtered = any({"paths", "paths-ignore"}', + ), + ( + "path filter read from the first event only", + "set(c) for c in configs)", + "set(c) for c in configs[:1])", + ), + ( + "types on one event of two counted", + "if not any(t is None or DEFAULT_TYPES <= t for t in types):", + "if not all(t is None or DEFAULT_TYPES <= t for t in types):", + ), + ( + "own undecided pr refused again", + "in pr_unknown\n if other != path and c in (None, context)", + "in pr_unknown\n if c in (None, context)", + ), + ( + "undecided pr refused for any context", + "in pr_unknown\n if other != path and c in (None, context)", + "in pr_unknown\n if other != path", + ), + ( + "unknown-name pr job refused only by name", + "in pr_unknown\n if other != path and c in (None, context)", + "in pr_unknown\n if other != path and c == context", + ), + ( + "pr doubts never refused", + ( + " if doubts:\n incomplete.append(\n" + ' f"{unit}: not evaluated for a pull request producer' + ), + ( + " if False:\n incomplete.append(\n" + ' f"{unit}: not evaluated for a pull request producer' + ), + ), + # a mutant that excluded only disabled_manually survived before (#20) + ( + "only disabled_manually excluded", + ' if state != "active":\n', + ' if state == "disabled_manually":\n', + ), + ( + "only disabled_manually excluded from producers", + 'if data["workflow_state"].get(path) != "active":', + 'if data["workflow_state"].get(path) == "disabled_manually":', + ), ] TIMEOUT = 30 # seconds per mutant; the self-test itself takes well under one From a85060d5194fec887668d394faaff8b517b0973c Mon Sep 17 00:00:00 2001 From: Cameron Brooks Date: Thu, 8 Oct 2026 04:43:30 -0400 Subject: [PATCH 2/3] fix: report pr producer pairs past allow, and flag target-only checks A pull request pair was reported only from the first workflow by path, and the main loop skips allowlisted jobs, so an ALLOW entry on that workflow hid the DUPLICATE and the audit exited 0. The pairs are now computed per required context from every pull request producer, whatever ALLOW says, once per pair of workflows. A job in ALLOW still counts as producing its contexts. PATH-FILTER and TRIGGER were each decided over all events separately, so a path filter on one event and types: [labeled] on the other gave neither finding, and types: [opened] with types: [synchronize] gave a false TRIGGER. Each event is now blocked by a path filter or by types: that omit opened or synchronize. The blocks are findings only when the unfiltered events together miss one of those types, and each finding names its event. A required check whose only producers are pull_request_target jobs is now TARGET-ONLY. Such a job runs the default branch's workflow on the default branch's code unless it checks out the pull request, so it may be unable to fail because of the pull request. ALLOW on the job, with the reason, accepts one. The summary line counts "pull request jobs", since it now includes pull_request_target. Signed-off-by: Cameron Brooks --- AGENTS.md | 28 ++- README.md | 3 +- STATE.md | 28 ++- scripts/check_required_checks.py | 226 +++++++++++++++++++------ scripts/sweep_check_required_checks.py | 114 ++++++++----- 5 files changed, 286 insertions(+), 113 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4bbd769..bc0514d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -140,7 +140,7 @@ adversarial discipline, which is the more important of the two clauses, is honour-system and will stay that way until something can attest to it. Say so rather than implying the green tick covers it. -### 3.2 Four ways this gate can quietly stop working +### 3.2 Five ways this gate can quietly stop working Each is the pattern in §2.2, one layer up in the infrastructure. @@ -181,14 +181,24 @@ Each is the pattern in §2.2, one layer up in the infrastructure. different workflow with the same name. Restrict `push` to the default branch, or to tags. It can also run on the pull request itself: a job of the same name in another workflow on `pull_request` or `pull_request_target`, or one - workflow on both. `pull_request_target` reports on the pull request's head - like `pull_request` does, and satisfies a required check, but it runs the - workflow file from the base branch, so the pull request cannot change it. The - audit reports each of these as DUPLICATE: `push` with no branch filter, a - `branches-ignore` that leaves some other branch, or `branches` that match - one; or a second pull request producer into the default branch. It refuses, - exit 2, a filter it cannot settle either way, and a job elsewhere whose name - it cannot resolve. + workflow on both. The audit reports each of these as DUPLICATE: `push` with + no branch filter, a `branches-ignore` that leaves some other branch, or + `branches` that match one; or a second pull request producer into the + default branch, whether or not `ALLOW` names either job. It refuses, exit 2, + a filter it cannot settle either way, and a job elsewhere whose name it + cannot resolve. +- **A check that only `pull_request_target` runs may be unable to fail.** Its + check runs land on the pull request's head like `pull_request`'s do, and + GitHub's documentation says they satisfy a required check + (`content/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks.md` + in github/docs, lines 55–64 at `60f9f34`). But the job runs the workflow + file from the default branch of the base repository, on that branch's code + unless it checks out the pull request, so the pull request can neither change + the job nor, by default, make it fail — §2.2, one layer up. The audit reports + a required check whose only producers are `pull_request_target` jobs as + TARGET-ONLY. It cannot tell whether the job checks out the pull request, so + accepting one is a decision: put the job in the script's `ALLOW`, with the + reason. **Verify a rule by watching it reject, not by reading its configuration.** `git push --dry-run` does not evaluate server-side rules and will report success diff --git a/README.md b/README.md index 1f90afa..68035ee 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,8 @@ Org-level defaults for [kindspec](https://github.com/kindspec). requests gates no merge, a required check never reports or can be skipped, or a required check also comes from a `push` run on the pull request's branch, in the same workflow or another, or from a second workflow that runs - on the pull request (`pull_request` or `pull_request_target`); its mutation + on the pull request (`pull_request` or `pull_request_target`), or comes only + from `pull_request_target`, which runs the default branch's code; its mutation sweep is `scripts/sweep_check_required_checks.py`. `.github/workflows/required-checks-audit.yml` runs it daily, and on demand. - `profile/README.md` — the org landing page. diff --git a/STATE.md b/STATE.md index 63147ab..70ff689 100644 --- a/STATE.md +++ b/STATE.md @@ -245,7 +245,7 @@ DUPLICATE rule of .github#16, .github#18 and .github#20: $ python3 -I scripts/check_required_checks.py --public-only 1 private repositories skipped (--public-only) - 6 repositories, 7 workflows, 7 pull_request jobs, 7 required checks: 0 finding(s), 0 not evaluated + 6 repositories, 7 workflows, 7 pull request jobs, 7 required checks: 0 finding(s), 0 not evaluated .github#15 taught the script to expand a job that calls a reusable workflow into one ` / ` context per called job. A call into another @@ -286,7 +286,8 @@ DUPLICATE for `check`. **And a second producer on the pull request itself** (.github#20). Two active workflows that run a job of the same context on a pull request into the default branch, on `pull_request` or `pull_request_target`, are a DUPLICATE, reported -once per pair; so is one workflow on both events. A job there whose context it +once per pair whether or not `ALLOW` names either job; so is one workflow on +both events. A job there whose context it cannot resolve is refused, exit 2. The issue's reproduction, a `second.yml` with a job `check` added to kindkit in a `--save` snapshot, gives one DUPLICATE for `check` on `pull_request` and one on `pull_request_target`; main gives 0 @@ -294,24 +295,35 @@ findings for both. **`pull_request_target` reports on the pull request's head**, so the audit now counts it as a pull request event: a required check it alone produces is no -longer ORPHANED, and one it produces unrequired is UNGATED. Its check runs carry -the pull request's head sha: +longer ORPHANED, and one it produces unrequired is UNGATED. A required check +whose only producers are `pull_request_target` jobs is TARGET-ONLY instead, +exit 1: such a job runs on the default branch's code unless it checks out the +pull request, so it may be unable to fail because of the pull request, and the +audit cannot tell which. `ALLOW` on the job, with the reason, accepts one. The +`--public-only` run above has none; switching kindkit's `check.yml` from +`pull_request` to `pull_request_target` in a `--save` snapshot gives TARGET-ONLY +for `check` and `kind / conformance`, where main gave ORPHANED. + +`pull_request_target` check runs carry the pull request's head sha: python/cpython run 25066296099 (fork PR 149109, the issue's case), electron/electron run 37748122900 on PR 54550's head `a965781` (a same-repo branch), and react/react run 37730121633 on fork PR 37788's head `7af445b`. For the last two, `gh api repos///commits//check-runs` lists the job, and the GraphQL `statusCheckRollup` of the PR's last commit lists it with `event: pull_request_target`. GitHub's -"Troubleshooting required status checks" says: "For checks created by workflow +"Troubleshooting required status checks" (github/docs +`content/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks.md`, +lines 55–64 at `60f9f34`) says: "For checks created by workflow jobs to be evaluated for a pull request, the workflow run must be triggered by one of these events", and lists `push`, `pull_request`, `pull_request_review`, `pull_request_target`, `deployment` and `deployment_status`. It names `workflow_dispatch` on the head branch as one whose checks "do not satisfy a required status check in a branch ruleset". That a ruleset accepts a `pull_request_target` check is the documentation's word; no kindspec ruleset -has been watched doing it. A `pull_request_target` job runs the base branch's -workflow file with `GITHUB_SHA` the base's last commit, so a pull request cannot -change it, and it tests the pull request's code only if it checks that out. +has been watched doing it. A `pull_request_target` job runs the workflow file +from the default branch of the base repository, with `GITHUB_SHA` that branch's +last commit, so a pull request cannot change it, and it tests the pull request's +code only if it checks that out. **What it still does not see.** The same page also lists `pull_request_review`, `deployment` and `deployment_status` as events whose checks count; the audit diff --git a/scripts/check_required_checks.py b/scripts/check_required_checks.py index 8bfa15c..8726cd9 100644 --- a/scripts/check_required_checks.py +++ b/scripts/check_required_checks.py @@ -10,16 +10,19 @@ runs only on merge_group, push, on pull requests to other branches, or in a workflow that is not `active` does not count: the context never reports on the PR head - TRIGGER a required job's pull_request (or pull_request_target) `types:` + TRIGGER a required job's pull_request or pull_request_target `types:` omit opened or synchronize, so it does not report on every pull - request head + request head. With both events, only when the events that are not + path-filtered do not cover both types between them; each blocked + event is then named CONDITIONAL a required job can be skipped, and GitHub reports a skipped job as success: it has a job-level `if:`, or something in its `needs:` chain has one or is not itself required. When the skipped job calls a reusable workflow, its ' / ' contexts never report at all, and the merge waits on them instead PATH-FILTER a required job is path-filtered, so it never reports on an - unrelated pull request and blocks it forever (AGENTS.md 3.2) + unrelated pull request and blocks it forever (AGENTS.md 3.2). + With both events, on the same terms as TRIGGER DUPLICATE a required job's workflow also runs on push to some branch other than the default: no branch filter, a branches-ignore that leaves one, or branches that match one. A pull request head then carries @@ -34,16 +37,25 @@ pull request itself: the required job's workflow runs on both pull_request and pull_request_target, or another active workflow runs a job of the same context on either, into the default - branch. That pair is reported once, from the first workflow by path + branch. Each pair of workflows is reported once, whether or not + ALLOW names either job + TARGET-ONLY a required check's only producers are pull_request_target jobs. + Such a job runs the workflow from the default branch of the base + repository, on that branch's code unless it checks out the pull + request, so it may be unable to fail because of the pull request + (AGENTS.md 2.2). ALLOW on the job, with the reason, accepts it STALE-ALLOW an ALLOW entry matches no job any more pull_request_target counts as a pull request event: its check runs carry the -pull request's head sha, and GitHub lists it among the events whose checks -satisfy a required status check (kindspec/.github#20). It runs the workflow file -from the base branch, with GITHUB_SHA the base's last commit, so a pull request -cannot change what such a job does, and the job tests the pull request's code -only if it checks that out. A pull_request_target job that is meant to gate -nothing, such as a labeller, belongs in ALLOW with that reason. +pull request's head sha (kindspec/.github#20), and GitHub's documentation lists it +among the events whose checks satisfy a required status check (github/docs +content/pull-requests/how-tos/merge-and-close-pull-requests/ +troubleshooting-required-status-checks.md, lines 55-64 at 60f9f34). It runs the +workflow file from the default branch of the base repository, with GITHUB_SHA +that branch's last commit, so a pull request cannot change what such a job does, +and the job tests the pull request's code only if it checks that out. A +pull_request_target job that is meant to gate nothing, such as a labeller, +belongs in ALLOW with that reason. A job in ALLOW still produces its contexts. Exit 0 clean, 1 on any finding, 2 when something could not be evaluated: no repositories read; an API or authentication failure; fewer repositories listed @@ -103,8 +115,8 @@ ACTIONS_APP_ID = 15368 # GitHub Actions; a context from another app is not a workflow job DEFAULT_TYPES = {"opened", "synchronize"} # without both, a PR head can go unreported # Events whose check runs attach to a pull request's head commit and count toward its -# required checks. pull_request_target runs the base branch's workflow, but its check -# runs still carry the head sha (kindspec/.github#20). +# required checks. pull_request_target runs the workflow from the default branch of +# the base repository, but its check runs still carry the head sha (.github#20). PR_EVENTS = ("pull_request", "pull_request_target") SAMPLES = 4096 # names tried per branch filter pattern; a pattern with more is not enumerated GH = ["gh"] @@ -413,24 +425,32 @@ def pr_admits(on, branch): def pr_trigger(workflow, branch): - """(events that reach a PR into branch, path-filtered, restricted types, why not).""" + """(events that reach a PR into branch, [(finding, event, types)], why not). + + An event is blocked by a path filter, or by `types:` without opened and + synchronize. The blocks are findings only when the unfiltered events together + do not cover opened and synchronize: otherwise every head is still reported. + """ on = _events(workflow) events = pr_events(on, branch) if not events: why = "; ".join(f"{e} excludes '{branch}'" for e in PR_EVENTS if e in on) others = [e for e in ("merge_group", "push") if e in on] why = why or ("only " + ", ".join(others) if others else None) - return [], False, None, why - # a filter or a types: restriction matters only if every event that reaches the PR - # has one; otherwise another event reports the context on every head - configs = [on[e] if isinstance(on[e], dict) else {} for e in events] - filtered = all({"paths", "paths-ignore"} & set(c) for c in configs) - types = [c.get("types") for c in configs] - types = [None if t is None else set(map(str, t if isinstance(t, list) else [t])) for t in types] - restricted = None - if not any(t is None or DEFAULT_TYPES <= t for t in types): - restricted = sorted(set().union(*types)) - return events, filtered, restricted, None + return [], [], why + blocked, covered = [], set() + for event in events: + config = on[event] if isinstance(on[event], dict) else {} + types = config.get("types") + if types is not None: + types = set(map(str, types if isinstance(types, list) else [types])) + if {"paths", "paths-ignore"} & set(config): + blocked.append(("PATH-FILTER", event, None)) + else: + covered |= DEFAULT_TYPES if types is None else types + if types is not None and not DEFAULT_TYPES <= types: + blocked.append(("TRIGGER", event, sorted(types))) + return events, [] if DEFAULT_TYPES <= covered else blocked, None def jobs_of(workflow): @@ -582,7 +602,7 @@ def evaluate(snapshot, allow): for c in data["required"]: required.setdefault(c["context"], c.get("integration_id")) stats["required"] += len(required) - produced, elsewhere, blind = set(), {}, False + produced, elsewhere, blind, pr_events_of = set(), {}, False, {} on_push, push_unknown = producers(data, branch, push_duplicates) on_pr, pr_unknown = producers(data, branch, pr_admits) for path, text in sorted(data["workflows"].items()): @@ -594,7 +614,7 @@ def evaluate(snapshot, allow): continue try: workflow = parse_yaml(text) - events, filtered, types, why = pr_trigger(workflow, branch) + events, blocked, why = pr_trigger(workflow, branch) jobs = {j[0]: j for j in jobs_of(workflow)} except Unsupported as e: incomplete.append(f"{repo}: {path}: not evaluated: {e}") @@ -622,8 +642,13 @@ def evaluate(snapshot, allow): for job_id, context, _, _, unresolved, _ in jobs.values(): where = f"{repo}: {path}: job '{job_id}'" if (repo, path, job_id) in allow: + # it still reports its contexts; ALLOW only accepts what it gates stats["pr_jobs"] += 1 used.add((repo, path, job_id)) + contexts = ( + [c[1] for c in calls[job_id].values()] if job_id in calls else [context] + ) + produced.update(contexts) continue if unresolved: stats["pr_jobs"] += 1 @@ -640,6 +665,7 @@ def evaluate(snapshot, allow): for unit, context, cid, inner in units: stats["pr_jobs"] += 1 produced.add(context) + pr_events_of.setdefault(context, set()).update(events) if context not in required: findings.append( f"UNGATED {unit} runs on pull requests; " @@ -660,15 +686,6 @@ def evaluate(snapshot, allow): f"{' and '.join(events)}, so a pull request head can carry " f"'{context}' from two suites and the merge takes either" ) - # one finding per pair of workflows, from the first in path order - for other, job, why in on_pr.get(context, []): - if other > path: - findings.append( - f"DUPLICATE {unit} is required, and {other}: {job} also " - f"produces '{context}' on a pull request into '{branch}' ({why}), " - "so a pull request head can carry it from two suites and the " - "merge takes either" - ) # its own push is reported above; another workflow's push is this one for other, job, why in on_push.get(context, []): if other != path: @@ -698,16 +715,17 @@ def evaluate(snapshot, allow): f"{unit}: not evaluated for a pull request producer in another " "workflow: " + "; ".join(doubts) ) - if filtered: - findings.append( - f"PATH-FILTER {unit} is required but its pull_request " - "trigger is path-filtered" - ) - if types: - findings.append( - f"TRIGGER {unit} is required but pull_request types {types} " - "omit opened or synchronize" - ) + for kind, event, types in blocked: + if kind == "PATH-FILTER": + findings.append( + f"PATH-FILTER {unit} is required but its {event} " + "trigger is path-filtered" + ) + else: + findings.append( + f"TRIGGER {unit} is required but {event} types {types} " + "omit opened or synchronize" + ) skip = _skippable(job_id, jobs, required, calls) effect = "can be skipped, which reports success" if cid and skip: @@ -733,6 +751,24 @@ def evaluate(snapshot, allow): f"ORPHANED {repo}: required check '{context}' is produced by no " f"job that reports on a pull request into '{branch}'{note}" ) + elif pr_events_of.get(context) == {"pull_request_target"}: + findings.append( + f"TARGET-ONLY {repo}: required check '{context}' is produced only on " + "pull_request_target, which runs the default branch's workflow on the " + "default branch's code unless the job checks out the pull request, so " + "it may be unable to fail because of the pull request" + ) + # every pair of workflows producing it on a pull request, ALLOW or not + by_path = {} + for path, job, why in on_pr.get(context, []): + by_path.setdefault(path, (job, why)) + for a, b in itertools.combinations(sorted(by_path), 2): + findings.append( + f"DUPLICATE {repo}: required check '{context}' is produced on a pull " + f"request into '{branch}' by {a}: {by_path[a][0]} ({by_path[a][1]}) and by " + f"{b}: {by_path[b][0]} ({by_path[b][1]}), so a pull request head can carry " + "it from two suites and the merge takes either" + ) for key in sorted(set(allow) - used): findings.append(f"STALE-ALLOW {key} matches no pull_request job") if stats["repos"] == 0: @@ -1525,19 +1561,20 @@ def state_of(snap, path, state): ( "pull_request_target with an if:", repo(pr_target + " if: false\n"), - ["CONDITIONAL"], + ["CONDITIONAL", "TARGET-ONLY"], ), ( "pull_request_target types: [labeled]", repo(target(" pull_request_target:\n types: [labeled]\n")), - ["TRIGGER"], + ["TRIGGER", "TARGET-ONLY"], ), ( "pull_request_target path-filtered", repo(target(" pull_request_target:\n paths: [a]\n")), - ["PATH-FILTER"], + ["PATH-FILTER", "TARGET-ONLY"], ), - # with both events, a filter or types: matter only when both have one + # with both events, a filter or types: matter only when the unfiltered events + # together miss opened or synchronize; each blocked event is then named ( "pull_request path-filtered, pull_request_target not", repo(target(" pull_request:\n paths: [a]\n pull_request_target:\n")), @@ -1548,7 +1585,7 @@ def state_of(snap, path, state): repo( target(" pull_request:\n paths: [a]\n pull_request_target:\n paths: [a]\n") ), - ["DUPLICATE", "PATH-FILTER"], + ["DUPLICATE", "PATH-FILTER", "PATH-FILTER"], ), ( "pull_request types: [labeled], pull_request_target default", @@ -1563,7 +1600,55 @@ def state_of(snap, path, state): " pull_request_target:\n types: [labeled]\n" ) ), - ["DUPLICATE", "TRIGGER"], + ["DUPLICATE", "TRIGGER", "TRIGGER"], + ), + ( + "pull_request path-filtered, pull_request_target types: [labeled]", + repo( + target( + " pull_request:\n paths: [a]\n pull_request_target:\n types: [labeled]\n" + ) + ), + ["DUPLICATE", "PATH-FILTER", "TRIGGER"], + ), + ( + "pull_request types: [labeled], pull_request_target path-filtered", + repo( + target( + " pull_request:\n types: [labeled]\n pull_request_target:\n paths: [a]\n" + ) + ), + ["DUPLICATE", "TRIGGER", "PATH-FILTER"], + ), + ( + "types: [opened] and types: [synchronize] together cover every head", + repo( + target( + " pull_request:\n types: [opened]\n" + " pull_request_target:\n types: [synchronize]\n" + ) + ), + dup, + ), + ( + "path-filtered, with types that include opened and synchronize", + repo(target(" pull_request:\n paths: [a]\n types: [opened, synchronize]\n")), + ["PATH-FILTER"], + ), + ( + "scalar types: opened and synchronize cover every head", + repo( + target( + " pull_request:\n types: opened\n" + " pull_request_target:\n types: synchronize\n" + ) + ), + dup, + ), + ( + "two pull_request_target workflows", + beside(pr_target, pr_target), + ["DUPLICATE", "TARGET-ONLY"], ), # negative controls ( @@ -1589,7 +1674,7 @@ def state_of(snap, path, state): ( "other pull_request_target job with a different name", beside(target(job="lint"), None, lint), - [], + ["TARGET-ONLY"], ), ("other merge_group job of the same name", beside(target(" merge_group:\n")), []), ( @@ -1620,6 +1705,34 @@ def state_of(snap, path, state): inc1, ), ] + # ALLOW neither hides a pull request pair nor counts as a producer's absence + first = {("r", "a.yml", "check"): "test"} + pr_allow = [ + ( + "allowlisted first producer still pairs", + repo(pr_plain, more={"a.yml": pr_plain}), + dup, + first, + ), + ( + "allowlisted first pull_request_target producer still pairs", + repo(pr_plain, more={"a.yml": pr_target}), + dup, + first, + ), + ( + "three producers, first allowlisted", + repo(pr_plain, more={"a.yml": pr_plain, "m.yml": pr_plain}), + ["DUPLICATE", "DUPLICATE", "DUPLICATE"], + first, + ), + ( + "allowlisted pull_request_target-only producer is accepted", + repo(pr_target), + [], + {("r", "w.yml", "check"): "test"}, + ), + ] cases = [ ("clean", repo(_PR.format(job="check")), [], None), ("named job is clean", repo(_PR.format(job="j") + " name: check\n"), [], None), @@ -1662,9 +1775,9 @@ def state_of(snap, path, state): None, ), ( - "pull_request_target reports on the PR head", + "pull_request_target reports on the PR head, but only base code", repo("on: pull_request_target\njobs:\n check:\n runs-on: x\n"), - [], + ["TARGET-ONLY"], None, ), ( @@ -1875,6 +1988,7 @@ def state_of(snap, path, state): *((label, snap, want, None) for label, snap, want in duplicate), *((label, snap, want, None) for label, snap, want in cross), *((label, snap, want, None) for label, snap, want in pr_pair), + *pr_allow, ] bad = 0 @@ -1898,7 +2012,7 @@ def report(label, got, code, want, want_code): ) f, inc, stats = evaluate(snap, {}) got = [x.split()[0] for x in f] + ["INCOMPLETE"] * len(inc) + ["?"] * (stats["pr_jobs"] != 3) - report(f"3 pull_request jobs counted ({stats['pr_jobs']})", got, exit_code(f, inc), [], 0) + report(f"3 pull request jobs counted ({stats['pr_jobs']})", got, exit_code(f, inc), [], 0) # collection: failures exit 2, never 1 and never a crash pub = {"name": "p", "default_branch": "main", "archived": False, "private": False} @@ -2076,7 +2190,7 @@ def main(): print(f"{count} private repositories skipped (--public-only)") print( f"{stats['repos']} repositories, {stats['workflows']} workflows, " - f"{stats['pr_jobs']} pull_request jobs, {stats['required']} required checks: " + f"{stats['pr_jobs']} pull request jobs, {stats['required']} required checks: " f"{len(findings)} finding(s), {len(incomplete)} not evaluated" ) return exit_code(findings, incomplete) diff --git a/scripts/sweep_check_required_checks.py b/scripts/sweep_check_required_checks.py index 7d8aaa2..b027215 100644 --- a/scripts/sweep_check_required_checks.py +++ b/scripts/sweep_check_required_checks.py @@ -32,10 +32,7 @@ " elif context not in produced:\n", " elif False:\n", ), - ("path filter ignored", " if filtered:\n", " if False:\n"), ("paths-ignore ignored", '{"paths", "paths-ignore"}', '{"paths"}'), - ("restrictive types ignored", " if types:\n", " if False:\n"), - ("types check inverted", "DEFAULT_TYPES <= t for t", "not DEFAULT_TYPES <= t for t"), ( "conditional never reported", " if skip:\n", @@ -111,16 +108,6 @@ ' if k.split("@")[0] == uses.split("@")[0]), None)\n' ), ), - ( - "path filter ignored for called jobs", - " if filtered:\n", - " if filtered and not cid:\n", - ), - ( - "types ignored for called jobs", - " if types:\n", - " if types and not cid:\n", - ), ( "called jobs not counted", ' stats["pr_jobs"] += 1\n produced.add(context)\n', @@ -401,38 +388,12 @@ "on_pr, pr_unknown = producers(data, branch, pr_admits)", "on_pr, pr_unknown = {}, []", ), - ( - "cross-workflow pr duplicate never reported", - "for other, job, why in on_pr.get(context, []):", - "for other, job, why in []:", - ), - ( - "pr pair reported from both sides", - " if other > path:\n", - (" if other != path:\n"), - ), - ("own job counted as its own pr producer", "if other > path:", "if other >= path:"), ("two-event workflow not flagged", "if len(events) > 1:", "if False:"), ( "pr branch filter ignored elsewhere", ' events = pr_events(on, branch)\n return "on "', ' events = [e for e in PR_EVENTS if e in on]\n return "on "', ), - ( - "path filter on one event of two counted", - 'filtered = all({"paths", "paths-ignore"}', - 'filtered = any({"paths", "paths-ignore"}', - ), - ( - "path filter read from the first event only", - "set(c) for c in configs)", - "set(c) for c in configs[:1])", - ), - ( - "types on one event of two counted", - "if not any(t is None or DEFAULT_TYPES <= t for t in types):", - "if not all(t is None or DEFAULT_TYPES <= t for t in types):", - ), ( "own undecided pr refused again", "in pr_unknown\n if other != path and c in (None, context)", @@ -470,6 +431,81 @@ 'if data["workflow_state"].get(path) != "active":', 'if data["workflow_state"].get(path) == "disabled_manually":', ), + # PATH-FILTER and TRIGGER, per event; a covered head clears them (#21 review) + ("path filter ignored", 'blocked.append(("PATH-FILTER", event, None))', "pass"), + ("restrictive types ignored", 'blocked.append(("TRIGGER", event, sorted(types)))', "pass"), + ( + "types check inverted", + "if types is not None and not DEFAULT_TYPES <= types:", + "if types is not None and DEFAULT_TYPES <= types:", + ), + ( + "filtered event counts as covering", + " else:\n covered |=", + " if True:\n covered |=", + ), + ("covering ignored", "[] if DEFAULT_TYPES <= covered else blocked", "blocked"), + ( + "default types do not cover", + "covered |= DEFAULT_TYPES if types is None else types", + "covered |= set() if types is None else types", + ), + ( + "blocks ignored for called jobs", + " for kind, event, types in blocked:\n", + " for kind, event, types in [] if cid else blocked:\n", + ), + ("every block named a path filter", 'if kind == "PATH-FILTER":', "if True:"), + ( + "types with opened and synchronize counted as restricted", + "if types is not None and not DEFAULT_TYPES <= types:", + "if types is not None:", + ), + ( + "scalar types not wrapped", + "set(map(str, types if isinstance(types, list) else [types]))", + "set(map(str, types))", + ), + # pull request pairs, independent of ALLOW (#21 review) + ( + "pr pairs never reported", + "for a, b in itertools.combinations(sorted(by_path), 2):", + "for a, b in []:", + ), + ( + "pr pairs only between the first two", + "itertools.combinations(sorted(by_path), 2)", + "itertools.combinations(sorted(by_path)[:2], 2)", + ), + ( + "allowlisted workflow dropped from pr pairs", + " for path, job, why in on_pr.get(context, []):\n", + ( + " for path, job, why in [e for e in on_pr.get(context, [])" + " if not any(k[:2] == (repo, e[0]) for k in allow)]:\n" + ), + ), + # TARGET-ONLY (#21 review) + ( + "target-only never reported", + 'elif pr_events_of.get(context) == {"pull_request_target"}:', + "elif False:", + ), + ( + "target-only with another event too", + 'elif pr_events_of.get(context) == {"pull_request_target"}:', + 'elif "pull_request_target" in pr_events_of.get(context, ()):', + ), + ("pr events not recorded", "pr_events_of.setdefault(context, set()).update(events)", "pass"), + ("allowlisted job not a producer", " produced.update(contexts)\n", ""), + ( + "allowlisted job counted for target-only", + " produced.update(contexts)\n", + ( + " produced.update(contexts)\n" + ' pr_events_of.setdefault(context, set()).add("pull_request_target")\n' + ), + ), ] TIMEOUT = 30 # seconds per mutant; the self-test itself takes well under one From 963ac707408d7bb5014e34198f509a56c17aa74a Mon Sep 17 00:00:00 2001 From: Cameron Brooks Date: Thu, 8 Oct 2026 04:54:18 -0400 Subject: [PATCH 3/3] fix: scope allow to ungated and accept target-only through target_ok An allowlisted job skipped every check, so accepting a TARGET-ONLY check through ALLOW also silenced that required job's CONDITIONAL, TRIGGER, PATH-FILTER and DUPLICATE findings. ALLOW now silences only UNGATED, for a job that gates nothing; a required job in it is checked as usual. A new TARGET_OK, keyed by (repo, context), accepts TARGET-ONLY and nothing else. TARGET-ONLY no longer fires when a push run on a pull request's branch also produces the check: that run tests the pull request's code, and the shape is already a DUPLICATE. The message now says the check is reported on a pull request only by pull_request_target. Self-test cases now pin types: [] as a TRIGGER (GitHub does not document it, so the audit reads it fail-closed), a lone types: [opened] as a TRIGGER, and pairs whose producers are all allowlisted. Signed-off-by: Cameron Brooks --- AGENTS.md | 11 +- README.md | 3 +- STATE.md | 6 +- scripts/check_required_checks.py | 150 +++++++++++++++++++------ scripts/sweep_check_required_checks.py | 61 ++++++++-- 5 files changed, 181 insertions(+), 50 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bc0514d..fbf1872 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -195,10 +195,13 @@ Each is the pattern in §2.2, one layer up in the infrastructure. file from the default branch of the base repository, on that branch's code unless it checks out the pull request, so the pull request can neither change the job nor, by default, make it fail — §2.2, one layer up. The audit reports - a required check whose only producers are `pull_request_target` jobs as - TARGET-ONLY. It cannot tell whether the job checks out the pull request, so - accepting one is a decision: put the job in the script's `ALLOW`, with the - reason. + a required check whose only pull request producers are `pull_request_target` + jobs as TARGET-ONLY, unless a `push` run on the pull request's branch also + produces it (that run tests the pull request's code, and is a DUPLICATE). It + cannot tell whether the job checks out the pull request, so accepting one is + a decision: add `(repo, context)` to the script's `TARGET_OK`, with the + reason. `ALLOW` does not accept it: `ALLOW` only silences UNGATED, for a job + that gates nothing. **Verify a rule by watching it reject, not by reading its configuration.** `git push --dry-run` does not evaluate server-side rules and will report success diff --git a/README.md b/README.md index 68035ee..ad0138e 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,8 @@ Org-level defaults for [kindspec](https://github.com/kindspec). or a required check also comes from a `push` run on the pull request's branch, in the same workflow or another, or from a second workflow that runs on the pull request (`pull_request` or `pull_request_target`), or comes only - from `pull_request_target`, which runs the default branch's code; its mutation + from `pull_request_target`, which runs the default branch's code (accepted + per check through `TARGET_OK`); its mutation sweep is `scripts/sweep_check_required_checks.py`. `.github/workflows/required-checks-audit.yml` runs it daily, and on demand. - `profile/README.md` — the org landing page. diff --git a/STATE.md b/STATE.md index 70ff689..1560c87 100644 --- a/STATE.md +++ b/STATE.md @@ -299,7 +299,11 @@ longer ORPHANED, and one it produces unrequired is UNGATED. A required check whose only producers are `pull_request_target` jobs is TARGET-ONLY instead, exit 1: such a job runs on the default branch's code unless it checks out the pull request, so it may be unable to fail because of the pull request, and the -audit cannot tell which. `ALLOW` on the job, with the reason, accepts one. The +audit cannot tell which. An entry in the script's `TARGET_OK`, with the reason, +accepts one; `ALLOW` does not, because it silences only UNGATED, and a required +job in it is checked as usual. A `push` run on the pull request's branch that +also produces the check tests its code, so that is a DUPLICATE and not +TARGET-ONLY. The `--public-only` run above has none; switching kindkit's `check.yml` from `pull_request` to `pull_request_target` in a `--save` snapshot gives TARGET-ONLY for `check` and `kind / conformance`, where main gave ORPHANED. diff --git a/scripts/check_required_checks.py b/scripts/check_required_checks.py index 8726cd9..bd556dc 100644 --- a/scripts/check_required_checks.py +++ b/scripts/check_required_checks.py @@ -14,7 +14,8 @@ omit opened or synchronize, so it does not report on every pull request head. With both events, only when the events that are not path-filtered do not cover both types between them; each blocked - event is then named + event is then named. `types: []` counts as no types (GitHub does + not document it), so it is a TRIGGER CONDITIONAL a required job can be skipped, and GitHub reports a skipped job as success: it has a job-level `if:`, or something in its `needs:` chain has one or is not itself required. When the skipped job calls @@ -39,11 +40,14 @@ runs a job of the same context on either, into the default branch. Each pair of workflows is reported once, whether or not ALLOW names either job - TARGET-ONLY a required check's only producers are pull_request_target jobs. - Such a job runs the workflow from the default branch of the base - repository, on that branch's code unless it checks out the pull - request, so it may be unable to fail because of the pull request - (AGENTS.md 2.2). ALLOW on the job, with the reason, accepts it + TARGET-ONLY a required check's only pull request producers are + pull_request_target jobs, and no push run on a pull request's + branch produces it (that one would test the pull request's code, + and is a DUPLICATE). Such a job runs the workflow from the default + branch of the base repository, on that branch's code unless it + checks out the pull request, so it may be unable to fail because + of the pull request (AGENTS.md 2.2). An entry in TARGET_OK, with + the reason, accepts it, and only it STALE-ALLOW an ALLOW entry matches no job any more pull_request_target counts as a pull request event: its check runs carry the @@ -55,7 +59,8 @@ that branch's last commit, so a pull request cannot change what such a job does, and the job tests the pull request's code only if it checks that out. A pull_request_target job that is meant to gate nothing, such as a labeller, -belongs in ALLOW with that reason. A job in ALLOW still produces its contexts. +belongs in ALLOW with that reason. ALLOW silences only UNGATED: a job in it +still produces its contexts, and if one is required it is checked as usual. Exit 0 clean, 1 on any finding, 2 when something could not be evaluated: no repositories read; an API or authentication failure; fewer repositories listed @@ -109,9 +114,14 @@ # Jobs that run on pull requests but deliberately gate nothing, as # (repo, workflow path, job id): reason. Jobs that never run on a pull request # (rowspec's release.yml: gate, build, publish run on v* tags) are excluded by -# their triggers and need no entry here. +# their triggers and need no entry here. An entry only silences UNGATED: a +# required job in ALLOW is checked like any other. ALLOW = {} +# Required checks accepted although only pull_request_target produces them, as +# (repo, context): reason, such as "the job checks out the pull request head". +TARGET_OK = {} + ACTIONS_APP_ID = 15368 # GitHub Actions; a context from another app is not a workflow job DEFAULT_TYPES = {"opened", "synchronize"} # without both, a PR head can go unreported # Events whose check runs attach to a pull request's head commit and count toward its @@ -583,7 +593,7 @@ def _skippable(job_id, jobs, required, calls, seen=frozenset()): return None -def evaluate(snapshot, allow): +def evaluate(snapshot, allow, target_ok=()): findings, incomplete, used = [], [], set() stats = {"repos": 0, "workflows": 0, "pr_jobs": 0, "required": 0} for repo, data in sorted(snapshot.items()): @@ -641,14 +651,11 @@ def evaluate(snapshot, allow): calls[job_id] = called for job_id, context, _, _, unresolved, _ in jobs.values(): where = f"{repo}: {path}: job '{job_id}'" - if (repo, path, job_id) in allow: - # it still reports its contexts; ALLOW only accepts what it gates - stats["pr_jobs"] += 1 + allowed = (repo, path, job_id) in allow + if allowed: used.add((repo, path, job_id)) - contexts = ( - [c[1] for c in calls[job_id].values()] if job_id in calls else [context] - ) - produced.update(contexts) + if unresolved and allowed: + stats["pr_jobs"] += 1 continue if unresolved: stats["pr_jobs"] += 1 @@ -667,10 +674,11 @@ def evaluate(snapshot, allow): produced.add(context) pr_events_of.setdefault(context, set()).update(events) if context not in required: - findings.append( - f"UNGATED {unit} runs on pull requests; " - f"'{context}' is not a required check" - ) + if not allowed: + findings.append( + f"UNGATED {unit} runs on pull requests; " + f"'{context}' is not a required check" + ) continue if pushed: findings.append( @@ -751,12 +759,17 @@ def evaluate(snapshot, allow): f"ORPHANED {repo}: required check '{context}' is produced by no " f"job that reports on a pull request into '{branch}'{note}" ) - elif pr_events_of.get(context) == {"pull_request_target"}: + # a push run on the pull request's branch tests its code; that is DUPLICATE + elif ( + pr_events_of.get(context) == {"pull_request_target"} + and context not in on_push + and (repo, context) not in target_ok + ): findings.append( - f"TARGET-ONLY {repo}: required check '{context}' is produced only on " - "pull_request_target, which runs the default branch's workflow on the " - "default branch's code unless the job checks out the pull request, so " - "it may be unable to fail because of the pull request" + f"TARGET-ONLY {repo}: required check '{context}' is reported on a pull " + "request only by pull_request_target, which runs the default branch's " + "workflow on the default branch's code unless the job checks out the pull " + "request, so it may be unable to fail because of the pull request" ) # every pair of workflows producing it on a pull request, ALLOW or not by_path = {} @@ -904,7 +917,7 @@ def run_live(org, public_only, save=None): if save: with open(save, "w") as fh: json.dump(snapshot, fh, indent=1, sort_keys=True) - return (*evaluate(snapshot, ALLOW), skipped) + return (*evaluate(snapshot, ALLOW, TARGET_OK), skipped) # --- self-test: every finding and every refusal must be seen to fire --- @@ -939,9 +952,9 @@ def api(path): def selftest(): - def run(snapshot, allow=None): + def run(snapshot, allow=None, *target_ok): try: - f, inc, _ = evaluate(snapshot, allow or {}) + f, inc, _ = evaluate(snapshot, allow or {}, *target_ok) except Exception as e: # noqa: BLE001 -- a crash is a failed case return [f"CRASH {type(e).__name__}"], -1 return [x.split()[0] for x in f] + ["INCOMPLETE"] * len(inc), exit_code(f, inc) @@ -1645,6 +1658,13 @@ def state_of(snap, path, state): ), dup, ), + # types: [] is read as no types at all; GitHub does not document it + ("types: []", repo(target(" pull_request:\n types: []\n")), ["TRIGGER"]), + ( + "types: [opened] alone", + repo(target(" pull_request:\n types: [opened]\n")), + ["TRIGGER"], + ), ( "two pull_request_target workflows", beside(pr_target, pr_target), @@ -1707,6 +1727,8 @@ def state_of(snap, path, state): ] # ALLOW neither hides a pull request pair nor counts as a producer's absence first = {("r", "a.yml", "check"): "test"} + mine = {("r", "w.yml", "check"): "test"} + ok = {("r", "check"): "test"} pr_allow = [ ( "allowlisted first producer still pairs", @@ -1727,10 +1749,70 @@ def state_of(snap, path, state): first, ), ( - "allowlisted pull_request_target-only producer is accepted", - repo(pr_target), + "every producer allowlisted still pairs", + repo(pr_plain, more={"a.yml": pr_plain}), + dup, + {**first, ("r", "w.yml", "check"): "test"}, + ), + # ALLOW silences UNGATED only; TARGET_OK accepts TARGET-ONLY, and nothing else + ("ALLOW does not accept target-only", repo(pr_target), ["TARGET-ONLY"], mine), + ( + "allowlisted matrix job gating nothing is not refused", + repo(pr_plain + " strategy:\n matrix:\n v: [1]\n", []), [], - {("r", "w.yml", "check"): "test"}, + mine, + ), + ( + "ALLOW on a required job keeps its own push DUPLICATE", + repo(target(" pull_request_target:\n push:\n")), + dup, + mine, + ), + ( + "ALLOW on a required job keeps its CONDITIONAL", + repo(pr_target + " if: false\n"), + ["CONDITIONAL", "TARGET-ONLY"], + mine, + ), + ( + "ALLOW on a required pull_request job: target-only needs every producer", + repo(pr_target, more={"a.yml": pr_plain}), + dup, + first, + ), + ("TARGET_OK accepts target-only", repo(pr_target), [], None, ok), + ( + "TARGET_OK keeps CONDITIONAL", + repo(pr_target + " if: false\n"), + ["CONDITIONAL"], + None, + ok, + ), + ( + "TARGET_OK for another context accepts nothing", + repo(pr_target), + ["TARGET-ONLY"], + None, + {("r", "lint"): "test"}, + ), + # a push run on the pull request's branch tests its code: DUPLICATE, not TARGET-ONLY + ( + "pull_request_target and push in one workflow", + repo(target(" pull_request_target:\n push:\n")), + dup, + None, + ), + ( + "pull_request_target, and push in another workflow", + beside(push_job(), pr_target), + dup, + None, + ), + ( + "pull_request_target, push to the default branch only elsewhere", + beside(push_job(on="\n push:\n branches: [main]\n"), pr_target), + ["TARGET-ONLY"], + None, ), ] cases = [ @@ -1999,8 +2081,8 @@ def report(label, got, code, want, want_code): mark = "ok " if ok else "FAIL" print(f"{mark} {label}: got {got} exit {code}, want {want} exit {want_code}") - for label, snap, want, allow in cases: - got, code = run(snap, allow) + for label, snap, want, allow, *target_ok in cases: + got, code = run(snap, allow, *target_ok) want_code = 1 if set(want) - {"INCOMPLETE"} else 2 if want else 0 # not via exit_code report(label, got, code, want, want_code) @@ -2178,7 +2260,7 @@ def main(): skipped = 0 if args.load: with open(args.load) as fh: - findings, incomplete, stats = evaluate(json.load(fh), ALLOW) + findings, incomplete, stats = evaluate(json.load(fh), ALLOW, TARGET_OK) else: findings, incomplete, stats, skipped = run_live(args.org, args.public_only, args.save) for line in findings: diff --git a/scripts/sweep_check_required_checks.py b/scripts/sweep_check_required_checks.py index b027215..75edebf 100644 --- a/scripts/sweep_check_required_checks.py +++ b/scripts/sweep_check_required_checks.py @@ -486,24 +486,65 @@ ), ), # TARGET-ONLY (#21 review) + ("pr events not recorded", "pr_events_of.setdefault(context, set()).update(events)", "pass"), ( "target-only never reported", - 'elif pr_events_of.get(context) == {"pull_request_target"}:', - "elif False:", + ' pr_events_of.get(context) == {"pull_request_target"}\n', + " False\n", ), ( "target-only with another event too", - 'elif pr_events_of.get(context) == {"pull_request_target"}:', - 'elif "pull_request_target" in pr_events_of.get(context, ()):', + ' pr_events_of.get(context) == {"pull_request_target"}\n', + ' "pull_request_target" in pr_events_of.get(context, ())\n', ), - ("pr events not recorded", "pr_events_of.setdefault(context, set()).update(events)", "pass"), - ("allowlisted job not a producer", " produced.update(contexts)\n", ""), + # ALLOW silences UNGATED only; TARGET_OK accepts TARGET-ONLY only (#21 re-review) + ( + "allowlisted job skips every check", + " if allowed:\n used.add((repo, path, job_id))\n", + ( + " if allowed:\n used.add((repo, path, job_id))\n" + " continue\n" + ), + ), + ( + "allow does not silence ungated", + " if not allowed:\n", + " if True:\n", + ), + ( + "allowlisted unresolved job refused", + " if unresolved and allowed:\n", + " if False:\n", + ), + ("target_ok ignored", " and (repo, context) not in target_ok\n", ""), ( - "allowlisted job counted for target-only", - " produced.update(contexts)\n", + "target_ok matched by repo only", + "and (repo, context) not in target_ok", + "and repo not in {k[0] for k in target_ok}", + ), + ( + "push producer does not clear target-only", + " and context not in on_push\n", + "", + ), + # the reviewer's S11, S12, S14 + ( + "types: [] reads as unrestricted", + "if types is not None and not DEFAULT_TYPES <= types:", + "if types and not DEFAULT_TYPES <= types:", + ), + ( + "coverage needs only one default type", + "[] if DEFAULT_TYPES <= covered else blocked", + "[] if DEFAULT_TYPES & covered else blocked", + ), + ( + "pairs skipped when every producer is allowlisted", + " for path, job, why in on_pr.get(context, []):\n", ( - " produced.update(contexts)\n" - ' pr_events_of.setdefault(context, set()).add("pull_request_target")\n' + " for path, job, why in on_pr.get(context, []):\n" + " if all(any(k[:2] == (repo, e[0]) for k in allow) for e in on_pr[context]):\n" + " continue\n" ), ), ]