diff --git a/dependabot/merge.py b/dependabot/merge.py index ab64702..f21dd5a 100644 --- a/dependabot/merge.py +++ b/dependabot/merge.py @@ -202,11 +202,18 @@ def checks_readable(gh, owner, kind): wide-enough page closes that; where an account has only public repositories there is no private blindness to detect and a public probe is complete for it. """ + def denied(detail): + """Evidence that the permission is missing, as opposed to any other failure.""" + return "403" in detail or "not accessible by integration" in detail.lower() + listing = f"{'orgs' if kind == 'org' else 'users'}/{owner}/repos?per_page=30&sort=pushed" try: repos = gh.api(listing) or [] except Exception as exc: - return False, f"cannot list repositories: {str(exc)[:200]}" + detail = str(exc)[:200] + if denied(detail): + return False, f"cannot list repositories: {detail}" + return None, f"cannot list repositories: {detail}" # False sorts before True, so private repositories come first. repos = sorted(repos, key=lambda r: not r.get("private")) @@ -222,11 +229,17 @@ def checks_readable(gh, owner, kind): return True, f"verified on {full}@{branch} ({scope})" except Exception as exc: detail = str(exc)[:200] - # An empty repository answers 409 and proves nothing either way; keep looking. - if "empty" in detail.lower() or "409" in detail: - last = f"{full} is empty" - continue - return False, f"cannot read check runs on {full}@{branch}: {detail}" + # Fail only on EVIDENCE that the permission is missing, and treat every other + # error as inconclusive. Enumerating the benign shapes instead was wrong once + # already: this skipped 409 and the word "empty", but an empty repository whose + # default_branch is nominally `main` answers 422 "No commit found for SHA" on + # *this* endpoint while /commits answers 409 -- so `simsange`, whose newest + # private repo is the empty `simsange/documentation`, was reported as a + # permission failure and would have gone red every quiet week. + if denied(detail): + return False, f"cannot read check runs on {full}@{branch}: {detail}" + last = f"{full}@{branch}: {detail}" + continue return None, last or "no repository with a default branch to probe" diff --git a/dependabot/tests/test_merge.py b/dependabot/tests/test_merge.py index d1867bf..c6017b0 100644 --- a/dependabot/tests/test_merge.py +++ b/dependabot/tests/test_merge.py @@ -174,6 +174,21 @@ class NonVacuityTest(unittest.TestCase): EMPTY_SEARCH = {"search/issues": [{"items": []}]} + # The real shapes this control has to tell apart. A generic exception is not a 403, and + # testing with one is how the first version came to treat an empty repository's 422 as a + # permission failure. + DENIED = "gh api --paginate --slurp: gh: Resource not accessible by integration (HTTP 403)" + NO_COMMIT = "gh api --paginate --slurp: gh: No commit found for SHA: main (HTTP 422)" + + def gh_raising(self, routes, on_check_runs): + """FakeGh that raises a specific error for check-runs paths.""" + class Raising(FakeGh): + def api(self, path, paginate=False): + if "/check-runs" in path: + raise common.GhError(on_check_runs) + return super().api(path, paginate=paginate) + return Raising(routes) + def repos_route(self, **extra): routes = dict(self.EMPTY_SEARCH) routes["orgs/laranail/repos"] = [{"full_name": "laranail/emojis", "default_branch": "main"}] @@ -185,8 +200,8 @@ def test_quiet_run_passes_when_the_control_verifies(self): self.assertEqual(merge.main(["--owner", "laranail"], gh=gh), 0) def test_quiet_run_FAILS_when_checks_are_unreadable(self): - # No route for the check-runs path, so FakeGh raises -- the shape of a 403. - gh = FakeGh(self.repos_route()) + # The real 403, not a generic exception: that is the only evidence of a missing permission. + gh = self.gh_raising(self.repos_route(), self.DENIED) self.assertEqual(merge.main(["--owner", "laranail"], gh=gh), 1) def test_quiet_run_passes_when_there_is_nothing_to_probe(self): @@ -195,13 +210,36 @@ def test_quiet_run_passes_when_there_is_nothing_to_probe(self): self.assertEqual(merge.main(["--owner", "laranail"], gh=gh), 0) def test_an_empty_repository_is_not_read_as_a_permission_failure(self): - class EmptyRepoGh(FakeGh): + gh = self.gh_raising(self.repos_route(), "gh: Git Repository is empty. (HTTP 409)") + self.assertEqual(merge.main(["--owner", "laranail"], gh=gh), 0) + + def test_an_empty_repo_answering_422_is_not_a_permission_failure(self): + """The shape that broke the first version. `simsange`'s newest private repository is + the empty `simsange/documentation`, whose default_branch is nominally `main`: /commits + answers 409 but commits/main/check-runs answers 422 'No commit found for SHA'. The + enumerate-the-benign-shapes version matched 409 and the word 'empty', missed 422, and + reported a permission failure that would have gone red every quiet week.""" + gh = self.gh_raising(self.repos_route(), self.NO_COMMIT) + self.assertEqual(merge.main(["--owner", "laranail"], gh=gh), 0) + ok, detail = merge.checks_readable(gh, "laranail", "org") + self.assertIsNone(ok, "an inconclusive error must not become a permission verdict") + self.assertIn("422", detail) + + def test_an_unlistable_account_is_inconclusive_unless_denied(self): + class NoList(FakeGh): + def __init__(self, routes, err): + super().__init__(routes) + self.err = err + def api(self, path, paginate=False): - if "/check-runs" in path: - raise common.GhError("gh: Git Repository is empty. (HTTP 409)") + if "/repos?" in path: + raise common.GhError(self.err) return super().api(path, paginate=paginate) - self.assertEqual(merge.main(["--owner", "laranail"], gh=EmptyRepoGh(self.repos_route())), 0) + ok, _ = merge.checks_readable(NoList(self.EMPTY_SEARCH, "gh: Not Found (HTTP 404)"), "laranail", "org") + self.assertIsNone(ok) + ok, _ = merge.checks_readable(NoList(self.EMPTY_SEARCH, self.DENIED), "laranail", "org") + self.assertIs(ok, False) def test_control_prefers_a_private_repository(self): """`checks: read` is enforced on private repos only, so probing a public one can @@ -213,10 +251,18 @@ def test_control_prefers_a_private_repository(self): {"full_name": "laranail/pub", "default_branch": "main", "private": False}, {"full_name": "laranail/priv", "default_branch": "main", "private": True}, ] - # Only the PUBLIC repo is readable -- the private one 403s, as in the real run. + # Only the PUBLIC repo is readable; the private one answers a real 403, as in the run. routes["repos/laranail/pub/commits/main/check-runs"] = [{"check_runs": []}] - gh = FakeGh(routes) - ok, detail = merge.checks_readable(gh, "laranail", "org") + + denied = self.DENIED + + class PrivateDenied(FakeGh): + def api(self, path, paginate=False): + if "laranail/priv" in path and "/check-runs" in path: + raise common.GhError(denied) + return super().api(path, paginate=paginate) + + ok, detail = merge.checks_readable(PrivateDenied(routes), "laranail", "org") self.assertIs(ok, False, "a public-only read must not pass as verification") self.assertIn("laranail/priv", detail) @@ -239,7 +285,9 @@ def test_a_public_only_account_still_verifies(self): def test_control_reports_absence_not_just_presence(self): # The negative half: a control that can only ever say "readable" proves nothing. - ok, detail = merge.checks_readable(FakeGh(self.repos_route()), "laranail", "org") + # It must answer False on the real 403, and only on that. + gh = self.gh_raising(self.repos_route(), self.DENIED) + ok, detail = merge.checks_readable(gh, "laranail", "org") self.assertIs(ok, False) self.assertIn("cannot read check runs", detail)