fix(document): allow render-pdf to be framed and 503 cleanly on missing PyMuPDF (#2103)

* fix(document): allow render-pdf to be framed and 503 cleanly on missing PyMuPDF

Fixes #2101.

Two related bugs in the PDF-form library preview flow:

1. SecurityHeadersMiddleware was sending X-Frame-Options: DENY and
   frame-ancestors 'none' on /api/document/{doc_id}/render-pdf, but
   static/js/documentLibrary.js embeds the response in an <iframe> for
   the library card preview. The browser blocked the load with
   ERR_BLOCKED_BY_RESPONSE, leaving the user with a blank panel.

   Extend the existing is_tool_render exemption to also cover
   /api/document/.../render-pdf. Per-document owner checks still run in
   the route handler, so the exemption is scoped the same way as the
   tool-render exemption it mirrors. /api/document/.../export-pdf is
   left untouched — it's a download (Content-Disposition: attachment),
   not an iframe embed.

2. routes/document_routes.py:render_pdf called fill_fields, which
   raises RuntimeError via _require_fitz() when the optional PyMuPDF
   dependency isn't installed. That RuntimeError bubbled out as a
   generic 500 with a cryptic 'PDF render failed' detail.

   Reuse the existing _load_pdf_viewer_fitz() helper to fail fast with
   a 503 and a user-actionable install hint (mentions
   requirements-optional.txt and AGPL-3.0), matching the convention
   used by the other PDF endpoints.

Tests cover both fixes:
- middleware headers on /api/document/.../render-pdf (iframeable, but
  X-Content-Type-Options and Referrer-Policy are still set)
- middleware headers on /api/document/.../export-pdf (must stay strict)
- middleware path matching precision (similar-but-different paths stay
  strict)
- middleware headers on /api/tools/.../render (no regression)
- middleware headers on /api/chat (no regression)
- render-pdf returns 503 with install hint when PyMuPDF is missing
- 503 is raised before any file I/O (fail-fast ordering)

* chore: address maintainer feedback on PDF previews same-origin framing and comment trimming

* chore: make render-pdf regression tests order-independent

core/middleware.py

@@ -67,10 +67,9 @@ class SecurityHeadersMiddleware(BaseHTTPMiddleware):
         response = await call_next(request)
         path = request.url.path
 
-        # Tool render endpoints are served inside iframes — allow framing by self
+        # Tool render endpoints
         is_tool_render = path.startswith("/api/tools/") and path.endswith("/render")
-        # PDF previews are embedded by the in-app document library. Keep the
-        # exception route-scoped so normal app pages remain unframeable.
+        # Document library PDF preview endpoint
         is_document_pdf_preview = path.startswith("/api/document/") and path.endswith("/render-pdf")
         # Visual report pages are self-contained HTML — need inline scripts + external images
         is_report = path.startswith("/api/research/report/")
@@ -97,9 +96,7 @@ class SecurityHeadersMiddleware(BaseHTTPMiddleware):
                 "frame-ancestors 'none'"
             )
         elif is_tool_render:
-            # Tool iframe content: skip all framing headers — the iframe's
-            # sandbox="allow-scripts" attribute provides isolation.
-            # Don't overwrite the route's own restrictive CSP either.
+            # Skip framing headers for tools.
             pass
         elif is_document_pdf_preview:
             response.headers["X-Frame-Options"] = "SAMEORIGIN"

routes/document_routes.py

@@ -1332,6 +1332,12 @@ def setup_document_routes(session_manager, upload_handler=None) -> APIRouter:
             if not pdf_path:
                 raise HTTPException(404, f"Source PDF {upload_id} not found")
 
+            # Fail fast with a clear 503 if the optional PyMuPDF dependency
+            # is missing — fill_fields/stamp_annotations will otherwise
+            # raise RuntimeError deep inside and bubble out as a 500.
+            # Mirrors the convention in _load_pdf_viewer_fitz above.
+            _load_pdf_viewer_fitz()
+
             values = parse_markdown_to_values(doc.current_content or "")
             out_path = tempfile.NamedTemporaryFile(suffix=".pdf", delete=False).name
             _to_unlink.append(out_path)

src/agent_tools/__init__.py

@@ -22,6 +22,7 @@ from .subprocess_tools import BashTool, PythonTool
 from .web_tools import WebSearchTool, WebFetchTool
 from .filesystem_tools import ReadFileTool, WriteFileTool, EditFileTool, LsTool, GlobTool, GrepTool, GetWorkspaceTool
 from .document_tools import CreateDocumentTool, UpdateDocumentTool, EditDocumentTool, SuggestDocumentTool, ManageDocumentTool
+from .model_interaction_tools import ChatWithModelTool, AskTeacherTool, ListModelsTool
 
 TOOL_HANDLERS = {
     "bash": BashTool().execute,
@@ -40,6 +41,9 @@ TOOL_HANDLERS = {
     "suggest_document": SuggestDocumentTool().execute,
     "manage_documents": ManageDocumentTool().execute,
     "get_workspace": GetWorkspaceTool().execute,
+    "chat_with_model": ChatWithModelTool().execute,
+    "ask_teacher": AskTeacherTool().execute,
+    "list_models": ListModelsTool().execute,
 }
 
 # ---------------------------------------------------------------------------

src/agent_tools/model_interaction_tools.py

@@ -0,0 +1,208 @@
+"""model_interaction_tools.py - agent tools for talking to other models.
+
+Owns the model-interaction tool implementations (chat_with_model, ask_teacher,
+list_models) and their handler classes, registered in ``TOOL_HANDLERS``. Part
+of the tool -> registry migration (#3629): the implementations were moved here
+out of ``src.ai_interaction`` so dispatch flows through the registry instead of
+the elif chain / dispatch_ai_tool in tool_execution.py.
+
+Shared helpers that still live in ``src.ai_interaction`` and are used by tools
+not yet migrated (``_resolve_model``, ``AI_CHAT_TIMEOUT``) are imported lazily
+inside the functions to avoid an import cycle at module load.
+"""
+import logging
+from typing import Dict, Optional
+
+logger = logging.getLogger(__name__)
+
+
+_TEACHER_SYSTEM_PROMPT = (
+    "You are a senior AI mentor. A less capable model is stuck on a problem and asking for help. "
+    "Provide clear, actionable guidance:\n"
+    "1. Brief analysis of the problem\n"
+    "2. Recommended approach (step by step)\n"
+    "3. Key things to watch out for\n\n"
+    "Be concise and practical. No preamble."
+)
+
+
+async def chat_with_model(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
+    """Send a message to a specific model and return its response.
+
+    Content format:
+      Line 1: model_name (or model_name@endpoint_name)
+      Line 2+: the message to send
+    """
+    from src.ai_interaction import _resolve_model, AI_CHAT_TIMEOUT
+    from src.llm_core import llm_call_async
+
+    lines = content.strip().split("\n", 1)
+    if not lines or not lines[0].strip():
+        return {"error": "First line must be the model name"}
+
+    model_spec = lines[0].strip()
+    message = lines[1].strip() if len(lines) > 1 else ""
+    if not message:
+        return {"error": "No message provided (line 2+ is the message)"}
+
+    try:
+        url, model, headers = _resolve_model(model_spec, owner=owner)
+    except ValueError as e:
+        return {"error": str(e)}
+
+    try:
+        response = await llm_call_async(
+            url, model,
+            [{"role": "user", "content": message}],
+            headers=headers,
+            timeout=AI_CHAT_TIMEOUT,
+        )
+        # Truncate very long responses
+        if len(response) > 10000:
+            response = response[:10000] + "\n... (truncated)"
+        return {"model": model, "response": response}
+    except Exception as e:
+        logger.error(f"chat_with_model failed: {e}")
+        return {"error": f"Failed to get response from {model_spec}: {e}"}
+
+
+async def ask_teacher(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
+    """Ask a more capable model for help.
+
+    Content format:
+      Line 1: model_name (or 'auto')
+      Line 2+: the problem description
+    """
+    from src.ai_interaction import _resolve_model, AI_CHAT_TIMEOUT
+    from src.llm_core import llm_call_async
+    from src.settings import get_setting
+
+    lines = content.strip().split("\n", 1)
+    model_spec = lines[0].strip() if lines else "auto"
+    problem = lines[1].strip() if len(lines) > 1 else ""
+
+    if not problem:
+        return {"error": "No problem description provided"}
+
+    if model_spec.lower() in ("auto", ""):
+        model_spec = get_setting("teacher_model", "")
+        if not model_spec:
+            return {"error": "No teacher model configured. Specify a model name or set teacher_model in settings."}
+
+    try:
+        url, model, headers = _resolve_model(model_spec, owner=owner)
+    except ValueError as e:
+        return {"error": str(e)}
+
+    try:
+        response = await llm_call_async(
+            url, model,
+            [
+                {"role": "system", "content": _TEACHER_SYSTEM_PROMPT},
+                {"role": "user", "content": f"Problem:\n{problem}"},
+            ],
+            headers=headers,
+            timeout=AI_CHAT_TIMEOUT,
+        )
+        if len(response) > 8000:
+            response = response[:8000] + "\n... (truncated)"
+        return {"model": model, "response": response, "teacher": True}
+    except Exception as e:
+        logger.error(f"ask_teacher failed: {e}")
+        return {"error": f"Teacher call failed ({model_spec}): {e}"}
+
+
+async def list_models(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
+    """List all available models across configured endpoints.
+
+    Content = optional filter keyword.
+    """
+    import json
+    import httpx
+    from src.database import SessionLocal, ModelEndpoint
+    from src.llm_core import _detect_provider, ANTHROPIC_MODELS
+    from src.auth_helpers import owner_filter
+    from src.endpoint_resolver import resolve_endpoint_runtime, build_headers, build_models_url
+
+    keyword = content.strip().lower() if content.strip() else None
+
+    db = SessionLocal()
+    try:
+        query = db.query(ModelEndpoint).filter(ModelEndpoint.is_enabled == True)
+        if owner:
+            query = owner_filter(query, ModelEndpoint, owner)
+        endpoints = query.all()
+        if not endpoints:
+            return {"results": "No enabled model endpoints configured."}
+
+        result_lines = []
+        total_models = 0
+
+        for ep in endpoints:
+            try:
+                base, api_key = resolve_endpoint_runtime(ep, owner=owner)
+            except Exception:
+                continue
+            provider = _detect_provider(base)
+            headers = build_headers(api_key, base)
+
+            model_ids = []
+            if provider == "anthropic":
+                model_ids = list(ANTHROPIC_MODELS)
+            else:
+                try:
+                    models_url = build_models_url(base)
+                    if models_url:
+                        r = httpx.get(models_url, headers=headers, timeout=5)
+                        r.raise_for_status()
+                        data = r.json()
+                        model_ids = [m.get("id") for m in (data.get("data") or []) if m.get("id")]
+                        if not model_ids:
+                            model_ids = [
+                                m.get("name") or m.get("model")
+                                for m in (data.get("models") or [])
+                                if m.get("name") or m.get("model")
+                            ]
+                    else:
+                        model_ids = json.loads(ep.cached_models or "[]")
+                except Exception:
+                    model_ids = ["(endpoint offline)"]
+
+            if keyword:
+                model_ids = [m for m in model_ids if keyword in m.lower() or keyword in (ep.name or "").lower()]
+
+            if model_ids:
+                result_lines.append(f"\n**{ep.name or base}** ({provider}):")
+                for mid in model_ids:
+                    result_lines.append(f"  - `{mid}`")
+                    total_models += 1
+
+        if not result_lines:
+            return {"results": "No models found" + (f" matching '{keyword}'" if keyword else "") + "."}
+
+        header = f"Available models ({total_models} total):"
+        return {"results": header + "\n".join(result_lines)}
+    except Exception as e:
+        logger.error(f"list_models failed: {e}")
+        return {"error": str(e)}
+    finally:
+        db.close()
+
+
+# ---------------------------------------------------------------------------
+# Handler classes registered in TOOL_HANDLERS
+# ---------------------------------------------------------------------------
+
+class ChatWithModelTool:
+    async def execute(self, content: str, ctx: dict) -> Dict:
+        return await chat_with_model(content, ctx.get("session_id"), owner=ctx.get("owner"))
+
+
+class AskTeacherTool:
+    async def execute(self, content: str, ctx: dict) -> Dict:
+        return await ask_teacher(content, ctx.get("session_id"), owner=ctx.get("owner"))
+
+
+class ListModelsTool:
+    async def execute(self, content: str, ctx: dict) -> Dict:
+        return await list_models(content, ctx.get("session_id"), owner=ctx.get("owner"))

src/ai_interaction.py

@@ -1,8 +1,12 @@
 """
 ai_interaction.py
 
-AI-to-AI interaction tools: chat_with_model, create_session, list_sessions,
-send_to_session, pipeline.
+AI-to-AI interaction tools: create_session, list_sessions, send_to_session,
+pipeline, plus shared model resolution (_resolve_model).
+
+chat_with_model, ask_teacher and list_models were moved to
+src/agent_tools/model_interaction_tools.py as part of the tool -> registry
+migration (#3629); they still reuse _resolve_model / AI_CHAT_TIMEOUT from here.
 
 These are agent tools — the LLM writes fenced code blocks and they execute
 through the standard agent_tools.py pipeline.
@@ -159,242 +163,6 @@ def _resolve_model(spec: str, owner: Optional[str] = None) -> Tuple[str, str, Di
 # Tool implementations
 # ---------------------------------------------------------------------------
 
-async def do_chat_with_model(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
-    """Send a message to a specific model and return its response.
-
-    Content format:
-      Line 1: model_name (or model_name@endpoint_name)
-      Line 2+: the message to send
-    """
-    from src.llm_core import llm_call_async
-
-    lines = content.strip().split("\n", 1)
-    if not lines or not lines[0].strip():
-        return {"error": "First line must be the model name"}
-
-    model_spec = lines[0].strip()
-    message = lines[1].strip() if len(lines) > 1 else ""
-    if not message:
-        return {"error": "No message provided (line 2+ is the message)"}
-
-    try:
-        url, model, headers = _resolve_model(model_spec, owner=owner)
-    except ValueError as e:
-        return {"error": str(e)}
-
-    try:
-        response = await llm_call_async(
-            url, model,
-            [{"role": "user", "content": message}],
-            headers=headers,
-            timeout=AI_CHAT_TIMEOUT,
-        )
-        # Truncate very long responses
-        if len(response) > 10000:
-            response = response[:10000] + "\n... (truncated)"
-        return {"model": model, "response": response}
-    except Exception as e:
-        logger.error(f"chat_with_model failed: {e}")
-        return {"error": f"Failed to get response from {model_spec}: {e}"}
-
-
-_TEACHER_SYSTEM_PROMPT = (
-    "You are a senior AI mentor. A less capable model is stuck on a problem and asking for help. "
-    "Provide clear, actionable guidance:\n"
-    "1. Brief analysis of the problem\n"
-    "2. Recommended approach (step by step)\n"
-    "3. Key things to watch out for\n\n"
-    "Be concise and practical. No preamble."
-)
-
-
-async def do_ask_teacher(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
-    """Ask a more capable model for help.
-
-    Content format:
-      Line 1: model_name (or 'auto')
-      Line 2+: the problem description
-    """
-    from src.llm_core import llm_call_async
-    from src.settings import get_setting
-
-    lines = content.strip().split("\n", 1)
-    model_spec = lines[0].strip() if lines else "auto"
-    problem = lines[1].strip() if len(lines) > 1 else ""
-
-    if not problem:
-        return {"error": "No problem description provided"}
-
-    if model_spec.lower() in ("auto", ""):
-        model_spec = get_setting("teacher_model", "")
-        if not model_spec:
-            return {"error": "No teacher model configured. Specify a model name or set teacher_model in settings."}
-
-    try:
-        url, model, headers = _resolve_model(model_spec, owner=owner)
-    except ValueError as e:
-        return {"error": str(e)}
-
-    try:
-        response = await llm_call_async(
-            url, model,
-            [
-                {"role": "system", "content": _TEACHER_SYSTEM_PROMPT},
-                {"role": "user", "content": f"Problem:\n{problem}"},
-            ],
-            headers=headers,
-            timeout=AI_CHAT_TIMEOUT,
-        )
-        if len(response) > 8000:
-            response = response[:8000] + "\n... (truncated)"
-        return {"model": model, "response": response, "teacher": True}
-    except Exception as e:
-        logger.error(f"ask_teacher failed: {e}")
-        return {"error": f"Teacher call failed ({model_spec}): {e}"}
-
-
-async def do_second_opinion(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
-    """Get a second opinion from another model, then have the original model
-    evaluate the feedback and produce a unified version.
-
-    Content format:
-      Line 1: model_name (or model_name@endpoint_name)
-      Line 2+ (optional): specific question or focus area
-
-    Flow:
-      1. Pull recent conversation context
-      2. Send to reviewer model → get honest feedback
-      3. Send feedback back to the session's own model → evaluate & unify
-      4. Return both the review and the unified response
-    """
-    from src.llm_core import llm_call_async
-
-    lines = content.strip().split("\n", 1)
-    if not lines or not lines[0].strip():
-        return {"error": "First line must be the model name"}
-
-    model_spec = lines[0].strip()
-    focus = lines[1].strip() if len(lines) > 1 else ""
-
-    try:
-        reviewer_url, reviewer_model, reviewer_headers = _resolve_model(model_spec, owner=owner)
-    except ValueError as e:
-        return {"error": str(e)}
-
-    # Pull recent conversation context from current session
-    context_text = ""
-    sess = None
-    if session_id and _session_manager:
-        sess = _session_manager.get_session(session_id)
-        if sess:
-            messages = sess.get_context_messages()
-            recent = messages[-15:] if len(messages) > 15 else messages
-            parts = []
-            for m in recent:
-                role = m.get("role", "unknown").upper()
-                text = m.get("content", "")
-                if isinstance(text, list):
-                    text = " ".join(
-                        p.get("text", "") for p in text if isinstance(p, dict)
-                    )
-                if text:
-                    parts.append(f"[{role}]: {text[:2000]}")
-            context_text = "\n\n".join(parts)
-
-    if not context_text:
-        return {"error": "No conversation context found to review"}
-
-    # ── Step 1: Get the reviewer's feedback ──
-    reviewer_system = (
-        "You are giving a second opinion on a conversation between a user and an AI assistant. "
-        "Your job is to be genuinely helpful and honest — not a yes-man, but not a contrarian either.\n\n"
-        "Guidelines:\n"
-        "- If the plan/idea is solid, say so clearly. Don't manufacture problems that aren't there.\n"
-        "- If you spot a real flaw, blind spot, or simpler approach — call it out directly.\n"
-        "- Be practical. Don't over-engineer or over-analyze. Real-world tradeoffs matter.\n"
-        "- If there's a meaningfully better way to do something, suggest it concretely.\n"
-        "- Give credit where it's due — highlight what's working well.\n"
-        "- Keep it concise and actionable. No fluff.\n"
-        "- You're a second pair of eyes, not a professor grading a paper."
-    )
-
-    reviewer_message = f"Here's the conversation so far:\n\n{context_text}"
-    if focus:
-        reviewer_message += f"\n\n---\nSpecifically, I want your take on: {focus}"
-    else:
-        reviewer_message += "\n\n---\nGive me your honest second opinion on what's being discussed."
-
-    try:
-        review = await llm_call_async(
-            reviewer_url, reviewer_model,
-            [
-                {"role": "system", "content": reviewer_system},
-                {"role": "user", "content": reviewer_message},
-            ],
-            headers=reviewer_headers,
-            timeout=AI_CHAT_TIMEOUT,
-        )
-        if len(review) > 8000:
-            review = review[:8000] + "\n... (truncated)"
-    except Exception as e:
-        logger.error(f"second_opinion reviewer call failed: {e}")
-        return {"error": f"Failed to get second opinion from {model_spec}: {e}"}
-
-    # ── Step 2: Send review back to session's own model for evaluation ──
-    unified = ""
-    original_model = "unknown"
-    if sess:
-        original_url = sess.endpoint_url
-        original_model = sess.model
-        original_headers = getattr(sess, "headers", None) or {}
-
-        unify_system = (
-            "Another AI model just reviewed the conversation you've been having with the user. "
-            "Read their feedback carefully, then respond with:\n\n"
-            "1. **What you agree with** — acknowledge valid points honestly.\n"
-            "2. **What you disagree with** — explain why, briefly.\n"
-            "3. **Unified version** — produce an updated/refined version of whatever was being discussed, "
-            "incorporating the feedback you found valid. Don't accept every note blindly — "
-            "use your judgment on what actually improves things vs what's unnecessary.\n\n"
-            "Be concise and practical. The user wants a better result, not a meta-discussion."
-        )
-
-        unify_message = (
-            f"Here's the conversation context:\n\n{context_text}\n\n"
-            f"---\n\n"
-            f"**Review from {reviewer_model}:**\n\n{review}\n\n"
-            f"---\n\n"
-            f"Evaluate this feedback and produce a unified improved version."
-        )
-
-        try:
-            unified = await llm_call_async(
-                original_url, original_model,
-                [
-                    {"role": "system", "content": unify_system},
-                    {"role": "user", "content": unify_message},
-                ],
-                headers=original_headers,
-                timeout=AI_CHAT_TIMEOUT,
-            )
-            if len(unified) > 10000:
-                unified = unified[:10000] + "\n... (truncated)"
-        except Exception as e:
-            logger.error(f"second_opinion unify call failed: {e}")
-            unified = f"(Failed to get unified response: {e})"
-
-    # Build combined result
-    combined = (
-        f"## Second Opinion from {reviewer_model}\n\n{review}"
-        f"\n\n---\n\n"
-        f"## {original_model}'s Response\n\n{unified}"
-    )
-
-    return {
-        "model": reviewer_model,
-        "response": combined,
-        "instruction": "Present these results to the user exactly as they are. Do NOT call second_opinion again. The user can continue the conversation from here.",
-    }
 
 
 async def do_create_session(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
@@ -1104,83 +872,6 @@ async def do_manage_memory(content: str, session_id: Optional[str] = None, owner
         return {"error": f"Unknown action '{action}'. Use: list, add, edit, delete, search"}
 
 
-# ---------------------------------------------------------------------------
-# List models tool
-# ---------------------------------------------------------------------------
-
-async def do_list_models(content: str, session_id: Optional[str] = None, owner: Optional[str] = None) -> Dict:
-    """List all available models across configured endpoints.
-
-    Content = optional filter keyword.
-    """
-    import httpx
-    from src.database import SessionLocal, ModelEndpoint
-    from src.llm_core import _detect_provider, ANTHROPIC_MODELS
-    from src.auth_helpers import owner_filter
-
-    keyword = content.strip().lower() if content.strip() else None
-
-    db = SessionLocal()
-    try:
-        query = db.query(ModelEndpoint).filter(ModelEndpoint.is_enabled == True)
-        if owner:
-            query = owner_filter(query, ModelEndpoint, owner)
-        endpoints = query.all()
-        if not endpoints:
-            return {"results": "No enabled model endpoints configured."}
-
-        result_lines = []
-        total_models = 0
-
-        for ep in endpoints:
-            try:
-                base, api_key = resolve_endpoint_runtime(ep, owner=owner)
-            except Exception:
-                continue
-            provider = _detect_provider(base)
-            headers = build_headers(api_key, base)
-
-            model_ids = []
-            if provider == "anthropic":
-                model_ids = list(ANTHROPIC_MODELS)
-            else:
-                try:
-                    models_url = build_models_url(base)
-                    if models_url:
-                        r = httpx.get(models_url, headers=headers, timeout=5)
-                        r.raise_for_status()
-                        data = r.json()
-                        model_ids = [m.get("id") for m in (data.get("data") or []) if m.get("id")]
-                        if not model_ids:
-                            model_ids = [
-                                m.get("name") or m.get("model")
-                                for m in (data.get("models") or [])
-                                if m.get("name") or m.get("model")
-                            ]
-                    else:
-                        model_ids = json.loads(ep.cached_models or "[]")
-                except Exception:
-                    model_ids = ["(endpoint offline)"]
-
-            if keyword:
-                model_ids = [m for m in model_ids if keyword in m.lower() or keyword in (ep.name or "").lower()]
-
-            if model_ids:
-                result_lines.append(f"\n**{ep.name or base}** ({provider}):")
-                for mid in model_ids:
-                    result_lines.append(f"  - `{mid}`")
-                    total_models += 1
-
-        if not result_lines:
-            return {"results": "No models found" + (f" matching '{keyword}'" if keyword else "") + "."}
-
-        header = f"Available models ({total_models} total):"
-        return {"results": header + "\n".join(result_lines)}
-    except Exception as e:
-        logger.error(f"list_models failed: {e}")
-        return {"error": str(e)}
-    finally:
-        db.close()
 
 
 # ---------------------------------------------------------------------------
@@ -1831,12 +1522,7 @@ async def dispatch_ai_tool(
 ) -> Tuple[str, Dict]:
     """Dispatch an AI interaction tool. Returns (description, result_dict)."""
 
-    if tool == "chat_with_model":
-        model_spec = content.split("\n")[0].strip()[:60]
-        desc = f"chat_with_model: {model_spec}"
-        result = await do_chat_with_model(content, session_id, owner=owner)
-
-    elif tool == "create_session":
+    if tool == "create_session":
         name = content.split("\n")[0].strip()[:60]
         desc = f"create_session: {name}"
         result = await do_create_session(content, session_id, owner=owner)
@@ -1865,21 +1551,11 @@ async def dispatch_ai_tool(
         desc = f"manage_memory: {action}"
         result = await do_manage_memory(content, session_id, owner=owner)
 
-    elif tool == "list_models":
-        keyword = content.strip()[:40]
-        desc = f"list_models{': ' + keyword if keyword else ''}"
-        result = await do_list_models(content, session_id, owner=owner)
-
     elif tool == "ui_control":
         action = content.split("\n")[0].strip()[:60]
         desc = f"ui_control: {action}"
         result = await do_ui_control(content, session_id, owner=owner)
 
-    elif tool == "ask_teacher":
-        problem = content.split("\n", 1)[-1].strip()[:60]
-        desc = f"ask_teacher: {problem}"
-        result = await do_ask_teacher(content, session_id, owner=owner)
-
     else:
         desc = f"unknown ai tool: {tool}"
         result = {"error": f"Unknown AI interaction tool: {tool}"}

src/tool_execution.py

@@ -766,10 +766,19 @@ async def _execute_tool_block_impl(
         query = content.split("\n")[0].strip()
         desc = f"search_chats: {query[:80]}"
         result = await do_search_chats(query, owner=owner)
-    elif tool in ("chat_with_model", "create_session", "list_sessions",
+    elif tool in ("chat_with_model", "ask_teacher", "list_models"):
+        # Migrated to the agent_tools registry (#3629): dispatched through
+        # TOOL_HANDLERS with the owner/session ctx these tools need, instead
+        # of the legacy dispatch_ai_tool elif. The do_* impls stay in
+        # ai_interaction.py (dispatch_ai_tool + the owner-scope test use them).
+        first_line = content.split(chr(10))[0].strip()[:60]
+        desc = f"{tool}: {first_line}" if first_line else tool
+        result = await _document_tool_dispatch(tool, content, session_id, owner) \
+            or {"error": f"{tool}: execution failed", "exit_code": 1}
+    elif tool in ("create_session", "list_sessions",
                   "send_to_session", "pipeline",
-                  "manage_session", "manage_memory", "list_models",
-                  "ui_control", "ask_teacher"):
+                  "manage_session", "manage_memory",
+                  "ui_control"):
         from src.ai_interaction import dispatch_ai_tool
         desc, result = await dispatch_ai_tool(tool, content, session_id, owner=owner)
     elif tool == "manage_tasks":

static/js/admin.js

@@ -1756,7 +1756,6 @@ const TOOL_META = {
   manage_skills:     { name: 'Skills',           desc: 'Learn and use procedures',        cat: 'Knowledge',  ctx: '~200' },
   manage_rag:        { name: 'RAG / Docs',       desc: 'Query indexed documents',         cat: 'Knowledge',  ctx: '~150' },
   chat_with_model:   { name: 'Chat with Model',  desc: 'Talk to another AI model',        cat: 'Multi-Agent', ctx: '~200' },
-  second_opinion:    { name: 'Second Opinion',   desc: 'Get another model\'s take',       cat: 'Multi-Agent', ctx: '~150' },
   pipeline:          { name: 'Pipeline',         desc: 'Multi-step AI workflows',         cat: 'Multi-Agent', ctx: '~200' },
   ask_teacher:       { name: 'Ask Teacher',      desc: 'Query a more capable model',      cat: 'Multi-Agent', ctx: '~150' },
   send_to_session:   { name: 'Send to Session',  desc: 'Send message to another chat',    cat: 'Sessions',   ctx: '~100' },

static/js/assistant.js

@@ -125,7 +125,7 @@ const TOOL_GROUPS = {
   'Knowledge': ['web_search', 'read_file', 'manage_memory', 'manage_rag', 'search_chats'],
   'Code': ['bash', 'python', 'write_file'],
   'Documents': ['create_document', 'edit_document', 'update_document', 'suggest_document'],
-  'AI & Models': ['chat_with_model', 'second_opinion', 'ask_teacher', 'pipeline', 'list_models', 'generate_image'],
+  'AI & Models': ['chat_with_model', 'ask_teacher', 'pipeline', 'list_models', 'generate_image'],
   'System': ['manage_session', 'manage_endpoints', 'manage_mcp', 'manage_settings', 'manage_skills', 'manage_webhooks', 'manage_tokens', 'manage_documents', 'create_session', 'list_sessions', 'send_to_session', 'ui_control'],
 };
 

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_document_render_pdf_iframe.py

@@ -0,0 +1,238 @@
+"""Regression tests for the document PDF preview framing headers and PyMuPDF dependency handling."""
+
+import builtins
+import tempfile
+import uuid
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from sqlalchemy import create_engine
+from sqlalchemy.orm import sessionmaker
+from sqlalchemy.pool import NullPool
+
+import core.database as cdb
+import routes.document_routes as droutes
+from core.database import Document
+from core.middleware import SecurityHeadersMiddleware
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+class _FakeURL:
+    def __init__(self, path: str):
+        self.path = path
+        self.scheme = "http"
+
+
+class _FakeRequest:
+    def __init__(self, path: str):
+        self.url = _FakeURL(path)
+        self.headers = {}
+        self.state = SimpleNamespace()
+
+
+class _FakeResponse:
+    def __init__(self):
+        self.headers: dict[str, str] = {}
+
+
+async def _dispatch(path: str) -> _FakeResponse:
+    mw = SecurityHeadersMiddleware(MagicMock())
+    resp = _FakeResponse()
+    call_next = AsyncMock(return_value=resp)
+    await mw.dispatch(_FakeRequest(path), call_next)
+    return resp
+
+
+# ---------------------------------------------------------------------------
+# Test 1: middleware framing policy on /api/document/.../render-pdf
+# ---------------------------------------------------------------------------
+
+
+async def test_doc_render_pdf_same_origin_framing():
+    """Assert that /api/document/{id}/render-pdf allows same-origin framing."""
+    resp = await _dispatch("/api/document/abc-123/render-pdf")
+
+    assert resp.headers.get("X-Frame-Options") == "SAMEORIGIN"
+    csp = resp.headers.get("Content-Security-Policy", "")
+    assert "frame-ancestors 'self'" in csp
+
+
+async def test_doc_render_pdf_keeps_baseline_security_headers():
+    """Assert that baseline security headers are preserved on the render-pdf path."""
+    resp = await _dispatch("/api/document/abc-123/render-pdf")
+
+    assert resp.headers.get("X-Content-Type-Options") == "nosniff"
+    assert resp.headers.get("Referrer-Policy") == "no-referrer"
+
+
+async def test_doc_export_pdf_still_frame_blocked():
+    """Assert that the export-pdf path remains frame-blocked."""
+    resp = await _dispatch("/api/document/abc-123/export-pdf")
+
+    assert resp.headers.get("X-Frame-Options") == "DENY"
+    assert "frame-ancestors 'none'" in resp.headers.get("Content-Security-Policy", "")
+
+
+async def test_doc_path_matching_is_precise():
+    """Assert that similar paths are not exempted from framing restrictions."""
+    for path in [
+        "/api/document/abc-123/render-pdfx",
+        "/api/document/abc-123/render-pdf/foo",
+        "/api/documents/abc-123/render-pdf",
+    ]:
+        resp = await _dispatch(path)
+        assert resp.headers.get("X-Frame-Options") == "DENY"
+
+
+async def test_tool_render_exemption_preserved():
+    """Assert that the tool-render path remains exempt from framing headers."""
+    resp = await _dispatch("/api/tools/foo/bar/render")
+
+    assert "X-Frame-Options" not in resp.headers
+    csp = resp.headers.get("Content-Security-Policy", "")
+    assert "frame-ancestors" not in csp
+
+
+async def test_unrelated_paths_keep_strict_policy():
+    """Assert that other paths keep the strict framing policy."""
+    resp = await _dispatch("/api/chat")
+
+    assert resp.headers.get("X-Frame-Options") == "DENY"
+    csp = resp.headers.get("Content-Security-Policy", "")
+    assert "frame-ancestors 'none'" in csp
+
+
+# ---------------------------------------------------------------------------
+# Test 2: render-pdf route must return 503 (not 500) when PyMuPDF is missing
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def test_db(monkeypatch):
+    """Create a temporary SQLite database and patch routes.document_routes.SessionLocal."""
+    import os
+    tmpdb = tempfile.NamedTemporaryFile(suffix=".db", delete=False)
+    tmpdb.close()
+    engine = create_engine(
+        f"sqlite:///{tmpdb.name}",
+        connect_args={"check_same_thread": False},
+        poolclass=NullPool,
+    )
+    cdb.Base.metadata.create_all(engine)
+    ts = sessionmaker(bind=engine, autoflush=False, autocommit=False)
+    monkeypatch.setattr(droutes, "SessionLocal", ts)
+    try:
+        yield ts
+    finally:
+        engine.dispose()
+        try:
+            os.unlink(tmpdb.name)
+        except OSError:
+            pass
+
+
+def _req():
+    """Minimal request stub."""
+    return SimpleNamespace(
+        state=SimpleNamespace(current_user="tester"),
+        app=SimpleNamespace(state=SimpleNamespace(auth_manager=None)),
+    )
+
+
+def _endpoint(method: str, path: str, upload_handler=None):
+    router = droutes.setup_document_routes(MagicMock(), upload_handler)
+    for r in router.routes:
+        if getattr(r, "path", None) == path and method in getattr(r, "methods", set()):
+            return r.endpoint
+    raise RuntimeError(f"{method} {path} not found")
+
+
+def _make_pdf_doc(db_session) -> str:
+    """Create a test Document with a pdf_form_source front-matter pointer."""
+    content = (
+        '<!-- pdf_form_source upload_id="'
+        + "a" * 32
+        + '" fields="3" -->\n'
+        "- Field 1: value1\n- Field 2: value2\n- Field 3: value3\n"
+    )
+    db = db_session()
+    try:
+        doc = Document(
+            id=str(uuid.uuid4()),
+            session_id=None,
+            title="t",
+            language="markdown",
+            current_content=content,
+            version_count=1,
+            is_active=True,
+            owner="tester",
+        )
+        db.add(doc)
+        db.commit()
+        return doc.id
+    finally:
+        db.close()
+
+
+async def test_render_pdf_returns_503_when_pymupdf_missing(monkeypatch, test_db):
+    """Assert that the render-pdf path returns 503 when PyMuPDF is not installed."""
+    real_import = builtins.__import__
+
+    def fake_import(name, *args, **kwargs):
+        if name == "fitz":
+            raise ImportError("No module named 'fitz'")
+        return real_import(name, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "__import__", fake_import)
+
+    # Stub route dependencies to isolate the PyMuPDF check
+    import src.pdf_form_doc as pdf_form_doc
+    monkeypatch.setattr(pdf_form_doc, "find_source_upload_id", lambda _content: "a" * 32)
+    monkeypatch.setattr(droutes, "_resolve_user_upload_path", lambda *a, **kw: "/tmp/fake.pdf")
+
+    render_pdf = _endpoint("GET", "/api/document/{doc_id}/render-pdf", upload_handler=MagicMock())
+    doc_id = _make_pdf_doc(test_db)
+
+    from fastapi import HTTPException
+    with pytest.raises(HTTPException) as excinfo:
+        await render_pdf(doc_id, _req())
+
+    assert excinfo.value.status_code == 503
+    detail = str(excinfo.value.detail)
+    assert "requirements-optional.txt" in detail
+    assert "PyMuPDF" in detail
+
+
+async def test_render_pdf_503_runs_before_file_io(monkeypatch, test_db, tmp_path):
+    """Assert that the PyMuPDF check runs before resolving or checking the source file path."""
+    real_import = builtins.__import__
+
+    def fake_import(name, *args, **kwargs):
+        if name == "fitz":
+            raise ImportError("No module named 'fitz'")
+        return real_import(name, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "__import__", fake_import)
+
+    # Use a non-existent path to verify the check fails before checking path existence
+    sentinel_dir = tmp_path / "should-never-be-touched"
+    sentinel_dir.mkdir()
+    sentinel_path = str(sentinel_dir / "source.pdf")
+
+    import src.pdf_form_doc as pdf_form_doc
+    monkeypatch.setattr(pdf_form_doc, "find_source_upload_id", lambda _content: "a" * 32)
+    monkeypatch.setattr(droutes, "_resolve_user_upload_path", lambda *a, **kw: sentinel_path)
+
+    render_pdf = _endpoint("GET", "/api/document/{doc_id}/render-pdf", upload_handler=MagicMock())
+    doc_id = _make_pdf_doc(test_db)
+
+    from fastapi import HTTPException
+    with pytest.raises(HTTPException) as excinfo:
+        await render_pdf(doc_id, _req())
+
+    assert excinfo.value.status_code == 503

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"