diff --git a/changes/457.fixed.md b/changes/457.fixed.md new file mode 100644 index 000000000..607c25c55 --- /dev/null +++ b/changes/457.fixed.md @@ -0,0 +1,3 @@ +**Tapping Local only in the /model list of a Telegram or Discord chat asks first, like typing it.** It used to switch +the models without asking and leave the Local only switch off. Replying "/model Local only no" now leaves everything as +it is, instead of saying there is no setup with that name. diff --git a/sentient/channels/base.py b/sentient/channels/base.py index 9d7f1c6ac..ed3cd710c 100644 --- a/sentient/channels/base.py +++ b/sentient/channels/base.py @@ -1083,24 +1083,30 @@ async def _model_command(self, chat_id: str, arg: str) -> None: listing = await model_presets.listing(self.app) usable = [p for p in listing["presets"] if p["available"]] arg = arg.strip() - confirmed = arg.lower().endswith(" yes") # "/model Local only yes" answers the question below (#446) - if confirmed: - arg = arg[:-4].rstrip() + # "/model Local only yes" or "... no" answers the question below (#446, #457). A setup whose own name ends in + # "yes" or "no" is looked up first, so the trailing word only counts as an answer when nothing matches the whole text. + # The question's own two replies always answer it: a setup with one of those names is picked by number or button. + answer = None + replies = {f"{model_presets.LOCAL_PRESET} {word}".lower() for word in ("yes", "no")} + pick = None if arg.lower() in replies else self._find_preset(listing, usable, arg) + if pick is None and arg.lower() not in {"undo", "undo no"}: # "no" never runs an undo + answer = next((w for w in ("yes", "no") if arg.lower().endswith(" " + w)), None) + if answer: + arg = arg[:-len(answer)].rstrip() + pick = self._find_preset(listing, usable, arg) if arg.lower() == "undo": await self.reply(chat_id, await self._preset_outcome(None)) return if arg: - pick = usable[int(arg) - 1] if arg.isdigit() and 1 <= int(arg) <= len(usable) else None - if pick is None: - pick = next((p for p in listing["presets"] if p["name"].lower() == arg.lower()), None) if pick is None: await self.reply(chat_id, f"I don't have a model setup called {arg!r}. Send /model to see them.") return - # the window asks before Local only goes on with its preset (#335); a chat has no dialog, so it asks here - wants_switch = pick["name"] == model_presets.LOCAL_PRESET and not self.app.config.models.local_only - if wants_switch and pick["available"] and not confirmed: - await self.reply(chat_id, "Turn on Local only too, so nothing leaves this computer? " - f"Reply /model {pick['name']} yes") + if answer == "no": + await self.reply(chat_id, "Okay, I left your models as they are.") + return + wants_switch = self._asks_local_only(pick) + if wants_switch and answer != "yes": + await self.reply(chat_id, self._local_only_question(pick["name"])) return await self.reply(chat_id, await self._preset_outcome(pick["name"], local_only=wants_switch)) return @@ -1124,6 +1130,24 @@ async def _model_command(self, chat_id: str, arg: str) -> None: except Exception as exc: log.warning("%s: /model failed: %s", self.id, self.redact(str(exc))) + @staticmethod + def _find_preset(listing: dict, usable: list, text: str) -> dict | None: + """A setup by its number in the usable list or by its exact name, ignoring case.""" + text = text.strip() + if text.isdigit() and 1 <= int(text) <= len(usable): + return usable[int(text) - 1] + return next((p for p in listing["presets"] if p["name"].lower() == text.lower()), None) + + def _asks_local_only(self, preset: dict) -> bool: + """The window asks before Local only goes on with its preset (#335); a chat has no dialog, so it asks with a + reply instead, for ``/model Local only`` and its button alike (#446, #457).""" + return (preset["name"] == model_presets.LOCAL_PRESET and preset["available"] + and not self.app.config.models.local_only) + + @staticmethod + def _local_only_question(name: str) -> str: + return f"Turn on Local only too, so nothing leaves this computer? Reply /model {name} yes or /model {name} no" + async def _chat_model_command(self, chat: dict, value: str) -> None: """``/model chat `` keeps a model for this conversation only (#312); ``/model chat default`` follows the default again; ``/model chat`` says which one it uses.""" @@ -1273,10 +1297,15 @@ async def _preset_outcome(self, name: str | None, *, local_only: bool = False) - return "\n".join(lines) async def _preset_button(self, chat_id: str, message_id: str, ref: str) -> str: - name = next((p["name"] for p in model_presets.presets(self.app.config, model_presets.hardware(self.app)) if self.preset_ref(p["name"]) == ref), None) - if name is None: + preset = next((p for p in model_presets.presets(self.app.config, model_presets.hardware(self.app)) if self.preset_ref(p["name"]) == ref), None) + if preset is None: await self.settle_buttons(chat_id, message_id, "That setup no longer exists") return "That setup no longer exists." + name = preset["name"] + if self._asks_local_only(preset): + await self.settle_buttons(chat_id, message_id, "Turn on Local only too?") + await self.reply(chat_id, self._local_only_question(name)) + return "Turn on Local only too?" text = await self._preset_outcome(name) head, _, rest = text.partition("\n") await self.settle_buttons(chat_id, message_id, head.rstrip(".")) diff --git a/tests/channels/test_model_command.py b/tests/channels/test_model_command.py index 7c50f714e..f9bc3be58 100644 --- a/tests/channels/test_model_command.py +++ b/tests/channels/test_model_command.py @@ -5,7 +5,7 @@ from __future__ import annotations -from sentient.config.schema import PRESET_CLOUD_MODELS, ModelRoles +from sentient.config.schema import PRESET_CLOUD_MODELS, ModelPreset, ModelRoles from tests.channels.conftest import callback DEFAULTS = ModelRoles() @@ -28,6 +28,7 @@ async def test_model_lists_presets_and_a_button_applies_one(tg): assert [b["text"] for b in buttons] == ["Local only"] # Cloud and Mixed need a key first assert all(len(b["callback_data"].encode()) <= 64 for b in buttons) + app.config.models.local_only = True # already on, so the button has nothing to ask (see the tests below) tg.ch.dispatch(callback(42, msg["_id"], buttons[0]["callback_data"])) await tg.ch.wait_idle() assert app.config.models.active_preset == "Local only" @@ -164,3 +165,77 @@ async def test_local_only_preset_by_number_asks_and_skips_the_question_when_alre await tg.say(42, "/model Local only") # the switch is already on: nothing to ask assert tg.api.screen()[-1].startswith("Already using Local only.") # no question, and nothing left to turn on assert app.config.models.local_only + + +async def test_local_only_button_asks_before_turning_the_switch_on(tg): + """Tapping "Local only" in the /model list asks the same question as ``/model Local only`` (#457).""" + app = tg.app + await tg.pair(42) + await tg.say(42, "/model") + msg = tg.api.button_messages()[-1] + tg.ch.dispatch(callback(42, msg["_id"], keyboard(msg)[0]["callback_data"])) + await tg.ch.wait_idle() + assert tg.api.sent("answerCallbackQuery")[-1]["text"] == "Turn on Local only too?" + edit = [p for p in tg.api.sent("editMessageText") if p["message_id"] == msg["_id"]][-1] + assert "Turn on Local only too?" in edit["text"] and "reply_markup" not in edit + assert "/model Local only yes" in tg.api.screen()[-1] and "/model Local only no" in tg.api.screen()[-1] + assert app.config.models.active_preset is None and not app.config.models.local_only # nothing changed yet + + await tg.say(42, "/model Local only yes") + assert tg.api.screen()[-1].startswith("Switched to Local only, and Local only is on.") + assert app.config.models.active_preset == "Local only" and app.config.models.local_only + + +async def test_local_only_no_cancels(tg): + """``/model Local only no`` (#457) answers the question by leaving everything as it is.""" + app = tg.app + await tg.pair(42) + await tg.say(42, "/model Local only") + assert "/model Local only no" in tg.api.screen()[-1] + await tg.say(42, "/model Local only no") + assert tg.api.screen()[-1] == "Okay, I left your models as they are." + assert app.config.models.active_preset is None and not app.config.models.local_only + + +async def test_a_setup_named_with_a_trailing_yes_or_no_is_picked_by_its_full_name(tg): + """The whole text is matched first (#457): "Just say no" and "Work yes" are setups, not answers.""" + app = tg.app + app.config.models.presets = { + "Work": ModelPreset(roles={"primary": DEFAULTS.primary}), + "Work yes": ModelPreset(roles={"primary": DEFAULTS.primary, "fast": DEFAULTS.fast}), + "Just say no": ModelPreset(roles={"primary": DEFAULTS.primary}), + } + await tg.pair(42) + await tg.say(42, "/model Just say no") + assert app.config.models.active_preset == "Just say no" + assert "Just say no" in tg.api.screen()[-1] + await tg.say(42, "/model Work yes") + assert app.config.models.active_preset == "Work yes" + + +async def test_the_local_only_replies_answer_the_question_even_with_a_setup_of_that_name(tg): + """The question's two replies mean what it says; a setup with one of those names is still picked by number (#457).""" + app = tg.app + app.config.models.presets = { + "Local only no": ModelPreset(roles={"primary": DEFAULTS.primary}), + "Local only yes": ModelPreset(roles={"primary": DEFAULTS.primary}), + } + await tg.pair(42) + await tg.say(42, "/model Local only no") + assert tg.api.screen()[-1] == "Okay, I left your models as they are." + assert app.config.models.active_preset is None and not app.config.models.local_only + await tg.say(42, "/model 3") # "Local only yes" in the list: a setup, not an answer + assert app.config.models.active_preset == "Local only yes" and not app.config.models.local_only + await tg.say(42, "/model Local only yes") + assert tg.api.screen()[-1].startswith("Switched to Local only, and Local only is on.") + assert app.config.models.active_preset == "Local only" and app.config.models.local_only + + +async def test_undo_no_does_not_undo(tg): + """A trailing "no" never runs the undo it follows (#457).""" + app = tg.app + await tg.pair(42) + await tg.say(42, "/model Local only yes") + await tg.say(42, "/model undo no") + assert tg.api.screen()[-1] == "I don't have a model setup called 'undo no'. Send /model to see them." + assert app.config.models.active_preset == "Local only" and app.config.models.local_only