mirror of
https://github.com/Skyvern-AI/skyvern.git
synced 2026-07-09 16:09:13 +00:00
copilot enforcement: drain post-run screenshot only when a nudge fires (SKY-9480) (#5807)
This commit is contained in:
parent
9313065d51
commit
afa78ce7be
42 changed files with 3601 additions and 338 deletions
|
|
@ -563,6 +563,7 @@ export type TaskRunListItem = {
|
|||
finished_at: string | null;
|
||||
created_at: string;
|
||||
workflow_permanent_id: string | null;
|
||||
workflow_deleted: boolean;
|
||||
script_run: boolean;
|
||||
searchable_text: string | null;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1,4 +1,8 @@
|
|||
import { LightningBoltIcon, MixerHorizontalIcon } from "@radix-ui/react-icons";
|
||||
import {
|
||||
ExclamationTriangleIcon,
|
||||
LightningBoltIcon,
|
||||
MixerHorizontalIcon,
|
||||
} from "@radix-ui/react-icons";
|
||||
|
||||
import {
|
||||
Select,
|
||||
|
|
@ -192,16 +196,31 @@ function RunHistory() {
|
|||
const isExpanded = isWorkflowRun && expandedRows.has(run.run_id);
|
||||
const navPath = getRunNavigationPath(run);
|
||||
|
||||
const titleContent = run.script_run ? (
|
||||
<div className="flex items-center gap-2">
|
||||
<Tip content="Ran with code">
|
||||
<LightningBoltIcon className="text-[gold]" />
|
||||
</Tip>
|
||||
<span>{run.title ?? ""}</span>
|
||||
</div>
|
||||
) : (
|
||||
(run.title ?? "")
|
||||
);
|
||||
const titleContent =
|
||||
run.script_run || run.workflow_deleted ? (
|
||||
<div className="flex items-center gap-2">
|
||||
{run.script_run && (
|
||||
<Tip content="Ran with code">
|
||||
<LightningBoltIcon className="text-[gold]" />
|
||||
</Tip>
|
||||
)}
|
||||
{run.workflow_deleted && (
|
||||
<Tip content="Source workflow deleted">
|
||||
<ExclamationTriangleIcon className="text-amber-400" />
|
||||
</Tip>
|
||||
)}
|
||||
<span
|
||||
className={cn(
|
||||
run.workflow_deleted && "text-slate-400",
|
||||
"truncate",
|
||||
)}
|
||||
>
|
||||
{run.title ?? ""}
|
||||
</span>
|
||||
</div>
|
||||
) : (
|
||||
(run.title ?? "")
|
||||
);
|
||||
|
||||
return (
|
||||
<React.Fragment key={run.task_run_id}>
|
||||
|
|
|
|||
|
|
@ -80,6 +80,7 @@ function WorkflowRun() {
|
|||
const workflowPermanentId = workflow?.workflow_permanent_id;
|
||||
const cacheKey = workflow?.cache_key ?? "";
|
||||
const isFinalized = workflowRun ? statusIsFinalized(workflowRun) : null;
|
||||
const isWorkflowDeleted = Boolean(workflow?.deleted_at);
|
||||
|
||||
const [hasPublishedCode, setHasPublishedCode] = useState(false);
|
||||
|
||||
|
|
@ -93,7 +94,7 @@ function WorkflowRun() {
|
|||
cacheKey,
|
||||
debounceMs: 100,
|
||||
page: 1,
|
||||
workflowPermanentId,
|
||||
workflowPermanentId: isWorkflowDeleted ? undefined : workflowPermanentId,
|
||||
});
|
||||
|
||||
useEffect(() => {
|
||||
|
|
@ -106,7 +107,7 @@ function WorkflowRun() {
|
|||
const { data: blockScriptsPublished } = useBlockScriptsQuery({
|
||||
cacheKey,
|
||||
cacheKeyValue,
|
||||
workflowPermanentId,
|
||||
workflowPermanentId: isWorkflowDeleted ? undefined : workflowPermanentId,
|
||||
pollIntervalMs: !hasPublishedCode && !isFinalized ? 3000 : undefined,
|
||||
status: "published",
|
||||
workflowRunId: workflowRun?.workflow_run_id,
|
||||
|
|
@ -159,7 +160,7 @@ function WorkflowRun() {
|
|||
const { data: fallbackEpisodes } = useFallbackEpisodesQuery({
|
||||
workflowPermanentId,
|
||||
workflowRunId: workflowRun?.workflow_run_id,
|
||||
enabled: workflowRunIsFinalized === true,
|
||||
enabled: workflowRunIsFinalized === true && !isWorkflowDeleted,
|
||||
});
|
||||
const finallyBlockLabel =
|
||||
workflow?.workflow_definition?.finally_block_label ?? null;
|
||||
|
|
@ -176,6 +177,8 @@ function WorkflowRun() {
|
|||
|
||||
const title = workflowRunIsLoading ? (
|
||||
<Skeleton className="h-9 w-48" />
|
||||
) : isWorkflowDeleted ? (
|
||||
<h1 className="text-3xl">{workflow!.title}</h1>
|
||||
) : (
|
||||
<h1 className="text-3xl">
|
||||
<Link
|
||||
|
|
@ -362,8 +365,10 @@ function WorkflowRun() {
|
|||
</div>
|
||||
<h2 className="text-2xl text-slate-400">{workflowRunId}</h2>
|
||||
{workflowRun &&
|
||||
(workflowRun.started_at || workflowRun.finished_at) && (
|
||||
<div className="flex gap-4 text-sm text-slate-400">
|
||||
(workflowRun.started_at ||
|
||||
workflowRun.finished_at ||
|
||||
isWorkflowDeleted) && (
|
||||
<div className="flex flex-wrap gap-x-4 gap-y-1 text-sm text-slate-400">
|
||||
{workflowRun.started_at && (
|
||||
<span title={basicTimeFormat(workflowRun.started_at)}>
|
||||
Started: {basicLocalTimeFormat(workflowRun.started_at)}
|
||||
|
|
@ -374,6 +379,12 @@ function WorkflowRun() {
|
|||
Finished: {basicLocalTimeFormat(workflowRun.finished_at)}
|
||||
</span>
|
||||
)}
|
||||
{isWorkflowDeleted && (
|
||||
<span title={basicTimeFormat(workflow!.deleted_at!)}>
|
||||
Workflow deleted on{" "}
|
||||
{basicLocalTimeFormat(workflow!.deleted_at!)}
|
||||
</span>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
{workflowRun?.browser_session_id && (
|
||||
|
|
@ -387,51 +398,57 @@ function WorkflowRun() {
|
|||
</div>
|
||||
|
||||
<div className="flex gap-2">
|
||||
<ApiWebhookActionsMenu
|
||||
getOptions={() => {
|
||||
// Build headers - x-max-steps-override is optional and can be added manually if needed
|
||||
const headers: Record<string, string> = {
|
||||
"Content-Type": "application/json",
|
||||
"x-api-key": apiCredential ?? "<your-api-key>",
|
||||
};
|
||||
{!isWorkflowDeleted && (
|
||||
<>
|
||||
<ApiWebhookActionsMenu
|
||||
getOptions={() => {
|
||||
// Build headers - x-max-steps-override is optional and can be added manually if needed
|
||||
const headers: Record<string, string> = {
|
||||
"Content-Type": "application/json",
|
||||
"x-api-key": apiCredential ?? "<your-api-key>",
|
||||
};
|
||||
|
||||
const body: Record<string, unknown> = {
|
||||
workflow_id: workflowPermanentId,
|
||||
parameters: workflowRun?.parameters,
|
||||
proxy_location: proxyLocation,
|
||||
};
|
||||
const body: Record<string, unknown> = {
|
||||
workflow_id: workflowPermanentId,
|
||||
parameters: workflowRun?.parameters,
|
||||
proxy_location: proxyLocation,
|
||||
};
|
||||
|
||||
if (maxScreenshotScrolls !== null) {
|
||||
body.max_screenshot_scrolls = maxScreenshotScrolls;
|
||||
}
|
||||
if (maxScreenshotScrolls !== null) {
|
||||
body.max_screenshot_scrolls = maxScreenshotScrolls;
|
||||
}
|
||||
|
||||
if (workflowRun?.webhook_callback_url) {
|
||||
body.webhook_url = workflowRun.webhook_callback_url;
|
||||
}
|
||||
if (workflowRun?.webhook_callback_url) {
|
||||
body.webhook_url = workflowRun.webhook_callback_url;
|
||||
}
|
||||
|
||||
return {
|
||||
method: "POST",
|
||||
url: `${runsApiBaseUrl}/run/workflows`,
|
||||
body,
|
||||
headers,
|
||||
} satisfies ApiCommandOptions;
|
||||
}}
|
||||
webhookDisabled={workflowRunIsLoading || !workflowRunIsFinalized}
|
||||
onTestWebhook={() => setReplayOpen(true)}
|
||||
/>
|
||||
<WebhookReplayDialog
|
||||
runId={workflowRunId ?? ""}
|
||||
disabled={workflowRunIsLoading || !workflowRunIsFinalized}
|
||||
open={replayOpen}
|
||||
onOpenChange={setReplayOpen}
|
||||
hideTrigger
|
||||
/>
|
||||
<Button asChild variant="secondary">
|
||||
<Link to={`/workflows/${workflowPermanentId}/build`}>
|
||||
<Pencil2Icon className="mr-2 h-4 w-4" />
|
||||
Edit
|
||||
</Link>
|
||||
</Button>
|
||||
return {
|
||||
method: "POST",
|
||||
url: `${runsApiBaseUrl}/run/workflows`,
|
||||
body,
|
||||
headers,
|
||||
} satisfies ApiCommandOptions;
|
||||
}}
|
||||
webhookDisabled={
|
||||
workflowRunIsLoading || !workflowRunIsFinalized
|
||||
}
|
||||
onTestWebhook={() => setReplayOpen(true)}
|
||||
/>
|
||||
<WebhookReplayDialog
|
||||
runId={workflowRunId ?? ""}
|
||||
disabled={workflowRunIsLoading || !workflowRunIsFinalized}
|
||||
open={replayOpen}
|
||||
onOpenChange={setReplayOpen}
|
||||
hideTrigger
|
||||
/>
|
||||
<Button asChild variant="secondary">
|
||||
<Link to={`/workflows/${workflowPermanentId}/build`}>
|
||||
<Pencil2Icon className="mr-2 h-4 w-4" />
|
||||
Edit
|
||||
</Link>
|
||||
</Button>
|
||||
</>
|
||||
)}
|
||||
{workflowRunIsCancellable && (
|
||||
<Dialog>
|
||||
<DialogTrigger asChild>
|
||||
|
|
@ -464,7 +481,7 @@ function WorkflowRun() {
|
|||
</DialogContent>
|
||||
</Dialog>
|
||||
)}
|
||||
{workflowRunIsFinalized && !isTaskv2Run && (
|
||||
{workflowRunIsFinalized && !isTaskv2Run && !isWorkflowDeleted && (
|
||||
<Button asChild>
|
||||
<Link
|
||||
to={`/workflows/${workflowPermanentId}/run`}
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import hashlib
|
|||
import json
|
||||
import keyword
|
||||
import re
|
||||
import zlib
|
||||
from collections import deque
|
||||
from dataclasses import dataclass
|
||||
from typing import Any
|
||||
|
|
@ -31,6 +32,7 @@ from skyvern.core.script_generations.parameter_reference_guard import (
|
|||
log_or_raise_guard_result,
|
||||
validate_context_parameter_refs,
|
||||
)
|
||||
from skyvern.core.script_generations.script_validators import validate_missing_selectors
|
||||
from skyvern.forge import app
|
||||
from skyvern.forge.sdk.workflow.models.parameter import (
|
||||
WorkflowParameterType,
|
||||
|
|
@ -354,6 +356,9 @@ ACTIONS_WITH_XPATH = [
|
|||
"upload_file",
|
||||
"select_option",
|
||||
]
|
||||
# Methods whose runtime threads `recoverable_marker_id` to the recorder.
|
||||
# Markers emitted on other methods would be dead — the recovery loop can never match them.
|
||||
RECOVERABLE_MARKER_METHODS = frozenset({"click", "fill", "type"})
|
||||
|
||||
|
||||
def _build_semantic_selector(act: dict[str, Any]) -> str | None:
|
||||
|
|
@ -861,6 +866,8 @@ def _action_to_stmt(
|
|||
method = ACTION_MAP[act["action_type"]]
|
||||
|
||||
args: list[cst.Arg] = []
|
||||
selector_emitted = False
|
||||
recoverable_marker_id: int | None = None
|
||||
if method in ACTIONS_WITH_XPATH:
|
||||
if use_semantic_selectors:
|
||||
semantic = _build_semantic_selector(act)
|
||||
|
|
@ -875,7 +882,26 @@ def _action_to_stmt(
|
|||
),
|
||||
)
|
||||
)
|
||||
# If no semantic selector, skip selector arg — ai with prompt= handles it
|
||||
selector_emitted = True
|
||||
# No semantic selector — caller downgrades to ai='proactive' so the
|
||||
# runtime never sees selectorless ai='fallback' (which crashes with
|
||||
# `selector: expected string, got undefined`). The marker_id is the
|
||||
# stable join key so a future recovery loop can later upgrade this
|
||||
# call back to fallback+selector once the AI's element pick is
|
||||
# captured at runtime.
|
||||
elif method in RECOVERABLE_MARKER_METHODS:
|
||||
# crc32 is process-stable (built-in hash() is salted); action_id
|
||||
# disambiguates duplicate xpath+intention pairs within a block;
|
||||
# 31-bit mask keeps the value within PG signed INTEGER range.
|
||||
# Both intention and reasoning are concatenated (not OR'd) so the marker
|
||||
# stays stable if a regen flips which field is populated.
|
||||
marker_seed = (
|
||||
f"{act.get('xpath', '')}|"
|
||||
f"{act.get('intention') or ''}|"
|
||||
f"{act.get('reasoning') or ''}|"
|
||||
f"{act.get('action_id', '')}"
|
||||
)
|
||||
recoverable_marker_id = (zlib.crc32(marker_seed.encode("utf-8")) & 0x7FFFFFFF) or 1
|
||||
else:
|
||||
args.append(
|
||||
cst.Arg(
|
||||
|
|
@ -887,11 +913,11 @@ def _action_to_stmt(
|
|||
),
|
||||
)
|
||||
)
|
||||
selector_emitted = True
|
||||
|
||||
if method == "click":
|
||||
if use_semantic_selectors:
|
||||
# With semantic selectors, try selector first, AI only if miss
|
||||
ai_mode = GENERATE_CODE_AI_MODE_FALLBACK
|
||||
ai_mode = GENERATE_CODE_AI_MODE_FALLBACK if selector_emitted else GENERATE_CODE_AI_MODE_PROACTIVE
|
||||
else:
|
||||
ai_mode = GENERATE_CODE_AI_MODE_PROACTIVE
|
||||
click_context = act.get("click_context")
|
||||
|
|
@ -970,7 +996,7 @@ def _action_to_stmt(
|
|||
else:
|
||||
text_value = _value(act["text"])
|
||||
|
||||
ai_mode = GENERATE_CODE_AI_MODE_FALLBACK
|
||||
ai_mode = GENERATE_CODE_AI_MODE_FALLBACK if selector_emitted else GENERATE_CODE_AI_MODE_PROACTIVE
|
||||
if _requires_mini_agent(act):
|
||||
ai_mode = GENERATE_CODE_AI_MODE_PROACTIVE
|
||||
|
||||
|
|
@ -1022,6 +1048,17 @@ def _action_to_stmt(
|
|||
value = option.get("value")
|
||||
label = option.get("label")
|
||||
value = value or label
|
||||
if not value and use_semantic_selectors and (act.get("intention") or act.get("reasoning")):
|
||||
args.append(
|
||||
cst.Arg(
|
||||
keyword=cst.Name("ai"),
|
||||
value=_value(GENERATE_CODE_AI_MODE_PROACTIVE),
|
||||
whitespace_after_arg=cst.ParenthesizedWhitespace(
|
||||
indent=True,
|
||||
last_line=cst.SimpleWhitespace(INDENT),
|
||||
),
|
||||
)
|
||||
)
|
||||
if value:
|
||||
# Mirror the click branch: with semantic selectors we have a real
|
||||
# CSS selector + value, so try the selector path first and fall
|
||||
|
|
@ -1029,7 +1066,7 @@ def _action_to_stmt(
|
|||
# selector is an xpath harvested from iteration 0 and unlikely to
|
||||
# be reliable, so go straight to AI.
|
||||
if use_semantic_selectors:
|
||||
ai_mode = GENERATE_CODE_AI_MODE_FALLBACK
|
||||
ai_mode = GENERATE_CODE_AI_MODE_FALLBACK if selector_emitted else GENERATE_CODE_AI_MODE_PROACTIVE
|
||||
else:
|
||||
ai_mode = GENERATE_CODE_AI_MODE_PROACTIVE
|
||||
if act.get("field_name"):
|
||||
|
|
@ -1207,16 +1244,36 @@ def _action_to_stmt(
|
|||
if prompt_value is None:
|
||||
prompt_value = _value(intention)
|
||||
|
||||
# When a marker arg follows the prompt, prompt's last_line must point
|
||||
# at the inner indent so the marker lines up with sibling args (libcst
|
||||
# uses the *previous* arg's last_line to position the next arg).
|
||||
prompt_trailing = (
|
||||
cst.ParenthesizedWhitespace(indent=True, last_line=cst.SimpleWhitespace(INDENT))
|
||||
if recoverable_marker_id is not None
|
||||
else cst.ParenthesizedWhitespace(indent=True)
|
||||
)
|
||||
args.extend(
|
||||
[
|
||||
cst.Arg(
|
||||
keyword=cst.Name("prompt"),
|
||||
value=prompt_value,
|
||||
whitespace_after_arg=cst.ParenthesizedWhitespace(indent=True),
|
||||
whitespace_after_arg=prompt_trailing,
|
||||
comma=cst.Comma(),
|
||||
),
|
||||
]
|
||||
)
|
||||
if recoverable_marker_id is not None:
|
||||
args.append(
|
||||
cst.Arg(
|
||||
keyword=cst.Name("recoverable_marker_id"),
|
||||
value=_value(recoverable_marker_id),
|
||||
whitespace_after_arg=cst.ParenthesizedWhitespace(
|
||||
indent=True,
|
||||
last_line=cst.SimpleWhitespace(INDENT),
|
||||
),
|
||||
comma=cst.Comma(),
|
||||
)
|
||||
)
|
||||
_mark_last_arg_as_comma(args)
|
||||
|
||||
# Only use indented parentheses if we have arguments
|
||||
|
|
@ -3705,9 +3762,42 @@ async def generate_workflow_script_python_code(
|
|||
except Exception:
|
||||
LOG.warning("parameter_reference_guard_failed_to_run", exc_info=True)
|
||||
|
||||
_check_missing_selectors_and_warn(
|
||||
source_code,
|
||||
organization_id=organization_id,
|
||||
workflow_permanent_id=workflow.get("workflow_permanent_id"),
|
||||
workflow_run_id=run_id,
|
||||
)
|
||||
|
||||
return CodeGenResult(source_code=source_code, blocks_created=blocks_created, blocks_failed=blocks_failed)
|
||||
|
||||
|
||||
def _check_missing_selectors_and_warn(
|
||||
source_code: str,
|
||||
*,
|
||||
organization_id: str | None = None,
|
||||
workflow_permanent_id: str | None,
|
||||
workflow_run_id: str | None,
|
||||
) -> str | None:
|
||||
# Returns the validator error string (or None) so tests can assert without
|
||||
# capturing log output. Validator exceptions are swallowed so a parse crash
|
||||
# never blocks codegen.
|
||||
try:
|
||||
selector_warning = validate_missing_selectors(source_code)
|
||||
except Exception:
|
||||
LOG.warning("script_generator_missing_selector_validator_failed_to_run", exc_info=True)
|
||||
return None
|
||||
if selector_warning:
|
||||
LOG.warning(
|
||||
"script_generator_emitted_selectorless_action",
|
||||
organization_id=organization_id,
|
||||
workflow_permanent_id=workflow_permanent_id,
|
||||
workflow_run_id=workflow_run_id,
|
||||
detail=selector_warning,
|
||||
)
|
||||
return selector_warning
|
||||
|
||||
|
||||
async def create_or_update_script_block(
|
||||
block_code: str | bytes,
|
||||
script_revision_id: str,
|
||||
|
|
|
|||
|
|
@ -169,6 +169,7 @@ class RealSkyvernPageAi(SkyvernPageAi):
|
|||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
failed_selector: str | None = None,
|
||||
block_label: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
) -> str | None:
|
||||
"""Click an element using AI to locate it based on intention.
|
||||
|
||||
|
|
@ -236,6 +237,7 @@ class RealSkyvernPageAi(SkyvernPageAi):
|
|||
intention=intention,
|
||||
action=action,
|
||||
block_label=block_label,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
|
||||
return selector
|
||||
|
|
@ -263,6 +265,7 @@ class RealSkyvernPageAi(SkyvernPageAi):
|
|||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
failed_selector: str | None = None,
|
||||
block_label: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
) -> str:
|
||||
"""Input text into an element using AI to determine the value."""
|
||||
|
||||
|
|
@ -386,6 +389,7 @@ class RealSkyvernPageAi(SkyvernPageAi):
|
|||
intention=intention,
|
||||
action=action,
|
||||
block_label=block_label,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
else:
|
||||
locator = self.page.locator(selector)
|
||||
|
|
@ -691,28 +695,32 @@ class RealSkyvernPageAi(SkyvernPageAi):
|
|||
intention: str,
|
||||
action: Any,
|
||||
block_label: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
) -> None:
|
||||
"""Record an element-level fallback episode when ai_click/ai_input_text fires
|
||||
because a CSS selector failed or was missing. Gated on code_version >= 2.
|
||||
|
||||
This gives the script reviewer the signal AND the action data (css_suggestion,
|
||||
element attributes) it needs to write a proper selector for the next script version.
|
||||
Two trigger conditions:
|
||||
- `failed_selector` set → fallback path: a tried selector missed.
|
||||
- `recoverable_marker_id` set → SKY-9436 escape hatch: generator emitted
|
||||
`ai='proactive'` because no semantic selector existed at codegen.
|
||||
AI succeeded; capture the element pick so the reviewer can later
|
||||
upgrade the call to `selector=, ai='fallback'`.
|
||||
"""
|
||||
if failed_selector is None:
|
||||
# None means the caller didn't pass failed_selector — this is a direct
|
||||
# ai_click call (not from the ai='fallback' path), so don't record.
|
||||
if failed_selector is None and recoverable_marker_id is None:
|
||||
return
|
||||
if (context.code_version or 0) < 2:
|
||||
return
|
||||
if not context.workflow_run_id or not context.workflow_permanent_id:
|
||||
return
|
||||
try:
|
||||
# Build agent_actions data for the reviewer
|
||||
action_data: dict[str, Any] = {
|
||||
"action_type": action_type,
|
||||
"intention": intention,
|
||||
"failed_selector": failed_selector if failed_selector else "(missing — no selector= argument)",
|
||||
}
|
||||
if recoverable_marker_id is not None:
|
||||
action_data["recoverable_marker_id"] = recoverable_marker_id
|
||||
if hasattr(action, "element_id"):
|
||||
action_data["element_id"] = action.element_id
|
||||
if hasattr(action, "skyvern_element_data") and action.skyvern_element_data:
|
||||
|
|
@ -752,12 +760,19 @@ class RealSkyvernPageAi(SkyvernPageAi):
|
|||
if hasattr(action, "reasoning"):
|
||||
action_data["reasoning"] = action.reasoning
|
||||
|
||||
error_msg = (
|
||||
f"Selector {'failed' if failed_selector else 'missing'} on page.{action_type}(), "
|
||||
f"AI fallback succeeded. "
|
||||
f"Original selector: {failed_selector or '(none)'}. "
|
||||
f"Intention: {intention}"
|
||||
)
|
||||
if recoverable_marker_id is not None and failed_selector is None:
|
||||
error_msg = (
|
||||
f"Proactive recovery on page.{action_type}() (marker={recoverable_marker_id}): "
|
||||
f"generator emitted ai='proactive' (no semantic selector at codegen); "
|
||||
f"AI picked the element. Intention: {intention}"
|
||||
)
|
||||
else:
|
||||
error_msg = (
|
||||
f"Selector {'failed' if failed_selector else 'missing'} on page.{action_type}(), "
|
||||
f"AI fallback succeeded. "
|
||||
f"Original selector: {failed_selector or '(none)'}. "
|
||||
f"Intention: {intention}"
|
||||
)
|
||||
await app.DATABASE.scripts.create_fallback_episode(
|
||||
organization_id=context.organization_id or "",
|
||||
workflow_permanent_id=context.workflow_permanent_id,
|
||||
|
|
|
|||
288
skyvern/core/script_generations/script_validators.py
Normal file
288
skyvern/core/script_generations/script_validators.py
Normal file
|
|
@ -0,0 +1,288 @@
|
|||
"""Shared validators for generated cached-script code.
|
||||
|
||||
Both the script generator (`generate_script.py`) and the script reviewer
|
||||
(`script_reviewer.py`) need to enforce the same code-quality rules. Keeping
|
||||
the validators here avoids drift between the two paths.
|
||||
|
||||
Validators are AST-based to correctly distinguish kwargs from text inside
|
||||
string literals (e.g. a `prompt='No selector= available'` must not look
|
||||
like the call has a selector).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import ast
|
||||
import re
|
||||
from collections import Counter
|
||||
from dataclasses import dataclass
|
||||
|
||||
INTERACTION_METHODS: frozenset[str] = frozenset({"click", "fill", "fill_autocomplete", "type", "select_option"})
|
||||
|
||||
PAGE_CALL_RE: re.Pattern[str] = re.compile(r"""\bpage\.(\w+)\s*\(""")
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class InteractionCall:
|
||||
method: str
|
||||
lineno: int
|
||||
has_selector: bool
|
||||
has_prompt: bool
|
||||
ai_value: str | None
|
||||
recoverable_marker_id: int | None = None
|
||||
sorted_kwarg_names: tuple[str, ...] = ()
|
||||
# Frozen tuple of (kwarg_name, ast_dump) pairs for kwargs whose value
|
||||
# we want to compare verbatim across input/output (prompt, value, intention).
|
||||
semantic_kwargs: tuple[tuple[str, str], ...] = ()
|
||||
|
||||
|
||||
def iter_interaction_calls(code: str) -> list[InteractionCall]:
|
||||
"""Walk `code` and yield each `await page.<interaction_method>(...)` call.
|
||||
|
||||
Returns an empty list on parse failure rather than raising — the validator
|
||||
itself must never block codegen.
|
||||
"""
|
||||
try:
|
||||
tree = ast.parse(code)
|
||||
except SyntaxError:
|
||||
return []
|
||||
|
||||
out: list[InteractionCall] = []
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
func = node.func
|
||||
if not (isinstance(func, ast.Attribute) and isinstance(func.value, ast.Name) and func.value.id == "page"):
|
||||
continue
|
||||
method = func.attr
|
||||
if method not in INTERACTION_METHODS:
|
||||
continue
|
||||
has_selector = any(kw.arg == "selector" for kw in node.keywords)
|
||||
has_prompt = any(kw.arg == "prompt" for kw in node.keywords)
|
||||
ai_value: str | None = None
|
||||
marker_id: int | None = None
|
||||
for kw in node.keywords:
|
||||
if kw.arg == "ai" and isinstance(kw.value, ast.Constant) and isinstance(kw.value.value, str):
|
||||
ai_value = kw.value.value
|
||||
elif (
|
||||
kw.arg == "recoverable_marker_id"
|
||||
and isinstance(kw.value, ast.Constant)
|
||||
and isinstance(kw.value.value, int)
|
||||
):
|
||||
marker_id = kw.value.value
|
||||
sorted_names = tuple(sorted(kw.arg for kw in node.keywords if kw.arg))
|
||||
# Capture verbatim ast.dump of EVERY kwarg so the safety validator
|
||||
# detects any semantic edit (prompt text, value source, timeout, data, etc.).
|
||||
semantic_kwargs = tuple(sorted((kw.arg, ast.dump(kw.value)) for kw in node.keywords if kw.arg))
|
||||
out.append(
|
||||
InteractionCall(
|
||||
method=method,
|
||||
lineno=node.lineno,
|
||||
has_selector=has_selector,
|
||||
has_prompt=has_prompt,
|
||||
ai_value=ai_value,
|
||||
recoverable_marker_id=marker_id,
|
||||
sorted_kwarg_names=sorted_names,
|
||||
semantic_kwargs=semantic_kwargs,
|
||||
)
|
||||
)
|
||||
return out
|
||||
|
||||
|
||||
def validate_missing_selectors(code: str) -> str | None:
|
||||
"""Flag interaction methods that lack a `selector=` argument.
|
||||
|
||||
Two cases are flagged:
|
||||
1. `ai='fallback'` but no selector — the CSS-try block is skipped entirely
|
||||
and AI fires as the primary path on every run, burning LLM tokens
|
||||
silently. Worse, on some Playwright code paths a missing selector
|
||||
raises `Locator.fill: selector: expected string, got undefined`.
|
||||
2. No `ai=` argument at all and no selector — the call has no
|
||||
deterministic path and no explicit AI strategy.
|
||||
|
||||
`ai='proactive'` without a selector is intentional (AI always generates
|
||||
the value, no caching benefit but no crash) and is NOT flagged.
|
||||
|
||||
Returns an error message describing the offending sites, or None if clean.
|
||||
"""
|
||||
issues: list[str] = []
|
||||
for call in iter_interaction_calls(code):
|
||||
if call.has_selector:
|
||||
continue
|
||||
if not call.has_prompt:
|
||||
issues.append(
|
||||
f"page.{call.method}() on line {call.lineno} (no selector= AND no prompt= — runtime will raise)"
|
||||
)
|
||||
continue
|
||||
if call.ai_value == "proactive":
|
||||
continue
|
||||
suffix = "" if call.ai_value else " (no ai= argument)"
|
||||
issues.append(f"page.{call.method}() on line {call.lineno}{suffix}")
|
||||
|
||||
if not issues:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"Missing selector on interaction methods: {', '.join(issues[:5])}. "
|
||||
f"Interaction methods without a selector= argument have no deterministic path — "
|
||||
f"they silently invoke the LLM on every run, burning tokens with no fallback "
|
||||
f"episode created. Add a selector= argument with a stable CSS selector "
|
||||
f"(aria-label, placeholder, name, role, :has-text()) and set ai='fallback' "
|
||||
f"so the element is found without an LLM call."
|
||||
)
|
||||
|
||||
|
||||
def validate_proactive_misuse(code: str) -> str | None:
|
||||
"""Flag `ai='proactive'` on interaction methods that ALSO supply a `selector=`.
|
||||
|
||||
`ai='proactive'` WITH `selector=` defeats caching — the LLM is always invoked
|
||||
even though a deterministic selector is available.
|
||||
|
||||
Two intentional exceptions are NOT flagged:
|
||||
- `ai='proactive'` WITHOUT `selector=` (the SKY-9436 escape hatch).
|
||||
- `page.select_option(selector=..., ai='proactive')` without a `value=` — the
|
||||
generator emits this when a no-value select_option has a stable label-based
|
||||
selector. The selector locates the dropdown but the LLM must still pick the
|
||||
option text at runtime, so proactive is unavoidable here.
|
||||
|
||||
Returns an error message or None if no issues found.
|
||||
"""
|
||||
issues: list[str] = []
|
||||
for call in iter_interaction_calls(code):
|
||||
if call.ai_value != "proactive" or not call.has_selector:
|
||||
continue
|
||||
if call.method == "select_option" and "value" not in call.sorted_kwarg_names:
|
||||
continue
|
||||
issues.append(f"page.{call.method}() on line {call.lineno}")
|
||||
|
||||
if not issues:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"ai='proactive' combined with selector= on interaction methods: {', '.join(issues[:5])}. "
|
||||
f"When a selector is present, ai='proactive' still ALWAYS invokes the LLM, defeating the "
|
||||
f"zero-LLM-cost goal of caching. Change to ai='fallback' — this tries the selector first "
|
||||
f"and only invokes the LLM if the selector fails. (Note: ai='proactive' WITHOUT selector= "
|
||||
f"is the documented escape hatch when no semantic selector is feasible — that is allowed.)"
|
||||
)
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class RecoverableProactiveCandidate:
|
||||
method: str
|
||||
lineno: int
|
||||
marker_id: int
|
||||
|
||||
|
||||
def find_recoverable_proactive_candidates(code: str) -> list[RecoverableProactiveCandidate]:
|
||||
"""Find selectorless `ai='proactive'` interaction calls that carry a
|
||||
`recoverable_marker_id` kwarg (SKY-9436 escape hatch).
|
||||
|
||||
These are the calls the script reviewer's Rule 8f can upgrade to
|
||||
`selector=, ai='fallback'` when a recovery episode with the same marker_id
|
||||
is available. Marker presence is the sole disambiguator vs intentional
|
||||
selectorless proactive (essay/fuzzy/ambiguous cases).
|
||||
|
||||
Per-method value-precondition for upgrade:
|
||||
- click: always safe.
|
||||
- fill / type: only when value= is present.
|
||||
|
||||
Only click/fill/type are admitted — the runtime threads `recoverable_marker_id`
|
||||
only for these methods, so a marked select_option / hover / etc. could never
|
||||
have produced a recovery episode and must never have been emitted.
|
||||
|
||||
The reviewer's prompt receives this list to focus its rewrites; not a hard
|
||||
error itself.
|
||||
"""
|
||||
out: list[RecoverableProactiveCandidate] = []
|
||||
for call in iter_interaction_calls(code):
|
||||
if call.has_selector or call.ai_value != "proactive":
|
||||
continue
|
||||
if call.recoverable_marker_id is None:
|
||||
continue
|
||||
if call.method not in {"click", "fill", "type"}:
|
||||
continue
|
||||
if call.method != "click" and "value" not in call.sorted_kwarg_names:
|
||||
continue
|
||||
out.append(
|
||||
RecoverableProactiveCandidate(
|
||||
method=call.method,
|
||||
lineno=call.lineno,
|
||||
marker_id=call.recoverable_marker_id,
|
||||
)
|
||||
)
|
||||
return out
|
||||
|
||||
|
||||
def validate_marker_kwarg_only_on_recoverable_proactive(code: str) -> str | None:
|
||||
"""`recoverable_marker_id` is only valid on `ai='proactive'` interaction calls
|
||||
with no `selector=`. Any other shape means the marker leaked through a rewrite
|
||||
that should have removed it (Rule 8f) — the runtime forwards unknown kwargs
|
||||
to Playwright, which can crash or silently change behavior.
|
||||
|
||||
Returns an error message or None if clean.
|
||||
"""
|
||||
issues: list[str] = []
|
||||
for call in iter_interaction_calls(code):
|
||||
if call.recoverable_marker_id is None:
|
||||
continue
|
||||
if call.ai_value == "proactive" and not call.has_selector:
|
||||
continue
|
||||
issues.append(f"page.{call.method}() on line {call.lineno}")
|
||||
|
||||
if not issues:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"`recoverable_marker_id` kwarg present on non-recoverable interaction calls: "
|
||||
f"{', '.join(issues[:5])}. The marker is valid only on `ai='proactive'` calls "
|
||||
f"WITHOUT a selector= (the SKY-9436 escape hatch). On any other shape, remove "
|
||||
f"the kwarg — Rule 8f explicitly says to drop it on upgrade to fallback+selector."
|
||||
)
|
||||
|
||||
|
||||
def validate_unmarked_proactive_unchanged(input_code: str, output_code: str) -> str | None:
|
||||
"""Hard safety check: any `ai='proactive'` call in `input_code` that lacks
|
||||
a `recoverable_marker_id` kwarg MUST be unchanged in `output_code`.
|
||||
|
||||
Identity tuple covers structure AND value semantics: method, ai value,
|
||||
selector presence, marker presence, kwarg-name set, AND the verbatim AST
|
||||
of `prompt=`, `value=`, `intention=` kwargs. Detects both structural and
|
||||
semantic mutations. Multiplicity preserved via Counter so removing one
|
||||
of N identical calls is caught.
|
||||
|
||||
Returns an error message or None if clean.
|
||||
"""
|
||||
input_calls = iter_interaction_calls(input_code)
|
||||
output_calls = iter_interaction_calls(output_code)
|
||||
|
||||
def _identity(c: InteractionCall) -> tuple:
|
||||
return (
|
||||
c.method,
|
||||
c.ai_value,
|
||||
c.has_selector,
|
||||
c.recoverable_marker_id is not None,
|
||||
c.sorted_kwarg_names,
|
||||
c.semantic_kwargs,
|
||||
)
|
||||
|
||||
unmarked_proactive = [c for c in input_calls if c.ai_value == "proactive" and c.recoverable_marker_id is None]
|
||||
output_identity_counts = Counter(_identity(c) for c in output_calls)
|
||||
|
||||
violations: list[str] = []
|
||||
for call in unmarked_proactive:
|
||||
ident = _identity(call)
|
||||
if output_identity_counts[ident] <= 0:
|
||||
violations.append(f"page.{call.method}() on line {call.lineno}")
|
||||
else:
|
||||
output_identity_counts[ident] -= 1
|
||||
|
||||
if not violations:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"Reviewer modified unmarked ai='proactive' calls: {', '.join(violations[:5])}. "
|
||||
f"Selectorless proactive calls without `recoverable_marker_id` are intentional "
|
||||
f"(essay generation, fuzzy matching, ambiguous targets) and MUST be left unchanged "
|
||||
f"— including their prompt= and value= text. Only proactive calls with "
|
||||
f"`recoverable_marker_id` are eligible for upgrade (Rule 8f)."
|
||||
)
|
||||
|
|
@ -306,6 +306,7 @@ class SkyvernPage(Page):
|
|||
prompt: str | None = None,
|
||||
ai: str | None = "fallback",
|
||||
mode: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
**kwargs: Any,
|
||||
) -> str | None:
|
||||
"""Click an element using a CSS selector, AI-powered prompt matching, or both.
|
||||
|
|
@ -410,6 +411,7 @@ class SkyvernPage(Page):
|
|||
timeout=timeout,
|
||||
failed_selector=original_selector or "",
|
||||
block_label=self.current_label,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
if error_to_raise:
|
||||
raise error_to_raise
|
||||
|
|
@ -422,6 +424,8 @@ class SkyvernPage(Page):
|
|||
intention=prompt,
|
||||
data=data,
|
||||
timeout=timeout,
|
||||
block_label=self.current_label,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
|
||||
if selector:
|
||||
|
|
@ -490,6 +494,7 @@ class SkyvernPage(Page):
|
|||
mode: str | None = None,
|
||||
totp_identifier: str | None = None,
|
||||
totp_url: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
**kwargs: Any,
|
||||
) -> str:
|
||||
"""Fill an input field using a CSS selector, AI-powered prompt matching, or both.
|
||||
|
|
@ -572,24 +577,26 @@ class SkyvernPage(Page):
|
|||
|
||||
return await self._input_text(
|
||||
selector=selector,
|
||||
value=value or "",
|
||||
value=value if value is not None else "",
|
||||
ai=ai,
|
||||
intention=prompt,
|
||||
data=data,
|
||||
timeout=timeout,
|
||||
totp_identifier=totp_identifier,
|
||||
totp_url=totp_url,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
|
||||
@action_wrap(ActionType.INPUT_TEXT)
|
||||
async def type(
|
||||
self,
|
||||
selector: str | None,
|
||||
value: str,
|
||||
selector: str | None = None,
|
||||
value: str | None = None,
|
||||
ai: str | None = "fallback",
|
||||
prompt: str | None = None,
|
||||
totp_identifier: str | None = None,
|
||||
totp_url: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
**kwargs: Any,
|
||||
) -> str:
|
||||
# Backward compatibility
|
||||
|
|
@ -605,13 +612,14 @@ class SkyvernPage(Page):
|
|||
|
||||
return await self._input_text(
|
||||
selector=selector,
|
||||
value=value,
|
||||
value=value if value is not None else "",
|
||||
ai=ai,
|
||||
intention=prompt,
|
||||
data=data,
|
||||
timeout=timeout,
|
||||
totp_identifier=totp_identifier,
|
||||
totp_url=totp_url,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
|
||||
@action_wrap(ActionType.INPUT_TEXT)
|
||||
|
|
@ -921,6 +929,7 @@ class SkyvernPage(Page):
|
|||
totp_identifier: str | None = None,
|
||||
totp_url: str | None = None,
|
||||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
recoverable_marker_id: int | None = None,
|
||||
) -> str:
|
||||
"""Input text into an element identified by ``selector``.
|
||||
|
||||
|
|
@ -984,6 +993,7 @@ class SkyvernPage(Page):
|
|||
timeout=timeout,
|
||||
failed_selector=original_selector or "",
|
||||
block_label=self.current_label,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
if error_to_raise:
|
||||
raise error_to_raise
|
||||
|
|
@ -998,6 +1008,8 @@ class SkyvernPage(Page):
|
|||
totp_identifier=totp_identifier,
|
||||
totp_url=totp_url,
|
||||
timeout=timeout,
|
||||
block_label=self.current_label,
|
||||
recoverable_marker_id=recoverable_marker_id,
|
||||
)
|
||||
|
||||
if not selector:
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ class SkyvernPageAi(Protocol):
|
|||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
failed_selector: str | None = None,
|
||||
block_label: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
) -> str | None:
|
||||
"""Click an element using AI to locate it based on intention."""
|
||||
...
|
||||
|
|
@ -37,6 +38,7 @@ class SkyvernPageAi(Protocol):
|
|||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
failed_selector: str | None = None,
|
||||
block_label: str | None = None,
|
||||
recoverable_marker_id: int | None = None,
|
||||
) -> str:
|
||||
"""Input text into an element using AI to determine the value."""
|
||||
...
|
||||
|
|
|
|||
|
|
@ -5271,12 +5271,7 @@ class ForgeAgent:
|
|||
return json_response
|
||||
|
||||
LOG.info("Need verification code")
|
||||
# 1. Check navigation payload first for inline OTP.
|
||||
otp_value = extract_totp_from_navigation_inputs(task.navigation_payload)
|
||||
# 2. Then try to generate TOTP from credential if payload has no OTP.
|
||||
if not otp_value:
|
||||
otp_value = try_generate_totp_from_credential(task.workflow_run_id)
|
||||
# 3. Lastly, poll for OTP if organization has config and no OTP was found yet.
|
||||
if not otp_value and (task.totp_verification_url or task.totp_identifier) and task.organization_id:
|
||||
workflow_id = workflow_permanent_id = None
|
||||
if task.workflow_run_id:
|
||||
|
|
@ -5293,6 +5288,8 @@ class ForgeAgent:
|
|||
totp_verification_url=task.totp_verification_url,
|
||||
totp_identifier=task.totp_identifier,
|
||||
)
|
||||
if not otp_value:
|
||||
otp_value = try_generate_totp_from_credential(task.workflow_run_id)
|
||||
|
||||
if not otp_value or otp_value.get_otp_type() != OTPType.TOTP:
|
||||
return json_response
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ You are a script reviewer for a browser automation system. Your job is to make a
|
|||
b. For form fields that need values but have NO matching parameter:
|
||||
- **Essay/freeform questions** (textarea, "Why do you want to work here?", "Describe a problem you solved"): use `ai='proactive'` with a descriptive prompt. The AI will generate a brief, professional answer using the workflow's overall context. Do NOT hardcode `'N/A'`.
|
||||
- **Short factual fields** (text inputs like "GitHub URL", "Portfolio"): use `ai='proactive'` with a prompt describing the field. The AI will provide a reasonable value or skip appropriately.
|
||||
- **Dropdowns/radios with no parameter**: use `ai='fallback'` with a prompt (existing pattern — the AI picks the best option).
|
||||
- **Dropdowns/radios with no parameter**: pair `selector=` (the dropdown/radiogroup element) with `ai='fallback'` and a descriptive `prompt=`. If you truly cannot pin down a selector, use `ai='proactive'` (omit `selector=`) instead — never combine `ai='fallback'` with a missing selector.
|
||||
c. NEVER hardcode `value='N/A'` for any field. Either use a parameter value or let the AI fill it with `ai='proactive'`.
|
||||
|
||||
## Key Difference for Extraction Blocks
|
||||
|
|
|
|||
|
|
@ -11,13 +11,14 @@ You are a script reviewer for a browser automation system. Your job is to update
|
|||
- **Don't add** a branch whose actions (fills, clicks, selectors) duplicate an existing branch — unless it has **distinct `text_patterns`** that improve page detection.
|
||||
- Branches with empty or `"N/A"` text_patterns that duplicate another branch's actions are redundant. The page is the same variant — investigate root cause (timing, page load) instead.
|
||||
- **Consolidate**: if you see multiple existing branches with identical actions and empty/`"N/A"` text_patterns, merge them into one and remove the duplicates.
|
||||
7. **SEMANTIC SELECTORS**: Use `ai='fallback'` with label-based selectors — NEVER copy xpaths from other branches. Build selectors using `:has-text()` (case-insensitive substring match). Examples: `selector='label:has-text("Full name") input'`, `selector='button:has-text("Submit")'`. If no semantic selector is possible (complex widgets), use `ai='fallback'` with only a `prompt=`.
|
||||
7. **SEMANTIC SELECTORS**: A `selector=` argument is REQUIRED on every `page.click()`/`page.fill()`/`page.type()`/`page.select_option()` call that uses `ai='fallback'`. Build selectors using label-based matching with `:has-text()` (case-insensitive substring match). Examples: `selector='label:has-text("Full name") input'`, `selector='button:has-text("Submit")'`. NEVER copy xpaths from other branches. **Do NOT use overly broad selectors** like `'button, a, [role="button"]'` — first-match wins at runtime, so a broad selector will silently click the wrong element. If no specific semantic selector is possible (complex widgets, no stable text/aria/name signal), use `ai='proactive'` with `prompt=` and **omit** `selector=` entirely. `ai='proactive'` always invokes the LLM, which is acceptable for the rare case where no selector is feasible.
|
||||
8. **SELECTOR MANAGEMENT**:
|
||||
a. **Accumulate across variants**: When different page variants use different labels for the same field, use comma-separated CSS selectors: `selector='label:has-text("Website") input, label:has-text("URL") input'`. The first match wins.
|
||||
b. **Replace failed selectors**: When an episode reports a `failed_selector` (the selector that was tried and did NOT match), REMOVE that specific selector from the list and replace it with a working alternative derived from the episode's `agent_actions` data (look for `css_suggestion`, `all_attributes`, `element_id`, `element_tag`). Do NOT keep selectors that are known to fail — they add timeout delays on every run.
|
||||
c. **Broaden element types**: If a `button:has-text("X")` selector fails, the element may not be a `<button>`. Broaden to include other clickable elements: `button:has-text("X"), input[type="submit"], a:has-text("X"), [role="button"]:has-text("X")`.
|
||||
d. When the episode has NO `agent_actions` data (only an error message like "AI click failed"), derive the selector from the **Page Text at failure** section — look for the button/link text mentioned in the error and build a broader selector.
|
||||
e. **Same selector succeeds in agent but fails in script**: If the agent actions show the SAME selector succeeding that the script failed on, consider what might be different. Look at the agent actions BEFORE the successful one — did the agent perform additional steps first (dismissing a popup, waiting for a page transition, filling a prerequisite field)? If so, add those steps to the script before the failing action. If no additional steps are apparent, it may be a timing/page-load issue — try adding `await page.wait(seconds=3)` before the action to give the page time to render.
|
||||
f. **PROACTIVE RECOVERY** (only triggers on calls with a `recoverable_marker_id=<n>` kwarg): when you see a marked `ai='proactive'` interaction call (no `selector=`) AND an episode below has `agent_actions.recoverable_marker_id` matching that integer, derive a stable selector from the episode's `agent_actions` (preference: `css_suggestion` → `all_attributes[aria-label]` → `all_attributes[placeholder]` → `all_attributes[name]` → `element_tag` + `:has-text()`). Rewrite the call to `selector='...', ai='fallback', prompt='...'` AND **remove the `recoverable_marker_id=` kwarg** as part of the rewrite. Per-method preconditions: `click` is always safe; `fill`/`type` only upgrade when the call already has a non-empty `value=`. **NEVER modify a proactive call that lacks the marker** — those are intentional always-LLM patterns (essay generation, fuzzy matching, ambiguous targets) and a hard validator will reject any rewrite that touches them.
|
||||
9. **PARAMETER NAMES — CRITICAL**:
|
||||
a. The **Workflow Parameter Keys** section below lists EVERY valid parameter name. `context.parameters['key']` is ONLY allowed when `key` appears in that list.
|
||||
b. **NEVER INVENT parameter names.** If a form field does not have a matching parameter key, do NOT create a `context.parameters['made_up_name']` reference. This will crash at runtime with a KeyError.
|
||||
|
|
@ -25,7 +26,7 @@ You are a script reviewer for a browser automation system. Your job is to update
|
|||
- **If the navigation goal describes a deterministic condition** (threshold, status check, string match, comparison) that decides the value → write Python code to compute it (see "Deterministic Logic" section below). This is FREE — no LLM call.
|
||||
- **Essay/freeform questions** (textarea, "Why do you want to work here?"): use `ai='proactive'` with a descriptive prompt. The AI generates an answer from the workflow's overall context at runtime.
|
||||
- **Short factual fields** (text inputs like "How did you hear about us?"): use `ai='proactive'` with a prompt describing the field.
|
||||
- **Dropdowns/radios with subjective choice**: use `ai='fallback'` with a prompt (the AI picks the best option).
|
||||
- **Dropdowns/radios with subjective choice**: pair `selector=` (the dropdown/radiogroup element) with `ai='fallback'` and a descriptive `prompt=`. If you truly cannot pin down a selector, use `ai='proactive'` (omit `selector=`) — never combine `ai='fallback'` with a missing selector (Rule 7).
|
||||
d. NEVER hardcode `value='N/A'` for any field.
|
||||
e. **Wrong** (crashes — `how_heard_about_job` is not a workflow parameter):
|
||||
```python
|
||||
|
|
@ -158,7 +159,11 @@ The cached code failed and the AI agent took over. Here is what happened:
|
|||
{%- if a.page_url %}{% set ns.prev_url = a.page_url %}{% endif %}
|
||||
{% endfor %}
|
||||
{% elif episode.agent_actions is mapping and episode.agent_actions.failed_selector is defined %}
|
||||
{%- if episode.agent_actions.recoverable_marker_id is defined %}
|
||||
**Proactive-recovery episode** — `recoverable_marker_id={{ episode.agent_actions.recoverable_marker_id }}` (match this against the corresponding `recoverable_marker_id=` kwarg in the script per Rule 8f).
|
||||
{%- else %}
|
||||
**Element-level fallback** — the selector `{{ episode.agent_actions.failed_selector }}` failed.
|
||||
{%- endif %}
|
||||
- Intention: {{ episode.agent_actions.action_type or "click" }}: {{ episode.agent_actions.intention or "N/A" }}
|
||||
- AI clicked:
|
||||
{%- if episode.agent_actions.element_tag %} [tag: {{ episode.agent_actions.element_tag }}]{% endif %}
|
||||
|
|
@ -281,7 +286,7 @@ When no suggested selector is available, build one from the element's `[tag]`, `
|
|||
| `input_text: Location` (autocomplete/typeahead) | `await page.fill_autocomplete(selector='label:has-text("Location") input', value=context.parameters['current_location'], ai='fallback', prompt='Fill the location')` |
|
||||
| `select_option: Location` [tag: select] [attrs: {"name": "location"}] | `await page.select_option(selector='select[name="location"]', ai='fallback', prompt='Select the first location option')` |
|
||||
| `click: Submit` [tag: button] [text: "Submit"] | `await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='Click Submit')` |
|
||||
| `click: Gender option` (no tag/attrs) | `await page.click(ai='fallback', prompt='Select the first gender option')` |
|
||||
| `click: Gender option` (no tag/attrs) | `await page.click(ai='proactive', prompt='Select the first gender option')` — no selector available, escape hatch |
|
||||
| `complete: ...` | `await page.complete()` |
|
||||
| `terminate: ...` | `if <condition>: await page.terminate(errors=["reason"])` — see **Termination Rules** in the Lifecycle section below |
|
||||
|
||||
|
|
@ -557,7 +562,7 @@ The ONLY valid use of space-separated selectors is the `label:has-text("...") in
|
|||
Avoid overly generic selectors on content-heavy pages:
|
||||
- **Too generic**: `a:has-text("Lepton")` — may match multiple links (e.g., sidebar, footer, related articles)
|
||||
- **Better**: `#search-results a:has-text("Lepton")`, or use a more specific parent: `table a:has-text("Lepton")`
|
||||
- If you cannot determine a unique parent, use `ai='fallback'` with a descriptive `prompt=` that disambiguates (e.g., "Click the first search result link for Lepton")
|
||||
- If you cannot determine a unique parent, use `ai='proactive'` (omit `selector=`) with a descriptive `prompt=` that disambiguates (e.g., "Click the first search result link for Lepton"). Do NOT pair `ai='fallback'` with a broad/ambiguous selector — first-match would silently target the wrong element.
|
||||
|
||||
### Autocomplete / Typeahead Fields
|
||||
Use `page.fill_autocomplete()` instead of `page.fill()` for inputs where typing triggers a dropdown and the user must select an option (e.g., Google Places location autocomplete, city/address pickers, company name typeaheads). Signs of an autocomplete field:
|
||||
|
|
|
|||
|
|
@ -1148,25 +1148,17 @@ async def run_with_enforcement(
|
|||
)
|
||||
raise
|
||||
|
||||
# Inject pending screenshots as a follow-up user message because OpenAI
|
||||
# rejects images in tool messages.
|
||||
screenshot_msg = _consume_pending_screenshots(ctx)
|
||||
if screenshot_msg is not None:
|
||||
LOG.info("Injecting screenshot user message", count=len(screenshot_msg["content"]) - 1)
|
||||
current_input = (
|
||||
[screenshot_msg]
|
||||
if session is not None
|
||||
else _prune_input_list(result.to_input_list()) + [screenshot_msg]
|
||||
)
|
||||
iteration += 1
|
||||
continue
|
||||
|
||||
# The post-run screenshot drain must follow the enforcement check:
|
||||
# without a nudge, re-invoking with just the screenshot would replace
|
||||
# the agent's already-final REPLY with one synthesized from a single
|
||||
# browser frame.
|
||||
if pending_recovery_nudge is not None:
|
||||
nudge: str | None = pending_recovery_nudge
|
||||
pending_recovery_nudge = None
|
||||
else:
|
||||
nudge = _check_enforcement(ctx, result)
|
||||
if nudge is None:
|
||||
_consume_pending_screenshots(ctx)
|
||||
_maybe_raise_non_retriable_nav(ctx)
|
||||
return result
|
||||
|
||||
|
|
@ -1176,6 +1168,7 @@ async def run_with_enforcement(
|
|||
"Enforcement exhausted post-update nudges, allowing response",
|
||||
nudge_count=ctx.post_update_nudge_count,
|
||||
)
|
||||
_consume_pending_screenshots(ctx)
|
||||
_maybe_raise_non_retriable_nav(ctx)
|
||||
return result
|
||||
ctx.post_update_nudge_count += 1
|
||||
|
|
@ -1183,10 +1176,17 @@ async def run_with_enforcement(
|
|||
nudge_type = _NUDGE_TYPE_BY_MESSAGE.get(nudge, "intermediate_success")
|
||||
LOG.info("Enforcement nudge", nudge_type=nudge_type, iteration=iteration)
|
||||
|
||||
# OpenAI rejects images in tool messages, so a queued post-run
|
||||
# screenshot rides as its own user message just before the nudge.
|
||||
screenshot_msg = _consume_pending_screenshots(ctx)
|
||||
if screenshot_msg is not None:
|
||||
LOG.info("Injecting screenshot user message", count=len(screenshot_msg["content"]) - 1)
|
||||
|
||||
with copilot_span("enforcement_nudge", data={"nudge_type": nudge_type, "iteration": iteration}):
|
||||
nudge_msg = {"role": "user", "content": NUDGE_SENTINEL + nudge}
|
||||
extra_msgs = [nudge_msg] if screenshot_msg is None else [screenshot_msg, nudge_msg]
|
||||
current_input = (
|
||||
[nudge_msg] if session is not None else _prune_input_list(result.to_input_list()) + [nudge_msg]
|
||||
extra_msgs if session is not None else _prune_input_list(result.to_input_list()) + extra_msgs
|
||||
)
|
||||
# Signal the narrator that the agent is re-entering the loop after an
|
||||
# enforcement correction. stream_to_sse creates the state on the first
|
||||
|
|
|
|||
|
|
@ -1,13 +1,24 @@
|
|||
"""Shared loop detection utilities for copilot tool dispatch.
|
||||
|
||||
Detects consecutive same-tool streaks (e.g., A-A-A). Does not detect
|
||||
oscillating patterns (e.g., A-B-A-B) — those are left for higher-layer
|
||||
enforcement to catch.
|
||||
Two independent guards:
|
||||
|
||||
* ``detect_tool_loop`` fires on strictly consecutive same-tool streaks
|
||||
(A-A-A). Resets the moment the tool name changes, so oscillating
|
||||
patterns (A-B-A-B) bypass it by design.
|
||||
* ``detect_failed_tool_step_loop`` fires on N repeated failures of the
|
||||
same (tool, args) pair, even when other tools dispatch in between.
|
||||
A successful invocation of the same step resets its counter.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
import json
|
||||
from collections.abc import Iterable, Mapping, MutableMapping
|
||||
from typing import Any
|
||||
|
||||
MAX_CONSECUTIVE_SAME_TOOL = 3
|
||||
MAX_REPEATED_FAILED_STEP = 3
|
||||
LOOP_DETECTED_MARKER = "LOOP DETECTED:"
|
||||
|
||||
|
||||
|
|
@ -33,3 +44,108 @@ def detect_tool_loop(
|
|||
tracker.append(tool_name)
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def _normalize_step_argument(value: Any) -> Any:
|
||||
if isinstance(value, Mapping):
|
||||
return {str(k): _normalize_step_argument(v) for k, v in sorted(value.items(), key=lambda item: str(item[0]))}
|
||||
if isinstance(value, list | tuple):
|
||||
return [_normalize_step_argument(item) for item in value]
|
||||
if isinstance(value, frozenset | set):
|
||||
return sorted((_normalize_step_argument(item) for item in value), key=repr)
|
||||
if isinstance(value, str | int | float | bool) or value is None:
|
||||
return value
|
||||
return repr(value)
|
||||
|
||||
|
||||
def tool_step_identity(tool_name: str, arguments: Mapping[str, Any] | None = None) -> str:
|
||||
normalized = _normalize_step_argument(arguments or {})
|
||||
payload = json.dumps(normalized, sort_keys=True, separators=(",", ":"))
|
||||
digest = hashlib.sha256(payload.encode("utf-8")).hexdigest()
|
||||
return f"{tool_name}:{digest}"
|
||||
|
||||
|
||||
def detect_failed_tool_step_loop(
|
||||
tracker: MutableMapping[str, int],
|
||||
tool_name: str,
|
||||
arguments: Mapping[str, Any] | None = None,
|
||||
threshold: int = MAX_REPEATED_FAILED_STEP,
|
||||
) -> str | None:
|
||||
if not tracker:
|
||||
return None
|
||||
|
||||
identity = tool_step_identity(tool_name, arguments)
|
||||
failure_count = tracker.get(identity, 0)
|
||||
next_attempt = failure_count + 1
|
||||
if next_attempt < threshold:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"{LOOP_DETECTED_MARKER} '{tool_name}' has already failed "
|
||||
f"{failure_count} consecutive times with these arguments; "
|
||||
f"blocking attempt #{next_attempt}. "
|
||||
"Use different arguments, a DIFFERENT tool, ask the user, "
|
||||
"or produce your final JSON response."
|
||||
)
|
||||
|
||||
|
||||
def record_tool_step_result(
|
||||
tracker: MutableMapping[str, int],
|
||||
tool_name: str,
|
||||
arguments: Mapping[str, Any] | None,
|
||||
result: Mapping[str, Any],
|
||||
threshold: int = MAX_REPEATED_FAILED_STEP,
|
||||
) -> None:
|
||||
identity = tool_step_identity(tool_name, arguments)
|
||||
if result.get("ok", True):
|
||||
tracker.pop(identity, None)
|
||||
return
|
||||
|
||||
tracker[identity] = min(tracker.get(identity, 0) + 1, threshold)
|
||||
|
||||
|
||||
def clear_failed_step_tracker_for_tools(
|
||||
tracker: MutableMapping[str, int],
|
||||
tool_names: Iterable[str],
|
||||
) -> None:
|
||||
prefixes = tuple(f"{name}:" for name in tool_names)
|
||||
if not prefixes:
|
||||
return
|
||||
for key in list(tracker):
|
||||
if key.startswith(prefixes):
|
||||
del tracker[key]
|
||||
|
||||
|
||||
def _ctx_failed_step_tracker(ctx: Any) -> MutableMapping[str, int] | None:
|
||||
tracker = getattr(ctx, "failed_tool_step_tracker", None)
|
||||
return tracker if isinstance(tracker, dict) else None
|
||||
|
||||
|
||||
def detect_failed_tool_step_loop_for_ctx(
|
||||
ctx: Any,
|
||||
tool_name: str,
|
||||
arguments: Mapping[str, Any] | None = None,
|
||||
) -> str | None:
|
||||
tracker = _ctx_failed_step_tracker(ctx)
|
||||
if tracker is None:
|
||||
return None
|
||||
return detect_failed_tool_step_loop(tracker, tool_name, arguments)
|
||||
|
||||
|
||||
def record_tool_step_result_for_ctx(
|
||||
ctx: Any,
|
||||
tool_name: str,
|
||||
arguments: Mapping[str, Any] | None,
|
||||
result: Mapping[str, Any],
|
||||
) -> None:
|
||||
tracker = _ctx_failed_step_tracker(ctx)
|
||||
if tracker is None:
|
||||
return
|
||||
record_tool_step_result(tracker, tool_name, arguments, result)
|
||||
|
||||
|
||||
def clear_failed_step_tracker_for_tools_in_ctx(ctx: Any, tool_names: Iterable[str]) -> None:
|
||||
tracker = _ctx_failed_step_tracker(ctx)
|
||||
if tracker is None:
|
||||
return
|
||||
clear_failed_step_tracker_for_tools(tracker, tool_names)
|
||||
|
|
|
|||
|
|
@ -20,7 +20,11 @@ from mcp.types import (
|
|||
TextContent,
|
||||
)
|
||||
|
||||
from skyvern.forge.sdk.copilot.loop_detection import detect_tool_loop
|
||||
from skyvern.forge.sdk.copilot.loop_detection import (
|
||||
detect_failed_tool_step_loop_for_ctx,
|
||||
detect_tool_loop,
|
||||
record_tool_step_result_for_ctx,
|
||||
)
|
||||
from skyvern.forge.sdk.copilot.output_utils import sanitize_tool_result_for_llm
|
||||
from skyvern.forge.sdk.copilot.runtime import (
|
||||
AgentContext,
|
||||
|
|
@ -194,6 +198,14 @@ class SkyvernOverlayMCPServer(MCPServer):
|
|||
copilot_ctx = self._context_provider()
|
||||
overlay = self._overlays.get(tool_name, SchemaOverlay())
|
||||
|
||||
loop_error = detect_failed_tool_step_loop_for_ctx(copilot_ctx, tool_name, arguments)
|
||||
if loop_error:
|
||||
LOG.warning(
|
||||
"Failed tool step loop detected, skipping execution",
|
||||
tool_name=tool_name,
|
||||
)
|
||||
return _copilot_to_call_tool_result({"ok": False, "error": loop_error})
|
||||
|
||||
tracker = getattr(copilot_ctx, "consecutive_tool_tracker", None)
|
||||
loop_error = detect_tool_loop(tracker, tool_name) if isinstance(tracker, list) else None
|
||||
if loop_error:
|
||||
|
|
@ -219,6 +231,7 @@ class SkyvernOverlayMCPServer(MCPServer):
|
|||
if overlay.pre_hook:
|
||||
hook_result = await overlay.pre_hook(arguments, copilot_ctx)
|
||||
if hook_result is not None:
|
||||
record_tool_step_result_for_ctx(copilot_ctx, tool_name, arguments, hook_result)
|
||||
return _copilot_to_call_tool_result(hook_result)
|
||||
|
||||
mcp_name = self._alias_map.get(tool_name, tool_name)
|
||||
|
|
@ -227,6 +240,7 @@ class SkyvernOverlayMCPServer(MCPServer):
|
|||
if overlay.requires_browser:
|
||||
err = await ensure_browser_session(copilot_ctx)
|
||||
if err:
|
||||
record_tool_step_result_for_ctx(copilot_ctx, tool_name, arguments, err)
|
||||
return _copilot_to_call_tool_result(err)
|
||||
mcp_args["session_id"] = copilot_ctx.browser_session_id
|
||||
|
||||
|
|
@ -244,7 +258,9 @@ class SkyvernOverlayMCPServer(MCPServer):
|
|||
error=str(e),
|
||||
exc_info=True,
|
||||
)
|
||||
return _copilot_to_call_tool_result({"ok": False, "error": f"{tool_name} failed: {e}"})
|
||||
err = {"ok": False, "error": f"{tool_name} failed: {e}"}
|
||||
record_tool_step_result_for_ctx(copilot_ctx, tool_name, arguments, err)
|
||||
return _copilot_to_call_tool_result(err)
|
||||
|
||||
# Copy fastmcp's structured_content so mutations below stay local to
|
||||
# this call — the client may reuse or cache the response object.
|
||||
|
|
@ -261,6 +277,7 @@ class SkyvernOverlayMCPServer(MCPServer):
|
|||
if overlay.post_hook:
|
||||
copilot_result = await overlay.post_hook(copilot_result, raw_mcp, copilot_ctx)
|
||||
|
||||
record_tool_step_result_for_ctx(copilot_ctx, tool_name, arguments, copilot_result)
|
||||
enqueue_screenshot_from_result(copilot_ctx, copilot_result)
|
||||
return _copilot_to_call_tool_result(copilot_result)
|
||||
|
||||
|
|
|
|||
|
|
@ -51,6 +51,7 @@ class AgentContext:
|
|||
supports_vision: bool = True
|
||||
pending_screenshots: list[ScreenshotEntry] = field(default_factory=list)
|
||||
tool_activity: list[dict[str, Any]] = field(default_factory=list)
|
||||
failed_tool_step_tracker: dict[str, int] = field(default_factory=dict)
|
||||
|
||||
# Cross-turn agent state accumulated by tools.py as the agent runs.
|
||||
# Read back by failure_tracking / loop_detection to detect stuck loops,
|
||||
|
|
|
|||
|
|
@ -29,7 +29,12 @@ from skyvern.forge.sdk.copilot.failure_tracking import (
|
|||
compute_action_sequence_fingerprint,
|
||||
update_repeated_failure_state,
|
||||
)
|
||||
from skyvern.forge.sdk.copilot.loop_detection import detect_tool_loop
|
||||
from skyvern.forge.sdk.copilot.loop_detection import (
|
||||
clear_failed_step_tracker_for_tools_in_ctx,
|
||||
detect_failed_tool_step_loop_for_ctx,
|
||||
detect_tool_loop,
|
||||
record_tool_step_result_for_ctx,
|
||||
)
|
||||
from skyvern.forge.sdk.copilot.mcp_adapter import SchemaOverlay
|
||||
from skyvern.forge.sdk.copilot.narration import NarratorState
|
||||
from skyvern.forge.sdk.copilot.narration import handler_available as narration_handler_available
|
||||
|
|
@ -375,10 +380,14 @@ async def _attach_failed_block_screenshots(
|
|||
BLOCK_RUNNING_TOOLS = frozenset({"run_blocks_and_collect_debug", "update_and_run_blocks"})
|
||||
|
||||
|
||||
def _tool_loop_error(ctx: AgentContext, tool_name: str) -> str | None:
|
||||
# The name-only guard false-positives on the intended iterative build
|
||||
# (one new block per update_and_run_blocks). Block-running tools rely
|
||||
# on the progress-aware checks below instead.
|
||||
def _tool_loop_error(ctx: AgentContext, tool_name: str, arguments: dict[str, Any] | None = None) -> str | None:
|
||||
detected = detect_failed_tool_step_loop_for_ctx(ctx, tool_name, arguments or {})
|
||||
if detected is not None:
|
||||
return detected
|
||||
|
||||
# Consecutive same-name guard: false-positives on the intended iterative
|
||||
# build (one new block per update_and_run_blocks). Block-running tools
|
||||
# rely on the progress-aware checks below instead.
|
||||
tracker = getattr(ctx, "consecutive_tool_tracker", None)
|
||||
if isinstance(tracker, list) and tool_name not in BLOCK_RUNNING_TOOLS:
|
||||
detected = detect_tool_loop(tracker, tool_name)
|
||||
|
|
@ -2311,6 +2320,11 @@ def _record_workflow_update_result(copilot_ctx: Any, result: dict[str, Any]) ->
|
|||
copilot_ctx.non_retriable_nav_error_last_emitted_signature = None
|
||||
copilot_ctx.workflow_persisted = True
|
||||
|
||||
# Block-running failures keyed off (labels, parameters) go stale once the
|
||||
# workflow itself changes — without this clear, a user who fixes the bug
|
||||
# via update_workflow gets a LOOP DETECTED on the next legitimate run.
|
||||
clear_failed_step_tracker_for_tools_in_ctx(copilot_ctx, BLOCK_RUNNING_TOOLS)
|
||||
|
||||
|
||||
def _analyze_run_blocks(result: dict[str, Any]) -> tuple[str | None, bool, list[dict] | None]:
|
||||
"""Single-pass analysis of run result blocks.
|
||||
|
|
@ -2517,13 +2531,15 @@ async def update_workflow_tool(
|
|||
Returns the validated workflow or validation errors.
|
||||
"""
|
||||
copilot_ctx = ctx.context
|
||||
loop_error = _tool_loop_error(copilot_ctx, "update_workflow")
|
||||
arguments = {"workflow_yaml": workflow_yaml}
|
||||
loop_error = _tool_loop_error(copilot_ctx, "update_workflow", arguments)
|
||||
if loop_error:
|
||||
return json.dumps({"ok": False, "error": loop_error})
|
||||
|
||||
with copilot_span("update_workflow", data={"yaml_length": len(workflow_yaml)}):
|
||||
result = await _update_workflow({"workflow_yaml": workflow_yaml}, copilot_ctx)
|
||||
result = await _update_workflow(arguments, copilot_ctx)
|
||||
_record_workflow_update_result(copilot_ctx, result)
|
||||
record_tool_step_result_for_ctx(copilot_ctx, "update_workflow", arguments, result)
|
||||
sanitized = sanitize_tool_result_for_llm("update_workflow", result)
|
||||
return json.dumps(sanitized)
|
||||
|
||||
|
|
@ -2543,11 +2559,13 @@ async def list_credentials_tool(
|
|||
a credential they have already stored on a later page.
|
||||
"""
|
||||
copilot_ctx = ctx.context
|
||||
loop_error = _tool_loop_error(copilot_ctx, "list_credentials")
|
||||
arguments = {"page": page, "page_size": page_size}
|
||||
loop_error = _tool_loop_error(copilot_ctx, "list_credentials", arguments)
|
||||
if loop_error:
|
||||
return json.dumps({"ok": False, "error": loop_error})
|
||||
|
||||
result = await _list_credentials({"page": page, "page_size": page_size}, copilot_ctx)
|
||||
result = await _list_credentials(arguments, copilot_ctx)
|
||||
record_tool_step_result_for_ctx(copilot_ctx, "list_credentials", arguments, result)
|
||||
sanitized = sanitize_tool_result_for_llm("list_credentials", result)
|
||||
return json.dumps(sanitized)
|
||||
|
||||
|
|
@ -2604,7 +2622,8 @@ async def run_blocks_tool(
|
|||
HANDLING refusal rule in the system prompt.
|
||||
"""
|
||||
copilot_ctx = ctx.context
|
||||
loop_error = _tool_loop_error(copilot_ctx, "run_blocks_and_collect_debug")
|
||||
arguments = {"block_labels": block_labels, "parameters": parameters or {}}
|
||||
loop_error = _tool_loop_error(copilot_ctx, "run_blocks_and_collect_debug", arguments)
|
||||
if loop_error:
|
||||
return json.dumps({"ok": False, "error": loop_error})
|
||||
|
||||
|
|
@ -2623,13 +2642,14 @@ async def run_blocks_tool(
|
|||
),
|
||||
):
|
||||
result = await _run_blocks_and_collect_debug(
|
||||
{"block_labels": block_labels, "parameters": parameters or {}},
|
||||
arguments,
|
||||
copilot_ctx,
|
||||
labels_to_execute=labels_to_execute,
|
||||
block_outputs_to_seed=block_outputs_to_seed,
|
||||
frontier_start_label=frontier_start_label,
|
||||
)
|
||||
_record_run_blocks_result(copilot_ctx, result)
|
||||
record_tool_step_result_for_ctx(copilot_ctx, "run_blocks_and_collect_debug", arguments, result)
|
||||
enqueue_screenshot_from_result(copilot_ctx, result)
|
||||
|
||||
sanitized = sanitize_tool_result_for_llm("run_blocks_and_collect_debug", result)
|
||||
|
|
@ -2649,15 +2669,16 @@ async def get_run_results_tool(
|
|||
pass an explicit workflow_run_id from a prior tool response.
|
||||
"""
|
||||
copilot_ctx = ctx.context
|
||||
loop_error = _tool_loop_error(copilot_ctx, "get_run_results")
|
||||
if loop_error:
|
||||
return json.dumps({"ok": False, "error": loop_error})
|
||||
|
||||
params: dict[str, Any] = {}
|
||||
if workflow_run_id:
|
||||
params["workflow_run_id"] = workflow_run_id
|
||||
loop_error = _tool_loop_error(copilot_ctx, "get_run_results", params)
|
||||
if loop_error:
|
||||
return json.dumps({"ok": False, "error": loop_error})
|
||||
|
||||
result = await _get_run_results(params, copilot_ctx)
|
||||
_maybe_clear_reconciliation_flag(copilot_ctx, result)
|
||||
record_tool_step_result_for_ctx(copilot_ctx, "get_run_results", params, result)
|
||||
|
||||
sanitized = sanitize_tool_result_for_llm("get_run_results", result)
|
||||
return json.dumps(sanitized)
|
||||
|
|
@ -2690,7 +2711,8 @@ async def update_and_run_blocks_tool(
|
|||
HANDLING refusal rule in the system prompt.
|
||||
"""
|
||||
copilot_ctx = ctx.context
|
||||
loop_error = _tool_loop_error(copilot_ctx, "update_and_run_blocks")
|
||||
arguments = {"workflow_yaml": workflow_yaml, "block_labels": block_labels, "parameters": parameters or {}}
|
||||
loop_error = _tool_loop_error(copilot_ctx, "update_and_run_blocks", arguments)
|
||||
if loop_error:
|
||||
return json.dumps({"ok": False, "error": loop_error})
|
||||
|
||||
|
|
@ -2712,6 +2734,7 @@ async def update_and_run_blocks_tool(
|
|||
_record_workflow_update_result(copilot_ctx, update_result)
|
||||
|
||||
if not update_result.get("ok"):
|
||||
record_tool_step_result_for_ctx(copilot_ctx, "update_and_run_blocks", arguments, update_result)
|
||||
sanitized = sanitize_tool_result_for_llm("update_workflow", update_result)
|
||||
return json.dumps(sanitized)
|
||||
|
||||
|
|
@ -2761,6 +2784,7 @@ async def update_and_run_blocks_tool(
|
|||
frontier_start_label=frontier_start_label,
|
||||
)
|
||||
_record_run_blocks_result(copilot_ctx, run_result)
|
||||
record_tool_step_result_for_ctx(copilot_ctx, "update_and_run_blocks", arguments, run_result)
|
||||
enqueue_screenshot_from_result(copilot_ctx, run_result)
|
||||
|
||||
sanitized = sanitize_tool_result_for_llm("run_blocks_and_collect_debug", run_result)
|
||||
|
|
|
|||
|
|
@ -5,11 +5,11 @@ from skyvern.forge.sdk.schemas.organizations import Organization
|
|||
|
||||
class ScheduleLimitChecker(abc.ABC):
|
||||
@abc.abstractmethod
|
||||
async def get_schedule_limit(self, organization: Organization, workflow_permanent_id: str) -> int | None: ...
|
||||
async def get_schedule_limit(self, organization: Organization) -> int | None: ...
|
||||
|
||||
|
||||
class NoopScheduleLimitChecker(ScheduleLimitChecker):
|
||||
async def get_schedule_limit(self, organization: Organization, workflow_permanent_id: str) -> int | None:
|
||||
async def get_schedule_limit(self, organization: Organization) -> int | None:
|
||||
return None # unlimited in OSS
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -32,6 +32,7 @@ class SkyvernContext:
|
|||
navigation_goal: str | None = None
|
||||
navigation_payload: dict[str, Any] | list | str | None = None
|
||||
totp_codes: dict[str, str | None] = field(default_factory=dict)
|
||||
active_credential_parameter_key: str | None = None
|
||||
log: list[dict] = field(default_factory=list)
|
||||
hashed_href_map: dict[str, str] = field(default_factory=dict)
|
||||
refresh_working_page: bool = False
|
||||
|
|
|
|||
|
|
@ -583,9 +583,6 @@ class AgentDB(BaseAlchemyDB):
|
|||
async def restore_workflow_schedule(self, *args: Any, **kwargs: Any) -> Any:
|
||||
return await self.schedules.restore_workflow_schedule(*args, **kwargs)
|
||||
|
||||
async def count_workflow_schedules(self, *args: Any, **kwargs: Any) -> Any:
|
||||
return await self.schedules.count_workflow_schedules(*args, **kwargs)
|
||||
|
||||
async def list_organization_schedules(self, *args: Any, **kwargs: Any) -> Any:
|
||||
return await self.schedules.list_organization_schedules(*args, **kwargs)
|
||||
|
||||
|
|
|
|||
|
|
@ -3,11 +3,10 @@ class NotFoundError(Exception):
|
|||
|
||||
|
||||
class ScheduleLimitExceededError(Exception):
|
||||
"""Raised when attempting to create a schedule that would exceed the per-workflow limit."""
|
||||
"""Raised when attempting to create a schedule that would exceed the org-wide tier limit."""
|
||||
|
||||
def __init__(self, organization_id: str, workflow_permanent_id: str, current_count: int, max_allowed: int):
|
||||
def __init__(self, organization_id: str, current_count: int, max_allowed: int):
|
||||
self.organization_id = organization_id
|
||||
self.workflow_permanent_id = workflow_permanent_id
|
||||
self.current_count = current_count
|
||||
self.max_allowed = max_allowed
|
||||
super().__init__(f"Schedule limit {max_allowed} reached (current: {current_count})")
|
||||
|
|
|
|||
|
|
@ -83,16 +83,16 @@ class SchedulesRepository(BaseRepository):
|
|||
parameters: dict[str, Any] | None = None,
|
||||
name: str | None = None,
|
||||
description: str | None = None,
|
||||
) -> tuple[WorkflowSchedule, int]:
|
||||
"""Create a schedule atomically with limit enforcement.
|
||||
) -> WorkflowSchedule:
|
||||
"""Create a schedule atomically with org-wide limit enforcement.
|
||||
|
||||
On PostgreSQL, uses an advisory lock to serialize concurrent creates for
|
||||
the same workflow, preventing TOCTOU races on the schedule count.
|
||||
On PostgreSQL, uses an advisory lock keyed on the organization to
|
||||
serialize concurrent creates within the org, preventing TOCTOU races on
|
||||
the org-wide schedule count.
|
||||
|
||||
On SQLite, uses an asyncio.Lock (set on AgentDB.__init__) since SQLite
|
||||
is single-writer and has no advisory lock support.
|
||||
|
||||
Returns (created_schedule, count_before_insert).
|
||||
Raises ScheduleLimitExceededError if count >= max_schedules.
|
||||
"""
|
||||
# SQLite: serialize via Python lock (no advisory locks available).
|
||||
|
|
@ -139,10 +139,10 @@ class SchedulesRepository(BaseRepository):
|
|||
description: str | None,
|
||||
*,
|
||||
use_advisory_lock: bool,
|
||||
) -> tuple[WorkflowSchedule, int]:
|
||||
) -> WorkflowSchedule:
|
||||
async with self.Session() as session:
|
||||
if use_advisory_lock:
|
||||
lock_key = f"schedule:{organization_id}:{workflow_permanent_id}"
|
||||
lock_key = f"schedule:{organization_id}"
|
||||
await session.execute(
|
||||
text("SELECT pg_advisory_xact_lock(hashtext(:key))"),
|
||||
{"key": lock_key},
|
||||
|
|
@ -152,7 +152,6 @@ class SchedulesRepository(BaseRepository):
|
|||
await session.execute(
|
||||
select(func.count()).where(
|
||||
WorkflowScheduleModel.organization_id == organization_id,
|
||||
WorkflowScheduleModel.workflow_permanent_id == workflow_permanent_id,
|
||||
WorkflowScheduleModel.deleted_at.is_(None),
|
||||
)
|
||||
)
|
||||
|
|
@ -161,7 +160,6 @@ class SchedulesRepository(BaseRepository):
|
|||
if max_schedules is not None and count >= max_schedules:
|
||||
raise ScheduleLimitExceededError(
|
||||
organization_id=organization_id,
|
||||
workflow_permanent_id=workflow_permanent_id,
|
||||
current_count=count,
|
||||
max_allowed=max_schedules,
|
||||
)
|
||||
|
|
@ -179,7 +177,7 @@ class SchedulesRepository(BaseRepository):
|
|||
session.add(workflow_schedule)
|
||||
await session.commit()
|
||||
await session.refresh(workflow_schedule)
|
||||
return convert_to_workflow_schedule(workflow_schedule, self.debug_enabled), count
|
||||
return convert_to_workflow_schedule(workflow_schedule, self.debug_enabled)
|
||||
|
||||
@db_operation("set_backend_schedule_id")
|
||||
async def set_backend_schedule_id(
|
||||
|
|
@ -400,22 +398,6 @@ class SchedulesRepository(BaseRepository):
|
|||
await session.refresh(workflow_schedule)
|
||||
return convert_to_workflow_schedule(workflow_schedule, self.debug_enabled)
|
||||
|
||||
@db_operation("count_workflow_schedules")
|
||||
async def count_workflow_schedules(
|
||||
self,
|
||||
organization_id: str,
|
||||
workflow_permanent_id: str,
|
||||
) -> int:
|
||||
async with self.Session() as session:
|
||||
result = await session.execute(
|
||||
select(func.count()).where(
|
||||
WorkflowScheduleModel.organization_id == organization_id,
|
||||
WorkflowScheduleModel.workflow_permanent_id == workflow_permanent_id,
|
||||
WorkflowScheduleModel.deleted_at.is_(None),
|
||||
)
|
||||
)
|
||||
return result.scalar_one()
|
||||
|
||||
@db_operation("list_organization_schedules")
|
||||
async def list_organization_schedules(
|
||||
self,
|
||||
|
|
|
|||
|
|
@ -516,12 +516,18 @@ class WorkflowRunsRepository(BaseRepository):
|
|||
) -> list[dict[str, Any]]:
|
||||
async with self.Session() as session:
|
||||
effective_status = func.coalesce(WorkflowRunModel.status, TaskRunModel.status)
|
||||
# task_runs.workflow_permanent_id is unreliable on legacy workflow_run rows; the joined
|
||||
# workflow_runs row carries the canonical WPID, so coalesce both before deriving anything.
|
||||
effective_wpid = func.coalesce(
|
||||
TaskRunModel.workflow_permanent_id,
|
||||
WorkflowRunModel.workflow_permanent_id,
|
||||
)
|
||||
# True iff this row's workflow_permanent_id has no active (deleted_at IS NULL) version.
|
||||
workflow_deleted_expr = and_(
|
||||
TaskRunModel.workflow_permanent_id.isnot(None),
|
||||
effective_wpid.isnot(None),
|
||||
~exists().where(
|
||||
and_(
|
||||
WorkflowModel.workflow_permanent_id == TaskRunModel.workflow_permanent_id,
|
||||
WorkflowModel.workflow_permanent_id == effective_wpid,
|
||||
WorkflowModel.organization_id == TaskRunModel.organization_id,
|
||||
WorkflowModel.deleted_at.is_(None),
|
||||
)
|
||||
|
|
@ -537,7 +543,7 @@ class WorkflowRunsRepository(BaseRepository):
|
|||
TaskRunModel.started_at.label("started_at"),
|
||||
TaskRunModel.finished_at.label("finished_at"),
|
||||
TaskRunModel.created_at.label("created_at"),
|
||||
TaskRunModel.workflow_permanent_id.label("workflow_permanent_id"),
|
||||
effective_wpid.label("workflow_permanent_id"),
|
||||
TaskRunModel.script_run.label("script_run"),
|
||||
TaskRunModel.searchable_text.label("searchable_text"),
|
||||
workflow_deleted_expr,
|
||||
|
|
@ -566,7 +572,7 @@ class WorkflowRunsRepository(BaseRepository):
|
|||
or_(
|
||||
TaskRunModel.searchable_text.icontains(search_key, autoescape=True),
|
||||
TaskRunModel.run_id.icontains(search_key, autoescape=True),
|
||||
TaskRunModel.workflow_permanent_id.icontains(search_key, autoescape=True),
|
||||
effective_wpid.icontains(search_key, autoescape=True),
|
||||
)
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -246,14 +246,12 @@ async def create_workflow_schedule(
|
|||
)
|
||||
stored_parameters = _strip_none_parameters(body.parameters)
|
||||
|
||||
# Atomic limit check + insert
|
||||
max_schedules = await ScheduleLimitCheckerFactory.get_instance().get_schedule_limit(
|
||||
organization,
|
||||
workflow_permanent_id,
|
||||
)
|
||||
|
||||
try:
|
||||
schedule, count = await app.DATABASE.schedules.create_workflow_schedule_with_limit(
|
||||
schedule = await app.DATABASE.schedules.create_workflow_schedule_with_limit(
|
||||
organization_id=organization.organization_id,
|
||||
workflow_permanent_id=workflow_permanent_id,
|
||||
max_schedules=max_schedules,
|
||||
|
|
@ -274,25 +272,9 @@ async def create_workflow_schedule(
|
|||
)
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_409_CONFLICT,
|
||||
detail=f"Maximum of {e.max_allowed} schedules per workflow reached",
|
||||
detail=f"Maximum of {e.max_allowed} schedules reached for your plan",
|
||||
) from e
|
||||
|
||||
# Auto-generate name if not provided
|
||||
if not body.name:
|
||||
auto_name = f"{workflow.title} - Schedule #{count + 1}"
|
||||
updated = await app.DATABASE.schedules.update_workflow_schedule(
|
||||
workflow_schedule_id=schedule.workflow_schedule_id,
|
||||
organization_id=organization.organization_id,
|
||||
cron_expression=body.cron_expression,
|
||||
timezone=body.timezone,
|
||||
enabled=body.enabled,
|
||||
parameters=stored_parameters,
|
||||
name=auto_name,
|
||||
description=body.description,
|
||||
)
|
||||
if updated:
|
||||
schedule = updated
|
||||
|
||||
backend_schedule_id = app.AGENT_FUNCTION.build_workflow_schedule_id(schedule.workflow_schedule_id)
|
||||
# The 501 guard above ensures this route only runs when schedules are enabled,
|
||||
# in which case a conforming AgentFunction override must return a non-None id.
|
||||
|
|
|
|||
|
|
@ -66,6 +66,15 @@ jinja_sandbox_env = SandboxedEnvironment()
|
|||
|
||||
RANDOM_SECRET_ID_PREFIX = "placeholder_"
|
||||
|
||||
_CREDENTIAL_PARAMETER_TYPES: tuple[type, ...] = (
|
||||
AzureVaultCredentialParameter,
|
||||
BitwardenCreditCardDataParameter,
|
||||
BitwardenLoginCredentialParameter,
|
||||
BitwardenSensitiveInformationParameter,
|
||||
CredentialParameter,
|
||||
OnePasswordCredentialParameter,
|
||||
)
|
||||
|
||||
# 1Password's Python SDK forwards generic 5xx upstream failures as plain Exceptions
|
||||
# whose stringified message embeds the HTTP status.
|
||||
_ONEPASSWORD_5XX_PATTERN = re.compile(
|
||||
|
|
@ -569,8 +578,8 @@ class WorkflowRunContext:
|
|||
}
|
||||
credential_dict: dict[str, str | None] = credential.model_dump()
|
||||
for key, value in credential_dict.items():
|
||||
# Exclude totp_type from navigation payload as it's metadata, not input data
|
||||
if key == "totp_type":
|
||||
# totp_type is metadata; totp is registered as a TOTP seed below, not as a plain field.
|
||||
if key in ("totp_type", "totp"):
|
||||
continue
|
||||
if not value:
|
||||
continue
|
||||
|
|
@ -1351,6 +1360,18 @@ class WorkflowRunContext:
|
|||
def totp_secret_value_key(self, totp_secret_id: str) -> str:
|
||||
return f"{totp_secret_id}_value"
|
||||
|
||||
def find_credential_parameter_key_for_secret(self, secret_id: str) -> str | None:
|
||||
for parameter_key, value in self.values.items():
|
||||
if not isinstance(value, dict):
|
||||
continue
|
||||
parameter = self.parameters.get(parameter_key)
|
||||
if not isinstance(parameter, _CREDENTIAL_PARAMETER_TYPES):
|
||||
continue
|
||||
for field_value in value.values():
|
||||
if field_value == secret_id:
|
||||
return parameter_key
|
||||
return None
|
||||
|
||||
def _resolve_required_parameter_value(self, parameter_value: str | None, name: str) -> str:
|
||||
result = self._resolve_parameter_value(parameter_value)
|
||||
if not result:
|
||||
|
|
|
|||
|
|
@ -5610,32 +5610,37 @@ class WorkflowService:
|
|||
task_block = task_id_to_block[action.task_id]
|
||||
task_block.actions.append(action)
|
||||
|
||||
result = []
|
||||
block_map: dict[str, WorkflowRunTimeline] = {}
|
||||
counter = 0
|
||||
while workflow_run_blocks:
|
||||
counter += 1
|
||||
block = workflow_run_blocks.pop(0)
|
||||
workflow_run_timeline = WorkflowRunTimeline(
|
||||
for block in workflow_run_blocks:
|
||||
if block.workflow_run_block_id in block_map:
|
||||
LOG.warning(
|
||||
"Duplicate workflow_run_block_id in timeline; later occurrence wins",
|
||||
workflow_run_id=workflow_run_id,
|
||||
workflow_run_block_id=block.workflow_run_block_id,
|
||||
)
|
||||
block_map[block.workflow_run_block_id] = WorkflowRunTimeline(
|
||||
type=WorkflowRunTimelineType.block,
|
||||
block=block,
|
||||
created_at=block.created_at,
|
||||
modified_at=block.modified_at,
|
||||
)
|
||||
if block.parent_workflow_run_block_id:
|
||||
if block.parent_workflow_run_block_id in block_map:
|
||||
block_map[block.parent_workflow_run_block_id].children.append(workflow_run_timeline)
|
||||
block_map[block.workflow_run_block_id] = workflow_run_timeline
|
||||
else:
|
||||
# put the block back to the queue
|
||||
workflow_run_blocks.append(block)
|
||||
else:
|
||||
result.append(workflow_run_timeline)
|
||||
block_map[block.workflow_run_block_id] = workflow_run_timeline
|
||||
|
||||
if counter > 1000:
|
||||
LOG.error("Too many blocks in the workflow run", workflow_run_id=workflow_run_id)
|
||||
break
|
||||
result: list[WorkflowRunTimeline] = []
|
||||
for timeline in block_map.values():
|
||||
if timeline.block is None:
|
||||
continue
|
||||
parent_id = timeline.block.parent_workflow_run_block_id
|
||||
if parent_id and parent_id in block_map:
|
||||
block_map[parent_id].children.append(timeline)
|
||||
continue
|
||||
if parent_id:
|
||||
LOG.warning(
|
||||
"Workflow run block references missing parent; surfacing as root",
|
||||
workflow_run_id=workflow_run_id,
|
||||
workflow_run_block_id=timeline.block.workflow_run_block_id,
|
||||
parent_workflow_run_block_id=parent_id,
|
||||
)
|
||||
result.append(timeline)
|
||||
|
||||
return result
|
||||
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ class SdkSkyvernPageAi(SkyvernPageAi):
|
|||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
failed_selector: str | None = None, # noqa: ARG002 — accepted for Protocol compat, no episode recording in library path
|
||||
block_label: str | None = None, # noqa: ARG002
|
||||
recoverable_marker_id: int | None = None, # noqa: ARG002 — Protocol compat
|
||||
) -> str | None:
|
||||
"""Click an element using AI via API call.
|
||||
|
||||
|
|
@ -79,6 +80,7 @@ class SdkSkyvernPageAi(SkyvernPageAi):
|
|||
timeout: float = settings.BROWSER_ACTION_TIMEOUT_MS,
|
||||
failed_selector: str | None = None, # noqa: ARG002 — Protocol compat, see ai_click docstring
|
||||
block_label: str | None = None, # noqa: ARG002
|
||||
recoverable_marker_id: int | None = None, # noqa: ARG002 — Protocol compat
|
||||
) -> str:
|
||||
"""Input text into an element using AI via API call."""
|
||||
|
||||
|
|
|
|||
|
|
@ -1,15 +1,20 @@
|
|||
import asyncio
|
||||
import re
|
||||
from datetime import datetime, timedelta
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
import pyotp
|
||||
import structlog
|
||||
from pydantic import BaseModel, Field
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from skyvern.forge.sdk.workflow.context_manager import WorkflowRunContext
|
||||
|
||||
from skyvern.config import settings
|
||||
from skyvern.exceptions import FailedToGetTOTPVerificationCode, NoTOTPVerificationCodeFound
|
||||
from skyvern.forge import app
|
||||
from skyvern.forge.prompts import prompt_engine
|
||||
from skyvern.forge.sdk.core import skyvern_context
|
||||
from skyvern.forge.sdk.core.aiohttp_helper import aiohttp_post
|
||||
from skyvern.forge.sdk.core.security import generate_skyvern_webhook_signature
|
||||
from skyvern.forge.sdk.db.enums import OrganizationAuthTokenType
|
||||
|
|
@ -120,13 +125,43 @@ def extract_totp_from_navigation_inputs(navigation_payload: MFANavigationPayload
|
|||
return None
|
||||
|
||||
|
||||
def try_generate_totp_from_credential(workflow_run_id: str | None) -> OTPValue | None:
|
||||
"""Try to generate a TOTP code from a credential secret stored in the workflow run context.
|
||||
def _try_generate_totp_for_credential(
|
||||
workflow_run_context: "WorkflowRunContext",
|
||||
credential_key: str,
|
||||
workflow_run_id: str,
|
||||
) -> OTPValue | None:
|
||||
value = workflow_run_context.values.get(credential_key)
|
||||
if not isinstance(value, dict):
|
||||
return None
|
||||
totp_secret_id = value.get("totp")
|
||||
if not totp_secret_id or not isinstance(totp_secret_id, str):
|
||||
return None
|
||||
totp_secret_key = workflow_run_context.totp_secret_value_key(totp_secret_id)
|
||||
totp_secret = workflow_run_context.get_original_secret_value_or_none(totp_secret_key)
|
||||
if not totp_secret:
|
||||
return None
|
||||
try:
|
||||
code = pyotp.TOTP(totp_secret).now()
|
||||
LOG.info(
|
||||
"Generated TOTP from credential secret",
|
||||
workflow_run_id=workflow_run_id,
|
||||
credential_key=credential_key,
|
||||
)
|
||||
return OTPValue(value=code, type=OTPType.TOTP)
|
||||
except Exception:
|
||||
LOG.warning(
|
||||
"Failed to generate TOTP from credential secret",
|
||||
workflow_run_id=workflow_run_id,
|
||||
credential_key=credential_key,
|
||||
exc_info=True,
|
||||
)
|
||||
return None
|
||||
|
||||
Scans workflow_run_context.values for credential entries with a "totp" key
|
||||
(e.g. Bitwarden, 1Password, Azure Key Vault credentials) and generates a
|
||||
TOTP code using pyotp. This should be checked BEFORE poll_otp_value so that
|
||||
credential-based TOTP takes priority over webhook (totp_url) and totp_identifier.
|
||||
|
||||
def try_generate_totp_from_credential(workflow_run_id: str | None) -> OTPValue | None:
|
||||
"""Generate a TOTP only for the credential the agent is currently typing into.
|
||||
|
||||
Falls back to single-credential heuristic when no active credential is recorded.
|
||||
"""
|
||||
if not workflow_run_id:
|
||||
return None
|
||||
|
|
@ -135,30 +170,26 @@ def try_generate_totp_from_credential(workflow_run_id: str | None) -> OTPValue |
|
|||
if not workflow_run_context:
|
||||
return None
|
||||
|
||||
for key, value in workflow_run_context.values.items():
|
||||
if isinstance(value, dict) and "totp" in value:
|
||||
totp_secret_id = value.get("totp")
|
||||
if not totp_secret_id or not isinstance(totp_secret_id, str):
|
||||
continue
|
||||
totp_secret_key = workflow_run_context.totp_secret_value_key(totp_secret_id)
|
||||
totp_secret = workflow_run_context.get_original_secret_value_or_none(totp_secret_key)
|
||||
if totp_secret:
|
||||
try:
|
||||
code = pyotp.TOTP(totp_secret).now()
|
||||
LOG.info(
|
||||
"Generated TOTP from credential secret",
|
||||
workflow_run_id=workflow_run_id,
|
||||
credential_key=key,
|
||||
)
|
||||
return OTPValue(value=code, type=OTPType.TOTP)
|
||||
except Exception:
|
||||
LOG.warning(
|
||||
"Failed to generate TOTP from credential secret",
|
||||
workflow_run_id=workflow_run_id,
|
||||
credential_key=key,
|
||||
exc_info=True,
|
||||
)
|
||||
return None
|
||||
current_context = skyvern_context.current()
|
||||
active_credential_key = current_context.active_credential_parameter_key if current_context else None
|
||||
|
||||
if active_credential_key:
|
||||
return _try_generate_totp_for_credential(workflow_run_context, active_credential_key, workflow_run_id)
|
||||
|
||||
candidate_keys = [
|
||||
key
|
||||
for key, value in workflow_run_context.values.items()
|
||||
if isinstance(value, dict) and isinstance(value.get("totp"), str)
|
||||
]
|
||||
if len(candidate_keys) != 1:
|
||||
if len(candidate_keys) > 1:
|
||||
LOG.info(
|
||||
"Skipping credential-TOTP: multiple credentials with TOTP and no active credential",
|
||||
workflow_run_id=workflow_run_id,
|
||||
candidate_credential_keys=candidate_keys,
|
||||
)
|
||||
return None
|
||||
return _try_generate_totp_for_credential(workflow_run_context, candidate_keys[0], workflow_run_id)
|
||||
|
||||
|
||||
async def poll_otp_value(
|
||||
|
|
|
|||
|
|
@ -14,6 +14,15 @@ from skyvern.core.script_generations.generate_script import (
|
|||
MAX_PARAM_VALUE_LENGTH_FOR_PROMPT_SUB,
|
||||
MIN_PARAM_VALUE_LENGTH_FOR_PROMPT_SUB,
|
||||
)
|
||||
from skyvern.core.script_generations.script_validators import (
|
||||
INTERACTION_METHODS,
|
||||
PAGE_CALL_RE,
|
||||
find_recoverable_proactive_candidates,
|
||||
validate_marker_kwarg_only_on_recoverable_proactive,
|
||||
validate_missing_selectors,
|
||||
validate_proactive_misuse,
|
||||
validate_unmarked_proactive_unchanged,
|
||||
)
|
||||
from skyvern.forge import app
|
||||
from skyvern.forge.prompts import prompt_engine
|
||||
from skyvern.forge.sdk.workflow.models.block import get_all_blocks
|
||||
|
|
@ -168,6 +177,348 @@ _ALLOWED_PAGE_API: frozenset[str] = frozenset(
|
|||
)
|
||||
|
||||
|
||||
class _ClassifyBranchCounter(cst.CSTVisitor):
|
||||
"""Count classify branches via literal options keys and matching if-arms.
|
||||
|
||||
Returns ``max(options_count, if_arm_count)`` so non-literal classify
|
||||
shapes don't silently underreport growth. If-arms are only counted when
|
||||
the tested name was bound from ``await page.classify(...)``.
|
||||
"""
|
||||
|
||||
def __init__(self) -> None:
|
||||
super().__init__()
|
||||
self.options_count = 0
|
||||
self.if_arm_count = 0
|
||||
# Names known to hold a classify result, e.g. ``state`` from
|
||||
# ``state = await page.classify(...)``. Populated lazily as the
|
||||
# visitor encounters the assignments. CSTVisitor walks in source
|
||||
# order, so a classify-assignment is visited before any if-arm that
|
||||
# tests its variable.
|
||||
self._classify_vars: set[str] = set()
|
||||
|
||||
@property
|
||||
def total(self) -> int:
|
||||
return max(self.options_count, self.if_arm_count)
|
||||
|
||||
def visit_Assign(self, node: cst.Assign) -> None:
|
||||
if len(node.targets) != 1:
|
||||
return
|
||||
target = node.targets[0].target
|
||||
if not isinstance(target, cst.Name):
|
||||
return
|
||||
if ScriptReviewer._classify_call_node(node.value) is not None:
|
||||
self._classify_vars.add(target.value)
|
||||
else:
|
||||
self._classify_vars.discard(target.value)
|
||||
|
||||
def visit_Call(self, node: cst.Call) -> None:
|
||||
if not ScriptReviewer._is_classify_call(node):
|
||||
return
|
||||
options = ScriptReviewer._dict_kwarg(node, "options")
|
||||
if options is None:
|
||||
return
|
||||
for el in options.elements:
|
||||
if isinstance(el, cst.DictElement) and ScriptReviewer._string_literal_value(el.key) is not None:
|
||||
self.options_count += 1
|
||||
|
||||
def visit_If(self, node: cst.If) -> None:
|
||||
test = node.test
|
||||
if not isinstance(test, cst.Comparison):
|
||||
return
|
||||
if not isinstance(test.left, cst.Name):
|
||||
return
|
||||
if test.left.value not in self._classify_vars:
|
||||
return
|
||||
if len(test.comparisons) != 1:
|
||||
return
|
||||
target = test.comparisons[0]
|
||||
if not isinstance(target.operator, cst.Equal):
|
||||
return
|
||||
if ScriptReviewer._string_literal_value(target.comparator) is None:
|
||||
return
|
||||
self.if_arm_count += 1
|
||||
|
||||
|
||||
class _ClassifyConsolidator(cst.CSTTransformer):
|
||||
"""Rewrite ``page.classify()`` calls + matching if-chains to merge duplicate branches.
|
||||
|
||||
Walks each function body looking for the pattern::
|
||||
|
||||
var = await page.classify(options={...}, text_patterns={...}, url_patterns={...})
|
||||
if var == "key_a":
|
||||
<body_a>
|
||||
elif var == "key_b":
|
||||
<body_b>
|
||||
...
|
||||
else:
|
||||
<fallback>
|
||||
|
||||
Within one such pair, branches whose action bodies serialize identically
|
||||
are collapsed into a single branch (preferring to retain the one with
|
||||
non-empty ``text_patterns``). The dict literals and the if-chain are
|
||||
rewritten in lockstep so the output remains a valid script.
|
||||
|
||||
Conservative bail-outs (skip consolidation for that classify call):
|
||||
- Pattern doesn't match (no var, no following if-chain, etc.)
|
||||
- Consolidation would leave zero keyed arms (would orphan the classify result)
|
||||
"""
|
||||
|
||||
def __init__(self) -> None:
|
||||
super().__init__()
|
||||
self.dropped_keys: list[str] = []
|
||||
|
||||
def leave_IndentedBlock(
|
||||
self,
|
||||
original_node: cst.IndentedBlock,
|
||||
updated_node: cst.IndentedBlock,
|
||||
) -> cst.IndentedBlock:
|
||||
new_body = self._rewrite_body(list(updated_node.body))
|
||||
if new_body is None:
|
||||
return updated_node
|
||||
return updated_node.with_changes(body=tuple(new_body))
|
||||
|
||||
def _rewrite_body(
|
||||
self,
|
||||
stmts: list[cst.BaseStatement],
|
||||
) -> list[cst.BaseStatement] | None:
|
||||
changed = False
|
||||
i = 0
|
||||
while i < len(stmts):
|
||||
assign_info = self._extract_classify_assignment(stmts[i])
|
||||
if assign_info is None:
|
||||
i += 1
|
||||
continue
|
||||
var_name, classify_call = assign_info
|
||||
# Find the if-chain following classify. Look up to
|
||||
# ``_MAX_PRE_IF_SCAN`` non-If statements ahead — matching the
|
||||
# tolerance of ``_validate_classify_handling`` so any LLM
|
||||
# output that passes validation is reachable for consolidation.
|
||||
if_stmt_idx: int | None = None
|
||||
scan_stop = min(i + 1 + ScriptReviewer._MAX_PRE_IF_SCAN + 1, len(stmts))
|
||||
for j in range(i + 1, scan_stop):
|
||||
candidate = stmts[j]
|
||||
if isinstance(candidate, cst.If):
|
||||
if_stmt_idx = j
|
||||
break
|
||||
if if_stmt_idx is None:
|
||||
i += 1
|
||||
continue
|
||||
if_stmt = stmts[if_stmt_idx]
|
||||
assert isinstance(if_stmt, cst.If)
|
||||
# Refuse to consolidate when ``options=`` is non-literal: keys
|
||||
# cannot be stripped from the classify call, so dropping their
|
||||
# if-arms would leave the keys live in the classify output with
|
||||
# no handler.
|
||||
if not isinstance(ScriptReviewer._dict_kwarg(classify_call, "options"), cst.Dict):
|
||||
i += 1
|
||||
continue
|
||||
arms = ScriptReviewer._walk_if_chain(if_stmt, var_name)
|
||||
if len(arms) < 2:
|
||||
i += 1
|
||||
continue
|
||||
# Skip if ``var_name`` is reassigned between classify and the
|
||||
# if-chain — the chain may be testing a rebound value.
|
||||
rebound = False
|
||||
for j in range(i + 1, if_stmt_idx):
|
||||
if ScriptReviewer._stmt_assigns_to(stmts[j], var_name):
|
||||
rebound = True
|
||||
break
|
||||
if rebound:
|
||||
i += 1
|
||||
continue
|
||||
options_dict = ScriptReviewer._dict_kwarg(classify_call, "options")
|
||||
assert isinstance(options_dict, cst.Dict)
|
||||
options_keys = set(ScriptReviewer._dict_keys(options_dict).keys())
|
||||
arm_keys = {key for key, _ in arms}
|
||||
if not arm_keys.issubset(options_keys):
|
||||
i += 1
|
||||
continue
|
||||
# If any arm body reads ``var_name``, textual-identity does not
|
||||
# imply runtime-identity (the matched value flows through the
|
||||
# body). Skip rather than risk semantic drift.
|
||||
if any(ScriptReviewer._body_references_name(body, var_name) for _, body in arms):
|
||||
i += 1
|
||||
continue
|
||||
text_patterns_dict = ScriptReviewer._dict_kwarg(classify_call, "text_patterns")
|
||||
url_patterns_dict = ScriptReviewer._dict_kwarg(classify_call, "url_patterns")
|
||||
text_patterns_signal = {
|
||||
k: ScriptReviewer._pattern_node_has_signal(el.value)
|
||||
for k, el in ScriptReviewer._dict_keys(text_patterns_dict).items()
|
||||
}
|
||||
url_patterns_signal = {
|
||||
k: ScriptReviewer._url_pattern_node_has_signal(el.value)
|
||||
for k, el in ScriptReviewer._dict_keys(url_patterns_dict).items()
|
||||
}
|
||||
text_dict_elements = ScriptReviewer._dict_keys(text_patterns_dict)
|
||||
url_dict_elements = ScriptReviewer._dict_keys(url_patterns_dict)
|
||||
# Two-stage rule, per Rule 6b in the reviewer prompt:
|
||||
# 1. Within a body-sig group, sub-group by exact pattern
|
||||
# values; drop within-bucket duplicates.
|
||||
# 2. Among survivors, drop no-signal duplicates of
|
||||
# signal-bearing keys; if all survivors are no-signal,
|
||||
# keep first by source order.
|
||||
groups: dict[str, list[str]] = {}
|
||||
for key, body in arms:
|
||||
sig = ScriptReviewer._body_signature(body)
|
||||
if not sig:
|
||||
continue
|
||||
groups.setdefault(sig, []).append(key)
|
||||
|
||||
def _has_pattern_signal(k: str) -> bool:
|
||||
return text_patterns_signal.get(k, False) or url_patterns_signal.get(k, False)
|
||||
|
||||
def _bucket_key(elements: dict[str, cst.DictElement], k: str, prefix: str) -> str:
|
||||
# "M" for missing entries (collapse together), "L:<v>" for
|
||||
# literals (collapse identical literals), "O:<dim>:<key>"
|
||||
# for opaque AST shapes (per-key unique — distinctness is
|
||||
# unknown, not proven absent).
|
||||
if k not in elements:
|
||||
return "M"
|
||||
repr_val = ScriptReviewer._pattern_node_repr(elements[k].value)
|
||||
if repr_val is None:
|
||||
return f"O:{prefix}:{k}"
|
||||
return f"L:{repr_val}"
|
||||
|
||||
drop_keys: set[str] = set()
|
||||
for sig_keys in groups.values():
|
||||
if len(sig_keys) < 2:
|
||||
continue
|
||||
pair_buckets: dict[tuple[str, str], list[str]] = {}
|
||||
for k in sig_keys:
|
||||
pair = (
|
||||
_bucket_key(text_dict_elements, k, "t"),
|
||||
_bucket_key(url_dict_elements, k, "u"),
|
||||
)
|
||||
pair_buckets.setdefault(pair, []).append(k)
|
||||
for bucket in pair_buckets.values():
|
||||
if len(bucket) > 1:
|
||||
drop_keys.update(bucket[1:])
|
||||
survivors = [k for k in sig_keys if k not in drop_keys]
|
||||
if len(survivors) < 2:
|
||||
continue
|
||||
signal_bearing = [k for k in survivors if _has_pattern_signal(k)]
|
||||
no_signal = [k for k in survivors if not _has_pattern_signal(k)]
|
||||
if signal_bearing and no_signal:
|
||||
drop_keys.update(no_signal)
|
||||
elif not signal_bearing:
|
||||
drop_keys.update(survivors[1:])
|
||||
|
||||
if not drop_keys:
|
||||
i += 1
|
||||
continue
|
||||
# Defense-in-depth: the picker always keeps at least one key per
|
||||
# body group, so this guard is unreachable today. Kept against
|
||||
# future refactors that might bypass keeper selection.
|
||||
remaining = len(arms) - len(drop_keys)
|
||||
if remaining < 1:
|
||||
i += 1
|
||||
continue
|
||||
new_classify = self._strip_keys_from_classify(classify_call, drop_keys)
|
||||
new_if_chain = self._filter_if_chain(if_stmt, var_name, drop_keys)
|
||||
if new_if_chain is None or not isinstance(new_if_chain, cst.If):
|
||||
i += 1
|
||||
continue
|
||||
stmts[i] = self._rewrap_classify_call(stmts[i], classify_call, new_classify)
|
||||
stmts[if_stmt_idx] = new_if_chain
|
||||
self.dropped_keys.extend(sorted(drop_keys))
|
||||
changed = True
|
||||
i = if_stmt_idx + 1
|
||||
return stmts if changed else None
|
||||
|
||||
@staticmethod
|
||||
def _extract_classify_assignment(
|
||||
stmt: cst.BaseStatement,
|
||||
) -> tuple[str, cst.Call] | None:
|
||||
"""If ``stmt`` is ``var = await page.classify(...)``, return (var_name, call)."""
|
||||
if not isinstance(stmt, cst.SimpleStatementLine):
|
||||
return None
|
||||
for small in stmt.body:
|
||||
if not isinstance(small, cst.Assign):
|
||||
continue
|
||||
if len(small.targets) != 1:
|
||||
continue
|
||||
target = small.targets[0].target
|
||||
if not isinstance(target, cst.Name):
|
||||
continue
|
||||
call = ScriptReviewer._classify_call_node(small.value)
|
||||
if call is None:
|
||||
continue
|
||||
return target.value, call
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _strip_keys_from_classify(call: cst.Call, drop: set[str]) -> cst.Call:
|
||||
"""Return a copy of the classify call with dropped keys removed from its dict kwargs."""
|
||||
new_args: list[cst.Arg] = []
|
||||
for arg in call.args:
|
||||
if (
|
||||
arg.keyword is not None
|
||||
and arg.keyword.value in {"options", "text_patterns", "url_patterns"}
|
||||
and isinstance(arg.value, cst.Dict)
|
||||
):
|
||||
new_dict = _ClassifyConsolidator._strip_keys_from_dict(arg.value, drop)
|
||||
new_args.append(arg.with_changes(value=new_dict))
|
||||
else:
|
||||
new_args.append(arg)
|
||||
return call.with_changes(args=tuple(new_args))
|
||||
|
||||
@staticmethod
|
||||
def _strip_keys_from_dict(d: cst.Dict, drop: set[str]) -> cst.Dict:
|
||||
new_elements: list[cst.BaseDictElement] = []
|
||||
for el in d.elements:
|
||||
if isinstance(el, cst.DictElement):
|
||||
key = ScriptReviewer._string_literal_value(el.key)
|
||||
if key is not None and key in drop:
|
||||
continue
|
||||
new_elements.append(el)
|
||||
return d.with_changes(elements=tuple(new_elements))
|
||||
|
||||
@staticmethod
|
||||
def _rewrap_classify_call(
|
||||
stmt: cst.BaseStatement,
|
||||
old_call: cst.Call,
|
||||
new_call: cst.Call,
|
||||
) -> cst.BaseStatement:
|
||||
"""Replace ``old_call`` with ``new_call`` inside the classify-assign statement."""
|
||||
if not isinstance(stmt, cst.SimpleStatementLine):
|
||||
return stmt
|
||||
new_body: list[cst.BaseSmallStatement] = []
|
||||
for small in stmt.body:
|
||||
if isinstance(small, cst.Assign):
|
||||
value = small.value
|
||||
if isinstance(value, cst.Await) and value.expression is old_call:
|
||||
new_body.append(small.with_changes(value=value.with_changes(expression=new_call)))
|
||||
continue
|
||||
if value is old_call:
|
||||
new_body.append(small.with_changes(value=new_call))
|
||||
continue
|
||||
new_body.append(small)
|
||||
return stmt.with_changes(body=tuple(new_body))
|
||||
|
||||
@staticmethod
|
||||
def _filter_if_chain(
|
||||
if_stmt: cst.If,
|
||||
var_name: str,
|
||||
drop: set[str],
|
||||
) -> cst.If | cst.Else | None:
|
||||
"""Recursively rebuild the if-chain with dropped arms elided.
|
||||
|
||||
Walks the chain via ``.orelse``. When an arm's test matches
|
||||
``var_name == "<dropped_key>"``, it is replaced by its tail
|
||||
(its ``.orelse``) — effectively removing the arm while keeping
|
||||
the remaining chain intact.
|
||||
"""
|
||||
key = ScriptReviewer._if_chain_test_against(if_stmt.test, var_name)
|
||||
new_orelse: cst.If | cst.Else | None
|
||||
if isinstance(if_stmt.orelse, cst.If):
|
||||
new_orelse = _ClassifyConsolidator._filter_if_chain(if_stmt.orelse, var_name, drop)
|
||||
else:
|
||||
new_orelse = if_stmt.orelse
|
||||
if key is not None and key in drop:
|
||||
return new_orelse
|
||||
return if_stmt.with_changes(orelse=new_orelse)
|
||||
|
||||
|
||||
class ScriptReviewer:
|
||||
"""Reviews fallback episodes and proposes updated cached scripts with new branches.
|
||||
|
||||
|
|
@ -725,6 +1076,20 @@ class ScriptReviewer:
|
|||
code_length=len(existing_code),
|
||||
)
|
||||
|
||||
# SKY-9436: log marker-tagged proactive calls for Datadog visibility.
|
||||
# Rule 8f instructs the LLM to upgrade these to fallback+selector when
|
||||
# matching proactive_recovery episode data is available (recording
|
||||
# path is the explicit follow-up).
|
||||
recoverable_candidates = find_recoverable_proactive_candidates(existing_code)
|
||||
if recoverable_candidates:
|
||||
LOG.info(
|
||||
"script_recoverable_proactive_detected",
|
||||
block_label=block_label,
|
||||
script_revision_id=script_revision_id,
|
||||
count=len(recoverable_candidates),
|
||||
marker_ids=[c.marker_id for c in recoverable_candidates[:10]],
|
||||
)
|
||||
|
||||
# Use provided navigation goal, or fall back to a generic description
|
||||
if not navigation_goal:
|
||||
navigation_goal = "Complete the navigation task for this block"
|
||||
|
|
@ -900,6 +1265,38 @@ class ScriptReviewer:
|
|||
current_prompt = self._build_retry_prompt(updated_code, api_error, function_signature)
|
||||
continue
|
||||
|
||||
# SKY-9436 safety: reject any rewrite that mutates an unmarked
|
||||
# ai='proactive' call (essay/fuzzy/ambiguous patterns are
|
||||
# intentional always-LLM and must be preserved).
|
||||
marker_safety_error = validate_unmarked_proactive_unchanged(existing_code, updated_code)
|
||||
if marker_safety_error is not None:
|
||||
LOG.warning(
|
||||
"safety_validator_blocked_unmarked_proactive_change",
|
||||
block_label=block_label,
|
||||
attempt=attempt,
|
||||
error=marker_safety_error,
|
||||
)
|
||||
if attempt < max_attempts:
|
||||
current_prompt = self._build_retry_prompt(updated_code, marker_safety_error, function_signature)
|
||||
continue
|
||||
|
||||
# SKY-9436: marker kwarg must be removed when call is upgraded
|
||||
# past the recoverable-proactive shape. If it leaks through, we
|
||||
# can crash at runtime when Playwright sees an unknown kwarg.
|
||||
marker_position_error = validate_marker_kwarg_only_on_recoverable_proactive(updated_code)
|
||||
if marker_position_error is not None:
|
||||
LOG.warning(
|
||||
"ScriptReviewer: stray recoverable_marker_id detected, retrying",
|
||||
block_label=block_label,
|
||||
attempt=attempt,
|
||||
error=marker_position_error,
|
||||
)
|
||||
if attempt < max_attempts:
|
||||
current_prompt = self._build_retry_prompt(
|
||||
updated_code, marker_position_error, function_signature
|
||||
)
|
||||
continue
|
||||
|
||||
# Validate method kwargs (catch invented kwargs like classify(description=...))
|
||||
kwargs_error = self._validate_method_kwargs(updated_code)
|
||||
if kwargs_error is not None:
|
||||
|
|
@ -1033,7 +1430,9 @@ class ScriptReviewer:
|
|||
current_prompt = self._build_retry_prompt(updated_code, hardcoded_error, function_signature)
|
||||
continue
|
||||
|
||||
# Validate ai='proactive' misuse (should be 'fallback' on interaction methods)
|
||||
# The validator already permits the one legitimate proactive+selector case
|
||||
# (no-value select_option with semantic selector), so any remaining flag is
|
||||
# a real caching regression — block and retry.
|
||||
proactive_error = self._validate_proactive_misuse(updated_code)
|
||||
if proactive_error is not None:
|
||||
LOG.warning(
|
||||
|
|
@ -1087,12 +1486,54 @@ class ScriptReviewer:
|
|||
current_prompt = self._build_retry_prompt(updated_code, run_data_error, function_signature)
|
||||
continue
|
||||
|
||||
# Rule 6b enforcement: merge classify branches with
|
||||
# byte-identical action bodies. Net-add growth is logged
|
||||
# for telemetry but not rejected.
|
||||
pre_consolidation_branches = self._count_classify_branches(updated_code)
|
||||
consolidated_code, dropped_keys = self._consolidate_classify_duplicates(updated_code)
|
||||
if dropped_keys:
|
||||
final_branches = self._count_classify_branches(consolidated_code)
|
||||
LOG.info(
|
||||
"ScriptReviewer: consolidated duplicate classify branches",
|
||||
block_label=block_label,
|
||||
dropped_keys=dropped_keys,
|
||||
branches_before=pre_consolidation_branches,
|
||||
branches_after=final_branches,
|
||||
)
|
||||
updated_code = consolidated_code
|
||||
else:
|
||||
final_branches = pre_consolidation_branches
|
||||
existing_branches = self._count_classify_branches(existing_code)
|
||||
if final_branches > existing_branches:
|
||||
LOG.info(
|
||||
"ScriptReviewer: classify branches net-added after consolidation",
|
||||
block_label=block_label,
|
||||
branches_before=existing_branches,
|
||||
branches_after=final_branches,
|
||||
net_added=final_branches - existing_branches,
|
||||
)
|
||||
|
||||
LOG.info(
|
||||
"ScriptReviewer: generated updated code for block",
|
||||
block_label=block_label,
|
||||
attempt=attempt,
|
||||
code_length=len(updated_code),
|
||||
)
|
||||
|
||||
# SKY-9436: emit applied event for each marker that vanished
|
||||
# from input → output (the LLM successfully upgraded it).
|
||||
input_markers = {c.marker_id for c in find_recoverable_proactive_candidates(existing_code)}
|
||||
output_markers = {c.marker_id for c in find_recoverable_proactive_candidates(updated_code)}
|
||||
applied_marker_ids = sorted(input_markers - output_markers)
|
||||
if applied_marker_ids:
|
||||
LOG.info(
|
||||
"script_proactive_recovery_applied",
|
||||
block_label=block_label,
|
||||
script_revision_id=script_revision_id,
|
||||
applied_count=len(applied_marker_ids),
|
||||
marker_ids=applied_marker_ids[:10],
|
||||
)
|
||||
|
||||
return BlockReviewResult(
|
||||
code=updated_code,
|
||||
original_prompt=reviewer_prompt,
|
||||
|
|
@ -1521,9 +1962,20 @@ class ScriptReviewer:
|
|||
# Anything not in the set triggers a retry so the LLM fixes its code.
|
||||
_METHOD_KWARGS: dict[str, frozenset[str]] = {
|
||||
"classify": frozenset({"options", "url_patterns", "text_patterns"}),
|
||||
"click": frozenset({"selector", "prompt", "ai", "intention", "data", "timeout"}),
|
||||
"click": frozenset({"selector", "prompt", "ai", "intention", "data", "timeout", "recoverable_marker_id"}),
|
||||
"fill": frozenset(
|
||||
{"selector", "value", "ai", "prompt", "intention", "data", "totp_identifier", "totp_url", "timeout"}
|
||||
{
|
||||
"selector",
|
||||
"value",
|
||||
"ai",
|
||||
"prompt",
|
||||
"intention",
|
||||
"data",
|
||||
"totp_identifier",
|
||||
"totp_url",
|
||||
"timeout",
|
||||
"recoverable_marker_id",
|
||||
}
|
||||
),
|
||||
"fill_autocomplete": frozenset(
|
||||
{
|
||||
|
|
@ -1540,6 +1992,20 @@ class ScriptReviewer:
|
|||
}
|
||||
),
|
||||
"select_option": frozenset({"selector", "value", "ai", "prompt", "intention", "data", "timeout"}),
|
||||
"type": frozenset(
|
||||
{
|
||||
"selector",
|
||||
"value",
|
||||
"ai",
|
||||
"prompt",
|
||||
"intention",
|
||||
"data",
|
||||
"totp_identifier",
|
||||
"totp_url",
|
||||
"timeout",
|
||||
"recoverable_marker_id",
|
||||
}
|
||||
),
|
||||
"extract": frozenset({"prompt", "schema", "error_code_mapping", "intention", "data"}),
|
||||
"validate": frozenset({"prompt", "model"}),
|
||||
"element_fallback": frozenset({"navigation_goal", "max_steps"}),
|
||||
|
|
@ -2174,105 +2640,16 @@ class ScriptReviewer:
|
|||
f"Replace ALL hardcoded parameter values with context.parameters['key'] references."
|
||||
)
|
||||
|
||||
# Methods whose primary purpose is interaction — ai='proactive' on these
|
||||
# defeats caching by always invoking the LLM even when the selector works.
|
||||
_INTERACTION_METHODS: frozenset[str] = frozenset({"click", "fill", "fill_autocomplete", "type", "select_option"})
|
||||
|
||||
# Regex to find page.<method>( calls
|
||||
_PAGE_CALL_RE: re.Pattern[str] = re.compile(r"""\bpage\.(\w+)\s*\(""")
|
||||
# Re-exported from the shared validator module to avoid drift between
|
||||
# generator-side and reviewer-side rules.
|
||||
_INTERACTION_METHODS: frozenset[str] = INTERACTION_METHODS
|
||||
_PAGE_CALL_RE: re.Pattern[str] = PAGE_CALL_RE
|
||||
|
||||
def _validate_proactive_misuse(self, code: str) -> str | None:
|
||||
"""Flag ai='proactive' on interaction methods (click, fill, type, select_option).
|
||||
|
||||
Using ai='proactive' means the LLM is always invoked even when the selector
|
||||
works, defeating the zero-LLM-cost goal of caching. These should almost always
|
||||
use ai='fallback' instead.
|
||||
|
||||
Returns an error message or None if no issues found.
|
||||
"""
|
||||
issues: list[str] = []
|
||||
lines = code.split("\n")
|
||||
i = 0
|
||||
while i < len(lines):
|
||||
stripped = lines[i].lstrip()
|
||||
if stripped.startswith("#"):
|
||||
i += 1
|
||||
continue
|
||||
match = self._PAGE_CALL_RE.search(lines[i])
|
||||
if match and match.group(1) in self._INTERACTION_METHODS:
|
||||
# Gather the full call (may span multiple lines)
|
||||
call_text = lines[i]
|
||||
end_line = self._find_call_end(lines, i)
|
||||
if end_line > i:
|
||||
call_text = "\n".join(lines[i : end_line + 1])
|
||||
if re.search(r"""\bai\s*=\s*['"]proactive['"]""", call_text):
|
||||
issues.append(f"page.{match.group(1)}() on line {i + 1}")
|
||||
i = end_line + 1
|
||||
else:
|
||||
i += 1
|
||||
|
||||
if not issues:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"ai='proactive' used on interaction methods: {', '.join(issues[:5])}. "
|
||||
f"Using ai='proactive' on interaction methods ({'/'.join(sorted(self._INTERACTION_METHODS))}) means the LLM is "
|
||||
f"ALWAYS invoked even when the selector works, defeating the zero-LLM-cost "
|
||||
f"goal of caching. Change to ai='fallback' — this tries the selector first "
|
||||
f"and only invokes the LLM if the selector fails."
|
||||
)
|
||||
return validate_proactive_misuse(code)
|
||||
|
||||
def _validate_missing_selectors(self, code: str) -> str | None:
|
||||
"""Flag interaction methods that lack a selector= argument.
|
||||
|
||||
Two cases are flagged:
|
||||
1. ai='fallback' but no selector — the CSS-try block is skipped entirely and
|
||||
AI fires as the primary path on every run, burning LLM tokens silently.
|
||||
2. No ai= argument at all and no selector — the call has no deterministic path
|
||||
and no explicit AI strategy, so it silently burns tokens with no fallback
|
||||
episode created.
|
||||
|
||||
ai='proactive' without a selector is intentional (AI always generates the value)
|
||||
and is NOT flagged here.
|
||||
|
||||
Returns an error message or None if no issues found.
|
||||
"""
|
||||
issues: list[str] = []
|
||||
lines = code.split("\n")
|
||||
i = 0
|
||||
while i < len(lines):
|
||||
stripped = lines[i].lstrip()
|
||||
if stripped.startswith("#"):
|
||||
i += 1
|
||||
continue
|
||||
match = self._PAGE_CALL_RE.search(lines[i])
|
||||
if match and match.group(1) in self._INTERACTION_METHODS:
|
||||
end_line = self._find_call_end(lines, i)
|
||||
call_text = "\n".join(lines[i : end_line + 1]) if end_line > i else lines[i]
|
||||
has_selector = bool(re.search(r"""\bselector\s*=""", call_text))
|
||||
has_any_ai = bool(re.search(r"""\bai\s*=""", call_text))
|
||||
has_proactive = bool(re.search(r"""\bai\s*=\s*['"]proactive['"]""", call_text))
|
||||
# Flag if no selector AND (explicit fallback OR no ai argument at all).
|
||||
# ai='proactive' without selector is fine — intentional AI-driven fill.
|
||||
if not has_selector and has_any_ai and not has_proactive:
|
||||
issues.append(f"page.{match.group(1)}() on line {i + 1}")
|
||||
elif not has_selector and not has_any_ai:
|
||||
issues.append(f"page.{match.group(1)}() on line {i + 1} (no ai= argument)")
|
||||
i = end_line + 1
|
||||
else:
|
||||
i += 1
|
||||
|
||||
if not issues:
|
||||
return None
|
||||
|
||||
return (
|
||||
f"Missing selector on interaction methods: {', '.join(issues[:5])}. "
|
||||
f"Interaction methods without a selector= argument have no deterministic path — "
|
||||
f"they silently invoke the LLM on every run, burning tokens with no fallback "
|
||||
f"episode created. Add a selector= argument with a stable CSS selector "
|
||||
f"(aria-label, placeholder, name, role, :has-text()) and set ai='fallback' "
|
||||
f"so the element is found without an LLM call."
|
||||
)
|
||||
return validate_missing_selectors(code)
|
||||
|
||||
# Known auto-generated ID patterns from popular web frameworks.
|
||||
# These IDs change across deployments/sessions and break cached selectors.
|
||||
|
|
@ -2347,7 +2724,8 @@ class ScriptReviewer:
|
|||
f"These IDs are generated by web frameworks (DotNetNuke, Ember, React, MUI, etc.) "
|
||||
f"and change across deployments. Replace with stable selectors: "
|
||||
f"aria-label, placeholder, name, role, data-testid, or :has-text() with stable text. "
|
||||
f"If no stable selector exists, use ai='fallback' with a descriptive prompt and NO selector."
|
||||
f"If no stable selector exists, use ai='proactive' with a descriptive prompt and OMIT selector= entirely "
|
||||
f"(the no-selector escape hatch). Do NOT keep ai='fallback' without a selector — that crashes at runtime."
|
||||
)
|
||||
|
||||
# Regex for dates in common formats (MM/DD/YYYY, M/D/YYYY, YYYY-MM-DD)
|
||||
|
|
@ -2646,6 +3024,286 @@ class ScriptReviewer:
|
|||
|
||||
return "\n".join(lines)
|
||||
|
||||
# Patterns the consolidation pass treats as "no real text fingerprint" — branches
|
||||
# carrying these as their text_patterns entry are mergeable with any sibling whose
|
||||
# action body matches byte-for-byte.
|
||||
_CONSOLIDATE_EMPTY_PATTERNS: frozenset[str] = frozenset({"", "n/a"})
|
||||
|
||||
@staticmethod
|
||||
def _is_classify_call(node: cst.BaseExpression) -> bool:
|
||||
"""Return True if ``node`` is (``await``-wrapped) ``page.classify(...)``."""
|
||||
if isinstance(node, cst.Await):
|
||||
node = node.expression
|
||||
if not isinstance(node, cst.Call):
|
||||
return False
|
||||
func = node.func
|
||||
return (
|
||||
isinstance(func, cst.Attribute)
|
||||
and isinstance(func.value, cst.Name)
|
||||
and func.value.value == "page"
|
||||
and func.attr.value == "classify"
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _classify_call_node(value: cst.BaseExpression) -> cst.Call | None:
|
||||
"""Unwrap ``await page.classify(...)`` and return the inner Call node, or None."""
|
||||
if isinstance(value, cst.Await):
|
||||
value = value.expression
|
||||
if isinstance(value, cst.Call) and ScriptReviewer._is_classify_call(value):
|
||||
return value
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _string_literal_value(node: cst.BaseExpression) -> str | None:
|
||||
"""Extract the runtime string value from a ``cst.SimpleString`` (or None for f-strings)."""
|
||||
if isinstance(node, cst.SimpleString):
|
||||
try:
|
||||
evaluated = node.evaluated_value
|
||||
except Exception:
|
||||
return None
|
||||
return evaluated if isinstance(evaluated, str) else None
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _dict_kwarg(call: cst.Call, name: str) -> cst.Dict | None:
|
||||
"""Return the dict literal passed as ``name=...`` to this call, or None."""
|
||||
for arg in call.args:
|
||||
if arg.keyword is not None and arg.keyword.value == name and isinstance(arg.value, cst.Dict):
|
||||
return arg.value
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _dict_keys(d: cst.Dict | None) -> dict[str, cst.DictElement]:
|
||||
"""Index a Dict literal's elements by their string-literal key. Non-string keys are skipped."""
|
||||
if d is None:
|
||||
return {}
|
||||
out: dict[str, cst.DictElement] = {}
|
||||
for el in d.elements:
|
||||
if isinstance(el, cst.DictElement):
|
||||
key = ScriptReviewer._string_literal_value(el.key)
|
||||
if key is not None:
|
||||
out[key] = el
|
||||
return out
|
||||
|
||||
@staticmethod
|
||||
def _if_chain_test_against(test: cst.BaseExpression, var_name: str) -> str | None:
|
||||
"""If ``test`` is ``var_name == "literal"``, return the literal value; else None."""
|
||||
if not isinstance(test, cst.Comparison):
|
||||
return None
|
||||
if not isinstance(test.left, cst.Name) or test.left.value != var_name:
|
||||
return None
|
||||
if len(test.comparisons) != 1:
|
||||
return None
|
||||
target = test.comparisons[0]
|
||||
if not isinstance(target.operator, cst.Equal):
|
||||
return None
|
||||
return ScriptReviewer._string_literal_value(target.comparator)
|
||||
|
||||
@staticmethod
|
||||
def _walk_if_chain(if_stmt: cst.If, var_name: str) -> list[tuple[str, cst.IndentedBlock]]:
|
||||
"""Return ``[(branch_key, body), ...]`` for each ``if/elif var == "key"`` arm.
|
||||
|
||||
Stops at the first arm whose test does not match the ``var == literal`` shape
|
||||
(the ``else:`` branch returns no entry — it is the fallback, not a key).
|
||||
"""
|
||||
arms: list[tuple[str, cst.IndentedBlock]] = []
|
||||
node: cst.If | cst.Else | None = if_stmt
|
||||
while isinstance(node, cst.If):
|
||||
key = ScriptReviewer._if_chain_test_against(node.test, var_name)
|
||||
if key is None:
|
||||
break
|
||||
if isinstance(node.body, cst.IndentedBlock):
|
||||
arms.append((key, node.body))
|
||||
node = node.orelse
|
||||
return arms
|
||||
|
||||
@staticmethod
|
||||
def _body_signature(body: cst.IndentedBlock) -> str:
|
||||
"""Serialize an arm body to canonical Python source for byte-equal comparison."""
|
||||
try:
|
||||
return cst.Module(body=list(body.body)).code.strip()
|
||||
except Exception:
|
||||
return ""
|
||||
|
||||
@staticmethod
|
||||
def _is_empty_pattern(value: str | None) -> bool:
|
||||
if value is None:
|
||||
return True
|
||||
return value.strip().lower() in ScriptReviewer._CONSOLIDATE_EMPTY_PATTERNS
|
||||
|
||||
@staticmethod
|
||||
def _stmt_assigns_to(stmt: cst.BaseStatement, name: str) -> bool:
|
||||
"""Return True if ``stmt`` is a top-level rebind of ``name``.
|
||||
|
||||
Handles plain, annotated, and augmented assigns. Walrus/unpacking
|
||||
targets are conservatively reported as False — the consolidator's
|
||||
safety gate biases toward over-eager skipping, so a false negative
|
||||
here just means we skip consolidation when we could have proceeded.
|
||||
"""
|
||||
if not isinstance(stmt, cst.SimpleStatementLine):
|
||||
return False
|
||||
for small in stmt.body:
|
||||
if isinstance(small, cst.Assign):
|
||||
for target in small.targets:
|
||||
inner = target.target
|
||||
if isinstance(inner, cst.Name) and inner.value == name:
|
||||
return True
|
||||
elif isinstance(small, cst.AnnAssign):
|
||||
if isinstance(small.target, cst.Name) and small.target.value == name:
|
||||
return True
|
||||
elif isinstance(small, cst.AugAssign):
|
||||
if isinstance(small.target, cst.Name) and small.target.value == name:
|
||||
return True
|
||||
return False
|
||||
|
||||
@staticmethod
|
||||
def _body_references_name(body: cst.IndentedBlock, name: str) -> bool:
|
||||
"""Return True if ``body`` references the bare name ``name`` anywhere.
|
||||
|
||||
Used to refuse consolidation when textually-identical arm bodies
|
||||
depend on the matched value (e.g. ``log(state)``, ``f"go {state}"``)
|
||||
— those are not semantically equivalent. ``Attribute`` access like
|
||||
``self.name`` is not matched.
|
||||
"""
|
||||
|
||||
class _NameProbe(cst.CSTVisitor):
|
||||
def __init__(self) -> None:
|
||||
super().__init__()
|
||||
self.found = False
|
||||
|
||||
def visit_Name(self, node: cst.Name) -> None:
|
||||
if node.value == name:
|
||||
self.found = True
|
||||
|
||||
probe = _NameProbe()
|
||||
try:
|
||||
body.visit(probe)
|
||||
except Exception:
|
||||
return True # conservative on traversal failure
|
||||
return probe.found
|
||||
|
||||
@staticmethod
|
||||
def _pattern_node_repr(node: cst.BaseExpression | None) -> str | None:
|
||||
"""Canonical string repr of a pattern AST node, used for equality testing.
|
||||
|
||||
Returns the literal value for SimpleString, ``repr(tuple(values))`` for
|
||||
List/Tuple of strings (order-sensitive — over-preserving safer than
|
||||
over-merging), and None for opaque shapes (name refs, f-strings).
|
||||
"""
|
||||
if isinstance(node, cst.SimpleString):
|
||||
return ScriptReviewer._string_literal_value(node)
|
||||
if isinstance(node, (cst.List, cst.Tuple)):
|
||||
values: list[str] = []
|
||||
for el in node.elements:
|
||||
if not isinstance(el, cst.Element):
|
||||
return None
|
||||
if not isinstance(el.value, cst.SimpleString):
|
||||
return None
|
||||
v = ScriptReviewer._string_literal_value(el.value)
|
||||
if v is None:
|
||||
return None
|
||||
values.append(v)
|
||||
return repr(tuple(values))
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _url_pattern_node_has_signal(node: cst.BaseExpression | None) -> bool:
|
||||
"""Return True if a ``url_patterns`` value yields a runtime URL match.
|
||||
|
||||
Runtime classify wraps ``re.search`` in ``try/except re.error`` and
|
||||
silently skips invalid regex patterns. This helper mirrors that:
|
||||
SimpleString only, must be non-empty/non-N-A, must compile.
|
||||
"""
|
||||
if not isinstance(node, cst.SimpleString):
|
||||
return False
|
||||
value = ScriptReviewer._string_literal_value(node)
|
||||
if value is None or ScriptReviewer._is_empty_pattern(value):
|
||||
return False
|
||||
try:
|
||||
re.compile(value)
|
||||
except re.error:
|
||||
return False
|
||||
return True
|
||||
|
||||
@staticmethod
|
||||
def _pattern_node_has_signal(node: cst.BaseExpression | None) -> bool:
|
||||
"""Return True if a ``text_patterns`` value represents a real page-text signal.
|
||||
|
||||
``page.classify(text_patterns=...)`` accepts ``dict[str, str | list[str]]``.
|
||||
Runtime list-matching is conjunctive (``all(p in extracted_text for p in
|
||||
patterns)``) — one empty/N-A element makes the whole list fail. So a list
|
||||
is meaningful only when every element is a non-empty/non-N-A SimpleString.
|
||||
Opaque shapes (name refs, f-strings) return False — we cannot read them.
|
||||
"""
|
||||
if node is None:
|
||||
return False
|
||||
if isinstance(node, cst.SimpleString):
|
||||
return not ScriptReviewer._is_empty_pattern(ScriptReviewer._string_literal_value(node))
|
||||
if isinstance(node, (cst.List, cst.Tuple)):
|
||||
if not node.elements:
|
||||
return False
|
||||
for el in node.elements:
|
||||
if not isinstance(el, cst.Element):
|
||||
return False
|
||||
if not isinstance(el.value, cst.SimpleString):
|
||||
return False
|
||||
value = ScriptReviewer._string_literal_value(el.value)
|
||||
if ScriptReviewer._is_empty_pattern(value):
|
||||
return False
|
||||
return True
|
||||
return False
|
||||
|
||||
def _consolidate_classify_duplicates(self, code: str) -> tuple[str, list[str]]:
|
||||
"""Merge ``page.classify()`` branches with byte-identical action bodies.
|
||||
|
||||
Enforces Rule 6b deterministically. For every ``page.classify()`` call
|
||||
directly followed by an ``if/elif`` chain testing the assigned variable:
|
||||
|
||||
1. Group branch keys by serialized body code.
|
||||
2. For each group of size > 1, keep one key (preferring non-N/A
|
||||
``text_patterns``) and drop the rest from ``options``,
|
||||
``text_patterns``, ``url_patterns``, and the if/elif chain.
|
||||
|
||||
Returns ``(consolidated_code, dropped_keys)``. On any parse/transform
|
||||
failure, returns the input unchanged with an empty drop list — this is
|
||||
a defense-in-depth pass and must not break the review pipeline.
|
||||
"""
|
||||
if "page.classify(" not in code:
|
||||
return code, []
|
||||
try:
|
||||
tree = cst.parse_module(code)
|
||||
except Exception:
|
||||
return code, []
|
||||
transformer = _ClassifyConsolidator()
|
||||
try:
|
||||
new_tree = tree.visit(transformer)
|
||||
except Exception:
|
||||
return code, []
|
||||
if not transformer.dropped_keys:
|
||||
return code, []
|
||||
return new_tree.code, transformer.dropped_keys
|
||||
|
||||
@staticmethod
|
||||
def _count_classify_branches(code: str) -> int:
|
||||
"""Count the number of branch keys across all ``page.classify()`` calls.
|
||||
|
||||
Counts keys in the ``options=`` dict literal of every classify call in
|
||||
the code. Returns 0 if the code does not parse or contains no classify
|
||||
calls.
|
||||
"""
|
||||
if "page.classify(" not in code:
|
||||
return 0
|
||||
try:
|
||||
tree = cst.parse_module(code)
|
||||
except Exception:
|
||||
return 0
|
||||
counter = _ClassifyBranchCounter()
|
||||
try:
|
||||
tree.visit(counter)
|
||||
except Exception:
|
||||
return 0
|
||||
return counter.total
|
||||
|
||||
def _build_retry_prompt(self, failed_code: str, error: str, function_signature: str = "") -> str:
|
||||
"""Build a retry prompt that includes the failed code and error."""
|
||||
sig = function_signature or self._extract_function_signature(failed_code)
|
||||
|
|
|
|||
|
|
@ -2593,6 +2593,12 @@ ActionHandler.register_action_type(ActionType.CLOSE_PAGE, handle_close_page_acti
|
|||
def get_actual_value_of_parameter_if_secret(workflow_run_id: str, parameter: str) -> Any:
|
||||
workflow_run_context = app.WORKFLOW_CONTEXT_MANAGER.get_workflow_run_context(workflow_run_id)
|
||||
secret_value = workflow_run_context.get_original_secret_value_or_none(parameter)
|
||||
if secret_value is not None:
|
||||
credential_parameter_key = workflow_run_context.find_credential_parameter_key_for_secret(parameter)
|
||||
if credential_parameter_key is not None:
|
||||
current_context = skyvern_context.current()
|
||||
if current_context is not None:
|
||||
current_context.active_credential_parameter_key = credential_parameter_key
|
||||
return secret_value if secret_value is not None else parameter
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -196,7 +196,9 @@ async def test_get_all_runs_v2_search_key_matches_run_id_and_workflow_permanent_
|
|||
# list, so a substring check on the full SQL would be a false positive.
|
||||
where_clause = _where_clause_sql(captured["query"])
|
||||
assert "task_runs.run_id" in where_clause
|
||||
assert "task_runs.workflow_permanent_id" in where_clause
|
||||
# WPID search must match across both task_runs and the joined workflow_runs
|
||||
# so legacy rows with task_runs.workflow_permanent_id=NULL still hit.
|
||||
assert "coalesce(task_runs.workflow_permanent_id, workflow_runs.workflow_permanent_id)" in where_clause
|
||||
# autoescape rewrites '_' to e.g. '/_' so check the distinctive suffix.
|
||||
assert "abc123" in where_clause
|
||||
|
||||
|
|
@ -221,6 +223,9 @@ async def test_get_all_runs_v2_selects_workflow_deleted_flag() -> None:
|
|||
# NOT EXISTS subquery against an active (non-deleted) workflows row.
|
||||
assert "NOT (EXISTS" in rendered
|
||||
assert "workflows.deleted_at IS NULL" in rendered
|
||||
# WPID must coalesce task_runs over workflow_runs so legacy rows where
|
||||
# task_runs.workflow_permanent_id is NULL still resolve via the join.
|
||||
assert "coalesce(task_runs.workflow_permanent_id, workflow_runs.workflow_permanent_id)" in rendered
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -509,7 +509,19 @@ async def test_poll_passes_immediately_with_complete_file(setup, tmp_path):
|
|||
from skyvern.services.script_service import download
|
||||
|
||||
sleep_mock = AsyncMock()
|
||||
with patch(f"{MODULE}.asyncio.sleep", sleep_mock):
|
||||
|
||||
# Patching `script_service.asyncio.sleep` directly mutates the shared
|
||||
# asyncio module, so a stray sleep from any in-process code lands on
|
||||
# the mock and breaks `assert_not_called`. Swap script_service's
|
||||
# asyncio reference for a proxy that intercepts only `sleep`.
|
||||
class _AsyncioProxy:
|
||||
def __init__(self, sleep_attr):
|
||||
self.sleep = sleep_attr
|
||||
|
||||
def __getattr__(self, name):
|
||||
return getattr(asyncio, name)
|
||||
|
||||
with patch(f"{MODULE}.asyncio", _AsyncioProxy(sleep_mock)):
|
||||
await download(prompt="Download invoice", label="test_block")
|
||||
|
||||
refs["fallback"].assert_not_called()
|
||||
|
|
|
|||
|
|
@ -59,6 +59,7 @@ class TestCopilotContext:
|
|||
"explore_without_workflow_nudge_count",
|
||||
"user_message",
|
||||
"consecutive_tool_tracker",
|
||||
"failed_tool_step_tracker",
|
||||
"tool_activity",
|
||||
"last_workflow",
|
||||
"last_workflow_yaml",
|
||||
|
|
@ -86,6 +87,7 @@ class TestCopilotContext:
|
|||
assert ctx.explore_without_workflow_nudge_count == 0
|
||||
assert ctx.user_message == ""
|
||||
assert ctx.consecutive_tool_tracker == []
|
||||
assert ctx.failed_tool_step_tracker == {}
|
||||
assert ctx.tool_activity == []
|
||||
assert ctx.last_workflow is None
|
||||
assert ctx.workflow_persisted is False
|
||||
|
|
|
|||
|
|
@ -225,6 +225,62 @@ class TestSchemaOverlay:
|
|||
assert "clear_first" not in result
|
||||
|
||||
|
||||
class TestMCPFailedStepLoopDetection:
|
||||
@pytest.mark.asyncio
|
||||
async def test_interleaved_same_step_failures_short_circuit_third_dispatch(self) -> None:
|
||||
from skyvern.forge.sdk.copilot.mcp_adapter import SkyvernOverlayMCPServer
|
||||
|
||||
class FakeRawResult:
|
||||
def __init__(self, payload: dict[str, Any], is_error: bool = False) -> None:
|
||||
self.structured_content = payload
|
||||
self.is_error = is_error
|
||||
self.content: list[Any] = []
|
||||
|
||||
class FakeClient:
|
||||
def __init__(self) -> None:
|
||||
self.calls: list[tuple[str, dict[str, Any]]] = []
|
||||
|
||||
async def call_tool(
|
||||
self,
|
||||
name: str,
|
||||
args: dict[str, Any],
|
||||
raise_on_error: bool = False,
|
||||
) -> FakeRawResult:
|
||||
self.calls.append((name, args))
|
||||
if name == "get_browser_screenshot":
|
||||
return FakeRawResult({"ok": False, "error": "screenshot failed"}, is_error=True)
|
||||
return FakeRawResult({"ok": True, "data": {"status": "failed"}})
|
||||
|
||||
ctx = MagicMock()
|
||||
ctx.consecutive_tool_tracker = []
|
||||
ctx.failed_tool_step_tracker = {}
|
||||
client = FakeClient()
|
||||
server = SkyvernOverlayMCPServer(
|
||||
transport=MagicMock(),
|
||||
overlays={},
|
||||
alias_map={},
|
||||
allowlist=frozenset(),
|
||||
context_provider=lambda: ctx,
|
||||
)
|
||||
server._client = client
|
||||
|
||||
await server.call_tool("get_browser_screenshot", {})
|
||||
await server.call_tool("get_run_results", {})
|
||||
await server.call_tool("get_browser_screenshot", {})
|
||||
await server.call_tool("get_run_results", {})
|
||||
blocked = await server.call_tool("get_browser_screenshot", {})
|
||||
|
||||
parsed = json.loads(blocked.content[0].text)
|
||||
assert blocked.isError is True
|
||||
assert "LOOP DETECTED" in parsed["error"]
|
||||
assert client.calls == [
|
||||
("get_browser_screenshot", {}),
|
||||
("get_run_results", {}),
|
||||
("get_browser_screenshot", {}),
|
||||
("get_run_results", {}),
|
||||
]
|
||||
|
||||
|
||||
class TestMCPToolOverlayCompleteness:
|
||||
"""Verify alias map and overlay configs are in sync and complete."""
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,11 @@
|
|||
from __future__ import annotations
|
||||
|
||||
from skyvern.forge.sdk.copilot.loop_detection import detect_tool_loop
|
||||
from skyvern.forge.sdk.copilot.loop_detection import (
|
||||
clear_failed_step_tracker_for_tools,
|
||||
detect_failed_tool_step_loop,
|
||||
detect_tool_loop,
|
||||
record_tool_step_result,
|
||||
)
|
||||
|
||||
|
||||
def test_returns_none_below_threshold() -> None:
|
||||
|
|
@ -59,3 +64,156 @@ class TestLoopDetection:
|
|||
assert detect_tool_loop(tracker, "update_workflow") is None
|
||||
assert detect_tool_loop(tracker, "list_credentials") is None
|
||||
assert tracker == ["list_credentials"]
|
||||
|
||||
|
||||
class TestFailedToolStepLoopDetection:
|
||||
def test_interleaved_successful_tool_does_not_reset_failed_step(self) -> None:
|
||||
tracker: dict[str, int] = {}
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "get_browser_screenshot", {}) is None
|
||||
record_tool_step_result(tracker, "get_browser_screenshot", {}, {"ok": False, "error": "screenshot failed"})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "get_run_results", {}) is None
|
||||
record_tool_step_result(tracker, "get_run_results", {}, {"ok": True, "data": {"status": "failed"}})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "get_browser_screenshot", {}) is None
|
||||
record_tool_step_result(tracker, "get_browser_screenshot", {}, {"ok": False, "error": "screenshot failed"})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "get_run_results", {}) is None
|
||||
record_tool_step_result(tracker, "get_run_results", {}, {"ok": True, "data": {"status": "failed"}})
|
||||
|
||||
msg = detect_failed_tool_step_loop(tracker, "get_browser_screenshot", {})
|
||||
|
||||
assert msg is not None
|
||||
assert "LOOP DETECTED" in msg
|
||||
assert "get_browser_screenshot" in msg
|
||||
|
||||
def test_successful_same_step_resets_failure_streak(self) -> None:
|
||||
tracker: dict[str, int] = {}
|
||||
|
||||
record_tool_step_result(tracker, "evaluate", {"script": "document.title"}, {"ok": False, "error": "boom"})
|
||||
record_tool_step_result(tracker, "evaluate", {"script": "document.title"}, {"ok": True, "data": "ok"})
|
||||
record_tool_step_result(tracker, "evaluate", {"script": "document.title"}, {"ok": False, "error": "boom"})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "evaluate", {"script": "document.title"}) is None
|
||||
|
||||
def test_different_arguments_do_not_share_failure_streak(self) -> None:
|
||||
tracker: dict[str, int] = {}
|
||||
|
||||
record_tool_step_result(tracker, "click", {"selector": "#first"}, {"ok": False, "error": "missing"})
|
||||
record_tool_step_result(tracker, "click", {"selector": "#first"}, {"ok": False, "error": "missing"})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "click", {"selector": "#second"}) is None
|
||||
assert detect_failed_tool_step_loop(tracker, "click", {"selector": "#first"}) is not None
|
||||
|
||||
def test_block_threshold_is_two_failures(self) -> None:
|
||||
tracker: dict[str, int] = {}
|
||||
|
||||
record_tool_step_result(tracker, "click", {"selector": "#x"}, {"ok": False, "error": "boom"})
|
||||
record_tool_step_result(tracker, "click", {"selector": "#x"}, {"ok": False, "error": "boom"})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "click", {"selector": "#x"}) is not None
|
||||
|
||||
fresh: dict[str, int] = {}
|
||||
record_tool_step_result(fresh, "click", {"selector": "#y"}, {"ok": False, "error": "boom"})
|
||||
assert detect_failed_tool_step_loop(fresh, "click", {"selector": "#y"}) is None
|
||||
|
||||
def test_set_arguments_produce_stable_identity(self) -> None:
|
||||
tracker: dict[str, int] = {}
|
||||
args_a = {"keys": {"alpha", "beta", "gamma"}}
|
||||
args_b = {"keys": {"gamma", "alpha", "beta"}}
|
||||
|
||||
record_tool_step_result(tracker, "press_keys", args_a, {"ok": False, "error": "boom"})
|
||||
record_tool_step_result(tracker, "press_keys", args_b, {"ok": False, "error": "boom"})
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "press_keys", args_a) is not None
|
||||
|
||||
def test_clear_failed_step_tracker_for_tools_removes_only_named_tools(self) -> None:
|
||||
tracker: dict[str, int] = {}
|
||||
|
||||
record_tool_step_result(tracker, "run_blocks_and_collect_debug", {"x": 1}, {"ok": False, "error": "boom"})
|
||||
record_tool_step_result(tracker, "run_blocks_and_collect_debug", {"x": 1}, {"ok": False, "error": "boom"})
|
||||
record_tool_step_result(tracker, "update_and_run_blocks", {"y": 2}, {"ok": False, "error": "boom"})
|
||||
record_tool_step_result(tracker, "click", {"selector": "#z"}, {"ok": False, "error": "boom"})
|
||||
|
||||
clear_failed_step_tracker_for_tools(tracker, ["run_blocks_and_collect_debug", "update_and_run_blocks"])
|
||||
|
||||
assert detect_failed_tool_step_loop(tracker, "run_blocks_and_collect_debug", {"x": 1}) is None
|
||||
assert detect_failed_tool_step_loop(tracker, "update_and_run_blocks", {"y": 2}) is None
|
||||
record_tool_step_result(tracker, "click", {"selector": "#z"}, {"ok": False, "error": "boom"})
|
||||
assert detect_failed_tool_step_loop(tracker, "click", {"selector": "#z"}) is not None
|
||||
|
||||
def test_workflow_update_clears_block_running_failure_entries(self) -> None:
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
from skyvern.forge.sdk.copilot.context import CopilotContext
|
||||
from skyvern.forge.sdk.copilot.tools import _record_workflow_update_result
|
||||
|
||||
ctx = CopilotContext(
|
||||
organization_id="o",
|
||||
workflow_id="w",
|
||||
workflow_permanent_id="wp",
|
||||
workflow_yaml="updated yaml",
|
||||
browser_session_id=None,
|
||||
stream=MagicMock(),
|
||||
)
|
||||
record_tool_step_result(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"run_blocks_and_collect_debug",
|
||||
{"block_labels": ["A"], "parameters": {}},
|
||||
{"ok": False, "error": "boom"},
|
||||
)
|
||||
record_tool_step_result(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"run_blocks_and_collect_debug",
|
||||
{"block_labels": ["A"], "parameters": {}},
|
||||
{"ok": False, "error": "boom"},
|
||||
)
|
||||
record_tool_step_result(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"click",
|
||||
{"selector": "#x"},
|
||||
{"ok": False, "error": "boom"},
|
||||
)
|
||||
|
||||
_record_workflow_update_result(
|
||||
ctx,
|
||||
{
|
||||
"ok": True,
|
||||
"data": {"block_count": 2},
|
||||
"_workflow": SimpleNamespace(workflow_id="wf_new"),
|
||||
},
|
||||
)
|
||||
|
||||
# A follow-up run after the user's fix must not be blocked.
|
||||
assert (
|
||||
detect_failed_tool_step_loop(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"run_blocks_and_collect_debug",
|
||||
{"block_labels": ["A"], "parameters": {}},
|
||||
)
|
||||
is None
|
||||
)
|
||||
assert (
|
||||
detect_failed_tool_step_loop(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"click",
|
||||
{"selector": "#x"},
|
||||
)
|
||||
is None
|
||||
)
|
||||
record_tool_step_result(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"click",
|
||||
{"selector": "#x"},
|
||||
{"ok": False, "error": "boom"},
|
||||
)
|
||||
assert (
|
||||
detect_failed_tool_step_loop(
|
||||
ctx.failed_tool_step_tracker,
|
||||
"click",
|
||||
{"selector": "#x"},
|
||||
)
|
||||
is not None
|
||||
)
|
||||
|
|
|
|||
|
|
@ -182,7 +182,7 @@ async def test_db_operation_schedule_limit_exceeded_is_passthrough() -> None:
|
|||
class ScheduleDB:
|
||||
@db_operation("create_schedule")
|
||||
async def create_schedule(self) -> None:
|
||||
raise ScheduleLimitExceededError("org1", "wpid1", 5, 5)
|
||||
raise ScheduleLimitExceededError("org1", 5, 5)
|
||||
|
||||
db = ScheduleDB()
|
||||
with patch("skyvern.forge.sdk.db._error_handling.LOG") as mock_log:
|
||||
|
|
|
|||
|
|
@ -28,6 +28,11 @@ def _make_input_action(field_name: str, value: str = "test") -> dict:
|
|||
"element_id": f"elem_{field_name}",
|
||||
"reasoning": f"Fill the {field_name} field",
|
||||
"text": value,
|
||||
"skyvern_element_data": {
|
||||
"tagName": "input",
|
||||
"text": "",
|
||||
"attributes": {"name": field_name, "type": "text"},
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
|
|
@ -38,6 +43,11 @@ def _make_select_action(field_name: str) -> dict:
|
|||
"element_id": f"elem_{field_name}",
|
||||
"reasoning": f"Select {field_name}",
|
||||
"option": {"label": "Option A", "value": "a"},
|
||||
"skyvern_element_data": {
|
||||
"tagName": "select",
|
||||
"text": "",
|
||||
"attributes": {"name": field_name},
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -2,16 +2,21 @@
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pyotp
|
||||
import pytest
|
||||
|
||||
from skyvern.exceptions import FailedToGetTOTPVerificationCode
|
||||
from skyvern.forge.sdk.core import skyvern_context
|
||||
from skyvern.forge.sdk.core.skyvern_context import SkyvernContext
|
||||
from skyvern.services.otp_service import (
|
||||
OTPValue,
|
||||
_is_mfa_like_parameter_key,
|
||||
extract_totp_from_navigation_inputs,
|
||||
poll_otp_value,
|
||||
try_generate_totp_from_credential,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -195,3 +200,116 @@ class TestPollOtpValueRetry:
|
|||
)
|
||||
assert result == otp
|
||||
assert mock_fetch.call_count == 6
|
||||
|
||||
|
||||
class _FakeWorkflowRunContext:
|
||||
"""Minimal stub mirroring WorkflowRunContext shape for try_generate_totp_from_credential."""
|
||||
|
||||
def __init__(self, values: dict[str, dict[str, str]], secrets: dict[str, str]) -> None:
|
||||
self.values = values
|
||||
self.secrets = secrets
|
||||
|
||||
def totp_secret_value_key(self, totp_secret_id: str) -> str:
|
||||
return f"{totp_secret_id}_value"
|
||||
|
||||
def get_original_secret_value_or_none(self, key: str) -> str | None:
|
||||
return self.secrets.get(key)
|
||||
|
||||
|
||||
_VALID_TOTP_SEED = "JBSWY3DPEHPK3PXP"
|
||||
_OTHER_TOTP_SEED = "KRSXG5DJEBKWG33SMR2A"
|
||||
|
||||
|
||||
def _scoped_context(active: str | None) -> SkyvernContext:
|
||||
ctx = SkyvernContext(active_credential_parameter_key=active)
|
||||
return ctx
|
||||
|
||||
|
||||
class TestTryGenerateTotpFromCredential:
|
||||
"""Credential-aware lookup must scope to the credential the agent is typing into."""
|
||||
|
||||
def _patch_workflow_context(self, monkeypatch: pytest.MonkeyPatch, fake: _FakeWorkflowRunContext) -> None:
|
||||
from skyvern.services import otp_service
|
||||
|
||||
fake_app = SimpleNamespace(
|
||||
WORKFLOW_CONTEXT_MANAGER=SimpleNamespace(get_workflow_run_context=lambda _wr_id: fake),
|
||||
)
|
||||
monkeypatch.setattr(otp_service, "app", fake_app)
|
||||
|
||||
def test_active_credential_returns_its_own_totp(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""When two credentials exist, only the active one's TOTP is generated."""
|
||||
fake = _FakeWorkflowRunContext(
|
||||
values={
|
||||
"credentials_1": {"username": "u_a", "password": "p_a"},
|
||||
"credentials": {"username": "u_b", "password": "p_b", "totp": "tot_b"},
|
||||
},
|
||||
secrets={"tot_b_value": _OTHER_TOTP_SEED},
|
||||
)
|
||||
self._patch_workflow_context(monkeypatch, fake)
|
||||
|
||||
with skyvern_context.scoped(_scoped_context(active="credentials_1")):
|
||||
result = try_generate_totp_from_credential("wr_test")
|
||||
|
||||
assert result is None
|
||||
|
||||
def test_active_credential_with_totp_returns_code(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
fake = _FakeWorkflowRunContext(
|
||||
values={
|
||||
"credentials_1": {"username": "u_a", "password": "p_a"},
|
||||
"credentials": {"username": "u_b", "password": "p_b", "totp": "tot_b"},
|
||||
},
|
||||
secrets={"tot_b_value": _OTHER_TOTP_SEED},
|
||||
)
|
||||
self._patch_workflow_context(monkeypatch, fake)
|
||||
monkeypatch.setattr(pyotp.TOTP, "now", lambda _self: "424242")
|
||||
|
||||
with skyvern_context.scoped(_scoped_context(active="credentials")):
|
||||
result = try_generate_totp_from_credential("wr_test")
|
||||
|
||||
assert result is not None
|
||||
assert result.value == "424242"
|
||||
|
||||
def test_no_active_credential_with_multiple_totps_returns_none(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Avoid the original bug: walking all credentials when which is active is unknown."""
|
||||
fake = _FakeWorkflowRunContext(
|
||||
values={
|
||||
"credentials_1": {"username": "u_a", "totp": "tot_a"},
|
||||
"credentials": {"username": "u_b", "totp": "tot_b"},
|
||||
},
|
||||
secrets={"tot_a_value": _VALID_TOTP_SEED, "tot_b_value": _OTHER_TOTP_SEED},
|
||||
)
|
||||
self._patch_workflow_context(monkeypatch, fake)
|
||||
|
||||
with skyvern_context.scoped(_scoped_context(active=None)):
|
||||
result = try_generate_totp_from_credential("wr_test")
|
||||
|
||||
assert result is None
|
||||
|
||||
def test_no_active_credential_with_single_totp_falls_back(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Single-credential workflows still work even before any field has been typed."""
|
||||
fake = _FakeWorkflowRunContext(
|
||||
values={"credentials": {"username": "u_b", "totp": "tot_b"}},
|
||||
secrets={"tot_b_value": _VALID_TOTP_SEED},
|
||||
)
|
||||
self._patch_workflow_context(monkeypatch, fake)
|
||||
monkeypatch.setattr(pyotp.TOTP, "now", lambda _self: "131313")
|
||||
|
||||
with skyvern_context.scoped(_scoped_context(active=None)):
|
||||
result = try_generate_totp_from_credential("wr_test")
|
||||
|
||||
assert result is not None
|
||||
assert result.value == "131313"
|
||||
|
||||
def test_workflow_run_id_none_returns_none(self) -> None:
|
||||
assert try_generate_totp_from_credential(None) is None
|
||||
|
||||
def test_no_workflow_run_context_returns_none(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
from skyvern.services import otp_service
|
||||
|
||||
fake_app = SimpleNamespace(
|
||||
WORKFLOW_CONTEXT_MANAGER=SimpleNamespace(get_workflow_run_context=lambda _wr_id: None),
|
||||
)
|
||||
monkeypatch.setattr(otp_service, "app", fake_app)
|
||||
|
||||
with skyvern_context.scoped(_scoped_context(active="credentials")):
|
||||
assert try_generate_totp_from_credential("wr_test") is None
|
||||
|
|
|
|||
883
tests/unit/test_script_reviewer_consolidation.py
Normal file
883
tests/unit/test_script_reviewer_consolidation.py
Normal file
|
|
@ -0,0 +1,883 @@
|
|||
"""Tests for ScriptReviewer classify-branch consolidation pass.
|
||||
|
||||
The consolidation pass enforces Rule 6b deterministically: when the LLM
|
||||
emits multiple ``page.classify()`` branches with byte-identical action
|
||||
bodies, drop the duplicates and keep the branch with the more informative
|
||||
``text_patterns``.
|
||||
|
||||
Reproduces the SKY-9439 shape: a classify call with 4 of 12 branches
|
||||
carrying ``"N/A"`` text_patterns and resolving to identical actions as
|
||||
another branch — the reviewer was net-adding instead of replacing/merging.
|
||||
"""
|
||||
|
||||
import textwrap
|
||||
|
||||
from skyvern.services.script_reviewer import ScriptReviewer
|
||||
|
||||
|
||||
class TestConsolidateClassifyDuplicates:
|
||||
"""Direct tests for ``_consolidate_classify_duplicates``."""
|
||||
|
||||
def setup_method(self) -> None:
|
||||
self.reviewer = ScriptReviewer()
|
||||
|
||||
def test_no_classify_no_change(self) -> None:
|
||||
"""Code without page.classify() must pass through untouched."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
await page.complete()
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert new_code == code
|
||||
assert dropped == []
|
||||
|
||||
def test_distinct_branches_unchanged(self) -> None:
|
||||
"""Two branches with distinct actions must not be merged."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "Apply Now", "b": "Submit Application"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("Apply")', ai='fallback', prompt='apply')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Handle the form")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == []
|
||||
assert new_code == code
|
||||
|
||||
def test_na_pattern_duplicate_merged(self) -> None:
|
||||
"""Branch with N/A text_patterns + duplicate body should merge into the keyed branch."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"bills": "bills page", "view_bills_fallback": "fallback variant"},
|
||||
text_patterns={"bills": "Billing & Payments", "view_bills_fallback": "N/A"},
|
||||
)
|
||||
if state == "bills":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
elif state == "view_bills_fallback":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Navigate to bills")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == ["view_bills_fallback"]
|
||||
# Dropped key removed from options, text_patterns, and the if-chain.
|
||||
assert "view_bills_fallback" not in new_code
|
||||
# Kept branch's identity is intact.
|
||||
assert '"bills"' in new_code
|
||||
# Fallback else: branch is preserved.
|
||||
assert "element_fallback" in new_code
|
||||
# Only one keyed arm remains; resulting code is still parseable.
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_five_arm_classify_with_four_na_duplicates_collapsed(self) -> None:
|
||||
"""SKY-9439 reproducer: 5-arm classify, 4 N/A duplicates → keep the keyed branch only."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={
|
||||
"bills_billing": "billing area",
|
||||
"bills_billing_link": "alt billing 1",
|
||||
"bills_billing_only": "alt billing 2",
|
||||
"view_bills_fallback": "alt billing 3",
|
||||
"view_bills_billing": "alt billing 4",
|
||||
},
|
||||
text_patterns={
|
||||
"bills_billing": "Billing & Payments",
|
||||
"bills_billing_link": "N/A",
|
||||
"bills_billing_only": "N/A",
|
||||
"view_bills_fallback": "N/A",
|
||||
"view_bills_billing": "N/A",
|
||||
},
|
||||
)
|
||||
if state == "bills_billing":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
elif state == "bills_billing_link":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
elif state == "bills_billing_only":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
elif state == "view_bills_fallback":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
elif state == "view_bills_billing":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Navigate to bills")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# All four N/A duplicates dropped; the keyed branch retains.
|
||||
assert set(dropped) == {
|
||||
"bills_billing_link",
|
||||
"bills_billing_only",
|
||||
"view_bills_fallback",
|
||||
"view_bills_billing",
|
||||
}
|
||||
for k in dropped:
|
||||
assert k not in new_code
|
||||
assert '"bills_billing"' in new_code
|
||||
# Resulting code still parses and the else: fallback is intact.
|
||||
compile(new_code, "<test>", "exec")
|
||||
assert "element_fallback" in new_code
|
||||
|
||||
def test_byte_identical_with_identical_meaningful_text_patterns_collapsed(self) -> None:
|
||||
"""Rule 6b's distinctness condition: identical patterns + duplicate actions = redundant.
|
||||
|
||||
Both branches have the **same** non-empty text_pattern AND the same
|
||||
body. At runtime the second branch can never match a page that the
|
||||
first didn't already match — it's truly redundant, not a Rule 6b
|
||||
exception. Must be collapsed to prevent unbounded growth.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "Apply Now", "b": "Apply Now"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit form")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Same pattern + same body → truly redundant; drop the second.
|
||||
assert dropped == ["b"]
|
||||
assert 'state == "b"' not in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_byte_identical_with_identical_meaningful_url_patterns_collapsed(self) -> None:
|
||||
"""Identical URL regex + identical body → redundant. Drop one."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "N/A", "b": "N/A"},
|
||||
url_patterns={"a": "example\\\\.com/foo", "b": "example\\\\.com/foo"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == ["b"]
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_byte_identical_with_distinct_meaningful_patterns_preserved(self) -> None:
|
||||
"""Rule 6b: distinct text_patterns with duplicate actions are explicitly allowed.
|
||||
|
||||
Both branches carry meaningful, **different** text patterns.
|
||||
Collapsing them would drop a deterministic Tier-1 match surface for
|
||||
the second pattern. Per script-reviewer.j2:11 ("unless it has
|
||||
distinct text_patterns that improve page detection"), both branches
|
||||
must be preserved.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "Page A", "b": "Page B"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit form")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Both have signal → preserved; nothing dropped.
|
||||
assert dropped == []
|
||||
assert new_code == code
|
||||
|
||||
def test_three_way_dedup_one_keyed_two_na_all_identical(self) -> None:
|
||||
"""Three identical bodies (1 keyed + 2 N/A) → keep the keyed one, drop two."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"keyed": "real variant", "na1": "alt 1", "na2": "alt 2"},
|
||||
text_patterns={"keyed": "Submit Application", "na1": "N/A", "na2": ""},
|
||||
)
|
||||
if state == "keyed":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "na1":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "na2":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert set(dropped) == {"na1", "na2"}
|
||||
assert '"keyed"' in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_drops_head_arm_promotes_next(self) -> None:
|
||||
"""If the head ``if`` arm is dropped, the next ``elif`` becomes the new head."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"bad": "duplicate variant", "keep": "real variant"},
|
||||
text_patterns={"bad": "N/A", "keep": "Real Page"},
|
||||
)
|
||||
if state == "bad":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
elif state == "keep":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="X")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == ["bad"]
|
||||
assert 'state == "keep"' in new_code
|
||||
# The else fallback survives.
|
||||
assert "element_fallback" in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_two_arm_dedup_collapses_to_single_keyed_arm(self) -> None:
|
||||
"""A 2-arm classify with both bodies identical and both N/A → keep first arm.
|
||||
|
||||
This is the boundary case: the keeper selection always preserves at
|
||||
least one arm per signature group, so the post-consolidation chain
|
||||
retains exactly one keyed arm + the else fallback.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "N/A", "b": "N/A"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(selector='button', ai='fallback', prompt='click')
|
||||
elif state == "b":
|
||||
await page.click(selector='button', ai='fallback', prompt='click')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="X")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == ["b"]
|
||||
assert 'state == "a"' in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_distinct_signal_kinds_both_preserved(self) -> None:
|
||||
"""Branches with distinct kinds of meaningful signal (URL vs text) are both kept.
|
||||
|
||||
Per Rule 6b, distinct deterministic match surfaces with duplicate
|
||||
actions are explicitly allowed: each contributes a separate runtime
|
||||
path to the same handler. Collapsing to one would drop the other's
|
||||
deterministic match (e.g., a page that matches the URL regex but
|
||||
whose text doesn't include "Submit" would lose its Tier-0 hit).
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"text_only": "alt 1", "url_branch": "alt 2"},
|
||||
text_patterns={"text_only": "Submit", "url_branch": "N/A"},
|
||||
url_patterns={"text_only": "", "url_branch": "example\\\\.com/foo"},
|
||||
)
|
||||
if state == "text_only":
|
||||
await page.click(selector='button:has-text("Go")', ai='fallback', prompt='go')
|
||||
elif state == "url_branch":
|
||||
await page.click(selector='button:has-text("Go")', ai='fallback', prompt='go')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Go")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Both branches carry signal of different kinds → both preserved.
|
||||
assert dropped == []
|
||||
assert '"url_branch"' in new_code
|
||||
assert '"text_only"' in new_code
|
||||
assert "example" in new_code
|
||||
|
||||
def test_invalid_url_regex_treated_as_no_url_signal(self) -> None:
|
||||
"""A syntactically-non-empty but invalid-regex URL pattern provides no runtime signal.
|
||||
|
||||
Runtime classify wraps ``re.search`` in ``try/except re.error`` and
|
||||
silently skips invalid patterns (real_skyvern_page_ai.py:598-610).
|
||||
Scoring that as URL signal would let consolidation prefer a branch
|
||||
whose URL regex never matches at runtime, regressing routing for the
|
||||
same page state.
|
||||
|
||||
Here ``"(((unbalanced"`` is unparseable — the branch with text-only
|
||||
signal must win.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"bad_url": "alt 1", "text_only": "alt 2"},
|
||||
text_patterns={"bad_url": "N/A", "text_only": "Submit Application"},
|
||||
url_patterns={"bad_url": "(((unbalanced", "text_only": ""},
|
||||
)
|
||||
if state == "bad_url":
|
||||
await page.click(selector='button:has-text("Go")', ai='fallback', prompt='go')
|
||||
elif state == "text_only":
|
||||
await page.click(selector='button:has-text("Go")', ai='fallback', prompt='go')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Go")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# bad_url's regex is invalid → no URL signal. text_only has text signal.
|
||||
# text_only wins. (Without the regex check, bad_url would have falsely
|
||||
# outranked text_only because URL > text in the tier cascade.)
|
||||
assert dropped == ["bad_url"]
|
||||
assert '"text_only"' in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_branches_with_overlapping_signals_both_preserved(self) -> None:
|
||||
"""Both branches carry meaningful signal (one text-only, one text+URL): both preserved.
|
||||
|
||||
Even though the second branch's signal is "stronger" (URL plus text),
|
||||
dropping the first removes the deterministic Tier-1 match for pages
|
||||
whose URL doesn't match the regex but whose text does include
|
||||
"Apply". Rule 6b's allowance for distinct meaningful patterns
|
||||
applies regardless of relative signal strength.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"text_only": "alt 1", "both": "alt 2"},
|
||||
text_patterns={"text_only": "Apply", "both": "Submit"},
|
||||
url_patterns={"text_only": "", "both": "example\\\\.com/apply"},
|
||||
)
|
||||
if state == "text_only":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
elif state == "both":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="X")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == []
|
||||
assert '"text_only"' in new_code
|
||||
assert '"both"' in new_code
|
||||
|
||||
def test_unparseable_input_is_passthrough(self) -> None:
|
||||
"""Malformed code passes through unchanged (defense-in-depth)."""
|
||||
bad = "async def block_fn(page, context\n await page.classify("
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(bad)
|
||||
assert new_code == bad
|
||||
assert dropped == []
|
||||
|
||||
def test_no_if_chain_after_classify_skipped(self) -> None:
|
||||
"""If classify isn't followed by an if-chain on its var, skip consolidation."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "x", "b": "y"},
|
||||
text_patterns={"a": "X", "b": "Y"},
|
||||
)
|
||||
await page.complete()
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert new_code == code
|
||||
assert dropped == []
|
||||
|
||||
def test_non_literal_options_skipped(self) -> None:
|
||||
"""If ``options=`` is a name reference (not a literal Dict), skip consolidation.
|
||||
|
||||
Without this guard, the if-arms would be elided but the dropped keys
|
||||
would remain valid classify outputs — a behavioral regression: at
|
||||
runtime, ``state == "<dropped>"`` would never match and the dropped
|
||||
key's actions would be lost (execution would fall through to the
|
||||
``else:`` fallback).
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
opts = {"bills": "billing area", "view_bills_fallback": "alt"}
|
||||
tps = {"bills": "Billing", "view_bills_fallback": "N/A"}
|
||||
state = await page.classify(options=opts, text_patterns=tps)
|
||||
if state == "bills":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
elif state == "view_bills_fallback":
|
||||
await page.click(selector='a:has-text("View Bills")', ai='fallback', prompt='view bills')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Navigate to bills")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Non-literal options ⇒ no consolidation. Both arms preserved.
|
||||
assert dropped == []
|
||||
assert new_code == code
|
||||
assert "view_bills_fallback" in new_code
|
||||
|
||||
def test_list_valued_text_patterns_meaningful_branch_kept(self) -> None:
|
||||
"""``text_patterns: dict[str, str | list[str]]`` — list values carry a real signal.
|
||||
|
||||
When one branch has list-valued patterns (with at least one non-empty
|
||||
element) and a duplicate-action sibling has ``"N/A"``, the list-valued
|
||||
branch must be kept, regardless of source order.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"weak": "duplicate variant", "strong": "real variant"},
|
||||
text_patterns={"weak": "N/A", "strong": ["Apply Now", "Submit Application"]},
|
||||
)
|
||||
if state == "weak":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "strong":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# The list-valued (meaningful) branch is kept; the N/A is dropped even
|
||||
# though it appears first in source.
|
||||
assert dropped == ["weak"]
|
||||
assert '"strong"' in new_code
|
||||
assert "weak" not in new_code or 'state == "weak"' not in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_list_with_mixed_real_and_na_treated_as_no_signal(self) -> None:
|
||||
"""List with one N-A element fails runtime ``all()`` matching → no signal.
|
||||
|
||||
Runtime classify uses ``all(p in extracted_text for p in patterns)``
|
||||
(real_skyvern_page_ai.py:623-624). A mixed list (one real token + one
|
||||
N-A placeholder) cannot match the page text. Treating it as
|
||||
meaningful would let consolidation prefer it over a sibling with a
|
||||
clean string signal — biasing toward branches that are actually
|
||||
weaker at runtime.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"mixed": "duplicate variant", "clean": "real variant"},
|
||||
text_patterns={"mixed": ["Apply Now", "N/A"], "clean": "Submit Application"},
|
||||
)
|
||||
if state == "mixed":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "clean":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# "mixed" has no signal (one element is N/A → list fails runtime all-match).
|
||||
# "clean" has a meaningful single string. "clean" wins.
|
||||
assert dropped == ["mixed"]
|
||||
assert '"clean"' in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_list_valued_all_empty_strings_treated_as_no_signal(self) -> None:
|
||||
"""A list of empty/N-A strings carries no signal — falls back to source order."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"first": "x", "second": "y"},
|
||||
text_patterns={"first": ["", "N/A"], "second": []},
|
||||
)
|
||||
if state == "first":
|
||||
await page.click(selector='button', ai='fallback', prompt='click')
|
||||
elif state == "second":
|
||||
await page.click(selector='button', ai='fallback', prompt='click')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="x")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Neither branch has signal → keep first, drop second by source order.
|
||||
assert dropped == ["second"]
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_non_literal_text_patterns_still_consolidates(self) -> None:
|
||||
"""Non-literal text_patterns is OK as long as ``options=`` is a literal Dict.
|
||||
|
||||
``text_patterns`` non-literal just means we cannot read pattern values
|
||||
for the keep-vs-drop preference — we fall back to source order. The
|
||||
rewrite is still semantically safe because keys are still removable
|
||||
from ``options`` and the if-chain.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
tps = {"a": "Apply", "b": "Submit"}
|
||||
state = await page.classify(options={"a": "alpha", "b": "beta"}, text_patterns=tps)
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Bodies match; keep first by source order (no preference data available).
|
||||
assert dropped == ["b"]
|
||||
# Verify "b" is removed from options dict and the if-chain. Use the
|
||||
# specific runtime-relevant tokens — the bare letter "b" appears too
|
||||
# often in unrelated places (Python keywords, strings) to be a clean
|
||||
# negative assertion.
|
||||
assert '"b": "beta"' not in new_code
|
||||
assert 'state == "b"' not in new_code
|
||||
# Surviving branch is intact.
|
||||
assert '"a": "alpha"' in new_code
|
||||
assert 'state == "a"' in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_consolidator_skips_when_var_rebound_before_if_chain(self) -> None:
|
||||
"""If ``var`` is rebound to a non-classify value between classify and the if-chain,
|
||||
skip consolidation (CORR-8). Otherwise we'd elide arms from an if-chain that
|
||||
isn't actually keyed on classify outputs.
|
||||
|
||||
Concrete pattern: ``state = await page.classify(...)`` then
|
||||
``state = result.get('status')`` then ``if state == "approved"`` —
|
||||
the if-arms test the rebound value, not classify's output. Their
|
||||
keys ("approved", "denied") are not in classify's options ({"a", "b"}),
|
||||
so the subset-of-options safety gate correctly skips this classify.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "Apply", "b": "Submit"},
|
||||
)
|
||||
state = "approved"
|
||||
if state == "approved":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
elif state == "denied":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="X")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Arm keys ("approved", "denied") not in classify options {"a", "b"} → skip.
|
||||
# Both arms must be preserved even though their bodies are identical.
|
||||
assert dropped == []
|
||||
assert 'state == "approved"' in new_code
|
||||
assert 'state == "denied"' in new_code
|
||||
|
||||
def test_consolidator_skips_when_var_rebound_to_overlapping_key(self) -> None:
|
||||
"""Subset-of-options is not enough — rebinding can preserve key overlap.
|
||||
|
||||
Concrete CORR-8 sharper-variant: classify with options ``{"a", "b"}``
|
||||
and the rebound value happens to also be ``"a"`` or ``"b"``. Subset
|
||||
gate alone would pass. Dataflow rebinding gate must catch this and
|
||||
skip consolidation.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "Apply", "b": "Submit"},
|
||||
)
|
||||
state = "a"
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("X")', ai='fallback', prompt='x')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="X")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Rebinding detected → skip. Both arms preserved despite identical bodies.
|
||||
assert dropped == []
|
||||
assert 'state == "a"' in new_code
|
||||
assert 'state == "b"' in new_code
|
||||
|
||||
def test_consolidator_skips_when_body_references_classify_var(self) -> None:
|
||||
"""If an arm body references the classify variable, refuse to consolidate (CORR-10).
|
||||
|
||||
Two arms can have textually-identical bodies but different runtime
|
||||
effects when the body's behavior depends on the matched variable's
|
||||
value (logging, forwarding it onward, etc.). The example here is
|
||||
contrived but real: if a future block emits ``await page.click(prompt=f"go {state}")``
|
||||
in each arm, the *string* is identical but the runtime click-prompt
|
||||
differs based on which arm matched.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": "Apply", "b": "Submit"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(prompt=f"go to {state} branch", ai='fallback')
|
||||
elif state == "b":
|
||||
await page.click(prompt=f"go to {state} branch", ai='fallback')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="X")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Body references ``state`` → consolidation skipped. Both arms preserved.
|
||||
assert dropped == []
|
||||
assert 'state == "a"' in new_code
|
||||
assert 'state == "b"' in new_code
|
||||
|
||||
def test_consolidator_does_not_collapse_branches_with_opaque_patterns(self) -> None:
|
||||
"""Non-literal pattern values must NOT bucket together as identical (CORR-9).
|
||||
|
||||
If both branches use name refs (``text_patterns={"a": tp_a, "b": tp_b}``),
|
||||
we cannot read the values to verify distinctness. We must NOT collapse
|
||||
them in Stage 1. Stage 2's all-no-signal path collapses them by source
|
||||
order, but only because they're truly indistinguishable to us — not
|
||||
because we proved them identical.
|
||||
|
||||
Here we use distinct meaningful text strings for each branch but wrap
|
||||
them in name references the consolidator can't read. Old code would
|
||||
bucket both under ``(None, None)`` and drop the second; new code keeps
|
||||
them via opaque sentinels — they end up no-signal in Stage 2 which
|
||||
collapses to first. Asserting the no-signal Stage 2 outcome.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
tp_a = "Apply Now"
|
||||
tp_b = "Submit Application"
|
||||
state = await page.classify(
|
||||
options={"a": "variant a", "b": "variant b"},
|
||||
text_patterns={"a": tp_a, "b": tp_b},
|
||||
)
|
||||
if state == "a":
|
||||
await page.click(selector='button:has-text("Go")', ai='fallback', prompt='go')
|
||||
elif state == "b":
|
||||
await page.click(selector='button:has-text("Go")', ai='fallback', prompt='go')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Go")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
# Both opaque → Stage 2 all-no-signal path drops second by source order.
|
||||
# Crucially: it's NOT Stage 1 dropping them because they "looked
|
||||
# identical" — they're now in distinct opaque buckets.
|
||||
assert dropped == ["b"]
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
def test_consolidation_with_intervening_statement_between_classify_and_if(self) -> None:
|
||||
"""LLM may emit a temp variable between classify and the if-chain.
|
||||
|
||||
``_validate_classify_handling`` allows up to 8 such intervening
|
||||
statements; the consolidator must accept the same shape or it
|
||||
leaves duplicates uncollapsed for code that passed validation
|
||||
(COMP-7).
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"keyed": "real variant", "duplicate": "alt variant"},
|
||||
text_patterns={"keyed": "Submit Application", "duplicate": "N/A"},
|
||||
)
|
||||
page_url = page.url
|
||||
if state == "keyed":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
elif state == "duplicate":
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="Submit")
|
||||
"""
|
||||
)
|
||||
new_code, dropped = self.reviewer._consolidate_classify_duplicates(code)
|
||||
assert dropped == ["duplicate"]
|
||||
assert '"keyed"' in new_code
|
||||
assert "page_url = page.url" in new_code # intervening line preserved
|
||||
assert 'state == "duplicate"' not in new_code
|
||||
compile(new_code, "<test>", "exec")
|
||||
|
||||
|
||||
class TestCountClassifyBranches:
|
||||
"""Tests for ``_count_classify_branches``."""
|
||||
|
||||
def setup_method(self) -> None:
|
||||
self.reviewer = ScriptReviewer()
|
||||
|
||||
def test_no_classify_returns_zero(self) -> None:
|
||||
assert self.reviewer._count_classify_branches("async def f(page, context):\n pass\n") == 0
|
||||
|
||||
def test_single_classify_three_options(self) -> None:
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "x", "b": "y", "c": "z"},
|
||||
text_patterns={"a": "A", "b": "B", "c": "C"},
|
||||
)
|
||||
await page.complete()
|
||||
"""
|
||||
)
|
||||
assert self.reviewer._count_classify_branches(code) == 3
|
||||
|
||||
def test_two_classify_calls_summed(self) -> None:
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(options={"a": "x", "b": "y"}, text_patterns={"a": "A", "b": "B"})
|
||||
if state == "a":
|
||||
other = await page.classify(options={"x": "p", "y": "q", "z": "r"}, text_patterns={"x": "X", "y": "Y", "z": "Z"})
|
||||
else:
|
||||
await page.complete()
|
||||
"""
|
||||
)
|
||||
assert self.reviewer._count_classify_branches(code) == 5
|
||||
|
||||
def test_unparseable_returns_zero(self) -> None:
|
||||
assert self.reviewer._count_classify_branches("def broken(") == 0
|
||||
|
||||
def test_non_literal_options_falls_back_to_if_arm_count(self) -> None:
|
||||
"""When ``options=`` is a name reference, count via the if/elif arm shape."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
opts = {"a": "x", "b": "y", "c": "z"}
|
||||
state = await page.classify(options=opts, text_patterns={"a": "A", "b": "B", "c": "C"})
|
||||
if state == "a":
|
||||
await page.complete()
|
||||
elif state == "b":
|
||||
await page.complete()
|
||||
elif state == "c":
|
||||
await page.complete()
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="x")
|
||||
"""
|
||||
)
|
||||
# options non-literal → 0 from options. if-arm count = 3. max = 3.
|
||||
assert self.reviewer._count_classify_branches(code) == 3
|
||||
|
||||
def test_extract_driven_if_arms_not_counted(self) -> None:
|
||||
"""``status == 'approved'`` arms from ``page.extract()`` must not inflate the count.
|
||||
|
||||
CORR-6: the reviewer prompt explicitly teaches non-classify branching
|
||||
on extract results (``status == 'approved'`` / ``status == 'denied'``).
|
||||
Counting those as classify arms would poison Rule-4 telemetry. The
|
||||
if-arm filter restricts to names known to be bound from
|
||||
``page.classify(...)``.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
result = await page.extract(prompt='get status', schema={'type': 'object'})
|
||||
status = result.get('status')
|
||||
if status == "approved":
|
||||
await page.complete()
|
||||
elif status == "denied":
|
||||
await page.terminate(errors=["denied"])
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="x")
|
||||
"""
|
||||
)
|
||||
# No classify call ⇒ options_count = 0. if_arm_count must also be 0
|
||||
# because ``status`` was not bound from classify.
|
||||
assert self.reviewer._count_classify_branches(code) == 0
|
||||
|
||||
def test_variable_rebinding_clears_classify_status(self) -> None:
|
||||
"""When a name previously bound to classify is reassigned to a non-classify value,
|
||||
subsequent if-arms testing that name MUST NOT be counted as classify branches.
|
||||
|
||||
CORR-6 re-raise: without removing the name from ``_classify_vars`` on
|
||||
rebinding, a later ``if state == "approved"`` chain (driven by an
|
||||
extract result that happened to reuse the name) would inflate the
|
||||
count.
|
||||
"""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(options={"a": "x", "b": "y"}, text_patterns={"a": "A", "b": "B"})
|
||||
if state == "a":
|
||||
await page.complete()
|
||||
elif state == "b":
|
||||
await page.complete()
|
||||
else:
|
||||
result = await page.extract(prompt='get status', schema={'type': 'object'})
|
||||
state = result.get('status')
|
||||
if state == "approved":
|
||||
await page.complete()
|
||||
elif state == "denied":
|
||||
await page.terminate(errors=["denied"])
|
||||
elif state == "pending":
|
||||
await page.element_fallback(navigation_goal="x")
|
||||
"""
|
||||
)
|
||||
# 2 classify arms (a, b). The 3 extract-driven arms (approved/denied/pending)
|
||||
# MUST NOT count even though they test the same name ``state``, because
|
||||
# ``state`` was rebound to a non-classify value.
|
||||
assert self.reviewer._count_classify_branches(code) == 2
|
||||
|
||||
def test_extract_arms_alongside_classify_arms_only_classify_counted(self) -> None:
|
||||
"""Mixed file: classify arms count, extract arms do not."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(options={"a": "x", "b": "y"}, text_patterns={"a": "A", "b": "B"})
|
||||
if state == "a":
|
||||
await page.complete()
|
||||
elif state == "b":
|
||||
await page.complete()
|
||||
else:
|
||||
result = await page.extract(prompt='x', schema={'type': 'object'})
|
||||
status = result.get('status')
|
||||
if status == "approved":
|
||||
await page.complete()
|
||||
elif status == "denied":
|
||||
await page.terminate(errors=["denied"])
|
||||
"""
|
||||
)
|
||||
# 2 classify arms (a, b). The 2 extract-driven arms (approved, denied)
|
||||
# MUST NOT be counted. options_count = 2, if_arm_count = 2, max = 2.
|
||||
assert self.reviewer._count_classify_branches(code) == 2
|
||||
|
||||
def test_options_and_if_arms_take_max(self) -> None:
|
||||
"""When both signals are present, the max wins (they should agree in normal code)."""
|
||||
code = textwrap.dedent(
|
||||
"""
|
||||
async def block_fn(page, context):
|
||||
state = await page.classify(
|
||||
options={"a": "x", "b": "y"},
|
||||
text_patterns={"a": "A", "b": "B"},
|
||||
)
|
||||
if state == "a":
|
||||
await page.complete()
|
||||
elif state == "b":
|
||||
await page.complete()
|
||||
else:
|
||||
await page.element_fallback(navigation_goal="x")
|
||||
"""
|
||||
)
|
||||
assert self.reviewer._count_classify_branches(code) == 2
|
||||
|
|
@ -63,6 +63,27 @@ async def block_fn(page, context):
|
|||
# extract is not in _INTERACTION_METHODS, so this should pass
|
||||
assert self.reviewer._validate_proactive_misuse(code) is None
|
||||
|
||||
def test_proactive_without_selector_not_flagged(self) -> None:
|
||||
"""ai='proactive' WITHOUT selector= is the documented escape hatch (SKY-9436).
|
||||
|
||||
It is intentional and runtime-safe (always invokes LLM, no
|
||||
`selector: expected string` crash). Only flag when paired with selector=.
|
||||
"""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Click the next-step button')
|
||||
await page.fill(value='hello', ai='proactive', prompt='Fill the field')
|
||||
"""
|
||||
assert self.reviewer._validate_proactive_misuse(code) is None
|
||||
|
||||
def test_selector_inside_prompt_does_not_falsely_flag(self) -> None:
|
||||
"""Regression: prompt text containing 'selector=' must not make a proactive call look like proactive-with-selector (CORR-3)."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='No selector= available for this widget')
|
||||
"""
|
||||
assert self.reviewer._validate_proactive_misuse(code) is None
|
||||
|
||||
def test_comments_ignored(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
|
|
|
|||
482
tests/unit/test_script_validators.py
Normal file
482
tests/unit/test_script_validators.py
Normal file
|
|
@ -0,0 +1,482 @@
|
|||
"""Tests for the shared script validators module and the generator's emission of selectorless actions."""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
import libcst as cst
|
||||
|
||||
from skyvern.core.script_generations import generate_script as generate_script_module
|
||||
from skyvern.core.script_generations.generate_script import _action_to_stmt
|
||||
from skyvern.core.script_generations.script_validators import (
|
||||
find_recoverable_proactive_candidates,
|
||||
validate_marker_kwarg_only_on_recoverable_proactive,
|
||||
validate_missing_selectors,
|
||||
validate_unmarked_proactive_unchanged,
|
||||
)
|
||||
|
||||
|
||||
def _render(stmt: cst.BaseStatement) -> str:
|
||||
"""Render a libcst statement node to source code."""
|
||||
module = cst.Module(body=[stmt])
|
||||
return module.code
|
||||
|
||||
|
||||
class TestValidateMissingSelectorsShared:
|
||||
def test_fallback_with_selector_is_fine(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(selector='button:has-text("Submit")', ai='fallback', prompt='submit')
|
||||
"""
|
||||
assert validate_missing_selectors(code) is None
|
||||
|
||||
def test_fallback_without_selector_flagged(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='fallback', prompt='Click Billing & Payments')
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
assert "Missing selector" in error
|
||||
|
||||
def test_proactive_without_selector_not_flagged(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Click something')
|
||||
"""
|
||||
assert validate_missing_selectors(code) is None
|
||||
|
||||
def test_no_ai_arg_without_selector_flagged(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(prompt='Click something')
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
assert "no ai= argument" in error
|
||||
|
||||
def test_multiline_call_with_selector_ok(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(
|
||||
selector='a:has-text("Billing")',
|
||||
ai='fallback',
|
||||
prompt='Click billing link',
|
||||
)
|
||||
"""
|
||||
assert validate_missing_selectors(code) is None
|
||||
|
||||
def test_selector_inside_prompt_string_does_not_pass(self) -> None:
|
||||
"""Regression: prompt text containing 'selector=' must not satisfy the validator (CORR-3)."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='fallback', prompt='No selector= available for this widget')
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
|
||||
def test_ai_proactive_inside_prompt_string_still_flagged(self) -> None:
|
||||
"""Regression: prompt text containing ai='proactive' must not falsely look like the proactive escape hatch (CORR-3)."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='fallback', prompt="The original used ai='proactive'")
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
|
||||
def test_proactive_without_selector_AND_prompt_is_flagged(self) -> None:
|
||||
"""Regression: ai='proactive' without selector AND without prompt would crash at runtime
|
||||
(`Missing input: pass a selector and/or a prompt.`). Validator must catch it (CORR-3 from debate-2)."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive')
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
assert "no selector= AND no prompt=" in error
|
||||
|
||||
def test_no_selector_no_prompt_no_ai_is_flagged(self) -> None:
|
||||
"""All interaction methods missing selector AND prompt are flagged regardless of ai."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.fill(value='x')
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
assert "page.fill()" in error
|
||||
|
||||
def test_multiple_methods_flagged(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.fill(ai='fallback', value='x', prompt='enter')
|
||||
await page.type(ai='fallback', value='y', prompt='type')
|
||||
await page.select_option(ai='fallback', value='z', prompt='select')
|
||||
await page.fill_autocomplete(ai='fallback', value='w', prompt='auto')
|
||||
"""
|
||||
error = validate_missing_selectors(code)
|
||||
assert error is not None
|
||||
for method in ("fill", "type", "select_option", "fill_autocomplete"):
|
||||
assert f"page.{method}()" in error
|
||||
|
||||
|
||||
class TestGeneratorDoesNotEmitSelectorlessFallback:
|
||||
"""Verify the generator downgrades ai='fallback' to ai='proactive' when no semantic selector is available.
|
||||
|
||||
This is the SKY-9436 fix: the runtime crashes with `Locator.fill: selector:
|
||||
expected string, got undefined` when an interaction call has ai='fallback' but
|
||||
no selector= argument. We test the regression by emitting an action where
|
||||
`_build_semantic_selector` returns None (no aria-label, placeholder, name, or
|
||||
text content) and confirming the generated code uses ai='proactive'.
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _action_with_no_semantic_signal() -> dict:
|
||||
"""Build a CLICK action whose element has no aria-label/placeholder/name/text but has intention.
|
||||
|
||||
Real recordings include an `intention` from the agent — without one the runtime
|
||||
would correctly raise `Missing input: pass a selector and/or a prompt.`
|
||||
"""
|
||||
return {
|
||||
"action_type": "click",
|
||||
"xpath": "/html/body/div[3]/div[1]",
|
||||
"intention": "Click the help icon to expand the password requirements",
|
||||
"skyvern_element_data": {
|
||||
"tagName": "div",
|
||||
"text": "",
|
||||
"attributes": {},
|
||||
},
|
||||
}
|
||||
|
||||
@staticmethod
|
||||
def _action_with_aria_label() -> dict:
|
||||
return {
|
||||
"action_type": "click",
|
||||
"xpath": "/html/body/button[1]",
|
||||
"skyvern_element_data": {
|
||||
"tagName": "button",
|
||||
"text": "Submit",
|
||||
"attributes": {"aria-label": "Submit form"},
|
||||
},
|
||||
}
|
||||
|
||||
@staticmethod
|
||||
def _has_kwarg(rendered: str, key: str, val: str) -> bool:
|
||||
"""libcst preserves emitter spacing — kwargs may render as `key=val` or `key = val`.
|
||||
|
||||
Match either form by stripping whitespace around `=`.
|
||||
"""
|
||||
normalized = rendered.replace(" = ", "=").replace(" =", "=").replace("= ", "=")
|
||||
return f"{key}='{val}'" in normalized or f'{key}="{val}"' in normalized
|
||||
|
||||
def test_no_semantic_selector_downgrades_to_proactive(self) -> None:
|
||||
action = self._action_with_no_semantic_signal()
|
||||
stmt = _action_to_stmt(action, task={}, use_semantic_selectors=True)
|
||||
rendered = _render(stmt)
|
||||
assert self._has_kwarg(rendered, "ai", "proactive")
|
||||
assert not self._has_kwarg(rendered, "ai", "fallback")
|
||||
assert "selector=" not in rendered.replace(" ", "")
|
||||
assert validate_missing_selectors(rendered) is None
|
||||
|
||||
def test_semantic_selector_keeps_fallback(self) -> None:
|
||||
action = self._action_with_aria_label()
|
||||
stmt = _action_to_stmt(action, task={}, use_semantic_selectors=True)
|
||||
rendered = _render(stmt)
|
||||
assert 'aria-label="Submit form"' in rendered
|
||||
assert self._has_kwarg(rendered, "ai", "fallback")
|
||||
assert validate_missing_selectors(rendered) is None
|
||||
|
||||
def test_fill_no_semantic_selector_downgrades_to_proactive(self) -> None:
|
||||
action = {
|
||||
"action_type": "input_text",
|
||||
"xpath": "/html/body/div[3]/input[1]",
|
||||
"text": "hello",
|
||||
"intention": "Fill the captcha challenge box",
|
||||
"skyvern_element_data": {
|
||||
"tagName": "div",
|
||||
"text": "",
|
||||
"attributes": {},
|
||||
},
|
||||
}
|
||||
stmt = _action_to_stmt(action, task={}, use_semantic_selectors=True)
|
||||
rendered = _render(stmt)
|
||||
assert self._has_kwarg(rendered, "ai", "proactive")
|
||||
assert validate_missing_selectors(rendered) is None
|
||||
|
||||
def test_proactive_escape_hatch_with_intention_emits_prompt(self) -> None:
|
||||
"""When the action has an intention, the generator emits prompt= alongside ai='proactive'.
|
||||
|
||||
For truly degenerate cases (no semantic signal AND no intention/reasoning),
|
||||
we deliberately let the runtime raise `Missing input: pass a selector
|
||||
and/or a prompt` rather than synthesizing a generic prompt that would
|
||||
give the AI a vague/unsafe target (RISK-1).
|
||||
"""
|
||||
action = self._action_with_no_semantic_signal()
|
||||
action["intention"] = "Click the help icon next to the password field"
|
||||
stmt = _action_to_stmt(action, task={}, use_semantic_selectors=True)
|
||||
rendered = _render(stmt)
|
||||
assert "prompt=" in rendered.replace(" ", "")
|
||||
assert self._has_kwarg(rendered, "ai", "proactive")
|
||||
|
||||
|
||||
class TestGeneratorEndOfGenerationHook:
|
||||
"""Verifies the generator-side `validate_missing_selectors` safety net is wired in.
|
||||
|
||||
Regression guard for COMP-2: extracting the end-of-generation hook into a
|
||||
helper (`_check_missing_selectors_and_warn`) lets us test the integration
|
||||
directly without setting up a full workflow.
|
||||
"""
|
||||
|
||||
def test_warn_on_selectorless_call(self) -> None:
|
||||
bad_code = "async def block_fn(page, context):\n await page.click(ai='fallback', prompt='Click something')\n"
|
||||
with patch.object(generate_script_module, "LOG") as mock_log:
|
||||
warning = generate_script_module._check_missing_selectors_and_warn(
|
||||
bad_code,
|
||||
organization_id="o_test",
|
||||
workflow_permanent_id="wpid_test",
|
||||
workflow_run_id="wr_test",
|
||||
)
|
||||
assert warning is not None
|
||||
assert "page.click()" in warning
|
||||
mock_log.warning.assert_called_once()
|
||||
call_args = mock_log.warning.call_args
|
||||
assert call_args.args[0] == "script_generator_emitted_selectorless_action"
|
||||
assert call_args.kwargs["organization_id"] == "o_test"
|
||||
assert call_args.kwargs["workflow_permanent_id"] == "wpid_test"
|
||||
assert call_args.kwargs["workflow_run_id"] == "wr_test"
|
||||
|
||||
def test_no_warning_on_clean_code(self) -> None:
|
||||
clean_code = (
|
||||
"async def block_fn(page, context):\n"
|
||||
" await page.click(selector='button:has-text(\"Submit\")', ai='fallback', prompt='submit')\n"
|
||||
)
|
||||
with patch.object(generate_script_module, "LOG") as mock_log:
|
||||
warning = generate_script_module._check_missing_selectors_and_warn(
|
||||
clean_code,
|
||||
organization_id="o_test",
|
||||
workflow_permanent_id="wpid_test",
|
||||
workflow_run_id="wr_test",
|
||||
)
|
||||
assert warning is None
|
||||
mock_log.warning.assert_not_called()
|
||||
|
||||
def test_validator_crash_is_caught_and_logged(self) -> None:
|
||||
"""A regex crash inside the validator must not block codegen — log and continue."""
|
||||
with (
|
||||
patch.object(generate_script_module, "validate_missing_selectors", side_effect=ValueError("boom")),
|
||||
patch.object(generate_script_module, "LOG") as mock_log,
|
||||
):
|
||||
warning = generate_script_module._check_missing_selectors_and_warn(
|
||||
"irrelevant", workflow_permanent_id=None, workflow_run_id=None
|
||||
)
|
||||
assert warning is None
|
||||
mock_log.warning.assert_called_once()
|
||||
assert mock_log.warning.call_args.args[0] == "script_generator_missing_selector_validator_failed_to_run"
|
||||
|
||||
|
||||
class TestRecoverableProactive:
|
||||
"""Marker-based recovery: opportunity detector + safety enforcer (SKY-9436)."""
|
||||
|
||||
def test_marked_click_is_a_candidate(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Click help', recoverable_marker_id=42)
|
||||
"""
|
||||
candidates = find_recoverable_proactive_candidates(code)
|
||||
assert len(candidates) == 1
|
||||
assert candidates[0].method == "click"
|
||||
assert candidates[0].marker_id == 42
|
||||
|
||||
def test_unmarked_proactive_is_not_a_candidate(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick the most professional option')
|
||||
"""
|
||||
assert find_recoverable_proactive_candidates(code) == []
|
||||
|
||||
def test_marked_with_selector_is_not_a_candidate(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(selector='button', ai='proactive', prompt='click', recoverable_marker_id=1)
|
||||
"""
|
||||
assert find_recoverable_proactive_candidates(code) == []
|
||||
|
||||
def test_marked_fill_without_value_is_not_a_candidate(self) -> None:
|
||||
"""Per Rule 8f restrictions: fill without value cannot be safely upgraded."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.fill(ai='proactive', prompt='fill', recoverable_marker_id=1)
|
||||
"""
|
||||
assert find_recoverable_proactive_candidates(code) == []
|
||||
|
||||
def test_marked_fill_with_value_is_a_candidate(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.fill(value='hi', ai='proactive', prompt='fill', recoverable_marker_id=1)
|
||||
"""
|
||||
candidates = find_recoverable_proactive_candidates(code)
|
||||
assert len(candidates) == 1
|
||||
assert candidates[0].method == "fill"
|
||||
|
||||
def test_safety_validator_passes_when_unmarked_unchanged(self) -> None:
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best option')
|
||||
"""
|
||||
after = before
|
||||
assert validate_unmarked_proactive_unchanged(before, after) is None
|
||||
|
||||
def test_safety_validator_passes_when_marked_call_upgraded(self) -> None:
|
||||
"""Upgrading a MARKED proactive call is allowed; the unmarked sibling stays."""
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best option')
|
||||
await page.click(ai='proactive', prompt='Click help', recoverable_marker_id=42)
|
||||
"""
|
||||
after = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best option')
|
||||
await page.click(selector='button[aria-label="Help"]', ai='fallback', prompt='Click help')
|
||||
"""
|
||||
assert validate_unmarked_proactive_unchanged(before, after) is None
|
||||
|
||||
def test_safety_validator_blocks_when_unmarked_proactive_was_mutated(self) -> None:
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best option')
|
||||
"""
|
||||
after = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(selector='button.primary', ai='fallback', prompt='Pick best option')
|
||||
"""
|
||||
error = validate_unmarked_proactive_unchanged(before, after)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
assert "intentional" in error.lower()
|
||||
|
||||
def test_safety_validator_detects_prompt_text_change(self) -> None:
|
||||
"""Regression: kwarg-name-only matching missed semantic edits to prompt text (CORR-2)."""
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best option')
|
||||
"""
|
||||
after = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Click the submit button')
|
||||
"""
|
||||
error = validate_unmarked_proactive_unchanged(before, after)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
|
||||
def test_safety_validator_detects_removal_among_duplicates(self) -> None:
|
||||
"""Regression: set-based matching lost multiplicity (CORR-3)."""
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best')
|
||||
await page.click(ai='proactive', prompt='Pick best')
|
||||
await page.click(ai='proactive', prompt='Pick best')
|
||||
"""
|
||||
after = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick best')
|
||||
await page.click(ai='proactive', prompt='Pick best')
|
||||
"""
|
||||
error = validate_unmarked_proactive_unchanged(before, after)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
|
||||
|
||||
class TestGeneratorEmitsRecoverableMarker:
|
||||
"""The generator emits `recoverable_marker_id` only when no semantic selector is buildable (SKY-9436)."""
|
||||
|
||||
def test_marker_emitted_when_no_semantic_selector(self) -> None:
|
||||
action = {
|
||||
"action_type": "click",
|
||||
"xpath": "/html/body/div[3]/div[1]",
|
||||
"intention": "Click help icon",
|
||||
"skyvern_element_data": {"tagName": "div", "text": "", "attributes": {}},
|
||||
}
|
||||
stmt = _action_to_stmt(action, task={}, use_semantic_selectors=True)
|
||||
rendered = cst.Module(body=[stmt]).code
|
||||
normalized = rendered.replace(" ", "")
|
||||
assert "recoverable_marker_id=" in normalized
|
||||
|
||||
def test_marker_not_emitted_when_semantic_selector_available(self) -> None:
|
||||
action = {
|
||||
"action_type": "click",
|
||||
"xpath": "/html/body/button[1]",
|
||||
"intention": "Click submit",
|
||||
"skyvern_element_data": {"tagName": "button", "text": "Submit", "attributes": {"aria-label": "Submit"}},
|
||||
}
|
||||
stmt = _action_to_stmt(action, task={}, use_semantic_selectors=True)
|
||||
rendered = cst.Module(body=[stmt]).code
|
||||
assert "recoverable_marker_id" not in rendered
|
||||
|
||||
def test_marker_stable_across_invocations(self) -> None:
|
||||
"""Same action data → same marker_id (matches across recording → reviewer)."""
|
||||
action = {
|
||||
"action_type": "click",
|
||||
"xpath": "/html/body/div[3]/div[1]",
|
||||
"intention": "Click help",
|
||||
"skyvern_element_data": {"tagName": "div", "text": "", "attributes": {}},
|
||||
}
|
||||
a = cst.Module(body=[_action_to_stmt(action, task={}, use_semantic_selectors=True)]).code
|
||||
b = cst.Module(body=[_action_to_stmt(action, task={}, use_semantic_selectors=True)]).code
|
||||
assert a == b
|
||||
|
||||
|
||||
class TestMarkerKwargPosition:
|
||||
"""validate_marker_kwarg_only_on_recoverable_proactive — Rule 8f cleanup enforcement."""
|
||||
|
||||
def test_marker_on_proactive_no_selector_is_ok(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='click', recoverable_marker_id=42)
|
||||
"""
|
||||
assert validate_marker_kwarg_only_on_recoverable_proactive(code) is None
|
||||
|
||||
def test_marker_on_fallback_with_selector_is_flagged(self) -> None:
|
||||
"""Reviewer upgraded marker→selector but forgot to remove the kwarg."""
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(selector='button', ai='fallback', prompt='click', recoverable_marker_id=42)
|
||||
"""
|
||||
error = validate_marker_kwarg_only_on_recoverable_proactive(code)
|
||||
assert error is not None
|
||||
assert "page.click()" in error
|
||||
|
||||
def test_marker_on_proactive_with_selector_is_flagged(self) -> None:
|
||||
code = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(selector='button', ai='proactive', prompt='click', recoverable_marker_id=42)
|
||||
"""
|
||||
assert validate_marker_kwarg_only_on_recoverable_proactive(code) is not None
|
||||
|
||||
|
||||
class TestSemanticKwargComparison:
|
||||
"""Safety validator catches edits to ANY kwarg, not just prompt/value/intention (CORR-2 round 2)."""
|
||||
|
||||
def test_safety_validator_detects_timeout_change(self) -> None:
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick', timeout=5000)
|
||||
"""
|
||||
after = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick', timeout=30000)
|
||||
"""
|
||||
error = validate_unmarked_proactive_unchanged(before, after)
|
||||
assert error is not None
|
||||
|
||||
def test_safety_validator_detects_data_change(self) -> None:
|
||||
before = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick', data='abc')
|
||||
"""
|
||||
after = """
|
||||
async def block_fn(page, context):
|
||||
await page.click(ai='proactive', prompt='Pick', data='xyz')
|
||||
"""
|
||||
error = validate_unmarked_proactive_unchanged(before, after)
|
||||
assert error is not None
|
||||
|
|
@ -2,10 +2,11 @@ from types import SimpleNamespace
|
|||
|
||||
import pytest
|
||||
|
||||
from skyvern.forge.sdk.schemas.credentials import CredentialVaultType
|
||||
from skyvern.forge.sdk.schemas.credentials import CredentialVaultType, PasswordCredential
|
||||
from skyvern.forge.sdk.workflow import context_manager as cm
|
||||
from skyvern.forge.sdk.workflow.context_manager import WorkflowRunContext
|
||||
from skyvern.forge.sdk.workflow.models.block import TaskV2Block
|
||||
from skyvern.forge.sdk.workflow.models.parameter import CredentialParameter
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -65,6 +66,100 @@ async def test_register_credential_parameter_uses_db_totp_identifier(monkeypatch
|
|||
assert context.get_credential_totp_identifier("credential_param") == "user@example.com"
|
||||
|
||||
|
||||
async def _register_with_credential(
|
||||
monkeypatch: pytest.MonkeyPatch, credential: PasswordCredential
|
||||
) -> WorkflowRunContext:
|
||||
db_credential = SimpleNamespace(
|
||||
credential_id="cred-1",
|
||||
organization_id="org-1",
|
||||
vault_type=CredentialVaultType.BITWARDEN,
|
||||
totp_identifier=None,
|
||||
)
|
||||
|
||||
class FakeCredentialItem:
|
||||
def __init__(self, cred: PasswordCredential) -> None:
|
||||
self.credential = cred
|
||||
|
||||
class FakeCredentialService:
|
||||
async def get_credential_item(self, _db_credential: object) -> FakeCredentialItem:
|
||||
return FakeCredentialItem(credential)
|
||||
|
||||
class FakeCredentialRepo:
|
||||
async def get_credential(self, credential_id: str, organization_id: str) -> object:
|
||||
return db_credential
|
||||
|
||||
class FakeDatabase:
|
||||
def __init__(self) -> None:
|
||||
self.credentials = FakeCredentialRepo()
|
||||
|
||||
fake_app = SimpleNamespace(
|
||||
DATABASE=FakeDatabase(),
|
||||
CREDENTIAL_VAULT_SERVICES={CredentialVaultType.BITWARDEN: FakeCredentialService()},
|
||||
)
|
||||
monkeypatch.setattr(cm, "app", fake_app)
|
||||
|
||||
context = WorkflowRunContext(
|
||||
workflow_title="title",
|
||||
workflow_id="wf-1",
|
||||
workflow_permanent_id="wfp-1",
|
||||
workflow_run_id="wr-1",
|
||||
aws_client=SimpleNamespace(),
|
||||
)
|
||||
parameter = CredentialParameter.model_construct(
|
||||
key="credential_param",
|
||||
credential_parameter_id="cp-1",
|
||||
workflow_id="wf-1",
|
||||
credential_id="cred-1",
|
||||
)
|
||||
organization = SimpleNamespace(organization_id="org-1")
|
||||
await context._register_credential_parameter_value("cred-1", parameter, organization)
|
||||
return context
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_register_credential_registers_totp_seed_when_present(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
credential = PasswordCredential(
|
||||
username="user@example.com",
|
||||
password="secret",
|
||||
totp="JBSWY3DPEHPK3PXP",
|
||||
)
|
||||
context = await _register_with_credential(monkeypatch, credential)
|
||||
assert "totp" in context.values["credential_param"]
|
||||
totp_secret_id = context.values["credential_param"]["totp"]
|
||||
totp_seed = context.secrets[context.totp_secret_value_key(totp_secret_id)]
|
||||
assert totp_seed == "JBSWY3DPEHPK3PXP"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_register_credential_skips_totp_when_seed_missing(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
credential = PasswordCredential(
|
||||
username="user@example.com",
|
||||
password="secret",
|
||||
totp=None,
|
||||
)
|
||||
context = await _register_with_credential(monkeypatch, credential)
|
||||
assert "totp" not in context.values["credential_param"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_find_credential_parameter_key_for_secret_round_trip(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
credential = PasswordCredential(
|
||||
username="user@example.com",
|
||||
password="secret",
|
||||
totp="JBSWY3DPEHPK3PXP",
|
||||
)
|
||||
context = await _register_with_credential(monkeypatch, credential)
|
||||
username_secret_id = context.values["credential_param"]["username"]
|
||||
assert context.find_credential_parameter_key_for_secret(username_secret_id) == "credential_param"
|
||||
assert context.find_credential_parameter_key_for_secret("nonexistent") is None
|
||||
|
||||
|
||||
def test_task_v2_block_resolves_totp_identifier_from_context() -> None:
|
||||
block = TaskV2Block.model_construct(totp_identifier=None)
|
||||
workflow_run_context = SimpleNamespace(credential_totp_identifiers={"credential_param": "user@example.com"})
|
||||
|
|
|
|||
127
tests/unit/workflow/test_workflow_run_timeline_tree.py
Normal file
127
tests/unit/workflow/test_workflow_run_timeline_tree.py
Normal file
|
|
@ -0,0 +1,127 @@
|
|||
"""Tests for WorkflowService.get_workflow_run_timeline tree assembly."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import datetime, timedelta, timezone
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from skyvern.forge import app
|
||||
from skyvern.forge.sdk.schemas.workflow_runs import WorkflowRunBlock
|
||||
from skyvern.forge.sdk.workflow.service import WorkflowService
|
||||
from skyvern.schemas.workflows import BlockType
|
||||
|
||||
|
||||
def _block(
|
||||
block_id: str,
|
||||
*,
|
||||
parent_id: str | None = None,
|
||||
created_at: datetime,
|
||||
block_type: BlockType = BlockType.TASK,
|
||||
) -> WorkflowRunBlock:
|
||||
return WorkflowRunBlock(
|
||||
workflow_run_block_id=block_id,
|
||||
workflow_run_id="wr_test",
|
||||
organization_id="o_test",
|
||||
parent_workflow_run_block_id=parent_id,
|
||||
block_type=block_type,
|
||||
created_at=created_at,
|
||||
modified_at=created_at,
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def mock_db(monkeypatch: pytest.MonkeyPatch) -> AsyncMock:
|
||||
blocks_mock = AsyncMock(return_value=[])
|
||||
actions_mock = AsyncMock(return_value=[])
|
||||
monkeypatch.setattr(app.DATABASE.observer, "get_workflow_run_blocks", blocks_mock)
|
||||
monkeypatch.setattr(app.DATABASE.tasks, "get_tasks_actions", actions_mock)
|
||||
return blocks_mock
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeline_handles_more_than_1000_blocks_under_one_parent(mock_db: AsyncMock) -> None:
|
||||
"""Conditional with >1000 nested children keeps every child in the tree."""
|
||||
base = datetime(2026, 5, 4, 12, 0, tzinfo=timezone.utc)
|
||||
parent = _block("wrb_parent", created_at=base, block_type=BlockType.CONDITIONAL)
|
||||
child_count = 1500
|
||||
children = [
|
||||
_block(f"wrb_child_{i:04d}", parent_id="wrb_parent", created_at=base + timedelta(seconds=i + 1))
|
||||
for i in range(child_count)
|
||||
]
|
||||
mock_db.return_value = list(reversed(children)) + [parent]
|
||||
|
||||
service = WorkflowService()
|
||||
timeline = await service.get_workflow_run_timeline(workflow_run_id="wr_test", organization_id="o_test")
|
||||
|
||||
assert len(timeline) == 1
|
||||
root = timeline[0]
|
||||
assert root.block is not None and root.block.workflow_run_block_id == "wrb_parent"
|
||||
assert len(root.children) == child_count
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeline_orphan_parent_surfaces_as_root(mock_db: AsyncMock) -> None:
|
||||
"""Block whose parent is absent from the row set still appears as a root."""
|
||||
base = datetime(2026, 5, 4, 12, 0, tzinfo=timezone.utc)
|
||||
orphan = _block("wrb_orphan", parent_id="wrb_missing", created_at=base)
|
||||
mock_db.return_value = [orphan]
|
||||
|
||||
service = WorkflowService()
|
||||
timeline = await service.get_workflow_run_timeline(workflow_run_id="wr_test", organization_id="o_test")
|
||||
|
||||
assert len(timeline) == 1
|
||||
assert timeline[0].block is not None
|
||||
assert timeline[0].block.workflow_run_block_id == "wrb_orphan"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeline_preserves_deep_nesting(mock_db: AsyncMock) -> None:
|
||||
"""Loop -> conditional -> tasks round-trips with full structure."""
|
||||
base = datetime(2026, 5, 4, 12, 0, tzinfo=timezone.utc)
|
||||
loop = _block("wrb_loop", created_at=base, block_type=BlockType.FOR_LOOP)
|
||||
cond = _block(
|
||||
"wrb_cond",
|
||||
parent_id="wrb_loop",
|
||||
created_at=base + timedelta(seconds=1),
|
||||
block_type=BlockType.CONDITIONAL,
|
||||
)
|
||||
leaf_a = _block("wrb_a", parent_id="wrb_cond", created_at=base + timedelta(seconds=2))
|
||||
leaf_b = _block("wrb_b", parent_id="wrb_cond", created_at=base + timedelta(seconds=3))
|
||||
mock_db.return_value = [leaf_b, leaf_a, cond, loop]
|
||||
|
||||
service = WorkflowService()
|
||||
timeline = await service.get_workflow_run_timeline(workflow_run_id="wr_test", organization_id="o_test")
|
||||
|
||||
assert len(timeline) == 1
|
||||
assert timeline[0].block is not None and timeline[0].block.workflow_run_block_id == "wrb_loop"
|
||||
assert len(timeline[0].children) == 1
|
||||
cond_node = timeline[0].children[0]
|
||||
assert cond_node.block is not None and cond_node.block.workflow_run_block_id == "wrb_cond"
|
||||
child_ids = {child.block.workflow_run_block_id for child in cond_node.children if child.block is not None}
|
||||
assert child_ids == {"wrb_a", "wrb_b"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeline_empty_run_returns_empty_list(mock_db: AsyncMock) -> None:
|
||||
"""No blocks → empty timeline."""
|
||||
service = WorkflowService()
|
||||
timeline = await service.get_workflow_run_timeline(workflow_run_id="wr_test", organization_id="o_test")
|
||||
|
||||
assert timeline == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeline_duplicate_block_ids_keep_last(mock_db: AsyncMock, caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""Duplicate workflow_run_block_id collapses to one entry and logs a warning."""
|
||||
base = datetime(2026, 5, 4, 12, 0, tzinfo=timezone.utc)
|
||||
first = _block("wrb_dup", created_at=base)
|
||||
second = _block("wrb_dup", created_at=base + timedelta(seconds=1))
|
||||
mock_db.return_value = [first, second]
|
||||
|
||||
service = WorkflowService()
|
||||
timeline = await service.get_workflow_run_timeline(workflow_run_id="wr_test", organization_id="o_test")
|
||||
|
||||
assert len(timeline) == 1
|
||||
assert any("Duplicate workflow_run_block_id" in record.message for record in caplog.records)
|
||||
Loading…
Add table
Add a link
Reference in a new issue