Skip to content

fix: make missing PR review backend opt-in advisory #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Pigbibi merged 1 commit into from
Jul 5, 2026
Merged

fix: make missing PR review backend opt-in advisory #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/codex_pr_review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -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
Expand Down
37 changes: 31 additions & 6 deletions scripts/run_codex_pr_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"})
Expand Down Expand Up @@ -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,
Expand All @@ -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}")
Expand All @@ -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(
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 = (
"<!-- codex-pr-review -->\n"
"## 🤖 Codex PR Review\n\n"
Expand All @@ -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")
Expand Down
68 changes: 68 additions & 0 deletions tests/test_run_codex_pr_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
Expand Down
Loading