diff --git a/README.md b/README.md index 2bfa154..d2cd37e 100644 --- a/README.md +++ b/README.md @@ -201,6 +201,34 @@ for all available options. > [Optional Inputs](#optional-inputs), so env vars and config files are the > recommended way to customize. +## Outputs + +### `result` + +Structured check results as JSON, available to downstream steps via +[`fromJSON`](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#fromjson): + +```yaml +- uses: commit-check/commit-check-action@v2 + id: commit-check + with: + dry-run: true # (1) + +- name: Inspect results + run: | + echo "Status: ${{ fromJSON(steps.commit-check.outputs.result).status }}" + echo "Scopes: ${{ toJSON(fromJSON(steps.commit-check.outputs.result).scopes) }}" +``` + +1. Without `dry-run`, a failing check ends the job before any later step runs. + Use `dry-run` (or `continue-on-error`) when a downstream step is meant to + read the result and decide for itself. + +Each scope carries the check outcomes (`rule_id`, `check`, `status`, `value`, +`error`, `suggest`, `docs_url`) exactly as produced by +`commit-check --format json`, so downstream jobs can build their own reports +or gate on individual rules. + ## GitHub Action Job Summary By default, commit-check-action results are shown on the job summary page of the workflow. diff --git a/action.yml b/action.yml index 0018a52..d02f20a 100644 --- a/action.yml +++ b/action.yml @@ -37,10 +37,19 @@ inputs: description: check pull request title following conventional commits required: false default: false +outputs: + result: + description: Structured check results as JSON (status + per-scope checks). Consume with fromJSON(steps..outputs.result). + # Composite actions do not forward step outputs automatically: without this + # mapping (and the step id it refers to) the output is always the empty + # string, and fromJSON('') fails the calling workflow. + value: ${{ steps.commit-check.outputs.result }} + runs: using: "composite" steps: - name: Install dependencies and run commit-check + id: commit-check shell: bash run: | # Platform-specific settings diff --git a/assets/logo.png b/assets/logo.png new file mode 100644 index 0000000..240abb1 Binary files /dev/null and b/assets/logo.png differ diff --git a/main.py b/main.py index 1db4704..8a76005 100755 --- a/main.py +++ b/main.py @@ -1,19 +1,62 @@ #!/usr/bin/env python3 +"""GitHub Action that runs commit-check and renders results. + +The action runs ``commit-check --format json`` to collect structured check +results (rule IDs, error messages, suggestions, docs links), then renders +them to three output surfaces: + +* **step log** — grouped sections with ``::error`` annotations per rule +* **job summary** — a Markdown policy report table +* **PR comment** — a compact Markdown summary (idempotently updated) +""" + import json import os -import re import subprocess import sys -import tempfile -from typing import TextIO +from dataclasses import dataclass, field +from typing import Any -# Constants for message titles -SUCCESS_TITLE = "# Commit-Check ✔️" -FAILURE_TITLE = "# Commit-Check ❌" COMMIT_MESSAGE_DELIMITER = "\x00" -COMMIT_SECTION_SEPARATOR = "\n---\n" - -GITHUB_STEP_SUMMARY = os.environ["GITHUB_STEP_SUMMARY"] +RULES_URL = "https://commit-check.com/rules/" + +#: Hidden marker identifying comments this action owns. +# +# Comment identity has to be something a human cannot type by accident. The +# previous title-prefix match meant any comment opening with "# Commit Check" +# was treated as ours — and old ones are deleted, not just skipped. An HTML +# comment is invisible in the rendered body and is what Codecov, SonarQube and +# CodSpeed all use for the same purpose. +COMMENT_MARKER = "" + +#: Logo shown next to the report title. +# +# Served from this repository rather than commit-check.com so the report has no +# cross-repository dependency, and as PNG rather than SVG because GitHub proxies +# comment images through camo, which handles SVG unreliably. Point this at a +# single org-wide asset if the other tools grow the same header. +LOGO_URL = ( + "https://raw.githubusercontent.com/commit-check/commit-check-action/main/" + "assets/logo.png" +) + +#: Report heading. h2 rather than h1: this renders inside a PR comment, where an +#: h1 is louder than anything else on the page. +REPORT_TITLE = f'## Commit Check' + +#: Prefixes of report bodies written by earlier versions, kept so the first run +#: after upgrading adopts the existing comment instead of posting a second one. +#: Drop these once a release has been out long enough. +LEGACY_TITLES = ("# Commit Check", "# Commit-Check") + +GITHUB_STEP_SUMMARY = os.getenv("GITHUB_STEP_SUMMARY", "") + +#: Human-readable labels for the non-message CLI flags. +CHECK_LABELS = { + "--branch": "Branch", + "--author-name": "Author name", + "--author-email": "Author email", +} def env_flag(name: str, default: str = "false") -> bool: @@ -21,6 +64,14 @@ def env_flag(name: str, default: str = "false") -> bool: return os.getenv(name, default).lower() == "true" +def _reconfigure_io() -> None: + """Reconfigure stdout/stderr to UTF-8 so emoji and check marks never + crash on runners with legacy encodings (e.g. cp1252 on Windows).""" + for stream in (sys.stdout, sys.stderr): + if hasattr(stream, "reconfigure"): + stream.reconfigure(encoding="utf-8", errors="replace") + + MESSAGE_ENABLED = env_flag("MESSAGE") BRANCH_ENABLED = env_flag("BRANCH") AUTHOR_NAME_ENABLED = env_flag("AUTHOR_NAME") @@ -31,6 +82,33 @@ def env_flag(name: str, default: str = "false") -> bool: PR_TITLE_ENABLED = env_flag("PR_TITLE") +@dataclass +class ScopeResult: + """Result of running commit-check against one scope (PR title, one commit, + branch, author, ...). + + ``checks`` holds the parsed JSON check outcomes (only set when the CLI + produced valid JSON); ``raw_text`` holds the raw CLI output when parsing + failed (a defensive fallback so unexpected output is never swallowed). + """ + + label: str + checks: list[dict[str, str]] = field(default_factory=list) + raw_text: str = "" + + @property + def status(self) -> str: + """Overall status: ``pass`` when every check passed.""" + if self.raw_text and not self.checks: + return "fail" + return "fail" if any(c["status"] == "fail" for c in self.checks) else "pass" + + @property + def failures(self) -> list[dict[str, str]]: + """The checks that failed in this scope.""" + return [c for c in self.checks if c["status"] == "fail"] + + def log_env_vars(): """Logs the environment variables for debugging purposes. @@ -143,14 +221,15 @@ def get_pr_commit_messages() -> list[str]: return [] -def run_check_command( - args: list[str], - result_file: TextIO, - input_text: str | None = None, - output_prefix: str | None = None, -) -> int: - """Run commit-check and write both stdout and stderr to the result file.""" - command = ["commit-check"] + args +def run_check_json( + args: list[str], input_text: str | None = None +) -> tuple[int, dict[str, Any] | None, str]: + """Run ``commit-check --format json`` and return (exit code, parsed JSON, raw output). + + The parsed JSON is ``None`` when the CLI did not produce valid JSON; the + raw output is kept so callers can fall back to showing it as text. + """ + command = ["commit-check", "--format", "json"] + args result = subprocess.run( command, input=input_text, @@ -160,59 +239,42 @@ def run_check_command( encoding="utf-8", check=False, ) - if result.stdout: - if output_prefix: - result_file.write(output_prefix) - result_file.write(result.stdout.rstrip("\n")) - result_file.write("\n") - return result.returncode - - -def run_pr_message_checks( - pr_messages: list[str], - result_file: TextIO, - initial_emitted: bool = False, -) -> int: - """Checks each PR commit message individually via commit-check --message. - - Parameters - ---------- - initial_emitted : bool - Whether another check (e.g. PR title) has already produced banner output, - so the first failing commit should use --no-banner. - - Returns 1 if any message fails, 0 if all pass. - """ - has_failure = False - emitted_failure_output = initial_emitted - total = len(pr_messages) - for index, msg in enumerate(pr_messages, start=1): - command_args = ["--message"] - if emitted_failure_output: - command_args.append("--no-banner") + raw = result.stdout or "" + try: + return result.returncode, json.loads(raw), raw + except json.JSONDecodeError: + return result.returncode, None, raw - if emitted_failure_output: - output_prefix = f"\n--- Commit {index}/{total}:\n" - else: - output_prefix = None - return_code = run_check_command( - command_args, - result_file, - input_text=msg, - output_prefix=output_prefix, +def check_scope( + label: str, args: list[str], input_text: str | None = None +) -> ScopeResult: + """Run commit-check for one scope and wrap the outcome in a ScopeResult.""" + _rc, data, raw = run_check_json(args, input_text=input_text) + if isinstance(data, dict): + return ScopeResult(label=label, checks=data.get("checks", [])) + return ScopeResult(label=label, raw_text=raw) + + +def run_pr_message_checks(pr_messages: list[str]) -> list[ScopeResult]: + """Check each PR commit message individually via commit-check --message.""" + results: list[ScopeResult] = [] + total = len(pr_messages) + for index, msg in enumerate(pr_messages, start=1): + results.append( + check_scope(f"Commit {index}/{total}", ["--message"], input_text=msg) ) - if return_code != 0: - has_failure = True - emitted_failure_output = True - return 1 if has_failure else 0 + return results -def run_other_checks(args: list[str], result_file: TextIO) -> int: - """Runs non-message checks (branch, author) once. Returns 0 if args is empty.""" - if not args: - return 0 - return run_check_command(args, result_file) +def run_other_checks(args: list[str]) -> list[ScopeResult]: + """Run each non-message check (branch, author) once, as its own scope.""" + results: list[ScopeResult] = [] + for flag in args: + label = CHECK_LABELS.get(flag) + if label: + results.append(check_scope(label, [flag])) + return results def build_check_args() -> list[str]: @@ -226,19 +288,8 @@ def build_check_args() -> list[str]: return [flag for flag, enabled in flags if enabled] -def get_result_path() -> str: - """Return a safe path for the result file using a temp directory. - - In GitHub Actions this uses ``RUNNER_TEMP`` which is cleaned up - automatically after the job. Falls back to ``tempfile.gettempdir()`` - for local testing. - """ - base = os.environ.get("RUNNER_TEMP") or tempfile.gettempdir() - return os.path.join(base, "commit-check-result.txt") - - -def run_commit_check() -> int: - """Runs all enabled checks and returns the overall exit code. +def run_commit_check() -> tuple[int, list[ScopeResult]]: + """Runs all enabled checks and returns the overall exit code and results. Checks are evaluated in order: 1. PR title (when ``pr-title: true`` and in a PR event) @@ -248,82 +299,428 @@ def run_commit_check() -> int: Outside of a PR event all enabled checks are handed to the CLI at once. """ args = build_check_args() - exit_code = 0 - emitted_failure_output = False - - with open(get_result_path(), "w", encoding="utf-8") as result_file: - # ---- 1. PR title check ------------------------------------------------ - # Always label the PR title section and suppress its banner so the - # output flows consistently with the commit-message section labels: - # - # --- PR Title: - # - # --- Commit 1/1: - # - if PR_TITLE_ENABLED and is_pr_event(): - pr_title = get_pr_title() - if pr_title: - rc = run_check_command( - ["--message", "--no-banner"], - result_file, - input_text=pr_title, - output_prefix=f"--- PR Title:\n", - ) - if rc != 0: - exit_code = max(exit_code, rc) - emitted_failure_output = True - - # ---- 2. Commit message checks ----------------------------------------- - if MESSAGE_ENABLED: - pr_messages = get_pr_commit_messages() - if pr_messages: - # In PR context: check each commit individually to avoid - # only validating the synthetic merge commit at HEAD. - rc = run_pr_message_checks( - pr_messages, result_file, initial_emitted=emitted_failure_output - ) - if rc != 0: - exit_code = max(exit_code, rc) - args = [a for a in args if a != "--message"] + results: list[ScopeResult] = [] + + # ---- 1. PR title check ------------------------------------------------ + if PR_TITLE_ENABLED and is_pr_event(): + pr_title = get_pr_title() + if pr_title: + results.append(check_scope("PR title", ["--message"], input_text=pr_title)) + + # ---- 2. Commit message checks ----------------------------------------- + if MESSAGE_ENABLED: + pr_messages = get_pr_commit_messages() + if pr_messages: + # In PR context: check each commit individually to avoid + # only validating the synthetic merge commit at HEAD. + results.extend(run_pr_message_checks(pr_messages)) + args = [a for a in args if a != "--message"] + + # ---- 3. Remaining checks (branch, author, etc.) ----------------------- + # Outside a PR, check the HEAD commit message directly. + if "--message" in args: + results.append(check_scope("Commit message", ["--message"])) + args = [a for a in args if a != "--message"] + results.extend(run_other_checks(args)) + + exit_code = 1 if any(scope.status == "fail" for scope in results) else 0 + return exit_code, results + + +# --------------------------------------------------------------------------- +# Rendering +# --------------------------------------------------------------------------- + + +def _rule_label(check: dict[str, str]) -> str: + """Human-readable label for a check: ``CC001 message`` (kebab-case).""" + rule_id = check.get("rule_id", "") + name = check.get("check", "").replace("_", "-") + return f"{rule_id} {name}" if rule_id else name + + +def _rule_markdown_link(check: dict[str, str]) -> str: + """Markdown link for a check: ``[CC001 message](docs_url)``.""" + label = _rule_label(check) + docs_url = check.get("docs_url", "") + return f"[{label}]({docs_url})" if docs_url else label + + +def _scope_group(label: str) -> str: + """Group name for a scope label, used to fold the step log output.""" + if label == "PR title" or label.startswith("Commit"): + return "Commit message" + if label.startswith("Author"): + return "Author" + return label + + +def _grouped(results: list[ScopeResult]) -> list[tuple[str, list[ScopeResult]]]: + """Split results into ordered groups for step log folding.""" + groups: list[tuple[str, list[ScopeResult]]] = [] + for scope in results: + group_name = _scope_group(scope.label) + if groups and groups[-1][0] == group_name: + groups[-1][1].append(scope) + else: + groups.append((group_name, [scope])) + return groups - # ---- 3. Remaining checks (branch, author, etc.) ----------------------- - if args: - rc = run_other_checks(args, result_file) - if rc != 0: - exit_code = max(exit_code, rc) - return 1 if exit_code else 0 +def _render_scopes(scopes: list[ScopeResult], include_docs: bool) -> list[str]: + """Render the indented listing for one group of scopes, without its header. + Shared by both output surfaces so they cannot drift: the step log and the + Markdown details block are the same tree, and the only difference is the + docs link, which the Markdown report already carries on the rule ID in the + table above it. + + A failing scope shows its value in full rather than truncated. It is the one + value the reader has to act on, and the table's 60-character cap can cut off + the part that explains the failure. + """ + lines: list[str] = [] + for scope in scopes: + if scope.status == "pass": + value = _scope_value(scope) + lines.append(f" ✔ {scope.label}{f' ({value})' if value else ''}") + continue + if scope.raw_text and not scope.checks: + # Defensive fallback: commit-check produced unexpected output. + lines.append(f" ✖ {scope.label}") + lines.extend(f" {ln}" for ln in scope.raw_text.strip().splitlines()) + continue + failures = scope.failures + count = f" ({len(failures)} failure{'s' if len(failures) != 1 else ''})" + lines.append(f" ✖ {scope.label}{count}") + for check in failures: + lines.append(f" {_rule_label(check)}") + if check.get("value"): + lines.append(f" value: {check['value']}") + for line in check.get("error", "").splitlines(): + lines.append(f" {line}") + if check.get("suggest"): + lines.append(f" Suggest: {check['suggest']}") + if include_docs and check.get("docs_url"): + lines.append(f" Docs: {check['docs_url']}") + return lines + + +def _render_tree(results: list[ScopeResult], include_docs: bool) -> list[str]: + """Render the full grouped listing: a header line per group, then its scopes.""" + lines: list[str] = [] + for group_name, scopes in _grouped(results): + lines.append(group_name) + lines.extend(_render_scopes(scopes, include_docs)) + return lines + + +def _annotation_escape(text: str) -> str: + """Escape text for a workflow command payload. + + A newline would end the command and leave the rest of the message as a + stray log line, and a bare ``%`` can be read as the start of an escape. + """ + return text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") -def read_result_file() -> str | None: - """Reads the result.txt file and removes ANSI color codes.""" - if os.path.getsize(get_result_path()) > 0: - with open(get_result_path(), "r", encoding="utf-8") as result_file: - result_text = re.sub( - r"\x1B\[[0-9;]*[a-zA-Z]", "", result_file.read() - ) # Remove ANSI colors - return result_text.rstrip() - return None +def render_step_log(results: list[ScopeResult]) -> None: + """Print results to the step log, then emit one annotation per failure. -def build_result_body(result_text: str | None) -> str: - """Create the human-readable result body used in summaries and PR comments.""" - if result_text is None: - return SUCCESS_TITLE - return f"{FAILURE_TITLE}\n```\n{result_text}\n```" + The two are separated deliberately. An ``::error`` command renders as a + line of its own wherever it is printed, so emitting one inside the indented + listing broke the tree apart, and its ``title=`` \u2014 which is what carries the + rule ID \u2014 is only shown in the annotations UI, never inline. Printing the + detail once in the listing and the annotations after all the groups keeps + the log readable and still surfaces failures in the run summary and on the + Files changed tab. + """ + # The tree is grouped, so it is printed group by group rather than in one + # block: ::group:: and ::endgroup:: have to bracket each section's lines. + for group_name, scopes in _grouped(results): + print(f"::group::{group_name}") + for line in _render_scopes(scopes, include_docs=True): + print(line) + print("::endgroup::") + + annotations: list[tuple[str, str]] = [] + for scope in results: + if scope.status == "pass": + continue + if scope.raw_text and not scope.checks: + annotations.append( + (f"commit-check: {scope.label}", "output could not be parsed") + ) + continue + for check in scope.failures: + error = check.get("error", "") + first_line = error.splitlines()[0] if error else "check failed" + annotations.append((_rule_label(check), f"{scope.label}: {first_line}")) + for title, message in annotations: + print( + f"::error title={_annotation_escape(title)}" + f"::{_annotation_escape(message)}" + ) -def add_job_summary() -> int: + if not annotations: + print("\u2714 commit-check: all checks passed") + + +def _check_counts(results: list[ScopeResult]) -> tuple[int, int]: + """Return ``(failed, total)`` where one check is one thing that was checked. + + A "check" here is a scope \u2014 one commit message, the branch, the author name + \u2014 not one rule evaluation. Counting rule evaluations produced a number that + grew with the size of the pull request rather than with the strictness of + the policy: sixteen commit messages against six enabled rules reported + "1 of 100 checks failed", where 96 of the 100 were the same six rules run + again per commit. The large denominator also made a real failure look + negligible \u2014 one bad commit out of fifteen reads very differently from + 1 of 100. + + This number matches what the reader can count: the rows in the table plus + the \u2714/\u2716 lines in the details block. Which rules failed is not lost, it is + just reported where it belongs \u2014 in the table and the details. + """ + failed = sum(1 for scope in results if scope.status == "fail") + return failed, len(results) + + +def _failure_count(results: list[ScopeResult]) -> int: + """Number of scopes that failed.""" + return _check_counts(results)[0] + + +def _markdown_table(results: list[ScopeResult]) -> str: + """Render the failure table shared by summary and PR comment. + + Only failed scopes appear, so a per-row result column would read ``\u274c`` on + every row and carry no information; the pass/fail picture for everything + else lives in the details block. + """ + rows = [ + "| Scope | Checked value | Failed checks |", + "|---|---|---|", + ] + for scope in results: + if scope.status == "pass": + continue + value = _scope_value(scope) + value_display = f"`{value}`" if value else "\u2014" + if scope.raw_text and not scope.checks: + links = "_output could not be parsed \u2014 see details_" + else: + links = " \u00b7 ".join( + _rule_markdown_link(check) for check in scope.failures + ) + rows.append(f"| {scope.label} | {value_display} | {links} |") + return "\n".join(rows) + + +def _markdown_details(results: list[ScopeResult]) -> str: + """Render the collapsible details block listing every scope. + + Mirrors the step log layout (group name, ✔/✖ scope lines with the checked + value) and adds the failure reason and suggestion under each failing rule, + so one expand answers both "what was checked" and "what failed and why". + The same block is used whether or not anything failed — on a clean run the + failure branches simply never fire. + """ + _failed, total = _check_counts(results) + unit = "check" if total == 1 else "checks" + label = f"Show all {total} {unit}" if total else "Show details" + lines = ["
", f"{label}", "", "```text"] + lines.extend(_render_tree(results, include_docs=False)) + lines.extend(["```", "", "
"]) + return "\n".join(lines) + + +def _scope_value(scope: ScopeResult, max_len: int = 60) -> str: + """First non-empty check value for a scope, trimmed to a single line. + + The value is the concrete thing that was checked (PR title, commit + subject, branch name, author name/email) and reads naturally next to + the scope label in the success details. The 60-character cap keeps the + full line (prefix + value + parentheses) short enough to avoid wrapping + in the fenced details block. + """ + for check in scope.checks: + value = check.get("value", "") + if value: + first_line = value.splitlines()[0].strip() + if len(first_line) > max_len: + return first_line[: max_len - 3] + "..." + return first_line + return "" + + +# --------------------------------------------------------------------------- +# Output specification +# +# The Markdown report shared by the job summary and the PR comment renders +# as follows (values are filled from ScopeResult data): +# +# Every body opens with COMMENT_MARKER, which is invisible when rendered and is +# how the action recognises its own PR comment on the next run. +# +# Success: +# +# +# ## Commit Check +# +# ✅ **All 5 checks passed** +# +#
+# Show all 5 checks +# +# ```text +# Commit message +# ✔ PR title (feat: add login page) +# ✔ Commit 1/11 (feat: add user auth) +# Branch +# ✔ Branch (feature/add-login) +# Author +# ✔ Author name (Jane Doe) +# ✔ Author email (jane@example.com) +# ``` +# +#
+# +# _commit-check 2.13.1 · [Rules reference](https://commit-check.com/rules/)_ +# +# Failure: +# +# +# ## Commit Check +# +# ❌ **1 of 5 checks failed** +# +# | Scope | Checked value | Failed checks | +# |---|---|---| +# | Commit 2/11 | `bad msg` | [CC001 message](https://commit-check.com/rules/#cc001) | +# +#
+# Show all 5 checks +# +# ```text +# Commit message +# ✔ PR title (feat: add login page) +# ✖ Commit 2/11 (1 failure) +# CC001 message +# value: bad msg +# The commit message should follow Conventional Commits. +# Suggest: Use (): +# Branch +# ✔ Branch (feature/add-login) +# ``` +# +#
+# +# _commit-check 2.13.1 · [Rules reference](https://commit-check.com/rules/)_ +# +# Notes: +# - One check is one thing that was checked — a commit message, the branch, the +# author — not one rule evaluation. The total therefore matches the number of +# ✔/✖ lines the reader can count in the details block, and does not grow with +# the number of commits in the pull request or rules in the config. +# - The table lists only failed scopes; there is no per-row result column +# because it would read ❌ on every row. Passing scopes live in the details. +# - Values are capped at 60 characters with a literal "..." suffix, except on a +# failing scope, where the details block prints the value in full — it is the +# one value the reader has to act on and the cap can hide the reason. +# - The step log renders the same tree (_render_scopes); it adds the docs URL, +# which the Markdown report already carries on the rule ID in the table. +# --------------------------------------------------------------------------- + + +def _commit_check_version() -> str: + """Version of the commit-check CLI that produced these results.""" + try: + from importlib.metadata import version + + return version("commit-check") + except Exception: + return "" + + +def _report_footer() -> str: + """Attribution line: which version ran, and where the rules are documented. + + The version is the first thing worth knowing when a result looks wrong, and + it is otherwise buried in the step log. + """ + rules = f"[Rules reference]({RULES_URL})" + installed = _commit_check_version() + return f"_commit-check {installed} · {rules}_" if installed else f"_{rules}_" + + +def render_report(results: list[ScopeResult]) -> str: + """Render the Markdown report shared by the job summary and PR comment. + + Opens with the hidden marker and the title, then a one-line verdict — + ``✅ **All N checks passed**`` or ``❌ **N of M checks failed**`` — then the + failure table (failures only) and the collapsible per-scope details. + """ + failed, total = _check_counts(results) + unit = "check" if total == 1 else "checks" + + lines = [COMMENT_MARKER, REPORT_TITLE, ""] + if failed == 0: + lines.append(f"✅ **All {total} {unit} passed**") + lines.append("") + else: + lines.append(f"❌ **{failed} of {total} {unit} failed**") + lines.extend(["", _markdown_table(results), ""]) + lines.extend([_markdown_details(results), "", _report_footer()]) + return "\n".join(lines) + + +def render_job_summary(results: list[ScopeResult]) -> str: + """Create the Markdown body for the GitHub job summary.""" + return render_report(results) + + +def render_pr_comment(results: list[ScopeResult]) -> str: + """Create the Markdown body for the PR comment (same report as summary).""" + return render_report(results) + + +# --------------------------------------------------------------------------- +# Output surfaces +# --------------------------------------------------------------------------- + + +def add_job_summary(results: list[ScopeResult]) -> int: """Adds the commit check result to the GitHub job summary.""" - if not JOB_SUMMARY_ENABLED: + if not JOB_SUMMARY_ENABLED or not GITHUB_STEP_SUMMARY: return 0 - result_text = read_result_file() - with open(GITHUB_STEP_SUMMARY, "a", encoding="utf-8") as summary_file: - summary_file.write(build_result_body(result_text)) + summary_file.write(render_job_summary(results)) + + return 0 if all(scope.status == "pass" for scope in results) else 1 - return 0 if result_text is None else 1 + +def set_result_output(results: list[ScopeResult]) -> None: + """Expose the structured results as the ``result`` action output. + + Uses the heredoc form of ``GITHUB_OUTPUT`` so multi-line JSON survives. + """ + output_path = os.getenv("GITHUB_OUTPUT") + if not output_path: + return + payload = { + "status": "pass" if all(s.status == "pass" for s in results) else "fail", + "scopes": [ + {"label": scope.label, "status": scope.status, "checks": scope.checks} + for scope in results + ], + } + with open(output_path, "a", encoding="utf-8") as f: + f.write("result< bool: @@ -383,7 +780,33 @@ def get_pr_number() -> int: ) -def add_pr_comments() -> int: +def _is_bot(comment: Any) -> bool: + """Whether a comment was posted by a bot account rather than a person.""" + try: + return comment.user.type == "Bot" + except Exception: + return False + + +def _find_own_comments(comments: list[Any]) -> tuple[Any | None, list[Any]]: + """Pick the comment to update and the ones to delete. + + Returns ``(target, stale)``. Only comments carrying ``COMMENT_MARKER`` are + ever deleted — those are unambiguously ours. A comment from an earlier + version has no marker, so it is adopted (edited, which adds the marker) + when there is no marked comment yet, and only if a bot posted it: the + legacy signal is a title prefix, which a person can type by accident, and + editing someone's comment out from under them is not recoverable. + """ + marked = [c for c in comments if COMMENT_MARKER in c.body] + if marked: + return marked[-1], marked[:-1] + + legacy = [c for c in comments if c.body.startswith(LEGACY_TITLES) and _is_bot(c)] + return (legacy[-1], []) if legacy else (None, []) + + +def add_pr_comments(results: list[ScopeResult]) -> int: """Posts the commit check result as a comment on the pull request.""" if not PR_COMMENTS_ENABLED: return 0 @@ -400,7 +823,7 @@ def add_pr_comments() -> int: "for how to enable PR comments on fork PRs." ) print(f"::warning::{msg}") - if JOB_SUMMARY_ENABLED: + if JOB_SUMMARY_ENABLED and GITHUB_STEP_SUMMARY: with open(GITHUB_STEP_SUMMARY, "a", encoding="utf-8") as f: f.write( "\n---\n" @@ -416,43 +839,47 @@ def add_pr_comments() -> int: try: from github import Auth, Github, GithubException # type: ignore + except ImportError as e: + # Imported here, so it has to be caught here. Leaving it inside the + # try below would bind GithubException only on success — and an + # ImportError would then make the `except GithubException` clause + # itself raise NameError, which propagates past the `except Exception` + # underneath it and kills a step that is meant to be non-fatal. + print(f"::warning::Unable to post PR comment: {e}", file=sys.stderr) + return 0 + try: token = os.getenv("GITHUB_TOKEN") repo_name = os.getenv("GITHUB_REPOSITORY") pr_number = get_pr_number() if not token: raise ValueError("GITHUB_TOKEN is not set") + if not repo_name: + raise ValueError("GITHUB_REPOSITORY is not set") g = Github(auth=Auth.Token(token)) repo = g.get_repo(repo_name) pull_request = repo.get_issue(pr_number) - result_text = read_result_file() - pr_comment_body = build_result_body(result_text) + pr_comment_body = render_pr_comment(results) - comments = pull_request.get_comments() - matching_comments = [ - c - for c in comments - if c.body.startswith(SUCCESS_TITLE) or c.body.startswith(FAILURE_TITLE) - ] + target, stale = _find_own_comments(list(pull_request.get_comments())) - if matching_comments: - last_comment = matching_comments[-1] - if last_comment.body == pr_comment_body: + if target is not None: + if target.body == pr_comment_body: print(f"PR comment already up-to-date for PR #{pr_number}.") - return 0 + return 0 if all(scope.status == "pass" for scope in results) else 1 print(f"Updating the last comment on PR #{pr_number}.") - last_comment.edit(pr_comment_body) - for comment in matching_comments[:-1]: + target.edit(pr_comment_body) + for comment in stale: print(f"Deleting an old comment on PR #{pr_number}.") comment.delete() else: print(f"Creating a new comment on PR #{pr_number}.") pull_request.create_comment(body=pr_comment_body) - return 0 if result_text is None else 1 + return 0 if all(scope.status == "pass" for scope in results) else 1 except GithubException as e: if e.status == 403: print( @@ -469,34 +896,31 @@ def add_pr_comments() -> int: return 0 -def log_error_and_exit( - failure_title: str, result_text: str | None, ret_code: int -) -> None: - """ - Logs an error message to GitHub Actions and exits with the specified return code. - - Args: - failure_title (str): The title of the failure message. - result_text (str): The detailed result text to include in the error message. - ret_code (int): The return code to exit with. - """ - if result_text: - error_message = f"{failure_title}\n```\n{result_text}\n```" - print(f"::error::{error_message}") +def log_error_and_exit(ret_code: int, results: list[ScopeResult]) -> None: + """Logs a summary error to GitHub Actions and exits with the given code.""" + if ret_code != 0 and results: + failures = _failure_count(results) + unit = "failure" if failures == 1 else "failures" + print(f"::error::commit-check found {failures} {unit}.") sys.exit(ret_code) def main(): - """Main function to run commit-check, add job summary and post PR comments.""" + """Main function to run commit-check and render all output surfaces.""" + _reconfigure_io() log_env_vars() - ret_code = max(run_commit_check(), add_job_summary(), add_pr_comments()) + ret_code, results = run_commit_check() + + render_step_log(results) + set_result_output(results) + + ret_code = max(ret_code, add_job_summary(results), add_pr_comments(results)) if DRY_RUN_ENABLED: ret_code = 0 - result_text = read_result_file() - log_error_and_exit(FAILURE_TITLE, result_text, ret_code) + log_error_and_exit(ret_code, results) if __name__ == "__main__": diff --git a/main_test.py b/main_test.py index 1f5ef70..d1e6bcd 100644 --- a/main_test.py +++ b/main_test.py @@ -3,15 +3,77 @@ import io import json import os +import sys +import tempfile import unittest from unittest.mock import MagicMock, patch -# GITHUB_STEP_SUMMARY is accessed via os.environ[] (not getenv) at import time, -# so we must set it before importing main. os.environ.setdefault("GITHUB_STEP_SUMMARY", "/tmp/step_summary.txt") import main # noqa: E402 +#: The report footer names the installed commit-check version, which differs +#: between a contributor's machine and CI. Golden tests pin it so they assert on +#: the report layout rather than on whatever version happens to be installed. +PINNED_VERSION = "2.13.1" +FOOTER = ( + f"_commit-check {PINNED_VERSION} · " + "[Rules reference](https://commit-check.com/rules/)_" +) +pin_version = patch("main._commit_check_version", new=lambda: PINNED_VERSION) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def make_check( + check: str, + status: str = "pass", + rule_id: str = "CC001", + value: str = "", + error: str = "", + suggest: str = "", + docs_url: str = "", +) -> dict[str, str]: + """Build a single check outcome dict as produced by commit-check JSON.""" + return { + "rule_id": rule_id, + "check": check, + "status": status, + "value": value, + "error": error, + "suggest": suggest, + "docs_url": docs_url, + } + + +def json_output(*checks) -> str: + """Serialize checks to the CLI JSON output shape.""" + status = "fail" if any(c["status"] == "fail" for c in checks) else "pass" + return json.dumps({"status": status, "checks": list(checks)}) + + +def pass_scope(label: str = "Branch", value: str = "") -> main.ScopeResult: + return main.ScopeResult(label=label, checks=[make_check("branch", value=value)]) + + +def fail_scope(label: str = "Commit 1/1") -> main.ScopeResult: + return main.ScopeResult( + label=label, + checks=[ + make_check( + "message", + status="fail", + rule_id="CC001", + value="bad message", + error="The commit message should follow Conventional Commits.", + suggest="Use (): ", + docs_url="https://commit-check.com/rules/#cc001", + ) + ], + ) + class TestEnvFlag(unittest.TestCase): def test_true_value(self): @@ -27,6 +89,40 @@ def test_missing_uses_default(self): self.assertTrue(main.env_flag("FEATURE_FLAG", default="true")) +class TestReconfigureIo(unittest.TestCase): + def test_reconfigures_streams_to_utf8(self): + class FakeStream: + def __init__(self): + self.reconfigured = None + + def reconfigure(self, **kwargs): + self.reconfigured = kwargs + + fake_out = FakeStream() + fake_err = FakeStream() + with ( + patch.object(sys, "stdout", fake_out), + patch.object(sys, "stderr", fake_err), + ): + main._reconfigure_io() + self.assertEqual( + fake_out.reconfigured, {"encoding": "utf-8", "errors": "replace"} + ) + self.assertEqual( + fake_err.reconfigured, {"encoding": "utf-8", "errors": "replace"} + ) + + def test_streams_without_reconfigure_are_ignored(self): + class NoopStream: + pass + + with ( + patch.object(sys, "stdout", NoopStream()), + patch.object(sys, "stderr", NoopStream()), + ): + main._reconfigure_io() # should not raise + + class TestBuildCheckArgs(unittest.TestCase): def test_all_true(self): with ( @@ -73,8 +169,6 @@ def test_non_pr_event_returns_none(self): self.assertIsNone(main.get_pr_title()) def test_pr_event_returns_title(self): - import tempfile - event = { "pull_request": {"title": "feat: add login page"}, } @@ -91,8 +185,6 @@ def test_pr_event_returns_title(self): os.unlink(event_path) def test_pull_request_target_event(self): - import tempfile - event = { "pull_request": {"title": "fix: resolve timeout"}, } @@ -112,17 +204,12 @@ def test_pull_request_target_event(self): os.unlink(event_path) def test_missing_event_path_returns_none(self): - with ( - patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), - patch.dict(os.environ, {}, clear=True), - ): + with patch.dict(os.environ, {}, clear=True): os.environ["GITHUB_EVENT_NAME"] = "pull_request" os.environ.pop("GITHUB_EVENT_PATH", None) self.assertIsNone(main.get_pr_title()) def test_invalid_json_returns_none(self): - import tempfile - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: f.write("not valid json") event_path = f.name @@ -137,157 +224,166 @@ def test_invalid_json_returns_none(self): os.unlink(event_path) -class TestRunCheckCommand(unittest.TestCase): - def test_with_args_calls_subprocess(self): - mock_result = MagicMock(returncode=0, stdout="") +class TestRunCheckJson(unittest.TestCase): + def test_parses_json_output(self): + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("branch"))) with patch("main.subprocess.run", return_value=mock_result) as mock_run: - rc = main.run_check_command(["--branch"], io.StringIO()) + rc, data, raw = main.run_check_json(["--branch"]) self.assertEqual(rc, 0) - self.assertEqual(mock_run.call_args[0][0], ["commit-check", "--branch"]) + self.assertEqual(data["status"], "pass") + self.assertEqual(len(data["checks"]), 1) + self.assertIn("checks", raw) + + def test_command_includes_format_json(self): + mock_result = MagicMock(returncode=0, stdout="{}") + with patch("main.subprocess.run", return_value=mock_result) as mock_run: + main.run_check_json(["--branch"]) + self.assertEqual( + mock_run.call_args[0][0], + ["commit-check", "--format", "json", "--branch"], + ) - def test_with_input_uses_text_mode(self): - mock_result = MagicMock(returncode=0, stdout="") + def test_input_text_is_passed_through(self): + mock_result = MagicMock(returncode=0, stdout="{}") with patch("main.subprocess.run", return_value=mock_result) as mock_run: - main.run_check_command(["--message"], io.StringIO(), input_text="fix: demo") + main.run_check_json(["--message"], input_text="fix: demo") self.assertEqual(mock_run.call_args[1]["input"], "fix: demo") self.assertTrue(mock_run.call_args[1]["text"]) - def test_success_returns_zero(self): - mock_result = MagicMock(returncode=0, stdout="") + def test_invalid_json_returns_none_with_raw_output(self): + mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n") with patch("main.subprocess.run", return_value=mock_result): - rc = main.run_check_command(["--branch"], io.StringIO()) - self.assertEqual(rc, 0) + rc, data, raw = main.run_check_json(["--branch"]) + self.assertEqual(rc, 1) + self.assertIsNone(data) + self.assertEqual(raw, "Commit rejected.\n") + + +class TestScopeResult(unittest.TestCase): + def test_status_pass_when_all_checks_pass(self): + scope = main.ScopeResult( + label="Branch", checks=[make_check("branch"), make_check("merge_base")] + ) + self.assertEqual(scope.status, "pass") + self.assertEqual(scope.failures, []) + + def test_status_fail_when_any_check_fails(self): + scope = main.ScopeResult( + label="Branch", + checks=[ + make_check("branch", status="fail"), + make_check("merge_base"), + ], + ) + self.assertEqual(scope.status, "fail") + self.assertEqual(len(scope.failures), 1) + + def test_raw_text_fallback_is_failure(self): + scope = main.ScopeResult(label="Branch", raw_text="unexpected output") + self.assertEqual(scope.status, "fail") + + +class TestCheckScope(unittest.TestCase): + def test_parses_checks_into_scope(self): + mock_result = MagicMock( + returncode=1, stdout=json_output(make_check("branch", status="fail")) + ) + with patch("main.subprocess.run", return_value=mock_result): + scope = main.check_scope("Branch", ["--branch"]) + self.assertEqual(scope.label, "Branch") + self.assertEqual(scope.status, "fail") + self.assertEqual(scope.failures[0]["rule_id"], "CC001") + + def test_invalid_json_falls_back_to_raw_text(self): + mock_result = MagicMock(returncode=1, stdout="unexpected output") + with patch("main.subprocess.run", return_value=mock_result): + scope = main.check_scope("Branch", ["--branch"]) + self.assertEqual(scope.label, "Branch") + self.assertEqual(scope.raw_text, "unexpected output") + self.assertEqual(scope.status, "fail") class TestRunPrMessageChecks(unittest.TestCase): def test_single_message_pass(self): - mock_result = MagicMock(returncode=0, stdout="") - result_file = io.StringIO() + mock_result = MagicMock(returncode=0, stdout=json_output(make_check("message"))) with patch("main.subprocess.run", return_value=mock_result) as mock_run: - rc = main.run_pr_message_checks(["fix: something"], result_file) - self.assertEqual(rc, 0) - self.assertEqual(mock_run.call_args[0][0], ["commit-check", "--message"]) + scopes = main.run_pr_message_checks(["fix: something"]) + self.assertEqual(len(scopes), 1) + self.assertEqual(scopes[0].status, "pass") + self.assertEqual(scopes[0].label, "Commit 1/1") + self.assertEqual( + mock_run.call_args[0][0], + ["commit-check", "--format", "json", "--message"], + ) self.assertEqual(mock_run.call_args[1]["input"], "fix: something") - self.assertEqual(result_file.getvalue(), "") - def test_failed_message_writes_output(self): - mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n") - result_file = io.StringIO() + def test_failed_message_marks_scope_failed(self): + mock_result = MagicMock( + returncode=1, + stdout=json_output(make_check("message", status="fail")), + ) with patch("main.subprocess.run", return_value=mock_result): - rc = main.run_pr_message_checks(["fix: something"], result_file) - self.assertEqual(rc, 1) - self.assertIn("Commit rejected.", result_file.getvalue()) + scopes = main.run_pr_message_checks(["bad commit"]) + self.assertEqual(scopes[0].status, "fail") + self.assertEqual(len(scopes[0].failures), 1) - def test_multiple_messages_partial_failure(self): + def test_labels_commits_in_order(self): results = [ - MagicMock(returncode=0, stdout=""), - MagicMock(returncode=1, stdout="Commit rejected.\n"), - MagicMock(returncode=0, stdout=""), + MagicMock(returncode=0, stdout=json_output(make_check("message"))), + MagicMock( + returncode=1, + stdout=json_output(make_check("message", status="fail")), + ), + MagicMock(returncode=0, stdout=json_output(make_check("message"))), ] with patch("main.subprocess.run", side_effect=results): - rc = main.run_pr_message_checks(["ok", "bad", "ok"], io.StringIO()) - self.assertEqual(rc, 1) + scopes = main.run_pr_message_checks(["ok", "bad", "ok"]) + self.assertEqual( + [s.label for s in scopes], ["Commit 1/3", "Commit 2/3", "Commit 3/3"] + ) + self.assertEqual(scopes[1].status, "fail") def test_empty_list(self): with patch("main.subprocess.run") as mock_run: - rc = main.run_pr_message_checks([], io.StringIO()) - self.assertEqual(rc, 0) + scopes = main.run_pr_message_checks([]) + self.assertEqual(scopes, []) mock_run.assert_not_called() - def test_first_failure_keeps_banner_and_later_failures_use_no_banner(self): - results = [ - MagicMock(returncode=0, stdout=""), - MagicMock(returncode=1, stdout="Commit rejected.\n"), - MagicMock(returncode=1, stdout="Type subject_imperative check failed\n"), - ] - with patch("main.subprocess.run", side_effect=results) as mock_run: - main.run_pr_message_checks( - ["ok first", "bad second", "bad third"], io.StringIO() - ) - self.assertEqual( - mock_run.call_args_list[0][0][0], ["commit-check", "--message"] - ) - self.assertEqual( - mock_run.call_args_list[1][0][0], - ["commit-check", "--message"], - ) - self.assertEqual( - mock_run.call_args_list[2][0][0], - ["commit-check", "--message", "--no-banner"], - ) +class TestRunOtherChecks(unittest.TestCase): + def test_empty_args_returns_no_scopes(self): + with patch("main.subprocess.run") as mock_run: + scopes = main.run_other_checks([]) + self.assertEqual(scopes, []) + mock_run.assert_not_called() - def test_initial_emitted_suppresses_banner_for_first_failure(self): + def test_runs_each_flag_as_its_own_scope(self): results = [ - MagicMock(returncode=1, stdout="Commit rejected.\n"), + MagicMock( + returncode=1, stdout=json_output(make_check("branch", status="fail")) + ), + MagicMock(returncode=0, stdout=json_output(make_check("author_name"))), ] with patch("main.subprocess.run", side_effect=results) as mock_run: - main.run_pr_message_checks( - ["bad commit"], io.StringIO(), initial_emitted=True - ) + scopes = main.run_other_checks(["--branch", "--author-name"]) + self.assertEqual([s.label for s in scopes], ["Branch", "Author name"]) + self.assertEqual(scopes[0].status, "fail") + self.assertEqual(scopes[1].status, "pass") self.assertEqual( - mock_run.call_args[0][0], - ["commit-check", "--message", "--no-banner"], + mock_run.call_args_list[0][0][0], + ["commit-check", "--format", "json", "--branch"], ) - - def test_initial_not_emitted_allows_banner(self): - results = [ - MagicMock(returncode=1, stdout="Commit rejected.\n"), - ] - with patch("main.subprocess.run", side_effect=results) as mock_run: - main.run_pr_message_checks( - ["bad commit"], io.StringIO(), initial_emitted=False - ) self.assertEqual( - mock_run.call_args[0][0], - ["commit-check", "--message"], - ) - - def test_later_failure_prefix_uses_short_separator_without_extra_blank_lines(self): - results = [ - MagicMock(returncode=0, stdout=""), - MagicMock(returncode=1, stdout="Commit rejected.\n"), - MagicMock( - returncode=1, - stdout=( - "Type subject_imperative check failed ==> bad third\n" - "Commit message should use imperative mood\n" - "Suggest: Use imperative mood\n\n" - ), - ), - ] - result_file = io.StringIO() - with patch("main.subprocess.run", side_effect=results): - main.run_pr_message_checks( - ["ok first", "bad second", "bad third"], result_file - ) - - output = result_file.getvalue() - self.assertIn("Commit rejected.\n", output) - self.assertIn( - "\n--- Commit 3/3:\nType subject_imperative check failed ==> bad third\n", - output, - ) - self.assertNotIn( - "------------------------------------------------------------------------", - output, + mock_run.call_args_list[1][0][0], + ["commit-check", "--format", "json", "--author-name"], ) - self.assertNotIn("\n\n\n", output) - -class TestRunOtherChecks(unittest.TestCase): - def test_empty_args_returns_zero(self): + def test_unknown_flag_is_skipped(self): with patch("main.subprocess.run") as mock_run: - rc = main.run_other_checks([], io.StringIO()) - self.assertEqual(rc, 0) + scopes = main.run_other_checks(["--unknown"]) + self.assertEqual(scopes, []) mock_run.assert_not_called() - def test_with_args_returns_returncode(self): - mock_result = MagicMock(returncode=1, stdout="branch check failed\n") - with patch("main.subprocess.run", return_value=mock_result): - rc = main.run_other_checks(["--branch", "--author-name"], io.StringIO()) - self.assertEqual(rc, 1) - class TestGetPrCommitMessages(unittest.TestCase): def test_non_pr_event_returns_empty(self): @@ -378,46 +474,34 @@ def test_get_messages_from_head_ref(self): class TestRunCommitCheck(unittest.TestCase): - def setUp(self): - self._orig_dir = os.getcwd() - import tempfile - - self._tmpdir = tempfile.mkdtemp() - os.environ["RUNNER_TEMP"] = self._tmpdir - os.chdir(self._tmpdir) - - def tearDown(self): - os.chdir(self._orig_dir) - os.environ.pop("RUNNER_TEMP", None) - - def test_pr_path_calls_pr_message_checks(self): + def test_pr_path_checks_each_commit(self): with ( patch("main.MESSAGE_ENABLED", True), patch("main.BRANCH_ENABLED", False), patch("main.AUTHOR_NAME_ENABLED", False), patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.get_pr_commit_messages", return_value=["fix: something"]), - patch("main.run_pr_message_checks", return_value=0) as mock_pr, - patch("main.run_other_checks", return_value=0), - patch("main.run_check_command") as mock_command, + patch("main.run_pr_message_checks", return_value=[pass_scope()]) as mock_pr, + patch("main.run_other_checks", return_value=[]), ): - rc = main.run_commit_check() + rc, results = main.run_commit_check() self.assertEqual(rc, 0) - mock_pr.assert_called_once() - mock_command.assert_not_called() + mock_pr.assert_called_once_with(["fix: something"]) + self.assertEqual(len(results), 1) - def test_pr_path_returns_nonzero_when_any_check_fails(self): + def test_pr_path_fails_when_any_scope_fails(self): with ( patch("main.MESSAGE_ENABLED", True), patch("main.BRANCH_ENABLED", True), patch("main.AUTHOR_NAME_ENABLED", False), patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.get_pr_commit_messages", return_value=["bad msg"]), - patch("main.run_pr_message_checks", return_value=1), - patch("main.run_other_checks", return_value=1), + patch("main.run_pr_message_checks", return_value=[fail_scope()]), + patch("main.run_other_checks", return_value=[pass_scope()]), ): - rc = main.run_commit_check() + rc, results = main.run_commit_check() self.assertEqual(rc, 1) + self.assertEqual(len(results), 2) def test_pr_title_check_runs_when_enabled(self): with ( @@ -428,16 +512,16 @@ def test_pr_title_check_runs_when_enabled(self): patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.is_pr_event", return_value=True), patch("main.get_pr_title", return_value="feat: a feature"), - patch("main.run_check_command", return_value=0) as mock_cmd, - patch("main.run_other_checks", return_value=0), + patch( + "main.check_scope", return_value=pass_scope("PR title") + ) as mock_scope, + patch("main.run_other_checks", return_value=[]), ): - rc = main.run_commit_check() + rc, results = main.run_commit_check() self.assertEqual(rc, 0) - self.assertEqual( - mock_cmd.call_args[0][0], - ["--message", "--no-banner"], + mock_scope.assert_called_once_with( + "PR title", ["--message"], input_text="feat: a feature" ) - self.assertEqual(mock_cmd.call_args[1]["input_text"], "feat: a feature") def test_pr_title_failure_propagates(self): with ( @@ -448,10 +532,10 @@ def test_pr_title_failure_propagates(self): patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.is_pr_event", return_value=True), patch("main.get_pr_title", return_value="bad title"), - patch("main.run_check_command", return_value=1), - patch("main.run_other_checks", return_value=0), + patch("main.check_scope", return_value=fail_scope("PR title")), + patch("main.run_other_checks", return_value=[]), ): - rc = main.run_commit_check() + rc, results = main.run_commit_check() self.assertEqual(rc, 1) def test_pr_title_skipped_outside_pr_context(self): @@ -463,36 +547,13 @@ def test_pr_title_skipped_outside_pr_context(self): patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.is_pr_event", return_value=False), patch("main.get_pr_title") as mock_title, - patch("main.run_check_command", return_value=0), - patch("main.run_other_checks", return_value=0), + patch("main.run_other_checks", return_value=[]), ): - rc = main.run_commit_check() + rc, results = main.run_commit_check() self.assertEqual(rc, 0) mock_title.assert_not_called() - def test_pr_title_and_message_both_run(self): - with ( - patch("main.PR_TITLE_ENABLED", True), - patch("main.MESSAGE_ENABLED", True), - patch("main.BRANCH_ENABLED", False), - patch("main.AUTHOR_NAME_ENABLED", False), - patch("main.AUTHOR_EMAIL_ENABLED", False), - patch("main.is_pr_event", return_value=True), - patch("main.get_pr_title", return_value="feat: nice pr"), - patch( - "main.get_pr_commit_messages", - return_value=["fix: first", "feat: second"], - ), - patch("main.run_check_command", return_value=0) as mock_cmd, - patch("main.run_pr_message_checks", return_value=0) as mock_pr, - patch("main.run_other_checks", return_value=0), - ): - rc = main.run_commit_check() - self.assertEqual(rc, 0) - mock_cmd.assert_called_once() # PR title check - mock_pr.assert_called_once() # commit message checks - - def test_non_pr_path_uses_direct_command(self): + def test_non_pr_message_check_uses_commit_message_scope(self): with ( patch("main.MESSAGE_ENABLED", True), patch("main.BRANCH_ENABLED", False), @@ -500,44 +561,22 @@ def test_non_pr_path_uses_direct_command(self): patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.get_pr_commit_messages", return_value=[]), patch("main.run_pr_message_checks") as mock_pr, - patch("main.run_check_command", return_value=0) as mock_command, - ): - rc = main.run_commit_check() - self.assertEqual(rc, 0) - mock_pr.assert_not_called() - mock_command.assert_called_once() - - def test_message_disabled_uses_direct_command(self): - with ( - patch("main.MESSAGE_ENABLED", False), - patch("main.BRANCH_ENABLED", True), - patch("main.AUTHOR_NAME_ENABLED", False), - patch("main.AUTHOR_EMAIL_ENABLED", False), - patch("main.run_pr_message_checks") as mock_pr, - patch("main.run_check_command", return_value=0) as mock_command, + patch( + "main.check_scope", return_value=pass_scope("Commit message") + ) as mock_scope, + patch("main.run_other_checks", return_value=[]), ): - rc = main.run_commit_check() + rc, results = main.run_commit_check() self.assertEqual(rc, 0) mock_pr.assert_not_called() - mock_command.assert_called_once() - - def test_result_txt_is_created(self): - with ( - patch("main.MESSAGE_ENABLED", False), - patch("main.BRANCH_ENABLED", False), - patch("main.AUTHOR_NAME_ENABLED", False), - patch("main.AUTHOR_EMAIL_ENABLED", False), - patch("main.run_check_command", return_value=0), - ): - main.run_commit_check() - self.assertTrue(os.path.exists(main.get_result_path())) + mock_scope.assert_called_once_with("Commit message", ["--message"]) - def test_other_args_excludes_message(self): + def test_message_flag_removed_before_other_checks_in_pr(self): captured_args = [] - def fake_other_checks(args, result_file): + def fake_other_checks(args): captured_args.extend(args) - return 0 + return [] with ( patch("main.MESSAGE_ENABLED", True), @@ -545,7 +584,7 @@ def fake_other_checks(args, result_file): patch("main.AUTHOR_NAME_ENABLED", False), patch("main.AUTHOR_EMAIL_ENABLED", False), patch("main.get_pr_commit_messages", return_value=["fix: x"]), - patch("main.run_pr_message_checks", return_value=0), + patch("main.run_pr_message_checks", return_value=[pass_scope()]), patch("main.run_other_checks", side_effect=fake_other_checks), ): main.run_commit_check() @@ -553,112 +592,375 @@ def fake_other_checks(args, result_file): self.assertIn("--branch", captured_args) -class TestReadResultFile(unittest.TestCase): - def setUp(self): - import tempfile - - self._orig_dir = os.getcwd() - self._tmpdir = tempfile.mkdtemp() - os.environ["RUNNER_TEMP"] = self._tmpdir - os.chdir(self._tmpdir) - - def tearDown(self): - os.chdir(self._orig_dir) - os.environ.pop("RUNNER_TEMP", None) +class TestRenderStepLog(unittest.TestCase): + def _run(self, results): + buffer = io.StringIO() + with patch("sys.stdout", buffer): + main.render_step_log(results) + return buffer.getvalue() + + def test_all_pass_prints_success_line(self): + output = self._run([pass_scope("Branch")]) + self.assertIn("✔ commit-check: all checks passed", output) + + def test_failure_prints_group_and_error_annotation(self): + output = self._run([fail_scope("Commit 1/1")]) + self.assertIn("::group::Commit message", output) + self.assertIn("::endgroup::", output) + self.assertIn("✖ Commit 1/1 (1 failure)", output) + self.assertIn(" CC001 message", output) + self.assertIn("value: bad message", output) + self.assertIn("Suggest: Use (): ", output) + self.assertIn("Docs: https://commit-check.com/rules/#cc001", output) + # The annotation names the scope, which the title alone cannot carry. + self.assertIn( + "::error title=CC001 message::Commit 1/1: The commit message should " + "follow Conventional Commits.", + output, + ) - def _write_result(self, content: str): - with open(main.get_result_path(), "w", encoding="utf-8") as file_obj: - file_obj.write(content) + def test_failure_reason_is_printed_once(self): + """The listing and the annotation must not both print the reason. - def test_empty_file_returns_none(self): - self._write_result("") - self.assertIsNone(main.read_result_file()) + They used to: the ::error command carried the first line of the error + and the listing printed the whole error underneath it, so the same + sentence appeared twice in a row in the job log. + """ + output = self._run([fail_scope("Commit 1/1")]) + self.assertEqual( + output.count("The commit message should follow Conventional Commits."), + 2, # once in the listing, once in the annotation after the groups + ) + listing = output.split("::endgroup::")[0] + self.assertEqual( + listing.count("The commit message should follow Conventional Commits."), 1 + ) - def test_file_with_content(self): - self._write_result("some output\n") - self.assertEqual(main.read_result_file(), "some output") + def test_annotations_come_after_every_group(self): + """An ::error inside a group breaks the indented listing apart.""" + output = self._run([fail_scope("Commit 1/1"), pass_scope("Branch")]) + self.assertLess(output.rindex("::endgroup::"), output.index("::error ")) + + def test_annotation_payload_is_escaped(self): + scope = main.ScopeResult( + label="Commit 1/1", + checks=[ + make_check( + "message", + status="fail", + error="first line\nsecond line with 100% certainty", + ) + ], + ) + output = self._run([scope]) + annotation = [ln for ln in output.splitlines() if ln.startswith("::error")][0] + self.assertNotIn("\n", annotation.removeprefix("::error ")) + self.assertIn("first line", annotation) - def test_ansi_codes_are_stripped(self): - self._write_result("\x1b[31mError\x1b[0m: bad commit") - self.assertEqual(main.read_result_file(), "Error: bad commit") + def test_pass_scopes_show_the_checked_value(self): + output = self._run([pass_scope("Branch", value="feature/add-login")]) + self.assertIn("✔ Branch (feature/add-login)", output) + def test_groups_scopes_by_category(self): + results = [ + fail_scope("PR title"), + pass_scope("Commit 1/2"), + fail_scope("Branch"), + ] + output = self._run(results) + # One group for commit-message scopes, one for the branch scope. + self.assertEqual(output.count("::group::"), 2) + self.assertIn("::group::Commit message", output) + self.assertIn("::group::Branch", output) + + def test_raw_text_fallback_is_printed(self): + scope = main.ScopeResult(label="Branch", raw_text="unexpected output") + output = self._run([scope]) + # No "(0 failures)": there is no check list to count, and claiming zero + # next to a ✖ reads as a contradiction. + self.assertIn("✖ Branch", output) + self.assertNotIn("0 failures", output) + self.assertIn("unexpected output", output) + self.assertIn("::error title=commit-check: Branch::", output) + + +class TestRenderJobSummary(unittest.TestCase): + @pin_version + def test_success_golden_output(self): + """Pin the full success report so the spec stays visible and exact.""" + results = [ + pass_scope("PR title", value="feat: add login page"), + pass_scope("Commit 1/2", value="feat: add user auth"), + pass_scope("Commit 2/2", value="fix: resolve timeout"), + pass_scope("Branch", value="feature/add-login"), + ] + body = main.render_report(results) + self.assertEqual( + body, + f"{main.COMMENT_MARKER}\n" + f"{main.REPORT_TITLE}\n" + "\n" + "✅ **All 4 checks passed**\n" + "\n" + "
\n" + "Show all 4 checks\n" + "\n" + "```text\n" + "Commit message\n" + " ✔ PR title (feat: add login page)\n" + " ✔ Commit 1/2 (feat: add user auth)\n" + " ✔ Commit 2/2 (fix: resolve timeout)\n" + "Branch\n" + " ✔ Branch (feature/add-login)\n" + "```\n" + "\n" + "
\n" + "\n" + f"{FOOTER}", + ) -class TestBuildResultBody(unittest.TestCase): - def test_success_body(self): - self.assertEqual(main.build_result_body(None), main.SUCCESS_TITLE) + @pin_version + def test_failure_golden_output(self): + """Pin the full failure report: failed row in the table, all in details.""" + results = [ + pass_scope("PR title", value="feat: add login page"), + fail_scope("Commit 2/2"), + pass_scope("Branch", value="feature/add-login"), + ] + body = main.render_report(results) + self.assertEqual( + body, + f"{main.COMMENT_MARKER}\n" + f"{main.REPORT_TITLE}\n" + "\n" + "❌ **1 of 3 checks failed**\n" + "\n" + "| Scope | Checked value | Failed checks |\n" + "|---|---|---|\n" + "| Commit 2/2 | `bad message` | " + "[CC001 message](https://commit-check.com/rules/#cc001) |\n" + "\n" + "
\n" + "Show all 3 checks\n" + "\n" + "```text\n" + "Commit message\n" + " ✔ PR title (feat: add login page)\n" + " ✖ Commit 2/2 (1 failure)\n" + " CC001 message\n" + " value: bad message\n" + " The commit message should follow Conventional Commits.\n" + " Suggest: Use (): \n" + "Branch\n" + " ✔ Branch (feature/add-login)\n" + "```\n" + "\n" + "
\n" + "\n" + f"{FOOTER}", + ) - def test_failure_body(self): - result = main.build_result_body("bad commit") - self.assertIn(main.FAILURE_TITLE, result) - self.assertIn("bad commit", result) + def test_a_check_is_a_thing_checked_not_a_rule_evaluation(self): + """The total counts scopes, so it tracks the policy, not the PR size. + + Counting rule evaluations made the denominator grow with the number of + commits: sixteen messages against six enabled rules reported "1 of 100 + checks failed", which both overstated the work done and made one bad + commit out of fifteen look negligible. Two rules failing on one commit + is still one thing to go and fix. + """ + two_failures = main.ScopeResult( + label="Commit 1/2", + checks=[ + make_check("message", status="fail", rule_id="CC001"), + make_check("subject_min_length", status="fail", rule_id="CC005"), + ], + ) + body = main.render_report([pass_scope("Branch"), two_failures]) + self.assertIn("❌ **1 of 2 checks failed**", body) + self.assertIn("Show all 2 checks", body) + # Both failing rules are still named, in the table and the details. + self.assertIn("CC001 message", body) + self.assertIn("CC005 subject-min-length", body) + + def test_total_does_not_grow_with_the_number_of_rules(self): + """Adding rules to one scope must not change the headline total.""" + one_rule = [pass_scope("Branch")] + many_rules = [ + main.ScopeResult( + label="Branch", + checks=[ + make_check(f"rule_{i}", rule_id=f"CC{i:03d}") for i in range(9) + ], + ) + ] + self.assertIn("✅ **All 1 check passed**", main.render_report(one_rule)) + self.assertIn("✅ **All 1 check passed**", main.render_report(many_rules)) + + def test_table_has_no_constant_result_column(self): + """Only failures reach the table, so a result column would never vary.""" + body = main.render_job_summary([fail_scope("Commit 1/1")]) + self.assertIn("| Scope | Checked value | Failed checks |", body) + self.assertNotIn("| Result |", body) + + def test_body_opens_with_hidden_marker(self): + body = main.render_job_summary([pass_scope("Branch")]) + self.assertTrue(body.startswith(main.COMMENT_MARKER)) + + def test_all_pass(self): + body = main.render_job_summary([pass_scope("Branch", value="main")]) + self.assertIn(main.REPORT_TITLE, body) + self.assertIn("✅ **All 1 check passed**", body) + self.assertIn("
", body) + self.assertIn("Show all 1 check", body) + self.assertIn("```text", body) + self.assertIn("Branch", body) + self.assertIn(" ✔ Branch (main)", body) + + def test_all_pass_groups_scopes_like_step_log(self): + results = [ + pass_scope("PR title", value="feat: add login page"), + pass_scope("Commit 1/2", value="feat: add user auth"), + pass_scope("Commit 2/2", value="fix: resolve timeout"), + pass_scope("Branch", value="feature/pr-12"), + pass_scope("Author name", value="Jane Doe"), + pass_scope("Author email", value="jane@example.com"), + ] + body = main.render_job_summary(results) + # Group headers in the details block mirror the step log ordering. + self.assertLess(body.index("Commit message"), body.index("Branch")) + self.assertLess(body.index("Branch"), body.index("Author")) + self.assertIn(" ✔ PR title (feat: add login page)", body) + self.assertIn(" ✔ Branch (feature/pr-12)", body) + self.assertIn(" ✔ Author email (jane@example.com)", body) + + def test_all_pass_truncates_long_values(self): + long_value = "x" * 200 + body = main.render_job_summary([pass_scope("Commit 1/1", value=long_value)]) + self.assertIn(f" ✔ Commit 1/1 ({'x' * 57}...)", body) + + def test_all_pass_without_value_shows_plain_label(self): + body = main.render_job_summary([pass_scope("Branch")]) + self.assertIn(" ✔ Branch", body) + self.assertNotIn(" ✔ Branch (", body) + + def test_failure_renders_table_with_rule_links(self): + body = main.render_job_summary([fail_scope("Commit 1/1")]) + self.assertIn(main.REPORT_TITLE, body) + self.assertIn("❌ **1 of 1 check failed**", body) + self.assertIn("| Scope | Checked value | Failed checks |", body) + self.assertIn( + "| Commit 1/1 | `bad message` | " + "[CC001 message](https://commit-check.com/rules/#cc001) |", + body, + ) + self.assertIn("
", body) + self.assertIn("Show all 1 check", body) + self.assertIn("```text", body) + self.assertIn("✖ Commit 1/1 (1 failure)", body) + self.assertIn(" CC001 message", body) + # The failing value appears in full; the table truncates at 60. + self.assertIn(" value: bad message", body) + self.assertIn(" Suggest: Use (): ", body) + self.assertIn("[Rules reference](https://commit-check.com/rules/)", body) + + def test_failure_details_show_all_scopes_and_values(self): + results = [ + fail_scope("Commit 1/2"), + pass_scope("Commit 2/2", value="fix: resolve timeout"), + ] + body = main.render_job_summary(results) + self.assertIn("✔ Commit 2/2 (fix: resolve timeout)", body) + self.assertIn("✖ Commit 1/2 (1 failure)", body) + + def test_pass_scope_renders_checkmark_without_value(self): + body = main.render_job_summary([pass_scope("Branch"), fail_scope("Commit 1/1")]) + # Pass scopes stay out of the table; the details block carries them. + table = body.split("
")[0] + self.assertNotIn("| Branch |", table) + self.assertIn("| Commit 1/1 | `bad message` |", table) + self.assertIn("✔ Branch", body) + + +class TestRenderPrComment(unittest.TestCase): + def test_all_pass_matches_job_summary(self): + comment = main.render_pr_comment([pass_scope("Branch")]) + summary = main.render_job_summary([pass_scope("Branch")]) + self.assertEqual(comment, summary) + self.assertTrue(comment.startswith(main.COMMENT_MARKER)) + self.assertIn("✅ **All 1 check passed**", comment) + + def test_failure_matches_job_summary(self): + comment = main.render_pr_comment([fail_scope("Commit 1/1")]) + summary = main.render_job_summary([fail_scope("Commit 1/1")]) + self.assertEqual(comment, summary) + self.assertTrue(comment.startswith(main.COMMENT_MARKER)) + self.assertIn("❌ **1 of 1 check failed**", comment) + self.assertIn("| Scope | Checked value | Failed checks |", comment) class TestAddJobSummary(unittest.TestCase): - def setUp(self): - import tempfile - - self._orig_dir = os.getcwd() - self._tmpdir = tempfile.mkdtemp() - os.environ["RUNNER_TEMP"] = self._tmpdir - os.chdir(self._tmpdir) - with open(main.get_result_path(), "w", encoding="utf-8"): - pass - - def tearDown(self): - os.chdir(self._orig_dir) - os.environ.pop("RUNNER_TEMP", None) - def test_false_skips(self): with patch("main.JOB_SUMMARY_ENABLED", False): - rc = main.add_job_summary() + rc = main.add_job_summary([pass_scope()]) self.assertEqual(rc, 0) - def test_success_writes_success_title(self): - summary_path = os.path.join(self._tmpdir, "summary.txt") + def test_success_writes_policy_report(self): + summary_path = os.path.join(tempfile.mkdtemp(), "summary.txt") with ( patch("main.JOB_SUMMARY_ENABLED", True), patch("main.GITHUB_STEP_SUMMARY", summary_path), - patch("main.read_result_file", return_value=None), ): - rc = main.add_job_summary() + rc = main.add_job_summary([pass_scope("Branch")]) self.assertEqual(rc, 0) with open(summary_path, encoding="utf-8") as file_obj: content = file_obj.read() - self.assertIn(main.SUCCESS_TITLE, content) + self.assertIn("✅ **All 1 check passed**", content) - def test_failure_writes_failure_title(self): - summary_path = os.path.join(self._tmpdir, "summary.txt") + def test_failure_returns_nonzero(self): + summary_path = os.path.join(tempfile.mkdtemp(), "summary.txt") with ( patch("main.JOB_SUMMARY_ENABLED", True), patch("main.GITHUB_STEP_SUMMARY", summary_path), - patch("main.read_result_file", return_value="bad commit message"), ): - rc = main.add_job_summary() + rc = main.add_job_summary([fail_scope()]) self.assertEqual(rc, 1) with open(summary_path, encoding="utf-8") as file_obj: content = file_obj.read() - self.assertIn(main.FAILURE_TITLE, content) - self.assertIn("bad commit message", content) + self.assertIn("| Scope | Checked value | Failed checks |", content) + self.assertIn("❌", content) -class TestAddPrComments(unittest.TestCase): - def setUp(self): - import tempfile - - self._orig_dir = os.getcwd() - self._tmpdir = tempfile.mkdtemp() - os.environ["RUNNER_TEMP"] = self._tmpdir - os.chdir(self._tmpdir) - with open(main.get_result_path(), "w", encoding="utf-8"): - pass +class TestSetResultOutput(unittest.TestCase): + def test_writes_heredoc_json(self): + output_path = os.path.join(tempfile.mkdtemp(), "output.txt") + with patch.dict(os.environ, {"GITHUB_OUTPUT": output_path}): + main.set_result_output([fail_scope("Commit 1/1"), pass_scope("Branch")]) + with open(output_path, encoding="utf-8") as file_obj: + content = file_obj.read() + self.assertIn("result< MagicMock: + comment = MagicMock() + comment.body = body + comment.user.type = user_type + return comment + + def test_marked_comment_is_updated_and_older_ones_deleted(self): + first = self._comment(f"{main.COMMENT_MARKER}\nold") + second = self._comment(f"{main.COMMENT_MARKER}\nnewer") + target, stale = main._find_own_comments([first, second]) + self.assertIs(target, second) + self.assertEqual(stale, [first]) + + def test_human_comment_with_the_old_title_is_never_deleted(self): + """A person can type '# Commit Check'; deleting on that is destructive.""" + human = self._comment("# Commit Check\nwhy is this failing?", user_type="User") + mine = self._comment(f"{main.COMMENT_MARKER}\nreport") + target, stale = main._find_own_comments([human, mine]) + self.assertIs(target, mine) + self.assertEqual(stale, []) + + def test_human_comment_with_the_old_title_is_not_adopted(self): + human = self._comment("# Commit Check\nwhy is this failing?", user_type="User") + target, stale = main._find_own_comments([human]) + self.assertIsNone(target) + self.assertEqual(stale, []) + + def test_bot_comment_from_an_older_version_is_adopted(self): + legacy = self._comment("# Commit-Check\nold report") + target, stale = main._find_own_comments([legacy]) + self.assertIs(target, legacy) + self.assertEqual(stale, []) + + def test_no_comments_yields_nothing_to_update(self): + target, stale = main._find_own_comments([]) + self.assertIsNone(target) + self.assertEqual(stale, [])