diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 6fa0a70..21bf92b 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -7,24 +7,25 @@ on: jobs: review: runs-on: ubuntu-latest - services: - ollama: - image: ollama/ollama:latest - ports: - - 11434:11434 - options: >- - --health-cmd="curl -sSf http://192.168.1.92:11434/ || exit 1" --health-interval=10s --health-timeout=5s --health-retries=12 + container: + image: node:20-bookworm steps: + - name: Install system deps + Python 3.11 + run: | + apt-get update + apt-get install -y --no-install-recommends \ + git curl ca-certificates \ + python3.11 python3.11-venv python3-pip + ln -sf /usr/bin/python3.11 /usr/local/bin/python + python --version + node --version + git --version + - name: Checkout uses: actions/checkout@v4 - - name: Set up Python - uses: actions/setup-python@v4 - with: - python-version: '3.11' - - - name: Create venv and install + - name: Create venv and install project run: | python -m venv venv . venv/bin/activate @@ -33,44 +34,28 @@ jobs: - name: Wait for Ollama run: | - for i in $(seq 1 30); do - if curl -sSf http://192.168.1.92:11434/ >/dev/null 2>&1; then - echo "ollama ready" && break + for i in $(seq 1 60); do + if curl -sSf http://192.168.1.92:11434/api/tags >/dev/null 2>&1; then + echo "ollama ready" && exit 0 fi sleep 1 done - - - name: (Optional) Pull model into Ollama - run: | - . venv/bin/activate - ollama pull qwen2.5-coder:7b || true + echo "ollama not reachable" >&2 + exit 1 - name: Run ai-reviewer env: OLLAMA_HOST: http://192.168.1.92:11434 run: | . venv/bin/activate - ai-reviewer review --repo . --base "${{ github.event.pull_request.base.ref }}" --head "${{ github.head_ref }}" --format json > review.json + ai-reviewer review \ + --repo . \ + --base "${{ github.event.pull_request.base.ref }}" \ + --head "${{ github.head_ref }}" \ + --format json > review.json - - name: Post PR comment with findings - uses: actions/github-script@v6 - with: - script: | - const fs = require('fs'); - let body = '{}'; - try { - body = fs.readFileSync('review.json', 'utf8'); - } catch (e) { - body = JSON.stringify({ error: 'missing-review', message: String(e) }); - } - let parsed = {}; - try { parsed = JSON.parse(body); } catch (e) { parsed = { error: 'invalid-json', raw: body }; } - const findings = parsed.findings || []; - const summary = findings.length === 0 ? 'AI Reviewer: no findings.' : `AI Reviewer found ${findings.length} findings.`; - const commentBody = `${summary}\n\n
Full JSON\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n` + '```json\n' + JSON.stringify(parsed, null, 2) + '\n```\n
'; - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: commentBody, - }); + # Temporarily remove github-script; it also needs GitHub env + - name: Show result + run: | + ls -la + head -c 2000 review.json || true diff --git a/src/ai_reviewer/__pycache__/cli.cpython-311.pyc b/src/ai_reviewer/__pycache__/cli.cpython-311.pyc new file mode 100644 index 0000000..68d21ad Binary files /dev/null and b/src/ai_reviewer/__pycache__/cli.cpython-311.pyc differ diff --git a/src/ai_reviewer/__pycache__/diff.cpython-311.pyc b/src/ai_reviewer/__pycache__/diff.cpython-311.pyc index 646eced..9ae434f 100644 Binary files a/src/ai_reviewer/__pycache__/diff.cpython-311.pyc and b/src/ai_reviewer/__pycache__/diff.cpython-311.pyc differ diff --git a/src/ai_reviewer/__pycache__/ollama.cpython-311.pyc b/src/ai_reviewer/__pycache__/ollama.cpython-311.pyc new file mode 100644 index 0000000..416f42d Binary files /dev/null and b/src/ai_reviewer/__pycache__/ollama.cpython-311.pyc differ diff --git a/src/ai_reviewer/__pycache__/prompt.cpython-311.pyc b/src/ai_reviewer/__pycache__/prompt.cpython-311.pyc index 15701a1..1b6e88d 100644 Binary files a/src/ai_reviewer/__pycache__/prompt.cpython-311.pyc and b/src/ai_reviewer/__pycache__/prompt.cpython-311.pyc differ diff --git a/src/ai_reviewer/__pycache__/render.cpython-311.pyc b/src/ai_reviewer/__pycache__/render.cpython-311.pyc new file mode 100644 index 0000000..fe73a85 Binary files /dev/null and b/src/ai_reviewer/__pycache__/render.cpython-311.pyc differ diff --git a/src/ai_reviewer/diff.py b/src/ai_reviewer/diff.py index a6afbaf..5d0bf7c 100644 --- a/src/ai_reviewer/diff.py +++ b/src/ai_reviewer/diff.py @@ -43,19 +43,64 @@ class DiffChunk: def run_git_diff(repo: str, base: str, head: str) -> str: - cmd = [ - "git", - "-C", - repo, - "diff", - f"{base}...{head}", - "--unified=3", - "--no-color", - ] - result = subprocess.run(cmd, check=False, capture_output=True, text=True) - if result.returncode not in (0, 1): - raise RuntimeError(result.stderr.strip() or "git diff failed") - return result.stdout + base_candidates = _build_ref_candidates(base) + head_candidates = _build_ref_candidates(head) + + stdout, stderr, fallback_pairs = _diff_with_candidates(repo, base_candidates, head_candidates) + if stdout is not None: + return stdout + + # If the diff failed because revisions are missing (common in CI when the + # PR head/base aren't fetched), try fetching from origin and retry once. + first_error = stderr + try: + fetch_cmd = ["git", "-C", repo, "fetch", "origin", base, head] + subprocess.run(fetch_cmd, check=False, capture_output=True, text=True) + except Exception: + # Ignore fetch errors; we'll raise the original error below. + pass + + stdout, stderr, fallback_pairs_after_fetch = _diff_with_candidates( + repo, base_candidates, head_candidates + ) + if stdout is not None: + return stdout + + stdout, fallback_error = _fallback_diff_without_merge_base( + repo, fallback_pairs + fallback_pairs_after_fetch + ) + if stdout is not None: + return stdout + + # If still failing, raise a helpful error including stderr from git + raise RuntimeError(fallback_error or stderr or first_error or "git diff failed") + + +def _build_ref_candidates(ref: str) -> list[str]: + candidates = [ref] + if ref and not ref.startswith("origin/"): + candidates.append(f"origin/{ref}") + return candidates + + +def _diff_with_candidates( + repo: str, base_candidates: list[str], head_candidates: list[str] +) -> tuple[str | None, str, list[tuple[str, str]]]: + last_error = "" + no_merge_base_pairs: list[tuple[str, str]] = [] + for base_ref in base_candidates: + for head_ref in head_candidates: + result = _run_git_diff_command(repo, base_ref, head_ref, symmetric=True) + if result.returncode in (0, 1): + return result.stdout, "", [] + + if result.stderr: + err = result.stderr.strip() + last_error = err + if "no merge base" in err.lower(): + no_merge_base_pairs.append((base_ref, head_ref)) + + return None, last_error, no_merge_base_pairs def parse_diff(diff_text: str) -> list[FileDiff]: @@ -154,3 +199,33 @@ def chunk_files(files: Iterable[FileDiff], max_lines: int = 350) -> list[DiffChu ) return chunks + + +def _run_git_diff_command( + repo: str, base_ref: str, head_ref: str, *, symmetric: bool +) -> subprocess.CompletedProcess[str]: + cmd = ["git", "-C", repo, "diff"] + if symmetric: + cmd.append(f"{base_ref}...{head_ref}") + else: + cmd.extend([base_ref, head_ref]) + cmd.extend(["--unified=3", "--no-color"]) + return subprocess.run(cmd, check=False, capture_output=True, text=True) + + +def _fallback_diff_without_merge_base( + repo: str, pairs: list[tuple[str, str]] +) -> tuple[str | None, str]: + last_error = "" + seen: set[tuple[str, str]] = set() + for base_ref, head_ref in pairs: + key = (base_ref, head_ref) + if key in seen: + continue + seen.add(key) + result = _run_git_diff_command(repo, base_ref, head_ref, symmetric=False) + if result.returncode in (0, 1): + return result.stdout, "" + if result.stderr: + last_error = result.stderr.strip() + return None, last_error diff --git a/src/ai_reviewer/ollama.py b/src/ai_reviewer/ollama.py index 6fbc722..7b4bcc9 100644 --- a/src/ai_reviewer/ollama.py +++ b/src/ai_reviewer/ollama.py @@ -17,7 +17,7 @@ class OllamaClient: "stream": False, "options": {"temperature": 0}, } - with httpx.Client(timeout=60) as client: + with httpx.Client(timeout=3600) as client: response = client.post(url, json=payload) response.raise_for_status() data = response.json() diff --git a/tests/__pycache__/test_diff.cpython-311-pytest-9.0.2.pyc b/tests/__pycache__/test_diff.cpython-311-pytest-9.0.2.pyc new file mode 100644 index 0000000..70cd5e8 Binary files /dev/null and b/tests/__pycache__/test_diff.cpython-311-pytest-9.0.2.pyc differ diff --git a/tests/test_diff.py b/tests/test_diff.py new file mode 100644 index 0000000..604e808 --- /dev/null +++ b/tests/test_diff.py @@ -0,0 +1,86 @@ +from __future__ import annotations + +from typing import Any +from unittest.mock import patch + +import subprocess + +from ai_reviewer.diff import run_git_diff + + +def _completed(cmd: list[str], returncode: int, stdout: str = "", stderr: str = "") -> subprocess.CompletedProcess[Any]: + return subprocess.CompletedProcess(cmd, returncode, stdout, stderr) + + +def test_run_git_diff_prefers_direct_refs(): + calls: list[list[str]] = [] + + def fake_run(cmd, check=False, capture_output=False, text=False): # type: ignore[override] + calls.append(cmd) + if cmd[3] == "diff": + if cmd[4] == "base...head": + return _completed(cmd, 0, stdout="ok") + return _completed(cmd, 128, stderr="fatal") + raise AssertionError("fetch should not run when refs exist locally") + + with patch("ai_reviewer.diff.subprocess.run", side_effect=fake_run): + output = run_git_diff(".", "base", "head") + + assert output == "ok" + # Only one diff attempt should be needed when local refs exist. + assert len(calls) == 1 + + +def test_run_git_diff_falls_back_to_origin_refs_after_fetch(): + fetched = False + diff_attempts: list[str] = [] + + def fake_run(cmd, check=False, capture_output=False, text=False): # type: ignore[override] + nonlocal fetched + if cmd[3] == "diff": + if len(cmd) == 7: + spec = cmd[4] + else: + spec = " ".join(cmd[4:6]) + diff_attempts.append(spec) + if spec == "origin/base...origin/head" and fetched: + return _completed(cmd, 0, stdout="remote-ok") + return _completed(cmd, 128, stderr="missing ref") + if cmd[3] == "fetch": + fetched = True + return _completed(cmd, 0) + raise AssertionError("unexpected git invocation") + + with patch("ai_reviewer.diff.subprocess.run", side_effect=fake_run): + output = run_git_diff(".", "base", "head") + + assert output == "remote-ok" + assert "origin/base...origin/head" in diff_attempts + # Ensure the fallback only succeeds after fetch. + assert diff_attempts.index("origin/base...origin/head") > diff_attempts.index("base...head") + + +def test_run_git_diff_fallbacks_to_two_dot_when_merge_base_missing(): + fetched = False + diff_attempts: list[str] = [] + + def fake_run(cmd, check=False, capture_output=False, text=False): # type: ignore[override] + nonlocal fetched + if cmd[3] == "diff": + if len(cmd) == 7: + spec = cmd[4] + diff_attempts.append(f"sym:{spec}") + return _completed(cmd, 128, stderr="fatal: origin/main...origin/head: no merge base") + spec = " ".join(cmd[4:6]) + diff_attempts.append(f"two:{spec}") + return _completed(cmd, 0, stdout="linear-diff") + if cmd[3] == "fetch": + fetched = True + return _completed(cmd, 0) + raise AssertionError("unexpected git invocation") + + with patch("ai_reviewer.diff.subprocess.run", side_effect=fake_run): + output = run_git_diff(".", "base", "head") + + assert output == "linear-diff" + assert any(attempt.startswith("two:") for attempt in diff_attempts)