fix(client): serialize request bodies in JSON mode - #390
Merged
Conversation
Every request model with a `datetime` field was unusable: `model_dump()` leaves `start_time` / `end_time` as `datetime` objects, and `requests` then raises `TypeError: Object of type datetime is not JSON serializable` before the request leaves the process. This hit every time-filtered read — `get_requests`, `get_interactions`, `get_profiles`, `get_user_playbooks`, `get_agent_playbooks`, the four `search_*` methods, and both evaluation reads — on the kwargs path and the request-object path alike. 11 request models carry the field. Switch all 51 `json=<model>.model_dump()` call sites to `model_dump(mode="json")`. Datetimes become ISO strings; every other field type serializes identically, so the wire format is otherwise unchanged. `test_get_retrieved_learning_evaluation_results_posts_filters` asserted the buggy payload (raw `datetime` objects in the body) — that expectation is why the unserializable body went unnoticed, so it is corrected here rather than worked around. Adds `tests/client/test_request_serialization.py`: - every time-filtered read is called and its body passed through `json.dumps`, which is the assertion a mock-based call check cannot make because the mock never serializes; - an AST guard failing on any new `json=` call site that omits `mode="json"`, so the bug class cannot come back one method at a time. All 9 fail without the fix and pass with it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe client changes request-body serialization from Pydantic’s default ChangesRequest serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
Problem
Every request model carrying a
datetimefield was unusable over the client.model_dump()leavesstart_time/end_timeasdatetimeobjects, andrequeststhen raises before the request ever leaves the process:Reproducing against a live backend, on the plain kwargs path:
11 request models carry the field, so this covered every time-filtered read —
get_requests,get_interactions,get_profiles,get_user_playbooks,get_agent_playbooks, the foursearch_*methods, and both evaluation reads —on the kwargs path and the request-object path alike.
Fix
All 51
json=<model>.model_dump()call sites becomemodel_dump(mode="json"). Datetimes render as ISO strings; every other fieldtype serializes identically, so the wire format is otherwise unchanged.
params=call sites were checked and are plainstr/intdicts — no models,nothing to change there.
Test change worth a look
test_get_retrieved_learning_evaluation_results_posts_filtersasserted thebuggy payload — raw
datetimeobjects in the JSON body. That expectationis why the unserializable body went unnoticed, so it is corrected rather than
worked around. The body it described could never have been sent.
New tests
tests/client/test_request_serialization.py:json.dumps.This is the assertion a mock-based call check structurally cannot make, since
the mock never serializes — which is how the bug survived existing coverage.
json=call site omittingmode="json",so the class cannot return one method at a time.
All 9 fail without the fix and pass with it (verified by reverting the source
change and re-running).
Verification
tests/client/: 68 passedtest_extraction_eval.py::test_live_extraction_provider_returns_canned_items,test_llm_mock_schema_compliance.py::...[playbook_extraction-playbook_extraction])reproduce on a clean tree — pre-existing, unrelated to serialization
Summary by CodeRabbit
Bug Fixes
Tests