Skip to content

_coerce_to_dict: validate json.loads result so the JSON string "null" can't crash route handlers - #13

Open
danshapiro wants to merge 1 commit into
zapier:mainfrom
danshapiro:fix/coerce-to-dict-json-null
Open

_coerce_to_dict: validate json.loads result so the JSON string "null" can't crash route handlers#13
danshapiro wants to merge 1 commit into
zapier:mainfrom
danshapiro:fix/coerce-to-dict-json-null

Conversation

@danshapiro

Copy link
Copy Markdown

Fixes #12. This fixes the TypeError: 'NoneType' object is not a mapping crash described
in that issue (minimal repro there).

Change (one function, automationbench/tools/api/fetch.py):

_coerce_to_dict now validates the json.loads result:

  • JSON null{} (matches the existing None/empty-string handling at the top of the
    function);
  • other non-object JSON → json.JSONDecodeError, which the existing except-path in
    api_fetch (fetch.py:317) converts to the structured
    {"error": {"code": 400, ...}} response. We deliberately reuse JSONDecodeError so
    that handler stays unchanged; a distinct error type would require widening the except
    clause.

Behavior changes (all three):

  1. body="null" / params="null": raw TypeError crash on unpacking handlers → normal
    successful routing, identical to passing None. On handlers that ignore the argument
    (e.g. Gmail messages.list body), behavior is unchanged.
  2. Non-object JSON (e.g. body="[1, 2]") on unpacking handlers: raw TypeError crash →
    structured 400 beginning Invalid JSON: Expected a JSON object, got list (the
    JSONDecodeError position suffix follows).
  3. Non-object JSON on handlers that ignore the argument: previously silently succeeded;
    now returns the same structured 400. This makes payload validation uniform across
    services — the same validation the tool already applies to syntactically bad JSON —
    rather than success depending on which handler happens to read the argument.

Score-comparability note: changes 1–2 only affect calls that previously crashed; change 3
tightens validation on a shape that was previously inert. Worth a line in release notes.

Tests (conventions matched to tests/test_api_fetch.py):

  • TestCoerceToDict.test_json_null_string_returns_empty
  • TestCoerceToDict.test_json_non_object_raises
  • TestApiFetch.test_drive_files_list_with_json_null_body — end-to-end regression for
    the reported crash (passes on an empty WorldState()).

All 39 tests in tests/test_api_fetch.py pass with the patch (36 existing unmodified +
3 new). Full-suite run on a repo checkout still recommended before merge.

Scope note: this PR deliberately does not touch two adjacent behaviors we observed —
query strings inlined in the URL 404-ing (already addressed by the open #2), and the
tool-execution layer's default exception formatting that surfaces raw exception text (the
verifiers ToolEnv default, not this repo). If #2 lands first, the only overlap is
test-file context; the _coerce_to_dict change is orthogonal to its diff.


Test run (fork checkout of main @ 4a8e106 with this patch applied; Python 3.13, env via uv sync --dev):

  • uv run pytest tests/test_api_fetch.py -q39 passed (0.06s)
  • uv run pytest -q (full suite) → 1401 passed (24.95s), no failures
…sh route handlers

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-authored-by: Amplifier <amplifier@microsoft.com>
@GithubX-F

Copy link
Copy Markdown

We ran into the same problem, and I think your solution makes a lot of sense.

@shaozhou-azu

Copy link
Copy Markdown

Thanks for the clear _coerce_to_dict fix and regression coverage. I preserved that commit with your authorship in #16, which supersedes this PR with one additional routing fix.

During independent review, the QuickBooks /query text-body fallback exposed a shared try/except bug: if params parsing fails while body is non-empty, the fallback continues without assigning parsed_params, and dispatch raises UnboundLocalError. Malformed params already trigger this on main; this PR broadens the trigger to non-object params because _coerce_to_dict now correctly rejects them.

#16 splits params/body parsing so invalid params always return a structured 400, while the QuickBooks text/plain exception remains body-only. It includes the base/#13/new behavior matrix, six additional regression cases, and passing full-suite/static-check results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants