fix: handle unprotected JSON parsing in mime detection and Steel Browser API#671
fix: handle unprotected JSON parsing in mime detection and Steel Browser API#671lawrence3699 wants to merge 1 commit intodynamiq-ai:mainfrom
Conversation
…ser API Wrap json.loads() in guess_mime_type_from_bytes() and response.json() in Steel Browser session methods with try-except to prevent unhandled exceptions from crashing callers when responses contain invalid JSON or non-UTF-8 data.
There was a problem hiding this comment.
Pull request overview
This PR hardens tool utilities against unhandled exceptions caused by optimistic JSON parsing, ensuring MIME detection and Steel Browser file operations fail gracefully instead of crashing.
Changes:
- Wrap JSON detection in
guess_mime_type_from_bytes()withtry/exceptso invalid JSON payloads fall through to other MIME heuristics. - Guard
response.json()in Steel Browser upload/list helpers to handle 2xx non-JSON responses, raising a recoverable tool error for upload and returning an empty list for file listing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dynamiq/nodes/tools/utils.py |
Makes JSON MIME detection best-effort by catching decode/parse failures and continuing heuristics. |
dynamiq/nodes/tools/stagehand.py |
Adds defensive JSON parsing for Steel Browser API responses to avoid unexpected crashes on non-JSON bodies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif data.startswith(b"{") or data.startswith(b"["): | ||
| json.loads(data.decode("utf-8")) | ||
| return "application/json" | ||
| try: | ||
| json.loads(data.decode("utf-8")) | ||
| return "application/json" | ||
| except (json.JSONDecodeError, UnicodeDecodeError): | ||
| pass |
There was a problem hiding this comment.
Add unit coverage for the new JSON-parse failure path. guess_mime_type_from_bytes now swallows JSONDecodeError/UnicodeDecodeError and falls through to the other heuristics; there are already unit tests for dynamiq.nodes.tools.utils (currently only for sanitize_filename), but nothing asserts MIME detection behavior for invalid/valid JSON payloads.
| raise ToolExecutionException( | ||
| f"Steel Browser API returned invalid JSON after file upload: {e}", | ||
| recoverable=True, | ||
| ) |
There was a problem hiding this comment.
Consider chaining the caught JSON parsing error when re-raising ToolExecutionException (e.g., ... from e). Without exception chaining, the original traceback/context for why response.json() failed is lost, which makes debugging proxy/HTML/empty-body cases harder.
| ) | |
| ) from e |
Summary
Was reading through the tool utilities and noticed a few places where JSON parsing can throw unhandled exceptions:
guess_mime_type_from_bytes()innodes/tools/utils.py—json.loads()is called on data that starts with{or[but may not be valid JSON (e.g., a binary file or truncated payload). An unhandledJSONDecodeErrororUnicodeDecodeErrorcrashes the MIME detection instead of falling through to the next heuristic. Wrapped it in try-except so the function continues to the CSV/text/octet-stream fallbacks, matching the best-effort behavior of the rest of the function._upload_file_bytes_to_steel_browser_session()and_list_steel_browser_session_files()innodes/tools/stagehand.py—response.json()is called afterraise_for_status(), but a 2xx response with a non-JSON body (e.g., a proxy returning an HTML error page, or an empty body) still throwsValueError. The rest of the codebase already handles this — for example,memory/backends/dynamiq.pywrapsresponse.json()in try-except. Applied the same pattern here: for upload, raise a recoverableToolExecutionException; for listing, log and return an empty list.Test plan
guess_mime_type_from_bytes(b"{not json")returns a fallback MIME type instead of crashing