fix(channels): ask before the Local only button turns the switch on, and cancel on "no"

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.

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 <provider/model>`` 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("."))

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