From 7e1540e9fdfdb157fef2626f215329e4d59a54a2 Mon Sep 17 00:00:00 2001 From: "rasul.osmanbayli" Date: Tue, 23 Jun 2026 11:26:23 +0400 Subject: [PATCH 1/3] Fix reasoning manager response validation --- mobilerun/agent/droid/droid_agent.py | 6 +- mobilerun/agent/manager/manager_agent.py | 90 +++-- mobilerun/agent/manager/prompts.py | 172 +++++++-- .../agent/manager/stateless_manager_agent.py | 87 ++--- mobilerun/config/prompts/manager/rev1.jinja2 | 6 + .../config/prompts/manager/stateless.jinja2 | 2 +- .../config/prompts/manager/system.jinja2 | 10 +- tests/test_manager_response_validation.py | 360 ++++++++++++++++++ 8 files changed, 612 insertions(+), 121 deletions(-) create mode 100644 tests/test_manager_response_validation.py diff --git a/mobilerun/agent/droid/droid_agent.py b/mobilerun/agent/droid/droid_agent.py index 34e78475..9dc489dd 100644 --- a/mobilerun/agent/droid/droid_agent.py +++ b/mobilerun/agent/droid/droid_agent.py @@ -42,6 +42,7 @@ from mobilerun.agent.fast_agent import FastAgent from mobilerun.agent.fast_agent.events import FastAgentOutputEvent from mobilerun.agent.manager import ManagerAgent, StatelessManagerAgent +from mobilerun.agent.manager.prompts import ManagerResponseValidationError from mobilerun.agent.oneflows.structured_output_agent import StructuredOutputAgent from mobilerun.agent.trajectory import TrajectoryWriter from mobilerun.agent.utils.llm_loader import ( @@ -875,6 +876,9 @@ async def run_manager( except DeviceDisconnectedError as e: logger.error(f"Device disconnected: {e}") return FinalizeEvent(success=False, reason=f"Device disconnected: {e}") + except ManagerResponseValidationError as e: + logger.error(f"Manager response validation failed: {e}") + return FinalizeEvent(success=False, reason=str(e)) event = ManagerPlanEvent( plan=result["plan"], @@ -900,7 +904,7 @@ async def handle_manager_plan( extra={"color": "cyan"}, ) return ManagerInputEvent() - success = ev.success if ev.success is not None else True + success = ev.success if ev.success is not None else False self.shared_state.progress_summary = f"Answer: {ev.answer}" return FinalizeEvent(success=success, reason=ev.answer) diff --git a/mobilerun/agent/manager/manager_agent.py b/mobilerun/agent/manager/manager_agent.py index 59ef0964..678632d6 100644 --- a/mobilerun/agent/manager/manager_agent.py +++ b/mobilerun/agent/manager/manager_agent.py @@ -35,7 +35,12 @@ ManagerPlanDetailsEvent, ManagerResponseEvent, ) -from mobilerun.agent.manager.prompts import parse_manager_response +from mobilerun.agent.manager.prompts import ( + ManagerResponseValidationError, + parse_manager_response, + strip_manager_final_tags, + validate_manager_response, +) from mobilerun.agent.usage import get_usage_from_response from mobilerun.agent.utils.chat_utils import filter_empty_messages from mobilerun.agent.utils.inference import acall_with_retries @@ -324,55 +329,46 @@ async def _validate_and_retry( retry_count = 0 while retry_count < max_retries: - error_message = None - - if parsed["answer"] and not parsed["plan"]: - if parsed["success"] is None: - error_message = ( - 'You must include success="true" or success="false" attribute ' - "in the tag.\n" - 'Example: Task completed\n' - "Retry again." - ) - else: - break # Valid - elif parsed["plan"] and parsed["answer"]: - error_message = ( - "You cannot use both request_accomplished tag while the plan is not finished. " - "If you want to use request_accomplished tag, please make sure the plan is finished.\n" - "Retry again." - ) - elif not parsed["plan"]: - error_message = ( - "You must provide a plan to complete the task. " - "Please provide a plan with the correct format." - ) - else: - break # Valid: plan without answer - - if error_message: - retry_count += 1 - logger.warning( - f"Manager response invalid (retry {retry_count}/{max_retries}): {error_message}" - ) + validation = validate_manager_response(parsed) + if validation.is_valid: + return output + + error_message = f"{validation.error_message}\nRetry again." + retry_count += 1 + logger.warning( + f"Manager response invalid (retry {retry_count}/{max_retries}): {error_message}" + ) - # Build retry messages - retry_messages = messages + [ - ChatMessage(role="assistant", content=output), - ChatMessage(role="user", content=error_message), - ] + retry_messages = messages + [ + ChatMessage(role="assistant", content=output), + ChatMessage(role="user", content=error_message), + ] - try: - response = await acall_with_retries( - self.llm, retry_messages, stream=self.agent_config.streaming - ) - output = response.message.content - parsed = parse_manager_response(output) - except Exception as e: - logger.error(f"LLM retry failed: {e}") - break + try: + response = await acall_with_retries( + self.llm, retry_messages, stream=self.agent_config.streaming + ) + output = response.message.content + parsed = parse_manager_response(output) + except Exception as e: + logger.error(f"LLM retry failed: {e}") + if validation.can_continue_with_plan: + return strip_manager_final_tags(output) + raise ManagerResponseValidationError(error_message) from e + + validation = validate_manager_response(parsed) + if validation.is_valid: + return output + if validation.can_continue_with_plan: + logger.warning( + "Manager response stayed invalid after retries; continuing with the plan " + "and ignoring the conflicting final answer." + ) + return strip_manager_final_tags(output) - return output + raise ManagerResponseValidationError( + validation.error_message or "Invalid manager response." + ) # ======================================================================== # Workflow Steps diff --git a/mobilerun/agent/manager/prompts.py b/mobilerun/agent/manager/prompts.py index 56cad317..b903b6b2 100644 --- a/mobilerun/agent/manager/prompts.py +++ b/mobilerun/agent/manager/prompts.py @@ -3,6 +3,55 @@ """ import re +from dataclasses import dataclass + +FINAL_RESPONSE_TAGS = ("request_accomplished", "answer") + +_SUCCESS_ATTR_RE = re.compile( + r"""\bsuccess\s*=\s*(?P["'])(?Ptrue|false)(?P=quote)""", + re.IGNORECASE | re.DOTALL, +) +_FINAL_TAG_RE = re.compile( + r"<(?Prequest_accomplished|answer)\b(?P[^>]*)>" + r"(?P.*?)" + r"", + re.IGNORECASE | re.DOTALL, +) + + +@dataclass(frozen=True) +class ManagerResponseValidation: + is_valid: bool + error_message: str | None = None + can_continue_with_plan: bool = False + + +class ManagerResponseValidationError(RuntimeError): + """Raised when the manager cannot produce a valid response after retries.""" + + +def _find_tag_matches(response: str, tag: str) -> list[re.Match[str]]: + pattern = re.compile( + rf"<{tag}\b(?P[^>]*)>(?P.*?)", + re.IGNORECASE | re.DOTALL, + ) + return list(pattern.finditer(response)) + + +def _tag_content(match: re.Match[str]) -> str: + return match.group("body").strip() + + +def _success_from_attrs(attrs: str) -> bool | None: + match = _SUCCESS_ATTR_RE.search(attrs) + if not match: + return None + return match.group("value").lower() == "true" + + +def strip_manager_final_tags(response: str) -> str: + """Remove final answer tags while leaving reasoning and plan tags intact.""" + return _FINAL_TAG_RE.sub("", response).strip() def parse_manager_response(response: str) -> dict: @@ -35,43 +84,45 @@ def parse_manager_response(response: str) -> dict: def extract(tag: str) -> str: """Extract content between XML-style tags (handles attributes).""" - # Use regex to handle tags with attributes like - pattern = rf"<{tag}(?:\s+[^>]*)?>(.+?)" - match = re.search(pattern, response, re.DOTALL) - if match: - return match.group(1).strip() + matches = _find_tag_matches(response, tag) + if matches: + return _tag_content(matches[0]) return "" def extract_all(tag: str) -> str: """Extract and combine content from all occurrences of a tag.""" - pattern = rf"<{tag}(?:\s+[^>]*)?>(.+?)" - matches = re.findall(pattern, response, re.DOTALL) + matches = [_tag_content(match) for match in _find_tag_matches(response, tag)] if not matches: return "" - return "\n".join(m.strip() for m in matches if m.strip()) + return "\n".join(match for match in matches if match) thought = extract("thought") memory_section = extract_all("add_memory") plan = extract("plan") progress_summary = extract("progress_summary") - answer = extract("request_accomplished") - - # Also support tag as alternative to - if not answer: - answer = extract("answer") - - # Parse success attribute from request_accomplished or answer tag + plan_matches = _find_tag_matches(response, "plan") + final_matches = list(_FINAL_TAG_RE.finditer(response)) + final_matches.sort(key=lambda match: match.start()) + + final_match = None + for match in final_matches: + if _tag_content(match): + final_match = match + break + if final_match is None and final_matches: + final_match = final_matches[0] + + answer = _tag_content(final_match) if final_match else "" success = None - if answer: - # Try to extract success attribute from tag - success_match = re.search( - r'<(?:request_accomplished|answer)\s+success="(true|false)">', response - ) - if success_match: - success = success_match.group(1) == "true" - else: - # Default to True for backward compatibility if no attribute present - success = True + final_tag = None + if final_match: + final_tag = final_match.group("tag").lower() + success = _success_from_attrs(final_match.group("attrs")) + + final_counts = { + tag: sum(1 for match in final_matches if match.group("tag").lower() == tag) + for tag in FINAL_RESPONSE_TAGS + } # Parse current subgoal from first line of plan current_goal_text = plan @@ -111,4 +162,75 @@ def extract_all(tag: str) -> str: "answer": answer, "success": success, "progress_summary": progress_summary, + "final_tag": final_tag, + "tag_counts": { + "plan": len(plan_matches), + "request_accomplished": final_counts["request_accomplished"], + "answer": final_counts["answer"], + "final": len(final_matches), + }, } + + +def validate_manager_response(parsed: dict) -> ManagerResponseValidation: + """Validate the manager output contract parsed by parse_manager_response.""" + plan = (parsed.get("plan") or "").strip() + answer = (parsed.get("answer") or "").strip() + success = parsed.get("success") + tag_counts = parsed.get("tag_counts") or {} + plan_count = tag_counts.get("plan", 1 if plan else 0) + final_count = tag_counts.get("final", 1 if answer else 0) + + if plan_count > 1: + return ManagerResponseValidation( + is_valid=False, + error_message="Manager response contains multiple tags. " + "Provide exactly one , or exactly one final answer tag.", + ) + + if final_count > 1: + return ManagerResponseValidation( + is_valid=False, + error_message="Manager response contains multiple final answer tags. " + "Provide exactly one or tag.", + can_continue_with_plan=plan_count == 1 and bool(plan), + ) + + if plan_count == 1 and not plan: + return ManagerResponseValidation( + is_valid=False, + error_message="Manager tag must not be empty.", + ) + + if final_count == 1 and not answer: + return ManagerResponseValidation( + is_valid=False, + error_message="Manager final answer tag must not be empty.", + can_continue_with_plan=plan_count == 1 and bool(plan), + ) + + if plan and answer: + return ManagerResponseValidation( + is_valid=False, + error_message="Manager response must provide exactly one of " + "or a final answer tag, not both.", + can_continue_with_plan=plan_count == 1 and final_count == 1, + ) + + if answer: + if success is None: + return ManagerResponseValidation( + is_valid=False, + error_message='Final answer tag must include success="true" ' + 'or success="false".', + ) + return ManagerResponseValidation(is_valid=True) + + if plan: + return ManagerResponseValidation(is_valid=True) + + return ManagerResponseValidation( + is_valid=False, + error_message="Manager response must provide exactly one of " + "or a final answer tag.", + ) diff --git a/mobilerun/agent/manager/stateless_manager_agent.py b/mobilerun/agent/manager/stateless_manager_agent.py index 6252ac07..ef966cb3 100644 --- a/mobilerun/agent/manager/stateless_manager_agent.py +++ b/mobilerun/agent/manager/stateless_manager_agent.py @@ -19,7 +19,12 @@ ManagerPlanDetailsEvent, ManagerResponseEvent, ) -from mobilerun.agent.manager.prompts import parse_manager_response +from mobilerun.agent.manager.prompts import ( + ManagerResponseValidationError, + parse_manager_response, + strip_manager_final_tags, + validate_manager_response, +) from mobilerun.agent.usage import get_usage_from_response from mobilerun.agent.utils.chat_utils import to_chat_messages from mobilerun.agent.utils.inference import acall_with_retries @@ -124,54 +129,46 @@ async def _validate_and_retry( retry_count = 0 while retry_count < max_retries: - error_message = None - - if parsed["answer"] and not parsed["plan"]: - if parsed["success"] is None: - error_message = ( - 'You must include success="true" or success="false" attribute ' - "in the or tag.\n" - 'Example: Task completed\n' - "Retry again." - ) - else: - break - elif parsed["plan"] and parsed["answer"]: - error_message = ( - "You cannot include both and tags. " - "Use only when the task is complete.\n" - "Retry again." - ) - elif not parsed["plan"] and not parsed["answer"]: - error_message = ( - "You must provide either a or an . " - "Please provide a plan with numbered steps." - ) - else: - break - - if error_message: - retry_count += 1 - logger.warning( - f"Manager response invalid (retry {retry_count}/{max_retries}): {error_message}" - ) + validation = validate_manager_response(parsed) + if validation.is_valid: + return output + + error_message = f"{validation.error_message}\nRetry again." + retry_count += 1 + logger.warning( + f"Manager response invalid (retry {retry_count}/{max_retries}): {error_message}" + ) - retry_messages = messages + [ - {"role": "assistant", "content": [{"text": output}]}, - {"role": "user", "content": [{"text": error_message}]}, - ] + retry_messages = messages + [ + {"role": "assistant", "content": [{"text": output}]}, + {"role": "user", "content": [{"text": error_message}]}, + ] - chat_messages = to_chat_messages(retry_messages) + chat_messages = to_chat_messages(retry_messages) - try: - response = await acall_with_retries(self.llm, chat_messages) - output = response.message.content - parsed = parse_manager_response(output) - except Exception as e: - logger.error(f"LLM retry failed: {e}") - break + try: + response = await acall_with_retries(self.llm, chat_messages) + output = response.message.content + parsed = parse_manager_response(output) + except Exception as e: + logger.error(f"LLM retry failed: {e}") + if validation.can_continue_with_plan: + return strip_manager_final_tags(output) + raise ManagerResponseValidationError(error_message) from e + + validation = validate_manager_response(parsed) + if validation.is_valid: + return output + if validation.can_continue_with_plan: + logger.warning( + "Manager response stayed invalid after retries; continuing with the plan " + "and ignoring the conflicting final answer." + ) + return strip_manager_final_tags(output) - return output + raise ManagerResponseValidationError( + validation.error_message or "Invalid manager response." + ) @step async def prepare_context( diff --git a/mobilerun/config/prompts/manager/rev1.jinja2 b/mobilerun/config/prompts/manager/rev1.jinja2 index f8151ec0..166e53ae 100644 --- a/mobilerun/config/prompts/manager/rev1.jinja2 +++ b/mobilerun/config/prompts/manager/rev1.jinja2 @@ -104,6 +104,8 @@ You can reference these custom actions or tell the Executer agent to use them in ## Output Format +Always include ``, optionally include ``, and then include exactly one of `` or ``. Do not include both and . + Explain your reasoning for the plan and next subgoal. @@ -120,6 +122,8 @@ Example: "At step 5, I obtained recipe from recipes.jpg: Chicken Pasta - ingredi ... +OR (if the request is complete or cannot proceed, do not include ``): + Use ONLY when request is fully completed successfully. Include confirmation message of what was accomplished. @@ -129,3 +133,5 @@ OR Use when request cannot be completed due to insurmountable errors or constraints. Explain why it failed. + +**Important:** Use exactly one of `` or ``; never both. The `` tag must include `success="true"` or `success="false"`. diff --git a/mobilerun/config/prompts/manager/stateless.jinja2 b/mobilerun/config/prompts/manager/stateless.jinja2 index 3140662c..435a0e59 100644 --- a/mobilerun/config/prompts/manager/stateless.jinja2 +++ b/mobilerun/config/prompts/manager/stateless.jinja2 @@ -121,5 +121,5 @@ Use this when the task cannot be completed. Explain why it failed. IMPORTANT: -- Use EITHER OR , never both. If you include both, your response will be rejected. +- Use exactly one of or , never both. If you include both and , your response will be rejected. - The tag MUST include the success attribute (success="true" or success="false"). diff --git a/mobilerun/config/prompts/manager/system.jinja2 b/mobilerun/config/prompts/manager/system.jinja2 index 8a97d8e0..832ca094 100644 --- a/mobilerun/config/prompts/manager/system.jinja2 +++ b/mobilerun/config/prompts/manager/system.jinja2 @@ -98,7 +98,7 @@ If an `` block is present, it contains live corrections o Carefully assess the current status and the provided screenshot. Check if the current plan needs to be revised. Determine if the user request has been fully completed. If you are confident that no further actions are required and the task succeeded, use the request_accomplished tag with success="true" and a confirmation message. If the task failed and cannot be completed, use success="false" with an explanation. If the user request is not finished, update the plan and don't use the request_accomplished tag. If you are stuck with errors, think step by step about whether the overall plan needs to be revised to address the error. NOTE: 1. If the current situation prevents proceeding with the original plan or requires clarification from the user, make reasonable assumptions and revise the plan accordingly. Act as though you are the user in such cases. 2. Please refer to the helpful information and steps in the Guidelines first for planning. 3. If the first subgoal in plan has been completed, please update the plan in time according to the screenshot and progress to ensure that the next subgoal is always the first item in the plan. 4. If the first subgoal is not completed, please copy the previous round's plan or update the plan based on the completion of the subgoal. -Provide your output in the following format, which contains four or five parts: +Provide your output in the following format. Include , optionally include , and then include exactly one of or . Do not include both and . An explanation of your rationale for the updated plan and current subgoal. @@ -114,9 +114,11 @@ Store important information here with step context for later reference. -Please update or copy the existing plan according to the current page and progress. Please pay close attention to the historical operations. Please do not repeat the plan of completed content unless you can judge from the screen status that a subgoal is indeed not completed. +Use this tag only when the user request is not finished. Please update or copy the existing plan according to the current page and progress. Please pay close attention to the historical operations. Please do not repeat the plan of completed content unless you can judge from the screen status that a subgoal is indeed not completed. +OR (if the request is complete or cannot proceed, do not include ): + Use this tag ONLY after successfully completing the user's request through concrete actions, not at the beginning or for planning. @@ -136,3 +138,7 @@ Use this tag when the task cannot be completed due to insurmountable errors or c 3. Ensure both opening and closing tags are present 4. Use only when you are certain the task cannot proceed further + +IMPORTANT: +- Use exactly one of or ; never both. +- The tag MUST include success="true" or success="false". diff --git a/tests/test_manager_response_validation.py b/tests/test_manager_response_validation.py new file mode 100644 index 00000000..2eb63517 --- /dev/null +++ b/tests/test_manager_response_validation.py @@ -0,0 +1,360 @@ +import asyncio +from pathlib import Path +from types import SimpleNamespace + +import pytest +from llama_index.core.base.llms.types import ChatMessage + +from mobilerun.agent.droid.droid_agent import MobileAgent +from mobilerun.agent.droid.events import ( + FinalizeEvent, + ManagerInputEvent, + ManagerPlanEvent, +) +from mobilerun.agent.manager.manager_agent import ManagerAgent +from mobilerun.agent.manager.prompts import ( + ManagerResponseValidationError, + parse_manager_response, + strip_manager_final_tags, + validate_manager_response, +) +from mobilerun.agent.manager.stateless_manager_agent import StatelessManagerAgent + + +def _response(content: str) -> SimpleNamespace: + return SimpleNamespace(message=SimpleNamespace(content=content)) + + +def _stateful_manager() -> ManagerAgent: + agent = object.__new__(ManagerAgent) + agent.llm = SimpleNamespace() + agent.agent_config = SimpleNamespace(streaming=False) + return agent + + +def _stateless_manager() -> StatelessManagerAgent: + agent = object.__new__(StatelessManagerAgent) + agent.llm = SimpleNamespace() + return agent + + +def _run(coro): + return asyncio.run(coro) + + +def test_parse_final_success_attribute_is_bound_to_matching_final_tag(): + parsed = parse_manager_response( + """ + done + + Android 16 is installed. + + """ + ) + + assert parsed["answer"] == "Android 16 is installed." + assert parsed["success"] is True + assert parsed["final_tag"] == "request_accomplished" + + +def test_parse_answer_alias_and_missing_success_stays_none(): + parsed = parse_manager_response("Done") + + assert parsed["answer"] == "Done" + assert parsed["success"] is None + assert parsed["final_tag"] == "answer" + + +def test_validate_rejects_plan_plus_final_answer_but_can_continue_with_plan(): + parsed = parse_manager_response( + """ + the version is visible + 1. Read the Android version from Settings. + Android 16. + """ + ) + + validation = validate_manager_response(parsed) + + assert not validation.is_valid + assert validation.can_continue_with_plan + assert "exactly one" in validation.error_message + + +@pytest.mark.parametrize( + "response", + [ + "1. Read version" + "", + "1. Read version" + "Done" + "Failed", + ], +) +def test_validate_rejects_stray_final_tags_but_can_continue_with_valid_plan( + response, +): + validation = validate_manager_response(parse_manager_response(response)) + + assert not validation.is_valid + assert validation.can_continue_with_plan + + +def test_strip_manager_final_tags_keeps_plan_for_safe_continuation(): + output = """ + the version is visible + 1. Read the Android version from Settings. + Android 16. + """ + + sanitized = strip_manager_final_tags(output) + parsed = parse_manager_response(sanitized) + + assert parsed["plan"] == "1. Read the Android version from Settings." + assert parsed["answer"] == "" + assert parsed["success"] is None + + +@pytest.mark.parametrize( + "response, expected_error", + [ + ("", "provide exactly one"), + ("Done", "success"), + ( + "Done", + "success", + ), + ( + "", + "must not be empty", + ), + ( + "1. Open Settings2. Read version", + "multiple ", + ), + ( + "Done", + " tag must not be empty", + ), + ( + "DoneNo", + "multiple final", + ), + ], +) +def test_validate_rejects_malformed_responses(response, expected_error): + validation = validate_manager_response(parse_manager_response(response)) + + assert not validation.is_valid + assert expected_error in validation.error_message + + +@pytest.mark.parametrize( + "response", + [ + "1. Open Settings", + "Done", + "Could not complete.", + ], +) +def test_validate_accepts_single_valid_manager_output(response): + validation = validate_manager_response(parse_manager_response(response)) + + assert validation.is_valid + assert validation.error_message is None + + +def test_stateful_manager_retries_invalid_response_then_returns_valid(monkeypatch): + calls = [] + + async def fake_acall(llm, messages, stream=False): + calls.append((llm, messages, stream)) + return _response( + "Android 16." + ) + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + output = _run( + _stateful_manager()._validate_and_retry( + [ChatMessage(role="user", content="prompt")], + "1. Read version" + "Android 16.", + ) + ) + + assert output == ( + "Android 16." + ) + assert len(calls) == 1 + assert calls[0][2] is False + + +def test_stateful_manager_strips_final_answer_after_retry_exhaustion(monkeypatch): + async def fake_acall(llm, messages, stream=False): + return _response( + "1. Read version" + "Android 16." + ) + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + output = _run( + _stateful_manager()._validate_and_retry( + [ChatMessage(role="user", content="prompt")], + "1. Read version" + "Android 16.", + ) + ) + + parsed = parse_manager_response(output) + assert parsed["plan"] == "1. Read version" + assert parsed["answer"] == "" + + +def test_stateful_manager_raises_validation_error_after_retry_exception(monkeypatch): + async def fake_acall(llm, messages, stream=False): + raise RuntimeError("provider unavailable") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + with pytest.raises(ManagerResponseValidationError): + _run( + _stateful_manager()._validate_and_retry( + [ChatMessage(role="user", content="prompt")], + "Done", + ) + ) + + +def test_stateful_manager_strips_final_answer_after_retry_exception(monkeypatch): + async def fake_acall(llm, messages, stream=False): + raise RuntimeError("provider unavailable") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + output = _run( + _stateful_manager()._validate_and_retry( + [ChatMessage(role="user", content="prompt")], + "1. Read version" + "Android 16.", + ) + ) + + parsed = parse_manager_response(output) + assert parsed["plan"] == "1. Read version" + assert parsed["answer"] == "" + + +def test_stateless_manager_strips_final_answer_after_retry_exhaustion(monkeypatch): + async def fake_acall(llm, messages): + return _response( + "1. Read version" "Android 16." + ) + + monkeypatch.setattr( + "mobilerun.agent.manager.stateless_manager_agent.acall_with_retries", fake_acall + ) + + output = _run( + _stateless_manager()._validate_and_retry( + [{"role": "user", "content": [{"text": "prompt"}]}], + "1. Read version" + "Android 16.", + ) + ) + + parsed = parse_manager_response(output) + assert parsed["plan"] == "1. Read version" + assert parsed["answer"] == "" + + +def test_stateless_manager_raises_validation_error_after_retry_exhaustion(monkeypatch): + async def fake_acall(llm, messages): + return _response("Done") + + monkeypatch.setattr( + "mobilerun.agent.manager.stateless_manager_agent.acall_with_retries", fake_acall + ) + + with pytest.raises(ManagerResponseValidationError): + _run( + _stateless_manager()._validate_and_retry( + [{"role": "user", "content": [{"text": "prompt"}]}], + "Done", + ) + ) + + +def test_droid_agent_run_manager_turns_validation_error_into_failed_finalize(): + class FakeHandler: + async def stream_events(self): + if False: + yield None + + def __await__(self): + async def fail(): + raise ManagerResponseValidationError("bad manager response") + + return fail().__await__() + + events = [] + agent = object.__new__(MobileAgent) + agent.shared_state = SimpleNamespace( + step_number=0, + drain_user_messages=lambda: [], + ) + agent.config = SimpleNamespace(agent=SimpleNamespace(max_steps=5)) + agent.manager_agent = SimpleNamespace(run=lambda: FakeHandler()) + agent.handle_stream_event = lambda nested_ev, ctx: None + ctx = SimpleNamespace(write_event_to_stream=events.append) + + result = _run(agent.run_manager(ctx, ManagerInputEvent())) + + assert isinstance(result, FinalizeEvent) + assert result.success is False + assert result.reason == "bad manager response" + assert events == [] + + +def test_droid_agent_does_not_default_missing_success_to_true(): + agent = object.__new__(MobileAgent) + agent.shared_state = SimpleNamespace( + pending_user_messages=[], + progress_summary="", + ) + + event = ManagerPlanEvent( + plan="", + current_subgoal="", + thought="done", + answer="Done, but success is missing.", + success=None, + ) + + result = _run(agent.handle_manager_plan(SimpleNamespace(), event)) + + assert isinstance(result, FinalizeEvent) + assert result.success is False + assert result.reason == "Done, but success is missing." + + +def test_manager_prompt_contracts_require_exactly_one_terminal_form(): + prompt_dir = Path("mobilerun/config/prompts/manager") + for prompt_name in ("system.jinja2", "rev1.jinja2", "stateless.jinja2"): + prompt = (prompt_dir / prompt_name).read_text() + assert "exactly one" in prompt.lower() + assert ( + "both " in prompt.lower() or "do not include " in prompt.lower() + ) + + trained = (prompt_dir / "trained.jinja2").read_text() + assert "both " not in trained.lower() From 442bc4f8d4341d50f37680816b21c436e5918a37 Mon Sep 17 00:00:00 2001 From: "rasul.osmanbayli" Date: Tue, 23 Jun 2026 13:21:26 +0400 Subject: [PATCH 2/3] Soften manager missing-success fallback --- mobilerun/agent/manager/manager_agent.py | 9 +++ mobilerun/agent/manager/prompts.py | 26 +++++++- .../agent/manager/stateless_manager_agent.py | 9 +++ mobilerun/config/prompts/manager/rev1.jinja2 | 2 - .../config/prompts/manager/system.jinja2 | 4 -- tests/test_manager_response_validation.py | 61 ++++++++++++++++++- 6 files changed, 101 insertions(+), 10 deletions(-) diff --git a/mobilerun/agent/manager/manager_agent.py b/mobilerun/agent/manager/manager_agent.py index 678632d6..94ed903f 100644 --- a/mobilerun/agent/manager/manager_agent.py +++ b/mobilerun/agent/manager/manager_agent.py @@ -37,6 +37,7 @@ ) from mobilerun.agent.manager.prompts import ( ManagerResponseValidationError, + add_default_success_to_final_tag, parse_manager_response, strip_manager_final_tags, validate_manager_response, @@ -354,6 +355,8 @@ async def _validate_and_retry( logger.error(f"LLM retry failed: {e}") if validation.can_continue_with_plan: return strip_manager_final_tags(output) + if validation.can_accept_final_without_success: + return add_default_success_to_final_tag(output) raise ManagerResponseValidationError(error_message) from e validation = validate_manager_response(parsed) @@ -365,6 +368,12 @@ async def _validate_and_retry( "and ignoring the conflicting final answer." ) return strip_manager_final_tags(output) + if validation.can_accept_final_without_success: + logger.warning( + "Manager response omitted final success after retries; accepting the " + "terminal answer as success for backward compatibility." + ) + return add_default_success_to_final_tag(output) raise ManagerResponseValidationError( validation.error_message or "Invalid manager response." diff --git a/mobilerun/agent/manager/prompts.py b/mobilerun/agent/manager/prompts.py index b903b6b2..952c4aeb 100644 --- a/mobilerun/agent/manager/prompts.py +++ b/mobilerun/agent/manager/prompts.py @@ -11,12 +11,17 @@ r"""\bsuccess\s*=\s*(?P["'])(?Ptrue|false)(?P=quote)""", re.IGNORECASE | re.DOTALL, ) +_SUCCESS_ATTR_PRESENT_RE = re.compile(r"\bsuccess\s*=", re.IGNORECASE | re.DOTALL) _FINAL_TAG_RE = re.compile( r"<(?Prequest_accomplished|answer)\b(?P[^>]*)>" r"(?P.*?)" r"", re.IGNORECASE | re.DOTALL, ) +_FINAL_OPEN_TAG_RE = re.compile( + r"<(?Prequest_accomplished|answer)\b(?P[^>]*)>", + re.IGNORECASE | re.DOTALL, +) @dataclass(frozen=True) @@ -24,6 +29,7 @@ class ManagerResponseValidation: is_valid: bool error_message: str | None = None can_continue_with_plan: bool = False + can_accept_final_without_success: bool = False class ManagerResponseValidationError(RuntimeError): @@ -54,6 +60,18 @@ def strip_manager_final_tags(response: str) -> str: return _FINAL_TAG_RE.sub("", response).strip() +def add_default_success_to_final_tag(response: str) -> str: + """Add success=true to a final tag that omitted the success attribute.""" + + def replace(match: re.Match[str]) -> str: + attrs = match.group("attrs") + if _SUCCESS_ATTR_PRESENT_RE.search(attrs): + return match.group(0) + return f'<{match.group("tag")}{attrs} success="true">' + + return _FINAL_OPEN_TAG_RE.sub(replace, response, count=1) + + def parse_manager_response(response: str) -> dict: """ Parse manager LLM response into structured dict. @@ -115,9 +133,12 @@ def extract_all(tag: str) -> str: answer = _tag_content(final_match) if final_match else "" success = None final_tag = None + success_attr_present = False if final_match: final_tag = final_match.group("tag").lower() - success = _success_from_attrs(final_match.group("attrs")) + final_attrs = final_match.group("attrs") + success_attr_present = bool(_SUCCESS_ATTR_PRESENT_RE.search(final_attrs)) + success = _success_from_attrs(final_attrs) final_counts = { tag: sum(1 for match in final_matches if match.group("tag").lower() == tag) @@ -161,6 +182,7 @@ def extract_all(tag: str) -> str: "current_subgoal": current_subgoal, "answer": answer, "success": success, + "success_attr_present": success_attr_present, "progress_summary": progress_summary, "final_tag": final_tag, "tag_counts": { @@ -177,6 +199,7 @@ def validate_manager_response(parsed: dict) -> ManagerResponseValidation: plan = (parsed.get("plan") or "").strip() answer = (parsed.get("answer") or "").strip() success = parsed.get("success") + success_attr_present = bool(parsed.get("success_attr_present")) tag_counts = parsed.get("tag_counts") or {} plan_count = tag_counts.get("plan", 1 if plan else 0) final_count = tag_counts.get("final", 1 if answer else 0) @@ -223,6 +246,7 @@ def validate_manager_response(parsed: dict) -> ManagerResponseValidation: is_valid=False, error_message='Final answer tag must include success="true" ' 'or success="false".', + can_accept_final_without_success=not success_attr_present, ) return ManagerResponseValidation(is_valid=True) diff --git a/mobilerun/agent/manager/stateless_manager_agent.py b/mobilerun/agent/manager/stateless_manager_agent.py index ef966cb3..e35623b7 100644 --- a/mobilerun/agent/manager/stateless_manager_agent.py +++ b/mobilerun/agent/manager/stateless_manager_agent.py @@ -21,6 +21,7 @@ ) from mobilerun.agent.manager.prompts import ( ManagerResponseValidationError, + add_default_success_to_final_tag, parse_manager_response, strip_manager_final_tags, validate_manager_response, @@ -154,6 +155,8 @@ async def _validate_and_retry( logger.error(f"LLM retry failed: {e}") if validation.can_continue_with_plan: return strip_manager_final_tags(output) + if validation.can_accept_final_without_success: + return add_default_success_to_final_tag(output) raise ManagerResponseValidationError(error_message) from e validation = validate_manager_response(parsed) @@ -165,6 +168,12 @@ async def _validate_and_retry( "and ignoring the conflicting final answer." ) return strip_manager_final_tags(output) + if validation.can_accept_final_without_success: + logger.warning( + "Manager response omitted final success after retries; accepting the " + "terminal answer as success for backward compatibility." + ) + return add_default_success_to_final_tag(output) raise ManagerResponseValidationError( validation.error_message or "Invalid manager response." diff --git a/mobilerun/config/prompts/manager/rev1.jinja2 b/mobilerun/config/prompts/manager/rev1.jinja2 index 166e53ae..6acf2082 100644 --- a/mobilerun/config/prompts/manager/rev1.jinja2 +++ b/mobilerun/config/prompts/manager/rev1.jinja2 @@ -133,5 +133,3 @@ OR Use when request cannot be completed due to insurmountable errors or constraints. Explain why it failed. - -**Important:** Use exactly one of `` or ``; never both. The `` tag must include `success="true"` or `success="false"`. diff --git a/mobilerun/config/prompts/manager/system.jinja2 b/mobilerun/config/prompts/manager/system.jinja2 index 832ca094..0dc2e703 100644 --- a/mobilerun/config/prompts/manager/system.jinja2 +++ b/mobilerun/config/prompts/manager/system.jinja2 @@ -138,7 +138,3 @@ Use this tag when the task cannot be completed due to insurmountable errors or c 3. Ensure both opening and closing tags are present 4. Use only when you are certain the task cannot proceed further - -IMPORTANT: -- Use exactly one of or ; never both. -- The tag MUST include success="true" or success="false". diff --git a/tests/test_manager_response_validation.py b/tests/test_manager_response_validation.py index 2eb63517..c2862a31 100644 --- a/tests/test_manager_response_validation.py +++ b/tests/test_manager_response_validation.py @@ -62,6 +62,7 @@ def test_parse_answer_alias_and_missing_success_stays_none(): assert parsed["answer"] == "Done" assert parsed["success"] is None + assert parsed["success_attr_present"] is False assert parsed["final_tag"] == "answer" @@ -81,6 +82,16 @@ def test_validate_rejects_plan_plus_final_answer_but_can_continue_with_plan(): assert "exactly one" in validation.error_message +def test_validate_missing_success_final_answer_can_fallback_to_success(): + validation = validate_manager_response( + parse_manager_response("Done") + ) + + assert not validation.is_valid + assert validation.can_accept_final_without_success + assert "success" in validation.error_message + + @pytest.mark.parametrize( "response", [ @@ -228,11 +239,55 @@ async def fake_acall(llm, messages, stream=False): _run( _stateful_manager()._validate_and_retry( [ChatMessage(role="user", content="prompt")], - "Done", + "Done", ) ) +def test_stateful_manager_normalizes_missing_success_after_retry_exhaustion( + monkeypatch, +): + async def fake_acall(llm, messages, stream=False): + return _response("Done") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + output = _run( + _stateful_manager()._validate_and_retry( + [ChatMessage(role="user", content="prompt")], + "Done", + ) + ) + + parsed = parse_manager_response(output) + assert parsed["answer"] == "Done" + assert parsed["success"] is True + + +def test_stateful_manager_normalizes_missing_success_after_retry_exception( + monkeypatch, +): + async def fake_acall(llm, messages, stream=False): + raise RuntimeError("provider unavailable") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + output = _run( + _stateful_manager()._validate_and_retry( + [ChatMessage(role="user", content="prompt")], + "Done", + ) + ) + + parsed = parse_manager_response(output) + assert parsed["answer"] == "Done" + assert parsed["success"] is True + + def test_stateful_manager_strips_final_answer_after_retry_exception(monkeypatch): async def fake_acall(llm, messages, stream=False): raise RuntimeError("provider unavailable") @@ -279,7 +334,7 @@ async def fake_acall(llm, messages): def test_stateless_manager_raises_validation_error_after_retry_exhaustion(monkeypatch): async def fake_acall(llm, messages): - return _response("Done") + return _response("Done") monkeypatch.setattr( "mobilerun.agent.manager.stateless_manager_agent.acall_with_retries", fake_acall @@ -289,7 +344,7 @@ async def fake_acall(llm, messages): _run( _stateless_manager()._validate_and_retry( [{"role": "user", "content": [{"text": "prompt"}]}], - "Done", + "Done", ) ) From 397b5ec35a991c7a05e66dc85150a6fa7840573f Mon Sep 17 00:00:00 2001 From: "rasul.osmanbayli" Date: Tue, 23 Jun 2026 14:57:40 +0400 Subject: [PATCH 3/3] Add forced reasoning fallback coverage --- tests/test_reasoning_fallback_integration.py | 175 +++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 tests/test_reasoning_fallback_integration.py diff --git a/tests/test_reasoning_fallback_integration.py b/tests/test_reasoning_fallback_integration.py new file mode 100644 index 00000000..54d72749 --- /dev/null +++ b/tests/test_reasoning_fallback_integration.py @@ -0,0 +1,175 @@ +import asyncio +import logging +from types import SimpleNamespace + +import pytest +from llama_index.core.base.llms.types import ChatMessage + +from mobilerun.agent.manager.events import ManagerContextEvent +from mobilerun.agent.manager.manager_agent import ManagerAgent +from mobilerun.agent.manager.prompts import ( + ManagerResponseValidationError, + parse_manager_response, +) + + +def _response(content: str) -> SimpleNamespace: + return SimpleNamespace(message=SimpleNamespace(content=content)) + + +def _run(coro): + return asyncio.run(coro) + + +class FakeContext: + def __init__(self) -> None: + self.events = [] + + def write_event_to_stream(self, event) -> None: + self.events.append(event) + + +def _manager() -> ManagerAgent: + agent = object.__new__(ManagerAgent) + agent.llm = SimpleNamespace(class_name=lambda: "FakeLLM") + agent.agent_config = SimpleNamespace(streaming=False) + agent.shared_state = SimpleNamespace( + screenshot=None, + message_history=[], + previous_plan="", + plan="", + current_subgoal="", + last_thought="", + answer="", + progress_summary="", + append_memory=lambda text: None, + ) + + async def build_system_prompt() -> str: + return "system" + + agent._build_system_prompt = build_system_prompt + agent._build_messages_with_context = lambda **kwargs: [ + ChatMessage(role="user", content="prompt") + ] + return agent + + +@pytest.fixture +def mobilerun_caplog(caplog): + logger = logging.getLogger("mobilerun") + previous = logger.propagate + logger.propagate = True + caplog.set_level(logging.WARNING, logger="mobilerun") + yield caplog + logger.propagate = previous + + +def test_manager_boundary_normalizes_missing_success_after_retry_exhaustion( + monkeypatch, + mobilerun_caplog, +): + calls = [] + + async def fake_acall(llm, messages, stream=False): + calls.append((llm, messages, stream)) + return _response("Done") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + manager = _manager() + ctx = FakeContext() + + response_event = _run(manager.get_response(ctx, ManagerContextEvent())) + details_event = _run(manager.process_response(ctx, response_event)) + parsed = parse_manager_response(response_event.response) + + assert len(calls) == 4 + assert parsed["answer"] == "Done" + assert parsed["success"] is True + assert details_event.answer == "Done" + assert details_event.success is True + assert any( + "omitted final success after retries" in record.message + for record in mobilerun_caplog.records + ) + + +def test_manager_boundary_rejects_malformed_success_after_retry_exhaustion( + monkeypatch, +): + calls = [] + + async def fake_acall(llm, messages, stream=False): + calls.append((llm, messages, stream)) + return _response("Done") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + with pytest.raises(ManagerResponseValidationError): + _run(_manager().get_response(FakeContext(), ManagerContextEvent())) + + assert len(calls) == 4 + + +def test_manager_boundary_rejects_empty_final_body_after_retry_exhaustion( + monkeypatch, +): + calls = [] + + async def fake_acall(llm, messages, stream=False): + calls.append((llm, messages, stream)) + return _response("") + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + with pytest.raises(ManagerResponseValidationError): + _run(_manager().get_response(FakeContext(), ManagerContextEvent())) + + assert len(calls) == 4 + + +def test_manager_boundary_plan_final_conflict_does_not_fallback_to_success( + monkeypatch, + mobilerun_caplog, +): + calls = [] + invalid_response = ( + "1. Continue checking the screen." + "Done" + ) + + async def fake_acall(llm, messages, stream=False): + calls.append((llm, messages, stream)) + return _response(invalid_response) + + monkeypatch.setattr( + "mobilerun.agent.manager.manager_agent.acall_with_retries", fake_acall + ) + + manager = _manager() + ctx = FakeContext() + + response_event = _run(manager.get_response(ctx, ManagerContextEvent())) + details_event = _run(manager.process_response(ctx, response_event)) + parsed = parse_manager_response(response_event.response) + + assert len(calls) == 4 + assert parsed["plan"] == "1. Continue checking the screen." + assert parsed["answer"] == "" + assert parsed["success"] is None + assert details_event.success is None + assert any( + "continuing with the plan" in record.message + for record in mobilerun_caplog.records + ) + assert not any( + "omitted final success after retries" in record.message + for record in mobilerun_caplog.records + )