From 778f3828c4cbf84fc00de8976259e5a3171173c0 Mon Sep 17 00:00:00 2001 From: Andrew Neilson Date: Mon, 22 Jun 2026 14:56:10 -0700 Subject: [PATCH] fix(SKY-11251): repair main-red code-repair-progress seam test after import-strip merge skew (#6749) Co-authored-by: Claude Opus 4.8 --- skyvern/cli/mcp_tools/workflow.py | 26 +- skyvern/forge/agent_functions.py | 348 +++++++++++++---- .../test_svg_convert_cache_resilience.py | 363 ++++++++++++++++++ tests/unit/test_mcp_workflow_tools.py | 193 ++++++++++ 4 files changed, 846 insertions(+), 84 deletions(-) create mode 100644 tests/unit/forge/test_svg_convert_cache_resilience.py diff --git a/skyvern/cli/mcp_tools/workflow.py b/skyvern/cli/mcp_tools/workflow.py index 0436c2c4f..baa23ae48 100644 --- a/skyvern/cli/mcp_tools/workflow.py +++ b/skyvern/cli/mcp_tools/workflow.py @@ -441,6 +441,7 @@ _CODE_V2_DEFAULTS: dict[str, Any] = { "run_with": "agent", } _DEFAULT_MCP_PROXY_LOCATION = ProxyLocation.RESIDENTIAL +_WORKFLOW_UPDATE_PRESERVED_TOP_LEVEL_FIELDS = ("run_sequentially", "sequential_key") def _deep_merge(base: Any, override: Any) -> Any: @@ -609,6 +610,28 @@ async def _inject_workflow_update_proxy_default(definition: str, fmt: str, workf return _dump_definition_dict(raw, parsed_format) +async def _inject_workflow_update_top_level_settings(definition: str, fmt: str, workflow_id: str) -> str: + """Preserve workflow-level settings that schema defaults would otherwise clobber.""" + + raw, parsed_format = _load_definition_dict(definition, fmt) + if raw is None or parsed_format is None: + return definition + + missing_fields = [field for field in _WORKFLOW_UPDATE_PRESERVED_TOP_LEVEL_FIELDS if field not in raw] + if not missing_fields: + return definition + + existing_workflow = await get_workflow_by_id(workflow_id) + changed = False + for field in missing_fields: + existing_value = existing_workflow.get(field) + if existing_value is not None: + raw[field] = existing_value + changed = True + + return _dump_definition_dict(raw, parsed_format) if changed else definition + + # Parameter types that are auto-managed (credentials and secrets set via the UI) and should # always be preserved from the existing workflow during MCP updates, regardless of what the # caller sends. These should NEVER be modifiable via MCP — only via the UI credential picker. @@ -1082,6 +1105,7 @@ async def skyvern_workflow_update( try: definition = await _inject_workflow_update_proxy_default(definition, format, workflow_id) + definition = await _inject_workflow_update_top_level_settings(definition, format, workflow_id) definition = await _inject_workflow_update_parameters(definition, format, workflow_id) except NotFoundError: return make_result( @@ -1094,7 +1118,7 @@ async def skyvern_workflow_update( ), ) except Exception as e: - LOG.warning("workflow_update_proxy_default_injection_failed", workflow_id=workflow_id, error=str(e)) + LOG.warning("workflow_update_preprocessing_failed", workflow_id=workflow_id, error=str(e)) return make_result( "skyvern_workflow_update", ok=False, diff --git a/skyvern/forge/agent_functions.py b/skyvern/forge/agent_functions.py index ce1a1b029..5446db905 100644 --- a/skyvern/forge/agent_functions.py +++ b/skyvern/forge/agent_functions.py @@ -2,6 +2,9 @@ import asyncio import copy import hashlib import os +import time +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager from dataclasses import dataclass, field from datetime import timedelta from typing import TYPE_CHECKING, Any, Dict, List @@ -9,6 +12,7 @@ from typing import TYPE_CHECKING, Any, Dict, List import aiohttp import httpx import structlog +from cachetools import TTLCache from playwright.async_api import Frame, Page from skyvern.config import settings @@ -26,6 +30,7 @@ from skyvern.forge.prompts import prompt_engine from skyvern.forge.sdk.api.aws import AsyncAWSClient from skyvern.forge.sdk.api.azure import AzureClientFactory from skyvern.forge.sdk.api.llm.exceptions import LLMProviderError +from skyvern.forge.sdk.cache.base import CACHE_EXPIRE_TIME from skyvern.forge.sdk.copilot.config import CopilotConfig, block_authoring_policy_from_code_only_mode from skyvern.forge.sdk.core import skyvern_context from skyvern.forge.sdk.db.agent_db import AgentDB @@ -53,6 +58,23 @@ USELESS_SHAPE_ATTRIBUTE = [SKYVERN_ID_ATTR, "id", "aria-describedby"] SVG_SHAPE_CONVERTION_ATTEMPTS = 3 CSS_SHAPE_CONVERTION_ATTEMPTS = 1 INVALID_SHAPE = "N/A" +DISABLE_SVG_CONVERT_CACHE_RESILIENCE_FLAG = "DISABLE_SVG_CONVERT_CACHE_RESILIENCE" +SVG_LOCAL_CACHE_MAX_ITEMS = 4096 +SVG_LOCAL_NEGATIVE_CACHE_EXPIRE_TIME = timedelta(hours=1) +SVGLocalCacheValue = tuple[str, float | None] + +# TTLCache has one global TTL, so each value also carries an optional shorter +# expiry timestamp for negative cache entries. +_SVG_LOCAL_SHAPE_CACHE: TTLCache[str, SVGLocalCacheValue] = TTLCache( + maxsize=SVG_LOCAL_CACHE_MAX_ITEMS, + ttl=CACHE_EXPIRE_TIME.total_seconds(), +) +# Best-effort single-flight cache: eviction under extreme distinct-key pressure can +# allow duplicate conversions, but does not affect conversion correctness. +_SVG_CONVERSION_LOCKS: TTLCache[str, asyncio.Lock] = TTLCache( + maxsize=SVG_LOCAL_CACHE_MAX_ITEMS, + ttl=CACHE_EXPIRE_TIME.total_seconds(), +) @dataclass @@ -115,6 +137,158 @@ def _get_shape_cache_key(hash: str) -> str: return f"skyvern:shape:{hash}" +def _get_svg_conversion_lock(svg_key: str) -> asyncio.Lock: + lock = _SVG_CONVERSION_LOCKS.get(svg_key) + if lock is None: + lock = asyncio.Lock() + _SVG_CONVERSION_LOCKS[svg_key] = lock + return lock + + +@asynccontextmanager +async def _svg_conversion_lock_scope(svg_key: str, *, use_lock: bool) -> AsyncIterator[None]: + if not use_lock: + yield + return + + async with _get_svg_conversion_lock(svg_key): + yield + + +async def _is_svg_convert_cache_resilience_disabled() -> bool: + context = skyvern_context.current() + if context is None: + return False + + distinct_id = context.run_id or context.workflow_run_id or context.task_id + organization_id = context.organization_id + if not distinct_id or not organization_id: + return False + + experimentation_provider = getattr(app, "EXPERIMENTATION_PROVIDER", None) + if not experimentation_provider: + return False + + try: + flag_enabled = await experimentation_provider.is_feature_enabled_cached( + DISABLE_SVG_CONVERT_CACHE_RESILIENCE_FLAG, + distinct_id, + properties={"organization_id": organization_id}, + ) + except Exception: + LOG.warning( + "Failed to evaluate SVG convert cache resilience flag; defaulting to enabled", + exc_info=True, + distinct_id=distinct_id, + organization_id=organization_id, + ) + return False + + return bool(flag_enabled) + + +def _get_local_cache_ttl_seconds(svg_shape: str, ex: int | timedelta | None) -> float | None: + if ex is None: + ttl_seconds = None + else: + ttl_seconds = ex.total_seconds() if isinstance(ex, timedelta) else float(ex) + + if svg_shape == INVALID_SHAPE: + negative_ttl_seconds = SVG_LOCAL_NEGATIVE_CACHE_EXPIRE_TIME.total_seconds() + if ttl_seconds is None: + return negative_ttl_seconds + return min(ttl_seconds, negative_ttl_seconds) + return ttl_seconds + + +def _get_local_cache_expires_at(svg_shape: str, ex: int | timedelta | None) -> float | None: + ttl_seconds = _get_local_cache_ttl_seconds(svg_shape, ex) + if ttl_seconds is None: + return None + return time.monotonic() + max(ttl_seconds, 0.0) + + +def _get_svg_shape_from_local_cache(svg_key: str) -> str | None: + cached_value = _SVG_LOCAL_SHAPE_CACHE.get(svg_key) + if cached_value is None: + return None + + svg_shape, expires_at = cached_value + if expires_at is not None and expires_at <= time.monotonic(): + _SVG_LOCAL_SHAPE_CACHE.pop(svg_key, None) + return None + return svg_shape + + +def _cache_svg_shape_locally( + svg_key: str, + svg_shape: str | None, + *, + ex: int | timedelta | None = CACHE_EXPIRE_TIME, +) -> None: + if svg_shape: + _SVG_LOCAL_SHAPE_CACHE[svg_key] = (svg_shape, _get_local_cache_expires_at(svg_shape, ex)) + + +async def _get_cached_svg_shape(svg_key: str, *, use_local_cache: bool = True) -> str | None: + if use_local_cache: + local_shape = _get_svg_shape_from_local_cache(svg_key) + if local_shape: + return local_shape + + try: + cached_svg_shape = await app.CACHE.get(svg_key) + except Exception: + LOG.warning( + "Failed to loaded SVG cache", + exc_info=True, + key=svg_key, + ) + if use_local_cache: + return _get_svg_shape_from_local_cache(svg_key) + return None + + svg_shape = cached_svg_shape if isinstance(cached_svg_shape, str) else None + if use_local_cache: + _cache_svg_shape_locally(svg_key, svg_shape) + return svg_shape + + +async def _set_svg_cache( + svg_key: str, + svg_shape: str, + *, + ex: int | timedelta | None = CACHE_EXPIRE_TIME, + cache_locally: bool = True, +) -> None: + if cache_locally: + _cache_svg_shape_locally(svg_key, svg_shape, ex=ex) + try: + await app.CACHE.set(svg_key, svg_shape, ex=ex) + except Exception: + LOG.warning( + "Failed to store SVG cache", + exc_info=True, + key=svg_key, + ) + + +async def _set_css_shape_cache( + shape_key: str, + css_shape: str, + *, + ex: int | timedelta | None = CACHE_EXPIRE_TIME, +) -> None: + try: + await app.CACHE.set(shape_key, css_shape, ex=ex) + except Exception: + LOG.warning( + "Failed to store CSS shape cache", + exc_info=True, + key=shape_key, + ) + + def _remove_skyvern_attributes(element: Dict) -> Dict: """ To get the original HTML element without skyvern attributes @@ -223,99 +397,107 @@ async def _convert_svg_to_string( svg_key = _get_svg_cache_key(svg_hash) svg_shape: str | None = None - try: - svg_shape = await app.CACHE.get(svg_key) - except Exception: - LOG.warning( - "Failed to loaded SVG cache", - exc_info=True, - key=svg_key, - ) + refresh_svg_cache = False + use_cache_resilience = not await _is_svg_convert_cache_resilience_disabled() + async with _svg_conversion_lock_scope(svg_key, use_lock=use_cache_resilience): + svg_shape = await _get_cached_svg_shape(svg_key, use_local_cache=use_cache_resilience) - if svg_shape: - LOG.debug("SVG loaded from cache", element_id=element_id, key=svg_key, shape=svg_shape) - else: - if _is_element_already_dropped(svg_key): - LOG.debug("SVG is already dropped, going to abort conversion", element_id=element_id, key=svg_key) - _mark_element_as_dropped(element, hashed_key=svg_key) - return + if svg_shape: + LOG.debug("SVG loaded from cache", element_id=element_id, key=svg_key, shape=svg_shape) + refresh_svg_cache = True + else: + if _is_element_already_dropped(svg_key): + LOG.debug("SVG is already dropped, going to abort conversion", element_id=element_id, key=svg_key) + _mark_element_as_dropped(element, hashed_key=svg_key) + return - if len(svg_html) > settings.SVG_MAX_LENGTH: - # TODO: implement a fallback solution for "too large" case, maybe convert by screenshot - LOG.warning( - "SVG element is too large to convert, going to drop the svg element.", - element_id=element_id, - length=len(svg_html), - key=svg_key, - ) - _mark_element_as_dropped(element, hashed_key=svg_key) - return - - LOG.debug("call LLM to convert SVG to string shape", element_id=element_id) - svg_convert_prompt = prompt_engine.load_prompt("svg-convert", svg_element=svg_html) - - for retry in range(SVG_SHAPE_CONVERTION_ATTEMPTS): - try: - async with asyncio.timeout(_LLM_CALL_TIMEOUT_SECONDS): - if app.SVG_CSS_CONVERTER_LLM_API_HANDLER is None: - raise Exception("To enable svg shape conversion, please set the Secondary LLM key") - json_response = await app.SVG_CSS_CONVERTER_LLM_API_HANDLER( - prompt=svg_convert_prompt, step=step, prompt_name="svg-convert" - ) - svg_shape = json_response.get("shape", "") - recognized = json_response.get("recognized", False) - if not svg_shape or not recognized: - raise Exception("Empty or unrecognized SVG shape replied by secondary llm") - LOG.info("SVG converted by LLM", element_id=element_id, key=svg_key, shape=svg_shape) - await app.CACHE.set(svg_key, svg_shape) - break - except LLMProviderError: - LOG.info( - "Failed to convert SVG to string due to llm error. Will retry if haven't met the max try attempt.", - exc_info=True, - element_id=element_id, - key=svg_key, - retry=retry, - ) - if retry == SVG_SHAPE_CONVERTION_ATTEMPTS - 1: - # set the invalid css shape to cache to avoid retry in the near future - await app.CACHE.set(svg_key, INVALID_SHAPE, ex=timedelta(hours=1)) - else: - await asyncio.sleep(0.5 * (2**retry)) - except asyncio.TimeoutError: + if len(svg_html) > settings.SVG_MAX_LENGTH: + # TODO: implement a fallback solution for "too large" case, maybe convert by screenshot LOG.warning( - "Timeout to call LLM to parse SVG. Going to drop the svg element directly.", + "SVG element is too large to convert, going to drop the svg element.", element_id=element_id, + length=len(svg_html), key=svg_key, ) _mark_element_as_dropped(element, hashed_key=svg_key) return - except Exception: - LOG.info( - "Failed to convert SVG to string shape by secondary llm. Will retry if haven't met the max try attempt.", - exc_info=True, + + LOG.debug("call LLM to convert SVG to string shape", element_id=element_id) + svg_convert_prompt = prompt_engine.load_prompt("svg-convert", svg_element=svg_html) + + for retry in range(SVG_SHAPE_CONVERTION_ATTEMPTS): + try: + async with asyncio.timeout(_LLM_CALL_TIMEOUT_SECONDS): + if app.SVG_CSS_CONVERTER_LLM_API_HANDLER is None: + raise Exception("To enable svg shape conversion, please set the Secondary LLM key") + json_response = await app.SVG_CSS_CONVERTER_LLM_API_HANDLER( + prompt=svg_convert_prompt, step=step, prompt_name="svg-convert" + ) + svg_shape = json_response.get("shape", "") + recognized = json_response.get("recognized", False) + if not svg_shape or not recognized: + raise Exception("Empty or unrecognized SVG shape replied by secondary llm") + LOG.info("SVG converted by LLM", element_id=element_id, key=svg_key, shape=svg_shape) + await _set_svg_cache(svg_key, svg_shape, cache_locally=use_cache_resilience) + break + except LLMProviderError: + LOG.info( + "Failed to convert SVG to string due to llm error. Will retry if haven't met the max try attempt.", + exc_info=True, + element_id=element_id, + key=svg_key, + retry=retry, + ) + if retry == SVG_SHAPE_CONVERTION_ATTEMPTS - 1: + # set the invalid css shape to cache to avoid retry in the near future + await _set_svg_cache( + svg_key, + INVALID_SHAPE, + ex=timedelta(hours=1), + cache_locally=use_cache_resilience, + ) + else: + await asyncio.sleep(0.5 * (2**retry)) + except asyncio.TimeoutError: + LOG.warning( + "Timeout to call LLM to parse SVG. Going to drop the svg element directly.", + element_id=element_id, + key=svg_key, + ) + _mark_element_as_dropped(element, hashed_key=svg_key) + return + except Exception: + LOG.info( + "Failed to convert SVG to string shape by secondary llm. Will retry if haven't met the max try attempt.", + exc_info=True, + element_id=element_id, + retry=retry, + ) + if retry == SVG_SHAPE_CONVERTION_ATTEMPTS - 1: + # set the invalid css shape to cache to avoid retry in the near future + await _set_svg_cache( + svg_key, + INVALID_SHAPE, + ex=timedelta(weeks=1), + cache_locally=use_cache_resilience, + ) + else: + await asyncio.sleep(0.5 * (2**retry)) + else: + LOG.warning( + "Reaching the max try to convert svg element, going to drop the svg element.", element_id=element_id, - retry=retry, + key=svg_key, + length=len(svg_html), ) - if retry == SVG_SHAPE_CONVERTION_ATTEMPTS - 1: - # set the invalid css shape to cache to avoid retry in the near future - await app.CACHE.set(svg_key, INVALID_SHAPE, ex=timedelta(weeks=1)) - else: - await asyncio.sleep(0.5 * (2**retry)) - else: - LOG.warning( - "Reaching the max try to convert svg element, going to drop the svg element.", - element_id=element_id, - key=svg_key, - length=len(svg_html), - ) - _mark_element_as_dropped(element, hashed_key=svg_key) - return + _mark_element_as_dropped(element, hashed_key=svg_key) + return element["attributes"] = dict() if svg_shape != INVALID_SHAPE: # refresh the cache expiration - await app.CACHE.set(svg_key, svg_shape) + if refresh_svg_cache: + await _set_svg_cache(svg_key, svg_shape, cache_locally=use_cache_resilience) element["attributes"]["alt"] = svg_shape if "children" in element: del element["children"] @@ -404,7 +586,7 @@ async def _convert_css_shape_to_string( if not css_shape or not recognized: raise Exception("Empty or unrecognized css shape replied by secondary llm") LOG.info("CSS Shape converted by LLM", element_id=element_id, key=shape_key, shape=css_shape) - await app.CACHE.set(shape_key, css_shape) + await _set_css_shape_cache(shape_key, css_shape) break except LLMProviderError: LOG.info( @@ -416,7 +598,7 @@ async def _convert_css_shape_to_string( ) if retry == CSS_SHAPE_CONVERTION_ATTEMPTS - 1: # set the invalid css shape to cache to avoid retry in the near future - await app.CACHE.set(shape_key, INVALID_SHAPE, ex=timedelta(hours=1)) + await _set_css_shape_cache(shape_key, INVALID_SHAPE, ex=timedelta(hours=1)) except asyncio.TimeoutError: LOG.warning( "Timeout to call LLM to parse css shape. Going to abort the convertion directly.", @@ -435,7 +617,7 @@ async def _convert_css_shape_to_string( ) if retry == CSS_SHAPE_CONVERTION_ATTEMPTS - 1: # set the invalid css shape to cache to avoid retry in the near future - await app.CACHE.set(shape_key, INVALID_SHAPE, ex=timedelta(weeks=1)) + await _set_css_shape_cache(shape_key, INVALID_SHAPE, ex=timedelta(weeks=1)) else: LOG.info( "Max css shape convertion retry, going to abort the convertion.", @@ -458,7 +640,7 @@ async def _convert_css_shape_to_string( element["attributes"] = dict() if css_shape != INVALID_SHAPE: # refresh the cache expiration - await app.CACHE.set(shape_key, css_shape) + await _set_css_shape_cache(shape_key, css_shape) element["attributes"]["shape-description"] = css_shape return None diff --git a/tests/unit/forge/test_svg_convert_cache_resilience.py b/tests/unit/forge/test_svg_convert_cache_resilience.py new file mode 100644 index 000000000..4c83a3317 --- /dev/null +++ b/tests/unit/forge/test_svg_convert_cache_resilience.py @@ -0,0 +1,363 @@ +import asyncio +import copy +import hashlib +from collections.abc import Generator +from datetime import timedelta +from types import SimpleNamespace +from typing import Any +from unittest.mock import AsyncMock + +import pytest + +from skyvern.forge import agent_functions +from skyvern.forge.sdk.cache.base import CACHE_EXPIRE_TIME +from skyvern.forge.sdk.core import skyvern_context + + +def _svg_element() -> dict[str, Any]: + return { + "tagName": "svg", + "id": "AAAK", + "attributes": {"id": "AAAK"}, + "children": [{"tagName": "path", "attributes": {"d": "M0 0h10v10z"}}], + } + + +def _svg_cache_key(element: dict[str, Any]) -> str: + svg_element = agent_functions._remove_skyvern_attributes(element) + svg_html = agent_functions.json_to_html(svg_element) + return agent_functions._get_svg_cache_key(hashlib.sha256(svg_html.encode("utf-8")).hexdigest()) + + +class _FailingCache: + async def get(self, key: str) -> Any: + raise ConnectionError("redis unavailable") + + async def set(self, key: str, value: Any, ex: Any = CACHE_EXPIRE_TIME) -> None: + raise ConnectionError("redis unavailable") + + +class _RecordingFailingCache(_FailingCache): + def __init__(self) -> None: + self.set_calls: list[tuple[str, Any, Any]] = [] + + async def set(self, key: str, value: Any, ex: Any = CACHE_EXPIRE_TIME) -> None: + self.set_calls.append((key, value, ex)) + await super().set(key, value, ex=ex) + + +class _MemoryCache: + def __init__(self) -> None: + self.values: dict[str, Any] = {} + self.fail_get = False + + async def get(self, key: str) -> Any: + if self.fail_get: + raise ConnectionError("redis get unavailable") + return self.values.get(key) + + async def set(self, key: str, value: Any, ex: Any = CACHE_EXPIRE_TIME) -> None: + self.values[key] = value + + +class _SetFailingMemoryCache: + async def get(self, key: str) -> Any: + return None + + async def set(self, key: str, value: Any, ex: Any = CACHE_EXPIRE_TIME) -> None: + raise ConnectionError("redis set unavailable") + + +class _FakeLocator: + async def count(self) -> int: + return 1 + + async def element_handle(self, timeout: float) -> object: + return object() + + async def scroll_into_view_if_needed(self, timeout: float) -> None: + return None + + async def wait_for(self, state: str, timeout: float) -> None: + return None + + async def screenshot(self, timeout: float, animations: str) -> bytes: + return b"fake-png" + + +class _FakeFrame: + def locator(self, selector: str) -> _FakeLocator: + return _FakeLocator() + + +class _FakeSkyvernFrame: + def get_frame(self) -> _FakeFrame: + return _FakeFrame() + + async def get_blocking_element_id(self, element: object) -> tuple[None, bool]: + return None, False + + +def test_svg_local_invalid_shape_cache_uses_short_ttl(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(agent_functions.time, "monotonic", lambda: 100.0) + + agent_functions._cache_svg_shape_locally( + "svg-key", + agent_functions.INVALID_SHAPE, + ex=timedelta(weeks=1), + ) + + assert agent_functions._SVG_LOCAL_SHAPE_CACHE["svg-key"] == ( + agent_functions.INVALID_SHAPE, + 100.0 + agent_functions.SVG_LOCAL_NEGATIVE_CACHE_EXPIRE_TIME.total_seconds(), + ) + + +@pytest.fixture(autouse=True) +def _reset_svg_convert_state(monkeypatch: pytest.MonkeyPatch) -> Generator[None, None, None]: + agent_functions._SVG_LOCAL_SHAPE_CACHE.clear() + agent_functions._SVG_CONVERSION_LOCKS.clear() + skyvern_context.set(skyvern_context.SkyvernContext()) + monkeypatch.setattr(agent_functions.prompt_engine, "load_prompt", lambda *args, **kwargs: "svg prompt") + yield + skyvern_context.reset() + agent_functions._SVG_LOCAL_SHAPE_CACHE.clear() + agent_functions._SVG_CONVERSION_LOCKS.clear() + + +@pytest.mark.asyncio +async def test_svg_convert_does_not_retry_llm_when_cache_set_fails(monkeypatch: pytest.MonkeyPatch) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "search icon", "recognized": True} + + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace(CACHE=_FailingCache(), SVG_CSS_CONVERTER_LLM_API_HANDLER=handler), + ) + + element = _svg_element() + await agent_functions._convert_svg_to_string(element) + + assert calls == 1 + assert element["attributes"] == {"alt": "search icon"} + assert "children" not in element + + +@pytest.mark.asyncio +async def test_svg_convert_uses_local_fallback_when_redis_is_unavailable(monkeypatch: pytest.MonkeyPatch) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "search icon", "recognized": True} + + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace(CACHE=_FailingCache(), SVG_CSS_CONVERTER_LLM_API_HANDLER=handler), + ) + + first = _svg_element() + second = _svg_element() + await agent_functions._convert_svg_to_string(first) + await agent_functions._convert_svg_to_string(second) + + assert calls == 1 + assert first["attributes"] == {"alt": "search icon"} + assert second["attributes"] == {"alt": "search icon"} + + +@pytest.mark.asyncio +async def test_svg_convert_caches_invalid_shape_locally_when_redis_is_unavailable( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "", "recognized": False} + + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace(CACHE=_FailingCache(), SVG_CSS_CONVERTER_LLM_API_HANDLER=handler), + ) + + first = _svg_element() + await agent_functions._convert_svg_to_string(first) + + assert calls == 3 + assert first["isDropped"] is True + assert "children" not in first + + skyvern_context.set(skyvern_context.SkyvernContext()) + second = _svg_element() + await agent_functions._convert_svg_to_string(second) + + assert calls == 3 + assert second["attributes"] == {} + assert "children" not in second + + +@pytest.mark.asyncio +async def test_svg_convert_caches_invalid_shape_loaded_from_redis_locally( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "search icon", "recognized": True} + + cache = _MemoryCache() + cache.values[_svg_cache_key(_svg_element())] = agent_functions.INVALID_SHAPE + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace(CACHE=cache, SVG_CSS_CONVERTER_LLM_API_HANDLER=handler), + ) + + first = _svg_element() + await agent_functions._convert_svg_to_string(first) + + assert calls == 0 + assert first["attributes"] == {} + assert "children" not in first + + cache.fail_get = True + skyvern_context.set(skyvern_context.SkyvernContext()) + second = _svg_element() + await agent_functions._convert_svg_to_string(second) + + assert calls == 0 + assert second["attributes"] == {} + assert "children" not in second + + +@pytest.mark.asyncio +async def test_svg_convert_single_flights_concurrent_duplicate_misses( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "search icon", "recognized": True} + + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace(CACHE=_MemoryCache(), SVG_CSS_CONVERTER_LLM_API_HANDLER=handler), + ) + + elements = [copy.deepcopy(_svg_element()) for _ in range(5)] + await asyncio.gather(*[agent_functions._convert_svg_to_string(element) for element in elements]) + + assert calls == 1 + assert all(element["attributes"] == {"alt": "search icon"} for element in elements) + + +@pytest.mark.asyncio +async def test_svg_convert_disable_flag_bypasses_local_cache_and_single_flight( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + await asyncio.sleep(0) + return {"shape": "search icon", "recognized": True} + + provider = SimpleNamespace(is_feature_enabled_cached=AsyncMock(return_value=True)) + cache = _RecordingFailingCache() + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace( + CACHE=cache, + SVG_CSS_CONVERTER_LLM_API_HANDLER=handler, + EXPERIMENTATION_PROVIDER=provider, + ), + ) + skyvern_context.set(skyvern_context.SkyvernContext(run_id="wr_1", organization_id="o_1")) + + elements = [copy.deepcopy(_svg_element()) for _ in range(5)] + await asyncio.gather(*[agent_functions._convert_svg_to_string(element) for element in elements]) + + assert calls == len(elements) + assert len(cache.set_calls) == len(elements) + assert all(element["attributes"] == {"alt": "search icon"} for element in elements) + provider.is_feature_enabled_cached.assert_any_await( + agent_functions.DISABLE_SVG_CONVERT_CACHE_RESILIENCE_FLAG, + "wr_1", + properties={"organization_id": "o_1"}, + ) + + +@pytest.mark.asyncio +async def test_svg_convert_flag_error_defaults_to_cache_resilience( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "search icon", "recognized": True} + + provider = SimpleNamespace(is_feature_enabled_cached=AsyncMock(side_effect=RuntimeError("posthog down"))) + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace( + CACHE=_FailingCache(), + SVG_CSS_CONVERTER_LLM_API_HANDLER=handler, + EXPERIMENTATION_PROVIDER=provider, + ), + ) + skyvern_context.set(skyvern_context.SkyvernContext(run_id="wr_1", organization_id="o_1")) + + first = _svg_element() + second = _svg_element() + await agent_functions._convert_svg_to_string(first) + await agent_functions._convert_svg_to_string(second) + + assert calls == 1 + assert first["attributes"] == {"alt": "search icon"} + assert second["attributes"] == {"alt": "search icon"} + + +@pytest.mark.asyncio +async def test_css_shape_convert_keeps_result_when_cache_set_fails(monkeypatch: pytest.MonkeyPatch) -> None: + calls = 0 + + async def handler(**kwargs: Any) -> dict[str, Any]: + nonlocal calls + calls += 1 + return {"shape": "calendar icon", "recognized": True} + + monkeypatch.setattr( + agent_functions, + "app", + SimpleNamespace(CACHE=_SetFailingMemoryCache(), SVG_CSS_CONVERTER_LLM_API_HANDLER=handler), + ) + + element: dict[str, Any] = { + "tagName": "span", + "id": "AAAK", + "attributes": {"id": "AAAK"}, + } + await agent_functions._convert_css_shape_to_string(_FakeSkyvernFrame(), element) + + assert calls == 1 + assert element["attributes"]["shape-description"] == "calendar icon" diff --git a/tests/unit/test_mcp_workflow_tools.py b/tests/unit/test_mcp_workflow_tools.py index 35b4ba7c6..52dbad586 100644 --- a/tests/unit/test_mcp_workflow_tools.py +++ b/tests/unit/test_mcp_workflow_tools.py @@ -507,6 +507,199 @@ async def test_workflow_update_defaults_proxy_when_existing_is_null(monkeypatch: assert sent_definition.get("proxy_location") == "RESIDENTIAL" +@pytest.mark.asyncio +async def test_workflow_update_preserves_sequential_settings_when_omitted( + monkeypatch: pytest.MonkeyPatch, +) -> None: + request_mock = _patch_skyvern_http(monkeypatch, response_payload=_fake_workflow_dict()) + + async def fake_get_workflow_by_id(workflow_id: str, version: int | None = None) -> dict[str, object]: + assert workflow_id == "wpid_test" + assert version is None + return { + "proxy_location": "RESIDENTIAL", + "run_sequentially": True, + "sequential_key": "existing-sequential-key", + "workflow_definition": { + "parameters": [], + "blocks": [], + }, + } + + _patch_get_workflow_by_id(monkeypatch, fake_get_workflow_by_id) + + definition = { + "title": "Updated workflow", + "workflow_definition": { + "parameters": [], + "blocks": [ + { + "block_type": "navigation", + "label": "visit", + "url": "https://example.com", + "title": "Visit", + "navigation_goal": "Open the page", + } + ], + }, + } + + result = await workflow_tools.skyvern_workflow_update( + workflow_id="wpid_test", + definition=json.dumps(definition), + format="json", + ) + + sent_definition = request_mock.await_args.kwargs["json"]["json_definition"] + assert result["ok"] is True + assert sent_definition.get("run_sequentially") is True + assert sent_definition.get("sequential_key") == "existing-sequential-key" + + +@pytest.mark.asyncio +async def test_workflow_update_respects_explicit_run_sequentially_false( + monkeypatch: pytest.MonkeyPatch, +) -> None: + request_mock = _patch_skyvern_http(monkeypatch, response_payload=_fake_workflow_dict()) + + async def fake_get_workflow_by_id(workflow_id: str, version: int | None = None) -> dict[str, object]: + assert workflow_id == "wpid_test" + assert version is None + return { + "proxy_location": "RESIDENTIAL", + "run_sequentially": True, + "sequential_key": "existing-sequential-key", + "workflow_definition": { + "parameters": [], + "blocks": [], + }, + } + + _patch_get_workflow_by_id(monkeypatch, fake_get_workflow_by_id) + + definition = { + "title": "Updated workflow", + "run_sequentially": False, + "workflow_definition": { + "parameters": [], + "blocks": [ + { + "block_type": "navigation", + "label": "visit", + "url": "https://example.com", + "title": "Visit", + "navigation_goal": "Open the page", + } + ], + }, + } + + result = await workflow_tools.skyvern_workflow_update( + workflow_id="wpid_test", + definition=json.dumps(definition), + format="json", + ) + + sent_definition = request_mock.await_args.kwargs["json"]["json_definition"] + assert result["ok"] is True + assert sent_definition.get("run_sequentially") is False + assert sent_definition.get("sequential_key") == "existing-sequential-key" + + +@pytest.mark.asyncio +async def test_workflow_update_preserves_overlap_and_credentials_via_mcp_client( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Exercise workflow update through the registered FastMCP tool boundary.""" + + request_mock = _patch_skyvern_http(monkeypatch, response_payload=_fake_workflow_dict()) + + async def fake_get_workflow_by_id(workflow_id: str, version: int | None = None) -> dict[str, object]: + assert workflow_id == "wpid_test" + assert version is None + return { + "proxy_location": "RESIDENTIAL", + "run_sequentially": True, + "sequential_key": "existing-sequential-key", + "workflow_definition": { + "parameters": [ + { + "parameter_type": "credential", + "key": "credentials", + "credential_id": "cred_abc123", + "credential_parameter_id": "cp_xyz", + "workflow_id": "wf_test", + }, + { + "parameter_type": "workflow", + "key": "url_input", + "workflow_parameter_type": "string", + "default_value": "https://example.com", + }, + ], + "blocks": [ + { + "block_type": "login", + "label": "login_block", + "parameter_keys": ["credentials"], + } + ], + }, + } + + _patch_get_workflow_by_id(monkeypatch, fake_get_workflow_by_id) + + definition = { + "title": "Updated workflow", + "workflow_definition": { + "parameters": [ + { + "parameter_type": "workflow", + "key": "url_input", + "workflow_parameter_type": "string", + "default_value": "https://new-url.com", + }, + ], + "blocks": [ + { + "block_type": "login", + "label": "login_block", + "parameter_keys": [], + "navigation_goal": "Login to the site", + } + ], + }, + } + + async with Client(mcp) as client: + result = await client.call_tool( + "skyvern_workflow_update", + { + "workflow_id": "wpid_test", + "definition": json.dumps(definition), + "format": "json", + }, + ) + + assert result.is_error is False + assert isinstance(result.data, dict) + assert result.data["ok"] is True + + sent_definition = request_mock.await_args.kwargs["json"]["json_definition"] + assert sent_definition.get("run_sequentially") is True + assert sent_definition.get("sequential_key") == "existing-sequential-key" + + params = sent_definition["workflow_definition"]["parameters"] + cred_params = [p for p in params if p.get("parameter_type") == "credential"] + assert len(cred_params) == 1 + assert cred_params[0]["key"] == "credentials" + assert cred_params[0]["credential_id"] == "cred_abc123" + + blocks = sent_definition["workflow_definition"]["blocks"] + login_block = next(b for b in blocks if b.get("label") == "login_block") + assert "credentials" in login_block.get("parameter_keys", []) + + @pytest.mark.asyncio async def test_workflow_create_falls_back_on_schema_validation_error(monkeypatch: pytest.MonkeyPatch) -> None: """If the internal schema rejects the payload, normalization is skipped and the raw dict is forwarded."""