From 62ecd82f3c910a22c2b22391336ec4a29aa23949 Mon Sep 17 00:00:00 2001 From: Allan Scott Date: Tue, 2 Jun 2026 17:40:55 -0400 Subject: [PATCH 1/3] fix(runtime-python): resolve QA findings RT-06/07/12/14 Four confirmed runtime bugs from reports/python-runtime-qa-report.md, all reproduced end-to-end and covered by regression tests in tests/test_qa_fixes.py (9 tests). Full suite: 115 passed (was 106). - RT-12 (HIGH): thread the triggering Event through guard evaluation so `event.*` guards resolve against the payload instead of None. - RT-14 (MED): ordered comparisons (< > <= >=) fail closed on None/non-numeric operands instead of falling back to lexicographic string compare. RT-14 was masking RT-12, so the two are fixed together. - RT-06 (MED): run a state's on_entry before starting its invoked child (XState order) instead of dropping on_entry via an early return. - RT-07 (MED-HIGH): resume()/restore() rehydrate invoked child machines and active_invoke via _resume_children(), so a machine that crashed inside an invoke state is no longer permanently wedged. Sibling defs must be re-registered before resume; a missing sibling is skipped with a warning. Fixes documented in docs/runtime-python-production-hardening.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/runtime-python-production-hardening.md | 72 +++++ .../orca_runtime_python/machine.py | 197 +++++++++--- .../runtime-python/tests/test_qa_fixes.py | 293 ++++++++++++++++++ reports/python-runtime-qa-report.md | 242 +++++++++++++++ 4 files changed, 762 insertions(+), 42 deletions(-) create mode 100644 packages/runtime-python/tests/test_qa_fixes.py create mode 100644 reports/python-runtime-qa-report.md diff --git a/docs/runtime-python-production-hardening.md b/docs/runtime-python-production-hardening.md index bc9cd10..531dabf 100644 --- a/docs/runtime-python-production-hardening.md +++ b/docs/runtime-python-production-hardening.md @@ -363,5 +363,77 @@ When an `on_entry` action has `has_effect=True`, the effect result data is (a) m | M-1 | Minor | `parser.py` | Post-parse structural validation with informative `ParseError` messages | | M-2 | Minor | `machine.py` | `to_state_leaf` field on `TransitionResult` | | M-3 | Minor | `README.md` | Document `EFFECT_COMPLETED` event payload structure | +| RT-12 | **Required** | `machine.py` | Thread the triggering event through guard evaluation so `event.*` guards resolve against the payload | +| RT-14 | **Required** | `machine.py` | Ordered guard comparisons fail closed on `None`/non-numeric operands instead of lexicographic fallback | +| RT-06 | Recommended | `machine.py` | Run `on_entry` before starting an invoked child, instead of dropping it | +| RT-07 | Recommended | `machine.py` | `resume()`/`restore()` rehydrate invoked child machines and `active_invoke` | Gaps 1 and 2 should ship together — both are required for any deployment that persists machine state externally. Gaps 3 and 4 can follow independently. The minor improvements are best batched into a single small PR to avoid noise. + +--- + +## Runtime correctness fixes — QA report (RT-06, RT-07, RT-12, RT-14) + +Four runtime bugs surfaced by the QA pass in `reports/python-runtime-qa-report.md`. +All four were reproduced end-to-end through `parse_orca_md` + `OrcaMachine` and are +covered by regression tests in `tests/test_qa_fixes.py`. RT-12 and RT-14 interact +(RT-14 masked RT-12) and were fixed together. + +### RT-12 — `event.*` guards resolve against the event payload + +**Classification: Required fix** + +A guard such as `event.amount > 100` parsed to a path `["event", "amount"]`, but +guard evaluation took no event and `_resolve_variable` walked `self.context` only, +so the reference resolved to `context["event"]` → `None`. The guard was insensitive +to the payload (and, via RT-14, tended to pass). + +**Fix**: the triggering `Event` is threaded through `_evaluate_guard` → +`_eval_guard` → `_eval_compare`/`_eval_nullcheck` → `_resolve_variable`. A variable +path whose leading segment is `event` or `payload` now resolves against the event's +payload; everything else resolves against context as before. An `event.*` reference +with no event in scope (or no matching payload key) resolves to `None`. + +### RT-14 — ordered comparisons fail closed + +**Classification: Required fix** + +`_eval_compare` fell back to a lexicographic `str(lhs) > str(rhs)` when an operand +was not numeric. So ` > 100` became `"None" > "100"` → `True` — an ordered +guard over a missing/null context field passed spuriously (fail **open**). + +**Fix**: ordered comparisons (`lt`/`gt`/`le`/`ge`) require both operands to be +numeric (numeric-looking strings are still coerced via `float()`); if either operand +is `None` or non-numeric the comparison evaluates to `False` (fail **closed**). +Equality (`eq`/`ne`) is unchanged — it is well-defined on mixed types. + +### RT-06 — `on_entry` runs alongside `invoke` + +**Classification: Recommended fix** + +`_execute_entry_actions` returned early when a state declared `invoke`, silently +discarding that state's `on_entry` action. + +**Fix**: the entry action runs first, then the child machine is started — matching +XState, where entry actions precede invoked services. A state may declare both; +neither is dropped. (Entry-action execution was factored into `_run_on_entry`.) + +### RT-07 — `resume()`/`restore()` rehydrate children and `active_invoke` + +**Classification: Recommended fix** + +`snapshot()` already persisted `children` and `active_invoke`, but `resume()` and +`restore()` restored only state, context, and timeouts. A machine that crashed +inside an invoke state resumed with no child running and `active_invoke = None` — +nothing could drive it to `on_done`, so it was permanently wedged. + +**Fix**: `_resume_children()` restores `active_invoke` and, for each child snapshot, +re-instantiates the child from its sibling definition, re-attaches the completion +handler (factored out as `_make_child_done_handler`), and recursively `resume()`s it +from its own snapshot. Both `resume()` and `restore()` call it. + +**Precondition** (resume/persistence contract): sibling definitions +(`register_machines`) and this machine's action handlers must be re-registered +*before* `resume()`/`restore()`. A child snapshot whose target machine is not a +registered sibling (or whose state has no invoke definition) is skipped with a +`UserWarning` rather than silently dropped or raised. diff --git a/packages/runtime-python/orca_runtime_python/machine.py b/packages/runtime-python/orca_runtime_python/machine.py index 540f30a..27db0c6 100644 --- a/packages/runtime-python/orca_runtime_python/machine.py +++ b/packages/runtime-python/orca_runtime_python/machine.py @@ -173,6 +173,9 @@ async def restore(self, snap: dict[str, Any]) -> None: self._state = StateValue(copy.deepcopy(snap["state"])) self.context = copy.deepcopy(snap["context"]) + # Rehydrate invoked child machines and the active-invoke marker. + await self._resume_children(snap) + # If machine was active, restart timeouts for current leaf states if self._active: for leaf in self._state.leaves(): @@ -241,6 +244,11 @@ async def resume(self, snap: dict[str, Any]) -> None: } )) + # Rehydrate invoked child machines and the active-invoke marker, so a + # machine that crashed inside an invoke state resumes with its child + # running rather than wedged with active_invoke=None. + await self._resume_children(snap) + for leaf in self._state.leaves(): self._start_timeout_for_state(leaf) @@ -283,11 +291,22 @@ async def start_child_machine(self, state_name: str, invoke_def: InvokeDef) -> N ) self._child_machines[state_name] = child self._active_invoke = state_name + child.on_transition = self._make_child_done_handler(state_name, invoke_def) + await child.start() - # Set up completion/error listeners + def _make_child_done_handler(self, state_name: str, invoke_def: InvokeDef) -> TransitionCallback: + """Build the on_transition handler that fires `on_done` when an invoked + child reaches a final state, then stops and detaches it. + + Shared by the start path (`start_child_machine`) and the resume path + (`_resume_children`) so a rehydrated child completes the same way. + """ async def on_transition_handler(old: StateValue, new: StateValue) -> None: if new.is_compound(): return + child = self._child_machines.get(state_name) + if child is None: + return child_state = new.leaf() child_state_def = child._find_state_def(child_state) if child_state_def and child_state_def.is_final: @@ -303,8 +322,61 @@ async def on_transition_handler(old: StateValue, new: StateValue) -> None: if self._active_invoke == state_name: self._active_invoke = None - child.on_transition = on_transition_handler - await child.start() + return on_transition_handler + + async def _resume_children(self, snap: dict[str, Any]) -> None: + """Rehydrate invoked child machines and the active-invoke marker. + + Restores `_active_invoke` and, for each child snapshot under + `snap["children"]`, re-instantiates the child machine from its sibling + definition, re-attaches the completion handler, and resumes it from its + own snapshot (recursively). + + Preconditions: sibling definitions (`register_machines`) must be + registered before resume()/restore(). A child snapshot whose state has no + invoke definition, or whose target machine is not a registered sibling, + is skipped with a warning rather than silently dropped. + """ + import warnings + + # Clear any pre-existing children before rehydrating from the snapshot. + for existing in list(self._child_machines.values()): + await existing.stop() + self._child_machines.clear() + + self._active_invoke = snap.get("active_invoke") + children = snap.get("children") or {} + siblings = self._sibling_machines or {} + + for state_name, child_snap in children.items(): + state_def = self._find_state_def(state_name) + invoke_def = state_def.invoke if state_def else None + if invoke_def is None: + warnings.warn( + f"Cannot rehydrate child for state '{state_name}' in machine " + f"'{self.definition.name}': no invoke definition found.", + UserWarning, + stacklevel=2, + ) + continue + if invoke_def.machine not in siblings: + warnings.warn( + f"Cannot rehydrate child '{invoke_def.machine}' for state " + f"'{state_name}' in machine '{self.definition.name}': sibling " + "definition not registered (call register_machines before resume).", + UserWarning, + stacklevel=2, + ) + continue + child_def = siblings[invoke_def.machine] + child = OrcaMachine( + definition=child_def, + event_bus=self.event_bus, + context=dict(child_def.context), + ) + self._child_machines[state_name] = child + child.on_transition = self._make_child_done_handler(state_name, invoke_def) + await child.resume(child_snap) async def _invoke_foreign(self, invoke_def: InvokeDef) -> None: """Dispatch an invoke to a foreign (other-tool) child over the bridge. @@ -469,7 +541,7 @@ async def send( last_guard_name = None for candidate in candidates: if candidate.guard: - guard_passed = await self._evaluate_guard(candidate.guard) + guard_passed = await self._evaluate_guard(candidate.guard, evt) if guard_passed: transition = candidate break @@ -751,38 +823,55 @@ async def _check_parallel_sync(self) -> None: if self.on_transition: await self.on_transition(old_state, self._state) - async def _evaluate_guard(self, guard_name: str) -> bool: - """Evaluate a guard by name.""" + async def _evaluate_guard(self, guard_name: str, event: Event | None = None) -> bool: + """Evaluate a guard by name. + + `event` is the triggering event; it is threaded through so guards may + reference the event payload (e.g. `event.amount > 100`). + """ # Guards are defined in definition.guards if guard_name not in self.definition.guards: return True # Unknown guard = allow # Evaluate the guard expression guard_expr = self.definition.guards[guard_name] - return await self._eval_guard(guard_expr) + return await self._eval_guard(guard_expr, event) - async def _eval_guard(self, expr: GuardExpression) -> bool: - """Evaluate a guard expression against the machine context.""" + async def _eval_guard(self, expr: GuardExpression, event: Event | None = None) -> bool: + """Evaluate a guard expression against the machine context and event.""" if isinstance(expr, GuardTrue): return True if isinstance(expr, GuardFalse): return False if isinstance(expr, GuardNot): - return not await self._eval_guard(expr.expr) + return not await self._eval_guard(expr.expr, event) if isinstance(expr, GuardAnd): - return await self._eval_guard(expr.left) and await self._eval_guard(expr.right) + return await self._eval_guard(expr.left, event) and await self._eval_guard(expr.right, event) if isinstance(expr, GuardOr): - return await self._eval_guard(expr.left) or await self._eval_guard(expr.right) + return await self._eval_guard(expr.left, event) or await self._eval_guard(expr.right, event) if isinstance(expr, GuardCompare): - return self._eval_compare(expr.op, expr.left, expr.right) + return self._eval_compare(expr.op, expr.left, expr.right, event) if isinstance(expr, GuardNullcheck): - return self._eval_nullcheck(expr.expr, expr.is_null) + return self._eval_nullcheck(expr.expr, expr.is_null, event) return True - def _resolve_variable(self, ref: VariableRef) -> Any: - """Resolve a variable path against the machine context.""" - current: Any = self.context - for part in ref.path: + def _resolve_variable(self, ref: VariableRef, event: Event | None = None) -> Any: + """Resolve a variable path against the machine context or triggering event. + + A path whose leading segment is `event` or `payload` resolves against the + triggering event's payload; otherwise it resolves against the machine + context. A leading `ctx` / `context` segment is the (optional) explicit + context prefix. If an `event.*` guard fires with no event in scope (or no + matching payload key), the reference resolves to None. + """ + path = ref.path + if path and path[0] in ("event", "payload"): + current: Any = event.payload if event is not None else None + rest = path[1:] + else: + current = self.context + rest = path + for part in rest: # Skip "ctx" or "context" prefix — context is already the root if part in ("ctx", "context"): continue @@ -798,54 +887,78 @@ def _resolve_value(self, ref: ValueRef) -> Any: """Resolve a ValueRef to its Python value.""" return ref.value - def _eval_compare(self, op: str, left: VariableRef, right: "ValueRef | VariableRef") -> bool: - """Evaluate a comparison guard.""" - lhs = self._resolve_variable(left) - rhs = self._resolve_variable(right) if isinstance(right, VariableRef) else self._resolve_value(right) + def _resolve_operand(self, ref: "ValueRef | VariableRef", event: Event | None) -> Any: + """Resolve either side of a comparison to a concrete value.""" + if isinstance(ref, VariableRef): + return self._resolve_variable(ref, event) + return self._resolve_value(ref) - # Try numeric comparison - try: - lnum = float(lhs) if not isinstance(lhs, (int, float)) else lhs - rnum = float(rhs) if not isinstance(rhs, (int, float)) else rhs - both_numeric = True - except (TypeError, ValueError): - both_numeric = False - lnum = rnum = 0 + def _eval_compare( + self, + op: str, + left: VariableRef, + right: "ValueRef | VariableRef", + event: Event | None = None, + ) -> bool: + """Evaluate a comparison guard. + + Equality (`eq`/`ne`) is well-defined on mixed types and is compared + directly. Ordered comparisons (`lt`/`gt`/`le`/`ge`) require BOTH operands + to be numeric (numeric-looking strings are coerced); if either operand is + None or non-numeric the guard evaluates to False (fail closed) rather than + silently falling back to a lexicographic string compare. + """ + lhs = self._resolve_operand(left, event) + rhs = self._resolve_operand(right, event) if op == "eq": return lhs == rhs if op == "ne": return lhs != rhs + + # Ordered comparisons: require numeric operands, else fail closed. + try: + lnum = float(lhs) if not isinstance(lhs, (int, float)) else lhs + rnum = float(rhs) if not isinstance(rhs, (int, float)) else rhs + except (TypeError, ValueError): + return False + if op == "lt": - return lnum < rnum if both_numeric else str(lhs) < str(rhs) + return lnum < rnum if op == "gt": - return lnum > rnum if both_numeric else str(lhs) > str(rhs) + return lnum > rnum if op == "le": - return lnum <= rnum if both_numeric else str(lhs) <= str(rhs) + return lnum <= rnum if op == "ge": - return lnum >= rnum if both_numeric else str(lhs) >= str(rhs) + return lnum >= rnum return False - def _eval_nullcheck(self, expr: VariableRef, is_null: bool) -> bool: + def _eval_nullcheck(self, expr: VariableRef, is_null: bool, event: Event | None = None) -> bool: """Evaluate a null check guard.""" - val = self._resolve_variable(expr) + val = self._resolve_variable(expr, event) value_is_null = val is None return value_is_null if is_null else not value_is_null async def _execute_entry_actions(self, state_name: str) -> None: - """Execute on_entry action for a state.""" + """Execute a state's on_entry action, then start any invoked child machine. + + A state may declare BOTH `on_entry` and `invoke`. The entry action runs + first (matching XState, where entry actions precede invoked services), + then the child machine is started — neither is dropped. + """ state_def = self._find_state_def(state_name) if not state_def: return - # Handle invoke - start child machine if present + if state_def.on_entry: + await self._run_on_entry(state_def) + + # Start child machine if this state invokes one (after on_entry). if state_def.invoke: await self.start_child_machine(state_name, state_def.invoke) - return # Don't execute on_entry if invoke is set - - if not state_def.on_entry: - return + async def _run_on_entry(self, state_def: StateDef) -> None: + """Run a state's on_entry action (as an effect or a plain handler).""" action_def = self._find_action_def(state_def.on_entry) if action_def and action_def.has_effect: # Execute as effect via event bus diff --git a/packages/runtime-python/tests/test_qa_fixes.py b/packages/runtime-python/tests/test_qa_fixes.py new file mode 100644 index 0000000..d746a2b --- /dev/null +++ b/packages/runtime-python/tests/test_qa_fixes.py @@ -0,0 +1,293 @@ +"""Regression tests for the Python-runtime QA findings. + +Each test corresponds to a confirmed bug in +`reports/python-runtime-qa-report.md` (RT-06, RT-07, RT-12, RT-14) and is lifted +directly from the repro inlined in that report. + + RT-06 on_entry dropped when a state also declares `invoke` + RT-07 resume()/restore() drop child machines and `active_invoke` + RT-12 `event.*` guards parse but the event is never in guard scope + RT-14 ordered comparisons fall back to string compare and fail open +""" + +import asyncio +import warnings + +from orca_runtime_python.parser import parse_orca_md +from orca_runtime_python.machine import OrcaMachine +from orca_runtime_python.bus import EventBus + + +# -------------------------------------------------------------------------- +# RT-12 — event.* guards resolve against the event payload +# -------------------------------------------------------------------------- + +RT12_MD = """# machine pay + +## events + +- PAY + +## state idle [initial] +## state approved [final] +## state denied [final] + +## guards + +| Name | Expression | +|------|------------| +| big | `event.amount > 100` | + +## transitions + +| Source | Event | Guard | Target | +|--------|-------|-------|--------| +| idle | PAY | big | approved | +| idle | PAY | | denied | +""" + + +async def _test_rt12_event_guard_denies_small_payload(): + m = OrcaMachine(parse_orca_md(RT12_MD), event_bus=EventBus()) + await m.start() + await m.send("PAY", {"amount": 5}) + assert m.state == "denied", f"event.amount>100 with amount=5 should deny, got {m.state}" + + +async def _test_rt12_event_guard_approves_large_payload(): + m = OrcaMachine(parse_orca_md(RT12_MD), event_bus=EventBus()) + await m.start() + await m.send("PAY", {"amount": 200}) + assert m.state == "approved", f"event.amount>100 with amount=200 should approve, got {m.state}" + + +def test_rt12_event_guard_denies_small_payload(): + asyncio.run(_test_rt12_event_guard_denies_small_payload()) + + +def test_rt12_event_guard_approves_large_payload(): + asyncio.run(_test_rt12_event_guard_approves_large_payload()) + + +# -------------------------------------------------------------------------- +# RT-14 — ordered comparisons fail closed on None / non-numeric operands +# -------------------------------------------------------------------------- + +RT14_MD = """# machine g + +## context + +| Field | Type | Default | +|-------|------|---------| +| x | number | | + +## events + +- GO + +## state idle [initial] +## state done [final] +## state blocked [final] + +## guards + +| Name | Expression | +|------|------------| +| big | `ctx.x > 100` | + +## transitions + +| Source | Event | Guard | Target | +|--------|-------|-------|--------| +| idle | GO | big | done | +| idle | GO | | blocked | +""" + + +async def _test_rt14_ordered_compare_none_fails_closed(): + m = OrcaMachine(parse_orca_md(RT14_MD), event_bus=EventBus(), context={"x": None}) + await m.start() + await m.send("GO") + assert m.state == "blocked", f"ctx.x>100 with x=None must be False (fail closed), got {m.state}" + + +async def _test_rt14_ordered_compare_nonnumeric_fails_closed(): + m = OrcaMachine(parse_orca_md(RT14_MD), event_bus=EventBus(), context={"x": "abc"}) + await m.start() + await m.send("GO") + assert m.state == "blocked", f"ctx.x>100 with x='abc' must be False (fail closed), got {m.state}" + + +async def _test_rt14_numeric_string_still_compares(): + # A numeric-looking string is still coerced and compared numerically — the + # fix must not over-reject these. + m = OrcaMachine(parse_orca_md(RT14_MD), event_bus=EventBus(), context={"x": "150"}) + await m.start() + await m.send("GO") + assert m.state == "done", f"ctx.x>100 with x='150' should pass, got {m.state}" + + +def test_rt14_ordered_compare_none_fails_closed(): + asyncio.run(_test_rt14_ordered_compare_none_fails_closed()) + + +def test_rt14_ordered_compare_nonnumeric_fails_closed(): + asyncio.run(_test_rt14_ordered_compare_nonnumeric_fails_closed()) + + +def test_rt14_numeric_string_still_compares(): + asyncio.run(_test_rt14_numeric_string_still_compares()) + + +# -------------------------------------------------------------------------- +# RT-06 / RT-07 — invoke + on_entry, and resume rehydration +# -------------------------------------------------------------------------- + +CHILD_MD = """# machine Child + +## events + +- FIN + +## state running [initial] +## state ok [final] + +## transitions + +| Source | Event | Target | +|--------|-------|--------| +| running | FIN | ok | +""" + +PARENT_MD = """# machine Parent + +## events + +- E +- DONE + +## state working [initial] +- invoke: Child +- on_done: DONE +- on_entry: mark_entry +## state idle [final] + +## transitions + +| Source | Event | Target | +|--------|-------|--------| +| working | DONE | idle | +| working | E | idle | +""" + + +async def _test_rt06_on_entry_runs_with_invoke(): + pdef, cdef = parse_orca_md(PARENT_MD), parse_orca_md(CHILD_MD) + m = OrcaMachine(pdef, event_bus=EventBus()) + m.register_machines({"Child": cdef}) + calls = [] + m.register_action("mark_entry", lambda ctx, ev: calls.append(1)) + await m.start() + assert calls == [1], f"on_entry must run exactly once alongside invoke, got {calls}" + assert "working" in m._child_machines, "child machine should have started" + assert m._active_invoke == "working", f"active_invoke should be 'working', got {m._active_invoke!r}" + + +def test_rt06_on_entry_runs_with_invoke(): + asyncio.run(_test_rt06_on_entry_runs_with_invoke()) + + +async def _test_rt07_resume_rehydrates_child_and_active_invoke(): + pdef, cdef = parse_orca_md(PARENT_MD), parse_orca_md(CHILD_MD) + parent = OrcaMachine(pdef, event_bus=EventBus()) + parent.register_machines({"Child": cdef}) + await parent.start() + snap = parent.snapshot() + assert list(snap["children"]) == ["working"] + assert snap["active_invoke"] == "working" + + m2 = OrcaMachine(pdef, event_bus=EventBus()) + m2.register_machines({"Child": cdef}) + await m2.resume(snap) + + assert m2.state == "working" + assert "working" in m2._child_machines, "child must be re-instantiated on resume" + assert m2._active_invoke == "working", f"active_invoke must be restored, got {m2._active_invoke!r}" + + +async def _test_rt07_resumed_machine_is_not_wedged(): + # The core impact: a machine resumed in an invoke state must still be + # drivable to on_done via its (rehydrated) child, not permanently stalled. + pdef, cdef = parse_orca_md(PARENT_MD), parse_orca_md(CHILD_MD) + parent = OrcaMachine(pdef, event_bus=EventBus()) + parent.register_machines({"Child": cdef}) + await parent.start() + snap = parent.snapshot() + + m2 = OrcaMachine(pdef, event_bus=EventBus()) + m2.register_machines({"Child": cdef}) + await m2.resume(snap) + + # Drive the rehydrated child to its final state → completion handler fires + # on_done ("DONE") on the parent, which transitions working -> idle. + await m2._child_machines["working"].send("FIN") + + assert m2.state == "idle", f"parent should reach idle via on_done, got {m2.state}" + assert "working" not in m2._child_machines, "completed child should be detached" + assert m2._active_invoke is None, "active_invoke should clear after completion" + + +async def _test_rt07_resume_without_sibling_warns_not_crashes(): + # Child snapshot present but sibling def not registered → warn + skip, + # rather than silently dropping or raising. + pdef, cdef = parse_orca_md(PARENT_MD), parse_orca_md(CHILD_MD) + parent = OrcaMachine(pdef, event_bus=EventBus()) + parent.register_machines({"Child": cdef}) + await parent.start() + snap = parent.snapshot() + + m2 = OrcaMachine(pdef, event_bus=EventBus()) # NOTE: no register_machines + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + await m2.resume(snap) + assert any("sibling" in str(w.message).lower() for w in caught), \ + "missing sibling should produce a warning" + assert m2._child_machines == {}, "no child should be rehydrated without its sibling def" + + +def test_rt07_resume_rehydrates_child_and_active_invoke(): + asyncio.run(_test_rt07_resume_rehydrates_child_and_active_invoke()) + + +def test_rt07_resumed_machine_is_not_wedged(): + asyncio.run(_test_rt07_resumed_machine_is_not_wedged()) + + +def test_rt07_resume_without_sibling_warns_not_crashes(): + asyncio.run(_test_rt07_resume_without_sibling_warns_not_crashes()) + + +if __name__ == "__main__": + tests = [ + ("RT-12 event guard denies small payload", test_rt12_event_guard_denies_small_payload), + ("RT-12 event guard approves large payload", test_rt12_event_guard_approves_large_payload), + ("RT-14 None fails closed", test_rt14_ordered_compare_none_fails_closed), + ("RT-14 non-numeric fails closed", test_rt14_ordered_compare_nonnumeric_fails_closed), + ("RT-14 numeric string still compares", test_rt14_numeric_string_still_compares), + ("RT-06 on_entry runs with invoke", test_rt06_on_entry_runs_with_invoke), + ("RT-07 resume rehydrates child + active_invoke", test_rt07_resume_rehydrates_child_and_active_invoke), + ("RT-07 resumed machine is not wedged", test_rt07_resumed_machine_is_not_wedged), + ("RT-07 resume without sibling warns", test_rt07_resume_without_sibling_warns_not_crashes), + ] + passed = failed = 0 + for name, fn in tests: + try: + fn() + print(f" PASS {name}") + passed += 1 + except Exception as e: + print(f" FAIL {name}: {e}") + failed += 1 + print(f"\n{passed} passed, {failed} failed, {passed + failed} total") + if failed: + exit(1) diff --git a/reports/python-runtime-qa-report.md b/reports/python-runtime-qa-report.md new file mode 100644 index 0000000..71615bc --- /dev/null +++ b/reports/python-runtime-qa-report.md @@ -0,0 +1,242 @@ +# QA Report — Python Runtime (`packages/runtime-python`) + +**Date:** 2026-06-02 +**Verified against:** `origin/main` `bf76c67` (v0.1.28). The implicated files are +byte-identical at the local `1743f37`, so line numbers below are stable for both. +**Scope:** `packages/runtime-python/orca_runtime_python/machine.py` (+ the guard +grammar in `packages/orca-lang/src/parser/markdown-parser.ts` and the Python +parser). +**Method:** every issue below was reproduced end-to-end through the real runtime +(`parse_orca_md` + `OrcaMachine`), not judged by reading alone. Repro scripts are +inlined and convert directly into regression tests. + +## Provenance / caveat + +This report originated while triaging an external bug list ("Hermes"). That list +also contained a tier of TypeScript-verifier findings (BUG-01..05, 08, 13, C-01) +that were investigated and **did not reproduce** at `bf76c67` — `StateDef` is +camelCase as `structural.ts` reads it, `MachineDef.effects` exists and is +populated, `flattenStates` has no duplicate push, `checkReachability` iterates the +flattened state map, and the TS verifier/parser **are** tested (`verifier.test.ts`, +`markdown-parser.test.ts`; 109 passing). **Do not action those.** This report is +scoped to the Python-runtime findings, which are real. + +## Summary + +| ID | Issue | Severity | Status | +|----|-------|----------|--------| +| RT-06 | `on_entry` silently dropped when a state also has `invoke` | MEDIUM | **Fixed** (regression test) | +| RT-07 | `resume()` / `restore()` drop child machines **and** `active_invoke` | MEDIUM-HIGH | **Fixed** (regression test) | +| RT-12 | `event.*` guards parse but are unresolvable — events never in guard scope | HIGH | **Fixed** (regression test) | +| RT-14 | Ordered guard comparisons (`<`,`>`,`<=`,`>=`) fall back to string compare on non-numeric / `None` → fail **open** | MEDIUM | **Fixed** (regression test) | + +**Resolution (2026-06-02):** All four fixed in `packages/runtime-python/orca_runtime_python/machine.py`, +with regression tests in `packages/runtime-python/tests/test_qa_fixes.py` (9 tests) and the fixes +documented in `docs/runtime-python-production-hardening.md`. RT-12 + RT-14 were fixed together (RT-14 +masked RT-12). Full suite: 115 passed. + +Severities are re-ranked from the source list: RT-12 fails *open* (a guard passes +when it should deny), which is a correctness/security landmine, not "LOW". + +None of these four paths are covered by the existing suite (no test combines +`invoke` + `on_entry`; `test_snapshot_restore.py` never reads `children` / +`active_invoke`; no `event.*` guard test). Current suite: 104 passed; the 2 +`test_bridge.py` failures are environmental (`async def` tests need +`pytest-asyncio`), not regressions. + +--- + +## RT-06 — `on_entry` dropped when a state also declares `invoke` + +**Location:** `machine.py:835-844`, `_execute_entry_actions`. + +```python +async def _execute_entry_actions(self, state_name: str) -> None: + state_def = self._find_state_def(state_name) + if not state_def: + return + if state_def.invoke: + await self.start_child_machine(state_name, state_def.invoke) + return # <-- on_entry is never reached + if not state_def.on_entry: + return + ... +``` + +**Root cause:** the `invoke` branch returns early, so a state with both `invoke` +and `on_entry` silently discards `on_entry`. Intentional in code, but undocumented +and contrary to XState semantics (entry actions run **before** services are +invoked). + +**Repro:** +```python +PARENT = """# machine Parent +## events +- E +## state working [initial] +- invoke: Child +- on_entry: mark_entry +## state idle [final] +## transitions +| Source | Event | Target | +|--------|-------|--------| +| working | E | idle | +""" +m = OrcaMachine(parse_orca_md(PARENT)); m.register_machines({"Child": child_def}) +calls = []; m.register_action("mark_entry", lambda ctx, ev: calls.append(1)) +await m.start() +# observed: calls == [] (on_entry NOT fired), child started, active_invoke == "working" +``` + +**Impact:** any state that both conditions a child machine and runs an entry +action loses the entry action with no error or warning. + +**Proposed fix:** run the `on_entry` action first, then start the invoke — i.e. +drop the early `return` and fall through to the on_entry block before (or after, +per chosen semantics, but XState says before) `start_child_machine`. Document the +ordering. + +**Regression test:** assert `mark_entry` ran exactly once *and* the child started. + +--- + +## RT-07 — `resume()` / `restore()` lose child machines and `active_invoke` + +**Location:** `snapshot()` `machine.py:143-161`; `resume()` `:204-245`; +`restore()` `:163-179`. + +`snapshot()` persists the relevant state: +```python +"children": {k: m.snapshot() for k, m in self._child_machines.items()}, # :158 +"active_invoke": self._active_invoke, # :159 +``` +…but `resume()` (and `restore()`) restore only `_state`, `context`, and timeouts. +They **never read `snap["children"]` or `snap["active_invoke"]`**. + +**Repro:** +```python +snap = parent.snapshot() +# snap["children"] == {"working": {...}}, snap["active_invoke"] == "working" +m2 = OrcaMachine(pdef); m2.register_machines({"Child": cdef}) +await m2.resume(snap) +# observed: m2.state.value == "working" (an invoke state) +# m2._child_machines == {} (child NOT re-instantiated) +# m2._active_invoke is None (invoke marker lost) +``` + +**Impact:** a machine that crashes inside an `invoke` state resumes *in* that state +with no child running and `active_invoke=None`. Nothing will ever drive it to +`on_done` → the machine is permanently stalled. This is worse than "child machines +are lost": the parent is wedged. Affects the production resume/persistence path. + +**Proposed fix (touches the resume contract — confirm before implementing):** +in `resume()`/`restore()`, after restoring state/context: +1. restore `self._active_invoke = snap.get("active_invoke")`; +2. for each `state_name, child_snap` in `snap.get("children", {})`, re-instantiate + the child `OrcaMachine` from the sibling definition, re-attach the completion + `on_transition` handler used in `start_child_machine`, and `resume(child_snap)` + it (recursive). + - Requires sibling defs (`register_machines`) and action handlers to be + re-registered *before* resume — document this precondition. +3. Decide handling when a child snapshot exists but its sibling def isn't + registered (warn vs raise). + +**Regression test:** snapshot a parent in an invoke state with a live child → +`resume()` into a fresh parent → assert the child is rehydrated and +`active_invoke` is restored. + +--- + +## RT-12 — `event.*` guards are writable but silently unresolvable + +**Location:** grammar `packages/orca-lang/src/parser/markdown-parser.ts:313-316` +(`parseVariablePath` accepts any `IDENT(.IDENT)*`); the Python parser likewise. +Evaluation: `_evaluate_guard(self, guard_name)` `machine.py:754` takes **no event**; +`_resolve_variable` `:782-795` walks `self.context` only. + +```python +def _resolve_variable(self, ref): + current = self.context + for part in ref.path: + if part in ("ctx", "context"): # only these prefixes are special-cased + continue + ... + current = current.get(part) if isinstance(current, dict) else getattr(current, part, None) + return current +``` + +A guard `event.amount > 100` parses to `path = ["event", "amount"]`, but the event +is never in scope, so it resolves to `context["event"]` → `None`. + +**Repro:** +```python +GUARD = "`event.amount > 100`" # in a ## guards table, used by idle--PAY-->approved +m = OrcaMachine(parse_orca_md(...)) +await m.start(); await m.send("PAY", {"amount": 5}) +# observed: state == "approved" (should be "denied"; the event payload is ignored) +# m._resolve_variable() == None +``` +Both `amount=5` and `amount=200` → `approved`, i.e. the guard is insensitive to the +payload entirely. (The spurious *pass* is due to RT-14 below.) + +**Impact:** guards that reference event payloads compile and run but ignore the +event — and, via RT-14, tend to **pass** rather than fail. Silent, fail-open +incorrect routing. This is the most dangerous of the four. + +**Proposed fix:** thread the triggering `Event` through guard evaluation +(`_evaluate_guard` / `_eval_guard` / `_eval_compare` / `_resolve_variable`) and +resolve a leading `event` / `payload` segment against the event's payload. If +event-referencing guards are *not* intended to be supported, the parser must +**reject** them rather than silently mis-resolve. + +**Regression test:** `event.amount > 100` with payload `amount=5` → `denied`; +`amount=200` → `approved`. + +--- + +## RT-14 — Ordered comparisons fall back to string compare and fail open + +**Location:** `_eval_compare` `machine.py:801-827`. + +```python +try: + lnum = float(lhs) ...; rnum = float(rhs) ...; both_numeric = True +except (TypeError, ValueError): + both_numeric = False +... +if op == "gt": + return lnum > rnum if both_numeric else str(lhs) > str(rhs) # <-- lexicographic fallback +``` + +**Root cause:** when an operand isn't numeric (e.g. an unset/`None` context field, or +a non-numeric value), `<`,`>`,`<=`,`>=` silently compare `str(lhs)` vs `str(rhs)`. +So ` > 100` becomes `"None" > "100"` → `True`. + +**Repro:** this is what makes RT-12's broken guard return `approved` — `str(None) > +str(100)` is `True`. Independently, any ordered guard over a missing/null context +field passes spuriously. + +**Impact:** ordered guards on null/unset/non-numeric values fail **open** (pass +when they should not). Easy to hit with an uninitialized context field. + +**Proposed fix:** for ordered comparisons, treat a non-numeric/`None` operand as the +guard being **false** (do not silently fall back to lexicographic compare), or +raise a typed guard-evaluation error. Keep `eq`/`ne` as-is (equality on mixed types +is well-defined). Document the chosen semantics in `docs/error-catalog.md`. + +**Regression test:** guard `ctx.x > 100` with `x` unset/`None` → guard is `False`. + +--- + +## Suggested order & acceptance + +1. **RT-12 + RT-14** together (they interact; RT-14 masks RT-12). Acceptance: the + RT-12 repro routes correctly for both payloads; null-operand ordered guards + evaluate `False`. +2. **RT-06.** Acceptance: `on_entry` fires once before/with `invoke`; documented. +3. **RT-07.** Acceptance: round-trip snapshot→resume of an invoke-state machine + rehydrates the child and `active_invoke`. **Confirm the resume/persistence + contract change first** (re-registration preconditions, missing-sibling policy). + +All four come with the repros above; lifting them into +`tests/test_*.py` closes the coverage gap that let these ship. From 2cc446e2ad41dcb0f96266ece975c1dcfd0228b8 Mon Sep 17 00:00:00 2001 From: Allan Scott Date: Tue, 2 Jun 2026 17:50:47 -0400 Subject: [PATCH 2/3] review: clarify ctx-prefix docstring, actionable sibling warning, add mixed-operand guard test Addresses PR #16 review nits: - _resolve_variable docstring: state that ctx/context segments are skipped (ctx.amount and amount are equivalent). - _resume_children missing-sibling warning now names the exact register_machines({...}) call to make it actionable. - Add test_rt12_mixed_event_and_ctx_operands: a single comparison (event.amount > ctx.limit) that exercises both the event-payload and context resolution paths, guarding the resolution logic against regression. Suite: 116 passed (10 in test_qa_fixes.py). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../orca_runtime_python/machine.py | 9 ++-- .../runtime-python/tests/test_qa_fixes.py | 50 +++++++++++++++++++ reports/python-runtime-qa-report.md | 4 +- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/packages/runtime-python/orca_runtime_python/machine.py b/packages/runtime-python/orca_runtime_python/machine.py index 27db0c6..97c6fe6 100644 --- a/packages/runtime-python/orca_runtime_python/machine.py +++ b/packages/runtime-python/orca_runtime_python/machine.py @@ -363,7 +363,9 @@ async def _resume_children(self, snap: dict[str, Any]) -> None: warnings.warn( f"Cannot rehydrate child '{invoke_def.machine}' for state " f"'{state_name}' in machine '{self.definition.name}': sibling " - "definition not registered (call register_machines before resume).", + "definition not registered. Call " + f"register_machines({{'{invoke_def.machine}': }}) " + "before resume()/restore().", UserWarning, stacklevel=2, ) @@ -860,8 +862,9 @@ def _resolve_variable(self, ref: VariableRef, event: Event | None = None) -> Any A path whose leading segment is `event` or `payload` resolves against the triggering event's payload; otherwise it resolves against the machine - context. A leading `ctx` / `context` segment is the (optional) explicit - context prefix. If an `event.*` guard fires with no event in scope (or no + context. Any `ctx` / `context` path segment is treated as the explicit + context-root prefix and skipped (so `ctx.amount` and `amount` are + equivalent). If an `event.*` guard fires with no event in scope (or no matching payload key), the reference resolves to None. """ path = ref.path diff --git a/packages/runtime-python/tests/test_qa_fixes.py b/packages/runtime-python/tests/test_qa_fixes.py index d746a2b..4eec9c7 100644 --- a/packages/runtime-python/tests/test_qa_fixes.py +++ b/packages/runtime-python/tests/test_qa_fixes.py @@ -61,6 +61,51 @@ async def _test_rt12_event_guard_approves_large_payload(): assert m.state == "approved", f"event.amount>100 with amount=200 should approve, got {m.state}" +RT12_MIXED_MD = """# machine mixed + +## context + +| Field | Type | Default | +|-------|------|---------| +| limit | number | 100 | + +## events + +- PAY + +## state idle [initial] +## state approved [final] +## state denied [final] + +## guards + +| Name | Expression | +|------|------------| +| over | `event.amount > ctx.limit` | + +## transitions + +| Source | Event | Guard | Target | +|--------|-------|-------|--------| +| idle | PAY | over | approved | +| idle | PAY | | denied | +""" + + +async def _test_rt12_mixed_event_and_ctx_operands(): + # One comparison touches BOTH resolution paths: LHS is an event payload + # field, RHS is a context field. Guards both that event and ctx resolve. + over = OrcaMachine(parse_orca_md(RT12_MIXED_MD), event_bus=EventBus(), context={"limit": 100}) + await over.start() + await over.send("PAY", {"amount": 200}) + assert over.state == "approved", f"event.amount(200) > ctx.limit(100) should approve, got {over.state}" + + under = OrcaMachine(parse_orca_md(RT12_MIXED_MD), event_bus=EventBus(), context={"limit": 100}) + await under.start() + await under.send("PAY", {"amount": 50}) + assert under.state == "denied", f"event.amount(50) > ctx.limit(100) should deny, got {under.state}" + + def test_rt12_event_guard_denies_small_payload(): asyncio.run(_test_rt12_event_guard_denies_small_payload()) @@ -69,6 +114,10 @@ def test_rt12_event_guard_approves_large_payload(): asyncio.run(_test_rt12_event_guard_approves_large_payload()) +def test_rt12_mixed_event_and_ctx_operands(): + asyncio.run(_test_rt12_mixed_event_and_ctx_operands()) + + # -------------------------------------------------------------------------- # RT-14 — ordered comparisons fail closed on None / non-numeric operands # -------------------------------------------------------------------------- @@ -271,6 +320,7 @@ def test_rt07_resume_without_sibling_warns_not_crashes(): tests = [ ("RT-12 event guard denies small payload", test_rt12_event_guard_denies_small_payload), ("RT-12 event guard approves large payload", test_rt12_event_guard_approves_large_payload), + ("RT-12 mixed event.* and ctx.* operands", test_rt12_mixed_event_and_ctx_operands), ("RT-14 None fails closed", test_rt14_ordered_compare_none_fails_closed), ("RT-14 non-numeric fails closed", test_rt14_ordered_compare_nonnumeric_fails_closed), ("RT-14 numeric string still compares", test_rt14_numeric_string_still_compares), diff --git a/reports/python-runtime-qa-report.md b/reports/python-runtime-qa-report.md index 71615bc..e406fa4 100644 --- a/reports/python-runtime-qa-report.md +++ b/reports/python-runtime-qa-report.md @@ -31,9 +31,9 @@ scoped to the Python-runtime findings, which are real. | RT-14 | Ordered guard comparisons (`<`,`>`,`<=`,`>=`) fall back to string compare on non-numeric / `None` → fail **open** | MEDIUM | **Fixed** (regression test) | **Resolution (2026-06-02):** All four fixed in `packages/runtime-python/orca_runtime_python/machine.py`, -with regression tests in `packages/runtime-python/tests/test_qa_fixes.py` (9 tests) and the fixes +with regression tests in `packages/runtime-python/tests/test_qa_fixes.py` (10 tests) and the fixes documented in `docs/runtime-python-production-hardening.md`. RT-12 + RT-14 were fixed together (RT-14 -masked RT-12). Full suite: 115 passed. +masked RT-12). Full suite: 116 passed. Severities are re-ranked from the source list: RT-12 fails *open* (a guard passes when it should deny), which is a correctness/security landmine, not "LOW". From 9649a60ed0ed543035f646f63adadfebab3e1948 Mon Sep 17 00:00:00 2001 From: Allan Scott Date: Tue, 2 Jun 2026 18:03:16 -0400 Subject: [PATCH 3/3] release: changeset + changelog for v0.1.29 (runtime-python QA fixes) Adds the changeset that drives CI to bump the whole release train to v0.1.29 (the fixed npm group bumps together; CI then syncs pyproject.toml + server.json and tags v0.1.29, triggering the publish workflow). Also bumps the runtime-python __init__.__version__ to match and adds a root CHANGELOG entry for the GitHub release notes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/runtime-python-qa-fixes.md | 14 ++++++++++++++ CHANGELOG.md | 15 +++++++++++++++ .../orca_runtime_python/__init__.py | 2 +- 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 .changeset/runtime-python-qa-fixes.md diff --git a/.changeset/runtime-python-qa-fixes.md b/.changeset/runtime-python-qa-fixes.md new file mode 100644 index 0000000..4d6fad2 --- /dev/null +++ b/.changeset/runtime-python-qa-fixes.md @@ -0,0 +1,14 @@ +--- +"@orcalang/orca-lang": patch +--- + +v0.1.29 — runtime-python QA fixes (RT-06/07/12/14). + +The functional changes are in `orca-runtime-python`: `event.*` guards now resolve +against the event payload (e.g. `event.amount > 100`), ordered comparisons fail +closed on null/non-numeric operands instead of a lexicographic fallback, `on_entry` +runs alongside `invoke` instead of being dropped, and `resume()`/`restore()` +rehydrate invoked child machines and `active_invoke` (so a machine that crashed +inside an invoke state is no longer wedged). The npm packages are version-bumped to +keep the release train in lockstep — no functional changes to them. See +`reports/python-runtime-qa-report.md`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 785f4c0..1b80775 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,21 @@ This project uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). --- +## [v0.1.29] — 2026-06-02 + +### Fixed + +- `packages/runtime-python`: four QA findings from `reports/python-runtime-qa-report.md` (RT-06/07/12/14), reproduced end-to-end and covered by 10 regression tests in `tests/test_qa_fixes.py`: + - **RT-12** (HIGH): `event.*` guards now resolve against the triggering event's payload (e.g. `event.amount > 100`) instead of `None`. The event is threaded through `_evaluate_guard → _eval_guard → _eval_compare/_eval_nullcheck → _resolve_variable`. + - **RT-14** (MED): ordered comparisons (`< > <= >=`) fail **closed** on `None`/non-numeric operands instead of falling back to lexicographic string compare (which masked RT-12). Numeric-looking strings still coerce; `eq`/`ne` unchanged. + - **RT-06** (MED): a state's `on_entry` runs **before** its invoked child starts (XState order) instead of being dropped by an early return. + - **RT-07** (MED-HIGH): `resume()`/`restore()` rehydrate invoked child machines and `active_invoke`, so a machine that crashed inside an `invoke` state is no longer permanently wedged. Sibling defs must be re-registered before resume; a missing sibling is skipped with a `UserWarning`. +- Full runtime-python suite: 116 passed. + +The npm packages (`@orcalang/orca-lang`, `@orcalang/orca-runtime-ts`, `@orcalang/orca-mcp-server`) are version-bumped to keep the release train in lockstep — no functional changes to them. + +--- + ## [v0.1.18] — 2026-03-30 ### Added diff --git a/packages/runtime-python/orca_runtime_python/__init__.py b/packages/runtime-python/orca_runtime_python/__init__.py index 7b875d9..f283fa3 100644 --- a/packages/runtime-python/orca_runtime_python/__init__.py +++ b/packages/runtime-python/orca_runtime_python/__init__.py @@ -46,7 +46,7 @@ from .logging import LogSink, FileSink, ConsoleSink, MultiSink -__version__ = "0.1.28" +__version__ = "0.1.29" __all__ = [ # Types