diff --git a/openadapt_types/__init__.py b/openadapt_types/__init__.py index 5ae870c..fb861c5 100644 --- a/openadapt_types/__init__.py +++ b/openadapt_types/__init__.py @@ -20,6 +20,8 @@ ) """ +from importlib import metadata + from openadapt_types.action import ( Action, ActionResult, @@ -103,6 +105,7 @@ sign_human_decision_task_hmac, ) from openadapt_types.parsing import ( + PARSE_ERROR_KEY, from_benchmark_action, parse_action, parse_action_dsl, @@ -110,7 +113,12 @@ to_benchmark_action_dict, ) -__version__ = "0.1.0" +try: + __version__ = metadata.version("openadapt-types") +except metadata.PackageNotFoundError: # pragma: no cover - source checkout only + # Never report a hard-coded version. An unmeasurable version is reported as + # unknown so a caller cannot mistake a stale literal for the installed one. + __version__ = "unknown" __all__ = [ # computer_state @@ -194,6 +202,7 @@ "sign_human_decision_receipt_hmac", "sign_human_decision_task_hmac", # parsing + "PARSE_ERROR_KEY", "from_benchmark_action", "parse_action", "parse_action_dsl", diff --git a/openadapt_types/_compat.py b/openadapt_types/_compat.py index 4687f0a..7c6d3ec 100644 --- a/openadapt_types/_compat.py +++ b/openadapt_types/_compat.py @@ -26,6 +26,22 @@ from .action import Action, ActionTarget, ActionType from .computer_state import BoundingBox, ComputerState, UINode +#: Marker placed in ``Action.raw`` when the source action type was absent or +#: unrecognized. These converters previously substituted ``ActionType.DONE``, +#: which a runner reads as "the task completed successfully" - so an +#: unconvertible record became a successful terminal step. ``ActionType.FAIL`` +#: keeps "could not convert" distinguishable from "the agent finished". +UNCONVERTIBLE_ACTION_KEY = "unconvertible_action_type" + + +def _unconvertible_action(reason: str, source: dict[str, Any]) -> Action: + """Return a FAIL action naming why the source action could not convert.""" + return Action( + type=ActionType.FAIL, + reasoning=f"conversion failure: {reason}", + raw={UNCONVERTIBLE_ACTION_KEY: reason, "source": source}, + ) + # ===================================================================== # From openadapt_evals.adapters.base (dataclass dicts) @@ -75,11 +91,15 @@ def from_benchmark_action(act: dict[str, Any]) -> Action: - ``answer`` → ``answer`` - ``raw_action`` → ``raw`` """ - action_type_str = act.get("type", "done") + if act.get("type") is None: + return _unconvertible_action("BenchmarkAction has no 'type' field", act) + action_type_str = act["type"] try: action_type = ActionType(action_type_str) except ValueError: - action_type = ActionType.DONE + return _unconvertible_action( + f"unknown BenchmarkAction type {action_type_str!r}", act + ) # Build target target = None @@ -133,11 +153,15 @@ def from_ml_observation(obs: dict[str, Any]) -> ComputerState: def from_ml_action(act: dict[str, Any]) -> Action: """Convert an ``openadapt_ml.schema.episode.Action.model_dump()`` to :class:`Action`.""" - action_type_str = act.get("type", "done") + if act.get("type") is None: + return _unconvertible_action("openadapt-ml action has no 'type' field", act) + action_type_str = act["type"] try: action_type = ActionType(action_type_str) except ValueError: - action_type = ActionType.DONE + return _unconvertible_action( + f"unknown openadapt-ml action type {action_type_str!r}", act + ) # Coordinates target = None @@ -230,7 +254,9 @@ def from_omnimcp_action_decision(decision: dict[str, Any]) -> Action: - ``parameters`` → keyboard/scroll/wait params - ``analysis_reasoning`` → ``reasoning`` """ - action_type_str = decision.get("action_type", "done") + if decision.get("action_type") is None: + return _unconvertible_action("ActionDecision has no 'action_type' field", decision) + action_type_str = decision["action_type"] type_map = { "click": ActionType.CLICK, "type": ActionType.TYPE, @@ -240,7 +266,11 @@ def from_omnimcp_action_decision(decision: dict[str, Any]) -> Action: "finish": ActionType.DONE, "launch_app": ActionType.OPEN_APP, } - action_type = type_map.get(action_type_str, ActionType.DONE) + if action_type_str not in type_map: + return _unconvertible_action( + f"unknown omnimcp action_type {action_type_str!r}", decision + ) + action_type = type_map[action_type_str] target = None elem_id = decision.get("target_element_id") diff --git a/openadapt_types/computer_state.py b/openadapt_types/computer_state.py index 798e86b..8953f4d 100644 --- a/openadapt_types/computer_state.py +++ b/openadapt_types/computer_state.py @@ -240,10 +240,20 @@ def get_node(self, node_id: str) -> Optional[UINode]: return None def get_children(self, node_id: str) -> list[UINode]: - """Return direct children of *node_id*.""" + """Return direct children of *node_id*. + + Raises: + KeyError: If *node_id* is not in this state. An unknown node and a + childless node previously both returned ``[]``, so a caller + querying a stale or mistyped id observed "this element has no + children" instead of "this element is not here". + """ node = self.get_node(node_id) - if not node: - return [] + if node is None: + raise KeyError( + f"node_id {node_id!r} is not in this ComputerState; " + "an unknown node is not a node without children" + ) return [n for n in self.nodes if n.node_id in node.children_ids] def to_text_tree(self, max_depth: int = 5) -> str: diff --git a/openadapt_types/parsing.py b/openadapt_types/parsing.py index 1c23b00..2f98324 100644 --- a/openadapt_types/parsing.py +++ b/openadapt_types/parsing.py @@ -2,9 +2,16 @@ Converts multiple input formats into the canonical :class:`openadapt_types.Action` model. Every public function is safe to call with arbitrary input -- malformed -data yields ``Action(type=ActionType.DONE)`` with a logged warning instead of +data yields ``Action(type=ActionType.FAIL)`` with a logged warning instead of raising. +A parse failure is reported as ``ActionType.FAIL``, never ``ActionType.DONE``. +``DONE`` means the agent finished the task; a runner treats it as a successful +terminal state. Returning it for input the parser could not read made an +unreadable model response indistinguishable from a completed task. The failure +Action carries the reason in ``Action.reasoning`` and in +``Action.raw[PARSE_ERROR_KEY]``. + Supported input formats ----------------------- @@ -83,10 +90,27 @@ def _make_target( ) -def _done(reason: str) -> Action: - """Return a DONE action and log a warning.""" - logger.warning("Falling back to DONE: %s", reason) - return Action(type=ActionType.DONE) +#: Marker placed in ``Action.raw`` when the parser could not parse its input. +#: Its presence is the machine-checkable "this action was never parsed" signal. +PARSE_ERROR_KEY = "parse_error" + + +def _failed(reason: str) -> Action: + """Return a FAIL action carrying the reason, and log a warning. + + A parse failure must not be reported as ``ActionType.DONE``. ``DONE`` is a + real, terminal, successful outcome: it means the agent completed the task. + Returning it for unparseable input made "the model said it finished" and + "we could not read the model's output at all" the same value, so a runner + ended the episode as a success. ``ActionType.FAIL`` exists for exactly this + and forces the caller to handle the two cases apart. + """ + logger.warning("Parse failed: %s", reason) + return Action( + type=ActionType.FAIL, + reasoning=f"parse failure: {reason}", + raw={PARSE_ERROR_KEY: reason}, + ) def _safe_float(value: Any) -> Optional[float]: @@ -152,11 +176,11 @@ def parse_action_dsl(text: str) -> Action: Returns ------- Action - Parsed action, or ``Action(type=ActionType.DONE)`` on failure. + Parsed action, or ``Action(type=ActionType.FAIL)`` on failure. """ text = text.strip() if not text: - return _done("empty DSL input") + return _failed("empty DSL input") # Extract thought and action call thought: Optional[str] = None @@ -172,7 +196,7 @@ def parse_action_dsl(text: str) -> Action: action_call = m2.group("action_call") if not action_call: - return _done(f"no DSL action call found in: {text!r}") + return _failed(f"no DSL action call found in: {text!r}") # Split name and args paren_idx = action_call.index("(") @@ -181,7 +205,7 @@ def parse_action_dsl(text: str) -> Action: action_type = _resolve_action_type(action_name) if action_type is None: - return _done(f"unknown action type: {action_name!r}") + return _failed(f"unknown action type: {action_name!r}") args = _parse_dsl_args(args_str) @@ -192,7 +216,7 @@ def parse_action_dsl(text: str) -> Action: try: return _build_action_from_parsed(action_type, args, reasoning=thought) except Exception as exc: - return _done(f"failed to build action: {exc}") + return _failed(f"failed to build action: {exc}") def _build_action_from_parsed( @@ -206,7 +230,7 @@ def _build_action_from_parsed( # Validate coordinates -- if they were supposed to be numbers but aren't if ("x" in args and x is None) or ("y" in args and y is None): - return _done(f"malformed coordinates: x={args.get('x')!r}, y={args.get('y')!r}") + return _failed(f"malformed coordinates: x={args.get('x')!r}, y={args.get('y')!r}") kwargs: dict[str, Any] = {"type": action_type, "reasoning": reasoning} @@ -214,7 +238,7 @@ def _build_action_from_parsed( end_x = _safe_float(args.get("end_x")) end_y = _safe_float(args.get("end_y")) if end_x is None or end_y is None: - return _done("DRAG requires end_x and end_y") + return _failed("DRAG requires end_x and end_y") kwargs["target"] = _make_target(x, y) kwargs["drag_end"] = _make_target(end_x, end_y) elif x is not None and y is not None: @@ -235,7 +259,7 @@ def _build_action_from_parsed( try: return Action(**kwargs) except ValidationError as exc: - return _done(f"validation error: {exc}") + return _failed(f"validation error: {exc}") # --------------------------------------------------------------------------- @@ -265,11 +289,11 @@ def parse_action_json(text: str) -> Action: Returns ------- Action - Parsed action, or ``Action(type=ActionType.DONE)`` on failure. + Parsed action, or ``Action(type=ActionType.FAIL)`` on failure. """ text = text.strip() if not text: - return _done("empty JSON input") + return _failed("empty JSON input") # Strip markdown fences fence_match = _FENCE_RE.search(text) @@ -279,17 +303,17 @@ def parse_action_json(text: str) -> Action: # Find first JSON object json_match = _JSON_OBJECT_RE.search(text) if not json_match: - return _done(f"no JSON object found in: {text[:100]!r}") + return _failed(f"no JSON object found in: {text[:100]!r}") json_str = json_match.group(0) try: data = json.loads(json_str) except json.JSONDecodeError as exc: - return _done(f"invalid JSON: {exc}") + return _failed(f"invalid JSON: {exc}") if not isinstance(data, dict): - return _done(f"JSON is not an object: {type(data)}") + return _failed(f"JSON is not an object: {type(data)}") return _parse_json_dict(data) @@ -299,11 +323,11 @@ def _parse_json_dict(data: dict[str, Any]) -> Action: # Normalize type field raw_type = data.get("type") or data.get("action_type") or data.get("action") if raw_type is None: - return _done("JSON missing 'type' or 'action_type' field") + return _failed("JSON missing 'type' or 'action_type' field") action_type = _resolve_action_type(str(raw_type)) if action_type is None: - return _done(f"unknown action type in JSON: {raw_type!r}") + return _failed(f"unknown action type in JSON: {raw_type!r}") # Extract reasoning from various field names reasoning = ( @@ -385,7 +409,7 @@ def _parse_json_dict(data: dict[str, Any]) -> Action: try: return Action(**kwargs) except ValidationError as exc: - return _done(f"validation error: {exc}") + return _failed(f"validation error: {exc}") # --------------------------------------------------------------------------- @@ -397,7 +421,7 @@ def parse_action(text: str) -> Action: """Auto-detect format and parse *text* into an :class:`Action`. Tries JSON first if the text looks like it contains JSON, otherwise - tries DSL. Falls back to ``Action(type=ActionType.DONE)`` on failure. + tries DSL. Falls back to ``Action(type=ActionType.FAIL)`` on failure. Parameters ---------- @@ -411,17 +435,17 @@ def parse_action(text: str) -> Action: """ text = text.strip() if not text: - return _done("empty input") + return _failed("empty input") # Heuristic: if it looks like JSON, try JSON first if "{" in text: result = parse_action_json(text) - if result.type != ActionType.DONE: + if result.type != ActionType.FAIL: return result - # JSON parse returned DONE -- might be DSL with a brace in argument - # Try DSL as well + # JSON parse failed -- might be DSL with a brace in an argument. + # Try DSL as well. dsl_result = parse_action_dsl(text) - if dsl_result.type != ActionType.DONE: + if dsl_result.type != ActionType.FAIL: return dsl_result # Both failed, return the JSON result (first attempt) return result @@ -450,18 +474,18 @@ def from_benchmark_action(data: dict[str, Any]) -> Action: Returns ------- Action - Converted action, or ``Action(type=ActionType.DONE)`` on failure. + Converted action, or ``Action(type=ActionType.FAIL)`` on failure. """ if not isinstance(data, dict): - return _done(f"expected dict, got {type(data)}") + return _failed(f"expected dict, got {type(data)}") raw_type = data.get("type") or data.get("action_type") if raw_type is None: - return _done("BenchmarkAction missing 'type' field") + return _failed("BenchmarkAction missing 'type' field") action_type = _resolve_action_type(str(raw_type)) if action_type is None: - return _done(f"unknown action type: {raw_type!r}") + return _failed(f"unknown action type: {raw_type!r}") # Build target x = _safe_float(data.get("x")) @@ -509,7 +533,7 @@ def from_benchmark_action(data: dict[str, Any]) -> Action: try: return Action(**kwargs) except ValidationError as exc: - return _done(f"validation error from BenchmarkAction: {exc}") + return _failed(f"validation error from BenchmarkAction: {exc}") def to_benchmark_action_dict(action: Action) -> dict[str, Any]: diff --git a/tests/test_compat.py b/tests/test_compat.py index 2add431..9af8477 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -2,6 +2,7 @@ from openadapt_types import ActionType, ComputerState from openadapt_types._compat import ( + UNCONVERTIBLE_ACTION_KEY, from_benchmark_action, from_benchmark_observation, from_ml_action, @@ -67,7 +68,10 @@ def test_action_drag(self): def test_unknown_type(self): act = {"type": "some_future_action"} action = from_benchmark_action(act) - assert action.type == ActionType.DONE # fallback + # An unconvertible action is FAIL, never DONE. DONE would report the + # unreadable record as a completed task. + assert action.type == ActionType.FAIL + assert UNCONVERTIBLE_ACTION_KEY in action.raw class TestMLCompat: diff --git a/tests/test_no_false_success.py b/tests/test_no_false_success.py new file mode 100644 index 0000000..ae204e3 --- /dev/null +++ b/tests/test_no_false_success.py @@ -0,0 +1,124 @@ +"""Regressions against a failure rendered as a successful result. + +`ActionType.DONE` is a successful terminal outcome: a runner that sees it ends +the episode as complete. Every path that could not read or convert its input +must therefore produce `ActionType.FAIL`, never `DONE`. +""" + +from __future__ import annotations + +from importlib import metadata + +import pytest + +import openadapt_types +from openadapt_types import ( + PARSE_ERROR_KEY, + ActionType, + ComputerState, + UINode, + parse_action, + parse_action_dsl, + parse_action_json, +) +from openadapt_types._compat import ( + UNCONVERTIBLE_ACTION_KEY, + from_benchmark_action, + from_ml_action, + from_omnimcp_action_decision, +) + + +class TestParseFailureIsNotCompletion: + @pytest.mark.parametrize( + "text", + [ + "", + "just some random text with no action", + "FOOBAR(x=0.5, y=0.3)", + 'CLICK(x="...", y=0.3)', + "DRAG(x=0.1, y=0.2)", + ], + ) + def test_dsl_failure_is_fail_with_a_reason(self, text: str) -> None: + action = parse_action_dsl(text) + assert action.type is ActionType.FAIL + assert action.raw is not None and PARSE_ERROR_KEY in action.raw + assert action.reasoning and action.reasoning.startswith("parse failure:") + + @pytest.mark.parametrize( + "text", + [ + "", + "{not valid json at all", + '{"type": "teleport", "x": 0.5, "y": 0.3}', + '{"x": 0.5, "y": 0.3}', + "just plain text no braces", + ], + ) + def test_json_failure_is_fail_with_a_reason(self, text: str) -> None: + action = parse_action_json(text) + assert action.type is ActionType.FAIL + assert action.raw is not None and PARSE_ERROR_KEY in action.raw + + def test_a_real_done_is_still_done_and_carries_no_parse_error(self) -> None: + # The distinction this whole change exists for: a model that genuinely + # reports completion must be separable from a response nobody could + # read. + action = parse_action_json('{"type": "done"}') + assert action.type is ActionType.DONE + assert action.raw is None + + def test_parse_action_does_not_retry_a_successful_done_as_dsl(self) -> None: + # `parse_action` used to treat DONE as its failure marker, so a valid + # JSON `done` was re-parsed as DSL before being returned. + action = parse_action('{"type": "done", "reasoning": "task complete"}') + assert action.type is ActionType.DONE + assert action.reasoning == "task complete" + + def test_parse_action_garbage_is_fail(self) -> None: + assert parse_action("some random garbage").type is ActionType.FAIL + + +class TestCompatConversionFailureIsNotCompletion: + @pytest.mark.parametrize( + "converter, payload", + [ + (from_benchmark_action, {"x": 0.5, "y": 0.3}), + (from_benchmark_action, {"type": "some_future_action"}), + (from_ml_action, {"coordinates": {"x": 1, "y": 2}}), + (from_ml_action, {"type": "some_future_action"}), + (from_omnimcp_action_decision, {"parameters": {}}), + (from_omnimcp_action_decision, {"action_type": "teleport"}), + ], + ) + def test_unconvertible_source_is_fail(self, converter, payload) -> None: + action = converter(payload) + assert action.type is ActionType.FAIL + assert action.raw is not None and UNCONVERTIBLE_ACTION_KEY in action.raw + + def test_a_real_finish_still_converts_to_done(self) -> None: + action = from_omnimcp_action_decision({"action_type": "finish", "parameters": {}}) + assert action.type is ActionType.DONE + + +class TestComputerStateLookup: + def test_children_of_an_unknown_node_raises(self) -> None: + state = ComputerState( + nodes=[ + UINode(node_id="root", children_ids=["a"]), + UINode(node_id="a", parent_id="root"), + ], + ) + + # "this node is not here" must not look like "this node has no + # children". + with pytest.raises(KeyError): + state.get_children("n99") + + assert state.get_children("a") == [] + + +class TestVersionReporting: + def test_reported_version_matches_installed_distribution(self) -> None: + assert openadapt_types.__version__ == metadata.version("openadapt-types") diff --git a/tests/test_parsing.py b/tests/test_parsing.py index ed7e6a4..21f1c11 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -5,6 +5,7 @@ import pytest from openadapt_types import ( + PARSE_ERROR_KEY, Action, ActionTarget, ActionType, @@ -37,7 +38,8 @@ def test_click_pixel_coords(self): def test_click_malformed_coords_dots(self): action = parse_action_dsl('CLICK(x="...", y=0.3)') - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL + assert PARSE_ERROR_KEY in action.raw def test_click_malformed_coords_negative(self): # Negative coordinates get clamped for normalized, but for pixels @@ -60,9 +62,10 @@ def test_type_escaped_quotes(self): assert action.text == 'say "hi"' def test_type_empty_text(self): - # Empty text violates TYPE validation -> DONE + # Empty text violates TYPE validation -> FAIL, never DONE action = parse_action_dsl('TYPE(text="")') - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL + assert PARSE_ERROR_KEY in action.raw def test_key_standard(self): action = parse_action_dsl('KEY(key="enter")') @@ -127,19 +130,19 @@ def test_coordinate_clamping_normalized_within_range(self): def test_empty_input(self): action = parse_action_dsl("") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_garbage_input(self): action = parse_action_dsl("just some random text with no action") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_unknown_action_type(self): action = parse_action_dsl("FOOBAR(x=0.5, y=0.3)") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_drag_missing_end_coords(self): action = parse_action_dsl("DRAG(x=0.1, y=0.2)") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL # ── JSON Parsing ───────────────────────────────────────────────────── @@ -202,12 +205,12 @@ def test_nested_json_find_first(self): def test_invalid_json(self): text = '{not valid json at all' action = parse_action_json(text) - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_unknown_type(self): text = '{"type": "teleport", "x": 0.5, "y": 0.3}' action = parse_action_json(text) - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_type_action(self): text = '{"type": "type", "text": "hello"}' @@ -223,16 +226,16 @@ def test_key_action(self): def test_empty_input(self): action = parse_action_json("") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_no_json_object(self): action = parse_action_json("just plain text no braces") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_missing_type_field(self): text = '{"x": 0.5, "y": 0.3}' action = parse_action_json(text) - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_scroll_action(self): text = '{"type": "scroll", "scroll_direction": "down", "scroll_amount": 3}' @@ -273,13 +276,13 @@ def test_json_detected(self): assert action.type == ActionType.CLICK assert action.target.x == pytest.approx(0.5) - def test_garbage_returns_done(self): + def test_garbage_returns_fail_not_done(self): action = parse_action("some random garbage") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL - def test_empty_returns_done(self): + def test_empty_returns_fail_not_done(self): action = parse_action("") - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_json_with_fences(self): action = parse_action('```json\n{"type": "type", "text": "hello"}\n```') @@ -361,12 +364,12 @@ def test_with_answer(self): def test_missing_type(self): data = {"x": 0.5, "y": 0.3} action = from_benchmark_action(data) - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL def test_unknown_type(self): data = {"type": "teleport", "x": 0.5, "y": 0.3} action = from_benchmark_action(data) - assert action.type == ActionType.DONE + assert action.type == ActionType.FAIL class TestToBenchmarkActionDict: