From 08c359ca596771a1e5e2a6e8d47c32340c56d8ef Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Fri, 23 Oct 2026 14:21:35 -0400 Subject: [PATCH] Auto-resolve git remotes for PR and MR reviews Add automatic git remote resolution when reviewing pull or merge requests without an explicit remote specified. The resolver checks g:vimini_default_remote, prioritizes 'upstream' in fork environments, inspects canonical branch tracking configurations, and falls back to 'origin'. Also pass '--' in git fetch commands to ensure positional arguments are handled safely. Assisted-by: Gemini:Gemini 3.8 Flash Signed-off-by: Simo Sorce --- README.md | 16 +- plugin/vimini.vim | 5 +- python3/vimini/main.py | 2 +- python3/vimini/review.py | 179 +++++++++++++++++-- tests/test_commit_files.py | 5 +- tests/test_review_pr_mr.py | 356 ++++++++++++++++++++++++++++++++++++- 6 files changed, 538 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 568921a..b30ca28 100644 --- a/README.md +++ b/README.md @@ -126,14 +126,22 @@ customizations. let g:vimini_review_max_turns = 30 ``` -8. **Context Files (`g:context_files`)**: +8. **Default Git Remote (`g:vimini_default_remote`)**: + Set a default git remote to fetch GitHub PRs or GitLab MRs from when using `:ViminiReview` without the `--remote` flag. + If not configured, Vimini will check available remotes and use `upstream` (preferred for fork workflows), or the + canonical remote for the current branch, falling back to default branches (`main`/`master`) or `origin`. + ```vim + let g:vimini_default_remote = 'gh' + ``` + +9. **Context Files (`g:context_files`)**: In addition to all files currently open in Vim buffers, you can specify a list of files to always include as context for AI commands like `:ViminiCode`. This is useful for providing the AI with important project files (like configurations, core utilities, or type definitions) without needing to have them open. Note: The total size of all uploaded context files combined is strictly limited to 1MB; if this limit is exceeded, the largest files will automatically be excluded. ```vim " Always include these files as context for code generation tasks let g:context_files = ['package.json', 'src/main.js', 'src/utils/api.js'] ``` -9. **Project Configuration (`ViminiConfig`)**: +10. **Project Configuration (`ViminiConfig`)**: Configure per-project settings such as whether compilation is needed, shell commands for compiling/building code and running tests when invoked by chat tools (`build_code` and `test_code`), and the directory path where temporary git worktrees can be created. Project configuration is stored in `~/.var/vimini/projects/`: ```json { @@ -264,7 +272,7 @@ Sends content to the Gemini model for a code review. This command has three main 1. **Current Buffer Review**: If no `-c`, `--pr`, or `--mr` option is provided, it sends the content of the current buffer for review. 2. **Git Object Review**: If the `-c ` option is provided, it reviews the changes specified by the git objects. It sends the `git show` output (the diff) to the AI, and critically, it also uploads the full content of all changed files to provide complete context for the review. `` can be any valid git object reference, like a commit hash, branch name, or a range like `HEAD~3..HEAD`. -3. **GitHub PR / GitLab MR Review**: Use `--pr ` (or `--github `) or `--mr ` (or `--gitlab `) to automatically fetch the branch from the remote repository and run the review on its commits. You can optionally specify `--remote ` (default: `origin`). +3. **GitHub PR / GitLab MR Review**: Use `--pr ` (or `--github `) or `--mr ` (or `--gitlab `) to automatically fetch the branch from the remote repository and run the review on its commits. You can optionally specify `--remote ` (defaults to `g:vimini_default_remote`, `upstream`, the canonical remote for the current branch, or `origin`). You can add an optional `{prompt}` to guide the AI's review. The review will be displayed in a new vertical split buffer. If `g:vimini_thinking` is `on`, an additional buffer showing the AI's thought process will also be opened. @@ -272,7 +280,7 @@ You can add an optional `{prompt}` to guide the AI's review. The review will be * `--pr ` / `--github `: Fetches and reviews the specified GitHub Pull Request number. * `--mr ` / `--gitlab `: Fetches and reviews the specified GitLab Merge Request number. -* `--remote `: Remote git repository to fetch PR/MR from (defaults to `origin`). +* `--remote `: Remote git repository to fetch PR/MR from (defaults to `g:vimini_default_remote`, `upstream`, the canonical remote for the current branch, or `origin`). * `--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`). diff --git a/plugin/vimini.vim b/plugin/vimini.vim index c9642d1..8c236b6 100644 --- a/plugin/vimini.vim +++ b/plugin/vimini.vim @@ -26,6 +26,9 @@ let g:vimini_review_path = get(g:, 'vimini_review_path', '') " Configuration: Maximum review tool iterations let g:vimini_review_max_turns = get(g:, 'vimini_review_max_turns', 30) +" Configuration: Default git remote for PR/MR reviews +let g:vimini_default_remote = get(g:, 'vimini_default_remote', '') + " Configuration: Agent Channel let g:vimini_channel = get(g:, 'vimini_channel', v:null) @@ -234,7 +237,7 @@ function! ViminiReview(args) let l:save_path = '' let l:pr_arg = v:null let l:mr_arg = v:null - let l:remote_arg = 'origin' + let l:remote_arg = v:null let l:args = a:args " Handle key=value options like --save=path, --pr=123, --mr=123, --remote=origin diff --git a/python3/vimini/main.py b/python3/vimini/main.py index b4d1562..6dea22d 100644 --- a/python3/vimini/main.py +++ b/python3/vimini/main.py @@ -299,7 +299,7 @@ def help(command_name=None): " -c : Review changes in git ref (can be a range for batch review).", " --pr : Fetch and review GitHub Pull Request.", " --mr : Fetch and review GitLab Merge Request.", - " --remote : Remote repository to fetch from (defaults to origin).", + " --remote : Remote repository to fetch from (defaults to g:vimini_default_remote, upstream, branch remote, or origin).", " --security: Focus on security vulnerabilities.", " --save: Save reviews to text files (useful with batch review).", " --save=path: Save reviews to a specific directory (defaults to g:vimini_review_path).", diff --git a/python3/vimini/review.py b/python3/vimini/review.py index 9c1fa69..59e4002 100644 --- a/python3/vimini/review.py +++ b/python3/vimini/review.py @@ -90,6 +90,158 @@ def send_review_termination(req_id): util.send_channel_request(req, True) +def resolve_remote(repo_path, remote=None): + """ + Determines the git remote to use. + If 'remote' is explicitly specified and non-empty, returns it. + If 'g:vimini_default_remote' is configured and non-empty, returns it (or warns if invalid). + Otherwise, inspects available remotes in repo_path: + - If 'upstream' is present in remotes, returns 'upstream' (preferred for fork workflows). + - Finds the canonical remote for the current branch: + 1. branch..remote + 2. Upstream tracking branch remote (@{upstream}) + - Falls back to default branch configurations: + 3. Default branch remote (branch.main.remote, branch.master.remote) + - 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. + """ + if remote is not None and str(remote).strip() != "": + return str(remote).strip() + + try: + cfg_remote = vim.eval("get(g:, 'vimini_default_remote', '')") + if isinstance(cfg_remote, str) and cfg_remote.strip(): + candidate = cfg_remote.strip() + if not candidate.startswith("-") and re.match( + r"^[a-zA-Z0-9_\-\./]+$", candidate + ): + return candidate + util.display_message( + f"Warning: Invalid 'g:vimini_default_remote' setting '{candidate}'.", + error=True, + ) + except Exception: + pass + + if not repo_path: + return "origin" + + # Find available remotes + remotes = [] + try: + res = subprocess.run( + ["git", "-C", repo_path, "remote"], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=False, + ) + if res.returncode == 0: + remotes = [line.strip() for line in res.stdout.splitlines() if line.strip()] + except Exception: + pass + + # In fork setups, PRs/MRs reside on upstream; prefer it early to avoid overhead + if "upstream" in remotes: + return "upstream" + + # Find canonical remote for current branch + try: + branch_res = subprocess.run( + ["git", "-C", repo_path, "symbolic-ref", "--short", "-q", "HEAD"], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=False, + ) + current_branch = branch_res.stdout.strip() if branch_res.returncode == 0 else "" + if current_branch: + # 1. branch..remote + cfg_res = subprocess.run( + [ + "git", + "-C", + repo_path, + "config", + f"branch.{current_branch}.remote", + ], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=False, + ) + branch_remote = cfg_res.stdout.strip() if cfg_res.returncode == 0 else "" + if ( + branch_remote + and branch_remote != "." + and (not remotes or branch_remote in remotes) + ): + return branch_remote + + # 2. @{upstream} tracking remote + u_res = subprocess.run( + [ + "git", + "-C", + repo_path, + "rev-parse", + "--abbrev-ref", + "--symbolic-full-name", + "@{upstream}", + ], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=False, + ) + if u_res.returncode == 0 and u_res.stdout.strip(): + u_remote = u_res.stdout.strip().split("/")[0] + if u_remote and u_remote in remotes: + return u_remote + except Exception: + pass + + # 3. Fallback to main/master branch remote config + try: + for def_branch in ["main", "master"]: + def_res = subprocess.run( + [ + "git", + "-C", + repo_path, + "config", + f"branch.{def_branch}.remote", + ], + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=False, + ) + def_remote = def_res.stdout.strip() if def_res.returncode == 0 else "" + if ( + def_remote + and def_remote != "." + and (not remotes or def_remote in remotes) + ): + return def_remote + except Exception: + pass + + if "origin" in remotes: + return "origin" + if remotes: + return remotes[0] + + return "origin" + + def review( prompt, git_objects=None, @@ -99,7 +251,7 @@ def review( save_path=None, pr=None, mr=None, - remote="origin", + remote=None, ): """ Sends content to the Gemini API for a code review via the background agent. @@ -110,18 +262,6 @@ def review( f"review({prompt}, git_objects='{git_objects}', security_focus={security_focus}, verbose={verbose}, save={save}, save_path='{save_path}', pr={pr}, mr={mr}, remote='{remote}')" ) try: - # Validate remote - if remote is None or str(remote).strip() == "": - remote = "origin" - else: - remote = str(remote).strip() - - if remote.startswith("-") or not re.match(r"^[a-zA-Z0-9_\-\./]+$", remote): - util.display_message( - f"Security error: Invalid remote name '{remote}'.", error=True - ) - return - # Handle GitHub PR or GitLab MR fetching pr_id = str(pr).strip() if pr is not None else None mr_id = str(mr).strip() if mr is not None else None @@ -144,6 +284,17 @@ def review( if not repo_path: return + remote = resolve_remote(repo_path, remote) + if ( + not remote + or remote.startswith("-") + or not re.match(r"^[a-zA-Z0-9_\-\./]+$", remote) + ): + util.display_message( + f"Security error: Invalid remote name '{remote}'.", error=True + ) + return + if pr_id: refspec = f"+refs/pull/{pr_id}/head:refs/vimini/pr/{pr_id}" local_ref = f"refs/vimini/pr/{pr_id}" @@ -161,7 +312,7 @@ def review( worktree_ref = local_ref util.display_message(f"Fetching {service_desc} from {remote}...") - fetch_cmd = ["git", "-C", repo_path, "fetch", remote, refspec] + fetch_cmd = ["git", "-C", repo_path, "fetch", "--", remote, refspec] res = subprocess.run( fetch_cmd, capture_output=True, diff --git a/tests/test_commit_files.py b/tests/test_commit_files.py index 692cf18..df29782 100644 --- a/tests/test_commit_files.py +++ b/tests/test_commit_files.py @@ -77,10 +77,10 @@ def test_toggle_file(): ] with ( + patch.object(vim.current, "buffer", mock_buffer), patch.object(vim, "eval") as mock_eval, patch.object(vim, "command") as mock_cmd, ): - vim.current.buffer = mock_buffer vim.current.window.cursor = (4, 4) # Row 4 is "# src/app.py" mock_eval.side_effect = lambda expr: ( @@ -119,9 +119,8 @@ def test_handle_commit_response_adds_files_and_mapping(tmp_path): } mock_buffer = [] - vim.current.buffer = mock_buffer - with ( + patch.object(vim.current, "buffer", mock_buffer), patch("vimini.util.new_split") as mock_split, patch.object(vim, "command") as mock_cmd, patch("vimini.util.display_message") as mock_display, diff --git a/tests/test_review_pr_mr.py b/tests/test_review_pr_mr.py index 3f84948..902366d 100644 --- a/tests/test_review_pr_mr.py +++ b/tests/test_review_pr_mr.py @@ -10,6 +10,7 @@ ) import vimini.review as review_module +from vimini.review import resolve_remote from vimini.common.util import temporary_git_worktree, resolve_worktree_dir @@ -39,13 +40,14 @@ def test_review_github_pr_success(tmp_path): review_module.review("Review this PR", pr="123", remote="origin") - # Verify git fetch call + # Verify git fetch call with '--' before positional arguments fetch_call = mock_run.call_args_list[0] assert fetch_call[0][0] == [ "git", "-C", repo_path, "fetch", + "--", "origin", "+refs/pull/123/head:refs/vimini/pr/123", ] @@ -112,6 +114,7 @@ def test_review_gitlab_mr_success(tmp_path): "-C", repo_path, "fetch", + "--", "upstream", "+refs/merge-requests/456/head:refs/vimini/mr/456", ] @@ -142,8 +145,10 @@ def test_review_pr_invalid_number(): mock_run.assert_not_called() -def test_review_pr_invalid_remote(): +def test_review_pr_invalid_remote(tmp_path): + repo_path = str(tmp_path) with ( + patch("vimini.util.get_git_repo_root", return_value=repo_path), patch("vimini.util.display_message") as mock_display, patch("subprocess.run") as mock_run, ): @@ -153,6 +158,44 @@ def test_review_pr_invalid_remote(): mock_run.assert_not_called() +def test_review_pr_auto_resolved_invalid_remote(tmp_path): + repo_path = str(tmp_path) + with ( + patch("vimini.util.get_git_repo_root", return_value=repo_path), + patch("vimini.review.resolve_remote", return_value="-oProxyCommand=calc"), + patch("vimini.util.display_message") as mock_display, + patch("subprocess.run") as mock_run, + ): + review_module.review("test", pr="123", remote=None) + mock_display.assert_called() + err_msgs = [ + call[0][0] + for call in mock_display.call_args_list + if "Security error: Invalid remote name" in call[0][0] + ] + assert len(err_msgs) == 1 + mock_run.assert_not_called() + + +def test_review_pr_auto_resolved_invalid_characters(tmp_path): + repo_path = str(tmp_path) + with ( + patch("vimini.util.get_git_repo_root", return_value=repo_path), + patch("vimini.review.resolve_remote", return_value="remote;rm -rf"), + patch("vimini.util.display_message") as mock_display, + patch("subprocess.run") as mock_run, + ): + review_module.review("test", pr="123", remote=None) + mock_display.assert_called() + err_msgs = [ + call[0][0] + for call in mock_display.call_args_list + if "Security error: Invalid remote name" in call[0][0] + ] + assert len(err_msgs) == 1 + mock_run.assert_not_called() + + def test_review_pr_fetch_failure(tmp_path): repo_path = str(tmp_path) with ( @@ -366,3 +409,312 @@ def test_temporary_git_worktree_real_repo(tmp_path): # Worktree path must be cleaned up after exit assert captured_worktree_path is not None assert not os.path.exists(captured_worktree_path) + + +def test_resolve_remote_explicit(): + # Explicit remote argument always takes precedence without running git + with patch("subprocess.run") as mock_run: + assert resolve_remote("/repo", remote="gh") == "gh" + mock_run.assert_not_called() + + +def test_resolve_remote_g_vimini_default_remote(): + with ( + patch( + "vim.eval", + side_effect=lambda expr: ( + "gh" if expr == "get(g:, 'vimini_default_remote', '')" else "" + ), + ), + patch("subprocess.run") as mock_run, + ): + assert resolve_remote("/repo", remote=None) == "gh" + mock_run.assert_not_called() + + +def test_resolve_remote_g_vimini_default_remote_invalid(): + mock_remote = MagicMock(returncode=0, stdout="origin\n", stderr="") + with ( + patch( + "vim.eval", + side_effect=lambda expr: ( + "-oProxyCommand=calc" + if expr == "get(g:, 'vimini_default_remote', '')" + else "" + ), + ), + patch("subprocess.run", return_value=mock_remote), + patch("vimini.util.display_message") as mock_display, + ): + assert resolve_remote("/repo", remote=None) == "origin" + mock_display.assert_called_once() + assert "Invalid 'g:vimini_default_remote'" in mock_display.call_args[0][0] + + +def test_resolve_remote_origin_available(): + mock_remote = MagicMock(returncode=0, stdout="gh\norigin\n", stderr="") + mock_branch = MagicMock(returncode=1, stdout="", stderr="") # detached HEAD + mock_main_remote = MagicMock(returncode=1, stdout="", stderr="") + mock_master_remote = MagicMock(returncode=1, stdout="", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [ + mock_remote, + mock_branch, + mock_main_remote, + mock_master_remote, + ] + assert resolve_remote("/repo", remote=None) == "origin" + + +def test_resolve_remote_origin_not_available_branch_tracking(): + mock_remote = MagicMock(returncode=0, stdout="gh\n", stderr="") + mock_branch = MagicMock(returncode=0, stdout="main\n", stderr="") + mock_cfg = MagicMock(returncode=0, stdout="gh\n", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote, mock_branch, mock_cfg] + assert resolve_remote("/repo", remote=None) == "gh" + + assert mock_run.call_args_list[0][0][0] == ["git", "-C", "/repo", "remote"] + assert mock_run.call_args_list[1][0][0] == [ + "git", + "-C", + "/repo", + "symbolic-ref", + "--short", + "-q", + "HEAD", + ] + assert mock_run.call_args_list[2][0][0] == [ + "git", + "-C", + "/repo", + "config", + "branch.main.remote", + ] + + +def test_resolve_remote_origin_not_available_upstream_tracking(): + mock_remote = MagicMock(returncode=0, stdout="gh\n", stderr="") + mock_branch = MagicMock(returncode=0, stdout="feature\n", stderr="") + mock_cfg = MagicMock(returncode=1, stdout="", stderr="") + mock_u = MagicMock(returncode=0, stdout="gh/feature\n", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote, mock_branch, mock_cfg, mock_u] + assert resolve_remote("/repo", remote=None) == "gh" + + +def test_resolve_remote_branch_tracking_dot_ignored(): + # When branch..remote is "." (local tracking), it must be ignored and not returned + mock_remote = MagicMock(returncode=0, stdout="origin\n", stderr="") + mock_branch = MagicMock(returncode=0, stdout="feature\n", stderr="") + mock_cfg = MagicMock(returncode=0, stdout=".\n", stderr="") + mock_u = MagicMock(returncode=1, stdout="", stderr="") + mock_main_remote = MagicMock(returncode=1, stdout="", stderr="") + mock_master_remote = MagicMock(returncode=1, stdout="", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [ + mock_remote, + mock_branch, + mock_cfg, + mock_u, + mock_main_remote, + mock_master_remote, + ] + assert resolve_remote("/repo", remote=None) == "origin" + + +def test_resolve_remote_branch_tracking_dot_with_empty_remotes(): + mock_remote = MagicMock(returncode=0, stdout="", stderr="") + mock_branch = MagicMock(returncode=0, stdout="feature\n", stderr="") + mock_cfg = MagicMock(returncode=0, stdout=".\n", stderr="") + mock_u = MagicMock(returncode=1, stdout="", stderr="") + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote, mock_branch, mock_cfg, mock_u] + assert resolve_remote("/repo", remote=None) == "origin" + + +def test_resolve_remote_upstream_tracking_local_branch_ignored(): + # When rev-parse @{upstream} returns a local branch name (not in remotes), ignore it + mock_remote = MagicMock(returncode=0, stdout="origin\n", stderr="") + mock_branch = MagicMock(returncode=0, stdout="feature\n", stderr="") + mock_cfg = MagicMock(returncode=1, stdout="", stderr="") + mock_u = MagicMock( + returncode=0, stdout="main\n", stderr="" + ) # tracking local 'main' + mock_main_remote = MagicMock(returncode=1, stdout="", stderr="") + mock_master_remote = MagicMock(returncode=1, stdout="", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [ + mock_remote, + mock_branch, + mock_cfg, + mock_u, + mock_main_remote, + mock_master_remote, + ] + # 'main' is not in remotes, so it should fall back to 'origin' + assert resolve_remote("/repo", remote=None) == "origin" + + +def test_resolve_remote_upstream_tracking_empty_remotes_ignored(): + # When remotes is empty, @{upstream} should not be used as remote + mock_remote = MagicMock(returncode=0, stdout="", stderr="") + mock_branch = MagicMock(returncode=0, stdout="feature\n", stderr="") + mock_cfg = MagicMock(returncode=1, stdout="", stderr="") + mock_u = MagicMock(returncode=0, stdout="origin/feature\n", stderr="") + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote, mock_branch, mock_cfg, mock_u] + assert resolve_remote("/repo", remote=None) == "origin" + + +def test_resolve_remote_push_configs_ignored(): + # Push configurations (pushRemote, remote.pushDefault) should not be used for PR/MR fetching + mock_remote = MagicMock(returncode=0, stdout="origin\n", stderr="") + mock_branch = MagicMock(returncode=0, stdout="feature\n", stderr="") + mock_cfg = MagicMock(returncode=1, stdout="", stderr="") + mock_u = MagicMock(returncode=1, stdout="", stderr="") + mock_main_remote = MagicMock(returncode=1, stdout="", stderr="") + mock_master_remote = MagicMock(returncode=1, stdout="", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [ + mock_remote, + mock_branch, + mock_cfg, + mock_u, + mock_main_remote, + mock_master_remote, + ] + assert resolve_remote("/repo", remote=None) == "origin" + cmds = [call[0][0] for call in mock_run.call_args_list] + assert not any("pushRemote" in " ".join(cmd) for cmd in cmds) + assert not any("pushDefault" in " ".join(cmd) for cmd in cmds) + + +def test_resolve_remote_origin_not_available_main_branch_remote(): + mock_remote = MagicMock(returncode=0, stdout="gh\nother\n", stderr="") + mock_branch = MagicMock(returncode=1, stdout="", stderr="") # detached HEAD + mock_main_remote = MagicMock(returncode=0, stdout="gh\n", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote, mock_branch, mock_main_remote] + assert resolve_remote("/repo", remote=None) == "gh" + + +def test_resolve_remote_origin_not_available_single_remote(): + mock_remote = MagicMock(returncode=0, stdout="gh\n", stderr="") + mock_branch = MagicMock(returncode=1, stdout="", stderr="") # detached HEAD + mock_main_remote = MagicMock(returncode=1, stdout="", stderr="") + mock_master_remote = MagicMock(returncode=1, stdout="", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [ + mock_remote, + mock_branch, + mock_main_remote, + mock_master_remote, + ] + assert resolve_remote("/repo", remote=None) == "gh" + + +def test_resolve_remote_origin_not_available_upstream_preference(): + mock_remote = MagicMock(returncode=0, stdout="fork\nupstream\n", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote] + assert resolve_remote("/repo", remote=None) == "upstream" + # Immediate return when upstream remote is detected without extra subprocesses + assert mock_run.call_count == 1 + + +def test_resolve_remote_no_remotes(): + mock_remote = MagicMock(returncode=1, stdout="", stderr="") + with patch("subprocess.run", return_value=mock_remote): + assert resolve_remote("/repo", remote=None) == "origin" + + +def test_resolve_remote_fork_workflow_prefers_upstream(): + mock_remote = MagicMock(returncode=0, stdout="origin\nupstream\n", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote] + # In a fork repository, upstream is preferred for PR/MR reviews + assert resolve_remote("/repo", remote=None) == "upstream" + assert mock_run.call_count == 1 + + +def test_resolve_remote_fork_workflow_origin_tracking_overridden_by_upstream(): + # Even if current branch tracks origin, upstream remote takes precedence for reviews + mock_remote = MagicMock(returncode=0, stdout="origin\nupstream\n", stderr="") + + with patch("subprocess.run") as mock_run: + mock_run.side_effect = [mock_remote] + assert resolve_remote("/repo", remote=None) == "upstream" + assert mock_run.call_count == 1 + + +def test_resolve_remote_fork_workflow_explicit_override(): + # User can explicitly request origin or any other remote via argument + with patch("subprocess.run") as mock_run: + assert resolve_remote("/repo", remote="origin") == "origin" + mock_run.assert_not_called() + + +def test_review_github_pr_auto_detect_gh_remote(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") as mock_send, + 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, + ] + + # 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] == [ + "git", + "-C", + repo_path, + "fetch", + "--", + "gh", + "+refs/pull/789/head:refs/vimini/pr/789", + ] + mock_send.assert_called_once()