fix(output): make the json default hook single-pass - #761
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughChangesThis change updates JSON serialization and terminology
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes JSON default handling terminate in a single pass and adds focused coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`json.dumps` feeds a `default` hook's return value back through the encoder and calls the hook again when that value still is not serializable, so the hook only terminates if it returns something json can already encode. Two branches broke that rule. `isoformat()` was returned unchecked. Any stub or mock answers `hasattr(obj, "isoformat")` and returns a fresh object of its own kind, so each pass allocated another object and re-entered forever. The `Enum` branch returned `obj.value` unchecked, so an opaque member value came straight back in, and an `Enum` wrapping an `Enum` chained a hop per level. Both are coerced now, so the hook runs exactly once per value and there is no recursion depth left to bound. The new test asserts that directly by counting hook invocations. This hung `tests/comfy_cli/command/test_run.py::TestLocalExecuteItemMapAndGroupedOutputs` on Python 3.12 and newer, where the encoder no longer trips a recursion guard early. On 3.10 and 3.11 that guard raised RecursionError after ~978 nested calls and the bare `except Exception` here swallowed it, so the encode limped to a string and CI, pinned to 3.10, stayed green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The single-pass guarantee was enforced twice, once inside the `Enum` branch and once inside the `isoformat` branch, so each new branch would have to carry its own guard. Coercing once at the end states the rule in one place and holds it for any branch added above. The explicit `Path` branch and its local import go with it: the tail already turns a `Path` into its string, which is what that branch returned. Behaviour is unchanged except that an `Enum` whose value is a datetime now serializes as its ISO string rather than `str(datetime)`.
ERA001 landed on main with two hits still in the tree, so `ruff check` is red on main and on every branch cut from it. `# status (billing / plan / concurrency)` and `# set-widget (name-addressed)` both parse as call expressions, which is what the rule looks for. Reworded the same way #735 handled `# browse: types / categories`, so the dividers keep saying what they said without looking like code. (cherry picked from commit 31be2b0)
62ebc50 to
3c423d8
Compare
dosubot
Bot
added
size:S
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @annehe9.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 1 |
Panel: 8/8 reviewers contributed findings.
| except Exception: # noqa: BLE001 | ||
| pass | ||
| return str(obj) | ||
| return obj if isinstance(obj, str | int | float | None) else str(obj) |
There was a problem hiding this comment.
🟡 Medium — The tail whitelist admits only str/int/float/None, so JSON-native containers are now stringified: an Enum whose value is [1, 2] or {'a': 1} used to be handed back to json and encoded as a real array/object, and now emits the Python repr "[1, 2]" / "{'a': 1}", silently changing machine-mode output shape (the hook is shared by selector.py::_dumps, so --select output is affected too). The docstring's promise that the tail "holds for any branch added above it" makes this a trap for the next natural branch, e.g. a dataclass/to_dict -> dict coercion, which would be flattened to a repr string. Letting list/dict/tuple through the check (json encodes tuples as arrays) preserves the previous behavior. Raised by 6 of 8 reviewers (gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial, gemini-3.1-pro edge-case, gemini-3.1-pro adversarial, kimi-k3-max edge-case).
| """ | ||
| if isinstance(obj, Enum): | ||
| return obj.value | ||
| obj = obj.value |
There was a problem hiding this comment.
🟢 Low — Enum unwrapping is now single-level: previously return obj.value handed an inner Enum member back to json, which re-entered the hook and unwrapped down to the innermost scalar, whereas now the inner member falls through to str(obj) and serializes as "Inner.MEMBER". A while isinstance(obj, Enum): obj = obj.value loop restores the old output while keeping the single-pass guarantee this redesign is after. Raised by 5 of 8 reviewers (claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case, kimi-k3-max adversarial, gemini-3.1-pro adversarial, gpt-5.6-sol-max edge-case).
The single-pass rewrite narrowed the tail coercion to str/int/float/None, which stringified values json already encodes. An Enum whose value is a list or a dict came back as its Python repr instead of a real array or object, changing machine-mode output shape. `selector.py::_dumps` shares the hook, so `--select` output changed too. The tail now passes list, dict and tuple straight through; their members are separate values, so each still costs at most one further hook call. The same rewrite also made enum unwrapping single-level, so an Enum wrapping an Enum serialized as "Inner.MEMBER" rather than the innermost scalar. Unwrapping now loops, bounded by an id-seen set so a member whose `_value_` was mutated into a cycle cannot hang the encode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019deuWey4LoCRaxADqop4nB
2 participants
What
json.dumpsre-encodes whatever adefaulthook returns and calls the hook again when the result is still not serializable, so adefaultthat returns a non-JSON value never terminates._json_defaultreturnedobj.isoformat()unchecked. Any stub or mock answershasattr(obj, "isoformat")and returns another object of its own kind, so each pass allocated a new object and re-entered.The rule the hook has to hold is that every return is a value json can already encode. Two branches broke it:
isoformat()was returned unchecked, so a stub that returns a fresh object of its own kind produced a new object per pass and never terminated.Enumbranch returnedobj.valueunchecked, so an opaque member value came straight back into the hook, and anEnumwrapping anEnumchained.Both are now coerced, so the hook runs exactly once per value. There is no recursion depth left to bound, so no
RecursionErrorto raise, catch, or swallow. A test asserts the single-pass property directly by counting hook invocations rather than by checking that some depth limit stops it.How it showed up
tests/comfy_cli/command/test_run.py::TestLocalExecuteItemMapAndGroupedOutputs::test_local_wait_envelope_groups_by_node_and_itemnever finished on a local checkout. It reaches_emit_queued, which putsexecution.validation_warningsandexecution.workflow_manifest()into the envelope; that test's mock leaves both as auto-created mocks.CI never caught it because both test workflows pin Python 3.10, where the C encoder's recursion guard raises
RecursionErrorafter ~978 nesteddefaultcalls — andRecursionErroris anException, so the bareexcept Exception: passinside_json_defaultswallowed it and the encode limped to a string. Measured: 3.10.20 finishes after 978 calls, 3.11.15 after 979, while 3.12, 3.13 and 3.14 had to be killed by a watchdog.pyproject.tomldeclaresrequires-python = ">=3.10", so anyone on a newer interpreter hits it.Verification
main, that test hangs indefinitely (killed at 40s). A leaked probe process reached 43% of system RAM, so an unattended run can push a machine into swap.tests/comfy_cli/output/test_renderer.py56 passed.test_logs,test_jobs,test_host_port,test_onboarding,test_pretty_print_sanitize) that also fail onmain. Before this change the suite could not complete locally at all.ValueError: Circular reference detected, and the single-pass counter sees 4 hook calls for 3 values.ruff check .clean.Follow-ups (not in this PR)
requires-pythonclaims support past 3.10 and nothing verifies it.pytest-timeoutin the dev extra. It is not installed today, so a hang in CI would burn the whole job timeout instead of failing fast.🤖 Generated with Claude Code