fix(agent): self-review follow-ups on vLLM local-context salvage

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.
This commit is contained in:
kshitijk4poor 2026-07-03 03:36:22 +05:30
parent eb806c7f50
commit 1c93799b49
3 changed files with 39 additions and 5 deletions

View file

@ -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

View file

@ -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

View file

@ -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"