docs skills: make reviewer requests real, and fix drift-watch sandbox footguns

.agents/skills/create_pr/SKILL.md

@@ -109,10 +109,42 @@ Include the trailing slash on `destination` and the `statusCode`, matching the e
 
 ## PR Description Guidelines
 
-Structure your PR description with these sections:
+Structure your PR description with these sections, in this order. The feature summary comes first; everything else follows it.
+
+### What this feature does (required on drafting PRs)
+
+Open the body with a plain-language summary of what the feature does **for the user**. This is the first thing a reviewing engineer reads, so it must not be pipeline bookkeeping — which spec produced the draft, which workflow generated it, and which run it came from all belong further down. A reviewer who only reads this section should be able to tell whether the docs describe the right thing.
+
+End the summary with the shipped-in fact, not a forecast. Read the version and date from the release accessor the drift-watch gate already uses, rather than adding a second way to look up a release:
+
+```bash
+# Exits 10 when the current stable release was already processed, which is not an
+# error for this purpose — we only want the version and date it reports.
+python3 .agents/skills/missing_docs/scripts/check_new_release.py --json > /tmp/release.json || true
+python3 -c "import json; d=json.load(open('/tmp/release.json')); print(d['current_version'], d['release_date'])"
+```
+
+Write "shipped in `<version>` (`<date>`)". Do not write a target or predicted ship date: there is no trustworthy source for one, and a forecast in a merged PR body ages into a false claim.
+
+**Length budget: 75 words maximum**, ideally two to four sentences. Drafts are already too wordy; a summary that runs longer than a short paragraph has stopped being a summary. `check_pr_body.py` enforces the budget, the heading text, and the position.
+
+```markdown
+## What this feature does
+
+Workspace admin roles let a workspace owner delegate whole-workspace management — membership, billing, and cloud agent run visibility — to an admin without handing over ownership. Shipped in `v0.2026.08.18.02.52.stable_00` (`2026-08-18`).
+```
+
+Verify it before submitting, along with the other body checks:
+
+```bash
+python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md \
+  --require-lead-section "## What this feature does"
+```
+
+The check fails if the section is missing, is not the first content in the body, is empty, or exceeds the word budget. Position is checked against content rather than headings, so a body cannot open with a few unheaded lines of spec/workflow/run-ID preamble and still pass. Omit the section — and the flag — only for the small corrections listed under "When a plan can be skipped": typos, link fixes, terminology sweeps, generated updates, and screenshot swaps have no feature to summarize.
 
 ### Summary
-Brief explanation of what the PR accomplishes and why.
+Brief explanation of what the PR accomplishes and why. This is where the pipeline detail goes: the source spec, the generating workflow, the new page path, and the sidebar entry.
 
 ### Changes
 Bulleted list of specific changes, organized by file or area:
@@ -203,8 +235,15 @@ Exit code 0 if PR exists, 1 if not.
 :::
 
 ```bash
-# 1. Write the description to a temp file using the create_file tool or a heredoc
+# 1. Write the description to a temp file using the create_file tool or a heredoc.
+#    The `## What this feature does` block is DRAFTING-PR ONLY - drop it (and the
+#    --require-lead-section flag in step 2) for typos, link fixes, terminology
+#    sweeps, generated updates, and screenshot swaps.
 cat > /tmp/pr-body.md << 'EOF'
+## What this feature does
+One short paragraph: what the feature does for the user, ending with
+shipped in `<version>` (`<date>`).
+
 ## Summary
 Description of changes
 
@@ -215,16 +254,163 @@ Description of changes
 Co-Authored-By: Oz <oz-agent@warp.dev>
 EOF
 
-# 2. Verify the body for corruption before submitting (exits non-zero on failure)
-python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md
+# 2. Verify the body for corruption before submitting (exits non-zero on failure).
+python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md \
+  --require-lead-section "## What this feature does"   # drafting PRs only
+
+# For a non-drafting correction, run the check without the flag:
+# python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md
 
 # 3. Create the PR using the file (only if the check passed)
 gh pr create --title "docs: Add feature documentation" --body-file /tmp/pr-body.md
 
+# 4. REQUIRED: request the reviewer for real (see "Request reviewers" below).
+#    The PR is not complete until this has succeeded.
+
 # Open in browser to fill details
 gh pr create --web
 ```
 
+### Request reviewers (required)
+
+**Naming a reviewer in the body is not a review request.** A `/cc @engineer` mention notifies nobody through GitHub's review queue: the PR shows no requested reviewer, never appears in that engineer's "Review requested" filter, and quietly goes unreviewed. Every one of the four ambient-drafted docs PRs — #414, #415, #416, #417 — named reviewers in prose and received zero reviews; three had an empty requested-reviewers list and the fourth had a single reviewer added by hand.
+
+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.
+
+Two details below are load-bearing, and getting either wrong reintroduces the silent drop this section exists to prevent:
+
+- **Request one reviewer per call.** `gh pr edit --add-reviewer a,b,c` sends a single atomic mutation, so one unassignable entry rejects the whole list. Since a resolution routinely mixes users with a team, and a team with no access to this repo cannot be requested here, a comma-joined call can fail wholesale and take every valid owner down with it.
+- **Verify against the resolved set, not against emptiness.** "Is the list non-empty?" passes when the real owner was dropped and only the fallback landed, which looks identical to success.
+
+```bash
+PR=123
+FALLBACK_REVIEWER=dannyneira
+
+# 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
+#    missing_docs skill's "Reviewer routing" section for how to pick those files.
+#    Diagnostics go to stderr, so this captures only the reviewer list.
+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.
+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
+fi
+
+# 3. Request each reviewer separately so one bad entry cannot drop the rest.
+IFS=',' read -ra WANT <<< "$REVIEWERS"
+GOT=()
+for R in "${WANT[@]}"; do
+  if gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$R"; then
+    GOT+=("$R")
+  else
+    echo "warning: could not request $R on PR $PR"
+  fi
+done
+
+# 4. Read back from the PR. This is the only trustworthy signal: `gh pr edit`
+#    can exit 0 while quietly skipping a reviewer, so GOT records what gh
+#    *claimed* and the read-back is what actually landed. Every decision below
+#    keys off the read-back. Note the jq: teams have no .login, and
+#    `[.reviewRequests[].login // .reviewRequests[].name]` silently drops them
+#    from a mixed list.
+read_requested() {
+  gh pr view "$PR" --repo warpdotdev/docs \
+    --json reviewRequests --jq '[.reviewRequests[] | .login // .slug // .name] | join(",")'
+}
+REQUESTED=$(read_requested)
+
+# 5. A helper to check whether a specific reviewer is present in the
+#    read-back, not just whether the read-back is non-empty. Match on the
+#    last path segment, lowercased: a team resolves as `org/team` but reads
+#    back as its bare slug, and GitHub logins are case-insensitive.
+_norm() { printf '%s' "${1##*/}" | tr 'A-Z' 'a-z'; }
+has_reviewer() {
+  local want target
+  want=$(_norm "$1")
+  IFS=',' read -ra _have <<< "$REQUESTED"
+  for target in "${_have[@]}"; do
+    [[ "$(_norm "$target")" == "$want" ]] && return 0
+  done
+  return 1
+}
+
+# 6. Verify the fallback actually landed whenever resolution came back empty,
+#    and otherwise fall back when nothing at all landed. An emptiness check on
+#    $REQUESTED alone is wrong for the empty-resolution case: a PR that
+#    already carries an unrelated reviewer (requested before this script ran,
+#    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 "$FALLBACK_REVIEWER"; then
+    gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" ||
+      echo "warning: fallback $FALLBACK_REVIEWER could not be requested"
+    REQUESTED=$(read_requested)
+  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)
+fi
+
+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" \
+       "(existing reviewers: $REQUESTED); report this run as failed."
+  exit 1
+fi
+
+# 7. Compare the read-back against what was resolved.
+IFS=',' read -ra HAVE <<< "$REQUESTED"
+MISSING=()
+for R in "${WANT[@]}"; do
+  found=0
+  for H in "${HAVE[@]}"; do
+    [[ "$(_norm "$R")" == "$(_norm "$H")" ]] && { found=1; break; }
+  done
+  (( found )) || MISSING+=("$R")
+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"
+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.
+  echo "ERROR: none of the ${#WANT[@]} resolved owners are on PR $PR" \
+       "(wanted: ${WANT[*]}); only the fallback is assigned. Report this run as failed."
+  exit 1
+elif (( ${#MISSING[@]} > 0 )); then
+  echo "warning: ${#MISSING[@]}/${#WANT[@]} resolved owners missing from PR $PR" \
+       "(missing: ${MISSING[*]}); name them and why in the run output"
+fi
+
+echo "Requested reviewers: $REQUESTED"
+```
+
+A partial result is a reportable outcome, not a pass: if some owners could not be requested, say which ones and why in the run output, so the gap is visible rather than buried. If even the fallback cannot be assigned, report the run as failed. Do not close out a PR whose requested-reviewers list is empty.
+
+:::caution
+A team handle resolved from `STAKEHOLDERS` or `CODEOWNERS` can only be requested on a repo that team has access to. `warpdotdev/oss-maintainers` is the root-rule owner in the warp client repo and therefore appears in most resolutions, but it has no access to `warpdotdev/docs`, so requesting it here fails. That is why step 3 requests one at a time.
+:::
+
+:::note
+Auto-requesting the review does not make it *block* merge. Whether an ambient docs PR should require that approval through branch protection is an open question for the docs owner, not something this skill decides.
+:::
+
 ### Update an existing PR
 
 When updating the body of an existing PR, make the **smallest** change rather than regenerating the whole description from memory — re-emitting a long body is what invites repetition-loop degeneration. Fetch the current body, apply a minimal or additive edit, verify it, then submit.
@@ -245,8 +431,12 @@ gh pr edit 123 --body-file /tmp/pr-body.md
 # Edit title only
 gh pr edit 123 --title "New title"
 
-# Add reviewers or labels
-gh pr edit 123 --add-reviewer username --add-label documentation
+# Add labels
+gh pr edit 123 --add-label documentation
+
+# Add reviewers - see "Request reviewers (required)" above; this is mandatory on a
+# new PR, not an optional extra.
+gh pr edit 123 --add-reviewer username
 ```
 
 ### View PR status
@@ -266,10 +456,11 @@ Co-Authored-By: Oz <oz-agent@warp.dev>
 
 ## After Opening the PR
 
-1. **Monitor for merge conflicts** - If main is updated, merge it into your branch
-2. **Respond to review comments** - Address feedback promptly
-3. **Re-run checks after changes** - Run `trunk check` and link checker after making updates
-4. **Verify Astro Starlight preview** - Astro Starlight automatically generates a preview for PRs; check that rendering looks correct
+1. **Confirm the review request landed** - Re-read `reviewRequests` on the PR. An empty list means the PR is not finished, whatever the body says. See "Request reviewers (required)".
+2. **Monitor for merge conflicts** - If main is updated, merge it into your branch
+3. **Respond to review comments** - Address feedback promptly
+4. **Re-run checks after changes** - Run `trunk check` and link checker after making updates
+5. **Verify Astro Starlight preview** - Astro Starlight automatically generates a preview for PRs; check that rendering looks correct
 
 ## Best Practices