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()