From 55a77650ff667b0253094ee16901ab7fcb260884 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Thu, 29 Oct 2026 08:44:22 +0800 Subject: [PATCH] emrg: check-pr-base names the repository it measures --- scripts/check-pr-base.py | 141 ++++++++++++++++++++++++----- tests/test_check_pr_base.py | 173 ++++++++++++++++++++++++++++++++++++ 2 files changed, 294 insertions(+), 20 deletions(-) diff --git a/scripts/check-pr-base.py b/scripts/check-pr-base.py index bc46bcf3d..71e33d088 100644 --- a/scripts/check-pr-base.py +++ b/scripts/check-pr-base.py @@ -59,8 +59,28 @@ 0 no open PR's base is a dead end (bases either reach master, or are an open PR's head - the latter reported as LIVE) 1 at least one open PR is based on a dead end - retarget it before merging -2 the question could not be answered (gh failed, response unparseable, or a - requested PR number is not among the open PRs) +2 the question could not be answered (gh failed, response unparseable, a + requested PR number is not among the open PRs, or no repository could be named + to measure - see "Which repository answered" below) + +Which repository answered +------------------------- +Every verdict here is about exactly one repository, and the first line of every report +names it. `master` and the branch list are that repository's, so a reader who cannot see +which one was read cannot tell whose bases were classified. + +`--repo` names it. Without `--repo` the repository used to be left to gh's own +`{owner}/{repo}` placeholder, which gh resolves from the remotes of the directory this +runs in - correct on an ordinary clone, and wrong on a checkout whose only GitHub remote +is a fork. A fork holds a **copy** of `master` and the branches it had when it was forked, +so the gate answered about the fork and said nothing about it. Measured 2026-10-29 +(`cyc20261029-082021`) on master `7e76668`: the placeholder resolved to `pm25coder/emrg` +with 50 branches against the upstream's 1159, so 1158 upstream bases were reported as "no +longer exists on the remote"; and the upstream master head `7e766683` - the commit this +very file is committed in - read as *not on master*, flipping `_ref_is_on_master` from +True to False. Same commit, same question, two answers, and no line of output naming the +repository that gave either. So the placeholder never reaches gh as a path now: a fork is +refused by name, and any other resolution is reported before the verdicts. What the count covers --------------------- @@ -86,6 +106,12 @@ MASTER = "master" +#: gh's placeholder for "the repository of the directory this runs in". Spelled once, +#: because it is the one token that must never again reach a `repos/...` path: gh resolves +#: it from the cwd's remotes, and on a checkout whose only GitHub remote is a fork that +#: resolution *is* the fork. `_repo_for_the_question` is where it is resolved and refused. +PLACEHOLDER = "{owner}/{repo}" + def _gh(*args: str) -> str: """Run `gh`, failing loud rather than guessing. @@ -123,8 +149,15 @@ def _gh_json(*args: str) -> object: return json.loads(_gh(*args)) -def _open_prs(repo: str | None) -> list[dict]: - """Every open PR with the fields this check needs.""" +def _open_prs(repo: str) -> list[dict]: + """Every open PR **in `repo`**, with the fields this check needs. + + `repo` is always a concrete `owner/name` - the caller named it, or + `_repo_for_the_question` resolved it. The `if repo:` that used to be here was the + second half of the old defect: omitting `-R` is what made gh answer about whatever + repository the checkout resolved to, and leaving the omissible branch in place would + keep that answer one forgotten argument away. + """ args = [ "pr", "list", @@ -134,9 +167,9 @@ def _open_prs(repo: str | None) -> list[dict]: "100", "--json", "number,baseRefName,headRefName,state", + "-R", + repo, ] - if repo: - args += ["-R", repo] payload = _gh_json(*args) if not isinstance(payload, list): raise RuntimeError(f"expected a list of PRs, got {type(payload).__name__}") @@ -147,16 +180,63 @@ def _open_prs(repo: str | None) -> list[dict]: return payload -def _repo_path(repo: str | None) -> str: - """`owner/name` - the caller's, or gh's placeholder for the current repo.""" - return repo if repo else "{owner}/{repo}" +class Unmeasurable(RuntimeError): + """The question cannot be answered, and refusing is the answer. + + The same exit path as any other `RuntimeError` here (rc 2), and named apart so a + reader of the refusal can see which of the two it was: this one means the repository + to measure could not be named, not that gh failed. + """ + + +def _repo_for_the_question(repo: str | None) -> str: + """The concrete `owner/name` this run reads - resolved, never a placeholder. + `--repo` is the caller naming a repository, which is not a guess. Without it, the + repository used to be gh's placeholder and gh resolved it from the remotes of the + checkout - on which a fork is the answer, and a fork is the wrong repository for this + question for two measured reasons: its `master` is a copy that lags, and its branch + list is a snapshot taken at fork time. Measured 2026-10-29 (`cyc20261029-082021`) on + master `7e76668`: the placeholder resolved to `pm25coder/emrg` (50 branches against + the upstream's 1159), and the upstream master head `7e766683` itself read as *not on + master* - the same commit answering False here and True under `--repo argszero/emrg`. -def _branch_heads(repo: str | None) -> dict[str, str]: - """branch name -> head sha, for every branch on the remote.""" + So a fork is refused **by name**, together with the repository it forks, and the + remedy is spelled out; a non-fork resolution is returned so `main` can print it before + any verdict. The non-fork half is load-bearing: an ordinary clone, whose remote is the + repository itself, must keep working without new flags. + """ + if repo: + return repo + meta = _gh_json("api", f"repos/{PLACEHOLDER}") + if not isinstance(meta, dict): + raise Unmeasurable( + f"gh answered {type(meta).__name__} for {PLACEHOLDER}, not a repository - " + f"there is no repository to measure" + ) + resolved = meta.get("full_name") + if not resolved: + raise Unmeasurable( + f"gh did not name the repository it resolved from {PLACEHOLDER}: {meta!r}" + ) + parent = (meta.get("parent") or {}).get("full_name") + if parent: + raise Unmeasurable( + f"refusing to measure: with no --repo, gh resolved this checkout to " + f"{resolved}, which is a fork of {parent}. The base a PR declares and the " + f"master it has to reach both belong to one repository, and a fork holds a " + f"copy of master and the branches it had when it was forked, so a verdict " + f"read here is a verdict about {resolved}, not about {parent}. Name the " + f"repository to measure: --repo {parent}" + ) + return resolved + + +def _branch_heads(repo: str) -> dict[str, str]: + """branch name -> head sha, for every branch of `repo`.""" # `_gh` (not a bare subprocess.run) so a missing `gh` raises the same # RuntimeError as a failing one and takes the rc-2 path. - out = _gh("api", f"repos/{_repo_path(repo)}/branches", "--paginate") + out = _gh("api", f"repos/{repo}/branches", "--paginate") # --paginate concatenates page arrays, which is not valid JSON as a whole; # the endpoint returns an array per page, so parse per document if it parses, # else fall back to line-delimited objects. @@ -175,9 +255,9 @@ def _branch_heads(repo: str | None) -> dict[str, str]: return heads -def _ref_is_on_master(sha: str, repo: str | None) -> bool: - """True when `sha` is an ancestor of (or equal to) master's head.""" - data = _gh_json("api", f"repos/{_repo_path(repo)}/compare/{MASTER}...{sha}") +def _ref_is_on_master(sha: str, repo: str) -> bool: + """True when `sha` is an ancestor of (or equal to) `repo`'s master head.""" + data = _gh_json("api", f"repos/{repo}/compare/{MASTER}...{sha}") status = data.get("status") if status is None: raise RuntimeError(f"compare response has no status: {data}") @@ -187,7 +267,7 @@ def _ref_is_on_master(sha: str, repo: str | None) -> bool: def classify_base( - base: str, branches: dict[str, str], repo: str | None, open_heads: set[str] = frozenset() + base: str, branches: dict[str, str], repo: str, open_heads: set[str] = frozenset() ) -> tuple[str, str]: """`(verdict, why)` for one PR's base branch. @@ -228,15 +308,36 @@ def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( description="Report PRs whose base branch cannot reach master." ) - parser.add_argument("--repo", help="owner/name (default: the current repo)") + parser.add_argument( + "--repo", + help=( + "owner/name to measure (default: the repository gh resolves for this " + "checkout, refused when that repository is a fork)" + ), + ) parser.add_argument( "prs", nargs="*", type=int, help="only these PR numbers (default: all open)" ) args = parser.parse_args(argv) + # Resolved before anything is measured: a refusal that arrived after a verdict would + # still be a verdict, and the shape this file used to print was a verdict about an + # unnamed repository. + try: + repo = _repo_for_the_question(args.repo) + except (RuntimeError, json.JSONDecodeError) as exc: + print(f"cannot determine PR bases: {exc}", file=sys.stderr) + return 2 + + # Named before any verdict, on every run: `master` and the branch list are this + # repository's, so the reader has to be able to see whose they were. The line carries + # no other word on purpose - the tests below assert what the *verdict* lines say, and + # this one must not fall inside those assertions. + print(f"repo: {repo}") + try: - prs = _open_prs(args.repo) - branches = _branch_heads(args.repo) + prs = _open_prs(repo) + branches = _branch_heads(repo) except (RuntimeError, json.JSONDecodeError) as exc: print(f"cannot determine PR bases: {exc}", file=sys.stderr) return 2 @@ -274,7 +375,7 @@ def main(argv: list[str] | None = None) -> int: for pr in sorted(prs, key=lambda p: p["number"]): try: verdict, why = classify_base( - pr["baseRefName"], branches, args.repo, open_heads + pr["baseRefName"], branches, repo, open_heads ) except (RuntimeError, json.JSONDecodeError) as exc: print(f"#{pr['number']} UNKNOWN base={pr['baseRefName']!r}: {exc}") diff --git a/tests/test_check_pr_base.py b/tests/test_check_pr_base.py index 443aab4da..f5263afef 100644 --- a/tests/test_check_pr_base.py +++ b/tests/test_check_pr_base.py @@ -434,3 +434,176 @@ def test_the_two_halves_of_the_sentence_are_different_numbers( assert rc == 1, out assert "2 of the 3 named PR(s)" in out, out + + +class TestTheRepositoryItReadIsNamed: + """A verdict here is about exactly one repository, and this gate has to say which. + + Measured 2026-10-29 (`cyc20261029-082021`) on master `7e76668`, in a checkout whose + `origin` is a local directory and whose only GitHub remote is `fork = pm25coder/emrg`. + With no `--repo`, gh resolved the tool's `{owner}/{repo}` placeholder to the fork, and + the same commit asked the same question answered twice: + + _ref_is_on_master("7e766683...", None) -> False + _ref_is_on_master("7e766683...", "argszero/emrg") -> True + + because `master` was read in two repositories. The branch sets the verdicts are + classified against differ the same way - 50 against 1159 - so 1158 upstream bases came + back as "no longer exists on the remote", and the upstream master head itself read as + not on master. No line of that output named the repository that had answered, and with + an empty queue the exit code was a clean 0. + + Three directions are pinned, and each rules out one bad fix: a fork is refused **and + named** (so restoring the silent fallback reds), an ordinary checkout is still measured + **and named** (so "refuse whenever `--repo` is absent" cannot pass), and `--repo` + performs no resolution read at all (so a fix that keeps asking gh and only prints the + answer afterwards is not this fix). + """ + + FORK = { + "full_name": "pm25coder/emrg", + "fork": True, + "parent": {"full_name": "argszero/emrg"}, + } + ORDINARY = {"full_name": "someone/emrg", "fork": False, "parent": None} + + @staticmethod + def _dead_pr() -> list[dict]: + return [{"number": 1148, "baseRefName": "feature/dead", "headRefName": "h1"}] + + def _wire_resolution(self, mod, monkeypatch, resolution, calls=None): + def fake(*args): + if calls is not None: + calls.append(args) + return resolution + + monkeypatch.setattr(mod, "_gh_json", fake) + + def _wire_measurement(self, mod, monkeypatch, seen, prs=None): + def fake_open(repo): + seen.append(repo) + return self._dead_pr() if prs is None else prs + + monkeypatch.setattr(mod, "_open_prs", fake_open) + monkeypatch.setattr(mod, "_branch_heads", lambda repo: {"feature/dead": "s1"}) + monkeypatch.setattr(mod, "_ref_is_on_master", lambda sha, repo: False) + + def test_a_fork_is_refused_by_name(self, mod, monkeypatch, capsys) -> None: + """The measured shape: the checkout resolves to a fork, so nothing is measured.""" + seen: list[str] = [] + self._wire_resolution(mod, monkeypatch, self.FORK) + self._wire_measurement(mod, monkeypatch, seen) + + rc = mod.main([]) + err = capsys.readouterr().err + + assert rc == 2, "a fork is not a repository this question can be asked of" + assert seen == [], "the repository must be settled before anything is measured" + assert "pm25coder/emrg" in err, err + assert "argszero/emrg" in err, "the refusal must name the repository it is a fork of" + assert "--repo argszero/emrg" in err, "and it must name the remedy" + + def test_an_ordinary_checkout_is_measured_and_named( + self, mod, monkeypatch, capsys + ) -> None: + """The control: a non-fork resolution keeps the tool's ordinary invocation alive. + + Without this, a fix that refused whenever `--repo` is absent would pass the test + above while taking away the way every normal clone runs this gate. + """ + seen: list[str] = [] + self._wire_resolution(mod, monkeypatch, self.ORDINARY) + self._wire_measurement(mod, monkeypatch, seen) + + rc = mod.main([]) + out = capsys.readouterr().out + + assert rc == 1, out + assert seen == ["someone/emrg"], "the measurement must use the resolved repository" + assert out.splitlines()[0] == "repo: someone/emrg", out + + def test_a_named_repo_is_printed_before_any_verdict( + self, mod, monkeypatch, capsys + ) -> None: + """`--repo` is the caller naming the repository, so gh is never asked to guess.""" + seen: list[str] = [] + calls: list[tuple] = [] + self._wire_resolution(mod, monkeypatch, self.FORK, calls) + self._wire_measurement(mod, monkeypatch, seen) + + rc = mod.main(["--repo", "owner/repo"]) + out = capsys.readouterr().out + + assert rc == 1, out + assert calls == [], "--repo means no resolution read at all" + assert seen == ["owner/repo"], seen + assert out.splitlines()[0] == "repo: owner/repo", out + + def test_the_placeholder_is_a_lookup_and_never_a_path( + self, mod, monkeypatch, capsys + ) -> None: + """The defect itself, read off every argument the tool hands `gh`. + + The token `{owner}/{repo}` is what gh resolves from the cwd's remotes, so the fix + is not "print the repository afterwards" but "never let gh pick": the token may + appear exactly once, as the lookup this module performs itself, and never inside a + `repos/...` path a verdict then rests on. + """ + calls: list[tuple] = [] + + def fake_gh_json(*args): + calls.append(args) + if " ".join(args) == f"api repos/{mod.PLACEHOLDER}": + return self.ORDINARY + return {"status": "ahead"} # not on master: the DEAD verdict + + monkeypatch.setattr(mod, "_gh_json", fake_gh_json) + monkeypatch.setattr( + mod, + "_gh", + lambda *args: json.dumps([{"name": "feature/dead", "commit": {"sha": "s1"}}]), + ) + monkeypatch.setattr(mod, "_open_prs", lambda repo: self._dead_pr()) + + rc = mod.main([]) + assert rc == 1, capsys.readouterr() + + with_token = [a for a in calls if any(mod.PLACEHOLDER in part for part in a)] + assert with_token == [("api", f"repos/{mod.PLACEHOLDER}")], with_token + assert any( + "repos/someone/emrg/compare/master..." in part for a in calls for part in a + ), calls + + def test_a_resolution_that_names_no_repository_is_unmeasurable( + self, mod, monkeypatch, capsys + ) -> None: + """A resolution that answers nothing is not a repository to measure.""" + seen: list[str] = [] + self._wire_resolution(mod, monkeypatch, {"full_name": ""}) + self._wire_measurement(mod, monkeypatch, seen) + + rc = mod.main([]) + err = capsys.readouterr().err + + assert rc == 2, err + assert seen == [], seen + assert "did not name the repository" in err, err + + def test_an_empty_queue_still_names_the_repository( + self, mod, monkeypatch, capsys + ) -> None: + """Named even when there is nothing to classify. + + The repository line is not a header of the verdict table - it is the answer to + "whose queue was read", and an empty queue is exactly the case where a caller has + nothing else to go on. What the queue being empty *means* (this gate answers 0 + where its siblings answer 2) is a separate question and not pinned here. + """ + seen: list[str] = [] + self._wire_resolution(mod, monkeypatch, self.ORDINARY) + self._wire_measurement(mod, monkeypatch, seen, prs=[]) + + mod.main([]) + out = capsys.readouterr().out + + assert out.splitlines()[0] == "repo: someone/emrg", out