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..b0095689 --- /dev/null +++ b/.github/scripts/assign_release_docs_reviewers.py @@ -0,0 +1,453 @@ +#!/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 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 + +import json +import os +import re +import subprocess +import sys +from dataclasses import dataclass, field +from enum import Enum +from pathlib import Path + + +BOT_LOGIN = re.compile( + 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: + 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 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 + + @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) -> ReviewRequestOutcome: + result = self._run( + "api", + "--include", + "--method", + "POST", + ( + f"repos/{self.config.review_repo}/pulls/" + f"{self.config.pr_number}/requested_reviewers" + ), + "-f", + f"reviewers[]={login}", + ) + 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( + "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] = {} + 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( + 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() + 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 outcome + + owner_review_outcome: ReviewRequestOutcome | None = None + 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.") + + 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}.") + 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 request_once(contributor) is ReviewRequestOutcome.REQUESTED: + continue + + print( + f"Direct review request rejected for {contributor}; " + "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 " + f"stand-in resolution. {routing_actor} 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 " + f"upstream PR. {routing_actor} 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 " + f"a stand-in. {routing_actor} must route this review." + ) + continue + 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}`. {routing_actor} must route this review." + ) + continue + result.standin_notes.append(f"@{merger} (merged {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}") + + 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) + 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) + 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 ''}") + 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..a038ac99 --- /dev/null +++ b/.github/scripts/test_assign_release_docs_reviewers.py @@ -0,0 +1,357 @@ +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, +) + + +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, ReviewRequestOutcome.REQUESTED) + + 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": ReviewRequestOutcome.ACCESS_REJECTED + } + 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": ReviewRequestOutcome.ACCESS_REJECTED + } + 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_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": ReviewRequestOutcome.ACCESS_REJECTED + } + 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": [ + {"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": ReviewRequestOutcome.ACCESS_REJECTED + } + + result = assign_reviewers( + config(owner="owner", candidates=["external"]), selection, github + ) + + self.assertEqual(result.assigned, ["owner"]) + 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, []) + + 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/_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..845b3513 100644 --- a/.github/workflows/upstream-release-docs.yml +++ b/.github/workflows/upstream-release-docs.yml @@ -569,14 +569,15 @@ 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, - # 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 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 +1195,34 @@ 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 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 + # 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 +1241,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 +1297,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** — 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 @@ -1435,23 +1328,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 +1352,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 +1473,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 that + # requires manual routing. 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 "These reviews require manual routing:" + 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