From 6594a73c4fb927360f81b571cda6be52448b2546 Mon Sep 17 00:00:00 2001 From: Geoff Hackett Date: Thu, 13 Aug 2026 22:45:24 -0400 Subject: [PATCH 1/2] The composer doesn't offer a verdict on your own pull request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub refuses a review of a pull request you opened yourself, so the PR page's Approve and Request changes buttons could only ever come back as an error there. They now sit out a PR the signed-in account authored, the same way they already sit out a merged or closed one; Comment — the half that is always yours to do — stays. Who "you" are comes from one `gh api user`, asked the first time a PR page loads and remembered for the run (prstatus.viewer_login), and the answer rides on the detail record as viewer_is_author. Unanswerable — no gh, offline, signed out, an author gh didn't name — reads as somebody else's PR, which leaves the page exactly as it was before anyone asked. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EfA4T1qTZ8MV7SiJW99FTf --- collins/prdetail.py | 43 +++++++++++++++++++++++------- collins/prstatus.py | 32 +++++++++++++++++++++++ collins/prview.py | 25 ++++++++++++------ tests/test_prdetail.py | 59 +++++++++++++++++++++++++++++++++++++++--- tests/test_prstatus.py | 36 ++++++++++++++++++++++++++ 5 files changed, 174 insertions(+), 21 deletions(-) diff --git a/collins/prdetail.py b/collins/prdetail.py index f49629de..63d6f381 100644 --- a/collins/prdetail.py +++ b/collins/prdetail.py @@ -8,7 +8,8 @@ timeline, checks, per-file diffs — fetched only when a view asks for it, never polled, and never persisted (a diff must not end up in state.json). -A load is three `gh` calls, all through prstatus's transport so the URL gate, +A load is three `gh` calls (four on the first load of a run, which also asks +who the signed-in user is), all through prstatus's transport so the URL gate, timeouts, argv-only policy and missing-gh latch stay in one place: one ``gh pr view --json`` with the full field list, one paginated ``gh api graphql`` for the review threads (the CLI's --json surface has no thread @@ -200,6 +201,13 @@ class PullRequestDetail: timeline: tuple[PrComment | PrReview | PrThread, ...] files: tuple[PrFile, ...] threads: tuple[PrThread, ...] = () + # Whether the account gh is signed in as is the one who opened this PR. + # What takes the review verdicts off the page (see prview's composer): + # GitHub refuses an approval of your own pull request, so a button + # offering one could only ever come back as an error. False whenever the + # question can't be answered — no gh, offline, an author gh didn't name — + # which leaves the page exactly as it was before anyone asked. + viewer_is_author: bool = False def fetch(url: str) -> PullRequestDetail | None: @@ -209,10 +217,11 @@ def fetch(url: str) -> PullRequestDetail | None: gh, offline — and the caller keeps showing what it has (stale beats blank here too). A failed or over-cap diff is *not* a failure (the files arrive stat-only, patches None), and neither is a failed thread fetch (the - conversation arrives threadless). The reply is folded into the summary - cache on the way through (`prstatus.absorb`), so the chip and mark update - with the view. Never call on the main thread — this waits on gh three - times. + conversation arrives threadless) or an unanswerable "who am I" (the PR + reads as somebody else's, which is what the page already assumed). The + reply is folded into the summary cache on the way through + (`prstatus.absorb`), so the chip and mark update with the view. Never call + on the main thread — this waits on gh three times. """ if prstatus.repository_for(url) is None: return None @@ -227,7 +236,11 @@ def fetch(url: str) -> PullRequestDetail | None: prstatus.absorb(url, data) threads = fetch_threads(url) diff = prstatus.gh_text(["pr", "diff", url], max_bytes=MAX_DIFF_BYTES) - return parse_detail(url, data, diff, threads) + # Who "you" are, so the page knows whether this PR is the user's own. Asked + # once for the whole run (prstatus.viewer_login caches it), so only the + # first load of a session pays for it, and "" — the unanswerable case — + # simply means no PR reads as authored here. + return parse_detail(url, data, diff, threads, viewer=prstatus.viewer_login()) def fetch_threads(url: str) -> tuple[PrThread, ...]: @@ -279,23 +292,30 @@ def fetch_threads(url: str) -> tuple[PrThread, ...]: def parse_detail( - url: str, data: dict, diff: str | None, threads: tuple[PrThread, ...] = () + url: str, + data: dict, + diff: str | None, + threads: tuple[PrThread, ...] = (), + viewer: str = "", ) -> PullRequestDetail | None: """One gh view reply (with its diff and threads, if any) as the record the view renders. Pure — module state is never touched — so recorded gh output drives it - straight in tests. None only when *url*/*data* can't even identify a PR - (`prstatus.summarize`'s answer). + straight in tests; *viewer* is the signed-in login the caller looked up + (`prstatus.viewer_login`), passed in rather than asked for here so this + stays a function of its arguments. None only when *url*/*data* can't even + identify a PR (`prstatus.summarize`'s answer). """ summary = prstatus.summarize(url, data) if summary is None: return None patches = dict(split_unified_diff(diff)) if diff else {} + author = _author(data.get("author")) return PullRequestDetail( summary=summary, body=_text(data.get("body")), - author=_author(data.get("author")), + author=author, created_at=_line(data.get("createdAt")), base_ref=_line(data.get("baseRefName")), head_ref=_line(data.get("headRefName")), @@ -307,6 +327,9 @@ def parse_detail( timeline=_timeline(data.get("comments"), data.get("reviews"), threads), files=_files(data.get("files"), patches), threads=threads, + # Logins are case-insensitive on GitHub, and gh spells one back the + # way the account was registered rather than the way it was asked for. + viewer_is_author=bool(author) and author.casefold() == viewer.casefold(), ) diff --git a/collins/prstatus.py b/collins/prstatus.py index 0d113430..9246dbc4 100644 --- a/collins/prstatus.py +++ b/collins/prstatus.py @@ -146,6 +146,9 @@ _SWEEP_WORKERS = 8 # How many PRs a row's tooltip spells out before it starts counting them. _MAX_TOOLTIP_PRS = 8 +# The longest a GitHub login can plausibly be: 39 characters for an account, +# plus room for the ``[bot]`` an app posts under. What `viewer_login` keeps. +_MAX_LOGIN = 50 # Only fetch for URLs shaped like a PR page. The URL comes out of a transcript # — repo content, i.e. untrusted — and lands in an argv, so this also keeps a @@ -168,6 +171,7 @@ _statuses: dict[str, tuple[float, dict | None]] = {} _inflight: set[str] = set() _gh_missing = False # gh isn't on PATH; nothing to retry against this run +_viewer = "" # the signed-in login, once asked for; "" until then (viewer_login) # What a PR's badge — the small status mark riding its base icon — can say. # Pure names rather than icon names: which icon and color each one gets is the @@ -783,6 +787,34 @@ def gh_text(args: list[str], max_bytes: int | None = None) -> str | None: return result.stdout +def viewer_login() -> str: + """The GitHub login gh is signed in as — "" when it can't be had. + + One ``gh api user`` the first time somebody asks, then that answer for the + rest of the run: the signed-in account doesn't change under a running app, + and the question is asked on every PR page load (whether the PR is the + user's own decides what the page offers to do about it — see + prdetail.PullRequestDetail.viewer_is_author). A failure isn't remembered: + no gh, offline or signed out is a state that gets better, so the next + caller asks again. Never call on the main thread. + """ + global _viewer + with _lock: + if _viewer: + return _viewer + if _gh_missing: + return "" + data = gh_json(["api", "user"]) + login = data.get("login") if isinstance(data, dict) else None + # GitHub's own logins are short; anything else isn't one, and this is the + # value every PR's author is compared against. + if not isinstance(login, str) or not login or len(login) > _MAX_LOGIN: + return "" + with _lock: + _viewer = login + return login + + def _entry(data: dict) -> dict: """A gh reply reduced to the CLI cache's `{state, checks}` shape, plus the title and mergeability — which that cache has no room for and the chips' diff --git a/collins/prview.py b/collins/prview.py index ae937d65..7cd72bc0 100644 --- a/collins/prview.py +++ b/collins/prview.py @@ -27,7 +27,8 @@ types the COMMENTS prompt into the owning session ("Address comments", while someone is waiting on a reply) or asks the repository's workflow for a review ("Request review") — the composer is for answering a reviewer yourself, the -button for making the agent do it. +button for making the agent do it. The verdicts sit out a pull request the +signed-in account opened, which GitHub won't let anyone review their own of. Review threads render as their own cards (`_ThreadCard`): anchored in the Conversation timeline by when they started, and again under their file's @@ -618,7 +619,7 @@ def _rebuild(self) -> None: empty = Gtk.Label(label=_("No comments yet."), xalign=0.0) empty.add_css_class("dim-label") self._content.append(empty) - self._composer.sync(self._pr) + self._composer.sync(self._pr, detail.viewer_is_author) self._content.append(self._composer) def _acted(self) -> None: @@ -1085,6 +1086,12 @@ class _Composer(Gtk.Box): stands alone. A failure comes back as gh's own sentence in a dialog, the text kept where it was typed; success clears the box and re-reads the PR. + The two verdicts are only there to be pressed on somebody else's pull + request: GitHub won't take a review of your own, so on a PR the signed-in + account opened (see prdetail's `viewer_is_author`) they aren't drawn at + all — a button whose only possible answer is a refusal is worse than no + button. Commenting is the half that is always yours to do. + The Claude button beside them is the complement, not a competitor, and which complement depends on who is waiting: "Address comments" while somebody's word is unanswered, typing the COMMENTS prompt into the owning @@ -1184,14 +1191,16 @@ def __init__( self.append(row) self._sync_buttons() - def sync(self, pr: PullRequest) -> None: + def sync(self, pr: PullRequest, viewer_is_author: bool = False) -> None: """Point the composer at *pr* as freshly fetched, and re-read the - session behind it. Verdicts only show for a live PR — GitHub refuses - a review on a merged or closed one, commenting stays open forever.""" + session behind it. Verdicts only show for a live PR that somebody else + opened — GitHub refuses a review on a merged or closed one, and + refuses your own pull request's approval whatever state it is in. + Commenting stays open in every case.""" self._pr = pr - live = pr.state in practions.LIVE - self._approve_btn.set_visible(live) - self._request_btn.set_visible(live) + verdicts = pr.state in practions.LIVE and not viewer_is_author + self._approve_btn.set_visible(verdicts) + self._request_btn.set_visible(verdicts) self._comment_btn.set_tooltip_text(_("Comment on {slug}").format(slug=pr.slug)) self._approve_btn.set_tooltip_text(_("Approve {slug}").format(slug=pr.slug)) self._request_btn.set_tooltip_text( diff --git a/tests/test_prdetail.py b/tests/test_prdetail.py index 837b130f..601f219c 100644 --- a/tests/test_prdetail.py +++ b/tests/test_prdetail.py @@ -25,12 +25,15 @@ @pytest.fixture(autouse=True) def clean_status_cache(): - """fetch() absorbs into prstatus's module cache; keep tests independent.""" + """fetch() absorbs into prstatus's module cache and remembers the signed-in + login there for the run; keep tests independent of both.""" prstatus._statuses.clear() prstatus._inflight.clear() + prstatus._viewer = "" yield prstatus._statuses.clear() prstatus._inflight.clear() + prstatus._viewer = "" # Shaped like real `gh pr view --json` output (recorded off episode6/collins @@ -150,6 +153,25 @@ def test_the_header_fields_arrive_whole(): assert detail.labels == ("enhancement",) +def test_a_pr_is_the_viewers_own_when_the_logins_match(): + """What takes the review verdicts off the page: GitHub refuses a review of + your own pull request, and logins compare case-insensitively — gh spells + an author back the way the account was registered.""" + assert parse_detail(URL, _reply(), DIFF, viewer="ghackett").viewer_is_author + assert parse_detail(URL, _reply(), DIFF, viewer="GHackett").viewer_is_author + assert not parse_detail(URL, _reply(), DIFF, viewer="someone-else").viewer_is_author + + +def test_an_unanswerable_author_reads_as_somebody_elses(): + """No gh, offline, signed out, or a reply that never named an author: all + of them leave the page as it was before anyone asked.""" + assert not parse_detail(URL, _reply(), DIFF).viewer_is_author # no viewer + assert not parse_detail(URL, _reply(author=None), DIFF, viewer="").viewer_is_author + assert not parse_detail( + URL, _reply(author=None), DIFF, viewer="ghackett" + ).viewer_is_author + + def test_checks_cover_both_of_ghs_shapes(): checks = parse_detail(URL, _reply(), DIFF).checks assert [(c.name, c.state) for c in checks] == [ @@ -588,6 +610,36 @@ def gh_json(args, cwd=None, timeout=None): assert any(isinstance(entry, PrThread) for entry in detail.timeline) +def test_fetch_asks_who_is_signed_in_and_hands_it_to_the_detail(monkeypatch): + """The fourth call, and the only one whose answer outlives the load: the + login is remembered for the run, so a second page doesn't ask again.""" + calls = [] + + def gh_json(args, cwd=None, timeout=None): + calls.append(args) + if args[:2] == ["api", "user"]: + return {"login": "ghackett"} + return None if args[0] == "api" else _reply() + + monkeypatch.setattr(prstatus, "gh_json", gh_json) + monkeypatch.setattr(prstatus, "gh_text", lambda args, max_bytes=None: DIFF) + assert fetch(URL).viewer_is_author is True + assert ["api", "user"] in calls + calls.clear() + assert fetch(URL).viewer_is_author is True + assert ["api", "user"] not in calls + + +def test_fetch_survives_not_knowing_who_is_signed_in(gh): + """The gh fixture answers nothing to every `api` call, this one included: + the load lands whole, the PR simply reads as somebody else's.""" + set_view, _set_diff, _calls = gh + set_view(_reply()) + detail = fetch(URL) + assert detail is not None + assert detail.viewer_is_author is False + + def test_fetch_absorbs_into_the_summary_cache(gh): """Opening the view updates the chip and mark for free: the reply lands in prstatus as a fetch of our own.""" @@ -625,7 +677,8 @@ def test_fetch_survives_a_dead_or_oversized_diff(gh): def test_fetch_runs_every_call_on_the_action_budget(monkeypatch): """All the way down to subprocess.run: a load someone is waiting on gets the action timeout for the heavy view reply, the thread query and the - diff alike, not the poll's short one.""" + diff alike, not the poll's short one. The who-am-I call rides along on the + poll budget — it is one line of reply, and a load survives losing it.""" seen = [] def run(argv, **kwargs): @@ -636,5 +689,5 @@ def run(argv, **kwargs): monkeypatch.setattr(prstatus.shutil, "which", lambda _: "/usr/bin/gh") monkeypatch.setattr(prstatus.subprocess, "run", run) assert fetch(URL) is not None - assert [kwargs["timeout"] for _argv, kwargs in seen] \ + assert [kwargs["timeout"] for _argv, kwargs in seen][:3] \ == [prstatus._GH_ACTION_TIMEOUT_S] * 3 diff --git a/tests/test_prstatus.py b/tests/test_prstatus.py index 0faa2fa6..680cc8ed 100644 --- a/tests/test_prstatus.py +++ b/tests/test_prstatus.py @@ -95,6 +95,7 @@ def scheduled(monkeypatch): urls: list[str] = [] monkeypatch.setattr(prstatus, "_schedule", urls.append) monkeypatch.setattr(prstatus, "_gh_missing", False) + monkeypatch.setattr(prstatus, "_viewer", "") # the run's remembered login prstatus._statuses.clear() prstatus._inflight.clear() yield urls @@ -796,6 +797,41 @@ def test_gh_run_feeds_stdin_rather_than_argv(monkeypatch): assert "A comment body.\n" not in argv +# -- who is signed in (what tells a PR page it is looking at your own PR) ----- + + +def test_viewer_login_is_asked_once_and_kept(monkeypatch): + calls = [] + monkeypatch.setattr( + prstatus, "gh_json", + lambda args, cwd=None, timeout=None: calls.append(args) or {"login": "ghackett"}, + ) + assert prstatus.viewer_login() == "ghackett" + assert prstatus.viewer_login() == "ghackett" + assert calls == [["api", "user"]] # the second answer came out of the run's memory + + +def test_viewer_login_doesnt_remember_a_failure(monkeypatch): + """Offline, signed out, or a reply that isn't a user: all of them are + states that get better, so the next caller asks again.""" + replies = [None, {"login": ""}, {"login": 7}, {"login": "x" * 200}, {"login": "gh"}] + monkeypatch.setattr( + prstatus, "gh_json", lambda args, cwd=None, timeout=None: replies.pop(0) + ) + assert [prstatus.viewer_login() for _ in range(4)] == ["", "", "", ""] + assert prstatus.viewer_login() == "gh" + + +def test_viewer_login_stays_off_a_machine_without_gh(monkeypatch): + calls = [] + monkeypatch.setattr(prstatus, "_gh_missing", True) + monkeypatch.setattr( + prstatus, "gh_json", lambda args, cwd=None, timeout=None: calls.append(args) + ) + assert prstatus.viewer_login() == "" + assert calls == [] + + # -- absorbing a detail fetch ------------------------------------------------- # Shaped like the detail view's `gh pr view --json` reply (prdetail): a strict From e4387c343c18d5fb16421ef6d388bfcc33267ee1 Mon Sep 17 00:00:00 2001 From: Geoff Hackett Date: Fri, 14 Aug 2026 06:41:04 -0400 Subject: [PATCH 2/2] Say why viewer_login's lock doesn't cover the gh call A review asked after the two cold loads that can both reach `gh api user`. They can, and the duplicate is the cheaper half of the trade: the alternative parks every thread wanting a status behind one network round trip. Written down so the next reader doesn't have to derive it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EfA4T1qTZ8MV7SiJW99FTf --- collins/prstatus.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/collins/prstatus.py b/collins/prstatus.py index 9246dbc4..93ec72eb 100644 --- a/collins/prstatus.py +++ b/collins/prstatus.py @@ -797,6 +797,12 @@ def viewer_login() -> str: prdetail.PullRequestDetail.viewer_is_author). A failure isn't remembered: no gh, offline or signed out is a state that gets better, so the next caller asks again. Never call on the main thread. + + The lock is held around the answer, never around the call: two pages + loading cold at the same moment may both ask gh, which costs one spare + subprocess and writes the same login twice. Holding it across a + subprocess to save that would park every other thread that wants a + status behind a network round trip — the worse of the two. """ global _viewer with _lock: