docs: prefer requester and HYC over dannyneira for ambient PR reviewer fallback - #655
docs: prefer requester and HYC over dannyneira for ambient PR reviewer fallback#655warp-agent-staging[bot] wants to merge 4 commits into
Conversation
…r 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 <agent@warp.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@warp-agent-staging[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR updates the docs create_pr reviewer-request fallback and the release-docs workflow to prefer the requester or HYC before dannyneira, plus regression tests for the copied shell snippet.
Concerns
- The requester resolver is documented as a docs-repo-relative command even though the resolver and overrides live outside this repo, so the requester tier will silently fall through where that script is unavailable.
- The
create_prsnippet never tries the finaldannyneirasafety net when the secondary fallback cannot be requested; it retries the same secondary reviewer and fails. - The new tests encode that incorrect failure behavior instead of covering HYC rejection falling through to the final fallback.
Security
- The skill text commits a real requester Slack ID into repository documentation; keep that mapping in the private override source or use a placeholder.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| 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 <requester_slack_id> --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. |
There was a problem hiding this comment.
| 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) |
There was a problem hiding this comment.
scripts/factory-resolve-reviewer from the docs repo, but the resolver and overrides live at the factory-agents level and this repo has no local copy. In a real create_pr run with REQUESTER_SLACK_ID set, this will fail and silently fall through to HYC instead of preferring the requester.
| 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" || |
There was a problem hiding this comment.
hongyi-chen cannot be requested, this retries the same $REVIEWERS value and then errors instead of trying $FALLBACK_REVIEWER. That means the documented final dannyneira safety net is not actually reached for the secondary-fallback failure case.
| 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" |
There was a problem hiding this comment.
hongyi-chen should verify that the snippet requests dannyneira next, not that the run fails before trying the final safety net.
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 <agent@warp.dev>
- 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.
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
The requester resolver and documented fallback chain now meet the prior review findings, and the current PR still requests rachaelrenk. However, the release workflow's final safety-net failure remains non-fatal, so it does not uphold the requirement that a reviewer be confirmed before the workflow reports success.
Verdict
Found: 0 critical, 1 important, 0 suggestions, 0 nits
Rejected — human decision required because the automated rework budget is exhausted.
| 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" |
There was a problem hiding this comment.
dannyneira did not land, but this branch only logs a warning and then the step exits successfully. If GitHub silently omits both the intended reviewer and the final fallback, the release workflow still succeeds and leaves the generated PR without a confirmed reviewer—the silent-drop case this change is intended to prevent. Fail the step after this read-back fails (and add a regression test for that branch) so the run is surfaced as failed instead of reporting a successful release update.
There was a problem hiding this comment.
We still get alerted via Slack that there is a PR so it's clear that it needs reviewed/approved. I think that's "good enough" in this situation. If anything, we can add "ready for review" to the PR link in the Slack message.
Co-Authored-By: Warp <agent@warp.dev>
1 participant
Summary
For small, ambient/agent-generated docs PRs, the reviewer-request fallback in
create_pr/SKILL.mdalways landed ondannyneirawheneversuggest_reviewers.pyfound no CODEOWNERS/git-blame owner — the common case for these runs (e.g. thedocs-seo-auditPR #652). Per the reporting Slack thread, the default should instead prefer the run's requester, then a secondary human fallback, before pagingdannyneira.Changes
.agents/skills/create_pr/SKILL.md.agents/skills/create_pr/resolve_reviewer.py, (3) a secondary human fallback (hongyi-chen, "HYC"), (4)dannyneiraas the final safety net — never removed.U0A1Z732333) resolves to GitHubrachaelrenkvia this repo's own checked-inreviewer_overrides.json— the reviewer below reflects that..agents/skills/create_pr/test_request_reviewers.pydannyneirasafety net remaining reachable, and the existing partial-resolution/rejection edge cases (renamed to referencehongyi-chenwhere the default fallback target changed)..github/workflows/release-docs-update.ymlhongyi-chenbeforedannyneira) to the "Assign last docs PR reviewer" step. This scheduled/dispatched workflow has no run requester to prefer (it isn't tied to any particular person the way an ambientcreate_prskill run is), so only the HYC-before-dannyneira tier applies here;dannyneiraremains the final safety net if both the last-reviewer heuristic andhongyi-chenare unavailable.Verification
Ran the regression suite, which extracts and executes the documented bash snippet against a stubbed
ghandsuggest_reviewers.py(plus, for two cases, the real checked-inresolve_reviewer.py+reviewer_overrides.json— no stub standing in for that call):All 10 tests pass, including the new
test_hyc_rejection_falls_through_to_final_fallback, the correctedtest_unrelated_reviewer_does_not_mask_fallback_failure(both fallbacks rejected → hard failure), andtest_real_resolver_resolves_seeded_requester/test_real_resolver_falls_through_for_unknown_requesteragainst the real resolver.Also ran
python3 .agents/skills/create_pr/resolve_reviewer.py --user U0A1Z732333directly, confirming it resolves torachaelrenk, and validated the release-docs-update workflow's read-back/fallback logic against a stubbedghreproducing a HYC rejection, confirming it now falls through todannyneira.trunkisn't vendored in this sandbox sotrunk check/trunk fmtcould not be run here.Re-ran the final mergeability gate after pushing:
mergeable: MERGEABLE, basemaincurrent,rachaelrenkstill onreviewRequests.Unverified claims
None — this is a skill-doc and CI-workflow change with no new UI, CLI flag, or product-facing claim to verify against source.
Additional context
scripts/reviewer_overrides.json(addinghongyi-chen/ "HYC" andrachaelrenkentries) is being opened separately againstwarpdotdev/factory-agents, since that file covers the wider roster for the whole factory pipeline and is independent of this repo's own override map.Rework changes
.agents/skills/create_pr/SKILL.md:315(the documentedscripts/factory-resolve-revieweris not callable from a normal docs checkout) — Implemented. That script lives in the separatefactory-agentsrepo, which isn't checked out alongside a plainwarpdotdev/docsclone, so the requester tier always failed and silently fell through to HYC. Vendored a docs-repo-local resolver (new file, same directory as this SKILL.md:resolve_reviewer.py) and a seeded override map (reviewer_overrides.json, with therachaelrenkandhongyi-chenentries), updated the SKILL.md snippet and prose to call it, and added tests that run the real (non-stubbed) resolver + override file rather than a temp-file stand-in..agents/skills/create_pr/SKILL.md:375(a HYC rejection must still reachdannyneira) — Implemented. The empty-resolution branch only retried the settled-on fallback (the requester tier's result, or HYC) and stopped there if it was rejected, never advancing todannyneira. Refactored that branch into a reusablerequest_fallbackhelper and added a follow-up check that advancesREVIEWERStodannyneiraand re-verifies it via read-back whenever the prior tier was rejected.gh pr editno-op isn't caught) — Implemented, in the same workflow file listed above. Added areviewRequestsread-back (has_reviewer) after thegh pr editcall in the "Assign last docs PR reviewer" step; when the intended reviewer isn't actually present, it now requests and re-verifiesdannyneiraas a hard fallback, regardless ofgh's exit code.test_unrelated_reviewer_does_not_mask_fallback_failurewithtest_hyc_rejection_falls_through_to_final_fallback(assertsdannyneirais requested and confirmed present after a HYC rejection) and repurposed the old test name to cover the case where both HYC anddannyneiraare rejected (a genuine hard failure). Also added two tests that exercise the actual checked-in resolver + override map (not stubs).Originating thread: https://warpdev.slack.com/archives/C09BVK0PL3Y/p1787900556500469
Co-Authored-By: Warp agent@warp.dev