Repository navigation
fix(tools): accept pre-parsed dict in ToolConfirmation response - #7682
aryanpatil-07 wants to merge 3 commits into
Conversation
ToolConfirmation.from_response_dict expected response['response'] to always be a JSON-encoded string. When the payload is already deserialized into a dict (e.g. by an HTTP framework or test runner), calling json.loads raised TypeError. Check whether the wrapped payload is a string or bytes before attempting to decode it with json.loads, allowing pre-parsed dictionaries to be validated directly.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
BichengWang
left a comment
There was a problem hiding this comment.
Reviewed 3f190e9acde7788548eceefab1a03d0dc2bf59bc against the requested coverage in #7681. No correctness blocker found: pre-parsed dictionaries reach validation, existing JSON/bytes handling is preserved, and invalid inner values become WorkflowDataError in the HITL parser.
Offline validation on macOS arm64 / Python 3.13.16: 118 passed across test_tool_confirmation.py, test_workflow_hitl_utils.py, and test_workflow.py. Restoring only the original decoder from base facd7ddafe0e23715e079e50971ad27b251e2e98 in-process makes four added cases fail with TypeError; the bytes control still passes. Sixteen additional probes cover approval/rejection with dict/string/bytes/bytearray payloads, nested payload preservation, unchanged inputs, and invalid values through the resume/parser helpers.
Small nonblocking cleanup: isort --check-only flags the imports in test_workflow_hitl_utils.py; please sort them. Production-file Ruff and diff whitespace checks pass. Pyink could not be installed from the offline cache; I did not run the full tox matrix or a live model/HTTP workflow.
|
Hi @BichengWang, Thanks for the thorough review and testing! I've sorted the imports in |
acd2d4e to
ee2aa1e
Compare
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
ToolConfirmation.from_response_dictassumedresponse["response"]was always a JSON-encoded string and passed it directly tojson.loads(). When receiving an already-deserialized dictionary (e.g., from FastAPI or test runners),json.loads()raisesTypeError. In_workflow_hitl_utils.py, onlyValueErroris caught, causing HITL workflow resumption to crash.Solution:
Inspect
response["response"]first: if it is astr,bytes, orbytearray, deserialize it withjson.loads(); otherwise, pass the pre-parsed dictionary directly tomodel_validate().Testing Plan
Unit Tests:
Summary of test results:
Manual End-to-End (E2E) Tests:
Verified that ToolConfirmation.from_response_dict({"response": {"confirmed": True}}) returns a valid ToolConfirmation(confirmed=True) instance without raising a TypeError.
Checklist