From a767ded5e1be81b64b4917cccce3892e8dbb2d3a Mon Sep 17 00:00:00 2001 From: Jens Tirsvad Nielsen Date: Mon, 19 Oct 2026 12:34:07 +0800 Subject: [PATCH] Add the reporting step and the reusable english-comments workflow tools/english_comments_report.py keeps one comment on the pull request that mentions the author, updates it on a re-run and marks it resolved when the check passes. english-comments.yml runs the check on the full history, tells the author even when the check fails, and then fails the run. A script that no release holds yet no longer counts as missing from the default tools-ref. Refs #201 Refs #202 Task: MIL-013#5 Task: MIL-013#6 --- .gitea/workflows/english-comments.yml | 116 +++++++++++ tests/test_english_comments_report.py | 277 ++++++++++++++++++++++++++ tests/test_tools_ref.py | 27 ++- tools/english_comments_report.py | 172 ++++++++++++++++ 4 files changed, 590 insertions(+), 2 deletions(-) create mode 100644 .gitea/workflows/english-comments.yml create mode 100644 tests/test_english_comments_report.py create mode 100644 tools/english_comments_report.py diff --git a/.gitea/workflows/english-comments.yml b/.gitea/workflows/english-comments.yml new file mode 100644 index 0000000..4402174 --- /dev/null +++ b/.gitea/workflows/english-comments.yml @@ -0,0 +1,116 @@ +name: English comments + +# Reusable workflow (MIL-013, ADR-0012). A repository calls it on its pull +# requests with +# +# on: +# pull_request: +# jobs: +# english-comments: +# uses: TirSystem/github-action/.gitea/workflows/english-comments.yml@v2 +# +# It reads the comments and docstrings that the pull request adds, judges their +# language, fails the run when one is not English and writes one comment on the +# pull request that mentions the author. It changes no file. Mark the check as +# required in the branch protection of the target branch to block the merge +# (the workflow cannot do that). +on: + workflow_call: + inputs: + scope: + description: "changed: the comments on the lines the pull request adds; all: every tracked file" + type: string + default: "changed" + exclude: + description: "Comma list of file name or path patterns that are not checked (* and ** are wildcards)" + type: string + default: "" + allowed-words-file: + description: "File of the repository with words that are never judged, one per line" + type: string + default: "" + threshold: + description: "Probability, above 0 and at most 1, from which a comment counts as not English (ADR-0012)" + type: string + default: "0.6" + min-words: + description: "A comment with fewer words is not judged (ADR-0012)" + type: string + default: "3" + langid-version: + description: "Version of the langid package" + type: string + default: "1.1.6" + numpy-version: + description: "Version of numpy, which langid needs" + type: string + default: "2.1.3" + tools-ref: + description: "Tag or branch of TirSystem/github-action whose tools/english_comments.py is run; pin it to the exact tag together with the workflow" + type: string + default: "v2" + secrets: + REPORT_TOKEN: + description: "Optional Gitea token that may comment on pull requests; without it the token of the run is used" + required: false + +jobs: + english-comments: + permissions: + contents: read + issues: write + pull-requests: write + runs-on: ubuntu-latest + timeout-minutes: 15 + env: + SCOPE: ${{ inputs.scope }} + EXCLUDE: ${{ inputs.exclude }} + ALLOWED_WORDS: ${{ inputs.allowed-words-file }} + THRESHOLD: ${{ inputs.threshold }} + MIN_WORDS: ${{ inputs.min-words }} + LANGID_VERSION: ${{ inputs.langid-version }} + NUMPY_VERSION: ${{ inputs.numpy-version }} + BASE_REF: ${{ gitea.base_ref }} + FINDINGS_FILE: /tmp/english-comments-findings.json + steps: + - name: Check out the repository + uses: actions/checkout@v4.2.2 + with: + fetch-depth: 0 + + - name: Fetch the shared scripts + uses: actions/checkout@v4.2.2 + with: + repository: TirSystem/github-action + ref: ${{ inputs.tools-ref }} + path: shared-scripts + + - name: Install the language detector + run: | + python3 -m venv /tmp/english-comments-venv + /tmp/english-comments-venv/bin/pip install --quiet "langid==${LANGID_VERSION}" "numpy==${NUMPY_VERSION}" + + - name: Check the comments + id: check + continue-on-error: true + run: >- + /tmp/english-comments-venv/bin/python shared-scripts/tools/english_comments.py check + --scope "$SCOPE" --base "origin/$BASE_REF" --exclude "shared-scripts/**,$EXCLUDE" + --allowed-words "$ALLOWED_WORDS" --threshold "$THRESHOLD" --min-words "$MIN_WORDS" + --output "$FINDINGS_FILE" + + - name: Tell the author + if: ${{ always() && gitea.event_name == 'pull_request' }} + continue-on-error: true + env: + GITEA_API_URL: ${{ gitea.api_url }} + SOURCE_REPOSITORY: ${{ gitea.repository }} + PR_NUMBER: ${{ gitea.event.pull_request.number }} + PR_AUTHOR: ${{ gitea.event.pull_request.user.login }} + RUN_URL: ${{ gitea.server_url }}/${{ gitea.repository }}/actions/runs/${{ gitea.run_number }} + REPORT_TOKEN: ${{ secrets.REPORT_TOKEN || gitea.token }} + run: python3 shared-scripts/tools/english_comments_report.py + + - name: Fail the run when a comment is not in English + if: ${{ steps.check.outcome == 'failure' }} + run: exit 1 diff --git a/tests/test_english_comments_report.py b/tests/test_english_comments_report.py new file mode 100644 index 0000000..6f21d4a --- /dev/null +++ b/tests/test_english_comments_report.py @@ -0,0 +1,277 @@ +"""Offline tests for tools/english_comments_report.py and .gitea/workflows/english-comments.yml. + +The Gitea API is a fake that records the writes, so no host is contacted. +""" + +import io +import json +import urllib.error + +import pytest +import yaml + +from workflow_source import REPO_ROOT, load_script + +report = load_script(REPO_ROOT / "tools" / "english_comments_report.py", "english_comments_report") +WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "english-comments.yml" +TOKEN = "t" * 40 +FINDING = { + "path": "src/a.py", + "line": 4, + "end_line": 4, + "language": "da", + "probability": 0.93, + "text": "Denne funktion beregner totalen", +} + + +class FakeGitea: + """Lists the comments of one pull request and records the writes.""" + + def __init__(self, comments=()): + self.comments = list(comments) + self.writes = [] + + def __call__(self, url, method="GET", headers=None, body=None): + assert headers["Authorization"] == f"token {TOKEN}" + if method == "GET": + return self.comments + self.writes.append((method, url.split("/repos/o/r")[-1], body["body"])) + return {"id": 99} + + +@pytest.fixture +def env(tmp_path, monkeypatch): + findings = tmp_path / "findings.json" + findings.write_text(json.dumps([FINDING]), encoding="utf-8") + values = { + "GITEA_API_URL": "https://git.example/api/v1/", + "SOURCE_REPOSITORY": "o/r", + "PR_NUMBER": "7", + "PR_AUTHOR": "anna", + "REPORT_TOKEN": TOKEN, + "FINDINGS_FILE": str(findings), + "RUN_URL": "https://git.example/o/r/actions/runs/5", + } + for key, value in values.items(): + monkeypatch.setenv(key, value) + monkeypatch.setattr("builtins.print", lambda *a, **k: None) + return findings + + +def write_findings(path, findings): + path.write_text(json.dumps(findings), encoding="utf-8") + + +def test_a_finding_writes_one_comment_that_mentions_the_author(env): + gitea = FakeGitea() + assert report.main(request=gitea) == 0 + [(method, path, body)] = gitea.writes + assert (method, path) == ("POST", "/issues/7/comments") + assert report.MARKER in body and body.startswith(report.MARKER) + assert "@anna" in body and "`src/a.py`" in body and "| 4 |" in body + assert "Denne funktion beregner totalen" in body + assert "[The run](https://git.example/o/r/actions/runs/5)" in body + + +def test_a_second_run_updates_the_comment_instead_of_adding_one(env): + old = {"id": 11, "body": f"{report.MARKER}\nsomething older"} + gitea = FakeGitea([{"id": 3, "body": "hello"}, old]) + assert report.main(request=gitea) == 0 + [(method, path, _)] = gitea.writes + assert (method, path) == ("PATCH", "/issues/comments/11") + + +def test_an_unchanged_comment_is_not_written_again(env): + body = report.failing_body("anna", [FINDING], "https://git.example/o/r/actions/runs/5") + gitea = FakeGitea([{"id": 11, "body": body}]) + assert report.main(request=gitea) == 0 + assert gitea.writes == [] + + +def test_a_passing_run_marks_the_comment_resolved_without_a_mention(env): + write_findings(env, []) + gitea = FakeGitea([{"id": 11, "body": f"{report.MARKER}\n@anna failing"}]) + assert report.main(request=gitea) == 0 + [(method, path, body)] = gitea.writes + assert (method, path) == ("PATCH", "/issues/comments/11") + assert "passes" in body and "@anna" not in body + + +def test_a_passing_run_with_no_earlier_comment_writes_nothing(env): + write_findings(env, []) + gitea = FakeGitea() + assert report.main(request=gitea) == 0 + assert gitea.writes == [] + + +def test_comments_of_other_authors_are_not_touched(env): + gitea = FakeGitea([{"id": 3, "body": "no marker here"}]) + report.main(request=gitea) + assert [w[0] for w in gitea.writes] == ["POST"] + + +def test_all_pages_of_comments_are_searched(env): + page = [{"id": n, "body": "x"} for n in range(report.PAGE_SIZE)] + calls = [] + + def paged(url, method="GET", headers=None, body=None): + if method != "GET": + calls.append((method, url)) + return None + number = int(url.rsplit("page=", 1)[1]) + return page if number == 1 else [{"id": 900, "body": report.MARKER}] + + assert report.main(request=paged) == 0 + assert calls == [("PATCH", "https://git.example/api/v1/repos/o/r/issues/comments/900")] + + +def test_no_token_writes_nothing_and_does_not_fail(env, monkeypatch): + monkeypatch.setenv("REPORT_TOKEN", "") + gitea = FakeGitea() + assert report.main(request=gitea) == 0 + assert gitea.writes == [] + + +def test_a_missing_findings_file_means_the_check_did_not_run(env, monkeypatch): + monkeypatch.setenv("FINDINGS_FILE", str(env.parent / "absent.json")) + assert report.main(request=FakeGitea()) == 0 + + +def test_an_unreadable_findings_file_is_a_configuration_error(env, capsys): + env.write_text("not json", encoding="utf-8") + assert report.main(request=FakeGitea()) == 1 + + +@pytest.mark.parametrize("name", ["GITEA_API_URL", "PR_NUMBER", "PR_AUTHOR"]) +def test_a_missing_setting_is_a_configuration_error(env, monkeypatch, name): + monkeypatch.delenv(name) + assert report.main(request=FakeGitea()) == 1 + + +def test_text_in_a_comment_cannot_mention_someone_or_break_the_table(): + body = report.failing_body("anna", [{**FINDING, "text": "ping @bob `x` | y\nz"}]) + assert "@bob" not in body.replace("@​bob", "") + assert "\\|" in body and "\n| " in body and "`x`" not in body + + +def test_long_lists_are_cut(env): + many = [{**FINDING, "line": n} for n in range(report.MAX_ROWS + 5)] + body = report.failing_body("anna", many) + assert "...and 5 more" in body + assert body.count("`src/a.py`") == report.MAX_ROWS + + +class FakeResponse(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + +def test_request_json_sends_the_body_and_reads_the_answer(monkeypatch): + seen = {} + + def fake_urlopen(request, timeout): + seen["method"] = request.get_method() + seen["data"] = request.data + return FakeResponse(b'{"id": 5}') + + monkeypatch.setattr(report.urllib.request, "urlopen", fake_urlopen) + assert report.request_json("https://h/x", "POST", {"A": "b"}, {"body": "t"}) == {"id": 5} + assert seen == {"method": "POST", "data": b'{"body": "t"}'} + + +def test_request_json_returns_none_for_an_empty_answer(monkeypatch): + monkeypatch.setattr(report.urllib.request, "urlopen", lambda request, timeout: FakeResponse(b"")) + assert report.request_json("https://h/x") is None + + +@pytest.mark.parametrize( + ("code", "category"), [(401, "credentials"), (403, "permissions"), (500, "api")] +) +def test_http_errors_have_a_category(monkeypatch, code, category): + def fail(request, timeout): + raise urllib.error.HTTPError(request.full_url, code, "reason", {}, None) + + monkeypatch.setattr(report.urllib.request, "urlopen", fail) + with pytest.raises(report.ReportError, match=rf"\[{category}\]"): + report.request_json("https://h/x?token=secret") + + +def test_an_unreachable_host_is_an_api_error(monkeypatch): + def fail(request, timeout): + raise urllib.error.URLError("down") + + monkeypatch.setattr(report.urllib.request, "urlopen", fail) + with pytest.raises(report.ReportError, match=r"\[api\]"): + report.request_json("https://h/x") + + +def test_an_api_error_is_printed_without_the_token_and_fails_the_step(env, monkeypatch, capsys): + monkeypatch.undo() # restore print + for key, value in { + "GITEA_API_URL": "https://git.example/api/v1", + "SOURCE_REPOSITORY": "o/r", + "PR_NUMBER": "7", + "PR_AUTHOR": "anna", + "REPORT_TOKEN": TOKEN, + "FINDINGS_FILE": str(env), + }.items(): + monkeypatch.setenv(key, value) + + def refuse(url, method="GET", headers=None, body=None): + raise report.ReportError("permissions", "the token may not comment") + + assert report.main(request=refuse) == 1 + out = capsys.readouterr().out + assert "::error::[permissions]" in out and TOKEN not in out + + +@pytest.fixture(scope="module") +def document(): + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + +class TestWorkflow: + + def test_is_a_reusable_workflow_with_the_contract_inputs(self, document): + call = document[True]["workflow_call"] # PyYAML reads `on` as True + assert set(call["inputs"]) == { + "scope", "exclude", "allowed-words-file", "threshold", "min-words", + "langid-version", "numpy-version", "tools-ref", + } + assert call["inputs"]["threshold"]["default"] == "0.6" + assert call["inputs"]["min-words"]["default"] == "3" + assert call["inputs"]["scope"]["default"] == "changed" + assert call["secrets"]["REPORT_TOKEN"]["required"] is False + + def test_permissions_are_explicit_and_small(self, document): + permissions = document["jobs"]["english-comments"]["permissions"] + assert permissions == {"contents": "read", "issues": "write", "pull-requests": "write"} + + def test_checks_out_the_full_history_for_the_merge_base(self, document): + first = document["jobs"]["english-comments"]["steps"][0] + assert first["with"]["fetch-depth"] == 0 + + def test_the_check_does_not_end_the_job_before_the_author_is_told(self, document): + steps = {s.get("name"): s for s in document["jobs"]["english-comments"]["steps"]} + assert steps["Check the comments"]["continue-on-error"] is True + tell = steps["Tell the author"] + assert tell["continue-on-error"] is True and "always()" in tell["if"] + assert "steps.check.outcome == 'failure'" in steps["Fail the run when a comment is not in English"]["if"] + + def test_inputs_reach_the_scripts_as_environment_variables_only(self, document): + for step in document["jobs"]["english-comments"]["steps"]: + assert "inputs." not in step.get("run", "") + assert "secrets." not in step.get("run", "") + + def test_the_token_falls_back_to_the_token_of_the_run(self, document): + tell = next(s for s in document["jobs"]["english-comments"]["steps"] if s.get("name") == "Tell the author") + assert "secrets.REPORT_TOKEN || gitea.token" in tell["env"]["REPORT_TOKEN"] + + def test_uses_gitea_contexts_only(self): + text = WORKFLOW.read_text(encoding="utf-8") + assert "gitea.event.pull_request.user.login" in text + assert "github." not in text.replace("TirSystem/github-action", "") diff --git a/tests/test_tools_ref.py b/tests/test_tools_ref.py index 3936f53..03dd362 100644 --- a/tests/test_tools_ref.py +++ b/tests/test_tools_ref.py @@ -58,8 +58,31 @@ def current_major(): return MAJOR_TAG_FILE.read_text(encoding="utf-8").strip() +def released_in_any_tag(path): + """True when some version tag holds the file; False for a script not released yet.""" + tags = subprocess.run( + ["git", "-C", str(REPO_ROOT), "tag", "--list", "v*"], + capture_output=True, + text=True, + check=False, + ).stdout.split() + return any( + subprocess.run( + ["git", "-C", str(REPO_ROOT), "cat-file", "-e", f"{tag}:{path}"], + capture_output=True, + check=False, + ).returncode + == 0 + for tag in tags + ) + + def script_in_tag(tag, path): - """True when the tag holds the file; True when the tag is not in this checkout.""" + """True when the tag holds the file; True when the tag is not in this checkout. + + A script that no tag holds yet is new in the unreleased version: the release + moves the major tag to it, so it does not count as missing (MIL-013). + """ known = subprocess.run( ["git", "-C", str(REPO_ROOT), "rev-parse", "--verify", "--quiet", f"refs/tags/{tag}"], capture_output=True, @@ -72,7 +95,7 @@ def script_in_tag(tag, path): capture_output=True, check=False, ) - return holds.returncode == 0 + return holds.returncode == 0 or not released_in_any_tag(path) def test_major_tag_file_names_the_highest_major_of_the_changelog(): diff --git a/tools/english_comments_report.py b/tools/english_comments_report.py new file mode 100644 index 0000000..14b7d74 --- /dev/null +++ b/tools/english_comments_report.py @@ -0,0 +1,172 @@ +"""Tell the author of a pull request which comments are not in English (MIL-013). + +Run by .gitea/workflows/english-comments.yml after the check. It reads the +findings file of tools/english_comments.py and keeps ONE comment on the pull +request: + + - findings: the comment mentions the author and lists each finding; an + existing comment of this check is updated, never a second one added; + - no findings: an existing comment of this check is marked resolved (and no + longer mentions the author); with none nothing is written. + +Settings come from the environment: + + GITEA_API_URL Gitea API base, for example https://host/api/v1 + SOURCE_REPOSITORY owner/repo of the pull request + PR_NUMBER the number of the pull request + PR_AUTHOR the login of the author, mentioned in the comment + REPORT_TOKEN a token that may comment on pull requests (a CI secret); + without it nothing is written and the step ends with a + warning (the check run itself still fails) + FINDINGS_FILE the JSON file written by english_comments.py --output + RUN_URL optional, linked in the comment + +Every failure is printed as ``::error::[category] message`` with the categories +configuration, credentials, permissions and api; the token is never printed. +""" + +import json +import os +import sys +import urllib.error +import urllib.request + +MARKER = "" +REQUEST_TIMEOUT_SECONDS = 30 +PAGE_SIZE = 50 +MAX_ROWS = 30 + + +class ReportError(RuntimeError): + """A failure with a category: configuration, credentials, permissions or api.""" + + def __init__(self, category: str, message: str) -> None: + super().__init__(f"[{category}] {message}") + + +def request_json(url, method="GET", headers=None, body=None): + request = urllib.request.Request( + url, + data=json.dumps(body).encode("utf-8") if body is not None else None, + headers=headers or {}, + method=method, + ) + try: + with urllib.request.urlopen(request, timeout=REQUEST_TIMEOUT_SECONDS) as response: + content = response.read() + return json.loads(content) if content else None + except urllib.error.HTTPError as error: + target = url.split("?")[0] + detail = f"Gitea API request failed with HTTP {error.code} ({error.reason}) for {method} {target}" + if error.code == 401: + raise ReportError("credentials", f"{detail}. The token was rejected.") from None + if error.code == 403: + raise ReportError( + "permissions", f"{detail}. The token may not comment on pull requests." + ) from None + raise ReportError("api", detail) from None + except (urllib.error.URLError, TimeoutError) as error: + raise ReportError("api", f"Gitea API is unreachable ({error}).") from None + + +def _cell(text: str, limit: int = 100) -> str: + """Text safe for a table cell: no pipes, line breaks, mentions or backticks.""" + text = " ".join(text.split()).replace("|", "\\|").replace("`", "'").replace("@", "@​") + return text if len(text) <= limit else text[: limit - 3] + "..." + + +def failing_body(author: str, findings: list[dict], run_url: str = "") -> str: + rows = [ + f"| `{_cell(f['path'], 80)}` | {f['line']} | {_cell(str(f['language']))} {f['probability']} | {_cell(f['text'])} |" + for f in findings[:MAX_ROWS] + ] + more = len(findings) - MAX_ROWS + lines = [ + MARKER, + f"@{author}, the **English comments** check found {len(findings)} comment(s) that do not look " + "like English. Source code comments are written in English only.", + "", + "| File | Line | Looks like | Comment |", + "| --- | --- | --- | --- |", + *rows, + ] + if more > 0: + lines.append(f"\n...and {more} more; see the run.") + lines += [ + "", + "Rewrite the comment in English. For a quotation or a name, add `english-comments: ignore` " + "to the comment, or add the word to the allowed-words file.", + ] + if run_url: + lines.append(f"\n[The run]({run_url})") + return "\n".join(lines) + + +def resolved_body() -> str: + return f"{MARKER}\nThe **English comments** check passes: every checked comment is in English." + + +def find_comment(api, headers, request, repository, number): + """Return the newest comment of this check on the pull request, or None.""" + found = None + page = 1 + while True: + url = f"{api}/repos/{repository}/issues/{number}/comments?limit={PAGE_SIZE}&page={page}" + comments = request(url, "GET", headers, None) or [] + for comment in comments: + if MARKER in (comment.get("body") or ""): + found = comment + if len(comments) < PAGE_SIZE: + return found + page += 1 + + +def main(request=request_json) -> int: + env = os.environ + token = env.get("REPORT_TOKEN", "") + try: + missing = [ + name + for name in ("GITEA_API_URL", "SOURCE_REPOSITORY", "PR_NUMBER", "PR_AUTHOR", "FINDINGS_FILE") + if not env.get(name) + ] + if missing: + raise ReportError("configuration", f"{', '.join(missing)} not set.") + if not os.path.exists(env["FINDINGS_FILE"]): + print("::warning::The check wrote no findings file (it did not run); nothing to report.") + return 0 + try: + with open(env["FINDINGS_FILE"], encoding="utf-8") as handle: + findings = json.load(handle) + except (OSError, ValueError): + raise ReportError("configuration", f"findings file '{env['FINDINGS_FILE']}' cannot be read.") from None + if not token: + print("::warning::No REPORT_TOKEN: the pull request is not commented. The check itself still decides.") + return 0 + api = env["GITEA_API_URL"].rstrip("/") + repository = env["SOURCE_REPOSITORY"] + number = env["PR_NUMBER"] + headers = {"Authorization": f"token {token}", "Content-Type": "application/json"} + existing = find_comment(api, headers, request, repository, number) + if findings: + body = failing_body(env["PR_AUTHOR"], findings, env.get("RUN_URL", "")) + elif existing: + body = resolved_body() + else: + print("No findings and no earlier comment: nothing to write.") + return 0 + if existing: + if existing.get("body") != body: + request(f"{api}/repos/{repository}/issues/comments/{existing['id']}", "PATCH", headers, {"body": body}) + print("Updated the comment of the check.") + else: + request(f"{api}/repos/{repository}/issues/{number}/comments", "POST", headers, {"body": body}) + print("Wrote a comment on the pull request.") + return 0 + except ReportError as error: + print(f"::error::{error}") + return 1 + + +if __name__ == "__main__": + sys.exit(main())