missing_docs: make reviewer requests real, and test the PR body checker - #619
missing_docs: make reviewer requests real, and test the PR body checker#619rachaelrenk wants to merge 9 commits into
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>
… 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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 repository's PR creation and missing-docs automation guidance, adds a lead-section check to check_pr_body.py, extends reviewer resolution output with --reviewers-only, and wires the new stdlib tests into CI. The affected tests pass locally after applying the attached diff, but the reviewer-request snippet still lets the fallback reviewer mask a dropped resolved owner.
Concerns
- The reviewer verification snippet counts
dannyneiraas if it were one of the resolved owners after fallback, so a run can pass with only the fallback requested while the actual owner was never added.
Verdict
Found: 0 critical, 1 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
| # 4. If nothing at all landed, fall back rather than ship an unreviewed PR. | ||
| if (( ${#GOT[@]} == 0 )); then | ||
| gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" && | ||
| GOT+=("$FALLBACK_REVIEWER") |
There was a problem hiding this comment.
GOT makes the later count and non-empty read-back checks pass when every resolved owner failed but dannyneira landed, so the snippet still treats a fallback-only request as complete instead of surfacing that the actual owner was dropped.
There was a problem hiding this comment.
Good catch, and it reproduces. Fixed in b5a9aefe.
I extracted the snippet straight out of SKILL.md and ran it against a stubbed gh so the documented text is what got tested, not a paraphrase. With resolved owners alice and bob both rejected and the fallback accepted:
before: warning: requested 1/2 resolved reviewers
Requested reviewers: dannyneira
exit 0
after: ERROR: none of the 2 resolved owners could be requested on PR 123
(wanted: alice bob); only the fallback is assigned. Report this run as failed.
exit 1
The 1/2 is the tell — dannyneira was being counted as one of the resolved owners, so the count check passed while neither real owner had been requested. Two paragraphs above, the section says "Verify against the resolved set, not against emptiness." The code was doing precisely that.
The fallback now stays out of GOT, and the result is reported as four distinct states instead of one count:
- all resolved owners requested — success
- partial — warning naming which owners are missing
- owners resolved, none requested — error, because the PR has a reviewer but the wrong one
- nothing resolved at all — a note, since the fallback is the intended path there rather than a gap
Not even the fallback landing stays a hard failure.
Verified across all five cases: all-land, partial, all-owners-fail, none-resolved, and nothing-lands. The FALLBACK_USED variable I first added became dead once the accounting was separated, so it is gone rather than left sitting there.
Review catch on #619. Step 4 appended FALLBACK_REVIEWER to GOT, but GOT answers "which resolved owners did I actually request". Counting the fallback there let the verification pass on a run where every real owner was rejected -- the exact silent failure the section exists to prevent, two paragraphs after it says "verify against the resolved set, not against emptiness". Traced against the documented snippet with a stubbed gh. Resolved owners alice and bob, both rejected, fallback accepted: before: warning: requested 1/2 resolved reviewers Requested reviewers: dannyneira exit 0 after: ERROR: none of the 2 resolved owners could be requested (wanted: alice bob); only the fallback is assigned. exit 1 The fallback now stays out of GOT, and the outcomes are reported as four distinct states rather than one count: all owners requested, a partial result naming who is missing, owners resolved but none requested (an error, because the PR has the wrong reviewer), and nothing resolved at all (a note, because the fallback is the intended pall there). Not even the fallback landing remains a hard failure. Verified by extracting the snippet from SKILL.md and executing it against a stubbed gh across all five cases, so the documented text is what was tested rather than a paraphrase of it. Co-Authored-By: Warp <agent@warp.dev>
… rrenk/ambient-pr-reviewers
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review 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 |
There was a problem hiding this comment.
Overview
This PR strengthens the docs PR creation workflow by requiring a lead feature-summary section, making reviewer requests explicit, adding reviewer-only resolver output, and wiring the new tests into CI. I found two correctness issues in the new guidance/checker behavior that should be addressed before merge.
Concerns
- The reviewer-request snippet says to verify the requested reviewers against the resolved owners, but it only uses the read-back list as a non-empty check and still trusts
gh pr editexit status for owner matching. - The PR body checker strips HTML comments after fence detection, so a fenced block inside a multiline HTML comment can cause valid PR bodies to fail the lead-section check.
- No approved or repository spec context was provided for this PR, so there are no spec-drift findings. I did not find separate security issues.
Verdict
Found: 0 critical, 2 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
| # skipping a reviewer, so the PR is the source of truth. Note the jq: teams | ||
| # have no .login, and `[.reviewRequests[].login // .reviewRequests[].name]` | ||
| # silently drops them from a mixed list. | ||
| REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ |
There was a problem hiding this comment.
GOT from gh pr edit exit statuses. The section says gh can exit 0 while skipping a reviewer, so a skipped owner can still pass; compare REQUESTED against WANT/GOT and fail or report missing entries.
There was a problem hiding this comment.
Right, and the section was contradicting itself — it warns that gh can exit 0 while skipping a reviewer, then verified against GOT, which is built from exactly those exit statuses. Fixed in 870e3af1.
Verification now compares the read-back against WANT. I also moved the fallback decision onto the read-back, because your point had a second consequence: when gh exits 0 for every owner and requests none of them, GOT is non-empty, so the fallback never fires and the PR ends up with no reviewer at all.
Extended the harness with a stub that exits 0 without recording the reviewer:
bob silently skipped -> warning: 1/2 resolved owners missing (missing: bob) [was silent]
ALL silently skipped -> fallback fires, then ERROR ... exit 1 [was: no reviewer, misleading message]
One thing that fell out of testing: a team resolves as org/team but reads back as its bare slug, so a naive comparison reported every team as missing. Matching is on the last path segment, lowercased, which also covers case-insensitive logins. Two cases cover it — a lone team, and a mixed user + team list.
Nine cases total: all-land, partial, all-rejected, none-resolved, nothing-lands, the two silent-skip variants, and the two team variants.
Two review catches on #619. 1. Reviewer verification trusted gh's exit status The section warns that `gh pr edit` can exit 0 while quietly skipping a reviewer, then verified against GOT -- which is built from those exit statuses. A silently skipped owner passed. Verification now compares the read-back against WANT. Step 4's fallback also keys off the read-back rather than GOT, because when gh exits 0 for every owner and requests none of them, a GOT-based check skips the fallback entirely and leaves the PR with no reviewer at all. Match on the last path segment, lowercased: a team resolves as org/team but reads back as its bare slug, so a naive compare reported every team as missing. Verified by extracting the snippet from SKILL.md and running it against a stubbed gh across nine cases, including a stub that exits 0 without recording the reviewer: bob silently skipped -> warning names bob (previously silent) all silently skipped -> fall all silently skipped -> fall all silently skipped -> fall es its bare slug, no false "missing" 2. Fence detection ran before comment stripping A ``` line inside an HTML comment opened a phantom code block that ate the closing --> and every line after it, including the lead heading. A valid body failed with "missing required lead section", which reads as an authoring mistake rather than a parser bug. Precisely: only an *odd* number of fence lines inside a comment breaks it. A balanced pair opens and closes a phantom block that happens to end before the -->, so it passed by luck. The tests say which case is the real regression rather than implying all of them were. The fix honors fence state first, then strips comments, then looks for a fence in the visible text. Simply reordering the two would break the mirror case:mirror case:mirror case:mirror case:mirror case:mirror case:mirror cs a comment would swallow tmirror case:mirror case:mirror cver both directions plus a guard tmirror case:mirror case:mirror case:mirror case:mirror case:mirror case:mirror cs a comment would swallow tmirror caseent@warp.dev>
… rrenk/ambient-pr-reviewers
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review 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 |
There was a problem hiding this comment.
Overview
This PR updates the docs PR creation and missing_docs workflows so ambient-generated docs PRs must open with a concise feature summary and must make real GitHub reviewer requests instead of only mentioning reviewers in prose. It also adds the --reviewers-only mode, PR body lead-section validation, unit coverage for the new behavior, and CI coverage for the affected stdlib-only tests.
Concerns
No blocking concerns found. The added comments explain parser edge cases and reviewer-routing failure modes that are easy to regress, and the new tests cover distinct behavior rather than superficial input permutations. The security pass did not identify any issues in the changed scripts, workflow, or skill guidance.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
2 participants
Summary
Replaces #594, which cannot be merged as-is. Same two commits, same author, cherry-picked onto current
main.#594 branched off the #586 branch on Aug 21, while that PR was still evolving. It therefore carries 8 of #586's commits as a stale fork — missing the spec-reading input, Gate 4, the content-design-plan routing, and the merge with #606's tone overhaul. Merging it today would revert all of that. Every one of its 14 conflicts is on a file #586 already delivered.
Cherry-picking the two commits it actually owns produced one conflict instead of fourteen.
What this adds
Reviewer requests become real, not advisory. Naming an engineer in a PR body is not a review request — the skill now says so, and cites #414–#417 as the evidence: four agent-authored PRs that named reviewers in prose and received zero reviews.
gh pr edit --add-reviewerhas succeeded and the requested reviewers read back as the owners resolved.dannyneirarather than no-opping, matching the existing fallback in.github/workflows/release-docs-update.yml.ghcalls. A comma-joined call is atomic, so one unassignable entry silently drops every valid owner with it.suggest_reviewers.pygains--reviewers-onlyfor feeding that snippet.PR bodies must open with a feature summary.
check_pr_body.pygrows a## What this feature doeslead-section check, so a drafting PR opens by saying what shipped rather than dropping the reviewer into a diff.Test coverage that did not exist.
test_check_pr_body.py— new file, 20 tests.mainhas none for the body checker.test_suggest_reviewers.py— 15 tests to 18.CI wiring.
ci.ymlnow runstest_check_new_release.pyand the new body-checker tests. Worth calling out: the 17 release-gate tests added in #586 have never run in CI until this PR.The conflict, and one thing it caught
The single conflict was the scheduled-agent prompt in
missing_docs/SKILL.md. Both sides had real content:mainhas the full script path — a fix made in docs: gate the new feature docs pipeline on worthiness and trigger it from releases #586 after a bot review caught thatscripts/suggest_reviewers.pydoes not resolve from the repo root.--reviewers-only, the feature summary requirement, thedannyneirafallback, and the non-empty verification.#594 predates that path fix, so it reintroduced the relative path. The resolution keeps both: full path plus all of #594's additions. Checked the rest of the file for the same regression — the other two occurrences are prose pointing at the runnable command, not commands themselves.
Validation
test_check_pr_body.py20/20,test_check_new_release.py17/17,test_suggest_reviewers.py18/18test_audit_docs.py18 tests, 1 failure:test_diff_against_committed_snapshot_is_current. Environmental — my localwarp-servercheckout is three months stale, so the committed snapshot reads as drifted.TestAuditBehaviorskips entirely in CI, where the sibling repos are not checked out.Notes for reviewers
Authorship on both commits is preserved as the original factory agent. #594 should be closed as superseded once this lands.
Unverified claims
None — no page content changes. Reviewer-routing and body-checker behavior is covered by the test suites above.
Co-Authored-By: Warp agent@warp.dev