diff --git a/.github/scripts/check-milestone-landed.sh b/.github/scripts/check-milestone-landed.sh index ff732ed9..7391ff77 100755 --- a/.github/scripts/check-milestone-landed.sh +++ b/.github/scripts/check-milestone-landed.sh @@ -159,9 +159,24 @@ if [ "${MILESTONE_EXISTS}" = "0" ]; then " - set RECOTEM_ALLOW_NO_MILESTONE=1 to release without one deliberately." fi +# `--limit` is a cap, not a request: `gh pr list` returns at most that many rows +# and says nothing when there were more. Measured: `--limit 5` on milestone +# 2.1.0 returns 5 rows with an empty stderr, and `--limit 1100` against a +# repository with more merged PRs than that returns exactly 1000 -- the search +# API's own ceiling, also silent. So the previous `--limit 200` would have +# checked the 200 most recent PRs of a larger milestone and printed the same +# "OK: every merged PR ... is an ancestor" line about the rest, which is the +# vacuous-check shape this file's header refuses everywhere else. Milestone +# 2.1.0 carries 151 merged PRs today, so the old cap was 49 away from silently +# under-reporting. +# +# 1000 is chosen because it is where `gh pr list --search` truncates anyway, so +# the guard below fires exactly when truncation starts rather than at an +# arbitrary number of our own. +PR_LIMIT=1000 PRS="$( gh pr list --state merged --search "milestone:${MILESTONE}" \ - --limit 200 --json number,title,mergeCommit \ + --limit "${PR_LIMIT}" --json number,title,mergeCommit \ --jq '.[] | "\(.number)\t\(.mergeCommit.oid // "none")\t\(.title)"' )" @@ -170,6 +185,25 @@ if [ -z "${PRS}" ]; then exit 0 fi +# Refuse a full page rather than checking a prefix of the milestone. Placed +# before the ancestry loop: a truncated list cannot be made trustworthy by +# examining the part of it that arrived. +PR_COUNT="$(printf '%s\n' "${PRS}" | grep -c '.' || true)" +if [ "${PR_COUNT}" -ge "${PR_LIMIT}" ]; then + fail "milestone '${MILESTONE}' returned ${PR_COUNT} merged PRs, the maximum this query can return." \ + "The list is truncated, so the PRs beyond it were never checked -- and passing" \ + "here would print 'every merged PR in milestone ${MILESTONE} is an ancestor'" \ + "about a set nobody read. Refused rather than skipped, for the same reason as" \ + "the missing-milestone case above." \ + "" \ + "gh pr list --search cannot return more than ${PR_LIMIT} rows (the search API's" \ + "ceiling), so raising the number here does not help. To fix, verify by hand:" \ + " gh pr list --state merged --search 'milestone:${MILESTONE} created: HEAD" +fi + # --------------------------------------------------------------------------- # 4. Every merge commit must be an ancestor of HEAD # --------------------------------------------------------------------------- diff --git a/tests/unit/test_check_milestone_landed.py b/tests/unit/test_check_milestone_landed.py index aa30db1a..949c0e3a 100644 --- a/tests/unit/test_check_milestone_landed.py +++ b/tests/unit/test_check_milestone_landed.py @@ -408,3 +408,62 @@ def test_a_stale_record_for_a_healthy_pr_does_not_mask_it(repo: Path) -> None: assert "re-landed by" not in proc.stdout, ( "a reachable PR passed via the record rather than on its own ancestry" ) + + +# The number the script asks `gh pr list` for. Read from the script rather than +# restated, so raising it there without moving the guard fails here instead of +# leaving these two tests exercising a boundary that has moved. +_PR_LIMIT = int( + next( + line.split("=", 1)[1] + for line in SCRIPT.read_text(encoding="utf-8").splitlines() + if line.startswith("PR_LIMIT=") + ) +) + + +def _rows_with_no_merge_commit(count: int) -> list[str]: + """Rows the ancestry loop skips without spawning git. + + `none` takes the `UNKNOWN` branch, so a full page costs no subprocesses and + the two tests below measure the guard rather than the loop. + """ + return [f"{n}\tnone\tPR {n}" for n in range(1, count + 1)] + + +@requires_bash +def test_a_full_page_of_results_is_refused_not_checked_partially(repo: Path) -> None: + """`--limit` is a cap, and `gh` says nothing when it truncates. + + Measured against the real API: `gh pr list --search ... --limit 5` returns + five rows and an empty stderr, and `--limit 1100` against a repository with + more merged PRs than that returns exactly 1000. A milestone larger than the + cap would therefore have its most recent PRs checked and the rest reported + on -- the success line claims "every merged PR in milestone X", so passing + here would vouch for a set nobody read. Same shape as the absent-milestone + case, and refused the same way. + """ + _set_prs(repo, *_rows_with_no_merge_commit(_PR_LIMIT)) + proc = _run(repo) + combined = proc.stdout + proc.stderr + assert proc.returncode == 1, combined + assert "the maximum this query can return" in combined + assert "is an ancestor of" not in combined, ( + "the success line was printed about a truncated list" + ) + + +@requires_bash +def test_one_short_of_the_cap_is_still_checked(repo: Path) -> None: + """The boundary control: without it the test above passes for "many rows". + + A guard written with `-gt`, or placed after an off-by-one, would refuse a + complete list too -- blocking every large release for a truncation that did + not happen. This pins that the refusal starts exactly at the cap. + """ + _set_prs(repo, *_rows_with_no_merge_commit(_PR_LIMIT - 1)) + proc = _run(repo) + combined = proc.stdout + proc.stderr + assert proc.returncode == 0, combined + assert "the maximum this query can return" not in combined + assert f"Checked {_PR_LIMIT - 1} merged PR(s)" in proc.stdout