diff --git a/.github/workflows/pr-review.yaml b/.github/workflows/pr-review.yaml index f47a17d..2a53314 100644 --- a/.github/workflows/pr-review.yaml +++ b/.github/workflows/pr-review.yaml @@ -8,6 +8,10 @@ permissions: contents: read pull-requests: write +concurrency: + group: pr-review-${{ github.repository }}-${{ github.event.issue.number }} + cancel-in-progress: true + jobs: review: # Restrict to the repository owner account. The review runner is checked @@ -44,6 +48,9 @@ jobs: - name: Install dependencies run: pip install --require-hashes -r .github/requirements-pr-review.txt + - name: Test trusted review runner + run: python -m unittest tests/test_pr_review_routing.py + - name: Determine review mode id: mode env: @@ -59,10 +66,15 @@ jobs: - name: Run PR review env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + LITELLM_BASE_URL: ${{ secrets.LITELLM_BASE_URL }} + LITELLM_API_KEY: ${{ secrets.LITELLM_API_KEY }} OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} REVIEW_MODE: ${{ steps.mode.outputs.mode }} PR_NUMBER: ${{ github.event.issue.number }} REPO: ${{ github.repository }} - PR_REVIEW_MODEL_FAST: gpt-5.4-mini - PR_REVIEW_MODEL_DEEP: gpt-5.5 + # Empty/unset repository variables deliberately fall back to the + # Python defaults in scripts/pr-review.py. Keep defaults there so + # manual and workflow runs cannot drift. + PR_REVIEW_MODEL_FAST: ${{ vars.PR_REVIEW_MODEL_FAST }} + PR_REVIEW_MODEL_DEEP: ${{ vars.PR_REVIEW_MODEL_DEEP }} run: python scripts/pr-review.py diff --git a/scripts/pr-review.py b/scripts/pr-review.py index efd4f59..a175d20 100644 --- a/scripts/pr-review.py +++ b/scripts/pr-review.py @@ -5,8 +5,8 @@ """AI-powered PR review for the pipelock-verify Python package. Triggered by /review comments on PRs. Supports multiple review modes: - /review - Security and correctness review (smaller model, default) - /review deep - Deeper review (larger model) + /review - Security and correctness review (default model) + /review deep - Deeper review (higher-capacity model) /review tests - Test coverage and boundary analysis /review docs - Documentation accuracy check @@ -21,12 +21,14 @@ OPENAI_API_KEY - Direct OpenAI API Model selection: - PR_REVIEW_MODEL_FAST - Model for default/tests/docs (default: gpt-5.4-mini) - PR_REVIEW_MODEL_DEEP - Model for /review deep (default: gpt-5.5) + PR_REVIEW_MODEL_FAST - Optional model override for default/tests/docs + (default: gpt-5.6-luna) + PR_REVIEW_MODEL_DEEP - Optional model override for /review deep + (default: gpt-5.6-terra) The PR_REVIEW_MODEL_FAST env var keeps its name for backwards compatibility -with any existing repo-secrets overrides; the user-facing /review fast -alias was dropped 2026-04-23 because the default mode is fast enough. +with existing repository-variable overrides; the user-facing /review fast +alias was dropped 2026-04-23 because the default mode is sufficient. """ import json @@ -38,10 +40,13 @@ # --- Constants --- MAX_DIFF_CHARS = 100_000 -DEFAULT_MODEL_FAST = "gpt-5.4-mini" -DEFAULT_MODEL_DEEP = "gpt-5.5" +# These are the sole default model definitions. GitHub Actions may supply +# optional repository-variable overrides, but an unset or empty override must +# fall back here so workflow configuration cannot drift from local behavior. +DEFAULT_MODEL_FAST = "gpt-5.6-luna" +DEFAULT_MODEL_DEEP = "gpt-5.6-terra" DEFAULT_TEMPERATURE = 0.2 -DEFAULT_MAX_COMPLETION_TOKENS = 4096 +DEFAULT_MAX_COMPLETION_TOKENS = 8192 DEEP_MAX_COMPLETION_TOKENS = 25000 DEFAULT_LLM_TIMEOUT_SECONDS = 120 DEEP_LLM_TIMEOUT_SECONDS = 300 @@ -181,6 +186,13 @@ def build_llm_payload( return payload +def model_for_mode(mode: str) -> str: + """Return the configured model for a review mode, with Python defaults.""" + if mode == "deep": + return os.environ.get("PR_REVIEW_MODEL_DEEP") or DEFAULT_MODEL_DEEP + return os.environ.get("PR_REVIEW_MODEL_FAST") or DEFAULT_MODEL_FAST + + def summarize_usage(data: dict) -> str: """Return compact token usage details for operator-visible errors.""" usage = data.get("usage") @@ -200,11 +212,11 @@ def summarize_usage(data: dict) -> str: def extract_chat_content(data: dict) -> str: """Extract visible text from a chat-completions response.""" choices = data.get("choices", []) - if not choices: - raise LLMReviewError("LLM returned no choices. Raw response: " + json.dumps(data)[:500]) + if not isinstance(choices, list) or not choices: + raise LLMReviewError("LLM returned no choices.") - choice = choices[0] - message = choice.get("message", {}) + choice = choices[0] if isinstance(choices[0], dict) else {} + message = choice.get("message") if isinstance(choice.get("message"), dict) else {} content = message.get("content", "") if isinstance(content, list): content = "".join(part.get("text", "") for part in content if isinstance(part, dict)) @@ -229,10 +241,7 @@ def call_llm(diff: str, mode: str, system_prompt: str) -> str: litellm_key = os.environ.get("LITELLM_API_KEY", "") openai_key = os.environ.get("OPENAI_API_KEY", "") - if mode == "deep": - model = os.environ.get("PR_REVIEW_MODEL_DEEP") or DEFAULT_MODEL_DEEP - else: - model = os.environ.get("PR_REVIEW_MODEL_FAST") or DEFAULT_MODEL_FAST + model = model_for_mode(mode) if litellm_url and litellm_key: api_url = litellm_url.rstrip("/") + "/chat/completions" @@ -264,13 +273,13 @@ def call_llm(diff: str, mode: str, system_prompt: str) -> str: timeout = DEEP_LLM_TIMEOUT_SECONDS if is_deep else DEFAULT_LLM_TIMEOUT_SECONDS resp = requests.post(api_url, headers=headers, json=payload, timeout=timeout) if resp.status_code != 200: - body = resp.text[:500] - raise LLMReviewError( - f"LLM API returned {resp.status_code}.\n\n" - f"**Model:** `{model}`\n\n" - f"**Response:**\n```\n{body}\n```" - ) - data = resp.json() + raise LLMReviewError(f"LLM API returned {resp.status_code} for model `{model}`.") + try: + data = resp.json() + except (json.JSONDecodeError, ValueError) as error: + raise LLMReviewError("LLM returned invalid JSON.") from error + if not isinstance(data, dict): + raise LLMReviewError("LLM returned a non-object JSON response.") return extract_chat_content(data) @@ -328,9 +337,7 @@ def main() -> None: post_comment(repo, pr_number, gh_token, f"**AI Review Error:** {e}") sys.exit(1) - model_name = os.environ.get( - "PR_REVIEW_MODEL_DEEP" if mode == "deep" else "PR_REVIEW_MODEL_FAST" - ) or (DEFAULT_MODEL_DEEP if mode == "deep" else DEFAULT_MODEL_FAST) + model_name = model_for_mode(mode) mode_labels = { "default": "security", diff --git a/tests/test_pr_review_routing.py b/tests/test_pr_review_routing.py new file mode 100644 index 0000000..c207a5a --- /dev/null +++ b/tests/test_pr_review_routing.py @@ -0,0 +1,147 @@ +"""Regression tests for the trusted GitHub PR-review runner's model routing.""" + +from __future__ import annotations + +import importlib.util +import re +import unittest +from pathlib import Path +from types import ModuleType +from unittest.mock import Mock, patch + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT_PATH = ROOT / "scripts" / "pr-review.py" +WORKFLOW_PATH = ROOT / ".github" / "workflows" / "pr-review.yaml" + + +def load_pr_review_module() -> ModuleType: + """Load the workflow script without executing its CLI entry point.""" + spec = importlib.util.spec_from_file_location("pr_review_routing", SCRIPT_PATH) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + with patch.dict("sys.modules", {"requests": Mock()}): + spec.loader.exec_module(module) + return module + + +class PRReviewRoutingTests(unittest.TestCase): + def test_model_defaults_and_mode_routing(self): + module = load_pr_review_module() + with patch.dict( + module.os.environ, + {"PR_REVIEW_MODEL_FAST": "", "PR_REVIEW_MODEL_DEEP": ""}, + clear=False, + ): + self.assertEqual(module.DEFAULT_MODEL_FAST, "gpt-5.6-luna") + self.assertEqual(module.DEFAULT_MODEL_DEEP, "gpt-5.6-terra") + self.assertEqual(module.model_for_mode("default"), "gpt-5.6-luna") + self.assertEqual(module.model_for_mode("tests"), "gpt-5.6-luna") + self.assertEqual(module.model_for_mode("docs"), "gpt-5.6-luna") + self.assertEqual(module.model_for_mode("deep"), "gpt-5.6-terra") + self.assertEqual(module.FAST_REASONING_EFFORT, "low") + self.assertEqual(module.DEEP_REASONING_EFFORT, "medium") + + def test_gpt_5_6_payloads_pin_the_requested_reasoning_effort(self): + module = load_pr_review_module() + + ordinary_payload = module.build_llm_payload( + module.DEFAULT_MODEL_FAST, + "system prompt", + "diff", + reasoning_effort=module.FAST_REASONING_EFFORT, + ) + deep_payload = module.build_llm_payload( + module.DEFAULT_MODEL_DEEP, + "system prompt", + "diff", + reasoning_effort=module.DEEP_REASONING_EFFORT, + ) + + self.assertEqual(ordinary_payload["model"], "gpt-5.6-luna") + self.assertEqual(ordinary_payload["reasoning_effort"], "low") + self.assertEqual(ordinary_payload["max_completion_tokens"], 8192) + self.assertEqual(deep_payload["model"], "gpt-5.6-terra") + self.assertEqual(deep_payload["reasoning_effort"], "medium") + + def test_model_repository_variable_overrides(self): + module = load_pr_review_module() + with patch.dict( + module.os.environ, + { + "PR_REVIEW_MODEL_FAST": "provider/ordinary-override", + "PR_REVIEW_MODEL_DEEP": "provider/deep-override", + }, + clear=False, + ): + self.assertEqual(module.model_for_mode("default"), "provider/ordinary-override") + self.assertEqual(module.model_for_mode("tests"), "provider/ordinary-override") + self.assertEqual(module.model_for_mode("docs"), "provider/ordinary-override") + self.assertEqual(module.model_for_mode("deep"), "provider/deep-override") + + def test_empty_repository_variable_overrides_use_python_defaults(self): + module = load_pr_review_module() + with patch.dict( + module.os.environ, + {"PR_REVIEW_MODEL_FAST": "", "PR_REVIEW_MODEL_DEEP": ""}, + clear=False, + ): + self.assertEqual(module.model_for_mode("default"), module.DEFAULT_MODEL_FAST) + self.assertEqual(module.model_for_mode("deep"), module.DEFAULT_MODEL_DEEP) + + def test_workflow_delegates_model_defaults_to_python(self): + workflow = WORKFLOW_PATH.read_text(encoding="utf-8") + + self.assertIn("PR_REVIEW_MODEL_FAST: ${{ vars.PR_REVIEW_MODEL_FAST }}", workflow) + self.assertIn("PR_REVIEW_MODEL_DEEP: ${{ vars.PR_REVIEW_MODEL_DEEP }}", workflow) + self.assertIsNone(re.search(r"PR_REVIEW_MODEL_(?:FAST|DEEP): gpt-", workflow)) + + def test_workflow_keeps_trusted_runner_and_owner_gate(self): + workflow = WORKFLOW_PATH.read_text(encoding="utf-8") + + self.assertIn("github.event.comment.user.login == 'luckyPipewrench'", workflow) + self.assertIn("github.event.comment.author_association == 'OWNER'", workflow) + self.assertIn("ref: ${{ github.event.repository.default_branch }}", workflow) + self.assertIn("persist-credentials: false", workflow) + self.assertIn("LITELLM_BASE_URL: ${{ secrets.LITELLM_BASE_URL }}", workflow) + self.assertIn("LITELLM_API_KEY: ${{ secrets.LITELLM_API_KEY }}", workflow) + self.assertIn("OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}", workflow) + self.assertIn( + "group: pr-review-${{ github.repository }}-${{ github.event.issue.number }}", workflow + ) + self.assertIn("cancel-in-progress: true", workflow) + self.assertIn("python -m unittest tests/test_pr_review_routing.py", workflow) + + runner = SCRIPT_PATH.read_text(encoding="utf-8") + self.assertNotIn("resp.text[:500]", runner) + + def test_response_shape_errors_are_generic_and_fail_closed(self): + module = load_pr_review_module() + + with self.assertRaises(module.LLMReviewError) as ctx: + module.extract_chat_content({"choices": [], "private": "provider detail"}) + self.assertIn("no choices", str(ctx.exception)) + self.assertNotIn("provider detail", str(ctx.exception)) + + with self.assertRaisesRegex(module.LLMReviewError, "empty content"): + module.extract_chat_content({"choices": [None]}) + + def test_call_rejects_invalid_or_non_object_json(self): + module = load_pr_review_module() + invalid = Mock(status_code=200) + invalid.json.side_effect = ValueError("invalid") + with ( + patch.dict(module.os.environ, {"OPENAI_API_KEY": "test"}, clear=True), + patch.object(module.requests, "post", return_value=invalid), + self.assertRaisesRegex(module.LLMReviewError, "invalid JSON"), + ): + module.call_llm("diff", "default", "system") + + non_object = Mock(status_code=200) + non_object.json.return_value = [] + with ( + patch.dict(module.os.environ, {"OPENAI_API_KEY": "test"}, clear=True), + patch.object(module.requests, "post", return_value=non_object), + self.assertRaisesRegex(module.LLMReviewError, "non-object JSON"), + ): + module.call_llm("diff", "default", "system")