Skip to content

Fix unhandled JSONDecodeError in AnthropicClient tool call parsing - #69

Closed
hobostay wants to merge 1 commit into
antoinezambelli:mainfrom
hobostay:fix/anthropic-json-error
Closed

Fix unhandled JSONDecodeError in AnthropicClient tool call parsing#69
hobostay wants to merge 1 commit into
antoinezambelli:mainfrom
hobostay:fix/anthropic-json-error

Conversation

@hobostay

Copy link
Copy Markdown
Contributor

Summary

  • json.loads(args) in _convert_messages was called without a try/except when parsing tool call arguments
  • Malformed JSON in arguments would raise an unhandled json.JSONDecodeError, crashing the inference loop with an unhelpful traceback
  • Wrap in try/except and fall back to empty dict on parse failure, consistent with LlamafileClient's handling

Impact

  • Without this fix, malformed tool call arguments from upstream systems would crash the entire inference loop instead of producing a recoverable error

Test plan

  • Verify malformed JSON in tool call arguments doesn't crash the client
  • Verify valid JSON arguments still parse correctly
  • Run existing test suite

🤖 Generated with Claude Code

`json.loads(args)` in `_convert_messages` was called without a
try/except. If the upstream system passes malformed JSON in tool call
arguments, this raises an unhandled `json.JSONDecodeError` that
propagates as an uncaught exception, crashing the inference loop with
an unhelpful traceback instead of a recoverable error.

Wrap in try/except and fall back to empty dict on parse failure,
consistent with how LlamafileClient handles the same case.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@antoinezambelli

Copy link
Copy Markdown
Owner

Thanks for digging into this — the asymmetry you spotted is real. One nuance steered the decision: this json.loads is on the request-construction path (history → backend), where the args were serialized by us, so malformed JSON is an invariant violation we'd rather surface than swallow. Silently coercing to {} would also fire the tool with empty args, which is worse than not firing. (The direct analog, llamafile.py:110, is also unguarded; where llamafile does guard — :521, parsing untrusted model output — it falls back to returning the content as text, not {}.) So rather than soften it, we'd like to keep it loud but friendlier: wrap the decode to raise a clear error with context instead of a bare traceback. Queuing that. Appreciate the report!

antoinezambelli added a commit that referenced this pull request May 26, 2026
…lient shutdown, loud arg decode (#86)

* test: cover proxy 400 paths for bad Content-Length and non-object body

Backfills tests for the input validation merged in #71, which landed
without coverage for its new 400 responses.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: make Ollama model unload non-blocking in async stop()

subprocess.run() blocked the event loop for the duration of
"ollama stop". Switch to asyncio.create_subprocess_exec + await so
concurrent coroutines stay responsive. Reimplements #66; lets the
command's stderr surface rather than suppressing it.

Co-authored-by: hobostay <110803307+hobostay@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat: close client connection pools on proxy shutdown; clear error on bad tool-call args

- Add aclose() to the LLMClient protocol and all clients; ProxyServer
  closes the active client's httpx pool in _async_stop, eliminating the
  unclosed-pool ResourceWarning. Completes #67, which added the method
  but wired no caller.
- AnthropicClient._convert_messages now raises a clear ValueError naming
  the tool and offending payload on malformed tool-call argument JSON,
  instead of an opaque JSONDecodeError. Addresses #69 (kept loud rather
  than swallowing to an empty dict).

Co-authored-by: hobostay <110803307+hobostay@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs: note the Guardrails facade does not enforce tool prerequisites

The facade covers validation, retry nudges, and step enforcement;
prerequisites are a granular-API feature (StepEnforcer.check_prerequisites).
Makes the existing "caller wires" intent explicit in the Mode 3 section.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: hobostay <110803307+hobostay@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants