From 71a23f83a5f55fa59e22668ede2ead7eb1bf8be0 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:21:55 -0400 Subject: [PATCH 1/3] Fix release docs reviewer routing Use GitHub's review-request result as the repository-access check and route rejected docs-facing contributors to the human mergers of their specific upstream PRs. Keep owner and noisy fallback behavior, avoid contributor pings, and cover the routing cases with tests. --- .claude/skills/upstream-release-docs/SKILL.md | 6 +- .../scripts/assign_release_docs_reviewers.py | 384 ++++++++++++++++++ .../test_assign_release_docs_reviewers.py | 209 ++++++++++ .github/workflows/_static-checks.yaml | 3 + .github/workflows/upstream-release-docs.yml | 226 +++-------- .gitignore | 1 + 6 files changed, 659 insertions(+), 170 deletions(-) create mode 100644 .github/scripts/assign_release_docs_reviewers.py create mode 100644 .github/scripts/test_assign_release_docs_reviewers.py diff --git a/.claude/skills/upstream-release-docs/SKILL.md b/.claude/skills/upstream-release-docs/SKILL.md index b57d37b9..269f273f 100644 --- a/.claude/skills/upstream-release-docs/SKILL.md +++ b/.claude/skills/upstream-release-docs/SKILL.md @@ -106,7 +106,7 @@ Read `.release-meta.json` first (the caller writes it before invoking you): "contributors": ["alice", "bob", "carol"], "commits": [ { - "sha": "8343851e", + "sha": "8343851e9f06c0d67e315eb6aa4e9371d6ef76cf", "subject": "Push skills unsigned until keyless signing lands (#6334)", "author": "alice" } @@ -126,6 +126,7 @@ Classify **every** login in `contributors` as docs-facing or not, and write `REV { "login": "alice", "docs_facing": true, + "docs_facing_shas": ["8343851e9f06c0d67e315eb6aa4e9371d6ef76cf"], "note": "Confirm the ai-plugin timeout flag section matches what you shipped." }, { @@ -138,9 +139,10 @@ Classify **every** login in `contributors` as docs-facing or not, and write `REV ``` - `docs_facing: true` means at least one of this person's commits in the release range changed something a reader of the docs can observe: a CLI flag or subcommand, a CRD or config field, an API route, a default, an error message, a user-visible behavior, or anything you documented or corrected in this run. When you are unsure, classify as `true`. A needless review request is a minor annoyance; a missing one means a wrong page ships. +- `docs_facing_shas` (docs-facing only): every full commit SHA from `commits` that made the contributor docs-facing. If GitHub cannot request the contributor as a reviewer, the workflow uses all of these SHAs to find the human mergers of the relevant upstream PRs. Include only commits you verified as reader-visible; do not list an unrelated commit merely because the same contributor authored it. - `docs_facing: false` is for changes with no reader-visible surface: CI and build plumbing, dependency bumps, tests and fixtures, internal refactors, lint fixes, comment-only edits. **Base this on the actual diff you read in Phase 2, not on the commit message.** A commit titled "refactor" that changes a default value is docs-facing. - `note` (docs-facing only): one short sentence naming the specific thing that person should check, in their terms. Not a summary of the release, and not a restatement of their PR title. Skip the note rather than pad it. -- `reason` (non-docs-facing only): a short phrase naming what their changes actually were. This is shown to them as the justification for not requesting their review, so it has to be specific enough that they can tell whether you got it wrong. +- `reason` (non-docs-facing only): a short phrase naming what their changes actually were. This keeps the classification auditable when the run artifact is inspected; the workflow counts these contributors without rendering their names or reasons in the PR body. - Include the owner in the list with an honest classification. The workflow requests a review from the owner either way, so classifying the owner `false` costs nothing and keeps the classification truthful. - Bot logins are already filtered out of `contributors`; if one appears anyway, omit it. diff --git a/.github/scripts/assign_release_docs_reviewers.py b/.github/scripts/assign_release_docs_reviewers.py new file mode 100644 index 00000000..8a54ed63 --- /dev/null +++ b/.github/scripts/assign_release_docs_reviewers.py @@ -0,0 +1,384 @@ +#!/usr/bin/env python3 +"""Assign reviewers for an upstream-release documentation pull request. + +GitHub itself is the reviewer-access authority: request the upstream +contributor directly, then fall back to the human who merged each relevant +upstream pull request when GitHub rejects that request. This deliberately +avoids organization-membership and collaborator lookups, which require +broader credentials or behave inconsistently for team-derived access. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +from dataclasses import dataclass, field +from pathlib import Path + + +BOT_LOGIN = re.compile( + r"(\[bot\]$|^app/|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)", + re.IGNORECASE, +) + + +def warning(message: str) -> None: + print(f"::warning::{message}") + + +def unique(values: list[str]) -> list[str]: + return list(dict.fromkeys(value for value in values if value)) + + +@dataclass(frozen=True) +class Config: + review_repo: str + release_repo: str + pr_number: str + owner: str + compare_ok: str + candidates: list[str] + github_output: Path + + @classmethod + def from_env(cls) -> "Config": + output = os.environ.get("GITHUB_OUTPUT") + if not output: + raise RuntimeError("GITHUB_OUTPUT is required") + return cls( + review_repo=os.environ["REVIEW_REPO"], + release_repo=os.environ["RELEASE_REPO"], + pr_number=os.environ["PR_NUMBER"], + owner=os.environ.get("OWNER", ""), + compare_ok=os.environ.get("COMPARE_OK", ""), + candidates=unique(os.environ.get("CANDIDATES", "").splitlines()), + github_output=Path(output), + ) + + +@dataclass +class Selection: + docs_facing: dict[str, list[str]] + non_docs_facing: list[str] + classified: bool + + +@dataclass +class AssignmentResult: + owner_assigned: bool = False + assigned: list[str] = field(default_factory=list) + fyi_count: int = 0 + standin_notes: list[str] = field(default_factory=list) + unresolved_notes: list[str] = field(default_factory=list) + + +class GitHub: + def __init__(self, config: Config): + self.config = config + + @staticmethod + def _run(*args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["gh", *args], + check=False, + capture_output=True, + text=True, + ) + + def assign_owner(self, login: str) -> bool: + result = self._run( + "pr", + "edit", + self.config.pr_number, + "--repo", + self.config.review_repo, + "--add-assignee", + login, + ) + return result.returncode == 0 + + def request_review(self, login: str) -> bool: + result = self._run( + "api", + "--method", + "POST", + ( + f"repos/{self.config.review_repo}/pulls/" + f"{self.config.pr_number}/requested_reviewers" + ), + "-f", + f"reviewers[]={login}", + ) + return result.returncode == 0 + + def merged_prs_for_commit(self, sha: str) -> list[int]: + result = self._run( + "api", + f"repos/{self.config.release_repo}/commits/{sha}/pulls", + ) + if result.returncode != 0: + return [] + try: + pulls = json.loads(result.stdout) + except json.JSONDecodeError: + return [] + return unique_ints( + [ + pull.get("number") + for pull in pulls + if pull.get("merged_at") and isinstance(pull.get("number"), int) + ] + ) + + def merger_for_pr(self, pr_number: int) -> str: + result = self._run( + "pr", + "view", + str(pr_number), + "--repo", + self.config.release_repo, + "--json", + "mergedBy", + ) + if result.returncode != 0: + return "" + try: + merged_by = json.loads(result.stdout).get("mergedBy") or {} + except json.JSONDecodeError: + return "" + return merged_by.get("login") or "" + + +def unique_ints(values: list[int | None]) -> list[int]: + return list(dict.fromkeys(value for value in values if value is not None)) + + +def read_json(path: Path) -> dict | None: + try: + value = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError): + return None + return value if isinstance(value, dict) else None + + +def commits_by_author(release_meta: dict | None) -> dict[str, list[str]]: + result: dict[str, list[str]] = {} + for commit in (release_meta or {}).get("commits", []): + if not isinstance(commit, dict): + continue + login = commit.get("author") + sha = commit.get("sha") + if isinstance(login, str) and isinstance(sha, str): + result.setdefault(login, []).append(sha) + return {login: unique(shas) for login, shas in result.items()} + + +def fallback_selection( + candidates: list[str], commits: dict[str, list[str]], reason: str +) -> Selection: + warning(f"{reason}; falling back to requesting all contributors.") + return Selection( + docs_facing={login: commits.get(login, []) for login in candidates}, + non_docs_facing=[], + classified=False, + ) + + +def select_contributors( + candidates: list[str], reviewers: dict | None, release_meta: dict | None +) -> Selection: + commits = commits_by_author(release_meta) + if reviewers is None: + return fallback_selection(candidates, commits, "No usable REVIEWERS.json") + + entries = reviewers.get("contributors") + if not isinstance(entries, list) or not entries: + return fallback_selection( + candidates, commits, "REVIEWERS.json classified no contributors" + ) + + by_login: dict[str, dict] = {} + for entry in entries: + if not isinstance(entry, dict): + continue + login = entry.get("login") + docs_facing = entry.get("docs_facing") + if isinstance(login, str) and isinstance(docs_facing, bool): + by_login[login] = entry + + missing = [login for login in candidates if login not in by_login] + if missing: + return fallback_selection( + candidates, + commits, + "REVIEWERS.json omitted release contributors: " + ", ".join(missing), + ) + + docs_facing: dict[str, list[str]] = {} + non_docs_facing: list[str] = [] + for login in candidates: + entry = by_login[login] + if not entry["docs_facing"]: + non_docs_facing.append(login) + continue + + shas = entry.get("docs_facing_shas") + if not isinstance(shas, list) or not shas or not all( + isinstance(sha, str) for sha in shas + ): + return fallback_selection( + candidates, + commits, + f"REVIEWERS.json omitted docs_facing_shas for {login}", + ) + + authored_shas = set(commits.get(login, [])) + unknown = [sha for sha in shas if sha not in authored_shas] + if unknown: + return fallback_selection( + candidates, + commits, + f"REVIEWERS.json listed unknown commit SHAs for {login}", + ) + docs_facing[login] = unique(shas) + + return Selection(docs_facing, non_docs_facing, True) + + +def is_human_standin(login: str, contributor: str) -> bool: + return bool(login) and login != contributor and not BOT_LOGIN.search(login) + + +def assign_reviewers( + config: Config, selection: Selection, github: GitHub +) -> AssignmentResult: + result = AssignmentResult() + requested: set[str] = set() + + def request_once(login: str) -> bool: + if login in requested: + return True + if github.request_review(login): + requested.add(login) + result.assigned.append(login) + print(f"Review requested: {login}") + return True + return False + + if config.owner: + result.owner_assigned = github.assign_owner(config.owner) + if result.owner_assigned: + print(f"Assigned owner: {config.owner}") + else: + warning(f"Could not assign owner {config.owner} as assignee.") + + if not request_once(config.owner): + warning(f"Could not request a review from owner {config.owner}.") + result.unresolved_notes.append( + f"Release owner `{config.owner}` could not be requested as a reviewer. " + "A docs maintainer must route this review." + ) + else: + warning("No release owner resolved; PR has no assignee or owner review.") + + result.fyi_count = len( + [login for login in selection.non_docs_facing if login != config.owner] + ) + + for contributor, shas in selection.docs_facing.items(): + if contributor == config.owner: + continue + if request_once(contributor): + continue + + print( + f"Direct review request rejected for {contributor}; " + "resolving upstream merger stand-ins." + ) + seen_prs: set[int] = set() + if not shas: + result.unresolved_notes.append( + f"Docs-facing contributor `{contributor}` has no commit available for " + "stand-in resolution. The release owner must route this review." + ) + continue + + for sha in shas: + pull_numbers = github.merged_prs_for_commit(sha) + if not pull_numbers: + result.unresolved_notes.append( + f"Docs-facing commit `{sha[:12]}` could not be mapped to a merged " + "upstream PR. The release owner must route this review." + ) + continue + + for pull_number in pull_numbers: + if pull_number in seen_prs: + continue + seen_prs.add(pull_number) + merger = github.merger_for_pr(pull_number) + upstream_pr = f"{config.release_repo}#{pull_number}" + if not is_human_standin(merger, contributor): + result.unresolved_notes.append( + f"Upstream PR `{upstream_pr}` has no human merger available as " + "a stand-in. The release owner must route this review." + ) + continue + if not request_once(merger): + result.unresolved_notes.append( + f"Upstream merger `{merger}` could not be requested for " + f"`{upstream_pr}`. The release owner must route this review." + ) + continue + result.standin_notes.append(f"@{merger} (merged {upstream_pr})") + print(f"Stand-in review requested: {merger} for {upstream_pr}") + + result.assigned = unique(result.assigned) + result.standin_notes = unique(result.standin_notes) + result.unresolved_notes = unique(result.unresolved_notes) + return result + + +def write_multiline(output, name: str, lines: list[str]) -> None: + marker = f"{name.upper()}_EOF" + output.write(f"{name}<<{marker}\n") + if lines: + output.write("\n".join(lines) + "\n") + output.write(f"{marker}\n") + + +def write_outputs( + config: Config, selection: Selection, result: AssignmentResult +) -> None: + with config.github_output.open("a") as output: + output.write(f"compare_ok={config.compare_ok}\n") + output.write(f"owner={config.owner}\n") + output.write(f"owner_assigned={str(result.owner_assigned).lower()}\n") + output.write(f"list={','.join(result.assigned)}\n") + output.write(f"fyi_count={result.fyi_count}\n") + output.write(f"classified={str(selection.classified).lower()}\n") + output.write(f"unresolved_count={len(result.unresolved_notes)}\n") + write_multiline(output, "standin_block", result.standin_notes) + write_multiline(output, "unresolved_block", result.unresolved_notes) + + +def main() -> int: + config = Config.from_env() + reviewers = read_json(Path("REVIEWERS.json")) + release_meta = read_json(Path(".release-meta.json")) + selection = select_contributors(config.candidates, reviewers, release_meta) + result = assign_reviewers(config, selection, GitHub(config)) + write_outputs(config, selection, result) + print(f"Owner: {config.owner or ''}") + print(f"Requested: {','.join(result.assigned) or ''}") + print(f"Stand-ins: {len(result.standin_notes)}") + print(f"Unresolved: {len(result.unresolved_notes)}") + print(f"No-docs impact: {result.fyi_count} (not auto-notified)") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/test_assign_release_docs_reviewers.py b/.github/scripts/test_assign_release_docs_reviewers.py new file mode 100644 index 00000000..bc2d8f9c --- /dev/null +++ b/.github/scripts/test_assign_release_docs_reviewers.py @@ -0,0 +1,209 @@ +import unittest +from pathlib import Path + +from assign_release_docs_reviewers import ( + Config, + assign_reviewers, + select_contributors, +) + + +SHA_A = "a" * 40 +SHA_B = "b" * 40 +SHA_C = "c" * 40 + + +class FakeGitHub: + def __init__(self): + self.owner_assignment = True + self.review_results = {} + self.prs_by_sha = {} + self.mergers = {} + self.assignee_calls = [] + self.review_calls = [] + + def assign_owner(self, login): + self.assignee_calls.append(login) + return self.owner_assignment + + def request_review(self, login): + self.review_calls.append(login) + return self.review_results.get(login, True) + + def merged_prs_for_commit(self, sha): + return self.prs_by_sha.get(sha, []) + + def merger_for_pr(self, pr_number): + return self.mergers.get(pr_number, "") + + +def config(owner="", candidates=None): + return Config( + review_repo="stacklok/docs-website", + release_repo="stacklok/toolhive", + pr_number="123", + owner=owner, + compare_ok="true", + candidates=candidates or [], + github_output=Path("unused"), + ) + + +def release_meta(commits): + return {"commits": commits} + + +class ReviewerSelectionTests(unittest.TestCase): + def test_owner_is_requested_when_classified_non_docs_facing(self): + reviewers = { + "contributors": [ + {"login": "owner", "docs_facing": False}, + { + "login": "alice", + "docs_facing": True, + "docs_facing_shas": [SHA_A], + }, + ] + } + meta = release_meta( + [ + {"author": "owner", "sha": SHA_B}, + {"author": "alice", "sha": SHA_A}, + ] + ) + selection = select_contributors(["owner", "alice"], reviewers, meta) + github = FakeGitHub() + + result = assign_reviewers( + config("owner", ["owner", "alice"]), selection, github + ) + + self.assertEqual(github.assignee_calls, ["owner"]) + self.assertEqual(github.review_calls, ["owner", "alice"]) + self.assertEqual(result.assigned, ["owner", "alice"]) + self.assertEqual(result.fyi_count, 0) + + def test_missing_classification_requests_all_and_falls_back_to_merger(self): + meta = release_meta( + [ + {"author": "alice", "sha": SHA_A}, + {"author": "external", "sha": SHA_B}, + ] + ) + selection = select_contributors(["alice", "external"], None, meta) + github = FakeGitHub() + github.review_results = {"external": False} + github.prs_by_sha = {SHA_B: [77]} + github.mergers = {77: "merger"} + + result = assign_reviewers( + config(candidates=["alice", "external"]), selection, github + ) + + self.assertFalse(selection.classified) + self.assertEqual(github.review_calls, ["alice", "external", "merger"]) + self.assertEqual(result.assigned, ["alice", "merger"]) + self.assertEqual(result.standin_notes, ["@merger (merged stacklok/toolhive#77)"]) + + def test_all_docs_facing_pr_mergers_are_requested_and_deduplicated(self): + reviewers = { + "contributors": [ + { + "login": "external", + "docs_facing": True, + "docs_facing_shas": [SHA_A, SHA_B, SHA_C], + } + ] + } + meta = release_meta( + [ + {"author": "external", "sha": SHA_A}, + {"author": "external", "sha": SHA_B}, + {"author": "external", "sha": SHA_C}, + ] + ) + selection = select_contributors(["external"], reviewers, meta) + github = FakeGitHub() + github.review_results = {"external": False} + github.prs_by_sha = {SHA_A: [10], SHA_B: [10], SHA_C: [11]} + github.mergers = {10: "alice", 11: "bob"} + + result = assign_reviewers(config(candidates=["external"]), selection, github) + + self.assertEqual(github.review_calls, ["external", "alice", "bob"]) + self.assertEqual(result.assigned, ["alice", "bob"]) + self.assertEqual( + result.standin_notes, + [ + "@alice (merged stacklok/toolhive#10)", + "@bob (merged stacklok/toolhive#11)", + ], + ) + + def test_non_docs_facing_contributors_are_counted_without_requests(self): + reviewers = { + "contributors": [ + {"login": "alice", "docs_facing": False}, + {"login": "external", "docs_facing": False}, + ] + } + meta = release_meta( + [ + {"author": "alice", "sha": SHA_A}, + {"author": "external", "sha": SHA_B}, + ] + ) + selection = select_contributors(["alice", "external"], reviewers, meta) + github = FakeGitHub() + + result = assign_reviewers( + config(candidates=["alice", "external"]), selection, github + ) + + self.assertEqual(github.review_calls, []) + self.assertEqual(result.fyi_count, 2) + + def test_missing_standin_is_an_owner_routing_warning_without_at_mention(self): + reviewers = { + "contributors": [ + { + "login": "external", + "docs_facing": True, + "docs_facing_shas": [SHA_A], + } + ] + } + meta = release_meta([{"author": "external", "sha": SHA_A}]) + selection = select_contributors(["external"], reviewers, meta) + github = FakeGitHub() + github.review_results = {"external": False} + + result = assign_reviewers(config(candidates=["external"]), selection, github) + + self.assertEqual(result.assigned, []) + self.assertEqual(len(result.unresolved_notes), 1) + self.assertNotIn("@external", result.unresolved_notes[0]) + + def test_invalid_docs_facing_shas_trigger_noisy_fallback(self): + reviewers = { + "contributors": [ + {"login": "alice", "docs_facing": True}, + {"login": "bob", "docs_facing": False}, + ] + } + meta = release_meta( + [ + {"author": "alice", "sha": SHA_A}, + {"author": "bob", "sha": SHA_B}, + ] + ) + + selection = select_contributors(["alice", "bob"], reviewers, meta) + + self.assertFalse(selection.classified) + self.assertEqual(selection.docs_facing, {"alice": [SHA_A], "bob": [SHA_B]}) + self.assertEqual(selection.non_docs_facing, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/_static-checks.yaml b/.github/workflows/_static-checks.yaml index 97aaa924..0b56e927 100644 --- a/.github/workflows/_static-checks.yaml +++ b/.github/workflows/_static-checks.yaml @@ -24,3 +24,6 @@ jobs: - name: Run Prettier run: npm run prettier + + - name: Test workflow helper scripts + run: python3 -m unittest discover -s .github/scripts -p 'test_*.py' diff --git a/.github/workflows/upstream-release-docs.yml b/.github/workflows/upstream-release-docs.yml index 686fcdd3..0a02ef63 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -569,14 +569,14 @@ jobs: # ---------- handoff file for the skill ---------- # The skill reads .release-meta.json to learn who the owner # is and which contributors it must classify. Gitignored. - # One entry per commit in the range: short sha, subject, + # One entry per commit in the range: full SHA, subject, # author login. Empty when the compare failed. COMMITS_JSON='[]' COMMITS_TRUNCATED=false if [ -n "$COMPARE_JSON" ]; then COMMITS_JSON=$(printf '%s' "$COMPARE_JSON" | jq -c ' [ .commits[] - | { sha: (.sha[0:8]), + | { sha: .sha, subject: (.commit.message | split("\n")[0]), author: (.author.login? // .commit.author.name? // null) } ]' \ 2>/dev/null || echo '[]') @@ -1194,147 +1194,33 @@ jobs: # ------------------------- # OWNER -> assignee AND reviewer, unconditionally. They own # collecting the remaining approvals and merging, so the - # request stands even when the classification marks their - # own commits non-docs-facing. - # DOCS-FACING contributors -> reviewer. - # NON-DOCS-FACING contributors -> no request. Named on one cc - # line in the PR body and one line in the Slack message, - # asking them to speak up if the call was wrong. + # request stands even when their commits are non-docs-facing. + # DOCS-FACING contributors -> try a direct review request. + # GitHub accepts only people with repository access. When it + # rejects the request, find every upstream PR containing a + # docs-facing commit and request each human merger as a + # stand-in. If routing still fails, make the owner resolve it. + # NON-DOCS-FACING contributors -> no request or automatic + # mention. They have nothing to act on, so the workflow does + # not need to identify which are organization members. Count + # this group in the PR body without pinging anyone. # # The classification comes from REVIEWERS.json, written by the # skill (see its "Execution modes" section). An absent, # unparseable, or empty file falls back to requesting every # contributor: a noisy request costs less than a missing one, so # the degraded path is the noisy one. - - name: Assign reviewers and prepare contributor mentions + - name: Assign reviewers and prepare review routing id: reviewers env: REVIEW_REPO: ${{ github.repository }} + RELEASE_REPO: ${{ steps.detect.outputs.repo }} PR_NUMBER: ${{ steps.eff.outputs.number }} OWNER: ${{ steps.release_meta.outputs.owner }} COMPARE_OK: ${{ steps.release_meta.outputs.compare_ok }} CANDIDATES: ${{ steps.release_meta.outputs.candidates }} run: | - # compare_ok is re-exported unchanged so the PR body's - # tri-state contributor cell keeps working: "" means this - # step never ran, "false" means the upstream compare failed. - echo "compare_ok=$COMPARE_OK" >> "$GITHUB_OUTPUT" - - # ---------- read the skill's classification ---------- - # Both lists are newline-separated logins. - DOCS_FACING="" - NON_DOCS_FACING="" - CLASSIFIED=false - if [ -f REVIEWERS.json ] && jq -e . REVIEWERS.json >/dev/null 2>&1; then - DOCS_FACING=$(jq -r ' - (.contributors // []) - | map(select(.docs_facing == true)) - | .[].login' REVIEWERS.json 2>/dev/null || true) - NON_DOCS_FACING=$(jq -r ' - (.contributors // []) - | map(select(.docs_facing == false)) - | .[].login' REVIEWERS.json 2>/dev/null || true) - # A file that classified nobody is a stub; fall back - # rather than silently requesting no reviews at all. - if [ -n "$DOCS_FACING$NON_DOCS_FACING" ]; then - CLASSIFIED=true - else - echo "::warning::REVIEWERS.json classified no contributors; falling back to requesting all." - fi - else - echo "::warning::No usable REVIEWERS.json; falling back to requesting a review from all contributors." - fi - - if [ "$CLASSIFIED" != "true" ]; then - DOCS_FACING="$CANDIDATES" - NON_DOCS_FACING="" - fi - - # The owner always gets a review request, even when their own - # commits were classified non-docs-facing. Fold them into the - # request list and out of the cc list. - if [ -n "$OWNER" ]; then - DOCS_FACING=$(printf '%s\n%s\n' "$DOCS_FACING" "$OWNER" | grep -v '^$' | sort -u || true) - NON_DOCS_FACING=$(printf '%s\n' "$NON_DOCS_FACING" | grep -v '^$' | grep -Fxv "$OWNER" || true) - fi - - # ---------- assign the owner ---------- - # Assignee, not just reviewer: it puts the PR in the owner's - # "Assigned to me" queue. Soft-failed, since an owner - # GitHub won't accept (not a repo collaborator) must not - # abort the run. - OWNER_ASSIGNED=false - if [ -n "$OWNER" ]; then - if gh pr edit "$PR_NUMBER" --add-assignee "$OWNER" 2>/dev/null; then - OWNER_ASSIGNED=true - echo "Assigned owner: $OWNER" - else - echo "::warning::Could not assign $OWNER as assignee (GitHub rejected it)." - fi - else - echo "::warning::No release owner resolved; PR has no assignee." - fi - echo "owner=$OWNER" >> "$GITHUB_OUTPUT" - echo "owner_assigned=$OWNER_ASSIGNED" >> "$GITHUB_OUTPUT" - - # ---------- request reviews ---------- - # Attempt to assign each candidate as a reviewer individually, - # rather than filtering upfront and batching. Rationale: - # - `gh pr edit --add-reviewer "a,b,c"` is atomic. A single - # 422 on any name aborts the whole call, dropping valid - # names alongside invalid ones. - # - `gh api repos/X/collaborators/Y` as a pre-filter is - # unreliable from a GITHUB_TOKEN in Actions: on PR #759 - # the check returned 404 for Stacklok employees who ARE - # collaborators via the `stackers` team (push perm on - # this repo), and only `rdimitrov` slipped through. We - # suspect the collaborator endpoint treats team-based - # access differently for GITHUB_TOKEN vs PATs with - # read:org, but haven't nailed down the exact rule. - # Per-user attempts sidestep both issues: the authoritative - # answer is "does GitHub accept this as a reviewer right now" - # and we ask the API that question directly. - ASSIGN_LIST="" - MENTION_LIST="" - while IFS= read -r login; do - [ -z "$login" ] && continue - if gh pr edit "$PR_NUMBER" --add-reviewer "$login" 2>/dev/null; then - ASSIGN_LIST="${ASSIGN_LIST:+$ASSIGN_LIST,}$login" - echo "Review requested: $login" - else - MENTION_LIST="${MENTION_LIST:+$MENTION_LIST }@$login" - echo "Mention (assignment rejected by GitHub): $login" - fi - done <<< "$DOCS_FACING" - - # ---------- cc list for non-docs-facing contributors ---------- - # Rendered as one consolidated line downstream, not a - # per-person section. - FYI_LIST="" - while IFS= read -r login; do - [ -z "$login" ] && continue - FYI_LIST="${FYI_LIST:+$FYI_LIST }@$login" - done <<< "$NON_DOCS_FACING" - - # Exposed for diagnostic visibility in the PR body (e.g., - # "Auto-assigned: @alice @bob") and for the next workflow_ - # dispatch retry to know what was attempted. - echo "list=$ASSIGN_LIST" >> "$GITHUB_OUTPUT" - echo "fyi_list=$FYI_LIST" >> "$GITHUB_OUTPUT" - echo "classified=$CLASSIFIED" >> "$GITHUB_OUTPUT" - { - echo "mention_block<> "$GITHUB_OUTPUT" - echo "Owner: ${OWNER:-}" - echo "Requested: ${ASSIGN_LIST:-}" - echo "Mentioned: ${MENTION_LIST:-}" - echo "FYI (no req): ${FYI_LIST:-}" + python3 .github/scripts/assign_release_docs_reviewers.py - name: Augment PR body (marker-delimited section) # Runs even if earlier steps soft-failed so the augmentation @@ -1353,9 +1239,11 @@ jobs: AUTOGEN_TOUCHED: ${{ steps.autogen.outputs.touched }} CRD_PAGES_DIRS: ${{ steps.crd_pages.outputs.dirs }} COMPARE_OK: ${{ steps.reviewers.outputs.compare_ok }} - MENTION_BLOCK: ${{ steps.reviewers.outputs.mention_block }} + STANDIN_BLOCK: ${{ steps.reviewers.outputs.standin_block }} + UNRESOLVED_BLOCK: ${{ steps.reviewers.outputs.unresolved_block }} + UNRESOLVED_COUNT: ${{ steps.reviewers.outputs.unresolved_count }} ASSIGN_LIST: ${{ steps.reviewers.outputs.list }} - FYI_LIST: ${{ steps.reviewers.outputs.fyi_list }} + FYI_COUNT: ${{ steps.reviewers.outputs.fyi_count }} OWNER: ${{ steps.reviewers.outputs.owner }} OWNER_ASSIGNED: ${{ steps.reviewers.outputs.owner_assigned }} OWNER_SOURCE: ${{ steps.release_meta.outputs.owner_source }} @@ -1407,11 +1295,14 @@ jobs: fi # Action-required verdict drives the At-a-glance table's - # last row. Priority: autogen-drift > gaps > silent > content > none. + # last row. Priority: autogen drift > gaps > unresolved review + # routing > silent run > content review > none. if [ -n "$AUTOGEN_DRIFT" ]; then ACTION_REQUIRED="**Yes** — revert auto-generated-path drift (see above)" elif [ "$GAPS_COUNT" -gt 0 ]; then ACTION_REQUIRED="**Yes** — resolve $GAPS_COUNT gap(s), then spot-check prose" + elif [ "${UNRESOLVED_COUNT:-0}" -gt 0 ]; then + ACTION_REQUIRED="**Yes** — owner must route $UNRESOLVED_COUNT unresolved review(s)" elif [ "$SILENT_RUN" = "true" ]; then ACTION_REQUIRED="**None** — approve and merge if the silent-run signal is expected" elif [ "$SKILL_COMMIT_COUNT" != "0" ] && [ -n "$SKILL_COMMIT_COUNT" ]; then @@ -1435,23 +1326,15 @@ jobs: *) REFRESH_CELL="—" ;; esac - # Contributor counts. Auto-assigned folks already appear in + # Contributor counts. Requested reviewers already appear in # GitHub's reviewer sidebar, so we don't also list them in # the PR body -- that would duplicate info across three # places (sidebar, at-a-glance cell, dedicated section). - # Only the overflow (non-collaborator) mentions need a - # render target, so the dedicated section is skipped when - # MENTION_COUNT is zero. if [ -n "$ASSIGN_LIST" ]; then ASSIGN_COUNT=$(echo "$ASSIGN_LIST" | tr ',' '\n' | grep -c . || true) else ASSIGN_COUNT=0 fi - if [ -n "$MENTION_BLOCK" ]; then - MENTION_COUNT=$(printf '%s\n' "$MENTION_BLOCK" | grep -oE '@[A-Za-z0-9_-]+' | wc -l | tr -d ' ') - else - MENTION_COUNT=0 - fi # COMPARE_OK tri-state: # "true" -> compare succeeded, contributor list is populated @@ -1467,23 +1350,22 @@ jobs: CONTRIB_CELL="**Not attempted** — run failed before reviewer assignment" elif [ "$COMPARE_OK" != "true" ]; then CONTRIB_CELL="**Compare failed** — pinned \`$PREV_TAG\` missing upstream, no auto-assignment" - elif [ "$ASSIGN_COUNT" -gt 0 ] && [ "$MENTION_COUNT" -gt 0 ]; then - CONTRIB_CELL="$ASSIGN_COUNT review requested · $MENTION_COUNT mentioned below" elif [ "$ASSIGN_COUNT" -gt 0 ]; then CONTRIB_CELL="$ASSIGN_COUNT review requested (see sidebar)" - elif [ "$MENTION_COUNT" -gt 0 ]; then - CONTRIB_CELL="$MENTION_COUNT mentioned below" else CONTRIB_CELL="none in release range" fi - # Folded into the same cell so the table keeps one - # contributor row; the names go on the cc line further down. - if [ -n "$FYI_LIST" ]; then - FYI_COUNT=$(printf '%s\n' "$FYI_LIST" | grep -oE '@[A-Za-z0-9_-]+' | wc -l | tr -d ' ') + # Fold the count into the same cell so the table keeps one + # contributor row. These contributors have no review action, + # so the workflow does not need to determine membership. + FYI_COUNT=${FYI_COUNT:-0} + if [ "$FYI_COUNT" -gt 0 ]; then CONTRIB_CELL="$CONTRIB_CELL · $FYI_COUNT not requested (no docs impact)" - else - FYI_COUNT=0 + fi + UNRESOLVED_COUNT=${UNRESOLVED_COUNT:-0} + if [ "$UNRESOLVED_COUNT" -gt 0 ]; then + CONTRIB_CELL="$CONTRIB_CELL · **$UNRESOLVED_COUNT review routing issue(s)**" fi # Owner cell. An unresolved owner means nobody is on the @@ -1589,30 +1471,38 @@ jobs: fi # ----- NO-DOCS-IMPACT CONTRIBUTORS (no review requested) ----- - # One line, not a per-person section. These contributors - # are absent from the reviewer sidebar by design, so this - # is the only place their involvement is recorded. + # Contributors in this group get no automatic mention. They + # have no review action, so membership is irrelevant. if [ "$FYI_COUNT" -gt 0 ]; then echo "### No docs impact identified" echo "" - echo "$FYI_LIST - your changes in this release didn't appear to affect the docs, so no review is requested and you're not blocking this PR. Please skim the diff anyway and comment if something of yours was missed or misjudged." + echo "$FYI_COUNT contributor(s) had no-docs-impact commits in this release. No review was requested, and the workflow did not auto-notify them." echo "" fi - # ----- OVERFLOW CONTRIBUTORS (not auto-assigned) ----- - # Skipped entirely when everyone who should review got - # auto-assigned -- GitHub's sidebar covers that case. This - # section exists only to render @-mentions for contributors - # GitHub refused to accept as reviewers (usually because - # they're not collaborators on this repo). - if [ "$MENTION_COUNT" -gt 0 ]; then - echo "### Additional release contributors" - echo "" - echo "Couldn't be auto-assigned as reviewers on this repo, but cc'd below so they see the PR:" - echo "" - MENTIONS_ONLY=$(printf '%s\n' "$MENTION_BLOCK" | grep -oE '@[A-Za-z0-9_-]+' | paste -sd' ' -) - echo "$MENTIONS_ONLY" + # ----- REVIEW ROUTING ----- + # Explain stand-in requests and surface anything the owner + # must route manually. Contributors who cannot review this + # repository are not @-mentioned. + STANDIN_COUNT=0 + if [ -n "$STANDIN_BLOCK" ]; then + STANDIN_COUNT=$(printf '%s\n' "$STANDIN_BLOCK" | grep -c . || true) + fi + if [ "$STANDIN_COUNT" -gt 0 ] || [ "$UNRESOLVED_COUNT" -gt 0 ]; then + echo "### Review routing" echo "" + if [ "$STANDIN_COUNT" -gt 0 ]; then + echo "GitHub could not request the upstream contributor directly, so review went to the human merger of each relevant upstream PR:" + echo "" + printf '%s\n' "$STANDIN_BLOCK" | awk '{ print "- " $0 }' + echo "" + fi + if [ "$UNRESOLVED_COUNT" -gt 0 ]; then + echo "The release owner must route these unresolved reviews:" + echo "" + printf '%s\n' "$UNRESOLVED_BLOCK" | awk '{ print "- " $0 }' + echo "" + fi fi # ----- RUN COST ----- diff --git a/.gitignore b/.gitignore index 68175f85..6980cd14 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ # Misc .DS_Store +__pycache__/ .env.local .env.development.local .env.test.local From ca38ddeed53a265fdb18acf5f5f16c84245cbef2 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:44:02 -0400 Subject: [PATCH 2/3] Address reviewer routing feedback --- .../scripts/assign_release_docs_reviewers.py | 8 ++++- .../test_assign_release_docs_reviewers.py | 33 +++++++++++++++++++ .github/workflows/upstream-release-docs.yml | 13 ++++---- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/.github/scripts/assign_release_docs_reviewers.py b/.github/scripts/assign_release_docs_reviewers.py index 8a54ed63..0d188f54 100644 --- a/.github/scripts/assign_release_docs_reviewers.py +++ b/.github/scripts/assign_release_docs_reviewers.py @@ -327,6 +327,7 @@ def request_once(login: str) -> bool: "a stand-in. The release owner must route this review." ) continue + review_already_requested = merger in requested if not request_once(merger): result.unresolved_notes.append( f"Upstream merger `{merger}` could not be requested for " @@ -334,7 +335,12 @@ def request_once(login: str) -> bool: ) continue result.standin_notes.append(f"@{merger} (merged {upstream_pr})") - print(f"Stand-in review requested: {merger} for {upstream_pr}") + if review_already_requested: + print( + f"Stand-in review already active: {merger} for {upstream_pr}" + ) + else: + print(f"Stand-in review requested: {merger} for {upstream_pr}") result.assigned = unique(result.assigned) result.standin_notes = unique(result.standin_notes) diff --git a/.github/scripts/test_assign_release_docs_reviewers.py b/.github/scripts/test_assign_release_docs_reviewers.py index bc2d8f9c..b4a70fa0 100644 --- a/.github/scripts/test_assign_release_docs_reviewers.py +++ b/.github/scripts/test_assign_release_docs_reviewers.py @@ -1,4 +1,6 @@ import unittest +from contextlib import redirect_stdout +from io import StringIO from pathlib import Path from assign_release_docs_reviewers import ( @@ -140,6 +142,37 @@ def test_all_docs_facing_pr_mergers_are_requested_and_deduplicated(self): ], ) + def test_existing_reviewer_is_recorded_as_an_active_standin(self): + reviewers = { + "contributors": [ + { + "login": "external", + "docs_facing": True, + "docs_facing_shas": [SHA_A], + } + ] + } + meta = release_meta([{"author": "external", "sha": SHA_A}]) + selection = select_contributors(["external"], reviewers, meta) + github = FakeGitHub() + github.review_results = {"external": False} + github.prs_by_sha = {SHA_A: [77]} + github.mergers = {77: "owner"} + output = StringIO() + + with redirect_stdout(output): + result = assign_reviewers( + config(owner="owner", candidates=["external"]), selection, github + ) + + self.assertEqual(github.review_calls, ["owner", "external"]) + self.assertEqual(result.assigned, ["owner"]) + self.assertEqual(result.standin_notes, ["@owner (merged stacklok/toolhive#77)"]) + self.assertIn( + "Stand-in review already active: owner for stacklok/toolhive#77", + output.getvalue(), + ) + def test_non_docs_facing_contributors_are_counted_without_requests(self): reviewers = { "contributors": [ diff --git a/.github/workflows/upstream-release-docs.yml b/.github/workflows/upstream-release-docs.yml index 0a02ef63..9a1fd23d 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -569,8 +569,9 @@ jobs: # ---------- handoff file for the skill ---------- # The skill reads .release-meta.json to learn who the owner # is and which contributors it must classify. Gitignored. - # One entry per commit in the range: full SHA, subject, - # author login. Empty when the compare failed. + # One entry per commit in the range: full SHA, subject, and + # author login when available (otherwise the commit author + # name). Empty when the compare failed. COMMITS_JSON='[]' COMMITS_TRUNCATED=false if [ -n "$COMPARE_JSON" ]; then @@ -1302,7 +1303,7 @@ jobs: elif [ "$GAPS_COUNT" -gt 0 ]; then ACTION_REQUIRED="**Yes** — resolve $GAPS_COUNT gap(s), then spot-check prose" elif [ "${UNRESOLVED_COUNT:-0}" -gt 0 ]; then - ACTION_REQUIRED="**Yes** — owner must route $UNRESOLVED_COUNT unresolved review(s)" + ACTION_REQUIRED="**Yes** — manually route $UNRESOLVED_COUNT unresolved review(s)" elif [ "$SILENT_RUN" = "true" ]; then ACTION_REQUIRED="**None** — approve and merge if the silent-run signal is expected" elif [ "$SKILL_COMMIT_COUNT" != "0" ] && [ -n "$SKILL_COMMIT_COUNT" ]; then @@ -1481,8 +1482,8 @@ jobs: fi # ----- REVIEW ROUTING ----- - # Explain stand-in requests and surface anything the owner - # must route manually. Contributors who cannot review this + # Explain stand-in requests and surface anything that + # requires manual routing. Contributors who cannot review this # repository are not @-mentioned. STANDIN_COUNT=0 if [ -n "$STANDIN_BLOCK" ]; then @@ -1498,7 +1499,7 @@ jobs: echo "" fi if [ "$UNRESOLVED_COUNT" -gt 0 ]; then - echo "The release owner must route these unresolved reviews:" + echo "These reviews require manual routing:" echo "" printf '%s\n' "$UNRESOLVED_BLOCK" | awk '{ print "- " $0 }' echo "" From 00746ba3281a76d6561f16cdea428ebc246b11ef Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Fri, 28 Aug 2026 10:40:09 -0400 Subject: [PATCH 3/3] Handle reviewer routing failures --- .../scripts/assign_release_docs_reviewers.py | 121 ++++++++++++---- .../test_assign_release_docs_reviewers.py | 129 +++++++++++++++++- .github/workflows/upstream-release-docs.yml | 7 +- 3 files changed, 218 insertions(+), 39 deletions(-) diff --git a/.github/scripts/assign_release_docs_reviewers.py b/.github/scripts/assign_release_docs_reviewers.py index 0d188f54..b0095689 100644 --- a/.github/scripts/assign_release_docs_reviewers.py +++ b/.github/scripts/assign_release_docs_reviewers.py @@ -3,9 +3,10 @@ GitHub itself is the reviewer-access authority: request the upstream contributor directly, then fall back to the human who merged each relevant -upstream pull request when GitHub rejects that request. This deliberately -avoids organization-membership and collaborator lookups, which require -broader credentials or behave inconsistently for team-derived access. +upstream pull request when GitHub rejects that request because the contributor +lacks access. Other API failures stop the routing step. This deliberately avoids +organization-membership and collaborator lookups, which require broader +credentials or behave inconsistently for team-derived access. """ from __future__ import annotations @@ -16,6 +17,7 @@ import subprocess import sys from dataclasses import dataclass, field +from enum import Enum from pathlib import Path @@ -23,6 +25,8 @@ r"(\[bot\]$|^app/|^github-actions|^stacklokbot$|^dependabot|^renovate|^copilot)", re.IGNORECASE, ) +HTTP_STATUS = re.compile(r"^HTTP/\S+ (\d{3})\b", re.MULTILINE) +REVIEW_ACCESS_REJECTION = "Reviews may only be requested from collaborators" def warning(message: str) -> None: @@ -75,6 +79,15 @@ class AssignmentResult: unresolved_notes: list[str] = field(default_factory=list) +class ReviewRequestOutcome(Enum): + REQUESTED = "requested" + ACCESS_REJECTED = "access_rejected" + + +class ReviewRequestError(RuntimeError): + """Raised when GitHub cannot provide a trustworthy routing decision.""" + + class GitHub: def __init__(self, config: Config): self.config = config @@ -100,9 +113,10 @@ def assign_owner(self, login: str) -> bool: ) return result.returncode == 0 - def request_review(self, login: str) -> bool: + def request_review(self, login: str) -> ReviewRequestOutcome: result = self._run( "api", + "--include", "--method", "POST", ( @@ -112,7 +126,21 @@ def request_review(self, login: str) -> bool: "-f", f"reviewers[]={login}", ) - return result.returncode == 0 + if result.returncode == 0: + return ReviewRequestOutcome.REQUESTED + + statuses = HTTP_STATUS.findall(result.stdout) + status = int(statuses[-1]) if statuses else None + response = f"{result.stdout}\n{result.stderr}" + if status == 422 and REVIEW_ACCESS_REJECTION in response: + return ReviewRequestOutcome.ACCESS_REJECTED + + detail = result.stderr.strip() or "no error detail from gh" + status_label = str(status) if status is not None else "unavailable" + raise ReviewRequestError( + f"Review request for {login} failed without an access decision " + f"(HTTP status {status_label}): {detail}" + ) def merged_prs_for_commit(self, sha: str) -> list[int]: result = self._run( @@ -201,14 +229,26 @@ def select_contributors( ) by_login: dict[str, dict] = {} + duplicate_candidates: list[str] = [] + candidate_set = set(candidates) for entry in entries: if not isinstance(entry, dict): continue login = entry.get("login") docs_facing = entry.get("docs_facing") if isinstance(login, str) and isinstance(docs_facing, bool): + if login in by_login and login in candidate_set: + duplicate_candidates.append(login) by_login[login] = entry + if duplicate_candidates: + return fallback_selection( + candidates, + commits, + "REVIEWERS.json repeated release contributors: " + + ", ".join(unique(duplicate_candidates)), + ) + missing = [login for login in candidates if login not in by_login] if missing: return fallback_selection( @@ -256,18 +296,19 @@ def assign_reviewers( config: Config, selection: Selection, github: GitHub ) -> AssignmentResult: result = AssignmentResult() - requested: set[str] = set() - - def request_once(login: str) -> bool: - if login in requested: - return True - if github.request_review(login): - requested.add(login) + request_outcomes: dict[str, ReviewRequestOutcome] = {} + + def request_once(login: str) -> ReviewRequestOutcome: + if login in request_outcomes: + return request_outcomes[login] + outcome = github.request_review(login) + request_outcomes[login] = outcome + if outcome is ReviewRequestOutcome.REQUESTED: result.assigned.append(login) print(f"Review requested: {login}") - return True - return False + return outcome + owner_review_outcome: ReviewRequestOutcome | None = None if config.owner: result.owner_assigned = github.assign_owner(config.owner) if result.owner_assigned: @@ -275,23 +316,22 @@ def request_once(login: str) -> bool: else: warning(f"Could not assign owner {config.owner} as assignee.") - if not request_once(config.owner): + owner_review_outcome = request_once(config.owner) + if owner_review_outcome is ReviewRequestOutcome.ACCESS_REJECTED: warning(f"Could not request a review from owner {config.owner}.") - result.unresolved_notes.append( - f"Release owner `{config.owner}` could not be requested as a reviewer. " - "A docs maintainer must route this review." - ) else: warning("No release owner resolved; PR has no assignee or owner review.") + result.unresolved_notes.append( + "No release owner could be resolved. A docs maintainer must adopt this " + "PR and route its reviews." + ) result.fyi_count = len( [login for login in selection.non_docs_facing if login != config.owner] ) for contributor, shas in selection.docs_facing.items(): - if contributor == config.owner: - continue - if request_once(contributor): + if request_once(contributor) is ReviewRequestOutcome.REQUESTED: continue print( @@ -299,10 +339,15 @@ def request_once(login: str) -> bool: "resolving upstream merger stand-ins." ) seen_prs: set[int] = set() + routing_actor = ( + "A docs maintainer" + if contributor == config.owner + else "The release owner" + ) if not shas: result.unresolved_notes.append( f"Docs-facing contributor `{contributor}` has no commit available for " - "stand-in resolution. The release owner must route this review." + f"stand-in resolution. {routing_actor} must route this review." ) continue @@ -311,7 +356,7 @@ def request_once(login: str) -> bool: if not pull_numbers: result.unresolved_notes.append( f"Docs-facing commit `{sha[:12]}` could not be mapped to a merged " - "upstream PR. The release owner must route this review." + f"upstream PR. {routing_actor} must route this review." ) continue @@ -324,14 +369,19 @@ def request_once(login: str) -> bool: if not is_human_standin(merger, contributor): result.unresolved_notes.append( f"Upstream PR `{upstream_pr}` has no human merger available as " - "a stand-in. The release owner must route this review." + f"a stand-in. {routing_actor} must route this review." ) continue - review_already_requested = merger in requested - if not request_once(merger): + review_already_requested = ( + request_outcomes.get(merger) is ReviewRequestOutcome.REQUESTED + ) + if ( + request_once(merger) + is not ReviewRequestOutcome.REQUESTED + ): result.unresolved_notes.append( f"Upstream merger `{merger}` could not be requested for " - f"`{upstream_pr}`. The release owner must route this review." + f"`{upstream_pr}`. {routing_actor} must route this review." ) continue result.standin_notes.append(f"@{merger} (merged {upstream_pr})") @@ -342,6 +392,15 @@ def request_once(login: str) -> bool: else: print(f"Stand-in review requested: {merger} for {upstream_pr}") + if ( + owner_review_outcome is ReviewRequestOutcome.ACCESS_REJECTED + and config.owner not in selection.docs_facing + ): + result.unresolved_notes.append( + f"Release owner `{config.owner}` could not be requested as a reviewer. " + "A docs maintainer must route this review." + ) + result.assigned = unique(result.assigned) result.standin_notes = unique(result.standin_notes) result.unresolved_notes = unique(result.unresolved_notes) @@ -376,7 +435,11 @@ def main() -> int: reviewers = read_json(Path("REVIEWERS.json")) release_meta = read_json(Path(".release-meta.json")) selection = select_contributors(config.candidates, reviewers, release_meta) - result = assign_reviewers(config, selection, GitHub(config)) + try: + result = assign_reviewers(config, selection, GitHub(config)) + except ReviewRequestError as exc: + print(f"::error::{exc}") + return 1 write_outputs(config, selection, result) print(f"Owner: {config.owner or ''}") print(f"Requested: {','.join(result.assigned) or ''}") diff --git a/.github/scripts/test_assign_release_docs_reviewers.py b/.github/scripts/test_assign_release_docs_reviewers.py index b4a70fa0..a038ac99 100644 --- a/.github/scripts/test_assign_release_docs_reviewers.py +++ b/.github/scripts/test_assign_release_docs_reviewers.py @@ -1,12 +1,20 @@ +import subprocess +import tempfile import unittest from contextlib import redirect_stdout from io import StringIO from pathlib import Path +from unittest.mock import patch from assign_release_docs_reviewers import ( Config, + GitHub, + ReviewRequestError, + ReviewRequestOutcome, + Selection, assign_reviewers, select_contributors, + write_outputs, ) @@ -30,7 +38,7 @@ def assign_owner(self, login): def request_review(self, login): self.review_calls.append(login) - return self.review_results.get(login, True) + return self.review_results.get(login, ReviewRequestOutcome.REQUESTED) def merged_prs_for_commit(self, sha): return self.prs_by_sha.get(sha, []) @@ -94,7 +102,9 @@ def test_missing_classification_requests_all_and_falls_back_to_merger(self): ) selection = select_contributors(["alice", "external"], None, meta) github = FakeGitHub() - github.review_results = {"external": False} + github.review_results = { + "external": ReviewRequestOutcome.ACCESS_REJECTED + } github.prs_by_sha = {SHA_B: [77]} github.mergers = {77: "merger"} @@ -126,7 +136,9 @@ def test_all_docs_facing_pr_mergers_are_requested_and_deduplicated(self): ) selection = select_contributors(["external"], reviewers, meta) github = FakeGitHub() - github.review_results = {"external": False} + github.review_results = { + "external": ReviewRequestOutcome.ACCESS_REJECTED + } github.prs_by_sha = {SHA_A: [10], SHA_B: [10], SHA_C: [11]} github.mergers = {10: "alice", 11: "bob"} @@ -155,7 +167,9 @@ def test_existing_reviewer_is_recorded_as_an_active_standin(self): meta = release_meta([{"author": "external", "sha": SHA_A}]) selection = select_contributors(["external"], reviewers, meta) github = FakeGitHub() - github.review_results = {"external": False} + github.review_results = { + "external": ReviewRequestOutcome.ACCESS_REJECTED + } github.prs_by_sha = {SHA_A: [77]} github.mergers = {77: "owner"} output = StringIO() @@ -209,11 +223,15 @@ def test_missing_standin_is_an_owner_routing_warning_without_at_mention(self): meta = release_meta([{"author": "external", "sha": SHA_A}]) selection = select_contributors(["external"], reviewers, meta) github = FakeGitHub() - github.review_results = {"external": False} + github.review_results = { + "external": ReviewRequestOutcome.ACCESS_REJECTED + } - result = assign_reviewers(config(candidates=["external"]), selection, github) + result = assign_reviewers( + config(owner="owner", candidates=["external"]), selection, github + ) - self.assertEqual(result.assigned, []) + self.assertEqual(result.assigned, ["owner"]) self.assertEqual(len(result.unresolved_notes), 1) self.assertNotIn("@external", result.unresolved_notes[0]) @@ -237,6 +255,103 @@ def test_invalid_docs_facing_shas_trigger_noisy_fallback(self): self.assertEqual(selection.docs_facing, {"alice": [SHA_A], "bob": [SHA_B]}) self.assertEqual(selection.non_docs_facing, []) + def test_duplicate_contributor_records_trigger_noisy_fallback(self): + reviewers = { + "contributors": [ + { + "login": "alice", + "docs_facing": True, + "docs_facing_shas": [SHA_A], + }, + {"login": "alice", "docs_facing": False}, + ] + } + meta = release_meta([{"author": "alice", "sha": SHA_A}]) + + selection = select_contributors(["alice"], reviewers, meta) + + self.assertFalse(selection.classified) + self.assertEqual(selection.docs_facing, {"alice": [SHA_A]}) + self.assertEqual(selection.non_docs_facing, []) + + def test_rejected_docs_facing_owner_falls_back_to_merger(self): + reviewers = { + "contributors": [ + { + "login": "owner", + "docs_facing": True, + "docs_facing_shas": [SHA_A], + } + ] + } + meta = release_meta([{"author": "owner", "sha": SHA_A}]) + selection = select_contributors(["owner"], reviewers, meta) + github = FakeGitHub() + github.review_results = {"owner": ReviewRequestOutcome.ACCESS_REJECTED} + github.prs_by_sha = {SHA_A: [77]} + github.mergers = {77: "merger"} + + result = assign_reviewers(config(owner="owner"), selection, github) + + self.assertEqual(github.review_calls, ["owner", "merger"]) + self.assertEqual(result.assigned, ["merger"]) + self.assertEqual(result.standin_notes, ["@merger (merged stacklok/toolhive#77)"]) + self.assertEqual(result.unresolved_notes, []) + + def test_missing_owner_is_an_unresolved_routing_outcome(self): + selection = Selection(docs_facing={}, non_docs_facing=[], classified=True) + + result = assign_reviewers(config(), selection, FakeGitHub()) + + self.assertEqual(len(result.unresolved_notes), 1) + self.assertIn("No release owner could be resolved", result.unresolved_notes[0]) + with tempfile.TemporaryDirectory() as temp_dir: + output_path = Path(temp_dir) / "github-output" + test_config = Config( + review_repo="stacklok/docs-website", + release_repo="stacklok/toolhive", + pr_number="123", + owner="", + compare_ok="true", + candidates=[], + github_output=output_path, + ) + write_outputs(test_config, selection, result) + self.assertIn("unresolved_count=1", output_path.read_text()) + + +class GitHubReviewRequestTests(unittest.TestCase): + def test_access_rejection_is_the_only_expected_failure(self): + response = subprocess.CompletedProcess( + args=[], + returncode=1, + stdout=( + "HTTP/2.0 422 Unprocessable Entity\r\n\r\n" + '{"message":"Reviews may only be requested from collaborators."}' + ), + stderr="gh: Reviews may only be requested from collaborators. (HTTP 422)", + ) + + with patch.object(GitHub, "_run", return_value=response): + outcome = GitHub(config()).request_review("external") + + self.assertIs(outcome, ReviewRequestOutcome.ACCESS_REJECTED) + + def test_transient_api_failure_stops_routing(self): + response = subprocess.CompletedProcess( + args=[], + returncode=1, + stdout=( + "HTTP/2.0 503 Service Unavailable\r\n\r\n" + '{"message":"Service unavailable"}' + ), + stderr="gh: Service unavailable (HTTP 503)", + ) + + with patch.object(GitHub, "_run", return_value=response): + with self.assertRaisesRegex(ReviewRequestError, "HTTP status 503"): + GitHub(config()).request_review("alice") + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/upstream-release-docs.yml b/.github/workflows/upstream-release-docs.yml index 9a1fd23d..845b3513 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -1198,9 +1198,10 @@ jobs: # request stands even when their commits are non-docs-facing. # DOCS-FACING contributors -> try a direct review request. # GitHub accepts only people with repository access. When it - # rejects the request, find every upstream PR containing a - # docs-facing commit and request each human merger as a - # stand-in. If routing still fails, make the owner resolve it. + # rejects the request for that reason, find every upstream PR + # containing a docs-facing commit and request each human merger + # as a stand-in. Other API failures stop the routing step. If + # routing still fails, make the owner resolve it. # NON-DOCS-FACING contributors -> no request or automatic # mention. They have nothing to act on, so the workflow does # not need to identify which are organization members. Count