fix(SKY-11910): surface custom dropdown options in MCP observe + non-leaking action errors (#7165)

Co-authored-by: AronPerez <aperez0295@gmail.com>

tests/unit/test_mcp_browser_tools.py

@@ -4,12 +4,13 @@ from __future__ import annotations
 
 import asyncio
 from types import SimpleNamespace
-from unittest.mock import AsyncMock
+from unittest.mock import AsyncMock, MagicMock
 
 import pytest
 
 from skyvern.cli.core.result import BrowserContext
 from skyvern.cli.mcp_tools import browser as mcp_browser
+from skyvern.client.errors import InternalServerError, UnprocessableEntityError
 
 
 @pytest.mark.asyncio
@@ -192,6 +193,35 @@ async def test_skyvern_click_selector_plus_intent_falls_back_by_default(monkeypa
     assert click.await_args.kwargs.get("mode") != "direct"
 
 
+@pytest.mark.asyncio
+async def test_skyvern_click_selector_plus_intent_runs_native_option_precheck(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    page, select_option = _native_option_page(monkeypatch)
+
+    result = await mcp_browser.skyvern_click(selector="#region > option:nth-of-type(4)", intent="pick the region")
+
+    assert result["ok"] is True
+    page.click.assert_not_awaited()
+    select_option.assert_awaited_once_with(value="east", timeout=30000)
+    assert result["data"]["selected_option"]["select_selector"] == "#region"
+
+
+@pytest.mark.asyncio
+async def test_skyvern_click_selector_plus_intent_probe_miss_falls_back(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    click = _click_page(monkeypatch)
+    probe = AsyncMock(return_value=None)
+    monkeypatch.setattr(mcp_browser, "select_native_option_if_targeted", probe)
+
+    result = await mcp_browser.skyvern_click(selector="#possibly-stale", intent="the blue submit button")
+
+    assert result["ok"] is True
+    probe.assert_awaited_once()
+    assert click.await_args.kwargs.get("ai") == "fallback"
+
+
 @pytest.mark.asyncio
 async def test_skyvern_click_selector_mode_direct_ignores_intent(monkeypatch: pytest.MonkeyPatch) -> None:
     click = _click_page(monkeypatch)
@@ -222,6 +252,33 @@ def _action_page(monkeypatch: pytest.MonkeyPatch, **methods: AsyncMock) -> None:
     monkeypatch.setattr(mcp_browser, "get_page", AsyncMock(return_value=(page, context)))
 
 
+def _native_option_page(
+    monkeypatch: pytest.MonkeyPatch,
+    *,
+    option_selector: str = "#region > option:nth-of-type(4)",
+    option_info: dict[str, str] | None = None,
+    select_side_effect: object | None = None,
+) -> tuple[SimpleNamespace, AsyncMock]:
+    option_info = option_info or {
+        "select_selector": "#region",
+        "value": "east",
+        "label": "East",
+    }
+    option_locator = SimpleNamespace(evaluate=AsyncMock(return_value=option_info))
+    option_locator.first = option_locator
+    select_option = AsyncMock(side_effect=select_side_effect)
+    select_locator = SimpleNamespace(select_option=select_option)
+    raw_page = SimpleNamespace(
+        locator=MagicMock(
+            side_effect=lambda selector: option_locator if selector == option_selector else select_locator
+        )
+    )
+    page = SimpleNamespace(page=raw_page, click=AsyncMock(return_value="#unexpected-click"))
+    context = BrowserContext(mode="cloud_session", session_id="pbs_test")
+    monkeypatch.setattr(mcp_browser, "get_page", AsyncMock(return_value=(page, context)))
+    return page, select_option
+
+
 @pytest.mark.asyncio
 async def test_skyvern_type_selector_is_resilient_by_default(monkeypatch: pytest.MonkeyPatch) -> None:
     fill = AsyncMock(return_value="Noor")
@@ -269,6 +326,175 @@ async def test_skyvern_type_intent_only_uses_proactive_ai(monkeypatch: pytest.Mo
     assert fill.await_args.kwargs.get("mode") != "direct"
 
 
+@pytest.mark.asyncio
+async def test_skyvern_click_native_option_selector_uses_parent_select(monkeypatch: pytest.MonkeyPatch) -> None:
+    page, select_option = _native_option_page(monkeypatch)
+
+    result = await mcp_browser.skyvern_click(selector="#region > option:nth-of-type(4)")
+
+    assert result["ok"] is True
+    page.click.assert_not_awaited()
+    select_option.assert_awaited_once_with(value="east", timeout=30000)
+    assert result["data"]["resolved_selector"] == "#region"
+    assert result["data"]["selected_option"] == {
+        "select_selector": "#region",
+        "selected_by": "value",
+        "value": "east",
+        "label": "East",
+    }
+    assert result["data"]["sdk_equivalent"] == 'await page.select_option("#region", value="east")'
+
+
+@pytest.mark.asyncio
+async def test_skyvern_click_native_option_selector_can_fall_back_to_label(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    page, select_option = _native_option_page(monkeypatch, select_side_effect=[Exception("bad value"), None])
+
+    result = await mcp_browser.skyvern_click(selector="#region > option:nth-of-type(4)", timeout=5000)
+
+    assert result["ok"] is True
+    page.click.assert_not_awaited()
+    assert select_option.await_args_list[0].kwargs == {"value": "east", "timeout": 5000}
+    assert select_option.await_args_list[1].kwargs == {"label": "East", "timeout": 5000}
+    assert result["data"]["selected_option"]["selected_by"] == "label"
+    assert result["data"]["sdk_equivalent"] == 'await page.select_option("#region", label="East")'
+
+
+@pytest.mark.asyncio
+async def test_skyvern_click_native_option_selector_selects_by_index(monkeypatch: pytest.MonkeyPatch) -> None:
+    # The ref identifies ONE specific <option>; a duplicate/empty value must not resolve to the
+    # wrong option. Selection must use the probe's index, not the value.
+    page, select_option = _native_option_page(
+        monkeypatch,
+        option_info={"select_selector": "#region", "index": 3, "value": "", "label": "East"},
+    )
+
+    result = await mcp_browser.skyvern_click(selector="#region > option:nth-of-type(4)")
+
+    assert result["ok"] is True
+    page.click.assert_not_awaited()
+    select_option.assert_awaited_once_with(index=3, timeout=30000)
+    assert result["data"]["selected_option"]["selected_by"] == "index"
+    assert result["data"]["selected_option"]["index"] == 3
+    assert result["data"]["sdk_equivalent"] == 'await page.select_option("#region", index=3)'
+
+
+@pytest.mark.asyncio
+async def test_skyvern_type_ai_error_surfaces_http_status_and_body(monkeypatch: pytest.MonkeyPatch) -> None:
+    # 4xx bodies are the API's intended client-facing feedback — surfaced for self-correction.
+    fill = AsyncMock(
+        side_effect=UnprocessableEntityError(
+            body={"detail": "Element locator did not match any element"},
+            headers={"authorization": "redacted"},
+        )
+    )
+    _action_page(monkeypatch, fill=fill)
+
+    result = await mcp_browser.skyvern_type(intent="the first name field", text="Noor")
+
+    assert result["ok"] is False
+    assert result["error"]["code"] == mcp_browser.ErrorCode.AI_FALLBACK_FAILED
+    assert result["error"]["message"] == "HTTP 422: Element locator did not match any element"
+    assert "headers" not in result["error"]["message"].lower()
+    assert "authorization" not in result["error"]["message"].lower()
+    assert result["error"]["details"] == {
+        "exception_type": "UnprocessableEntityError",
+        "status_code": 422,
+    }
+
+
+@pytest.mark.asyncio
+async def test_skyvern_type_ai_error_5xx_body_text_is_suppressed(monkeypatch: pytest.MonkeyPatch) -> None:
+    # 5xx bodies carry the backend's wrapped internal exception text
+    # ("Unexpected error: {exception}"); only status + exception type may surface.
+    fill = AsyncMock(side_effect=InternalServerError(body={"error": "Unexpected error: KeyError('internal-host')"}))
+    _action_page(monkeypatch, fill=fill)
+
+    result = await mcp_browser.skyvern_type(intent="the first name field", text="Noor")
+
+    assert result["ok"] is False
+    assert result["error"]["message"] == "HTTP 500: InternalServerError"
+    assert "internal-host" not in result["error"]["message"]
+    assert "Unexpected error" not in result["error"]["message"]
+
+
+@pytest.mark.asyncio
+async def test_skyvern_type_ai_error_empty_body_does_not_leak_headers(monkeypatch: pytest.MonkeyPatch) -> None:
+    # An ApiError whose body carries no recognized message key must not fall back to str(exc),
+    # which renders headers + raw body via the SDK ApiError.__str__. Surface only status + type.
+    fill = AsyncMock(
+        side_effect=InternalServerError(
+            body=None,
+            headers={"authorization": "Bearer sk-SECRET", "set-cookie": "sess=abc"},
+        )
+    )
+    _action_page(monkeypatch, fill=fill)
+
+    result = await mcp_browser.skyvern_type(intent="the first name field", text="Noor")
+
+    assert result["ok"] is False
+    message = result["error"]["message"]
+    assert message == "HTTP 500: InternalServerError"
+    for leaked in ("authorization", "bearer", "sk-secret", "set-cookie", "sess=abc", "headers"):
+        assert leaked not in message.lower()
+
+
+@pytest.mark.asyncio
+async def test_skyvern_type_ai_error_string_body_does_not_leak_token(monkeypatch: pytest.MonkeyPatch) -> None:
+    # A raw string body (not a recognized dict shape) can carry tokens/secrets; it must never
+    # be surfaced verbatim — only status + exception type.
+    fill = AsyncMock(side_effect=InternalServerError(body="upstream said: token=sk-LEAKED-1234 is invalid"))
+    _action_page(monkeypatch, fill=fill)
+
+    result = await mcp_browser.skyvern_type(intent="the first name field", text="Noor")
+
+    assert result["ok"] is False
+    message = result["error"]["message"]
+    assert message == "HTTP 500: InternalServerError"
+    assert "sk-LEAKED-1234" not in message
+    assert "token=" not in message
+
+
+@pytest.mark.asyncio
+async def test_skyvern_act_sdk_error_does_not_leak_headers(monkeypatch: pytest.MonkeyPatch) -> None:
+    # The SDK/agent handlers (act/extract/validate/run_task/login) must also route ApiError
+    # through _exception_message, not str(e) — otherwise ApiError.__str__ leaks headers/body.
+    _action_page(monkeypatch)
+    monkeypatch.setattr(
+        mcp_browser,
+        "do_act",
+        AsyncMock(side_effect=InternalServerError(body=None, headers={"authorization": "Bearer sk-SECRET"})),
+    )
+
+    result = await mcp_browser.skyvern_act(prompt="close the banner")
+
+    assert result["ok"] is False
+    message = result["error"]["message"]
+    assert message == "HTTP 500: InternalServerError"
+    for leaked in ("authorization", "bearer", "sk-secret", "headers"):
+        assert leaked not in message.lower()
+
+
+@pytest.mark.asyncio
+async def test_skyvern_type_ai_error_uses_exception_type_for_empty_message(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    class SilentFailure(Exception):
+        def __str__(self) -> str:
+            return ""
+
+    fill = AsyncMock(side_effect=SilentFailure())
+    _action_page(monkeypatch, fill=fill)
+
+    result = await mcp_browser.skyvern_type(intent="the first name field", text="Noor")
+
+    assert result["ok"] is False
+    assert result["error"]["code"] == mcp_browser.ErrorCode.AI_FALLBACK_FAILED
+    assert result["error"]["message"] == "SilentFailure"
+    assert result["error"]["details"] == {"exception_type": "SilentFailure"}
+
+
 @pytest.mark.asyncio
 async def test_skyvern_select_option_selector_is_resilient_by_default(monkeypatch: pytest.MonkeyPatch) -> None:
     select = AsyncMock(return_value="us")

tests/unit/test_mcp_observe_execute.py

@@ -250,6 +250,230 @@ class TestDoObserve:
         result = await do_observe(page)
         assert result.elements[0].options == ["US", "UK", "CA"]
 
+    @pytest.mark.asyncio
+    async def test_dom_interactables_are_merged_with_selectors(self) -> None:
+        page = _make_page(_make_a11y_tree(children=[{"role": "textbox", "name": "City", "value": ""}]))
+        page.evaluate = AsyncMock(
+            return_value=[
+                {
+                    "role": "option",
+                    "name": "Lakewood",
+                    "tag": "div",
+                    "selector": "#city-list > div:nth-of-type(1)",
+                }
+            ]
+        )
+
+        result = await do_observe(page)
+        serialized = serialize_elements(result.elements)
+
+        city_option = next(element for element in serialized if element["name"] == "Lakewood")
+        assert city_option["selector"] == "#city-list > div:nth-of-type(1)"
+        assert ref_to_selector(city_option) == "#city-list > div:nth-of-type(1)"
+
+    @pytest.mark.asyncio
+    async def test_dom_interactables_work_without_accessibility_snapshot(self) -> None:
+        page = SimpleNamespace(
+            url="https://example.com/form",
+            title=AsyncMock(return_value="Form"),
+            evaluate=AsyncMock(
+                return_value=[
+                    {
+                        "role": "option",
+                        "name": "Music",
+                        "tag": "div",
+                        "selector": "#category-options > div:nth-of-type(2)",
+                    }
+                ]
+            ),
+        )
+
+        result = await do_observe(page)
+        serialized = serialize_elements(result.elements)
+
+        assert len(serialized) == 1
+        assert serialized[0]["name"] == "Music"
+        assert serialized[0]["selector"] == "#category-options > div:nth-of-type(2)"
+
+    @pytest.mark.asyncio
+    async def test_dom_selector_is_attached_to_matching_a11y_option(self) -> None:
+        tree = _make_a11y_tree(
+            children=[
+                {
+                    "role": "combobox",
+                    "name": "Region",
+                    "children": [
+                        {"role": "option", "name": "North"},
+                        {"role": "option", "name": "East"},
+                    ],
+                },
+            ]
+        )
+        page = _make_page(tree)
+        page.evaluate = AsyncMock(
+            return_value=[
+                {
+                    "role": "option",
+                    "name": "East",
+                    "tag": "option",
+                    "selector": "#region > option:nth-of-type(2)",
+                    "value": "east",
+                }
+            ]
+        )
+
+        result = await do_observe(page)
+        serialized = serialize_elements(result.elements)
+
+        east_option = next(element for element in serialized if element["name"] == "East")
+        assert east_option["selector"] == "#region > option:nth-of-type(2)"
+        assert east_option["value"] == "east"
+        assert ref_to_selector(east_option) == "#region > option:nth-of-type(2)"
+
+    @pytest.mark.asyncio
+    async def test_dom_password_field_value_redacted_by_type_not_name(self) -> None:
+        # A password input whose accessible name is NOT password-like must still be redacted
+        # when the DOM scan flags it (is_password); its raw value must never surface.
+        page = _make_page(_make_a11y_tree(children=[]))
+        page.evaluate = AsyncMock(
+            return_value=[
+                {
+                    "role": "textbox",
+                    "name": "Current",
+                    "tag": "input",
+                    "value": "",
+                    "is_password": True,
+                    "selector": "#pw",
+                }
+            ]
+        )
+        result = await do_observe(page)
+        serialized = serialize_elements(result.elements)
+        pw = next(element for element in serialized if element["name"] == "Current")
+        assert pw["value"] == "***"
+
+    @pytest.mark.asyncio
+    async def test_duplicate_label_options_do_not_get_wrong_selector(self) -> None:
+        # Two a11y options share a label and two DOM options share it too. The merge must NOT
+        # attach a DOM selector to an a11y option (ambiguous) — that would risk a confidently
+        # wrong deterministic action.
+        tree = _make_a11y_tree(
+            children=[
+                {
+                    "role": "combobox",
+                    "name": "Region",
+                    "children": [
+                        {"role": "option", "name": "North"},
+                        {"role": "option", "name": "North"},
+                    ],
+                },
+            ]
+        )
+        page = _make_page(tree)
+        page.evaluate = AsyncMock(
+            return_value=[
+                {
+                    "role": "option",
+                    "name": "North",
+                    "tag": "option",
+                    "selector": "#r > option:nth-of-type(1)",
+                    "value": "n1",
+                },
+                {
+                    "role": "option",
+                    "name": "North",
+                    "tag": "option",
+                    "selector": "#r > option:nth-of-type(2)",
+                    "value": "n2",
+                },
+            ]
+        )
+        result = await do_observe(page)
+        serialized = serialize_elements(result.elements)
+        a11y_norths = [e for e in serialized if e["name"] == "North" and not e.get("selector")]
+        assert len(a11y_norths) == 2
+
+    @pytest.mark.asyncio
+    async def test_duplicate_name_password_redacts_all_a11y_values(self) -> None:
+        # A password field whose accessible name is shared with another textbox must have its
+        # a11y value redacted even when the DOM match is ambiguous (duplicate name).
+        tree = _make_a11y_tree(
+            children=[
+                {"role": "textbox", "name": "Current", "value": "s3cr3t"},
+                {"role": "textbox", "name": "Current", "value": ""},
+            ]
+        )
+        page = _make_page(tree)
+        page.evaluate = AsyncMock(
+            return_value=[
+                {
+                    "role": "textbox",
+                    "name": "Current",
+                    "tag": "input",
+                    "value": "",
+                    "is_password": True,
+                    "selector": "#pw",
+                },
+            ]
+        )
+        result = await do_observe(page, max_elements=100)
+        serialized = serialize_elements(result.elements)
+        currents = [element for element in serialized if element["name"] == "Current"]
+        assert currents, serialized
+        for element in currents:
+            assert element["value"] in ("***", "", None), element
+        assert all("s3cr3t" not in str(element.get("value") or "") for element in serialized)
+
+    @pytest.mark.asyncio
+    async def test_match_index_stable_when_cap_reorders(self) -> None:
+        # A selector-bearing duplicate reordered ahead of the cap must NOT shift the nth ordinal
+        # of a selectorless duplicate sharing its (role, name). The a11y duplicate must keep its
+        # original ordinal (0), so its `nth=N` fallback ref points at the right element.
+        tree = _make_a11y_tree(
+            children=[
+                {"role": "combobox", "name": "Dup", "value": ""},
+                {"role": "combobox", "name": "Dup", "value": ""},
+            ]
+        )
+        page = _make_page(tree)
+        page.evaluate = AsyncMock(
+            return_value=[
+                {"role": "combobox", "name": "Dup", "tag": "select", "selector": "#dup3"},
+            ]
+        )
+        result = await do_observe(page, max_elements=2)
+        selectorless_dups = [e for e in result.elements if e.name == "Dup" and not e.selector]
+        assert selectorless_dups, result.elements
+        assert selectorless_dups[0].match_index == 0
+
+    @pytest.mark.asyncio
+    async def test_divergent_name_dom_password_fails_closed(self) -> None:
+        # If a DOM password field's accessible name diverges from the a11y name (so it cannot be
+        # mapped), fail closed: redact every a11y textbox value rather than leak the a11y value.
+        tree = _make_a11y_tree(
+            children=[
+                {"role": "textbox", "name": "Account", "value": "s3cr3t"},
+            ]
+        )
+        page = _make_page(tree)
+        page.evaluate = AsyncMock(
+            return_value=[
+                {
+                    "role": "textbox",
+                    "name": "DifferentName",
+                    "tag": "input",
+                    "value": "",
+                    "is_password": True,
+                    "selector": "#pw",
+                },
+            ]
+        )
+        result = await do_observe(page, max_elements=100)
+        serialized = serialize_elements(result.elements)
+        account = next(element for element in serialized if element["name"] == "Account")
+        assert account["value"] == "***"
+        assert all("s3cr3t" not in str(element.get("value") or "") for element in serialized)
+
     @pytest.mark.asyncio
     async def test_role_to_tag_mapping(self) -> None:
         page = _make_page()
@@ -615,6 +839,60 @@ class TestSkyvernExecuteMCP:
         click_call = mcp_browser.skyvern_click.call_args
         assert 'role=button[name="Sign In"]' in str(click_call)
 
+    @pytest.mark.asyncio
+    async def test_execute_observe_then_click_native_option_ref_selects_parent(
+        self,
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> None:
+        option_selector = "#region > option:nth-of-type(4)"
+        option_locator = SimpleNamespace(
+            evaluate=AsyncMock(
+                return_value={
+                    "select_selector": "#region",
+                    "value": "east",
+                    "label": "East",
+                }
+            )
+        )
+        option_locator.first = option_locator
+        select_option = AsyncMock()
+        select_locator = SimpleNamespace(select_option=select_option)
+        page = SimpleNamespace(
+            url="https://example.com/form",
+            title=AsyncMock(return_value="Form"),
+            accessibility=None,
+            evaluate=AsyncMock(
+                return_value=[
+                    {
+                        "role": "option",
+                        "name": "East",
+                        "tag": "option",
+                        "selector": option_selector,
+                        "value": "east",
+                    }
+                ]
+            ),
+            locator=MagicMock(
+                side_effect=lambda selector: option_locator if selector == option_selector else select_locator
+            ),
+            click=AsyncMock(return_value="#unexpected-click"),
+        )
+        ctx = BrowserContext(mode="local")
+        monkeypatch.setattr(mcp_browser, "get_page", AsyncMock(return_value=(page, ctx)))
+
+        result = await mcp_browser.skyvern_execute(
+            steps=[
+                {"tool": "observe", "params": {}},
+                {"tool": "click", "params": {"ref": "e0"}},
+            ]
+        )
+
+        assert result["ok"] is True
+        assert result["data"]["steps_completed"] == 2
+        page.click.assert_not_awaited()
+        select_option.assert_awaited_once_with(value="east", timeout=30000)
+        assert result["data"]["results"][1]["data"]["resolved_selector"] == "#region"
+
     @pytest.mark.asyncio
     async def test_execute_unknown_ref_fails(self, monkeypatch: pytest.MonkeyPatch) -> None:
         page = _make_page()