fix: adjust action validation and preserve recording timeline gaps
This commit is contained in:
1 parent
2fea9865ee
commit
ab03e1d2bb
15 files changed
+235
-401
No files matched your search
@@ -53,8 +53,8 @@ from artemis.memory.transcript import (
|
||||
|
||||
# SHA-256 snapshots of the legacy system message with the fixed inputs below.
|
||||
# Update these when an intentional template change alters the rendered prompt.
|
||||
GOLDEN_EMPTY_PLAN = "dfcb15a164fa32172cf4bd2ab7b39409229a05dd1d5995eae26992784cfaa4f6"
|
||||
GOLDEN_SENTINEL_PLAN = "4b49a3c1ace6387df05a6f1f163dda007e4c2f8ff3d544dac12520317f17d21b"
|
||||
GOLDEN_EMPTY_PLAN = "2ed35be503e3872f008ae5a12616af6f90cde65f23b63adbb6da782079ae4e7c"
|
||||
GOLDEN_SENTINEL_PLAN = "8bd6aa60cf3fad8d21cf5c284a01f586c57d209fcd1d2b4a517a791736f9b1de"
|
||||
|
||||
SCREENSHOT_B64 = base64.b64encode(b"fake-jpeg-bytes").decode("utf-8")
|
||||
|
||||
|
||||
@@ -98,6 +98,3 @@ def test_prompt_rules_are_keyed_on_target_kind():
|
||||
assert "`Kind: coordinates only`" in prompt
|
||||
assert "hidden" in prompt
|
||||
assert "Identify what appears under the red dot" in prompt
|
||||
# Rule 1 judges the dot, not the screen: a control surviving elsewhere (or as
|
||||
# an active input field) does not make the tapped spot valid.
|
||||
assert "Judge the red dot, not the screen" in prompt
|
||||
@@ -28,13 +28,6 @@ from langchain_core.messages import AIMessage
|
||||
import pytest
|
||||
|
||||
from artemis.agents.validator.categories import ValidationErrorCategory
|
||||
from artemis.agents.validator.execution_loop import BURST_SKIPPED_MARKER
|
||||
from artemis.agents.validator.precondition_xml import (
|
||||
XML_STRONG_MIN_ELEMENTS,
|
||||
XML_VERDICT_STRONG,
|
||||
XML_VERDICT_WEAK,
|
||||
classify_xml_verdict,
|
||||
)
|
||||
from artemis.agents.validator.validator import ValidatorNode
|
||||
from artemis.context import ArtemisContext
|
||||
from artemis.mcp.action_types import ActionCode, ActionResult
|
||||
@@ -114,28 +107,6 @@ def temp_screenshot(tmp_path):
|
||||
return str(p)
|
||||
|
||||
|
||||
def _unrelated_hierarchy(count: int) -> list[dict]:
|
||||
"""``count`` labelled rows far below the target region (nothing matches it)."""
|
||||
return [
|
||||
{
|
||||
"text": f"Item {i}",
|
||||
"resource-id": f"row_{i}",
|
||||
"bounds": f"[0,{1000 + i * 100}][1080,{1080 + i * 100}]",
|
||||
"clickable": "true",
|
||||
}
|
||||
for i in range(count)
|
||||
]
|
||||
|
||||
|
||||
_LOGIN_TAP = {
|
||||
"action": "tap",
|
||||
"coordinates": [200, 250],
|
||||
"target_text": "Login",
|
||||
"target_bounds": [100, 200, 300, 300],
|
||||
"target_resource_id": "btn_login",
|
||||
}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_mcp():
|
||||
fake = FakeActionSession()
|
||||
@@ -212,12 +183,11 @@ async def test_validator_exec_error_opens_incident(mock_mcp, mock_context, temp_
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_burst_vets_first_member_only_and_aborts_on_first_failure(
|
||||
async def test_validator_burst_skips_safety_net_and_aborts_on_first_failure(
|
||||
mock_mcp, mock_context, temp_screenshot
|
||||
):
|
||||
"""Two or more actions form a fast-action burst: the precondition gate runs
|
||||
for the first member only, every member is dispatched once, and the first
|
||||
failure aborts the rest."""
|
||||
"""Two or more actions form a fast-action burst: no precondition gate, a
|
||||
single dispatch per member, and the first failure aborts the rest."""
|
||||
calls = []
|
||||
|
||||
def handler(name, args):
|
||||
@@ -245,12 +215,9 @@ async def test_validator_burst_vets_first_member_only_and_aborts_on_first_failur
|
||||
) as mock_gate,
|
||||
patch("artemis.utils.image_diff.check_ui_change", return_value=False),
|
||||
):
|
||||
mock_gate.return_value = (True, ValidationErrorCategory.NONE, "")
|
||||
result = await node(state)
|
||||
|
||||
# Only the first member (decided on the still-live screen) is vetted.
|
||||
mock_gate.assert_called_once()
|
||||
assert mock_gate.call_args.args[2]["coordinates"] == [105, 205]
|
||||
mock_gate.assert_not_called()
|
||||
|
||||
report = result["last_execution_result"]
|
||||
assert report["burst"] is True
|
||||
@@ -272,42 +239,6 @@ async def test_validator_burst_vets_first_member_only_and_aborts_on_first_failur
|
||||
assert result["open_incident"] == incident
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_burst_first_member_blocked_aborts_whole_burst(
|
||||
mock_mcp, mock_context, temp_screenshot
|
||||
):
|
||||
"""A refused first member opens a safety-net incident and skips the rest
|
||||
of the burst; nothing is dispatched to the device."""
|
||||
mock_mcp.hierarchy = _unrelated_hierarchy(XML_STRONG_MIN_ELEMENTS + 2)
|
||||
decisions = json.dumps([dict(_LOGIN_TAP), {"action": "press_key", "keycode": "ENTER"}])
|
||||
state = DummyState(structured_decisions=decisions, latest_screenshot=temp_screenshot)
|
||||
|
||||
node = ValidatorNode(mock_context)
|
||||
with (
|
||||
patch("artemis.utils.image_diff.check_ui_change", return_value=False),
|
||||
patch.object(
|
||||
ValidatorNode, "_validate_action_precondition_pixel", new_callable=AsyncMock
|
||||
) as mock_pixel,
|
||||
):
|
||||
result = await node(state)
|
||||
|
||||
report = result["last_execution_result"]
|
||||
assert report["burst"] is True
|
||||
assert report["status"] == "failed"
|
||||
assert report["execution"][0]["attempts"][0].startswith("Pre-execution validation failed")
|
||||
assert report["execution"][1]["attempts"] == [BURST_SKIPPED_MARKER]
|
||||
assert mock_mcp.calls_for("click") == []
|
||||
assert mock_mcp.calls_for("press_key") == []
|
||||
mock_pixel.assert_not_called()
|
||||
|
||||
incident = report["incident"]
|
||||
assert incident["kind"] == "safety_net"
|
||||
assert incident["category"] == ValidationErrorCategory.TARGET_DISAPPEARED.value
|
||||
assert incident["action_index"] == 0
|
||||
assert incident["burst_size"] == 2
|
||||
assert result["open_incident"] == incident
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_burst_success_executes_every_member(
|
||||
mock_mcp, mock_context, temp_screenshot
|
||||
@@ -643,126 +574,6 @@ async def test_validator_pre_execution_validation_anonymous_occupant(
|
||||
)
|
||||
|
||||
|
||||
def test_classify_xml_verdict_grades_strength():
|
||||
strong, why = classify_xml_verdict(XML_STRONG_MIN_ELEMENTS, "Login", None)
|
||||
assert (strong, why) == (XML_VERDICT_STRONG, "")
|
||||
assert classify_xml_verdict(XML_STRONG_MIN_ELEMENTS, None, "btn_login")[0] == XML_VERDICT_STRONG
|
||||
|
||||
weak, why = classify_xml_verdict(XML_STRONG_MIN_ELEMENTS - 1, "Login", "btn_login")
|
||||
assert weak == XML_VERDICT_WEAK and "sparse hierarchy" in why
|
||||
|
||||
weak, why = classify_xml_verdict(XML_STRONG_MIN_ELEMENTS, None, None)
|
||||
assert weak == XML_VERDICT_WEAK and "no resource-id/text" in why
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_strong_xml_missing_verdict_is_final_without_pixel_judge(
|
||||
mock_mcp, mock_context, temp_screenshot
|
||||
):
|
||||
"""A strong XML verdict blocks the action without consulting the pixel judge."""
|
||||
mock_mcp.hierarchy = _unrelated_hierarchy(XML_STRONG_MIN_ELEMENTS + 2)
|
||||
state = DummyState(
|
||||
structured_decisions=json.dumps([dict(_LOGIN_TAP)]), latest_screenshot=temp_screenshot
|
||||
)
|
||||
node = ValidatorNode(mock_context)
|
||||
|
||||
with (
|
||||
patch("artemis.utils.image_diff.check_ui_change", return_value=True),
|
||||
patch.object(
|
||||
ValidatorNode, "_validate_action_precondition_pixel", new_callable=AsyncMock
|
||||
) as mock_pixel,
|
||||
):
|
||||
# A positive pixel verdict must not override strong XML evidence.
|
||||
mock_pixel.return_value = (True, ValidationErrorCategory.NONE, "")
|
||||
result = await node(state)
|
||||
|
||||
mock_pixel.assert_not_called()
|
||||
assert mock_mcp.calls_for("click") == []
|
||||
|
||||
incident = result["last_execution_result"]["incident"]
|
||||
assert incident["kind"] == "safety_net"
|
||||
assert incident["category"] == ValidationErrorCategory.TARGET_DISAPPEARED.value
|
||||
assert "was not found on the screen" in incident["reason"]
|
||||
assert "pixel judge not consulted" in incident["reason"]
|
||||
assert incident["evidence"]["xml_verdict"] == XML_VERDICT_STRONG
|
||||
assert incident["evidence"]["xml_element_count"] == XML_STRONG_MIN_ELEMENTS + 2
|
||||
assert incident["evidence"]["pixel_judge"] == "not consulted (strong XML verdict)"
|
||||
# The clause also rides in the action record the traces keep.
|
||||
assert (
|
||||
"pixel judge not consulted"
|
||||
in result["last_execution_result"]["execution"][0]["attempts"][0]
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_weak_xml_sparse_tree_lets_pixel_judge_allow(
|
||||
mock_mcp, mock_context, temp_screenshot
|
||||
):
|
||||
"""A sparse hierarchy (WebView/game-like) cannot vouch for a missing target;
|
||||
the pixel judge decides, and a confident "present" lets the action run."""
|
||||
mock_mcp.hierarchy = _unrelated_hierarchy(2)
|
||||
state = DummyState(
|
||||
structured_decisions=json.dumps([dict(_LOGIN_TAP)]), latest_screenshot=temp_screenshot
|
||||
)
|
||||
node = ValidatorNode(mock_context)
|
||||
|
||||
with (
|
||||
patch("artemis.utils.image_diff.check_ui_change", return_value=True),
|
||||
patch.object(
|
||||
ValidatorNode, "_validate_action_precondition_pixel", new_callable=AsyncMock
|
||||
) as mock_pixel,
|
||||
):
|
||||
mock_pixel.return_value = (True, ValidationErrorCategory.NONE, "")
|
||||
result = await node(state)
|
||||
|
||||
mock_pixel.assert_called_once()
|
||||
report = result["last_execution_result"]
|
||||
assert report["status"] == "success"
|
||||
assert report["incident"] is None
|
||||
assert len(mock_mcp.calls_for("click")) == 1
|
||||
assert "safety_net_evidence" not in report["execution"][0]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_weak_xml_identifierless_target_pixel_judge_blocks(
|
||||
mock_mcp, mock_context, temp_screenshot
|
||||
):
|
||||
"""Bare bounds from a hit test carry no identifiers to search for, so the
|
||||
XML "missing" is weak; the pixel judge is consulted and its refusal is
|
||||
recorded next to the XML verdict."""
|
||||
mock_mcp.hierarchy = _unrelated_hierarchy(XML_STRONG_MIN_ELEMENTS + 2)
|
||||
item = {
|
||||
"action": "tap",
|
||||
"coordinates": [200, 250],
|
||||
"target_class": "android.widget.ImageView",
|
||||
"target_bounds": [100, 200, 300, 300],
|
||||
"target_label_source": "hit_test",
|
||||
}
|
||||
state = DummyState(structured_decisions=json.dumps([item]), latest_screenshot=temp_screenshot)
|
||||
node = ValidatorNode(mock_context)
|
||||
|
||||
with (
|
||||
patch("artemis.utils.image_diff.check_ui_change", return_value=True),
|
||||
patch.object(
|
||||
ValidatorNode, "_validate_action_precondition_pixel", new_callable=AsyncMock
|
||||
) as mock_pixel,
|
||||
):
|
||||
mock_pixel.return_value = (
|
||||
False,
|
||||
ValidationErrorCategory.PIXEL_TARGET_DISAPPEARED,
|
||||
"Pixel check failed",
|
||||
)
|
||||
result = await node(state)
|
||||
|
||||
mock_pixel.assert_called_once()
|
||||
incident = result["last_execution_result"]["incident"]
|
||||
assert incident["category"] == ValidationErrorCategory.TARGET_DISAPPEARED.value
|
||||
assert "XML evidence weak (target had no resource-id/text" in incident["reason"]
|
||||
assert "pixel judge did not confirm the target" in incident["reason"]
|
||||
assert incident["evidence"]["xml_verdict"] == XML_VERDICT_WEAK
|
||||
assert incident["evidence"]["pixel_judge"].startswith("consulted (weak XML verdict)")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validator_pixel_validation_success(mock_mcp, mock_context, temp_screenshot):
|
||||
"""Test that the pixel safety net succeeds when Gemini reports target is present."""
|
||||
|
||||
@@ -35,6 +35,7 @@ from artemis.utils.video import (
|
||||
get_ffmpeg_path,
|
||||
get_active_session,
|
||||
normalize_recording_to_mp4,
|
||||
plan_timeline_pieces,
|
||||
render_timeline_clip,
|
||||
remove_active_session,
|
||||
set_active_session,
|
||||
@@ -145,6 +146,40 @@ async def test_normalize_recording_handles_resolution_change(tmp_path):
|
||||
assert dimensions == {(108, 242)}
|
||||
|
||||
|
||||
def _make_solid_clip(ffmpeg: str, color: str, size: str, duration: float, path) -> None:
|
||||
"""Encode a single-colour test clip (``size`` is ``WxH``) at 15 fps."""
|
||||
subprocess.run(
|
||||
[
|
||||
ffmpeg,
|
||||
"-y",
|
||||
"-f",
|
||||
"lavfi",
|
||||
"-i",
|
||||
f"color={color}:size={size}:rate=15:duration={duration}",
|
||||
"-c:v",
|
||||
"libx264",
|
||||
"-pix_fmt",
|
||||
"yuv420p",
|
||||
str(path),
|
||||
],
|
||||
check=True,
|
||||
capture_output=True,
|
||||
)
|
||||
|
||||
|
||||
def _read_frames(path) -> list:
|
||||
"""Decode every frame of ``path`` with OpenCV, in order."""
|
||||
capture = cv2.VideoCapture(str(path))
|
||||
frames = []
|
||||
while True:
|
||||
ok, frame = capture.read()
|
||||
if not ok:
|
||||
break
|
||||
frames.append(frame)
|
||||
capture.release()
|
||||
return frames
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_analyzer_clip_stays_continuous_across_orientation_segments(tmp_path):
|
||||
"""The analyzer must still receive one decodable MP4 across a rotation."""
|
||||
@@ -152,27 +187,8 @@ async def test_analyzer_clip_stays_continuous_across_orientation_segments(tmp_pa
|
||||
portrait = tmp_path / "recording.mkv"
|
||||
landscape = tmp_path / "recording_001.mkv"
|
||||
output = tmp_path / "agent_clip.mp4"
|
||||
for color, size, path in (
|
||||
("red", "108x242", portrait),
|
||||
("blue", "242x108", landscape),
|
||||
):
|
||||
subprocess.run(
|
||||
[
|
||||
ffmpeg,
|
||||
"-y",
|
||||
"-f",
|
||||
"lavfi",
|
||||
"-i",
|
||||
f"color={color}:size={size}:rate=15:duration=1",
|
||||
"-c:v",
|
||||
"libx264",
|
||||
"-pix_fmt",
|
||||
"yuv420p",
|
||||
str(path),
|
||||
],
|
||||
check=True,
|
||||
capture_output=True,
|
||||
)
|
||||
_make_solid_clip(ffmpeg, "red", "108x242", 1.0, portrait)
|
||||
_make_solid_clip(ffmpeg, "blue", "242x108", 1.0, landscape)
|
||||
|
||||
assert await render_timeline_clip(
|
||||
[
|
||||
@@ -186,18 +202,91 @@ async def test_analyzer_clip_stays_continuous_across_orientation_segments(tmp_pa
|
||||
canvas_height=320,
|
||||
)
|
||||
|
||||
capture = cv2.VideoCapture(str(output))
|
||||
dimensions = set()
|
||||
frames = 0
|
||||
while True:
|
||||
ok, frame = capture.read()
|
||||
if not ok:
|
||||
break
|
||||
dimensions.add((frame.shape[1], frame.shape[0]))
|
||||
frames += 1
|
||||
capture.release()
|
||||
assert dimensions == {(180, 320)}
|
||||
assert frames >= 20
|
||||
frames = _read_frames(output)
|
||||
assert {(frame.shape[1], frame.shape[0]) for frame in frames} == {(180, 320)}
|
||||
assert len(frames) >= 20
|
||||
|
||||
|
||||
def test_timeline_pieces_fill_restart_gaps(tmp_path):
|
||||
"""Holes between segments (and a window starting in one) become black gaps."""
|
||||
first = tmp_path / "recording.mkv"
|
||||
second = tmp_path / "recording_001.mkv"
|
||||
first.write_bytes(b"x")
|
||||
second.write_bytes(b"x")
|
||||
segments = [
|
||||
{"path": first, "start": 0.0, "end": 10.0},
|
||||
{"path": second, "start": 11.5, "end": 20.0}, # scrcpy restart took 1.5s
|
||||
]
|
||||
|
||||
pieces = plan_timeline_pieces(segments, 8.0, 14.0)
|
||||
assert pieces == [
|
||||
("file", first, 8.0, 10.0),
|
||||
("gap", pytest.approx(1.5)),
|
||||
("file", second, 0.0, pytest.approx(2.5)),
|
||||
]
|
||||
|
||||
# Window opening inside the hole: the clip's t=0 is still the requested start.
|
||||
pieces = plan_timeline_pieces(segments, 10.5, 13.0)
|
||||
assert pieces == [("gap", pytest.approx(1.0)), ("file", second, 0.0, pytest.approx(1.5))]
|
||||
|
||||
# Sub-epsilon seams are rounding noise, not gaps.
|
||||
seam = [
|
||||
{"path": first, "start": 0.0, "end": 10.0},
|
||||
{"path": second, "start": 10.02, "end": 20.0},
|
||||
]
|
||||
assert [p[0] for p in plan_timeline_pieces(seam, 9.0, 11.0)] == ["file", "file"]
|
||||
|
||||
assert plan_timeline_pieces(segments, 10.1, 11.4) == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_analyzer_clip_keeps_timeline_time_across_restart_gap(tmp_path):
|
||||
"""After a rotation the clip must not run ahead of the recording timeline.
|
||||
|
||||
Two 1s segments with a 1.5s restart hole between them; the requested 3s
|
||||
window must render as 3s of video with black frames where the hole was,
|
||||
so ``start_time + frame_offset`` still names the true recording time.
|
||||
"""
|
||||
ffmpeg = get_ffmpeg_path()
|
||||
portrait = tmp_path / "recording.mkv"
|
||||
landscape = tmp_path / "recording_001.mkv"
|
||||
_make_solid_clip(ffmpeg, "red", "108x242", 1.0, portrait)
|
||||
_make_solid_clip(ffmpeg, "blue", "242x108", 1.0, landscape)
|
||||
segments = [
|
||||
{"path": portrait, "start": 0.0, "end": 1.0},
|
||||
{"path": landscape, "start": 2.5, "end": 3.5},
|
||||
]
|
||||
|
||||
output = tmp_path / "agent_clip.mp4"
|
||||
assert await render_timeline_clip(
|
||||
segments, 0.25, 3.25, output, canvas_width=180, canvas_height=320
|
||||
)
|
||||
frames = _read_frames(output)
|
||||
# 3.0s at 15 fps; allow one frame of encoder slack on either side.
|
||||
assert 43 <= len(frames) <= 47
|
||||
|
||||
def is_black(frame) -> bool:
|
||||
return frame.max() < 40
|
||||
|
||||
def is_blue(frame) -> bool:
|
||||
b, g, r = frame[160, 90]
|
||||
return b > 150 and r < 80 and g < 80
|
||||
|
||||
# t=0.5s -> red segment, t=1.5s -> restart hole, t=3.0s -> blue segment.
|
||||
assert not is_black(frames[7]) and not is_blue(frames[7])
|
||||
assert is_black(frames[22])
|
||||
assert is_blue(frames[-1])
|
||||
|
||||
# A window opening inside the hole leads with black, then blue at the
|
||||
# expected timeline offset (2.5s - 1.5s = 1.0s into the clip).
|
||||
output2 = tmp_path / "agent_clip2.mp4"
|
||||
assert await render_timeline_clip(
|
||||
segments, 1.5, 3.25, output2, canvas_width=180, canvas_height=320
|
||||
)
|
||||
frames2 = _read_frames(output2)
|
||||
assert 24 <= len(frames2) <= 28
|
||||
assert is_black(frames2[3])
|
||||
assert is_blue(frames2[20])
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
|
||||
Reference in new issue
Block a user