From 95dcf6bc1b79957d1325b573ec86d07fda184cf8 Mon Sep 17 00:00:00 2001 From: Warp Date: Fri, 21 Aug 2026 00:07:57 +0000 Subject: [PATCH 1/6] 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 | 65 ++++++-- .../missing_docs/scripts/suggest_reviewers.py | 65 ++++++-- .../scripts/test_suggest_reviewers.py | 47 ++++++ .github/workflows/ci.yml | 5 + 7 files changed, 481 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 977d537dd..d32846429 100644 --- a/.agents/skills/missing_docs/SKILL.md +++ b/.agents/skills/missing_docs/SKILL.md @@ -365,7 +365,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 @@ -458,17 +462,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 @@ -490,13 +523,17 @@ 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 .agents/skills/missing_docs/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 +> .agents/skills/missing_docs/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 99e3330645a88731bd927473089c27c2a293f60c Mon Sep 17 00:00:00 2001 From: Warp Date: Fri, 21 Aug 2026 00:38:58 +0000 Subject: [PATCH 2/6] =?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 d32846429..6c777fc08 100644 --- a/.agents/skills/missing_docs/SKILL.md +++ b/.agents/skills/missing_docs/SKILL.md @@ -469,30 +469,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. From b5a9aefe1d6817b42783ab9503cd5f1d2c6224ba Mon Sep 17 00:00:00 2001 From: Rachael Rose Renk <91027132+rachaelrenk@users.noreply.github.com> Date: Mon, 24 Aug 2026 17:19:47 -0600 Subject: [PATCH 3/6] create_pr: stop the fallback reviewer from masking a dropped owner 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 --- .agents/skills/create_pr/SKILL.md | 33 ++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index 2aa6f7824..ba1d52297 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -296,10 +296,13 @@ 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. +# 2. Never let an empty resolution drop the request. Track that this was a +# fallback so step 5 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. @@ -314,9 +317,13 @@ for R in "${WANT[@]}"; do done # 4. If nothing at all landed, fall back rather than ship an unreviewed PR. +# Keep the fallback OUT of GOT. GOT answers "which resolved owners did I +# actually request", and counting the fallback there makes step 5 pass on a +# run where every real owner was dropped — the exact silent failure this +# section exists to prevent. if (( ${#GOT[@]} == 0 )); then - gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" && - GOT+=("$FALLBACK_REVIEWER") + gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" || + echo "warning: fallback $FALLBACK_REVIEWER could not be requested either" fi # 5. Read back and compare against what was resolved. `gh` can exit 0 while @@ -325,13 +332,25 @@ fi # silently drops them from a mixed list. REQUESTED=$(gh pr view "$PR" --repo warpdotdev/docs \ --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 - echo "ERROR: no reviewer requested on PR $PR" + echo "ERROR: no reviewer requested on PR $PR - not even the fallback landed" exit 1 fi + +if (( RESOLUTION_WAS_EMPTY )); then + # Nothing resolved, so the fallback is the intended outcome, not a gap. + echo "note: no owner resolved for PR $PR; fallback $FALLBACK_REVIEWER requested" +elif (( ${#GOT[@]} == 0 )); then + # Owners resolved and every one was rejected. The PR has a reviewer, but not + # the right one, and that must not read as success. + echo "ERROR: none of the ${#WANT[@]} resolved owners could be requested on PR $PR" \ + "(wanted: ${WANT[*]}); only the fallback is assigned. Report this run as failed." + exit 1 +elif (( ${#GOT[@]} < ${#WANT[@]} )); then + echo "warning: requested ${#GOT[@]}/${#WANT[@]} resolved owners on PR $PR" \ + "(got: ${GOT[*]}); name the missing owners and why in the run output" +fi + echo "Requested reviewers: $REQUESTED" ``` From 870e3af19b6af7c2e81fa65448f7c72e4db0b51b Mon Sep 17 00:00:00 2001 From: Rachael Rose Renk <91027132+rachaelrenk@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:07:17 -0600 Subject: [PATCH 4/6] create_pr: trust the read-back, and fix comment/fence ordering 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> --- .agents/skills/create_pr/SKILL.md | 60 ++++++++++++------- .agents/skills/create_pr/check_pr_body.py | 26 ++++++-- .../skills/create_pr/test_check_pr_body.py | 58 ++++++++++++++++++ 3 files changed, 118 insertions(+), 26 deletions(-) diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index ba1d52297..165524098 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -316,39 +316,59 @@ for R in "${WANT[@]}"; do fi done -# 4. If nothing at all landed, fall back rather than ship an unreviewed PR. -# Keep the fallback OUT of GOT. GOT answers "which resolved owners did I -# actually request", and counting the fallback there makes step 5 pass on a -# run where every real owner was dropped — the exact silent failure this -# section exists to prevent. -if (( ${#GOT[@]} == 0 )); then +# 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. If nothing actually landed, fall back rather than ship an unreviewed PR, +# then read back again. Keying this off the read-back rather than GOT +# matters: when gh exits 0 for every owner but requests none of them, a +# GOT-based check skips the fallback and leaves the PR with no reviewer. +if [[ -z "$REQUESTED" ]]; then gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$FALLBACK_REVIEWER" || - echo "warning: fallback $FALLBACK_REVIEWER could not be requested either" + echo "warning: fallback $FALLBACK_REVIEWER could not be requested" + REQUESTED=$(read_requested) fi -# 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 // .slug // .name] | join(",")') if [[ -z "$REQUESTED" ]]; then - echo "ERROR: no reviewer requested on PR $PR - not even the fallback landed" + echo "ERROR: no reviewer is on PR $PR - not even the fallback landed" exit 1 fi +# 6. Compare the read-back against what was resolved. 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'; } +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 # Nothing resolved, so the fallback is the intended outcome, not a gap. echo "note: no owner resolved for PR $PR; fallback $FALLBACK_REVIEWER requested" -elif (( ${#GOT[@]} == 0 )); then - # Owners resolved and every one was rejected. The PR has a reviewer, but not +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 could be requested on PR $PR" \ + 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 (( ${#GOT[@]} < ${#WANT[@]} )); then - echo "warning: requested ${#GOT[@]}/${#WANT[@]} resolved owners on PR $PR" \ - "(got: ${GOT[*]}); name the missing owners and why in the run output" +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" diff --git a/.agents/skills/create_pr/check_pr_body.py b/.agents/skills/create_pr/check_pr_body.py index 693b4ec9b..224976d99 100644 --- a/.agents/skills/create_pr/check_pr_body.py +++ b/.agents/skills/create_pr/check_pr_body.py @@ -106,21 +106,35 @@ def _iter_non_code_lines(lines: List[str]): 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) + # Inside a code fence, only a matching fence closes it. `` and everything after it — including the + # lead heading — so a valid body fails the check. + was_in_comment = in_comment 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: + # Yield blanks so callers still see the line, but drop comment-only + # lines, including the one carrying the closing `-->`. + if was_in_comment or in_comment or visible != line: continue + yield line_num, visible + continue + + fence_match = re.match(r"^\s*(`{3,}|~{3,})", visible) + if fence_match: + fence = fence_match.group(1) + continue + yield line_num, visible diff --git a/.agents/skills/create_pr/test_check_pr_body.py b/.agents/skills/create_pr/test_check_pr_body.py index a6e34fd22..42e325a7e 100644 --- a/.agents/skills/create_pr/test_check_pr_body.py +++ b/.agents/skills/create_pr/test_check_pr_body.py @@ -161,6 +161,64 @@ def test_heading_inside_a_code_fence_is_not_a_real_heading(self): ) self.assertEqual(cpb.check_lead_section(lines(body), LEAD), []) + def test_balanced_code_fence_inside_an_html_comment_is_fine(self): + """A balanced fence inside a comment. Passed before the fix, by luck. + + Fence detection ran before comment stripping, so the ``` lines opened + and closed a phantom block that happened to end before the `-->`. Kept + as a guard: the fix must not break the case that already worked. + """ + 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_unbalanced_fence_inside_an_html_comment_does_not_swallow_the_body(self): + """The actual regression: an odd number of ``` lines inside a comment. + + Fence detection ran before comment stripping, so a lone ``` opened a + phantom block that then ate the closing `-->` and every line after it, + including the real lead heading. The body was valid; the check reported + "missing required lead section", which reads as an authoring mistake + rather than a parser bug. This is the only one of these four that fails + against the pre-fix parser. + """ + 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_comment_marker_inside_a_code_fence_does_not_open_a_comment(self): + """The mirror case. Stripping comments first would break this instead. + + `\n\n## Summary\n\nNo real lead section here.\n" + ) + problems = cpb.check_lead_section(lines(body), LEAD) + self.assertEqual(len(problems), 1, problems) + self.assertIn("missing required lead section", problems[0]) + class TestMainExitCodes(unittest.TestCase): def _write(self, tmpdir: Path, text: str) -> str: From 82dfdf7ceb7975d6625d268317a3cdb15c568f5b Mon Sep 17 00:00:00 2001 From: "warp-agent-staging[bot]" <240773466+warp-agent-staging[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:02:56 +0000 Subject: [PATCH 5/6] create_pr: verify the fallback reviewer by name, not by read-back emptiness 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 --- .agents/skills/create_pr/SKILL.md | 49 +++-- .../create_pr/test_request_reviewers.py | 167 ++++++++++++++++++ .github/workflows/ci.yml | 6 + 3 files changed, 211 insertions(+), 11 deletions(-) create mode 100644 .agents/skills/create_pr/test_request_reviewers.py diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index 165524098..d8fccd592 100644 --- a/.agents/skills/create_pr/SKILL.md +++ b/.agents/skills/create_pr/SKILL.md @@ -297,7 +297,7 @@ REVIEWERS=$(python3 .agents/skills/missing_docs/scripts/suggest_reviewers.py \ 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 5 does not report it as an owner who was requested. +# 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" @@ -328,11 +328,35 @@ read_requested() { } REQUESTED=$(read_requested) -# 5. If nothing actually landed, fall back rather than ship an unreviewed PR, -# then read back again. Keying this off the read-back rather than GOT -# matters: when gh exits 0 for every owner but requests none of them, a -# GOT-based check skips the fallback and leaves the PR with no reviewer. -if [[ -z "$REQUESTED" ]]; then +# 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) @@ -342,11 +366,13 @@ 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 -# 6. Compare the read-back against what was resolved. 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'; } +# 7. Compare the read-back against what was resolved. IFS=',' read -ra HAVE <<< "$REQUESTED" MISSING=() for R in "${WANT[@]}"; do @@ -358,7 +384,8 @@ for R in "${WANT[@]}"; do done if (( RESOLUTION_WAS_EMPTY )); then - # Nothing resolved, so the fallback is the intended outcome, not a gap. + # 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 diff --git a/.agents/skills/create_pr/test_request_reviewers.py b/.agents/skills/create_pr/test_request_reviewers.py new file mode 100644 index 000000000..ee93ec9d1 --- /dev/null +++ b/.agents/skills/create_pr/test_request_reviewers.py @@ -0,0 +1,167 @@ +#!/usr/bin/env python3 +"""Regression tests for the reviewer-request snippet in create_pr/SKILL.md. + +The tests extract the documented bash snippet and run it against stubbed `gh` +and `suggest_reviewers.py` commands. This exercises the text users copy rather +than a paraphrased implementation. + +Run with: python3 .agents/skills/create_pr/test_request_reviewers.py +""" + +import json +import os +import re +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + + +HERE = Path(__file__).resolve().parent +SKILL = HERE / "SKILL.md" + +GH_STUB = """#!/usr/bin/env python3 +import json +import os +import sys +from pathlib import Path + +state_file = Path(os.environ["GH_STUB_STATE"]) +calls_file = Path(os.environ["GH_STUB_CALLS"]) +reject = set(filter(None, os.environ.get("GH_STUB_REJECT", "").split(","))) +args = sys.argv[1:] + +with calls_file.open("a", encoding="utf-8") as stream: + stream.write(json.dumps(args) + "\\n") + +state = json.loads(state_file.read_text(encoding="utf-8")) +if args[:2] == ["pr", "edit"]: + reviewer = args[args.index("--add-reviewer") + 1] + if reviewer in reject: + sys.exit(1) + if reviewer not in state: + state.append(reviewer) + state_file.write_text(json.dumps(state), encoding="utf-8") + sys.exit(0) +if args[:2] == ["pr", "view"]: + print(",".join(state)) + sys.exit(0) +sys.exit(1) +""" + +RESOLVER_STUB = """#!/usr/bin/env python3 +import os +import sys +sys.stdout.write(os.environ.get("STUB_REVIEWERS", "")) +""" + + +def extract_reviewer_snippet(): + """Extract the bash fence whose first two assignments identify the snippet.""" + text = SKILL.read_text(encoding="utf-8") + match = re.search( + r"```bash\n(PR=123\nFALLBACK_REVIEWER=dannyneira\n.*?)(?=\n```)", + text, + re.DOTALL, + ) + if not match: + raise AssertionError("reviewer-request snippet not found in SKILL.md") + return match.group(1) + + +class ReviewerSnippetTest(unittest.TestCase): + def run_snippet(self, *, initial=(), resolved="", reject=""): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + bin_dir = root / "bin" + bin_dir.mkdir() + gh = bin_dir / "gh" + gh.write_text(GH_STUB, encoding="utf-8") + gh.chmod(0o755) + + resolver = ( + root / ".agents/skills/missing_docs/scripts/suggest_reviewers.py" + ) + resolver.parent.mkdir(parents=True) + resolver.write_text(RESOLVER_STUB, encoding="utf-8") + resolver.chmod(0o755) + + state_file = root / "state.json" + state_file.write_text(json.dumps(list(initial)), encoding="utf-8") + calls_file = root / "calls.jsonl" + calls_file.write_text("", encoding="utf-8") + + env = os.environ.copy() + env.update( + { + "PATH": f"{bin_dir}{os.pathsep}{env['PATH']}", + "GH_STUB_STATE": str(state_file), + "GH_STUB_CALLS": str(calls_file), + "GH_STUB_REJECT": reject, + "STUB_REVIEWERS": resolved, + } + ) + result = subprocess.run( + ["bash", "-c", extract_reviewer_snippet()], + cwd=root, + env=env, + capture_output=True, + text=True, + ) + state = json.loads(state_file.read_text(encoding="utf-8")) + calls = [ + json.loads(line) + for line in calls_file.read_text(encoding="utf-8").splitlines() + ] + return result, state, calls + + @staticmethod + def requested_reviewers(calls): + return [ + call[call.index("--add-reviewer") + 1] + for call in calls + if call[:2] == ["pr", "edit"] + ] + + def test_resolved_owner_lands(self): + result, state, calls = self.run_snippet(resolved="alice") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["alice"]) + self.assertEqual(self.requested_reviewers(calls), ["alice"]) + + def test_empty_resolution_requests_fallback(self): + result, state, calls = self.run_snippet() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(state, ["dannyneira"]) + self.assertEqual(self.requested_reviewers(calls), ["dannyneira"]) + + def test_unrelated_existing_reviewer_does_not_skip_fallback(self): + result, state, calls = self.run_snippet(initial=["carol"]) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("dannyneira", self.requested_reviewers(calls)) + self.assertEqual(set(state), {"carol", "dannyneira"}) + self.assertIn( + "no owner resolved for PR 123; fallback dannyneira requested", + result.stdout, + ) + + def test_unrelated_reviewer_does_not_mask_fallback_failure(self): + result, state, calls = self.run_snippet( + initial=["carol"], reject="dannyneira" + ) + self.assertIn("dannyneira", self.requested_reviewers(calls)) + self.assertEqual(state, ["carol"]) + self.assertNotEqual(result.returncode, 0) + self.assertIn( + "ERROR: fallback dannyneira could not be requested on PR 123", + result.stdout, + ) + self.assertNotIn("note: no owner resolved", result.stdout) + + +if __name__ == "__main__": + if sys.platform.startswith("win"): + print("skipping: reviewer snippet requires bash") + sys.exit(0) + unittest.main() diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 045721b1e..d997a77c4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,6 +53,12 @@ jobs: - name: Test create_pr body checker run: python3 .agents/skills/create_pr/test_check_pr_body.py + # Extracts the documented reviewer-request bash snippet from SKILL.md and + # runs it against a stubbed gh/suggest_reviewers.py, so CI exercises the + # exact text agents copy rather than a paraphrase of it. + - name: Test create_pr reviewer-request snippet + run: python3 .agents/skills/create_pr/test_request_reviewers.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 cab58805eae38d2e84e69409728dfefc9adefe46 Mon Sep 17 00:00:00 2001 From: Rachael Rose Renk <91027132+rachaelrenk@users.noreply.github.com> Date: Tue, 25 Aug 2026 11:28:46 -0600 Subject: [PATCH 6/6] missing_docs: fix the three sandbox footguns validation run #2 hit 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 --- .agents/skills/missing_docs/SKILL.md | 53 +++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/.agents/skills/missing_docs/SKILL.md b/.agents/skills/missing_docs/SKILL.md index 6c777fc08..1b720e5da 100644 --- a/.agents/skills/missing_docs/SKILL.md +++ b/.agents/skills/missing_docs/SKILL.md @@ -33,6 +33,29 @@ audits in the report's `audits_skipped` field (`extraction:*` entries identify broken parsers). Never treat an exit-2 run as a clean audit — fix the problem and re-run. Exit 0 means all requested audits ran (findings may still exist). +### Run every command from the docs repo root + +Every path in this skill — scripts, references, doc pages — is relative to the docs +repo root, and nothing resolves them for you. A sandbox commonly starts a run one level +up (`/workspace`, with the checkout at `/workspace/docs`), so `cd` before anything else: + +```bash +cd "$(git rev-parse --show-toplevel)" +``` + +A wrong working directory fails in a way that reads like a real failure: `python3` exits +**2** with `can't open file`, the same exit code `audit_docs.py` uses to fail loud on a +broken environment. Read the message before concluding a sanity guard tripped. + +### Install Node dependencies before the first build + +`npm run build` is the only validation this repo has, and it needs `node_modules`, which +a fresh sandbox does not have. Install once per sandbox: + +```bash +npm ci +``` + ## Public vs. private surfaces (what you may document) Only document surfaces that are **publicly released**. This is the most important guardrail in this skill: do not reveal private or unreleased surfaces in public docs. Two independent gates, both required: @@ -315,6 +338,15 @@ For each gap to address (prioritize high → medium → low): repeat findings, and an unmaintained map is how gaps get lost. Per the PR strategy below, collect all map edits into the single companion audit-bookkeeping PR (only fold them into a feature PR when the run documents exactly one feature). + + **Edit map entries individually; never find-and-replace across the file.** The + left-hand side of every entry is a literal code identifier — a flag name, command, + route, setting key, or doc slug — and it only matches code because it matches exactly. + A rename sweep applied to the whole map (say, replacing `cost` with `usage` while + renaming a feature) rewrites unrelated keys into surfaces that do not exist. Map + hygiene catches the corruption on the next audit, but only after it has shipped in a + PR. Change the entries you mean to change, then re-run `--category map` to confirm + nothing else moved. 9. Run `--update-snapshot` and commit the refreshed `surface_snapshot.json` in that same bookkeeping PR. Never split the snapshot across multiple PRs. @@ -397,10 +429,10 @@ their area. Do NOT bundle unrelated features into a single mega PR. - **API spec gaps stay separate** — released endpoints go through the `sync-openapi-spec` skill as their own change, never bundled into a feature PR. - **Validate once, then split.** Run `npm run build` on the combined working tree (all - features together) to confirm everything compiles, then peel each feature onto its own - branch off `main` (e.g. `git checkout -b ` then - `git checkout -- `). Each feature branch is then a strict subset - of the already-validated tree. + features together) to confirm everything compiles — `npm ci` first if the sandbox has + no `node_modules` — then peel each feature onto its own branch off `main` (e.g. + `git checkout -b ` then `git checkout -- `). Each + feature branch is then a strict subset of the already-validated tree. - List any deferred findings in the most relevant PR body (or the bookkeeping PR) so nothing is silently dropped. @@ -417,7 +449,9 @@ with the product. Each run: ``` Exit `0` means a new stable release is available — continue. Exit `10` means no new release; record the no-op outcome in run output and **stop**. Exit `1` is a fetch or - parse failure; report it and stop rather than proceeding as if nothing shipped. + parse failure; report it and stop rather than proceeding as if nothing shipped. Exit + `2` with `can't open file` is not a gate outcome at all — it is `python3` reporting the + wrong working directory. `cd` to the docs repo root and re-run. The gate also prints any `oz_updates` bullets for the release. Keep them — they are platform-side changes the audit cannot see, and this is the only place they surface. @@ -460,8 +494,9 @@ with the product. Each run: python3 .agents/skills/missing_docs/scripts/check_new_release.py --commit python3 .agents/skills/missing_docs/scripts/audit_docs.py --update-snapshot ``` -6. **Validate**: `npm run build` if doc pages changed; re-run the audit and confirm - the addressed findings are gone. +6. **Validate**: if doc pages changed, run `npm ci && npm run build` — a fresh sandbox + has no `node_modules`, and the build is the only validation this repo has. Then + re-run the audit and confirm the addressed findings are gone. 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 @@ -501,7 +536,9 @@ run. Recommended scheduled-agent prompt (copy when setting up the agent): -> Run the missing_docs skill in drift-watch mode. First run +> Run the missing_docs skill in drift-watch mode. Work from the docs repo root — every +> path below is relative to it, and a python exit code of 2 with "can't open file" means +> you are in the wrong directory, not that a check failed. First run > .agents/skills/missing_docs/scripts/check_new_release.py; if it reports no new stable > release, record the no-op outcome and stop. Otherwise use the audit script with > explicit --warp (public warpdotdev/warp checkout) and --warp-server paths and --diff.