fix: redact model output from JSON validation errors - #4208
Conversation
validate_json embedded the offending JSON in its ModelBehaviorError message and chained the pydantic ValidationError, which repeats the same payload in its own input_value. Both surfaced regardless of DONT_LOG_MODEL_DATA, so structured output that failed schema validation leaked into the exception, its cause chain and any traceback the application logged. This is the model side of the tool argument redaction added in openai#4182, and it covers the three callers that pass model generated JSON: output schema validation, handoff input validation and its realtime equivalent. Follow the same shape as that fix: keep the detailed error when the flag allows it, and otherwise raise outside the except block so neither __cause__ nor __context__ keeps the ValidationError reachable. The span error was already payload free and is unchanged.
![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: eac6d33bf3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # raised outside the except block on purpose: chaining it would keep the ValidationError | ||
| # reachable through __cause__ and __context__, and that error embeds the offending model | ||
| # output in its own message. | ||
| raise ModelBehaviorError(f"Invalid JSON when parsing model output for {type_adapter}") |
There was a problem hiding this comment.
When model-data logging is disabled, this removes the message/cause leak but the redacted ModelBehaviorError traceback still includes the validate_json frame, whose locals contain json_str with the raw model output. Any traceback renderer or telemetry integration that captures locals can still expose the payload in the default redacted path; raise from a helper that never receives the payload or clear sensitive locals before raising.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
| if not _debug.DONT_LOG_MODEL_DATA: | ||
| raise ModelBehaviorError( | ||
| f"Invalid JSON when parsing {json_str} for {type_adapter}; {e}" | ||
| ) from e |
There was a problem hiding this comment.
When this helper is reached from handoff(), json_str is the handoff tool-call arguments, so an app that enables model diagnostics but keeps tool-data redaction on (DONT_LOG_MODEL_DATA=False, DONT_LOG_TOOL_DATA=True) still gets the raw handoff payload and chained Pydantic ValidationError. Use a caller-specific or mixed model/tool redaction policy here instead of keying every validate_json failure only off the model flag.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
| # raised outside the except block on purpose: chaining it would keep the ValidationError | ||
| # reachable through __cause__ and __context__, and that error embeds the offending model | ||
| # output in its own message. | ||
| raise ModelBehaviorError(f"Invalid JSON when parsing model output for {type_adapter}") |
There was a problem hiding this comment.
For a Runner.run() structured-output validation failure with model-data logging disabled, this redacted ModelBehaviorError is later populated with run_data, and that RunErrorDetails.raw_responses still contains the original model message text. Any handler or telemetry exporter that inspects the exception object can therefore recover the same payload even though the message and cause were redacted; mark these redacted validation errors so runner error details omit or sanitize raw model responses.
AGENTS.md reference: AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
|
Thanks for the contribution. The underlying issue is valid, but the current patch still leaves model-generated handoff arguments exposed when tool-data redaction remains enabled, and the payload remains reachable through SDK traceback locals. These are part of the same default redaction contract, including the synchronous and asynchronous Realtime paths. We are going to supersede this PR with a maintainer patch that applies the combined model/tool policy to handoffs and clears redacted traceback frames at the relevant Runner and Realtime boundaries. The broader RunErrorDetails.raw_responses sanitization is not needed because the failing current response is not appended before turn processing returns. |
Summary
agents.util._json.validate_jsonformats the offending model output into itsModelBehaviorErrormessage and chains the pydanticValidationError, which repeats the same payload in its owninput_value. Neither is gated on_debug.DONT_LOG_MODEL_DATA, which isTrueby default, so structured output that fails schema validation reaches the exception, its cause chain, and any traceback the application logs.This is the model side of the tool argument redaction in #4182, and one change covers the three callers that pass model generated JSON: output schema validation (any agent with an
output_type), handoff input validation, and its realtime equivalent.Before, with the default flag:
After:
The fix follows the same shape as #4182: keep the detailed error when the flag allows it, and otherwise raise outside the
exceptblock so neither__cause__nor__context__keeps theValidationErrorreachable. Suppressing only the message would not be enough, because the chained error carries the payload independently.Scope notes:
OPENAI_AGENTS_DONT_LOG_MODEL_DATA=0: same message, same chainedValidationError.SpanErrorraised alongside already useddata={}, so tracing was correct and is untouched.DONT_LOG_MODEL_DATAis the right flag for all three callers because the payload is LLM output in every case, including handoff arguments.Test plan
Four tests in
tests/test_error_logging_redaction.py, alongside the existing #4182 cases:test_output_schema_validation_error_redacts_payload_when_model_data_disabledtest_output_schema_validation_error_preserves_diagnostics_when_model_data_enabledtest_handoff_input_validation_error_redacts_payload_when_model_data_disabledtest_run_surfaces_redacted_output_validation_error, an end to end run throughRunner.runEach redaction test asserts the payload is absent from the message and that
__cause__and__context__are bothNone, so the check cannot pass by suppressing the message alone.Three fail on
mainand pass with the fix. The diagnostics test passes in both runs, which is the control that the flag still enables the detailed error:Verification from the repository root:
make formatmake lintmake mypymain, none in the touched filesmake pyrightmain(src/agents/sandbox/util/tar_utils.py:161)uv run pytest tests/test_error_logging_redaction.pymake testsThe full suite run was done on Windows, where some sandbox symlink and tracing timing tests fail independently of this change. I diffed the failing set against a clean
maincheckout in the same environment. This branch has one fewer failure than the baseline, and the difference istest_tracing_errors_streamed.py::test_max_turns_exceeded, a timing test that flakes onmainon its own. Nothing else differs in either direction.Issue number
Closes #4207
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRThe verification script is a bash script that shells out to
make. I ran the underlying steps individually instead, with the results above.