From 1bc746776a6cfda4e0b3e1873b31e792c95bd773 Mon Sep 17 00:00:00 2001 From: Paladin173 <35980893+Paladin173@users.noreply.github.com> Date: Wed, 26 Aug 2026 23:40:42 -0500 Subject: [PATCH] ci: harden docs-only checks --- .github/workflows/0-ci-build-and-test.yml | 42 ++----------- ARCHITECTURE.md | 1 + README.md | 11 ++++ tools/check_markdown.py | 8 +++ tools/detect_docs_only.py | 68 +++++++++++++++++++++ tools/tests/test_check_markdown.py | 42 +++++++++++++ tools/tests/test_detect_docs_only.py | 72 +++++++++++++++++++++++ tools/tests/test_workflow_contracts.py | 4 +- 8 files changed, 208 insertions(+), 40 deletions(-) create mode 100644 tools/detect_docs_only.py create mode 100644 tools/tests/test_detect_docs_only.py diff --git a/.github/workflows/0-ci-build-and-test.yml b/.github/workflows/0-ci-build-and-test.yml index 0c130c5..def3653 100644 --- a/.github/workflows/0-ci-build-and-test.yml +++ b/.github/workflows/0-ci-build-and-test.yml @@ -57,46 +57,12 @@ jobs: env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + BEFORE_SHA: ${{ github.event.before }} + SHA: ${{ github.sha }} run: | - # Fail-safe by construction: this step always exits 0 and always emits - # docs_only, defaulting to 'false' (= run the full suite). Any - # uncertainty - no base to diff, an unreadable diff, a single non-doc - # path - runs everything. It can over-run CI; it can never skip a - # Gradle gate on a change that touches real code. - docs_only="false" - emit() { echo "docs_only=$docs_only" >> "$GITHUB_OUTPUT"; } - trap emit EXIT - set -uo pipefail - - # Strict allowlist. A path is docs only if it carries a Markdown - # extension. Deliberately NOT docs: *.txt (requirements.txt controls - # dependencies), and any code file merely named README/CHANGELOG. - if [ -n "${BASE_SHA:-}" ] && [ -n "${HEAD_SHA:-}" ]; then - RANGE="$BASE_SHA...$HEAD_SHA" - else - RANGE="${{ github.event.before }}...${{ github.sha }}" - fi - - # --no-renames so `git mv MainActivity.kt notes.md` still reveals the - # source-file deletion instead of collapsing to a docs destination. - if ! FILES="$(git diff --name-only --no-renames "$RANGE" 2>/dev/null)"; then - echo "Could not diff $RANGE - running the full suite."; exit 0 - fi - if [ -z "$FILES" ]; then - echo "Empty diff - running the full suite."; exit 0 - fi - - echo "Changed files:"; echo "$FILES" - result="true" - while IFS= read -r file; do - [ -z "$file" ] && continue - case "$(printf '%s' "$file" | tr '[:upper:]' '[:lower:]')" in - *.md) ;; - *) echo "Non-docs path -> full suite required: $file"; result="false"; break ;; - esac - done <<< "$FILES" - docs_only="$result" + docs_only="$(python3 tools/detect_docs_only.py)" echo "docs_only=$docs_only" + echo "docs_only=$docs_only" >> "$GITHUB_OUTPUT" # Not a Markdown style linter. `docs-render` blocks only on breaks that render # as literal garbage or point at a file that does not exist; `docs-links` diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 49c3409..7c63eb9 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -70,6 +70,7 @@ Recent cleanup keeps `MainActivity` as the Android boundary while moving cluster - After artifacts are uploaded, `.github/workflows/1-orchestration-release.yml` publishes both targets in the same run: GitHub (`hermes-webui-v-github.apk`) and Play production (`hermes-webui-v.aab`) with the configured Play service account. - The beta workflow `.github/workflows/play-store-beta-manual.yml` remains available for manual/open-testing runs and is no longer part of default orchestration. - The build job generates release metadata once from `.github/release.yml` categories and bundles it with both artifacts. GitHub notes preserve clickable PR links; Play's brief `whatsnew-en-US` keeps compact PR/issue URLs, caps output below the Play limit, and appends the in-app bug-report reminder. Retry publishers validate version, tag, commit, build-run SHA, exact artifact name, and bundled metadata before publishing. +- CI's `tools/check_markdown.py` rejects rendering breaks, missing in-repository targets, and links whose resolved files escape the repository, including through symlinks. `tools/detect_docs_only.py` owns the docs-only Gradle-gate short-circuit and returns `false` for empty, unreadable, invalid, renamed, or mixed diffs. - Release workflows use concurrency groups so duplicate runs for the same ref or target version do not publish over each other. - The build and publish workflows validate that exactly one matching APK or AAB exists before upload or publication. - The publish workflows also support manual dispatch with the build run ID and artifact metadata so a failed GitHub or Play publish can be retried without rebuilding both release artifacts. diff --git a/README.md b/README.md index 47df524..6064cb7 100644 --- a/README.md +++ b/README.md @@ -249,6 +249,17 @@ Device check: .\gradlew.bat connectedDebugAndroidTest --no-daemon ``` +Repository tooling checks can be run with: + +```powershell +python -m unittest discover -s tools/tests -p 'test_*.py' -v +python tools/check_markdown.py +``` + +The docs-only CI short-circuit is fail-safe: only a non-empty diff containing +Markdown files exclusively can skip the Android Gradle gates. Unreadable, +empty, invalid, renamed, or mixed diffs run the full suite. + Release automation is centered on: - `.github/workflows/1-orchestration-release.yml` diff --git a/tools/check_markdown.py b/tools/check_markdown.py index c0e6c11..3091740 100644 --- a/tools/check_markdown.py +++ b/tools/check_markdown.py @@ -66,6 +66,7 @@ def check_rendering_breaks(path: Path, lines: list[tuple[int, str]]) -> list[Fin def check_repo_links(root: Path, path: Path, lines: list[tuple[int, str]]) -> list[Finding]: findings = [] + root = root.resolve() for number, line in lines: without_code = re.sub(r"`[^`]*`", "", line) for match in INLINE_LINK_RE.finditer(without_code): @@ -79,6 +80,13 @@ def check_repo_links(root: Path, path: Path, lines: list[tuple[int, str]]) -> li continue base = root if target_path.startswith("/") else path.parent target = (base / target_path.lstrip("/")).resolve() + try: + target.relative_to(root) + except ValueError: + findings.append( + Finding(path, number, f"link target outside repository: {dest}") + ) + continue if not target.exists(): findings.append(Finding(path, number, f"link target not found: {dest}")) return findings diff --git a/tools/detect_docs_only.py b/tools/detect_docs_only.py new file mode 100644 index 0000000..c80ee2b --- /dev/null +++ b/tools/detect_docs_only.py @@ -0,0 +1,68 @@ +#!/usr/bin/env python3 +"""Report whether the current Git diff contains Markdown files only.""" + +from __future__ import annotations + +import argparse +import os +import subprocess +import sys +from collections.abc import Iterable + + +def classify_paths(paths: Iterable[str]) -> bool: + paths = list(paths) + return bool(paths) and all(path.lower().endswith(".md") for path in paths) + + +def changed_paths(base: str, head: str, before: str, sha: str) -> list[str] | None: + if base and head: + revision_range = f"{base}...{head}" + elif before and sha: + revision_range = f"{before}...{sha}" + else: + return None + + if "0" * 40 in revision_range: + return None + + try: + result = subprocess.run( + ["git", "diff", "--name-only", "--no-renames", revision_range], + capture_output=True, + text=True, + check=False, + ) + except OSError: + print("Could not run git diff - running the full suite.", file=sys.stderr) + return None + if result.returncode != 0: + print(f"Could not diff {revision_range} - running the full suite.", file=sys.stderr) + return None + return result.stdout.splitlines() + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--base", default=os.environ.get("BASE_SHA", "")) + parser.add_argument("--head", default=os.environ.get("HEAD_SHA", "")) + parser.add_argument("--before", default=os.environ.get("BEFORE_SHA", "")) + parser.add_argument("--sha", default=os.environ.get("SHA", "")) + args = parser.parse_args(argv) + + paths = changed_paths(args.base, args.head, args.before, args.sha) + if paths is None: + print("false") + return 0 + + if not paths: + print("Empty diff - running the full suite.", file=sys.stderr) + print("false") + return 0 + + print("true" if classify_paths(paths) else "false") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) \ No newline at end of file diff --git a/tools/tests/test_check_markdown.py b/tools/tests/test_check_markdown.py index fc7067b..aac0028 100644 --- a/tools/tests/test_check_markdown.py +++ b/tools/tests/test_check_markdown.py @@ -44,6 +44,48 @@ def test_missing_relative_target_is_reported(self) -> None: def test_existing_relative_target_passes(self) -> None: self.assertEqual(self._check("See [the guide](guide.md).", ("guide.md",)), []) + def test_existing_target_outside_repository_is_reported(self) -> None: + with TemporaryDirectory() as tmp, TemporaryDirectory() as outside_tmp: + root = Path(tmp) + outside = Path(outside_tmp) / "guide.md" + outside.write_text("", encoding="utf-8") + doc = root / "docs" / "doc.md" + doc.parent.mkdir() + doc.write_text("See [the guide](../../" + outside.name + ").", encoding="utf-8") + findings = check_file(root, doc) + self.assertEqual( + [finding.message for finding in findings], + ["link target outside repository: ../../" + outside.name], + ) + + def test_symlink_target_outside_repository_is_reported(self) -> None: + with TemporaryDirectory() as tmp, TemporaryDirectory() as outside_tmp: + root = Path(tmp) + outside = Path(outside_tmp) / "guide.md" + outside.write_text("", encoding="utf-8") + link = root / "guide.md" + try: + link.symlink_to(outside) + except (NotImplementedError, OSError) as error: + self.skipTest(f"symlinks unavailable: {error}") + doc = root / "doc.md" + doc.write_text("See [the guide](guide.md).", encoding="utf-8") + findings = check_file(root, doc) + self.assertEqual( + [finding.message for finding in findings], + ["link target outside repository: guide.md"], + ) + + def test_parent_target_inside_repository_passes(self) -> None: + with TemporaryDirectory() as tmp: + root = Path(tmp) + guide = root / "guide.md" + guide.write_text("", encoding="utf-8") + doc = root / "docs" / "doc.md" + doc.parent.mkdir() + doc.write_text("See [the guide](../guide.md).", encoding="utf-8") + self.assertEqual(check_file(root, doc), []) + def test_anchor_on_a_real_file_passes(self) -> None: self.assertEqual( self._check("See [setup](guide.md#setup).", ("guide.md",)), diff --git a/tools/tests/test_detect_docs_only.py b/tools/tests/test_detect_docs_only.py new file mode 100644 index 0000000..1060590 --- /dev/null +++ b/tools/tests/test_detect_docs_only.py @@ -0,0 +1,72 @@ +import io +import sys +import unittest +from contextlib import redirect_stdout +from pathlib import Path +from unittest.mock import patch + +ROOT = Path(__file__).resolve().parents[2] +sys.path.insert(0, str(ROOT / "tools")) + +from detect_docs_only import main # noqa: E402 + + +class DocsOnlyDetectorTests(unittest.TestCase): + def _run(self, paths: str, **arguments: str) -> str: + completed = type("Completed", (), {"returncode": 0, "stdout": paths})() + output = io.StringIO() + with patch("detect_docs_only.subprocess.run", return_value=completed), redirect_stdout(output): + self.assertEqual( + main(sum(([f"--{key}", value] for key, value in arguments.items()), [])), + 0, + ) + return output.getvalue().strip() + + def test_markdown_paths_are_docs_only(self) -> None: + self.assertEqual(self._run("README.md\nUPPER.MD\n", base="base", head="head"), "true") + + def test_mixed_paths_require_full_suite(self) -> None: + self.assertEqual(self._run("README.md\napp/src/MainActivity.kt\n", base="base", head="head"), "false") + + def test_empty_diff_requires_full_suite(self) -> None: + self.assertEqual(self._run("", base="base", head="head"), "false") + + def test_missing_revision_range_requires_full_suite(self) -> None: + output = io.StringIO() + with redirect_stdout(output): + self.assertEqual(main([]), 0) + self.assertEqual(output.getvalue().strip(), "false") + + def test_invalid_diff_requires_full_suite(self) -> None: + completed = type("Completed", (), {"returncode": 128, "stdout": ""})() + output = io.StringIO() + with patch("detect_docs_only.subprocess.run", return_value=completed), redirect_stdout(output): + self.assertEqual(main(["--base", "base", "--head", "head"]), 0) + self.assertEqual(output.getvalue().strip(), "false") + + def test_unavailable_git_requires_full_suite(self) -> None: + output = io.StringIO() + with patch("detect_docs_only.subprocess.run", side_effect=OSError), redirect_stdout(output): + self.assertEqual(main(["--base", "base", "--head", "head"]), 0) + self.assertEqual(output.getvalue().strip(), "false") + + def test_push_event_revision_fallback_is_supported(self) -> None: + self.assertEqual(self._run("README.md\n", before="before", sha="head"), "true") + + def test_all_zero_revision_requires_full_suite(self) -> None: + zero = "0" * 40 + self.assertEqual(self._run("README.md\n", base=zero, head="head"), "false") + + def test_diff_disables_rename_detection(self) -> None: + completed = type("Completed", (), {"returncode": 0, "stdout": "README.md\n"})() + output = io.StringIO() + with patch("detect_docs_only.subprocess.run", return_value=completed) as run, redirect_stdout(output): + self.assertEqual(main(["--base", "base", "--head", "head"]), 0) + self.assertEqual( + run.call_args.args[0], + ["git", "diff", "--name-only", "--no-renames", "base...head"], + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tools/tests/test_workflow_contracts.py b/tools/tests/test_workflow_contracts.py index bc77126..ae5e501 100644 --- a/tools/tests/test_workflow_contracts.py +++ b/tools/tests/test_workflow_contracts.py @@ -111,8 +111,8 @@ def test_docs_only_change_sets_skip_the_gradle_gates_but_fail_safe(self) -> None # change is exactly when they matter most. self.assertNotIn("docs_only", blocks["release-tooling-tests"]) # The detector must default to running everything. - self.assertIn('docs_only="false"', blocks["changes"]) - self.assertIn("trap emit EXIT", blocks["changes"]) + self.assertIn("python3 tools/detect_docs_only.py", blocks["changes"]) + self.assertTrue((ROOT / "tools" / "detect_docs_only.py").exists()) def test_required_check_candidates_always_report_a_conclusion(self) -> None: """Jobs intended as required status checks must never be skipped.