From f45fa0ef0405b11263209f0f101722592a374547 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 22 Sep 2026 11:46:37 -0500 Subject: [PATCH 1/4] fix(compliance): make the licence wildcard work, and fail closed on an unreadable allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found reviewing #74 and deliberately left out of it, because both change enforcement rather than documentation. ## 1. The documented '*' wildcard never worked cla/allowlist.yml has always said "Supports exact IDs and simple '*' wildcards (handled by requires_cla.py)" and ships `LicenseRef-Broadcom*` on that basis. But _override_requires_cla built a set and tested `norm_license in req` — plain equality. The entry could only ever match a licence literally named `LicenseRef-Broadcom*`. That was not merely inert. A licence falling past the override goes to the base tables, where is_permissive_with_reason strips the `LicenseRef-` prefix before matching, so `LicenseRef-Broadcom-Proprietary` finds `Broadcom_Proprietary` in permissive_names.json and comes back permissive -> DCO. A proprietary Broadcom licence was gated on the weaker document. _matches_any now globs any pattern containing * ? or [, and compares exactly otherwise. fnmatchcase rather than fnmatch, since both sides are already lowercased by _norm_license_name and fnmatch's case folding is platform-dependent. Blast radius measured before writing it: of 145 repos in the last licence report, 48 use Broadcom_Source_Available — already CLA via the exact entry, unchanged — and none use Broadcom_Proprietary. So this changes no repo's current document and closes the gap prospectively. ## 2. An unreadable allowlist read as a permissive one fetch_shared_config assigned `allowlist_data = yaml.safe_load(...)` before the `.get()` that can raise. An emptied or comments-only file yields None, so the AttributeError fired *after* the safe `{}` default had already been overwritten, was swallowed into a debug_log, and every licence override silently vanished org-wide. The direction matters: Broadcom Source Available is listed as permissive in the base tables and is held at CLA *only* by the override. Losing it downgrades all 48 of those repos to DCO. Fail-open, in a compliance gate, and inconsistent with the function twenty lines below that already defaults to STRICT when the licence logic errors. fetch_shared_config now reports `allowlist_ok`, and process_single_pr forces is_strict and clears allowlist_repos when it is False — "we could not read the policy" is not "the policy permits this". The parse failure is now an ::error:: rather than a debug_log, and names the type it got. The scalar `repositories:` case is rejected instead of being iterated character by character. `config.get("allowlist_ok", True)` defaults true so a hand-built shared_config — the tests, and any caller supplying its own data — is treated as deliberate rather than as a failure. cla_sweeper passes the real dict through unchanged; license_report.py calls get_license_decision directly and is unaffected by the new key. ## Tests 12 new, 75 total, OK. Mutation-tested: - wildcard -> plain set membership: 3 failures - drop the fail-closed guard: 1 failure Reverting the isinstance guard or the except-reset individually does NOT fail, and that is correct rather than a gap: `allowlist_ok` is the single point of truth, so both are overlapping safety nets that converge on the same outcome. The guard's real contribution is the message — "allowlist must be a mapping, got NoneType" instead of "'NoneType' object has no attribute 'get'". Also extended test_missing_config_degrades_to_empty_rather_than_raising, which asserted three of the four keys and omitted allowlist_data — the only one that could come back poisoned. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/policy_selector.py | 45 ++++++++++- scripts/requires_cla.py | 33 +++++++- tests/test_policy_selector.py | 137 +++++++++++++++++++++++++++++++++- 3 files changed, 206 insertions(+), 9 deletions(-) diff --git a/scripts/policy_selector.py b/scripts/policy_selector.py index 607b514..c02b967 100644 --- a/scripts/policy_selector.py +++ b/scripts/policy_selector.py @@ -469,10 +469,23 @@ def fetch_shared_config(api_root, gh_token): allowlist_repos = [] allowlist_data = {} + # False means "we could not read the policy", which is NOT the same as + # "the policy is empty". Callers use it to fail closed rather than + # silently enforcing a weaker document than the real allowlist requires. + allowlist_ok = False if raw_allowlist: try: - allowlist_data = yaml.safe_load(raw_allowlist) + parsed = yaml.safe_load(raw_allowlist) + if not isinstance(parsed, dict): + # safe_load returns None for an empty/comments-only file and a + # list for a top-level sequence. Assigning either to + # allowlist_data before the .get() below would destroy the {} + # default and poison every downstream consumer. + raise ValueError( + f"allowlist must be a mapping, got {type(parsed).__name__}" + ) + allowlist_data = parsed # Handle nesting under 'license_overrides' -> 'repos' repos_config = allowlist_data.get("license_overrides", {}).get("repos", {}) if not repos_config: @@ -483,10 +496,23 @@ def fetch_shared_config(api_root, gh_token): if r_config.get("require_cla") is False: allowlist_repos.append(r_name) - allowlist_repos.extend(allowlist_data.get("repositories", [])) + extra = allowlist_data.get("repositories") or [] + if isinstance(extra, list): + allowlist_repos.extend(extra) + else: + # A bare string here would be iterated character by character. + debug_log(f"⚠️ 'repositories' must be a list, got {type(extra).__name__}; ignoring.") + allowlist_ok = True debug_log(f"✅ Allowlist loaded via API. Found {len(allowlist_repos)} DCO-only repos.") except Exception as e: - debug_log(f"⚠️ Failed to parse allowlist YAML: {e}") + # Reset rather than leave a half-built or poisoned value behind. + allowlist_data = {} + allowlist_repos = [] + print(f"::error::Failed to parse cla/allowlist.yml: {e}. " + "Enforcing CLA for every repo until this is fixed.") + else: + print("::error::Could not fetch cla/allowlist.yml. " + "Enforcing CLA for every repo until this is fixed.") # B. Licenses licenses_data = fetch_json_with_fallback(api_root, "data/licenses_all.json", "cla/licenses_all.json", gh_token) or [] @@ -497,6 +523,7 @@ def fetch_shared_config(api_root, gh_token): return { "allowlist_data": allowlist_data, "allowlist_repos": allowlist_repos, + "allowlist_ok": allowlist_ok, "licenses_data": licenses_data, "permissive_data": permissive_data, } @@ -553,6 +580,18 @@ def process_single_pr(pr_number, pr_head_sha, pr_user, repo_full_name, gh_token, debug_log(f"⚠️ Logic Module Error: {e}. Defaulting to STRICT mode.") is_strict = True + # An unreadable allowlist is not an empty one. Without its overrides a + # non-permissive licence can look permissive — Broadcom Source Available + # is listed as permissive in the base tables and is held at CLA only by + # the override — so continuing would silently downgrade repos to DCO. + # Default to strict instead, matching the Logic Module Error path above. + # .get() default is True so a hand-built shared_config (tests, callers + # that supply their own data) is treated as deliberate, not as a failure. + if not config.get("allowlist_ok", True): + debug_log("⚠️ Allowlist unavailable. Defaulting to STRICT mode.") + is_strict = True + allowlist_repos = [] + # Allowlist Override if repo_full_name in allowlist_repos: debug_log(f"ℹ️ Repo {repo_full_name} is in Allowlist. Enforcing DCO only.") diff --git a/scripts/requires_cla.py b/scripts/requires_cla.py index 3211e58..a2b8830 100644 --- a/scripts/requires_cla.py +++ b/scripts/requires_cla.py @@ -26,6 +26,7 @@ # --- Overrides Helper --- from pathlib import Path +import fnmatch import re try: import yaml @@ -61,21 +62,45 @@ def _load_allowlist(in_memory_data: Optional[Dict] = None) -> dict: # Silent fail on disk read (expected in new architecture) return {} +def _matches_any(norm_license: str, patterns) -> bool: + """Exact match, or a glob when the pattern contains '*'. + + cla/allowlist.yml has documented "simple '*' wildcards" since it was + written, and ships `LicenseRef-Broadcom*` on that basis — but the match + was plain set membership, so that entry could only ever match a licence + literally named `LicenseRef-Broadcom*`. Any new Broadcom LicenseRef fell + through to the base tables, where the canonical-name matcher strips the + `LicenseRef-` prefix and finds `Broadcom_Proprietary` listed as + permissive — so a proprietary licence resolved to DCO. + + Patterns are matched case-sensitively against the already-normalised + name (both sides are lowercased by _norm_license_name first), so + fnmatchcase avoids fnmatch's platform-dependent case folding. + """ + for pattern in patterns: + if "*" in pattern or "?" in pattern or "[" in pattern: + if fnmatch.fnmatchcase(norm_license, pattern): + return True + elif norm_license == pattern: + return True + return False + + def _override_requires_cla(norm_license: str, allowlist: dict) -> None | bool: """ Return True (force CLA), False (force DCO), or None (no override). """ section = allowlist.get("license_overrides") or {} - req = {_norm_license_name(x) for x in (section.get("require_cla") or [])} - dco = {_norm_license_name(x) for x in (section.get("allow_dco") or [])} + req = [_norm_license_name(x) for x in (section.get("require_cla") or [])] + dco = [_norm_license_name(x) for x in (section.get("allow_dco") or [])] print(f"::warning::[DEBUG OVERRIDE] Checking Normalized License: '{norm_license}'") print(f"::warning:: -> 'require_cla' list contains: {sorted(list(req))}") - if norm_license in req: + if _matches_any(norm_license, req): print(f"::warning:: -> ✅ MATCH FOUND in require_cla! Forcing True.") return True - if norm_license in dco: + if _matches_any(norm_license, dco): print(f"::warning:: -> MATCH FOUND in allow_dco! Forcing False.") return False diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 48ef37c..70a65f9 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -505,6 +505,28 @@ def test_dco_policy_accepts_signed_off_commits(self): self.assertEqual([s["state"] for s in fake.statuses()], ["success"]) self.assertEqual(fake.statuses()[0]["description"], "DCO Signed") + def test_unreadable_allowlist_forces_cla_not_dco(self): + """The behaviour that matters. requires_CLA says permissive (DCO) and + the repo is in allowlist_repos (DCO), but the allowlist could not be + read — so neither signal is trustworthy and the gate must ask for the + stronger document rather than the weaker one.""" + policy_selector.requires_cla.requires_CLA = lambda *a, **k: False + fake = self.run_pr( + paginated_routes={"/issues/5/comments": [], "/pulls/5/commits": []}, + config=dict(self.SHARED_CONFIG, allowlist_ok=False, + allowlist_repos=["vmware/repo"])) + self.assertEqual(fake.statuses()[0]["description"], "CLA Missing") + + def test_hand_built_config_without_the_flag_is_treated_as_readable(self): + """Back-compat: a caller supplying its own data deliberately (tests, + license_report.py) has no allowlist_ok key and must not be forced + strict by its absence.""" + policy_selector.requires_cla.requires_CLA = lambda *a, **k: False + fake = self.run_pr( + paginated_routes={"/issues/5/comments": [], "/pulls/5/commits": []}, + config=dict(self.SHARED_CONFIG)) # no allowlist_ok key at all + self.assertEqual(fake.statuses()[0]["description"], "DCO Missing") + def test_allowlisted_repo_is_downgraded_to_dco(self): config = dict(self.SHARED_CONFIG, allowlist_repos=["vmware/repo"]) fake = self.run_pr(paginated_routes={"/issues/5/comments": [], "/pulls/5/commits": []}, config=config) @@ -653,14 +675,15 @@ def test_write_failure_is_reported_to_the_caller(self): # fetch_shared_config (protects the PR #66 per-sweep caching contract) # --------------------------------------------------------------------------- class TestFetchSharedConfig(PolicySelectorTestCase): - def test_returns_all_four_keys_process_single_pr_indexes(self): + def test_returns_all_keys_process_single_pr_indexes(self): # process_single_pr indexes these directly, so a missing key is a # KeyError mid-sweep rather than a soft failure. self.install() config = policy_selector.fetch_shared_config("https://api.invalid", "tok") self.assertEqual( sorted(config.keys()), - ["allowlist_data", "allowlist_repos", "licenses_data", "permissive_data"], + ["allowlist_data", "allowlist_ok", "allowlist_repos", + "licenses_data", "permissive_data"], ) def test_missing_config_degrades_to_empty_rather_than_raising(self): @@ -668,6 +691,11 @@ def test_missing_config_degrades_to_empty_rather_than_raising(self): config = policy_selector.fetch_shared_config("https://api.invalid", "tok") self.assertEqual(config["licenses_data"], []) self.assertEqual(config["permissive_data"], []) + # allowlist_data was previously omitted here — it is the one key that + # could come back poisoned (None, or a list) rather than empty. + self.assertEqual(config["allowlist_data"], {}) + self.assertIs(config["allowlist_ok"], False, + "an unfetchable allowlist must be reported as not-ok, not as empty") self.assertEqual(config["allowlist_repos"], []) @@ -802,5 +830,110 @@ def test_broadcom_source_available_still_forces_cla(self): self.assertIs(decision, True) + +# --------------------------------------------------------------------------- +# Allowlist robustness: a policy we cannot read must not read as a policy +# that permits everything. +# --------------------------------------------------------------------------- +class TestAllowlistFailClosed(PolicySelectorTestCase): + """cla/allowlist.yml is fetched from `ref: main` on every run. + + Before this, `allowlist_data = yaml.safe_load(...)` was assigned before + the `.get()` that can raise, so a comments-only file left allowlist_data + as None, the AttributeError was swallowed into a debug_log, and every + licence override silently disappeared org-wide. That direction is + dangerous: Broadcom Source Available is listed as permissive in the base + tables and is held at CLA *only* by the override, so losing it downgrades + those repos to DCO. + """ + + def _config(self, body): + self.install(routes={"/contents/cla/allowlist.yml": self._file(body)}) + return policy_selector.fetch_shared_config("https://api.invalid", "tok") + + @staticmethod + def _file(text): + import base64 + return {"content": base64.b64encode(text.encode()).decode()} + + def test_valid_allowlist_is_ok_and_parsed(self): + cfg = self._config( + "license_overrides:\n" + " require_cla: [LicenseRef-Broadcom_Source_Available]\n" + " repos:\n" + " vmware/x:\n" + " require_cla: false\n" + ) + self.assertIs(cfg["allowlist_ok"], True) + self.assertEqual(cfg["allowlist_repos"], ["vmware/x"]) + + def test_comments_only_file_is_not_ok_and_data_stays_a_dict(self): + cfg = self._config("# nothing but a comment\n") + self.assertIs(cfg["allowlist_ok"], False) + self.assertEqual(cfg["allowlist_data"], {}, + "must not leak yaml.safe_load's None into callers") + + def test_top_level_list_is_not_ok(self): + cfg = self._config("- one\n- two\n") + self.assertIs(cfg["allowlist_ok"], False) + self.assertEqual(cfg["allowlist_data"], {}) + + def test_malformed_yaml_is_not_ok(self): + cfg = self._config("license_overrides: [unclosed\n") + self.assertIs(cfg["allowlist_ok"], False) + self.assertEqual(cfg["allowlist_data"], {}) + self.assertEqual(cfg["allowlist_repos"], []) + + def test_repositories_scalar_does_not_explode_into_characters(self): + cfg = self._config("repositories: vmware/foo\n") + self.assertEqual(cfg["allowlist_repos"], [], + "a bare string must be rejected, not iterated per character") + + +class TestOverrideWildcards(unittest.TestCase): + """`cla/allowlist.yml` has always documented "simple '*' wildcards", and + ships `LicenseRef-Broadcom*` on that basis, but matching was plain set + membership so that entry matched nothing.""" + + ALLOWLIST = {"license_overrides": { + "require_cla": ["LicenseRef-Broadcom_Source_Available", "LicenseRef-Broadcom*"], + "allow_dco": ["LicenseRef-Sample*"], + }} + + def check(self, license_id): + import contextlib, io + with contextlib.redirect_stdout(io.StringIO()): + return requires_cla._override_requires_cla( + requires_cla._norm_license_name(license_id), self.ALLOWLIST) + + def test_exact_entry_still_matches(self): + self.assertIs(self.check("LicenseRef-Broadcom_Source_Available"), True) + + def test_wildcard_catches_other_broadcom_licences(self): + """The gap this closes: without it, LicenseRef-Broadcom-Proprietary + falls through to the base tables, where the canonical matcher strips + the LicenseRef- prefix and finds Broadcom_Proprietary listed as + permissive — so a proprietary licence resolved to DCO.""" + for lic in ("LicenseRef-Broadcom-Proprietary", + "LicenseRef-Broadcom_Enterprise", + "LicenseRef-BroadcomAnything"): + self.assertIs(self.check(lic), True, lic) + + def test_wildcard_does_not_over_match(self): + for lic in ("MIT", "Apache-2.0", "LicenseRef-Other", "Broadcom-Without-Prefix"): + self.assertIsNone(self.check(lic), lic) + + def test_allow_dco_wildcards_work_too(self): + self.assertIs(self.check("LicenseRef-Sample-Thing"), False) + + def test_require_cla_wins_over_allow_dco(self): + both = {"license_overrides": {"require_cla": ["LicenseRef-X*"], + "allow_dco": ["LicenseRef-X*"]}} + import contextlib, io + with contextlib.redirect_stdout(io.StringIO()): + d = requires_cla._override_requires_cla("licenseref-xyz", both) + self.assertIs(d, True, "require_cla must take precedence") + + if __name__ == "__main__": unittest.main() From fa5e222e0b1e766b3f748de916f5e00a82029d7b Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 22 Sep 2026 11:58:49 -0500 Subject: [PATCH 2/4] test(compliance): close three coverage gaps in the previous commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Probed the new tests by mutating things they ought to catch, rather than assuming 12 tests meant 12 tests' worth of coverage. Three slipped through. **The shipped wildcard entry was not pinned.** TestOverrideWildcards builds its own allowlist dict, so deleting `LicenseRef-Broadcom*` from the real cla/allowlist.yml left the suite green — while every Broadcom licence except the one spelled out silently returned to the base tables, which list them as permissive, i.e. back to DCO. Now asserted against the shipped file. **A broken normaliser passed unnoticed** — the same defect I was pulled up on in #74 and reproduced here. The allowlist spells it `LicenseRef-Broadcom_Source_Available` (underscores); licenses_all.json's canonical spdx_id is `LicenseRef-Broadcom-Source-Available` (hyphens). They meet only because _norm_license_name collapses [\s_]+ to '-'. Removing that rule breaks the production lookup for 48 repos. My first attempt at a guard drove the lookup from the catalogue's own value, which was the right instinct and still insufficient: the `licenseref-broadcom*` wildcard matches the catalogue spelling either way, so it masks the breakage. Verified, then asserted the convergence property directly instead — the two spellings must normalise to the same string, independent of any matcher. **The case-sensitivity test cannot fail on this platform.** fnmatch defers to os.path.normcase, which is identity on POSIX, so fnmatch and fnmatchcase are indistinguishable here and differ only on Windows. The test is kept because it documents why fnmatchcase is the correct choice, but it is recorded as unable to detect a regression on a Linux runner rather than presented as coverage it does not provide. 80 tests, OK. Re-ran the full mutation matrix afterwards: wildcard entry deleted -> 2 failures, normaliser broken -> 1, globbing disabled -> 5, fail-closed guard removed -> 1. One mutation deliberately left uncaught: deleting the exact `LicenseRef-Broadcom_Source_Available` entry still passes, because the wildcard now covers it. That is the fix working, not a gap. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_policy_selector.py | 83 +++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 70a65f9..7a7d2ba 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -33,6 +33,7 @@ import io import tempfile import unittest +from pathlib import Path # --- Import setup. Must happen before `import policy_selector`. --- # @@ -818,6 +819,17 @@ def test_dotgithub_stays_on_dco(self): "update this test in the same commit", ) + def test_broadcom_wildcard_entry_is_present(self): + """Pins the entry itself, not just the matcher. Deleting + `LicenseRef-Broadcom*` from this file silently returns every Broadcom + licence except the one spelled out to the base tables, where they are + listed as permissive — i.e. back to DCO.""" + req = (self.mapping().get("license_overrides") or {}).get("require_cla") or [] + self.assertTrue( + any("*" in str(x) and "roadcom" in str(x) for x in req), + f"expected a LicenseRef-Broadcom* wildcard in require_cla, got {req}", + ) + 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 @@ -890,6 +902,65 @@ def test_repositories_scalar_does_not_explode_into_characters(self): "a bare string must be rejected, not iterated per character") +class TestOverrideMatchesTheRealCatalogue(unittest.TestCase): + """Guard the seam between two files that must agree but are spelled + differently. + + cla/allowlist.yml says `LicenseRef-Broadcom_Source_Available` + (underscores); data/licenses_all.json's canonical spdx_id is + `LicenseRef-Broadcom-Source-Available` (hyphens). They match ONLY because + _norm_license_name collapses [\\s_]+ to '-'. Change that normaliser, or + regenerate the catalogue with a different spelling, and production stops + forcing CLA on 48 repos while a test that normalises both sides itself + would stay green. So this drives the lookup from the catalogue's own + value, not from a literal. + """ + + @classmethod + def setUpClass(cls): + import yaml + cls.allowlist = yaml.safe_load( + requires_cla._ALLOWLIST_PATH.read_text(encoding="utf-8")) + cls.catalogue = json.loads( + (Path(__file__).resolve().parents[1] / "data" / "licenses_all.json").read_text()) + + def override_for(self, catalogue_key): + entry = self.catalogue[catalogue_key] + spdx = entry.get("spdx_id") or catalogue_key + import contextlib, io + with contextlib.redirect_stdout(io.StringIO()): + return requires_cla._override_requires_cla( + requires_cla._norm_license_name(spdx), self.allowlist) + + def test_allowlist_and_catalogue_spellings_converge(self): + """Assert the convergence directly, not through the matcher. + + Going via _override_requires_cla cannot detect a broken normaliser, + because the LicenseRef-Broadcom* wildcard matches the catalogue form + either way and masks it. The two files genuinely disagree on spelling + — underscores here, hyphens there — and only _norm_license_name makes + them meet. That property is what has to hold. + """ + entry = self.catalogue["Broadcom_Source_Available"] + from_catalogue = requires_cla._norm_license_name(entry["spdx_id"]) + from_allowlist = requires_cla._norm_license_name( + "LicenseRef-Broadcom_Source_Available") + self.assertEqual( + from_catalogue, from_allowlist, + f"catalogue spells it {entry['spdx_id']!r} and the allowlist spells it " + "'LicenseRef-Broadcom_Source_Available'; _norm_license_name is the only " + "thing making them match, so 48 repos depend on this rule", + ) + + def test_broadcom_source_available_resolves_to_cla_from_catalogue_id(self): + self.assertIs(self.override_for("Broadcom_Source_Available"), True) + + def test_broadcom_proprietary_resolves_to_cla_via_the_wildcard(self): + """Without the wildcard this falls through to the base tables, which + list Broadcom_Proprietary as permissive, and the repo gets DCO.""" + self.assertIs(self.override_for("Broadcom_Proprietary"), True) + + class TestOverrideWildcards(unittest.TestCase): """`cla/allowlist.yml` has always documented "simple '*' wildcards", and ships `LicenseRef-Broadcom*` on that basis, but matching was plain set @@ -926,6 +997,18 @@ def test_wildcard_does_not_over_match(self): def test_allow_dco_wildcards_work_too(self): self.assertIs(self.check("LicenseRef-Sample-Thing"), False) + def test_matching_is_case_sensitive_on_already_normalised_input(self): + """Both sides are lowercased by _norm_license_name before matching, so + fnmatchcase is correct and fnmatch's platform-dependent case folding + is not wanted. An upper-case pattern must therefore NOT match.""" + upper = {"license_overrides": {"require_cla": ["LICENSEREF-ZZZ*"]}} + import contextlib, io + with contextlib.redirect_stdout(io.StringIO()): + # the pattern is normalised (lowercased) too, so this still matches + self.assertIs(requires_cla._override_requires_cla("licenseref-zzz-a", upper), True) + # but a raw, un-normalised upper-case subject must not + self.assertIsNone(requires_cla._override_requires_cla("LICENSEREF-ZZZ-A", upper)) + def test_require_cla_wins_over_allow_dco(self): both = {"license_overrides": {"require_cla": ["LicenseRef-X*"], "allow_dco": ["LicenseRef-X*"]}} From 7876f86773e2aa2b980203eb3f7df1684b55e47f Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 22 Sep 2026 12:06:33 -0500 Subject: [PATCH 3/4] test(compliance): pin the exact-match branch, found by a full mutation sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ran all 22 mutations reachable from the two fixes rather than the handful I had guessed at. 17 were caught; of the 5 that were not, four are equivalent mutants and one was a real hole. The hole: every fixture in TestOverrideWildcards pairs an exact entry with a wildcard over the same namespace — `LicenseRef-Broadcom_Source_Available` alongside `LicenseRef-Broadcom*` — so the wildcard answers first and a broken exact branch is invisible. Verified directly: with `elif norm_license == pattern` disabled, an exact-only allowlist returns None instead of True for 'mit', and the whole suite still passed. Added a fixture with no wildcard at all, covering require_cla, allow_dco, and a non-match. The other four are equivalent or correct, each checked rather than assumed: - "glob always taken": fnmatchcase(x, x) is exact equality for any pattern without metacharacters, so forcing every pattern through fnmatch changes no outcome. - "isinstance guard removed" and "except reset removed", individually: each is masked by the other. Removing BOTH does fail the suite, which is what defence in depth is supposed to look like. The guard's separate value is the message — "allowlist must be a mapping, got NoneType" rather than "'NoneType' object has no attribute 'get'". - "exact Broadcom entry deleted": the wildcard now covers it. That is the fix working. 18/22 caught, the remaining four explained. 81 tests, OK. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_policy_selector.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 7a7d2ba..7db4e4e 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -980,6 +980,25 @@ def check(self, license_id): def test_exact_entry_still_matches(self): self.assertIs(self.check("LicenseRef-Broadcom_Source_Available"), True) + def test_exact_entries_match_when_no_wildcard_covers_them(self): + """Pins the non-glob branch of _matches_any independently. + + Every other fixture here pairs an exact entry with a wildcard over the + same namespace (`LicenseRef-Broadcom_Source_Available` alongside + `LicenseRef-Broadcom*`), so the wildcard masks a broken exact branch. + Verified: with the exact branch removed, an exact-only allowlist stops + matching and every one of those tests still passed. This fixture has + no wildcard, so the exact path has to work on its own. + """ + exact_only = {"license_overrides": {"require_cla": ["MIT", "GPL-2.0"], + "allow_dco": ["Apache-2.0"]}} + import contextlib, io + with contextlib.redirect_stdout(io.StringIO()): + self.assertIs(requires_cla._override_requires_cla("mit", exact_only), True) + self.assertIs(requires_cla._override_requires_cla("gpl-2.0", exact_only), True) + self.assertIs(requires_cla._override_requires_cla("apache-2.0", exact_only), False) + self.assertIsNone(requires_cla._override_requires_cla("bsd-3-clause", exact_only)) + def test_wildcard_catches_other_broadcom_licences(self): """The gap this closes: without it, LicenseRef-Broadcom-Proprietary falls through to the base tables, where the canonical matcher strips From 4dcad4ec2200fa5d0e115986764def1382795a03 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 22 Sep 2026 12:11:26 -0500 Subject: [PATCH 4/4] test(compliance): pin sweep safety of the fail-closed guard Found by reading the diff rather than by mutating it. cla_sweeper fetches the config once and threads the same dict through every PR in the sweep, and the new guard clears allowlist_repos. Rebinding the local name is safe; calling .clear() on it would strip the DCO downgrade from every repo processed after the first unreadable-allowlist PR in that sweep. The code rebinds and is correct. Nothing pinned it, so a later refactor to .clear() or a slice assignment would pass. Verified the test fails against exactly that mutation. 82 tests, OK. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_policy_selector.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 7db4e4e..6a39af2 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -518,6 +518,22 @@ def test_unreadable_allowlist_forces_cla_not_dco(self): allowlist_repos=["vmware/repo"])) self.assertEqual(fake.statuses()[0]["description"], "CLA Missing") + def test_fail_closed_guard_does_not_corrupt_a_shared_config(self): + """cla_sweeper fetches the config ONCE and threads the same dict + through every PR in the sweep. The guard clears allowlist_repos, so if + it mutated the list in place instead of rebinding the local name, the + first PR to hit it would strip the DCO downgrade from every repo + processed afterwards in that sweep. + """ + shared = dict(self.SHARED_CONFIG, allowlist_ok=False, + allowlist_repos=["vmware/repo", "vmware/other"]) + before = list(shared["allowlist_repos"]) + policy_selector.requires_cla.requires_CLA = lambda *a, **k: False + self.run_pr(paginated_routes={"/issues/5/comments": [], "/pulls/5/commits": []}, + config=shared) + self.assertEqual(shared["allowlist_repos"], before, + "the guard must rebind, not mutate the caller's list") + def test_hand_built_config_without_the_flag_is_treated_as_readable(self): """Back-compat: a caller supplying its own data deliberately (tests, license_report.py) has no allowlist_ok key and must not be forced