Conversation
EventSerializer already converted non-finite floats and JS-unsafe integers for values that reached _default_inner directly, and for Pydantic model_dump() output. tuple/set/frozenset, numpy scalars and arrays, dataclasses, enum values, and LangChain to_json() returned converted containers without routing them back through default(), so nested NaN/Infinity leaked as invalid JSON tokens. Route those branches through default() the same way Pydantic dumps already do. Co-authored-by: hassieb.pakzad <hassieb.pakzad@clickhouse.com>
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
No review was started: this request came from a bot account. Manual reviews can only be requested by someone with write access to this repository. Ask a maintainer to comment @claude review, or have your automation post the comment from a user account with write access.
Tip: disable this comment in your organization's Code Review settings.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 653c06c52a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _reject_json_constant(token): | ||
| # json.loads accepts bare NaN/Infinity by default; reject them the way | ||
| # the ingestion server's strict parser does. | ||
| raise ValueError(f"invalid JSON constant emitted: {token}") |
There was a problem hiding this comment.
When the strict loader encounters a bare JSON constant, this line constructs the exception message directly inside raise, contrary to the repository’s explicit Python convention. Assign the formatted text to a variable first and then pass that variable to ValueError.
AGENTS.md reference: AGENTS.md:L147-L149
Useful? React with 👍 / 👎.
| return f"<{type(obj).__name__}>" | ||
|
|
||
| if is_dataclass(obj): | ||
| return asdict(obj) # type: ignore | ||
| return self.default(asdict(obj)) # type: ignore | ||
|
|
||
| if isinstance(obj, BaseModel): | ||
| obj.model_rebuild() |
There was a problem hiding this comment.
🟡 (optional) Routing dataclass/tuple-set-frozenset/numpy/langchain conversions through self.default() (lines 79-150) now makes their entire nested content consume the shared _MAX_DEPTH=20 budget, whereas before these branches handed the converted dict/list straight to the native JSON encoder with no depth cap at all. Deeply nested LangChain to_json() graphs, nested dataclasses, or multi-level tuples/sets that used to serialize completely now get silently truncated to a bare ""/"" placeholder past ~20 levels. Fix: exempt pure container recursion (dict/list/tuple values that are themselves JSON-native) from the depth counter, or only count depth for genuinely custom-object conversions, so plain nested data is never truncated just because it now passes through self.default().
Extended reasoning...
Before: is_dataclass(obj): return asdict(obj) (and similarly tolist()/list(obj)/to_json()) returned a fully-converted plain dict/list; json's native encoder walks dict/list/tuple internally without ever calling our default(), so no depth limit applied to their contents, only Python's C recursion limit. After: return self.default(asdict(obj)) immediately re-enters _default_inner, hits isinstance(obj, dict) (line 152) which recursively calls self.default() on every value, incrementing self._depth each level; once self._depth>=20 (line 126) any further nested value is replaced by f"<{type(obj).name}>" regardless of whether it is a plain dict/list/int. A LangChain chain/agent whose to_json() output nests >20 levels (common for composed Runnable graphs) or a recursive dataclass tree now loses data mid-structure that fully serialized on the base branch.
Verification: nit. The mechanism is real and demonstrable. default() increments self._depth on every entry (lines 47-51) and line 126-127 truncates any container/object to a bare <type> placeholder once _depth >= _MAX_DEPTH (20). On the base branch, is_dataclass → return asdict(obj), tuple/set/frozenset → return list(obj), ndarray → tolist(), numpy generic → item(), enum → obj.value, and…
2 participants
What does this PR do?
EventSerializeralready turnsNaN/Infinity/-Infinityand JavaScript-unsafe integers into JSON-safe strings for values that hit_default_innerdirectly, and for Pydanticmodel_dump()output. Several other conversion branches returned a container or scalar without routing it back throughdefault(), so nested non-finite floats were emitted as bareNaN/Infinitytokens that strict JSON parsers reject.This routes the remaining branches through
default()the same way Pydantic dumps already do:tuple/set/frozensetndarray/genericasdict)enum.EnumvaluesSerializable.to_json()Fixes #1876
Type of change
Verification
Wrote failing unit tests first, then the production change.
All checks passed!(ruff)44 passed(vitest/pytest serializer + json unit tests)Checklist
code_review.md..env.templateif needed.The behavioral fix appears correct, but the explicit module-level import requirement must be satisfied before merging.
Summary
Reviews (1) · Last reviewed commit: "fix(serializer): sanitize NaN/Inf nested..."