diff --git a/src/coding_review_agent_loop/checks.py b/src/coding_review_agent_loop/checks.py index 0764198..aba15dc 100644 --- a/src/coding_review_agent_loop/checks.py +++ b/src/coding_review_agent_loop/checks.py @@ -104,7 +104,11 @@ def _pr_check_details(pr_checks: PullRequestChecks) -> list[str]: if pr_checks.failing: details.append( "Failing checks: " - + ", ".join(f"{check.name} ({check.status})" for check in pr_checks.failing) + + ", ".join( + f"{check.name} ({check.status.lower()})" + + (f" — {check.url.strip()}" if check.url and check.url.strip() else "") + for check in pr_checks.failing + ) ) if pr_checks.pending: details.append( diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index fcadd77..d8da12a 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -4197,7 +4197,6 @@ def run_pr_loop( else None ) approved_review_outputs: list[tuple[str, str]] = [] - pr_checks = get_pr_checks(runner, config=config, metadata=pr_metadata) resumed_by_name = { record.metadata.agent: record for record in (current_resume.completed_reviews if current_resume is not None else ()) } @@ -4236,6 +4235,7 @@ def run_pr_loop( f"(context mode: {context_mode})", ) sync_reviewer_pr_before_review(config, runner, reviewer, pr_number, pr_metadata) + reviewer_pr_checks = get_pr_checks(runner, config=config, metadata=pr_metadata) compact_tail = ( CompactPrReviewTailContext( head_sha=pr_metadata.head_sha, @@ -4254,7 +4254,7 @@ def run_pr_loop( config, reviewer=reviewer, pr_metadata=pr_metadata, - pr_checks=pr_checks, + pr_checks=reviewer_pr_checks, memory=memory, issue_context=issue_context, human_requirements=human_requirements, @@ -4296,12 +4296,12 @@ def run_pr_loop( if ( resumed_record is None and review_state == "blocking" - and _is_pending_ci_only_review(parsed_review, pr_checks) + and _is_pending_ci_only_review(parsed_review, reviewer_pr_checks) ): log( config, f"Round {round_number}: {reviewer_name} blocking review only restates " - f"GitHub check status ({pr_checks.state}); treating as approved instead " + f"GitHub check status ({reviewer_pr_checks.state}); treating as approved instead " "of starting a new coder follow-up round", ) parsed_review = dataclasses_replace(parsed_review, state="approved", blocking_items=()) @@ -4650,6 +4650,7 @@ def run_pr_loop( ) next_unresolved_item_number += 1 must_fix_items = [item for item in unresolved_items if item.status in {"blocking", "same-pr"}] + pr_checks = get_pr_checks(runner, config=config, metadata=pr_metadata) if not must_fix_items: if pr_checks.state in {"pending", "unavailable"}: details = _pr_check_details(pr_checks) diff --git a/src/coding_review_agent_loop/prompts.py b/src/coding_review_agent_loop/prompts.py index 7da22f7..a97c481 100644 --- a/src/coding_review_agent_loop/prompts.py +++ b/src/coding_review_agent_loop/prompts.py @@ -593,7 +593,11 @@ def format_pr_checks(checks: PullRequestChecks) -> str: if checks.failing: lines.append( "- Failing checks: " - + ", ".join(f"{check.name} ({check.status})" for check in checks.failing) + + ", ".join( + f"{check.name} ({check.status.lower()})" + + (f" — {check.url.strip()}" if check.url and check.url.strip() else "") + for check in checks.failing + ) ) if checks.pending: lines.append( diff --git a/tests/test_orchestrator_pr.py b/tests/test_orchestrator_pr.py index c7cc326..8d8b87f 100644 --- a/tests/test_orchestrator_pr.py +++ b/tests/test_orchestrator_pr.py @@ -828,10 +828,26 @@ def test_pr_loop_routes_failing_github_checks_through_coder_followup(tmp_path, m { "check_runs": [ {"name": "tests/test_server.py", "status": "completed", "conclusion": "success"}, - {"name": "tests/test_security.py", "status": "completed", "conclusion": "failure"}, + { + "name": "tests/test_security.py", + "status": "completed", + "conclusion": "failure", + "html_url": "https://github.com/OWNER/REPO/actions/runs/555", + }, + ] + }, + { + "check_runs": [ + { + "name": "tests/test_security.py", + "status": "completed", + "conclusion": "failure", + "html_url": "https://github.com/OWNER/REPO/actions/runs/555", + } ] }, {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, + {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, ] ) @@ -856,8 +872,121 @@ def advance_checks(*_args, **_kwargs): and "GitHub PR checks unresolved blocking item [item-1] from round 1:" in cmd[-1] ) assert "Failing checks: tests/test_security.py (failure)" in followup_prompt + assert "https://github.com/OWNER/REPO/actions/runs/555" in followup_prompt assert "Do not claim global test success unless GitHub PR checks are green." in followup_prompt + +def test_pr_loop_refreshes_checks_between_reviewers_and_before_coder(tmp_path, monkeypatch): + runner = FakeRunner( + codex_outputs=[ + structured_pr_review(state="approved", summary="Codex approves."), + structured_pr_review( + state="approved", + summary="Codex approves after the fix.", + prior_item_dispositions=[{"item_id": "item-1", "disposition": "resolved"}], + ), + ], + gemini_outputs=[ + structured_pr_review(state="approved", summary="Gemini approves."), + structured_pr_review( + state="approved", + summary="Gemini approves after the fix.", + prior_item_dispositions=[{"item_id": "item-1", "disposition": "resolved"}], + ), + ], + claude_outputs=[ + structured_coder_followup( + state="approved", summary="Coder addressed the failure.", addressed_items=["item-1"] + ) + ], + ) + config = make_config(tmp_path, reviewer=("codex", "gemini"), max_rounds=2) + failure_url = "https://github.com/OWNER/REPO/actions/runs/555" + check_states = iter( + [ + {"check_runs": [{"name": "test", "status": "in_progress", "conclusion": None}]}, + { + "check_runs": [ + { + "name": "test", + "status": "completed", + "conclusion": "failure", + "html_url": failure_url, + } + ] + }, + { + "check_runs": [ + { + "name": "test", + "status": "completed", + "conclusion": "failure", + "html_url": failure_url, + } + ] + }, + {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, + {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, + {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, + ] + ) + + def next_checks(*args, **kwargs): + runner.pr_check_runs_payload = next(check_states) + return original_get_pr_checks(*args, **kwargs) + + from coding_review_agent_loop import orchestrator as orchestrator_module + + original_get_pr_checks = orchestrator_module.get_pr_checks + monkeypatch.setattr(orchestrator_module, "get_pr_checks", next_checks) + + assert run_pr_loop(runner, pr_number=77, config=config) == 0 + + review_prompts = [ + cmd[-1] + for cmd, _cwd in runner.commands + if cmd[:2] == ["codex", "exec"] or cmd[:1] == ["gemini"] + ] + assert "- Overall state: pending" in review_prompts[0] + assert "- Overall state: failing" in review_prompts[1] + assert f"- Failing checks: test (failure) — {failure_url}" in review_prompts[1] + assert any( + comment.startswith("GitHub PR checks are failing for PR #77.") + and failure_url in comment + for comment in runner.comments + ) + coder_prompt = next(cmd[-1] for cmd, _cwd in runner.commands if cmd[:1] == ["claude"]) + assert f"Failing checks: test (failure) — {failure_url}" in coder_prompt + assert "Overall state: pending" not in coder_prompt + + +def test_pr_loop_refreshes_pending_to_passing_without_ci_coder_round(tmp_path, monkeypatch): + runner = FakeRunner( + codex_outputs=[structured_pr_review(state="approved", summary="Codex approves.")], + gemini_outputs=[structured_pr_review(state="approved", summary="Gemini approves.")], + ) + config = make_config(tmp_path, reviewer=("codex", "gemini"), max_rounds=1) + check_states = iter( + [ + {"check_runs": [{"name": "test", "status": "in_progress", "conclusion": None}]}, + {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, + {"check_runs": [{"name": "test", "status": "completed", "conclusion": "success"}]}, + ] + ) + + def next_checks(*args, **kwargs): + runner.pr_check_runs_payload = next(check_states) + return original_get_pr_checks(*args, **kwargs) + + from coding_review_agent_loop import orchestrator as orchestrator_module + + original_get_pr_checks = orchestrator_module.get_pr_checks + monkeypatch.setattr(orchestrator_module, "get_pr_checks", next_checks) + + assert run_pr_loop(runner, pr_number=77, config=config) == 0 + assert not any(cmd[:1] == ["claude"] for cmd, _cwd in runner.commands) + assert not any("checks are still pending" in comment for comment in runner.comments) + def test_pr_loop_failing_github_checks_block_approval_even_with_auto_merge(tmp_path): runner = FakeRunner( codex_outputs=[structured_pr_review(state="approved", summary="LGTM.")], diff --git a/tests/test_prompts.py b/tests/test_prompts.py index b8b7a2e..66989b8 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -1,6 +1,8 @@ import pytest from agent_loop_helpers import * # noqa: F403 +from coding_review_agent_loop.github import PullRequestCheck, PullRequestChecks +from coding_review_agent_loop.prompts import format_pr_checks _EXPECTED_UNRESOLVED_ITEMS_GUIDANCE = """Prior unresolved items are present. Disposition every listed item in the JSON `prior_item_dispositions` array — do not add a separate prose section @@ -1452,6 +1454,33 @@ def test_review_prompt_includes_failing_github_check_status(tmp_path): assert "- Overall state: failing" in prompt assert "- Failing checks: tests/test_security.py (failure)" in prompt + +@pytest.mark.parametrize("url", [None, "", "https://github.com/OWNER/REPO/actions/runs/555"]) +def test_format_pr_checks_renders_failure_url_when_available_without_empty_link(url): + check = PullRequestCheck( + name="test", + kind="check_run", + status="FAILURE", + url=url, + ) + checks = PullRequestChecks( + state="failing", + required_checks=(), + passing=(), + pending=(), + failing=(check,), + missing_required=(), + branch_protection_status="not_found", + ) + + rendered = format_pr_checks(checks) + + assert "- Failing checks: test (failure)" in rendered + if url: + assert f"— {url}" in rendered + else: + assert "—" not in rendered + @pytest.mark.parametrize("compact_context", [False, True]) def test_review_prompt_distinguishes_failing_from_pending_check_blocking_policy( tmp_path, compact_context