fix: address reviewer feedback — type annotations, closure, model aliases

- callback.py: replace shared _model_effort_handler closure with two
  explicit lambdas (model:/effort:) — eliminates outer-scope capture;
  move import to module level
- command.py: drop _MODELS dict with hardcoded version IDs; use
  _MODEL_FAMILIES list of short CLI aliases ("opus"/"sonnet"/"haiku")
  which the CLI resolves to current latest automatically
- command.py: add CallbackQuery + ContextTypes type annotations to
  _handle_model_selection (fixes mypy strict mode)
- command.py: simplify _current_model_label (no reverse-map needed)
- command.py: add PR #165 compatibility comment on force_new_session
- tests: update imports/assertions for _MODEL_FAMILIES; add 3 tests:
  closure regression guard, effort: prefix isolation, force_new_session

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: Talla

src/bot/handlers/callback.py

@@ -12,6 +12,7 @@
 from ...security.audit import AuditLogger
 from ...security.validators import SecurityValidator
 from ..utils.html_format import escape_html
+from .command import _handle_model_selection
 
 logger = structlog.get_logger()
 
@@ -57,11 +58,6 @@ async def handle_callback_query(
             action, param = data, None
 
         # Route to appropriate handler
-        from .command import _handle_model_selection
-
-        async def _model_effort_handler(query, param, context):
-            await _handle_model_selection(query, f"{action}:{param}", context)
-
         handlers = {
             "cd": handle_cd_callback,
             "action": handle_action_callback,
@@ -71,8 +67,8 @@ async def _model_effort_handler(query, param, context):
             "conversation": handle_conversation_callback,
             "git": handle_git_callback,
             "export": handle_export_callback,
-            "model": _model_effort_handler,
-            "effort": _model_effort_handler,
+            "model": lambda q, p, ctx: _handle_model_selection(q, f"model:{p}", ctx),
+            "effort": lambda q, p, ctx: _handle_model_selection(q, f"effort:{p}", ctx),
         }
 
         handler = handlers.get(action)

src/bot/handlers/command.py

@@ -7,7 +7,7 @@
 from typing import Optional
 
 import structlog
-from telegram import InlineKeyboardButton, InlineKeyboardMarkup, Update
+from telegram import CallbackQuery, InlineKeyboardButton, InlineKeyboardMarkup, Update
 from telegram.ext import ContextTypes
 
 from ...claude.facade import ClaudeIntegration
@@ -1233,14 +1233,13 @@ async def git_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> Non
         logger.error("Error in git_command", error=str(e), user_id=user_id)
 
 
-# Model IDs mapped to user-friendly labels
-_MODELS = {
-    "opus": "claude-opus-4-6",
-    "sonnet": "claude-sonnet-4-6",
-    "haiku": "claude-haiku-4-5-20251001",
-}
+# Short CLI aliases passed directly to the Claude CLI, which resolves them to
+# the current latest model of each family. No version numbers to maintain here.
+# See: https://docs.anthropic.com/en/docs/about-claude/models/overview
+_MODEL_FAMILIES = ["opus", "sonnet", "haiku"]
 
-# Effort levels per model (Haiku doesn't support effort; "max" is Opus-only)
+# Effort levels per model family. Haiku has none; "max" is Opus-only.
+# Update here if a future model's effort support changes.
 _EFFORT_BY_MODEL = {
     "opus": ["low", "medium", "high", "max"],
     "sonnet": ["low", "medium", "high"],
@@ -1250,23 +1249,15 @@ async def git_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> Non
 
 def _current_model_label(context: ContextTypes.DEFAULT_TYPE) -> str:
     """Return a human-friendly label for the active model + effort."""
-    override = context.user_data.get("model_override")
+    override = context.user_data.get("model_override")  # "opus", "sonnet", "haiku", or None
     effort = context.user_data.get("effort_override")
-    # Reverse-map model ID to short name
-    model_id = override or ""
-    label = model_id
-    for short, full in _MODELS.items():
-        if full == model_id:
-            label = short.capitalize()
-            break
     if not override:
         settings = context.bot_data.get("settings")
         server_model = getattr(settings, "claude_model", None) if settings else None
         label = f"Default ({server_model or 'CLI default'})"
-    parts = [label]
-    if effort:
-        parts.append(f"effort={effort}")
-    return " | ".join(parts)
+    else:
+        label = override.capitalize()
+    return f"{label} | effort={effort}" if effort else label
 
 
 async def model_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None:
@@ -1291,14 +1282,19 @@ async def model_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> N
     )
 
 
-async def _handle_model_selection(query, data: str, context) -> None:
+async def _handle_model_selection(
+    query: CallbackQuery,
+    data: str,
+    context: ContextTypes.DEFAULT_TYPE,
+) -> None:
     """Shared logic for model/effort selection (used by both callback routes)."""
     if data.startswith("model:"):
         choice = data.split(":", 1)[1]
 
         if choice == "default":
             context.user_data.pop("model_override", None)
             context.user_data.pop("effort_override", None)
+            # Note: if PR #165 merges first, change this to context.chat_data
             context.user_data["force_new_session"] = True
             await query.edit_message_text(
                 "🤖 Model and effort reset to server defaults.\n"
@@ -1308,21 +1304,23 @@ async def _handle_model_selection(query, data: str, context) -> None:
             logger.info("Model override cleared", user_id=query.from_user.id)
             return
 
-        model_id = _MODELS.get(choice)
-        if not model_id:
+        if choice not in _MODEL_FAMILIES:
             await query.edit_message_text("Unknown model.")
             return
 
-        context.user_data["model_override"] = model_id
+        # Store short CLI alias ("opus"/"sonnet"/"haiku") — the CLI resolves it
+        # to the current latest model, so no version numbers to maintain.
+        context.user_data["model_override"] = choice
         # Clear stale effort when switching models
         context.user_data.pop("effort_override", None)
         # Force new session so the model change takes effect immediately
+        # Note: if PR #165 merges first, change this to context.chat_data
         context.user_data["force_new_session"] = True
 
         logger.info(
             "Model override set",
             user_id=query.from_user.id,
-            model=model_id,
+            model=choice,
         )
 
         # Show effort level selection (if supported by this model)

tests/unit/test_bot/test_model_command.py

@@ -8,6 +8,8 @@
 - Haiku skips effort keyboard (not supported)
 - Opus shows "max" effort, Sonnet does not
 - _current_model_label returns correct labels
+- Regression: model: callback data prefix is never rewritten as effort:
+- force_new_session is always set on model change
 """
 
 from unittest.mock import AsyncMock, MagicMock
@@ -17,7 +19,7 @@
 
 from src.bot.handlers.command import (
     _EFFORT_BY_MODEL,
-    _MODELS,
+    _MODEL_FAMILIES,
     _current_model_label,
     _handle_model_selection,
     model_command,
@@ -85,7 +87,7 @@ async def test_model_command_shows_keyboard(update, context):
 @pytest.mark.asyncio
 async def test_model_command_shows_current_override(update, context):
     """When an override is active, /model should show it."""
-    context.user_data["model_override"] = _MODELS["sonnet"]
+    context.user_data["model_override"] = "sonnet"
     await model_command(update, context)
 
     text = update.message.reply_text.call_args.args[0]
@@ -99,27 +101,27 @@ async def test_model_command_shows_current_override(update, context):
 
 @pytest.mark.asyncio
 async def test_select_opus_sets_override(callback_query, context):
-    """Selecting Opus sets model_override and force_new_session."""
+    """Selecting Opus sets model_override to short alias and force_new_session."""
     await _handle_model_selection(callback_query, "model:opus", context)
 
-    assert context.user_data["model_override"] == _MODELS["opus"]
+    assert context.user_data["model_override"] == "opus"
     assert context.user_data["force_new_session"] is True
 
 
 @pytest.mark.asyncio
 async def test_select_sonnet_sets_override(callback_query, context):
-    """Selecting Sonnet sets the correct model ID."""
+    """Selecting Sonnet sets the correct short alias."""
     await _handle_model_selection(callback_query, "model:sonnet", context)
 
-    assert context.user_data["model_override"] == _MODELS["sonnet"]
+    assert context.user_data["model_override"] == "sonnet"
 
 
 @pytest.mark.asyncio
 async def test_select_haiku_skips_effort(callback_query, context):
     """Selecting Haiku should not show effort keyboard (not supported)."""
     await _handle_model_selection(callback_query, "model:haiku", context)
 
-    assert context.user_data["model_override"] == _MODELS["haiku"]
+    assert context.user_data["model_override"] == "haiku"
     # Final message, no reply_markup (no effort keyboard)
     call_kwargs = callback_query.edit_message_text.call_args
     assert "reply_markup" not in call_kwargs.kwargs or call_kwargs.kwargs.get("reply_markup") is None
@@ -156,7 +158,7 @@ async def test_select_sonnet_shows_effort_without_max(callback_query, context):
 @pytest.mark.asyncio
 async def test_default_clears_overrides(callback_query, context):
     """Selecting 'default' clears model, effort, and forces new session."""
-    context.user_data["model_override"] = _MODELS["opus"]
+    context.user_data["model_override"] = "opus"
     context.user_data["effort_override"] = "high"
 
     await _handle_model_selection(callback_query, "model:default", context)
@@ -174,7 +176,7 @@ async def test_default_clears_overrides(callback_query, context):
 @pytest.mark.asyncio
 async def test_effort_sets_override(callback_query, context):
     """Selecting an effort level stores it in user_data."""
-    context.user_data["model_override"] = _MODELS["opus"]
+    context.user_data["model_override"] = "opus"
 
     await _handle_model_selection(callback_query, "effort:high", context)
 
@@ -184,7 +186,7 @@ async def test_effort_sets_override(callback_query, context):
 @pytest.mark.asyncio
 async def test_effort_skip_keeps_existing(callback_query, context):
     """Selecting 'skip' should not set effort_override."""
-    context.user_data["model_override"] = _MODELS["sonnet"]
+    context.user_data["model_override"] = "sonnet"
 
     await _handle_model_selection(callback_query, "effort:skip", context)
 
@@ -201,6 +203,44 @@ async def test_model_switch_clears_stale_effort(callback_query, context):
     assert "effort_override" not in context.user_data
 
 
+# ---------------------------------------------------------------------------
+# Regression: callback data prefix integrity (closure-bug guard)
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.asyncio
+async def test_model_callback_sets_model_not_effort(callback_query, context):
+    """model: callback must set model_override, not effort_override.
+
+    Regression guard: a shared closure capturing 'action' from outer scope
+    could silently rewrite 'model:opus' as 'effort:opus'. This test would
+    have caught that.
+    """
+    await _handle_model_selection(callback_query, "model:sonnet", context)
+
+    assert context.user_data.get("model_override") == "sonnet"
+    assert "effort_override" not in context.user_data
+
+
+@pytest.mark.asyncio
+async def test_effort_callback_sets_effort_not_model(callback_query, context):
+    """effort: callback must set effort_override and not overwrite model_override."""
+    context.user_data["model_override"] = "sonnet"
+
+    await _handle_model_selection(callback_query, "effort:high", context)
+
+    assert context.user_data.get("effort_override") == "high"
+    assert context.user_data.get("model_override") == "sonnet"  # unchanged
+
+
+@pytest.mark.asyncio
+async def test_force_new_session_set_on_model_switch(callback_query, context):
+    """force_new_session must be True after any model switch."""
+    await _handle_model_selection(callback_query, "model:opus", context)
+
+    assert context.user_data.get("force_new_session") is True
+
+
 # ---------------------------------------------------------------------------
 # Label helper
 # ---------------------------------------------------------------------------
@@ -222,13 +262,13 @@ def test_label_default_with_server_model():
 
 def test_label_with_model_and_effort():
     ctx = MagicMock()
-    ctx.user_data = {"model_override": _MODELS["sonnet"], "effort_override": "medium"}
+    ctx.user_data = {"model_override": "sonnet", "effort_override": "medium"}
     assert _current_model_label(ctx) == "Sonnet | effort=medium"
 
 
 def test_label_model_only():
     ctx = MagicMock()
-    ctx.user_data = {"model_override": _MODELS["opus"]}
+    ctx.user_data = {"model_override": "opus"}
     assert _current_model_label(ctx) == "Opus"
 
 
@@ -248,3 +288,9 @@ def test_sonnet_has_no_max():
 
 def test_opus_has_max():
     assert "max" in _EFFORT_BY_MODEL["opus"]
+
+
+def test_model_families_contains_expected():
+    assert "opus" in _MODEL_FAMILIES
+    assert "sonnet" in _MODEL_FAMILIES
+    assert "haiku" in _MODEL_FAMILIES