fix(cla-assistant): make the allowlist bots only, and default it here (ENG-2017)

.github/workflows/cla-assistant.yml

@@ -25,7 +25,9 @@
 #         uses: mindsdb/github-actions/.github/workflows/cla-assistant.yml@<sha> # 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

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@<sha> # 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.

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