Skip to content

Avoid stale approved follow-up issues#75

Merged
wwind123 merged 1 commit into
mainfrom
fix-73-final-round-approved-followups
May 17, 2026
Merged

Avoid stale approved follow-up issues#75
wwind123 merged 1 commit into
mainfrom
fix-73-final-round-approved-followups

Conversation

@wwind123
Copy link
Copy Markdown
Owner

Summary

  • Process approved-review future follow-ups only from the final approval round
  • Stop carrying pre-fix future follow-ups across same-PR fix rounds
  • Update tests and docs for final-round-only follow-up handling

Fixes #73

Tests

  • /tmp/coding-review-agent-loop/scratch/issue73-venv/bin/python -m pytest tests/test_agent_loop.py

@wwind123
Copy link
Copy Markdown
Owner Author

Implemented issue #73 and opened PR #75.

Changed approved follow-up handling so stale future follow-ups from earlier same-PR fix rounds are not carried forward; only future follow-ups restated in the final approval round are summarized or turned into issues. Updated tests and docs accordingly.

Tests:
/tmp/coding-review-agent-loop/scratch/issue73-venv/bin/python -m pytest tests/test_agent_loop.py - 130 passed

-- OpenAI Codex

@wwind123
Copy link
Copy Markdown
Owner Author

Review: Avoid stale approved follow-up issues (#75)

The fix is minimal and correct. Removing pending_future_followups entirely and switching to approved_followups = round_future_followups is exactly the right approach for issue #73.

Logic review:

The old accumulator pending_future_followups.extend(round_future_followups) in the same-PR fix path meant that when round 1 approved-with-same-PR-fixes also included future follow-ups, those future items were merged with round 2's future follow-ups on final approval. Since round 2 reviewers see the updated code and can restate whatever is still relevant, carrying round 1 items forward created stale noise. The fix correctly trusts the final round's reviewers to surface anything worth tracking.

The discard-on-blocking path (the else branch) already had the right behavior and didn't need a logic change — just a comment cleanup, which reads more accurately now.

Test review:

The three test changes work together:

  • test_pr_loop_fix_and_summarize_sends_same_pr_followups_to_coder_then_rereviews: comment count drops from 4 → 3 because round 2 has no future follow-ups, so no summary comment is posted. Correct.
  • Renamed test_pr_loop_fix_and_issue_ignores_future_followups_from_pre_fix_round: same setup, final round has no future follow-ups → runner.issues == []. Tests the discarded case cleanly.
  • New test_pr_loop_fix_and_issue_uses_only_final_round_future_followups: round 1 has "Stale item…", round 2 has "Add a separate migration dry-run command." — verifies exactly one issue is created for the final-round item and the stale item is absent. This is the core coverage for the new behavior.
  • test_pr_loop_fix_and_summarize_uses_only_final_round_future_followups: multi-reviewer case — only "final follow-up" items appear in the summary, "larger follow-up" items from round 1 do not. Correct.

Docs review:

Both README.md and docs/local_agent_loop.md now say "only future follow-ups restated in the final approval round are summarized or turned into issues," which accurately describes the new behavior.

No blocking issues.

-- Anthropic Claude

@wwind123
Copy link
Copy Markdown
Owner Author

Review: Avoid stale approved follow-up issues (#75)

The changes correctly address issue #73 by ensuring that only future follow-ups from the final approval round are processed. This prevents "stale" or redundant suggestions from being carried forward when multiple review rounds occur within the same PR.

Logic Review

  • The removal of the pending_future_followups accumulator in run_pr_loop is correct.
  • By resetting round_future_followups in each round and only using it when the PR is finally approved, the orchestrator now correctly trusts the final state of the review.
  • Discarding items when there are blocking reviews or same-PR fixes is the right behavior, as it allows reviewers to re-evaluate what is still a "future" item once the code has been updated.

Test Coverage

  • The updated tests (test_pr_loop_fix_and_summarize_sends_same_pr_followups_to_coder_then_rereview, etc.) correctly verify the expected behavior.
  • The new tests (test_pr_loop_fix_and_issue_uses_only_final_round_future_followups and test_pr_loop_fix_and_summarize_uses_only_final_round_future_followups) provide solid coverage for the fix, confirming that stale items are indeed excluded.

Documentation

  • Both README.md and docs/local_agent_loop.md have been updated to accurately describe the new behavior.

The implementation is surgical, well-tested, and maintainable.

-- Google Gemini

@wwind123 wwind123 merged commit f323f64 into main May 17, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid creating stale approved-followup issues from earlier PR rounds

1 participant