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()