From bba838b099957361aaea25a39c94a4b88fc026fb Mon Sep 17 00:00:00 2001 From: Warp Date: Fri, 21 Aug 2026 00:07:57 +0000 Subject: [PATCH 1/2] docs: request reviewers for real and lead ambient PRs with a feature 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 --- .agents/skills/create_pr/SKILL.md | 115 ++++++++++++-- .agents/skills/create_pr/check_pr_body.py | 77 +++++++++ .../skills/create_pr/test_check_pr_body.py | 146 ++++++++++++++++++ .agents/skills/missing_docs/SKILL.md | 64 ++++++-- .../missing_docs/scripts/suggest_reviewers.py | 65 ++++++-- .../scripts/test_suggest_reviewers.py | 47 ++++++ .github/workflows/ci.yml | 5 + 7 files changed, 480 insertions(+), 39 deletions(-) create mode 100644 .agents/skills/create_pr/test_check_pr_body.py diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index 894cfd96e..cb60b866a 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.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 `` (``)". 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 heading, is empty, or exceeds the word budget. 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: @@ -205,6 +237,10 @@ 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 cat > /tmp/pr-body.md << 'EOF' +## What this feature does +One short paragraph: what the feature does for the user, ending with +shipped in `` (``). + ## Summary Description of changes @@ -215,16 +251,70 @@ Description of changes Co-Authored-By: Oz 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). +# On a drafting PR, also assert the feature-summary lead section. +python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md \ + --require-lead-section "## What this feature does" # 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. + +```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. +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. +if [[ -z "$REVIEWERS" ]]; then + echo "warning: no owner resolved - falling back to $FALLBACK_REVIEWER" + REVIEWERS="$FALLBACK_REVIEWER" +fi + +# 3. Make the request. +gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$REVIEWERS" || + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" + +# 4. Verify it landed. gh exits 0 even when it silently skips a reviewer it +# cannot assign (no repo access, a bad handle, or the PR author themselves), +# so confirm against the PR rather than trusting the exit code. +REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ + --json reviewRequests --jq '[.reviewRequests[].login // .reviewRequests[].name] | join(",")') +if [[ -z "$REQUESTED" ]]; then + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" + REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ + --json reviewRequests --jq '[.reviewRequests[].login // .reviewRequests[].name] | join(",")') +fi +[[ -n "$REQUESTED" ]] || { echo "ERROR: no reviewer requested on PR $PR"; exit 1; } +echo "Requested reviewers: $REQUESTED" +``` + +If even the fallback cannot be assigned, report it as a failure of the run. Do not close out a PR whose requested-reviewers list is empty. + +:::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 +335,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 +360,11 @@ Co-Authored-By: Oz ## 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 diff --git a/.agents/skills/create_pr/check_pr_body.py b/.agents/skills/create_pr/check_pr_body.py index 64f0269ce..8e9653db4 100644 --- a/.agents/skills/create_pr/check_pr_body.py +++ b/.agents/skills/create_pr/check_pr_body.py @@ -23,6 +23,11 @@ * Duplicate heading - the same Markdown heading text appearing more than once. * Required heading - (optional) assert specific headings are present exactly once, for skills that emit a fixed body template. + * Lead section - (optional) assert a heading is the FIRST heading in the + body, has prose under it, and stays within a word budget. + Drafting PRs must open with a plain-language summary of + what the feature does for the user, so a reviewer learns + that before any pipeline bookkeeping. Usage: python3 check_pr_body.py /tmp/pr-body.md @@ -30,6 +35,8 @@ python3 check_pr_body.py /tmp/pr-body.md \ --require-heading "## Patterns addressed" \ --require-heading "## Improvement targets" + python3 check_pr_body.py /tmp/pr-body.md \ + --require-lead-section "## What this feature does" Exit codes: 0 no issues found @@ -52,6 +59,11 @@ LONG_WINDOW = 80 LONG_MIN_COUNT = 2 +# Word budget for the lead section. "Drafts are too wordy" is a standing complaint, +# and a summary that runs past a short paragraph stops being a summary. Two to four +# sentences fit comfortably under this cap. +LEAD_SECTION_MAX_WORDS = 75 + def _strip_urls(text: str) -> str: """Remove URLs so repeated link targets don't cause false positives. @@ -133,6 +145,60 @@ def check_required_headings(lines: List[str], required: List[str]) -> List[str]: return problems +def check_lead_section(lines: List[str], heading: str) -> List[str]: + """Return messages if the lead section is missing, misplaced, empty, or too long. + + The lead section is the plain-language answer to "what does this feature do for + the user?", and it only does that job if the reviewer hits it first. Ambient + drafts previously opened with pipeline bookkeeping (which spec, which workflow, + which run), so the check asserts position as well as presence. + """ + wanted = heading.strip() + problems: List[str] = [] + + headings: List[Tuple[int, str]] = [] + for line_num, line in _iter_non_code_lines(lines): + if re.match(r"^#{1,6}\s+\S", line): + headings.append((line_num, line.strip())) + + matches = [ln for ln, text in headings if text == wanted] + if not matches: + return [f"missing required lead section: {wanted!r} (must be the first heading in the body)"] + if len(matches) > 1: + problems.append( + f"lead section appears {len(matches)}x (expected once): {wanted!r}" + ) + + first_line, first_text = headings[0] + if first_text != wanted: + problems.append( + f"lead section is not first: {first_text!r} (line {first_line}) precedes " + f"{wanted!r} (line {matches[0]}). The reader must get the feature summary " + "before any other section." + ) + + # Collect the prose between the lead heading and the next heading. + start = matches[0] + body_words: List[str] = [] + for line_num, line in _iter_non_code_lines(lines): + if line_num <= start: + continue + if re.match(r"^#{1,6}\s+\S", line): + break + body_words.extend(line.split()) + + if not body_words: + problems.append(f"lead section {wanted!r} has no content under it") + elif len(body_words) > LEAD_SECTION_MAX_WORDS: + problems.append( + f"lead section {wanted!r} is {len(body_words)} words " + f"(budget: {LEAD_SECTION_MAX_WORDS}). Cut it to a short paragraph: what the " + "feature does for the user, plus the shipped-in version and date." + ) + + return problems + + def main(argv: Optional[List[str]] = None) -> int: parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) parser.add_argument("body", help="path to the PR body file, or '-' for stdin") @@ -143,6 +209,14 @@ def main(argv: Optional[List[str]] = None) -> int: metavar="HEADING", help="assert this exact heading line is present exactly once (repeatable)", ) + parser.add_argument( + "--require-lead-section", + metavar="HEADING", + help=( + "assert this exact heading is the FIRST heading in the body, appears once, " + f"and carries 1-{LEAD_SECTION_MAX_WORDS} words of prose" + ), + ) args = parser.parse_args(argv) if args.body == "-": @@ -176,6 +250,9 @@ def main(argv: Optional[List[str]] = None) -> int: issues.extend(check_required_headings(lines, args.require_heading)) + if args.require_lead_section: + issues.extend(check_lead_section(lines, args.require_lead_section)) + if issues: print("PR body integrity check FAILED:\n", file=sys.stderr) for issue in issues: diff --git a/.agents/skills/create_pr/test_check_pr_body.py b/.agents/skills/create_pr/test_check_pr_body.py new file mode 100644 index 000000000..9d17e8a79 --- /dev/null +++ b/.agents/skills/create_pr/test_check_pr_body.py @@ -0,0 +1,146 @@ +#!/usr/bin/env python3 +"""Unit tests for check_pr_body.py. + +Stdlib unittest only, no third-party deps and no network. + +Run: + python3 .agents/skills/create_pr/test_check_pr_body.py +""" + +from __future__ import annotations + +import importlib.util +import unittest +from pathlib import Path + +_HERE = Path(__file__).resolve().parent +_MODULE_PATH = _HERE / "check_pr_body.py" + +_spec = importlib.util.spec_from_file_location("check_pr_body", _MODULE_PATH) +cpb = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(cpb) + +LEAD = "## What this feature does" + +GOOD_BODY = """## What this feature does + +Workspace admin roles let an owner delegate whole-workspace management -- membership, +billing, and run visibility -- without handing over ownership. Shipped in +`v0.2026.08.18.02.52.stable_00` (2026-08-18). + +## Summary + +Auto-drafted documentation for workspace admin roles. + +## Content design plan + +**Audience and JTBD:** A workspace owner onboarding a second admin. +""" + + +def lines(text: str): + return text.splitlines() + + +class TestCheckLeadSection(unittest.TestCase): + def test_accepts_a_well_formed_lead_section(self): + self.assertEqual(cpb.check_lead_section(lines(GOOD_BODY), LEAD), []) + + def test_heading_is_matched_after_whitespace_normalization(self): + self.assertEqual(cpb.check_lead_section(lines(GOOD_BODY), f" {LEAD} "), []) + + def test_missing_lead_section(self): + body = "## Summary\n\nAuto-drafted documentation.\n" + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1) + self.assertIn("missing required lead section", problems[0]) + + def test_lead_section_not_first(self): + body = ( + "## Summary\n\nAuto-drafted documentation for workspace admin roles.\n\n" + f"{LEAD}\n\nIt lets an owner delegate workspace management. " + "Shipped in `v1` (2026-08-18).\n" + ) + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1) + self.assertIn("lead section is not first", problems[0]) + self.assertIn("## Summary", problems[0]) + + def test_lead_section_with_no_content(self): + body = f"{LEAD}\n\n## Summary\n\nAuto-drafted documentation.\n" + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1) + self.assertIn("has no content under it", problems[0]) + + def test_lead_section_over_word_budget(self): + filler = " ".join(["word"] * (cpb.LEAD_SECTION_MAX_WORDS + 1)) + body = f"{LEAD}\n\n{filler}\n\n## Summary\n\nAuto-drafted documentation.\n" + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1) + self.assertIn(f"budget: {cpb.LEAD_SECTION_MAX_WORDS}", problems[0]) + + def test_lead_section_exactly_at_word_budget_is_allowed(self): + filler = " ".join(["word"] * cpb.LEAD_SECTION_MAX_WORDS) + body = f"{LEAD}\n\n{filler}\n\n## Summary\n\nAuto-drafted documentation.\n" + self.assertEqual(cpb.check_lead_section(lines(body), LEAD), []) + + def test_duplicate_lead_section(self): + body = ( + f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n\n" + f"{LEAD}\n\nAgain.\n" + ) + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertTrue(any("appears 2x" in p for p in problems), problems) + + def test_heading_inside_a_code_fence_does_not_count_as_first(self): + """A fenced example of the template must not satisfy or displace the check.""" + body = ( + "```markdown\n## Summary\nan example\n```\n\n" + f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n" + ) + self.assertEqual(cpb.check_lead_section(lines(body), LEAD), []) + + +class TestMainExitCodes(unittest.TestCase): + def _write(self, tmpdir: Path, text: str) -> str: + path = tmpdir / "body.md" + path.write_text(text, encoding="utf-8") + return str(path) + + def test_exit_zero_on_good_body(self): + import tempfile + + with tempfile.TemporaryDirectory() as d: + path = self._write(Path(d), GOOD_BODY) + self.assertEqual(cpb.main([path, "--require-lead-section", LEAD]), 0) + + def test_exit_one_when_lead_section_missing(self): + import tempfile + + with tempfile.TemporaryDirectory() as d: + path = self._write(Path(d), "## Summary\n\nAuto-drafted documentation.\n") + self.assertEqual(cpb.main([path, "--require-lead-section", LEAD]), 1) + + def test_lead_section_check_is_opt_in(self): + """Without the flag, a body with no lead section still passes.""" + import tempfile + + with tempfile.TemporaryDirectory() as d: + path = self._write(Path(d), "## Summary\n\nAuto-drafted documentation.\n") + self.assertEqual(cpb.main([path]), 0) + + +class TestExistingChecksStillWork(unittest.TestCase): + def test_unbalanced_backtick_detected(self): + issues = cpb.find_unbalanced_backticks(lines("A sentence that stops because `m\n")) + self.assertEqual(len(issues), 1) + + def test_duplicate_heading_detected(self): + self.assertEqual( + cpb.find_duplicate_headings(lines("## Summary\na\n## Summary\nb\n")), + ["## Summary"], + ) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/.agents/skills/missing_docs/SKILL.md b/.agents/skills/missing_docs/SKILL.md index 21f767aeb..77c30dd6e 100644 --- a/.agents/skills/missing_docs/SKILL.md +++ b/.agents/skills/missing_docs/SKILL.md @@ -340,7 +340,11 @@ python3 .agents/skills/missing_docs/scripts/suggest_reviewers.py \ warp:app/src/search/slash_command_menu/static_commands/commands.rs ``` -Then assign the resolved reviewers on the PR with `gh pr edit --add-reviewer `. Unresolved paths are non-fatal — leave them for manual assignment rather than blocking the run. +Add `--reviewers-only` to get just the comma-joined `--add-reviewer` argument (empty output when nothing resolved), which is the form the mandatory request step below consumes. + +Then **actually request the review on GitHub** with `gh pr edit --add-reviewer `. A `/cc @engineer` line in the PR body is not a review request: it puts nothing in the engineer's review queue. All four ambient-drafted docs PRs (#414, #415, #416, #417) named reviewers in prose and got zero reviews, three of them with an empty requested-reviewers list. + +An individual unresolved *path* is non-fatal — other paths usually resolve the same owner. An empty *result* is not: fall back to `dannyneira` rather than opening the PR with no reviewer. See step 7 of drift-watch mode for the required command. ### PR strategy: one PR per feature @@ -430,17 +434,46 @@ with the product. Each run: ``` 6. **Validate**: `npm run build` if doc pages changed; re-run the audit and confirm the addressed findings are gone. -7. **Route reviewers**: run `scripts/suggest_reviewers.py` (see Reviewer routing) - with the source files behind the addressed findings to resolve the owning - engineers for the PR. +7. **Route reviewers and request the review** (required, not advisory): resolve the + owning engineers with `scripts/suggest_reviewers.py` (see Reviewer routing), passing + the source files behind the addressed findings, then make a real GitHub review request + on each PR you open in step 8. Naming the engineer in the body is not a request — that + is exactly how #414–#417 ended up with zero reviews. + + **A PR is not complete until `gh pr edit --add-reviewer` has succeeded and the + requested-reviewers list is non-empty.** A resolution failure falls back to + `dannyneira`; it never no-ops. This matches the fallback in + `.github/workflows/release-docs-update.yml` (the "Assign last docs PR reviewer" step). + ```bash + PR= + FALLBACK_REVIEWER=dannyneira + + 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) + [[ -z "$REVIEWERS" ]] && REVIEWERS="$FALLBACK_REVIEWER" + + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$REVIEWERS" || + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" + + # gh exits 0 even when it silently skips a reviewer it cannot assign, so verify. + REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ + --json reviewRequests --jq '[.reviewRequests[].login // .reviewRequests[].name] | join(",")') + [[ -n "$REQUESTED" ]] || { echo "ERROR: no reviewer requested on PR $PR"; exit 1; } + echo "PR $PR reviewers: $REQUESTED" + ``` + Keep the prose `/cc @engineer` mention in the body as well — this adds the real + request, it does not replace the mention. Report any PR whose requested-reviewers list + is still empty as a run failure. 8. **Open one PR per feature** following the PR strategy above (not a single mega PR): one focused PR per documented feature (grouping only features that share a doc file or owner), each carrying its content design plan as a section in the PR body, plus a single companion audit-bookkeeping PR for all `feature_surface_map.md`, `changelog_decisions.md`, `last_release_processed.json`, and `surface_snapshot.json` - changes. Use the `create_pr` skill, assign each PR's owning reviewer from step 7 - (`gh pr edit --add-reviewer ...`), and summarize remaining (deferred) findings in - the relevant PR body so nothing is silently dropped. + changes. Use the `create_pr` skill: every drafting PR body opens with the required + `## What this feature does` summary, and every PR gets its owning reviewer requested + per step 7 before the run is done. Summarize remaining (deferred) findings in the + relevant PR body so nothing is silently dropped. A run that gates out every candidate is a successful run. It opens no feature PRs and only the bookkeeping PR recording the verdicts. Do not manufacture work to justify the @@ -462,13 +495,16 @@ Recommended scheduled-agent prompt (copy when setting up the agent): > page over creating a new one, and use the sync-openapi-spec skill for API spec gaps. > Update the surface map for every triaged flag, append every verdict to > changelog_decisions.md, and regenerate the surface snapshot with --update-snapshot. -> Resolve reviewers by running scripts/suggest_reviewers.py against the source files -> behind each addressed finding. Open one focused PR per documented feature (grouping only -> features that share a doc file or owner), each with the content design plan as a section -> in its body, plus a single companion bookkeeping PR for the feature_surface_map.md, -> changelog_decisions.md, last_release_processed.json, and surface_snapshot.json changes; -> assign each PR's resolved owner as reviewer, and list any findings you deferred in the -> relevant PR body. +> Resolve reviewers by running scripts/suggest_reviewers.py --reviewers-only against the +> source files behind each addressed finding. Open one focused PR per documented feature +> (grouping only features that share a doc file or owner), each opening with the required +> "## What this feature does" summary and carrying the content design plan as a section in +> its body, plus a single companion bookkeeping PR for the feature_surface_map.md, +> changelog_decisions.md, last_release_processed.json, and surface_snapshot.json changes. +> Request the resolved owner as reviewer on every PR with gh pr edit --add-reviewer, +> falling back to dannyneira when nothing resolves, and verify the requested-reviewers +> list is non-empty before you finish — a PR with no requested reviewer is an incomplete +> run, not a delivered one. List any findings you deferred in the relevant PR body. ### Invocation modes diff --git a/.agents/skills/missing_docs/scripts/suggest_reviewers.py b/.agents/skills/missing_docs/scripts/suggest_reviewers.py index 68ad5b48c..83f575735 100755 --- a/.agents/skills/missing_docs/scripts/suggest_reviewers.py +++ b/.agents/skills/missing_docs/scripts/suggest_reviewers.py @@ -29,6 +29,14 @@ and a ready-to-run `gh pr edit --add-reviewer` snippet. Exit code is always 0; unresolved paths are reported but never fatal (so a scheduled run is not blocked by an ownership gap — it just falls back to the default owners or none). + +Pass `--reviewers-only` to print just the comma-joined argument for +`gh pr edit --add-reviewer` (empty output when nothing resolved). That is the form +the mandatory reviewer-request step consumes, so callers never have to scrape the +human-readable table: + + REVIEWERS=$(python3 suggest_reviewers.py --reviewers-only --warp ../warp warp:app/src/x.rs) + [[ -z "$REVIEWERS" ]] && REVIEWERS=dannyneira # never drop the review request """ import argparse @@ -81,8 +89,17 @@ def main(): ap = argparse.ArgumentParser(description="Suggest PR reviewers from code ownership.") ap.add_argument("--warp", help="Path to the warp client repo root (warp-internal accepted).") ap.add_argument("--warp-server", dest="warp_server", help="Path to the warp-server repo root.") + ap.add_argument( + "--reviewers-only", + action="store_true", + help=( + "Print only the comma-joined `gh pr edit --add-reviewer` argument " + "(empty when nothing resolved), for scripted use." + ), + ) ap.add_argument("paths", nargs="*", help="Source paths as repo:relpath.") args = ap.parse_args() + quiet = args.reviewers_only # Build per-repo rule lists (STAKEHOLDERS first, then CODEOWNERS so enforced # rules take precedence as later matches). @@ -107,45 +124,63 @@ def main(): print("No source paths given. Pass repo:relpath args or pipe them on stdin.", file=sys.stderr) return 0 + def report(message=""): + """Print human-readable progress, suppressed under --reviewers-only.""" + if not quiet: + print(message) + users, teams = [], [] unresolved = [] - print("Reviewer resolution:") + report("Reviewer resolution:") for item in inputs: if ":" not in item: unresolved.append(item) - print(f" ? {item} — missing repo prefix (use warp: or warp-server:)") + report(f" ? {item} — missing repo prefix (use warp: or warp-server:)") continue repo, rel = item.split(":", 1) rules = repos.get(repo) if rules is None: unresolved.append(item) - print(f" ? {item} — no ownership file loaded for repo '{repo}'") + report(f" ? {item} — no ownership file loaded for repo '{repo}'") continue owners, pattern = owners_for(rel, rules) if not owners: unresolved.append(item) - print(f" ? {repo}:{rel} — no owner match") + report(f" ? {repo}:{rel} — no owner match") continue - print(f" - {repo}:{rel} -> {' '.join(owners)} (matched: {pattern})") + report(f" - {repo}:{rel} -> {' '.join(owners)} (matched: {pattern})") for o in owners: handle = o.lstrip("@") bucket = teams if "/" in handle else users if handle not in bucket: bucket.append(handle) - print() - print(f"Reviewers (users): {', '.join(users) if users else '(none)'}") - print(f"Reviewers (teams): {', '.join(teams) if teams else '(none)'}") - if unresolved: - print(f"Unresolved paths: {len(unresolved)} (left for manual assignment)") - # gh accepts users by login and teams as org/team; both via --add-reviewer. review_args = users + teams + joined = ",".join(review_args) + + if quiet: + # Sole output: the --add-reviewer argument, or nothing at all. An empty + # result is the caller's cue to use the fallback reviewer, never to skip + # the request. + if joined: + print(joined) + return 0 + + report() + report(f"Reviewers (users): {', '.join(users) if users else '(none)'}") + report(f"Reviewers (teams): {', '.join(teams) if teams else '(none)'}") + if unresolved: + report(f"Unresolved paths: {len(unresolved)} (left for manual assignment)") + if review_args: - joined = ",".join(review_args) - print() - print("Suggested command (replace with the PR number):") - print(f" gh pr edit --add-reviewer {joined}") + report() + report("Suggested command (replace with the PR number):") + report(f" gh pr edit --add-reviewer {joined}") + else: + report() + report("No owners resolved. Do NOT skip the review request — assign the") + report("fallback reviewer (dannyneira) so the PR still reaches a human.") return 0 diff --git a/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py b/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py index c03f6bb60..dba27740c 100755 --- a/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py +++ b/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py @@ -150,6 +150,53 @@ def test_resolution_dedup_and_team_split(self): # The unmatched server path is reported, not fatal. self.assertIn("no owner match", out) + def test_reviewers_only_prints_bare_add_reviewer_argument(self): + """--reviewers-only must be directly consumable by `gh pr edit --add-reviewer`.""" + with tempfile.TemporaryDirectory() as d: + warp = Path(d) / "warp" + self._make_repo( + warp, + "/ @warpdotdev/oss-maintainers\n/app/src/settings/ @lucie\n", + ) + result = subprocess.run( + [ + sys.executable, + str(_MODULE_PATH), + "--reviewers-only", + "--warp", + str(warp), + "warp:app/src/settings/ssh.rs", + "warp:crates/warp_features/src/lib.rs", # default team fallback + ], + capture_output=True, + text=True, + stdin=subprocess.DEVNULL, + ) + self.assertEqual(result.returncode, 0, result.stderr) + # Exactly one line, no resolution table, no "Suggested command" prose. + self.assertEqual(result.stdout, "lucie,warpdotdev/oss-maintainers\n") + + def test_reviewers_only_is_empty_when_nothing_resolves(self): + """An empty result is the caller's cue to use the fallback reviewer.""" + with tempfile.TemporaryDirectory() as d: + warp = Path(d) / "warp" + self._make_repo(warp, "/app/src/settings/ @lucie\n") + result = subprocess.run( + [ + sys.executable, + str(_MODULE_PATH), + "--reviewers-only", + "--warp", + str(warp), + "warp:crates/nothing/owns/this.rs", + ], + capture_output=True, + text=True, + stdin=subprocess.DEVNULL, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, "") + def test_warp_internal_alias(self): with tempfile.TemporaryDirectory() as d: warp = Path(d) / "warp" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e005713d..542e5df55 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,6 +47,11 @@ jobs: python3 .agents/skills/missing_docs/scripts/test_suggest_reviewers.py python3 .agents/skills/missing_docs/scripts/test_audit_docs.py + # Stdlib-only tests for the PR body integrity checker, including the + # feature-summary lead section that drafting PRs must open with. + - name: Test create_pr body checker + run: python3 .agents/skills/create_pr/test_check_pr_body.py + # Validate the validate_ui_refs snapshot and script invariants. Uses # a synthetic warp client fixture internally — no checkout required. - name: Self-test validate_ui_refs skill From aeda3e640077ed1866c21a014581b060f5bcede9 Mon Sep 17 00:00:00 2001 From: Warp Date: Fri, 21 Aug 2026 00:38:58 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs:=20address=20review=20=E2=80=94=20per-?= =?UTF-8?q?reviewer=20requests,=20first-content=20check,=20CI=20wiring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .agents/skills/create_pr/SKILL.md | 64 ++++++++++---- .agents/skills/create_pr/check_pr_body.py | 85 ++++++++++++++++--- .../skills/create_pr/test_check_pr_body.py | 69 ++++++++++++++- .agents/skills/missing_docs/SKILL.md | 36 +++----- .../missing_docs/scripts/suggest_reviewers.py | 32 +++++-- .../scripts/test_suggest_reviewers.py | 27 ++++++ .github/workflows/ci.yml | 1 + 7 files changed, 252 insertions(+), 62 deletions(-) diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index cb60b866a..2aa6f7824 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -131,7 +131,7 @@ Write "shipped in `` (``)". Do not write a target or predicted sh ```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). +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: @@ -141,7 +141,7 @@ 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 heading, is empty, or exceeds the word budget. 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. +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. This is where the pipeline detail goes: the source spec, the generating workflow, the new page path, and the sidebar entry. @@ -235,7 +235,10 @@ 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 @@ -252,9 +255,11 @@ Co-Authored-By: Oz EOF # 2. Verify the body for corruption before submitting (exits non-zero on failure). -# On a drafting PR, also assert the feature-summary lead section. python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md \ - --require-lead-section "## What this feature does" + --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 @@ -274,6 +279,11 @@ So the mention stays, and a real request is added alongside it. **A PR is not co 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 @@ -281,6 +291,7 @@ 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) @@ -291,25 +302,44 @@ if [[ -z "$REVIEWERS" ]]; then REVIEWERS="$FALLBACK_REVIEWER" fi -# 3. Make the request. -gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$REVIEWERS" || - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" +# 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. 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") +fi -# 4. Verify it landed. gh exits 0 even when it silently skips a reviewer it -# cannot assign (no repo access, a bad handle, or the PR author themselves), -# so confirm against the PR rather than trusting the exit code. +# 5. Read back and compare against what was resolved. `gh` can exit 0 while +# 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 \ - --json reviewRequests --jq '[.reviewRequests[].login // .reviewRequests[].name] | join(",")') + --json reviewRequests --jq '[.reviewRequests[] | .login // .slug // .name] | join(",")') +if (( ${#GOT[@]} < ${#WANT[@]} )); then + echo "warning: requested ${#GOT[@]}/${#WANT[@]} resolved reviewers on PR $PR" +fi if [[ -z "$REQUESTED" ]]; then - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" - REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ - --json reviewRequests --jq '[.reviewRequests[].login // .reviewRequests[].name] | join(",")') + echo "ERROR: no reviewer requested on PR $PR" + exit 1 fi -[[ -n "$REQUESTED" ]] || { echo "ERROR: no reviewer requested on PR $PR"; exit 1; } echo "Requested reviewers: $REQUESTED" ``` -If even the fallback cannot be assigned, report it as a failure of the run. Do not close out a PR whose requested-reviewers list is empty. +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. diff --git a/.agents/skills/create_pr/check_pr_body.py b/.agents/skills/create_pr/check_pr_body.py index 8e9653db4..693b4ec9b 100644 --- a/.agents/skills/create_pr/check_pr_body.py +++ b/.agents/skills/create_pr/check_pr_body.py @@ -23,11 +23,13 @@ * Duplicate heading - the same Markdown heading text appearing more than once. * Required heading - (optional) assert specific headings are present exactly once, for skills that emit a fixed body template. - * Lead section - (optional) assert a heading is the FIRST heading in the + * Lead section - (optional) assert a heading is the FIRST content in the body, has prose under it, and stays within a word budget. Drafting PRs must open with a plain-language summary of what the feature does for the user, so a reviewer learns - that before any pipeline bookkeeping. + that before any pipeline bookkeeping. Position is checked + against content, not just headings, so a body cannot open + with unheaded spec/workflow/run-ID preamble. Usage: python3 check_pr_body.py /tmp/pr-body.md @@ -95,8 +97,14 @@ def find_repeated_span(text: str) -> Optional[Tuple[str, int]]: def _iter_non_code_lines(lines: List[str]): - """Yield (line_num, text) for lines outside fenced code blocks.""" + """Yield (line_num, text) for lines outside fenced code blocks and HTML comments. + + HTML comments are skipped for the same reason code fences are: a `##` line or a + stray backtick inside `` is commentary, not real body content. PR + bodies carry machine-managed comment banners, so this is a live case. + """ fence: Optional[str] = None + in_comment = False for line_num, line in enumerate(lines, start=1): fence_match = re.match(r"^\s*(`{3,}|~{3,})", line) if fence is not None: @@ -106,7 +114,55 @@ def _iter_non_code_lines(lines: List[str]): if fence_match: fence = fence_match.group(1) continue - yield line_num, line + + visible, in_comment = _strip_html_comments(line, in_comment) + if not visible.strip(): + # Either a genuinely blank line or a line that was entirely comment. + # Yield blanks so callers still see the line, but drop comment-only lines. + if in_comment or visible != line: + continue + yield line_num, visible + + +def _strip_html_comments(line: str, in_comment: bool) -> Tuple[str, bool]: + """Remove HTML-comment spans from one line. Returns (visible_text, still_open).""" + out = [] + rest = line + while rest: + if in_comment: + end = rest.find("-->") + if end == -1: + rest = "" + break + rest = rest[end + 3 :] + in_comment = False + else: + start = rest.find("\n" + "\n\n" + f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n" + ) + self.assertEqual(cpb.check_lead_section(lines(body), LEAD), []) + + def test_multiline_html_comment_heading_is_not_a_heading(self): + """A `##` line inside a multi-line comment must not displace the lead section.""" + body = ( + "\n\n" + f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n" + ) + self.assertEqual(cpb.check_lead_section(lines(body), LEAD), []) + + def test_crlf_line_endings_are_handled(self): + """A body round-tripped through the GitHub API can arrive with CRLF endings.""" + self.assertEqual( + cpb.check_lead_section(lines(GOOD_BODY.replace("\n", "\r\n")), LEAD), [] + ) + + def test_crlf_body_still_detects_a_real_violation(self): + """CRLF handling must not be so lenient that it stops catching problems.""" + body = f"## Summary\r\n\r\nBookkeeping.\r\n\r\n{LEAD}\r\n\r\nIt ships in `v1`.\r\n" + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1, problems) + self.assertIn("lead section is not first", problems[0]) + + def test_code_fence_above_the_lead_section_fails(self): + """A fenced block before the summary is content and pushes it below the fold.""" + body = ( + "```bash\nsome --command\n```\n\n" + f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n" + ) + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1, problems) + self.assertIn("lead section is not first", problems[0]) + def test_lead_section_with_no_content(self): body = f"{LEAD}\n\n## Summary\n\nAuto-drafted documentation.\n" problems = cpb.check_lead_section(lines(body), LEAD) @@ -92,11 +149,15 @@ def test_duplicate_lead_section(self): problems = cpb.check_lead_section(lines(body), LEAD) self.assertTrue(any("appears 2x" in p for p in problems), problems) - def test_heading_inside_a_code_fence_does_not_count_as_first(self): - """A fenced example of the template must not satisfy or displace the check.""" + def test_heading_inside_a_code_fence_is_not_a_real_heading(self): + """A fenced example of another section must not count as a duplicate or a heading. + + The fence sits below the lead section here; a fence *above* it is real content + and is covered by test_code_fence_above_the_lead_section_fails. + """ body = ( - "```markdown\n## Summary\nan example\n```\n\n" - f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n" + f"{LEAD}\n\nIt delegates workspace management. Shipped in `v1` (2026-08-18).\n\n" + "```markdown\n## What this feature does\na fenced example of this very section\n```\n" ) self.assertEqual(cpb.check_lead_section(lines(body), LEAD), []) diff --git a/.agents/skills/missing_docs/SKILL.md b/.agents/skills/missing_docs/SKILL.md index 77c30dd6e..8f9b8ce8e 100644 --- a/.agents/skills/missing_docs/SKILL.md +++ b/.agents/skills/missing_docs/SKILL.md @@ -441,30 +441,22 @@ with the product. Each run: is exactly how #414–#417 ended up with zero reviews. **A PR is not complete until `gh pr edit --add-reviewer` has succeeded and the - requested-reviewers list is non-empty.** A resolution failure falls back to - `dannyneira`; it never no-ops. This matches the fallback in + requested reviewers read back as the owners you resolved.** A resolution failure + falls back to `dannyneira`; it never no-ops. This matches the fallback in `.github/workflows/release-docs-update.yml` (the "Assign last docs PR reviewer" step). - ```bash - PR= - FALLBACK_REVIEWER=dannyneira - - 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) - [[ -z "$REVIEWERS" ]] && REVIEWERS="$FALLBACK_REVIEWER" - - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$REVIEWERS" || - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" - - # gh exits 0 even when it silently skips a reviewer it cannot assign, so verify. - REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ - --json reviewRequests --jq '[.reviewRequests[].login // .reviewRequests[].name] | join(",")') - [[ -n "$REQUESTED" ]] || { echo "ERROR: no reviewer requested on PR $PR"; exit 1; } - echo "PR $PR reviewers: $REQUESTED" - ``` + + **Use the snippet in the `create_pr` skill under "Request reviewers (required)" — + it is the canonical copy; do not paste a second version here.** It requests each + reviewer in a separate `gh` call (a comma-joined call is atomic, so one + unassignable entry drops every valid owner with it) and verifies the read-back + against the resolved set rather than merely against empty. Feed it the reviewers + from `suggest_reviewers.py --reviewers-only`, using the source files behind the + addressed findings. + Keep the prose `/cc @engineer` mention in the body as well — this adds the real - request, it does not replace the mention. Report any PR whose requested-reviewers list - is still empty as a run failure. + request, it does not replace the mention. Report any PR whose requested-reviewers + list is empty as a run failure, and any PR that got only some of its resolved + owners as a partial result worth naming in the run output. 8. **Open one PR per feature** following the PR strategy above (not a single mega PR): one focused PR per documented feature (grouping only features that share a doc file or owner), each carrying its content design plan as a section in the PR body, plus a diff --git a/.agents/skills/missing_docs/scripts/suggest_reviewers.py b/.agents/skills/missing_docs/scripts/suggest_reviewers.py index 83f575735..8c93d59ee 100755 --- a/.agents/skills/missing_docs/scripts/suggest_reviewers.py +++ b/.agents/skills/missing_docs/scripts/suggest_reviewers.py @@ -125,28 +125,38 @@ def main(): return 0 def report(message=""): - """Print human-readable progress, suppressed under --reviewers-only.""" + """Print human-readable progress on stdout, suppressed under --reviewers-only.""" if not quiet: print(message) + def diagnose(message): + """Report a resolution problem. + + Under --reviewers-only this goes to stderr, so `$(...)` still captures only + the reviewer list while the run log keeps a record of why a fallback + happened. A silent fallback is indistinguishable from a correct resolution + when you are reading the log afterwards. + """ + print(message, file=sys.stderr if quiet else sys.stdout) + users, teams = [], [] unresolved = [] report("Reviewer resolution:") for item in inputs: if ":" not in item: unresolved.append(item) - report(f" ? {item} — missing repo prefix (use warp: or warp-server:)") + diagnose(f" ? {item} — missing repo prefix (use warp: or warp-server:)") continue repo, rel = item.split(":", 1) rules = repos.get(repo) if rules is None: unresolved.append(item) - report(f" ? {item} — no ownership file loaded for repo '{repo}'") + diagnose(f" ? {item} — no ownership file loaded for repo '{repo}'") continue owners, pattern = owners_for(rel, rules) if not owners: unresolved.append(item) - report(f" ? {repo}:{rel} — no owner match") + diagnose(f" ? {repo}:{rel} — no owner match") continue report(f" - {repo}:{rel} -> {' '.join(owners)} (matched: {pattern})") for o in owners: @@ -160,10 +170,16 @@ def report(message=""): joined = ",".join(review_args) if quiet: - # Sole output: the --add-reviewer argument, or nothing at all. An empty - # result is the caller's cue to use the fallback reviewer, never to skip - # the request. - if joined: + # Sole *stdout* output: the --add-reviewer argument, or nothing at all. An + # empty result is the caller's cue to use the fallback reviewer, never to + # skip the request. Diagnostics already went to stderr. + if not joined: + print( + "suggest_reviewers: no owners resolved from " + f"{len(inputs)} path(s); caller must use its fallback reviewer.", + file=sys.stderr, + ) + else: print(joined) return 0 diff --git a/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py b/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py index dba27740c..012fd3b49 100755 --- a/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py +++ b/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py @@ -196,6 +196,33 @@ def test_reviewers_only_is_empty_when_nothing_resolves(self): ) self.assertEqual(result.returncode, 0, result.stderr) self.assertEqual(result.stdout, "") + # A silent fallback is indistinguishable from a correct resolution when you + # read the log afterwards, so the reason must still surface on stderr. + self.assertIn("no owner match", result.stderr) + self.assertIn("no owners resolved", result.stderr) + + def test_reviewers_only_keeps_stdout_clean_when_diagnosing(self): + """Diagnostics must not leak into the captured reviewer list.""" + with tempfile.TemporaryDirectory() as d: + warp = Path(d) / "warp" + self._make_repo(warp, "/app/src/settings/ @lucie\n") + result = subprocess.run( + [ + sys.executable, + str(_MODULE_PATH), + "--reviewers-only", + "--warp", + str(warp), + "warp:app/src/settings/ssh.rs", # resolves + "warp:crates/nothing/owns/this.rs", # does not + ], + capture_output=True, + text=True, + stdin=subprocess.DEVNULL, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, "lucie\n") + self.assertIn("no owner match", result.stderr) def test_warp_internal_alias(self): with tempfile.TemporaryDirectory() as d: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 542e5df55..045721b1e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,7 @@ jobs: run: | python3 .agents/skills/missing_docs/scripts/test_suggest_reviewers.py python3 .agents/skills/missing_docs/scripts/test_audit_docs.py + python3 .agents/skills/missing_docs/scripts/test_check_new_release.py # Stdlib-only tests for the PR body integrity checker, including the # feature-summary lead section that drafting PRs must open with.