fix(doctor): diagnose ignored web fetch proxy settings#109778
Conversation
openclaw-barnacle
Bot
added
docs
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(doctor): diagnose ignored web fetch proxy settings This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
![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: ff2a1b00c6
ℹ️ 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".
| service: Pick<GatewayService, "readCommand">; | ||
| }): Promise<ProxyEnvSource[]> { | ||
| const sources: ProxyEnvSource[] = []; | ||
| if (hasEnvHttpProxyConfigured("https", params.env)) { |
There was a problem hiding this comment.
When NO_PROXY/no_proxy bypasses the target, especially NO_PROXY=* or NO_PROXY=docs.openclaw.ai, this still treats the proxy as usable because hasEnvHttpProxyConfigured ignores bypass rules. The runtime path in src/infra/net/fetch-guard.ts only switches to the env proxy when shouldUseEnvHttpProxyForUrl(...) is true, and that helper returns false on NO_PROXY matches, so doctor can recommend tools.web.fetch.useTrustedEnvProxy even though enabling it will not change web_fetch routing for those targets.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 4:06 AM ET / 08:06 UTC. Summary PR surface: Source +130, Tests +152, Docs +1. Total +283 across 5 files. Reproducibility: no. high-confidence runtime reproduction was executed, but the source path is concrete: Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the read-only doctor diagnostic, but gate its remediation through the same protocol-aware proxy and Do we have a high-confidence way to reproduce the issue? No high-confidence runtime reproduction was executed, but the source path is concrete: Is this the best way to solve the issue? No in its current form; doctor is the correct owner and a read-only diagnostic is the narrowest safe approach, but it must reuse the runtime's effective proxy-and-bypass decision rather than infer applicability from proxy-variable presence. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6ac15970a742. Label changesLabel justifications:
Evidence reviewedPR surface: Source +130, Tests +152, Docs +1. Total +283 across 5 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
|
Merged via squash.
|
* fix(doctor): diagnose direct web fetch proxy routing * test(doctor): cover proxy diagnostic through public API * test(doctor): validate emitted proxy diagnostic type
1 participant
Closes #95560
AI-assisted (approved by Peter)
What Problem This Solves
Fixes an issue where operators with HTTP(S) proxy environment variables could see
web_fetchtime out without doctor explaining that the tool still uses direct connections unless trusted proxy routing is explicitly enabled.Why This Change Was Made
Doctor now detects effective HTTP(S) proxy variables in its process or the installed Gateway service, reports only variable names, probes direct TLS connectivity, and names the existing
tools.web.fetch.useTrustedEnvProxyopt-in. It remains read-only: no startup warning and no automatic proxy trust.User Impact
Operators get a concrete diagnosis and remediation command while the secure explicit-opt-in default remains unchanged. Thanks @Footree for the report and detailed reproduction.
Evidence
node scripts/run-vitest.mjs src/commands/doctor-web-fetch-proxy.test.ts— 8/8 passed.HTTPS_PROXY, reported direct routing, and confirmed direct TLS connectivity without printing the proxy value.tbx_01kxqfn2qafqtmbn7v8q2128tr: https://github.com/openclaw/openclaw/actions/runs/29563219328tbx_01kxqg1w6qesxmhe63mxjczh9n: https://github.com/openclaw/openclaw/actions/runs/29563596042