From 0ef23c9c5e4e4508073560d35defad7878dbaa71 Mon Sep 17 00:00:00 2001 From: webtecnica <75556242+webtecnica@users.noreply.github.com> Date: Sat, 18 Jul 2026 20:55:43 -0300 Subject: [PATCH] fix(models): prevent bare-id picker revert when provider hint is empty (#6195) (#6199) Prevent bare-id model picker revert when the provider hint is empty: an ambiguous bare id that collides across provider groups no longer snaps to the default group on re-render. Adds a revert-sensitive regression test and fixes three cross-file test-isolation leaks found while gating. Thanks @webtecnica. Co-authored-by: webtecnica --- static/ui.js | 14 ++- tests/test_agent_runtime_revision_guard.py | 17 +++ tests/test_issue2720_bedrock_model_picker.py | 7 ++ ...t_issue6195_bare_id_ambiguous_no_revert.py | 109 ++++++++++++++++++ tests/test_provider_sort_order.py | 20 +++- 5 files changed, 164 insertions(+), 3 deletions(-) create mode 100644 tests/test_issue6195_bare_id_ambiguous_no_revert.py diff --git a/static/ui.js b/static/ui.js index 00952e4a..5589f564 100644 --- a/static/ui.js +++ b/static/ui.js @@ -3197,9 +3197,19 @@ function _findModelInDropdown(modelId, sel, preferredProviderId){ const providerMatch=options.find(o=>norm(o.value)===target && _getOptionProviderId(o).toLowerCase()===preferred); if(providerMatch) return providerMatch.value; } - // 2. Normalized match + // 2. Normalized match — but ONLY when unambiguous. If the bare id + // matches across multiple provider groups AND no provider hint is + // available, return null instead of snapping to the first group's + // option. This prevents a deliberate non-default pick from reverting + // to the default provider on re-render (#6195). const exact=opts.find(o=>norm(o)===target); - if(exact) return exact; + if(exact){ + const normMatches=options.filter(o=>norm(o.value)===target); + if(normMatches.length>1 && !preferred && !explicitProvider && !rawModel.includes('/')){ + return null; // ambiguous bare id — caller must inject the correct option + } + return exact; + } // If the request is provider-qualified (either explicit @provider:model or // a slash-qualified vendor/model id), do NOT fuzzy-match a sibling model // once exact/provider-aware lookup failed. Returning null lets the caller diff --git a/tests/test_agent_runtime_revision_guard.py b/tests/test_agent_runtime_revision_guard.py index 9712f711..0cc1d5e7 100644 --- a/tests/test_agent_runtime_revision_guard.py +++ b/tests/test_agent_runtime_revision_guard.py @@ -39,6 +39,23 @@ def test_loaded_agent_runtime_fails_closed_after_source_revision_changes(tmp_pat probe = tmp_path / "probe.py" probe.write_text( """ +import sys + +# On a machine where hermes_agent is installed as an editable package, setuptools +# registers a meta-path finder (__editable___hermes_agent_*_finder) that maps +# `run_agent` -> the real on-disk agent module. That finder runs BEFORE the +# PYTHONPATH-based PathFinder, so it would shadow the synthetic per-test agent +# dir this test points HERMES_WEBUI_AGENT_DIR at (the real AIAgent has no +# `.revision` attribute). Drop any such editable finder + purge cached agent +# modules so `import run_agent` resolves from the test's agent_dir on PYTHONPATH. +# On CI (no editable install) this is a harmless no-op. +sys.meta_path[:] = [ + _f for _f in sys.meta_path + if "__editable__" not in type(_f).__module__ and "__editable__" not in getattr(_f, "__module__", "") +] +for _m in ("run_agent", "hermes_state", "agent", "tools"): + sys.modules.pop(_m, None) + from pathlib import Path import subprocess diff --git a/tests/test_issue2720_bedrock_model_picker.py b/tests/test_issue2720_bedrock_model_picker.py index dc68ab65..1d93ea9c 100644 --- a/tests/test_issue2720_bedrock_model_picker.py +++ b/tests/test_issue2720_bedrock_model_picker.py @@ -22,6 +22,7 @@ def _force_env_fallback(monkeypatch): def _run_available_models_with_cfg(monkeypatch, tmp_path, cfg): old_cfg = dict(config.cfg) old_mtime = config._cfg_mtime + old_cfg_path = config._cfg_path monkeypatch.setattr(config, "_models_cache_path", tmp_path / "models_cache.json") monkeypatch.setattr(config, "_get_config_path", lambda: tmp_path / "missing-config.yaml") monkeypatch.setattr("api.profiles.get_active_hermes_home", lambda: tmp_path, raising=False) @@ -35,6 +36,12 @@ def _run_available_models_with_cfg(monkeypatch, tmp_path, cfg): config.cfg.clear() config.cfg.update(old_cfg) config._cfg_mtime = old_mtime + # get_available_models() stamps config._cfg_path to the monkeypatched + # tmp path as a side effect of its staleness check; restore it so a + # later test's get_available_models() doesn't see path_changed=True and + # reload config from disk over that test's in-memory cfg (isolation leak + # that flaked test_issue3691 / test_issue1426 under cross-file ordering). + config._cfg_path = old_cfg_path config.invalidate_models_cache() diff --git a/tests/test_issue6195_bare_id_ambiguous_no_revert.py b/tests/test_issue6195_bare_id_ambiguous_no_revert.py new file mode 100644 index 00000000..17d97a8a --- /dev/null +++ b/tests/test_issue6195_bare_id_ambiguous_no_revert.py @@ -0,0 +1,109 @@ +"""Regression for #6195 / #6199: a bare model id that collides with the profile +default across multiple provider groups must NOT silently snap to the default +group's option when no provider hint is available. + +`_findModelInDropdown('glm-5.2', sel, '')` previously returned the FIRST +normalized match (`z-ai/glm-5.2`, the default group), so a deliberate non-default +pick reverted to the default provider on re-render when the provider hint was +momentarily unavailable. The Stage-2 guard now returns null in that ambiguous +case so the caller injects a provider-scoped option instead of asserting the +wrong provider. + +Invariants this locks in (proven against origin/master vs the fix): + - ambiguous bare id + EMPTY hint -> null (was: default-group snap) [FIXED] + - ambiguous bare id + correct hint -> the hinted provider option [unchanged] + - uniquely-named bare id -> its single match [unchanged] + - bare id that IS an exact option value -> returned at Stage 0 [unchanged] +""" + +from __future__ import annotations + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).parent.parent.resolve() +UI_JS = REPO_ROOT / "static" / "ui.js" +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + +_DRIVER = r""" +const fs = require('fs'); +const uiSrc = fs.readFileSync(process.argv[1], 'utf8'); + +function extractFunction(source, name){ + const marker='function '+name+'('; + const start=source.indexOf(marker); + if(start<0) throw new Error('not found: '+name); + const brace=source.indexOf('{', source.indexOf(')', start)); + let d=0; + for(let i=brace;i extractFunction(uiSrc, name)).join('\n')); + +function makeOpt(value, provider){ + return { value, textContent:value, dataset:{}, parentElement:{tagName:'OPTGROUP', dataset:{provider}} }; +} +function makeSelect(options){ + let idx = 0; + return { + id:'modelSelect', options, + querySelectorAll(){ return []; }, + get selectedOptions(){ return idx>=0?[options[idx]]:[]; }, + get value(){ return idx>=0?options[idx].value:''; }, + set value(v){ idx = options.findIndex(o=>o.value===v); }, + }; +} + +const AMB = () => [ makeOpt('z-ai/glm-5.2','z-ai'), makeOpt('@custom:tokenrouter:z-ai/glm-5.2','custom:tokenrouter') ]; + +const out = { + // #6195 repro: ambiguous bare id, no hint -> must be null (not a default-group snap) + ambiguousEmptyHint: _findModelInDropdown('glm-5.2', makeSelect(AMB()), ''), + // with the correct hint -> resolves to the hinted provider (unchanged) + ambiguousWithHint: _findModelInDropdown('glm-5.2', makeSelect(AMB()), 'custom:tokenrouter'), + // uniquely-named bare id still matches (unchanged) + uniqueBare: _findModelInDropdown('glm-5.2-free', makeSelect([makeOpt('z-ai/glm-5.2-free','z-ai')]), ''), + // bare id that IS an exact option value -> returned at Stage 0 even when ambiguous (unchanged) + bareExactAmbiguous: _findModelInDropdown('glm-5.2', makeSelect([makeOpt('glm-5.2','z-ai'), makeOpt('@custom:tokenrouter:glm-5.2','custom:tokenrouter')]), ''), +}; +process.stdout.write(JSON.stringify(out)); +""" + + +def _run(): + assert NODE is not None + result = subprocess.run([NODE, "-e", _DRIVER, str(UI_JS)], + capture_output=True, text=True, timeout=30) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout) + + +def test_ambiguous_bare_id_no_hint_does_not_snap_to_default(): + # THE fix: no wrong-provider revert. null => caller injects a scoped option. + assert _run()["ambiguousEmptyHint"] is None + + +def test_ambiguous_bare_id_with_hint_resolves_to_hinted_provider(): + assert _run()["ambiguousWithHint"] == "@custom:tokenrouter:z-ai/glm-5.2" + + +def test_unique_bare_id_still_matches(): + assert _run()["uniqueBare"] == "z-ai/glm-5.2-free" + + +def test_bare_id_that_is_exact_option_value_returns_at_stage0(): + assert _run()["bareExactAmbiguous"] == "glm-5.2" diff --git a/tests/test_provider_sort_order.py b/tests/test_provider_sort_order.py index 8dd1658a..1745f083 100644 --- a/tests/test_provider_sort_order.py +++ b/tests/test_provider_sort_order.py @@ -48,7 +48,19 @@ def _install_fake_hermes_cli(monkeypatch): @pytest.fixture(autouse=True) def _isolate_cache(): - """Invalidate the TTL model cache around each test.""" + """Invalidate the TTL model cache around each test AND restore config.cfg. + + ``_setup_config`` calls ``config.reload_config()`` against a temp config.yaml, + which repopulates the shared ``config.cfg`` with this file's provider/model + config and never resets it. Under cross-file ordering a sibling test (e.g. + test_model_resolver) then inherits a leaked ``providers`` block that injects + an unexpected provider group and flakes its assertions. Snapshot + restore + ``config.cfg`` (a shallow dict copy — cheap) so the leak can't escape this + file. We deliberately do NOT touch ``_cfg_cache`` / ``_yaml_file_cache`` + (restoring those globals destabilizes unrelated state-db reconciliation + tests in the same shard). + """ + _saved_cfg = dict(config.cfg) try: config.invalidate_models_cache() except Exception: @@ -58,6 +70,12 @@ def _isolate_cache(): config.invalidate_models_cache() except Exception: pass + try: + if isinstance(config.cfg, dict): + config.cfg.clear() + config.cfg.update(_saved_cfg) + except Exception: + pass def _model_list(*ids):