From 4b074a73af25894de3311fe2f4222c6bfbb4b7c3 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Fri, 18 Sep 2026 11:07:07 -0500 Subject: [PATCH 1/4] chore(compliance): remove dead workflow-override section from the CLA allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The allowlist's Section A (org_members, dco_on_permissive, users, bots, teams, temporary_exemptions) was read only by reusable-cla-check.yml, which was decommissioned when policy_selector.py took over enforcement. Nothing has read those keys since, so the section described a policy the gate was not applying. That mattered beyond tidiness: `users:` listed a contributor who has since left the company, and the file read as though that account held a standing bypass of the Legal Compliance Gate. It did not — the gate's only bypasses are BOT_ALLOWLIST/"[bot]" suffix and a live org-membership check, both in policy_selector.process_single_pr() — but an allowlist that appears to grant bypasses it cannot grant is worse than no allowlist, because it invites someone to trust it during an audit. Section B (license_overrides) is the only part the live code reads and is unchanged, byte for byte. Verified behaviour-neutral by running requires_cla._override_requires_cla() against the old and new files for MIT, Apache-2.0, GPL-2.0, LicenseRef-Broadcom_Source_Available and LicenseRef-Broadcom_Internal — identical results. Full suite: 57 tests, OK. The header comment now points readers at policy_selector.py for the two real bypasses, so the next person looking for "who skips the gate" finds the code that decides it rather than a file that no longer does. Co-Authored-By: Claude Opus 5 (1M context) --- cla/allowlist.yml | 54 ++++++++++++++--------------------------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/cla/allowlist.yml b/cla/allowlist.yml index 49a0bc3..2a486cb 100644 --- a/cla/allowlist.yml +++ b/cla/allowlist.yml @@ -1,45 +1,23 @@ # .github/cla/allowlist.yml -# Purpose: (A) workflow overrides for CLA/DCO, (B) license overrides for requires_cla.py +# Purpose: license-policy overrides for requires_cla.py. +# +# This file previously also carried a "workflow overrides" section +# (org_members, dco_on_permissive, users, bots, teams, temporary_exemptions). +# Those keys were read only by reusable-cla-check.yml, which was decommissioned +# when policy_selector.py took over enforcement. No live code has read them +# since, so they were removed rather than left looking authoritative — an +# allowlist that appears to grant standing gate bypasses, but doesn't, is worse +# than no allowlist at all. +# +# Who actually skips the gate today is decided in scripts/policy_selector.py, +# not here. There are exactly two bypasses, both in process_single_pr(): +# * bots — BOT_ALLOWLIST, plus any login ending in "[bot]" +# * org members — is_org_member(), checked live against the GitHub API +# To change either, change that script. # ----------------------------- -# A) WORKFLOW OVERRIDES (used by reusable-cla-check.yml) -# ----------------------------- - -# If true, any org member is treated as allowed (no CLA/DCO gate). -org_members: true - -# Run DCO on permissive repos? -# off -> never run assistant on permissive repos -# external_only -> run assistant (DCO mode) for non-members only -# all -> run assistant (DCO mode) for everyone -dco_on_permissive: external_only - -# GitHub logins that always skip the gate (case-insensitive) -users: - - normansden -# - bob-consultant - -# Bot accounts that always skip the gate -bots: - - dependabot[bot] - - github-actions[bot] - -# Teams whose members always skip the gate. -# Accepts "org/team" or bare "team" (interpreted as /team). -# teams: -# - myorg/maintainers -# - release-engineering - -# Optional global exemption window (everyone is allowed during this period). -# ISO 8601 timestamps; end is inclusive to the minute. -#temporary_exemptions: -# start: 2025-12-30T00:00:00Z -# end: 2025-12-31T23:59:59Z -# reason: "Partner pilot" - -# ----------------------------- -# B) LICENSE POLICY OVERRIDES (used by requires_cla.py) +# LICENSE POLICY OVERRIDES (used by requires_cla.py) # ----------------------------- # Base truth for permissive/non-permissive comes from: # data/permissive.json + data/licenses_all.json From f9c9a07be47e30db2bb6f338fa3b3cfb9c13fa69 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Fri, 18 Sep 2026 11:18:22 -0500 Subject: [PATCH 2/4] test(compliance): cover the real allowlist file, and trim its header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the Section A removal. First, a test gap this change exposed. Every existing test injects allowlist data as a dict, so none of them read the shipped cla/allowlist.yml — the suite passes whether that file is valid, emptied, or absent. The deletion in the previous commit reached a PR with 57 tests green and nothing exercising the file it changed. Since the file is fetched at `ref: main` by every gated repo, a malformed version would be live org-wide on merge. AllowlistFileTests parses the real file and asserts: it is a mapping, license_overrides is present and shaped as the live code expects, LicenseRef-Broadcom_Source_Available still resolves to require-CLA, and none of the stale workflow-override keys have been re-added. That last one guards the specific footgun here — re-adding `users:` would look like a gate bypass while doing nothing at all. Verified by mutation rather than assumed: re-adding a `users:` key fails test_no_stale_workflow_override_keys; deleting the Broadcom require_cla entry fails the override test; corrupting the YAML errors the whole class. All three caught. 61 tests, OK. Second, the header comment was 16 lines of history sitting on top of ten lines of config — the file read as mostly prose. Cut to three lines stating what it drives and where bypasses actually live. The history belongs in this commit and the PR, which have it. Co-Authored-By: Claude Opus 5 (1M context) --- cla/allowlist.yml | 18 ++--------- tests/test_policy_selector.py | 58 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/cla/allowlist.yml b/cla/allowlist.yml index 2a486cb..a638259 100644 --- a/cla/allowlist.yml +++ b/cla/allowlist.yml @@ -1,20 +1,8 @@ # .github/cla/allowlist.yml -# Purpose: license-policy overrides for requires_cla.py. -# -# This file previously also carried a "workflow overrides" section -# (org_members, dco_on_permissive, users, bots, teams, temporary_exemptions). -# Those keys were read only by reusable-cla-check.yml, which was decommissioned -# when policy_selector.py took over enforcement. No live code has read them -# since, so they were removed rather than left looking authoritative — an -# allowlist that appears to grant standing gate bypasses, but doesn't, is worse -# than no allowlist at all. -# -# Who actually skips the gate today is decided in scripts/policy_selector.py, -# not here. There are exactly two bypasses, both in process_single_pr(): -# * bots — BOT_ALLOWLIST, plus any login ending in "[bot]" -# * org members — is_org_member(), checked live against the GitHub API -# To change either, change that script. +# License-policy overrides for requires_cla.py. This file does NOT decide who +# skips the gate — that is scripts/policy_selector.py::process_single_pr(), +# which bypasses only bots and org members. # ----------------------------- # LICENSE POLICY OVERRIDES (used by requires_cla.py) diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 1e695de..ffad4e8 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -31,6 +31,7 @@ import sys import tempfile import unittest +from pathlib import Path # --- Import setup. Must happen before `import policy_selector`. --- # @@ -686,5 +687,62 @@ def test_dedup_markers_are_present(self): self.assertIn("Sign via Comment", msg) +class AllowlistFileTests(unittest.TestCase): + """Parse the REAL cla/allowlist.yml. + + Every other test in this file injects allowlist data as a dict, so none of + them read the shipped file — the suite passes whether or not it is valid, + or even present. That gap let a change to this file reach a PR with the + full suite green and nothing actually exercising it. + + The file is fetched at runtime from `ref: main` by every gated repo, so a + malformed or emptied version is live org-wide the moment it merges. These + tests are cheap insurance against that. + """ + + @classmethod + def setUpClass(cls): + import yaml + cls.path = Path(__file__).resolve().parents[1] / "cla" / "allowlist.yml" + cls.data = yaml.safe_load(cls.path.read_text()) + + def test_file_parses_to_a_mapping(self): + self.assertIsInstance(self.data, dict, "allowlist.yml must parse to a mapping") + + def test_license_overrides_present_and_shaped(self): + overrides = self.data.get("license_overrides") + self.assertIsInstance(overrides, dict, "license_overrides is the only section live code reads") + self.assertIsInstance(overrides.get("require_cla"), list) + + def test_broadcom_source_available_still_forces_cla(self): + """The one override with real teeth: a non-permissive Broadcom licence + must still be pushed to CLA rather than falling through to DCO.""" + decision = policy_selector_module_requires_cla()._override_requires_cla( + "licenseref-broadcom-source-available", self.data + ) + self.assertIs(decision, True) + + def test_no_stale_workflow_override_keys(self): + """org_members / users / bots / teams / dco_on_permissive / + temporary_exemptions were read only by the decommissioned + reusable-cla-check.yml. Re-adding one would look like a gate bypass + while doing nothing, which is how a departed employee came to appear + allowlisted long after the workflow that honoured it was retired.""" + stale = {"org_members", "dco_on_permissive", "users", "bots", "teams", + "temporary_exemptions"} + found = stale & set(self.data) + self.assertEqual( + found, set(), + f"{sorted(found)} is not read by any live code — enforcement bypasses " + "live in policy_selector.process_single_pr(), not in this file", + ) + + +def policy_selector_module_requires_cla(): + """Import requires_cla the same way policy_selector does at runtime.""" + import requires_cla + return requires_cla + + if __name__ == "__main__": unittest.main() From 55e6c24a0ee3a47fdd84b8d32825036742542498 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Mon, 21 Sep 2026 13:19:26 -0500 Subject: [PATCH 3/4] fix(compliance): correct this PR's own inaccuracies, and make its tests reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent review of the previous two commits found the change did not meet the bar it set for itself. Three problems were mine. **The new tests could never run for the change class they guard.** tests.yml filters on scripts/**, tests/** and itself; cla/** was absent. A PR editing only cla/allowlist.yml triggered no workflow at all — the exact gap the test class was written to close. This PR looked green only because it also touches tests/. Added cla/** to both paths lists; that one line is what makes the rest of the class worth having. **The header I wrote was also wrong.** It claimed the file "does NOT decide who skips the gate". In fact policy_selector.fetch_shared_config() reads license_overrides.repos and uses require_cla: false to force is_strict=False, downgrading a whole repo from CLA to DCO — vmware/.github is on DCO today because of the entry in this file. I replaced one misleading header with another. It now states both effects, names all three early-return paths in process_single_pr (existing success status, bots, org members — the first was missing), and documents the undocumented top-level `repos:`/`repositories:` keys that fetch_shared_config also honours. **The stale-key guard had the wrong keys.** It denied `temporary_exemptions` — a spelling nothing ever read — while missing `temp_exemptions`, the one the retired workflow actually indexed. Replaced the denylist with a subset assertion against the three keys live code reads, which catches any unread key including ones nobody has thought of. Also fixed, all found by the same review: - `require_cla: false # force enforcement for this repo` said the opposite of what the value does, on the one live entry, in a PR about comment accuracy. The commented twin below it was already correct. - data/permissive.json does not exist; the file is permissive_names.json. - No coverage of license_overrides.repos — the only part policy_selector reads. A null entry or a list instead of a mapping is swallowed into a debug log in production, silently reverting every DCO downgrade. Now asserted. - read_text() without encoding=, in a file this PR gave an em dash to: a non-UTF-8 locale raised in setUpClass and erased all four tests from the report. - A non-mapping file errored in three places with AttributeError/TypeError; `data='hello'` even passed the key check. One mapping() guard now fails with a message naming the file. - The Broadcom assertion hand-normalised its input, so a change to _norm_license_name could break production while the test stayed green. It now feeds the raw ID through the real normaliser. - Dropped policy_selector_module_requires_cla(); a plain import. Its docstring claimed parity with policy_selector's try/except stub fallback, which it did not have. - Reused requires_cla._ALLOWLIST_PATH instead of a second copy of the path. - Suppressed the three ::warning:: lines _override_requires_cla prints, so a green run stops painting annotations on the repo's only gate. - Renamed AllowlistFileTests -> TestAllowlistFile; pytest's default python_classes would have collected zero of them. Mutation-tested rather than assumed. Six cases now fail that should, four of which the previous version missed entirely: temp_exemptions, a null repos entry, repos as a list, a stale users: key, a deleted Broadcom override, and corrupt YAML. 62 tests, OK. Deliberately NOT in scope, both pre-existing and both wanting their own change: the LicenseRef-Broadcom* wildcard is inert (requires_cla does exact membership, so LicenseRef-Broadcom-Proprietary resolves to DCO today), and fetch_shared_config assigns yaml.safe_load's result before the .get() that can raise, so a malformed allowlist leaves allowlist_data=None and silently drops every override org-wide. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/tests.yml | 2 + cla/allowlist.yml | 34 ++++++++-- tests/test_policy_selector.py | 116 ++++++++++++++++++++++------------ 3 files changed, 105 insertions(+), 47 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 750306c..49d0dc6 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -14,12 +14,14 @@ on: paths: - 'scripts/**' - 'tests/**' + - 'cla/**' - '.github/workflows/tests.yml' push: branches: [main] paths: - 'scripts/**' - 'tests/**' + - 'cla/**' - '.github/workflows/tests.yml' permissions: diff --git a/cla/allowlist.yml b/cla/allowlist.yml index a638259..41018e6 100644 --- a/cla/allowlist.yml +++ b/cla/allowlist.yml @@ -1,14 +1,36 @@ # .github/cla/allowlist.yml -# License-policy overrides for requires_cla.py. This file does NOT decide who -# skips the gate — that is scripts/policy_selector.py::process_single_pr(), -# which bypasses only bots and org members. +# +# This file does NOT contain a per-user or per-team bypass list. It has two +# effects, both on WHICH DOCUMENT a contributor must sign, never on whether +# someone is checked at all: +# * license_overrides.require_cla / allow_dco — read by requires_cla.py, +# override a licence's permissive/non-permissive classification. +# * license_overrides.repos..require_cla — read by +# policy_selector.fetch_shared_config(), and `false` downgrades that whole +# repo from CLA to DCO. This is live: vmware/.github is on DCO because of +# the entry below. +# +# Two further top-level keys are honoured but undocumented elsewhere, so they +# are recorded here rather than left to be rediscovered: a top-level `repos:` +# block (used only when license_overrides.repos is absent or empty), and a +# top-level `repositories:` list whose entries are appended straight to the +# DCO-only set. Prefer license_overrides.repos; the other two exist for +# backwards compatibility. +# +# Who skips the gate entirely is decided in policy_selector.process_single_pr(), +# not here. Three paths return early, in order: an existing successful +# "Check CLA/DCO" status on the head SHA, a bot (BOT_ALLOWLIST or any login +# ending "[bot]"), and an org member (live is_org_member() API call). +# +# Both production workflows fetch this file from the default branch with no +# pinned ref, so any edit is live across every gated repo on the next sweep. # ----------------------------- -# LICENSE POLICY OVERRIDES (used by requires_cla.py) +# LICENSE POLICY OVERRIDES # ----------------------------- # Base truth for permissive/non-permissive comes from: -# data/permissive.json + data/licenses_all.json +# data/permissive_names.json + data/licenses_all.json # The keys below *override* that base decision. license_overrides: @@ -28,7 +50,7 @@ license_overrides: # Useful when the detector result needs correction or you want to force policy. repos: vmware/.github: - require_cla: false # force enforcement for this repo + require_cla: false # false = downgrade this repo to DCO-only # force_spdx: "MIT" # pretend this is the detected license (rarely needed) # docs-site: # require_cla: false # force permissive for this repo diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index ffad4e8..5077014 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -29,9 +29,10 @@ import json import os import sys +import contextlib +import io import tempfile import unittest -from pathlib import Path # --- Import setup. Must happen before `import policy_selector`. --- # @@ -55,6 +56,7 @@ sys.path.insert(0, os.path.join(_REPO_ROOT, "scripts")) import policy_selector # noqa: E402 +import requires_cla # noqa: E402 SIGNED_SUFFIX = "for this and all future contributions" @@ -687,61 +689,93 @@ def test_dedup_markers_are_present(self): self.assertIn("Sign via Comment", msg) -class AllowlistFileTests(unittest.TestCase): +class TestAllowlistFile(unittest.TestCase): """Parse the REAL cla/allowlist.yml. - Every other test in this file injects allowlist data as a dict, so none of - them read the shipped file — the suite passes whether or not it is valid, - or even present. That gap let a change to this file reach a PR with the - full suite green and nothing actually exercising it. + Every other test in this file monkeypatches `requires_cla.requires_CLA` + away in setUp, so none of them exercise the shipped file — the suite + passes whether it is valid, emptied, or absent. That gap let a change to + this file reach a PR with the full suite green and nothing reading it. - The file is fetched at runtime from `ref: main` by every gated repo, so a - malformed or emptied version is live org-wide the moment it merges. These - tests are cheap insurance against that. + (Note the seam is the monkeypatch, not `allowlist_data={}`: `_load_allowlist` + gates on a truthiness check, so an empty dict falls through to the disk + read and would pick up the real file anyway.) + + Both production workflows fetch this file from `ref: main` with no pinned + ref, so a malformed version is live org-wide the moment it merges. """ + # The only top-level keys any live code reads: `license_overrides` via + # requires_cla, and `repos`/`repositories` via + # policy_selector.fetch_shared_config. Asserting a subset rather than + # denying a list of known-dead names catches keys nobody has thought of — + # including `temp_exemptions`, the spelling the retired workflow actually + # read, which an earlier denylist here missed while blocking the inert + # `temporary_exemptions`. + READ_BY_LIVE_CODE = {"license_overrides", "repos", "repositories"} + @classmethod def setUpClass(cls): import yaml - cls.path = Path(__file__).resolve().parents[1] / "cla" / "allowlist.yml" - cls.data = yaml.safe_load(cls.path.read_text()) + # Reuse production's own path constant so moving the file fails loudly + # here instead of leaving the test reading a stale location. + cls.path = requires_cla._ALLOWLIST_PATH + # encoding= matters: _load_allowlist opens utf-8 explicitly, and this + # file contains non-ASCII. Without it a non-UTF-8 locale raises in + # setUpClass and silently erases all of these tests from the report. + cls.data = yaml.safe_load(cls.path.read_text(encoding="utf-8")) + + def mapping(self): + """Fail fast with a message naming the file, rather than letting a + non-mapping surface as AttributeError/TypeError in four places.""" + self.assertIsInstance( + self.data, dict, + f"{self.path} must parse to a mapping; got {type(self.data).__name__}", + ) + return self.data def test_file_parses_to_a_mapping(self): - self.assertIsInstance(self.data, dict, "allowlist.yml must parse to a mapping") + self.mapping() - def test_license_overrides_present_and_shaped(self): - overrides = self.data.get("license_overrides") - self.assertIsInstance(overrides, dict, "license_overrides is the only section live code reads") - self.assertIsInstance(overrides.get("require_cla"), list) - - def test_broadcom_source_available_still_forces_cla(self): - """The one override with real teeth: a non-permissive Broadcom licence - must still be pushed to CLA rather than falling through to DCO.""" - decision = policy_selector_module_requires_cla()._override_requires_cla( - "licenseref-broadcom-source-available", self.data - ) - self.assertIs(decision, True) - - def test_no_stale_workflow_override_keys(self): - """org_members / users / bots / teams / dco_on_permissive / - temporary_exemptions were read only by the decommissioned - reusable-cla-check.yml. Re-adding one would look like a gate bypass - while doing nothing, which is how a departed employee came to appear - allowlisted long after the workflow that honoured it was retired.""" - stale = {"org_members", "dco_on_permissive", "users", "bots", "teams", - "temporary_exemptions"} - found = stale & set(self.data) + def test_only_keys_live_code_reads(self): + extra = set(self.mapping()) - self.READ_BY_LIVE_CODE self.assertEqual( - found, set(), - f"{sorted(found)} is not read by any live code — enforcement bypasses " - "live in policy_selector.process_single_pr(), not in this file", + extra, set(), + f"{sorted(extra)} is read by no live code. Enforcement bypasses live in " + "policy_selector.process_single_pr(); a key here that looks like one " + "but does nothing is how a departed employee stayed apparently " + "allowlisted after the workflow honouring it was retired.", ) + def test_license_overrides_shaped_as_requires_cla_expects(self): + overrides = self.mapping().get("license_overrides") + self.assertIsInstance(overrides, dict) + self.assertIsInstance(overrides.get("require_cla"), list) + + def test_repo_overrides_shaped_as_policy_selector_expects(self): + """`license_overrides.repos` is the part policy_selector reads, and it + decides CLA-vs-DCO for a whole repo. fetch_shared_config swallows a + malformed shape into a debug log, so every downgrade would silently + revert with no failing check — this is the assertion that catches it.""" + repos = (self.mapping().get("license_overrides") or {}).get("repos") + if repos is None: + return # optional section + self.assertIsInstance(repos, dict, "repos must be a mapping, not a list") + for name, cfg in repos.items(): + self.assertIsInstance(cfg, dict, f"repos[{name}] must be a mapping, not null") + self.assertIsInstance(cfg.get("require_cla"), bool, + f"repos[{name}].require_cla must be a bool") -def policy_selector_module_requires_cla(): - """Import requires_cla the same way policy_selector does at runtime.""" - import requires_cla - return requires_cla + def test_broadcom_source_available_still_forces_cla(self): + """Broadcom Source Available is listed as permissive in the base + tables, so without this override it would fall through to DCO. Feed + the raw ID through production's own normaliser rather than a + hand-normalised literal, so a change to that normaliser fails here.""" + norm = requires_cla._norm_license_name("LicenseRef-Broadcom_Source_Available") + buf = io.StringIO() + with contextlib.redirect_stdout(buf): # keeps ::warning:: out of CI annotations + decision = requires_cla._override_requires_cla(norm, self.mapping()) + self.assertIs(decision, True) if __name__ == "__main__": From 604499da6ef790a7eb8ade8f2fb2fdb17505fd14 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Mon, 21 Sep 2026 13:28:30 -0500 Subject: [PATCH 4/4] fix(compliance): make the repos assertion non-vacuous, and stop the suite collapsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A gap sweep over the previous commit found that its headline new test could not fail for any of the scenarios its own docstring named. test_repo_overrides_shaped_as_policy_selector_expects claimed to be "the assertion that catches" a silently reverted DCO downgrade. Replayed against the shipped file, it passed when the repos block was deleted, when it was emptied to {}, when the vmware/.github entry was removed, and when require_cla was flipped to true — every realistic regression. It fired only on a type error inside an entry that still existed, which is the one case that would not silently revert. Having just been corrected for overclaiming in a docstring, I did it again. Split into two: the shape assertion now requires repos to be a mapping outright (no `if repos is None: return` escape) and requires every key to contain "/", because process_single_pr compares against owner/repo so a bare key is collected and never matches. And test_dotgithub_stays_on_dco pins the policy itself — vmware/.github being on DCO is a deliberate decision, so changing it should fail here and be re-affirmed in the same commit rather than drifting silently. Verified by mutation: all four cases that previously passed now fail, and the three that already failed still do. Also from the sweep: - The module-level `import requires_cla` I added turned a graceful degradation into a total collapse. policy_selector wraps that import in try/except and substitutes a stub; with aiohttp absent, origin/main runs 57 tests OK while this branch produced one collection error and zero tests. requires_cla pulls aiohttp, and license_detector pulls rapidfuzz, which is not in scripts/requirements.txt. Now guarded, with the class skipped rather than erroring. - I corrected the inverted `# force enforcement` comment on the live entry but left its commented twin three lines below still reading "force permissive", so the file again explained the same value two ways. Fixed, and the example key changed to vmware/docs-site since the bare form it used cannot work. - "Keys can be / or just " was false, and the example demonstrated the dead form. - The commented `permissive:` stub named a key nothing reads; the real one is allow_dco, which the new header already cites. Renamed, with a note saying why. Still out of scope, unchanged: the LicenseRef-Broadcom* wildcard, the fetch_shared_config fail-open, spdx_aliases/force_spdx, and the unchecked extend() on a top-level `repositories:` scalar. 63 tests, OK. Co-Authored-By: Claude Opus 5 (1M context) --- cla/allowlist.yml | 16 ++++++++------ tests/test_policy_selector.py | 40 ++++++++++++++++++++++++++++------- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/cla/allowlist.yml b/cla/allowlist.yml index 41018e6..11a1b0e 100644 --- a/cla/allowlist.yml +++ b/cla/allowlist.yml @@ -42,18 +42,20 @@ license_overrides: - LicenseRef-Broadcom* # (add more as needed) - # (Optional) Treat specific IDs as permissive (force allow), even if base says otherwise. - # Only use this if you intend to *downgrade* certain licenses to permissive. -# permissive: + # (Optional) The inverse of require_cla: force these IDs to DCO even if the + # base tables call them non-permissive. The key is allow_dco — an earlier + # version of this file documented it as "permissive", which nothing reads. +# allow_dco: - # (Optional) Per-repo overrides. Keys can be "/" or just "". - # Useful when the detector result needs correction or you want to force policy. + # (Optional) Per-repo overrides. Keys MUST be "/" — process_single_pr + # compares against the full name, so a bare "" key is collected but never + # matches anything. repos: vmware/.github: require_cla: false # false = downgrade this repo to DCO-only # force_spdx: "MIT" # pretend this is the detected license (rarely needed) -# docs-site: -# require_cla: false # force permissive for this repo +# vmware/docs-site: +# require_cla: false # false = downgrade that repo to DCO-only # (Optional) Normalize odd/legacy license IDs to canonical SPDX before evaluation. # The left side is what the detector finds; the right side is the canonical ID. diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 5077014..48ef37c 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -56,7 +56,15 @@ sys.path.insert(0, os.path.join(_REPO_ROOT, "scripts")) import policy_selector # noqa: E402 -import requires_cla # noqa: E402 + +# policy_selector wraps this import in try/except and substitutes a stub, so a +# missing optional dep (aiohttp, rapidfuzz) degrades rather than breaking. Match +# that: an unguarded import here turns one absent dep into a collection error +# that erases the whole suite instead of skipping the tests that need it. +try: + import requires_cla # noqa: E402 +except Exception: # pragma: no cover - exercised only on a degraded runner + requires_cla = None SIGNED_SUFFIX = "for this and all future contributions" @@ -689,6 +697,7 @@ def test_dedup_markers_are_present(self): self.assertIn("Sign via Comment", msg) +@unittest.skipIf(requires_cla is None, "requires_cla unavailable (optional dep missing)") class TestAllowlistFile(unittest.TestCase): """Parse the REAL cla/allowlist.yml. @@ -753,18 +762,33 @@ def test_license_overrides_shaped_as_requires_cla_expects(self): self.assertIsInstance(overrides.get("require_cla"), list) def test_repo_overrides_shaped_as_policy_selector_expects(self): - """`license_overrides.repos` is the part policy_selector reads, and it - decides CLA-vs-DCO for a whole repo. fetch_shared_config swallows a - malformed shape into a debug log, so every downgrade would silently - revert with no failing check — this is the assertion that catches it.""" + """`license_overrides.repos` decides CLA-vs-DCO for a whole repo, and + fetch_shared_config swallows a malformed shape into a debug log.""" repos = (self.mapping().get("license_overrides") or {}).get("repos") - if repos is None: - return # optional section - self.assertIsInstance(repos, dict, "repos must be a mapping, not a list") + self.assertIsInstance(repos, dict, "repos must be a mapping, not a list or null") for name, cfg in repos.items(): self.assertIsInstance(cfg, dict, f"repos[{name}] must be a mapping, not null") self.assertIsInstance(cfg.get("require_cla"), bool, f"repos[{name}].require_cla must be a bool") + self.assertIn("/", name, + f"repos[{name}] must be '/' — process_single_pr " + "compares against the full name, so a bare repo never matches") + + def test_dotgithub_stays_on_dco(self): + """Pins a deliberate policy decision rather than the file's shape. + + Deleting the repos block, emptying it, dropping this entry, or flipping + it to true all leave the previous shape-only assertions green while + silently moving this repo from DCO back to CLA. Changing that is a + legitimate decision — it just has to be a deliberate one, so it fails + here first. + """ + repos = (self.mapping().get("license_overrides") or {}).get("repos") or {} + self.assertIs( + repos.get("vmware/.github", {}).get("require_cla"), False, + "vmware/.github is intentionally on DCO; if that changed on purpose, " + "update this test in the same commit", + ) def test_broadcom_source_available_still_forces_cla(self): """Broadcom Source Available is listed as permissive in the base