From f7f27a02c57052cb6470543eb6ee34da378252dc Mon Sep 17 00:00:00 2001 From: Lucas Koontz Date: Thu, 27 Aug 2026 17:14:09 -0700 Subject: [PATCH] fix(cla-assistant): make the allowlist bots only, and default it here (ENG-2017) Staff sign like everybody else now. The allowlist defaults in this file and a wrapper stops passing one. Fifteen repos carried six different hand-maintained lists naming 25 people between them, and 11 of those 25 had already left the org, so each was a former colleague still exempt from signing a CLA on GPL-3.0 and Elastic-2.0 code. A list of people is an access-control list: it needs an owner and a review, and nobody was going to give six copies either. One comment from an employee is cheaper than that, and it produces a signature instead of an assumption that an employment agreement was in place and covered the work. Bots stay, because a bot cannot sign. It cannot post the agreement sentence, so with no exemption its pull request is red forever, and lightwood alone has 36 Dependabot pull requests in its last 100. Only the two that actually open pull requests in this org are listed. github-actions[bot] needs no entry: the action already drops user id 41898282 in graphql.ts. Dropping the human entries also removes two live defects. checkAllowList.ts compares non-wildcard entries with `pattern === committer`, exact and case-sensitive, so every list's `Stpmax` never once matched the real login `StpMax`. And `bot*` compiled to an UNANCHORED `new RegExp("bot.*").test(login)`, which matches any login containing "bot": `robotnik`, `sabotage` and `elliotbotson` were all exempt from the CLA in all fifteen repos. Naming the two bots removes the wildcard and the hole with it. tests/test_cla_allowlist.py pins all three: no wildcard, no humans, and the lookalikes stay unexempt. Against the old list it fails 8 of its 11 cases. --- .github/workflows/cla-assistant.yml | 32 +++++++++- README.md | 27 ++++++++- tests/test_cla_allowlist.py | 92 +++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 tests/test_cla_allowlist.py diff --git a/.github/workflows/cla-assistant.yml b/.github/workflows/cla-assistant.yml index 87ccff7..3792724 100644 --- a/.github/workflows/cla-assistant.yml +++ b/.github/workflows/cla-assistant.yml @@ -25,7 +25,9 @@ # uses: mindsdb/github-actions/.github/workflows/cla-assistant.yml@ # v1 # with: # path-to-document: 'https://github.com/mindsdb/mindshub/blob/main/assets/contributions-agreement/individual-contributor.md' -# allowlist: bot*, ZoranPandovski, ... +# +# That is the whole wrapper. `allowlist`, `path-to-signatures` and `branch` all +# default, so a caller passes the agreement URL and nothing else. # # The caller must declare those four permissions: the action writes the # signature file, comments on the PR, and sets the commit status, and a called @@ -46,6 +48,30 @@ # exist; and its own handler for a missing FILE is dead code, because it # compares `error.status` against the string `"404"`. A repo adopting this # workflow would otherwise fail on its first signature either way. +# +# THE ALLOWLIST IS BOTS, NOT PEOPLE, AND IT LIVES HERE RATHER THAN PER REPO. +# +# Staff sign like everybody else. It is one comment, it produces an actual +# signed record instead of an assumption that somebody's employment agreement +# was in place and covered the work, and it removes a list that nobody prunes. +# Fifteen repos previously carried six different hand-maintained lists naming +# 25 people between them, and 11 of those 25 had already left the org, so each +# was a former colleague still exempt from signing. +# +# Bots stay, because a bot cannot sign: it cannot post the agreement sentence, +# so with no exemption its pull request is red forever. Only the two that +# actually open pull requests here are listed. `github-actions[bot]` needs no +# entry, since the action filters user id 41898282 in `graphql.ts` itself. +# +# Add a bot by editing the default below, not by passing the input. Anything +# passed per repo drifts, which is the whole reason this moved. +# +# Never use a `*` wildcard here. The action turns `bot*` into +# `new RegExp("bot.*").test(login)`, which is unanchored and searches anywhere +# in the string, so `bot*` also exempts `robotnik`, `sabotage` and +# `elliotbotson`. Anyone could opt out of the CLA by choosing a username. Every +# entry is an exact, case-sensitive login: matching is `pattern === committer`, +# so `Stpmax` never matched `StpMax` in any of the old lists. name: CLA Assistant @@ -57,9 +83,9 @@ on: type: string required: true allowlist: - description: "Comma-separated logins exempt from signing (bots, employees)" + description: "Comma-separated logins exempt from signing. Bots only; see the header. Override only for a genuine one-off." type: string - required: true + default: 'dependabot[bot], mindsdb-release-train[bot]' path-to-signatures: description: "Signature ledger path inside the calling repo" type: string diff --git a/README.md b/README.md index d1c3290..a9f8b07 100644 --- a/README.md +++ b/README.md @@ -358,7 +358,7 @@ the group name) and skipping auto-version commits with `cla-assistant.yml` runs the contributor-agreement check on the public repos. The wrapper keeps the `issue_comment` + `pull_request_target` triggers (the action reads those payloads directly) and declares the permissions, and passes the -per-repo agreement URL and allowlist: +agreement URL. That is the whole wrapper: ```yaml permissions: @@ -372,7 +372,6 @@ jobs: uses: mindsdb/github-actions/.github/workflows/cla-assistant.yml@ # v1 with: path-to-document: 'https://github.com/mindsdb/mindshub/blob/main/assets/contributions-agreement/individual-contributor.md' - allowlist: bot*, ZoranPandovski, ... ``` `mindsdb/mindshub` is the repository that holds the agreement. `mindsdb/mindsdb` @@ -381,6 +380,30 @@ neither belongs in a new caller: a redirect stops being harmless the moment somebody creates a repository at the old name, and this is the page a contributor reads before agreeing to it. +**Staff sign like everybody else, and the allowlist is bots.** It defaults here +and a wrapper does not pass it. Fifteen repos used to carry six different +hand-maintained lists naming 25 people between them, and by the time anyone +counted, 11 of those 25 had left the org and were still exempt from signing. A +list of people is an access-control list, so it needs an owner and a review, and +nobody was ever going to give six copies either. Asking an employee for one +comment is cheaper, and it produces a signature rather than an assumption that +an employment agreement was in place and covered the work. + +Bots stay, because a bot cannot sign. It cannot post the agreement sentence, so +without an exemption its pull request is red forever, and `lightwood` alone has +36 Dependabot pull requests in its last 100. Two are listed, `dependabot[bot]` +and `mindsdb-release-train[bot]`, which are the only two that open pull requests +anywhere in the fleet. `github-actions[bot]` needs no entry: the action already +filters user id 41898282 in `graphql.ts`. + +**Never put a `*` in that list.** The action compiles `bot*` to +`new RegExp("bot.*")` and calls `.test()`, which is unanchored and searches +anywhere in the string, so `bot*` also exempts `robotnik`, `sabotage` and +`elliotbotson`. That is an opt-out anybody can grant themselves by choosing a +username. Every entry is an exact login and matching is case-sensitive +`pattern === committer`, which is why the old lists' `Stpmax` never once matched +`StpMax`. `tests/test_cla_allowlist.py` fails the build on either mistake. + Signatures are committed to the calling repo's own `cla` branch, so each repo keeps its own ledger. `path-to-signatures` and `branch` default to that shape; pass them only where a repo already keeps its ledger somewhere else. diff --git a/tests/test_cla_allowlist.py b/tests/test_cla_allowlist.py new file mode 100644 index 0000000..78ca8fb --- /dev/null +++ b/tests/test_cla_allowlist.py @@ -0,0 +1,92 @@ +"""Guards on ``cla-assistant.yml``'s allowlist default. + +The allowlist decides who is exempt from signing the contributor agreement, so a +wrong entry is a legal gap rather than a broken build, and nothing else fails +when it drifts. Three real defects sat in it across fifteen repos before anyone +looked, and each one is pinned below. + +The matching logic these tests encode lives in the action, not here: +``checkAllowList.ts`` compiles an entry containing ``*`` to +``new RegExp(escaped).test(committer)`` and compares every other entry with +``pattern === committer``. Both halves matter, and both are quoted in the tests +that depend on them. +""" + +import re +from pathlib import Path + +import pytest +import yaml + +_WORKFLOW = ( + Path(__file__).resolve().parents[1] / ".github" / "workflows" / "cla-assistant.yml" +) + +# github-actions[bot] never reaches the allowlist: the action drops user id +# 41898282 in graphql.ts before the check runs, so an entry for it is dead +# weight that reads like a decision. +FILTERED_UPSTREAM = "github-actions[bot]" + + +def allowlist_default() -> str: + spec = yaml.safe_load(_WORKFLOW.read_text(encoding="utf-8")) + # PyYAML resolves the bare `on:` key to True, so accept either spelling. + triggers = spec.get("on", spec.get(True)) + return triggers["workflow_call"]["inputs"]["allowlist"]["default"] + + +def entries() -> list[str]: + return [e.strip() for e in allowlist_default().split(",") if e.strip()] + + +def test_the_default_exists_so_no_caller_has_to_pass_one(): + """A caller passing its own list is how six copies drifted apart.""" + assert allowlist_default() + + +def test_no_wildcards(): + """`bot*` becomes an UNANCHORED `bot.*`, so it also exempts `robotnik`. + + `checkAllowList.ts` builds `new RegExp(pattern.replace('*', '.*'))` and calls + `.test()`, which searches anywhere in the string. Anyone could opt out of the + CLA by picking a username containing the pattern. + """ + assert [e for e in entries() if "*" in e] == [] + + +def test_every_entry_is_a_bot(): + """People sign. A list of people is an access-control list nobody prunes. + + Eleven of the twenty-five names in the old per-repo lists had already left + the org and were still exempt. + """ + assert [e for e in entries() if not e.endswith("[bot]")] == [] + + +def test_github_actions_bot_is_not_listed(): + assert FILTERED_UPSTREAM not in entries() + + +def test_no_duplicates(): + assert len(entries()) == len(set(entries())) + + +@pytest.mark.parametrize( + "impostor", ["robotnik", "sabotage", "elliotbotson", "dependabot-impostor"] +) +def test_the_default_does_not_exempt_a_lookalike(impostor): + """Replays the action's own matcher against logins a wildcard would have let through.""" + + def action_matches(pattern: str, committer: str) -> bool: + pattern = pattern.strip() + if "*" in pattern: + return re.search(re.escape(pattern).replace(r"\*", ".*"), committer) is not None + return pattern == committer + + assert not any(action_matches(e, impostor) for e in entries()) + + +@pytest.mark.parametrize("bot", ["dependabot[bot]", "mindsdb-release-train[bot]"]) +def test_the_bots_that_actually_open_pull_requests_stay_exempt(bot): + """A bot cannot post the agreement sentence, so dropping it means a permanent red check.""" + assert bot in entries()