fix(openai): allowlist metadata pass-through, drop eager response serialization#606
Merged
Abhijeet Prasad (AbhiPrasad) merged 3 commits intoJul 21, 2026
Merged
Conversation
…ialization Aligns the OpenAI integration with `.agents/skills/sdk-integrations/SKILL.md`, following the same pattern as #604 (mistral), #594 (litellm), #603 (livekit), and #583 (anthropic). - **Allowlist instead of denylist for metadata.** Every `_parse_params` copy (`ChatCompletionWrapper`, `ResponseWrapper`, `BaseWrapper` for Embedding/ Moderation/Speech, `_AudioFileWrapper`, `_ImageBaseWrapper`) previously did `metadata: {**params, "provider": "openai"}` — every non-input request kwarg landed in span metadata. `ResponseWrapper._parse_event_from_result` did the same on the response side. That leaked `extra_headers`, `extra_query`, `extra_body`, user-provided Responses `metadata`, `user`, `safety_identifier`, `prompt_cache_key`, and anything OpenAI adds in future SDK versions straight into telemetry. Replaced with per-surface allowlist tuples plus a shared `_filter_metadata` helper that drops `NOT_GIVEN`/`Omit` sentinels and serializes `response_format` inline. Mirrors Anthropic's `METADATA_PARAMS`. - **Drop eager response serialization.** `_try_to_dict(raw_response)` was full-dumping every non-streaming Chat/Response API response just to read `usage`/`choices`/`output` — `bt_safe_deep_copy` already dumps at log time. Now reads via `getattr`; a narrow `_choices_output` helper dumps only `choices` (needed for the audio-attachment walker). `_parse_event_from_result` and `_extract_moderation_metadata` were made dict-or-object aware. - **Note on `moderation`.** Added to both chat + responses request allowlists and the responses result allowlist because it's a Braintrust-proxy request parameter that deep-merges with the response's moderation object at log time. Three tests that asserted `extra_headers` leaked into metadata (`extra_headers ["X-Stainless-Helper-Method"] == "chat.completions.stream"`) flipped to assert the leak is now blocked. Test plan: - No new mocks/fakes; no cassette re-records. - `nox -s "test_openai(latest)"` — 76/76 pass - `nox -s "test_openai(1.92.0)"`, `test_openai(1.77.0)`, `test_openai(1.71.0)` — all green - `nox -s "test_openai_http2_streaming(latest)"` — 3/3 - `nox -s "test_btx_openai(latest)"` — 5/5 - `nox -s "test_openai_agents(latest)"` — 7/7 - `ruff check` + `ruff format --check` clean Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- `_get_attr` only earned its keep in `_extract_moderation_metadata` (which legitimately sees both dicts from stream postprocess and pydantic objects from non-streaming). Its other callers — `_parse_event_from_result` and `_choices_output` — only ever see pydantic responses from the OpenAI SDK, so plain `getattr(x, k, None)` suffices. - `_filter_metadata` was checking `value is None or _is_not_given(value)` — the `None` half was redundant defensive belt: `_is_not_given` already handles the sentinels, and the old denylist behavior kept None-valued params too. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`text_format` on Responses API (`.parse(text_format=SomeModel)`) accepts a Pydantic BaseModel class exactly like `response_format` on Chat Completions. It was allowlisted but not passed through `_serialize_response_format`, so the class reference landed in span metadata as `<class 'x.SomeModel'>` instead of the JSON schema. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Abhijeet Prasad (AbhiPrasad)
approved these changes
Jul 21, 2026
This was referenced Jul 21, 2026
Abhijeet Prasad (AbhiPrasad)
pushed a commit
that referenced
this pull request
Jul 21, 2026
…reads (#612) ## Summary Aligns the Strands integration with `.agents/skills/sdk-integrations/SKILL.md`, following the pattern of #606 (openai), #604 (mistral), #603 (livekit), #594 (litellm), #583 (anthropic). Audit turned up several issues: - **Missing span_origin instrumentation name.** Every other integration has `_INSTRUMENTATION = "<provider>-auto"` + a module-level `start_span` wrapper; strands was the only one whose spans lacked `context.span_origin.instrumentation.name`. Added the standard wrapper and pass `internal={"instrumentation": _INSTRUMENTATION}` explicitly through the nested `parent.start_span(...)` path, which doesn't route through the module wrapper. - **Denylist metrics extraction (real leak vector).** `_end_model_invoke_span_wrapper` was doing `{k: v for k, v in metrics.items() if _is_supported_metric_value(v)}` — every numeric key strands emits (present + future) flowed into the span's top-level `metrics` bag. Replaced with an explicit allowlist (`latencyMs`, `timeToFirstByteMs`). Since neither key is spec-listed, they land under `metadata.strands_metrics` rather than top-level `metrics`. - **Silently-broken agent metadata extraction.** `_agent_metrics_and_metadata` did `metrics = getattr(result, "metrics"); if not isinstance(metrics, dict): return {}, {}` — but `AgentResult.metrics` is an `EventLoopMetrics` **dataclass**, so the guard discarded everything. Renamed to `_agent_metadata_from_result` and read `cycle_count` / `accumulated_usage` / `accumulated_metrics` off the dataclass through a single `_pick_numeric(source, keys)` allowlist helper. Moved `cycles` from `metrics` to `metadata` (not a spec metric key). - **Positional-arg reads.** `_start_agent_span_wrapper` / `_start_model_invoke_span_wrapper` were `kwargs.get("tools")` / `kwargs.get("system_prompt")` etc. Strands' signatures put those at fixed positional slots — switched to `_arg(args, kwargs, N, name)` so positional callers work. - **Simplifications (via `/simplify`):** collapsed twin allowlist helpers into `_pick_numeric`; collapsed the parent-vs-module `if/else` in `_start_span_for_otel`; replaced the `_bt_parent_otel_span` metadata back-channel with a direct `parent_otel_span=` kwarg; dropped the now-orphan `metrics=` parameter from `_end_span_for_otel`; trimmed the verbose `wrap_strands_tracer` and `_InvalidOtelSpanKey` docstrings. ## Known SKILL gaps NOT fixed here Each deserves its own PR/discussion — flagging so they don't get lost: 1. **`metadata.provider` on LLM spans.** Strands' `Tracer` only receives `model_id`, not the model class — provider inference would require brittle id-pattern matching. 2. **Tokens under `metadata.strands_usage` rather than `metrics.tokens` / `prompt_tokens` / `completion_tokens`.** Fixing correctly means deciding whether model-invoke should stay typed `llm` at all — Strands' `OpenAIModel` / `AnthropicModel` delegate to the underlying provider SDK, which per the SKILL's "framework llm-like spans that delegate" rule should NOT be typed `llm`. 3. **No `test_auto_strands.py` subprocess auto-instrument test.** Strands is wired into `auto.py` but lacks the subprocess parity test other integrations have. ## Test plan - [x] **No new mocks/fakes.** Existing cassette-backed VCR test (`test_strands_openai_agent_traces_native_otel_lifecycle`) covers the behavior; added a `span_origin.instrumentation.name == "strands-auto"` assertion on every emitted span. - [x] **No cassette re-recording.** HTTP behavior is unchanged; only in-process span shaping was touched. - [x] `cd py && nox -s "test_strands(latest)"` — 2/2 pass - [x] `cd py && nox -s "test_strands(1.20.0)"` — 2/2 pass - [x] `ruff check` + `ruff format --check` on touched files — clean - [x] `nox -s pylint` — clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- sfk:created-approved-by --> Created by abhijeet <!-- sfk:slack-thread --> [Slack thread](https://starfolkai.slack.com/archives/C0AQDETAVT3/p1784659948425149?thread_ts=1784659948.425149&cid=C0AQDETAVT3) Co-authored-by: Starfolk <noreply@starfolk.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Abhijeet Prasad (AbhiPrasad)
pushed a commit
that referenced
this pull request
Jul 21, 2026
…eshape (#610) ## Summary Aligns the pydantic_ai integration with `.agents/skills/sdk-integrations/SKILL.md`, following the same pattern as #606 (openai), #608 (openrouter), and #604/#603/#594 (anthropic/livekit/litellm/mistral). Audit turned up two issues: - **Denylist metadata pass-through (real leak vector).** `_build_agent_input_and_metadata` and `_build_direct_model_input_and_metadata` did `for key, value in kwargs.items(): if key not in [...]: input_data[key] = value` — every non-`deps` / non-`{model,messages}` kwarg landed in span input. That flowed `usage` (a mutable counter), `event_stream_handler` (a callable), `infer_name`, `instrument`, and anything pydantic_ai adds in future SDK versions straight into telemetry. `deps` (user's app-level container — commonly holds API keys / DB connections) was the sole excluded key on the agent path; on the direct-model path there was no exclusion at all. Replaced with per-surface allowlist tuples (`_AGENT_INPUT_ALLOWLIST`, `_DIRECT_MODEL_INPUT_ALLOWLIST`) covering the known kwargs across `Agent.run` / `run_sync` / `run_stream` / `run_stream_events` / `to_cli_sync` and `direct.model_request*`. Explicitly excludes `deps`, `usage`, `event_stream_handler`, `infer_name`, `instrument`. - **Eager message reshaping dropping fields.** `_shape_message` / `_shape_content_part` / `_shape_model_response` reshaped every message into shallow dicts via a hard-coded field allowlist (`_MESSAGE_FIELDS`, `_PART_FIELDS`, `_RESPONSE_FIELDS`) even when no binary content needed materialization. That dropped real pydantic_ai v2 fields — `instructions`, `run_id`, `conversation_id`, `metadata`, `state`, `provider_details`, `outcome`, ... — and burned CPU rebuilding dicts that `bt_safe_deep_copy` (which already handles dataclasses + Pydantic models) would produce anyway at log time. Added `_has_binary_leaf` walker; the four shape helpers now short-circuit to the raw object when no binary is present. Reshape (and its field allowlist) only runs when a binary attachment actually needs materialization. Also: - Extracted `_shape_toolset(ts)` from a nested 20-line block inside `_build_agent_input_and_metadata`. - Trimmed docstrings/comments that just restated the helper name. Kept the ones explaining non-obvious *why* (portal-thread context propagation, wrapper-vs-leaf token ownership, 1.78.0 ToolManager alias, `_system_prompts` private-API access). - Hoisted `import inspect` from inside `_shape_type` to module top. ## Test plan - [x] **No new mocks/fakes.** All provider-behavior coverage stays on the existing `@pytest.mark.vcr` cassette-backed tests. - [x] **No cassette re-recording.** HTTP behavior unchanged; only in-process input/shape logic was touched. - [x] `nox -s "test_pydantic_ai_integration(latest)"` — 68/68 pass (`pydantic-ai==2.13.0`) - [x] `nox -s "test_pydantic_ai_integration(1.10.0)"` — 68/68 pass (min-supported matrix version) - [x] `nox -s "test_pydantic_ai_wrap_openai(latest)"` — green - [x] `nox -s "test_pydantic_ai_logfire(latest)"` — 1/1 pass Two new tests pin the security invariant: - `test_agent_input_kwargs_are_allowlisted` — asserts `deps` / `usage` / `event_stream_handler` / `infer_name` never land in span input; allowlisted kwargs (`instructions`, `usage_limits`, `retries`, `conversation_id`, `metadata`) do. - `test_direct_model_request_kwargs_are_allowlisted` — asserts `instrument` never leaks; `model_settings` / `model_request_parameters` do. One existing test replaced: `test_v2_message_and_response_fields_are_shaped` → `test_shape_message_and_response_pass_through_when_no_binary` — the old test was locking in the reshape that was losing v2 fields. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- sfk:created-approved-by --> Created by abhijeet <!-- sfk:slack-thread --> [Slack thread](https://starfolkai.slack.com/archives/C0AQDETAVT3/p1784659060950489?thread_ts=1784659060.950489&cid=C0AQDETAVT3) --------- Co-authored-by: Starfolk <noreply@starfolk.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Aligns the OpenAI integration with
.agents/skills/sdk-integrations/SKILL.md, following the same pattern as #604 (mistral), #594 (litellm), #603 (livekit), and #583 (anthropic). Audit turned up two issues:Denylist metadata pass-through (real leak vector). Every
_parse_paramscopy (ChatCompletionWrapper,ResponseWrapper,BaseWrapperfor Embedding/Moderation/Speech,_AudioFileWrapper,_ImageBaseWrapper) didmetadata: {**params, "provider": "openai"}— every non-input request kwarg landed in span metadata.ResponseWrapper._parse_event_from_resultdid the same on the response side ({k: v for k, v in result.items() if k not in ["output", "usage"]}). That flowedextra_headers,extra_query,extra_body, user-provided Responsesmetadata,user,safety_identifier,prompt_cache_key, and anything OpenAI adds in future SDK versions straight into telemetry. Three tests actively ratified the header leak.Replaced with per-surface allowlist tuples plus a shared
_filter_metadatahelper that dropsNOT_GIVEN/Omitsentinels and serializesresponse_formatinline. Mirrors Anthropic'sMETADATA_PARAMS.Eager response serialization.
_try_to_dict(raw_response)was full-dumping every non-streaming Chat/Response API response just to readusage+choices/output—bt_safe_deep_copyalready dumps at log time. Same fix pattern the mistral/litellm/anthropic PRs used: reads viagetattrthrough a small_get_attrhelper, plus a narrow_choices_outputthat dumps onlychoices(needed for the audio-attachment walker in_process_attachments_in_chat_output)._parse_event_from_resultand_extract_moderation_metadatawere made dict-or-object aware.Streaming paths still pre-dump chunks via
_try_to_dict(item)because the aggregator reads deeply nested delta fields — that's not the "over-serialize" case the SKILL warns about.Note on
moderationAdded to both chat + responses request allowlists and the responses result allowlist. It's a Braintrust-proxy request parameter (
{"model": "omni-moderation-latest"}) that deep-merges with the response'smoderationobject ({"input": ..., "output": ...}) at log time viamerge_dicts. The two existing*_inline_moderation_metadatatests rely on this merge to seelogged_moderation["model"].Test plan
@pytest.mark.vcrcassette-backed tests.nox -s "test_openai(latest)"— 76/76 pass acrosstest_openai.py(70),test_openai_ddtrace.py(3),test_openai_openrouter_gateway.py(3)nox -s "test_openai(1.92.0)",test_openai(1.77.0),test_openai(1.71.0)— all greennox -s "test_openai_http2_streaming(latest)"— 3/3nox -s "test_btx_openai(latest)"— 5/5nox -s "test_openai_agents(latest)"— 7/7ruff check+ruff format --checkon touched files — cleanThree tests updated: assertions that expected
metadata["extra_headers"]["X-Stainless-Helper-Method"] == "chat.completions.stream"flipped to"extra_headers" not in span["metadata"]— pins the specific leak vector the fix closes.🤖 Generated with Claude Code
Created by abhijeet
Slack thread