fix(gateway/discord): respect env proxy vars and prevent ECONNRESET#47446
fix(gateway/discord): respect env proxy vars and prevent ECONNRESET#47446winter-loo wants to merge 2 commits into
Conversation
openclaw-barnacle
Bot
added
channel: discord
Greptile SummaryThis PR fixes two proxy-related reliability issues for Discord gateway connections: it bootstraps a global
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/net/undici-global-dispatcher.ts
Line: 126
Comment:
**Spread order may allow `connect` to override `keepAlive: false`**
The intent of `proxyConnect` is to always enforce `keepAlive: false` to prevent ECONNRESET from local proxies. However, using `{ keepAlive: false, ...connect }` means any `keepAlive` property that `connect` carries (e.g., if `resolveConnectOptions` is ever extended) would override the `false` value. For the invariant to hold unconditionally, `keepAlive: false` should be the last spread:
```suggestion
const proxyConnect = { ...connect, keepAlive: false };
```
Currently `resolveConnectOptions` never returns `keepAlive`, so there is no active bug — but the defensive order guards against future regressions.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 3e8943e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const connect = resolveConnectOptions(autoSelectFamily); | ||
| try { | ||
| if (kind === "env-proxy") { | ||
| const proxyConnect = { keepAlive: false, ...connect }; |
There was a problem hiding this comment.
Spread order may allow connect to override keepAlive: false
The intent of proxyConnect is to always enforce keepAlive: false to prevent ECONNRESET from local proxies. However, using { keepAlive: false, ...connect } means any keepAlive property that connect carries (e.g., if resolveConnectOptions is ever extended) would override the false value. For the invariant to hold unconditionally, keepAlive: false should be the last spread:
| const proxyConnect = { keepAlive: false, ...connect }; | |
| const proxyConnect = { ...connect, keepAlive: false }; |
Currently resolveConnectOptions never returns keepAlive, so there is no active bug — but the defensive order guards against future regressions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/net/undici-global-dispatcher.ts
Line: 126
Comment:
**Spread order may allow `connect` to override `keepAlive: false`**
The intent of `proxyConnect` is to always enforce `keepAlive: false` to prevent ECONNRESET from local proxies. However, using `{ keepAlive: false, ...connect }` means any `keepAlive` property that `connect` carries (e.g., if `resolveConnectOptions` is ever extended) would override the `false` value. For the invariant to hold unconditionally, `keepAlive: false` should be the last spread:
```suggestion
const proxyConnect = { ...connect, keepAlive: false };
```
Currently `resolveConnectOptions` never returns `keepAlive`, so there is no active bug — but the defensive order guards against future regressions.
How can I resolve this? If you propose a fix, please make it concise.![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: 3e8943e82b
ℹ️ 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".
| bodyTimeout: timeoutMs, | ||
| headersTimeout: timeoutMs, | ||
| ...(connect ? { connect } : {}), | ||
| connect: proxyConnect, |
There was a problem hiding this comment.
EnvHttpProxyAgent does not apply connect settings to proxied HTTPS tunnels (those settings only affect the no-proxy/direct agent), so keepAlive: false here will not stop CONNECT tunnel reuse. This repo already codifies that behavior in extensions/telegram/src/fetch.ts and extensions/telegram/src/fetch.env-proxy-runtime.test.ts, so users behind local proxies that close reused tunnels can still hit ECONNRESET unless this is passed in proxyTls.
Useful? React with 👍 / 👎.
| } | ||
| try { | ||
| const agent = new ProxyAgent(proxy); | ||
| const agent = new ProxyAgent({ uri: proxy, connect: { keepAlive: false } }); |
There was a problem hiding this comment.
ProxyAgent uses proxyTls for proxy-connection transport options, so passing keepAlive: false under connect leaves proxy tunnel reuse unchanged. In explicit Discord proxy setups, repeated REST calls can still reuse stale CONNECT tunnels and continue surfacing ECONNRESET from proxies that close tunnels after a single request.
Useful? React with 👍 / 👎.
openclaw-barnacle
Bot
added
gateway
01c1c0f to
e7cbad7
Compare
openclaw-barnacle
Bot
removed
the
extensions: google-gemini-cli-auth
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7cbad7271
ℹ️ 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".
| }): GatewayPlugin { | ||
| const intents = resolveDiscordGatewayIntents(params.discordConfig?.intents); | ||
| const proxy = params.discordConfig?.proxy?.trim(); | ||
| const proxy = params.discordConfig?.proxy?.trim() || resolveEnvHttpProxyUrl("https"); |
There was a problem hiding this comment.
Falling back to resolveEnvHttpProxyUrl("https") here forces Discord gateway traffic into an explicit HttpsProxyAgent/ProxyAgent whenever HTTPS_PROXY is set, but this path does not evaluate NO_PROXY/no_proxy exclusions. In split-proxy environments (for example, global proxy set but discord.com bypassed via NO_PROXY), the gateway now incorrectly routes through the proxy and can fail to connect or fetch gateway metadata, even though env-proxy semantics should bypass that host.
Useful? React with 👍 / 👎.
…n proxy tunnels Gateway startup now bootstraps the global undici EnvHttpProxyAgent so Node's fetch() honors https_proxy/HTTP_PROXY. The Discord gateway plugin also falls back to env proxy vars for its WebSocket connection when no explicit channels.discord.proxy is configured. All proxy ProxyAgent instances disable keepAlive to avoid ECONNRESET from local proxies (Clash, Surge, etc.) that close CONNECT tunnels after one request. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
e7cbad7 to
cb4d03b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4d03bb86
ℹ️ 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".
| import type { RuntimeEnv } from "openclaw/plugin-sdk/runtime-env"; | ||
| import { ProxyAgent, fetch as undiciFetch } from "undici"; | ||
| import WebSocket from "ws"; | ||
| import { resolveEnvHttpProxyUrl } from "../../../../src/infra/net/proxy-env.js"; |
There was a problem hiding this comment.
This new static import makes the bundled Discord extension depend on ../../../../src/..., but npm installs package extensions/ and dist/ (not src/), and bundled-plugin discovery loads from the package-root extensions/ tree when dist-runtime/extensions is absent; in that common install path, resolving this module throws ERR_MODULE_NOT_FOUND before the Discord plugin can initialize. This means Discord users on normal npm-distributed builds can fail at startup as soon as the extension is loaded.
Useful? React with 👍 / 👎.
|
Rebased onto current Current branch diff is limited to:
Current failing checks:
What I found:
I’m happy to rerun once |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: this is a plausible Discord proxy bug fix, but the branch is not merge-ready because it crosses the plugin boundary, ignores target NO_PROXY semantics, changes bot-token proxy egress through a raw Undici path, and lacks real Discord proxy proof. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes for the patch defects: source inspection plus Undici 8.3.0 source show the plugin-boundary import, NO_PROXY bypass, raw token proxy path, and ineffective keep-alive placement. No high-confidence live reproduction exists for the original Discord local-proxy ECONNRESET path because manual proof remains unchecked. Is this the best way to solve the issue? No. The best fix should be rebuilt on current main using SDK proxy helpers and guarded Discord metadata policy rather than merging a stale raw Undici proxy path from extension code. Security review: Security review needs attention: The diff changes auth-bearing Discord proxy routing and bypasses the guarded metadata fetch path, so the security review is not cleared.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 6981051682f2. |
clawsweeper
Bot
added
the
P1
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
impact:security
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Summary
EnvHttpProxyAgentin gateway startup so Node'sfetch()honorshttps_proxy/HTTP_PROXYenv vars — fixes Discord REST calls (command deployment, bot identity fetch) timing out behind local proxieschannels.discord.proxyis configuredkeepAliveon all proxyProxyAgentinstances to preventECONNRESETfrom local proxies (Clash, Surge, etc.) that close HTTP CONNECT tunnels after one requestTest plan
pnpm test -- src/infra/net/undici-global-dispatcher.test.tspasses (10/10)pnpm test -- extensions/discord/src/monitor/provider.proxy.test.ts extensions/discord/src/monitor/provider.rest-proxy.test.tspasses (8/8)https_proxyset🤖 Generated with Claude Code