diff --git a/phone_agent/model/client.py b/phone_agent/model/client.py index 72377a619..83ae89f3c 100644 --- a/phone_agent/model/client.py +++ b/phone_agent/model/client.py @@ -178,11 +178,18 @@ def _parse_response(self, content: str) -> tuple[str, str]: Parse the model response into thinking and action parts. Parsing rules: - 1. If content contains 'finish(message=', everything before is thinking, - everything from 'finish(message=' onwards is action. - 2. If rule 1 doesn't apply but content contains 'do(action=', - everything before is thinking, everything from 'do(action=' onwards is action. - 3. Fallback: If content contains '', use legacy parsing with XML tags. + 1. If content contains '' (the format requested by the system + prompt), extract the thinking from the '' tag and the action + from the '' tag. This is checked first because the answer + itself normally contains a 'do(action=' or 'finish(message=' call, + and matching those markers before stripping the tags would leave a + trailing '' inside the action string. + 2. If no answer tag is present but content contains 'finish(message=', + everything before is thinking, everything from 'finish(message=' + onwards is action. + 3. If rule 2 doesn't apply but content contains 'do(action=', + everything before is thinking, everything from 'do(action=' onwards + is action. 4. Otherwise, return empty thinking and full content as action. Args: @@ -191,27 +198,30 @@ def _parse_response(self, content: str) -> tuple[str, str]: Returns: Tuple of (thinking, action). """ - # Rule 1: Check for finish(message= + # Rule 1: Prefer the XML tag format requested by the system prompt. + # The '' block usually wraps a do()/finish() call, so it must + # be handled before the marker checks below; otherwise the trailing + # '' would be captured as part of the action. + if "" in content: + parts = content.split("", 1) + thinking = parts[0].replace("", "").replace("", "").strip() + action = parts[1].replace("", "").strip() + return thinking, action + + # Rule 2: Check for finish(message= if "finish(message=" in content: parts = content.split("finish(message=", 1) thinking = parts[0].strip() action = "finish(message=" + parts[1] return thinking, action - # Rule 2: Check for do(action= + # Rule 3: Check for do(action= if "do(action=" in content: parts = content.split("do(action=", 1) thinking = parts[0].strip() action = "do(action=" + parts[1] return thinking, action - # Rule 3: Fallback to legacy XML tag parsing - if "" in content: - parts = content.split("", 1) - thinking = parts[0].replace("", "").replace("", "").strip() - action = parts[1].replace("", "").strip() - return thinking, action - # Rule 4: No markers found, return content as action return "", content diff --git a/tests/test_parse_response_order.py b/tests/test_parse_response_order.py new file mode 100644 index 000000000..8d808efbf --- /dev/null +++ b/tests/test_parse_response_order.py @@ -0,0 +1,104 @@ +"""Regression tests for ModelClient._parse_response tag/marker ordering. + +The system prompt asks the model to answer in the form:: + + {reasoning} + {action} + +where ``{action}`` is a ``do(...)`` or ``finish(...)`` call. These tests pin +down that a fully tagged response is split into a clean ``(thinking, action)`` +pair, with the surrounding XML tags removed from *both* parts. + +Previously the ``do(action=`` / ``finish(message=`` markers were matched before +the ```` tag was stripped, so the action string retained a trailing +```` (e.g. ``do(action="Tap", element=[500, 300])``). That +string is not valid Python and later fails ``parse_action``, which makes the +agent abort the step. See ``phone_agent.model.client.ModelClient._parse_response``. + +No network or device access is required: ``ModelClient`` is instantiated with +its default config and the OpenAI client constructor performs no I/O. +""" + +import pytest + +from phone_agent.model.client import ModelClient + + +@pytest.fixture +def client(): + """A ModelClient built from the default configuration.""" + return ModelClient() + + +class TestTaggedResponsesAreClean: + """A tagged answer must not leak XML tags into the action string.""" + + def test_tagged_do_action_has_no_trailing_answer_tag(self, client): + content = ( + "I should tap the search box" + 'do(action="Tap", element=[500, 300])' + ) + thinking, action = client._parse_response(content) + + assert thinking == "I should tap the search box" + assert action == 'do(action="Tap", element=[500, 300])' + # Regression: the closing tag must not remain in the action. + assert "" not in action + assert "" not in action + + def test_tagged_finish_action_has_no_trailing_answer_tag(self, client): + content = ( + "task is complete" + 'finish(message="all done")' + ) + thinking, action = client._parse_response(content) + + assert thinking == "task is complete" + assert action == 'finish(message="all done")' + assert "" not in action + + def test_tagged_response_with_newlines(self, client): + content = ( + "navigate back\ndo(action=\"Back\")" + ) + thinking, action = client._parse_response(content) + + assert thinking == "navigate back" + assert action == 'do(action="Back")' + + def test_answer_tag_without_think_tag(self, client): + content = 'some reasoningdo(action="Home")' + thinking, action = client._parse_response(content) + + assert thinking == "some reasoning" + assert action == 'do(action="Home")' + + +class TestTaglessResponsesUnchanged: + """Existing tag-less behaviour (Rules 2-4) must be preserved.""" + + def test_tagless_do(self, client): + thinking, action = client._parse_response( + 'I will tap the button. do(action="Tap", element=[500, 500])' + ) + assert thinking == "I will tap the button." + assert action == 'do(action="Tap", element=[500, 500])' + + def test_tagless_finish(self, client): + thinking, action = client._parse_response( + 'I have completed the task. finish(message="all done")' + ) + assert thinking == "I have completed the task." + assert action == 'finish(message="all done")' + + def test_tagless_finish_precedes_do(self, client): + thinking, action = client._parse_response( + 'do(action="Tap", element=[1, 2]) then finish(message="ok")' + ) + assert thinking == 'do(action="Tap", element=[1, 2]) then' + assert action == 'finish(message="ok")' + + def test_plain_text_fallback(self, client): + thinking, action = client._parse_response("just some plain text") + assert thinking == "" + assert action == "just some plain text"