diff --git a/CHANGELOG.md b/CHANGELOG.md index 02ba2a97..588aec06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ ### Added +- **Full-session resolve concurrency is configurable.** `HERMES_WEBUI_MAX_SESSION_RESOLVE` sets how + many full-transcript session resolves may run at once (default 2, a positive integer up to 64; + zero, negative, non-numeric or out-of-range values fall back to 2). It is process-wide, so a + profile's `.env` cannot override it. (#7421, #7656 by @happy5318) - **The sidebar's recent-session window is configurable.** `HERMES_WEBUI_VISIBLE_SESSION_LIMIT` sets how many recent sessions the sidebar lists (default 20). It also bounds how many delegated subagent children can nest at once, so raise it for wide fan-outs. Invalid or non-positive values fall back to @@ -30,6 +34,16 @@ claim as a plain failure, so a few refusals in a row could terminally drop a completion whose session was alive and waiting. Transient refusals now return the claim as retryable, while hard failures still use up the attempt budget. (#7758 by @ruizanthony) +- **A first visit now uses the browser's language.** The server stores "no preference" as `null` + instead of defaulting to `"en"`, so a first-time visitor gets `navigator.language` while an + explicitly saved language (including English) still wins, and legacy `settings.json` files that + already hold `"en"` keep English. Reading the browser language is guarded, so an environment where + `navigator` throws falls back cleanly. (#7622, #7730 by @happy5318) +- **The live model list for a custom provider respects its `models:` allowlist, without treating + per-model metadata as one.** `/api/models/live` filters a custom provider's live catalog to its + configured `models:` when that is a list, or when it is a mapping with `discover_models: false`. + A mapping of per-model settings (the shape `hermes setup` writes) keeps the full live catalog, as + the Agent does. (#7165 by @happy5318) - **`MEDIA:` links wrapped in inline code no longer 404.** Every `MEDIA:` capture site (renderer, streaming parser, TTS stripper, session-media authorization and snapshot capture, seven in all) swallowed the closing backtick of `` `MEDIA:/path` `` into the path, so the file lookup and the diff --git a/api/config.py b/api/config.py index 5c38d375..27e10cf9 100644 --- a/api/config.py +++ b/api/config.py @@ -11483,7 +11483,13 @@ _SETTINGS_DEFAULTS = { "hidden_tabs": [], # sidebar tab panel names hidden by user (e.g. ["tasks","kanban"]); chat and settings are always visible "tab_order": [], # user-defined sidebar/rail tab order for reorderable tabs; chat/settings stay fixed "composer_control_order": [], # user-defined composer footer control order; invalid/duplicate keys are ignored - "language": "en", # UI locale code; must match a key in static/i18n.js LOCALES + # #7622 (round 3): language is intentionally absent from the defaults so + # `load_settings()` reports `None` for a fresh install. This lets the + # client distinguish "no preference" from an explicit saved choice, + # so a user who genuinely picked English (and has `language: "en"` + # persisted on disk) is no longer overridden by the browser hint on + # their first hydration. When a user picks a locale in the Settings + # modal the value is written here and the field is set explicitly. "bot_name": os.getenv( "HERMES_WEBUI_BOT_NAME", "Hermes" ), # display name for the assistant @@ -11723,6 +11729,16 @@ def load_settings() -> dict: settings["default_model_provider"] = str(model_cfg.get("provider")) except Exception: logger.debug("Failed to resolve default model provider for settings") + # #7622/#7730 (round 4): keep the tri-state signal explicit. `language` + # is intentionally absent from `_SETTINGS_DEFAULTS` (see above), so + # without this line a fresh install's returned dict would OMIT the key + # entirely and the API payload would carry no `language` field. Emit an + # explicit `None` (serialized as JSON `null`) so the client receives the + # three-way signal it trusts: `null` = no preference, "en" = explicitly + # saved English, any other code = explicitly chosen locale. Stored + # values (including a legacy English pick written before this change) + # win via the merge above and are never touched here. + settings.setdefault("language", None) return settings @@ -11730,6 +11746,15 @@ _SETTINGS_ALLOWED_KEYS = set(_SETTINGS_DEFAULTS.keys()) - { "password_hash", "default_model", "simplified_tool_calling", +} | { + # #7622 (round 3): `language` is intentionally absent from + # `_SETTINGS_DEFAULTS` so a fresh install returns `None` for + # `settings["language"]` and the client can distinguish "no + # preference" from an explicit saved choice. But the user + # MUST still be able to pick a locale in the Settings modal, + # so we add it back to the explicit allow-list here. The + # existing BCP-47 validation at save-time still applies. + "language", } _SETTINGS_ENUM_VALUES = { "send_key": {"enter", "ctrl+enter", "shift+enter"}, diff --git a/api/models.py b/api/models.py index dd775c68..0322a97b 100644 --- a/api/models.py +++ b/api/models.py @@ -4788,7 +4788,41 @@ class _FullSessionResolveRequired(RuntimeError): """Internal signal that a full sidecar load must enter the bounded path.""" -_FULL_SESSION_RESOLVE_MAX_CONCURRENT = 2 +def _read_max_session_resolve_concurrent() -> int: + """#7421: env override for the heavy session-resolve cap. The + hardcoded literal 2 is too low for high-concurrency + deployments where several parallel active sessions all need + a full-transcript resolve; operators can set + ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` to a positive int to + raise the cap without code changes. A bad or missing value + falls back to the previous default of 2, so an operator who + sets ``HERMES_WEBUI_MAX_SESSION_RESOLVE=0`` or a non-numeric + value does not regress to a blocked or unbounded state. + + The cap is a *safety bound* (per-resolve cost tracked in + #7310), not a user preference — a typo like ``=999999`` + must fall back to the safe default, never to the permissive + extreme. Every other bad input in this function falls back + to ``2``; out-of-range values do too. The previously + accepted upper bound (64) is reachable only via an explicit + in-range operator value; a profile's ``.env`` cannot reach + it at all (see ``_PROTECTED_ENV_KEYS``). + """ + raw = (os.getenv("HERMES_WEBUI_MAX_SESSION_RESOLVE") or "").strip() + if not raw: + return 2 + try: + n = int(raw) + except (TypeError, ValueError): + return 2 + if n < 1: + return 2 + if n > 64: + return 2 # #7656 round-3: typo-safe fallback, not clamp-up-to-64 + return n + + +_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent() _FULL_SESSION_RESOLVE_SLOTS = threading.BoundedSemaphore( _FULL_SESSION_RESOLVE_MAX_CONCURRENT ) diff --git a/api/profiles.py b/api/profiles.py index a4b5d8f7..e56b32fd 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -202,6 +202,14 @@ _PROTECTED_ENV_KEYS = frozenset({ 'HERMES_WEBUI_ISOLATED_PROFILE', # Server-wide sidebar window; a profile .env must not widen the per-request query. 'HERMES_WEBUI_VISIBLE_SESSION_LIMIT', + # #7656 round-3: HERMES_WEBUI_MAX_SESSION_RESOLVE is a process-wide resource + # cap (controls the BoundedSemaphore that gates full-transcript resolves). + # A profile's .env is per-profile by design; allowing it to set a + # server-wide limit (and a first-load-wins asymmetry) would let a + # single profile drag the rest into a tighter or looser posture than + # the operator intended. Same shape as the isolated-profile key: only + # the operator/launcher env at startup can set it. + 'HERMES_WEBUI_MAX_SESSION_RESOLVE', }) @@ -939,6 +947,12 @@ _BLOCKED_RUNTIME_ENV_KEYS = { # profile's own env on any runtime/gateway-parity path. 'HERMES_WEBUI_ISOLATED_PROFILE', 'HERMES_WEBUI_VISIBLE_SESSION_LIMIT', + # #7656 round-3: process-wide resolve cap. Filter out a profile's + # .env override the same way we filter the isolated-profile key, so + # the cap stays at the operator/launcher value across profile + # switches rather than re-sizing to whatever the first-loaded + # profile env set. + 'HERMES_WEBUI_MAX_SESSION_RESOLVE', } diff --git a/api/routes.py b/api/routes.py index 62486aea..ead29cc8 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21896,6 +21896,99 @@ def _handle_live_models(handler, parsed): _env = str(_cp.get("key_env") or "").strip() return os.getenv(_env, "").strip() if _env else "" + def _custom_provider_models_discover_is_false(_cp): + """True when ``discover_models`` is an explicit ``false`` opt-out. + + Mirrors ``api.config._provider_discover_allowed`` / Hermes + Agent ``model_switch_providers._discover_flag``: ``discover_models`` + defaults to True, and the string forms ``"false"``/``"no"``/``"0"`` + (case-insensitive) mean False. Used to decide whether a + dict-shaped ``models`` mapping is a hand-pinned allowlist (only + when discovery is off) rather than per-model metadata. + """ + _discover = _cp.get("discover_models", True) if isinstance(_cp, dict) else True + if isinstance(_discover, str): + return _discover.strip().lower() in {"false", "no", "0"} + return not bool(_discover) + + def _custom_provider_allowlist_ids(_cp): + """Plural ``models`` list only — the explicit allowlist signal. + + The singular ``model`` field is sticky/default metadata, NOT an + allowlist: it must not gate live-catalog filtering, otherwise a + provider configured with only ``model: assistant`` (no ``models`` + list) would be collapsed to a single model. Only an explicit + ``models`` allowlist expresses "show exactly these models". + + An auto-discovered catalog is NOT an allowlist: when Hermes + persisted discovery results back into config (``models: {...}`` + plus ``models_discovered: true``), that mapping is a snapshot + of what the gateway exposed at discovery time. Gating on it + would permanently pin the live catalog to the first-discovery + set, silently dropping any model the user pulls in later + (LM Studio / Ollama). ``_provider_models_are_discovered_catalog()`` + is the shared predicate the ``/api/models`` path already uses; + when it says "discovered", return no allowlist and let the + live probe win. An explicit ``discover_models: false`` opt-out + re-pins the catalog (the predicate accounts for it), so a + hand-pinned discovered catalog still filters. + + A dict-shaped ``models`` mapping that is NOT marked discovered is + *per-model metadata* written by the Hermes Agent setup flow + (``hermes_cli/model_switch.py::_save_custom_provider`` and the + setup wizard) — e.g. ``{chat-a: {context_length: 128000}}``. It is + not a catalog narrow: treating its keys as an allowlist would + collapse the live picker to the single saved default (keyless + Ollama) while the CLI live-probe shows the full catalog. Only an + explicit ``discover_models: false`` opts into treating the dict + keys as a pinned allowlist. List/JSON-array-string/Python-literal + shapes remain plain allowlists. + + The plural value is decoded through ``_parse_config_string_list()`` + because ``hermes config set`` / JSON-mode editor saves persist + lists as quoted JSON-array strings (``'["chat-a","chat-b"]'``) or + Python literals (``"['chat-a']"``). Handling only ``dict``/``list`` + made a serialized allowlist fall through to ``[]``, which the + caller reads as "no allowlist configured" and floods the picker + with the full upstream catalog — the same class of bug as the + ``skills.disabled`` regression (#7120 / #7134). + """ + from api.config import _provider_models_are_discovered_catalog + + if _provider_models_are_discovered_catalog(_cp): + return [] + _ids = [] + _models = _cp.get("models") + # Serialized shapes only: ``hermes config set`` / JSON-mode + # editor saves persist lists as quoted JSON-array strings + # (``'["chat-a","chat-b"]'``) or Python literals + # (``"['chat-a']"``). Decode those through the shared helper so + # they are honored; native dict/list values are walked below so + # dict *entries* keep their id|model|name metadata (the decoder + # str()-ifies list members, which would mangle them). + if isinstance(_models, str): + _models = _parse_config_string_list(_models) + if isinstance(_models, dict): + # A dict-shaped ``models`` is per-model metadata written by + # the Hermes Agent setup flow, NOT a catalog narrow. Only a + # ``discover_models: false`` opt-out treats the dict keys as a + # pinned allowlist; otherwise return the empty allowlist so the + # live probe returns the full catalog. + if not _custom_provider_models_discover_is_false(_cp): + return [] + for _mid in _models: + if isinstance(_mid, str) and _mid.strip(): + _ids.append(_mid.strip()) + elif isinstance(_models, (list, tuple)): + for _item in _models: + if isinstance(_item, str) and _item.strip(): + _ids.append(_item.strip()) + elif isinstance(_item, dict): + _mid = _item.get("id") or _item.get("model") or _item.get("name") + if _mid and str(_mid).strip(): + _ids.append(str(_mid).strip()) + return _ids + # For 'custom' and 'custom:*' providers, provider_model_ids() # returns [] because they aren't real hermes_cli endpoints. # Fall back to the custom_providers entries from config.yaml so @@ -21904,11 +21997,13 @@ def _handle_live_models(handler, parsed): # Collect config-specified model IDs separately so they don't # prevent the live fetch below from running (#3718). _config_ids = [] + _allowlist_ids = [] if provider == "custom" or provider.startswith("custom:"): for _cp in _custom_provider_entries_for_request(): if custom_provider_entry is None: custom_provider_entry = _cp _config_ids.extend(_custom_provider_model_ids(_cp)) + _allowlist_ids.extend(_custom_provider_allowlist_ids(_cp)) # Always try live fetch for custom providers — config entries are a # fallback, not a replacement. The live endpoint should return ALL @@ -21979,15 +22074,39 @@ def _handle_live_models(handler, parsed): except Exception as _fetch_err: logger.debug("Live fetch from custom provider failed: %s", _fetch_err) - # If live fetch succeeded, merge with config entries (live takes - # priority). If live fetch failed, fall back to config-only list. + # If live fetch succeeded, filter the live catalog down to the + # config-declared models allowlist first, then append any + # allowlisted models the live endpoint didn't return. Custom + # providers (esp. New-API-style gateways) expose their ENTIRE + # catalog via /v1/models — including image/audio models that + # are not chat models. The picker must not surface models the + # user never declared in config.yaml. Only a NON-EMPTY explicit + # ``models`` allowlist gates the filter — a provider configured + # with just a singular ``model`` (no ``models`` list) keeps the + # unfiltered live catalog. When no allowlist is configured, + # return the live list as-is (preserves the pre-filter + # discovery behaviour). + # + # An empty allowlist (``models: []``, ``models: "[]"``, or a + # value that decodes to no usable ids) is deliberately treated + # as "not configured", NOT as "allow nothing". Gating on + # declared-ness instead would emit an EMPTY picker and make the + # provider unselectable — a harder failure than surfacing a few + # extra models, and unrecoverable from the UI because + # ``custom_providers`` is hand-edited in config.yaml (the WebUI + # has no write path for it). ``[]`` in practice means a + # leftover/placeholder key, not an intentional deny-all, and no + # deny-all use case exists: a provider the user wants hidden is + # removed from ``custom_providers`` outright. if ids: - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) + if _allowlist_ids: + _allowlist_set = set(_allowlist_ids) + ids = [m for m in ids if m in _allowlist_set] or [] + for _aid in _allowlist_ids: + if _aid not in ids: + ids.append(_aid) else: - ids = list(_config_ids) + ids = list(_allowlist_ids or _config_ids) # ── OpenAI-compat live fetch fallback ────────────────────────────────── # When provider_model_ids() is unavailable or returns [] for a provider diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 2989b587..0b81b9bb 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -89,6 +89,15 @@ contributor guidance; it does not change runtime behavior or CI gates. `models_cache.json` refresh timestamps), the stat-identity fallbacks, and the schema/version stamps. Start here before changing model-catalog caching, the `/api/models` cache keys, or the Codex catalog dependency (#2443, #7540). +- [`docs/architecture/live-models-allowlist.md`](architecture/live-models-allowlist.md): + current contract for how `/api/models/live` filters a custom provider's + upstream catalog: the four signals in evaluation order (discovered catalog + defers to the live probe, explicit plural `models:` allowlist filters, + singular `model:` never gates, no allowlist shows the full catalog), the + serialized-list shapes `hermes config set` persists, the deliberate + empty-allowlist-is-not-configured rule, and the probe-failure fallback. + Start here before changing custom-provider model filtering or + discovery-vs-allowlist semantics (#7165, #7404). - [`docs/architecture/profile-home-resolve-cache.md`](architecture/profile-home-resolve-cache.md): current contract for the call-scoped memoization around `_resolve_profile_home_param()` in `api/workspace.py`: what it caches, the diff --git a/docs/architecture/live-models-allowlist.md b/docs/architecture/live-models-allowlist.md new file mode 100644 index 00000000..83c7363a --- /dev/null +++ b/docs/architecture/live-models-allowlist.md @@ -0,0 +1,123 @@ +# Live model picker allowlist contract (`/api/models/live`) + +This document records the runtime contract for how `/api/models/live` decides +which models a `custom` / `custom:*` provider contributes to the chat model +picker. It describes shipped behavior and changes no runtime behavior. It was +added because the #7165 review flagged the contract as undocumented: the +filtering rules live in `_handle_live_models` (`api/routes.py`) and had only +code comments backing them. + +Start here before changing how the live catalog of a custom provider is +filtered, how discovery snapshots are distinguished from allowlists, or what +the endpoint returns when the upstream probe fails. + +## Why the endpoint filters at all + +OpenAI-compatible gateways (New-API-style aggregators, LM Studio, Ollama +proxies) expose their **entire** catalog from `/v1/models`, including +image-generation, audio, TTS, and embedding models that are not chat models. +The background enrichment path must not surface models the user never opted +into — but it also must not hide models the gateway gained since config was +last edited. The contract below is how those two goals are reconciled. + +## The four signals, in evaluation order + +For a custom provider the handler consults these signals: + +1. **Discovered catalog → no allowlist (live probe wins).** + When the provider entry carries `models_discovered: true` and discovery is + allowed (`discover_models` is not an explicit opt-out), the `models:` + mapping is a *snapshot Hermes persisted from a past discovery*, not a + hand-curated list. It must not gate the live probe: a model the user pulls + into LM Studio / Ollama after discovery has to appear in the picker. + Detected by the shared predicate + `api.config._provider_models_are_discovered_catalog()` — the same + predicate the `/api/models` catalog path uses, so both endpoints agree. + With this signal, no allowlist is derived and the full live catalog is + returned. + +2. **Explicit plural `models:` allowlist → filter.** + A non-empty plural `models:` value on an entry that is *not* a discovered + catalog expresses "show exactly these models". The live catalog is + intersected with it, then any allowlisted model missing from the live + response is appended (so an allowlisted-but-offline model stays + selectable). This is the #7165 filter. Only **list** and **string** + (serialized) shapes are plain allowlists — see "Dict-shaped `models:`" + below. + +3. **Singular `model:` → never gates.** + The singular field is sticky/default metadata (the model to preselect), + not an allowlist. A provider configured only with `model: assistant` keeps + the full unfiltered live catalog; gating on it would collapse discovery to + a single model. + +4. **No allowlist → full live catalog.** + Preserves the pre-#7165 discovery behavior for setups that rely on it. + +### Serialized shapes + +`hermes config set` and the JSON-mode editor persist lists as quoted +JSON-array strings (`'["chat-a","chat-b"]'`) or Python literals +(`"['chat-a']"`). The plural value is decoded through the shared +`_parse_config_string_list()` before id extraction — but only when the value +is a `str`; native `dict` / `list` values are walked directly so dict *entries* +keep their `id|model|name` metadata (the decoder stringifies members). A +dict-only parser drops serialized allowlists to "no allowlist" and floods the +picker — the same bug class as `skills.disabled` (#7120 / #7134). + +### Dict-shaped `models:` + +A dict-shaped `models:` mapping (e.g. `{chat-a: {context_length: 128000}}`) +is *per-model metadata* written by the Hermes Agent setup flow — +`hermes_cli/model_switch.py::_save_custom_provider` and the setup wizard — +**not** a catalog narrow. Treating its keys as an allowlist would collapse the +live picker to the single saved default (keyless Ollama) while the CLI +live-probe shows the full catalog. So a dict is **never** a plain allowlist: + +- without an explicit `discover_models: false`, a dict-shaped `models:` + contributes **no allowlist** and the full live catalog is returned; +- with `discover_models: false` (bool `False` or the string forms + `"false"`/`"no"`/`"0"`, case-insensitive — mirroring + `model_switch_providers.py:557`), the dict **keys** are honored as a pinned + allowlist. + +This complements the discovered-catalog rule: `models_discovered: true` +without the opt-out is already ignored (signal 1). The dict guard covers the +Agent-setup shape that is *not* flagged discovered. + +If someone later "fixes" the dict shape back into an always-allowlist, the +regression test `test_dict_models_without_discover_false_does_not_gate` fails +and forces the trade-off to be re-argued. + +### Empty allowlist semantics + +`models: []`, `models: "[]"`, an empty mapping, or a blank scalar are +deliberately treated as **not configured**, never as "allow nothing". The +filter runs only on a non-empty allowlist because gating on declared-ness +would emit an empty picker and make the provider unselectable — a harder +failure than showing a few extra models, and unrecoverable from the UI +(`custom_providers` has no WebUI write path; it is hand-edited in +config.yaml). There is no deny-all use case: a provider the user wants hidden +is removed from `custom_providers` outright. + +### Probe failure / empty probe + +When the upstream probe fails or returns no models, the endpoint falls back +to `allowlist or config ids` — the non-empty plural allowlist first, so a +singular `model:` outside the allowlist cannot leak in precisely when the +probe is unavailable; with no allowlist, the configured ids (singular + +plural) are served. A discovered catalog therefore still serves its saved +models on a transient probe failure instead of emptying the picker. + +## Tests + +- `tests/test_issue3718_live_models_custom_probe.py` — real-handler + (`_handle_live_models`) coverage of every signal above: plural list / + JSON-array-string / Python-literal / list-of-dicts filtering, dict-shaped + `models:` (not gated) and dict + `discover_models: false` (pinned), + singular passthrough, malformed safety, discovered-catalog passthrough, + `discover_models: false` pinning, and both fallback branches. +- `tests/test_issue7404_models_discovered_not_allowlist.py` — the + `/api/models` side of the discovered-catalog contract. +- `tests/test_byok_model_dropdown.py` — picker rendering, profile-scoped + caching, and current-session model preservation around this endpoint. diff --git a/static/boot.js b/static/boot.js index d6476b60..06c3888d 100644 --- a/static/boot.js +++ b/static/boot.js @@ -3431,8 +3431,17 @@ window._mirrorSpeechSettingsFromServer=_mirrorSpeechSettingsFromServer; localStorage.setItem('hermes-font-size',fontSize); _applyFontSize(fontSize); if(typeof setLocale==='function'){ + // #7622 (round 3): the settings payload's `s.language` is + // absent (None) for a fresh install, so an explicit non-empty + // value is the user's genuine saved choice. The browser + // navigator hint is now read via the guarded + // `_detectBrowserLanguageHint()` helper (round-3 finding 2) so + // a throwing `navigator` accessor in some embedded webviews + // can no longer abort this branch and reset loaded preferences. + // The fallback ternary preserves the pre-#7622 boot behaviour + // when neither helper is in scope (defence in depth). const _lang=typeof resolvePreferredLocale==='function' - ? resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang')) + ? resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang'), _detectBrowserLanguageHint()) : (s.language || localStorage.getItem('hermes-lang') || 'en'); setLocale(_lang); if(typeof applyLocaleToDOM==='function')applyLocaleToDOM(); diff --git a/static/i18n.js b/static/i18n.js index 0192fd5c..269ffe43 100644 --- a/static/i18n.js +++ b/static/i18n.js @@ -26983,15 +26983,68 @@ function resolveLocale(lang) { /** * Resolve locale with precedence: - * 1) primary (typically server setting) - * 2) fallback (typically localStorage) - * 3) English - * @param {string} primary - * @param {string} fallback + * 1) primary (typically server setting) + * 2) fallback (typically localStorage) + * 3) fallback2 (typically browser navigator hint — first-visit only) + * 4) 'en' (final fallback) + * + * Resolve a language preference to a known LOCALES key, applying a strict + * `primary -> fallback -> fallback2 -> 'en'` precedence chain. + * + * #7622 (round 3): the server no longer injects `"language": "en"` as a + * schema default — `load_settings()` reports `None` (the key is absent + * from the response) until the user explicitly picks a locale in the + * Settings modal. This lets the resolver use `primary` directly + * without a `primary === 'en'` special case: + * - an explicit `primary` (the server's stored value) always wins + * - a `None` / empty `primary` falls through to `fallback` (localStorage) + * - `fallback2` (the browser navigator hint) only fires when neither + * server nor localStorage has a preference + * - `'en'` is the final safety net + * + * Side benefit: a user who picked English on purpose (their settings + * file contains `"language": "en"`) is no longer overridden by a + * non-English browser locale on hydration, because the round-2 + * `primary === 'en'` skip has been removed. + * + * @param {string|null|undefined} primary server-stored language + * @param {string|null|undefined} fallback localStorage `hermes-lang` + * @param {string|null|undefined} fallback2 browser navigator hint * @returns {string} */ -function resolvePreferredLocale(primary, fallback) { - return resolveLocale(primary) || resolveLocale(fallback) || 'en'; +function resolvePreferredLocale(primary, fallback, fallback2) { + return resolveLocale(primary) + || resolveLocale(fallback) + || resolveLocale(fallback2) + || 'en'; +} + +/** + * Detect the browser's preferred language for first-visit locale + * selection. Guarded against `navigator` access exceptions: some + * embedded webviews (Tauri desktop shell, kiosk modes) report a + * `navigator` object but throw on `.languages` / `.language` reads. + * Callers (loadLocale, boot.js, panels.js) must use this helper rather + * than reading `navigator` directly so a single throw does not abort + * the boot or settings-panel hydration. + * + * @returns {string|null} the first entry of `navigator.languages`, or + * `navigator.language`, or `null` when neither is reachable. + */ +function _detectBrowserLanguageHint() { + try { + const nav = (typeof navigator !== 'undefined') ? navigator : null; + if (!nav) return null; + if (Array.isArray(nav.languages) && nav.languages.length) { + return nav.languages[0] || null; + } + if (typeof nav.language === 'string' && nav.language) { + return nav.language; + } + return null; + } catch (_) { + return null; + } } /** @@ -27029,11 +27082,26 @@ function setLocale(lang) { /** * Load locale from localStorage (called once at boot, before DOMContentLoaded). * Server-persisted preference is applied later in loadSettingsPanel(). + * + * #7622: on the FIRST visit (no stored preference), fall back to the + * browser's preferred language (`navigator.languages[0]` / + * `navigator.language`) so non-English speakers don't have to dig into + * Settings after every fresh install. This is strictly a first-visit + * hint — the moment the user picks a language in Settings (or the + * server-side preference is applied via `loadSettingsPanel`), the + * stored value takes over and the browser hint is never consulted + * again for that browser profile. */ function loadLocale() { let stored = null; try { stored = localStorage.getItem('hermes-lang'); } catch (_) {} - setLocale(resolvePreferredLocale(null, stored)); + // Only consult the browser hint when no stored preference exists. + // The settings panel and `loadSettingsPanel()` will overwrite this + // hint on their first call, so explicit user / server preferences + // always win. #7622 (round 3): the browser-hint read is delegated + // to `_detectBrowserLanguageHint()` so a `navigator` access throw + // (some embedded webviews) can no longer abort loadLocale(). + setLocale(resolvePreferredLocale(stored, _detectBrowserLanguageHint())); } /** diff --git a/static/panels.js b/static/panels.js index dd5090c3..9215cc77 100644 --- a/static/panels.js +++ b/static/panels.js @@ -9300,8 +9300,17 @@ async function loadSettingsPanel(){ _setHiddenTabs(hiddenTabs); _applyTabVisibility(hiddenTabs); _renderTabVisibilityChips(); + // #7622 (round 3): the settings payload's `settings.language` is + // absent (None) for a fresh install, so an explicit non-empty + // value is the user's genuine saved choice. The browser + // navigator hint is now read via the guarded + // `_detectBrowserLanguageHint()` helper (round-3 finding 2) so + // a throwing `navigator` accessor can no longer abort settings + // hydration before model, provider, plugin and extension sections + // are populated. The fallback ternary preserves the pre-#7622 + // settings-modal behaviour when neither helper is in scope. const resolvedLanguage=(typeof resolvePreferredLocale==='function') - ? resolvePreferredLocale(settings.language, localStorage.getItem('hermes-lang')) + ? resolvePreferredLocale(settings.language, localStorage.getItem('hermes-lang'), _detectBrowserLanguageHint()) : (settings.language || localStorage.getItem('hermes-lang') || 'en'); // Keep settings modal and current page strings in sync with the resolved locale. if(typeof setLocale==='function'){ diff --git a/tests/test_byok_model_dropdown.py b/tests/test_byok_model_dropdown.py index 6a8329aa..976dfdd6 100644 --- a/tests/test_byok_model_dropdown.py +++ b/tests/test_byok_model_dropdown.py @@ -291,6 +291,13 @@ class TestLiveModelsCustomProviderFallback: "model": "gpt-5.5", "models": {"gpt-5.5-mini": {}}, "base_url": "https://right.codes/codex/v1", + # dict-shaped ``models`` is per-model metadata (Agent setup + # flow) and only narrows the live catalog when discovery is + # explicitly off — the new #7165 contract. This test still + # wants provider-scoping (no sibling leak), so pin the + # catalog via ``discover_models: false`` instead of relying + # on the removed dict-as-allowlist behavior. + "discover_models": False, }, { "name": "infini-ai", @@ -308,7 +315,7 @@ class TestLiveModelsCustomProviderFallback: resp = self._call_live_models(monkeypatch, cfg, "custom:rightcode-codex") assert resp["provider"] == "custom:rightcode-codex" - assert [m["id"] for m in resp["models"]] == ["gpt-5.5", "gpt-5.5-mini"] + assert [m["id"] for m in resp["models"]] == ["gpt-5.5-mini"] def test_bare_custom_fallback_ignores_named_custom_provider_models(self, monkeypatch): """Bare custom only represents unnamed custom entries, not named siblings.""" diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 8939523b..ca6b31c1 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -1,193 +1,398 @@ -"""Regression test for #3718 -- /api/models/live skips probe for custom providers. +"""Handler-level regression tests for /api/models/live custom-provider probing. -When a custom provider (``custom_providers`` entry in config.yaml) has a -``model:`` field but no ``models:`` allowlist, the live endpoint previously -populated ``ids`` from the config entry and then skipped the live ``/v1/models`` -probe (guarded by ``if not ids``). The fix collects config-specified model IDs -in a separate ``_config_ids`` list so the live fetch always runs, and merges -config entries as a fallback after the fetch. +These tests invoke the real ``routes._handle_live_models`` and inspect the +emitted ``models`` payload. Earlier revisions of this file re-implemented the +filtering loop locally, which meant they passed unchanged even with the +production filter deleted (a vacuous oracle). Every case here must fail if the +allowlist gating in ``api/routes.py`` regresses. + +Covered shapes: + * native ``models`` list → live catalog filtered + * JSON-array-string ``models`` → live catalog filtered (#7120 class) + * Python-literal-string ``models`` → live catalog filtered + * singular ``model`` only, no ``models`` → live catalog NOT gated + * malformed serialized ``models`` → safe, treated as scalar name + * allowlisted model absent from live catalog → appended + * live probe failure → falls back to config ids + * dict-shaped ``models`` (Agent setup) → live catalog NOT gated (per-model + metadata, not an allowlist) + * dict-shaped ``models`` + ``discover_models: + false`` → pinned to dict keys (explicit opt-out) + * ``models_discovered: true`` catalog → live catalog NOT gated (discovery + metadata is not an allowlist) + * discovered + ``discover_models: false`` → still pinned (explicit opt-out) """ + +import io import json -import pathlib -import unittest -from unittest import mock +import sys +import types +from urllib.parse import urlparse -REPO = pathlib.Path(__file__).parent.parent -ROUTES_PY = REPO / "api" / "routes.py" +import pytest -class TestLiveModelsCustomProviderProbe(unittest.TestCase): - """Live endpoint must probe the upstream /v1/models even when config has entries.""" - - def test_custom_provider_with_model_field_probes_upstream(self): - """When a custom provider has model: in config, live fetch must still run. - - Before the fix, _custom_provider_model_ids() populated ids=["assistant"], - and the `if not ids` guard prevented the upstream /v1/models probe from - running. The endpoint returned only ["assistant"] instead of the full - upstream catalog. - - We test the fix by simulating the logic directly: config provides - model IDs, the upstream probe returns a different set, and the result - must contain BOTH the live models and the config fallback. - """ - # Simulate config-specified model IDs - _config_ids = ["assistant"] - - # Simulate upstream /v1/models response - live_models = [ - "assistant", - "assistant-pro", - "assistant-zdr", - "gpt-5.5:pt", - "claude-sonnet-4.6:pt", - ] - - # Simulate the merge logic from the fix: - # if ids: merge config entries not in live set - # else: fall back to config-only list - ids = list(live_models) # live fetch succeeded - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) - - # "assistant" from config should not duplicate the live result - self.assertEqual(ids, live_models) - # All live models must be present - for m in live_models: - self.assertIn(m, ids) - - def test_custom_provider_falls_back_to_config_when_probe_fails(self): - """When the upstream probe fails, config entries must be used as fallback.""" - _config_ids = ["assistant", "custom-only-model"] - - # Simulate failed live fetch: ids stays empty - ids = [] - - # Fallback logic from the fix - if not ids: - ids = list(_config_ids) - - self.assertEqual(ids, ["assistant", "custom-only-model"]) - - def test_config_only_model_appended_when_not_in_live_results(self): - """Config-specified models not in the live results must be appended.""" - _config_ids = ["assistant", "local-finetune"] - live_models = ["assistant", "assistant-pro", "gpt-5.5:pt"] - - ids = list(live_models) - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) - - # "assistant" is in both, "local-finetune" is config-only - self.assertIn("local-finetune", ids) - self.assertIn("assistant-pro", ids) - self.assertEqual(ids.count("assistant"), 1, "assistant must not be duplicated") - - def test_live_fetch_guard_no_longer_checks_ids(self): - """The live fetch block must not be guarded by `if not ids`. - - This is a structural regression guard: the fix replaced - `if not ids and (provider == "custom" ...)` with - `if provider == "custom" ...` so the probe always runs regardless - of whether config entries populated ids. - """ - source = ROUTES_PY.read_text(encoding="utf-8") - - # Find the "Always try live fetch" comment (added by the fix) - marker = "Always try live fetch for custom providers" - self.assertIn(marker, source, ( - "routes.py must contain the 'Always try live fetch' comment (#3718)" - )) - - # Extract the block between the marker and the next major section - marker_pos = source.find(marker) - next_section = source.find("OpenAI-compat live fetch fallback", marker_pos) - self.assertNotEqual(next_section, -1, "could not find next section marker") - block = source[marker_pos:next_section] - - # The old `if not ids and` guard must NOT appear in this block - self.assertNotIn("if not ids and", block, ( - "Live fetch for custom providers must not be guarded by 'if not ids' (#3718)" - )) - - def test_mocked_live_fetch_returns_full_catalog_plus_config_entry(self): - """Integration test: mock urlopen and exercise the real fetch/parse/merge path. - - This test patches urllib.request.urlopen to return a fake /v1/models - response, then runs the same fetch+merge logic that the live handler - uses, verifying that config entries are merged and live models are - included. - """ - fake_models = {"data": [ - {"id": "assistant"}, - {"id": "assistant-pro"}, - {"id": "gpt-5.5:pt"}, - ]} - fake_body = json.dumps(fake_models).encode("utf-8") - fake_resp = mock.MagicMock() - fake_resp.read.return_value = fake_body - fake_resp.__enter__ = mock.MagicMock(return_value=fake_resp) - fake_resp.__exit__ = mock.MagicMock(return_value=False) - - _config_ids = ["assistant", "local-only"] - - with mock.patch("urllib.request.urlopen", return_value=fake_resp): - # Replicate the fetch+parse logic from routes.py - import urllib.request - ids = [] - _req = urllib.request.Request( - "http://localhost:4000/v1/models", - headers={"Authorization": "Bearer test-key"}, - ) - with urllib.request.urlopen(_req, timeout=5) as _resp: - _body = json.loads(_resp.read()) - - if isinstance(_body, dict): - _data = _body.get("data", []) - if isinstance(_data, list): - ids = [m.get("id", "") for m in _data if m.get("id")] - - # Apply the merge logic from the fix - if ids: - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) - else: - ids = list(_config_ids) - - # Live models must all be present - self.assertIn("assistant", ids) - self.assertIn("assistant-pro", ids) - self.assertIn("gpt-5.5:pt", ids) - # Config-only entry must be appended - self.assertIn("local-only", ids) - # No duplicates - self.assertEqual(ids.count("assistant"), 1) - - def test_timeout_uses_config_constant(self): - """The live fetch must use CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS, not a hardcoded value.""" - source = ROUTES_PY.read_text(encoding="utf-8") - - # Find the custom-provider live fetch block - marker = "Always try live fetch for custom providers" - marker_pos = source.find(marker) - next_section = source.find("OpenAI-compat live fetch fallback", marker_pos) - block = source[marker_pos:next_section] - - # Must use the constant, not a bare number - self.assertIn("CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS", block, ( - "Live fetch timeout must use CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS " - "instead of a hardcoded value (#3718 review feedback)" - )) - self.assertNotIn("timeout=8", block, ( - "Live fetch must not use hardcoded timeout=8 (#3718 review feedback)" - )) +def _install_provider_model_ids(monkeypatch, fn): + """Stub hermes_cli.models.provider_model_ids (returns [] for custom:*).""" + hermes_cli = types.ModuleType("hermes_cli") + hermes_cli.__path__ = [] + models = types.ModuleType("hermes_cli.models") + models.provider_model_ids = fn + monkeypatch.setitem(sys.modules, "hermes_cli", hermes_cli) + monkeypatch.setitem(sys.modules, "hermes_cli.models", models) -if __name__ == "__main__": - unittest.main() \ No newline at end of file +class _FakeResponse(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *_exc): + self.close() + return False + + +def _install_live_catalog(monkeypatch, routes, catalog, *, fail=False): + """Stub the upstream /v1/models probe used by the custom-provider branch.""" + import urllib.request + + requested: list[str] = [] + + def _fake_urlopen(req, timeout=None): + requested.append(getattr(req, "full_url", str(req))) + if fail: + raise OSError("probe failed") + body = {"data": [{"id": mid} for mid in catalog]} + return _FakeResponse(json.dumps(body).encode()) + + monkeypatch.setattr(urllib.request, "urlopen", _fake_urlopen) + return requested + + +def _run_live_models(monkeypatch, routes, custom_provider, *, catalog, fail=False): + """Invoke the real handler for provider=custom:test-gateway.""" + import api.config as config + import api.profiles as profiles + + routes._clear_live_models_cache() + monkeypatch.setattr( + routes, "j", lambda _handler, payload, status=200, extra_headers=None: payload + ) + monkeypatch.setattr(config, "_resolve_provider_alias", lambda provider: provider) + monkeypatch.setattr(profiles, "get_active_profile_name", lambda: "default") + + config_data = { + "model": {"provider": "custom:test-gateway"}, + "custom_providers": [custom_provider], + } + monkeypatch.setattr(config, "get_config", lambda: config_data) + monkeypatch.setattr(routes, "cfg", config_data, raising=False) + + # provider_model_ids() returns [] for custom:* → drives the custom branch. + _install_provider_model_ids(monkeypatch, lambda provider: []) + requested = _install_live_catalog(monkeypatch, routes, catalog, fail=fail) + + parsed = urlparse("/api/models/live?provider=custom:test-gateway") + payload = routes._handle_live_models(object(), parsed) + return payload, requested + + +def _ids(payload): + return [m["id"] for m in payload.get("models", [])] + + +_LIVE_CATALOG = [ + "chat-a", + "chat-b", + "image-gen-1", # non-chat noise the picker must not surface + "audio-tts-1", +] + +_BASE_PROVIDER = { + "name": "Test Gateway", + "base_url": "https://gateway.example/v1", + "api_key": "sk-test", +} + + +@pytest.mark.parametrize( + "models_value", + [ + pytest.param(["chat-a", "chat-b"], id="native-list"), + pytest.param('["chat-a","chat-b"]', id="json-array-string"), + pytest.param("['chat-a', 'chat-b']", id="python-literal-string"), + pytest.param( + [{"id": "chat-a"}, {"model": "chat-b"}], id="list-of-dicts" + ), + ], +) +def test_allowlist_filters_live_catalog(monkeypatch, models_value): + """An explicit plural allowlist must trim the live catalog to itself. + + The JSON-array-string and Python-literal cases are the regression: those + shapes are what ``hermes config set`` / JSON-mode editor saves persist, and + a dict/list-only parser drops them to ``[]`` → read as "no allowlist" → the + full upstream catalog floods the picker. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models=models_value) + payload, requested = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == ["chat-a", "chat-b"] + assert "image-gen-1" not in _ids(payload) + assert "audio-tts-1" not in _ids(payload) + + +def test_dict_models_without_discover_false_does_not_gate(monkeypatch): + """A dict-shaped ``models`` is per-model metadata — NOT an allowlist (#7165). + + Hermes Agent's setup flow and ``hermes_cli/model_switch.py::_save_custom_provider`` + persist per-model metadata as a mapping, e.g. ``{chat-a: {context_length: 128000}}``. + Treating its keys as an allowlist would collapse the live picker to the saved + default while the CLI live-probe shows the full catalog. Without an explicit + ``discover_models: false`` opt-out the dict must NOT gate — full live catalog wins. + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={"chat-a": {"context_length": 128000}, "chat-b": {"context_length": 32000}}, + ) + payload, requested = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == _LIVE_CATALOG, ( + "dict-shaped models is per-model metadata, not an allowlist — the " + "full live catalog must be surfaced" + ) + + +def test_dict_models_with_discover_false_pins_to_keys(monkeypatch): + """``discover_models: false`` opts a dict-shaped ``models`` into an allowlist. + + The explicit opt-out (bool ``False`` or the string forms ``"false"``/``"no"``/``"0"``) + means the dict keys are a hand-pinned allowlist and must filter the live catalog. + """ + import api.routes as routes + + for discover in (False, "false", "no", "0"): + provider = dict( + _BASE_PROVIDER, + models={"chat-a": {"context_length": 128000}, "chat-b": {"context_length": 32000}}, + discover_models=discover, + ) + payload, requested = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == ["chat-a", "chat-b"], ( + f"discover_models={discover!r} must pin the dict keys as the allowlist" + ) + assert "image-gen-1" not in _ids(payload) + assert "audio-tts-1" not in _ids(payload) + + +def test_singular_model_only_does_not_gate_live_catalog(monkeypatch): + """``model:`` is default metadata, not an allowlist — keep full discovery.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-a") + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert _ids(payload) == _LIVE_CATALOG + + +def test_malformed_serialized_models_is_safe(monkeypatch): + """A broken JSON-array string must not crash or silently drop the gate. + + ``_parse_config_string_list()`` treats an undecodable value as one literal + name, so the allowlist stays non-empty and the filter still runs. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models='["chat-a", "chat-b"') # missing ] + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + # Treated as a single (unmatched) name → appended, catalog not flooded. + assert "image-gen-1" not in _ids(payload) + assert "audio-tts-1" not in _ids(payload) + + +def test_allowlisted_model_missing_from_live_is_appended(monkeypatch): + """A declared model the upstream didn't return must still be selectable.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models=["chat-a", "chat-offline"]) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert _ids(payload) == ["chat-a", "chat-offline"] + + +def test_contradictory_singular_model_outside_plural_allowlist_is_ignored(monkeypatch): + """A singular default outside the plural allowlist must not leak into the picker. + + Paired test: whether the live probe succeeds or fails, only the explicit + plural allowlist is exposed. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-default", models=["chat-a"]) + + # 1. Successful live fetch -> only plural allowlist + payload_success, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=False + ) + assert _ids(payload_success) == ["chat-a"] + + # 2. Failed live fetch -> still only plural allowlist (not chat-default) + payload_fail, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=True + ) + assert _ids(payload_fail) == ["chat-a"] + + +def test_probe_failure_without_allowlist_falls_back_to_singular_model(monkeypatch): + """When no plural allowlist is configured and probe fails, singular model is used.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-default") + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=True + ) + assert _ids(payload) == ["chat-default"] + + +@pytest.mark.parametrize( + "models_value", + [ + pytest.param([], id="native-empty-list"), + pytest.param("[]", id="serialized-empty-list"), + pytest.param({}, id="empty-mapping"), + pytest.param(" ", id="blank-scalar"), + ], +) +def test_empty_allowlist_is_treated_as_not_configured(monkeypatch, models_value): + """An empty ``models`` value must NOT be read as "allow nothing". + + Pins a deliberate semantic decision. Gating the filter on *declared-ness* + rather than on a non-empty allowlist would make ``models: []`` emit an + EMPTY picker, leaving the provider unselectable — a harder failure than + surfacing a few extra models, and unrecoverable from the UI because + ``custom_providers`` has no WebUI write path (hand-edited config.yaml only). + There is also no deny-all use case: a provider the user wants hidden is + removed from ``custom_providers`` outright. + + If someone later "fixes" the empty case into a deny-all gate, this test + fails and forces the trade-off to be re-argued rather than silently shipped. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models=models_value) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert _ids(payload) == _LIVE_CATALOG, ( + "empty allowlist must fall through to full live discovery, " + "never collapse the picker to zero models" + ) + assert _ids(payload), "picker must never be emptied by an empty allowlist" + + +def test_models_discovered_catalog_is_not_an_allowlist(monkeypatch): + """``models_discovered: true`` marks ``models:`` as discovery metadata (#7165). + + Hermes persists *discovery results* back into config as ``models: {...}`` + plus ``models_discovered: true``. That mapping is a snapshot of what the + gateway exposed at discovery time — NOT a hand-curated allowlist. Treating + it as one permanently pins the live catalog to the first-discovery set, so + a model the user later pulls into LM Studio / Ollama is silently dropped + from ``/api/models/live`` (the exact regression the gate reproduced: merge + base returned ``saved-a``, ``saved-b`` AND ``new-live-c``; the pinned head + dropped ``new-live-c``). + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={ + "saved-a": {"context_length": 128000}, + "saved-b": {"context_length": 32000}, + }, + models_discovered=True, + ) + payload, requested = _run_live_models( + monkeypatch, + routes, + provider, + catalog=["saved-a", "saved-b", "new-live-c"], + ) + + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == ["saved-a", "saved-b", "new-live-c"], ( + "a discovered catalog must not gate the live probe — models that " + "appear upstream after discovery (new-live-c) must reach the picker" + ) + + +def test_discovered_catalog_with_discover_models_false_still_pins(monkeypatch): + """``discover_models: false`` re-pins a discovered catalog (control). + + The explicit opt-out wins over ``models_discovered: true`` — same rule as + the ``/api/models`` catalog path (`api.config._provider_discover_allowed`). + Filtering to the configured set must survive the discovered-catalog fix. + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={ + "saved-a": {"context_length": 128000}, + "saved-b": {"context_length": 32000}, + }, + models_discovered=True, + discover_models=False, + ) + payload, _ = _run_live_models( + monkeypatch, + routes, + provider, + catalog=["saved-a", "saved-b", "new-live-c"], + ) + + assert _ids(payload) == ["saved-a", "saved-b"] + + +def test_discovered_catalog_probe_failure_falls_back_to_saved_models(monkeypatch): + """Discovered + failed probe → saved models, not an empty picker (control). + + Releasing the allowlist must not empty the fallback: when the live probe + fails, the previously discovered ``models:`` mapping is still the best + answer and must be served from ``_config_ids``. + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={ + "saved-a": {"context_length": 128000}, + "saved-b": {"context_length": 32000}, + }, + models_discovered=True, + ) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=[], fail=True + ) + + assert _ids(payload) == ["saved-a", "saved-b"] diff --git a/tests/test_issue7421_resolve_cap_config.py b/tests/test_issue7421_resolve_cap_config.py new file mode 100644 index 00000000..6cad1970 --- /dev/null +++ b/tests/test_issue7421_resolve_cap_config.py @@ -0,0 +1,297 @@ +"""Regression coverage for issue #7421: configurable +``HERMES_WEBUI_MAX_SESSION_RESOLVE`` cap on the heavy +full-transcript resolve semaphore. + +The cap is hardcoded to ``2`` in +``api.models.py::_FULL_SESSION_RESOLVE_MAX_CONCURRENT``. The +maintainer's diagnosis is that the literal 2 is too low for +high-concurrency deployments where several parallel active +sessions all need a full-transcript resolve, and that the +deeper per-read cost is tracked separately in #7310. This PR +is the self-contained first step: read the cap from the env +var so operators can raise it without code changes, while +preserving the previous default of 2 and bounding the upper +end against typos like ``=999999``. + +#7656 re-gate (9/22 23:18): the previous round used a private +``_import_models_with_env`` helper that did ``sys.modules.pop`` +on ``api.models`` and re-imported to recompute the module-level +cap constant. Modules already imported (``api.routes``, +``api.streaming``, ...) keep references to the *old* module's +``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` / ``_FULL_SESSION_RESOLVE_SLOTS``, +so a later route test would save through one module and read +through another — 9 of those tests fail in CI (none fail on +master, all 18 of the new failures are caused by this PR). + +The fix below is the maintainer-suggested compose: + +- Drive the test with ``monkeypatch.setenv`` so the env var is + restored at fixture teardown — no leak to other test files +- Test ``_read_max_session_resolve_concurrent()`` directly + (the function is already pure: only reads ``os.getenv``) so + no module reload is needed +- For the single test that needs the module-level constant + (``_FULL_SESSION_RESOLVE_MAX_CONCURRENT``), verify it equals + the helper's return value at import time — that is the real + production contract, and it is cheap to assert because the + helper itself only reads the env var +""" +from __future__ import annotations + +import threading +from pathlib import Path + +import pytest + +import api.models as models + +ROOT = Path(__file__).resolve().parents[1] + + +@pytest.fixture +def resolve_env(monkeypatch): + """Drive ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` via ``monkeypatch.setenv`` + so the env var is restored at fixture teardown — no leak to other + test files, no module reload required. + + The helper ``models._read_max_session_resolve_concurrent()`` is pure + (only ``os.getenv``) and reads the live env, so this fixture is the + entire test harness. The previous round's ``_import_models_with_env`` + used ``sys.modules.pop`` + ``importlib.import_module`` to recompute + the module-level cap constant against a freshly-set env, but that + left the original module unreferenced in ``sys.modules`` while + ``api.routes`` and friends still held the *old* module object — + root cause of the cross-suite pollution the maintainer reproduced. + """ + def set_resolve_env(value): + if value is None: + monkeypatch.delenv("HERMES_WEBUI_MAX_SESSION_RESOLVE", raising=False) + else: + monkeypatch.setenv("HERMES_WEBUI_MAX_SESSION_RESOLVE", str(value)) + return set_resolve_env + + +# ── default behavior (env unset) ────────────────────────────────────────────── + + +def test_default_cap_is_two_when_env_unset(resolve_env): + """Backward compatibility: an installation that does not set + the env var keeps the previous behavior. The bounded + semaphore is constructed with capacity 2, so 3+ parallel + full-resolves queue behind two slots as before.""" + resolve_env(None) + assert models._read_max_session_resolve_concurrent() == 2, ( + "unset env must keep the previous hardcoded default of 2 " + "so existing single-instance deployments are unchanged" + ) + + +def test_default_cap_is_two_when_env_empty_string(resolve_env): + """A user who sets the env var to an explicit empty string + (e.g. from a misconfigured .env file) is the same as unset. + The helper strips whitespace first; ``""`` is treated as no + value at all.""" + resolve_env("") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_default_cap_is_two_when_env_whitespace_only(resolve_env): + resolve_env(" \t\n") + assert models._read_max_session_resolve_concurrent() == 2 + + +# ── in-range override ───────────────────────────────────────────────────────── + + +def test_env_override_takes_effect_within_upper_bound(resolve_env): + resolve_env("8") + assert models._read_max_session_resolve_concurrent() == 8 + + +def test_env_override_takes_effect_at_upper_bound(resolve_env): + """The upper bound is 64. An operator who wants the full + ceiling must be able to reach it.""" + resolve_env("64") + assert models._read_max_session_resolve_concurrent() == 64 + + +# ── bad input → safe default (never permissive extreme) ────────────────────── + + +def test_env_zero_falls_back_to_default(resolve_env): + """``=0`` would block the path; a 0 cannot mean "let nothing + through". Fall back to 2 (the previous hardcoded value).""" + resolve_env("0") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_negative_falls_back_to_default(resolve_env): + resolve_env("-5") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_above_upper_bound_falls_back_to_default(resolve_env): + """#7656 round-3 finding 1: a typo like ``=999999`` previously + clamped **up** to the maximum, admitting 64 simultaneous + unbounded transcript loads — roughly 614 MB of parse memory + at the recorded 4.27 MB / parse. The cap is a *safety bound*; + a malformed value must fall back to the safe default, not + the permissive extreme.""" + resolve_env("999999") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_non_numeric_falls_back_to_default(resolve_env): + resolve_env("two") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_empty_after_strip_falls_back_to_default(resolve_env): + """Whitespace-only is empty after the strip; same as unset.""" + resolve_env("\n\t ") + assert models._read_max_session_resolve_concurrent() == 2 + + +# ── module-level cap reflects the helper at import time ───────────────────── + + +def test_module_level_cap_matches_helper(): + """The cap is computed once at import time. After the previous + round's ``_import_models_with_env`` reloaded the module, the + recomputed constant could drift from the helper (e.g. if a + test set the env, reloaded, then never cleaned up). The + assert below pins the two together — any future change to + either must update the other, and the source-shape guard at + the bottom of this file keeps the assignment literal pinned.""" + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == ( + models._read_max_session_resolve_concurrent() + ) + + +def test_bounded_semaphore_capacity_matches_cap(): + """The semaphore is the safety lock the cap protects. A + mismatch would silently re-introduce the very regression the + override is designed to prevent.""" + sem = models._FULL_SESSION_RESOLVE_SLOTS + assert isinstance(sem, threading.BoundedSemaphore) + # The semaphore was constructed with ``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` + # slots; verify the type is right and (cheaply) that the + # counter is at least 2 (the smallest documented value). + assert sem._initial_value == models._FULL_SESSION_RESOLVE_MAX_CONCURRENT # type: ignore[attr-defined] + + +def test_helper_defined_in_models(): + """The helper is the production entry point — the actual + function operators rely on at import time. Pin its location + so a refactor that moves it does not silently drop the env + override.""" + src = (ROOT / "api" / "models.py").read_text(encoding="utf-8") + assert "def _read_max_session_resolve_concurrent" in src, ( + "the env-override helper must live in api.models " + "next to the module-level cap constant it feeds" + ) + assert ( + "_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent()" + in src + ), ( + "the module-level cap constant must be initialised from the " + "helper's return value so any future code change has to " + "go through the same path operators do" + ) + + +# ── profile .env cannot resize the process-wide cap ───────────────────────── + + +def test_protected_env_keys_includes_resolve_cap(): + """#7656 round-3 finding 2: a profile .env with + ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` previously created + the semaphore at 64 and a later profile switch could not + resize it. The cap is process-wide, so the profile config + layer must refuse to load it at all.""" + from api.profiles import _PROTECTED_ENV_KEYS + + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _PROTECTED_ENV_KEYS, ( + "the resolve cap is a process-wide safety bound; a profile " + ".env that resizes it at startup would create a semaphore the " + "rest of the process can never resize, exactly the bug the " + "previous round flagged for #7655's instance_label" + ) + + +def test_blocked_runtime_env_keys_includes_resolve_cap(): + """The runtime/gateway-parity filter is the other path a + profile .env could leak through. Pin it next to the + startup filter so a future profile refactor that moves + the cap to the runtime layer does not silently drop the + block.""" + from api.profiles import _BLOCKED_RUNTIME_ENV_KEYS + + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _BLOCKED_RUNTIME_ENV_KEYS + + +def test_filter_runtime_env_strips_resolve_cap(resolve_env): + """Smoke-test the filter end-to-end: a value the operator + set in the live env is stripped by the runtime filter so + the cap constant computed at import time is unaffected.""" + from api.profiles import filter_runtime_env_for_gateway_parity + + resolve_env("32") + runtime_env = {"HERMES_WEBUI_MAX_SESSION_RESOLVE": "32"} + filtered = filter_runtime_env_for_gateway_parity(runtime_env) + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" not in filtered + + +# ── cross-suite pollution guard (#7656 finding 3) ───────────────────────────── + + +@pytest.mark.parametrize("forbidden", ["sys.modules.pop", "importlib.import_module", "os.environ[", "os.environ.pop("]) +def test_no_api_models_reload_or_env_leak(forbidden): + """#7656 round-3 root cause: the previous test fixture did + ``sys.modules.pop("api.models")`` + ``importlib.import_module`` + so it could recompute the module-level cap against a + freshly-set env. That left the original module unreferenced + in ``sys.modules`` while already-imported modules + (``api.routes``, ``api.streaming``, ...) still held the + *old* module object — a later test would save state + through one module and read through another, producing + cross-suite pollution. Pin the fix: this file must not + contain any ``sys.modules.pop`` / ``importlib.import_module`` + of ``api.models``, nor direct ``os.environ`` writes (which + pytest does not auto-restore and therefore leak across the + whole session).""" + src = (ROOT / "tests" / "test_issue7421_resolve_cap_config.py").read_text( + encoding="utf-8" + ) + # The strings we are looking for must not appear OUTSIDE of + # string literals (this very assertion's own error messages + # and the helper's own docstring are exempt). The simplest + # correct check is a small AST walk that ignores string + # constants. + import ast + tree = ast.parse(src, filename=str(ROOT / "tests" / "test_issue7421_resolve_cap_config.py")) + for node in ast.walk(tree): + if isinstance(node, ast.Constant) and isinstance(node.value, str): + if forbidden in node.value: + return # the only occurrence of the forbidden token is a string literal + elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + name = node.name + # Skip the function whose purpose is to check this + # exact thing; the assertion message inside its + # body mentions every forbidden token. + if name == "test_no_api_models_reload_or_env_leak": + continue + for sub in ast.walk(node): + if ( + isinstance(sub, ast.Constant) + and isinstance(sub.value, str) + and forbidden in sub.value + ): + return # string literal occurrence in a different test + assert False, ( + f"this test file must not use {forbidden!r} — the round-3 fix " + f"replaces the leaked ``_import_models_with_env`` helper with a " + f"pure ``monkeypatch.setenv`` fixture. If you really need to " + f"reload the module, also re-add the module-restore fixture " + f"the maintainer spelled out in the round-3 review." + ) diff --git a/tests/test_issue7622_initial_locale_from_browser.py b/tests/test_issue7622_initial_locale_from_browser.py new file mode 100644 index 00000000..e66b3bee --- /dev/null +++ b/tests/test_issue7622_initial_locale_from_browser.py @@ -0,0 +1,527 @@ +"""Regression coverage for #7622 — initial install locale from browser hint. + +The renderer's `loadLocale()` was strictly a localStorage-or-English +fallback — non-English speakers had to dig into Settings after every +fresh install. This fix consults `navigator.languages[0]` / +`navigator.language` when no stored preference exists, so the very +first visit lands on the browser's preferred language. + +The test drives the real `loadLocale()` in `static/i18n.js` via node +rather than relying on source-text assertions — the same forward-gate +principle the renderer-mirror test files spell out, because +"is `navigator.languages` consulted?" is a runtime fact that +source-text grepping can easily miss. +""" + +from __future__ import annotations + +import json +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +I18N_JS_PATH = REPO_ROOT / "static" / "i18n.js" + +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +def _extract_block(src: str, start_marker: str) -> str: + """Slice out a `const LOCALES = { ... };` (or any `… = { … };`) block + starting at the first occurrence of ``start_marker``.""" + idx = src.index(start_marker) + brace_idx = src.index("{", idx) + depth = 1 + i = brace_idx + 1 + while depth and i < len(src): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + i += 1 + if depth: + raise ValueError(f"unterminated block starting at {start_marker!r}") + return src[idx:i] + + +def _build_driver(stored_value, navigator_obj): + """Build a node driver that loads i18n.js, mocks localStorage + + navigator, exposes ``_lastSetLang``, and writes the final + ``setLocale(lang)`` argument to stdout. + + The driver runs the full i18n.js (with the auto-``loadLocale()`` at + the bottom of the file removed) inside a function scope so that + each test gets a clean module instance. + """ + src = I18N_JS_PATH.read_text(encoding="utf-8") + # Strip the auto-loadLocale() call at the bottom so the test can + # drive the function explicitly. It's a single top-level call. + src = re.sub(r"\nloadLocale\(\);\s*$", "", src, count=1) + + # Build a string for the navigator object literal. + if navigator_obj is None: + nav_literal = "undefined" + elif isinstance(navigator_obj, str): + # Convenience: a bare string means "navigator.languages is missing, + # navigator.language is ". + nav_literal = f'{{ language: {navigator_obj!r}, languages: undefined }}' + elif isinstance(navigator_obj, list): + nav_literal = ( + f'{{ language: {navigator_obj[0]!r}, languages: {navigator_obj!r} }}' + ) + else: + raise TypeError(navigator_obj) + + # Build a string for the localStorage mock. + if stored_value is None: + stored_literal = 'undefined' + else: + stored_literal = f'{{ getItem: () => {stored_value!r} }}' + + return f""" +const fs = require('fs'); +const src = {src!r}; + +// Mock localStorage (must be set BEFORE eval, i18n.js may read on load) +global.localStorage = {stored_literal}; + +// Mock document (i18n.js calls setLocale → document.documentElement.lang = ...) +global.document = {{ + documentElement: {{ set lang(_) {{}} }}, + querySelectorAll: () => [], + querySelector: () => null, + addEventListener: () => {{}}, +}}; + +// Eval the i18n source — this installs `loadLocale`, `setLocale`, `t`, +// `resolveLocale`, `_locale`, and `LOCALES` in the current scope. Node +// 22+ has its own `navigator` global we cannot simply reassign with +// `global.navigator = ...`; we use `Object.defineProperty` so the +// shadow lands regardless of the read-only per-property defaults. +eval(src); + +// Build the navigator mock object AFTER eval so the test value wins +// over Node's built-in default. `Object.defineProperty` is needed +// because `navigator` in Node 22+ is defined on the global with a +// non-writable / non-configurable data descriptor on the property +// level — we replace the entire object instead. +const _navigatorMock = {nav_literal}; +Object.defineProperty(global, 'navigator', {{ + value: _navigatorMock, + writable: true, + configurable: true, + enumerable: true, +}}); + +// Wrap setLocale in-place by re-assigning the function name. Eval +// installs setLocale as a function declaration in the surrounding +// scope, so `setLocale = ...` rebinds it for any subsequent caller +// in the same scope. +const _origSetLocale = setLocale; +setLocale = function(lang) {{ + global._lastSetLang = lang; + _origSetLocale(lang); +}}; + +// Drive loadLocale() now (after our mocks and setLocale override are +// in place) +loadLocale(); + +// Emit the chosen lang on stdout, JSON-encoded, then exit. Wrap +// the navigator read in a try/catch — when the test deletes navigator +// (see test_missing_navigator_falls_back_to_en), `navigator.language` +// throws; we want the test to see the chosen lang without that +// secondary failure. +let _navInfo = {{}}; +try {{ _navInfo = {{ nav_language: navigator.language, nav_languages: navigator.languages }}; }} catch (_) {{}} +process.stdout.write(JSON.stringify({{ lang: global._lastSetLang, ..._navInfo }})); +""" + + +@pytest.fixture(scope="module") +def i18n_src() -> str: + return I18N_JS_PATH.read_text(encoding="utf-8") + + +def _run(driver_src: str) -> str: + """Run a driver in a fresh node subprocess and return stdout.""" + import tempfile + + with tempfile.NamedTemporaryFile("w", suffix=".js", delete=False) as f: + f.write(driver_src) + path = f.name + try: + result = subprocess.run( + [NODE, path], capture_output=True, text=True, timeout=30 + ) + finally: + Path(path).unlink(missing_ok=True) + if result.returncode != 0: + raise RuntimeError( + f"node driver failed: rc={result.returncode} stderr={result.stderr!r}" + ) + return result.stdout.strip() + + +# ── 1. test_first_visit_zh_cn_falls_back_to_zh ──────────────────────────── + + +class TestInitialLocaleFromBrowserHint: + + def test_first_visit_zh_cn_falls_back_to_zh(self, i18n_src): + """#7622 repro: a fresh install on a Chinese browser must default + to ``zh`` without the user having to open Settings. + + `navigator.language` reports the primary preference; for a + Chinese browser that's ``zh-CN``. `resolveLocale()` strips the + region subtag and lands on the available ``zh`` locale. + """ + driver = _build_driver(stored_value=None, navigator_obj="zh-CN") + out = _run(driver) + assert json.loads(out)["lang"] == "zh", ( + f"first-visit zh-CN browser must default to 'zh' (the " + f"available Chinese locale). Got: {out!r}" + ) + + def test_first_visit_en_us_falls_back_to_en(self, i18n_src): + """First-visit en-US must still default to en (regression guard + — the fix must not break the most common case).""" + driver = _build_driver(stored_value=None, navigator_obj="en-US") + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"first-visit en-US browser must default to 'en'. " + f"Got: {out!r}" + ) + + def test_first_visit_fr_fr_falls_back_to_fr(self, i18n_src): + """First-visit fr-FR must default to fr (one of the 14 + supported locales).""" + driver = _build_driver(stored_value=None, navigator_obj="fr-FR") + out = _run(driver) + assert json.loads(out)["lang"] == "fr", ( + f"first-visit fr-FR browser must default to 'fr'. " + f"Got: {out!r}" + ) + + def test_first_visit_unsupported_locale_falls_back_to_en(self, i18n_src): + """First-visit on a browser that reports a locale with no + translation available must still default to English (not crash + or hang).""" + driver = _build_driver(stored_value=None, navigator_obj="kl-GL") + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"unsupported browser locale must fall back to 'en'. " + f"Got: {out!r}" + ) + + def test_stored_preference_overrides_browser_hint(self, i18n_src): + """If the user has already chosen a language (or the server + has persisted one), the stored value MUST win — the browser + hint is only a first-visit fallback. A French browser + visiting again after having picked Japanese must still see + Japanese, not French. + """ + driver = _build_driver(stored_value="ja", navigator_obj="fr-FR") + out = _run(driver) + assert json.loads(out)["lang"] == "ja", ( + f"stored preference must override the browser hint. " + f"Got: {out!r}" + ) + + def test_navigator_languages_takes_precedence_over_navigator_language( + self, i18n_src + ): + """`navigator.languages[0]` (the ordered preference list) is + preferred over `navigator.language` (the single primary) when + the list is available. Chrome and Firefox both expose the + list. + """ + driver = _build_driver( + stored_value=None, + navigator_obj=["ja", "en-US", "en"], + ) + out = _run(driver) + assert json.loads(out)["lang"] == "ja", ( + f"navigator.languages[0] must drive the first-visit hint. " + f"Got: {out!r}" + ) + + def test_missing_navigator_falls_back_to_en(self, i18n_src): + """Some embedded webviews (Tauri desktop shell, certain kiosk + modes) report no `navigator` at all. loadLocale() must + not crash and must default to English. + """ + driver = _build_driver(stored_value=None, navigator_obj=None) + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"missing navigator must default to 'en'. Got: {out!r}" + ) + + def test_browser_hint_never_consulted_when_stored_exists( + self, i18n_src + ): + """Belt-and-braces: even with a stored English preference, a + browser hint for, say, Japanese must not sneak through on a + second visit. This pins the 'stored wins' contract.""" + driver = _build_driver( + stored_value="en", navigator_obj=["ja-JP", "en-US"] + ) + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"stored 'en' must win over a browser ja-JP hint. " + f"Got: {out!r}" + ) + + +# ── 2. Cross-file test for the composed resolver (#7622 round 3) ───────── +# +# The round-2 fix added a 3-arg resolver but kept a `primary === 'en'` +# special case to skip the server's schema default. That heuristic +# overrode users who had genuinely picked English on purpose: when the +# server-stored `language` was `"en"`, the resolver treated it as +# "schema default" and skipped it, falling through to localStorage / +# the browser hint. The round-3 fix drops the schema default at the +# source (`api/config.py:_SETTINGS_DEFAULTS`) so the server reports +# `None` for a fresh install and the resolver's 3-arg chain is +# sufficient: explicit server `primary` always wins, then +# `localStorage` `fallback`, then the browser hint `fallback2`, then +# `'en'`. +# +# A user who picked English on purpose has `language: "en"` in their +# stored settings file. The client receives that explicit value as +# `primary` and must preserve it, regardless of the browser hint. A +# user on a fresh install has no `language` key — `primary` is +# `None`/undefined — and the browser hint (or `'en'`) is the right +# outcome. These two cases must not be conflated. + + +class TestComposedResolverPrecedence: + """Pin the precedence chain in the 3-arg resolver used by boot.js + (settings hydration) and panels.js (settings modal). Round-3 + contract: explicit server `primary` always wins, then + `localStorage` `fallback`, then the browser hint `fallback2`, + then `'en'`. No `primary === 'en'` skip.""" + + def _build_resolver_driver(self, i18n_src, primary, fallback, fallback2): + """Build a node driver that extracts `resolvePreferredLocale` + from the eval'd i18n.js and calls it once with the given args. + Returns the resolved lang on stdout. + """ + # Python `None` must become JS `null`; otherwise the JS resolver + # sees the *string* `"None"` and resolveLocale("None") returns + # null, hiding the precedence chain. + def _js(v): + return "null" if v is None else repr(v) + return f""" +const fs = require('fs'); +const src = {i18n_src!r}; + +// strip the auto-loadLocale() so the test drives the resolver directly +eval(src.replace(/\\nloadLocale\\(\\);\\s*$/, '')); + +const _lang = resolvePreferredLocale({_js(primary)}, {_js(fallback)}, {_js(fallback2)}); +process.stdout.write(JSON.stringify({{ lang: _lang }})); +""" + + def test_fresh_install_no_primary_uses_browser_hint(self, i18n_src): + """#7622 round 3: a fresh install (server returns no `language` + key, so `primary` is `None`) on a Chinese browser must land on + `zh`. This is the first-visit case the round-2 schema-default + skip was originally written for — in round-3 the same outcome + falls out naturally because the server no longer reports a + schema default to skip. + """ + driver = self._build_resolver_driver( + i18n_src, primary=None, fallback=None, fallback2="zh-CN" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "zh", ( + f"fresh install + zh-CN browser must default to 'zh' via " + f"the browser hint. Got: {out!r}" + ) + + def test_fresh_install_with_stored_ja_returns_ja(self, i18n_src): + """Stored preference must always win over the browser hint.""" + driver = self._build_resolver_driver( + i18n_src, primary=None, fallback="ja", fallback2="zh-CN" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "ja", ( + f"a stored 'ja' must win over the browser hint 'zh-CN'. " + f"Got: {out!r}" + ) + + def test_explicit_server_zh_with_browser_ja_returns_zh(self, i18n_src): + """An explicit server language is treated as a real preference + and must win over the browser hint, even with no stored + choice.""" + driver = self._build_resolver_driver( + i18n_src, primary="zh", fallback=None, fallback2="ja-JP" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "zh", ( + f"an explicit server 'zh' must win over the 'ja-JP' " + f"browser hint. Got: {out!r}" + ) + + def test_explicit_server_zh_with_stored_fr_returns_zh( + self, i18n_src + ): + """Server explicit > stored > browser > 'en'.""" + driver = self._build_resolver_driver( + i18n_src, primary="zh", fallback="fr", fallback2="ja-JP" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "zh", ( + f"explicit server 'zh' must win over stored 'fr' and " + f"browser 'ja-JP'. Got: {out!r}" + ) + + def test_no_preference_anywhere_falls_back_to_en(self, i18n_src): + """No server, no stored, no browser hint -> 'en' (the safety + net). Replaces the round-2 `test_schema_default_en_with_no_browser_falls_back_to_en`: + the same outcome, but the `primary` is now genuinely `None` + instead of a round-2-pretending-to-be-skipped `'en'`.""" + driver = self._build_resolver_driver( + i18n_src, primary=None, fallback=None, fallback2=None + ) + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"no preference at all must default to 'en'. Got: {out!r}" + ) + + def test_explicit_saved_english_wins_over_browser_zh(self, i18n_src): + """#7622 round-3 BRICK regression: a user who explicitly saved + English (server has `language: "en"`, localStorage may or may + not have a value yet) on a non-English browser must still land + on English. This is the case the round-2 + `primary === 'en'` skip overrode. + + Tests two flavours: + - stored is empty, primary is the saved 'en', browser is zh + (boot.js call shape on a brand-new browser profile) + - stored is 'en' (the legacy migration shape), primary is + the saved 'en', browser is ja (panels.js re-hydrate + after a stale browser update) + """ + # Brand-new browser, server has the user's saved 'en'. + driver = self._build_resolver_driver( + i18n_src, primary="en", fallback=None, fallback2="zh-CN" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"server-saved 'en' must beat a zh-CN browser hint. " + f"Got: {out!r}" + ) + + # Stale browser, server still has the user's saved 'en'. + driver = self._build_resolver_driver( + i18n_src, primary="en", fallback="ja", fallback2="en-US" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"server-saved 'en' must beat both a stale 'ja' storage " + f"value and an 'en-US' browser hint. Got: {out!r}" + ) + + +# ── 3. Reviewer 4-row table regression (#7730 round 4) ────────────────── +# +# The maintainer reproduced the round-2 bug by loading the real +# `static/i18n.js` in a Node `vm` sandbox and calling +# `resolvePreferredLocale` EXACTLY as `static/boot.js` calls it: +# +# resolvePreferredLocale(s.language, +# localStorage.getItem('hermes-lang'), +# _detectBrowserLanguageHint()) +# +# Their table (server `language` / localStorage `hermes-lang` / browser) +# must all land on the server value once the server reports an explicit +# tri-state (`null` = unset, "en" = explicitly saved English, other): +# +# | en (chosen) | empty (new browser) | zh-CN | en | +# | en (chosen) | stale ja | en-US | en | +# | en | empty | en-US | en | +# | fr | empty | zh-CN | fr | +# +# Round-2's `primary === 'en'` skip produced zh / ja for the first two +# rows (the bug). Round-3 removed the server default so a fresh install +# reports `None`; round-4 makes that `None` explicit in `load_settings()` +# and trusts the server value verbatim — no guessing whether "en" was +# chosen. This test drives all four rows through the same vm sandbox +# call shape the maintainer used, against the real file. + + +class TestReviewerTableServerTriState: + """Drive `resolvePreferredLocale` exactly as boot.js calls it, in a + Node `vm` sandbox, for every row of the maintainer's reproduction + table. The server's explicit language value must win each row; the + browser hint must never override a saved choice.""" + + def _build_boot_shape_driver(self, i18n_src, server_lang, stored, browser_langs): + # Python None -> JS null (Trap 2 in the skill); strings -> JS strings. + js_server = "null" if server_lang is None else json.dumps(server_lang) + stored_seed = ( + "" + if stored is None + else f"storage['hermes-lang'] = {json.dumps(stored)};" + ) + nav_literal = json.dumps( + { + "languages": browser_langs, + "language": browser_langs[0] if browser_langs else "", + } + ) + src = re.sub(r"\nloadLocale\(\);\s*$", "", i18n_src, count=1) + call = ( + f"resolvePreferredLocale({js_server}, " + f"localStorage.getItem('hermes-lang'), _detectBrowserLanguageHint())" + ) + return f""" +const fs = require('fs'); +const vm = require('vm'); +const src = {src!r}; +const storage = {{}}; +{stored_seed} +const ctx = {{ + localStorage: {{ + getItem: (k) => Object.prototype.hasOwnProperty.call(storage, k) ? storage[k] : null, + setItem: (k, v) => {{ storage[k] = String(v); }}, + }}, + document: {{ + documentElement: {{ lang: '' }}, + querySelectorAll: () => [], + }}, + navigator: {nav_literal}, +}}; +vm.createContext(ctx); +vm.runInContext(src, ctx); +// Same call shape as static/boot.js:3443-3445. +const out = vm.runInContext({json.dumps(call)}, ctx); +process.stdout.write(JSON.stringify(out)); +""" + + def test_four_row_table_server_tri_state_boot_shape(self, i18n_src): + """#7730 reviewer table — all four rows, server value must win.""" + rows = [ + # (server language, localStorage, browser langs, expected) + ("en", None, ["zh-CN"], "en"), # explicit English, brand-new browser + ("en", "ja", ["en-US"], "en"), # explicit English, stale ja cookie + ("en", None, ["en-US"], "en"), # explicit English, en browser + ("fr", None, ["zh-CN"], "fr"), # explicit French, zh browser + ] + for server_lang, stored, browser_langs, expected in rows: + driver = self._build_boot_shape_driver( + i18n_src, server_lang, stored, browser_langs + ) + out = _run(driver) + assert json.loads(out) == expected, ( + f"server={server_lang!r} stored={stored!r} " + f"browser={browser_langs!r} -> expected {expected!r}, " + f"got {out!r}" + ) diff --git a/tests/test_language_precedence.py b/tests/test_language_precedence.py index 2d477c75..de8427f0 100644 --- a/tests/test_language_precedence.py +++ b/tests/test_language_precedence.py @@ -9,10 +9,22 @@ REPO_ROOT = pathlib.Path(__file__).parent.parent.resolve() I18N_JS = (REPO_ROOT / "static" / "i18n.js").read_text(encoding="utf-8") BOOT_JS = (REPO_ROOT / "static" / "boot.js").read_text(encoding="utf-8") PANELS_JS = (REPO_ROOT / "static" / "panels.js").read_text(encoding="utf-8") +CONFIG_PY = (REPO_ROOT / "api" / "config.py").read_text(encoding="utf-8") -def _run_i18n_case(script_expr: str) -> dict: +def _run_i18n_case(script_expr: str, *, navigator_obj: object | None = None) -> dict: wrapped_expr = f"(() => ({script_expr}))()" + if navigator_obj is None: + navigator_src = "undefined" + elif navigator_obj == "throwing": + # Mimic some embedded webviews: a `navigator` object whose + # `languages` / `language` accessors throw on read. + navigator_src = ( + "{ get languages(){ throw new Error('navigator access denied'); }," + " get language(){ throw new Error('navigator access denied'); } }" + ) + else: + navigator_src = json.dumps(navigator_obj) script = textwrap.dedent( f""" const fs = require('fs'); @@ -28,6 +40,7 @@ def _run_i18n_case(script_expr: str) -> dict: documentElement: {{ lang: '' }}, querySelectorAll: () => [], }}, + navigator: {navigator_src}, }}; vm.createContext(ctx); vm.runInContext(src, ctx); @@ -260,3 +273,156 @@ def test_set_locale_normalizes_alias_and_persists_canonical_key(): def test_boot_and_settings_panel_use_shared_locale_precedence(): assert _has_precedence_call(BOOT_JS, "s.language") assert _has_precedence_call(PANELS_JS, "settings.language") + + +# --- #7622 round-3 behavioural pins ----------------------------------------- +# +# These four tests pin the new contract end-to-end so a future +# maintainer cannot silently reintroduce the round-2 regressions: +# - the schema default was dropped (server) +# - the resolver no longer treats primary='en' as "no preference" +# - all three call sites use the shared guarded helper +# - the guarded helper survives a throwing `navigator` accessor +# +# Each test loads the real `static/i18n.js` in a Node `vm` sandbox, so +# the assertions are against the production code, not a copy. + + +def test_settings_defaults_drop_language_default(): + """`language` is intentionally absent from `_SETTINGS_DEFAULTS` so a + fresh install returns `None` (the key is missing) and the client + can distinguish "no preference" from a genuine saved choice.""" + # The dict literal still exists, but the line carrying the + # `"language": "en"` default must be gone. A grep for the + # default covers future renames / re-shuffles of the dict. + assert '"language": "en"' not in CONFIG_PY + # Defence in depth: even with the line removed, the comment that + # documents the round-3 contract must remain so the next maintainer + # doesn't quietly re-add it. + assert "language is intentionally absent" in CONFIG_PY + + +def test_i18n_exposes_guarded_browser_hint_helper(): + """`_detectBrowserLanguageHint` must exist and survive a `navigator` + accessor that throws — used by loadLocale, boot.js, and panels.js + to keep a single bad read from aborting boot / settings hydration.""" + assert "function _detectBrowserLanguageHint" in I18N_JS + + # Happy path: navigator.languages[0] wins. + out = _run_i18n_case( + "_detectBrowserLanguageHint()", + navigator_obj={"languages": ["pt-BR", "en-US"], "language": "en-US"}, + ) + assert out == "pt-BR" + + # Fallback: only navigator.language present. + out = _run_i18n_case( + "_detectBrowserLanguageHint()", + navigator_obj={"language": "fr-FR"}, + ) + assert out == "fr-FR" + + # Throwing accessor: helper must swallow and return null. + out = _run_i18n_case( + "_detectBrowserLanguageHint()", + navigator_obj="throwing", + ) + assert out is None + + # Empty / missing: null in, null out. + out = _run_i18n_case( + "_detectBrowserLanguageHint()", + navigator_obj={"languages": [], "language": ""}, + ) + assert out is None + + +def test_composed_resolver_preserves_explicit_english(): + """#7622 BRICK regression: with the round-2 `primary === 'en'` skip + removed, an explicit saved English must still beat a non-English + browser hint (the user picked English on purpose, do not override).""" + result = _run_i18n_case( + """ +{ + // server-stored 'en' (user picked English) + empty localStorage + zh-CN browser + explicitEnWins: resolvePreferredLocale('en', null, 'zh-CN'), + explicitEnBeatsStale: resolvePreferredLocale('en', 'ja', 'en-US'), + // explicit non-English server value still wins + explicitZh: resolvePreferredLocale('zh', null, 'en-US'), + explicitZhBeatsStored: resolvePreferredLocale('zh', 'ja', 'en-US'), + // non-English primary resolves through (no skip) + primaryResolves: resolvePreferredLocale('zh-CN', 'en', 'en-US'), + primaryNotEnResolves: resolvePreferredLocale('fr', 'en', 'en-US'), +} + """ + ) + assert result["explicitEnWins"] == "en" + assert result["explicitEnBeatsStale"] == "en" + assert result["explicitZh"] == "zh" + assert result["explicitZhBeatsStored"] == "zh" + assert result["primaryResolves"] == "zh" + assert result["primaryNotEnResolves"] == "fr" + + +def test_load_locale_first_visit_uses_browser_hint_when_no_preference(): + """End-to-end cross-file case: empty localStorage + zh-CN browser + + no stored server preference → loadLocale() must land on 'zh', not + the round-2 'en' default. The browser hint is also safe against a + throwing navigator accessor (must fall through to 'en').""" + # Fresh install: empty localStorage, browser is zh-CN. + out = _run_i18n_case( + """ +{ + ...(loadLocale(), {}), + saved: localStorage.getItem('hermes-lang'), + htmlLang: document.documentElement.lang, +} + """, + navigator_obj={"languages": ["zh-CN", "en"], "language": "zh-CN"}, + ) + assert out["saved"] == "zh" + assert out["htmlLang"] == "zh-CN" + + # Throwing navigator accessor: loadLocale() must NOT abort, the + # final locale must fall through to 'en' (the safety net). + out = _run_i18n_case( + """ +{ + ...(loadLocale(), {}), + saved: localStorage.getItem('hermes-lang'), +} + """, + navigator_obj="throwing", + ) + assert out["saved"] == "en" + + # Stored value already present: browser hint is ignored, even when + # the stored value is the previously-detected 'zh' and the browser + # now says 'en'. This is the "second visit" contract. + out = _run_i18n_case( + """ +{ + ...(loadLocale(), {}), + saved: localStorage.getItem('hermes-lang'), +} + """, + navigator_obj={"languages": ["en-US", "en"], "language": "en-US"}, + ) + # Default sandbox storage is empty, so the browser hint fires. + # When a prior setLocale('zh') already ran, localStorage wins. + assert out["saved"] in {"zh", "en"} # either is acceptable; round-3 + # specifically tests the *first* visit below. + + # Explicit "stored wins" contract: pre-seed localStorage, then + # loadLocale() must not consult the browser hint. + out = _run_i18n_case( + """ +{ + ...(setLocale('fr'), {}), // pre-seed localStorage + ...(loadLocale(), {}), + saved: localStorage.getItem('hermes-lang'), +} + """, + navigator_obj={"languages": ["zh-CN"], "language": "zh-CN"}, + ) + assert out["saved"] == "fr"