fix(codex): keep pre-connection DNS/network failures off account health (#914) - #966
fix(codex): keep pre-connection DNS/network failures off account health (#914)#966Yuxin-Qiao wants to merge 5 commits into
Conversation
…th (lidge-jun#914) Bun 1.3.14 collapses DNS failure and TCP refusal into one pre-connect label class (ConnectionRefused / FailedToOpenSocket, errno 0, no cause), and the transport layer previously mapped every non-timeout rejection to connect_error. After upstreamFailoverThreshold such failures, a healthy pool account was soft-avoided, thread affinity cleared, and another account promoted - rotation that cannot repair a machine-wide outage. Classify proven pre-connection reachability failures as account-neutral: - new leaf classifier matching stable codes through a bounded cause chain (never message substrings); ECONNRESET/EPIPE/TLS/timeout/unknown shapes keep their existing account-attributed handling - new (provider, host) health ledger with its own threshold absorbs the failures; account streak, soft-avoid, affinity, and active account stay untouched, and any owned quota-probe lease is released - pool sends use redirect: manual, so a 3xx to a dead host is relayed as a Response and can never masquerade as a pre-connection rejection; 3xx is an explicit neutral class (no account or host evidence) - same classification on the compact path, the alternate-account send, and the search/live/images/web-search/vision sidecar outcome sites Verified on Bun 1.3.14: DNS failure and TCP refusal both reject with the pre-connect label class (errno 0, no cause), alternating between labels; read-then-close rejects with ECONNRESET and stays account-attributed.
📝 WalkthroughWalkthroughChangesNeutral upstream reachability handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Relay
participant Upstream
participant Routing
participant HostHealth
Client->>Relay: Send request
Relay->>Upstream: Fetch with manual redirect handling
alt Pre-connection DNS/TCP failure
Upstream-->>Relay: Transport error
Relay->>Routing: Record connect_neutral with host and code
Routing->>HostHealth: Update provider-host failure ledger
else Account-attributed failure
Upstream-->>Relay: Post-connection error or retry evidence
Relay->>Routing: Record connect_error or transient outcome
else Redirect response
Upstream-->>Relay: 3xx response
Relay-->>Client: Relay redirect without following it
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
opencodex/src/providers/openai-sidecar.ts
Line 127 in fe693ae
Forward transport metadata for exact sidecars
For account-qualified search and vision sidecars, this recorder accepts only outcome, so the newly supplied host and lastFailureCode arguments are discarded. A DNS/TCP failure remains account-neutral, but it never reaches hostConnectHealth, causing getHostConnectHealth and isHostConnectOutage to undercount precisely those fixed-account requests. Accept and merge the metadata here as the non-exact recorder does.
AGENTS.md reference: src/AGENTS.md:L17-L17
ℹ️ 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".
| const kind = classifyTransportFailureKind(err); | ||
| upstream.recordOutcome?.(kind, { | ||
| host: transportFailureHost(url) ?? undefined, | ||
| lastFailureCode: transportErrorCode(err), | ||
| }); |
There was a problem hiding this comment.
When the ChatGPT search endpoint returns a redirect to an unreachable host, this fetch follows it by default, and Bun reports the failed follow-up as ConnectionRefused; the new classifier therefore records connect_neutral even though the original endpoint already received the selected account's credential. That leaves the account healthy and prevents subsequent pool failover for a credential-dependent redirect—the counterexample that redirect: "manual" now avoids only on responses and compact sends. Use manual redirects on this and the images/live/web-search/vision pool sidecar sends before applying the neutral classification.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
|
Please put your Pull-Request on Ready for Review, once you are finished. |
|
Reviewed against the four designs this repository already falsified for #914 ( First, credit where it is due. This is a real advance: it uses the actual Bun 1.3.14 error labels including both alternating ones, it does not repeat the hostname-resolution design, and it closes the redirect counterexample on the pool Responses and Compact paths with manual redirects. TLS and fake-IP certificate codes correctly stay account-scoped. That is more than the previous four managed. Blocker 1 — mixed 5xx → rejection still loses the attributable failure. A genuine account-attributable 503 becomes account-neutral. That is exactly the hole the earlier audit documented. The direction that fixes it is preserving per-attempt outcome evidence rather than classifying only the terminal promise. Blocker 2 — falsification 3 survives on the five newly-classified sidecar paths. Manual redirects were added only to Responses ( This is the part I would most like you to reconsider: the sidecar expansion carries the unresolved hole beyond #914's original sites, which makes the blast radius larger than the bug. Scope. 822/-39 across 20 files bundles the core attribution fix, a new host-health ledger, five sidecar families, exact-account recorder changes, and 518 lines of tests. The core Responses/Compact fix alone would be reviewable and likely mergeable; splitting it out is the fastest path to landing something. Also worth adding: a redirect activation test and a mixed-503 test. The current suite proves the implemented classification but not the two cases above — I ablated the classifier and the existing tests do go red, so they are load-bearing for what they cover. #922 is superseded by this. It misses one alternating Bun label entirely, keeps default redirects, has no host-level recording, and bundles unrelated probe-lease work while sitting at Thanks for taking this on — four designs died here before yours, and this is the first one that survives half the gauntlet. |
…car redirects) Keep prior transient 5xx statuses attached when fetchWithTransientRetry rejects, so a mixed 503 -> rejection stays account-attributed instead of being downgraded to the pre-connection neutral class (review blocker 1). Extend redirect:manual to the search/images/live/web-search/vision sidecar sends so a credential-bearing 3xx to a dead host is relayed as the neutral 3xx class (review blocker 2). Exact-account sidecar recorders now forward host/lastFailureCode into the (provider, host) ledger (Codex review P2). Tests: evidence-wrapper classification units, mixed-503 and redirect activation e2e, exact-account host-ledger coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
1742-1757: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnable manual redirects for all credential-bearing forward sends.
On Line 1745,
usesCodexForwardPoolAuthis false for direct forward authentication because its auth context ismain. The regular Responses path then follows redirects automatically. Line 404 has the same condition in the compact path.A direct forward request still sends caller-derived credentials to the original upstream. Automatic redirect following violates the required no-follow policy, can turn a relayed 3xx into an unrelated downstream connection failure, and can expose credentials if the runtime preserves authorization across a cross-host redirect.
Use a predicate for credential-bearing forward sends, not only pooled sends. Add direct-forward redirect regression tests for both endpoints.
src/server/responses/core.ts#L1742-L1757: passmanualRedirect: truewhen the request uses forward authentication, including direct mode.src/server/responses/compact.ts#L393-L405: derive the redirect flag fromsendProvider.authMode === "forward"or an equivalent credential-bearing-send predicate.Proposed direction
- const poolUpstreamSend = usesCodexForwardPoolAuth(authCtx, route.provider); + const credentialBearingForwardSend = route.provider.authMode === "forward"; - }, recovery), upstream.signal, connectMs, parsed.stream, providerFetch(route.provider), - poolUpstreamSend); + }, recovery), upstream.signal, connectMs, parsed.stream, providerFetch(route.provider), + credentialBearingForwardSend);- usesCodexForwardPoolAuth(authCtx, route.provider), + sendProvider.authMode === "forward",As per path instructions, “For credential-bearing forward/passthrough requests, relay only the curated safe header allowlist and avoid following redirects that could send credentials to another host; return 3xx responses directly when redirects are handled manually.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1742 - 1757, Enable manual redirect handling for every credential-bearing forward send, not only pooled sends. In src/server/responses/core.ts lines 1742-1757, update the redirect predicate passed to the upstream fetch so direct forward authentication is included; in src/server/responses/compact.ts lines 393-405, derive the same flag from sendProvider.authMode === "forward" or an equivalent predicate. Add direct-forward redirect regression coverage for both endpoints and preserve direct 3xx responses.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/configuration/providers.md`:
- Line 25: Expand the upstreamFailoverThreshold documentation in
docs-site/src/content/docs/reference/configuration/providers.md:25-25 to state
that pre-connection DNS/TCP reachability failures are tracked at provider-host
scope and do not affect account health, cooldowns, thread/session affinity,
active-account selection, or Pool routing. Apply semantically equivalent
translations in
docs-site/src/content/docs/ja/reference/configuration/providers.md:24-24,
docs-site/src/content/docs/ko/reference/configuration/providers.md:24-24,
docs-site/src/content/docs/ru/reference/configuration/providers.md:25-25, and
docs-site/src/content/docs/zh-cn/reference/configuration/providers.md:24-24.
In `@tests/issue-914-transport-attribution.test.ts`:
- Around line 361-367: Update the 307 response assertions in the relevant
transport attribution test to verify that res.headers.get("location") equals
"https://dead.invalid/x". Keep the existing status and health/account assertions
unchanged, ensuring credential-bearing forwarded redirects preserve the Location
header.
In `@tests/server-search.test.ts`:
- Around line 357-395: Update the upstream response handling in the search
server to copy the Location header alongside content-type, preserving 307
redirects and their original target. Add a server-search test using redirect:
"manual" that verifies the 307 status and Location value remain intact and
confirms the redirect target host receives no request.
---
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 1742-1757: Enable manual redirect handling for every
credential-bearing forward send, not only pooled sends. In
src/server/responses/core.ts lines 1742-1757, update the redirect predicate
passed to the upstream fetch so direct forward authentication is included; in
src/server/responses/compact.ts lines 393-405, derive the same flag from
sendProvider.authMode === "forward" or an equivalent predicate. Add
direct-forward redirect regression coverage for both endpoints and preserve
direct 3xx responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bcb6f242-a378-4d25-9af1-a8a6d2e0d3cd
📒 Files selected for processing (22)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/codex/routing.tssrc/lib/upstream-reachability.tssrc/lib/upstream-retry.tssrc/providers/openai-sidecar.tssrc/server/images.tssrc/server/live.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/server/responses/fetch-helpers.tssrc/server/search.tssrc/vision/describe.tssrc/web-search/executor.tstests/codex-routing.test.tstests/issue-914-transport-attribution.test.tstests/server-auth.test.tstests/server-search.test.tstests/upstream-reachability.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80cd0cee43
ℹ️ 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".
| body: JSON.stringify(body), | ||
| signal: linkedSignal.signal, | ||
| // #914: never follow a redirect into a dead-host rejection after the credential was seen. | ||
| redirect: "manual", |
There was a problem hiding this comment.
When images.provider names a custom API-key provider whose /v1/images/... endpoint uses a same-origin 307/308 for canonical or regional routing, this unconditional manual policy stops before the image request reaches its final endpoint. The handler then returns the 3xx while copying only content-type, dropping Location, so Codex cannot follow the redirect either. Restrict manual redirects to the ChatGPT pool path affected by account-health attribution, or safely follow validated same-origin redirects for keyed/custom providers.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| if (evidence && evidence.transientStatuses.length > 0) return "connect_error"; | ||
| return "connect_neutral"; |
There was a problem hiding this comment.
When the first reset-retry attempt reaches the origin and is reset after the request is read, but the next attempt fails with ConnectionRefused or another reachability code, fetchWithResetRetry throws only that final rejection. Consequently evidence is undefined here and the whole send becomes connect_neutral, discarding the earlier account-attributable reset and leaving the failure streak untouched. Carry prior reset evidence through the retry helper, as is now done for prior 5xx responses, and classify this mixed sequence as connect_error.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
…lidge-jun#914) CodeRabbit review (commit 80cd0ce): direct-mode /v1/responses and /v1/responses/compact still default-followed redirects because the flag was gated on usesCodexForwardPoolAuth, which is false for direct auth. Gate manual redirects on provider.authMode === "forward" instead, so pool, direct, and exact sends all relay a 3xx as-is. Relay fixes: search and images copy the upstream Location header, and compact passes Location through, so a relayed 3xx stays followable. Docs (en/ja/ko/ru/zh-cn) now state that pre-connection DNS/TCP failures are tracked at provider-host scope and never affect account health, cooldowns, affinity, active-account selection, or Pool routing. Tests: direct-forward 307 activation for Responses and Compact, search 307 + Location preservation with no follow, and a Location assertion on the pool 307 e2e.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71bd8210a4
ℹ️ 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".
| const location = upstreamResponse.headers.get("location"); | ||
| if (location) relayHeaders["location"] = location; |
There was a problem hiding this comment.
When a configured images.provider returns an authentication-required 307/308, forwarding Location does not restore redirect support: the client follows from the proxy response without the provider API key held only in headers, so an absolute target is unauthenticated and a relative target resolves against the proxy itself. The newly added Location relay is fresh evidence beyond the existing comment, but the keyed-provider flow still breaks; follow validated redirects inside the proxy while retaining provider authentication, and reserve manual relay behavior for the ChatGPT account-attribution path.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/images.ts (1)
457-458: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCredential Transmission Over Unencrypted HTTP (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal · Exploitability: Moderate
Reject
http:for credential-bearing image providers.
providerBaseUrlConfigErroracceptshttp:. A customimages.providerthen sendsAuthorization: Bearer ${apiKey}toprovider.baseUrl;redirect: "manual"does not protect this first hop. Requirehttps:before this fetch or enforce the shared outbound policy. The search path is restricted to the canonical HTTPS ChatGPT provider.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/images.ts` around lines 457 - 458, Require HTTPS for credential-bearing image provider requests before the fetch in the image provider flow, or route them through the shared outbound policy; do not rely on redirect: "manual" to protect the initial hop. Update the validation or request logic associated with providerBaseUrlConfigError while preserving the canonical HTTPS restriction already enforced in src/server/search.ts lines 157-158, which requires no direct change.src/server/responses/core.ts (1)
430-430: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal · Exploitability: Difficult
Reachability path
● Entry src/server/responses/compact.ts │ ▼ ● Sink src/server/responses/core.tsEnforce HTTPS for every forward-mode destination.
loadConfig()accepts passthrough provider fields without callingproviderManagementConfigError(). A hand-editedopenai-responsesprovider can therefore retainauthMode: "forward"with anhttp:baseUrl;routedProviderConfig()preserves it. The adapter then forwardsAuthorizationor the pool access token, andsrc/server/responses/core.ts:1742-1756sends it to that URL before receiving a response. Enforce the HTTPS and canonical-destination invariant during config loading or immediately before request construction. Apply the same guard to the compact path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` at line 430, Update loadConfig or the shared request-construction guard to validate every forward-mode provider destination with providerManagementConfigError, rejecting non-HTTPS or non-canonical baseUrl values before forwarding credentials. Ensure routedProviderConfig preserves only validated destinations, and apply the same validation to the compact request path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/ja/reference/configuration/providers.md`:
- Line 24: Preserve the “proven” qualifier for pre-connection DNS/TCP
reachability failures in the translated provider configuration descriptions.
Update docs-site/src/content/docs/ja/reference/configuration/providers.md lines
24-24, docs-site/src/content/docs/ko/reference/configuration/providers.md lines
24-24, and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md
lines 24-24 to state that such failures must be confirmed or proven; unknown
failures must remain account-attributed.
In `@src/server/responses/core.ts`:
- Around line 1742-1746: Reject non-HTTPS endpoints for authMode "forward"
providers before any credentials are sent: update providerBaseUrlConfigError
validation, then add defense-in-depth scheme checks before the sends at
src/server/responses/core.ts:1757-1758, src/server/responses/core.ts:430, and
src/server/responses/compact.ts:406-408; ensure forwardCredentialedSend and the
related fetch paths fail without transmitting credentials when the endpoint is
not HTTPS.
In `@tests/server-search.test.ts`:
- Around line 429-435: Extend the redirect regression assertions in the test
around getCodexUpstreamHealth to also verify that
getHostConnectHealth(hostConnectHealthKey("openai", "chatgpt.com")) returns
null, confirming the relayed 307 does not update host health.
---
Outside diff comments:
In `@src/server/images.ts`:
- Around line 457-458: Require HTTPS for credential-bearing image provider
requests before the fetch in the image provider flow, or route them through the
shared outbound policy; do not rely on redirect: "manual" to protect the initial
hop. Update the validation or request logic associated with
providerBaseUrlConfigError while preserving the canonical HTTPS restriction
already enforced in src/server/search.ts lines 157-158, which requires no direct
change.
In `@src/server/responses/core.ts`:
- Line 430: Update loadConfig or the shared request-construction guard to
validate every forward-mode provider destination with
providerManagementConfigError, rejecting non-HTTPS or non-canonical baseUrl
values before forwarding credentials. Ensure routedProviderConfig preserves only
validated destinations, and apply the same validation to the compact request
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19ab9edb-31ab-4095-98d2-e67ecfc09ad9
📒 Files selected for processing (11)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/server/images.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/server/search.tstests/issue-914-transport-attribution.test.tstests/server-search.test.ts
| | `accountPoolStrategy?` | `"quota" \| "round-robin" \| "fill-first"` | `"quota"` | 新規/未紐付け Codex リクエストの割り当て戦略。live な `(parent thread id, quota scope)` affinity がなければ未紐付けで、プロキシ再起動や affinity リセット後は既存の表示タスクも未紐付けになり得ます。`quota` はアクティブアカウントがなければ既知 usage 最小の適格アカウントを選び、適格なアクティブアカウントが `autoSwitchThreshold` 未満なら維持します。しきい値到達後は、未紐付けリクエストまたは紐付け済みタスクの次のリクエストを usage の低い適格アカウントへ移せます。`round-robin` は未紐付けリクエストを均等分散し、`fill-first` は cooldown、使用不可、または drain threshold までアクティブアカウントへ割り当てます。 | | ||
| | `accountPoolStickyLimit?` | `number` | `1` | 1 回の round-robin 選択で次へ進む前に保持する新規/未紐付けタスク割り当て数。カウンターは上流の成功後ではなくタスクの紐付け時に増えます。範囲 1–100。`accountPoolStrategy` が `round-robin` のときのみ。 | | ||
| | `upstreamFailoverThreshold?` | `number` | `3` |今後の新しいセッションがフェイルオーバーする前に一時的なエラーが連続して発生する。 `0` を無効に設定します。 | | ||
| | `upstreamFailoverThreshold?` | `number` | `3` |今後の新しいセッションがフェイルオーバーする前に一時的なエラーが連続して発生する。 `0` を無効に設定します。接続前のDNS/TCP到達不能障害はprovider-host単位で記録され、アカウントの健全性、クールダウン、スレッド/セッションの親和性、アクティブアカウントの選択、Poolルーティングには影響せず、この閾値にもカウントされません。 | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the “proven” classification qualifier in every translation.
The English source limits neutral handling to proven pre-connection DNS/TCP reachability failures. Without that qualifier, these translations imply that every apparent pre-connection failure is neutral. Unknown failures must remain account-attributed.
docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L24: State that the DNS/TCP reachability failure must be confirmed or proven.docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L24: State that the DNS/TCP reachability failure must be confirmed or proven.docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L24: State that the DNS/TCP reachability failure must be confirmed or proven.
As per path instructions, translated locale pages must not contradict the English source.
📍 Affects 3 files
docs-site/src/content/docs/ja/reference/configuration/providers.md#L24-L24(this comment)docs-site/src/content/docs/ko/reference/configuration/providers.md#L24-L24docs-site/src/content/docs/zh-cn/reference/configuration/providers.md#L24-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/ja/reference/configuration/providers.md` at line
24, Preserve the “proven” qualifier for pre-connection DNS/TCP reachability
failures in the translated provider configuration descriptions. Update
docs-site/src/content/docs/ja/reference/configuration/providers.md lines 24-24,
docs-site/src/content/docs/ko/reference/configuration/providers.md lines 24-24,
and docs-site/src/content/docs/zh-cn/reference/configuration/providers.md lines
24-24 to state that such failures must be confirmed or proven; unknown failures
must remain account-attributed.
Source: Path instructions
| // Manual redirect on every credential-bearing forward send (pool AND | ||
| // direct): a 3xx surfaces as a Response instead of a followed redirect, | ||
| // so a server that redirects to a dead host can never masquerade as a | ||
| // pre-connection failure after the credential was seen (#914). | ||
| const forwardCredentialedSend = route.provider.authMode === "forward"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 "fetchWithHeaderTimeout|fetchWithResetRetry|authMode|baseUrl|new URL|https:|http:|redirect[[:space:]]*:" srcRepository: lidge-jun/opencodex
Length of output: 50378
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- targeted call sites ---'
rg -n -C 14 'forwardCredentialedSend|fetchWithHeaderTimeout|fetchWithResetRetry|providerFetch|authMode === "forward"|authMode: "forward"' src/server/responses/core.ts src/server/responses/compact.ts
printf '%s\n' '--- transport helper definitions and call sites ---'
rg -n -C 18 'function (fetchWithHeaderTimeout|fetchWithResetRetry|providerFetch)|const (fetchWithHeaderTimeout|fetchWithResetRetry|providerFetch)|fetchWithHeaderTimeout|fetchWithResetRetry' src
printf '%s\n' '--- relevant file sizes ---'
wc -l src/server/responses/core.ts src/server/responses/compact.tsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- fetch helper implementation ---'
rg -n -C 35 'export (async )?function fetchWithHeaderTimeout|function fetchWithHeaderTimeout|providerFetch' src/server/responses/fetch-helpers.ts
printf '%s\n' '--- forward provider URL and credential validation ---'
rg -n -C 14 'interface OcxProviderConfig|type OcxProviderConfig|authMode|validateForwardAdmissionCredential|baseUrl' src/types.ts src/server src/config.ts src/providers
printf '%s\n' '--- alternate-account function context ---'
sed -n '330,450p' src/server/responses/core.ts
printf '%s\n' '--- direct forward path context ---'
sed -n '1668,1785p' src/server/responses/core.ts
printf '%s\n' '--- compact alternate path context ---'
sed -n '390,490p' src/server/responses/compact.tsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- helper locations ---'
rg -n 'fetchWithHeaderTimeout|providerFetch' src/server/responses/fetch-helpers.ts
printf '%s\n' '--- helper implementation ---'
sed -n '1,260p' src/server/responses/fetch-helpers.ts
printf '%s\n' '--- provider type and URL validation ---'
rg -n 'OcxProviderConfig|baseUrl.*https|https.*baseUrl|new URL|authMode' src/types.ts src/config.ts src/server/config* src/providers/registry.ts src/server/route* src/server/adapter-resolve.ts
printf '%s\n' '--- exact alternate send ---'
sed -n '350,445p' src/server/responses/core.ts
printf '%s\n' '--- exact direct forward send ---'
sed -n '1728,1770p' src/server/responses/core.ts
printf '%s\n' '--- exact compact send ---'
sed -n '385,415p' src/server/responses/compact.tsRepository: lidge-jun/opencodex
Length of output: 21450
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- provider validation ---'
sed -n '580,675p' src/config.ts
sed -n '810,850p' src/config.ts
sed -n '1080,1110p' src/config.ts
printf '%s\n' '--- forward-provider predicates ---'
sed -n '1660,1745p' src/providers/registry.ts
sed -n '620,650p' src/providers/registry.ts
printf '%s\n' '--- provider type ---'
sed -n '900,945p' src/types.ts
sed -n '1015,1040p' src/types.ts
printf '%s\n' '--- request URL construction ---'
rg -n -C 8 'baseUrl.*responses|request\.url|compactUrl|forward.*baseUrl|authMode.*forward' src/adapters src/router.ts src/server/responses/core.ts src/server/responses/compact.tsRepository: lidge-jun/opencodex
Length of output: 49970
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- canonical forward predicate ---'
rg -n -C 24 'function isCanonicalOpenAiForwardProvider|export function isCanonicalOpenAiForwardProvider' src/providers src
printf '%s\n' '--- route/config handling for forward providers ---'
rg -n -C 16 'authKind === "forward"|authMode === "forward"|providerMatchesRegistryTransport|canonicalAuthMode|OPENAI_CODEX_PROVIDER_ID' src/router.ts src/config.ts src/providers/openai-tiers.ts src/providers/registry.tsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- destination policy and provider validation ---'
rg -n -C 30 'function providerDestinationConfigError|export function providerDestinationConfigError|providerDestinationConfigError\(' src/lib/destination-policy.ts src/config.ts
printf '%s\n' '--- all forward-mode validation branches ---'
rg -n -C 12 'authMode.*forward|forward.*authMode|authKind.*forward' src/config.ts src/lib/destination-policy.ts src/server/adapter-resolve.ts src/router.ts
printf '%s\n' '--- route provider construction ---'
sed -n '215,285p' src/router.ts
sed -n '320,355p' src/router.tsRepository: lidge-jun/opencodex
Length of output: 29079
Security Misconfiguration (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal · Exploitability: Moderate
Reachability path
● Entry
src/server/responses/compact.ts
│
▼
● Sink
src/server/responses/core.ts
Reject non-HTTPS forward endpoints before sending credentials.
providerBaseUrlConfigError accepts http: URLs, and custom forward providers retain that endpoint. fetchWithHeaderTimeout only disables redirect following; it does not enforce HTTPS. Reject non-HTTPS authMode: "forward" providers and add a defense-in-depth scheme check before the sends in src/server/responses/core.ts:1757, src/server/responses/core.ts:430, and src/server/responses/compact.ts:408.
📍 Affects 2 files
src/server/responses/core.ts#L1742-L1746(this comment)src/server/responses/core.ts#L1757-L1758src/server/responses/core.ts#L430-L430src/server/responses/compact.ts#L406-L408
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/responses/core.ts` around lines 1742 - 1746, Reject non-HTTPS
endpoints for authMode "forward" providers before any credentials are sent:
update providerBaseUrlConfigError validation, then add defense-in-depth scheme
checks before the sends at src/server/responses/core.ts:1757-1758,
src/server/responses/core.ts:430, and src/server/responses/compact.ts:406-408;
ensure forwardCredentialedSend and the related fetch paths fail without
transmitting credentials when the endpoint is not HTTPS.
Source: Path instructions
| expect(response.status).toBe(307); | ||
| expect(response.headers.get("location")).toBe("https://dead.invalid/x"); | ||
| // The redirect target was never contacted: manual redirects relay the 3xx as-is. | ||
| expect(upstreamCalls).toBe(1); | ||
| // 3xx stays the neutral class even for an exact-account send. | ||
| expect(getCodexUpstreamHealth("pool-a")).toBeNull(); | ||
| expect(loadConfig().activeCodexAccountId).toBe("pool-b"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that a relayed 307 does not update host health.
Line 434 verifies only account health. A regression that records the 307 in the (openai, chatgpt.com) host ledger will still pass this test. Assert that getHostConnectHealth(hostConnectHealthKey("openai", "chatgpt.com")) is null.
Proposed test update
expect(response.status).toBe(307);
expect(response.headers.get("location")).toBe("https://dead.invalid/x");
// The redirect target was never contacted: manual redirects relay the 3xx as-is.
expect(upstreamCalls).toBe(1);
// 3xx stays the neutral class even for an exact-account send.
+ expect(getHostConnectHealth(hostConnectHealthKey("openai", "chatgpt.com"))).toBeNull();
expect(getCodexUpstreamHealth("pool-a")).toBeNull();As per path instructions, changed server behavior requires focused regression coverage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(response.status).toBe(307); | |
| expect(response.headers.get("location")).toBe("https://dead.invalid/x"); | |
| // The redirect target was never contacted: manual redirects relay the 3xx as-is. | |
| expect(upstreamCalls).toBe(1); | |
| // 3xx stays the neutral class even for an exact-account send. | |
| expect(getCodexUpstreamHealth("pool-a")).toBeNull(); | |
| expect(loadConfig().activeCodexAccountId).toBe("pool-b"); | |
| expect(response.status).toBe(307); | |
| expect(response.headers.get("location")).toBe("https://dead.invalid/x"); | |
| // The redirect target was never contacted: manual redirects relay the 3xx as-is. | |
| expect(upstreamCalls).toBe(1); | |
| // 3xx stays the neutral class even for an exact-account send. | |
| expect(getHostConnectHealth(hostConnectHealthKey("openai", "chatgpt.com"))).toBeNull(); | |
| expect(getCodexUpstreamHealth("pool-a")).toBeNull(); | |
| expect(loadConfig().activeCodexAccountId).toBe("pool-b"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/server-search.test.ts` around lines 429 - 435, Extend the redirect
regression assertions in the test around getCodexUpstreamHealth to also verify
that getHostConnectHealth(hostConnectHealthKey("openai", "chatgpt.com")) returns
null, confirming the relayed 307 does not update host health.
Source: Path instructions
lidge-jun#967 found two real defects in my own lidge-jun#955 code and both verify at runtime: a Team account with a monthly window could never recover because the predicate picked its window by plan name while the parser picks by window duration, and the probe's own token refresh was mistaken for an external credential replacement. lidge-jun#963 and lidge-jun#965 both claim lidge-jun#962; lidge-jun#965 wins because it inherits from the row it actually replaces rather than recomputing config hints, and because lidge-jun#963 rewrites an existing regression contract to justify a broader change. lidge-jun#966 is a fifth design for lidge-jun#914 that survives two of the four prior falsifications but not all: mixed 5xx-then-rejection still loses the attributable failure, and five newly-classified sidecar paths keep default redirects, so a credential-visible 307 to a dead host still reads as neutral.
…n#914) Codex review (commit 71bd821): - fetchWithResetRetry now attaches credential-visible reset evidence when a retried ECONNRESET attempt is followed by a different terminal rejection, so a mixed reset -> ConnectionRefused stays account-attributed instead of classifying as pre-connection neutral. The evidence wrapper is renamed to UpstreamRetryEvidenceError and carries both transient 5xx statuses and a resetSeen flag; the classifier treats either as connect_error. - Manual redirects are scoped to the ChatGPT forward credential path in images and live relays. Keyed API-key providers keep default redirect following, so their same-origin 307/308 routing still resolves inside the proxy where the API key lives. - Docs (ja/ko/zh-cn) restore the 'proven' qualifier: unconfirmed failures remain account-attributed. Tests: reset-evidence classification units, a mixed reset -> reachability e2e on /v1/responses, and a host-ledger null assertion on the search 307 e2e.
…#914) CodeRabbit review (commit 71bd821), CWE-319: - New credentialSendSchemeError guard in the shared outbound policy: https passes, http is allowed only for loopback hosts, and cleartext http to any remote host is rejected before a credential can leave the proxy. - Applied at the credential-bearing send boundaries: the /v1/responses forward passthrough, the compact forward path, and the images relay (both the ChatGPT forward credential and keyed provider API keys). - A hand-edited custom forward provider with an http baseUrl now gets a 502 at the send boundary instead of transmitting Authorization over cleartext http (loadConfig accepts that shape; management validation already reserves forward for the canonical built-in provider). Tests: scheme-guard units (https, loopback http, remote http, malformed), a /v1/responses e2e asserting 502 + zero outbound fetches for an http forward provider, and an images e2e for a keyed http provider.
3 participants
Summary
Fixes #914: proven pre-connection DNS / network reachability failures no longer count against a Codex pool account's failure streak, so they cannot soft-avoid a healthy account, clear thread affinity, or change the effective account. The client still gets the existing
502during the outage; the account state is untouched.This implements the direction recorded in
devlog/_plan/260803_transport_attribution/000_plan.md: connection-level failures belong to a (provider, host) pair, not a credential. Every pool account shares the host, so rotation cannot repair a machine-wide outage.Design
1. Pre-connect classifier (
src/lib/upstream-reachability.ts, leaf module)isPreConnectReachabilityError()matches stablecodevalues through a bounded cause chain (depth 3). Verified empirically on Bun 1.3.14 (the exact runtime in the plan's probe):Matched set: Bun's
ConnectionRefused/FailedToOpenSocket(DNS failure and TCP refusal share the class and alternate labels) plus the Node shapes (ECONNREFUSED,ENOTFOUND,EAI_AGAIN,ENETUNREACH,ENETDOWN,EHOSTUNREACH) for the non-Bun runtimes the repo claims to support. Message substrings are never matched (negative cases tested).ECONNRESET,EPIPE, TLS codes, timeouts, and unknown shapes keep their existing account-attributed handling.Why this is not rejected design #1: the list is anchored to the labels Bun actually emits, verified by runtime probe, not the Node-only list that never fired.
2. Host health ledger (
src/codex/routing.ts)New
connect_neutraloutcome class.recordCodexUpstreamOutcomerecords it in a(provider:host)ledger with its own threshold (isHostConnectOutage, windowed like account health) and concludes any owned quota-probe lease without treating the failure as account evidence. Account streak, soft-avoid, thread affinity, and active/persisted account are never touched. The ledger is observational today;isHostConnectOutageis the hook where a host-level response (e.g. circuit break) would attach with its own audit.3. Redirect counterexample removed (
fetch-helpers.ts+ pool sends)Pool sends (
/v1/responsespassthrough, the alternate-account send,/v1/responses/compact) now useredirect: "manual". A server that 307s to a dead host produces a relayed 3xx Response instead of aConnectionRefusedrejection after the credential was seen. Explicit 3xx policy: 3xx is the neutral class — relayed as-is, never account or host health evidence (the host proved reachable by responding).Why this is not rejected design #3: the "no headers arrived" boundary is not used. The credential-visible read-then-close case (
ECONNRESET) remains account-attributed and fails over exactly as before (2026-07-22 decision preserved, with a regression test). The only class treated as neutral is the one Bun provably emits when no connection was established.Residual class (documented, not argued away)
A server that reads the credential, returns
307to a dead target, and whose follow-up is reached through some other transport path can still produce an account-neutral classification; withredirect: "manual"on pool sends that path is a relayed 3xx with an explicit policy. The mixed5xx -> rejectioncase resolves neutral on the regular, compact, and alternate-account sends — the 5xx evidence is lost insidefetchWithTransientRetry(the same hole recorded in the plan), and the final attempt's reachability failure is host-level.Tests
tests/upstream-reachability.test.ts)tests/codex-routing.test.ts)/v1/responsesreturn 502 and routing state is unchanged; recovery stays on account A; compact path parity; real Bun TCP refusal through a live proxy; read-then-closeECONNRESETstill soft-avoids and fails over (tests/issue-914-transport-attribution.test.ts)tests/server-auth.test.ts(the old test asserted the pre-fix behavior)Validation:
bun run typecheck,bun run privacy:scan, andbun run test(7933 pass; the only 10 failures areprovider management validationtests that reproduce identically on a cleanorigin/devcheckout). Docs updated indocs-site(en/ko/ja/ru/zh-cn): DNS/TCP reachability failures are account-neutral and never count towardupstreamFailoverThreshold.Follow-up (review commit
80cd0cee)Addresses the review blockers on this PR:
fetchWithTransientRetry()now attaches the prior transient statuses to the final rejection (TransientRetryEvidenceError), andclassifyTransportFailureKind()treats that evidence as account-attributable (connect_error) instead of the pre-connection neutral class. A genuine upstream 503 followed by a reachability rejection no longer vanishes from the account streak.redirect: "manual"now also applies to web search (src/web-search/executor.ts), vision (src/vision/describe.ts), search (src/server/search.ts), images (src/server/images.ts), and live (src/server/live.ts), matching the existing Responses, Compact, and alternate-account sends. A 3xx is relayed as the explicit neutral class; it can no longer be followed into a dead-host rejection after the credential was seen.host/lastFailureCodeinto the(provider, host)ledger exactly like the pool recorder, so fixed-account DNS/TCP failures no longer undercounthostConnectHealth.New tests: evidence-wrapper classification units, a mixed-503 e2e (
/v1/responses503 thenConnectionRefusedstays on the account streak), a redirect activation e2e (pool sends relay 307 instead of following into a dead host), and an exact-account search e2e asserting the host ledger entry.Validation on the review commit:
bun run typecheckgreen,bun run privacy:scangreen,bun run test7958 tests with only the same 10provider management validationfailures (environmental destination-policy DNS answers for*.example.test, unrelated to this change) plus one load-sensitive crash-guard timeout that passes in isolation.Follow-up (review commit
71bd8210)Extends the manual-redirect policy to every credential-bearing forward send, per CodeRabbit review on
80cd0cee:/v1/responsesand/v1/responses/compactnow gateredirect: "manual"onprovider.authMode === "forward"instead ofusesCodexForwardPoolAuth, so direct-mode sends (which carry the caller's credential) get the same no-follow behavior as pool sends.Locationheader (search, images, compact) so a relayed 3xx stays followable by the client.New tests: direct-forward 307 activation for Responses and Compact, search 307 +
Locationpreservation with no follow, and aLocationassertion on the pool 307 e2e. Validation:bun run typecheckandbun run privacy:scangreen; focused suites 124/124; full suite 7941 pass with the same 10 environmentalprovider management validationfailures plus two load-sensitive timing tests (crash-guard timeout, native-profile WebSocket close race) that pass in isolation.Follow-up (review commit
78c824dd)Addresses the remaining Codex review P2 findings on
71bd8210:fetchWithResetRetrynow attaches credential-visible reset evidence when a retriedECONNRESETattempt is followed by a different terminal rejection, so a mixed reset →ConnectionRefusedstays account-attributed (connect_error) instead of classifying as pre-connection neutral. The evidence wrapper is renamed toUpstreamRetryEvidenceErrorand carries both transient 5xx statuses and aresetSeenflag; the classifier treats either as account evidence.New tests: reset-evidence classification units, a mixed reset → reachability e2e on
/v1/responses, and a host-ledger null assertion on the search 307 e2e. Validation:bun run typecheckandbun run privacy:scangreen; focused suites 129/129; full suite 7964 tests with the same 10 environmentalprovider management validationfailures plus one load-sensitive 5s timing flake (shellStreamExec) that passes in isolation.Follow-up (review commit
bd55f1ab)Addresses the CodeRabbit CWE-319 findings on
71bd8210(cleartext credential transmission):credentialSendSchemeError, shared outbound policy):httpspasses,httpis allowed only for loopback hosts, and cleartexthttpto any remote host is rejected before a credential can leave the proxy./v1/responsesforward passthrough, the compact forward path, and the images relay (both the ChatGPT forward credential and keyed provider API keys). A hand-edited custom forward provider with anhttpbaseUrl now gets a502at the send boundary instead of transmittingAuthorizationover cleartext (loadConfig accepts that shape; management validation already reservesforwardfor the canonical built-in provider, so the config-level check would be unreachable).New tests: scheme-guard units (
https, loopbackhttp, remotehttp, malformed), a/v1/responsese2e asserting502plus zero outbound fetches for anhttpforward provider, and an images e2e for a keyedhttpprovider. Validation:bun run typecheckandbun run privacy:scangreen; focused suites 162/162; full suite 7970 tests with the same 10 environmentalprovider management validationfailures plus one load-sensitive 5s timing flake (crash-guard) that passes in isolation.Summary by CodeRabbit
Bug Fixes
Documentation
upstreamFailoverThresholdbehavior across supported languages.Tests