diff --git a/.github/workflows/codex_pr_review.yml b/.github/workflows/codex_pr_review.yml index 215e81d5..b15c8f23 100644 --- a/.github/workflows/codex_pr_review.yml +++ b/.github/workflows/codex_pr_review.yml @@ -13,6 +13,11 @@ on: description: "Stable caller-side key used to cancel stale review jobs for the same PR." required: false type: string + allow_unconfigured_backend: + description: "Allow the review job to pass with a human-review note when no AI backend is configured in the caller repository." + required: false + type: boolean + default: false secrets: CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN: description: "Token that can read QuantStrategyLab/AIAuditBridge when this workflow is called from another private repo." @@ -75,6 +80,7 @@ jobs: CODEX_AUDIT_SERVICE_URL: ${{ secrets.CODEX_AUDIT_SERVICE_URL }} CODEX_AUDIT_SERVICE_AUDIENCE: ${{ vars.CODEX_AUDIT_SERVICE_AUDIENCE || 'quant-codex-audit' }} CODEX_PR_REVIEW_REPO_ROOT: ${{ github.workspace }}/source + CODEX_PR_REVIEW_ALLOW_UNCONFIGURED_BACKEND: ${{ inputs.allow_unconfigured_backend || 'false' }} working-directory: source run: | set -euo pipefail diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py index 752e4cea..9d4e5950 100644 --- a/scripts/run_codex_pr_review.py +++ b/scripts/run_codex_pr_review.py @@ -43,6 +43,10 @@ "rate limit", "quota", ) +NO_REVIEW_BACKEND_CONFIGURED = ( + "No Codex service URL or API key configured. " + "Set CODEX_AUDIT_SERVICE_URL, ANTHROPIC_API_KEY, or OPENAI_API_KEY." +) # Risk → block mapping BLOCK_SEVERITIES = frozenset({"critical", "high"}) @@ -480,6 +484,14 @@ def _service_review_should_fallback(exc: ReviewError) -> bool: return any(signal in message for signal in CODEX_SERVICE_FALLBACK_SIGNALS) +def _review_backend_is_unconfigured(exc: ReviewError) -> bool: + return str(exc).strip() == NO_REVIEW_BACKEND_CONFIGURED + + +def _allow_unconfigured_backend() -> bool: + return parse_bool(env_value("CODEX_PR_REVIEW_ALLOW_UNCONFIGURED_BACKEND")) + + def run_codex_review_with_fallback( prompt: str, timeout_minutes: int, @@ -490,6 +502,7 @@ def run_codex_review_with_fallback( # env_value() returns "" when CODEX_AUDIT_SERVICE_URL is unset, so this # guard keeps the direct-API path intact without special error handling. service_url = env_value("CODEX_AUDIT_SERVICE_URL") + service_failure: Exception | None = None if service_url: try: print(f"Running Codex review via service: {service_url}") @@ -503,12 +516,21 @@ def run_codex_review_with_fallback( except ReviewError as exc: if not _service_review_should_fallback(exc): raise + service_failure = exc print(f"::warning::Codex service review failed; falling back to direct API: {exc}") except (json.JSONDecodeError, OSError, urllib.error.URLError) as exc: + service_failure = exc print(f"::error::Codex service review failed; falling back to direct API: {exc}") print("Running Codex review via direct API") - return run_direct_api_review(prompt, complexity=complexity) + try: + return run_direct_api_review(prompt, complexity=complexity) + except ReviewError as exc: + if service_failure is not None and _review_backend_is_unconfigured(exc): + raise ReviewError( + f"Codex service review failed and no direct API fallback is configured: {service_failure}" + ) from exc + raise def _service_request( @@ -570,10 +592,7 @@ def run_direct_api_review(prompt: str, complexity: str = "") -> str: model=_direct_api_model_for_complexity(provider, normalized), ) - raise ReviewError( - "No Codex service URL or API key configured. " - "Set CODEX_AUDIT_SERVICE_URL, ANTHROPIC_API_KEY, or OPENAI_API_KEY." - ) + raise ReviewError(NO_REVIEW_BACKEND_CONFIGURED) def _run_anthropic_review(prompt: str, api_key: str, model: str = "") -> str: @@ -985,7 +1004,10 @@ def main() -> int: upsert_pr_comment(token, repo, pr_number, warning_body) return 0 - # High-risk changes should not fail open on review infrastructure errors. + # Caller repositories may not have the central AI backend secrets configured. + # In that case, leave an explicit human-review note but do not create + # persistent red workflow runs. Other high-risk review infrastructure + # failures still fail closed. warning_body = ( "\n" "## 🤖 Codex PR Review\n\n" @@ -994,6 +1016,9 @@ def main() -> int: "Please ensure a human reviewer checks this PR before merging.\n" ) upsert_pr_comment(token, repo, pr_number, warning_body) + if _review_backend_is_unconfigured(exc) and _allow_unconfigured_backend(): + print("::warning::Codex review backend is not configured; leaving human-review note without failing the workflow.") + return 0 return 1 print(f"Codex output: {len(output)} chars") diff --git a/tests/test_run_codex_pr_review.py b/tests/test_run_codex_pr_review.py index 388e7b69..a7b3d0c4 100644 --- a/tests/test_run_codex_pr_review.py +++ b/tests/test_run_codex_pr_review.py @@ -65,6 +65,27 @@ def test_service_failure_falls_back_to_direct_api(self) -> None: self.assertEqual(output, "api review") direct_api.assert_called_once_with("Review this PR.", complexity="high") + + def test_service_fallback_without_api_keys_preserves_service_failure(self) -> None: + with ( + patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), + patch( + "scripts.run_codex_pr_review.run_codex_service_review", + side_effect=ReviewError("HTTP 429 Too Many Requests"), + ), + ): + with self.assertRaises(ReviewError) as raised: + run_codex_review_with_fallback( + "Review this PR.", + timeout_minutes=20, + complexity="high", + changed_file_count=3, + changed_line_count=120, + ) + + self.assertIn("Codex service review failed", str(raised.exception)) + self.assertFalse(run_codex_pr_review._review_backend_is_unconfigured(raised.exception)) + def test_service_auth_failure_does_not_fall_back_to_direct_api(self) -> None: with ( patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), @@ -124,6 +145,51 @@ def test_main_allows_low_risk_docs_on_review_infra_error(self) -> None: comment.assert_called_once() + + def test_main_allows_unconfigured_backend_with_explicit_opt_in(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + event_path = self._write_event(tmpdir, ["scripts/run_codex_pr_review.py"]) + env = { + "GH_TOKEN": "token", + "GITHUB_REPOSITORY": "org/repo", + "GITHUB_EVENT_PATH": event_path, + "GITHUB_EVENT_NAME": "pull_request", + "CODEX_PR_REVIEW_ALLOW_UNCONFIGURED_BACKEND": "true", + } + with ( + patch.dict(os.environ, env, clear=True), + patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "scripts/run_codex_pr_review.py"}]), + patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py"), + patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", side_effect=ReviewError(run_codex_pr_review.NO_REVIEW_BACKEND_CONFIGURED)), + patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, + ): + self.assertEqual(run_codex_pr_review.main(), 0) + + comment.assert_called_once() + self.assertIn("Human review required", comment.call_args.args[3]) + + + def test_main_fails_closed_on_unconfigured_backend_without_opt_in(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + event_path = self._write_event(tmpdir, ["scripts/run_codex_pr_review.py"]) + env = { + "GH_TOKEN": "token", + "GITHUB_REPOSITORY": "org/repo", + "GITHUB_EVENT_PATH": event_path, + "GITHUB_EVENT_NAME": "pull_request", + } + with ( + patch.dict(os.environ, env, clear=True), + patch("scripts.run_codex_pr_review.fetch_pr_files", return_value=[{"filename": "scripts/run_codex_pr_review.py"}]), + patch("scripts.run_codex_pr_review.fetch_pr_diff", return_value="diff --git a/scripts/run_codex_pr_review.py b/scripts/run_codex_pr_review.py"), + patch("scripts.run_codex_pr_review.run_codex_review_with_fallback", side_effect=ReviewError(run_codex_pr_review.NO_REVIEW_BACKEND_CONFIGURED)), + patch("scripts.run_codex_pr_review.upsert_pr_comment") as comment, + ): + self.assertEqual(run_codex_pr_review.main(), 1) + + comment.assert_called_once() + self.assertIn("Human review required", comment.call_args.args[3]) + def test_service_timeout_does_not_fall_back_to_direct_api(self) -> None: with ( patch.dict(os.environ, {"CODEX_AUDIT_SERVICE_URL": "https://service.example"}, clear=True), @@ -173,6 +239,8 @@ def test_reusable_workflow_runs_bridge_script_against_source_checkout(self) -> N self.assertIn("path: bridge", workflow) self.assertIn("CODEX_AUDIT_REUSABLE_WORKFLOW_TOKEN", workflow) self.assertIn("caller_concurrency_key", workflow) + self.assertIn("allow_unconfigured_backend", workflow) + self.assertIn("CODEX_PR_REVIEW_ALLOW_UNCONFIGURED_BACKEND", workflow) self.assertIn("inputs.caller_concurrency_key || github.event.pull_request.number || github.run_id", workflow) self.assertNotIn("Validate bridge checkout token", workflow) self.assertIn("required: false", workflow)