fix(tracing): record the tool output on approval-gated function spans - #4866
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1be570c04b
ℹ️ 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".
seratch
left a comment
There was a problem hiding this comment.
The wrapper-output fix is useful, but the rejection message still reaches the exported span through SpanError.message with sensitive-data tracing disabled. This leak also exists on the base; the issue is that the rejection-redaction part of this PR remains incomplete. Please gate the error message at the same approval boundary and assert against the complete exported span, or split this into the wrapper-output fix alone.
|
@seratch Updated - the error message is gated the same way now. Have a look. |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The rejection message now follows the same sensitive-data tracing gate as tool output, so the exported span no longer leaks app-supplied rejection text when sensitive tracing is disabled. This resolves the remaining blocker.
|
@sylvesterkaczmarek Could you please refrain from posting this type of comment on other people's PRs? |
Summary
When a function tool is gated by
needs_approval, the function span exports the internalFunctionToolResultwrapper intospan_data.outputinstead of the tool's output._run_single_toolreturns either a raw tool output or aFunctionToolResult, because the approval path short-circuits. Every other consumer discriminates the two withisinstance, including_build_function_tool_resultsin the same file; the span write does not.FunctionSpanData.export()then callsstr()on the wrapper, so the trace carries a repr of the wholeAgentobject in the field documented to hold the tool output, includingagent.instructions, the tool inventory with JSON schemas, the model settings and the raw arguments. On the repo's own test agent that is 2506 characters, under the defaulttrace_include_sensitive_data=True.The rejection message leaked by two separate routes: it was written to
span_data.outputfrom outside the sensitive-data gate, and it went ontoSpanError.message, whichSpan.export()sends through unchanged. Both reached the backend with the flag off.The fix reads the output off the wrapper the way the rest of the file already does, drops the ungated write, and routes the error message through
_error_tracing.get_trace_errorunder the same flag.Test plan
tests/test_run_step_execution.py, all failing onmain: theFunctionToolResult(...)repr whereNoneis expected,assert error["message"] == "Tool execution rejected"against a leakedsecret-denial-456, and the include-sensitive-data path keeping the message intact.span_data.outputalone, so a leak through any other field fails it too.ruff check,ruff format --checkandmypyon the changed module are clean.The behaviour change worth a second look: with
trace_include_sensitive_data=Falsethe exported rejection output goes from the message text toNone, and the span error message to a fixed string. That is what the flag asks for and it matches the error paths, but it is visible to anyone reading rejection reasons out of a redacted trace.redacted_messageis passed inline at the call site, where_tool_errors.pyhoists its equivalent to a module constant. Say the word and I will match that shape.Issue number
None.