Skip to content

Ignore empty approved followup placeholders#59

Merged
wwind123 merged 1 commit into
mainfrom
codex/ignore-empty-followup-placeholders
May 2, 2026
Merged

Ignore empty approved followup placeholders#59
wwind123 merged 1 commit into
mainfrom
codex/ignore-empty-followup-placeholders

Conversation

@wwind123
Copy link
Copy Markdown
Owner

@wwind123 wwind123 commented May 2, 2026

Summary

  • Ignore explicit empty approved-followup placeholders such as None, N/A, and No follow-ups.
  • Prevent fix-and-* loops from sending - None back to the coder as an actionable same-PR follow-up.
  • Add parser regression coverage for same-PR and future follow-up sections.

Tests

  • /home/wwind123/tools/coding-review-agent-loop/.venv/bin/python -m pytest tests/test_agent_loop.py -q

-- OpenAI Codex

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

The regex handles all expected inputs correctly, including the subtle case where a placeholder sentence starts with a valid keyword but continues with actual content (which correctly does not match).

Review

Purpose: Prevent placeholder text like - None, - N/A, - No follow-ups in the ### Same-PR follow-ups and ### Future follow-ups sections from being parsed as actionable items and routed back to the coder.

Correctness: The EMPTY_FOLLOWUP_RE regex is properly anchored (^…$) so it cannot accidentally swallow real follow-up items that happen to start with "None" or "No". The re.I flag handles mixed-case variants. The change is inserted at the one correct spot in flush_current where finalized items are admitted to the list.

Multi-line bullets: The parser joins continuation lines into item before calling EMPTY_FOLLOWUP_RE.match, so a placeholder that happens to span a continuation line (unusual but theoretically possible) would be collapsed before matching. This is fine — the regex will still match the joined single-word result.

Test coverage: The parametrized test checks six representative variants across both same_pr and future lists simultaneously, which is appropriate. Real-item cases are already covered by existing tests.

No blocking issues found. The change is minimal, targeted, and correctly implemented.

-- Anthropic Claude

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

The changes correctly implement the filtering of placeholder text in follow-up sections. The use of an anchored, case-insensitive regex in flush_current ensures that only exact matches for common "empty" indicators are ignored, while actual content remains unaffected.

Same-PR follow-ups

  • None.

Future follow-ups

  • None.

-- Google Gemini

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

Checked PR #59 and reviewer comments. Claude and Gemini both approved; the only same-PR follow-up listed is None., so there was no localized code change to make.

Verified current PR head 2905ed9 matches origin/codex/ignore-empty-followup-placeholders, worktree is clean, and python -m pytest passes with 123 passed in 1.80s. No commit or push was needed because there were no follow-up items to address.

-- OpenAI Codex

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

This is round 2 of the same PR with the same head SHA (2905ed9). My round 1 review found no blocking issues and approved. The diff and implementation have not changed. My analysis from round 1 stands: the regex is correctly anchored, the flush_current integration point is correct, and the test coverage is appropriate.

-- Anthropic Claude

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

The changes in this PR correctly address the issue of filtering out placeholder text in the follow-up sections of a review. By using a specific, anchored regex, it avoids treating "None" or "N/A" as actionable follow-up items while remaining safe against accidentally skipping real content.

Key Observations:

  • Regex Correctness: The EMPTY_FOLLOWUP_RE is anchored at both ends and covers common variants (None, N/A, No follow-ups, etc.) with case-insensitivity.
  • Integration: Filtering at the flush_current level ensures that multi-line bullets are fully reconstructed before being checked against the placeholder regex.
  • Testing: The new test cases verify that various placeholder formats are correctly ignored across both same-PR and future follow-up sections.

The implementation is clean and fulfills the PR objectives without any identified side effects.

-- Google Gemini

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

Created approved-review future follow-up issues for PR #59:

These were mentioned in approved reviews as future work and did not block merge readiness.

-- coding-review-agent-loop

@wwind123 wwind123 merged commit 6bde107 into main May 2, 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.

1 participant