fix: redact tool-argument values from invalid-input errors - #4012
fix: redact tool-argument values from invalid-input errors#4012ioleksiuk wants to merge 1 commit into
Conversation
![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: fe146f7283
ℹ️ 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".
fe146f7 to
6f8c1b0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f8c1b0f9b
ℹ️ 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".
6f8c1b0 to
81283f0
Compare
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the update. The latest revision fixes the demonstrated mapping-key and unknown top-level-key leaks, but this needs one scope reset before merge.
Please remove field/error summarization from redacted mode and emit only the fixed base error message. The repository's redaction contract requires a fixed message without inspecting sensitive exception metadata, and the successive loc special cases show that classifying Pydantic locations is not a stable security boundary. Preserve the current full validation detail and exception chaining when DONT_LOG_TOOL_DATA=False.
Please also apply the same redacted behavior to Agent.as_tool(parameters=...), which still interpolates and chains the complete ValidationError. Add caller-level adversarial tests for function tools, agent-as-tool, and Codex tools asserting that the sentinel is absent and both __cause__ and __context__ are None in redacted mode.
81283f0 to
c55087d
Compare
|
Once the codex cloud review says 👍, this can be merged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c55087d504
ℹ️ 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".
When DONT_LOG_TOOL_DATA is set, the ModelBehaviorError raised for an invalid tool argument still embedded the raw values: pydantic's ValidationError string carries input_value=..., and `raise ... from e` attached the payload-bearing exception as __cause__/__context__, so the data surfaced in tracebacks and telemetry even though the adjacent log calls are already redacted. Follow the existing _parse_function_tool_json_input hardening: under DONT_LOG_TOOL_DATA, raise a fixed base message from outside the except block, so the ValidationError is neither interpolated into the message nor attached as __context__. The redacted path also avoids copying the exception to an outer local, so it is not retained in the raising frame where telemetry that captures frame locals could recover it. Full validation detail and exception chaining are preserved when DONT_LOG_TOOL_DATA is disabled. This covers all three tool-argument parsing surfaces: function tools (tool.py), agent tools (Agent.as_tool), and the experimental Codex tool. Add caller-level regression tests for each surface asserting the raw value is absent and __cause__/__context__ are None in redacted mode (and that no ValidationError is retained in the traceback frame locals), and present with chaining when enabled.
c55087d to
f988471
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f988471e77
ℹ️ 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".
| validation_failed = True | ||
|
|
||
| if validation_failed: | ||
| raise ModelBehaviorError(base_message) |
There was a problem hiding this comment.
When DONT_LOG_TOOL_DATA is true and validation fails after JSON decoding, this redacted raise still happens in _on_invoke_tool_impl while input, ctx.tool_arguments, and json_data remain live in the traceback frame, so traceback-with-locals telemetry can recover the model-supplied arguments even though the message and exception chain are scrubbed. Fresh evidence beyond the prior exception-local comments is the decoded payload/raw-input locals that remain at the redacted raise; clear or avoid carrying those locals before raising the redacted ModelBehaviorError.
AGENTS.md reference: AGENTS.md:L89-L89
Useful? React with 👍 / 👎.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Let me know once codex reviews say 👍
2 participants
Summary
With
DONT_LOG_TOOL_DATAset, theModelBehaviorErrorraised for an invalid function-tool or Codex-tool argument still embedded the raw values. Pydantic'sValidationErrorstring carriesinput_value=..., andraise ... from eattached the payload-bearing exception as__cause__/__context__, so tool data surfaced in tracebacks and telemetry even though the adjacent log calls at these sites are already gated onDONT_LOG_TOOL_DATA.This extends the hardening already applied to the sibling
_parse_function_tool_json_inputto the type-validation path intool.pyand tocodex_tool.py: underDONT_LOG_TOOL_DATA, raise a payload-free summary (field location + error type, no values) from outside theexceptblock, so theValidationErroris neither interpolated into the message nor attached as__context__. Full detail and exception chaining are preserved when tool-data logging is enabled, and the redacted error still names the invalid field so the model can self-correct.Test plan
Ran the full local stack —
make format,make lint,make typecheck,make tests— all pass. Added regression tests intests/test_error_logging_redaction.pyandtests/extensions/experiemental/codex/test_codex_tool.pyasserting that the raw value is absent (and__cause__/__context__areNone) when redaction is on, present when off, and that the invalid field name still reaches the caller.Issue number
None (defense-in-depth; completes the redaction already applied to the sibling parser).
Checks
make format && make lint && make typecheck && make tests)/reviewbefore submitting this PR (N/A)