diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index d8fccd59..63e558a8 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -277,7 +277,13 @@ gh pr create --web So the mention stays, and a real request is added alongside it. **A PR is not complete until `gh pr edit --add-reviewer` has succeeded and been verified.** -A resolution failure must fall back, never no-op. When no owner resolves, assign `dannyneira`, matching the fallback the release docs workflow already uses (`.github/workflows/release-docs-update.yml`, "Assign last docs PR reviewer"). An unassignable reviewer is a problem to surface, not a reason to ship an unreviewed PR. +A resolution failure must fall back, never no-op. When no owner resolves — the common case for a small, ambient/agent-generated docs PR — prefer the run's requester next, then a secondary human fallback, and only then `dannyneira` as the final safety net (the release docs workflow keeps its own last-resort `dannyneira` fallback too — `.github/workflows/release-docs-update.yml`, "Assign last docs PR reviewer"). An unassignable reviewer is a problem to surface, not a reason to ship an unreviewed PR. + +The full priority chain, in order: (1) the CODEOWNERS/git-blame owner from `suggest_reviewers.py`; (2) the run's requester, resolved via this repo's own `.agents/skills/create_pr/resolve_reviewer.py --user ` and a runtime-supplied private override map; (3) a secondary human fallback, currently `hongyi-chen` ("HYC"); (4) `dannyneira`. + +Step 2's resolver is a docs-repo-local script, not `factory-agents`' `scripts/factory-resolve-reviewer`: that script lives in the separate `factory-agents` repo and is not checked out alongside a normal `warpdotdev/docs` clone, so calling it by that relative path fails in a real docs run and silently falls through to the next tier. The invoking factory must mount its private Slack-to-GitHub override map and set `REVIEWER_OVERRIDES_PATH` before calling `resolve_reviewer.py`; never commit Slack user IDs or mappings to this repository. The helper does nothing else — no public-email search, no cross-repo assumptions — so the requester tier resolves from a plain docs checkout when that private runtime context is available. Like `factory-resolve-reviewer`, it never guesses: an unavailable map or unresolved Slack id prints nothing and the chain moves to the next tier. + +The factory-level private override map owns requester identity mappings. This repository stores no real Slack IDs: keep `REQUESTER_SLACK_ID` as a runtime value and use a placeholder in examples and tests. Two details below are load-bearing, and getting either wrong reintroduces the silent drop this section exists to prevent: @@ -286,7 +292,9 @@ Two details below are load-bearing, and getting either wrong reintroduces the si ```bash PR=123 -FALLBACK_REVIEWER=dannyneira +REQUESTER_SLACK_ID="" # this run's requester Slack user id, when known +SECONDARY_FALLBACK_REVIEWER=hongyi-chen # HYC - confirmed second-tier fallback +FALLBACK_REVIEWER=dannyneira # final safety net; never remove # 1. Resolve the owning engineer(s). For missing_docs drift-watch runs, use the # ownership resolver with the source files behind the change; see the @@ -296,13 +304,26 @@ REVIEWERS=$(python3 .agents/skills/missing_docs/scripts/suggest_reviewers.py \ --reviewers-only --warp ../warp --warp-server ../warp-server \ warp:app/src/settings/ssh.rs < /dev/null) -# 2. Never let an empty resolution drop the request. Track that this was a -# fallback so step 6 does not report it as an owner who was requested. +# 2. No code-owner resolved. For a small, ambient/agent-generated PR with no +# clear owner - the common case here - prefer the run's requester over +# paging dannyneira: resolve their GitHub handle with this docs-local helper +# and the invoking factory's private REVIEWER_OVERRIDES_PATH map. The helper +# is callable from a plain docs checkout, unlike factory-agents' +# scripts/factory-resolve-reviewer, which lives in a separate repo that +# isn't checked out alongside this one. +# Fall to the secondary human fallback next, and only then to the final +# dannyneira safety net. Never let an empty resolution drop the request. +# Track that this was a fallback so step 6 does not report it as an owner +# who was requested. RESOLUTION_WAS_EMPTY=0 if [[ -z "$REVIEWERS" ]]; then - echo "warning: no owner resolved - falling back to $FALLBACK_REVIEWER" - REVIEWERS="$FALLBACK_REVIEWER" RESOLUTION_WAS_EMPTY=1 + if [[ -n "$REQUESTER_SLACK_ID" ]]; then + REVIEWERS=$(python3 .agents/skills/create_pr/resolve_reviewer.py --user "$REQUESTER_SLACK_ID") + fi + [[ -z "$REVIEWERS" ]] && REVIEWERS="$SECONDARY_FALLBACK_REVIEWER" + [[ -z "$REVIEWERS" ]] && REVIEWERS="$FALLBACK_REVIEWER" + echo "warning: no owner resolved - falling back to $REVIEWERS" fi # 3. Request each reviewer separately so one bad entry cannot drop the rest. @@ -350,24 +371,35 @@ has_reviewer() { # e.g. by a human) makes $REQUESTED non-empty even though the fallback was # never assigned, which would skip re-requesting it here and then have the # next step falsely report it as requested when it never landed. -if (( RESOLUTION_WAS_EMPTY )); then - if ! has_reviewer "$FALLBACK_REVIEWER"; then - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" || - echo "warning: fallback $FALLBACK_REVIEWER could not be requested" +request_fallback() { + local candidate="$1" + if ! has_reviewer "$candidate"; then + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$candidate" || + echo "warning: fallback $candidate could not be requested" REQUESTED=$(read_requested) fi +} + +if (( RESOLUTION_WAS_EMPTY )); then + request_fallback "$REVIEWERS" + if [[ "$REVIEWERS" != "$FALLBACK_REVIEWER" ]] && ! has_reviewer "$REVIEWERS"; then + # The settled-on fallback (requester tier or HYC) was rejected - a + # rejection at this tier must still reach the final dannyneira safety net + # rather than stopping here. + echo "warning: $REVIEWERS rejected - advancing to final fallback $FALLBACK_REVIEWER" + REVIEWERS="$FALLBACK_REVIEWER" + request_fallback "$REVIEWERS" + fi elif [[ -z "$REQUESTED" ]]; then - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" || - echo "warning: fallback $FALLBACK_REVIEWER could not be requested" - REQUESTED=$(read_requested) + request_fallback "$FALLBACK_REVIEWER" fi if [[ -z "$REQUESTED" ]]; then echo "ERROR: no reviewer is on PR $PR - not even the fallback landed" exit 1 fi -if (( RESOLUTION_WAS_EMPTY )) && ! has_reviewer "$FALLBACK_REVIEWER"; then - echo "ERROR: fallback $FALLBACK_REVIEWER could not be requested on PR $PR" \ +if (( RESOLUTION_WAS_EMPTY )) && ! has_reviewer "$REVIEWERS"; then + echo "ERROR: fallback $REVIEWERS could not be requested on PR $PR" \ "(existing reviewers: $REQUESTED); report this run as failed." exit 1 fi @@ -384,9 +416,10 @@ for R in "${WANT[@]}"; do done if (( RESOLUTION_WAS_EMPTY )); then - # Step 6 already guaranteed the fallback landed (or exited above), so this - # always reports a true outcome, not just "nothing resolved." - echo "note: no owner resolved for PR $PR; fallback $FALLBACK_REVIEWER requested" + # Step 6 already guaranteed the settled-on fallback landed (or exited + # above), so this always reports a true outcome, not just "nothing + # resolved." + echo "note: no owner resolved for PR $PR; fallback $REVIEWERS requested" elif (( ${#MISSING[@]} == ${#WANT[@]} )); then # Owners resolved and none of them are on the PR. It has a reviewer, but not # the right one, and that must not read as success. diff --git a/.agents/skills/create_pr/resolve_reviewer.py b/.agents/skills/create_pr/resolve_reviewer.py new file mode 100755 index 00000000..e17e882c --- /dev/null +++ b/.agents/skills/create_pr/resolve_reviewer.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +"""Resolve a task requester's Slack user id to a GitHub handle, for this repo. + +This docs-repo-local helper covers only the requester tier. Its private +Slack-to-GitHub mapping is supplied at runtime through +`REVIEWER_OVERRIDES_PATH`; it is intentionally not committed to this +repository. This keeps requester identity data in the factory-level private +override map while allowing a normal docs checkout to resolve a requester when +that map is mounted by the invoking environment. + +It intentionally does the bare minimum and nothing more — no public-email +search, no cross-repo assumptions, no guessing. A missing map or unresolved +Slack id prints nothing (exit 0) and the caller's chain moves to the next tier. + +Usage: + REVIEWER_OVERRIDES_PATH=/private/path/reviewer_overrides.json \ + python3 resolve_reviewer.py --user + +Prints the resolved GitHub handle to stdout, or nothing when unresolved. +""" +import argparse +import json +import os +import sys + + +def load_overrides(path): + """Return a {slack_id: github_handle} map from a private override map. + + Returns an empty map when its runtime path is absent, missing, malformed, + or has no usable entries — a missing override is not an error, it just + fails to resolve. + """ + if not path: + return {} + try: + with open(path, encoding="utf-8") as fh: + data = json.load(fh) + except (FileNotFoundError, OSError, json.JSONDecodeError): + return {} + users = data.get("users") if isinstance(data, dict) else None + if not isinstance(users, list): + return {} + indexed = {} + for user in users: + if not isinstance(user, dict): + continue + slack_id = str(user.get("slack_id", "")).strip() + github = str(user.get("github", "")).strip() + if slack_id and github: + indexed[slack_id] = github + return indexed + + +def main(argv=None): + parser = argparse.ArgumentParser( + prog="resolve_reviewer.py", + description="Resolve a Slack user id to a GitHub handle via a private override map.", + ) + parser.add_argument("--user", dest="user", help="Slack user id to resolve.") + parser.add_argument( + "--overrides", + help="Private override-map path; defaults to REVIEWER_OVERRIDES_PATH.", + ) + args = parser.parse_args(argv) + + if not args.user: + return 0 + handle = load_overrides( + args.overrides or os.environ.get("REVIEWER_OVERRIDES_PATH") + ).get(args.user) + if handle: + print(handle) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.agents/skills/create_pr/test_request_reviewers.py b/.agents/skills/create_pr/test_request_reviewers.py index ee93ec9d..18232c8e 100644 --- a/.agents/skills/create_pr/test_request_reviewers.py +++ b/.agents/skills/create_pr/test_request_reviewers.py @@ -1,9 +1,14 @@ #!/usr/bin/env python3 """Regression tests for the reviewer-request snippet in create_pr/SKILL.md. -The tests extract the documented bash snippet and run it against stubbed `gh` -and `suggest_reviewers.py` commands. This exercises the text users copy rather -than a paraphrased implementation. +The tests extract the documented bash snippet and run it against a stubbed +`gh` and `suggest_reviewers.py`. Most cases also stub the requester-tier +resolver to drive specific resolutions deterministically, but +`test_real_resolver_*` below runs the actual checked-in +`resolve_reviewer.py` against a private-map fixture, so the requester tier is +proven callable from a plain docs checkout without committing requester +identity data. This exercises the text users copy rather than a paraphrased +implementation. Run with: python3 .agents/skills/create_pr/test_request_reviewers.py """ @@ -11,6 +16,7 @@ import json import os import re +import shutil import subprocess import sys import tempfile @@ -56,22 +62,68 @@ sys.stdout.write(os.environ.get("STUB_REVIEWERS", "")) """ +REQUESTER_RESOLVER_STUB = """#!/usr/bin/env python3 +import os +import sys +sys.stdout.write(os.environ.get("STUB_REQUESTER_REVIEWER", "")) +""" + + +def extract_reviewer_snippet(requester_slack_id=None, secondary_fallback=None): + """Extract the bash fence whose first assignments identify the snippet. -def extract_reviewer_snippet(): - """Extract the bash fence whose first two assignments identify the snippet.""" + ``requester_slack_id`` / ``secondary_fallback``, when given, override the + documented placeholder values via targeted substitution so tests can + exercise the requester and secondary-fallback tiers without hand-copying + the script's logic. + """ text = SKILL.read_text(encoding="utf-8") match = re.search( - r"```bash\n(PR=123\nFALLBACK_REVIEWER=dannyneira\n.*?)(?=\n```)", + r"```bash\n(PR=123\nREQUESTER_SLACK_ID=.*?)(?=\n```)", text, re.DOTALL, ) if not match: raise AssertionError("reviewer-request snippet not found in SKILL.md") - return match.group(1) + snippet = match.group(1) + + if requester_slack_id is not None: + snippet, count = re.subn( + r'REQUESTER_SLACK_ID="[^"]*"', + 'REQUESTER_SLACK_ID="%s"' % requester_slack_id, + snippet, + count=1, + ) + if count != 1: + raise AssertionError("could not override REQUESTER_SLACK_ID in snippet") + + if secondary_fallback is not None: + snippet, count = re.subn( + r"SECONDARY_FALLBACK_REVIEWER=\S+", + "SECONDARY_FALLBACK_REVIEWER=%s" % secondary_fallback, + snippet, + count=1, + ) + if count != 1: + raise AssertionError( + "could not override SECONDARY_FALLBACK_REVIEWER in snippet" + ) + + return snippet class ReviewerSnippetTest(unittest.TestCase): - def run_snippet(self, *, initial=(), resolved="", reject=""): + def run_snippet( + self, + *, + initial=(), + resolved="", + reject="", + requester_slack_id=None, + requester_resolved="", + secondary_fallback=None, + use_real_requester_resolver=False, + ): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) bin_dir = root / "bin" @@ -87,6 +139,34 @@ def run_snippet(self, *, initial=(), resolved="", reject=""): resolver.write_text(RESOLVER_STUB, encoding="utf-8") resolver.chmod(0o755) + requester_resolver = ( + root / ".agents/skills/create_pr/resolve_reviewer.py" + ) + requester_resolver.parent.mkdir(parents=True) + if use_real_requester_resolver: + # Copy the actual checked-in resolver (not a stub), and mount + # a test-only private map as the invoking factory would. + shutil.copy(HERE / "resolve_reviewer.py", requester_resolver) + private_overrides = root / "private-reviewer-overrides.json" + private_overrides.write_text( + json.dumps( + { + "users": [ + { + "slack_id": "U_TEST_REAL_REQUESTER", + "github": "the-real-requester", + } + ] + } + ), + encoding="utf-8", + ) + else: + requester_resolver.write_text( + REQUESTER_RESOLVER_STUB, encoding="utf-8" + ) + requester_resolver.chmod(0o755) + state_file = root / "state.json" state_file.write_text(json.dumps(list(initial)), encoding="utf-8") calls_file = root / "calls.jsonl" @@ -100,10 +180,18 @@ def run_snippet(self, *, initial=(), resolved="", reject=""): "GH_STUB_CALLS": str(calls_file), "GH_STUB_REJECT": reject, "STUB_REVIEWERS": resolved, + "STUB_REQUESTER_REVIEWER": requester_resolved, + "REVIEWER_OVERRIDES_PATH": str( + root / "private-reviewer-overrides.json" + ), } ) + snippet = extract_reviewer_snippet( + requester_slack_id=requester_slack_id, + secondary_fallback=secondary_fallback, + ) result = subprocess.run( - ["bash", "-c", extract_reviewer_snippet()], + ["bash", "-c", snippet], cwd=root, env=env, capture_output=True, @@ -130,26 +218,89 @@ def test_resolved_owner_lands(self): self.assertEqual(state, ["alice"]) self.assertEqual(self.requested_reviewers(calls), ["alice"]) - def test_empty_resolution_requests_fallback(self): + def test_secondary_fallback_used_with_no_requester_context(self): + """The documented default (no REQUESTER_SLACK_ID) skips straight to HYC.""" result, state, calls = self.run_snippet() self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["hongyi-chen"]) + self.assertEqual(self.requested_reviewers(calls), ["hongyi-chen"]) + + def test_requester_resolves_before_secondary_and_final_fallback(self): + result, state, calls = self.run_snippet( + requester_slack_id="U_TEST", requester_resolved="the-requester" + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["the-requester"]) + self.assertEqual(self.requested_reviewers(calls), ["the-requester"]) + + def test_secondary_fallback_used_when_requester_unresolved(self): + result, state, calls = self.run_snippet(requester_slack_id="U_TEST") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["hongyi-chen"]) + self.assertEqual(self.requested_reviewers(calls), ["hongyi-chen"]) + + def test_final_fallback_still_reachable_when_secondary_unset(self): + """dannyneira remains the ultimate safety net if HYC is ever blanked.""" + result, state, calls = self.run_snippet(secondary_fallback="") + self.assertEqual(result.returncode, 0, result.stderr) self.assertEqual(state, ["dannyneira"]) self.assertEqual(self.requested_reviewers(calls), ["dannyneira"]) def test_unrelated_existing_reviewer_does_not_skip_fallback(self): result, state, calls = self.run_snippet(initial=["carol"]) self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("hongyi-chen", self.requested_reviewers(calls)) + self.assertEqual(set(state), {"carol", "hongyi-chen"}) + self.assertIn( + "no owner resolved for PR 123; fallback hongyi-chen requested", + result.stdout, + ) + + def test_real_resolver_resolves_seeded_requester(self): + """Runs the actual resolve_reviewer.py (not a stub) against a private-map fixture, + proving the requester tier resolves from a plain docs checkout without + a committed Slack-to-GitHub mapping.""" + result, state, calls = self.run_snippet( + requester_slack_id="U_TEST_REAL_REQUESTER", + use_real_requester_resolver=True, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["the-real-requester"]) + self.assertEqual(self.requested_reviewers(calls), ["the-real-requester"]) + + def test_real_resolver_falls_through_for_unknown_requester(self): + """An id absent from the private override map must fall through to the + secondary fallback rather than erroring or guessing.""" + result, state, calls = self.run_snippet( + requester_slack_id="U_NOT_IN_OVERRIDES", + use_real_requester_resolver=True, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["hongyi-chen"]) + + def test_hyc_rejection_falls_through_to_final_fallback(self): + """A HYC rejection must not stop the chain - dannyneira is attempted + next and its landing is confirmed via read-back, not assumed.""" + result, state, calls = self.run_snippet( + initial=["carol"], reject="hongyi-chen" + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("hongyi-chen", self.requested_reviewers(calls)) self.assertIn("dannyneira", self.requested_reviewers(calls)) self.assertEqual(set(state), {"carol", "dannyneira"}) self.assertIn( - "no owner resolved for PR 123; fallback dannyneira requested", + "hongyi-chen rejected - advancing to final fallback dannyneira", result.stdout, ) def test_unrelated_reviewer_does_not_mask_fallback_failure(self): + """When even the final dannyneira safety net is rejected, the run must + fail loudly rather than quietly accept the unrelated pre-existing + reviewer as if the fallback chain had succeeded.""" result, state, calls = self.run_snippet( - initial=["carol"], reject="dannyneira" + initial=["carol"], reject="hongyi-chen,dannyneira" ) + self.assertIn("hongyi-chen", self.requested_reviewers(calls)) self.assertIn("dannyneira", self.requested_reviewers(calls)) self.assertEqual(state, ["carol"]) self.assertNotEqual(result.returncode, 0) diff --git a/.github/workflows/release-docs-update.yml b/.github/workflows/release-docs-update.yml index c3ebc1cc..58f3cc52 100644 --- a/.github/workflows/release-docs-update.yml +++ b/.github/workflows/release-docs-update.yml @@ -239,11 +239,44 @@ jobs: fi done + # This scheduled/dispatched workflow has no run requester to prefer + # (unlike the ambient create_pr skill runs, it isn't tied to any + # particular person) — so it keeps a secondary human fallback + # (hongyi-chen, "HYC") ahead of the final dannyneira safety net + # instead. if [[ -z "$LAST_REVIEWER" ]]; then - echo "::warning::No recent reviewer found — using default reviewer dannyneira" - LAST_REVIEWER="dannyneira" + echo "::warning::No recent reviewer found — trying secondary fallback hongyi-chen" + LAST_REVIEWER="hongyi-chen" fi + # A helper for the reviewRequests read-back: `gh pr edit` can exit 0 + # while quietly failing to add a reviewer, so the read-back — not the + # exit code — decides whether the hard dannyneira fallback runs. + read_requested() { + gh pr view "$PR_NUMBER" --repo warpdotdev/docs \ + --json reviewRequests --jq '[.reviewRequests[] | .login // .slug // .name] | join(",")' + } + has_reviewer() { + local want target requested + want=$(printf '%s' "$1" | tr 'A-Z' 'a-z') + requested=$(read_requested) + IFS=',' read -ra _have <<< "$requested" + for target in "${_have[@]}"; do + [[ "$(printf '%s' "$target" | tr 'A-Z' 'a-z')" == "$want" ]] && return 0 + done + return 1 + } + echo "Assigning reviewer: $LAST_REVIEWER" gh pr edit "$PR_NUMBER" --add-reviewer "$LAST_REVIEWER" --repo warpdotdev/docs 2>&1 || \ - echo "::warning::Could not assign $LAST_REVIEWER as reviewer" + echo "::warning::gh pr edit exited nonzero for $LAST_REVIEWER — verifying via read-back" + + if ! has_reviewer "$LAST_REVIEWER"; then + echo "::warning::$LAST_REVIEWER is not on the reviewRequests read-back — falling back to dannyneira" + gh pr edit "$PR_NUMBER" --add-reviewer dannyneira --repo warpdotdev/docs 2>&1 || \ + echo "::warning::Could not assign dannyneira as reviewer" + if ! has_reviewer "dannyneira"; then + echo "::error::dannyneira is not on the reviewRequests read-back either — no reviewer could be confirmed on PR #$PR_NUMBER" + exit 1 + fi + fi diff --git a/.github/workflows/test_release_docs_reviewer.py b/.github/workflows/test_release_docs_reviewer.py new file mode 100644 index 00000000..84578417 --- /dev/null +++ b/.github/workflows/test_release_docs_reviewer.py @@ -0,0 +1,142 @@ +#!/usr/bin/env python3 +"""Regression tests for release-docs-update.yml reviewer assignment. + +The tests execute the workflow's exact final run block with stubbed `oz` and +`gh` commands. This specifically guards against GitHub silently dropping both +the selected reviewer and the final dannyneira fallback. + +Run with: python3 .github/workflows/test_release_docs_reviewer.py +""" + +import json +import os +import subprocess +import tempfile +import textwrap +import unittest +from pathlib import Path + + +WORKFLOW = Path(__file__).with_name("release-docs-update.yml") + +OZ_STUB = """#!/usr/bin/env python3 +print("PR: docs #123") +""" +GREP_STUB = """#!/bin/sh +cat >/dev/null +printf '123\\n' +""" + +GH_STUB = """#!/usr/bin/env python3 +import json +import os +import sys +from pathlib import Path + +state_file = Path(os.environ["GH_STUB_STATE"]) +calls_file = Path(os.environ["GH_STUB_CALLS"]) +silent_drops = set(filter(None, os.environ.get("GH_STUB_SILENT_DROPS", "").split(","))) +args = sys.argv[1:] + +with calls_file.open("a", encoding="utf-8") as stream: + stream.write(json.dumps(args) + "\\n") + +if args[:1] == ["api"]: + print("[]") + sys.exit(0) + +state = json.loads(state_file.read_text(encoding="utf-8")) +if args[:2] == ["pr", "edit"]: + reviewer = args[args.index("--add-reviewer") + 1] + if reviewer not in silent_drops and reviewer not in state: + state.append(reviewer) + state_file.write_text(json.dumps(state), encoding="utf-8") + sys.exit(0) +if args[:2] == ["pr", "view"]: + print(",".join(state)) + sys.exit(0) +sys.exit(1) +""" + + +def reviewer_assignment_script(): + """Extract and dedent the workflow's exact final reviewer-assignment run.""" + text = WORKFLOW.read_text(encoding="utf-8") + start = text.index(" # Get the PR number from the oz run") + return textwrap.dedent(text[start:]).replace( + "${{ steps.oz-dispatch.outputs.run_id }}", "test-run-id" + ) + + +class ReleaseDocsReviewerTest(unittest.TestCase): + def run_assignment(self, *, silent_drops=()): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + bin_dir = root / "bin" + bin_dir.mkdir() + for name, source in ( + ("oz", OZ_STUB), + ("grep", GREP_STUB), + ("gh", GH_STUB), + ): + command = bin_dir / name + command.write_text(source, encoding="utf-8") + command.chmod(0o755) + + state_file = root / "state.json" + calls_file = root / "calls.jsonl" + state_file.write_text("[]", encoding="utf-8") + calls_file.write_text("", encoding="utf-8") + env = os.environ.copy() + env.update( + { + "PATH": f"{bin_dir}{os.pathsep}{env['PATH']}", + "GH_STUB_STATE": str(state_file), + "GH_STUB_CALLS": str(calls_file), + "GH_STUB_SILENT_DROPS": ",".join(silent_drops), + } + ) + result = subprocess.run( + ["bash", "-c", reviewer_assignment_script()], + cwd=root, + env=env, + capture_output=True, + text=True, + ) + calls = [ + json.loads(line) + for line in calls_file.read_text(encoding="utf-8").splitlines() + ] + return result, calls + + @staticmethod + def requested_reviewers(calls): + return [ + call[call.index("--add-reviewer") + 1] + for call in calls + if call[:2] == ["pr", "edit"] + ] + + def test_fails_when_final_fallback_is_silently_dropped(self): + result, calls = self.run_assignment( + silent_drops=("hongyi-chen", "dannyneira") + ) + self.assertNotEqual(result.returncode, 0) + self.assertEqual( + self.requested_reviewers(calls), ["hongyi-chen", "dannyneira"] + ) + self.assertIn( + "::error::dannyneira is not on the reviewRequests read-back either", + result.stdout, + ) + + def test_succeeds_when_final_fallback_is_confirmed(self): + result, calls = self.run_assignment(silent_drops=("hongyi-chen",)) + self.assertEqual(result.returncode, 0, result.stdout) + self.assertEqual( + self.requested_reviewers(calls), ["hongyi-chen", "dannyneira"] + ) + + +if __name__ == "__main__": + unittest.main()