From a8b6076802a502e086c8d141438c5f37cec1fb15 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 15 Sep 2026 06:18:24 -0500 Subject: [PATCH 1/3] fix(compliance): never treat a failed fetch as evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit github_api() returns None for every failure mode — a rate-limit 403, a permissions 403, a 404, a network error — so once collapsed into a list those are indistinguishable from "there is nothing here". Four places turned that emptiness into a contributor-visible decision, and each got it wrong: * post_pr_comment's dedup scan was an unpaginated fetch, which returns only GitHub's default first 30 comments (verified against a 2495-comment issue). On a thread with 30+ comments older than ours the scan never saw our own comment and posted another. Each post bumps the PR's updated_at, keeping it inside the sweeper's lookback window, so it posted again every sweep — and each new comment lands later in the listing, so a 30-item window can never catch up. Unbounded, self-sustaining, on a public PR. * check_comments_for_signature read an unreadable thread as "no sign-off", painting failure on a contributor who had in fact signed. * check_dco_commits read an unreadable list as "not signed off". * the signature-registry read treated an unfetchable registry as "this user has not signed" — the primary compliance path, so a single failed fetch failed everyone who had signed. Adds github_api_paginated_checked() returning (items, ok) and threads that signal through each caller. The plain wrapper stays, so the remaining call sites are untouched. process_single_pr now writes no status and posts no comment when any input is unreadable: whatever status a PR already has is a better answer than one derived from a failed read. Also fixes, found while auditing rather than from a failing test: * get_existing_status_state had the same conflation, which quietly disabled the already-resolved short-circuit (#67) during API trouble and let the sweeper repaint — and so bump updated_at on — PRs it should have left alone. * commit.get('sha')[:7] raised TypeError when a commit payload lacked 'sha'. The sweeper's per-PR except swallowed it, silently skipping the PR. * fetch_shared_config now reports completeness and cla_sweeper aborts the sweep when the licence catalogues cannot be loaded. An empty catalogue does not fail one PR, it decides CLA-vs-DCO for every PR in the sweep from no data at all. Completeness deliberately covers only the two licence catalogues, NOT the allowlist. An empty allowlist is a valid configuration meaning "no overrides" and fetch_mothership_file cannot tell empty from failed, so gating on it would mean that emptying cla/allowlist.yml silently aborts every sweep org-wide — far worse than the skewed decision it would prevent. New error_log() emits ::error:: for genuine failures; debug_log() emits ::warning:: for everything including success messages, so a real problem was indistinguishable from noise. Note the sweeper still exits 0 on abort — making a red exit the alerting channel is deliberately left to a follow-up, since turning a 100%-green workflow into a notifier carries its own risk. 93 tests, covering 100% of the executable statements this change adds (measured with an AST-filtered line tracer, not estimated). Every guard is mutation- proven: removing it makes a specific test fail. That standard caught four of our own tests passing vacuously — asserting an outcome that held even with the bug present — and two guards that were not pinned at all despite being claimed as covered. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/cla_sweeper.py | 9 + scripts/policy_selector.py | 250 +++++++++++---- tests/test_policy_selector.py | 578 +++++++++++++++++++++++++++++++--- 3 files changed, 734 insertions(+), 103 deletions(-) diff --git a/scripts/cla_sweeper.py b/scripts/cla_sweeper.py index 840cf03..da9e3ca 100644 --- a/scripts/cla_sweeper.py +++ b/scripts/cla_sweeper.py @@ -124,6 +124,15 @@ def main(): # policy_selector.fetch_shared_config's docstring for why. shared_config = policy_selector.fetch_shared_config(api_root, gh_token) + # Without the licence catalogues every CLA-vs-DCO decision in this sweep + # would be made from empty data, mislabelling repos across the whole org. + # Doing nothing is strictly better: statuses stay as they are and the next + # sweep retries. (A missing allowlist deliberately does NOT abort — see + # fetch_shared_config, where an empty allowlist is a valid configuration.) + if not shared_config.get("complete", True): + policy_selector.error_log("❌ Aborting sweep: shared config could not be loaded.") + return + for repo in repos: full_name = repo.get("full_name") time.sleep(2) # Rate Limit Safety diff --git a/scripts/policy_selector.py b/scripts/policy_selector.py index 42ad28d..ae28c7f 100644 --- a/scripts/policy_selector.py +++ b/scripts/policy_selector.py @@ -94,6 +94,12 @@ def ensure_valid_token(): def debug_log(message): print(f"::warning::{message}") +def error_log(message): + """For genuine failures. debug_log emits ::warning:: for everything — + including success messages — so a real problem is indistinguishable from + noise. Anything logged here is something a human should look at.""" + print(f"::error::{message}") + def is_org_member(api_root, org_name, user, token): url = f"{api_root}/orgs/{org_name}/members/{user}" debug_log(f"🕵️ Checking membership for @{user} in {org_name}...") @@ -167,30 +173,62 @@ def github_api(url, token, method="GET", data=None, _retry_on_rate_limit=True): debug_log(f"Network Error: {e}") return None -# --- NEW: PAGINATION HELPER (Fixes 100 Item Limit) --- -def github_api_paginated(url, token): - """Fetches ALL pages of results.""" +# --- PAGINATION HELPERS (Fixes 100 Item Limit) --- +def github_api_paginated_checked(url, token): + """Fetches ALL pages, and reports whether the fetch actually completed. + + Returns (items, ok). `ok` is False if any page's request failed or came + back malformed. + + This distinction matters because `github_api` returns None for every + failure mode — a rate-limit 403, a permissions 403, a 404, a network + error — so once collapsed into a list they are indistinguishable from + "there is nothing here". Any caller that turns "empty" into a *decision* + ("nobody signed", "we haven't commented yet") will silently turn a + transient error into a wrong answer, and in the commenting case into a + self-sustaining loop: posting bumps the PR's updated_at, which keeps it + inside the sweeper's lookback window, so it re-posts every sweep. + """ all_results = [] page = 1 - + while True: separator = "&" if "?" in url else "?" paged_url = f"{url}{separator}page={page}&per_page=100" - + data = github_api(paged_url, token) - + if data is None: + # Request failed. Whatever we collected so far may be partial, so + # the caller must not read it as a complete picture. + return all_results, False + # Handle cases where API returns dict (like search results) vs list items = data.get("items") if isinstance(data, dict) and "items" in data else data - - if not items or not isinstance(items, list): + + if not isinstance(items, list): + return all_results, False + + if not items: + # A genuinely empty page: the listing is complete. break - + all_results.extend(items) if len(items) < 100: break page += 1 - - return all_results + + return all_results, True + + +def github_api_paginated(url, token): + """Fetches ALL pages of results. + + A failed fetch is indistinguishable from an empty result here. Use + github_api_paginated_checked() wherever that difference changes a + contributor-visible outcome. + """ + items, _ok = github_api_paginated_checked(url, token) + return items # --- UNIFIED RESOURCE LOADER --- def fetch_mothership_file(api_root, file_path, token): @@ -278,21 +316,28 @@ def signature_candidate_text(body): # --- UPDATED: Uses Pagination for Comments --- def check_comments_for_signature(api_root, repo, pr_number, user, doc_type, token): - """Returns (comment_id, signed_doc_type), or (None, None) if the PR author - has not signed. + """Returns (comment_id, signed_doc_type, readable). `signed_doc_type` is the document the author actually signed, which is not necessarily the one this repo requires — the caller compares them. This used to return only a comment id and accept either phrase regardless of `doc_type`, so posting the DCO sentence on a CLA repo passed the gate and then recorded a CLA consent record for someone who never agreed to the CLA. + + `readable` is False when the comment listing could not be fetched. Without + it, an unreadable thread looks exactly like a thread with no sign-off in + it, and we would paint `failure` on a contributor who had in fact signed. """ - if not pr_number: return None, None + if not pr_number: return None, None, True # Fix: Use paginated fetch to see >100 comments url = f"{api_root}/repos/{repo}/issues/{pr_number}/comments" - comments = github_api_paginated(url, token) + comments, readable = github_api_paginated_checked(url, token) + + if not readable: + debug_log(f"⚠️ Could not read comments on {repo}#{pr_number}; treating the sign-off state as unknown.") + return None, None, False - if not comments: return None, None + if not comments: return None, None, True # Check the document this repo actually requires first, so a comment that # happens to contain both sentences is credited to the required one. @@ -320,39 +365,69 @@ def check_comments_for_signature(api_root, repo, pr_number, user, doc_type, toke # would contradict the failure status we go on to set. if current_type == doc_type: add_reaction_to_comment(api_root, repo, c.get("id"), token) - return c.get("id"), current_type + return c.get("id"), current_type, True debug_log(f"❌ No matching CLA or DCO signature found in {len(comments)} comments.") - return None, None + return None, None, True # --- NEW: DCO Commit Check (Fixes 100 Commit Limit) --- def check_dco_commits(api_root, repo, pr_number, token): - """Returns True if ALL commits are signed-off.""" + """Returns (all_signed_off, readable). + + `readable` is False when the commit listing could not be fetched. An + unreadable list previously returned False, i.e. "not signed off", which + fails a contributor whose commits are in fact all signed. + """ url = f"{api_root}/repos/{repo}/pulls/{pr_number}/commits" # Fix: Use paginated fetch for >100 commits - commits = github_api_paginated(url, token) - - if not commits: return False + commits, readable = github_api_paginated_checked(url, token) + + if not readable: + debug_log(f"⚠️ Could not read commits on {repo}#{pr_number}; treating DCO sign-off state as unknown.") + return False, False + + if not commits: return False, True for commit in commits: message = commit.get("commit", {}).get("message", "") if "Signed-off-by:" not in message: - debug_log(f"❌ Commit {commit.get('sha')[:7]} missing DCO Sign-off.") - return False + # A missing sha used to raise TypeError here, which the sweeper's + # per-PR except swallowed — silently skipping the PR entirely. + sha = commit.get("sha") or "unknown" + debug_log(f"❌ Commit {sha[:7]} missing DCO Sign-off.") + return False, True debug_log(f"✅ All {len(commits)} commits have DCO Sign-off.") - return True + return True, True def post_pr_comment(api_root, repo, pr_number, message, token): + """Posts the instruction comment, unless we have already posted it. + + The dedup scan used to be an unpaginated fetch, which returns only + GitHub's default first 30 comments. On a thread with 30+ comments older + than ours, the scan never saw our comment and posted another — and since + each post bumps the PR's updated_at, the PR stayed inside the sweeper's + lookback window and got another comment every sweep, forever. Each new + comment also lands later in the listing, so a 30-item window can never + catch up. + """ if not pr_number: return comments_url = f"{api_root}/repos/{repo}/issues/{pr_number}/comments" - existing_comments = github_api(comments_url, token) - if existing_comments: - for c in existing_comments: - if "I have read the" in c.get("body", "") and "Sign via Comment" in c.get("body", ""): - return - payload = {"body": message} - github_api(comments_url, token, "POST", payload) + existing_comments, readable = github_api_paginated_checked(comments_url, token) + + if not readable: + # We cannot tell whether we already commented. Posting on a failed + # read is exactly how one duplicate becomes an endless stream, so stay + # quiet: the cost is one delayed instruction comment, and the next + # sweep retries. + debug_log(f"⚠️ Could not read comments on {repo}#{pr_number}; not posting instructions this pass.") + return + + for c in existing_comments: + if "I have read the" in c.get("body", "") and "Sign via Comment" in c.get("body", ""): + return + + github_api(comments_url, token, "POST", {"body": message}) def force_merge_check_refresh(api_root, repo, pr_number, token): url = f"{api_root}/repos/{repo}/pulls/{pr_number}" @@ -370,16 +445,23 @@ def set_commit_status(api_root, repo, sha, state, description, target_url, token github_api(url, token, "POST", payload) def get_existing_status_state(api_root, repo, sha, token): - """Returns the current state of our STATUS_CONTEXT on this commit - ('success'/'failure'/'pending'), or None if we haven't posted one yet.""" + """Returns (state, readable). + + `state` is our STATUS_CONTEXT's current state on this commit + ('success'/'failure'/'pending'), or None if we have not posted one yet. + `readable` is False when the status could not be fetched — previously + indistinguishable from "no status yet", which quietly disabled the + already-resolved short-circuit during API trouble and let the sweeper + repaint (and so bump updated_at on) PRs it should have left alone. + """ url = f"{api_root}/repos/{repo}/commits/{sha}/status" data = github_api(url, token) if not data: - return None + return None, False for s in data.get("statuses", []): if s.get("context") == STATUS_CONTEXT: - return s.get("state") - return None + return s.get("state"), True + return None, True from datetime import datetime @@ -491,11 +573,42 @@ def fetch_shared_config(api_root, gh_token): # C. Permissive Names permissive_data = fetch_json_with_fallback(api_root, "data/permissive_names.json", "cla/permissive_names.json", gh_token) or [] + # Completeness deliberately covers only the two licence catalogues. They + # are pure data tables (multi-MB) that cannot legitimately be empty, so a + # falsy value means the fetch failed — and deciding CLA-vs-DCO from an + # empty catalogue would mislabel every PR in the sweep. + # + # The allowlist is NOT included, even though a failed allowlist fetch also + # skews decisions (DCO-only repos would be treated as CLA). An *empty* + # allowlist is a perfectly valid configuration meaning "no overrides", and + # fetch_mothership_file cannot tell empty from failed — so gating on it + # would mean that emptying cla/allowlist.yml silently aborts every sweep + # org-wide. A wrong-but-stricter policy that self-corrects next sweep is + # far better than switching compliance off without telling anyone. + complete = bool(licenses_data) and bool(permissive_data) + if not complete: + error_log( + "❌ Licence catalogues could not be loaded " + f"(licenses={len(licenses_data)}, permissive={len(permissive_data)}). " + "Policy decisions would be made from empty data, so callers should " + "abort rather than guess." + ) + if not raw_allowlist: + # Warning, not error: an intentionally-empty allowlist is valid, and we + # cannot tell it from a failed fetch — so raising this to ::error:: + # would emit a permanent alert for a legitimate configuration. + debug_log( + "⚠️ Allowlist is empty or could not be loaded. Proceeding, but " + "repos configured as DCO-only will be evaluated as CLA until it " + "loads again." + ) + return { "allowlist_data": allowlist_data, "allowlist_repos": allowlist_repos, "licenses_data": licenses_data, "permissive_data": permissive_data, + "complete": complete, } @@ -509,7 +622,10 @@ def process_single_pr(pr_number, pr_head_sha, pr_user, repo_full_name, gh_token, # already-successful PR just repaints the same result and pushes # updated_at again, looping forever every sweep cycle. A new commit gets # a fresh SHA (no prior status), so this only skips true no-op re-checks. - existing_state = get_existing_status_state(api_root, repo_full_name, pr_head_sha, gh_token) + existing_state, status_readable = get_existing_status_state(api_root, repo_full_name, pr_head_sha, gh_token) + if not status_readable: + debug_log(f"⚠️ Could not read the existing status on {pr_head_sha[:7]}; leaving PR #{pr_number} alone this pass.") + return if existing_state == "success": debug_log(f"✅ PR #{pr_number} already has a successful '{STATUS_CONTEXT}' status on {pr_head_sha[:7]}. Skipping re-check.") return @@ -531,6 +647,11 @@ def process_single_pr(pr_number, pr_head_sha, pr_user, repo_full_name, gh_token, # fetching once for many PRs); otherwise fetch fresh — correct either # way, since required-compliance.yml only ever processes one PR per run. config = shared_config or fetch_shared_config(api_root, gh_token) + # .get() so a hand-built config (e.g. in tests) without the key is treated + # as complete rather than raising. + if not config.get("complete", True): + debug_log(f"⚠️ Shared config incomplete; leaving PR #{pr_number} alone rather than guessing its policy.") + return allowlist_data = config["allowlist_data"] allowlist_repos = config["allowlist_repos"] licenses_data = config["licenses_data"] @@ -559,28 +680,47 @@ def process_single_pr(pr_number, pr_head_sha, pr_user, repo_full_name, gh_token, doc_type = "CLA" if is_strict else "DCO" # --- 3. CHECK SIGNATURES (Registry Check) --- + # An unreadable registry is NOT the same as "this user has not signed". + # fetch_mothership_file returns None for a missing file and for every + # failure alike, so without this guard a transient error on the primary + # compliance path fails everyone who has actually signed. has_signed_json = False sig_file_path = f"signatures/{doc_type.lower()}.json" raw_signatures = fetch_mothership_file(api_root, sig_file_path, gh_token) - - if raw_signatures: - try: - data = json.loads(raw_signatures) - contributors = data.get("signedContributors", []) if isinstance(data, dict) else data - for c in contributors: - if isinstance(c, dict): - if c.get("name", "").lower() == pr_user.lower(): has_signed_json = True; break - elif isinstance(c, str): - if c.lower() == pr_user.lower(): has_signed_json = True; break - except Exception as e: - debug_log(f"⚠️ Failed to parse Signatures JSON: {e}") - + + if not raw_signatures: + # Deliberately NOT treated as "nobody has signed": that would fail + # every contributor who has. But note the cost of bailing — if this + # file were genuinely absent rather than briefly unfetchable, PRs on + # this policy would get no status at all and stay blocked by the + # required check. That is silent unless this is loud, hence error_log. + error_log(f"❌ Could not read {sig_file_path}; leaving PR #{pr_number} untouched rather than guessing.") + return + + try: + data = json.loads(raw_signatures) + contributors = data.get("signedContributors", []) if isinstance(data, dict) else data + for c in contributors: + if isinstance(c, dict): + if c.get("name", "").lower() == pr_user.lower(): has_signed_json = True; break + elif isinstance(c, str): + if c.lower() == pr_user.lower(): has_signed_json = True; break + except Exception as e: + # Malformed registry is also not evidence of non-compliance. + error_log(f"❌ Failed to parse {sig_file_path}: {e}. Leaving PR #{pr_number} untouched.") + return + # 4. Check Comments (Forensics Collection) comment_id = None signed_type = None if not has_signed_json: - # Returns (ID, document actually signed), or (None, None) - comment_id, signed_type = check_comments_for_signature(api_root, repo_full_name, pr_number, pr_user, doc_type, gh_token) + # Returns (ID, document actually signed, whether the thread was readable) + comment_id, signed_type, comments_readable = check_comments_for_signature( + api_root, repo_full_name, pr_number, pr_user, doc_type, gh_token) + if not comments_readable: + # Don't paint anything: whatever status the PR already has is a + # better answer than one derived from a failed read. + return # A sign-off only counts if it is for the document this repo requires. has_valid_signature = bool(comment_id) and signed_type == doc_type @@ -593,7 +733,9 @@ def process_single_pr(pr_number, pr_head_sha, pr_user, repo_full_name, gh_token, # properly signed-off commits, and shouldn't lose that fallback. dco_commits_valid = False if doc_type == "DCO" and not has_signed_json and not has_valid_signature: - dco_commits_valid = check_dco_commits(api_root, repo_full_name, pr_number, gh_token) + dco_commits_valid, commits_readable = check_dco_commits(api_root, repo_full_name, pr_number, gh_token) + if not commits_readable: + return doc_url = os.environ.get("CLA_DOC_URL") if doc_type == "CLA" else os.environ.get("DCO_DOC_URL") diff --git a/tests/test_policy_selector.py b/tests/test_policy_selector.py index 1e695de..bf02e31 100644 --- a/tests/test_policy_selector.py +++ b/tests/test_policy_selector.py @@ -88,9 +88,12 @@ class FakeGitHub(object): statuses when it shouldn't. """ - def __init__(self, routes=None, paginated_routes=None): + def __init__(self, routes=None, paginated_routes=None, unreadable=()): self.routes = routes or {} self.paginated_routes = paginated_routes or {} + # URL fragments whose fetch should report failure rather than an empty + # list — the distinction the engine now depends on. + self.unreadable = tuple(unreadable) self.calls = [] def _match(self, table, url): @@ -103,9 +106,15 @@ def github_api(self, url, token, method="GET", data=None, _retry_on_rate_limit=T self.calls.append((method, url, data)) return self._match(self.routes, url) - def github_api_paginated(self, url, token): + def github_api_paginated_checked(self, url, token): self.calls.append(("GET-paginated", url, None)) - return self._match(self.paginated_routes, url) or [] + if any(frag in url for frag in self.unreadable): + return [], False + return (self._match(self.paginated_routes, url) or []), True + + def github_api_paginated(self, url, token): + items, _ok = self.github_api_paginated_checked(url, token) + return items # --- assertion helpers --- @@ -126,16 +135,23 @@ def statuses(self): class PolicySelectorTestCase(unittest.TestCase): """Installs the fake and restores the real functions afterwards.""" - def install(self, routes=None, paginated_routes=None): - fake = FakeGitHub(routes, paginated_routes) - self._saved = (policy_selector.github_api, policy_selector.github_api_paginated) + def install(self, routes=None, paginated_routes=None, unreadable=()): + fake = FakeGitHub(routes, paginated_routes, unreadable) + self._saved = ( + policy_selector.github_api, + policy_selector.github_api_paginated, + policy_selector.github_api_paginated_checked, + ) policy_selector.github_api = fake.github_api policy_selector.github_api_paginated = fake.github_api_paginated + policy_selector.github_api_paginated_checked = fake.github_api_paginated_checked self.addCleanup(self._restore) return fake def _restore(self): - policy_selector.github_api, policy_selector.github_api_paginated = self._saved + (policy_selector.github_api, + policy_selector.github_api_paginated, + policy_selector.github_api_paginated_checked) = self._saved # --------------------------------------------------------------------------- @@ -151,30 +167,30 @@ def call(self, comments, user="contributor", doc_type="CLA"): def test_exact_signature_matches(self): result, _ = self.call([comment(signature_body("CLA"), comment_id=42)]) - self.assertEqual(result, (42, "CLA")) + self.assertEqual(result, (42, "CLA", True)) def test_signature_without_suffix_is_rejected(self): # The phrase alone is not a signature; the "and all future # contributions" suffix is what makes it a standing warranty. result, _ = self.call([comment(signature_body("CLA", suffix=False))]) - self.assertEqual(result, (None, None)) + self.assertEqual(result, (None, None, True)) def test_comment_from_another_user_is_ignored(self): result, _ = self.call([comment(signature_body("CLA"), login="someone-else")]) - self.assertEqual(result, (None, None)) + self.assertEqual(result, (None, None, True)) def test_login_match_is_case_insensitive(self): result, _ = self.call( [comment(signature_body("CLA"), login="ConTributor", comment_id=7)], user="contributor" ) - self.assertEqual(result, (7, "CLA")) + self.assertEqual(result, (7, "CLA", True)) def test_non_breaking_spaces_are_normalised(self): # Copy-pasting the phrase out of a rendered web page can bring # U+00A0 along with it. body = signature_body("CLA").replace(" ", "\xa0") result, _ = self.call([comment(body, comment_id=11)]) - self.assertEqual(result, (11, "CLA")) + self.assertEqual(result, (11, "CLA", True)) def test_signature_is_found_beyond_the_first_page(self): # This function paginates, so a signature buried under a long @@ -182,7 +198,7 @@ def test_signature_is_found_beyond_the_first_page(self): comments = [comment("just a normal comment", comment_id=i) for i in range(60)] comments.append(comment(signature_body("CLA"), comment_id=12345)) result, _ = self.call(comments) - self.assertEqual(result, (12345, "CLA")) + self.assertEqual(result, (12345, "CLA", True)) def test_reaction_is_added_to_the_signing_comment(self): _, fake = self.call([comment(signature_body("CLA"), comment_id=42)]) @@ -192,7 +208,7 @@ def test_reaction_is_added_to_the_signing_comment(self): def test_no_comments_returns_none(self): result, fake = self.call([]) - self.assertEqual(result, (None, None)) + self.assertEqual(result, (None, None, True)) self.assertEqual(fake.writes(), []) def test_signature_in_a_code_fence_still_counts(self): @@ -201,12 +217,12 @@ def test_signature_in_a_code_fence_still_counts(self): # brings the fence markers along is signing in good faith. body = "```text\n" + signature_body("CLA") + "\n```" result, _ = self.call([comment(body, comment_id=21)]) - self.assertEqual(result, (21, "CLA")) + self.assertEqual(result, (21, "CLA", True)) def test_signature_with_surrounding_chat_still_counts(self): body = signature_body("CLA") + "\n\nThanks for the quick review!" result, _ = self.call([comment(body, comment_id=22)]) - self.assertEqual(result, (22, "CLA")) + self.assertEqual(result, (22, "CLA", True)) # --------------------------------------------------------------------------- @@ -234,13 +250,13 @@ def _bot_message(self, doc_type="CLA"): def test_quote_reply_to_the_bot_is_not_a_signature(self): # GitHub's "Quote reply" button prefixes every line with "> ". quoted = "\n".join("> " + ln for ln in self._bot_message().splitlines()) - self.assertEqual(self.call(quoted + "\n\nwhat do I do here?"), (None, None)) + self.assertEqual(self.call(quoted + "\n\nwhat do I do here?"), (None, None, True)) def test_verbatim_paste_of_the_instructions_is_not_a_signature(self): - self.assertEqual(self.call(self._bot_message()), (None, None)) + self.assertEqual(self.call(self._bot_message()), (None, None, True)) def test_quoted_signature_sentence_alone_is_not_a_signature(self): - self.assertEqual(self.call("> " + signature_body("CLA")), (None, None)) + self.assertEqual(self.call("> " + signature_body("CLA")), (None, None, True)) def test_instruction_markers_really_are_in_the_message(self): # The rejection above keys off these markers. If the instruction @@ -251,7 +267,7 @@ def test_instruction_markers_really_are_in_the_message(self): self.assertIn(marker, msg) def test_a_genuine_signature_quoting_nothing_is_unaffected(self): - self.assertEqual(self.call(signature_body("CLA")), (99, "CLA")) + self.assertEqual(self.call(signature_body("CLA")), (99, "CLA", True)) # --------------------------------------------------------------------------- @@ -269,17 +285,17 @@ def call(self, body, doc_type): def test_cla_repo_cla_sentence(self): result, _ = self.call(signature_body("CLA"), "CLA") - self.assertEqual(result, (31, "CLA")) + self.assertEqual(result, (31, "CLA", True)) def test_dco_repo_dco_sentence(self): result, _ = self.call(signature_body("DCO"), "DCO") - self.assertEqual(result, (31, "DCO")) + self.assertEqual(result, (31, "DCO", True)) def test_cla_repo_dco_sentence_reports_the_mismatch(self): # Previously this passed the gate and wrote a CLA consent record for # someone who only ever agreed to the DCO text. result, _ = self.call(signature_body("DCO"), "CLA") - self.assertEqual(result, (31, "DCO")) + self.assertEqual(result, (31, "DCO", True)) def test_no_rocket_reaction_on_a_mismatched_document(self): # A 🚀 reads as "accepted" and would contradict the failure status. @@ -289,7 +305,7 @@ def test_no_rocket_reaction_on_a_mismatched_document(self): def test_both_sentences_present_credits_the_required_one(self): body = signature_body("DCO") + "\n\n" + signature_body("CLA") result, _ = self.call(body, "CLA") - self.assertEqual(result, (31, "CLA")) + self.assertEqual(result, (31, "CLA", True)) # --------------------------------------------------------------------------- @@ -332,11 +348,11 @@ def call(self, payload): def test_returns_our_context_state(self): state, _ = self.call({"statuses": [{"context": policy_selector.STATUS_CONTEXT, "state": "success"}]}) - self.assertEqual(state, "success") + self.assertEqual(state, ("success", True)) def test_ignores_other_contexts(self): state, _ = self.call({"statuses": [{"context": "Some Other CI", "state": "failure"}]}) - self.assertIsNone(state) + self.assertEqual(state, (None, True)) def test_picks_our_context_out_of_a_crowd(self): state, _ = self.call({"statuses": [ @@ -344,15 +360,17 @@ def test_picks_our_context_out_of_a_crowd(self): {"context": policy_selector.STATUS_CONTEXT, "state": "failure"}, {"context": "build", "state": "success"}, ]}) - self.assertEqual(state, "failure") + self.assertEqual(state, ("failure", True)) def test_no_statuses_returns_none(self): state, _ = self.call({"statuses": []}) - self.assertIsNone(state) + self.assertEqual(state, (None, True)) - def test_api_failure_returns_none(self): + def test_api_failure_is_reported_as_unreadable(self): + # Not the same as "no status yet": acting on it would repaint PRs the + # already-resolved short-circuit is meant to leave alone. state, _ = self.call(None) - self.assertIsNone(state) + self.assertEqual(state, (None, False)) # --------------------------------------------------------------------------- @@ -367,25 +385,32 @@ def _commit(self, message, sha="abcdef1234"): return {"sha": sha, "commit": {"message": message}} def test_all_commits_signed_off(self): - self.assertTrue(self.call([ + self.assertEqual(self.call([ self._commit("fix: thing\n\nSigned-off-by: A "), self._commit("fix: other\n\nSigned-off-by: A "), - ])) + ]), (True, True)) def test_one_unsigned_commit_fails_the_whole_pr(self): - self.assertFalse(self.call([ + self.assertEqual(self.call([ self._commit("fix: thing\n\nSigned-off-by: A "), self._commit("fix: forgot the sign-off"), - ])) + ]), (False, True)) def test_no_commits_is_not_compliant(self): - self.assertFalse(self.call([])) + self.assertEqual(self.call([]), (False, True)) # --------------------------------------------------------------------------- # process_single_pr — the decision table # --------------------------------------------------------------------------- -class TestProcessSinglePr(PolicySelectorTestCase): +class ProcessSinglePrHarness(object): + """Shared fixture for process_single_pr tests. + + Deliberately NOT a TestCase. Subclassing a TestCase to reuse its + helpers makes unittest re-run every parent test under each child, + which silently inflated this suite's reported count by 34. + """ + SHARED_CONFIG = { "allowlist_data": {}, "allowlist_repos": [], @@ -408,8 +433,24 @@ def _restore_helpers(self): policy_selector.requires_cla.requires_CLA = self._saved_requires policy_selector.is_org_member = self._saved_is_member - def run_pr(self, routes=None, paginated_routes=None, user="contributor", config=None): - fake = self.install(routes or {}, paginated_routes or {}) + def _default_routes(self): + """process_single_pr now bails rather than guessing when the signature + registry cannot be read, so a readable (empty) registry is the baseline + for any test that is not specifically about that failure.""" + return { + # process_single_pr now refuses to act on an unreadable status, so + # a readable "no status yet" is the baseline. + "/commits/abc123/status": {"statuses": []}, + "/contents/signatures/cla.json": self._registry([]), + "/contents/signatures/dco.json": self._registry([]), + "/users/": {"id": 4242}, + } + + def run_pr(self, routes=None, paginated_routes=None, user="contributor", config=None, + unreadable=()): + merged = dict(self._default_routes()) + merged.update(routes or {}) + fake = self.install(merged, paginated_routes or {}, unreadable) policy_selector.process_single_pr( 5, "abc123", user, "vmware/repo", "tok", "/tmp", "https://api.invalid", shared_config=self.SHARED_CONFIG if config is None else config, @@ -429,13 +470,12 @@ def _registry(self, names): def _writable_registry_routes(self): """Routes that let record_signature get all the way to its PUT, so a - test asserting that no record was written can actually fail.""" - return { - "/users/": {"id": 4242}, - "/contents/signatures/cla.json": self._registry([]), - "/contents/signatures/dco.json": self._registry([]), - } + test asserting that no record was written can actually fail. Same as + the defaults now, kept named for readability at the call sites that + depend on a write being possible.""" + return self._default_routes() +class TestProcessSinglePr(ProcessSinglePrHarness, PolicySelectorTestCase): def test_already_successful_status_short_circuits_with_no_writes(self): # This is the PR #67 fix. Repainting an already-green PR bumps its # updated_at, which puts it back in the sweeper's lookback window. @@ -581,9 +621,14 @@ def routed(url, token, method="GET", data=None, _retry_on_rate_limit=True): return None if put_fails else {"commit": {"sha": "newsha"}} return get_only(url, token, method, data) - self._saved = (policy_selector.github_api, policy_selector.github_api_paginated) + self._saved = ( + policy_selector.github_api, + policy_selector.github_api_paginated, + policy_selector.github_api_paginated_checked, + ) policy_selector.github_api = routed policy_selector.github_api_paginated = fake.github_api_paginated + policy_selector.github_api_paginated_checked = fake.github_api_paginated_checked self.addCleanup(self._restore) # The retry loop sleeps 1-3s with jitter between attempts; skip the wait. @@ -647,10 +692,9 @@ def test_returns_all_four_keys_process_single_pr_indexes(self): # 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"], - ) + for key in ("allowlist_data", "allowlist_repos", "licenses_data", "permissive_data"): + self.assertIn(key, config) + self.assertIn("complete", config) def test_missing_config_degrades_to_empty_rather_than_raising(self): self.install() @@ -686,5 +730,441 @@ def test_dedup_markers_are_present(self): self.assertIn("Sign via Comment", msg) + + +# --------------------------------------------------------------------------- +# A failed fetch is not evidence of anything +# --------------------------------------------------------------------------- +class TestFetchFailureIsNotEmptiness(PolicySelectorTestCase): + """`github_api` returns None for every failure, so a 403/404/network error + collapses into the same empty list as "there is nothing here". Every place + that turned emptiness into a decision could therefore turn a transient + error into a wrong, contributor-visible answer.""" + + def patch_transport(self, fn): + """Replace only `github_api`, so the REAL github_api_paginated_checked + is the thing under test. install() would swap the checked helper out + for the fake and the test would assert on the fake instead.""" + saved = policy_selector.github_api + policy_selector.github_api = fn + self.addCleanup(lambda: setattr(policy_selector, "github_api", saved)) + + def test_checked_helper_reports_a_failed_page(self): + self.patch_transport(lambda *a, **k: None) + items, ok = policy_selector.github_api_paginated_checked("https://api.invalid/x", "tok") + self.assertEqual(items, []) + self.assertFalse(ok) + + def test_checked_helper_reports_a_genuinely_empty_listing(self): + self.patch_transport(lambda *a, **k: []) + items, ok = policy_selector.github_api_paginated_checked("https://api.invalid/x", "tok") + self.assertEqual(items, []) + self.assertTrue(ok) + + def test_checked_helper_reports_a_malformed_page_as_not_ok(self): + self.patch_transport(lambda *a, **k: {"unexpected": "shape"}) + items, ok = policy_selector.github_api_paginated_checked("https://api.invalid/x", "tok") + self.assertFalse(ok) + + def test_checked_helper_reports_partial_pages_as_not_ok(self): + # Page 1 full, page 2 fails: the list is truncated, not complete. + state = {"n": 0} + + def flaky(url, token, method="GET", data=None, _retry_on_rate_limit=True): + state["n"] += 1 + return [{"i": i} for i in range(100)] if state["n"] == 1 else None + + self.patch_transport(flaky) + items, ok = policy_selector.github_api_paginated_checked("https://api.invalid/x", "tok") + self.assertEqual(len(items), 100) + self.assertFalse(ok) + + def test_plain_wrapper_still_returns_just_the_items(self): + # The ~6 existing call sites keep working unchanged. + self.patch_transport(lambda *a, **k: [{"a": 1}]) + self.assertEqual(policy_selector.github_api_paginated("https://api.invalid/x", "tok"), + [{"a": 1}]) + + def test_unreadable_comments_do_not_produce_a_duplicate_comment(self): + # The regression that the pagination fix alone would have introduced: + # posting on a failed read, which bumps updated_at and so re-posts + # every sweep. + fake = self.install(unreadable=("/issues/5/comments",)) + policy_selector.post_pr_comment( + "https://api.invalid", "vmware/repo", 5, "INSTRUCTIONS", "tok") + self.assertEqual(fake.posted_comments(), []) + + def test_readable_comments_still_post_when_absent(self): + fake = self.install(paginated_routes={"/issues/5/comments": []}) + policy_selector.post_pr_comment( + "https://api.invalid", "vmware/repo", 5, "INSTRUCTIONS", "tok") + self.assertEqual(len(fake.posted_comments()), 1) + + def test_instruction_comment_beyond_the_first_thirty_is_still_found(self): + # The original defect: the dedup scan was unpaginated, so it saw only + # GitHub's default first 30 comments. + comments = [comment("chatter %d" % i, comment_id=i) for i in range(30)] + comments.append(instruction_comment(comment_id=9001)) + comments += [comment("more chatter %d" % i, comment_id=100 + i) for i in range(5)] + fake = self.install(paginated_routes={"/issues/5/comments": comments}) + policy_selector.post_pr_comment( + "https://api.invalid", "vmware/repo", 5, "INSTRUCTIONS", "tok") + self.assertEqual(fake.posted_comments(), []) + + def test_unreadable_comments_report_unknown_not_unsigned(self): + self.install(unreadable=("/issues/5/comments",)) + result = policy_selector.check_comments_for_signature( + "https://api.invalid", "vmware/repo", 5, "contributor", "CLA", "tok") + self.assertEqual(result, (None, None, False)) + + def test_unreadable_commits_report_unknown_not_unsigned(self): + self.install(unreadable=("/pulls/5/commits",)) + result = policy_selector.check_dco_commits( + "https://api.invalid", "vmware/repo", 5, "tok") + self.assertEqual(result, (False, False)) + + +class TestUnreadableInputLeavesTheStatusAlone(ProcessSinglePrHarness, PolicySelectorTestCase): + """Whatever status a PR already has is a better answer than one derived + from a fetch that failed, so process_single_pr writes nothing at all.""" + + def test_unreadable_comments_write_no_status_and_no_comment(self): + fake = self.run_pr(unreadable=("/issues/5/comments",)) + self.assertEqual(fake.statuses(), []) + self.assertEqual(fake.posted_comments(), []) + + def test_unreadable_registry_writes_no_status_and_no_comment(self): + fake = self.run_pr(routes={"/contents/signatures/cla.json": None}) + self.assertEqual(fake.statuses(), []) + self.assertEqual(fake.posted_comments(), []) + + def test_unreadable_commits_write_no_status_on_a_dco_repo(self): + policy_selector.requires_cla.requires_CLA = lambda *a, **k: False + fake = self.run_pr( + paginated_routes={"/issues/5/comments": []}, + unreadable=("/pulls/5/commits",)) + self.assertEqual(fake.statuses(), []) + + def test_a_readable_but_unsigned_pr_is_still_failed(self): + # The guard must not become a blanket excuse to do nothing. + fake = self.run_pr(paginated_routes={"/issues/5/comments": []}) + self.assertEqual([s["state"] for s in fake.statuses()], ["failure"]) + self.assertEqual(len(fake.posted_comments()), 1) + + +# --------------------------------------------------------------------------- +# Edge cases found by auditing rather than by a failing test +# --------------------------------------------------------------------------- +class TestPaginationBoundaries(PolicySelectorTestCase): + def patch_transport(self, fn): + saved = policy_selector.github_api + policy_selector.github_api = fn + self.addCleanup(lambda: setattr(policy_selector, "github_api", saved)) + + def test_search_style_items_envelope_is_unwrapped_and_paged(self): + state = {"n": 0} + + def search(url, token, method="GET", data=None, _retry_on_rate_limit=True): + state["n"] += 1 + return {"items": [{"x": i} for i in range(100)]} if state["n"] == 1 else {"items": []} + + self.patch_transport(search) + items, ok = policy_selector.github_api_paginated_checked("https://api.invalid/search?q=1", "tok") + self.assertEqual(len(items), 100) + self.assertTrue(ok) + self.assertEqual(state["n"], 2) + + def test_full_page_followed_by_an_empty_page_is_complete(self): + # The boundary case: exactly 100 items means "maybe more", so it pages + # again; an empty second page means the listing really is done. + state = {"n": 0} + + def two_pages(url, token, method="GET", data=None, _retry_on_rate_limit=True): + state["n"] += 1 + return [{"i": i} for i in range(100)] if state["n"] == 1 else [] + + self.patch_transport(two_pages) + items, ok = policy_selector.github_api_paginated_checked("https://api.invalid/x", "tok") + self.assertEqual(len(items), 100) + self.assertTrue(ok) + + +class TestCommitWithoutShaDoesNotCrash(PolicySelectorTestCase): + def test_unsigned_commit_missing_its_sha(self): + # commit.get('sha')[:7] raised TypeError, which the sweeper's per-PR + # except swallowed — the PR was skipped with no status at all. + self.install(paginated_routes={ + "/pulls/5/commits": [{"commit": {"message": "no sign-off here"}}] + }) + self.assertEqual( + policy_selector.check_dco_commits("https://api.invalid", "vmware/repo", 5, "tok"), + (False, True)) + + +class TestUnreadableStatusIsNotAbsentStatus(PolicySelectorTestCase): + def test_failed_status_fetch_reports_unreadable(self): + self.install(routes={}) # no route -> github_api returns None + self.assertEqual( + policy_selector.get_existing_status_state("https://api.invalid", "vmware/repo", "abc123", "tok"), + (None, False)) + + def test_absent_status_is_readable_but_none(self): + self.install(routes={"/commits/abc123/status": {"statuses": []}}) + self.assertEqual( + policy_selector.get_existing_status_state("https://api.invalid", "vmware/repo", "abc123", "tok"), + (None, True)) + + def test_present_status_is_readable(self): + self.install(routes={"/commits/abc123/status": { + "statuses": [{"context": policy_selector.STATUS_CONTEXT, "state": "failure"}]}}) + self.assertEqual( + policy_selector.get_existing_status_state("https://api.invalid", "vmware/repo", "abc123", "tok"), + ("failure", True)) + + +class TestIncompleteConfigAborts(PolicySelectorTestCase): + def test_fetch_shared_config_flags_a_total_failure(self): + self.install() # every mothership fetch returns None + config = policy_selector.fetch_shared_config("https://api.invalid", "tok") + self.assertFalse(config["complete"]) + + def test_process_single_pr_writes_nothing_on_incomplete_config(self): + # An empty catalogue does not fail one PR — it decides CLA-vs-DCO for + # every PR in the sweep from no data. + # Every *other* guard must be satisfied, or this test passes for the + # wrong reason: without a readable registry process_single_pr bails + # there first and the config guard is never reached. + import base64 + registry = {"content": base64.b64encode( + json.dumps({"signedContributors": []}).encode()).decode(), "sha": "filesha"} + fake = self.install(routes={ + "/commits/abc123/status": {"statuses": []}, + "/contents/signatures/cla.json": registry, + "/contents/signatures/dco.json": registry, + }, paginated_routes={"/issues/5/comments": []}) + saved = policy_selector.is_org_member + saved_req = policy_selector.requires_cla.requires_CLA + policy_selector.is_org_member = lambda *a, **k: False + policy_selector.requires_cla.requires_CLA = lambda *a, **k: True + self.addCleanup(lambda: setattr(policy_selector, "is_org_member", saved)) + self.addCleanup(lambda: setattr(policy_selector.requires_cla, "requires_CLA", saved_req)) + policy_selector.process_single_pr( + 5, "abc123", "contributor", "vmware/repo", "tok", "/tmp", "https://api.invalid", + shared_config={"allowlist_data": {}, "allowlist_repos": [], + "licenses_data": [], "permissive_data": [], "complete": False}) + self.assertEqual(fake.statuses(), []) + self.assertEqual(fake.posted_comments(), []) + + def test_a_config_without_the_key_is_treated_as_complete(self): + # Back-compat: hand-built configs (including this suite's own) predate + # the key and must not be read as broken. + self.assertTrue({"allowlist_data": {}}.get("complete", True)) + + +class TestConfigCompletenessIsNarrowlyScoped(PolicySelectorTestCase): + """Completeness must cover the licence catalogues and NOT the allowlist. + Gating on the allowlist would mean that emptying cla/allowlist.yml — a + valid configuration meaning "no overrides" — silently aborts every sweep + org-wide, which is far worse than the skewed decision it would prevent.""" + + def _b64(self, text): + import base64 + return {"content": base64.b64encode(text.encode()).decode()} + + def test_empty_allowlist_does_not_make_the_config_incomplete(self): + self.install(routes={ + "allowlist": self._b64(""), + "licenses_all": self._b64('[{"spdx_id": "MIT"}]'), + "permissive": self._b64('["MIT"]'), + }) + config = policy_selector.fetch_shared_config("https://api.invalid", "tok") + self.assertTrue(config["complete"]) + + def test_missing_licence_catalogue_does_make_it_incomplete(self): + self.install(routes={ + "allowlist": self._b64("repos: {}"), + "permissive": self._b64('["MIT"]'), + # licenses_all deliberately absent -> fetch returns None + }) + config = policy_selector.fetch_shared_config("https://api.invalid", "tok") + self.assertFalse(config["complete"]) + + def test_missing_permissive_table_does_make_it_incomplete(self): + self.install(routes={ + "allowlist": self._b64("repos: {}"), + "licenses_all": self._b64('[{"spdx_id": "MIT"}]'), + }) + config = policy_selector.fetch_shared_config("https://api.invalid", "tok") + self.assertFalse(config["complete"]) + + + + +# --------------------------------------------------------------------------- +# Guards that an audit found were NOT pinned by any test +# --------------------------------------------------------------------------- +class TestGuardsThatWereNotPinned(ProcessSinglePrHarness, PolicySelectorTestCase): + """Both guards below were production-correct but unpinned: deleting either + left the suite green, because another code path happened to reach the same + end state. A guard no test can distinguish is a guard the next refactor + silently removes.""" + + def test_unreadable_status_guard_is_reached_and_stops_everything(self): + # Every other process_single_pr test gets a readable status from + # _default_routes, so none of them exercise the caller's guard — + # only the helper's return contract. Override it to None here. + fake = self.run_pr( + routes={"/commits/abc123/status": None}, + paginated_routes={"/issues/5/comments": []}, + ) + self.assertEqual(fake.statuses(), []) + self.assertEqual(fake.posted_comments(), []) + # And prove the guard fired *early*: with the status unreadable we must + # not have gone on to read the registry at all. + registry_reads = [c for c in fake.calls if "/contents/signatures/" in c[1]] + self.assertEqual(registry_reads, []) + + def test_unreadable_registry_guard_is_distinguishable_from_the_parse_guard(self): + # `if not raw_signatures: return` was indistinguishable from letting + # json.loads(None) raise into the except branch — same end state, so + # flipping the guard to `if False:` kept the suite green. A body that + # is present but not JSON separates them: only the parse guard can + # catch that, so this test pins the parse guard... + fake = self.run_pr( + routes={"/contents/signatures/cla.json": { + "content": __import__("base64").b64encode(b"not json at all").decode()}}, + paginated_routes={"/issues/5/comments": []}, + ) + self.assertEqual(fake.statuses(), []) + self.assertEqual(fake.posted_comments(), []) + + def test_absent_registry_is_diagnosed_as_unreadable_not_as_malformed(self): + """The `if not raw_signatures` guard cannot be pinned by end state: with + it disabled, json.loads(None) raises and the parse guard catches it, + reaching the identical outcome. Its distinct value is the diagnosis — an + operator seeing "failed to parse" would go hunting for corrupt JSON when + the real problem is that the fetch failed. So assert on that, and keep + the guard from depending on an accidental TypeError.""" + import contextlib, io + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + self.run_pr( + routes={"/contents/signatures/cla.json": None}, + paginated_routes={"/issues/5/comments": []}, + ) + out = buf.getvalue() + self.assertIn("Could not read signatures/cla.json", out) + self.assertNotIn("Failed to parse", out) + + def test_absent_registry_stops_before_the_comment_scan(self): + # ...and this pins the *first* guard: when the registry cannot be + # fetched at all we must bail before scanning comments. If the first + # guard is removed, json.loads(None) raises immediately and the comment + # scan is likewise skipped — so assert on the ordering signal that + # differs: no comment fetch is attempted either way, but the first + # guard must produce no exception-derived log. Assert the observable + # contract both guards share, plus that we never touched comments. + fake = self.run_pr( + routes={"/contents/signatures/cla.json": None}, + paginated_routes={"/issues/5/comments": [comment(signature_body("CLA"))]}, + ) + self.assertEqual(fake.statuses(), []) + comment_reads = [c for c in fake.calls if c[1].endswith("/comments")] + self.assertEqual(comment_reads, []) + + +# --------------------------------------------------------------------------- +# Gaps found by measuring line coverage of the diff, not by reasoning +# --------------------------------------------------------------------------- +class TestLegacyStringRegistryEntries(ProcessSinglePrHarness, PolicySelectorTestCase): + """signatures/*.json entries are dicts today, but the reader still supports + a bare-string form. That branch had no coverage, so nothing would notice if + it broke — and a repo using the old format would start failing everyone.""" + + def _string_registry(self, names): + import base64 + return {"content": base64.b64encode(json.dumps(names).encode()).decode(), + "sha": "filesha"} + + def test_bare_string_entry_is_recognised(self): + fake = self.run_pr(routes={"/contents/signatures/cla.json": self._string_registry(["contributor"])}) + self.assertEqual([s["state"] for s in fake.statuses()], ["success"]) + self.assertEqual(fake.statuses()[0]["description"], "CLA Signed") + + def test_bare_string_entry_match_is_case_insensitive(self): + fake = self.run_pr(routes={"/contents/signatures/cla.json": self._string_registry(["ConTributor"])}) + self.assertEqual([s["state"] for s in fake.statuses()], ["success"]) + + def test_bare_string_entry_for_a_different_user_does_not_match(self): + fake = self.run_pr( + routes={"/contents/signatures/cla.json": self._string_registry(["someone-else"])}, + paginated_routes={"/issues/5/comments": []}) + self.assertEqual([s["state"] for s in fake.statuses()], ["failure"]) + + +class TestSweeperAbortsOnIncompleteConfig(unittest.TestCase): + """cla_sweeper.main()'s abort guard had ZERO coverage — the widest-blast- + radius guard in the change, since it stops the whole sweep rather than one + PR. Measuring coverage of the diff found this; reasoning about it did not.""" + + def setUp(self): + import cla_sweeper + self.sweeper = cla_sweeper + self.processed = [] + self._saved = { + "paginated": cla_sweeper.github_api_paginated, + "config": policy_selector.fetch_shared_config, + "process": policy_selector.process_single_pr, + "sleep": cla_sweeper.time.sleep, + "ensure": getattr(policy_selector, "ensure_valid_token", None), + } + # cla_sweeper has its OWN paginated helper (different envelope unwrap), + # so it must be stubbed separately from policy_selector's. + cla_sweeper.github_api_paginated = lambda url, token: ( + [{"full_name": "vmware/repo"}] if "installation/repositories" in url + else [{"number": 5, "updated_at": "2099-01-01T00:00:00Z", + "head": {"sha": "abc123"}, "user": {"login": "contributor"}, + "draft": False}]) + policy_selector.process_single_pr = lambda *a, **k: self.processed.append(a) + cla_sweeper.time.sleep = lambda _s: None + policy_selector.ensure_valid_token = lambda: None + os.environ.setdefault("GH_TOKEN", "test-token") + self.addCleanup(self._restore) + + def _restore(self): + self.sweeper.github_api_paginated = self._saved["paginated"] + policy_selector.fetch_shared_config = self._saved["config"] + policy_selector.process_single_pr = self._saved["process"] + self.sweeper.time.sleep = self._saved["sleep"] + if self._saved["ensure"]: + policy_selector.ensure_valid_token = self._saved["ensure"] + + def test_incomplete_config_aborts_before_touching_any_pr(self): + policy_selector.fetch_shared_config = lambda *a, **k: { + "allowlist_data": {}, "allowlist_repos": [], + "licenses_data": [], "permissive_data": [], "complete": False} + self.sweeper.main() + self.assertEqual(self.processed, [], + "sweep must process nothing when policy data is missing") + + def test_complete_config_does_process_prs(self): + # Positive control: without this, the test above would pass even if + # main() were broken in some unrelated way. + policy_selector.fetch_shared_config = lambda *a, **k: { + "allowlist_data": {}, "allowlist_repos": [], + "licenses_data": [{"spdx_id": "MIT"}], "permissive_data": ["MIT"], + "complete": True} + self.sweeper.main() + self.assertEqual(len(self.processed), 1) + + def test_config_without_the_complete_key_still_processes(self): + # Back-compat: a config predating the key must not read as broken. + policy_selector.fetch_shared_config = lambda *a, **k: { + "allowlist_data": {}, "allowlist_repos": [], + "licenses_data": [{"spdx_id": "MIT"}], "permissive_data": ["MIT"]} + self.sweeper.main() + self.assertEqual(len(self.processed), 1) + + if __name__ == "__main__": unittest.main() From 3739b6393d898e651b8a0979966e940201764a98 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 15 Sep 2026 06:19:10 -0500 Subject: [PATCH 2/3] chore(compliance): temporarily point sweeper checkout at this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TEMPORARY, to live-test the fetch-failure fix before merging. Revert before merging this branch. `schedule:` only fires from the default branch, so the production cron keeps running main's scripts; a `workflow_dispatch` from this branch checks out this branch's scripts instead. Full-fidelity production run, no effect on the cron. Note what this can and cannot prove: every failure path in this change is unreachable on demand (you cannot ask GitHub to 403 you), so the run only confirms the happy path is unbroken across all 84 repos. That is the failure mode worth checking here, since the change altered the return arity of four functions and added several early returns — a missed call site would crash. --- .github/workflows/cla_sweeper.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cla_sweeper.yml b/.github/workflows/cla_sweeper.yml index 36362d2..11e4ab6 100644 --- a/.github/workflows/cla_sweeper.yml +++ b/.github/workflows/cla_sweeper.yml @@ -44,7 +44,7 @@ jobs: repository: ${{ vars.CENTRAL_ORG }}/.github token: ${{ steps.app-token.outputs.token }} path: .github-tools - ref: main + ref: fix/distinguish-fetch-failure-from-empty # TEMPORARY: live-test before merge, revert before merging sparse-checkout: | scripts From 9fa4c64d55fe09257c5307e2e7521ebfd9e5bf28 Mon Sep 17 00:00:00 2001 From: Amr AbuSair Date: Tue, 15 Sep 2026 06:45:27 -0500 Subject: [PATCH 3/3] Revert "chore(compliance): temporarily point sweeper checkout at this branch" This reverts commit 3739b6393d898e651b8a0979966e940201764a98. --- .github/workflows/cla_sweeper.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cla_sweeper.yml b/.github/workflows/cla_sweeper.yml index 11e4ab6..36362d2 100644 --- a/.github/workflows/cla_sweeper.yml +++ b/.github/workflows/cla_sweeper.yml @@ -44,7 +44,7 @@ jobs: repository: ${{ vars.CENTRAL_ORG }}/.github token: ${{ steps.app-token.outputs.token }} path: .github-tools - ref: fix/distinguish-fetch-failure-from-empty # TEMPORARY: live-test before merge, revert before merging + ref: main sparse-checkout: | scripts