From a6ff0199b185474f9cbf0ffd4aabdd8ef1e09e8e Mon Sep 17 00:00:00 2001 From: "warp-agent-staging[bot]" <240773466+warp-agent-staging[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:32:40 +0000 Subject: [PATCH 1/4] docs: prefer requester and HYC over dannyneira for ambient PR reviewer fallback For small, ambient/agent-generated docs PRs with no clear code owner, the reviewer-request chain now tries the run's requester (Slack-first resolved, same as factory-github-ops) and a secondary human fallback (hongyi-chen, "HYC") before paging dannyneira as the final safety net. Applies the same priority tier to the release-docs-update.yml scheduled reviewer assignment, minus the requester tier (that workflow has no run requester to prefer). Co-Authored-By: Warp --- .agents/skills/create_pr/SKILL.md | 44 ++++--- .../create_pr/test_request_reviewers.py | 109 +++++++++++++++--- .github/workflows/release-docs-update.yml | 16 ++- 3 files changed, 135 insertions(+), 34 deletions(-) diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index d8fccd592..f068a35c9 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -277,7 +277,11 @@ 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 the same Slack-first way `factory-github-ops` resolves any requester (`scripts/factory-resolve-reviewer --user --repo warpdotdev/docs`, which checks `reviewer_overrides.json` first, then a public-email search); (3) a secondary human fallback, currently `hongyi-chen` ("HYC"); (4) `dannyneira`. This repo has no `reviewer_overrides.json` of its own — the factory-agents-level `scripts/reviewer_overrides.json` is the source of truth for step 2's override lookups. + +**TODO for a human:** the requester behind this chain's design (Slack id `U0A1Z732333`) did not resolve to a GitHub handle automatically (no public email and no override on file). Confirm their GitHub username and add it to `scripts/reviewer_overrides.json` (factory-agents-level) so future runs from them resolve step 2 automatically, then set `REQUESTER_SLACK_ID` below to exercise it end to end. Two details below are load-bearing, and getting either wrong reintroduces the silent drop this section exists to prevent: @@ -286,7 +290,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 +302,22 @@ 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 the same Slack-first way +# factory-github-ops resolves any requester. 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=$(scripts/factory-resolve-reviewer --user "$REQUESTER_SLACK_ID" --repo warpdotdev/docs) + 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. @@ -351,9 +366,9 @@ has_reviewer() { # 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" + if ! has_reviewer "$REVIEWERS"; then + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$REVIEWERS" || + echo "warning: fallback $REVIEWERS could not be requested" REQUESTED=$(read_requested) fi elif [[ -z "$REQUESTED" ]]; then @@ -366,8 +381,8 @@ 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 +399,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/test_request_reviewers.py b/.agents/skills/create_pr/test_request_reviewers.py index ee93ec9d1..ded409cf6 100644 --- a/.agents/skills/create_pr/test_request_reviewers.py +++ b/.agents/skills/create_pr/test_request_reviewers.py @@ -1,9 +1,9 @@ #!/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 stubbed `gh`, +`suggest_reviewers.py`, and `factory-resolve-reviewer` commands. This exercises +the text users copy rather than a paraphrased implementation. Run with: python3 .agents/skills/create_pr/test_request_reviewers.py """ @@ -56,22 +56,67 @@ 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, + ): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) bin_dir = root / "bin" @@ -87,6 +132,11 @@ def run_snippet(self, *, initial=(), resolved="", reject=""): resolver.write_text(RESOLVER_STUB, encoding="utf-8") resolver.chmod(0o755) + requester_resolver = root / "scripts/factory-resolve-reviewer" + requester_resolver.parent.mkdir(parents=True) + 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 +150,15 @@ 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, } ) + 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,31 +185,53 @@ 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("dannyneira", self.requested_reviewers(calls)) - self.assertEqual(set(state), {"carol", "dannyneira"}) + self.assertIn("hongyi-chen", self.requested_reviewers(calls)) + self.assertEqual(set(state), {"carol", "hongyi-chen"}) self.assertIn( - "no owner resolved for PR 123; fallback dannyneira requested", + "no owner resolved for PR 123; fallback hongyi-chen requested", result.stdout, ) def test_unrelated_reviewer_does_not_mask_fallback_failure(self): result, state, calls = self.run_snippet( - initial=["carol"], reject="dannyneira" + initial=["carol"], reject="hongyi-chen" ) - self.assertIn("dannyneira", self.requested_reviewers(calls)) + self.assertIn("hongyi-chen", self.requested_reviewers(calls)) self.assertEqual(state, ["carol"]) self.assertNotEqual(result.returncode, 0) self.assertIn( - "ERROR: fallback dannyneira could not be requested on PR 123", + "ERROR: fallback hongyi-chen could not be requested on PR 123", result.stdout, ) self.assertNotIn("note: no owner resolved", result.stdout) diff --git a/.github/workflows/release-docs-update.yml b/.github/workflows/release-docs-update.yml index c3ebc1cc0..578d6fe20 100644 --- a/.github/workflows/release-docs-update.yml +++ b/.github/workflows/release-docs-update.yml @@ -239,11 +239,19 @@ 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 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" + gh pr edit "$PR_NUMBER" --add-reviewer "$LAST_REVIEWER" --repo warpdotdev/docs 2>&1 || { + echo "::warning::Could not assign $LAST_REVIEWER as reviewer — 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" + } From ee93da8c6ecde21edede77d7d5f66df8cfdc9c99 Mon Sep 17 00:00:00 2001 From: "warp-agent-staging[bot]" <240773466+warp-agent-staging[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:41:16 +0000 Subject: [PATCH 2/4] docs: resolve JAS-4 requester's GitHub handle in the reviewer skill doc The requester (Slack id U0A1Z732333) confirmed their GitHub handle as rachaelrenk, now on file in factory-agents' scripts/reviewer_overrides.json. Drops the now-resolved TODO placeholder. Co-Authored-By: Warp --- .agents/skills/create_pr/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index f068a35c9..91a5038ab 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -281,7 +281,7 @@ A resolution failure must fall back, never no-op. When no owner resolves — the The full priority chain, in order: (1) the CODEOWNERS/git-blame owner from `suggest_reviewers.py`; (2) the run's requester, resolved the same Slack-first way `factory-github-ops` resolves any requester (`scripts/factory-resolve-reviewer --user --repo warpdotdev/docs`, which checks `reviewer_overrides.json` first, then a public-email search); (3) a secondary human fallback, currently `hongyi-chen` ("HYC"); (4) `dannyneira`. This repo has no `reviewer_overrides.json` of its own — the factory-agents-level `scripts/reviewer_overrides.json` is the source of truth for step 2's override lookups. -**TODO for a human:** the requester behind this chain's design (Slack id `U0A1Z732333`) did not resolve to a GitHub handle automatically (no public email and no override on file). Confirm their GitHub username and add it to `scripts/reviewer_overrides.json` (factory-agents-level) so future runs from them resolve step 2 automatically, then set `REQUESTER_SLACK_ID` below to exercise it end to end. +The requester behind this chain's design (Slack id `U0A1Z732333`, GitHub `rachaelrenk`) is now on file in `scripts/reviewer_overrides.json` (factory-agents-level), so setting `REQUESTER_SLACK_ID` below to their Slack id resolves step 2 directly instead of falling through to the secondary fallback. Two details below are load-bearing, and getting either wrong reintroduces the silent drop this section exists to prevent: From 400468ed89492f65d68bba6090a4763d1f9bc975 Mon Sep 17 00:00:00 2001 From: "warp-agent-staging[bot]" <240773466+warp-agent-staging[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:00:45 +0000 Subject: [PATCH 3/4] docs: fix reviewer-fallback rework findings (JAS-4) - Vendor a docs-repo-local resolve_reviewer.py + reviewer_overrides.json so the requester tier is actually callable from a plain docs checkout (factory-agents' scripts/factory-resolve-reviewer lives in a separate repo and is not checked out alongside warpdotdev/docs). - Fix the reviewer-request snippet so a HYC rejection advances to the final dannyneira fallback and verifies it lands, instead of stopping after the HYC attempt. - Add a reviewRequests read-back to the release-docs-update workflow's reviewer-assignment step so a silent gh pr edit no-op still triggers the dannyneira hard fallback. - Replace the test that asserted the old (incorrect) HYC-rejection behavior with cases proving the fall-through, plus real (non-stubbed) integration tests against the checked-in resolver + override map. --- .agents/skills/create_pr/SKILL.md | 46 +++++++---- .agents/skills/create_pr/resolve_reviewer.py | 75 ++++++++++++++++++ .../skills/create_pr/reviewer_overrides.json | 14 ++++ .../create_pr/test_request_reviewers.py | 77 +++++++++++++++++-- .github/workflows/release-docs-update.yml | 30 +++++++- 5 files changed, 217 insertions(+), 25 deletions(-) create mode 100755 .agents/skills/create_pr/resolve_reviewer.py create mode 100644 .agents/skills/create_pr/reviewer_overrides.json diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index 91a5038ab..ac7f1a887 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -279,9 +279,11 @@ So the mention stays, and a real request is added alongside it. **A PR is not co 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 the same Slack-first way `factory-github-ops` resolves any requester (`scripts/factory-resolve-reviewer --user --repo warpdotdev/docs`, which checks `reviewer_overrides.json` first, then a public-email search); (3) a secondary human fallback, currently `hongyi-chen` ("HYC"); (4) `dannyneira`. This repo has no `reviewer_overrides.json` of its own — the factory-agents-level `scripts/reviewer_overrides.json` is the source of truth for step 2's override lookups. +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 `; (3) a secondary human fallback, currently `hongyi-chen` ("HYC"); (4) `dannyneira`. -The requester behind this chain's design (Slack id `U0A1Z732333`, GitHub `rachaelrenk`) is now on file in `scripts/reviewer_overrides.json` (factory-agents-level), so setting `REQUESTER_SLACK_ID` below to their Slack id resolves step 2 directly instead of falling through to the secondary fallback. +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. `resolve_reviewer.py` reads the checked-in `reviewer_overrides.json` (sibling to it, in this same directory) and does nothing else — no public-email search, no cross-repo assumptions — so the requester tier actually resolves from a plain docs checkout. Like `factory-resolve-reviewer`, it never guesses: an unresolved Slack id prints nothing and the chain moves to the next tier. + +The requester behind this chain's design (Slack id `U0A1Z732333`, GitHub `rachaelrenk`) is on file in this repo's `.agents/skills/create_pr/reviewer_overrides.json`, alongside the secondary fallback `hongyi-chen`, so setting `REQUESTER_SLACK_ID` below to their Slack id resolves step 2 directly instead of falling through to the secondary fallback. Add new entries there directly — this file does not sync from `factory-agents`' broader `scripts/reviewer_overrides.json`, which covers the wider roster for the whole factory pipeline. Two details below are load-bearing, and getting either wrong reintroduces the silent drop this section exists to prevent: @@ -304,16 +306,19 @@ REVIEWERS=$(python3 .agents/skills/missing_docs/scripts/suggest_reviewers.py \ # 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 the same Slack-first way -# factory-github-ops resolves any requester. 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. +# paging dannyneira: resolve their GitHub handle via this repo's own +# checked-in override map (resolve_reviewer.py), which 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 RESOLUTION_WAS_EMPTY=1 if [[ -n "$REQUESTER_SLACK_ID" ]]; then - REVIEWERS=$(scripts/factory-resolve-reviewer --user "$REQUESTER_SLACK_ID" --repo warpdotdev/docs) + 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" @@ -365,16 +370,27 @@ 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 "$REVIEWERS"; then - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$REVIEWERS" || - echo "warning: fallback $REVIEWERS 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 diff --git a/.agents/skills/create_pr/resolve_reviewer.py b/.agents/skills/create_pr/resolve_reviewer.py new file mode 100755 index 000000000..aaefbf0d2 --- /dev/null +++ b/.agents/skills/create_pr/resolve_reviewer.py @@ -0,0 +1,75 @@ +#!/usr/bin/env python3 +"""Resolve a task requester's Slack user id to a GitHub handle, for this repo. + +This is the docs-repo-local counterpart to `factory-agents`' broader +`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 a call to it from this repo's `create_pr/SKILL.md` +reviewer-request chain fails in a real docs run and silently falls through to +the next tier. This script covers exactly the requester tier: a manual, +checked-in override map (`reviewer_overrides.json`, sibling to this script) +keyed by Slack user id. + +It intentionally does the bare minimum and nothing more — no public-email +search, no cross-repo assumptions, no guessing. An unresolved Slack id prints +nothing (exit 0) and the caller's chain moves to the next tier, matching the +"never guesses" contract of `factory-resolve-reviewer`. + +Usage: + python3 resolve_reviewer.py --user + +Prints the resolved GitHub handle to stdout, or nothing when unresolved. +""" +import argparse +import json +import os +import sys + +SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) +OVERRIDES_PATH = os.path.join(SCRIPT_DIR, "reviewer_overrides.json") + + +def load_overrides(path=OVERRIDES_PATH): + """Return a {slack_id: github_handle} map from reviewer_overrides.json. + + Returns an empty map when the file is absent, malformed, or has no usable + entries — a missing override is not an error, it just fails to resolve. + """ + 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 this repo's checked-in override map.", + ) + parser.add_argument("--user", dest="user", help="Slack user id to resolve.") + args = parser.parse_args(argv) + + if not args.user: + return 0 + + handle = load_overrides().get(args.user) + if handle: + print(handle) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.agents/skills/create_pr/reviewer_overrides.json b/.agents/skills/create_pr/reviewer_overrides.json new file mode 100644 index 000000000..16d591d54 --- /dev/null +++ b/.agents/skills/create_pr/reviewer_overrides.json @@ -0,0 +1,14 @@ +{ + "_comment": "Docs-repo-local manual overrides for the create_pr reviewer-request chain's requester tier (see SKILL.md, 'Request reviewers'). This is a minimal, docs-repo-local counterpart to factory-agents' scripts/reviewer_overrides.json, which covers the full factory roster but lives in a separate repo that isn't checked out alongside a normal warpdotdev/docs clone. Add new entries here directly when a run's requester needs to resolve from this repo; this file does not sync automatically from factory-agents. Keyed by Slack user id -> GitHub handle via resolve_reviewer.py.", + "users": [ + { + "label": "Rachael Renk", + "github": "rachaelrenk", + "slack_id": "U0A1Z732333" + }, + { + "label": "HYC (Hongyi Chen)", + "github": "hongyi-chen" + } + ] +} diff --git a/.agents/skills/create_pr/test_request_reviewers.py b/.agents/skills/create_pr/test_request_reviewers.py index ded409cf6..cc82ec6d0 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`, -`suggest_reviewers.py`, and `factory-resolve-reviewer` 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` + `reviewer_overrides.json` unmodified, so the +requester tier is proven callable from a plain docs checkout rather than +assumed from a stub. 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 @@ -116,6 +122,7 @@ def run_snippet( requester_slack_id=None, requester_resolved="", secondary_fallback=None, + use_real_requester_resolver=False, ): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp) @@ -132,9 +139,23 @@ def run_snippet( resolver.write_text(RESOLVER_STUB, encoding="utf-8") resolver.chmod(0o755) - requester_resolver = root / "scripts/factory-resolve-reviewer" + requester_resolver = ( + root / ".agents/skills/create_pr/resolve_reviewer.py" + ) requester_resolver.parent.mkdir(parents=True) - requester_resolver.write_text(REQUESTER_RESOLVER_STUB, encoding="utf-8") + if use_real_requester_resolver: + # Copy the actual checked-in resolver + override map (not a + # stub) so the test exercises the real requester tier exactly + # as a plain docs checkout would run it. + shutil.copy(HERE / "resolve_reviewer.py", requester_resolver) + shutil.copy( + HERE / "reviewer_overrides.json", + requester_resolver.parent / "reviewer_overrides.json", + ) + else: + requester_resolver.write_text( + REQUESTER_RESOLVER_STUB, encoding="utf-8" + ) requester_resolver.chmod(0o755) state_file = root / "state.json" @@ -223,15 +244,57 @@ def test_unrelated_existing_reviewer_does_not_skip_fallback(self): result.stdout, ) - def test_unrelated_reviewer_does_not_mask_fallback_failure(self): + def test_real_resolver_resolves_seeded_requester(self): + """Runs the actual resolve_reviewer.py + reviewer_overrides.json + checked into this repo (not a stub) against the requester seeded for + this chain's design, proving the requester tier genuinely resolves + from a plain docs checkout instead of always falling through.""" + result, state, calls = self.run_snippet( + requester_slack_id="U0A1Z732333", # rachaelrenk's seeded Slack id + use_real_requester_resolver=True, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["rachaelrenk"]) + self.assertEqual(self.requested_reviewers(calls), ["rachaelrenk"]) + + def test_real_resolver_falls_through_for_unknown_requester(self): + """An id absent from the real 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( + "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="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) self.assertIn( - "ERROR: fallback hongyi-chen could not be requested on PR 123", + "ERROR: fallback dannyneira could not be requested on PR 123", result.stdout, ) self.assertNotIn("note: no owner resolved", result.stdout) diff --git a/.github/workflows/release-docs-update.yml b/.github/workflows/release-docs-update.yml index 578d6fe20..3615b29fe 100644 --- a/.github/workflows/release-docs-update.yml +++ b/.github/workflows/release-docs-update.yml @@ -249,9 +249,33 @@ jobs: 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 — falling back to dannyneira" + gh pr edit "$PR_NUMBER" --add-reviewer "$LAST_REVIEWER" --repo warpdotdev/docs 2>&1 || \ + 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 "::warning::dannyneira is not on the reviewRequests read-back either — no reviewer could be confirmed on PR #$PR_NUMBER" + fi + fi From c04160e8f33322fc4b51bd67794be5449ddc9df3 Mon Sep 17 00:00:00 2001 From: Rachael Rose Renk <91027132+rachaelrenk@users.noreply.github.com> Date: Fri, 28 Aug 2026 09:16:36 -0600 Subject: [PATCH 4/4] docs: address reviewer fallback feedback Co-Authored-By: Warp --- .agents/skills/create_pr/SKILL.md | 15 +- .agents/skills/create_pr/resolve_reviewer.py | 47 +++--- .../skills/create_pr/reviewer_overrides.json | 14 -- .../create_pr/test_request_reviewers.py | 47 +++--- .github/workflows/release-docs-update.yml | 3 +- .../workflows/test_release_docs_reviewer.py | 142 ++++++++++++++++++ 6 files changed, 206 insertions(+), 62 deletions(-) delete mode 100644 .agents/skills/create_pr/reviewer_overrides.json create mode 100644 .github/workflows/test_release_docs_reviewer.py diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index ac7f1a887..63e558a8c 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -279,11 +279,11 @@ So the mention stays, and a real request is added alongside it. **A PR is not co 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 `; (3) a secondary human fallback, currently `hongyi-chen` ("HYC"); (4) `dannyneira`. +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. `resolve_reviewer.py` reads the checked-in `reviewer_overrides.json` (sibling to it, in this same directory) and does nothing else — no public-email search, no cross-repo assumptions — so the requester tier actually resolves from a plain docs checkout. Like `factory-resolve-reviewer`, it never guesses: an unresolved Slack id prints nothing and the chain moves to the next tier. +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 requester behind this chain's design (Slack id `U0A1Z732333`, GitHub `rachaelrenk`) is on file in this repo's `.agents/skills/create_pr/reviewer_overrides.json`, alongside the secondary fallback `hongyi-chen`, so setting `REQUESTER_SLACK_ID` below to their Slack id resolves step 2 directly instead of falling through to the secondary fallback. Add new entries there directly — this file does not sync from `factory-agents`' broader `scripts/reviewer_overrides.json`, which covers the wider roster for the whole factory pipeline. +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: @@ -306,10 +306,11 @@ REVIEWERS=$(python3 .agents/skills/missing_docs/scripts/suggest_reviewers.py \ # 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 via this repo's own -# checked-in override map (resolve_reviewer.py), which 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). +# 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 diff --git a/.agents/skills/create_pr/resolve_reviewer.py b/.agents/skills/create_pr/resolve_reviewer.py index aaefbf0d2..e17e882ce 100755 --- a/.agents/skills/create_pr/resolve_reviewer.py +++ b/.agents/skills/create_pr/resolve_reviewer.py @@ -1,22 +1,20 @@ #!/usr/bin/env python3 """Resolve a task requester's Slack user id to a GitHub handle, for this repo. -This is the docs-repo-local counterpart to `factory-agents`' broader -`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 a call to it from this repo's `create_pr/SKILL.md` -reviewer-request chain fails in a real docs run and silently falls through to -the next tier. This script covers exactly the requester tier: a manual, -checked-in override map (`reviewer_overrides.json`, sibling to this script) -keyed by Slack user id. +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. An unresolved Slack id prints -nothing (exit 0) and the caller's chain moves to the next tier, matching the -"never guesses" contract of `factory-resolve-reviewer`. +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: - python3 resolve_reviewer.py --user + REVIEWER_OVERRIDES_PATH=/private/path/reviewer_overrides.json \ + python3 resolve_reviewer.py --user Prints the resolved GitHub handle to stdout, or nothing when unresolved. """ @@ -25,16 +23,16 @@ import os import sys -SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) -OVERRIDES_PATH = os.path.join(SCRIPT_DIR, "reviewer_overrides.json") +def load_overrides(path): + """Return a {slack_id: github_handle} map from a private override map. -def load_overrides(path=OVERRIDES_PATH): - """Return a {slack_id: github_handle} map from reviewer_overrides.json. - - Returns an empty map when the file is absent, malformed, or has no usable - entries — a missing override is not an error, it just fails to resolve. + 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) @@ -57,15 +55,20 @@ def load_overrides(path=OVERRIDES_PATH): def main(argv=None): parser = argparse.ArgumentParser( prog="resolve_reviewer.py", - description="Resolve a Slack user id to a GitHub handle via this repo's checked-in override map.", + 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().get(args.user) + handle = load_overrides( + args.overrides or os.environ.get("REVIEWER_OVERRIDES_PATH") + ).get(args.user) if handle: print(handle) return 0 diff --git a/.agents/skills/create_pr/reviewer_overrides.json b/.agents/skills/create_pr/reviewer_overrides.json deleted file mode 100644 index 16d591d54..000000000 --- a/.agents/skills/create_pr/reviewer_overrides.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "_comment": "Docs-repo-local manual overrides for the create_pr reviewer-request chain's requester tier (see SKILL.md, 'Request reviewers'). This is a minimal, docs-repo-local counterpart to factory-agents' scripts/reviewer_overrides.json, which covers the full factory roster but lives in a separate repo that isn't checked out alongside a normal warpdotdev/docs clone. Add new entries here directly when a run's requester needs to resolve from this repo; this file does not sync automatically from factory-agents. Keyed by Slack user id -> GitHub handle via resolve_reviewer.py.", - "users": [ - { - "label": "Rachael Renk", - "github": "rachaelrenk", - "slack_id": "U0A1Z732333" - }, - { - "label": "HYC (Hongyi Chen)", - "github": "hongyi-chen" - } - ] -} diff --git a/.agents/skills/create_pr/test_request_reviewers.py b/.agents/skills/create_pr/test_request_reviewers.py index cc82ec6d0..18232c8e9 100644 --- a/.agents/skills/create_pr/test_request_reviewers.py +++ b/.agents/skills/create_pr/test_request_reviewers.py @@ -5,10 +5,10 @@ `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` + `reviewer_overrides.json` unmodified, so the -requester tier is proven callable from a plain docs checkout rather than -assumed from a stub. This exercises the text users copy rather than a -paraphrased implementation. +`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 """ @@ -144,13 +144,22 @@ def run_snippet( ) requester_resolver.parent.mkdir(parents=True) if use_real_requester_resolver: - # Copy the actual checked-in resolver + override map (not a - # stub) so the test exercises the real requester tier exactly - # as a plain docs checkout would run it. + # 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) - shutil.copy( - HERE / "reviewer_overrides.json", - requester_resolver.parent / "reviewer_overrides.json", + 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( @@ -172,6 +181,9 @@ def run_snippet( "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( @@ -245,20 +257,19 @@ def test_unrelated_existing_reviewer_does_not_skip_fallback(self): ) def test_real_resolver_resolves_seeded_requester(self): - """Runs the actual resolve_reviewer.py + reviewer_overrides.json - checked into this repo (not a stub) against the requester seeded for - this chain's design, proving the requester tier genuinely resolves - from a plain docs checkout instead of always falling through.""" + """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="U0A1Z732333", # rachaelrenk's seeded Slack id + requester_slack_id="U_TEST_REAL_REQUESTER", use_real_requester_resolver=True, ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(state, ["rachaelrenk"]) - self.assertEqual(self.requested_reviewers(calls), ["rachaelrenk"]) + 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 real override map must fall through to the + """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", diff --git a/.github/workflows/release-docs-update.yml b/.github/workflows/release-docs-update.yml index 3615b29fe..58f3cc520 100644 --- a/.github/workflows/release-docs-update.yml +++ b/.github/workflows/release-docs-update.yml @@ -276,6 +276,7 @@ jobs: 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 "::warning::dannyneira is not on the reviewRequests read-back either — no reviewer could be confirmed on PR #$PR_NUMBER" + 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 000000000..845784171 --- /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()