docs skills: make reviewer requests real, and fix drift-watch sandbox footguns - #619
Merged
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.
|
Contributor
|
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}
Contributor
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
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
Contributor
Author
|
/oz-review |
Contributor
|
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 |
Contributor
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
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
Contributor
Author
|
/oz-review |
Contributor
|
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 |
Contributor
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
…tiness Review catch on #619 (QUALITY-1875 rework). When owner resolution came back empty and the PR already carried an unrelated reviewer, the prior "is $REQUESTED non-empty" check treated that unrelated reviewer as proof the dannyneira fallback had landed, so it skipped verifying/re-requesting the fallback by name -- and the script still printed "fallback requested" and exited 0 even when dannyneira was never assigned. Added a has_reviewer helper that checks the read-back for a specific reviewer, used it to gate the fallback request/verification when resolution was empty, and split the final error check so a fallback that truly can't be assigned is reported as a failure instead of masked by an unrelated reviewer already on the PR. Added test_request_reviewers.py, which extracts the documented snippet from SKILL.md and runs it against a stubbed gh/suggest_reviewers.py across the normal-resolution, empty-resolution, and pre-existing-unrelated-reviewer cases. test_unrelated_reviewer_does_not_mask_fallback_failure fails against the pre-fix snippet and passes after the fix. Wired into ci.yml. Co-Authored-By: Warp <agent@warp.dev>
![warp-agent-staging[bot]](https://avatars.githubusercontent.com/in/2196476?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Overview
Reviewed the final PR state at 82dfdf7c.
Verdict
The reviewer-request flow verifies resolved owners and the named fallback from GitHub read-back, including the pre-existing unrelated-reviewer case. The regression tests, green CI, and final PR description all satisfy the review criteria.
Approved
The second drift-watch validation run surfaced three ways the skill misleads an unattended agent. All three are documentation gaps in the skill, not code bugs. 1. Working directory. Every path in the skill is relative to the docs repo root, but the skill never says so, and a sandbox commonly starts one level up. The failure mode is the problem: python3 exits 2 with "can't open file", which is the same exit code audit_docs.py uses to fail loud on a broken environment. An agent that reads the code and not the message concludes a sanity guard tripped and stops. State the cwd requirement up front, name the collision, and repeat it at the release-gate step and in the scheduled-agent prompt -- the prompt is the only one of the three a cron run is guaranteed to read. 2. npm install. `npm run build` is the only validation this repo has and it needs node_modules, which a fresh sandbox does not have. Add `npm ci` as a stated precondition in Requirements and at both build sites. 3. Surface-map key edits. A rename sweep run across feature_surface_map.md corrupted an corrupted an corrupted an corrupted an corrupted an corrupted aon corrupted an corrupted an corrupted an corrupted an corrupted an map entry is a literal code identifier that only matches because it matches ex because it matches ex because it matches ex beCo-Authored-By: Warp <agent@warp.dev>
rachaelrenk
changed the title
rachaelrenk
added a commit
that referenced
this pull request
Aug 26, 2026
* missing_docs: four fast-follows from the drift-watch validation runs Documentation-only. Each item was reproduced against the current code rather than carried over from run notes; two items from the original feedback list turned out to be already fixed and are not included. 1. Snapshot regen has an unstated environment precondition. --update-snapshot rewrites the baseline from whatever the sibling repos hold and cannot tell a current checkout from a stale one, so regenerating from a feature-branched warp or a months-old warp-server silently writes surfaces that are not on the default branch -- and the next --diff compares two wrong baselines. Adds the precondition and a check. This is not hypothetical: on the machine this was written on, warp sits on a feature branch and warp-server is three months behind, and the documented check reports both. 2. Gate 0 lacked the Early Access case. A published page describing a feature is not evidence its API is released; EA features routinely have public prose while their routes stay out of the spec. The /factor while their routes stay out of the spec. The /factor while their ent rat while their routes stay out of the spec. The /factor while their rurface-scoped. audit_slash_commands counts a command as covered when any page mentions it, so /usage read as covered from the GUI s covered from the GUI s covered from the GUI s covered from the GUI sross-check caught it. The CLI and settings audits scope to the pages that own th the pages that own th the pages that own th the pages that own th ix cha the pages thlume and belongs in its own PR. 4. warp-server findings routinel4. warp-server findings routinel4. warp-server findings routinel4. warp-sewarp-server does not, and the /factory handlers have no entry, so those findings land on the dannyneira fallback. Verified by running suggest_reviewers.py against the two factory handler paths. That is the fallback working, and the run output should say so rather than reporting a resolution failure. The real fix belongs in warp-server's ownership file. Not included, because they are already handled: the stash-based branch split (the skill only recommends `git checkout <ref> -- <files>`), and the missing reviewer request on bookkeeping PRs (#619 made the fallback mandatory and verified). Co-Authored-By: Warp <agent@warp.dev> * missing_docs: fix warp default branch to master, not main Co-Authored-By: Warp <agent@warp.dev> * missing_docs: document the two schedule-setup choices the prompt cannot carry Setting the drift-watch schedule up surfaced two configuration decisions the skill never mentions, both of which change what a run does and neither of which is recoverable from the prompt text. Run identity. Quick run is the default in the agent picker, and it executes as the calling user -- so its PRs are authored by that person, which for a schedule means whoever created it. Selecting a cloud agent runs as that agent, and with team GitHub authorization its PRs are authored by the Warp Factories GitHub App. A schedule that opens PRs wants the agent. This is easy to get wrong from observation alone: the two validation runs were quick runs and their PRs were authored by a human, which looks like evidence that agent selection does not affect authorship. It is not -- those runs never selected an agent. Skill inheritance. A run inherits every skill on the agent it runs as, and the schedule form does not let you detach an agent-level skill. Pointing drift-watch at the existing general-purpose docs agent pulled release_updates in, a weekly job that defaults to running all of its tasks, onto a daily trigger. The fix is a dedicated agent, not a different skills selection. Co-Authored-By: Warp <agent@warp.dev> --------- Co-authored-by: Warp <agent@warp.dev> Co-authored-by: warp-agent-staging[bot] <240773466+warp-agent-staging[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes docs-repo reviewer requests real instead of advisory, and adds test coverage that didn't exist for the PR-body checker and the reviewer-request path.
create_pr(Request reviewers). A PR isn't complete untilgh pr edit --add-reviewersucceeds and the read-back confirms the requested owners actually landed — reviewers are requested one at a time so a single unassignable entry can't drop the rest. A failed or empty resolution falls back todannyneira; the fallback is verified by name in the read-back regardless of any other reviewer already on the PR (not just checked for emptiness), so a failed fallback assignment is never reported as a success.create_pr(PR body).check_pr_body.pyrequires drafting PRs to open with a## What this feature doeslead section, so a draft states what shipped before any pipeline detail.missing_docs(reviewer routing).suggest_reviewers.pygains--reviewers-only, the form the reviewer-request snippet consumes directly.missing_docs(sandbox footguns). Three fixes from the second drift-watch validation run, all gaps in the skill's own instructions rather than code bugs. The skill never stated that its paths are relative to the docs repo root, and the failure mode is misleading:python3exits 2 withcan't open file, the same codeaudit_docs.pyuses to fail loud, so an unattended agent reads it as a tripped sanity guard. The cwd requirement is now stated in Requirements, at the release-gate step, and in the scheduled-agent prompt.npm ciis named as a precondition fornpm run build, which a fresh sandbox cannot run. And surface-map entries must be edited individually: a rename sweep across the map corrupted an unrelated key (cost-by-pr-sizetousage-by-pr-size), which map hygiene caught only after it had shipped.ci.yml. Runs the test suites below on every PR.Replaces #594 (superseded, to be closed once this lands) — same two commits and authorship, cherry-picked cleanly onto current
maininstead of #594's stale fork of#586.Verification
test_check_pr_body.py20/20,test_suggest_reviewers.py18/18,test_check_new_release.py17/17,test_request_reviewers.py4/4 — the last extracts the documented reviewer-request bash snippet and runs it against a stubbedgh/suggest_reviewers.py, including the case where an unrelated reviewer is already on the PR and resolution comes back empty.test_audit_docs.py: one pre-existing environmental failure (stale localwarp-servercheckout);TestAuditBehaviorskips entirely in CI, where the sibling repos aren't checked out.Co-Authored-By: Warp agent@warp.dev
Originating thread: https://warpdotdev.slack.com/archives/C09BVK0PL3Y/p1787608478362199