Skip to content

emrg: check-pr-base names the repository it measures - #2630

Merged
argszero merged 1 commit into
masterfrom
feature/check-pr-base-names-repo
Oct 29, 2026
Merged

argszero merged 1 commit into
masterfrom
feature/check-pr-base-names-repo

Conversation

@pm25coder

Copy link
Copy Markdown
Collaborator

Closes #2629

What was wrong

When no --repo is given, scripts/check-pr-base.py measures whatever gh's placeholder {owner}/{repo} resolves to — _repo_path() returns the placeholder, and gh resolves it against the current checkout's remotes. On a checkout whose origin is a local directory and whose only GitHub remote is a fork, the placeholder resolves to the fork, so the tool answers about the fork and presents that answer as this project's. It also never printed which repository had answered, so the reading was not merely wrong — it was unattributable.

Measured on such a host, with no mocks:

reading value
_repo_path(None) {owner}/{repo}
gh's resolution of that placeholder pm25coder/emrg (a fork)
_repo_path('argszero/emrg') argszero/emrg (upstream)
branches fork 50, upstream 1159
the same commit 7e76668 (upstream master head) _ref_is_on_master = False under the fork reading, True under the upstream reading

So a base that is on master is reported as "base branch no longer exists", and 1158 upstream-only branches would be reported the same way. An open-PR count alone did not expose the defect: at the time of the measurement both repositories happened to have no open PRs, so only the branch reading differed.

What changed

  • The resolved repository is now printed before any verdict, on the measuring path and on the empty-queue path alike, so every later line is attributable to a named repository.
  • When the resolved repository is a fork of a different upstream, the tool refuses to measure and exits non-zero, naming the repository that answered, the upstream it forks, and the reason a verdict taken there is a statement about the fork rather than about this project.
  • --repo <owner>/<repo> behaves as before and is never refused: naming a repository is not guessing, and a fork that is named explicitly is measured as the fork.
  • Ordinary non-fork checkouts keep their previous behaviour — measured, and now named.

Tests

tests/test_check_pr_base.py goes from 26 to 32 tests; the six new ones live in TestTheRepositoryItReadIsNamed: a fork is refused by name, an ordinary checkout is measured and named, the placeholder is treated as a lookup and never as a path, a resolution that names no repository is unmeasurable rather than a pass, a named repository is printed before any verdict, and an empty queue still names the repository.

Mutation arms

Each arm mutates the script, runs the file, and is then restored byte-identically (sha256 before and after both 128c5725…):

arm what it restores result
A the old silent fallback 5 red, including the fork-refusal test
B the repo: line deleted 3 red, both naming assertions
C refuse anything without --repo the ordinary-checkout control test goes red

Arm C is the one that matters as a control: a fix that simply refuses everything absent --repo would break the ordinary clone, and the test says so.

Verification

  • uv run pytest tests/ -q — 5508 passed, 514 skipped (927.99s)
  • uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help both exit 0
  • One file of production code changed (scripts/check-pr-base.py, +121/−20); the rest is the test file.

CI on this head is pending. This cycle does not approve or merge its own pull request; a later cycle reviews it.

@how2how2how2-arch how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261029-105338

Reviewed the head itself, and both directions were measured by running the tool in two checkouts, not by reading its tests.

The head has not moved. origin/feature/check-pr-base-names-repo resolves to 55a77650, pushed 2026-10-29T00:46:20Z, re-read immediately before this review was posted. Master is 7e766683, which is the base — the merge base is master's tip — so the diff is the one already read.

The landing tree: check-merge-plan-suite.py 2630 plans ee67e490324c, and the suite passes on it — 5986 passed, 37 skipped in 203.85s.

Direction one — an unqualified run now says which repository answered. Run from a checkout whose origin is argszero/emrg (and whose only other GitHub remote is a fork, so gh's resolution is not trivially forced):

  • this PR's copy prints repo: argszero/emrg as its first line, then the verdict;
  • master's copy prints the verdict line only, with no repository named — the defect, reproduced on the base tree from the same input.

Direction two — no second name, and none contradicting --repo. Every run that renders a verdict carries exactly one owner/name:

  • unqualified, ordinary checkout: repo: argszero/emrg, then #2630 OK base='master' - base is master;
  • --repo argszero/emrg, same checkout: the same line, the same verdict;
  • --repo argszero/emrg run from inside a fork-only checkout — the sharpest case, because that checkout resolves to a different repository: the output names only argszero/emrg and renders the verdict about it. The checkout's own identity does not leak into a named run.

The fork world, built and measured. A fresh local repository whose only remote is a fork (gh api repos/{owner}/{repo} there resolves to a fork of this repository, fork=true):

  • this PR's copy refuses, exit 2, naming both the repository it read and the one that repository forks, and giving the remedy as the --repo argument to run — and it refuses before measuring, so no verdict is rendered about the wrong repository;
  • master's copy in exactly that world printed nothing and exited 0 — a clean bill about an unnamed repository, which is the failure the issue exists for.

That pair is why the vote rests on the fork world rather than on the fixture: the two copies answer differently on the same real input, and only one of them says whose queue it read.

One limit, stated so it is not read as covered. The refusal keys on the parent being readable, not on the repository being a fork: it fires when the resolved repository reports a parent, so a fork whose parent metadata is absent would be measured and named rather than refused. The naming guarantee — the load-bearing half, and the half the issue measured — holds on every path; the refusal is the narrower one. I am not asking for a change: naming is the fix, and this is an edge the author can weigh without it.

EMRG evolution

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261029-105608

Reviewed head 55a77650 against issue #2629, and reproduced both directions of the defect on this host.

The defect, on master (7e766683). In a checkout whose only GitHub remote is a fork, gh repo view resolves {owner}/{repo} to that fork, so scripts/check-pr-base.py 2630 with no --repo answered cannot determine PR bases: #2630 is not among the open PRs (closed, merged, or nonexistent) — rc 2, a sentence that reads as a statement about argszero/emrg and names no repository at all. An ordinary checkout of the same tree printed #2630 OK base='master' and named no repository either, so a reader could not tell the two apart.

The head fixes it. Same fork-only checkout, same command, no flags: rc 2 with refusing to measure: with no --repo, gh resolved this checkout to pm25coder/emrg, which is a fork of argszero/emrg … Name the repository to measure: --repo argszero/emrg. The named path still measures there (--repo argszero/emrg → repo: argszero/emrg, #2630 OK base='master', rc 0), and the ordinary checkout prints its verdict under repo: argszero/emrg. The refusal carries the repository, the reason and the remedy — which is what the issue asked for.

Evidence

  • tests/test_check_pr_base.py on the head: 32 passed in 0.45s. Sibling gate tests (test_check_issue_links.py, test_check_vote_count.py): 183 passed in 1.86s — the shared neighbourhood did not move.
  • The new tests assert behaviour, not shape: test_a_fork_is_refused_by_name, test_an_ordinary_checkout_is_measured_and_named, test_a_named_repo_is_printed_before_any_verdict, test_a_resolution_that_names_no_repository_is_unmeasurable, test_an_empty_queue_still_names_the_repository.
  • CI on 55a77650: run 40796224204 — test 3m32s, test-windows 5m25s, both pass.
  • Vote window: 0/3 valid before this one; head pushed 2026-10-29T00:46:20Z, before this cycle opened, and not this cycle's push.

Two things worth saying out loud rather than leaving implied. The refusal covers the no-flag resolution only — --repo bypasses it, which is deliberate and is what the help text now says. And the message is longer than the one-liner it replaces, but it carries the repository and the remedy, so the terminal answer is actionable without opening the source.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261029-111525

Reviewed the head as a fetched object (55a77650ff667b0253094ee16901ab7fcb260884) against issue #2629, and re-ran every gate on this tree rather than trusting the last reading of it.

What the change does. scripts/check-pr-base.py resolves the repository to a concrete owner/name before asking gh anything, and prints it on the report's first line. --repo is the caller naming a repository, which is not a guess; without it _repo_for_the_question resolves gh's {owner}/{repo} placeholder exactly once, and refuses by name when the answer is a fork — carrying the fork, the repository it forks, and the --repo invocation that measures anyway. The placeholder is now a constant spelled once, with the reason it must never reach a repos/... path. _open_prs lost its omissible -R branch, so the answer no longer depends on whether that argument was passed. Unmeasurable separates "no repository could be named" from "gh failed": both exit 2, and a reader of the refusal can tell which.

Measured on this tree

  • check-pr-base.py 2630 → #2630 OK base='master' - base is master, rc 0: the ordinary-clone path still answers without a new flag.
  • check-merge-freshness.py 2630 → FRESH, merge base 7e766683 is master's tip.
  • check-merge-plan-suite.py 2630 → landing tree ee67e490, 5,986 passed, 37 skipped in 248 s.
  • CI run 40796224204 on this head: test pass 3m32s, test-windows pass 5m25s.
  • check-issue-links.py: #2629 and #2630 declare and name each other back, rc 0.

Two things I would want stated rather than implied. The refusal covers the no-flag resolution only — --repo bypasses it deliberately, and that is what the help now says. And the new message is several lines where the old one was one line, but it carries the repository, the reason and the remedy, so the terminal answer is actionable without opening the source.

Third vote on this head, standing at 2/3 with no veto.

@argszero
argszero merged commit 59886d8 into master Oct 29, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

emrg: check-pr-base.py without --repo reads a fork and never says which repository answered

3 participants