From 9c3d9f5482c6888dc19c38fd881b8f1413b25e0a Mon Sep 17 00:00:00 2001 From: Ben Thomasson Date: Fri, 26 Jun 2026 11:34:45 -0400 Subject: [PATCH 1/2] Add GitLab merge request support via glab CLI Auto-detect GitHub vs GitLab from PR/MR URLs, GITLAB_HOST env var, or git remote. All existing public API functions (get_pr_diff, post_pr_comment, fetch_pr_locally, etc.) dispatch internally so the CLI layer needs no functional changes. Co-Authored-By: Claude --- src/ftl_code_review/cli.py | 32 +- src/ftl_code_review/git_utils.py | 497 ++++++++++++++++++++++--------- tests/test_git_utils.py | 274 +++++++++++++++++ 3 files changed, 641 insertions(+), 162 deletions(-) create mode 100644 tests/test_git_utils.py diff --git a/src/ftl_code_review/cli.py b/src/ftl_code_review/cli.py index 64cb133..fe55aa3 100644 --- a/src/ftl_code_review/cli.py +++ b/src/ftl_code_review/cli.py @@ -74,7 +74,7 @@ def cli(): @click.option( "--pr", default=None, - help="GitHub PR to review (URL, owner/repo#N, or number)", + help="PR/MR to review (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--repo", @@ -130,13 +130,13 @@ def cli(): @click.option( "--github-issue", default=None, - help="GitHub issue to check changes against (URL, owner/repo#N, or number)", + help="Issue to check against (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--comment", is_flag=True, default=False, - help="Post review as a comment on the PR (requires --pr)", + help="Post review as a comment on the PR/MR (requires --pr)", ) @click.option( "--timeout", @@ -222,9 +222,9 @@ def review(branch, base, pr, repo, spec, model, output, output_dir, observations if github_issue: try: issue_content = get_github_issue(github_issue) - click.echo(f"Fetched GitHub issue {github_issue}", err=True) + click.echo(f"Fetched issue {github_issue}", err=True) except (RuntimeError, ValueError) as e: - click.echo(f"Warning: Could not fetch GitHub issue: {e}", err=True) + click.echo(f"Warning: Could not fetch issue: {e}", err=True) elif issue: issue_content = read_file_content(issue) if issue_content: @@ -399,7 +399,7 @@ def observe(branch, base, repo, model, output, run): @click.option( "--pr", default=None, - help="GitHub PR to review (URL, owner/repo#N, or number)", + help="PR/MR to review (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--repo", @@ -453,13 +453,13 @@ def observe(branch, base, repo, model, output, run): @click.option( "--github-issue", default=None, - help="GitHub issue to check changes against (URL, owner/repo#N, or number)", + help="Issue to check against (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--comment", is_flag=True, default=False, - help="Post review as a comment on the PR (requires --pr)", + help="Post review as a comment on the PR/MR (requires --pr)", ) def gate(branch, base, pr, repo, spec, model, output_dir, lint, fix_lint, beliefs, issue, github_issue, comment): """ @@ -553,9 +553,9 @@ def gate(branch, base, pr, repo, spec, model, output_dir, lint, fix_lint, belief if github_issue: try: issue_content = get_github_issue(github_issue) - click.echo(f"Fetched GitHub issue {github_issue}", err=True) + click.echo(f"Fetched issue {github_issue}", err=True) except (RuntimeError, ValueError) as e: - click.echo(f"Warning: Could not fetch GitHub issue: {e}", err=True) + click.echo(f"Warning: Could not fetch issue: {e}", err=True) elif issue: issue_content = read_file_content(issue) if issue_content: @@ -623,7 +623,7 @@ def gate(branch, base, pr, repo, spec, model, output_dir, lint, fix_lint, belief @click.option( "--pr", default=None, - help="GitHub PR to review (URL, owner/repo#N, or number)", + help="PR/MR to review (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--repo", @@ -859,7 +859,7 @@ def models(): @click.option( "--pr", default=None, - help="GitHub PR to review (URL, owner/repo#N, or number)", + help="PR/MR to review (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--repo", @@ -915,13 +915,13 @@ def models(): @click.option( "--github-issue", default=None, - help="GitHub issue to check changes against (URL, owner/repo#N, or number)", + help="Issue to check against (GitHub/GitLab URL, owner/repo#N, or number)", ) @click.option( "--comment", is_flag=True, default=False, - help="Post review as a comment on the PR (requires --pr)", + help="Post review as a comment on the PR/MR (requires --pr)", ) @click.option( "--run-tests/--no-run-tests", @@ -1014,9 +1014,9 @@ def review_loop(branch, base, pr, repo, spec, model, output, output_dir, max_ite if github_issue: try: issue_content = get_github_issue(github_issue) - click.echo(f"Fetched GitHub issue {github_issue}", err=True) + click.echo(f"Fetched issue {github_issue}", err=True) except (RuntimeError, ValueError) as e: - click.echo(f"Warning: Could not fetch GitHub issue: {e}", err=True) + click.echo(f"Warning: Could not fetch issue: {e}", err=True) elif issue: issue_content = read_file_content(issue) if issue_content: diff --git a/src/ftl_code_review/git_utils.py b/src/ftl_code_review/git_utils.py index 8af2ee5..ea4d6aa 100644 --- a/src/ftl_code_review/git_utils.py +++ b/src/ftl_code_review/git_utils.py @@ -1,7 +1,61 @@ """Git utilities for extracting diffs.""" +import json +import os import re import subprocess +from enum import Enum + + +class Platform(Enum): + GITHUB = "github" + GITLAB = "gitlab" + + +def _detect_platform_from_remote() -> Platform: + """Detect platform from the current repo's git remote URL.""" + result = subprocess.run( + ["git", "remote", "get-url", "origin"], + capture_output=True, text=True, + ) + if result.returncode != 0: + return Platform.GITHUB + + remote_url = result.stdout.strip() + gitlab_host = os.environ.get("GITLAB_HOST", "gitlab.com") + if "gitlab" in remote_url.lower() or gitlab_host in remote_url: + return Platform.GITLAB + return Platform.GITHUB + + +def detect_platform(ref: str) -> Platform: + """Detect whether a PR/MR/issue reference points to GitHub or GitLab. + + Detection order: + 1. URL contains github.com → GITHUB + 2. URL contains /-/merge_requests/ or /-/issues/ → GITLAB + 3. URL contains gitlab.com or GITLAB_HOST → GITLAB + 4. Shorthand or bare number → check git remote + """ + if "github.com" in ref: + return Platform.GITHUB + + if "/-/merge_requests/" in ref or "/-/issues/" in ref: + return Platform.GITLAB + + gitlab_host = os.environ.get("GITLAB_HOST", "gitlab.com") + if "gitlab.com" in ref or (gitlab_host != "gitlab.com" and gitlab_host in ref): + return Platform.GITLAB + + if re.match(r"https?://", ref): + return Platform.GITHUB + + return _detect_platform_from_remote() + + +# --------------------------------------------------------------------------- +# Local git operations (platform-independent) +# --------------------------------------------------------------------------- def get_diff( @@ -10,8 +64,7 @@ def get_diff( cwd: str | None = None, context_lines: int = 10, ) -> str: - """ - Get git diff for review. + """Get git diff for review. Args: ref: Branch or commit to diff. If None, uses staged changes. @@ -28,13 +81,9 @@ def get_diff( context_arg = f"-U{context_lines}" if ref is None: - # Staged changes cmd = ["git", "diff", "--staged", context_arg] else: - # Diff between base and ref - # Default to origin/main to avoid stale local main issues if base is None: - # Check if origin/main exists, fall back to main check = subprocess.run( ["git", "rev-parse", "--verify", "origin/main"], capture_output=True, @@ -52,15 +101,7 @@ def get_diff( def read_file_content(path: str) -> str | None: - """ - Read file content, returning None if file doesn't exist. - - Args: - path: Path to file - - Returns: - File content or None - """ + """Read file content, returning None if file doesn't exist.""" try: with open(path, encoding="utf-8") as f: return f.read() @@ -68,29 +109,21 @@ def read_file_content(path: str) -> str | None: return None -def parse_pr_url(pr_ref: str) -> tuple[str | None, int]: - """ - Parse a PR reference into (repo, number). +# --------------------------------------------------------------------------- +# GitHub implementation (private) +# --------------------------------------------------------------------------- - Accepts: - - Full URL: https://github.com/owner/repo/pull/123 - - Shorthand: owner/repo#123 - - Number only: 123 (repo=None, uses current repo) - Returns: - (repo, pr_number) where repo is "owner/repo" or None - """ - # Full GitHub URL +def _parse_github_pr_url(pr_ref: str) -> tuple[str | None, int]: + """Parse a GitHub PR reference into (repo, number).""" m = re.match(r"https?://github\.com/([^/]+/[^/]+)/pull/(\d+)", pr_ref) if m: return m.group(1), int(m.group(2)) - # Shorthand: owner/repo#123 m = re.match(r"([^/]+/[^#]+)#(\d+)", pr_ref) if m: return m.group(1), int(m.group(2)) - # Plain number m = re.match(r"(\d+)$", pr_ref) if m: return None, int(m.group(1)) @@ -98,22 +131,10 @@ def parse_pr_url(pr_ref: str) -> tuple[str | None, int]: raise ValueError(f"Cannot parse PR reference: {pr_ref}") -def get_pr_diff(pr_ref: str) -> tuple[str, str, str]: - """ - Get diff for a GitHub PR using gh CLI. +def _get_github_pr_diff(pr_ref: str) -> tuple[str, str, str]: + """Get diff for a GitHub PR using gh CLI.""" + repo, pr_number = _parse_github_pr_url(pr_ref) - Args: - pr_ref: PR URL, owner/repo#N, or number - - Returns: - (diff_content, diff_ref, repo) where diff_ref is "owner/repo#N" - - Raises: - RuntimeError: If gh command fails - """ - repo, pr_number = parse_pr_url(pr_ref) - - # Build gh command cmd = ["gh", "pr", "diff", str(pr_number)] if repo: cmd.extend(["--repo", repo]) @@ -122,15 +143,12 @@ def get_pr_diff(pr_ref: str) -> tuple[str, str, str]: if result.returncode != 0: raise RuntimeError(f"gh pr diff failed: {result.stderr.strip()}") - # Build diff_ref label if repo: diff_ref = f"{repo}#{pr_number}" else: - # Try to get repo name from gh info_cmd = ["gh", "pr", "view", str(pr_number), "--json", "url"] info_result = subprocess.run(info_cmd, capture_output=True, text=True) if info_result.returncode == 0: - import json url = json.loads(info_result.stdout).get("url", "") m = re.match(r"https?://github\.com/([^/]+/[^/]+)/pull/\d+", url) diff_ref = f"{m.group(1)}#{pr_number}" if m else f"PR #{pr_number}" @@ -140,25 +158,10 @@ def get_pr_diff(pr_ref: str) -> tuple[str, str, str]: return result.stdout, diff_ref, repo or "" -def fetch_pr_locally(pr_ref: str, cwd: str) -> tuple[str, str, str]: - """ - Fetch a PR's branch into a local repo and check it out. - - Args: - pr_ref: PR URL, owner/repo#N, or number - cwd: Local repo directory - - Returns: - (head_branch, base_branch, diff_ref) — ready for get_diff() - - Raises: - RuntimeError: If gh or git commands fail - """ - import json +def _fetch_github_pr_locally(pr_ref: str, cwd: str) -> tuple[str, str, str]: + """Fetch a GitHub PR's branch into a local repo and check it out.""" + repo, pr_number = _parse_github_pr_url(pr_ref) - repo, pr_number = parse_pr_url(pr_ref) - - # Get PR metadata cmd = ["gh", "pr", "view", str(pr_number), "--json", "headRefName,baseRefName,url"] if repo: cmd.extend(["--repo", repo]) @@ -171,7 +174,6 @@ def fetch_pr_locally(pr_ref: str, cwd: str) -> tuple[str, str, str]: head_branch = data["headRefName"] base_branch = data["baseRefName"] - # Build diff_ref label if repo: diff_ref = f"{repo}#{pr_number}" else: @@ -179,107 +181,303 @@ def fetch_pr_locally(pr_ref: str, cwd: str) -> tuple[str, str, str]: m = re.match(r"https?://github\.com/([^/]+/[^/]+)/pull/\d+", url) diff_ref = f"{m.group(1)}#{pr_number}" if m else f"PR #{pr_number}" - # Fetch and checkout the PR branch + _checkout_branch(head_branch, cwd) + + return head_branch, base_branch, diff_ref + + +def _post_github_pr_comment(pr_ref: str, body: str) -> None: + """Post a comment on a GitHub PR using gh CLI.""" + repo, pr_number = _parse_github_pr_url(pr_ref) + + cmd = ["gh", "pr", "comment", str(pr_number), "--body", body] + if repo: + cmd.extend(["--repo", repo]) + + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"gh pr comment failed: {result.stderr.strip()}") + + +def _parse_github_issue_url(issue_ref: str) -> tuple[str | None, int]: + """Parse a GitHub issue reference into (repo, number).""" + m = re.match(r"https?://github\.com/([^/]+/[^/]+)/issues/(\d+)", issue_ref) + if m: + return m.group(1), int(m.group(2)) + + m = re.match(r"([^/]+/[^#]+)#(\d+)", issue_ref) + if m: + return m.group(1), int(m.group(2)) + + m = re.match(r"(\d+)$", issue_ref) + if m: + return None, int(m.group(1)) + + raise ValueError(f"Cannot parse issue reference: {issue_ref}") + + +def _get_github_issue_impl(issue_ref: str) -> str: + """Fetch a GitHub issue's title and body using gh CLI.""" + repo, issue_number = _parse_github_issue_url(issue_ref) + + cmd = ["gh", "issue", "view", str(issue_number), "--json", "title,body"] + if repo: + cmd.extend(["--repo", repo]) + + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"gh issue view failed: {result.stderr.strip()}") + + data = json.loads(result.stdout) + title = data.get("title", "") + body = data.get("body", "") or "" + + return f"## {title}\n\n{body}" + + +# --------------------------------------------------------------------------- +# GitLab implementation (private) +# --------------------------------------------------------------------------- + + +def _parse_gitlab_mr_url(mr_ref: str) -> tuple[str | None, int]: + """Parse a GitLab MR reference into (repo, number). + + Accepts: + - Full URL: https://gitlab.com/owner/repo/-/merge_requests/123 + - Full URL with nested groups: https://gitlab.example.com/group/sub/repo/-/merge_requests/5 + - Shorthand: owner/repo#123 (when platform already detected as GitLab) + - Number only: 123 + """ + m = re.match(r"https?://[^/]+/(.+?)/-/merge_requests/(\d+)", mr_ref) + if m: + return m.group(1), int(m.group(2)) + + m = re.match(r"([^/]+/[^#]+)#(\d+)", mr_ref) + if m: + return m.group(1), int(m.group(2)) + + m = re.match(r"(\d+)$", mr_ref) + if m: + return None, int(m.group(1)) + + raise ValueError(f"Cannot parse MR reference: {mr_ref}") + + +def _get_gitlab_mr_diff(mr_ref: str) -> tuple[str, str, str]: + """Get diff for a GitLab MR using glab CLI.""" + repo, mr_number = _parse_gitlab_mr_url(mr_ref) + + cmd = ["glab", "mr", "diff", str(mr_number), "--color=never"] + if repo: + cmd.extend(["--repo", repo]) + + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"glab mr diff failed: {result.stderr.strip()}") + + if repo: + diff_ref = f"{repo}!{mr_number}" + else: + info_cmd = ["glab", "mr", "view", str(mr_number), "--output", "json"] + info_result = subprocess.run(info_cmd, capture_output=True, text=True) + if info_result.returncode == 0: + data = json.loads(info_result.stdout) + web_url = data.get("web_url", "") + m = re.match(r"https?://[^/]+/(.+?)/-/merge_requests/\d+", web_url) + diff_ref = f"{m.group(1)}!{mr_number}" if m else f"MR !{mr_number}" + else: + diff_ref = f"MR !{mr_number}" + + return result.stdout, diff_ref, repo or "" + + +def _fetch_gitlab_mr_locally(mr_ref: str, cwd: str) -> tuple[str, str, str]: + """Fetch a GitLab MR's branch into a local repo and check it out.""" + repo, mr_number = _parse_gitlab_mr_url(mr_ref) + + cmd = ["glab", "mr", "view", str(mr_number), "--output", "json"] + if repo: + cmd.extend(["--repo", repo]) + + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"glab mr view failed: {result.stderr.strip()}") + + data = json.loads(result.stdout) + head_branch = data["source_branch"] + base_branch = data["target_branch"] + + if repo: + diff_ref = f"{repo}!{mr_number}" + else: + web_url = data.get("web_url", "") + m = re.match(r"https?://[^/]+/(.+?)/-/merge_requests/\d+", web_url) + diff_ref = f"{m.group(1)}!{mr_number}" if m else f"MR !{mr_number}" + + _checkout_branch(head_branch, cwd) + + return head_branch, base_branch, diff_ref + + +def _post_gitlab_mr_comment(mr_ref: str, body: str) -> None: + """Post a comment on a GitLab MR using glab CLI.""" + repo, mr_number = _parse_gitlab_mr_url(mr_ref) + + cmd = ["glab", "mr", "note", str(mr_number), "--message", body] + if repo: + cmd.extend(["--repo", repo]) + + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"glab mr note failed: {result.stderr.strip()}") + + +def _parse_gitlab_issue_url(issue_ref: str) -> tuple[str | None, int]: + """Parse a GitLab issue reference into (repo, number).""" + m = re.match(r"https?://[^/]+/(.+?)/-/issues/(\d+)", issue_ref) + if m: + return m.group(1), int(m.group(2)) + + m = re.match(r"([^/]+/[^#]+)#(\d+)", issue_ref) + if m: + return m.group(1), int(m.group(2)) + + m = re.match(r"(\d+)$", issue_ref) + if m: + return None, int(m.group(1)) + + raise ValueError(f"Cannot parse issue reference: {issue_ref}") + + +def _get_gitlab_issue_impl(issue_ref: str) -> str: + """Fetch a GitLab issue's title and description using glab CLI.""" + repo, issue_number = _parse_gitlab_issue_url(issue_ref) + + cmd = ["glab", "issue", "view", str(issue_number), "--output", "json"] + if repo: + cmd.extend(["--repo", repo]) + + result = subprocess.run(cmd, capture_output=True, text=True) + if result.returncode != 0: + raise RuntimeError(f"glab issue view failed: {result.stderr.strip()}") + + data = json.loads(result.stdout) + title = data.get("title", "") + description = data.get("description", "") or "" + + return f"## {title}\n\n{description}" + + +# --------------------------------------------------------------------------- +# Shared helpers +# --------------------------------------------------------------------------- + + +def _checkout_branch(branch: str, cwd: str) -> None: + """Fetch and checkout a branch in a local repo.""" subprocess.run( - ["git", "fetch", "origin", head_branch], + ["git", "fetch", "origin", branch], cwd=cwd, capture_output=True, text=True, ) - # Check if branch exists locally check = subprocess.run( - ["git", "rev-parse", "--verify", head_branch], + ["git", "rev-parse", "--verify", branch], cwd=cwd, capture_output=True, ) if check.returncode == 0: - # Branch exists, update it subprocess.run( - ["git", "checkout", head_branch], + ["git", "checkout", branch], cwd=cwd, capture_output=True, ) subprocess.run( - ["git", "pull", "--ff-only", "origin", head_branch], + ["git", "pull", "--ff-only", "origin", branch], cwd=cwd, capture_output=True, ) else: - # Create tracking branch subprocess.run( - ["git", "checkout", "-b", head_branch, f"origin/{head_branch}"], + ["git", "checkout", "-b", branch, f"origin/{branch}"], cwd=cwd, capture_output=True, ) - return head_branch, base_branch, diff_ref +# --------------------------------------------------------------------------- +# Public API (dispatch based on platform detection) +# --------------------------------------------------------------------------- -def pr_output_dir_name(pr_ref: str) -> str: - """ - Generate a clean output directory name from a PR reference. - Examples: - "https://github.com/owner/repo/pull/123" -> "owner-repo-123" - "owner/repo#123" -> "owner-repo-123" - "123" -> "pr-123" - """ - repo, pr_number = parse_pr_url(pr_ref) - if repo: - return f"{repo.replace('/', '-')}-{pr_number}" - return f"pr-{pr_number}" +def parse_pr_url(pr_ref: str) -> tuple[str | None, int]: + """Parse a PR/MR reference into (repo, number). Works for GitHub and GitLab.""" + platform = detect_platform(pr_ref) + if platform == Platform.GITLAB: + return _parse_gitlab_mr_url(pr_ref) + return _parse_github_pr_url(pr_ref) -def post_pr_comment(pr_ref: str, body: str) -> None: - """ - Post a comment on a GitHub PR using gh CLI. +def get_pr_diff(pr_ref: str) -> tuple[str, str, str]: + """Get diff for a GitHub PR or GitLab MR. Args: - pr_ref: PR URL, owner/repo#N, or number - body: Comment body (markdown) + pr_ref: PR/MR URL, owner/repo#N, or number + + Returns: + (diff_content, diff_ref, repo) Raises: - RuntimeError: If gh command fails + RuntimeError: If CLI command fails """ - repo, pr_number = parse_pr_url(pr_ref) + platform = detect_platform(pr_ref) + if platform == Platform.GITLAB: + return _get_gitlab_mr_diff(pr_ref) + return _get_github_pr_diff(pr_ref) - cmd = ["gh", "pr", "comment", str(pr_number), "--body", body] - if repo: - cmd.extend(["--repo", repo]) - result = subprocess.run(cmd, capture_output=True, text=True) - if result.returncode != 0: - raise RuntimeError(f"gh pr comment failed: {result.stderr.strip()}") +def fetch_pr_locally(pr_ref: str, cwd: str) -> tuple[str, str, str]: + """Fetch a PR/MR's branch into a local repo and check it out. + Args: + pr_ref: PR/MR URL, owner/repo#N, or number + cwd: Local repo directory -def parse_issue_ref(issue_ref: str) -> tuple[str | None, int]: + Returns: + (head_branch, base_branch, diff_ref) + + Raises: + RuntimeError: If CLI or git commands fail """ - Parse a GitHub issue reference into (repo, number). + platform = detect_platform(pr_ref) + if platform == Platform.GITLAB: + return _fetch_gitlab_mr_locally(pr_ref, cwd) + return _fetch_github_pr_locally(pr_ref, cwd) - Accepts: - - Full URL: https://github.com/owner/repo/issues/123 - - Shorthand: owner/repo#123 - - Number only: 123 (repo=None, uses current repo) - Returns: - (repo, issue_number) where repo is "owner/repo" or None - """ - # Full GitHub URL - m = re.match(r"https?://github\.com/([^/]+/[^/]+)/issues/(\d+)", issue_ref) - if m: - return m.group(1), int(m.group(2)) +def post_pr_comment(pr_ref: str, body: str) -> None: + """Post a comment on a GitHub PR or GitLab MR. - # Shorthand: owner/repo#123 - m = re.match(r"([^/]+/[^#]+)#(\d+)", issue_ref) - if m: - return m.group(1), int(m.group(2)) + Args: + pr_ref: PR/MR URL, owner/repo#N, or number + body: Comment body (markdown) - # Plain number - m = re.match(r"(\d+)$", issue_ref) - if m: - return None, int(m.group(1)) + Raises: + RuntimeError: If CLI command fails + """ + platform = detect_platform(pr_ref) + if platform == Platform.GITLAB: + _post_gitlab_mr_comment(pr_ref, body) + else: + _post_github_pr_comment(pr_ref, body) - raise ValueError(f"Cannot parse issue reference: {issue_ref}") + +def parse_issue_ref(issue_ref: str) -> tuple[str | None, int]: + """Parse an issue reference into (repo, number). Works for GitHub and GitLab.""" + platform = detect_platform(issue_ref) + if platform == Platform.GITLAB: + return _parse_gitlab_issue_url(issue_ref) + return _parse_github_issue_url(issue_ref) def get_github_issue(issue_ref: str) -> str: - """ - Fetch a GitHub issue's title and body using gh CLI. + """Fetch an issue's title and body. Works for GitHub and GitLab. Args: issue_ref: Issue URL, owner/repo#N, or number @@ -288,30 +486,41 @@ def get_github_issue(issue_ref: str) -> str: Formatted issue content (title + body) Raises: - RuntimeError: If gh command fails + RuntimeError: If CLI command fails """ - import json + platform = detect_platform(issue_ref) + if platform == Platform.GITLAB: + return _get_gitlab_issue_impl(issue_ref) + return _get_github_issue_impl(issue_ref) - repo, issue_number = parse_issue_ref(issue_ref) - cmd = ["gh", "issue", "view", str(issue_number), "--json", "title,body"] - if repo: - cmd.extend(["--repo", repo]) +def pr_output_dir_name(pr_ref: str) -> str: + """Generate a clean output directory name from a PR/MR reference. - result = subprocess.run(cmd, capture_output=True, text=True) - if result.returncode != 0: - raise RuntimeError(f"gh issue view failed: {result.stderr.strip()}") + Examples: + "https://github.com/owner/repo/pull/123" -> "owner-repo-123" + "https://gitlab.com/owner/repo/-/merge_requests/123" -> "owner-repo-123" + "123" -> "pr-123" (GitHub) or "mr-123" (GitLab) + """ + platform = detect_platform(pr_ref) + if platform == Platform.GITLAB: + repo, mr_number = _parse_gitlab_mr_url(pr_ref) + if repo: + return f"{repo.replace('/', '-')}-{mr_number}" + return f"mr-{mr_number}" + repo, pr_number = _parse_github_pr_url(pr_ref) + if repo: + return f"{repo.replace('/', '-')}-{pr_number}" + return f"pr-{pr_number}" - data = json.loads(result.stdout) - title = data.get("title", "") - body = data.get("body", "") or "" - return f"## {title}\n\n{body}" +# --------------------------------------------------------------------------- +# Diff parsing utilities (platform-independent) +# --------------------------------------------------------------------------- def extract_changed_files(diff_content: str) -> list[str]: - """ - Extract file paths from a git diff. + """Extract file paths from a git diff. Args: diff_content: Git diff output @@ -320,18 +529,16 @@ def extract_changed_files(diff_content: str) -> list[str]: List of file paths that were changed """ files = [] - # Match "+++ b/path/to/file" lines for line in diff_content.split("\n"): if line.startswith("+++ b/"): - path = line[6:] # Remove "+++ b/" prefix - if path != "/dev/null": # Exclude deleted files + path = line[6:] + if path != "/dev/null": files.append(path) return files def extract_modified_line_ranges(diff_content: str) -> dict[str, list[tuple[int, int]]]: - """ - Parse a unified diff to extract modified line ranges in the new version of each file. + """Parse a unified diff to extract modified line ranges in the new version of each file. Parses ``@@ -a,b +c,d @@`` hunk headers to determine which lines were touched. Returns ranges in the **new** file (the ``+`` side). @@ -361,13 +568,11 @@ def extract_modified_line_ranges(diff_content: str) -> dict[str, list[tuple[int, current_file: str | None = None for line in diff_content.split("\n"): - # Track current file from "+++ b/..." lines if line.startswith("+++ b/"): path = line[6:] current_file = path if path != "/dev/null" else None continue - # Parse hunk headers: @@ -old_start,old_count +new_start,new_count @@ if current_file and line.startswith("@@"): m = re.match(r"@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", line) if m: diff --git a/tests/test_git_utils.py b/tests/test_git_utils.py new file mode 100644 index 0000000..4667d5d --- /dev/null +++ b/tests/test_git_utils.py @@ -0,0 +1,274 @@ +"""Tests for git_utils platform detection, parsing, and dispatch.""" + +from unittest.mock import MagicMock, patch + +import pytest + +from ftl_code_review.git_utils import ( + Platform, + _parse_github_pr_url, + _parse_gitlab_mr_url, + _parse_github_issue_url, + _parse_gitlab_issue_url, + detect_platform, + get_pr_diff, + fetch_pr_locally, + post_pr_comment, + get_github_issue, + parse_pr_url, + parse_issue_ref, + pr_output_dir_name, +) + + +# --------------------------------------------------------------------------- +# detect_platform +# --------------------------------------------------------------------------- + + +class TestDetectPlatform: + def test_github_pr_url(self): + assert detect_platform("https://github.com/owner/repo/pull/123") == Platform.GITHUB + + def test_github_issue_url(self): + assert detect_platform("https://github.com/owner/repo/issues/42") == Platform.GITHUB + + def test_gitlab_mr_url(self): + assert detect_platform("https://gitlab.com/owner/repo/-/merge_requests/123") == Platform.GITLAB + + def test_gitlab_issue_url(self): + assert detect_platform("https://gitlab.com/owner/repo/-/issues/42") == Platform.GITLAB + + def test_gitlab_nested_group_url(self): + assert detect_platform("https://gitlab.com/group/sub/repo/-/merge_requests/5") == Platform.GITLAB + + def test_self_hosted_gitlab_mr_url(self): + assert detect_platform("https://git.company.com/team/project/-/merge_requests/99") == Platform.GITLAB + + def test_self_hosted_gitlab_via_env(self): + with patch.dict("os.environ", {"GITLAB_HOST": "git.internal.co"}): + assert detect_platform("https://git.internal.co/team/repo/something") == Platform.GITLAB + + def test_unknown_url_defaults_github(self): + assert detect_platform("https://bitbucket.org/owner/repo/pull-requests/1") == Platform.GITHUB + + @patch("ftl_code_review.git_utils._detect_platform_from_remote") + def test_bare_number_checks_remote(self, mock_remote): + mock_remote.return_value = Platform.GITLAB + assert detect_platform("123") == Platform.GITLAB + mock_remote.assert_called_once() + + @patch("ftl_code_review.git_utils._detect_platform_from_remote") + def test_shorthand_checks_remote(self, mock_remote): + mock_remote.return_value = Platform.GITHUB + assert detect_platform("owner/repo#42") == Platform.GITHUB + mock_remote.assert_called_once() + + +class TestDetectPlatformFromRemote: + @patch("ftl_code_review.git_utils.subprocess") + def test_github_remote(self, mock_sub): + mock_sub.run.return_value = MagicMock(returncode=0, stdout="https://github.com/o/r.git\n") + from ftl_code_review.git_utils import _detect_platform_from_remote + assert _detect_platform_from_remote() == Platform.GITHUB + + @patch("ftl_code_review.git_utils.subprocess") + def test_gitlab_remote(self, mock_sub): + mock_sub.run.return_value = MagicMock(returncode=0, stdout="git@gitlab.com:group/repo.git\n") + from ftl_code_review.git_utils import _detect_platform_from_remote + assert _detect_platform_from_remote() == Platform.GITLAB + + @patch("ftl_code_review.git_utils.subprocess") + def test_no_remote_defaults_github(self, mock_sub): + mock_sub.run.return_value = MagicMock(returncode=1, stdout="") + from ftl_code_review.git_utils import _detect_platform_from_remote + assert _detect_platform_from_remote() == Platform.GITHUB + + @patch("ftl_code_review.git_utils.subprocess") + def test_gitlab_host_env(self, mock_sub): + mock_sub.run.return_value = MagicMock(returncode=0, stdout="git@git.internal.co:team/repo.git\n") + from ftl_code_review.git_utils import _detect_platform_from_remote + with patch.dict("os.environ", {"GITLAB_HOST": "git.internal.co"}): + assert _detect_platform_from_remote() == Platform.GITLAB + + +# --------------------------------------------------------------------------- +# GitHub URL parsing +# --------------------------------------------------------------------------- + + +class TestParseGitHubPrUrl: + def test_full_url(self): + assert _parse_github_pr_url("https://github.com/owner/repo/pull/123") == ("owner/repo", 123) + + def test_shorthand(self): + assert _parse_github_pr_url("owner/repo#42") == ("owner/repo", 42) + + def test_number_only(self): + assert _parse_github_pr_url("7") == (None, 7) + + def test_invalid_raises(self): + with pytest.raises(ValueError): + _parse_github_pr_url("not-a-ref") + + +class TestParseGitHubIssueUrl: + def test_full_url(self): + assert _parse_github_issue_url("https://github.com/owner/repo/issues/99") == ("owner/repo", 99) + + def test_shorthand(self): + assert _parse_github_issue_url("owner/repo#10") == ("owner/repo", 10) + + def test_number_only(self): + assert _parse_github_issue_url("5") == (None, 5) + + def test_invalid_raises(self): + with pytest.raises(ValueError): + _parse_github_issue_url("bad-ref!") + + +# --------------------------------------------------------------------------- +# GitLab URL parsing +# --------------------------------------------------------------------------- + + +class TestParseGitLabMrUrl: + def test_full_url(self): + assert _parse_gitlab_mr_url("https://gitlab.com/owner/repo/-/merge_requests/123") == ("owner/repo", 123) + + def test_nested_group_url(self): + assert _parse_gitlab_mr_url("https://gitlab.com/group/subgroup/repo/-/merge_requests/5") == ("group/subgroup/repo", 5) + + def test_self_hosted_url(self): + assert _parse_gitlab_mr_url("https://git.company.com/team/project/-/merge_requests/99") == ("team/project", 99) + + def test_shorthand(self): + assert _parse_gitlab_mr_url("owner/repo#42") == ("owner/repo", 42) + + def test_number_only(self): + assert _parse_gitlab_mr_url("7") == (None, 7) + + def test_invalid_raises(self): + with pytest.raises(ValueError): + _parse_gitlab_mr_url("not-a-ref") + + +class TestParseGitLabIssueUrl: + def test_full_url(self): + assert _parse_gitlab_issue_url("https://gitlab.com/owner/repo/-/issues/42") == ("owner/repo", 42) + + def test_nested_group_url(self): + assert _parse_gitlab_issue_url("https://gitlab.com/group/sub/repo/-/issues/10") == ("group/sub/repo", 10) + + def test_shorthand(self): + assert _parse_gitlab_issue_url("owner/repo#5") == ("owner/repo", 5) + + def test_number_only(self): + assert _parse_gitlab_issue_url("3") == (None, 3) + + +# --------------------------------------------------------------------------- +# Dispatch: public API routes to correct implementation +# --------------------------------------------------------------------------- + + +class TestParseDispatch: + def test_github_url_dispatches(self): + repo, num = parse_pr_url("https://github.com/owner/repo/pull/123") + assert repo == "owner/repo" + assert num == 123 + + def test_gitlab_url_dispatches(self): + repo, num = parse_pr_url("https://gitlab.com/group/repo/-/merge_requests/45") + assert repo == "group/repo" + assert num == 45 + + def test_issue_github_dispatches(self): + repo, num = parse_issue_ref("https://github.com/owner/repo/issues/10") + assert repo == "owner/repo" + assert num == 10 + + def test_issue_gitlab_dispatches(self): + repo, num = parse_issue_ref("https://gitlab.com/owner/repo/-/issues/10") + assert repo == "owner/repo" + assert num == 10 + + +class TestGetPrDiffDispatch: + @patch("ftl_code_review.git_utils._get_github_pr_diff") + def test_github_url(self, mock_gh): + mock_gh.return_value = ("diff", "o/r#1", "o/r") + result = get_pr_diff("https://github.com/o/r/pull/1") + mock_gh.assert_called_once_with("https://github.com/o/r/pull/1") + assert result == ("diff", "o/r#1", "o/r") + + @patch("ftl_code_review.git_utils._get_gitlab_mr_diff") + def test_gitlab_url(self, mock_gl): + mock_gl.return_value = ("diff", "o/r!1", "o/r") + result = get_pr_diff("https://gitlab.com/o/r/-/merge_requests/1") + mock_gl.assert_called_once_with("https://gitlab.com/o/r/-/merge_requests/1") + assert result == ("diff", "o/r!1", "o/r") + + +class TestFetchPrLocallyDispatch: + @patch("ftl_code_review.git_utils._fetch_github_pr_locally") + def test_github_url(self, mock_gh): + mock_gh.return_value = ("feature", "main", "o/r#1") + result = fetch_pr_locally("https://github.com/o/r/pull/1", "/tmp") + mock_gh.assert_called_once_with("https://github.com/o/r/pull/1", "/tmp") + + @patch("ftl_code_review.git_utils._fetch_gitlab_mr_locally") + def test_gitlab_url(self, mock_gl): + mock_gl.return_value = ("feature", "main", "o/r!1") + result = fetch_pr_locally("https://gitlab.com/o/r/-/merge_requests/1", "/tmp") + mock_gl.assert_called_once_with("https://gitlab.com/o/r/-/merge_requests/1", "/tmp") + + +class TestPostPrCommentDispatch: + @patch("ftl_code_review.git_utils._post_github_pr_comment") + def test_github_url(self, mock_gh): + post_pr_comment("https://github.com/o/r/pull/1", "review text") + mock_gh.assert_called_once_with("https://github.com/o/r/pull/1", "review text") + + @patch("ftl_code_review.git_utils._post_gitlab_mr_comment") + def test_gitlab_url(self, mock_gl): + post_pr_comment("https://gitlab.com/o/r/-/merge_requests/1", "review text") + mock_gl.assert_called_once_with("https://gitlab.com/o/r/-/merge_requests/1", "review text") + + +class TestGetIssueDispatch: + @patch("ftl_code_review.git_utils._get_github_issue_impl") + def test_github_url(self, mock_gh): + mock_gh.return_value = "## Title\n\nbody" + result = get_github_issue("https://github.com/o/r/issues/1") + mock_gh.assert_called_once_with("https://github.com/o/r/issues/1") + + @patch("ftl_code_review.git_utils._get_gitlab_issue_impl") + def test_gitlab_url(self, mock_gl): + mock_gl.return_value = "## Title\n\ndescription" + result = get_github_issue("https://gitlab.com/o/r/-/issues/1") + mock_gl.assert_called_once_with("https://gitlab.com/o/r/-/issues/1") + + +# --------------------------------------------------------------------------- +# pr_output_dir_name +# --------------------------------------------------------------------------- + + +class TestPrOutputDirName: + def test_github_url(self): + assert pr_output_dir_name("https://github.com/owner/repo/pull/123") == "owner-repo-123" + + def test_gitlab_url(self): + assert pr_output_dir_name("https://gitlab.com/owner/repo/-/merge_requests/123") == "owner-repo-123" + + def test_gitlab_nested_group(self): + assert pr_output_dir_name("https://gitlab.com/group/sub/repo/-/merge_requests/5") == "group-sub-repo-5" + + @patch("ftl_code_review.git_utils._detect_platform_from_remote", return_value=Platform.GITHUB) + def test_github_bare_number(self, _): + assert pr_output_dir_name("42") == "pr-42" + + @patch("ftl_code_review.git_utils._detect_platform_from_remote", return_value=Platform.GITLAB) + def test_gitlab_bare_number(self, _): + assert pr_output_dir_name("42") == "mr-42" From 2cd8c258f27d4a6536e4de42eea2cdaf39006d49 Mon Sep 17 00:00:00 2001 From: Ben Thomasson Date: Fri, 26 Jun 2026 12:05:38 -0400 Subject: [PATCH 2/2] Fix test gaps from code review: add missing assertions and error case - Assert return values in FetchPrLocallyDispatch and GetIssueDispatch tests - Add test_invalid_raises for _parse_gitlab_issue_url Co-Authored-By: Claude --- tests/test_git_utils.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_git_utils.py b/tests/test_git_utils.py index 4667d5d..718a1dc 100644 --- a/tests/test_git_utils.py +++ b/tests/test_git_utils.py @@ -166,6 +166,10 @@ def test_shorthand(self): def test_number_only(self): assert _parse_gitlab_issue_url("3") == (None, 3) + def test_invalid_raises(self): + with pytest.raises(ValueError): + _parse_gitlab_issue_url("not-a-ref") + # --------------------------------------------------------------------------- # Dispatch: public API routes to correct implementation @@ -216,12 +220,14 @@ def test_github_url(self, mock_gh): mock_gh.return_value = ("feature", "main", "o/r#1") result = fetch_pr_locally("https://github.com/o/r/pull/1", "/tmp") mock_gh.assert_called_once_with("https://github.com/o/r/pull/1", "/tmp") + assert result == ("feature", "main", "o/r#1") @patch("ftl_code_review.git_utils._fetch_gitlab_mr_locally") def test_gitlab_url(self, mock_gl): mock_gl.return_value = ("feature", "main", "o/r!1") result = fetch_pr_locally("https://gitlab.com/o/r/-/merge_requests/1", "/tmp") mock_gl.assert_called_once_with("https://gitlab.com/o/r/-/merge_requests/1", "/tmp") + assert result == ("feature", "main", "o/r!1") class TestPostPrCommentDispatch: @@ -242,12 +248,14 @@ def test_github_url(self, mock_gh): mock_gh.return_value = "## Title\n\nbody" result = get_github_issue("https://github.com/o/r/issues/1") mock_gh.assert_called_once_with("https://github.com/o/r/issues/1") + assert result == "## Title\n\nbody" @patch("ftl_code_review.git_utils._get_gitlab_issue_impl") def test_gitlab_url(self, mock_gl): mock_gl.return_value = "## Title\n\ndescription" result = get_github_issue("https://gitlab.com/o/r/-/issues/1") mock_gl.assert_called_once_with("https://gitlab.com/o/r/-/issues/1") + assert result == "## Title\n\ndescription" # ---------------------------------------------------------------------------