From 830fe1bcc54f63c1e16700645d73719a0620e82f Mon Sep 17 00:00:00 2001 From: pedrohsdb Date: Fri, 2 Oct 2026 12:17:43 -0700 Subject: [PATCH] Let Task V3's finish settle check pass a page with a ticking countdown, clock or headline (#8772) --- skyvern/forge/agent.py | 27 +++++++-- skyvern/forge/taskv3/engine.py | 3 +- skyvern/forge/taskv3/loop.py | 4 +- tests/unit/test_agent_task_v3.py | 34 ++++++++++- tests/unit/test_taskv3_engine.py | 36 +++++++----- tests/unit/test_taskv3_tools.py | 99 ++++++++++++++++++++++++++++++++ 6 files changed, 181 insertions(+), 22 deletions(-) diff --git a/skyvern/forge/agent.py b/skyvern/forge/agent.py index 70af3a208..3ab1a0e73 100644 --- a/skyvern/forge/agent.py +++ b/skyvern/forge/agent.py @@ -564,7 +564,7 @@ _RUN_TYPE_BY_ENGINE: dict[RunEngine, str] = { _PAGE_FINGERPRINT_PROBE_JS = ( - "() => {" + OTP_INPUT_PRIVACY_JS + " if (!document.body) return '0'; let h = 0; let v = 0; let elems = 0;" + "(maskTicks) => {" + OTP_INPUT_PRIVACY_JS + " if (!document.body) return '0'; let h = 0; let v = 0; let elems = 0;" " const mix = (str, seed) => { let x = seed;" " for (let i = 0; i < str.length; i++) x = (Math.imul(x, 31) + str.charCodeAt(i)) | 0; return x; };" # The act-by-mark tag is OUR writing and it now outlives the action, so a page that never @@ -573,8 +573,21 @@ _PAGE_FINGERPRINT_PROBE_JS = ( # mark after mark, which is precisely the run the stall detector exists to catch. # OTP bookkeeping attributes must also be ignored after masking so stamps do not count as page progress. ' const scrub = (s) => s.replace(/ data-(?:tv3-act|tv3-cover|tv3-pick|skyvern-otp-[^\\s=]+)="[^"]*"/gi, \'\');' - " const walk = (root) => { h = mix(scrub(otpSafeHtml(root, true)), h);" + # Each sample scores every element's own text, capped at 2: +1 when rewritten, -1 when unchanged, 0 when it grew. + # From 2 until back at 0 it is a ticker, and the settle check (maskTicks) ignores a ticker's change only to a + # text whose digit-masked shape it has shown before: a countdown, a clock or a carousel, never a new word. + " let seen = window.__tv3_settle_text;" + " if (!(seen instanceof WeakMap)) seen = window.__tv3_settle_text = new WeakMap();" + " const own = (n) => { let t = ''; for (const c of n.childNodes) if (c.nodeType === 3) t += c.nodeValue; return t; };" + " const walk = (root) => { const html = scrub(otpSafeHtml(root, true)); let text = 0;" " const all = root.querySelectorAll('*'); elems += all.length;" + " for (const el of [root, ...all]) { const t = own(el); const r = seen.get(el); if (!r && !t.trim()) continue;" + " const shape = t.replace(/[0-9]+/g, '#');" + " if (!r) seen.set(el, { t: t, n: 0, s: new Set([shape]) }); else if (r.t === t) r.n = Math.max(0, r.n - 1);" + " else { r.n = t.length > r.t.length && t.startsWith(r.t.slice(0, -3)) ? 0 : Math.min(2, r.n + 1); r.t = t; }" + " if (r) r.k = r.n >= 2 || (!!r.k && r.n > 0); if (!(r && r.k && r.s.has(shape))) text = mix('|' + t, text);" + " if (r && r.s.size < 16) r.s.add(shape); }" + " h = mix(maskTicks ? ('>' + html + '<').replace(/>[^<]*<') + ':' + text : html, h);" " for (const el of root.querySelectorAll('input, textarea, select'))" " v = mix((isOtpInputValueSecret(el) ? '*' : String(el.value || '')) + '|' + (el.checked === true ? '1' : '0'), v);" " for (const el of all) { if (el.shadowRoot) walk(el.shadowRoot); } };" @@ -2861,11 +2874,11 @@ class ForgeAgent: " return window.__skyvern_doc_nonce; }" ) - async def _page_fingerprint() -> str | None: + async def _page_fingerprint(mask_ticks: bool = False) -> str | None: peek = await _fingerprint_page() if peek is None: return None - own = await peek.evaluate(_PAGE_FINGERPRINT_PROBE_JS) + own = await peek.evaluate(_PAGE_FINGERPRINT_PROBE_JS, mask_ticks) # The completion-side settle deferral rides this, and it is a LIVE gate rather than only the # shadow stall measurement: a main-frame-only fingerprint reads a page whose child frame is # still rendering as settled. When work can happen in a frame, whatever judges that work has @@ -2880,13 +2893,16 @@ class ForgeAgent: parts = [own or ""] for frame in frames: try: - parts.append(str(await frame.evaluate(_PAGE_FINGERPRINT_PROBE_JS) or "")) + parts.append(str(await frame.evaluate(_PAGE_FINGERPRINT_PROBE_JS, mask_ticks) or "")) except Exception: # A frame that will not answer contributes nothing rather than costing the page's # own fingerprint -- the deferral still has the main document to judge. LOG.debug("taskv3 page fingerprint could not read a child frame", exc_info=True) return "\n".join(parts) + async def _settle_fingerprint() -> str | None: + return await _page_fingerprint(mask_ticks=True) + # Document identity, not content: a failed call's leftover text or open menu changes the DOM # without re-mapping other selectors, while a navigation or reload (which does) wipes the nonce. async def _page_probe() -> str | None: @@ -3075,6 +3091,7 @@ class ForgeAgent: resolve_totp_placeholder=verification_state.resolve_totp_placeholder, page_free=page_free_validation, page_fingerprint=_page_fingerprint, + settle_fingerprint=_settle_fingerprint, page_probe=_page_probe, document_identity=_document_identity, reload_page=_reload_page, diff --git a/skyvern/forge/taskv3/engine.py b/skyvern/forge/taskv3/engine.py index 8f60bb070..a7c354214 100644 --- a/skyvern/forge/taskv3/engine.py +++ b/skyvern/forge/taskv3/engine.py @@ -268,6 +268,7 @@ async def run_task_v3_agent_loop( resolve_totp_placeholder: TotpPlaceholderResolver | None = None, page_free: bool = False, page_fingerprint: Callable[[], Awaitable[str | None]] | None = None, + settle_fingerprint: Callable[[], Awaitable[str | None]] | None = None, max_settle_deferrals: int = DEFAULT_MAX_SETTLE_DEFERRALS, pending_marker: Callable[[str], Awaitable[str | None]] | None = None, completion_probe: CompletionProbe | None = None, @@ -532,7 +533,7 @@ async def run_task_v3_agent_loop( return result finish_tool = make_finish_tool( - page_fingerprint=None if page_free else page_fingerprint, + page_fingerprint=None if page_free else (settle_fingerprint or page_fingerprint), max_settle_deferrals=max_settle_deferrals, pending_marker=None if page_free else pending_marker, submit_watch=None if page_free else submit_watch, diff --git a/skyvern/forge/taskv3/loop.py b/skyvern/forge/taskv3/loop.py index 7f4119e04..7ef531fd1 100644 --- a/skyvern/forge/taskv3/loop.py +++ b/skyvern/forge/taskv3/loop.py @@ -2668,7 +2668,9 @@ def make_finish_tool( "the page was still rendering, or could not be verified as settled, when you " "called finish. Wait for it to settle, re-observe, confirm the goal's effect is " "present in the loaded content (not a loading indicator or empty container), " - "then finish again." + f"then finish again. This check holds a finish at most {max_settle_deferrals} times, so a page " + "that keeps changing on its own (a clock, countdown or ticker) is not by itself a reason to " + "report failure." ) if status == "completed" and goal_check is not None: try: diff --git a/tests/unit/test_agent_task_v3.py b/tests/unit/test_agent_task_v3.py index e35c49122..c9d55b2b8 100644 --- a/tests/unit/test_agent_task_v3.py +++ b/tests/unit/test_agent_task_v3.py @@ -2869,7 +2869,8 @@ async def test_execute_task_v3_atomic_block_ceiling_pinned_to_its_own_cap(monkey class _AdvancingFormPage(_FakePage): """Every action advances the form, so the next observe is fresh page-change evidence.""" - async def evaluate(self, _js: str) -> str: + # Takes the probe's argument, as Playwright's evaluate does, so the real settle sampler reads this page. + async def evaluate(self, _js: str, _arg: Any = None) -> str: raw = await super().evaluate(_js) if "document.readyState" in _js: return raw @@ -5075,6 +5076,37 @@ async def test_execute_task_v3_page_fingerprint_samples_child_frames(monkeypatch assert second != first +@pytest.mark.asyncio +async def test_execute_task_v3_settle_fingerprint_masks_ticks_and_page_fingerprint_does_not( + monkeypatch: pytest.MonkeyPatch, +) -> None: + outcome = LoopOutcome(status="completed", reason="done", billable_actions=[]) + main_frame = object() + child = MagicMock() + child.parent_frame = main_frame + child.is_detached = MagicMock(return_value=False) + host = MagicMock() + host.is_visible = AsyncMock(return_value=True) + host.dispose = AsyncMock() + child.frame_element = AsyncMock(return_value=host) + child.evaluate = AsyncMock(side_effect=lambda _js, mask=None: "frame-masked" if mask else "frame-raw") + pinned = MagicMock() + pinned.is_closed = MagicMock(return_value=False) + pinned.main_frame = main_frame + pinned.frames = [main_frame, child] + pinned.evaluate = AsyncMock(side_effect=lambda _js, mask=None: "masked" if mask else "raw") + + _step, _task, loop_mock, _post = await _run_execute_task_v3( + monkeypatch, + outcome, + working_page=pinned, + data_extraction_goal=None, + extracted_information_schema=None, + ) + assert await loop_mock.await_args.kwargs["settle_fingerprint"]() == "masked\nframe-masked" + assert await loop_mock.await_args.kwargs["page_fingerprint"]() == "raw\nframe-raw" + + @pytest.mark.asyncio async def test_execute_task_v3_document_identity_changes_with_an_acted_in_frame_only( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/unit/test_taskv3_engine.py b/tests/unit/test_taskv3_engine.py index 41e5fdc42..7f9c30b29 100644 --- a/tests/unit/test_taskv3_engine.py +++ b/tests/unit/test_taskv3_engine.py @@ -705,6 +705,12 @@ async def test_engine_forwards_the_page_fingerprint_and_withholds_it_from_page_f from skyvern.forge.taskv3.loop import LoopOutcome captured: list[object] = [] + finish_samplers: list[object] = [] + real_make = engine_mod.make_finish_tool + + def _capture_finish(*args: Any, **kwargs: Any) -> Any: + finish_samplers.append(kwargs.get("page_fingerprint")) + return real_make(*args, **kwargs) async def _capture(**kwargs: object) -> LoopOutcome: captured.append(kwargs.get("page_fingerprint")) @@ -713,21 +719,23 @@ async def test_engine_forwards_the_page_fingerprint_and_withholds_it_from_page_f async def fingerprint() -> str | None: return "markup-1" + async def settle() -> str | None: + return "markup-#" + + monkeypatch.setattr(engine_mod, "make_finish_tool", _capture_finish) monkeypatch.setattr(engine_mod, "run_agent_tool_loop", _capture) - await run_task_v3_agent_loop( - page_provider=_fixed_page_provider(_FakePage()), - llm_caller=_ScriptedCaller([]), - goal="x", - page_fingerprint=fingerprint, - ) - await run_task_v3_agent_loop( - page_provider=_fixed_page_provider(_FakePage()), - llm_caller=_ScriptedCaller([]), - goal="x", - page_fingerprint=fingerprint, - page_free=True, - ) - assert captured == [fingerprint, None] + for settle_sampler, page_free in ((None, False), (settle, False), (settle, True)): + await run_task_v3_agent_loop( + page_provider=_fixed_page_provider(_FakePage()), + llm_caller=_ScriptedCaller([]), + goal="x", + page_fingerprint=fingerprint, + settle_fingerprint=settle_sampler, + page_free=page_free, + ) + assert captured == [fingerprint, fingerprint, None] + # The settle gate reads the tick-masked sampler when one is given; stall telemetry keeps the raw one. + assert finish_samplers == [fingerprint, settle, None] @pytest.mark.asyncio diff --git a/tests/unit/test_taskv3_tools.py b/tests/unit/test_taskv3_tools.py index 592e99825..a0fa9c3e9 100644 --- a/tests/unit/test_taskv3_tools.py +++ b/tests/unit/test_taskv3_tools.py @@ -24593,6 +24593,105 @@ async def test_acting_on_a_new_mark_does_not_read_as_a_page_change() -> None: assert await page.evaluate(_PAGE_FINGERPRINT_PROBE_JS) == after +_TICK = "t.textContent = String(Number(t.textContent) - 1)" + + +@_skip_no_browser +@pytest.mark.asyncio +@pytest.mark.parametrize( + "html,mutations,settled", + [ + # A countdown is a ticker once it has been seen rewriting itself twice. + ("

Session expires in 3600 s

", [_TICK, _TICK, _TICK], True), + # The settle check's 0.7s pair often falls between two ticks; a still sample must not demote the ticker. + ("

Session expires in 3600 s

", [_TICK, _TICK, ""], True), + # A carousel cycles through texts it has shown before. + ( + "

First story

", + ["t.textContent = 'Second story'", "t.textContent = 'First story'", "t.textContent = 'Second story'"], + True, + ), + # A ticker's change to a text it has never shown (a progress counter ending in a word) still blocks. + ( + "

Loading 12%

", + [ + "t.textContent = 'Loading 47%'", + "t.textContent = 'Loading 63%'", + "t.textContent = 'Loading 81%'", + "t.textContent = 'Saved'", + ], + False, + ), + # A tracked ticker cleared to empty inside the pair still blocks. + ("

Session expires in 3600 s

", [_TICK, _TICK, _TICK, "t.textContent = ''"], False), + # Two rewrites make a ticker, not one: a value that changed before both samples of the pair still blocks. + ("

0.00

", ["t.textContent = '0.50'", "t.textContent = '1.25'"], False), + # A value that loads once never becomes a ticker, so a load during the wait still blocks. + ("

Balance 0.00

", ["", "", "t.textContent = '1,234.56'"], False), + # A status line moves only when acted on, with still samples between, so its third change still blocks. + ( + "

Step 1 of 3

", + ["t.textContent = 'Step 2 of 3'", "", "t.textContent = 'Step 3 of 3'", "", "t.textContent = 'Submitted'"], + False, + ), + # A status line that toggled in a burst is a ticker only briefly: two still samples demote it. + ( + "

Ready

", + [ + "t.textContent = 'Busy'", + "t.textContent = 'Done'", + "t.textContent = 'Busy'", + "t.textContent = 'Done'", + "", + "", + "t.textContent = 'Busy'", + ], + False, + ), + ( + "

Writing

", + [ + "t.textContent = 'Writing the▍'", + "t.textContent = 'Writing the answer▍'", + "t.textContent = 'Writing the answer now▍'", + "t.textContent = 'Writing the answer now done▍'", + ], + False, + ), + # Text streaming into a node appends; it is never a ticker. + ("

The

", ["t.textContent += ' answer'", "t.textContent += ' is'", "t.textContent += ' 42'"], False), + ( + "

3600

", + [_TICK, _TICK, _TICK + "; l.append(document.createElement('li'))"], + False, + ), + ( + "

3600x

", + [_TICK, _TICK, _TICK + "; b.style.width = '9px'"], + False, + ), + ], +) +async def test_the_settle_fingerprint_ignores_tickers_but_not_rendering( + html: str, mutations: list[str], settled: bool +) -> None: + from skyvern.forge.agent import _PAGE_FINGERPRINT_PROBE_JS + + async with _content_page(html) as page: + samples = [await page.evaluate(_PAGE_FINGERPRINT_PROBE_JS, True)] + raw_before = await page.evaluate(_PAGE_FINGERPRINT_PROBE_JS, False) + for mutate in mutations: + await page.evaluate( + "() => { const t = document.getElementById('t'), l = document.getElementById('l');" + f" const b = document.getElementById('b'); {mutate}; }}" + ) + samples.append(await page.evaluate(_PAGE_FINGERPRINT_PROBE_JS, True)) + raw_after = await page.evaluate(_PAGE_FINGERPRINT_PROBE_JS, False) + # The settle check's own two samples are the last two; earlier ones are the run's history. + assert (samples[-2] == samples[-1]) is settled + assert raw_before != raw_after # the unmasked fingerprint, which page-change telemetry reads, still moves + + # A payload URL long enough to hit observe's per-field display caps (label 140, value 100, # placeholder 60). Masking is by provenance over the WHOLE URL, so a cap applied before the masker # runs leaves a truncated URL the masker cannot recognise -- and the signing tail with it.