diff --git a/.github/workflows/docs-pipeline.yml b/.github/workflows/docs-pipeline.yml index a21b98e8..ca85da90 100644 --- a/.github/workflows/docs-pipeline.yml +++ b/.github/workflows/docs-pipeline.yml @@ -5,6 +5,11 @@ name: Docs Pipeline # pipeline detects gaps, drafts + reviews the missing content, and opens a PR. # There is deliberately no schedule/cron trigger — spec change is the only # automatic entry point. (workflow_dispatch is a manual escape hatch.) +# +# Gaps stay open until their PR merges, so consecutive runs see the same ones. +# The pipeline skips gaps an open PR already covers and won't open a duplicate +# PR (pipeline/open_prs.py) — a run with nothing new to say exits cleanly having +# done nothing. GH_TOKEN below is what lets it read the open PRs. on: push: branches: [main] @@ -62,7 +67,8 @@ jobs: # Required — add this secret in repo Settings → Secrets → Actions. # The pipeline can't call Claude without it. ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - # ship.py / post_review.py use the gh CLI, which reads GH_TOKEN. + # ship.py / post_review.py and the open-PR duplicate check all use the + # gh CLI, which reads GH_TOKEN. GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml index 7aa0f189..5707b9d7 100644 --- a/.github/workflows/test-scripts.yml +++ b/.github/workflows/test-scripts.yml @@ -4,11 +4,13 @@ on: pull_request: paths: - "scripts/**" + - "pipeline/**" push: branches: - main paths: - "scripts/**" + - "pipeline/**" jobs: test: @@ -19,3 +21,14 @@ jobs: with: node-version: 22 - run: node --test scripts/*.test.mjs + + pipeline: + name: Test pipeline + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + - uses: actions/setup-python@v5 + with: + python-version: "3.12" + # Stdlib only — these tests stub out gh and never call the network. + - run: python -m unittest discover -s pipeline -p "test_*.py" -v diff --git a/pipeline/README.md b/pipeline/README.md index 0c46a50a..661dbb9f 100644 --- a/pipeline/README.md +++ b/pipeline/README.md @@ -30,6 +30,46 @@ pip install anthropic pyyaml export ANTHROPIC_API_KEY=... ``` +## Duplicate PRs + +A gap stays open until its PR **merges**, so every run re-detects the gaps that are +already waiting for review. Left alone, the pipeline opens one PR per run for the +same missing pages. + +`pipeline/open_prs.py` is the check that prevents that. It runs twice: + +- **`generate.py`** drops gaps an open PR already covers, before any model call, so a + duplicate run costs nothing. +- **`ship.py`** refuses to open a PR when an open one already covers the whole run. + +Both exit with code **3** ("nothing to do") instead of 0 or 1, and `run.py` stops the +chain cleanly when it sees it. + +A gap is "already in flight" if any open PR matches it on: + +| Signal | Source | Catches | +| --- | --- | --- | +| Gap key | `` marker in the PR body, written by `ship.py` | Any gap, including ones whose filename the model invents (two runs of the same gap produce different slugs) | +| File path | The PR's changed files | Gaps with a predictable target path, including PRs the pipeline didn't open | +| `covers:` frontmatter | The PR's diff | `missing_group_coverage` gaps, whose filename is unpredictable but whose group name is in the diff | + +Overrides, for when you want the PR anyway: + +```bash +python pipeline/generate.py --ignore-open-prs # draft claimed gaps too +python pipeline/ship.py --latest --force-new-pr # open a second PR +``` + +If `gh` can't be reached the check is skipped with a warning rather than blocking the +run, so a missing CLI never stops the pipeline (it just allows a duplicate). + +The logic has tests (stdlib only, no gh, no network), run in CI on any `pipeline/**` +change: + +```bash +python pipeline/test_open_prs.py +``` + ## Pipeline Steps The pipeline runs as a sequence of independent scripts. Each step reads the output of the previous one. You can run any step standalone, or use `run.py` to chain them all. @@ -64,8 +104,12 @@ python pipeline/generate.py --section admin # generate for one fami python pipeline/generate.py --type missing_orientation # generate one gap type python pipeline/generate.py --force # regenerate even if files exist python pipeline/generate.py --section admin --force # regenerate one section +python pipeline/generate.py --ignore-open-prs # include gaps an open PR covers ``` +Gaps that an open PR already covers are skipped before any model call. See +[Duplicate PRs](#duplicate-prs). + Each run creates a timestamped folder under `pipeline/drafts/` with the generated `.mdx` files and a `report.json` with metadata. ### 2b. Rework Existing Pages (Phase 2 alternative to Generate) @@ -137,10 +181,15 @@ python pipeline/ship.py --latest --dry-run # preview branch, commit python pipeline/ship.py --latest # create branch, commit, prompt before push python pipeline/ship.py --latest --yes # skip push confirmation python pipeline/ship.py --latest --branch docs/my-branch # custom branch name +python pipeline/ship.py --latest --force-new-pr # ship even if an open PR covers it ``` Requires `gh` CLI authenticated. Stages only `docs/**/*.mdx` and `docs.json`. Never force-pushes. +Exits 3 without creating a branch when an open PR already covers the whole run, and +stamps every PR body with the gap keys it covers so later runs recognize it. See +[Duplicate PRs](#duplicate-prs). + ### 7. Post Review (PR Suggestions) Posts "consider" findings from the review report as GitHub PR review comments with `suggestion` blocks. The reviewer can click "Apply suggestion" to accept changes directly in the PR. @@ -306,5 +355,5 @@ The live site structure (which page sits in which tab) is read from `docs.json` - Python 3.8+ - `anthropic` (for generate.py, rework.py, review.py, post_review.py) - `pyyaml` (for all scripts) -- `gh` CLI (for ship.py, post_review.py) +- `gh` CLI (for ship.py, post_review.py, and the open-PR check in generate.py) - `mint` / `npx` (optional, for broken-links check in promote.py) diff --git a/pipeline/generate.py b/pipeline/generate.py index 4f75c837..e9b1c8eb 100644 --- a/pipeline/generate.py +++ b/pipeline/generate.py @@ -12,6 +12,11 @@ python pipeline/generate.py --dry-run # show what would be generated python pipeline/generate.py --force # regenerate even if files exist python pipeline/generate.py --section admin --force # regenerate one section + python pipeline/generate.py --ignore-open-prs # draft gaps even if a PR is open for them + +Gaps that an open PR already covers are skipped before any model call — a gap is +only closed when its PR merges, so every run would otherwise redraft and reship +the same pages. See pipeline/open_prs.py. """ import argparse @@ -33,6 +38,7 @@ from util import build_authoring_system_prompt +from open_prs import EXIT_NOTHING_TO_DO, fetch_open_prs, split_claimed_gaps REPO_ROOT = Path(__file__).resolve().parent.parent OPENAPI_PATH = REPO_ROOT / "api-reference" / "openapi.yaml" @@ -292,6 +298,29 @@ def determine_output_path(gap, family_name, content=None): return None # missing_description handled inline; non-generative types skipped +def _predicted_path(gap): + """The repo-relative path a gap would write to, or None if unpredictable. + + Used only for open-PR matching. For missing_howto / missing_group_coverage the + filename comes from the title the model invents, so there's nothing to predict + and those gaps are matched on their gap key instead. + """ + out_path = determine_output_path(gap, gap.get("family", "unknown")) + if not out_path: + return None + try: + return str(out_path.relative_to(REPO_ROOT)) + except ValueError: + return None + + +def _pr_review_hint(claimed): + """One line pointing at the PRs worth reviewing, for the skip summary.""" + numbers = sorted({pr.get("number") for _, pr, _ in claimed if pr.get("number")}) + listed = ", ".join(f"#{n}" for n in numbers) + return f"Review or close {listed} to let the pipeline redraft these." + + def apply_description(gap, description): """Insert a frontmatter description into an existing page.""" page_path = REPO_ROOT / gap["path"] @@ -332,6 +361,10 @@ def main(): parser.add_argument("--dry-run", action="store_true", help="Show what would be generated") parser.add_argument("--force", action="store_true", help="Regenerate even when files already exist") parser.add_argument("--gap-report", help="Path to existing gap report JSON (skips re-running detection)") + parser.add_argument( + "--ignore-open-prs", action="store_true", + help="Draft every gap, even ones an open PR already covers (default: skip those)", + ) args = parser.parse_args() if args.gap_report: @@ -348,7 +381,33 @@ def main(): if not gaps: print("No gaps to generate for.") - return 0 + return EXIT_NOTHING_TO_DO + + # Drop gaps that an open PR already covers. Done here, before any model call, + # so a duplicate run costs nothing instead of a full generate → review cycle + # that ends in a PR nobody wants. + if args.ignore_open_prs: + print("Skipping the open-PR check (--ignore-open-prs).") + else: + prs, err = fetch_open_prs() + if prs is None: + print(f"Warning: could not check open PRs ({err}).") + print(" Proceeding without deduplication — this run may duplicate an open PR.") + else: + gaps, claimed = split_claimed_gaps( + gaps, prs, + path_for_gap=_predicted_path, + ) + if claimed: + print(f"Skipping {len(claimed)} gap(s) already covered by an open PR:") + for gap, pr, reason in claimed: + print(f" - {gap['type']} ({gap.get('family', 'site-wide')}): {reason}") + print(f" {_pr_review_hint(claimed)}") + + if not gaps: + print("\nEvery detected gap is already covered by an open PR. Nothing to do.") + print("Merge or close those PRs, then re-run the pipeline.") + return EXIT_NOTHING_TO_DO standards = yaml.safe_load(open(STANDARDS_PATH)) families = standards.get("families", {}) diff --git a/pipeline/open_prs.py b/pipeline/open_prs.py new file mode 100644 index 00000000..e8785dda --- /dev/null +++ b/pipeline/open_prs.py @@ -0,0 +1,286 @@ +"""Open-PR awareness: don't re-ship docs that already have a PR waiting. + +Every run of the pipeline re-detects gaps from scratch, and a gap stays open until +its PR *merges*. So the next spec change re-detects the same gaps and, without this +module, drafts and ships them again — one open PR per run, all saying the same thing. +This is the memory the pipeline otherwise lacks: it reads the repo's open PRs and +answers "is this gap already in flight?". + +Three independent signals, because none alone is enough: + + 1. **Gap keys**, from a machine-readable marker `ship.py` writes into the PR body. + Authoritative: it's the same identity `detect_gaps.py` produced. Required + because some gap types have no predictable output path — the model picks the + filename, so two runs of the *same* gap produce two different slugs + (`query-supported-languages-for-a-resource.mdx` vs + `...-for-a-deepl-api-resource.mdx`), which path matching alone can't connect. + 2. **File paths**, from each PR's changed files. Catches gaps whose target path + *is* predictable, and does so for PRs this pipeline didn't open — a human + writing the same page by hand, or a pipeline PR from before markers existed. + 3. **`covers:` frontmatter**, read from each PR's diff. `missing_group_coverage` + is detected by asking which endpoint groups no page declares it `covers` — + so a PR whose diff adds `covers: []` closes that gap the moment it + merges. This is signal 1's answer for PRs with no marker: unpredictable + filename, but the group name is right there in the diff. + +All three are best-effort: if `gh` can't be reached we say so and let the caller +proceed rather than block the pipeline on a missing CLI. +""" + +import json +import re + +from util import run_cmd, check_gh_available + + +# Exit code shared by generate.py and ship.py: "nothing to do, an open PR already +# covers this". Distinct from 0 (did work) and 1 (failed) so run.py can stop the +# chain cleanly instead of treating it as either success or breakage. +EXIT_NOTHING_TO_DO = 3 + +MARKER_RE = re.compile(r"", re.DOTALL) + +# Gap types that `covers:` frontmatter actually answers. detect_gaps raises +# missing_group_coverage by asking which groups no page declares it covers, so a +# PR adding that declaration closes exactly this gap — and nothing else. Other +# group-scoped types (a missing API Reference group, say) are about where a page +# sits in the nav, which `covers` says nothing about. +COVERS_GAP_TYPES = {"missing_group_coverage"} + +# PR fields we need in one `gh pr list` call — `files` included, so path matching +# costs no extra API round-trips per PR. +PR_JSON_FIELDS = "number,title,url,headRefName,body,files" + + +# --------------------------------------------------------------------------- # +# Gap identity # +# --------------------------------------------------------------------------- # + +def gap_key(gap): + """Stable identity for a gap, independent of when it was detected. + + Built only from what makes a gap *the same gap* — its type and what it's + about (family / endpoint group / page). Deliberately excludes run IDs, + timestamps, descriptions and generated filenames, all of which change + between runs of an unchanged gap. + """ + gap = gap or {} + parts = [ + gap.get("type") or "-", + gap.get("family") or "-", + gap.get("group") or "-", + gap.get("path") or "-", + ] + return ":".join( + re.sub(r"\s+", "-", str(p).strip().lower()) or "-" for p in parts + ) + + +def gap_keys_from_report(report): + """Gap keys for everything a run's report.json claims to have produced.""" + keys = set() + for entry in (report or {}).get("generated", []): + if entry.get("gap"): + keys.add(gap_key(entry["gap"])) + return keys + + +# --------------------------------------------------------------------------- # +# The PR-body marker # +# --------------------------------------------------------------------------- # + +def format_gap_marker(keys): + """Render gap keys as an HTML comment — invisible in the rendered PR body.""" + return f"" + + +def parse_gap_marker(body): + """Read gap keys back out of a PR body. Empty set if there's no marker.""" + keys = set() + for match in MARKER_RE.finditer(body or ""): + try: + parsed = json.loads(match.group(1)) + except json.JSONDecodeError: + continue + if isinstance(parsed, list): + keys.update(str(k) for k in parsed) + return keys + + +# --------------------------------------------------------------------------- # +# Reading the open PRs # +# --------------------------------------------------------------------------- # + +def fetch_open_prs(limit=100): + """Return (prs, error). `prs` is None when the check itself failed. + + None and [] mean different things and callers must treat them differently: + [] is "nothing in flight, go ahead"; None is "I don't know" and must never + be read as permission to open a duplicate. + """ + gh_ok, gh_err = check_gh_available() + if not gh_ok: + return None, gh_err + + result = run_cmd( + ["gh", "pr", "list", "--state", "open", "--limit", str(limit), + "--json", PR_JSON_FIELDS], + check=False, + ) + if result.returncode != 0: + return None, (result.stderr or "").strip() or "gh pr list failed" + + try: + prs = json.loads(result.stdout or "[]") + except json.JSONDecodeError as e: + return None, f"could not parse gh output: {e}" + + return prs, "" + + +def pr_label(pr): + return f"#{pr.get('number', '?')} ({pr.get('headRefName', '?')})" + + +def covers_claimed_by_open_prs(prs, exclude_branch=None): + """Return {group_name_lower: pr} for endpoint groups open PRs already cover. + + Reads each PR's diff for added `covers:` frontmatter — the same declaration + detect_gaps reads off merged pages to decide a group is covered. One `gh pr + diff` per PR, so this is called only when there's a group gap left to match. + """ + index = {} + for pr in prs or []: + number = pr.get("number") + if not number or (exclude_branch and pr.get("headRefName") == exclude_branch): + continue + # Cheap filter: only PRs that touch docs pages can declare `covers`. + if not any((f.get("path") or "").endswith(".mdx") for f in pr.get("files") or []): + continue + result = run_cmd(["gh", "pr", "diff", str(number)], check=False) + if result.returncode != 0: + continue + for group in _covers_in_diff(result.stdout): + index.setdefault(group.lower(), pr) + return index + + +COVERS_RE = re.compile(r"^\+\s*covers:\s*(.+?)\s*$", re.MULTILINE) + + +def _covers_in_diff(diff): + """Group names from added `covers:` frontmatter lines in a unified diff.""" + groups = [] + for raw in COVERS_RE.findall(diff or ""): + value = raw.strip() + if value.startswith("[") and value.endswith("]"): + value = value[1:-1] + for part in value.split(","): + name = part.strip().strip("\"'") + if name: + groups.append(name) + return groups + + +def index_open_prs(prs, exclude_branch=None): + """Build {gap_key: pr} and {file_path: pr} lookups from open PRs. + + `exclude_branch` skips the PR for the branch we're shipping to, so re-running + the pipeline onto an existing branch isn't mistaken for a duplicate of itself. + """ + by_key = {} + by_path = {} + for pr in prs or []: + if exclude_branch and pr.get("headRefName") == exclude_branch: + continue + for key in parse_gap_marker(pr.get("body")): + by_key.setdefault(key, pr) + for f in pr.get("files") or []: + path = f.get("path") + if path: + by_path.setdefault(path, pr) + return by_key, by_path + + +# --------------------------------------------------------------------------- # +# Answering "is this already in flight?" # +# --------------------------------------------------------------------------- # + +def split_claimed_gaps(gaps, prs, path_for_gap=None, exclude_branch=None): + """Split gaps into (todo, claimed). + + `claimed` is a list of (gap, pr, reason) so the caller can print exactly why + each gap was skipped and which PR to go look at. + + `path_for_gap(gap)` optionally returns the repo-relative path the gap would + write to; gaps with no predictable path are matched on their key alone. + """ + by_key, by_path = index_open_prs(prs, exclude_branch=exclude_branch) + by_covers = None # built lazily: costs one gh call per PR + + todo, claimed = [], [] + for gap in gaps: + key = gap_key(gap) + pr = by_key.get(key) + if pr: + claimed.append((gap, pr, f"gap {key} is already in PR {pr_label(pr)}")) + continue + + path = None + if path_for_gap: + try: + path = path_for_gap(gap) + except Exception: + path = None + # A gap's own `path` (thin_page, missing_description) is the page it edits. + path = path or gap.get("path") + + pr = by_path.get(path) if path else None + if pr: + claimed.append((gap, pr, f"{path} is already changed in PR {pr_label(pr)}")) + continue + + # Last resort for group gaps: a PR may already add a page declaring it + # covers this group under a filename we could never have predicted. + group = gap.get("group") + if group and gap.get("type") in COVERS_GAP_TYPES: + if by_covers is None: + by_covers = covers_claimed_by_open_prs(prs, exclude_branch=exclude_branch) + pr = by_covers.get(group.lower()) + if pr: + claimed.append(( + gap, pr, + f"a page covering '{group}' is already in PR {pr_label(pr)}", + )) + continue + + todo.append(gap) + + return todo, claimed + + +def find_duplicate_pr(gap_keys, paths, prs, exclude_branch=None): + """Decide whether a finished run is a duplicate of something already open. + + Returns (is_duplicate, matches) where `matches` is a list of + (what, pr, reason) for everything that overlaps an open PR. + + Duplicate only when *every* gap key, or *every* changed page, is already + claimed. A partial overlap is reported but not blocked: the run carries work + that no open PR has, and dropping it would silently lose that work. + """ + by_key, by_path = index_open_prs(prs, exclude_branch=exclude_branch) + + key_matches = [ + (k, by_key[k], f"gap {k} is already in PR {pr_label(by_key[k])}") + for k in gap_keys if k in by_key + ] + path_matches = [ + (p, by_path[p], f"{p} is already changed in PR {pr_label(by_path[p])}") + for p in paths if p in by_path + ] + + fully_claimed_keys = bool(gap_keys) and len(key_matches) == len(gap_keys) + fully_claimed_paths = bool(paths) and len(path_matches) == len(paths) + + return (fully_claimed_keys or fully_claimed_paths), key_matches + path_matches diff --git a/pipeline/rework.py b/pipeline/rework.py index 45dc3e2f..afbf6b16 100644 --- a/pipeline/rework.py +++ b/pipeline/rework.py @@ -391,6 +391,11 @@ def main(): "gap": { "type": args.task_type, "family": Path(target_path).parts[1] if len(Path(target_path).parts) > 1 else "", + # The page being reworked, so this task has an identity of its own. + # Without it every rework of the same family shares one gap key and + # the open-PR check reads task B as a duplicate of task A + # (see pipeline/open_prs.py). + "path": target_path, "description": args.instruction, }, }) diff --git a/pipeline/run.py b/pipeline/run.py index b50a1bb9..c6365b3a 100644 --- a/pipeline/run.py +++ b/pipeline/run.py @@ -35,6 +35,7 @@ from pathlib import Path from util import REPO_ROOT, find_latest_run +from open_prs import EXIT_NOTHING_TO_DO STEPS = ["generate", "evaluate", "review", "promote", "ship", "post_review"] @@ -51,7 +52,11 @@ def run_step(step_name, args, run_dir=None, dry_run=False): - """Run a single pipeline step. Returns (success, run_dir).""" + """Run a single pipeline step. Returns its exit code. + + Exit codes: 0 did work, EXIT_NOTHING_TO_DO an open PR already covers this, + anything else a failure. + """ script = REPO_ROOT / "pipeline" / STEP_SCRIPTS[step_name] cmd = [sys.executable, str(script)] @@ -96,7 +101,7 @@ def run_step(step_name, args, run_dir=None, dry_run=False): print(f"{'='*60}\n") result = subprocess.run(cmd, cwd=REPO_ROOT) - return result.returncode == 0 + return result.returncode def parse_args(): @@ -229,11 +234,21 @@ def main(): args_for_step = config["mode_args"][1:] if dry_run and "--dry-run" not in args_for_step: args_for_step = args_for_step + ["--dry-run"] - success = run_step(step, args_for_step, dry_run=dry_run) + code = run_step(step, args_for_step, dry_run=dry_run) else: - success = run_step(step, [], run_dir=run_dir, dry_run=dry_run) + code = run_step(step, [], run_dir=run_dir, dry_run=dry_run) + + # "Nothing to do" is not a failure: an open PR already covers this work. + # Stop here rather than continuing — the later steps operate on --latest, + # so carrying on would promote and reship an older run's drafts. + if code == EXIT_NOTHING_TO_DO: + print(f"\n{'='*60}") + print(f" PIPELINE STOPPED at '{step}': nothing to do.") + print(f" An open PR already covers this work (or no gaps were found).") + print(f"{'='*60}") + return 0 - if not success: + if code != 0: if step == "evaluate": print(f"\n{'='*60}") print(f" EVALUATE FAILED — drafts have errors.") diff --git a/pipeline/ship.py b/pipeline/ship.py index f49425c8..e0c2be5f 100644 --- a/pipeline/ship.py +++ b/pipeline/ship.py @@ -12,6 +12,11 @@ python pipeline/ship.py --latest --dry-run python pipeline/ship.py --latest --branch docs/my-custom-branch python pipeline/ship.py --latest --yes # skip push confirmation + python pipeline/ship.py --latest --force-new-pr # ship even if an open PR covers it + +Refuses to open a PR that duplicates one already open (exit code 3), and stamps +each PR body with the gap keys it covers so later runs can recognize it. See +pipeline/open_prs.py. """ import argparse @@ -30,6 +35,13 @@ stage_and_commit_docs, push_and_create_pr, ) +from open_prs import ( + EXIT_NOTHING_TO_DO, + fetch_open_prs, + find_duplicate_pr, + format_gap_marker, + gap_keys_from_report, +) def load_report(run_dir): @@ -133,7 +145,9 @@ def build_pr_body(report, run_id, changed_files): 3. Verify navigation in docs.json makes sense --- -Generated by the agentic docs pipeline (`pipeline/generate.py`)""" +Generated by the agentic docs pipeline (`pipeline/generate.py`) + +{format_gap_marker(gap_keys_from_report(report))}""" return body @@ -163,6 +177,10 @@ def main(): "--yes", action="store_true", help="Skip push confirmation prompt", ) + parser.add_argument( + "--force-new-pr", action="store_true", + help="Open a PR even when an open one already covers this run", + ) args = parser.parse_args() # --- Resolve run directory --- @@ -236,6 +254,41 @@ def main(): else: print(f" On branch: {branch_name}") + # --- Refuse to duplicate an open PR --- + # The gaps this run addresses stay open until their PR merges, so a later run + # detects them again and arrives here with the same pages. Without this guard + # every run stacks another PR saying the same thing. + if args.force_new_pr: + print(" Duplicate check: skipped (--force-new-pr)") + else: + prs, err = fetch_open_prs() + if prs is None: + print(f" Duplicate check: SKIPPED — could not list open PRs ({err})") + else: + gap_keys = gap_keys_from_report(report) + # docs.json is touched by nearly every docs PR, so it would make any + # run look like a duplicate. Match on the pages themselves. + page_paths = [f for f in changed_files if f != "docs.json"] + is_dup, matches = find_duplicate_pr( + gap_keys, page_paths, prs, exclude_branch=branch_name, + ) + if is_dup: + dup_prs = sorted({pr.get("number") for _, pr, _ in matches if pr.get("number")}) + print("\nAn open PR already covers everything in this run. Not opening another.") + for _, _, reason in matches: + print(f" - {reason}") + print("\nReview, merge, or close " + + ", ".join(f"#{n}" for n in dup_prs) + + " to let the pipeline ship this again.") + print("To open a PR anyway: python pipeline/ship.py --latest --force-new-pr") + return EXIT_NOTHING_TO_DO + if matches: + print(f" Duplicate check: partial overlap with {len(matches)} open item(s) — shipping the rest") + for _, _, reason in matches: + print(f" - {reason}") + else: + print(" Duplicate check: no open PR covers this run") + # --- Build commit message and PR body --- commit_msg = build_commit_message(report, run_id) pr_title = f"docs: pipeline-generated pages ({', '.join(sorted(set(g['gap'].get('family', '?') for g in report.get('generated', []) if g.get('gap'))))})" diff --git a/pipeline/test_open_prs.py b/pipeline/test_open_prs.py new file mode 100644 index 00000000..e5e406ea --- /dev/null +++ b/pipeline/test_open_prs.py @@ -0,0 +1,231 @@ +#!/usr/bin/env python3 +"""Tests for the open-PR duplicate check. + + python pipeline/test_open_prs.py + +No gh calls and no network: every test hands in the PR data `fetch_open_prs` would +have returned, and `run_cmd` is stubbed so a stray subprocess fails the test rather +than reaching GitHub. +""" + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +import open_prs # noqa: E402 +from open_prs import ( # noqa: E402 + find_duplicate_pr, + format_gap_marker, + gap_key, + gap_keys_from_report, + index_open_prs, + parse_gap_marker, + split_claimed_gaps, + _covers_in_diff, +) + + +# PR number -> the diff `gh pr diff` should return. Empty means "no covers here". +DIFFS = {} + + +class _FakeResult: + def __init__(self, returncode, stdout): + self.returncode = returncode + self.stdout = stdout + self.stderr = "" + + +def _fake_run_cmd(cmd, check=True, **kwargs): + """Stand in for gh. Anything but a `pr diff` lookup is a bug in the code.""" + assert cmd[:3] == ["gh", "pr", "diff"], f"unexpected subprocess call: {cmd}" + return _FakeResult(0, DIFFS.get(int(cmd[3]), "")) + + +def setUpModule(): + open_prs.run_cmd = _fake_run_cmd + + +def tearDownModule(): + DIFFS.clear() + + +def pr(number, branch="docs/pipeline-x", body="", files=()): + return { + "number": number, + "headRefName": branch, + "body": body, + "files": [{"path": f} for f in files], + } + + +class GapKeyTest(unittest.TestCase): + def test_same_gap_two_runs_has_one_key(self): + """The wording of `description` changes between runs; identity must not.""" + a = {"type": "missing_group_coverage", "family": "Voice", "group": "Voice Jobs", + "severity": "high", "description": "No guide covers the Voice Jobs endpoints"} + b = dict(a, description="reworded", severity="medium") + self.assertEqual(gap_key(a), gap_key(b)) + + def test_different_groups_differ(self): + base = {"type": "missing_group_coverage", "family": "Voice"} + self.assertNotEqual( + gap_key(dict(base, group="Voice Jobs")), + gap_key(dict(base, group="Translate Audio Files")), + ) + + def test_page_scoped_gaps_differ_by_page(self): + base = {"type": "thin_page"} + self.assertNotEqual( + gap_key(dict(base, path="docs/a.mdx")), + gap_key(dict(base, path="docs/b.mdx")), + ) + + def test_missing_pieces_do_not_collide_with_empty(self): + self.assertEqual(gap_key({}), "-:-:-:-") + + +class MarkerTest(unittest.TestCase): + def test_roundtrip_through_a_pr_body(self): + keys = {"missing_group_coverage:voice:voice-jobs:-", "thin_page:-:-:docs/a.mdx"} + body = "## Summary\nGenerated pages.\n\n---\n" + format_gap_marker(keys) + self.assertEqual(parse_gap_marker(body), keys) + + def test_missing_or_malformed_marker_is_empty(self): + for body in (None, "", "no marker", ""): + self.assertEqual(parse_gap_marker(body), set()) + + def test_keys_come_from_the_run_report(self): + report = {"generated": [ + {"path": "docs/a.mdx", "gap": {"type": "thin_page", "path": "docs/a.mdx"}}, + {"path": "docs/b.mdx"}, # no gap recorded + ]} + self.assertEqual(gap_keys_from_report(report), {"thin_page:-:-:docs/a.mdx"}) + + +class SplitClaimedGapsTest(unittest.TestCase): + def setUp(self): + self.gap = {"type": "missing_group_coverage", "family": "Voice", "group": "Voice Jobs"} + + def test_marker_claims_a_gap_whose_filename_is_unpredictable(self): + open_pr = pr(447, body=format_gap_marker({gap_key(self.gap)}), + files=["docs/voice/some-slug-we-never-predicted.mdx"]) + todo, claimed = split_claimed_gaps([self.gap], [open_pr]) + self.assertEqual(todo, []) + self.assertEqual(claimed[0][1]["number"], 447) + + def test_path_claims_a_gap_with_a_predictable_target(self): + gap = {"type": "thin_page", "path": "docs/a.mdx"} + todo, claimed = split_claimed_gaps([gap], [pr(1, files=["docs/a.mdx"])]) + self.assertEqual((todo, len(claimed)), ([], 1)) + + def test_unclaimed_gap_survives(self): + todo, claimed = split_claimed_gaps([self.gap], [pr(1, files=["docs/other.mdx"])]) + self.assertEqual((todo, claimed), ([self.gap], [])) + + def test_no_open_prs_claims_nothing(self): + todo, claimed = split_claimed_gaps([self.gap], []) + self.assertEqual((todo, claimed), ([self.gap], [])) + + +class FindDuplicatePrTest(unittest.TestCase): + def test_all_keys_claimed_is_a_duplicate(self): + keys = {"missing_group_coverage:voice:voice-jobs:-"} + open_pr = pr(447, body=format_gap_marker(keys)) + # Different filename this run, same gap: the whole point of gap keys. + is_dup, matches = find_duplicate_pr(keys, ["docs/voice/different-slug.mdx"], [open_pr]) + self.assertTrue(is_dup) + self.assertTrue(matches) + + def test_all_paths_claimed_is_a_duplicate(self): + is_dup, _ = find_duplicate_pr(set(), ["docs/a.mdx"], [pr(1, files=["docs/a.mdx"])]) + self.assertTrue(is_dup) + + def test_partial_overlap_still_ships(self): + """A run carrying work no open PR has must not be dropped.""" + keys = {"a:-:-:-", "b:-:-:-"} + open_pr = pr(1, body=format_gap_marker({"a:-:-:-"}), files=["docs/a.mdx"]) + is_dup, matches = find_duplicate_pr(keys, ["docs/a.mdx", "docs/b.mdx"], [open_pr]) + self.assertFalse(is_dup) + self.assertEqual(len(matches), 2) # reported, not blocked + + def test_nothing_to_compare_is_not_a_duplicate(self): + is_dup, _ = find_duplicate_pr(set(), [], [pr(1, files=["docs/a.mdx"])]) + self.assertFalse(is_dup) + + def test_own_branch_is_not_its_own_duplicate(self): + keys = {"a:-:-:-"} + mine = pr(9, branch="docs/pipeline-mine", body=format_gap_marker(keys), + files=["docs/a.mdx"]) + is_dup, _ = find_duplicate_pr(keys, ["docs/a.mdx"], [mine], + exclude_branch="docs/pipeline-mine") + self.assertFalse(is_dup) + + def test_first_pr_wins_when_two_claim_the_same_thing(self): + body = format_gap_marker({"a:-:-:-"}) + by_key, _ = index_open_prs([pr(446, body=body), pr(447, body=body)]) + self.assertEqual(by_key["a:-:-:-"]["number"], 446) + + +class CoversClaimTest(unittest.TestCase): + """The fallback for PRs with no marker: read `covers:` out of the diff.""" + + def setUp(self): + DIFFS.clear() + self.gap = {"type": "missing_group_coverage", "family": "Voice", "group": "Voice Jobs"} + + def test_covers_in_an_open_diff_claims_the_group_gap(self): + DIFFS[447] = "+++ b/docs/voice/slug.mdx\n+covers: [Voice Jobs]\n" + open_pr = pr(447, files=["docs/voice/a-slug-we-never-predicted.mdx"]) + todo, claimed = split_claimed_gaps([self.gap], [open_pr]) + self.assertEqual(todo, []) + self.assertIn("covering 'Voice Jobs'", claimed[0][2]) + + def test_a_different_group_does_not_claim_it(self): + DIFFS[447] = "+covers: [Languages]\n" + todo, claimed = split_claimed_gaps([self.gap], [pr(447, files=["docs/x.mdx"])]) + self.assertEqual((todo, claimed), ([self.gap], [])) + + def test_group_name_case_and_spacing_still_match(self): + DIFFS[447] = "+covers: voice jobs \n" + todo, _ = split_claimed_gaps([self.gap], [pr(447, files=["docs/x.mdx"])]) + self.assertEqual(todo, []) + + def test_prs_touching_no_mdx_are_not_diffed(self): + """The cheap filter: a spec-only PR can't declare `covers`.""" + todo, claimed = split_claimed_gaps( + [self.gap], [pr(447, files=["api-reference/openapi.yaml", "docs.json"])] + ) + self.assertEqual((todo, claimed), ([self.gap], [])) + + +class CoversInDiffTest(unittest.TestCase): + def test_reads_added_covers_lines_only(self): + diff = ( + "+++ b/docs/voice/x.mdx\n" + "+covers: [Translate Audio Files, Languages]\n" + "-covers: [Removed Group]\n" + " covers: [Context Line]\n" + "+covers: Voice Jobs\n" + ) + self.assertEqual( + _covers_in_diff(diff), + ["Translate Audio Files", "Languages", "Voice Jobs"], + ) + + def test_quoted_and_empty_values(self): + self.assertEqual(_covers_in_diff('+covers: ["Voice Jobs"]\n'), ["Voice Jobs"]) + self.assertEqual(_covers_in_diff("+covers: []\n"), []) + self.assertEqual(_covers_in_diff(""), []) + + def test_covers_only_claims_the_gap_type_it_answers(self): + """`covers` says a guide exists, not that the nav group does.""" + gap = {"type": "missing_api_reference_group", "family": "Voice", "group": "Voice Jobs"} + todo, claimed = split_claimed_gaps([gap], [pr(1, files=["docs/voice/x.mdx"])]) + self.assertEqual((todo, claimed), ([gap], [])) # no gh diff call either + + +if __name__ == "__main__": + unittest.main(verbosity=2)