diff --git a/commit_check/engine.py b/commit_check/engine.py index ff06ad40..39531f87 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,7 +147,7 @@ def _author_in_ignore_list(self, context: ValidationContext) -> bool: if not ignore_authors: return False - current_author = get_commit_info("an") + current_author = self._resolve_current_author(context) if current_author and current_author in ignore_authors: return True @@ -180,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 = get_commit_info("an") - 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 73abc644..ec4c4e64 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"]}} @@ -378,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") @@ -428,11 +486,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"]}} @@ -1293,6 +1355,109 @@ 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 + + @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).""" @@ -1747,7 +1912,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