diff --git a/README.md b/README.md index 92bbe03..360db48 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,7 @@ runners need a few tools — see [Good to know](#good-to-know). | `branch` | `true` | Check the branch name against [Conventional Branch](https://conventionalbranch.org/) | | `author-name` | `false` | Check each commit's author name | | `author-email` | `false` | Check each commit's author email | +| `files` | `false` | Check the files each commit changes against the `[files]` rules of `cchk.toml` (size, prohibited paths, path length). Needs `fetch-depth: 0`: a commit the clone holds without its parent is skipped | | `pr-title` | `false` | Check the pull request title against Conventional Commits — the one that matters for squash merges. Pull request events only | | `message-ai-attribution` | `false` | Check every commit against the AI rules of `cchk.toml` (`ai_attribution`) only, author included, without the format rules. Takes the place of `message` | | `pr-body-ai-attribution` | `false` | Check the pull request description against the AI rules of `cchk.toml` (`ai_attribution`) only. Pull request events only | diff --git a/action.yml b/action.yml index 9a1487c..b891bf2 100644 --- a/action.yml +++ b/action.yml @@ -21,6 +21,10 @@ inputs: description: check committer author email required: false default: false + files: + description: check the files each commit changes against the [files] rules in cchk.toml (size, prohibited paths, path length); needs fetch-depth 0 + required: false + default: false dry-run: description: report failures (summary, PR comment, annotations as warnings) but always exit 0 required: false @@ -122,6 +126,7 @@ runs: BRANCH: ${{ inputs.branch }} AUTHOR_NAME: ${{ inputs.author-name }} AUTHOR_EMAIL: ${{ inputs.author-email }} + FILES: ${{ inputs.files }} DRY_RUN: ${{ inputs.dry-run }} JOB_SUMMARY: ${{ inputs.job-summary }} PR_COMMENTS: ${{ inputs.pr-comments }} diff --git a/main.py b/main.py index e496ac4..4b48787 100755 --- a/main.py +++ b/main.py @@ -49,6 +49,10 @@ } +#: Label of the scopes that check the files a commit changes (``files: true``). +FILES_LABEL = "Files" + + #: Boolean inputs whose value was neither true nor false, as ``(name, value)``. #: ``main`` fails the step on them before any check runs. INVALID_FLAGS: list[tuple[str, str]] = [] @@ -79,6 +83,7 @@ def _reconfigure_io() -> None: BRANCH_ENABLED = env_flag("BRANCH") AUTHOR_NAME_ENABLED = env_flag("AUTHOR_NAME") AUTHOR_EMAIL_ENABLED = env_flag("AUTHOR_EMAIL") +FILES_ENABLED = env_flag("FILES") DRY_RUN_ENABLED = env_flag("DRY_RUN") JOB_SUMMARY_ENABLED = env_flag("JOB_SUMMARY") PR_COMMENTS_ENABLED = env_flag("PR_COMMENTS") @@ -259,6 +264,7 @@ def log_env_vars(): "BRANCH", "AUTHOR_NAME", "AUTHOR_EMAIL", + "FILES", "DRY_RUN", "JOB_SUMMARY", "PR_COMMENTS", @@ -442,6 +448,36 @@ def skipped_author_scope(flag: str) -> ScopeResult: ) +#: Why a files scope checked nothing. +FILES_UNCONFIGURED = "no rule is set in the [files] section of the config" +FILES_NOT_IN_CLONE = "the checkout lacks the commit or its parent" +FILES_UNLISTED = "the pull request's commits could not be listed" +FILES_NOTHING_CHANGED = "the commit adds or changes no file" + + +def skipped_files_scope(reason: str, sha: str = "") -> ScopeResult: + """A scope recording that the files of a commit, or of all, went unchecked. + + Reported as ``skip``, never as a pass: nothing was validated. + """ + return ScopeResult( + label=FILES_LABEL, + sha=sha, + checks=[ + { + "rule_id": "", + "check": "files", + "status": "skip", + "value": "", + "error": "", + "suggest": "", + "docs_url": "", + "reason": reason, + } + ], + ) + + #: Why an AI-only scope ran no rule: under the default policy there is none to run. AI_POLICY_IGNORED = 'ai_attribution is "ignore": set it to "forbid" or "disclose"' @@ -919,6 +955,91 @@ def run_pr_author_checks( ] +def _shallow_boundary() -> set[str]: + """The commits a shallow clone holds without their parents. + + Git lists them, one hash per line, in the repository's ``shallow`` file; + a complete clone has none. Such a commit looks like a root commit, so a + diff of it names every file in the repository as changed. + """ + try: + located = subprocess.run( + ["git", "rev-parse", "--git-path", "shallow"], + capture_output=True, + encoding="utf-8", + check=False, + ) + if located.returncode != 0: + return set() + with open(located.stdout.strip(), encoding="utf-8") as f: + return set(f.read().split()) + except OSError: + return set() + + +def run_files_checks(shas: list[str]) -> list[ScopeResult]: + """Check the files each commit changes, via ``commit-check --files --rev``. + + One scope per commit, so a finding names the commit that has to change: + a file added in one commit and removed in the next is still in the + history. A commit is only checked when the checkout holds it together + with its parent, since the files it changes are a diff against that + parent. The others are reported as skipped, never guessed at. + + With no rule set in ``[files]`` the CLI runs none, for any commit, and + one skipped scope says so in place of a row of empty passes. + """ + boundary = _shallow_boundary() + readable = { + sha for sha in shas if sha and sha not in boundary and _rev_resolves(sha) + } + if not readable: + # Whether a rule is set at all, asked of HEAD: only the list of + # checks is read, never a verdict on HEAD itself. + probe = check_scope(FILES_LABEL, ["--files"]) + if not (probe.checks or probe.raw_text): + return [skipped_files_scope(FILES_UNCONFIGURED)] + warn_shallow_checkout( + "The checkout holds no commit to check together with its parent", + "the files check was skipped", + ) + return [skipped_files_scope(FILES_NOT_IN_CLONE)] + + results: list[ScopeResult] = [] + for sha in shas: + if sha not in readable: + results.append(skipped_files_scope(FILES_NOT_IN_CLONE, sha)) + continue + scope = check_scope(FILES_LABEL, ["--files", "--rev", sha], sha=sha) + if not (scope.checks or scope.raw_text): + return [skipped_files_scope(FILES_UNCONFIGURED)] + if scope.status == "skip" and not scope.skip_reason: + # The file rules skip a commit that only deletes, or merges + # without bringing a file along, and do not say why. + for check in scope.checks: + check["reason"] = FILES_NOTHING_CHANGED + results.append(scope) + missing = sum(1 for sha in shas if sha not in readable) + if missing: + warn_shallow_checkout( + f"{missing} of {len(shas)} commits are not in the checkout " + "together with their parent", + "their files were not checked", + ) + return results + + +def run_pr_files_checks(pr_commits: list[Commit]) -> list[ScopeResult]: + """Check the files of every commit of the pull request.""" + if not pr_commits: + warn_shallow_checkout( + "Could not list the pull request's commits", + "the files check was skipped", + ) + return [skipped_files_scope(FILES_UNLISTED)] + return run_files_checks([sha for sha, _msg in pr_commits]) + + def check_empty_message( label: str, sha: str, env: dict[str, str] | None ) -> ScopeResult: @@ -976,7 +1097,9 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]: scope is reported) 3. Individual PR commit messages (when ``message: true`` and in a PR event; AI rules only with ``message-ai-attribution: true``) - 4. Author checks on every PR commit's author, then the remaining checks + 4. The files each PR commit changes (when ``files: true``; outside a PR, + the files of the HEAD commit) + 5. Author checks on every PR commit's author, then the remaining checks (branch; outside a PR, the HEAD commit and its author) Outside of a PR event all enabled checks are handed to the CLI at once. @@ -1021,13 +1144,23 @@ def run_commit_check() -> tuple[int, list[ScopeResult]]: "Could not list the pull request's commits", "only HEAD was checked" ) - # ---- 4. Remaining checks (branch, author, etc.) ----------------------- # Outside a PR, check the HEAD commit message directly. if "--message" in args: results.append( keep_ai_rules(check_scope("Commit message", ["--message"], sha=head_sha())) ) args = [a for a in args if a != "--message"] + + # ---- 4. Files of each commit ------------------------------------------ + if FILES_ENABLED: + if is_pr_event(): + if pr_commits is None: + pr_commits = get_pr_commit_messages() + results.extend(run_pr_files_checks(pr_commits)) + else: + results.extend(run_files_checks([head_sha()])) + + # ---- 5. Remaining checks (branch, author, etc.) ----------------------- rev = None author_flags = [flag for flag in args if flag in AUTHOR_FLAGS] if is_pr_event() and author_flags: @@ -1106,6 +1239,19 @@ def _rule_ref(check: dict[str, str]) -> str: return f"[{label}]({docs_url})" if docs_url else label +def _names_a_commit(scope: ScopeResult) -> bool: + """Whether the scope is one commit's: its message, or the files it changes.""" + return bool(scope.sha) and ( + scope.label.startswith("Commit") or scope.label == FILES_LABEL + ) + + +def _found(check: dict[str, str]) -> str: + """First line of what a rule reports it found.""" + value = str(check.get("value") or "").strip() + return value.splitlines()[0].strip() if value else "" + + def _scope_group(label: str) -> str: """Group name for a scope label, used to fold the step log output.""" if label in ("PR title", "PR description"): @@ -1167,6 +1313,12 @@ def _first_value(scope: ScopeResult) -> str: """ if scope.label == "PR description": return "" + if scope.label == FILES_LABEL: + # Each file rule names its own finding, a path and what is wrong + # with it. The first finding stands for the scope; with none, the + # count of files a passing rule reports. + checks = [*scope.failures, *scope.warnings] or scope.checks + return next((_found(check) for check in checks if _found(check)), "") values = [scope.text, _message_value(scope)] values.extend( str(check.get("value") or "") @@ -1265,12 +1417,17 @@ def _instruction(scope: ScopeResult, check: dict[str, str]) -> str: return ", and ".join(parts) or _advice(check) -def _target(scope: ScopeResult, instructions: list[str], label: str) -> str: +def _target( + scope: ScopeResult, instructions: list[str], label: str, value: str | None = None +) -> str: """Name what an instruction is about: ``Commit 2/3 (5584f46) "bad msg"``. - The quoted value is left out when an instruction already quotes it. + The quoted value is left out when an instruction already quotes it. It + is the scope's own unless ``value`` names another: a files scope holds + one finding per rule, each about a path of its own. """ - value = _first_value(scope) + if value is None: + value = _first_value(scope) if value and not any(f'"{value}"' in text for text in instructions): return f'{label} "{value}"' return label @@ -1340,7 +1497,7 @@ def _row_label(scope: ScopeResult) -> str: The group header already says "Commits" or "Author", so the row only needs what tells it apart from its neighbours. """ - if scope.sha and scope.label.startswith("Commit"): + if _names_a_commit(scope): return scope.sha[:7] _group, _, name = scope.label.partition(" ") return name or scope.label.lower() @@ -1468,7 +1625,9 @@ def _annotation_body(scope: ScopeResult, check: dict[str, str]) -> str: for whoever expands it. """ instruction = _instruction(scope, check) - headline = f"{_target(scope, [instruction], scope.display_label)}: {instruction}" + found = _found(check) if scope.label == FILES_LABEL else None + target = _target(scope, [instruction], scope.display_label, found) + headline = f"{target}: {instruction}" error = _plain_error(check) return headline if error == instruction else f"{headline}\n{error}" @@ -1701,8 +1860,21 @@ def _hunk(scope: ScopeResult, status: str) -> list[str]: literal text to offer. GitHub colours all three in a ``diff`` block, which is the only colour a comment or a job summary can carry. """ - is_commit = bool(scope.sha) and scope.label.startswith("Commit") - lines = [f"@@ {scope.sha[:7] if is_commit else scope.label} @@"] + if scope.sha and scope.label.startswith("Commit"): + where = scope.sha[:7] + elif scope.sha and scope.label == FILES_LABEL: + where = f"{scope.sha[:7]} files" + else: + where = scope.label + lines = [f"@@ {where} @@"] + if scope.label == FILES_LABEL and scope.checks: + # One finding per rule, each about a path of its own: the path it + # found, then what to do about it. + for check in _findings(scope, status): + if _found(check): + lines.append(f"- {_found(check)}") + lines.append(f"# {_advice(check)}") + return lines value = _first_value(scope) removed: list[str] = [] added: list[str] = [] @@ -1759,11 +1931,13 @@ def _diff_block(results: list[ScopeResult], status: str = "fail") -> list[str]: rest = [line for hunk in hunks[shown:] for line in hunk] lines += ["", "
", f"and {len(hunks) - shown} more"] lines += ["", *_fenced("diff", rest), "", "
"] - commits = [ - f"[`{scope.sha[:7]}`]({_commit_url(scope.sha)})" - for scope in scopes[:shown] - if scope.label.startswith("Commit") and _commit_url(scope.sha) - ] + commits = list( + dict.fromkeys( + f"[`{scope.sha[:7]}`]({_commit_url(scope.sha)})" + for scope in scopes[:shown] + if _names_a_commit(scope) and _commit_url(scope.sha) + ) + ) rules = list( dict.fromkeys( _rule_ref(check) for scope in scopes for check in _findings(scope, status) @@ -1781,6 +1955,10 @@ def _diff_block(results: list[ScopeResult], status: str = "fail") -> list[str]: #: First line of the prompt "How to fix" offers for an AI coding agent. AGENT_PROMPT = "Fix these Commit Check findings without changing any code:" +#: The same line when a finding is about a file, which then has to change. +AGENT_PROMPT_FILES = ( + "Fix these Commit Check findings, changing only the files they name:" +) BRANCH_HELP = ( "**Branch name.** Rename the branch and push it. A pull request cannot " "change its branch, so open a new one from the renamed branch:" @@ -1790,6 +1968,11 @@ def _diff_block(results: list[ScopeResult], status: str = "fail") -> list[str]: "`git config user.email`), then `git commit --amend --reset-author` " "and force-push." ) +FILES_HELP = ( + "**Files.** Amend the commit that changed the file (`git rebase -i`, or " + "`git commit --amend` for the last one), then force-push. A later commit " + "that removes the file leaves it in the history." +) HOOK_HELP = ( f"**Next time.** The [pre-commit hook]({LOCAL_HOOK_URL}) runs the same " "rules before you push." @@ -1797,9 +1980,21 @@ def _diff_block(results: list[ScopeResult], status: str = "fail") -> list[str]: def _agent_prompt(failing: list[ScopeResult]) -> list[str]: - """A prompt to paste into an AI coding agent, one line per failing scope.""" - lines = [AGENT_PROMPT] + """A prompt to paste into an AI coding agent, one line per failing scope. + + A files scope gets a line per finding: each is about a path of its own. + """ + about_files = any(scope.label == FILES_LABEL for scope in failing) + lines = [AGENT_PROMPT_FILES if about_files else AGENT_PROMPT] for scope in failing: + if scope.label == FILES_LABEL: + label = f"Files of commit {scope.sha[:7]}" if scope.sha else scope.label + for check in scope.failures: + todo = [_advice(check)] + lines.append( + f"- {_target(scope, todo, label, _found(check))}: {todo[0]}" + ) + continue todo = list(dict.fromkeys(_instruction(scope, c) for c in scope.failures)) is_commit = bool(scope.sha) and scope.label.startswith("Commit") label = f"Commit {scope.sha[:7]}" if is_commit else scope.label @@ -1845,6 +2040,8 @@ def _how_to_fix(results: list[ScopeResult]) -> list[str]: many = "it" if len(commits) == 1 else "them" parts.append([f"**Commit messages.** Reword {many}, then force-push:"]) parts.append(_fenced("bash", _reword_commands(commits))) + if FILES_LABEL in labels: + parts.append([FILES_HELP]) if any(label.startswith("Author") for label in labels): parts.append([AUTHOR_HELP]) if "Branch" in labels: @@ -2021,6 +2218,11 @@ def _all_checks(results: list[ScopeResult]) -> list[str]: # folded; passing scopes live in the listing. # - A commit is named by its short hash on every surface; the label # ("Commit 1/11") stays bare in the `result` output. +# - With `files: true` each commit also gets a row in a "Files" group, for the +# files it changes. Its hunk reads `@@ 5584f46 files @@`, with a red line per +# path a rule found and the rule's advice under it. The agent prompt then +# opens with "changing only the files they name" in place of "without +# changing any code". # - One check is one thing that was checked — a commit message, the branch, the # author — not one rule evaluation. The totals therefore match the number of # ✔/✖ rows the reader can count in the listing, and do not grow with the diff --git a/main_test.py b/main_test.py index e3476c5..7e43c21 100644 --- a/main_test.py +++ b/main_test.py @@ -5,6 +5,7 @@ import io import json import os +import pathlib import re import runpy import shutil @@ -172,6 +173,7 @@ def test_every_input_is_logged_as_a_debug_command(self): "::debug::BRANCH=false", "::debug::AUTHOR_NAME=false", "::debug::AUTHOR_EMAIL=false", + "::debug::FILES=false", "::debug::DRY_RUN=false", "::debug::JOB_SUMMARY=false", "::debug::PR_COMMENTS=false", @@ -5160,3 +5162,540 @@ def run(command, **kwargs): [(s.label, s.sha, s.status) for s in results], [("Commit 1/1", "a1", "fail")], ) + + +def files_scope(sha: str = SHA_B, *checks) -> main.ScopeResult: + """One commit's files scope; by default the two findings of a bad commit.""" + return main.ScopeResult( + label="Files", + sha=sha, + checks=list(checks) + or [ + make_check( + "file_size", + status="fail", + rule_id="CC302", + value="big.bin (4 KB)", + error="File exceeds the maximum size of 1 KB", + suggest="Keep files under 1 KB, or raise max_size in the [files] config", + docs_url="https://commit-check.com/rules/#cc302", + ), + make_check( + "file_pattern", + status="fail", + rule_id="CC303", + value="key.pem (pattern *.pem)", + error="File path matches a prohibited pattern", + suggest="Remove the file from the commit", + docs_url="https://commit-check.com/rules/#cc303", + ), + ], + ) + + +def files_pass(value: str = "2 file(s)", status: str = "pass") -> dict[str, str]: + return make_check("file_pattern", status=status, rule_id="CC303", value=value) + + +class TestShallowBoundary(unittest.TestCase): + """The commits a shallow clone holds without their parents.""" + + def test_a_shallow_clone_lists_them_in_its_shallow_file(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "shallow") + with open(path, "w", encoding="utf-8") as f: + f.write(f"{SHA_A}\n{SHA_B}\n") + located = MagicMock(returncode=0, stdout=f"{path}\n") + with patch("main.subprocess.run", return_value=located) as run: + self.assertEqual(main._shallow_boundary(), {SHA_A, SHA_B}) + self.assertEqual( + run.call_args.args[0], ["git", "rev-parse", "--git-path", "shallow"] + ) + + def test_a_complete_clone_has_no_such_file(self): + with tempfile.TemporaryDirectory() as tmp: + located = MagicMock(returncode=0, stdout=os.path.join(tmp, "shallow")) + with patch("main.subprocess.run", return_value=located): + self.assertEqual(main._shallow_boundary(), set()) + + def test_outside_a_repository_there_is_none(self): + with patch("main.subprocess.run", return_value=MagicMock(returncode=128)): + self.assertEqual(main._shallow_boundary(), set()) + + def test_without_git_there_is_none(self): + with patch("main.subprocess.run", side_effect=OSError("no git")): + self.assertEqual(main._shallow_boundary(), set()) + + +class TestRunFilesChecks(unittest.TestCase): + """``files: true`` checks the files of each commit, one scope per commit.""" + + def _run(self, shas, answers, boundary=(), missing=()): + """Run the files checks; ``answers`` maps a rev to the CLI's checks.""" + calls: list[list[str]] = [] + + def check_scope(label, args, sha=""): + calls.append(args) + answer = answers.get(args[-1], []) + if isinstance(answer, str): + return main.ScopeResult(label=label, raw_text=answer, sha=sha) + return main.ScopeResult(label=label, checks=list(answer), sha=sha) + + out = io.StringIO() + with ( + patch("main._shallow_boundary", return_value=set(boundary)), + patch("main._rev_resolves", side_effect=lambda sha: sha not in missing), + patch("main.check_scope", side_effect=check_scope), + patch("sys.stdout", out), + ): + results = main.run_files_checks(shas) + return results, calls, out.getvalue() + + def test_every_commit_is_checked_at_its_own_revision(self): + results, calls, out = self._run( + ["a1", "b2"], {"a1": [files_pass()], "b2": files_scope().checks} + ) + self.assertEqual( + calls, [["--files", "--rev", "a1"], ["--files", "--rev", "b2"]] + ) + self.assertEqual( + [(s.label, s.sha, s.status) for s in results], + [("Files", "a1", "pass"), ("Files", "b2", "fail")], + ) + self.assertEqual(out, "") + + def test_a_commit_without_its_parent_is_skipped_not_guessed_at(self): + """At the edge of a shallow clone every file would look newly added.""" + results, calls, out = self._run( + ["a1", "b2", "c3"], + {"c3": [files_pass()]}, + boundary=["b2"], + missing=["a1"], + ) + self.assertEqual(calls, [["--files", "--rev", "c3"]]) + self.assertEqual( + [(s.sha, s.status, s.skip_reason) for s in results], + [ + ("a1", "skip", main.FILES_NOT_IN_CLONE), + ("b2", "skip", main.FILES_NOT_IN_CLONE), + ("c3", "pass", ""), + ], + ) + self.assertIn( + "::warning title=commit-check::2 of 3 commits are not in the checkout " + "together with their parent (is actions/checkout using fetch-depth: 0?); " + "their files were not checked", + out, + ) + + def test_a_checkout_with_none_of_them_gets_one_skipped_scope(self): + results, calls, out = self._run( + ["a1", "b2"], {"--files": [files_pass()]}, missing=["a1", "b2"] + ) + # HEAD is only asked whether any rule is set; its verdict is not used. + self.assertEqual(calls, [["--files"]]) + self.assertEqual( + [(s.label, s.sha, s.status, s.skip_reason) for s in results], + [("Files", "", "skip", main.FILES_NOT_IN_CLONE)], + ) + self.assertIn("the files check was skipped", out) + + def test_an_unknown_head_is_not_checked(self): + """Outside a PR the one commit is HEAD; git may fail to name it.""" + results, calls, _out = self._run([""], {"--files": "boom"}) + self.assertEqual(calls, [["--files"]]) + self.assertEqual(results[0].skip_reason, main.FILES_NOT_IN_CLONE) + + def test_no_rule_set_is_one_skipped_scope_and_no_shallow_warning(self): + for missing in ([], ["a1", "b2"]): + results, _calls, out = self._run(["a1", "b2"], {}, missing=missing) + self.assertEqual( + [(s.label, s.sha, s.status, s.skip_reason) for s in results], + [("Files", "", "skip", main.FILES_UNCONFIGURED)], + ) + self.assertEqual(out, "") + + def test_a_commit_that_changes_no_file_says_so(self): + skipped = files_pass(value="", status="skip") + skipped["reason"] = "" + results, _calls, _out = self._run(["a1"], {"a1": [skipped]}) + self.assertEqual(results[0].status, "skip") + self.assertEqual(results[0].skip_reason, main.FILES_NOTHING_CHANGED) + + def test_the_clis_own_skip_reason_is_kept(self): + skipped = files_pass(value="", status="skip") + skipped["reason"] = "what the commit changed could not be read" + results, _calls, _out = self._run(["a1"], {"a1": [skipped]}) + self.assertEqual( + results[0].skip_reason, "what the commit changed could not be read" + ) + + def test_unparsable_output_is_a_failure_of_that_commit(self): + results, _calls, _out = self._run(["a1"], {"a1": "Traceback ..."}) + self.assertEqual([(s.sha, s.status) for s in results], [("a1", "fail")]) + + def test_a_pull_request_whose_commits_cannot_be_listed_is_skipped(self): + out = io.StringIO() + with patch("sys.stdout", out), patch("main.check_scope") as check: + results = main.run_pr_files_checks([]) + check.assert_not_called() + self.assertEqual( + [(s.label, s.status, s.skip_reason) for s in results], + [("Files", "skip", main.FILES_UNLISTED)], + ) + self.assertIn("Could not list the pull request's commits", out.getvalue()) + + def test_a_pull_request_hands_over_its_commits(self): + with patch("main.run_files_checks", return_value=[]) as run: + main.run_pr_files_checks([("a1", "feat: one"), ("b2", "feat: two")]) + run.assert_called_once_with(["a1", "b2"]) + + +class TestRunCommitCheckFiles(unittest.TestCase): + def _run(self, event="pull_request", message=True, files=True): + # Only a pull request has commits to list. + listable = event == "pull_request" + commits = [("a1", "feat: one"), ("b2", "feat: two")] if listable else [] + + def check_scope(label, args, **kwargs): + return main.ScopeResult( + label=label, checks=[files_pass()], sha=kwargs.get("sha", "") + ) + + with ( + patch("main.MESSAGE_ENABLED", message), + patch("main.BRANCH_ENABLED", True), + patch("main.AUTHOR_NAME_ENABLED", False), + patch("main.AUTHOR_EMAIL_ENABLED", False), + patch("main.FILES_ENABLED", files), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": event}), + patch("main.get_pr_commit_messages", return_value=commits) as listed, + patch("main._local_author", return_value=None), + patch("main._shallow_boundary", return_value=set()), + patch("main._rev_resolves", return_value=True), + patch("main.head_sha", return_value="h0"), + patch("main.check_scope", side_effect=check_scope), + ): + _rc, results = main.run_commit_check() + self.listed_calls = listed.call_count + return [(s.label, s.sha) for s in results] + + def test_off_by_default(self): + self.assertFalse(main.FILES_ENABLED) + self.assertNotIn("Files", [label for label, _ in self._run(files=False)]) + + def test_a_pull_request_gets_one_scope_per_commit_after_the_messages(self): + self.assertEqual( + self._run(), + [ + ("Commit 1/2", "a1"), + ("Commit 2/2", "b2"), + ("Files", "a1"), + ("Files", "b2"), + ("Branch", ""), + ], + ) + self.assertEqual(self.listed_calls, 1) + + def test_the_commits_are_listed_for_the_files_alone(self): + self.assertEqual( + self._run(message=False), + [("Files", "a1"), ("Files", "b2"), ("Branch", "")], + ) + self.assertEqual(self.listed_calls, 1) + + def test_outside_a_pull_request_the_head_commit_is_checked(self): + self.assertEqual( + self._run(event="push"), + [("Commit message", "h0"), ("Files", "h0"), ("Branch", "")], + ) + + +class TestFilesInTheReport(unittest.TestCase): + """A files scope holds one finding per rule, each about its own path.""" + + def test_the_first_finding_stands_for_the_scope(self): + scope = files_scope(SHA_B, files_pass("3 file(s)"), *files_scope().checks) + self.assertEqual(main._first_value(scope), "big.bin (4 KB)") + self.assertEqual( + main._first_value(files_scope(SHA_A, files_pass())), "2 file(s)" + ) + warned = files_scope( + SHA_A, files_pass("3 file(s)"), files_pass("a.pem (pattern *.pem)", "warn") + ) + self.assertEqual(main._first_value(warned), "a.pem (pattern *.pem)") + self.assertEqual(main._first_value(main.skipped_files_scope("why")), "") + + def test_the_listing_names_each_commit_by_its_hash(self): + results = [files_scope(SHA_A, files_pass()), files_scope()] + self.assertEqual( + main._render_tree(results, include_docs=False), + [ + "Files", + " ✔ d87faca 2 file(s)", + " ✖ 5584f46 big.bin (4 KB)", + " CC302 file-size", + " value: big.bin (4 KB)", + " File exceeds the maximum size of 1 KB", + ( + " Suggest: Keep files under 1 KB, or raise max_size in " + "the [files] config" + ), + " CC303 file-pattern", + " value: key.pem (pattern *.pem)", + " File path matches a prohibited pattern", + " Suggest: Remove the file from the commit", + ], + ) + self.assertEqual( + main._render_tree([main.skipped_files_scope("why")], include_docs=False), + ["Files", " ⊘ files skipped: why"], + ) + + def test_the_diff_shows_each_path_with_its_advice(self): + with patch.dict(os.environ, {"GITHUB_REPOSITORY": "o/r"}): + lines = main._diff_block( + [fail_scope("Commit 1/1", sha=SHA_B), files_scope()] + ) + self.assertEqual( + lines[: lines.index("```") + 1], + [ + "```diff", + "@@ 5584f46 @@", + "- bad message", + "# Use (): ", + "@@ 5584f46 files @@", + "- big.bin (4 KB)", + "# Keep files under 1 KB, or raise max_size in the [files] config", + "- key.pem (pattern *.pem)", + "# Remove the file from the commit", + "```", + ], + ) + # One link for the commit, though its message and its files both failed. + self.assertEqual( + lines[-1], + f"Commits: [`5584f46`](https://github.com/o/r/commit/{SHA_B})" + "  |  Rules: [CC001](https://commit-check.com/rules/#cc001)" + " · [CC302](https://commit-check.com/rules/#cc302)" + " · [CC303](https://commit-check.com/rules/#cc303)", + ) + + def test_a_warned_finding_goes_in_the_warning_diff(self): + warned = files_scope(SHA_A, files_pass("a.pem (pattern *.pem)", "warn")) + warned.checks[0]["suggest"] = "Remove the file from the commit" + lines = main._diff_block([warned], "warn") + self.assertEqual( + lines[1:4], + [ + "@@ d87faca files @@", + "- a.pem (pattern *.pem)", + "# Remove the file from the commit", + ], + ) + + def test_a_finding_that_names_nothing_still_gives_its_advice(self): + scope = files_scope(SHA_A, make_check("file_size", "fail", "CC302")) + self.assertEqual( + main._hunk(scope, "fail"), ["@@ d87faca files @@", "# check failed"] + ) + + def test_without_a_commit_the_scope_goes_by_its_label(self): + scope = files_scope("") + self.assertEqual(main._hunk(scope, "fail")[0], "@@ Files @@") + self.assertEqual( + main._agent_prompt([scope])[1], + '- Files "big.bin (4 KB)": Keep files under 1 KB, or raise max_size ' + "in the [files] config", + ) + self.assertEqual(main._row_label(scope), "files") + + def test_unparsable_output_points_to_the_step_log(self): + scope = main.ScopeResult(label="Files", sha=SHA_A, raw_text="Traceback ...") + self.assertEqual( + main._hunk(scope, "fail"), + [ + "@@ d87faca files @@", + "# the output could not be parsed, see the step log", + ], + ) + + def test_how_to_fix_gives_each_finding_a_line_and_no_reword_commands(self): + how = "\n".join(main._how_to_fix([files_scope()])) + self.assertIn( + '- Files of commit 5584f46 "big.bin (4 KB)": Keep files under 1 KB, ' + "or raise max_size in the [files] config\n" + '- Files of commit 5584f46 "key.pem (pattern *.pem)": Remove the file ' + "from the commit", + how, + ) + self.assertIn(main.FILES_HELP, how) + self.assertNotIn("Reword", how) + self.assertNotIn(main.FILES_HELP, "\n".join(main._how_to_fix([fail_scope()]))) + + def test_the_prompt_lets_the_named_files_change(self): + """Taking a file out of a commit cannot be done "without changing any code".""" + self.assertEqual(main._agent_prompt([fail_scope()])[0], AGENT_PROMPT) + for failing in ([files_scope()], [fail_scope(), files_scope()]): + self.assertEqual( + main._agent_prompt(failing)[0], + "Fix these Commit Check findings, changing only the files they name:", + ) + + def test_each_finding_is_an_annotation_about_its_own_path(self): + out = io.StringIO() + with patch("sys.stdout", out), patch("main.DRY_RUN_ENABLED", False): + main.render_step_log([files_scope()]) + log = ANSI_CODES.sub("", out.getvalue()) + self.assertIn("::group::✖ Files: failed", log) + self.assertIn( + '::error title=CC302 file-size::Files (5584f46) "big.bin (4 KB)": Keep ' + "files under 1 KB, or raise max_size in the [files] config" + "%0AFile exceeds the maximum size of 1 KB", + log, + ) + self.assertIn( + '::error title=CC303 file-pattern::Files (5584f46) "key.pem (pattern ' + '*.pem)": Remove the file from the commit' + "%0AFile path matches a prohibited pattern", + log, + ) + + def test_the_result_output_carries_the_commit(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "output") + with patch.dict(os.environ, {"GITHUB_OUTPUT": path}): + main.set_result_output([files_scope()]) + with open(path, encoding="utf-8") as f: + payload = json.loads(f.read().split("\n", 1)[1].rsplit("\n", 2)[0]) + self.assertEqual( + [(s["label"], s["sha"], s["status"]) for s in payload["scopes"]], + [("Files", SHA_B, "fail")], + ) + + +class TestFilesInputIsWired(unittest.TestCase): + """The input reaches the script, and the README lists it.""" + + def _read(self, name: str) -> str: + here = os.path.dirname(os.path.abspath(main.__file__)) + with open(os.path.join(here, name), encoding="utf-8") as f: + return f.read() + + def test_action_yml_declares_it_off_and_passes_it_on(self): + action = self._read("action.yml") + declared = re.search( + r"^ files:\n description: .+\n required: false\n default: false$", + action, + re.MULTILINE, + ) + self.assertIsNotNone(declared) + self.assertIn(" FILES: ${{ inputs.files }}\n", action) + + def test_the_readme_lists_it(self): + self.assertIn("| `files` | `false` |", self._read("README.md")) + + +@unittest.skipUnless( + shutil.which("commit-check") and shutil.which("git"), "needs commit-check and git" +) +class TestFilesAgainstRealCommits(unittest.TestCase): + """The pinned commit-check and git, unmocked, on a small repository.""" + + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.repo = os.path.join(tmp.name, "repo") + os.mkdir(self.repo) + gitconfig = os.path.join(tmp.name, "gitconfig") + with open(gitconfig, "w", encoding="utf-8") as f: + f.write( + "[user]\n\tname = test-author\n\temail = test@example.com\n" + "[commit]\n\tgpgsign = false\n[init]\n\tdefaultBranch = main\n" + ) + env = {k: v for k, v in os.environ.items() if not k.startswith("CCHK_")} + env["GIT_CONFIG_GLOBAL"] = gitconfig + env["GIT_CONFIG_SYSTEM"] = gitconfig + env_patch = patch.dict(os.environ, env, clear=True) + env_patch.start() + self.addCleanup(env_patch.stop) + cwd = os.getcwd() + self.addCleanup(os.chdir, cwd) + os.chdir(self.repo) + self._git("init", "-q") + self._commit( + "chore: init", {"cchk.toml": '[files]\nprohibited_patterns = ["*.pem"]\n'} + ) + + def _git(self, *args: str, cwd: str | None = None) -> str: + result = subprocess.run( + ["git", *args], + cwd=cwd or self.repo, + capture_output=True, + encoding="utf-8", + check=True, + ) + return result.stdout.strip() + + def _commit(self, subject: str, files: dict[str, str | None]) -> str: + for name, content in files.items(): + if content is None: + self._git("rm", "-q", name) + continue + with open(os.path.join(self.repo, name), "w", encoding="utf-8") as f: + f.write(content) + self._git("add", "-A") + self._git("commit", "-q", "-m", subject) + return self._git("rev-parse", "HEAD") + + def test_each_commit_answers_for_the_files_it_changes(self): + good = self._commit("feat: app", {"app.py": "x = 1\n"}) + bad = self._commit("feat: key", {"key.pem": "k\n"}) + gone = self._commit("fix: drop key", {"key.pem": None}) + with patch("sys.stdout", io.StringIO()): + results = main.run_files_checks([good, bad, gone]) + self.assertEqual( + [(s.sha, s.status) for s in results], + [(good, "pass"), (bad, "fail"), (gone, "skip")], + ) + self.assertEqual( + [(c["rule_id"], c["value"]) for c in results[1].failures], + [("CC303", "key.pem (pattern *.pem)")], + ) + self.assertEqual(results[2].skip_reason, main.FILES_NOTHING_CHANGED) + + def test_no_rule_set_is_reported_as_skipped(self): + sha = self._commit("chore: no rules", {"cchk.toml": ""}) + with patch("sys.stdout", io.StringIO()), patch("sys.stderr", io.StringIO()): + results = main.run_files_checks([sha]) + self.assertEqual( + [(s.status, s.skip_reason) for s in results], + [("skip", main.FILES_UNCONFIGURED)], + ) + + def test_a_shallow_clone_skips_the_commit_at_its_edge(self): + """Without the guard it would answer for every file in the repository.""" + self._commit("feat: key", {"key.pem": "k\n"}) + tip = self._commit("feat: app", {"app.py": "x = 1\n"}) + shallow = os.path.join(os.path.dirname(self.repo), "shallow") + # A URL, not a path: git ignores --depth when cloning a local path. + origin = pathlib.Path(self.repo).as_uri() + self._git( + "clone", + "-q", + "--depth", + "1", + origin, + shallow, + cwd=os.path.dirname(self.repo), + ) + os.chdir(shallow) + self.assertEqual(main._shallow_boundary(), {tip}) + out = io.StringIO() + with patch("sys.stdout", out): + results = main.run_files_checks([tip]) + self.assertEqual( + [(s.status, s.skip_reason) for s in results], + [("skip", main.FILES_NOT_IN_CLONE)], + ) + self.assertIn("fetch-depth: 0", out.getvalue())