Harden SCXML import validation - #32
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the SCXML import functionality by introducing canonical guard handlers, document-global ID validation, required-attribute validation, XML whitespace-aware targets, and full MyPy coverage. The review feedback suggests several improvements to simplify the code, including removing redundant state ID and attribute validation checks that are already handled globally, simplifying transition map typing to avoid runtime assertions, and ensuring targetless transitions omit the target key entirely when the target attribute is empty or whitespace.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def accumulate_states(element: ET.Element) -> dict[str, StateNodeConfig]: | ||
| states: dict[str, StateNodeConfig] = {} | ||
| for state_element in get_all_state_els(element): | ||
| state_id = _required_attribute(state_element, "id") | ||
| if state_id in states: | ||
| raise InvalidConfigError( | ||
| f"Duplicate SCXML state id {state_id!r} under " | ||
| f"<{get_tag(element)}> element." | ||
| ) | ||
| states[state_id] = convert_state(state_element) | ||
| return states |
There was a problem hiding this comment.
Since _validate_state_ids runs globally on the root element before conversion, every state element is already guaranteed to have a non-empty id attribute and there are no duplicate state IDs anywhere in the document. Therefore, both the _required_attribute check and the duplicate check in accumulate_states are redundant and can be safely simplified.
def accumulate_states(element: ET.Element) -> dict[str, StateNodeConfig]:
states: dict[str, StateNodeConfig] = {}
for state_element in get_all_state_els(element):
state_id = state_element.attrib["id"]
states[state_id] = convert_state(state_element)
return states| def convert_state(element: ET.Element) -> StateNodeConfig: | ||
| state_id = _required_attribute(element, "id") | ||
| child_elements = get_all_state_els(element) | ||
| states = accumulate_states(element) | ||
|
|
||
| result: StateNodeConfig = {"id": state_id} | ||
| if get_tag(element) == "parallel": | ||
| result["type"] = "parallel" | ||
| if states: | ||
| result["states"] = states | ||
| if get_tag(element) != "parallel": | ||
| result["initial"] = element.attrib.get("initial") or _required_attribute( | ||
| child_elements[0], "id" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Similarly to accumulate_states, we can avoid redundant _required_attribute calls here because the global _validate_state_ids pass has already validated all state IDs in the document.
def convert_state(element: ET.Element) -> StateNodeConfig:
state_id = element.attrib["id"]
child_elements = get_all_state_els(element)
states = accumulate_states(element)
result: StateNodeConfig = {"id": state_id}
if get_tag(element) == "parallel":
result["type"] = "parallel"
if states:
result["states"] = states
if get_tag(element) != "parallel":
result["initial"] = element.attrib.get("initial") or child_elements[0].attrib["id"]| transition_map: dict[ | ||
| str | None, | ||
| TransitionConfig | str | list[TransitionConfig | str], | ||
| ] = {} | ||
| for transition_element in _children(element, frozenset({"transition"})): | ||
| transition = convert_transition(transition_element) | ||
| event = transition_element.attrib.get("event") | ||
| bucket = transition_map.setdefault(event, []) | ||
| if not isinstance(bucket, list): | ||
| raise AssertionError("SCXML transition bucket must be a list") | ||
| bucket.append(transition) | ||
| if transition_map: | ||
| result["on"] = transition_map |
There was a problem hiding this comment.
We can simplify the transition_map typing and construction to avoid the runtime isinstance check and AssertionError (which are only there to satisfy MyPy). Typing transition_map as dict[str | None, list[TransitionConfig]] and casting it when assigning to result["on"] is cleaner and more idiomatic.
| transition_map: dict[ | |
| str | None, | |
| TransitionConfig | str | list[TransitionConfig | str], | |
| ] = {} | |
| for transition_element in _children(element, frozenset({"transition"})): | |
| transition = convert_transition(transition_element) | |
| event = transition_element.attrib.get("event") | |
| bucket = transition_map.setdefault(event, []) | |
| if not isinstance(bucket, list): | |
| raise AssertionError("SCXML transition bucket must be a list") | |
| bucket.append(transition) | |
| if transition_map: | |
| result["on"] = transition_map | |
| transition_map: dict[str | None, list[TransitionConfig]] = {} | |
| for transition_element in _children(element, frozenset({"transition"})): | |
| transition = convert_transition(transition_element) | |
| event = transition_element.attrib.get("event") | |
| transition_map.setdefault(event, []).append(transition) | |
| if transition_map: | |
| result["on"] = cast(Any, transition_map) |
| def convert_transition(element: ET.Element) -> TransitionConfig: | ||
| result: TransitionConfig = {} | ||
| target = element.attrib.get("target") | ||
| if target is not None: | ||
| result["target"] = [f"#{target_id}" for target_id in target.split()] |
There was a problem hiding this comment.
If the target attribute is present but empty or contains only whitespace, target.split() will return an empty list []. In XState, a transition without targets is a targetless transition and should omit the target key entirely. We should only set result["target"] if there is at least one target ID after splitting.
| def convert_transition(element: ET.Element) -> TransitionConfig: | |
| result: TransitionConfig = {} | |
| target = element.attrib.get("target") | |
| if target is not None: | |
| result["target"] = [f"#{target_id}" for target_id in target.split()] | |
| def convert_transition(element: ET.Element) -> TransitionConfig: | |
| result: TransitionConfig = {} | |
| target = element.attrib.get("target") | |
| if target is not None: | |
| targets = [f"#{target_id}" for target_id in target.split()] | |
| if targets: | |
| result["target"] = targets |
1 participant
Summary
MachineConfig,StateNodeConfig, andTransitionConfigconstructionxstate.scxmlcondattributes to canonical XStateguardhandlersInvalidConfigErrorWhy
The safe condition evaluator no longer uses dynamic JavaScript evaluation, but the importer was still excluded from MyPy under an obsolete comment. It also generated legacy
condconfigurations, producing 117 deprecation warnings in the configured SCXML suite.Malformed documents could leak incidental exceptions such as
IndexError, and duplicate state IDs could make global transition targets ambiguous.Behavior
scxml_to_machine(path)true,false,!,&&,||, and parenthesesInvalidConfigErrorNew regressions
Validation
poetry run python -m pytest tests/test_scxml_import.py -q(10 passed)poetry run python -m pytest tests/test_scxml_import.py tests/test_scxml.py -W error(64 passed)poetry run python -m pytest tests/ --ignore=tests/test_scxml.py(412 passed)poetry run mypy src/xstate/poetry run ruff format --check src/ tests/ docs/examples/poetry run ruff check src/ tests/ docs/examples/git diff --check