diff --git a/.github/workflows/pr-syntax-check.yml b/.github/workflows/pr-syntax-check.yml new file mode 100644 index 00000000..d46bae02 --- /dev/null +++ b/.github/workflows/pr-syntax-check.yml @@ -0,0 +1,94 @@ +name: PR Syntax Check + +# Validates the syntax of config/content files modified by a pull request +# (.yml/.yaml, .json, .md/.markdown, requirements.txt, Makefile). A problem +# fails the check (visible red X) and, on same-repo PRs, posts inline review +# suggestions. This check is intentionally NOT a required status check — it +# does not block merging, it just makes syntax problems visible before merge. + +on: + pull_request: + types: [opened, synchronize, reopened] + +permissions: + contents: read + pull-requests: write + +# One run per PR; cancel superseded runs on rapid pushes. +concurrency: + group: pr-syntax-check-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + syntax-check: + name: Validate changed files + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v7.0.0 + with: + fetch-depth: 0 # full history so base...head diff is computable + + - name: Set up Python + uses: actions/setup-python@v6.3.0 + with: + python-version: '3.11' + + - name: Install checker dependencies + # Pin to the exact versions from requirements.txt so the checker's + # parser behaviour stays reproducible and in sync with the site build + # (requirements.txt is the single source of truth, bumped monthly). + run: | + pip install \ + "$(grep -iE '^pyyaml==' requirements.txt)" \ + "$(grep -iE '^packaging==' requirements.txt)" + + - name: Determine changed files + id: changed + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + # Added/copied/modified/renamed files only (skip deletions), then + # keep only the types we validate. + git diff --name-only --diff-filter=ACMR "${BASE_SHA}...${HEAD_SHA}" \ + | grep -E '(\.ya?ml|\.json|\.md|\.markdown)$|(^|/)requirements\.txt$|(^|/)Makefile$' \ + > changed.txt || true + echo "Files to check:" + cat changed.txt + echo "count=$(wc -l < changed.txt | tr -d ' ')" >> "$GITHUB_OUTPUT" + + - name: Run syntax checker + if: steps.changed.outputs.count != '0' + run: | + mapfile -t FILES < changed.txt + python3 scripts/ci/check-syntax.py "${FILES[@]}" \ + --findings-out syntax-findings.json + + - name: Post inline review suggestions + # Fork PRs get a read-only token and cannot create reviews; they still + # receive annotations and the failing check below. + if: >- + steps.changed.outputs.count != '0' && + github.event.pull_request.head.repo.full_name == github.repository + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + FINDINGS: syntax-findings.json + run: python3 scripts/ci/post-review.py + + - name: Fail if problems were found + if: steps.changed.outputs.count != '0' + run: | + python3 - <<'PY' + import json, sys + findings = json.load(open("syntax-findings.json")) + if findings: + print(f"{len(findings)} syntax problem(s) found in changed files " + f"- see the annotations and inline comments above.") + sys.exit(1) + print("All changed files passed syntax validation.") + PY diff --git a/scripts/ci/check-syntax.py b/scripts/ci/check-syntax.py new file mode 100644 index 00000000..cf004340 --- /dev/null +++ b/scripts/ci/check-syntax.py @@ -0,0 +1,300 @@ +#!/usr/bin/env python3 +""" +check-syntax.py — validate the syntax of PR-modified config/content files. + +Called by .github/workflows/pr-syntax-check.yml with a list of changed file +paths. Each file is validated according to its type (see CHECKERS below). The +goal is *syntax validity only* — is the file parseable / well-formed — not +style or formatting opinions. + +Output: + - GitHub Actions error annotations (::error file=...,line=...::message) for + every problem. These render inline on the PR "Files changed" tab and work + for fork PRs too, since they come from the check run rather than the API. + - A findings JSON file (path given by --findings-out) that a later workflow + step reads to (a) post inline review suggestions on same-repo PRs and + (b) fail the job when non-empty. + +This script itself always exits 0 — failing the job is the enforce step's +job, so that annotations print and suggestions post first regardless. + +A "suggestion" in a finding is an unambiguous one-line fix (e.g. a Makefile +recipe line that must start with a tab). Genuine syntax breaks (a missing +JSON brace, bad YAML indentation) get a precise location + parser message but +no suggestion, because the correct repair cannot be guessed reliably. +""" + +import argparse +import json +import os +import re +import sys + +# packaging + pyyaml are installed by the workflow (pip install) before this +# script runs. Import lazily inside the checkers so a missing dep only affects +# the relevant type rather than aborting the whole run. + + +def _finding(path, line, col, message, suggestion=None): + """Build one finding dict. `suggestion`, if given, is the full replacement + text for `line` (1-based) offered as a one-click review suggestion.""" + return { + "path": path, + "line": max(1, int(line)), + "col": max(1, int(col)) if col else 1, + "message": message, + "suggestion": suggestion, + } + + +def _escape_data(value): + """Escape a workflow-command *message*. GitHub decodes these sequences, + so untrusted text (e.g. an invalid requirement string from the PR) must be + escaped or it could inject additional ::commands. Escape % first so the + %0A/%0D we introduce aren't themselves re-escaped.""" + return ( + str(value) + .replace("%", "%25") + .replace("\r", "%0D") + .replace("\n", "%0A") + ) + + +def _escape_property(value): + """Escape a workflow-command *property* value (e.g. file). Same as data, + plus ':' and ',' which are property delimiters.""" + return _escape_data(value).replace(":", "%3A").replace(",", "%2C") + + +def check_yaml(path, text): + """Validate YAML syntax. Uses compose_all, which parses the node graph + without constructing Python objects — so it tolerates the + `!!python/name:...` tags in config/*/mkdocs.yml that a plain safe_load + would reject with a false 'could not determine a constructor' error.""" + import yaml + + findings = [] + try: + # Consume the generator so multi-document files are fully parsed. + list(yaml.compose_all(text)) + except yaml.YAMLError as exc: + line, col = 1, 1 + mark = getattr(exc, "problem_mark", None) + if mark is not None: + line, col = mark.line + 1, mark.column + 1 + problem = getattr(exc, "problem", None) or str(exc).splitlines()[0] + findings.append(_finding(path, line, col, f"Invalid YAML: {problem}")) + return findings + + +def check_json(path, text): + """Validate JSON syntax.""" + findings = [] + try: + json.loads(text) + except json.JSONDecodeError as exc: + findings.append( + _finding(path, exc.lineno, exc.colno, f"Invalid JSON: {exc.msg}") + ) + return findings + + +def check_requirements(path, text): + """Validate each requirement specifier in a pip requirements file. + + Skips blanks, comments, option lines (-r, -e, --hash, ...) and VCS/URL + installs (git+..., https://...), which are valid but not parseable by + packaging.requirements.Requirement.""" + from packaging.requirements import InvalidRequirement, Requirement + + findings = [] + for i, raw in enumerate(text.splitlines(), start=1): + line = raw.strip() + if not line or line.startswith("#") or line.startswith("-"): + continue + # Strip inline comments (" #" per pip's grammar). + if " #" in line: + line = line.split(" #", 1)[0].strip() + if "://" in line or line.startswith("git+"): + continue + try: + Requirement(line) + except InvalidRequirement as exc: + findings.append( + _finding(path, i, 1, f"Invalid requirement specifier: {exc}") + ) + return findings + + +def check_makefile(path, text): + """Flag recipe lines indented with spaces instead of a tab — the classic + 'missing separator' Makefile syntax error. Conservative and stateful to + keep false positives low: a line is treated as a recipe line only when it + is indented AND the current context is inside a rule (i.e. following a + target line, with no blank line having closed the recipe block). + + This is the one auto-fixable case here, so it emits a suggestion that + replaces the leading spaces with a single tab.""" + findings = [] + in_rule = False + in_define = False + lines = text.splitlines() + for i, raw in enumerate(lines, start=1): + # A define ... endef block holds arbitrary text where any indentation + # (tabs or spaces) is valid, so skip recipe-indentation checks inside + # it. Otherwise a tab-indented line in the block would wrongly set + # in_rule and produce false positives on later space-indented lines. + if in_define: + # endef ends the block. Leading whitespace before a directive may + # only be spaces — a tab would make it a recipe line, not endef. + if re.match(r" *endef(\s|$)", raw): + in_define = False + continue + if re.match(r" *define(\s|$)", raw): + in_define = True + in_rule = False + continue + if raw.strip() == "": + in_rule = False + continue + if raw.startswith("\t"): + # Already a proper (tab-indented) recipe line. + in_rule = True + continue + if raw.startswith(" "): + if in_rule: + stripped = raw.lstrip(" ") + findings.append( + _finding( + path, + i, + 1, + "Recipe line is indented with spaces; GNU make " + "requires a tab (causes 'missing separator').", + suggestion="\t" + stripped, + ) + ) + # Non-recipe indented line (e.g. inside a define): leave in_rule. + continue + # A line starting at column 0 that isn't blank: does it open a rule? + # A target line has an unescaped ':' and is not a variable assignment. + without_comment = raw.split("#", 1)[0] + is_assignment = any( + op in without_comment for op in (":=", "::=", "?=", "+=") + ) or ( + "=" in without_comment + and ":" not in without_comment.split("=", 1)[0] + ) + in_rule = (":" in without_comment) and not is_assignment + return findings + + +def check_markdown(path, text): + """Detect an unclosed fenced code block (``` or ~~~) — the one Markdown + 'syntax' error with real impact (it renders the rest of the page as code). + Annotate-only: where the fence *should* have closed is ambiguous, so no + one-click suggestion is offered.""" + findings = [] + fence_char = None + fence_len = 0 + fence_line = 0 + for i, raw in enumerate(text.splitlines(), start=1): + stripped = raw.lstrip(" ") + indent = len(raw) - len(stripped) + if indent > 3: # 4+ spaces is an indented code block, not a fence. + continue + ch = stripped[:1] + if ch not in ("`", "~"): + continue + run = len(stripped) - len(stripped.lstrip(ch)) + if run < 3: + continue + rest = stripped[run:] + if fence_char is None: + # Opening fence. An info string may follow (e.g. ```python). + fence_char, fence_len, fence_line = ch, run, i + elif ch == fence_char and run >= fence_len and rest.strip() == "": + # Closing fence: same char, length >= opening, nothing trailing. + fence_char, fence_len, fence_line = None, 0, 0 + if fence_char is not None: + findings.append( + _finding( + path, + fence_line, + 1, + f"Unclosed code fence opened here (missing closing " + f"'{fence_char * fence_len}').", + ) + ) + return findings + + +def checker_for(path): + """Return the checker function for a path, or None if unhandled.""" + base = os.path.basename(path) + if base == "Makefile": + return check_makefile + if base == "requirements.txt": + return check_requirements + ext = os.path.splitext(path)[1].lower() + return { + ".yml": check_yaml, + ".yaml": check_yaml, + ".json": check_json, + ".md": check_markdown, + ".markdown": check_markdown, + }.get(ext) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("files", nargs="*", help="paths to check") + parser.add_argument( + "--findings-out", + default="syntax-findings.json", + help="where to write the findings JSON", + ) + args = parser.parse_args() + + all_findings = [] + for path in args.files: + if not os.path.isfile(path): + # File deleted in the PR (still shows up in the diff) — skip. + continue + checker = checker_for(path) + if checker is None: + continue + try: + with open(path, "r", encoding="utf-8") as handle: + text = handle.read() + except (OSError, UnicodeDecodeError) as exc: + all_findings.append(_finding(path, 1, 1, f"Could not read file: {exc}")) + continue + all_findings.extend(checker(path, text)) + + # Emit GitHub Actions annotations (inline on the Files-changed tab). + for f in all_findings: + print( + f"::error file={_escape_property(f['path'])}," + f"line={f['line']},col={f['col']}::{_escape_data(f['message'])}" + ) + + with open(args.findings_out, "w", encoding="utf-8") as handle: + json.dump(all_findings, handle, indent=2) + + if all_findings: + print( + f"\nFound {len(all_findings)} syntax problem(s) across " + f"{len({f['path'] for f in all_findings})} file(s).", + file=sys.stderr, + ) + else: + print("No syntax problems found in changed files.", file=sys.stderr) + + # Always exit 0 — the workflow's enforce step decides pass/fail so that + # annotations print and suggestions post first. + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci/post-review.py b/scripts/ci/post-review.py new file mode 100644 index 00000000..eb86b801 --- /dev/null +++ b/scripts/ci/post-review.py @@ -0,0 +1,186 @@ +#!/usr/bin/env python3 +""" +post-review.py — post syntax-check findings as one inline PR review. + +Reads the findings JSON produced by check-syntax.py and posts a single review +(event=COMMENT) whose comments sit on the offending lines, embedding a +``suggestion`` block when the finding carries an unambiguous fix. + +Only runs on same-repo PRs (the workflow gates this), because PRs from forks +get a read-only GITHUB_TOKEN that cannot create reviews. Those PRs still get +the inline annotations and the failing check. + +GitHub rejects an entire review if any comment targets a line that is not part +of the PR diff, so findings are filtered to lines actually added/modified by +the PR (parsed from `git diff`). Findings on unchanged lines of a changed file +fall back to the annotation that check-syntax.py already emitted. + +Configuration comes from the environment: + GH_TOKEN - token with pull-requests: write + REPO - "owner/name" + PR_NUMBER - pull request number + BASE_SHA - PR base commit + HEAD_SHA - PR head commit (used as the review commit_id) + FINDINGS - path to findings JSON (default: syntax-findings.json) +""" + +import json +import os +import re +import subprocess +import sys +import urllib.error +import urllib.request + +HUNK_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") + + +def added_lines(base_sha, head_sha, path): + """Return the set of new-file line numbers added/modified for `path` in + the PR diff, parsed from unified diff hunk headers.""" + result = subprocess.run( + ["git", "diff", "--unified=0", f"{base_sha}...{head_sha}", "--", path], + capture_output=True, + text=True, + ) + if result.returncode != 0: + return set() + + lines = set() + new_line = 0 + in_hunk = False + for row in result.stdout.splitlines(): + header = HUNK_RE.match(row) + if header: + new_line = int(header.group(1)) + in_hunk = True + continue + if not in_hunk: + # Pre-hunk header region ("diff --git", "index", "---", "+++"). + # Skipping here avoids mistaking the "+++ b/path" header for an + # added line, while still counting real added lines whose content + # begins with "+++" (those only appear after the first hunk). + continue + if row.startswith("\\"): + # "\ No newline at end of file" marker — not a real line, and must + # not advance the new-side counter or later hunks desync. + continue + if row.startswith("+"): + lines.add(new_line) + new_line += 1 + elif row.startswith("-"): + # Deletion — does not exist on the new side; do not advance. + continue + else: + # Context line (rare with --unified=0) advances the new counter. + new_line += 1 + return lines + + +def comment_body(finding): + """Render a review-comment body, with a suggestion block when available.""" + message = finding["message"] + suggestion = finding.get("suggestion") + if suggestion is not None: + return ( + f"{message}\n\n" + f"Suggested fix:\n\n" + f"```suggestion\n{suggestion}\n```" + ) + return message + + +def post_review(repo, pr_number, token, commit_id, comments): + """POST a single review with the given inline comments.""" + payload = { + "commit_id": commit_id, + "event": "COMMENT", + "body": ( + "Automated syntax check found issues in changed files. " + "See the inline comments below." + ), + "comments": comments, + } + request = urllib.request.Request( + f"https://api.github.com/repos/{repo}/pulls/{pr_number}/reviews", + data=json.dumps(payload).encode("utf-8"), + method="POST", + headers={ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "Content-Type": "application/json", + }, + ) + # Never fail the workflow on a review-posting problem — annotations and + # the enforce step already communicate the failure. HTTPError is caught + # first for its response body; OSError covers URLError (DNS/TLS/refused) + # and bare timeouts, both of which subclass OSError. + try: + with urllib.request.urlopen(request, timeout=30) as response: + print(f"Posted review ({response.status}) with {len(comments)} comment(s).") + except urllib.error.HTTPError as exc: + detail = exc.read().decode("utf-8", "replace") + print( + f"Warning: could not post review ({exc.code}): {detail}", + file=sys.stderr, + ) + except OSError as exc: + print(f"Warning: could not post review: {exc}", file=sys.stderr) + + +def main(): + findings_path = os.environ.get("FINDINGS", "syntax-findings.json") + try: + with open(findings_path, "r", encoding="utf-8") as handle: + findings = json.load(handle) + except (OSError, json.JSONDecodeError) as exc: + # Best-effort: if the findings file is missing or malformed (e.g. the + # checker step errored before writing it), rely on annotations and the + # enforce step rather than failing the job from here. + print( + f"Warning: could not read findings ({exc}); skipping review.", + file=sys.stderr, + ) + return 0 + if not findings: + print("No findings; nothing to post.") + return 0 + + repo = os.environ["REPO"] + pr_number = os.environ["PR_NUMBER"] + token = os.environ["GH_TOKEN"] + base_sha = os.environ["BASE_SHA"] + head_sha = os.environ["HEAD_SHA"] + + # Cache the added-line set per file. + added_by_file = {} + comments = [] + for f in findings: + path = f["path"] + if path not in added_by_file: + added_by_file[path] = added_lines(base_sha, head_sha, path) + if f["line"] not in added_by_file[path]: + # Not on a changed line — annotation-only, skip inline comment. + continue + comments.append( + { + "path": path, + "line": f["line"], + "side": "RIGHT", + "body": comment_body(f), + } + ) + + if not comments: + print( + "All findings are on unchanged lines; relying on annotations only." + ) + return 0 + + post_review(repo, pr_number, token, head_sha, comments) + return 0 + + +if __name__ == "__main__": + sys.exit(main())