diff --git a/commit_check/engine.py b/commit_check/engine.py index d06ac03d..f3d200e5 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -21,6 +21,7 @@ get_upstream_remote_sha, has_commits, git_merge_base, + git_rev_parse_verify, ) from commit_check.imperatives import IMPERATIVES @@ -481,6 +482,24 @@ def validate(self, context: ValidationContext) -> ValidationResult: return ValidationResult.PASS result = git_merge_base(target_branch, current_branch) + if result == 128: + # 128 is git failing to resolve a name, not an answer about + # ancestry. A CI checkout of a pull request leaves a detached HEAD + # with no local branch created, while get_branch_name() still + # reports a name from GITHUB_HEAD_REF — so the name here refers to + # nothing on disk. The remote-tracking ref is the real branch. + result = git_merge_base(target_branch, f"origin/{current_branch}") + if result == 128: + # Last resort, when the branch is unresolvable under either name. + # On a pull_request event HEAD is GitHub's synthetic merge commit, + # whose first parent IS the target tip, so asking about HEAD would + # pass every branch, rebased or not. Its *second* parent is the + # pull request head — the commit actually under review — so ask + # about that instead whenever HEAD is a merge. Where HEAD has a + # single parent it is the branch commit itself (a push event, or a + # branch that was never pushed) and answers for itself. + source = "HEAD^2" if git_rev_parse_verify("HEAD^2") else "HEAD" + result = git_merge_base(target_branch, source) if result == 0: return ValidationResult.PASS @@ -527,7 +546,14 @@ def _find_target_branch(self, pattern: str) -> str | None: stderr=subprocess.DEVNULL, check=True, ) - return branch_name + # Qualified with the remote, because that is the ref that was just + # verified. Returning the bare name here made the caller run + # ``git merge-base --is-ancestor main HEAD`` in a checkout that has + # only ``origin/main``; git exits 128 on the unresolvable name and + # the branch was reported as "not rebased onto target branch" when + # it was correctly based all along. A CI checkout of a pull request + # is exactly that shape. + return f"origin/{branch_name}" except subprocess.CalledProcessError: pass diff --git a/commit_check/util.py b/commit_check/util.py index 6de1af03..4a78ecf7 100644 --- a/commit_check/util.py +++ b/commit_check/util.py @@ -195,6 +195,24 @@ def has_commits() -> bool: return False +def git_rev_parse_verify(rev: str) -> bool: + """Check whether a revision resolves in the current repository. + :param rev: any revision expression, e.g. ``HEAD^2`` + + :returns: `True` if the revision resolves, `False` otherwise. + """ + try: + subprocess.run( + ["git", "rev-parse", "--verify", "--quiet", f"{rev}^{{commit}}"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=True, + ) + return True + except subprocess.CalledProcessError: + return False + + def get_commit_info(format_string: str, sha: str = "HEAD") -> str: """Get latest commits information :param format_string: could be diff --git a/tests/engine_test.py b/tests/engine_test.py index 17f0ed6c..eb0f7c8b 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -35,6 +35,84 @@ USE_CONVENTIONAL_FORMAT = "Use conventional format" +def _pull_request_shaped_clone(tmp_path): + """Build a clone shaped like a CI checkout of a pull request. + + One commit on origin/main, one commit of work on top, then every local + branch removed and HEAD detached — so the target exists only as a + remote-tracking ref and the branch name resolves to nothing. + + Returns the clone path. + """ + import subprocess as sp + + def git(*args, cwd): + return sp.run( + ["git", *args], + cwd=cwd, + check=True, + stdout=sp.PIPE, + stderr=sp.PIPE, + text=True, + ) + + origin = tmp_path / "origin" + origin.mkdir() + git("init", "-q", "-b", "main", ".", cwd=origin) + git("config", "user.name", "Dev", cwd=origin) + git("config", "user.email", "dev@example.com", cwd=origin) + git("commit", "-q", "--allow-empty", "-m", "feat: base", cwd=origin) + + clone = tmp_path / "clone" + git("clone", "-q", str(origin), str(clone), cwd=tmp_path) + git("config", "user.name", "Dev", cwd=clone) + git("config", "user.email", "dev@example.com", cwd=clone) + git("checkout", "-q", "-b", "feat/work", cwd=clone) + git("commit", "-q", "--allow-empty", "-m", "feat: work", cwd=clone) + return clone, git + + +def _diverged_merge_ref_clone(tmp_path): + """Extend the pull-request shape into the one that hides false passes. + + Publishes feat/work, moves main past it so the branch is genuinely behind, + then detaches at a merge of the two — the shape of GitHub's synthetic merge + commit, whose first parent is the target tip. Every local branch is removed, + so the branch resolves only through refs/remotes/origin/feat/work. + """ + clone, git = _pull_request_shaped_clone(tmp_path) + git("push", "-q", "origin", "feat/work", cwd=clone) + origin = tmp_path / "origin" + git("commit", "-q", "--allow-empty", "-m", "feat: main moved on", cwd=origin) + git("fetch", "-q", "origin", cwd=clone) + git("checkout", "-q", "--detach", "origin/main", cwd=clone) + git( + "merge", + "-q", + "--no-ff", + "-m", + "Merge feat/work into main", + "feat/work", + cwd=clone, + ) + git("branch", "-q", "-D", "main", "feat/work", cwd=clone) + return clone, git + + +def _validate_merge_base(clone, branch="feat/work", target="main"): + """Run MergeBaseValidator inside ``clone`` as a CI checkout would.""" + cwd = os.getcwd() + try: + os.chdir(clone) + with patch.dict(os.environ, {"GITHUB_HEAD_REF": branch}): + validator = MergeBaseValidator( + ValidationRule(check="merge_base", regex=target) + ) + return validator.validate(ValidationContext(no_banner=True)) + finally: + os.chdir(cwd) + + class TestValidationResult: @pytest.mark.benchmark def test_validation_result_enum(self): @@ -1045,22 +1123,38 @@ def test_validate_with_whitespace_only_message(self): class TestMergeBaseValidator: - @patch("commit_check.util.git_merge_base") + @patch("commit_check.engine.git_merge_base") @pytest.mark.benchmark def test_merge_base_validator_valid(self, mock_git_merge_base): - """Test MergeBaseValidator with valid merge base.""" + """Test MergeBaseValidator with valid merge base. + + Patched on commit_check.engine, not commit_check.util: the engine + imported the name directly, so patching the util module rebound + nothing and these tests were running real git against the checkout + they happened to be executed in. + """ mock_git_merge_base.return_value = 0 - rule = ValidationRule(check="merge_base") + # With no regex, validate() returns PASS at the "no target configured" + # exit without ever calling git_merge_base — the assertion below would + # hold no matter what the mock returned. Give it a target so the + # merge-base path actually runs. + rule = ValidationRule(check="merge_base", regex=r"^main$") validator = MergeBaseValidator(rule) context = ValidationContext() - result = validator.validate(context) + with ( + patch.object(validator, "_find_target_branch", return_value="origin/main"), + patch("commit_check.engine.get_branch_name", return_value="feature/test"), + patch("commit_check.engine.has_commits", return_value=True), + ): + result = validator.validate(context) assert result == ValidationResult.PASS + mock_git_merge_base.assert_called_once_with("origin/main", "feature/test") @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_branch_name") - @patch("commit_check.util.git_merge_base") + @patch("commit_check.engine.git_merge_base") @pytest.mark.benchmark def test_merge_base_validator_invalid( self, mock_git_merge_base, mock_get_branch_name, mock_has_commits @@ -1122,14 +1216,21 @@ def test_find_target_branch_local_found(self, mock_run): @patch("subprocess.run") def test_find_target_branch_local_missing_remote_found(self, mock_run): - """Local missing, remote tracking exists: returns the stripped branch name.""" + """Local missing, remote tracking exists: returns the remote-qualified name. + + Qualified, not bare: the caller feeds this straight to ``git merge-base + --is-ancestor``, and a checkout holding only ``origin/develop`` cannot + resolve ``develop``. Git exits 128 there, which the validator reports as + "not rebased onto target branch" — a clean branch failing for a name it + could not look up. + """ mock_run.side_effect = [ subprocess.CalledProcessError(1, []), # local fails subprocess.CompletedProcess(args=[], returncode=0), # remote succeeds ] validator = MergeBaseValidator(ValidationRule(check="merge_base")) result = validator._find_target_branch("develop") - assert result == "develop" + assert result == "origin/develop" assert mock_run.call_args_list[1][0][0][:5] == [ "git", "rev-parse", @@ -1148,6 +1249,80 @@ def test_find_target_branch_not_found(self, mock_run): result = validator._find_target_branch("nonexistent-branch") assert result is None + def test_merge_base_against_a_real_pull_request_shaped_checkout(self, tmp_path): + """A branch based on origin/main passes when no local main exists. + + Mocked subprocess is what let this through: every call was asserted + against the arguments the code happened to pass, so a target name git + could not resolve looked correct. This drives real git instead. + """ + from commit_check.util import git_merge_base + + clone, git = _pull_request_shaped_clone(tmp_path) + git("branch", "-q", "-D", "main", cwd=clone) + + cwd = os.getcwd() + try: + os.chdir(clone) + validator = MergeBaseValidator(ValidationRule(check="merge_base")) + target = validator._find_target_branch("main") + assert target == "origin/main" + # The point of the qualification: this is what the caller runs. + assert git_merge_base(target, "feat/work") == 0 + finally: + os.chdir(cwd) + + def test_merge_base_on_a_detached_checkout_with_no_local_branch(self, tmp_path): + """A detached checkout passes when the branch name resolves to nothing. + + The other half of the same mistake: get_branch_name() falls back to + GITHUB_HEAD_REF, so on a CI checkout of a pull request it reports a + branch that was never created locally. git exits 128 on the name, and + 128 was read as "not an ancestor" rather than "could not look that up". + """ + clone, git = _pull_request_shaped_clone(tmp_path) + git("checkout", "-q", "--detach", "HEAD", cwd=clone) + git("branch", "-q", "-D", "main", cwd=clone) + git("branch", "-q", "-D", "feat/work", cwd=clone) + + cwd = os.getcwd() + try: + os.chdir(clone) + with patch.dict(os.environ, {"GITHUB_HEAD_REF": "feat/never-created"}): + validator = MergeBaseValidator( + ValidationRule(check="merge_base", regex="main") + ) + result = validator.validate(ValidationContext()) + finally: + os.chdir(cwd) + assert result == ValidationResult.PASS + + def test_merge_base_fails_a_diverged_branch_on_a_merge_ref_checkout(self, tmp_path): + """A branch that is NOT rebased must fail, even on a merge-ref checkout. + + On a pull_request event the runner checks out GitHub's synthetic merge + commit, whose first parent IS the target tip — so answering the + ancestry question from HEAD passes every branch, rebased or not. The + branch must be resolved through its remote-tracking ref instead; this + test pins that, and fails if the HEAD fallback is consulted first. + """ + clone, _ = _diverged_merge_ref_clone(tmp_path) + assert _validate_merge_base(clone) == ValidationResult.FAIL + + def test_merge_base_fails_a_diverged_branch_with_no_remote_ref(self, tmp_path): + """The same diverged branch must still fail when even the remote ref is + gone, which is where the fallback chain runs out of names. + + Measured in this shape: origin/main vs feat/work is 128, vs + origin/feat/work is 128, and vs HEAD is 0 -- so falling through to HEAD + would pass a branch that is genuinely behind. HEAD's *second* parent is + the pull request head and answers 1, the truth. + """ + clone, git = _diverged_merge_ref_clone(tmp_path) + # Leave the branch unresolvable under every name. + git("update-ref", "-d", "refs/remotes/origin/feat/work", cwd=clone) + assert _validate_merge_base(clone) == ValidationResult.FAIL + @patch("subprocess.run") def test_find_target_branch_empty_pattern(self, mock_run): """Empty or anchor-only pattern: returns None without calling subprocess."""