Conversation
…I_BASE_URL mismatch
![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: a73d393368
ℹ️ 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".
| model_base = ( | ||
| os.environ.get("OPENAI_BASE_URL") or os.environ.get("OPENAI_API_BASE") or "" | ||
| ).strip() |
There was a problem hiding this comment.
When only OPENAI_API_BASE is set, this branch logs that model traffic uses that URL even though OpenAIProvider._get_client passes only OPENAI_BASE_URL as its environment-derived base_url (src/agents/models/openai_provider.py:162-166). A repository-wide search finds no SDK model path that consumes OPENAI_API_BASE, so model requests still target the default host and this emits a false mismatch diagnostic. Remove this alias from the warning condition, or make the provider honor it as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ca34a12. The warning now keys only off OPENAI_BASE_URL, which is the env var OpenAIProvider._get_client actually reads. OPENAI_API_BASE alone no longer emits a mismatch diagnostic.
| # Keep a client open for connection pooling across multiple export calls | ||
| self._client = httpx2.Client(timeout=httpx2.Timeout(timeout=60, connect=5.0)) | ||
| self._warn_if_trace_endpoint_ignores_model_base_url() |
There was a problem hiding this comment.
When an application sets OPENAI_BASE_URL and calls the supported set_tracing_disabled(True) before its first run, that call first initializes the default provider and processor, so this constructor emits the warning before DefaultTraceProvider.set_disabled records the manual disable. The application will not create or export any traces, making the new message that traces “still export” false and capable of triggering erroneous operational or privacy alerts. Check at the first enabled export, or after the provider's disabled state is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ca34a12. The warning is deferred to the first enabled export (_export_with_deadline), so constructing the default exporter during set_tracing_disabled(True) no longer claims that traces still export.
| logger.warning( | ||
| "[non-fatal] Tracing still exports to %s while model traffic uses %s. " | ||
| "Set OPENAI_TRACING_INGEST_ENDPOINT to redirect traces, or disable tracing with " | ||
| "OPENAI_AGENTS_DISABLE_TRACING=1.", | ||
| self.endpoint, | ||
| model_base, |
There was a problem hiding this comment.
If OPENAI_BASE_URL contains userinfo or a signed query parameter for a gateway, this new warning interpolates the complete URL into an ordinary logger call. This path has no redaction, so the credential is written to application logs whenever the default tracing endpoint is selected, even when model/tool payload logging is disabled. Log a sanitized URL or only its origin instead.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ca34a12. Logged model-base and ingest URLs now drop userinfo, query, and fragment before the warning is emitted.
Warn on the first enabled export, not construction. Ignore OPENAI_API_BASE. Redact userinfo, query, and fragment from logged URLs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca34a12c2f
ℹ️ 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".
| if not items: | ||
| return | ||
|
|
||
| self._warn_if_trace_endpoint_ignores_model_base_url() |
There was a problem hiding this comment.
When OPENAI_BASE_URL is set but neither an item-specific tracing key nor OPENAI_API_KEY is present—for example, model credentials were provided to OpenAIProvider(api_key=...)—this call emits and permanently consumes the once-only warning, then lines 185-188 skip every request. Consequently the warning falsely claims tracing exports off the custom model endpoint, and a later set_tracing_export_api_key() cannot produce the warning once exports actually begin; invoke it only after resolving a usable key for a group.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the follow-up commit. The mismatch warning now runs only after a group has a usable tracing key, so a key-less export no longer consumes the once-only diagnostic.
Skip the once-only warning when export is skipped for a missing API key, so a later set_tracing_export_api_key() can still surface the mismatch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c33235ace
ℹ️ 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".
| global _warned_default_trace_endpoint_with_custom_model_base | ||
| if _warned_default_trace_endpoint_with_custom_model_base: | ||
| return | ||
| model_base = (os.environ.get("OPENAI_BASE_URL") or "").strip() |
There was a problem hiding this comment.
When an application explicitly sets OPENAI_BASE_URL=https://api.openai.com/v1 (or the trailing-slash variant), this truthiness check emits the new once-only warning even though OpenAIProvider._get_client sends model requests to the same OpenAI host as the default tracing exporter. The suggested redirect/disable action is therefore a false operational diagnostic; compare the normalized model endpoint’s origin with the OpenAI origin before consuming the warning.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The warning now compares origins, so OPENAI_BASE_URL=https://api.openai.com/v1 (and the trailing-slash variant) no longer emits a false mismatch diagnostic.
| host = f"[{hostname}]" | ||
| else: | ||
| host = hostname | ||
| if parts.port is not None: |
There was a problem hiding this comment.
If OPENAI_BASE_URL has a malformed or out-of-range port, urlsplit() succeeds but accessing parts.port here raises ValueError. The first enabled trace export then fails before post(); the batch processor drops that batch (and direct BackendSpanExporter.export() calls raise) despite having a usable tracing endpoint and API key. Include port access in the parsing error handling so this diagnostic cannot disable exporting.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Port access is included in URL parsing error handling, so a malformed or out-of-range port in OPENAI_BASE_URL cannot raise during the diagnostic or drop the export.
Do not warn when OPENAI_BASE_URL is api.openai.com. Invalid ports in that URL no longer raise during log redaction and cannot drop a trace export.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 938fd7f3fe
ℹ️ 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".
| else: | ||
| host = hostname | ||
| netloc = f"{host}:{port}" if port is not None else host | ||
| redacted = urlunsplit((scheme, netloc, path, "", "")) |
There was a problem hiding this comment.
When an OpenAI-compatible gateway puts an authentication token in its base path (for example, https://gateway.example/v1/tenants/<token>), the first enabled trace export logs the entire OPENAI_BASE_URL: this reconstruction retains path. Such URLs are valid model base URLs, so the new warning leaks the token to application logs; redact the path or log only the origin. Fresh evidence beyond the addressed userinfo/query case: urlunsplit(..., path, ...) preserves this component.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 444ddff. The mismatch warning now logs only the origin, so a token in the OPENAI_BASE_URL path is not written to application logs.
| def endpoint(self) -> str: | ||
| return ( | ||
| self._endpoint | ||
| or os.environ.get("OPENAI_TRACING_INGEST_ENDPOINT") |
There was a problem hiding this comment.
When OPENAI_TRACING_INGEST_ENDPOINT is set to a semantically equivalent official URL such as https://api.openai.com:443/v1/traces/ingest (or an uppercase host), the new fallback makes that value self.endpoint, but _should_sanitize_for_openai_tracing_api() uses a case- and port-sensitive string comparison. It therefore skips the truncation and usage cleanup required by OpenAI ingest and sends payloads that are normally sanitized, causing the backend to reject otherwise exportable trace batches; compare normalized endpoint origins and paths rather than raw strings.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 444ddff. _should_sanitize_for_openai_tracing_api() now compares normalized origin and path, so equivalent official ingest URLs such as https://api.openai.com:443/v1/traces/ingest or an uppercase host still sanitize payloads.
| self._endpoint | ||
| or os.environ.get("OPENAI_TRACING_INGEST_ENDPOINT") | ||
| or self._OPENAI_TRACING_INGEST_ENDPOINT |
There was a problem hiding this comment.
When an application constructs BackendSpanExporter(endpoint=DEFAULT_ENDPOINT) and sets OPENAI_BASE_URL to a gateway, the mismatch warning tells it to set OPENAI_TRACING_INGEST_ENDPOINT; however, the new _endpoint precedence means the constructor argument still wins, so that environment variable cannot redirect the trace request. The once-only warning is then consumed while its suggested remediation has no effect; direct callers with an explicit endpoint to change that argument or omit it instead. Fresh evidence beyond the prior warning findings: the added resolver explicitly prioritizes _endpoint over the environment.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 444ddff. When endpoint= is passed to BackendSpanExporter, the mismatch warning tells callers to change or omit that argument instead of suggesting OPENAI_TRACING_INGEST_ENDPOINT, which cannot override an explicit constructor value.
|
I found one released-surface compatibility concern in the endpoint refactor. Before this PR, |
…truction Restore the released public attribute so exporter.endpoint = url updates the configured ingest target and invalidates the resolved value.
|
Addressed in 9e71d7f. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e71d7f791
ℹ️ 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".
| global _warned_default_trace_endpoint_with_custom_model_base | ||
| if _warned_default_trace_endpoint_with_custom_model_base: | ||
| return | ||
| model_base = (os.environ.get("OPENAI_BASE_URL") or "").strip() |
There was a problem hiding this comment.
When OPENAI_BASE_URL is set to a gateway but a supported OpenAIProvider(base_url="https://api.openai.com/v1") is used, this still claims that model traffic uses the gateway. OpenAIProvider._get_client gives its explicit base_url precedence over the environment variable, so both the model and the default exporter use OpenAI while this emits and consumes the once-only mismatch warning. Either phrase this as an environment-setting notice or propagate the resolved provider endpoint to the diagnostic.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the follow-up commit. The exporter cannot see a later OpenAIProvider(base_url=...), so the mismatch warning now reports the OPENAI_BASE_URL setting instead of claiming that model traffic uses that host.
The exporter cannot see OpenAIProvider(base_url=...), so do not claim model traffic uses the environment value.
2 participants
Summary
OPENAI_BASE_URLredirects model traffic, but the tracing exporter still posted spans tohttps://api.openai.com/v1/traces/ingest. Credentials already came from the environment; the ingest endpoint did not.OPENAI_TRACING_INGEST_ENDPOINTonBackendSpanExporter, same fallback shape as the existing key/org/project accessors.endpoint=constructor argument still wins.OPENAI_BASE_URLis set to a different origin than the default OpenAI ingest host.OPENAI_API_BASEis unused by the SDK model client.set_tracing_disabled/OPENAI_AGENTS_DISABLE_TRACINGunchanged. Construction of the default exporter no longer emits the warning.This is the code side of #5008. #5009 documents the existing constructor path and does not add the environment override.
Test plan
75 passed locally (16 endpoint tests, 3 existing processor API-key tests, 56 existing trace-processor tests).
Issue number
Fixes #5008
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR