From e423b6122085990ad568f67cc1884e08c4dc1151 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Wed, 23 Sep 2026 09:18:57 -0500 Subject: [PATCH 1/3] fix(compliance): remove config that reads as live but does nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three knobs in this repo look like working configuration and are read by no code. Each is the same failure: something plausible, sitting where configuration lives, that silently does nothing when used. 1. LICENSES_JSON in required-compliance.yml pointed at .github-tools/data/licenses_all.json, but that job's checkout is `sparse-checkout: scripts`, so the path never existed on the runner. Inert only by luck — policy_selector fetches both catalogues over the API and passes them to requires_cla in memory, and license_detector reads LICENSES_JSON from disk only when handed no catalog_data. Any future caller that omitted catalog_data would have found the missing file in production, where ruleset 12239008 pins this workflow to refs/heads/main and it cannot be tested pre-merge. Removed. 2. `spdx_aliases` and a per-repo `force_spdx` were documented in cla/allowlist.yml as ready-to-uncomment examples. No code has ever read either. A commented example is a promise: uncomment it, see no error, conclude it worked. Removed, and the header now says plainly that only the listed keys do anything. 3. The top-level `repos:` block is a FALLBACK, not a supplement — read only when license_overrides.repos is absent or empty. That block is populated, so anything added to a top-level `repos:` is dropped without a word. It now logs a warning naming what it ignored. Also fixed while in those lines: `.get("license_overrides", {})` returns None for a key that is present but null, so `license_overrides:` with nothing under it raised AttributeError, which the surrounding except swallowed into "enforce CLA for every repo". One empty key cost the entire policy. Both lookups now use `or {}`. And widened the tests.yml paths filter to `.github/workflows/**`. The new TestGateWorkflowPaths guards required-compliance.yml, which the filter did not cover — so the guard would not have run when the file it protects was edited. That is the third instance of this exact bug (`cla/**` in #74, `data/**` in #76), so TestTestsWorkflowPathsFilter now asserts the filter covers every file the suite reads, deriving the list from the same constants the tests use rather than a hand-kept copy. Verification: * 95 tests pass (was 92). * Every new guard mutation-tested: reinstating LICENSES_JSON, restoring either commented knob, dropping the shadowing warning, turning the fallback into a merge, restoring the fragile .get(), reverting the paths filter, and desyncing the two filters are each caught by the named test and by no other. Tree restored clean after each. * The allowlist edit is provably inert: replaying origin/main's file and this one through the real fetch_shared_config offline gives identical allowlist_ok, allowlist_repos and allowlist_data, and identical decisions for Broadcom Source Available, Broadcom Proprietary, Apache-2.0 and GPL-2.0. Done offline because fetch_mothership_file has no ?ref= — a pre-merge sweeper run would test the old file and prove nothing while writing real statuses to real PRs. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/required-compliance.yml | 13 +- .github/workflows/tests.yml | 4 +- cla/allowlist.yml | 24 +- scripts/policy_selector.py | 26 +- tests/test_policy_selector.py | 289 ++++++++++++++++++++++ 5 files changed, 338 insertions(+), 18 deletions(-) diff --git a/.github/workflows/required-compliance.yml b/.github/workflows/required-compliance.yml index cc578fd..129c6fb 100644 --- a/.github/workflows/required-compliance.yml +++ b/.github/workflows/required-compliance.yml @@ -93,10 +93,19 @@ jobs: PR_COMMENTS_URL: ${{ github.event.pull_request.comments_url }} CENTRAL_ORG: ${{ vars.CENTRAL_ORG }} - # Paths for Python to find your scripts & data + # Paths for Python to find your scripts. + # + # Only `scripts` is sparse-checked-out above, so nothing here may + # point into another directory. There was a LICENSES_JSON pointing at + # .github-tools/data/licenses_all.json, which never existed on the + # runner. It was inert only by luck: policy_selector fetches both + # catalogues over the API and passes them to requires_cla in memory, + # and license_detector reads LICENSES_JSON from disk only when it is + # handed no catalog_data. Any future call that omits catalog_data + # would have hit a missing file in production, where this workflow is + # pinned to refs/heads/main and cannot be tested pre-merge. TOOLS_PATH: ${{ github.workspace }}/.github-tools PYTHONPATH: ${{ github.workspace }}/.github-tools/scripts - LICENSES_JSON: ${{ github.workspace }}/.github-tools/data/licenses_all.json # Documentation Links (Used in the failure comment) CLA_DOC_URL: "https://${{ vars.CENTRAL_ORG }}.github.io/oss-public-policy/Broadcom_CLA" diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0ca681b..c8666ed 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,7 +16,7 @@ on: - 'tests/**' - 'cla/**' - 'data/**' - - '.github/workflows/tests.yml' + - '.github/workflows/**' push: branches: [main] paths: @@ -24,7 +24,7 @@ on: - 'tests/**' - 'cla/**' - 'data/**' - - '.github/workflows/tests.yml' + - '.github/workflows/**' permissions: contents: read diff --git a/cla/allowlist.yml b/cla/allowlist.yml index 11a1b0e..b346c2e 100644 --- a/cla/allowlist.yml +++ b/cla/allowlist.yml @@ -13,10 +13,20 @@ # # 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. +# block, 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. +# +# The top-level `repos:` block is a FALLBACK, not a supplement: it is read only +# when license_overrides.repos is absent or empty. Because that block is +# currently populated, anything added to a top-level `repos:` here would be +# silently ignored. fetch_shared_config logs a warning when both are present. +# +# Only these keys do anything. Anything else — including plausible-sounding +# knobs like `spdx_aliases` or a per-repo `force_spdx` — is read by no code, +# so adding one is a silent no-op. tests/test_policy_selector.py asserts this +# for live keys AND for commented-out examples, so a well-meaning suggestion +# in a comment cannot quietly become folklore. # # Who skips the gate entirely is decided in policy_selector.process_single_pr(), # not here. Three paths return early, in order: an existing successful @@ -53,11 +63,5 @@ license_overrides: repos: vmware/.github: require_cla: false # false = downgrade this repo to DCO-only - # force_spdx: "MIT" # pretend this is the detected license (rarely needed) # 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. -# spdx_aliases: -# "LicenseRef-BSA": "LicenseRef-Broadcom_Source_Available" diff --git a/scripts/policy_selector.py b/scripts/policy_selector.py index c02b967..083c83a 100644 --- a/scripts/policy_selector.py +++ b/scripts/policy_selector.py @@ -486,10 +486,28 @@ def fetch_shared_config(api_root, gh_token): 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: - repos_config = allowlist_data.get("repos", {}) + # Handle nesting under 'license_overrides' -> 'repos', falling back + # to a top-level 'repos' for backwards compatibility. + # + # `or {}` rather than a .get() default on both lookups: a key that is + # PRESENT but null (`license_overrides:` with nothing under it) yields + # None, and the default only applies when the key is absent. The old + # `.get("license_overrides", {}).get(...)` therefore raised + # AttributeError on that file, which the except below swallowed into + # "enforce CLA everywhere" — the whole allowlist lost to one empty key. + nested_repos = (allowlist_data.get("license_overrides") or {}).get("repos") or {} + legacy_repos = allowlist_data.get("repos") or {} + repos_config = nested_repos or legacy_repos + + # The fallback is unreachable whenever the nested block has entries, + # so a top-level 'repos' added alongside one silently does nothing. + # Say so rather than letting someone conclude their entry is live. + if nested_repos and legacy_repos: + debug_log( + "⚠️ Top-level 'repos:' is ignored because license_overrides.repos " + "is non-empty. Move those entries under license_overrides.repos; " + f"currently ignored: {sorted(legacy_repos)}" + ) if isinstance(repos_config, dict): for r_name, r_config in repos_config.items(): diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 6a39af2..9861469 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -801,6 +801,62 @@ def test_only_keys_live_code_reads(self): "allowlisted after the workflow honouring it was retired.", ) + # Nested keys live code reads, in addition to the top-level ones above: + # requires_cla._override_requires_cla reads require_cla and allow_dco, + # fetch_shared_config reads require_cla inside each repo entry. + NESTED_KEYS_READ_BY_LIVE_CODE = {"require_cla", "allow_dco"} + + def test_commented_out_keys_are_also_read_by_live_code(self): + """The check above sees live keys only, so a dead knob parked in a + comment is invisible to it. + + Two were: a per-repo `force_spdx` and a top-level `spdx_aliases`, both + written as ready-to-uncomment examples, neither read by any code. A + commented example is a promise — someone uncomments it, sees no error, + and concludes it took effect. That is worse than no documentation, + because the file itself is the most authoritative-looking source here. + + Matches only `:` after a `#`, so prose, bullet + lines, quoted map keys and `owner/repo:` names are all left alone. + """ + import re + allowed = self.READ_BY_LIVE_CODE | self.NESTED_KEYS_READ_BY_LIVE_CODE + found = set() + for line in self.path.read_text(encoding="utf-8").splitlines(): + m = re.match(r"\s*#\s*([a-z][a-z0-9_]*):", line) + if m: + found.add(m.group(1)) + extra = found - allowed + self.assertEqual( + extra, set(), + f"{sorted(extra)} appears as a commented-out key but is read by no " + "code, so uncommenting it is a silent no-op. Either implement it or " + "describe it in prose that cannot be mistaken for working config.", + ) + + def test_commented_key_guard_can_actually_see_a_commented_key(self): + """Guards the guard. The regex above is narrow by design, so a change + that made it match nothing would leave the test passing vacuously on + an empty set. + """ + import re + pattern = re.compile(r"\s*#\s*([a-z][a-z0-9_]*):") + self.assertEqual(pattern.match("# allow_dco:").group(1), "allow_dco") + self.assertEqual(pattern.match(" # force_spdx: \"MIT\"").group(1), "force_spdx") + self.assertIsNone(pattern.match("# vmware/docs-site:"), + "owner/repo keys must not be treated as config knobs") + self.assertIsNone(pattern.match("# The left side is what the detector finds"), + "prose must not be treated as config") + self.assertIsNone(pattern.match('# "LicenseRef-BSA": "x"'), + "quoted map values must not be treated as config knobs") + # And it must still find the real ones in the shipped file. + live = {m.group(1) for m in + (pattern.match(l) for l in self.path.read_text(encoding="utf-8").splitlines()) + if m} + self.assertIn("allow_dco", live, + "the shipped file documents a commented allow_dco; if that " + "went away, this guard is no longer exercised by real input") + def test_license_overrides_shaped_as_requires_cla_expects(self): overrides = self.mapping().get("license_overrides") self.assertIsInstance(overrides, dict) @@ -1055,3 +1111,236 @@ def test_require_cla_wins_over_allow_dco(self): if __name__ == "__main__": unittest.main() + + +# --------------------------------------------------------------------------- +# The gate workflow may only reference paths it actually checks out. +# --------------------------------------------------------------------------- +class TestGateWorkflowPaths(unittest.TestCase): + """`required-compliance.yml` sparse-checks-out part of this repo, then + hands filesystem paths to the job through `env:`. A path pointing outside + that subset names a file that does not exist on the runner. + + This shipped: `LICENSES_JSON` pointed at + `.github-tools/data/licenses_all.json` while the checkout pulled only + `scripts`. It was inert purely by luck — `license_detector` consults that + variable only when handed no `catalog_data`, and production always passes + both catalogues in memory from the API fetch. Ruleset 12239008 pins this + workflow to `refs/heads/main`, so a future caller that omitted + `catalog_data` would have found the missing file in production, with no + pre-merge signal anywhere. + """ + + WORKFLOW = (Path(__file__).resolve().parents[1] + / ".github" / "workflows" / "required-compliance.yml") + + @classmethod + def setUpClass(cls): + import yaml + cls.text = cls.WORKFLOW.read_text(encoding="utf-8") + cls.doc = yaml.safe_load(cls.text) + + def checkout_step(self): + for job in (self.doc.get("jobs") or {}).values(): + for step in job.get("steps", []): + if str(step.get("uses", "")).startswith("actions/checkout"): + return step + self.fail(f"no actions/checkout step found in {self.WORKFLOW}") + + def sparse_dirs(self): + raw = (self.checkout_step().get("with") or {}).get("sparse-checkout") + self.assertIsNotNone( + raw, "the checkout step declares no sparse-checkout; if that is " + "deliberate, this test's premise no longer holds") + return {ln.strip().strip("/") for ln in str(raw).splitlines() if ln.strip()} + + def test_sparse_checkout_is_declared_and_non_empty(self): + self.assertTrue(self.sparse_dirs()) + + def test_every_tools_path_in_env_is_actually_checked_out(self): + """Generic on purpose. Pinning the absence of `LICENSES_JSON` by name + would fail a legitimate future change that reinstated it *and* added + `data` to the sparse-checkout; this passes exactly when the paths and + the checkout agree, which is the property that matters.""" + import re + allowed = self.sparse_dirs() + referenced = {} + for m in re.finditer( + r"([A-Z][A-Z0-9_]*):\s*\$\{\{\s*github\.workspace\s*\}\}/\.github-tools/(\S+)", + self.text, + ): + referenced[m.group(1)] = m.group(2).split("/")[0] + self.assertTrue( + referenced, + "expected at least one .github-tools/ path in env; if the " + "workflow stopped using them this test should be retired, not left " + "passing vacuously", + ) + bad = {var: top for var, top in referenced.items() if top not in allowed} + self.assertEqual( + bad, {}, + f"{bad} point outside the sparse-checkout {sorted(allowed)}, so the " + "path will not exist on the runner. Either drop the variable or add " + "the directory to sparse-checkout in the same change.", + ) + + +# --------------------------------------------------------------------------- +# Top-level `repos:` is a fallback, not a supplement. +# --------------------------------------------------------------------------- +class TestTopLevelReposFallback(PolicySelectorTestCase): + """`fetch_shared_config` reads `license_overrides.repos`, and only falls + back to a top-level `repos:` when that is absent or empty. + + Worth pinning because the fallback is invisible in the shipped file: + `license_overrides.repos` is populated, so a top-level `repos:` added + beside it does nothing at all. Whoever added it would see a valid-looking + entry and no effect. + """ + + 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()} + + NESTED = ("license_overrides:\n" + " repos:\n" + " vmware/nested:\n" + " require_cla: false\n") + LEGACY = ("repos:\n" + " vmware/legacy:\n" + " require_cla: false\n") + + def test_top_level_repos_is_used_when_nested_is_absent(self): + cfg = self._config(self.LEGACY) + self.assertIs(cfg["allowlist_ok"], True) + self.assertEqual(cfg["allowlist_repos"], ["vmware/legacy"]) + + def test_top_level_repos_is_used_when_nested_is_empty(self): + cfg = self._config("license_overrides:\n repos: {}\n" + self.LEGACY) + self.assertEqual(cfg["allowlist_repos"], ["vmware/legacy"]) + + def test_nested_repos_shadows_top_level_entirely(self): + """Not a merge. The legacy entry is dropped, not appended.""" + cfg = self._config(self.NESTED + self.LEGACY) + self.assertEqual( + cfg["allowlist_repos"], ["vmware/nested"], + "top-level repos must be ignored, not merged, when the nested " + "block has entries", + ) + + def test_shadowing_is_reported_rather_than_silent(self): + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + self._config(self.NESTED + self.LEGACY) + out = buf.getvalue() + self.assertIn("Top-level 'repos:' is ignored", out) + self.assertIn("vmware/legacy", out, + "the warning must name what is being ignored, or it " + "cannot be acted on") + + def test_no_warning_when_only_one_source_is_present(self): + for label, body in (("nested only", self.NESTED), ("legacy only", self.LEGACY)): + with self.subTest(label): + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + self._config(body) + self.assertNotIn("Top-level 'repos:' is ignored", buf.getvalue()) + + def test_null_license_overrides_does_not_discard_the_whole_allowlist(self): + """`license_overrides:` present but empty parses to None. A `.get()` + default does not apply to a key that exists with a null value, so + `.get("license_overrides", {}).get("repos")` raised AttributeError, + which the caller swallowed into "enforce CLA for every repo". One + empty key cost the entire policy. + """ + cfg = self._config("license_overrides:\n" + self.LEGACY) + self.assertIs(cfg["allowlist_ok"], True, + "a null license_overrides must not read as an unparseable file") + self.assertEqual(cfg["allowlist_repos"], ["vmware/legacy"]) + + +# --------------------------------------------------------------------------- +# The CI paths filter must cover everything this suite depends on. +# --------------------------------------------------------------------------- +class TestTestsWorkflowPathsFilter(unittest.TestCase): + """`tests.yml` only runs on a paths filter, so a file this suite reads but + the filter omits can be changed with CI green and nothing checking it. + + That has now happened three times: `cla/**` was missing when the tests + that parse the allowlist were added (#74), `data/**` was missing while + TestOverrideMatchesTheRealCatalogue parsed the real catalogue (#76), and + `required-compliance.yml` was missing when TestGateWorkflowPaths was added + to guard it. Each time the guard existed and simply never ran. + + Derives its expectations from the same constants the tests use, rather + than a hand-kept list that would drift out of date in the same way. + """ + + WORKFLOW = (Path(__file__).resolve().parents[1] + / ".github" / "workflows" / "tests.yml") + + @classmethod + def setUpClass(cls): + import yaml + cls.doc = yaml.safe_load(cls.WORKFLOW.read_text(encoding="utf-8")) + # PyYAML parses a bare `on:` key as the boolean True. + cls.triggers = cls.doc.get("on", cls.doc.get(True)) + + @staticmethod + def _covered(path, globs): + """GitHub path-filter semantics, narrowed to the forms we use: + `dir/**` covers anything beneath dir, and a literal path matches + itself.""" + for g in globs: + if g.endswith("/**"): + if path.startswith(g[:-2]): + return True + elif g == path: + return True + return False + + def repo_relative(self, p): + return str(Path(p).resolve().relative_to(Path(__file__).resolve().parents[1])) + + def required_paths(self): + """Files the suite genuinely reads, taken from production constants.""" + root = Path(__file__).resolve().parents[1] + return { + self.repo_relative(requires_cla._ALLOWLIST_PATH), + self.repo_relative(root / "data" / "licenses_all.json"), + self.repo_relative(TestGateWorkflowPaths.WORKFLOW), + self.repo_relative(Path(policy_selector.__file__)), + self.repo_relative(Path(__file__)), + } + + def test_both_triggers_declare_the_same_filter(self): + pr = self.triggers["pull_request"]["paths"] + push = self.triggers["push"]["paths"] + self.assertEqual( + pr, push, + "pull_request and push must agree, or a change can pass pre-merge " + "and go unverified on main, or vice versa", + ) + + def test_filter_covers_every_file_the_suite_reads(self): + globs = self.triggers["pull_request"]["paths"] + missing = sorted(p for p in self.required_paths() if not self._covered(p, globs)) + self.assertEqual( + missing, [], + f"{missing} are read by this suite but not matched by the tests.yml " + f"paths filter {globs}, so changing them runs no tests.", + ) + + def test_coverage_check_rejects_a_path_outside_the_filter(self): + """Guards the guard: _covered must be capable of returning False.""" + globs = ["scripts/**", ".github/workflows/tests.yml"] + self.assertTrue(self._covered("scripts/policy_selector.py", globs)) + self.assertTrue(self._covered(".github/workflows/tests.yml", globs)) + self.assertFalse(self._covered("data/licenses_all.json", globs)) + self.assertFalse(self._covered(".github/workflows/required-compliance.yml", globs), + "a literal glob must not match a sibling file") From ecf494922f4a342ea50d40a5aa8f4a4f3c055e0a Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Wed, 23 Sep 2026 09:31:43 -0500 Subject: [PATCH 2/3] fix(compliance): contain a malformed repo entry to that entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-probing malformed allowlist shapes against origin/main found that the shadowing warning added in the previous commit was itself a regression. `sorted(legacy_repos)` assumed a dict. A top-level `repos:` holding a list of dicts raises TypeError, and an int raises too — both swallowed by the enclosing except into "Enforcing CLA for every repo". origin/main handled both fine, so this commit's diagnostic would have made things worse than the bug it describes. Building a log line must never be the thing that discards the policy. Now renders defensively. Fixed alongside it, same failure family three lines down and pre-existing: `r_config.get("require_cla")` on a null or boolean entry raised AttributeError, taking the whole file with it. A single missing indent under one repo cost every gated repo in the org. Malformed entries are now skipped with a warning naming the repo. Direction stays safe: absence from allowlist_repos means CLA, so a skipped entry is held at the stricter document, and the readable entries survive. Measured against origin/main, per shape of a top-level `repos:`: list of dicts main ok -> branch DISCARDED (regression) -> now ok integer main ok -> branch DISCARDED (regression) -> now ok nested null main DISCARDED (pre-existing) -> now ok nested bool main DISCARDED (pre-existing) -> now ok Verification: * 101 tests pass (was 95). * All 11 mutations across both harnesses caught by their named tests. * test_warning_renders_a_non_dict_legacy_block_without_raising now uses TWO dicts. With one, `sorted()` never performs a comparison, so the test passed with the broken code in place — it could not fail for the scenario its own name describes. Mutation testing caught that; review did not. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/policy_selector.py | 21 ++++++- tests/test_policy_selector.py | 105 ++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/scripts/policy_selector.py b/scripts/policy_selector.py index 083c83a..d36da12 100644 --- a/scripts/policy_selector.py +++ b/scripts/policy_selector.py @@ -503,14 +503,33 @@ def fetch_shared_config(api_root, gh_token): # so a top-level 'repos' added alongside one silently does nothing. # Say so rather than letting someone conclude their entry is live. if nested_repos and legacy_repos: + # `legacy_repos` is whatever YAML produced and may be a list, + # an int, or a string. sorted() raises TypeError on a list of + # dicts, and the except below would turn that into "enforce CLA + # everywhere" — building a diagnostic must never be the thing + # that discards the policy. + ignored = sorted(legacy_repos) if isinstance(legacy_repos, dict) else repr(legacy_repos) debug_log( "⚠️ Top-level 'repos:' is ignored because license_overrides.repos " "is non-empty. Move those entries under license_overrides.repos; " - f"currently ignored: {sorted(legacy_repos)}" + f"currently ignored: {ignored}" ) if isinstance(repos_config, dict): for r_name, r_config in repos_config.items(): + # One malformed entry must cost that entry, not the file. + # `r_config.get(...)` on a null or boolean value raised + # AttributeError, which the except below swallowed into + # enforcing CLA across every gated repo — org-wide blast + # radius from a single missing indent. Skipping instead + # leaves this repo on CLA (absence from the DCO-only list + # is the strict direction) and keeps the rest intact. + if not isinstance(r_config, dict): + debug_log( + f"⚠️ Ignoring repos[{r_name!r}]: expected a mapping, got " + f"{type(r_config).__name__}. That repo stays on CLA." + ) + continue if r_config.get("require_cla") is False: allowlist_repos.append(r_name) diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 9861469..aadda70 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -1344,3 +1344,108 @@ def test_coverage_check_rejects_a_path_outside_the_filter(self): self.assertFalse(self._covered("data/licenses_all.json", globs)) self.assertFalse(self._covered(".github/workflows/required-compliance.yml", globs), "a literal glob must not match a sibling file") + + +# --------------------------------------------------------------------------- +# A malformed entry must cost that entry, not the whole policy. +# --------------------------------------------------------------------------- +class TestMalformedRepoEntriesAreContained(PolicySelectorTestCase): + """`fetch_shared_config` wraps the whole parse in one try/except whose + failure mode is "Enforcing CLA for every repo". Anything that raises in + there — including while building a log line — therefore has an org-wide + blast radius from a single bad indent. + + Two separate faults lived here. `r_config.get("require_cla")` on a null or + boolean value raised AttributeError. And the shadowing warning added in + this change called `sorted()` on a top-level `repos:` of arbitrary YAML + shape, so a list of dicts raised TypeError — a regression caught by + re-probing malformed shapes against origin/main rather than by review. + """ + + def _config(self, body): + self.install(routes={"/contents/cla/allowlist.yml": self._file(body)}) + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + cfg = policy_selector.fetch_shared_config("https://api.invalid", "tok") + return cfg, buf.getvalue() + + @staticmethod + def _file(text): + import base64 + return {"content": base64.b64encode(text.encode()).decode()} + + NESTED = ("license_overrides:\n repos:\n vmware/nested:\n" + " require_cla: false\n") + + # A top-level `repos:` of each shape YAML can produce. Only dicts are + # meaningful; the rest must be inert, never fatal. + LEGACY_SHAPES = { + "list of dicts": "repos:\n - vmware/a: true\n - vmware/b: true\n", + "list of strings": "repos:\n - vmware/a\n - vmware/b\n", + "scalar string": "repos: vmware/a\n", + "integer": "repos: 5\n", + "null": "repos:\n", + } + + def test_odd_top_level_repos_shape_never_discards_the_allowlist(self): + for label, legacy in self.LEGACY_SHAPES.items(): + with self.subTest(label): + cfg, _ = self._config(self.NESTED + legacy) + self.assertIs( + cfg["allowlist_ok"], True, + f"a top-level repos: of type {label} must not cost the whole " + "policy — the except in fetch_shared_config enforces CLA " + "org-wide", + ) + self.assertEqual(cfg["allowlist_repos"], ["vmware/nested"]) + + NESTED_BAD_VALUES = {"null": "", "boolean": " true", "string": " hello", "integer": " 7"} + + def test_malformed_nested_entry_is_skipped_not_fatal(self): + for label, value in self.NESTED_BAD_VALUES.items(): + with self.subTest(label): + cfg, _ = self._config( + f"license_overrides:\n repos:\n vmware/x:{value}\n") + self.assertIs(cfg["allowlist_ok"], True, + f"a repo entry of type {label} must not discard the file") + self.assertEqual(cfg["allowlist_repos"], []) + + def test_a_good_entry_survives_alongside_a_malformed_one(self): + """The property that matters: blast radius is the bad entry, not the file.""" + cfg, _ = self._config( + "license_overrides:\n" + " repos:\n" + " vmware/good:\n" + " require_cla: false\n" + " vmware/bad:\n" + ) + self.assertIs(cfg["allowlist_ok"], True) + self.assertEqual( + cfg["allowlist_repos"], ["vmware/good"], + "one unparseable entry must not take the readable ones with it", + ) + + def test_malformed_entry_stays_on_cla_rather_than_being_let_through(self): + """Skipping must not be mistaken for permitting. allowlist_repos is the + DCO-only set, so absence from it means CLA — the strict direction.""" + cfg, _ = self._config("license_overrides:\n repos:\n vmware/bad:\n") + self.assertNotIn("vmware/bad", cfg["allowlist_repos"]) + + def test_skipped_entry_is_reported(self): + _, out = self._config("license_overrides:\n repos:\n vmware/bad:\n") + self.assertIn("vmware/bad", out) + self.assertIn("expected a mapping", out) + + def test_warning_renders_a_non_dict_legacy_block_without_raising(self): + """Direct pin on the regression: the diagnostic itself must be safe. + + TWO dicts, deliberately. `sorted()` on a single-element list never + performs a comparison, so a one-entry version of this test passes even + with the broken `sorted(legacy_repos)` in place — it cannot fail for + the scenario its own name describes. Found by mutation testing, which + is the only reason this reads as it does. + """ + cfg, out = self._config( + self.NESTED + "repos:\n - vmware/a: true\n - vmware/b: true\n") + self.assertIs(cfg["allowlist_ok"], True) + self.assertIn("Top-level 'repos:' is ignored", out) From 6fe61a7d8c0ece58b0bab686ba9c741a2212efee Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Wed, 23 Sep 2026 09:38:43 -0500 Subject: [PATCH 3/3] test(compliance): make the comment-key guard check the real pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_commented_key_guard_can_actually_see_a_commented_key declared its own copy of the regex, so it validated a duplicate rather than the pattern test_commented_out_keys_are_also_read_by_live_code actually uses. Editing the real one would not have tripped the guard — the exact failure the guard exists to prevent. Declared once as COMMENTED_KEY_RE and used by both; broadening it now fails both tests rather than neither. Found by a vacuity sweep: reverting each changed file in turn and checking which new tests die. Ten survived every revert, which is correct for characterisation tests but meant they were unproven, so each was mutation-tested individually afterwards. All 19 new tests are now provably killable by at least one of 16 mutations. Also confirmed by a 30-case differential against origin/main and a 2,788-case fuzz: no input reaches the DCO-only set without an explicit `require_cla: false` or a `repositories:` entry, and no input escapes an exception. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_policy_selector.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index aadda70..3d544a4 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -28,6 +28,7 @@ """ import json import os +import re import sys import contextlib import io @@ -806,6 +807,11 @@ def test_only_keys_live_code_reads(self): # fetch_shared_config reads require_cla inside each repo entry. NESTED_KEYS_READ_BY_LIVE_CODE = {"require_cla", "allow_dco"} + # Declared once and used by both the check and its guard below. + # Duplicating it meant the guard validated a copy, so editing the + # real pattern would not have tripped it. + COMMENTED_KEY_RE = re.compile(r"\s*#\s*([a-z][a-z0-9_]*):") + def test_commented_out_keys_are_also_read_by_live_code(self): """The check above sees live keys only, so a dead knob parked in a comment is invisible to it. @@ -819,11 +825,10 @@ def test_commented_out_keys_are_also_read_by_live_code(self): Matches only `:` after a `#`, so prose, bullet lines, quoted map keys and `owner/repo:` names are all left alone. """ - import re allowed = self.READ_BY_LIVE_CODE | self.NESTED_KEYS_READ_BY_LIVE_CODE found = set() for line in self.path.read_text(encoding="utf-8").splitlines(): - m = re.match(r"\s*#\s*([a-z][a-z0-9_]*):", line) + m = self.COMMENTED_KEY_RE.match(line) if m: found.add(m.group(1)) extra = found - allowed @@ -839,8 +844,7 @@ def test_commented_key_guard_can_actually_see_a_commented_key(self): that made it match nothing would leave the test passing vacuously on an empty set. """ - import re - pattern = re.compile(r"\s*#\s*([a-z][a-z0-9_]*):") + pattern = self.COMMENTED_KEY_RE self.assertEqual(pattern.match("# allow_dco:").group(1), "allow_dco") self.assertEqual(pattern.match(" # force_spdx: \"MIT\"").group(1), "force_spdx") self.assertIsNone(pattern.match("# vmware/docs-site:"),