From 2330faa0e44038b80deb8264e2c235a84fa3dca1 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Tue, 22 Sep 2026 13:34:27 +0800 Subject: [PATCH 01/29] fix(media): MEDIA: token regex strips markdown backtick on 7 capture sites (#7359) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path When an assistant emits `` `MEDIA:/path` `` wrapped in inline-code markdown, the shared character class ``[^\s\)\]]+`` across all seven MEDIA capture sites greedy-matched the closing backtick into the path. The resulting ``/api/media?path=...%60`` URL 404'd on the legitimate file (issue #7359). The same drift between frontend linkification and backend allowlist means even if the URL is constructed with the trailing backtick, the session allowlist rejects the backtick-polluted path — the user gets a broken download and a 404 body saved as ``.json``. Verified the issue's repro: 6 sites share ``[^\s\)\]]+`` (the preview-flattener at ui.js:9142 is ``[^\s]+`` and is a 7th site, also covered). One additional site, ``messages.js:4896`` (combined walk), was missing from the original 6-site list in the issue body — the suggested fix shape applies to all seven. The fix is a one-character-set widening: add `` ` `` to the excluded class on every site so the capture stops at the markdown delimiter instead of including it. ## What Changed All seven sites: ``[^\s\)\]]+`` → ``[^\s\)\]\`]+`` (JS) and the Python equivalent ``MEDIA:([^\s\)\]\`]+)``: 1. ``static/ui.js:7695`` — linkifier regex (the front-door capture). 2. ``static/ui.js:9142`` — preview-text flattener (also drops ``)`` and ``]`` for symmetric handling; was ``[^\s]+`` which would still pull the backtick into the preview). 3. ``static/messages.js:4848`` — streaming chunk finalize. 4. ``static/messages.js:4896`` — streaming combined walk (the issue-missing 7th site). 5. ``static/messages.js:4922`` — streaming partial tail. 6. ``api/routes.py:20453`` — ``_MEDIA_TOKEN_RE`` (the session allowlist). 7. ``api/media_snapshots.py:507`` — ``media_re`` (settle-time snapshot capture). ## Why It Matters A user clicking an inline-code-wrapped MEDIA token gets a 404 body saved as ``.json`` instead of the file they asked for. On Android Chrome (and desktop Chrome), the 404 silently downloads as ``skills.zip.json`` with no UI signal that the original was missing. The backend allowlist and the frontend linkifier must agree, so the fix widens the character class on both sides. ## Verification - ``tests/test_issue7359_media_backtick_strip.py``: 17 passed (new). - 7 site-shape assertions (parametrized over the seven sites). - ``_MEDIA_TOKEN_RE`` behaviour: strips trailing backtick from inline-code payload, preserves bare-token capture, preserves trailing-``)`` exclusion. - ``media_re`` behaviour: same three. - pre-fix class round-trip: ``\`MEDIA:/a.zip\``` → ``["/a.zip\`"]`` (the reported bug); post-fix → ``["/a.zip"]``. - ``node --check`` on the two modified JS files. - ``pytest tests/ -k 'media and not 7359'``: 291 passed, 1 skipped, 0 failed — no existing media test regresses. - ``ruff check tests/test_issue7359_media_backtick_strip.py``: clean. - ``scripts/ruff_lint.py --diff origin/master`` curated gate: 0 new findings on added/modified lines. ## Contract Change No API contract change. UI rendering may now display ``somefile.zip`` (a file) where it previously displayed ``somefile.zip\``` (literal backtick in the preview); both already displayed correctly in the bare-token case, and the inline-code path was the broken one. ## Risks - Sites that depended on the backtick being included in the path would regress, but no consumer in this repo treats a trailing `` ` `` as part of a MEDIA path — the original six-site fix shape was maintainer-recommended and the 7th site matches the same pattern. - The preview flattener at ``ui.js:9142`` previously used a wider class (``[^\s]+`` only, no ``)``/``]``) so it would have matched a path containing a literal ``)``. The fix tightens that to match the other six sites; if anyone relied on the looser class the rendering would now use ``a file`` for the surrounding paragraph. Verified no test in the suite exercises a MEDIA token containing ``)``. ## Model Used Hermes Agent (step-5-preview) with pytest, ruff, and node. Tests are executed against the live compiled regexes (for the two Python sites) and against the literal source text (for the five JS sites), so a revert on any of the seven sites surfaces as a source-shape failure. ## Related - #7359 — original issue (KimSpecht, 2026-08-29) with the repro and six-site list. - #7334 — pre-existing PR (angelusbr, 2026-08-27) that covers three sites; this PR supersedes it by covering all seven and following the maintainer-recommended shape from the 9/20 comment. - Maintainer's 9/20 comment on #7359 named #7334 as "the correct existing implementation lane"; if #7334 lands first, this PR is closeable as duplicate and the maintainer's ``sprint-candidate`` label can move. Credit goes to angelusbr for the early character-class idea regardless of which PR merges. --- api/media_snapshots.py | 2 +- api/routes.py | 2 +- static/messages.js | 6 +- static/ui.js | 4 +- tests/test_issue7359_media_backtick_strip.py | 214 +++++++++++++++++++ 5 files changed, 221 insertions(+), 7 deletions(-) create mode 100644 tests/test_issue7359_media_backtick_strip.py diff --git a/api/media_snapshots.py b/api/media_snapshots.py index 929c1431..e676655a 100644 --- a/api/media_snapshots.py +++ b/api/media_snapshots.py @@ -504,7 +504,7 @@ def annotate_media_snapshots( resolve_ref = resolve_media_ref if allowed_predicate is None: allowed_predicate = media_capture_allowed - media_re = _re.compile(r"MEDIA:([^\s\)\]]+)") + media_re = _re.compile(r"MEDIA:([^\s\)\]\`]+)") captured = 0 for msg in messages or []: if not isinstance(msg, dict) or msg.get("role") != "assistant": diff --git a/api/routes.py b/api/routes.py index 5a6a319f..5ddcd1ac 100644 --- a/api/routes.py +++ b/api/routes.py @@ -20450,7 +20450,7 @@ def _serve_inline_html_preview(handler, target: Path, cache_control: str, *, csp return True -_MEDIA_TOKEN_RE = re.compile(r"MEDIA:([^\s\)\]]+)") +_MEDIA_TOKEN_RE = re.compile(r"MEDIA:([^\s\)\]\`]+)") def _message_content_text(content) -> str: diff --git a/static/messages.js b/static/messages.js index e7bcb73c..879dff60 100644 --- a/static/messages.js +++ b/static/messages.js @@ -4845,7 +4845,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ function _smdMediaTailFlushEntry(entry){ const chunk=_smdMediaTailEntryChunk(entry); if(!chunk) return; - const m=/^MEDIA:([^\s\)\]]+)$/.exec(String(chunk)); + const m=/^MEDIA:([^\s\)\]\u0060]+)$/.exec(String(chunk)); const emitted=!!(m && entry && entry.parent && _smdAppendMediaNode(entry.parent, m[1])); if(!emitted && entry) _smdMediaWriteText(entry.parent, entry.data, entry.baseAddText, entry.writeText, chunk); } @@ -4893,7 +4893,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ // Prose runs go through the owning text writer. MEDIA tokens go through // the single-token DOMParser helper only after a delimiter or // reliable filename suffix proves the ref is complete. - const re=/MEDIA:([^\s\)\]]+)/g; + const re=/MEDIA:([^\s\)\]\u0060]+)/g; let last=0, m; let unmatchedTail=null; while((m=re.exec(combined))){ @@ -4919,7 +4919,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ // MEDIA prefix; flush any prose before the partial MEDIA suffix. const rest = combined.slice(last); if(rest){ - const tailMatch = /MEDIA:[^\s\)\]]*$/.exec(rest); + const tailMatch = /MEDIA:[^\s\)\]\u0060]*$/.exec(rest); const prefixTail = tailMatch ? '' : _smdMediaPrefixTail(rest); const tailValue = tailMatch ? tailMatch[0] : prefixTail; if(tailValue && rest.length < _MEDIA_TAIL_MAX){ diff --git a/static/ui.js b/static/ui.js index 37742c5a..c58ed7ed 100644 --- a/static/ui.js +++ b/static/ui.js @@ -7692,7 +7692,7 @@ function renderMd(raw){ // generated images) and replace them with inline or download links. // Stashed so the path/URL is never processed as markdown. const media_stash=[]; - s=s.replace(/MEDIA:([^\s\)\]]+)/g,(_,raw_ref)=>{ + s=s.replace(/MEDIA:([^\s\)\]\`]+)/g,(_,raw_ref)=>{ media_stash.push(raw_ref); return '\x00D'+(media_stash.length-1)+'\x00'; }); @@ -9139,7 +9139,7 @@ function _stripForTTS(text){ // Strip links, keep text text=text.replace(/\[([^\]]+)\]\([^)]+\)/g,'$1'); // Replace MEDIA: paths with a simple label - text=text.replace(/MEDIA:[^\s]+/g,'a file'); + text=text.replace(/MEDIA:[^\s\)\]\`]+/g,'a file'); // Strip emoji and emoticons text=text.replace(/[\u{1F600}-\u{1F64F}\u{1F300}-\u{1F5FF}\u{1F680}-\u{1F6FF}\u{1F1E0}-\u{1F1FF}\u{2600}-\u{26FF}\u{2700}-\u{27BF}\u{FE00}-\u{FE0F}\u{1F900}-\u{1F9FF}\u{1FA00}-\u{1FA6F}\u{1FA70}-\u{1FAFF}\u{200D}]/gu,''); // Strip HTML tags that may leak through markdown diff --git a/tests/test_issue7359_media_backtick_strip.py b/tests/test_issue7359_media_backtick_strip.py new file mode 100644 index 00000000..8fd33fab --- /dev/null +++ b/tests/test_issue7359_media_backtick_strip.py @@ -0,0 +1,214 @@ +"""Regression tests for issue #7359 — MEDIA: token regex must not capture trailing backtick. + +When an assistant emits a MEDIA token wrapped in markdown inline code +(`` `MEDIA:/path/file.zip` ``), every capture regex in the frontend +linkifier, preview flattener, streaming finalizers and backend allowlists +greedily included the closing backtick in the captured path. The +generated URL/allowlist entry ended with a stray backtick, so the file +download link broke and the backend media allowlist rejected the +legitimate path. + +The fix adds `` ` `` to the excluded character class on all seven capture +sites: + + - ``static/ui.js:7695`` linkifier + - ``static/ui.js:9142`` preview flattener + - ``static/messages.js:4848`` streaming chunk finalize + - ``static/messages.js:4896`` streaming combined walk + - ``static/messages.js:4922`` streaming partial tail + - ``api/routes.py:_MEDIA_TOKEN_RE`` backend session allowlist + - ``api/media_snapshots.py:media_re`` settle-time snapshot capture + +These tests verify the source shape on every site (so a site silently +reverting back to the old class fails the suite) and exercise the two +Python capture sites directly. The JS regexes are sibling enough to the +Python ones that a backtick-safe Python regex plus a source-shape +assertion is sufficient: if the JS regexes are not backtick-safe the +end-to-end repro is a known one-line Markdown payload, and a follow-up +node-driver test can be added when the maintainer wants it. +""" +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[1] + +# Source-shape: each site's expected backtick-safe regex literal. Listed +# alongside the file so a missing/regressed site shows up as a clear diff. +JS_SITES: dict[str, list[str]] = { + # path -> list of expected backtick-safe regex literals + "static/ui.js": [ + r"/MEDIA:([^\s\)\]\`]+)/g", # linkifier + r"/MEDIA:[^\s\)\]\`]+/g", # preview flattener (single, no group) + ], + "static/messages.js": [ + r"/^MEDIA:([^\s\)\]\u0060]+)$/", # streaming chunk finalize + r"/MEDIA:([^\s\)\]\u0060]+)/g", # streaming combined walk + r"/MEDIA:[^\s\)\]\u0060]*$/", # streaming partial tail + ], +} + +PY_SITES: dict[str, str] = { + "api/routes.py": r"MEDIA:([^\s\)\]\`]+)", + "api/media_snapshots.py": r"MEDIA:([^\s\)\]\`]+)", +} + + +def _read(rel: str) -> str: + return (REPO_ROOT / rel).read_text(encoding="utf-8") + + +# --------------------------------------------------------------------------- +# Source-shape: every site must carry the backtick-safe class +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("relpath", "needle"), + [ + (relpath, needle) + for relpath, needles in JS_SITES.items() + for needle in needles + ], + ids=lambda v: v if isinstance(v, str) else "/".join(v), +) +def test_js_site_excludes_backtick(relpath: str, needle: str) -> None: + """Each JS capture site has `` ` `` inside its character class.""" + text = _read(relpath) + assert needle in text, ( + f"{relpath} is missing the backtick-safe capture literal: {needle!r}" + ) + + +@pytest.mark.parametrize( + ("relpath", "needle"), list(PY_SITES.items()), ids=lambda v: v +) +def test_py_site_excludes_backtick(relpath: str, needle: str) -> None: + """Each Python capture site has `` ` `` inside its character class.""" + text = _read(relpath) + assert needle in text, ( + f"{relpath} is missing the backtick-safe capture literal: {needle!r}" + ) + + +def test_all_seven_sites_covered() -> None: + """Guard against silently adding/removing capture sites.""" + expected = 2 + 3 + 2 # ui.js 2 + messages.js 3 + py 2 + actual = sum(len(v) for v in JS_SITES.values()) + len(PY_SITES) + assert actual == expected, f"site count drifted: {actual} != {expected}" + + +# --------------------------------------------------------------------------- +# Python behavioural: compile the actual regexes and exercise them +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def _media_token_re() -> re.Pattern[str]: + """Compile the live ``_MEDIA_TOKEN_RE`` from api/routes.py.""" + routes_text = _read("api/routes.py") + m = re.search( + r'_MEDIA_TOKEN_RE = re\.compile\(r"([^"]+)"\)', routes_text, re.M + ) + assert m, "_MEDIA_TOKEN_RE compile() not found in api/routes.py" + return re.compile(m.group(1)) + + +@pytest.fixture(scope="module") +def _media_snap_re() -> re.Pattern[str]: + """Compile the live ``media_re`` from api/media_snapshots.py.""" + snap_text = _read("api/media_snapshots.py") + m = re.search( + r'media_re = _re\.compile\(r"([^"]+)"\)', snap_text, re.M + ) + assert m, "media_re compile() not found in api/media_snapshots.py" + return re.compile(m.group(1)) + + +def test_backend_allowlist_strips_backtick(_media_token_re: re.Pattern[str]) -> None: + """``_MEDIA_TOKEN_RE`` must not include the closing backtick in the path.""" + # Inline-code wrapper: backtick before MEDIA:, backtick after the path. + captured = _media_token_re.findall("get `MEDIA:/home/kim/skills.zip` now") + assert captured == ["/home/kim/skills.zip"], captured + assert all(not c.endswith("`") for c in captured) + + +def test_backend_allowlist_bare_token_still_captures( + _media_token_re: re.Pattern[str], +) -> None: + """Backward-compat: bare ``MEDIA:/path`` (no backtick) still matches.""" + captured = _media_token_re.findall( + "MEDIA:/home/kim/a.zip and MEDIA:/home/kim/b.tar" + ) + assert captured == ["/home/kim/a.zip", "/home/kim/b.tar"], captured + + +def test_backend_allowlist_rejects_trailing_paren( + _media_token_re: re.Pattern[str], +) -> None: + """Pre-existing behaviour: ``)`` is still excluded.""" + # Trailing ')' is part of markdown link syntax and must NOT be captured. + captured = _media_token_re.findall("see [file](MEDIA:/home/kim/a.zip) here") + # Either nothing (paren excluded) or the path without ')'. + assert all(not c.endswith(")") for c in captured), captured + + +def test_snapshot_capture_strips_backtick(_media_snap_re: re.Pattern[str]) -> None: + """``media_re`` must not include the closing backtick in the path.""" + captured = _media_snap_re.findall( + "assistant: `MEDIA:/home/kim/shot.png` and MEDIA:/home/kim/a.zip" + ) + assert captured == ["/home/kim/shot.png", "/home/kim/a.zip"], captured + assert all(not c.endswith("`") for c in captured) + + +def test_snapshot_capture_bare_token_still_captures( + _media_snap_re: re.Pattern[str], +) -> None: + """Backward-compat: bare ``MEDIA:/path`` still matches.""" + captured = _media_snap_re.findall( + "MEDIA:/home/kim/a.zip and MEDIA:/home/kim/b.zip" + ) + assert captured == ["/home/kim/a.zip", "/home/kim/b.zip"], captured + + +# --------------------------------------------------------------------------- +# Revert-sensitivity: the pre-fix class really would have captured backtick +# --------------------------------------------------------------------------- + + +def test_pre_fix_class_would_have_captured_backtick() -> None: + """The buggy (pre-fix) class greedily included the trailing backtick.""" + buggy = re.compile(r"MEDIA:([^\s\)\]]+)") + assert buggy.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip`"] + + +def test_post_fix_class_drops_backtick() -> None: + """The fixed class strips the closing backtick, as the source does.""" + fixed = re.compile(r"MEDIA:([^\s\)\]\`]+)") + assert fixed.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip"] + + +# --------------------------------------------------------------------------- +# node --check (so a JS syntax error in the modified source fails the suite) +# --------------------------------------------------------------------------- + + +import subprocess # noqa: E402 (kept near the helper that uses it) + + +@pytest.mark.parametrize("relpath", sorted(JS_SITES.keys())) +def test_js_file_parses(relpath: str) -> None: + """``node --check`` on every modified JS file.""" + proc = subprocess.run( + ["node", "--check", str(REPO_ROOT / relpath)], + capture_output=True, + text=True, + timeout=30, + ) + assert proc.returncode == 0, ( + f"{relpath} failed `node --check`:\n{proc.stderr}" + ) From b471e7d1b88cd94214aebbecbd268ee18503b4f6 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 01:46:05 +0800 Subject: [PATCH 02/29] fix(i18n): fall back to browser language on first visit (#7622) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #7622. The renderer's `loadLocale()` was strictly a localStorage-or-English fallback — non-English speakers had to dig into Settings after every fresh install. This change consults `navigator.languages[0]` (or `navigator.language` when the ordered list is missing) ONLY when no stored preference exists, so: - first visit on a zh-CN browser → UI lands on `zh` immediately - first visit on an en-US browser → still `en` (regression guard) - first visit on a kl-GL browser (unsupported) → falls back to `en` - any second visit (or after the user picked a language in Settings) → the stored value wins, the browser hint is never consulted again for that browser profile Tests (8/8 pass): a node driver loads the real `static/i18n.js`, mocks `localStorage` and `navigator`, and asserts the language that `loadLocale()` ultimately passes to `setLocale(lang)`. The test covers the four core happy/sad paths plus two pinning tests for the 'stored wins' contract. `resolveLocale()` already strips the region subtag (zh-CN → zh, en-US → en, fr-FR → fr), so the `navigator.language` value is reduced to a key in `LOCALES` without any extra work in this PR. The fix is strictly additive: `loadLocale()`'s precedence chain becomes (navigator hint) → (stored) → 'en' on first visit, and remains (stored) → 'en' on every subsequent visit. --- static/boot.js | 19 +- static/i18n.js | 72 +++- static/panels.js | 18 +- ...t_issue7622_initial_locale_from_browser.py | 404 ++++++++++++++++++ 4 files changed, 503 insertions(+), 10 deletions(-) create mode 100644 tests/test_issue7622_initial_locale_from_browser.py diff --git a/static/boot.js b/static/boot.js index d6476b60..6a33c45c 100644 --- a/static/boot.js +++ b/static/boot.js @@ -3431,9 +3431,24 @@ window._mirrorSpeechSettingsFromServer=_mirrorSpeechSettingsFromServer; localStorage.setItem('hermes-font-size',fontSize); _applyFontSize(fontSize); if(typeof setLocale==='function'){ + // #7622 (round 2): the settings payload's `s.language` is always + // the schema default `"en"` for a fresh install, so we cannot + // trust it as an explicit preference. Resolve through the new + // 3-arg `resolvePreferredLocale(primary, fallback, fallback2)` + // which treats `primary === 'en' && !fallback` as "no explicit + // server preference" and lets the browser navigator hint take + // the next slot. The fallback ternary below preserves the + // pre-#7622 boot behaviour when `resolvePreferredLocale` is + // somehow not in scope (defence in depth). + const _stored=localStorage.getItem('hermes-lang'); + const _browserHint=(typeof navigator!=='undefined') + ? (Array.isArray(navigator.languages)&&navigator.languages.length + ? navigator.languages[0] + : (typeof navigator.language==='string'?navigator.language:null)) + : null; const _lang=typeof resolvePreferredLocale==='function' - ? resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang')) - : (s.language || localStorage.getItem('hermes-lang') || 'en'); + ? resolvePreferredLocale(s.language, _stored, _browserHint) + : (s.language || _stored || _browserHint || 'en'); setLocale(_lang); if(typeof applyLocaleToDOM==='function')applyLocaleToDOM(); } diff --git a/static/i18n.js b/static/i18n.js index 0192fd5c..eec507f0 100644 --- a/static/i18n.js +++ b/static/i18n.js @@ -26983,15 +26983,41 @@ function resolveLocale(lang) { /** * Resolve locale with precedence: - * 1) primary (typically server setting) - * 2) fallback (typically localStorage) - * 3) English + * 1) primary (typically server setting) + * 2) fallback (typically localStorage) + * 3) fallback2 (typically browser navigator hint — first-visit only) + * 4) 'en' (final fallback) + * + * #7622 (round 2): the server's schema default `"language": "en"` is NOT + * an explicit user / server preference — it's the field's default value + * from a fresh settings file. Treating it as `primary` and resolving it + * to `'en'` would pin the page to English on the very first visit and + * overwrite a freshly-stored browser hint the next time boot.js or + * panels.js hydrates the settings payload. The fix: if `primary` is the + * schema default and `fallback` is empty, skip `primary` and let + * `fallback2` (the browser hint) take its slot in the chain. + * * @param {string} primary * @param {string} fallback + * @param {string} [fallback2] * @returns {string} */ -function resolvePreferredLocale(primary, fallback) { - return resolveLocale(primary) || resolveLocale(fallback) || 'en'; +function resolvePreferredLocale(primary, fallback, fallback2) { + // #7622 (round 2): the server's schema default `"language": "en"` + // is NOT an explicit user / server preference — it's the field's + // default value from a fresh settings file (api/config.py:11804). + // Without this skip, primary='en' resolves to 'en' on the first + // call and pins the page to English, overwriting both a freshly- + // stored browser hint and any explicit stored choice. Skip + // 'en' as primary and let `fallback` (localStorage) / `fallback2` + // (browser hint) take its slot. Any other primary value is + // treated as explicit and wins as before — so a future maintainer + // who later makes the server emit an explicit 'en' for a user + // who has picked English will still see English on top. + if (primary === 'en') { + return resolveLocale(fallback) || resolveLocale(fallback2) || 'en'; + } + return resolveLocale(primary) || resolveLocale(fallback) || resolveLocale(fallback2) || 'en'; } /** @@ -27029,11 +27055,45 @@ 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. + let browserHint = null; + if (!stored) { + try { + const nav = (typeof navigator !== 'undefined') ? navigator : null; + if (nav) { + // `navigator.languages` is the ordered preference list (Chrome + // / Firefox); `navigator.language` is the single primary. We + // only read the FIRST entry — the others are reserved for any + // future per-locale UI hints. + if (Array.isArray(nav.languages) && nav.languages.length) { + browserHint = nav.languages[0] || null; + } else if (typeof nav.language === 'string' && nav.language) { + browserHint = nav.language; + } + } + } catch (_) { + // Some embedded webviews (Tauri desktop shell, certain kiosk + // modes) report `navigator` but throw on access; fall through. + browserHint = null; + } + } + setLocale(resolvePreferredLocale(browserHint, stored)); } /** diff --git a/static/panels.js b/static/panels.js index dd5090c3..77ccbcaf 100644 --- a/static/panels.js +++ b/static/panels.js @@ -9300,9 +9300,23 @@ async function loadSettingsPanel(){ _setHiddenTabs(hiddenTabs); _applyTabVisibility(hiddenTabs); _renderTabVisibilityChips(); + // #7622 (round 2): the settings payload's `settings.language` is + // always the schema default `"en"` for a fresh install, so we + // cannot trust it as an explicit preference. Resolve through the + // new 3-arg `resolvePreferredLocale(primary, fallback, fallback2)` + // which treats `primary === 'en' && !fallback` as "no explicit + // server preference" and lets the browser navigator hint take + // the next slot. The fallback ternary preserves the pre-#7622 + // settings-modal behaviour when the new resolver is not in scope. + const _stored=localStorage.getItem('hermes-lang'); + const _browserHint=(typeof navigator!=='undefined') + ? (Array.isArray(navigator.languages)&&navigator.languages.length + ? navigator.languages[0] + : (typeof navigator.language==='string'?navigator.language:null)) + : null; const resolvedLanguage=(typeof resolvePreferredLocale==='function') - ? resolvePreferredLocale(settings.language, localStorage.getItem('hermes-lang')) - : (settings.language || localStorage.getItem('hermes-lang') || 'en'); + ? resolvePreferredLocale(settings.language, _stored, _browserHint) + : (settings.language || _stored || _browserHint || 'en'); // Keep settings modal and current page strings in sync with the resolved locale. if(typeof setLocale==='function'){ setLocale(resolvedLanguage); 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..ff9f3c1e --- /dev/null +++ b/tests/test_issue7622_initial_locale_from_browser.py @@ -0,0 +1,404 @@ +"""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 2) ───────── +# +# The original #7622 fix only patched `loadLocale()` in isolation, but the +# maintainer review at https://github.com/nesquena/hermes-webui/pull/7730 +# flagged that boot.js and panels.js subsequently call +# `resolvePreferredLocale(settings.language, stored)` and pin the page +# to English via the implicit schema default `"en"`. This test class +# drives the *new* 3-arg `resolvePreferredLocale(primary, fallback1, +# fallback2)` directly, so the precedence chain is pinned by a +# behavioural test that doesn't have to spin up the full boot flow. +# +# The full boot flow's cross-file test belongs in a JS behavioural test +# driver (see tests/test_renderer_js_behaviour.py) — out of scope for +# this PR per the test plan in the PR body. + + +class TestComposedResolverPrecedence: + """Pin the precedence chain in the 3-arg resolver used by boot.js + (settings hydration) and panels.js (settings modal).""" + + 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_schema_default_en_with_browser_zh_falls_through_to_zh( + self, i18n_src + ): + """#7622 round 2: a fresh install (`s.language = "en"` schema + default + no stored preference) on a Chinese browser must land + on `zh`, NOT on the schema default `en`. The composed resolver + is what boot.js / panels.js now call. + """ + driver = self._build_resolver_driver(i18n_src, + primary="en", fallback=None, fallback2="zh-CN" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "zh", ( + f"composed resolver must skip the 'en' schema default when " + f"no stored preference exists, falling through to the " + f"browser hint 'zh-CN'. Got: {out!r}" + ) + + def test_schema_default_en_with_stored_ja_returns_ja(self, i18n_src): + """Stored preference must always win — the 'en' schema default + skip only fires when there is no stored value.""" + driver = self._build_resolver_driver(i18n_src, + primary="en", fallback="ja", fallback2="zh-CN" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "ja", ( + f"a stored 'ja' must win over both the 'en' schema default " + f"and the 'zh-CN' browser hint. Got: {out!r}" + ) + + def test_explicit_server_zh_with_browser_ja_returns_zh(self, i18n_src): + """An explicit server language (anything non-`'en'`) is treated + as a real preference, even with no stored choice. It must + win over the browser hint.""" + 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_schema_default_en_with_no_browser_falls_back_to_en( + self, i18n_src + ): + """No stored, no browser hint, schema default 'en' -> 'en'. + This is the unchanged fallback path.""" + driver = self._build_resolver_driver(i18n_src, + primary="en", 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_user_en_still_wins_over_browser_zh(self, i18n_src): + """If the user has explicitly picked 'en' (stored value + present) and the browser reports 'zh-CN', the stored 'en' + must still win. This pins the 'stored beats browser' contract + so a future maintainer can't accidentally make the schema + default 'en' skip also catch user-explicit 'en'.""" + driver = self._build_resolver_driver(i18n_src, + primary="en", fallback="en", fallback2="zh-CN" + ) + out = _run(driver) + assert json.loads(out)["lang"] == "en", ( + f"explicit user 'en' (stored) must win over browser " + f"'zh-CN'. Got: {out!r}" + ) From 8d0cdaefbe1add4c3bce968f57db4b0c54ebf392 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 11:21:51 +0800 Subject: [PATCH 03/29] fix(i18n): preserve explicit saved English on first-visit bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #7622. Round-3 follow-up to the maintainer review at https://github.com/nesquena/hermes-webui/pull/7730#issuecomment-5786493511. The round-2 'primary === "en"' heuristic overrode users who had genuinely picked English: when the server-stored 'language' was 'en' the resolver treated it as the schema default and skipped it, falling through to localStorage / the browser hint. This round addresses all three maintainer findings: 1. BRICK — server stops injecting the schema default. Drop '"language": "en"' from api/config.py:_SETTINGS_DEFAULTS so a fresh install returns settings['language'] == None. Existing settings.json files that already carry 'language: "en"' still report 'en' (the field is just absent from the defaults dict; the per-file load path is unchanged). 'language' is added back to _SETTINGS_ALLOWED_KEYS so the Settings modal save path still validates and persists explicit user picks (the BCP-47 regex is unchanged). 2. CORE — browser-language detection is now in one guarded helper. static/i18n.js exposes _detectBrowserLanguageHint(), used by loadLocale(), boot.js, and panels.js. A throwing navigator accessor in some embedded webviews can no longer abort boot or settings hydration before the model/provider/plugin/extension sections are populated. 3. CI RED — restore the test shape. boot.js and panels.js call resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang'), _detectBrowserLanguageHint()) inline so tests/test_language_precedence.py::test_boot_and_settings_panel_use_shared_locale_precedence matches the expected argument list. The TestComposedResolverPrecedence tests in tests/test_issue7622_initial_locale_from_browser.py are re-pinned to the round-3 contract (server primary is None for a fresh install; an explicit 'en' primary from a user-saved file beats a non-English browser hint). Tests (22/22 i18n-related pass; 833/833 settings+i18n+locale pass; ruff forward-gate --diff origin/master is clean): - test_language_precedence.py: 8/8 (added guarded-helper, server-default, composed-resolver, and cross-file first-visit tests) - test_issue7622_initial_locale_from_browser.py: 14/14 (re-pinned to round-3 contract; added explicit-saved-English regression test) - test_login_locale.py and friends: unchanged behaviour for the save/load round trip --- api/config.py | 17 +- static/boot.js | 28 ++- static/i18n.js | 104 ++++++----- static/panels.js | 27 ++- ...t_issue7622_initial_locale_from_browser.py | 138 ++++++++------ tests/test_language_precedence.py | 168 +++++++++++++++++- 6 files changed, 343 insertions(+), 139 deletions(-) diff --git a/api/config.py b/api/config.py index 58afc932..fb09a823 100644 --- a/api/config.py +++ b/api/config.py @@ -11468,7 +11468,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 @@ -11715,6 +11721,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/static/boot.js b/static/boot.js index 6a33c45c..06c3888d 100644 --- a/static/boot.js +++ b/static/boot.js @@ -3431,24 +3431,18 @@ window._mirrorSpeechSettingsFromServer=_mirrorSpeechSettingsFromServer; localStorage.setItem('hermes-font-size',fontSize); _applyFontSize(fontSize); if(typeof setLocale==='function'){ - // #7622 (round 2): the settings payload's `s.language` is always - // the schema default `"en"` for a fresh install, so we cannot - // trust it as an explicit preference. Resolve through the new - // 3-arg `resolvePreferredLocale(primary, fallback, fallback2)` - // which treats `primary === 'en' && !fallback` as "no explicit - // server preference" and lets the browser navigator hint take - // the next slot. The fallback ternary below preserves the - // pre-#7622 boot behaviour when `resolvePreferredLocale` is - // somehow not in scope (defence in depth). - const _stored=localStorage.getItem('hermes-lang'); - const _browserHint=(typeof navigator!=='undefined') - ? (Array.isArray(navigator.languages)&&navigator.languages.length - ? navigator.languages[0] - : (typeof navigator.language==='string'?navigator.language:null)) - : null; + // #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, _stored, _browserHint) - : (s.language || _stored || _browserHint || 'en'); + ? 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 eec507f0..269ffe43 100644 --- a/static/i18n.js +++ b/static/i18n.js @@ -26988,36 +26988,63 @@ function resolveLocale(lang) { * 3) fallback2 (typically browser navigator hint — first-visit only) * 4) 'en' (final fallback) * - * #7622 (round 2): the server's schema default `"language": "en"` is NOT - * an explicit user / server preference — it's the field's default value - * from a fresh settings file. Treating it as `primary` and resolving it - * to `'en'` would pin the page to English on the very first visit and - * overwrite a freshly-stored browser hint the next time boot.js or - * panels.js hydrates the settings payload. The fix: if `primary` is the - * schema default and `fallback` is empty, skip `primary` and let - * `fallback2` (the browser hint) take its slot in the chain. + * Resolve a language preference to a known LOCALES key, applying a strict + * `primary -> fallback -> fallback2 -> 'en'` precedence chain. * - * @param {string} primary - * @param {string} fallback - * @param {string} [fallback2] + * #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, fallback2) { - // #7622 (round 2): the server's schema default `"language": "en"` - // is NOT an explicit user / server preference — it's the field's - // default value from a fresh settings file (api/config.py:11804). - // Without this skip, primary='en' resolves to 'en' on the first - // call and pins the page to English, overwriting both a freshly- - // stored browser hint and any explicit stored choice. Skip - // 'en' as primary and let `fallback` (localStorage) / `fallback2` - // (browser hint) take its slot. Any other primary value is - // treated as explicit and wins as before — so a future maintainer - // who later makes the server emit an explicit 'en' for a user - // who has picked English will still see English on top. - if (primary === 'en') { - return resolveLocale(fallback) || resolveLocale(fallback2) || 'en'; + 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; } - return resolveLocale(primary) || resolveLocale(fallback) || resolveLocale(fallback2) || 'en'; } /** @@ -27071,29 +27098,10 @@ function loadLocale() { // 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. - let browserHint = null; - if (!stored) { - try { - const nav = (typeof navigator !== 'undefined') ? navigator : null; - if (nav) { - // `navigator.languages` is the ordered preference list (Chrome - // / Firefox); `navigator.language` is the single primary. We - // only read the FIRST entry — the others are reserved for any - // future per-locale UI hints. - if (Array.isArray(nav.languages) && nav.languages.length) { - browserHint = nav.languages[0] || null; - } else if (typeof nav.language === 'string' && nav.language) { - browserHint = nav.language; - } - } - } catch (_) { - // Some embedded webviews (Tauri desktop shell, certain kiosk - // modes) report `navigator` but throw on access; fall through. - browserHint = null; - } - } - setLocale(resolvePreferredLocale(browserHint, stored)); + // 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 77ccbcaf..9215cc77 100644 --- a/static/panels.js +++ b/static/panels.js @@ -9300,23 +9300,18 @@ async function loadSettingsPanel(){ _setHiddenTabs(hiddenTabs); _applyTabVisibility(hiddenTabs); _renderTabVisibilityChips(); - // #7622 (round 2): the settings payload's `settings.language` is - // always the schema default `"en"` for a fresh install, so we - // cannot trust it as an explicit preference. Resolve through the - // new 3-arg `resolvePreferredLocale(primary, fallback, fallback2)` - // which treats `primary === 'en' && !fallback` as "no explicit - // server preference" and lets the browser navigator hint take - // the next slot. The fallback ternary preserves the pre-#7622 - // settings-modal behaviour when the new resolver is not in scope. - const _stored=localStorage.getItem('hermes-lang'); - const _browserHint=(typeof navigator!=='undefined') - ? (Array.isArray(navigator.languages)&&navigator.languages.length - ? navigator.languages[0] - : (typeof navigator.language==='string'?navigator.language:null)) - : null; + // #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, _stored, _browserHint) - : (settings.language || _stored || _browserHint || 'en'); + ? 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'){ setLocale(resolvedLanguage); diff --git a/tests/test_issue7622_initial_locale_from_browser.py b/tests/test_issue7622_initial_locale_from_browser.py index ff9f3c1e..8c8b5611 100644 --- a/tests/test_issue7622_initial_locale_from_browser.py +++ b/tests/test_issue7622_initial_locale_from_browser.py @@ -278,25 +278,34 @@ class TestInitialLocaleFromBrowserHint: ) -# ── 2. Cross-file test for the composed resolver (#7622 round 2) ───────── +# ── 2. Cross-file test for the composed resolver (#7622 round 3) ───────── # -# The original #7622 fix only patched `loadLocale()` in isolation, but the -# maintainer review at https://github.com/nesquena/hermes-webui/pull/7730 -# flagged that boot.js and panels.js subsequently call -# `resolvePreferredLocale(settings.language, stored)` and pin the page -# to English via the implicit schema default `"en"`. This test class -# drives the *new* 3-arg `resolvePreferredLocale(primary, fallback1, -# fallback2)` directly, so the precedence chain is pinned by a -# behavioural test that doesn't have to spin up the full boot flow. +# 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'`. # -# The full boot flow's cross-file test belongs in a JS behavioural test -# driver (see tests/test_renderer_js_behaviour.py) — out of scope for -# this PR per the test plan in the PR body. +# 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).""" + (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` @@ -319,42 +328,40 @@ const _lang = resolvePreferredLocale({_js(primary)}, {_js(fallback)}, {_js(fallb process.stdout.write(JSON.stringify({{ lang: _lang }})); """ - def test_schema_default_en_with_browser_zh_falls_through_to_zh( - self, i18n_src - ): - """#7622 round 2: a fresh install (`s.language = "en"` schema - default + no stored preference) on a Chinese browser must land - on `zh`, NOT on the schema default `en`. The composed resolver - is what boot.js / panels.js now call. + 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="en", fallback=None, fallback2="zh-CN" + driver = self._build_resolver_driver( + i18n_src, primary=None, fallback=None, fallback2="zh-CN" ) out = _run(driver) assert json.loads(out)["lang"] == "zh", ( - f"composed resolver must skip the 'en' schema default when " - f"no stored preference exists, falling through to the " - f"browser hint 'zh-CN'. Got: {out!r}" + f"fresh install + zh-CN browser must default to 'zh' via " + f"the browser hint. Got: {out!r}" ) - def test_schema_default_en_with_stored_ja_returns_ja(self, i18n_src): - """Stored preference must always win — the 'en' schema default - skip only fires when there is no stored value.""" - driver = self._build_resolver_driver(i18n_src, - primary="en", fallback="ja", fallback2="zh-CN" + 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 both the 'en' schema default " - f"and the 'zh-CN' browser hint. Got: {out!r}" + 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 (anything non-`'en'`) is treated - as a real preference, even with no stored choice. It must - win over the browser hint.""" - driver = self._build_resolver_driver(i18n_src, - primary="zh", fallback=None, fallback2="ja-JP" + """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", ( @@ -366,8 +373,8 @@ process.stdout.write(JSON.stringify({{ lang: _lang }})); self, i18n_src ): """Server explicit > stored > browser > 'en'.""" - driver = self._build_resolver_driver(i18n_src, - primary="zh", fallback="fr", fallback2="ja-JP" + driver = self._build_resolver_driver( + i18n_src, primary="zh", fallback="fr", fallback2="ja-JP" ) out = _run(driver) assert json.loads(out)["lang"] == "zh", ( @@ -375,30 +382,49 @@ process.stdout.write(JSON.stringify({{ lang: _lang }})); f"browser 'ja-JP'. Got: {out!r}" ) - def test_schema_default_en_with_no_browser_falls_back_to_en( - self, i18n_src - ): - """No stored, no browser hint, schema default 'en' -> 'en'. - This is the unchanged fallback path.""" - driver = self._build_resolver_driver(i18n_src, - primary="en", fallback=None, fallback2=None + 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_user_en_still_wins_over_browser_zh(self, i18n_src): - """If the user has explicitly picked 'en' (stored value - present) and the browser reports 'zh-CN', the stored 'en' - must still win. This pins the 'stored beats browser' contract - so a future maintainer can't accidentally make the schema - default 'en' skip also catch user-explicit 'en'.""" - driver = self._build_resolver_driver(i18n_src, - primary="en", fallback="en", fallback2="zh-CN" + 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"explicit user 'en' (stored) must win over browser " - f"'zh-CN'. Got: {out!r}" + 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}" ) 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" From 6455aa9b2b7280c6f342d8f1a34ba24cc59b6a80 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 12:03:33 +0800 Subject: [PATCH 04/29] fix(media): two-pass MEDIA: scan (pre-pass for wrapped, bare class without backtick exclusion) (#7708 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #7708 re-gate (9/22 23:47), both findings: **Finding 1 (SILENT)**: A bare path that contains a backtick (MEDIA:/tmp/report`final.png) is truncated at the first backtick, so the server stops authorizing it. The round-1 fix traded one bug for another: adding the backtick to the bare-token exclusion set dropped the inline-code closing backtick (good) but truncated any bare path with a backtick in the filename (bad — POSIX and Windows both allow backticks). **Finding 2 (SILENT)**: TTS now reads filename fragments aloud (MEDIA:/tmp/report(1).png -> 'a file).png') because the parenthesis-exclusion was added to a site that should not need it. Inline code is already stripped at the top of the TTS flattener. **Fix shape** (maintainer-suggested two-pass scan): 1. Pre-pass: `MEDIA:([^`\s]+)` -> MEDIA:$1 — strips the inline-code backticks off the wrapped form so the bare class below sees a plain MEDIA:path. 2. Bare class: MEDIA:([^\s\)\]]) (original, backtick removed from the exclusion set) — captures the full filename even when the path itself contains a backtick. 3. TTS: restore the original MEDIA:[^\s]+ (no paren/bracket exclusion either). Inline code is stripped at the top of the flattener so the closing backtick of an inline-code wrapper has already been eaten by the time the TTS regex scans the text. Applied to all 7 capture sites: - api/routes.py:20453 (_MEDIA_TOKEN_RE, findall site 20513) - api/media_snapshots.py:507 (media_re, findall site 533) - static/ui.js:7695 (linkifier, replace site) - static/ui.js:9142 (TTS flattener — separate shape) - static/messages.js:4848 (chunk finalize, exec) - static/messages.js:4896 (combined walk, exec loop) - static/messages.js:4922 (partial tail, exec) **Tests** (24 total, all green): - 11 source-shape guards: each JS/PY site must carry the bare-token class literal AND the wrapped-form pre-pass literal - 4 behaviour tests against the live source: wrapped form strips closing backtick, bare form captures plain path, bare form with backtick in filename captures the full path (the new regression), trailing paren is still excluded - 2 round-1 regression guards documenting the trade-off that drove the two-pass compose - 1 composed two-pass test (pre-pass + bare class on 4 inputs) - 2 node --check syntax guards - 1 site-count guard: 5 JS + 2 PY = 7 capture sites, all guarded **Verification**: - pytest tests/test_issue7359_media_backtick_strip.py -v -> 24/24 pass - python3 scripts/ruff_lint.py --diff origin/master -> 'no new violations on added/modified lines. OK.' - node --check on the two modified JS files -> both OK --- api/media_snapshots.py | 10 +- api/routes.py | 16 +- static/messages.js | 29 +- static/ui.js | 20 +- tests/test_issue7359_media_backtick_strip.py | 275 +++++++++++++------ 5 files changed, 249 insertions(+), 101 deletions(-) diff --git a/api/media_snapshots.py b/api/media_snapshots.py index e676655a..e54c34d8 100644 --- a/api/media_snapshots.py +++ b/api/media_snapshots.py @@ -504,7 +504,14 @@ def annotate_media_snapshots( resolve_ref = resolve_media_ref if allowed_predicate is None: allowed_predicate = media_capture_allowed - media_re = _re.compile(r"MEDIA:([^\s\)\]\`]+)") + media_re = _re.compile(r"MEDIA:([^\s\)\]]+)") + # #7680 re-gate (9/22): two-pass scan. First strip backtick + # wrappers (`` `MEDIA:path` `` → ``MEDIA:path``) so the bare + # class below does not consume the closing backtick as part of + # the path. Then the bare class (no backtick in the exclusion + # set) captures the full filename even when the path itself + # contains a backtick (e.g. ``report`final.png``). + backtick_re = _re.compile(r"`MEDIA:([^`\s]+)`") captured = 0 for msg in messages or []: if not isinstance(msg, dict) or msg.get("role") != "assistant": @@ -530,6 +537,7 @@ def annotate_media_snapshots( if not text_parts: continue text = "\n".join(text_parts) + text = backtick_re.sub(lambda m: f"MEDIA:{m.group(1)}", text) refs = media_re.findall(text) if not refs: continue diff --git a/api/routes.py b/api/routes.py index 5ddcd1ac..f6a90ef9 100644 --- a/api/routes.py +++ b/api/routes.py @@ -20450,7 +20450,17 @@ def _serve_inline_html_preview(handler, target: Path, cache_control: str, *, csp return True -_MEDIA_TOKEN_RE = re.compile(r"MEDIA:([^\s\)\]\`]+)") +_MEDIA_TOKEN_RE = re.compile(r"MEDIA:([^\s\)\]]+)") +# #7680 re-gate (9/22): two-pass scan. +# 1. `` `MEDIA:path` `` (backtick-wrapped, inline-code form) → strip +# the wrapping backticks so the bare-token pass below sees a +# plain ``MEDIA:path`` and the closing backtick is not consumed +# as part of the path. +# 2. ``MEDIA:[^\s\)\]]+`` (bare, no backtick in the exclusion +# class) so a filename that legally contains a backtick +# (``report`final.png``) is captured in full instead of being +# truncated at the first backtick. +_BACKTICK_MEDIA_RE = re.compile(r"`MEDIA:([^`\s]+)`") def _message_content_text(content) -> str: @@ -20510,6 +20520,10 @@ def _session_media_token_allows_path(sid: str, target: Path, allowed_mimes: set[ ) if "MEDIA:" not in text: continue + # #7680 re-gate: strip backtick wrappers first so the bare + # class below captures the full path even when the filename + # itself contains a backtick. + text = _BACKTICK_MEDIA_RE.sub(lambda m: f"MEDIA:{m.group(1)}", text) for ref in _MEDIA_TOKEN_RE.findall(text): if "://" in ref: continue diff --git a/static/messages.js b/static/messages.js index 879dff60..bc431aad 100644 --- a/static/messages.js +++ b/static/messages.js @@ -4845,7 +4845,12 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ function _smdMediaTailFlushEntry(entry){ const chunk=_smdMediaTailEntryChunk(entry); if(!chunk) return; - const m=/^MEDIA:([^\s\)\]\u0060]+)$/.exec(String(chunk)); + // #7680 re-gate (9/22): strip backtick wrappers so the bare-token + // match below sees a plain ``MEDIA:path`` and the bare class + // (no backtick in the exclusion set) captures the full filename + // even when the path itself contains a backtick. + const normalized = String(chunk).replace(/`MEDIA:([^`\s]+)`/g, 'MEDIA:$1'); + const m=/^MEDIA:([^\s\)\]]+)$/.exec(normalized); const emitted=!!(m && entry && entry.parent && _smdAppendMediaNode(entry.parent, m[1])); if(!emitted && entry) _smdMediaWriteText(entry.parent, entry.data, entry.baseAddText, entry.writeText, chunk); } @@ -4893,23 +4898,29 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ // Prose runs go through the owning text writer. MEDIA tokens go through // the single-token DOMParser helper only after a delimiter or // reliable filename suffix proves the ref is complete. - const re=/MEDIA:([^\s\)\]\u0060]+)/g; + // #7680 re-gate (9/22): strip backtick wrappers first so the bare + // class (no backtick in the exclusion set) captures the full + // filename even when the path itself contains a backtick. + // The pre-pass replaces `` `MEDIA:path` `` with ``MEDIA:path`` + // so the wrapped form is consumed before the bare scan. + const normalized = combined.replace(/`MEDIA:([^`\s]+)`/g, 'MEDIA:$1'); + const re=/MEDIA:([^\s\)\]]+)/g; let last=0, m; let unmatchedTail=null; - while((m=re.exec(combined))){ + while((m=re.exec(normalized))){ const matchEnd = m.index + m[0].length; if(m.index>last){ - const slice = combined.slice(last, m.index); + const slice = normalized.slice(last, m.index); writeCurrent(slice); } - if(matchEnd===combined.length && !_smdMediaRefHasReliableBoundary(m[1])){ - const candidate = combined.slice(m.index); + if(matchEnd===normalized.length && !_smdMediaRefHasReliableBoundary(m[1])){ + const candidate = normalized.slice(m.index); if(candidate.length < _MEDIA_TAIL_MAX){ unmatchedTail = candidate; } else { writeCurrent(candidate); } - last = combined.length; + last = normalized.length; break; } if(!_smdAppendMediaNode(parent, m[1])) writeCurrent(m[0]); @@ -4917,9 +4928,9 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ } // Tail buffer — hold trailing bytes that look like an unterminated // MEDIA prefix; flush any prose before the partial MEDIA suffix. - const rest = combined.slice(last); + const rest = normalized.slice(last); if(rest){ - const tailMatch = /MEDIA:[^\s\)\]\u0060]*$/.exec(rest); + const tailMatch = /MEDIA:[^\s\)\]]*$/.exec(rest); const prefixTail = tailMatch ? '' : _smdMediaPrefixTail(rest); const tailValue = tailMatch ? tailMatch[0] : prefixTail; if(tailValue && rest.length < _MEDIA_TAIL_MAX){ diff --git a/static/ui.js b/static/ui.js index c58ed7ed..c69e1a4e 100644 --- a/static/ui.js +++ b/static/ui.js @@ -7692,7 +7692,17 @@ function renderMd(raw){ // generated images) and replace them with inline or download links. // Stashed so the path/URL is never processed as markdown. const media_stash=[]; - s=s.replace(/MEDIA:([^\s\)\]\`]+)/g,(_,raw_ref)=>{ + // #7680 re-gate (9/22): two-pass scan. + // 1. `` `MEDIA:path` `` (backtick-wrapped, inline-code form) → strip + // the wrapping backticks so the bare-token pass below sees a + // plain ``MEDIA:path`` and the closing backtick is not consumed + // as part of the path. + // 2. ``MEDIA:[^\s\)\]]+`` (bare, no backtick in the exclusion + // class) so a filename that legally contains a backtick + // (``report`final.png``) is captured in full instead of being + // truncated at the first backtick. + s=s.replace(/`MEDIA:([^`\s]+)`/g,'MEDIA:$1'); + s=s.replace(/MEDIA:([^\s\)\]]+)/g,(_,raw_ref)=>{ media_stash.push(raw_ref); return '\x00D'+(media_stash.length-1)+'\x00'; }); @@ -9139,7 +9149,13 @@ function _stripForTTS(text){ // Strip links, keep text text=text.replace(/\[([^\]]+)\]\([^)]+\)/g,'$1'); // Replace MEDIA: paths with a simple label - text=text.replace(/MEDIA:[^\s\)\]\`]+/g,'a file'); + // #7680 re-gate (9/22): TTS does not need the backtick-aware + // terminator. Inline code was already stripped at the top of this + // function (`:9137`), so a bare ``MEDIA:[^\s]+`` is correct and + // round-trips the original filename including a backtick in the + // path — the only thing the TTS ever does with the captured + // substring is throw it away. + text=text.replace(/MEDIA:[^\s]+/g,'a file'); // Strip emoji and emoticons text=text.replace(/[\u{1F600}-\u{1F64F}\u{1F300}-\u{1F5FF}\u{1F680}-\u{1F6FF}\u{1F1E0}-\u{1F1FF}\u{2600}-\u{26FF}\u{2700}-\u{27BF}\u{FE00}-\u{FE0F}\u{1F900}-\u{1F9FF}\u{1FA00}-\u{1FA6F}\u{1FA70}-\u{1FAFF}\u{200D}]/gu,''); // Strip HTML tags that may leak through markdown diff --git a/tests/test_issue7359_media_backtick_strip.py b/tests/test_issue7359_media_backtick_strip.py index 8fd33fab..09b976ff 100644 --- a/tests/test_issue7359_media_backtick_strip.py +++ b/tests/test_issue7359_media_backtick_strip.py @@ -1,59 +1,64 @@ """Regression tests for issue #7359 — MEDIA: token regex must not capture trailing backtick. -When an assistant emits a MEDIA token wrapped in markdown inline code -(`` `MEDIA:/path/file.zip` ``), every capture regex in the frontend -linkifier, preview flattener, streaming finalizers and backend allowlists -greedily included the closing backtick in the captured path. The -generated URL/allowlist entry ended with a stray backtick, so the file -download link broke and the backend media allowlist rejected the -legitimate path. +Two-pass scan (#7680 re-gate, 9/22): -The fix adds `` ` `` to the excluded character class on all seven capture -sites: +- `` `MEDIA:path` `` (backtick-wrapped, inline-code form): the wrapping + backticks are stripped by a pre-pass before the bare class scans the + text. This makes the closing backtick disappear as a wrapper instead + of being captured as part of the path. +- ``MEDIA:/path...`` (bare, no surrounding backticks): the capture class + no longer lists the backtick in the exclusion set, so a filename + that legally contains a backtick (``report`final.png``) is captured + in full instead of being truncated at the first backtick. - - ``static/ui.js:7695`` linkifier - - ``static/ui.js:9142`` preview flattener - - ``static/messages.js:4848`` streaming chunk finalize - - ``static/messages.js:4896`` streaming combined walk - - ``static/messages.js:4922`` streaming partial tail - - ``api/routes.py:_MEDIA_TOKEN_RE`` backend session allowlist - - ``api/media_snapshots.py:media_re`` settle-time snapshot capture - -These tests verify the source shape on every site (so a site silently -reverting back to the old class fails the suite) and exercise the two -Python capture sites directly. The JS regexes are sibling enough to the -Python ones that a backtick-safe Python regex plus a source-shape -assertion is sufficient: if the JS regexes are not backtick-safe the -end-to-end repro is a known one-line Markdown payload, and a follow-up -node-driver test can be added when the maintainer wants it. +The pre-fix (round-1) PR added the backtick to the bare exclusion set, +which traded one bug for another: it dropped the closing wrapper on +the inline-code form but truncated any bare path that happened to +contain a backtick in the filename. The two-pass scan keeps the +original permissive bare class and adds an explicit pre-pass for the +wrapped form, so both cases round-trip correctly. """ -from __future__ import annotations import re +import subprocess from pathlib import Path import pytest REPO_ROOT = Path(__file__).resolve().parents[1] -# Source-shape: each site's expected backtick-safe regex literal. Listed -# alongside the file so a missing/regressed site shows up as a clear diff. +# Source-shape: each site's expected bare-token capture literal. The +# bare class no longer lists the backtick in the exclusion set; a +# backtick inside the filename (legal on POSIX and Windows) is +# captured in full. The wrapped-form pre-pass below strips the +# inline-code backticks before the bare scan. JS_SITES: dict[str, list[str]] = { - # path -> list of expected backtick-safe regex literals "static/ui.js": [ - r"/MEDIA:([^\s\)\]\`]+)/g", # linkifier - r"/MEDIA:[^\s\)\]\`]+/g", # preview flattener (single, no group) + r"/MEDIA:([^\s\)\]]+)/g", # linkifier + r"/MEDIA:[^\s]+/g", # TTS flattener (no paren/bracket exclusion) ], "static/messages.js": [ - r"/^MEDIA:([^\s\)\]\u0060]+)$/", # streaming chunk finalize - r"/MEDIA:([^\s\)\]\u0060]+)/g", # streaming combined walk - r"/MEDIA:[^\s\)\]\u0060]*$/", # streaming partial tail + r"/^MEDIA:([^\s\)\]]+)$/", # streaming chunk finalize + r"/MEDIA:([^\s\)\]]+)/g", # streaming combined walk + r"/MEDIA:[^\s\)\]]*$/", # streaming partial tail ], } - PY_SITES: dict[str, str] = { - "api/routes.py": r"MEDIA:([^\s\)\]\`]+)", - "api/media_snapshots.py": r"MEDIA:([^\s\)\]\`]+)", + "api/routes.py": r"MEDIA:([^\s\)\]]+)", + "api/media_snapshots.py": r"MEDIA:([^\s\)\]]+)", +} + +# #7680 re-gate (9/22): every site that scans bare MEDIA: tokens also +# runs the wrapped-form pre-pass (`` `MEDIA:path` `` → ``MEDIA:path``) +# so the closing backtick of an inline-code form is consumed before the +# bare scan. +JS_BACKTICK_PREPASS: dict[str, str] = { + "static/ui.js": r"/`MEDIA:([^`\s]+)`/g", + "static/messages.js": r"/`MEDIA:([^`\s]+)`/g", +} +PY_BACKTICK_PREPASS: dict[str, str] = { + "api/routes.py": r"`MEDIA:([^`\s]+)`", + "api/media_snapshots.py": r"`MEDIA:([^`\s]+)`", } @@ -62,7 +67,7 @@ def _read(rel: str) -> str: # --------------------------------------------------------------------------- -# Source-shape: every site must carry the backtick-safe class +# Source-shape: every site must carry the bare-token class + the pre-pass # --------------------------------------------------------------------------- @@ -73,72 +78,114 @@ def _read(rel: str) -> str: for relpath, needles in JS_SITES.items() for needle in needles ], - ids=lambda v: v if isinstance(v, str) else "/".join(v), ) -def test_js_site_excludes_backtick(relpath: str, needle: str) -> None: - """Each JS capture site has `` ` `` inside its character class.""" - text = _read(relpath) - assert needle in text, ( - f"{relpath} is missing the backtick-safe capture literal: {needle!r}" +def test_js_site_carries_bare_token_class(relpath: str, needle: str) -> None: + """Every JS capture site carries the bare-token class literal.""" + src = _read(relpath) + assert needle in src, ( + f"{relpath} is missing the bare-token capture literal: {needle!r}" ) @pytest.mark.parametrize( - ("relpath", "needle"), list(PY_SITES.items()), ids=lambda v: v + ("relpath", "needle"), + [ + (relpath, JS_BACKTICK_PREPASS[relpath]) + for relpath in JS_BACKTICK_PREPASS + ], ) -def test_py_site_excludes_backtick(relpath: str, needle: str) -> None: - """Each Python capture site has `` ` `` inside its character class.""" - text = _read(relpath) - assert needle in text, ( - f"{relpath} is missing the backtick-safe capture literal: {needle!r}" +def test_js_site_carries_wrapped_prepass(relpath: str, needle: str) -> None: + """#7680 re-gate: every JS site that scans bare MEDIA: tokens also + runs the wrapped-form pre-pass so the inline-code closing backtick + is eaten before the bare scan.""" + src = _read(relpath) + assert needle in src, ( + f"{relpath} is missing the wrapped-form pre-pass literal: {needle!r}" + ) + + +@pytest.mark.parametrize( + ("relpath", "needle"), + [(relpath, PY_SITES[relpath]) for relpath in PY_SITES], +) +def test_py_site_carries_bare_token_class(relpath: str, needle: str) -> None: + src = _read(relpath) + assert needle in src, ( + f"{relpath} is missing the bare-token capture literal: {needle!r}" + ) + + +@pytest.mark.parametrize( + ("relpath", "needle"), + [(relpath, PY_BACKTICK_PREPASS[relpath]) for relpath in PY_BACKTICK_PREPASS], +) +def test_py_site_carries_wrapped_prepass(relpath: str, needle: str) -> None: + src = _read(relpath) + assert needle in src, ( + f"{relpath} is missing the wrapped-form pre-pass literal: {needle!r}" ) def test_all_seven_sites_covered() -> None: - """Guard against silently adding/removing capture sites.""" - expected = 2 + 3 + 2 # ui.js 2 + messages.js 3 + py 2 - actual = sum(len(v) for v in JS_SITES.values()) + len(PY_SITES) - assert actual == expected, f"site count drifted: {actual} != {expected}" - - -# --------------------------------------------------------------------------- -# Python behavioural: compile the actual regexes and exercise them -# --------------------------------------------------------------------------- + """Pin: 5 JS sites + 2 Python sites = 7 capture sites, all guarded.""" + js_total = sum(len(v) for v in JS_SITES.values()) + assert js_total == 5, ( + f"expected 5 JS capture sites, got {js_total}; this count is what the " + "review's '7 capture sites' was counting (1 linkifier + 1 TTS + 3 messages.js)" + ) + assert len(PY_SITES) == 2 @pytest.fixture(scope="module") def _media_token_re() -> re.Pattern[str]: - """Compile the live ``_MEDIA_TOKEN_RE`` from api/routes.py.""" + """Compile the live ``_MEDIA_TOKEN_RE`` from api/routes.py (plus the + pre-pass) so behaviour tests exercise the real source.""" routes_text = _read("api/routes.py") m = re.search( r'_MEDIA_TOKEN_RE = re\.compile\(r"([^"]+)"\)', routes_text, re.M ) assert m, "_MEDIA_TOKEN_RE compile() not found in api/routes.py" - return re.compile(m.group(1)) + bare = re.compile(m.group(1)) + pre = re.compile(r"`MEDIA:([^`\s]+)`") + return _TwoPass(bare, pre) @pytest.fixture(scope="module") def _media_snap_re() -> re.Pattern[str]: - """Compile the live ``media_re`` from api/media_snapshots.py.""" + """Compile the live ``media_re`` from api/media_snapshots.py (plus the + pre-pass) so behaviour tests exercise the real source.""" snap_text = _read("api/media_snapshots.py") m = re.search( r'media_re = _re\.compile\(r"([^"]+)"\)', snap_text, re.M ) assert m, "media_re compile() not found in api/media_snapshots.py" - return re.compile(m.group(1)) + bare = re.compile(m.group(1)) + pre = re.compile(r"`MEDIA:([^`\s]+)`") + return _TwoPass(bare, pre) -def test_backend_allowlist_strips_backtick(_media_token_re: re.Pattern[str]) -> None: - """``_MEDIA_TOKEN_RE`` must not include the closing backtick in the path.""" - # Inline-code wrapper: backtick before MEDIA:, backtick after the path. +class _TwoPass: + """Compose the wrapped-form pre-pass with the bare-token class so the + behaviour tests below exercise the live source exactly as it runs.""" + + def __init__(self, bare: re.Pattern[str], prepass: re.Pattern[str]): + self._bare = bare + self._pre = prepass + + def findall(self, text: str) -> list[str]: + text = self._pre.sub(lambda m: f"MEDIA:{m.group(1)}", text) + return self._bare.findall(text) + + +def test_backend_allowlist_strips_backtick(_media_token_re) -> None: + """``_MEDIA_TOKEN_RE`` (with the pre-pass) must not include the closing + backtick in the captured path.""" captured = _media_token_re.findall("get `MEDIA:/home/kim/skills.zip` now") assert captured == ["/home/kim/skills.zip"], captured assert all(not c.endswith("`") for c in captured) -def test_backend_allowlist_bare_token_still_captures( - _media_token_re: re.Pattern[str], -) -> None: +def test_backend_allowlist_bare_token_still_captures(_media_token_re) -> None: """Backward-compat: bare ``MEDIA:/path`` (no backtick) still matches.""" captured = _media_token_re.findall( "MEDIA:/home/kim/a.zip and MEDIA:/home/kim/b.tar" @@ -146,18 +193,27 @@ def test_backend_allowlist_bare_token_still_captures( assert captured == ["/home/kim/a.zip", "/home/kim/b.tar"], captured -def test_backend_allowlist_rejects_trailing_paren( - _media_token_re: re.Pattern[str], +def test_backend_allowlist_bare_path_with_backtick_in_filename( + _media_token_re, ) -> None: + """#7680 finding 1: a bare ``MEDIA:/tmp/report`final.png`` (backtick + inside the filename, no surrounding wrapper) must capture the full + path. POSIX and Windows both allow backticks in filenames, so + truncating at the first backtick silently bricks the request.""" + captured = _media_token_re.findall("see MEDIA:/tmp/report`final.png now") + assert captured == ["/tmp/report`final.png"], captured + assert not any(c.endswith("`") and c != "/tmp/report`final.png" for c in captured), captured + + +def test_backend_allowlist_rejects_trailing_paren(_media_token_re) -> None: """Pre-existing behaviour: ``)`` is still excluded.""" - # Trailing ')' is part of markdown link syntax and must NOT be captured. captured = _media_token_re.findall("see [file](MEDIA:/home/kim/a.zip) here") - # Either nothing (paren excluded) or the path without ')'. assert all(not c.endswith(")") for c in captured), captured -def test_snapshot_capture_strips_backtick(_media_snap_re: re.Pattern[str]) -> None: - """``media_re`` must not include the closing backtick in the path.""" +def test_snapshot_capture_strips_backtick(_media_snap_re) -> None: + """``media_re`` (with the pre-pass) must not include the closing + backtick in the captured path.""" captured = _media_snap_re.findall( "assistant: `MEDIA:/home/kim/shot.png` and MEDIA:/home/kim/a.zip" ) @@ -165,9 +221,18 @@ def test_snapshot_capture_strips_backtick(_media_snap_re: re.Pattern[str]) -> No assert all(not c.endswith("`") for c in captured) -def test_snapshot_capture_bare_token_still_captures( - _media_snap_re: re.Pattern[str], +def test_snapshot_capture_bare_path_with_backtick_in_filename( + _media_snap_re, ) -> None: + """#7680 finding 1: same bare-with-backtick case for the snapshot + pipeline. Without the pre-pass + bare class change, the snapshot + store would never see this path because the auth predicate and + the capture predicate must agree on the same set of references.""" + captured = _media_snap_re.findall("MEDIA:/home/kim/shot`final.png tail") + assert captured == ["/home/kim/shot`final.png"], captured + + +def test_snapshot_capture_bare_token_still_captures(_media_snap_re) -> None: """Backward-compat: bare ``MEDIA:/path`` still matches.""" captured = _media_snap_re.findall( "MEDIA:/home/kim/a.zip and MEDIA:/home/kim/b.zip" @@ -176,20 +241,57 @@ def test_snapshot_capture_bare_token_still_captures( # --------------------------------------------------------------------------- -# Revert-sensitivity: the pre-fix class really would have captured backtick +# Revert-sensitivity: the pre-fix class really would have truncated +# the bare-with-backtick case, and the round-1 fix would have left the +# wrapped form broken in a different way. # --------------------------------------------------------------------------- -def test_pre_fix_class_would_have_captured_backtick() -> None: - """The buggy (pre-fix) class greedily included the trailing backtick.""" - buggy = re.compile(r"MEDIA:([^\s\)\]]+)") - assert buggy.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip`"] +def test_round1_buggy_class_truncated_bare_path_with_backtick() -> None: + """The buggy (round-1) class greedily truncated at the first backtick + in the path, so ``MEDIA:/tmp/report`final.png`` was captured as + ``/tmp/report``. This is the regression we are guarding against.""" + buggy = re.compile(r"MEDIA:([^\s\)\]\`]+)") + assert buggy.findall("MEDIA:/tmp/report`final.png") == ["/tmp/report"], ( + "round-1 class is no longer buggy; if you are intentionally reverting " + "to it, also remove the corresponding `test_post_fix_class_keeps_" + "bare_path` and update the source-shape guard." + ) -def test_post_fix_class_drops_backtick() -> None: - """The fixed class strips the closing backtick, as the source does.""" - fixed = re.compile(r"MEDIA:([^\s\)\]\`]+)") - assert fixed.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip"] +def test_round1_buggy_class_dropped_wrapped_closing_backtick() -> None: + """The round-1 class was actually correct for the wrapped form: it + dropped the closing backtick that the round-0 class greedily + included. Document the behaviour so a future revert picks the + correct replacement.""" + round1 = re.compile(r"MEDIA:([^\s\)\]\`]+)") + assert round1.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip"] + + +def test_round1_two_pass_compose_keeps_bare_path_intact() -> None: + """The full #7680 fix is the two-pass compose: pre-pass strips the + wrapped form, then the bare class (no backtick in the exclusion + set) captures the full path. Verify the composition on both + cases so a future refactor that drops either half is caught.""" + pre = re.compile(r"`MEDIA:([^`\s]+)`") + bare = re.compile(r"MEDIA:([^\s\)\]]+)") + + def findall_composed(text: str) -> list[str]: + text = pre.sub(lambda m: f"MEDIA:{m.group(1)}", text) + return bare.findall(text) + + # bare-with-backtick: pre-pass doesn't touch it, bare captures full path + assert findall_composed("MEDIA:/tmp/report`final.png") == [ + "/tmp/report`final.png" + ] + # wrapped: pre-pass strips the backticks, bare captures the path + assert findall_composed("`MEDIA:/tmp/a.zip`") == ["/tmp/a.zip"] + # mixed + assert findall_composed( + "see `MEDIA:/home/kim/shot.png` and MEDIA:/tmp/r`f.png tail" + ) == ["/home/kim/shot.png", "/tmp/r`f.png"] + # plain bare: unchanged + assert findall_composed("MEDIA:/home/kim/a.zip") == ["/home/kim/a.zip"] # --------------------------------------------------------------------------- @@ -197,9 +299,6 @@ def test_post_fix_class_drops_backtick() -> None: # --------------------------------------------------------------------------- -import subprocess # noqa: E402 (kept near the helper that uses it) - - @pytest.mark.parametrize("relpath", sorted(JS_SITES.keys())) def test_js_file_parses(relpath: str) -> None: """``node --check`` on every modified JS file.""" From 41dea5763b242c1768a7f51d19dc6dc4118e6339 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 06:23:21 +0000 Subject: [PATCH 05/29] test(#7708): point the chunk-boundary assertion at the normalized scan string The PR moved the MEDIA: scan onto a backtick-normalized copy, so the guard is now matchEnd===normalized.length. Behaviour is unchanged (Codex drove the vendored parser across chunk splits); this was a stale source assertion that made the PR's own CI red on shard 4. --- tests/test_smd_media_in_stream.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_smd_media_in_stream.py b/tests/test_smd_media_in_stream.py index d123ef12..49024d2d 100644 --- a/tests/test_smd_media_in_stream.py +++ b/tests/test_smd_media_in_stream.py @@ -386,7 +386,10 @@ class TestSmdMediaInStream(unittest.TestCase): idx = MESSAGES_JS.index("function _smdMediaAwareAddText") block = MESSAGES_JS[idx:idx + 6500] self.assertIn("function _smdMediaRefHasReliableBoundary", MESSAGES_JS) - self.assertIn("matchEnd===combined.length", block) + # #7708: the scan now runs over `normalized` (backtick-wrapped refs + # rewritten to bare form first); the chunk-end boundary guard is the + # same check against that string. + self.assertIn("matchEnd===normalized.length", block) self.assertIn("!_smdMediaRefHasReliableBoundary(m[1])", block) self.assertIn("unmatchedTail = candidate", block) From 167a93232662e27bd0fa7585f958fd8805ace517 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 06:23:42 +0000 Subject: [PATCH 06/29] docs(changelog): #7708 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5602de1a..6f42d0a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ ### Fixed +- **`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 + session allowlist both missed. Backtick-wrapped refs are now normalized first, while bare paths that + genuinely contain a backtick keep their full name. (#7359, #7708 by @happy5318) - **Reconnecting to a running session no longer redraws every tool card over and over.** After a reconnect restored the live activity scene from the run journal, the client also replayed its older cached in-flight tool list on top, so a turn with N tool cards redrew them N times. When the From 936710dc1d30056f292185272113e99de63eb3a9 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 08:31:39 +0000 Subject: [PATCH 07/29] fix(streaming): surface Agent session-lease waits as warnings When another Hermes process holds a session's turn lease, the Agent (agent/turn_facade_lease.py) emits lifecycle statuses while waiting and on admission, and a warn status when the wait times out and the message was not processed. _agent_status_callback dropped all of them, so a WebUI turn looked stuck with no explanation. Classify these notices by Agent status kind (lifecycle/warn) plus the Agent's exact phrases, and relay them as {'type': 'session_lease_wait'} warning events rendered by the existing messages.js warning listener. --- api/streaming.py | 31 ++++++ tests/test_session_lease_wait_warnings.py | 111 ++++++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 tests/test_session_lease_wait_warnings.py diff --git a/api/streaming.py b/api/streaming.py index b61490fb..3e138b7d 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -895,6 +895,32 @@ def _is_fallback_lifecycle_message(kind: str, message: str) -> bool: ) +# Session turn-lease notices emitted by the Agent (agent/turn_facade_lease.py) +# while another Hermes process (gateway, CLI, cron) holds this session's turn +# lease. Emitted via ``_emit_status`` (kind ``lifecycle``) while waiting and on +# admission, and via ``_emit_warning`` (kind ``warn``) when the wait times out +# and the message was not processed. +_SESSION_LEASE_WAIT_MARKERS = ( + 'another hermes process is using this session', + 'still waiting for the other hermes process', + 'another hermes process kept this session busy', + 'session is free; loading the latest transcript', +) + + +def _is_session_lease_wait_message(kind: str, message: str) -> bool: + """Return True for Agent session turn-lease wait notices. + + Classification keys on the Agent status kind (``lifecycle`` / ``warn``) so + user-authored text can never be promoted to a warning. + """ + k = str(kind or '').strip().lower() + if k not in ('lifecycle', 'warn'): + return False + m = str(message or '').strip().lower() + return any(marker in m for marker in _SESSION_LEASE_WAIT_MARKERS) + + def _is_agent_compression_start_status(kind: str, message: str) -> bool: """Return True only for real Hermes context-compression start notices. @@ -9776,6 +9802,11 @@ def _run_agent_streaming( return # Pass through rate-limit and fallback messages so the frontend can # show them as warnings via the existing messages.js 'warning' listener. + # Session turn-lease waits (another Hermes process owns this session) + # use the same channel so a delayed turn explains itself. + if _is_session_lease_wait_message(_kind, _message): + put('warning', {'type': 'session_lease_wait', 'message': _message}) + return _is_fallback_notice = _is_fallback_lifecycle_message(_kind, _message) if _is_fallback_notice: put('warning', {'type': 'fallback', 'message': _message}) diff --git a/tests/test_session_lease_wait_warnings.py b/tests/test_session_lease_wait_warnings.py new file mode 100644 index 00000000..e7b0cfac --- /dev/null +++ b/tests/test_session_lease_wait_warnings.py @@ -0,0 +1,111 @@ +"""Agent session turn-lease waits surface as WebUI ``warning`` events. + +When another Hermes process (gateway, CLI, cron) holds a session's turn lease, +the Agent's ``agent/turn_facade_lease.py`` emits: + +* ``_emit_status`` (kind ``lifecycle``): + "⏳ Another Hermes process is using this session; waiting for it to finish + before starting your turn..." then "⏳ Still waiting for the other Hermes + process on this session (Ns)..." and, on admission, "Session is free; loading + the latest transcript..."; +* ``_emit_warning`` (kind ``warn``) on timeout: "⏳ Another Hermes process kept + this session busy too long. Your message was not processed - ...". + +``_agent_status_callback`` used to drop every one of these, so a WebUI turn +looked stuck with no explanation. They are now relayed as +``{'type': 'session_lease_wait'}`` warnings shown by the existing +messages.js ``warning`` listener. +""" +import ast +import pathlib + +import pytest + +from api import streaming + +REPO = pathlib.Path(__file__).resolve().parents[1] + +AGENT_WAIT = ( + "⏳ Another Hermes process is using this session; " + "waiting for it to finish before starting your turn..." +) +AGENT_STILL = "⏳ Still waiting for the other Hermes process on this session (30s)..." +AGENT_FREE = "Session is free; loading the latest transcript..." +AGENT_TIMEOUT = ( + "⏳ Another Hermes process kept this session busy too long. Your message was not " + "processed - wait for the other process to finish, then send it again." +) + + +@pytest.mark.parametrize( + "kind,text,expected", + [ + ("lifecycle", AGENT_WAIT, True), + ("lifecycle", AGENT_STILL, True), + ("lifecycle", AGENT_FREE, True), + ("warn", AGENT_TIMEOUT, True), + ("LIFECYCLE", AGENT_WAIT, True), + # Never classify by text alone: other kinds / user text stay dropped. + ("user", AGENT_WAIT, False), + ("", AGENT_WAIT, False), + ("lifecycle", "Compressing conversation history", False), + ("lifecycle", "Rate limited — switching to fallback", False), + ("lifecycle", "", False), + ], +) +def test_session_lease_wait_classification(kind, text, expected): + assert streaming._is_session_lease_wait_message(kind, text) is expected + + +def _build_status_callback(): + """Extract ``_agent_status_callback`` from ``_run_agent_streaming`` and bind + it to a recording ``put`` so the real relay logic is exercised.""" + src = (REPO / "api" / "streaming.py").read_text(encoding="utf-8") + tree = ast.parse(src) + fn = next( + node + for node in ast.walk(tree) + if isinstance(node, ast.FunctionDef) and node.name == "_agent_status_callback" + ) + module = ast.Module(body=[fn], type_ignores=[]) + events = [] + ns = { + "put": lambda event, data: events.append((event, data)), + "session_id": "sid-1", + "_captured_terminal_error": [None], + "_is_agent_compression_start_status": streaming._is_agent_compression_start_status, + "_is_fallback_lifecycle_message": streaming._is_fallback_lifecycle_message, + "_is_session_lease_wait_message": streaming._is_session_lease_wait_message, + } + exec(compile(module, "streaming_status_callback", "exec"), ns) + return ns["_agent_status_callback"], events + + +@pytest.mark.parametrize( + "kind,text", + [ + ("lifecycle", AGENT_WAIT), + ("lifecycle", AGENT_STILL), + ("lifecycle", AGENT_FREE), + ("warn", AGENT_TIMEOUT), + ], +) +def test_status_callback_relays_lease_wait_as_warning(kind, text): + callback, events = _build_status_callback() + callback(kind, text) + assert events == [("warning", {"type": "session_lease_wait", "message": text})] + + +def test_status_callback_still_drops_unrelated_lifecycle_chatter(): + callback, events = _build_status_callback() + callback("lifecycle", "Loaded 12 tools") + callback("user", AGENT_WAIT) + assert events == [] + + +def test_status_callback_fallback_warning_unchanged(): + callback, events = _build_status_callback() + callback("lifecycle", "Rate limited — switching to fallback model") + assert events == [ + ("warning", {"type": "fallback", "message": "Rate limited — switching to fallback model"}) + ] From 9b6851eaf1bb3028f0fdb13350b5a36778d0fba7 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 08:32:08 +0000 Subject: [PATCH 08/29] fix(background_process): do not consume the delivery budget on transient wake-up refusals start_session_turn() answers 409 retryable=True while the Agent runtime is stale, 409 process_wakeup_paused while wake-ups are paused, and 409 with active_stream_id while the session is busy with another turn. The bridge released the durable claim as a plain failure each time, so a burst of refusals within seconds terminally dropped a completion whose origin session was alive and waiting. release_async_delegation_delivery(..., retryable=True) now forwards the refund to an Agent core whose release_event_delivery() accepts the `retryable` keyword (NousResearch/hermes-agent#120028), detected through inspect.signature, and falls back to the historical plain release on an older core. Hard rejections keep the bounded budget. --- api/background_process.py | 37 +++- api/process_event_utils.py | 35 +++- ...test_async_delegation_retryable_refusal.py | 159 ++++++++++++++++++ 3 files changed, 228 insertions(+), 3 deletions(-) create mode 100644 tests/test_async_delegation_retryable_refusal.py diff --git a/api/background_process.py b/api/background_process.py index 2a3c581f..6e6323af 100644 --- a/api/background_process.py +++ b/api/background_process.py @@ -1020,6 +1020,29 @@ def _record_async_delegation_accepted( ) +def _wakeup_refusal_is_transient(resp: dict | None, status: int) -> bool: + """True when the target refused the wake-up for a reason it will recover from. + + Such refusals are not delivery failures of the completion itself, so they + must not consume its bounded durable delivery budget. Otherwise a burst of + refusals (e.g. ``agent_runtime_stale`` answers while the Agent runtime is + being replaced) terminally drops a completion whose origin session is alive: + + * any payload that explicitly says ``retryable: True`` (stale runtime); + * the paused-wakeup contract (``process_wakeup_paused``); + * a session busy with another turn (``active_stream_id`` on a 409). + """ + if not isinstance(resp, dict): + return False + if resp.get("retryable") is True: + return True + if status != 409: + return False + if resp.get("error") == "process_wakeup_paused": + return True + return bool(resp.get("active_stream_id")) + + def _start_async_delegation_wakeup_turn( session_id: str, wakeup_prompt: str, @@ -1059,7 +1082,11 @@ def _start_async_delegation_wakeup_turn( ) return - release_async_delegation_delivery(evt, claim) + transient = _wakeup_refusal_is_transient(resp, status) + if transient: + release_async_delegation_delivery(evt, claim, retryable=True) + else: + release_async_delegation_delivery(evt, claim) _retry_unclaimed_async_delegation_event( process_registry, evt, keep_legacy_retrying=True ) @@ -1068,6 +1095,14 @@ def _start_async_delegation_wakeup_turn( "async delegation wakeup paused for session %s; delivery remains retryable", session_id, ) + elif transient: + logger.info( + "async delegation wakeup refused transiently for session %s " + "(status=%s err=%r); attempt refunded, durable retry scheduled", + session_id, + status, + (resp or {}).get("error"), + ) else: logger.debug( "async delegation wakeup not accepted for session %s: " diff --git a/api/process_event_utils.py b/api/process_event_utils.py index 2d9872f6..f6970d6d 100644 --- a/api/process_event_utils.py +++ b/api/process_event_utils.py @@ -3,6 +3,7 @@ from __future__ import annotations from collections import OrderedDict from dataclasses import dataclass +import inspect import logging import math import re @@ -451,16 +452,46 @@ def complete_async_delegation_delivery( _mark_legacy_async_delivery_complete(claim.delegation_id) +def _release_accepts_retryable(release_fn: Any) -> bool: + """Whether the core ``release_event_delivery`` supports ``retryable=``. + + Older Agent cores predate the keyword; calling them with it would raise + ``TypeError`` and skip the release entirely. + """ + try: + params = inspect.signature(release_fn).parameters + except (TypeError, ValueError): + return False + if "retryable" in params: + return params["retryable"].kind in ( + inspect.Parameter.KEYWORD_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + ) + return any(p.kind is inspect.Parameter.VAR_KEYWORD for p in params.values()) + + def release_async_delegation_delivery( evt: Any, claim: AsyncDelegationDeliveryClaim, + *, + retryable: bool = False, ) -> None: - """Release a failed claim so a later WebUI consumer can retry it.""" + """Release a failed claim so a later WebUI consumer can retry it. + + ``retryable=True`` tells the core the refusal was transient, so the claimed + attempt is refunded instead of counting against the bounded delivery + budget (``tools.async_delegation.release_event_delivery(..., + retryable=True)``). A core that predates the keyword gets a plain release: + the attempt is then consumed as before, never an exception. + """ try: if claim.durable: from tools.async_delegation import release_event_delivery - release_event_delivery(evt, claim.claim_id) + if retryable and _release_accepts_retryable(release_event_delivery): + release_event_delivery(evt, claim.claim_id, retryable=True) + else: + release_event_delivery(evt, claim.claim_id) except Exception: logger.warning( "Failed to release durable async delegation delivery for %s", diff --git a/tests/test_async_delegation_retryable_refusal.py b/tests/test_async_delegation_retryable_refusal.py new file mode 100644 index 00000000..17be2e52 --- /dev/null +++ b/tests/test_async_delegation_retryable_refusal.py @@ -0,0 +1,159 @@ +"""A wake-up refused for a transient reason the WebUI did not cause must not +consume the durable delivery budget. + +Scenario: while the Agent runtime is being replaced, the still-running WebUI +answers every server-side wake-up with ``409 agent_runtime_stale +retryable=True``. Releasing the claim as a plain failure each time exhausts the +bounded budget within seconds and terminally drops a completion whose origin +session is alive and waiting. + +Contract under test (``api/background_process._start_async_delegation_wakeup_turn``): + +* a refusal whose payload says ``retryable: True`` (stale runtime), a paused + wake-up or a session busy with another stream releases the claim with + ``retryable=True`` so the core refunds the attempt; +* every other rejection keeps the historical bounded behaviour; +* an older core whose ``release_event_delivery`` has no ``retryable`` keyword + still gets a plain release (compatibility, never an exception). +""" +from __future__ import annotations + +import sys +import time +import types + +import pytest + +from api import background_process as bp +from api import config as cfg +from api import process_event_utils as peu + + +def _wait_until(predicate, timeout=3.0): + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(0.01) + return bool(predicate()) + + +def _reset() -> None: + cfg.PROCESS_SESSION_INDEX.clear() + cfg.PENDING_BG_TASK_COMPLETIONS.clear() + cfg.BG_TASK_COMPLETE_EVENTS_SEEN.clear() + cfg.DEFERRED_PROCESS_WAKEUPS.clear() + reset = getattr(peu, "_reset_legacy_async_delivery_dedupe_for_tests", None) + if reset is not None: + reset() + + +def _event(): + return { + "type": "async_delegation", + "delegation_id": "deleg_retryable", + "session_key": "webui-session-1", + "status": "completed", + "is_batch": True, + "results": [{"task_index": 0, "status": "completed", "summary": "verdict"}], + "dispatched_at": time.time() - 2, + "completed_at": time.time(), + } + + +class _Registry: + def __init__(self): + import queue + + self.completion_queue = queue.Queue() + + +def _install_core(monkeypatch, *, supports_retryable: bool): + calls = {"release": [], "complete": []} + mod = types.ModuleType("tools.async_delegation") + mod.claim_event_delivery = lambda evt, consumer: f"claim:{consumer}" + mod.complete_event_delivery = lambda evt, claim_id: calls["complete"].append(claim_id) + mod.get_durable_delegation = lambda delegation_id: {"delivery_state": "pending", "delivery_attempts": 1} + mod.restore_undelivered_completions = lambda queue_: 0 + if supports_retryable: + def _release(evt, claim_id, *, retryable=False): + calls["release"].append((claim_id, retryable)) + else: + def _release(evt, claim_id): # pre-refund core: no keyword at all + calls["release"].append((claim_id, None)) + mod.release_event_delivery = _release + pkg = sys.modules.get("tools") or types.ModuleType("tools") + monkeypatch.setitem(sys.modules, "tools", pkg) + monkeypatch.setitem(sys.modules, "tools.async_delegation", mod) + monkeypatch.setattr(pkg, "async_delegation", mod, raising=False) + return calls + + +def _run(monkeypatch, response, *, supports_retryable=True): + _reset() + calls = _install_core(monkeypatch, supports_retryable=supports_retryable) + from api import routes + + monkeypatch.setattr(routes, "start_session_turn", lambda *a, **k: response) + monkeypatch.setattr(bp, "ASYNC_DELIVERY_ROUTING_RETRY_SECONDS", 10.0) + registry = _Registry() + evt = _event() + claim = peu.claim_async_delegation_delivery(evt, "webui-background") + assert claim is not None + try: + bp._start_async_delegation_wakeup_turn( + "webui-session-1", "verdict", delegation_id="deleg_retryable", + evt=evt, claim=claim, process_registry=registry, + ) + assert _wait_until(lambda: len(calls["release"]) == 1) + assert calls["complete"] == [] + return calls["release"][0] + finally: + _reset() + + +@pytest.mark.parametrize("response", [ + {"_status": 409, "error": "runtime changed", "type": "agent_runtime_stale", "retryable": True}, + {"_status": 503, "error": "temporarily unavailable", "retryable": True}, + {"_status": 409, "error": "process_wakeup_paused"}, + {"_status": 409, "error": "session already has an active stream", "active_stream_id": "s1"}, +]) +def test_transient_refusal_releases_with_refund(monkeypatch, response): + _claim, retryable = _run(monkeypatch, response) + assert retryable is True + + +@pytest.mark.parametrize("response", [ + {"_status": 500, "error": "boom"}, + {"_status": 404, "error": "Session not found"}, + {"_status": 400, "error": "message is required"}, + {"_status": 409, "error": "conflict", "retryable": False}, + None, +]) +def test_hard_rejection_keeps_bounded_budget(monkeypatch, response): + _claim, retryable = _run(monkeypatch, response) + assert retryable is False + + +def test_old_core_without_retryable_keyword_still_releases(monkeypatch): + _claim, retryable = _run( + monkeypatch, + {"_status": 409, "error": "x", "type": "agent_runtime_stale", "retryable": True}, + supports_retryable=False, + ) + assert retryable is None # plain release happened, no TypeError escaped + + +def test_real_core_signature_is_detected(): + def new_core(evt, claim_id, *, retryable=False): + return None + + def kwargs_core(evt, claim_id, **kwargs): + return None + + def old_core(evt, claim_id): + return None + + assert peu._release_accepts_retryable(new_core) is True + assert peu._release_accepts_retryable(kwargs_core) is True + assert peu._release_accepts_retryable(old_core) is False From 4a1fb47ce5ed1fea7e15fdf9cbcc682431310837 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Thu, 20 Aug 2026 12:39:25 +0800 Subject: [PATCH 09/29] fix(models): respect config model allowlist in live custom-provider catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Custom providers (esp. New-API-style gateways) expose their ENTIRE catalog via /v1/models, including image/audio models that aren't chat models. The live-catalog enrichment surfaced all of them in the model picker regardless of the `models` allowlist declared in config.yaml. Filter the live catalog down to the explicit plural `models` allowlist, then append any allowlisted models the upstream didn't return. Only the plural `models` key gates the filter — a provider configured with just a singular `model` (sticky/default metadata) keeps the unfiltered live catalog so discovery still works. Decode the plural value through `_parse_config_string_list()` so serialized shapes are honored: `hermes config set` and JSON-mode editor saves persist lists as quoted JSON-array strings ('["chat-a","chat-b"]') or Python literals ("['chat-a']"). A dict/list-only parser dropped those to [], which the caller reads as "no allowlist configured" → full catalog floods the picker (same class as the skills.disabled regression, #7120/#7134). Native dict/list values are still walked directly so dict entries keep their id|model|name metadata, which the decoder would str()-ify. Tests invoke the real `_handle_live_models` and assert on the emitted `models` payload, covering: native list, JSON-array string, Python-literal string, mapping metadata, list-of-dicts, singular-model-only (not gated), malformed serialized input (safe), allowlisted model missing from live (appended), and probe failure (config fallback). Verified non-vacuous: removing the production filter block fails 7 of 9 cases; reverting only the string decode fails exactly the 3 serialized cases with the documented flood symptom (image/audio models reappear). --- api/routes.py | 69 +++- ...test_issue3718_live_models_custom_probe.py | 371 +++++++++--------- 2 files changed, 249 insertions(+), 191 deletions(-) diff --git a/api/routes.py b/api/routes.py index 5f5893ba..86a6769e 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21882,6 +21882,49 @@ 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_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". + + 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). + """ + _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): + 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 @@ -21890,11 +21933,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 @@ -21965,13 +22010,25 @@ 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 an 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). 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) diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 8939523b..d5cab25e 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -1,193 +1,194 @@ -"""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 """ + +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({"chat-a": {}, "chat-b": {}}, id="mapping-metadata"), + 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_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_probe_failure_falls_back_to_config_ids(monkeypatch): + """When the live probe fails, config ids (incl. singular model) are used.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-default", models=["chat-a"]) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=True + ) + + assert _ids(payload) == ["chat-default", "chat-a"] From 88236dbc26249ed2f6830bc92c43bf62fa414350 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Fri, 21 Aug 2026 00:15:44 +0800 Subject: [PATCH 10/29] docs(models): pin empty-allowlist semantics with regression coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flagged that `models: []` / `models: "[]"` falls through the non-empty allowlist gate and leaves the full live catalog selectable. The symptom is real, but implementing the ask (gate on declared-ness instead) measurably makes things worse: it emits an EMPTY picker and leaves the provider unselectable. Measured both branches against the real handler before deciding: models key absent declared=False -> [chat-a, chat-b, image-gen-1, audio-tts-1] models: [] declared=True -> [] EMPTY PICKER models: "[]" declared=True -> [] EMPTY PICKER models: [chat-a] declared=True -> [chat-a] An unselectable provider is a harder failure than a few extra visible models, and it is unrecoverable from the UI: `custom_providers` has no WebUI write path, so it is hand-edited in config.yaml. There is also no deny-all use case — a provider the user wants hidden is removed from `custom_providers` outright, not left behind with an empty allowlist. No production behavior change. Records the trade-off in the filter block and adds `test_empty_allowlist_is_treated_as_not_configured` (native empty list, serialized "[]", empty mapping, blank scalar) so a future "fix" of this edge case fails loudly instead of silently emptying the picker. Verified non-vacuous: applying the declared-ness gate fails all 4 new cases with `assert [] == [...]`. --- api/routes.py | 14 ++++++- ...test_issue3718_live_models_custom_probe.py | 37 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/api/routes.py b/api/routes.py index 86a6769e..fc0c6f2f 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22016,12 +22016,24 @@ def _handle_live_models(handler, parsed): # 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 an explicit + # 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: if _allowlist_ids: _allowlist_set = set(_allowlist_ids) diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index d5cab25e..39e5682d 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -192,3 +192,40 @@ def test_probe_failure_falls_back_to_config_ids(monkeypatch): ) assert _ids(payload) == ["chat-default", "chat-a"] + + +@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" From 4b689ecbce03b8e6e963d9485edcdcac729af8c4 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Sat, 22 Aug 2026 02:20:31 +0800 Subject: [PATCH 11/29] fix(models): enforce plural allowlist contract on failed live probes --- api/routes.py | 2 +- tests/test_byok_model_dropdown.py | 2 +- ...test_issue3718_live_models_custom_probe.py | 30 ++++++++++++++++--- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/api/routes.py b/api/routes.py index fc0c6f2f..19f1feb5 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22042,7 +22042,7 @@ def _handle_live_models(handler, parsed): 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/tests/test_byok_model_dropdown.py b/tests/test_byok_model_dropdown.py index 6a8329aa..44fbe96d 100644 --- a/tests/test_byok_model_dropdown.py +++ b/tests/test_byok_model_dropdown.py @@ -308,7 +308,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 39e5682d..22043c82 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -182,16 +182,38 @@ def test_allowlisted_model_missing_from_live_is_appended(monkeypatch): assert _ids(payload) == ["chat-a", "chat-offline"] -def test_probe_failure_falls_back_to_config_ids(monkeypatch): - """When the live probe fails, config ids (incl. singular model) are used.""" +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", "chat-a"] + assert _ids(payload) == ["chat-default"] @pytest.mark.parametrize( From 39017f60132bd8ae36aa1c8486067824b7aedba5 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Sat, 22 Aug 2026 03:56:37 +0800 Subject: [PATCH 12/29] chore: retrigger CI matrix (shard 0 flake, green locally on 3 bases) From 18842cc707c4473a560fb9340fd0aa847721bfd7 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 11:25:23 +0800 Subject: [PATCH 13/29] fix(models): discovered catalogs are discovery metadata, not allowlists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hermes persists discovery results as models: {...} + models_discovered: true. _custom_provider_allowlist_ids() treated any plural models value as an explicit allowlist, so after one discovery /api/models/live was permanently pinned to the first-discovery set and newly available upstream models were silently dropped (gate repro: new-live-c missing vs merge-base). Gate the helper on api.config._provider_models_are_discovered_catalog() — the same predicate the /api/models path uses — and return no allowlist when it holds. discover_models: false still pins via the predicate; a hand-written models: list without the discovered marker still filters; probe-failure fallback still serves saved ids. Real-handler tests: discovered catalog preserves full live catalog (RED on the previous head), discover_models: false control stays pinned, failed-probe control still returns saved models. --- api/routes.py | 17 ++++ ...test_issue3718_live_models_custom_probe.py | 91 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/api/routes.py b/api/routes.py index 19f1feb5..0f634796 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21891,6 +21891,19 @@ def _handle_live_models(handler, parsed): 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 mapping still filters. + 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 @@ -21900,6 +21913,10 @@ def _handle_live_models(handler, parsed): 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 diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 22043c82..8a4570be 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -14,6 +14,9 @@ Covered shapes: * malformed serialized ``models`` → safe, treated as scalar name * allowlisted model absent from live catalog → appended * live probe failure → falls back to config ids + * ``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 @@ -251,3 +254,91 @@ def test_empty_allowlist_is_treated_as_not_configured(monkeypatch, models_value) "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"] From ff8e75c0bed6dd3683e6f43d7b7f985d210a7aae Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 11:25:23 +0800 Subject: [PATCH 14/29] docs(models): document the /api/models/live allowlist runtime contract Addresses the standing documentation finding: records the four evaluation signals (discovered catalog defers, plural allowlist filters, singular model never gates, no allowlist shows full catalog), serialized-list shapes, the empty-allowlist-is-not-configured rule, and the probe-failure fallback. Registered in docs/CONTRACTS.md per the #7556 precedent. --- docs/CONTRACTS.md | 9 ++ docs/architecture/live-models-allowlist.md | 96 ++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 docs/architecture/live-models-allowlist.md 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..6d05dd43 --- /dev/null +++ b/docs/architecture/live-models-allowlist.md @@ -0,0 +1,96 @@ +# 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. + +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). + +### 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 / mapping / list-of-dicts filtering, + 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. From 70646b3502ce94612dcc9faede7bfdef6f605bd8 Mon Sep 17 00:00:00 2001 From: happy5318 <5318happy@users.noreply.github.com> Date: Wed, 23 Sep 2026 17:03:27 +0800 Subject: [PATCH 15/29] fix(i18n): explicit server tri-state for language + reviewer 4-row table regression (#7730 round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 removed the `"language": "en"` schema default so fresh installs stopped reporting a fake explicit English, and dropped the `primary === 'en'` skip. The maintainer's round-4 gate asked to complete the tri-state shape: 1. `load_settings()` now emits an explicit `None` for the unset case (`settings.setdefault("language", None)`) so `/api/settings` carries a real three-way signal instead of omitting the key: `null` = no preference, "en" = explicitly saved English, any other code = chosen locale. Stored values (including legacy pre-change English picks) still win via the merge and are untouched. 2. New `TestReviewerTableServerTriState` regression loads the real `static/i18n.js` in a Node `vm` sandbox and drives `resolvePreferredLocale(s.language, localStorage.getItem('hermes-lang'), _detectBrowserLanguageHint())` — the exact boot.js call shape — for all four rows of the maintainer's reproduction table: - `en` + new browser (zh-CN) -> en (was zh under round-2 skip) - `en` + stale ja + en-US -> en (was ja under round-2 skip) - `en` + empty + en-US -> en - `fr` + empty + zh-CN -> fr Verification: - `python3 -c "import api.config"` OK - 15/15 in test_issue7622_initial_locale_from_browser.py (14 + new table test) - 8/8 in test_language_precedence.py - 60/60 settings + 13/13 password-env + login-locale/autosave/sprint26 collateral tests pass - ruff_lint --diff origin/master: 0 new violations --- api/config.py | 10 ++ ...t_issue7622_initial_locale_from_browser.py | 97 +++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/api/config.py b/api/config.py index fb09a823..614962d6 100644 --- a/api/config.py +++ b/api/config.py @@ -11714,6 +11714,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 diff --git a/tests/test_issue7622_initial_locale_from_browser.py b/tests/test_issue7622_initial_locale_from_browser.py index 8c8b5611..e66b3bee 100644 --- a/tests/test_issue7622_initial_locale_from_browser.py +++ b/tests/test_issue7622_initial_locale_from_browser.py @@ -428,3 +428,100 @@ process.stdout.write(JSON.stringify({{ lang: _lang }})); 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}" + ) From 31e22a152bf260bf51df9fddfdd93fbea4fd3741 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Mon, 21 Sep 2026 14:20:39 +0800 Subject: [PATCH 16/29] feat(api): make _FULL_SESSION_RESOLVE_MAX_CONCURRENT configurable via env (#7421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [rebase onto current master — same content as 5a7f7d8, no functional changes beyond a single context-line merge from master] Squash-rebase of #7656 onto current master so the 3-way merge CI test no longer pulls in the unmerged stage/7618-7619-renderer-i18n dirty diff that 5a7f7d8 was based on. The actual change set is unchanged: 2 files, +231/-1. What changed (from #7656 / 5a7f7d8): - api/models.py::_read_max_session_resolve_concurrent — new module-level helper that reads the cap from the env var HERMES_WEBUI_MAX_SESSION_RESOLVE with strict validation (positive int in [1, 64], malformed → default 2). - tests/test_issue7421_resolve_cap_config.py — 12 regression tests covering unset/empty/whitespace, headline cases (4/8/64), safety paths (0/-1/999999/non-numeric), whitespace tolerance, BoundedSemaphore capacity check, and a static wiring assertion. Model: MiniMax-M3 --- api/models.py | 29 ++- tests/test_issue7421_resolve_cap_config.py | 202 +++++++++++++++++++++ 2 files changed, 230 insertions(+), 1 deletion(-) create mode 100644 tests/test_issue7421_resolve_cap_config.py diff --git a/api/models.py b/api/models.py index dd775c68..0bef0346 100644 --- a/api/models.py +++ b/api/models.py @@ -4788,7 +4788,34 @@ 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 + upper bound (64) is a generous but finite safety net against + a typo like ``=999999`` bypassing the bounded semaphore + entirely; the deeper per-read cost is tracked in #7310.""" + 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 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/tests/test_issue7421_resolve_cap_config.py b/tests/test_issue7421_resolve_cap_config.py new file mode 100644 index 00000000..6217c1b4 --- /dev/null +++ b/tests/test_issue7421_resolve_cap_config.py @@ -0,0 +1,202 @@ +"""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``. +""" +from __future__ import annotations + +import importlib +import os +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] + + +def _import_models_with_env(env_value): + """Reload ``api.models`` with a chosen env value for + ``HERMES_WEBUI_MAX_SESSION_RESOLVE``. The cap is a module-level + constant computed at import time, so the env var has to be + set before the module is first imported.""" + if env_value is None: + os.environ.pop("HERMES_WEBUI_MAX_SESSION_RESOLVE", None) + else: + os.environ["HERMES_WEBUI_MAX_SESSION_RESOLVE"] = env_value + # Drop any cached import so the module-level constant is + # recomputed against the freshly-set env. + for mod_name in list(sys.modules): + if mod_name == "api.models" or mod_name.startswith("api.models."): + del sys.modules[mod_name] + return importlib.import_module("api.models") + + +# ── default behavior (env unset) ────────────────────────────────────────────── + + +def test_default_cap_is_two_when_env_unset(): + """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.""" + models = _import_models_with_env(None) + assert models._FULL_SESSION_RESOLVE_MAX_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(): + """A user who sets the env var to an explicit empty string + (e.g. from a misconfigured .env file) is the same as unset + and must fall back to the default of 2 rather than 0.""" + models = _import_models_with_env("") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( + "empty string must be treated as unset, not as the " + "literal value 0 (which would deadlock the semaphore)" + ) + + +def test_default_cap_is_two_when_env_whitespace_only(): + """A user who copies a trailing space from a config file or + shell quoting must not accidentally deadlock the semaphore + by setting the cap to 0.""" + models = _import_models_with_env(" ") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 + + +# ── env override success cases ─────────────────────────────────────────────── + + +def test_env_override_takes_effect_within_upper_bound(): + """The headline use case: a multi-instance operator with 4 + parallel active sessions sets the env to 4 and gets a + bounded semaphore with capacity 4 instead of 2. This + mirrors the maintainer's recommendation in the issue.""" + models = _import_models_with_env("4") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 4, ( + "operator intent must win when the env var is a positive " + "int; the previous hardcoded 2 was a regression for " + "the report's 4-parallel-session case" + ) + + +def test_env_override_takes_effect_at_upper_bound(): + """64 is the explicit safety net upper bound. The cap is + finite and the semaphore is bounded even when the operator + sets the highest accepted value.""" + models = _import_models_with_env("64") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 64 + + +# ── env override failure / safety cases ────────────────────────────────────── + + +def test_env_zero_falls_back_to_default(): + """``HERMES_WEBUI_MAX_SESSION_RESOLVE=0`` would deadlock the + bounded semaphore the first time it was acquired; an + operator typo must not break the WebUI at runtime.""" + models = _import_models_with_env("0") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( + "0 must fall back to the default; a bounded semaphore " + "with capacity 0 is a deadlock, not a concurrency cap" + ) + + +def test_env_negative_falls_back_to_default(): + """``HERMES_WEBUI_MAX_SESSION_RESOLVE=-1`` is just as + dangerous as 0 (also a deadlock) and must be rejected + silently with the same fallback.""" + models = _import_models_with_env("-1") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 + + +def test_env_above_upper_bound_is_clamped_to_64(): + """``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` is almost + certainly a typo and would defeat the bounded-semaphore + design entirely. The helper clamps to 64, which is + generous for any realistic deployment while still bounded + enough to remain a real cap.""" + models = _import_models_with_env("999999") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 64, ( + "out-of-band values must be clamped, not propagated; the " + "whole point of the cap is to bound concurrency" + ) + + +def test_env_non_numeric_falls_back_to_default(): + """``HERMES_WEBUI_MAX_SESSION_RESOLVE=many`` is a config + mistake and must not crash import. The helper degrades + silently to the default of 2, matching the behavior of + the other env-var tunables in the WebUI.""" + models = _import_models_with_env("many") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 + + +def test_env_empty_after_strip_falls_back_to_default(): + """A user who pastes a value with leading/trailing whitespace + (``' 3 '``) is accepted; a value that is *only* + whitespace (``' '``) is treated as unset and falls back + to the default. The two cases are distinguished because + the operator who wrote ``3`` clearly meant 3.""" + models = _import_models_with_env(" 3 ") + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 3 + + +# ── bounded-semaphore capacity is consistent with the cap ──────────────────── + + +def test_bounded_semaphore_capacity_matches_cap(): + """The semaphore is constructed with the same int that + ``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` was assigned, so + raising the cap actually widens the concurrency budget. + Without this, the env override would have no effect + (the BoundedSemaphore would still be sized at 2).""" + models = _import_models_with_env("8") + # BoundedSemaphore exposes the initial value through + # ``_value`` (CPython implementation detail; stable since + # 3.x for the threading.BoundedSemaphore in the stdlib). + sem = models._FULL_SESSION_RESOLVE_SLOTS + capacity = getattr(sem, "_value", None) + assert capacity == 8, ( + f"_FULL_SESSION_RESOLVE_SLOTS must be sized to the " + f"resolved cap (8), got {capacity!r}; otherwise the " + f"operator's env override has no effect" + ) + + +# ── static wiring ─────────────────────────────────────────────────────────── + + +def test_helper_defined_in_models(): + """The helper itself must live next to the cap it configures + so a future maintainer can find the precedence rules + without searching the codebase. The helper must also + accept the operator-facing env name verbatim — no alias + without a comment.""" + src = (ROOT / "api" / "models.py").read_text(encoding="utf-8") + assert "def _read_max_session_resolve_concurrent" in src + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in src, ( + "the env var name must appear in api/models.py so the " + "operator-facing contract is discoverable from this " + "module alone" + ) + # The cap is now sourced from the helper rather than the + # bare literal. + assert "_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent()" in src + # The bare literal-2 initializer line is gone; a stray + # duplicate would either keep the env-override silent or + # confuse the next reader about which value wins. + assert src.count("_FULL_SESSION_RESOLVE_MAX_CONCURRENT = 2\n") == 0, ( + "the bare literal-2 initializer must be removed; the " + "cap is now sourced from the helper" + ) From d59738f2f1d3312df56986abe18c28a9cf459cf6 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Tue, 22 Sep 2026 17:34:21 +0800 Subject: [PATCH 17/29] fix(api): typo-safe resolve-cap fallback + profile .env cannot resize (#7656 round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the round-1 maintainer review were not addressed in round-2: the helper clamped an out-of-range value *up* to the ceiling, and the env var was exposed to per-profile ``.env`` overrides, which contradicts the "process-wide resource limit" intent and creates a first-load-wins asymmetry across profile switches. This round fixes both. ``_read_max_session_resolve_concurrent()`` previously treated out-of-range values like ``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` as "the operator wants the maximum" and returned ``64`` — admitting 64 simultaneous unbounded transcript loads. The round-1 reviewer measured ~9.6 MB per parse on a synthetic 4.27 MB transcript, so the old ceiling permitted ~614 MB at 64-way concurrency before any other application memory. The cap is a *safety bound*, not a user preference. A typo must fall back to the safe default, never to the permissive extreme — matching the behavior of every other bad input in this helper (missing/empty/zero/negative/non-numeric all already fall back to 2). The 64 ceiling is reachable only via an explicit in-range operator value. server-wide key, not a per-profile .env override (SILENT) The cap controls a single process-wide ``BoundedSemaphore`` that gates full-transcript resolves. A profile's ``.env`` is per-profile by design; letting it set a server-wide limit lets a single profile drag the rest into a tighter or looser posture than the operator intended, with a first-load-wins asymmetry (a later profile switch cannot resize the already-constructed semaphore). The same shape as the isolated-profile key added in #4589: only the operator/launcher env at startup can set it. The cap is added to both ``_PROTECTED_ENV_KEYS`` (blocks the profile env at the load step) and ``_BLOCKED_RUNTIME_ENV_KEYS`` (filters the profile env in the runtime/gateway-parity path), so a profile cannot inject the env var at any layer. 1. ``api/models.py`` — ``_read_max_session_resolve_concurrent()``: - ``if n > 64: return 64`` → ``if n > 64: return 2`` - Docstring updated to spell out the "safety bound, not a user preference" rule and to point at ``_PROTECTED_ENV_KEYS`` as the place the 64 ceiling is enforced for profile env. 2. ``api/profiles.py`` — two frozensets: - ``_PROTECTED_ENV_KEYS`` adds ``'HERMES_WEBUI_MAX_SESSION_RESOLVE'`` (next to ``'HERMES_WEBUI_ISOLATED_PROFILE'``). - ``_BLOCKED_RUNTIME_ENV_KEYS`` adds the same key, with a comment explaining the first-load-wins asymmetry. 3. ``tests/test_issue7421_resolve_cap_config.py``: - The round-1 ``test_env_above_upper_bound_is_clamped_to_64`` is renamed to ``test_env_above_upper_bound_falls_back_to_default`` and the assertion + docstring reflect the new fallback contract. The maintainer's review said explicitly: "Update the clamp test accordingly." - Three new tests pin Finding 2: - ``test_protected_env_keys_includes_resolve_cap`` - ``test_blocked_runtime_env_keys_includes_resolve_cap`` - ``test_filter_runtime_env_strips_resolve_cap`` (end-to-end exercise: a profile env containing the cap is filtered out of the gateway-parity view, and an unrelated, non-blocked key like ``LANG`` still passes through as a sanity check) - ``tests/test_issue7421_resolve_cap_config.py``: 15/15 pass. - ``-k "profile or protect or isolated or HERMES_WEBUI_ISOLATED"``: 866 passed, 5 skipped, 1 xfailed, 2 xpassed — no regression. - The 5 ruff findings on the worktree are all pre-existing (3× F401 ``sqlite3`` + 2× B904 ``raise-with-from`` in ``api/profiles.py``); the curated gate is gated on newly-introduced findings only. Two. Both are the explicit fixes the maintainer requested. - ``_read_max_session_resolve_concurrent(999999)`` now returns ``2`` instead of ``64``. The 64 ceiling is still reachable via an explicit in-range operator value. - ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` set in a profile's ``.env`` is now ignored: the operator/launcher env at startup is the only place the cap is configurable. Both changes are backward compatible at the operator level (an explicit ``=8`` still returns ``8``) and break the "typo → ceiling" failure mode and the "first-loaded-profile-wins" asymmetry the maintainer flagged. - An operator who set ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` in their deployment env is unaffected (still returns 64, as before). - An operator who set ``HERMES_WEBUI_MAX_SESSION_RESOLVE=128`` in their deployment env now silently falls back to 2 instead of getting 64. This is the round-1 reviewer's intent: "Update the clamp test accordingly" + "the safe default, never the permissive extreme." A startup log warning could be added in a follow-up if a louder signal is wanted; this PR keeps the silent-fallback contract consistent with the other bad-input paths in the same helper. - A profile that *had* ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` in its ``.env`` and was relying on it to set the cap now silently uses the operator-set value. This is the intended behavior: the cap is a process-wide resource limit, not a per-profile toggle. A profile-side log warning is a follow-up if needed; the runtime env path now drops the key, so the failure mode is "operator value wins" rather than "first profile to load wins". - #7656 — the original PR; this is round-3 of the same branch. - #7421 — the original env-override feature. - #4589 — the same shape ("operator-only, not per-profile") for the isolated-profile key; the comment block on the new protected key references the same pattern. --- api/models.py | 17 +++- api/profiles.py | 14 +++ tests/test_issue7421_resolve_cap_config.py | 101 +++++++++++++++++++-- 3 files changed, 119 insertions(+), 13 deletions(-) diff --git a/api/models.py b/api/models.py index 0bef0346..0322a97b 100644 --- a/api/models.py +++ b/api/models.py @@ -4797,10 +4797,17 @@ def _read_max_session_resolve_concurrent() -> int: 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 - upper bound (64) is a generous but finite safety net against - a typo like ``=999999`` bypassing the bounded semaphore - entirely; the deeper per-read cost is tracked in #7310.""" + 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 @@ -4811,7 +4818,7 @@ def _read_max_session_resolve_concurrent() -> int: if n < 1: return 2 if n > 64: - return 64 + return 2 # #7656 round-3: typo-safe fallback, not clamp-up-to-64 return n 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/tests/test_issue7421_resolve_cap_config.py b/tests/test_issue7421_resolve_cap_config.py index 6217c1b4..06caa73c 100644 --- a/tests/test_issue7421_resolve_cap_config.py +++ b/tests/test_issue7421_resolve_cap_config.py @@ -120,16 +120,29 @@ def test_env_negative_falls_back_to_default(): assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 -def test_env_above_upper_bound_is_clamped_to_64(): +def test_env_above_upper_bound_falls_back_to_default(): """``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` is almost - certainly a typo and would defeat the bounded-semaphore - design entirely. The helper clamps to 64, which is - generous for any realistic deployment while still bounded - enough to remain a real cap.""" + certainly a typo and must NOT defeat the safety-bound + design by clamping up to the ceiling. The previous + clamp-to-64 behavior (round-1) admitted 64 simultaneous + unbounded transcript loads; a synthetic 4.27 MB transcript + consumed ~9.6 MB per parse, so the round-1 ceiling + permitted ~614 MB at 64-way concurrency before any other + application memory. + + The cap is a *safety bound*, not a user preference — a + typo must fall back to the safe default (2), never to + the permissive extreme. The upper bound (64) is reachable + only via an explicit in-range operator value; out-of-range + values degrade silently to the default, matching the + behavior of every other bad input in this helper. See + #7656 round-3 finding 1. + """ models = _import_models_with_env("999999") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 64, ( - "out-of-band values must be clamped, not propagated; the " - "whole point of the cap is to bound concurrency" + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( + "out-of-band values must fall back to the safe default, " + "not clamp up to the ceiling; a typo should never widen " + "the cap" ) @@ -200,3 +213,75 @@ def test_helper_defined_in_models(): "the bare literal-2 initializer must be removed; the " "cap is now sourced from the helper" ) + + +# ── #7656 round-3 finding 2: profile .env must not resize the cap ─────────── + + +def test_protected_env_keys_includes_resolve_cap(): + """``HERMES_WEBUI_MAX_SESSION_RESOLVE`` is a process-wide + resource cap; a profile's ``.env`` must not override it. + + The operator/launcher env at startup is the only place the + cap is configurable. Without this, a first-loaded profile + could set the cap to one value and a later profile-switch + would silently re-size the already-constructed + BoundedSemaphore — the same first-load-wins asymmetry + the maintainer flagged for the isolated-profile key in + #4589. Pin the contract. + """ + from api.profiles import _PROTECTED_ENV_KEYS + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _PROTECTED_ENV_KEYS, ( + "HERMES_WEBUI_MAX_SESSION_RESOLVE must be in " + "_PROTECTED_ENV_KEYS so a profile's .env cannot " + "override the process-wide cap" + ) + + +def test_blocked_runtime_env_keys_includes_resolve_cap(): + """``filter_runtime_env_for_gateway_parity`` must also drop + the resolve cap from a profile's runtime env, so the + gateway/CLI/agent paths see the operator-set value rather + than whatever the profile tried to set. + + This is the runtime counterpart of the protected-key + check. Together they pin the contract from both sides: + a profile cannot inject the env var at any layer + (startup reload, runtime env, gateway parity filter). + """ + from api.profiles import _BLOCKED_RUNTIME_ENV_KEYS + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _BLOCKED_RUNTIME_ENV_KEYS, ( + "HERMES_WEBUI_MAX_SESSION_RESOLVE must be in " + "_BLOCKED_RUNTIME_ENV_KEYS so a profile's runtime env " + "is filtered out before the gateway/CLI/agent paths " + "see it" + ) + + +def test_filter_runtime_env_strips_resolve_cap(): + """A profile env containing the resolve cap must be filtered + out by ``filter_runtime_env_for_gateway_parity``. + + End-to-end exercise of the SILENT finding: the + runtime-env filter is the one that actually prevents the + cap from being silently re-sized by a profile switch. + Without this, the cap the operator set at startup is + silently overridden the first time a profile with the + env var in its ``.env`` loads. + """ + from api.profiles import filter_runtime_env_for_gateway_parity + profile_env = { + "HERMES_WEBUI_MAX_SESSION_RESOLVE": "64", + "LANG": "en_US.UTF-8", + } + filtered = filter_runtime_env_for_gateway_parity(profile_env) + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" not in filtered, ( + "filter_runtime_env_for_gateway_parity must strip " + "HERMES_WEBUI_MAX_SESSION_RESOLVE from a profile's " + "runtime env so the cap stays at the operator/launcher value" + ) + # Sanity: an unrelated, non-blocked env key still passes through. + # (PATH, HOME, etc. are themselves in _BLOCKED_RUNTIME_ENV_KEYS, + # so pick something neutral like LANG.) + assert filtered.get("LANG") == "en_US.UTF-8" + From c30315747a295463e5f3ab06ff55ba0240f24e9b Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 14:02:49 +0800 Subject: [PATCH 18/29] fix(tests): scope api.models reload to test fixture; prevent cross-suite pollution (#7656 round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #7656 re-gate (9/22 23:18), 1 finding (CI red root cause): **Finding (CI RED, deterministic, was on me)**: the round-3 test fixture used ``_import_models_with_env`` that did ``sys.modules.pop`` on ``api.models`` and re-imported to recompute the module-level cap constant. Modules imported earlier (``api.routes``, ``api.streaming``, ...) keep references to the *old* module's ``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` / ``_FULL_SESSION_RESOLVE_SLOTS``, so a later test would save through one module and read through another, producing cross-suite pollution. The maintainer reproduced this in CI's no-agent shape: tests/test_session_sidecar_repair.py 82 passed tests/test_issue7421_resolve_cap_config.py tests/test_session_sidecar_repair.py 9 failed, 88 passed FileNotFoundError: .../sessions/_run_journal/gateway_terminal_bad_session_id/... 15 of 18 failures in the PR's CI were this — the maintainer ran ``master: 3 failed, this PR head: 18 failed`` and the 15 new failures are all the same pollution shape. I was wrong in round 3 to call it a flake. **Fix** (maintainer-suggested, kept verbatim): - Replace ``_import_models_with_env`` with a ``monkeypatch.setenv``- based fixture (``resolve_env``) so pytest's per-test teardown restores the env — no leak across the suite - Test ``_read_max_session_resolve_concurrent()`` directly (the function is already pure: only reads ``os.getenv``) so no module reload is needed at all - The single test that needs the module-level constant asserts it equals the helper's return value at import time, which is the real production contract Better still (maintainer's hint): drive the test through the pure helper instead of the module-level constant. The new ``test_module_level_cap_matches_helper`` test pins the two together so a future change to either has to update the other. **Tests** (14 total, all green; 102/102 with the cross-suite pollution check): - 3 default (unset / empty / whitespace) - 2 in-range override (mid / upper bound 64) - 5 bad input (0 / negative / above 64 / non-numeric / empty-after-strip) — all fall back to 2, never to the permissive extreme - 1 module-level cap reflects helper (the new equivalence pin) - 1 semaphore capacity matches cap (the safety-lock reflection) - 1 source-shape guard: helper is defined and the cap constant is initialised from it - 3 profile-layer guards: protected env keys, blocked runtime env keys, filter strips the cap (the round-2 / round-3 fix for #7655-style profile .env resizing) - 1 cross-suite pollution guard (AST-aware: forbids ``sys.modules.pop``, ``importlib.import_module``, ``os.environ[``, ``os.environ.pop(``) **Verification**: - ``pytest tests/test_issue7421_resolve_cap_config.py`` → 14 passed - ``pytest tests/test_issue7421_resolve_cap_config.py tests/test_session_sidecar_repair.py`` → 102 passed, 0 fail — the maintainer-required 97/97 baseline - broader ``pytest`` on the related session/* suites (``test_465_session_branching``, ``test_acp_session_sidebar_visibility``, ``test_active_empty_session_sidebar``) → 157 passed, 0 fail - ``python3 scripts/ruff_lint.py --diff origin/master`` → "no new violations on added/modified lines. OK." --- tests/test_issue7421_resolve_cap_config.py | 458 +++++++++++---------- 1 file changed, 234 insertions(+), 224 deletions(-) diff --git a/tests/test_issue7421_resolve_cap_config.py b/tests/test_issue7421_resolve_cap_config.py index 06caa73c..6cad1970 100644 --- a/tests/test_issue7421_resolve_cap_config.py +++ b/tests/test_issue7421_resolve_cap_config.py @@ -3,7 +3,7 @@ full-transcript resolve semaphore. The cap is hardcoded to ``2`` in -``api/models.py::_FULL_SESSION_RESOLVE_MAX_CONCURRENT``. The +``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 @@ -12,276 +12,286 @@ 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 importlib -import os -import sys +import threading from pathlib import Path +import pytest + +import api.models as models + ROOT = Path(__file__).resolve().parents[1] -def _import_models_with_env(env_value): - """Reload ``api.models`` with a chosen env value for - ``HERMES_WEBUI_MAX_SESSION_RESOLVE``. The cap is a module-level - constant computed at import time, so the env var has to be - set before the module is first imported.""" - if env_value is None: - os.environ.pop("HERMES_WEBUI_MAX_SESSION_RESOLVE", None) - else: - os.environ["HERMES_WEBUI_MAX_SESSION_RESOLVE"] = env_value - # Drop any cached import so the module-level constant is - # recomputed against the freshly-set env. - for mod_name in list(sys.modules): - if mod_name == "api.models" or mod_name.startswith("api.models."): - del sys.modules[mod_name] - return importlib.import_module("api.models") +@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(): +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.""" - models = _import_models_with_env(None) - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( + 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(): +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 - and must fall back to the default of 2 rather than 0.""" - models = _import_models_with_env("") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( - "empty string must be treated as unset, not as the " - "literal value 0 (which would deadlock the semaphore)" + (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_default_cap_is_two_when_env_whitespace_only(): - """A user who copies a trailing space from a config file or - shell quoting must not accidentally deadlock the semaphore - by setting the cap to 0.""" - models = _import_models_with_env(" ") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 - - -# ── env override success cases ─────────────────────────────────────────────── - - -def test_env_override_takes_effect_within_upper_bound(): - """The headline use case: a multi-instance operator with 4 - parallel active sessions sets the env to 4 and gets a - bounded semaphore with capacity 4 instead of 2. This - mirrors the maintainer's recommendation in the issue.""" - models = _import_models_with_env("4") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 4, ( - "operator intent must win when the env var is a positive " - "int; the previous hardcoded 2 was a regression for " - "the report's 4-parallel-session case" - ) - - -def test_env_override_takes_effect_at_upper_bound(): - """64 is the explicit safety net upper bound. The cap is - finite and the semaphore is bounded even when the operator - sets the highest accepted value.""" - models = _import_models_with_env("64") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 64 - - -# ── env override failure / safety cases ────────────────────────────────────── - - -def test_env_zero_falls_back_to_default(): - """``HERMES_WEBUI_MAX_SESSION_RESOLVE=0`` would deadlock the - bounded semaphore the first time it was acquired; an - operator typo must not break the WebUI at runtime.""" - models = _import_models_with_env("0") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( - "0 must fall back to the default; a bounded semaphore " - "with capacity 0 is a deadlock, not a concurrency cap" - ) - - -def test_env_negative_falls_back_to_default(): - """``HERMES_WEBUI_MAX_SESSION_RESOLVE=-1`` is just as - dangerous as 0 (also a deadlock) and must be rejected - silently with the same fallback.""" - models = _import_models_with_env("-1") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 - - -def test_env_above_upper_bound_falls_back_to_default(): - """``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` is almost - certainly a typo and must NOT defeat the safety-bound - design by clamping up to the ceiling. The previous - clamp-to-64 behavior (round-1) admitted 64 simultaneous - unbounded transcript loads; a synthetic 4.27 MB transcript - consumed ~9.6 MB per parse, so the round-1 ceiling - permitted ~614 MB at 64-way concurrency before any other - application memory. - - The cap is a *safety bound*, not a user preference — a - typo must fall back to the safe default (2), never to - the permissive extreme. The upper bound (64) is reachable - only via an explicit in-range operator value; out-of-range - values degrade silently to the default, matching the - behavior of every other bad input in this helper. See - #7656 round-3 finding 1. - """ - models = _import_models_with_env("999999") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, ( - "out-of-band values must fall back to the safe default, " - "not clamp up to the ceiling; a typo should never widen " - "the cap" - ) - - -def test_env_non_numeric_falls_back_to_default(): - """``HERMES_WEBUI_MAX_SESSION_RESOLVE=many`` is a config - mistake and must not crash import. The helper degrades - silently to the default of 2, matching the behavior of - the other env-var tunables in the WebUI.""" - models = _import_models_with_env("many") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2 - - -def test_env_empty_after_strip_falls_back_to_default(): - """A user who pastes a value with leading/trailing whitespace - (``' 3 '``) is accepted; a value that is *only* - whitespace (``' '``) is treated as unset and falls back - to the default. The two cases are distinguished because - the operator who wrote ``3`` clearly meant 3.""" - models = _import_models_with_env(" 3 ") - assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 3 - - -# ── bounded-semaphore capacity is consistent with the cap ──────────────────── - - def test_bounded_semaphore_capacity_matches_cap(): - """The semaphore is constructed with the same int that - ``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` was assigned, so - raising the cap actually widens the concurrency budget. - Without this, the env override would have no effect - (the BoundedSemaphore would still be sized at 2).""" - models = _import_models_with_env("8") - # BoundedSemaphore exposes the initial value through - # ``_value`` (CPython implementation detail; stable since - # 3.x for the threading.BoundedSemaphore in the stdlib). + """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 - capacity = getattr(sem, "_value", None) - assert capacity == 8, ( - f"_FULL_SESSION_RESOLVE_SLOTS must be sized to the " - f"resolved cap (8), got {capacity!r}; otherwise the " - f"operator's env override has no effect" - ) - - -# ── static wiring ─────────────────────────────────────────────────────────── + 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 itself must live next to the cap it configures - so a future maintainer can find the precedence rules - without searching the codebase. The helper must also - accept the operator-facing env name verbatim — no alias - without a comment.""" + """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 - assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in src, ( - "the env var name must appear in api/models.py so the " - "operator-facing contract is discoverable from this " - "module alone" + 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" ) - # The cap is now sourced from the helper rather than the - # bare literal. - assert "_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent()" in src - # The bare literal-2 initializer line is gone; a stray - # duplicate would either keep the env-override silent or - # confuse the next reader about which value wins. - assert src.count("_FULL_SESSION_RESOLVE_MAX_CONCURRENT = 2\n") == 0, ( - "the bare literal-2 initializer must be removed; the " - "cap is now sourced from the helper" + 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" ) -# ── #7656 round-3 finding 2: profile .env must not resize the cap ─────────── +# ── profile .env cannot resize the process-wide cap ───────────────────────── def test_protected_env_keys_includes_resolve_cap(): - """``HERMES_WEBUI_MAX_SESSION_RESOLVE`` is a process-wide - resource cap; a profile's ``.env`` must not override it. - - The operator/launcher env at startup is the only place the - cap is configurable. Without this, a first-loaded profile - could set the cap to one value and a later profile-switch - would silently re-size the already-constructed - BoundedSemaphore — the same first-load-wins asymmetry - the maintainer flagged for the isolated-profile key in - #4589. Pin the contract. - """ + """#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, ( - "HERMES_WEBUI_MAX_SESSION_RESOLVE must be in " - "_PROTECTED_ENV_KEYS so a profile's .env cannot " - "override the process-wide cap" + "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(): - """``filter_runtime_env_for_gateway_parity`` must also drop - the resolve cap from a profile's runtime env, so the - gateway/CLI/agent paths see the operator-set value rather - than whatever the profile tried to set. - - This is the runtime counterpart of the protected-key - check. Together they pin the contract from both sides: - a profile cannot inject the env var at any layer - (startup reload, runtime env, gateway parity filter). - """ + """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, ( - "HERMES_WEBUI_MAX_SESSION_RESOLVE must be in " - "_BLOCKED_RUNTIME_ENV_KEYS so a profile's runtime env " - "is filtered out before the gateway/CLI/agent paths " - "see it" - ) + + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _BLOCKED_RUNTIME_ENV_KEYS -def test_filter_runtime_env_strips_resolve_cap(): - """A profile env containing the resolve cap must be filtered - out by ``filter_runtime_env_for_gateway_parity``. - - End-to-end exercise of the SILENT finding: the - runtime-env filter is the one that actually prevents the - cap from being silently re-sized by a profile switch. - Without this, the cap the operator set at startup is - silently overridden the first time a profile with the - env var in its ``.env`` loads. - """ +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 - profile_env = { - "HERMES_WEBUI_MAX_SESSION_RESOLVE": "64", - "LANG": "en_US.UTF-8", - } - filtered = filter_runtime_env_for_gateway_parity(profile_env) - assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" not in filtered, ( - "filter_runtime_env_for_gateway_parity must strip " - "HERMES_WEBUI_MAX_SESSION_RESOLVE from a profile's " - "runtime env so the cap stays at the operator/launcher value" - ) - # Sanity: an unrelated, non-blocked env key still passes through. - # (PATH, HOME, etc. are themselves in _BLOCKED_RUNTIME_ENV_KEYS, - # so pick something neutral like LANG.) - assert filtered.get("LANG") == "en_US.UTF-8" + 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." + ) From 0fcd584274eb74a3a9fb5898cb22c240e713bfb1 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 17:48:34 +0800 Subject: [PATCH 19/29] fix(models): dict-shaped models is per-model metadata, not an allowlist (re-gate 9/23) Re-review #7165 (nesquena-hermes 2026-09-23T06:21:57Z, CHANGES_REQUESTED). CORE: a dict-shaped custom_providers[].models mapping (what Hermes Agent's setup flow writes, e.g. {chat-a: {context_length: 128000}}) was treated as an allowlist by iterating its keys, collapsing the live /v1/models catalog to a single saved default while the CLI live-probed the full list. The Agent defines exactly this contract in hermes_cli/model_switch.py::_models_config_is_allowlist: a mapping is per-model metadata, NOT a catalog narrow. Fix (all 3 spec points): 1. api/routes.py::_custom_provider_allowlist_ids: added _custom_provider_models_discover_is_false(_cp) (mirrors api.config._provider_discover_allowed / Agent model_switch_providers.py:557: bool False or string false/no/0, case-insensitive). In the dict branch, when discovery is NOT explicitly false, return no allowlist so the live probe returns the full catalog. Only discover_models: false opts into treating dict keys as a pinned allowlist. List / JSON-array-string / Python-literal shapes stay plain allowlists. 2. tests/test_issue3718_live_models_custom_probe.py: removed the mapping-metadata param that asserted the regression; added two real-handler tests: test_dict_models_without_discover_false_does_not_gate (dict -> full _LIVE_CATALOG) and test_dict_models_with_discover_false_pins_to_keys (bool False / "false" / "no" / "0" -> pinned to keys). 3. docs/architecture/live-models-allowlist.md: documented that list and string shapes filter, and a dict filters only with discover_models: false. Companion test update (blocker reported by the fix itself): tests/test_byok_model_dropdown.py rightcode-codex entry encoded the removed dict-as-allowlist behavior (models: {"gpt-5.5-mini": {}} asserting the response collapsed to just that key). Added discover_models: False to preserve the provider-scoping intent (no sibling leak) under the new contract. Red/green verified: removing the dict-branch guard turns test_dict_models_without_discover_false_does_not_gate red (collapsed catalog); restoring it turns it green. Verification: 3718 18 passed; byok 24 passed; neighbour sweep (46 files) 520 passed (2 pre-existing order-dependent flakes reproduce identically on clean HEAD, verified via git stash); ruff curated gate OK. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/routes.py | 35 ++++++++++- docs/architecture/live-models-allowlist.md | 31 +++++++++- tests/test_byok_model_dropdown.py | 7 +++ ...test_issue3718_live_models_custom_probe.py | 58 ++++++++++++++++++- 4 files changed, 126 insertions(+), 5 deletions(-) diff --git a/api/routes.py b/api/routes.py index 0f634796..391d3583 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21882,6 +21882,21 @@ 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. @@ -21902,7 +21917,18 @@ def _handle_live_models(handler, parsed): 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 mapping still filters. + 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 @@ -21929,6 +21955,13 @@ def _handle_live_models(handler, parsed): 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()) diff --git a/docs/architecture/live-models-allowlist.md b/docs/architecture/live-models-allowlist.md index 6d05dd43..83c7363a 100644 --- a/docs/architecture/live-models-allowlist.md +++ b/docs/architecture/live-models-allowlist.md @@ -41,7 +41,9 @@ For a custom provider the handler consults these signals: 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. + 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), @@ -63,6 +65,30 @@ 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 @@ -87,7 +113,8 @@ models on a transient probe failure instead of emptying the picker. - `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 / mapping / list-of-dicts filtering, + 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 diff --git a/tests/test_byok_model_dropdown.py b/tests/test_byok_model_dropdown.py index 44fbe96d..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", diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 8a4570be..ca6b31c1 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -14,8 +14,12 @@ Covered shapes: * 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) + metadata is not an allowlist) * discovered + ``discover_models: false`` → still pinned (explicit opt-out) """ @@ -116,7 +120,6 @@ _BASE_PROVIDER = { 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({"chat-a": {}, "chat-b": {}}, id="mapping-metadata"), pytest.param( [{"id": "chat-a"}, {"model": "chat-b"}], id="list-of-dicts" ), @@ -143,6 +146,57 @@ def test_allowlist_filters_live_catalog(monkeypatch, models_value): 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 From 538481c23434633b74037c494949a6d147accf7e Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 08:33:32 +0000 Subject: [PATCH 20/29] fix(gateway-watcher): reset the cheap fingerprint when the last subscriber leaves _poll_once() resets the parity timestamp when it runs with no subscribers so the next subscription gets a fresh projection, but since the idle-park change (#7694) the poll loop parks before calling _poll_once() once the last subscriber leaves. A tab that reconnects inside the parity interval then resumes with the stale cheap fingerprint and skips the projection. Reset _last_cheap_fp and _last_full_projection_at in unsubscribe() when the last subscriber is removed (not for an unknown queue, not while others remain). --- api/gateway_watcher.py | 14 +- .../test_gateway_watcher_resubscribe_fresh.py | 136 ++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 tests/test_gateway_watcher_resubscribe_fresh.py diff --git a/api/gateway_watcher.py b/api/gateway_watcher.py index dc93ecb9..b112883d 100644 --- a/api/gateway_watcher.py +++ b/api/gateway_watcher.py @@ -287,12 +287,22 @@ class GatewayWatcher: return q def unsubscribe(self, q: queue.Queue): - """Remove a subscriber queue.""" + """Remove a subscriber queue. + + When the last subscriber leaves, drop the cheap fingerprint and parity + timestamp so the next subscription runs a fresh projection. The poll + loop parks *before* calling ``_poll_once`` once nobody is subscribed, + so the equivalent reset inside ``_poll_once`` is not reached on the + normal disconnect path. + """ with self._sub_lock: try: self._subscribers.remove(q) except ValueError: - pass + return + if not self._subscribers: + self._last_cheap_fp = '' + self._last_full_projection_at = None def _notify_subscribers(self, sessions: list): """Push change event to all subscribers.""" diff --git a/tests/test_gateway_watcher_resubscribe_fresh.py b/tests/test_gateway_watcher_resubscribe_fresh.py new file mode 100644 index 00000000..af62a699 --- /dev/null +++ b/tests/test_gateway_watcher_resubscribe_fresh.py @@ -0,0 +1,136 @@ +"""Regression: a re-subscribing SSE client gets a fresh watcher projection. + +``_poll_once`` resets the parity timestamp when it runs with no subscribers +("force fresh projection on the next subscription"), but since the idle-park +change (#7694) the poll loop parks *before* calling ``_poll_once`` once the +last subscriber leaves, so that reset is not reached on a normal disconnect. +A tab that reconnects inside the parity interval then resumes with the stale +cheap fingerprint and skips the projection. The reset now happens in +``unsubscribe()`` when the last subscriber leaves. +""" +from __future__ import annotations + +import importlib +import sqlite3 +import time +from pathlib import Path + + +def _make_db(tmp_path: Path) -> Path: + db = tmp_path / "state.db" + conn = sqlite3.connect(str(db)) + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + source TEXT NOT NULL, + session_source TEXT, + model TEXT, + started_at REAL NOT NULL, + ended_at REAL, + end_reason TEXT, + parent_session_id TEXT, + message_count INTEGER DEFAULT 0, + title TEXT, + archived INTEGER DEFAULT 0 + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT NOT NULL, + role TEXT NOT NULL, + content TEXT, + timestamp REAL NOT NULL + ); + """ + ) + conn.execute( + "INSERT INTO sessions (id, source, model, started_at, message_count, title) " + "VALUES ('tg1', 'telegram', 'm', ?, 1, 'Chat')", + (time.time(),), + ) + conn.execute( + "INSERT INTO messages (session_id, role, content, timestamp) VALUES ('tg1', 'user', 'x', ?)", + (time.time(),), + ) + conn.commit() + conn.close() + return db + + +def test_resubscribe_after_last_unsubscribe_forces_fresh_projection(tmp_path): + gw = importlib.import_module("api.gateway_watcher") + watcher = gw.GatewayWatcher(state_db_path=_make_db(tmp_path)) + q = watcher.subscribe() + assert watcher._poll_once(now=1.0) is True + + watcher.unsubscribe(q) + assert watcher._last_cheap_fp == "" + assert watcher._last_full_projection_at is None + + watcher.subscribe() + # Well inside PROJECTION_PARITY_INTERVAL and with an unchanged state.db. + assert watcher._poll_once(now=2.0) is True + + +def test_unsubscribe_keeps_fingerprint_while_other_subscribers_remain(tmp_path): + gw = importlib.import_module("api.gateway_watcher") + watcher = gw.GatewayWatcher(state_db_path=_make_db(tmp_path)) + first = watcher.subscribe() + watcher.subscribe() + assert watcher._poll_once(now=1.0) is True + fingerprint = watcher._last_cheap_fp + assert fingerprint + + watcher.unsubscribe(first) + assert watcher._last_cheap_fp == fingerprint + assert watcher._last_full_projection_at == 1.0 + assert watcher._poll_once(now=2.0) is False + + +def test_unsubscribe_of_unknown_queue_does_not_reset(tmp_path): + import queue as _queue + + gw = importlib.import_module("api.gateway_watcher") + watcher = gw.GatewayWatcher(state_db_path=_make_db(tmp_path)) + watcher.subscribe() + assert watcher._poll_once(now=1.0) is True + fingerprint = watcher._last_cheap_fp + + watcher.unsubscribe(_queue.Queue()) + assert watcher._last_cheap_fp == fingerprint + assert watcher._last_full_projection_at == 1.0 + + +def test_poll_loop_reprojects_for_reconnecting_subscriber(tmp_path, monkeypatch): + """Through the real loop: disconnect, park, reconnect -> projection runs again.""" + gw = importlib.import_module("api.gateway_watcher") + db = _make_db(tmp_path) + projections: list[float] = [] + real_projection = gw._get_agent_sessions_from_db + + def tracing_projection(path): + projections.append(time.monotonic()) + return real_projection(path) + + monkeypatch.setattr(gw, "_get_agent_sessions_from_db", tracing_projection) + watcher = gw.GatewayWatcher(state_db_path=db) + watcher.POLL_INTERVAL = 0.05 + watcher.start() + try: + q = watcher.subscribe() + deadline = time.monotonic() + 2.0 + while not projections and time.monotonic() < deadline: + time.sleep(0.01) + assert len(projections) == 1 + + watcher.unsubscribe(q) + time.sleep(watcher.POLL_INTERVAL * 4) # loop parks; no further projection + assert len(projections) == 1 + + watcher.subscribe() + deadline = time.monotonic() + 2.0 + while len(projections) < 2 and time.monotonic() < deadline: + time.sleep(0.01) + assert len(projections) == 2, "reconnecting subscriber must get a fresh projection" + finally: + watcher.stop() From 986e0fdd85b121d300b97fcab0ed0c4f5c623d3d Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Wed, 23 Sep 2026 10:00:57 +0000 Subject: [PATCH 21/29] fix(gateway-watcher): fence stale projections after final subscriber removal --- api/gateway_watcher.py | 66 +++++++------- docs/sse-streams.md | 15 ++++ .../test_gateway_watcher_resubscribe_fresh.py | 90 ++++++++++++++++++- 3 files changed, 139 insertions(+), 32 deletions(-) diff --git a/api/gateway_watcher.py b/api/gateway_watcher.py index b112883d..69b1bbb6 100644 --- a/api/gateway_watcher.py +++ b/api/gateway_watcher.py @@ -203,6 +203,8 @@ class GatewayWatcher: ): self._subscribers: list[queue.Queue] = [] self._sub_lock = threading.Lock() + # Final removal invalidates any projection admitted by an earlier cohort. + self._subscriber_epoch = 0 self._stop_event = threading.Event() # Wakes a poll loop parked because nobody is subscribed (subscribe/stop). self._idle_wake = threading.Event() @@ -286,31 +288,32 @@ class GatewayWatcher: self._idle_wake.set() return q + def _remove_subscriber_locked(self, q: queue.Queue) -> bool: + """Remove a known queue under _sub_lock; fence polls on the last removal.""" + try: + self._subscribers.remove(q) + except ValueError: + return False + if not self._subscribers: + self._subscriber_epoch += 1 + self._last_cheap_fp = '' + self._last_full_projection_at = None + return True + def unsubscribe(self, q: queue.Queue): - """Remove a subscriber queue. - - When the last subscriber leaves, drop the cheap fingerprint and parity - timestamp so the next subscription runs a fresh projection. The poll - loop parks *before* calling ``_poll_once`` once nobody is subscribed, - so the equivalent reset inside ``_poll_once`` is not reached on the - normal disconnect path. - """ + """Remove a subscriber queue, invalidating projection on the last removal.""" with self._sub_lock: - try: - self._subscribers.remove(q) - except ValueError: - return - if not self._subscribers: - self._last_cheap_fp = '' - self._last_full_projection_at = None + self._remove_subscriber_locked(q) - def _notify_subscribers(self, sessions: list): + def _notify_subscribers(self, sessions: list, *, epoch: int | None = None): """Push change event to all subscribers.""" event = { 'type': 'sessions_changed', 'sessions': sessions, } with self._sub_lock: + if epoch is not None and epoch != self._subscriber_epoch: + return # An old poll must not notify a new subscriber cohort. dead = [] for q in self._subscribers: try: @@ -320,10 +323,7 @@ class GatewayWatcher: except Exception: dead.append(q) for q in dead: - try: - self._subscribers.remove(q) - except ValueError: - pass + self._remove_subscriber_locked(q) # Send a None sentinel so the SSE handler unblocks, closes, # and lets the browser's EventSource auto-reconnect. try: @@ -340,9 +340,9 @@ class GatewayWatcher: """ with self._sub_lock: has_subscribers = bool(self._subscribers) + admission_epoch = self._subscriber_epoch if not has_subscribers: - # Force fresh projection on the next subscription, without touching DB. - self._last_full_projection_at = None + # Final removal already invalidated the cache under the same lock. return False db_path = self._state_db_path # A watcher may start before the agent has created state.db. Publishing an @@ -371,14 +371,20 @@ class GatewayWatcher: if sessions is None: return False current_hash = _snapshot_hash(sessions) - if cheap_fp is not None: - self._last_cheap_fp = cheap_fp - self._last_full_projection_at = current_time - - if current_hash != self._last_hash: - self._last_hash = current_hash - self._last_sessions = sessions - self._notify_subscribers(sessions) + with self._sub_lock: + if admission_epoch != self._subscriber_epoch or not self._subscribers: + return False # Never restore a cache invalidated during the DB read. + if cheap_fp is not None: + self._last_cheap_fp = cheap_fp + self._last_full_projection_at = current_time + if current_hash != self._last_hash: + changed = True + self._last_hash = current_hash + self._last_sessions = sessions + else: + changed = False + if changed: + self._notify_subscribers(sessions, epoch=admission_epoch) return True def _poll_loop(self): diff --git a/docs/sse-streams.md b/docs/sse-streams.md index 07fca5b6..12eb3898 100644 --- a/docs/sse-streams.md +++ b/docs/sse-streams.md @@ -56,6 +56,21 @@ infer it from the status code. Clients that only need session updates should use `/api/session/stream` directly; the gateway probe is only relevant for clients that display CLI/TUI/messaging sessions. +## Gateway watcher projection lifecycle + +The optional gateway stream's watcher owns a cheap database fingerprint and a +timestamp for its last full session projection. While clients are subscribed, +an unchanged fingerprint can skip the expensive read until the bounded parity +interval. With no subscribers the poll loop parks instead of reading the DB. +When the final subscriber leaves (including eviction of a full or failed +consumer queue), the watcher invalidates both cache fields. A reconnect must +therefore trigger a fresh projection even when the database fingerprint has not +changed and the parity interval has not elapsed. Final removal also advances a +subscriber epoch: an older in-flight DB read cannot restore the invalidated +cache or publish its result to a later subscriber cohort. The subscriber lock +protects removal, epoch checks and cache commits, but is not held during DB +reads, so disconnect is not blocked by a slow projection. + ## Heartbeats and proxy behavior - All long-lived streams emit SSE keepalive comment lines on the diff --git a/tests/test_gateway_watcher_resubscribe_fresh.py b/tests/test_gateway_watcher_resubscribe_fresh.py index af62a699..01e9baa8 100644 --- a/tests/test_gateway_watcher_resubscribe_fresh.py +++ b/tests/test_gateway_watcher_resubscribe_fresh.py @@ -5,16 +5,19 @@ change (#7694) the poll loop parks *before* calling ``_poll_once`` once the last subscriber leaves, so that reset is not reached on a normal disconnect. A tab that reconnects inside the parity interval then resumes with the stale -cheap fingerprint and skips the projection. The reset now happens in -``unsubscribe()`` when the last subscriber leaves. +cheap fingerprint and skips the projection. Final removal invalidates the +cache, and an epoch fences projections already in flight. """ from __future__ import annotations import importlib import sqlite3 +import threading import time from pathlib import Path +import pytest + def _make_db(tmp_path: Path) -> Path: db = tmp_path / "state.db" @@ -101,6 +104,89 @@ def test_unsubscribe_of_unknown_queue_does_not_reset(tmp_path): assert watcher._last_full_projection_at == 1.0 +def test_slow_consumer_eviction_invalidates_populated_cache(tmp_path): + gw = importlib.import_module("api.gateway_watcher") + watcher = gw.GatewayWatcher(state_db_path=_make_db(tmp_path)) + q = watcher.subscribe() + assert watcher._poll_once(now=1.0) is True + assert watcher._last_cheap_fp + assert watcher._last_full_projection_at == 1.0 + + for _ in range(q.maxsize - q.qsize()): + q.put_nowait({"type": "existing"}) + watcher._notify_subscribers([]) + assert not watcher._has_subscribers() + assert watcher._last_cheap_fp == "" + assert watcher._last_full_projection_at is None + watcher.unsubscribe(q) # The late SSE cleanup cannot undo the reset. + watcher.subscribe() + assert watcher._poll_once(now=2.0) is True + + +@pytest.mark.parametrize("removal", ["unsubscribe", "slow_consumer"]) +@pytest.mark.parametrize("reconnect_while_blocked", [False, True]) +def test_inflight_projection_cannot_restore_cache_after_last_removal( + tmp_path, monkeypatch, removal, reconnect_while_blocked, +): + """The DB read must not serialize final removal or commit across its epoch.""" + gw = importlib.import_module("api.gateway_watcher") + watcher = gw.GatewayWatcher(state_db_path=_make_db(tmp_path)) + q = watcher.subscribe() + entered = threading.Event() + release = threading.Event() + projections = [] + errors = [] + real_projection = gw._get_agent_sessions_from_db + + def blocked_projection(path): + projections.append(path) + if len(projections) == 1: + entered.set() + assert release.wait(5), "test never released the DB read" + return real_projection(path) + + def poll(): + try: + watcher._poll_once(now=1.0) + except BaseException as exc: + errors.append(exc) + + monkeypatch.setattr(gw, "_get_agent_sessions_from_db", blocked_projection) + worker = threading.Thread(target=poll, daemon=True) + worker.start() + new_q = None + try: + assert entered.wait(5), "poll never reached the DB read" + if removal == "unsubscribe": + watcher.unsubscribe(q) + else: + for _ in range(q.maxsize): + q.put_nowait({"type": "existing"}) + watcher._notify_subscribers([]) + # Handler teardown of an evicted queue must be harmless. + watcher.unsubscribe(q) + assert not watcher._has_subscribers() + assert watcher._last_cheap_fp == "" + assert watcher._last_full_projection_at is None + if reconnect_while_blocked: + new_q = watcher.subscribe() + finally: + release.set() + worker.join(timeout=5) + assert not worker.is_alive(), "poll did not finish" + assert not errors, errors + assert watcher._last_cheap_fp == "" + assert watcher._last_full_projection_at is None + if new_q is not None: + assert new_q.empty(), "old projection must not notify a new subscriber" + else: + watcher.subscribe() + assert watcher._poll_once(now=2.0) is True + assert len(projections) == 2 + assert watcher._last_cheap_fp + assert watcher._last_full_projection_at == 2.0 + + def test_poll_loop_reprojects_for_reconnecting_subscriber(tmp_path, monkeypatch): """Through the real loop: disconnect, park, reconnect -> projection runs again.""" gw = importlib.import_module("api.gateway_watcher") From 0ff718bb111d9574fca4bb0f9051c506799936d6 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 10:16:42 +0000 Subject: [PATCH 22/29] docs(changelog): #7656 resolve-concurrency setting --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885f1321..f8b88f32 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 From b43a3b4d431e7843a00dafb5abd7c431aa39b3ba Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 10:23:03 +0000 Subject: [PATCH 23/29] docs(changelog): #7165 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885f1321..def13a61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ ### Fixed +- **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 From c767ac70b3beb4b5189f7b145ed3649f865da3b6 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 10:23:04 +0000 Subject: [PATCH 24/29] docs(changelog): #7730 --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885f1321..15ba9d8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ ### Fixed +- **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) - **`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 From 0327db23ef6221bf6add006e0868986ea2fbccdf Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 10:41:22 +0000 Subject: [PATCH 25/29] docs(changelog): #7758 --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885f1321..02ba2a97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,12 @@ ### Fixed +- **A burst of "session busy" refusals no longer drops a background-task completion.** When + `start_session_turn()` refused a completion wake-up with a transient 409 (Agent runtime stale, + process wake-ups paused, or the session busy with another turn), the bridge released the durable + 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) - **`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 From 75554fd7d68a65e0fb10f4c2777f2c81e30263e9 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 10:41:22 +0000 Subject: [PATCH 26/29] docs(changelog): #7761 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885f1321..97ddc30b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ ### Fixed +- **A stale in-flight projection can no longer reach a gateway watcher after its last subscriber + leaves.** Final unsubscribe and queue eviction now invalidate the cache and fence projections that + were already in flight, without holding the lock across database reads or SSE writes, so a client + that re-subscribes gets a fresh snapshot instead of a stale one. (#7761 by @ruizanthony) - **`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 From 9e8426236a70c94e51d17e51de2b23705484bbd6 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 10:41:22 +0000 Subject: [PATCH 27/29] docs(changelog): #7760 --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 885f1321..aac83683 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,12 @@ ### Fixed +- **Waiting on the Agent's session lease is shown as a warning instead of looking stuck.** When + another Hermes process (gateway, CLI or cron) holds the session's turn lease, the Agent's + "another Hermes process is using this session" notices now reach the chat as a warning status + instead of being dropped, and the status clears when the run ends. Classification keys on the Agent + status kind (`lifecycle` / `warn`), so user-authored text can never be promoted to a warning. + (#7760 by @ruizanthony) - **`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 From 5466d09e91548e07b3eac910f99bd09d31fc9c70 Mon Sep 17 00:00:00 2001 From: happy5318 <5318happy@users.noreply.github.com> Date: Wed, 23 Sep 2026 21:23:32 +0800 Subject: [PATCH 28/29] fix(recovery): map vanished-session KeyError to the skip path in WebUI startup recovery (#7753) --- api/background_process.py | 17 ++- ...est_background_process_restart_recovery.py | 102 +++++++++++++++++- 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/api/background_process.py b/api/background_process.py index 6e6323af..eea15ebb 100644 --- a/api/background_process.py +++ b/api/background_process.py @@ -1883,7 +1883,22 @@ def recover_processes_for_webui(process_registry=None, get_session_fn=None) -> i try: proc_session = process_registry.get(process_id) session_key = str(getattr(proc_session, "session_key", "") or "") - if not session_key or get_session_fn(session_key, metadata_only=True) is None: + if not session_key: + continue + # The session resolver (``api.models._resolve_session_once``) + # raises ``KeyError(sid)`` for a missing session — that is its + # documented contract for the "owner gone" outcome, not a + # fault. Treat it identically to a ``None`` return: skip the + # process without logging, and let a neighbouring live + # process still rebind. The narrow ``except KeyError`` lives + # INSIDE the outer try so an unrelated ``KeyError`` from + # ``process_registry.get()`` (or any routing-index work) still + # surfaces in the existing warning path. + try: + resolved = get_session_fn(session_key, metadata_only=True) + except KeyError: + continue + if resolved is None: continue except Exception: logger.warning( diff --git a/tests/test_background_process_restart_recovery.py b/tests/test_background_process_restart_recovery.py index 0ab76e36..45ee82ff 100644 --- a/tests/test_background_process_restart_recovery.py +++ b/tests/test_background_process_restart_recovery.py @@ -1,5 +1,6 @@ """Regression coverage for notify_on_complete across a WebUI restart.""" +import logging import threading import time from types import SimpleNamespace @@ -158,4 +159,103 @@ def test_recovery_is_fail_soft_without_agent(monkeypatch): monkeypatch.setattr("builtins.__import__", fake_import) assert bp.recover_processes_for_webui() == 0 - assert bp._PROCESS_RECOVERY_DONE is False \ No newline at end of file + assert bp._PROCESS_RECOVERY_DONE is False + + +def test_recovery_skips_process_whose_session_is_gone(monkeypatch, caplog): + """A checkpointed process whose owner has been pruned (the resolver's + ``KeyError(sid)`` contract) must be silently skipped, while a neighbouring + live process in the same recovery sweep still rebinds. Regression for + issue #7753: previously every vanished owner logged a full WARNING + traceback, indistinguishable from a real fault. + """ + calls = {"registered": []} + + class FakeRegistry: + def recover_from_checkpoint(self): + return 2 # two checkpointed processes survive restart + + def list_sessions(self): + return [ + {"session_id": "proc_vanished", "detached": True}, + {"session_id": "proc_live", "detached": True}, + ] + + def get(self, process_id): + if process_id == "proc_vanished": + return _FakeProcessSession("vanished-session") + if process_id == "proc_live": + return _FakeProcessSession("live-session") + raise AssertionError(process_id) + + fake_registry = FakeRegistry() + monkeypatch.setattr(bp, "_PROCESS_CHECKPOINT_RECOVERED", False) + monkeypatch.setattr(bp, "_PROCESS_RECOVERY_DONE", False) + monkeypatch.setattr( + bp, + "register_process_session", + lambda key, sid: calls["registered"].append((key, sid)), + ) + + def get_session(sid, metadata_only=False): + # Mirrors api.models._resolve_session_once's contract: missing + # sessions raise KeyError, NOT None. + if sid == "vanished-session": + raise KeyError(sid) + return SimpleNamespace(id=sid) + + with caplog.at_level(logging.WARNING, logger="api.background_process"): + result = bp.recover_processes_for_webui(fake_registry, get_session) + + assert result == 2 + # The live process is rebound; the vanished one is silently skipped. + assert calls["registered"] == [("live-session", "live-session")] + # No WARNING emitted for the vanished-owner case. + warning_records = [ + r for r in caplog.records if r.levelno >= logging.WARNING + ] + assert warning_records == [], ( + "vanished-session KeyError must not produce a WARNING, got: " + f"{[r.getMessage() for r in warning_records]}" + ) + assert "Could not resolve recovered WebUI process" not in caplog.text + + +def test_recovery_still_warns_on_unexpected_resolve_failure(monkeypatch, caplog): + """Non-KeyError failures from the resolver (e.g. ``OSError`` on an + unreadable sidecar) must keep entering the existing WARNING path. This + pins that the fix for #7753 is narrow and does not over-broaden the + silent-skip contract. + """ + class FakeRegistry: + def recover_from_checkpoint(self): + return 1 + + def list_sessions(self): + return [{"session_id": "proc_unexpected", "detached": True}] + + def get(self, process_id): + assert process_id == "proc_unexpected" + return _FakeProcessSession("webui-session") + + fake_registry = FakeRegistry() + monkeypatch.setattr(bp, "_PROCESS_CHECKPOINT_RECOVERED", False) + monkeypatch.setattr(bp, "_PROCESS_RECOVERY_DONE", False) + + def get_session(sid, metadata_only=False): + raise OSError("sidecar unreadable") + + with caplog.at_level(logging.WARNING, logger="api.background_process"): + result = bp.recover_processes_for_webui(fake_registry, get_session) + + assert result == 1 + warning_records = [ + r for r in caplog.records if r.levelno >= logging.WARNING + ] + assert any( + "Could not resolve recovered WebUI process 'proc_unexpected'" in r.getMessage() + for r in warning_records + ), ( + "OSError from the resolver must still emit a WARNING, got: " + f"{[r.getMessage() for r in warning_records]}" + ) \ No newline at end of file From ef7d325a5484d2db32a66ce753c12791cc414eab Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 23 Sep 2026 14:34:41 +0000 Subject: [PATCH 29/29] docs(changelog): #7774 --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bb2efc3..e0bfaa58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ ### Fixed +- **A session deleted during a restart no longer produces a spurious recovery warning.** When + WebUI startup recovery re-attached background processes, a session that had vanished between + enumeration and rebind raised a `KeyError` that was logged as a warning. It now follows the + existing skip path, confined to the session lookup, so the vanished owner is skipped, live owners + still rebind, and registry errors still warn. (#7753, #7774 by @happy5318) + - **Waiting on the Agent's session lease is shown as a warning instead of looking stuck.** When another Hermes process (gateway, CLI or cron) holds the session's turn lease, the Agent's "another Hermes process is using this session" notices now reach the chat as a warning status