diff --git a/.github/workflows/enhance.yml b/.github/workflows/enhance.yml new file mode 100644 index 00000000000..d17c30318b7 --- /dev/null +++ b/.github/workflows/enhance.yml @@ -0,0 +1,93 @@ +name: Enhance + +# pull_request_target grants GITHUB_TOKEN write access on fork PRs. +# PR code is checked out as data only; scripts run from the trusted base checkout. +on: + pull_request_target: + types: + - opened + - synchronize + - reopened + +permissions: + contents: read + pull-requests: write + +jobs: + ensure-enhancements: + runs-on: ubuntu-24.04 + steps: + - name: Checkout PR code + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + repository: ${{ github.event.pull_request.head.repo.full_name }} + fetch-depth: 0 + + - name: Checkout trusted base scripts + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.base.sha }} + path: .trusted-base + + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Install PyYAML + run: pip install --no-warn-script-location pyyaml + + - name: Ensure documentation enhancements + id: ensure + continue-on-error: true + env: + DIFF_BASE: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + git fetch origin "$DIFF_BASE" + make -f .trusted-base/Makefile ensure-enhancements \ + TOOLS_DIR=.trusted-base/tools \ + DIFF_BASE="$DIFF_BASE" \ + STATUS_FILE="$GITHUB_OUTPUT" + + # The ensure step soft-fails whenever enhancements are missing, so an empty + # enhancements_checked is the only signal that the check itself never ran. + - name: Verify enhancement check ran + if: steps.ensure.outputs.enhancements_checked == '' + run: | + echo "The enhancement check produced no outputs; see the ensure step log." + exit 1 + + - name: Supersede stale enhancement reviews + if: steps.ensure.outputs.enhancements_checked == 'true' + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPOSITORY: ${{ github.repository }} + run: | + set -euo pipefail + make -f .trusted-base/Makefile supersede-enhancement-reviews \ + TOOLS_DIR=.trusted-base/tools + + # Posts the summary so the Conversation view has a current review after + # the stale ones are minimised. + - name: Post enhancement review comment + if: >- + ${{ !cancelled() + && steps.ensure.outputs.has_results == 'true' }} + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REVIEW_COMMENT: ${{ steps.ensure.outputs.comment }} + run: | + set -euo pipefail + gh pr review "$PR_NUMBER" \ + --comment \ + --body "$REVIEW_COMMENT" + + - name: Enforce required enhancements + if: always() && steps.ensure.outputs.has_errors == 'true' + run: | + echo "Required enhancements (error level) are still missing." + exit 1 diff --git a/Makefile b/Makefile index 1f3ad314581..0cc3db7bbb5 100644 --- a/Makefile +++ b/Makefile @@ -7,6 +7,7 @@ PYTHON := python3 ifeq ($(OS),Windows_NT) PYTHON := python endif +BASH := bash BUILD = $(PYTHON) -m sphinx JOBS ?= auto # Attached form (-j, no space) so sphinx-multiversion forwards it to sphinx-build @@ -15,6 +16,12 @@ OPTS =-c . -W -j$(JOBS) # Treat warnings as errors, build in parallel ($(J LIVE_HOST ?= 0.0.0.0 LIVE_PORT ?= 2022 +TOOLS_DIR ?= tools +DIFF_BASE ?= +STATUS_FILE ?= +PR_NUMBER ?= +REPOSITORY ?= + DICTIONARIES := codespell_dictionary.txt codespell_whitelist.txt help: @@ -36,11 +43,32 @@ test: doc8 --ignore D001 --ignore-path $(OUT) -- $(SOURCE) test-tools: - $(PYTHON) -m pytest test/ + $(PYTHON) -m pytest test/ tools/tests/ spellcheck: git ls-files '*.md' '*.rst' | xargs codespell --config codespell.cfg +ensure-enhancements: +ifndef DIFF_BASE + $(error DIFF_BASE is required) +endif +ifndef STATUS_FILE + $(error STATUS_FILE is required) +endif + $(PYTHON) $(TOOLS_DIR)/ensure_enhancements.py \ + --config $(TOOLS_DIR)/enhance.yaml \ + --diff-base $(DIFF_BASE) \ + --status-file $(STATUS_FILE) + +supersede-enhancement-reviews: +ifndef PR_NUMBER + $(error PR_NUMBER is required) +endif +ifndef REPOSITORY + $(error REPOSITORY is required) +endif + $(BASH) $(TOOLS_DIR)/supersede_enhancement_reviews.sh + check-dictionaries: @echo "Checking dictionaries..." @for dict in $(DICTIONARIES); do \ @@ -69,4 +97,4 @@ linkcheck: serve: sphinx-autobuild --host $(LIVE_HOST) --port $(LIVE_PORT) -c . $(SOURCE) $(OUT)/html -.PHONY: help Makefile multiversion test test-tools linkcheck serve lint spellcheck check-dictionaries sort-dictionaries +.PHONY: help Makefile multiversion test test-tools linkcheck serve lint spellcheck check-dictionaries sort-dictionaries ensure-enhancements supersede-enhancement-reviews $(MAKEFILE_LIST) diff --git a/conf.py b/conf.py index c6aee806cb8..1a2d5768ee4 100644 --- a/conf.py +++ b/conf.py @@ -90,6 +90,8 @@ 'sphinxcontrib.googleanalytics', 'sphinxcontrib.mermaid', 'sphinxext.opengraph', + 'short_description', + 'showmeta' ] # Intersphinx mapping @@ -186,6 +188,7 @@ 'DISTRO_UBUNTU_DEB_PLATFORM': distro_ubuntu_deb_platform['rolling'], 'DISTRO_ARM_STATUS_SUFFIX': distro_arm_status_suffix.get('rolling', 'unv8'), 'REPOS_FILE_BRANCH': 'rolling', + 'PRODUCT': 'ROS 2', } html_favicon = 'favicon.ico' diff --git a/plugins/meta_util.py b/plugins/meta_util.py new file mode 100644 index 00000000000..6242ef196b0 --- /dev/null +++ b/plugins/meta_util.py @@ -0,0 +1,70 @@ +# Copyright 2026 Open Robotics — shared helpers for ``.. meta::`` / Pagefind +""" +Collect every ``.. meta::`` field from the doctree, sanitize keys, and expand +``{MACRO}`` placeholders using the Sphinx ``macros`` config (longest keys first). + +Sphinx / the HTML theme may also emit plain ```` tags for the same fields. +The Pagefind extension emits additional tags with ``data-pagefind-filter`` and may +split comma-separated values into multiple tags for faceted search. +""" + +from __future__ import annotations + +import re +from typing import Dict, List, Optional + +from docutils import nodes + +# HTML ```` names should be conservative; allow common patterns. +_META_NAME_RE = re.compile(r'^[A-Za-z0-9_.:-]+$') + + +def sanitize_meta_key(raw: str) -> Optional[str]: + s = str(raw).strip() + if not s or not _META_NAME_RE.match(s): + return None + return s + + +def all_doctree_meta(doctree: Optional[nodes.document]) -> Dict[str, str]: + """Return last-wins mapping of every ``nodes.meta`` ``name``/``property`` → ``content``.""" + if doctree is None: + return {} + + out: Dict[str, str] = {} + for meta in doctree.findall(nodes.meta): + if meta.get('http-equiv'): + continue + content = meta.get('content') + if not content: + continue + key: Optional[str] = None + name = meta.get('name') + if name: + key = sanitize_meta_key(str(name)) + else: + prop = meta.get('property') + if prop: + key = sanitize_meta_key(str(prop)) + if not key: + continue + out[key] = str(content).strip() + return out + + +def expand_meta_macros(text: str, macros: Dict[str, str]) -> str: + """Expand ``{KEY}`` placeholders; longer macro names first to avoid partial matches.""" + result = text + for key, value in sorted(macros.items(), key=lambda kv: len(kv[0]), reverse=True): + result = result.replace(f'{{{key}}}', value) + return result + + +def expand_all_meta_values(meta: Dict[str, str], macros: Dict[str, str]) -> Dict[str, str]: + """Apply ``expand_meta_macros`` to every meta value.""" + return {k: expand_meta_macros(v, macros) for k, v in meta.items()} + + +def split_meta_values(value: str) -> List[str]: + """Return comma-separated metadata values as individual Pagefind values.""" + return [part.strip() for part in value.split(',') if part.strip()] \ No newline at end of file diff --git a/plugins/short_description.py b/plugins/short_description.py new file mode 100644 index 00000000000..9b275626162 --- /dev/null +++ b/plugins/short_description.py @@ -0,0 +1,32 @@ +from __future__ import annotations + +from docutils import nodes +from sphinx.util.docutils import SphinxDirective + + +class ShortDescriptionDirective(SphinxDirective): + """Directive to render the short description of an article.""" + + has_content = True + required_arguments = 0 + optional_arguments = 0 + option_spec = {} + + def run(self) -> list[nodes.Node]: + # Create a container node to hold the parsed content + node = nodes.container() + node['classes'].append('short-description') + + # Parse the directive content into the container node + self.state.nested_parse(self.content, self.content_offset, node) + + return [node] + + +def setup(app): + app.add_directive('short-description', ShortDescriptionDirective) + return { + 'parallel_read_safe': True, + 'parallel_write_safe': True, + 'version': '0.1.0', + } \ No newline at end of file diff --git a/plugins/showmeta.py b/plugins/showmeta.py new file mode 100644 index 00000000000..41ad69ca28a --- /dev/null +++ b/plugins/showmeta.py @@ -0,0 +1,120 @@ +# Copyright 2026 Open Robotics — explicit in-body ``.. showmeta::`` summary +""" +Render selected ``.. meta::`` fields in the document body with author-controlled +order and labels. Place ``.. showmeta::`` where the summary should appear (HTML only). +""" + +from __future__ import annotations + +import html as html_module +import re +from typing import List + +from docutils import nodes +from docutils.parsers.rst import directives +from sphinx.util.docutils import SphinxDirective + +from .meta_util import all_doctree_meta, expand_all_meta_values + + +def _macros_flat(app) -> dict[str, str]: + return {str(k): str(v) for k, v in (getattr(app.config, 'macros', {}) or {}).items()} + + +def _default_showmeta_label(key: str) -> str: + spaced = re.sub(r'([a-z])([A-Z])', r'\1 \2', key) + return spaced.replace('_', ' ').replace('-', ' ').strip().title() + + +class showmeta_node(nodes.General, nodes.Element): + """Placeholder replaced on ``doctree-resolved`` (HTML builds only).""" + + +class ShowMetaDirective(SphinxDirective): + """Insert a visible metadata line built from ``.. meta::`` on this page.""" + + has_content = False + option_spec = { + 'order': directives.unchanged, + 'labels': directives.unchanged, + } + + def run(self) -> List[nodes.Node]: + node = showmeta_node() + node['order'] = self.options.get('order', '') + node['labels'] = self.options.get('labels', '') + self.set_source_info(node) + return [node] + + +def visit_skip_showmeta(self, node: showmeta_node) -> None: + raise nodes.SkipNode + + +def depart_showmeta_noop(self, node: showmeta_node) -> None: + pass + + +def _parse_labels(raw: str) -> dict[str, str]: + out: dict[str, str] = {} + for part in [p.strip() for p in raw.split(',') if p.strip() and '=' in p]: + key, _, value = part.partition('=') + key, value = key.strip(), value.strip() + if key: + out[key] = value + return out + + +def replace_showmeta_nodes(app, doctree: nodes.document, docname: str) -> None: + if app.builder.format != 'html': + for node in list(doctree.findall(showmeta_node)): + node.parent.remove(node) + return + + macros = _macros_flat(app) + meta = expand_all_meta_values(all_doctree_meta(doctree), macros) + + for node in list(doctree.findall(showmeta_node)): + order = [x.strip() for x in node.get('order', '').split(',') if x.strip()] + labels_map = _parse_labels(node.get('labels', '')) + if not order: + node.parent.remove(node) + continue + + parts: List[str] = [] + for key in order: + val = meta.get(key, '').strip() + if not val: + continue + label_base = labels_map.get(key) or _default_showmeta_label(key) + label_display = label_base if label_base.rstrip().endswith(':') else f'{label_base}:' + parts.append( + f'{html_module.escape(label_display)} ' + f'{html_module.escape(val)}' + ) + + if not parts: + node.parent.remove(node) + else: + inner = ' | '.join(parts) + raw = nodes.raw( + '', + f'

{inner}

', + format='html', + ) + node.replace_self(raw) + + +def setup(app): + app.add_node( + showmeta_node, + html=(visit_skip_showmeta, depart_showmeta_noop), + latex=(visit_skip_showmeta, depart_showmeta_noop), + ) + app.add_directive('showmeta', ShowMetaDirective) + app.connect('doctree-resolved', replace_showmeta_nodes) + return { + 'version': '1.0.0', + 'parallel_read_safe': True, + 'parallel_write_safe': True, + } \ No newline at end of file diff --git a/source/_static/custom.css b/source/_static/custom.css index 4252f921bb8..99f8209fdbb 100644 --- a/source/_static/custom.css +++ b/source/_static/custom.css @@ -1,3 +1,10 @@ .wy-nav-content { max-width: 64rem; } + +.short-description p{ + font-size: 1.25rem; + line-height: 1.5; + color: #777777; + margin-bottom: 1.5rem; +} diff --git a/tools/README.md b/tools/README.md new file mode 100644 index 00000000000..f60ed41261d --- /dev/null +++ b/tools/README.md @@ -0,0 +1,239 @@ +# Documentation tools + +Helpers for ensuring reStructuredText (`.rst`) documentation enhancements on pull requests. + +--- + +## User guide + +Information for documentation contributors creating or updating `.rst` files. + +### Prerequisites + +| Component | Used by | +|-----------|---------| +| [PyYAML](https://pyyaml.org/) (`pip install pyyaml`) | [`ensure_enhancements.py`](ensure_enhancements.py) and unit tests in [`tests/`](tests/) | +| Git (with a usable `HEAD` and refs for your base commit) | PR-scope discovery (`--diff-base`) | +| [GitHub CLI](https://cli.github.com/) (`gh`) and `jq` | [`supersede_enhancement_reviews.sh`](supersede_enhancement_reviews.sh) only (Enhance workflow on `ubuntu-24.04`; optional for local supersede testing) | + +### Enhancement configuration + +[`enhance.yaml`](enhance.yaml) defines documentation enhancement rules. The `meta` section lists every `.. meta::` field checked by the tooling. Each entry has: + +- **`severity`**: `warning` (advisory; soft-fails the ensure step in CI) or `error` (fails the workflow after the review is posted). +- **`value`**: suggested default text when the field is missing or blank. Leave empty when the contributor must supply a non-empty value. + +```yaml +meta: + product: + severity: warning + value: "{PRODUCT}" + distribution: + severity: warning + value: "{DISTRO}" + area: + severity: error + value: + experience: + severity: warning + value: + content-type: + severity: warning + value: +``` + +`{PRODUCT}` and `{DISTRO}` are Sphinx substitution macros expanded at build time from [`conf.py`](../conf.py). + +The `after_title` section maps directive names to rules, in the order they should appear after the first document title: + +```yaml +after_title: + short-description: + severity: warning + content: first_paragraph + showmeta: + severity: warning + options: + order: area, content-type, experience + required_options: + - order +``` + +The `:order:` value lists `.. meta::` field names and must match the `meta` section (e.g. `content-type`, not `contentType`). + +For `short-description`, the contributor should wrap the first prose paragraph after the title into the directive. For `showmeta`, the contributor should add the directive with the configured `:order:` option when missing. + +#### Severity behaviour + +| Severity | Missing or blank field | CI ensure step | Workflow job | +|----------|------------------------|----------------|--------------| +| `warning` | Listed in review | Soft warning (`continue-on-error`) | Succeeds | +| `error` | Listed in review | Soft warning (same step) | **Fails** on final enforce step | + +### Checking enhancements locally + +[`ensure_enhancements.py`](ensure_enhancements.py) checks `.rst` files against [`enhance.yaml`](enhance.yaml). It reports missing meta fields and after-title directives; it does not modify files. + +#### Usage + +From the repository root: + +```bash +python3 tools/ensure_enhancements.py path/to/article.rst +``` + +Multiple files: + +```bash +python3 tools/ensure_enhancements.py source/Topic/A.rst source/Topic/B.rst +``` + +Pull request scope (discovers changed `ACMR` `*.rst` files via `git diff`): + +```bash +python3 tools/ensure_enhancements.py --diff-base origin/rolling +``` + +For day-to-day editing, pass paths explicitly so the tool exits `1` only when **error**-severity issues remain. + +To simulate the CI ensure step locally (writes status outputs and uses CI exit codes): + +```bash +make ensure-enhancements DIFF_BASE=origin/rolling STATUS_FILE=/tmp/enhance-out.txt +``` + +#### Exit codes + +With `--status-file` (CI), exit `1` when any issues remain. Locally, exit `1` only when **error**-severity issues are still unresolved; warning-only issues exit `0`. + +### Contributor CI experience + +When you open or update a pull request, CI automatically checks enhancements on all modified `.rst` files. + +| Situation | Ensure step | Pull request review | Job result | +|-----------|-------------|---------------------|------------| +| All enhancements resolved | Green | None (stale bot reviews cleared) | Success | +| Warning-only gaps | Soft warning | Summary review comment | Success | +| Error gaps (e.g. missing `area`) | Soft warning | Summary review comment | **Failure** after enforce step | +| No changed `.rst` files in the PR | No check; `enhancements_checked=false` | Supersede/review steps skipped | Success | + +The **Documentation enhancements** review comment (`## Documentation enhancements`) lists every affected file and what is missing. Each file section names missing `.. meta::` fields (with required/optional labels and suggested values where configured) and missing after-title directives (with brief guidance). + +When you push new commits, the workflow minimises the previous summary review as outdated and posts a fresh one reflecting the current state. + +--- + +## Developer guidance + +Information for maintainers and developers working on or extending the enhancement tooling and CI workflows. + +### Repository layout + +| File | Purpose | +|------|---------| +| [`rst_utils.py`](rst_utils.py) | Read-only detection of `.. meta::`, `.. short-description::`, and `.. showmeta::` directives | +| [`enhance.yaml`](enhance.yaml) | Enhancement rules (`meta` fields and `after_title` directives) | +| [`ensure_enhancements.py`](ensure_enhancements.py) | CLI that checks enhancements from the config and builds the review comment | +| [`supersede_enhancement_reviews.sh`](supersede_enhancement_reviews.sh) | Minimise outdated bot PR reviews | +| [`tests/`](tests/) | Unit tests for the tools in this directory | + +### Code modules + +#### `rst_utils.py` + +Read-only utilities for locating Sphinx directives in RST source: + +- **`get_meta_fields_from_content`** — field names and values in the first `.. meta::` block +- **`has_short_description_content`** — whether a non-empty `.. short-description::` body exists +- **`has_showmeta_with_order`** — whether `.. showmeta::` exists with a non-empty `:order:` option + +#### Extending configuration + +Add a new key under `meta` in [`enhance.yaml`](enhance.yaml) to extend metadata coverage without changing Python code. `_parse_meta_rules` accepts any key with `severity` and `value`, and the rest of the pipeline (unresolved-field detection and review hints) is driven entirely by that mapping. + +`after_title` is only partly config-driven. Its mapping shape lets you reorder or retune the two existing directives (`short-description`, `showmeta`) from YAML alone — but `_parse_after_title_rules` whitelists `supported_directives = {"short-description", "showmeta"}`, so adding a *new* after-title directive needs Python changes: + +1. Add the name to `supported_directives` and its directive-specific validation in `_parse_after_title_rules`. +2. Extend `_after_title_rule_satisfied` with an "already present" check for the new directive. +3. Extend `_after_title_hint` with review text for the new directive. +4. Add matching read-only detection helpers to [`rst_utils.py`](rst_utils.py) — see `has_showmeta_with_order` for the `showmeta` example. + +### CLI options & Makefile targets + +#### Options (`ensure_enhancements.py`) + +- `paths` — optional; when omitted, `--diff-base` is required and changed `.rst` files are discovered automatically +- `--config PATH` — YAML config file (default: `tools/enhance.yaml`) +- `--diff-base SHA` — PR base commit; discovers changed `.rst` files for the check +- `--status-file PATH` — write `enhancements_checked`, `has_results`, `has_errors`, and the review comment body for CI; when issues remain, exits `1` (the ensure step uses `continue-on-error`) +- `-v` / `--verbose` — enable debug logging + +#### Makefile targets + +Enhancement Make targets live in the repository root [`Makefile`](../Makefile). CI invokes them with `make -f .trusted-base/Makefile …` so those recipes run from the PR **base** branch, not the PR head. + +| Target | Required variables | Purpose | +|--------|-------------------|---------| +| `ensure-enhancements` | `DIFF_BASE`, `STATUS_FILE`; optional `TOOLS_DIR` (default `tools`) | Discover changed RST, run enhancement check, append CI outputs | +| `supersede-enhancement-reviews` | `PR_NUMBER`, `REPOSITORY`; optional `TOOLS_DIR` | Minimise stamped bot reviews | + +Environment for `supersede-enhancement-reviews` (set by the workflow or locally): `GH_TOKEN`, plus `PR_NUMBER` and `REPOSITORY`. It writes no CI outputs; the workflow decides what to post from the ensure step’s outputs. + +### Continuous integration architecture + +The workflow [`.github/workflows/enhance.yml`](../.github/workflows/enhance.yml) runs on **`pull_request_target`** when a pull request is **opened**, **synchronised**, or **reopened** (including from forks). That event type allows the default `GITHUB_TOKEN` to post review comments on fork PRs; the workflow file on the repository **default branch** defines the job, while trusted tooling comes from the PR **base** branch (see [Security](#security) below). + +The Enhance workflow installs PyYAML in the job; it does not install the full documentation `requirements.txt` for enhancement checks. + +#### Job flow (`ensure-enhancements`) + +1. Check out the PR **head** (`.rst` content to inspect). +2. Check out the PR **base** into `.trusted-base/` (Makefile, `ensure_enhancements.py`, `enhance.yaml`, `supersede_enhancement_reviews.sh`). +3. Install Python 3.12 and PyYAML. +4. **Ensure documentation enhancements** — `git fetch` the base SHA, then `make -f .trusted-base/Makefile ensure-enhancements` with `TOOLS_DIR=.trusted-base/tools`, `DIFF_BASE`, and `STATUS_FILE=$GITHUB_OUTPUT`. The step uses `continue-on-error: true` so warning-only gaps do not fail the job immediately. +5. **Verify enhancement check ran** — fail the job if `enhancements_checked` is empty. The ensure step soft-fails by design, so a missing output is the only way to tell a crashed check from a clean run. +6. **Supersede stale enhancement reviews** (only if `enhancements_checked=true`) — `make -f .trusted-base/Makefile supersede-enhancement-reviews`, which minimises stamped reviews and writes nothing back. +7. **Post enhancement review comment** — if `has_results`, post the stamped `comment` body via `gh pr review` (Conversation view). +8. **Enforce required enhancements** — if `has_errors`, fail the job (runs `always()` so error gaps fail even when the ensure step soft-failed). + +Step 7 uses `!cancelled()` rather than depending on the supersede step, so a transient GitHub API failure while minimising old reviews cannot stop contributors receiving feedback. + +#### CI outputs + +The script writes **CI outputs** (for example `$GITHUB_OUTPUT`) that describe which workflow steps to run: + +| Output | Meaning | +|--------|---------| +| `enhancements_checked` | At least one changed `.rst` was in scope (`false` when discovery finds no changed RST; supersede is skipped). Empty only when the check never ran | +| `has_results` | Enhancement issues remain; post the Conversation review | +| `has_errors` | Unresolved **error**-severity issues (triggers the final enforce step) | +| `comment` | Full stamped review body (multiline heredoc) posted to Conversation via `gh pr review`; written only when `has_results` | + +The outputs are self-consistent by construction: `comment` exists whenever `has_results` is true, and `has_errors` implies `has_results`. The enforce step cannot fail the job without a review having been posted. + +The ensure step uses `continue-on-error: true`, so warning-only gaps do not fail the job. Error-severity gaps (for example `area`) still fail the workflow on the enforce step after contributors receive review feedback. + +#### Superseding outdated reviews + +The summary review body includes a hidden HTML marker (``). The marker id `ros2-doc-enhance-ensure` (constant `REVIEW_MARKER_ID` in Python; override in the shell script with `ENHANCEMENT_REVIEW_MARKER_ID`) is what the supersede script searches for in review bodies. + +When `enhancements_checked=true`, the workflow runs `make -f .trusted-base/Makefile supersede-enhancement-reviews` ([`supersede_enhancement_reviews.sh`](supersede_enhancement_reviews.sh)): + +1. Lists pull request reviews whose body contains the marker id. +2. Minimises each as **Outdated** via the GitHub GraphQL API (`gh api graphql`; individual failures are ignored). + +When all issues are fixed (`has_results=false`), stale summary reviews are minimised and no new "all clear" comment is posted. + +#### Security + +The workflow uses [`pull_request_target`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target) so the default `GITHUB_TOKEN` can post review comments on fork PRs. **Do not** run `make` or scripts from the PR head checkout in that job; always use `-f .trusted-base/Makefile` and `TOOLS_DIR=.trusted-base/tools`. See [Mitigating the risks of untrusted code checkout](https://docs.github.com/en/actions/reference/security/secure-use#mitigating-the-risks-of-untrusted-code-checkout). + +### Tests + +Unit tests for this directory live in [`tests/`](tests/). From the repository root (with PyYAML installed): + +```bash +python3 -m pytest tools/tests/ +``` + +The [`test-tools`](../Makefile) target (run in [`.github/workflows/test.yml`](../.github/workflows/test.yml)) runs `pytest` on the top-level [`test/`](../test/) tree and `tools/tests/`. diff --git a/tools/enhance.yaml b/tools/enhance.yaml new file mode 100644 index 00000000000..c11bf53cc2c --- /dev/null +++ b/tools/enhance.yaml @@ -0,0 +1,31 @@ +# Enhancement rules for ensure_enhancements.py. +# meta: .. meta:: field rules (severity and optional default values). +# after_title: mapping of directive name → rules for post-title insertion (YAML key order). +# Sphinx expands {PRODUCT} and {DISTRO} at build time (see conf.py). +meta: + product: + severity: warning + value: "{PRODUCT}" + distribution: + severity: warning + value: "{DISTRO}" + area: + severity: error + value: + experience: + severity: warning + value: + content-type: + severity: warning + value: + +after_title: + short-description: + severity: warning + content: first_paragraph + showmeta: + severity: warning + options: + order: area, content-type, experience + required_options: + - order diff --git a/tools/ensure_enhancements.py b/tools/ensure_enhancements.py new file mode 100644 index 00000000000..2af59f5681f --- /dev/null +++ b/tools/ensure_enhancements.py @@ -0,0 +1,723 @@ +#!/usr/bin/env python3 +""" +Ensure configured documentation enhancements exist in RST source files. + +Rules are defined in ``enhance.yaml``: ``meta`` and ``after_title`` mappings +for ``.. meta::`` fields and post-heading directives such as +``.. short-description::`` and ``.. showmeta::``. + +In CI (``--status-file``), writes GitHub Actions outputs and exits with code +``1`` when issues remain so the ensure step can soft-fail. Error-severity +issues also set ``has_errors`` for a final workflow gate. +""" + +from __future__ import annotations + +import argparse +import logging +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path +from typing import Literal, TextIO + +import yaml + +# Allow ``python3 tools/ensure_enhancements.py`` from the repository root. +_TOOLS_DIR = Path(__file__).resolve().parent +if str(_TOOLS_DIR) not in sys.path: + sys.path.insert(0, str(_TOOLS_DIR)) + +from rst_utils import ( # noqa: E402 + get_meta_fields_from_content, + has_short_description_content, + has_showmeta_with_order, +) + +logger = logging.getLogger(__name__) + +DEFAULT_CONFIG_PATH = _TOOLS_DIR / "enhance.yaml" +RST_EXTENSION = ".rst" +Severity = Literal["warning", "error"] + +# Hidden marker in review bodies so CI can find and supersede prior bot reviews. +REVIEW_MARKER_ID = "ros2-doc-enhance-ensure" +REVIEW_MARKER = f"" + +# Title for pull request review bodies (GitHub Markdown). +SUMMARY_REVIEW_TITLE = "## Documentation enhancements" + + +@dataclass(frozen=True) +class AfterTitleRule: + """ + A single after-title directive rule from ``enhance.yaml``. + + The directive name is the key in the ``after_title`` mapping (like ``meta``). + + Attributes: + severity: Advisory ``warning`` or blocking ``error`` in CI. + content: Content source for body directives (e.g. ``first_paragraph``). + options: Option name/value pairs for option-only directives. + required_options: Option names that must be non-empty when present. + """ + + severity: Severity + content: str | None = None + options: dict[str, str] | None = None + required_options: tuple[str, ...] = () + + +@dataclass(frozen=True) +class EnhanceConfig: + """Full enhancement configuration loaded from ``enhance.yaml``.""" + + meta: dict[str, MetaRule] + after_title: dict[str, AfterTitleRule] + + +@dataclass(frozen=True) +class MetaRule: + """ + A single metadata field rule from ``enhance.yaml``. + + Attributes: + severity: Advisory ``warning`` or blocking ``error`` in CI. + value: Default text to inject when missing or blank; empty when the + contributor must supply a non-empty value. + """ + + severity: Severity + value: str + + @property + def has_configured_value(self) -> bool: + """ + Return whether the rule supplies a non-empty default value. + + Returns: + ``True`` when ``value`` is non-empty after stripping whitespace. + """ + return bool(self.value.strip()) + + +def _parse_meta_rules(meta: dict, config_path: Path) -> dict[str, MetaRule]: + """Validate and parse the ``meta`` mapping from config YAML.""" + validated: dict[str, MetaRule] = {} + for key, entry in meta.items(): + if not isinstance(key, str) or not key.strip(): + logger.error("Config %s: meta keys must be non-empty strings", config_path) + raise SystemExit(1) + if not isinstance(entry, dict): + logger.error( + "Config %s: meta entry for %r must be a mapping with severity and value", + config_path, + key, + ) + raise SystemExit(1) + severity = entry.get("severity") + if severity not in ("warning", "error"): + logger.error( + "Config %s: meta entry %r severity must be 'warning' or 'error', got %r", + config_path, + key, + severity, + ) + raise SystemExit(1) + if "value" not in entry: + logger.error("Config %s: meta entry %r must include a 'value' key", config_path, key) + raise SystemExit(1) + raw_value = entry.get("value") + if raw_value is None: + value = "" + elif isinstance(raw_value, str): + value = raw_value + else: + logger.error( + "Config %s: meta value for %r must be a string or null, got %s", + config_path, + key, + type(raw_value).__name__, + ) + raise SystemExit(1) + validated[key] = MetaRule(severity=severity, value=value) + return validated + + +def _parse_after_title_rules( + raw: object, + config_path: Path, +) -> dict[str, AfterTitleRule]: + """Validate and parse the ``after_title`` mapping from config YAML.""" + if raw is None: + return {} + if not isinstance(raw, dict): + logger.error( + "Config %s: 'after_title' must be a mapping keyed by directive name", + config_path, + ) + raise SystemExit(1) + + supported_directives = {"short-description", "showmeta"} + validated: dict[str, AfterTitleRule] = {} + for directive, entry in raw.items(): + if not isinstance(directive, str) or not directive.strip(): + logger.error( + "Config %s: after_title keys must be non-empty directive names", + config_path, + ) + raise SystemExit(1) + if directive not in supported_directives: + logger.error( + "Config %s: after_title directive %r is not supported", + config_path, + directive, + ) + raise SystemExit(1) + if not isinstance(entry, dict): + logger.error( + "Config %s: after_title entry for %r must be a mapping", + config_path, + directive, + ) + raise SystemExit(1) + severity = entry.get("severity") + if severity not in ("warning", "error"): + logger.error( + "Config %s: after_title entry %r severity must be 'warning' or 'error', got %r", + config_path, + directive, + severity, + ) + raise SystemExit(1) + + content = entry.get("content") + if content is not None and not isinstance(content, str): + logger.error( + "Config %s: after_title entry %r content must be a string", + config_path, + directive, + ) + raise SystemExit(1) + + raw_options = entry.get("options") + options: dict[str, str] | None = None + if raw_options is not None: + if not isinstance(raw_options, dict): + logger.error( + "Config %s: after_title entry %r options must be a mapping", + config_path, + directive, + ) + raise SystemExit(1) + options = {} + for opt_key, opt_value in raw_options.items(): + if not isinstance(opt_key, str) or not isinstance(opt_value, str): + logger.error( + "Config %s: after_title entry %r option keys and values must be strings", + config_path, + directive, + ) + raise SystemExit(1) + options[opt_key] = opt_value + + raw_required = entry.get("required_options") + required_options: tuple[str, ...] = () + if raw_required is not None: + if not isinstance(raw_required, list): + logger.error( + "Config %s: after_title entry %r required_options must be a list", + config_path, + directive, + ) + raise SystemExit(1) + required_options = tuple(str(item) for item in raw_required) + + if directive == "short-description": + if content != "first_paragraph": + logger.error( + "Config %s: short-description rule must use content: first_paragraph", + config_path, + ) + raise SystemExit(1) + elif directive == "showmeta": + if not options or not options.get("order", "").strip(): + logger.error( + "Config %s: showmeta rule must include options.order", + config_path, + ) + raise SystemExit(1) + if "order" not in required_options: + logger.error( + "Config %s: showmeta rule must list order in required_options", + config_path, + ) + raise SystemExit(1) + + validated[directive] = AfterTitleRule( + severity=severity, + content=content, + options=options, + required_options=required_options, + ) + return validated + + +def load_enhance_config(config_path: Path) -> EnhanceConfig: + """ + Load and validate enhancement rules from a YAML config file. + + Args: + config_path: Path to the YAML configuration file. + + Returns: + Parsed enhancement configuration. + + Raises: + SystemExit: If the file is missing, invalid, or has unusable rules. + """ + if not config_path.is_file(): + logger.error("Config file not found: %s", config_path) + raise SystemExit(1) + + try: + raw = yaml.safe_load(config_path.read_text(encoding="utf-8")) + except yaml.YAMLError as exc: + logger.error("Invalid YAML in %s: %s", config_path, exc) + raise SystemExit(1) from exc + + if not isinstance(raw, dict): + logger.error("Config %s must be a YAML mapping", config_path) + raise SystemExit(1) + + meta = raw.get("meta") + if not isinstance(meta, dict) or not meta: + logger.error("Config %s must contain a non-empty 'meta' mapping", config_path) + raise SystemExit(1) + + return EnhanceConfig( + meta=_parse_meta_rules(meta, config_path), + after_title=_parse_after_title_rules(raw.get("after_title"), config_path), + ) + + +def _unresolved_fields(content: str, rules: dict[str, MetaRule]) -> list[str]: + """ + Find configured meta fields that are absent or blank in RST content. + + Args: + content: RST source to inspect. + rules: Configured metadata rules. + + Returns: + Unresolved field names in configuration order. + """ + present = get_meta_fields_from_content(content) + return [ + name for name in rules + if name not in present or not present[name].strip() + ] + + +def changed_rst_paths(diff_base: str) -> list[Path]: + """ + List ``.rst`` files changed between a pull request base and ``HEAD``. + + Args: + diff_base: Base commit SHA used for the three-dot comparison. + + Returns: + Repository-relative paths for added, copied, modified, or renamed + ``.rst`` files, or an empty list if ``git diff`` fails. + """ + result = subprocess.run( + [ + "git", + "diff", + "--name-only", + "--diff-filter=ACMR", + f"{diff_base}...HEAD", + "--", + "*.rst", + ], + check=False, + capture_output=True, + text=True, + ) + if result.returncode not in (0, 1): + logger.warning( + "git diff failed listing changed RST files (exit %s): %s", + result.returncode, + result.stderr.strip(), + ) + return [] + paths: list[Path] = [] + for line in result.stdout.splitlines(): + stripped = line.strip() + if stripped: + paths.append(Path(stripped)) + return paths + + +def _write_multiline_output(handle: TextIO, key: str, value: str) -> None: + """ + Write a multiline GitHub Actions output using heredoc syntax. + + Args: + handle: Open text handle for the status file. + key: Output name. + value: Output value, which may span several lines. + + Returns: + None. + """ + delimiter = f"EOF_{key.upper()}" + handle.write(f"{key}<<{delimiter}\n") + handle.write(value) + if not value.endswith("\n"): + handle.write("\n") + handle.write(f"{delimiter}\n") + + +def _write_ci_status_file( + status_file: Path, + *, + enhancements_checked: bool, + results: list[dict[str, object]], + config: EnhanceConfig, + has_errors: bool, +) -> None: + """ + Append GitHub Actions output flags and optional review comment. + + Writes ``enhancements_checked``, ``has_results``, and ``has_errors``, plus + a multiline ``comment`` block when ``results`` is non-empty. + + Args: + status_file: Path to append to (for example ``$GITHUB_OUTPUT``). + enhancements_checked: Whether changed RST files were in scope for this run. + results: Per-file result dicts from ``ensure_enhancements_in_file``. + config: Enhancement configuration used to build the review comment. + has_errors: Whether any result has unresolved error-severity fields. + + Returns: + None. + """ + has_results = bool(results) + with status_file.open("a", encoding="utf-8") as f: + for key, flag in ( + ("enhancements_checked", enhancements_checked), + ("has_results", has_results), + ("has_errors", has_errors), + ): + f.write(f"{key}={'true' if flag else 'false'}\n") + if results: + _write_multiline_output( + f, + "comment", + build_review_comment(results, config=config), + ) + + +def _after_title_rule_satisfied(content: str, directive: str) -> bool: + """Return whether an after-title rule is already satisfied in ``content``.""" + if directive == "short-description": + return has_short_description_content(content) + if directive == "showmeta": + return has_showmeta_with_order(content) + return True + + +def _severity_fields( + field_names: list[str], + rules: dict[str, MetaRule] | dict[str, AfterTitleRule], + severity: Severity, +) -> list[str]: + """ + Return field or directive names that use the given severity in ``rules``. + + Args: + field_names: Candidate meta field or directive names. + rules: Configured metadata or after-title rules keyed by name. + severity: Severity label to match. + + Returns: + Names from ``field_names`` whose rule uses ``severity``. + """ + return [name for name in field_names if rules[name].severity == severity] + + +def ensure_enhancements_in_file( + path: Path, + config: EnhanceConfig, +) -> dict[str, object] | None: + """ + Check one RST file for missing documentation enhancements. + + Args: + path: RST file to inspect. + config: Enhancement configuration. + + Returns: + A result dict when issues were found, otherwise ``None``. + + Raises: + OSError: If the RST file cannot be read. + UnicodeError: If the RST file cannot be decoded as UTF-8. + """ + content = path.read_text(encoding="utf-8") + path_str = str(path).replace("\\", "/") + + unresolved = _unresolved_fields(content, config.meta) + after_title_unresolved = [ + directive + for directive in config.after_title + if not _after_title_rule_satisfied(content, directive) + ] + + if not unresolved and not after_title_unresolved: + logger.info("%s: all configured enhancements present", path) + return None + + return { + "path": path_str, + "meta_required": _severity_fields(unresolved, config.meta, "error"), + "meta_optional": _severity_fields(unresolved, config.meta, "warning"), + "after_title_required": _severity_fields( + after_title_unresolved, + config.after_title, + "error", + ), + "after_title_optional": _severity_fields( + after_title_unresolved, + config.after_title, + "warning", + ), + } + + +def _collect_rst_paths(paths: list[str]) -> list[Path]: + """ + Collect existing RST files from command-line path strings. + + Args: + paths: Candidate filesystem paths. + + Returns: + Existing paths whose suffix is ``.rst`` (case-insensitive). + """ + rst_paths: list[Path] = [] + for raw in paths: + path = Path(raw) + if path.suffix.lower() != RST_EXTENSION: + logger.debug("Skipping non-RST path: %s", raw) + continue + if not path.is_file(): + logger.warning("Skipping missing file: %s", raw) + continue + rst_paths.append(path) + return rst_paths + + +def _meta_field_hint(name: str, rule: MetaRule) -> str: + """Format one missing meta field for the review comment.""" + severity = "required" if rule.severity == "error" else "optional" + if rule.has_configured_value: + return f"`{name}` ({severity}, suggested value `{rule.value}`)" + return f"`{name}` ({severity})" + + +def _after_title_hint(name: str, rule: AfterTitleRule) -> str: + """Format one missing after-title directive for the review comment.""" + severity = "required" if rule.severity == "error" else "optional" + if name == "short-description": + return f"`{name}` ({severity}, wrap the opening paragraph)" + if name == "showmeta" and rule.options: + order = rule.options.get("order", "") + return f"`{name}` ({severity}, add with `:order: {order}`)" + return f"`{name}` ({severity})" + + +def build_review_comment( + results: list[dict[str, object]], + *, + config: EnhanceConfig, +) -> str: + """ + Build a pull-request review body from enhancement check results. + + Args: + results: Per-file result dictionaries from ``ensure_enhancements_in_file``. + config: Enhancement configuration used to format hints. + + Returns: + A stamped Markdown review body listing missing items per file. + """ + lines = [ + SUMMARY_REVIEW_TITLE, + "", + "This pull request is missing configured documentation enhancements " + "(see `tools/enhance.yaml`).", + "", + ] + + for result in results: + path = str(result["path"]) + lines.append(f"### `{path}`") + + meta_required = list(result.get("meta_required") or []) + meta_optional = list(result.get("meta_optional") or []) + if meta_required or meta_optional: + hints = [ + _meta_field_hint(name, config.meta[name]) + for name in meta_required + meta_optional + ] + lines.append(f"- Missing `.. meta::` fields: {', '.join(hints)}") + + after_title_required = list(result.get("after_title_required") or []) + after_title_optional = list(result.get("after_title_optional") or []) + if after_title_required or after_title_optional: + hints = [ + _after_title_hint(name, config.after_title[name]) + for name in after_title_required + after_title_optional + ] + lines.append(f"- Missing after-title directives: {', '.join(hints)}") + + lines.append("") + + return "\n".join(lines).rstrip() + f"\n\n{REVIEW_MARKER}\n" + + +def main(argv: list[str] | None = None) -> int: + """ + Run the command-line documentation enhancement check. + + When no ``paths`` are given, ``--diff-base`` must be set so changed ``.rst`` + files are discovered with ``git diff``. With ``--status-file``, writes CI + outputs and exits ``1`` when any per-file results remain; without it, exits + ``1`` only for unresolved error-severity fields. + + Args: + argv: Command-line arguments excluding the executable name, or ``None`` + to read them from ``sys.argv``. + + Returns: + Process exit code (``0`` on success, ``1`` when issues remain per mode + above). + + Raises: + SystemExit: If command-line arguments or enhancement configuration are invalid. + """ + parser = argparse.ArgumentParser( + description=( + "Ensure configured documentation enhancements exist in RST files " + "using rules from a YAML config file." + ), + ) + parser.add_argument( + "paths", + nargs="*", + help=( + "One or more .rst file paths to check; when omitted, " + "--diff-base must be set to discover changed files" + ), + ) + parser.add_argument( + "--config", + type=Path, + default=DEFAULT_CONFIG_PATH, + help=f"YAML config file (default: {DEFAULT_CONFIG_PATH})", + ) + parser.add_argument( + "--diff-base", + help=( + "Git commit SHA for the pull request base. When set, changed " + "`.rst` files are discovered automatically." + ), + ) + parser.add_argument( + "--status-file", + type=Path, + help=( + "Write enhancements_checked, has_results, has_errors, and the " + "review comment body for CI" + ), + ) + parser.add_argument( + "-v", + "--verbose", + action="store_true", + help="Enable debug logging", + ) + args = parser.parse_args(argv) + + logging.basicConfig( + level=logging.DEBUG if args.verbose else logging.INFO, + format="%(levelname)s: %(message)s", + ) + + if not args.paths and not args.diff_base: + parser.error("provide at least one .rst path or set --diff-base to discover changes") + + config = load_enhance_config(args.config) + checked_pull_request_rst = False + + if not args.paths: + assert args.diff_base is not None + discovered = changed_rst_paths(args.diff_base) + if not discovered: + logger.info("No changed RST files in this pull request.") + if args.status_file is not None: + _write_ci_status_file( + args.status_file, + enhancements_checked=False, + results=[], + config=config, + has_errors=False, + ) + return 0 + checked_pull_request_rst = True + rst_paths = _collect_rst_paths([str(p) for p in discovered]) + else: + checked_pull_request_rst = True + rst_paths = _collect_rst_paths(args.paths) + + results: list[dict[str, object]] = [] + + if not rst_paths: + logger.info("No RST files to process") + else: + for path in rst_paths: + result = ensure_enhancements_in_file(path, config) + if result is not None: + results.append(result) + + logger.info( + "Processed %d file(s): %d with missing enhancements", + len(rst_paths), + len(results), + ) + + has_errors = any( + result["meta_required"] or result.get("after_title_required") + for result in results + ) + + if args.status_file is not None: + _write_ci_status_file( + args.status_file, + enhancements_checked=checked_pull_request_rst, + results=results, + config=config, + has_errors=has_errors, + ) + + if args.status_file is not None and results: + return 1 + if args.status_file is None and has_errors: + return 1 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/rst_utils.py b/tools/rst_utils.py new file mode 100644 index 00000000000..3c172a1c55b --- /dev/null +++ b/tools/rst_utils.py @@ -0,0 +1,121 @@ +""" +Read-only utilities for detecting Sphinx directives in reStructuredText source. + +Supports ``.. meta::``, ``.. short-description::``, and ``.. showmeta::``. +""" + +import re + + +def _find_directive_block(content: str, directive: str) -> str | None: + """ + Locate the inner body of the first ``.. ::`` block in RST source. + + The directive block consists of the explicit marker line followed by + contiguous indented lines; a blank line or a less-indented line ends the + block (per reStructuredText directive block rules). + + Args: + content: The RST file content to search. + directive: Directive name without the ``..`` prefix (e.g. ``meta``). + + Returns: + The inner body text of the directive block, or ``None`` when no block + is found. + """ + match = re.search( + rf"^\.\.\s+{re.escape(directive)}::\s*\n", + content, + re.MULTILINE, + ) + if not match: + return None + + marker_end = match.end() + inner_parts: list[str] = [] + remainder = content[marker_end:] + + for line in remainder.splitlines(keepends=True): + if line.strip() == "": + break + if not line.startswith((" ", "\t")): + break + inner_parts.append(line) + + inner = "".join(inner_parts) + if inner and not inner.endswith("\n"): + inner += "\n" + return inner if inner.strip() else None + + +def _extract_field_values(block_inner: str) -> dict[str, str]: + """ + Collect field or option names and values from a directive body. + + Each line of the form ``:name: value`` contributes ``name`` (Docutils also + allows forms such as ``:name attr=value:``; the captured segment matches + that usage). + + Args: + block_inner: The inner text of a directive block. + + Returns: + Mapping from field or option name to body text (may be empty). + """ + fields: dict[str, str] = {} + for field_match in re.finditer( + r"^[ \t]+:([^:\n]+?):\s*(.*)$", + block_inner, + re.MULTILINE, + ): + fields[field_match.group(1).strip()] = field_match.group(2) + return fields + + +def get_meta_fields_from_content(content: str) -> dict[str, str]: + """ + Return field names and values from the first ``.. meta::`` block. + + If no ``.. meta::`` directive exists, returns an empty mapping. + + Args: + content: The RST file content to search. + + Returns: + Mapping from meta field name to field body text. + """ + inner = _find_directive_block(content, "meta") + if not inner: + return {} + return _extract_field_values(inner) + + +def has_short_description_content(content: str) -> bool: + """ + Return whether the document already has a non-empty ``.. short-description::`` body. + + Args: + content: The RST file content to search. + + Returns: + True if a non-empty short-description block exists, False otherwise. + """ + inner = _find_directive_block(content, "short-description") + return bool(inner and inner.strip()) + + +def has_showmeta_with_order(content: str) -> bool: + """ + Return whether the first ``.. showmeta::`` block has a non-empty ``:order:``. + + Args: + content: RST source to search. + + Returns: + True when showmeta exists with a non-blank order option. + """ + inner = _find_directive_block(content, "showmeta") + if not inner: + return False + options = _extract_field_values(inner) + return bool(options.get("order", "").strip()) diff --git a/tools/supersede_enhancement_reviews.sh b/tools/supersede_enhancement_reviews.sh new file mode 100755 index 00000000000..8d685feee9c --- /dev/null +++ b/tools/supersede_enhancement_reviews.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +# Minimise prior stamped summary reviews from ensure_enhancements.py so each +# workflow run replaces the last Documentation enhancements review comment. +set -euo pipefail + +: "${GH_TOKEN:?GH_TOKEN is required}" +: "${REPOSITORY:?REPOSITORY is required}" +: "${PR_NUMBER:?PR_NUMBER is required}" + +MARKER="${ENHANCEMENT_REVIEW_MARKER_ID:-ros2-doc-enhance-ensure}" + +echo "Marking prior enhancement reviews as outdated." + +reviews_json="$(gh api "repos/${REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate)" +mapfile -t review_ids < <( + echo "${reviews_json}" | jq -r \ + ".[] | select(.body != null and (.body | contains(\"${MARKER}\"))) | .node_id" +) + +minimize_query='mutation($subjectId: ID!) { + minimizeComment(input: { subjectId: $subjectId, classifier: OUTDATED }) { + minimizedComment { isMinimized } + } +}' + +for node_id in "${review_ids[@]:-}"; do + [ -z "${node_id}" ] && continue + gh api graphql -f query="${minimize_query}" -f subjectId="${node_id}" || true +done + +echo "Superseded ${#review_ids[@]} prior review comment(s)." + diff --git a/tools/tests/test_ensure_enhancements.py b/tools/tests/test_ensure_enhancements.py new file mode 100644 index 00000000000..90a2bdf9aac --- /dev/null +++ b/tools/tests/test_ensure_enhancements.py @@ -0,0 +1,410 @@ +# Copyright 2026 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from __future__ import annotations + +import sys +import tempfile +import textwrap +import unittest +from pathlib import Path +from unittest import mock + +_TOOLS_DIR = Path(__file__).resolve().parent.parent +if str(_TOOLS_DIR) not in sys.path: + sys.path.insert(0, str(_TOOLS_DIR)) + +from ensure_enhancements import ( # noqa: E402 + REVIEW_MARKER, + SUMMARY_REVIEW_TITLE, + AfterTitleRule, + EnhanceConfig, + MetaRule, + _unresolved_fields, + build_review_comment, + changed_rst_paths, + ensure_enhancements_in_file, + load_enhance_config, + main, +) + +SAMPLE_CONFIG = textwrap.dedent( + """ + meta: + product: + severity: warning + value: "{PRODUCT}" + area: + severity: error + value: + experience: + severity: warning + value: + after_title: + short-description: + severity: warning + content: first_paragraph + showmeta: + severity: warning + options: + order: area, content-type, experience + required_options: + - order + """ +).strip() + +AFTER_TITLE_RULES = { + "short-description": AfterTitleRule( + severity="warning", + content="first_paragraph", + ), + "showmeta": AfterTitleRule( + severity="warning", + options={"order": "area, content-type, experience"}, + required_options=("order",), + ), +} + +META_ONLY_CONFIG = textwrap.dedent( + """ + meta: + product: + severity: warning + value: "{PRODUCT}" + area: + severity: error + value: + experience: + severity: warning + value: + """ +).strip() + + +class TestEnhanceConfig(unittest.TestCase): + def test_load_enhance_config_parses_meta_rules(self) -> None: + with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as handle: + handle.write(SAMPLE_CONFIG) + path = Path(handle.name) + try: + config = load_enhance_config(path) + finally: + path.unlink() + rules = config.meta + self.assertEqual(rules["product"].severity, "warning") + self.assertEqual(rules["product"].value, "{PRODUCT}") + self.assertTrue(rules["product"].has_configured_value) + self.assertFalse(rules["area"].has_configured_value) + self.assertEqual(rules["area"].severity, "error") + + def test_load_enhance_config_parses_after_title(self) -> None: + with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as handle: + handle.write(SAMPLE_CONFIG) + path = Path(handle.name) + try: + config = load_enhance_config(path) + finally: + path.unlink() + self.assertEqual(len(config.after_title), 2) + self.assertEqual(list(config.after_title.keys()), ["short-description", "showmeta"]) + self.assertEqual(config.after_title["short-description"].content, "first_paragraph") + self.assertEqual( + config.after_title["showmeta"].options, + {"order": "area, content-type, experience"}, + ) + + +class TestUnresolvedFields(unittest.TestCase): + def test_missing_and_blank_count_as_unresolved(self) -> None: + rules = { + "product": MetaRule("warning", "{PRODUCT}"), + "area": MetaRule("error", ""), + } + content = textwrap.dedent( + """ + .. meta:: + :area: + + Title + ===== + """ + ) + self.assertEqual(_unresolved_fields(content, rules), ["product", "area"]) + + +class TestEnsureEnhancementsInFile(unittest.TestCase): + def test_reports_missing_meta_fields(self) -> None: + config = EnhanceConfig( + meta={ + "product": MetaRule("warning", "{PRODUCT}"), + "area": MetaRule("error", ""), + }, + after_title={}, + ) + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "page.rst" + path.write_text("Title\n=====\n", encoding="utf-8") + result = ensure_enhancements_in_file(path, config) + self.assertIsNotNone(result) + assert result is not None + self.assertIn("product", result["meta_optional"]) + self.assertIn("area", result["meta_required"]) + self.assertEqual(result["after_title_optional"], []) + self.assertEqual(result["after_title_required"], []) + + def test_no_result_when_all_fields_present(self) -> None: + config = EnhanceConfig( + meta={"product": MetaRule("warning", "{PRODUCT}")}, + after_title={}, + ) + content = textwrap.dedent( + """ + .. meta:: + :product: ROS 2 + + Title + ===== + """ + ).lstrip() + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "page.rst" + path.write_text(content, encoding="utf-8") + self.assertIsNone(ensure_enhancements_in_file(path, config)) + + def test_does_not_modify_files(self) -> None: + config = EnhanceConfig( + meta={"product": MetaRule("warning", "{PRODUCT}")}, + after_title={}, + ) + original = "Title\n=====\n" + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "page.rst" + path.write_text(original, encoding="utf-8") + ensure_enhancements_in_file(path, config) + self.assertEqual(path.read_text(encoding="utf-8"), original) + + +class TestAfterTitleEnhancements(unittest.TestCase): + def test_reports_missing_after_title_directives(self) -> None: + config = EnhanceConfig(meta={}, after_title=AFTER_TITLE_RULES) + content = textwrap.dedent( + """ + Title + ===== + + Opening paragraph for the page. + """ + ).lstrip() + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "page.rst" + path.write_text(content, encoding="utf-8") + result = ensure_enhancements_in_file(path, config) + self.assertIsNotNone(result) + assert result is not None + self.assertIn("short-description", result["after_title_optional"]) + self.assertIn("showmeta", result["after_title_optional"]) + + +class TestReviewAndExit(unittest.TestCase): + def test_build_review_comment_lists_missing_items(self) -> None: + config = EnhanceConfig( + meta={ + "product": MetaRule("warning", "{PRODUCT}"), + "area": MetaRule("error", ""), + "experience": MetaRule("warning", ""), + }, + after_title=AFTER_TITLE_RULES, + ) + results = [ + { + "path": "source/Page.rst", + "meta_required": ["area"], + "meta_optional": ["product", "experience"], + "after_title_required": [], + "after_title_optional": ["short-description", "showmeta"], + }, + ] + body = build_review_comment(results, config=config) + self.assertIn(SUMMARY_REVIEW_TITLE, body) + self.assertIn("source/Page.rst", body) + self.assertIn("Missing `.. meta::` fields", body) + self.assertIn("Missing after-title directives", body) + self.assertIn("required", body) + self.assertIn("{PRODUCT}", body) + self.assertIn(":order: area, content-type, experience", body) + self.assertIn(REVIEW_MARKER, body) + + def test_local_exit_nonzero_only_for_error_severity(self) -> None: + warning_config = textwrap.dedent( + """ + meta: + product: + severity: warning + value: "{PRODUCT}" + """ + ).strip() + rules_path = Path(tempfile.mkdtemp()) / "meta.yaml" + rules_path.write_text(warning_config, encoding="utf-8") + with tempfile.TemporaryDirectory() as tmp: + warning_only = Path(tmp) / "warn.rst" + warning_only.write_text("Title\n=====\n", encoding="utf-8") + code = main( + [ + str(warning_only), + "--config", + str(rules_path), + ], + ) + self.assertEqual(code, 0) + + rules_path.write_text(SAMPLE_CONFIG, encoding="utf-8") + with tempfile.TemporaryDirectory() as tmp: + error_file = Path(tmp) / "err.rst" + error_file.write_text("Title\n=====\n", encoding="utf-8") + code = main( + [ + str(error_file), + "--config", + str(rules_path), + ], + ) + self.assertEqual(code, 1) + + +def _extract_multiline_output(status: str, key: str) -> str | None: + """Return the body of a GitHub Actions heredoc output block, or None if absent.""" + delimiter = f"EOF_{key.upper()}" + header = f"{key}<<{delimiter}\n" + start = status.find(header) + if start < 0: + return None + start += len(header) + end = status.find(f"\n{delimiter}\n", start) + if end < 0: + return None + return status[start:end] + + +class TestCiStatusOutputs(unittest.TestCase): + """The workflow gates steps on these outputs, so keep them self-consistent.""" + + def _run_with_status_file(self, content: str, *, config: str = META_ONLY_CONFIG) -> str: + rules_path = Path(tempfile.mkdtemp()) / "meta.yaml" + rules_path.write_text(config, encoding="utf-8") + with tempfile.TemporaryDirectory() as tmp: + page = Path(tmp) / "page.rst" + page.write_text(content, encoding="utf-8") + status_path = Path(tmp) / "status.txt" + main( + [ + str(page), + "--config", + str(rules_path), + "--status-file", + str(status_path), + ], + ) + return status_path.read_text(encoding="utf-8") + + def test_status_file_writes_comment_when_issues_remain(self) -> None: + status = self._run_with_status_file("Title\n=====\n") + self.assertIn("has_results=true", status) + self.assertIn("has_errors=true", status) + comment = _extract_multiline_output(status, "comment") + self.assertIsNotNone(comment) + self.assertIn(SUMMARY_REVIEW_TITLE, comment or "") + self.assertIn(REVIEW_MARKER, comment or "") + + def test_clean_file_writes_no_review_body(self) -> None: + content = textwrap.dedent( + """ + .. meta:: + :product: ROS 2 + :area: docs + :experience: beginner + + Title + ===== + + .. short-description:: + Summary for the page. + + .. showmeta:: + :order: area, content-type, experience + """ + ).lstrip() + status = self._run_with_status_file(content) + self.assertIn("has_results=false", status) + self.assertIn("has_errors=false", status) + self.assertNotIn("comment<<", status) + + +class TestChangedRstPaths(unittest.TestCase): + def test_parses_git_diff_output(self) -> None: + completed = mock.Mock(returncode=0, stdout="source/A.rst\nsource/B.rst\n", stderr="") + with mock.patch("ensure_enhancements.subprocess.run", return_value=completed) as run: + paths = changed_rst_paths("abc123") + self.assertEqual(paths, [Path("source/A.rst"), Path("source/B.rst")]) + run.assert_called_once() + call_args = run.call_args[0][0] + self.assertEqual(call_args[:4], ["git", "diff", "--name-only", "--diff-filter=ACMR"]) + self.assertIn("abc123...HEAD", call_args[4]) + + +class TestMainDiscovery(unittest.TestCase): + def test_requires_paths_or_diff_base(self) -> None: + with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as handle: + handle.write(SAMPLE_CONFIG) + config_path = Path(handle.name) + try: + with self.assertRaises(SystemExit) as ctx: + main(["--config", str(config_path)]) + self.assertEqual(ctx.exception.code, 2) + finally: + config_path.unlink() + + def test_empty_discovery_writes_enhancements_checked_false(self) -> None: + with tempfile.NamedTemporaryFile("w", suffix=".yaml", delete=False) as handle: + handle.write(SAMPLE_CONFIG) + config_path = Path(handle.name) + try: + with tempfile.NamedTemporaryFile("w", delete=False) as status_handle: + status_path = Path(status_handle.name) + try: + with mock.patch( + "ensure_enhancements.changed_rst_paths", + return_value=[], + ): + code = main( + [ + "--config", + str(config_path), + "--diff-base", + "base-sha", + "--status-file", + str(status_path), + ], + ) + self.assertEqual(code, 0) + status_text = status_path.read_text(encoding="utf-8") + self.assertIn("enhancements_checked=false", status_text) + self.assertIn("has_errors=false", status_text) + finally: + status_path.unlink() + finally: + config_path.unlink() + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/tests/test_rst_utils.py b/tools/tests/test_rst_utils.py new file mode 100644 index 00000000000..7e0577b6321 --- /dev/null +++ b/tools/tests/test_rst_utils.py @@ -0,0 +1,122 @@ +# Copyright 2026 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from __future__ import annotations + +import sys +import textwrap +import unittest +from pathlib import Path + +_TOOLS_DIR = Path(__file__).resolve().parent.parent +if str(_TOOLS_DIR) not in sys.path: + sys.path.insert(0, str(_TOOLS_DIR)) + +from rst_utils import ( # noqa: E402 + get_meta_fields_from_content, + has_short_description_content, + has_showmeta_with_order, +) + + +class TestMetaFields(unittest.TestCase): + def test_get_meta_fields_from_content(self) -> None: + content = textwrap.dedent( + """ + .. meta:: + :product: ROS 2 + :area: docs + + Title + ===== + """ + ).lstrip() + fields = get_meta_fields_from_content(content) + self.assertEqual(fields["product"], "ROS 2") + self.assertEqual(fields["area"], "docs") + + def test_returns_empty_when_no_meta_block(self) -> None: + content = "Title\n=====\n" + self.assertEqual(get_meta_fields_from_content(content), {}) + + +class TestShortDescription(unittest.TestCase): + def test_has_short_description_content(self) -> None: + content = textwrap.dedent( + """ + Title + ===== + + .. short-description:: + Existing summary. + + Body paragraph. + """ + ).lstrip() + self.assertTrue(has_short_description_content(content)) + + def test_empty_short_description_is_not_present(self) -> None: + content = textwrap.dedent( + """ + Title + ===== + + .. short-description:: + + Body paragraph. + """ + ).lstrip() + self.assertFalse(has_short_description_content(content)) + + def test_missing_short_description(self) -> None: + content = "Title\n=====\n\nBody paragraph.\n" + self.assertFalse(has_short_description_content(content)) + + +class TestShowmeta(unittest.TestCase): + def test_has_showmeta_with_order(self) -> None: + content = textwrap.dedent( + """ + Title + ===== + + .. showmeta:: + :order: area, content-type, experience + + Body content. + """ + ).lstrip() + self.assertTrue(has_showmeta_with_order(content)) + + def test_blank_order_is_not_present(self) -> None: + content = textwrap.dedent( + """ + Title + ===== + + .. showmeta:: + :order: + + Body content. + """ + ).lstrip() + self.assertFalse(has_showmeta_with_order(content)) + + def test_missing_showmeta(self) -> None: + content = "Title\n=====\n\nBody content.\n" + self.assertFalse(has_showmeta_with_order(content)) + + +if __name__ == "__main__": + unittest.main()