Repository navigation
chore(compliance): remove dead workflow-override section from the CLA allowlist - #74
Merged
Merged
Conversation
… allowlist 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Collaborator
Author
|
Two follow-ups pushed. Trimmed the header. It was 16 lines of history above ten lines of config — the file read as mostly prose. Now three lines: what it drives, and that bypasses live in Closed a test gap this change exposed. Every existing test injects allowlist data as a dict, so none read the shipped
Verified by mutation, not assumed:
All three caught. 61 tests, OK. |
…ts reachable
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) <noreply@anthropic.com>
Collaborator
Author
|
Ran an independent review at high effort — six reviewers, all findings verified by executing the code rather than reading it. They converged, and three of the problems were mine. Pushed fixes. The tests could never run for the change they guard
The header I wrote was also wrongI claimed the file "does NOT decide who skips the gate." It does. It now states both effects, names all three early returns in The stale-key guard had the wrong keysIt denied Also fixed
Mutation-tested, not assumedSix cases now fail that should — four of which the previous version missed: Deliberately out of scopeBoth pre-existing, both wanting their own change:
Also worth noting: |
…uite collapsing
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 <owner>/<repo> or just <repo>" 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) <noreply@anthropic.com>
aabusair
pushed a commit
that referenced
this pull request
Sep 22, 2026
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) <noreply@anthropic.com>
aabusair
added a commit
that referenced
this pull request
Sep 22, 2026
…n unreadable allowlist (#75) * fix(compliance): make the licence wildcard work, and fail closed on an unreadable allowlist 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) <noreply@anthropic.com> * test(compliance): close three coverage gaps in the previous commit 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) <noreply@anthropic.com> * test(compliance): pin the exact-match branch, found by a full mutation sweep 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Amr AbuSair <amr.abusair@> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 22, 2026
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.
What
Removes Section A of
cla/allowlist.yml—org_members,dco_on_permissive,users,bots,teams,temporary_exemptions— rewrites the file's header to describe what it actually does, adds a test class that parses the real file, and addscla/**to the CI paths filter so those tests can run.Why
Those keys were read only by
reusable-cla-check.yml, decommissioned whenpolicy_selector.pytook over enforcement. Nothing has read them since, so the file described a policy the gate was not applying.users:listed a contributor who has left the company, which is how this was found.Two of them were not merely inert but stale in a misleading direction:
bots:omittedrenovate[bot], which the hardcodedBOT_ALLOWLISTdoes include; and settingorg_members: falsewould have changed nothing, since the member bypass runs before the allowlist is fetched.What this file actually does
Correcting the original version of this description, which was wrong:
license_overrides.require_cla/allow_dco— read byrequires_cla.py, override a licence's permissive classification.license_overrides.repos.<repo>.require_cla— read bypolicy_selector.fetch_shared_config();falsedowngrades that repo from CLA to DCO. This is live:allowlist_repos == ['vmware/.github'], so this repo is on DCO because of this file.Who skips the gate entirely is decided in
process_single_pr(), via three early returns — an existing successful status on the head SHA, a bot, and an org member. The first was missing from my original description.Verification
Merges are live org-wide at
ref: main, so:license_overridesparses identically before and after_override_requires_cla()run against both old and new files for five licences — identicalMutation-tested rather than assumed. These all now fail, as they should:
users:key re-addedtemp_exemptions(the spelling the retired workflow really read)reposblock deleted / emptied / entry removed / flipped totrue<repo>key, which never matches in productionReview history
This PR was substantially wrong twice, and both rounds are in the commits.
An independent review found the tests could never run for the change class they guard (
cla/**missing from the paths filter), that my replacement header was itself inaccurate, and that the stale-key denylist blocked a spelling nothing read while missing the real one. A follow-up sweep then found the newreposassertion was vacuous — it passed for every regression its docstring claimed to catch.Known issues, deliberately not fixed here
LicenseRef-Broadcom*is inert._override_requires_cladoes exact set membership, no globbing.LicenseRef-Broadcom-Proprietaryis listed permissive in the base tables, so such a repo is gated on DCO instead of CLA today. A real enforcement gap.fetch_shared_configfails open. It assignsyaml.safe_load's result before the.get()that can raise, so a malformed allowlist leavesallowlist_data = Noneand silently drops every override, with only a debug log.spdx_aliasesandforce_spdxare documented but read by nothing.repositories:scalar isextend()-ed without a type check, appending individual characters.data/**is still absent from the CI paths filter, and those catalogues drive licence classification.🤖 Generated with Claude Code