diff --git a/.agents/skills/create_pr/SKILL.md b/.agents/skills/create_pr/SKILL.md index 894cfd96e..2aa6f7824 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 content in the body, is empty, or exceeds the word budget. Position is checked against content rather than headings, so a body cannot open with a few unheaded lines of spec/workflow/run-ID preamble and still pass. Omit the section — and the flag — only for the small corrections listed under "When a plan can be skipped": typos, link fixes, terminology sweeps, generated updates, and screenshot swaps have no feature to summarize. ### Summary -Brief explanation of what the PR accomplishes and why. +Brief explanation of what the PR accomplishes and why. This is where the pipeline detail goes: the source spec, the generating workflow, the new page path, and the sidebar entry. ### Changes Bulleted list of specific changes, organized by file or area: @@ -203,8 +235,15 @@ Exit code 0 if PR exists, 1 if not. ::: ```bash -# 1. Write the description to a temp file using the create_file tool or a heredoc +# 1. Write the description to a temp file using the create_file tool or a heredoc. +# The `## What this feature does` block is DRAFTING-PR ONLY - drop it (and the +# --require-lead-section flag in step 2) for typos, link fixes, terminology +# sweeps, generated updates, and screenshot swaps. cat > /tmp/pr-body.md << 'EOF' +## What this feature does +One short paragraph: what the feature does for the user, ending with +shipped in `` (``). + ## Summary Description of changes @@ -215,16 +254,97 @@ 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). +python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md \ + --require-lead-section "## What this feature does" # drafting PRs only + +# For a non-drafting correction, run the check without the flag: +# python3 .agents/skills/create_pr/check_pr_body.py /tmp/pr-body.md # 3. Create the PR using the file (only if the check passed) gh pr create --title "docs: Add feature documentation" --body-file /tmp/pr-body.md +# 4. REQUIRED: request the reviewer for real (see "Request reviewers" below). +# The PR is not complete until this has succeeded. + # Open in browser to fill details gh pr create --web ``` +### Request reviewers (required) + +**Naming a reviewer in the body is not a review request.** A `/cc @engineer` mention notifies nobody through GitHub's review queue: the PR shows no requested reviewer, never appears in that engineer's "Review requested" filter, and quietly goes unreviewed. Every one of the four ambient-drafted docs PRs — #414, #415, #416, #417 — named reviewers in prose and received zero reviews; three had an empty requested-reviewers list and the fourth had a single reviewer added by hand. + +So the mention stays, and a real request is added alongside it. **A PR is not complete until `gh pr edit --add-reviewer` has succeeded and been verified.** + +A resolution failure must fall back, never no-op. When no owner resolves, assign `dannyneira`, matching the fallback the release docs workflow already uses (`.github/workflows/release-docs-update.yml`, "Assign last docs PR reviewer"). An unassignable reviewer is a problem to surface, not a reason to ship an unreviewed PR. + +Two details below are load-bearing, and getting either wrong reintroduces the silent drop this section exists to prevent: + +- **Request one reviewer per call.** `gh pr edit --add-reviewer a,b,c` sends a single atomic mutation, so one unassignable entry rejects the whole list. Since a resolution routinely mixes users with a team, and a team with no access to this repo cannot be requested here, a comma-joined call can fail wholesale and take every valid owner down with it. +- **Verify against the resolved set, not against emptiness.** "Is the list non-empty?" passes when the real owner was dropped and only the fallback landed, which looks identical to success. + +```bash +PR=123 +FALLBACK_REVIEWER=dannyneira + +# 1. Resolve the owning engineer(s). For missing_docs drift-watch runs, use the +# ownership resolver with the source files behind the change; see the +# missing_docs skill's "Reviewer routing" section for how to pick those files. +# Diagnostics go to stderr, so this captures only the reviewer list. +REVIEWERS=$(python3 .agents/skills/missing_docs/scripts/suggest_reviewers.py \ + --reviewers-only --warp ../warp --warp-server ../warp-server \ + warp:app/src/settings/ssh.rs < /dev/null) + +# 2. Never let an empty resolution drop the request. +if [[ -z "$REVIEWERS" ]]; then + echo "warning: no owner resolved - falling back to $FALLBACK_REVIEWER" + REVIEWERS="$FALLBACK_REVIEWER" +fi + +# 3. Request each reviewer separately so one bad entry cannot drop the rest. +IFS=',' read -ra WANT <<< "$REVIEWERS" +GOT=() +for R in "${WANT[@]}"; do + if gh pr edit "$PR" --repo warpdotdev/docs --add-reviewer "$R"; then + GOT+=("$R") + else + echo "warning: could not request $R on PR $PR" + fi +done + +# 4. 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 + +# 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 (( ${#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" + exit 1 +fi +echo "Requested reviewers: $REQUESTED" +``` + +A partial result is a reportable outcome, not a pass: if some owners could not be requested, say which ones and why in the run output, so the gap is visible rather than buried. If even the fallback cannot be assigned, report the run as failed. Do not close out a PR whose requested-reviewers list is empty. + +:::caution +A team handle resolved from `STAKEHOLDERS` or `CODEOWNERS` can only be requested on a repo that team has access to. `warpdotdev/oss-maintainers` is the root-rule owner in the warp client repo and therefore appears in most resolutions, but it has no access to `warpdotdev/docs`, so requesting it here fails. That is why step 3 requests one at a time. +::: + +:::note +Auto-requesting the review does not make it *block* merge. Whether an ambient docs PR should require that approval through branch protection is an open question for the docs owner, not something this skill decides. +::: + ### Update an existing PR When updating the body of an existing PR, make the **smallest** change rather than regenerating the whole description from memory — re-emitting a long body is what invites repetition-loop degeneration. Fetch the current body, apply a minimal or additive edit, verify it, then submit. @@ -245,8 +365,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 +390,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..693b4ec9b 100644 --- a/.agents/skills/create_pr/check_pr_body.py +++ b/.agents/skills/create_pr/check_pr_body.py @@ -23,6 +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 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. 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 @@ -30,6 +37,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 +61,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. @@ -83,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: @@ -94,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) + 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_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 = ( + 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), []) + + +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..8f9b8ce8e 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,38 @@ 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 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). + + **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 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 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 +487,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..8c93d59ee 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,79 @@ 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 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 = [] - 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:)") + 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) - print(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) - print(f" ? {repo}:{rel} — no owner match") + diagnose(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 *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 + + 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..012fd3b49 100755 --- a/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py +++ b/.agents/skills/missing_docs/scripts/test_suggest_reviewers.py @@ -150,6 +150,80 @@ 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, "") + # 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: warp = Path(d) / "warp" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e005713d..045721b1e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,12 @@ 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. + - 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.