fix(web): probe loopback endpoints directly, never via env proxy - #63656
Solitud1nem wants to merge 2 commits into
Conversation
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused reproduction and narrow fix. The premise remains valid on current main: hermes_cli/web_server.py:6372-6374 creates the local-endpoint httpx.Client with default environment-proxy behavior and returns an empty model list with no diagnostic; apps/desktop/src/store/onboarding.ts:825-834 then displays the misleading no-models copy.
Problems
- The new E2E test clears only
NO_PROXY(tests/hermes_cli/test_web_server.py:7260on this branch). It should also clear lowercaseno_proxy; existing proxy tests consistently clear both forms, e.g.tests/agent/test_auxiliary_client_proxy_env.py:27-30. Otherwise an inherited lowercase bypass can allow the unfixed client to pass the test. - The Desktop message preference added in
apps/desktop/src/store/onboarding.ts:827-840has no direct Vitest coverage. The existing no-model test atapps/desktop/src/store/onboarding.test.ts:400-420covers only an empty probe message.
Suggested changes
- Clear both proxy-bypass variable spellings in the real-server regression test.
- Add a Desktop test asserting a reachable no-model probe with an HTTP-status message displays that detail.
Automated hermes-sweeper review.
teknium1
added
sweeper:risk-compatibility
|
Also pushed the Desktop coverage you asked for, in Unrelated heads-up for anyone running the desktop suite on this branch: |
3d36cc5 to
1757288
Compare
SummaryThree PRs address two distinct issues: #63656 fixes loopback-proxy interception of Desktop model discovery for #63472, #68328 preserves the selected model during same-provider reauthentication for #68144, and #58589 fixes a separate first-provider fallback and Codex persistence path. Their visible diffs target different causes rather than competing implementations. Related pull requests
Suggested consolidationKeep #63656 open with a salvage path centered on its loopback trust_env bypass, real-server proxy regression, and Desktop error-status coverage; this preserves the recorded best-fix verdict and the contributor keep_open review. Keep #68328 open with its same-provider preservation guard and test while addressing or splitting out the unpinned-cron resolver path, and keep #58589 open for its distinct Codex/API-key onboarding fix; none should be closed as a duplicate. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I63472(["issue #63472 (open)"])
P63656["PR #63656 (open)"]
P63656 -->|best fix| I63472
class I63472 open
class P63656 open
class P63656 best
class P63656 target
click I63472 "https://github.com/NousResearch/hermes-agent/issues/63472"
click P63656 "https://github.com/NousResearch/hermes-agent/pull/63656"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 29 kB of PR diffs, 17 kB of issue/PR text, 9 kB of discussion (8 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
The local/custom endpoint probe (/api/providers/validate, OPENAI_BASE_URL branch) used httpx with default trust_env, so an HTTP(S)_PROXY inherited from the environment hijacked even 127.0.0.1 probes. A proxy that answers with an error page turns into models=[], which the desktop onboarding reports as 'advertised no models' although the endpoint serves a model and the CLI (urllib honors Windows <local> proxy bypass) sees it fine. - bypass env proxies whenever the probed host is loopback - surface the HTTP status when a reachable endpoint answers non-2xx so the GUI stops suggesting to 'start a model' on a 502 - desktop: prefer the probe's error message over the generic no-models copy Fixes NousResearch#63472
… Desktop message
Review follow-up on both counts.
The end-to-end regression cleared only `NO_PROXY`, so an inherited
lowercase `no_proxy` covering loopback — the spelling half of us
actually have exported, and `localhost,127.0.0.1` is the usual value —
would have handed the unfixed `trust_env=True` client a direct route to
the server and let the test pass while guarding nothing. It now clears
all eight spellings before arming the dead proxy, the same list
tests/agent/test_auxiliary_client_proxy_env.py already uses.
Worth noting the hole is invisible on Windows: os.environ folds keys to
upper case there, so `no_proxy` and `NO_PROXY` are one variable and the
old delenv happened to cover it. On Linux they are two, `getproxies()`
still reports `{'no': ...}` after the old delenv, and httpx turns a
loopback-covering value into an `all://127.0.0.1 -> None` mount — a
direct connection. That is CI's platform, which is exactly where the
test was weakest. (A non-loopback value such as `no_proxy=.corp.example`
still routes the probe into the dead proxy, so the fix is plain test
hygiene either way; the hole just happens to open on the value most of
us export.)
The Desktop side now has the Vitest the review asked for: a reachable
probe that answers HTTP 502 must surface the status, not flatten into
"advertised no models". The mock mirrors the exact string the backend
builds (`{base}/models answered HTTP {code}.`), and it's verified with
teeth — dropping the probeMessage branch fails it.
1757288 to
e26fa90
Compare
|
Rebased onto current main. CI is green: 48 check-runs, no failures, mergeable_state=clean, including the apps/desktop checks. The conflict was real, not textual. This branch predates the move of /api/providers/validate from httpx.Client to httpx.AsyncClient and the _BlockingClient guard in the fixture. The fix now sits on the AsyncClient — trust_env=not _probe_ignores_env_proxy(url), awaited call — and the three tests use async stubs. Behaviour unchanged. For test_web_server.py I reset the file to main's version and appended my tests at the end of the class, so test_local_endpoint_without_key_sends_no_auth_header and test_named_custom_endpoint_probe_is_async stay exactly as they are on main. Premise still holds: trust_env appears nowhere in web_server.py on main, so loopback probes still go through the inherited proxy and its error page still reads as "no models". Mutations: trust_env=True fails four tests, including the one that runs a real localhost server behind a dead proxy; dropping the status-message branch fails test_local_endpoint_error_status_surfaces_in_message; dropping probeMessage in onboarding.ts fails its vitest. |
…es the status (#63472) httpx honours the env/system proxy (on Windows, the registry ProxyServer even with no *_PROXY vars) but never the bypass list, so a system proxy (Clash, corporate) answered 127.0.0.1 probes from both Desktop validators with its own error page. That parsed as models=[] and the GUI said "advertised no models at /v1/models" for a llama.cpp server the CLI (urllib, honours <local>) saw fine. Local endpoints (loopback, LAN, Tailscale via is_local_endpoint) now probe with trust_env=False; public endpoints keep honouring env proxies. A reachable endpoint answering non-2xx with no model list reports "<url> answered HTTP <status>." instead of an empty catalog, so the onboarding card stops telling the user to start a model. Reimplemented on the decomposed router (the original patched web_server.py before the split). Diagnosis and fix direction by Solitud1nem in #63656; Windows registry-proxy confirmation by Ulysses-Gaia on #63472. Live repro (real loopback server, HTTP_PROXY=http://127.0.0.1:9): before ok=False reachable=False 'Could not reach .../v1/models' after ok=True models=['Qwen3.6-35B-A3B-Q5_K_M.gguf'] Co-authored-by: Solitud1nem <76743883+Solitud1nem@users.noreply.github.com>
|
Landed on Thanks for the diagnosis, the Windows repro, and the rebase work; closing as superseded by the merged commit. |
4 participants
What does this PR do?
POST /api/providers/validate(the local/custom endpoint probe used byDesktop onboarding) creates its
httpx.Clientwith defaulttrust_env,so an
HTTP(S)_PROXYinherited from the environment hijacks even127.0.0.1probes. A proxy that answers with its own error page (Clash,corporate proxies, …) parses as
models=[], and the GUI reports"Connected …, but it advertised no models at /v1/models" — although the
endpoint serves a model and the CLI sees it fine (the CLI probe goes
through
urllib, which on Windows honors the<local>proxy-bypass;httpx only reads
*_PROXYenv vars and has no such bypass).The mismatch is sharpest on Windows Desktop: the GUI-spawned backend
inherits the login environment block (where user-scoped proxy vars live),
while an interactive CLI shell may not — same machine, CLI works, Desktop
fails, exactly as reported in #63472.
Changes:
127.0.0.0/8,::1,localhost) are probed withtrust_env=False— a loopback endpoint is never meaningfully reachablethrough an HTTP proxy, so this cannot break a legitimate setup;
non-loopback custom endpoints keep honoring env proxies (corporate
setups stay intact)
"… answered HTTP <status>."instead of silently returning no models,and Desktop onboarding prefers that message over the misleading
"start a model on that endpoint" copy
Related Issue
Fixes #63472
Type of Change
Changes Made
hermes_cli/web_server.py— new_probe_ignores_env_proxy()helper(loopback detection via
ipaddress); theOPENAI_BASE_URLprobe branchpasses
trust_env=not _probe_ignores_env_proxy(url)and reports the HTTPstatus when a reachable endpoint answers non-2xx with no parseable models
apps/desktop/src/store/onboarding.ts—saveOnboardingLocalEndpointprefers the probe's error message over the generic no-models copy
tests/hermes_cli/test_web_server.py— 6 new tests: parametrizedloopback-vs-public
trust_envmatrix, an end-to-end repro (reallocalhost HTTP server +
HTTP(S)_PROXYpointing at a dead port; failson
main, passes with the fix), and the non-2xx message contractHow to Test
llama-server,vLLM, Ollama) on
http://127.0.0.1:8080/v1.HTTP_PROXY=http://127.0.0.1:9andHTTPS_PROXY=http://127.0.0.1:9(any dead or error-answering proxy), leave
NO_PROXYunset.http://127.0.0.1:8080/v1→ Connect.Before: "…advertised no models at /v1/models". After: the model is
auto-detected (or, if the endpoint itself errors, the real HTTP status
is shown).
pytest tests/hermes_cli/test_web_server.py -q— the newtest_loopback_probe_ignores_proxy_env_end_to_endfails onmain,passes here.
Checklist
Code
fix(scope):,feat(scope):, etc.)tests/hermes_cli/test_web_server.py— 361 passed, 19 skippedDocumentation & Housekeeping
cli-config.yaml.example— N/A (no config keys)CONTRIBUTING.mdorAGENTS.md— N/Ascripts/check-windows-footguns.py --diff mainis cleanScreenshots / Logs
Deterministic repro on Windows 11 (fake llama.cpp serving the reporter's
exact JSON on
127.0.0.1:18080, fake proxy answering 502 on:18081),running the exact probe code path: