From a41187db55fea0ffa84fd15ceac1c0bea85262ea Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Wed, 12 Aug 2026 08:08:17 +0000 Subject: [PATCH 1/5] fix: never block waiting for stdin that nothing will write commit-check read stdin whenever it was not a tty, for every check type. On a pipe that is open but that nothing will ever write to or close -- which is what stdin looks like under some CI runners and process managers -- read() blocks forever, so 'commit-check --author-name' became a stuck step rather than a failed one. Reproduced: it hangs until killed. Gate the read with select(): piped input is already in the pipe buffer by the time this process starts (and EOF counts as readable), while an idle pipe is not readable and never will be. Windows keeps the historic blocking read, since select() only handles sockets there and the hang has only been observed on POSIX runners. Three regression tests use real pipes rather than mocks: the idle-open pipe returns None promptly, piped content still arrives, /dev/null reads as nothing. The existing tests that fake piped input by mocking sys.stdin.read get an autouse fixture that opens the gate, restoring the semantics those mocks assume. --- commit_check/main.py | 32 +++++++++++++++++++-- pyproject.toml | 1 + tests/main_test.py | 66 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/commit_check/main.py b/commit_check/main.py index a7a044c3..e18c4db7 100644 --- a/commit_check/main.py +++ b/commit_check/main.py @@ -22,10 +22,38 @@ class StdinReader: """Handles stdin reading with proper error handling.""" @staticmethod - def read_piped_input() -> str | None: + def _has_pending_data(timeout: float) -> bool: + """Whether reading stdin would return promptly rather than block. + + ``read()`` on a pipe that is open but has no writer about to close it + blocks forever. That is what stdin looks like under some CI runners + and process managers, so an unconditional read turns "nothing was + piped" into a hang — the step neither fails nor finishes. + + ``select`` distinguishes the two: piped input is already in the pipe + buffer by the time this process is exec'd (and EOF, as with + ``< /dev/null``, counts as readable), while an idle pipe is not + readable and never will be. The timeout is margin, not a wait. + """ + if sys.platform == "win32": # pragma: no cover + # select() only works on sockets on Windows. Keep the historic + # blocking read there; the hang has only been observed on POSIX + # runners, and a wrong guess here would break piping instead. + return True + import select + + try: + ready, _, _ = select.select([sys.stdin], [], [], timeout) + except (OSError, ValueError): + # No usable stdin descriptor at all: nothing to read. + return False + return bool(ready) + + @classmethod + def read_piped_input(cls) -> str | None: """Read commit message content if piped, with proper error handling.""" try: - if not sys.stdin.isatty(): + if not sys.stdin.isatty() and cls._has_pending_data(timeout=0.1): data = sys.stdin.read() return data.strip() if data else None except (OSError, IOError): diff --git a/pyproject.toml b/pyproject.toml index 3f788b85..61ac7f5f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -80,4 +80,5 @@ omit = [ # Silence PytestUnknownMarkWarning for custom marks used in tests markers = [ "benchmark: performance-related tests (no-op marker in this project)", + "real_stdin_gate: opt out of the fixture that force-opens the stdin readiness gate", ] diff --git a/tests/main_test.py b/tests/main_test.py index d1b5f59c..2ff084a3 100644 --- a/tests/main_test.py +++ b/tests/main_test.py @@ -3,6 +3,7 @@ import sys import pytest import tempfile +import time import os from commit_check.main import ( StdinReader, @@ -14,6 +15,26 @@ FEATURE_TOPIC_BRANCH = "feature/topic" +@pytest.fixture(autouse=True) +def _stdin_gate_open(request, monkeypatch): + """Force the stdin readiness gate open for tests that fake piped input. + + Tests in this file simulate a pipe by mocking ``sys.stdin.read``. The + gate added to fix the idle-pipe hang consults ``select`` on the *real* + stdin, which under pytest is not readable — so those mocks would never + be reached. Opening the gate restores the semantics the mocks assume. + + Tests that exercise the gate itself opt out via ``real_stdin_gate``. + """ + if request.node.get_closest_marker("real_stdin_gate"): + yield + return + monkeypatch.setattr( + StdinReader, "_has_pending_data", staticmethod(lambda timeout=0.1: True) + ) + yield + + class TestMain: @pytest.mark.benchmark def test_help(self, capfd, monkeypatch): @@ -148,6 +169,51 @@ def test_read_piped_input_with_ioerror(self, mocker): result = reader.read_piped_input() assert result is None + @pytest.mark.real_stdin_gate + @pytest.mark.skipif(sys.platform == "win32", reason="select() needs POSIX") + def test_an_idle_open_pipe_returns_none_instead_of_hanging(self, monkeypatch): + """The bug this guards against was a hang, not a wrong value. + + Under some CI runners stdin is a pipe that is open but that nothing + will ever write to or close. ``read()`` there blocks forever, which + in a workflow is a stuck step rather than a failed one. + """ + read_fd, write_fd = os.pipe() + try: + with os.fdopen(read_fd, "r") as fake_stdin: + monkeypatch.setattr(sys, "stdin", fake_stdin) + start = time.monotonic() + result = StdinReader.read_piped_input() + elapsed = time.monotonic() - start + assert result is None + # ~0.1s select timeout; anything near a second means it blocked. + assert elapsed < 2 + finally: + os.close(write_fd) + + @pytest.mark.real_stdin_gate + @pytest.mark.skipif(sys.platform == "win32", reason="select() needs POSIX") + def test_piped_content_is_still_read(self, monkeypatch): + """Real piped input predates the exec, so the gate must let it through.""" + read_fd, write_fd = os.pipe() + os.write(write_fd, b"feat: add a thing\n") + os.close(write_fd) + with os.fdopen(read_fd, "r") as fake_stdin: + monkeypatch.setattr(sys, "stdin", fake_stdin) + assert StdinReader.read_piped_input() == "feat: add a thing" + + @pytest.mark.real_stdin_gate + @pytest.mark.skipif(sys.platform == "win32", reason="select() needs POSIX") + def test_dev_null_stdin_reads_as_nothing_promptly(self, monkeypatch): + """`< /dev/null` is immediate EOF: readable, empty, no hang.""" + with open(os.devnull, "r") as fake_stdin: + monkeypatch.setattr(sys, "stdin", fake_stdin) + start = time.monotonic() + result = StdinReader.read_piped_input() + elapsed = time.monotonic() - start + assert result is None + assert elapsed < 2 + class TestMainFunctionEdgeCases: """Test main function edge cases for better coverage.""" From ded73fc9a9a8f7170b623653fdde88250515d893 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Wed, 12 Aug 2026 08:13:34 +0000 Subject: [PATCH 2/5] feat: add --rev to check the commit at a named revision There was no way to tell commit-check which commit to check. Message checks read HEAD, and the author checks read the local git config before falling back to HEAD's author -- so CI could not iterate a pull request's commits, and a malformed author on any commit passed as long as the identity of whoever ran the check was valid. --rev REVISION names the commit under test. Message checks read that commit's message. Author checks read that commit's author and never the config: an existing commit's identity is a fact about the commit, not about the operator. The revision is verified up front, so a typo is a clear one-line error instead of a missing-message mystery deep in a validator. stdin is not consulted when --rev is given, and combining it with a message file is rejected -- both would name a second subject for the same checks. End-to-end tests run against a real two-commit repository where HEAD is clean and its parent carries both a bad message and a bad author: the verdict follows the revision, the author verdict flips even though the config identity stays valid, and JSON mode reports the named commit's values. --- commit_check/engine.py | 41 +++++++++++++++++++--- commit_check/main.py | 46 +++++++++++++++++++++---- tests/main_test.py | 77 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 10 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index 62dabf57..c7cb2ec6 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -56,6 +56,13 @@ class ValidationContext: no_banner: bool = False compact: bool = False push_upstream_fallback: bool = False + # A git revision naming the commit under test. When set, message and + # author checks read that commit -- the author is the commit's author, + # never the local git config, because an existing commit's identity is + # a fact about the commit rather than about whoever is running the + # check. The CLI verifies the revision resolves before it gets here. + # Last on purpose: positional construction predates it. + rev: str | None = None @dataclass @@ -152,11 +159,13 @@ def _should_skip_validation(self, context: ValidationContext) -> bool: """ Determine if validation should be skipped. - Skip only when there is no stdin_text, no commit_file, and no commits. + Skip only when there is no stdin_text, no commit_file, no rev, and + no commits. """ return ( context.stdin_text is None and context.commit_file is None + and context.rev is None and not has_commits() ) @@ -176,6 +185,10 @@ def _resolve_current_author(context: ValidationContext) -> str: (``get_commit_info("an")``), not the local git config which may belong to a different person. """ + if context.rev is not None: + # An explicit revision names an existing commit; its author is a + # fact about that commit, so the config never enters into it. + return get_commit_info("an", context.rev) if context.stdin_text is not None or context.commit_file is not None: return get_git_config_value("user.name") or get_commit_info("an") return get_commit_info("an") or get_git_config_value("user.name") @@ -194,7 +207,11 @@ def _message_was_supplied(context: ValidationContext) -> bool: genuinely empty, and rejecting that under allow_empty_commits = false is the verdict the rule exists to give. """ - return context.stdin_text is not None or context.commit_file is not None + return ( + context.stdin_text is not None + or context.commit_file is not None + or context.rev is not None + ) @staticmethod def _get_commit_message(context: ValidationContext) -> str: @@ -210,8 +227,12 @@ def _get_commit_message(context: ValidationContext) -> str: pass # Fallback to git log - subject = get_commit_info("s") - body = get_commit_info("b") + if context.rev is not None: + subject = get_commit_info("s", context.rev) + body = get_commit_info("b", context.rev) + else: + subject = get_commit_info("s") + body = get_commit_info("b") return f"{subject}\n\n{body}".strip() def _author_in_ignore_list(self, context: ValidationContext) -> bool: @@ -254,6 +275,8 @@ def _get_commit_body(context: ValidationContext) -> str: return f.read() except (OSError, IOError): pass + if context.rev is not None: + return get_commit_info("b", context.rev) return get_commit_info("b") def _should_skip_commit_validation(self, context: ValidationContext) -> bool: @@ -269,6 +292,7 @@ def _should_skip_commit_validation(self, context: ValidationContext) -> bool: return ( context.stdin_text is None and context.commit_file is None + and context.rev is None and not has_commits() ) @@ -359,6 +383,8 @@ def _get_subject(self, context: ValidationContext) -> str: except FileNotFoundError: pass + if context.rev is not None: + return get_commit_info("s", context.rev) return get_commit_info("s") def _validate_subject(self, _subject: str) -> ValidationResult: @@ -513,6 +539,13 @@ def _get_author_value(self, context: ValidationContext) -> str: "author_email": "ae", } + # An explicit revision names an existing commit, whose identity is a + # fact about the commit: read it from the commit and never from the + # config, which describes whoever happens to be running the check. + if context.rev is not None: + format_str = git_log_map.get(self.rule.check, "") + return get_commit_info(format_str, context.rev) if format_str else "" + # Try git config first (validates configured identity for new commits) config_key = git_config_map.get(self.rule.check, "") if config_key: diff --git a/commit_check/main.py b/commit_check/main.py index e18c4db7..a9473770 100644 --- a/commit_check/main.py +++ b/commit_check/main.py @@ -131,6 +131,16 @@ def _get_parser() -> argparse.ArgumentParser: help="path to config file (cchk.toml or commit-check.toml). If not specified, searches for config in: cchk.toml, commit-check.toml, .github/cchk.toml, .github/commit-check.toml", ) + parser.add_argument( + "--rev", + metavar="REVISION", + default=None, + help="check the commit at this git revision (e.g. HEAD^2, a SHA) " + "instead of HEAD or the working state. Message checks read that " + "commit's message; author checks read that commit's author, not " + "the local git config", + ) + parser.add_argument( "commit_msg_file", nargs="?", @@ -506,6 +516,24 @@ def main() -> int: if args.commit_msg_file: args.message = True + if args.rev: + if args.commit_msg_file: + parser.error( + "--rev and a commit message file both name the " + "thing to check; pass one or the other" + ) + # Fail here, with the revision named, rather than deep inside a + # validator where the error would surface as a missing message. + from commit_check.util import git_rev_parse_verify + + if not git_rev_parse_verify(args.rev): + print( + f"Error: --rev {args.rev!r} does not resolve to a commit " + "in this repository", + file=sys.stderr, + ) + return 1 + # Load and merge configuration from all sources: CLI > Env > TOML > Defaults config_data = ConfigMerger.from_all_sources(args, args.config) @@ -528,12 +556,17 @@ def main() -> int: filtered_rules = [rule for rule in all_rules if rule.check in requested_checks] engine = ValidationEngine(filtered_rules) - # Resolve validation context inputs - stdin_content, commit_file_path = _resolve_commit_message_source( - args, stdin_reader - ) - if not args.message: - stdin_content = _resolve_stdin_for_non_message(args, stdin_reader) + # Resolve validation context inputs. With --rev the commit itself is + # the thing under test, so stdin is never consulted: piping and a + # revision would name two different subjects for the same checks. + if args.rev: + stdin_content, commit_file_path = None, None + else: + stdin_content, commit_file_path = _resolve_commit_message_source( + args, stdin_reader + ) + if not args.message: + stdin_content = _resolve_stdin_for_non_message(args, stdin_reader) # Reset banner state for this run from commit_check.util import print_error_header as _peh @@ -543,6 +576,7 @@ def main() -> int: context = ValidationContext( stdin_text=stdin_content, commit_file=commit_file_path, + rev=args.rev, config=config_data, no_banner=getattr(args, "no_banner", False), compact=getattr(args, "compact", False), diff --git a/tests/main_test.py b/tests/main_test.py index 2ff084a3..fb864f01 100644 --- a/tests/main_test.py +++ b/tests/main_test.py @@ -215,6 +215,83 @@ def test_dev_null_stdin_reads_as_nothing_promptly(self, monkeypatch): assert elapsed < 2 +class TestRevOption: + """--rev names the commit under test, end to end on a real repository.""" + + @pytest.fixture + def two_commit_repo(self, tmp_path, monkeypatch): + """A repo whose HEAD is fine and whose first commit is not. + + The parent commit carries both a non-conventional message and a + deliberately malformed author, so checks that quietly read HEAD (or + the config) instead of the requested revision come out different. + """ + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True) + monkeypatch.chdir(tmp_path) + git = ["git", "-C", str(tmp_path)] + subprocess.run(git + ["config", "user.name", "Good Author"], check=True) + subprocess.run(git + ["config", "user.email", "good@example.com"], check=True) + subprocess.run( + git + + [ + "-c", + "user.name=bad", + "-c", + "user.email=nonsense", + "commit", + "-q", + "--allow-empty", + "-m", + "updated the parser", + ], + check=True, + ) + subprocess.run( + git + ["commit", "-q", "--allow-empty", "-m", "feat: add a thing"], + check=True, + ) + return tmp_path + + def test_rev_checks_the_named_commits_message(self, two_commit_repo, monkeypatch): + """HEAD passes, HEAD^ fails: the verdict must follow --rev.""" + monkeypatch.setattr("sys.argv", [CMD, "--message", "--rev", "HEAD"]) + assert main() == 0 + monkeypatch.setattr("sys.argv", [CMD, "--message", "--rev", "HEAD^"]) + assert main() == 1 + + def test_rev_reads_the_commits_author_not_the_config( + self, two_commit_repo, monkeypatch + ): + """The config identity is valid here, so a pass would mean the + config was consulted -- the revision's own author must decide.""" + monkeypatch.setattr("sys.argv", [CMD, "--author-email", "--rev", "HEAD^"]) + assert main() == 1 + monkeypatch.setattr("sys.argv", [CMD, "--author-email", "--rev", "HEAD"]) + assert main() == 0 + + def test_rev_that_does_not_resolve_is_a_clear_early_error( + self, two_commit_repo, monkeypatch, capsys + ): + monkeypatch.setattr("sys.argv", [CMD, "--message", "--rev", "no-such-ref"]) + assert main() == 1 + assert "does not resolve" in capsys.readouterr().err + + def test_rev_and_a_message_file_conflict(self, two_commit_repo, monkeypatch): + monkeypatch.setattr("sys.argv", [CMD, "--rev", "HEAD", "some-file.txt"]) + with pytest.raises(SystemExit) as excinfo: + main() + assert excinfo.value.code == 2 + + def test_rev_works_in_json_mode(self, two_commit_repo, monkeypatch, capsys): + monkeypatch.setattr( + "sys.argv", [CMD, "--message", "--rev", "HEAD^", "--format", "json"] + ) + assert main() == 1 + payload = json.loads(capsys.readouterr().out) + values = [c.get("value", "") for c in payload["checks"]] + assert any("updated the parser" in v for v in values) + + class TestMainFunctionEdgeCases: """Test main function edge cases for better coverage.""" From db0266d61cba2c2290bb67076c60fc5f41aaa031 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Wed, 12 Aug 2026 08:17:44 +0000 Subject: [PATCH 3/5] fix: say which checks were skipped instead of passing in silence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A silent skip is indistinguishable from a pass. That is how a merge commit at HEAD -- which is what every pull_request checkout points at -- let a bare 'commit-check --message' report success having judged nothing it was asked about. The subject rules' merge and fixup bypasses now return SKIP rather than PASS, matching what those bypasses mean: the rule declined to judge a machine-written subject, it did not approve it. The same goes for a message git never supplied. ignore_authors keeps its PASS there, since it judges the author and had already done so; a SKIP would wrongly read as the author having been bypassed. validate_all then prints one stderr line naming every skipped check: ⊘ skipped (nothing validated): subject-max-length, subject-min-length stderr so that nothing parsing stdout notices; exit codes are unchanged because a skip is still not a failure. JSON consumers already saw skip statuses; now the human running the text mode sees them too. --- commit_check/engine.py | 36 +++++++++++++++++++++++++++++------- tests/engine_test.py | 41 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index c7cb2ec6..95ca0f5b 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -396,9 +396,9 @@ class SubjectCapitalizationValidator(SubjectValidator): """Validates that subject starts with capital letter.""" def _validate_subject(self, subject: str) -> ValidationResult: - # Skip merge commits + # A merge subject is machine-written; the rule declines to judge it. if subject.lower().startswith("merge"): - return ValidationResult.PASS + return ValidationResult.SKIP # For conventional commits, check the description part after the colon import re @@ -434,9 +434,9 @@ class SubjectImperativeValidator(SubjectValidator): _INFLECTED = ("ed", "ing") def _validate_subject(self, subject: str) -> ValidationResult: - # Skip merge commits and fixup commits + # Merge and fixup subjects are machine-written; decline to judge them. if subject.lower().startswith(("merge", "fixup!")): - return ValidationResult.PASS + return ValidationResult.SKIP # Extract first word (ignore conventional commit prefixes) import re @@ -487,9 +487,9 @@ class SubjectLengthValidator(SubjectValidator): """Validates subject line length constraints.""" def _validate_subject(self, subject: str) -> ValidationResult: - # Skip merge commits for length checks + # A merge subject's length is git's doing, not the author's. if subject.lower().startswith("merge"): - return ValidationResult.PASS + return ValidationResult.SKIP length = len(subject) constraint_value = self.rule.value @@ -896,7 +896,14 @@ def validate(self, context: ValidationContext) -> ValidationResult: # never run. A message the caller supplied goes to the rule even when # it is empty; an empty one from git is still nothing to check. if not message and not self._message_was_supplied(context): - return ValidationResult.PASS + # ignore_authors delivered its verdict above -- it judges the + # author, so an absent message is no reason to disown it, and a + # SKIP here would wrongly read as "author was bypassed". + return ( + ValidationResult.PASS + if self.rule.check == "ignore_authors" + else ValidationResult.SKIP + ) self._checked_value = message @@ -1049,6 +1056,7 @@ def __init__(self, rules: list[ValidationRule]): def validate_all(self, context: ValidationContext) -> ValidationResult: """Run all validations and return overall result.""" results = [] + skipped: list[str] = [] for rule in self.rules: validator_class = self.VALIDATOR_MAP.get(rule.check) @@ -1060,6 +1068,20 @@ def validate_all(self, context: ValidationContext) -> ValidationResult: validator._compact = context.compact result = validator.validate(context) results.append(result) + if result == ValidationResult.SKIP: + skipped.append(rule.check.replace("_", "-")) + + if skipped: + # A skipped check validated nothing, and a silent skip is + # indistinguishable from a pass — which is how a merge commit at + # HEAD once let a whole run report success having read nothing. + # One line, stderr, so scripts parsing stdout are unaffected. + import sys + + print( + f"⊘ skipped (nothing validated): {', '.join(skipped)}", + file=sys.stderr, + ) # Return FAIL if any validation failed return ( diff --git a/tests/engine_test.py b/tests/engine_test.py index ffba921a..d6d8bd2a 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -705,13 +705,14 @@ def test_supplied_empty_message_reaches_the_empty_commit_rule(self): mock_commit_info.assert_not_called() def test_absent_message_still_skips_the_empty_commit_rule(self): - """A message git never supplied is nothing to check, not a failure.""" + """A message git never supplied is nothing to check -- and now the + status says so, instead of dressing the non-verdict up as a pass.""" rule = ValidationRule(check="allow_empty_commits", value=False) validator = CommitTypeValidator(rule) with patch("commit_check.engine.get_commit_info", return_value=""): with patch("commit_check.engine.has_commits", return_value=True): result = validator.validate(ValidationContext(no_banner=True)) - assert result == ValidationResult.PASS + assert result == ValidationResult.SKIP @pytest.mark.benchmark def test_commit_type_validator_merge_commits(self): @@ -1453,6 +1454,36 @@ def test_validation_engine_empty_rules(self): result = engine.validate_all(context) assert result == ValidationResult.PASS + def test_skipped_checks_are_named_on_stderr(self, capsys): + """A silent skip reads as a pass; the notice is what tells them apart. + + This is the CI trap in miniature: HEAD is a merge commit, the + subject rules decline to judge it, and before the notice existed the + run reported success having validated nothing it was asked about. + """ + rules = [ + ValidationRule(check="subject_imperative"), + ValidationRule(check="subject_max_length", value=80), + ] + engine = ValidationEngine(rules) + context = ValidationContext( + stdin_text="Merge branch 'main' into topic", no_banner=True + ) + + assert engine.validate_all(context) == ValidationResult.PASS + err = capsys.readouterr().err + assert "skipped" in err + assert "subject-imperative" in err + assert "subject-max-length" in err + + def test_no_notice_when_nothing_skipped(self, capsys): + rules = [ValidationRule(check="subject_max_length", value=80)] + engine = ValidationEngine(rules) + context = ValidationContext(stdin_text="feat: add a thing", no_banner=True) + + assert engine.validate_all(context) == ValidationResult.PASS + assert "skipped" not in capsys.readouterr().err + @pytest.mark.benchmark def test_validation_engine_unknown_validator_type(self): """Test ValidationEngine with unknown validator type.""" @@ -2672,5 +2703,7 @@ def test_a_noun_led_subject_now_passes(self): @pytest.mark.benchmark def test_merge_and_fixup_subjects_still_bypass_the_rule(self): - assert self._verdict("Merge branch 'main' into topic") == ValidationResult.PASS - assert self._verdict("fixup! fixed the parser") == ValidationResult.PASS + """Bypassed, and reported as bypassed: a machine-written subject is + not judged, and SKIP keeps that distinct from having passed.""" + assert self._verdict("Merge branch 'main' into topic") == ValidationResult.SKIP + assert self._verdict("fixup! fixed the parser") == ValidationResult.SKIP From 1483eb10b0982a0de139218a239cacda079c43c0 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Wed, 12 Aug 2026 08:27:17 +0000 Subject: [PATCH 4/5] fix: tighten merge bypass to git's literal prefix and reject empty --rev The subject rules bypassed anything starting with "merge" in any case, so an author's own "merge the parser tables" escaped judgement. Git writes "Merge " and "fixup! " exactly; only those forms are machine-written, so only those are declined now. --rev "" slipped past verification (an empty string is falsy) yet reached the engine, where git's fatal message leaked into the checked value with a green exit. Both rev sites now test against None, and the empty string fails early with the same clear error as a bad revision. Also rewords the skip notice to "not validated" and adds the tests codecov flagged as uncovered. --- commit_check/engine.py | 11 +++++++---- commit_check/main.py | 4 ++-- tests/engine_test.py | 39 +++++++++++++++++++++++++++++++++++++++ tests/main_test.py | 10 ++++++++++ 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index 95ca0f5b..96ac606d 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -397,7 +397,8 @@ class SubjectCapitalizationValidator(SubjectValidator): def _validate_subject(self, subject: str) -> ValidationResult: # A merge subject is machine-written; the rule declines to judge it. - if subject.lower().startswith("merge"): + # Git writes "Merge " exactly, so anything else is author prose. + if subject.startswith("Merge "): return ValidationResult.SKIP # For conventional commits, check the description part after the colon @@ -435,7 +436,9 @@ class SubjectImperativeValidator(SubjectValidator): def _validate_subject(self, subject: str) -> ValidationResult: # Merge and fixup subjects are machine-written; decline to judge them. - if subject.lower().startswith(("merge", "fixup!")): + # Git writes "Merge " and "fixup! " exactly, so anything else is + # author prose. + if subject.startswith(("Merge ", "fixup! ")): return ValidationResult.SKIP # Extract first word (ignore conventional commit prefixes) @@ -488,7 +491,7 @@ class SubjectLengthValidator(SubjectValidator): def _validate_subject(self, subject: str) -> ValidationResult: # A merge subject's length is git's doing, not the author's. - if subject.lower().startswith("merge"): + if subject.startswith("Merge "): return ValidationResult.SKIP length = len(subject) @@ -1079,7 +1082,7 @@ def validate_all(self, context: ValidationContext) -> ValidationResult: import sys print( - f"⊘ skipped (nothing validated): {', '.join(skipped)}", + f"⊘ skipped (not validated): {', '.join(skipped)}", file=sys.stderr, ) diff --git a/commit_check/main.py b/commit_check/main.py index a9473770..e1a65cc2 100644 --- a/commit_check/main.py +++ b/commit_check/main.py @@ -516,7 +516,7 @@ def main() -> int: if args.commit_msg_file: args.message = True - if args.rev: + if args.rev is not None: if args.commit_msg_file: parser.error( "--rev and a commit message file both name the " @@ -559,7 +559,7 @@ def main() -> int: # Resolve validation context inputs. With --rev the commit itself is # the thing under test, so stdin is never consulted: piping and a # revision would name two different subjects for the same checks. - if args.rev: + if args.rev is not None: stdin_content, commit_file_path = None, None else: stdin_content, commit_file_path = _resolve_commit_message_source( diff --git a/tests/engine_test.py b/tests/engine_test.py index d6d8bd2a..1a8f63b0 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -1454,6 +1454,45 @@ def test_validation_engine_empty_rules(self): result = engine.validate_all(context) assert result == ValidationResult.PASS + def test_rev_resolves_the_author_from_that_commit(self): + """With a rev, the ignore-list identity is the commit's author.""" + with patch( + "commit_check.engine.get_commit_info", return_value="Rev Author" + ) as info: + author = BaseValidator._resolve_current_author( + ValidationContext(rev="abc123") + ) + assert author == "Rev Author" + info.assert_called_once_with("an", "abc123") + + def test_rev_reads_the_body_from_that_commit(self): + """The body fallback follows the named revision, not HEAD.""" + rule = ValidationRule(check="require_body", value=True) + validator = BodyValidator(rule) + + def fake_info(fmt, sha="HEAD"): + assert sha == "abc123" + return "a body line" if fmt == "b" else "feat: subject" + + with patch("commit_check.engine.get_commit_info", side_effect=fake_info): + with patch("commit_check.engine.has_commits", return_value=True): + result = validator.validate(ValidationContext(rev="abc123")) + assert result == ValidationResult.PASS + + def test_capitalization_declines_to_judge_a_merge_subject(self): + rule = ValidationRule(check="subject_capitalized") + validator = SubjectCapitalizationValidator(rule) + context = ValidationContext(stdin_text="Merge branch 'x' into y") + assert validator.validate(context) == ValidationResult.SKIP + + def test_capitalization_judges_a_subject_that_merely_mentions_merge(self): + """Only git's exact "Merge " prefix is machine-written; a subject + that happens to start with the word in lowercase is author prose.""" + rule = ValidationRule(check="subject_capitalized") + validator = SubjectCapitalizationValidator(rule) + context = ValidationContext(stdin_text="merge the parser tables") + assert validator.validate(context) == ValidationResult.FAIL + def test_skipped_checks_are_named_on_stderr(self, capsys): """A silent skip reads as a pass; the notice is what tells them apart. diff --git a/tests/main_test.py b/tests/main_test.py index fb864f01..448a0804 100644 --- a/tests/main_test.py +++ b/tests/main_test.py @@ -276,6 +276,16 @@ def test_rev_that_does_not_resolve_is_a_clear_early_error( assert main() == 1 assert "does not resolve" in capsys.readouterr().err + def test_rev_empty_string_is_rejected_not_ignored( + self, two_commit_repo, monkeypatch, capsys + ): + """An empty --rev must hit the same early error as a bad one, not + fall through to the engine where git's own failure leaks into the + checked value with a green exit.""" + monkeypatch.setattr("sys.argv", [CMD, "--message", "--rev", ""]) + assert main() == 1 + assert "does not resolve" in capsys.readouterr().err + def test_rev_and_a_message_file_conflict(self, two_commit_repo, monkeypatch): monkeypatch.setattr("sys.argv", [CMD, "--rev", "HEAD", "some-file.txt"]) with pytest.raises(SystemExit) as excinfo: From 629e2437d43a87b6a1b185f364aab0ef5390b011 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Wed, 12 Aug 2026 08:33:46 +0000 Subject: [PATCH 5/5] test: cover the revision body path in the co-author and AI scans BodyValidator reads the full message, so the earlier test never touched _get_commit_body's rev branch; the attribution scan does. --- tests/engine_test.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/engine_test.py b/tests/engine_test.py index 1a8f63b0..c3a9441b 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -1479,6 +1479,21 @@ def fake_info(fmt, sha="HEAD"): result = validator.validate(ValidationContext(rev="abc123")) assert result == ValidationResult.PASS + def test_rev_scans_that_commits_body_for_ai_attribution(self): + """The attribution scan follows the named revision, not HEAD.""" + rule = ValidationRule(check="ai_attribution", value="forbid") + validator = AiAttributionValidator(rule) + validator._suppress_output = True + + def fake_info(fmt, sha="HEAD"): + assert sha == "abc123" + return "feat: subject\n\nCo-Authored-By: Claude " + + with patch("commit_check.engine.get_commit_info", side_effect=fake_info): + with patch("commit_check.engine.has_commits", return_value=True): + result = validator.validate(ValidationContext(rev="abc123")) + assert result == ValidationResult.FAIL + def test_capitalization_declines_to_judge_a_merge_subject(self): rule = ValidationRule(check="subject_capitalized") validator = SubjectCapitalizationValidator(rule)