From 3bc458c40c440d2b08895a016927d4cf0e14f4c2 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Tue, 7 Jul 2026 00:16:05 +0300 Subject: [PATCH 1/3] fix: prefer git config user.name for author validation in BaseValidator --- commit_check/engine.py | 9 +++++++-- tests/engine_test.py | 15 ++++++++++++--- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index ff06ad40..91061f18 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -127,7 +127,11 @@ def _author_in_ignore_list(self, context: ValidationContext) -> bool: if not ignore_authors: return False - current_author = get_commit_info("an") + # Use git config user.name (the committer's identity) first — for + # piped stdin or pre-commit scenarios there is no new commit yet, + # so the last commit's author may be unrelated (e.g. a bot) and + # would incorrectly suppress all validation. + current_author = get_git_config_value("user.name") or get_commit_info("an") if current_author and current_author in ignore_authors: return True @@ -180,7 +184,8 @@ def _should_skip_branch_validation(self, context: ValidationContext) -> bool: or if no stdin_text and no commits exist. """ ignore_authors = context.config.get("branch", {}).get("ignore_authors", []) - current_author = get_commit_info("an") + # Prefer git config user.name — same rationale as commit checks. + current_author = get_git_config_value("user.name") or get_commit_info("an") if current_author and current_author in ignore_authors: return True return context.stdin_text is None and not has_commits() diff --git a/tests/engine_test.py b/tests/engine_test.py index 73abc644..ac149aaa 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -245,14 +245,16 @@ def test_branch_validator_invalid_branch( assert result == ValidationResult.FAIL @patch("commit_check.engine.get_branch_name") + @patch("commit_check.engine.get_git_config_value") @patch("commit_check.engine.get_commit_info") @pytest.mark.benchmark def test_branch_validator_ignored_author( - self, mock_get_commit_info, mock_get_branch_name + self, mock_get_commit_info, mock_get_git_config_value, mock_get_branch_name ): """Test BranchValidator skips validation for ignored author.""" mock_get_branch_name.return_value = "invalid-branch-name" mock_get_commit_info.return_value = "ignored" + mock_get_git_config_value.return_value = "" rule = ValidationRule(check="branch", regex=r"^(feature|bugfix|hotfix)/.+") validator = BranchValidator(rule) config = {"branch": {"ignore_authors": ["ignored"]}} @@ -428,11 +430,15 @@ def test_author_validator_email_valid( assert mock_get_commit_info.call_args_list[0][0][0] == "an" assert mock_get_commit_info.call_args_list[2][0][0] == "ae" + @patch("commit_check.engine.get_git_config_value") @patch("commit_check.engine.get_commit_info") @pytest.mark.benchmark - def test_author_validator_ignored_author(self, mock_get_commit_info): + def test_author_validator_ignored_author( + self, mock_get_commit_info, mock_get_git_config_value + ): """Test AuthorValidator skips validation for ignored author.""" mock_get_commit_info.return_value = "ignored" + mock_get_git_config_value.return_value = "" rule = ValidationRule(check="author_name", regex=r"^[A-Z][a-z]+ [A-Z][a-z]+$") validator = AuthorValidator(rule) config = {"commit": {"ignore_authors": ["ignored"]}} @@ -1747,7 +1753,10 @@ def test_skip_when_author_ignored(self): config = {"commit": {"ignore_authors": ["bot-user"]}} context = ValidationContext(stdin_text=message, config=config) - with patch("commit_check.engine.get_commit_info", return_value="bot-user"): + with ( + patch("commit_check.engine.get_commit_info", return_value="bot-user"), + patch("commit_check.engine.get_git_config_value", return_value=""), + ): result = validator.validate(context) assert result == ValidationResult.PASS # Skipped due to ignored author From c7e818fd6151e05f6f0c1b191a62e8b7185ede81 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Tue, 7 Jul 2026 00:27:24 +0300 Subject: [PATCH 2/3] fix: make ignore_authors author resolution mode-aware to avoid regression The original fix replaced get_commit_info("an") unconditionally with get_git_config_value("user.name") or get_commit_info("an") in both _author_in_ignore_list and _should_skip_branch_validation. This fixed the reported bug (piped stdin silently skipped when HEAD is a bot) but introduced a regression: when validating an existing bot commit with no stdin, the local git config user.name (a human) was used instead of the commit's own author, so bots like dependabot/renovate were no longer recognized as ignored. Instead, add a mode-aware _resolve_current_author helper: - Prospective message (stdin or commit_file): resolve from git config user.name first, falling back to the last commit's author. The last commit's author is unrelated to the pending commit. - Existing commit (no stdin, no commit_file): resolve from the last commit's author first, falling back to git config. The commit being validated is the last commit. Fixes the reported bug without breaking dependabot/renovate skip. --- commit_check/engine.py | 29 +++++++--- tests/engine_test.py | 127 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 7 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index 91061f18..fd9be868 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -101,6 +101,26 @@ def _should_skip_validation(self, context: ValidationContext) -> bool: and not has_commits() ) + @staticmethod + def _resolve_current_author(context: ValidationContext) -> str: + """Resolve the relevant author identity based on validation mode. + + Two distinct modes: + + *Prospective message* (``stdin_text`` or ``commit_file`` is set): + the user is about to create a new commit. The last commit's author + is unrelated — the relevant identity is the local git config + (``user.name``), i.e. the person who will author the pending commit. + + *Existing commit* (no ``stdin_text``, no ``commit_file``): + the last commit is the one being validated. Use its own author + (``get_commit_info("an")``), not the local git config which may + belong to a different person. + """ + 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") + @staticmethod def _get_commit_message(context: ValidationContext) -> str: """Get commit message from context or git.""" @@ -127,11 +147,7 @@ def _author_in_ignore_list(self, context: ValidationContext) -> bool: if not ignore_authors: return False - # Use git config user.name (the committer's identity) first — for - # piped stdin or pre-commit scenarios there is no new commit yet, - # so the last commit's author may be unrelated (e.g. a bot) and - # would incorrectly suppress all validation. - current_author = get_git_config_value("user.name") or get_commit_info("an") + current_author = self._resolve_current_author(context) if current_author and current_author in ignore_authors: return True @@ -184,8 +200,7 @@ def _should_skip_branch_validation(self, context: ValidationContext) -> bool: or if no stdin_text and no commits exist. """ ignore_authors = context.config.get("branch", {}).get("ignore_authors", []) - # Prefer git config user.name — same rationale as commit checks. - current_author = get_git_config_value("user.name") or get_commit_info("an") + current_author = self._resolve_current_author(context) if current_author and current_author in ignore_authors: return True return context.stdin_text is None and not has_commits() diff --git a/tests/engine_test.py b/tests/engine_test.py index ac149aaa..8297501c 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -380,6 +380,62 @@ def test_validate_without_regex(self): result = validator.validate(context) assert result == ValidationResult.PASS + @pytest.mark.benchmark + def test_branch_ignored_author_uses_git_config_when_stdin(self): + """ + Bug-fix guard (branch side): when stdin is piped, the last commit's + author must NOT suppress branch-author skip logic. + """ + rule = ValidationRule(check="branch", regex=r"^feature/") + validator = BranchValidator(rule) + + config = {"branch": {"ignore_authors": ["pre-commit-ci[bot]"]}} + context = ValidationContext(stdin_text="feature/valid-branch", config=config) + + with ( + patch( + "commit_check.engine.get_commit_info", return_value="pre-commit-ci[bot]" + ), + patch( + "commit_check.engine.get_git_config_value", + return_value="Alice Developer", + ), + ): + result = validator.validate(context) + # Not skipped — Alice is not in ignore_authors for branches + assert result == ValidationResult.PASS # branch name is valid + + @pytest.mark.benchmark + def test_branch_ignored_author_uses_commit_author_when_no_stdin(self): + """ + Regression guard (branch side): when validating the current branch + (no stdin), the check must use the last commit's author for + ignore_authors, not the local git config. + """ + rule = ValidationRule(check="branch", regex=r"^feature/") + validator = BranchValidator(rule) + + config = {"branch": {"ignore_authors": ["dependabot[bot]"]}} + context = ValidationContext(config=config) + + with ( + patch("commit_check.engine.has_commits", return_value=True), + patch( + "commit_check.engine.get_branch_name", + return_value="dependabot/go-mod-upgrade", + ), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + patch( + "commit_check.engine.get_git_config_value", + return_value="Alice Developer", + ), + ): + result = validator.validate(context) + # Skipped — the commit's author (dependabot[bot]) is in ignore_authors + assert result == ValidationResult.PASS + class TestAuthorValidator: @patch("commit_check.engine.has_commits") @@ -1299,6 +1355,77 @@ def test_co_author_in_ignore_list_from_commit_file(self): finally: os.unlink(commit_file) + @pytest.mark.benchmark + def test_author_in_ignore_list_uses_git_config_when_stdin(self): + """ + Bug-fix guard: when stdin is piped, the last commit's author + (e.g. a bot in the ignore list) must NOT suppress validation. + The check should use the local git config user.name instead. + """ + rule = ValidationRule( + check="message", + regex=CONVENTIONAL_COMMIT_REGEX, + error=BAD_COMMIT_MSG, + suggest=USE_CONVENTIONAL_FORMAT, + ) + validator = CommitMessageValidator(rule) + + # HEAD author is "pre-commit-ci[bot]" (in ignore list) + # but local git config user.name is a human (not ignored) + # stdin is a proper conventional commit — validation should run. + message = "fix: resolve edge case in parser" + config = {"commit": {"ignore_authors": ["pre-commit-ci[bot]"]}} + context = ValidationContext(stdin_text=message, config=config) + + with ( + patch( + "commit_check.engine.get_commit_info", return_value="pre-commit-ci[bot]" + ), + patch( + "commit_check.engine.get_git_config_value", + return_value="Alice Developer", + ), + ): + result = validator.validate(context) + # Not skipped — Alice is not in ignore_authors, so validation runs + assert result == ValidationResult.PASS # message is valid + + @pytest.mark.benchmark + def test_author_in_ignore_list_uses_commit_author_when_no_stdin(self): + """ + Regression guard: when validating an existing commit (no stdin), + the check must use the commit's own author, not the local git config. + A bot commit should still be skipped when its author is ignore_authors, + even if user.name is a human. + """ + rule = ValidationRule( + check="message", + regex=CONVENTIONAL_COMMIT_REGEX, + error=BAD_COMMIT_MSG, + suggest=USE_CONVENTIONAL_FORMAT, + ) + validator = CommitMessageValidator(rule) + + # HEAD author is "dependabot[bot]" (in ignore list) + # local git config user.name is a human (not ignored) + # no stdin — validating the last commit as-is. + config = {"commit": {"ignore_authors": ["dependabot[bot]"]}} + context = ValidationContext(config=config) + + with ( + patch("commit_check.engine.has_commits", return_value=True), + patch( + "commit_check.engine.get_commit_info", return_value="dependabot[bot]" + ), + patch( + "commit_check.engine.get_git_config_value", + return_value="Alice Developer", + ), + ): + result = validator.validate(context) + # Skipped — the commit's author (dependabot[bot]) is in ignore_authors + assert result == ValidationResult.PASS + class TestGetGitConfigValue: """Tests for the AuthorValidator using git config (Issue #298).""" From 1f3ec2a3fa0acfeb394225522eed1864cfa056f3 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Sun, 12 Jul 2026 22:12:13 +0300 Subject: [PATCH 3/3] fix: skip unnecessary subprocess call in _should_skip_branch_validation fix: add test for _resolve_current_author fallback path (coverage) --- commit_check/engine.py | 7 ++++--- tests/engine_test.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/commit_check/engine.py b/commit_check/engine.py index fd9be868..39531f87 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -200,9 +200,10 @@ def _should_skip_branch_validation(self, context: ValidationContext) -> bool: or if no stdin_text and no commits exist. """ ignore_authors = context.config.get("branch", {}).get("ignore_authors", []) - current_author = self._resolve_current_author(context) - if current_author and current_author in ignore_authors: - return True + if ignore_authors: + current_author = self._resolve_current_author(context) + if current_author and current_author in ignore_authors: + return True return context.stdin_text is None and not has_commits() def _print_failure(self, actual_value: str, regex_or_constraint: str = "") -> None: diff --git a/tests/engine_test.py b/tests/engine_test.py index 8297501c..ec4c4e64 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -1426,6 +1426,38 @@ def test_author_in_ignore_list_uses_commit_author_when_no_stdin(self): # Skipped — the commit's author (dependabot[bot]) is in ignore_authors assert result == ValidationResult.PASS + @pytest.mark.benchmark + def test_author_in_ignore_list_falls_back_to_git_config_when_commit_info_empty( + self, + ): + """ + Coverage guard: when no stdin/commit_file and get_commit_info("an") + returns empty, _resolve_current_author must fall back to + get_git_config_value("user.name"). + """ + rule = ValidationRule( + check="message", + regex=CONVENTIONAL_COMMIT_REGEX, + error=BAD_COMMIT_MSG, + suggest=USE_CONVENTIONAL_FORMAT, + ) + validator = CommitMessageValidator(rule) + + config = {"commit": {"ignore_authors": ["Developer Bot"]}} + context = ValidationContext(config=config) + + with ( + patch("commit_check.engine.has_commits", return_value=True), + patch("commit_check.engine.get_commit_info", return_value=""), + patch( + "commit_check.engine.get_git_config_value", + return_value="Developer Bot", + ), + ): + result = validator.validate(context) + # Skipped — fallback author (Developer Bot) is in ignore_authors + assert result == ValidationResult.PASS + class TestGetGitConfigValue: """Tests for the AuthorValidator using git config (Issue #298)."""