diff --git a/scripts/check-merge-pairs.py b/scripts/check-merge-pairs.py index 9553b46b..6a51f63c 100644 --- a/scripts/check-merge-pairs.py +++ b/scripts/check-merge-pairs.py @@ -92,7 +92,8 @@ land badly - that is why this is 0 here and 3 in `check-merge-sequence.py`, where a conflict leaves later steps of a chain unmeasured) 1 at least one ordered pair merges cleanly and lands a tree that fails the guards - 2 the question could not be answered (git/gh/guard failure) - fail loud; never + 2 the question could not be answered (git/gh/guard failure, or a requested PR + number whose head cannot be fetched) - fail loud; never report "no dangerous pair" about pairs that were not measured """ @@ -205,6 +206,16 @@ def main(argv: list[str] | None = None) -> int: # itself's twin and buy the same answer twice, and the ordering makes the output # diffable between runs. numbers = sorted(set(args.prs or seq._open_pr_numbers(args.repo))) + # A number that is not an open PR (closed, merged, or mistyped) fetches no + # head. It used to be taken verbatim: a lone number produces no pairs at all, + # so the loop below never ran, the summary line claimed "1 PR(s) -> 0 ordered + # pair(s)" about a PR that does not exist, and the run exited 0 - "no + # dangerous pair" about a PR that was never measured, which is the reading the + # exit-code contract above rules out. Resolving each head once, here, makes + # such a number fail loud as rc 2. The resolved heads are reused below, so + # this costs no extra fetches - and every pair is now measured against one + # snapshot of each head rather than a fresh fetch per pair. + heads = {n: seq._fetch_head(n) for n in numbers} except MeasurementError as exc: print(f"could not measure: {exc}", file=sys.stderr) return 2 @@ -229,12 +240,12 @@ def main(argv: list[str] | None = None) -> int: for a, b in pairs: try: if a not in first_step: - first_step[a] = seq._merge_commit(base, seq._fetch_head(a)) + first_step[a] = seq._merge_commit(base, heads[a]) landed_a = first_step[a] if landed_a is None: blocked += 1 continue - landed_b = seq._merge_commit(landed_a, seq._fetch_head(b)) + landed_b = seq._merge_commit(landed_a, heads[b]) if landed_b is None: blocked += 1 continue diff --git a/scripts/check-pr-base.py b/scripts/check-pr-base.py index d09495ef..6f159da7 100644 --- a/scripts/check-pr-base.py +++ b/scripts/check-pr-base.py @@ -59,7 +59,8 @@ 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) +2 the question could not be answered (gh failed, response unparseable, or a + requested PR number is not among the open PRs) Never reports a count it could not obtain: a check that guesses "OK" when it could not read the state is worse than no check, because the failure it hides is @@ -238,6 +239,25 @@ def main(argv: list[str] | None = None) -> int: if args.prs: wanted = set(args.prs) + # A requested number that is not among the open PRs used to be dropped in + # silence: the loop below then had nothing to classify, so the tool printed + # nothing and returned 0 - a clean bill for a PR it never looked at. That is + # the one thing its own contract forbids (never guess "OK" when the state + # could not be read), and it is indistinguishable from a pass at the exit + # code, which is what every caller reads. The number is answered as + # unmeasurable instead: this tool lists open PRs, so a closed, merged or + # mistyped number has no base it could classify. + missing = sorted(wanted - {p["number"] for p in prs}) + if missing: + print( + "cannot determine PR bases: " + + ", ".join(f"#{n}" for n in missing) + + " is not among the open PRs (closed, merged, or nonexistent) - " + "this tool classifies open PRs only, and will not report a clean bill " + "for one it did not look at", + file=sys.stderr, + ) + return 2 prs = [p for p in prs if p["number"] in wanted] dead = 0 diff --git a/tests/test_check_merge_pairs.py b/tests/test_check_merge_pairs.py index 813dfed0..ab8d314b 100644 --- a/tests/test_check_merge_pairs.py +++ b/tests/test_check_merge_pairs.py @@ -549,3 +549,69 @@ def fake_rev_parse(ref): assert rc == 2, err assert refreshed == [], "a base refused as a stray local branch must not be refreshed" + + +def test_a_requested_pr_whose_head_cannot_be_fetched_is_not_a_pass(mod, monkeypatch, capsys): + """A lone unmeasurable number must not become a reassurance about zero pairs. + + Measured before the fix (`cyc20260919-173431`): `check-merge-pairs.py 99999` printed + `pairs: 1 PR(s) -> 0 ordered pair(s)` and `no ordered pair merges cleanly into a + failing tree`, exit 0. The number was taken verbatim, a single PR forms no pair, the + loop never ran, and the summary line *became* the verdict - "no dangerous pair" about + a PR that does not exist. The exit-code contract above rules that reading out, and the + empty-pair case reaches it without a single `_fetch_head` call, which is why the head + is resolved for every requested number before any pair is formed. + + The existing tests are the other direction: explicit numbers that *do* resolve are + still measured, so this cannot be satisfied by refusing every explicit selection. + """ + + def missing(n): + raise mod.seq.MeasurementError(f"could not fetch PR #{n}") + + monkeypatch.setattr(mod.seq, "_rev_parse", lambda ref: BASE) + monkeypatch.setattr(mod.seq, "_refresh_base", lambda ref: None) + monkeypatch.setattr(mod.seq, "_fetch_head", missing) + monkeypatch.setattr(mod.seq, "_merge_commit", lambda a, b: pytest.fail("no pair to merge")) + rc = mod.main(["99999"]) + captured = capsys.readouterr() + + assert rc == 2, captured.out + assert "no ordered pair" not in captured.out, ( + "a verdict about unformed pairs is the fail-open shape this tool forbids" + ) + assert "99999" in captured.err, captured.err + + +def test_every_requested_head_is_resolved_once_before_any_pair(mod, monkeypatch, capsys): + """The heads are fetched once per run, not once per pair - and before the pairs. + + Two properties in one measurement, because they are the same edit: the up-front + resolution is what turns an unfetchable number into rc 2, and reusing its result is + what keeps that from costing a fetch per pair. Asserted by count, with three PRs so + each head is an element of more than one ordered pair. + """ + fetched: list[int] = [] + chain = { + (BASE, C1): C1, (BASE, C2): C2, (BASE, C3): C3, + (C1, C2): C2, (C1, C3): C3, (C2, C1): C1, (C2, C3): C3, (C3, C1): C1, (C3, C2): C2, + } + heads = {1: C1, 2: C2, 3: C3} + + def fetch(n): + fetched.append(n) + return heads[n] + + monkeypatch.setattr(mod.seq, "_rev_parse", lambda ref: BASE) + monkeypatch.setattr(mod.seq, "_refresh_base", lambda ref: None) + monkeypatch.setattr(mod.seq, "_fetch_head", fetch) + monkeypatch.setattr(mod.seq, "_merge_commit", lambda a, b: chain.get((a, b))) + monkeypatch.setattr(mod.seq, "_guard_verdict", lambda tree, workdir: (True, "documents 1564")) + monkeypatch.setattr( + mod.seq, "_run", lambda argv, cwd=None: _FakeProc(argv[-1].removesuffix("^{tree}")) + ) + rc = mod.main(["1", "2", "3"]) + capsys.readouterr() + + assert rc == 0, "three clean, healthy pairs" + assert sorted(fetched) == [1, 2, 3], f"each head once, not once per pair: {fetched}" diff --git a/tests/test_check_pr_base.py b/tests/test_check_pr_base.py index d55ac8b4..fe15056d 100644 --- a/tests/test_check_pr_base.py +++ b/tests/test_check_pr_base.py @@ -219,6 +219,40 @@ def test_selecting_one_pr_filters_the_rest(self, mod, monkeypatch, capsys) -> No assert "#2" in out and "#1" not in out + def test_a_requested_pr_that_is_not_open_exits_2(self, mod, monkeypatch, capsys) -> None: + """A number this tool never looked at must not come back as a clean bill. + + Measured before the fix (`cyc20260919-173431`): `check-pr-base.py 99999` printed + **nothing** and exited **0**. The requested number was filtered out of the + open-PR list, so the loop had nothing to classify and `dead` stayed 0 - at the + exit code, byte-identical to "every base reaches master", which is exactly what + a Committer reads before merging. The sibling gates all answer rc 2 for the same + input (`check-vote-count.py`, `check-merge-freshness.py`, `check-merge-landing-diff.py`, + `check-merge-order.py`, `check-merge-sequence.py`); this one was the outlier. + """ + prs = [{"number": 1151, "baseRefName": "master", "headRefName": "h1"}] + self._wire(mod, monkeypatch, prs, {}, lambda sha: True) + rc = mod.main(["--repo", "owner/repo", "99999"]) + err = capsys.readouterr().err + + assert rc == 2, "a PR that was never looked at is not a pass" + assert "#99999" in err and "not among the open PRs" in err, err + + def test_asking_about_an_open_pr_is_still_answered(self, mod, monkeypatch, capsys) -> None: + """The other direction: the refusal is about *membership*, not about selecting. + + Without this, a mutant that answered rc 2 for every explicit selection would pass + the test above while making `check-pr-base.py ` useless. + """ + prs = [{"number": 1151, "baseRefName": "master", "headRefName": "h1"}] + self._wire(mod, monkeypatch, prs, {}, lambda sha: True) + rc = mod.main(["--repo", "owner/repo", "1151"]) + out = capsys.readouterr().out + + assert rc == 0, out + assert "#1151" in out and "base is master" in out, out + + class TestRealInvocationSurface: """The script must be runnable as a tool, and must not silently pass on error."""