Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions docs/local_agent_loop.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 1 addition & 3 deletions src/coding_review_agent_loop/orchestrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand Down
35 changes: 20 additions & 15 deletions tests/test_agent_loop.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -1103,20 +1103,18 @@ 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]
)
assert "Rename the helper before merge." in followup_prompt
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<!-- AGENT_STATE: approved -->\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"
Expand All @@ -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<!-- AGENT_STATE: approved -->\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"
Expand All @@ -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"
"<!-- AGENT_STATE: approved -->\n-- OpenAI Codex",
"Codex approves final pass.\n<!-- AGENT_STATE: approved -->\n-- OpenAI Codex",
"Codex approves final pass.\n\n"
"### Future follow-ups\n"
"- Add Codex's final follow-up later.\n"
"<!-- AGENT_STATE: approved -->\n-- OpenAI Codex",
],
claude_outputs=[
"Claude approves.\n\n"
"### Future follow-ups\n"
"- Add Claude's larger follow-up later.\n"
"<!-- AGENT_STATE: approved -->\n-- Anthropic Claude",
"Claude approves final pass.\n<!-- AGENT_STATE: approved -->\n-- Anthropic Claude",
"Claude approves final pass.\n\n"
"### Future follow-ups\n"
"- Add Claude's final follow-up later.\n"
"<!-- AGENT_STATE: approved -->\n-- Anthropic Claude",
],
gemini_outputs=["Added assertion.\n<!-- AGENT_STATE: blocking -->\n-- Google Gemini"],
)
Expand All @@ -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):
Expand Down
Loading