diff --git a/docs/pr-media/1771/session-model-fallback.png b/docs/pr-media/1771/session-model-fallback.png new file mode 100644 index 00000000..e16a3538 Binary files /dev/null and b/docs/pr-media/1771/session-model-fallback.png differ diff --git a/static/ui.js b/static/ui.js index 67f27ac8..3ad6b706 100644 --- a/static/ui.js +++ b/static/ui.js @@ -516,16 +516,58 @@ function _findModelInDropdown(modelId, sel, preferredProviderId){ // Set the model picker to the best match for modelId. // Returns the resolved value that was actually set, or null if nothing matched. +function _refreshOpenModelDropdown(){ + const dd=$('composerModelDropdown'); + if(dd&&dd.classList&&dd.classList.contains('open')&&typeof renderModelDropdown==='function'){ + renderModelDropdown(); + if(typeof _positionModelDropdown==='function') _positionModelDropdown(); + } +} function _applyModelToDropdown(modelId, sel, preferredProviderId){ if(!modelId||!sel) return null; const resolved=_findModelInDropdown(modelId,sel,preferredProviderId); if(resolved){ sel.value=resolved; - if(sel.id==='modelSelect' && typeof syncModelChip==='function') syncModelChip(); + if(sel.id==='modelSelect'){ + if(typeof syncModelChip==='function') syncModelChip(); + _refreshOpenModelDropdown(); + } return resolved; } return null; } +function _modelStateFromAppliedDropdown(sel, modelValue){ + const state=(typeof _modelStateForSelect==='function') + ? _modelStateForSelect(sel,modelValue) + : {model:modelValue,model_provider:null}; + return {model:state.model||modelValue,model_provider:state.model_provider||null}; +} +function _persistSessionModelCorrection(model, provider){ + if(!S.session) return; + fetch(new URL('api/session/update',document.baseURI||location.href).href,{ + method:'POST',credentials:'include', + headers:{'Content-Type':'application/json'}, + body:JSON.stringify({session_id:S.session.id||S.session.session_id,model:model,model_provider:provider||null}) + }).catch(()=>{}); +} +function _applySessionModelFallback(sel){ + if(!sel) return null; + const configuredDefault=String(window._defaultModel||'').trim(); + if(configuredDefault){ + const appliedDefault=_applyModelToDropdown(configuredDefault,sel,window._activeProvider||null); + if(appliedDefault) return _modelStateFromAppliedDropdown(sel,appliedDefault); + } + const first=sel.querySelector('optgroup > option, option'); + if(first){ + sel.value=first.value; + if(sel.id==='modelSelect'){ + if(typeof syncModelChip==='function') syncModelChip(); + _refreshOpenModelDropdown(); + } + return _modelStateFromAppliedDropdown(sel,first.value); + } + return null; +} async function populateModelDropdown(){ const sel=$('modelSelect'); @@ -3660,35 +3702,41 @@ function syncTopbar(){ _applyModelToDropdown(modelOverride,$('modelSelect'),providerOverride); currentModel=modelOverride; } else { - const applied=_applyModelToDropdown(currentModel,$('modelSelect'),S.session.model_provider||null); - // If the model isn't in the current provider list, silently reset to the - // first available model so stale values don't pollute the picker (#829). - if(!applied && currentModel){ - const deferModelCorrection=Boolean(S.session._modelResolutionDeferred); - // Also defer if a live model fetch is still in flight — the model may be - // in the list once the fetch completes. Persisting now would corrupt the - // session with the wrong model before live models arrive (#1169). - const liveStillPending=window._activeProvider&&_liveModelFetchPending.has(window._activeProvider); - if(liveStillPending){ - // Live fetch in flight — don't touch sel.value or S.session.model yet. - // _addLiveModelsToSelect() will re-apply S.session.model once done (#1169). - } else { - // Stale session model not in the current provider catalog — reset to the - // first available model rather than injecting an "(unavailable)" option - // that visually appears under the wrong provider group (#829). - const modelSel=$('modelSelect'); - const first=modelSel&&modelSel.querySelector('optgroup > option, option'); - if(first){ - modelSel.value=first.value; - if(!deferModelCorrection){ - S.session.model=first.value; - S.session.model_provider=_getOptionProviderId(first)||null; + const modelSel=$('modelSelect'); + const rawCurrentModel=String(currentModel||'').trim(); + const hasSessionModel=rawCurrentModel&&rawCurrentModel.toLowerCase()!=='unknown'; + if(!hasSessionModel){ + // Missing/unknown session metadata must not leave the picker on the + // previously viewed chat's model (#1771). Apply the configured default + // first, then the first available option only as an HTML fallback. + const fallback=_applySessionModelFallback(modelSel); + if(fallback){ + S.session.model=fallback.model; + S.session.model_provider=fallback.model_provider||null; + currentModel=fallback.model; + _persistSessionModelCorrection(fallback.model,S.session.model_provider||null); + } + } else { + const applied=_applyModelToDropdown(currentModel,modelSel,S.session.model_provider||null); + // If the model isn't in the current provider list, reset to the configured + // default rather than silently retaining the previous chat's selection (#1771). + if(!applied){ + const deferModelCorrection=Boolean(S.session._modelResolutionDeferred); + // Also defer if a live model fetch is still in flight — the model may be + // in the list once the fetch completes. Persisting now would corrupt the + // session with the wrong model before live models arrive (#1169). + const liveStillPending=window._activeProvider&&_liveModelFetchPending.has(window._activeProvider); + if(liveStillPending){ + // Live fetch in flight — don't touch sel.value or S.session.model yet. + // _addLiveModelsToSelect() will re-apply S.session.model once done (#1169). + } else { + const fallback=_applySessionModelFallback(modelSel); + if(fallback&&!deferModelCorrection){ + S.session.model=fallback.model; + S.session.model_provider=fallback.model_provider||null; + currentModel=fallback.model; // Persist the correction so the session doesn't re-inject on next load. - fetch(new URL('api/session/update',document.baseURI||location.href).href,{ - method:'POST',credentials:'include', - headers:{'Content-Type':'application/json'}, - body:JSON.stringify({session_id:S.session.id||S.session.session_id,model:first.value,model_provider:S.session.model_provider||null}) - }).catch(()=>{}); + _persistSessionModelCorrection(fallback.model,S.session.model_provider||null); } } } diff --git a/tests/test_issue1771_session_model_switch_sync.py b/tests/test_issue1771_session_model_switch_sync.py new file mode 100644 index 00000000..7384387d --- /dev/null +++ b/tests/test_issue1771_session_model_switch_sync.py @@ -0,0 +1,192 @@ +""" +Regression tests for issue #1771: switching sessions with missing/stale model +metadata must not leave the composer model picker on the previously viewed +chat's model. + +These tests execute the real static/ui.js syncTopbar() path in Node with a tiny +DOM/select shim so the behavioral contract is protected without needing a full +browser harness. +""" +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[1] +UI_JS_PATH = REPO_ROOT / "static" / "ui.js" +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +_DRIVER_SRC = r""" +const fs = require('fs'); +const ui = fs.readFileSync(process.argv[2], 'utf8'); + +function extractFunc(name, opts = {}) { + const re = new RegExp('function\\s+' + name + '\\s*\\('); + const start = ui.search(re); + if (start < 0) { + if (opts.optional) return ''; + throw new Error(name + ' not found'); + } + let i = ui.indexOf('{', start); + let depth = 1; + i++; + while (depth > 0 && i < ui.length) { + if (ui[i] === '{') depth++; + else if (ui[i] === '}') depth--; + i++; + } + return ui.slice(start, i); +} + +const calls = {syncModelChip: 0, renderModelDropdown: 0, positionModelDropdown: 0, fetches: []}; +let modelSelect; +let dropdownOpen = false; +const dropdown = {classList: {contains: (name) => name === 'open' && dropdownOpen}}; + +function makeSelect(options, initialValue) { + const sel = {id: 'modelSelect', options: [], selectedIndex: -1, selectedOptions: []}; + Object.defineProperty(sel, 'value', { + get() { return this._value || ''; }, + set(v) { + this._value = v; + const idx = this.options.findIndex(o => o.value === v); + this.selectedIndex = idx; + this.selectedOptions = idx >= 0 ? [this.options[idx]] : []; + } + }); + sel.querySelector = function(_selector) { return this.options[0] || null; }; + for (const item of options) { + const group = {tagName: 'OPTGROUP', dataset: {provider: item.provider || ''}}; + const opt = {value: item.value, textContent: item.label || item.value, parentElement: group, dataset: {}}; + sel.options.push(opt); + } + sel.value = initialValue || ''; + return sel; +} + +function $(id) { + if (id === 'modelSelect') return modelSelect; + if (id === 'composerModelDropdown') return dropdown; + return {textContent: '', style: {}, classList: {add(){}, remove(){}, toggle(){}, contains(){return false;}}, appendChild(){}, appendChildNode(){}}; +} +function t(key) { return key; } +function syncModelChip() { calls.syncModelChip++; } +function renderModelDropdown() { calls.renderModelDropdown++; } +function _positionModelDropdown() { calls.positionModelDropdown++; } +function syncAppTitlebar() {} +function syncWorkspaceDisplays() {} +function syncReasoningChip() {} +function syncToolsetsChip() {} +function syncTerminalButton() {} +function _syncHermesPanelSessionActions() {} +function _latestGatewayRoutingForSession() { return null; } +function getModelLabel(v) { return v; } +function _formatGatewayModelLabel(_v, text) { return text; } +const _liveModelFetchPending = new Set(); +const document = { + title: '', + baseURI: 'http://127.0.0.1/hermes/', + createElement(tag) { return {tagName: tag.toUpperCase(), className: '', textContent: '', appendChild(){}}; }, + createTextNode(text) { return {textContent: text}; }, +}; +const window = { _botName: 'Hermes', _defaultModel: null, _activeProvider: null }; +function fetch(url, opts) { calls.fetches.push({url: String(url), body: opts && opts.body || ''}); return Promise.resolve({ok: true}); } + +for (const name of [ + '_getOptionProviderId', '_providerFromModelValue', '_modelStateForSelect', + '_findModelInDropdown', '_refreshOpenModelDropdown', '_applyModelToDropdown', + '_modelStateFromAppliedDropdown', '_persistSessionModelCorrection', + '_applySessionModelFallback', 'syncTopbar' +]) { + const src = extractFunc(name, {optional: name !== 'syncTopbar'}); + if (src) eval(src); +} + +const args = JSON.parse(process.argv[3]); +modelSelect = makeSelect(args.options, args.initialValue); +dropdownOpen = !!args.dropdownOpen; +window._defaultModel = args.defaultModel || null; +window._activeProvider = args.activeProvider || null; +var S = { + session: { + session_id: 'session-b', + id: 'session-b', + title: 'Session B', + model: args.sessionModel, + model_provider: args.sessionProvider || null, + messages: [], + }, + messages: [], + activeProfile: 'default', +}; + +syncTopbar(); + +process.stdout.write(JSON.stringify({ + selectValue: modelSelect.value, + sessionModel: S.session.model, + sessionProvider: S.session.model_provider, + calls, +})); +""" + + +@pytest.fixture(scope="module") +def driver_path(tmp_path_factory): + p = tmp_path_factory.mktemp("issue1771_driver") / "driver.js" + p.write_text(_DRIVER_SRC, encoding="utf-8") + return str(p) + + +def _run_sync(driver_path, *, session_model, initial_value="@expensive:gpt-5.5", default_model="@safe:gpt-4o-mini", dropdown_open=False): + payload = { + "sessionModel": session_model, + "sessionProvider": None, + "initialValue": initial_value, + "defaultModel": default_model, + "activeProvider": "safe", + "dropdownOpen": dropdown_open, + "options": [ + {"provider": "expensive", "value": "@expensive:gpt-5.5", "label": "GPT-5.5"}, + {"provider": "safe", "value": "@safe:gpt-4o-mini", "label": "GPT-4o mini"}, + ], + } + result = subprocess.run( + [NODE, driver_path, str(UI_JS_PATH), json.dumps(payload)], + capture_output=True, + text=True, + timeout=10, + ) + if result.returncode != 0: + raise RuntimeError(f"node driver failed:\nSTDOUT={result.stdout}\nSTDERR={result.stderr}") + return json.loads(result.stdout) + + +def test_sync_topbar_missing_model_falls_back_to_configured_default_not_previous_chat(driver_path): + got = _run_sync(driver_path, session_model="") + + assert got["selectValue"] == "@safe:gpt-4o-mini" + assert got["sessionModel"] == "@safe:gpt-4o-mini" + assert got["sessionProvider"] == "safe" + assert got["selectValue"] != "@expensive:gpt-5.5" + + +def test_sync_topbar_unknown_model_falls_back_to_configured_default_not_first_option(driver_path): + got = _run_sync(driver_path, session_model="unknown") + + assert got["selectValue"] == "@safe:gpt-4o-mini" + assert got["sessionModel"] == "@safe:gpt-4o-mini" + assert got["sessionProvider"] == "safe" + + +def test_sync_topbar_rerenders_open_visible_model_dropdown_after_session_model_change(driver_path): + got = _run_sync(driver_path, session_model="", dropdown_open=True) + + assert got["selectValue"] == "@safe:gpt-4o-mini" + assert got["calls"]["renderModelDropdown"] >= 1 + assert got["calls"]["positionModelDropdown"] >= 1 diff --git a/tests/test_provider_mismatch.py b/tests/test_provider_mismatch.py index 632bf0fe..bc00a3b8 100644 --- a/tests/test_provider_mismatch.py +++ b/tests/test_provider_mismatch.py @@ -1344,8 +1344,11 @@ def test_stale_ui_js_does_not_inject_unavailable_option(): "stale models should be silently reset to the first available model (#829)" ) - # The new silent-reset pattern must be present - assert "first.value" in src and "S.session.model=first.value" in src, ( - "renderSession() must silently reset S.session.model to the first " - "available option when the session model is not in the dropdown (#829)" + # The reset path remains, but #1771 now prefers the configured default + # before using the first HTML option as a last-resort fallback. + assert "_applySessionModelFallback" in src and "configuredDefault" in src, ( + "stale session models should be reset through the safe fallback helper" + ) + assert "const first=sel.querySelector('optgroup > option, option');" in src, ( + "the first available option should remain only as a fallback when no configured default applies" ) diff --git a/tests/test_session_metadata_fast_path.py b/tests/test_session_metadata_fast_path.py index b967a288..c4e5e719 100644 --- a/tests/test_session_metadata_fast_path.py +++ b/tests/test_session_metadata_fast_path.py @@ -39,7 +39,7 @@ def test_session_switch_defers_model_resolution_without_blocking(): assert "messages=0&resolve_model=1" in src assert "_modelResolutionDeferred=true" in src assert "deferModelCorrection" in ui - assert "if(!deferModelCorrection)" in ui + assert "if(fallback&&!deferModelCorrection)" in ui def test_boot_does_not_block_session_restore_on_model_catalog():