diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml index 5707b9d7..f54a59d6 100644 --- a/.github/workflows/test-scripts.yml +++ b/.github/workflows/test-scripts.yml @@ -30,5 +30,7 @@ jobs: - uses: actions/setup-python@v5 with: python-version: "3.12" - # Stdlib only — these tests stub out gh and never call the network. + # pyyaml only: the tests stub out gh, never call the network, and the + # Anthropic SDK is imported lazily so it isn't needed here. + - run: pip install pyyaml - run: python -m unittest discover -s pipeline -p "test_*.py" -v diff --git a/pipeline/README.md b/pipeline/README.md index 661dbb9f..cf69654d 100644 --- a/pipeline/README.md +++ b/pipeline/README.md @@ -92,6 +92,14 @@ python pipeline/detect_gaps.py --force # report requirement gap **What it checks (per family, across surfaces):** missing product tab, missing overview / tutorial / how-to, missing hub link, narrative pages sitting in the API Reference tab, ungrouped OpenAPI tags, plus thin pages (< 100 words) and missing frontmatter descriptions. +Pages with children are exempt from the thin-page check: a hub is short because its +content sits on the child pages, so padding it out is the wrong fix. + +Gap `path` values are docs.json nav entries, which carry **no file extension** and can +share a name with the directory holding their children (`docs/resources/breaking-changes-change-notices` +is both a page and that folder). Resolve one to a file with `detect_gaps.page_file()`; +never append to it by hand. + **What it doesn't check:** fine-grained judgment (is *this specific* endpoint missing a how-to, is a page the wrong Diataxis type) — that's the LLM audit (`audit_gaps.py`, follow-up); style/Diataxis prose compliance (the review step); code sample correctness (validation step). ### 2. Generate Drafts @@ -353,7 +361,8 @@ The live site structure (which page sits in which tab) is read from `docs.json` ## Dependencies - Python 3.8+ -- `anthropic` (for generate.py, rework.py, review.py, post_review.py) +- `anthropic` (for generate.py, rework.py, review.py, post_review.py). generate.py + imports it lazily, so gap detection, `--dry-run` and the tests work without it - `pyyaml` (for all scripts) - `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/detect_gaps.py b/pipeline/detect_gaps.py index f2e909b0..dbf88f73 100644 --- a/pipeline/detect_gaps.py +++ b/pipeline/detect_gaps.py @@ -155,23 +155,44 @@ def ungrouped_tags(endpoints, families): # Pages — frontmatter + coarse Diataxis classification # # --------------------------------------------------------------------------- # -def page_frontmatter(page_path): - """Return (frontmatter_dict, raw_content). (None, None) if the file is missing.""" +def page_file(page_path): + """The file backing a docs.json page entry, or None if nothing does. + + Nav entries carry no extension, so a nav path is never a usable file path. + Worse, a page can share its name with the directory holding its children + (docs/resources/breaking-changes-change-notices is both a page and the folder + its notices live in), so appending nothing and opening the result hands you a + directory. Resolve through here; never build a path from a nav entry by hand. + """ for ext in (".mdx", ".md"): fp = REPO_ROOT / (page_path + ext) - if fp.exists(): - try: - content = fp.read_text(encoding="utf-8") - except Exception: - return {}, "" - if content.startswith("---"): - end = content.find("---", 3) - if end != -1: - try: - return (yaml.safe_load(content[3:end]) or {}), content - except yaml.YAMLError: - return {}, content - return {}, content + if fp.is_file(): + return fp + return None + + +def has_child_pages(all_pages, page_path): + """Whether other nav entries sit under this one, making it a parent page.""" + prefix = page_path + "/" + return any(p.startswith(prefix) for p in all_pages) + + +def page_frontmatter(page_path): + """Return (frontmatter_dict, raw_content). (None, None) if the file is missing.""" + fp = page_file(page_path) + if fp: + try: + content = fp.read_text(encoding="utf-8") + except OSError: + return {}, "" + if content.startswith("---"): + end = content.find("---", 3) + if end != -1: + try: + return (yaml.safe_load(content[3:end]) or {}), content + except yaml.YAMLError: + return {}, content + return {}, content return None, None # page listed in nav but file missing @@ -329,14 +350,17 @@ def detect_gaps(docs_json, endpoints, families, section=None, force=False): # --- Doc-quality checks on non-reference pages ------------------------ # if not section: - for p in all_doc_pages(docs_json, apiref_tab): + doc_pages = all_doc_pages(docs_json, apiref_tab) + for p in doc_pages: fm, content = page_frontmatter(p) if fm is None: continue if not fm.get("description"): gaps.append(gap("missing_description", "low", None, path=p, desc=f"{p} has no frontmatter description")) - if len(strip_frontmatter(content).split()) < 100: + # A page with children is a hub: it's short because the content sits + # on the child pages. Padding it out is the wrong fix, so don't ask. + if len(strip_frontmatter(content).split()) < 100 and not has_child_pages(doc_pages, p): gaps.append(gap("thin_page", "medium", None, path=p, desc=f"{p} has under 100 words")) diff --git a/pipeline/generate.py b/pipeline/generate.py index e9b1c8eb..83a1e058 100644 --- a/pipeline/generate.py +++ b/pipeline/generate.py @@ -30,15 +30,9 @@ import yaml -try: - import anthropic -except ImportError: - print("Install the Anthropic SDK: pip install anthropic") - sys.exit(1) - - from util import build_authoring_system_prompt from open_prs import EXIT_NOTHING_TO_DO, fetch_open_prs, split_claimed_gaps +from detect_gaps import page_file REPO_ROOT = Path(__file__).resolve().parent.parent OPENAPI_PATH = REPO_ROOT / "api-reference" / "openapi.yaml" @@ -74,6 +68,25 @@ def load_file(path): return "" +def load_existing_page(gap): + """Content of the page a gap edits, resolved from its nav entry. + + Raises instead of returning nothing. A gap's `path` is a docs.json nav entry + with no extension, so opening it directly either fails or (when a directory + shares the page's name) hands back a directory. Both used to surface as an + empty page, which turned "expand this page" into "write one from scratch" + and silently discarded the real content. + """ + page_path = gap.get("path", "") + resolved = page_file(page_path) + if not resolved: + raise FileNotFoundError(f"no .mdx or .md file backs the nav entry '{page_path}'") + content = load_file(resolved) + if not content.strip(): + raise ValueError(f"{resolved.relative_to(REPO_ROOT)} is empty, refusing to rewrite it from nothing") + return content + + def load_openapi_for_tags(tags): """Extract the portion of the OpenAPI spec whose endpoints carry any of `tags`.""" with open(OPENAPI_PATH) as f: @@ -128,14 +141,26 @@ def find_existing_docs_for_family(family_name): context_docs = {} for pp in dict.fromkeys(page_paths): # dedupe, preserve order - for ext in (".mdx", ".md"): - full_path = REPO_ROOT / (pp + ext) - if full_path.exists(): - context_docs[pp + ext] = load_file(full_path) - break + full_path = page_file(pp) + if full_path: + context_docs[str(full_path.relative_to(REPO_ROOT))] = load_file(full_path) return context_docs +def make_client(): + """Build the API client, importing the SDK only when a call is imminent. + + Kept lazy so --dry-run, gap detection and the tests all work without the + SDK installed — none of them talk to the model. + """ + try: + import anthropic + except ImportError: + print("Install the Anthropic SDK: pip install anthropic") + sys.exit(1) + return anthropic.Anthropic() + + def build_system_prompt(): # All writing rules live in the agent files, assembled in one place. return build_authoring_system_prompt( @@ -208,7 +233,7 @@ def build_generation_prompt(gap, family_name, openapi_context, existing_docs_con elif gap_type == "thin_page": page_path = gap.get("path", "") - page_content = load_file(REPO_ROOT / page_path) + page_content = load_existing_page(gap) return f"""Expand this thin page. It currently has only {gap.get('word_count', 0)} words. Current content of {page_path}: @@ -225,7 +250,7 @@ def build_generation_prompt(gap, family_name, openapi_context, existing_docs_con elif gap_type == "missing_code_examples": page_path = gap.get("path", "") - page_content = load_file(REPO_ROOT / page_path) + page_content = load_existing_page(gap) return f"""Add code examples to this page. It's a guide but has no runnable code. Current content of {page_path}: @@ -242,7 +267,7 @@ def build_generation_prompt(gap, family_name, openapi_context, existing_docs_con elif gap_type == "missing_description": page_path = gap.get("path", "") - page_content = load_file(REPO_ROOT / page_path)[:500] + page_content = load_existing_page(gap)[:500] return f"""Generate a frontmatter description for this page. Current content of {page_path} (first 500 chars): @@ -292,10 +317,13 @@ def determine_output_path(gap, family_name, content=None): slug = slugify(title_from_content(content)) return DOCS_DIR / family_name.lower() / f"{slug}.mdx" - elif gap_type in ("thin_page", "missing_code_examples"): - return REPO_ROOT / gap["path"] + elif gap_type in ("thin_page", "missing_code_examples", "missing_description"): + # Resolve the nav entry to the real file. Without the extension the draft + # is written as `docs--a--b` and promote.py, which only collects *.mdx, + # drops it without a word — the page looks generated but never ships. + return page_file(gap.get("path", "")) - return None # missing_description handled inline; non-generative types skipped + return None # non-generative types skipped def _predicted_path(gap): @@ -323,7 +351,9 @@ def _pr_review_hint(claimed): def apply_description(gap, description): """Insert a frontmatter description into an existing page.""" - page_path = REPO_ROOT / gap["path"] + page_path = page_file(gap.get("path", "")) + if not page_path: + return False content = page_path.read_text(encoding="utf-8") if not content.startswith("---"): @@ -423,10 +453,14 @@ def main(): out_path = determine_output_path(g, family) if out_path: print(f" -> {out_path.relative_to(REPO_ROOT)}") + elif g.get("path"): + print(f" -> skip (no file backs the nav entry '{g['path']}')") + else: + print(f" -> filename chosen from the generated title") print() return 0 - client = anthropic.Anthropic() + client = make_client() system_prompt = build_system_prompt() run_id = datetime.now(timezone.utc).strftime("%Y%m%d-%H%M%S") @@ -476,8 +510,12 @@ def main(): print(f" Wrote {draft_path.relative_to(REPO_ROOT)}") generated.append({"gap": gap, "action": "file_written", "path": str(rel), "draft": str(draft_path.relative_to(REPO_ROOT))}) else: - print(f" No output path determined, skipping") - errors.append({"gap": gap, "error": "No output path"}) + detail = ( + f"no file backs the nav entry '{gap['path']}'" + if gap.get("path") else "no output path could be determined" + ) + print(f" Skipping: {detail}") + errors.append({"gap": gap, "error": detail}) except Exception as e: print(f" Error: {e}") diff --git a/pipeline/test_gap_paths.py b/pipeline/test_gap_paths.py new file mode 100644 index 00000000..dd5acf76 --- /dev/null +++ b/pipeline/test_gap_paths.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +"""Tests for turning a docs.json nav entry into a file path. + + python pipeline/test_gap_paths.py + +A nav entry ("docs/resources/breaking-changes-change-notices") has no extension, +and may share its name with the directory holding its child pages. Treating one as +a file path produced three failures at once: a crash on the directory, an empty +"current content" that turned an expand into a rewrite-from-scratch, and an +extensionless draft that promote.py silently dropped. These pin all three down. + +Runs against a temp docs tree, so no repo file is read and none is written. +""" + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +import detect_gaps # noqa: E402 +import generate # noqa: E402 + + +class TempDocsTree(unittest.TestCase): + """Points both modules at a scratch tree laid out like the real repo.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = Path(self._tmp.name) + # DOCS_DIR is bound at import time, so redirect it alongside the roots. + self._saved = (detect_gaps.REPO_ROOT, generate.REPO_ROOT, generate.DOCS_DIR) + detect_gaps.REPO_ROOT = self.root + generate.REPO_ROOT = self.root + generate.DOCS_DIR = self.root / "docs" + self.addCleanup(self._restore) + + def _restore(self): + detect_gaps.REPO_ROOT, generate.REPO_ROOT, generate.DOCS_DIR = self._saved + self._tmp.cleanup() + + def write(self, rel, text="---\ntitle: T\n---\n\nSome body copy.\n"): + path = self.root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +class PageFileTest(TempDocsTree): + def test_resolves_the_mdx_behind_a_nav_entry(self): + self.write("docs/a/b.mdx") + self.assertEqual(detect_gaps.page_file("docs/a/b"), self.root / "docs/a/b.mdx") + + def test_falls_back_to_md(self): + self.write("docs/a/b.md") + self.assertEqual(detect_gaps.page_file("docs/a/b"), self.root / "docs/a/b.md") + + def test_a_directory_of_the_same_name_is_not_the_page(self): + """The [Errno 21] regression: page and child-page folder share a name.""" + (self.root / "docs/resources/notices").mkdir(parents=True) + self.write("docs/resources/notices/july-2024.mdx") + self.assertIsNone(detect_gaps.page_file("docs/resources/notices")) + + # With the page's own file present, that file is what resolves. + self.write("docs/resources/notices.mdx") + self.assertEqual( + detect_gaps.page_file("docs/resources/notices"), + self.root / "docs/resources/notices.mdx", + ) + + def test_nothing_backing_the_entry(self): + self.assertIsNone(detect_gaps.page_file("docs/a/ghost")) + self.assertIsNone(detect_gaps.page_file("")) + + +class HasChildPagesTest(unittest.TestCase): + def setUp(self): + self.pages = [ + "docs/resources/notices", + "docs/resources/notices/july-2024", + "docs/resources/notices-and-more", + "docs/a/leaf", + ] + + def test_a_page_with_children_is_a_parent(self): + self.assertTrue(detect_gaps.has_child_pages(self.pages, "docs/resources/notices")) + + def test_a_leaf_is_not(self): + self.assertFalse(detect_gaps.has_child_pages(self.pages, "docs/a/leaf")) + + def test_a_name_prefix_is_not_a_child(self): + """'notices-and-more' must not count as a child of 'notices'.""" + self.assertFalse( + detect_gaps.has_child_pages(["docs/x", "docs/x-ray"], "docs/x") + ) + + +class ThinPageDetectionTest(TempDocsTree): + """Hub pages are short by design; padding them out is the wrong fix.""" + + def _gaps(self): + docs_json = {"navigation": {"tabs": [ + {"tab": "Home", "pages": ["docs/hub", "docs/hub/child", "docs/lonely"]}, + ]}} + return detect_gaps.detect_gaps(docs_json, [], {}) + + def test_parent_page_is_not_flagged_but_a_real_thin_page_is(self): + short = '---\ntitle: T\ndescription: d\n---\n\nToo short.\n' + self.write("docs/hub.mdx", short) + self.write("docs/hub/child.mdx", short) + self.write("docs/lonely.mdx", short) + + thin = [g["path"] for g in self._gaps() if g["type"] == "thin_page"] + self.assertNotIn("docs/hub", thin) + self.assertIn("docs/lonely", thin) + self.assertIn("docs/hub/child", thin) # a child can still be thin + + +class OutputPathTest(TempDocsTree): + def test_page_edits_keep_the_extension(self): + """promote.py only collects *.mdx drafts, so losing it loses the page.""" + self.write("docs/a/b.mdx") + for gap_type in ("thin_page", "missing_code_examples", "missing_description"): + out = generate.determine_output_path({"type": gap_type, "path": "docs/a/b"}, "unknown") + self.assertEqual(out, self.root / "docs/a/b.mdx", gap_type) + + def test_unbacked_entry_yields_no_path(self): + out = generate.determine_output_path({"type": "thin_page", "path": "docs/a/ghost"}, "unknown") + self.assertIsNone(out) + + def test_new_pages_are_unaffected(self): + out = generate.determine_output_path({"type": "missing_tutorial"}, "Voice") + self.assertEqual(out, self.root / "docs/voice/tutorial.mdx") + + +class LoadExistingPageTest(TempDocsTree): + def test_reads_the_resolved_file(self): + self.write("docs/a/b.mdx", "---\ntitle: T\n---\n\nReal content.\n") + self.assertIn("Real content.", generate.load_existing_page({"path": "docs/a/b"})) + + def test_unbacked_entry_raises_instead_of_returning_nothing(self): + """An empty read used to become "write this page from scratch".""" + with self.assertRaises(FileNotFoundError): + generate.load_existing_page({"path": "docs/a/ghost"}) + + def test_directory_collision_raises_rather_than_crashing_on_the_directory(self): + (self.root / "docs/a/b").mkdir(parents=True) + self.write("docs/a/b/child.mdx") + with self.assertRaises(FileNotFoundError): # not IsADirectoryError + generate.load_existing_page({"path": "docs/a/b"}) + + def test_empty_page_raises(self): + self.write("docs/a/blank.mdx", " \n") + with self.assertRaises(ValueError): + generate.load_existing_page({"path": "docs/a/blank"}) + + +class ApplyDescriptionTest(TempDocsTree): + def test_writes_into_the_resolved_file(self): + self.write("docs/a/b.mdx", "---\ntitle: T\n---\n\nBody.\n") + self.assertTrue(generate.apply_description({"path": "docs/a/b"}, "A description.")) + self.assertIn('description: "A description."', + (self.root / "docs/a/b.mdx").read_text()) + + def test_unbacked_entry_is_a_no_op(self): + self.assertFalse(generate.apply_description({"path": "docs/a/ghost"}, "x")) + + +if __name__ == "__main__": + unittest.main(verbosity=2)