Repository navigation
ToolConfirmation.from_response_dict crashes with TypeError on parsed dicts #7681
Description
🔴 Required Information
Describe the Bug:
ToolConfirmation.from_response_dict parses tool confirmations from a response dict and intends to support both raw dicts and the wrapper format {"response": ...}. However, it assumes response["response"] is always a serialized JSON string and unconditionally calls json.loads(response["response"]). If the payload is already deserialized (e.g., by FastAPI, an HTTP client, or an in-memory runner) into {"response": {"confirmed": True}}, calling json.loads(dict) raises TypeError: the JSON object must be str, bytes or bytearray, not dict.
In Human-In-The-Loop workflows (src/google/adk/workflow/utils/_workflow_hitl_utils.py), the resume handler wraps from_response_dict in try: ... except ValueError as e:. Because TypeError does not inherit from ValueError, resumes with pre-parsed dictionary payloads crash unexpectedly.
Steps to Reproduce:
- Call
ToolConfirmation.from_response_dict({"response": {"confirmed": True}}) - Observe
TypeError: the JSON object must be str, bytes or bytearray, not dict
Expected Behavior:
ToolConfirmation.from_response_dict should accept pre-parsed dictionary payloads inside response["response"] and validate them directly via model_validate(raw).
Observed Behavior:
TypeError: the JSON object must be str, bytes or bytearray, not dict
Environment Details:
Desktop OS: Windows / Linux / macOS
Python Version: 3.11+
ADK Library Version: main branch
Model Information:
Are you using LiteLLM: No
Which model is being used: N/A
🟡 Optional Information
Minimal Reproduction Code:
python
from google.adk.tools.tool_confirmation import ToolConfirmation
Crashes with TypeError:
ToolConfirmation.from_response_dict({"response": {"confirmed": True}})
How often has this issue occurred?: Always (100%)
Activity
surajksharma07 commentedon Oct 23, 2026
Reproduced this on main @aryanpatil-07 and the TypeError also leaks out of parse_tool_confirmation() in the workflow HITL resume path instead of becoming a WorkflowDataError.
The approach in #7682 looks right as a workaround: only call json.loads() when the inner value is a str/bytes, otherwise pass it straight to model_validate(). Invalid values like {"response": 123} then raise a ValidationError which the HITL code already catches.
Could you confirm this covers your real case too? Also which caller sent you the pre-parsed dict (FastAPI, a custom client, workflow resume)?
Before it goes for review please test it well and add a test for the HITL resume path and for a non-dict inner value so we cover more than the direct call.
aryanpatil-07 commentedon Oct 23, 2026
Hi @surajksharma07 , thanks for reproducing and reviewing!
Yes, this covers our real case completely.
The pre-parsed dict came from a FastAPI / REST API server setup. The incoming HTTP request body containing the resume payload was parsed by FastAPI into Python dictionaries before being passed to
runner.run_async(..., resume_inputs=...)/ workflow resume. Because the client payload used the wrapper shape{"response": {"confirmed": True, ...}},response["response"]was already adictrather than a JSON string, which triggered theTypeErrorinjson.loads().As requested, I have updated the PR with:
test_single_response_key_with_invalid_inner_value_raises_validation_errorintest_tool_confirmation.py, verifying that{"response": 123}raisesValidationError.test_workflow_hitl_utils.pyverifying that:{"response": {"confirmed": True}}) successfully setsctx.tool_confirmation.{"response": 123}) properly get converted toWorkflowDataErrorinstead of leakingTypeError.All checks are passing and it is ready for another review!
surajksharma07 commentedon Oct 26, 2026
Went through acd2d4e against current main @aryanpatil-07 and thanks @BichengWang for the review. The fix itself is right: str/bytes/bytearray still go through json.loads(), anything else goes to model_validate() so {"response": 123} or {"response": [..]} now becomes a ValidationError which parse_tool_confirmation() turns into WorkflowDataError. It merges cleanly on main and main/2.13.0 still have the old json.loads() line so this is still needed.
One gap since your real case is FastAPI: parse_tool_confirmation() drops None values before calling from_response_dict() but only at the top level. A flat {"confirmed": true, "hint": null} works but the wrapped {"response": {"confirmed": true, "hint": null}} keeps hint=None and hint is a plain str so the resume fails with WorkflowDataError. A client that serializes its model without exclude_none will send exactly that. Could you strip None from the inner dict as well (in the HITL parser is probably the cleaner place) and add a test with a null field inside the wrapper?
Two small things for lint: 7 added lines are over 80 chars (the with pytest.raises(WorkflowDataError, match=...) line, the two long test names and their docstrings) so please run pyink and shorten those docstrings. Also the from_response_dict docstring still says the wrapper is a json_string; worth adding that a parsed dict is accepted too.
After that it just needs a maintainer review and workflow approval.
aryanpatil-07 commentedon Oct 26, 2026
Hi @surajksharma07,
Thanks for pointing that out!
I've updated
parse_tool_confirmation()to stripNonevalues from the inner dictionary as well when wrapped under{"response": ...}, and addedtest_parse_tool_confirmation_wrapped_dict_with_none_fieldcovering that case.I also updated the
from_response_dictdocstring to mention parsed dicts, shortened the test names/docstrings to stay under 80 characters, and formatted everything withpyink.All changes have been amended into commit
ee2aa1e.surajksharma07 commentedon Oct 27, 2026
Checked ee2aa1e on current main @aryanpatil-07. It merges cleanly and {"response": {"confirmed": true, "hint": null}} now resumes fine while 123 and list inner values still come back as WorkflowDataError. The tool_confirmation and HITL tests (50) plus the confirmation tests under llm_flows (41) all pass.
The added lines are within 80 chars now apart from the parse_tool_confirmation import which is fine for an import and the docstring reads right.
Small note, not blocking: a wrapped JSON string with a null inside ('{"confirmed": true, "hint": null}') still fails since only the dict form gets cleaned. That was already the case on main so fine to leave for a follow-up.
From the triage side this looks good, it just needs the maintainer review and workflow approval now.