From 1c93799b4917ba058fc46efcefb090a7f896bec3 Mon Sep 17 00:00:00 2001 From: kshitijk4poor Date: Fri, 3 Jul 2026 03:36:22 +0530 Subject: [PATCH] fix(agent): self-review follow-ups on vLLM local-context salvage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review (ruff+ty lint diff = 0 net-new; 2-agent deep review) surfaced one Warning + comment-accuracy nits; no Critical: - W1: the local-probe TTL cache memoized None (probe failure) for 30s, so a probe that failed during a startup race would suppress a legit retry once the server came up. Cache only positive results — still fully bounds the hot-path probe rate (reachable servers cache their value) while an unreachable one re-probes on the next call. Add a regression test asserting a None result is NOT cached (retry re-probes); mutation-verified. - Tighten the platform-guard comment: gateway/TUI/cron already construct with quiet_mode=True (gated by `not agent.quiet_mode`), so the guard's active job is CLI dedup vs show_banner, not "filling the gateway/TUI gap" as originally worded. Verified not-issues (per review): positive-value 30s cache does not break the reconcile-after-restart freshness contract (restart = fresh process, empty cache); cache key is collision-safe; platform guard is correct in both directions (no runtime path leaves platform None on a non-CLI surface). Tests: 149 passed. ruff clean; ty 0 net-new vs base. --- agent/agent_init.py | 11 +++++---- agent/model_metadata.py | 9 +++++++- tests/agent/test_model_metadata_local_ctx.py | 24 ++++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/agent/agent_init.py b/agent/agent_init.py index 29041308aa3..41ed85e998d 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1721,10 +1721,13 @@ def init_agent( f"(this must be at least {MINIMUM_CONTEXT_LENGTH // 1000}K)." ) - # Nous Hermes 3/4 are chat models, not tool-call-tuned — surface the - # warning on gateway/TUI. The interactive CLI already warns via - # cli.py show_banner() (richer output + /model switch hint), so skip the - # CLI platform here to avoid emitting the warning twice per startup. + # Nous Hermes 3/4 are chat models, not tool-call-tuned. The interactive + # CLI already warns via cli.py show_banner() (richer output + /model hint), + # so skip platform=="cli" here to avoid emitting the warning twice per + # startup. (Gateway/TUI/cron construct with quiet_mode=True and are already + # gated off by the `not agent.quiet_mode` check above; this guard's active + # job is the CLI dedup, and it leaves the door open for any non-quiet + # non-CLI surface to still surface the warning.) if not agent.quiet_mode and (agent.platform or "cli") != "cli": try: from hermes_cli.model_switch import _check_hermes_model_warning diff --git a/agent/model_metadata.py b/agent/model_metadata.py index 28d8e1d9cf6..726c3300a90 100644 --- a/agent/model_metadata.py +++ b/agent/model_metadata.py @@ -1546,7 +1546,14 @@ def _query_local_context_length(model: str, base_url: str, api_key: str = "") -> return cached[0] result = _query_local_context_length_uncached(model, base_url, api_key=api_key) - _LOCAL_CTX_PROBE_CACHE[cache_key] = (result, now) + # Cache only positive results. A None/failure (server not up yet, + # connection refused, timeout) must NOT be memoized — otherwise a probe + # that fails during a startup race would suppress a legit retry seconds + # later once the server is reachable. Positive-only caching still fully + # bounds the hot-path probe rate (a reachable server returns a value and + # gets cached); an unreachable one simply re-probes on the next call. + if result: + _LOCAL_CTX_PROBE_CACHE[cache_key] = (result, now) return result diff --git a/tests/agent/test_model_metadata_local_ctx.py b/tests/agent/test_model_metadata_local_ctx.py index f2250f204d4..2c069aaf6a4 100644 --- a/tests/agent/test_model_metadata_local_ctx.py +++ b/tests/agent/test_model_metadata_local_ctx.py @@ -802,3 +802,27 @@ class TestLocalContextProbeTTLCache: _query_local_context_length("m2", "http://localhost:11434/v1") assert detect.call_count == 2 + + + def test_none_result_not_cached(self): + """A failed probe (None) must NOT be memoized — a retry within the TTL + window must re-probe so a server that comes up mid-startup is caught.""" + from agent.model_metadata import _query_local_context_length + + # First probe: server unreachable -> detect returns None, all queries miss -> None. + fail_resp = self._make_resp(404, {}) + client_mock = MagicMock() + client_mock.__enter__ = lambda s: client_mock + client_mock.__exit__ = MagicMock(return_value=False) + client_mock.post.return_value = fail_resp + client_mock.get.return_value = fail_resp + + with patch("agent.model_metadata.detect_local_server_type", return_value=None) as detect, \ + patch("httpx.Client", return_value=client_mock): + first = _query_local_context_length("m", "http://localhost:11434/v1") + # Retry within TTL must re-probe (None was not cached). + second = _query_local_context_length("m", "http://localhost:11434/v1") + + assert first is None + assert second is None + assert detect.call_count == 2, "None result was wrongly cached; retry did not re-probe"