mirror of
https://github.com/Skyvern-AI/skyvern.git
synced 2026-10-02 19:57:59 +00:00
Let Task V3's finish settle check pass a page with a ticking countdown, clock or headline (#8772)
Some checks are pending
Run tests and pre-commit / Run tests and pre-commit hooks (push) Waiting to run
Run tests and pre-commit / Frontend Lint and Build (push) Waiting to run
Run tests and pre-commit / pip Package Smoke Tests (3.11) (push) Waiting to run
Run tests and pre-commit / pip Package Smoke Tests (3.13) (push) Waiting to run
Publish Fern Docs / run (push) Waiting to run
Some checks are pending
Run tests and pre-commit / Run tests and pre-commit hooks (push) Waiting to run
Run tests and pre-commit / Frontend Lint and Build (push) Waiting to run
Run tests and pre-commit / pip Package Smoke Tests (3.11) (push) Waiting to run
Run tests and pre-commit / pip Package Smoke Tests (3.13) (push) Waiting to run
Publish Fern Docs / run (push) Waiting to run
This commit is contained in:
parent
d962f9fa13
commit
830fe1bcc5
6 changed files with 181 additions and 22 deletions
|
|
@ -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(/>[^<]*</g, '><') + ':' + 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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
("<p>Session expires in <span id=t>3600</span> s</p>", [_TICK, _TICK, _TICK], True),
|
||||
# The settle check's 0.7s pair often falls between two ticks; a still sample must not demote the ticker.
|
||||
("<p>Session expires in <span id=t>3600</span> s</p>", [_TICK, _TICK, ""], True),
|
||||
# A carousel cycles through texts it has shown before.
|
||||
(
|
||||
"<h2 id=t>First story</h2>",
|
||||
["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.
|
||||
(
|
||||
"<p id=t>Loading 12%</p>",
|
||||
[
|
||||
"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.
|
||||
("<p>Session expires in <span id=t>3600</span> s</p>", [_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.
|
||||
("<p id=t>0.00</p>", ["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.
|
||||
("<p>Balance <span id=t>0.00</span></p>", ["", "", "t.textContent = '1,234.56'"], False),
|
||||
# A status line moves only when acted on, with still samples between, so its third change still blocks.
|
||||
(
|
||||
"<p id=t>Step 1 of 3</p>",
|
||||
["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.
|
||||
(
|
||||
"<p id=t>Ready</p>",
|
||||
[
|
||||
"t.textContent = 'Busy'",
|
||||
"t.textContent = 'Done'",
|
||||
"t.textContent = 'Busy'",
|
||||
"t.textContent = 'Done'",
|
||||
"",
|
||||
"",
|
||||
"t.textContent = 'Busy'",
|
||||
],
|
||||
False,
|
||||
),
|
||||
(
|
||||
"<p id=t>Writing</p>",
|
||||
[
|
||||
"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.
|
||||
("<p id=t>The</p>", ["t.textContent += ' answer'", "t.textContent += ' is'", "t.textContent += ' 42'"], False),
|
||||
(
|
||||
"<p><span id=t>3600</span><ul id=l></ul></p>",
|
||||
[_TICK, _TICK, _TICK + "; l.append(document.createElement('li'))"],
|
||||
False,
|
||||
),
|
||||
(
|
||||
"<p><span id=t>3600</span><b id=b style='width: 1px'>x</b></p>",
|
||||
[_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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue