Sanitize mixin enums before the primitives check - #267
Open
cboulay wants to merge 3 commits into
Open
Conversation
`_sanitize` checked `isinstance(value, (bool, int, float, str))` before it checked `isinstance(value, enum.Enum)`. An IntEnum, StrEnum or IntFlag member is an instance of its mixed-in builtin, so it matched the first branch and was returned verbatim rather than reduced to its value. Those members then travel to the graph server and out over the settings event, settings snapshot and component metadata wires as pickled enum members -- `SettingsSnapshotValue.structured_value` / `.repr_value` and `SettingsFieldMetadata.default` / `.choices`. A client that does not have the defining package installed cannot unpickle them, which is the case sanitizing exists to prevent. It fails hard: `_subscribe_pickled_stream` has no per-payload recovery, and a reconnecting subscriber replays the retained event history, so one such event ends settings observation for the life of the graph server. Reorder the two checks. Nothing rendered changes -- json.dumps already emitted an IntEnum as its integer -- but the payload no longer references the package that defined the enum. Fixes #260
The test module used enum.StrEnum, which is 3.11+, so collection failed on 3.10 and took the whole suite with it. Spell the same thing the pre-3.11 way: what matters is the str mixin, and members are str instances either way. That exposed a second version dependence, in _sanitize itself. Mapping keys were passed through str() without being sanitized, and str() of an IntEnum member changed in 3.11 -- 'Rate.SLOW' before, '1' after. A settings field holding an enum-keyed mapping therefore produced different keys depending on which interpreter the graph ran under. Sanitize keys like any other value so they render as the enum's value on every supported version.
cboulay
force-pushed
the
cboulay/sanitize-enum-leak
branch
from
September 3, 2026 13:04
7aeebcd to
d056f48
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #265, which is stacked on #258. Base here is
cboulay/coordinate-axis-fingerprint; this PR's own diff is the two commits below.Fixes #260.
Mixin enums leaked onto the wire
_sanitizecheckedisinstance(value, (bool, int, float, str))beforeisinstance(value, enum.Enum). AnIntEnum,StrEnumorIntFlagmember is an instance of its mixed-in builtin, so it matched the first branch and was returned verbatim rather than reduced to its value.Those members then travelled to the graph server and out over the settings event, settings snapshot and component metadata wires as pickled enum members —
SettingsSnapshotValue.structured_value/.repr_valueandSettingsFieldMetadata.default/.choices. A client without the defining package installed cannot unpickle them, which is exactly what sanitizing exists to prevent.It fails hard rather than degrading:
_subscribe_pickled_streamhas no per-payload recovery, and a reconnecting subscriber replays the retained event history — so one such event ends settings observation for the life of the graph server.Reordering the two checks fixes it. Nothing rendered changes (
json.dumpsalready emitted anIntEnumas its integer); the payload simply no longer references the package that defined the enum.Enum-keyed mappings rendered differently per interpreter
Found while writing the tests. Mapping keys went through
str()without being sanitized, andstr()of anIntEnummember changed in 3.11 —'Rate.SLOW'before,'1'after. A settings field holding an enum-keyed mapping therefore produced different keys depending on which interpreter the graph ran under. Keys are now sanitized like any other value, so they render as the enum's value on every supported version.The second commit also spells the test enums the pre-3.11 way: the original used
enum.StrEnum, which is 3.11+, so collection failed on 3.10 and took the whole suite with it. What matters for the test is thestrmixin, and members arestrinstances either way.Why stacked rather than standalone
There is no file overlap with #258 or #265 and no dependency between them — it rebases cleanly either way. It is stacked because a downstream project pins
ezmsgto this branch, and needs the axis-identity fixes from #265 alongside this one; the stack is also what a draft release will be cut from.Testing
7 new tests in
tests/test_settingsmeta.py, coveringIntEnum/StrEnum/IntFlagmembers, nested containers, dataclass fields and enum-keyed mappings.Full suite on the rebased stack: 468 passed, with one pre-existing failure (
test_perf_analysisneedsxarray, fails identically at the base commit).🤖 Generated with Claude Code