docs: request reviewers for real and lead ambient PRs with a feature summary - #594
Conversation
…summary Two fixes to the ambient new-feature docs pipeline (GROW-6093). 1. Actually request reviewers. The drafted PR only named reviewers in prose, which puts nothing in GitHub's review queue: docs #414, #415, #416 and #417 all named reviewers in the body and received zero reviews, three with an empty requested-reviewers list. Wire a required `gh pr edit --add-reviewer` step into missing_docs drift-watch step 7 and into the create_pr skill, with the `dannyneira` fallback that release-docs-update.yml already uses, plus a verification read-back so a silently skipped assignment is caught. The prose /cc mention stays. suggest_reviewers.py gains `--reviewers-only` so the step can consume the resolved set without scraping the human-readable table. 2. Lead the PR body with a feature summary. Drafting PRs must open with `## What this feature does`: plain language, what the feature does for the user, ending with the shipped-in version and date read from check_new_release.py --json. Budget 75 words. check_pr_body.py gains `--require-lead-section`, asserting the heading is present once, is the first heading, is non-empty, and is within budget. Co-Authored-By: Warp <agent@warp.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This PR was generated with Warp. Comment |
… wiring Blocking fix. The reviewer verification was emptiness-only, so the owning engineer could be dropped silently — the exact bug this PR exists to fix. `gh pr edit --add-reviewer a,b,c` is one atomic mutation, so a single unassignable entry rejected the whole list and the `||` then replaced every resolved owner with the fallback; a non-empty readback still passed. This is live: `warpdotdev/oss-maintainers` is the root-rule owner in the warp client repo and appears in most resolutions, but `/repos/warpdotdev/docs/teams` is empty, so it cannot be requested here. Now each reviewer is requested in its own call and the readback is compared against the resolved set, with partial results reported. Also fixed the readback jq: the old `[.reviewRequests[].login // .reviewRequests[].name]` silently drops teams from a mixed list (verified). Also: - check_lead_section now asserts the summary is the first *content*, not just the first heading. A body opening with unheaded spec/workflow/run-ID preamble previously exited 0, which is the shape the check exists to stop. - _iter_non_code_lines skips HTML comments, so a `##` inside a multi-line comment no longer displaces the lead section — same class already handled for code fences. - Wired test_check_new_release.py into CI. The earlier deferral was wrong: #586 does not touch ci.yml and this PR already edits it, while missing_docs/SKILL.md advertises the test as covered. - suggest_reviewers.py routes resolution diagnostics to stderr under --reviewers-only, so a fallback leaves a trace without polluting stdout. - Removed the duplicated reviewer snippet from missing_docs; create_pr holds the canonical copy. The copies had already diverged, and the missing_docs one used `[[ -z ... ]] && ...`, which returns 1 and would abort a `set -e` scheduled run. - Backticked the date in the worked example; marked the drafting-only lines in the copy-paste heredoc. - Tests locking in first-content, HTML-comment banners, multi-line comments, CRLF bodies, and the stderr diagnostics. Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Overview
Adds real GitHub review requests and a required feature-summary lead section to ambient-drafted docs PRs, stacked on #586. The implementation is sound and an adversarial pass found nothing further to fix in it — what remains are four decisions that need a human, so this is posted as findings rather than as an approval or a rejection.
Concerns
warpdotdev/oss-maintainersowns the root rule inwarp/.github/STAKEHOLDERSbut has no access towarpdotdev/docs(/repos/warpdotdev/docs/teamsreturns zero entries), so it lands in nearly every reviewer resolution and can never be requested here. The PR is now resilient to this — it requests reviewers one at a time and keeps the humans — but the root cause remains, and for any path that resolves to the team alone the pipeline will always fall back todannyneira. Either grant the team read on this repo, or filter teams out of docs reviewer resolution.- The one genuinely automated reviewer assignment is untouched:
.github/workflows/release-docs-update.yml:248-249still ends in|| echo "::warning::Could not assign …"with no read-back. That is the same swallow-and-continue pattern this PR exists to eliminate, in the only non-agent code path. Deliberate, or an oversight worth folding in? - "Required" here means instruction, not enforcement. CI runs only the unit tests of
check_pr_body.py; nothing inspects a real PR body or a real requested-reviewers list, and change 1 is skill prose plus a bash snippet an agent may decline to run. For a skill-driven repo that is arguably the right mechanism — the PR body now says so plainly rather than implying machine enforcement — but it is worth confirming that is the intended ceiling. - Should the auto-requested review block merge? Requesting the reviewer is in this PR; branch protection is not. This is the open question from GROW-6093, restated here because it determines whether the change actually closes the "zero reviews on four docs PRs" gap or merely makes the request visible.
- Note, untouched upstream data:
danielpenginwarp/.github/STAKEHOLDERSresolves to a real but unrelated GitHub user who is not a docs collaborator; the actual docs collaborator isdanielpeng2. It resolves cleanly and then 422s at request time — a live instance of the failure path above. Belongs to the warp repo owners, not this PR.
Verdict
Checks: build pass, tests pass, CI green, visual proof n/a
Found: 0 critical, 0 important, 4 questions, 1 note
|
Hand-off note for @rachaelrenk, since this came out of GROW-6093. Merge order: #586 first. The dependency is functional, not just conflict avoidance — One defect was found and fixed during review: the first revision verified the reviewer request by asking only whether the result was non-empty. Against the real Six broader design questions from this issue are waiting on your call at https://linear.app/warpdotdev/issue/GROW-6093/iterate-on-docs-pipeline-system-design — each has a recommended default, so "yes to all" is a valid answer. Two of them (whether the auto-requested review blocks merge, and the |



Two changes to the ambient new-feature docs pipeline, both from GROW-6093.
Base branch
This targets
rrenk/release-docs-gate(the head branch of #586), notmain.This is a functional dependency, not just merge-conflict avoidance. Change 2 tells drafters to read the shipped-in version and date from
check_new_release.py, and that script does not exist onmain— #586 adds it. If #586 never merges, the documented command breaks. This PR should not be retargeted atmain; it should follow #586 or be reworked.The conflict avoidance is real too: #586 modifies both
.agents/skills/create_pr/SKILL.mdand.agents/skills/missing_docs/SKILL.md, and the new lead section has to sit above the## Content design plansection #586 adds to the create_pr body template.Nothing in #586 is modified by this PR.
Change 1: actually request reviewers
The pipeline named reviewers in prose and stopped there. A
/cc @engineerline puts nothing in GitHub's review queue — no requested reviewer on the PR, no entry in that engineer's "Review requested" filter.The evidence, re-confirmed against the API while writing this:
hongyi-chenAll four named reviewers in the body. Three never requested anyone; #416's single reviewer was added by hand.
So the review request becomes a real one, and mandatory:
.agents/skills/create_pr/SKILL.md— a new "Request reviewers (required)" section holding the canonical snippet, referenced from the create-a-PR steps and from "After Opening the PR"..agents/skills/missing_docs/SKILL.md— drift-watch step 7 is now "Route reviewers and request the review (required, not advisory)" and points at that canonical snippet rather than duplicating it. The "Reviewer routing" section is updated to match.ghcall per reviewer.gh pr edit --add-reviewer a,b,cis a single atomic mutation, so one unassignable entry rejects the whole list. That matters here:warpdotdev/oss-maintainersis the root-rule owner in the warp client repo and lands in most resolutions, but/repos/warpdotdev/docs/teamsis empty, so it cannot be requested on this repo. Requesting one at a time keeps the real owners.dannyneira— the same fallback.github/workflows/release-docs-update.ymluses in its "Assign last docs PR reviewer" step. An individual unresolved path stays non-fatal; an empty result is not./cc @engineermention stays. This adds the real request, it does not replace the mention.suggest_reviewers.pygains one additive flag,--reviewers-only, which prints just the comma-joined--add-reviewerargument on stdout (and nothing at all when nothing resolved), with resolution diagnostics on stderr so a fallback still leaves a trace in the run log. Without it the required step would have to scrape a human-readable table to find the reviewers.The resolution logic is untouched, and #586's own unmodified
test_suggest_reviewers.pystill passes against the changed script (15/15, re-verified). The default output path is not byte-identical, though: in the no-owners-resolved case it now prints three advisory lines pointing at the fallback.Change 2: feature summary at the top of the PR body
Ambient PR bodies opened with pipeline bookkeeping — which spec, which workflow, which run. A reviewing engineer had to read past all of it to reach what the feature actually does.
Drafting PRs now open with a
## What this feature doessection: plain language, what the feature does for the user, ending with the shipped-in fact.check_new_release.py --json(current_version/release_date) — the accessor docs: gate the new feature docs pipeline on worthiness and trigger it from releases #586 already adds — rather than a second way to read a release. There is no trustworthy source for a target ship date, and a forecast in a merged body ages into a false claim..agents/skills/create_pr/check_pr_body.pygains--require-lead-section HEADING, asserting the heading appears exactly once, is the first content in the body, has prose under it, and is within budget. Checking position against content rather than headings is the point: a body can open with four unheaded lines of spec/workflow/run-ID preamble and still have the summary as its first heading, which is exactly the shape being fixed. The flag is opt-in, so non-drafting PRs are unaffected.Validation
Everything below was actually run.
Unit tests, all green:
create_pr/test_check_pr_body.py— 20 tests, OK. New file;check_pr_body.pyhad no tests. Covers missing / not-first / empty / over-budget / duplicate lead sections, the exact-budget boundary, unheaded preamble above the summary, HTML-comment banners, multi-line comments containing a##line, a code fence above the summary, CRLF bodies (both passing and failing), fenced headings,main()exit codes, and the pre-existing backtick and duplicate-heading checks.missing_docs/scripts/test_suggest_reviewers.py— 18 tests, OK (15 pre-existing, plus 3 new ones for--reviewers-only: exact single-line output, empty stdout with a diagnostic on stderr when nothing resolves, and stdout staying clean while diagnosing a partial resolution).missing_docs/scripts/test_audit_docs.py— 12 tests, OK (7 skipped, because the warp / warp-server checkouts aren't present; same as in docs CI).missing_docs/scripts/test_check_new_release.py— 17 tests, OK (from docs: gate the new feature docs pipeline on worthiness and trigger it from releases #586, and now actually wired into CI — see the note at the end).Beyond the unit tests:
kevinyang372,harryalbert,warpdotdev/oss-maintainersagainst a stubbedghthat rejects the team: both human owners are kept, the team failure is reported, and the run warns "requested 2/3 resolved reviewers". The previous snippet would have replaced all three withdannyneiraand passed its own check.test_suggest_reviewers.py, taken unmodified from the base branch, passes 15/15 against the changed script.check_links.py --internal-only) — 3852 internal links, 0 broken.check_new_release.py --jsonreturnedcurrent_version: v0.2026.08.19.08.15.stable_01,release_date: 2026-08-19T22:28:46Z, so the documented extraction works against the real endpoint.py_compileon all four changed Python files, and the body checker run against this description.Not run, and why:
npm run buildandtrunk check— no files undersrc/content/docs/are touched, so there is no site content to build or lint.style_lint.pyscanssrc/content/docs/only and has nothing to say about.agents/.CI now runs both
test_check_pr_body.pyandtest_check_new_release.pyalongside the existing skill-script tests.This PR also dogfoods change 1:
rachaelrenkwas added throughgh pr edit --add-reviewerand the read-back confirmed a real requested reviewer, not just a prose mention.What "required" actually means here
Worth being precise, because the language above could read stronger than the mechanism.
This is a skill-driven repo, so both changes are instructions to an agent plus tooling that agent can run. CI runs the unit tests of
check_pr_body.py; nothing in CI inspects a real PR body or asserts that a real PR has a requested reviewer. An agent that skips the snippet still opens a PR. "Required" means the skill states it as non-optional and gives a command that fails loudly when the contract is broken — not that a gate blocks the PR.That is the right mechanism for this repo, and closing the gap would mean a CI job that inspects PR metadata, which is a separate decision. Flagging it so the enforcement is not overread.
Out of scope / open questions
Tracked on GROW-6093; deliberately not addressed here.
scan-new-specs— skills: retire scan-new-specs and remove write-feature-docs ambient mode common-skills#77 owns it.sK1Lsz3jJMEhNPdZMX0DjF, and anyDOCS:PR marker.One more thing found while validating, not fixed here:
check_pr_body.py's repetition-loop detector has a false-positive mode on repeated long file paths. It fired on an earlier draft of this very description, where four validation bullets each began with the same.agents/skills/missing_docs/scripts/prefix — the same class of false positive the script already strips URLs to avoid. I reworded the bullets rather than relax the threshold, since loosening a corruption check to make my own PR pass is the wrong trade. The detector could reasonably strip repo-relative paths the way it strips URLs; worth a follow-up.Revision 2
Addressing review feedback. One blocking defect and seven smaller fixes.
Blocking — the reviewer verification could silently drop the owning engineer, which is the exact bug this PR exists to fix. The first version sent one comma-joined
ghcall, so a single unassignable entry rejected the whole list; the||then replaced every resolved owner with the fallback, and an emptiness-only read-back passed anyway. Fixed by requesting one reviewer per call and comparing the read-back against the resolved set, as described under Change 1. The read-back jq was wrong too —[.reviewRequests[].login // .reviewRequests[].name]silently drops teams from a mixed list (confirmed:[User lucie, Team oss-maintainers]came back as justlucie); it now uses[.reviewRequests[] | .login // .slug // .name].Also in this revision:
_iter_non_code_linesskips HTML comments, so a##inside a multi-line comment no longer displaces the lead section — same class already handled for code fences.test_check_new_release.pyis wired into CI after all. The earlier deferral was wrong: docs: gate the new feature docs pipeline on worthiness and trigger it from releases #586 does not touchci.ymland this PR already edits it, whilemissing_docs/SKILL.mdadvertised the test as covered.--reviewers-onlysends resolution diagnostics to stderr, so a fallback leaves a trace without polluting the captured list.create_prholds the canonical copy andmissing_docsreferences it. The two copies had already diverged, and themissing_docsone used[[ -z ... ]] && ..., which returns 1 and would abort aset -escheduled run.I agreed with every finding; none needed pushback. I re-verified the three load-bearing claims independently before implementing (the jq drop, the empty team list, and the prose-preamble body exiting 0) rather than taking them on trust.