fix(mcp): chain JSON decode errors during tool invocation#3576
fix(mcp): chain JSON decode errors during tool invocation#3576mshsheikh wants to merge 2 commits into
Conversation
When tool input JSON parsing fails in `invoke_mcp_tool()`, the resulting `ModelBehaviorError` should always preserve the original `JSONDecodeError` as its cause. Today, the `_debug.DONT_LOG_TOOL_DATA` path raises `ModelBehaviorError` without exception chaining, which drops the original parse failure from the traceback and makes debugging harder. The OpenAI Agents SDK issue tracker also calls out this error path as a source of unsafe or unclear malformed-input handling. This change removes the early raise in the debug-suppressed branch and ensures the function always raises `ModelBehaviorError(...) from json_decode_error`. That keeps the user-facing message generic, avoids logging raw payload data in the sensitive path, and still preserves the root cause for debugging through `__cause__`. The behavior stays consistent with the non-debug branch while improving observability and error traceability. Testing: - Add a regression test for malformed JSON with `DONT_LOG_TOOL_DATA=True` and assert the original exception is chained - Add a regression test for malformed JSON with `DONT_LOG_TOOL_DATA=False` and assert the message still includes the input payload as expected - Verify the raised exception type remains `ModelBehaviorError` in both branches
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db5ec90e02
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if _debug.DONT_LOG_TOOL_DATA: | ||
| logger.debug(error_message) |
There was a problem hiding this comment.
When _debug.DONT_LOG_TOOL_DATA is true, this branch now falls through to raise ... from json_decode_error, but json.JSONDecodeError retains the original payload in its .doc attribute. For malformed MCP tool arguments containing secrets, any caller or error reporter that inspects chained exceptions can recover the redacted input; this also contradicts the existing redaction regression in tests/mcp/test_mcp_util.py that expects __cause__ and __context__ to be absent in this mode.
Useful? React with 👍 / 👎.
When MCP tool argument parsing fails in `invoke_mcp_tool()`, the error handling should preserve debugging information only when it is safe to do so. In the non-redacted path, the raised `ModelBehaviorError` should continue to chain the original `JSONDecodeError`, since that helps preserve the root cause for debugging and keeps the failure mode easy to trace. In the redacted path (`_debug.DONT_LOG_TOOL_DATA=True`), the original `JSONDecodeError` must not be chained, because `json.JSONDecodeError` retains the raw malformed input in its internal state. Chaining that exception would make it possible to recover sensitive tool input through exception inspection, which defeats the purpose of the redaction path. This change keeps the existing privacy protection in the sensitive path while still preserving useful error causality in the non-redacted path: - redacted path: raise `ModelBehaviorError(...) from None` - non-redacted path: raise `ModelBehaviorError(...) from json_decode_error` This keeps the behavior consistent, safer, and easier to debug without exposing raw malformed input when logging is disabled.
|
@codex review |
1 participant
When tool input JSON parsing fails in
invoke_mcp_tool(), the resultingModelBehaviorErrorshould always preserve the originalJSONDecodeErroras its cause. Today, the_debug.DONT_LOG_TOOL_DATApath raisesModelBehaviorErrorwithout exception chaining, which drops the original parse failure from the traceback and makes debugging harder. The OpenAI Agents SDK issue tracker also calls out this error path as a source of unsafe or unclear malformed-input handling.This change removes the early raise in the debug-suppressed branch and ensures the function always raises
ModelBehaviorError(...) from json_decode_error. That keeps the user-facing message generic, avoids logging raw payload data in the sensitive path, and still preserves the root cause for debugging through__cause__. The behavior stays consistent with the non-debug branch while improving observability and error traceability.Testing:
DONT_LOG_TOOL_DATA=Trueand assert the original exception is chainedDONT_LOG_TOOL_DATA=Falseand assert the message still includes the input payload as expectedModelBehaviorErrorin both branches