diff --git a/static/panels.js b/static/panels.js index 9215cc77..4f708aae 100644 --- a/static/panels.js +++ b/static/panels.js @@ -25,6 +25,7 @@ let _currentCronDetail = null; // full cron job object let _currentCronDetailKey = ''; let _cronMode = 'empty'; // 'empty' | 'read' | 'create' | 'edit' let _cronPreFormDetail = null; // snapshot of prior selection when entering a form +let _cronModelPickerTouched = false; // true once the user changes the model picker in the current form let _showAllCronProfiles = false; let _cronOtherProfileCount = 0; let _currentWorkspaceDetail = null; // { path, name, is_default } @@ -1550,6 +1551,7 @@ function duplicateCurrentCron(){ const job = _currentCronDetail; if (typeof switchPanel === 'function' && _currentPanel !== 'tasks') switchPanel('tasks'); _cronPreFormDetail = { ...job }; + _cronModelPickerTouched = false; _editingCronId = null; _cronMode = 'create'; _cronIsDuplicate = true; @@ -1606,6 +1608,7 @@ let _cronDeliveryOptionsCache=null; function openCronCreate(){ if (typeof switchPanel === 'function' && _currentPanel !== 'tasks') switchPanel('tasks'); _cronPreFormDetail = _currentCronDetail ? { ..._currentCronDetail } : null; + _cronModelPickerTouched = false; _editingCronId = null; _cronMode = 'create'; _cronIsDuplicate = false; @@ -1623,6 +1626,7 @@ function openCronCreate(){ function openCronEdit(job){ if (!job) return; _cronPreFormDetail = { ...job }; + _cronModelPickerTouched = false; _editingCronId = job.id; _cronMode = 'edit'; _cronSelectedSkills = Array.isArray(job.skills) ? [...job.skills] : []; @@ -1855,6 +1859,9 @@ async function _populateCronFormModelSelect(selectedModel, selectedProvider, dis sel.appendChild(opt); } sel.dataset.loaded = '1'; + // Track deliberate picker changes so an untouched "Default" can keep a + // hidden provider-only pin on save (see _cronProviderForClear). + sel.addEventListener('change', () => { _cronModelPickerTouched = true; }); } catch (e) { console.warn('Failed to load cron model picker:', e.message); // Load failed: dataset.loaded stays unset so saveCronForm omits model/provider @@ -1940,6 +1947,18 @@ function _cronModelBareName(model, provider) { return _modelBareNameForProvider(model, provider); } +function _cronProviderForClear(prevDetail, pickerTouched) { + // Provider to submit when the picker shows "Default" (no model) on save. + // An explicit return to Default honors "Default = no overrides" (#4030) and + // clears the provider. An untouched picker must not: provider-only jobs + // (a pinned provider with no model) render as "Default" in the combined + // picker, so saving any other field would silently erase the pin the user + // never touched. + const prev = prevDetail || {}; + if (pickerTouched) return null; + return (prev.model == null) ? prev.provider : null; +} + async function saveCronForm(){ const nameEl=$('cronFormName'); const schEl=$('cronFormSchedule'); @@ -1977,8 +1996,11 @@ async function saveCronForm(){ updates.model = _cronModelBareName(modelState.model, modelState.model_provider) || null; updates.provider = modelState.model_provider || null; } else if (modelLoaded) { + // "Default" selected (no model). An untouched picker preserves a + // hidden provider-only pin; a deliberate return to Default clears + // it (#4030 "Default = no overrides"). updates.model = null; - updates.provider = null; + updates.provider = _cronProviderForClear(_cronPreFormDetail, _cronModelPickerTouched); } // else: select not yet populated — omit model/provider to preserve saved value } diff --git a/tests/test_cron_model_override.py b/tests/test_cron_model_override.py index bf3b4b89..6c9501c8 100644 --- a/tests/test_cron_model_override.py +++ b/tests/test_cron_model_override.py @@ -8,6 +8,8 @@ import sys import types from pathlib import Path +import pytest + REPO = Path(__file__).resolve().parents[1] PANELS_JS = (REPO / "static" / "panels.js").read_text(encoding="utf-8") @@ -170,3 +172,89 @@ def test_cron_model_picker_marks_loaded_only_after_successful_population(): catch_block = body.split("} catch (e)", 1)[1] assert "sel.dataset.loaded = '1'" in try_block assert "sel.dataset.loaded = '1'" not in catch_block + + +def test_cron_update_preserves_provider_only_pin_when_model_cleared(): + """Clearing the picker to "Default" must not wipe a provider-only pin. + + #4030 intentionally made "clearing the picker = no overrides", which is + correct for model+provider pins but silently destroyed provider-only + pins (jobs that pin a provider with no model, e.g. self-hosted LLM + endpoints). The combined picker cannot represent that state, so + saveCronForm must preserve prev.provider when the job carried no model + AND the user never changed the picker; a deliberate return to Default + must clear it. + """ + save_body = _function_body("saveCronForm") + helper_body = _function_body("_cronProviderForClear") + + # The clear branch delegates to the helper and records picker changes. + assert "_cronProviderForClear(_cronPreFormDetail, _cronModelPickerTouched)" in save_body + assert "preserveProvider" not in save_body + + # The helper distinguishes an explicit clear (pickerTouched) from an + # untouched provider-only pin. + assert "if (pickerTouched) return null" in helper_body + assert "prev.model == null" in helper_body + + # The form entry points reset the touched flag. + for fn in ("openCronCreate", "openCronEdit", "duplicateCurrentCron"): + assert "_cronModelPickerTouched = false" in _function_body(fn) + # The picker records deliberate changes. + pop_body = _function_body("_populateCronFormModelSelect") + assert "_cronModelPickerTouched = true" in pop_body + + +def test_cron_provider_for_clear_behavior(): + """Exercise _cronProviderForClear through node for both pin shapes.""" + import shutil + import subprocess + + node = shutil.which("node") + if node is None: + pytest.skip("node not on PATH") + + # Locate the full declaration (signature + body) of the helper. + src = PANELS_JS + start = src.find("function _cronProviderForClear(") + assert start != -1, "helper declaration not found" + brace = src.find("{", start) + depth = 0 + end = None + for i in range(brace, len(src)): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + if depth == 0: + end = i + 1 + break + decl = src[start:end] + + driver = f""" +{decl} +const cases = [ + ['provider_only_untouched', {{model: null, provider: 'custom:llama-swap'}}, false], + ['provider_only_touched', {{model: null, provider: 'custom:llama-swap'}}, true], + ['model_pin_touched', {{model: 'gpt-5', provider: 'openai'}}, true], + ['no_override_untouched', {{model: null, provider: null}}, false], + ['no_prev', null, false], +]; +const out = {{}}; +for (const [name, detail, touched] of cases) out[name] = _cronProviderForClear(detail, touched) ?? null; +process.stdout.write(JSON.stringify(out)); +""" + proc = subprocess.run([node, "-e", driver], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node driver failed: {proc.stderr}" + r = json.loads(proc.stdout) + + # Untouched picker on a provider-only job: the pin survives. + assert r["provider_only_untouched"] == "custom:llama-swap" + # Deliberate return to Default: "Default = no overrides" wins. + assert r["provider_only_touched"] is None + # Model-pinned job, explicit clear: provider removed too. + assert r["model_pin_touched"] is None + # No overrides stored, untouched picker: nothing to preserve. + assert r["no_override_untouched"] is None + # No pre-form snapshot (create form): no provider. + assert r["no_prev"] is None