Skip to content

fix(config): don't warn when SEARCH_RERANK_URL points at the gateway - #1358

Open
cbcoutinho wants to merge 2 commits into
masterfrom
fix/rerank-warning-false-positive
Open

fix(config): don't warn when SEARCH_RERANK_URL points at the gateway#1358
cbcoutinho wants to merge 2 commits into
masterfrom
fix/rerank-warning-false-positive

Conversation

@cbcoutinho

@cbcoutinho cbcoutinho commented Aug 19, 2026

Copy link
Copy Markdown
Owner

The mis-namespaced-model warning added in #1356 assumed "explicit SEARCH_RERANK_URL" implies "not the gateway". Pinning the URL to the gateway's own /v1/rerank is a legitimate configuration — it is how you fix the endpoint while still using the gateway — and the local/ routing prefix is correct there.

So the warning fired on a working setup, which is how operators learn to ignore warnings.

How it was found

Running the gateway-free rerank path end to end against a real login-flow stack. The server logged:

SEARCH_RERANK_MODEL='local/BAAI/bge-reranker-v2-m3' carries the embedding gateway's
'local/' routing prefix, but SEARCH_RERANK_URL points at a direct endpoint which has
no such routing. Use the bare model id (...) or reranking will silently degrade to
retrieval order.

…while reranking was working correctly, because SEARCH_RERANK_URL was the gateway's own /v1/rerank.

The fix

Warn only when the URL is somewhere other than the configured gateway. Verified silent on the live deployment afterwards, with rerank_available: true and the endpoint still resolving to the explicit URL.

Test coverage

Added test_rerank_url_pointing_at_the_gateway_is_quiet alongside the existing quiet-case test, so both sides of the boundary are pinned: a genuinely direct endpoint (http://infinity:7997/rerank + local/…) still warns; a gateway-pinned URL does not.

No API surface change, so no e2e/contract tier applies. 108 tests in tests/unit/test_config.py pass; ruff, ruff format, ty clean.


This PR was generated with the help of AI, and reviewed by a Human

cbcoutinho and others added 2 commits August 19, 2026 17:12
The mis-namespaced-model warning added in #1356 assumed "explicit
SEARCH_RERANK_URL" implies "not the gateway". Pinning the URL to the
gateway's own /v1/rerank is a legitimate configuration — it is how you
fix the endpoint while still using the gateway — and the `local/` routing
prefix is correct there. The warning fired on that working setup, which
is how operators learn to ignore warnings.

Found by running the gateway-free rerank path end to end against a real
login-flow stack.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Claude finished @cbcoutinho's task in 2m 1s —— View job


Claude's Review

  • Gather context (read diff, changed files, CLAUDE.md)
  • Understand the request (code review)
  • Review nextcloud_mcp_server/config.py changes
  • Review tests/unit/test_config.py changes
  • Post review feedback

Summary

This PR narrows the "gateway-namespaced model on a direct rerank endpoint" warning (added in #1356) so it stays quiet when SEARCH_RERANK_URL is explicitly pinned to the gateway's own /v1/rerank — a legitimate configuration. The fix and accompanying test are well-targeted and the doc comment clearly explains the why.

One correctness issue in the new logic, plus a test-coverage gap:


🟡 str.startswith is not a safe way to check "is this URL the gateway"

nextcloud_mcp_server/config.py:1741-1746:

_direct_rerank = bool(self.search_rerank_url) and not (
    self.embedding_gateway_url
    and self.search_rerank_url.startswith(
        self.embedding_gateway_url.rstrip("/")
    )
)

startswith does a raw character-prefix match, not a host/authority comparison, so a lookalike or subdomain host is indistinguishable from a genuine sub-path of the gateway. E.g. with EMBEDDING_GATEWAY_URL=https://gw.example:

  • SEARCH_RERANK_URL=https://gw.example.attacker.com/v1/rerankstartswith("https://gw.example") is True, so this is (incorrectly) treated as "pointing at the gateway" and the warning is suppressed even though it's a completely different host.
  • Less severe, but also wrong the other way: EMBEDDING_GATEWAY_URL=https://GW.example vs SEARCH_RERANK_URL=https://gw.example/v1/rerank (case differs) would not match even though hostnames are case-insensitive, so the warning would fire on a working setup — the exact false positive this PR is trying to eliminate.

A more robust check parses both URLs and compares scheme + netloc (optionally also verifying the path is a proper sub-path), e.g.:

from urllib.parse import urlsplit

def _same_origin(url: str, base: str) -> bool:
    u, b = urlsplit(url), urlsplit(base)
    return (u.scheme, u.netloc.lower()) == (b.scheme, b.netloc.lower())

Given the module already imports comment-heavy validation logic like this, this is a small, self-contained fix.

Fix this →

🟢 Test coverage gap

The new test (test_rerank_url_pointing_at_the_gateway_is_quiet) only covers the "quiet when pointed at the gateway" case. There's no test for the case this feature must still catch: EMBEDDING_GATEWAY_URL and a different, non-gateway SEARCH_RERANK_URL both set with a gateway-namespaced model — i.e. confirming the warning still fires when both env vars are present but genuinely point at different hosts. Right now that combination is only implicitly exercised by test_direct_url_warns_about_the_gateway_namespaced_default_model, which doesn't set EMBEDDING_GATEWAY_URL at all, so it wouldn't catch a regression in the and/startswith branch specifically.

Other notes

  • Logic and control flow are otherwise sound: bool(self.search_rerank_url) and not (...) short-circuits correctly, so there's no risk of calling .startswith() on None.
  • Comment style matches the file's existing convention of explaining why non-obvious validation decisions were made — consistent with CLAUDE.md.
  • The third_party/astrolabe submodule bump appears incidental to this stacked branch and unrelated to the fix itself.
    · Branch

@cbcoutinho cbcoutinho changed the title fix/rerank warning false positive fix(config): don't warn when SEARCH_RERANK_URL points at the gateway Aug 19, 2026
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant