Repository navigation
ci(compliance): run the tests when the licence catalogues change - #76
Merged
Merged
Conversation
`data/licenses_all.json` and `data/permissive_names.json` are production data, not fixtures. policy_selector.py fetches them from the mothership over the API, and fetch_mothership_file has no `?ref=`, so it always reads the DEFAULT BRANCH — a change to either file is live across every gated repo the moment it merges, exactly like `scripts/` and `cla/`. The paths filter did not list them, so regenerating a catalogue merged with no test run at all. That was not theoretical. TestOverrideMatchesTheRealCatalogue parses the real data/licenses_all.json and exists specifically to catch the catalogue being regenerated with a different spelling: the allowlist says `LicenseRef-Broadcom_Source_Available` (underscores), the catalogue says `LicenseRef-Broadcom-Source-Available` (hyphens), and only _norm_license_name makes them meet. 48 repos depend on that convergence. The one change the test was written to catch was the one change that could not trigger it. Verified by mutation: rewriting the catalogue's spdx_id to `LicenseRef-BroadcomSourceAvail` fails test_allowlist_and_catalogue_spellings_converge, and the catalogue was restored byte-identical afterwards. Audited the whole suite for the same class of gap by tracing every repo file it opens — `cla/allowlist.yml` and `data/licenses_all.json` are the only two, and both are now covered. 82 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aabusair
added a commit
that referenced
this pull request
Sep 23, 2026
* fix(compliance): remove config that reads as live but does nothing
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) <noreply@anthropic.com>
* fix(compliance): contain a malformed repo entry to that entry
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) <noreply@anthropic.com>
* test(compliance): make the comment-key guard check the real pattern
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) <noreply@anthropic.com>
---------
Co-authored-by: Amr AbuSair <amr.abusair@>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #74, which added
cla/**to the test workflow's paths filter.data/**was missed, and it belongs there for the same reason.The gap
data/licenses_all.jsonanddata/permissive_names.jsonare production data, not fixtures:fetch_mothership_filetakes no?ref=, so it always reads the default branch. A change to either catalogue is live across every gated repo the moment it merges — exactly likescripts/andcla/. The paths filter did not list them, so regenerating a catalogue merged with no test run at all.Why it matters here specifically
TestOverrideMatchesTheRealCatalogueparses the realdata/licenses_all.json, and its docstring says it exists to catch the catalogue being regenerated with a different spelling. The allowlist saysLicenseRef-Broadcom_Source_Available(underscores); the catalogue's canonicalspdx_idisLicenseRef-Broadcom-Source-Available(hyphens). They match only because_norm_license_namecollapses[\s_]+to-. 48 repos depend on that convergence holding.So the one change the test was written to catch was the one change that could not trigger it.
Verification
Mutation test. Rewrote the catalogue's
spdx_idtoLicenseRef-BroadcomSourceAvailand reran the suite:The guard is real, and it was unreachable from CI. The catalogue was restored byte-identical afterwards (
cmpagainst a pre-mutation copy).Audited for the same class of gap. Traced every repo file the suite opens, via a
sys.addaudithookonopen. Exactly two:cla/allowlist.ymlanddata/licenses_all.json. Both are now covered, so the filter is complete rather than one instance less wrong.Suite green before and after:
Ran 82 tests ... OK.Risk
CI-only. This workflow touches no repo outside this one and cannot affect the Compliance Sweeper or the Legal Compliance Gate. The only behaviour change is that more pull requests run the tests. Worth noting the change is self-testing in the negative direction only: this PR edits
.github/workflows/tests.yml, which was already in the filter, so CI running here does not by itself demonstrate the newdata/**entry works — the mutation test above is the evidence for that.data/permissive_names.jsonis not read by the suite today, but it is production-live on the same path, sodata/**covers it going forward.🤖 Generated with Claude Code