refactor(tools): move model-interaction tools to the agent_tools registry (#4445)

Moves chat_with_model, ask_teacher and list_models out of ai_interaction.py
into src/agent_tools/model_interaction_tools.py (the do_ prefix dropped) and
registers them in TOOL_HANDLERS, so dispatch flows through the registry instead
of the dispatch_ai_tool elif in tool_execution.py.

The implementations are relocated, not wrapped. ai_interaction.py keeps only
the shared helpers they reuse (_resolve_model, AI_CHAT_TIMEOUT), still used by
the not-yet-migrated session/pipeline tools. dispatch_ai_tool loses its three
now-unused branches.

Also removes the dead do_second_opinion: it was already off the live tool
surface (no tag/schema/parsing/dispatch; tool_index.py notes it was removed),
so the function and its stale frontend catalog entries (admin.js, assistant.js)
are deleted.

Tests: owner-scope test points at the new list_models location and drops the
moved tools from the dispatch_ai_tool parametrize; a new
test_model_interaction_registry covers registration, owner threading, and
registry dispatch.

tests/test_ai_interaction_owner_scope.py

@@ -3,6 +3,7 @@ import inspect
 import pytest
 
 from src import ai_interaction
+from src.agent_tools import model_interaction_tools
 
 
 def _source(fn) -> str:
@@ -18,7 +19,8 @@ def test_model_resolver_applies_owner_filter():
 
 
 def test_model_listing_and_image_fallback_are_owner_scoped():
-    list_body = _source(ai_interaction.do_list_models)
+    # list_models moved to agent_tools.model_interaction_tools (#3629).
+    list_body = _source(model_interaction_tools.list_models)
     image_body = _source(ai_interaction.do_generate_image)
 
     assert "owner: Optional[str] = None" in list_body
@@ -28,12 +30,13 @@ def test_model_listing_and_image_fallback_are_owner_scoped():
     assert "_resolve_model(model_spec, owner=owner)" in image_body
 
 
+# chat_with_model, list_models and ask_teacher moved to the registry (#3629)
+# and no longer route through dispatch_ai_tool; their owner threading is covered
+# by tests/test_model_interaction_registry.py. The remaining model-ish tools
+# still dispatched here:
 @pytest.mark.parametrize("tool,content", [
-    ("chat_with_model", "gpt-test\nhello"),
     ("pipeline", "gpt-test | summarize this"),
-    ("list_models", ""),
     ("ui_control", "switch_model gpt-test"),
-    ("ask_teacher", "gpt-test\nhelp me"),
 ])
 async def test_dispatch_passes_owner_to_model_tools(monkeypatch, tool, content):
     seen = {}
@@ -42,31 +45,16 @@ async def test_dispatch_passes_owner_to_model_tools(monkeypatch, tool, content):
         seen[name] = {"content": content, "session_id": session_id, "owner": owner}
         return {"ok": True}
 
-    monkeypatch.setattr(
-        ai_interaction,
-        "do_chat_with_model",
-        lambda content, session_id=None, owner=None: capture("chat_with_model", content, session_id, owner),
-    )
     monkeypatch.setattr(
         ai_interaction,
         "do_pipeline",
         lambda content, session_id=None, owner=None: capture("pipeline", content, session_id, owner),
     )
-    monkeypatch.setattr(
-        ai_interaction,
-        "do_list_models",
-        lambda content, session_id=None, owner=None: capture("list_models", content, session_id, owner),
-    )
     monkeypatch.setattr(
         ai_interaction,
         "do_ui_control",
         lambda content, session_id=None, owner=None: capture("ui_control", content, session_id, owner),
     )
-    monkeypatch.setattr(
-        ai_interaction,
-        "do_ask_teacher",
-        lambda content, session_id=None, owner=None: capture("ask_teacher", content, session_id, owner),
-    )
 
     _desc, result = await ai_interaction.dispatch_ai_tool(tool, content, session_id="sid1", owner="alice")
 

tests/test_model_interaction_registry.py

@@ -0,0 +1,104 @@
+"""Tests for the model-interaction tools after their move to the agent_tools
+registry (#3629): chat_with_model, ask_teacher, list_models.
+
+The implementations now live in src/agent_tools/model_interaction_tools.py
+(moved out of src/ai_interaction.py). These assert (1) the handlers are
+registered in TOOL_HANDLERS, (2) each handler runs the moved logic and threads
+session_id/owner from the ctx, and (3) tool_execution.py dispatches them
+through the registry rather than the legacy dispatch_ai_tool elif.
+"""
+import asyncio
+from pathlib import Path
+
+import src.ai_interaction as ai_interaction
+import src.llm_core as llm_core
+import src.database as database
+from src.agent_tools import TOOL_HANDLERS
+from src.agent_tools import model_interaction_tools as mit
+
+_MODEL_TOOLS = ("chat_with_model", "ask_teacher", "list_models")
+
+
+def test_model_interaction_tools_registered():
+    for name in _MODEL_TOOLS:
+        assert name in TOOL_HANDLERS, f"{name} missing from TOOL_HANDLERS"
+
+
+def test_chat_with_model_threads_owner_and_returns(monkeypatch):
+    seen = {}
+
+    def fake_resolve(spec, owner=None):
+        seen["spec"] = spec
+        seen["owner"] = owner
+        return ("http://x", "model-x", {})
+
+    async def fake_call(url, model, messages, headers=None, timeout=None):
+        seen["message"] = messages[-1]["content"]
+        return "hi back"
+
+    monkeypatch.setattr(ai_interaction, "_resolve_model", fake_resolve)
+    monkeypatch.setattr(llm_core, "llm_call_async", fake_call)
+
+    res = asyncio.run(mit.ChatWithModelTool().execute(
+        "model-x\nhello there", {"owner": "alice", "session_id": "s1"}))
+
+    assert res == {"model": "model-x", "response": "hi back"}
+    assert seen["owner"] == "alice"
+    assert seen["spec"] == "model-x"
+    assert seen["message"] == "hello there"
+
+
+def test_ask_teacher_threads_owner_and_marks_teacher(monkeypatch):
+    seen = {}
+
+    def fake_resolve(spec, owner=None):
+        seen["owner"] = owner
+        return ("http://x", "teacher-x", {})
+
+    async def fake_call(url, model, messages, headers=None, timeout=None):
+        return "do this and that"
+
+    monkeypatch.setattr(ai_interaction, "_resolve_model", fake_resolve)
+    monkeypatch.setattr(llm_core, "llm_call_async", fake_call)
+
+    res = asyncio.run(mit.AskTeacherTool().execute(
+        "teacher-x\nI am stuck", {"owner": "bob"}))
+
+    assert res["teacher"] is True
+    assert res["response"] == "do this and that"
+    assert seen["owner"] == "bob"
+
+
+def test_list_models_no_endpoints(monkeypatch):
+    class _Q:
+        def filter(self, *a, **k):
+            return self
+
+        def all(self):
+            return []
+
+    class _S:
+        def query(self, *a, **k):
+            return _Q()
+
+        def close(self):
+            pass
+
+    monkeypatch.setattr(database, "SessionLocal", lambda: _S())
+
+    res = asyncio.run(mit.ListModelsTool().execute("", {}))
+    assert res == {"results": "No enabled model endpoints configured."}
+
+
+def test_dispatched_via_registry_not_dispatch_ai_tool():
+    """The model tools route through the registry (_document_tool_dispatch), and
+    are no longer in the dispatch_ai_tool elif tuple."""
+    source = (Path(__file__).resolve().parent.parent / "src" / "tool_execution.py").read_text(encoding="utf-8")
+    assert 'elif tool in ("chat_with_model", "ask_teacher", "list_models"):' in source
+
+    marker = "from src.ai_interaction import dispatch_ai_tool"
+    idx = source.index(marker)
+    branch_head = source.rfind("elif tool in (", 0, idx)
+    legacy_tuple = source[branch_head:idx]
+    for name in _MODEL_TOOLS:
+        assert f'"{name}"' not in legacy_tuple, f"{name} still routed via dispatch_ai_tool"