fix(cron): preserve provider-only pin on untouched save; honor explicit clear
Editing a cron job with the model picker at "Default" sent provider: null unconditionally, wiping provider-only pins (provider set, no model — the pattern for self-hosted LLM endpoints). The combined picker cannot express that state, so such jobs always rendered as "Default" and were silently re-resolved to the bare global default on any save, breaking dispatch. #4030 deliberately made clearing the picker mean "no overrides"; that contract is preserved for explicit clears and model+provider jobs, while provider-only pins survive saves where the picker was never touched. - _cronProviderForClear(prevDetail, pickerTouched) decides the provider submitted on clear: touched picker -> null; untouched + no model on the job -> prev.provider. - _cronModelPickerTouched is set by a change listener added when the picker populates, and reset at openCronEdit/openCronCreate/duplicateCurrentCron. - Tests: delegation + flag lifecycle locked in, and the helper driven through node for provider-only / model-pinned / no-override / no-snapshot inputs (6 passed). Refs: #4030
This commit is contained in:
1 parent
2ba3e40210
commit
e1f90aed09
2 files changed
+111
-1
No files matched your search
+23
-1
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
Reference in new issue
Block a user