Tighten _to_serializable docs and drop casts that were not load-bearing

The docstring promised a "JSON-safe structure", which the function does
not guarantee: values reached through dict[str, Any] fields keep their
original behavior, and a few shapes -- a tuple holding a datetime, for
one -- were never JSON-encodable. Say that plainly rather than implying
every output can be encoded.

The list and dict branches test value_type rather than the value, so the
value is never narrowed and iterating it directly is already clean under
strict type checking. The casts there did nothing but call typing.cast
on every container.

The cast on the type lookup is load-bearing and now says so. type(value)
is type[Unknown] to a checker because value is Any. Two details that
look like noise are deliberate: the quoted annotation avoids building a
throwaway GenericAlias per call, and the lookup stays type(value) rather
than the cheaper value.__class__ because asdict dispatched on type(obj).
Dispatching on __class__ would let an object claiming to be a str take
the JSON-native fast path and be returned by reference, where the
original pipeline deep-copied it. A regression test pins that, and fails
if the swap is made.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6322dc7f-81ee-4d42-be2d-cef84cf62d15

Author: Bernd Verst

durabletask/history.py

@@ -389,7 +389,13 @@ def _legacy_compat(value: Any) -> Any:
 
 
 def _to_serializable(value: Any) -> Any:
-    """Recursively convert *value* into a JSON-safe structure.
+    """Recursively convert *value* into the form history export writes.
+
+    Values the SDK itself produces convert to JSON-native types. Arbitrary
+    values can also arrive through ``dict[str, Any]`` fields, and those keep
+    whatever the original pipeline did with them -- which for a few shapes,
+    such as a tuple holding a ``datetime``, is not JSON-encodable. That is
+    preserved on purpose rather than fixed here; see :func:`_legacy_compat`.
 
     This walks dataclass instances directly instead of going through
     ``dataclasses.asdict``, which would deep-copy the whole event graph
@@ -404,6 +410,14 @@ def _to_serializable(value: Any) -> Any:
     so their original semantics -- constructor round-trips, key
     recursion and deep-copied leaves -- are preserved exactly.
     """
+    # ``type(value)`` is ``type[Unknown]`` to a type checker because *value*
+    # is ``Any``, so the cast is what keeps this module clean under strict
+    # checking. Two details are deliberate: the annotation is quoted, since
+    # an unquoted ``type[Any]`` is evaluated on every call and builds a
+    # throwaway ``types.GenericAlias``; and the lookup is ``type(value)``
+    # rather than the cheaper ``value.__class__``, because ``asdict`` used
+    # ``type(obj)`` and an object overriding ``__class__`` would otherwise
+    # be dispatched differently than it was before.
     value_type = cast('type[Any]', type(value))
     if value_type in _JSON_NATIVE_TYPES:
         return value
@@ -417,15 +431,15 @@ def _to_serializable(value: Any) -> Any:
     if value_type is datetime:
         return value.isoformat()
     if value_type is list:
-        return [_to_serializable(item) for item in cast(list[Any], value)]
+        return [_to_serializable(item) for item in value]
     if value_type is dict:
         # ``asdict`` recursed into keys but the conversion pass that followed
         # it did not, so keys get ``asdict`` semantics only. Native keys are
         # returned as-is because ``asdict`` leaves those untouched too.
         return {
             (key if type(key) in _JSON_NATIVE_TYPES else _asdict_only(key)):
                 _to_serializable(item)
-            for key, item in cast(dict[Any, Any], value).items()
+            for key, item in value.items()
         }
     return _legacy_compat(value)
 

tests/durabletask/test_history_serialization.py

@@ -582,6 +582,34 @@ def __eq__(self, other: object) -> bool:
         return isinstance(other, _DeepCopyKey) and other.name == self.name
 
 
+class _ClassSpoofer:
+    """Reports a JSON-native ``__class__`` while being no such thing.
+
+    ``asdict`` dispatched on ``type(obj)``, which cannot be overridden, so
+    this reached the deep-copy fallback. Anything that dispatches on
+    ``__class__`` instead would mistake it for a ``str`` and hand back the
+    original object untouched.
+    """
+
+    def __init__(self, tag: str) -> None:
+        self.tag = tag
+
+    @property
+    def __class__(self) -> Any:  # type: ignore[override]
+        return str
+
+    def __eq__(self, other: object) -> bool:
+        # Compares on ``type`` rather than ``isinstance`` because the
+        # spoofed ``__class__`` makes ``isinstance`` checks ambiguous here.
+        return type(other) is _ClassSpoofer and other.tag == self.tag
+
+    def __hash__(self) -> int:
+        return hash(self.tag)
+
+    def __repr__(self) -> str:
+        return f'_ClassSpoofer({self.tag!r})'
+
+
 _EXOTIC_PAYLOADS = [
     pytest.param(_OddIsoDatetime(2024, 1, 2, 3, 4, 5), id='datetime-subclass'),
     pytest.param(
@@ -608,6 +636,7 @@ def __eq__(self, other: object) -> bool:
     pytest.param(bytearray(b'abc'), id='bytearray'),
     pytest.param(b'abc', id='bytes'),
     pytest.param(_MutableLeaf(1), id='custom-object'),
+    pytest.param(_ClassSpoofer('t'), id='class-spoofing-object'),
 ]
 
 
@@ -770,6 +799,26 @@ def test_mapping_key_is_not_converted_by_the_value_walker(self) -> None:
 
         assert list(exported) == [_TS]
 
+    def test_dispatch_uses_type_not_the_overridable_class_attribute(self) -> None:
+        """Guards a tempting micro-optimization that would change behavior.
+
+        ``value.__class__`` is measurably cheaper than ``type(value)`` and
+        satisfies a type checker without a cast, so it is an easy swap to
+        make. It is also wrong here: ``__class__`` can be overridden, and a
+        value claiming to be a ``str`` would take the JSON-native fast path
+        and be returned by reference. ``asdict`` dispatched on
+        ``type(obj)``, so it deep-copied this instead.
+        """
+        spoofer = _ClassSpoofer('t')
+        event = _state_event(spoofer)
+
+        exported = event.to_dict()['orchestration_state']['value']
+
+        assert exported is not spoofer, 'fast path taken on a spoofed __class__'
+        assert type(exported) is _ClassSpoofer
+        assert exported.tag == 't'
+        assert _legacy_to_dict(event)['orchestration_state']['value'] is not spoofer
+
 
 class TestTupleHandling:
     """Tuples keep their original ``asdict`` semantics.