Skip to content

Commit fff8418

Browse files
committed
fix(ai): test the on-screen Ollama address, not only the saved one
The model dropdown probes the address typed into the Settings form, so it populated for a hosted Ollama. "Test connection" beside it took no body and read the org's saved address instead — so a host entered but not yet saved was tested against the default (localhost) and reported as unreachable, while the dropdown right above it listed that same host's models. Let the test carry the on-screen host/model/thinking and check those, falling back to the stored value per field when absent. The credential is deliberately not overridable here — it comes from storage, so a test can't be turned into a way to send a saved secret to a just-typed address. The address is validated in the handler, so a bad one is a clear 400 rather than a silent probe of the default. Signed-off-by: Arun Rajkumar <mickyarunr@gmail.com>
1 parent 5972bea commit fff8418

4 files changed

Lines changed: 171 additions & 5 deletions

File tree

backend/app/api/v1/settings_claude.py

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,14 +253,46 @@ async def update_claude_settings(
253253
return _read_model(org)
254254

255255

256+
class ClaudeConnectionTest(BaseModel):
257+
"""On-screen host settings to test, so "Test connection" checks what the
258+
user is looking at rather than only what was last saved.
259+
260+
All optional: an omitted field falls back to the stored value, and an empty
261+
body reproduces the original "test what's saved" behaviour. Carries no
262+
credential — the token comes from storage, so a test can't be used to send a
263+
secret to a just-typed address.
264+
"""
265+
266+
base_url: str | None = Field(None, description="Server address, for HTTP-based providers.")
267+
model: str | None = None
268+
thinking: bool | None = None
269+
270+
256271
@router.post("/claude/test")
257272
async def test_claude_settings(
273+
body: ClaudeConnectionTest | None = None,
258274
current_user: User = Depends(get_current_user),
259275
db: AsyncSession = Depends(get_db),
260276
) -> dict[str, Any]:
261-
"""Run the org's provider version check + a trivial prompt against its auth."""
277+
"""Run the org's provider version check + a trivial prompt against its auth.
278+
279+
Tests the on-screen host settings when the client sends them, so the Test
280+
button checks the same address the model dropdown just probed instead of the
281+
last-saved one — a host entered but not yet saved was otherwise tested
282+
against localhost and reported broken.
283+
"""
262284
# Ensure the most recent stored credential (if any) is in process env
263285
# first, in case the backend was restarted since the last PATCH.
264286
org = await OrganizationRepository(db).get_for_user(current_user)
265287
apply_claude_auth_to_env(org)
266-
return await check_provider_connection(org)
288+
if body is None:
289+
return await check_provider_connection(org)
290+
# Validate the address here so a bad one is a clear 400, not a silent probe
291+
# of the default. _clean_base_url returns None for an empty string, which the
292+
# connection check reads as "use the stored value".
293+
return await check_provider_connection(
294+
org,
295+
base_url=_clean_base_url(body.base_url) if body.base_url is not None else None,
296+
model=body.model,
297+
thinking=body.thinking,
298+
)

backend/app/services/ai_runner/connection_check.py

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626

2727
from app.models.organization import AIProvider, Organization
2828
from app.services.ai_runner.capabilities import capabilities_for
29-
from app.services.ai_runner.capability_gate import adapt_config
29+
from app.services.ai_runner.capability_gate import adapt_config, org_api_key, provider_env
3030
from app.services.ai_runner.registry import provider_instance
3131
from app.services.ai_runner.subprocess_env import build_provider_env
3232
from app.services.claude_runner import NO_REPO_CONTEXT, ClaudeRunnerConfig
@@ -124,16 +124,45 @@ async def check_connection(
124124
return result
125125

126126

127-
async def check_provider_connection(org: Organization) -> dict[str, Any]:
127+
async def check_provider_connection(
128+
org: Organization,
129+
*,
130+
base_url: str | None = None,
131+
model: str | None = None,
132+
thinking: bool | None = None,
133+
) -> dict[str, Any]:
128134
"""Verify the org's provider is reachable and can authenticate.
129135
130136
The org's own host/model/thinking settings are resolved through the same
131137
seam a real run uses, so "Test connection" checks the configuration the org
132138
will actually run with. Without that, a provider pointed at a remote host
133139
would silently be probed on localhost — reporting a confident green for a
134140
host nobody tested, or an install hint for a config that was fine.
141+
142+
The keyword overrides let the Settings page test what the user is *looking
143+
at* rather than only what was last saved. The model dropdown already probes
144+
the typed address to populate itself; the Test button beside it read the
145+
saved value instead, so a host entered but not yet saved was tested against
146+
localhost and reported broken. Each override is applied only when supplied,
147+
falling back to the stored value — so an unrelated caller keeps today's
148+
behaviour. The credential is never overridden here: it comes from storage,
149+
because a test must not be a way to have the backend send a secret to an
150+
address the caller just typed.
135151
"""
136152
provider = org.ai_provider or AIProvider.claude
137153
caps = capabilities_for(provider)
154+
155+
overriding = base_url is not None or model is not None or thinking is not None
156+
if overriding and caps.requires_base_url:
157+
env = provider_env(
158+
caps,
159+
base_url=base_url if base_url is not None else org.ai_base_url,
160+
model=model if model is not None else org.ai_model,
161+
thinking=thinking if thinking is not None else bool(org.ai_thinking),
162+
api_key=org_api_key(caps, org),
163+
)
164+
timeout = int(_DEFAULT_PING_TIMEOUT_S * caps.timeout_multiplier)
165+
return await check_connection(provider, env, timeout)
166+
138167
probe = adapt_config(caps, org, ClaudeRunnerConfig(timeout_seconds=_DEFAULT_PING_TIMEOUT_S))
139168
return await check_connection(provider, probe.env_extra, probe.timeout_seconds)
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
# Copyright 2025-2026 Arun Rajkumar
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
""" "Test connection" must test what the user is looking at.
16+
17+
A hosted Ollama entered in the Settings form populates the model dropdown —
18+
because that probe reads the typed address — but "Test connection" read the
19+
saved address instead, so a host typed and not yet saved was tested against
20+
localhost and reported broken. These pin that the connection test honours the
21+
on-screen overrides, and still falls back to the stored value without them.
22+
"""
23+
24+
import uuid
25+
from types import SimpleNamespace
26+
from typing import Any
27+
28+
from app.models.organization import AIProvider
29+
from app.services.ai_runner import connection_check
30+
from app.services.ai_runner.ollama_models import OLLAMA_HOST_ENV
31+
32+
33+
def _ollama_org(base_url: str | None) -> Any:
34+
"""An Ollama org with no stored credential (no-auth / host mode)."""
35+
return SimpleNamespace(
36+
id=uuid.uuid4(),
37+
ai_provider=AIProvider.ollama,
38+
ai_base_url=base_url,
39+
ai_model="qwen3",
40+
ai_thinking=False,
41+
claude_api_key_encrypted=None,
42+
claude_auth_mode="host",
43+
)
44+
45+
46+
def _capture_env(monkeypatch: Any) -> dict[str, Any]:
47+
seen: dict[str, Any] = {}
48+
49+
async def _fake_check(provider: Any, env_extra: Any, timeout: Any) -> dict[str, Any]:
50+
seen["provider"] = provider
51+
seen["env"] = env_extra or {}
52+
return {"test_passed": True}
53+
54+
monkeypatch.setattr(connection_check, "check_connection", _fake_check)
55+
return seen
56+
57+
58+
async def test_a_typed_address_is_tested_not_the_empty_saved_one(monkeypatch: Any) -> None:
59+
"""The field's exact failure: nothing saved yet, so the old path probed
60+
localhost while the dropdown had already listed the typed host's models."""
61+
seen = _capture_env(monkeypatch)
62+
63+
await connection_check.check_provider_connection(
64+
_ollama_org(base_url=None),
65+
base_url="https://gw.example.com/ollama",
66+
model="qwen3",
67+
thinking=False,
68+
)
69+
70+
assert seen["env"][OLLAMA_HOST_ENV] == "https://gw.example.com/ollama"
71+
72+
73+
async def test_no_overrides_still_tests_the_saved_address(monkeypatch: Any) -> None:
74+
"""An empty body must reproduce the original behaviour — other callers, and
75+
the auto-test right after a save, rely on it."""
76+
seen = _capture_env(monkeypatch)
77+
78+
await connection_check.check_provider_connection(
79+
_ollama_org("https://saved.example.com/ollama")
80+
)
81+
82+
assert seen["env"][OLLAMA_HOST_ENV] == "https://saved.example.com/ollama"
83+
84+
85+
async def test_a_partial_override_falls_back_to_the_saved_address(monkeypatch: Any) -> None:
86+
"""Overriding only the model must not blank the host — each field falls back
87+
independently, so a thinking-toggle test still hits the saved server."""
88+
seen = _capture_env(monkeypatch)
89+
90+
await connection_check.check_provider_connection(
91+
_ollama_org("https://saved.example.com/ollama"), thinking=True
92+
)
93+
94+
assert seen["env"][OLLAMA_HOST_ENV] == "https://saved.example.com/ollama"

frontend/src/views/settings/SettingsClaudeCode.vue

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,18 @@ async function checkConnection(): Promise<void> {
543543
claudeVersion.value = ''
544544
showInstallHint.value = false
545545
try {
546-
const { data } = await api.post('/v1/settings/claude/test', null, { timeout: 120_000 })
546+
// Send the on-screen host settings so the test checks the same address the
547+
// model dropdown just probed, not the last-saved one — otherwise a host
548+
// typed but not yet saved tests against the default (localhost) and reports
549+
// a working server as broken. Non-host providers ignore these fields.
550+
const body = currentCaps.value?.requires_base_url
551+
? {
552+
base_url: baseUrl.value.trim() || undefined,
553+
model: model.value || undefined,
554+
thinking: thinking.value,
555+
}
556+
: null
557+
const { data } = await api.post('/v1/settings/claude/test', body, { timeout: 120_000 })
547558
applyTestResult(data)
548559
} catch (err) {
549560
claudeStatus.value = 'failed'

0 commit comments

Comments
 (0)