fix(tagging): refuse non-text default ids and keep one listener per list (#431)

A default instructions id that is not text is a 400 on save and falls back
with a warning on load; a damaged list warns once and migrates the old
template afresh. Option clicks use one listener on the list, the reset
note clears once the defaults are saved, and a duplicated long name is cut
to fit.

py/config.py

@@ -775,17 +775,23 @@ class AutotagConfig:
                     f"autotag.defaults.template {error}; using the built-in one"
                 )
         entries: List[Dict[str, str]] = []
+        damaged = False
         if "instructions" in defaults:
             try:
                 entries = validate_instructions(defaults["instructions"])
             except ValueError as error:
+                damaged = True
                 config_logger.warning(
                     f"autotag.defaults.instructions {_reason(error)}; "
                     "using Built-in only"
                 )
         chosen = {engine: BUILTIN_INSTRUCTIONS_ID for engine in INSTRUCTION_ENGINES}
         stored = defaults.get("default_instructions")
-        if "default_instructions" in defaults and (
+        if damaged:
+            # Both defaults fall back with the list (its one warning says so);
+            # the old template is migrated as on a first upgrade.
+            pass
+        elif "default_instructions" in defaults and (
             not isinstance(stored, dict) or set(stored) != set(INSTRUCTION_ENGINES)
         ):
             config_logger.warning(
@@ -795,8 +801,11 @@ class AutotagConfig:
         elif isinstance(stored, dict):
             ids = {item["id"] for item in entries}
             for engine in INSTRUCTION_ENGINES:
-                if stored[engine] == BUILTIN_INSTRUCTIONS_ID or stored[engine] in ids:
-                    chosen[engine] = stored[engine]
+                value = stored[engine]
+                if isinstance(value, str) and (
+                    value == BUILTIN_INSTRUCTIONS_ID or value in ids
+                ):
+                    chosen[engine] = value
                 else:
                     config_logger.warning(
                         f"autotag.defaults.default_instructions.{engine} names no "
@@ -805,8 +814,8 @@ class AutotagConfig:
         cls._migrate_template(
             entries,
             chosen,
-            first="instructions" not in defaults,
-            keep_defaults="default_instructions" in defaults,
+            first=damaged or "instructions" not in defaults,
+            keep_defaults=not damaged and "default_instructions" in defaults,
         )
         cls.INSTRUCTIONS = tuple(entries)
         cls.DEFAULT_INSTRUCTIONS = chosen

py/tagging_defaults.py

@@ -314,7 +314,7 @@ def validate_default_instructions(value: Any, entries) -> Dict[str, str]:
         raise ValueError("Default instructions must name GGUF and HuggingFace.")
     ids = {BUILTIN_INSTRUCTIONS_ID} | {item["id"] for item in entries}
     for engine in INSTRUCTION_ENGINES:
-        if value[engine] not in ids:
+        if not isinstance(value[engine], str) or value[engine] not in ids:
             raise ValueError(
                 f"Default for {ENGINE_LABELS[engine]} must be one of the saved "
                 "instructions."

tests/js/workspace-system-instructions.test.js

@@ -83,7 +83,9 @@ async function mounted({ host = HOST } = {}) {
     const options = () => byId('instruction-list').children.map(option => [option.children[0].textContent,
         option.children[1]?.children.map(badge => badge.textContent) ?? [], option.getAttribute('aria-selected')]);
     const click = id => byId(id).dispatch('click');
-    const pick = name => byId('instruction-list').children.find(option => option.children[0].textContent === name).dispatch('click');
+    // A click on an option reaches the list's one listener (as it bubbles in a browser).
+    const pick = name => byId('instruction-list').dispatch('click', { target: byId('instruction-list').children
+        .find(option => option.children[0].textContent === name).children[0] });
     return { dom, drafts, scope, block, byId, options, click, pick, root };
 }
 
@@ -222,3 +224,34 @@ test('arrow keys move the selection; before the read everything is disabled', as
         assert.equal(unread.byId('instruction-default-gguf').disabled, true);
     } finally { unread.scope.dispose(); unread.dom.restore(); }
 });
+
+test('the reset note goes once the defaults are saved; a long name is cut to fit its copy suffix', async () => {
+    const view = await mounted();
+    try {
+        await view.pick('Custom template');
+        await view.click('instruction-delete');
+        assert.equal(view.byId('instruction-reset').textContent, 'GGUF and HuggingFace now use Built-in.');
+        await view.drafts.save();
+        assert.equal(view.byId('instruction-reset').textContent, '');
+        await view.pick('Anime');
+        view.byId('instruction-name').value = 'x'.repeat(60);
+        await view.byId('instruction-name').dispatch('input');
+        await view.click('instruction-duplicate');
+        const copy = view.drafts.get('host', 'autotag_instructions').at(-1).name;
+        assert.equal(copy, `${'x'.repeat(55)} copy`);
+        await view.click('instruction-duplicate');
+        assert.equal([...view.drafts.get('host', 'autotag_instructions').at(-1).name].length <= 60, true);
+    } finally { view.scope.dispose(); view.dom.restore(); }
+});
+
+test('option clicks are one listener on the list, not one per repaint', async () => {
+    const view = await mounted();
+    try {
+        const list = view.byId('instruction-list');
+        const before = list.listeners.length;
+        for (let i = 0; i < 5; i += 1) await view.pick('Anime');
+        assert.equal(list.listeners.length, before);
+        assert.equal(view.byId('instruction-list').children.every(option => option.listeners.length === 0), true);
+        assert.equal(view.byId('instruction-name').value, 'Anime');
+    } finally { view.scope.dispose(); view.dom.restore(); }
+});

tests/test_saved_instructions.py

@@ -128,6 +128,10 @@ class TestValidation(unittest.TestCase):
                 {"gguf": "builtin"},
                 "Default instructions must name GGUF and HuggingFace.",
             ),
+            (
+                {"gguf": [], "hf": "builtin"},
+                "Default for GGUF JoyCaption must be one of the saved instructions.",
+            ),
             ("builtin", "Default instructions must name GGUF and HuggingFace."),
         ):
             with self.subTest(value=value):
@@ -232,7 +236,7 @@ class TestLoad(ConfigTestCase):
                 ],
                 "default_instructions": {"gguf": "0123456789ab", "hf": "builtin"},
             },
-            warnings=2,
+            warnings=1,  # one warning; the defaults follow the list silently
         )
         self.assertEqual(self.state(), ([], {"gguf": "builtin", "hf": "builtin"}))
         self.assertEqual(
@@ -347,5 +351,29 @@ class TestGetConfig(ConfigTestCase):
         self.assertNotIn("template", AutotagConfig.get_config()["defaults"])
 
 
+class TestDamagedValues(ConfigTestCase):
+    def test_a_non_text_default_falls_back_with_a_warning(self):
+        self.load({"default_instructions": {"gguf": [], "hf": "builtin"}}, warnings=1)
+        self.assertEqual(self.state(), ([], {"gguf": "builtin", "hf": "builtin"}))
+
+    def test_a_damaged_list_warns_once_and_migrates_the_template_afresh(self):
+        custom = "My own tags, please."
+        self.load(
+            {
+                "template": custom,
+                "instructions": "broken",
+                "default_instructions": {"gguf": "0123456789ab", "hf": "builtin"},
+            },
+            warnings=1,
+        )
+        self.assertEqual(
+            self.state(),
+            (
+                [entry(sha12(custom), "Custom template", custom)],
+                {"gguf": sha12(custom), "hf": sha12(custom)},
+            ),
+        )
+
+
 if __name__ == "__main__":
     unittest.main()

web/js/workspace/system-instructions.mjs

@@ -74,6 +74,16 @@ export function freeName(base, entries) {
     for (let n = 2; ; n += 1) if (!taken.has(fold(`${base} (${n})`))) return `${base} (${n})`;
 }
 
+/** "<name> copy" (then "(2)"…), the name cut so the whole stays within the 60-character limit. */
+export function copyName(name, entries) {
+    for (let room = MAX_NAME_CHARS - ' copy'.length; room > 0; room -= 1) {
+        const base = [...String(name ?? '').trim()].slice(0, room).join('').trimEnd();
+        const candidate = freeName(INSTRUCTIONS_COPY.copyName(base), entries);
+        if (characters(candidate) <= MAX_NAME_CHARS) return candidate;
+    }
+    return freeName(INSTRUCTIONS_COPY.copyName(BUILTIN_NAME), entries);
+}
+
 /** A new entry id: 12 lowercase hex characters from `random` (crypto.getRandomValues), never one already used. */
 export function newId(entries, random = bytes => globalThis.crypto.getRandomValues(bytes)) {
     const used = new Set(entries.map(entry => entry.id));
@@ -249,6 +259,7 @@ export function savedInstructionsBlock({ scope, drafts, random }) {
         list.replaceChildren(...entries.map(entry => {
             const option = el('div');
             option.id = `instruction-option-${entry.id}`;
+            option.dataset.id = entry.id;
             option.setAttribute('role', 'option');
             option.setAttribute('aria-selected', String(entry.id === selectedId));
             option.append(el('span', entry.name));
@@ -258,13 +269,14 @@ export function savedInstructionsBlock({ scope, drafts, random }) {
                 if (chosen[engine] === entry.id) badges.append(el('span', INSTRUCTIONS_COPY.defaultFor(short), 'chip blue'));
             }
             if (badges.children.length) option.append(badges);
-            scope.listen(option, 'click', () => select(entry.id));
             return option;
         }));
         list.setAttribute('aria-activedescendant', `instruction-option-${selectedId}`);
     }
 
     function repaint() {
+        // The note a deleted default left goes once the defaults are saved (or set back).
+        if (!Object.hasOwn(drafts.changes('host'), DEFAULTS_KEY)) reset.textContent = '';
         const loaded = ready();
         for (const control of [create, duplicate, remove, nameInput, textArea, ...selects.map(item => item.select)]) {
             control.disabled = !loaded;
@@ -310,9 +322,15 @@ export function savedInstructionsBlock({ scope, drafts, random }) {
     scope.listen(duplicate, 'click', () => {
         const entries = allEntries(values());
         const source = selected();
-        add({ id: newId(entries, random), name: freeName(INSTRUCTIONS_COPY.copyName(source.name), entries), text: source.text });
+        add({ id: newId(entries, random), name: copyName(source.name, entries), text: source.text });
     });
     scope.listen(remove, 'click', deleteSelected);
+    // One listener for every option, however often the list is rebuilt.
+    scope.listen(list, 'click', event => {
+        let node = event.target;
+        while (node && node !== list && node.getAttribute?.('role') !== 'option') node = node.parentNode;
+        if (node && node !== list) select(node.dataset.id);
+    });
     scope.listen(nameInput, 'input', () => edit('name', nameInput.value));
     scope.listen(textArea, 'input', () => edit('text', textArea.value));
     scope.listen(list, 'keydown', event => {