diff --git a/README.md b/README.md index f48334e..e5ce007 100644 --- a/README.md +++ b/README.md @@ -195,9 +195,10 @@ include small, localized, low-risk current-PR cleanup under: Same-PR follow-ups are sent back to the coder in the existing PR and require a new review round. They should stay narrowly scoped to files already touched by the PR or directly adjacent code; larger redesigns and independent work belong -under Future follow-ups. Future follow-ups are retained and processed only after -final approval. The issue modes create at most three follow-up issues per -approved round to avoid issue noise. +under Future follow-ups. Future follow-ups are processed only from the final +approval round, so reviewers should restate any still-relevant future work after +same-PR updates. The issue modes create at most three follow-up issues per final +approval round to avoid issue noise. By default, `--approved-followups=ignore` asks reviewers not to include approved-review follow-up sections. Reviewers should mark the review blocking diff --git a/docs/local_agent_loop.md b/docs/local_agent_loop.md index 03159a8..c5193ca 100644 --- a/docs/local_agent_loop.md +++ b/docs/local_agent_loop.md @@ -387,9 +387,10 @@ under: Same-PR follow-ups are sent back to the coder in the existing PR and require a new review round. They should stay narrowly scoped to files already touched by the PR or directly adjacent code; larger redesigns and independent work belong -under Future follow-ups. Future follow-ups are retained and processed only -after the final approval round. Without a `fix-and-*` mode, reviewers should -mark same-PR cleanup blocking instead. +under Future follow-ups. Future follow-ups are processed only from the final +approval round, so reviewers should restate any still-relevant future work after +same-PR updates. Without a `fix-and-*` mode, reviewers should mark same-PR +cleanup blocking instead. By default, `--approved-followups=ignore` asks reviewers not to include these sections. Reviewers should mark the review blocking instead when cleanup should diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index 8963bb3..d1897ac 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -367,7 +367,6 @@ def run_pr_loop( # Backward-compatible single-reviewer resume support: older callers # pass one reviewer session, so attach it to the first configured reviewer. reviewer_session_ids[configured_reviewers[0]] = reviewer_session_id - pending_future_followups: list[ApprovedFollowup] = [] for round_number in range(1, config.max_rounds + 1): coder_name = agent_display_name(config.coder) @@ -424,7 +423,7 @@ def run_pr_loop( ) if not blocking_reviews and not same_pr_followups: - approved_followups = [*pending_future_followups, *round_future_followups] + approved_followups = round_future_followups if config.approved_followups in ("summarize", "fix-and-summarize") and approved_followups: body = _format_approved_followup_summary(pr_number, approved_followups) post_pr_comment(runner, config=config, pr_number=pr_number, body=body) @@ -451,7 +450,6 @@ def run_pr_loop( ) if same_pr_followups and not blocking_reviews: - pending_future_followups.extend(round_future_followups) combined_review = _format_same_pr_followups(same_pr_followups) followup_prompt = build_same_pr_followup_prompt( pr_number, diff --git a/tests/test_agent_loop.py b/tests/test_agent_loop.py index 1c92b3c..c990a99 100644 --- a/tests/test_agent_loop.py +++ b/tests/test_agent_loop.py @@ -1084,7 +1084,7 @@ def test_pr_loop_caps_approved_followup_issues(tmp_path): assert issue_summary.endswith("-- coding-review-agent-loop") -def test_pr_loop_fix_and_summarize_sends_same_pr_followups_to_coder_then_rereviews(tmp_path): +def test_pr_loop_fix_and_summarize_ignores_future_followups_before_same_pr_rereview(tmp_path): runner = FakeRunner( codex_outputs=[ "Codex approves with cleanup.\n\n" @@ -1103,7 +1103,7 @@ def test_pr_loop_fix_and_summarize_sends_same_pr_followups_to_coder_then_rerevie agent_commands = [cmd[:2] for cmd, _cwd in runner.commands if cmd[:1] in (["claude"], ["codex"])] assert agent_commands == [["codex", "exec"], ["claude", "--print"], ["codex", "exec"]] - assert len(runner.comments) == 4 + assert len(runner.comments) == 3 followup_prompt = next( cmd[-1] for cmd, _cwd in runner.commands if cmd[:1] == ["claude"] and "Same-PR follow-ups" in cmd[-1] ) @@ -1111,12 +1111,10 @@ def test_pr_loop_fix_and_summarize_sends_same_pr_followups_to_coder_then_rerevie assert "small, localized cleanup for the\ncurrent PR" in followup_prompt assert "Keep the change narrowly scoped to the listed items" in followup_prompt assert "Do not take on\nlarger redesigns or unrelated future work" in followup_prompt - summary = runner.comments[-1] - assert summary.startswith("Approved-review future follow-ups for PR #77:") - assert "- Add broader integration coverage later. (Codex)" in summary + assert runner.comments[-1] == "Codex approves final pass.\n\n-- OpenAI Codex" -def test_pr_loop_fix_and_issue_retains_future_followups_across_same_pr_round(tmp_path): +def test_pr_loop_fix_and_issue_ignores_stale_future_followups_after_same_pr_rereview(tmp_path): runner = FakeRunner( codex_outputs=[ "Codex approves with cleanup.\n\n" @@ -1133,14 +1131,13 @@ def test_pr_loop_fix_and_issue_retains_future_followups_across_same_pr_round(tmp assert run_pr_loop(runner, pr_number=77, config=config) == 0 - assert len(runner.issues) == 1 - assert runner.issues[0]["title"] == "Follow up future review note: Add a separate migration dry-run command." - assert "- https://github.com/OWNER/REPO/issues/99" in runner.comments[-1] + assert runner.issues == [] + assert runner.comments[-1] == "Codex approves final pass.\n\n-- OpenAI Codex" commands = [cmd[:3] for cmd, _cwd in runner.commands] - assert commands.count(["gh", "issue", "create"]) == 1 + assert commands.count(["gh", "issue", "create"]) == 0 -def test_pr_loop_fix_and_summarize_retains_future_followups_from_multiple_reviewers(tmp_path): +def test_pr_loop_fix_and_summarize_only_uses_final_round_future_followups(tmp_path): runner = FakeRunner( codex_outputs=[ "Codex approves with cleanup.\n\n" @@ -1149,14 +1146,20 @@ def test_pr_loop_fix_and_summarize_retains_future_followups_from_multiple_review "### Future follow-ups\n" "- Add Codex's larger follow-up later.\n" "\n-- OpenAI Codex", - "Codex approves final pass.\n\n-- OpenAI Codex", + "Codex approves final pass.\n\n" + "### Future follow-ups\n" + "- Add Codex's final follow-up later.\n" + "\n-- OpenAI Codex", ], claude_outputs=[ "Claude approves.\n\n" "### Future follow-ups\n" "- Add Claude's larger follow-up later.\n" "\n-- Anthropic Claude", - "Claude approves final pass.\n\n-- Anthropic Claude", + "Claude approves final pass.\n\n" + "### Future follow-ups\n" + "- Add Claude's final follow-up later.\n" + "\n-- Anthropic Claude", ], gemini_outputs=["Added assertion.\n\n-- Google Gemini"], ) @@ -1178,8 +1181,10 @@ def test_pr_loop_fix_and_summarize_retains_future_followups_from_multiple_review ["claude", "--print"], ] summary = runner.comments[-1] - assert "- Add Codex's larger follow-up later. (Codex)" in summary - assert "- Add Claude's larger follow-up later. (Claude)" in summary + assert "- Add Codex's final follow-up later. (Codex)" in summary + assert "- Add Claude's final follow-up later. (Claude)" in summary + assert "Add Codex's larger follow-up later." not in summary + assert "Add Claude's larger follow-up later." not in summary def test_pr_loop_reruns_all_reviewers_when_any_reviewer_blocks(tmp_path):