From 1ba66d9ac1f0539c20f4e8414b4b79a51b312f02 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Sun, 7 Jun 2026 16:40:42 -0700 Subject: [PATCH] Fix is_clarification_request to treat AGENT_CLARIFY as active only when it is the final marker A valid AGENT_STATE, AGENT_PLAN_STATE, or AGENT_PR marker appearing after the last AGENT_CLARIFY now takes precedence, preventing embedded protocol examples in plan prose or code blocks from triggering spurious clarification handling. Fixes #278 Co-Authored-By: Claude Sonnet 4.6 --- src/coding_review_agent_loop/protocol.py | 12 ++++- tests/test_agent_loop.py | 61 ++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/src/coding_review_agent_loop/protocol.py b/src/coding_review_agent_loop/protocol.py index eedd361..8250fb9 100644 --- a/src/coding_review_agent_loop/protocol.py +++ b/src/coding_review_agent_loop/protocol.py @@ -232,7 +232,17 @@ def parse_pr_number(text: str) -> int | None: def is_clarification_request(text: str) -> bool: - return bool(CLARIFY_RE.search(text)) + matches = list(CLARIFY_RE.finditer(text)) + if not matches: + return False + last_clarify_pos = matches[-1].end() + # A valid final AGENT_STATE, AGENT_PLAN_STATE, or AGENT_PR marker appearing after the last + # AGENT_CLARIFY takes precedence. This prevents embedded clarification marker examples + # (e.g. in prose or code blocks describing the protocol) from overriding a valid footer. + remaining = text[last_clarify_pos:] + if STATE_RE.search(remaining) or PLAN_STATE_RE.search(remaining) or PR_RE.search(remaining): + return False + return True def parse_signed_human_requirement_body(text: str | None) -> str | None: diff --git a/tests/test_agent_loop.py b/tests/test_agent_loop.py index 16d7335..125f289 100644 --- a/tests/test_agent_loop.py +++ b/tests/test_agent_loop.py @@ -14722,6 +14722,67 @@ def test_is_clarification_request_detects_marker(): assert not is_clarification_request("done\n") +def test_is_clarification_request_state_marker_after_clarify_wins(): + # AGENT_PLAN_STATE after embedded AGENT_CLARIFY → not clarification (issue #278) + plan_with_embedded_clarify = ( + "Here is the plan.\n" + "The protocol supports `` for asking questions.\n" + "\n" + "\n" + "-- Anthropic Claude" + ) + assert not is_clarification_request(plan_with_embedded_clarify) + + # AGENT_STATE after embedded AGENT_CLARIFY → not clarification + response_with_embedded_clarify = ( + "Here is my work.\n" + "Use `` to request more info.\n" + "\n" + "\n" + "-- Anthropic Claude" + ) + assert not is_clarification_request(response_with_embedded_clarify) + + # AGENT_PR after embedded AGENT_CLARIFY → not clarification + pr_response_with_embedded_clarify = ( + "PR created.\n" + "If needed, use `` to ask questions.\n" + "\n" + "\n" + "\n" + "-- Anthropic Claude" + ) + assert not is_clarification_request(pr_response_with_embedded_clarify) + + +def test_is_clarification_request_clarify_as_final_marker_is_still_detected(): + # AGENT_CLARIFY as the final marker → still a clarification + assert is_clarification_request( + "What endpoint should I use?\n\n-- Anthropic Claude" + ) + # AGENT_STATE in body then AGENT_CLARIFY at end → clarification wins + assert is_clarification_request( + "Prior state: \n" + "But I need clarification.\n" + "\n" + "-- Anthropic Claude" + ) + + +def test_is_clarification_request_code_block_example_does_not_trigger(): + # Embedded in a fenced code block with AGENT_PLAN_STATE at end + response = ( + "To request clarification, use:\n" + "```\n" + "\n" + "```\n" + "\n" + "\n" + "-- Anthropic Claude" + ) + assert not is_clarification_request(response) + + def test_task_loop_creates_pr_then_alternates_until_codex_approval(tmp_path): runner = FakeRunner( claude_outputs=[