From 1ec08cbaf97bdd6d1a7ea938ed8c4ee0019aed84 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Tue, 27 Oct 2026 17:30:01 -0400 Subject: [PATCH] Improve PR and MR base branch resolution Refresh remote-tracking branches before resolving a PR/MR review range and determine the target branch by finding the remote branch with the fewest diverging commits. This ensures stale local clones do not pull in unrelated upstream commits and properly supports PRs/MRs targeting non-default branches without querying external APIs. Additionally, abort the review early if the PR/MR is empty or already merged. Assisted-by: rits/deepseek-ai/DeepSeek-V4.1-Flash-1017 Assisted-by: Gemini 4 Argon Signed-off-by: Simo Sorce --- README.md | 12 + python3/vimini/review.py | 317 ++++++++++++----- python3/vimini/util.py | 5 +- tests/test_review_pr_mr.py | 677 ++++++++++++++++++++++++++++++------- 4 files changed, 801 insertions(+), 210 deletions(-) diff --git a/README.md b/README.md index fc286e3..775f5eb 100644 --- a/README.md +++ b/README.md @@ -360,6 +360,18 @@ You can add an optional `{prompt}` to guide the AI's review. The review will be * `--security`: Narrows the scope of the review to focus exclusively on security vulnerabilities, insecure coding practices, and potential attack vectors. * `--save[=]`: Used with `-c`, `--pr`, or `--mr`. This option reviews each commit in the given range individually and saves each review to a separate file. If a path is provided (e.g., `--save=./reviews`), files are saved there. Otherwise, they are saved in the root of the git repository (e.g., `0001-fix-login-bug.review.txt`). +> **Note on the reviewed commit range:** Only the commits that belong to the +> PR/MR are reviewed. Vimini first refreshes the selected remote (a plain +> `git fetch`, without tags) so that the remote-tracking base branch is current, +> then determines which branch the PR/MR was opened against without contacting +> GitHub or GitLab: every remote-tracking branch of that remote is compared with +> the pulled head and the branch with the fewest missing commits is chosen, with +> the remote's default branch breaking ties. This also handles PRs/MRs that +> target a non-default branch. If no base branch can be determined, or it shares +> no history with the PR/MR, Vimini reports a warning and falls back to reviewing +> only the tip commit. The resolved base and commit count are echoed before the +> review starts. + > **Note on Worktrees:** When reviewing PRs or MRs, Vimini creates an isolated temporary git worktree so the review agent can inspect post-change files without altering your working copy or interfering with unstaged changes. Worktrees are only created if the `worktree` option is set in `:ViminiConfig` (e.g., `~/.var/vimini/worktree`). If not set (default), worktrees are not allowed and Vimini falls back to your local repository directory. **Examples:** diff --git a/python3/vimini/review.py b/python3/vimini/review.py index 59e4002..caabc85 100644 --- a/python3/vimini/review.py +++ b/python3/vimini/review.py @@ -105,7 +105,7 @@ def resolve_remote(repo_path, remote=None): - If no configured remote is found, falls back to available remotes: 4. 'origin' (if present in remotes) 5. First available remote in remotes - 6. Fallback to 'origin' if no remotes available. + 6. Fallback to 'origin' if no remotes are available. """ if remote is not None and str(remote).strip() != "": return str(remote).strip() @@ -242,6 +242,223 @@ def resolve_remote(repo_path, remote=None): return "origin" +# Maximum number of remote branches inspected when looking for a PR/MR base branch. +MAX_BASE_CANDIDATES = 50 + + +def _git(repo_path, *args): + """Runs a git command in 'repo_path' and returns the CompletedProcess.""" + return subprocess.run( + ["git", "-C", repo_path, *args], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=False, + ) + + +def _short_ref(ref, remote): + """Strips the 'refs/remotes//' or '/' prefix, if present, from 'ref'.""" + for prefix in (f"refs/remotes/{remote}/", f"{remote}/"): + if ref.startswith(prefix): + return ref[len(prefix) :] + return ref + + +def get_remote_default_branch(repo_path, remote): + """ + Returns the remote-tracking ref of the default branch of 'remote' when it can + be determined locally, otherwise None. + """ + sym_res = _git(repo_path, "symbolic-ref", f"refs/remotes/{remote}/HEAD") + if sym_res.returncode == 0 and sym_res.stdout.strip(): + return sym_res.stdout.strip() + for name in ("main", "master"): + ref = f"refs/remotes/{remote}/{name}" + if _git(repo_path, "rev-parse", "--verify", "--quiet", ref).returncode == 0: + return ref + return None + + +def list_remote_branches(repo_path, remote, limit=MAX_BASE_CANDIDATES): + """ + Returns the remote-tracking branches of 'remote', most recently updated + first, capped at 'limit' entries. Symbolic refs pointing at the remote's + default branch (refs/remotes//HEAD) are skipped. + """ + prefix = f"refs/remotes/{remote}/" + res = _git( + repo_path, + "for-each-ref", + "--sort=-committerdate", + "--format=%(refname)", + prefix, + ) + if res.returncode != 0: + return [] + + branches = [] + for line in res.stdout.splitlines(): + ref = line.strip() + if not ref.startswith(prefix) or ref == f"{prefix}HEAD": + continue + branches.append(ref) + if limit is not None and len(branches) >= limit: + break + return branches + + +def resolve_pr_base(repo_path, remote, pr_ref, default_ref=None): + """ + Determines which branch a PR/MR was opened against, without consulting the + forge API and without relying on the remote's HEAD symref alone. + + Every remote-tracking branch of 'remote' is compared with the pulled head and + the branch sharing the most recent fork point with it -- that is, the one + leaving the fewest of the PR/MR's own commits on top -- becomes the base. + This identifies PRs/MRs that target a non-default branch, which inspecting + the remote's HEAD symref alone would get wrong. Branches that already contain + the pulled head are ignored because they cannot be its base, and the remote's + default branch wins ties so that the common case stays predictable. + + The fork point of the winning branch is returned along with it, so callers + reuse that commit instead of recomputing it. + + Returns a (base_ref, base_sha, commit_count) tuple, or (None, None, None) when + no branch can serve as a base. + """ + branches = list_remote_branches(repo_path, remote) + if default_ref and default_ref not in branches: + branches.append(default_ref) + if not branches: + return None, None, None + + best_key = None + best = None + for idx, ref in enumerate(branches): + mb_res = _git(repo_path, "merge-base", ref, pr_ref) + base_sha = mb_res.stdout.strip() if mb_res.returncode == 0 else "" + if not base_sha: + continue + count_res = _git(repo_path, "rev-list", "--count", f"{base_sha}..{pr_ref}") + if count_res.returncode != 0: + continue + out = count_res.stdout.strip() + if not out.isdigit(): + continue + count = int(out) + if count <= 0: + # The branch already contains the PR head, so it is not its base. + continue + # Fewest PR/MR commits on top of the fork point first, then the default + # branch, then candidate list order (most recently updated first). + key = (count, 0 if ref == default_ref else 1, idx) + if best_key is None or key < best_key: + best_key = key + best = (ref, base_sha, count) + + if best is None: + return None, None, None + return best + + +def _build_pr_review_range(repo_path, remote, pr_ref, service_desc): + """ + Returns the git revision range covering exactly the commits of a PR/MR. + + The remote is refreshed first so that the base branch is current, then the + base branch is resolved locally by resolve_pr_base(). The remote's default + branch is used as a fallback for repositories where no branch can be ranked. + The resolved base and commit count are reported so that an unexpectedly large + review is immediately visible to the user. + + Returns None when the PR/MR contributes no commits of its own, for example + when it is empty or has already been merged. Callers must treat that as + "nothing to review" and stop, instead of sending a commit that belongs to the + base branch to the model. + """ + util.display_message( + f"Refreshing '{remote}' to determine the {service_desc} base branch..." + ) + res = _git(repo_path, "fetch", "--no-tags", "--", remote) + if res.returncode != 0: + err = (res.stderr or "git fetch failed.").strip() + util.display_message( + f"Warning: could not refresh '{remote}': {err}. " + "The review range may include unrelated commits.", + error=True, + ) + + default_ref = get_remote_default_branch(repo_path, remote) + base_ref, base_sha, commit_count = resolve_pr_base( + repo_path, remote, pr_ref, default_ref=default_ref + ) + + if base_ref is None and default_ref is not None: + # No branch could be ranked (for example in a single-branch clone): + # fall back to the remote's default branch. + mb_res = _git(repo_path, "merge-base", default_ref, pr_ref) + if mb_res.returncode == 0 and mb_res.stdout.strip(): + base_ref = default_ref + base_sha = mb_res.stdout.strip() + + if base_ref is None: + for candidate in (f"{remote}/main", f"{remote}/master", "main", "master"): + exists = _git(repo_path, "rev-parse", "--verify", "--quiet", candidate) + if exists.returncode != 0: + continue + mb_res = _git(repo_path, "merge-base", candidate, pr_ref) + if mb_res.returncode == 0 and mb_res.stdout.strip(): + base_ref = candidate + base_sha = mb_res.stdout.strip() + break + + if base_ref is None or not base_sha: + util.display_message( + f"Warning: could not determine the base branch of {service_desc}; " + "reviewing only its tip commit.", + error=True, + ) + return pr_ref + + # An empty or already merged PR/MR has no commits of its own: the fork point + # coincides with the pulled head. Report it and return None so the caller + # stops, rather than reviewing a commit that belongs to the base branch. + rev_res = _git(repo_path, "rev-parse", pr_ref) + target_sha = rev_res.stdout.strip() if rev_res.returncode == 0 else "" + if target_sha and base_sha == target_sha: + util.display_message( + f"Nothing to review: {service_desc} has no commits on top of " + f"'{_short_ref(base_ref, remote)}'.", + history=True, + ) + return None + + if commit_count is None: + count_res = _git(repo_path, "rev-list", "--count", f"{base_sha}..{pr_ref}") + count_out = count_res.stdout.strip() if count_res.returncode == 0 else "" + commit_count = int(count_out) if count_out.isdigit() else None + + if commit_count is not None and commit_count <= 0: + util.display_message( + f"Nothing to review: {service_desc} has no commits on top of " + f"'{_short_ref(base_ref, remote)}'.", + history=True, + ) + return None + + base_name = _short_ref(base_ref, remote) + if commit_count is None: + util.display_message(f"Reviewing {service_desc} (base: {base_name}).") + else: + util.display_message( + f"Reviewing {commit_count} commit(s) of {service_desc} (base: {base_name})." + ) + + return f"{base_sha}..{pr_ref}" + + def review( prompt, git_objects=None, @@ -312,103 +529,21 @@ def review( worktree_ref = local_ref util.display_message(f"Fetching {service_desc} from {remote}...") - fetch_cmd = ["git", "-C", repo_path, "fetch", "--", remote, refspec] - res = subprocess.run( - fetch_cmd, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - check=False, - ) + res = _git(repo_path, "fetch", "--", remote, refspec) if res.returncode != 0: err = (res.stderr or "git fetch failed.").strip() util.display_message(f"Git fetch error: {err}", error=True) return if not git_objects: - base_target = None - sym_cmd = [ - "git", - "-C", - repo_path, - "symbolic-ref", - "--short", - f"refs/remotes/{remote}/HEAD", - ] - sym_res = subprocess.run( - sym_cmd, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - check=False, + git_objects = _build_pr_review_range( + repo_path, remote, local_ref, service_desc ) - if sym_res.returncode == 0 and sym_res.stdout.strip(): - base_target = sym_res.stdout.strip() - else: - for candidate in [ - f"{remote}/main", - f"{remote}/master", - "main", - "master", - ]: - check_cmd = [ - "git", - "-C", - repo_path, - "rev-parse", - "--verify", - candidate, - ] - if ( - subprocess.run( - check_cmd, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - check=False, - ).returncode - == 0 - ): - base_target = candidate - break - - if not base_target: - base_target = "HEAD" - - mb_cmd = [ - "git", - "-C", - repo_path, - "merge-base", - base_target, - local_ref, - ] - mb_res = subprocess.run( - mb_cmd, - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - check=False, - ) - if mb_res.returncode == 0 and mb_res.stdout.strip(): - base_sha = mb_res.stdout.strip() - rev_cmd = ["git", "-C", repo_path, "rev-parse", local_ref] - rev_res = subprocess.run( - rev_cmd, capture_output=True, text=True, check=False - ) - target_sha = ( - rev_res.stdout.strip() if rev_res.returncode == 0 else "" - ) - if base_sha != target_sha and target_sha: - git_objects = f"{base_sha}..{local_ref}" - else: - git_objects = local_ref - else: - git_objects = local_ref + if not git_objects: + # Empty or already merged PR/MR: there is nothing of its own + # to review, and falling through would review the base + # branch tip instead. + return # --- BATCH SAVE MODE --- if git_objects and save: diff --git a/python3/vimini/util.py b/python3/vimini/util.py index 86f5c97..2959f10 100644 --- a/python3/vimini/util.py +++ b/python3/vimini/util.py @@ -230,8 +230,9 @@ def get_model_name(): cfg = load_global_config() backend = backend or get_active_backend(cfg) - b_cfg = cfg.get("backends", {}).get(backend, {}) - api_type = b_cfg.get("api", "genai") + b_cfg = cfg.get("backends", {}).get(backend) + if isinstance(b_cfg, dict): + api_type = b_cfg.get("api", "genai") except Exception: pass diff --git a/tests/test_review_pr_mr.py b/tests/test_review_pr_mr.py index 902366d..7b45915 100644 --- a/tests/test_review_pr_mr.py +++ b/tests/test_review_pr_mr.py @@ -10,10 +10,90 @@ ) import vimini.review as review_module -from vimini.review import resolve_remote +from vimini.review import ( + resolve_remote, + resolve_pr_base, + get_remote_default_branch, + _short_ref, +) from vimini.common.util import temporary_git_worktree, resolve_worktree_dir +def _fake_git( + remotes="origin\n", + default_branch="refs/remotes/origin/main", + branches=("refs/remotes/origin/main",), + commit_counts=None, + merge_bases=None, + exists=(), + fetch_returncode=0, + fetch_stderr="", + merge_base="sha_base", + target_sha="sha_pr", + show_output="diff_content\n", + show_returncode=0, +): + """ + Builds a subprocess.run side effect that answers git invocations based on + the git subcommand, so tests do not depend on the exact call sequence. + """ + commit_counts = commit_counts or {} + merge_bases = merge_bases or {} + branches = list(branches) + exists = set(exists) + + def fake_run(cmd, *args, **kwargs): + gargs = list(cmd[3:]) if cmd[:2] == ["git", "-C"] else list(cmd) + sub = gargs[0] if gargs else "" + + if sub == "remote": + return MagicMock(returncode=0, stdout=remotes, stderr="") + if sub == "symbolic-ref": + if any("refs/remotes/" in a for a in gargs): + if default_branch is None: + return MagicMock(returncode=1, stdout="", stderr="") + return MagicMock(returncode=0, stdout=f"{default_branch}\n", stderr="") + # Current branch: report a detached HEAD for these tests. + return MagicMock(returncode=1, stdout="", stderr="") + if sub == "for-each-ref": + return MagicMock( + returncode=0, stdout="".join(f"{b}\n" for b in branches), stderr="" + ) + if sub == "rev-list": + count = commit_counts.get(gargs[-1]) + if count is None: + return MagicMock(returncode=128, stdout="", stderr="unknown revision") + return MagicMock(returncode=0, stdout=f"{count}\n", stderr="") + if sub == "fetch": + return MagicMock( + returncode=fetch_returncode, stdout="", stderr=fetch_stderr + ) + if sub == "merge-base": + # Allow per-branch merge bases, keyed by the compared branch ref. + looked_up = merge_bases.get(gargs[1], merge_base) + if looked_up is None: + return MagicMock(returncode=1, stdout="", stderr="") + return MagicMock(returncode=0, stdout=f"{looked_up}\n", stderr="") + if sub == "rev-parse": + if "--verify" in gargs: + if gargs[-1] in exists: + return MagicMock(returncode=0, stdout="sha\n", stderr="") + return MagicMock(returncode=1, stdout="", stderr="") + if target_sha is None: + return MagicMock(returncode=1, stdout="", stderr="") + return MagicMock(returncode=0, stdout=f"{target_sha}\n", stderr="") + if sub == "show": + return MagicMock(returncode=show_returncode, stdout=show_output, stderr="") + return MagicMock(returncode=0, stdout="", stderr="") + + return fake_run + + +def _git_commands(mock_run): + """Returns the list of git argument lists issued through a mocked run.""" + return [call[0][0] for call in mock_run.call_args_list] + + def test_review_github_pr_success(tmp_path): repo_path = str(tmp_path) with ( @@ -24,25 +104,16 @@ def test_review_github_pr_success(tmp_path): patch("vimini.util.new_split"), patch("vim.command"), ): - # Configure mock subprocess returns: - # 1: git fetch -> success - # 2: git symbolic-ref -> "origin/main" - # 3: git merge-base -> "sha_base" - # 4: git rev-parse -> "sha_pr" - # 5: git show -> "diff_content" - mock_fetch = MagicMock(returncode=0, stdout="", stderr="") - mock_sym = MagicMock(returncode=0, stdout="origin/main\n", stderr="") - mock_mb = MagicMock(returncode=0, stdout="sha_base\n", stderr="") - mock_rev = MagicMock(returncode=0, stdout="sha_pr\n", stderr="") - mock_show = MagicMock(returncode=0, stdout="diff_content\n", stderr="") - - mock_run.side_effect = [mock_fetch, mock_sym, mock_mb, mock_rev, mock_show] + mock_run.side_effect = _fake_git( + commit_counts={"sha_base..refs/vimini/pr/123": 2} + ) review_module.review("Review this PR", pr="123", remote="origin") - # Verify git fetch call with '--' before positional arguments - fetch_call = mock_run.call_args_list[0] - assert fetch_call[0][0] == [ + cmds = _git_commands(mock_run) + + # The PR head is fetched with '--' before positional arguments. + assert [ "git", "-C", repo_path, @@ -50,28 +121,23 @@ def test_review_github_pr_success(tmp_path): "--", "origin", "+refs/pull/123/head:refs/vimini/pr/123", - ] + ] in cmds - # Verify merge-base call uses origin/main - mb_call = mock_run.call_args_list[2] - assert mb_call[0][0] == [ + # The remote is refreshed so the base branch is current. + assert ["git", "-C", repo_path, "fetch", "--no-tags", "--", "origin"] in cmds + + # The base branch is resolved against the pulled head. + assert [ "git", "-C", repo_path, "merge-base", - "origin/main", + "refs/remotes/origin/main", "refs/vimini/pr/123", - ] + ] in cmds - # Verify git show call - show_call = mock_run.call_args_list[4] - assert show_call[0][0] == [ - "git", - "-C", - repo_path, - "show", - "sha_base..refs/vimini/pr/123", - ] + show_cmd = ["git", "-C", repo_path, "show", "sha_base..refs/vimini/pr/123"] + assert show_cmd in cmds # Verify request sent to channel mock_send.assert_called_once() @@ -90,26 +156,16 @@ def test_review_gitlab_mr_success(tmp_path): patch("vimini.util.new_split"), patch("vim.command"), ): - mock_fetch = MagicMock(returncode=0, stdout="", stderr="") - mock_sym = MagicMock(returncode=1, stdout="", stderr="") - mock_chk_main = MagicMock(returncode=0, stdout="sha_main\n", stderr="") - mock_mb = MagicMock(returncode=0, stdout="sha_base\n", stderr="") - mock_rev = MagicMock(returncode=0, stdout="sha_mr\n", stderr="") - mock_show = MagicMock(returncode=0, stdout="diff_content\n", stderr="") - - mock_run.side_effect = [ - mock_fetch, - mock_sym, - mock_chk_main, - mock_mb, - mock_rev, - mock_show, - ] + mock_run.side_effect = _fake_git( + default_branch="refs/remotes/upstream/main", + branches=("refs/remotes/upstream/main",), + commit_counts={"sha_base..refs/vimini/mr/456": 1}, + ) review_module.review("Review MR", mr="456", remote="upstream", git_objects=None) - fetch_call = mock_run.call_args_list[0] - assert fetch_call[0][0] == [ + cmds = _git_commands(mock_run) + assert [ "git", "-C", repo_path, @@ -117,17 +173,15 @@ def test_review_gitlab_mr_success(tmp_path): "--", "upstream", "+refs/merge-requests/456/head:refs/vimini/mr/456", - ] - - mb_call = mock_run.call_args_list[3] - assert mb_call[0][0] == [ + ] in cmds + assert [ "git", "-C", repo_path, "merge-base", - "upstream/main", + "refs/remotes/upstream/main", "refs/vimini/mr/456", - ] + ] in cmds mock_send.assert_called_once() req_params = mock_send.call_args[0][0]["params"] assert req_params["review_content"] == "diff_content\n" @@ -227,13 +281,9 @@ def test_review_pr_without_worktree_config(tmp_path): patch("vimini.util.new_split"), patch("vim.command"), ): - mock_fetch = MagicMock(returncode=0, stdout="", stderr="") - mock_sym = MagicMock(returncode=0, stdout="origin/main\n", stderr="") - mock_mb = MagicMock(returncode=0, stdout="sha_base\n", stderr="") - mock_rev = MagicMock(returncode=0, stdout="sha_pr\n", stderr="") - mock_show = MagicMock(returncode=0, stdout="diff_content\n", stderr="") - - mock_run.side_effect = [mock_fetch, mock_sym, mock_mb, mock_rev, mock_show] + mock_run.side_effect = _fake_git( + commit_counts={"sha_base..refs/vimini/pr/123": 1} + ) review_module.review("Review this PR", pr="123", remote="origin") @@ -251,27 +301,18 @@ def test_review_subprocess_non_utf8_handling(tmp_path): patch("vimini.util.new_split"), patch("vim.command"), ): - mock_fetch = MagicMock(returncode=0, stdout="", stderr="") - mock_sym = MagicMock(returncode=0, stdout="origin/main\n", stderr="") - mock_mb = MagicMock(returncode=0, stdout="sha_base\n", stderr="") - mock_rev = MagicMock(returncode=0, stdout="sha_pr\n", stderr="") - # Simulate stdout with replaced non-utf8 characters (\ufffd) - mock_show = MagicMock( - returncode=0, stdout="diff with binary byte: \ufffd\n", stderr="" + mock_run.side_effect = _fake_git( + commit_counts={"sha_base..refs/vimini/pr/123": 1}, + # Simulate stdout with replaced non-utf8 characters (\ufffd) + show_output="diff with binary byte: \ufffd\n", ) - mock_run.side_effect = [mock_fetch, mock_sym, mock_mb, mock_rev, mock_show] - review_module.review("Review non-utf8 PR", pr="123", remote="origin") - # Verify encoding and errors parameters in git show and fetch calls - fetch_call = mock_run.call_args_list[0] - assert fetch_call.kwargs.get("encoding") == "utf-8" - assert fetch_call.kwargs.get("errors") == "replace" - - show_call = mock_run.call_args_list[4] - assert show_call.kwargs.get("encoding") == "utf-8" - assert show_call.kwargs.get("errors") == "replace" + # Every git invocation must decode with replacement semantics. + for call in mock_run.call_args_list: + assert call.kwargs.get("encoding") == "utf-8" + assert call.kwargs.get("errors") == "replace" # Verify request sent to channel with replacement character preserved mock_send.assert_called_once() @@ -294,13 +335,9 @@ def test_review_max_turns_from_vim_variable(tmp_path): ), ), ): - mock_fetch = MagicMock(returncode=0, stdout="", stderr="") - mock_sym = MagicMock(returncode=0, stdout="origin/main\n", stderr="") - mock_mb = MagicMock(returncode=0, stdout="sha_base\n", stderr="") - mock_rev = MagicMock(returncode=0, stdout="sha_pr\n", stderr="") - mock_show = MagicMock(returncode=0, stdout="diff_content\n", stderr="") - - mock_run.side_effect = [mock_fetch, mock_sym, mock_mb, mock_rev, mock_show] + mock_run.side_effect = _fake_git( + commit_counts={"sha_base..refs/vimini/pr/123": 1} + ) review_module.review("Review with custom turns", pr="123", remote="origin") @@ -675,40 +712,18 @@ def test_review_github_pr_auto_detect_gh_remote(tmp_path): patch("vimini.util.new_split"), patch("vim.command"), ): - # 1: git remote -> "gh\n" - mock_remote = MagicMock(returncode=0, stdout="gh\n", stderr="") - # 2: git symbolic-ref -> "main" - mock_branch = MagicMock(returncode=0, stdout="main\n", stderr="") - # 3: git config branch.main.remote -> "gh" - mock_cfg = MagicMock(returncode=0, stdout="gh\n", stderr="") - # 4: git fetch -- gh ... -> success - mock_fetch = MagicMock(returncode=0, stdout="", stderr="") - # 5: git symbolic-ref refs/remotes/gh/HEAD -> "gh/main" - mock_sym = MagicMock(returncode=0, stdout="gh/main\n", stderr="") - # 6: git merge-base -> "sha_base" - mock_mb = MagicMock(returncode=0, stdout="sha_base\n", stderr="") - # 7: git rev-parse -> "sha_pr" - mock_rev = MagicMock(returncode=0, stdout="sha_pr\n", stderr="") - # 8: git show -> "diff_content" - mock_show = MagicMock(returncode=0, stdout="diff_content\n", stderr="") - - mock_run.side_effect = [ - mock_remote, - mock_branch, - mock_cfg, - mock_fetch, - mock_sym, - mock_mb, - mock_rev, - mock_show, - ] + mock_run.side_effect = _fake_git( + remotes="gh\n", + default_branch="refs/remotes/gh/main", + branches=("refs/remotes/gh/main",), + commit_counts={"sha_base..refs/vimini/pr/789": 3}, + ) # Call review without remote argument review_module.review("Review PR auto-detect", pr="789") - # Verify fetch call was made with '--' and 'gh' - fetch_call = mock_run.call_args_list[3] - assert fetch_call[0][0] == [ + cmds = _git_commands(mock_run) + assert [ "git", "-C", repo_path, @@ -716,5 +731,433 @@ def test_review_github_pr_auto_detect_gh_remote(tmp_path): "--", "gh", "+refs/pull/789/head:refs/vimini/pr/789", - ] + ] in cmds + assert ["git", "-C", repo_path, "fetch", "--no-tags", "--", "gh"] in cmds mock_send.assert_called_once() + + +def test_resolve_pr_base_picks_branch_with_fewest_commits(): + # The PR targets 'release-1'; using 'main' would pull in unrelated commits. + pr_ref = "refs/vimini/pr/1" + with patch("subprocess.run") as mock_run: + mock_run.side_effect = _fake_git( + branches=("refs/remotes/origin/release-1", "refs/remotes/origin/main"), + merge_bases={ + "refs/remotes/origin/release-1": "sha_release", + "refs/remotes/origin/main": "sha_main", + }, + commit_counts={ + f"sha_release..{pr_ref}": 2, + f"sha_main..{pr_ref}": 9, + }, + ) + base_ref, base_sha, commit_count = resolve_pr_base( + "/repo", + "origin", + pr_ref, + default_ref="refs/remotes/origin/main", + ) + + assert base_ref == "refs/remotes/origin/release-1" + assert base_sha == "sha_release" + assert commit_count == 2 + + +def test_resolve_pr_base_ignores_branches_containing_the_pr(): + pr_ref = "refs/vimini/pr/1" + with patch("subprocess.run") as mock_run: + mock_run.side_effect = _fake_git( + branches=("refs/remotes/origin/main", "refs/remotes/origin/topic"), + merge_bases={ + # 'main' already contains the PR head, so it is not the base. + "refs/remotes/origin/main": "sha_pr", + "refs/remotes/origin/topic": "sha_topic", + }, + commit_counts={ + f"sha_pr..{pr_ref}": 0, + f"sha_topic..{pr_ref}": 1, + }, + ) + base_ref, base_sha, commit_count = resolve_pr_base( + "/repo", "origin", pr_ref, default_ref="refs/remotes/origin/main" + ) + + assert base_ref == "refs/remotes/origin/topic" + assert base_sha == "sha_topic" + assert commit_count == 1 + + +def test_resolve_pr_base_prefers_default_branch_on_tie(): + pr_ref = "refs/vimini/pr/1" + with patch("subprocess.run") as mock_run: + mock_run.side_effect = _fake_git( + branches=("refs/remotes/origin/devel", "refs/remotes/origin/main"), + merge_bases={ + "refs/remotes/origin/devel": "sha_devel", + "refs/remotes/origin/main": "sha_main", + }, + commit_counts={ + f"sha_devel..{pr_ref}": 4, + f"sha_main..{pr_ref}": 4, + }, + ) + base_ref, _, commit_count = resolve_pr_base( + "/repo", "origin", pr_ref, default_ref="refs/remotes/origin/main" + ) + + assert base_ref == "refs/remotes/origin/main" + assert commit_count == 4 + + +def test_resolve_pr_base_prefers_recency_order_over_name_length_on_tie(): + pr_ref = "refs/vimini/pr/1" + with patch("subprocess.run") as mock_run: + mock_run.side_effect = _fake_git( + branches=( + "refs/remotes/origin/release-1.0", + "refs/remotes/origin/ci", + ), + merge_bases={ + "refs/remotes/origin/release-1.0": "sha_rel", + "refs/remotes/origin/ci": "sha_ci", + }, + commit_counts={ + f"sha_rel..{pr_ref}": 3, + f"sha_ci..{pr_ref}": 3, + }, + ) + base_ref, _, commit_count = resolve_pr_base( + "/repo", "origin", pr_ref, default_ref="refs/remotes/origin/main" + ) + + assert base_ref == "refs/remotes/origin/release-1.0" + assert commit_count == 3 + + +def test_short_ref_strips_remote_prefixes(): + assert _short_ref("refs/remotes/origin/main", "origin") == "main" + assert _short_ref("origin/main", "origin") == "main" + assert _short_ref("main", "origin") == "main" + + +def test_get_remote_default_branch_uses_full_symbolic_ref(): + with patch("subprocess.run") as mock_run: + mock_run.side_effect = _fake_git(default_branch="refs/remotes/origin/main") + assert ( + get_remote_default_branch("/repo", "origin") == "refs/remotes/origin/main" + ) + cmds = _git_commands(mock_run) + assert [ + "git", + "-C", + "/repo", + "symbolic-ref", + "refs/remotes/origin/HEAD", + ] in cmds + + +def test_resolve_pr_base_returns_none_without_candidates(): + with patch("subprocess.run") as mock_run: + mock_run.side_effect = _fake_git(branches=()) + assert resolve_pr_base("/repo", "origin", "refs/vimini/pr/1") == ( + None, + None, + None, + ) + + +def test_review_pr_warns_when_remote_refresh_fails(tmp_path): + repo_path = str(tmp_path) + with ( + patch("vimini.util.get_git_repo_root", return_value=repo_path), + patch("vimini.review.get_project_config", return_value=None), + patch("subprocess.run") as mock_run, + patch("vimini.util.send_channel_request"), + patch("vimini.util.new_split"), + patch("vim.command"), + patch("vimini.util.display_message") as mock_display, + ): + inner = _fake_git(commit_counts={"sha_base..refs/vimini/pr/5": 1}) + + def fake_run(cmd, *args, **kwargs): + gargs = list(cmd[3:]) if cmd[:2] == ["git", "-C"] else list(cmd) + if gargs[0] == "fetch" and "--no-tags" in gargs: + return MagicMock(returncode=1, stdout="", stderr="network unreachable") + return inner(cmd, *args, **kwargs) + + mock_run.side_effect = fake_run + + review_module.review("Review", pr="5", remote="origin") + + warnings = [ + call[0][0] + for call in mock_display.call_args_list + if "could not refresh 'origin'" in call[0][0] + ] + assert len(warnings) == 1 + assert "network unreachable" in warnings[0] + + +def test_review_reports_resolved_base_and_commit_count(tmp_path): + repo_path = str(tmp_path) + with ( + patch("vimini.util.get_git_repo_root", return_value=repo_path), + patch("vimini.review.get_project_config", return_value=None), + patch("subprocess.run") as mock_run, + patch("vimini.util.send_channel_request"), + patch("vimini.util.new_split"), + patch("vim.command"), + patch("vimini.util.display_message") as mock_display, + ): + mock_run.side_effect = _fake_git( + branches=("refs/remotes/origin/release-1", "refs/remotes/origin/main"), + merge_bases={ + "refs/remotes/origin/release-1": "sha_release", + "refs/remotes/origin/main": "sha_main", + }, + commit_counts={ + "sha_release..refs/vimini/pr/7": 2, + "sha_main..refs/vimini/pr/7": 8, + }, + ) + + review_module.review("Review", pr="7", remote="origin") + + messages = [call[0][0] for call in mock_display.call_args_list] + assert any( + "Reviewing 2 commit(s) of GitHub PR #7 (base: release-1)" in m + for m in messages + ), messages + + +# --- End-to-end regression tests against real git repositories --- + + +def _git(repo, *args): + res = subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True, check=False + ) + assert res.returncode == 0, (args, res.stderr) + return res.stdout.strip() + + +def _commit(repo, name, message): + (repo / name).write_text(message + "\n") + _git(repo, "add", name) + _git(repo, "commit", "-m", message) + return _git(repo, "rev-parse", "HEAD") + + +def _show_commit_count(content): + return sum(1 for line in content.splitlines() if line.startswith("commit ")) + + +def _review_pr_capturing_params(local_repo, pr="1", **kwargs): + """Runs review() against a real repository and returns the request params.""" + captured = {} + + with ( + patch("vimini.util.get_git_repo_root", return_value=str(local_repo)), + patch("vimini.review.get_project_config", return_value=None), + patch("vimini.util.get_git_repo_name", return_value="local"), + patch( + "vimini.util.send_channel_request", + side_effect=lambda req: captured.update(req), + ), + patch("vimini.util.new_split"), + patch("vimini.util.append_job_summary"), + patch("vimini.util.append_to_buffer"), + patch("vimini.util.reserve_next_job_id", return_value=1), + patch("vimini.util.display_message"), + patch("vim.command"), + ): + review_module.review("review this", pr=pr, remote="origin", **kwargs) + + assert "params" in captured, "review() did not submit a review request" + return captured["params"] + + +def _build_upstream(tmp_path, pr_base="main", extra_main_commits=7): + """ + Creates a bare upstream with a stale clone, a PR pushed to refs/pull/1/head, + and returns the clone path. The clone's remote-tracking refs intentionally + lag behind upstream. + """ + upstream = tmp_path / "upstream.git" + subprocess.run(["git", "init", "--bare", "-b", "main", str(upstream)], check=True) + + work = tmp_path / "work" + subprocess.run( + ["git", "clone", str(upstream), str(work)], capture_output=True, check=True + ) + _git(work, "config", "user.name", "T") + _git(work, "config", "user.email", "t@example.com") + + for i in range(3): + _commit(work, f"base{i}.txt", f"BASE {i}") + _git(work, "push", "origin", "main") + + # The user clones here, so their remote-tracking refs stop at BASE 2. + local = tmp_path / "local" + subprocess.run( + ["git", "clone", str(upstream), str(local)], capture_output=True, check=True + ) + _git(local, "config", "user.name", "T") + _git(local, "config", "user.email", "t@example.com") + + if pr_base != "main": + _git(work, "checkout", "-b", pr_base) + _commit(work, f"{pr_base}-1.txt", f"{pr_base.upper()} 1") + _commit(work, f"{pr_base}-2.txt", f"{pr_base.upper()} 2") + _git(work, "push", "origin", pr_base) + _git(work, "checkout", "main") + + # Upstream moves on after the clone. + for i in range(extra_main_commits): + _commit(work, f"later{i}.txt", f"LATER {i}") + _git(work, "push", "origin", "main") + + # The PR is opened on the current tip of its target branch. + _git(work, "checkout", pr_base) + _git(work, "checkout", "-b", "feature") + _commit(work, "pr1.txt", "PR COMMIT 1") + _commit(work, "pr2.txt", "PR COMMIT 2") + _git(work, "push", "origin", "feature") + _git(work, "push", "origin", "feature:refs/pull/1/head") + _git(work, "checkout", "main") + + return local + + +def _build_empty_pr(tmp_path): + """ + Creates an upstream and a clone where refs/pull/1/head points at the base + branch tip, i.e. a PR that has already been merged or is empty. + """ + upstream = tmp_path / "upstream.git" + subprocess.run(["git", "init", "--bare", "-b", "main", str(upstream)], check=True) + + work = tmp_path / "work" + subprocess.run( + ["git", "clone", str(upstream), str(work)], capture_output=True, check=True + ) + _git(work, "config", "user.name", "T") + _git(work, "config", "user.email", "t@example.com") + + for i in range(3): + _commit(work, f"base{i}.txt", f"BASE {i}") + _git(work, "push", "origin", "main") + # The PR head is the base tip: nothing of its own to review. + _git(work, "push", "origin", "main:refs/pull/1/head") + + local = tmp_path / "local" + subprocess.run( + ["git", "clone", str(upstream), str(local)], capture_output=True, check=True + ) + return local + + +def test_review_pr_uses_fresh_base_branch_with_stale_clone(tmp_path): + """ + A stale remote-tracking base branch must not pull unrelated upstream commits + into the review (regression test). + """ + local = _build_upstream(tmp_path) + + params = _review_pr_capturing_params(local) + + content = params["review_content"] + assert _show_commit_count(content) == 2, content + assert "PR COMMIT 1" in content + assert "PR COMMIT 2" in content + assert "LATER" not in content + + +def test_review_pr_uses_target_branch_not_default_branch(tmp_path): + """ + A PR against a non-default branch must be diffed against that branch, not + against the remote's default branch (regression test). + """ + local = _build_upstream(tmp_path, pr_base="release-1") + + params = _review_pr_capturing_params(local) + + content = params["review_content"] + assert _show_commit_count(content) == 2, content + assert "PR COMMIT 1" in content + assert "PR COMMIT 2" in content + # Commits belonging to the target branch must not be reviewed. + assert "RELEASE-1 1" not in content + assert "RELEASE-1 2" not in content + + +def test_review_pr_batch_save_reviews_only_pr_commits(tmp_path): + """ + Batch (--save) mode must derive its commit list from the same corrected + range, so each saved review covers one PR commit only (regression test). + """ + local = _build_upstream(tmp_path) + + params = _review_pr_capturing_params(local, save=True) + + commit_list = params["commit_list"] + assert len(commit_list) == 2, commit_list + + subjects = [_git(local, "log", "-1", "--pretty=%s", sha) for sha in commit_list] + assert subjects == ["PR COMMIT 1", "PR COMMIT 2"] + + +def test_review_empty_pr_submits_nothing(tmp_path): + """ + An empty/already merged PR must not be submitted for review at all, so that + no commit belonging to the base branch reaches the model (regression test). + """ + local = _build_empty_pr(tmp_path) + sent = [] + + with ( + patch("vimini.util.get_git_repo_root", return_value=str(local)), + patch("vimini.review.get_project_config", return_value=None), + patch("vimini.util.get_git_repo_name", return_value="local"), + patch("vimini.util.send_channel_request", side_effect=sent.append), + patch("vimini.util.new_split") as mock_split, + patch("vimini.util.append_job_summary"), + patch("vimini.util.append_to_buffer"), + patch("vimini.util.reserve_next_job_id", return_value=1) as mock_job_id, + patch("vimini.util.display_message") as mock_display, + patch("vim.command"), + ): + review_module.review("review this", pr="1", remote="origin") + + assert sent == [] + mock_job_id.assert_not_called() + mock_split.assert_not_called() + + messages = [call[0][0] for call in mock_display.call_args_list] + assert any("Nothing to review" in m and "GitHub PR #1" in m for m in messages), ( + messages + ) + + +def test_review_empty_pr_in_batch_mode_submits_nothing(tmp_path): + """ + Batch (--save) mode must stop for an empty/already merged PR too, instead of + walking the base branch tip with --no-walk (regression test). + """ + local = _build_empty_pr(tmp_path) + sent = [] + + with ( + patch("vimini.util.get_git_repo_root", return_value=str(local)), + patch("vimini.review.get_project_config", return_value=None), + patch("vimini.util.get_git_repo_name", return_value="local"), + patch("vimini.util.send_channel_request", side_effect=sent.append), + patch("vimini.util.reserve_next_job_id", return_value=1) as mock_job_id, + patch("vimini.util.display_message"), + ): + review_module.review( + "review this", pr="1", remote="origin", save=True, save_path=str(tmp_path) + ) + + assert sent == [] + mock_job_id.assert_not_called()